fix(hook): repo admins are wrongly denied the right to force merge
The right to force merge is uses the wrong predicate and applies to instance admins: ctx.user.IsAdmin It must apply to repository admins and use the following predicate: ctx.userPerm.IsAdmin() This regression is from the ApplyToAdmins implementation in79b7089360
. Fixes: https://codeberg.org/forgejo/forgejo/issues/3780 (cherry picked from commit09f3518069
)
This commit is contained in:
parent
baec3dc6b9
commit
2df082393e
3 changed files with 13 additions and 8 deletions
1
release-notes/8.0.0/fix/3976.md
Normal file
1
release-notes/8.0.0/fix/3976.md
Normal file
|
@ -0,0 +1 @@
|
||||||
|
- repository admins are always denied the right to force merge and instance admins are subject to restrictions to merge that must only apply to repository admins
|
|
@ -405,7 +405,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r
|
||||||
|
|
||||||
// It's not allowed t overwrite protected files. Unless if the user is an
|
// It's not allowed t overwrite protected files. Unless if the user is an
|
||||||
// admin and the protected branch rule doesn't apply to admins.
|
// admin and the protected branch rule doesn't apply to admins.
|
||||||
if changedProtectedfiles && (!ctx.user.IsAdmin || protectBranch.ApplyToAdmins) {
|
if changedProtectedfiles && (!ctx.userPerm.IsAdmin() || protectBranch.ApplyToAdmins) {
|
||||||
log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
|
log.Warn("Forbidden: Branch: %s in %-v is protected from changing file %s", branchName, repo, protectedFilePath)
|
||||||
ctx.JSON(http.StatusForbidden, private.Response{
|
ctx.JSON(http.StatusForbidden, private.Response{
|
||||||
UserMsg: fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
|
UserMsg: fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath),
|
||||||
|
@ -417,7 +417,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r
|
||||||
if pb, err := pull_service.CheckPullBranchProtections(ctx, pr, true); err != nil {
|
if pb, err := pull_service.CheckPullBranchProtections(ctx, pr, true); err != nil {
|
||||||
if models.IsErrDisallowedToMerge(err) {
|
if models.IsErrDisallowedToMerge(err) {
|
||||||
// Allow this if the rule doesn't apply to admins and the user is an admin.
|
// Allow this if the rule doesn't apply to admins and the user is an admin.
|
||||||
if ctx.user.IsAdmin && !pb.ApplyToAdmins {
|
if ctx.userPerm.IsAdmin() && !pb.ApplyToAdmins {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", ctx.opts.UserID, branchName, repo, pr.Index, err.Error())
|
log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", ctx.opts.UserID, branchName, repo, pr.Index, err.Error())
|
||||||
|
|
|
@ -119,12 +119,16 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
|
||||||
|
|
||||||
// * if the doer is admin, they could skip the branch protection check,
|
// * if the doer is admin, they could skip the branch protection check,
|
||||||
// if that's allowed by the protected branch rule.
|
// if that's allowed by the protected branch rule.
|
||||||
if adminSkipProtectionCheck && !pb.ApplyToAdmins {
|
if adminSkipProtectionCheck {
|
||||||
if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil {
|
if doer.IsAdmin {
|
||||||
log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, errCheckAdmin)
|
err = nil // instance admin can skip the check, so clear the error
|
||||||
return errCheckAdmin
|
} else if !pb.ApplyToAdmins {
|
||||||
} else if isRepoAdmin {
|
if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil {
|
||||||
err = nil // repo admin can skip the check, so clear the error
|
log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, errCheckAdmin)
|
||||||
|
return errCheckAdmin
|
||||||
|
} else if isRepoAdmin {
|
||||||
|
err = nil // repo admin can skip the check, so clear the error
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
Loading…
Add table
Reference in a new issue