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

Merged
merged 12 commits into from
Oct 23, 2024
Merged

Make admins adhere to branch protection rules #32248

merged 12 commits into from
Oct 23, 2024

Conversation

enko
Copy link
Contributor

@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
Collaborator

@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
@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 and removed size/S 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
Contributor 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
Contributor 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 and removed size/M labels Oct 17, 2024
@enko
Copy link
Contributor 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
Contributor 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 added 3 commits October 19, 2024 22:17
@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
@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.

@lunny lunny added this to the 1.23.0 milestone Oct 22, 2024
@GiteaBot GiteaBot 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 Oct 22, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 22, 2024
@enko
Copy link
Contributor Author

enko commented Oct 22, 2024

@wxiaoguang @lunny Thank you for your support in bringing this feature to fruition.

Is there anything for me to do?

@lunny
Copy link
Member

lunny commented Oct 22, 2024

@wxiaoguang @lunny Thank you for your support in bringing this feature to fruition.

Is there anything for me to do?

No action is needed unless additional maintainers submit new review requests. Otherwise, this pull request will be merged in a few days.

@wxiaoguang wxiaoguang merged commit de2ad2e into go-gitea:main Oct 23, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 23, 2024
@enko enko deleted the feature/block-admin-merge-override branch October 23, 2024 07:39
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 24, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
2d1d485 
* giteaofficial/main:
  Fix broken image when editing comment with non-image attachments (go-gitea#32319)
  Fix disable 2fa bug (go-gitea#32320)
  Upgrade rollup to 4.24.0 (go-gitea#32312)
  Upgrade vue to 3.5.12 (go-gitea#32311)
  Make admins adhere to branch protection rules (go-gitea#32248)
  Prevent from submitting issue/comment on uploading (go-gitea#32263)
  Add warn log when deleting inactive users (go-gitea#32318)
  Add `DISABLE_ORGANIZATIONS_PAGE` and `DISABLE_CODE_PAGE` settings for explore pages and fix an issue related to user search (go-gitea#32288)
  chore: fix some function names in comment (go-gitea#32300)
silverwind added a commit to silverwind/gitea that referenced this pull request Oct 30, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into types-4
5600f34 
* origin/main: (21 commits)
  Fix toAbsoluteLocaleDate and add more tests (go-gitea#32387)
  Respect UI.ExploreDefaultSort setting again (go-gitea#32357)
  Fix absolute-date (go-gitea#32375)
  Fix undefined errors on Activity page (go-gitea#32378)
  Add new [lfs_client].BATCH_SIZE and [server].LFS_MAX_BATCH_SIZE config settings. (go-gitea#32307)
  remove unused call to $.HeadRepo in view_title template (go-gitea#32317)
  Fix clean tmp dir (go-gitea#32360)
  Optimize branch protection rule loading (go-gitea#32280)
  Suggestions for issues (go-gitea#32327)
  Migrate vue components to setup (go-gitea#32329)
  Fix db engine (go-gitea#32351)
  Refactor the DB migration system slightly (go-gitea#32344)
  Fix broken image when editing comment with non-image attachments (go-gitea#32319)
  Fix disable 2fa bug (go-gitea#32320)
  Upgrade rollup to 4.24.0 (go-gitea#32312)
  Upgrade vue to 3.5.12 (go-gitea#32311)
  Make admins adhere to branch protection rules (go-gitea#32248)
  Prevent from submitting issue/comment on uploading (go-gitea#32263)
  Add warn log when deleting inactive users (go-gitea#32318)
  Add `DISABLE_ORGANIZATIONS_PAGE` and `DISABLE_CODE_PAGE` settings for explore pages and fix an issue related to user search (go-gitea#32288)
  ...
@wxiaoguang wxiaoguang mentioned this pull request Jan 15, 2025
Merged
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

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

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. 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
Projects
None yet
Milestone
1.23.0
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