fix: allow logins for token refresh failures due to unstable connections#1383
Open
nooreldeenmansour wants to merge 1 commit intomainfrom
Open
fix: allow logins for token refresh failures due to unstable connections#1383nooreldeenmansour wants to merge 1 commit intomainfrom
nooreldeenmansour wants to merge 1 commit intomainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses intermittent unlock/login failures when an OIDC token refresh times out on unstable connections by allowing password-based authentication to fall back to cached/offline authentication (unless provider authentication is explicitly forced), matching the behavior users see when fully offline.
Changes:
- Add fallback-to-cached authentication when password auth token refresh fails with a non-protocol (intended: network) error and provider auth isn’t forced.
- Add/adjust
TestIsAuthenticatedcases to cover the fallback path and the forced-provider-auth timeout path. - Add golden outputs and generic cached token/password fixtures for the new test scenarios.
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| authd-oidc-brokers/internal/broker/broker.go | Implements fallback to cached authentication on token refresh failures intended to represent unstable network conditions. |
| authd-oidc-brokers/internal/broker/broker_test.go | Updates TestIsAuthenticated table to add fallback scenario and keep a forced-provider-auth timeout denial scenario. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_falls_back_to_cached_on_token_refresh_network_error/first_call | Golden output for the new “fallback to cached” scenario. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_falls_back_to_cached_on_token_refresh_network_error/data/provider_url/test-user@email.com/token.json | Generic token fixture for the new test scenario. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_falls_back_to_cached_on_token_refresh_network_error/data/provider_url/test-user@email.com/password | Generic password fixture for the new test scenario. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Error_when_mode_is_password_and_token_refresh_times_out_with_forced_provider_auth/first_call | Golden output for forced-provider-auth refresh timeout denial. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Error_when_mode_is_password_and_token_refresh_times_out_with_forced_provider_auth/data/provider_url/test-user@email.com/token.json | Generic token fixture for forced-provider-auth timeout scenario. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Error_when_mode_is_password_and_token_refresh_times_out_with_forced_provider_auth/data/provider_url/test-user@email.com/password | Generic password fixture for forced-provider-auth timeout scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1383 +/- ##
==========================================
- Coverage 86.32% 80.18% -6.15%
==========================================
Files 99 20 -79
Lines 6690 989 -5701
Branches 111 0 -111
==========================================
- Hits 5775 793 -4982
+ Misses 859 196 -663
+ Partials 56 0 -56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nooreldeenmansour
force-pushed
the
allow-logins-unstable-connections
branch
from
754b580 to
fd7fc34
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...refresh_times_out_with_forced_provider_auth/data/provider_url/test-user@email.com/token.json
Show resolved
Hide resolved
...n_refresh_times_out_with_forced_provider_auth/data/provider_url/test-user@email.com/password
Show resolved
Hide resolved
nooreldeenmansour
force-pushed
the
allow-logins-unstable-connections
branch
from
fd7fc34 to
28a859b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nooreldeenmansour
force-pushed
the
allow-logins-unstable-connections
branch
3 times, most recently
from
bc864f8 to
36dff11
Compare
nooreldeenmansour
force-pushed
the
allow-logins-unstable-connections
branch
from
36dff11 to
bc99294
Compare
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
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.
Closes #998
UDENG-9464