Fix branch_protection api shows users/teams who has no readAccess (#30291) (#30544)

Backport #30291 by @edwardzhanged

Add some logic in `convert.ToBranchProtection` to return only the names
associated with readAccess instead of returning all names. This will
ensure consistency in behavior between the frontend and backend.
Fixes: #27694

Co-authored-by: Edward Zhang <[email protected]>
Co-authored-by: techknowlogick <[email protected]>
Co-authored-by: wenzhuo.zhang <[email protected]>

Author: 4 people

routers/api/v1/repo/branch.go

@@ -437,7 +437,7 @@ func GetBranchProtection(ctx *context.APIContext) {
 		return
 	}
 
-	ctx.JSON(http.StatusOK, convert.ToBranchProtection(ctx, bp))
+	ctx.JSON(http.StatusOK, convert.ToBranchProtection(ctx, bp, repo))
 }
 
 // ListBranchProtections list branch protections for a repo
@@ -470,7 +470,7 @@ func ListBranchProtections(ctx *context.APIContext) {
 	}
 	apiBps := make([]*api.BranchProtection, len(bps))
 	for i := range bps {
-		apiBps[i] = convert.ToBranchProtection(ctx, bps[i])
+		apiBps[i] = convert.ToBranchProtection(ctx, bps[i], repo)
 	}
 
 	ctx.JSON(http.StatusOK, apiBps)
@@ -681,7 +681,7 @@ func CreateBranchProtection(ctx *context.APIContext) {
 		return
 	}
 
-	ctx.JSON(http.StatusCreated, convert.ToBranchProtection(ctx, bp))
+	ctx.JSON(http.StatusCreated, convert.ToBranchProtection(ctx, bp, repo))
 }
 
 // EditBranchProtection edits a branch protection for a repo
@@ -959,7 +959,7 @@ func EditBranchProtection(ctx *context.APIContext) {
 		return
 	}
 
-	ctx.JSON(http.StatusOK, convert.ToBranchProtection(ctx, bp))
+	ctx.JSON(http.StatusOK, convert.ToBranchProtection(ctx, bp, repo))
 }
 
 // DeleteBranchProtection deletes a branch protection for a repo

services/convert/convert.go

@@ -21,6 +21,7 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/container"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	api "code.gitea.io/gitea/modules/structs"
@@ -105,33 +106,46 @@ func ToBranch(ctx context.Context, repo *repo_model.Repository, branchName strin
 	return branch, nil
 }
 
-// ToBranchProtection convert a ProtectedBranch to api.BranchProtection
-func ToBranchProtection(ctx context.Context, bp *git_model.ProtectedBranch) *api.BranchProtection {
-	pushWhitelistUsernames, err := user_model.GetUserNamesByIDs(ctx, bp.WhitelistUserIDs)
-	if err != nil {
-		log.Error("GetUserNamesByIDs (WhitelistUserIDs): %v", err)
-	}
-	mergeWhitelistUsernames, err := user_model.GetUserNamesByIDs(ctx, bp.MergeWhitelistUserIDs)
-	if err != nil {
-		log.Error("GetUserNamesByIDs (MergeWhitelistUserIDs): %v", err)
-	}
-	approvalsWhitelistUsernames, err := user_model.GetUserNamesByIDs(ctx, bp.ApprovalsWhitelistUserIDs)
-	if err != nil {
-		log.Error("GetUserNamesByIDs (ApprovalsWhitelistUserIDs): %v", err)
-	}
-	pushWhitelistTeams, err := organization.GetTeamNamesByID(ctx, bp.WhitelistTeamIDs)
-	if err != nil {
-		log.Error("GetTeamNamesByID (WhitelistTeamIDs): %v", err)
+// getWhitelistEntities returns the names of the entities that are in the whitelist
+func getWhitelistEntities[T *user_model.User | *organization.Team](entities []T, whitelistIDs []int64) []string {
+	whitelistUserIDsSet := container.SetOf(whitelistIDs...)
+	whitelistNames := make([]string, 0)
+	for _, entity := range entities {
+		switch v := any(entity).(type) {
+		case *user_model.User:
+			if whitelistUserIDsSet.Contains(v.ID) {
+				whitelistNames = append(whitelistNames, v.Name)
+			}
+		case *organization.Team:
+			if whitelistUserIDsSet.Contains(v.ID) {
+				whitelistNames = append(whitelistNames, v.Name)
+			}
+		}
 	}
-	mergeWhitelistTeams, err := organization.GetTeamNamesByID(ctx, bp.MergeWhitelistTeamIDs)
+
+	return whitelistNames
+}
+
+// ToBranchProtection convert a ProtectedBranch to api.BranchProtection
+func ToBranchProtection(ctx context.Context, bp *git_model.ProtectedBranch, repo *repo_model.Repository) *api.BranchProtection {
+	readers, err := access_model.GetRepoReaders(ctx, repo)
 	if err != nil {
-		log.Error("GetTeamNamesByID (MergeWhitelistTeamIDs): %v", err)
+		log.Error("GetRepoReaders: %v", err)
 	}
-	approvalsWhitelistTeams, err := organization.GetTeamNamesByID(ctx, bp.ApprovalsWhitelistTeamIDs)
+
+	pushWhitelistUsernames := getWhitelistEntities(readers, bp.WhitelistUserIDs)
+	mergeWhitelistUsernames := getWhitelistEntities(readers, bp.MergeWhitelistUserIDs)
+	approvalsWhitelistUsernames := getWhitelistEntities(readers, bp.ApprovalsWhitelistUserIDs)
+
+	teamReaders, err := organization.OrgFromUser(repo.Owner).TeamsWithAccessToRepo(ctx, repo.ID, perm.AccessModeRead)
 	if err != nil {
-		log.Error("GetTeamNamesByID (ApprovalsWhitelistTeamIDs): %v", err)
+		log.Error("Repo.Owner.TeamsWithAccessToRepo: %v", err)
 	}
 
+	pushWhitelistTeams := getWhitelistEntities(teamReaders, bp.WhitelistTeamIDs)
+	mergeWhitelistTeams := getWhitelistEntities(teamReaders, bp.MergeWhitelistTeamIDs)
+	approvalsWhitelistTeams := getWhitelistEntities(teamReaders, bp.ApprovalsWhitelistTeamIDs)
+
 	branchName := ""
 	if !git_model.IsRuleNameSpecial(bp.RuleName) {
 		branchName = bp.RuleName