Added checks for protected branches in pull requests #3544

Chri-s · 2018-02-19T19:29:11Z

Fixes #2875. This pull request adds checks whether the user is allowed to push to the base branch of a pull request.

If a user may not push to the base branch, the green merge button is hidden on the pull request view. If the user tries merge via the web api, an error (http code 500) with
{"message":"branch is protected [name: %s]","url":"https://godoc.org/github.com/go-gitea/go-sdk/gitea"}
is returned.

There shouldn't be a way to circumvent the branch protection via a pull request.

Signed-off-by: Christian Wulff <NChris@posteo.net>

codecov-io · 2018-02-19T19:38:20Z

Codecov Report

Merging #3544 into master will decrease coverage by 0.02%.
The diff coverage is 12.12%.

@@            Coverage Diff             @@
##           master    #3544      +/-   ##
==========================================
- Coverage   35.88%   35.86%   -0.03%     
==========================================
  Files         287      287              
  Lines       41331    41362      +31     
==========================================
+ Hits        14833    14835       +2     
- Misses      24316    24339      +23     
- Partials     2182     2188       +6

Impacted Files	Coverage Δ
models/error.go	`32.65% <0%> (-0.48%)`	⬇️
routers/repo/issue.go	`33.09% <14.28%> (-0.18%)`	⬇️
models/pull.go	`51.2% <14.28%> (-0.97%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0d41b1...fbf9bf2. Read the comment docs.

lafriks · 2018-02-19T21:05:54Z

models/error.go

 }

+// ErrBranchProtected represents an error that a branch is protected and the current user is not allowed to modify it
+type ErrBranchProtected struct {


ErrNotAllowedToMerge would be better name for this error

lafriks · 2018-02-19T21:09:46Z

models/pull.go

 	}
 	prConfig := prUnit.PullRequestsConfig()

+	if protected, err := pr.BaseRepo.IsProtectedBranch(pr.BaseBranch, doer); err != nil {


Would be better to create new function pr.CheckUserAllowedToMerge that would return error (nil for allowed to merge)

lafriks · 2018-02-19T21:10:44Z

routers/repo/issue.go

 		}
 		prConfig := prUnit.PullRequestsConfig()

+		if ctx.IsSigned {


Reuse function pull.CheckUserAllowedToMerge here

lafriks · 2018-02-19T21:12:37Z

routers/repo/issue.go


+		if ctx.IsSigned {
+			if err := pull.GetBaseRepo(); err != nil {
+				log.Error(4, "GetBaseRepo: %v", err)


in such cases ctx.ServerError(... should be used

lafriks · 2018-02-19T21:15:20Z

routers/repo/issue.go

+					log.Error(4, "IsProtectedBranch: %v", err)
+					ctx.Data["BaseBranchNotProtected"] = false
+				} else {
+					ctx.Data["BaseBranchNotProtected"] = !protected


Reuse IsRepositoryWriter && CheckUserAllowedToMerge to assign to something like ctx.Data["AllowMerge"] and check that in pull.tmpl with {{if .AllowMerge}}

lafriks · 2018-02-19T21:18:47Z

With moving that check to function would allow to reuse that function to add additional conditions for merge limitations

…Merge Signed-off-by: Christian Wulff <NChris@posteo.net>

Signed-off-by: Christian Wulff <NChris@posteo.net>

Chri-s · 2018-02-20T00:05:20Z

Sorry, had a problem with the first commit after your change request :( I hope the last commit meets your requirements.

lafriks · 2018-02-20T01:07:18Z

models/pull.go

+			"Not signed in",
+		}
+	}
+	if err = pr.GetBaseRepo(); err != nil {


No need to load repository if it is already loaded (need check if pr.BaseRepo == nil {)

I added it, but GetBaseRepo() already checks for this:

func (pr *PullRequest) GetBaseRepo() (err error) { if pr.BaseRepo != nil { return nil }

Sorry I overlooked that function than... I was checking code but somehow missed that check. If it does than yes there is no need to do additional check

lafriks · 2018-02-20T01:15:57Z

routers/repo/issue.go

 		}
 		prConfig := prUnit.PullRequestsConfig()

+		if err := pull.CheckUserAllowedToMerge(ctx.User); err != nil {


This could be rewritten as:

ctx.Data["AllowMerge"] = ctx.Data["IsRepositoryWriter"] if err := pull.CheckUserAllowedToMerge(ctx.User); err != nil { if !models.IsErrNotAllowedToMerge(err) { ctx.ServerError(err) return } ctx.Data["AllowMerge"] = false }

Chri-s · 2018-02-20T13:38:26Z

The CI build failed because of a git error (https://drone.gitea.io/go-gitea/gitea/3754/2):

+ git init
Initialized empty Git repository in /srv/app/src/code.gitea.io/gitea/.git/
+ git remote add origin https://github.com/go-gitea/gitea.git
+ git fetch --tags --depth=50 origin +refs/pull/3544/merge:
fatal: The remote end hung up unexpectedly
exit status 128

I don't think that this has something to do with my latest commit, is there some way to tell drone to retry?

Signed-off-by: Christian Wulff <NChris@posteo.net>

lunny · 2018-02-22T06:10:30Z

LGTM

lunny · 2018-03-03T15:31:53Z

need @lafriks 's review.

Added checks for protected branches in pull requests

fed2b26

Signed-off-by: Christian Wulff <NChris@posteo.net>

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 19, 2018

lafriks requested changes Feb 19, 2018

View reviewed changes

lafriks added the type/bug label Feb 19, 2018

Christian Wulff and others added 2 commits February 20, 2018 00:49

Moved check for protected branch into new function CheckUserAllowedTo…

1c46c68

…Merge Signed-off-by: Christian Wulff <NChris@posteo.net>

Removed merge conflict lines from last commit

c71f689

Signed-off-by: Christian Wulff <NChris@posteo.net>

lafriks requested changes Feb 20, 2018

View reviewed changes

lunny added this to the 1.x.x milestone Feb 20, 2018

Explicit check for error type in ViewIssue

ebaa008

Signed-off-by: Christian Wulff <NChris@posteo.net>

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 22, 2018

appleboy approved these changes Feb 22, 2018

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 22, 2018

appleboy modified the milestones: 1.x.x, 1.5.0 Feb 22, 2018

lafriks approved these changes Mar 4, 2018

View reviewed changes

lafriks and others added 6 commits March 4, 2018 23:57

Merge branch 'master' into master

cdbfa25

Merge branch 'master' into master

8335097

Merge branch 'master' into master

a1ee345

Merge branch 'master' into master

de829b0

Merge branch 'master' into master

116b91d

Merge branch 'master' into master

fbf9bf2

lunny merged commit a2a49c9 into go-gitea:master Mar 13, 2018

lunny mentioned this pull request Mar 15, 2018

No button to merge PR #3668

Closed

7 tasks

Chri-s mentioned this pull request Mar 18, 2018

Add protected branch whitelists for merging #3689

Merged

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Uh oh!

Uh oh!

Added checks for protected branches in pull requests #3544

Added checks for protected branches in pull requests #3544

Uh oh!

Conversation

Chri-s commented Feb 19, 2018

Uh oh!

codecov-io commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lafriks commented Feb 19, 2018

Uh oh!

Chri-s commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chri-s commented Feb 20, 2018

Uh oh!

lunny commented Feb 22, 2018

Uh oh!

lunny commented Mar 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Feb 19, 2018 •

edited

Loading