Fix bug to check whether user can update pull request branch or rebase branch#36465
Fix bug to check whether user can update pull request branch or rebase branch#36465lunny wants to merge 16 commits intogo-gitea:mainfrom
Conversation
GiteaBot
added
the
lgtm/need 2
GiteaBot
added
lgtm/need 1
lunny
changed the title
There was a problem hiding this comment.
The test is not complete, there will be regressions.
- You need to assert that "user can push" before the branch protection. Otherwise, any readers use also pass the tests.
pushAllowed = pushAllowed || mergeAllowedMaintainermeans "allow maintainer edit" can by pass the branch protection?- if yes, clearly test the behavior
- if no, fix the bug
- The test code just duplicates with TestAPIPullUpdateByRebase2.
- TestAPIPullUpdateByRebase2 can be deleted, and carefully test the function in unit test
GiteaBot
added
lgtm/blocked
This has been checked and deeaece added some new test for that.
deeaece updated the logic, now the user will inherit the pull request creator's permission when he is a maintainer and allowing maintainer edit is enabled.
Removed. |
There was a problem hiding this comment.
Pull request overview
This PR updates the pull request “update branch” permission logic so that, when Allow maintainer edits is enabled, a maintainer can update/rebase the PR branch by inheriting the PR author’s permissions (while still honoring repo config and branch protections).
Changes:
- Refactors
IsUserAllowedToUpdateto evaluate push/force-push permissions against head-branch protections and add “inherit poster permissions” behavior for maintainers. - Adds service-level unit tests for
IsUserAllowedToUpdate(protected branches, repo config disabling rebase updates, read-only access, maintainer-edit inheritance). - Removes an integration test case that previously covered additional rebase-update scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/integration/pull_update_test.go | Removes a rebase-update integration test case. |
| services/pull/update.go | Refactors PR update permission checks; adds maintainer-edit inheritance via poster permissions. |
| services/pull/update_test.go | Introduces new unit tests for update permission logic and maintainer-edit behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rebaseAllowed = rebaseAllowed && prBaseUnit.PullRequestsConfig().AllowRebaseUpdate | ||
|
|
||
| // 4. if the pull creator allows maintainer to edit, it means the write permissions of the head branch has been | ||
| // granted to the user with write permission of the base repository | ||
| if pull.AllowMaintainerEdit { | ||
| mergeAllowedMaintainer, err := isUserAllowedToMergeInRepoBranch(ctx, pull.BaseRepoID, pull.BaseBranch, baseRepoPerm, user) | ||
| // 3. if the pull creator allows maintainer to edit, we needs to check whether | ||
| // user is a maintainer and inherit pull request creator's permission | ||
| if pull.AllowMaintainerEdit && (!pushAllowed || !rebaseAllowed) { |
There was a problem hiding this comment.
AllowRebaseUpdate is applied before the AllowMaintainerEdit inheritance logic, but later the inheritance block can set rebaseAllowed back to true (from the poster’s force-push permission), effectively bypassing the base repo’s AllowRebaseUpdate=false setting. Consider storing the base setting in a local variable and applying it after any inheritance, or AND-ing it into the inherited value as well.
| // granted to the user with write permission of the base repository | ||
| if pull.AllowMaintainerEdit { | ||
| mergeAllowedMaintainer, err := isUserAllowedToMergeInRepoBranch(ctx, pull.BaseRepoID, pull.BaseBranch, baseRepoPerm, user) | ||
| // 3. if the pull creator allows maintainer to edit, we needs to check whether |
There was a problem hiding this comment.
Typo/grammar: "we needs" should be "we need".
| // 3. if the pull creator allows maintainer to edit, we needs to check whether | |
| // 3. if the pull creator allows maintainer to edit, we need to check whether |
| // isUserAllowedToPushOrForcePushInRepoBranch checks whether user is allowed to push or force push in the given repo and branch | ||
| // it will check both user permission and branch protection rules | ||
| func isUserAllowedToPushOrForcePushInRepoBranch(ctx context.Context, user *user_model.User, repo *repo_model.Repository, branch string) (pushAllowed, rebaseAllowed bool, err error) { |
There was a problem hiding this comment.
The helper name/comment describe “push or force push”, but the second return value is named rebaseAllowed. Since this value is actually “force-push allowed” (used to decide rebase updates), consider renaming the return value (and related locals) to forcePushAllowed to avoid semantic confusion.
| func TestIsUserAllowedToUpdateMaintainerEditInheritsPosterPermissions(t *testing.T) { | ||
| assert.NoError(t, unittest.PrepareTestDatabase()) | ||
|
|
||
| pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 3}) | ||
| pr.AllowMaintainerEdit = true | ||
| assert.NoError(t, pr.LoadHeadRepo(t.Context())) | ||
| assert.NoError(t, pr.LoadIssue(t.Context())) | ||
| assert.NoError(t, pr.Issue.LoadPoster(t.Context())) | ||
|
|
There was a problem hiding this comment.
The maintainer-edit inheritance behavior isn’t currently tested together with AllowRebaseUpdate=false. Adding a test case where AllowMaintainerEdit=true, the user is a maintainer, the poster can force-push, but the base repo disables rebase updates would prevent regressions (and would catch cases where inheritance accidentally bypasses the config).
4 participants
When checking whether a user can update a pull request branch or perform an update via rebase, a maintainer should inherit the pull request author’s permissions if Allow maintainer edits is enabled.