Fix policy settings

Policy setting was broken, this would always get the other ACLs check to
run even when the user was not there.

The tests was also not testing the policy setting properly.

The E2E tests is hard to test on GitHub since we need to create another
two others users and another organisation with a bunch of teams to test
it properly. Hopefully if we can get our own GitHub enterprise instance
we would be able to do those in the future.

I have done those tests manually:

An organisation with three users:

chmouel
sam
rubben

A team with the users sam and chmouel in there but not rubben.

I have created a repo CR with the policy settings like this:

```yaml
  settings:
    policy:
      pull_request:
      - pull_request
```

the user sam gets allowed to run CI but the user rubben get disallowed.

If I issue a comment with /ok-to-test from the user sam on the rubben PR
it will be disallowed (since it only to run the pr and not issue
ok-to-test).

I changed the policy on the repo settings with the ok-to-test section
allowing the group pull_request:

```yaml
  settings:
    policy:
      ok_to_test:
      - pull_request
      pull_request:
      - pull_request
```

and then I can issue /ok-to-test from the user sam on the user rubben
and it will be tested.

Signed-off-by: Chmouel Boudjnah <[email protected]>

Author: chmouel

pkg/policy/policy.go

@@ -27,13 +27,13 @@ type Policy struct {
 	EventEmitter *events.EventEmitter
 }
 
-func (p *Policy) checkAllowed(ctx context.Context, tType info.TriggerType) (Result, error) {
+func (p *Policy) checkAllowed(ctx context.Context, tType info.TriggerType) (Result, string) {
 	if p.Repository == nil {
-		return ResultNotSet, nil
+		return ResultNotSet, ""
 	}
 	settings := p.Repository.Spec.Settings
 	if settings == nil || settings.Policy == nil {
-		return ResultNotSet, nil
+		return ResultNotSet, ""
 	}
 
 	var sType []string
@@ -45,42 +45,61 @@ func (p *Policy) checkAllowed(ctx context.Context, tType info.TriggerType) (Resu
 		sType = settings.Policy.PullRequest
 	// NOTE: not supported yet, will imp if it gets requested and reasonable to implement
 	case info.TriggerTypePush, info.TriggerTypeCancel, info.TriggerTypeCheckSuiteRerequested, info.TriggerTypeCheckRunRerequested:
-		return ResultNotSet, nil
+		return ResultNotSet, ""
 	default:
-		return ResultNotSet, nil
+		return ResultNotSet, ""
 	}
 
+	// if policy is set but empty then it mean disallow everything
 	if len(sType) == 0 {
-		return ResultNotSet, nil
+		return ResultDisallowed, "no policy set"
 	}
 
-	allowed, reason := p.VCX.CheckPolicyAllowing(ctx, p.Event, sType)
-	reasonMsg := fmt.Sprintf("policy check: %s, %s", string(tType), reason)
-	if reason != "" {
-		p.EventEmitter.EmitMessage(p.Repository, zap.InfoLevel, "PolicyCheck", reasonMsg)
+	// remove empty values from sType
+	temp := []string{}
+	for _, val := range sType {
+		if val != "" {
+			temp = append(temp, val)
+		}
+	}
+	sType = temp
+
+	// if policy is set but with empty values then bail out.
+	if len(sType) == 0 {
+		return ResultDisallowed, "policy set and empty with no groups"
 	}
+
+	allowed, reason := p.VCX.CheckPolicyAllowing(ctx, p.Event, sType)
 	if allowed {
-		return ResultAllowed, nil
+		return ResultAllowed, ""
 	}
-	return ResultDisallowed, fmt.Errorf(reasonMsg)
+	return ResultDisallowed, fmt.Sprintf("policy check: %s, %s", string(tType), reason)
 }
 
-func (p *Policy) IsAllowed(ctx context.Context, tType info.TriggerType) (bool, string) {
+func (p *Policy) IsAllowed(ctx context.Context, tType info.TriggerType) (Result, string) {
 	var reason string
-	policyRes, err := p.checkAllowed(ctx, tType)
-	if err != nil {
-		return false, err.Error()
-	}
+	policyRes, reason := p.checkAllowed(ctx, tType)
 	switch policyRes {
 	case ResultAllowed:
 		reason = fmt.Sprintf("policy check: policy is set for sender %s has been allowed to run CI via policy", p.Event.Sender)
 		p.EventEmitter.EmitMessage(p.Repository, zap.InfoLevel, "PolicySetAllowed", reason)
-		return true, ""
+		return ResultAllowed, ""
 	case ResultDisallowed:
-		reason = fmt.Sprintf("policy check: policy is set but sender %s is not in the allowed groups trying the next ACL conditions", p.Event.Sender)
+		allowed, err := p.VCX.IsAllowedOwnersFile(ctx, p.Event)
+		if err != nil {
+			return ResultDisallowed, err.Error()
+		}
+		if allowed {
+			reason = fmt.Sprintf("policy check: policy is set, sender %s not in the allowed policy but allowed via OWNERS file", p.Event.Sender)
+			p.EventEmitter.EmitMessage(p.Repository, zap.InfoLevel, "PolicySetAllowed", reason)
+			return ResultAllowed, ""
+		}
+		if reason == "" {
+			reason = fmt.Sprintf("policy check: policy is set but sender %s is not in the allowed groups", p.Event.Sender)
+		}
 		p.EventEmitter.EmitMessage(p.Repository, zap.InfoLevel, "PolicySetDisallowed", reason)
-	case ResultNotSet:
-		// should we put a warning here? it does fill up quite a bit the log every time! so I am not so sure..
+		return ResultDisallowed, ""
+	case ResultNotSet: // this is to make golangci-lint happy
 	}
-	return false, reason
+	return ResultNotSet, reason
 }

pkg/policy/policy_test.go

@@ -1,6 +1,8 @@
 package policy
 
 import (
+	"fmt"
+	"strings"
 	"testing"
 
 	"go.uber.org/zap"
@@ -26,6 +28,10 @@ func newRepoWithPolicy(policy *v1alpha1.Policy) *v1alpha1.Repository {
 }
 
 func TestPolicy_IsAllowed(t *testing.T) {
+	senderName := "sender"
+	eventWithSender := info.NewEvent()
+	eventWithSender.Sender = senderName
+
 	type fields struct {
 		repository *v1alpha1.Repository
 		event      *info.Event
@@ -34,130 +40,180 @@ func TestPolicy_IsAllowed(t *testing.T) {
 		tType info.TriggerType
 	}
 	tests := []struct {
-		name         string
-		fields       fields
-		args         args
-		replyAllowed bool
-		want         bool
-		wantErr      bool
-		wantReason   string
+		name                 string
+		fields               fields
+		args                 args
+		vcsReplyAllowed      bool
+		want                 Result
+		wantErr              bool
+		wantReason           string
+		allowedInOwnersFile  bool
+		expectedLogsSnippets []string
 	}{
 		{
-			name: "Test Policy.IsAllowed with no settings",
+			name: "notset/not set",
 			fields: fields{
 				repository: nil,
 				event:      nil,
 			},
 			args: args{
 				tType: info.TriggerTypePush,
 			},
-			want: false,
+			want: ResultNotSet,
 		},
 		{
-			name: "Test Policy.IsAllowed with unknown event type",
+			name: "notset/unknown event type",
 			fields: fields{
 				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"pull_request"}}),
 				event:      nil,
 			},
 			args: args{
 				tType: "unknown",
 			},
-			want: false,
+			want: ResultNotSet,
 		},
 		{
-			name: "allowing member not in team for pull request",
+			name: "notset/push",
 			fields: fields{
 				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"pull_request"}}),
-				event:      info.NewEvent(),
+				event:      nil,
 			},
 			args: args{
-				tType: info.TriggerTypePullRequest,
+				tType: info.TriggerTypePush,
 			},
-			replyAllowed: true,
-			want:         true,
+			want: ResultNotSet,
 		},
 		{
-			name:       "empty settings policy ignore",
-			wantReason: "policy check: pull_request, policy disallowing",
+			name: "allowed/allowing member for pull request",
 			fields: fields{
-				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{""}}),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"pull_request"}}),
 				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypePullRequest,
 			},
-			want: false,
+			vcsReplyAllowed: true,
+			want:            ResultAllowed,
 		},
 		{
-			name:       "disallowing member not in team for pull request",
-			wantReason: "policy check: pull_request, policy disallowing",
+			name: "allowed/from owners file",
 			fields: fields{
 				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"pull_request"}}),
-				event:      info.NewEvent(),
+				event:      eventWithSender,
 			},
 			args: args{
 				tType: info.TriggerTypePullRequest,
 			},
-			want:    false,
-			wantErr: true,
+			vcsReplyAllowed:     false,
+			allowedInOwnersFile: true,
+			want:                ResultAllowed,
+			expectedLogsSnippets: []string{
+				fmt.Sprintf("policy check: policy is set, sender %s not in the allowed policy but allowed via OWNERS file", senderName),
+			},
 		},
 		{
-			name: "allowing member in team for ok-to-test",
+			name: "allowed/member in team for ok-to-test",
 			fields: fields{
-				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"ok-to-test"}}),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{OkToTest: []string{"ok-to-test"}}),
 				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypeOkToTest,
 			},
-			replyAllowed: true,
-			want:         false,
+			vcsReplyAllowed: true,
+			want:            ResultAllowed,
+		},
+		{
+			name: "allowed/retest same as ok-to-test",
+			fields: fields{
+				repository: newRepoWithPolicy(&v1alpha1.Policy{OkToTest: []string{"ok-to-test"}}),
+				event:      info.NewEvent(),
+			},
+			args: args{
+				tType: info.TriggerTypeRetest,
+			},
+			vcsReplyAllowed: true,
+			want:            ResultAllowed,
+		},
+		{
+			name:                 "disallowed/policy set with empty list",
+			expectedLogsSnippets: []string{"policy set and empty with no groups"},
+			fields: fields{
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{""}}),
+				event:      eventWithSender,
+			},
+			args: args{
+				tType: info.TriggerTypePullRequest,
+			},
+			want: ResultDisallowed,
+		},
+		{
+			name: "disallowed/member not in team for pull request",
+			fields: fields{
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"pull_request"}}),
+				event:      info.NewEvent(),
+			},
+			args: args{
+				tType: info.TriggerTypePullRequest,
+			},
+			want:                 ResultDisallowed,
+			wantErr:              true,
+			expectedLogsSnippets: []string{"policy check: pull_request, policy disallowing"},
 		},
 		{
-			name: "disallowing member not in team for ok-to-test",
+			name: "disallowed/member not in team for ok-to-test",
 			fields: fields{
-				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"ok-to-test"}}),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{OkToTest: []string{"nono"}}),
 				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypeOkToTest,
 			},
-			want:    false,
-			wantErr: true,
+			want:                 ResultDisallowed,
+			wantErr:              true,
+			expectedLogsSnippets: []string{"policy check: ok-to-test, policy disallowing"},
 		},
 		{
-			name: "allowing member not in team for retest",
+			name: "disallowed/member not in team",
 			fields: fields{
-				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"ok-to-test"}}),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{OkToTest: []string{"ok-to-test"}}),
 				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypeRetest,
 			},
-			want: false,
+			want:                 ResultDisallowed,
+			wantErr:              true,
+			vcsReplyAllowed:      false,
+			expectedLogsSnippets: []string{"policy check: retest, policy disallowing"},
 		},
 		{
-			name: "disallowing member not in team for retest",
+			name: "disallowed/from owners file",
 			fields: fields{
-				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"ok-to-test"}}),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"pull_request"}}),
 				event:      info.NewEvent(),
 			},
 			args: args{
-				tType: info.TriggerTypeRetest,
+				tType: info.TriggerTypePullRequest,
 			},
-			want:    false,
-			wantErr: true,
+			vcsReplyAllowed:      false,
+			allowedInOwnersFile:  false,
+			want:                 ResultDisallowed,
+			expectedLogsSnippets: []string{"policy check: pull_request, policy disallowing"},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			observer, _ := zapobserver.New(zap.InfoLevel)
+			observer, log := zapobserver.New(zap.InfoLevel)
 			logger := zap.New(observer).Sugar()
 
 			ctx, _ := rtesting.SetupFakeContext(t)
 			stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{})
 
-			vcx := &testprovider.TestProviderImp{PolicyDisallowing: !tt.replyAllowed}
+			vcx := &testprovider.TestProviderImp{
+				PolicyDisallowing:   !tt.vcsReplyAllowed,
+				AllowedInOwnersFile: tt.allowedInOwnersFile,
+			}
 			if tt.fields.event == nil {
 				tt.fields.event = info.NewEvent()
 			}
@@ -169,10 +225,20 @@ func TestPolicy_IsAllowed(t *testing.T) {
 				EventEmitter: events.NewEventEmitter(stdata.Kube, logger),
 			}
 			got, reason := p.IsAllowed(ctx, tt.args.tType)
-			if got != tt.want {
-				t.Errorf("Policy.IsAllowed() = %v, want %v", got, tt.want)
+			switch got {
+			case ResultAllowed:
+				assert.Equal(t, tt.want, ResultAllowed)
+			case ResultDisallowed:
+				assert.Equal(t, tt.want, ResultDisallowed)
+			case ResultNotSet:
+				assert.Equal(t, tt.want, ResultNotSet)
 			}
 			assert.Equal(t, tt.wantReason, reason)
+
+			for k, snippet := range tt.expectedLogsSnippets {
+				logmsg := log.AllUntimed()[k].Message
+				assert.Assert(t, strings.Contains(logmsg, snippet), "\n on index: %d\n we want: %s\n we  got: %s", k, snippet, logmsg)
+			}
 		})
 	}
 }