diff --git a/internal/auth/auth.go b/internal/auth/auth.go index c920719..2afaa71 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -114,8 +114,8 @@ func Login(ctx context.Context) error { TokenType: token.TokenType, TokenEndpoint: oauthCfg.Endpoint.TokenURL, // enables future token refresh } - if err := SaveTokens(host, stored); err != nil { - return fmt.Errorf("saving tokens: %w", err) + if err := persistLoginState(host, stored); err != nil { + return err } if email != "" { @@ -126,7 +126,21 @@ func Login(ctx context.Context) error { return nil } -// Logout removes stored OAuth tokens and clears the config file. +// persistLoginState stores the resolved host in config and persists OAuth tokens. +// Saving the host here ensures a successful `glean auth login` remains usable +// even when the host originally came from an environment variable. +func persistLoginState(host string, tok *StoredTokens) error { + if err := config.SaveHostToFile(host); err != nil { + return fmt.Errorf("saving host: %w", err) + } + if err := SaveTokens(host, tok); err != nil { + return fmt.Errorf("saving tokens: %w", err) + } + return nil +} + +// Logout removes stored OAuth tokens, OAuth client registration, and any saved +// config/keyring credentials for the current host. func Logout(ctx context.Context) error { cfg, err := config.LoadConfig() if err != nil || cfg.GleanHost == "" { @@ -135,9 +149,11 @@ func Logout(ctx context.Context) error { if err := DeleteTokens(cfg.GleanHost); err != nil { return fmt.Errorf("removing tokens: %w", err) } - // Also wipe the config file so any stored API token / host is cleared. - if config.ConfigPath != "" { - _ = os.Remove(config.ConfigPath) + if err := DeleteClient(cfg.GleanHost); err != nil { + return fmt.Errorf("removing oauth client: %w", err) + } + if err := config.ClearConfig(); err != nil { + return fmt.Errorf("clearing config: %w", err) } fmt.Printf("✓ Logged out from Glean (%s)\n", cfg.GleanHost) return nil @@ -293,7 +309,7 @@ func resolveHost(ctx context.Context) (string, error) { host = strings.TrimPrefix(host, "http://") host = strings.SplitN(host, "/", 2)[0] - _ = config.SaveConfig(host, "") + _ = config.SaveHostToFile(host) return host, nil } diff --git a/internal/auth/auth_persistence_test.go b/internal/auth/auth_persistence_test.go new file mode 100644 index 0000000..4970d8c --- /dev/null +++ b/internal/auth/auth_persistence_test.go @@ -0,0 +1,108 @@ +package auth_test + +import ( + "context" + "path/filepath" + "testing" + "time" + + "github.com/gleanwork/glean-cli/internal/auth" + gleanClient "github.com/gleanwork/glean-cli/internal/client" + "github.com/gleanwork/glean-cli/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/zalando/go-keyring" +) + +func isolateAuthState(t *testing.T) { + t.Helper() + + home := t.TempDir() + t.Setenv("HOME", home) + + oldConfigPath := config.ConfigPath + config.ConfigPath = filepath.Join(home, ".glean", "config.json") + t.Cleanup(func() { config.ConfigPath = oldConfigPath }) + + oldServiceName := config.ServiceName + config.ServiceName = "glean-cli-test-auth-persistence" + t.Cleanup(func() { config.ServiceName = oldServiceName }) + + keyring.MockInit() +} + +func oauthToken() *auth.StoredTokens { + return &auth.StoredTokens{ + AccessToken: "oauth-access-token", + RefreshToken: "oauth-refresh-token", + Expiry: time.Now().Add(time.Hour), + Email: "user@example.com", + TokenType: "Bearer", + TokenEndpoint: "https://example.com/oauth/token", + } +} + +func TestOAuthLoginStateRequiresPersistedHostAfterEnvHostIsRemoved(t *testing.T) { + isolateAuthState(t) + + const host = "acme-be.glean.com" + require.NoError(t, auth.SaveTokens(host, oauthToken())) + + t.Setenv("GLEAN_HOST", host) + cfg, err := config.LoadConfig() + require.NoError(t, err) + + token, authType := gleanClient.ResolveToken(cfg) + assert.Equal(t, "oauth-access-token", token) + assert.Equal(t, "OAUTH", authType) + + // Simulate a fresh shell/session after login where GLEAN_HOST is no longer set. + t.Setenv("GLEAN_HOST", "") + cfg, err = config.LoadConfig() + require.NoError(t, err) + assert.Empty(t, cfg.GleanHost, "host was never persisted by login") + + token, authType = gleanClient.ResolveToken(cfg) + assert.Empty(t, token) + assert.Empty(t, authType) +} + +func TestOAuthTokenResolvesWhenHostIsPersisted(t *testing.T) { + isolateAuthState(t) + + const host = "acme-be.glean.com" + require.NoError(t, config.SaveHostToFile(host)) + require.NoError(t, auth.SaveTokens(host, oauthToken())) + + cfg, err := config.LoadConfig() + require.NoError(t, err) + require.Equal(t, host, cfg.GleanHost) + + token, authType := gleanClient.ResolveToken(cfg) + assert.Equal(t, "oauth-access-token", token) + assert.Equal(t, "OAUTH", authType) +} + +func TestLogoutClearsPersistedHostAndOAuthTokens(t *testing.T) { + isolateAuthState(t) + + const host = "acme-be.glean.com" + require.NoError(t, config.SaveHostToFile(host)) + require.NoError(t, auth.SaveTokens(host, oauthToken())) + require.NoError(t, auth.SaveClient(host, &auth.StoredClient{ClientID: "cid-123"})) + + require.NoError(t, auth.Logout(context.Background())) + + cfg, err := config.LoadConfig() + require.NoError(t, err) + assert.Empty(t, cfg.GleanHost) + assert.Empty(t, cfg.GleanToken) + + tok, err := auth.LoadTokens(host) + require.NoError(t, err) + assert.Nil(t, tok) + + cl, err := auth.LoadClient(host) + require.NoError(t, err) + assert.Nil(t, cl) +} diff --git a/internal/auth/storage.go b/internal/auth/storage.go index 0bb3bd6..f13d15c 100644 --- a/internal/auth/storage.go +++ b/internal/auth/storage.go @@ -123,3 +123,12 @@ func LoadClient(host string) (*StoredClient, error) { } return &cl, nil } + +// DeleteClient removes a stored client registration for the given host. +func DeleteClient(host string) error { + err := os.Remove(clientPath(host)) + if os.IsNotExist(err) { + return nil + } + return err +} diff --git a/internal/auth/storage_test.go b/internal/auth/storage_test.go index c6a9027..1058773 100644 --- a/internal/auth/storage_test.go +++ b/internal/auth/storage_test.go @@ -62,6 +62,16 @@ func TestSaveAndLoadClient(t *testing.T) { assert.Equal(t, "cid-123", got.ClientID) } +func TestDeleteClient(t *testing.T) { + withTempHome(t) + cl := &StoredClient{ClientID: "cid-123", ClientSecret: "cs-abc"} + require.NoError(t, SaveClient("host.glean.com", cl)) + require.NoError(t, DeleteClient("host.glean.com")) + got, err := LoadClient("host.glean.com") + require.NoError(t, err) + assert.Nil(t, got) +} + func TestStoredTokens_IsExpired(t *testing.T) { assert.True(t, (&StoredTokens{Expiry: time.Now().Add(-time.Minute)}).IsExpired()) assert.False(t, (&StoredTokens{Expiry: time.Now().Add(time.Hour)}).IsExpired()) diff --git a/internal/config/config.go b/internal/config/config.go index 2e27629..963a5a8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -157,6 +157,28 @@ func SaveConfig(host, token string) error { return nil } +// SaveHostToFile persists only the host in ~/.glean/config.json without touching +// the system keyring. This is intended for OAuth flows where the host is not +// secret and persisting it should not trigger OS keychain prompts. +func SaveHostToFile(host string) error { + if host != "" { + validHost, err := ValidateAndTransformHost(host) + if err != nil { + return err + } + host = validHost + } + + cfg := &Config{} + existingCfg, err := loadFromFile() + if err == nil { + cfg = existingCfg + } + cfg.GleanHost = host + + return saveToFile(cfg) +} + // ClearConfig removes all stored configuration from both keyring and file storage. func ClearConfig() error { var keyringErr error diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 2437856..811e4e8 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -314,6 +314,21 @@ func TestLoadConfig_EnvHostWithFileToken(t *testing.T) { assert.Equal(t, "file-token", result.GleanToken, "token from file must be used even when host comes from env") } +func TestSaveHostToFile_DoesNotTouchKeyring(t *testing.T) { + mock, cleanupKeyring := setupTestKeyring(t) + _, cleanupConfig := setupTestConfig(t) + defer cleanupKeyring() + defer cleanupConfig() + + mock.err = assert.AnError + require.NoError(t, SaveHostToFile("linkedin")) + + cfg, err := loadFromFile() + require.NoError(t, err) + assert.Equal(t, "linkedin-be.glean.com", cfg.GleanHost) + assert.Empty(t, cfg.GleanToken) +} + func TestLoadFromFile(t *testing.T) { _, cleanup := setupTestConfig(t) defer cleanup()