Make GitHub App installationId optional when scanning all installations (#5275)

When scanAllInstallations is true, the scanner no longer requires
githubApp.installationId in the config. Repos, gists, and org
members outside any installation's listing now fail with a clear
error instead of the app hitting a nil client or a bad request.

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)

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 {

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{

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 {

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()