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

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 {