diff --git a/app/src/main/java/app/waveflow/data/remote/CatalogRepository.kt b/app/src/main/java/app/waveflow/data/remote/CatalogRepository.kt index 08d1635..ec27b35 100644 --- a/app/src/main/java/app/waveflow/data/remote/CatalogRepository.kt +++ b/app/src/main/java/app/waveflow/data/remote/CatalogRepository.kt @@ -5,7 +5,6 @@ import app.waveflow.model.RemoteAlbumDetail import app.waveflow.model.RemoteArtist import app.waveflow.model.RemoteArtistDetail import app.waveflow.model.RemoteSearchResults -import app.waveflow.model.ServerSession /** * Le catalogue distant, muni d'une session. @@ -46,42 +45,38 @@ class CatalogRepository( /** * Exécute [call] avec un jeton valide, en réessayant une fois sur refus. * - * [ServerSessionRepository.validAccessToken] renouvelle déjà avant - * l'échéance, mais un jeton peut être révoqué depuis un autre appareil : il - * est alors valide selon l'horloge et refusé par le serveur. Le second essai - * repart d'un jeton fraîchement obtenu ; s'il échoue à son tour, c'est que - * la session est bel et bien fermée. + * [ServerSessionRepository.authorize] renouvelle déjà avant l'échéance, + * mais un jeton peut être révoqué depuis un autre appareil : il est alors + * valide selon l'horloge et refusé par le serveur. Le second essai repart + * d'un jeton fraîchement obtenu ; s'il échoue à son tour, c'est que la + * session est bel et bien fermée. + * + * L'adresse et le jeton viennent du même appel, donc de la même session : + * les demander séparément permettrait d'adresser à un serveur le jeton d'un + * autre, si l'utilisateur se reconnecte ailleurs entre les deux. */ private suspend fun authorized(call: suspend (String, String) -> T): T { - val first = token() ?: throw ServerException.Unauthorized(SESSION_CLOSED) + val first = sessionRepository.authorize() + ?: throw ServerException.Unauthorized(SESSION_CLOSED) return try { - call(first.first, first.second) + call(first.serverUrl, first.accessToken) } catch (refused: ServerException.Unauthorized) { - val renewed = renewedToken() ?: throw refused - call(renewed.first, renewed.second) + // Périmer d'abord : sans ça, le second essai réutiliserait le jeton + // que le serveur vient de refuser, l'échéance locale le croyant bon. + sessionRepository.expireAccessToken(first.accessToken) + // Un renouvellement n'est utilisable que sur le serveur du premier + // essai : l'appel s'est déroulé sans verrou, et la session a pu + // basculer ailleurs entre-temps. Les identifiants n'ont de sens que + // pour celui qui les a émis — rejouer ailleurs rendrait une erreur, + // ou pire une ressource étrangère portant le même identifiant. + val renewed = sessionRepository.authorize() + ?.takeIf { it.serverUrl == first.serverUrl } + ?: throw refused + call(renewed.serverUrl, renewed.accessToken) } } - /** Adresse et jeton courants, ou `null` sans session. */ - private suspend fun token(): Pair? { - val accessToken = sessionRepository.validAccessToken() ?: return null - val url = (sessionRepository.session.value as? ServerSession.Connected)?.serverUrl - ?: return null - return url to accessToken - } - - /** - * Force un renouvellement en périmant le jeton courant. - * - * Sans ça, le second essai réutiliserait celui que le serveur vient de - * refuser : l'échéance locale le croit encore bon. - */ - private suspend fun renewedToken(): Pair? { - sessionRepository.expireAccessToken() - return token() - } - private companion object { const val SESSION_CLOSED = "Aucune session serveur." } diff --git a/app/src/main/java/app/waveflow/data/remote/ServerImageAuthInterceptor.kt b/app/src/main/java/app/waveflow/data/remote/ServerImageAuthInterceptor.kt index bf92624..e0245e7 100644 --- a/app/src/main/java/app/waveflow/data/remote/ServerImageAuthInterceptor.kt +++ b/app/src/main/java/app/waveflow/data/remote/ServerImageAuthInterceptor.kt @@ -1,6 +1,5 @@ package app.waveflow.data.remote -import app.waveflow.model.ServerSession import kotlinx.coroutines.runBlocking import okhttp3.Interceptor import okhttp3.Response @@ -16,6 +15,17 @@ import okhttp3.Response * jaquette venue d'ailleurs pourrait ; lui joindre le jeton reviendrait à le * confier à un tiers. * + * L'origine est comparée à celle que [ServerSessionRepository.authorize] rend + * **avec** le jeton, et non à une session lue auparavant : entre les deux + * lectures, l'utilisateur peut s'être reconnecté ailleurs, et le jeton du + * nouveau serveur partirait à l'ancien. + * + * Conséquence assumée : une image venue d'ailleurs passe elle aussi par le + * verrou de session, et peut déclencher un renouvellement qui n'attendait plus + * qu'un appel. Le prix est modeste — le renouvellement était dû — et la + * garantie ne tient qu'à ce prix : filtrer avant de demander le jeton, c'est + * filtrer sur une session qui n'est peut-être plus celle du jeton obtenu. + * * L'appel est bloquant : les intercepteurs OkHttp le sont, et s'exécutent sur * ses propres fils. */ @@ -25,14 +35,7 @@ class ServerImageAuthInterceptor( override fun intercept(chain: Interceptor.Chain): Response { val request = chain.request() - val session = sessionRepository.session.value as? ServerSession.Connected - ?: return chain.proceed(request) - - if (!request.url.isSameOriginAs(session.serverUrl)) return chain.proceed(request) - - val token = runCatching { runBlocking { sessionRepository.validAccessToken() } } - .getOrNull() - ?: return chain.proceed(request) + val token = tokenFor(request) ?: return chain.proceed(request) val signed = chain.proceed(request.withBearer(token)) if (signed.code != HTTP_UNAUTHORIZED) return signed @@ -41,16 +44,41 @@ class ServerImageAuthInterceptor( // le catalogue, on le périme et on rejoue une fois — un second refus // veut dire que la session est fermée, et la pochette manquera. signed.close() - val renewed = runCatching { - runBlocking { - sessionRepository.expireAccessToken() - sessionRepository.validAccessToken() - } - }.getOrNull() ?: return chain.proceed(request) + val renewed = renewedTokenFor(request, refused = token) ?: return chain.proceed(request) return chain.proceed(request.withBearer(renewed)) } + /** Périme le jeton refusé, puis en redemande un — la session en émettra un neuf. */ + private fun renewedTokenFor(request: okhttp3.Request, refused: String): String? { + // Si la péremption échoue, redemander rendrait le même jeton que le + // serveur vient de refuser : autant renoncer que rejouer pour rien. + obtain { sessionRepository.expireAccessToken(refused) } ?: return null + return tokenFor(request) + } + + /** + * Le jeton à joindre à [request], ou `null` s'il n'y a rien à signer. + * + * La session est prise d'un bloc — adresse et jeton — puis confrontée à + * l'URL demandée : le jeton n'est joint que s'il appartient bien au serveur + * auquel la requête s'adresse. + */ + private fun tokenFor(request: okhttp3.Request): String? { + val authorized = obtain { sessionRepository.authorize() } ?: return null + return authorized.accessToken.takeIf { request.url.isSameOriginAs(authorized.serverUrl) } + } + + /** + * Exécute [call] en bloquant, sans laisser remonter d'échec. + * + * Un serveur injoignable ou un refus ne doivent pas faire échouer le + * chargement de l'image : la requête partira sans signature, et la pochette + * manquera au pire. + */ + private fun obtain(call: suspend () -> T): T? = + runCatching { runBlocking { call() } }.getOrNull() + private fun okhttp3.Request.withBearer(token: String) = newBuilder().header("Authorization", "Bearer $token").build() diff --git a/app/src/main/java/app/waveflow/data/remote/ServerSessionRepository.kt b/app/src/main/java/app/waveflow/data/remote/ServerSessionRepository.kt index 5b68227..26b5981 100644 --- a/app/src/main/java/app/waveflow/data/remote/ServerSessionRepository.kt +++ b/app/src/main/java/app/waveflow/data/remote/ServerSessionRepository.kt @@ -9,6 +9,19 @@ import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock +/** + * Une adresse de serveur et un jeton d'accès **de la même session**. + * + * Les deux ne valent qu'ensemble : un jeton n'a de sens que pour le serveur qui + * l'a émis, et l'envoyer ailleurs reviendrait à le confier à un tiers. Les lire + * en deux temps laisserait la session changer entre les deux — d'où cette + * paire, rendue en une seule prise du verrou. + */ +data class AuthorizedCall( + val serverUrl: String, + val accessToken: String, +) + /** * La session serveur, et les seuls chemins qui la font changer. * @@ -78,23 +91,30 @@ class ServerSessionRepository( } /** - * Un jeton d'accès utilisable, renouvelé si son échéance approche. + * De quoi appeler le serveur connecté : son adresse et un jeton d'accès + * utilisable, renouvelé si son échéance approche. * * Renvoie `null` quand il n'y a pas de session, ou quand le renouvellement * a été refusé — auquel cas la session est effacée et l'utilisateur devra * ressaisir son mot de passe. * + * Les deux sont rendus ensemble à dessein : un appelant qui lirait + * l'adresse d'un côté et le jeton de l'autre pourrait les prendre à deux + * sessions différentes, et adresser à l'un le jeton de l'autre. + * * @throws ServerException.Unreachable si le serveur ne répond pas ; la * session est conservée, l'appelant réessaiera. */ - suspend fun validAccessToken(): String? = mutex.withLock { + suspend fun authorize(): AuthorizedCall? = mutex.withLock { val current = _session.value as? ServerSession.Connected ?: return@withLock null - if (now() < current.accessExpiresAtMs - EXPIRY_MARGIN_MS) return@withLock current.accessToken + if (now() < current.accessExpiresAtMs - EXPIRY_MARGIN_MS) { + return@withLock AuthorizedCall(current.serverUrl, current.accessToken) + } try { val tokens = api.refresh(current.serverUrl, current.refreshToken) persist(tokens.toSession(current.serverUrl)) - tokens.accessToken + AuthorizedCall(current.serverUrl, tokens.accessToken) } catch (refused: ServerException.Unauthorized) { // Le jeton de rafraîchissement est mort : révoqué ailleurs, ou // périmé. Rien à réessayer, il faut une nouvelle connexion. @@ -105,14 +125,19 @@ class ServerSessionRepository( } /** - * Marque le jeton d'accès comme périmé. + * Marque [refused] comme périmé, si c'est bien le jeton courant. * * Utile quand le serveur en refuse un que l'horloge locale croit encore * bon — révoqué depuis un autre appareil, par exemple. Le prochain - * [validAccessToken] renouvellera au lieu de resservir le même. + * [authorize] renouvellera au lieu de resservir le même. + * + * Le jeton refusé est exigé parce qu'un autre appelant a pu renouveler + * entre le refus et cet appel : périmer aveuglément jetterait un jeton neuf + * et déclencherait un renouvellement pour rien. */ - suspend fun expireAccessToken() = mutex.withLock { + suspend fun expireAccessToken(refused: String) = mutex.withLock { val current = _session.value as? ServerSession.Connected ?: return@withLock + if (current.accessToken != refused) return@withLock persist(current.copy(accessExpiresAtMs = 0L)) } diff --git a/app/src/test/java/app/waveflow/data/remote/CatalogRepositoryTest.kt b/app/src/test/java/app/waveflow/data/remote/CatalogRepositoryTest.kt index dd66706..2d82153 100644 --- a/app/src/test/java/app/waveflow/data/remote/CatalogRepositoryTest.kt +++ b/app/src/test/java/app/waveflow/data/remote/CatalogRepositoryTest.kt @@ -93,6 +93,28 @@ class CatalogRepositoryTest { assertEquals(2, catalog.calls) } + @Test + fun `un rejeu ne part pas vers un autre serveur que le premier essai`() = runTest { + // Le jeton est refusé et, pendant l'appel, l'utilisateur se reconnecte + // ailleurs. La paire adresse-jeton reste cohérente, donc rien ne fuit — + // mais les identifiants n'ont de sens que pour le serveur qui les a + // émis : rejouer sur le nouveau rendrait une erreur, ou pire une + // ressource étrangère portant le même identifiant. + val catalog = FakeCatalogApi(failuresBeforeSuccess = 1) + val sessions = connectedSessionRepository() + catalog.pendantLAppel = { + // Une seule fois : c'est la bascule qu'on veut, pas une boucle. + catalog.pendantLAppel = null + sessions.connect("https://ailleurs.test", "autre", "secret") + } + val repository = CatalogRepository(catalog, sessions) + + val error = runCatching { repository.album("alb-1") }.exceptionOrNull() + + assertTrue("le refus initial doit remonter", error is ServerException.Unauthorized) + assertEquals("rien ne doit être rejoué ailleurs", 1, catalog.calls) + } + @Test fun `une panne reseau n'est pas prise pour un jeton perime`() = runTest { val catalog = FakeCatalogApi(failure = ServerException.Unreachable("coupure")) diff --git a/app/src/test/java/app/waveflow/data/remote/ServerImageAuthInterceptorTest.kt b/app/src/test/java/app/waveflow/data/remote/ServerImageAuthInterceptorTest.kt index caf09f7..c98ede4 100644 --- a/app/src/test/java/app/waveflow/data/remote/ServerImageAuthInterceptorTest.kt +++ b/app/src/test/java/app/waveflow/data/remote/ServerImageAuthInterceptorTest.kt @@ -7,8 +7,10 @@ import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest import okhttp3.OkHttpClient import okhttp3.Request +import okhttp3.mockwebserver.Dispatcher import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer +import okhttp3.mockwebserver.RecordedRequest import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertNull @@ -139,6 +141,46 @@ class ServerImageAuthInterceptorTest { assertEquals(1, api.refreshCalls) } + @Test + fun `un jeton obtenu apres un changement de serveur ne part pas a l'ancien`() = runTest { + // La fenêtre : l'intercepteur attend la réponse de l'ancien serveur — + // il ne tient alors aucun verrou — et l'utilisateur se reconnecte + // ailleurs pendant ce temps. Le jeton qu'il obtiendra ensuite est celui + // du nouveau serveur ; le rejeu l'enverrait à l'ancien, qui n'a rien à + // en connaître. + // + // La bascule est déclenchée depuis la réponse elle-même, ce qui la + // place exactement dans la fenêtre plutôt que d'espérer l'y croiser. + val autre = MockWebServer() + autre.start() + + try { + val sessions = sessions() + var bascule = false + server.dispatcher = object : Dispatcher() { + override fun dispatch(request: RecordedRequest): MockResponse { + if (!bascule) { + bascule = true + val ailleurs = autre.url("/").toString().trimEnd('/') + runBlocking { sessions.connect(ailleurs, "autre", "secret") } + } + return MockResponse().setResponseCode(401) + } + } + + fetch(clientWith(sessions), "${url()}/api/v2/artwork/1daf991a") + + assertEquals("Bearer wfa_stocke", server.takeRequest().getHeader("Authorization")) + assertNull( + "le jeton du nouveau serveur ne doit pas partir à l'ancien", + server.takeRequest().getHeader("Authorization"), + ) + assertEquals("le nouveau serveur n'a rien demandé", 0, autre.requestCount) + } finally { + autre.shutdown() + } + } + @Test fun `un second refus n'est pas rejoue indefiniment`() = runTest { server.enqueue(MockResponse().setResponseCode(401)) @@ -164,6 +206,6 @@ class ServerImageAuthInterceptorTest { fetch(clientWith(sessions), "${url()}/api/v2/artwork/1daf991a") - assertEquals("wfa_stocke", runBlocking { sessions.validAccessToken() }) + assertEquals("wfa_stocke", runBlocking { sessions.authorize()?.accessToken }) } } diff --git a/app/src/test/java/app/waveflow/data/remote/ServerSessionRepositoryTest.kt b/app/src/test/java/app/waveflow/data/remote/ServerSessionRepositoryTest.kt index bbc8406..e29dbd4 100644 --- a/app/src/test/java/app/waveflow/data/remote/ServerSessionRepositoryTest.kt +++ b/app/src/test/java/app/waveflow/data/remote/ServerSessionRepositoryTest.kt @@ -114,7 +114,11 @@ class ServerSessionRepositoryTest { val repository = repository(api = api, store = FakeSessionStore(stored = connectedSession())) repository.restore() - assertEquals("wfa_stocke", repository.validAccessToken()) + val autorise = repository.authorize() + assertEquals("wfa_stocke", autorise?.accessToken) + // L'adresse compte autant que le jeton : c'est leur appariement qui + // empêche d'adresser à un serveur le jeton d'un autre. + assertEquals("https://musique.test", autorise?.serverUrl) assertEquals(0, api.refreshCalls) } @@ -128,7 +132,10 @@ class ServerSessionRepositoryTest { ) repository.restore() - assertEquals("wfa_1", repository.validAccessToken()) + val autorise = repository.authorize() + assertEquals("wfa_1", autorise?.accessToken) + // Le renouvellement ne change pas de serveur. + assertEquals("https://musique.test", autorise?.serverUrl) assertEquals(1, api.refreshCalls) } @@ -140,7 +147,7 @@ class ServerSessionRepositoryTest { val repository = repository(store = store) repository.restore() - repository.validAccessToken() + repository.authorize() val session = repository.session.value as ServerSession.Connected assertEquals("wfr_1", session.refreshToken) @@ -157,11 +164,11 @@ class ServerSessionRepositoryTest { ) repository.restore() - repository.validAccessToken() + repository.authorize() assertEquals("wfr_stocke", api.lastRefreshToken) maintenant += 900_000L - repository.validAccessToken() + repository.authorize() assertEquals("wfr_1", api.lastRefreshToken) } @@ -179,8 +186,8 @@ class ServerSessionRepositoryTest { ) repository.restore() - val premier = async { repository.validAccessToken() } - val second = async { repository.validAccessToken() } + val premier = async { repository.authorize()?.accessToken } + val second = async { repository.authorize()?.accessToken } runCurrent() portail.complete(Unit) @@ -198,7 +205,7 @@ class ServerSessionRepositoryTest { val repository = repository(api = api, store = store) repository.restore() - assertNull(repository.validAccessToken()) + assertNull(repository.authorize()?.accessToken) assertEquals(ServerSession.Disconnected, repository.session.value) assertTrue("la session doit aussi être effacée du disque", store.cleared) } @@ -214,7 +221,7 @@ class ServerSessionRepositoryTest { ) repository.restore() - val error = runCatching { repository.validAccessToken() }.exceptionOrNull() + val error = runCatching { repository.authorize()?.accessToken }.exceptionOrNull() assertTrue(error is ServerException.Unreachable) assertTrue(repository.session.value is ServerSession.Connected) @@ -222,7 +229,41 @@ class ServerSessionRepositoryTest { @Test fun `sans session il n'y a pas de jeton`() = runTest { - assertNull(repository().validAccessToken()) + assertNull(repository().authorize()?.accessToken) + } + + @Test + fun `perimer le jeton refuse force un renouvellement`() = runTest { + // Le serveur refuse un jeton que l'horloge locale croit encore bon. + // Sans le périmer, l'essai suivant reservirait le même. + val api = FakeServerApi() + val repository = repository(api = api, store = FakeSessionStore(stored = connectedSession())) + repository.restore() + + repository.expireAccessToken("wfa_stocke") + + assertEquals("wfa_1", repository.authorize()?.accessToken) + assertEquals(1, api.refreshCalls) + } + + @Test + fun `perimer un jeton deja remplace laisse le neuf en place`() = runTest { + // Deux appels essuient un refus en même temps et le premier renouvelle. + // Si le second périmait à l'aveugle, il jetterait un jeton neuf et + // provoquerait un renouvellement de plus — le serveur faisant tourner + // son jeton de rafraîchissement pour rien. + val api = FakeServerApi() + val repository = repository( + api = api, + store = FakeSessionStore(stored = connectedSession(expiresAtMs = maintenant)), + ) + repository.restore() + assertEquals("wfa_1", repository.authorize()?.accessToken) + + repository.expireAccessToken("wfa_stocke") + + assertEquals("wfa_1", repository.authorize()?.accessToken) + assertEquals("un seul renouvellement", 1, api.refreshCalls) } @Test diff --git a/app/src/test/java/app/waveflow/testing/ServerFakes.kt b/app/src/test/java/app/waveflow/testing/ServerFakes.kt index 1de247d..4a634c6 100644 --- a/app/src/test/java/app/waveflow/testing/ServerFakes.kt +++ b/app/src/test/java/app/waveflow/testing/ServerFakes.kt @@ -109,6 +109,14 @@ class FakeCatalogApi( var lastPage: Pair? = null private set + /** + * Exécuté à chaque appel, avant l'échec éventuel. + * + * De quoi faire bouger la session pendant que le dépôt attend sa réponse : + * c'est la fenêtre où il ne tient aucun verrou. + */ + var pendantLAppel: (suspend () -> Unit)? = null + override suspend fun albums( serverUrl: String, accessToken: String, @@ -173,12 +181,13 @@ class FakeCatalogApi( return "$serverUrl/api/v2/stream/ticket-$trackId" } - private fun record(serverUrl: String, accessToken: String, page: Pair?) { + private suspend fun record(serverUrl: String, accessToken: String, page: Pair?) { calls++ lastServerUrl = serverUrl lastAccessToken = accessToken page?.let { lastPage = it } + pendantLAppel?.invoke() failure?.let { throw it } if (calls <= failuresBeforeSuccess) { throw ServerException.Unauthorized("jeton refusé")