refactor: migrate protected branch fields into one json column and support system users (#39735)

1. migrate protected branch fields into one json column
2. use the dropdown list for push/force-push user selection, and Actions
user can be added into the list now.

routers/api/v1/repo/branch.go

@@ -7,6 +7,7 @@ package repo
 import (
 	"errors"
 	"net/http"
+	"slices"
 
 	"gitea.dev/models/db"
 	git_model "gitea.dev/models/git"
@@ -618,6 +619,34 @@ func ListBranchProtections(ctx *context.APIContext) {
 	ctx.JSON(http.StatusOK, apiBps)
 }
 
+func legacySetDeployKeyAllowlist(userIDs []int64, enabled bool) []int64 {
+	if !enabled {
+		return util.SliceRemoveAll(userIDs, user_model.DeployKeyUserID)
+	}
+	if !slices.Contains(userIDs, user_model.DeployKeyUserID) {
+		userIDs = append(userIDs, user_model.DeployKeyUserID)
+	}
+	return userIDs
+}
+
+func loadBranchProtectionUserIDs(ctx *context.APIContext, names []string, def ...[]int64) []int64 {
+	if names == nil {
+		return util.OptionalArg(def)
+	}
+	if ctx.Written() {
+		return nil
+	}
+	ids, err := user_model.GetDoerUserIDsByNamesWithSystem(ctx, names)
+	if err != nil {
+		if errors.Is(err, util.ErrNotExist) {
+			ctx.APIError(http.StatusUnprocessableEntity, err.Error())
+		} else {
+			ctx.APIErrorInternal(err)
+		}
+	}
+	return ids
+}
+
 // CreateBranchProtection creates a branch protection for a repo
 func CreateBranchProtection(ctx *context.APIContext) {
 	// swagger:operation POST /repos/{owner}/{repo}/branch_protections repository repoCreateBranchProtection
@@ -680,54 +709,15 @@ func CreateBranchProtection(ctx *context.APIContext) {
 		requiredApprovals = form.RequiredApprovals
 	}
 
-	whitelistUsers, err := user_model.GetUserIDsByNames(ctx, form.PushWhitelistUsernames, false)
-	if err != nil {
-		if user_model.IsErrUserNotExist(err) {
-			ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-			return
-		}
-		ctx.APIErrorInternal(err)
+	whitelistUsers := loadBranchProtectionUserIDs(ctx, form.PushWhitelistUsernames)
+	forcePushAllowlistUsers := loadBranchProtectionUserIDs(ctx, form.ForcePushAllowlistUsernames)
+	mergeWhitelistUsers := loadBranchProtectionUserIDs(ctx, form.MergeWhitelistUsernames)
+	approvalsWhitelistUsers := loadBranchProtectionUserIDs(ctx, form.ApprovalsWhitelistUsernames)
+	bypassAllowlistUsers := loadBranchProtectionUserIDs(ctx, util.Iif(form.EnableBypassAllowlist, form.BypassAllowlistUsernames, nil))
+	if ctx.Written() {
 		return
 	}
-	forcePushAllowlistUsers, err := user_model.GetUserIDsByNames(ctx, form.ForcePushAllowlistUsernames, false)
-	if err != nil {
-		if user_model.IsErrUserNotExist(err) {
-			ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-			return
-		}
-		ctx.APIErrorInternal(err)
-		return
-	}
-	mergeWhitelistUsers, err := user_model.GetUserIDsByNames(ctx, form.MergeWhitelistUsernames, false)
-	if err != nil {
-		if user_model.IsErrUserNotExist(err) {
-			ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-			return
-		}
-		ctx.APIErrorInternal(err)
-		return
-	}
-	approvalsWhitelistUsers, err := user_model.GetUserIDsByNames(ctx, form.ApprovalsWhitelistUsernames, false)
-	if err != nil {
-		if user_model.IsErrUserNotExist(err) {
-			ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-			return
-		}
-		ctx.APIErrorInternal(err)
-		return
-	}
-	var bypassAllowlistUsers []int64
-	if form.EnableBypassAllowlist {
-		bypassAllowlistUsers, err = user_model.GetUserIDsByNames(ctx, form.BypassAllowlistUsernames, false)
-		if err != nil {
-			if user_model.IsErrUserNotExist(err) {
-				ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-				return
-			}
-			ctx.APIErrorInternal(err)
-			return
-		}
-	}
+
 	var whitelistTeams, forcePushAllowlistTeams, mergeWhitelistTeams, approvalsWhitelistTeams, bypassAllowlistTeams []int64
 	if repo.Owner.IsOrganization() {
 		whitelistTeams, err = organization.GetTeamIDsByNames(ctx, repo.OwnerID, form.PushWhitelistTeams, false)
@@ -779,32 +769,40 @@ func CreateBranchProtection(ctx *context.APIContext) {
 		}
 	}
 
+	if form.EnablePush && form.EnablePushWhitelist && form.PushWhitelistDeployKeys {
+		whitelistUsers = legacySetDeployKeyAllowlist(whitelistUsers, true)
+	}
+	if form.EnablePush && form.EnableForcePush && form.EnableForcePushAllowlist && form.ForcePushAllowlistDeployKeys {
+		forcePushAllowlistUsers = legacySetDeployKeyAllowlist(forcePushAllowlistUsers, true)
+	}
+
 	protectBranch = &git_model.ProtectedBranch{
-		RepoID:                        ctx.Repo.Repository.ID,
-		RuleName:                      ruleName,
-		Priority:                      form.Priority,
-		CanPush:                       form.EnablePush,
-		EnableWhitelist:               form.EnablePush && form.EnablePushWhitelist,
-		WhitelistDeployKeys:           form.EnablePush && form.EnablePushWhitelist && form.PushWhitelistDeployKeys,
-		CanForcePush:                  form.EnablePush && form.EnableForcePush,
-		EnableForcePushAllowlist:      form.EnablePush && form.EnableForcePush && form.EnableForcePushAllowlist,
-		ForcePushAllowlistDeployKeys:  form.EnablePush && form.EnableForcePush && form.EnableForcePushAllowlist && form.ForcePushAllowlistDeployKeys,
-		EnableMergeWhitelist:          form.EnableMergeWhitelist,
-		EnableBypassAllowlist:         form.EnableBypassAllowlist,
-		EnableStatusCheck:             form.EnableStatusCheck,
-		StatusCheckContexts:           form.StatusCheckContexts,
-		EnableApprovalsWhitelist:      form.EnableApprovalsWhitelist,
-		RequiredApprovals:             requiredApprovals,
-		BlockOnRejectedReviews:        form.BlockOnRejectedReviews,
-		BlockOnOfficialReviewRequests: form.BlockOnOfficialReviewRequests,
-		BlockOnCodeownerReviews:       form.BlockOnCodeownerReviews,
-		DismissStaleApprovals:         form.DismissStaleApprovals,
-		IgnoreStaleApprovals:          form.IgnoreStaleApprovals,
-		RequireSignedCommits:          form.RequireSignedCommits,
-		ProtectedFilePatterns:         form.ProtectedFilePatterns,
-		UnprotectedFilePatterns:       form.UnprotectedFilePatterns,
-		BlockOnOutdatedBranch:         form.BlockOnOutdatedBranch,
-		BlockAdminMergeOverride:       form.BlockAdminMergeOverride,
+		ProtectedBranchConfig: git_model.ProtectedBranchConfig{
+			CanPush:                       form.EnablePush,
+			EnableWhitelist:               form.EnablePush && form.EnablePushWhitelist,
+			CanForcePush:                  form.EnablePush && form.EnableForcePush,
+			EnableForcePushAllowlist:      form.EnablePush && form.EnableForcePush && form.EnableForcePushAllowlist,
+			EnableMergeWhitelist:          form.EnableMergeWhitelist,
+			EnableBypassAllowlist:         form.EnableBypassAllowlist,
+			EnableStatusCheck:             form.EnableStatusCheck,
+			StatusCheckContexts:           form.StatusCheckContexts,
+			EnableApprovalsWhitelist:      form.EnableApprovalsWhitelist,
+			RequiredApprovals:             requiredApprovals,
+			BlockOnRejectedReviews:        form.BlockOnRejectedReviews,
+			BlockOnOfficialReviewRequests: form.BlockOnOfficialReviewRequests,
+			BlockOnCodeownerReviews:       form.BlockOnCodeownerReviews,
+			DismissStaleApprovals:         form.DismissStaleApprovals,
+			IgnoreStaleApprovals:          form.IgnoreStaleApprovals,
+			RequireSignedCommits:          form.RequireSignedCommits,
+			ProtectedFilePatterns:         form.ProtectedFilePatterns,
+			UnprotectedFilePatterns:       form.UnprotectedFilePatterns,
+			BlockOnOutdatedBranch:         form.BlockOnOutdatedBranch,
+			BlockAdminMergeOverride:       form.BlockAdminMergeOverride,
+		},
+
+		RepoID:   ctx.Repo.Repository.ID,
+		RuleName: ruleName,
+		Priority: form.Priority,
 	}
 
 	if err := pull_service.CreateOrUpdateProtectedBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{
@@ -819,18 +817,17 @@ func CreateBranchProtection(ctx *context.APIContext) {
 		BypassUserIDs:    bypassAllowlistUsers,
 		BypassTeamIDs:    bypassAllowlistTeams,
 	}); err != nil {
-		ctx.APIErrorInternal(err)
+		ctx.APIErrorAuto(err)
 		return
 	}
 
 	// Reload from db to get all whitelists
 	bp, err := git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, ruleName)
 	if err != nil {
-		ctx.APIErrorInternal(err)
+		ctx.APIErrorAuto(err)
 		return
-	}
-	if bp == nil || bp.RepoID != ctx.Repo.Repository.ID {
-		ctx.APIErrorInternal(err)
+	} else if bp == nil {
+		ctx.APIErrorNotFound()
 		return
 	}
 
@@ -888,21 +885,23 @@ func EditBranchProtection(ctx *context.APIContext) {
 		return
 	}
 
+	pushDeployKeys := protectBranch.HasDeployKeyPush()
+	forcePushDeployKeys := protectBranch.HasDeployKeyForcePush()
 	if form.EnablePush != nil {
 		if !*form.EnablePush {
 			protectBranch.CanPush = false
 			protectBranch.EnableWhitelist = false
-			protectBranch.WhitelistDeployKeys = false
+			pushDeployKeys = false
 		} else {
 			protectBranch.CanPush = true
 			if form.EnablePushWhitelist != nil {
 				if !*form.EnablePushWhitelist {
 					protectBranch.EnableWhitelist = false
-					protectBranch.WhitelistDeployKeys = false
+					pushDeployKeys = false
 				} else {
 					protectBranch.EnableWhitelist = true
 					if form.PushWhitelistDeployKeys != nil {
-						protectBranch.WhitelistDeployKeys = *form.PushWhitelistDeployKeys
+						pushDeployKeys = *form.PushWhitelistDeployKeys
 					}
 				}
 			}
@@ -913,17 +912,17 @@ func EditBranchProtection(ctx *context.APIContext) {
 		if !*form.EnableForcePush {
 			protectBranch.CanForcePush = false
 			protectBranch.EnableForcePushAllowlist = false
-			protectBranch.ForcePushAllowlistDeployKeys = false
+			forcePushDeployKeys = false
 		} else {
 			protectBranch.CanForcePush = true
 			if form.EnableForcePushAllowlist != nil {
 				if !*form.EnableForcePushAllowlist {
 					protectBranch.EnableForcePushAllowlist = false
-					protectBranch.ForcePushAllowlistDeployKeys = false
+					forcePushDeployKeys = false
 				} else {
 					protectBranch.EnableForcePushAllowlist = true
 					if form.ForcePushAllowlistDeployKeys != nil {
-						protectBranch.ForcePushAllowlistDeployKeys = *form.ForcePushAllowlistDeployKeys
+						forcePushDeployKeys = *form.ForcePushAllowlistDeployKeys
 					}
 				}
 			}
@@ -998,72 +997,23 @@ func EditBranchProtection(ctx *context.APIContext) {
 		protectBranch.BlockAdminMergeOverride = *form.BlockAdminMergeOverride
 	}
 
-	var whitelistUsers, forcePushAllowlistUsers, mergeWhitelistUsers, approvalsWhitelistUsers, bypassAllowlistUsers []int64
-	if form.PushWhitelistUsernames != nil {
-		whitelistUsers, err = user_model.GetUserIDsByNames(ctx, form.PushWhitelistUsernames, false)
-		if err != nil {
-			if user_model.IsErrUserNotExist(err) {
-				ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-				return
-			}
-			ctx.APIErrorInternal(err)
-			return
-		}
-	} else {
-		whitelistUsers = protectBranch.WhitelistUserIDs
+	whitelistUsers := loadBranchProtectionUserIDs(ctx, form.PushWhitelistUsernames, protectBranch.WhitelistUserIDs)
+	forcePushAllowlistUsers := loadBranchProtectionUserIDs(ctx, form.ForcePushAllowlistUsernames, protectBranch.ForcePushAllowlistUserIDs)
+	mergeWhitelistUsers := loadBranchProtectionUserIDs(ctx, form.MergeWhitelistUsernames, protectBranch.MergeWhitelistUserIDs)
+	approvalsWhitelistUsers := loadBranchProtectionUserIDs(ctx, form.ApprovalsWhitelistUsernames, protectBranch.ApprovalsWhitelistUserIDs)
+	bypassAllowlistUsers := loadBranchProtectionUserIDs(ctx, form.BypassAllowlistUsernames, protectBranch.BypassAllowlistUserIDs)
+	if ctx.Written() {
+		return
 	}
-	if form.ForcePushAllowlistUsernames != nil {
-		forcePushAllowlistUsers, err = user_model.GetUserIDsByNames(ctx, form.ForcePushAllowlistUsernames, false)
-		if err != nil {
-			if user_model.IsErrUserNotExist(err) {
-				ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-				return
-			}
-			ctx.APIErrorInternal(err)
-			return
-		}
-	} else {
-		forcePushAllowlistUsers = protectBranch.ForcePushAllowlistUserIDs
+
+	if form.PushWhitelistUsernames != nil && form.PushWhitelistDeployKeys == nil && slices.Contains(whitelistUsers, user_model.DeployKeyUserID) {
+		pushDeployKeys = true
 	}
-	if form.MergeWhitelistUsernames != nil {
-		mergeWhitelistUsers, err = user_model.GetUserIDsByNames(ctx, form.MergeWhitelistUsernames, false)
-		if err != nil {
-			if user_model.IsErrUserNotExist(err) {
-				ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-				return
-			}
-			ctx.APIErrorInternal(err)
-			return
-		}
-	} else {
-		mergeWhitelistUsers = protectBranch.MergeWhitelistUserIDs
-	}
-	if form.ApprovalsWhitelistUsernames != nil {
-		approvalsWhitelistUsers, err = user_model.GetUserIDsByNames(ctx, form.ApprovalsWhitelistUsernames, false)
-		if err != nil {
-			if user_model.IsErrUserNotExist(err) {
-				ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-				return
-			}
-			ctx.APIErrorInternal(err)
-			return
-		}
-	} else {
-		approvalsWhitelistUsers = protectBranch.ApprovalsWhitelistUserIDs
-	}
-	if form.BypassAllowlistUsernames != nil {
-		bypassAllowlistUsers, err = user_model.GetUserIDsByNames(ctx, form.BypassAllowlistUsernames, false)
-		if err != nil {
-			if user_model.IsErrUserNotExist(err) {
-				ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-				return
-			}
-			ctx.APIErrorInternal(err)
-			return
-		}
-	} else {
-		bypassAllowlistUsers = protectBranch.BypassAllowlistUserIDs
+	if form.ForcePushAllowlistUsernames != nil && form.ForcePushAllowlistDeployKeys == nil && slices.Contains(forcePushAllowlistUsers, user_model.DeployKeyUserID) {
+		forcePushDeployKeys = true
 	}
+	whitelistUsers = legacySetDeployKeyAllowlist(whitelistUsers, pushDeployKeys)
+	forcePushAllowlistUsers = legacySetDeployKeyAllowlist(forcePushAllowlistUsers, forcePushDeployKeys)
 
 	var whitelistTeams, forcePushAllowlistTeams, mergeWhitelistTeams, approvalsWhitelistTeams, bypassAllowlistTeams []int64
 	if repo.Owner.IsOrganization() {
@@ -1151,7 +1101,7 @@ func EditBranchProtection(ctx *context.APIContext) {
 		BypassTeamIDs:    bypassAllowlistTeams,
 	})
 	if err != nil {
-		ctx.APIErrorInternal(err)
+		ctx.APIErrorAuto(err)
 		return
 	}
 
@@ -1199,11 +1149,10 @@ func EditBranchProtection(ctx *context.APIContext) {
 	// Reload from db to ensure get all whitelists
 	bp, err := git_model.GetProtectedBranchRuleByName(ctx, repo.ID, bpName)
 	if err != nil {
-		ctx.APIErrorInternal(err)
+		ctx.APIErrorAuto(err)
 		return
-	}
-	if bp == nil || bp.RepoID != ctx.Repo.Repository.ID {
-		ctx.APIErrorInternal(err)
+	} else if bp == nil {
+		ctx.APIErrorNotFound()
 		return
 	}
 

routers/api/v1/repo/issue.go

@@ -584,8 +584,8 @@ func CreateIssue(ctx *context.APIContext) {
 		issue.MilestoneID = form.Milestone
 		assigneeIDs, err = issues_model.MakeIDsFromAPIAssigneesToAdd(ctx, form.Assignee, form.Assignees)
 		if err != nil {
-			if user_model.IsErrUserNotExist(err) {
-				ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("Assignee does not exist: [name: %s]", err))
+			if errors.Is(err, util.ErrNotExist) {
+				ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("Assignee does not exist: %s", err))
 			} else {
 				ctx.APIErrorInternal(err)
 			}

routers/api/v1/repo/issue_assignee.go

@@ -4,6 +4,7 @@
 package repo
 
 import (
+	"errors"
 	"net/http"
 
 	issues_model "gitea.dev/models/issues"
@@ -11,6 +12,7 @@ import (
 	repo_model "gitea.dev/models/repo"
 	user_model "gitea.dev/models/user"
 	api "gitea.dev/modules/structs"
+	"gitea.dev/modules/util"
 	"gitea.dev/modules/web"
 	"gitea.dev/services/context"
 	"gitea.dev/services/convert"
@@ -203,13 +205,13 @@ func updateIssueAssignees(ctx *context.APIContext, opts api.IssueAssigneesOption
 		return
 	}
 
-	assigneeIDs, err := user_model.GetUserIDsByNames(ctx, opts.Assignees, false)
+	assigneeIDs, err := user_model.GetDoerUserIDsByNames(ctx, opts.Assignees)
 	if err != nil {
-		if user_model.IsErrUserNotExist(err) {
+		if errors.Is(err, util.ErrNotExist) {
 			ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-			return
+		} else {
+			ctx.APIErrorAuto(err)
 		}
-		ctx.APIErrorAuto(err)
 		return
 	}
 

routers/api/v1/repo/pull.go

@@ -521,7 +521,7 @@ func CreatePullRequest(ctx *context.APIContext) {
 	// Get all assignee IDs
 	assigneeIDs, err := issues_model.MakeIDsFromAPIAssigneesToAdd(ctx, form.Assignee, form.Assignees)
 	if err != nil {
-		if user_model.IsErrUserNotExist(err) {
+		if errors.Is(err, util.ErrNotExist) {
 			ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("Assignee does not exist: [name: %s]", err))
 		} else {
 			ctx.APIErrorInternal(err)

routers/api/v1/repo/tag.go

@@ -14,6 +14,7 @@ import (
 	repo_model "gitea.dev/models/repo"
 	user_model "gitea.dev/models/user"
 	api "gitea.dev/modules/structs"
+	"gitea.dev/modules/util"
 	"gitea.dev/modules/web"
 	"gitea.dev/routers/api/v1/utils"
 	"gitea.dev/services/context"
@@ -435,13 +436,13 @@ func CreateTagProtection(ctx *context.APIContext) {
 	}
 
 	var whitelistUsers, whitelistTeams []int64
-	whitelistUsers, err = user_model.GetUserIDsByNames(ctx, form.WhitelistUsernames, false)
+	whitelistUsers, err = user_model.GetDoerUserIDsByNames(ctx, form.WhitelistUsernames)
 	if err != nil {
-		if user_model.IsErrUserNotExist(err) {
+		if errors.Is(err, util.ErrNotExist) {
 			ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-			return
+		} else {
+			ctx.APIErrorInternal(err)
 		}
-		ctx.APIErrorInternal(err)
 		return
 	}
 
@@ -557,13 +558,13 @@ func EditTagProtection(ctx *context.APIContext) {
 	}
 
 	if form.WhitelistUsernames != nil {
-		whitelistUsers, err = user_model.GetUserIDsByNames(ctx, form.WhitelistUsernames, false)
+		whitelistUsers, err = user_model.GetDoerUserIDsByNames(ctx, form.WhitelistUsernames)
 		if err != nil {
-			if user_model.IsErrUserNotExist(err) {
+			if errors.Is(err, util.ErrNotExist) {
 				ctx.APIError(http.StatusUnprocessableEntity, err.Error())
-				return
+			} else {
+				ctx.APIErrorInternal(err)
 			}
-			ctx.APIErrorInternal(err)
 			return
 		}
 		pt.AllowlistUserIDs = whitelistUsers