Bug fixes for repo permissions in API

ethantkoenig · bkcsoft · commit 04b9a7e7a22d · 2016-12-07T12:55:24.000+01:00
Also move duplicated code into repo.APIFormat(..)
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,
 	}

Original file line number	Diff line number	Diff line change
`@@ -18,9 +18,17 @@ func getStarredRepos(userID int64, private bool) ([]*api.Repository, error) {`
`18`	`18`	`if err != nil {`
`19`	`19`	`return nil, err`
`20`	`20`	`}`
	`21`	`+ user, err := models.GetUserByID(userID)`
	`22`	`+ if err != nil {`
	`23`	`+ return nil, err`
	`24`	`+ }`
`21`	`25`	`repos := make([]*api.Repository, len(starredRepos))`
`22`	`26`	`for i, starred := range starredRepos {`
`23`		`- repos[i] = starred.APIFormat(&api.Permission{true, true, true})`
	`27`	`+ access, err := models.AccessLevel(user, starred)`
	`28`	`+ if err != nil {`
	`29`	`+ return nil, err`
	`30`	`+ }`
	`31`	`+ repos[i] = starred.APIFormat(access)`
`24`	`32`	`}`
`25`	`33`	`return repos, nil`
`26`	`34`	`}`
Original file line number	Diff line number	Diff line change
`@@ -388,7 +388,7 @@ func TestWebhook(ctx *context.Context) {`
`388`	`388`	`},`
`389`	`389`	`},`
`390`	`390`	`},`
`391`		`- Repo: ctx.Repo.Repository.APIFormat(nil),`
	`391`	`+ Repo: ctx.Repo.Repository.APIFormat(models.AccessModeNone),`
`392`	`392`	`Pusher: apiUser,`
`393`	`393`	`Sender: apiUser,`
`394`	`394`	`}`