From b14bb6acc0c9403f4eba7f43d7c35b2038a36f41 Mon Sep 17 00:00:00 2001 From: "Dat H. Pham" Date: Mon, 9 Feb 2026 14:01:12 +0700 Subject: [PATCH] Update ADR for OIDC token refresh --- core/lib/utils/app_logger.dart | 6 ++- docs/adr/0031-fix-refresh-token-with-oidc.md | 4 +- ...-handling-on-no-longer-valid-oidc-token.md | 28 +++++++++-- .../authorization_interceptors.dart | 48 ++++++++++++------- .../authorization_interceptor_test.dart | 30 +++++++++++- 5 files changed, 90 insertions(+), 26 deletions(-) diff --git a/core/lib/utils/app_logger.dart b/core/lib/utils/app_logger.dart index 799d472f6..fe9dd9b93 100644 --- a/core/lib/utils/app_logger.dart +++ b/core/lib/utils/app_logger.dart @@ -82,7 +82,9 @@ void _internalLog( if (shouldSentry) { if (level == Level.trace) { - SentryManager.instance.captureMessage(rawMessage, extras: extras); + unawaited( + SentryManager.instance.captureMessage(rawMessage, extras: extras), + ); } else { unawaited( SentryManager.instance.captureException( @@ -208,11 +210,13 @@ void logDebug( void logTrace( String? message, { bool webConsoleEnabled = false, + Map? extras, }) { _internalLog( message, level: Level.trace, webConsoleEnabled: webConsoleEnabled, + extras: extras, ); } diff --git a/docs/adr/0031-fix-refresh-token-with-oidc.md b/docs/adr/0031-fix-refresh-token-with-oidc.md index 3e28cf293..31b92e117 100644 --- a/docs/adr/0031-fix-refresh-token-with-oidc.md +++ b/docs/adr/0031-fix-refresh-token-with-oidc.md @@ -4,9 +4,9 @@ Date: 2023-09-11 ## Status -- Issue: +Superseded by [ADR-0035](0035-error-handling-on-no-longer-valid-oidc-token.md) -[1974](https://github.com/linagora/tmail-flutter/issues/1974) +- Issue: [1974](https://github.com/linagora/tmail-flutter/issues/1974) ## Context diff --git a/docs/adr/0035-error-handling-on-no-longer-valid-oidc-token.md b/docs/adr/0035-error-handling-on-no-longer-valid-oidc-token.md index 424ad15c8..68f6eac02 100644 --- a/docs/adr/0035-error-handling-on-no-longer-valid-oidc-token.md +++ b/docs/adr/0035-error-handling-on-no-longer-valid-oidc-token.md @@ -1,19 +1,37 @@ -# 35. Error handling on no longer valid OIDC token (Issue #2592) +# 35. OIDC Token Refresh Mechanism in AuthorizationInterceptors Date: 2024-02-15 +Updated: 2026-02-09 ## Status -Accepted +Accepted (supersedes [ADR-0031](0031-fix-refresh-token-with-oidc.md)) ## Context -- When my OIDC token is expired, I see an awful red error message while being redirected +Multiple issues drove the evolution of the refresh token logic: + +1. **Concurrent 401s caused logout** ([#1974](https://github.com/linagora/tmail-flutter/issues/1974)) — parallel requests with stale headers all triggered independent refresh attempts. +2. **Ugly error on expired token** ([#2592](https://github.com/linagora/tmail-flutter/issues/2592)) — users saw a red error instead of a silent redirect. +3. **400 on refresh not handled** — when the refresh token itself was revoked/expired, the server returned 400 (invalid_grant) but the interceptor did not distinguish this from transient errors, leading to confusing failures. +4. **Server-side revocation before local expiry** — relying on local `isExpired` check caused the interceptor to skip refresh when the server had already revoked the token. ## Decision -- When my OIDC token is expired, I am just redirected to the OIDC provider portal. No need to show an awful error. +`AuthorizationInterceptors` extends `QueuedInterceptorsWrapper` (Dio). The `onError` handler processes errors one at a time with the following ordered checks: + +1. **`_refreshAttemptedKey` guard** — if the request already attempted a refresh/retry, skip everything and propagate the error. Prevents infinite loops. +2. **`validateToRetryTheRequestWithNewToken`** (checked first) — only on **401** responses: if `_token` was already updated by a preceding queued request, retry immediately with the new token. No refresh call needed. Non-401 errors (500, 403, etc.) are never retried here — they are server errors unrelated to authentication. +3. **`validateToRefreshToken`** — if status is 401, auth type is OIDC, and access/refresh tokens are present, attempt refresh. The local `isExpired` check was **removed**; we trust the server's 401. + - **Success (different token):** update `_token`, persist to cache, mark `_refreshAttemptedKey`, retry. + - **Success (same token — duplicate):** propagate original error, no retry. + - **`DioException` 400:** call `clear()` (wipe auth state) and reject with `RefreshTokenFailedException` — triggers silent logout. + - **`DioException` other:** propagate original error, preserve OIDC state. +4. **Outer catch:** `ServerError`/`TemporarilyUnavailable` wrapped in `DioException`; other exceptions forwarded via `err.copyWith`. ## Consequences -- In case of receiving a `BadCredentials` error, the system automatically logs out and returns to the login screen without any notification to the user. \ No newline at end of file +- Queued requests after a successful refresh retry without redundant refresh calls. +- 400 from the OIDC provider causes immediate session cleanup and silent redirect to login. +- No infinite retry loops thanks to the `_refreshAttemptedKey` per-request flag. +- Server-side token revocation is handled correctly regardless of local expiry time. diff --git a/lib/features/login/data/network/interceptors/authorization_interceptors.dart b/lib/features/login/data/network/interceptors/authorization_interceptors.dart index 49b4ececd..5387e965a 100644 --- a/lib/features/login/data/network/interceptors/authorization_interceptors.dart +++ b/lib/features/login/data/network/interceptors/authorization_interceptors.dart @@ -91,13 +91,10 @@ class AuthorizationInterceptors extends QueuedInterceptorsWrapper { final extraInRequest = requestOptions.extra; bool isRetryRequest = false; - // Check if this request has already attempted a refresh/retry final hasAttemptedRefresh = extraInRequest[_refreshAttemptedKey] == true; - // FIRST: Check if token was already updated by another request in the queue - // If so, just retry with the new token - no refresh needed - // But skip if we've already attempted (to prevent infinite loops) if (!hasAttemptedRefresh && validateToRetryTheRequestWithNewToken( + responseStatusCode: err.response?.statusCode, authHeader: requestOptions.headers[HttpHeaders.authorizationHeader], tokenOIDC: _token )) { @@ -107,7 +104,6 @@ class AuthorizationInterceptors extends QueuedInterceptorsWrapper { responseStatusCode: err.response?.statusCode, tokenOIDC: _token )) { - // SECOND: Check if we should attempt to refresh the token try { log('AuthorizationInterceptors::onError: Perform get New Token'); final newTokenOidc = PlatformInfo.isIOS @@ -126,7 +122,6 @@ class AuthorizationInterceptors extends QueuedInterceptorsWrapper { await _iosSharingManager.saveKeyChainSharingSession(personalAccount); } - // Mark that we've attempted refresh for this request requestOptions.extra[_refreshAttemptedKey] = true; isRetryRequest = true; } on DioException catch (refreshError, st) { @@ -149,12 +144,30 @@ class AuthorizationInterceptors extends QueuedInterceptorsWrapper { return handler.reject(sessionExpiredError); } - return super.onError(err, handler); + logError( + 'AuthorizationInterceptors: Refresh token failed with ' + 'statusCode=${refreshError.response?.statusCode}', + exception: refreshError, + stackTrace: st, + ); + + if (refreshError is ServerError || + refreshError is TemporarilyUnavailable) { + return super.onError( + DioException( + requestOptions: err.requestOptions, + error: refreshError, + ), + handler, + ); + } else { + return super.onError(err.copyWith(error: refreshError), handler); + } } } else { logTrace( 'AuthorizationInterceptors::onError: ' - '401 received but refresh skipped. ' + 'No retry or refresh applicable. ' 'statusCode = ${err.response?.statusCode} | ' 'authType = $_authenticationType | ' 'hasConfig = ${_configOIDC != null} | ' @@ -249,9 +262,6 @@ class AuthorizationInterceptors extends QueuedInterceptorsWrapper { final hasAccessToken = _isTokenNotEmpty(tokenOIDC); final hasRefreshToken = _isRefreshTokenNotEmpty(tokenOIDC); - // Note: We removed isExpired check. If server returns 401, we trust it - // and attempt refresh regardless of local expiry time. This handles cases - // where server clock is ahead or token was revoked server-side. final canProceedRefresh = isStatusCode401 && isLoginWithOIDC && hasAccessToken && @@ -271,21 +281,25 @@ class AuthorizationInterceptors extends QueuedInterceptorsWrapper { } bool validateToRetryTheRequestWithNewToken( - {required String? authHeader, required TokenOIDC? tokenOIDC}) { + {required int? responseStatusCode, + required String? authHeader, + required TokenOIDC? tokenOIDC}) { + final isStatusCode401 = responseStatusCode == 401; final hasAuthHeader = authHeader != null; final hasAccessToken = _isTokenNotEmpty(tokenOIDC); final isTokenStillValid = !_isTokenExpired(tokenOIDC); final isTokenUpdated = tokenOIDC != null && authHeader?.contains(tokenOIDC.token) != true; - // Note: We don't check isTokenExpired here. If another request already - // refreshed the token, we should retry with the new token regardless of - // its expiry status. The key check is isTokenUpdated. - final shouldRetry = - hasAuthHeader && hasAccessToken && isTokenStillValid && isTokenUpdated; + final shouldRetry = isStatusCode401 && + hasAuthHeader && + hasAccessToken && + isTokenStillValid && + isTokenUpdated; logTrace( 'AuthorizationInterceptors::validateToRetryWithNewToken: ' + 'isStatusCode401 = $isStatusCode401 | ' 'hasHeader = $hasAuthHeader | ' 'hasAccessToken = $hasAccessToken | ' 'isTokenValid = $isTokenStillValid | ' diff --git a/test/features/interceptor/authorization_interceptor_test.dart b/test/features/interceptor/authorization_interceptor_test.dart index cf3d4edb6..fbbc0f3a5 100644 --- a/test/features/interceptor/authorization_interceptor_test.dart +++ b/test/features/interceptor/authorization_interceptor_test.dart @@ -225,10 +225,11 @@ void main() { // ============================================================ group('validateToRetryTheRequestWithNewToken', () { test( - 'should return TRUE when auth header present, token updated, and token not expired', + 'should return TRUE when 401, auth header present, token updated, and token not expired', () { final result = authorizationInterceptors.validateToRetryTheRequestWithNewToken( + responseStatusCode: 401, authHeader: 'Bearer old_token', tokenOIDC: OIDCFixtures.newTokenOidc, ); @@ -237,9 +238,32 @@ void main() { }, ); + test('should return FALSE when status code is not 401 (e.g. 500)', () { + final result = + authorizationInterceptors.validateToRetryTheRequestWithNewToken( + responseStatusCode: 500, + authHeader: 'Bearer old_token', + tokenOIDC: OIDCFixtures.newTokenOidc, + ); + + expect(result, false); + }); + + test('should return FALSE when status code is null', () { + final result = + authorizationInterceptors.validateToRetryTheRequestWithNewToken( + responseStatusCode: null, + authHeader: 'Bearer old_token', + tokenOIDC: OIDCFixtures.newTokenOidc, + ); + + expect(result, false); + }); + test('should return FALSE when auth header is null', () { final result = authorizationInterceptors.validateToRetryTheRequestWithNewToken( + responseStatusCode: 401, authHeader: null, tokenOIDC: OIDCFixtures.newTokenOidc, ); @@ -252,6 +276,7 @@ void main() { () { final result = authorizationInterceptors.validateToRetryTheRequestWithNewToken( + responseStatusCode: 401, authHeader: 'Bearer ${OIDCFixtures.newTokenOidc.token}', tokenOIDC: OIDCFixtures.newTokenOidc, ); @@ -263,6 +288,7 @@ void main() { test('should return FALSE when token is expired', () { final result = authorizationInterceptors.validateToRetryTheRequestWithNewToken( + responseStatusCode: 401, authHeader: 'Bearer some_other_token', tokenOIDC: OIDCFixtures.tokenOidcExpiredTime, ); @@ -273,6 +299,7 @@ void main() { test('should return FALSE when token is empty', () { final result = authorizationInterceptors.validateToRetryTheRequestWithNewToken( + responseStatusCode: 401, authHeader: 'Bearer some_token', tokenOIDC: OIDCFixtures.tokenOidcExpiredTimeAndTokenEmpty, ); @@ -283,6 +310,7 @@ void main() { test('should return FALSE when tokenOIDC is null', () { final result = authorizationInterceptors.validateToRetryTheRequestWithNewToken( + responseStatusCode: 401, authHeader: 'Bearer some_token', tokenOIDC: null, );