From dfa7291f8fafd2aac032ef73a78b588e4d0e9a36 Mon Sep 17 00:00:00 2001 From: 赵智超 <1012112796@qq.com> Date: Wed, 14 Oct 2020 02:50:57 +0800 Subject: [Enhancement] Allow admin to merge pr with protected file changes (#12078) * [Enhancement] Allow admin to merge pr with protected file changes As tilte, show protected message in diff page and merge box. Signed-off-by: a1012112796 <1012112796@qq.com> * remove unused ver * Update options/locale/locale_en-US.ini Co-authored-by: Cirno the Strongest <1447794+CirnoT@users.noreply.github.com> * Add TrN * Apply suggestions from code review * fix lint * Update options/locale/locale_en-US.ini Co-authored-by: zeripath * Apply suggestions from code review * move pr proteced files check to TestPatch * Call TestPatch when protected branches settings changed * Apply review suggestion @CirnoT * move to service @lunny * slightly restructure routers/private/hook.go Adds a lot of comments and simplifies the logic Signed-off-by: Andrew Thornton * placate lint Signed-off-by: Andrew Thornton * skip duplicate protected files check * fix check logic * slight refactor of TestPatch Signed-off-by: Andrew Thornton * When checking for protected files changes in TestPatch use the temporary repository Signed-off-by: Andrew Thornton * fix introduced issue with hook Signed-off-by: Andrew Thornton * Remove the check on PR index being greater than 0 as it unnecessary Signed-off-by: Andrew Thornton Co-authored-by: techknowlogick Co-authored-by: Cirno the Strongest <1447794+CirnoT@users.noreply.github.com> Co-authored-by: zeripath Co-authored-by: techknowlogick --- routers/api/v1/repo/branch.go | 11 ++ routers/api/v1/repo/pull.go | 2 +- routers/private/hook.go | 311 +++++++++++++++---------------- routers/repo/issue.go | 3 + routers/repo/pull.go | 16 +- routers/repo/setting_protected_branch.go | 5 + 6 files changed, 190 insertions(+), 158 deletions(-) (limited to 'routers') diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 6c63bde948..50ca3977a5 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/log" repo_module "code.gitea.io/gitea/modules/repository" api "code.gitea.io/gitea/modules/structs" + pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" ) @@ -545,6 +546,11 @@ func CreateBranchProtection(ctx *context.APIContext, form api.CreateBranchProtec return } + if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil { + ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err) + return + } + // Reload from db to get all whitelists bp, err := models.GetProtectedBranchBy(ctx.Repo.Repository.ID, form.BranchName) if err != nil { @@ -768,6 +774,11 @@ func EditBranchProtection(ctx *context.APIContext, form api.EditBranchProtection return } + if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil { + ctx.Error(http.StatusInternalServerError, "CheckPrsForBaseBranch", err) + return + } + // Reload from db to ensure get all whitelists bp, err := models.GetProtectedBranchBy(repo.ID, bpName) if err != nil { diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index afbe7ddab0..e2cab70adc 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -774,7 +774,7 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { return } - if err := pull_service.CheckPRReadyToMerge(pr); err != nil { + if err := pull_service.CheckPRReadyToMerge(pr, false); err != nil { if !models.IsErrNotAllowedToMerge(err) { ctx.Error(http.StatusInternalServerError, "CheckPRReadyToMerge", err) return diff --git a/routers/private/hook.go b/routers/private/hook.go index 05f0b124c5..a2033fc1dd 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -25,7 +25,6 @@ import ( "gitea.com/macaron/macaron" "github.com/go-git/go-git/v5/plumbing" - "github.com/gobwas/glob" ) func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env []string) error { @@ -59,53 +58,6 @@ func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env [] return err } -func checkFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, repo *git.Repository, env []string) error { - - stdoutReader, stdoutWriter, err := os.Pipe() - if err != nil { - log.Error("Unable to create os.Pipe for %s", repo.Path) - return err - } - defer func() { - _ = stdoutReader.Close() - _ = stdoutWriter.Close() - }() - - // This use of ... is safe as force-pushes have already been ruled out. - err = git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID). - RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, - stdoutWriter, nil, nil, - func(ctx context.Context, cancel context.CancelFunc) error { - _ = stdoutWriter.Close() - - scanner := bufio.NewScanner(stdoutReader) - for scanner.Scan() { - path := strings.TrimSpace(scanner.Text()) - if len(path) == 0 { - continue - } - lpath := strings.ToLower(path) - for _, pat := range patterns { - if pat.Match(lpath) { - cancel() - return models.ErrFilePathProtected{ - Path: path, - } - } - } - } - if err := scanner.Err(); err != nil { - return err - } - _ = stdoutReader.Close() - return err - }) - if err != nil && !models.IsErrFilePathProtected(err) { - log.Error("Unable to check file protection for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err) - } - return err -} - func readAndVerifyCommitsFromShaReader(input io.ReadCloser, repo *git.Repository, env []string) error { scanner := bufio.NewScanner(input) for scanner.Scan() { @@ -202,6 +154,7 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { private.GitQuarantinePath+"="+opts.GitQuarantinePath) } + // Iterate across the provided old commit IDs for i := range opts.OldCommitIDs { oldCommitID := opts.OldCommitIDs[i] newCommitID := opts.NewCommitIDs[i] @@ -224,143 +177,189 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) { }) return } - if protectBranch != nil && protectBranch.IsProtected() { - // detect and prevent deletion - if newCommitID == git.EmptySHA { - log.Warn("Forbidden: Branch: %s in %-v is protected from deletion", branchName, repo) + + // Allow pushes to non-protected branches + if protectBranch == nil || !protectBranch.IsProtected() { + continue + } + + // This ref is a protected branch. + // + // First of all we need to enforce absolutely: + // + // 1. Detect and prevent deletion of the branch + if newCommitID == git.EmptySHA { + log.Warn("Forbidden: Branch: %s in %-v is protected from deletion", branchName, repo) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("branch %s is protected from deletion", branchName), + }) + return + } + + // 2. Disallow force pushes to protected branches + if git.EmptySHA != oldCommitID { + output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env) + if err != nil { + log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Fail to detect force push: %v", err), + }) + return + } else if len(output) > 0 { + log.Warn("Forbidden: Branch: %s in %-v is protected from force push", branchName, repo) ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("branch %s is protected from deletion", branchName), + "err": fmt.Sprintf("branch %s is protected from force push", branchName), }) return + } + } - // detect force push - if git.EmptySHA != oldCommitID { - output, err := git.NewCommand("rev-list", "--max-count=1", oldCommitID, "^"+newCommitID).RunInDirWithEnv(repo.RepoPath(), env) - if err != nil { - log.Error("Unable to detect force push between: %s and %s in %-v Error: %v", oldCommitID, newCommitID, repo, err) + // 3. Enforce require signed commits + if protectBranch.RequireSignedCommits { + err := verifyCommits(oldCommitID, newCommitID, gitRepo, env) + if err != nil { + if !isErrUnverifiedCommit(err) { + log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Fail to detect force push: %v", err), + "err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err), }) return - } else if len(output) > 0 { - log.Warn("Forbidden: Branch: %s in %-v is protected from force push", branchName, repo) - ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("branch %s is protected from force push", branchName), - }) - return - } + unverifiedCommit := err.(*errUnverifiedCommit).sha + log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, unverifiedCommit) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit), + }) + return } + } - // Require signed commits - if protectBranch.RequireSignedCommits { - err := verifyCommits(oldCommitID, newCommitID, gitRepo, env) - if err != nil { - if !isErrUnverifiedCommit(err) { - log.Error("Unable to check commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to check commits from %s to %s: %v", oldCommitID, newCommitID, err), - }) - return - } - unverifiedCommit := err.(*errUnverifiedCommit).sha - log.Warn("Forbidden: Branch: %s in %-v is protected from unverified commit %s", branchName, repo, unverifiedCommit) - ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("branch %s is protected from unverified commit %s", branchName, unverifiedCommit), + // Now there are several tests which can be overridden: + // + // 4. Check protected file patterns - this is overridable from the UI + changedProtectedfiles := false + protectedFilePath := "" + + globs := protectBranch.GetProtectedFilePatterns() + if len(globs) > 0 { + _, err := pull_service.CheckFileProtection(oldCommitID, newCommitID, globs, 1, env, gitRepo) + if err != nil { + if !models.IsErrFilePathProtected(err) { + log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err), }) return } + + changedProtectedfiles = true + protectedFilePath = err.(models.ErrFilePathProtected).Path } + } - // Detect Protected file pattern - globs := protectBranch.GetProtectedFilePatterns() - if len(globs) > 0 { - err := checkFileProtection(oldCommitID, newCommitID, globs, gitRepo, env) - if err != nil { - if !models.IsErrFilePathProtected(err) { - log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err), - }) - return - } - protectedFilePath := err.(models.ErrFilePathProtected).Path + // 5. Check if the doer is allowed to push + canPush := false + if opts.IsDeployKey { + canPush = !changedProtectedfiles && protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys) + } else { + canPush = !changedProtectedfiles && protectBranch.CanUserPush(opts.UserID) + } + + // 6. If we're not allowed to push directly + if !canPush { + // Is this is a merge from the UI/API? + if opts.ProtectedBranchID == 0 { + // 6a. If we're not merging from the UI/API then there are two ways we got here: + // + // We are changing a protected file and we're not allowed to do that + if changedProtectedfiles { log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath) ctx.JSON(http.StatusForbidden, map[string]interface{}{ "err": fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath), }) return } + + // Or we're simply not able to push to this protected branch + log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName), + }) + return + } + // 6b. Merge (from UI or API) + + // Get the PR, user and permissions for the user in the repository + pr, err := models.GetPullRequestByID(opts.ProtectedBranchID) + if err != nil { + log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err), + }) + return + } + user, err := models.GetUserByID(opts.UserID) + if err != nil { + log.Error("Unable to get User id %d Error: %v", opts.UserID, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err), + }) + return + } + perm, err := models.GetUserRepoPermission(repo, user) + if err != nil { + log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err), + }) + return } - canPush := false - if opts.IsDeployKey { - canPush = protectBranch.CanPush && (!protectBranch.EnableWhitelist || protectBranch.WhitelistDeployKeys) - } else { - canPush = protectBranch.CanUserPush(opts.UserID) + // Now check if the user is allowed to merge PRs for this repository + allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user) + if err != nil { + log.Error("Error calculating if allowed to merge: %v", err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Error calculating if allowed to merge: %v", err), + }) + return } - if !canPush && opts.ProtectedBranchID > 0 { - // Merge (from UI or API) - pr, err := models.GetPullRequestByID(opts.ProtectedBranchID) - if err != nil { - log.Error("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to get PullRequest %d Error: %v", opts.ProtectedBranchID, err), - }) - return - } - user, err := models.GetUserByID(opts.UserID) - if err != nil { - log.Error("Unable to get User id %d Error: %v", opts.UserID, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to get User id %d Error: %v", opts.UserID, err), - }) - return - } - perm, err := models.GetUserRepoPermission(repo, user) - if err != nil { - log.Error("Unable to get Repo permission of repo %s/%s of User %s", repo.OwnerName, repo.Name, user.Name, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to get Repo permission of repo %s/%s of User %s: %v", repo.OwnerName, repo.Name, user.Name, err), - }) - return - } - allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, perm, user) - if err != nil { - log.Error("Error calculating if allowed to merge: %v", err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Error calculating if allowed to merge: %v", err), - }) - return - } - if !allowedMerge { - log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v and is not allowed to merge pr #%d", opts.UserID, branchName, repo, pr.Index) + + if !allowedMerge { + log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v and is not allowed to merge pr #%d", opts.UserID, branchName, repo, pr.Index) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName), + }) + return + } + + // If we're an admin for the repository we can ignore status checks, reviews and override protected files + if perm.IsAdmin() { + continue + } + + // Now if we're not an admin - we can't overwrite protected files so fail now + if changedProtectedfiles { + log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath) + ctx.JSON(http.StatusForbidden, map[string]interface{}{ + "err": fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath), + }) + return + } + + // Check all status checks and reviews are ok + if err := pull_service.CheckPRReadyToMerge(pr, true); err != nil { + if models.IsErrNotAllowedToMerge(err) { + log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error()) ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName), + "err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()), }) return } - // Check all status checks and reviews is ok, unless repo admin which can bypass this. - if !perm.IsAdmin() { - if err := pull_service.CheckPRReadyToMerge(pr); err != nil { - if models.IsErrNotAllowedToMerge(err) { - log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", opts.UserID, branchName, repo, pr.Index, err.Error()) - ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, opts.ProtectedBranchID, err.Error()), - }) - return - } - log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err) - ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err), - }) - } - } - } else if !canPush { - log.Warn("Forbidden: User %d is not allowed to push to protected branch: %s in %-v", opts.UserID, branchName, repo) - ctx.JSON(http.StatusForbidden, map[string]interface{}{ - "err": fmt.Sprintf("Not allowed to push to protected branch %s", branchName), + log.Error("Unable to check if mergable: protected branch %s in %-v and pr #%d. Error: %v", opts.UserID, branchName, repo, pr.Index, err) + ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ + "err": fmt.Sprintf("Unable to get status of pull request %d. Error: %v", opts.ProtectedBranchID, err), }) return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index ef10651aa9..7977647097 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1426,6 +1426,9 @@ func ViewIssue(ctx *context.Context) { ctx.Data["IsBlockedByOutdatedBranch"] = pull.ProtectedBranch.MergeBlockedByOutdatedBranch(pull) ctx.Data["GrantedApprovals"] = cnt ctx.Data["RequireSigned"] = pull.ProtectedBranch.RequireSignedCommits + ctx.Data["ChangedProtectedFiles"] = pull.ChangedProtectedFiles + ctx.Data["IsBlockedByChangedProtectedFiles"] = len(pull.ChangedProtectedFiles) != 0 + ctx.Data["ChangedProtectedFilesNum"] = len(pull.ChangedProtectedFiles) } ctx.Data["WillSign"] = false if ctx.User != nil { diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 535bd0cdb5..54da7b76b7 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -624,6 +624,20 @@ func ViewPullFiles(ctx *context.Context) { return } + if err = pull.LoadProtectedBranch(); err != nil { + ctx.ServerError("LoadProtectedBranch", err) + return + } + + if pull.ProtectedBranch != nil { + glob := pull.ProtectedBranch.GetProtectedFilePatterns() + if len(glob) != 0 { + for _, file := range diff.Files { + file.IsProtected = pull.ProtectedBranch.IsProtectedFile(glob, file.Name) + } + } + } + ctx.Data["Diff"] = diff ctx.Data["DiffNotAvailable"] = diff.NumFiles == 0 @@ -772,7 +786,7 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) { return } - if err := pull_service.CheckPRReadyToMerge(pr); err != nil { + if err := pull_service.CheckPRReadyToMerge(pr, false); err != nil { if !models.IsErrNotAllowedToMerge(err) { ctx.ServerError("Merge PR status", err) return diff --git a/routers/repo/setting_protected_branch.go b/routers/repo/setting_protected_branch.go index ab0fd77eee..f864e8a75c 100644 --- a/routers/repo/setting_protected_branch.go +++ b/routers/repo/setting_protected_branch.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" + pull_service "code.gitea.io/gitea/services/pull" ) // ProtectedBranch render the page to protect the repository @@ -262,6 +263,10 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm) ctx.ServerError("UpdateProtectBranch", err) return } + if err = pull_service.CheckPrsForBaseBranch(ctx.Repo.Repository, protectBranch.BranchName); err != nil { + ctx.ServerError("CheckPrsForBaseBranch", err) + return + } ctx.Flash.Success(ctx.Tr("repo.settings.update_protect_branch_success", branch)) ctx.Redirect(fmt.Sprintf("%s/settings/branches/%s", ctx.Repo.RepoLink, branch)) } else { -- cgit v1.2.3