Fallback to other ACL rules if the policy is set

On failure we were not falling back to other ACL rules and we were not
checking if the other ACL rules were matching (like OWNERS file).

Tests is more comprehensive and check that we are falling back properly
and as well that the target is matching the OWNERS file.

We now properly dump the errors/allowance in the kube events streams.

Try to make sharing functions between gitea and GitHub as much as
possible.

Fix https://issues.redhat.com/browse/SRVKP-3542

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

Author: chmouel

hack/dev/kind/install.sh

@@ -10,7 +10,7 @@
 # You probably need to install passwordstore https://www.passwordstore.org/ and
 # add your github secrets : github-application-id github-private-key
 # webhook.secret in a folder which needs to be specified  in
-# the PAC_PASS_SECRET_FOLDER variable. or otheriwse you have to create the
+# the PAC_PASS_SECRET_FOLDER variable. or otherwise you have to create the
 # pipelines-as-code-secret manually
 #
 # If you need to install old pac as shipped in OSP1.7, you check it out somewhere

pkg/pipelineascode/match.go

@@ -92,7 +92,7 @@ is that what you want? make sure you use -n when generating the secret, eg: echo
 
 	// Set the client, we should error out if there is a problem with
 	// token or secret or we won't be able to do much.
-	err = p.vcx.SetClient(ctx, p.run, p.event, repo.Spec.Settings)
+	err = p.vcx.SetClient(ctx, p.run, p.event, repo, p.eventEmitter)
 	if err != nil {
 		return repo, err
 	}

pkg/policy/policy.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	"go.uber.org/zap"
@@ -19,24 +20,29 @@ const (
 )
 
 type Policy struct {
-	Settings *v1alpha1.Settings
-	Event    *info.Event
-	VCX      provider.Interface
-	Logger   *zap.SugaredLogger
+	Repository   *v1alpha1.Repository
+	Event        *info.Event
+	VCX          provider.Interface
+	Logger       *zap.SugaredLogger
+	EventEmitter *events.EventEmitter
 }
 
-func (p *Policy) IsAllowed(ctx context.Context, tType info.TriggerType) (Result, error) {
-	if p.Settings == nil || p.Settings.Policy == nil {
+func (p *Policy) checkAllowed(ctx context.Context, tType info.TriggerType) (Result, error) {
+	if p.Repository == nil {
+		return ResultNotSet, nil
+	}
+	settings := p.Repository.Spec.Settings
+	if settings == nil || settings.Policy == nil {
 		return ResultNotSet, nil
 	}
 
 	var sType []string
 	switch tType {
-	// NOTE: This make /retest /ok-to-test /test bound to the same policy, which is fine from a security standpoint but maybe we want to refind
+	// NOTE: This make /retest /ok-to-test /test bound to the same policy, which is fine from a security standpoint but maybe we want to refine this in the future.
 	case info.TriggerTypeOkToTest, info.TriggerTypeRetest:
-		sType = p.Settings.Policy.OkToTest
+		sType = settings.Policy.OkToTest
 	case info.TriggerTypePullRequest:
-		sType = p.Settings.Policy.PullRequest
+		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
@@ -51,10 +57,30 @@ func (p *Policy) IsAllowed(ctx context.Context, tType info.TriggerType) (Result,
 	allowed, reason := p.VCX.CheckPolicyAllowing(ctx, p.Event, sType)
 	reasonMsg := fmt.Sprintf("policy check: %s, %s", string(tType), reason)
 	if reason != "" {
-		p.Logger.Info(reasonMsg)
+		p.EventEmitter.EmitMessage(p.Repository, zap.InfoLevel, "PolicyCheck", reasonMsg)
 	}
 	if allowed {
 		return ResultAllowed, nil
 	}
 	return ResultDisallowed, fmt.Errorf(reasonMsg)
 }
+
+func (p *Policy) IsAllowed(ctx context.Context, tType info.TriggerType) (bool, string) {
+	var reason string
+	policyRes, err := p.checkAllowed(ctx, tType)
+	if err != nil {
+		return false, err.Error()
+	}
+	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, ""
+	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)
+		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 false, reason
+}

pkg/policy/policy_test.go

@@ -3,18 +3,32 @@ package policy
 import (
 	"testing"
 
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
-	testprovider "github.com/openshift-pipelines/pipelines-as-code/pkg/test/provider"
 	"go.uber.org/zap"
 	zapobserver "go.uber.org/zap/zaptest/observer"
+	"gotest.tools/v3/assert"
 	rtesting "knative.dev/pkg/reconciler/testing"
+
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
+	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
+	testprovider "github.com/openshift-pipelines/pipelines-as-code/pkg/test/provider"
 )
 
+func newRepoWithPolicy(policy *v1alpha1.Policy) *v1alpha1.Repository {
+	return &v1alpha1.Repository{
+		Spec: v1alpha1.RepositorySpec{
+			Settings: &v1alpha1.Settings{
+				Policy: policy,
+			},
+		},
+	}
+}
+
 func TestPolicy_IsAllowed(t *testing.T) {
 	type fields struct {
-		Settings *v1alpha1.Settings
-		Event    *info.Event
+		repository *v1alpha1.Repository
+		event      *info.Event
 	}
 	type args struct {
 		tType info.TriggerType
@@ -24,172 +38,141 @@ func TestPolicy_IsAllowed(t *testing.T) {
 		fields       fields
 		args         args
 		replyAllowed bool
-		want         Result
+		want         bool
 		wantErr      bool
+		wantReason   string
 	}{
 		{
 			name: "Test Policy.IsAllowed with no settings",
 			fields: fields{
-				Settings: nil,
-				Event:    nil,
+				repository: nil,
+				event:      nil,
 			},
 			args: args{
 				tType: info.TriggerTypePush,
 			},
-			want: ResultNotSet,
+			want: false,
 		},
 		{
 			name: "Test Policy.IsAllowed with unknown event type",
 			fields: fields{
-				Settings: &v1alpha1.Settings{
-					Policy: &v1alpha1.Policy{
-						PullRequest: []string{"pull_request"},
-					},
-				},
-				Event: nil,
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"pull_request"}}),
+				event:      nil,
 			},
 			args: args{
 				tType: "unknown",
 			},
-			want: ResultNotSet,
+			want: false,
 		},
 		{
 			name: "allowing member not in team for pull request",
 			fields: fields{
-				Settings: &v1alpha1.Settings{
-					Policy: &v1alpha1.Policy{
-						PullRequest: []string{"pull_request"},
-					},
-				},
-				Event: info.NewEvent(),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"pull_request"}}),
+				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypePullRequest,
 			},
 			replyAllowed: true,
-			want:         ResultAllowed,
+			want:         true,
 		},
 		{
-			name: "empty settings policy ignore",
+			name:       "empty settings policy ignore",
+			wantReason: "policy check: pull_request, policy disallowing",
 			fields: fields{
-				Settings: &v1alpha1.Settings{
-					Policy: &v1alpha1.Policy{
-						PullRequest: []string{},
-					},
-				},
-				Event: info.NewEvent(),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{""}}),
+				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypePullRequest,
 			},
-			replyAllowed: true,
-			want:         ResultNotSet,
+			want: false,
 		},
 		{
-			name: "disallowing member not in team for pull request",
+			name:       "disallowing member not in team for pull request",
+			wantReason: "policy check: pull_request, policy disallowing",
 			fields: fields{
-				Settings: &v1alpha1.Settings{
-					Policy: &v1alpha1.Policy{
-						PullRequest: []string{"pull_request"},
-					},
-				},
-				Event: info.NewEvent(),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"pull_request"}}),
+				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypePullRequest,
 			},
-			replyAllowed: false,
-			want:         ResultDisallowed,
-			wantErr:      true,
+			want:    false,
+			wantErr: true,
 		},
 		{
-			name: "allowing member not in team for ok-to-test",
+			name: "allowing member in team for ok-to-test",
 			fields: fields{
-				Settings: &v1alpha1.Settings{
-					Policy: &v1alpha1.Policy{
-						OkToTest: []string{"ok-to-test"},
-					},
-				},
-				Event: info.NewEvent(),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"ok-to-test"}}),
+				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypeOkToTest,
 			},
 			replyAllowed: true,
-			want:         ResultAllowed,
+			want:         false,
 		},
 		{
 			name: "disallowing member not in team for ok-to-test",
 			fields: fields{
-				Settings: &v1alpha1.Settings{
-					Policy: &v1alpha1.Policy{
-						OkToTest: []string{"ok-to-test"},
-					},
-				},
-				Event: info.NewEvent(),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"ok-to-test"}}),
+				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypeOkToTest,
 			},
-			replyAllowed: false,
-			want:         ResultDisallowed,
-			wantErr:      true,
+			want:    false,
+			wantErr: true,
 		},
 		{
 			name: "allowing member not in team for retest",
 			fields: fields{
-				Settings: &v1alpha1.Settings{
-					Policy: &v1alpha1.Policy{
-						OkToTest: []string{"ok-to-test"},
-					},
-				},
-				Event: info.NewEvent(),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"ok-to-test"}}),
+				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypeRetest,
 			},
-			replyAllowed: true,
-			want:         ResultAllowed,
+			want: false,
 		},
 		{
 			name: "disallowing member not in team for retest",
 			fields: fields{
-				Settings: &v1alpha1.Settings{
-					Policy: &v1alpha1.Policy{
-						OkToTest: []string{"ok-to-test"},
-					},
-				},
-				Event: info.NewEvent(),
+				repository: newRepoWithPolicy(&v1alpha1.Policy{PullRequest: []string{"ok-to-test"}}),
+				event:      info.NewEvent(),
 			},
 			args: args{
 				tType: info.TriggerTypeRetest,
 			},
-			replyAllowed: false,
-			want:         ResultDisallowed,
-			wantErr:      true,
+			want:    false,
+			wantErr: true,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			observer, _ := 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}
-			p := &Policy{
-				Settings: tt.fields.Settings,
-				Event:    tt.fields.Event,
-				VCX:      vcx,
-				Logger:   logger,
+			if tt.fields.event == nil {
+				tt.fields.event = info.NewEvent()
 			}
-			ctx, _ := rtesting.SetupFakeContext(t)
-			got, err := p.IsAllowed(ctx, tt.args.tType)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("Policy.IsAllowed() error = %v, wantErr %v", err, tt.wantErr)
-				return
+			p := &Policy{
+				Repository:   tt.fields.repository,
+				Event:        tt.fields.event,
+				VCX:          vcx,
+				Logger:       logger,
+				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)
 			}
+			assert.Equal(t, tt.wantReason, reason)
 		})
 	}
 }

pkg/provider/bitbucketcloud/bitbucket.go

@@ -9,6 +9,7 @@ import (
 	"github.com/ktrysmt/go-bitbucket"
 	"github.com/mitchellh/mapstructure"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
@@ -165,7 +166,7 @@ func (v *Provider) GetFileInsideRepo(_ context.Context, event *info.Event, path,
 	return v.getBlob(event, revision, path)
 }
 
-func (v *Provider) SetClient(_ context.Context, _ *params.Run, event *info.Event, _ *v1alpha1.Settings) error {
+func (v *Provider) SetClient(_ context.Context, _ *params.Run, event *info.Event, _ *v1alpha1.Repository, _ *events.EventEmitter) error {
 	if event.Provider.Token == "" {
 		return fmt.Errorf("no git_provider.secret has been set in the repo crd")
 	}

pkg/provider/bitbucketcloud/bitbucket_test.go

@@ -130,7 +130,7 @@ func TestSetClient(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			ctx, _ := rtesting.SetupFakeContext(t)
 			v := Provider{}
-			err := v.SetClient(ctx, nil, tt.event, nil)
+			err := v.SetClient(ctx, nil, tt.event, nil, nil)
 			if tt.wantErrSubstr != "" {
 				assert.ErrorContains(t, err, tt.wantErrSubstr)
 				return