diff --git a/internal/appsetup/appsetup.go b/internal/appsetup/appsetup.go index ee746fa4d1..ff7a03a650 100644 --- a/internal/appsetup/appsetup.go +++ b/internal/appsetup/appsetup.go @@ -19,6 +19,7 @@ import ( "os/exec" "regexp" "runtime" + "strconv" "strings" "time" @@ -129,6 +130,7 @@ type Setup struct { permErrors []string publicApps bool appSet string + storedAppIDs map[string]string // org/role → app_id from ROLE_APP_IDS } // NewSetup creates a new Setup instance. @@ -170,6 +172,13 @@ func (s *Setup) WithPublicApps(public bool) *Setup { return s } +// WithStoredAppIDs sets the stored ROLE_APP_IDS mapping (org/role → app_id) +// used to detect stale credentials when an app is deleted and recreated. +func (s *Setup) WithStoredAppIDs(ids map[string]string) *Setup { + s.storedAppIDs = ids + return s +} + // appSetPattern validates app set slugs: lowercase alphanumeric with hyphens, // must start with a letter or digit, no leading/trailing/consecutive hyphens. var appSetPattern = regexp.MustCompile(`^[a-z0-9]+(-[a-z0-9]+)*$`) @@ -431,6 +440,19 @@ func (s *Setup) recoverPEM(ctx context.Context, org, slug, role string) (string, return pemStr, nil } +// isAppIDStale checks whether the live installation's app ID differs from the +// stored ROLE_APP_IDS value, indicating the app was deleted and recreated. +func (s *Setup) isAppIDStale(org, role string, liveID int) bool { + if s.storedAppIDs == nil { + return false + } + storedID, ok := s.storedAppIDs[org+"/"+role] + if !ok { + return false + } + return storedID != strconv.Itoa(liveID) +} + // handleExistingApp reuses an existing app if its credentials are still // available, or reports that the private key is lost. // @@ -438,6 +460,7 @@ func (s *Setup) recoverPEM(ctx context.Context, org, slug, role string) (string, // manifest code exchange (POST /app-manifests/{code}/conversions) is the // one and only time the PEM is returned. If the secret wasn't stored or // was deleted, the key is lost and the app must be deleted and recreated. +// // The secretExists callback checks the appropriate backend (Secret Manager // in OIDC mint mode, GitHub repo secrets otherwise). // @@ -457,7 +480,9 @@ func (s *Setup) handleExistingApp(ctx context.Context, inst *forge.Installation, return nil, fmt.Errorf("checking secret for role %s: %w", role, err) } - if exists { + stale := s.isAppIDStale(org, role, inst.AppID) + + if exists && !stale { s.checkPermissions(inst, org, role) s.ui.StepDone(fmt.Sprintf("Reusing existing app %s (credentials present)", inst.AppSlug)) return &AppCredentials{ @@ -469,12 +494,26 @@ func (s *Setup) handleExistingApp(ctx context.Context, inst *forge.Installation, }, nil } - // Secret doesn't exist — try to recover by generating a new key. + if exists && stale { + s.ui.StepWarn(fmt.Sprintf( + "App %s was recreated (ID changed) — stored key is invalid", + inst.AppSlug)) + } + + // Secret doesn't exist or is stale — try to recover by generating a new key. pemStr, recoverErr := s.recoverPEM(ctx, org, inst.AppSlug, role) if recoverErr != nil { return nil, fmt.Errorf("recovering PEM for %s: %w", inst.AppSlug, recoverErr) } if pemStr == "" { + if stale { + return nil, fmt.Errorf( + "app %s was recreated (ID changed) and needs a new private key; "+ + "generate one at https://github.com/apps/%s "+ + "or run 'fullsend admin uninstall' and re-run install", + inst.AppSlug, inst.AppSlug, + ) + } return nil, fmt.Errorf( "app %s exists but its private key secret is missing; "+ "run 'fullsend admin uninstall' first, then delete the app at "+ diff --git a/internal/appsetup/appsetup_test.go b/internal/appsetup/appsetup_test.go index 9bd0ce15ff..8e7ee2b576 100644 --- a/internal/appsetup/appsetup_test.go +++ b/internal/appsetup/appsetup_test.go @@ -676,6 +676,136 @@ func TestSetup_ExistingApp_PEMRecovery_FileNotFound(t *testing.T) { assert.Contains(t, err.Error(), "checking PEM file") } +func TestSetup_ExistingApp_StaleAppID_TriggersRecovery(t *testing.T) { + pemData := generateTestPEM(t) + pemPath := writeTempPEM(t, pemData) + + client := &forge.FakeClient{ + Installations: []forge.Installation{ + {ID: 100, AppID: 20, AppSlug: "fullsend-fullsend"}, + }, + AppClientIDs: map[string]string{ + "fullsend-fullsend": "Iv1.fullsend123", + }, + } + prompter := &fakePrompter{confirmResult: true, readLineResult: pemPath} + printer := ui.New(&discardWriter{}) + + var storedPEM string + s := NewSetup(client, prompter, newFakeBrowser(), printer). + WithAppSet("fullsend"). + WithSecretExists(func(_ string) (bool, error) { return true, nil }). + WithStoredAppIDs(map[string]string{"myorg/fullsend": "10"}). + WithStoreSecret(func(_ context.Context, _, p string) error { + storedPEM = p + return nil + }) + + creds, err := s.Run(context.Background(), "myorg", "fullsend") + require.NoError(t, err) + assert.Equal(t, 20, creds.AppID) + assert.NotEmpty(t, creds.PEM, "should have new PEM from recovery") + assert.NotEmpty(t, storedPEM, "should have stored new PEM") + assert.True(t, prompter.confirmCalled, "should prompt for PEM recovery") +} + +func TestSetup_ExistingApp_MatchingAppID_Reuses(t *testing.T) { + client := &forge.FakeClient{ + Installations: []forge.Installation{ + {ID: 100, AppID: 10, AppSlug: "fullsend-fullsend"}, + }, + AppClientIDs: map[string]string{ + "fullsend-fullsend": "Iv1.fullsend123", + }, + } + prompter := &fakePrompter{} + printer := ui.New(&discardWriter{}) + + s := NewSetup(client, prompter, newFakeBrowser(), printer). + WithAppSet("fullsend"). + WithSecretExists(func(_ string) (bool, error) { return true, nil }). + WithStoredAppIDs(map[string]string{"myorg/fullsend": "10"}) + + creds, err := s.Run(context.Background(), "myorg", "fullsend") + require.NoError(t, err) + assert.Equal(t, 10, creds.AppID) + assert.Empty(t, creds.PEM, "PEM should be empty to signal reuse") + assert.False(t, prompter.confirmCalled, "should not prompt — IDs match") +} + +func TestSetup_ExistingApp_NoStoredIDs_Reuses(t *testing.T) { + client := &forge.FakeClient{ + Installations: []forge.Installation{ + {ID: 100, AppID: 10, AppSlug: "fullsend-fullsend"}, + }, + AppClientIDs: map[string]string{ + "fullsend-fullsend": "Iv1.fullsend123", + }, + } + prompter := &fakePrompter{} + printer := ui.New(&discardWriter{}) + + // No WithStoredAppIDs — backwards compatible behavior. + s := NewSetup(client, prompter, newFakeBrowser(), printer). + WithAppSet("fullsend"). + WithSecretExists(func(_ string) (bool, error) { return true, nil }) + + creds, err := s.Run(context.Background(), "myorg", "fullsend") + require.NoError(t, err) + assert.Equal(t, 10, creds.AppID) + assert.Empty(t, creds.PEM, "PEM should be empty to signal reuse") + assert.False(t, prompter.confirmCalled, "should not prompt — no stored IDs to compare") +} + +func TestIsAppIDStale(t *testing.T) { + s := &Setup{} + + t.Run("nil map returns false", func(t *testing.T) { + assert.False(t, s.isAppIDStale("org", "role", 10)) + }) + + s.storedAppIDs = map[string]string{ + "myorg/fullsend": "10", + "myorg/prioritize": "20", + } + + t.Run("matching ID returns false", func(t *testing.T) { + assert.False(t, s.isAppIDStale("myorg", "fullsend", 10)) + }) + + t.Run("mismatched ID returns true", func(t *testing.T) { + assert.True(t, s.isAppIDStale("myorg", "fullsend", 99)) + }) + + t.Run("unknown key returns false", func(t *testing.T) { + assert.False(t, s.isAppIDStale("otherog", "fullsend", 10)) + }) +} + +func TestSetup_ExistingApp_StaleAppID_UserDeclines(t *testing.T) { + client := &forge.FakeClient{ + Installations: []forge.Installation{ + {ID: 100, AppID: 20, AppSlug: "fullsend-fullsend"}, + }, + AppClientIDs: map[string]string{ + "fullsend-fullsend": "Iv1.fullsend123", + }, + } + prompter := &fakePrompter{confirmResult: false} + printer := ui.New(&discardWriter{}) + + s := NewSetup(client, prompter, newFakeBrowser(), printer). + WithAppSet("fullsend"). + WithSecretExists(func(_ string) (bool, error) { return true, nil }). + WithStoredAppIDs(map[string]string{"myorg/fullsend": "10"}) + + _, err := s.Run(context.Background(), "myorg", "fullsend") + require.Error(t, err) + assert.Contains(t, err.Error(), "was recreated") + assert.True(t, prompter.confirmCalled, "should prompt for PEM recovery") + assert.False(t, prompter.readLineCalled, "should not ask for file path after decline") +} + func TestValidateRSAPEM_Valid(t *testing.T) { assert.NoError(t, ValidateRSAPEM(generateTestPEM(t))) } diff --git a/internal/cli/admin.go b/internal/cli/admin.go index 25e56b8752..29e15c2df5 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -487,12 +487,14 @@ Inference authentication: // Pre-copy PEM secrets for shared public apps before app setup. var sharedSlugs map[string]string + var perOrgStoredIDs map[string]string if mintProject != "" && !skipAppSetup && !skipMintCheck { - slugs, err := copySharedAppPEMs(ctx, client, printer, org, roles, mintProject, mintRegion) + slugs, storedIDs, err := copySharedAppPEMs(ctx, client, printer, org, roles, mintProject, mintRegion) if err != nil { return err } sharedSlugs = slugs + perOrgStoredIDs = storedIDs } // Collect agent credentials via app setup. @@ -501,7 +503,7 @@ Inference authentication: if err := ensureConfigRepoExists(ctx, client, printer, org); err != nil { return err } - creds, err := runAppSetup(ctx, client, printer, org, roles, mintProject, publicApps, sharedSlugs, appSet) + creds, err := runAppSetup(ctx, client, printer, org, roles, mintProject, publicApps, sharedSlugs, appSet, perOrgStoredIDs) if err != nil { return err } @@ -832,14 +834,17 @@ func runPerRepoInstall(ctx context.Context, c perRepoInstallConfig) error { if needAppSetup { var sharedSlugs map[string]string if mintProject != "" { - slugs, slugErr := copySharedAppPEMs(ctx, client, printer, owner, roles, mintProject, mintRegion) + slugs, storedIDs, slugErr := copySharedAppPEMs(ctx, client, printer, owner, roles, mintProject, mintRegion) if slugErr != nil { return slugErr } sharedSlugs = slugs + if existingIDs == nil { + existingIDs = storedIDs + } } - creds, credErr := runAppSetup(ctx, client, printer, owner, roles, mintProject, publicApps, sharedSlugs, c.AppSet) + creds, credErr := runAppSetup(ctx, client, printer, owner, roles, mintProject, publicApps, sharedSlugs, c.AppSet, existingIDs) if credErr != nil { return credErr } @@ -1230,9 +1235,10 @@ func resolveSharedRoleAppIDs(ctx context.Context, client forge.Client, existingI // their PEM secrets to the target org's naming convention. This runs before // app setup so that handleExistingApp finds the PEM and returns credentials // without trying to generate a new key. -// Returns a role → app-slug mapping for detected shared apps so callers -// can pass them as known slugs to app setup. -func copySharedAppPEMs(ctx context.Context, client forge.Client, printer *ui.Printer, org string, roles []string, mintProject, mintRegion string) (map[string]string, error) { +// Returns a role → app-slug mapping for detected shared apps and the full +// ROLE_APP_IDS map (org/role → app_id) so callers can pass it to app setup +// without a redundant GCP API call. +func copySharedAppPEMs(ctx context.Context, client forge.Client, printer *ui.Printer, org string, roles []string, mintProject, mintRegion string) (map[string]string, map[string]string, error) { prov := gcf.NewProvisioner(gcf.Config{ ProjectID: mintProject, Region: mintRegion, @@ -1240,13 +1246,17 @@ func copySharedAppPEMs(ctx context.Context, client forge.Client, printer *ui.Pri }, gcf.NewLiveGCFClient()) existingIDs, err := prov.GetExistingRoleAppIDs(ctx) - if err != nil || len(existingIDs) == 0 { - return nil, nil + if err != nil { + printer.StepWarn(fmt.Sprintf("Could not read ROLE_APP_IDS: %v", err)) + return nil, nil, nil + } + if len(existingIDs) == 0 { + return nil, nil, nil } installations, err := client.ListOrgInstallations(ctx, org) if err != nil { - return nil, nil + return nil, existingIDs, nil } roleSet := make(map[string]bool, len(roles)) @@ -1279,25 +1289,26 @@ func copySharedAppPEMs(ctx context.Context, client forge.Client, printer *ui.Pri printer.StepStart(fmt.Sprintf("Shared app detected: %s (app %d) — copying PEM from %s", role, inst.AppID, srcOrg)) if err := prov.CopyAgentPEM(ctx, srcOrg, org, role); err != nil { - return nil, fmt.Errorf("copying shared PEM for %s: %w", role, err) + return nil, nil, fmt.Errorf("copying shared PEM for %s: %w", role, err) } printer.StepDone(fmt.Sprintf("Copied shared %s PEM", role)) break } } - return sharedSlugs, nil + return sharedSlugs, existingIDs, nil } // runAppSetup creates or reuses GitHub Apps for each role. When mintProject is // non-empty, PEMs are also stored in GCP Secret Manager during app creation so // they survive partial provisioning failures. -func runAppSetup(ctx context.Context, client forge.Client, printer *ui.Printer, org string, roles []string, mintProject string, publicApps bool, sharedSlugs map[string]string, appSet string) ([]layers.AgentCredentials, error) { +func runAppSetup(ctx context.Context, client forge.Client, printer *ui.Printer, org string, roles []string, mintProject string, publicApps bool, sharedSlugs map[string]string, appSet string, storedAppIDs map[string]string) ([]layers.AgentCredentials, error) { printer.Header("Setting up GitHub Apps") printer.Blank() setup := appsetup.NewSetup(client, appsetup.StdinPrompter{}, appsetup.DefaultBrowser{}, printer). WithPublicApps(publicApps). - WithAppSet(appSet) + WithAppSet(appSet). + WithStoredAppIDs(storedAppIDs) // Merge known slugs: config-based first, then shared app overrides. knownSlugs := loadKnownSlugs(ctx, client, org)