From 515cdaa85d6087d91a61ebe74fae39e0c4bdf1c4 Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Wed, 20 Dec 2017 23:43:26 -0800 Subject: 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 ++++++- 6 files changed, 141 insertions(+), 77 deletions(-) (limited to 'models') 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) { -- cgit v1.2.3