From 515cdaa85d6087d91a61ebe74fae39e0c4bdf1c4 Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Wed, 20 Dec 2017 23:43:26 -0800 Subject: [PATCH] Fix ignored errors when checking if organization, team member (#3177) --- models/org.go | 39 +++++++------- models/org_team.go | 31 +++++++---- models/org_team_test.go | 19 ++++--- models/org_test.go | 101 +++++++++++++++++++++++------------ models/repo.go | 14 +++-- models/user.go | 14 ++++- modules/context/org.go | 13 +++-- routers/api/v1/api.go | 10 +++- routers/api/v1/org/member.go | 41 ++++++++++---- routers/api/v1/org/team.go | 6 ++- routers/api/v1/repo/fork.go | 6 ++- routers/api/v1/repo/repo.go | 39 +++++++++++--- routers/repo/issue.go | 32 ++++++++--- routers/repo/pull.go | 6 ++- routers/repo/repo.go | 12 ++++- routers/repo/setting.go | 42 ++++----------- 16 files changed, 281 insertions(+), 144 deletions(-) diff --git a/models/org.go b/models/org.go index b349e4c170..a28a8e28e1 100644 --- a/models/org.go +++ b/models/org.go @@ -21,13 +21,13 @@ var ( ) // IsOwnedBy returns true if given user is in the owner team. -func (org *User) IsOwnedBy(uid int64) bool { +func (org *User) IsOwnedBy(uid int64) (bool, error) { return IsOrganizationOwner(org.ID, uid) } // IsOrgMember returns true if given user is member of organization. -func (org *User) IsOrgMember(uid int64) bool { - return org.IsOrganization() && IsOrganizationMember(org.ID, uid) +func (org *User) IsOrgMember(uid int64) (bool, error) { + return IsOrganizationMember(org.ID, uid) } func (org *User) getTeam(e Engine, name string) (*Team, error) { @@ -285,32 +285,32 @@ type OrgUser struct { } // IsOrganizationOwner returns true if given user is in the owner team. -func IsOrganizationOwner(orgID, uid int64) bool { - has, _ := x. +func IsOrganizationOwner(orgID, uid int64) (bool, error) { + return x. Where("is_owner=?", true). And("uid=?", uid). And("org_id=?", orgID). - Get(new(OrgUser)) - return has + Table("org_user"). + Exist() } // IsOrganizationMember returns true if given user is member of organization. -func IsOrganizationMember(orgID, uid int64) bool { - has, _ := x. +func IsOrganizationMember(orgID, uid int64) (bool, error) { + return x. Where("uid=?", uid). And("org_id=?", orgID). - Get(new(OrgUser)) - return has + Table("org_user"). + Exist() } // IsPublicMembership returns true if given user public his/her membership. -func IsPublicMembership(orgID, uid int64) bool { - has, _ := x. +func IsPublicMembership(orgID, uid int64) (bool, error) { + return x. Where("uid=?", uid). And("org_id=?", orgID). And("is_public=?", true). - Get(new(OrgUser)) - return has + Table("org_user"). + Exist() } func getOrgsByUserID(sess *xorm.Session, userID int64, showAll bool) ([]*User, error) { @@ -401,8 +401,9 @@ func ChangeOrgUserStatus(orgID, uid int64, public bool) error { // AddOrgUser adds new user to given organization. func AddOrgUser(orgID, uid int64) error { - if IsOrganizationMember(orgID, uid) { - return nil + isAlreadyMember, err := IsOrganizationMember(orgID, uid) + if err != nil || isAlreadyMember { + return err } sess := x.NewSession() @@ -447,7 +448,9 @@ func RemoveOrgUser(orgID, userID int64) error { } // Check if the user to delete is the last member in owner team. - if IsOrganizationOwner(orgID, userID) { + if isOwner, err := IsOrganizationOwner(orgID, userID); err != nil { + return err + } else if isOwner { t, err := org.GetOwnerTeam() if err != nil { return err diff --git a/models/org_team.go b/models/org_team.go index dcbf073837..1e3bc27071 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -8,6 +8,8 @@ import ( "errors" "fmt" "strings" + + "code.gitea.io/gitea/modules/log" ) const ownerTeamName = "Owners" @@ -47,7 +49,12 @@ func (t *Team) IsOwnerTeam() bool { // IsMember returns true if given user is a member of team. func (t *Team) IsMember(userID int64) bool { - return IsTeamMember(t.OrgID, t.ID, userID) + isMember, err := IsTeamMember(t.OrgID, t.ID, userID) + if err != nil { + log.Error(4, "IsMember: %v", err) + return false + } + return isMember } func (t *Team) getRepositories(e Engine) error { @@ -413,17 +420,17 @@ type TeamUser struct { UID int64 `xorm:"UNIQUE(s)"` } -func isTeamMember(e Engine, orgID, teamID, userID int64) bool { - has, _ := e. +func isTeamMember(e Engine, orgID, teamID, userID int64) (bool, error) { + return e. Where("org_id=?", orgID). And("team_id=?", teamID). And("uid=?", userID). - Get(new(TeamUser)) - return has + Table("team_user"). + Exist() } // IsTeamMember returns true if given user is a member of team. -func IsTeamMember(orgID, teamID, userID int64) bool { +func IsTeamMember(orgID, teamID, userID int64) (bool, error) { return isTeamMember(x, orgID, teamID, userID) } @@ -471,8 +478,9 @@ func GetUserTeams(orgID, userID int64) ([]*Team, error) { // AddTeamMember adds new membership of given team to given organization, // the user will have membership to given organization automatically when needed. func AddTeamMember(team *Team, userID int64) error { - if IsTeamMember(team.OrgID, team.ID, userID) { - return nil + isAlreadyMember, err := IsTeamMember(team.OrgID, team.ID, userID) + if err != nil || isAlreadyMember { + return err } if err := AddOrgUser(team.OrgID, userID); err != nil { @@ -529,8 +537,9 @@ func AddTeamMember(team *Team, userID int64) error { } func removeTeamMember(e Engine, team *Team, userID int64) error { - if !isTeamMember(e, team.OrgID, team.ID, userID) { - return nil + isMember, err := isTeamMember(e, team.OrgID, team.ID, userID) + if err != nil || !isMember { + return err } // Check if the user to delete is the last member in owner team. @@ -566,7 +575,7 @@ func removeTeamMember(e Engine, team *Team, userID int64) error { // This must exist. ou := new(OrgUser) - _, err := e. + _, err = e. Where("uid = ?", userID). And("org_id = ?", team.OrgID). Get(ou) diff --git a/models/org_team_test.go b/models/org_team_test.go index 9afd733d81..05429c7cc2 100644 --- a/models/org_team_test.go +++ b/models/org_team_test.go @@ -250,16 +250,21 @@ func TestDeleteTeam(t *testing.T) { func TestIsTeamMember(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) + test := func(orgID, teamID, userID int64, expected bool) { + isMember, err := IsTeamMember(orgID, teamID, userID) + assert.NoError(t, err) + assert.Equal(t, expected, isMember) + } - assert.True(t, IsTeamMember(3, 1, 2)) - assert.False(t, IsTeamMember(3, 1, 4)) - assert.False(t, IsTeamMember(3, 1, NonexistentID)) + test(3, 1, 2, true) + test(3, 1, 4, false) + test(3, 1, NonexistentID, false) - assert.True(t, IsTeamMember(3, 2, 2)) - assert.True(t, IsTeamMember(3, 2, 4)) + test(3, 2, 2, true) + test(3, 2, 4, true) - assert.False(t, IsTeamMember(3, NonexistentID, NonexistentID)) - assert.False(t, IsTeamMember(NonexistentID, NonexistentID, NonexistentID)) + test(3, NonexistentID, NonexistentID, false) + test(NonexistentID, NonexistentID, NonexistentID, false) } func TestGetTeamMembers(t *testing.T) { diff --git a/models/org_test.go b/models/org_test.go index 8f59af0744..aef313d05e 100644 --- a/models/org_test.go +++ b/models/org_test.go @@ -12,28 +12,44 @@ import ( func TestUser_IsOwnedBy(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - org := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User) - assert.True(t, org.IsOwnedBy(2)) - assert.False(t, org.IsOwnedBy(1)) - assert.False(t, org.IsOwnedBy(3)) - assert.False(t, org.IsOwnedBy(4)) - - nonOrg := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) - assert.False(t, nonOrg.IsOwnedBy(2)) - assert.False(t, nonOrg.IsOwnedBy(3)) + for _, testCase := range []struct { + OrgID int64 + UserID int64 + ExpectedOwner bool + }{ + {3, 2, true}, + {3, 1, false}, + {3, 3, false}, + {3, 4, false}, + {2, 2, false}, // user2 is not an organization + {2, 3, false}, + } { + org := AssertExistsAndLoadBean(t, &User{ID: testCase.OrgID}).(*User) + isOwner, err := org.IsOwnedBy(testCase.UserID) + assert.NoError(t, err) + assert.Equal(t, testCase.ExpectedOwner, isOwner) + } } func TestUser_IsOrgMember(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - org := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User) - assert.True(t, org.IsOrgMember(2)) - assert.True(t, org.IsOrgMember(4)) - assert.False(t, org.IsOrgMember(1)) - assert.False(t, org.IsOrgMember(3)) - - nonOrg := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) - assert.False(t, nonOrg.IsOrgMember(2)) - assert.False(t, nonOrg.IsOrgMember(3)) + for _, testCase := range []struct { + OrgID int64 + UserID int64 + ExpectedMember bool + }{ + {3, 2, true}, + {3, 4, true}, + {3, 1, false}, + {3, 3, false}, + {2, 2, false}, // user2 is not an organization + {2, 3, false}, + } { + org := AssertExistsAndLoadBean(t, &User{ID: testCase.OrgID}).(*User) + isMember, err := org.IsOrgMember(testCase.UserID) + assert.NoError(t, err) + assert.Equal(t, testCase.ExpectedMember, isMember) + } } func TestUser_GetTeam(t *testing.T) { @@ -257,31 +273,46 @@ func TestDeleteOrganization(t *testing.T) { func TestIsOrganizationOwner(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - assert.True(t, IsOrganizationOwner(3, 2)) - assert.False(t, IsOrganizationOwner(3, 3)) - assert.True(t, IsOrganizationOwner(6, 5)) - assert.False(t, IsOrganizationOwner(6, 4)) - assert.False(t, IsOrganizationOwner(NonexistentID, NonexistentID)) + test := func(orgID, userID int64, expected bool) { + isOwner, err := IsOrganizationOwner(orgID, userID) + assert.NoError(t, err) + assert.EqualValues(t, expected, isOwner) + } + test(3, 2, true) + test(3, 3, false) + test(6, 5, true) + test(6, 4, false) + test(NonexistentID, NonexistentID, false) } func TestIsOrganizationMember(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - assert.True(t, IsOrganizationMember(3, 2)) - assert.False(t, IsOrganizationMember(3, 3)) - assert.True(t, IsOrganizationMember(3, 4)) - assert.True(t, IsOrganizationMember(6, 5)) - assert.False(t, IsOrganizationMember(6, 4)) - assert.False(t, IsOrganizationMember(NonexistentID, NonexistentID)) + test := func(orgID, userID int64, expected bool) { + isMember, err := IsOrganizationMember(orgID, userID) + assert.NoError(t, err) + assert.EqualValues(t, expected, isMember) + } + test(3, 2, true) + test(3, 3, false) + test(3, 4, true) + test(6, 5, true) + test(6, 4, false) + test(NonexistentID, NonexistentID, false) } func TestIsPublicMembership(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - assert.True(t, IsPublicMembership(3, 2)) - assert.False(t, IsPublicMembership(3, 3)) - assert.False(t, IsPublicMembership(3, 4)) - assert.True(t, IsPublicMembership(6, 5)) - assert.False(t, IsPublicMembership(6, 4)) - assert.False(t, IsPublicMembership(NonexistentID, NonexistentID)) + test := func(orgID, userID int64, expected bool) { + isMember, err := IsPublicMembership(orgID, userID) + assert.NoError(t, err) + assert.EqualValues(t, expected, isMember) + } + test(3, 2, true) + test(3, 3, false) + test(3, 4, false) + test(6, 5, true) + test(6, 4, false) + test(NonexistentID, NonexistentID, false) } func TestGetOrgsByUserID(t *testing.T) { diff --git a/models/repo.go b/models/repo.go index 40495e4399..7c538525f2 100644 --- a/models/repo.go +++ b/models/repo.go @@ -1493,12 +1493,18 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error // Dummy object. collaboration := &Collaboration{RepoID: repo.ID} for _, c := range collaborators { - collaboration.UserID = c.ID - if c.ID == newOwner.ID || newOwner.IsOrgMember(c.ID) { - if _, err = sess.Delete(collaboration); err != nil { - return fmt.Errorf("remove collaborator '%d': %v", c.ID, err) + if c.ID != newOwner.ID { + isMember, err := newOwner.IsOrgMember(c.ID) + if err != nil { + return fmt.Errorf("IsOrgMember: %v", err) + } else if !isMember { + continue } } + collaboration.UserID = c.ID + if _, err = sess.Delete(collaboration); err != nil { + return fmt.Errorf("remove collaborator '%d': %v", c.ID, err) + } } // Remove old team-repository relations. diff --git a/models/user.go b/models/user.go index fa5dc73deb..3839e14590 100644 --- a/models/user.go +++ b/models/user.go @@ -487,12 +487,22 @@ func (u *User) IsOrganization() bool { // IsUserOrgOwner returns true if user is in the owner team of given organization. func (u *User) IsUserOrgOwner(orgID int64) bool { - return IsOrganizationOwner(orgID, u.ID) + isOwner, err := IsOrganizationOwner(orgID, u.ID) + if err != nil { + log.Error(4, "IsOrganizationOwner: %v", err) + return false + } + return isOwner } // IsPublicMember returns true if user public his/her membership in given organization. func (u *User) IsPublicMember(orgID int64) bool { - return IsPublicMembership(orgID, u.ID) + isMember, err := IsPublicMembership(orgID, u.ID) + if err != nil { + log.Error(4, "IsPublicMembership: %v", err) + return false + } + return isMember } func (u *User) getOrganizationCount(e Engine) (int64, error) { diff --git a/modules/context/org.go b/modules/context/org.go index cfe9a26220..29cc67dcc8 100644 --- a/modules/context/org.go +++ b/modules/context/org.go @@ -73,14 +73,21 @@ func HandleOrgAssignment(ctx *Context, args ...bool) { ctx.Org.IsTeamMember = true ctx.Org.IsTeamAdmin = true } else if ctx.IsSigned { - ctx.Org.IsOwner = org.IsOwnedBy(ctx.User.ID) + ctx.Org.IsOwner, err = org.IsOwnedBy(ctx.User.ID) + if err != nil { + ctx.Handle(500, "IsOwnedBy", err) + return + } + if ctx.Org.IsOwner { ctx.Org.IsMember = true ctx.Org.IsTeamMember = true ctx.Org.IsTeamAdmin = true } else { - if org.IsOrgMember(ctx.User.ID) { - ctx.Org.IsMember = true + ctx.Org.IsMember, err = org.IsOrgMember(ctx.User.ID) + if err != nil { + ctx.Handle(500, "IsOrgMember", err) + return } } } else { diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index f6ed844d4a..588a763616 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -177,7 +177,10 @@ func reqOrgMembership() macaron.Handler { return } - if !models.IsOrganizationMember(orgID, ctx.User.ID) { + if isMember, err := models.IsOrganizationMember(orgID, ctx.User.ID); err != nil { + ctx.Error(500, "IsOrganizationMember", err) + return + } else if !isMember { if ctx.Org.Organization != nil { ctx.Error(403, "", "Must be an organization member") } else { @@ -200,7 +203,10 @@ func reqOrgOwnership() macaron.Handler { return } - if !models.IsOrganizationOwner(orgID, ctx.User.ID) { + isOwner, err := models.IsOrganizationOwner(orgID, ctx.User.ID) + if err != nil { + ctx.Error(500, "IsOrganizationOwner", err) + } else if !isOwner { if ctx.Org.Organization != nil { ctx.Error(403, "", "Must be an organization owner") } else { diff --git a/routers/api/v1/org/member.go b/routers/api/v1/org/member.go index 7cae7c19fa..0cc5317802 100644 --- a/routers/api/v1/org/member.go +++ b/routers/api/v1/org/member.go @@ -67,7 +67,15 @@ func ListMembers(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/UserList" - publicOnly := ctx.User == nil || !ctx.Org.Organization.IsOrgMember(ctx.User.ID) + publicOnly := true + if ctx.User != nil { + isMember, err := ctx.Org.Organization.IsOrgMember(ctx.User.ID) + if err != nil { + ctx.Error(500, "IsOrgMember", err) + return + } + publicOnly = !isMember + } listMembers(ctx, publicOnly) } @@ -119,19 +127,30 @@ func IsMember(ctx *context.APIContext) { if ctx.Written() { return } - if ctx.User != nil && ctx.Org.Organization.IsOrgMember(ctx.User.ID) { - if ctx.Org.Organization.IsOrgMember(userToCheck.ID) { - ctx.Status(204) - } else { + if ctx.User != nil { + userIsMember, err := ctx.Org.Organization.IsOrgMember(ctx.User.ID) + if err != nil { + ctx.Error(500, "IsOrgMember", err) + return + } else if userIsMember { + userToCheckIsMember, err := ctx.Org.Organization.IsOrgMember(ctx.User.ID) + if err != nil { + ctx.Error(500, "IsOrgMember", err) + } else if userToCheckIsMember { + ctx.Status(204) + } else { + ctx.Status(404) + } + return + } else if ctx.User.ID == userToCheck.ID { ctx.Status(404) + return } - } else if ctx.User != nil && ctx.User.ID == userToCheck.ID { - ctx.Status(404) - } else { - redirectURL := fmt.Sprintf("%sapi/v1/orgs/%s/public_members/%s", - setting.AppURL, ctx.Org.Organization.Name, userToCheck.Name) - ctx.Redirect(redirectURL, 302) } + + redirectURL := fmt.Sprintf("%sapi/v1/orgs/%s/public_members/%s", + setting.AppURL, ctx.Org.Organization.Name, userToCheck.Name) + ctx.Redirect(redirectURL, 302) } // IsPublicMember check if a user is a public member of an organization diff --git a/routers/api/v1/org/team.go b/routers/api/v1/org/team.go index eead7dd8fd..b999d62aa1 100644 --- a/routers/api/v1/org/team.go +++ b/routers/api/v1/org/team.go @@ -176,7 +176,11 @@ func GetTeamMembers(ctx *context.APIContext) { // responses: // "200": // "$ref": "#/responses/UserList" - if !models.IsOrganizationMember(ctx.Org.Team.OrgID, ctx.User.ID) { + isMember, err := models.IsOrganizationMember(ctx.Org.Team.OrgID, ctx.User.ID) + if err != nil { + ctx.Error(500, "IsOrganizationMember", err) + return + } else if !isMember { ctx.Status(404) return } diff --git a/routers/api/v1/repo/fork.go b/routers/api/v1/repo/fork.go index 90301cc35e..ec1b37b91a 100644 --- a/routers/api/v1/repo/fork.go +++ b/routers/api/v1/repo/fork.go @@ -89,7 +89,11 @@ func CreateFork(ctx *context.APIContext, form api.CreateForkOption) { } return } - if !org.IsOrgMember(ctx.User.ID) { + isMember, err := org.IsOrgMember(ctx.User.ID) + if err != nil { + ctx.Handle(500, "IsOrgMember", err) + return + } else if !isMember { ctx.Status(403) return } diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index b154d50a05..c9c7aa805d 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -108,8 +108,19 @@ func Search(ctx *context.APIContext) { } // Check visibility. - if ctx.IsSigned && (ctx.User.ID == repoOwner.ID || (repoOwner.IsOrganization() && repoOwner.IsOwnedBy(ctx.User.ID))) { - opts.Private = true + if ctx.IsSigned { + if ctx.User.ID == repoOwner.ID { + opts.Private = true + } else if repoOwner.IsOrganization() { + opts.Private, err = repoOwner.IsOwnedBy(ctx.User.ID) + if err != nil { + ctx.JSON(500, api.SearchError{ + OK: false, + Error: err.Error(), + }) + return + } + } } } @@ -245,7 +256,11 @@ func CreateOrgRepo(ctx *context.APIContext, opt api.CreateRepoOption) { return } - if !org.IsOwnedBy(ctx.User.ID) { + isOwner, err := org.IsOwnedBy(ctx.User.ID) + if err != nil { + ctx.Handle(500, "IsOwnedBy", err) + return + } else if !isOwner { ctx.Error(403, "", "Given user is not owner of organization.") return } @@ -292,7 +307,11 @@ func Migrate(ctx *context.APIContext, form auth.MigrateRepoForm) { if ctxUser.IsOrganization() && !ctx.User.IsAdmin { // Check ownership of organization. - if !ctxUser.IsOwnedBy(ctx.User.ID) { + isOwner, err := ctxUser.IsOwnedBy(ctx.User.ID) + if err != nil { + ctx.Error(500, "IsOwnedBy", err) + return + } else if !isOwner { ctx.Error(403, "", "Given user is not owner of organization.") return } @@ -431,9 +450,15 @@ func Delete(ctx *context.APIContext) { owner := ctx.Repo.Owner repo := ctx.Repo.Repository - if owner.IsOrganization() && !owner.IsOwnedBy(ctx.User.ID) { - ctx.Error(403, "", "Given user is not owner of organization.") - return + if owner.IsOrganization() { + isOwner, err := owner.IsOwnedBy(ctx.User.ID) + if err != nil { + ctx.Error(500, "IsOwnedBy", err) + return + } else if !isOwner { + ctx.Error(403, "", "Given user is not owner of organization.") + return + } } if err := models.DeleteRepository(ctx.User, owner.ID, repo.ID); err != nil { diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 578ead1343..4e12d62f30 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -475,6 +475,26 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) { ctx.Redirect(ctx.Repo.RepoLink + "/issues/" + com.ToStr(issue.Index)) } +// 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 + } + } + if poster.IsWriterOfRepo(repo) { + return models.CommentTagWriter, nil + } else if poster.ID == issue.PosterID { + return models.CommentTagPoster, nil + } + return models.CommentTagNone, nil +} + // ViewIssue render issue view page func ViewIssue(ctx *context.Context) { ctx.Data["RequireHighlightJS"] = true @@ -644,15 +664,11 @@ func ViewIssue(ctx *context.Context) { continue } - if repo.IsOwnedBy(comment.PosterID) || - (repo.Owner.IsOrganization() && repo.Owner.IsOwnedBy(comment.PosterID)) { - comment.ShowTag = models.CommentTagOwner - } else if comment.Poster.IsWriterOfRepo(repo) { - comment.ShowTag = models.CommentTagWriter - } else if comment.PosterID == issue.PosterID { - comment.ShowTag = models.CommentTagPoster + comment.ShowTag, err = commentTag(repo, comment.Poster, issue) + if err != nil { + ctx.Handle(500, "commentTag", err) + return } - marked[comment.PosterID] = comment.ShowTag isAdded := false diff --git a/routers/repo/pull.go b/routers/repo/pull.go index c2f0a07fe7..5575009af4 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -173,7 +173,11 @@ func ForkPost(ctx *context.Context, form auth.CreateRepoForm) { // Check ownership of organization. if ctxUser.IsOrganization() { - if !ctxUser.IsOwnedBy(ctx.User.ID) { + isOwner, err := ctxUser.IsOwnedBy(ctx.User.ID) + if err != nil { + ctx.Handle(500, "IsOwnedBy", err) + return + } else if !isOwner { ctx.Error(403) return } diff --git a/routers/repo/repo.go b/routers/repo/repo.go index aedc4e5477..4cd7c8062c 100644 --- a/routers/repo/repo.go +++ b/routers/repo/repo.go @@ -74,10 +74,20 @@ func checkContextUser(ctx *context.Context, uid int64) *models.User { } // Check ownership of organization. - if !org.IsOrganization() || !(ctx.User.IsAdmin || org.IsOwnedBy(ctx.User.ID)) { + if !org.IsOrganization() { ctx.Error(403) return nil } + if !ctx.User.IsAdmin { + isOwner, err := org.IsOwnedBy(ctx.User.ID) + if err != nil { + ctx.Handle(500, "IsOwnedBy", err) + return nil + } else if !isOwner { + ctx.Error(403) + return nil + } + } return org } diff --git a/routers/repo/setting.go b/routers/repo/setting.go index 329802673d..8cb551707c 100644 --- a/routers/repo/setting.go +++ b/routers/repo/setting.go @@ -234,13 +234,6 @@ func SettingsPost(ctx *context.Context, form auth.RepoSettingForm) { return } - if ctx.Repo.Owner.IsOrganization() { - if !ctx.Repo.Owner.IsOwnedBy(ctx.User.ID) { - ctx.Error(404) - return - } - } - if !repo.IsMirror { ctx.Error(404) return @@ -268,13 +261,6 @@ func SettingsPost(ctx *context.Context, form auth.RepoSettingForm) { return } - if ctx.Repo.Owner.IsOrganization() { - if !ctx.Repo.Owner.IsOwnedBy(ctx.User.ID) { - ctx.Error(404) - return - } - } - newOwner := ctx.Query("new_owner_name") isExist, err := models.IsUserExist(0, newOwner) if err != nil { @@ -307,13 +293,6 @@ func SettingsPost(ctx *context.Context, form auth.RepoSettingForm) { return } - if ctx.Repo.Owner.IsOrganization() { - if !ctx.Repo.Owner.IsOwnedBy(ctx.User.ID) { - ctx.Error(404) - return - } - } - if err := models.DeleteRepository(ctx.User, ctx.Repo.Owner.ID, repo.ID); err != nil { ctx.Handle(500, "DeleteRepository", err) return @@ -333,13 +312,6 @@ func SettingsPost(ctx *context.Context, form auth.RepoSettingForm) { return } - if ctx.Repo.Owner.IsOrganization() { - if !ctx.Repo.Owner.IsOwnedBy(ctx.User.ID) { - ctx.Error(404) - return - } - } - repo.DeleteWiki() log.Trace("Repository wiki deleted: %s/%s", ctx.Repo.Owner.Name, repo.Name) @@ -393,10 +365,16 @@ func CollaborationPost(ctx *context.Context) { } // Check if user is organization member. - if ctx.Repo.Owner.IsOrganization() && ctx.Repo.Owner.IsOrgMember(u.ID) { - ctx.Flash.Info(ctx.Tr("repo.settings.user_is_org_member")) - ctx.Redirect(ctx.Repo.RepoLink + "/settings/collaboration") - return + if ctx.Repo.Owner.IsOrganization() { + isMember, err := ctx.Repo.Owner.IsOrgMember(u.ID) + if err != nil { + ctx.Handle(500, "IsOrgMember", err) + return + } else if isMember { + ctx.Flash.Info(ctx.Tr("repo.settings.user_is_org_member")) + ctx.Redirect(ctx.Repo.RepoLink + "/settings/collaboration") + return + } } if err = ctx.Repo.Repository.AddCollaborator(u); err != nil {