Skip to content

refactor items #157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wesleymccollam merged 6 commits into from
Feb 19, 2026
Merged

refactor items #157

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cb3a115
reduce AuthStorageOption duplication, refactor the StorageLocation to…
wesleymccollam Feb 10, 2026
81d7bc3
remove login code duplication
wesleymccollam Feb 10, 2026
e09a03b
Revised error handling, ClearToken method cleanup, updated test checks
wesleymccollam Feb 11, 2026
1d2e8a8
Remove unneeded default token keys, Improved handling for GetOptionVa…
wesleymccollam Feb 12, 2026
caa7786
add changelog entry
wesleymccollam Feb 18, 2026
e3de180
revert golangci-lint files
wesleymccollam Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 59 additions & 46 deletions internal/commands/auth/credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -68,26 +78,23 @@ 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
}
// 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) {
Expand Down Expand Up @@ -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
Expand All @@ -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
}

Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
}
Expand All @@ -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
}

Expand Down Expand Up @@ -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
Comment thread
henryrecker-pingidentity marked this conversation as resolved.
Outdated
func ClearToken(authMethod string) (customtypes.StorageLocationType, error) {
var errList []error
location := StorageLocation{}
var location customtypes.StorageLocationType

// Clear from keychain
storage, err := getTokenStorage(authMethod)
Expand All @@ -383,7 +396,7 @@ func ClearToken(authMethod string) (StorageLocation, error) {
Err: err,
})
} else {
location.Keychain = true
location = customtypes.StorageLocationKeychain
}
}

Expand All @@ -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...)
Expand Down Expand Up @@ -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) == "" {
Comment thread
henryrecker-pingidentity marked this conversation as resolved.
Outdated
providerName = customtypes.ENUM_AUTH_PROVIDER_PINGONE
}
cfg = cfg.WithStorageOptionalSuffix(fmt.Sprintf("_%s_%s_%s", providerName, string(svcOAuth2.GrantTypeDeviceCode), profileName))
Expand Down Expand Up @@ -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) == "" {
Comment thread
henryrecker-pingidentity marked this conversation as resolved.
Outdated
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))
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
Expand Down
9 changes: 5 additions & 4 deletions internal/commands/auth/credentials_storage_fallback_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
Comment thread
henryrecker-pingidentity marked this conversation as resolved.
Outdated
}
if !location.File {
if location != customtypes.StorageLocationFile {
t.Fatalf("expected File=true, got false")
}

Expand Down Expand Up @@ -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")
}

Expand Down
5 changes: 3 additions & 2 deletions internal/commands/auth/credentials_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
}

Expand Down
19 changes: 15 additions & 4 deletions internal/commands/auth/login_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
Expand Down
29 changes: 22 additions & 7 deletions internal/commands/auth/logout_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
Expand Down
Loading