From cb3a1154f88bd48f5d9f46e4daf94341351ae5a4 Mon Sep 17 00:00:00 2001 From: wesleymccollam Date: Tue, 10 Feb 2026 15:37:56 -0500 Subject: [PATCH 1/4] reduce AuthStorageOption duplication, refactor the StorageLocation to enum, add missing error handling for some GetOptionValue lines --- internal/commands/auth/credentials.go | 105 ++++++++++-------- .../auth/credentials_storage_fallback_test.go | 9 +- internal/commands/auth/credentials_test.go | 5 +- internal/commands/auth/login_internal.go | 19 +++- internal/commands/auth/logout_internal.go | 29 +++-- internal/commands/auth/use_keychain_test.go | 21 ++-- internal/commands/auth/utils.go | 22 ++-- internal/commands/platform/export_internal.go | 20 +++- internal/customtypes/storage_location_type.go | 11 ++ 9 files changed, 150 insertions(+), 91 deletions(-) create mode 100644 internal/customtypes/storage_location_type.go diff --git a/internal/commands/auth/credentials.go b/internal/commands/auth/credentials.go index 92e383d6..47b835c3 100644 --- a/internal/commands/auth/credentials.go +++ b/internal/commands/auth/credentials.go @@ -46,10 +46,20 @@ func getTokenStorage(authMethod string) (tokenStorage, error) { return newKeychainStorage("pingcli", authMethod) } +// getAuthStorageOption retrieves and normalizes the auth storage option +func getAuthStorageOption() (string, error) { + v, err := profiles.GetOptionValue(options.AuthStorageOption) + if err != nil { + return "", 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) + v, err := getAuthStorageOption() if err != nil { return true // default to keychain } @@ -68,8 +78,11 @@ func shouldUseKeychain() bool { // 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)) + s, err := getAuthStorageOption() + if err != nil { + return config.StorageTypeSecureLocal + } + if s == string(config.StorageTypeSecureLocal) || s == "" { return config.StorageTypeSecureLocal } @@ -77,17 +90,11 @@ func getStorageType() config.StorageType { 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) { @@ -159,9 +166,9 @@ func getConfigForCurrentAuthType() (*config.Configuration, error) { // SaveTokenForMethod saves an OAuth2 token to file storage 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,9 +189,12 @@ func SaveTokenForMethod(token *oauth2.Token, authMethod string) (StorageLocation } // Check storage option - v, _ := profiles.GetOptionValue(options.AuthStorageOption) + v, err := getAuthStorageOption() + if err != nil { + return location, fmt.Errorf("%s: %w", credentialsErrorPrefix, err) + } - if strings.EqualFold(v, string(config.StorageTypeNone)) { + if v == string(config.StorageTypeNone) { return location, nil } @@ -194,7 +204,7 @@ func SaveTokenForMethod(token *oauth2.Token, authMethod string) (StorageLocation 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,8 +234,11 @@ 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, fmt.Errorf("%s: %w", credentialsErrorPrefix, err) + } + if v == string(config.StorageTypeNone) { return nil, ErrTokenStorageDisabled } @@ -369,10 +382,10 @@ 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) { +// Returns StorageLocationType indicating what was cleared +func ClearToken(authMethod string) (customtypes.StorageLocationType, error) { var errList []error - location := StorageLocation{} + var location customtypes.StorageLocationType // Clear from keychain storage, err := getTokenStorage(authMethod) @@ -383,7 +396,7 @@ func ClearToken(authMethod string) (StorageLocation, error) { Err: err, }) } else { - location.Keychain = true + location = customtypes.StorageLocationKeychain } } @@ -393,8 +406,8 @@ func ClearToken(authMethod string) (StorageLocation, error) { Prefix: credentialsErrorPrefix, Err: err, }) - } else { - location.File = true + } else if location == "" { + location = customtypes.StorageLocationFile } return location, errors.Join(errList...) @@ -554,12 +567,12 @@ func GetDeviceCodeConfiguration() (*config.Configuration, error) { cfg = cfg.WithStorageType(getStorageType()).WithStorageName("pingcli") // Provide optional suffix so SDK keychain entries align with file names - profileName, _ := profiles.GetOptionValue(options.RootActiveProfileOption) - if strings.TrimSpace(profileName) == "" { + profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil || strings.TrimSpace(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 = customtypes.ENUM_AUTH_PROVIDER_PINGONE } cfg = cfg.WithStorageOptionalSuffix(fmt.Sprintf("_%s_%s_%s", providerName, string(svcOAuth2.GrantTypeDeviceCode), profileName)) @@ -787,12 +800,12 @@ func GetAuthorizationCodeConfiguration() (*config.Configuration, error) { WithStorageName("pingcli") // Provide optional suffix so SDK keychain entries align with file names - profileName, _ := profiles.GetOptionValue(options.RootActiveProfileOption) - if strings.TrimSpace(profileName) == "" { + profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil || strings.TrimSpace(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 = customtypes.ENUM_AUTH_PROVIDER_PINGONE } cfg = cfg.WithStorageOptionalSuffix(fmt.Sprintf("_%s_%s_%s", providerName, string(svcOAuth2.GrantTypeAuthorizationCode), profileName)) @@ -975,12 +988,12 @@ func GetClientCredentialsConfiguration() (*config.Configuration, error) { WithStorageName("pingcli") // Provide optional suffix so SDK keychain entries align with file names - profileName, _ := profiles.GetOptionValue(options.RootActiveProfileOption) - if strings.TrimSpace(profileName) == "" { + profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil || strings.TrimSpace(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 = customtypes.ENUM_AUTH_PROVIDER_PINGONE } cfg = cfg.WithStorageOptionalSuffix(fmt.Sprintf("_%s_%s_%s", providerName, string(svcOAuth2.GrantTypeClientCredentials), profileName)) @@ -1056,12 +1069,12 @@ func GetWorkerConfiguration() (*config.Configuration, error) { WithStorageName("pingcli") // Provide optional suffix so SDK keychain entries align with file names - profileName, _ := profiles.GetOptionValue(options.RootActiveProfileOption) - if strings.TrimSpace(profileName) == "" { + profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil || strings.TrimSpace(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 = customtypes.ENUM_AUTH_PROVIDER_PINGONE } cfg = cfg.WithStorageOptionalSuffix(fmt.Sprintf("_%s_%s_%s", providerName, string(svcOAuth2.GrantTypeClientCredentials), profileName)) @@ -1131,12 +1144,12 @@ 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) - 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/credentials_storage_fallback_test.go b/internal/commands/auth/credentials_storage_fallback_test.go index 2683ddeb..44f6648a 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,10 +85,10 @@ func TestSaveTokenForMethod_FallsBackToFileWhenKeychainSaveFails(t *testing.T) { if err != nil { t.Fatalf("expected no error, got %v", err) } - if location.Keychain { + if location == customtypes.StorageLocationKeychain { t.Fatalf("expected Keychain=false, got true") } - if !location.File { + if location != customtypes.StorageLocationFile { t.Fatalf("expected File=true, got false") } @@ -137,10 +138,10 @@ func TestSaveTokenForMethod_UsesKeychainWhenAvailable(t *testing.T) { mockStorage.AssertExpectations(t) - if !location.Keychain { + if location != customtypes.StorageLocationKeychain { t.Fatalf("expected Keychain=true") } - if location.File { + if location == customtypes.StorageLocationFile { t.Fatalf("expected File=false") } diff --git a/internal/commands/auth/credentials_test.go b/internal/commands/auth/credentials_test.go index e4e436b4..36d5077f 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" @@ -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..3ad4c147 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" @@ -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..5034486f 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 @@ -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") { @@ -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") { @@ -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/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" +) From 81d7bc32e85cca4f1543b089cfc0a81f64b630fd Mon Sep 17 00:00:00 2001 From: wesleymccollam Date: Tue, 10 Feb 2026 17:34:25 -0500 Subject: [PATCH 2/4] remove login code duplication --- internal/commands/auth/credentials.go | 281 +++++--------------------- 1 file changed, 52 insertions(+), 229 deletions(-) diff --git a/internal/commands/auth/credentials.go b/internal/commands/auth/credentials.go index 47b835c3..7c8c71f9 100644 --- a/internal/commands/auth/credentials.go +++ b/internal/commands/auth/credentials.go @@ -413,16 +413,8 @@ func ClearToken(authMethod string) (customtypes.StorageLocationType, error) { return location, 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, defaultTokenKey string) (*LoginResult, error) { // Get profile name for token key generation profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) if err != nil { @@ -435,16 +427,14 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { 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 + tokenKey = defaultTokenKey } // Check if we have a valid cached token before calling TokenSource @@ -473,17 +463,35 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { if !shouldUseKeychain() { 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 { + 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 @@ -495,12 +503,6 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { } } } - 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() @@ -513,7 +515,13 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { // 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) + err = clearAllTokenFilesForGrantType(providerName, string(grantType), profileName) + if err != nil { + return nil, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: fmt.Errorf("failed to clean up old token files: %w", err), + } + } // Save token using our own storage logic (handles both file and keychain based on flags) location, err := SaveTokenForMethod(token, tokenKey) @@ -529,7 +537,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, @@ -537,6 +544,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, deviceCodeTokenKey) +} + // GetDeviceCodeConfiguration builds a device code authentication configuration from the CLI profile options func GetDeviceCodeConfiguration() (*config.Configuration, error) { cfg := config.NewConfiguration() @@ -619,118 +639,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, authorizationCodeTokenKey) } // GetAuthorizationCodeConfiguration builds an authorization code authentication configuration from the CLI profile options @@ -852,93 +761,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, clientCredentialsTokenKey) } // GetClientCredentialsConfiguration builds a client credentials authentication configuration from the CLI profile options From e09a03b390c1ce09c39a968c604782c7e6f11957 Mon Sep 17 00:00:00 2001 From: wesleymccollam Date: Wed, 11 Feb 2026 12:45:43 -0500 Subject: [PATCH 3/4] Revised error handling, ClearToken method cleanup, updated test checks --- internal/commands/auth/credentials.go | 144 +++++++++++------- .../auth/credentials_storage_fallback_test.go | 8 +- internal/commands/auth/credentials_test.go | 2 +- internal/commands/auth/logout_internal.go | 4 +- internal/commands/auth/use_keychain_test.go | 12 +- internal/constants/constants.go | 2 + 6 files changed, 102 insertions(+), 70 deletions(-) diff --git a/internal/commands/auth/credentials.go b/internal/commands/auth/credentials.go index 7c8c71f9..93ce2421 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" @@ -43,14 +44,17 @@ 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 "", err + return "", &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } } return strings.TrimSpace(strings.ToLower(v)), nil @@ -58,20 +62,20 @@ func getAuthStorageOption() (string, error) { // 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 { +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 } } @@ -79,11 +83,7 @@ func shouldUseKeychain() bool { // SDK handles keychain storage, pingcli handles file storage separately func getStorageType() config.StorageType { s, err := getAuthStorageOption() - if err != nil { - return config.StorageTypeSecureLocal - } - - if s == string(config.StorageTypeSecureLocal) || s == "" { + if err != nil || s == string(config.StorageTypeSecureLocal) || s == "" { return config.StorageTypeSecureLocal } // For file_system/none/secure_remote, avoid SDK persistence (pingcli manages file persistence) @@ -191,7 +191,10 @@ func SaveTokenForMethod(token *oauth2.Token, authMethod string) (customtypes.Sto // Check storage option v, err := getAuthStorageOption() if err != nil { - return location, fmt.Errorf("%s: %w", credentialsErrorPrefix, err) + return location, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } } if v == string(config.StorageTypeNone) { @@ -200,7 +203,11 @@ func SaveTokenForMethod(token *oauth2.Token, authMethod string) (customtypes.Sto // 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 { @@ -236,14 +243,21 @@ func LoadTokenForMethod(authMethod string) (*oauth2.Token, error) { // Check if storage is disabled v, err := getAuthStorageOption() if err != nil { - return nil, fmt.Errorf("%s: %w", credentialsErrorPrefix, err) + 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) } @@ -284,7 +298,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 != "" { @@ -338,7 +356,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{ @@ -366,7 +388,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) } @@ -382,10 +404,8 @@ func ClearAllTokens() error { // ClearToken removes the cached token for a specific authentication method // Clears from both keychain and file storage -// Returns StorageLocationType indicating what was cleared -func ClearToken(authMethod string) (customtypes.StorageLocationType, error) { +func ClearToken(authMethod string) error { var errList []error - var location customtypes.StorageLocationType // Clear from keychain storage, err := getTokenStorage(authMethod) @@ -395,8 +415,6 @@ func ClearToken(authMethod string) (customtypes.StorageLocationType, error) { Prefix: credentialsErrorPrefix, Err: err, }) - } else { - location = customtypes.StorageLocationKeychain } } @@ -406,11 +424,9 @@ func ClearToken(authMethod string) (customtypes.StorageLocationType, error) { Prefix: credentialsErrorPrefix, Err: err, }) - } else if location == "" { - location = customtypes.StorageLocationFile } - return location, errors.Join(errList...) + return errors.Join(errList...) } // performLogin consolidates common authentication logic for different grant types @@ -418,7 +434,7 @@ func performLogin(ctx context.Context, cfg *config.Configuration, grantType svcO // 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 @@ -433,7 +449,7 @@ func performLogin(ctx context.Context, cfg *config.Configuration, grantType svcO // Use SDK-consistent token key generation to avoid mismatches tokenKey, err := GetAuthMethodKeyFromConfig(cfg) if err != nil || tokenKey == "" { - // Fallback to simple key if generation fails + // Fallback to default key if generation fails, though this shouldn't happen with valid config tokenKey = defaultTokenKey } @@ -442,8 +458,12 @@ func performLogin(ctx context.Context, cfg *config.Configuration, grantType svcO 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 @@ -451,19 +471,14 @@ func performLogin(ctx context.Context, cfg *config.Configuration, grantType svcO } } - // 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 !useKeychain { if existingToken, err := loadTokenFromFile(tokenKey); err == nil && existingToken != nil && existingToken.RefreshToken != "" { endpoints, eerr := cfg.AuthEndpoints() if eerr == nil { + grantType := *cfg.Auth.GrantType var oauthCfg *oauth2.Config switch grantType { case svcOAuth2.GrantTypeDeviceCode: @@ -584,12 +599,15 @@ 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, err := profiles.GetOptionValue(options.RootActiveProfileOption) - if err != nil || strings.TrimSpace(profileName) == "" { - profileName = "default" + if err != nil { + return nil, &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} + } + if strings.TrimSpace(profileName) == "" { + profileName = constants.DefaultProfileName } providerName, err := profiles.GetOptionValue(options.AuthProviderOption) if err != nil || strings.TrimSpace(providerName) == "" { @@ -701,17 +719,20 @@ 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, err := profiles.GetOptionValue(options.RootActiveProfileOption) - if err != nil || strings.TrimSpace(profileName) == "" { - profileName = "default" + if err != nil { + return nil, &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} + } + if strings.TrimSpace(profileName) == "" { + profileName = constants.DefaultProfileName } providerName, err := profiles.GetOptionValue(options.AuthProviderOption) if err != nil || strings.TrimSpace(providerName) == "" { @@ -803,17 +824,20 @@ 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, err := profiles.GetOptionValue(options.RootActiveProfileOption) - if err != nil || strings.TrimSpace(profileName) == "" { - profileName = "default" + if err != nil { + return nil, &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} + } + if strings.TrimSpace(profileName) == "" { + profileName = constants.DefaultProfileName } providerName, err := profiles.GetOptionValue(options.AuthProviderOption) if err != nil || strings.TrimSpace(providerName) == "" { @@ -885,16 +909,19 @@ 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, err := profiles.GetOptionValue(options.RootActiveProfileOption) - if err != nil || strings.TrimSpace(profileName) == "" { - profileName = "default" + if err != nil { + return nil, &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} + } + if strings.TrimSpace(profileName) == "" { + profileName = constants.DefaultProfileName } providerName, err := profiles.GetOptionValue(options.AuthProviderOption) if err != nil || strings.TrimSpace(providerName) == "" { @@ -968,12 +995,15 @@ func GetAuthMethodKeyFromConfig(cfg *config.Configuration) (string, error) { // Build suffix to disambiguate across provider/grant/profile for both keychain and files profileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) - if err != nil || profileName == "" { - profileName = "default" + if err != nil { + return "", &errs.PingCLIError{Prefix: loginInteractiveErrorPrefix, Err: err} + } + if profileName == "" { + profileName = constants.DefaultProfileName } providerName, err := profiles.GetOptionValue(options.AuthProviderOption) if err != nil || 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 44f6648a..ff362f07 100644 --- a/internal/commands/auth/credentials_storage_fallback_test.go +++ b/internal/commands/auth/credentials_storage_fallback_test.go @@ -86,10 +86,10 @@ func TestSaveTokenForMethod_FallsBackToFileWhenKeychainSaveFails(t *testing.T) { t.Fatalf("expected no error, got %v", err) } if location == customtypes.StorageLocationKeychain { - t.Fatalf("expected Keychain=false, got true") + t.Fatalf("expected storage location to not be %s", customtypes.StorageLocationKeychain) } if location != customtypes.StorageLocationFile { - t.Fatalf("expected File=true, got false") + t.Fatalf("expected storage location %s, got %s", customtypes.StorageLocationFile, location) } // Verify it actually wrote the expected file under HOME @@ -139,10 +139,10 @@ func TestSaveTokenForMethod_UsesKeychainWhenAvailable(t *testing.T) { mockStorage.AssertExpectations(t) if location != customtypes.StorageLocationKeychain { - t.Fatalf("expected Keychain=true") + t.Fatalf("expected storage location %s, got %s", customtypes.StorageLocationKeychain, location) } if location == customtypes.StorageLocationFile { - t.Fatalf("expected File=false") + 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 36d5077f..09c48749 100644 --- a/internal/commands/auth/credentials_test.go +++ b/internal/commands/auth/credentials_test.go @@ -265,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 { diff --git a/internal/commands/auth/logout_internal.go b/internal/commands/auth/logout_internal.go index 3ad4c147..e4beefac 100644 --- a/internal/commands/auth/logout_internal.go +++ b/internal/commands/auth/logout_internal.go @@ -86,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) diff --git a/internal/commands/auth/use_keychain_test.go b/internal/commands/auth/use_keychain_test.go index 5034486f..453b58df 100644 --- a/internal/commands/auth/use_keychain_test.go +++ b/internal/commands/auth/use_keychain_test.go @@ -73,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 @@ -175,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) @@ -215,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 @@ -271,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 @@ -287,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) } @@ -335,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) 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" ) From 1d2e8a875435fc32d45f31cd2a637629b66fff36 Mon Sep 17 00:00:00 2001 From: wesleymccollam Date: Thu, 12 Feb 2026 11:28:48 -0500 Subject: [PATCH 4/4] Remove unneeded default token keys, Improved handling for GetOptionValue errors, add missing fallback for file storage checking, comment lines corrected --- internal/commands/auth/credentials.go | 83 +++++++++++++++++++-------- 1 file changed, 60 insertions(+), 23 deletions(-) diff --git a/internal/commands/auth/credentials.go b/internal/commands/auth/credentials.go index 93ce2421..b6bc67e7 100644 --- a/internal/commands/auth/credentials.go +++ b/internal/commands/auth/credentials.go @@ -19,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" @@ -164,7 +157,7 @@ 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 StorageLocationType indicating where the token was saved func SaveTokenForMethod(token *oauth2.Token, authMethod string) (customtypes.StorageLocationType, error) { @@ -379,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 { @@ -430,7 +423,7 @@ func ClearToken(authMethod string) error { } // performLogin consolidates common authentication logic for different grant types -func performLogin(ctx context.Context, cfg *config.Configuration, grantType svcOAuth2.GrantType, defaultTokenKey string) (*LoginResult, error) { +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 { @@ -439,7 +432,13 @@ func performLogin(ctx context.Context, cfg *config.Configuration, grantType svcO // 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 } @@ -448,9 +447,11 @@ func performLogin(ctx context.Context, cfg *config.Configuration, grantType svcO // Use SDK-consistent token key generation to avoid mismatches tokenKey, err := GetAuthMethodKeyFromConfig(cfg) - if err != nil || tokenKey == "" { - // Fallback to default key if generation fails, though this shouldn't happen with valid config - tokenKey = defaultTokenKey + if err != nil { + return nil, &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } } // Check if we have a valid cached token before calling TokenSource @@ -471,6 +472,13 @@ func performLogin(ctx context.Context, cfg *config.Configuration, grantType svcO } } + // 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 @@ -529,7 +537,6 @@ func performLogin(ctx context.Context, cfg *config.Configuration, grantType svcO } // 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 err = clearAllTokenFilesForGrantType(providerName, string(grantType), profileName) if err != nil { return nil, &errs.PingCLIError{ @@ -569,7 +576,7 @@ func PerformDeviceCodeLogin(ctx context.Context) (*LoginResult, error) { } } - return performLogin(ctx, cfg, svcOAuth2.GrantTypeDeviceCode, deviceCodeTokenKey) + return performLogin(ctx, cfg, svcOAuth2.GrantTypeDeviceCode) } // GetDeviceCodeConfiguration builds a device code authentication configuration from the CLI profile options @@ -610,7 +617,13 @@ func GetDeviceCodeConfiguration() (*config.Configuration, error) { profileName = constants.DefaultProfileName } 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 } cfg = cfg.WithStorageOptionalSuffix(fmt.Sprintf("_%s_%s_%s", providerName, string(svcOAuth2.GrantTypeDeviceCode), profileName)) @@ -657,7 +670,7 @@ func PerformAuthorizationCodeLogin(ctx context.Context) (*LoginResult, error) { } } - return performLogin(ctx, cfg, svcOAuth2.GrantTypeAuthorizationCode, authorizationCodeTokenKey) + return performLogin(ctx, cfg, svcOAuth2.GrantTypeAuthorizationCode) } // GetAuthorizationCodeConfiguration builds an authorization code authentication configuration from the CLI profile options @@ -735,7 +748,13 @@ func GetAuthorizationCodeConfiguration() (*config.Configuration, error) { profileName = constants.DefaultProfileName } 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 } cfg = cfg.WithStorageOptionalSuffix(fmt.Sprintf("_%s_%s_%s", providerName, string(svcOAuth2.GrantTypeAuthorizationCode), profileName)) @@ -782,7 +801,7 @@ func PerformClientCredentialsLogin(ctx context.Context) (*LoginResult, error) { } } - return performLogin(ctx, cfg, svcOAuth2.GrantTypeClientCredentials, clientCredentialsTokenKey) + return performLogin(ctx, cfg, svcOAuth2.GrantTypeClientCredentials) } // GetClientCredentialsConfiguration builds a client credentials authentication configuration from the CLI profile options @@ -840,7 +859,13 @@ func GetClientCredentialsConfiguration() (*config.Configuration, error) { profileName = constants.DefaultProfileName } 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 } cfg = cfg.WithStorageOptionalSuffix(fmt.Sprintf("_%s_%s_%s", providerName, string(svcOAuth2.GrantTypeClientCredentials), profileName)) @@ -924,7 +949,13 @@ func GetWorkerConfiguration() (*config.Configuration, error) { profileName = constants.DefaultProfileName } 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 } cfg = cfg.WithStorageOptionalSuffix(fmt.Sprintf("_%s_%s_%s", providerName, string(svcOAuth2.GrantTypeClientCredentials), profileName)) @@ -1002,7 +1033,13 @@ func GetAuthMethodKeyFromConfig(cfg *config.Configuration) (string, error) { profileName = constants.DefaultProfileName } providerName, err := profiles.GetOptionValue(options.AuthProviderOption) - if err != nil || strings.TrimSpace(providerName) == "" { + if err != nil { + return "", &errs.PingCLIError{ + Prefix: credentialsErrorPrefix, + Err: err, + } + } + if strings.TrimSpace(providerName) == "" { providerName = customtypes.ENUM_AUTH_PROVIDER_PINGONE } suffix := fmt.Sprintf("_%s_%s_%s", providerName, string(grantType), profileName)