diff --git a/pkg/sources/github/connector.go b/pkg/sources/github/connector.go index 3f460295b..bd4932511 100644 --- a/pkg/sources/github/connector.go +++ b/pkg/sources/github/connector.go @@ -117,7 +117,7 @@ func newConnector(ctx context.Context, source *Source) (Connector, error) { switch cred := source.conn.GetCredential().(type) { case *sourcespb.GitHub_GithubApp: log.RedactGlobally(cred.GithubApp.GetPrivateKey()) - return NewAppConnector(ctx, apiEndpoint, cred.GithubApp) + return NewAppConnector(ctx, apiEndpoint, cred.GithubApp, source.conn.GetScanAllInstallations()) case *sourcespb.GitHub_BasicAuth: log.RedactGlobally(cred.BasicAuth.GetPassword()) return NewBasicAuthConnector(ctx, apiEndpoint, source.conn.GetClonePath(), cred.BasicAuth) diff --git a/pkg/sources/github/connector_app.go b/pkg/sources/github/connector_app.go index cef1567db..4d3c34678 100644 --- a/pkg/sources/github/connector_app.go +++ b/pkg/sources/github/connector_app.go @@ -45,10 +45,16 @@ var _ Connector = (*appConnector)(nil) const githubHTTPTimeoutSeconds = 60 -func NewAppConnector(ctx context.Context, apiEndpoint string, app *credentialspb.GitHubApp) (Connector, error) { - installationID, err := strconv.ParseInt(app.InstallationId, 10, 64) - if err != nil { - return nil, fmt.Errorf("could not parse app installation ID %q: %w", app.InstallationId, err) +func NewAppConnector(ctx context.Context, apiEndpoint string, app *credentialspb.GitHubApp, scanAllInstallations bool) (Connector, error) { + var installationID int64 + if app.InstallationId != "" { + var err error + installationID, err = strconv.ParseInt(app.InstallationId, 10, 64) + if err != nil { + return nil, fmt.Errorf("could not parse app installation ID %q: %w", app.InstallationId, err) + } + } else if !scanAllInstallations { + return nil, fmt.Errorf("githubApp.installationId is required unless scanAllInstallations is set") } appID, err := strconv.ParseInt(app.AppId, 10, 64) @@ -75,25 +81,45 @@ func NewAppConnector(ctx context.Context, apiEndpoint string, app *credentialspb repoInstallationMap: make(map[string]int64), } - if _, err := connector.APIClientForInstallation(installationID); err != nil { - return nil, fmt.Errorf("could not create API client for configured installation: %w", err) - } + // When no default installation is configured (installationID == 0, only + // possible with scanAllInstallations), installations are discovered and + // clients created lazily per-repo, so there is no default client to + // validate up front. + if installationID != 0 { + if _, err := connector.APIClientForInstallation(installationID); err != nil { + return nil, fmt.Errorf("could not create API client for configured installation: %w", err) + } - if _, err := connector.graphqlClientForInstallation(ctx, installationID); err != nil { - return nil, fmt.Errorf("error creating GraphQL client: %w", err) + if _, err := connector.graphqlClientForInstallation(ctx, installationID); err != nil { + return nil, fmt.Errorf("error creating GraphQL client: %w", err) + } } return connector, nil } -func (c *appConnector) APIClient() *github.Client { - c.mu.RLock() - defer c.mu.RUnlock() +// HasDefaultInstallation reports whether a default installation is +// configured. False only when scanAllInstallations is set and +// githubApp.installationId was omitted, in which case there is no +// authoritative installation to fall back to for repos/gists discovered +// outside any installation's own listing. +func (c *appConnector) HasDefaultInstallation() bool { + return c.installationID != 0 +} - if clients := c.clientsByInstallationID[c.installationID]; clients != nil { - return clients.apiClient +// APIClient returns the client for the connector's default installation. +// When scanAllInstallations is configured without a githubApp.installationId +// (installationID == 0), NewAppConnector does not pre-create this client, so +// it is created lazily here rather than returning nil: several callers +// (Validate, mapRemainingAccessibleRepos, member gist/repo lookups) use this +// as a fallback client for repos/resources outside any installation listing +// and call it unconditionally. +func (c *appConnector) APIClient() *github.Client { + client, err := c.APIClientForInstallation(c.installationID) + if err != nil { + return nil } - return nil + return client } func (c *appConnector) APIClientForRepo(repoURL string) (*github.Client, error) { @@ -140,6 +166,15 @@ func (c *appConnector) graphqlClientForInstallation(ctx context.Context, install func (c *appConnector) Clone(ctx context.Context, repoURL string, args ...string) (string, *gogit.Repository, error) { installID, _ := c.installationIDForRepo(repoURL) + if installID == 0 { + // installID falls back to the connector's default installationID + // (see installationIDForRepo/ensureRepoInstallation) for repos + // discovered outside any installation's repo listing, e.g. org + // members' personal repos with scanUsers. That default is unset (0) + // when scanAllInstallations is true and githubApp.installationId + // was omitted, since no single installation is authoritative. + return "", nil, fmt.Errorf("no GitHub App installation resolved for repo %q; set githubApp.installationId to a fallback installation to scan repos outside installation listings (e.g. member repos with scanUsers) together with scanAllInstallations", repoURL) + } // TODO: Check rate limit for this call. token, _, err := c.installationClient.Apps.CreateInstallationToken( @@ -216,14 +251,15 @@ func (c *appConnector) setRepoInstallationForWiki(repoURL string, installationID } } +// GraphQLClient returns the GraphQL client for the connector's default +// installation, lazily creating it if needed. See APIClient for why this +// cannot simply return nil when no default installation is configured. func (c *appConnector) GraphQLClient() *githubv4.Client { - c.mu.RLock() - defer c.mu.RUnlock() - - if clients := c.clientsByInstallationID[c.installationID]; clients != nil { - return clients.graphqlClient + client, err := c.graphqlClientForInstallation(context.Background(), c.installationID) + if err != nil { + return nil } - return nil + return client } func (c *appConnector) InstallationClient() *github.Client { diff --git a/pkg/sources/github/connector_app_test.go b/pkg/sources/github/connector_app_test.go index 9d1cbfa87..d0bd479d7 100644 --- a/pkg/sources/github/connector_app_test.go +++ b/pkg/sources/github/connector_app_test.go @@ -152,7 +152,7 @@ func TestNewAppConnectorDefaultAPIClientUsesConfiguredInstallation(t *testing.T) PrivateKey: string(privKey), InstallationId: "4242", AppId: "12345", - }) + }, false) require.NoError(t, err) require.NotNil(t, connector.APIClient()) require.NotNil(t, connector.GraphQLClient()) @@ -174,6 +174,75 @@ func TestNewAppConnectorDefaultAPIClientUsesConfiguredInstallation(t *testing.T) assert.Contains(t, tokenRequestPaths[0], "/app/installations/4242/access_tokens") } +func TestNewAppConnectorInstallationIDOptionalWithScanAllInstallations(t *testing.T) { + privKey := generateTestPrivateKey(t) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode([]map[string]string{}) + })) + defer server.Close() + + // scanAllInstallations=true with no installationId configured should succeed. + connector, err := NewAppConnector(trContext.Background(), server.URL, &credentialspb.GitHubApp{ + PrivateKey: string(privKey), + AppId: "12345", + }, true) + require.NoError(t, err) + require.NotNil(t, connector) + + // Without scanAllInstallations, installationId is still required. + _, err = NewAppConnector(trContext.Background(), server.URL, &credentialspb.GitHubApp{ + PrivateKey: string(privKey), + AppId: "12345", + }, false) + require.Error(t, err) +} + +// TestAPIClientAndGraphQLClientNonNilWithoutDefaultInstallation guards against +// a nil-pointer panic: APIClient/GraphQLClient must never return nil, since +// callers like Validate, mapRemainingAccessibleRepos, and member gist/repo +// lookups call them unconditionally and dereference the result. Before this +// fix, an appConnector with no default installation (installationID == 0, +// the case when scanAllInstallations is true and installationId is omitted) +// returned nil from a bare map lookup instead of lazily creating a client. +func TestAPIClientAndGraphQLClientNonNilWithoutDefaultInstallation(t *testing.T) { + privKey := generateTestPrivateKey(t) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode([]map[string]string{}) + })) + defer server.Close() + + connector, err := NewAppConnector(trContext.Background(), server.URL, &credentialspb.GitHubApp{ + PrivateKey: string(privKey), + AppId: "12345", + }, true) + require.NoError(t, err) + + assert.NotNil(t, connector.APIClient()) + assert.NotNil(t, connector.GraphQLClient()) +} + +// TestCloneErrorsWithoutResolvedInstallation covers repos that fall back to +// the connector's default installationID (e.g. member personal repos found +// outside any installation's repo listing, see ensureRepoInstallation). When +// scanAllInstallations is true and no githubApp.installationId is +// configured, that default is unset (0); Clone must fail with an actionable +// error instead of attempting to use installation ID 0. +func TestCloneErrorsWithoutResolvedInstallation(t *testing.T) { + connector := &appConnector{ + installationID: 0, + clientsByInstallationID: make(map[int64]*appInstallationClients), + repoInstallationMap: make(map[string]int64), + } + + _, _, err := connector.Clone(trContext.Background(), "https://github.com/some-member/personal-repo.git") + require.Error(t, err) + assert.Contains(t, err.Error(), "no GitHub App installation resolved") +} + func TestAPIClientForInstallationUsesConfiguredClient(t *testing.T) { defaultClient := github.NewClient(nil) connector := &appConnector{ diff --git a/pkg/sources/github/github.go b/pkg/sources/github/github.go index 61170789a..1ccc72714 100644 --- a/pkg/sources/github/github.go +++ b/pkg/sources/github/github.go @@ -402,6 +402,17 @@ func (s *Source) Validate(ctx context.Context) []error { - Returns 401 for invalid credentials but works with no auth (as unauthenticated) - Doesn't consume API quota when called */ + // When scanAllInstallations is set with no githubApp.installationId, + // there's no default installation token to call RateLimit with (see + // appConnector.HasDefaultInstallation). Validate the App credentials + // directly against the app-level (JWT-authenticated) client instead. + if connector, ok := s.connector.(*appConnector); ok && !connector.HasDefaultInstallation() { + if _, _, err := connector.InstallationClient().Apps.Get(ctx, ""); err != nil { + return []error{err} + } + return nil + } + if _, _, err := s.connector.APIClient().RateLimit.Get(ctx); err != nil { return []error{err} } @@ -584,6 +595,9 @@ func (s *Source) ensureRepoInfoCache(ctx context.Context, repo string, reporter } if !s.ignoreGists && isGistUrl(urlParts) { + if connector, ok := s.connector.(*appConnector); ok && !connector.HasDefaultInstallation() { + return repo, fmt.Errorf("cannot fetch gist %q: githubApp.installationId is required as a fallback installation to scan gists together with scanAllInstallations", repo) + } // Cache gist info. for { gistID := extractGistID(urlParts) @@ -753,6 +767,15 @@ func (s *Source) enumerateWithApp(ctx context.Context, connector *appConnector, return err } ctx.Logger().Info("Scanning repos", "org_members", len(s.memberCache)) + if !connector.HasDefaultInstallation() { + // Member repos/gists live outside any installation's own + // listing, so there is no installation token to fetch them + // with (see appConnector.HasDefaultInstallation). Skip with a + // clear message instead of letting each per-member API call + // fail against installation ID 0. + ctx.Logger().Info("Skipping org member repos and gists: githubApp.installationId is required as a fallback installation to scan members together with scanAllInstallations", "org_members", len(s.memberCache)) + return nil + } // TODO: Replace loop below with a call to s.addReposForMembers(ctx, reporter) for member := range s.memberCache { logger := ctx.Logger().WithValues("member", member) @@ -1130,6 +1153,10 @@ func (s *Source) mapRemainingAccessibleRepos( connector *appConnector, wantedRepos map[string]repoMappingRequest, ) error { + if !connector.HasDefaultInstallation() { + return fmt.Errorf("cannot resolve %d configured repo(s) outside installation listings: githubApp.installationId is required as a fallback installation together with scanAllInstallations", len(wantedRepos)) + } + var errs error client := connector.APIClient() for _, requested := range wantedRepos { diff --git a/pkg/sources/github/github_test.go b/pkg/sources/github/github_test.go index 906924016..9a0d2e6ff 100644 --- a/pkg/sources/github/github_test.go +++ b/pkg/sources/github/github_test.go @@ -382,7 +382,8 @@ func TestAppConnector_EnterpriseBaseURL(t *testing.T) { PrivateKey: privateKey, InstallationId: "1337", AppId: "4141", - }) + }, + false) require.NoError(t, err) appConn, ok := connector.(*appConnector) @@ -411,6 +412,40 @@ func TestAppConnector_EnterpriseBaseURL(t *testing.T) { "AppsTransport.BaseURL should be set to enterprise endpoint") } +// TestValidate_ScanAllInstallationsWithoutDefaultInstallation covers the +// scanAllInstallations-without-githubApp.installationId case: there is no +// default installation token to call RateLimit with (see +// appConnector.HasDefaultInstallation), so Validate must fall back to +// checking App credentials via the app-level (JWT) client instead of +// erroring against a non-existent installation ID 0. +func TestValidate_ScanAllInstallationsWithoutDefaultInstallation(t *testing.T) { + defer gock.Off() + + gock.New("https://api.github.com"). + Get("/app/installations"). + Reply(200). + JSON([]map[string]any{}) + gock.New("https://api.github.com"). + Get("^/app$"). + Reply(200). + JSON(map[string]any{"id": 4141}) + + s := initTestSource(&sourcespb.GitHub{ + Endpoint: "https://api.github.com", + ScanAllInstallations: true, + Credential: &sourcespb.GitHub_GithubApp{ + GithubApp: &credentialspb.GitHubApp{ + PrivateKey: createPrivateKey(), + AppId: "4141", + }, + }}) + + errs := s.Validate(context.Background()) + assert.Empty(t, errs) + assert.False(t, gock.HasUnmatchedRequest()) + assert.True(t, gock.IsDone()) +} + func TestAddOrgsByUser(t *testing.T) { defer gock.Off()