From 93a1de48428c2f89502cf6bba80f3976c6f5839f Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Tue, 11 Jul 2017 21:23:41 -0400 Subject: [PATCH] Fix repo API bug (#2133) Don't require token when not necessary --- integrations/api_fork_test.go | 18 ++++++++ integrations/api_keys_test.go | 39 +++++++++++++++++ integrations/api_repo_test.go | 12 ++++++ routers/api/v1/api.go | 76 +++++++++++++++++++--------------- routers/api/v1/repo/fork.go | 3 +- routers/api/v1/repo/release.go | 8 +--- routers/api/v1/repo/repo.go | 8 +--- routers/api/v1/repo/status.go | 7 +--- routers/api/v1/utils/utils.go | 15 +++++++ 9 files changed, 132 insertions(+), 54 deletions(-) create mode 100644 integrations/api_fork_test.go create mode 100644 integrations/api_keys_test.go create mode 100644 routers/api/v1/utils/utils.go diff --git a/integrations/api_fork_test.go b/integrations/api_fork_test.go new file mode 100644 index 0000000000..2c5eb55166 --- /dev/null +++ b/integrations/api_fork_test.go @@ -0,0 +1,18 @@ +// Copyright 2017 The Gogs Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package integrations + +import ( + "net/http" + "testing" + + api "code.gitea.io/sdk/gitea" +) + +func TestCreateForkNoLogin(t *testing.T) { + prepareTestEnv(t) + req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/forks", &api.CreateForkOption{}) + MakeRequest(t, req, http.StatusUnauthorized) +} diff --git a/integrations/api_keys_test.go b/integrations/api_keys_test.go new file mode 100644 index 0000000000..8e2b6d3fbf --- /dev/null +++ b/integrations/api_keys_test.go @@ -0,0 +1,39 @@ +// Copyright 2017 The Gogs Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package integrations + +import ( + "net/http" + "testing" + + api "code.gitea.io/sdk/gitea" +) + +func TestViewDeployKeysNoLogin(t *testing.T) { + prepareTestEnv(t) + req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/keys") + MakeRequest(t, req, http.StatusUnauthorized) +} + +func TestCreateDeployKeyNoLogin(t *testing.T) { + prepareTestEnv(t) + req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/keys", api.CreateKeyOption{ + Title: "title", + Key: "key", + }) + MakeRequest(t, req, http.StatusUnauthorized) +} + +func TestGetDeployKeyNoLogin(t *testing.T) { + prepareTestEnv(t) + req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/keys/1") + MakeRequest(t, req, http.StatusUnauthorized) +} + +func TestDeleteDeployKeyNoLogin(t *testing.T) { + prepareTestEnv(t) + req := NewRequest(t, "DELETE", "/api/v1/repos/user2/repo1/keys/1") + MakeRequest(t, req, http.StatusUnauthorized) +} diff --git a/integrations/api_repo_test.go b/integrations/api_repo_test.go index 8f17fce391..8073f773ac 100644 --- a/integrations/api_repo_test.go +++ b/integrations/api_repo_test.go @@ -51,3 +51,15 @@ func TestAPISearchRepoNotLogin(t *testing.T) { assert.True(t, strings.Contains(repo.Name, keyword)) } } + +func TestAPIViewRepo(t *testing.T) { + prepareTestEnv(t) + + req := NewRequest(t, "GET", "/api/v1/repos/user2/repo1") + resp := MakeRequest(t, req, http.StatusOK) + + var repo api.Repository + DecodeJSON(t, resp, &repo) + assert.EqualValues(t, 1, repo.ID) + assert.EqualValues(t, "repo1", repo.Name) +} diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 14a8d59855..8dda892955 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -42,6 +42,7 @@ import ( "code.gitea.io/gitea/routers/api/v1/org" "code.gitea.io/gitea/routers/api/v1/repo" "code.gitea.io/gitea/routers/api/v1/user" + "code.gitea.io/gitea/routers/api/v1/utils" ) func repoAssignment() macaron.Handler { @@ -92,7 +93,7 @@ func repoAssignment() macaron.Handler { if ctx.IsSigned && ctx.User.IsAdmin { ctx.Repo.AccessMode = models.AccessModeOwner } else { - mode, err := models.AccessLevel(ctx.User.ID, repo) + mode, err := models.AccessLevel(utils.UserID(ctx), repo) if err != nil { ctx.Error(500, "AccessLevel", err) return @@ -341,27 +342,27 @@ func RegisterRoutes(m *macaron.Macaron) { m.Combo("/repositories/:id", reqToken()).Get(repo.GetByID) m.Group("/repos", func() { - m.Post("/migrate", bind(auth.MigrateRepoForm{}), repo.Migrate) + m.Post("/migrate", reqToken(), bind(auth.MigrateRepoForm{}), repo.Migrate) m.Group("/:username/:reponame", func() { - m.Combo("").Get(repo.Get).Delete(repo.Delete) + m.Combo("").Get(repo.Get).Delete(reqToken(), repo.Delete) m.Group("/hooks", func() { m.Combo("").Get(repo.ListHooks). Post(bind(api.CreateHookOption{}), repo.CreateHook) m.Combo("/:id").Get(repo.GetHook). Patch(bind(api.EditHookOption{}), repo.EditHook). Delete(repo.DeleteHook) - }, reqRepoWriter()) + }, reqToken(), reqRepoWriter()) m.Group("/collaborators", func() { m.Get("", repo.ListCollaborators) m.Combo("/:collaborator").Get(repo.IsCollaborator). Put(bind(api.AddCollaboratorOption{}), repo.AddCollaborator). Delete(repo.DeleteCollaborator) - }) + }, reqToken()) m.Get("/raw/*", context.RepoRef(), repo.GetRawFile) m.Get("/archive/*", repo.GetArchive) m.Combo("/forks").Get(repo.ListForks). - Post(bind(api.CreateForkOption{}), repo.CreateFork) + Post(reqToken(), bind(api.CreateForkOption{}), repo.CreateFork) m.Group("/branches", func() { m.Get("", repo.ListBranches) m.Get("/*", context.RepoRef(), repo.GetBranch) @@ -371,78 +372,87 @@ func RegisterRoutes(m *macaron.Macaron) { Post(bind(api.CreateKeyOption{}), repo.CreateDeployKey) m.Combo("/:id").Get(repo.GetDeployKey). Delete(repo.DeleteDeploykey) - }) + }, reqToken()) m.Group("/issues", func() { - m.Combo("").Get(repo.ListIssues).Post(bind(api.CreateIssueOption{}), repo.CreateIssue) + m.Combo("").Get(repo.ListIssues). + Post(reqToken(), bind(api.CreateIssueOption{}), repo.CreateIssue) m.Group("/comments", func() { m.Get("", repo.ListRepoIssueComments) - m.Combo("/:id").Patch(bind(api.EditIssueCommentOption{}), repo.EditIssueComment) + m.Combo("/:id", reqToken()). + Patch(bind(api.EditIssueCommentOption{}), repo.EditIssueComment) }) m.Group("/:index", func() { - m.Combo("").Get(repo.GetIssue).Patch(bind(api.EditIssueOption{}), repo.EditIssue) + m.Combo("").Get(repo.GetIssue). + Patch(reqToken(), bind(api.EditIssueOption{}), repo.EditIssue) m.Group("/comments", func() { - m.Combo("").Get(repo.ListIssueComments).Post(bind(api.CreateIssueCommentOption{}), repo.CreateIssueComment) - m.Combo("/:id").Patch(bind(api.EditIssueCommentOption{}), repo.EditIssueComment). + m.Combo("").Get(repo.ListIssueComments). + Post(reqToken(), bind(api.CreateIssueCommentOption{}), repo.CreateIssueComment) + m.Combo("/:id", reqToken()).Patch(bind(api.EditIssueCommentOption{}), repo.EditIssueComment). Delete(repo.DeleteIssueComment) }) m.Group("/labels", func() { m.Combo("").Get(repo.ListIssueLabels). - Post(bind(api.IssueLabelsOption{}), repo.AddIssueLabels). - Put(bind(api.IssueLabelsOption{}), repo.ReplaceIssueLabels). - Delete(repo.ClearIssueLabels) - m.Delete("/:id", repo.DeleteIssueLabel) + Post(reqToken(), bind(api.IssueLabelsOption{}), repo.AddIssueLabels). + Put(reqToken(), bind(api.IssueLabelsOption{}), repo.ReplaceIssueLabels). + Delete(reqToken(), repo.ClearIssueLabels) + m.Delete("/:id", reqToken(), repo.DeleteIssueLabel) }) }) }, mustEnableIssues) m.Group("/labels", func() { m.Combo("").Get(repo.ListLabels). - Post(bind(api.CreateLabelOption{}), repo.CreateLabel) - m.Combo("/:id").Get(repo.GetLabel).Patch(bind(api.EditLabelOption{}), repo.EditLabel). - Delete(repo.DeleteLabel) + Post(reqToken(), bind(api.CreateLabelOption{}), repo.CreateLabel) + m.Combo("/:id").Get(repo.GetLabel). + Patch(reqToken(), bind(api.EditLabelOption{}), repo.EditLabel). + Delete(reqToken(), repo.DeleteLabel) }) m.Group("/milestones", func() { m.Combo("").Get(repo.ListMilestones). - Post(reqRepoWriter(), bind(api.CreateMilestoneOption{}), repo.CreateMilestone) + Post(reqToken(), reqRepoWriter(), bind(api.CreateMilestoneOption{}), repo.CreateMilestone) m.Combo("/:id").Get(repo.GetMilestone). - Patch(reqRepoWriter(), bind(api.EditMilestoneOption{}), repo.EditMilestone). - Delete(reqRepoWriter(), repo.DeleteMilestone) + Patch(reqToken(), reqRepoWriter(), bind(api.EditMilestoneOption{}), repo.EditMilestone). + Delete(reqToken(), reqRepoWriter(), repo.DeleteMilestone) }) m.Get("/stargazers", repo.ListStargazers) m.Get("/subscribers", repo.ListSubscribers) m.Group("/subscription", func() { m.Get("", user.IsWatching) - m.Put("", user.Watch) - m.Delete("", user.Unwatch) + m.Put("", reqToken(), user.Watch) + m.Delete("", reqToken(), user.Unwatch) }) m.Group("/releases", func() { m.Combo("").Get(repo.ListReleases). - Post(bind(api.CreateReleaseOption{}), repo.CreateRelease) + Post(reqToken(), bind(api.CreateReleaseOption{}), repo.CreateRelease) m.Combo("/:id").Get(repo.GetRelease). - Patch(bind(api.EditReleaseOption{}), repo.EditRelease). - Delete(repo.DeleteRelease) + Patch(reqToken(), bind(api.EditReleaseOption{}), repo.EditRelease). + Delete(reqToken(), repo.DeleteRelease) }) - m.Post("/mirror-sync", repo.MirrorSync) + m.Post("/mirror-sync", reqToken(), repo.MirrorSync) m.Get("/editorconfig/:filename", context.RepoRef(), repo.GetEditorconfig) m.Group("/pulls", func() { - m.Combo("").Get(bind(api.ListPullRequestsOptions{}), repo.ListPullRequests).Post(reqRepoWriter(), bind(api.CreatePullRequestOption{}), repo.CreatePullRequest) + m.Combo("").Get(bind(api.ListPullRequestsOptions{}), repo.ListPullRequests). + Post(reqToken(), reqRepoWriter(), bind(api.CreatePullRequestOption{}), repo.CreatePullRequest) m.Group("/:index", func() { - m.Combo("").Get(repo.GetPullRequest).Patch(reqRepoWriter(), bind(api.EditPullRequestOption{}), repo.EditPullRequest) - m.Combo("/merge").Get(repo.IsPullRequestMerged).Post(reqRepoWriter(), repo.MergePullRequest) + m.Combo("").Get(repo.GetPullRequest). + Patch(reqToken(), reqRepoWriter(), bind(api.EditPullRequestOption{}), repo.EditPullRequest) + m.Combo("/merge").Get(repo.IsPullRequestMerged). + Post(reqToken(), reqRepoWriter(), repo.MergePullRequest) }) }, mustAllowPulls, context.ReferencesGitRepo()) m.Group("/statuses", func() { - m.Combo("/:sha").Get(repo.GetCommitStatuses).Post(reqRepoWriter(), bind(api.CreateStatusOption{}), repo.NewCommitStatus) + m.Combo("/:sha").Get(repo.GetCommitStatuses). + Post(reqToken(), reqRepoWriter(), bind(api.CreateStatusOption{}), repo.NewCommitStatus) }) m.Group("/commits/:ref", func() { m.Get("/status", repo.GetCombinedCommitStatus) m.Get("/statuses", repo.GetCommitStatuses) }) }, repoAssignment()) - }, reqToken()) + }) // Organizations m.Get("/user/orgs", reqToken(), org.ListMyOrgs) diff --git a/routers/api/v1/repo/fork.go b/routers/api/v1/repo/fork.go index 44b79a6fef..25464dbd78 100644 --- a/routers/api/v1/repo/fork.go +++ b/routers/api/v1/repo/fork.go @@ -9,6 +9,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/routers/api/v1/utils" ) // ListForks list a repository's forks @@ -29,7 +30,7 @@ func ListForks(ctx *context.APIContext) { } apiForks := make([]*api.Repository, len(forks)) for i, fork := range forks { - access, err := models.AccessLevel(ctx.User.ID, fork) + access, err := models.AccessLevel(utils.UserID(ctx), fork) if err != nil { ctx.Error(500, "AccessLevel", err) return diff --git a/routers/api/v1/repo/release.go b/routers/api/v1/repo/release.go index ed5b8f4f78..302be8dbab 100644 --- a/routers/api/v1/repo/release.go +++ b/routers/api/v1/repo/release.go @@ -34,14 +34,8 @@ func GetRelease(ctx *context.APIContext) { // ListReleases list a repository's releases func ListReleases(ctx *context.APIContext) { - access, err := models.AccessLevel(ctx.User.ID, ctx.Repo.Repository) - if err != nil { - ctx.Error(500, "AccessLevel", err) - return - } - releases, err := models.GetReleasesByRepoID(ctx.Repo.Repository.ID, models.FindReleasesOptions{ - IncludeDrafts: access >= models.AccessModeWrite, + IncludeDrafts: ctx.Repo.AccessMode >= models.AccessModeWrite, }, 1, 2147483647) if err != nil { ctx.Error(500, "GetReleasesByRepoID", err) diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 7fb828ddbc..178f1005e5 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -267,13 +267,7 @@ func Get(ctx *context.APIContext) { // 200: Repository // 500: error - repo := ctx.Repo.Repository - access, err := models.AccessLevel(ctx.User.ID, repo) - if err != nil { - ctx.Error(500, "GetRepository", err) - return - } - ctx.JSON(200, repo.APIFormat(access)) + ctx.JSON(200, ctx.Repo.Repository.APIFormat(ctx.Repo.AccessMode)) } // GetByID returns a single Repository diff --git a/routers/api/v1/repo/status.go b/routers/api/v1/repo/status.go index d9b101df05..e4cc20a50b 100644 --- a/routers/api/v1/repo/status.go +++ b/routers/api/v1/repo/status.go @@ -103,15 +103,10 @@ func GetCombinedCommitStatus(ctx *context.APIContext) { return } - acl, err := models.AccessLevel(ctx.User.ID, repo) - if err != nil { - ctx.Error(500, "AccessLevel", fmt.Errorf("AccessLevel[%d, %s]: %v", ctx.User.ID, repo.FullName(), err)) - return - } retStatus := &combinedCommitStatus{ SHA: sha, TotalCount: len(statuses), - Repo: repo.APIFormat(acl), + Repo: repo.APIFormat(ctx.Repo.AccessMode), URL: "", } diff --git a/routers/api/v1/utils/utils.go b/routers/api/v1/utils/utils.go new file mode 100644 index 0000000000..f7c2b224d5 --- /dev/null +++ b/routers/api/v1/utils/utils.go @@ -0,0 +1,15 @@ +// Copyright 2017 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package utils + +import "code.gitea.io/gitea/modules/context" + +// UserID user ID of authenticated user, or 0 if not authenticated +func UserID(ctx *context.APIContext) int64 { + if ctx.User == nil { + return 0 + } + return ctx.User.ID +}