From 04b9a7e7a22d968790aeee9c316391252b0aaf67 Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Mon, 5 Dec 2016 18:48:51 -0500 Subject: [PATCH] Bug fixes for repo permissions in API Also move duplicated code into repo.APIFormat(..) --- models/action.go | 2 +- models/issue.go | 12 ++++++------ models/pull.go | 12 ++++++------ models/repo.go | 7 ++++++- routers/api/v1/repo/repo.go | 26 ++++++++++++++++---------- routers/api/v1/user/star.go | 10 +++++++++- routers/repo/webhook.go | 2 +- 7 files changed, 45 insertions(+), 26 deletions(-) diff --git a/models/action.go b/models/action.go index e2ac1756a..9c79dc5df 100644 --- a/models/action.go +++ b/models/action.go @@ -539,7 +539,7 @@ func CommitRepoAction(opts CommitRepoActionOptions) error { }() apiPusher := pusher.APIFormat() - apiRepo := repo.APIFormat(nil) + apiRepo := repo.APIFormat(AccessModeNone) var shaSum string switch opType { diff --git a/models/issue.go b/models/issue.go index 7ef3d00e4..4937bf1b5 100644 --- a/models/issue.go +++ b/models/issue.go @@ -265,7 +265,7 @@ func (issue *Issue) sendLabelUpdatedWebhook(doer *User) { Action: api.HookIssueLabelUpdated, Index: issue.Index, PullRequest: issue.PullRequest.APIFormat(), - Repository: issue.Repo.APIFormat(nil), + Repository: issue.Repo.APIFormat(AccessModeNone), Sender: doer.APIFormat(), }) } @@ -371,7 +371,7 @@ func (issue *Issue) ClearLabels(doer *User) (err error) { Action: api.HookIssueLabelCleared, Index: issue.Index, PullRequest: issue.PullRequest.APIFormat(), - Repository: issue.Repo.APIFormat(nil), + Repository: issue.Repo.APIFormat(AccessModeNone), Sender: doer.APIFormat(), }) } @@ -493,7 +493,7 @@ func (issue *Issue) ChangeStatus(doer *User, repo *Repository, isClosed bool) (e apiPullRequest := &api.PullRequestPayload{ Index: issue.Index, PullRequest: issue.PullRequest.APIFormat(), - Repository: repo.APIFormat(nil), + Repository: repo.APIFormat(AccessModeNone), Sender: doer.APIFormat(), } if isClosed { @@ -531,7 +531,7 @@ func (issue *Issue) ChangeTitle(doer *User, title string) (err error) { }, }, PullRequest: issue.PullRequest.APIFormat(), - Repository: issue.Repo.APIFormat(nil), + Repository: issue.Repo.APIFormat(AccessModeNone), Sender: doer.APIFormat(), }) } @@ -563,7 +563,7 @@ func (issue *Issue) ChangeContent(doer *User, content string) (err error) { }, }, PullRequest: issue.PullRequest.APIFormat(), - Repository: issue.Repo.APIFormat(nil), + Repository: issue.Repo.APIFormat(AccessModeNone), Sender: doer.APIFormat(), }) } @@ -596,7 +596,7 @@ func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (err error) { apiPullRequest := &api.PullRequestPayload{ Index: issue.Index, PullRequest: issue.PullRequest.APIFormat(), - Repository: issue.Repo.APIFormat(nil), + Repository: issue.Repo.APIFormat(AccessModeNone), Sender: doer.APIFormat(), } if isRemoveAssignee { diff --git a/models/pull.go b/models/pull.go index d149a142d..b7a1ce5d2 100644 --- a/models/pull.go +++ b/models/pull.go @@ -160,14 +160,14 @@ func (pr *PullRequest) APIFormat() *api.PullRequest { Ref: pr.BaseBranch, Sha: baseCommit.ID.String(), RepoID: pr.BaseRepoID, - Repository: pr.BaseRepo.APIFormat(nil), + Repository: pr.BaseRepo.APIFormat(AccessModeNone), } apiHeadBranchInfo := &api.PRBranchInfo{ Name: pr.HeadBranch, Ref: pr.HeadBranch, Sha: headCommit.ID.String(), RepoID: pr.HeadRepoID, - Repository: pr.HeadRepo.APIFormat(nil), + Repository: pr.HeadRepo.APIFormat(AccessModeNone), } apiPullRequest := &api.PullRequest{ ID: pr.ID, @@ -355,7 +355,7 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository) (err error Action: api.HookIssueClosed, Index: pr.Index, PullRequest: pr.APIFormat(), - Repository: pr.Issue.Repo.APIFormat(nil), + Repository: pr.Issue.Repo.APIFormat(AccessModeNone), Sender: doer.APIFormat(), }); err != nil { log.Error(4, "PrepareWebhooks: %v", err) @@ -385,7 +385,7 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository) (err error After: pr.MergedCommitID, CompareURL: setting.AppURL + pr.BaseRepo.ComposeCompareURL(pr.MergeBase, pr.MergedCommitID), Commits: ListToPushCommits(l).ToAPIPayloadCommits(pr.BaseRepo.HTMLURL()), - Repo: pr.BaseRepo.APIFormat(nil), + Repo: pr.BaseRepo.APIFormat(AccessModeNone), Pusher: pr.HeadRepo.MustOwner().APIFormat(), Sender: doer.APIFormat(), } @@ -514,7 +514,7 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str Action: api.HookIssueOpened, Index: pull.Index, PullRequest: pr.APIFormat(), - Repository: repo.APIFormat(nil), + Repository: repo.APIFormat(AccessModeNone), Sender: pull.Poster.APIFormat(), }); err != nil { log.Error(4, "PrepareWebhooks: %v", err) @@ -840,7 +840,7 @@ func AddTestPullRequestTask(doer *User, repoID int64, branch string, isSync bool Action: api.HookIssueSynchronized, Index: pr.Issue.Index, PullRequest: pr.Issue.PullRequest.APIFormat(), - Repository: pr.Issue.Repo.APIFormat(nil), + Repository: pr.Issue.Repo.APIFormat(AccessModeNone), Sender: doer.APIFormat(), }); err != nil { log.Error(4, "PrepareWebhooks [pull_id: %v]: %v", pr.ID, err) diff --git a/models/repo.go b/models/repo.go index af74e6c8f..f339234fb 100644 --- a/models/repo.go +++ b/models/repo.go @@ -277,8 +277,13 @@ func (repo *Repository) HTMLURL() string { // APIFormat converts a Repository to api.Repository // Arguments that are allowed to be nil: permission -func (repo *Repository) APIFormat(permission *api.Permission) *api.Repository { +func (repo *Repository) APIFormat(mode AccessMode) *api.Repository { cloneLink := repo.CloneLink() + permission := &api.Permission{ + Admin: mode >= AccessModeAdmin, + Push: mode >= AccessModeWrite, + Pull: mode >= AccessModeRead, + } return &api.Repository{ ID: repo.ID, Owner: repo.Owner.APIFormat(), diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 0c5c1ef5f..35e655427 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -95,16 +95,12 @@ func ListMyRepos(ctx *context.APIContext) { repos := make([]*api.Repository, numOwnRepos+len(accessibleRepos)) for i := range ownRepos { - repos[i] = ownRepos[i].APIFormat(&api.Permission{true, true, true}) + repos[i] = ownRepos[i].APIFormat(models.AccessModeOwner) } i := numOwnRepos for repo, access := range accessibleRepos { - repos[i] = repo.APIFormat(&api.Permission{ - Admin: access >= models.AccessModeAdmin, - Push: access >= models.AccessModeWrite, - Pull: true, - }) + repos[i] = repo.APIFormat(access) i++ } @@ -138,7 +134,7 @@ func CreateUserRepo(ctx *context.APIContext, owner *models.User, opt api.CreateR return } - ctx.JSON(201, repo.APIFormat(&api.Permission{true, true, true})) + ctx.JSON(201, repo.APIFormat(models.AccessModeOwner)) } // Create one repository of mine @@ -241,14 +237,19 @@ func Migrate(ctx *context.APIContext, form auth.MigrateRepoForm) { } log.Trace("Repository migrated: %s/%s", ctxUser.Name, form.RepoName) - ctx.JSON(201, repo.APIFormat(&api.Permission{true, true, true})) + ctx.JSON(201, repo.APIFormat(models.AccessModeAdmin)) } // Get one repository // see https://github.com/gogits/go-gogs-client/wiki/Repositories#get func Get(ctx *context.APIContext) { repo := ctx.Repo.Repository - ctx.JSON(200, repo.APIFormat(&api.Permission{true, true, true})) + access, err := models.AccessLevel(ctx.User, repo) + if err != nil { + ctx.Error(500, "GetRepository", err) + return + } + ctx.JSON(200, repo.APIFormat(access)) } // GetByID returns a single Repository @@ -263,7 +264,12 @@ func GetByID(ctx *context.APIContext) { return } - ctx.JSON(200, repo.APIFormat(&api.Permission{true, true, true})) + access, err := models.AccessLevel(ctx.User, repo) + if err != nil { + ctx.Error(500, "GetRepositoryByID", err) + return + } + ctx.JSON(200, repo.APIFormat(access)) } // Delete one repository diff --git a/routers/api/v1/user/star.go b/routers/api/v1/user/star.go index c856ecefd..0937fd190 100644 --- a/routers/api/v1/user/star.go +++ b/routers/api/v1/user/star.go @@ -18,9 +18,17 @@ func getStarredRepos(userID int64, private bool) ([]*api.Repository, error) { if err != nil { return nil, err } + user, err := models.GetUserByID(userID) + if err != nil { + return nil, err + } repos := make([]*api.Repository, len(starredRepos)) for i, starred := range starredRepos { - repos[i] = starred.APIFormat(&api.Permission{true, true, true}) + access, err := models.AccessLevel(user, starred) + if err != nil { + return nil, err + } + repos[i] = starred.APIFormat(access) } return repos, nil } diff --git a/routers/repo/webhook.go b/routers/repo/webhook.go index 5a4770cdf..c3150a02f 100644 --- a/routers/repo/webhook.go +++ b/routers/repo/webhook.go @@ -388,7 +388,7 @@ func TestWebhook(ctx *context.Context) { }, }, }, - Repo: ctx.Repo.Repository.APIFormat(nil), + Repo: ctx.Repo.Repository.APIFormat(models.AccessModeNone), Pusher: apiUser, Sender: apiUser, }