Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[suggest] change merge strategy: do not check write access if user in merge white list #10935

Closed
3 of 7 tasks
everhopingandwaiting opened this issue Apr 3, 2020 · 2 comments · Fixed by #10951
Closed
3 of 7 tasks
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@everhopingandwaiting
Copy link
Contributor

everhopingandwaiting commented Apr 3, 2020

  • Gitea version (or commit ref): 1.12.0+dev-82-g3723b0647
  • Git version: 2.24
  • Operating system: arch linux
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • Can you reproduce the bug at https://try.gitea.io:
    • Yes (provide example URL)
    • No
    • Not relevant
  • Log gist:

Description

func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *models.User) (bool, error) {

before

// IsUserAllowedToMerge check if user is allowed to merge PR with given permissions and branch protections
func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *models.User) (bool, error) {
	if !p.CanWrite(models.UnitTypeCode) {
		return false, nil
	}

	err := pr.LoadProtectedBranch()
	if err != nil {
		return false, err
	}

	if pr.ProtectedBranch == nil || pr.ProtectedBranch.IsUserMergeWhitelisted(user.ID) {
		return true, nil
	}

	return false, nil
}

atter:

// IsUserAllowedToMerge check if user is allowed to merge PR with given permissions and branch protections
func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *models.User) (bool, error) {
	//if !p.CanWrite(models.UnitTypeCode) {
	//	return false, nil
	//}

	err := pr.LoadProtectedBranch()
	if err != nil {
		return false, err
	}

	if (p.CanWrite(models.UnitTypeCode) && pr.ProtectedBranch == nil) || pr.ProtectedBranch.IsUserMergeWhitelisted(user.ID) {
		return true, nil
	}

	return false, nil
}

remove Route verification

m.Post("/merge", context.RepoMustNotBeArchived(), /*reqRepoPullsWriter,*/ bindIgnErr(auth.MergePullRequestForm{}), repo.MergePullRequest)

...

Screenshots

image

@guillep2k guillep2k added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label Apr 4, 2020
@guillep2k
Copy link
Member

I would have thought this was the case already. Care to send a PR?

@everhopingandwaiting
Copy link
Contributor Author

OK,i will check my local code and test it later

everhopingandwaiting added a commit to everhopingandwaiting/gitea that referenced this issue Apr 4, 2020
… merge white list go-gitea#10935

(cherry picked from commit ba74fc6389dfcad03c273441a49b54e4d38c86ee)
lafriks pushed a commit that referenced this issue Apr 8, 2020
… merge white list (#10951)

* [suggest] change merge strategy: do not check write access if user in merge white list #10935

(cherry picked from commit ba74fc6389dfcad03c273441a49b54e4d38c86ee)

* fix NPE

* Fix cross compile (#10952)

* Fix cross compile

* Add test for cross compile

* Fix drone

* Fix drone

* Also prevent CC environment not to generate

Co-authored-by: zeripath <[email protected]>

* fix merge box icon color bug (#10974)

that because need some space beturn ``text`` and color defines

Signed-off-by: a1012112796 <[email protected]>

* [skip ci] Updated translations via Crowdin

* Allow X in addition to x in tasks (#10979)

Signed-off-by: Andrew Thornton <[email protected]>

* remove api: merge  reqRepoWriter

Co-authored-by: zeripath <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: 赵智超 <[email protected]>
Co-authored-by: GiteaBot <[email protected]>
Co-authored-by: techknowlogick <[email protected]>
Co-authored-by: guillep2k <[email protected]>
ydelafollye pushed a commit to ydelafollye/gitea that referenced this issue Jul 31, 2020
… merge white list (go-gitea#10951)

* [suggest] change merge strategy: do not check write access if user in merge white list go-gitea#10935

(cherry picked from commit ba74fc6389dfcad03c273441a49b54e4d38c86ee)

* fix NPE

* Fix cross compile (go-gitea#10952)

* Fix cross compile

* Add test for cross compile

* Fix drone

* Fix drone

* Also prevent CC environment not to generate

Co-authored-by: zeripath <[email protected]>

* fix merge box icon color bug (go-gitea#10974)

that because need some space beturn ``text`` and color defines

Signed-off-by: a1012112796 <[email protected]>

* [skip ci] Updated translations via Crowdin

* Allow X in addition to x in tasks (go-gitea#10979)

Signed-off-by: Andrew Thornton <[email protected]>

* remove api: merge  reqRepoWriter

Co-authored-by: zeripath <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: 赵智超 <[email protected]>
Co-authored-by: GiteaBot <[email protected]>
Co-authored-by: techknowlogick <[email protected]>
Co-authored-by: guillep2k <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
2 participants