diff --git a/app/api/controller/githook/pre_receive.go b/app/api/controller/githook/pre_receive.go index d6b248b0d..292f443b3 100644 --- a/app/api/controller/githook/pre_receive.go +++ b/app/api/controller/githook/pre_receive.go @@ -123,6 +123,7 @@ func (c *Controller) checkProtectionRules( violations, err := protectionRules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, + AllowBypass: true, IsRepoOwner: isRepoOwner, Repo: repo, RefAction: refAction, diff --git a/app/api/controller/pullreq/merge.go b/app/api/controller/pullreq/merge.go index 4266dd04a..2d43cabb5 100644 --- a/app/api/controller/pullreq/merge.go +++ b/app/api/controller/pullreq/merge.go @@ -37,9 +37,10 @@ import ( ) type MergeInput struct { - Method enum.MergeMethod `json:"method"` - SourceSHA string `json:"source_sha"` - DryRun bool `json:"dry_run"` + Method enum.MergeMethod `json:"method"` + SourceSHA string `json:"source_sha"` + BypassRules bool `json:"bypass_rules"` + DryRun bool `json:"dry_run"` } // Merge merges the pull request. @@ -93,6 +94,7 @@ func (c *Controller) Merge( return nil, nil, usererror.BadRequest("Pull request must be open") } + // TODO: Uncomment when the front-end side starts sending SourceSHA. /* if pr.SourceSHA != in.SourceSHA { return nil, nil, @@ -153,6 +155,7 @@ func (c *Controller) Merge( ruleOut, violations, err := protectionRules.MergeVerify(ctx, protection.MergeVerifyInput{ Actor: &session.Principal, + AllowBypass: in.BypassRules, IsRepoOwner: isRepoOwner, TargetRepo: targetRepo, SourceRepo: sourceRepo, diff --git a/app/api/controller/repo/commit.go b/app/api/controller/repo/commit.go index 5b611bbb7..3e0d13605 100644 --- a/app/api/controller/repo/commit.go +++ b/app/api/controller/repo/commit.go @@ -79,6 +79,7 @@ func (c *Controller) CommitFiles(ctx context.Context, violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, + AllowBypass: true, IsRepoOwner: isRepoOwner, Repo: repo, RefAction: refAction, diff --git a/app/api/controller/repo/create_branch.go b/app/api/controller/repo/create_branch.go index aec8e3fb7..3b93b51d1 100644 --- a/app/api/controller/repo/create_branch.go +++ b/app/api/controller/repo/create_branch.go @@ -58,6 +58,7 @@ func (c *Controller) CreateBranch(ctx context.Context, violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, + AllowBypass: true, IsRepoOwner: isRepoOwner, Repo: repo, RefAction: protection.RefActionCreate, diff --git a/app/api/controller/repo/create_commit_tag.go b/app/api/controller/repo/create_commit_tag.go index 4964f448d..e18f863f3 100644 --- a/app/api/controller/repo/create_commit_tag.go +++ b/app/api/controller/repo/create_commit_tag.go @@ -62,6 +62,7 @@ func (c *Controller) CreateCommitTag(ctx context.Context, violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, + AllowBypass: true, IsRepoOwner: isRepoOwner, Repo: repo, RefAction: protection.RefActionCreate, diff --git a/app/api/controller/repo/delete_branch.go b/app/api/controller/repo/delete_branch.go index 3f4755c5a..792235332 100644 --- a/app/api/controller/repo/delete_branch.go +++ b/app/api/controller/repo/delete_branch.go @@ -53,6 +53,7 @@ func (c *Controller) DeleteBranch(ctx context.Context, violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, + AllowBypass: true, IsRepoOwner: isRepoOwner, Repo: repo, RefAction: protection.RefActionDelete, diff --git a/app/api/controller/repo/delete_tag.go b/app/api/controller/repo/delete_tag.go index 1ab9fd47b..3a34e7a9c 100644 --- a/app/api/controller/repo/delete_tag.go +++ b/app/api/controller/repo/delete_tag.go @@ -44,6 +44,7 @@ func (c *Controller) DeleteTag(ctx context.Context, violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, + AllowBypass: true, IsRepoOwner: isRepoOwner, Repo: repo, RefAction: protection.RefActionDelete, diff --git a/app/services/protection/bypass.go b/app/services/protection/bypass.go index 51a499528..a842c9acd 100644 --- a/app/services/protection/bypass.go +++ b/app/services/protection/bypass.go @@ -16,6 +16,10 @@ package protection import ( "fmt" + + "github.com/harness/gitness/types" + + "golang.org/x/exp/slices" ) type DefBypass struct { @@ -23,6 +27,13 @@ type DefBypass struct { RepoOwners bool `json:"repo_owners,omitempty"` } +func (v DefBypass) matches(actor *types.Principal, isRepoOwner bool) bool { + return actor != nil && + (actor.Admin || + v.RepoOwners && isRepoOwner || + slices.Contains(v.UserIDs, actor.ID)) +} + func (v DefBypass) Sanitize() error { if err := validateIDSlice(v.UserIDs); err != nil { return fmt.Errorf("user IDs error: %w", err) diff --git a/app/services/protection/bypass_test.go b/app/services/protection/bypass_test.go new file mode 100644 index 000000000..ed3497ef6 --- /dev/null +++ b/app/services/protection/bypass_test.go @@ -0,0 +1,85 @@ +// Copyright 2023 Harness, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package protection + +import ( + "testing" + + "github.com/harness/gitness/types" +) + +func TestBranch_matches(t *testing.T) { + user := &types.Principal{ID: 42} + admin := &types.Principal{ID: 66, Admin: true} + + tests := []struct { + name string + bypass DefBypass + actor *types.Principal + owner bool + exp bool + }{ + { + name: "empty", + bypass: DefBypass{UserIDs: nil, RepoOwners: false}, + actor: user, + exp: false, + }, + { + name: "admin", + bypass: DefBypass{UserIDs: nil, RepoOwners: false}, + actor: admin, + exp: true, + }, + { + name: "repo-owners-false", + bypass: DefBypass{UserIDs: nil, RepoOwners: false}, + actor: user, + owner: true, + exp: false, + }, + { + name: "repo-owners-true", + bypass: DefBypass{UserIDs: nil, RepoOwners: true}, + actor: user, + owner: true, + exp: true, + }, + { + name: "selected-false", + bypass: DefBypass{UserIDs: []int64{1, 66}, RepoOwners: false}, + actor: user, + exp: false, + }, + { + name: "selected-true", + bypass: DefBypass{UserIDs: []int64{1, 42, 66}, RepoOwners: false}, + actor: user, + exp: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if err := test.bypass.Sanitize(); err != nil { + t.Errorf("invalid: %s", err.Error()) + } + + if want, got := test.exp, test.bypass.matches(test.actor, test.owner); want != got { + t.Errorf("want=%t got=%t", want, got) + } + }) + } +} diff --git a/app/services/protection/pattern.go b/app/services/protection/pattern.go index 908167246..8bcd84267 100644 --- a/app/services/protection/pattern.go +++ b/app/services/protection/pattern.go @@ -74,7 +74,7 @@ func (p *Pattern) Matches(branchName, defaultName string) bool { func patternValidate(pattern string) error { if pattern == "" { - return ErrPatternEmptyPattern + return ErrPatternEmpty } _, err := doublestar.Match(pattern, "test") if err != nil { diff --git a/app/services/protection/pattern_test.go b/app/services/protection/pattern_test.go index 019cffead..a7231ace9 100644 --- a/app/services/protection/pattern_test.go +++ b/app/services/protection/pattern_test.go @@ -124,12 +124,12 @@ func TestPattern_Validate(t *testing.T) { { name: "empty-include-globstar", pattern: Pattern{Default: false, Include: []string{""}, Exclude: nil}, - expect: ErrPatternEmptyPattern, + expect: ErrPatternEmpty, }, { name: "empty-exclude-globstar", pattern: Pattern{Default: false, Include: nil, Exclude: []string{""}}, - expect: ErrPatternEmptyPattern, + expect: ErrPatternEmpty, }, { name: "bad-include-pattern", diff --git a/app/services/protection/rule_branch.go b/app/services/protection/rule_branch.go index 8da493df4..b1325ad00 100644 --- a/app/services/protection/rule_branch.go +++ b/app/services/protection/rule_branch.go @@ -19,8 +19,6 @@ import ( "fmt" "github.com/harness/gitness/types" - - "golang.org/x/exp/slices" ) var TypeBranch types.RuleType = "branch" @@ -37,27 +35,39 @@ var ( _ Definition = (*Branch)(nil) ) -//nolint:gocognit // well aware of this func (v *Branch) MergeVerify( ctx context.Context, in MergeVerifyInput, -) (MergeVerifyOutput, []types.RuleViolations, error) { - if v.isBypassed(in.Actor, in.IsRepoOwner) { - return MergeVerifyOutput{}, nil, nil +) (out MergeVerifyOutput, violations []types.RuleViolations, err error) { + out, violations, err = v.PullReq.MergeVerify(ctx, in) + if err != nil { + return } - return v.PullReq.MergeVerify(ctx, in) + bypassed := in.AllowBypass && v.Bypass.matches(in.Actor, in.IsRepoOwner) + for i := range violations { + violations[i].Bypassed = bypassed + } + + return } func (v *Branch) RefChangeVerify( ctx context.Context, in RefChangeVerifyInput, -) ([]types.RuleViolations, error) { - if v.isBypassed(in.Actor, in.IsRepoOwner) || in.RefType != RefTypeBranch || len(in.RefNames) == 0 { - return nil, nil +) (violations []types.RuleViolations, err error) { + if in.RefType != RefTypeBranch || len(in.RefNames) == 0 { + return []types.RuleViolations{}, nil } - return v.Lifecycle.RefChangeVerify(ctx, in) + violations, err = v.Lifecycle.RefChangeVerify(ctx, in) + + bypassed := in.AllowBypass && v.Bypass.matches(in.Actor, in.IsRepoOwner) + for i := range violations { + violations[i].Bypassed = bypassed + } + + return } func (v *Branch) Sanitize() error { @@ -75,10 +85,3 @@ func (v *Branch) Sanitize() error { return nil } - -func (v *Branch) isBypassed(actor *types.Principal, isRepoOwner bool) bool { - return actor != nil && - (actor.Admin || - v.Bypass.RepoOwners && isRepoOwner || - slices.Contains(v.Bypass.UserIDs, actor.ID)) -} diff --git a/app/services/protection/rule_branch_test.go b/app/services/protection/rule_branch_test.go index 3299f4244..388ca2292 100644 --- a/app/services/protection/rule_branch_test.go +++ b/app/services/protection/rule_branch_test.go @@ -15,73 +15,239 @@ package protection import ( + "context" "testing" "github.com/harness/gitness/types" ) -func TestBranch_isBypass(t *testing.T) { +// nolint:gocognit // it's a unit test +func TestBranch_MergeVerify(t *testing.T) { user := &types.Principal{ID: 42} admin := &types.Principal{ID: 66, Admin: true} tests := []struct { name string - bypass DefBypass - actor *types.Principal - owner bool - exp bool + branch Branch + in MergeVerifyInput + expOut MergeVerifyOutput + expVs []types.RuleViolations }{ { name: "empty", - bypass: DefBypass{UserIDs: nil, RepoOwners: false}, - actor: user, - exp: false, + branch: Branch{}, + in: MergeVerifyInput{Actor: user}, + expOut: MergeVerifyOutput{}, + expVs: []types.RuleViolations{}, }, { - name: "admin", - bypass: DefBypass{UserIDs: nil, RepoOwners: false}, - actor: admin, - exp: true, + name: "admin-no-bypass", + branch: Branch{ + Bypass: DefBypass{}, + PullReq: DefPullReq{ + StatusChecks: DefStatusChecks{RequireUIDs: []string{"abc"}}, + Comments: DefComments{RequireResolveAll: true}, + Merge: DefMerge{DeleteBranch: true}, + }, + }, + in: MergeVerifyInput{ + Actor: admin, + AllowBypass: false, + PullReq: &types.PullReq{UnresolvedCount: 1}, + }, + expOut: MergeVerifyOutput{ + DeleteSourceBranch: true, + }, + expVs: []types.RuleViolations{ + { + Bypassed: false, + Violations: []types.Violation{ + {Code: codePullReqCommentsReqResolveAll}, + {Code: codePullReqStatusChecksReqUIDs}, + }, + }, + }, }, { - name: "repo-owners-false", - bypass: DefBypass{UserIDs: nil, RepoOwners: false}, - actor: user, - owner: true, - exp: false, - }, - { - name: "repo-owners-true", - bypass: DefBypass{UserIDs: nil, RepoOwners: true}, - actor: user, - owner: true, - exp: true, - }, - { - name: "selected-false", - bypass: DefBypass{UserIDs: []int64{1, 66}, RepoOwners: false}, - actor: user, - exp: false, - }, - { - name: "selected-true", - bypass: DefBypass{UserIDs: []int64{1, 42, 66}, RepoOwners: false}, - actor: user, - exp: true, + name: "user-bypass", + branch: Branch{ + Bypass: DefBypass{UserIDs: []int64{user.ID}}, + PullReq: DefPullReq{ + StatusChecks: DefStatusChecks{RequireUIDs: []string{"abc"}}, + Comments: DefComments{RequireResolveAll: true}, + Merge: DefMerge{DeleteBranch: true}, + }, + }, + in: MergeVerifyInput{ + Actor: user, + AllowBypass: true, + PullReq: &types.PullReq{UnresolvedCount: 1}, + }, + expOut: MergeVerifyOutput{ + DeleteSourceBranch: true, + }, + expVs: []types.RuleViolations{ + { + Bypassed: true, + Violations: []types.Violation{ + {Code: codePullReqCommentsReqResolveAll}, + {Code: codePullReqStatusChecksReqUIDs}, + }, + }, + }, }, } + ctx := context.Background() + for _, test := range tests { t.Run(test.name, func(t *testing.T) { - branch := Branch{Bypass: test.bypass} - - if err := branch.Sanitize(); err != nil { + if err := test.branch.Sanitize(); err != nil { t.Errorf("invalid: %s", err.Error()) return } - if want, got := test.exp, branch.isBypassed(test.actor, test.owner); want != got { - t.Errorf("want=%t got=%t", want, got) + out, results, err := test.branch.MergeVerify(ctx, test.in) + if err != nil { + t.Errorf("error: %s", err.Error()) + return + } + + if want, got := test.expOut, out; want != got { + t.Errorf("output: want=%+v got=%+v", want, got) + } + + if want, got := len(test.expVs), len(results); want != got { + t.Errorf("number of violations mismatch: want=%d got=%d", want, got) + return + } + + for i := range results { + if want, got := test.expVs[i].Bypassed, results[i].Bypassed; want != got { + t.Errorf("rule result %d, bypassed mismatch: want=%t got=%t", i, want, got) + return + } + + if want, got := len(test.expVs[i].Violations), len(results[i].Violations); want != got { + t.Errorf("rule result %d, violations count mismatch: want=%d got=%d", i, want, got) + return + } + + for j := range results[i].Violations { + if want, got := test.expVs[i].Violations[j].Code, results[i].Violations[j].Code; want != got { + t.Errorf("rule result %d, violation %d, code mismatch: want=%s got=%s", i, j, want, got) + } + } + } + }) + } +} + +// nolint:gocognit // it's a unit test +func TestBranch_RefChangeVerify(t *testing.T) { + user := &types.Principal{ID: 42} + admin := &types.Principal{ID: 66, Admin: true} + + tests := []struct { + name string + branch Branch + in RefChangeVerifyInput + expVs []types.RuleViolations + }{ + { + name: "empty", + branch: Branch{ + Bypass: DefBypass{}, + Lifecycle: DefLifecycle{}, + }, + in: RefChangeVerifyInput{ + Actor: user, + }, + expVs: []types.RuleViolations{}, + }, + { + name: "admin-no-bypass", + branch: Branch{ + Bypass: DefBypass{}, + Lifecycle: DefLifecycle{DeleteForbidden: true}, + }, + in: RefChangeVerifyInput{ + Actor: admin, + AllowBypass: false, + RefAction: RefActionDelete, + RefType: RefTypeBranch, + RefNames: []string{"abc"}, + }, + expVs: []types.RuleViolations{ + { + Bypassed: false, + Violations: []types.Violation{ + {Code: codeLifecycleDelete}, + }, + }, + }, + }, + { + name: "owner-bypass", + branch: Branch{ + Bypass: DefBypass{RepoOwners: true}, + Lifecycle: DefLifecycle{DeleteForbidden: true}, + }, + in: RefChangeVerifyInput{ + Actor: admin, + AllowBypass: true, + IsRepoOwner: true, + RefAction: RefActionDelete, + RefType: RefTypeBranch, + RefNames: []string{"abc"}, + }, + expVs: []types.RuleViolations{ + { + Bypassed: true, + Violations: []types.Violation{ + {Code: codeLifecycleDelete}, + }, + }, + }, + }, + } + + ctx := context.Background() + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if err := test.branch.Sanitize(); err != nil { + t.Errorf("invalid: %s", err.Error()) + return + } + + results, err := test.branch.RefChangeVerify(ctx, test.in) + if err != nil { + t.Errorf("error: %s", err.Error()) + return + } + + if want, got := len(test.expVs), len(results); want != got { + t.Errorf("number of violations mismatch: want=%d got=%d", want, got) + return + } + + for i := range results { + if want, got := test.expVs[i].Bypassed, results[i].Bypassed; want != got { + t.Errorf("rule result %d, bypassed mismatch: want=%t got=%t", i, want, got) + return + } + + if want, got := len(test.expVs[i].Violations), len(results[i].Violations); want != got { + t.Errorf("rule result %d, violations count mismatch: want=%d got=%d", i, want, got) + return + } + + for j := range results[i].Violations { + if want, got := test.expVs[i].Violations[j].Code, results[i].Violations[j].Code; want != got { + t.Errorf("rule result %d, violation %d, code mismatch: want=%s got=%s", i, j, want, got) + } + } } }) } diff --git a/app/services/protection/service.go b/app/services/protection/service.go index 0ad588cc0..f7a5215d9 100644 --- a/app/services/protection/service.go +++ b/app/services/protection/service.go @@ -54,7 +54,7 @@ type ( var ( ErrUnrecognizedType = errors.New("unrecognized protection type") ErrAlreadyRegistered = errors.New("protection type already registered") - ErrPatternEmptyPattern = errors.New("name pattern can't be empty") + ErrPatternEmpty = errors.New("name pattern can't be empty") ErrInvalidGlobstarPattern = errors.New("invalid globstar pattern") ) @@ -132,15 +132,3 @@ func (m *Manager) ForRepository(ctx context.Context, repoID int64) (Protection, manager: m, }, nil } - -func (m *Manager) MergeVerifierForRepository(ctx context.Context, repoID int64) (MergeVerifier, error) { - ruleInfos, err := m.ruleStore.ListAllRepoRules(ctx, repoID) - if err != nil { - return nil, fmt.Errorf("failed to list rules for repository: %w", err) - } - - return ruleSet{ - rules: ruleInfos, - manager: m, - }, nil -} diff --git a/app/services/protection/service_test.go b/app/services/protection/service_test.go new file mode 100644 index 000000000..ee44b231e --- /dev/null +++ b/app/services/protection/service_test.go @@ -0,0 +1,133 @@ +// Copyright 2023 Harness, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package protection + +import ( + "encoding/json" + "testing" + + "github.com/harness/gitness/types" + "github.com/harness/gitness/types/enum" +) + +func TestIsCritical(t *testing.T) { + tests := []struct { + name string + input []types.RuleViolations + exp bool + }{ + { + name: "empty", + input: []types.RuleViolations{}, + exp: false, + }, + { + name: "non-critical", + input: []types.RuleViolations{ + { + Rule: types.RuleInfo{State: enum.RuleStateMonitor}, + Bypassed: false, + Violations: []types.Violation{{Code: "x"}, {Code: "x"}}, + }, + { + Rule: types.RuleInfo{State: enum.RuleStateActive}, + Bypassed: true, + Violations: []types.Violation{{Code: "x"}, {Code: "x"}}, + }, + { + Rule: types.RuleInfo{State: enum.RuleStateActive}, + Bypassed: false, + Violations: []types.Violation{}, + }, + }, + exp: false, + }, + { + name: "critical", + input: []types.RuleViolations{ + { + Rule: types.RuleInfo{State: enum.RuleStateActive}, + Bypassed: false, + Violations: []types.Violation{{Code: "x"}, {Code: "x"}}, + }, + }, + exp: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if want, got := test.exp, IsCritical(test.input); want != got { + t.Errorf("want=%t got=%t", want, got) + } + }) + } +} + +func TestManager_SanitizeJSON(t *testing.T) { + tests := []struct { + name string + ruleTypes []types.RuleType + ruleType types.RuleType + errReg error + errSan error + }{ + { + name: "success", + ruleTypes: []types.RuleType{TypeBranch}, + ruleType: TypeBranch, + }, + { + name: "duplicate", + ruleTypes: []types.RuleType{TypeBranch, TypeBranch}, + ruleType: TypeBranch, + errReg: ErrAlreadyRegistered, + }, + { + name: "unregistered", + ruleTypes: []types.RuleType{}, + ruleType: TypeBranch, + errSan: ErrUnrecognizedType, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + m := NewManager(nil) + + err := func() error { + for _, ruleType := range test.ruleTypes { + err := m.Register(ruleType, func() Definition { return &Branch{} }) + if err != nil { + return err + } + } + return nil + }() + // nolint:errorlint // deliberately comparing errors with == + if test.errReg != err { + t.Errorf("register type error mismatch: want=%v got=%v", test.errReg, err) + return + } + + _, err = m.SanitizeJSON(test.ruleType, json.RawMessage("{}")) + // nolint:errorlint // deliberately comparing errors with == + if test.errSan != err { + t.Errorf("register type error mismatch: want=%v got=%v", test.errSan, err) + return + } + }) + } +} diff --git a/app/services/protection/set.go b/app/services/protection/set.go index c6765cd5d..16437729f 100644 --- a/app/services/protection/set.go +++ b/app/services/protection/set.go @@ -95,16 +95,8 @@ func (s ruleSet) RefChangeVerify(ctx context.Context, in RefChangeVerifyInput) ( } func backFillRule(vs []types.RuleViolations, rule types.RuleInfo) []types.RuleViolations { - violations := make([]types.RuleViolations, 0, len(vs)) - for i := range vs { - if len(vs[i].Violations) == 0 { - continue - } - vs[i].Rule = rule - violations = append(violations, vs[i]) } - - return violations + return vs } diff --git a/app/services/protection/verify_lifecycle.go b/app/services/protection/verify_lifecycle.go index b9ca7f4a9..bf1c27b54 100644 --- a/app/services/protection/verify_lifecycle.go +++ b/app/services/protection/verify_lifecycle.go @@ -27,6 +27,7 @@ type ( RefChangeVerifyInput struct { Actor *types.Principal + AllowBypass bool IsRepoOwner bool Repo *types.Repository RefAction RefAction @@ -90,7 +91,11 @@ func (v *DefLifecycle) RefChangeVerify(_ context.Context, in RefChangeVerifyInpu } } - return []types.RuleViolations{violations}, nil + if len(violations.Violations) > 0 { + return []types.RuleViolations{violations}, nil + } + + return nil, nil } func (*DefLifecycle) Sanitize() error { diff --git a/app/services/protection/verify_pullreq.go b/app/services/protection/verify_pullreq.go index 4f1046ccc..733a4840c 100644 --- a/app/services/protection/verify_pullreq.go +++ b/app/services/protection/verify_pullreq.go @@ -33,6 +33,7 @@ type ( MergeVerifyInput struct { Actor *types.Principal + AllowBypass bool IsRepoOwner bool Membership *types.Membership TargetRepo *types.Repository @@ -163,7 +164,11 @@ func (v *DefPullReq) MergeVerify( } } - return out, []types.RuleViolations{violations}, nil + if len(violations.Violations) > 0 { + return out, []types.RuleViolations{violations}, nil + } + + return out, nil, nil } type DefApprovals struct { diff --git a/types/rule.go b/types/rule.go index 5fa02bfbd..951e0447c 100644 --- a/types/rule.go +++ b/types/rule.go @@ -63,6 +63,7 @@ type Violation struct { // RuleViolations holds several violations of a rule. type RuleViolations struct { Rule RuleInfo `json:"rule"` + Bypassed bool `json:"bypassed"` Violations []Violation `json:"violations"` } @@ -83,7 +84,7 @@ func (violations *RuleViolations) Addf(code, format string, params ...any) { } func (violations *RuleViolations) IsCritical() bool { - return violations.Rule.State == enum.RuleStateActive + return violations.Rule.State == enum.RuleStateActive && !violations.Bypassed && len(violations.Violations) > 0 } // RuleInfo holds basic info about a rule that is used to describe the rule in RuleViolations.