Support auth_code, client_credentials, device_code authentication #143
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Detect cached vs new tokens by comparing AccessToken before/after SDK call in PerformDeviceCodeLogin, PerformAuthCodeLogin, PerformClientCredentialsLogin - Add GetAuthMethodKeyFromConfig() to extract grant type from Configuration instead of reading from config file (fixes incorrect auth method detection) - Fix device_code nil pointer by always calling WithDeviceCodeScopes() - Alphabetize PingOne authentication flags in platform export command - Update test assertions and clean up unused parameters Login commands now correctly show "Already authenticated" for cached tokens and "Successfully logged in" only for new authentication.
- Use Cobra flag validation (MarkFlagsMutuallyExclusive/MarkFlagsOneRequired) - Consolidate duplicate switch statements and applyRegionConfiguration functions - Validate auth methods against SDK grant type enums - Use errors.Join() in ClearToken() - Add error handling in GetAuthMethodKey() - Remove worker type support and unused test helpers - Clean up commented code blocks and fix linting issues All three OAuth2 methods tested and working with proper cache detection.
…authentication Add comprehensive OAuth2 authentication support with three methods: - Authorization code flow (auth_code) - now default when no flag specified - Client credentials flow (client_credentials) - Device code flow (device_code) Changes: - Add auth method flags to login command: --auth-code/-a, --client-credentials/-c, --device-code/-d - Make auth_code the default authentication method (no flag required) - Add auth method flags to logout command for targeted credential clearing - Implement keychain storage per auth method using SDK's GenerateKeychainAccountName - Replace custom token key hashing with SDK's built-in implementation - Add comprehensive integration tests for all three authentication flows - Update unit tests with table-driven approach for better coverage - Simplify token verification in tests (SDK handles keychain storage internally)
…code-authentication
- Token validation: Check cached tokens before re-authenticating (all 3 auth methods) - Dual storage: System keychain (primary) + ~/.pingcli/credentials/ (fallback) - --use-keychain flag (default: true) with PINGCLI_AUTH_USE_KEYCHAIN env var - Per-method token storage using hash-based keys to prevent conflicts - Logout clears both keychain and file storage - Updated messages to reflect "storage" instead of "Keychain" * Update 18 auth tests to handle automatic authentication behavior * Fix platform export tests to use correct static error definitions * Add comprehensive test coverage (auth_services_test.go, workflow tests) * Resolve all linting violations (err113, errorlint, nlreturn, gocritic, staticcheck) * Use newly implemented Errors package Enables: pingcli login --auth_code | --client_credentials | --device_code All 83 tests passing (50 command + 33 auth). UAT verified.
Implements dual storage strategy for OAuth2 tokens to ensure reliability across all environments (SSH, containers, CI/CD, desktop). Token Storage: - Primary: OS credential stores (Keychain/Credential Manager/Secret Service) - Secondary: File storage at ~/.pingcli/credentials/*.json - Tokens saved to BOTH locations on authentication - One file per auth method (device-code-token.json, auth-code-token.json, etc.) New --use-keychain Flag: - Controls token retrieval preference (default: true with fallback) - Added to: auth login, platform export, request commands - --use-keychain=true: Keychain only (fails if unavailable) - Default behavior: Try keychain first, fallback to file automatically Commands Updated: - platform export: Added --use-keychain and PingOne auth flags - request: Added --use-keychain and PingOne auth flags Testing: - New test-auth Makefile target for sequential auth test execution - Prevents parallel test resource conflicts - Refactored list_keys_test.go to table-driven pattern Documentation: - Updated README.md with improved installation instructions - Added comprehensive dual storage documentation - Updated auth docs (login.md, logout.md) with storage behavior - Removed redundant overview.md Code Cleanup: - Removed deprecated legacy worker token caching - Removed unused generateLegacyWorkerTokenKey function - Removed unused crypto/sha256 import - Removed local go.mod replacement line
cmd/auth/login.go
Outdated
| var token *oauth2.Token | ||
| var newAuth bool | ||
|
|
||
| switch selectedMethod { |
Contributor
There was a problem hiding this comment.
This block below the if !flagProvided block seems to duplicate the logic in performLoginByConfiguredType, we should avoid the repetition
Contributor
Author
There was a problem hiding this comment.
this logic has been moved to login_internal
| pwd, err := os.Getwd() | ||
| if err != nil { | ||
| return resolvedOutputDir, &errs.PingCLIError{Prefix: exportErrorPrefix, Err: fmt.Errorf("%w: %w", ErrGetPresentWorkingDirectory, err)} | ||
| return "", &errs.PingCLIError{Prefix: exportErrorPrefix, Err: fmt.Errorf("%w: %w", ErrGetPresentWorkingDirectory, err)} |
Contributor
There was a problem hiding this comment.
Why are these returns changed?
Contributor
Author
There was a problem hiding this comment.
this should be resolved
| } | ||
|
|
||
| func initRegionCodeOption() { | ||
| func initPingOneAuthenticationWorkerClientIDOption() { |
Contributor
There was a problem hiding this comment.
If we consider these worker options legacy (based on comments in other files), we should somehow mark them as deprecated here and inform people what the proper args to use are.
Contributor
Author
There was a problem hiding this comment.
added deprecation instructions to usage info
| ) | ||
|
|
||
| // AuthServices represents a list of authentication service names (pingone, pingfederate) | ||
| type AuthServices []string |
Contributor
There was a problem hiding this comment.
It doesn't seem like these are actually used in login logic - it always uses pingone. I'd leave this code out until we need it in the future, or at least remove "pingfederate" for this commit because selecting it doesn't actually do anything.
Contributor
Author
There was a problem hiding this comment.
these are removed
| case options.PINGONE_REGION_CODE: | ||
| switch typedValue := vValue.(type) { | ||
| case *customtypes.PingOneRegionCode: | ||
| case *customtypes.String: |
Contributor
There was a problem hiding this comment.
Why is this changed?
Contributor
Author
There was a problem hiding this comment.
this has been reverted
- refactor redirectURI to include path and port for auth_code flow, update tests - revert auth-specific parallel tests, run all tests in parallel - replace --use-keychain flag with --file-storage flag, which disables keychain usage - remove unused ClearPingOneClientCache method - correct login_interactive file name
|
|
||
| if useKeychain == "" { | ||
| // If not set, default to true | ||
| if useFileStorage == "" { |
Contributor
There was a problem hiding this comment.
We should use ParseBool to check the value of the --file-storage flag, similar to how other commands do this (for example see the overwrite export option
Contributor
Author
There was a problem hiding this comment.
this should be resolved
|
|
||
| storage := getTokenStorage(authMethod) | ||
|
|
||
| // Try keychain storage first |
Contributor
There was a problem hiding this comment.
Does pingcli need to do this if the storage method is keychain? I would expect the CLI to handle keychain storage for us, pingcli only needs to worry about the fallback to file storage.
Contributor
Author
There was a problem hiding this comment.
pingcli should be using all SDK methods for keychain usage
| func SaveToken(token *oauth2.Token) (StorageLocation, error) { | ||
| return SaveTokenForMethod(token, deviceCodeTokenKey) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
The err in LoadToken below is being ignored when not nil
Contributor
Author
There was a problem hiding this comment.
this should be resolved
* clear any previous token keychain entries or files if previous use of the same grant type was found on login and logout * use new WithStorageName method * update package name call for top level domain * correct context in internal auth method * use GetOptionValue methods to retrieve auth method values * remove redundant login and logout logic based on auth type * return missing configuration error, do not ask user if they'd like to reconfigure * remove unreliable isMissingConfigError, return authentication failed error instead * correct some errors to use PingCLI error type * update logout output message to include where token storage method was removed * correct login and logout documentation * improve error handling and logic in credentials.go * correct comment lines * remove unused pingfederate and pingone files * remove unneeded legacy methods * remove old test-auth references from Makefile * use ParseBool for file storage option * remove redundant check for cached token * require redirecturi port when using device code flow * store profile name and auth method in json file name, adding ability to clean up and remove past used token files. Added test coverage for this as well.
…code, use one environment id for all grant types
…remove unused clearAllTokenFiles function
…t for worker applications
…ction Add token expiry tracking to distinguish between new authentication and cached token reuse. Improve user messaging to clearly indicate when tokens are being refreshed automatically vs. using existing valid tokens.
…in the config, user will receive interactive login experience
… invalid authentication type" after interactive login is complete
… doc, add success output to request DELETE method
…v id when using platform export, correct file storage flag behavior
…v id when using platform export, correct file storage flag behavior, support storage name prefix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Description
authorization_code,client_credentials, anddevice_code~/.pingcli/credentials/)--use-keychainflag tologinandexportcommands for storage controlChange Characteristics
Checklist
All full (or complete) PRs that need review prior to merge should have the following box checked.
If contributing a partial or incomplete change (expecting the development team to complete the remaining work) please leave the box unchecked
Required SDK Upgrades
https://github.com/pingidentity/pingone-go-clientv0.3.0Testing
This PR has been tested with:
Shell Command(s)
Testing Results
Expand Results
End-to-end Tests Workflow Links