From 0bf41c2d5042b4d91259321876c61fb060f228e5 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sat, 10 Nov 2018 19:45:31 +0800 Subject: [PATCH 01/31] fix units permission problems --- models/repo.go | 10 ++++- modules/context/permission.go | 59 ++++++++++++++++++++++++++++ modules/context/repo.go | 53 +++++++++---------------- routers/api/v1/api.go | 24 +++++------ routers/api/v1/repo/collaborators.go | 8 ++-- routers/api/v1/repo/file.go | 2 +- routers/api/v1/repo/issue.go | 15 ++++--- routers/api/v1/repo/issue_label.go | 40 +++++++++---------- routers/api/v1/repo/label.go | 6 +-- routers/api/v1/repo/pull.go | 6 +-- routers/api/v1/repo/repo.go | 2 +- routers/repo/branch.go | 4 +- routers/repo/issue.go | 14 +++---- routers/repo/release.go | 5 ++- routers/repo/view.go | 6 +-- routers/repo/wiki.go | 2 + routers/routes/routes.go | 39 ++++++++++++------ templates/repo/release/list.tmpl | 4 +- templates/repo/wiki/pages.tmpl | 2 +- templates/repo/wiki/start.tmpl | 2 +- templates/repo/wiki/view.tmpl | 2 +- 21 files changed, 186 insertions(+), 119 deletions(-) create mode 100644 modules/context/permission.go diff --git a/models/repo.go b/models/repo.go index b7be50e9d17a5..24c9051e01277 100644 --- a/models/repo.go +++ b/models/repo.go @@ -191,8 +191,9 @@ type Repository struct { IsMirror bool `xorm:"INDEX"` *Mirror `xorm:"-"` - ExternalMetas map[string]string `xorm:"-"` - Units []*RepoUnit `xorm:"-"` + ExternalMetas map[string]string `xorm:"-"` + Units []*RepoUnit `xorm:"-"` + UnitsMode map[UnitType]AccessMode `xorm:"-"` IsFork bool `xorm:"INDEX NOT NULL DEFAULT false"` ForkID int64 `xorm:"INDEX"` @@ -369,11 +370,16 @@ func (repo *Repository) getUnitsByUserID(e Engine, userID int64, isAdmin bool) ( return err } + repo.UnitsMode = make(map[UnitType]AccessMode) // unique var newRepoUnits = make([]*RepoUnit, 0, len(repo.Units)) for _, u := range repo.Units { for _, team := range teams { if team.unitEnabled(e, u.Type) { + m := repo.UnitsMode[u.Type] + if m < team.Authorize { + repo.UnitsMode[u.Type] = m + } newRepoUnits = append(newRepoUnits, u) break } diff --git a/modules/context/permission.go b/modules/context/permission.go new file mode 100644 index 0000000000000..967cffc488b55 --- /dev/null +++ b/modules/context/permission.go @@ -0,0 +1,59 @@ +// Copyright 2018 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 context + +import "code.gitea.io/gitea/models" + +// Permission contains all the permissions related variables to a repository +type Permission struct { + AccessMode models.AccessMode + Units []*models.RepoUnit + UnitsMode map[models.UnitType]models.AccessMode +} + +// IsOwner returns true if current user is the owner of repository. +func (p *Permission) IsOwner() bool { + return p.AccessMode >= models.AccessModeOwner +} + +// IsAdmin returns true if current user has admin or higher access of repository. +func (p *Permission) IsAdmin() bool { + return p.AccessMode >= models.AccessModeAdmin +} + +// HasAccess returns true if the current user has at least read access to any unit of this repository +func (p *Permission) HasAccess() bool { + if p.UnitsMode == nil { + return p.AccessMode >= models.AccessModeRead + } + return len(p.UnitsMode) > 0 +} + +// UnitAccessMode returns current user accessmode to the specify unit of the repository +func (p *Permission) UnitAccessMode(unitType models.UnitType) models.AccessMode { + if p.UnitsMode == nil { + return p.AccessMode + } + return p.UnitsMode[unitType] +} + +// CanAccess returns true if user has read access to the unit of the repository +func (p *Permission) CanAccess(unitType models.UnitType) bool { + return p.UnitAccessMode(unitType) >= models.AccessModeRead +} + +// CanWrite returns true if user could write to this unit +func (p *Permission) CanWrite(unitType models.UnitType) bool { + return p.UnitAccessMode(unitType) >= models.AccessModeWrite +} + +// CanWriteIssuesOrPulls returns true if isPull is true and user could write to pull requests and +// returns true if isPull is false and user could write to issues +func (p *Permission) CanWriteIssuesOrPulls(isPull bool) bool { + if isPull { + return p.CanWrite(models.UnitTypePullRequests) + } + return p.CanWrite(models.UnitTypeIssues) +} diff --git a/modules/context/repo.go b/modules/context/repo.go index be08bc4c77f9f..8f1d381f9cb04 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -32,7 +32,7 @@ type PullRequest struct { // Repository contains information to operate a repository type Repository struct { - AccessMode models.AccessMode + Permission IsWatching bool IsViewBranch bool IsViewTag bool @@ -54,34 +54,14 @@ type Repository struct { PullRequest *PullRequest } -// IsOwner returns true if current user is the owner of repository. -func (r *Repository) IsOwner() bool { - return r.AccessMode >= models.AccessModeOwner -} - -// IsAdmin returns true if current user has admin or higher access of repository. -func (r *Repository) IsAdmin() bool { - return r.AccessMode >= models.AccessModeAdmin -} - -// IsWriter returns true if current user has write or higher access of repository. -func (r *Repository) IsWriter() bool { - return r.AccessMode >= models.AccessModeWrite -} - -// HasAccess returns true if the current user has at least read access for this repository -func (r *Repository) HasAccess() bool { - return r.AccessMode >= models.AccessModeRead -} - // CanEnableEditor returns true if repository is editable and user has proper access level. func (r *Repository) CanEnableEditor() bool { - return r.Repository.CanEnableEditor() && r.IsViewBranch && r.IsWriter() + return r.Permission.CanWrite(models.UnitTypeCode) && r.Repository.CanEnableEditor() && r.IsViewBranch } // CanCreateBranch returns true if repository is editable and user has proper access level. func (r *Repository) CanCreateBranch() bool { - return r.Repository.CanCreateBranch() && r.IsWriter() + return r.Permission.CanWrite(models.UnitTypeCode) && r.Repository.CanCreateBranch() } // CanCommitToBranch returns true if repository is editable and user has proper access level @@ -101,12 +81,12 @@ func (r *Repository) CanUseTimetracker(issue *models.Issue, user *models.User) b // 2. Is the user a contributor, admin, poster or assignee and do the repository policies require this? isAssigned, _ := models.IsUserAssignedToIssue(issue, user) return r.Repository.IsTimetrackerEnabled() && (!r.Repository.AllowOnlyContributorsToTrackTime() || - r.IsWriter() || issue.IsPoster(user.ID) || isAssigned) + r.Permission.CanWrite(models.UnitTypeIssues) || issue.IsPoster(user.ID) || isAssigned) } // CanCreateIssueDependencies returns whether or not a user can create dependencies. func (r *Repository) CanCreateIssueDependencies(user *models.User) bool { - return r.Repository.IsDependenciesEnabled() && r.IsWriter() + return r.Permission.CanWrite(models.UnitTypeIssues) && r.Repository.IsDependenciesEnabled() } // GetCommitsCount returns cached commit count for current view @@ -223,7 +203,7 @@ func RedirectToRepo(ctx *Context, redirectRepoID int64) { func repoAssignment(ctx *Context, repo *models.Repository) { // Admin has super access. if ctx.IsSigned && ctx.User.IsAdmin { - ctx.Repo.AccessMode = models.AccessModeOwner + ctx.Repo.Permission.AccessMode = models.AccessModeOwner } else { var userID int64 if ctx.User != nil { @@ -234,11 +214,11 @@ func repoAssignment(ctx *Context, repo *models.Repository) { ctx.ServerError("AccessLevel", err) return } - ctx.Repo.AccessMode = mode + ctx.Repo.Permission.AccessMode = mode } // Check access. - if ctx.Repo.AccessMode == models.AccessModeNone { + if ctx.Repo.Permission.AccessMode == models.AccessModeNone { if ctx.Query("go-get") == "1" { EarlyResponseForGoGetMeta(ctx) return @@ -379,9 +359,10 @@ func RepoAssignment() macaron.Handler { ctx.Data["Title"] = owner.Name + "/" + repo.Name ctx.Data["Repository"] = repo ctx.Data["Owner"] = ctx.Repo.Repository.Owner - ctx.Data["IsRepositoryOwner"] = ctx.Repo.IsOwner() - ctx.Data["IsRepositoryAdmin"] = ctx.Repo.IsAdmin() - ctx.Data["IsRepositoryWriter"] = ctx.Repo.IsWriter() + ctx.Data["IsRepositoryOwner"] = ctx.Repo.Permission.IsOwner() + ctx.Data["IsRepositoryAdmin"] = ctx.Repo.Permission.IsAdmin() + // FIXME: IsRepositoryWriter will only be used on codes related operations. + ctx.Data["IsRepositoryWriter"] = ctx.Repo.Permission.CanWrite(models.UnitTypeCode) if ctx.Data["CanSignedUserFork"], err = ctx.Repo.Repository.CanUserFork(ctx.User); err != nil { ctx.ServerError("CanUserFork", err) @@ -435,7 +416,7 @@ func RepoAssignment() macaron.Handler { } // People who have push access or have forked repository can propose a new pull request. - if ctx.Repo.IsWriter() || (ctx.IsSigned && ctx.User.HasForkedRepo(ctx.Repo.Repository.ID)) { + if ctx.Repo.CanWrite(models.UnitTypeCode) || (ctx.IsSigned && ctx.User.HasForkedRepo(ctx.Repo.Repository.ID)) { // Pull request is allowed if this is a fork repository // and base repository accepts pull requests. if repo.BaseRepo != nil && repo.BaseRepo.AllowsPulls() { @@ -671,10 +652,10 @@ func RequireRepoAdmin() macaron.Handler { } } -// RequireRepoWriter returns a macaron middleware for requiring repository write permission -func RequireRepoWriter() macaron.Handler { +// RequireRepoWriter returns a macaron middleware for requiring repository write to code permission +func RequireRepoWriter(unitType models.UnitType) macaron.Handler { return func(ctx *Context) { - if !ctx.IsSigned || (!ctx.Repo.IsWriter() && !ctx.User.IsAdmin) { + if !ctx.IsSigned || (!ctx.Repo.CanWrite(unitType) && !ctx.User.IsAdmin) { ctx.NotFound(ctx.Req.RequestURI, nil) return } @@ -698,6 +679,8 @@ func LoadRepoUnits() macaron.Handler { ctx.ServerError("LoadUnitsByUserID", err) return } + ctx.Repo.Permission.Units = ctx.Repo.Repository.Units + ctx.Repo.Permission.UnitsMode = ctx.Repo.Repository.UnitsMode } } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 0be8f84836f3c..52f84c7e0b6b8 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -152,6 +152,7 @@ func repoAssignment() macaron.Handler { return } repo.Owner = owner + ctx.Repo.Repository = repo if ctx.IsSigned && ctx.User.IsAdmin { ctx.Repo.AccessMode = models.AccessModeOwner @@ -168,8 +169,6 @@ func repoAssignment() macaron.Handler { ctx.Status(404) return } - - ctx.Repo.Repository = repo } } @@ -205,12 +204,15 @@ func reqAdmin() macaron.Handler { } } -func reqRepoWriter() macaron.Handler { +func reqRepoWriter(unitTypes ...models.UnitType) macaron.Handler { return func(ctx *context.Context) { - if !ctx.Repo.IsWriter() { - ctx.Error(403) - return + for _, unitType := range unitTypes { + if ctx.Repo.CanWrite(unitType) { + return + } } + + ctx.Error(403) } } @@ -453,7 +455,7 @@ func RegisterRoutes(m *macaron.Macaron) { Delete(repo.DeleteHook) m.Post("/tests", context.RepoRef(), repo.TestHook) }) - }, reqToken(), reqRepoWriter()) + }, reqToken(), reqRepoWriter(models.UnitTypeCode)) m.Group("/collaborators", func() { m.Get("", repo.ListCollaborators) m.Combo("/:collaborator").Get(repo.IsCollaborator). @@ -473,7 +475,7 @@ func RegisterRoutes(m *macaron.Macaron) { Post(bind(api.CreateKeyOption{}), repo.CreateDeployKey) m.Combo("/:id").Get(repo.GetDeployKey). Delete(repo.DeleteDeploykey) - }, reqToken(), reqRepoWriter()) + }, reqToken(), reqRepoWriter(models.UnitTypeCode)) m.Group("/times", func() { m.Combo("").Get(repo.ListTrackedTimesByRepository) m.Combo("/:timetrackingusername").Get(repo.ListTrackedTimesByUser) @@ -524,10 +526,10 @@ func RegisterRoutes(m *macaron.Macaron) { }) m.Group("/milestones", func() { m.Combo("").Get(repo.ListMilestones). - Post(reqToken(), reqRepoWriter(), bind(api.CreateMilestoneOption{}), repo.CreateMilestone) + Post(reqToken(), reqRepoWriter(models.UnitTypeIssues, models.UnitTypePullRequests), bind(api.CreateMilestoneOption{}), repo.CreateMilestone) m.Combo("/:id").Get(repo.GetMilestone). - Patch(reqToken(), reqRepoWriter(), bind(api.EditMilestoneOption{}), repo.EditMilestone). - Delete(reqToken(), reqRepoWriter(), repo.DeleteMilestone) + Patch(reqToken(), reqRepoWriter(models.UnitTypeIssues, models.UnitTypePullRequests), bind(api.EditMilestoneOption{}), repo.EditMilestone). + Delete(reqToken(), reqRepoWriter(models.UnitTypeIssues, models.UnitTypePullRequests), repo.DeleteMilestone) }) m.Get("/stargazers", repo.ListStargazers) m.Get("/subscribers", repo.ListSubscribers) diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index c52472b0f8724..200392e903e4a 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -34,7 +34,7 @@ func ListCollaborators(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/UserList" - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeCode) { ctx.Error(403, "", "User does not have push access") return } @@ -78,7 +78,7 @@ func IsCollaborator(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "404": // "$ref": "#/responses/empty" - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeCode) { ctx.Error(403, "", "User does not have push access") return } @@ -133,7 +133,7 @@ func AddCollaborator(ctx *context.APIContext, form api.AddCollaboratorOption) { // responses: // "204": // "$ref": "#/responses/empty" - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeCode) { ctx.Error(403, "", "User does not have push access") return } @@ -193,7 +193,7 @@ func DeleteCollaborator(ctx *context.APIContext) { // responses: // "204": // "$ref": "#/responses/empty" - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeCode) { ctx.Error(403, "", "User does not have push access") return } diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 610247bc27af5..a04dd39e0242f 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -38,7 +38,7 @@ func GetRawFile(ctx *context.APIContext) { // responses: // 200: // description: success - if !ctx.Repo.HasAccess() { + if !ctx.Repo.CanAccess(models.UnitTypeCode) { ctx.Status(404) return } diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index cd78135a62160..7a8ed09b48647 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -169,7 +169,7 @@ func CreateIssue(ctx *context.APIContext, form api.CreateIssueOption) { // "$ref": "#/responses/Issue" var deadlineUnix util.TimeStamp - if form.Deadline != nil && ctx.Repo.IsWriter() { + if form.Deadline != nil && ctx.Repo.CanWrite(models.UnitTypeIssues) { deadlineUnix = util.TimeStamp(form.Deadline.Unix()) } @@ -184,7 +184,7 @@ func CreateIssue(ctx *context.APIContext, form api.CreateIssueOption) { var assigneeIDs = make([]int64, 0) var err error - if ctx.Repo.IsWriter() { + if ctx.Repo.CanWrite(models.UnitTypeIssues) { issue.MilestoneID = form.Milestone assigneeIDs, err = models.MakeIDsFromAPIAssigneesToAdd(form.Assignee, form.Assignees) if err != nil { @@ -274,7 +274,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { return } - if !issue.IsPoster(ctx.User.ID) && !ctx.Repo.IsWriter() { + if !issue.IsPoster(ctx.User.ID) && !ctx.Repo.CanWrite(models.UnitTypeIssues) { ctx.Status(403) return } @@ -288,7 +288,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { // Update the deadline var deadlineUnix util.TimeStamp - if form.Deadline != nil && !form.Deadline.IsZero() && ctx.Repo.IsWriter() { + if form.Deadline != nil && !form.Deadline.IsZero() && ctx.Repo.CanWrite(models.UnitTypeIssues) { deadlineUnix = util.TimeStamp(form.Deadline.Unix()) } @@ -305,8 +305,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { // Pass one or more user logins to replace the set of assignees on this Issue. // Send an empty array ([]) to clear all assignees from the Issue. - if ctx.Repo.IsWriter() && (form.Assignees != nil || form.Assignee != nil) { - + if ctx.Repo.CanWrite(models.UnitTypeIssues) && (form.Assignees != nil || form.Assignee != nil) { oneAssignee := "" if form.Assignee != nil { oneAssignee = *form.Assignee @@ -319,7 +318,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { } } - if ctx.Repo.IsWriter() && form.Milestone != nil && + if ctx.Repo.CanWrite(models.UnitTypeIssues) && form.Milestone != nil && issue.MilestoneID != *form.Milestone { oldMilestoneID := issue.MilestoneID issue.MilestoneID = *form.Milestone @@ -403,7 +402,7 @@ func UpdateIssueDeadline(ctx *context.APIContext, form api.EditDeadlineOption) { return } - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeIssues) { ctx.Status(403) return } diff --git a/routers/api/v1/repo/issue_label.go b/routers/api/v1/repo/issue_label.go index 35defa25b57d1..f006ee888e6e0 100644 --- a/routers/api/v1/repo/issue_label.go +++ b/routers/api/v1/repo/issue_label.go @@ -90,11 +90,6 @@ func AddIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) { // responses: // "200": // "$ref": "#/responses/LabelList" - if !ctx.Repo.IsWriter() { - ctx.Status(403) - return - } - issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if models.IsErrIssueNotExist(err) { @@ -105,6 +100,11 @@ func AddIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) { return } + if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { + ctx.Status(403) + return + } + labels, err := models.GetLabelsInRepoByIDs(ctx.Repo.Repository.ID, form.Labels) if err != nil { ctx.Error(500, "GetLabelsInRepoByIDs", err) @@ -162,11 +162,6 @@ func DeleteIssueLabel(ctx *context.APIContext) { // responses: // "204": // "$ref": "#/responses/empty" - if !ctx.Repo.IsWriter() { - ctx.Status(403) - return - } - issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if models.IsErrIssueNotExist(err) { @@ -177,6 +172,11 @@ func DeleteIssueLabel(ctx *context.APIContext) { return } + if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { + ctx.Status(403) + return + } + label, err := models.GetLabelInRepoByID(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id")) if err != nil { if models.IsErrLabelNotExist(err) { @@ -228,11 +228,6 @@ func ReplaceIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) { // responses: // "200": // "$ref": "#/responses/LabelList" - if !ctx.Repo.IsWriter() { - ctx.Status(403) - return - } - issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if models.IsErrIssueNotExist(err) { @@ -243,6 +238,11 @@ func ReplaceIssueLabels(ctx *context.APIContext, form api.IssueLabelsOption) { return } + if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { + ctx.Status(403) + return + } + labels, err := models.GetLabelsInRepoByIDs(ctx.Repo.Repository.ID, form.Labels) if err != nil { ctx.Error(500, "GetLabelsInRepoByIDs", err) @@ -294,11 +294,6 @@ func ClearIssueLabels(ctx *context.APIContext) { // responses: // "204": // "$ref": "#/responses/empty" - if !ctx.Repo.IsWriter() { - ctx.Status(403) - return - } - issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if models.IsErrIssueNotExist(err) { @@ -309,6 +304,11 @@ func ClearIssueLabels(ctx *context.APIContext) { return } + if !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { + ctx.Status(403) + return + } + if err := issue.ClearLabels(ctx.User); err != nil { ctx.Error(500, "ClearLabels", err) return diff --git a/routers/api/v1/repo/label.go b/routers/api/v1/repo/label.go index 359ccf0e41eca..91ed30ec006ed 100644 --- a/routers/api/v1/repo/label.go +++ b/routers/api/v1/repo/label.go @@ -123,7 +123,7 @@ func CreateLabel(ctx *context.APIContext, form api.CreateLabelOption) { // responses: // "201": // "$ref": "#/responses/Label" - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.Repo.CanWrite(models.UnitTypePullRequests) { ctx.Status(403) return } @@ -173,7 +173,7 @@ func EditLabel(ctx *context.APIContext, form api.EditLabelOption) { // responses: // "200": // "$ref": "#/responses/Label" - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.Repo.CanWrite(models.UnitTypePullRequests) { ctx.Status(403) return } @@ -226,7 +226,7 @@ func DeleteLabel(ctx *context.APIContext) { // responses: // "204": // "$ref": "#/responses/empty" - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.Repo.CanWrite(models.UnitTypePullRequests) { ctx.Status(403) return } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 83ef886d7d319..c0891793b25b1 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -351,7 +351,7 @@ func EditPullRequest(ctx *context.APIContext, form api.EditPullRequestOption) { pr.LoadIssue() issue := pr.Issue - if !issue.IsPoster(ctx.User.ID) && !ctx.Repo.IsWriter() { + if !issue.IsPoster(ctx.User.ID) && !ctx.Repo.CanWrite(models.UnitTypePullRequests) { ctx.Status(403) return } @@ -382,7 +382,7 @@ func EditPullRequest(ctx *context.APIContext, form api.EditPullRequestOption) { // Pass one or more user logins to replace the set of assignees on this Issue. // Send an empty array ([]) to clear all assignees from the Issue. - if ctx.Repo.IsWriter() && (form.Assignees != nil || len(form.Assignee) > 0) { + if ctx.Repo.CanWrite(models.UnitTypePullRequests) && (form.Assignees != nil || len(form.Assignee) > 0) { err = models.UpdateAPIAssignee(issue, form.Assignee, form.Assignees, ctx.User) if err != nil { @@ -395,7 +395,7 @@ func EditPullRequest(ctx *context.APIContext, form api.EditPullRequestOption) { } } - if ctx.Repo.IsWriter() && form.Milestone != 0 && + if ctx.Repo.CanWrite(models.UnitTypePullRequests) && form.Milestone != 0 && issue.MilestoneID != form.Milestone { oldMilestoneID := issue.MilestoneID issue.MilestoneID = form.Milestone diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index bf6346eebdfca..f75b6807f1981 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -553,7 +553,7 @@ func MirrorSync(ctx *context.APIContext) { // "$ref": "#/responses/empty" repo := ctx.Repo.Repository - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeCode) { ctx.Error(403, "MirrorSync", "Must have write access") } diff --git a/routers/repo/branch.go b/routers/repo/branch.go index 7b1e2a8bbe7bc..2c009f55ab700 100644 --- a/routers/repo/branch.go +++ b/routers/repo/branch.go @@ -33,7 +33,7 @@ func Branches(ctx *context.Context) { ctx.Data["Title"] = "Branches" ctx.Data["IsRepoToolbarBranches"] = true ctx.Data["DefaultBranch"] = ctx.Repo.Repository.DefaultBranch - ctx.Data["IsWriter"] = ctx.Repo.IsWriter() + ctx.Data["IsWriter"] = ctx.Repo.CanWrite(models.UnitTypeCode) ctx.Data["IsMirror"] = ctx.Repo.Repository.IsMirror ctx.Data["PageIsViewCode"] = true ctx.Data["PageIsBranches"] = true @@ -161,7 +161,7 @@ func loadBranches(ctx *context.Context) []*Branch { } } - if ctx.Repo.IsWriter() { + if ctx.Repo.CanWrite(models.UnitTypeCode) { deletedBranches, err := getDeletedBranches(ctx) if err != nil { ctx.ServerError("getDeletedBranches", err) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 08bdf311f912c..6fd15cdd42abe 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -280,7 +280,7 @@ func RetrieveRepoMilestonesAndAssignees(ctx *context.Context, repo *models.Repos // RetrieveRepoMetas find all the meta information of a repository func RetrieveRepoMetas(ctx *context.Context, repo *models.Repository) []*models.Label { - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeIssues) { return nil } @@ -380,7 +380,7 @@ func ValidateRepoMetas(ctx *context.Context, form auth.CreateIssueForm) ([]int64 return nil, nil, 0 } - if !ctx.Repo.IsWriter() { + if !ctx.Repo.CanWrite(models.UnitTypeIssues) { return nil, nil, 0 } @@ -616,7 +616,7 @@ func ViewIssue(ctx *context.Context) { ctx.Data["Labels"] = labels // Check milestone and assignee. - if ctx.Repo.IsWriter() { + if ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { RetrieveRepoMilestonesAndAssignees(ctx, repo) if ctx.Written() { return @@ -818,7 +818,7 @@ func ViewIssue(ctx *context.Context) { ctx.Data["NumParticipants"] = len(participants) ctx.Data["Issue"] = issue ctx.Data["ReadOnly"] = true - ctx.Data["IsIssueOwner"] = ctx.Repo.IsWriter() || (ctx.IsSigned && issue.IsPoster(ctx.User.ID)) + ctx.Data["IsIssueOwner"] = ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) || (ctx.IsSigned && issue.IsPoster(ctx.User.ID)) ctx.Data["SignInLink"] = setting.AppSubURL + "/user/login?redirect_to=" + ctx.Data["Link"].(string) ctx.HTML(200, tplIssueView) } @@ -890,7 +890,7 @@ func UpdateIssueTitle(ctx *context.Context) { return } - if !ctx.IsSigned || (!issue.IsPoster(ctx.User.ID) && !ctx.Repo.IsWriter()) { + if !ctx.IsSigned || (!issue.IsPoster(ctx.User.ID) && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull)) { ctx.Error(403) return } @@ -918,7 +918,7 @@ func UpdateIssueContent(ctx *context.Context) { return } - if !ctx.IsSigned || (ctx.User.ID != issue.PosterID && !ctx.Repo.IsWriter()) { + if !ctx.IsSigned || (ctx.User.ID != issue.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull)) { ctx.Error(403) return } @@ -1051,7 +1051,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { var comment *models.Comment defer func() { // Check if issue admin/poster changes the status of issue. - if (ctx.Repo.IsWriter() || (ctx.IsSigned && issue.IsPoster(ctx.User.ID))) && + if (ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) || (ctx.IsSigned && issue.IsPoster(ctx.User.ID))) && (form.Status == "reopen" || form.Status == "close") && !(issue.IsPull && issue.PullRequest.HasMerged) { diff --git a/routers/repo/release.go b/routers/repo/release.go index bae87efdcdc81..33abf07412316 100644 --- a/routers/repo/release.go +++ b/routers/repo/release.go @@ -65,8 +65,11 @@ func Releases(ctx *context.Context) { limit = 10 } + writeAccess := ctx.Repo.CanWrite(models.UnitTypeReleases) + ctx.Data["CanCreateRelease"] = writeAccess + opts := models.FindReleasesOptions{ - IncludeDrafts: ctx.Repo.IsWriter(), + IncludeDrafts: writeAccess, IncludeTags: true, } diff --git a/routers/repo/view.go b/routers/repo/view.go index 657fe315a2a73..f00329f9a9bee 100644 --- a/routers/repo/view.go +++ b/routers/repo/view.go @@ -137,7 +137,7 @@ func renderDirectory(ctx *context.Context, treeLink string) { ctx.Data["LatestCommitStatus"] = models.CalcCommitStatus(statuses) // Check permission to add or upload new file. - if ctx.Repo.IsWriter() && ctx.Repo.IsViewBranch { + if ctx.Repo.CanWrite(models.UnitTypeCode) && ctx.Repo.IsViewBranch { ctx.Data["CanAddFile"] = true ctx.Data["CanUploadFile"] = setting.Repository.Upload.Enabled } @@ -256,7 +256,7 @@ func renderFile(ctx *context.Context, entry *git.TreeEntry, treeLink, rawLink st ctx.Data["EditFileTooltip"] = ctx.Tr("repo.editor.edit_this_file") } else if !ctx.Repo.IsViewBranch { ctx.Data["EditFileTooltip"] = ctx.Tr("repo.editor.must_be_on_a_branch") - } else if !ctx.Repo.IsWriter() { + } else if !ctx.Repo.CanWrite(models.UnitTypeCode) { ctx.Data["EditFileTooltip"] = ctx.Tr("repo.editor.fork_before_edit") } @@ -275,7 +275,7 @@ func renderFile(ctx *context.Context, entry *git.TreeEntry, treeLink, rawLink st ctx.Data["DeleteFileTooltip"] = ctx.Tr("repo.editor.delete_this_file") } else if !ctx.Repo.IsViewBranch { ctx.Data["DeleteFileTooltip"] = ctx.Tr("repo.editor.must_be_on_a_branch") - } else if !ctx.Repo.IsWriter() { + } else if !ctx.Repo.CanWrite(models.UnitTypeCode) { ctx.Data["DeleteFileTooltip"] = ctx.Tr("repo.editor.must_have_write_access") } } diff --git a/routers/repo/wiki.go b/routers/repo/wiki.go index 3220ab134d46a..b319c258d22b4 100644 --- a/routers/repo/wiki.go +++ b/routers/repo/wiki.go @@ -200,6 +200,7 @@ func renderWikiPage(ctx *context.Context, isViewPage bool) (*git.Repository, *gi // Wiki renders single wiki page func Wiki(ctx *context.Context) { ctx.Data["PageIsWiki"] = true + ctx.Data["CanWriteWiki"] = ctx.Repo.CanWrite(models.UnitTypeWiki) if !ctx.Repo.Repository.HasWiki() { ctx.Data["Title"] = ctx.Tr("repo.wiki") @@ -237,6 +238,7 @@ func Wiki(ctx *context.Context) { func WikiPages(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("repo.wiki.pages") ctx.Data["PageIsWiki"] = true + ctx.Data["CanWriteWiki"] = ctx.Repo.CanWrite(models.UnitTypeWiki) if !ctx.Repo.Repository.HasWiki() { ctx.Redirect(ctx.Repo.RepoLink + "/wiki") diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 1c1bcd8f95354..388093fc2911e 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -393,7 +393,20 @@ func RegisterRoutes(m *macaron.Macaron) { } reqRepoAdmin := context.RequireRepoAdmin() - reqRepoWriter := context.RequireRepoWriter() + reqRepoCodeWriter := context.RequireRepoWriter(models.UnitTypeCode) + reqRepoReleaseWriter := context.RequireRepoWriter(models.UnitTypeReleases) + reqRepoWikiWriter := context.RequireRepoWriter(models.UnitTypeWiki) + reqRepoPullsWriter := context.RequireRepoWriter(models.UnitTypePullRequests) + reqRepoIssuesOrPullsWriter := func() macaron.Handler { + return func(ctx *context.Context) { + if !ctx.IsSigned || (!ctx.Repo.CanWrite(models.UnitTypeIssues) && + !ctx.Repo.CanWrite(models.UnitTypePullRequests) && + !ctx.User.IsAdmin) { + ctx.NotFound(ctx.Req.RequestURI, nil) + return + } + } + } // ***** START: Organization ***** m.Group("/org", func() { @@ -545,10 +558,10 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/reactions/:action", bindIgnErr(auth.ReactionForm{}), repo.ChangeIssueReaction) }) - m.Post("/labels", reqRepoWriter, repo.UpdateIssueLabel) - m.Post("/milestone", reqRepoWriter, repo.UpdateIssueMilestone) - m.Post("/assignee", reqRepoWriter, repo.UpdateIssueAssignee) - m.Post("/status", reqRepoWriter, repo.UpdateIssueStatus) + m.Post("/labels", reqRepoIssuesOrPullsWriter, repo.UpdateIssueLabel) + m.Post("/milestone", reqRepoIssuesOrPullsWriter, repo.UpdateIssueMilestone) + m.Post("/assignee", reqRepoIssuesOrPullsWriter, repo.UpdateIssueAssignee) + m.Post("/status", reqRepoIssuesOrPullsWriter, repo.UpdateIssueStatus) }) m.Group("/comments/:id", func() { m.Post("", repo.UpdateCommentContent) @@ -560,7 +573,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/edit", bindIgnErr(auth.CreateLabelForm{}), repo.UpdateLabel) m.Post("/delete", repo.DeleteLabel) m.Post("/initialize", bindIgnErr(auth.InitializeLabelsForm{}), repo.InitializeLabels) - }, reqRepoWriter, context.RepoRef(), context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests)) + }, reqRepoIssuesOrPullsWriter, context.RepoRef()) m.Group("/milestones", func() { m.Combo("/new").Get(repo.NewMilestone). Post(bindIgnErr(auth.CreateMilestoneForm{}), repo.NewMilestonePost) @@ -568,7 +581,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/:id/edit", bindIgnErr(auth.CreateMilestoneForm{}), repo.EditMilestonePost) m.Get("/:id/:action", repo.ChangeMilestonStatus) m.Post("/delete", repo.DeleteMilestone) - }, reqRepoWriter, context.RepoRef(), context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests)) + }, reqRepoIssuesOrPullsWriter, context.RepoRef()) m.Combo("/compare/*", repo.MustAllowPulls, repo.SetEditorconfigIfExists). Get(repo.SetDiffViewStyle, repo.CompareAndPullRequest). @@ -591,7 +604,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/upload-file", repo.UploadFileToServer) m.Post("/upload-remove", bindIgnErr(auth.RemoveUploadFileForm{}), repo.RemoveUploadFileFromServer) }, context.RepoRef(), repo.MustBeEditable, repo.MustBeAbleToUpload) - }, repo.MustBeNotBare, reqRepoWriter) + }, repo.MustBeNotBare, reqRepoCodeWriter) m.Group("/branches", func() { m.Group("/_new/", func() { @@ -601,7 +614,7 @@ func RegisterRoutes(m *macaron.Macaron) { }, bindIgnErr(auth.NewBranchForm{})) m.Post("/delete", repo.DeleteBranchPost) m.Post("/restore", repo.RestoreBranchPost) - }, reqRepoWriter, repo.MustBeNotBare, context.CheckUnit(models.UnitTypeCode)) + }, reqRepoCodeWriter, repo.MustBeNotBare) }, reqSignIn, context.RepoAssignment(), context.UnitTypes(), context.LoadRepoUnits()) @@ -614,11 +627,11 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/new", repo.NewRelease) m.Post("/new", bindIgnErr(auth.NewReleaseForm{}), repo.NewReleasePost) m.Post("/delete", repo.DeleteRelease) - }, reqSignIn, repo.MustBeNotBare, reqRepoWriter, context.RepoRef()) + }, reqSignIn, repo.MustBeNotBare, reqRepoReleaseWriter, context.RepoRef()) m.Group("/releases", func() { m.Get("/edit/*", repo.EditRelease) m.Post("/edit/*", bindIgnErr(auth.EditReleaseForm{}), repo.EditReleasePost) - }, reqSignIn, repo.MustBeNotBare, reqRepoWriter, func(ctx *context.Context) { + }, reqSignIn, repo.MustBeNotBare, reqRepoReleaseWriter, func(ctx *context.Context) { var err error ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetBranchCommit(ctx.Repo.Repository.DefaultBranch) if err != nil { @@ -656,7 +669,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Combo("/:page/_edit").Get(repo.EditWiki). Post(bindIgnErr(auth.NewWikiForm{}), repo.EditWikiPost) m.Post("/:page/delete", repo.DeleteWikiPagePost) - }, reqSignIn, reqRepoWriter) + }, reqSignIn, reqRepoWikiWriter) }, repo.MustEnableWiki, context.RepoRef()) m.Group("/wiki", func() { @@ -678,7 +691,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get(".diff", repo.DownloadPullDiff) m.Get(".patch", repo.DownloadPullPatch) m.Get("/commits", context.RepoRef(), repo.ViewPullCommits) - m.Post("/merge", reqRepoWriter, bindIgnErr(auth.MergePullRequestForm{}), repo.MergePullRequest) + m.Post("/merge", reqRepoPullsWriter, bindIgnErr(auth.MergePullRequestForm{}), repo.MergePullRequest) m.Post("/cleanup", context.RepoRef(), repo.CleanUpPullRequest) m.Group("/files", func() { m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.ViewPullFiles) diff --git a/templates/repo/release/list.tmpl b/templates/repo/release/list.tmpl index 7337b8ca0ed23..8e1ce06f5deaf 100644 --- a/templates/repo/release/list.tmpl +++ b/templates/repo/release/list.tmpl @@ -5,7 +5,7 @@ {{template "base/alert" .}} <h2 class="ui header"> {{.i18n.Tr "repo.release.releases"}} - {{if .IsRepositoryWriter}} + {{if .CanCreateRelease}} <div class="ui right"> <a class="ui small green button" href="{{$.RepoLink}}/releases/new"> {{.i18n.Tr "repo.release.new_release"}} @@ -50,7 +50,7 @@ {{else}} <h3> <a href="{{$.RepoLink}}/src/tag/{{.TagName | EscapePound}}">{{.Title}}</a> - {{if $.IsRepositoryWriter}}<small>(<a href="{{$.RepoLink}}/releases/edit/{{.TagName | EscapePound}}" rel="nofollow">{{$.i18n.Tr "repo.release.edit"}}</a>)</small>{{end}} + {{if $.CanCreateRelease}}<small>(<a href="{{$.RepoLink}}/releases/edit/{{.TagName | EscapePound}}" rel="nofollow">{{$.i18n.Tr "repo.release.edit"}}</a>)</small>{{end}} </h3> <p class="text grey"> <span class="author"> diff --git a/templates/repo/wiki/pages.tmpl b/templates/repo/wiki/pages.tmpl index 2829b61374731..61903423f390a 100644 --- a/templates/repo/wiki/pages.tmpl +++ b/templates/repo/wiki/pages.tmpl @@ -4,7 +4,7 @@ <div class="ui container"> <div class="ui header"> {{.i18n.Tr "repo.wiki.pages"}} - {{if and .IsRepositoryWriter (not .IsRepositoryMirror)}} + {{if and .CanWriteWiki (not .IsRepositoryMirror)}} <div class="ui right"> <a class="ui green small button" href="{{.RepoLink}}/wiki/_new">{{.i18n.Tr "repo.wiki.new_page_button"}}</a> </div> diff --git a/templates/repo/wiki/start.tmpl b/templates/repo/wiki/start.tmpl index 1b8d52a587b86..0341e8067b1f2 100644 --- a/templates/repo/wiki/start.tmpl +++ b/templates/repo/wiki/start.tmpl @@ -6,7 +6,7 @@ <span class="mega-octicon octicon-book"></span> <h2>{{.i18n.Tr "repo.wiki.welcome"}}</h2> <p>{{.i18n.Tr "repo.wiki.welcome_desc"}}</p> - {{if and .IsRepositoryWriter (not .Repository.IsMirror)}} + {{if and .CanWriteWiki (not .Repository.IsMirror)}} <a class="ui green button" href="{{.RepoLink}}/wiki/_new">{{.i18n.Tr "repo.wiki.create_first_page"}}</a> {{end}} </div> diff --git a/templates/repo/wiki/view.tmpl b/templates/repo/wiki/view.tmpl index 93b7c10640110..dd2de2a041380 100644 --- a/templates/repo/wiki/view.tmpl +++ b/templates/repo/wiki/view.tmpl @@ -63,7 +63,7 @@ </div> </div> <div class="eight wide right aligned column"> - {{if and .IsRepositoryWriter (not .Repository.IsMirror)}} + {{if and .CanWriteWiki (not .Repository.IsMirror)}} <div class="ui right"> <a class="ui small button" href="{{.RepoLink}}/wiki/{{.PageURL}}/_edit">{{.i18n.Tr "repo.wiki.edit_page_button"}}</a> <a class="ui green small button" href="{{.RepoLink}}/wiki/_new">{{.i18n.Tr "repo.wiki.new_page_button"}}</a> From 15f80b9ed6a70f2fb0a1bf2b2f4f8655cfdd3f56 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 11 Nov 2018 09:56:23 +0800 Subject: [PATCH 02/31] fix some bugs and merge LoadUnits to repoAssignment --- models/repo.go | 2 +- modules/context/permission.go | 7 ++- modules/context/repo.go | 43 +++++++++---------- routers/repo/issue.go | 2 +- routers/routes/routes.go | 25 ++++------- templates/repo/bare.tmpl | 2 +- templates/repo/issue/labels.tmpl | 8 ++-- templates/repo/issue/milestone_new.tmpl | 2 +- templates/repo/issue/milestones.tmpl | 6 +-- .../repo/issue/view_content/sidebar.tmpl | 10 ++--- 10 files changed, 51 insertions(+), 56 deletions(-) diff --git a/models/repo.go b/models/repo.go index 24c9051e01277..e851246e6f363 100644 --- a/models/repo.go +++ b/models/repo.go @@ -378,7 +378,7 @@ func (repo *Repository) getUnitsByUserID(e Engine, userID int64, isAdmin bool) ( if team.unitEnabled(e, u.Type) { m := repo.UnitsMode[u.Type] if m < team.Authorize { - repo.UnitsMode[u.Type] = m + repo.UnitsMode[u.Type] = team.Authorize } newRepoUnits = append(newRepoUnits, u) break diff --git a/modules/context/permission.go b/modules/context/permission.go index 967cffc488b55..96f14fb714349 100644 --- a/modules/context/permission.go +++ b/modules/context/permission.go @@ -4,7 +4,11 @@ package context -import "code.gitea.io/gitea/models" +import ( + "fmt" + + "code.gitea.io/gitea/models" +) // Permission contains all the permissions related variables to a repository type Permission struct { @@ -46,6 +50,7 @@ func (p *Permission) CanAccess(unitType models.UnitType) bool { // CanWrite returns true if user could write to this unit func (p *Permission) CanWrite(unitType models.UnitType) bool { + fmt.Println("p:", unitType, p.UnitsMode) return p.UnitAccessMode(unitType) >= models.AccessModeWrite } diff --git a/modules/context/repo.go b/modules/context/repo.go index 8f1d381f9cb04..c2cd187074179 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -201,11 +201,11 @@ func RedirectToRepo(ctx *Context, redirectRepoID int64) { } func repoAssignment(ctx *Context, repo *models.Repository) { + var userID int64 // Admin has super access. if ctx.IsSigned && ctx.User.IsAdmin { ctx.Repo.Permission.AccessMode = models.AccessModeOwner } else { - var userID int64 if ctx.User != nil { userID = ctx.User.ID } @@ -217,6 +217,14 @@ func repoAssignment(ctx *Context, repo *models.Repository) { ctx.Repo.Permission.AccessMode = mode } + err := repo.LoadUnitsByUserID(userID, ctx.Repo.IsAdmin()) + if err != nil { + ctx.ServerError("LoadUnitsByUserID", err) + return + } + ctx.Repo.Permission.Units = repo.Units + ctx.Repo.Permission.UnitsMode = repo.UnitsMode + // Check access. if ctx.Repo.Permission.AccessMode == models.AccessModeNone { if ctx.Query("go-get") == "1" { @@ -359,10 +367,11 @@ func RepoAssignment() macaron.Handler { ctx.Data["Title"] = owner.Name + "/" + repo.Name ctx.Data["Repository"] = repo ctx.Data["Owner"] = ctx.Repo.Repository.Owner - ctx.Data["IsRepositoryOwner"] = ctx.Repo.Permission.IsOwner() - ctx.Data["IsRepositoryAdmin"] = ctx.Repo.Permission.IsAdmin() - // FIXME: IsRepositoryWriter will only be used on codes related operations. - ctx.Data["IsRepositoryWriter"] = ctx.Repo.Permission.CanWrite(models.UnitTypeCode) + ctx.Data["IsRepositoryOwner"] = ctx.Repo.IsOwner() + ctx.Data["IsRepositoryAdmin"] = ctx.Repo.IsAdmin() + ctx.Data["CanWriteCode"] = ctx.Repo.CanWrite(models.UnitTypeCode) + ctx.Data["CanWriteIssues"] = ctx.Repo.CanWrite(models.UnitTypeIssues) + ctx.Data["CanWritePulls"] = ctx.Repo.CanWrite(models.UnitTypePullRequests) if ctx.Data["CanSignedUserFork"], err = ctx.Repo.Repository.CanUserFork(ctx.User); err != nil { ctx.ServerError("CanUserFork", err) @@ -662,25 +671,15 @@ func RequireRepoWriter(unitType models.UnitType) macaron.Handler { } } -// LoadRepoUnits loads repsitory's units, it should be called after repository and user loaded -func LoadRepoUnits() macaron.Handler { +// RequireRepoWriterOr returns a macaron middleware for requiring repository write to one of the unit permission +func RequireRepoWriterOr(unitTypes ...models.UnitType) macaron.Handler { return func(ctx *Context) { - var isAdmin bool - if ctx.User != nil && ctx.User.IsAdmin { - isAdmin = true - } - - var userID int64 - if ctx.User != nil { - userID = ctx.User.ID - } - err := ctx.Repo.Repository.LoadUnitsByUserID(userID, isAdmin) - if err != nil { - ctx.ServerError("LoadUnitsByUserID", err) - return + for _, unitType := range unitTypes { + if ctx.Repo.CanWrite(unitType) { + return + } } - ctx.Repo.Permission.Units = ctx.Repo.Repository.Units - ctx.Repo.Permission.UnitsMode = ctx.Repo.Repository.UnitsMode + ctx.NotFound(ctx.Req.RequestURI, nil) } } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 6fd15cdd42abe..43dea84480cbf 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -779,7 +779,7 @@ func ViewIssue(ctx *context.Context) { } prConfig := prUnit.PullRequestsConfig() - ctx.Data["AllowMerge"] = ctx.Data["IsRepositoryWriter"] + ctx.Data["AllowMerge"] = ctx.Repo.CanWrite(models.UnitTypeCode) if err := pull.CheckUserAllowedToMerge(ctx.User); err != nil { if !models.IsErrNotAllowedToMerge(err) { ctx.ServerError("CheckUserAllowedToMerge", err) diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 388093fc2911e..788aef9191cc2 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -397,16 +397,7 @@ func RegisterRoutes(m *macaron.Macaron) { reqRepoReleaseWriter := context.RequireRepoWriter(models.UnitTypeReleases) reqRepoWikiWriter := context.RequireRepoWriter(models.UnitTypeWiki) reqRepoPullsWriter := context.RequireRepoWriter(models.UnitTypePullRequests) - reqRepoIssuesOrPullsWriter := func() macaron.Handler { - return func(ctx *context.Context) { - if !ctx.IsSigned || (!ctx.Repo.CanWrite(models.UnitTypeIssues) && - !ctx.Repo.CanWrite(models.UnitTypePullRequests) && - !ctx.User.IsAdmin) { - ctx.NotFound(ctx.Req.RequestURI, nil) - return - } - } - } + reqRepoIssuesOrPullsWriter := context.RequireRepoWriterOr(models.UnitTypeIssues, models.UnitTypePullRequests) // ***** START: Organization ***** m.Group("/org", func() { @@ -476,7 +467,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/fork", func() { m.Combo("/:repoid").Get(repo.Fork). Post(bindIgnErr(auth.CreateRepoForm{}), repo.ForkPost) - }, context.RepoIDAssignment(), context.UnitTypes(), context.LoadRepoUnits(), context.CheckUnit(models.UnitTypeCode)) + }, context.RepoIDAssignment(), context.UnitTypes(), context.CheckUnit(models.UnitTypeCode)) }, reqSignIn) m.Group("/:username/:reponame", func() { @@ -527,7 +518,7 @@ func RegisterRoutes(m *macaron.Macaron) { }, func(ctx *context.Context) { ctx.Data["PageIsSettings"] = true }) - }, reqSignIn, context.RepoAssignment(), reqRepoAdmin, context.UnitTypes(), context.LoadRepoUnits(), context.RepoRef()) + }, reqSignIn, context.RepoAssignment(), reqRepoAdmin, context.UnitTypes(), context.RepoRef()) m.Get("/:username/:reponame/action/:action", reqSignIn, context.RepoAssignment(), repo.Action) @@ -616,7 +607,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/restore", repo.RestoreBranchPost) }, reqRepoCodeWriter, repo.MustBeNotBare) - }, reqSignIn, context.RepoAssignment(), context.UnitTypes(), context.LoadRepoUnits()) + }, reqSignIn, context.RepoAssignment(), context.UnitTypes()) // Releases m.Group("/:username/:reponame", func() { @@ -645,7 +636,7 @@ func RegisterRoutes(m *macaron.Macaron) { } ctx.Data["CommitsCount"] = ctx.Repo.CommitsCount }) - }, context.RepoAssignment(), context.UnitTypes(), context.LoadRepoUnits(), context.CheckUnit(models.UnitTypeReleases)) + }, context.RepoAssignment(), context.UnitTypes(), context.CheckUnit(models.UnitTypeReleases)) m.Group("/:username/:reponame", func() { m.Post("/topics", repo.TopicsPost) @@ -740,18 +731,18 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/compare/:before([a-z0-9]{40})\\.\\.\\.:after([a-z0-9]{40})", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.MustBeNotBare, context.CheckUnit(models.UnitTypeCode), repo.CompareDiff) - }, ignSignIn, context.RepoAssignment(), context.UnitTypes(), context.LoadRepoUnits()) + }, ignSignIn, context.RepoAssignment(), context.UnitTypes()) m.Group("/:username/:reponame", func() { m.Get("/stars", repo.Stars) m.Get("/watchers", repo.Watchers) m.Get("/search", context.CheckUnit(models.UnitTypeCode), repo.Search) - }, ignSignIn, context.RepoAssignment(), context.RepoRef(), context.UnitTypes(), context.LoadRepoUnits()) + }, ignSignIn, context.RepoAssignment(), context.RepoRef(), context.UnitTypes()) m.Group("/:username", func() { m.Group("/:reponame", func() { m.Get("", repo.SetEditorconfigIfExists, repo.Home) m.Get("\\.git$", repo.SetEditorconfigIfExists, repo.Home) - }, ignSignIn, context.RepoAssignment(), context.RepoRef(), context.UnitTypes(), context.LoadRepoUnits()) + }, ignSignIn, context.RepoAssignment(), context.RepoRef(), context.UnitTypes()) m.Group("/:reponame", func() { m.Group("\\.git/info/lfs", func() { diff --git a/templates/repo/bare.tmpl b/templates/repo/bare.tmpl index f1e1d585425c5..ec4be2bdea689 100644 --- a/templates/repo/bare.tmpl +++ b/templates/repo/bare.tmpl @@ -5,7 +5,7 @@ <div class="ui grid"> <div class="sixteen wide column content"> {{template "base/alert" .}} - {{if .IsRepositoryWriter}} + {{if .CanWriteCode}} <h4 class="ui top attached header"> {{.i18n.Tr "repo.quick_guide"}} </h4> diff --git a/templates/repo/issue/labels.tmpl b/templates/repo/issue/labels.tmpl index 38d914520635b..c89d3740ee7f6 100644 --- a/templates/repo/issue/labels.tmpl +++ b/templates/repo/issue/labels.tmpl @@ -4,7 +4,7 @@ <div class="ui container"> <div class="navbar"> {{template "repo/issue/navbar" .}} - {{if .IsRepositoryWriter}} + {{if or .CanWriteIssues .CanWritePulls}} <div class="ui right"> <div class="ui green new-label button">{{.i18n.Tr "repo.issues.new_label"}}</div> </div> @@ -57,7 +57,7 @@ {{template "base/alert" .}} <div class="ui black label">{{.i18n.Tr "repo.issues.label_count" .NumLabels}}</div> <div class="label list"> - {{if and $.IsRepositoryWriter (eq .NumLabels 0)}} + {{if and (or $.CanWriteIssues $.CanWritePulls) (eq .NumLabels 0)}} <div class="ui centered grid"> <div class="twelve wide column eight wide computer column"> <div class="ui attached left aligned segment"> @@ -105,7 +105,7 @@ <a class="ui right open-issues" href="{{$.RepoLink}}/issues?labels={{.ID}}"><i class="octicon octicon-issue-opened"></i> {{$.i18n.Tr "repo.issues.label_open_issues" .NumOpenIssues}}</a> </div> <div class="three wide column"> - {{if $.IsRepositoryWriter}} + {{if or $.CanWriteIssues $.CanWritePulls}} <a class="ui right delete-button" href="#" data-url="{{$.RepoLink}}/labels/delete" data-id="{{.ID}}"><i class="octicon octicon-trashcan"></i> {{$.i18n.Tr "repo.issues.label_delete"}}</a> <a class="ui right edit-label-button" href="#" data-id="{{.ID}}" data-title="{{.Name}}" data-description="{{.Description}}" data-color={{.Color}}><i class="octicon octicon-pencil"></i> {{$.i18n.Tr "repo.issues.label_edit"}}</a> {{end}} @@ -117,7 +117,7 @@ </div> </div> -{{if .IsRepositoryWriter}} +{{if or $.CanWriteIssues $.CanWritePulls}} <div class="ui small basic delete modal"> <div class="ui icon header"> <i class="trash icon"></i> diff --git a/templates/repo/issue/milestone_new.tmpl b/templates/repo/issue/milestone_new.tmpl index bd2f79389ac0a..30a7d7ebbb3d0 100644 --- a/templates/repo/issue/milestone_new.tmpl +++ b/templates/repo/issue/milestone_new.tmpl @@ -4,7 +4,7 @@ <div class="ui container"> <div class="navbar"> {{template "repo/issue/navbar" .}} - {{if and .IsRepositoryWriter .PageIsEditMilestone}} + {{if and (or .CanWriteIssues .CanWritePulls) .PageIsEditMilestone}} <div class="ui right floated secondary menu"> <a class="ui green button" href="{{$.RepoLink}}/milestones/new">{{.i18n.Tr "repo.milestones.new"}}</a> </div> diff --git a/templates/repo/issue/milestones.tmpl b/templates/repo/issue/milestones.tmpl index 75bd8db8ed35c..7b98c0cf56f46 100644 --- a/templates/repo/issue/milestones.tmpl +++ b/templates/repo/issue/milestones.tmpl @@ -4,7 +4,7 @@ <div class="ui container"> <div class="navbar"> {{template "repo/issue/navbar" .}} - {{if .IsRepositoryWriter}} + {{if or .CanWriteIssues .CanWritePulls}} <div class="ui right"> <a class="ui green button" href="{{$.Link}}/new">{{.i18n.Tr "repo.milestones.new"}}</a> </div> @@ -67,7 +67,7 @@ {{if .TotalTrackedTime}}<i class="octicon octicon-clock"></i> {{.TotalTrackedTime|Sec2Time}}{{end}} </span> </div> - {{if $.IsRepositoryWriter}} + {{if or $.CanWriteIssues $.CanWritePulls}} <div class="ui right operate"> <a href="{{$.Link}}/{{.ID}}/edit" data-id={{.ID}} data-title={{.Name}}><i class="octicon octicon-pencil"></i> {{$.i18n.Tr "repo.issues.label_edit"}}</a> {{if .IsClosed}} @@ -111,7 +111,7 @@ </div> </div> -{{if .IsRepositoryWriter}} +{{if or .CanWriteIssues .CanWritePulls}} <div class="ui small basic delete modal"> <div class="ui icon header"> <i class="trash icon"></i> diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index fa7a05512ddec..b696cbcfdc057 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -2,7 +2,7 @@ <div class="ui segment metas"> {{template "repo/issue/branch_selector_field" .}} - <div class="ui {{if not .IsRepositoryWriter}}disabled{{end}} floating jump select-label dropdown"> + <div class="ui {{if not (or .CanWriteIssues .CanWritePulls)}}disabled{{end}} floating jump select-label dropdown"> <span class="text"> <strong>{{.i18n.Tr "repo.issues.new.labels"}}</strong> <span class="octicon octicon-gear"></span> @@ -27,7 +27,7 @@ <div class="ui divider"></div> - <div class="ui {{if not .IsRepositoryWriter}}disabled{{end}} floating jump select-milestone dropdown"> + <div class="ui {{if not (or .CanWriteIssues .CanWritePulls)}}disabled{{end}} floating jump select-milestone dropdown"> <span class="text"> <strong>{{.i18n.Tr "repo.issues.new.milestone"}}</strong> <span class="octicon octicon-gear"></span> @@ -68,7 +68,7 @@ <div class="ui divider"></div> <input id="assignee_id" name="assignee_id" type="hidden" value="{{.assignee_id}}"> - <div class="ui {{if not .IsRepositoryWriter}}disabled{{end}} floating jump select-assignees-modify dropdown"> + <div class="ui {{if not (or .CanWriteIssues .CanWritePulls)}}disabled{{end}} floating jump select-assignees-modify dropdown"> <span class="text"> <strong>{{.i18n.Tr "repo.issues.new.assignees"}}</strong> <span class="octicon octicon-gear"></span> @@ -223,7 +223,7 @@ {{if .Issue.IsOverdue}} <span style="color: red;">{{.i18n.Tr "repo.issues.due_date_overdue"}}</span> {{end}} - {{if and .IsSigned .IsRepositoryWriter}} + {{if and .IsSigned (or .CanWriteIssues .CanWritePulls)}} <br/> <a style="cursor:pointer;" onclick="toggleDeadlineForm();"><i class="edit icon"></i>{{$.i18n.Tr "repo.issues.due_date_form_edit"}}</a> - <a style="cursor:pointer;" onclick="updateDeadline('');"><i class="remove icon"></i>{{$.i18n.Tr "repo.issues.due_date_form_remove"}}</a> @@ -233,7 +233,7 @@ <p><i>{{.i18n.Tr "repo.issues.due_date_not_set"}}</i></p> {{end}} - {{if and .IsSigned .IsRepositoryWriter}} + {{if and .IsSigned (or .CanWriteIssues .CanWritePulls)}} <div {{if ne .Issue.DeadlineUnix 0}} style="display: none;"{{end}} id="deadlineForm"> <form class="ui fluid action input" action="{{AppSubUrl}}/api/v1/repos/{{.Repository.Owner.Name}}/{{.Repository.Name}}/issues/{{.Issue.Index}}" method="post" id="update-issue-deadline-form" onsubmit="setDeadline();return false;"> {{$.CsrfTokenHtml}} From 40d552c1373ba12c22913408dd8f1d8bb4f34447 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 11 Nov 2018 11:44:30 +0800 Subject: [PATCH 03/31] refactor permission struct and add some copyright heads --- models/repo_permission.go | 135 ++++++++++++++++++ models/repo_unit.go | 5 +- modules/context/permission.go | 64 --------- modules/context/repo.go | 29 +--- routers/api/v1/api.go | 2 +- routers/api/v1/repo/collaborators.go | 1 + routers/api/v1/repo/file.go | 1 + routers/api/v1/repo/issue_label.go | 1 + routers/api/v1/repo/label.go | 1 + routers/api/v1/repo/pull.go | 2 +- routers/api/v1/repo/repo.go | 1 + routers/repo/branch.go | 1 + routers/repo/issue.go | 4 +- routers/repo/release.go | 1 + routers/repo/wiki.go | 2 +- templates/repo/issue/view_content.tmpl | 4 +- .../repo/issue/view_content/sidebar.tmpl | 10 +- templates/repo/issue/view_title.tmpl | 2 +- 18 files changed, 161 insertions(+), 105 deletions(-) create mode 100644 models/repo_permission.go delete mode 100644 modules/context/permission.go diff --git a/models/repo_permission.go b/models/repo_permission.go new file mode 100644 index 0000000000000..c90163ba74304 --- /dev/null +++ b/models/repo_permission.go @@ -0,0 +1,135 @@ +// Copyright 2018 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 models + +// Permission contains all the permissions related variables to a repository for a user +type Permission struct { + AccessMode AccessMode + Units []*RepoUnit + UnitsMode map[UnitType]AccessMode +} + +// IsOwner returns true if current user is the owner of repository. +func (p *Permission) IsOwner() bool { + return p.AccessMode >= AccessModeOwner +} + +// IsAdmin returns true if current user has admin or higher access of repository. +func (p *Permission) IsAdmin() bool { + return p.AccessMode >= AccessModeAdmin +} + +// HasAccess returns true if the current user has at least read access to any unit of this repository +func (p *Permission) HasAccess() bool { + if p.UnitsMode == nil { + return p.AccessMode >= AccessModeRead + } + return len(p.UnitsMode) > 0 +} + +// UnitAccessMode returns current user accessmode to the specify unit of the repository +func (p *Permission) UnitAccessMode(unitType UnitType) AccessMode { + if p.UnitsMode == nil { + return p.AccessMode + } + return p.UnitsMode[unitType] +} + +// CanAccess returns true if user has read access to the unit of the repository +func (p *Permission) CanAccess(unitType UnitType) bool { + return p.UnitAccessMode(unitType) >= AccessModeRead +} + +// CanWrite returns true if user could write to this unit +func (p *Permission) CanWrite(unitType UnitType) bool { + return p.UnitAccessMode(unitType) >= AccessModeWrite +} + +// CanWriteIssuesOrPulls returns true if isPull is true and user could write to pull requests and +// returns true if isPull is false and user could write to issues +func (p *Permission) CanWriteIssuesOrPulls(isPull bool) bool { + if isPull { + return p.CanWrite(UnitTypePullRequests) + } + return p.CanWrite(UnitTypeIssues) +} + +// GetUserRepoPermission returns the user permissions to the repository +func GetUserRepoPermission(repo *Repository, user *User) (Permission, error) { + return getUserRepoPermission(x, repo, user) +} + +func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permission, err error) { + // anonymous user visit private repo. TODO: anonymous user visit public unit of private repo??? + if user == nil && repo.IsPrivate { + perm.AccessMode = AccessModeNone + return + } + + if err = repo.getUnits(e); err != nil { + return + } + + perm.Units = repo.Units + + // anonymous visit public repo + if user == nil { + perm.AccessMode = AccessModeRead + return + } + + // Admin has super access or user is the owner of the repository + if user.IsAdmin || user.ID == repo.OwnerID { + perm.AccessMode = AccessModeOwner + return + } + + // plain user + perm.AccessMode, err = accessLevel(e, user.ID, repo) + if err != nil { + return + } + + if err = repo.getOwner(e); err != nil { + return + } + if !repo.Owner.IsOrganization() { + return + } + + teams, err := getUserRepoTeams(e, repo.OwnerID, user.ID, repo.ID) + if err != nil { + return + } + + perm.UnitsMode = make(map[UnitType]AccessMode) + for _, u := range repo.Units { + var found bool + for _, team := range teams { + if team.unitEnabled(e, u.Type) { + m := perm.UnitsMode[u.Type] + if m < team.Authorize { + perm.UnitsMode[u.Type] = team.Authorize + } + found = true + } + } + + if !found && !repo.IsPrivate { + perm.UnitsMode[u.Type] = AccessModeRead + } + } + + perm.Units = make([]*RepoUnit, 0, len(repo.Units)) + for t, _ := range perm.UnitsMode { + for _, u := range repo.Units { + if u.Type == t { + perm.Units = append(perm.Units, u) + } + } + } + + return +} diff --git a/models/repo_unit.go b/models/repo_unit.go index 1e1778356a06d..9eaec884bb11a 100644 --- a/models/repo_unit.go +++ b/models/repo_unit.go @@ -166,10 +166,7 @@ func (r *RepoUnit) IssuesConfig() *IssuesConfig { func (r *RepoUnit) ExternalTrackerConfig() *ExternalTrackerConfig { return r.Config.(*ExternalTrackerConfig) } + func getUnitsByRepoID(e Engine, repoID int64) (units []*RepoUnit, err error) { return units, e.Where("repo_id = ?", repoID).Find(&units) } - -func getUnitsByRepoIDAndIDs(e Engine, repoID int64, types []UnitType) (units []*RepoUnit, err error) { - return units, e.Where("repo_id = ?", repoID).In("`type`", types).Find(&units) -} diff --git a/modules/context/permission.go b/modules/context/permission.go deleted file mode 100644 index 96f14fb714349..0000000000000 --- a/modules/context/permission.go +++ /dev/null @@ -1,64 +0,0 @@ -// Copyright 2018 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 context - -import ( - "fmt" - - "code.gitea.io/gitea/models" -) - -// Permission contains all the permissions related variables to a repository -type Permission struct { - AccessMode models.AccessMode - Units []*models.RepoUnit - UnitsMode map[models.UnitType]models.AccessMode -} - -// IsOwner returns true if current user is the owner of repository. -func (p *Permission) IsOwner() bool { - return p.AccessMode >= models.AccessModeOwner -} - -// IsAdmin returns true if current user has admin or higher access of repository. -func (p *Permission) IsAdmin() bool { - return p.AccessMode >= models.AccessModeAdmin -} - -// HasAccess returns true if the current user has at least read access to any unit of this repository -func (p *Permission) HasAccess() bool { - if p.UnitsMode == nil { - return p.AccessMode >= models.AccessModeRead - } - return len(p.UnitsMode) > 0 -} - -// UnitAccessMode returns current user accessmode to the specify unit of the repository -func (p *Permission) UnitAccessMode(unitType models.UnitType) models.AccessMode { - if p.UnitsMode == nil { - return p.AccessMode - } - return p.UnitsMode[unitType] -} - -// CanAccess returns true if user has read access to the unit of the repository -func (p *Permission) CanAccess(unitType models.UnitType) bool { - return p.UnitAccessMode(unitType) >= models.AccessModeRead -} - -// CanWrite returns true if user could write to this unit -func (p *Permission) CanWrite(unitType models.UnitType) bool { - fmt.Println("p:", unitType, p.UnitsMode) - return p.UnitAccessMode(unitType) >= models.AccessModeWrite -} - -// CanWriteIssuesOrPulls returns true if isPull is true and user could write to pull requests and -// returns true if isPull is false and user could write to issues -func (p *Permission) CanWriteIssuesOrPulls(isPull bool) bool { - if isPull { - return p.CanWrite(models.UnitTypePullRequests) - } - return p.CanWrite(models.UnitTypeIssues) -} diff --git a/modules/context/repo.go b/modules/context/repo.go index c2cd187074179..931c994276da0 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -32,7 +32,7 @@ type PullRequest struct { // Repository contains information to operate a repository type Repository struct { - Permission + models.Permission IsWatching bool IsViewBranch bool IsViewTag bool @@ -201,29 +201,12 @@ func RedirectToRepo(ctx *Context, redirectRepoID int64) { } func repoAssignment(ctx *Context, repo *models.Repository) { - var userID int64 - // Admin has super access. - if ctx.IsSigned && ctx.User.IsAdmin { - ctx.Repo.Permission.AccessMode = models.AccessModeOwner - } else { - if ctx.User != nil { - userID = ctx.User.ID - } - mode, err := models.AccessLevel(userID, repo) - if err != nil { - ctx.ServerError("AccessLevel", err) - return - } - ctx.Repo.Permission.AccessMode = mode - } - - err := repo.LoadUnitsByUserID(userID, ctx.Repo.IsAdmin()) + var err error + ctx.Repo.Permission, err = models.GetUserRepoPermission(repo, ctx.User) if err != nil { - ctx.ServerError("LoadUnitsByUserID", err) + ctx.ServerError("AccessLevel", err) return } - ctx.Repo.Permission.Units = repo.Units - ctx.Repo.Permission.UnitsMode = repo.UnitsMode // Check access. if ctx.Repo.Permission.AccessMode == models.AccessModeNone { @@ -269,10 +252,6 @@ func RepoIDAssignment() macaron.Handler { return } - if err = repo.GetOwner(); err != nil { - ctx.ServerError("GetOwner", err) - return - } repoAssignment(ctx, repo) } } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 52f84c7e0b6b8..31f746dfaafbb 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1,5 +1,5 @@ // Copyright 2015 The Gogs Authors. All rights reserved. -// Copyright 2018 The Gitea Authors. All rights reserved. +// Copyright 2016 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. diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index 200392e903e4a..5bb80abb1a3cd 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -1,4 +1,5 @@ // Copyright 2016 The Gogs Authors. All rights reserved. +// Copyright 2018 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. diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index a04dd39e0242f..88d7a1ea2b714 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -1,4 +1,5 @@ // Copyright 2014 The Gogs Authors. All rights reserved. +// Copyright 2018 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. diff --git a/routers/api/v1/repo/issue_label.go b/routers/api/v1/repo/issue_label.go index f006ee888e6e0..715dd0ed771fc 100644 --- a/routers/api/v1/repo/issue_label.go +++ b/routers/api/v1/repo/issue_label.go @@ -1,4 +1,5 @@ // Copyright 2016 The Gogs Authors. All rights reserved. +// Copyright 2018 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. diff --git a/routers/api/v1/repo/label.go b/routers/api/v1/repo/label.go index 91ed30ec006ed..2efaace5bdc46 100644 --- a/routers/api/v1/repo/label.go +++ b/routers/api/v1/repo/label.go @@ -1,4 +1,5 @@ // Copyright 2016 The Gogs Authors. All rights reserved. +// Copyright 2018 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. diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index c0891793b25b1..916490767ee1c 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -405,7 +405,7 @@ func EditPullRequest(ctx *context.APIContext, form api.EditPullRequestOption) { } } - if ctx.Repo.IsWriter() && (form.Labels != nil && len(form.Labels) > 0) { + if ctx.Repo.CanWrite(models.UnitTypePullRequests) && (form.Labels != nil && len(form.Labels) > 0) { labels, err := models.GetLabelsInRepoByIDs(ctx.Repo.Repository.ID, form.Labels) if err != nil { ctx.Error(500, "GetLabelsInRepoByIDsError", err) diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index f75b6807f1981..a56f696974002 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -1,4 +1,5 @@ // Copyright 2014 The Gogs Authors. All rights reserved. +// Copyright 2018 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. diff --git a/routers/repo/branch.go b/routers/repo/branch.go index 2c009f55ab700..295aeaa24ba60 100644 --- a/routers/repo/branch.go +++ b/routers/repo/branch.go @@ -1,4 +1,5 @@ // Copyright 2014 The Gogs Authors. All rights reserved. +// Copyright 2018 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. diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 43dea84480cbf..304d110b69a72 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1,4 +1,5 @@ // Copyright 2014 The Gogs Authors. All rights reserved. +// Copyright 2018 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. @@ -818,8 +819,9 @@ func ViewIssue(ctx *context.Context) { ctx.Data["NumParticipants"] = len(participants) ctx.Data["Issue"] = issue ctx.Data["ReadOnly"] = true - ctx.Data["IsIssueOwner"] = ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) || (ctx.IsSigned && issue.IsPoster(ctx.User.ID)) ctx.Data["SignInLink"] = setting.AppSubURL + "/user/login?redirect_to=" + ctx.Data["Link"].(string) + ctx.Data["IsIssuePoster"] = ctx.IsSigned && issue.IsPoster(ctx.User.ID) + ctx.Data["IsIssueWriter"] = ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) ctx.HTML(200, tplIssueView) } diff --git a/routers/repo/release.go b/routers/repo/release.go index 33abf07412316..5a869520f0b0a 100644 --- a/routers/repo/release.go +++ b/routers/repo/release.go @@ -1,4 +1,5 @@ // Copyright 2014 The Gogs Authors. All rights reserved. +// Copyright 2018 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. diff --git a/routers/repo/wiki.go b/routers/repo/wiki.go index b319c258d22b4..f1bd191286ed0 100644 --- a/routers/repo/wiki.go +++ b/routers/repo/wiki.go @@ -1,4 +1,5 @@ // Copyright 2015 The Gogs Authors. All rights reserved. +// Copyright 2018 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. @@ -239,7 +240,6 @@ func WikiPages(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("repo.wiki.pages") ctx.Data["PageIsWiki"] = true ctx.Data["CanWriteWiki"] = ctx.Repo.CanWrite(models.UnitTypeWiki) - if !ctx.Repo.Repository.HasWiki() { ctx.Redirect(ctx.Repo.RepoLink + "/wiki") return diff --git a/templates/repo/issue/view_content.tmpl b/templates/repo/issue/view_content.tmpl index e133ee9dc3700..54531cf4d8a11 100644 --- a/templates/repo/issue/view_content.tmpl +++ b/templates/repo/issue/view_content.tmpl @@ -20,7 +20,7 @@ <span class="text grey"><a {{if gt .Issue.Poster.ID 0}}href="{{.Issue.Poster.HomeLink}}"{{end}}>{{.Issue.Poster.Name}}</a> {{.i18n.Tr "repo.issues.commented_at" .Issue.HashTag $createdStr | Safe}}</span> <div class="ui right actions"> {{template "repo/issue/view_content/add_reaction" Dict "ctx" $ "ActionURL" (Printf "%s/issues/%d/reactions" $.RepoLink .Issue.Index) }} - {{if .IsIssueOwner}} + {{if or .IsIssueWriter .IsIssuePoster}} <div class="item action"> <a class="edit-content" href="#"><i class="octicon octicon-pencil"></i></a> </div> @@ -79,7 +79,7 @@ {{.CsrfTokenHtml}} <input id="status" name="status" type="hidden"> <div class="text right"> - {{if and .IsIssueOwner (not .DisableStatusChange)}} + {{if and (or .IsIssueWriter .IsIssuePoster) (not .DisableStatusChange)}} {{if .Issue.IsClosed}} <div id="status-button" class="ui green basic button" tabindex="6" data-status="{{.i18n.Tr "repo.issues.reopen_issue"}}" data-status-and-comment="{{.i18n.Tr "repo.issues.reopen_comment_issue"}}" data-status-val="reopen"> {{.i18n.Tr "repo.issues.reopen_issue"}} diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index b696cbcfdc057..ee8ed0e1a1e94 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -2,7 +2,7 @@ <div class="ui segment metas"> {{template "repo/issue/branch_selector_field" .}} - <div class="ui {{if not (or .CanWriteIssues .CanWritePulls)}}disabled{{end}} floating jump select-label dropdown"> + <div class="ui {{if not .IsIssueWriter}}disabled{{end}} floating jump select-label dropdown"> <span class="text"> <strong>{{.i18n.Tr "repo.issues.new.labels"}}</strong> <span class="octicon octicon-gear"></span> @@ -27,7 +27,7 @@ <div class="ui divider"></div> - <div class="ui {{if not (or .CanWriteIssues .CanWritePulls)}}disabled{{end}} floating jump select-milestone dropdown"> + <div class="ui {{if not .IsIssueWriter}}disabled{{end}} floating jump select-milestone dropdown"> <span class="text"> <strong>{{.i18n.Tr "repo.issues.new.milestone"}}</strong> <span class="octicon octicon-gear"></span> @@ -68,7 +68,7 @@ <div class="ui divider"></div> <input id="assignee_id" name="assignee_id" type="hidden" value="{{.assignee_id}}"> - <div class="ui {{if not (or .CanWriteIssues .CanWritePulls)}}disabled{{end}} floating jump select-assignees-modify dropdown"> + <div class="ui {{if not .IsIssueWriter}}disabled{{end}} floating jump select-assignees-modify dropdown"> <span class="text"> <strong>{{.i18n.Tr "repo.issues.new.assignees"}}</strong> <span class="octicon octicon-gear"></span> @@ -223,7 +223,7 @@ {{if .Issue.IsOverdue}} <span style="color: red;">{{.i18n.Tr "repo.issues.due_date_overdue"}}</span> {{end}} - {{if and .IsSigned (or .CanWriteIssues .CanWritePulls)}} + {{if .IsIssueWriter}} <br/> <a style="cursor:pointer;" onclick="toggleDeadlineForm();"><i class="edit icon"></i>{{$.i18n.Tr "repo.issues.due_date_form_edit"}}</a> - <a style="cursor:pointer;" onclick="updateDeadline('');"><i class="remove icon"></i>{{$.i18n.Tr "repo.issues.due_date_form_remove"}}</a> @@ -233,7 +233,7 @@ <p><i>{{.i18n.Tr "repo.issues.due_date_not_set"}}</i></p> {{end}} - {{if and .IsSigned (or .CanWriteIssues .CanWritePulls)}} + {{if .IsIssueWriter}} <div {{if ne .Issue.DeadlineUnix 0}} style="display: none;"{{end}} id="deadlineForm"> <form class="ui fluid action input" action="{{AppSubUrl}}/api/v1/repos/{{.Repository.Owner.Name}}/{{.Repository.Name}}/issues/{{.Issue.Index}}" method="post" id="update-issue-deadline-form" onsubmit="setDeadline();return false;"> {{$.CsrfTokenHtml}} diff --git a/templates/repo/issue/view_title.tmpl b/templates/repo/issue/view_title.tmpl index d69f699ac18ab..b9edd0b562063 100644 --- a/templates/repo/issue/view_title.tmpl +++ b/templates/repo/issue/view_title.tmpl @@ -6,7 +6,7 @@ <input value="{{.Issue.Title}}"> </div> </h1> - {{if .IsIssueOwner}} + {{if or .IsIssueWriter .IsIssuePoster}} <div class="four wide column"> <div class="edit-zone text right"> <div id="edit-title" class="ui basic green not-in-edit button">{{.i18n.Tr "repo.issues.edit"}}</div> From d80fea2c74df1f326c573bbe7d1505070bbca906 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 11 Nov 2018 13:21:10 +0800 Subject: [PATCH 04/31] remove unused codes --- models/repo.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/models/repo.go b/models/repo.go index e851246e6f363..b7be50e9d17a5 100644 --- a/models/repo.go +++ b/models/repo.go @@ -191,9 +191,8 @@ type Repository struct { IsMirror bool `xorm:"INDEX"` *Mirror `xorm:"-"` - ExternalMetas map[string]string `xorm:"-"` - Units []*RepoUnit `xorm:"-"` - UnitsMode map[UnitType]AccessMode `xorm:"-"` + ExternalMetas map[string]string `xorm:"-"` + Units []*RepoUnit `xorm:"-"` IsFork bool `xorm:"INDEX NOT NULL DEFAULT false"` ForkID int64 `xorm:"INDEX"` @@ -370,16 +369,11 @@ func (repo *Repository) getUnitsByUserID(e Engine, userID int64, isAdmin bool) ( return err } - repo.UnitsMode = make(map[UnitType]AccessMode) // unique var newRepoUnits = make([]*RepoUnit, 0, len(repo.Units)) for _, u := range repo.Units { for _, team := range teams { if team.unitEnabled(e, u.Type) { - m := repo.UnitsMode[u.Type] - if m < team.Authorize { - repo.UnitsMode[u.Type] = team.Authorize - } newRepoUnits = append(newRepoUnits, u) break } From fb4a2cbd43f18fc21209e8ad9772dbfccbbd0ef1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 11 Nov 2018 13:49:22 +0800 Subject: [PATCH 05/31] fix routes units check --- modules/context/repo.go | 26 ++++++++++++++++++++++++-- routers/routes/routes.go | 36 ++++++++++++++++++++---------------- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/modules/context/repo.go b/modules/context/repo.go index 931c994276da0..88ffc182c1b27 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -640,10 +640,10 @@ func RequireRepoAdmin() macaron.Handler { } } -// RequireRepoWriter returns a macaron middleware for requiring repository write to code permission +// RequireRepoWriter returns a macaron middleware for requiring repository write to the specify unitType func RequireRepoWriter(unitType models.UnitType) macaron.Handler { return func(ctx *Context) { - if !ctx.IsSigned || (!ctx.Repo.CanWrite(unitType) && !ctx.User.IsAdmin) { + if !ctx.Repo.CanWrite(unitType) { ctx.NotFound(ctx.Req.RequestURI, nil) return } @@ -662,6 +662,28 @@ func RequireRepoWriterOr(unitTypes ...models.UnitType) macaron.Handler { } } +// RequireRepoReader returns a macaron middleware for requiring repository read to the specify unitType +func RequireRepoReader(unitType models.UnitType) macaron.Handler { + return func(ctx *Context) { + if !ctx.Repo.CanAccess(unitType) { + ctx.NotFound(ctx.Req.RequestURI, nil) + return + } + } +} + +// RequireRepoReaderOr returns a macaron middleware for requiring repository write to one of the unit permission +func RequireRepoReaderOr(unitTypes ...models.UnitType) macaron.Handler { + return func(ctx *Context) { + for _, unitType := range unitTypes { + if ctx.Repo.CanAccess(unitType) { + return + } + } + ctx.NotFound(ctx.Req.RequestURI, nil) + } +} + // CheckUnit will check whether unit type is enabled func CheckUnit(unitType models.UnitType) macaron.Handler { return func(ctx *Context) { diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 788aef9191cc2..0372f6ec76e20 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -394,10 +394,14 @@ func RegisterRoutes(m *macaron.Macaron) { reqRepoAdmin := context.RequireRepoAdmin() reqRepoCodeWriter := context.RequireRepoWriter(models.UnitTypeCode) + reqRepoCodeReader := context.RequireRepoReader(models.UnitTypeCode) reqRepoReleaseWriter := context.RequireRepoWriter(models.UnitTypeReleases) + reqRepoReleaseReader := context.RequireRepoReader(models.UnitTypeReleases) reqRepoWikiWriter := context.RequireRepoWriter(models.UnitTypeWiki) + reqRepoIssueWriter := context.RequireRepoWriter(models.UnitTypeIssues) reqRepoPullsWriter := context.RequireRepoWriter(models.UnitTypePullRequests) reqRepoIssuesOrPullsWriter := context.RequireRepoWriterOr(models.UnitTypeIssues, models.UnitTypePullRequests) + reqRepoIssuesOrPullsReader := context.RequireRepoReaderOr(models.UnitTypeIssues, models.UnitTypePullRequests) // ***** START: Organization ***** m.Group("/org", func() { @@ -467,7 +471,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/fork", func() { m.Combo("/:repoid").Get(repo.Fork). Post(bindIgnErr(auth.CreateRepoForm{}), repo.ForkPost) - }, context.RepoIDAssignment(), context.UnitTypes(), context.CheckUnit(models.UnitTypeCode)) + }, context.RepoIDAssignment(), context.UnitTypes(), reqRepoCodeReader) }, reqSignIn) m.Group("/:username/:reponame", func() { @@ -526,7 +530,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/issues", func() { m.Combo("/new").Get(context.RepoRef(), repo.NewIssue). Post(bindIgnErr(auth.CreateIssueForm{}), repo.NewIssuePost) - }, context.CheckUnit(models.UnitTypeIssues)) + }, reqRepoIssueWriter) // FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest. // So they can apply their own enable/disable logic on routers. m.Group("/issues", func() { @@ -558,7 +562,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("", repo.UpdateCommentContent) m.Post("/delete", repo.DeleteComment) m.Post("/reactions/:action", bindIgnErr(auth.ReactionForm{}), repo.ChangeCommentReaction) - }, context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests)) + }, reqRepoIssuesOrPullsReader) m.Group("/labels", func() { m.Post("/new", bindIgnErr(auth.CreateLabelForm{}), repo.NewLabel) m.Post("/edit", bindIgnErr(auth.CreateLabelForm{}), repo.UpdateLabel) @@ -636,7 +640,7 @@ func RegisterRoutes(m *macaron.Macaron) { } ctx.Data["CommitsCount"] = ctx.Repo.CommitsCount }) - }, context.RepoAssignment(), context.UnitTypes(), context.CheckUnit(models.UnitTypeReleases)) + }, context.RepoAssignment(), context.UnitTypes(), reqRepoReleaseReader) m.Group("/:username/:reponame", func() { m.Post("/topics", repo.TopicsPost) @@ -646,8 +650,8 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("", func() { m.Get("/^:type(issues|pulls)$", repo.RetrieveLabels, repo.Issues) m.Get("/^:type(issues|pulls)$/:index", repo.ViewIssue) - m.Get("/labels/", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.RetrieveLabels, repo.Labels) - m.Get("/milestones", context.CheckAnyUnit(models.UnitTypeIssues, models.UnitTypePullRequests), repo.Milestones) + m.Get("/labels/", reqRepoIssuesOrPullsReader, repo.RetrieveLabels, repo.Labels) + m.Get("/milestones", reqRepoIssuesOrPullsReader, repo.Milestones) }, context.RepoRef()) m.Group("/wiki", func() { @@ -670,13 +674,13 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/activity", func() { m.Get("", repo.Activity) m.Get("/:period", repo.Activity) - }, context.RepoRef(), repo.MustBeNotBare, context.CheckAnyUnit(models.UnitTypePullRequests, models.UnitTypeIssues, models.UnitTypeReleases)) + }, context.RepoRef(), repo.MustBeNotBare, context.RequireRepoReaderOr(models.UnitTypePullRequests, models.UnitTypeIssues, models.UnitTypeReleases)) - m.Get("/archive/*", repo.MustBeNotBare, context.CheckUnit(models.UnitTypeCode), repo.Download) + m.Get("/archive/*", repo.MustBeNotBare, reqRepoCodeReader, repo.Download) m.Group("/branches", func() { m.Get("", repo.Branches) - }, repo.MustBeNotBare, context.RepoRef(), context.CheckUnit(models.UnitTypeCode)) + }, repo.MustBeNotBare, context.RepoRef(), reqRepoCodeReader) m.Group("/pulls/:index", func() { m.Get(".diff", repo.DownloadPullDiff) @@ -700,7 +704,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/blob/:sha", context.RepoRefByType(context.RepoRefBlob), repo.DownloadByID) // "/*" route is deprecated, and kept for backward compatibility m.Get("/*", context.RepoRefByType(context.RepoRefLegacy), repo.SingleDownload) - }, repo.MustBeNotBare, context.CheckUnit(models.UnitTypeCode)) + }, repo.MustBeNotBare, reqRepoCodeReader) m.Group("/commits", func() { m.Get("/branch/*", context.RepoRefByType(context.RepoRefBranch), repo.RefCommits) @@ -708,12 +712,12 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/commit/*", context.RepoRefByType(context.RepoRefCommit), repo.RefCommits) // "/*" route is deprecated, and kept for backward compatibility m.Get("/*", context.RepoRefByType(context.RepoRefLegacy), repo.RefCommits) - }, repo.MustBeNotBare, context.CheckUnit(models.UnitTypeCode)) + }, repo.MustBeNotBare, reqRepoCodeReader) m.Group("", func() { m.Get("/graph", repo.Graph) m.Get("/commit/:sha([a-f0-9]{7,40})$", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.Diff) - }, repo.MustBeNotBare, context.RepoRef(), context.CheckUnit(models.UnitTypeCode)) + }, repo.MustBeNotBare, context.RepoRef(), reqRepoCodeReader) m.Group("/src", func() { m.Get("/branch/*", context.RepoRefByType(context.RepoRefBranch), repo.Home) @@ -725,17 +729,17 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("", func() { m.Get("/forks", repo.Forks) - }, context.RepoRef(), context.CheckUnit(models.UnitTypeCode)) + }, context.RepoRef(), reqRepoCodeReader) m.Get("/commit/:sha([a-f0-9]{7,40})\\.:ext(patch|diff)", - repo.MustBeNotBare, context.CheckUnit(models.UnitTypeCode), repo.RawDiff) + repo.MustBeNotBare, reqRepoCodeReader, repo.RawDiff) m.Get("/compare/:before([a-z0-9]{40})\\.\\.\\.:after([a-z0-9]{40})", repo.SetEditorconfigIfExists, - repo.SetDiffViewStyle, repo.MustBeNotBare, context.CheckUnit(models.UnitTypeCode), repo.CompareDiff) + repo.SetDiffViewStyle, repo.MustBeNotBare, reqRepoCodeReader, repo.CompareDiff) }, ignSignIn, context.RepoAssignment(), context.UnitTypes()) m.Group("/:username/:reponame", func() { m.Get("/stars", repo.Stars) m.Get("/watchers", repo.Watchers) - m.Get("/search", context.CheckUnit(models.UnitTypeCode), repo.Search) + m.Get("/search", reqRepoCodeReader, repo.Search) }, ignSignIn, context.RepoAssignment(), context.RepoRef(), context.UnitTypes()) m.Group("/:username", func() { From a21bfde663bf1452e55d7ce41aaef57bbcddb1ca Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 11 Nov 2018 22:21:00 +0800 Subject: [PATCH 06/31] improve permission check --- cmd/serv.go | 12 +----------- models/repo.go | 5 ----- models/repo_permission.go | 17 +++++++++++++++-- modules/context/repo.go | 3 --- modules/private/internal.go | 16 +++++++++++----- routers/private/internal.go | 19 ++++++++++++++++--- routers/repo/http.go | 37 ++++++++++--------------------------- routers/repo/view.go | 4 ++-- 8 files changed, 55 insertions(+), 58 deletions(-) diff --git a/cmd/serv.go b/cmd/serv.go index ca042e2b2b70d..9e018aecc2620 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -236,7 +236,7 @@ func runServ(c *cli.Context) error { user.Name, repoPath) } - mode, err := private.AccessLevel(user.ID, repo.ID) + mode, err := private.CheckUnitUser(user.ID, repo.ID, user.IsAdmin, unitType) if err != nil { fail("Internal error", "Failed to check access: %v", err) } else if *mode < requestedMode { @@ -249,16 +249,6 @@ func runServ(c *cli.Context) error { user.Name, requestedMode, repoPath) } - check, err := private.CheckUnitUser(user.ID, repo.ID, user.IsAdmin, unitType) - if err != nil { - fail("You do not have allowed for this action", "Failed to access internal api: [user.Name: %s, repoPath: %s]", user.Name, repoPath) - } - if !check { - fail("You do not have allowed for this action", - "User %s does not have allowed access to repository %s 's code", - user.Name, repoPath) - } - os.Setenv(models.EnvPusherName, user.Name) os.Setenv(models.EnvPusherID, fmt.Sprintf("%d", user.ID)) } diff --git a/models/repo.go b/models/repo.go index b7be50e9d17a5..3be17680df3fe 100644 --- a/models/repo.go +++ b/models/repo.go @@ -337,11 +337,6 @@ func (repo *Repository) checkUnitUser(e Engine, userID int64, isAdmin bool, unit return false } -// LoadUnitsByUserID loads units according userID's permissions -func (repo *Repository) LoadUnitsByUserID(userID int64, isAdmin bool) error { - return repo.getUnitsByUserID(x, userID, isAdmin) -} - func (repo *Repository) getUnitsByUserID(e Engine, userID int64, isAdmin bool) (err error) { if repo.Units != nil { return nil diff --git a/models/repo_permission.go b/models/repo_permission.go index c90163ba74304..af8bd538de49e 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -32,7 +32,12 @@ func (p *Permission) HasAccess() bool { // UnitAccessMode returns current user accessmode to the specify unit of the repository func (p *Permission) UnitAccessMode(unitType UnitType) AccessMode { if p.UnitsMode == nil { - return p.AccessMode + for _, u := range p.Units { + if u.Type == unitType { + return p.AccessMode + } + } + return AccessModeNone } return p.UnitsMode[unitType] } @@ -62,7 +67,8 @@ func GetUserRepoPermission(repo *Repository, user *User) (Permission, error) { } func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permission, err error) { - // anonymous user visit private repo. TODO: anonymous user visit public unit of private repo??? + // anonymous user visit private repo. + // TODO: anonymous user visit public unit of private repo??? if user == nil && repo.IsPrivate { perm.AccessMode = AccessModeNone return @@ -99,6 +105,13 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss return } + // Collaborators on organization repos will not be limited by teams units + if isCollaborator, err := repo.isCollaborator(e, user.ID); err != nil { + return perm, err + } else if isCollaborator { + return perm, nil + } + teams, err := getUserRepoTeams(e, repo.OwnerID, user.ID, repo.ID) if err != nil { return diff --git a/modules/context/repo.go b/modules/context/repo.go index 88ffc182c1b27..14f5fb2cc8cd5 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -422,9 +422,6 @@ func RepoAssignment() macaron.Handler { ctx.Repo.PullRequest.HeadInfo = ctx.Repo.BranchName } } - - // Reset repo units as otherwise user specific units wont be loaded later - ctx.Repo.Repository.Units = nil } ctx.Data["PullRequestCtx"] = ctx.Repo.PullRequest diff --git a/modules/private/internal.go b/modules/private/internal.go index f4ac1c515a4bf..351ff57213f84 100644 --- a/modules/private/internal.go +++ b/modules/private/internal.go @@ -51,20 +51,26 @@ func newInternalRequest(url, method string) *httplib.Request { } // CheckUnitUser check whether user could visit the unit of this repository -func CheckUnitUser(userID, repoID int64, isAdmin bool, unitType models.UnitType) (bool, error) { +func CheckUnitUser(userID, repoID int64, isAdmin bool, unitType models.UnitType) (*models.AccessMode, error) { reqURL := setting.LocalURL + fmt.Sprintf("api/internal/repositories/%d/user/%d/checkunituser?isAdmin=%t&unitType=%d", repoID, userID, isAdmin, unitType) log.GitLogger.Trace("AccessLevel: %s", reqURL) resp, err := newInternalRequest(reqURL, "GET").Response() if err != nil { - return false, err + return nil, err } defer resp.Body.Close() - if resp.StatusCode == 200 { - return true, nil + if resp.StatusCode != 200 { + return nil, fmt.Errorf("Failed to CheckUnitUser: %s", decodeJSONError(resp).Err) } - return false, nil + + var a models.AccessMode + if err := json.NewDecoder(resp.Body).Decode(&a); err != nil { + return nil, err + } + + return &a, nil } // AccessLevel returns the Access a user has to a repository. Will return NoneAccess if the diff --git a/routers/private/internal.go b/routers/private/internal.go index 23e0122642b37..c9f9e4fcb8b76 100644 --- a/routers/private/internal.go +++ b/routers/private/internal.go @@ -70,11 +70,24 @@ func CheckUnitUser(ctx *macaron.Context) { }) return } - if repo.CheckUnitUser(userID, ctx.QueryBool("isAdmin"), models.UnitType(ctx.QueryInt("unitType"))) { - ctx.PlainText(200, []byte("success")) + + user, err := models.GetUserByID(userID) + if err != nil { + ctx.JSON(500, map[string]interface{}{ + "err": err.Error(), + }) return } - ctx.PlainText(404, []byte("no access")) + + perm, err := models.GetUserRepoPermission(repo, user) + if err != nil { + ctx.JSON(500, map[string]interface{}{ + "err": err.Error(), + }) + return + } + + ctx.JSON(200, perm.UnitAccessMode(models.UnitType(ctx.QueryInt("unitType")))) } // RegisterRoutes registers all internal APIs routes to web application. diff --git a/routers/repo/http.go b/routers/repo/http.go index 5b469754adf52..35c03af7cfb6e 100644 --- a/routers/repo/http.go +++ b/routers/repo/http.go @@ -182,36 +182,19 @@ func HTTP(ctx *context.Context) { } } - if !isPublicPull { - has, err := models.HasAccess(authUser.ID, repo, accessMode) - if err != nil { - ctx.ServerError("HasAccess", err) - return - } else if !has { - if accessMode == models.AccessModeRead { - has, err = models.HasAccess(authUser.ID, repo, models.AccessModeWrite) - if err != nil { - ctx.ServerError("HasAccess2", err) - return - } else if !has { - ctx.HandleText(http.StatusForbidden, "User permission denied") - return - } - } else { - ctx.HandleText(http.StatusForbidden, "User permission denied") - return - } - } + perm, err := models.GetUserRepoPermission(repo, authUser) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return + } - if !isPull && repo.IsMirror { - ctx.HandleText(http.StatusForbidden, "mirror repository is read-only") - return - } + if perm.UnitAccessMode(unitType) < accessMode { + ctx.HandleText(http.StatusForbidden, "User permission denied") + return } - if !repo.CheckUnitUser(authUser.ID, authUser.IsAdmin, unitType) { - ctx.HandleText(http.StatusForbidden, fmt.Sprintf("User %s does not have allowed access to repository %s 's code", - authUser.Name, repo.RepoPath())) + if !isPull && repo.IsMirror { + ctx.HandleText(http.StatusForbidden, "mirror repository is read-only") return } diff --git a/routers/repo/view.go b/routers/repo/view.go index f00329f9a9bee..78a305aa28aa3 100644 --- a/routers/repo/view.go +++ b/routers/repo/view.go @@ -282,9 +282,9 @@ func renderFile(ctx *context.Context, entry *git.TreeEntry, treeLink, rawLink st // Home render repository home page func Home(ctx *context.Context) { - if len(ctx.Repo.Repository.Units) > 0 { + if len(ctx.Repo.Units) > 0 { var firstUnit *models.Unit - for _, repoUnit := range ctx.Repo.Repository.Units { + for _, repoUnit := range ctx.Repo.Units { if repoUnit.Type == models.UnitTypeCode { renderCode(ctx) return From 422ba40b5dcd9cd24f58d931e27480907250555b Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Mon, 12 Nov 2018 11:23:04 +0800 Subject: [PATCH 07/31] add unit tests for permission --- models/fixtures/repo_unit.yml | 86 +++++++++++- models/fixtures/repository.yml | 2 +- models/fixtures/team.yml | 27 ++++ models/fixtures/team_repo.yml | 18 +++ models/fixtures/team_unit.yml | 15 ++ models/fixtures/team_user.yml | 18 +++ models/repo.go | 52 +------ models/repo_permission.go | 28 +++- models/repo_permission_test.go | 246 +++++++++++++++++++++++++++++++++ modules/context/repo.go | 18 --- routers/api/v1/api.go | 2 +- routers/repo/issue.go | 4 +- 12 files changed, 441 insertions(+), 75 deletions(-) create mode 100644 models/repo_permission_test.go diff --git a/models/fixtures/repo_unit.yml b/models/fixtures/repo_unit.yml index 581f9d6ed5600..2fde65b05bfe6 100644 --- a/models/fixtures/repo_unit.yml +++ b/models/fixtures/repo_unit.yml @@ -108,4 +108,88 @@ repo_id: 33 type: 5 config: "{}" - created_unix: 1535593231 \ No newline at end of file + created_unix: 1535593231 + +- + id: 17 + repo_id: 4 + type: 4 + config: "{}" + created_unix: 946684810 + +- + id: 18 + repo_id: 4 + type: 5 + config: "{}" + created_unix: 946684810 + +- + id: 19 + repo_id: 4 + type: 1 + config: "{}" + created_unix: 946684810 + +- + id: 20 + repo_id: 4 + type: 2 + config: "{\"EnableTimetracker\":true,\"AllowOnlyContributorsToTrackTime\":true}" + created_unix: 946684810 + +- + id: 21 + repo_id: 4 + type: 3 + config: "{\"IgnoreWhitespaceConflicts\":false,\"AllowMerge\":true,\"AllowRebase\":true,\"AllowSquash\":true}" + created_unix: 946684810 + +- + id: 22 + repo_id: 2 + type: 4 + config: "{}" + created_unix: 946684810 + +- + id: 23 + repo_id: 2 + type: 5 + config: "{}" + created_unix: 946684810 + +- + id: 24 + repo_id: 2 + type: 1 + config: "{}" + created_unix: 946684810 + +- + id: 25 + repo_id: 32 + type: 1 + config: "{}" + created_unix: 1524304355 + +- + id: 26 + repo_id: 32 + type: 2 + config: "{}" + created_unix: 1524304355 + +- + id: 27 + repo_id: 24 + type: 1 + config: "{}" + created_unix: 1524304355 + +- + id: 28 + repo_id: 24 + type: 2 + config: "{}" + created_unix: 1524304355 \ No newline at end of file diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index c2987b9658678..aa96656530aec 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -391,7 +391,7 @@ is_mirror: false - - id: 32 + id: 32 # org public repo owner_id: 3 lower_name: repo21 name: repo21 diff --git a/models/fixtures/team.yml b/models/fixtures/team.yml index 4b4a1d798b4e6..2d0dd9cd56ca1 100644 --- a/models/fixtures/team.yml +++ b/models/fixtures/team.yml @@ -51,3 +51,30 @@ authorize: 4 # owner num_repos: 2 num_members: 1 + +- + id: 7 + org_id: 3 + lower_name: test_team + name: test_team + authorize: 2 # write + num_repos: 1 + num_members: 1 + +- + id: 8 + org_id: 17 + lower_name: test_team + name: test_team + authorize: 2 # write + num_repos: 1 + num_members: 1 + +- + id: 9 + org_id: 17 + lower_name: review_team + name: review_team + authorize: 1 # read + num_repos: 1 + num_members: 1 \ No newline at end of file diff --git a/models/fixtures/team_repo.yml b/models/fixtures/team_repo.yml index b324e094151f4..a523a90b204d2 100644 --- a/models/fixtures/team_repo.yml +++ b/models/fixtures/team_repo.yml @@ -45,3 +45,21 @@ org_id: 3 team_id: 1 repo_id: 32 + +- + id: 9 + org_id: 3 + team_id: 7 + repo_id: 32 + +- + id: 10 + org_id: 17 + team_id: 8 + repo_id: 24 + +- + id: 11 + org_id: 17 + team_id: 9 + repo_id: 24 \ No newline at end of file diff --git a/models/fixtures/team_unit.yml b/models/fixtures/team_unit.yml index ad5466a5c1336..943745c000f9b 100644 --- a/models/fixtures/team_unit.yml +++ b/models/fixtures/team_unit.yml @@ -207,3 +207,18 @@ id: 42 team_id: 6 type: 7 + +- + id: 43 + team_id: 7 + type: 2 # issues + +- + id: 44 + team_id: 8 + type: 2 # issues + +- + id: 45 + team_id: 9 + type: 1 # code \ No newline at end of file diff --git a/models/fixtures/team_user.yml b/models/fixtures/team_user.yml index b1dfcdfdef8aa..e20b5c9684bff 100644 --- a/models/fixtures/team_user.yml +++ b/models/fixtures/team_user.yml @@ -44,4 +44,22 @@ id: 8 org_id: 19 team_id: 6 + uid: 20 + +- + id: 9 + org_id: 3 + team_id: 7 + uid: 15 + +- + id: 10 + org_id: 17 + team_id: 8 + uid: 2 + +- + id: 11 + org_id: 17 + team_id: 9 uid: 20 \ No newline at end of file diff --git a/models/repo.go b/models/repo.go index 3be17680df3fe..5db73c566e20b 100644 --- a/models/repo.go +++ b/models/repo.go @@ -325,58 +325,16 @@ func (repo *Repository) CheckUnitUser(userID int64, isAdmin bool, unitType UnitT } func (repo *Repository) checkUnitUser(e Engine, userID int64, isAdmin bool, unitType UnitType) bool { - if err := repo.getUnitsByUserID(e, userID, isAdmin); err != nil { + user, err := getUserByID(e, userID) + if err != nil { return false } - - for _, unit := range repo.Units { - if unit.Type == unitType { - return true - } - } - return false -} - -func (repo *Repository) getUnitsByUserID(e Engine, userID int64, isAdmin bool) (err error) { - if repo.Units != nil { - return nil - } - - if err = repo.getUnits(e); err != nil { - return err - } else if err = repo.getOwner(e); err != nil { - return err - } - - if !repo.Owner.IsOrganization() || userID == 0 || isAdmin || !repo.IsPrivate { - return nil - } - - // Collaborators will not be limited - if isCollaborator, err := repo.isCollaborator(e, userID); err != nil { - return err - } else if isCollaborator { - return nil - } - - teams, err := getUserRepoTeams(e, repo.OwnerID, userID, repo.ID) + perm, err := getUserRepoPermission(e, repo, user) if err != nil { - return err - } - - // unique - var newRepoUnits = make([]*RepoUnit, 0, len(repo.Units)) - for _, u := range repo.Units { - for _, team := range teams { - if team.unitEnabled(e, u.Type) { - newRepoUnits = append(newRepoUnits, u) - break - } - } + return false } - repo.Units = newRepoUnits - return nil + return perm.CanAccess(unitType) } // UnitEnabled if this repository has the given unit enabled diff --git a/models/repo_permission.go b/models/repo_permission.go index af8bd538de49e..5a1b40e919391 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -47,6 +47,16 @@ func (p *Permission) CanAccess(unitType UnitType) bool { return p.UnitAccessMode(unitType) >= AccessModeRead } +// CanAccessAny returns true if user has read access to any of the units of the repository +func (p *Permission) CanAccessAny(unitTypes ...UnitType) bool { + for _, u := range unitTypes { + if p.CanAccess(u) { + return true + } + } + return false +} + // CanWrite returns true if user could write to this unit func (p *Permission) CanWrite(unitType UnitType) bool { return p.UnitAccessMode(unitType) >= AccessModeWrite @@ -105,19 +115,23 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss return } - // Collaborators on organization repos will not be limited by teams units + perm.UnitsMode = make(map[UnitType]AccessMode) + + // Collaborators on organization if isCollaborator, err := repo.isCollaborator(e, user.ID); err != nil { return perm, err } else if isCollaborator { - return perm, nil + for _, u := range repo.Units { + perm.UnitsMode[u.Type] = perm.AccessMode + } } + // get units mode from teams teams, err := getUserRepoTeams(e, repo.OwnerID, user.ID, repo.ID) if err != nil { return } - perm.UnitsMode = make(map[UnitType]AccessMode) for _, u := range repo.Units { var found bool for _, team := range teams { @@ -130,13 +144,17 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss } } + // for a public repo on an organization, user have read permission on non-team defined units. if !found && !repo.IsPrivate { - perm.UnitsMode[u.Type] = AccessModeRead + if _, ok := perm.UnitsMode[u.Type]; !ok { + perm.UnitsMode[u.Type] = AccessModeRead + } } } + // remove no permission units perm.Units = make([]*RepoUnit, 0, len(repo.Units)) - for t, _ := range perm.UnitsMode { + for t := range perm.UnitsMode { for _, u := range repo.Units { if u.Type == t { perm.Units = append(perm.Units, u) diff --git a/models/repo_permission_test.go b/models/repo_permission_test.go new file mode 100644 index 0000000000000..570ad84a05779 --- /dev/null +++ b/models/repo_permission_test.go @@ -0,0 +1,246 @@ +// Copyright 2018 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 models + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRepoPermissionPublicNonOrgRepo(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + // public non-organization repo + repo := AssertExistsAndLoadBean(t, &Repository{ID: 4}).(*Repository) + assert.NoError(t, repo.getUnits(x)) + + // plain user + user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + perm, err := GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.False(t, perm.CanWrite(unit.Type)) + } + + // change to collaborator + assert.NoError(t, repo.AddCollaborator(user)) + perm, err = GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } + + // collaborator + collaborator := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User) + perm, err = GetUserRepoPermission(repo, collaborator) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } + + // owner + owner := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User) + perm, err = GetUserRepoPermission(repo, owner) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } + + // admin + admin := AssertExistsAndLoadBean(t, &User{ID: 1}).(*User) + perm, err = GetUserRepoPermission(repo, admin) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } +} + +func TestRepoPermissionPrivateNonOrgRepo(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + // private non-organization repo + repo := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository) + assert.NoError(t, repo.getUnits(x)) + + // plain user + user := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User) + perm, err := GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.False(t, perm.CanAccess(unit.Type)) + assert.False(t, perm.CanWrite(unit.Type)) + } + + // change to collaborator to default write access + assert.NoError(t, repo.AddCollaborator(user)) + perm, err = GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } + + assert.NoError(t, repo.ChangeCollaborationAccessMode(user.ID, AccessModeRead)) + perm, err = GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.False(t, perm.CanWrite(unit.Type)) + } + + // owner + owner := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + perm, err = GetUserRepoPermission(repo, owner) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } + + // admin + admin := AssertExistsAndLoadBean(t, &User{ID: 1}).(*User) + perm, err = GetUserRepoPermission(repo, admin) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } +} + +func TestRepoPermissionPublicOrgRepo(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + // public organization repo + repo := AssertExistsAndLoadBean(t, &Repository{ID: 32}).(*Repository) + assert.NoError(t, repo.getUnits(x)) + + // plain user + user := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User) + perm, err := GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.False(t, perm.CanWrite(unit.Type)) + } + + // change to collaborator to default write access + assert.NoError(t, repo.AddCollaborator(user)) + perm, err = GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } + + assert.NoError(t, repo.ChangeCollaborationAccessMode(user.ID, AccessModeRead)) + perm, err = GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.False(t, perm.CanWrite(unit.Type)) + } + + // org memeber team owner + owner := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + perm, err = GetUserRepoPermission(repo, owner) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } + + // org memeber team tester + member := AssertExistsAndLoadBean(t, &User{ID: 15}).(*User) + perm, err = GetUserRepoPermission(repo, member) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + } + assert.True(t, perm.CanWrite(UnitTypeIssues)) + assert.False(t, perm.CanWrite(UnitTypeCode)) + + // admin + admin := AssertExistsAndLoadBean(t, &User{ID: 1}).(*User) + perm, err = GetUserRepoPermission(repo, admin) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } +} + +func TestRepoPermissionPrivateOrgRepo(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + // private organization repo + repo := AssertExistsAndLoadBean(t, &Repository{ID: 24}).(*Repository) + assert.NoError(t, repo.getUnits(x)) + + // plain user + user := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User) + perm, err := GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.False(t, perm.CanAccess(unit.Type)) + assert.False(t, perm.CanWrite(unit.Type)) + } + + // change to collaborator to default write access + assert.NoError(t, repo.AddCollaborator(user)) + perm, err = GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } + + assert.NoError(t, repo.ChangeCollaborationAccessMode(user.ID, AccessModeRead)) + perm, err = GetUserRepoPermission(repo, user) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.False(t, perm.CanWrite(unit.Type)) + } + + // org memeber team owner + owner := AssertExistsAndLoadBean(t, &User{ID: 15}).(*User) + perm, err = GetUserRepoPermission(repo, owner) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } + + // org memeber team tester + tester := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + perm, err = GetUserRepoPermission(repo, tester) + assert.NoError(t, err) + assert.True(t, perm.CanWrite(UnitTypeIssues)) + assert.False(t, perm.CanWrite(UnitTypeCode)) + assert.False(t, perm.CanAccess(UnitTypeCode)) + + // org memeber team reviewer + reviewer := AssertExistsAndLoadBean(t, &User{ID: 20}).(*User) + perm, err = GetUserRepoPermission(repo, reviewer) + assert.NoError(t, err) + assert.False(t, perm.CanAccess(UnitTypeIssues)) + assert.False(t, perm.CanWrite(UnitTypeCode)) + assert.True(t, perm.CanAccess(UnitTypeCode)) + + // admin + admin := AssertExistsAndLoadBean(t, &User{ID: 1}).(*User) + perm, err = GetUserRepoPermission(repo, admin) + assert.NoError(t, err) + for _, unit := range repo.Units { + assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanWrite(unit.Type)) + } +} diff --git a/modules/context/repo.go b/modules/context/repo.go index 14f5fb2cc8cd5..1b51939466a9a 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -681,24 +681,6 @@ func RequireRepoReaderOr(unitTypes ...models.UnitType) macaron.Handler { } } -// CheckUnit will check whether unit type is enabled -func CheckUnit(unitType models.UnitType) macaron.Handler { - return func(ctx *Context) { - if !ctx.Repo.Repository.UnitEnabled(unitType) { - ctx.NotFound("CheckUnit", fmt.Errorf("%s: %v", ctx.Tr("units.error.unit_not_allowed"), unitType)) - } - } -} - -// CheckAnyUnit will check whether any of the unit types are enabled -func CheckAnyUnit(unitTypes ...models.UnitType) macaron.Handler { - return func(ctx *Context) { - if !ctx.Repo.Repository.AnyUnitEnabled(unitTypes...) { - ctx.NotFound("CheckAnyUnit", fmt.Errorf("%s: %v", ctx.Tr("units.error.unit_not_allowed"), unitTypes)) - } - } -} - // GitHookService checks if repository Git hooks service has been enabled. func GitHookService() macaron.Handler { return func(ctx *Context) { diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 31f746dfaafbb..585e4c6e5bfb6 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -310,7 +310,7 @@ func orgAssignment(args ...bool) macaron.Handler { } func mustEnableIssues(ctx *context.APIContext) { - if !ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) { + if !ctx.Repo.CanAccess(models.UnitTypeIssues) { ctx.Status(404) return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 304d110b69a72..ec8a25cf633e8 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -64,8 +64,8 @@ var ( // MustEnableIssues check if repository enable internal issues func MustEnableIssues(ctx *context.Context) { - if !ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) && - !ctx.Repo.Repository.UnitEnabled(models.UnitTypeExternalTracker) { + if !ctx.Repo.CanAccess(models.UnitTypeIssues) && + !ctx.Repo.CanAccess(models.UnitTypeExternalTracker) { ctx.NotFound("MustEnableIssues", nil) return } From dae595b0f6dbc7e6a057eafb5ff8560c4cbdbf19 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Mon, 12 Nov 2018 11:32:11 +0800 Subject: [PATCH 08/31] fix typo --- models/repo_permission_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/models/repo_permission_test.go b/models/repo_permission_test.go index 570ad84a05779..055bc3a4423a2 100644 --- a/models/repo_permission_test.go +++ b/models/repo_permission_test.go @@ -148,7 +148,7 @@ func TestRepoPermissionPublicOrgRepo(t *testing.T) { assert.False(t, perm.CanWrite(unit.Type)) } - // org memeber team owner + // org member team owner owner := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) perm, err = GetUserRepoPermission(repo, owner) assert.NoError(t, err) @@ -157,7 +157,7 @@ func TestRepoPermissionPublicOrgRepo(t *testing.T) { assert.True(t, perm.CanWrite(unit.Type)) } - // org memeber team tester + // org member team tester member := AssertExistsAndLoadBean(t, &User{ID: 15}).(*User) perm, err = GetUserRepoPermission(repo, member) assert.NoError(t, err) @@ -210,7 +210,7 @@ func TestRepoPermissionPrivateOrgRepo(t *testing.T) { assert.False(t, perm.CanWrite(unit.Type)) } - // org memeber team owner + // org member team owner owner := AssertExistsAndLoadBean(t, &User{ID: 15}).(*User) perm, err = GetUserRepoPermission(repo, owner) assert.NoError(t, err) @@ -219,7 +219,7 @@ func TestRepoPermissionPrivateOrgRepo(t *testing.T) { assert.True(t, perm.CanWrite(unit.Type)) } - // org memeber team tester + // org member team tester tester := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) perm, err = GetUserRepoPermission(repo, tester) assert.NoError(t, err) @@ -227,7 +227,7 @@ func TestRepoPermissionPrivateOrgRepo(t *testing.T) { assert.False(t, perm.CanWrite(UnitTypeCode)) assert.False(t, perm.CanAccess(UnitTypeCode)) - // org memeber team reviewer + // org member team reviewer reviewer := AssertExistsAndLoadBean(t, &User{ID: 20}).(*User) perm, err = GetUserRepoPermission(repo, reviewer) assert.NoError(t, err) From 6bed0d4422273d49996054a4cfc082e6d8d19ce8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Mon, 12 Nov 2018 13:36:03 +0800 Subject: [PATCH 09/31] fix tests --- models/fixtures/user.yml | 4 ++-- models/org_test.go | 3 ++- models/user_test.go | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index b3850e35991c1..dc3de2a2e1851 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -47,7 +47,7 @@ avatar_email: user3@example.com num_repos: 3 num_members: 2 - num_teams: 2 + num_teams: 3 - id: 4 @@ -266,7 +266,7 @@ num_repos: 2 is_active: true num_members: 2 - num_teams: 1 + num_teams: 3 - id: 18 diff --git a/models/org_test.go b/models/org_test.go index c54e7a93bf19d..4790f6997137e 100644 --- a/models/org_test.go +++ b/models/org_test.go @@ -84,9 +84,10 @@ func TestUser_GetTeams(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) org := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User) assert.NoError(t, org.GetTeams()) - if assert.Len(t, org.Teams, 2) { + if assert.Len(t, org.Teams, 3) { assert.Equal(t, int64(1), org.Teams[0].ID) assert.Equal(t, int64(2), org.Teams[1].ID) + assert.Equal(t, int64(7), org.Teams[2].ID) } } diff --git a/models/user_test.go b/models/user_test.go index 4713b6864c45f..ba700d36599da 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -169,7 +169,7 @@ func TestGetOrgRepositoryIDs(t *testing.T) { accessibleRepos, err := user2.GetOrgRepositoryIDs() assert.NoError(t, err) // User 2's team has access to private repos 3, 5, repo 32 is a public repo of the organization - assert.Equal(t, []int64{3, 5, 32}, accessibleRepos) + assert.Equal(t, []int64{3, 5, 23, 24, 32}, accessibleRepos) accessibleRepos, err = user4.GetOrgRepositoryIDs() assert.NoError(t, err) From d5ba3a040ca8c70ff0442cfe4b6924f13f02d9bd Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Mon, 12 Nov 2018 14:29:30 +0800 Subject: [PATCH 10/31] fix some routes --- routers/api/v1/api.go | 54 ++++++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 585e4c6e5bfb6..49baff36bcb9a 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -204,6 +204,24 @@ func reqAdmin() macaron.Handler { } } +func reqOwner() macaron.Handler { + return func(ctx *context.Context) { + if !ctx.Repo.IsAdmin() { + ctx.Error(403) + return + } + } +} + +func reqRepoReader(unitType models.UnitType) macaron.Handler { + return func(ctx *context.Context) { + if !ctx.Repo.CanAccess(unitType) { + ctx.Error(403) + return + } + } +} + func reqRepoWriter(unitTypes ...models.UnitType) macaron.Handler { return func(ctx *context.Context) { for _, unitType := range unitTypes { @@ -317,15 +335,15 @@ func mustEnableIssues(ctx *context.APIContext) { } func mustAllowPulls(ctx *context.Context) { - if !ctx.Repo.Repository.AllowsPulls() { + if !(ctx.Repo.Repository.CanEnablePulls() && ctx.Repo.CanAccess(models.UnitTypePullRequests)) { ctx.Status(404) return } } func mustEnableIssuesOrPulls(ctx *context.Context) { - if !ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) && - !ctx.Repo.Repository.AllowsPulls() { + if !ctx.Repo.CanAccess(models.UnitTypeIssues) && + !(ctx.Repo.Repository.CanEnablePulls() && ctx.Repo.CanAccess(models.UnitTypePullRequests)) { ctx.Status(404) return } @@ -519,10 +537,10 @@ func RegisterRoutes(m *macaron.Macaron) { }, mustEnableIssuesOrPulls) m.Group("/labels", func() { m.Combo("").Get(repo.ListLabels). - Post(reqToken(), bind(api.CreateLabelOption{}), repo.CreateLabel) + Post(reqToken(), reqRepoWriter(models.UnitTypeIssues, models.UnitTypePullRequests), bind(api.CreateLabelOption{}), repo.CreateLabel) m.Combo("/:id").Get(repo.GetLabel). - Patch(reqToken(), bind(api.EditLabelOption{}), repo.EditLabel). - Delete(reqToken(), repo.DeleteLabel) + Patch(reqToken(), reqRepoWriter(models.UnitTypeIssues, models.UnitTypePullRequests), bind(api.EditLabelOption{}), repo.EditLabel). + Delete(reqToken(), reqRepoWriter(models.UnitTypeIssues, models.UnitTypePullRequests), repo.DeleteLabel) }) m.Group("/milestones", func() { m.Combo("").Get(repo.ListMilestones). @@ -540,36 +558,36 @@ func RegisterRoutes(m *macaron.Macaron) { }) m.Group("/releases", func() { m.Combo("").Get(repo.ListReleases). - Post(reqToken(), reqRepoWriter(), context.ReferencesGitRepo(), bind(api.CreateReleaseOption{}), repo.CreateRelease) + Post(reqToken(), reqRepoWriter(models.UnitTypeReleases), context.ReferencesGitRepo(), bind(api.CreateReleaseOption{}), repo.CreateRelease) m.Group("/:id", func() { m.Combo("").Get(repo.GetRelease). - Patch(reqToken(), reqRepoWriter(), context.ReferencesGitRepo(), bind(api.EditReleaseOption{}), repo.EditRelease). - Delete(reqToken(), reqRepoWriter(), repo.DeleteRelease) + Patch(reqToken(), reqRepoWriter(models.UnitTypeReleases), context.ReferencesGitRepo(), bind(api.EditReleaseOption{}), repo.EditRelease). + Delete(reqToken(), reqRepoWriter(models.UnitTypeReleases), repo.DeleteRelease) m.Group("/assets", func() { m.Combo("").Get(repo.ListReleaseAttachments). - Post(reqToken(), reqRepoWriter(), repo.CreateReleaseAttachment) + Post(reqToken(), reqRepoWriter(models.UnitTypeReleases), repo.CreateReleaseAttachment) m.Combo("/:asset").Get(repo.GetReleaseAttachment). - Patch(reqToken(), reqRepoWriter(), bind(api.EditAttachmentOptions{}), repo.EditReleaseAttachment). - Delete(reqToken(), reqRepoWriter(), repo.DeleteReleaseAttachment) + Patch(reqToken(), reqRepoWriter(models.UnitTypeReleases), bind(api.EditAttachmentOptions{}), repo.EditReleaseAttachment). + Delete(reqToken(), reqRepoWriter(models.UnitTypeReleases), repo.DeleteReleaseAttachment) }) }) - }) - m.Post("/mirror-sync", reqToken(), reqRepoWriter(), repo.MirrorSync) + }, reqRepoReader(models.UnitTypeReleases)) + m.Post("/mirror-sync", reqToken(), reqRepoWriter(models.UnitTypeCode), repo.MirrorSync) m.Get("/editorconfig/:filename", context.RepoRef(), repo.GetEditorconfig) m.Group("/pulls", func() { m.Combo("").Get(bind(api.ListPullRequestsOptions{}), repo.ListPullRequests). - Post(reqToken(), reqRepoWriter(), bind(api.CreatePullRequestOption{}), repo.CreatePullRequest) + Post(reqToken(), bind(api.CreatePullRequestOption{}), repo.CreatePullRequest) m.Group("/:index", func() { m.Combo("").Get(repo.GetPullRequest). - Patch(reqToken(), reqRepoWriter(), bind(api.EditPullRequestOption{}), repo.EditPullRequest) + Patch(reqToken(), reqRepoWriter(models.UnitTypePullRequests), bind(api.EditPullRequestOption{}), repo.EditPullRequest) m.Combo("/merge").Get(repo.IsPullRequestMerged). - Post(reqToken(), reqRepoWriter(), bind(auth.MergePullRequestForm{}), repo.MergePullRequest) + Post(reqToken(), reqRepoWriter(models.UnitTypePullRequests), bind(auth.MergePullRequestForm{}), repo.MergePullRequest) }) }, mustAllowPulls, context.ReferencesGitRepo()) m.Group("/statuses", func() { m.Combo("/:sha").Get(repo.GetCommitStatuses). - Post(reqToken(), reqRepoWriter(), bind(api.CreateStatusOption{}), repo.NewCommitStatus) + Post(reqToken(), bind(api.CreateStatusOption{}), repo.NewCommitStatus) }) m.Group("/commits/:ref", func() { m.Get("/status", repo.GetCombinedCommitStatusByRef) From 426980d3e5deedde3cf73d438181f2b6b694fa4f Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Mon, 12 Nov 2018 15:28:55 +0800 Subject: [PATCH 11/31] fix api permission check --- routers/api/v1/api.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 49baff36bcb9a..f36af82203996 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -71,7 +71,6 @@ import ( "code.gitea.io/gitea/routers/api/v1/repo" _ "code.gitea.io/gitea/routers/api/v1/swagger" // for swagger generation "code.gitea.io/gitea/routers/api/v1/user" - "code.gitea.io/gitea/routers/api/v1/utils" api "code.gitea.io/sdk/gitea" "github.com/go-macaron/binding" @@ -154,15 +153,10 @@ func repoAssignment() macaron.Handler { repo.Owner = owner ctx.Repo.Repository = repo - if ctx.IsSigned && ctx.User.IsAdmin { - ctx.Repo.AccessMode = models.AccessModeOwner - } else { - mode, err := models.AccessLevel(utils.UserID(ctx), repo) - if err != nil { - ctx.Error(500, "AccessLevel", err) - return - } - ctx.Repo.AccessMode = mode + ctx.Repo.Permission, err = models.GetUserRepoPermission(repo, ctx.User) + if err != nil { + ctx.Error(500, "GetUserRepoPermission", err) + return } if !ctx.Repo.HasAccess() { From 50d12875666a0e1b2fc7f2e00d135a4688c43506 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Mon, 12 Nov 2018 22:16:42 +0800 Subject: [PATCH 12/31] improve permission check --- routers/api/v1/api.go | 49 ++++++++++++++++++++-------- routers/api/v1/repo/collaborators.go | 17 ---------- routers/api/v1/repo/file.go | 5 --- routers/api/v1/repo/label.go | 15 --------- routers/api/v1/repo/release.go | 12 ------- routers/api/v1/repo/repo.go | 10 ++---- routers/repo/activity.go | 6 ++-- routers/repo/issue.go | 8 ++--- routers/repo/wiki.go | 4 +-- 9 files changed, 47 insertions(+), 79 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index f36af82203996..b5eedf06e4738 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -189,7 +189,8 @@ func reqBasicAuth() macaron.Handler { } } -func reqAdmin() macaron.Handler { +// reqSiteAdmin user should be the site admin +func reqSiteAdmin() macaron.Handler { return func(ctx *context.Context) { if !ctx.IsSigned || !ctx.User.IsAdmin { ctx.Error(403) @@ -198,7 +199,18 @@ func reqAdmin() macaron.Handler { } } +// reqOwner user should be the owner of the repo. func reqOwner() macaron.Handler { + return func(ctx *context.Context) { + if !ctx.Repo.IsOwner() { + ctx.Error(403) + return + } + } +} + +// reqAdmin user should be an owner or a collaborator with admin write of a repository +func reqAdmin() macaron.Handler { return func(ctx *context.Context) { if !ctx.Repo.IsAdmin() { ctx.Error(403) @@ -216,6 +228,15 @@ func reqRepoReader(unitType models.UnitType) macaron.Handler { } } +func reqAnyRepoReader() macaron.Handler { + return func(ctx *context.Context) { + if !ctx.Repo.HasAccess() { + ctx.Error(403) + return + } + } +} + func reqRepoWriter(unitTypes ...models.UnitType) macaron.Handler { return func(ctx *context.Context) { for _, unitType := range unitTypes { @@ -457,7 +478,8 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/migrate", reqToken(), bind(auth.MigrateRepoForm{}), repo.Migrate) m.Group("/:username/:reponame", func() { - m.Combo("").Get(repo.Get).Delete(reqToken(), repo.Delete) + m.Combo("").Get(reqAnyRepoReader(), repo.Get). + Delete(reqToken(), reqOwner(), repo.Delete) m.Group("/hooks", func() { m.Combo("").Get(repo.ListHooks). Post(bind(api.CreateHookOption{}), repo.CreateHook) @@ -467,21 +489,21 @@ func RegisterRoutes(m *macaron.Macaron) { Delete(repo.DeleteHook) m.Post("/tests", context.RepoRef(), repo.TestHook) }) - }, reqToken(), reqRepoWriter(models.UnitTypeCode)) + }, reqToken(), reqAdmin()) 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.RepoRefByType(context.RepoRefAny), repo.GetRawFile) - m.Get("/archive/*", repo.GetArchive) + }, reqToken(), reqAdmin()) + m.Get("/raw/*", context.RepoRefByType(context.RepoRefAny), reqRepoReader(models.UnitTypeCode), repo.GetRawFile) + m.Get("/archive/*", reqRepoReader(models.UnitTypeCode), repo.GetArchive) m.Combo("/forks").Get(repo.ListForks). - Post(reqToken(), bind(api.CreateForkOption{}), repo.CreateFork) + Post(reqToken(), reqRepoReader(models.UnitTypeCode), bind(api.CreateForkOption{}), repo.CreateFork) m.Group("/branches", func() { m.Get("", repo.ListBranches) m.Get("/*", context.RepoRefByType(context.RepoRefBranch), repo.GetBranch) - }) + }, reqRepoReader(models.UnitTypeCode)) m.Group("/keys", func() { m.Combo("").Get(repo.ListDeployKeys). Post(bind(api.CreateKeyOption{}), repo.CreateDeployKey) @@ -491,7 +513,6 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/times", func() { m.Combo("").Get(repo.ListTrackedTimesByRepository) m.Combo("/:timetrackingusername").Get(repo.ListTrackedTimesByUser) - }, mustEnableIssues) m.Group("/issues", func() { m.Combo("").Get(repo.ListIssues). @@ -567,7 +588,7 @@ func RegisterRoutes(m *macaron.Macaron) { }) }, reqRepoReader(models.UnitTypeReleases)) m.Post("/mirror-sync", reqToken(), reqRepoWriter(models.UnitTypeCode), repo.MirrorSync) - m.Get("/editorconfig/:filename", context.RepoRef(), repo.GetEditorconfig) + m.Get("/editorconfig/:filename", context.RepoRef(), reqRepoReader(models.UnitTypeCode), repo.GetEditorconfig) m.Group("/pulls", func() { m.Combo("").Get(bind(api.ListPullRequestsOptions{}), repo.ListPullRequests). Post(reqToken(), bind(api.CreatePullRequestOption{}), repo.CreatePullRequest) @@ -582,15 +603,15 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/statuses", func() { m.Combo("/:sha").Get(repo.GetCommitStatuses). Post(reqToken(), bind(api.CreateStatusOption{}), repo.NewCommitStatus) - }) + }, reqRepoReader(models.UnitTypeCode)) m.Group("/commits/:ref", func() { m.Get("/status", repo.GetCombinedCommitStatusByRef) m.Get("/statuses", repo.GetCommitStatusesByRef) - }) + }, reqRepoReader(models.UnitTypeCode)) m.Group("/git", func() { m.Get("/refs", repo.GetGitAllRefs) m.Get("/refs/*", repo.GetGitRefs) - }) + }, reqRepoReader(models.UnitTypeCode)) }, repoAssignment()) }) @@ -659,7 +680,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/repos", bind(api.CreateRepoOption{}), admin.CreateRepo) }) }) - }, reqToken(), reqAdmin()) + }, reqToken(), reqSiteAdmin()) m.Group("/topics", func() { m.Get("/search", repo.TopicSearch) diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index 5bb80abb1a3cd..2f254f71033de 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -35,10 +35,6 @@ func ListCollaborators(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/UserList" - if !ctx.Repo.CanWrite(models.UnitTypeCode) { - ctx.Error(403, "", "User does not have push access") - return - } collaborators, err := ctx.Repo.Repository.GetCollaborators() if err != nil { ctx.Error(500, "ListCollaborators", err) @@ -79,10 +75,6 @@ func IsCollaborator(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "404": // "$ref": "#/responses/empty" - if !ctx.Repo.CanWrite(models.UnitTypeCode) { - ctx.Error(403, "", "User does not have push access") - return - } user, err := models.GetUserByName(ctx.Params(":collaborator")) if err != nil { if models.IsErrUserNotExist(err) { @@ -134,10 +126,6 @@ func AddCollaborator(ctx *context.APIContext, form api.AddCollaboratorOption) { // responses: // "204": // "$ref": "#/responses/empty" - if !ctx.Repo.CanWrite(models.UnitTypeCode) { - ctx.Error(403, "", "User does not have push access") - return - } collaborator, err := models.GetUserByName(ctx.Params(":collaborator")) if err != nil { if models.IsErrUserNotExist(err) { @@ -194,11 +182,6 @@ func DeleteCollaborator(ctx *context.APIContext) { // responses: // "204": // "$ref": "#/responses/empty" - if !ctx.Repo.CanWrite(models.UnitTypeCode) { - ctx.Error(403, "", "User does not have push access") - return - } - collaborator, err := models.GetUserByName(ctx.Params(":collaborator")) if err != nil { if models.IsErrUserNotExist(err) { diff --git a/routers/api/v1/repo/file.go b/routers/api/v1/repo/file.go index 88d7a1ea2b714..81f2332089d95 100644 --- a/routers/api/v1/repo/file.go +++ b/routers/api/v1/repo/file.go @@ -39,11 +39,6 @@ func GetRawFile(ctx *context.APIContext) { // responses: // 200: // description: success - if !ctx.Repo.CanAccess(models.UnitTypeCode) { - ctx.Status(404) - return - } - if ctx.Repo.Repository.IsBare { ctx.Status(404) return diff --git a/routers/api/v1/repo/label.go b/routers/api/v1/repo/label.go index 2efaace5bdc46..df7a37004191e 100644 --- a/routers/api/v1/repo/label.go +++ b/routers/api/v1/repo/label.go @@ -124,11 +124,6 @@ func CreateLabel(ctx *context.APIContext, form api.CreateLabelOption) { // responses: // "201": // "$ref": "#/responses/Label" - if !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.Repo.CanWrite(models.UnitTypePullRequests) { - ctx.Status(403) - return - } - label := &models.Label{ Name: form.Name, Color: form.Color, @@ -174,11 +169,6 @@ func EditLabel(ctx *context.APIContext, form api.EditLabelOption) { // responses: // "200": // "$ref": "#/responses/Label" - if !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.Repo.CanWrite(models.UnitTypePullRequests) { - ctx.Status(403) - return - } - label, err := models.GetLabelInRepoByID(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id")) if err != nil { if models.IsErrLabelNotExist(err) { @@ -227,11 +217,6 @@ func DeleteLabel(ctx *context.APIContext) { // responses: // "204": // "$ref": "#/responses/empty" - if !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.Repo.CanWrite(models.UnitTypePullRequests) { - ctx.Status(403) - return - } - if err := models.DeleteLabel(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id")); err != nil { ctx.Error(500, "DeleteLabel", err) return diff --git a/routers/api/v1/repo/release.go b/routers/api/v1/repo/release.go index 282ff582c509f..caa2905e93076 100644 --- a/routers/api/v1/repo/release.go +++ b/routers/api/v1/repo/release.go @@ -122,10 +122,6 @@ func CreateRelease(ctx *context.APIContext, form api.CreateReleaseOption) { // responses: // "201": // "$ref": "#/responses/Release" - if ctx.Repo.AccessMode < models.AccessModeWrite { - ctx.Status(403) - return - } rel, err := models.GetRelease(ctx.Repo.Repository.ID, form.TagName) if err != nil { if !models.IsErrReleaseNotExist(err) { @@ -209,10 +205,6 @@ func EditRelease(ctx *context.APIContext, form api.EditReleaseOption) { // responses: // "200": // "$ref": "#/responses/Release" - if ctx.Repo.AccessMode < models.AccessModeWrite { - ctx.Status(403) - return - } id := ctx.ParamsInt64(":id") rel, err := models.GetReleaseByID(id) if err != nil && !models.IsErrReleaseNotExist(err) { @@ -285,10 +277,6 @@ func DeleteRelease(ctx *context.APIContext) { // responses: // "204": // "$ref": "#/responses/empty" - if ctx.Repo.AccessMode < models.AccessModeWrite { - ctx.Status(403) - return - } id := ctx.ParamsInt64(":id") rel, err := models.GetReleaseByID(id) if err != nil && !models.IsErrReleaseNotExist(err) { diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index a56f696974002..9ce15ae1688d6 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -470,15 +470,15 @@ func GetByID(ctx *context.APIContext) { return } - access, err := models.AccessLevel(ctx.User.ID, repo) + perm, err := models.GetUserRepoPermission(repo, ctx.User) if err != nil { ctx.Error(500, "AccessLevel", err) return - } else if access < models.AccessModeRead { + } else if !perm.HasAccess() { ctx.Status(404) return } - ctx.JSON(200, repo.APIFormat(access)) + ctx.JSON(200, repo.APIFormat(perm.AccessMode)) } // Delete one repository @@ -504,10 +504,6 @@ func Delete(ctx *context.APIContext) { // "$ref": "#/responses/empty" // "403": // "$ref": "#/responses/forbidden" - if !ctx.Repo.IsAdmin() { - ctx.Error(403, "", "Must have admin rights") - return - } owner := ctx.Repo.Owner repo := ctx.Repo.Repository diff --git a/routers/repo/activity.go b/routers/repo/activity.go index a0a25dc936b2a..7168728e76aaa 100644 --- a/routers/repo/activity.go +++ b/routers/repo/activity.go @@ -45,9 +45,9 @@ func Activity(ctx *context.Context) { var err error if ctx.Data["Activity"], err = models.GetActivityStats(ctx.Repo.Repository.ID, timeFrom, - ctx.Repo.Repository.UnitEnabled(models.UnitTypeReleases), - ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues), - ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests)); err != nil { + ctx.Repo.CanAccess(models.UnitTypeReleases), + ctx.Repo.CanAccess(models.UnitTypeIssues), + ctx.Repo.CanAccess(models.UnitTypePullRequests)); err != nil { ctx.ServerError("GetActivityStats", err) return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index ec8a25cf633e8..72558c4c2e8eb 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -844,8 +844,8 @@ func GetActionIssue(ctx *context.Context) *models.Issue { } func checkIssueRights(ctx *context.Context, issue *models.Issue) { - if issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests) || - !issue.IsPull && !ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) { + if issue.IsPull && !ctx.Repo.CanAccess(models.UnitTypePullRequests) || + !issue.IsPull && !ctx.Repo.CanAccess(models.UnitTypeIssues) { ctx.NotFound("IssueOrPullRequestUnitNotAllowed", nil) } } @@ -870,8 +870,8 @@ func getActionIssues(ctx *context.Context) []*models.Issue { return nil } // Check access rights for all issues - issueUnitEnabled := ctx.Repo.Repository.UnitEnabled(models.UnitTypeIssues) - prUnitEnabled := ctx.Repo.Repository.UnitEnabled(models.UnitTypePullRequests) + issueUnitEnabled := ctx.Repo.CanAccess(models.UnitTypeIssues) + prUnitEnabled := ctx.Repo.CanAccess(models.UnitTypePullRequests) for _, issue := range issues { if issue.IsPull && !prUnitEnabled || !issue.IsPull && !issueUnitEnabled { ctx.NotFound("IssueOrPullRequestUnitNotAllowed", nil) diff --git a/routers/repo/wiki.go b/routers/repo/wiki.go index f1bd191286ed0..a36e0977edd65 100644 --- a/routers/repo/wiki.go +++ b/routers/repo/wiki.go @@ -31,8 +31,8 @@ const ( // MustEnableWiki check if wiki is enabled, if external then redirect func MustEnableWiki(ctx *context.Context) { - if !ctx.Repo.Repository.UnitEnabled(models.UnitTypeWiki) && - !ctx.Repo.Repository.UnitEnabled(models.UnitTypeExternalWiki) { + if !ctx.Repo.CanAccess(models.UnitTypeWiki) && + !ctx.Repo.CanAccess(models.UnitTypeExternalWiki) { ctx.NotFound("MustEnableWiki", nil) return } From 95d9a58bddf6fe24bda20f01deaca1e7c3ebfd75 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Tue, 13 Nov 2018 11:14:06 +0800 Subject: [PATCH 13/31] fix some permission check --- models/repo.go | 5 -- modules/context/permission.go | 64 +++++++++++++++++++ modules/context/repo.go | 55 +--------------- routers/repo/issue.go | 4 +- routers/repo/pull.go | 8 ++- templates/repo/activity.tmpl | 10 +-- templates/repo/diff/comments.tmpl | 2 +- templates/repo/header.tmpl | 20 +++--- templates/repo/home.tmpl | 4 +- .../repo/issue/view_content/comments.tmpl | 2 +- templates/repo/release/list.tmpl | 4 +- templates/repo/settings/options.tmpl | 6 +- templates/repo/settings/webhook/history.tmpl | 2 +- templates/repo/sub_menu.tmpl | 4 +- 14 files changed, 101 insertions(+), 89 deletions(-) create mode 100644 modules/context/permission.go diff --git a/models/repo.go b/models/repo.go index 5db73c566e20b..cca82d86b4679 100644 --- a/models/repo.go +++ b/models/repo.go @@ -657,11 +657,6 @@ func (repo *Repository) UpdateSize() error { return repo.updateSize(x) } -// CanBeForked returns true if repository meets the requirements of being forked. -func (repo *Repository) CanBeForked() bool { - return !repo.IsBare && repo.UnitEnabled(UnitTypeCode) -} - // CanUserFork returns true if specified user can fork repository. func (repo *Repository) CanUserFork(user *User) (bool, error) { if user == nil { diff --git a/modules/context/permission.go b/modules/context/permission.go new file mode 100644 index 0000000000000..f9621f9da48a1 --- /dev/null +++ b/modules/context/permission.go @@ -0,0 +1,64 @@ +// Copyright 2018 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 context + +import ( + "code.gitea.io/gitea/models" + macaron "gopkg.in/macaron.v1" +) + +// RequireRepoAdmin returns a macaron middleware for requiring repository admin permission +func RequireRepoAdmin() macaron.Handler { + return func(ctx *Context) { + if !ctx.IsSigned || (!ctx.Repo.IsAdmin() && !ctx.User.IsAdmin) { + ctx.NotFound(ctx.Req.RequestURI, nil) + return + } + } +} + +// RequireRepoWriter returns a macaron middleware for requiring repository write to the specify unitType +func RequireRepoWriter(unitType models.UnitType) macaron.Handler { + return func(ctx *Context) { + if !ctx.Repo.CanWrite(unitType) { + ctx.NotFound(ctx.Req.RequestURI, nil) + return + } + } +} + +// RequireRepoWriterOr returns a macaron middleware for requiring repository write to one of the unit permission +func RequireRepoWriterOr(unitTypes ...models.UnitType) macaron.Handler { + return func(ctx *Context) { + for _, unitType := range unitTypes { + if ctx.Repo.CanWrite(unitType) { + return + } + } + ctx.NotFound(ctx.Req.RequestURI, nil) + } +} + +// RequireRepoReader returns a macaron middleware for requiring repository read to the specify unitType +func RequireRepoReader(unitType models.UnitType) macaron.Handler { + return func(ctx *Context) { + if !ctx.Repo.CanAccess(unitType) { + ctx.NotFound(ctx.Req.RequestURI, nil) + return + } + } +} + +// RequireRepoReaderOr returns a macaron middleware for requiring repository write to one of the unit permission +func RequireRepoReaderOr(unitTypes ...models.UnitType) macaron.Handler { + return func(ctx *Context) { + for _, unitType := range unitTypes { + if ctx.Repo.CanAccess(unitType) { + return + } + } + ctx.NotFound(ctx.Req.RequestURI, nil) + } +} diff --git a/modules/context/repo.go b/modules/context/repo.go index 1b51939466a9a..e40fde25b17f4 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -218,6 +218,7 @@ func repoAssignment(ctx *Context, repo *models.Repository) { return } ctx.Data["HasAccess"] = true + ctx.Data["Permission"] = &ctx.Repo.Permission if repo.IsMirror { var err error @@ -627,60 +628,6 @@ func RepoRefByType(refType RepoRefType) macaron.Handler { } } -// RequireRepoAdmin returns a macaron middleware for requiring repository admin permission -func RequireRepoAdmin() macaron.Handler { - return func(ctx *Context) { - if !ctx.IsSigned || (!ctx.Repo.IsAdmin() && !ctx.User.IsAdmin) { - ctx.NotFound(ctx.Req.RequestURI, nil) - return - } - } -} - -// RequireRepoWriter returns a macaron middleware for requiring repository write to the specify unitType -func RequireRepoWriter(unitType models.UnitType) macaron.Handler { - return func(ctx *Context) { - if !ctx.Repo.CanWrite(unitType) { - ctx.NotFound(ctx.Req.RequestURI, nil) - return - } - } -} - -// RequireRepoWriterOr returns a macaron middleware for requiring repository write to one of the unit permission -func RequireRepoWriterOr(unitTypes ...models.UnitType) macaron.Handler { - return func(ctx *Context) { - for _, unitType := range unitTypes { - if ctx.Repo.CanWrite(unitType) { - return - } - } - ctx.NotFound(ctx.Req.RequestURI, nil) - } -} - -// RequireRepoReader returns a macaron middleware for requiring repository read to the specify unitType -func RequireRepoReader(unitType models.UnitType) macaron.Handler { - return func(ctx *Context) { - if !ctx.Repo.CanAccess(unitType) { - ctx.NotFound(ctx.Req.RequestURI, nil) - return - } - } -} - -// RequireRepoReaderOr returns a macaron middleware for requiring repository write to one of the unit permission -func RequireRepoReaderOr(unitTypes ...models.UnitType) macaron.Handler { - return func(ctx *Context) { - for _, unitType := range unitTypes { - if ctx.Repo.CanAccess(unitType) { - return - } - } - ctx.NotFound(ctx.Req.RequestURI, nil) - } -} - // GitHookService checks if repository Git hooks service has been enabled. func GitHookService() macaron.Handler { return func(ctx *Context) { diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 72558c4c2e8eb..73dee6ac29cf9 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -77,9 +77,9 @@ func MustEnableIssues(ctx *context.Context) { } } -// MustAllowPulls check if repository enable pull requests +// MustAllowPulls check if repository enable pull requests and user have right to do that func MustAllowPulls(ctx *context.Context) { - if !ctx.Repo.Repository.AllowsPulls() { + if !ctx.Repo.Repository.CanEnablePulls() || !ctx.Repo.CanAccess(models.UnitTypePullRequests) { ctx.NotFound("MustAllowPulls", nil) return } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 4ec1c27cea055..da94b952a3d54 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -57,7 +57,13 @@ func getForkRepository(ctx *context.Context) *models.Repository { return nil } - if !forkRepo.CanBeForked() || !forkRepo.HasAccess(ctx.User) { + perm, err := models.GetUserRepoPermission(forkRepo, ctx.User) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return nil + } + + if forkRepo.IsBare || !perm.CanAccess(models.UnitTypeCode) { ctx.NotFound("getForkRepository", nil) return nil } diff --git a/templates/repo/activity.tmpl b/templates/repo/activity.tmpl index 19866d53d77b4..29cd9be97c53d 100644 --- a/templates/repo/activity.tmpl +++ b/templates/repo/activity.tmpl @@ -23,10 +23,10 @@ </h2> <div class="ui divider"></div> - {{if (or (.Repository.UnitEnabled $.UnitTypeIssues) (.Repository.UnitEnabled $.UnitTypePullRequests))}} + {{if (or (.Permission.CanAccess $.UnitTypeIssues) (.Permission.CanAccess $.UnitTypePullRequests))}} <h4 class="ui top attached header">{{.i18n.Tr "repo.activity.overview"}}</h4> <div class="ui attached segment two column grid"> - {{if .Repository.UnitEnabled $.UnitTypePullRequests}} + {{if .Permission.CanAccess $.UnitTypePullRequests}} <div class="column"> {{if gt .Activity.ActivePRCount 0}} <div class="stats-table"> @@ -41,7 +41,7 @@ {{.i18n.Tr (TrN .i18n.Lang .Activity.ActivePRCount "repo.activity.active_prs_count_1" "repo.activity.active_prs_count_n") .Activity.ActivePRCount | Safe }} </div> {{end}} - {{if .Repository.UnitEnabled $.UnitTypeIssues}} + {{if .Permission.CanAccess $.UnitTypeIssues}} <div class="column"> {{if gt .Activity.ActiveIssueCount 0}} <div class="stats-table"> @@ -58,7 +58,7 @@ {{end}} </div> <div class="ui attached segment horizontal segments"> - {{if .Repository.UnitEnabled $.UnitTypePullRequests}} + {{if .Permission.CanAccess $.UnitTypePullRequests}} <a href="#merged-pull-requests" class="ui attached segment text center"> <i class="text purple octicon octicon-git-pull-request"></i> <strong>{{.Activity.MergedPRCount}}</strong><br> {{.i18n.Tr (TrN .i18n.Lang .Activity.MergedPRCount "repo.activity.merged_prs_count_1" "repo.activity.merged_prs_count_n") }} @@ -68,7 +68,7 @@ {{.i18n.Tr (TrN .i18n.Lang .Activity.OpenedPRCount "repo.activity.opened_prs_count_1" "repo.activity.opened_prs_count_n") }} </a> {{end}} - {{if .Repository.UnitEnabled $.UnitTypeIssues}} + {{if .Permission.CanAccess $.UnitTypeIssues}} <a href="#closed-issues" class="ui attached segment text center"> <i class="text red octicon octicon-issue-closed"></i> <strong>{{.Activity.ClosedIssueCount}}</strong><br> {{.i18n.Tr (TrN .i18n.Lang .Activity.ClosedIssueCount "repo.activity.closed_issues_count_1" "repo.activity.closed_issues_count_n") }} diff --git a/templates/repo/diff/comments.tmpl b/templates/repo/diff/comments.tmpl index 18221e36ed371..7e8588e60c848 100644 --- a/templates/repo/diff/comments.tmpl +++ b/templates/repo/diff/comments.tmpl @@ -21,7 +21,7 @@ {{end}} {{end}} {{template "repo/issue/view_content/add_reaction" Dict "ctx" $ "ActionURL" (Printf "%s/comments/%d/reactions" $.root.RepoLink .ID) }} - {{if or $.root.IsRepositoryAdmin (eq .Poster.ID $.root.SignedUserID)}} + {{if or $.root.Permission.IsAdmin (eq .Poster.ID $.root.SignedUserID)}} <div class="item action"> <a class="edit-content" href="#"><i class="octicon octicon-pencil"></i></a> <a class="delete-comment" href="#" data-comment-id={{.HashTag}} data-url="{{$.root.RepoLink}}/comments/{{.ID}}/delete" data-locale="{{$.root.i18n.Tr "repo.issues.delete_comment_confirm"}}"><i class="octicon octicon-x"></i></a> diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index bf052301555b2..3d8abba1b8bd2 100644 --- a/templates/repo/header.tmpl +++ b/templates/repo/header.tmpl @@ -30,7 +30,7 @@ {{.NumStars}} </a> </div> - {{if .CanBeForked}} + {{if and (not .IsBare) ($.Permission.CanAccess $.UnitTypeCode)}} <div class="ui compact labeled button" tabindex="0"> <a class="ui compact button {{if not $.CanSignedUserFork}}poping up{{end}}" {{if $.CanSignedUserFork}}href="{{AppSubUrl}}/repo/fork/{{.ID}}"{{else if $.IsSigned}} data-content="{{$.i18n.Tr "repo.fork_from_self"}}" data-position="top center" data-variation="tiny"{{end}}> <i class="octicon octicon-repo-forked"></i>{{$.i18n.Tr "repo.fork"}} @@ -47,43 +47,43 @@ {{if not .IsDiffCompare}} <div class="ui tabs container"> <div class="ui tabular stackable menu navbar"> - {{if .Repository.UnitEnabled $.UnitTypeCode}} + {{if .Permission.CanAccess $.UnitTypeCode}} <a class="{{if .PageIsViewCode}}active{{end}} item" href="{{.RepoLink}}{{if (ne .BranchName .Repository.DefaultBranch)}}/src/{{.BranchNameSubURL | EscapePound}}{{end}}"> <i class="octicon octicon-code"></i> {{.i18n.Tr "repo.code"}} </a> {{end}} - {{if .Repository.UnitEnabled $.UnitTypeIssues}} + {{if .Permission.CanAccess $.UnitTypeIssues}} <a class="{{if .PageIsIssueList}}active{{end}} item" href="{{.RepoLink}}/issues"> <i class="octicon octicon-issue-opened"></i> {{.i18n.Tr "repo.issues"}} <span class="ui {{if not .Repository.NumOpenIssues}}gray{{else}}blue{{end}} small label">{{.Repository.NumOpenIssues}}</span> </a> {{end}} - {{if .Repository.UnitEnabled $.UnitTypeExternalTracker}} + {{if .Permission.CanAccess $.UnitTypeExternalTracker}} <a class="{{if .PageIsIssueList}}active{{end}} item" href="{{.RepoLink}}/issues" target="_blank" rel="noopener noreferrer"> <i class="octicon octicon-issue-opened"></i> {{.i18n.Tr "repo.issues"}} </span> </a> {{end}} - {{if .Repository.AllowsPulls}} + {{if and .Repository.CanEnablePulls (.Permission.CanAccess $.UnitTypePullRequests)}} <a class="{{if .PageIsPullList}}active{{end}} item" href="{{.RepoLink}}/pulls"> <i class="octicon octicon-git-pull-request"></i> {{.i18n.Tr "repo.pulls"}} <span class="ui {{if not .Repository.NumOpenPulls}}gray{{else}}blue{{end}} small label">{{.Repository.NumOpenPulls}}</span> </a> {{end}} - {{if and (.Repository.UnitEnabled $.UnitTypeReleases) (not .IsBareRepo) }} + {{if and (.Permission.CanAccess $.UnitTypeReleases) (not .IsBareRepo) }} <a class="{{if .PageIsReleaseList}}active{{end}} item" href="{{.RepoLink}}/releases"> <i class="octicon octicon-tag"></i> {{.i18n.Tr "repo.releases"}} <span class="ui {{if not .Repository.NumReleases}}gray{{else}}blue{{end}} small label">{{.Repository.NumReleases}}</span> </a> {{end}} - {{if or (.Repository.UnitEnabled $.UnitTypeWiki) (.Repository.UnitEnabled $.UnitTypeExternalWiki)}} - <a class="{{if .PageIsWiki}}active{{end}} item" href="{{.RepoLink}}/wiki" {{if (.Repository.UnitEnabled $.UnitTypeExternalWiki)}} target="_blank" rel="noopener noreferrer" {{end}}> + {{if or (.Permission.CanAccess $.UnitTypeWiki) (.Permission.CanAccess $.UnitTypeExternalWiki)}} + <a class="{{if .PageIsWiki}}active{{end}} item" href="{{.RepoLink}}/wiki" {{if (.Permission.CanAccess $.UnitTypeExternalWiki)}} target="_blank" rel="noopener noreferrer" {{end}}> <i class="octicon octicon-book"></i> {{.i18n.Tr "repo.wiki"}} </a> {{end}} - {{if and (.Repository.AnyUnitEnabled $.UnitTypePullRequests $.UnitTypeIssues $.UnitTypeReleases) (not .IsBareRepo)}} + {{if and (.Permission.CanAccessAny $.UnitTypePullRequests $.UnitTypeIssues $.UnitTypeReleases) (not .IsBareRepo)}} <a class="{{if .PageIsActivity}}active{{end}} item" href="{{.RepoLink}}/activity"> <i class="octicon octicon-pulse"></i> {{.i18n.Tr "repo.activity"}} </a> @@ -91,7 +91,7 @@ {{template "custom/extra_tabs" .}} - {{if .IsRepositoryAdmin}} + {{if .Permission.IsAdmin}} <div class="right menu"> <a class="{{if .PageIsSettings}}active{{end}} item" href="{{.RepoLink}}/settings"> <i class="octicon octicon-tools"></i> {{.i18n.Tr "repo.settings"}} diff --git a/templates/repo/home.tmpl b/templates/repo/home.tmpl index 3e69506c0a05e..0a5886381d420 100644 --- a/templates/repo/home.tmpl +++ b/templates/repo/home.tmpl @@ -25,9 +25,9 @@ </div> <div class="ui repo-topic" id="repo-topic"> {{range .Topics}}<a class="ui green basic label topic" style="cursor:pointer;" href="{{AppSubUrl}}/explore/repos?q={{.Name}}&topic=1">{{.Name}}</a>{{end}} - {{if .IsRepositoryAdmin}}<a id="manage_topic" style="cursor:pointer;margin-left:10px;">{{.i18n.Tr "repo.topic.manage_topics"}}</a>{{end}} + {{if .Permission.IsAdmin}}<a id="manage_topic" style="cursor:pointer;margin-left:10px;">{{.i18n.Tr "repo.topic.manage_topics"}}</a>{{end}} </div> - {{if .IsRepositoryAdmin}} + {{if .Permission.IsAdmin}} <div class="ui repo-topic-edit grid form segment error" id="topic_edit" style="display:none"> <div class="fourteen wide column"> <div class="field"> diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 4e79fd866c19e..953aa4c2f02db 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -23,7 +23,7 @@ </div> {{end}} {{template "repo/issue/view_content/add_reaction" Dict "ctx" $ "ActionURL" (Printf "%s/comments/%d/reactions" $.RepoLink .ID) }} - {{if or $.IsRepositoryAdmin (eq .Poster.ID $.SignedUserID)}} + {{if or $.Permission.IsAdmin (eq .Poster.ID $.SignedUserID)}} <div class="item action"> <a class="edit-content" href="#"><i class="octicon octicon-pencil"></i></a> <a class="delete-comment" href="#" data-comment-id={{.HashTag}} data-url="{{$.RepoLink}}/comments/{{.ID}}/delete" data-locale="{{$.i18n.Tr "repo.issues.delete_comment_confirm"}}"><i class="octicon octicon-x"></i></a> diff --git a/templates/repo/release/list.tmpl b/templates/repo/release/list.tmpl index 8e1ce06f5deaf..80cf825495cfc 100644 --- a/templates/repo/release/list.tmpl +++ b/templates/repo/release/list.tmpl @@ -41,7 +41,7 @@ <a href="{{$.RepoLink}}/src/tag/{{.TagName | EscapePound}}" rel="nofollow"><i class="tag icon"></i> {{.TagName}}</a> </h4> <div class="download"> - {{if $.Repository.UnitEnabled $.UnitTypeCode}} + {{if $.Permission.CanAccess $.UnitTypeCode}} <a href="{{$.RepoLink}}/src/commit/{{.Sha1}}" rel="nofollow"><i class="code icon"></i> {{ShortSha .Sha1}}</a> <a href="{{$.RepoLink}}/archive/{{.TagName | EscapePound}}.zip" rel="nofollow"><i class="octicon octicon-file-zip"></i> ZIP</a> <a href="{{$.RepoLink}}/archive/{{.TagName | EscapePound}}.tar.gz"><i class="octicon octicon-file-zip"></i> TAR.GZ</a> @@ -66,7 +66,7 @@ <div class="download"> <h2>{{$.i18n.Tr "repo.release.downloads"}}</h2> <ul class="list"> - {{if $.Repository.UnitEnabled $.UnitTypeCode}} + {{if $.Permission.CanAccess $.UnitTypeCode}} <li> <a href="{{$.RepoLink}}/archive/{{.TagName | EscapePound}}.zip" rel="nofollow"><strong><i class="octicon octicon-file-zip"></i> {{$.i18n.Tr "repo.release.source_code"}} (ZIP)</strong></a> </li> diff --git a/templates/repo/settings/options.tmpl b/templates/repo/settings/options.tmpl index 744fa820620a7..b0c78ae9e02d1 100644 --- a/templates/repo/settings/options.tmpl +++ b/templates/repo/settings/options.tmpl @@ -266,7 +266,7 @@ </div> {{end}} - {{if .IsRepositoryOwner}} + {{if .Permission.IsOwner}} <h4 class="ui top attached warning header"> {{.i18n.Tr "repo.settings.danger_zone"}} </h4> @@ -294,7 +294,7 @@ </div> </div> - {{if .Repository.UnitEnabled $.UnitTypeWiki}} + {{if .Permission.CanAccess $.UnitTypeWiki}} <div class="ui divider"></div> <div class="item"> @@ -324,7 +324,7 @@ </div> </div> -{{if .IsRepositoryOwner}} +{{if .Permission.IsOwner}} {{if .Repository.IsMirror}} <div class="ui small modal" id="convert-repo-modal"> <div class="header"> diff --git a/templates/repo/settings/webhook/history.tmpl b/templates/repo/settings/webhook/history.tmpl index a08481aaface1..7f85c702b5440 100644 --- a/templates/repo/settings/webhook/history.tmpl +++ b/templates/repo/settings/webhook/history.tmpl @@ -1,7 +1,7 @@ {{if .PageIsSettingsHooksEdit}} <h4 class="ui top attached header"> {{.i18n.Tr "repo.settings.recent_deliveries"}} - {{if .IsRepositoryAdmin}} + {{if .Permission.IsAdmin}} <div class="ui right"> <button class="ui teal tiny button poping up" id="test-delivery" data-content= "{{.i18n.Tr "repo.settings.webhook.test_delivery_desc"}}" data-variation="inverted tiny" data-link="{{.Link}}/test" data-redirect="{{.Link}}">{{.i18n.Tr "repo.settings.webhook.test_delivery"}}</button> diff --git a/templates/repo/sub_menu.tmpl b/templates/repo/sub_menu.tmpl index cd37f80b70e81..4f084bca9a3d0 100644 --- a/templates/repo/sub_menu.tmpl +++ b/templates/repo/sub_menu.tmpl @@ -1,11 +1,11 @@ <div class="ui segment sub-menu"> <div class="ui two horizontal center link list"> - {{if and (.Repository.UnitEnabled $.UnitTypeCode) (not .IsBareRepo)}} + {{if and (.Permission.CanAccess $.UnitTypeCode) (not .IsBareRepo)}} <div class="item{{if .PageIsCommits}} active{{end}}"> <a href="{{.RepoLink}}/commits{{if .IsViewBranch}}/branch{{else if .IsViewTag}}/tag{{else if .IsViewCommit}}/commit{{end}}/{{EscapePound .BranchName}}"><i class="octicon octicon-history"></i> <b>{{.CommitsCount}}</b> {{.i18n.Tr (TrN .i18n.Lang .CommitsCount "repo.commit" "repo.commits") }}</a> </div> {{end}} - {{if and (.Repository.UnitEnabled $.UnitTypeCode) (not .IsBareRepo) }} + {{if and (.Permission.CanAccess $.UnitTypeCode) (not .IsBareRepo) }} <div class="item{{if .PageIsBranches}} active{{end}}"> <a href="{{.RepoLink}}/branches/"><i class="octicon octicon-git-branch"></i> <b>{{.BranchesCount}}</b> {{.i18n.Tr (TrN .i18n.Lang .BranchesCount "repo.branch" "repo.branches") }}</a> </div> From 5df61b6ceb6ae3e2bda4abc60113709110d17513 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Tue, 13 Nov 2018 13:08:42 +0800 Subject: [PATCH 14/31] fix tests --- modules/test/context_tests.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/test/context_tests.go b/modules/test/context_tests.go index b2b96fff9d44f..af986001ae3e0 100644 --- a/modules/test/context_tests.go +++ b/modules/test/context_tests.go @@ -47,6 +47,9 @@ func LoadRepo(t *testing.T, ctx *context.Context, repoID int64) { ctx.Repo = &context.Repository{} ctx.Repo.Repository = models.AssertExistsAndLoadBean(t, &models.Repository{ID: repoID}).(*models.Repository) ctx.Repo.RepoLink = ctx.Repo.Repository.Link() + var err error + ctx.Repo.Permission, err = models.GetUserRepoPermission(ctx.Repo.Repository, ctx.User) + assert.NoError(t, err) } // LoadRepoCommit loads a repo's commit into a test context. From 4ee6e1f9b2b9e9320c8401c7de45f5d347d1a714 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Tue, 13 Nov 2018 14:28:00 +0800 Subject: [PATCH 15/31] fix tests --- models/fixtures/repo_unit.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/models/fixtures/repo_unit.yml b/models/fixtures/repo_unit.yml index 2fde65b05bfe6..ca2cdb3c608bb 100644 --- a/models/fixtures/repo_unit.yml +++ b/models/fixtures/repo_unit.yml @@ -192,4 +192,11 @@ repo_id: 24 type: 2 config: "{}" + created_unix: 1524304355 + +- + id: 29 + repo_id: 16 + type: 1 + config: "{}" created_unix: 1524304355 \ No newline at end of file From de0437737aba991709b88c9e5c9b737ff78e1277 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Tue, 13 Nov 2018 17:16:23 +0800 Subject: [PATCH 16/31] improve some permission check --- models/repo_permission.go | 11 ++++++++- modules/context/permission.go | 2 +- routers/repo/issue.go | 44 ++++++++++++++++++++++++++--------- routers/repo/issue_watch.go | 25 ++++++++++++-------- routers/routes/routes.go | 4 ++-- 5 files changed, 61 insertions(+), 25 deletions(-) diff --git a/models/repo_permission.go b/models/repo_permission.go index 5a1b40e919391..80e7364f1a6a2 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -71,6 +71,15 @@ func (p *Permission) CanWriteIssuesOrPulls(isPull bool) bool { return p.CanWrite(UnitTypeIssues) } +// CanReadIssuesOrPulls returns true if isPull is true and user could read pull requests and +// returns true if isPull is false and user could read to issues +func (p *Permission) CanReadIssuesOrPulls(isPull bool) bool { + if isPull { + return p.CanAccess(UnitTypePullRequests) + } + return p.CanAccess(UnitTypeIssues) +} + // GetUserRepoPermission returns the user permissions to the repository func GetUserRepoPermission(repo *Repository, user *User) (Permission, error) { return getUserRepoPermission(x, repo, user) @@ -96,7 +105,7 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss return } - // Admin has super access or user is the owner of the repository + // Admin or the owner has super access to the repository if user.IsAdmin || user.ID == repo.OwnerID { perm.AccessMode = AccessModeOwner return diff --git a/modules/context/permission.go b/modules/context/permission.go index f9621f9da48a1..e13fbb939e6f7 100644 --- a/modules/context/permission.go +++ b/modules/context/permission.go @@ -12,7 +12,7 @@ import ( // RequireRepoAdmin returns a macaron middleware for requiring repository admin permission func RequireRepoAdmin() macaron.Handler { return func(ctx *Context) { - if !ctx.IsSigned || (!ctx.Repo.IsAdmin() && !ctx.User.IsAdmin) { + if !ctx.IsSigned || !ctx.Repo.IsAdmin() { ctx.NotFound(ctx.Req.RequestURI, nil) return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 73dee6ac29cf9..0d90c9297212f 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1039,6 +1039,11 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { return } + if !ctx.IsSigned || (ctx.User.ID != issue.PosterID && !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull)) { + ctx.Error(403) + return + } + var attachments []string if setting.AttachmentEnabled { attachments = form.Files @@ -1142,7 +1147,12 @@ func UpdateCommentContent(ctx *context.Context) { return } - if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin()) { + if err := comment.LoadIssue(); err != nil { + ctx.NotFoundOrServerError("LoadIssue", models.IsErrIssueNotExist, err) + return + } + + if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Error(403) return } else if comment.Type != models.CommentTypeComment && comment.Type != models.CommentTypeCode { @@ -1176,7 +1186,12 @@ func DeleteComment(ctx *context.Context) { return } - if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.IsAdmin()) { + if err := comment.LoadIssue(); err != nil { + ctx.NotFoundOrServerError("LoadIssue", models.IsErrIssueNotExist, err) + return + } + + if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Error(403) return } else if comment.Type != models.CommentTypeComment && comment.Type != models.CommentTypeCode { @@ -1419,6 +1434,11 @@ func ChangeIssueReaction(ctx *context.Context, form auth.ReactionForm) { return } + if !ctx.IsSigned || (ctx.User.ID != issue.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull)) { + ctx.Error(403) + return + } + if ctx.HasError() { ctx.ServerError("ChangeIssueReaction", errors.New(ctx.GetErrMsg())) return @@ -1488,20 +1508,22 @@ func ChangeCommentReaction(ctx *context.Context, form auth.ReactionForm) { return } - issue, err := models.GetIssueByID(comment.IssueID) - checkIssueRights(ctx, issue) - if ctx.Written() { + if err := comment.LoadIssue(); err != nil { + ctx.NotFoundOrServerError("LoadIssue", models.IsErrIssueNotExist, err) return } - if ctx.HasError() { - ctx.ServerError("ChangeCommentReaction", errors.New(ctx.GetErrMsg())) + if !ctx.IsSigned || (ctx.User.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { + ctx.Error(403) + return + } else if comment.Type != models.CommentTypeComment && comment.Type != models.CommentTypeCode { + ctx.Error(204) return } switch ctx.Params(":action") { case "react": - reaction, err := models.CreateCommentReaction(ctx.User, issue, comment, form.Content) + reaction, err := models.CreateCommentReaction(ctx.User, comment.Issue, comment, form.Content) if err != nil { log.Info("CreateCommentReaction: %s", err) break @@ -1513,9 +1535,9 @@ func ChangeCommentReaction(ctx *context.Context, form auth.ReactionForm) { break } - log.Trace("Reaction for comment created: %d/%d/%d/%d", ctx.Repo.Repository.ID, issue.ID, comment.ID, reaction.ID) + log.Trace("Reaction for comment created: %d/%d/%d/%d", ctx.Repo.Repository.ID, comment.Issue.ID, comment.ID, reaction.ID) case "unreact": - if err := models.DeleteCommentReaction(ctx.User, issue, comment, form.Content); err != nil { + if err := models.DeleteCommentReaction(ctx.User, comment.Issue, comment, form.Content); err != nil { ctx.ServerError("DeleteCommentReaction", err) return } @@ -1527,7 +1549,7 @@ func ChangeCommentReaction(ctx *context.Context, form auth.ReactionForm) { break } - log.Trace("Reaction for comment removed: %d/%d/%d", ctx.Repo.Repository.ID, issue.ID, comment.ID) + log.Trace("Reaction for comment removed: %d/%d/%d", ctx.Repo.Repository.ID, comment.Issue.ID, comment.ID) default: ctx.NotFound(fmt.Sprintf("Unknown action %s", ctx.Params(":action")), nil) return diff --git a/routers/repo/issue_watch.go b/routers/repo/issue_watch.go index a499b70d9c31f..c6a436801a1ee 100644 --- a/routers/repo/issue_watch.go +++ b/routers/repo/issue_watch.go @@ -14,23 +14,28 @@ import ( ) // IssueWatch sets issue watching -func IssueWatch(c *context.Context) { - watch, err := strconv.ParseBool(c.Req.PostForm.Get("watch")) - if err != nil { - c.ServerError("watch is not bool", err) +func IssueWatch(ctx *context.Context) { + issue := GetActionIssue(ctx) + if ctx.Written() { return } - issue := GetActionIssue(c) - if c.Written() { + if !ctx.IsSigned || (ctx.User.ID != issue.PosterID && !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull)) { + ctx.Error(403) + return + } + + watch, err := strconv.ParseBool(ctx.Req.PostForm.Get("watch")) + if err != nil { + ctx.ServerError("watch is not bool", err) return } - if err := models.CreateOrUpdateIssueWatch(c.User.ID, issue.ID, watch); err != nil { - c.ServerError("CreateOrUpdateIssueWatch", err) + if err := models.CreateOrUpdateIssueWatch(ctx.User.ID, issue.ID, watch); err != nil { + ctx.ServerError("CreateOrUpdateIssueWatch", err) return } - url := fmt.Sprintf("%s/issues/%d", c.Repo.RepoLink, issue.Index) - c.Redirect(url, http.StatusSeeOther) + url := fmt.Sprintf("%s/issues/%d", ctx.Repo.RepoLink, issue.Index) + ctx.Redirect(url, http.StatusSeeOther) } diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 0372f6ec76e20..8edbc51bc79df 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -562,7 +562,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("", repo.UpdateCommentContent) m.Post("/delete", repo.DeleteComment) m.Post("/reactions/:action", bindIgnErr(auth.ReactionForm{}), repo.ChangeCommentReaction) - }, reqRepoIssuesOrPullsReader) + }) m.Group("/labels", func() { m.Post("/new", bindIgnErr(auth.CreateLabelForm{}), repo.NewLabel) m.Post("/edit", bindIgnErr(auth.CreateLabelForm{}), repo.UpdateLabel) @@ -578,7 +578,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/delete", repo.DeleteMilestone) }, reqRepoIssuesOrPullsWriter, context.RepoRef()) - m.Combo("/compare/*", repo.MustAllowPulls, repo.SetEditorconfigIfExists). + m.Combo("/compare/*", reqRepoCodeReader, repo.MustAllowPulls, repo.SetEditorconfigIfExists). Get(repo.SetDiffViewStyle, repo.CompareAndPullRequest). Post(bindIgnErr(auth.CreateIssueForm{}), repo.CompareAndPullRequestPost) From 66fd8f33dcea1dcc4ffa722f8a97334f35d9c9d6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Wed, 14 Nov 2018 14:04:36 +0800 Subject: [PATCH 17/31] fix some permission check --- models/branches.go | 12 +++- models/issue.go | 12 ++-- models/lfs_lock.go | 5 +- models/repo.go | 9 +-- models/repo_permission.go | 20 +++++++ models/ssh_key.go | 6 +- models/user.go | 29 ---------- modules/lfs/server.go | 15 +++-- routers/api/v1/repo/pull.go | 7 ++- routers/repo/issue.go | 107 ++++++++++++++++++++---------------- routers/repo/pull.go | 16 +++++- routers/repo/wiki.go | 7 ++- routers/routes/routes.go | 2 +- routers/user/home.go | 7 ++- 14 files changed, 145 insertions(+), 109 deletions(-) diff --git a/models/branches.go b/models/branches.go index 3de76a5cc16c3..bbcd342baae44 100644 --- a/models/branches.go +++ b/models/branches.go @@ -243,10 +243,16 @@ func updateUserWhitelist(repo *Repository, currentWhitelist, newWhitelist []int6 whitelist = make([]int64, 0, len(newWhitelist)) for _, userID := range newWhitelist { - has, err := hasAccess(x, userID, repo, AccessModeWrite) + user, err := GetUserByID(userID) if err != nil { - return nil, fmt.Errorf("HasAccess [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err) - } else if !has { + return nil, fmt.Errorf("GetUserByID [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err) + } + perm, err := GetUserRepoPermission(repo, user) + if err != nil { + return nil, fmt.Errorf("GetUserRepoPermission [user_id: %d, repo_id: %d]: %v", userID, repo.ID, err) + } + + if !perm.CanWrite(UnitTypeCode) { continue // Drop invalid user ID } diff --git a/models/issue.go b/models/issue.go index ab4fe3107bac7..c0c29c2013e88 100644 --- a/models/issue.go +++ b/models/issue.go @@ -468,9 +468,11 @@ func (issue *Issue) RemoveLabel(doer *User, label *Label) error { return err } - if has, err := HasAccess(doer.ID, issue.Repo, AccessModeWrite); err != nil { + perm, err := GetUserRepoPermission(issue.Repo, doer) + if err != nil { return err - } else if !has { + } + if !perm.CanWriteIssuesOrPulls(issue.IsPull) { return ErrLabelNotExist{} } @@ -511,9 +513,11 @@ func (issue *Issue) ClearLabels(doer *User) (err error) { return err } - if has, err := hasAccess(sess, doer.ID, issue.Repo, AccessModeWrite); err != nil { + perm, err := getUserRepoPermission(sess, issue.Repo, doer) + if err != nil { return err - } else if !has { + } + if !perm.CanWriteIssuesOrPulls(issue.IsPull) { return ErrLabelNotExist{} } diff --git a/models/lfs_lock.go b/models/lfs_lock.go index 52e877b156aec..377fc41aac5ed 100644 --- a/models/lfs_lock.go +++ b/models/lfs_lock.go @@ -139,10 +139,11 @@ func CheckLFSAccessForRepo(u *User, repo *Repository, mode AccessMode) error { if u == nil { return ErrLFSUnauthorizedAction{repo.ID, "undefined", mode} } - has, err := HasAccess(u.ID, repo, mode) + perm, err := GetUserRepoPermission(repo, u) if err != nil { return err - } else if !has { + } + if !perm.CanWrite(UnitTypeCode) { return ErrLFSUnauthorizedAction{repo.ID, u.DisplayName(), mode} } return nil diff --git a/models/repo.go b/models/repo.go index cca82d86b4679..feece9838cb8c 100644 --- a/models/repo.go +++ b/models/repo.go @@ -553,11 +553,6 @@ func (repo *Repository) GetAssignees() (_ []*User, err error) { return repo.getAssignees(x) } -// GetUserIfHasWriteAccess returns the user that has write access of repository by given ID. -func (repo *Repository) GetUserIfHasWriteAccess(userID int64) (*User, error) { - return GetUserIfHasWriteAccess(repo, userID) -} - // GetMilestoneByID returns the milestone belongs to repository by given ID. func (repo *Repository) GetMilestoneByID(milestoneID int64) (*Milestone, error) { return GetMilestoneByRepoID(repo.ID, milestoneID) @@ -625,10 +620,10 @@ func (repo *Repository) ComposeCompareURL(oldCommitID, newCommitID string) strin } // HasAccess returns true when user has access to this repository -func (repo *Repository) HasAccess(u *User) bool { +/*func (repo *Repository) HasAccess(u *User) bool { has, _ := HasAccess(u.ID, repo, AccessModeRead) return has -} +}*/ // UpdateDefaultBranch updates the default branch func (repo *Repository) UpdateDefaultBranch() error { diff --git a/models/repo_permission.go b/models/repo_permission.go index 80e7364f1a6a2..ad8d6ede37acf 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -173,3 +173,23 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss return } + +// IsUserRepoAdmin return ture if user has admin right of a repo +func IsUserRepoAdmin(repo *Repository, user *User) (bool, error) { + return isUserRepoAdmin(x, repo, user) +} + +func isUserRepoAdmin(e Engine, repo *Repository, user *User) (bool, error) { + if user == nil || repo == nil { + return false, nil + } + if user.IsAdmin { + return true, nil + } + + mode, err := accessLevel(e, user.ID, repo) + if err != nil { + return false, err + } + return mode >= AccessModeAdmin, nil +} diff --git a/models/ssh_key.go b/models/ssh_key.go index 5d046c020113b..b7dd81b49b0b3 100644 --- a/models/ssh_key.go +++ b/models/ssh_key.go @@ -807,10 +807,10 @@ func DeleteDeployKey(doer *User, id int64) error { if err != nil { return fmt.Errorf("GetRepositoryByID: %v", err) } - yes, err := HasAccess(doer.ID, repo, AccessModeAdmin) + has, err := IsUserRepoAdmin(repo, doer) if err != nil { - return fmt.Errorf("HasAccess: %v", err) - } else if !yes { + return fmt.Errorf("GetUserRepoPermission: %v", err) + } else if !has { return ErrKeyAccessDenied{doer.ID, key.ID, "deploy"} } } diff --git a/models/user.go b/models/user.go index 9becee7760bd1..8f1b170b0d5ac 100644 --- a/models/user.go +++ b/models/user.go @@ -496,24 +496,6 @@ func (u *User) DeleteAvatar() error { return nil } -// IsAdminOfRepo returns true if user has admin or higher access of repository. -func (u *User) IsAdminOfRepo(repo *Repository) bool { - has, err := HasAccess(u.ID, repo, AccessModeAdmin) - if err != nil { - log.Error(3, "HasAccess: %v", err) - } - return has -} - -// IsWriterOfRepo returns true if user has write access to given repository. -func (u *User) IsWriterOfRepo(repo *Repository) bool { - has, err := HasAccess(u.ID, repo, AccessModeWrite) - if err != nil { - log.Error(3, "HasAccess: %v", err) - } - return has -} - // IsOrganization returns true if user is actually a organization. func (u *User) IsOrganization() bool { return u.Type == UserTypeOrganization @@ -1170,17 +1152,6 @@ func GetUserByID(id int64) (*User, error) { return getUserByID(x, id) } -// GetUserIfHasWriteAccess returns the user with write access of repository by given ID. -func GetUserIfHasWriteAccess(repo *Repository, userID int64) (*User, error) { - has, err := HasAccess(userID, repo, AccessModeWrite) - if err != nil { - return nil, err - } else if !has { - return nil, ErrUserNotExist{userID, "", 0} - } - return GetUserByID(userID) -} - // GetUserByName returns user by given name. func GetUserByName(name string) (*User, error) { return getUserByName(x, name) diff --git a/modules/lfs/server.go b/modules/lfs/server.go index d6543816b9268..0a61478bb9524 100644 --- a/modules/lfs/server.go +++ b/modules/lfs/server.go @@ -497,12 +497,12 @@ func authenticate(ctx *context.Context, repository *models.Repository, authoriza accessMode = models.AccessModeWrite } - if !repository.IsPrivate && !requireWrite { - return true + perm, err := models.GetUserRepoPermission(repository, ctx.User) + if err != nil { + return false } if ctx.IsSigned { - accessCheck, _ := models.HasAccess(ctx.User.ID, repository, accessMode) - return accessCheck + return perm.UnitAccessMode(models.UnitTypeCode) >= accessMode } user, repo, opStr, err := parseToken(authorization) @@ -511,8 +511,11 @@ func authenticate(ctx *context.Context, repository *models.Repository, authoriza } ctx.User = user if opStr == "basic" { - accessCheck, _ := models.HasAccess(ctx.User.ID, repository, accessMode) - return accessCheck + perm, err = models.GetUserRepoPermission(repository, ctx.User) + if err != nil { + return false + } + return perm.UnitAccessMode(models.UnitTypeCode) >= accessMode } if repository.ID == repo.ID { if requireWrite && opStr != "upload" { diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 916490767ee1c..a0117794ad54f 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -663,7 +663,12 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) } } - if !ctx.User.IsWriterOfRepo(headRepo) && !ctx.User.IsAdmin { + perm, err := models.GetUserRepoPermission(headRepo, ctx.User) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return nil, nil, nil, nil, "", "" + } + if !perm.CanWrite(models.UnitTypeCode) { log.Trace("ParseCompareInfo[%d]: does not have write access or site admin", baseRepo.ID) ctx.Status(404) return nil, nil, nil, nil, "", "" diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 0d90c9297212f..4dee7351bedac 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -370,7 +370,7 @@ func NewIssue(ctx *context.Context) { } // ValidateRepoMetas check and returns repository's meta informations -func ValidateRepoMetas(ctx *context.Context, form auth.CreateIssueForm) ([]int64, []int64, int64) { +func ValidateRepoMetas(ctx *context.Context, form auth.CreateIssueForm, isPull bool) ([]int64, []int64, int64) { var ( repo = ctx.Repo.Repository err error @@ -428,9 +428,19 @@ func ValidateRepoMetas(ctx *context.Context, form auth.CreateIssueForm) ([]int64 // Check if the passed assignees actually exists and has write access to the repo for _, aID := range assigneeIDs { - _, err = repo.GetUserIfHasWriteAccess(aID) + user, err := models.GetUserByID(aID) if err != nil { - ctx.ServerError("GetUserIfHasWriteAccess", err) + ctx.ServerError("GetUserByID", err) + return nil, nil, 0 + } + + perm, err := models.GetUserRepoPermission(repo, user) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return nil, nil, 0 + } + if !perm.CanWriteIssuesOrPulls(isPull) { + ctx.ServerError("CanWriteIssuesOrPulls", fmt.Errorf("No permission for %s", user.Name)) return nil, nil, 0 } } @@ -459,7 +469,7 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) { attachments []string ) - labelIDs, assigneeIDs, milestoneID := ValidateRepoMetas(ctx, form) + labelIDs, assigneeIDs, milestoneID := ValidateRepoMetas(ctx, form, false) if ctx.Written() { return } @@ -499,31 +509,23 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) { // commentTag returns the CommentTag for a comment in/with the given repo, poster and issue func commentTag(repo *models.Repository, poster *models.User, issue *models.Issue) (models.CommentTag, error) { - if repo.IsOwnedBy(poster.ID) { - return models.CommentTagOwner, nil - } else if repo.Owner.IsOrganization() { - isOwner, err := repo.Owner.IsOwnedBy(poster.ID) - if err != nil { - return models.CommentTagNone, err - } else if isOwner { - return models.CommentTagOwner, nil - } + perm, err := models.GetUserRepoPermission(repo, poster) + if err != nil { + return models.CommentTagNone, err } - if poster.IsWriterOfRepo(repo) { - return models.CommentTagWriter, nil + if perm.IsOwner() { + return models.CommentTagOwner, nil } else if poster.ID == issue.PosterID { return models.CommentTagPoster, nil + } else if perm.CanWrite(models.UnitTypeCode) { + return models.CommentTagWriter, nil } + return models.CommentTagNone, nil } // ViewIssue render issue view page func ViewIssue(ctx *context.Context) { - ctx.Data["RequireHighlightJS"] = true - ctx.Data["RequireDropzone"] = true - ctx.Data["RequireTribute"] = true - renderAttachmentSettings(ctx) - issue, err := models.GetIssueByIndex(ctx.Repo.Repository.ID, ctx.ParamsInt64(":index")) if err != nil { if models.IsErrIssueNotExist(err) { @@ -533,25 +535,6 @@ func ViewIssue(ctx *context.Context) { } return } - ctx.Data["Title"] = fmt.Sprintf("#%d - %s", issue.Index, issue.Title) - - var iw *models.IssueWatch - var exists bool - if ctx.User != nil { - iw, exists, err = models.GetIssueWatch(ctx.User.ID, issue.ID) - if err != nil { - ctx.ServerError("GetIssueWatch", err) - return - } - if !exists { - iw = &models.IssueWatch{ - UserID: ctx.User.ID, - IssueID: issue.ID, - IsWatching: models.IsWatching(ctx.User.ID, ctx.Repo.Repository.ID), - } - } - } - ctx.Data["IssueWatch"] = iw // Make sure type and URL matches. if ctx.Params(":type") == "issues" && issue.IsPull { @@ -577,6 +560,31 @@ func ViewIssue(ctx *context.Context) { ctx.Data["PageIsIssueList"] = true } + ctx.Data["RequireHighlightJS"] = true + ctx.Data["RequireDropzone"] = true + ctx.Data["RequireTribute"] = true + renderAttachmentSettings(ctx) + + ctx.Data["Title"] = fmt.Sprintf("#%d - %s", issue.Index, issue.Title) + + var iw *models.IssueWatch + var exists bool + if ctx.User != nil { + iw, exists, err = models.GetIssueWatch(ctx.User.ID, issue.ID) + if err != nil { + ctx.ServerError("GetIssueWatch", err) + return + } + if !exists { + iw = &models.IssueWatch{ + UserID: ctx.User.ID, + IssueID: issue.ID, + IsWatching: models.IsWatching(ctx.User.ID, ctx.Repo.Repository.ID), + } + } + } + ctx.Data["IssueWatch"] = iw + issue.RenderedContent = string(markdown.Render([]byte(issue.Content), ctx.Repo.RepoLink, ctx.Repo.Repository.ComposeMetas())) @@ -762,13 +770,20 @@ func ViewIssue(ctx *context.Context) { if ctx.IsSigned { if err := pull.GetHeadRepo(); err != nil { log.Error(4, "GetHeadRepo: %v", err) - } else if pull.HeadRepo != nil && pull.HeadBranch != pull.HeadRepo.DefaultBranch && ctx.User.IsWriterOfRepo(pull.HeadRepo) { - // Check if branch is not protected - if protected, err := pull.HeadRepo.IsProtectedBranch(pull.HeadBranch, ctx.User); err != nil { - log.Error(4, "IsProtectedBranch: %v", err) - } else if !protected { - canDelete = true - ctx.Data["DeleteBranchLink"] = ctx.Repo.RepoLink + "/pulls/" + com.ToStr(issue.Index) + "/cleanup" + } else if pull.HeadRepo != nil && pull.HeadBranch != pull.HeadRepo.DefaultBranch { + perm, err := models.GetUserRepoPermission(pull.HeadRepo, ctx.User) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return + } + if perm.CanWrite(models.UnitTypeCode) { + // Check if branch is not protected + if protected, err := pull.HeadRepo.IsProtectedBranch(pull.HeadBranch, ctx.User); err != nil { + log.Error(4, "IsProtectedBranch: %v", err) + } else if !protected { + canDelete = true + ctx.Data["DeleteBranchLink"] = ctx.Repo.RepoLink + "/pulls/" + com.ToStr(issue.Index) + "/cleanup" + } } } } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index da94b952a3d54..75294515dddb6 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -675,7 +675,12 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, * } } - if !ctx.User.IsWriterOfRepo(headRepo) && !ctx.User.IsAdmin { + perm, err := models.GetUserRepoPermission(headRepo, ctx.User) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return nil, nil, nil, nil, "", "" + } + if !perm.CanWrite(models.UnitTypeCode) { log.Trace("ParseCompareInfo[%d]: does not have write access or site admin", baseRepo.ID) ctx.NotFound("ParseCompareInfo", nil) return nil, nil, nil, nil, "", "" @@ -829,7 +834,7 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) return } - labelIDs, assigneeIDs, milestoneID := ValidateRepoMetas(ctx, form) + labelIDs, assigneeIDs, milestoneID := ValidateRepoMetas(ctx, form, true) if ctx.Written() { return } @@ -975,7 +980,12 @@ func CleanUpPullRequest(ctx *context.Context) { return } - if !ctx.User.IsWriterOfRepo(pr.HeadRepo) { + perm, err := models.GetUserRepoPermission(pr.HeadRepo, ctx.User) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return + } + if !perm.CanWrite(models.UnitTypeCode) { ctx.NotFound("CleanUpPullRequest", nil) return } diff --git a/routers/repo/wiki.go b/routers/repo/wiki.go index a36e0977edd65..b6cb64f96e3ed 100644 --- a/routers/repo/wiki.go +++ b/routers/repo/wiki.go @@ -237,14 +237,15 @@ func Wiki(ctx *context.Context) { // WikiPages render wiki pages list page func WikiPages(ctx *context.Context) { - ctx.Data["Title"] = ctx.Tr("repo.wiki.pages") - ctx.Data["PageIsWiki"] = true - ctx.Data["CanWriteWiki"] = ctx.Repo.CanWrite(models.UnitTypeWiki) if !ctx.Repo.Repository.HasWiki() { ctx.Redirect(ctx.Repo.RepoLink + "/wiki") return } + ctx.Data["Title"] = ctx.Tr("repo.wiki.pages") + ctx.Data["PageIsWiki"] = true + ctx.Data["CanWriteWiki"] = ctx.Repo.CanWrite(models.UnitTypeWiki) + wikiRepo, commit, err := findWikiRepoCommit(ctx) if err != nil { return diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 8edbc51bc79df..8924da210490b 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -599,7 +599,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/upload-file", repo.UploadFileToServer) m.Post("/upload-remove", bindIgnErr(auth.RemoveUploadFileForm{}), repo.RemoveUploadFileFromServer) }, context.RepoRef(), repo.MustBeEditable, repo.MustBeAbleToUpload) - }, repo.MustBeNotBare, reqRepoCodeWriter) + }, reqRepoCodeWriter, repo.MustBeNotBare) m.Group("/branches", func() { m.Group("/_new/", func() { diff --git a/routers/user/home.go b/routers/user/home.go index 49c65795564ae..b57eeb4a73d23 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -286,7 +286,12 @@ func Issues(ctx *context.Context) { repo := showReposMap[repoID] // Check if user has access to given repository. - if !repo.IsOwnedBy(ctxUser.ID) && !repo.HasAccess(ctxUser) { + perm, err := models.GetUserRepoPermission(repo, ctxUser) + if err != nil { + ctx.ServerError("GetUserRepoPermission", fmt.Errorf("[%d]%v", repoID, err)) + return + } + if !perm.CanAccess(models.UnitTypeIssues) { ctx.Status(404) return } From 11bde943868c9e7b50a7c6a4eef846c79fd2ecf6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sat, 17 Nov 2018 13:24:30 +0800 Subject: [PATCH 18/31] refactor AccessLevel --- models/access.go | 12 ++++++++++-- models/issue.go | 12 ++++++------ models/issue_comment.go | 6 +++--- models/issue_milestone.go | 2 +- models/org_team_test.go | 2 +- models/pull.go | 4 ++-- models/release.go | 6 +++--- models/repo.go | 4 ++-- modules/context/repo.go | 2 +- modules/private/internal.go | 6 +++--- routers/api/v1/org/team.go | 6 +++--- routers/api/v1/repo/fork.go | 3 +-- routers/api/v1/repo/repo.go | 7 +------ routers/api/v1/user/repo.go | 7 ++----- routers/api/v1/user/star.go | 10 +++++----- routers/api/v1/user/watch.go | 10 +++++----- routers/private/internal.go | 6 +++--- 17 files changed, 52 insertions(+), 53 deletions(-) diff --git a/models/access.go b/models/access.go index 98ead19a0beb2..3713dfc6dedfa 100644 --- a/models/access.go +++ b/models/access.go @@ -82,8 +82,16 @@ func accessLevel(e Engine, userID int64, repo *Repository) (AccessMode, error) { // AccessLevel returns the Access a user has to a repository. Will return NoneAccess if the // user does not have access. -func AccessLevel(userID int64, repo *Repository) (AccessMode, error) { - return accessLevel(x, userID, repo) +func AccessLevel(user *User, repo *Repository) (AccessMode, error) { + return accessLevelUnit(user, repo, UnitTypeCode) +} + +func accessLevelUnit(user *User, repo *Repository, unitType UnitType) (AccessMode, error) { + perm, err := GetUserRepoPermission(repo, user) + if err != nil { + return AccessModeNone, err + } + return perm.UnitAccessMode(UnitTypeCode), nil } func hasAccess(e Engine, userID int64, repo *Repository, testMode AccessMode) (bool, error) { diff --git a/models/issue.go b/models/issue.go index c0c29c2013e88..5c5960d6cb7aa 100644 --- a/models/issue.go +++ b/models/issue.go @@ -385,7 +385,7 @@ func (issue *Issue) sendLabelUpdatedWebhook(doer *User) { return } - mode, _ := AccessLevel(issue.Poster.ID, issue.Repo) + mode, _ := AccessLevel(issue.Poster, issue.Repo) if issue.IsPull { if err = issue.loadPullRequest(x); err != nil { log.Error(4, "loadPullRequest: %v", err) @@ -533,7 +533,7 @@ func (issue *Issue) ClearLabels(doer *User) (err error) { return fmt.Errorf("loadPoster: %v", err) } - mode, _ := AccessLevel(issue.Poster.ID, issue.Repo) + mode, _ := AccessLevel(issue.Poster, issue.Repo) if issue.IsPull { err = issue.PullRequest.LoadIssue() if err != nil { @@ -727,7 +727,7 @@ func (issue *Issue) ChangeStatus(doer *User, repo *Repository, isClosed bool) (e } sess.Close() - mode, _ := AccessLevel(issue.Poster.ID, issue.Repo) + mode, _ := AccessLevel(issue.Poster, issue.Repo) if issue.IsPull { // Merge pull request calls issue.changeStatus so we need to handle separately. issue.PullRequest.Issue = issue @@ -789,7 +789,7 @@ func (issue *Issue) ChangeTitle(doer *User, title string) (err error) { return err } - mode, _ := AccessLevel(issue.Poster.ID, issue.Repo) + mode, _ := AccessLevel(issue.Poster, issue.Repo) if issue.IsPull { issue.PullRequest.Issue = issue err = PrepareWebhooks(issue.Repo, HookEventPullRequest, &api.PullRequestPayload{ @@ -855,7 +855,7 @@ func (issue *Issue) ChangeContent(doer *User, content string) (err error) { return fmt.Errorf("UpdateIssueCols: %v", err) } - mode, _ := AccessLevel(issue.Poster.ID, issue.Repo) + mode, _ := AccessLevel(issue.Poster, issue.Repo) if issue.IsPull { issue.PullRequest.Issue = issue err = PrepareWebhooks(issue.Repo, HookEventPullRequest, &api.PullRequestPayload{ @@ -1075,7 +1075,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in log.Error(4, "MailParticipants: %v", err) } - mode, _ := AccessLevel(issue.Poster.ID, issue.Repo) + mode, _ := AccessLevel(issue.Poster, issue.Repo) if err = PrepareWebhooks(repo, HookEventIssues, &api.IssuePayload{ Action: api.HookIssueOpened, Index: issue.Index, diff --git a/models/issue_comment.go b/models/issue_comment.go index 0085c7a732ae5..96b162ca196b2 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -795,7 +795,7 @@ func CreateIssueComment(doer *User, repo *Repository, issue *Issue, content stri return nil, fmt.Errorf("CreateComment: %v", err) } - mode, _ := AccessLevel(doer.ID, repo) + mode, _ := AccessLevel(doer, repo) if err = PrepareWebhooks(repo, HookEventIssueComment, &api.IssueCommentPayload{ Action: api.HookIssueCommentCreated, Issue: issue.APIFormat(), @@ -990,7 +990,7 @@ func UpdateComment(doer *User, c *Comment, oldContent string) error { return err } - mode, _ := AccessLevel(doer.ID, c.Issue.Repo) + mode, _ := AccessLevel(doer, c.Issue.Repo) if err := PrepareWebhooks(c.Issue.Repo, HookEventIssueComment, &api.IssueCommentPayload{ Action: api.HookIssueCommentEdited, Issue: c.Issue.APIFormat(), @@ -1047,7 +1047,7 @@ func DeleteComment(doer *User, comment *Comment) error { return err } - mode, _ := AccessLevel(doer.ID, comment.Issue.Repo) + mode, _ := AccessLevel(doer, comment.Issue.Repo) if err := PrepareWebhooks(comment.Issue.Repo, HookEventIssueComment, &api.IssueCommentPayload{ Action: api.HookIssueCommentDeleted, diff --git a/models/issue_milestone.go b/models/issue_milestone.go index ead3e5a4f65df..2e512d7ba474c 100644 --- a/models/issue_milestone.go +++ b/models/issue_milestone.go @@ -377,7 +377,7 @@ func ChangeMilestoneAssign(issue *Issue, doer *User, oldMilestoneID int64) (err return err } - mode, _ := AccessLevel(doer.ID, issue.Repo) + mode, _ := AccessLevel(doer, issue.Repo) if issue.IsPull { err = issue.PullRequest.LoadIssue() if err != nil { diff --git a/models/org_team_test.go b/models/org_team_test.go index 05429c7cc236c..f9f1f289ec7c0 100644 --- a/models/org_team_test.go +++ b/models/org_team_test.go @@ -243,7 +243,7 @@ func TestDeleteTeam(t *testing.T) { // check that team members don't have "leftover" access to repos user := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User) repo := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository) - accessMode, err := AccessLevel(user.ID, repo) + accessMode, err := AccessLevel(user, repo) assert.NoError(t, err) assert.True(t, accessMode < AccessModeWrite) } diff --git a/models/pull.go b/models/pull.go index 79f6d7005d56a..e97faa8f519de 100644 --- a/models/pull.go +++ b/models/pull.go @@ -458,7 +458,7 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle return nil } - mode, _ := AccessLevel(doer.ID, pr.Issue.Repo) + mode, _ := AccessLevel(doer, pr.Issue.Repo) if err = PrepareWebhooks(pr.Issue.Repo, HookEventPullRequest, &api.PullRequestPayload{ Action: api.HookIssueClosed, Index: pr.Index, @@ -787,7 +787,7 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str pr.Issue = pull pull.PullRequest = pr - mode, _ := AccessLevel(pull.Poster.ID, repo) + mode, _ := AccessLevel(pull.Poster, repo) if err = PrepareWebhooks(repo, HookEventPullRequest, &api.PullRequestPayload{ Action: api.HookIssueOpened, Index: pull.Index, diff --git a/models/release.go b/models/release.go index 44d028a26f3b5..c3063d8e7c507 100644 --- a/models/release.go +++ b/models/release.go @@ -200,7 +200,7 @@ func CreateRelease(gitRepo *git.Repository, rel *Release, attachmentUUIDs []stri if err := rel.LoadAttributes(); err != nil { log.Error(2, "LoadAttributes: %v", err) } else { - mode, _ := AccessLevel(rel.PublisherID, rel.Repo) + mode, _ := AccessLevel(rel.Publisher, rel.Repo) if err := PrepareWebhooks(rel.Repo, HookEventRelease, &api.ReleasePayload{ Action: api.HookReleasePublished, Release: rel.APIFormat(), @@ -392,7 +392,7 @@ func UpdateRelease(doer *User, gitRepo *git.Repository, rel *Release, attachment err = addReleaseAttachments(rel.ID, attachmentUUIDs) - mode, _ := accessLevel(x, doer.ID, rel.Repo) + mode, _ := AccessLevel(doer, rel.Repo) if err1 := PrepareWebhooks(rel.Repo, HookEventRelease, &api.ReleasePayload{ Action: api.HookReleaseUpdated, Release: rel.APIFormat(), @@ -454,7 +454,7 @@ func DeleteReleaseByID(id int64, u *User, delTag bool) error { return fmt.Errorf("LoadAttributes: %v", err) } - mode, _ := accessLevel(x, u.ID, rel.Repo) + mode, _ := AccessLevel(u, rel.Repo) if err := PrepareWebhooks(rel.Repo, HookEventRelease, &api.ReleasePayload{ Action: api.HookReleaseDeleted, Release: rel.APIFormat(), diff --git a/models/repo.go b/models/repo.go index feece9838cb8c..bd2ef8aa5df6b 100644 --- a/models/repo.go +++ b/models/repo.go @@ -2429,8 +2429,8 @@ func ForkRepository(doer, u *User, oldRepo *Repository, name, desc string) (_ *R return nil, err } - oldMode, _ := AccessLevel(doer.ID, oldRepo) - mode, _ := AccessLevel(doer.ID, repo) + oldMode, _ := AccessLevel(doer, oldRepo) + mode, _ := AccessLevel(doer, repo) if err = PrepareWebhooks(oldRepo, HookEventFork, &api.ForkPayload{ Forkee: oldRepo.APIFormat(oldMode), diff --git a/modules/context/repo.go b/modules/context/repo.go index e40fde25b17f4..55d607a28ac4d 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -204,7 +204,7 @@ func repoAssignment(ctx *Context, repo *models.Repository) { var err error ctx.Repo.Permission, err = models.GetUserRepoPermission(repo, ctx.User) if err != nil { - ctx.ServerError("AccessLevel", err) + ctx.ServerError("GetUserRepoPermission", err) return } diff --git a/modules/private/internal.go b/modules/private/internal.go index 351ff57213f84..e8388c6b7b509 100644 --- a/modules/private/internal.go +++ b/modules/private/internal.go @@ -53,7 +53,7 @@ func newInternalRequest(url, method string) *httplib.Request { // CheckUnitUser check whether user could visit the unit of this repository func CheckUnitUser(userID, repoID int64, isAdmin bool, unitType models.UnitType) (*models.AccessMode, error) { reqURL := setting.LocalURL + fmt.Sprintf("api/internal/repositories/%d/user/%d/checkunituser?isAdmin=%t&unitType=%d", repoID, userID, isAdmin, unitType) - log.GitLogger.Trace("AccessLevel: %s", reqURL) + log.GitLogger.Trace("CheckUnitUser: %s", reqURL) resp, err := newInternalRequest(reqURL, "GET").Response() if err != nil { @@ -75,7 +75,7 @@ func CheckUnitUser(userID, repoID int64, isAdmin bool, unitType models.UnitType) // AccessLevel returns the Access a user has to a repository. Will return NoneAccess if the // user does not have access. -func AccessLevel(userID, repoID int64) (*models.AccessMode, error) { +/*func AccessLevel(userID, repoID int64) (*models.AccessMode, error) { reqURL := setting.LocalURL + fmt.Sprintf("api/internal/repositories/%d/user/%d/accesslevel", repoID, userID) log.GitLogger.Trace("AccessLevel: %s", reqURL) @@ -95,7 +95,7 @@ func AccessLevel(userID, repoID int64) (*models.AccessMode, error) { } return &a, nil -} +}*/ // GetRepositoryByOwnerAndName returns the repository by given ownername and reponame. func GetRepositoryByOwnerAndName(ownerName, repoName string) (*models.Repository, error) { diff --git a/routers/api/v1/org/team.go b/routers/api/v1/org/team.go index 8b67eda42fdf5..a22d25eae3018 100644 --- a/routers/api/v1/org/team.go +++ b/routers/api/v1/org/team.go @@ -311,7 +311,7 @@ func GetTeamRepos(ctx *context.APIContext) { } repos := make([]*api.Repository, len(team.Repos)) for i, repo := range team.Repos { - access, err := models.AccessLevel(ctx.User.ID, repo) + access, err := models.AccessLevel(ctx.User, repo) if err != nil { ctx.Error(500, "GetTeamRepos", err) return @@ -366,7 +366,7 @@ func AddTeamRepository(ctx *context.APIContext) { if ctx.Written() { return } - if access, err := models.AccessLevel(ctx.User.ID, repo); err != nil { + if access, err := models.AccessLevel(ctx.User, repo); err != nil { ctx.Error(500, "AccessLevel", err) return } else if access < models.AccessModeAdmin { @@ -413,7 +413,7 @@ func RemoveTeamRepository(ctx *context.APIContext) { if ctx.Written() { return } - if access, err := models.AccessLevel(ctx.User.ID, repo); err != nil { + if access, err := models.AccessLevel(ctx.User, repo); err != nil { ctx.Error(500, "AccessLevel", err) return } else if access < models.AccessModeAdmin { diff --git a/routers/api/v1/repo/fork.go b/routers/api/v1/repo/fork.go index 843559d523bd6..d10f668712ab2 100644 --- a/routers/api/v1/repo/fork.go +++ b/routers/api/v1/repo/fork.go @@ -7,7 +7,6 @@ package repo import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/context" - "code.gitea.io/gitea/routers/api/v1/utils" api "code.gitea.io/sdk/gitea" ) @@ -40,7 +39,7 @@ func ListForks(ctx *context.APIContext) { } apiForks := make([]*api.Repository, len(forks)) for i, fork := range forks { - access, err := models.AccessLevel(utils.UserID(ctx), fork) + access, err := models.AccessLevel(ctx.User, fork) if err != nil { ctx.Error(500, "AccessLevel", err) return diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 9ce15ae1688d6..6d8125a77fafb 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -184,11 +184,6 @@ func Search(ctx *context.APIContext) { return } - var userID int64 - if ctx.IsSigned { - userID = ctx.User.ID - } - results := make([]*api.Repository, len(repos)) for i, repo := range repos { if err = repo.GetOwner(); err != nil { @@ -198,7 +193,7 @@ func Search(ctx *context.APIContext) { }) return } - accessMode, err := models.AccessLevel(userID, repo) + accessMode, err := models.AccessLevel(ctx.User, repo) if err != nil { ctx.JSON(500, api.SearchError{ OK: false, diff --git a/routers/api/v1/user/repo.go b/routers/api/v1/user/repo.go index 5dccfac960907..1ddb3bd57baff 100644 --- a/routers/api/v1/user/repo.go +++ b/routers/api/v1/user/repo.go @@ -17,13 +17,10 @@ func listUserRepos(ctx *context.APIContext, u *models.User, private bool) { ctx.Error(500, "GetUserRepositories", err) return } + apiRepos := make([]*api.Repository, 0, len(repos)) - var ctxUserID int64 - if ctx.User != nil { - ctxUserID = ctx.User.ID - } for i := range repos { - access, err := models.AccessLevel(ctxUserID, repos[i]) + access, err := models.AccessLevel(ctx.User, repos[i]) if err != nil { ctx.Error(500, "AccessLevel", err) return diff --git a/routers/api/v1/user/star.go b/routers/api/v1/user/star.go index 1cf4f5239cc59..b0016399c8db6 100644 --- a/routers/api/v1/user/star.go +++ b/routers/api/v1/user/star.go @@ -13,15 +13,15 @@ import ( // getStarredRepos returns the repos that the user with the specified userID has // starred -func getStarredRepos(userID int64, private bool) ([]*api.Repository, error) { - starredRepos, err := models.GetStarredRepos(userID, private) +func getStarredRepos(user *models.User, private bool) ([]*api.Repository, error) { + starredRepos, err := models.GetStarredRepos(user.ID, private) if err != nil { return nil, err } repos := make([]*api.Repository, len(starredRepos)) for i, starred := range starredRepos { - access, err := models.AccessLevel(userID, starred) + access, err := models.AccessLevel(user, starred) if err != nil { return nil, err } @@ -48,7 +48,7 @@ func GetStarredRepos(ctx *context.APIContext) { // "$ref": "#/responses/RepositoryList" user := GetUserByParams(ctx) private := user.ID == ctx.User.ID - repos, err := getStarredRepos(user.ID, private) + repos, err := getStarredRepos(user, private) if err != nil { ctx.Error(500, "getStarredRepos", err) } @@ -65,7 +65,7 @@ func GetMyStarredRepos(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/RepositoryList" - repos, err := getStarredRepos(ctx.User.ID, true) + repos, err := getStarredRepos(ctx.User, true) if err != nil { ctx.Error(500, "getStarredRepos", err) } diff --git a/routers/api/v1/user/watch.go b/routers/api/v1/user/watch.go index 2971bf6869c8d..4afa18be2a75d 100644 --- a/routers/api/v1/user/watch.go +++ b/routers/api/v1/user/watch.go @@ -14,15 +14,15 @@ import ( // getWatchedRepos returns the repos that the user with the specified userID is // watching -func getWatchedRepos(userID int64, private bool) ([]*api.Repository, error) { - watchedRepos, err := models.GetWatchedRepos(userID, private) +func getWatchedRepos(user *models.User, private bool) ([]*api.Repository, error) { + watchedRepos, err := models.GetWatchedRepos(user.ID, private) if err != nil { return nil, err } repos := make([]*api.Repository, len(watchedRepos)) for i, watched := range watchedRepos { - access, err := models.AccessLevel(userID, watched) + access, err := models.AccessLevel(user, watched) if err != nil { return nil, err } @@ -49,7 +49,7 @@ func GetWatchedRepos(ctx *context.APIContext) { // "$ref": "#/responses/RepositoryList" user := GetUserByParams(ctx) private := user.ID == ctx.User.ID - repos, err := getWatchedRepos(user.ID, private) + repos, err := getWatchedRepos(user, private) if err != nil { ctx.Error(500, "getWatchedRepos", err) } @@ -66,7 +66,7 @@ func GetMyWatchedRepos(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/RepositoryList" - repos, err := getWatchedRepos(ctx.User.ID, true) + repos, err := getWatchedRepos(ctx.User, true) if err != nil { ctx.Error(500, "getWatchedRepos", err) } diff --git a/routers/private/internal.go b/routers/private/internal.go index c9f9e4fcb8b76..9bd15fcfc56ec 100644 --- a/routers/private/internal.go +++ b/routers/private/internal.go @@ -39,7 +39,7 @@ func GetRepositoryByOwnerAndName(ctx *macaron.Context) { } //AccessLevel chainload to models.AccessLevel -func AccessLevel(ctx *macaron.Context) { +/*func AccessLevel(ctx *macaron.Context) { repoID := ctx.ParamsInt64(":repoid") userID := ctx.ParamsInt64(":userid") repo, err := models.GetRepositoryByID(repoID) @@ -57,7 +57,7 @@ func AccessLevel(ctx *macaron.Context) { return } ctx.JSON(200, al) -} +}*/ //CheckUnitUser chainload to models.CheckUnitUser func CheckUnitUser(ctx *macaron.Context) { @@ -98,7 +98,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/ssh/:id/user", GetUserByKeyID) m.Post("/ssh/:id/update", UpdatePublicKey) m.Post("/repositories/:repoid/keys/:keyid/update", UpdateDeployKey) - m.Get("/repositories/:repoid/user/:userid/accesslevel", AccessLevel) + //m.Get("/repositories/:repoid/user/:userid/accesslevel", AccessLevel) m.Get("/repositories/:repoid/user/:userid/checkunituser", CheckUnitUser) m.Get("/repositories/:repoid/has-keys/:keyid", HasDeployKey) m.Post("/push/update", PushUpdate) From ba60cc8108853e27a0dd36bf6f1a67577d55639f Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sat, 17 Nov 2018 13:44:57 +0800 Subject: [PATCH 19/31] fix bug --- cmd/serv.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/serv.go b/cmd/serv.go index 9e018aecc2620..51b0b4984b39d 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -193,7 +193,7 @@ func runServ(c *cli.Context) error { keyID int64 user *models.User ) - if requestedMode == models.AccessModeWrite || repo.IsPrivate { + if requestedMode == models.AccessModeWrite || repo.IsPrivate || setting.Service.RequireSignInView { keys := strings.Split(c.Args()[0], "-") if len(keys) != 2 { fail("Key ID format error", "Invalid key argument: %s", c.Args()[0]) From a978acc877e2432951483588ae58077159900d59 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sat, 17 Nov 2018 13:48:14 +0800 Subject: [PATCH 20/31] fix tests --- models/access_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/models/access_test.go b/models/access_test.go index 46d6f723ea8cf..ba44509f5451b 100644 --- a/models/access_test.go +++ b/models/access_test.go @@ -29,19 +29,19 @@ func TestAccessLevel(t *testing.T) { repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository) assert.True(t, repo2.IsPrivate) - level, err := AccessLevel(user1.ID, repo1) + level, err := AccessLevel(user1, repo1) assert.NoError(t, err) assert.Equal(t, AccessModeOwner, level) - level, err = AccessLevel(user1.ID, repo2) + level, err = AccessLevel(user1, repo2) assert.NoError(t, err) assert.Equal(t, AccessModeWrite, level) - level, err = AccessLevel(user2.ID, repo1) + level, err = AccessLevel(user2, repo1) assert.NoError(t, err) assert.Equal(t, AccessModeRead, level) - level, err = AccessLevel(user2.ID, repo2) + level, err = AccessLevel(user2, repo2) assert.NoError(t, err) assert.Equal(t, AccessModeNone, level) } From d161315ac90f2885d1b11182d57970a966c05401 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sat, 17 Nov 2018 14:12:13 +0800 Subject: [PATCH 21/31] fix tests --- models/access_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/models/access_test.go b/models/access_test.go index ba44509f5451b..854af406884d5 100644 --- a/models/access_test.go +++ b/models/access_test.go @@ -20,28 +20,28 @@ var accessModes = []AccessMode{ func TestAccessLevel(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - user1 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) - user2 := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User) + user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) + user5 := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User) // A public repository owned by User 2 repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) assert.False(t, repo1.IsPrivate) // A private repository owned by Org 3 - repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository) - assert.True(t, repo2.IsPrivate) + repo3 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository) + assert.True(t, repo3.IsPrivate) - level, err := AccessLevel(user1, repo1) + level, err := AccessLevel(user2, repo1) assert.NoError(t, err) assert.Equal(t, AccessModeOwner, level) - level, err = AccessLevel(user1, repo2) + level, err = AccessLevel(user2, repo3) assert.NoError(t, err) - assert.Equal(t, AccessModeWrite, level) + assert.Equal(t, AccessModeOwner, level) - level, err = AccessLevel(user2, repo1) + level, err = AccessLevel(user5, repo1) assert.NoError(t, err) assert.Equal(t, AccessModeRead, level) - level, err = AccessLevel(user2, repo2) + level, err = AccessLevel(user5, repo3) assert.NoError(t, err) assert.Equal(t, AccessModeNone, level) } From e5e165c83f12b321d5e7f9e3f4cd0d11973e2f8c Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sat, 17 Nov 2018 21:32:18 +0800 Subject: [PATCH 22/31] fix tests --- routers/repo/setting_protected_branch.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index db12f0afcdf9e..c8f6f843da3b4 100644 --- a/routers/repo/setting_protected_branch.go +++ b/routers/repo/setting_protected_branch.go @@ -165,12 +165,21 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm) } } + var whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams []int64 protectBranch.EnableWhitelist = f.EnableWhitelist - whitelistUsers, _ := base.StringsToInt64s(strings.Split(f.WhitelistUsers, ",")) - whitelistTeams, _ := base.StringsToInt64s(strings.Split(f.WhitelistTeams, ",")) + if strings.TrimSpace(f.WhitelistUsers) != "" { + whitelistUsers, _ = base.StringsToInt64s(strings.Split(f.WhitelistUsers, ",")) + } + if strings.TrimSpace(f.WhitelistTeams) != "" { + whitelistTeams, _ = base.StringsToInt64s(strings.Split(f.WhitelistTeams, ",")) + } protectBranch.EnableMergeWhitelist = f.EnableMergeWhitelist - mergeWhitelistUsers, _ := base.StringsToInt64s(strings.Split(f.MergeWhitelistUsers, ",")) - mergeWhitelistTeams, _ := base.StringsToInt64s(strings.Split(f.MergeWhitelistTeams, ",")) + if strings.TrimSpace(f.MergeWhitelistUsers) != "" { + mergeWhitelistUsers, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistUsers, ",")) + } + if strings.TrimSpace(f.MergeWhitelistTeams) != "" { + mergeWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistTeams, ",")) + } err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, whitelistUsers, whitelistTeams, mergeWhitelistUsers, mergeWhitelistTeams) if err != nil { ctx.ServerError("UpdateProtectBranch", err) From 92530156f2940fecec51c9eb7c6a6e97a99373bb Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 18 Nov 2018 10:12:10 +0800 Subject: [PATCH 23/31] fix AccessLevel --- integrations/api_repo_test.go | 2 +- models/access.go | 24 ------------- models/access_test.go | 29 +++++++--------- models/issue.go | 6 +++- models/issue_assignees.go | 7 ++-- models/org_team.go | 6 ++-- models/release.go | 7 ---- models/repo_permission.go | 63 ++++++++++++++++++++++++++++++++++- 8 files changed, 88 insertions(+), 56 deletions(-) diff --git a/integrations/api_repo_test.go b/integrations/api_repo_test.go index da748942f6435..237c4eea9abb1 100644 --- a/integrations/api_repo_test.go +++ b/integrations/api_repo_test.go @@ -164,7 +164,7 @@ func TestAPISearchRepo(t *testing.T) { assert.Len(t, body.Data, expected.count) for _, repo := range body.Data { r := getRepo(t, repo.ID) - hasAccess, err := models.HasAccess(userID, r, models.AccessModeRead) + hasAccess, err := models.HasAccess(userID, r) assert.NoError(t, err) assert.True(t, hasAccess) diff --git a/models/access.go b/models/access.go index 3713dfc6dedfa..34d76953f57b2 100644 --- a/models/access.go +++ b/models/access.go @@ -80,30 +80,6 @@ func accessLevel(e Engine, userID int64, repo *Repository) (AccessMode, error) { return a.Mode, nil } -// AccessLevel returns the Access a user has to a repository. Will return NoneAccess if the -// user does not have access. -func AccessLevel(user *User, repo *Repository) (AccessMode, error) { - return accessLevelUnit(user, repo, UnitTypeCode) -} - -func accessLevelUnit(user *User, repo *Repository, unitType UnitType) (AccessMode, error) { - perm, err := GetUserRepoPermission(repo, user) - if err != nil { - return AccessModeNone, err - } - return perm.UnitAccessMode(UnitTypeCode), nil -} - -func hasAccess(e Engine, userID int64, repo *Repository, testMode AccessMode) (bool, error) { - mode, err := accessLevel(e, userID, repo) - return testMode <= mode, err -} - -// HasAccess returns true if user has access to repo -func HasAccess(userID int64, repo *Repository, testMode AccessMode) (bool, error) { - return hasAccess(x, userID, repo, testMode) -} - type repoAccess struct { Access `xorm:"extends"` Repository `xorm:"extends"` diff --git a/models/access_test.go b/models/access_test.go index 854af406884d5..d6a1c92b90202 100644 --- a/models/access_test.go +++ b/models/access_test.go @@ -58,23 +58,18 @@ func TestHasAccess(t *testing.T) { repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository) assert.True(t, repo2.IsPrivate) - for _, accessMode := range accessModes { - has, err := HasAccess(user1.ID, repo1, accessMode) - assert.NoError(t, err) - assert.True(t, has) - - has, err = HasAccess(user1.ID, repo2, accessMode) - assert.NoError(t, err) - assert.Equal(t, accessMode <= AccessModeWrite, has) - - has, err = HasAccess(user2.ID, repo1, accessMode) - assert.NoError(t, err) - assert.Equal(t, accessMode <= AccessModeRead, has) - - has, err = HasAccess(user2.ID, repo2, accessMode) - assert.NoError(t, err) - assert.Equal(t, accessMode <= AccessModeNone, has) - } + has, err := HasAccess(user1.ID, repo1) + assert.NoError(t, err) + assert.True(t, has) + + has, err = HasAccess(user1.ID, repo2) + assert.NoError(t, err) + + has, err = HasAccess(user2.ID, repo1) + assert.NoError(t, err) + + has, err = HasAccess(user2.ID, repo2) + assert.NoError(t, err) } func TestUser_GetRepositoryAccesses(t *testing.T) { diff --git a/models/issue.go b/models/issue.go index 5c5960d6cb7aa..9e0166ef65d68 100644 --- a/models/issue.go +++ b/models/issue.go @@ -950,7 +950,11 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { // Check for and validate assignees if len(opts.AssigneeIDs) > 0 { for _, assigneeID := range opts.AssigneeIDs { - valid, err := hasAccess(e, assigneeID, opts.Repo, AccessModeWrite) + user, err := getUserByID(e, assigneeID) + if err != nil { + return fmt.Errorf("getUserByID [user_id: %d, repo_id: %d]: %v", assigneeID, opts.Repo.ID, err) + } + valid, err := canBeAssigned(e, user, opts.Repo) if err != nil { return fmt.Errorf("hasAccess [user_id: %d, repo_id: %d]: %v", assigneeID, opts.Repo.ID, err) } diff --git a/models/issue_assignees.go b/models/issue_assignees.go index 09d4d310dc6bd..f330ade1c8eec 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -159,13 +159,14 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in return fmt.Errorf("createAssigneeComment: %v", err) } - // if issue/pull is in the middle of creation - don't call webhook + // if pull request is in the middle of creation - don't call webhook if isCreate { return nil } - mode, _ := accessLevel(sess, doer.ID, issue.Repo) if issue.IsPull { + mode, _ := accessLevelUnit(sess, doer, issue.Repo, UnitTypePullRequests) + if err = issue.loadPullRequest(sess); err != nil { return fmt.Errorf("loadPullRequest: %v", err) } @@ -186,6 +187,8 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in return nil } } else { + mode, _ := accessLevelUnit(sess, doer, issue.Repo, UnitTypeIssues) + apiIssue := &api.IssuePayload{ Index: issue.Index, Issue: issue.APIFormat(), diff --git a/models/org_team.go b/models/org_team.go index 53c1ec34d8bdb..cad4af25066ea 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -177,7 +177,7 @@ func (t *Team) removeRepository(e Engine, repo *Repository, recalculate bool) (e return fmt.Errorf("getTeamUsersByTeamID: %v", err) } for _, teamUser := range teamUsers { - has, err := hasAccess(e, teamUser.UID, repo, AccessModeRead) + has, err := hasAccess(e, teamUser.UID, repo) if err != nil { return err } else if has { @@ -434,7 +434,7 @@ func DeleteTeam(t *Team) error { // Remove watches from all users and now unaccessible repos for _, user := range t.Members { - has, err := hasAccess(sess, user.ID, repo, AccessModeRead) + has, err := hasAccess(sess, user.ID, repo) if err != nil { return err } else if has { @@ -652,7 +652,7 @@ func removeTeamMember(e *xorm.Session, team *Team, userID int64) error { } // Remove watches from now unaccessible - has, err := hasAccess(e, userID, repo, AccessModeRead) + has, err := hasAccess(e, userID, repo) if err != nil { return err } else if has { diff --git a/models/release.go b/models/release.go index c3063d8e7c507..c18e152293029 100644 --- a/models/release.go +++ b/models/release.go @@ -419,13 +419,6 @@ func DeleteReleaseByID(id int64, u *User, delTag bool) error { return fmt.Errorf("GetRepositoryByID: %v", err) } - has, err := HasAccess(u.ID, repo, AccessModeWrite) - if err != nil { - return fmt.Errorf("HasAccess: %v", err) - } else if !has { - return fmt.Errorf("DeleteReleaseByID: permission denied") - } - if delTag { _, stderr, err := process.GetManager().ExecDir(-1, repo.RepoPath(), fmt.Sprintf("DeleteReleaseByID (git tag -d): %d", rel.ID), diff --git a/models/repo_permission.go b/models/repo_permission.go index ad8d6ede37acf..192e036a4dee0 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -191,5 +191,66 @@ func isUserRepoAdmin(e Engine, repo *Repository, user *User) (bool, error) { if err != nil { return false, err } - return mode >= AccessModeAdmin, nil + if mode >= AccessModeAdmin { + return true, nil + } + + teams, err := getUserRepoTeams(e, repo.OwnerID, user.ID, repo.ID) + if err != nil { + return false, err + } + + for _, team := range teams { + if team.Authorize >= AccessModeAdmin { + return true, nil + } + } + return false, nil +} + +// AccessLevel returns the Access a user has to a repository. Will return NoneAccess if the +// user does not have access. +func AccessLevel(user *User, repo *Repository) (AccessMode, error) { + return accessLevelUnit(x, user, repo, UnitTypeCode) +} + +func accessLevelUnit(e Engine, user *User, repo *Repository, unitType UnitType) (AccessMode, error) { + perm, err := getUserRepoPermission(e, repo, user) + if err != nil { + return AccessModeNone, err + } + return perm.UnitAccessMode(UnitTypeCode), nil +} + +func hasAccessUnit(e Engine, user *User, repo *Repository, unitType UnitType, testMode AccessMode) (bool, error) { + mode, err := accessLevelUnit(e, user, repo, unitType) + return testMode <= mode, err +} + +// HasAccessUnit returns ture if user has testMode to the unit of the repository +func HasAccessUnit(user *User, repo *Repository, unitType UnitType, testMode AccessMode) (bool, error) { + return hasAccessUnit(x, user, repo, unitType, testMode) +} + +// canBeAssigned return true if user could be assigned to a repo +// FIXME: user could send PullRequest also could be assigned??? +func canBeAssigned(e Engine, user *User, repo *Repository) (bool, error) { + return hasAccessUnit(e, user, repo, UnitTypeCode, AccessModeWrite) +} + +func hasAccess(e Engine, userID int64, repo *Repository) (bool, error) { + user, err := getUserByID(e, userID) + if err != nil { + return false, err + } + perm, err := getUserRepoPermission(e, repo, user) + if err != nil { + return false, err + } + return perm.HasAccess(), nil +} + +// HasAccess returns true if user has access to repo +func HasAccess(userID int64, repo *Repository) (bool, error) { + return hasAccess(x, userID, repo) } From 2f65f7a8bc03b83d9a4f53b3572cd28d7e8be08b Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 18 Nov 2018 11:35:50 +0800 Subject: [PATCH 24/31] rename CanAccess --- models/issue.go | 2 +- models/lfs_lock.go | 2 +- models/repo.go | 20 ++++---------------- models/repo_permission.go | 19 ++++++++++++------- modules/context/permission.go | 4 ++-- routers/api/v1/api.go | 10 +++++----- routers/repo/activity.go | 6 +++--- routers/repo/issue.go | 14 +++++++------- routers/repo/pull.go | 2 +- routers/repo/wiki.go | 4 ++-- routers/user/home.go | 2 +- 11 files changed, 39 insertions(+), 46 deletions(-) diff --git a/models/issue.go b/models/issue.go index 9e0166ef65d68..26196274fe2b7 100644 --- a/models/issue.go +++ b/models/issue.go @@ -956,7 +956,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { } valid, err := canBeAssigned(e, user, opts.Repo) if err != nil { - return fmt.Errorf("hasAccess [user_id: %d, repo_id: %d]: %v", assigneeID, opts.Repo.ID, err) + return fmt.Errorf("canBeAssigned [user_id: %d, repo_id: %d]: %v", assigneeID, opts.Repo.ID, err) } if !valid { return ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: opts.Repo.Name} diff --git a/models/lfs_lock.go b/models/lfs_lock.go index 377fc41aac5ed..66fc2b2ec4fbf 100644 --- a/models/lfs_lock.go +++ b/models/lfs_lock.go @@ -143,7 +143,7 @@ func CheckLFSAccessForRepo(u *User, repo *Repository, mode AccessMode) error { if err != nil { return err } - if !perm.CanWrite(UnitTypeCode) { + if !perm.CanAccess(mode, UnitTypeCode) { return ErrLFSUnauthorizedAction{repo.ID, u.DisplayName(), mode} } return nil diff --git a/models/repo.go b/models/repo.go index bd2ef8aa5df6b..3de56079f2a5a 100644 --- a/models/repo.go +++ b/models/repo.go @@ -325,6 +325,9 @@ func (repo *Repository) CheckUnitUser(userID int64, isAdmin bool, unitType UnitT } func (repo *Repository) checkUnitUser(e Engine, userID int64, isAdmin bool, unitType UnitType) bool { + if isAdmin { + return true + } user, err := getUserByID(e, userID) if err != nil { return false @@ -334,7 +337,7 @@ func (repo *Repository) checkUnitUser(e Engine, userID int64, isAdmin bool, unit return false } - return perm.CanAccess(unitType) + return perm.CanRead(unitType) } // UnitEnabled if this repository has the given unit enabled @@ -350,21 +353,6 @@ func (repo *Repository) UnitEnabled(tp UnitType) bool { return false } -// AnyUnitEnabled if this repository has the any of the given units enabled -func (repo *Repository) AnyUnitEnabled(tps ...UnitType) bool { - if err := repo.getUnits(x); err != nil { - log.Warn("Error loading repository (ID: %d) units: %s", repo.ID, err.Error()) - } - for _, unit := range repo.Units { - for _, tp := range tps { - if unit.Type == tp { - return true - } - } - } - return false -} - var ( // ErrUnitNotExist organization does not exist ErrUnitNotExist = errors.New("Unit does not exist") diff --git a/models/repo_permission.go b/models/repo_permission.go index 192e036a4dee0..eba50676c19ba 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -43,23 +43,28 @@ func (p *Permission) UnitAccessMode(unitType UnitType) AccessMode { } // CanAccess returns true if user has read access to the unit of the repository -func (p *Permission) CanAccess(unitType UnitType) bool { - return p.UnitAccessMode(unitType) >= AccessModeRead +func (p *Permission) CanAccess(mode AccessMode, unitType UnitType) bool { + return p.UnitAccessMode(unitType) >= mode } // CanAccessAny returns true if user has read access to any of the units of the repository -func (p *Permission) CanAccessAny(unitTypes ...UnitType) bool { +func (p *Permission) CanAccessAny(mode AccessMode, unitTypes ...UnitType) bool { for _, u := range unitTypes { - if p.CanAccess(u) { + if p.CanAccess(mode, u) { return true } } return false } +// CanWrite returns true if user could write to this unit +func (p *Permission) CanRead(unitType UnitType) bool { + return p.CanAccess(AccessModeRead, unitType) +} + // CanWrite returns true if user could write to this unit func (p *Permission) CanWrite(unitType UnitType) bool { - return p.UnitAccessMode(unitType) >= AccessModeWrite + return p.CanAccess(AccessModeWrite, unitType) } // CanWriteIssuesOrPulls returns true if isPull is true and user could write to pull requests and @@ -75,9 +80,9 @@ func (p *Permission) CanWriteIssuesOrPulls(isPull bool) bool { // returns true if isPull is false and user could read to issues func (p *Permission) CanReadIssuesOrPulls(isPull bool) bool { if isPull { - return p.CanAccess(UnitTypePullRequests) + return p.CanRead(UnitTypePullRequests) } - return p.CanAccess(UnitTypeIssues) + return p.CanRead(UnitTypeIssues) } // GetUserRepoPermission returns the user permissions to the repository diff --git a/modules/context/permission.go b/modules/context/permission.go index e13fbb939e6f7..70f86953004bb 100644 --- a/modules/context/permission.go +++ b/modules/context/permission.go @@ -44,7 +44,7 @@ func RequireRepoWriterOr(unitTypes ...models.UnitType) macaron.Handler { // RequireRepoReader returns a macaron middleware for requiring repository read to the specify unitType func RequireRepoReader(unitType models.UnitType) macaron.Handler { return func(ctx *Context) { - if !ctx.Repo.CanAccess(unitType) { + if !ctx.Repo.CanRead(unitType) { ctx.NotFound(ctx.Req.RequestURI, nil) return } @@ -55,7 +55,7 @@ func RequireRepoReader(unitType models.UnitType) macaron.Handler { func RequireRepoReaderOr(unitTypes ...models.UnitType) macaron.Handler { return func(ctx *Context) { for _, unitType := range unitTypes { - if ctx.Repo.CanAccess(unitType) { + if ctx.Repo.CanRead(unitType) { return } } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index b5eedf06e4738..aced2b4ebf5b2 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -221,7 +221,7 @@ func reqAdmin() macaron.Handler { func reqRepoReader(unitType models.UnitType) macaron.Handler { return func(ctx *context.Context) { - if !ctx.Repo.CanAccess(unitType) { + if !ctx.Repo.CanRead(unitType) { ctx.Error(403) return } @@ -343,22 +343,22 @@ func orgAssignment(args ...bool) macaron.Handler { } func mustEnableIssues(ctx *context.APIContext) { - if !ctx.Repo.CanAccess(models.UnitTypeIssues) { + if !ctx.Repo.CanRead(models.UnitTypeIssues) { ctx.Status(404) return } } func mustAllowPulls(ctx *context.Context) { - if !(ctx.Repo.Repository.CanEnablePulls() && ctx.Repo.CanAccess(models.UnitTypePullRequests)) { + if !(ctx.Repo.Repository.CanEnablePulls() && ctx.Repo.CanRead(models.UnitTypePullRequests)) { ctx.Status(404) return } } func mustEnableIssuesOrPulls(ctx *context.Context) { - if !ctx.Repo.CanAccess(models.UnitTypeIssues) && - !(ctx.Repo.Repository.CanEnablePulls() && ctx.Repo.CanAccess(models.UnitTypePullRequests)) { + if !ctx.Repo.CanRead(models.UnitTypeIssues) && + !(ctx.Repo.Repository.CanEnablePulls() && ctx.Repo.CanRead(models.UnitTypePullRequests)) { ctx.Status(404) return } diff --git a/routers/repo/activity.go b/routers/repo/activity.go index 7168728e76aaa..5d90d73506db2 100644 --- a/routers/repo/activity.go +++ b/routers/repo/activity.go @@ -45,9 +45,9 @@ func Activity(ctx *context.Context) { var err error if ctx.Data["Activity"], err = models.GetActivityStats(ctx.Repo.Repository.ID, timeFrom, - ctx.Repo.CanAccess(models.UnitTypeReleases), - ctx.Repo.CanAccess(models.UnitTypeIssues), - ctx.Repo.CanAccess(models.UnitTypePullRequests)); err != nil { + ctx.Repo.CanRead(models.UnitTypeReleases), + ctx.Repo.CanRead(models.UnitTypeIssues), + ctx.Repo.CanRead(models.UnitTypePullRequests)); err != nil { ctx.ServerError("GetActivityStats", err) return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 4dee7351bedac..0dcbf285acb11 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -64,8 +64,8 @@ var ( // MustEnableIssues check if repository enable internal issues func MustEnableIssues(ctx *context.Context) { - if !ctx.Repo.CanAccess(models.UnitTypeIssues) && - !ctx.Repo.CanAccess(models.UnitTypeExternalTracker) { + if !ctx.Repo.CanRead(models.UnitTypeIssues) && + !ctx.Repo.CanRead(models.UnitTypeExternalTracker) { ctx.NotFound("MustEnableIssues", nil) return } @@ -79,7 +79,7 @@ func MustEnableIssues(ctx *context.Context) { // MustAllowPulls check if repository enable pull requests and user have right to do that func MustAllowPulls(ctx *context.Context) { - if !ctx.Repo.Repository.CanEnablePulls() || !ctx.Repo.CanAccess(models.UnitTypePullRequests) { + if !ctx.Repo.Repository.CanEnablePulls() || !ctx.Repo.CanRead(models.UnitTypePullRequests) { ctx.NotFound("MustAllowPulls", nil) return } @@ -859,8 +859,8 @@ func GetActionIssue(ctx *context.Context) *models.Issue { } func checkIssueRights(ctx *context.Context, issue *models.Issue) { - if issue.IsPull && !ctx.Repo.CanAccess(models.UnitTypePullRequests) || - !issue.IsPull && !ctx.Repo.CanAccess(models.UnitTypeIssues) { + if issue.IsPull && !ctx.Repo.CanRead(models.UnitTypePullRequests) || + !issue.IsPull && !ctx.Repo.CanRead(models.UnitTypeIssues) { ctx.NotFound("IssueOrPullRequestUnitNotAllowed", nil) } } @@ -885,8 +885,8 @@ func getActionIssues(ctx *context.Context) []*models.Issue { return nil } // Check access rights for all issues - issueUnitEnabled := ctx.Repo.CanAccess(models.UnitTypeIssues) - prUnitEnabled := ctx.Repo.CanAccess(models.UnitTypePullRequests) + issueUnitEnabled := ctx.Repo.CanRead(models.UnitTypeIssues) + prUnitEnabled := ctx.Repo.CanRead(models.UnitTypePullRequests) for _, issue := range issues { if issue.IsPull && !prUnitEnabled || !issue.IsPull && !issueUnitEnabled { ctx.NotFound("IssueOrPullRequestUnitNotAllowed", nil) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 75294515dddb6..4adfb96e748be 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -63,7 +63,7 @@ func getForkRepository(ctx *context.Context) *models.Repository { return nil } - if forkRepo.IsBare || !perm.CanAccess(models.UnitTypeCode) { + if forkRepo.IsBare || !perm.CanRead(models.UnitTypeCode) { ctx.NotFound("getForkRepository", nil) return nil } diff --git a/routers/repo/wiki.go b/routers/repo/wiki.go index b6cb64f96e3ed..23492363580ed 100644 --- a/routers/repo/wiki.go +++ b/routers/repo/wiki.go @@ -31,8 +31,8 @@ const ( // MustEnableWiki check if wiki is enabled, if external then redirect func MustEnableWiki(ctx *context.Context) { - if !ctx.Repo.CanAccess(models.UnitTypeWiki) && - !ctx.Repo.CanAccess(models.UnitTypeExternalWiki) { + if !ctx.Repo.CanRead(models.UnitTypeWiki) && + !ctx.Repo.CanRead(models.UnitTypeExternalWiki) { ctx.NotFound("MustEnableWiki", nil) return } diff --git a/routers/user/home.go b/routers/user/home.go index b57eeb4a73d23..4c6a9f6cd0957 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -291,7 +291,7 @@ func Issues(ctx *context.Context) { ctx.ServerError("GetUserRepoPermission", fmt.Errorf("[%d]%v", repoID, err)) return } - if !perm.CanAccess(models.UnitTypeIssues) { + if !perm.CanRead(models.UnitTypeIssues) { ctx.Status(404) return } From 962be7892aa931ed86cbed01b98ebef391157179 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 18 Nov 2018 12:42:49 +0800 Subject: [PATCH 25/31] fix tests --- models/repo_permission_test.go | 48 +++++++++++++++++----------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/models/repo_permission_test.go b/models/repo_permission_test.go index 055bc3a4423a2..fd55ae5e52412 100644 --- a/models/repo_permission_test.go +++ b/models/repo_permission_test.go @@ -22,7 +22,7 @@ func TestRepoPermissionPublicNonOrgRepo(t *testing.T) { perm, err := GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.False(t, perm.CanWrite(unit.Type)) } @@ -31,7 +31,7 @@ func TestRepoPermissionPublicNonOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } @@ -40,7 +40,7 @@ func TestRepoPermissionPublicNonOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, collaborator) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } @@ -49,7 +49,7 @@ func TestRepoPermissionPublicNonOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, owner) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } @@ -58,7 +58,7 @@ func TestRepoPermissionPublicNonOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, admin) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } } @@ -75,7 +75,7 @@ func TestRepoPermissionPrivateNonOrgRepo(t *testing.T) { perm, err := GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.False(t, perm.CanAccess(unit.Type)) + assert.False(t, perm.CanRead(unit.Type)) assert.False(t, perm.CanWrite(unit.Type)) } @@ -84,7 +84,7 @@ func TestRepoPermissionPrivateNonOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } @@ -92,7 +92,7 @@ func TestRepoPermissionPrivateNonOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.False(t, perm.CanWrite(unit.Type)) } @@ -101,7 +101,7 @@ func TestRepoPermissionPrivateNonOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, owner) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } @@ -110,7 +110,7 @@ func TestRepoPermissionPrivateNonOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, admin) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } } @@ -127,7 +127,7 @@ func TestRepoPermissionPublicOrgRepo(t *testing.T) { perm, err := GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.False(t, perm.CanWrite(unit.Type)) } @@ -136,7 +136,7 @@ func TestRepoPermissionPublicOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } @@ -144,7 +144,7 @@ func TestRepoPermissionPublicOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.False(t, perm.CanWrite(unit.Type)) } @@ -153,7 +153,7 @@ func TestRepoPermissionPublicOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, owner) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } @@ -162,7 +162,7 @@ func TestRepoPermissionPublicOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, member) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) } assert.True(t, perm.CanWrite(UnitTypeIssues)) assert.False(t, perm.CanWrite(UnitTypeCode)) @@ -172,7 +172,7 @@ func TestRepoPermissionPublicOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, admin) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } } @@ -189,7 +189,7 @@ func TestRepoPermissionPrivateOrgRepo(t *testing.T) { perm, err := GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.False(t, perm.CanAccess(unit.Type)) + assert.False(t, perm.CanRead(unit.Type)) assert.False(t, perm.CanWrite(unit.Type)) } @@ -198,7 +198,7 @@ func TestRepoPermissionPrivateOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } @@ -206,7 +206,7 @@ func TestRepoPermissionPrivateOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, user) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.False(t, perm.CanWrite(unit.Type)) } @@ -215,7 +215,7 @@ func TestRepoPermissionPrivateOrgRepo(t *testing.T) { perm, err = GetUserRepoPermission(repo, owner) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } @@ -225,22 +225,22 @@ func TestRepoPermissionPrivateOrgRepo(t *testing.T) { assert.NoError(t, err) assert.True(t, perm.CanWrite(UnitTypeIssues)) assert.False(t, perm.CanWrite(UnitTypeCode)) - assert.False(t, perm.CanAccess(UnitTypeCode)) + assert.False(t, perm.CanRead(UnitTypeCode)) // org member team reviewer reviewer := AssertExistsAndLoadBean(t, &User{ID: 20}).(*User) perm, err = GetUserRepoPermission(repo, reviewer) assert.NoError(t, err) - assert.False(t, perm.CanAccess(UnitTypeIssues)) + assert.False(t, perm.CanRead(UnitTypeIssues)) assert.False(t, perm.CanWrite(UnitTypeCode)) - assert.True(t, perm.CanAccess(UnitTypeCode)) + assert.True(t, perm.CanRead(UnitTypeCode)) // admin admin := AssertExistsAndLoadBean(t, &User{ID: 1}).(*User) perm, err = GetUserRepoPermission(repo, admin) assert.NoError(t, err) for _, unit := range repo.Units { - assert.True(t, perm.CanAccess(unit.Type)) + assert.True(t, perm.CanRead(unit.Type)) assert.True(t, perm.CanWrite(unit.Type)) } } From 9742d6338681fbbe9678fd0868887b9875a45ce9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 18 Nov 2018 12:45:45 +0800 Subject: [PATCH 26/31] fix comment --- models/repo_permission.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/repo_permission.go b/models/repo_permission.go index eba50676c19ba..8033c21a9125f 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -57,7 +57,7 @@ func (p *Permission) CanAccessAny(mode AccessMode, unitTypes ...UnitType) bool { return false } -// CanWrite returns true if user could write to this unit +// CanRead returns true if user could read to this unit func (p *Permission) CanRead(unitType UnitType) bool { return p.CanAccess(AccessModeRead, unitType) } From 2db05dbe39f0ae7c2123be799027f02df065978c Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 18 Nov 2018 13:15:54 +0800 Subject: [PATCH 27/31] fix bug --- models/repo_permission.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/models/repo_permission.go b/models/repo_permission.go index 8033c21a9125f..ce696561830d0 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -244,9 +244,13 @@ func canBeAssigned(e Engine, user *User, repo *Repository) (bool, error) { } func hasAccess(e Engine, userID int64, repo *Repository) (bool, error) { - user, err := getUserByID(e, userID) - if err != nil { - return false, err + var user *User + var err error + if userID > 0 { + user, err = getUserByID(e, userID) + if err != nil { + return false, err + } } perm, err := getUserRepoPermission(e, repo, user) if err != nil { From 36201099e6d1b35edae0d8d91caf4d62f7cfdc7a Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 18 Nov 2018 15:40:02 +0800 Subject: [PATCH 28/31] add missing unit for test repos --- models/fixtures/repo_unit.yml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/models/fixtures/repo_unit.yml b/models/fixtures/repo_unit.yml index ca2cdb3c608bb..24d77b9997e09 100644 --- a/models/fixtures/repo_unit.yml +++ b/models/fixtures/repo_unit.yml @@ -199,4 +199,25 @@ repo_id: 16 type: 1 config: "{}" + created_unix: 1524304355 + +- + id: 30 + repo_id: 23 + type: 1 + config: "{}" + created_unix: 1524304355 + +- + id: 31 + repo_id: 27 + type: 1 + config: "{}" + created_unix: 1524304355 + +- + id: 32 + repo_id: 28 + type: 1 + config: "{}" created_unix: 1524304355 \ No newline at end of file From 861b3b24b3670d3894db5a100e885f1a18c8eb0c Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 18 Nov 2018 15:58:16 +0800 Subject: [PATCH 29/31] fix bug --- models/repo_permission.go | 27 ++++++++++++++++----------- templates/repo/activity.tmpl | 10 +++++----- templates/repo/header.tmpl | 18 +++++++++--------- templates/repo/release/list.tmpl | 4 ++-- templates/repo/settings/options.tmpl | 2 +- templates/repo/sub_menu.tmpl | 4 ++-- 6 files changed, 35 insertions(+), 30 deletions(-) diff --git a/models/repo_permission.go b/models/repo_permission.go index ce696561830d0..9dd7cc559d2c9 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -42,12 +42,12 @@ func (p *Permission) UnitAccessMode(unitType UnitType) AccessMode { return p.UnitsMode[unitType] } -// CanAccess returns true if user has read access to the unit of the repository +// CanAccess returns true if user has mode access to the unit of the repository func (p *Permission) CanAccess(mode AccessMode, unitType UnitType) bool { return p.UnitAccessMode(unitType) >= mode } -// CanAccessAny returns true if user has read access to any of the units of the repository +// CanAccessAny returns true if user has mode access to any of the units of the repository func (p *Permission) CanAccessAny(mode AccessMode, unitTypes ...UnitType) bool { for _, u := range unitTypes { if p.CanAccess(mode, u) { @@ -62,6 +62,20 @@ func (p *Permission) CanRead(unitType UnitType) bool { return p.CanAccess(AccessModeRead, unitType) } +// CanReadAny returns true if user has read access to any of the units of the repository +func (p *Permission) CanReadAny(unitTypes ...UnitType) bool { + return p.CanAccessAny(AccessModeRead, unitTypes...) +} + +// CanReadIssuesOrPulls returns true if isPull is true and user could read pull requests and +// returns true if isPull is false and user could read to issues +func (p *Permission) CanReadIssuesOrPulls(isPull bool) bool { + if isPull { + return p.CanRead(UnitTypePullRequests) + } + return p.CanRead(UnitTypeIssues) +} + // CanWrite returns true if user could write to this unit func (p *Permission) CanWrite(unitType UnitType) bool { return p.CanAccess(AccessModeWrite, unitType) @@ -76,15 +90,6 @@ func (p *Permission) CanWriteIssuesOrPulls(isPull bool) bool { return p.CanWrite(UnitTypeIssues) } -// CanReadIssuesOrPulls returns true if isPull is true and user could read pull requests and -// returns true if isPull is false and user could read to issues -func (p *Permission) CanReadIssuesOrPulls(isPull bool) bool { - if isPull { - return p.CanRead(UnitTypePullRequests) - } - return p.CanRead(UnitTypeIssues) -} - // GetUserRepoPermission returns the user permissions to the repository func GetUserRepoPermission(repo *Repository, user *User) (Permission, error) { return getUserRepoPermission(x, repo, user) diff --git a/templates/repo/activity.tmpl b/templates/repo/activity.tmpl index 29cd9be97c53d..2b8fbc6c1c872 100644 --- a/templates/repo/activity.tmpl +++ b/templates/repo/activity.tmpl @@ -23,10 +23,10 @@ </h2> <div class="ui divider"></div> - {{if (or (.Permission.CanAccess $.UnitTypeIssues) (.Permission.CanAccess $.UnitTypePullRequests))}} + {{if (or (.Permission.CanRead $.UnitTypeIssues) (.Permission.CanRead $.UnitTypePullRequests))}} <h4 class="ui top attached header">{{.i18n.Tr "repo.activity.overview"}}</h4> <div class="ui attached segment two column grid"> - {{if .Permission.CanAccess $.UnitTypePullRequests}} + {{if .Permission.CanRead $.UnitTypePullRequests}} <div class="column"> {{if gt .Activity.ActivePRCount 0}} <div class="stats-table"> @@ -41,7 +41,7 @@ {{.i18n.Tr (TrN .i18n.Lang .Activity.ActivePRCount "repo.activity.active_prs_count_1" "repo.activity.active_prs_count_n") .Activity.ActivePRCount | Safe }} </div> {{end}} - {{if .Permission.CanAccess $.UnitTypeIssues}} + {{if .Permission.CanRead $.UnitTypeIssues}} <div class="column"> {{if gt .Activity.ActiveIssueCount 0}} <div class="stats-table"> @@ -58,7 +58,7 @@ {{end}} </div> <div class="ui attached segment horizontal segments"> - {{if .Permission.CanAccess $.UnitTypePullRequests}} + {{if .Permission.CanRead $.UnitTypePullRequests}} <a href="#merged-pull-requests" class="ui attached segment text center"> <i class="text purple octicon octicon-git-pull-request"></i> <strong>{{.Activity.MergedPRCount}}</strong><br> {{.i18n.Tr (TrN .i18n.Lang .Activity.MergedPRCount "repo.activity.merged_prs_count_1" "repo.activity.merged_prs_count_n") }} @@ -68,7 +68,7 @@ {{.i18n.Tr (TrN .i18n.Lang .Activity.OpenedPRCount "repo.activity.opened_prs_count_1" "repo.activity.opened_prs_count_n") }} </a> {{end}} - {{if .Permission.CanAccess $.UnitTypeIssues}} + {{if .Permission.CanRead $.UnitTypeIssues}} <a href="#closed-issues" class="ui attached segment text center"> <i class="text red octicon octicon-issue-closed"></i> <strong>{{.Activity.ClosedIssueCount}}</strong><br> {{.i18n.Tr (TrN .i18n.Lang .Activity.ClosedIssueCount "repo.activity.closed_issues_count_1" "repo.activity.closed_issues_count_n") }} diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index 3d8abba1b8bd2..c8ef4bced71f2 100644 --- a/templates/repo/header.tmpl +++ b/templates/repo/header.tmpl @@ -30,7 +30,7 @@ {{.NumStars}} </a> </div> - {{if and (not .IsBare) ($.Permission.CanAccess $.UnitTypeCode)}} + {{if and (not .IsBare) ($.Permission.CanRead $.UnitTypeCode)}} <div class="ui compact labeled button" tabindex="0"> <a class="ui compact button {{if not $.CanSignedUserFork}}poping up{{end}}" {{if $.CanSignedUserFork}}href="{{AppSubUrl}}/repo/fork/{{.ID}}"{{else if $.IsSigned}} data-content="{{$.i18n.Tr "repo.fork_from_self"}}" data-position="top center" data-variation="tiny"{{end}}> <i class="octicon octicon-repo-forked"></i>{{$.i18n.Tr "repo.fork"}} @@ -47,43 +47,43 @@ {{if not .IsDiffCompare}} <div class="ui tabs container"> <div class="ui tabular stackable menu navbar"> - {{if .Permission.CanAccess $.UnitTypeCode}} + {{if .Permission.CanRead $.UnitTypeCode}} <a class="{{if .PageIsViewCode}}active{{end}} item" href="{{.RepoLink}}{{if (ne .BranchName .Repository.DefaultBranch)}}/src/{{.BranchNameSubURL | EscapePound}}{{end}}"> <i class="octicon octicon-code"></i> {{.i18n.Tr "repo.code"}} </a> {{end}} - {{if .Permission.CanAccess $.UnitTypeIssues}} + {{if .Permission.CanRead $.UnitTypeIssues}} <a class="{{if .PageIsIssueList}}active{{end}} item" href="{{.RepoLink}}/issues"> <i class="octicon octicon-issue-opened"></i> {{.i18n.Tr "repo.issues"}} <span class="ui {{if not .Repository.NumOpenIssues}}gray{{else}}blue{{end}} small label">{{.Repository.NumOpenIssues}}</span> </a> {{end}} - {{if .Permission.CanAccess $.UnitTypeExternalTracker}} + {{if .Permission.CanRead $.UnitTypeExternalTracker}} <a class="{{if .PageIsIssueList}}active{{end}} item" href="{{.RepoLink}}/issues" target="_blank" rel="noopener noreferrer"> <i class="octicon octicon-issue-opened"></i> {{.i18n.Tr "repo.issues"}} </span> </a> {{end}} - {{if and .Repository.CanEnablePulls (.Permission.CanAccess $.UnitTypePullRequests)}} + {{if and .Repository.CanEnablePulls (.Permission.CanRead $.UnitTypePullRequests)}} <a class="{{if .PageIsPullList}}active{{end}} item" href="{{.RepoLink}}/pulls"> <i class="octicon octicon-git-pull-request"></i> {{.i18n.Tr "repo.pulls"}} <span class="ui {{if not .Repository.NumOpenPulls}}gray{{else}}blue{{end}} small label">{{.Repository.NumOpenPulls}}</span> </a> {{end}} - {{if and (.Permission.CanAccess $.UnitTypeReleases) (not .IsBareRepo) }} + {{if and (.Permission.CanRead $.UnitTypeReleases) (not .IsBareRepo) }} <a class="{{if .PageIsReleaseList}}active{{end}} item" href="{{.RepoLink}}/releases"> <i class="octicon octicon-tag"></i> {{.i18n.Tr "repo.releases"}} <span class="ui {{if not .Repository.NumReleases}}gray{{else}}blue{{end}} small label">{{.Repository.NumReleases}}</span> </a> {{end}} - {{if or (.Permission.CanAccess $.UnitTypeWiki) (.Permission.CanAccess $.UnitTypeExternalWiki)}} - <a class="{{if .PageIsWiki}}active{{end}} item" href="{{.RepoLink}}/wiki" {{if (.Permission.CanAccess $.UnitTypeExternalWiki)}} target="_blank" rel="noopener noreferrer" {{end}}> + {{if or (.Permission.CanRead $.UnitTypeWiki) (.Permission.CanRead $.UnitTypeExternalWiki)}} + <a class="{{if .PageIsWiki}}active{{end}} item" href="{{.RepoLink}}/wiki" {{if (.Permission.CanRead $.UnitTypeExternalWiki)}} target="_blank" rel="noopener noreferrer" {{end}}> <i class="octicon octicon-book"></i> {{.i18n.Tr "repo.wiki"}} </a> {{end}} - {{if and (.Permission.CanAccessAny $.UnitTypePullRequests $.UnitTypeIssues $.UnitTypeReleases) (not .IsBareRepo)}} + {{if and (.Permission.CanReadAny $.UnitTypePullRequests $.UnitTypeIssues $.UnitTypeReleases) (not .IsBareRepo)}} <a class="{{if .PageIsActivity}}active{{end}} item" href="{{.RepoLink}}/activity"> <i class="octicon octicon-pulse"></i> {{.i18n.Tr "repo.activity"}} </a> diff --git a/templates/repo/release/list.tmpl b/templates/repo/release/list.tmpl index 80cf825495cfc..7ee58c6b107f2 100644 --- a/templates/repo/release/list.tmpl +++ b/templates/repo/release/list.tmpl @@ -41,7 +41,7 @@ <a href="{{$.RepoLink}}/src/tag/{{.TagName | EscapePound}}" rel="nofollow"><i class="tag icon"></i> {{.TagName}}</a> </h4> <div class="download"> - {{if $.Permission.CanAccess $.UnitTypeCode}} + {{if $.Permission.CanRead $.UnitTypeCode}} <a href="{{$.RepoLink}}/src/commit/{{.Sha1}}" rel="nofollow"><i class="code icon"></i> {{ShortSha .Sha1}}</a> <a href="{{$.RepoLink}}/archive/{{.TagName | EscapePound}}.zip" rel="nofollow"><i class="octicon octicon-file-zip"></i> ZIP</a> <a href="{{$.RepoLink}}/archive/{{.TagName | EscapePound}}.tar.gz"><i class="octicon octicon-file-zip"></i> TAR.GZ</a> @@ -66,7 +66,7 @@ <div class="download"> <h2>{{$.i18n.Tr "repo.release.downloads"}}</h2> <ul class="list"> - {{if $.Permission.CanAccess $.UnitTypeCode}} + {{if $.Permission.CanRead $.UnitTypeCode}} <li> <a href="{{$.RepoLink}}/archive/{{.TagName | EscapePound}}.zip" rel="nofollow"><strong><i class="octicon octicon-file-zip"></i> {{$.i18n.Tr "repo.release.source_code"}} (ZIP)</strong></a> </li> diff --git a/templates/repo/settings/options.tmpl b/templates/repo/settings/options.tmpl index b0c78ae9e02d1..7994260f6ecaa 100644 --- a/templates/repo/settings/options.tmpl +++ b/templates/repo/settings/options.tmpl @@ -294,7 +294,7 @@ </div> </div> - {{if .Permission.CanAccess $.UnitTypeWiki}} + {{if .Permission.CanRead $.UnitTypeWiki}} <div class="ui divider"></div> <div class="item"> diff --git a/templates/repo/sub_menu.tmpl b/templates/repo/sub_menu.tmpl index 4f084bca9a3d0..4ca1aa5cbb310 100644 --- a/templates/repo/sub_menu.tmpl +++ b/templates/repo/sub_menu.tmpl @@ -1,11 +1,11 @@ <div class="ui segment sub-menu"> <div class="ui two horizontal center link list"> - {{if and (.Permission.CanAccess $.UnitTypeCode) (not .IsBareRepo)}} + {{if and (.Permission.CanRead $.UnitTypeCode) (not .IsBareRepo)}} <div class="item{{if .PageIsCommits}} active{{end}}"> <a href="{{.RepoLink}}/commits{{if .IsViewBranch}}/branch{{else if .IsViewTag}}/tag{{else if .IsViewCommit}}/commit{{end}}/{{EscapePound .BranchName}}"><i class="octicon octicon-history"></i> <b>{{.CommitsCount}}</b> {{.i18n.Tr (TrN .i18n.Lang .CommitsCount "repo.commit" "repo.commits") }}</a> </div> {{end}} - {{if and (.Permission.CanAccess $.UnitTypeCode) (not .IsBareRepo) }} + {{if and (.Permission.CanRead $.UnitTypeCode) (not .IsBareRepo) }} <div class="item{{if .PageIsBranches}} active{{end}}"> <a href="{{.RepoLink}}/branches/"><i class="octicon octicon-git-branch"></i> <b>{{.BranchesCount}}</b> {{.i18n.Tr (TrN .i18n.Lang .BranchesCount "repo.branch" "repo.branches") }}</a> </div> From b677d04404f26786291351561244e14b3bcbeb99 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 18 Nov 2018 17:10:08 +0800 Subject: [PATCH 30/31] rename some functions --- models/repo.go | 6 ------ modules/lfs/server.go | 4 ++-- modules/private/internal.go | 24 ------------------------ routers/api/v1/api.go | 2 +- routers/private/internal.go | 37 +++++++++---------------------------- routers/repo/http.go | 2 +- routers/repo/issue.go | 4 ---- routers/routes/routes.go | 7 ++++--- 8 files changed, 17 insertions(+), 69 deletions(-) diff --git a/models/repo.go b/models/repo.go index 3de56079f2a5a..b86226ec829d9 100644 --- a/models/repo.go +++ b/models/repo.go @@ -607,12 +607,6 @@ func (repo *Repository) ComposeCompareURL(oldCommitID, newCommitID string) strin return fmt.Sprintf("%s/%s/compare/%s...%s", repo.MustOwner().Name, repo.Name, oldCommitID, newCommitID) } -// HasAccess returns true when user has access to this repository -/*func (repo *Repository) HasAccess(u *User) bool { - has, _ := HasAccess(u.ID, repo, AccessModeRead) - return has -}*/ - // UpdateDefaultBranch updates the default branch func (repo *Repository) UpdateDefaultBranch() error { _, err := x.ID(repo.ID).Cols("default_branch").Update(repo) diff --git a/modules/lfs/server.go b/modules/lfs/server.go index 0a61478bb9524..f0f2d4bf44026 100644 --- a/modules/lfs/server.go +++ b/modules/lfs/server.go @@ -502,7 +502,7 @@ func authenticate(ctx *context.Context, repository *models.Repository, authoriza return false } if ctx.IsSigned { - return perm.UnitAccessMode(models.UnitTypeCode) >= accessMode + return perm.CanAccess(accessMode, models.UnitTypeCode) } user, repo, opStr, err := parseToken(authorization) @@ -515,7 +515,7 @@ func authenticate(ctx *context.Context, repository *models.Repository, authoriza if err != nil { return false } - return perm.UnitAccessMode(models.UnitTypeCode) >= accessMode + return perm.CanAccess(accessMode, models.UnitTypeCode) } if repository.ID == repo.ID { if requireWrite && opStr != "upload" { diff --git a/modules/private/internal.go b/modules/private/internal.go index e8388c6b7b509..a230bc744ca36 100644 --- a/modules/private/internal.go +++ b/modules/private/internal.go @@ -73,30 +73,6 @@ func CheckUnitUser(userID, repoID int64, isAdmin bool, unitType models.UnitType) return &a, nil } -// AccessLevel returns the Access a user has to a repository. Will return NoneAccess if the -// user does not have access. -/*func AccessLevel(userID, repoID int64) (*models.AccessMode, error) { - reqURL := setting.LocalURL + fmt.Sprintf("api/internal/repositories/%d/user/%d/accesslevel", repoID, userID) - log.GitLogger.Trace("AccessLevel: %s", reqURL) - - resp, err := newInternalRequest(reqURL, "GET").Response() - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode != 200 { - return nil, fmt.Errorf("Failed to get user access level: %s", decodeJSONError(resp).Err) - } - - var a models.AccessMode - if err := json.NewDecoder(resp.Body).Decode(&a); err != nil { - return nil, err - } - - return &a, nil -}*/ - // GetRepositoryByOwnerAndName returns the repository by given ownername and reponame. func GetRepositoryByOwnerAndName(ownerName, repoName string) (*models.Repository, error) { reqURL := setting.LocalURL + fmt.Sprintf("api/internal/repo/%s/%s", ownerName, repoName) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index aced2b4ebf5b2..707a8b2dadcc3 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -509,7 +509,7 @@ func RegisterRoutes(m *macaron.Macaron) { Post(bind(api.CreateKeyOption{}), repo.CreateDeployKey) m.Combo("/:id").Get(repo.GetDeployKey). Delete(repo.DeleteDeploykey) - }, reqToken(), reqRepoWriter(models.UnitTypeCode)) + }, reqToken(), reqAdmin()) m.Group("/times", func() { m.Combo("").Get(repo.ListTrackedTimesByRepository) m.Combo("/:timetrackingusername").Get(repo.ListTrackedTimesByUser) diff --git a/routers/private/internal.go b/routers/private/internal.go index 9bd15fcfc56ec..0221b1fee86a8 100644 --- a/routers/private/internal.go +++ b/routers/private/internal.go @@ -38,27 +38,6 @@ func GetRepositoryByOwnerAndName(ctx *macaron.Context) { ctx.JSON(200, repo) } -//AccessLevel chainload to models.AccessLevel -/*func AccessLevel(ctx *macaron.Context) { - repoID := ctx.ParamsInt64(":repoid") - userID := ctx.ParamsInt64(":userid") - repo, err := models.GetRepositoryByID(repoID) - if err != nil { - ctx.JSON(500, map[string]interface{}{ - "err": err.Error(), - }) - return - } - al, err := models.AccessLevel(userID, repo) - if err != nil { - ctx.JSON(500, map[string]interface{}{ - "err": err.Error(), - }) - return - } - ctx.JSON(200, al) -}*/ - //CheckUnitUser chainload to models.CheckUnitUser func CheckUnitUser(ctx *macaron.Context) { repoID := ctx.ParamsInt64(":repoid") @@ -71,12 +50,15 @@ func CheckUnitUser(ctx *macaron.Context) { return } - user, err := models.GetUserByID(userID) - if err != nil { - ctx.JSON(500, map[string]interface{}{ - "err": err.Error(), - }) - return + var user *models.User + if userID > 0 { + user, err = models.GetUserByID(userID) + if err != nil { + ctx.JSON(500, map[string]interface{}{ + "err": err.Error(), + }) + return + } } perm, err := models.GetUserRepoPermission(repo, user) @@ -98,7 +80,6 @@ func RegisterRoutes(m *macaron.Macaron) { m.Get("/ssh/:id/user", GetUserByKeyID) m.Post("/ssh/:id/update", UpdatePublicKey) m.Post("/repositories/:repoid/keys/:keyid/update", UpdateDeployKey) - //m.Get("/repositories/:repoid/user/:userid/accesslevel", AccessLevel) m.Get("/repositories/:repoid/user/:userid/checkunituser", CheckUnitUser) m.Get("/repositories/:repoid/has-keys/:keyid", HasDeployKey) m.Post("/push/update", PushUpdate) diff --git a/routers/repo/http.go b/routers/repo/http.go index 35c03af7cfb6e..ec5fbe6c0d336 100644 --- a/routers/repo/http.go +++ b/routers/repo/http.go @@ -188,7 +188,7 @@ func HTTP(ctx *context.Context) { return } - if perm.UnitAccessMode(unitType) < accessMode { + if !perm.CanAccess(accessMode, unitType) { ctx.HandleText(http.StatusForbidden, "User permission denied") return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 0dcbf285acb11..8d95f3394086b 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -381,10 +381,6 @@ func ValidateRepoMetas(ctx *context.Context, form auth.CreateIssueForm, isPull b return nil, nil, 0 } - if !ctx.Repo.CanWrite(models.UnitTypeIssues) { - return nil, nil, 0 - } - var labelIDs []int64 hasSelected := false // Check labels. diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 8924da210490b..eb5841f593109 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -398,8 +398,9 @@ func RegisterRoutes(m *macaron.Macaron) { reqRepoReleaseWriter := context.RequireRepoWriter(models.UnitTypeReleases) reqRepoReleaseReader := context.RequireRepoReader(models.UnitTypeReleases) reqRepoWikiWriter := context.RequireRepoWriter(models.UnitTypeWiki) - reqRepoIssueWriter := context.RequireRepoWriter(models.UnitTypeIssues) + reqRepoIssueReader := context.RequireRepoReader(models.UnitTypeIssues) reqRepoPullsWriter := context.RequireRepoWriter(models.UnitTypePullRequests) + reqRepoPullsReader := context.RequireRepoReader(models.UnitTypePullRequests) reqRepoIssuesOrPullsWriter := context.RequireRepoWriterOr(models.UnitTypeIssues, models.UnitTypePullRequests) reqRepoIssuesOrPullsReader := context.RequireRepoReaderOr(models.UnitTypeIssues, models.UnitTypePullRequests) @@ -530,7 +531,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/issues", func() { m.Combo("/new").Get(context.RepoRef(), repo.NewIssue). Post(bindIgnErr(auth.CreateIssueForm{}), repo.NewIssuePost) - }, reqRepoIssueWriter) + }, reqRepoIssueReader) // FIXME: should use different URLs but mostly same logic for comments of issue and pull reuqest. // So they can apply their own enable/disable logic on routers. m.Group("/issues", func() { @@ -578,7 +579,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/delete", repo.DeleteMilestone) }, reqRepoIssuesOrPullsWriter, context.RepoRef()) - m.Combo("/compare/*", reqRepoCodeReader, repo.MustAllowPulls, repo.SetEditorconfigIfExists). + m.Combo("/compare/*", reqRepoCodeReader, reqRepoPullsReader, repo.MustAllowPulls, repo.SetEditorconfigIfExists). Get(repo.SetDiffViewStyle, repo.CompareAndPullRequest). Post(bindIgnErr(auth.CreateIssueForm{}), repo.CompareAndPullRequestPost) From 79365d8aafdb2938e15654d72fd3994c00458a63 Mon Sep 17 00:00:00 2001 From: Lunny Xiao <xiaolunwen@gmail.com> Date: Sun, 18 Nov 2018 17:29:26 +0800 Subject: [PATCH 31/31] fix routes check --- routers/api/v1/api.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 707a8b2dadcc3..47e556c881502 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -598,8 +598,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Combo("/merge").Get(repo.IsPullRequestMerged). Post(reqToken(), reqRepoWriter(models.UnitTypePullRequests), bind(auth.MergePullRequestForm{}), repo.MergePullRequest) }) - - }, mustAllowPulls, context.ReferencesGitRepo()) + }, mustAllowPulls, reqRepoReader(models.UnitTypeCode), context.ReferencesGitRepo()) m.Group("/statuses", func() { m.Combo("/:sha").Get(repo.GetCommitStatuses). Post(reqToken(), bind(api.CreateStatusOption{}), repo.NewCommitStatus)