From 845ee7a15bc218e6bcd5de1e6545cd290689a5a8 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sat, 10 Jul 2021 12:52:22 +0200 Subject: [PATCH 1/6] Support for unprotected files --- integrations/git_test.go | 30 ++++++--- models/branches.go | 34 +++++++++- models/migrations/migrations.go | 2 + models/migrations/v188.go | 22 +++++++ modules/convert/convert.go | 1 + modules/repofiles/delete.go | 7 ++- modules/repofiles/update.go | 7 ++- modules/structs/repo_branch.go | 3 + options/locale/locale_en-US.ini | 2 + routers/api/v1/repo/branch.go | 5 ++ routers/private/hook.go | 17 +++++ routers/web/repo/setting_protected_branch.go | 1 + services/forms/repo_form.go | 1 + services/pull/patch.go | 63 +++++++++++++++++++ templates/repo/settings/protected_branch.tmpl | 5 ++ templates/swagger/v1_json.tmpl | 12 ++++ 16 files changed, 199 insertions(+), 13 deletions(-) create mode 100644 models/migrations/v188.go diff --git a/integrations/git_test.go b/integrations/git_test.go index a9848eaa4c309..678e73986f2fe 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -363,7 +363,7 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes t.Run("PushProtectedBranch", doGitPushTestRepository(dstPath, "origin", "protected")) ctx := NewAPITestContext(t, baseCtx.Username, baseCtx.Reponame) - t.Run("ProtectProtectedBranchNoWhitelist", doProtectBranch(ctx, "protected", "")) + t.Run("ProtectProtectedBranchNoWhitelist", doProtectBranch(ctx, "protected", "", "")) t.Run("GenerateCommit", func(t *testing.T) { _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "branch-data-file-") assert.NoError(t, err) @@ -389,7 +389,15 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes t.Run("MergePR2", doAPIMergePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, pr2.Index)) t.Run("MergePR", doAPIMergePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, pr.Index)) t.Run("PullProtected", doGitPull(dstPath, "origin", "protected")) - t.Run("ProtectProtectedBranchWhitelist", doProtectBranch(ctx, "protected", baseCtx.Username)) + + t.Run("ProtectProtectedBranchUnprotectedFilePaths", doProtectBranch(ctx, "protected", "", "unprotected-file-*")) + t.Run("GenerateCommit", func(t *testing.T) { + _, err := generateCommitWithNewData(littleSize, dstPath, "user2@example.com", "User Two", "unprotected-file-") + assert.NoError(t, err) + }) + t.Run("PushUnprotectedFilesToProtectedBranch", doGitPushTestRepository(dstPath, "origin", "protected")) + + t.Run("ProtectProtectedBranchWhitelist", doProtectBranch(ctx, "protected", baseCtx.Username, "")) t.Run("CheckoutMaster", doGitCheckoutBranch(dstPath, "master")) t.Run("CreateBranchForced", doGitCreateBranch(dstPath, "toforce")) @@ -404,7 +412,7 @@ func doBranchProtectPRMerge(baseCtx *APITestContext, dstPath string) func(t *tes } } -func doProtectBranch(ctx APITestContext, branch string, userToWhitelist string) func(t *testing.T) { +func doProtectBranch(ctx APITestContext, branch string, userToWhitelist string, unprotectedFilePatterns string) func(t *testing.T) { // We are going to just use the owner to set the protection. return func(t *testing.T) { csrf := GetCSRF(t, ctx.Session, fmt.Sprintf("/%s/%s/settings/branches", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame))) @@ -412,8 +420,9 @@ func doProtectBranch(ctx APITestContext, branch string, userToWhitelist string) if userToWhitelist == "" { // Change branch to protected req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/%s", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), url.PathEscape(branch)), map[string]string{ - "_csrf": csrf, - "protected": "on", + "_csrf": csrf, + "protected": "on", + "unprotected_file_patterns": unprotectedFilePatterns, }) ctx.Session.MakeRequest(t, req, http.StatusFound) } else { @@ -421,11 +430,12 @@ func doProtectBranch(ctx APITestContext, branch string, userToWhitelist string) assert.NoError(t, err) // Change branch to protected req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/settings/branches/%s", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), url.PathEscape(branch)), map[string]string{ - "_csrf": csrf, - "protected": "on", - "enable_push": "whitelist", - "enable_whitelist": "on", - "whitelist_users": strconv.FormatInt(user.ID, 10), + "_csrf": csrf, + "protected": "on", + "enable_push": "whitelist", + "enable_whitelist": "on", + "whitelist_users": strconv.FormatInt(user.ID, 10), + "unprotected_file_patterns": unprotectedFilePatterns, }) ctx.Session.MakeRequest(t, req, http.StatusFound) } diff --git a/models/branches.go b/models/branches.go index e13d84ee0524c..880cd42feb148 100644 --- a/models/branches.go +++ b/models/branches.go @@ -43,6 +43,7 @@ type ProtectedBranch struct { DismissStaleApprovals bool `xorm:"NOT NULL DEFAULT false"` RequireSignedCommits bool `xorm:"NOT NULL DEFAULT false"` ProtectedFilePatterns string `xorm:"TEXT"` + UnprotectedFilePatterns string `xorm:"TEXT"` CreatedUnix timeutil.TimeStamp `xorm:"created"` UpdatedUnix timeutil.TimeStamp `xorm:"updated"` @@ -214,8 +215,17 @@ func (protectBranch *ProtectedBranch) MergeBlockedByOutdatedBranch(pr *PullReque // GetProtectedFilePatterns parses a semicolon separated list of protected file patterns and returns a glob.Glob slice func (protectBranch *ProtectedBranch) GetProtectedFilePatterns() []glob.Glob { + return getFilePatterns(protectBranch.ProtectedFilePatterns) +} + +// GetUnprotectedFilePatterns parses a semicolon separated list of unprotected file patterns and returns a glob.Glob slice +func (protectBranch *ProtectedBranch) GetUnprotectedFilePatterns() []glob.Glob { + return getFilePatterns(protectBranch.UnprotectedFilePatterns) +} + +func getFilePatterns(filePatterns string) []glob.Glob { extarr := make([]glob.Glob, 0, 10) - for _, expr := range strings.Split(strings.ToLower(protectBranch.ProtectedFilePatterns), ";") { + for _, expr := range strings.Split(strings.ToLower(filePatterns), ";") { expr = strings.TrimSpace(expr) if expr != "" { if g, err := glob.Compile(expr, '.', '/'); err != nil { @@ -260,6 +270,28 @@ func (protectBranch *ProtectedBranch) IsProtectedFile(patterns []glob.Glob, path return r } +// IsUnprotectedFile return if path is unprotected +func (protectBranch *ProtectedBranch) IsUnprotectedFile(patterns []glob.Glob, path string) bool { + if len(patterns) == 0 { + patterns = protectBranch.GetUnprotectedFilePatterns() + if len(patterns) == 0 { + return false + } + } + + lpath := strings.ToLower(strings.TrimSpace(path)) + + r := false + for _, pat := range patterns { + if pat.Match(lpath) { + r = true + break + } + } + + return r +} + // GetProtectedBranchBy getting protected branch by ID/Name func GetProtectedBranchBy(repoID int64, branchName string) (*ProtectedBranch, error) { return getProtectedBranchBy(x, repoID, branchName) diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index de60d89bbe781..2a055e7073677 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -325,6 +325,8 @@ var migrations = []Migration{ NewMigration("Create protected tag table", createProtectedTagTable), // v187 -> v188 NewMigration("Drop unneeded webhook related columns", dropWebhookColumns), + // v188 -> v189 + NewMigration("Add Branch Protection Unprotected Files Column", addBranchProtectionUnprotectedFilesColumn), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v188.go b/models/migrations/v188.go new file mode 100644 index 0000000000000..7ea160e2b26bd --- /dev/null +++ b/models/migrations/v188.go @@ -0,0 +1,22 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "fmt" + + "xorm.io/xorm" +) + +func addBranchProtectionUnprotectedFilesColumn(x *xorm.Engine) error { + type ProtectedBranch struct { + UnprotectedFilePatterns string `xorm:"TEXT"` + } + + if err := x.Sync2(new(ProtectedBranch)); err != nil { + return fmt.Errorf("Sync2: %v", err) + } + return nil +} diff --git a/modules/convert/convert.go b/modules/convert/convert.go index 0b2135c5803dc..f4bd0597188c1 100644 --- a/modules/convert/convert.go +++ b/modules/convert/convert.go @@ -127,6 +127,7 @@ func ToBranchProtection(bp *models.ProtectedBranch) *api.BranchProtection { DismissStaleApprovals: bp.DismissStaleApprovals, RequireSignedCommits: bp.RequireSignedCommits, ProtectedFilePatterns: bp.ProtectedFilePatterns, + UnprotectedFilePatterns: bp.UnprotectedFilePatterns, Created: bp.CreatedUnix.AsTime(), Updated: bp.UpdatedUnix.AsTime(), } diff --git a/modules/repofiles/delete.go b/modules/repofiles/delete.go index 2b8ddf3cc266f..722a61de09d7c 100644 --- a/modules/repofiles/delete.go +++ b/modules/repofiles/delete.go @@ -62,7 +62,12 @@ func DeleteRepoFile(repo *models.Repository, doer *models.User, opts *DeleteRepo return nil, err } if protectedBranch != nil { - if !protectedBranch.CanUserPush(doer.ID) { + isUnprotectedFile := false + glob := protectedBranch.GetUnprotectedFilePatterns() + if len(glob) != 0 { + isUnprotectedFile = protectedBranch.IsUnprotectedFile(glob, opts.TreePath) + } + if !protectedBranch.CanUserPush(doer.ID) && !isUnprotectedFile { return nil, models.ErrUserCannotCommit{ UserName: doer.LowerName, } diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index ad984c465ae86..790f3a8499fc2 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -154,7 +154,12 @@ func CreateOrUpdateRepoFile(repo *models.Repository, doer *models.User, opts *Up return nil, err } if protectedBranch != nil { - if !protectedBranch.CanUserPush(doer.ID) { + isUnprotectedFile := false + glob := protectedBranch.GetUnprotectedFilePatterns() + if len(glob) != 0 { + isUnprotectedFile = protectedBranch.IsUnprotectedFile(glob, opts.TreePath) + } + if !protectedBranch.CanUserPush(doer.ID) && !isUnprotectedFile { return nil, models.ErrUserCannotCommit{ UserName: doer.LowerName, } diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index efb4caecb0827..1f3bc04e86401 100644 --- a/modules/structs/repo_branch.go +++ b/modules/structs/repo_branch.go @@ -44,6 +44,7 @@ type BranchProtection struct { DismissStaleApprovals bool `json:"dismiss_stale_approvals"` RequireSignedCommits bool `json:"require_signed_commits"` ProtectedFilePatterns string `json:"protected_file_patterns"` + UnprotectedFilePatterns string `json:"unprotected_file_patterns"` // swagger:strfmt date-time Created time.Time `json:"created_at"` // swagger:strfmt date-time @@ -73,6 +74,7 @@ type CreateBranchProtectionOption struct { DismissStaleApprovals bool `json:"dismiss_stale_approvals"` RequireSignedCommits bool `json:"require_signed_commits"` ProtectedFilePatterns string `json:"protected_file_patterns"` + UnprotectedFilePatterns string `json:"unprotected_file_patterns"` } // EditBranchProtectionOption options for editing a branch protection @@ -97,4 +99,5 @@ type EditBranchProtectionOption struct { DismissStaleApprovals *bool `json:"dismiss_stale_approvals"` RequireSignedCommits *bool `json:"require_signed_commits"` ProtectedFilePatterns *string `json:"protected_file_patterns"` + UnprotectedFilePatterns *string `json:"unprotected_file_patterns"` } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 191cb5de6764c..8876b5a30dd9e 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1886,6 +1886,8 @@ settings.require_signed_commits = Require Signed Commits settings.require_signed_commits_desc = Reject pushes to this branch if they are unsigned or unverifiable. settings.protect_protected_file_patterns = Protected file patterns (separated using semicolon '\;'): settings.protect_protected_file_patterns_desc = Protected files that are not allowed to be changed directly even if user has rights to add, edit, or delete files in this branch. Multiple patterns can be separated using semicolon ('\;'). See github.com/gobwas/glob documentation for pattern syntax. Examples: .drone.yml, /docs/**/*.txt. +settings.protect_unprotected_file_patterns = Unprotected file patterns (separated using semicolon '\;'): +settings.protect_unprotected_file_patterns_desc = Unprotected files that are allowed to be changed directly if user has write access, bypassing push restriction. Multiple patterns can be separated using semicolon ('\;'). See github.com/gobwas/glob documentation for pattern syntax. Examples: .drone.yml, /docs/**/*.txt. settings.add_protected_branch = Enable protection settings.delete_protected_branch = Disable protection settings.update_protect_branch_success = Branch protection for branch '%s' has been updated. diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 85c1681dfec1b..ac4212579758a 100644 --- a/routers/api/v1/repo/branch.go +++ b/routers/api/v1/repo/branch.go @@ -499,6 +499,7 @@ func CreateBranchProtection(ctx *context.APIContext) { DismissStaleApprovals: form.DismissStaleApprovals, RequireSignedCommits: form.RequireSignedCommits, ProtectedFilePatterns: form.ProtectedFilePatterns, + UnprotectedFilePatterns: form.UnprotectedFilePatterns, BlockOnOutdatedBranch: form.BlockOnOutdatedBranch, } @@ -644,6 +645,10 @@ func EditBranchProtection(ctx *context.APIContext) { protectBranch.ProtectedFilePatterns = *form.ProtectedFilePatterns } + if form.UnprotectedFilePatterns != nil { + protectBranch.UnprotectedFilePatterns = *form.UnprotectedFilePatterns + } + if form.BlockOnOutdatedBranch != nil { protectBranch.BlockOnOutdatedBranch = *form.BlockOnOutdatedBranch } diff --git a/routers/private/hook.go b/routers/private/hook.go index 9f5579b6ae688..205ac0573bcdc 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -292,6 +292,23 @@ func HookPreReceive(ctx *gitea_context.PrivateContext) { return } + // Allow commits that only touch unprotected files + globs := protectBranch.GetUnprotectedFilePatterns() + if len(globs) > 0 { + unprotectedFilesOnly, err := pull_service.CheckUnprotectedFiles(oldCommitID, newCommitID, globs, env, gitRepo) + if err != nil { + log.Error("Unable to check file protection for commits from %s to %s in %-v: %v", oldCommitID, newCommitID, repo, err) + ctx.JSON(http.StatusInternalServerError, private.Response{ + Err: fmt.Sprintf("Unable to check file protection for commits from %s to %s: %v", oldCommitID, newCommitID, err), + }) + return + } + if unprotectedFilesOnly { + // Commit only touches unprotected files, this is allowed + continue + } + } + // 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, private.Response{ diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index fba2c095cf936..5e4cf87a98a82 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -253,6 +253,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { protectBranch.DismissStaleApprovals = f.DismissStaleApprovals protectBranch.RequireSignedCommits = f.RequireSignedCommits protectBranch.ProtectedFilePatterns = f.ProtectedFilePatterns + protectBranch.UnprotectedFilePatterns = f.UnprotectedFilePatterns protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{ diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 71a83a8be36e2..f77cc3834df45 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -198,6 +198,7 @@ type ProtectBranchForm struct { DismissStaleApprovals bool RequireSignedCommits bool ProtectedFilePatterns string + UnprotectedFilePatterns string } // Validate validates the fields diff --git a/services/pull/patch.go b/services/pull/patch.go index 72b459bf2cb3d..36d703c61cdc2 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -309,6 +309,69 @@ func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, return changedProtectedFiles, err } +// CheckUnprotectedFiles check if the commit only touches unprotected files +func CheckUnprotectedFiles(oldCommitID, newCommitID string, patterns []glob.Glob, env []string, repo *git.Repository) (bool, error) { + // 1. If there are no patterns short-circuit and just return false + if len(patterns) == 0 { + return false, nil + } + + // 2. Prep the pipe + stdoutReader, stdoutWriter, err := os.Pipe() + if err != nil { + log.Error("Unable to create os.Pipe for %s", repo.Path) + return false, err + } + defer func() { + _ = stdoutReader.Close() + _ = stdoutWriter.Close() + }() + + unprotectedFilesOnly := true + + // 3. Run `git diff --name-only` to get the names of the changed files + err = git.NewCommand("diff", "--name-only", oldCommitID, newCommitID). + RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, + stdoutWriter, nil, nil, + func(ctx context.Context, cancel context.CancelFunc) error { + // Close the writer end of the pipe to begin processing + _ = stdoutWriter.Close() + defer func() { + // Close the reader on return to terminate the git command if necessary + _ = stdoutReader.Close() + }() + + // Now scan the output from the command + scanner := bufio.NewScanner(stdoutReader) + for scanner.Scan() { + path := strings.TrimSpace(scanner.Text()) + if len(path) == 0 { + continue + } + lpath := strings.ToLower(path) + unprotected := false + for _, pat := range patterns { + if pat.Match(lpath) { + unprotected = true + break + } + } + if !unprotected { + unprotectedFilesOnly = false + break + } + } + + return scanner.Err() + }) + // 4. log errors if there are any... + if err != nil { + log.Error("Unable to check file protection for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err) + } + + return unprotectedFilesOnly, err +} + // checkPullFilesProtection check if pr changed protected files and save results func checkPullFilesProtection(pr *models.PullRequest, gitRepo *git.Repository) error { if err := pr.LoadProtectedBranch(); err != nil { diff --git a/templates/repo/settings/protected_branch.tmpl b/templates/repo/settings/protected_branch.tmpl index f6ecd67fd2159..597567f057aa5 100644 --- a/templates/repo/settings/protected_branch.tmpl +++ b/templates/repo/settings/protected_branch.tmpl @@ -244,6 +244,11 @@

{{.i18n.Tr "repo.settings.protect_protected_file_patterns_desc" | Safe}}

+
+ + +

{{.i18n.Tr "repo.settings.protect_unprotected_file_patterns_desc" | Safe}}

+
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 669e3552cc5de..420f2ec250f13 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -12247,6 +12247,10 @@ "type": "string", "x-go-name": "ProtectedFilePatterns" }, + "unprotected_file_patterns": { + "type": "string", + "x-go-name": "UnprotectedFilePatterns" + }, "push_whitelist_deploy_keys": { "type": "boolean", "x-go-name": "PushWhitelistDeployKeys" @@ -12684,6 +12688,10 @@ "type": "string", "x-go-name": "ProtectedFilePatterns" }, + "unprotected_file_patterns": { + "type": "string", + "x-go-name": "UnprotectedFilePatterns" + }, "push_whitelist_deploy_keys": { "type": "boolean", "x-go-name": "PushWhitelistDeployKeys" @@ -13681,6 +13689,10 @@ "type": "string", "x-go-name": "ProtectedFilePatterns" }, + "unprotected_file_patterns": { + "type": "string", + "x-go-name": "UnprotectedFilePatterns" + }, "push_whitelist_deploy_keys": { "type": "boolean", "x-go-name": "PushWhitelistDeployKeys" From db448dab9d458295808a83b336721112b765ed97 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sun, 11 Jul 2021 00:41:44 +0200 Subject: [PATCH 2/6] Reuse duplicated code --- modules/repofiles/delete.go | 38 +----------------- modules/repofiles/update.go | 78 ++++++++++++++++++++----------------- 2 files changed, 44 insertions(+), 72 deletions(-) diff --git a/modules/repofiles/delete.go b/modules/repofiles/delete.go index 722a61de09d7c..5ae418b7f62a1 100644 --- a/modules/repofiles/delete.go +++ b/modules/repofiles/delete.go @@ -56,42 +56,8 @@ func DeleteRepoFile(repo *models.Repository, doer *models.User, opts *DeleteRepo BranchName: opts.NewBranch, } } - } else { - protectedBranch, err := repo.GetBranchProtection(opts.OldBranch) - if err != nil { - return nil, err - } - if protectedBranch != nil { - isUnprotectedFile := false - glob := protectedBranch.GetUnprotectedFilePatterns() - if len(glob) != 0 { - isUnprotectedFile = protectedBranch.IsUnprotectedFile(glob, opts.TreePath) - } - if !protectedBranch.CanUserPush(doer.ID) && !isUnprotectedFile { - return nil, models.ErrUserCannotCommit{ - UserName: doer.LowerName, - } - } - if protectedBranch.RequireSignedCommits { - _, _, _, err := repo.SignCRUDAction(doer, repo.RepoPath(), opts.OldBranch) - if err != nil { - if !models.IsErrWontSign(err) { - return nil, err - } - return nil, models.ErrUserCannotCommit{ - UserName: doer.LowerName, - } - } - } - patterns := protectedBranch.GetProtectedFilePatterns() - for _, pat := range patterns { - if pat.Match(strings.ToLower(opts.TreePath)) { - return nil, models.ErrFilePathProtected{ - Path: opts.TreePath, - } - } - } - } + } else if err := VerifyBranchProtection(repo, doer, opts.OldBranch, opts.TreePath); err != nil { + return nil, err } // Check that the path given in opts.treeName is valid (not a git path) diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 790f3a8499fc2..dc2893cb1c350 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -148,42 +148,8 @@ func CreateOrUpdateRepoFile(repo *models.Repository, doer *models.User, opts *Up if err != nil && !git.IsErrBranchNotExist(err) { return nil, err } - } else { - protectedBranch, err := repo.GetBranchProtection(opts.OldBranch) - if err != nil { - return nil, err - } - if protectedBranch != nil { - isUnprotectedFile := false - glob := protectedBranch.GetUnprotectedFilePatterns() - if len(glob) != 0 { - isUnprotectedFile = protectedBranch.IsUnprotectedFile(glob, opts.TreePath) - } - if !protectedBranch.CanUserPush(doer.ID) && !isUnprotectedFile { - return nil, models.ErrUserCannotCommit{ - UserName: doer.LowerName, - } - } - if protectedBranch.RequireSignedCommits { - _, _, _, err := repo.SignCRUDAction(doer, repo.RepoPath(), opts.OldBranch) - if err != nil { - if !models.IsErrWontSign(err) { - return nil, err - } - return nil, models.ErrUserCannotCommit{ - UserName: doer.LowerName, - } - } - } - patterns := protectedBranch.GetProtectedFilePatterns() - for _, pat := range patterns { - if pat.Match(strings.ToLower(opts.TreePath)) { - return nil, models.ErrFilePathProtected{ - Path: opts.TreePath, - } - } - } - } + } else if err := VerifyBranchProtection(repo, doer, opts.OldBranch, opts.TreePath); err != nil { + return nil, err } // If FromTreePath is not set, set it to the opts.TreePath @@ -470,3 +436,43 @@ func CreateOrUpdateRepoFile(repo *models.Repository, doer *models.User, opts *Up } return file, nil } + +// VerifyBranchProtection verify the branch protection for modifying the given treePath on the given branch +func VerifyBranchProtection(repo *models.Repository, doer *models.User, branchName string, treePath string) error { + protectedBranch, err := repo.GetBranchProtection(branchName) + if err != nil { + return err + } + if protectedBranch != nil { + isUnprotectedFile := false + glob := protectedBranch.GetUnprotectedFilePatterns() + if len(glob) != 0 { + isUnprotectedFile = protectedBranch.IsUnprotectedFile(glob, treePath) + } + if !protectedBranch.CanUserPush(doer.ID) && !isUnprotectedFile { + return models.ErrUserCannotCommit{ + UserName: doer.LowerName, + } + } + if protectedBranch.RequireSignedCommits { + _, _, _, err := repo.SignCRUDAction(doer, repo.RepoPath(), branchName) + if err != nil { + if !models.IsErrWontSign(err) { + return err + } + return models.ErrUserCannotCommit{ + UserName: doer.LowerName, + } + } + } + patterns := protectedBranch.GetProtectedFilePatterns() + for _, pat := range patterns { + if pat.Match(strings.ToLower(treePath)) { + return models.ErrFilePathProtected{ + Path: treePath, + } + } + } + } + return nil +} From 2900262773c9114bf00a4a440dadd43712f5f824 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sun, 11 Jul 2021 12:00:32 +0200 Subject: [PATCH 3/6] generate-swagger --- templates/swagger/v1_json.tmpl | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 420f2ec250f13..1d007814b00cd 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -12247,10 +12247,6 @@ "type": "string", "x-go-name": "ProtectedFilePatterns" }, - "unprotected_file_patterns": { - "type": "string", - "x-go-name": "UnprotectedFilePatterns" - }, "push_whitelist_deploy_keys": { "type": "boolean", "x-go-name": "PushWhitelistDeployKeys" @@ -12285,6 +12281,10 @@ }, "x-go-name": "StatusCheckContexts" }, + "unprotected_file_patterns": { + "type": "string", + "x-go-name": "UnprotectedFilePatterns" + }, "updated_at": { "type": "string", "format": "date-time", @@ -12688,10 +12688,6 @@ "type": "string", "x-go-name": "ProtectedFilePatterns" }, - "unprotected_file_patterns": { - "type": "string", - "x-go-name": "UnprotectedFilePatterns" - }, "push_whitelist_deploy_keys": { "type": "boolean", "x-go-name": "PushWhitelistDeployKeys" @@ -12725,6 +12721,10 @@ "type": "string" }, "x-go-name": "StatusCheckContexts" + }, + "unprotected_file_patterns": { + "type": "string", + "x-go-name": "UnprotectedFilePatterns" } }, "x-go-package": "code.gitea.io/gitea/modules/structs" @@ -13689,10 +13689,6 @@ "type": "string", "x-go-name": "ProtectedFilePatterns" }, - "unprotected_file_patterns": { - "type": "string", - "x-go-name": "UnprotectedFilePatterns" - }, "push_whitelist_deploy_keys": { "type": "boolean", "x-go-name": "PushWhitelistDeployKeys" @@ -13726,6 +13722,10 @@ "type": "string" }, "x-go-name": "StatusCheckContexts" + }, + "unprotected_file_patterns": { + "type": "string", + "x-go-name": "UnprotectedFilePatterns" } }, "x-go-package": "code.gitea.io/gitea/modules/structs" From 51f2a2502cd79e9e6448a8b02526bc1637b84c0e Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sat, 11 Sep 2021 13:36:17 +0200 Subject: [PATCH 4/6] Extract getAffectedFiles method to remove duplicated code --- services/pull/patch.go | 118 +++++++++++++++-------------------------- 1 file changed, 43 insertions(+), 75 deletions(-) diff --git a/services/pull/patch.go b/services/pull/patch.go index 36d703c61cdc2..74ae8c943946b 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -245,91 +245,73 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath // CheckFileProtection check file Protection func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, limit int, env []string, repo *git.Repository) ([]string, error) { - // 1. If there are no patterns short-circuit and just return nil if len(patterns) == 0 { return nil, nil } - - // 2. Prep the pipe - stdoutReader, stdoutWriter, err := os.Pipe() + affectedFiles, err := getAffectedFiles(oldCommitID, newCommitID, env, repo) if err != nil { - log.Error("Unable to create os.Pipe for %s", repo.Path) return nil, err } - defer func() { - _ = stdoutReader.Close() - _ = stdoutWriter.Close() - }() - changedProtectedFiles := make([]string, 0, limit) - - // 3. Run `git diff --name-only` to get the names of the changed files - err = git.NewCommand("diff", "--name-only", oldCommitID, newCommitID). - RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, - stdoutWriter, nil, nil, - func(ctx context.Context, cancel context.CancelFunc) error { - // Close the writer end of the pipe to begin processing - _ = stdoutWriter.Close() - defer func() { - // Close the reader on return to terminate the git command if necessary - _ = stdoutReader.Close() - }() - - // Now scan the output from the command - 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) { - changedProtectedFiles = append(changedProtectedFiles, path) - break - } - } - if len(changedProtectedFiles) >= limit { - break - } - } - - if len(changedProtectedFiles) > 0 { - return models.ErrFilePathProtected{ - Path: changedProtectedFiles[0], - } - } - return scanner.Err() - }) - // 4. log real errors if there are any... - 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) + for _, affectedFile := range affectedFiles { + lpath := strings.ToLower(affectedFile) + for _, pat := range patterns { + if pat.Match(lpath) { + changedProtectedFiles = append(changedProtectedFiles, lpath) + break + } + } + if len(changedProtectedFiles) >= limit { + break + } + } + if len(changedProtectedFiles) > 0 { + err = models.ErrFilePathProtected{ + Path: changedProtectedFiles[0], + } } - return changedProtectedFiles, err } // CheckUnprotectedFiles check if the commit only touches unprotected files func CheckUnprotectedFiles(oldCommitID, newCommitID string, patterns []glob.Glob, env []string, repo *git.Repository) (bool, error) { - // 1. If there are no patterns short-circuit and just return false if len(patterns) == 0 { return false, nil } + affectedFiles, err := getAffectedFiles(oldCommitID, newCommitID, env, repo) + if err != nil { + return false, err + } + for _, affectedFile := range affectedFiles { + lpath := strings.ToLower(affectedFile) + unprotected := false + for _, pat := range patterns { + if pat.Match(lpath) { + unprotected = true + break + } + } + if !unprotected { + return false, nil + } + } + return true, nil +} - // 2. Prep the pipe +func getAffectedFiles(oldCommitID, newCommitID string, env []string, repo *git.Repository) ([]string, error) { stdoutReader, stdoutWriter, err := os.Pipe() if err != nil { log.Error("Unable to create os.Pipe for %s", repo.Path) - return false, err + return nil, err } defer func() { _ = stdoutReader.Close() _ = stdoutWriter.Close() }() - unprotectedFilesOnly := true + affectedFiles := make([]string, 0, 32) - // 3. Run `git diff --name-only` to get the names of the changed files + // Run `git diff --name-only` to get the names of the changed files err = git.NewCommand("diff", "--name-only", oldCommitID, newCommitID). RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, stdoutWriter, nil, nil, @@ -340,7 +322,6 @@ func CheckUnprotectedFiles(oldCommitID, newCommitID string, patterns []glob.Glob // Close the reader on return to terminate the git command if necessary _ = stdoutReader.Close() }() - // Now scan the output from the command scanner := bufio.NewScanner(stdoutReader) for scanner.Scan() { @@ -348,28 +329,15 @@ func CheckUnprotectedFiles(oldCommitID, newCommitID string, patterns []glob.Glob if len(path) == 0 { continue } - lpath := strings.ToLower(path) - unprotected := false - for _, pat := range patterns { - if pat.Match(lpath) { - unprotected = true - break - } - } - if !unprotected { - unprotectedFilesOnly = false - break - } + affectedFiles = append(affectedFiles, path) } - return scanner.Err() }) - // 4. log errors if there are any... if err != nil { - log.Error("Unable to check file protection for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err) + log.Error("Unable to get affected files for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err) } - return unprotectedFilesOnly, err + return affectedFiles, err } // checkPullFilesProtection check if pr changed protected files and save results From 2613ef97f053cdeb5d55631df4dc02b51ab1aece Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sat, 11 Sep 2021 14:22:30 +0200 Subject: [PATCH 5/6] Move GetAffectedFiles to modules/git --- modules/git/diff.go | 44 ++++++++++++++++++++++++++++++++++++++++ services/pull/patch.go | 46 ++---------------------------------------- 2 files changed, 46 insertions(+), 44 deletions(-) diff --git a/modules/git/diff.go b/modules/git/diff.go index 20f25c1bee3f5..df79d053436ab 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -10,6 +10,7 @@ import ( "context" "fmt" "io" + "os" "os/exec" "regexp" "strconv" @@ -273,3 +274,46 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi oldBegin, oldNumOfLines, newBegin, newNumOfLines) return strings.Join(newHunk, "\n"), nil } + +// Get the affected files between two commits +func GetAffectedFiles(oldCommitID, newCommitID string, env []string, repo *Repository) ([]string, error) { + stdoutReader, stdoutWriter, err := os.Pipe() + if err != nil { + log.Error("Unable to create os.Pipe for %s", repo.Path) + return nil, err + } + defer func() { + _ = stdoutReader.Close() + _ = stdoutWriter.Close() + }() + + affectedFiles := make([]string, 0, 32) + + // Run `git diff --name-only` to get the names of the changed files + err = NewCommand("diff", "--name-only", oldCommitID, newCommitID). + RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, + stdoutWriter, nil, nil, + func(ctx context.Context, cancel context.CancelFunc) error { + // Close the writer end of the pipe to begin processing + _ = stdoutWriter.Close() + defer func() { + // Close the reader on return to terminate the git command if necessary + _ = stdoutReader.Close() + }() + // Now scan the output from the command + scanner := bufio.NewScanner(stdoutReader) + for scanner.Scan() { + path := strings.TrimSpace(scanner.Text()) + if len(path) == 0 { + continue + } + affectedFiles = append(affectedFiles, path) + } + return scanner.Err() + }) + if err != nil { + log.Error("Unable to get affected files for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err) + } + + return affectedFiles, err +} diff --git a/services/pull/patch.go b/services/pull/patch.go index 74ae8c943946b..73b979f647be1 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -248,7 +248,7 @@ func CheckFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob, if len(patterns) == 0 { return nil, nil } - affectedFiles, err := getAffectedFiles(oldCommitID, newCommitID, env, repo) + affectedFiles, err := git.GetAffectedFiles(oldCommitID, newCommitID, env, repo) if err != nil { return nil, err } @@ -278,7 +278,7 @@ func CheckUnprotectedFiles(oldCommitID, newCommitID string, patterns []glob.Glob if len(patterns) == 0 { return false, nil } - affectedFiles, err := getAffectedFiles(oldCommitID, newCommitID, env, repo) + affectedFiles, err := git.GetAffectedFiles(oldCommitID, newCommitID, env, repo) if err != nil { return false, err } @@ -298,48 +298,6 @@ func CheckUnprotectedFiles(oldCommitID, newCommitID string, patterns []glob.Glob return true, nil } -func getAffectedFiles(oldCommitID, newCommitID string, env []string, repo *git.Repository) ([]string, error) { - stdoutReader, stdoutWriter, err := os.Pipe() - if err != nil { - log.Error("Unable to create os.Pipe for %s", repo.Path) - return nil, err - } - defer func() { - _ = stdoutReader.Close() - _ = stdoutWriter.Close() - }() - - affectedFiles := make([]string, 0, 32) - - // Run `git diff --name-only` to get the names of the changed files - err = git.NewCommand("diff", "--name-only", oldCommitID, newCommitID). - RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, - stdoutWriter, nil, nil, - func(ctx context.Context, cancel context.CancelFunc) error { - // Close the writer end of the pipe to begin processing - _ = stdoutWriter.Close() - defer func() { - // Close the reader on return to terminate the git command if necessary - _ = stdoutReader.Close() - }() - // Now scan the output from the command - scanner := bufio.NewScanner(stdoutReader) - for scanner.Scan() { - path := strings.TrimSpace(scanner.Text()) - if len(path) == 0 { - continue - } - affectedFiles = append(affectedFiles, path) - } - return scanner.Err() - }) - if err != nil { - log.Error("Unable to get affected files for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err) - } - - return affectedFiles, err -} - // checkPullFilesProtection check if pr changed protected files and save results func checkPullFilesProtection(pr *models.PullRequest, gitRepo *git.Repository) error { if err := pr.LoadProtectedBranch(); err != nil { From 3a2572e53afb8de3b93addba475ffb86c6cfa8d4 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sat, 11 Sep 2021 14:25:54 +0200 Subject: [PATCH 6/6] Fix lint error --- modules/git/diff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/diff.go b/modules/git/diff.go index df79d053436ab..b473dc73f60e8 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -275,7 +275,7 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi return strings.Join(newHunk, "\n"), nil } -// Get the affected files between two commits +// GetAffectedFiles returns the affected files between two commits func GetAffectedFiles(oldCommitID, newCommitID string, env []string, repo *Repository) ([]string, error) { stdoutReader, stdoutWriter, err := os.Pipe() if err != nil {