Fix ignored errors when checking if organization, team member (#3177)

6 files changed, 141 insertions, 77 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) {