From 231d800b47498517faabe41a674c6d44ba1b5e03 Mon Sep 17 00:00:00 2001 From: AmirReza Jamali Date: Sat, 13 Jun 2026 16:20:23 +0330 Subject: [PATCH] Fix AuthInterceptor replay logout-on-transient-error + add tests Address PR re-review findings: - Only a 401 on the replay falls through to logout; other DioExceptions (timeout, connection drop, 5xx) now propagate so a flaky network can't wipe a valid session and callers see the real error. - Skip the refresh when the failed request's bearer no longer matches the stored one (a concurrent refresh already rotated it) and replay directly. - Guard _logout() with a _loggingOut flag so the auth-failed callback fires once per burst rather than once per concurrent caller. Add unit tests (fake Dio adapter + mocked prefs) covering the retry guard, single-flight refresh, and non-401 replay propagation paths. Co-Authored-By: Claude Opus 4.8 --- lib/core/network/auth_interceptor.dart | 62 +++++-- test/core/network/auth_interceptor_test.dart | 166 +++++++++++++++++++ 2 files changed, 214 insertions(+), 14 deletions(-) create mode 100644 test/core/network/auth_interceptor_test.dart diff --git a/lib/core/network/auth_interceptor.dart b/lib/core/network/auth_interceptor.dart index 7dd69a9..f4bf9cd 100644 --- a/lib/core/network/auth_interceptor.dart +++ b/lib/core/network/auth_interceptor.dart @@ -61,6 +61,13 @@ class AuthInterceptor extends Interceptor { /// when the refresh completes, allowing a later expiry to refresh again. Future? _ongoingRefresh; + /// Guards against the auth-failed callback running once per concurrent caller. + /// When the shared refresh fails, every request awaiting it falls through to + /// [_logout]; only the first should fire the (possibly non-idempotent) + /// callback. Set synchronously before the first `await` so the rest of the + /// burst short-circuits, and reset afterwards so a later session can log out. + bool _loggingOut = false; + /// Refreshes the token, coalescing concurrent callers onto a single refresh. Future _refreshTokenOnce() { return _ongoingRefresh ??= _refreshToken().whenComplete(() { @@ -118,7 +125,21 @@ class AuthInterceptor extends Interceptor { // tagged so a fresh 401 on it short-circuits the block above rather than // refreshing again. The Authorization header is intentionally not set here: // [onRequest] re-injects the (now refreshed) bearer token on the replay. - final String? newAccessToken = await _refreshTokenOnce(); + // + // Refresh-window straggler guard: a request sent with the old token can 401 + // just after a concurrent refresh already finished (so [_ongoingRefresh] is + // cleared and the single-flight no longer coalesces it). If the stored token + // no longer matches the one this request was sent with, it was already + // rotated by someone else — replay with the fresh token instead of starting + // a second refresh that would needlessly rotate the just-issued token again. + final String? storedToken = _prefs.getString(PrefKeys.bearer); + final String? sentAuthHeader = + error.requestOptions.headers['Authorization'] as String?; + final bool alreadyRefreshed = + storedToken != null && sentAuthHeader != 'Bearer $storedToken'; + + final String? newAccessToken = + alreadyRefreshed ? storedToken : await _refreshTokenOnce(); if (newAccessToken != null) { try { final RequestOptions options = error.requestOptions @@ -126,7 +147,14 @@ class AuthInterceptor extends Interceptor { final Response response = await _dio.fetch(options); return handler.resolve(response); } on DioException catch (e) { - // Replay still failed (e.g. token revoked) – fall through to logout. + // Only a fresh 401 means refreshing didn't help (token revoked, account + // disabled) — fall through to logout. Anything else (timeout, connection + // drop, 5xx) is a transient/server failure unrelated to auth: propagate + // it so a flaky network doesn't wipe a perfectly valid session, and so + // the caller sees the real error instead of the stale pre-refresh 401. + if (e.response?.statusCode != 401) { + return handler.next(e); + } appLogger.error('❌ Retry after token refresh failed: ${e.message}'); } } @@ -138,18 +166,24 @@ class AuthInterceptor extends Interceptor { } Future _logout() async { - final Future Function()? callback = _onAuthFailed(); - if (callback != null) { - await callback(); - return; - } + if (_loggingOut) return; + _loggingOut = true; + try { + final Future Function()? callback = _onAuthFailed(); + if (callback != null) { + await callback(); + return; + } - // Defensive fallback: if no logout/navigation callback has been wired yet, - // at least clear the stored credentials so the router's auth guard can - // redirect the user away from protected routes. - appLogger.error('⚠️ No auth-failed callback registered; clearing tokens'); - await _prefs.remove(PrefKeys.bearer); - await _prefs.remove(PrefKeys.refreshToken); - await _prefs.remove(PrefKeys.customerData); + // Defensive fallback: if no logout/navigation callback has been wired yet, + // at least clear the stored credentials so the router's auth guard can + // redirect the user away from protected routes. + appLogger.error('⚠️ No auth-failed callback registered; clearing tokens'); + await _prefs.remove(PrefKeys.bearer); + await _prefs.remove(PrefKeys.refreshToken); + await _prefs.remove(PrefKeys.customerData); + } finally { + _loggingOut = false; + } } } diff --git a/test/core/network/auth_interceptor_test.dart b/test/core/network/auth_interceptor_test.dart new file mode 100644 index 0000000..5b29aeb --- /dev/null +++ b/test/core/network/auth_interceptor_test.dart @@ -0,0 +1,166 @@ +import 'dart:convert'; +import 'dart:typed_data'; + +import 'package:dio/dio.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:tm_app/core/constants/pref_keys.dart'; +import 'package:tm_app/core/network/auth_interceptor.dart'; +import 'package:tm_app/core/utils/logger.dart'; + +/// A configurable [HttpClientAdapter] that lets each test decide, per request, +/// what the wire should return. Avoids a mocking dependency the repo doesn't +/// pull in while still exercising the real Dio request/replay machinery. +class _FakeAdapter implements HttpClientAdapter { + _FakeAdapter(this.responder); + + Future Function(RequestOptions options) responder; + int requestCount = 0; + + @override + Future fetch( + RequestOptions options, + Stream? requestStream, + Future? cancelFuture, + ) { + requestCount++; + return responder(options); + } + + @override + void close({bool force = false}) {} +} + +ResponseBody _json(int status, [Map body = const {}]) { + return ResponseBody.fromString( + jsonEncode(body), + status, + headers: >{ + Headers.contentTypeHeader: [Headers.jsonContentType], + }, + ); +} + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + // The interceptor logs through the app-wide singleton, which is `late`-init. + appLogger.init(); + + late Dio dio; + late SharedPreferences prefs; + late int refreshCalls; + late int logoutCalls; + + setUp(() async { + SharedPreferences.setMockInitialValues({ + PrefKeys.bearer: 'old-token', + PrefKeys.refreshToken: 'old-refresh', + }); + prefs = await SharedPreferences.getInstance(); + refreshCalls = 0; + logoutCalls = 0; + dio = Dio(BaseOptions(baseUrl: 'https://example.test')); + }); + + /// Refresh that succeeds: rotates the stored bearer and returns the new token, + /// mirroring the production refresh which persists before replaying. + Future refreshSucceeds() async { + refreshCalls++; + // A small delay so concurrent 401s both register on the single-flight + // future before it resolves, exercising the coalescing path for real. + await Future.delayed(const Duration(milliseconds: 10)); + await prefs.setString(PrefKeys.bearer, 'new-token'); + return 'new-token'; + } + + void attachInterceptor({Future Function()? refreshToken}) { + dio.interceptors.add( + AuthInterceptor( + dio: dio, + prefs: prefs, + refreshToken: refreshToken ?? refreshSucceeds, + onAuthFailed: () => () async { + logoutCalls++; + await prefs.clear(); + }, + ), + ); + } + + test( + 'retry guard: a replay that still 401s does not trigger a second refresh', + () async { + // Every request — original and replay — comes back 401. + dio.httpClientAdapter = _FakeAdapter( + (RequestOptions options) async => _json(401, { + 'message': 'unauthorized', + }), + ); + attachInterceptor(); + + await expectLater( + dio.get('/api/v1/protected'), + throwsA(isA()), + ); + + // The refresh + replay happens exactly once; the second 401 short-circuits + // via the auth_retried flag instead of refreshing again. + expect(refreshCalls, 1); + expect(logoutCalls, 1); + }, + ); + + test('single-flight: concurrent 401s share a single refresh', () async { + // First attempt 401s; the tagged replay succeeds. + dio.httpClientAdapter = _FakeAdapter((RequestOptions options) async { + final bool isReplay = options.extra['auth_retried'] == true; + return _json(isReplay ? 200 : 401, {'ok': isReplay}); + }); + attachInterceptor(); + + final List> responses = await Future.wait( + >>[ + dio.get('/api/v1/a'), + dio.get('/api/v1/b'), + ], + ); + + expect(refreshCalls, 1); + expect(logoutCalls, 0); + expect( + responses.every((Response r) => r.statusCode == 200), + isTrue, + ); + }); + + test( + 'non-401 replay failure propagates and does not log the user out', + () async { + // Refresh succeeds, but the replay hits a transient/server error (a 500 + // here stands in for the timeout/connection-drop case): authentication is + // fine, so the error must surface to the caller rather than wipe the + // session. + dio.httpClientAdapter = _FakeAdapter((RequestOptions options) async { + final bool isReplay = options.extra['auth_retried'] == true; + return _json(isReplay ? 500 : 401); + }); + attachInterceptor(); + + await expectLater( + dio.get('/api/v1/protected'), + throwsA( + isA().having( + (DioException e) => e.response?.statusCode, + 'replay status code', + 500, + ), + ), + ); + + expect(refreshCalls, 1); + expect(logoutCalls, 0); + // Session left intact for the retry — credentials were not cleared. + expect(prefs.getString(PrefKeys.bearer), 'new-token'); + }, + ); +}