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 <noreply@anthropic.com>
This commit is contained in:
@@ -61,6 +61,13 @@ class AuthInterceptor extends Interceptor {
|
|||||||
/// when the refresh completes, allowing a later expiry to refresh again.
|
/// when the refresh completes, allowing a later expiry to refresh again.
|
||||||
Future<String?>? _ongoingRefresh;
|
Future<String?>? _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.
|
/// Refreshes the token, coalescing concurrent callers onto a single refresh.
|
||||||
Future<String?> _refreshTokenOnce() {
|
Future<String?> _refreshTokenOnce() {
|
||||||
return _ongoingRefresh ??= _refreshToken().whenComplete(() {
|
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
|
// tagged so a fresh 401 on it short-circuits the block above rather than
|
||||||
// refreshing again. The Authorization header is intentionally not set here:
|
// refreshing again. The Authorization header is intentionally not set here:
|
||||||
// [onRequest] re-injects the (now refreshed) bearer token on the replay.
|
// [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) {
|
if (newAccessToken != null) {
|
||||||
try {
|
try {
|
||||||
final RequestOptions options = error.requestOptions
|
final RequestOptions options = error.requestOptions
|
||||||
@@ -126,7 +147,14 @@ class AuthInterceptor extends Interceptor {
|
|||||||
final Response<dynamic> response = await _dio.fetch(options);
|
final Response<dynamic> response = await _dio.fetch(options);
|
||||||
return handler.resolve(response);
|
return handler.resolve(response);
|
||||||
} on DioException catch (e) {
|
} 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}');
|
appLogger.error('❌ Retry after token refresh failed: ${e.message}');
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -138,18 +166,24 @@ class AuthInterceptor extends Interceptor {
|
|||||||
}
|
}
|
||||||
|
|
||||||
Future<void> _logout() async {
|
Future<void> _logout() async {
|
||||||
final Future<void> Function()? callback = _onAuthFailed();
|
if (_loggingOut) return;
|
||||||
if (callback != null) {
|
_loggingOut = true;
|
||||||
await callback();
|
try {
|
||||||
return;
|
final Future<void> Function()? callback = _onAuthFailed();
|
||||||
}
|
if (callback != null) {
|
||||||
|
await callback();
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
// Defensive fallback: if no logout/navigation callback has been wired yet,
|
// Defensive fallback: if no logout/navigation callback has been wired yet,
|
||||||
// at least clear the stored credentials so the router's auth guard can
|
// at least clear the stored credentials so the router's auth guard can
|
||||||
// redirect the user away from protected routes.
|
// redirect the user away from protected routes.
|
||||||
appLogger.error('⚠️ No auth-failed callback registered; clearing tokens');
|
appLogger.error('⚠️ No auth-failed callback registered; clearing tokens');
|
||||||
await _prefs.remove(PrefKeys.bearer);
|
await _prefs.remove(PrefKeys.bearer);
|
||||||
await _prefs.remove(PrefKeys.refreshToken);
|
await _prefs.remove(PrefKeys.refreshToken);
|
||||||
await _prefs.remove(PrefKeys.customerData);
|
await _prefs.remove(PrefKeys.customerData);
|
||||||
|
} finally {
|
||||||
|
_loggingOut = false;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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<ResponseBody> Function(RequestOptions options) responder;
|
||||||
|
int requestCount = 0;
|
||||||
|
|
||||||
|
@override
|
||||||
|
Future<ResponseBody> fetch(
|
||||||
|
RequestOptions options,
|
||||||
|
Stream<Uint8List>? requestStream,
|
||||||
|
Future<void>? cancelFuture,
|
||||||
|
) {
|
||||||
|
requestCount++;
|
||||||
|
return responder(options);
|
||||||
|
}
|
||||||
|
|
||||||
|
@override
|
||||||
|
void close({bool force = false}) {}
|
||||||
|
}
|
||||||
|
|
||||||
|
ResponseBody _json(int status, [Map<String, dynamic> body = const {}]) {
|
||||||
|
return ResponseBody.fromString(
|
||||||
|
jsonEncode(body),
|
||||||
|
status,
|
||||||
|
headers: <String, List<String>>{
|
||||||
|
Headers.contentTypeHeader: <String>[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(<String, Object>{
|
||||||
|
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<String?> 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<void>.delayed(const Duration(milliseconds: 10));
|
||||||
|
await prefs.setString(PrefKeys.bearer, 'new-token');
|
||||||
|
return 'new-token';
|
||||||
|
}
|
||||||
|
|
||||||
|
void attachInterceptor({Future<String?> 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, <String, dynamic>{
|
||||||
|
'message': 'unauthorized',
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
attachInterceptor();
|
||||||
|
|
||||||
|
await expectLater(
|
||||||
|
dio.get<dynamic>('/api/v1/protected'),
|
||||||
|
throwsA(isA<DioException>()),
|
||||||
|
);
|
||||||
|
|
||||||
|
// 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, <String, dynamic>{'ok': isReplay});
|
||||||
|
});
|
||||||
|
attachInterceptor();
|
||||||
|
|
||||||
|
final List<Response<dynamic>> responses = await Future.wait(
|
||||||
|
<Future<Response<dynamic>>>[
|
||||||
|
dio.get<dynamic>('/api/v1/a'),
|
||||||
|
dio.get<dynamic>('/api/v1/b'),
|
||||||
|
],
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(refreshCalls, 1);
|
||||||
|
expect(logoutCalls, 0);
|
||||||
|
expect(
|
||||||
|
responses.every((Response<dynamic> 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<dynamic>('/api/v1/protected'),
|
||||||
|
throwsA(
|
||||||
|
isA<DioException>().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');
|
||||||
|
},
|
||||||
|
);
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user