feat: implement client registration caching for AWS SSO OIDC#4
Conversation
This change adds client registration caching to reduce AWS SSO OIDC API calls and improve compatibility with aws-sso-util and granted/assume tools. Key improvements: - Client registrations cached for 90 days in botocore-compatible format - Token cache format updated to match aws-sso-util exactly - URL normalization ensures compatibility across tools - Smart token lookup scans all cache files for cross-tool compatibility Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughToken and client-registration caching were expanded and aligned with botocore: start URLs are normalized, token and registration caches record received/expiry timestamps and region, token loading falls back to directory scanning, and client flow now loads cached registration or registers-and-caches a public client before device authorization. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Cache
participant SSO
Client->>Cache: LoadClientRegistration(region)
alt cached & valid
Cache-->>Client: ClientId, ClientSecret
else cache miss/invalid
Client->>SSO: Register public client
SSO-->>Client: ClientId, ClientSecret, ExpiresIn
Client->>Cache: SaveClientRegistration(region, clientId, clientSecret, expiresAt)
Cache-->>Client: Saved
end
Client->>SSO: StartDeviceAuthorization(clientId, clientSecret, startUrl)
SSO-->>Client: DeviceCode, UserCode, VerificationUri, ExpiresIn
Client->>SSO: Poll token endpoint (clientId, clientSecret, device_code)
SSO-->>Client: AccessToken, ExpiresIn
Client->>Cache: SaveTokenCache(accessToken, startUrl, region, expiresAt)
Cache-->>Client: Saved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sso/cache.go (1)
144-173: ClearTokenCache removes all JSON files including client registrations.The function removes all
.jsonfiles in the cache directory (line 164), which includes thebotocore-client-id-*.jsonclient registration files. This may be unintended if the goal is to only clear tokens while preserving client registrations.🐛 Proposed fix to preserve client registrations
for _, entry := range entries { if !entry.IsDir() && filepath.Ext(entry.Name()) == ".json" { + // Skip client registration files + if strings.HasPrefix(entry.Name(), "botocore-client-id-") { + continue + } cachePath := filepath.Join(cacheDir, entry.Name()) if err := os.Remove(cachePath); err != nil { return err } } }
🤖 Fix all issues with AI agents
In @internal/sso/client.go:
- Line 136: The code dereferences token.AccessToken when calling SaveTokenCache;
add a nil check for token and token.AccessToken before using it (e.g., in the
function where SaveTokenCache(*token.AccessToken, c.startURL, ...) is called),
and only call SaveTokenCache when token != nil && token.AccessToken != nil; if
either is nil, handle gracefully by returning an error or skipping the cache
write with an appropriate log message (reference the variable token and the
SaveTokenCache call to locate the change).
- Around line 135-138: Replace the hard-coded 8-hour expiry when caching the
token by using the actual lifetime from the AWS CreateTokenOutput (use the
ExpiresIn field) to compute the expiration time passed to SaveTokenCache; read
token.ExpiresIn (handle nil/zero safely by falling back to a sensible default),
convert seconds to a time.Duration (time.Second * int64(*token.ExpiresIn)) and
use time.Now().Add(...) instead of time.Now().Add(8*time.Hour) while keeping
SaveTokenCache(*token.AccessToken, c.startURL, c.region, expiry).
🧹 Nitpick comments (3)
internal/sso/cache.go (2)
55-73: Silently swallowing unmarshal errors may hide corrupted cache files.When
json.Unmarshalfails on lines 65-67, the error is silently ignored and the function falls through tofindTokenByStartURL. Consider logging a warning for corrupted cache files to aid debugging.♻️ Suggested improvement
data, err := os.ReadFile(path) if err == nil { var token TokenCache if err := json.Unmarshal(data, &token); err == nil { return &token, nil + } else { + log.Printf("⚠️ Warning: Failed to parse token cache %s: %v", path, err) } }
191-211: No validation that the loaded cache has required fields populated.If a cache file exists but has empty
ClientIdorClientSecret, the function returns them as-is. Consider validating that essential fields are non-empty.♻️ Suggested improvement
var cache ClientRegistrationCache if err := json.Unmarshal(data, &cache); err != nil { return nil, err } + // Validate essential fields + if cache.ClientId == "" || cache.ClientSecret == "" { + return nil, nil // Treat as missing cache + } + return &cache, nilinternal/sso/client.go (1)
67-68: Potential nil pointer dereference ifRegisterClientreturns nil pointers.While AWS SDK typically returns valid pointers on success, it's defensive to add nil checks before dereferencing
register.ClientIdandregister.ClientSecret.♻️ Suggested defensive check
+ if register.ClientId == nil || register.ClientSecret == nil { + return nil, fmt.Errorf("RegisterClient returned nil credentials") + } clientId = *register.ClientId clientSecret = *register.ClientSecret
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/sso/cache.gointernal/sso/client.go
🔇 Additional comments (9)
internal/sso/cache.go (7)
13-20: LGTM - TokenCache struct is well-designed for botocore compatibility.The struct includes all necessary fields for cross-tool compatibility with aws-sso-util and granted/assume.
22-28: LGTM - ClientRegistrationCache struct properly structured.Includes essential fields for client registration caching with proper JSON tags.
75-120: Scanning all cache files is O(n) but acceptable for typical cache sizes.The fallback scan in
findTokenByStartURLiterates through all JSON files in the cache directory. For typical use cases with a handful of SSO sessions, this is fine. The function properly skips directories, non-JSON files, and botocore client registration files.One minor observation: errors during file reading (line 104-105) and JSON parsing (line 109-110) are silently skipped, which is appropriate for a best-effort scan.
122-142: Potential nil pointer dereference ifaccessTokencomes from dereferencing a nil pointer upstream.The function accepts
accessTokenas a string, so this is safe at this level. However, verify that callers (in client.go) properly check for nil before dereferencingtoken.AccessToken.
175-189: LGTM - Client registration cache path follows botocore conventions.The path format
botocore-client-id-{region}.jsonaligns with the stated compatibility goal.
213-232: LGTM - SaveClientRegistration properly persists registration data.The function correctly uses 0600 permissions for the cache file, which is appropriate for files containing secrets.
30-35: No action needed. The implementation already correctly handles trailing/#sequences. aws-sso-util expects normalized URLs without trailing slashes (e.g.,https://d-1234567890.awsapps.com/start), and the current code achieves this by removing#first, then/, which properly handles cases likeurl/#→url.Likely an incorrect or invalid review comment.
internal/sso/client.go (2)
44-75: LGTM - Load-or-create pattern for client registration is well implemented.The logic correctly:
- Attempts to load cached registration
- Validates expiration before reuse
- Falls back to registering a new client
- Caches successful registrations with AWS-provided expiration
Good defensive handling with warnings instead of fatal errors for cache operations.
97-133: Polling loop handles context cancellation and timeouts appropriately.Good implementation with:
- Context cancellation check via select
- Maximum retry limit
- Progress feedback to user
- Using AWS-provided interval
internal/sso/client.go
Outdated
| } | ||
| if err := SaveTokenCache(cacheToken); err != nil { | ||
| // Cache the new token in botocore-compatible format | ||
| if err := SaveTokenCache(*token.AccessToken, c.startURL, c.region, time.Now().Add(8*time.Hour)); err != nil { |
There was a problem hiding this comment.
Missing nil check before dereferencing token.AccessToken.
If token.AccessToken is nil (unlikely but possible), this will panic. Add a defensive check.
🐛 Proposed fix
// Cache the new token in botocore-compatible format
+ if token.AccessToken == nil {
+ return nil, fmt.Errorf("CreateToken returned nil access token")
+ }
if err := SaveTokenCache(*token.AccessToken, c.startURL, c.region, time.Now().Add(8*time.Hour)); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := SaveTokenCache(*token.AccessToken, c.startURL, c.region, time.Now().Add(8*time.Hour)); err != nil { | |
| // Cache the new token in botocore-compatible format | |
| if token.AccessToken == nil { | |
| return nil, fmt.Errorf("CreateToken returned nil access token") | |
| } | |
| if err := SaveTokenCache(*token.AccessToken, c.startURL, c.region, time.Now().Add(8*time.Hour)); err != nil { |
🤖 Prompt for AI Agents
In @internal/sso/client.go at line 136, The code dereferences token.AccessToken
when calling SaveTokenCache; add a nil check for token and token.AccessToken
before using it (e.g., in the function where SaveTokenCache(*token.AccessToken,
c.startURL, ...) is called), and only call SaveTokenCache when token != nil &&
token.AccessToken != nil; if either is nil, handle gracefully by returning an
error or skipping the cache write with an appropriate log message (reference the
variable token and the SaveTokenCache call to locate the change).
- Use token.ExpiresIn from AWS response instead of hardcoded 8-hour expiration - Preserve client registration files (botocore-client-id-*.json) during cache clear since they remain valid for 90 days Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @b3nk3. * #4 (comment) The following files were modified: * `internal/sso/cache.go`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/sso/cache.go (3)
55-73: Verify the fallback behavior doesn't mask errors.The LoadTokenCache function uses a fallback scan when direct lookup fails. However, the current implementation silently ignores all errors from the direct lookup (line 63) and falls back even for genuine errors like permission issues or corrupted JSON.
Consider distinguishing between "file not found" (fallback appropriate) and other errors (should be reported).
♻️ Suggested improvement to distinguish error types
func LoadTokenCache(startURL string) (*TokenCache, error) { // First try direct path lookup with normalized URL path, err := getTokenCachePath(startURL) if err != nil { return nil, err } data, err := os.ReadFile(path) - if err == nil { + if err != nil && !os.IsNotExist(err) { + // Report non-NotExist errors (permissions, I/O errors, etc.) + return nil, err + } + if err == nil { var token TokenCache if err := json.Unmarshal(data, &token); err == nil { return &token, nil + } else { + // Log unmarshal errors but continue to fallback + log.Printf("⚠️ Warning: Failed to unmarshal token cache at %s: %v", path, err) } } // If direct lookup fails, scan cache files for matching startUrl // This handles tokens created by other tools with different URL formats return findTokenByStartURL(startURL) }
75-120: Consider validating loaded tokens before returning.The findTokenByStartURL function scans cache files and returns the first matching token without validating:
- Required fields are present (AccessToken, ExpiresAt)
- The token hasn't expired
This could return unusable tokens to the caller.
♻️ Proposed validation before returning token
// Check if this token's startUrl matches (after normalization) if token.StartUrl != "" && normalizeStartURL(token.StartUrl) == normalizedTarget { + // Validate token has required fields and isn't expired + if token.AccessToken != "" && !token.ExpiresAt.IsZero() && time.Now().Before(token.ExpiresAt) { + return &token, nil + } - return &token, nil } }
197-217: Consider returning expired registrations with a flag or error.LoadClientRegistration returns cached registrations without checking expiration. The caller (client.go line 55) performs this check, but having expiration validation in the load function would:
- Centralize the validation logic
- Prevent returning unusable registrations
- Make the API clearer
Current implementation works but could be more robust.
♻️ Optional: Add expiration check in LoadClientRegistration
func LoadClientRegistration(region string) (*ClientRegistrationCache, error) { path, err := getClientRegistrationCachePath(region) if err != nil { return nil, err } data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { return nil, nil } return nil, err } var cache ClientRegistrationCache if err := json.Unmarshal(data, &cache); err != nil { return nil, err } + // Return nil for expired registrations + if !cache.ExpiresAt.IsZero() && time.Now().After(cache.ExpiresAt) { + return nil, nil + } + return &cache, nil }internal/sso/client.go (1)
44-75: Add explicit zero-value check forExpiresAtto improve code clarity.Both the token cache check (line 37) and client registration cache check (line 55) use
time.Now().Before(ExpiresAt)without validating whetherExpiresAtis initialized. While this works correctly in practice—zero time values are treated as expired, triggering re-registration—the intent is implicit and relies on Go's time comparison semantics. Adding an explicitIsZero()check makes the code more defensive and self-documenting:Suggested fix
- if cachedToken != nil && time.Now().Before(cachedToken.ExpiresAt) { + if cachedToken != nil && !cachedToken.ExpiresAt.IsZero() && time.Now().Before(cachedToken.ExpiresAt) {- if cachedReg != nil && time.Now().Before(cachedReg.ExpiresAt) { + if cachedReg != nil && !cachedReg.ExpiresAt.IsZero() && time.Now().Before(cachedReg.ExpiresAt) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/sso/cache.gointernal/sso/client.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sso/client.go (1)
internal/sso/cache.go (3)
LoadClientRegistration(197-217)SaveClientRegistration(219-238)SaveTokenCache(122-142)
🔇 Additional comments (10)
internal/sso/cache.go (6)
13-20: LGTM - Botocore-compatible token cache structure.The TokenCache struct now aligns with aws-sso-util and granted/assume tools. The addition of StartUrl, Region, and ReceivedAt fields provides proper metadata for cross-tool compatibility.
22-28: LGTM - Client registration cache structure.The ClientRegistrationCache struct correctly stores client credentials with expiration metadata. The removal of StartUrl and Region makes sense since client registrations are region-scoped by the cache file naming.
30-35: LGTM - URL normalization for cross-tool compatibility.The normalizeStartURL function correctly strips trailing characters to match aws-sso-util format.
94-100: Good filtering logic for cache file scanning.The implementation correctly skips directories, non-JSON files, and botocore client registration files during token scanning.
164-171: LGTM - Preserving long-lived client registrations.The ClearTokenCache function correctly preserves botocore-client-id-* files since those registrations remain valid for 90 days. This prevents unnecessary re-registration API calls.
219-238: LGTM - Client registration caching.SaveClientRegistration correctly stores client credentials with expiration metadata in botocore-compatible format. File permissions (0600) appropriately protect sensitive client secrets.
internal/sso/client.go (4)
49-53: Good error handling for cache load failures.Logging a warning for cache load failures (line 52) is appropriate since the application can continue by registering a new client. This aligns with the optional nature of caching.
77-84: LGTM - Device authorization using cached credentials.The device authorization flow correctly uses the loaded or newly registered client credentials. This enables proper reuse of cached registrations.
135-140: The ExpiresIn assumption is correct.The AWS CreateToken API response returns ExpiresIn as an integer number of seconds until token expiration. The code correctly converts this to a time duration using
time.Duration(token.ExpiresIn) * time.Second.
70-74: Code correctly interprets ClientSecretExpiresAt as Unix seconds timestamp.The AWS SDK Go v2 RegisterClient response returns
ClientSecretExpiresAtas a Unix epoch timestamp in seconds (int64), so the conversion usingtime.Unix(register.ClientSecretExpiresAt, 0)is correct.
Docstrings generation was requested by @b3nk3. * #4 (comment) The following files were modified: * `internal/sso/cache.go` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/sso/cache.go:
- Around line 204-205: The doc comment for getClientRegistrationCachePath is
truncated and starts mid-sentence; replace it with a complete comment that
describes the function's purpose, parameters and return values (e.g., that it
builds/returns the path to the client registration cache for the given region
and may return an error if the user home directory cannot be determined or the
cache directory cannot be created) to match the style of other functions in the
file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sso/cache.go
🔇 Additional comments (9)
internal/sso/cache.go (9)
13-28: LGTM!The cache structs are well-defined with appropriate JSON field names matching botocore conventions. File permissions (0600/0700 used elsewhere) ensure credentials are protected.
30-35: LGTM!The normalization handles typical AWS SSO URL variations appropriately.
41-57: LGTM!Good implementation matching AWS CLI cache path conventions. SHA1 is appropriate here for cache key generation (non-cryptographic use), and 0700 directory permissions provide proper security.
68-86: LGTM!The two-phase lookup (direct path then directory scan) is a good approach for cross-tool compatibility. The fallback when unmarshal fails allows finding tokens created by other tools with different formats.
92-136: LGTM!Good implementation of the fallback token search. Properly skips client registration files and gracefully handles unreadable/unparseable cache entries.
141-161: LGTM!Proper implementation with normalized URL storage, secure file permissions (0600), and ReceivedAt timestamp for cache freshness tracking.
167-202: LGTM!Good implementation that correctly preserves client registration files while clearing token caches. The early return on error is acceptable for a cache-clearing operation.
223-243: LGTM!Correct handling of the file-not-found case. Note that the caller should verify
ExpiresAtbefore using the cached registration.
247-266: LGTM!Consistent implementation with SaveTokenCache. Proper file permissions and timestamp handling.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/sso/cache.go (1)
30-35: Consider handling URLs with both trailing characters.The current implementation may not fully normalize URLs ending with both "#" and "/" in reverse order (e.g.,
"url/#"or"url#/"). While AWS SSO start URLs typically don't have such patterns, consider usingstrings.TrimRight(startURL, "#/")for more robust normalization.♻️ Alternative implementation
func normalizeStartURL(startURL string) string { - startURL = strings.TrimSuffix(startURL, "#") - startURL = strings.TrimSuffix(startURL, "/") - return startURL + return strings.TrimRight(startURL, "#/") }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/sso/cache.go
🔇 Additional comments (8)
internal/sso/cache.go (8)
37-57: LGTM - Token cache path generation is correct.The function properly:
- Creates the cache directory with secure permissions (0700)
- Normalizes the start URL for consistent hashing
- Uses SHA-1 for filename generation (appropriate for cache key derivation)
- Handles errors appropriately
68-86: LGTM - Robust token loading with smart fallback.The implementation correctly:
- Attempts direct lookup using normalized URL hash
- Falls back to directory scanning if direct lookup fails (handles tokens created by other tools with different URL formats)
- Silently handles unmarshal errors in the direct path, treating them as cache misses
The fallback provides good cross-tool compatibility as intended by the PR objectives.
88-136: LGTM - Directory scanning logic is sound.The function correctly:
- Scans the cache directory for matching tokens
- Filters out non-token files (directories, non-JSON, botocore-client-id- files)
- Handles read/unmarshal errors defensively by continuing scan
- Validates non-empty StartUrl before comparing (Line 130)
- Normalizes both URLs for consistent matching
The silent error handling is appropriate for a cache scanning operation where partial corruption shouldn't block the entire operation.
138-161: LGTM - Token cache saving is correct.The function properly:
- Normalizes the start URL before storage (Line 148)
- Captures the current timestamp in ReceivedAt (Line 152)
- Uses secure file permissions (0600)
- Follows botocore-compatible format
Note: Like other file-based AWS caching tools, this implementation doesn't prevent concurrent write races. Multiple processes writing the same cache file simultaneously could theoretically conflict, but this is a common pattern in AWS tooling and acceptable for typical usage.
167-202: LGTM - Cache clearing logic is correct.The function correctly:
- Handles non-existent cache directory gracefully (Lines 176-178)
- Filters to JSON files only (Line 188)
- Preserves botocore client registration files (Lines 191-194)
- Propagates errors appropriately
The preservation of client registration files aligns with the PR objective of caching registrations for 90 days.
204-222: LGTM - Client registration cache path follows botocore conventions.The function correctly:
- Uses the botocore-compatible filename format
botocore-client-id-{region}.json- Creates the cache directory with secure permissions (0700)
- Organizes registrations per region
224-246: LGTM - Client registration loading is correct.The function correctly:
- Distinguishes between missing cache (returns
nil, nil) and read errors- Handles JSON unmarshaling with appropriate error propagation
- Follows the same pattern as
LoadTokenCache
248-269: LGTM - Client registration saving is correct.The function properly:
- Captures the current timestamp in ReceivedAt (Line 260)
- Uses secure file permissions (0600)
- Follows botocore-compatible format
- Matches the pattern established by
SaveTokenCache
- Add newline at end of cache.go - Fix normalizeStartURL to handle edge cases like /# using TrimRight - Add warning log when JSON unmarshal fails (possible cache corruption) - Use aws.ToString() for safer pointer dereferencing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/sso/client.go:
- Around line 135-140: The code dereferences token.AccessToken directly which
can panic if nil; change the block around expiresAt and SaveTokenCache to
defensively handle a nil AccessToken by using aws.ToString(token.AccessToken)
(or check token.AccessToken != nil) before calling SaveTokenCache, and only call
SaveTokenCache when the resulting string is non-empty; keep the expiresAt
calculation (time.Now().Add(time.Duration(token.ExpiresIn) * time.Second)) and
log a warning if the access token is missing instead of dereferencing a nil
pointer.
🧹 Nitpick comments (1)
internal/sso/cache.go (1)
226-248: Consider adding unmarshal warning log for consistency.The
LoadClientRegistrationfunction correctly handles file I/O and missing files. However, unlikeLoadTokenCache(line 79), it doesn't log a warning when JSON unmarshalling fails. The commit message mentions "add warning logs when JSON unmarshal fails to surface possible cache corruption."📋 Proposed addition of warning log
var cache ClientRegistrationCache if err := json.Unmarshal(data, &cache); err != nil { + log.Printf("⚠️ Warning: Failed to parse client registration cache %s (possible corruption): %v", path, err) return nil, err }This improves observability and aligns with the error handling pattern in
LoadTokenCache.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/sso/cache.gointernal/sso/client.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sso/client.go (2)
internal/config/config.go (1)
Config(32-35)internal/sso/cache.go (3)
LoadClientRegistration(228-248)SaveClientRegistration(252-271)SaveTokenCache(143-163)
🔇 Additional comments (12)
internal/sso/client.go (3)
77-84: LGTM! Device authorization correctly uses cached/registered client credentials.The device authorization flow now properly uses the
clientIdandclientSecretobtained from the cache or registration step, replacing the previous hard-coded values.
119-122: LGTM! Token polling correctly uses cached/registered client credentials.The token creation polling now properly uses the dynamically obtained
clientIdandclientSecret, maintaining consistency with the device authorization flow.
44-75: LGTM! Client registration caching implementation is solid.The load-or-register workflow correctly:
- Attempts to load cached registration first
- Checks expiration before reuse
- Registers a new client on cache miss or expiry
- Uses
aws.ToString()for safe pointer dereferencing- Caches registration with AWS-provided expiration
Error handling is appropriate: cache load failures are logged as warnings (non-fatal), while registration failures propagate as errors. The
ClientSecretExpiresAtfield from AWS RegisterClient API is always populated as a Unix epoch timestamp, making the expiration calculation at line 71 safe and reliable.internal/sso/cache.go (9)
14-29: LGTM! Cache types are well-structured and botocore-compatible.The
TokenCacheandClientRegistrationCachetypes correctly implement:
- Botocore-compatible field naming (camelCase JSON tags)
- Both
ExpiresAtandReceivedAttimestamps for proper expiry tracking and cache validation- All necessary fields for cross-tool compatibility with aws-sso-util and granted/assume
31-35: LGTM! URL normalization handles edge cases correctly.The
normalizeStartURLfunction correctly usesstrings.TrimRightto remove all trailing#and/characters, which properly handles edge cases like"/#","/#/","/##//", etc. This ensures consistent URL normalization across different tools.
37-57: LGTM! Token cache path generation is secure and consistent.The
getTokenCachePathfunction correctly:
- Creates the AWS standard cache directory (
~/.aws/sso/cache) with secure permissions (0700)- Normalizes the start URL before hashing to ensure consistency
- Uses SHA-1 for deterministic filename generation
- Follows botocore conventions
59-88: LGTM! Smart token loading with graceful fallback.The
LoadTokenCachefunction implements an intelligent two-tier approach:
- Fast path: Direct lookup using normalized URL hash
- Fallback: Directory scan for tokens created by other tools with different URL formats
The graceful error handling (logging unmarshal errors as warnings and continuing to fallback) ensures resilience against cache corruption while maintaining cross-tool compatibility.
90-138: LGTM! Directory scan handles cross-tool token discovery effectively.The
findTokenByStartURLfunction correctly:
- Filters out non-token files (directories, non-JSON, client registration files)
- Normalizes both the target URL and cached URLs for accurate comparison
- Silently skips unreadable or corrupted files (appropriate for a scan operation)
- Returns
(nil, nil)when no match is found, maintaining the same signature as direct lookupThis enables seamless token discovery across different AWS SSO tools.
140-163: LGTM! Token caching is secure and consistent.The
SaveTokenCachefunction correctly:
- Normalizes the start URL before storage (ensures consistency with loading)
- Populates all required fields including
ReceivedAttimestamp- Writes the cache file with secure permissions (0600, owner-only read/write)
- Creates botocore-compatible JSON structure
165-204: LGTM! Cache clearing preserves client registrations appropriately.The
ClearTokenCachefunction correctly:
- Checks for cache directory existence before clearing (line 178)
- Filters out directories and non-JSON files
- Preserves
botocore-client-id-*files (client registrations valid for 90 days)- Removes only token cache files
This design ensures that client registrations aren't unnecessarily invalidated during token cache clearing, reducing API calls to AWS SSO OIDC.
206-224: LGTM! Client registration path follows botocore conventions.The
getClientRegistrationCachePathfunction correctly:
- Uses the standard AWS cache directory with secure permissions (0700)
- Implements botocore-compatible naming:
botocore-client-id-{region}.json- Scopes registration by region (appropriate for AWS regional services)
250-271: LGTM! Client registration caching is secure and complete.The
SaveClientRegistrationfunction correctly:
- Populates all required fields including the
ReceivedAttimestamp- Marshals to botocore-compatible JSON structure
- Writes with secure file permissions (0600)
- Stores registration data by region for proper scoping
| // Cache the new token in botocore-compatible format | ||
| // Use ExpiresIn from AWS response (seconds until expiration) | ||
| expiresAt := time.Now().Add(time.Duration(token.ExpiresIn) * time.Second) | ||
| if err := SaveTokenCache(*token.AccessToken, c.startURL, c.region, expiresAt); err != nil { | ||
| log.Printf("⚠️ Warning: Failed to cache token: %v", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add defensive nil check for token.AccessToken.
The expiration calculation and caching logic are correct, but dereferencing *token.AccessToken at line 138 could panic if the pointer is nil. Consider using aws.ToString() for safer dereferencing.
🛡️ Proposed fix for safer pointer dereferencing
// Cache the new token in botocore-compatible format
// Use ExpiresIn from AWS response (seconds until expiration)
expiresAt := time.Now().Add(time.Duration(token.ExpiresIn) * time.Second)
-if err := SaveTokenCache(*token.AccessToken, c.startURL, c.region, expiresAt); err != nil {
+if err := SaveTokenCache(aws.ToString(token.AccessToken), c.startURL, c.region, expiresAt); err != nil {
log.Printf("⚠️ Warning: Failed to cache token: %v", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Cache the new token in botocore-compatible format | |
| // Use ExpiresIn from AWS response (seconds until expiration) | |
| expiresAt := time.Now().Add(time.Duration(token.ExpiresIn) * time.Second) | |
| if err := SaveTokenCache(*token.AccessToken, c.startURL, c.region, expiresAt); err != nil { | |
| log.Printf("⚠️ Warning: Failed to cache token: %v", err) | |
| } | |
| // Cache the new token in botocore-compatible format | |
| // Use ExpiresIn from AWS response (seconds until expiration) | |
| expiresAt := time.Now().Add(time.Duration(token.ExpiresIn) * time.Second) | |
| if err := SaveTokenCache(aws.ToString(token.AccessToken), c.startURL, c.region, expiresAt); err != nil { | |
| log.Printf("⚠️ Warning: Failed to cache token: %v", err) | |
| } |
🤖 Prompt for AI Agents
In @internal/sso/client.go around lines 135 - 140, The code dereferences
token.AccessToken directly which can panic if nil; change the block around
expiresAt and SaveTokenCache to defensively handle a nil AccessToken by using
aws.ToString(token.AccessToken) (or check token.AccessToken != nil) before
calling SaveTokenCache, and only call SaveTokenCache when the resulting string
is non-empty; keep the expiresAt calculation
(time.Now().Add(time.Duration(token.ExpiresIn) * time.Second)) and log a warning
if the access token is missing instead of dereferencing a nil pointer.
1 participant
This PR implements client registration caching to reduce AWS SSO OIDC API calls and improve compatibility with aws-sso-util and granted/assume tools.
Key improvements: Client registrations are cached for 90 days in botocore-compatible format, token cache format matches aws-sso-util exactly, URL normalization ensures compatibility across tools, and smart token lookup scans all cache files for cross-tool compatibility.
The caching mechanism stores client registrations in
~/.aws/sso/cache/botocore-client-id-{region}.jsonand tokens remain compatible with other AWS tooling.Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.