From 09f3518069addd51fdf5bb3a7181b70f49f2699b Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 1 Jun 2024 10:45:20 +0200 Subject: [PATCH] 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 in 79b70893601c33a33d8d44eb0421797dfd846a47. Fixes: https://codeberg.org/forgejo/forgejo/issues/3780 --- release-notes/8.0.0/fix/3976.md | 1 + routers/private/hook_pre_receive.go | 4 ++-- services/pull/check.go | 16 ++++++++++------ 3 files changed, 13 insertions(+), 8 deletions(-) create mode 100644 release-notes/8.0.0/fix/3976.md diff --git a/release-notes/8.0.0/fix/3976.md b/release-notes/8.0.0/fix/3976.md new file mode 100644 index 0000000000..3588f94dfc --- /dev/null +++ b/release-notes/8.0.0/fix/3976.md @@ -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 diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 97568b3f65..d12a762db6 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -404,7 +404,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r // 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. - 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) ctx.JSON(http.StatusForbidden, private.Response{ UserMsg: fmt.Sprintf("branch %s is protected from changing file %s", branchName, protectedFilePath), @@ -416,7 +416,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r if pb, err := pull_service.CheckPullBranchProtections(ctx, pr, true); err != nil { if models.IsErrDisallowedToMerge(err) { // 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 } 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()) diff --git a/services/pull/check.go b/services/pull/check.go index 765f7580cb..2d91ed07f5 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -119,12 +119,16 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc // * if the doer is admin, they could skip the branch protection check, // if that's allowed by the protected branch rule. - if adminSkipProtectionCheck && !pb.ApplyToAdmins { - if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil { - 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 + if adminSkipProtectionCheck { + if doer.IsAdmin { + err = nil // instance admin can skip the check, so clear the error + } else if !pb.ApplyToAdmins { + if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil { + 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 + } } }