Add support for OIDC refresh token flow#130
Merged
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds refresh-token persistence and silent refresh flows to OIDC auth, extends default scopes with "offline_access", exposes RefreshCachedToken API, introduces CLI auth commands (status, refresh), and adds comprehensive tests for token exchange, caching, and refresh behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client/CLI
participant Auth as OIDCAuth
participant Cache as Token Cache
participant OIDC as OIDC Provider
CLI->>Auth: request token or run refresh
Auth->>Cache: LoadTokenCache()
alt cached token valid
Auth-->>CLI: return cached access_token
else token near expiry or expired
Auth->>Cache: read refresh_token
alt refresh_token present
Auth->>OIDC: POST /token (grant_type=refresh_token)
OIDC-->>Auth: tokenResponse (access_token, id_token, refresh_token)
Auth->>Cache: saveTokenCache(access_token, refresh_token)
Auth-->>CLI: return refreshed access_token
else no refresh_token
Auth-->>CLI: trigger full auth flow or return error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (3)
cmd/caib/auth/oidc.go (3)
290-317: Usedefer cancel()to prevent context leaks in shutdown helpers.Both
handleAuthCodeandshutdownAndReturncallcancel()synchronously afterserver.Shutdown(). IfShutdown()ever returns early (e.g., via panic-recovery in tests),cancel()is never called and the derived context leaks until GC.deferis the idiomatic fix.♻️ Proposed fix
func (a *OIDCAuth) handleAuthCode(ctx context.Context, server *http.Server, tokenEndpoint, code, redirectURI, codeVerifier string) (string, error) { shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() _ = server.Shutdown(shutdownCtx) - cancel() // ... } func (a *OIDCAuth) shutdownAndReturn(server *http.Server, err error) (string, error) { shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() _ = server.Shutdown(shutdownCtx) - cancel() return "", err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/auth/oidc.go` around lines 290 - 317, The context cancel functions in handleAuthCode and shutdownAndReturn are called after server.Shutdown which can leak the derived context if Shutdown returns early; change both to call defer cancel() immediately after creating shutdownCtx, e.g., in handleAuthCode where shutdownCtx, cancel := context.WithTimeout(...) and in shutdownAndReturn where shutdownCtx, cancel := context.WithTimeout(...), so the cancel is deferred and always executed regardless of how server.Shutdown behaves.
326-360: Per-callhttp.Transportallocation prevents TLS session reuse.
doTokenRequestallocates a newhttp.Transportandhttp.Clienton every invocation. For a CLI this is low-impact (at most 2–3 calls per auth flow), but it prevents TLS session resumption. Consider hoisting the client to anOIDCAuthfield or a package-level lazy initializer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/auth/oidc.go` around lines 326 - 360, doTokenRequest creates a new http.Transport and http.Client on every call which prevents TLS session reuse; add a reusable client/transport to OIDCAuth (e.g., a field like httpClient *http.Client or transport *http.Transport) and initialize it once (constructor, lazy init with sync.Once, or a package-level initializer) honoring the insecureSkipTLS flag when building the transport; then have doTokenRequest use that shared httpClient (retain per-request context via NewRequestWithContext and keep the same Timeout/Transport) so TLS sessions can be resumed across requests.
434-450: Document thatLoadTokenCacheskips issuer validation.The private
loadTokenCache(line 463) validates that the cached issuer matches the configured one, preventing cross-provider token reuse. The exportedLoadTokenCacheomits this check intentionally (it's used for display), but the doc comment should make this explicit so future callers don't assume the same guard is in place.📝 Proposed doc update
-// LoadTokenCache reads the token cache from disk. Returns nil if no cache exists. +// LoadTokenCache reads the token cache from disk without issuer validation. +// It is intended for informational/display purposes only. For authenticated flows +// use the internal loadTokenCache which enforces issuer matching. func LoadTokenCache() (*TokenCache, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/auth/oidc.go` around lines 434 - 450, Update the doc comment for the exported function LoadTokenCache to explicitly state that it intentionally skips issuer validation and only reads/parses the stored token cache for display/inspection; mention that the unexported loadTokenCache performs issuer validation to prevent cross-provider reuse, so callers requiring safety should use that behavior instead of LoadTokenCache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/auth/config.go`:
- Around line 96-100: The code currently hardcodes "offline_access" into the
default OIDC scopes in the OIDCConfig returned (see OIDCConfig, Scopes,
issuerURL, ClientID), which can cause invalid_scope errors for some providers;
change this by making inclusion of offline_access configurable (e.g., add a
boolean field like IncludeOfflineAccess or accept explicit Scopes from config
and only append "offline_access" when enabled) and update the code that
constructs OIDCConfig to respect that setting; optionally add a fallback in the
auth flow to detect an invalid_scope error and retry the request without
"offline_access" so legacy providers keep working.
In `@cmd/caib/authcmd/authcmd.go`:
- Around line 147-149: When RefreshCachedToken returns an empty token with no
error the user gets no output; update the logic in authcmd.go (around the call
to RefreshCachedToken and the existing fmt.Println("Access token refreshed
successfully.") guarded by if token != "") to handle the else case: if token ==
"" and err == nil print a clear fallback message (e.g., "No access token
returned.") so the user is informed, while keeping the existing error handling
path unchanged.
- Around line 93-103: The code currently assumes an "exp" claim exists and
computes expTime/remaining unconditionally; if "exp" is missing this yields a
zero time and a large negative duration displayed as "EXPIRED". Change the logic
around the claims["exp"] extraction: if the type-assertion ok is false, do not
compute remaining or call printTokenStatus; instead print a clear message like
"Token expiry: none (no exp claim)" or similar. Keep the existing behavior when
exp is present (use time.Unix, formatDuration, printTokenStatus) and reference
the variables/functions expTime, claims, printTokenStatus, and formatDuration
when making the change.
---
Nitpick comments:
In `@cmd/caib/auth/oidc.go`:
- Around line 290-317: The context cancel functions in handleAuthCode and
shutdownAndReturn are called after server.Shutdown which can leak the derived
context if Shutdown returns early; change both to call defer cancel()
immediately after creating shutdownCtx, e.g., in handleAuthCode where
shutdownCtx, cancel := context.WithTimeout(...) and in shutdownAndReturn where
shutdownCtx, cancel := context.WithTimeout(...), so the cancel is deferred and
always executed regardless of how server.Shutdown behaves.
- Around line 326-360: doTokenRequest creates a new http.Transport and
http.Client on every call which prevents TLS session reuse; add a reusable
client/transport to OIDCAuth (e.g., a field like httpClient *http.Client or
transport *http.Transport) and initialize it once (constructor, lazy init with
sync.Once, or a package-level initializer) honoring the insecureSkipTLS flag
when building the transport; then have doTokenRequest use that shared httpClient
(retain per-request context via NewRequestWithContext and keep the same
Timeout/Transport) so TLS sessions can be resumed across requests.
- Around line 434-450: Update the doc comment for the exported function
LoadTokenCache to explicitly state that it intentionally skips issuer validation
and only reads/parses the stored token cache for display/inspection; mention
that the unexported loadTokenCache performs issuer validation to prevent
cross-provider reuse, so callers requiring safety should use that behavior
instead of LoadTokenCache.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/caib/auth/config.gocmd/caib/auth/oidc.gocmd/caib/auth/wrapper.gocmd/caib/authcmd/authcmd.gocmd/caib/main.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/auth/oidc_test.go`:
- Around line 27-34: The linter flags makeValidTestJWT's iss parameter as always
identical; update makeValidTestJWT to remove the iss argument and hardcode a
test issuer constant (e.g., the same "https://issuer.example.com" used across
tests) or reference a shared test constant, then update all call sites
(including wrapper_test.go and any other tests) to call makeValidTestJWT with
only the expiresIn argument; ensure makeExpiredTestJWT still accepts a variable
issuer if needed or uses the same shared constant so signatures remain
consistent.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/caib/auth/config_test.gocmd/caib/auth/oidc_test.gocmd/caib/auth/wrapper_test.gocmd/caib/authcmd/authcmd_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/caib/auth/oidc_test.go (1)
21-21: Avoid swallowingjson.Marshalerrors in test setup helpers.Ignoring marshal errors at Line 21, Line 570, and Line 620 makes failures harder to diagnose and can hide bad fixture input.
♻️ Suggested fix
- payload, _ := json.Marshal(claims) + payload, err := json.Marshal(claims) + if err != nil { + panic("failed to marshal JWT claims: " + err.Error()) + }- data, _ := json.Marshal(cache) + data, err := json.Marshal(cache) + Expect(err).NotTo(HaveOccurred()) Expect(os.WriteFile(cachePath, data, 0600)).To(Succeed())- data, _ := json.Marshal(cache) + data, err := json.Marshal(cache) + Expect(err).NotTo(HaveOccurred()) Expect(os.WriteFile(cachePath, data, 0600)).To(Succeed())Also applies to: 570-571, 620-621
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/auth/oidc_test.go` at line 21, The test helpers currently ignore errors from json.Marshal when encoding claims (payload, _ := json.Marshal(claims)) in oidc_test.go (occurrences around lines 21, 570, 620); change each to capture the error (payload, err := json.Marshal(claims)) and handle it by failing the test immediately (e.g., using t.Fatalf/t.Fatalf-like helper or require.NoError) so bad fixture input or marshal failures surface instead of being swallowed. Ensure you update all three occurrences and keep the variable name payload and claims usage the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/auth/oidc_test.go`:
- Around line 111-113: The tests set originalHome := os.Getenv("HOME") and then
os.Setenv("HOME", tempDir) but restore HOME only when originalHome != "", which
leaves a stale HOME when it was originally unset; update every cleanup/restore
(references to originalHome in oidc_test.go) to conditionally call
os.Unsetenv("HOME") if originalHome == "" and os.Setenv("HOME", originalHome)
otherwise (i.e., replace the current restore code paths in the defer/AfterEach
blocks that reference originalHome with this robust restore logic).
In `@cmd/caib/auth/wrapper_test.go`:
- Line 116: The test currently ignores json.Marshal errors at the two sites
where you call data, _ := json.Marshal(cache) in wrapper_test.go; replace the
discard with explicit error checking using the test expectation helpers (e.g.,
capture as data, err := json.Marshal(cache) and assert err is nil with
Expect(err).To be consistent with the suite, update both occurrences (around the
variable name cache in the setup/helper blocks) to use Expect(err).To avoid
writing empty cache files on marshal failure, ensure the test fails immediately
when json.Marshal returns an error.
- Around line 68-72: In the AfterEach block restore the HOME environment
correctly: instead of only calling os.Setenv when originalHome != "" leave HOME
set to the tempDir for tests that originally had HOME unset. Use the
originalHome variable to decide: if originalHome == "" call os.Unsetenv("HOME"),
otherwise call os.Setenv("HOME", originalHome); then continue to remove tempDir.
Apply this change inside the AfterEach closure referencing originalHome,
tempDir, os.Setenv and os.Unsetenv.
---
Nitpick comments:
In `@cmd/caib/auth/oidc_test.go`:
- Line 21: The test helpers currently ignore errors from json.Marshal when
encoding claims (payload, _ := json.Marshal(claims)) in oidc_test.go
(occurrences around lines 21, 570, 620); change each to capture the error
(payload, err := json.Marshal(claims)) and handle it by failing the test
immediately (e.g., using t.Fatalf/t.Fatalf-like helper or require.NoError) so
bad fixture input or marshal failures surface instead of being swallowed. Ensure
you update all three occurrences and keep the variable name payload and claims
usage the same.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/caib/auth/config_test.gocmd/caib/auth/oidc_test.gocmd/caib/auth/wrapper_test.gocmd/caib/authcmd/authcmd.gocmd/caib/authcmd/authcmd_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/caib/authcmd/authcmd_test.go
- cmd/caib/auth/config_test.go
- cmd/caib/authcmd/authcmd.go
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmd/caib/auth/oidc_test.go (1)
124-127:⚠️ Potential issue | 🟡 MinorRestore
HOMEcorrectly when it was originally unset.These cleanup blocks only restore when
originalHome != "". If HOME was unset before the test, state leaks to later specs.🔧 Suggested pattern
- var originalHome string + var originalHome string + var hadHome bool - originalHome = os.Getenv("HOME") + originalHome, hadHome = os.LookupEnv("HOME") Expect(os.Setenv("HOME", tempDir)).To(Succeed()) - if originalHome != "" { + if hadHome { _ = os.Setenv("HOME", originalHome) + } else { + _ = os.Unsetenv("HOME") }Also applies to: 213-216, 391-394, 507-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/auth/oidc_test.go` around lines 124 - 127, The AfterEach cleanup currently restores HOME only when originalHome != "" which leaks state if HOME was originally unset; change the capture to record presence (use os.LookupEnv) into e.g. originalHome and originalHomeSet, and in each AfterEach (the blocks referencing originalHome) restore by setting HOME back when originalHomeSet is true and calling os.Unsetenv("HOME") when originalHomeSet is false; update all affected AfterEach blocks (the ones around originalHome at lines noted) to use this presence-aware restore logic and use os.Unsetenv instead of skipping restoration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/caib/auth/oidc.go`:
- Around line 434-435: The docstring for LoadTokenCache claims it "Returns nil
if no cache exists" but the implementation returns read errors directly; update
LoadTokenCache to match the comment by checking the os.ReadFile error and if
os.IsNotExist(err) return (nil, nil), otherwise return the error; keep JSON
unmarshalling behavior (return error on decode failure) and ensure the function
signature and TokenCache type are unchanged so callers still receive
(*TokenCache, error).
- Around line 126-138: The code currently requires parsing the access token
inside saveTokenCache and fails cache persistence for opaque tokens; change
saveTokenCache to accept an expiresIn int (e.g., saveTokenCache(token string,
refreshToken string, expiresIn int)), update both call sites that call
saveTokenCache (the one using tokenResp.AccessToken/IDToken and the other near
line ~305) to pass tokenResp.ExpiresIn, and modify saveTokenCache to prefer the
provided expiresIn for computing expiry, only attempt JWT parsing for exp when
expiresIn is zero, and do not treat JWT parse failures as fatal—log and continue
to persist the cache using expiresIn or a safe default instead.
---
Duplicate comments:
In `@cmd/caib/auth/oidc_test.go`:
- Around line 124-127: The AfterEach cleanup currently restores HOME only when
originalHome != "" which leaks state if HOME was originally unset; change the
capture to record presence (use os.LookupEnv) into e.g. originalHome and
originalHomeSet, and in each AfterEach (the blocks referencing originalHome)
restore by setting HOME back when originalHomeSet is true and calling
os.Unsetenv("HOME") when originalHomeSet is false; update all affected AfterEach
blocks (the ones around originalHome at lines noted) to use this presence-aware
restore logic and use os.Unsetenv instead of skipping restoration.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/caib/auth/config.gocmd/caib/auth/config_test.gocmd/caib/auth/oidc.gocmd/caib/auth/oidc_test.gocmd/caib/auth/wrapper.gocmd/caib/auth/wrapper_test.gocmd/caib/authcmd/authcmd.gocmd/caib/authcmd/authcmd_test.gocmd/caib/main.go
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/caib/auth/config_test.go
- cmd/caib/auth/wrapper.go
- cmd/caib/authcmd/authcmd.go
- cmd/caib/main.go
- cmd/caib/authcmd/authcmd_test.go
- cmd/caib/auth/wrapper_test.go
- cmd/caib/auth/config.go
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
bennyz
approved these changes
Feb 25, 2026
This was referenced Mar 4, 2026
10 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Issue #92
Summary by CodeRabbit
New Features
Tests