From 1a727b8be10f5a1d36689272b9bd6fd79af7bf2f Mon Sep 17 00:00:00 2001 From: Marko Gacesa Date: Fri, 27 Oct 2023 14:06:09 +0000 Subject: [PATCH] refactor of protection package (#734) --- app/api/controller/githook/pre_receive.go | 2 +- app/api/controller/pullreq/merge.go | 2 +- app/api/controller/repo/commit.go | 2 +- app/api/controller/repo/create_branch.go | 2 +- app/api/controller/repo/create_commit_tag.go | 2 +- app/api/controller/repo/delete_branch.go | 2 +- app/api/controller/repo/delete_tag.go | 2 +- app/services/protection/bypass.go | 32 ++ app/services/protection/interface.go | 99 ------ app/services/protection/pattern.go | 32 +- app/services/protection/pattern_test.go | 28 +- app/services/protection/rule_branch.go | 307 ++--------------- app/services/protection/rule_branch_test.go | 321 +----------------- app/services/protection/service.go | 65 +++- app/services/protection/set.go | 13 +- app/services/protection/verify_lifecycle.go | 98 ++++++ .../protection/verify_lifestyle_test.go | 114 +++++++ app/services/protection/verify_pullreq.go | 276 +++++++++++++++ .../protection/verify_pullreq_test.go | 295 ++++++++++++++++ 19 files changed, 963 insertions(+), 731 deletions(-) create mode 100644 app/services/protection/bypass.go delete mode 100644 app/services/protection/interface.go create mode 100644 app/services/protection/verify_lifecycle.go create mode 100644 app/services/protection/verify_lifestyle_test.go create mode 100644 app/services/protection/verify_pullreq.go create mode 100644 app/services/protection/verify_pullreq_test.go diff --git a/app/api/controller/githook/pre_receive.go b/app/api/controller/githook/pre_receive.go index f1db13df4..3c8f4b78f 100644 --- a/app/api/controller/githook/pre_receive.go +++ b/app/api/controller/githook/pre_receive.go @@ -101,7 +101,7 @@ func (c *Controller) checkProtectionRules( return } - violations, err := protectionRules.CanModifyRef(ctx, protection.CanModifyRefInput{ + violations, err := protectionRules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, IsSpaceOwner: isSpaceOwner, Repo: repo, diff --git a/app/api/controller/pullreq/merge.go b/app/api/controller/pullreq/merge.go index 9b6eed2b3..0cc0e236e 100644 --- a/app/api/controller/pullreq/merge.go +++ b/app/api/controller/pullreq/merge.go @@ -157,7 +157,7 @@ func (c *Controller) Merge( return nil, nil, fmt.Errorf("failed to get code owners with approval: %w", err) } - ruleOut, violations, err := protectionRules.CanMerge(ctx, protection.CanMergeInput{ + ruleOut, violations, err := protectionRules.MergeVerify(ctx, protection.MergeVerifyInput{ Actor: &session.Principal, IsSpaceOwner: isSpaceOwner, TargetRepo: targetRepo, diff --git a/app/api/controller/repo/commit.go b/app/api/controller/repo/commit.go index 959327587..0ee39b7ab 100644 --- a/app/api/controller/repo/commit.go +++ b/app/api/controller/repo/commit.go @@ -77,7 +77,7 @@ func (c *Controller) CommitFiles(ctx context.Context, branchName = in.Branch } - violations, err := rules.CanModifyRef(ctx, protection.CanModifyRefInput{ + violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, IsSpaceOwner: isSpaceOwner, Repo: repo, diff --git a/app/api/controller/repo/create_branch.go b/app/api/controller/repo/create_branch.go index 91515183c..5823cba67 100644 --- a/app/api/controller/repo/create_branch.go +++ b/app/api/controller/repo/create_branch.go @@ -56,7 +56,7 @@ func (c *Controller) CreateBranch(ctx context.Context, return nil, nil, err } - violations, err := rules.CanModifyRef(ctx, protection.CanModifyRefInput{ + violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, IsSpaceOwner: isSpaceOwner, Repo: repo, diff --git a/app/api/controller/repo/create_commit_tag.go b/app/api/controller/repo/create_commit_tag.go index 65e2861c1..f555f7e22 100644 --- a/app/api/controller/repo/create_commit_tag.go +++ b/app/api/controller/repo/create_commit_tag.go @@ -60,7 +60,7 @@ func (c *Controller) CreateCommitTag(ctx context.Context, return nil, nil, err } - violations, err := rules.CanModifyRef(ctx, protection.CanModifyRefInput{ + violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, IsSpaceOwner: isSpaceOwner, Repo: repo, diff --git a/app/api/controller/repo/delete_branch.go b/app/api/controller/repo/delete_branch.go index d73319e24..ed0fcb286 100644 --- a/app/api/controller/repo/delete_branch.go +++ b/app/api/controller/repo/delete_branch.go @@ -51,7 +51,7 @@ func (c *Controller) DeleteBranch(ctx context.Context, return nil, err } - violations, err := rules.CanModifyRef(ctx, protection.CanModifyRefInput{ + violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, IsSpaceOwner: isSpaceOwner, Repo: repo, diff --git a/app/api/controller/repo/delete_tag.go b/app/api/controller/repo/delete_tag.go index be71e08d1..dab43ffb5 100644 --- a/app/api/controller/repo/delete_tag.go +++ b/app/api/controller/repo/delete_tag.go @@ -42,7 +42,7 @@ func (c *Controller) DeleteTag(ctx context.Context, return nil, err } - violations, err := rules.CanModifyRef(ctx, protection.CanModifyRefInput{ + violations, err := rules.RefChangeVerify(ctx, protection.RefChangeVerifyInput{ Actor: &session.Principal, IsSpaceOwner: isSpaceOwner, Repo: repo, diff --git a/app/services/protection/bypass.go b/app/services/protection/bypass.go new file mode 100644 index 000000000..7c66bd2b4 --- /dev/null +++ b/app/services/protection/bypass.go @@ -0,0 +1,32 @@ +// 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 ( + "fmt" +) + +type DefBypass struct { + UserIDs []int64 `json:"user_ids,omitempty"` + SpaceOwners bool `json:"space_owners,omitempty"` +} + +func (v DefBypass) Sanitize() error { + if err := validateIDSlice(v.UserIDs); err != nil { + return fmt.Errorf("user IDs error: %w", err) + } + + return nil +} diff --git a/app/services/protection/interface.go b/app/services/protection/interface.go deleted file mode 100644 index 6b0442933..000000000 --- a/app/services/protection/interface.go +++ /dev/null @@ -1,99 +0,0 @@ -// 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 ( - "context" - "errors" - - "github.com/harness/gitness/app/services/codeowners" - "github.com/harness/gitness/types" - "github.com/harness/gitness/types/enum" -) - -type ( - // Definition represents a protection rule definition. - Definition interface { - // Sanitize validates if the definition is valid and automatically corrects minor issues. - Sanitize() error - - Protection - } - - // Protection defines interface for branch protection. - Protection interface { - CanMerge(ctx context.Context, in CanMergeInput) (CanMergeOutput, []types.RuleViolations, error) - CanModifyRef(ctx context.Context, in CanModifyRefInput) ([]types.RuleViolations, error) - } - - CanMergeInput struct { - Actor *types.Principal - IsSpaceOwner bool - Membership *types.Membership - TargetRepo *types.Repository - SourceRepo *types.Repository - PullReq *types.PullReq - Reviewers []*types.PullReqReviewer - Method enum.MergeMethod - CheckResults []types.CheckResult - CodeOwners *codeowners.Evaluation - } - - CanMergeOutput struct { - DeleteSourceBranch bool - } - - CanModifyRefInput struct { - Actor *types.Principal - IsSpaceOwner bool - Repo *types.Repository - RefAction RefAction - RefType RefType - RefNames []string - } - - RefType int - - RefAction int -) - -const ( - RefTypeRaw RefType = iota - RefTypeBranch - RefTypeTag -) - -const ( - RefActionCreate RefAction = iota - RefActionDelete - RefActionUpdate -) - -var ( - ErrUnrecognizedType = errors.New("unrecognized protection type") - ErrAlreadyRegistered = errors.New("protection type already registered") - ErrPatternEmpty = errors.New("pattern doesn't match anything") - ErrPatternEmptyPattern = errors.New("name pattern can't be empty") - ErrInvalidGlobstarPattern = errors.New("invalid globstar pattern") -) - -func IsCritical(violations []types.RuleViolations) bool { - for i := range violations { - if violations[i].IsCritical() { - return true - } - } - return false -} diff --git a/app/services/protection/pattern.go b/app/services/protection/pattern.go index 8896471a6..234af3ce2 100644 --- a/app/services/protection/pattern.go +++ b/app/services/protection/pattern.go @@ -53,27 +53,27 @@ func (p *Pattern) Validate() error { } func (p *Pattern) Matches(branchName, defaultName string) bool { - if len(p.Exclude) > 0 { - match := true - for _, exclude := range p.Exclude { - match = match && !patternMatches(exclude, branchName) - } - if match { - return true + // Initially match everything, unless the default is set or the include patterns are defined. + matches := !p.Default && len(p.Include) == 0 + + // Apply the default branch. + matches = matches || p.Default && branchName == defaultName + + // Apply the include patterns. + if !matches { + for _, include := range p.Include { + if matches = patternMatches(include, branchName); matches { + break + } } } - if p.Default && branchName == defaultName { - return true + // Apply the exclude patterns. + for _, exclude := range p.Exclude { + matches = matches && !patternMatches(exclude, branchName) } - for _, include := range p.Include { - if patternMatches(include, branchName) { - return true - } - } - - return false + return matches } func patternValidate(pattern string) error { diff --git a/app/services/protection/pattern_test.go b/app/services/protection/pattern_test.go index a392d47d2..f21ec1560 100644 --- a/app/services/protection/pattern_test.go +++ b/app/services/protection/pattern_test.go @@ -28,10 +28,10 @@ func TestPattern_Matches(t *testing.T) { want bool }{ { - name: "empty-matches-nothing", + name: "empty-matches-all", pattern: Pattern{Default: false, Include: nil, Exclude: nil}, input: "blah", - want: false, + want: true, }, { name: "default-matches-default", @@ -69,6 +69,30 @@ func TestPattern_Matches(t *testing.T) { input: "pr_69", want: false, }, + { + name: "complex:not-excluded", + pattern: Pattern{ + Include: []string{"test/**/*"}, + Exclude: []string{"test/release/*"}}, + input: "test/dev/1", + want: true, + }, + { + name: "complex:excluded", + pattern: Pattern{ + Include: []string{"test/**/*"}, + Exclude: []string{"test/release/*"}}, + input: "test/release/1", + want: false, + }, + { + name: "complex:default-excluded", + pattern: Pattern{ + Default: true, + Exclude: []string{defBranch}}, + input: defBranch, + want: false, + }, } for _, test := range tests { diff --git a/app/services/protection/rule_branch.go b/app/services/protection/rule_branch.go index 92e3d6dba..e70effc86 100644 --- a/app/services/protection/rule_branch.go +++ b/app/services/protection/rule_branch.go @@ -16,12 +16,9 @@ package protection import ( "context" - "errors" "fmt" - "github.com/harness/gitness/app/services/codeowners" "github.com/harness/gitness/types" - "github.com/harness/gitness/types/enum" "golang.org/x/exp/slices" ) @@ -35,146 +32,48 @@ type Branch struct { Lifecycle DefLifecycle `json:"lifecycle"` } -type Review struct { - ReviewSHA string - Decision enum.PullReqReviewDecision -} - -var _ Definition = (*Branch)(nil) // ensures that the Branch type implements Definition interface. +var ( + // ensures that the Branch type implements Definition interface. + _ Definition = (*Branch)(nil) +) //nolint:gocognit // well aware of this -func (v *Branch) CanMerge(_ context.Context, in CanMergeInput) (CanMergeOutput, []types.RuleViolations, error) { - var out CanMergeOutput - var violations types.RuleViolations - - out.DeleteSourceBranch = v.PullReq.Merge.DeleteBranch - - // bypass - +func (v *Branch) MergeVerify( + ctx context.Context, + in MergeVerifyInput, +) (MergeVerifyOutput, []types.RuleViolations, error) { if v.isBypassed(in.Actor, in.IsSpaceOwner) { - return out, nil, nil + return MergeVerifyOutput{}, nil, nil } - // pullreq.approvals - - approvedBy := make([]types.PrincipalInfo, 0, len(in.Reviewers)) - for _, reviewer := range in.Reviewers { - if reviewer.ReviewDecision != enum.PullReqReviewDecisionApproved { - continue - } - if v.PullReq.Approvals.RequireLatestCommit && reviewer.SHA != in.PullReq.SourceSHA { - continue - } - - approvedBy = append(approvedBy, reviewer.Reviewer) - } - - if len(approvedBy) < v.PullReq.Approvals.RequireMinimumCount { - violations.Addf("pullreq.approvals.require_minimum_count", - "Insufficient number of approvals. Have %d but need at least %d.", - len(approvedBy), v.PullReq.Approvals.RequireMinimumCount) - } - - //nolint:nestif - if v.PullReq.Approvals.RequireCodeOwners { - for _, entry := range in.CodeOwners.EvaluationEntries { - reviewDecision, approvers := getCodeOwnerApprovalStatus(entry.OwnerEvaluations) - - if reviewDecision == enum.PullReqReviewDecisionPending { - violations.Addf("pullreq.approvals.require_code_owners", - "Code owners approval pending for %s", entry.Pattern) - continue - } - - if reviewDecision == enum.PullReqReviewDecisionChangeReq { - violations.Addf("pullreq.approvals.require_code_owners", - "Code owners requested changes for %s", entry.Pattern) - continue - } - // pull req approved. check other settings - if !v.PullReq.Approvals.RequireLatestCommit { - continue - } - // check for latest commit approved or not - latestSHAApproved := false - for _, approver := range approvers { - if approver.ReviewSHA == in.PullReq.SourceSHA { - latestSHAApproved = true - break - } - } - if !latestSHAApproved { - violations.Addf("pullreq.approvals.require_code_owners", - "Code owners approval pending on latest commit for %s", entry.Pattern) - } - } - } - - // pullreq.comments - - if v.PullReq.Comments.RequireResolveAll && in.PullReq.UnresolvedCount > 0 { - violations.Addf("pullreq.comments.require_resolve_all", - "All comments must be resolved. There are %d unresolved comments.", - in.PullReq.UnresolvedCount) - } - - // pullreq.status_checks - - for _, requiredUID := range v.PullReq.StatusChecks.RequireUIDs { - var succeeded bool - - for i := range in.CheckResults { - if in.CheckResults[i].UID == requiredUID { - succeeded = in.CheckResults[i].Status == enum.CheckStatusSuccess - break - } - } - - if !succeeded { - violations.Add("pullreq.status_checks.required_uids", - "At least one required status check hasn't completed successfully.") - } - } - - // pullreq.merge - - if len(v.PullReq.Merge.StrategiesAllowed) > 0 { // Note: Empty allowed strategies list means all are allowed - if !slices.Contains(v.PullReq.Merge.StrategiesAllowed, in.Method) { - violations.Addf("pullreq.merge.strategies_allowed", - "The requested merge strategy %q is not allowed. Allowed strategies are %v.", - in.Method, v.PullReq.Merge.StrategiesAllowed) - } - } - - return out, []types.RuleViolations{violations}, nil + return v.PullReq.MergeVerify(ctx, in) } -func (v *Branch) CanModifyRef(_ context.Context, in CanModifyRefInput) ([]types.RuleViolations, error) { - var violations types.RuleViolations - +func (v *Branch) RefChangeVerify( + ctx context.Context, + in RefChangeVerifyInput, +) ([]types.RuleViolations, error) { if v.isBypassed(in.Actor, in.IsSpaceOwner) || in.RefType != RefTypeBranch || len(in.RefNames) == 0 { return nil, nil } - switch in.RefAction { - case RefActionCreate: - if v.Lifecycle.CreateForbidden { - violations.Addf("lifecycle.create", - "Creation of branch %q is not allowed.", in.RefNames[0]) - } - case RefActionDelete: - if v.Lifecycle.DeleteForbidden { - violations.Addf("lifecycle.delete", - "Delete of branch %q is not allowed.", in.RefNames[0]) - } - case RefActionUpdate: - if v.Lifecycle.UpdateForbidden { - violations.Addf("lifecycle.update", - "Push to branch %q is not allowed. Please use pull requests.", in.RefNames[0]) - } + return v.Lifecycle.RefChangeVerify(ctx, in) +} + +func (v *Branch) Sanitize() error { + if err := v.Bypass.Sanitize(); err != nil { + return fmt.Errorf("bypass: %w", err) } - return []types.RuleViolations{violations}, nil + if err := v.PullReq.Sanitize(); err != nil { + return fmt.Errorf("pull request: %w", err) + } + + if err := v.Lifecycle.Sanitize(); err != nil { + return fmt.Errorf("lifecycle: %w", err) + } + + return nil } func (v *Branch) isBypassed(actor *types.Principal, isSpaceOwner bool) bool { @@ -183,151 +82,3 @@ func (v *Branch) isBypassed(actor *types.Principal, isSpaceOwner bool) bool { v.Bypass.SpaceOwners && isSpaceOwner || slices.Contains(v.Bypass.UserIDs, actor.ID)) } - -func (v *Branch) Sanitize() error { - if err := v.Bypass.Validate(); err != nil { - return fmt.Errorf("bypass: %w", err) - } - - if err := v.PullReq.Validate(); err != nil { - return fmt.Errorf("pull request: %w", err) - } - - if err := v.Lifecycle.Validate(); err != nil { - return fmt.Errorf("lifecycle: %w", err) - } - - return nil -} - -type DefBypass struct { - UserIDs []int64 `json:"user_ids,omitempty"` - SpaceOwners bool `json:"space_owners,omitempty"` -} - -func (v DefBypass) Validate() error { - if err := validateIDSlice(v.UserIDs); err != nil { - return fmt.Errorf("user IDs error: %w", err) - } - - return nil -} - -type DefApprovals struct { - RequireCodeOwners bool `json:"require_code_owners,omitempty"` - RequireMinimumCount int `json:"require_minimum_count,omitempty"` - RequireLatestCommit bool `json:"require_latest_commit,omitempty"` -} - -func (v DefApprovals) Validate() error { - if v.RequireMinimumCount < 0 { - return errors.New("minimum count must be zero or a positive integer") - } - - return nil -} - -type DefComments struct { - RequireResolveAll bool `json:"require_resolve_all,omitempty"` -} - -func (v DefComments) Validate() error { - return nil -} - -type DefStatusChecks struct { - RequireUIDs []string `json:"require_uids,omitempty"` -} - -func (v DefStatusChecks) Validate() error { - if err := validateUIDSlice(v.RequireUIDs); err != nil { - return fmt.Errorf("required UIDs error: %w", err) - } - - return nil -} - -type DefMerge struct { - StrategiesAllowed []enum.MergeMethod `json:"strategies_allowed,omitempty"` - DeleteBranch bool `json:"delete_branch,omitempty"` -} - -func (v DefMerge) Validate() error { - m := make(map[enum.MergeMethod]struct{}, 0) - for _, strategy := range v.StrategiesAllowed { - if _, ok := strategy.Sanitize(); !ok { - return fmt.Errorf("unrecognized merge strategy: %s", strategy) - } - - if _, ok := m[strategy]; ok { - return fmt.Errorf("duplicate entry in merge strategy list: %s", strategy) - } - - m[strategy] = struct{}{} - } - - return nil -} - -type DefPush struct { - Block bool `json:"block,omitempty"` -} - -func (v DefPush) Validate() error { - return nil -} - -type DefLifecycle struct { - CreateForbidden bool `json:"create_forbidden,omitempty"` - DeleteForbidden bool `json:"delete_forbidden,omitempty"` - UpdateForbidden bool `json:"update_forbidden,omitempty"` -} - -func (v DefLifecycle) Validate() error { - return nil -} - -type DefPullReq struct { - Approvals DefApprovals `json:"approvals"` - Comments DefComments `json:"comments"` - StatusChecks DefStatusChecks `json:"status_checks"` - Merge DefMerge `json:"merge"` -} - -func (v DefPullReq) Validate() error { - if err := v.Approvals.Validate(); err != nil { - return fmt.Errorf("approvals: %w", err) - } - - if err := v.Comments.Validate(); err != nil { - return fmt.Errorf("comments: %w", err) - } - - if err := v.StatusChecks.Validate(); err != nil { - return fmt.Errorf("status checks: %w", err) - } - - if err := v.Merge.Validate(); err != nil { - return fmt.Errorf("merge: %w", err) - } - - return nil -} - -func getCodeOwnerApprovalStatus( - ownerStatus []codeowners.OwnerEvaluation, -) (enum.PullReqReviewDecision, []codeowners.OwnerEvaluation) { - approvers := make([]codeowners.OwnerEvaluation, 0) - for _, o := range ownerStatus { - if o.ReviewDecision == enum.PullReqReviewDecisionChangeReq { - return enum.PullReqReviewDecisionChangeReq, nil - } - if o.ReviewDecision == enum.PullReqReviewDecisionApproved { - approvers = append(approvers, o) - } - } - if len(approvers) > 0 { - return enum.PullReqReviewDecisionApproved, approvers - } - return enum.PullReqReviewDecisionPending, nil -} diff --git a/app/services/protection/rule_branch_test.go b/app/services/protection/rule_branch_test.go index 44412ab8a..3c8b07d6e 100644 --- a/app/services/protection/rule_branch_test.go +++ b/app/services/protection/rule_branch_test.go @@ -15,13 +15,9 @@ package protection import ( - "context" - "reflect" "testing" - gitrpcenum "github.com/harness/gitness/gitrpc/enum" "github.com/harness/gitness/types" - "github.com/harness/gitness/types/enum" ) func TestBranch_isBypass(t *testing.T) { @@ -78,320 +74,15 @@ func TestBranch_isBypass(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { branch := Branch{Bypass: test.bypass} + + if err := 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) } }) } } - -// nolint:gocognit // it's a unit test -func TestBranch_CanMerge(t *testing.T) { - tests := []struct { - name string - branch Branch - in CanMergeInput - expCodes []string - expParams [][]any - expOut CanMergeOutput - }{ - { - name: "empty", - }, - { - name: "pullreq.approvals.require_minimum_count-fail", - branch: Branch{ - PullReq: DefPullReq{Approvals: DefApprovals{RequireMinimumCount: 1}}, - }, - in: CanMergeInput{ - PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, - Reviewers: []*types.PullReqReviewer{ - {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc"}, - }, - }, - expCodes: []string{"pullreq.approvals.require_minimum_count"}, - expParams: [][]any{{0, 1}}, - }, - { - name: "pullreq.approvals.require_minimum_count-success", - branch: Branch{ - PullReq: DefPullReq{Approvals: DefApprovals{RequireMinimumCount: 2}}, - }, - in: CanMergeInput{ - PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, - Reviewers: []*types.PullReqReviewer{ - {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, - {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, - }, - }, - }, - { - name: "pullreq.approvals.require_latest_commit-fail", - branch: Branch{ - PullReq: DefPullReq{Approvals: DefApprovals{RequireMinimumCount: 2, RequireLatestCommit: true}}, - }, - in: CanMergeInput{ - PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, - Reviewers: []*types.PullReqReviewer{ - {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, - {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abd"}, - }, - }, - expCodes: []string{"pullreq.approvals.require_minimum_count"}, - expParams: [][]any{{1, 2}}, - }, - { - name: "pullreq.approvals.require_latest_commit-success", - branch: Branch{ - PullReq: DefPullReq{Approvals: DefApprovals{RequireMinimumCount: 2, RequireLatestCommit: true}}, - }, - in: CanMergeInput{ - PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, - Reviewers: []*types.PullReqReviewer{ - {ReviewDecision: enum.PullReqReviewDecisionPending, SHA: "abc"}, - {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, - {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, - }, - }, - }, - { - name: "pullreq.comments.require_resolve_all-fail", - branch: Branch{ - PullReq: DefPullReq{Comments: DefComments{RequireResolveAll: true}}, - }, - in: CanMergeInput{ - PullReq: &types.PullReq{UnresolvedCount: 6}, - }, - expCodes: []string{"pullreq.comments.require_resolve_all"}, - expParams: [][]any{{6}}, - }, - { - name: "pullreq.comments.require_resolve_all-success", - branch: Branch{ - PullReq: DefPullReq{Comments: DefComments{RequireResolveAll: true}}, - }, - in: CanMergeInput{ - PullReq: &types.PullReq{UnresolvedCount: 0}, - }, - }, - { - name: "pullreq.status_checks.required_uids-fail", - branch: Branch{ - PullReq: DefPullReq{StatusChecks: DefStatusChecks{RequireUIDs: []string{"check1"}}}, - }, - in: CanMergeInput{ - CheckResults: []types.CheckResult{ - {UID: "check1", Status: enum.CheckStatusFailure}, - {UID: "check2", Status: enum.CheckStatusSuccess}, - }, - }, - expCodes: []string{"pullreq.status_checks.required_uids"}, - expParams: [][]any{nil}, - }, - { - name: "pullreq.status_checks.required_uids-success", - branch: Branch{ - PullReq: DefPullReq{StatusChecks: DefStatusChecks{RequireUIDs: []string{"check1"}}}, - }, - in: CanMergeInput{ - CheckResults: []types.CheckResult{ - {UID: "check1", Status: enum.CheckStatusSuccess}, - {UID: "check2", Status: enum.CheckStatusFailure}, - }, - }, - }, - { - name: "pullreq.merge.strategies_allowed-fail", - branch: Branch{ - PullReq: DefPullReq{Merge: DefMerge{StrategiesAllowed: []enum.MergeMethod{ - enum.MergeMethod(gitrpcenum.MergeMethodRebase), - enum.MergeMethod(gitrpcenum.MergeMethodSquash), - }}}, - }, - in: CanMergeInput{ - Method: enum.MergeMethod(gitrpcenum.MergeMethodMerge), - }, - expCodes: []string{"pullreq.merge.strategies_allowed"}, - expParams: [][]any{{ - enum.MergeMethod(gitrpcenum.MergeMethodMerge), - []enum.MergeMethod{ - enum.MergeMethod(gitrpcenum.MergeMethodRebase), - enum.MergeMethod(gitrpcenum.MergeMethodSquash), - }}, - }, - }, - { - name: "pullreq.merge.strategies_allowed-success", - branch: Branch{ - PullReq: DefPullReq{Merge: DefMerge{StrategiesAllowed: []enum.MergeMethod{ - enum.MergeMethod(gitrpcenum.MergeMethodRebase), - enum.MergeMethod(gitrpcenum.MergeMethodSquash), - }}}, - }, - in: CanMergeInput{ - Method: enum.MergeMethod(gitrpcenum.MergeMethodSquash), - }, - }, - { - name: "pullreq.merge.delete_branch", - branch: Branch{ - PullReq: DefPullReq{Merge: DefMerge{DeleteBranch: true}}, - }, - in: CanMergeInput{}, - expOut: CanMergeOutput{DeleteSourceBranch: true}, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - if err := test.branch.Sanitize(); err != nil { - t.Errorf("branch input seems invalid: %s", err.Error()) - return - } - - out, violations, err := test.branch.CanMerge(context.Background(), test.in) - if err != nil { - t.Errorf("got an error: %s", err.Error()) - return - } - - if want, got := test.expOut, out; !reflect.DeepEqual(want, got) { - t.Errorf("output mismatch: want=%+v got=%+v", want, got) - } - - if len(test.expCodes) == 0 && - (len(violations) == 0 || len(violations) == 1 && len(violations[0].Violations) == 0) { - // no violations expected and no violations received - return - } - - if len(violations) != 1 { - t.Error("expected size of violation should always be one") - return - } - - if want, got := len(test.expCodes), len(violations[0].Violations); want != got { - t.Errorf("violation count: want=%d got=%d", want, got) - return - } - - for i, violation := range violations[0].Violations { - if want, got := test.expCodes[i], violation.Code; want != got { - t.Errorf("violation %d code mismatch: want=%s got=%s", i, want, got) - } - if want, got := test.expParams[i], violation.Params; !reflect.DeepEqual(want, got) { - t.Errorf("violation %d params mismatch: want=%v got=%v", i, want, got) - } - } - }) - } -} - -// nolint:gocognit // it's a unit test -func TestBranch_CanModifyRef(t *testing.T) { - tests := []struct { - name string - branch Branch - in CanModifyRefInput - expCodes []string - expParams [][]any - }{ - { - name: "empty", - }, - { - name: "lifecycle.create-fail", - branch: Branch{Lifecycle: DefLifecycle{CreateForbidden: true}}, - in: CanModifyRefInput{RefNames: []string{"a"}, RefAction: RefActionCreate, RefType: RefTypeBranch}, - expCodes: []string{"lifecycle.create"}, - expParams: [][]any{{"a"}}, - }, - { - name: "lifecycle.delete-fail", - branch: Branch{Lifecycle: DefLifecycle{DeleteForbidden: true}}, - in: CanModifyRefInput{RefNames: []string{"a"}, RefAction: RefActionDelete, RefType: RefTypeBranch}, - expCodes: []string{"lifecycle.delete"}, - expParams: [][]any{{"a"}}, - }, - { - name: "lifecycle.update-fail", - branch: Branch{Lifecycle: DefLifecycle{UpdateForbidden: true}}, - in: CanModifyRefInput{RefNames: []string{"a"}, RefAction: RefActionUpdate, RefType: RefTypeBranch}, - expCodes: []string{"lifecycle.update"}, - expParams: [][]any{{"a"}}, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - if err := test.branch.Sanitize(); err != nil { - t.Errorf("branch input seems invalid: %s", err.Error()) - return - } - - violations, err := test.branch.CanModifyRef(context.Background(), test.in) - if err != nil { - t.Errorf("got an error: %s", err.Error()) - return - } - - inspectBranchViolations(t, test.expCodes, test.expParams, violations) - - if len(test.expCodes) == 0 && - (len(violations) == 0 || len(violations) == 1 && len(violations[0].Violations) == 0) { - // no violations expected and no violations received - return - } - - if len(violations) != 1 { - t.Error("expected size of violation should always be one") - return - } - - if want, got := len(test.expCodes), len(violations[0].Violations); want != got { - t.Errorf("violation count: want=%d got=%d", want, got) - return - } - - for i, violation := range violations[0].Violations { - if want, got := test.expCodes[i], violation.Code; want != got { - t.Errorf("violation %d code mismatch: want=%s got=%s", i, want, got) - } - if want, got := test.expParams[i], violation.Params; !reflect.DeepEqual(want, got) { - t.Errorf("violation %d params mismatch: want=%v got=%v", i, want, got) - } - } - }) - } -} - -func inspectBranchViolations(t *testing.T, - expCodes []string, - expParams [][]any, - violations []types.RuleViolations, -) { - if len(expCodes) == 0 && - (len(violations) == 0 || len(violations) == 1 && len(violations[0].Violations) == 0) { - // no violations expected and no violations received - return - } - - if len(violations) != 1 { - t.Error("expected size of violation should always be one") - return - } - - if want, got := len(expCodes), len(violations[0].Violations); want != got { - t.Errorf("violation count: want=%d got=%d", want, got) - return - } - - for i, violation := range violations[0].Violations { - if want, got := expCodes[i], violation.Code; want != got { - t.Errorf("violation %d code mismatch: want=%s got=%s", i, want, got) - } - if want, got := expParams[i], violation.Params; !reflect.DeepEqual(want, got) { - t.Errorf("violation %d params mismatch: want=%v got=%v", i, want, got) - } - } -} diff --git a/app/services/protection/service.go b/app/services/protection/service.go index 30acd7321..4dc27711d 100644 --- a/app/services/protection/service.go +++ b/app/services/protection/service.go @@ -18,12 +18,56 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "github.com/harness/gitness/app/store" "github.com/harness/gitness/types" ) +type ( + Sanitizer interface { + // Sanitize validates if the definition is valid and automatically corrects minor issues. + Sanitize() error + } + + Protection interface { + MergeVerifier + RefChangeVerifier + } + + Definition interface { + Sanitizer + Protection + } + + // DefinitionGenerator is the function that creates blank rules. + DefinitionGenerator func() Definition + + // Manager is used to enforce protection rules. + Manager struct { + defGenMap map[types.RuleType]DefinitionGenerator + ruleStore store.RuleStore + } +) + +var ( + ErrUnrecognizedType = errors.New("unrecognized protection type") + ErrAlreadyRegistered = errors.New("protection type already registered") + ErrPatternEmpty = errors.New("pattern doesn't match anything") + ErrPatternEmptyPattern = errors.New("name pattern can't be empty") + ErrInvalidGlobstarPattern = errors.New("invalid globstar pattern") +) + +func IsCritical(violations []types.RuleViolations) bool { + for i := range violations { + if violations[i].IsCritical() { + return true + } + } + return false +} + // NewManager creates new protection Manager. func NewManager(ruleStore store.RuleStore) *Manager { return &Manager{ @@ -32,15 +76,6 @@ func NewManager(ruleStore store.RuleStore) *Manager { } } -// DefinitionGenerator is the function that creates blank rules. -type DefinitionGenerator func() Definition - -// Manager is used to enforce protection rules. -type Manager struct { - defGenMap map[types.RuleType]DefinitionGenerator - ruleStore store.RuleStore -} - // Register registers new types.RuleType. func (m *Manager) Register(ruleType types.RuleType, gen DefinitionGenerator) error { _, ok := m.defGenMap[ruleType] @@ -98,3 +133,15 @@ 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/set.go b/app/services/protection/set.go index cf15b62c6..c6765cd5d 100644 --- a/app/services/protection/set.go +++ b/app/services/protection/set.go @@ -28,8 +28,11 @@ type ruleSet struct { var _ Protection = ruleSet{} // ensure that ruleSet implements the Protection interface. -func (s ruleSet) CanMerge(ctx context.Context, in CanMergeInput) (CanMergeOutput, []types.RuleViolations, error) { - var out CanMergeOutput +func (s ruleSet) MergeVerify( + ctx context.Context, + in MergeVerifyInput, +) (MergeVerifyOutput, []types.RuleViolations, error) { + var out MergeVerifyOutput var violations []types.RuleViolations for _, r := range s.rules { @@ -47,7 +50,7 @@ func (s ruleSet) CanMerge(ctx context.Context, in CanMergeInput) (CanMergeOutput fmt.Errorf("failed to parse protection definition ID=%d Type=%s: %w", r.ID, r.Type, err) } - rOut, rVs, err := protection.CanMerge(ctx, in) + rOut, rVs, err := protection.MergeVerify(ctx, in) if err != nil { return out, nil, err } @@ -59,7 +62,7 @@ func (s ruleSet) CanMerge(ctx context.Context, in CanMergeInput) (CanMergeOutput return out, violations, nil } -func (s ruleSet) CanModifyRef(ctx context.Context, in CanModifyRefInput) ([]types.RuleViolations, error) { +func (s ruleSet) RefChangeVerify(ctx context.Context, in RefChangeVerifyInput) ([]types.RuleViolations, error) { var violations []types.RuleViolations for _, r := range s.rules { @@ -80,7 +83,7 @@ func (s ruleSet) CanModifyRef(ctx context.Context, in CanModifyRefInput) ([]type ruleIn := in ruleIn.RefNames = matched - rVs, err := protection.CanModifyRef(ctx, ruleIn) + rVs, err := protection.RefChangeVerify(ctx, ruleIn) if err != nil { return nil, err } diff --git a/app/services/protection/verify_lifecycle.go b/app/services/protection/verify_lifecycle.go new file mode 100644 index 000000000..ba14b6373 --- /dev/null +++ b/app/services/protection/verify_lifecycle.go @@ -0,0 +1,98 @@ +// 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 ( + "context" + + "github.com/harness/gitness/types" +) + +type ( + RefChangeVerifier interface { + RefChangeVerify(ctx context.Context, in RefChangeVerifyInput) ([]types.RuleViolations, error) + } + + RefChangeVerifyInput struct { + Actor *types.Principal + IsSpaceOwner bool + Repo *types.Repository + RefAction RefAction + RefType RefType + RefNames []string + } + + RefType int + + RefAction int + + DefLifecycle struct { + CreateForbidden bool `json:"create_forbidden,omitempty"` + DeleteForbidden bool `json:"delete_forbidden,omitempty"` + UpdateForbidden bool `json:"update_forbidden,omitempty"` + } +) + +const ( + RefTypeRaw RefType = iota + RefTypeBranch + RefTypeTag +) + +const ( + RefActionCreate RefAction = iota + RefActionDelete + RefActionUpdate +) + +// ensures that the DefLifecycle type implements Sanitizer and RefChangeVerifier interfaces. +var ( + _ Sanitizer = (*DefLifecycle)(nil) + _ RefChangeVerifier = (*DefLifecycle)(nil) +) + +const ( + codeLifecycleCreate = "lifecycle.create" + codeLifecycleDelete = "lifecycle.delete" + codeLifecycleUpdate = "lifecycle.update" +) + +func (v *DefLifecycle) RefChangeVerify(_ context.Context, in RefChangeVerifyInput) ([]types.RuleViolations, error) { + var violations types.RuleViolations + + switch in.RefAction { + case RefActionCreate: + if v.CreateForbidden { + violations.Addf(codeLifecycleCreate, + "Creation of branch %q is not allowed.", in.RefNames[0]) + } + case RefActionDelete: + if v.DeleteForbidden { + violations.Addf(codeLifecycleDelete, + "Delete of branch %q is not allowed.", in.RefNames[0]) + } + case RefActionUpdate: + if v.UpdateForbidden { + violations.Addf(codeLifecycleUpdate, + "Push to branch %q is not allowed. Please use pull requests.", in.RefNames[0]) + } + } + + return []types.RuleViolations{violations}, nil +} + +func (*DefLifecycle) Sanitize() error { + return nil +} diff --git a/app/services/protection/verify_lifestyle_test.go b/app/services/protection/verify_lifestyle_test.go new file mode 100644 index 000000000..cca787df0 --- /dev/null +++ b/app/services/protection/verify_lifestyle_test.go @@ -0,0 +1,114 @@ +// 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 ( + "context" + "reflect" + "testing" + + "github.com/harness/gitness/types" +) + +// nolint:gocognit // it's a unit test +func TestDefLifecycle_RefChangeVerify(t *testing.T) { + const refName = "a" + tests := []struct { + name string + def DefLifecycle + action RefAction + expCodes []string + expParams [][]any + }{ + { + name: "empty", + }, + { + name: "lifecycle.create-fail", + def: DefLifecycle{CreateForbidden: true}, + action: RefActionCreate, + expCodes: []string{"lifecycle.create"}, + expParams: [][]any{{refName}}, + }, + { + name: "lifecycle.delete-fail", + def: DefLifecycle{DeleteForbidden: true}, + action: RefActionDelete, + expCodes: []string{"lifecycle.delete"}, + expParams: [][]any{{refName}}, + }, + { + name: "lifecycle.update-fail", + def: DefLifecycle{UpdateForbidden: true}, + action: RefActionUpdate, + expCodes: []string{"lifecycle.update"}, + expParams: [][]any{{refName}}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + in := RefChangeVerifyInput{ + RefNames: []string{refName}, + RefAction: test.action, + RefType: RefTypeBranch, + } + + if err := test.def.Sanitize(); err != nil { + t.Errorf("def invalid: %s", err.Error()) + return + } + + violations, err := test.def.RefChangeVerify(context.Background(), in) + if err != nil { + t.Errorf("got an error: %s", err.Error()) + return + } + + inspectBranchViolations(t, test.expCodes, test.expParams, violations) + }) + } +} + +func inspectBranchViolations(t *testing.T, + expCodes []string, + expParams [][]any, + violations []types.RuleViolations, +) { + if len(expCodes) == 0 && + (len(violations) == 0 || len(violations) == 1 && len(violations[0].Violations) == 0) { + // no violations expected and no violations received + return + } + + if len(violations) != 1 { + t.Error("expected size of violation should always be one") + return + } + + if want, got := len(expCodes), len(violations[0].Violations); want != got { + t.Errorf("violation count: want=%d got=%d", want, got) + return + } + + for i, violation := range violations[0].Violations { + if want, got := expCodes[i], violation.Code; want != got { + t.Errorf("violation %d code mismatch: want=%s got=%s", i, want, got) + } + if want, got := expParams[i], violation.Params; !reflect.DeepEqual(want, got) { + t.Errorf("violation %d params mismatch: want=%v got=%v", i, want, got) + } + } +} diff --git a/app/services/protection/verify_pullreq.go b/app/services/protection/verify_pullreq.go new file mode 100644 index 000000000..abeed3fe7 --- /dev/null +++ b/app/services/protection/verify_pullreq.go @@ -0,0 +1,276 @@ +// 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 ( + "context" + "errors" + "fmt" + + "github.com/harness/gitness/app/services/codeowners" + "github.com/harness/gitness/types" + "github.com/harness/gitness/types/enum" + + "golang.org/x/exp/slices" +) + +type ( + MergeVerifier interface { + MergeVerify(ctx context.Context, in MergeVerifyInput) (MergeVerifyOutput, []types.RuleViolations, error) + } + + MergeVerifyInput struct { + Actor *types.Principal + IsSpaceOwner bool + Membership *types.Membership + TargetRepo *types.Repository + SourceRepo *types.Repository + PullReq *types.PullReq + Reviewers []*types.PullReqReviewer + Method enum.MergeMethod + CheckResults []types.CheckResult + CodeOwners *codeowners.Evaluation + } + + MergeVerifyOutput struct { + DeleteSourceBranch bool + } +) + +// ensures that the DefPullReq type implements Sanitizer and MergeVerifier interface. +var ( + _ Sanitizer = (*DefPullReq)(nil) + _ MergeVerifier = (*DefPullReq)(nil) +) + +const ( + codePullReqApprovalReqMinCount = "pullreq.approvals.require_minimum_count" + codePullReqApprovalReqLatestCommit = "pullreq.approvals.require_latest_commit" + codePullReqApprovalReqCodeOwnersNoApproval = "pullreq.approvals.require_code_owners:no_approval" + codePullReqApprovalReqCodeOwnersChangeRequested = "pullreq.approvals.require_code_owners:change_requested" + codePullReqApprovalReqCodeOwnersNoLatestApproval = "pullreq.approvals.require_code_owners:no_latest_approval" + codePullReqCommentsReqResolveAll = "pullreq.comments.require_resolve_all" + codePullReqStatusChecksReqUIDs = "pullreq.status_checks.required_uids" + codePullReqMergeStrategiesAllowed = "pullreq.merge.strategies_allowed" + codePullReqMergeDeleteBranch = "pullreq.merge.delete_branch" +) + +//nolint:gocognit // well aware of this +func (v *DefPullReq) MergeVerify( + _ context.Context, + in MergeVerifyInput, +) (MergeVerifyOutput, []types.RuleViolations, error) { + var out MergeVerifyOutput + var violations types.RuleViolations + + out.DeleteSourceBranch = v.Merge.DeleteBranch + + // pullreq.approvals + + approvedBy := make([]types.PrincipalInfo, 0, len(in.Reviewers)) + for _, reviewer := range in.Reviewers { + if reviewer.ReviewDecision != enum.PullReqReviewDecisionApproved { + continue + } + if v.Approvals.RequireLatestCommit && reviewer.SHA != in.PullReq.SourceSHA { + continue + } + + approvedBy = append(approvedBy, reviewer.Reviewer) + } + + if len(approvedBy) < v.Approvals.RequireMinimumCount { + violations.Addf(codePullReqApprovalReqMinCount, + "Insufficient number of approvals. Have %d but need at least %d.", + len(approvedBy), v.Approvals.RequireMinimumCount) + } + + if v.Approvals.RequireCodeOwners { + for _, entry := range in.CodeOwners.EvaluationEntries { + reviewDecision, approvers := getCodeOwnerApprovalStatus(entry.OwnerEvaluations) + + if reviewDecision == enum.PullReqReviewDecisionPending { + violations.Addf(codePullReqApprovalReqCodeOwnersNoApproval, + "Code owners approval pending for %q", entry.Pattern) + continue + } + + if reviewDecision == enum.PullReqReviewDecisionChangeReq { + violations.Addf(codePullReqApprovalReqCodeOwnersChangeRequested, + "Code owners requested changes for %q", entry.Pattern) + continue + } + + // pull req approved. check other settings + if !v.Approvals.RequireLatestCommit { + continue + } + latestSHAApproved := slices.ContainsFunc(approvers, func(ev codeowners.OwnerEvaluation) bool { + return ev.ReviewSHA == in.PullReq.SourceSHA + }) + if !latestSHAApproved { + violations.Addf(codePullReqApprovalReqCodeOwnersNoLatestApproval, + "Code owners approval pending on latest commit for %q", entry.Pattern) + } + } + } + + // pullreq.comments + + if v.Comments.RequireResolveAll && in.PullReq.UnresolvedCount > 0 { + violations.Addf(codePullReqCommentsReqResolveAll, + "All comments must be resolved. There are %d unresolved comments.", + in.PullReq.UnresolvedCount) + } + + // pullreq.status_checks + + for _, requiredUID := range v.StatusChecks.RequireUIDs { + var succeeded bool + + for i := range in.CheckResults { + if in.CheckResults[i].UID == requiredUID { + succeeded = in.CheckResults[i].Status == enum.CheckStatusSuccess + break + } + } + + if !succeeded { + violations.Add(codePullReqStatusChecksReqUIDs, + "At least one required status check hasn't completed successfully.") + } + } + + // pullreq.merge + + if len(v.Merge.StrategiesAllowed) > 0 { // Note: Empty allowed strategies list means all are allowed + if !slices.Contains(v.Merge.StrategiesAllowed, in.Method) { + violations.Addf(codePullReqMergeStrategiesAllowed, + "The requested merge strategy %q is not allowed. Allowed strategies are %v.", + in.Method, v.Merge.StrategiesAllowed) + } + } + + return out, []types.RuleViolations{violations}, nil +} + +type DefApprovals struct { + RequireCodeOwners bool `json:"require_code_owners,omitempty"` + RequireMinimumCount int `json:"require_minimum_count,omitempty"` + RequireLatestCommit bool `json:"require_latest_commit,omitempty"` +} + +func (v *DefApprovals) Sanitize() error { + if v.RequireMinimumCount < 0 { + return errors.New("minimum count must be zero or a positive integer") + } + + return nil +} + +type DefComments struct { + RequireResolveAll bool `json:"require_resolve_all,omitempty"` +} + +func (DefComments) Sanitize() error { + return nil +} + +type DefStatusChecks struct { + RequireUIDs []string `json:"require_uids,omitempty"` +} + +func (v *DefStatusChecks) Sanitize() error { + if err := validateUIDSlice(v.RequireUIDs); err != nil { + return fmt.Errorf("required UIDs error: %w", err) + } + + return nil +} + +type DefMerge struct { + StrategiesAllowed []enum.MergeMethod `json:"strategies_allowed,omitempty"` + DeleteBranch bool `json:"delete_branch,omitempty"` +} + +func (v *DefMerge) Sanitize() error { + m := make(map[enum.MergeMethod]struct{}, 0) + for _, strategy := range v.StrategiesAllowed { + if _, ok := strategy.Sanitize(); !ok { + return fmt.Errorf("unrecognized merge strategy: %s", strategy) + } + + if _, ok := m[strategy]; ok { + return fmt.Errorf("duplicate entry in merge strategy list: %s", strategy) + } + + m[strategy] = struct{}{} + } + + return nil +} + +type DefPush struct { + Block bool `json:"block,omitempty"` +} + +func (v *DefPush) Sanitize() error { + return nil +} + +type DefPullReq struct { + Approvals DefApprovals `json:"approvals"` + Comments DefComments `json:"comments"` + StatusChecks DefStatusChecks `json:"status_checks"` + Merge DefMerge `json:"merge"` +} + +func (v *DefPullReq) Sanitize() error { + if err := v.Approvals.Sanitize(); err != nil { + return fmt.Errorf("approvals: %w", err) + } + + if err := v.Comments.Sanitize(); err != nil { + return fmt.Errorf("comments: %w", err) + } + + if err := v.StatusChecks.Sanitize(); err != nil { + return fmt.Errorf("status checks: %w", err) + } + + if err := v.Merge.Sanitize(); err != nil { + return fmt.Errorf("merge: %w", err) + } + + return nil +} + +func getCodeOwnerApprovalStatus( + ownerStatus []codeowners.OwnerEvaluation, +) (enum.PullReqReviewDecision, []codeowners.OwnerEvaluation) { + approvers := make([]codeowners.OwnerEvaluation, 0) + for _, o := range ownerStatus { + if o.ReviewDecision == enum.PullReqReviewDecisionChangeReq { + return enum.PullReqReviewDecisionChangeReq, nil + } + if o.ReviewDecision == enum.PullReqReviewDecisionApproved { + approvers = append(approvers, o) + } + } + if len(approvers) > 0 { + return enum.PullReqReviewDecisionApproved, approvers + } + return enum.PullReqReviewDecisionPending, nil +} diff --git a/app/services/protection/verify_pullreq_test.go b/app/services/protection/verify_pullreq_test.go new file mode 100644 index 000000000..8d6468f13 --- /dev/null +++ b/app/services/protection/verify_pullreq_test.go @@ -0,0 +1,295 @@ +// 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 ( + "context" + "reflect" + "testing" + + "github.com/harness/gitness/app/services/codeowners" + gitrpcenum "github.com/harness/gitness/gitrpc/enum" + "github.com/harness/gitness/types" + "github.com/harness/gitness/types/enum" +) + +// nolint:gocognit // it's a unit test +func TestDefPullReq_MergeVerify(t *testing.T) { + tests := []struct { + name string + def DefPullReq + in MergeVerifyInput + expCodes []string + expParams [][]any + expOut MergeVerifyOutput + }{ + { + name: "empty", + }, + { + name: codePullReqApprovalReqMinCount + "-fail", + def: DefPullReq{Approvals: DefApprovals{RequireMinimumCount: 1}}, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionChangeReq, SHA: "abc"}, + }, + }, + expCodes: []string{codePullReqApprovalReqMinCount}, + expParams: [][]any{{0, 1}}, + }, + { + name: codePullReqApprovalReqMinCount + "-success", + def: DefPullReq{Approvals: DefApprovals{RequireMinimumCount: 2}}, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, + }, + }, + }, + { + name: codePullReqApprovalReqLatestCommit + "-fail", + def: DefPullReq{Approvals: DefApprovals{RequireMinimumCount: 2, RequireLatestCommit: true}}, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abd"}, + }, + }, + expCodes: []string{codePullReqApprovalReqMinCount}, + expParams: [][]any{{1, 2}}, + }, + { + name: codePullReqApprovalReqLatestCommit + "-success", + def: DefPullReq{Approvals: DefApprovals{RequireMinimumCount: 2, RequireLatestCommit: true}}, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + Reviewers: []*types.PullReqReviewer{ + {ReviewDecision: enum.PullReqReviewDecisionPending, SHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, SHA: "abc"}, + }, + }, + }, + { + name: codePullReqApprovalReqCodeOwnersNoApproval + "-fail", + def: DefPullReq{Approvals: DefApprovals{RequireCodeOwners: true}}, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + CodeOwners: &codeowners.Evaluation{ + EvaluationEntries: []codeowners.EvaluationEntry{ + { + Pattern: "app", + OwnerEvaluations: []codeowners.OwnerEvaluation{ + {ReviewDecision: enum.PullReqReviewDecisionPending, ReviewSHA: "abc"}, + }, + }, + { + Pattern: "doc", + OwnerEvaluations: []codeowners.OwnerEvaluation{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, ReviewSHA: "abc"}, + }, + }, + { + Pattern: "data", + OwnerEvaluations: []codeowners.OwnerEvaluation{}, + }, + }, + FileSha: "xyz", + }, + }, + expCodes: []string{ + codePullReqApprovalReqCodeOwnersNoApproval, + codePullReqApprovalReqCodeOwnersNoApproval, + }, + expParams: [][]any{{"app"}, {"data"}}, + }, + { + name: codePullReqApprovalReqCodeOwnersNoApproval + "-success", + def: DefPullReq{Approvals: DefApprovals{RequireCodeOwners: true}}, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + CodeOwners: &codeowners.Evaluation{ + EvaluationEntries: []codeowners.EvaluationEntry{ + { + Pattern: "app", + OwnerEvaluations: []codeowners.OwnerEvaluation{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, ReviewSHA: "abc"}, + }, + }, + { + Pattern: "doc", + OwnerEvaluations: []codeowners.OwnerEvaluation{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, ReviewSHA: "abc"}, + }, + }, + }, + FileSha: "xyz", + }, + }, + }, + { + name: codePullReqApprovalReqCodeOwnersChangeRequested + "-fail", + def: DefPullReq{Approvals: DefApprovals{RequireCodeOwners: true}}, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + CodeOwners: &codeowners.Evaluation{ + EvaluationEntries: []codeowners.EvaluationEntry{ + { + Pattern: "app", + OwnerEvaluations: []codeowners.OwnerEvaluation{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, ReviewSHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionChangeReq, ReviewSHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionPending, ReviewSHA: "abc"}, + }, + }, + { + Pattern: "data", + OwnerEvaluations: []codeowners.OwnerEvaluation{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, ReviewSHA: "abc"}, + }, + }, + }, + FileSha: "xyz", + }, + }, + expCodes: []string{codePullReqApprovalReqCodeOwnersChangeRequested}, + expParams: [][]any{{"app"}}, + }, + { + name: codePullReqApprovalReqCodeOwnersNoLatestApproval + "-fail", + def: DefPullReq{Approvals: DefApprovals{RequireCodeOwners: true, RequireLatestCommit: true}}, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0, SourceSHA: "abc"}, + CodeOwners: &codeowners.Evaluation{ + EvaluationEntries: []codeowners.EvaluationEntry{ + { + Pattern: "data", + OwnerEvaluations: []codeowners.OwnerEvaluation{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, ReviewSHA: "old"}, + }, + }, + { + Pattern: "app", + OwnerEvaluations: []codeowners.OwnerEvaluation{ + {ReviewDecision: enum.PullReqReviewDecisionApproved, ReviewSHA: "abc"}, + {ReviewDecision: enum.PullReqReviewDecisionApproved, ReviewSHA: "old"}, + }, + }, + }, + FileSha: "xyz", + }, + }, + expCodes: []string{codePullReqApprovalReqCodeOwnersNoLatestApproval}, + expParams: [][]any{{"data"}}, + }, + { + name: codePullReqCommentsReqResolveAll + "-fail", + def: DefPullReq{Comments: DefComments{RequireResolveAll: true}}, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 6}, + }, + expCodes: []string{"pullreq.comments.require_resolve_all"}, + expParams: [][]any{{6}}, + }, + { + name: codePullReqCommentsReqResolveAll + "-success", + def: DefPullReq{Comments: DefComments{RequireResolveAll: true}}, + in: MergeVerifyInput{ + PullReq: &types.PullReq{UnresolvedCount: 0}, + }, + }, + { + name: codePullReqStatusChecksReqUIDs + "-fail", + def: DefPullReq{StatusChecks: DefStatusChecks{RequireUIDs: []string{"check1"}}}, + in: MergeVerifyInput{ + CheckResults: []types.CheckResult{ + {UID: "check1", Status: enum.CheckStatusFailure}, + {UID: "check2", Status: enum.CheckStatusSuccess}, + }, + }, + expCodes: []string{codePullReqStatusChecksReqUIDs}, + expParams: [][]any{nil}, + }, + { + name: codePullReqStatusChecksReqUIDs + "-success", + def: DefPullReq{StatusChecks: DefStatusChecks{RequireUIDs: []string{"check1"}}}, + in: MergeVerifyInput{ + CheckResults: []types.CheckResult{ + {UID: "check1", Status: enum.CheckStatusSuccess}, + {UID: "check2", Status: enum.CheckStatusFailure}, + }, + }, + }, + { + name: codePullReqMergeStrategiesAllowed + "-fail", + def: DefPullReq{Merge: DefMerge{StrategiesAllowed: []enum.MergeMethod{ + enum.MergeMethod(gitrpcenum.MergeMethodRebase), + enum.MergeMethod(gitrpcenum.MergeMethodSquash), + }}}, + in: MergeVerifyInput{ + Method: enum.MergeMethod(gitrpcenum.MergeMethodMerge), + }, + expCodes: []string{codePullReqMergeStrategiesAllowed}, + expParams: [][]any{{ + enum.MergeMethod(gitrpcenum.MergeMethodMerge), + []enum.MergeMethod{ + enum.MergeMethod(gitrpcenum.MergeMethodRebase), + enum.MergeMethod(gitrpcenum.MergeMethodSquash), + }}, + }, + }, + { + name: codePullReqMergeStrategiesAllowed + "-success", + def: DefPullReq{Merge: DefMerge{StrategiesAllowed: []enum.MergeMethod{ + enum.MergeMethod(gitrpcenum.MergeMethodRebase), + enum.MergeMethod(gitrpcenum.MergeMethodSquash), + }}}, + in: MergeVerifyInput{ + Method: enum.MergeMethod(gitrpcenum.MergeMethodSquash), + }, + }, + { + name: codePullReqMergeDeleteBranch, + def: DefPullReq{Merge: DefMerge{DeleteBranch: true}}, + in: MergeVerifyInput{}, + expOut: MergeVerifyOutput{DeleteSourceBranch: true}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if err := test.def.Sanitize(); err != nil { + t.Errorf("def invalid: %s", err.Error()) + return + } + + out, violations, err := test.def.MergeVerify(context.Background(), test.in) + if err != nil { + t.Errorf("got an error: %s", err.Error()) + return + } + + if want, got := test.expOut, out; !reflect.DeepEqual(want, got) { + t.Errorf("output mismatch: want=%+v got=%+v", want, got) + } + + inspectBranchViolations(t, test.expCodes, test.expParams, violations) + }) + } +}