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 b663bf25..888963f8 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?, @@ -139,8 +162,13 @@ private 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() ?: "", @@ -150,7 +178,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 { @@ -160,21 +190,79 @@ private 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 { - // 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 } } +/** + * 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 * @@ -264,36 +352,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) } } @@ -328,6 +387,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) } } 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 new file mode 100644 index 00000000..cecdc30a --- /dev/null +++ b/shared/src/commonTest/kotlin/com/fivucsas/shared/di/RefreshTokenResilienceTest.kt @@ -0,0 +1,211 @@ +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. Plus (root cause 3): a 200 refresh + * that omits a new refresh token must KEEP the existing one, never overwrite + * it with "". + */ +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()) + } + + // ---- 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 + 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)) + } +}