Only allow admin to bypass rules if bypassing is allowed (#2047)

harness · May 17, 2024 · 6da5c93 · 6da5c93
1 parent fdd401c
commit 6da5c93
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 15 deletions.
diff --git a/app/services/protection/bypass.go b/app/services/protection/bypass.go
@@ -29,8 +29,7 @@ type DefBypass struct {
 
 func (v DefBypass) matches(actor *types.Principal, isRepoOwner bool) bool {
  return actor != nil &&
- (actor.Admin ||
- v.RepoOwners && isRepoOwner ||
+ (v.RepoOwners && isRepoOwner ||
  slices.Contains(v.UserIDs, actor.ID))
 }
 

diff --git a/app/services/protection/bypass_test.go b/app/services/protection/bypass_test.go
@@ -38,10 +38,11 @@ func TestBranch_matches(t *testing.T) {
  exp: false,
  },
  {
- name: "admin",
- bypass: DefBypass{UserIDs: nil, RepoOwners: false},
+ name: "admin-no-owner",
+ bypass: DefBypass{UserIDs: nil, RepoOwners: true},
  actor: admin,
- exp: true,
+ owner: false,
+ exp: false,
  },
  {
  name: "repo-owners-false",

diff --git a/app/services/protection/rule_branch_test.go b/app/services/protection/rule_branch_test.go
@@ -47,7 +47,7 @@ func TestBranch_MergeVerify(t *testing.T) {
  expVs: []types.RuleViolations{},
  },
  {
- name: "admin-no-bypass",
+ name: "admin-no-owner",
  branch: Branch{
  Bypass: DefBypass{},
  PullReq: DefPullReq{
@@ -58,7 +58,8 @@ func TestBranch_MergeVerify(t *testing.T) {
  },
  in: MergeVerifyInput{
  Actor: admin,
- AllowBypass: false,
+ IsRepoOwner: false,
+ AllowBypass: true,
  PullReq: &types.PullReq{UnresolvedCount: 1},
  },
  expOut: MergeVerifyOutput{
@@ -68,7 +69,7 @@ func TestBranch_MergeVerify(t *testing.T) {
  },
  expVs: []types.RuleViolations{
  {
- Bypassable: true,
+ Bypassable: false,
  Bypassed: false,
  Violations: []types.Violation{
  {Code: codePullReqCommentsReqResolveAll},
@@ -314,19 +315,20 @@ func TestBranch_RequiredChecks(t *testing.T) {
  },
  },
  {
- name: "admin-bypassable",
+ name: "admin-no-owner",
  branch: Branch{
  Bypass: DefBypass{},
  PullReq: DefPullReq{
  StatusChecks: DefStatusChecks{RequireIdentifiers: []string{"abc"}},
  },
  },
  in: RequiredChecksInput{
- Actor: admin,
+ Actor: admin,
+ IsRepoOwner: false,
  },
  expOut: RequiredChecksOutput{
- RequiredIdentifiers: nil,
- BypassableIdentifiers: map[string]struct{}{"abc": {}},
+ RequiredIdentifiers: map[string]struct{}{"abc": {}},
+ BypassableIdentifiers: nil,
  },
  },
  {
@@ -407,21 +409,22 @@ func TestBranch_RefChangeVerify(t *testing.T) {
  expVs: []types.RuleViolations{},
  },
  {
- name: "admin-no-bypass",
+ name: "admin-no-owner",
  branch: Branch{
  Bypass: DefBypass{},
  Lifecycle: DefLifecycle{DeleteForbidden: true},
  },
  in: RefChangeVerifyInput{
  Actor: admin,
- AllowBypass: false,
+ IsRepoOwner: false,
+ AllowBypass: true,
  RefAction: RefActionDelete,
  RefType: RefTypeBranch,
  RefNames: []string{"abc"},
  },
  expVs: []types.RuleViolations{
  {
- Bypassable: true,
+ Bypassable: false,
  Bypassed: false,
  Violations: []types.Violation{
  {Code: codeLifecycleDelete},