fix(oidc): retry provider init and decouple status from provider state#219
Merged
Conversation
#214) OIDC silently disabled after container restarts when the identity provider was temporarily unreachable. Add retry loop (10 attempts, 5s intervals) and report oidcEnabled based on config rather than runtime provider pointer, matching autobrr's approach.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded retry/backoff to OIDC initialization; introduced Changes
Sequence Diagram(s)sequenceDiagram
participant Init as NewOIDC()
participant Disc as Discovery (.well-known)
participant ProviderLib as oidc.NewProvider
participant Logger as Logger
participant Backoff as Backoff/Sleep
Init->>Disc: attempt getProviderEndpoints()
alt discovery success
Disc-->>Init: endpoints
Init->>ProviderLib: oidc.NewProvider(endpoints)
alt provider init success
ProviderLib-->>Init: provider
Init->>Logger: configure verifier & scopes
Init-->>Init: return configured OIDC
else provider init fails
ProviderLib-->>Init: error
Init->>Logger: warn attempt N failed
Init->>Backoff: sleep(retryDelay)
Init->>Init: retry (until maxAttempts)
end
else discovery fails
Disc-->>Init: error
Init->>Logger: warn attempt N failed
Init->>Backoff: sleep(retryDelay)
Init->>Init: retry (until maxAttempts)
end
alt all attempts exhausted
Init->>Logger: error final failure
Init-->>Init: return error (attempts, last error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/auth_oidc.go (1)
28-42:⚠️ Potential issue | 🟡 MinorInconsistent redirect URL construction for error cases.
The nil guard redirects correctly use
baseURL+"login?error=...", but the PKCE and state generation error redirects use hardcoded"/?error=...". This would break navigation if the application is deployed with a non-root base URL.🐛 Proposed fix for consistent redirect URLs
// Generate state parameter state, err := utils.GenerateSecureToken(32) if err != nil { log.Error().Err(err).Msg("Failed to generate state parameter") - c.Redirect(http.StatusTemporaryRedirect, "/?error=state_generation_failed") + c.Redirect(http.StatusTemporaryRedirect, baseURL+"login?error=state_generation_failed") return } // Generate PKCE parameters pkceParams, err := auth.GeneratePKCEParams() if err != nil { log.Error().Err(err).Msg("Failed to generate PKCE parameters") - c.Redirect(http.StatusTemporaryRedirect, "/?error=pkce_generation_failed") + c.Redirect(http.StatusTemporaryRedirect, baseURL+"login?error=pkce_generation_failed") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/auth_oidc.go` around lines 28 - 42, The state and PKCE error handlers use hardcoded c.Redirect(..., "/?error=...") which is inconsistent with other nil-guards that use baseURL+"login?error=..."; update the error redirects in the utils.GenerateSecureToken and auth.GeneratePKCEParams failure branches to build the redirect using the same baseURL+"login?error=..." pattern (preserving the existing error query values) so navigation works when the app is hosted on a non-root base URL.
🧹 Nitpick comments (1)
internal/auth/oidc.go (1)
157-179: Consider respecting context cancellation during retry delays.The retry loop uses
time.Sleepwhich doesn't respect context cancellation. If the application is shutting down or the context is cancelled during initialization, the loop will continue sleeping until completion.♻️ Proposed fix to use context-aware sleep
for attempt := 1; attempt <= oidcInitMaxAttempts; attempt++ { endpoints, _, lastErr = getProviderEndpoints(ctx, http.DefaultClient, cfg.Issuer) if lastErr != nil { log.Warn().Err(lastErr).Int("attempt", attempt).Int("max", oidcInitMaxAttempts). Str("issuer", cfg.Issuer).Msg("Failed to discover OIDC provider endpoints") if attempt < oidcInitMaxAttempts { - time.Sleep(oidcInitRetryDelay) + select { + case <-ctx.Done(): + return nil, fmt.Errorf("OIDC initialization cancelled: %w", ctx.Err()) + case <-time.After(oidcInitRetryDelay): + } } continue } provider, lastErr = oidc.NewProvider(ctx, cfg.Issuer) if lastErr != nil { log.Warn().Err(lastErr).Int("attempt", attempt).Int("max", oidcInitMaxAttempts). Str("issuer", cfg.Issuer).Msg("Failed to initialize OIDC provider") if attempt < oidcInitMaxAttempts { - time.Sleep(oidcInitRetryDelay) + select { + case <-ctx.Done(): + return nil, fmt.Errorf("OIDC initialization cancelled: %w", ctx.Err()) + case <-time.After(oidcInitRetryDelay): + } } continue } break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/oidc.go` around lines 157 - 179, The retry loop in the oidc initialization (loop using oidcInitMaxAttempts and calling getProviderEndpoints and oidc.NewProvider) currently uses time.Sleep(oidcInitRetryDelay), which ignores context cancellation; change the retry delay to be context-aware by replacing time.Sleep with a select that waits on either ctx.Done() or a time.After(oidcInitRetryDelay) before retrying, and if ctx is cancelled return the last error immediately instead of continuing to the next attempt; ensure this behavior is applied in both retry branches where time.Sleep(oidcInitRetryDelay) is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/server/auth_oidc.go`:
- Around line 28-42: The state and PKCE error handlers use hardcoded
c.Redirect(..., "/?error=...") which is inconsistent with other nil-guards that
use baseURL+"login?error=..."; update the error redirects in the
utils.GenerateSecureToken and auth.GeneratePKCEParams failure branches to build
the redirect using the same baseURL+"login?error=..." pattern (preserving the
existing error query values) so navigation works when the app is hosted on a
non-root base URL.
---
Nitpick comments:
In `@internal/auth/oidc.go`:
- Around line 157-179: The retry loop in the oidc initialization (loop using
oidcInitMaxAttempts and calling getProviderEndpoints and oidc.NewProvider)
currently uses time.Sleep(oidcInitRetryDelay), which ignores context
cancellation; change the retry delay to be context-aware by replacing time.Sleep
with a select that waits on either ctx.Done() or a
time.After(oidcInitRetryDelay) before retrying, and if ctx is cancelled return
the last error immediately instead of continuing to the next attempt; ensure
this behavior is applied in both retry branches where
time.Sleep(oidcInitRetryDelay) is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0143c26-afff-4bd9-b9d4-6151cab64ce5
📒 Files selected for processing (4)
internal/auth/oidc.gointernal/server/auth.gointernal/server/auth_oidc.gointernal/server/server.go
Pre-existing hardcoded root redirects broke non-root base URL deployments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/server/auth_oidc.go (1)
18-25: Remove dead code: no-op conditional.The conditional on lines 20-22 does nothing—assigning
baseURL = ""when it's already empty is a no-op. Either remove it or, if a fallback was intended, implement it properly.🧹 Proposed fix
func loginErrorRedirectURL(baseURL, errorCode string) string { baseURL = strings.TrimRight(baseURL, "/") - if baseURL == "" { - baseURL = "" - } return baseURL + "/login?error=" + url.QueryEscape(errorCode) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/auth_oidc.go` around lines 18 - 25, The conditional in loginErrorRedirectURL that sets baseURL = "" when baseURL is already empty is dead code; remove the no-op if-block or replace it with a proper fallback behavior. Locate the function loginErrorRedirectURL and either delete the lines checking if baseURL == "" / assigning baseURL = "" so it simply trims and returns baseURL + "/login?error="+url.QueryEscape(errorCode), or, if a fallback host was intended, implement that fallback (e.g., set baseURL to a specific default value) and use that variable in the return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/auth_test.go`:
- Around line 105-146: Change the CheckRegistrationStatus response to use the
AuthHandler.oidcConfigured field rather than testing h.oidc != nil: return
h.oidcConfigured for the oidcEnabled JSON field inside CheckRegistrationStatus.
Then update the test TestCheckRegistrationStatusReportsOIDCProviderReadiness to
drive and assert on that configuration flag (pass the intended oidcConfigured
boolean into NewAuthHandler for each case and set expectedOID to that value)
instead of relying on tt.oidc presence.
---
Nitpick comments:
In `@internal/server/auth_oidc.go`:
- Around line 18-25: The conditional in loginErrorRedirectURL that sets baseURL
= "" when baseURL is already empty is dead code; remove the no-op if-block or
replace it with a proper fallback behavior. Locate the function
loginErrorRedirectURL and either delete the lines checking if baseURL == "" /
assigning baseURL = "" so it simply trims and returns baseURL +
"/login?error="+url.QueryEscape(errorCode), or, if a fallback host was intended,
implement that fallback (e.g., set baseURL to a specific default value) and use
that variable in the return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ad8e3ad-e31c-4515-bcf5-fa0a35c2ae66
📒 Files selected for processing (3)
internal/server/auth.gointernal/server/auth_oidc.gointernal/server/auth_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/server/auth.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/server/auth_oidc.go (1)
23-27: Optional: extract shared base URL normalization into a tiny helper.The duplicated
base_urlfallback logic is small but repeated; extracting it would make future adjustments safer.Also applies to: 65-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/auth_oidc.go` around lines 23 - 27, Extract the repeated base_url normalization into a small helper method on AuthHandler (e.g., BaseURL(c *gin.Context) string) and replace the duplicated logic in handleOIDCLogin and the corresponding callback handler (the other block at lines ~65-69) to call this helper; the helper should read c.GetString("base_url") and return "/" when empty so all callers use a single source of truth for base URL fallback.internal/server/auth_test.go (1)
157-187: Consider merging these two redirect tests into one table-driven test.Both cases share setup/assertions and differ only by route+handler, so a single table would reduce duplication and future drift.
As per coding guidelines, "
**/*_test.go: Prefer table-driven tests for new backend logic."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/auth_test.go` around lines 157 - 187, Merge the two nearly identical tests TestHandleOIDCLoginRedirectUsesLoginPathUnderSubpathBaseURL and TestHandleOIDCCallbackRedirectUsesLoginPathUnderSubpathBaseURL into a single table-driven test that iterates over the two routes/handlers; create a table with entries for request path ("/api/auth/oidc/login" and "/api/auth/oidc/callback") and a function pointer or identifier for the handler method (handler.handleOIDCLogin and handler.handleOIDCCallback), perform the shared setup (gin.TestMode, NewAuthHandler, recorder, context, set "base_url"), invoke the handler from the table entry, and assert the same redirect status and Location header for each case to remove duplication and follow the project test style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/server/auth_oidc.go`:
- Around line 23-27: Extract the repeated base_url normalization into a small
helper method on AuthHandler (e.g., BaseURL(c *gin.Context) string) and replace
the duplicated logic in handleOIDCLogin and the corresponding callback handler
(the other block at lines ~65-69) to call this helper; the helper should read
c.GetString("base_url") and return "/" when empty so all callers use a single
source of truth for base URL fallback.
In `@internal/server/auth_test.go`:
- Around line 157-187: Merge the two nearly identical tests
TestHandleOIDCLoginRedirectUsesLoginPathUnderSubpathBaseURL and
TestHandleOIDCCallbackRedirectUsesLoginPathUnderSubpathBaseURL into a single
table-driven test that iterates over the two routes/handlers; create a table
with entries for request path ("/api/auth/oidc/login" and
"/api/auth/oidc/callback") and a function pointer or identifier for the handler
method (handler.handleOIDCLogin and handler.handleOIDCCallback), perform the
shared setup (gin.TestMode, NewAuthHandler, recorder, context, set "base_url"),
invoke the handler from the table entry, and assert the same redirect status and
Location header for each case to remove duplication and follow the project test
style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64d068a9-9fd1-4625-a1d2-b687a3a03fae
📒 Files selected for processing (3)
internal/server/auth.gointernal/server/auth_oidc.gointernal/server/auth_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/server/auth.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/src/api/auth.ts (1)
148-153: Remove the unusedregistrationEnabledfield from theRegistrationStatusinterface.The server endpoint does not return
registrationEnabled, sodata.registrationEnabled ?? falsealways evaluates tofalse. Additionally, no UI component actually reads this field—all consumers only accesshasUsers,oidcConfigured, andoidcReady. Removing this field will eliminate dead code and reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/api/auth.ts` around lines 148 - 153, Remove the unused registrationEnabled field from the RegistrationStatus type and from the object returned by the function in web/src/api/auth.ts: delete the registrationEnabled property from the RegistrationStatus interface (or type) and stop mapping data.registrationEnabled in the return value (leave hasUsers, oidcConfigured, oidcReady only). Also search for any direct references to RegistrationStatus.registrationEnabled and remove or update them so types and callers compile cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/api/auth.ts`:
- Around line 148-153: Remove the unused registrationEnabled field from the
RegistrationStatus type and from the object returned by the function in
web/src/api/auth.ts: delete the registrationEnabled property from the
RegistrationStatus interface (or type) and stop mapping data.registrationEnabled
in the return value (leave hasUsers, oidcConfigured, oidcReady only). Also
search for any direct references to RegistrationStatus.registrationEnabled and
remove or update them so types and callers compile cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9523a9a3-d795-4683-a090-6e7f43e906f4
📒 Files selected for processing (6)
internal/server/auth.gointernal/server/auth_test.goweb/src/api/auth.tsweb/src/components/auth/Login.tsxweb/src/context/auth.tsxweb/src/routes.tsx
✅ Files skipped from review due to trivial changes (1)
- internal/server/auth_test.go
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
1 participant
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.
OIDC silently disables itself when the container restarts while the identity provider is temporarily unreachable —
NewOIDCfails once and the server continues withh.oidc = nil, causing/api/auth/statusto reportoidcEnabled: falsepermanently until manual restart. This adds a retry loop (10 attempts, 5s intervals) to OIDC provider initialization and decouples theoidcEnabledresponse from runtime provider state by tracking whether OIDC was configured, matching autobrr's approach.OIDC routes are now registered whenever an issuer is configured rather than only when the provider initialized successfully, with nil guards in the handlers returning a graceful
oidc_unavailableredirect if the provider isn't ready.Closes #214
Summary by CodeRabbit
New Features
Bug Fixes
Tests