From ddf7014b9b16e571e86c87962be9f9cbe140a93b Mon Sep 17 00:00:00 2001 From: Peter Smit Date: Fri, 13 Feb 2015 13:58:19 +0200 Subject: [PATCH 1/4] Rewrite of access migration The old migration had a few issues: - It left old column names around - It did not give the right access levels for owners and admins Also, this includes a migration that fixes the authorization of owner teams, which was previously ORG_ADMIN (instead of ORG_OWNER) --- models/migrations/migrations.go | 138 ++++++++++++++++++-------------- 1 file changed, 78 insertions(+), 60 deletions(-) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index e69b0a1f5..f6ef51320 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -51,7 +51,8 @@ type Version struct { // update _MIN_VER_DB accordingly var migrations = []Migration{ NewMigration("generate collaboration from access", accessToCollaboration), // V0 -> V1 - NewMigration("refactor access table to use id's", accessRefactor), // V1 -> V2 + NewMigration("make authorize 4 if team is owners", ownerTeamUpdate), // V1 -> V2 + NewMigration("refactor access table to use id's", accessRefactor), // V2 -> V3 } // Migrate database to current version @@ -212,90 +213,107 @@ func accessToCollaboration(x *xorm.Engine) (err error) { return sess.Commit() } +func ownerTeamUpdate(x *xorm.Engine) (err error) { + if _, err := x.Exec("UPDATE team SET authorize=4 WHERE lower_name=?", "owners"); err != nil { + return fmt.Errorf("drop table: %v", err) + } + return nil +} + func accessRefactor(x *xorm.Engine) (err error) { type ( AccessMode int Access struct { - ID int64 `xorm:"pk autoincr"` - UserName string - RepoName string - UserID int64 `xorm:"UNIQUE(s)"` - RepoID int64 `xorm:"UNIQUE(s)"` - Mode AccessMode + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"UNIQUE(s)"` + RepoID int64 `xorm:"UNIQUE(s)"` + Mode AccessMode + } + UserRepo struct { + UserID int64 + RepoID int64 } ) - var rawSQL string - switch { - case setting.UseSQLite3, setting.UsePostgreSQL: - rawSQL = "DROP INDEX IF EXISTS `UQE_access_S`" - case setting.UseMySQL: - rawSQL = "DROP INDEX `UQE_access_S` ON `access`" - } - if _, err = x.Exec(rawSQL); err != nil && - !strings.Contains(err.Error(), "check that column/key exists") { - return fmt.Errorf("drop index: %v", err) - } + // We consiously don't start a session yet as we make only reads for now, no writes - sess := x.NewSession() - defer sessionRelease(sess) - if err = sess.Begin(); err != nil { + accessMap := make(map[UserRepo]AccessMode, 50) + + results, err := x.Query("SELECT r.id as `repo_id`, r.is_private as `is_private`, r.owner_id as `owner_id`, u.type as `owner_type` FROM `repository` r LEFT JOIN user u ON r.owner_id=u.id") + if err != nil { return err } + for _, repo := range results { + repoID := com.StrTo(repo["repo_id"]).MustInt64() + isPrivate := com.StrTo(repo["is_private"]).MustInt() > 0 + ownerID := com.StrTo(repo["owner_id"]).MustInt64() + ownerIsOrganization := com.StrTo(repo["owner_type"]).MustInt() > 0 - if err = sess.Sync2(new(Access)); err != nil { - return fmt.Errorf("sync: %v", err) - } + results, err := x.Query("SELECT user_id FROM collaboration WHERE repo_id=?", repoID) + if err != nil { + return fmt.Errorf("select repos: %v", err) + } + for _, user := range results { + userID := com.StrTo(user["user_id"]).MustInt64() + accessMap[UserRepo{userID, repoID}] = 2 // WRITE ACCESS + } - accesses := make([]*Access, 0, 50) - if err = sess.Iterate(new(Access), func(idx int, bean interface{}) error { - a := bean.(*Access) + if !ownerIsOrganization { + continue + } - // Update username to user ID. - users, err := sess.Query("SELECT `id` FROM `user` WHERE lower_name=?", a.UserName) + minAccessLevel := AccessMode(0) + if !isPrivate { + minAccessLevel = 1 + } + + repoString := "$" + string(repo["repo_id"]) + "|" + + results, err = x.Query("SELECT id, authorize, repo_ids FROM team WHERE org_id=? AND authorize > ? ORDER BY authorize ASC", ownerID, int(minAccessLevel)) if err != nil { - return fmt.Errorf("query user: %v", err) - } else if len(users) < 1 { - return nil + return fmt.Errorf("select teams from org: %v", err) } - a.UserID = com.StrTo(users[0]["id"]).MustInt64() - // Update repository name(username/reponame) to repository ID. - names := strings.Split(a.RepoName, "/") - ownerName := names[0] - repoName := names[1] + for _, team := range results { + if !strings.Contains(string(team["repo_ids"]), repoString) { + continue + } + teamID := com.StrTo(team["id"]).MustInt64() + mode := AccessMode(com.StrTo(team["authorize"]).MustInt()) - // Check if user is the owner of the repository. - ownerID := a.UserID - if ownerName != a.UserName { - users, err := sess.Query("SELECT `id` FROM `user` WHERE lower_name=?", ownerName) + results, err := x.Query("SELECT uid FROM team_user WHERE team_id=?", teamID) if err != nil { - return fmt.Errorf("query owner: %v", err) - } else if len(users) < 1 { - return nil + return fmt.Errorf("select users from team: %v", err) + } + for _, user := range results { + userID := com.StrTo(user["uid"]).MustInt64() + accessMap[UserRepo{userID, repoID}] = mode } - ownerID = com.StrTo(users[0]["id"]).MustInt64() } + } - repos, err := sess.Query("SELECT `id` FROM `repository` WHERE owner_id=? AND lower_name=?", ownerID, repoName) - if err != nil { - return fmt.Errorf("query repository: %v", err) - } else if len(repos) < 1 { - return nil - } - a.RepoID = com.StrTo(repos[0]["id"]).MustInt64() + // Drop table can't be in a session (at least not in sqlite) + if _, err = x.Exec("DROP TABLE access"); err != nil { + return fmt.Errorf("drop table: %v", err) + } - accesses = append(accesses, a) - return nil - }); err != nil { - return fmt.Errorf("iterate: %v", err) + // Now we start writing so we make a session + sess := x.NewSession() + defer sessionRelease(sess) + if err = sess.Begin(); err != nil { + return err } - for i := range accesses { - if _, err = sess.Id(accesses[i].ID).Update(accesses[i]); err != nil { - return fmt.Errorf("update: %v", err) - } + if err = sess.Sync2(new(Access)); err != nil { + return fmt.Errorf("sync: %v", err) + } + + accesses := make([]*Access, 0, len(accessMap)) + for ur, mode := range accessMap { + accesses = append(accesses, &Access{UserID: ur.UserID, RepoID: ur.RepoID, Mode: mode}) } + _, err = sess.Insert(accesses) + return sess.Commit() } From 0009a1d2b16baf1bcc9b69078853e9475662ab7f Mon Sep 17 00:00:00 2001 From: Peter Smit Date: Fri, 13 Feb 2015 23:12:33 +0200 Subject: [PATCH 2/4] Fix access for team mode update --- models/org.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/models/org.go b/models/org.go index 42b14bf07..115a062b4 100644 --- a/models/org.go +++ b/models/org.go @@ -666,6 +666,11 @@ func UpdateTeam(t *Team, authChanged bool) (err error) { return err } + t.LowerName = strings.ToLower(t.Name) + if _, err = sess.Id(t.Id).AllCols().Update(t); err != nil { + return err + } + // Update access for team members if needed. if authChanged { if err = t.getRepositories(sess); err != nil { @@ -679,10 +684,6 @@ func UpdateTeam(t *Team, authChanged bool) (err error) { } } - t.LowerName = strings.ToLower(t.Name) - if _, err = sess.Id(t.Id).AllCols().Update(t); err != nil { - return err - } return sess.Commit() } From f9454cc32c94780eb4c49753fc0ccd9b60b1deb7 Mon Sep 17 00:00:00 2001 From: Peter Smit Date: Mon, 16 Feb 2015 12:00:06 +0200 Subject: [PATCH 3/4] Make sure that a mirror can't be written to by http or ssh --- cmd/serve.go | 5 +++++ routers/repo/http.go | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/cmd/serve.go b/cmd/serve.go index e8e5c186c..9e34b95c5 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -164,6 +164,11 @@ func runServ(c *cli.Context) { println("You have no right to write this repository") log.GitLogger.Fatal(2, "User %s has no right to write repository %s", user.Name, repoPath) } + + if repo.IsMirror { + println("You can't write to a mirror repository") + log.GitLogger.Fatal(2, "User %s tried to write to a mirror repository %s", user.Name, repoPath) + } case isRead: if !repo.IsPrivate { break diff --git a/routers/repo/http.go b/routers/repo/http.go index 034b5a7b5..d47d73ef0 100644 --- a/routers/repo/http.go +++ b/routers/repo/http.go @@ -158,6 +158,11 @@ func Http(ctx *middleware.Context) { return } } + + if !isPull && repo.IsMirror { + ctx.Handle(401, "can't push to mirror", nil) + return + } } } From ed89b39984a9191380263eaf357c3a9c71770674 Mon Sep 17 00:00:00 2001 From: Peter Smit Date: Mon, 16 Feb 2015 12:51:56 +0200 Subject: [PATCH 4/4] Updating context and fixing permission issues The boolean flags in the repo context have been replaced with mode and two methods Also, the permissions have been brought more in line with https://help.github.com/articles/permission-levels-for-an-organization-repository/ , Admin Team members are able to change settings of their repositories. --- cmd/web.go | 4 +-- modules/middleware/context.go | 55 ++++++++++++++++++++--------------- modules/middleware/repo.go | 47 +++++++++++------------------- routers/api/v1/repo_file.go | 2 +- routers/repo/issue.go | 14 ++++----- routers/repo/release.go | 10 +++---- routers/repo/repo.go | 2 +- templates/repo/header.tmpl | 2 +- templates/repo/sidebar.tmpl | 2 +- templates/repo/toolbar.tmpl | 2 +- 10 files changed, 68 insertions(+), 72 deletions(-) diff --git a/cmd/web.go b/cmd/web.go index 3284acb9d..8b3b03c45 100644 --- a/cmd/web.go +++ b/cmd/web.go @@ -319,7 +319,7 @@ func runWeb(ctx *cli.Context) { m.Get("/template/*", dev.TemplatePreview) } - reqTrueOwner := middleware.RequireTrueOwner() + reqAdmin := middleware.RequireAdmin() // Organization. m.Group("/org", func() { @@ -394,7 +394,7 @@ func runWeb(ctx *cli.Context) { m.Post("/:name", repo.GitHooksEditPost) }, middleware.GitHookService()) }) - }, reqSignIn, middleware.RepoAssignment(true), reqTrueOwner) + }, reqSignIn, middleware.RepoAssignment(true), reqAdmin) m.Group("/:username/:reponame", func() { m.Get("/action/:action", repo.Action) diff --git a/modules/middleware/context.go b/modules/middleware/context.go index 28be3a302..a26610969 100644 --- a/modules/middleware/context.go +++ b/modules/middleware/context.go @@ -38,29 +38,7 @@ type Context struct { IsSigned bool IsBasicAuth bool - Repo struct { - IsOwner bool - IsTrueOwner bool - IsWatching bool - IsBranch bool - IsTag bool - IsCommit bool - IsAdmin bool // Current user is admin level. - HasAccess bool - Repository *models.Repository - Owner *models.User - Commit *git.Commit - Tag *git.Tag - GitRepo *git.Repository - BranchName string - TagName string - TreeName string - CommitId string - RepoLink string - CloneLink models.CloneLink - CommitsCount int - Mirror *models.Mirror - } + Repo RepoContext Org struct { IsOwner bool @@ -73,6 +51,37 @@ type Context struct { } } +type RepoContext struct { + AccessMode models.AccessMode + IsWatching bool + IsBranch bool + IsTag bool + IsCommit bool + Repository *models.Repository + Owner *models.User + Commit *git.Commit + Tag *git.Tag + GitRepo *git.Repository + BranchName string + TagName string + TreeName string + CommitId string + RepoLink string + CloneLink models.CloneLink + CommitsCount int + Mirror *models.Mirror +} + +// Return if the current user has write access for this repository +func (r RepoContext) IsOwner() bool { + return r.AccessMode >= models.ACCESS_MODE_WRITE +} + +// Return if the current user has read access for this repository +func (r RepoContext) HasAccess() bool { + return r.AccessMode >= models.ACCESS_MODE_READ +} + // HasError returns true if error occurs in form validation. func (ctx *Context) HasApiError() bool { hasErr, ok := ctx.Data["HasError"] diff --git a/modules/middleware/repo.go b/modules/middleware/repo.go index bd298819d..5c863dc01 100644 --- a/modules/middleware/repo.go +++ b/modules/middleware/repo.go @@ -58,24 +58,19 @@ func ApiRepoAssignment() macaron.Handler { return } - if ctx.IsSigned { - mode, err := models.AccessLevel(ctx.User, repo) - if err != nil { - ctx.JSON(500, &base.ApiJsonErr{"AccessLevel: " + err.Error(), base.DOC_URL}) - return - } - - ctx.Repo.IsOwner = mode >= models.ACCESS_MODE_WRITE - ctx.Repo.IsAdmin = mode >= models.ACCESS_MODE_READ - ctx.Repo.IsTrueOwner = mode >= models.ACCESS_MODE_OWNER + mode, err := models.AccessLevel(ctx.User, repo) + if err != nil { + ctx.JSON(500, &base.ApiJsonErr{"AccessLevel: " + err.Error(), base.DOC_URL}) + return } + ctx.Repo.AccessMode = mode + // Check access. - if repo.IsPrivate && !ctx.Repo.IsOwner { + if ctx.Repo.AccessMode == models.ACCESS_MODE_NONE { ctx.Error(404) return } - ctx.Repo.HasAccess = true ctx.Repo.Repository = repo } @@ -239,26 +234,18 @@ func RepoAssignment(redirect bool, args ...bool) macaron.Handler { return } - if ctx.IsSigned { - mode, err := models.AccessLevel(ctx.User, repo) - if err != nil { - ctx.Handle(500, "AccessLevel", err) - return - } - ctx.Repo.IsOwner = mode >= models.ACCESS_MODE_WRITE - ctx.Repo.IsAdmin = mode >= models.ACCESS_MODE_READ - ctx.Repo.IsTrueOwner = mode >= models.ACCESS_MODE_OWNER - if !ctx.Repo.IsTrueOwner && ctx.Repo.Owner.IsOrganization() { - ctx.Repo.IsTrueOwner = ctx.Repo.Owner.IsOwnedBy(ctx.User.Id) - } + mode, err := models.AccessLevel(ctx.User, repo) + if err != nil { + ctx.Handle(500, "AccessLevel", err) + return } + ctx.Repo.AccessMode = mode // Check access. - if repo.IsPrivate && !ctx.Repo.IsOwner { + if ctx.Repo.AccessMode == models.ACCESS_MODE_NONE { ctx.Handle(404, "no access right", err) return } - ctx.Repo.HasAccess = true ctx.Data["HasAccess"] = true @@ -306,8 +293,8 @@ func RepoAssignment(redirect bool, args ...bool) macaron.Handler { ctx.Data["Title"] = u.Name + "/" + repo.Name ctx.Data["Repository"] = repo ctx.Data["Owner"] = ctx.Repo.Repository.Owner - ctx.Data["IsRepositoryOwner"] = ctx.Repo.IsOwner - ctx.Data["IsRepositoryTrueOwner"] = ctx.Repo.IsTrueOwner + ctx.Data["IsRepositoryOwner"] = ctx.Repo.AccessMode >= models.ACCESS_MODE_WRITE + ctx.Data["IsRepositoryAdmin"] = ctx.Repo.AccessMode >= models.ACCESS_MODE_ADMIN ctx.Data["DisableSSH"] = setting.DisableSSH ctx.Repo.CloneLink, err = repo.CloneLink() @@ -362,9 +349,9 @@ func RepoAssignment(redirect bool, args ...bool) macaron.Handler { } } -func RequireTrueOwner() macaron.Handler { +func RequireAdmin() macaron.Handler { return func(ctx *Context) { - if !ctx.Repo.IsTrueOwner && !ctx.Repo.IsAdmin { + if ctx.Repo.AccessMode < models.ACCESS_MODE_ADMIN { if !ctx.IsSigned { ctx.SetCookie("redirect_to", "/"+url.QueryEscape(setting.AppSubUrl+ctx.Req.RequestURI), 0, setting.AppSubUrl) ctx.Redirect(setting.AppSubUrl + "/user/login") diff --git a/routers/api/v1/repo_file.go b/routers/api/v1/repo_file.go index a049904f9..73f97b2ca 100644 --- a/routers/api/v1/repo_file.go +++ b/routers/api/v1/repo_file.go @@ -12,7 +12,7 @@ import ( ) func GetRepoRawFile(ctx *middleware.Context) { - if ctx.Repo.Repository.IsPrivate && !ctx.Repo.HasAccess { + if !ctx.Repo.HasAccess() { ctx.Error(404) return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index bf39d9aba..40e933897 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -230,7 +230,7 @@ func CreateIssuePost(ctx *middleware.Context, form auth.CreateIssueForm) { } // Only collaborators can assign. - if !ctx.Repo.IsOwner { + if !ctx.Repo.IsOwner() { form.AssigneeId = 0 } issue := &models.Issue{ @@ -434,7 +434,7 @@ func ViewIssue(ctx *middleware.Context) { ctx.Data["Title"] = issue.Name ctx.Data["Issue"] = issue ctx.Data["Comments"] = comments - ctx.Data["IsIssueOwner"] = ctx.Repo.IsOwner || (ctx.IsSigned && issue.PosterId == ctx.User.Id) + ctx.Data["IsIssueOwner"] = ctx.Repo.IsOwner() || (ctx.IsSigned && issue.PosterId == ctx.User.Id) ctx.Data["IsRepoToolbarIssues"] = true ctx.Data["IsRepoToolbarIssuesList"] = false ctx.HTML(200, ISSUE_VIEW) @@ -457,7 +457,7 @@ func UpdateIssue(ctx *middleware.Context, form auth.CreateIssueForm) { return } - if ctx.User.Id != issue.PosterId && !ctx.Repo.IsOwner { + if ctx.User.Id != issue.PosterId && !ctx.Repo.IsOwner() { ctx.Error(403) return } @@ -484,7 +484,7 @@ func UpdateIssue(ctx *middleware.Context, form auth.CreateIssueForm) { } func UpdateIssueLabel(ctx *middleware.Context) { - if !ctx.Repo.IsOwner { + if !ctx.Repo.IsOwner() { ctx.Error(403) return } @@ -560,7 +560,7 @@ func UpdateIssueLabel(ctx *middleware.Context) { } func UpdateIssueMilestone(ctx *middleware.Context) { - if !ctx.Repo.IsOwner { + if !ctx.Repo.IsOwner() { ctx.Error(403) return } @@ -606,7 +606,7 @@ func UpdateIssueMilestone(ctx *middleware.Context) { } func UpdateAssignee(ctx *middleware.Context) { - if !ctx.Repo.IsOwner { + if !ctx.Repo.IsOwner() { ctx.Error(403) return } @@ -752,7 +752,7 @@ func Comment(ctx *middleware.Context) { // Check if issue owner changes the status of issue. var newStatus string - if ctx.Repo.IsOwner || issue.PosterId == ctx.User.Id { + if ctx.Repo.IsOwner() || issue.PosterId == ctx.User.Id { newStatus = ctx.Query("change_status") } if len(newStatus) > 0 { diff --git a/routers/repo/release.go b/routers/repo/release.go index 591810cc5..52d78b196 100644 --- a/routers/repo/release.go +++ b/routers/repo/release.go @@ -41,7 +41,7 @@ func Releases(ctx *middleware.Context) { tags := make([]*models.Release, len(rawTags)) for i, rawTag := range rawTags { for j, rel := range rels { - if rel == nil || (rel.IsDraft && !ctx.Repo.IsOwner) { + if rel == nil || (rel.IsDraft && !ctx.Repo.IsOwner()) { continue } if rel.TagName == rawTag { @@ -140,7 +140,7 @@ func Releases(ctx *middleware.Context) { } func NewRelease(ctx *middleware.Context) { - if !ctx.Repo.IsOwner { + if !ctx.Repo.IsOwner() { ctx.Handle(403, "release.ReleasesNew", nil) return } @@ -153,7 +153,7 @@ func NewRelease(ctx *middleware.Context) { } func NewReleasePost(ctx *middleware.Context, form auth.NewReleaseForm) { - if !ctx.Repo.IsOwner { + if !ctx.Repo.IsOwner() { ctx.Handle(403, "release.ReleasesNew", nil) return } @@ -211,7 +211,7 @@ func NewReleasePost(ctx *middleware.Context, form auth.NewReleaseForm) { } func EditRelease(ctx *middleware.Context) { - if !ctx.Repo.IsOwner { + if !ctx.Repo.IsOwner() { ctx.Handle(403, "release.ReleasesEdit", nil) return } @@ -234,7 +234,7 @@ func EditRelease(ctx *middleware.Context) { } func EditReleasePost(ctx *middleware.Context, form auth.EditReleaseForm) { - if !ctx.Repo.IsOwner { + if !ctx.Repo.IsOwner() { ctx.Handle(403, "release.EditReleasePost", nil) return } diff --git a/routers/repo/repo.go b/routers/repo/repo.go index 48f7b09bc..005372003 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -343,7 +343,7 @@ func Action(ctx *middleware.Context) { case "unstar": err = models.StarRepo(ctx.User.Id, ctx.Repo.Repository.Id, false) case "desc": - if !ctx.Repo.IsOwner { + if !ctx.Repo.IsOwner() { ctx.Error(404) return } diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index a0b927be6..21f9cea88 100644 --- a/templates/repo/header.tmpl +++ b/templates/repo/header.tmpl @@ -49,7 +49,7 @@
  • - +
    • -->{{end}}{{if .IsRepositoryTrueOwner}} + -->{{end}}{{if .IsRepositoryAdmin}}
  • Settings
    • {{end}}