fix(android): the app stops signing itself out at launch (#6911)
release / web (push) Successful in 1m24s
release / go (push) Successful in 1m52s
release / govulncheck (push) Successful in 18s
release / integration (push) Successful in 4m43s
release / android (push) Failing after 3m25s
release / Build signed APK (push) Successful in 4m41s
release / Attach APK to the Release (tag releases only) (push) Skipped
release / Build + push container image (push) Skipped
release / Verify release artifacts (tag releases only) (push) Skipped
release / web (push) Successful in 1m24s
release / go (push) Successful in 1m52s
release / govulncheck (push) Successful in 18s
release / integration (push) Successful in 4m43s
release / android (push) Failing after 3m25s
release / Build signed APK (push) Successful in 4m41s
release / Attach APK to the Release (tag releases only) (push) Skipped
release / Build + push container image (push) Skipped
release / Verify release artifacts (tag releases only) (push) Skipped
At launch the stored session cookie loads from the Keystore off the main thread. A request sent before that finished went out without it, typically the library sync that fires as soon as the server answers. The server then answered 401, and the interceptor cleared the store on any 401, which also stopped the load from restoring the cookie and wrote null over the stored one. The user had to sign in on every launch. A session lost mid-use also left them on Home with every request failing, because the app only routes to sign-in when it starts. - The interceptor waits for the stored cookie to load before attaching it, bounded by the existing hydration deadline. - A 401 signs out only when the request carried the current cookie (compare-and-clear). A request sent without one, or with a cookie a newer sign-in has replaced, leaves the login alone. - When the server refuses the session mid-use, the app opens the sign-in screen. A sign-out from Settings does not trigger this. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -24,6 +24,7 @@ import androidx.core.content.ContextCompat
|
|||||||
import androidx.hilt.navigation.compose.hiltViewModel
|
import androidx.hilt.navigation.compose.hiltViewModel
|
||||||
import androidx.lifecycle.compose.collectAsStateWithLifecycle
|
import androidx.lifecycle.compose.collectAsStateWithLifecycle
|
||||||
import androidx.lifecycle.lifecycleScope
|
import androidx.lifecycle.lifecycleScope
|
||||||
|
import androidx.navigation.NavHostController
|
||||||
import androidx.navigation.compose.rememberNavController
|
import androidx.navigation.compose.rememberNavController
|
||||||
import com.fabledsword.minstrel.auth.AuthStore
|
import com.fabledsword.minstrel.auth.AuthStore
|
||||||
import com.fabledsword.minstrel.auth.ui.AuthGateViewModel
|
import com.fabledsword.minstrel.auth.ui.AuthGateViewModel
|
||||||
@@ -33,6 +34,7 @@ import com.fabledsword.minstrel.connectivity.ServerHealth
|
|||||||
import com.fabledsword.minstrel.connectivity.NetworkStatusController
|
import com.fabledsword.minstrel.connectivity.NetworkStatusController
|
||||||
import com.fabledsword.minstrel.nav.DetailSeedCache
|
import com.fabledsword.minstrel.nav.DetailSeedCache
|
||||||
import com.fabledsword.minstrel.nav.LocalDetailSeedCache
|
import com.fabledsword.minstrel.nav.LocalDetailSeedCache
|
||||||
|
import com.fabledsword.minstrel.nav.Login
|
||||||
import com.fabledsword.minstrel.nav.MinstrelNavGraph
|
import com.fabledsword.minstrel.nav.MinstrelNavGraph
|
||||||
import com.fabledsword.minstrel.nav.Notifications
|
import com.fabledsword.minstrel.nav.Notifications
|
||||||
import com.fabledsword.minstrel.nav.NowPlaying
|
import com.fabledsword.minstrel.nav.NowPlaying
|
||||||
@@ -177,6 +179,7 @@ private fun App(
|
|||||||
navController.navigate(route) { launchSingleTop = true }
|
navController.navigate(route) { launchSingleTop = true }
|
||||||
onOpenedRoute()
|
onOpenedRoute()
|
||||||
}
|
}
|
||||||
|
SignInWhenSessionRejected(gate, navController)
|
||||||
MinstrelNavGraph(
|
MinstrelNavGraph(
|
||||||
navController = navController,
|
navController = navController,
|
||||||
startDestination = resolved,
|
startDestination = resolved,
|
||||||
@@ -187,6 +190,24 @@ private fun App(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* When the server ends the session while the app is in use, opens the
|
||||||
|
* sign-in screen rather than leaving the user on screens whose every
|
||||||
|
* request fails.
|
||||||
|
*/
|
||||||
|
@Composable
|
||||||
|
private fun SignInWhenSessionRejected(gate: AuthGateViewModel, navController: NavHostController) {
|
||||||
|
val sessionRejected by gate.sessionRejected.collectAsStateWithLifecycle()
|
||||||
|
LaunchedEffect(sessionRejected, navController) {
|
||||||
|
if (!sessionRejected) return@LaunchedEffect
|
||||||
|
navController.navigate(Login) {
|
||||||
|
popUpTo(0) { inclusive = true }
|
||||||
|
launchSingleTop = true
|
||||||
|
}
|
||||||
|
gate.consumeSessionRejected()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
@Composable
|
@Composable
|
||||||
private fun BootSplash() {
|
private fun BootSplash() {
|
||||||
Surface(
|
Surface(
|
||||||
|
|||||||
@@ -8,9 +8,9 @@ import javax.inject.Singleton
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Attaches the session cookie from [AuthStore] to Minstrel-server
|
* Attaches the session cookie from [AuthStore] to Minstrel-server
|
||||||
* requests, captures Set-Cookie from successful responses (login
|
* requests (waiting for the stored one to load at launch), captures
|
||||||
* flow), and clears the store on 401 so downstream code can react
|
* Set-Cookie from successful responses (login flow), and signs out when
|
||||||
* to logout.
|
* the server refuses the cookie a request carried.
|
||||||
*
|
*
|
||||||
* Scoped to the [BaseUrlInterceptor.PLACEHOLDER_HOST] sentinel host
|
* Scoped to the [BaseUrlInterceptor.PLACEHOLDER_HOST] sentinel host
|
||||||
* the same way BaseUrlInterceptor is. The shared OkHttpClient also
|
* the same way BaseUrlInterceptor is. The shared OkHttpClient also
|
||||||
@@ -32,7 +32,7 @@ class AuthCookieInterceptor @Inject constructor(
|
|||||||
// session. Pass through untouched.
|
// session. Pass through untouched.
|
||||||
return chain.proceed(original)
|
return chain.proceed(original)
|
||||||
}
|
}
|
||||||
val cookie = authStore.sessionCookie.value
|
val cookie = authStore.sessionCookieForRequest()
|
||||||
val request = original.newBuilder().apply {
|
val request = original.newBuilder().apply {
|
||||||
if (!cookie.isNullOrEmpty()) header("Cookie", cookie)
|
if (!cookie.isNullOrEmpty()) header("Cookie", cookie)
|
||||||
}.build()
|
}.build()
|
||||||
@@ -40,7 +40,11 @@ class AuthCookieInterceptor @Inject constructor(
|
|||||||
val response = chain.proceed(request)
|
val response = chain.proceed(request)
|
||||||
|
|
||||||
if (response.code == HTTP_UNAUTHORIZED) {
|
if (response.code == HTTP_UNAUTHORIZED) {
|
||||||
authStore.setSessionCookie(null)
|
// Only a refusal of the cookie this request carried ends the
|
||||||
|
// session. A request sent without one says nothing about the
|
||||||
|
// stored login, and a refusal of an older cookie must not
|
||||||
|
// discard one a sign-in has stored since.
|
||||||
|
if (!cookie.isNullOrEmpty()) authStore.dropRejectedSessionCookie(cookie)
|
||||||
} else if (response.isSuccessful) {
|
} else if (response.isSuccessful) {
|
||||||
// Capture the first Set-Cookie pair on login; subsequent
|
// Capture the first Set-Cookie pair on login; subsequent
|
||||||
// responses that include Set-Cookie keep the store fresh.
|
// responses that include Set-Cookie keep the store fresh.
|
||||||
|
|||||||
@@ -12,6 +12,7 @@ import kotlinx.coroutines.flow.MutableStateFlow
|
|||||||
import kotlinx.coroutines.flow.StateFlow
|
import kotlinx.coroutines.flow.StateFlow
|
||||||
import kotlinx.coroutines.flow.asStateFlow
|
import kotlinx.coroutines.flow.asStateFlow
|
||||||
import kotlinx.coroutines.launch
|
import kotlinx.coroutines.launch
|
||||||
|
import kotlinx.coroutines.runBlocking
|
||||||
import kotlinx.coroutines.sync.Mutex
|
import kotlinx.coroutines.sync.Mutex
|
||||||
import kotlinx.coroutines.sync.withLock
|
import kotlinx.coroutines.sync.withLock
|
||||||
import kotlinx.coroutines.withTimeoutOrNull
|
import kotlinx.coroutines.withTimeoutOrNull
|
||||||
@@ -148,6 +149,49 @@ class AuthStore @Inject constructor(
|
|||||||
scope.launch { cookieLock.withLock { storeCookie(value) } }
|
scope.launch { cookieLock.withLock { storeCookie(value) } }
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The cookie to attach to an outgoing request. Blocks the calling
|
||||||
|
* (OkHttp) thread until the stored cookie has loaded, up to
|
||||||
|
* [HYDRATION_DEADLINE_MS], so a request made in the first moment after
|
||||||
|
* launch carries the login instead of going out without it. Once
|
||||||
|
* hydration has finished this is a plain read.
|
||||||
|
*/
|
||||||
|
fun sessionCookieForRequest(): String? {
|
||||||
|
if (!cookieHydration.isCompleted) {
|
||||||
|
runBlocking { withTimeoutOrNull(HYDRATION_DEADLINE_MS) { cookieHydration.await() } }
|
||||||
|
}
|
||||||
|
return sessionCookieState.value
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* True once the server has refused the current session (see
|
||||||
|
* [dropRejectedSessionCookie]) and the UI has not yet sent the user to
|
||||||
|
* sign in. Held rather than fired, so a refusal that lands while the app
|
||||||
|
* is in the background is still acted on when it comes back. A sign-out
|
||||||
|
* the user asked for does not set it.
|
||||||
|
*/
|
||||||
|
private val sessionRejectedState = MutableStateFlow(false)
|
||||||
|
val sessionRejected: StateFlow<Boolean> = sessionRejectedState.asStateFlow()
|
||||||
|
|
||||||
|
/** The UI has opened the sign-in screen for a refused session. */
|
||||||
|
fun consumeSessionRejected() {
|
||||||
|
sessionRejectedState.value = false
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Signs out because the server answered 401 to a request that carried
|
||||||
|
* [rejected]. Does nothing when the store has since moved on to another
|
||||||
|
* cookie (a sign-in landed meanwhile), so a refusal of an old session
|
||||||
|
* never discards a new one. What is persisted is read inside the lock,
|
||||||
|
* so a sign-in racing this call is the value that ends up stored.
|
||||||
|
*/
|
||||||
|
fun dropRejectedSessionCookie(rejected: String) {
|
||||||
|
if (!sessionCookieState.compareAndSet(rejected, null)) return
|
||||||
|
cookieTouched = true
|
||||||
|
scope.launch { cookieLock.withLock { storeCookie(sessionCookieState.value) } }
|
||||||
|
sessionRejectedState.value = true
|
||||||
|
}
|
||||||
|
|
||||||
private suspend fun hydrateSessionCookie() = cookieLock.withLock {
|
private suspend fun hydrateSessionCookie() = cookieLock.withLock {
|
||||||
val legacy = runCatching { dao.get()?.sessionCookie }.getOrNull()
|
val legacy = runCatching { dao.get()?.sessionCookie }.getOrNull()
|
||||||
if (cookieTouched) return@withLock
|
if (cookieTouched) return@withLock
|
||||||
|
|||||||
@@ -38,6 +38,15 @@ class AuthGateViewModel @Inject constructor(
|
|||||||
private val internal = MutableStateFlow<Any?>(null)
|
private val internal = MutableStateFlow<Any?>(null)
|
||||||
val startDestination: StateFlow<Any?> = internal.asStateFlow()
|
val startDestination: StateFlow<Any?> = internal.asStateFlow()
|
||||||
|
|
||||||
|
/**
|
||||||
|
* True when the server has ended the session while the app was in use;
|
||||||
|
* the root NavHost answers by opening the sign-in screen, then calls
|
||||||
|
* [consumeSessionRejected].
|
||||||
|
*/
|
||||||
|
val sessionRejected: StateFlow<Boolean> = authStore.sessionRejected
|
||||||
|
|
||||||
|
fun consumeSessionRejected() = authStore.consumeSessionRejected()
|
||||||
|
|
||||||
init {
|
init {
|
||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
authStore.awaitSessionHydrated()
|
authStore.awaitSessionHydrated()
|
||||||
|
|||||||
@@ -2,23 +2,31 @@ package com.fabledsword.minstrel.api
|
|||||||
|
|
||||||
import com.fabledsword.minstrel.auth.AuthStore
|
import com.fabledsword.minstrel.auth.AuthStore
|
||||||
import com.fabledsword.minstrel.auth.FakeSessionVault
|
import com.fabledsword.minstrel.auth.FakeSessionVault
|
||||||
|
import com.fabledsword.minstrel.auth.SessionVault
|
||||||
import com.fabledsword.minstrel.cache.db.dao.AuthSessionDao
|
import com.fabledsword.minstrel.cache.db.dao.AuthSessionDao
|
||||||
import io.mockk.coEvery
|
import io.mockk.coEvery
|
||||||
import io.mockk.every
|
import io.mockk.every
|
||||||
import io.mockk.mockk
|
import io.mockk.mockk
|
||||||
|
import kotlinx.coroutines.CoroutineScope
|
||||||
|
import kotlinx.coroutines.Dispatchers
|
||||||
import kotlinx.coroutines.ExperimentalCoroutinesApi
|
import kotlinx.coroutines.ExperimentalCoroutinesApi
|
||||||
|
import kotlinx.coroutines.SupervisorJob
|
||||||
import kotlinx.coroutines.flow.flowOf
|
import kotlinx.coroutines.flow.flowOf
|
||||||
import kotlinx.coroutines.test.TestScope
|
import kotlinx.coroutines.test.TestScope
|
||||||
import kotlinx.coroutines.test.UnconfinedTestDispatcher
|
import kotlinx.coroutines.test.UnconfinedTestDispatcher
|
||||||
import okhttp3.OkHttpClient
|
import okhttp3.OkHttpClient
|
||||||
import okhttp3.Request
|
import okhttp3.Request
|
||||||
|
import okhttp3.mockwebserver.Dispatcher
|
||||||
import okhttp3.mockwebserver.MockResponse
|
import okhttp3.mockwebserver.MockResponse
|
||||||
import okhttp3.mockwebserver.MockWebServer
|
import okhttp3.mockwebserver.MockWebServer
|
||||||
|
import okhttp3.mockwebserver.RecordedRequest
|
||||||
import org.junit.jupiter.api.AfterEach
|
import org.junit.jupiter.api.AfterEach
|
||||||
import org.junit.jupiter.api.BeforeEach
|
import org.junit.jupiter.api.BeforeEach
|
||||||
import org.junit.jupiter.api.Test
|
import org.junit.jupiter.api.Test
|
||||||
import kotlin.test.assertEquals
|
import kotlin.test.assertEquals
|
||||||
|
import kotlin.test.assertFalse
|
||||||
import kotlin.test.assertNull
|
import kotlin.test.assertNull
|
||||||
|
import kotlin.test.assertTrue
|
||||||
|
|
||||||
@OptIn(ExperimentalCoroutinesApi::class)
|
@OptIn(ExperimentalCoroutinesApi::class)
|
||||||
class AuthCookieInterceptorTest {
|
class AuthCookieInterceptorTest {
|
||||||
@@ -92,6 +100,84 @@ class AuthCookieInterceptorTest {
|
|||||||
assertNull(authStore.sessionCookie.value)
|
assertNull(authStore.sessionCookie.value)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `a refused session asks the UI to open sign-in`() {
|
||||||
|
authStore.setSessionCookie("session=expired")
|
||||||
|
server.enqueue(MockResponse().setResponseCode(401))
|
||||||
|
|
||||||
|
client.newCall(Request.Builder().url(placeholderUrl("/api/test")).build()).execute()
|
||||||
|
|
||||||
|
assertTrue(authStore.sessionRejected.value)
|
||||||
|
}
|
||||||
|
|
||||||
|
// A refusal of a request that carried no cookie says nothing about the
|
||||||
|
// stored login, and must not raise the sign-in screen either.
|
||||||
|
@Test
|
||||||
|
fun `a 401 to a request sent without a cookie signs nobody out`() {
|
||||||
|
server.enqueue(MockResponse().setResponseCode(401))
|
||||||
|
|
||||||
|
client.newCall(Request.Builder().url(placeholderUrl("/api/test")).build()).execute()
|
||||||
|
|
||||||
|
assertFalse(authStore.sessionRejected.value)
|
||||||
|
}
|
||||||
|
|
||||||
|
// The interceptor reads the cookie when the request starts. A sign-in
|
||||||
|
// that lands while an old request is in flight must survive that old
|
||||||
|
// request's 401.
|
||||||
|
@Test
|
||||||
|
fun `a 401 for an older cookie keeps the one stored since`() {
|
||||||
|
authStore.setSessionCookie("session=old")
|
||||||
|
server.dispatcher =
|
||||||
|
object : Dispatcher() {
|
||||||
|
override fun dispatch(request: RecordedRequest): MockResponse {
|
||||||
|
authStore.setSessionCookie("session=new")
|
||||||
|
return MockResponse().setResponseCode(401)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
client.newCall(Request.Builder().url(placeholderUrl("/api/test")).build()).execute()
|
||||||
|
|
||||||
|
assertEquals("session=new", authStore.sessionCookie.value)
|
||||||
|
assertFalse(authStore.sessionRejected.value)
|
||||||
|
}
|
||||||
|
|
||||||
|
// At launch the stored cookie loads from the Keystore off the main
|
||||||
|
// thread. A request made before that finishes waits for it rather than
|
||||||
|
// going out without the login.
|
||||||
|
@Test
|
||||||
|
fun `a request made while the stored cookie is still loading carries it`() {
|
||||||
|
val slowVault =
|
||||||
|
object : SessionVault {
|
||||||
|
override fun read(): String? {
|
||||||
|
Thread.sleep(SLOW_VAULT_MS)
|
||||||
|
return "session=stored"
|
||||||
|
}
|
||||||
|
|
||||||
|
override fun write(value: String?): Boolean = true
|
||||||
|
}
|
||||||
|
val dao =
|
||||||
|
mockk<AuthSessionDao> {
|
||||||
|
every { observe() } returns flowOf(null)
|
||||||
|
coEvery { get() } returns null
|
||||||
|
coEvery { upsert(any()) } returns Unit
|
||||||
|
coEvery { setSessionCookie(any()) } returns Unit
|
||||||
|
coEvery { setBaseUrl(any()) } returns Unit
|
||||||
|
}
|
||||||
|
val loading = AuthStore(dao, slowVault, CoroutineScope(SupervisorJob() + Dispatchers.Default))
|
||||||
|
loading.setBaseUrl(server.url("/").toString())
|
||||||
|
val launchClient =
|
||||||
|
OkHttpClient.Builder()
|
||||||
|
.addInterceptor(AuthCookieInterceptor(loading))
|
||||||
|
.addInterceptor(BaseUrlInterceptor(loading))
|
||||||
|
.build()
|
||||||
|
server.enqueue(MockResponse().setResponseCode(200))
|
||||||
|
|
||||||
|
launchClient.newCall(Request.Builder().url(placeholderUrl("/api/library/sync")).build()).execute()
|
||||||
|
|
||||||
|
assertEquals("session=stored", server.takeRequest().getHeader("Cookie"))
|
||||||
|
assertEquals("session=stored", loading.sessionCookie.value)
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun `captures Set-Cookie on successful response`() {
|
fun `captures Set-Cookie on successful response`() {
|
||||||
server.enqueue(
|
server.enqueue(
|
||||||
@@ -145,4 +231,8 @@ class AuthCookieInterceptorTest {
|
|||||||
|
|
||||||
assertEquals("session=keep", authStore.sessionCookie.value)
|
assertEquals("session=keep", authStore.sessionCookie.value)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private companion object {
|
||||||
|
const val SLOW_VAULT_MS = 300L
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user