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.

Sign up for GitHub

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

Jump to bottom

Make admins adhere to branch protection rules #32248

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Make admins adhere to branch protection rules #32248

wants to merge 8 commits into from
+112 −9

Conversation

enko
Copy link

@enko enko commented Oct 12, 2024

This introduces a new flag BlockAdminMergeOverride on the branch protection rules that prevents admins/repo owners from bypassing branch protection rules and merging without approvals or failing status checks.

Fixes #17131

@GiteaBot
Copy link
Contributor

@enko I noticed you've updated the locales for non-English languages. These will be overwritten during the sync from our translation tool Crowdin. If you'd like to contribute your translations, please visit https://crowdin.com/project/gitea. Please revert the changes done on these files. 🍵

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 12, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 12, 2024
@github-actions github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Oct 12, 2024
@enko enko force-pushed the feature/block-admin-merge-override branch 2 times, most recently from 998af0a to 4595e4e Compare October 12, 2024 20:24
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 12, 2024
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Oct 12, 2024
@lunny
Copy link
Member

lunny commented Oct 13, 2024

Wouldn't the admin can change the protection rules?

@enko
Copy link
Author

enko commented Oct 13, 2024

Wouldn't the admin can change the protection rules?

Yes, but that would leave a paper trail.

It is the same on GitHub.

@lunny
Copy link
Member

lunny commented Oct 13, 2024

Wouldn't the admin can change the protection rules?

Yes, but that would leave a paper trail.

It is the same on GitHub.

I know Github's behaviour but I suspect it's not a better solution than having a configuration item in app.ini

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 14, 2024 •
edited
Loading

Wouldn't the admin can change the protection rules?

Yes, but that would leave a paper trail.
It is the same on GitHub.

I know Github's behaviour but I suspect it's not a better solution than having a configuration item in app.ini

But why a "global" config item? For large instance, different organizations have different requirements.

If it would use a "config option", Gitea does need a well-designed config system, global -> org-level -> repo-level.

@enko
Copy link
Author

enko commented Oct 14, 2024

I know Github's behaviour but I suspect it's not a better solution than having a configuration item in app.ini

I see why you would want to have it in your app.ini to have this setting behave very strict. My point would be that it is primarily about compliance and accidental merge. If an owner/admin changes the setting, he has to actively do it and if Gitea would have an audit log, it would leave a paper trail.

But why a "global" config item? For large instance, different organizations have different requirements.

Yes, different projects have different needs and sometimes even different branches have different needs.

[…] Gitea does need a well-designed config system, global -> org-level -> repo-level.

That is out of scope.

@enko enko force-pushed the feature/block-admin-merge-override branch from 4595e4e to 028022a Compare October 17, 2024 19:52
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 17, 2024
@enko
Copy link
Author

enko commented Oct 17, 2024

@wxiaoguang I added the service changes and also added a test case.

@enko enko force-pushed the feature/block-admin-merge-override branch from 028022a to 952bc01 Compare October 17, 2024 19:58
@wxiaoguang
Copy link
Contributor

Thank you for the update. There are still some things left:

  1. There are some lint errors.
  2. Some structs in "modules/structs/repo_branch.go" are changed, but I do not see how they are used in code. If I read correctly, only the one in "ProtectBranchForm" is really used in code.
  3. Database change needs a database migration.

@enko enko force-pushed the feature/block-admin-merge-override branch from 952bc01 to 4fadaf6 Compare October 18, 2024 13:39
@enko
Copy link
Author

enko commented Oct 19, 2024

@wxiaoguang Thanks for your input!

Thank you for the update. There are still some things left:

1. There are some lint errors.

Fixed that, sorry for that.

2. Some structs in "modules/structs/repo_branch.go" are changed, but I do not see how they are used in code. If I read correctly, only the one in "ProtectBranchForm" is really used in code.

I checked, and I noticed I missed the API did not use the new field. I added that.

3. Database change needs a database migration.

Added a migration, I hope I did that right?

@enko
Make admins adhere to branch protection rules
c5bc886 
This introduces a new flag `BlockAdminMergeOverride` on the branch
protection rules that prevents admins/repo owners from bypassing branch
protection rules and merging without approvals or failing status checks.

Fixes go-gitea#17131
@enko
Add migration
ffddf43
@enko
Make the API work
70eb2f0
@enko enko force-pushed the feature/block-admin-merge-override branch from 41daca4 to 70eb2f0 Compare October 19, 2024 20:17
wxiaoguang
wxiaoguang reviewed Oct 20, 2024
View reviewed changes
services/pull/check.go Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

Some changes in 71b5edd:

  1. Make the database migration work models/migrations/migrations.go)
  2. Add some spaces in options/locale/locale_en-US.ini
  3. Use Errorf(%w) to wrap the correct errors
  4. Simplify some if blocks
  5. Rename the argument adminSkipProtectionCheck to adminForceMerge to match the callers form.ForceMerge.

@GiteaBot GiteaBot 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 Oct 20, 2024
@lunny
Copy link
Member

lunny commented Oct 20, 2024

图片 Scheduled merge should also be checked from template.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 21, 2024 •
edited
Loading

Scheduled merge should also be checked from template.

Why?

image

@lunny
Copy link
Member

lunny commented Oct 21, 2024

I mean auto-merge means should be hidden for the administrator from the UI.

@wxiaoguang
Copy link
Contributor

I mean auto-merge means should be hidden for the administrator from the UI.

Why they should hide? Auto merge only merge when checks pass.

@lunny
Copy link
Member

lunny commented Oct 21, 2024

I mean auto-merge means should be hidden for the administrator from the UI.

Why they should hide? Auto merge only merge when checks pass.

Yes, but the permissions that can merge the pull request are checked when creating the schedule.

@wxiaoguang
Copy link
Contributor

I mean auto-merge means should be hidden for the administrator from the UI.

Why they should hide? Auto merge only merge when checks pass.

Yes, but the permissions that can merge the pull request are checked when creating the schedule.

I do not understand what you mean. Show a real case, what's wrong would happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@lalo-mx lalo-mx lalo-mx left review comments

Assignees
No one assigned
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files modifies/translation size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Option to turn off ability for administrator to merge pull request without getting approvals granted
5 participants
@enko @GiteaBot @lunny @wxiaoguang @lalo-mx