[MOB-4187] Implement proactive token refresh to prevent 401 errors#1047
[MOB-4187] Implement proactive token refresh to prevent 401 errors#1047
401 errors#1047Conversation
There was a problem hiding this comment.
Pull request overview
Implements proactive access-token refresh via Auth0’s CredentialsManager so API calls use fresh tokens and avoid “refresh-on-401” behavior, updating the UI integration and tests accordingly.
Changes:
- Add
getFreshAccessToken()toEcosiaAuthenticationServiceand extendAuthErrorwith retrieval/not-logged-in errors. - Update
EcosiaAuthUIStateProviderto fetch a fresh token before callingregisterVisit. - Remove reactive 401 retry/renew logic from
AccountsServiceand adjust/add unit tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| firefox-ios/EcosiaTests/Core/Accounts/AccountsServiceTests.swift | Updates 401/unauthorized expectations in tests. |
| firefox-ios/EcosiaTests/Account/Auth/AuthTests.swift | Adds unit tests for getFreshAccessToken(). |
| firefox-ios/Ecosia/UI/Account/EcosiaAuthUIStateProvider.swift | Injects auth service + uses fresh token for registerVisit. |
| firefox-ios/Ecosia/Core/Accounts/Service/AccountsService.swift | Simplifies registerVisit by removing 401 retry/renew flow. |
| firefox-ios/Ecosia/Account/Auth/EcosiaAuthenticationService.swift | Introduces getFreshAccessToken() using Auth0 credential retrieval. |
| firefox-ios/Ecosia/Account/Auth/AuthError.swift | Adds credentialsRetrievalFailed and notLoggedIn error cases. |
| public init(accountsProvider: AccountsProviderProtocol, authenticationService: EcosiaAuthenticationService = .shared) { | ||
| self.accountsProvider = accountsProvider | ||
| self.authenticationService = authenticationService | ||
|
|
||
| // Initialize state synchronously to prevent flickering | ||
| self.isLoggedIn = EcosiaAuthenticationService.shared.isLoggedIn | ||
| self.userProfile = EcosiaAuthenticationService.shared.userProfile | ||
| self.isLoggedIn = authenticationService.isLoggedIn | ||
| self.userProfile = authenticationService.userProfile |
There was a problem hiding this comment.
Now that EcosiaAuthUIStateProvider supports injecting an authenticationService, the class should use that dependency consistently. There are still code paths (e.g. user profile updates) that read from EcosiaAuthenticationService.shared, which makes the new initializer injection ineffective in tests and can desync state if a non-shared service is passed. Please replace remaining ...shared reads in this type with the authenticationService property.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| func testGetFreshAccessToken_withExpiredToken_refreshesAutomatically() async throws { | ||
| // Arrange - Auth0's CredentialsManager automatically refreshes expired tokens | ||
| let freshToken = "refreshed-access-token" | ||
| let credentials = Credentials( | ||
| accessToken: freshToken, | ||
| tokenType: "Bearer", | ||
| idToken: "test-id-token", | ||
| refreshToken: "test-refresh-token", | ||
| expiresIn: Date().addingTimeInterval(3600), | ||
| scope: "openid profile email" | ||
| ) |
There was a problem hiding this comment.
testGetFreshAccessToken_withExpiredToken_refreshesAutomatically doesn’t currently set up an expired token scenario or assert that a refresh path was taken (it just verifies whatever retrieveCredentials() returns). Consider renaming the test to reflect what’s actually being validated, or adjusting the mock/setup to simulate an expired credential + refresh to make the test match its intent.
There was a problem hiding this comment.
@copilot that's a fair point. I would adjust the mock/setup to match the test's real purpose
| let credentials = try await auth0Provider.retrieveCredentials() | ||
| // Update cached tokens with fresh values | ||
| setupTokensWithCredentials(credentials, settingLoggedInStateTo: true) | ||
|
|
||
| let accessToken = credentials.accessToken | ||
| guard !accessToken.isEmpty else { | ||
| EcosiaLogger.auth.error("Retrieved credentials do not contain a valid access token") | ||
| throw AuthError.notLoggedIn |
There was a problem hiding this comment.
getFreshAccessToken() calls setupTokensWithCredentials(..., settingLoggedInStateTo: true), which (a) dispatches an auth state change as .userLoggedIn every time and can trigger observers (e.g. EcosiaAuthUIStateProvider) to re-run login-only side effects like registerVisitIfNeeded(), potentially causing repeated calls/loops; and (b) sets isLoggedIn = true before validating that credentials.accessToken is non-empty. Consider updating cached tokens without broadcasting a login event (or only dispatching when isLoggedIn actually changes), and only marking logged-in after the access token is validated.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
lucaschifino
left a comment
There was a problem hiding this comment.
All good from my side, a couple minor comments
I see there are some linting errors to be fixed too
Previously, the app would wait for 401 responses before refreshing expired tokens, causing excessive authentication errors and poor user experience. This change implements a proactive approach where tokens are automatically refreshed before API calls, leveraging Auth0's CredentialsManager which handles token expiry detection and refresh seamlessly. Changes: - Add getFreshAccessToken() method to EcosiaAuthenticationService that retrieves fresh tokens from Auth0 - Update EcosiaAuthUIStateProvider to use fresh tokens for API calls - Simplify AccountsService by removing reactive 401 retry logic - Add new error cases to AuthError enum for better error handling - Add comprehensive tests for the new token refresh behavior - Update AccountsServiceTests to reflect that 401s now indicate genuine auth failures This approach eliminates unnecessary 401 errors and provides a more reliable authentication experience. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…teProvider` (#1050) * Initial plan * Replace EcosiaAuthenticationService.shared with injected authenticationService in handleUserProfileUpdate Co-authored-by: d4r1091 <3584008+d4r1091@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: d4r1091 <3584008+d4r1091@users.noreply.github.com>
#1053) * Initial plan * Remove performVisitRequest and inline body into registerVisit Co-authored-by: d4r1091 <3584008+d4r1091@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: d4r1091 <3584008+d4r1091@users.noreply.github.com>
…ials` protocol (#1054) * Initial plan * Clarify retrieveCredentials protocol wraps CredentialsManager.credentials() in doc comment Co-authored-by: d4r1091 <3584008+d4r1091@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: d4r1091 <3584008+d4r1091@users.noreply.github.com>
… dispatch auth events on actual transitions (#1052) * Initial plan * Fix getFreshAccessToken: validate token before updating state; only dispatch on state transitions Co-authored-by: d4r1091 <3584008+d4r1091@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: d4r1091 <3584008+d4r1091@users.noreply.github.com>
…o actually simulate expiry and refresh (#1051) * Initial plan * Adjust mock/setup for expired token refresh test Co-authored-by: d4r1091 <3584008+d4r1091@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: d4r1091 <3584008+d4r1091@users.noreply.github.com>
4726ac9 to
c2ab192
Compare
|
I had to apply some changes to the following implementation when realising that the SSO exchanges were failing with |
|
|
||
| ## How It Works | ||
|
|
||
| 1. Calls `auth0Provider.retrieveCredentials()` (protocol implementation backed by Auth0's `CredentialsManager.credentials()`) which checks token expiry |
There was a problem hiding this comment.
@copilot can we refernce the protocoll here, is there something written in a public RFC or on a website about this Auth0 standard
There was a problem hiding this comment.
Thanks for the addition @falkorichter!
I double-checked the documentation @copilot suggested. I guess it adds something generic and somewhat confusing. What can be relevant here is this doc. @copilot update your existing PR mentionig solely this doc
|
@falkorichter I've opened a new pull request, #1055, to work on those changes. Once the pull request is ready, I'll request review from you. |
97067fc to
3ac8d7f
Compare
[MOB-4187
Context
Previously, the app would wait for 401 responses before refreshing expired tokens, causing excessive authentication errors and poor user experience.
This change implements a proactive approach where tokens are automatically refreshed before API calls, leveraging Auth0's CredentialsManager which handles token expiry detection and refresh seamlessly.
Approach
getFreshAccessToken()method toEcosiaAuthenticationServicethat retrieves fresh tokens from Auth0EcosiaAuthUIStateProviderto use fresh tokens for API callsAccountsServiceby removing reactive 401 retry logicAuthErrorenum for better error handlingAccountsServiceTeststo reflect that 401s now indicate genuine auth failuresThis approach eliminates unnecessary 401 errors and provides a more reliable authentication experience.
Other
Before merging
Checklist