diff --git a/pkg/pipelineascode/match.go b/pkg/pipelineascode/match.go index dc50437df..9107c5077 100644 --- a/pkg/pipelineascode/match.go +++ b/pkg/pipelineascode/match.go @@ -25,10 +25,10 @@ import ( ) func (p *PacRun) matchRepoPR(ctx context.Context) ([]matcher.Match, *v1alpha1.Repository, error) { - p.debugf("matchRepoPR: starting repo verification for url=%s", p.event.URL) - repo, err := p.verifyRepoAndUser(ctx) + p.debugf("matchRepoPR: starting repo setup for url=%s", p.event.URL) + repo, err := p.setupRepo(ctx) if err != nil { - return nil, nil, err + return nil, repo, err } if repo == nil { p.debugf("matchRepoPR: no repository match for url=%s", p.event.URL) @@ -40,8 +40,24 @@ func (p *PacRun) matchRepoPR(ctx context.Context) ([]matcher.Match, *v1alpha1.Re return nil, repo, p.cancelPipelineRunsOpsComment(ctx, repo) } + rawTemplates, err := p.fetchTektonTemplates(ctx, repo) + if err != nil { + return nil, repo, err + } + if rawTemplates == "" { + return nil, repo, nil + } + allowed, err := p.checkUserAccess(ctx, repo) + if err != nil { + return nil, repo, err + } + if !allowed { + p.debugf("matchRepoPR: ACL check denied access") + return nil, repo, nil + } + p.debugf("matchRepoPR: fetching pipelineruns from repo=%s/%s", repo.GetNamespace(), repo.GetName()) - matchedPRs, err := p.getPipelineRunsFromRepo(ctx, repo) + matchedPRs, err := p.getPipelineRunsFromRepo(ctx, repo, rawTemplates) if err != nil { return nil, repo, err } @@ -50,11 +66,11 @@ func (p *PacRun) matchRepoPR(ctx context.Context) ([]matcher.Match, *v1alpha1.Re return matchedPRs, repo, nil } -// verifyRepoAndUser verifies if the Repo CR exists for the Git Repository, -// if the user has permission to run CI and also initialise provider client. -func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, error) { +// setupRepo matches the Repository CR, sets up the authenticated provider client, +// and fetches commit info. It does NOT perform ACL checks. +func (p *PacRun) setupRepo(ctx context.Context) (*v1alpha1.Repository, error) { // Match the Event URL to a Repository URL, - p.debugf("verifyRepoAndUser: matching repository for url=%s", p.event.URL) + p.debugf("setupRepo: matching repository for url=%s", p.event.URL) repo, err := matcher.MatchEventURLRepo(ctx, p.run, p.event, "") if err != nil { return nil, fmt.Errorf("error matching Repository for event: %w", err) @@ -65,13 +81,13 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e p.eventEmitter.EmitMessage(nil, zap.WarnLevel, "RepositoryNamespaceMatch", msg) return nil, nil } - p.debugf("verifyRepoAndUser: matched repo=%s/%s", repo.GetNamespace(), repo.GetName()) + p.debugf("setupRepo: matched repo=%s/%s", repo.GetNamespace(), repo.GetName()) p.logger = p.logger.With("namespace", repo.Namespace) p.vcx.SetLogger(p.logger) p.eventEmitter.SetLogger(p.logger) - // Set up authenticated client with proper token scoping + // Set up authenticated client with proper token scoping. // NOTE: This is typically already done in sinker.processEvent() for all event types, // but we call it here as a safety net for edge cases (e.g., tests calling Run() directly, // or if the early setup in sinker failed/was skipped). The call is idempotent. @@ -80,21 +96,25 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e if err != nil { return repo, err } - p.debugf("verifyRepoAndUser: authenticated client setup complete") + p.debugf("setupRepo: authenticated client setup complete") // Get the SHA commit info, we want to get the URL and commit title if p.event.SHA == "" || p.event.SHATitle == "" || p.event.SHAURL == "" { - p.debugf("verifyRepoAndUser: fetching commit info") + p.debugf("setupRepo: fetching commit info") if err = p.vcx.GetCommitInfo(ctx, p.event); err != nil { return repo, fmt.Errorf("could not find commit info: %w", err) } - p.debugf("verifyRepoAndUser: commit info loaded sha=%s title=%s", p.event.SHA, p.event.SHATitle) + p.debugf("setupRepo: commit info loaded sha=%s title=%s", p.event.SHA, p.event.SHATitle) } + return repo, nil +} + +func (p *PacRun) checkUserAccess(ctx context.Context, repo *v1alpha1.Repository) (bool, error) { // Verify whether the sender of the GitOps command (e.g., /test) has the appropriate permissions to // trigger CI on the repository, as any user is able to comment on a pushed commit in open-source repositories. if p.event.TriggerTarget == triggertype.Push && opscomments.IsAnyOpsEventType(p.event.EventType) { - p.debugf("verifyRepoAndUser: checking access for gitops comment on push") + p.debugf("checkUserAccess: checking access for gitops comment on push") status := providerstatus.StatusOpts{ Status: CompletedStatus, Title: "Permission denied", @@ -103,7 +123,7 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e AccessDenied: true, } if allowed, err := p.checkAccessOrError(ctx, repo, status, "by GitOps comment on push commit"); !allowed { - return nil, err + return false, err } } @@ -111,7 +131,7 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e // on push we don't need to check the policy since the user has pushed to the repo so it has access to it. // on comment we skip it for now, we are going to check later on if p.event.TriggerTarget != triggertype.Push && p.event.EventType != opscomments.NoOpsCommentEventType.String() { - p.debugf("verifyRepoAndUser: checking access for trigger target=%s event_type=%s", p.event.TriggerTarget, p.event.EventType) + p.debugf("checkUserAccess: checking access for trigger target=%s event_type=%s", p.event.TriggerTarget, p.event.EventType) status := providerstatus.StatusOpts{ Status: queuedStatus, Title: "Pending approval, waiting for an /ok-to-test", @@ -120,7 +140,7 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e AccessDenied: true, } if allowed, err := p.checkAccessOrError(ctx, repo, status, "via "+p.event.TriggerTarget.String()); !allowed { - return nil, err + return false, err } // When /ok-to-test is approved, update the parent "Pipelines as Code CI" status to success // to indicate the approval was successful before pipelines start running. @@ -136,16 +156,15 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e } } } - return repo, nil + return true, nil } -// getPipelineRunsFromRepo fetches pipelineruns from git repository and prepare them for creation. -func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Repository) ([]matcher.Match, error) { +func (p *PacRun) fetchTektonTemplates(ctx context.Context, repo *v1alpha1.Repository) (string, error) { provenance := "source" if repo.Spec.Settings != nil && repo.Spec.Settings.PipelineRunProvenance != "" { provenance = repo.Spec.Settings.PipelineRunProvenance } - p.debugf("getPipelineRunsFromRepo: repo=%s/%s provenance=%s", repo.GetNamespace(), repo.GetName(), provenance) + p.debugf("fetchTektonTemplates: repo=%s/%s provenance=%s", repo.GetNamespace(), repo.GetName(), provenance) rawTemplates, err := p.vcx.GetTektonDir(ctx, p.event, tektonDir, provenance) if err != nil && p.event.TriggerTarget == triggertype.PullRequest && strings.Contains(err.Error(), "error unmarshalling yaml file") { // make the error a bit more friendly for users who don't know what marshalling or intricacies of the yaml parser works @@ -163,43 +182,62 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep }, }, ) - return nil, nil + return "", nil } - return nil, err + return "", err } if err != nil { - p.debugf("getPipelineRunsFromRepo: GetTektonDir returned error: %v", err) - } else { - p.debugf("getPipelineRunsFromRepo: fetched templates length=%d", len(rawTemplates)) - } - - if rawTemplates == "" && p.event.EventType == opscomments.OkToTestCommentEventType.String() { - err = p.createNeutralStatus(ctx, ".tekton directory not found", tektonDirMissingError) - if err != nil { - p.eventEmitter.EmitMessage(nil, zap.ErrorLevel, "RepositoryCreateStatus", err.Error()) + p.debugf("fetchTektonTemplates: GetTektonDir returned error: %v", err) + msg := "" + if strings.Contains(err.Error(), "error unmarshalling yaml file") { + msg = "PipelineRun YAML validation" } + msg += fmt.Sprintf(" err: %s", err.Error()) + p.eventEmitter.EmitMessage(nil, zap.ErrorLevel, "RepositoryInvalidPipelineRunTemplate", msg) + return "", nil } - // This is for push event error logging because we can't create comment for yaml validation errors on push - if err != nil || rawTemplates == "" { - msg := "" - reason := "RepositoryPipelineRunNotFound" - logLevel := zap.InfoLevel - if err != nil { - reason = "RepositoryInvalidPipelineRunTemplate" - logLevel = zap.ErrorLevel - if strings.Contains(err.Error(), "error unmarshalling yaml file") { - msg = "PipelineRun YAML validation" + p.debugf("fetchTektonTemplates: fetched templates length=%d", len(rawTemplates)) + + if rawTemplates == "" { + if p.event.EventType == opscomments.OkToTestCommentEventType.String() { + if statusErr := p.createNeutralStatus(ctx, ".tekton directory not found", tektonDirMissingError); statusErr != nil { + p.eventEmitter.EmitMessage(nil, zap.ErrorLevel, "RepositoryCreateStatus", statusErr.Error()) } - msg += fmt.Sprintf(" err: %s", err.Error()) - } else { - msg = fmt.Sprintf("cannot locate templates in %s/ directory for this repository in %s", tektonDir, p.event.HeadBranch) } - p.eventEmitter.EmitMessage(nil, logLevel, reason, msg) + msg := fmt.Sprintf("cannot locate templates in %s/ directory for this repository in %s", tektonDir, p.event.HeadBranch) + p.eventEmitter.EmitMessage(nil, zap.InfoLevel, "RepositoryPipelineRunNotFound", msg) + return "", nil + } + + return rawTemplates, nil +} + +// verifyRepoAndUser verifies if the Repo CR exists for the Git Repository, +// if the user has permission to run CI and also initialise provider client. +func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, error) { + repo, err := p.setupRepo(ctx) + if err != nil { + return repo, err + } + if repo == nil { return nil, nil } + allowed, err := p.checkUserAccess(ctx, repo) + if err != nil { + return nil, err + } + if !allowed { + return nil, nil + } + return repo, nil +} + +// getPipelineRunsFromRepo processes pre-fetched .tekton templates and prepares PipelineRuns for creation. +func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Repository, rawTemplates string) ([]matcher.Match, error) { + p.debugf("getPipelineRunsFromRepo: repo=%s/%s templates_length=%d", repo.GetNamespace(), repo.GetName(), len(rawTemplates)) // check for condition if need update the pipelinerun with regexp from the // "raw" pipelinerun string diff --git a/pkg/pipelineascode/match_test.go b/pkg/pipelineascode/match_test.go index 18f4333a8..044f563ab 100644 --- a/pkg/pipelineascode/match_test.go +++ b/pkg/pipelineascode/match_test.go @@ -268,7 +268,8 @@ spec: nil, ) - matchedPRs, err := p.getPipelineRunsFromRepo(ctx, repositories[0]) + rawTemplates := fmt.Sprintf(template, tt.targetNamespaceLine) + matchedPRs, err := p.getPipelineRunsFromRepo(ctx, repositories[0], rawTemplates) assert.NilError(t, err) if tt.wantNoMatch { assert.Equal(t, len(matchedPRs), 0) @@ -431,26 +432,10 @@ func TestGetPipelineRunsFromRepo(t *testing.T) { }, Spec: v1alpha1.RepositorySpec{}, }, - // if `testdata/no_yaml` dir is supplied here p.getPipelineRunsFromRepo func will return after - // GetTektonDir so providing `testdat/push_branch` so that it should call MatchPipelineRunsByAnnotation - // first and then create a neutral check-run. tektondir: "testdata/push_branch", expectedNumberOfPruns: 0, event: okToTestEvent, }, - { - name: "no .tekton dir in repository", - repositories: &v1alpha1.Repository{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testrepo", - Namespace: "test", - }, - Spec: v1alpha1.RepositorySpec{}, - }, - tektondir: "testdata/no_tekton_dir", - expectedNumberOfPruns: 0, - event: okToTestEvent, - }, { name: "same name pipelineruns error on regular event", repositories: &v1alpha1.Repository{ @@ -572,7 +557,9 @@ func TestGetPipelineRunsFromRepo(t *testing.T) { vcx.SetPacInfo(pacInfo) p := NewPacs(tt.event, vcx, cs, pacInfo, k8int, logger, nil) p.eventEmitter = events.NewEventEmitter(stdata.Kube, logger) - matchedPRs, err := p.getPipelineRunsFromRepo(ctx, tt.repositories) + rawTemplates, err := vcx.GetTektonDir(ctx, tt.event, ".tekton", "source") + assert.NilError(t, err) + matchedPRs, err := p.getPipelineRunsFromRepo(ctx, tt.repositories, rawTemplates) if tt.wantErr { assert.Assert(t, err != nil, "expected an error but got nil") return @@ -827,3 +814,203 @@ func TestVerifyRepoAndUser(t *testing.T) { }) } } + +func TestMatchRepoPRSkipsACLWithoutTektonDir(t *testing.T) { + validTemplate := `apiVersion: tekton.dev/v1 +kind: PipelineRun +metadata: + name: test-pr + annotations: + pipelinesascode.tekton.dev/on-target-branch: "[main]" + pipelinesascode.tekton.dev/on-event: "[pull_request]" +spec: + pipelineSpec: + tasks: + - name: task + taskSpec: + steps: + - name: step + image: alpine + script: echo hello +` + + tests := []struct { + name string + event *info.Event + tektonDirTemplate string + allowIT bool + wantMatches int + wantLogSnippet string + dontWantLog string + }{ + { + name: "PR from external user without tekton dir skips ACL and status", + event: &info.Event{ + SHA: "abc123", + Organization: "org", + Repository: "repo", + URL: "https://example.com/org/repo", + HeadBranch: "feature", + BaseBranch: "main", + Sender: "external-user", + EventType: triggertype.PullRequest.String(), + TriggerTarget: triggertype.PullRequest, + InstallationID: 1, + Provider: &info.Provider{Token: "test"}, + }, + tektonDirTemplate: "", + allowIT: false, + wantMatches: 0, + wantLogSnippet: "fetched templates length=0", + dontWantLog: "checkUserAccess:", + }, + { + name: "push gitops comment without tekton dir skips ACL and status", + event: &info.Event{ + SHA: "abc123", + Organization: "org", + Repository: "repo", + URL: "https://example.com/org/repo", + HeadBranch: "main", + BaseBranch: "main", + Sender: "external-user", + EventType: opscomments.TestAllCommentEventType.String(), + TriggerTarget: triggertype.Push, + InstallationID: 1, + Provider: &info.Provider{Token: "test"}, + }, + tektonDirTemplate: "", + allowIT: false, + wantMatches: 0, + wantLogSnippet: "fetched templates length=0", + dontWantLog: "checkUserAccess:", + }, + { + name: "ok-to-test without tekton dir creates neutral status but skips ACL", + event: &info.Event{ + SHA: "abc123", + Organization: "org", + Repository: "repo", + URL: "https://example.com/org/repo", + HeadBranch: "feature", + BaseBranch: "main", + Sender: "maintainer", + EventType: opscomments.OkToTestCommentEventType.String(), + TriggerTarget: triggertype.PullRequest, + InstallationID: 1, + Provider: &info.Provider{Token: "test"}, + }, + tektonDirTemplate: "", + allowIT: true, + wantMatches: 0, + wantLogSnippet: "fetched templates length=0", + dontWantLog: "checkUserAccess:", + }, + { + name: "PR from external user with tekton dir runs ACL and creates status", + event: &info.Event{ + SHA: "abc123", + Organization: "org", + Repository: "repo", + URL: "https://example.com/org/repo", + HeadBranch: "main", + BaseBranch: "main", + Sender: "external-user", + EventType: triggertype.PullRequest.String(), + TriggerTarget: triggertype.PullRequest, + InstallationID: 1, + Provider: &info.Provider{Token: "test"}, + }, + tektonDirTemplate: validTemplate, + allowIT: false, + wantMatches: 0, + wantLogSnippet: "is not allowed to trigger CI", + }, + { + name: "PR from allowed user with tekton dir succeeds", + event: &info.Event{ + SHA: "abc123", + Organization: "org", + Repository: "repo", + URL: "https://example.com/org/repo", + HeadBranch: "main", + BaseBranch: "main", + Sender: "collaborator", + EventType: triggertype.PullRequest.String(), + TriggerTarget: triggertype.PullRequest, + InstallationID: 1, + Provider: &info.Provider{Token: "test"}, + }, + tektonDirTemplate: validTemplate, + allowIT: true, + wantMatches: 1, + wantLogSnippet: "checkUserAccess:", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + observerCore, logCatcher := zapobserver.New(zap.DebugLevel) + logger := zap.New(observerCore).Sugar() + + baseCtx, _ := rtesting.SetupFakeContext(t) + ctx := info.StoreNS(baseCtx, "pac") + + repositories := []*v1alpha1.Repository{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "repo", + Namespace: "ns", + }, + Spec: v1alpha1.RepositorySpec{ + URL: "https://example.com/org/repo", + }, + }} + + stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{ + Repositories: repositories, + }) + + in := info.NewInfo() + cs := ¶ms.Run{ + Info: in, + Clients: clients.Clients{ + PipelineAsCode: stdata.PipelineAsCode, + Kube: stdata.Kube, + Tekton: stdata.Pipeline, + Log: logger, + }, + } + cs.Clients.SetConsoleUI(consoleui.FallBackConsole{}) + + vcx := &testprovider.TestProviderImp{ + AllowIT: tt.allowIT, + TektonDirTemplate: tt.tektonDirTemplate, + } + + pacInfo := &info.PacOpts{ + Settings: settings.Settings{ + ApplicationName: "Pipelines as Code CI", + SecretAutoCreation: true, + RemoteTasks: true, + }, + } + + k8int := &kitesthelper.KinterfaceTest{ + GetSecretResult: map[string]string{"webhook.secret": "secret"}, + } + + p := NewPacs(tt.event, vcx, cs, pacInfo, k8int, logger, nil) + p.eventEmitter = events.NewEventEmitter(stdata.Kube, logger) + + matchedPRs, _, err := p.matchRepoPR(ctx) + assert.NilError(t, err) + assert.Equal(t, len(matchedPRs), tt.wantMatches) + assert.Assert(t, logCatcher.FilterMessageSnippet(tt.wantLogSnippet).Len() > 0, + "expected log containing %q, got: %v", tt.wantLogSnippet, logCatcher.All()) + if tt.dontWantLog != "" { + assert.Equal(t, logCatcher.FilterMessageSnippet(tt.dontWantLog).Len(), 0, + "unexpected log containing %q", tt.dontWantLog) + } + }) + } +} diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go index 0e5c8f441..4fc24d916 100644 --- a/pkg/pipelineascode/pipelineascode_test.go +++ b/pkg/pipelineascode/pipelineascode_test.go @@ -580,6 +580,10 @@ func TestRun(t *testing.T) { testSetupCommonGhReplies(t, mux, tt.runevent, tt.finalStatus, tt.finalStatusText, tt.skipReplyingOrgPublicMembers) if tt.tektondir != "" { ghtesthelper.SetupGitTree(t, mux, tt.tektondir, &tt.runevent, false) + } else { + replyString(mux, + fmt.Sprintf("/repos/%s/%s/git/trees/%s", tt.runevent.Organization, tt.runevent.Repository, tt.runevent.SHA), + `{"sha": "`+tt.runevent.SHA+`", "tree": []}`) } mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/issues/%d/comments", tt.runevent.Organization, tt.runevent.Repository, tt.runevent.PullRequestNumber),