diff --git a/internal/commands/auth/credentials.go b/internal/commands/auth/credentials.go index 92e383d6..b6bc67e7 100644 --- a/internal/commands/auth/credentials.go +++ b/internal/commands/auth/credentials.go @@ -10,6 +10,7 @@ import ( "time" "github.com/pingidentity/pingcli/internal/configuration/options" + "github.com/pingidentity/pingcli/internal/constants" "github.com/pingidentity/pingcli/internal/customtypes" "github.com/pingidentity/pingcli/internal/errs" "github.com/pingidentity/pingcli/internal/profiles" @@ -18,13 +19,6 @@ import ( "golang.org/x/oauth2" ) -// Token storage keys for different authentication methods -const ( - deviceCodeTokenKey = "device-code-token" - authorizationCodeTokenKey = "authorization-code-token" // #nosec G101 -- This is a keychain identifier, not a credential - clientCredentialsTokenKey = "client-credentials-token" -) - var ( credentialsErrorPrefix = "failed to manage credentials" tokenManagerErrorPrefix = "failed to manage token" @@ -43,51 +37,57 @@ var newKeychainStorage = func(serviceName, username string) (tokenStorage, error // getTokenStorage returns the appropriate keychain storage instance for the given authentication method func getTokenStorage(authMethod string) (tokenStorage, error) { - return newKeychainStorage("pingcli", authMethod) + return newKeychainStorage(constants.PingCliName, authMethod) +} + +// getAuthStorageOption retrieves and normalizes the auth storage option +func getAuthStorageOption() (string, error) { + v, err := profiles.GetOptionValue(options.AuthStorageOption) + if err != nil { + return "", &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } + } + + return strings.TrimSpace(strings.ToLower(v)), nil } // shouldUseKeychain checks if keychain storage should be used based on the storage type // Returns true if storage type is secure_local (default), false for file_system/none -func shouldUseKeychain() bool { - v, err := profiles.GetOptionValue(options.AuthStorageOption) +func shouldUseKeychain() (bool, error) { + v, err := getAuthStorageOption() if err != nil { - return true // default to keychain + return false, err } switch v { case string(config.StorageTypeSecureLocal): - return true + return true, nil case string(config.StorageTypeFileSystem), string(config.StorageTypeNone), string(config.StorageTypeSecureRemote): - return false + return false, nil default: // Unrecognized: lean secure by not disabling keychain - return true + return true, nil } } // getStorageType returns the appropriate storage type for SDK keychain operations // SDK handles keychain storage, pingcli handles file storage separately func getStorageType() config.StorageType { - v, _ := profiles.GetOptionValue(options.AuthStorageOption) - s := strings.TrimSpace(strings.ToLower(v)) - if s == string(config.StorageTypeSecureLocal) || s == "" { + s, err := getAuthStorageOption() + if err != nil || s == string(config.StorageTypeSecureLocal) || s == "" { return config.StorageTypeSecureLocal } // For file_system/none/secure_remote, avoid SDK persistence (pingcli manages file persistence) return config.StorageTypeNone } -// StorageLocation indicates where credentials were saved -type StorageLocation struct { - Keychain bool - File bool -} - // LoginResult contains the result of a login operation type LoginResult struct { Token *oauth2.Token NewAuth bool - Location StorageLocation + Location customtypes.StorageLocationType } func getConfigForCurrentAuthType() (*config.Configuration, error) { @@ -157,11 +157,11 @@ func getConfigForCurrentAuthType() (*config.Configuration, error) { } } -// SaveTokenForMethod saves an OAuth2 token to file storage using the specified authentication method key +// SaveTokenForMethod saves an OAuth2 token to storage (keychain or file) using the specified authentication method key // Note: SDK handles keychain storage separately with its own token key format -// Returns StorageLocation indicating where the token was saved -func SaveTokenForMethod(token *oauth2.Token, authMethod string) (StorageLocation, error) { - location := StorageLocation{} +// Returns StorageLocationType indicating where the token was saved +func SaveTokenForMethod(token *oauth2.Token, authMethod string) (customtypes.StorageLocationType, error) { + var location customtypes.StorageLocationType if token == nil { return location, ErrNilToken @@ -182,19 +182,29 @@ func SaveTokenForMethod(token *oauth2.Token, authMethod string) (StorageLocation } // Check storage option - v, _ := profiles.GetOptionValue(options.AuthStorageOption) + v, err := getAuthStorageOption() + if err != nil { + return location, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } + } - if strings.EqualFold(v, string(config.StorageTypeNone)) { + if v == string(config.StorageTypeNone) { return location, nil } // When keychain is enabled, attempt to save to keychain. // If keychain is unavailable or save fails, fall back to file storage. - if shouldUseKeychain() { + useKeychain, err := shouldUseKeychain() + if err != nil { + return location, err + } + if useKeychain { storage, kcErr := getTokenStorage(authMethod) if kcErr == nil { if saveErr := storage.SaveToken(token); saveErr == nil { - location.Keychain = true + location = customtypes.StorageLocationKeychain return location, nil } else { @@ -203,7 +213,7 @@ func SaveTokenForMethod(token *oauth2.Token, authMethod string) (StorageLocation } if err := saveTokenToFile(token, authMethod); err == nil { - location.File = true + location = customtypes.StorageLocationFile return location, nil } else { @@ -215,7 +225,7 @@ func SaveTokenForMethod(token *oauth2.Token, authMethod string) (StorageLocation if err := saveTokenToFile(token, authMethod); err != nil { return location, err } - location.File = true + location = customtypes.StorageLocationFile return location, nil } @@ -224,13 +234,23 @@ func SaveTokenForMethod(token *oauth2.Token, authMethod string) (StorageLocation // Falls back to file storage if keychain operations fail or if --use-keychain=false func LoadTokenForMethod(authMethod string) (*oauth2.Token, error) { // Check if storage is disabled - v, _ := profiles.GetOptionValue(options.AuthStorageOption) - if strings.EqualFold(v, string(config.StorageTypeNone)) { + v, err := getAuthStorageOption() + if err != nil { + return nil, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } + } + if v == string(config.StorageTypeNone) { return nil, ErrTokenStorageDisabled } // Check if user disabled keychain - if !shouldUseKeychain() { + useKeychain, err := shouldUseKeychain() + if err != nil { + return nil, err + } + if !useKeychain { // Directly load from file storage return loadTokenFromFile(authMethod) } @@ -271,7 +291,11 @@ func GetValidTokenSource(ctx context.Context) (oauth2.TokenSource, error) { if cfg != nil { // If using file storage, try to seed refresh from existing file token before new login - if !shouldUseKeychain() { + useKeychain, skErr := shouldUseKeychain() + if skErr != nil { + return nil, skErr + } + if !useKeychain { tokenKey, err := GetAuthMethodKeyFromConfig(cfg) if err == nil && tokenKey != "" { if existingToken, ferr := loadTokenFromFile(tokenKey); ferr == nil && existingToken != nil && existingToken.RefreshToken != "" { @@ -325,7 +349,11 @@ func GetValidTokenSource(ctx context.Context) (oauth2.TokenSource, error) { // If we are not using keychain (e.g. file storage), we need to wrap the source // to capture and persist the token to file when it is refreshed or created. // The SDK's TokenSource only handles keychain persistence internally. - if !shouldUseKeychain() { + useKeychain, skErr = shouldUseKeychain() + if skErr != nil { + return nil, skErr + } + if !useKeychain { tokenKey, err := GetAuthMethodKeyFromConfig(cfg) if err == nil { return &filePersistingTokenSource{ @@ -344,7 +372,7 @@ func GetValidTokenSource(ctx context.Context) (oauth2.TokenSource, error) { } } -// ClearAllTokens removes all cached tokens from the keychain for all authentication methods. +// ClearAllTokens removes all cached tokens from keychain and file storage for all authentication methods. // This clears tokens from ALL grant types, not just the currently configured one, // to handle cases where users switch between authentication methods func ClearAllTokens() error { @@ -353,7 +381,7 @@ func ClearAllTokens() error { // First, attempt to clear ALL keychain entries for the pingcli service // This ensures we clean up tokens from previous configurations (stale Client IDs, etc.) // We use a dummy username because the constructor requires it, but ClearAllTokens operates at service level - if ks, err := newKeychainStorage("pingcli", "clearAllTokens"); err == nil { + if ks, err := newKeychainStorage(constants.PingCliName, "clearAllTokens"); err == nil { if err := ks.ClearAllTokens(); err != nil { errs = append(errs, err) } @@ -369,10 +397,8 @@ func ClearAllTokens() error { // ClearToken removes the cached token for a specific authentication method // Clears from both keychain and file storage -// Returns StorageLocation indicating what was cleared -func ClearToken(authMethod string) (StorageLocation, error) { +func ClearToken(authMethod string) error { var errList []error - location := StorageLocation{} // Clear from keychain storage, err := getTokenStorage(authMethod) @@ -382,8 +408,6 @@ func ClearToken(authMethod string) (StorageLocation, error) { Prefix: credentialsErrorPrefix, Err: err, }) - } else { - location.Keychain = true } } @@ -393,45 +417,41 @@ func ClearToken(authMethod string) (StorageLocation, error) { Prefix: credentialsErrorPrefix, Err: err, }) - } else { - location.File = true } - return location, errors.Join(errList...) + return errors.Join(errList...) } -// PerformDeviceCodeLogin performs device code authentication, returning the result -func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { - cfg, err := GetDeviceCodeConfiguration() - if err != nil { - return nil, &errs.PingCLIError{ - Prefix: credentialsErrorPrefix, - Err: err, - } - } - +// performLogin consolidates common authentication logic for different grant types +func performLogin(ctx context.Context, cfg *config.Configuration, grantType svcOAuth2.GrantType) (*LoginResult, error) { // Get profile name for token key generation profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) if err != nil { - profileName = "default" // Fallback to default if we can't get profile name + return nil, &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} } // Get service name for token key generation providerName, err := profiles.GetOptionValue(options.AuthProviderOption) - if err != nil || strings.TrimSpace(providerName) == "" { + if err != nil { + return nil, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } + } + if strings.TrimSpace(providerName) == "" { providerName = customtypes.ENUM_AUTH_PROVIDER_PINGONE // Default to pingone } - // Client ID and environment ID no longer needed for manual key generation - - // Set grant type to device code - cfg = cfg.WithGrantType(svcOAuth2.GrantTypeDeviceCode) + // Set grant type + cfg = cfg.WithGrantType(grantType) // Use SDK-consistent token key generation to avoid mismatches tokenKey, err := GetAuthMethodKeyFromConfig(cfg) - if err != nil || tokenKey == "" { - // Fallback to simple key if generation fails - tokenKey = deviceCodeTokenKey + if err != nil { + return nil, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } } // Check if we have a valid cached token before calling TokenSource @@ -439,8 +459,12 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { var existingTokenExpiry *time.Time // First try SDK keychain storage if enabled - if shouldUseKeychain() { - keychainStorage, err := svcOAuth2.NewKeychainStorage("pingcli", tokenKey) + useKeychain, skErr := shouldUseKeychain() + if skErr != nil { + return nil, skErr + } + if useKeychain { + keychainStorage, err := svcOAuth2.NewKeychainStorage(constants.PingCliName, tokenKey) if err == nil { if existingToken, err := keychainStorage.LoadToken(); err == nil && existingToken != nil && existingToken.Valid() { existingTokenExpiry = &existingToken.Expiry @@ -457,20 +481,40 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { // If using file storage and we have a refresh token, seed refresh via oauth2.ReuseTokenSource var tokenSource oauth2.TokenSource - if !shouldUseKeychain() { + + if !useKeychain { if existingToken, err := loadTokenFromFile(tokenKey); err == nil && existingToken != nil && existingToken.RefreshToken != "" { endpoints, eerr := cfg.AuthEndpoints() - if eerr == nil && cfg.Auth.DeviceCode != nil && cfg.Auth.DeviceCode.DeviceCodeClientID != nil { - var scopes []string - if cfg.Auth.DeviceCode.DeviceCodeScopes != nil { - scopes = *cfg.Auth.DeviceCode.DeviceCodeScopes + if eerr == nil { + grantType := *cfg.Auth.GrantType + var oauthCfg *oauth2.Config + switch grantType { + case svcOAuth2.GrantTypeDeviceCode: + if cfg.Auth.DeviceCode != nil && cfg.Auth.DeviceCode.DeviceCodeClientID != nil { + var scopes []string + if cfg.Auth.DeviceCode.DeviceCodeScopes != nil { + scopes = *cfg.Auth.DeviceCode.DeviceCodeScopes + } + oauthCfg = &oauth2.Config{ClientID: *cfg.Auth.DeviceCode.DeviceCodeClientID, Endpoint: endpoints.Endpoint, Scopes: scopes} + } + case svcOAuth2.GrantTypeAuthorizationCode: + if cfg.Auth.AuthorizationCode != nil && cfg.Auth.AuthorizationCode.AuthorizationCodeClientID != nil { + var scopes []string + if cfg.Auth.AuthorizationCode.AuthorizationCodeScopes != nil { + scopes = *cfg.Auth.AuthorizationCode.AuthorizationCodeScopes + } + oauthCfg = &oauth2.Config{ClientID: *cfg.Auth.AuthorizationCode.AuthorizationCodeClientID, Endpoint: endpoints.Endpoint, Scopes: scopes} + } + } + + if oauthCfg != nil { + baseTS := oauthCfg.TokenSource(ctx, existingToken) + tokenSource = oauth2.ReuseTokenSource(nil, baseTS) } - oauthCfg := &oauth2.Config{ClientID: *cfg.Auth.DeviceCode.DeviceCodeClientID, Endpoint: endpoints.Endpoint, Scopes: scopes} - baseTS := oauthCfg.TokenSource(ctx, existingToken) - tokenSource = oauth2.ReuseTokenSource(nil, baseTS) } } } + // Fallback to SDK token source if we didn't create a seeded one if tokenSource == nil { var tsErr error @@ -482,6 +526,9 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { } } } + + // Get token (SDK will return cached token if valid, or perform new authentication) + token, err := tokenSource.Token() if err != nil { return nil, &errs.PingCLIError{ Prefix: credentialsErrorPrefix, @@ -489,19 +536,15 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { } } - // Get token (SDK will return cached token if valid, or perform new authentication) - token, err := tokenSource.Token() + // Clean up old token files for this grant type and profile (in case configuration changed) + err = clearAllTokenFilesForGrantType(providerName, string(grantType), profileName) if err != nil { return nil, &errs.PingCLIError{ Prefix: credentialsErrorPrefix, - Err: err, + Err: fmt.Errorf("failed to clean up old token files: %w", err), } } - // Clean up old token files for this grant type and profile (in case configuration changed) - // Ignore errors from cleanup - we still want to save the new token - _ = clearAllTokenFilesForGrantType(providerName, string(svcOAuth2.GrantTypeDeviceCode), profileName) - // Save token using our own storage logic (handles both file and keychain based on flags) location, err := SaveTokenForMethod(token, tokenKey) if err != nil { @@ -516,7 +559,6 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { // If expiry is different, new auth was performed isNewAuth := existingTokenExpiry == nil || !token.Expiry.Equal(*existingTokenExpiry) - // NewAuth indicates whether new authentication was performed return &LoginResult{ Token: token, NewAuth: isNewAuth, @@ -524,6 +566,19 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { }, nil } +// PerformDeviceCodeLogin performs device code authentication, returning the result +func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { + cfg, err := GetDeviceCodeConfiguration() + if err != nil { + return nil, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } + } + + return performLogin(ctx, cfg, svcOAuth2.GrantTypeDeviceCode) +} + // GetDeviceCodeConfiguration builds a device code authentication configuration from the CLI profile options func GetDeviceCodeConfiguration() (*config.Configuration, error) { cfg := config.NewConfiguration() @@ -551,14 +606,23 @@ func GetDeviceCodeConfiguration() (*config.Configuration, error) { cfg = cfg.WithDeviceCodeScopes(scopeDefaults) // Configure storage options based on --file-storage flag - cfg = cfg.WithStorageType(getStorageType()).WithStorageName("pingcli") + cfg = cfg.WithStorageType(getStorageType()).WithStorageName(constants.PingCliName) // Provide optional suffix so SDK keychain entries align with file names - profileName, _ := profiles.GetOptionValue(options.RootActiveProfileOption) + profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil { + return nil, &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} + } if strings.TrimSpace(profileName) == "" { - profileName = "default" + profileName = constants.DefaultProfileName + } + providerName, err := profiles.GetOptionValue(options.AuthProviderOption) + if err != nil { + return nil, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } } - providerName, _ := profiles.GetOptionValue(options.AuthProviderOption) if strings.TrimSpace(providerName) == "" { providerName = customtypes.ENUM_AUTH_PROVIDER_PINGONE } @@ -606,118 +670,7 @@ func PerformAuthorizationCodeLogin(ctx context.Context) (*LoginResult, error) { } } - // Get profile name for token key generation - profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) - if err != nil { - profileName = "default" // Fallback to default if we can't get profile name - } - - // Get service name for token key generation - providerName, err := profiles.GetOptionValue(options.AuthProviderOption) - if err != nil || strings.TrimSpace(providerName) == "" { - providerName = customtypes.ENUM_AUTH_PROVIDER_PINGONE // Default to pingone - } - - // Client ID and environment ID no longer needed for manual key generation - - // Set grant type to authorization code - cfg = cfg.WithGrantType(svcOAuth2.GrantTypeAuthorizationCode) - - // Use SDK-consistent token key generation to avoid mismatches - tokenKey, err := GetAuthMethodKeyFromConfig(cfg) - if err != nil || tokenKey == "" { - // Fallback to simple key if generation fails - tokenKey = authorizationCodeTokenKey - } - - // Check if we have a valid cached token before calling TokenSource - // Store the existing token's expiry to compare later - var existingTokenExpiry *time.Time - - // First try SDK keychain storage if enabled - if shouldUseKeychain() { - keychainStorage, err := svcOAuth2.NewKeychainStorage("pingcli", tokenKey) - if err == nil { - if existingToken, err := keychainStorage.LoadToken(); err == nil && existingToken != nil && existingToken.Valid() { - existingTokenExpiry = &existingToken.Expiry - } - } - } - - // If not found in keychain, check file storage - if existingTokenExpiry == nil { - if existingToken, err := loadTokenFromFile(tokenKey); err == nil && existingToken != nil && existingToken.Valid() { - existingTokenExpiry = &existingToken.Expiry - } - } - - // If using file storage and we have a refresh token, seed refresh via oauth2.ReuseTokenSource - var tokenSource oauth2.TokenSource - if !shouldUseKeychain() { - if existingToken, err := loadTokenFromFile(tokenKey); err == nil && existingToken != nil && existingToken.RefreshToken != "" { - endpoints, eerr := cfg.AuthEndpoints() - if eerr == nil && cfg.Auth.AuthorizationCode != nil && cfg.Auth.AuthorizationCode.AuthorizationCodeClientID != nil { - var scopes []string - if cfg.Auth.AuthorizationCode.AuthorizationCodeScopes != nil { - scopes = *cfg.Auth.AuthorizationCode.AuthorizationCodeScopes - } - oauthCfg := &oauth2.Config{ClientID: *cfg.Auth.AuthorizationCode.AuthorizationCodeClientID, Endpoint: endpoints.Endpoint, Scopes: scopes} - baseTS := oauthCfg.TokenSource(ctx, existingToken) - tokenSource = oauth2.ReuseTokenSource(nil, baseTS) - } - } - } - // Fallback to SDK token source if we didn't create a seeded one - if tokenSource == nil { - var tsErr error - tokenSource, tsErr = cfg.TokenSource(ctx) - if tsErr != nil { - return nil, &errs.PingCLIError{ - Prefix: credentialsErrorPrefix, - Err: tsErr, - } - } - } - if err != nil { - return nil, &errs.PingCLIError{ - Prefix: credentialsErrorPrefix, - Err: err, - } - } - - // Get token (SDK will return cached token if valid, or perform new authentication) - token, err := tokenSource.Token() - if err != nil { - return nil, &errs.PingCLIError{ - Prefix: credentialsErrorPrefix, - Err: err, - } - } - - // Clean up old token files for this grant type and profile (in case configuration changed) - // Ignore errors from cleanup - we still want to save the new token - _ = clearAllTokenFilesForGrantType(providerName, string(svcOAuth2.GrantTypeAuthorizationCode), profileName) - - // Save token using our own storage logic (handles both file and keychain based on flags) - location, err := SaveTokenForMethod(token, tokenKey) - if err != nil { - return nil, &errs.PingCLIError{ - Prefix: credentialsErrorPrefix, - Err: err, - } - } - - // Determine if this was new authentication - // If we had an existing token with the same expiry, it's cached - // If expiry is different, new auth was performed - isNewAuth := existingTokenExpiry == nil || !token.Expiry.Equal(*existingTokenExpiry) - - // NewAuth indicates whether new authentication was performed - return &LoginResult{ - Token: token, - NewAuth: isNewAuth, - Location: location, - }, nil + return performLogin(ctx, cfg, svcOAuth2.GrantTypeAuthorizationCode) } // GetAuthorizationCodeConfiguration builds an authorization code authentication configuration from the CLI profile options @@ -779,19 +732,28 @@ func GetAuthorizationCodeConfiguration() (*config.Configuration, error) { WithAuthorizationCodeRedirectURI(redirectURI) // This is the default scope. Additional scopes can be appended by the user later if needed. - scopeDefaults := []string{"openid"} + scopeDefaults := []string{constants.OpenIDScope} cfg = cfg.WithAuthorizationCodeScopes(scopeDefaults) // Configure storage options based on --file-storage flag cfg = cfg.WithStorageType(getStorageType()). - WithStorageName("pingcli") + WithStorageName(constants.PingCliName) // Provide optional suffix so SDK keychain entries align with file names - profileName, _ := profiles.GetOptionValue(options.RootActiveProfileOption) + profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil { + return nil, &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} + } if strings.TrimSpace(profileName) == "" { - profileName = "default" + profileName = constants.DefaultProfileName + } + providerName, err := profiles.GetOptionValue(options.AuthProviderOption) + if err != nil { + return nil, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } } - providerName, _ := profiles.GetOptionValue(options.AuthProviderOption) if strings.TrimSpace(providerName) == "" { providerName = customtypes.ENUM_AUTH_PROVIDER_PINGONE } @@ -839,93 +801,7 @@ func PerformClientCredentialsLogin(ctx context.Context) (*LoginResult, error) { } } - // Get profile name for token key generation - profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) - if err != nil { - profileName = "default" // Fallback to default if we can't get profile name - } - - // Get service name for token key generation - providerName, err := profiles.GetOptionValue(options.AuthProviderOption) - if err != nil || strings.TrimSpace(providerName) == "" { - providerName = customtypes.ENUM_AUTH_PROVIDER_PINGONE // Default to pingone - } - - // Client ID and environment ID no longer needed for manual key generation - - // Set grant type to client credentials - cfg = cfg.WithGrantType(svcOAuth2.GrantTypeClientCredentials) - - // Use SDK-consistent token key generation to avoid mismatches - tokenKey, err := GetAuthMethodKeyFromConfig(cfg) - if err != nil || tokenKey == "" { - // Fallback to simple key if generation fails - tokenKey = clientCredentialsTokenKey - } - - // Check if we have a valid cached token before calling TokenSource - // Store the existing token's expiry to compare later - var existingTokenExpiry *time.Time - - // First try SDK keychain storage if enabled - if shouldUseKeychain() { - keychainStorage, err := svcOAuth2.NewKeychainStorage("pingcli", tokenKey) - if err == nil { - if existingToken, err := keychainStorage.LoadToken(); err == nil && existingToken != nil && existingToken.Valid() { - existingTokenExpiry = &existingToken.Expiry - } - } - } - - // If not found in keychain, check file storage - if existingTokenExpiry == nil { - if existingToken, err := loadTokenFromFile(tokenKey); err == nil && existingToken != nil && existingToken.Valid() { - existingTokenExpiry = &existingToken.Expiry - } - } - - // Get token source - SDK handles keychain storage based on configuration - tokenSource, err := cfg.TokenSource(ctx) - if err != nil { - return nil, &errs.PingCLIError{ - Prefix: credentialsErrorPrefix, - Err: err, - } - } - - // Get token (SDK will return cached token if valid, or perform new authentication) - token, err := tokenSource.Token() - if err != nil { - return nil, &errs.PingCLIError{ - Prefix: credentialsErrorPrefix, - Err: err, - } - } - - // Clean up old token files for this grant type and profile (in case configuration changed) - // Ignore errors from cleanup - we still want to save the new token - _ = clearAllTokenFilesForGrantType(providerName, string(svcOAuth2.GrantTypeClientCredentials), profileName) - - // Save token using our own storage logic (handles both file and keychain based on flags) - location, err := SaveTokenForMethod(token, tokenKey) - if err != nil { - return nil, &errs.PingCLIError{ - Prefix: credentialsErrorPrefix, - Err: err, - } - } - - // Determine if this was new authentication - // If we had an existing token with the same expiry, it's cached - // If expiry is different, new auth was performed - isNewAuth := existingTokenExpiry == nil || !token.Expiry.Equal(*existingTokenExpiry) - - // NewAuth indicates whether new authentication was performed - return &LoginResult{ - Token: token, - NewAuth: isNewAuth, - Location: location, - }, nil + return performLogin(ctx, cfg, svcOAuth2.GrantTypeClientCredentials) } // GetClientCredentialsConfiguration builds a client credentials authentication configuration from the CLI profile options @@ -967,19 +843,28 @@ func GetClientCredentialsConfiguration() (*config.Configuration, error) { WithClientCredentialsClientSecret(clientSecret) // This is the default scope. Additional scopes can be appended by the user later if needed. - scopeDefaults := []string{"openid"} + scopeDefaults := []string{constants.OpenIDScope} cfg = cfg.WithClientCredentialsScopes(scopeDefaults) // Configure storage options based on --file-storage flag cfg = cfg.WithStorageType(getStorageType()). - WithStorageName("pingcli") + WithStorageName(constants.PingCliName) // Provide optional suffix so SDK keychain entries align with file names - profileName, _ := profiles.GetOptionValue(options.RootActiveProfileOption) + profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil { + return nil, &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} + } if strings.TrimSpace(profileName) == "" { - profileName = "default" + profileName = constants.DefaultProfileName + } + providerName, err := profiles.GetOptionValue(options.AuthProviderOption) + if err != nil { + return nil, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } } - providerName, _ := profiles.GetOptionValue(options.AuthProviderOption) if strings.TrimSpace(providerName) == "" { providerName = customtypes.ENUM_AUTH_PROVIDER_PINGONE } @@ -1049,18 +934,27 @@ func GetWorkerConfiguration() (*config.Configuration, error) { cfg = cfg.WithClientCredentialsClientID(clientID). WithClientCredentialsClientSecret(clientSecret) // Align default scopes with client credentials flow - scopeDefaults := []string{"openid"} + scopeDefaults := []string{constants.OpenIDScope} cfg = cfg.WithClientCredentialsScopes(scopeDefaults) // Configure storage options based on --file-storage flag cfg = cfg.WithStorageType(getStorageType()). - WithStorageName("pingcli") + WithStorageName(constants.PingCliName) // Provide optional suffix so SDK keychain entries align with file names - profileName, _ := profiles.GetOptionValue(options.RootActiveProfileOption) + profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil { + return nil, &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} + } if strings.TrimSpace(profileName) == "" { - profileName = "default" + profileName = constants.DefaultProfileName + } + providerName, err := profiles.GetOptionValue(options.AuthProviderOption) + if err != nil { + return nil, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } } - providerName, _ := profiles.GetOptionValue(options.AuthProviderOption) if strings.TrimSpace(providerName) == "" { providerName = customtypes.ENUM_AUTH_PROVIDER_PINGONE } @@ -1131,13 +1025,22 @@ func GetAuthMethodKeyFromConfig(cfg *config.Configuration) (string, error) { } // Build suffix to disambiguate across provider/grant/profile for both keychain and files - profileName, _ := profiles.GetOptionValue(options.RootActiveProfileOption) + profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil { + return "", &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} + } if profileName == "" { - profileName = "default" + profileName = constants.DefaultProfileName + } + providerName, err := profiles.GetOptionValue(options.AuthProviderOption) + if err != nil { + return "", &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } } - providerName, _ := profiles.GetOptionValue(options.AuthProviderOption) if strings.TrimSpace(providerName) == "" { - providerName = "pingone" + providerName = customtypes.ENUM_AUTH_PROVIDER_PINGONE } suffix := fmt.Sprintf("_%s_%s_%s", providerName, string(grantType), profileName) // Use the SDK's GenerateKeychainAccountName with optional suffix diff --git a/internal/commands/auth/credentials_storage_fallback_test.go b/internal/commands/auth/credentials_storage_fallback_test.go index 2683ddeb..ff362f07 100644 --- a/internal/commands/auth/credentials_storage_fallback_test.go +++ b/internal/commands/auth/credentials_storage_fallback_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/pingidentity/pingcli/internal/constants" + "github.com/pingidentity/pingcli/internal/customtypes" "github.com/pingidentity/pingcli/internal/testing/testutils_koanf" "github.com/stretchr/testify/mock" "golang.org/x/oauth2" @@ -84,11 +85,11 @@ func TestSaveTokenForMethod_FallsBackToFileWhenKeychainSaveFails(t *testing.T) { if err != nil { t.Fatalf("expected no error, got %v", err) } - if location.Keychain { - t.Fatalf("expected Keychain=false, got true") + if location == customtypes.StorageLocationKeychain { + t.Fatalf("expected storage location to not be %s", customtypes.StorageLocationKeychain) } - if !location.File { - t.Fatalf("expected File=true, got false") + if location != customtypes.StorageLocationFile { + t.Fatalf("expected storage location %s, got %s", customtypes.StorageLocationFile, location) } // Verify it actually wrote the expected file under HOME @@ -137,11 +138,11 @@ func TestSaveTokenForMethod_UsesKeychainWhenAvailable(t *testing.T) { mockStorage.AssertExpectations(t) - if !location.Keychain { - t.Fatalf("expected Keychain=true") + if location != customtypes.StorageLocationKeychain { + t.Fatalf("expected storage location %s, got %s", customtypes.StorageLocationKeychain, location) } - if location.File { - t.Fatalf("expected File=false") + if location == customtypes.StorageLocationFile { + t.Fatalf("expected storage location to not be %s", customtypes.StorageLocationFile) } // File should not be written when keychain save succeeds diff --git a/internal/commands/auth/credentials_test.go b/internal/commands/auth/credentials_test.go index e4e436b4..09c48749 100644 --- a/internal/commands/auth/credentials_test.go +++ b/internal/commands/auth/credentials_test.go @@ -13,6 +13,7 @@ import ( auth_internal "github.com/pingidentity/pingcli/internal/commands/auth" "github.com/pingidentity/pingcli/internal/configuration/options" "github.com/pingidentity/pingcli/internal/constants" + "github.com/pingidentity/pingcli/internal/customtypes" "github.com/pingidentity/pingcli/internal/profiles" "github.com/pingidentity/pingcli/internal/testing/testutils_koanf" "golang.org/x/oauth2" @@ -264,7 +265,7 @@ func TestClearToken(t *testing.T) { // Test that ClearTokenForMethod doesn't panic when no token exists // This should handle the case where keychain entry doesn't exist - _, err := auth_internal.ClearToken(testKey) + err := auth_internal.ClearToken(testKey) // Should not error when no token exists (handles ErrNotFound) if err != nil { @@ -775,10 +776,10 @@ func TestSaveTokenForMethod_StorageTypeNone(t *testing.T) { } // Verify neither File nor Keychain storage was used - if location.File { + if location == customtypes.StorageLocationFile { t.Error("Expected File storage to be false, got true") } - if location.Keychain { + if location == customtypes.StorageLocationKeychain { t.Error("Expected Keychain storage to be false, got true") } diff --git a/internal/commands/auth/login_internal.go b/internal/commands/auth/login_internal.go index 9befaa89..1d588174 100644 --- a/internal/commands/auth/login_internal.go +++ b/internal/commands/auth/login_internal.go @@ -46,9 +46,20 @@ func AuthLoginRunE(cmd *cobra.Command, args []string) error { switch provider { case customtypes.ENUM_AUTH_PROVIDER_PINGONE: // Determine desired authentication method - deviceCodeStr, _ := profiles.GetOptionValue(options.AuthMethodDeviceCodeOption) - clientCredentialsStr, _ := profiles.GetOptionValue(options.AuthMethodClientCredentialsOption) - authorizationCodeStr, _ := profiles.GetOptionValue(options.AuthMethodAuthorizationCodeOption) + deviceCodeStr, err := profiles.GetOptionValue(options.AuthMethodDeviceCodeOption) + if err != nil { + return &errs.PingCLIError{Prefix: loginErrorPrefix, Err: err} + } + + clientCredentialsStr, err := profiles.GetOptionValue(options.AuthMethodClientCredentialsOption) + if err != nil { + return &errs.PingCLIError{Prefix: loginErrorPrefix, Err: err} + } + + authorizationCodeStr, err := profiles.GetOptionValue(options.AuthMethodAuthorizationCodeOption) + if err != nil { + return &errs.PingCLIError{Prefix: loginErrorPrefix, Err: err} + } flagProvided := deviceCodeStr == "true" || clientCredentialsStr == "true" || authorizationCodeStr == "true" @@ -186,7 +197,7 @@ func performLoginByConfiguredType(ctx context.Context, authType, profileName str } // displayLoginSuccess displays the successful login message -func displayLoginSuccess(token *oauth2.Token, newAuth bool, location StorageLocation, selectedMethod, profileName string) { +func displayLoginSuccess(token *oauth2.Token, newAuth bool, location customtypes.StorageLocationType, selectedMethod, profileName string) { if newAuth { // Build storage location message storageMsg := formatStorageLocation(location) diff --git a/internal/commands/auth/logout_internal.go b/internal/commands/auth/logout_internal.go index 823a7eae..e4beefac 100644 --- a/internal/commands/auth/logout_internal.go +++ b/internal/commands/auth/logout_internal.go @@ -15,14 +15,29 @@ import ( "github.com/spf13/cobra" ) +var ( + logoutErrorPrefix = "failed to logout" +) + // AuthLogoutRunE implements the logout command logic, clearing credentials from both // keychain and file storage. If no grant type flag is provided, clears all tokens. // If a specific grant type flag is provided, clears only that method's token. func AuthLogoutRunE(cmd *cobra.Command, args []string) error { // Check if any grant type flags were provided - deviceCodeStr, _ := profiles.GetOptionValue(options.AuthMethodDeviceCodeOption) - clientCredentialsStr, _ := profiles.GetOptionValue(options.AuthMethodClientCredentialsOption) - authorizationCodeStr, _ := profiles.GetOptionValue(options.AuthMethodAuthorizationCodeOption) + deviceCodeStr, err := profiles.GetOptionValue(options.AuthMethodDeviceCodeOption) + if err != nil { + return &errs.PingCLIError{Prefix: logoutErrorPrefix, Err: err} + } + + clientCredentialsStr, err := profiles.GetOptionValue(options.AuthMethodClientCredentialsOption) + if err != nil { + return &errs.PingCLIError{Prefix: logoutErrorPrefix, Err: err} + } + + authorizationCodeStr, err := profiles.GetOptionValue(options.AuthMethodAuthorizationCodeOption) + if err != nil { + return &errs.PingCLIError{Prefix: logoutErrorPrefix, Err: err} + } flagProvided := deviceCodeStr == "true" || clientCredentialsStr == "true" || authorizationCodeStr == "true" @@ -71,9 +86,9 @@ func AuthLogoutRunE(cmd *cobra.Command, args []string) error { } // Clear only the token for the specified grant type - location, err := ClearToken(tokenKey) + err = ClearToken(tokenKey) if err != nil { - return &errs.PingCLIError{Prefix: credentialsErrorPrefix, Err: fmt.Errorf("failed to clear %s credentials. in %s: %w", authType, formatStorageLocation(location), err)} + return &errs.PingCLIError{Prefix: credentialsErrorPrefix, Err: fmt.Errorf("failed to clear %s credentials: %w", authType, err)} } output.Success(fmt.Sprintf("Successfully logged out and cleared credentials from %s for service '%s' using profile '%s'.", authType, providerName, profileName), nil) @@ -147,12 +162,12 @@ func GetAuthMethodKey(authMethod string) (string, error) { } // Build suffix to disambiguate across provider/grant/profile for both keychain and files - profileName, _ := profiles.GetOptionValue(options.RootActiveProfileOption) - if profileName == "" { + profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil || profileName == "" { profileName = "default" } - providerName, _ := profiles.GetOptionValue(options.AuthProviderOption) - if strings.TrimSpace(providerName) == "" { + providerName, err := profiles.GetOptionValue(options.AuthProviderOption) + if err != nil || strings.TrimSpace(providerName) == "" { providerName = "pingone" } suffix := fmt.Sprintf("_%s_%s_%s", providerName, string(grantType), profileName) diff --git a/internal/commands/auth/use_keychain_test.go b/internal/commands/auth/use_keychain_test.go index 3367ef55..453b58df 100644 --- a/internal/commands/auth/use_keychain_test.go +++ b/internal/commands/auth/use_keychain_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/pingidentity/pingcli/internal/customtypes" "github.com/pingidentity/pingcli/internal/testing/testutils_koanf" svcOAuth2 "github.com/pingidentity/pingone-go-client/oauth2" "golang.org/x/oauth2" @@ -40,8 +41,8 @@ func TestSaveTokenForMethod_WithKeychainDisabled(t *testing.T) { } // Verify location indicates file storage only - if !location.File || location.Keychain { - t.Errorf("Expected file storage only (File=true, Keychain=false), got %+v", location) + if location != customtypes.StorageLocationFile { + t.Errorf("Expected file storage only, got %+v", location) } // Verify token was saved to file @@ -72,7 +73,7 @@ func TestSaveTokenForMethod_WithKeychainEnabled(t *testing.T) { authMethod := "test-keychain-enabled" t.Cleanup(func() { - _, _ = ClearToken(authMethod) + _ = ClearToken(authMethod) }) // Save token - should try keychain first @@ -84,9 +85,9 @@ func TestSaveTokenForMethod_WithKeychainEnabled(t *testing.T) { } else { t.Logf("Token saved to: %v", location) - // If it's expected to be in keychain (location.Keychain=true), we must manually save it there + // If it's expected to be in keychain (location == StorageLocationKeychain), we must manually save it there // because SaveTokenForMethod doesn't actually perform the save (it assumes SDK did it) - if location.Keychain { + if location == customtypes.StorageLocationKeychain { storage, sErr := svcOAuth2.NewKeychainStorage("pingcli", authMethod) if sErr != nil { if strings.Contains(sErr.Error(), "keychain") || strings.Contains(sErr.Error(), "freedesktop") { @@ -174,7 +175,7 @@ func TestLoadTokenForMethod_FallbackToFileStorage(t *testing.T) { t.Cleanup(func() { _ = clearTokenFromFile(authMethod) - _, _ = ClearToken(authMethod) + _ = ClearToken(authMethod) }) // Save token only to file storage (keychain disabled) @@ -214,7 +215,7 @@ func TestShouldUseKeychain_Default(t *testing.T) { authMethod := "test-default-keychain" t.Cleanup(func() { - _, _ = ClearToken(authMethod) + _ = ClearToken(authMethod) }) // Save token - should try keychain by default @@ -224,8 +225,8 @@ func TestShouldUseKeychain_Default(t *testing.T) { } else { t.Logf("Token saved with default settings to: %v", location) - // If it's expected to be in keychain (location.Keychain=true), we must manually save it there - if location.Keychain { + // If it's expected to be in keychain (location == StorageLocationKeychain), we must manually save it there + if location == customtypes.StorageLocationKeychain { storage, sErr := svcOAuth2.NewKeychainStorage("pingcli", authMethod) if sErr != nil { if strings.Contains(sErr.Error(), "keychain") || strings.Contains(sErr.Error(), "freedesktop") { @@ -270,7 +271,7 @@ func TestClearToken_ClearsBothStorages(t *testing.T) { authMethod := "test-clear-both-storages" t.Cleanup(func() { - _, _ = ClearToken(authMethod) + _ = ClearToken(authMethod) }) // Save to file storage directly @@ -286,7 +287,7 @@ func TestClearToken_ClearsBothStorages(t *testing.T) { } // Clear token - should remove from both keychain and file storage - _, err = ClearToken(authMethod) + err = ClearToken(authMethod) if err != nil { t.Logf("ClearToken returned error (may be expected if keychain not available): %v", err) } @@ -334,7 +335,7 @@ func TestSaveTokenForMethod_FileStorageFallback(t *testing.T) { authMethod := "test-save-fallback" t.Cleanup(func() { - _, _ = ClearToken(authMethod) + _ = ClearToken(authMethod) }) // Save token - will try keychain first (may succeed or fail depending on environment) @@ -344,8 +345,8 @@ func TestSaveTokenForMethod_FileStorageFallback(t *testing.T) { } else { t.Logf("Token saved - fallback test to: %v", location) - // If it's expected to be in keychain (location.Keychain=true), we must manually save it there - if location.Keychain { + // If it's expected to be in keychain (location == StorageLocationKeychain), we must manually save it there + if location == customtypes.StorageLocationKeychain { storage, sErr := svcOAuth2.NewKeychainStorage("pingcli", authMethod) if sErr == nil { // Don't skip here if save fails, because we are testing fallback? @@ -427,8 +428,8 @@ func TestEnvironmentVariable_FileStorage(t *testing.T) { } // Verify location indicates file storage - if !location.File || location.Keychain { - t.Errorf("Expected file storage with env var (File=true, Keychain=false), got %+v", location) + if location != customtypes.StorageLocationFile { + t.Errorf("Expected file storage with env var, got %+v", location) } // Verify token was saved to file (since file-storage is true) diff --git a/internal/commands/auth/utils.go b/internal/commands/auth/utils.go index 56cc9d2b..4c058f54 100644 --- a/internal/commands/auth/utils.go +++ b/internal/commands/auth/utils.go @@ -4,7 +4,6 @@ package auth_internal import ( "fmt" - "strings" "github.com/pingidentity/pingcli/internal/configuration/options" "github.com/pingidentity/pingcli/internal/customtypes" @@ -44,19 +43,14 @@ func applyRegionConfiguration(cfg *config.Configuration) (*config.Configuration, } // formatStorageLocation returns a human-friendly message for where credentials were cleared -// based on StorageLocation. -func formatStorageLocation(location StorageLocation) string { - var locs []string - if location.Keychain { - locs = append(locs, "keychain") - } - if location.File { - locs = append(locs, "file storage") - } - - if len(locs) == 0 { +// based on StorageLocationType. +func formatStorageLocation(location customtypes.StorageLocationType) string { + switch location { + case customtypes.StorageLocationKeychain: + return "keychain" + case customtypes.StorageLocationFile: + return "file storage" + default: return "storage" } - - return strings.Join(locs, " and ") } diff --git a/internal/commands/platform/export_internal.go b/internal/commands/platform/export_internal.go index b5194f83..45a73356 100644 --- a/internal/commands/platform/export_internal.go +++ b/internal/commands/platform/export_internal.go @@ -345,9 +345,18 @@ func initPingOneApiClient(ctx context.Context, pingcliVersion string) (err error return &errs.PingCLIError{Prefix: exportErrorPrefix, Err: ErrNilContext} } - workerClientID, _ := profiles.GetOptionValue(options.PingOneAuthenticationWorkerClientIDOption) - workerClientSecret, _ := profiles.GetOptionValue(options.PingOneAuthenticationWorkerClientSecretOption) - workerEnvironmentID, _ := profiles.GetOptionValue(options.PingOneAuthenticationWorkerEnvironmentIDOption) + workerClientID, err := profiles.GetOptionValue(options.PingOneAuthenticationWorkerClientIDOption) + if err != nil { + return &errs.PingCLIError{Prefix: exportErrorPrefix, Err: err} + } + workerClientSecret, err := profiles.GetOptionValue(options.PingOneAuthenticationWorkerClientSecretOption) + if err != nil { + return &errs.PingCLIError{Prefix: exportErrorPrefix, Err: err} + } + workerEnvironmentID, err := profiles.GetOptionValue(options.PingOneAuthenticationWorkerEnvironmentIDOption) + if err != nil { + return &errs.PingCLIError{Prefix: exportErrorPrefix, Err: err} + } regionCode, err := profiles.GetOptionValue(options.PingOneRegionCodeOption) if err != nil { return &errs.PingCLIError{Prefix: exportErrorPrefix, Err: err} @@ -357,7 +366,10 @@ func initPingOneApiClient(ctx context.Context, pingcliVersion string) (err error return &errs.PingCLIError{Prefix: exportErrorPrefix, Err: ErrRegionCodeRequired} } - authType, _ := profiles.GetOptionValue(options.PingOneAuthenticationTypeOption) + authType, err := profiles.GetOptionValue(options.PingOneAuthenticationTypeOption) + if err != nil { + return &errs.PingCLIError{Prefix: exportErrorPrefix, Err: err} + } userAgent := fmt.Sprintf("pingcli/%s", pingcliVersion) if v := strings.TrimSpace(os.Getenv("PINGCLI_PINGONE_APPEND_USER_AGENT")); v != "" { diff --git a/internal/constants/constants.go b/internal/constants/constants.go index bb734287..04e55c71 100644 --- a/internal/constants/constants.go +++ b/internal/constants/constants.go @@ -6,4 +6,6 @@ const ( PingCliName = "pingcli" PingCliDirName = ".pingcli" CredentialsDirName = "credentials" + DefaultProfileName = "default" + OpenIDScope = "openid" ) diff --git a/internal/customtypes/storage_location_type.go b/internal/customtypes/storage_location_type.go new file mode 100644 index 00000000..c2a156fa --- /dev/null +++ b/internal/customtypes/storage_location_type.go @@ -0,0 +1,11 @@ +// Copyright © 2026 Ping Identity Corporation + +package customtypes + +// StorageLocationType defines the type of storage where credentials are saved +type StorageLocationType string + +const ( + StorageLocationKeychain StorageLocationType = "keychain" + StorageLocationFile StorageLocationType = "file" +)