diff --git a/pkg/adapter/sinker.go b/pkg/adapter/sinker.go index 35a6f2a93d..86ce1b2613 100644 --- a/pkg/adapter/sinker.go +++ b/pkg/adapter/sinker.go @@ -7,6 +7,7 @@ import ( "net/http" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" + "github.com/openshift-pipelines/pipelines-as-code/pkg/gitclient" "github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction" "github.com/openshift-pipelines/pipelines-as-code/pkg/matcher" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" @@ -149,7 +150,7 @@ func (s *sinker) findMatchingRepository(ctx context.Context) (*v1alpha1.Reposito // Centralizing this here ensures consistent behavior across all events and enables early // optimizations like skip-CI detection before expensive processing. func (s *sinker) setupClient(ctx context.Context, repo *v1alpha1.Repository) error { - return pipelineascode.SetupAuthenticatedClient( + return gitclient.SetupAuthenticatedClient( ctx, s.vcx, s.kint, diff --git a/pkg/adapter/sinker_test.go b/pkg/adapter/sinker_test.go index 0f6ef25058..aa255e39ce 100644 --- a/pkg/adapter/sinker_test.go +++ b/pkg/adapter/sinker_test.go @@ -105,18 +105,6 @@ func TestSetupClientGitHubAppVsOther(t *testing.T) { wantErr: false, wantRepositoryIDsCount: 0, // No extra repos }, - { - name: "GitHub App with extra repos - IDs should be populated", - installationID: 12345, - hasGitProvider: false, - extraReposConfigured: true, - extraRepoInstallIDs: map[string]int64{ - "another/one": 789, - "andanother/two": 10112, - }, - wantErr: false, - wantRepositoryIDsCount: 2, // Should have 2 extra repo IDs - }, { name: "Non-GitHub App requires git_provider", installationID: 0, @@ -162,37 +150,11 @@ func TestSetupClientGitHubAppVsOther(t *testing.T) { } } - // Setup extra repos if configured - extraRepos := []*v1alpha1.Repository{} - if tt.extraReposConfigured { - repo.Spec.Settings = &v1alpha1.Settings{ - GithubAppTokenScopeRepos: []string{}, - } - for repoName := range tt.extraRepoInstallIDs { - repo.Spec.Settings.GithubAppTokenScopeRepos = append( - repo.Spec.Settings.GithubAppTokenScopeRepos, - repoName, - ) - // Create matching repository CRs for extra repos - extraRepo := testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{ - Name: repoName, - URL: "https://github.com/" + repoName, - InstallNamespace: "default", - }) - extraRepos = append(extraRepos, extraRepo) - } - } - - // Create test data with all repositories - allRepos := append([]*v1alpha1.Repository{repo}, extraRepos...) - run := setupTestData(t, allRepos) + run := setupTestData(t, []*v1alpha1.Repository{repo}) // Create a tracking provider to verify behavior trackingProvider := &trackingProviderImpl{ - TestProviderImp: testprovider.TestProviderImp{AllowIT: true}, - createTokenCalled: false, - repositoryIDs: []int64{}, - extraRepoInstallIDs: tt.extraRepoInstallIDs, + TestProviderImp: testprovider.TestProviderImp{AllowIT: true}, } trackingProvider.SetLogger(log) @@ -229,32 +191,6 @@ func TestSetupClientGitHubAppVsOther(t *testing.T) { } else { assert.NilError(t, err, "unexpected error: %v", err) } - - // For GitHub Apps with extra repos, verify CreateToken was called - // and repository IDs were populated - if tt.extraReposConfigured && !tt.wantErr { - assert.Assert(t, trackingProvider.createTokenCalled, - "CreateToken should have been called for extra repos") - - // Verify all expected repo IDs are present - for repoName, expectedID := range tt.extraRepoInstallIDs { - found := false - for _, id := range trackingProvider.repositoryIDs { - if id == expectedID { - found = true - break - } - } - assert.Assert(t, found, - "Repository ID %d for %s not found in provider.RepositoryIDs: %v", - expectedID, repoName, trackingProvider.repositoryIDs) - } - - assert.Equal(t, len(trackingProvider.repositoryIDs), tt.wantRepositoryIDsCount, - "Expected %d repository IDs, got %d: %v", - tt.wantRepositoryIDsCount, len(trackingProvider.repositoryIDs), - trackingProvider.repositoryIDs) - } }) } } @@ -262,19 +198,9 @@ func TestSetupClientGitHubAppVsOther(t *testing.T) { // trackingProviderImpl wraps TestProviderImp to track CreateToken calls and repository IDs. type trackingProviderImpl struct { testprovider.TestProviderImp - createTokenCalled bool - repositoryIDs []int64 - extraRepoInstallIDs map[string]int64 } -func (t *trackingProviderImpl) CreateToken(_ context.Context, repositories []string, _ *info.Event) (string, error) { - t.createTokenCalled = true - // Simulate adding repository IDs like the real CreateToken does - for _, repo := range repositories { - if id, ok := t.extraRepoInstallIDs[repo]; ok { - t.repositoryIDs = append(t.repositoryIDs, id) - } - } +func (t *trackingProviderImpl) CreateToken(_ context.Context, _ []string, _ *info.Event) (string, error) { return "fake-token", nil } diff --git a/pkg/cli/webhook/secret.go b/pkg/cli/webhook/secret.go index 4b8b2a7fe0..f231985062 100644 --- a/pkg/cli/webhook/secret.go +++ b/pkg/cli/webhook/secret.go @@ -5,7 +5,7 @@ import ( "fmt" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" - "github.com/openshift-pipelines/pipelines-as-code/pkg/pipelineascode" + "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -16,8 +16,8 @@ func (w *Options) createWebhookSecret(ctx context.Context, response *response) e Name: w.RepositoryName, }, Data: map[string][]byte{ - pipelineascode.DefaultGitProviderSecretKey: []byte(response.PersonalAccessToken), - pipelineascode.DefaultGitProviderWebhookSecretKey: []byte(response.WebhookSecret), + secrets.DefaultGitProviderSecretKey: []byte(response.PersonalAccessToken), + secrets.DefaultGitProviderWebhookSecretKey: []byte(response.WebhookSecret), }, }, metav1.CreateOptions{}) if err != nil { @@ -33,7 +33,7 @@ func (w *Options) updateWebhookSecret(ctx context.Context, response *response) e if err != nil { return err } - secretInfo.Data[pipelineascode.DefaultGitProviderWebhookSecretKey] = []byte(response.WebhookSecret) + secretInfo.Data[secrets.DefaultGitProviderWebhookSecretKey] = []byte(response.WebhookSecret) _, err = w.Run.Clients.Kube.CoreV1().Secrets(w.RepositoryNamespace).Update(ctx, secretInfo, metav1.UpdateOptions{}) if err != nil { @@ -57,11 +57,11 @@ func (w *Options) updateRepositoryCR(ctx context.Context, res *response) error { repo.Spec.GitProvider.Secret = &v1alpha1.Secret{ Name: w.RepositoryName, - Key: pipelineascode.DefaultGitProviderSecretKey, + Key: secrets.DefaultGitProviderSecretKey, } repo.Spec.GitProvider.WebhookSecret = &v1alpha1.Secret{ Name: w.RepositoryName, - Key: pipelineascode.DefaultGitProviderWebhookSecretKey, + Key: secrets.DefaultGitProviderWebhookSecretKey, } if res.UserName != "" { diff --git a/pkg/cli/webhook/secret_test.go b/pkg/cli/webhook/secret_test.go index b6b9f33f3e..c2c4cf93e6 100644 --- a/pkg/cli/webhook/secret_test.go +++ b/pkg/cli/webhook/secret_test.go @@ -11,7 +11,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" - "github.com/openshift-pipelines/pipelines-as-code/pkg/pipelineascode" + "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" "github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger" "gotest.tools/v3/assert" @@ -35,7 +35,7 @@ func TestWebHookSecret(t *testing.T) { Namespace: repoNS, }, Data: map[string][]byte{ - pipelineascode.DefaultGitProviderSecretKey: []byte("somethingsomething"), + secrets.DefaultGitProviderSecretKey: []byte("somethingsomething"), }, }, }, diff --git a/pkg/cmd/tknpac/webhook/add.go b/pkg/cmd/tknpac/webhook/add.go index 39ff184cd6..a2c037a1e9 100644 --- a/pkg/cmd/tknpac/webhook/add.go +++ b/pkg/cmd/tknpac/webhook/add.go @@ -11,7 +11,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/completion" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" - "github.com/openshift-pipelines/pipelines-as-code/pkg/pipelineascode" + "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -117,7 +117,7 @@ func add(ctx context.Context, opts *cli.PacCliOpts, run *params.Run, ioStreams * gitProviderSecretKey := repo.Spec.GitProvider.Secret.Key if gitProviderSecretKey == "" { - gitProviderSecretKey = pipelineascode.DefaultGitProviderSecretKey + gitProviderSecretKey = secrets.DefaultGitProviderSecretKey } tokenData, ok := secretData.Data[gitProviderSecretKey] diff --git a/pkg/cmd/tknpac/webhook/update-token.go b/pkg/cmd/tknpac/webhook/update-token.go index c180708595..5197b54f04 100644 --- a/pkg/cmd/tknpac/webhook/update-token.go +++ b/pkg/cmd/tknpac/webhook/update-token.go @@ -10,7 +10,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/cli/prompt" "github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/completion" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" - "github.com/openshift-pipelines/pipelines-as-code/pkg/pipelineascode" + "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -110,7 +110,7 @@ func update(ctx context.Context, opts *cli.PacCliOpts, run *params.Run, ioStream gitProviderSecretKey := repo.Spec.GitProvider.Secret.Key if gitProviderSecretKey == "" { - gitProviderSecretKey = pipelineascode.DefaultGitProviderSecretKey + gitProviderSecretKey = secrets.DefaultGitProviderSecretKey } secretData.Data[gitProviderSecretKey] = []byte(personalAccessToken) diff --git a/pkg/pipelineascode/client_setup.go b/pkg/gitclient/client_setup.go similarity index 84% rename from pkg/pipelineascode/client_setup.go rename to pkg/gitclient/client_setup.go index dc57f21558..9f3bc47175 100644 --- a/pkg/pipelineascode/client_setup.go +++ b/pkg/gitclient/client_setup.go @@ -1,4 +1,4 @@ -package pipelineascode +package gitclient import ( "context" @@ -11,10 +11,11 @@ import ( "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" - "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github" + "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" semconv "go.opentelemetry.io/otel/semconv/v1.40.0" "go.opentelemetry.io/otel/trace" "go.uber.org/zap" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // SetupAuthenticatedClient sets up the authenticated VCS client with proper token scoping. @@ -32,6 +33,19 @@ func SetupAuthenticatedClient( pacInfo *info.PacOpts, logger *zap.SugaredLogger, ) error { + if globalRepo == nil && + run != nil && + run.Info.Controller != nil && + run.Info.Kube != nil && + run.Info.Kube.Namespace != "" && + run.Info.Controller.GlobalRepository != "" { + var err error + if globalRepo, err = run.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(run.Info.Kube.Namespace).Get( + ctx, run.Info.Controller.GlobalRepository, metav1.GetOptions{}, + ); err != nil { + logger.Errorf("cannot get global repository: %v", err) + } + } // Determine secret namespace BEFORE merging repos // This preserves the ability to detect when credentials come from global repo secretNS := repo.GetNamespace() @@ -49,10 +63,10 @@ func SetupAuthenticatedClient( // GitHub Apps use controller secret, not Repository git_provider if event.InstallationID > 0 { logger.Debugf("setupAuthenticatedClient: github app installation id=%d, using controller webhook secret", event.InstallationID) - event.Provider.WebhookSecret, _ = GetCurrentNSWebhookSecret(ctx, kint, run) + event.Provider.WebhookSecret, _ = secrets.GetCurrentNSWebhookSecret(ctx, kint, run) } else { // Non-GitHub App providers use git_provider section in Repository spec - scm := SecretFromRepository{ + scm := secrets.SecretFromRepository{ K8int: kint, Config: vcx.GetConfig(), Event: event, @@ -96,18 +110,5 @@ is that what you want? make sure you use -n when generating the secret, eg: echo } logger.Debugf("setupAuthenticatedClient: provider client initialized") - // Handle GitHub App token scoping for both global and repo-level configuration - if event.InstallationID > 0 { - logger.Debugf("setupAuthenticatedClient: scoping github app token") - token, err := github.ScopeTokenToListOfRepos(ctx, vcx, pacInfo, repo, run, event, eventEmitter, logger) - if err != nil { - return fmt.Errorf("failed to scope token: %w", err) - } - // If Global and Repo level configurations are not provided then lets not override the provider token. - if token != "" { - event.Provider.Token = token - } - } - return nil } diff --git a/pkg/pipelineascode/client_setup_test.go b/pkg/gitclient/client_setup_test.go similarity index 81% rename from pkg/pipelineascode/client_setup_test.go rename to pkg/gitclient/client_setup_test.go index 867edcb699..f09faa91aa 100644 --- a/pkg/pipelineascode/client_setup_test.go +++ b/pkg/gitclient/client_setup_test.go @@ -1,4 +1,4 @@ -package pipelineascode +package gitclient import ( "context" @@ -113,7 +113,11 @@ func seedTestData(ctx context.Context, t *testing.T, repos []*v1alpha1.Repositor }, Info: info.Info{ Controller: &info.ControllerInfo{ - Secret: "pipelines-as-code-secret", + Secret: "pipelines-as-code-secret", + GlobalRepository: "global-repo", + }, + Kube: &info.KubeOpts{ + Namespace: "default", }, }, } @@ -339,6 +343,155 @@ func TestSetupAuthenticatedClientRepositoryConfig(t *testing.T) { } } +// TestSetupAuthenticatedClientGlobalRepoAutoFetch tests the auto-fetch behavior +// when globalRepo is nil. +func TestSetupAuthenticatedClientGlobalRepoAutoFetch(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + globalRepoExist bool + passGlobalRepo bool + wantErr bool + description string + }{ + { + name: "globalRepo nil and exists in API is fetched and merged", + globalRepoExist: true, + passGlobalRepo: false, + wantErr: false, + description: "When globalRepo is nil and exists in the API, it should be fetched automatically", + }, + { + name: "globalRepo nil and does not exist in API continues without error", + globalRepoExist: false, + passGlobalRepo: false, + wantErr: false, + description: "When globalRepo is nil and does not exist in the API, function should continue without error", + }, + { + name: "globalRepo provided skips API fetch", + globalRepoExist: false, + passGlobalRepo: true, + wantErr: false, + description: "When globalRepo is provided, no API fetch should happen", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, log := setupTestContext(t) + + repo := createTestRepository(true) + repos := []*v1alpha1.Repository{repo} + + // If global repo should exist in the API, seed it + var globalRepo *v1alpha1.Repository + if tt.globalRepoExist { + globalRepo = testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{ + Name: "global-repo", + URL: "https://github.com/global/repo", + InstallNamespace: "default", + }) + globalRepo.Spec.GitProvider = &v1alpha1.GitProvider{ + URL: "https://github.com", + Secret: &v1alpha1.Secret{ + Name: "global-secret", + Key: "token", + }, + } + repos = append(repos, globalRepo) + } + + run := seedTestData(ctx, t, repos) + testProvider := createTestProvider(log) + + kint := createTestKintMock(map[string]string{ + "test-secret": "test-token", + "global-secret": "global-token", + }) + + event := createTestEvent("pull_request", 0, "") + pacInfo := createTestPacInfo() + + // Determine what to pass as globalRepo parameter + var globalRepoParam *v1alpha1.Repository + if tt.passGlobalRepo { + globalRepoParam = &v1alpha1.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "provided-global-repo", + Namespace: "default", + }, + } + } + + err := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, globalRepoParam, pacInfo, log) + + if tt.wantErr { + assert.Assert(t, err != nil, "%s: expected error but got nil", tt.description) + } else { + assert.NilError(t, err, "%s: %v", tt.description, err) + } + }) + } +} + +// TestSetupAuthenticatedClientGlobalRepoMergesSettings verifies that when globalRepo +// is auto-fetched, its settings are properly merged into the local repo. +func TestSetupAuthenticatedClientGlobalRepoMergesSettings(t *testing.T) { + t.Parallel() + + ctx, log := setupTestContext(t) + + // Local repo with git_provider URL but no secret (secret comes from global) + repo := testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{ + Name: "test-repo", + URL: "https://github.com/owner/repo", + InstallNamespace: "default", + }) + repo.Spec.GitProvider = &v1alpha1.GitProvider{ + URL: "https://github.com", + } + + // Global repo with git_provider credentials + globalRepo := testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{ + Name: "global-repo", + URL: "https://github.com/global/repo", + InstallNamespace: "default", + }) + globalRepo.Spec.GitProvider = &v1alpha1.GitProvider{ + URL: "https://github.com", + Secret: &v1alpha1.Secret{ + Name: "global-secret", + Key: "token", + }, + WebhookSecret: &v1alpha1.Secret{ + Name: "global-secret", + Key: "webhook.secret", + }, + } + + run := seedTestData(ctx, t, []*v1alpha1.Repository{repo, globalRepo}) + testProvider := createTestProvider(log) + + kint := createTestKintMock(map[string]string{ + "global-secret": "global-token-value", + }) + + event := createTestEvent("pull_request", 0, "") + pacInfo := createTestPacInfo() + + // Pass nil globalRepo so it gets auto-fetched + err := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + + assert.NilError(t, err, "should succeed with auto-fetched global repo providing credentials") + // After merge, the repo should have secret from global repo + assert.Assert(t, repo.Spec.GitProvider.Secret != nil, "secret should be merged from global repo") + assert.Equal(t, "global-token-value", event.Provider.Token, "token should come from global repo secret") +} + // TestSetupAuthenticatedClient_WebhookValidation tests webhook secret validation. func TestSetupAuthenticatedClientWebhookValidation(t *testing.T) { t.Parallel() diff --git a/pkg/pipelineascode/match.go b/pkg/pipelineascode/match.go index 9500346366..dc50437dfc 100644 --- a/pkg/pipelineascode/match.go +++ b/pkg/pipelineascode/match.go @@ -11,6 +11,7 @@ import ( apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" pacerrors "github.com/openshift-pipelines/pipelines-as-code/pkg/errors" + "github.com/openshift-pipelines/pipelines-as-code/pkg/gitclient" "github.com/openshift-pipelines/pipelines-as-code/pkg/matcher" "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" @@ -75,7 +76,7 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e // 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. // SetupAuthenticatedClient will merge global repo settings after determining secret namespace. - err = SetupAuthenticatedClient(ctx, p.vcx, p.k8int, p.run, p.event, repo, p.globalRepo, p.pacInfo, p.logger) + err = gitclient.SetupAuthenticatedClient(ctx, p.vcx, p.k8int, p.run, p.event, repo, p.globalRepo, p.pacInfo, p.logger) if err != nil { return repo, err } diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go index 6397af7790..0e5c8f4417 100644 --- a/pkg/pipelineascode/pipelineascode_test.go +++ b/pkg/pipelineascode/pipelineascode_test.go @@ -610,7 +610,11 @@ func TestRun(t *testing.T) { }, }, Controller: &info.ControllerInfo{ - Secret: info.DefaultPipelinesAscodeSecretName, + Secret: info.DefaultPipelinesAscodeSecretName, + GlobalRepository: "global-repo", + }, + Kube: &info.KubeOpts{ + Namespace: repo.InstallNamespace, }, }, } diff --git a/pkg/provider/github/acl_test.go b/pkg/provider/github/acl_test.go index de70722138..d815415b80 100644 --- a/pkg/provider/github/acl_test.go +++ b/pkg/provider/github/acl_test.go @@ -539,7 +539,7 @@ func TestOkToTestCommentSHA(t *testing.T) { pacInfo: pacopts, } - payload := fmt.Sprintf(`{"action": "created", "repository": {"name": "repo", "owner": {"login": "owner"}}, "sender": {"login": %q}, "issue": {"pull_request": {"html_url": "https://github.com/owner/repo/pull/1"}}, "comment": {"body": %q}}`, + payload := fmt.Sprintf(`{"action": "created", "repository": {"name": "repo", "owner": {"login": "owner"}, "html_url": "http://url.com/owner/repo/1"}, "sender": {"login": %q}, "issue": {"number": 1, "pull_request": {"html_url": "https://github.com/owner/repo/pull/1"}}, "comment": {"body": %q}}`, tt.runevent.Sender, tt.commentBody) _, err := gprovider.ParsePayload(ctx, gprovider.Run, &http.Request{Header: http.Header{"X-Github-Event": []string{"issue_comment"}}}, payload) if (err != nil) != tt.wantErr { diff --git a/pkg/provider/github/github.go b/pkg/provider/github/github.go index 8580e65e11..a261ff4c7c 100644 --- a/pkg/provider/github/github.go +++ b/pkg/provider/github/github.go @@ -343,6 +343,19 @@ func (v *Provider) SetClient(ctx context.Context, run *params.Run, event *info.E } } + // Handle GitHub App token scoping for both global and repo-level configuration + if event.InstallationID > 0 { + v.Logger.Debugf("setupAuthenticatedClient: scoping github app token") + token, err := ScopeTokenToListOfRepos(ctx, v, v.pacInfo, repo, run, event, v.eventEmitter, v.Logger) + if err != nil { + return fmt.Errorf("failed to scope token: %w", err) + } + // If Global and Repo level configurations are not provided then lets not override the provider token. + if token != "" { + event.Provider.Token = token + } + } + return nil } diff --git a/pkg/provider/github/github_test.go b/pkg/provider/github/github_test.go index 241da4b49e..f8414f3bc2 100644 --- a/pkg/provider/github/github_test.go +++ b/pkg/provider/github/github_test.go @@ -1124,8 +1124,16 @@ func TestGithubSetClient(t *testing.T) { Log: testLog, }, } - v := Provider{} - err := v.SetClient(ctx, fakeRun, tt.event, nil, nil) + v := Provider{ + Logger: testLog, + pacInfo: &info.PacOpts{}, + } + repo := &v1alpha1.Repository{ + Spec: v1alpha1.RepositorySpec{ + Settings: &v1alpha1.Settings{}, + }, + } + err := v.SetClient(ctx, fakeRun, tt.event, repo, nil) assert.NilError(t, err) assert.Equal(t, tt.expectedURL, *v.APIURL) assert.Equal(t, "https", v.Client().BaseURL.Scheme) diff --git a/pkg/provider/github/parse_payload.go b/pkg/provider/github/parse_payload.go index 7300e86159..72b556c9d3 100644 --- a/pkg/provider/github/parse_payload.go +++ b/pkg/provider/github/parse_payload.go @@ -1,6 +1,7 @@ package github import ( + "bytes" "context" "encoding/json" "errors" @@ -16,6 +17,8 @@ import ( "github.com/google/go-github/v84/github" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" + "github.com/openshift-pipelines/pipelines-as-code/pkg/gitclient" + "github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction" "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" @@ -98,6 +101,15 @@ func (v *Provider) GetAppToken(ctx context.Context, kube kubernetes.Interface, g return token, err } +func (v *Provider) initGitHubWebhookClient(ctx context.Context, event *info.Event, repo *v1alpha1.Repository) error { + kint, err := kubeinteraction.NewKubernetesInteraction(v.Run) + if err != nil { + return err + } + + return gitclient.SetupAuthenticatedClient(ctx, v, kint, v.Run, event, repo, nil, v.pacInfo, v.Logger) +} + func (v *Provider) parseEventType(request *http.Request, event *info.Event) error { event.EventType = request.Header.Get("X-GitHub-Event") if event.EventType == "" { @@ -156,6 +168,10 @@ func (v *Provider) ParsePayload(ctx context.Context, run *params.Run, request *h // Only apply for GitHub provider since we do fancy token creation at payload parsing v.Run = run event := info.NewEvent() + event.Request = &info.Request{ + Header: request.Header, + Payload: bytes.TrimSpace([]byte(payload)), + } systemNS := info.GetNS(ctx) if err := v.parseEventType(request, event); err != nil { return nil, err @@ -328,6 +344,8 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt var err error processedEvent = info.NewEvent() + // need this for validating the webhook signature + processedEvent.Request = event.Request switch gitEvent := eventInt.(type) { case *github.CheckRunEvent: @@ -349,11 +367,7 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt } return v.handleCheckSuites(ctx, gitEvent) case *github.IssueCommentEvent: - if v.ghClient == nil { - return nil, fmt.Errorf("no github client has been initialized, " + - "exiting... (hint: did you forget setting a secret on your repo?)") - } - processedEvent, err = v.handleIssueCommentEvent(ctx, gitEvent) + processedEvent, err = v.handleIssueCommentEvent(ctx, gitEvent, processedEvent) if err != nil { return nil, err } @@ -593,41 +607,42 @@ const ( errSHANotMatch = "the SHA provided in the `/ok-to-test` comment (`%s`) does not match the pull request's HEAD SHA (`%s`)" ) -func (v *Provider) handleIssueCommentEvent(ctx context.Context, event *github.IssueCommentEvent) (*info.Event, error) { +func (v *Provider) handleIssueCommentEvent(ctx context.Context, event *github.IssueCommentEvent, runevent *info.Event) (*info.Event, error) { action := "recheck" - runevent := info.NewEvent() runevent.Organization = event.GetRepo().GetOwner().GetLogin() runevent.Repository = event.GetRepo().GetName() runevent.Sender = event.GetSender().GetLogin() + runevent.URL = event.GetRepo().GetHTMLURL() // Always set the trigger target as pull_request on issue comment events runevent.TriggerTarget = triggertype.PullRequest if !event.GetIssue().IsPullRequest() { return info.NewEvent(), fmt.Errorf("issue comment is not coming from a pull_request") } v.userType = event.GetSender().GetType() + runevent.PullRequestNumber = event.GetIssue().GetNumber() - // We are getting the full URL so we have to get the last part to get the PR number, - // we don\'t have to care about URL query string/hash and other stuff because - // that comes up from the API. - var err error - runevent.PullRequestNumber, err = convertPullRequestURLtoNumber(event.GetIssue().GetPullRequestLinks().GetHTMLURL()) - if err != nil { - return info.NewEvent(), err + repo, repoErr := MatchEventURLRepo(ctx, v.Run, runevent, "") + if repoErr != nil { + return nil, repoErr + } + if repo == nil { + return nil, fmt.Errorf("no repository found matching URL: %s", runevent.URL) } - v.Logger.Infof("issue_comment: pipelinerun %s on %s/%s#%d has been requested", action, runevent.Organization, runevent.Repository, runevent.PullRequestNumber) - processedEvent, err := v.getPullRequest(ctx, runevent) - if err != nil { - return nil, err + // if v.ghClient is nil, then it means that it's Github webhook and client is not initialized yet + // because we initialize the client above only for github app events. + if v.ghClient == nil { + err := v.initGitHubWebhookClient(ctx, runevent, repo) + if err != nil { + return nil, fmt.Errorf("cannot initialize GitHub webhook client: %w", err) + } } - repo, err := MatchEventURLRepo(ctx, v.Run, processedEvent, "") + v.Logger.Infof("issue_comment: pipelinerun %s on %s/%s#%d has been requested", action, runevent.Organization, runevent.Repository, runevent.PullRequestNumber) + processedEvent, err := v.getPullRequest(ctx, runevent) if err != nil { return nil, err } - if repo == nil { - return nil, fmt.Errorf("no repository found matching URL: %s", processedEvent.URL) - } gitOpsCommentPrefix := provider.GetGitOpsCommentPrefix(repo) diff --git a/pkg/provider/github/parse_payload_test.go b/pkg/provider/github/parse_payload_test.go index 386f30758b..064e2b98bf 100644 --- a/pkg/provider/github/parse_payload_test.go +++ b/pkg/provider/github/parse_payload_test.go @@ -462,6 +462,7 @@ func TestParsePayLoad(t *testing.T) { objectType string gitopscommentprefix string wantRepoCRError bool + wantRequestSet bool }{ { name: "bad/unknown event", @@ -498,13 +499,6 @@ func TestParsePayLoad(t *testing.T) { triggerTarget: "pull_request", payloadEventStruct: github.CheckRunEvent{Action: github.Ptr("created")}, }, - { - name: "bad/issue comment retest only with github apps", - wantErrString: "no github client has been initialized", - eventType: "issue_comment", - triggerTarget: "pull_request", - payloadEventStruct: github.IssueCommentEvent{Action: github.Ptr("created"), Repo: sampleRepo}, - }, { name: "bad/issue comment not coming from pull request", eventType: "issue_comment", @@ -513,22 +507,6 @@ func TestParsePayLoad(t *testing.T) { payloadEventStruct: github.IssueCommentEvent{Action: github.Ptr("created"), Issue: &github.Issue{}, Repo: sampleRepo}, wantErrString: "issue comment is not coming from a pull_request", }, - { - name: "bad/issue comment invalid pullrequest", - eventType: "issue_comment", - triggerTarget: "pull_request", - githubClient: true, - payloadEventStruct: github.IssueCommentEvent{ - Action: github.Ptr("created"), - Issue: &github.Issue{ - PullRequestLinks: &github.PullRequestLinks{ - HTMLURL: github.Ptr("/bad"), - }, - }, - Repo: sampleRepo, - }, - wantErrString: "bad pull request number", - }, { name: "bad/rerequest error fetching PR", githubClient: true, @@ -755,11 +733,13 @@ func TestParsePayLoad(t *testing.T) { PullRequestLinks: &github.PullRequestLinks{ HTMLURL: github.Ptr("/666"), }, + Number: github.Ptr(666), }, Repo: sampleRepo, }, - muxReplies: map[string]any{"/repos/owner/reponame/pulls/666": samplePR}, - shaRet: "samplePRsha", + muxReplies: map[string]any{"/repos/owner/reponame/pulls/666": samplePR}, + shaRet: "samplePRsha", + wantRequestSet: true, }, { name: "good/pull request", @@ -799,6 +779,7 @@ func TestParsePayLoad(t *testing.T) { PullRequestLinks: &github.PullRequestLinks{ HTMLURL: github.Ptr("/777"), }, + Number: github.Ptr(777), }, Repo: sampleRepo, Comment: &github.IssueComment{ @@ -820,6 +801,7 @@ func TestParsePayLoad(t *testing.T) { PullRequestLinks: &github.PullRequestLinks{ HTMLURL: github.Ptr("/777"), }, + Number: github.Ptr(777), }, Repo: sampleRepo, Comment: &github.IssueComment{ @@ -842,6 +824,7 @@ func TestParsePayLoad(t *testing.T) { PullRequestLinks: &github.PullRequestLinks{ HTMLURL: github.Ptr("/777"), }, + Number: github.Ptr(777), }, Repo: sampleRepo, Comment: &github.IssueComment{ @@ -864,6 +847,7 @@ func TestParsePayLoad(t *testing.T) { PullRequestLinks: &github.PullRequestLinks{ HTMLURL: github.Ptr("/777"), }, + Number: github.Ptr(777), }, Repo: sampleRepo, Comment: &github.IssueComment{ @@ -886,6 +870,7 @@ func TestParsePayLoad(t *testing.T) { PullRequestLinks: &github.PullRequestLinks{ HTMLURL: github.Ptr("/999"), }, + Number: github.Ptr(999), }, Repo: sampleRepo, Comment: &github.IssueComment{ @@ -906,6 +891,7 @@ func TestParsePayLoad(t *testing.T) { PullRequestLinks: &github.PullRequestLinks{ HTMLURL: github.Ptr("/888"), }, + Number: github.Ptr(888), }, Repo: sampleRepo, Comment: &github.IssueComment{ @@ -1360,6 +1346,41 @@ func TestParsePayLoad(t *testing.T) { skipPushEventForPRCommits: true, muxReplies: map[string]any{"/repos/owner/pushRepo/commits/SHAPush/pulls": sampleGhPRs}, }, + { + name: "good/issue comment without ghClient initializes webhook client", + eventType: "issue_comment", + triggerTarget: "pull_request", + githubClient: false, + payloadEventStruct: github.IssueCommentEvent{ + Action: github.Ptr("created"), + Issue: &github.Issue{ + PullRequestLinks: &github.PullRequestLinks{ + HTMLURL: github.Ptr("/666"), + }, + Number: github.Ptr(666), + }, + Repo: sampleRepo, + }, + wantErrString: "cannot initialize GitHub webhook client", + }, + { + name: "bad/issue comment no matching repo", + eventType: "issue_comment", + triggerTarget: "pull_request", + githubClient: true, + payloadEventStruct: github.IssueCommentEvent{ + Action: github.Ptr("created"), + Issue: &github.Issue{ + PullRequestLinks: &github.PullRequestLinks{ + HTMLURL: github.Ptr("/666"), + }, + Number: github.Ptr(666), + }, + Repo: sampleRepo, + }, + wantRepoCRError: true, + wantErrString: "no repository found matching URL", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1397,6 +1418,15 @@ func TestParsePayLoad(t *testing.T) { Log: logger, Kube: stdata.Kube, }, + Info: info.Info{ + Controller: &info.ControllerInfo{ + Secret: "pipelines-as-code-secret", + GlobalRepository: "global-repo", + }, + Kube: &info.KubeOpts{ + Namespace: "default", + }, + }, } for key, value := range tt.muxReplies { @@ -1521,6 +1551,11 @@ func TestParsePayLoad(t *testing.T) { if tt.targetCancelPipelinerun != "" { assert.Equal(t, tt.targetCancelPipelinerun, ret.TargetCancelPipelineRun) } + if tt.wantRequestSet { + assert.Assert(t, ret.Request != nil, "Request should be set on returned event") + assert.Equal(t, ret.Request.Header.Get("X-GitHub-Event"), tt.eventType) + assert.Assert(t, len(ret.Request.Payload) > 0, "Payload should be set on returned event") + } assert.Equal(t, tt.triggerTarget, string(ret.TriggerTarget)) }) } diff --git a/pkg/provider/gitlab/parse_payload.go b/pkg/provider/gitlab/parse_payload.go index c10cc8431d..37a2e5ebc4 100644 --- a/pkg/provider/gitlab/parse_payload.go +++ b/pkg/provider/gitlab/parse_payload.go @@ -13,8 +13,8 @@ import ( "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/params/triggertype" - "github.com/openshift-pipelines/pipelines-as-code/pkg/pipelineascode" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" + "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" gitlab "gitlab.com/gitlab-org/api/client-go" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -205,7 +205,7 @@ func (v *Provider) initGitLabClient(ctx context.Context, event *info.Event) (*in return event, err } - scm := pipelineascode.SecretFromRepository{ + scm := secrets.SecretFromRepository{ K8int: kubeInterface, Config: v.GetConfig(), Event: event, diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index e7cf7a93f2..bd9a715aad 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -30,7 +30,6 @@ import ( "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/params/settings" - pac "github.com/openshift-pipelines/pipelines-as-code/pkg/pipelineascode" prmetrics "github.com/openshift-pipelines/pipelines-as-code/pkg/pipelinerunmetrics" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" providerstatus "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" @@ -286,9 +285,9 @@ func (r *Reconciler) reportFinalStatus(ctx context.Context, logger *zap.SugaredL r.run.Clients.ConsoleUI().SetParams(maptemplate) if event.InstallationID > 0 { - event.Provider.WebhookSecret, _ = pac.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run) + event.Provider.WebhookSecret, _ = secrets.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run) } else { - secretFromRepo := pac.SecretFromRepository{ + secretFromRepo := secrets.SecretFromRepository{ K8int: r.kinteract, Config: provider.GetConfig(), Event: event, @@ -424,7 +423,7 @@ func (r *Reconciler) initGitProviderClient(ctx context.Context, logger *zap.Suga // installation ID indicates Github App installation if event.InstallationID > 0 { - event.Provider.WebhookSecret, _ = pac.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run) + event.Provider.WebhookSecret, _ = secrets.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run) } else { // secretNS is needed when git provider is other than Github App. secretNS := repo.GetNamespace() @@ -435,7 +434,7 @@ func (r *Reconciler) initGitProviderClient(ctx context.Context, logger *zap.Suga repo.Spec.Merge(r.globalRepo.Spec) } - secretFromRepo := pac.SecretFromRepository{ + secretFromRepo := secrets.SecretFromRepository{ K8int: r.kinteract, Config: detectedProvider.GetConfig(), Event: event, diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 00da0ced48..e2f1d00cf2 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -72,7 +72,8 @@ func TestReconcilerReconcileKind(t *testing.T) { defer teardown() vcx := &ghprovider.Provider{ - Token: github.Ptr("None"), + Token: github.Ptr("None"), + Logger: fakelogger, } vcx.SetGithubClient(fakeclient) diff --git a/pkg/secrets/secrets.go b/pkg/secrets/pipelinerun_secrets.go similarity index 100% rename from pkg/secrets/secrets.go rename to pkg/secrets/pipelinerun_secrets.go diff --git a/pkg/secrets/secrets_test.go b/pkg/secrets/pipelinerun_secrets_test.go similarity index 100% rename from pkg/secrets/secrets_test.go rename to pkg/secrets/pipelinerun_secrets_test.go diff --git a/pkg/pipelineascode/secret.go b/pkg/secrets/secret.go similarity index 99% rename from pkg/pipelineascode/secret.go rename to pkg/secrets/secret.go index 1a73e1d18a..1ec3c948ec 100644 --- a/pkg/pipelineascode/secret.go +++ b/pkg/secrets/secret.go @@ -1,4 +1,4 @@ -package pipelineascode +package secrets import ( "context" diff --git a/pkg/pipelineascode/secret_test.go b/pkg/secrets/secret_test.go similarity index 99% rename from pkg/pipelineascode/secret_test.go rename to pkg/secrets/secret_test.go index 0fcdc48fa1..ad95dfcc08 100644 --- a/pkg/pipelineascode/secret_test.go +++ b/pkg/secrets/secret_test.go @@ -1,4 +1,4 @@ -package pipelineascode +package secrets import ( "fmt"