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

5 files changed, 80 insertions, 22 deletions

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 {