From b4cf47d3633f436ee0f66ea04498d8ff3d326222 Mon Sep 17 00:00:00 2001 From: Ahmet Abdullah Gultekin Date: Fri, 12 Jun 2026 13:37:54 +0000 Subject: [PATCH 1/3] fix(auth): keep session on transient refresh failure, clear only on invalid_grant (F8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause 1 (primary) of the intermittent mobile forced re-login. `refreshAccessToken` wiped the ENTIRE persisted session on ANY refresh failure: the blanket `catch (_) { clearTokens() }` plus the non-200 branches cleared tokens on every timeout, dropped keep-alive, brief offline, Traefik HTTP/2 stale-connection RST, 5xx, or 429. A transient network blip therefore logged the user fully out — surfacing as the "sometimes asks for login, sometimes doesn't" complaint. Now a DEFINITIVE auth failure (HTTP 400/401 invalid_grant from the refresh-token grant → token truly dead) is distinguished from a TRANSIENT failure (IOException/timeout/RST, or a non-auth non-200 such as 5xx/429). clearTokens() runs ONLY on a definitive failure; transient failures return false WITHOUT clearing, so only the in-flight request fails and the session survives for the next attempt. Adds `isDefinitiveAuthFailure(status)` + makes the refresh helper `internal` for unit testing. RefreshTokenResilienceTest covers transient 503 / transport-error / legacy 5xx (session kept), definitive 400/401 (session cleared), and the 200 happy path. By design: access TTL 15 min, refresh TTL 24 h (sliding) — an actively used app should never re-prompt; the only normal forced re-login is after ~24 h of inactivity. --- .../com/fivucsas/shared/di/NetworkModule.kt | 42 +++- .../shared/di/RefreshTokenResilienceTest.kt | 185 ++++++++++++++++++ 2 files changed, 220 insertions(+), 7 deletions(-) create mode 100644 shared/src/commonTest/kotlin/com/fivucsas/shared/di/RefreshTokenResilienceTest.kt diff --git a/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt b/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt index b663bf25..66768377 100644 --- a/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt +++ b/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt @@ -95,12 +95,35 @@ private fun isAuthEndpoint(url: String): Boolean = url.contains("/auth/login") || url.contains("/auth/refresh") || url.contains("/auth/logout") || url.contains("/oauth2/token") +/** + * A definitive auth failure is one the server reports for the refresh-token grant + * itself: HTTP 400/401 from `/oauth2/token` (or `/auth/refresh`) means the refresh + * token is truly dead (expired, revoked, reuse-detection family-revoke → the OAuth + * `invalid_grant` family). Only these justify wiping the session and routing to + * login. EVERYTHING else — a socket timeout, a dropped keep-alive, a brief offline, + * a Traefik HTTP/2 stale-connection RST, a 5xx/502/503, a 429 — is TRANSIENT: it + * says nothing about whether the refresh token is still valid, so we must NOT clear + * the persisted session over it; only the in-flight request fails and the next + * attempt re-tries with the surviving tokens. + */ +internal fun isDefinitiveAuthFailure(status: HttpStatusCode): Boolean = + status == HttpStatusCode.BadRequest || status == HttpStatusCode.Unauthorized + /** * Perform a single silent token refresh under [refreshMutex]. * * Returns true if, after this call, a usable access token exists (either because - * we refreshed it, or because a concurrent 401 handler already did). Returns - * false if refresh was impossible/failed (tokens are then cleared → re-login). + * we refreshed it, or because a concurrent 401 handler already did). Returns false + * if the refresh did not produce a usable token. + * + * Token-clearing policy (the F8 fix): the ENTIRE persisted session is wiped ONLY on + * a DEFINITIVE auth failure ([isDefinitiveAuthFailure]) — the server explicitly + * rejecting the refresh token. On a TRANSIENT failure (an IOException/timeout/RST + * caught below, or a non-auth non-200 such as 5xx/429) we return false WITHOUT + * clearing tokens, so the session survives for the next attempt. The previous + * blanket `catch (_) { clearTokens() }` + clear-on-any-non-200 wiped the whole + * session on any flaky network event, which surfaced as the intermittent + * "sometimes asks for login, sometimes doesn't" forced re-login. * * [staleAccessToken] is the `Authorization` header the failed request carried; if * the stored access token already differs we skip the refresh (a concurrent @@ -109,7 +132,7 @@ private fun isAuthEndpoint(url: String): Boolean = * it is now reusable from the HttpSend interceptor so the original request can be * transparently re-fired (closing the old "caller receives the 401" TODO). */ -private suspend fun refreshAccessToken( +internal suspend fun refreshAccessToken( client: HttpClient, tokenManager: TokenManager, staleAccessToken: String?, @@ -150,7 +173,9 @@ private suspend fun refreshAccessToken( ) true } else { - tokenManager.clearTokens() + // Definitive (400/401 invalid_grant) → token is dead, wipe + relogin. + // Transient (5xx/429/…) → keep the session, just fail this request. + if (isDefinitiveAuthFailure(refreshResponse.status)) tokenManager.clearTokens() false } } else { @@ -164,13 +189,16 @@ private suspend fun refreshAccessToken( tokenManager.updateTokens(authResponse.toModel()) true } else { - // Refresh failed — clear tokens to force re-login. - tokenManager.clearTokens() + // Same split as the OAuth branch: only a definitive auth rejection + // clears the session; a transient server/network error preserves it. + if (isDefinitiveAuthFailure(refreshResponse.status)) tokenManager.clearTokens() false } } } catch (_: Exception) { - tokenManager.clearTokens() + // Transport/IO failure (timeout, dropped keep-alive, Traefik RST, offline). + // This says NOTHING about refresh-token validity — keep the session intact + // and let the next request retry; do NOT clear tokens here. false } } diff --git a/shared/src/commonTest/kotlin/com/fivucsas/shared/di/RefreshTokenResilienceTest.kt b/shared/src/commonTest/kotlin/com/fivucsas/shared/di/RefreshTokenResilienceTest.kt new file mode 100644 index 00000000..14099951 --- /dev/null +++ b/shared/src/commonTest/kotlin/com/fivucsas/shared/di/RefreshTokenResilienceTest.kt @@ -0,0 +1,185 @@ +package com.fivucsas.shared.di + +import com.fivucsas.shared.data.local.TokenManager +import com.fivucsas.shared.data.local.TokenStorage +import com.fivucsas.shared.domain.repository.AuthTokens +import io.ktor.client.HttpClient +import io.ktor.client.engine.mock.MockEngine +import io.ktor.client.engine.mock.respond +import io.ktor.client.plugins.contentnegotiation.ContentNegotiation +import io.ktor.http.HttpHeaders +import io.ktor.http.HttpStatusCode +import io.ktor.http.headersOf +import io.ktor.serialization.kotlinx.json.json +import kotlinx.coroutines.test.runTest +import kotlinx.serialization.json.Json +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNotNull +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * F8 — mobile random-logout regression guards. + * + * The dominant root cause was [refreshAccessToken] wiping the ENTIRE persisted + * session on ANY refresh failure (a blanket `catch { clearTokens() }` plus + * clear-on-any-non-200). Transient network blips therefore forced a full + * re-login — the intermittent "sometimes asks, sometimes doesn't". + * + * The fix: clear tokens ONLY on a DEFINITIVE auth rejection (400/401 + * invalid_grant); on a transient failure (IOException/timeout/5xx) keep the + * session so the next request can retry. + */ +class RefreshTokenResilienceTest { + + private val json = Json { ignoreUnknownKeys = true; isLenient = true } + + /** Minimal in-memory TokenStorage so we can observe clear/preserve. */ + private class FakeTokenStorage : TokenStorage { + private var storedToken: String? = null + private var storedRefresh: String? = null + private var storedOauth: Boolean = false + private var storedRole: String? = null + + override fun saveToken(token: String) { storedToken = token } + override fun getToken(): String? = storedToken + override fun clearToken() { storedToken = null } + override fun saveRefreshToken(token: String) { storedRefresh = token } + override fun getRefreshToken(): String? = storedRefresh + override fun clearRefreshToken() { storedRefresh = null } + override fun saveRole(role: String) { storedRole = role } + override fun getRole(): String? = storedRole + override fun clearRole() { storedRole = null } + override fun saveOAuthSession(oauth: Boolean) { storedOauth = oauth } + override fun getOAuthSession(): Boolean = storedOauth + override fun clearOAuthSession() { storedOauth = false } + } + + private fun seededManager( + access: String = "stale-access", + refresh: String = "refresh-123", + oauth: Boolean = true, + ): Pair { + val storage = FakeTokenStorage() + val tm = TokenManager(storage) + tm.saveTokens( + AuthTokens( + accessToken = access, + refreshToken = refresh, + expiresIn = 300, + oauthSession = oauth, + ), + ) + return tm to storage + } + + private fun client(handler: io.ktor.client.engine.mock.MockRequestHandler): HttpClient = + HttpClient(MockEngine(handler)) { + install(ContentNegotiation) { json(json) } + } + + // ---- transient failures must NOT wipe the session ---- + + @Test + fun `oauth refresh on transient 503 returns false but KEEPS tokens`() = runTest { + val (tm, _) = seededManager() + val httpClient = client { respond("upstream unavailable", HttpStatusCode.ServiceUnavailable) } + + val ok = refreshAccessToken(httpClient, tm, "Bearer stale-access") + + assertFalse(ok, "a transient 503 cannot produce a usable token") + assertNotNull(tm.getRefreshToken(), "session must SURVIVE a transient 503") + assertEquals("refresh-123", tm.getRefreshToken()) + } + + @Test + fun `oauth refresh on a transport error returns false but KEEPS tokens`() = runTest { + val (tm, _) = seededManager() + // The MockEngine handler throwing models a dropped keep-alive / Traefik RST + // / offline — exactly the transient failure the broad catch must NOT punish + // with a session wipe. + val httpClient = client { throw RuntimeException("dropped keep-alive / Traefik RST") } + + val ok = refreshAccessToken(httpClient, tm, "Bearer stale-access") + + assertFalse(ok) + assertNotNull(tm.getRefreshToken(), "a transport error must not log the user out") + } + + @Test + fun `legacy refresh on transient 500 returns false but KEEPS tokens`() = runTest { + val (tm, _) = seededManager(oauth = false) + val httpClient = client { respond("boom", HttpStatusCode.InternalServerError) } + + val ok = refreshAccessToken(httpClient, tm, "Bearer stale-access") + + assertFalse(ok) + assertNotNull(tm.getRefreshToken(), "legacy session must survive a transient 5xx too") + } + + // ---- definitive auth failures DO wipe the session ---- + + @Test + fun `oauth refresh on definitive 400 invalid_grant CLEARS tokens`() = runTest { + val (tm, _) = seededManager() + val httpClient = client { + respond( + content = "{\"error\":\"invalid_grant\"}", + status = HttpStatusCode.BadRequest, + headers = headersOf(HttpHeaders.ContentType, "application/json"), + ) + } + + val ok = refreshAccessToken(httpClient, tm, "Bearer stale-access") + + assertFalse(ok) + assertNull(tm.getRefreshToken(), "a dead refresh token MUST clear the session") + assertNull(tm.getAccessToken()) + } + + @Test + fun `oauth refresh on definitive 401 CLEARS tokens`() = runTest { + val (tm, _) = seededManager() + val httpClient = client { respond("unauthorized", HttpStatusCode.Unauthorized) } + + val ok = refreshAccessToken(httpClient, tm, "Bearer stale-access") + + assertFalse(ok) + assertNull(tm.getRefreshToken()) + } + + // ---- happy path ---- + + @Test + fun `oauth refresh on 200 updates tokens and returns true`() = runTest { + val (tm, _) = seededManager() + val httpClient = client { + respond( + content = "{\"access_token\":\"new-access\",\"refresh_token\":\"new-refresh\",\"expires_in\":900}", + status = HttpStatusCode.OK, + headers = headersOf(HttpHeaders.ContentType, "application/json"), + ) + } + + val ok = refreshAccessToken(httpClient, tm, "Bearer stale-access") + + assertTrue(ok) + assertEquals("new-access", tm.getAccessToken()) + assertEquals("new-refresh", tm.getRefreshToken()) + } + + // ---- isDefinitiveAuthFailure classification ---- + + @Test + fun `only 400 and 401 are definitive auth failures`() { + assertTrue(isDefinitiveAuthFailure(HttpStatusCode.BadRequest)) + assertTrue(isDefinitiveAuthFailure(HttpStatusCode.Unauthorized)) + assertFalse(isDefinitiveAuthFailure(HttpStatusCode.InternalServerError)) + assertFalse(isDefinitiveAuthFailure(HttpStatusCode.ServiceUnavailable)) + assertFalse(isDefinitiveAuthFailure(HttpStatusCode.BadGateway)) + assertFalse(isDefinitiveAuthFailure(HttpStatusCode.TooManyRequests)) + assertFalse(isDefinitiveAuthFailure(HttpStatusCode.Forbidden)) + } +} From 81b25ac3ae900d8cc0c0d45cba530017a7fb3af5 Mon Sep 17 00:00:00 2001 From: Ahmet Abdullah Gultekin Date: Fri, 12 Jun 2026 13:38:54 +0000 Subject: [PATCH 2/3] fix(auth): route biometric client 401s through the shared mutexed refresh (F8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause 2 of the intermittent mobile forced re-login: refresh-token reuse-detection family-revoke from uncoordinated refresh. The server revokes the entire refresh-token rotation family when an already-rotated token is re-presented. `refreshMutex` serialized refreshes only WITHIN the identity client; the biometric client had NO refresh-on-401 interceptor and was outside the mutex. A biometric 401 racing an identity refresh — or presenting a token rotated out from under it — could re-present the old refresh token, triggering a family revoke that logs out ALL sessions non-deterministically. Extract the identity client's HttpSend refresh-on-401 + single-retry into a shared `installRefreshOn401(client, tokenManager)` and install it on BOTH clients. Both now funnel every 401 through the same `refreshAccessToken` and therefore the same module-level `refreshMutex`, so only one rotation is ever in flight. The refresh targets the identity token endpoint via absolute URLs, so driving it from the biometric client is correct (no behavior change to the identity path — pure extraction). --- .../com/fivucsas/shared/di/NetworkModule.kt | 90 ++++++++++++------- 1 file changed, 60 insertions(+), 30 deletions(-) diff --git a/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt b/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt index 66768377..c7fdd81f 100644 --- a/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt +++ b/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt @@ -203,6 +203,57 @@ internal suspend fun refreshAccessToken( } } +/** + * Install the shared transparent refresh-on-401 + single-retry interceptor on + * [httpClient]. + * + * Both the identity and the biometric clients use THIS one function so every 401 + * is funnelled through the SAME [refreshAccessToken] (and therefore the SAME + * module-level [refreshMutex]). That cross-client serialization is the F8 fix for + * refresh-token reuse-detection family-revoke: previously the biometric client had + * NO refresh interceptor and was outside the mutex, so a biometric 401 racing an + * identity refresh (or presenting a token rotated out from under it) could + * re-present an already-rotated refresh token → the server revokes the whole + * rotation family → all sessions logged out non-deterministically. Routing both + * through one mutexed refresh removes that race. + * + * The refresh itself always targets the IDENTITY token endpoint (absolute URLs in + * [refreshAccessToken]), so it is correct to drive it from the biometric client + * too — the absolute URL overrides that client's biometric `defaultRequest` base. + */ +private fun installRefreshOn401(httpClient: HttpClient, tokenManager: TokenManager) { + // Transparent refresh-on-401 + RETRY of the original request. + // + // We use the HttpSend plugin (part of ktor-client-core — no extra + // dependency; the project deliberately avoids io.ktor:ktor-client-auth, + // see desktopApp RefreshInterceptor) because, unlike HttpResponseValidator, + // its interceptor CAN re-execute the request. This closes the old TODO + // where the caller received the raw 401 and the session appeared to + // die after the ~15-min access-token lifetime: now the access token is + // refreshed under a mutex and the SAME request is re-fired once with the + // fresh bearer (defaultRequest re-reads the stored token), so the call + // succeeds transparently instead of surfacing a logout. + httpClient.plugin(HttpSend).intercept { request: HttpRequestBuilder -> + val originalCall = execute(request) + val url = originalCall.request.url.toString() + if (originalCall.response.status != HttpStatusCode.Unauthorized || isAuthEndpoint(url)) { + return@intercept originalCall + } + + // Snapshot the access token this request carried so the refresh + // helper can detect a concurrent rotation and avoid double-refresh. + val staleAccessToken = originalCall.request.headers[HttpHeaders.Authorization] + val refreshed = refreshAccessToken(httpClient, tokenManager, staleAccessToken) + if (!refreshed) { + return@intercept originalCall + } + + // Re-fire the original request once. defaultRequest stamps the + // freshly-stored access token onto the retry. + execute(request) + } +} + /** * Network module - Provides HTTP clients and API clients * @@ -292,36 +343,7 @@ val networkModule = module { } }.also { httpClient -> - // Transparent refresh-on-401 + RETRY of the original request. - // - // We use the HttpSend plugin (part of ktor-client-core — no extra - // dependency; the project deliberately avoids io.ktor:ktor-client-auth, - // see desktopApp RefreshInterceptor) because, unlike HttpResponseValidator, - // its interceptor CAN re-execute the request. This closes the old TODO - // where the caller received the raw 401 and the session appeared to - // die after the ~15-min access-token lifetime: now the access token is - // refreshed under a mutex and the SAME request is re-fired once with the - // fresh bearer (defaultRequest re-reads the stored token), so the call - // succeeds transparently instead of surfacing a logout. - httpClient.plugin(HttpSend).intercept { request: HttpRequestBuilder -> - val originalCall = execute(request) - val url = originalCall.request.url.toString() - if (originalCall.response.status != HttpStatusCode.Unauthorized || isAuthEndpoint(url)) { - return@intercept originalCall - } - - // Snapshot the access token this request carried so the refresh - // helper can detect a concurrent rotation and avoid double-refresh. - val staleAccessToken = originalCall.request.headers[HttpHeaders.Authorization] - val refreshed = refreshAccessToken(httpClient, tokenManager, staleAccessToken) - if (!refreshed) { - return@intercept originalCall - } - - // Re-fire the original request once. defaultRequest stamps the - // freshly-stored access token onto the retry. - execute(request) - } + installRefreshOn401(httpClient, tokenManager) } } @@ -356,6 +378,14 @@ val networkModule = module { header(HttpHeaders.Authorization, "Bearer $accessToken") } } + }.also { httpClient -> + // F8: the biometric client previously had NO refresh interceptor and + // sat outside [refreshMutex], so a biometric 401 could refresh out of + // band / re-present an already-rotated refresh token → reuse-detection + // family-revoke → all sessions logged out. Use the SAME shared, + // mutex-serialized refresh as the identity client. The refresh targets + // the identity token endpoint via absolute URLs, so it is correct here. + installRefreshOn401(httpClient, tokenManager) } } From 9e31ce3a20bff51d67c61bb7e04986cca58f77f9 Mon Sep 17 00:00:00 2001 From: Ahmet Abdullah Gultekin Date: Fri, 12 Jun 2026 13:44:17 +0000 Subject: [PATCH 3/3] fix(auth): never blank the stored refresh token; treat blank tokens as absent (F8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause 3 (hardening) of the intermittent mobile forced re-login. `AuthDto.toModel()` maps a missing `refresh_token`/`access_token` to "". On a refresh 200 that omits a new refresh token (rotation disabled), the old code overwrote the stored refresh token with "" — so the very next refresh had no token and forced a full re-login. - NetworkModule: on a 200 refresh, keep the existing refresh token when the response omits one (`oauth.refreshToken.ifBlank { refreshToken }`), in both the OAuth and legacy `/auth/refresh` branches (mirrors the desktop RefreshInterceptor's "reuse if rotation disabled"). - TokenManager.getAccessToken/getRefreshToken: treat a blank ("") stored value as ABSENT (return null), so a "" never looks like a usable token and `isAuthenticated()` falls back correctly instead of routing a session-less user to the dashboard or POSTing an empty grant the server rejects. Tests: RefreshTokenResilienceTest covers the 200-without-refresh_token preserve case; TokenManagerBlankTokenTest covers blank-as-absent for both tokens and isAuthenticated. --- .../shared/data/local/TokenManager.kt | 18 ++++-- .../com/fivucsas/shared/di/NetworkModule.kt | 13 +++- .../data/local/TokenManagerBlankTokenTest.kt | 59 +++++++++++++++++++ .../shared/di/RefreshTokenResilienceTest.kt | 28 ++++++++- 4 files changed, 111 insertions(+), 7 deletions(-) create mode 100644 shared/src/commonTest/kotlin/com/fivucsas/shared/data/local/TokenManagerBlankTokenTest.kt diff --git a/shared/src/commonMain/kotlin/com/fivucsas/shared/data/local/TokenManager.kt b/shared/src/commonMain/kotlin/com/fivucsas/shared/data/local/TokenManager.kt index f17cc6d6..796b6292 100644 --- a/shared/src/commonMain/kotlin/com/fivucsas/shared/data/local/TokenManager.kt +++ b/shared/src/commonMain/kotlin/com/fivucsas/shared/data/local/TokenManager.kt @@ -55,17 +55,27 @@ class TokenManager( } /** - * Get current access token + * Get current access token. + * + * A blank stored value is treated as ABSENT (returns null): the DTO mappers + * default a missing token to "", and a "" must never look like a usable token. */ fun getAccessToken(): String? { - return cachedTokens?.accessToken ?: tokenStorage.getToken() + return (cachedTokens?.accessToken ?: tokenStorage.getToken())?.takeIf { it.isNotBlank() } } /** - * Get current refresh token (from cache or persistent storage) + * Get current refresh token (from cache or persistent storage). + * + * A blank stored token is treated as ABSENT (returns null): the DTO mappers + * default a missing `refresh_token` to "", and a "" must never look like a + * usable token — otherwise the refresh path would POST an empty grant, the + * server would (rightly) reject it as `invalid_grant`, and the user would be + * spuriously forced to re-login. Treating blank as absent here also makes + * [isAuthenticated] fall back correctly. */ fun getRefreshToken(): String? { - return cachedTokens?.refreshToken ?: tokenStorage.getRefreshToken() + return (cachedTokens?.refreshToken ?: tokenStorage.getRefreshToken())?.takeIf { it.isNotBlank() } } /** diff --git a/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt b/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt index c7fdd81f..888963f8 100644 --- a/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt +++ b/shared/src/commonMain/kotlin/com/fivucsas/shared/di/NetworkModule.kt @@ -162,8 +162,13 @@ internal suspend fun refreshAccessToken( val oauth = refreshResponse.body().toModel() // The OAuth refresh response carries no profile fields — re-apply // the cached identity so role/tenant context survives the rotation. + // When the server omits `refresh_token` (rotation disabled), the DTO + // maps it to "" — KEEP the existing refresh token instead of wiping + // it to blank, or the very next refresh would have no token and force + // a re-login. tokenManager.updateTokens( oauth.copy( + refreshToken = oauth.refreshToken.ifBlank { refreshToken }, role = tokenManager.getRole() ?: oauth.role, userName = tokenManager.getUserName() ?: "", userEmail = tokenManager.getUserEmail() ?: "", @@ -185,8 +190,12 @@ internal suspend fun refreshAccessToken( setBody(mapOf("refreshToken" to refreshToken)) } if (refreshResponse.status == HttpStatusCode.OK) { - val authResponse = refreshResponse.body() - tokenManager.updateTokens(authResponse.toModel()) + val refreshed = refreshResponse.body().toModel() + // Same blank-refresh-token guard as the OAuth branch: if the server + // didn't rotate (omitted refreshToken → ""), preserve the current one. + tokenManager.updateTokens( + refreshed.copy(refreshToken = refreshed.refreshToken.ifBlank { refreshToken }), + ) true } else { // Same split as the OAuth branch: only a definitive auth rejection diff --git a/shared/src/commonTest/kotlin/com/fivucsas/shared/data/local/TokenManagerBlankTokenTest.kt b/shared/src/commonTest/kotlin/com/fivucsas/shared/data/local/TokenManagerBlankTokenTest.kt new file mode 100644 index 00000000..48e418fa --- /dev/null +++ b/shared/src/commonTest/kotlin/com/fivucsas/shared/data/local/TokenManagerBlankTokenTest.kt @@ -0,0 +1,59 @@ +package com.fivucsas.shared.data.local + +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * F8 hardening — a blank ("") stored token must be treated as ABSENT. + * + * The DTO mappers default a missing access/refresh token to "" (see + * AuthDto.toModel). If TokenManager treated "" as a real token it would (a) make + * [TokenManager.isAuthenticated] route a session-less user to the dashboard, and + * (b) drive the refresh path to POST an empty grant the server rejects as + * invalid_grant → a spurious forced re-login. Both contributed to F8. + */ +class TokenManagerBlankTokenTest { + + private class MutableStorage( + initialToken: String? = null, + initialRefresh: String? = null, + ) : TokenStorage { + private var storedToken: String? = initialToken + private var storedRefresh: String? = initialRefresh + override fun saveToken(token: String) { storedToken = token } + override fun getToken(): String? = storedToken + override fun clearToken() { storedToken = null } + override fun saveRefreshToken(token: String) { storedRefresh = token } + override fun getRefreshToken(): String? = storedRefresh + override fun clearRefreshToken() { storedRefresh = null } + override fun saveRole(role: String) {} + override fun getRole(): String? = null + override fun clearRole() {} + } + + @Test + fun `blank access token reads back as null`() { + val tm = TokenManager(MutableStorage(initialToken = "", initialRefresh = "r")) + assertNull(tm.getAccessToken(), "an empty-string access token must be treated as absent") + } + + @Test + fun `blank refresh token reads back as null`() { + val tm = TokenManager(MutableStorage(initialToken = "a", initialRefresh = "")) + assertNull(tm.getRefreshToken(), "an empty-string refresh token must be treated as absent") + } + + @Test + fun `not authenticated when only a blank refresh token is stored`() { + val tm = TokenManager(MutableStorage(initialToken = null, initialRefresh = "")) + assertFalse(tm.isAuthenticated(), "a blank refresh token must NOT count as a session") + } + + @Test + fun `authenticated when a real refresh token is stored but access is missing`() { + val tm = TokenManager(MutableStorage(initialToken = null, initialRefresh = "real-refresh")) + assertTrue(tm.isAuthenticated(), "a real refresh token keeps the user signed in") + } +} diff --git a/shared/src/commonTest/kotlin/com/fivucsas/shared/di/RefreshTokenResilienceTest.kt b/shared/src/commonTest/kotlin/com/fivucsas/shared/di/RefreshTokenResilienceTest.kt index 14099951..cecdc30a 100644 --- a/shared/src/commonTest/kotlin/com/fivucsas/shared/di/RefreshTokenResilienceTest.kt +++ b/shared/src/commonTest/kotlin/com/fivucsas/shared/di/RefreshTokenResilienceTest.kt @@ -30,7 +30,9 @@ import kotlin.test.assertTrue * * The fix: clear tokens ONLY on a DEFINITIVE auth rejection (400/401 * invalid_grant); on a transient failure (IOException/timeout/5xx) keep the - * session so the next request can retry. + * session so the next request can retry. Plus (root cause 3): a 200 refresh + * that omits a new refresh token must KEEP the existing one, never overwrite + * it with "". */ class RefreshTokenResilienceTest { @@ -170,6 +172,30 @@ class RefreshTokenResilienceTest { assertEquals("new-refresh", tm.getRefreshToken()) } + // ---- Root cause 3: a 200 omitting refresh_token must KEEP the old one ---- + + @Test + fun `oauth refresh 200 without refresh_token preserves existing refresh token`() = runTest { + val (tm, _) = seededManager(refresh = "original-refresh") + val httpClient = client { + respond( + content = "{\"access_token\":\"new-access\",\"expires_in\":900}", + status = HttpStatusCode.OK, + headers = headersOf(HttpHeaders.ContentType, "application/json"), + ) + } + + val ok = refreshAccessToken(httpClient, tm, "Bearer stale-access") + + assertTrue(ok) + assertEquals("new-access", tm.getAccessToken()) + assertEquals( + "original-refresh", + tm.getRefreshToken(), + "rotation-disabled refresh must not blank out the stored refresh token", + ) + } + // ---- isDefinitiveAuthFailure classification ---- @Test