From 36cde175e72099be1c0cf20bf5a76f5c549d37ca Mon Sep 17 00:00:00 2001 From: rxwatcher Date: Wed, 5 Aug 2026 19:50:29 +0200 Subject: [PATCH 1/6] fix(profiles): commit profile id and token as one identity Audit of the profile/PIN path found the client could claim one profile while holding another's proof, and could act on a verification the user had already abandoned. - selectProfile now writes id and token together. Previously it set only the id, so phone carried the PREVIOUS profile's token into the new selection and every request went out as X-Profile-Id: B with A's X-Profile-Token. TV escaped it only because its switch path happened to call clearProfile() first. Both real token managers do this in one lock and one preferences edit, so a crash cannot persist a mismatch. - verifyPin no longer touches TokenManager. It used to persist the token into whatever server slot was active when the response landed, so cancelling mid-flight or switching servers could install one server's profile token as another's. It is now a pure query and the caller commits the answer. - Both selection ViewModels gained a generation guard around the verify round trip. Cancel during verification still entered the profile. - Verification is gated on an issued token, not a bare valid=true, so the client cannot enter a protected profile holding nothing to present. - Profile commits are refused outright while a remote-playback overlay owns identity, and the managers refuse to merge into an overlay. - Profile list responses are dropped if the identity they were fetched under has been replaced; a stale grid let the user pick a profile from a session the app no longer holds. - Phone kept the raw PIN in rememberSaveable, which the OS serializes across process death. Now plain remember. Server side was verified against the running prod revision 8bde6f11 (== upstream/main HEAD): canManageHousehold requires admin or an active primary profile, plus a valid X-Profile-Token when that primary has a PIN, so the PIN boundary is enforced server-side and none of the above was a privilege escalation. Known and deliberately left: paired READS still take the lock separately (AuthInterceptorImpl, MediaAuthSession, PlaybackRealtimeClient), the ServerRegistry second durable write, TV profile-picker sign-out not clearing saved credentials during an overlay, and no picker reload on TEMPORARY_SCOPE_END. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW --- .../ui/screens/profiles/PINEntryDialog.kt | 12 +- .../profiles/ProfileSelectionViewModel.kt | 77 +++++- .../profiles/TvProfileSelectionViewModel.kt | 78 +++++- .../silo/network/EncryptedTokenManagerImpl.kt | 31 +++ .../silo/model/profile/ProfileModels.kt | 17 ++ .../siloserver/silo/network/TokenManager.kt | 21 ++ .../silo/network/TokenManagerImpl.kt | 11 + .../silo/repository/ProfileRepository.kt | 156 ++++++++++- .../repository/ProfileIdentityCommitTest.kt | 257 ++++++++++++++++++ 9 files changed, 629 insertions(+), 31 deletions(-) create mode 100644 shared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/PINEntryDialog.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/PINEntryDialog.kt index 8d0e1751e..705e49664 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/PINEntryDialog.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/PINEntryDialog.kt @@ -23,7 +23,7 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf -import androidx.compose.runtime.saveable.rememberSaveable +import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -57,7 +57,11 @@ fun PINEntryDialog( onPinComplete: (String) -> Unit, onDismiss: () -> Unit, ) { - var pin by rememberSaveable { mutableStateOf("") } + // Deliberately NOT rememberSaveable: saved-instance state is serialized by + // the OS across configuration change and process death, which would put the + // raw PIN in system-managed storage well beyond the request that needs it. + // Losing four digits on rotation is the correct trade. + var pin by remember { mutableStateOf("") } // Clear pin on new error so the user can re-enter. LaunchedEffect(error) { @@ -131,6 +135,10 @@ fun PINEntryDialog( Spacer(modifier = Modifier.height(32.dp)) + // Cancel stays live during verification (the user must be able + // to back out of a slow round trip), and the ViewModel's + // generation guard makes that abandon the in-flight answer + // rather than commit it late. TextButton(onClick = onDismiss) { Text( text = "Cancel", diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt index 5d3886df2..362a4288f 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt @@ -3,7 +3,10 @@ package org.siloserver.silo.android.ui.screens.profiles import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import org.siloserver.silo.model.profile.Profile +import org.siloserver.silo.model.profile.authorizedProfileToken import org.siloserver.silo.network.ApiResult +import org.siloserver.silo.network.AuthScopeSnapshot +import org.siloserver.silo.repository.ProfileCommitResult import org.siloserver.silo.repository.ProfileRepository import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -36,6 +39,12 @@ class ProfileSelectionViewModel( private val _uiState = MutableStateFlow(ProfileSelectionUiState()) val uiState: StateFlow = _uiState.asStateFlow() + /** Monotonic generation for PIN verification; see [onPinEntered]. */ + private var pinAttempt: Int = 0 + + /** Monotonic generation for profile-list loads; see [loadProfiles]. */ + private var loadAttempt: Int = 0 + init { loadProfiles() } @@ -46,11 +55,24 @@ class ProfileSelectionViewModel( * would otherwise silently swallow it. */ fun loadProfiles(clearError: Boolean = true) { + val load = ++loadAttempt viewModelScope.launch { _uiState.update { it.copy(isLoading = true, error = if (clearError) null else it.error) } + val scope = profileRepository.captureIdentityScope() val activeId = profileRepository.getActiveProfileId() - when (val result = profileRepository.listProfiles()) { + val result = profileRepository.listProfiles() + // Two separate reasons to drop this response: a newer load + // superseded it, or the identity it was fetched under is gone. + // The second is the one that matters — a stale grid lets the user + // pick a profile from a session the app no longer holds. + if (load != loadAttempt) return@launch + if (!profileRepository.identityScopeUnchanged(scope)) { + _uiState.update { it.copy(isLoading = false, profiles = emptyList()) } + return@launch + } + + when (result) { is ApiResult.Success -> { _uiState.update { it.copy(isLoading = false, profiles = result.data, activeProfileId = activeId) @@ -90,6 +112,11 @@ class ProfileSelectionViewModel( return } + // Bump before branching: ANY accepted selection supersedes a + // verification still in flight, including picking an unprotected + // profile while a protected one is mid-verify. + pinAttempt++ + if (profile.hasPin) { _uiState.update { it.copy( @@ -108,15 +135,27 @@ class ProfileSelectionViewModel( */ fun onPinEntered(pin: String) { val profile = _uiState.value.pinDialogProfile ?: return + val attempt = ++pinAttempt viewModelScope.launch { _uiState.update { it.copy(pinIsVerifying = true, pinError = null) } - when (val result = profileRepository.verifyPin(profile.id, pin)) { + // Pin the answer to the identity that was asked, not just to the + // dialog target: the active scope can move underneath us. + val scope = profileRepository.captureIdentityScope() + val result = profileRepository.verifyPin(profile.id, pin) + // The user can cancel (or tap a different profile) while the round + // trip is in flight. Intent proven before a suspension point is not + // intent after it, so re-check ownership before acting: committing + // unconditionally meant Cancel still entered the profile. + if (attempt != pinAttempt) return@launch + + when (result) { is ApiResult.Success -> { - if (result.data.valid) { + val token = result.data.authorizedProfileToken() + if (token != null) { _uiState.update { it.copy(pinIsVerifying = false, pinDialogProfile = null) } - selectProfile(profile.id) + selectProfile(profile.id, token, scope) } else { _uiState.update { it.copy(pinIsVerifying = false, pinError = "Incorrect PIN") @@ -143,6 +182,9 @@ class ProfileSelectionViewModel( } fun dismissPinDialog() { + // Bump the generation so an in-flight verification for the dismissed + // profile can no longer commit. + pinAttempt++ _uiState.update { it.copy(pinDialogProfile = null, pinIsVerifying = false, pinError = null) } @@ -194,9 +236,32 @@ class ProfileSelectionViewModel( _uiState.update { it.copy(selectedProfileId = null) } } - private fun selectProfile(profileId: String) { + private fun selectProfile( + profileId: String, + profileToken: String? = null, + expectedScope: AuthScopeSnapshot? = null, + ) { viewModelScope.launch { - profileRepository.selectProfile(profileId) + val result = profileRepository.selectProfile(profileId, profileToken, expectedScope) + if (result == ProfileCommitResult.ScopeChanged) { + // Someone else owns the identity now. Drop everything bound to + // the identity we no longer have — a retained grid would let + // the user pick a profile belonging to the previous session, + // and that commit carries no scope to reject it. + _uiState.update { + it.copy( + profiles = emptyList(), + activeProfileId = null, + selectedProfileId = null, + pinDialogProfile = null, + pinIsVerifying = false, + pinError = null, + deleteDialogProfile = null, + ) + } + loadProfiles() + return@launch + } _uiState.update { it.copy(selectedProfileId = profileId) } } } diff --git a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt index f3b5d9c34..b02018ff8 100644 --- a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt +++ b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt @@ -3,7 +3,10 @@ package org.siloserver.silo.tv.ui.screens.profiles import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import org.siloserver.silo.model.profile.Profile +import org.siloserver.silo.model.profile.authorizedProfileToken import org.siloserver.silo.network.ApiResult +import org.siloserver.silo.network.AuthScopeSnapshot +import org.siloserver.silo.repository.ProfileCommitResult import org.siloserver.silo.repository.ProfileRepository import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -39,14 +42,30 @@ class TvProfileSelectionViewModel( private val _uiState = MutableStateFlow(TvProfileSelectionUiState()) val uiState: StateFlow = _uiState.asStateFlow() + /** Monotonic generation for PIN verification; see [onPinEntered]. */ + private var pinAttempt: Int = 0 + + /** Monotonic generation for profile-list loads; see [loadProfiles]. */ + private var loadAttempt: Int = 0 + init { loadProfiles() } fun loadProfiles() { + val load = ++loadAttempt viewModelScope.launch { _uiState.update { it.copy(isLoading = true, error = null) } - when (val result = profileRepository.listProfiles()) { + val scope = profileRepository.captureIdentityScope() + val listed = profileRepository.listProfiles() + // Two separate reasons to drop this response: a newer load + // superseded it, or the identity it was fetched under is gone. + if (load != loadAttempt) return@launch + if (!profileRepository.identityScopeUnchanged(scope)) { + _uiState.update { it.copy(isLoading = false, profiles = emptyList()) } + return@launch + } + when (val result = listed) { is ApiResult.Success -> { _uiState.update { it.copy(isLoading = false, profiles = result.data) @@ -89,6 +108,11 @@ class TvProfileSelectionViewModel( // Manage-mode taps open edit, handled by the screen composable. return } + // Bump before branching: ANY accepted selection supersedes a + // verification still in flight, including choosing an unprotected + // profile while a protected one is mid-verify. + pinAttempt++ + if (profile.hasPin) { // Open the PIN dialog; actual selection happens in onPinEntered. _uiState.update { @@ -100,6 +124,8 @@ class TvProfileSelectionViewModel( } fun onPinDialogDismissed() { + // Abandon any in-flight verification for the dismissed profile. + pinAttempt++ _uiState.update { it.copy(pinProfile = null, pinError = null, isVerifyingPin = false) } @@ -107,16 +133,28 @@ class TvProfileSelectionViewModel( fun onPinEntered(pin: String) { val profile = _uiState.value.pinProfile ?: return + val attempt = ++pinAttempt _uiState.update { it.copy(isVerifyingPin = true, pinError = null) } viewModelScope.launch { - when (val r = profileRepository.verifyPin(profile.id, pin)) { + // Pin the answer to the identity that was asked. TV can install a + // temporary remote-playback identity mid-flight, and this profile's + // proof must never land in that overlay. + val scope = profileRepository.captureIdentityScope() + val r = profileRepository.verifyPin(profile.id, pin) + // Cancelling (or picking another profile) during the round trip + // must abandon this answer — otherwise Back still entered the + // profile, and on TV a "Change Server" mid-flight could land this + // profile's token on a different server. + if (attempt != pinAttempt) return@launch + + when (r) { is ApiResult.Success -> { // The server returns 200 with valid=false for a wrong PIN, so - // gate selection on .valid (matches phone) — never commit on a - // bare 200. - if (r.data.valid) { - // Repository stores the profile token and active profile. - commitSelection(profile) + // gate on the issued token (matches phone) — never commit on + // a bare 200, nor on a valid=true carrying no proof. + val token = r.data.authorizedProfileToken() + if (token != null) { + commitSelection(profile, token, scope) _uiState.update { it.copy(pinProfile = null, isVerifyingPin = false) } } else { _uiState.update { it.copy(isVerifyingPin = false, pinError = "Incorrect PIN") } @@ -138,9 +176,31 @@ class TvProfileSelectionViewModel( } } - private fun commitSelection(profile: Profile) { + private fun commitSelection( + profile: Profile, + profileToken: String? = null, + expectedScope: AuthScopeSnapshot? = null, + ) { viewModelScope.launch { - profileRepository.selectProfile(profile.id) + val result = profileRepository.selectProfile(profile.id, profileToken, expectedScope) + if (result == ProfileCommitResult.ScopeChanged) { + // Identity moved under us — don't route into this profile, and + // drop the grid with it. A retained grid keeps D-pad focus on + // profiles belonging to a session we no longer hold. + _uiState.update { + it.copy( + profiles = emptyList(), + selectedProfileId = null, + pinProfile = null, + isVerifyingPin = false, + pinError = null, + deleteCandidate = null, + isManageMode = false, + ) + } + loadProfiles() + return@launch + } _uiState.update { it.copy(selectedProfileId = profile.id) } } } diff --git a/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt b/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt index 3c08d4996..5e38fc56d 100644 --- a/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt +++ b/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt @@ -250,6 +250,37 @@ class EncryptedTokenManagerImpl( } } + /** + * One lock, one preferences edit, so the PERSISTED id and token cannot + * disagree even if the process dies immediately after. Concurrent readers + * are a separate problem — see [TokenManager.setProfileIdentity]. + * + * While a temporary overlay exists this refuses the write entirely rather + * than merging into it: remote-playback identity belongs to the overlay, + * and a partial merge is what produced the id/token mismatch in the first + * place. + */ + override suspend fun setProfileIdentity(profileId: String?, profileToken: String?) { + mutex.withLock { + // A temporary overlay owns its own identity for the lifetime of a + // remote-playback handoff. Merging a profile commit into it is how + // you get the exact defect this method exists to prevent: writing + // the new profile id beside the overlay's old token. Leave it + // alone; the repository rejects the commit outright. + if (temporaryScope != null) return@withLock + val serverId = activeServerId ?: return + if (this.profileId == profileId && this.profileToken == profileToken) return + this.profileId = profileId + this.profileToken = profileToken + val idKey = serverScopedKey(serverId, KEY_PROFILE_ID) + val tokenKey = serverScopedKey(serverId, KEY_PROFILE_TOKEN) + prefs.edit().apply { + if (profileId == null) remove(idKey) else putString(idKey, profileId) + if (profileToken == null) remove(tokenKey) else putString(tokenKey, profileToken) + }.apply() + } + } + override suspend fun getServerUrl(): String = mutex.withLock { temporaryScope?.serverUrl ?: registry.activeEntry.value?.url.orEmpty() } diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/model/profile/ProfileModels.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/model/profile/ProfileModels.kt index 9a1168675..7f20fc5a8 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/model/profile/ProfileModels.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/model/profile/ProfileModels.kt @@ -93,3 +93,20 @@ data class VerifyPinResponse( @SerialName("profile_token") val profileToken: String? = null, @SerialName("expires_at") val expiresAt: String? = null ) + +/** + * The profile token to commit for a successful PIN verification, or null if + * this response does not authorize entry. + * + * Fail closed on shape, not just on [VerifyPinResponse.valid]: the token is + * the artifact that proves the PIN was entered, and the server rejects + * management calls that cannot present one. Treating a bare `valid=true` with + * no token as success let the client enter a protected profile holding nothing + * to prove it — the failure then surfaced much later, as a confusing 403 on an + * unrelated action. + * + * Expiry is left to the server, which validates the token on every use; the + * client does not parse [VerifyPinResponse.expiresAt]. + */ +fun VerifyPinResponse.authorizedProfileToken(): String? = + profileToken?.takeIf { valid && it.isNotBlank() } diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt index ed8de9456..0551248b3 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt @@ -75,6 +75,27 @@ interface TokenManager { suspend fun setProfileId(profileId: String?) suspend fun getProfileToken(): String? suspend fun setProfileToken(token: String?) + + /** + * Commit a profile id and its matching profile token as ONE identity. + * + * A profile token is bound server-side to a single profile id, so the two + * are one fact, not two. Writing them separately means a process death + * between the writes persists a mismatch that survives to the next launch. + * + * This makes the WRITE one operation. It does not make concurrent reads + * consistent: [getProfileId] and [getProfileToken] still take the lock + * separately, so a reader interleaving with a commit can pair an old id + * with a new token. + * + * The default is the non-atomic pair, which keeps simple/test managers + * working; managers with real durable storage override this to do it in a + * single lock and a single edit. + */ + suspend fun setProfileIdentity(profileId: String?, profileToken: String?) { + setProfileId(profileId) + setProfileToken(profileToken) + } suspend fun getServerUrl(): String suspend fun setServerUrl(url: String) diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.kt index f7185a4ce..072bbdb75 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.kt @@ -135,6 +135,17 @@ class TokenManagerImpl( } } + /** Single lock so the stored pair is written together; see [TokenManager]. */ + override suspend fun setProfileIdentity(profileId: String?, profileToken: String?) { + mutex.withLock { + // See EncryptedTokenManagerImpl: an overlay owns its identity, and + // merging a commit into it recreates the id/token mismatch. + if (temporaryScope != null) return@withLock + this.profileId = profileId + this.profileToken = profileToken + } + } + override suspend fun getServerUrl(): String = mutex.withLock { temporaryScope?.serverUrl ?: serverUrl } diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.kt index a390acbb3..06b45a485 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/repository/ProfileRepository.kt @@ -5,6 +5,7 @@ import org.siloserver.silo.model.profile.Profile import org.siloserver.silo.model.profile.UpdateProfileRequest import org.siloserver.silo.model.profile.VerifyPinResponse import org.siloserver.silo.network.ApiResult +import org.siloserver.silo.network.AuthScopeSnapshot import org.siloserver.silo.network.DefaultIdentityTransitionBarrier import org.siloserver.silo.network.IdentityTransitionBarrier import org.siloserver.silo.network.IdentityTransitionKind @@ -13,6 +14,19 @@ import org.siloserver.silo.network.TokenManager import org.siloserver.silo.network.api.ProfileApi import org.siloserver.silo.network.map +/** Outcome of a scope-guarded profile commit. */ +enum class ProfileCommitResult { + /** Identity was written. */ + Committed, + + /** + * The commit was refused because this identity is not ours to write: + * either the scope moved between capture and commit, or a temporary + * remote-playback overlay owns identity right now. Nothing was written. + */ + ScopeChanged, +} + open class ProfileRepository( private val profileApi: ProfileApi, private val tokenManager: TokenManager, @@ -49,17 +63,17 @@ open class ProfileRepository( /** * Verifies a profile's PIN. - * On success, persists the profile token via [TokenManager]. + * + * This deliberately does NOT touch [TokenManager]. Verification is a + * question, not a commitment: the answer is only worth acting on if the + * caller still wants this profile when it arrives. Persisting the token + * here wrote it into whatever server slot happened to be active by then, + * so cancelling mid-flight (or switching servers) could install one + * server's profile token as another's. Callers commit the result through + * [selectProfile], which binds id and token together in one transition. */ - suspend fun verifyPin(profileId: String, pin: String): ApiResult { - val result = profileApi.verifyPin(profileId, pin) - if (result is ApiResult.Success) { - result.data.profileToken?.let { token -> - tokenManager.setProfileToken(token) - } - } - return result - } + suspend fun verifyPin(profileId: String, pin: String): ApiResult = + profileApi.verifyPin(profileId, pin) /** * Selects a profile as the active profile. @@ -67,18 +81,133 @@ open class ProfileRepository( * Persists the profile id on the active [TokenManager] slot AND on the * matching [ServerRegistry] entry — the latter is what restores the * "last used profile" when the user hops back to this server. + * + * [profileToken] is the artifact `verify-pin` just issued for *this* + * profile, or null for an unprotected one. Id and token are written as one + * stored identity: a profile token is bound server-side to a single profile + * id, and carrying the previous profile's token into the new selection made + * every request claim one profile while presenting another's proof. Phone + * hit that on the ordinary protected-A → unprotected-B switch (TV cleared + * first, so only one client was wrong). + * + * Scope note: the WRITE is atomic and so is what survives a crash, but + * readers still fetch id and token through separate calls + * ([TokenManager.getProfileId] / [TokenManager.getProfileToken]), so a + * request assembled exactly across a switch can still pair an old id with a + * new token. Closing that needs a combined accessor and a migration of + * every paired reader. */ - suspend fun selectProfile(profileId: String) { + suspend fun selectProfile( + profileId: String, + profileToken: String? = null, + expectedScope: AuthScopeSnapshot? = null, + ): ProfileCommitResult { + var result = ProfileCommitResult.Committed + // Read the barrier generation before entering the transition. `changing` + // runs its gates, then bumps the generation, then runs this block — so + // by the time we are inside, the live generation is already this + // transition's own. Comparing the captured scope against it directly + // would report "changed" on every single selection. + val generationBeforeTransition = identityTransitions.generation.value identityTransitions.changing(IdentityTransitionKind.PROFILE_SWITCH) { - tokenManager.setProfileId(profileId) + // Re-check INSIDE the transition: a scope captured before the PIN + // round trip proves nothing unless it still holds at the moment of + // the write. Remote playback can install a temporary identity + // mid-flight, and committing there would put this server's profile + // proof into an overlay that belongs to a different session. + // A remote-playback overlay owns identity while it exists, and it + // is not this user's session to repoint. This check is deliberately + // independent of [expectedScope]: an unprotected selection carries + // no scope to compare, and an already-dispatched tap can land after + // the overlay installs. + if (tokenManager.hasTemporaryScope()) { + result = ProfileCommitResult.ScopeChanged + return@changing + } + if (!identityScopeStillHolds(expectedScope, generationBeforeTransition)) { + result = ProfileCommitResult.ScopeChanged + return@changing + } + tokenManager.setProfileIdentity(profileId, profileToken) val activeServerId = tokenManager.getCurrentServerId() if (activeServerId != null) { + // Known, accepted window: this is a SECOND durable edit, so a + // process death between it and the identity write above leaves + // the registry naming the old profile while the token manager + // (and therefore every request header) names the new one. + // Startup prefers the registry, so the next launch can look + // like the old profile while authenticating as the new one. + // Not the same class as the id/token mismatch fixed above — + // that one sent mismatched credentials on every request — but + // closing it means making one of the two authoritative. serverRegistry?.setProfileId(activeServerId, profileId) } notificationsRepository?.reset() requestsRepository?.reset() _profileSwitches.tryEmit(Unit) } + return result + } + + /** + * Capture the identity scope a PIN verification is about to be asked + * against, for later hand-off to [selectProfile]. Null when the manager + * does not model scopes, which keeps the guard inert rather than failing + * closed on something it never recorded. + */ + suspend fun captureIdentityScope(): AuthScopeSnapshot? = + tokenManager.snapshotCurrentScope() + + /** + * Whether [expected] is still the live identity, for callers that are not + * inside an identity transition (so no generation offset applies). + * + * Used to discard a profile list fetched under an identity that has since + * been replaced — a stale grid lets the user pick a profile belonging to a + * session the app no longer holds. + */ + suspend fun identityScopeUnchanged(expected: AuthScopeSnapshot?): Boolean { + if (expected == null) return true + val current = tokenManager.snapshotCurrentScope() ?: return false + return current.serverId == expected.serverId && + current.identityGeneration == expected.identityGeneration && + current.credentialEpoch == expected.credentialEpoch + } + + /** + * Whether the identity active *now* is still the one [expected] was + * captured from. + * + * [generationBeforeTransition] is the barrier generation read immediately + * before the enclosing `changing` block, which bumps it on entry. So: + * + * - `expected.identityGeneration == generationBeforeTransition` says + * nothing moved between capturing the scope and starting this commit; + * - `current.identityGeneration == generationBeforeTransition + 1` says + * the only transition since is this one, so nobody slipped in while we + * were waiting on the barrier's mutex. + * + * A null [expected] means the caller never captured a scope (or the manager + * does not model them), so the guard stays inert rather than failing closed + * on information it never recorded. But once a scope WAS captured, a + * missing current scope means it is gone, not unsupported — that fails + * closed. + * + * Deliberately a repository function rather than a `TokenManager` default + * method: a default that calls another overridable member runs against the + * delegate under interface delegation, so wrappers would silently get the + * base behaviour. + */ + private suspend fun identityScopeStillHolds( + expected: AuthScopeSnapshot?, + generationBeforeTransition: Long, + ): Boolean { + if (expected == null) return true + if (expected.identityGeneration != generationBeforeTransition) return false + val current = tokenManager.snapshotCurrentScope() ?: return false + return current.serverId == expected.serverId && + current.identityGeneration == generationBeforeTransition + 1 && + current.credentialEpoch == expected.credentialEpoch } private val _profileSwitches = kotlinx.coroutines.flow.MutableSharedFlow(extraBufferCapacity = 1) @@ -136,8 +265,7 @@ open class ProfileRepository( suspend fun clearProfile() { identityTransitions.changing(IdentityTransitionKind.PROFILE_SWITCH) { val activeServerId = tokenManager.getCurrentServerId() - tokenManager.setProfileId(null) - tokenManager.setProfileToken(null) + tokenManager.setProfileIdentity(null, null) if (activeServerId != null) { serverRegistry?.setProfileId(activeServerId, null) } diff --git a/shared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt b/shared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt new file mode 100644 index 000000000..2c1aa0d37 --- /dev/null +++ b/shared/src/commonTest/kotlin/org/siloserver/silo/repository/ProfileIdentityCommitTest.kt @@ -0,0 +1,257 @@ +package org.siloserver.silo.repository + +import org.siloserver.silo.model.profile.VerifyPinResponse +import org.siloserver.silo.model.profile.authorizedProfileToken +import org.siloserver.silo.network.AuthScopeSnapshot +import org.siloserver.silo.network.DefaultIdentityTransitionBarrier +import org.siloserver.silo.network.IdentityTransitionBarrier +import org.siloserver.silo.network.IdentityTransitionKind +import org.siloserver.silo.network.TokenManagerImpl +import org.siloserver.silo.network.api.ProfileApi +import io.ktor.client.HttpClient +import io.ktor.client.engine.mock.MockEngine +import io.ktor.client.engine.mock.respond +import io.ktor.http.HttpStatusCode +import io.ktor.http.headersOf +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.runTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * The profile id and the profile token are one identity. These cover the ways + * they used to come apart — the client would claim one profile while holding + * another's proof, or commit an answer that arrived after the user had moved on. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class ProfileIdentityCommitTest { + + private val noOpClient = HttpClient(MockEngine { _ -> + respond(content = "{}", status = HttpStatusCode.OK, headers = headersOf("Content-Type", "application/json")) + }) + + private fun repository( + tokenManager: org.siloserver.silo.network.TokenManager, + barrier: IdentityTransitionBarrier = DefaultIdentityTransitionBarrier(), + ) = ProfileRepository( + profileApi = ProfileApi(noOpClient), + tokenManager = tokenManager, + identityTransitions = barrier, + ) + + /** + * The deterministic phone bug: switching from a PIN-protected profile to an + * unprotected one left the protected profile's token in place, so requests + * went out as `X-Profile-Id: B` with A's `X-Profile-Token`. + */ + @Test + fun `selecting an unprotected profile drops the previous profile's token`() = runTest { + val tokenManager = TokenManagerImpl() + val repo = repository(tokenManager) + + repo.selectProfile(profileId = "profile-a", profileToken = "token-for-a") + assertEquals("token-for-a", tokenManager.getProfileToken()) + + repo.selectProfile(profileId = "profile-b", profileToken = null) + + assertEquals("profile-b", tokenManager.getProfileId()) + assertNull( + tokenManager.getProfileToken(), + "profile B must not inherit profile A's proof", + ) + } + + @Test + fun `selecting a protected profile commits that profile's own token`() = runTest { + val tokenManager = TokenManagerImpl() + val repo = repository(tokenManager) + + repo.selectProfile(profileId = "profile-a", profileToken = "token-for-a") + repo.selectProfile(profileId = "profile-b", profileToken = "token-for-b") + + assertEquals("profile-b", tokenManager.getProfileId()) + assertEquals("token-for-b", tokenManager.getProfileToken()) + } + + /** + * A token manager that models identity scopes, like the production + * (encrypted) one does. Plain [TokenManagerImpl] reports no scope at all, + * which deliberately leaves the guard inert — so it cannot exercise this. + */ + private class ScopedTokenManager( + private val barrier: IdentityTransitionBarrier, + private val delegate: TokenManagerImpl = TokenManagerImpl(), + ) : org.siloserver.silo.network.TokenManager by delegate { + // Reads the SAME barrier the repository commits through, so the + // generation moves exactly as it does in production. A double with a + // hand-set generation hid a real bug: `changing` bumps the generation + // on entry, so an in-block comparison against the captured value + // reported "changed" for every ordinary selection. + override suspend fun snapshotCurrentScope() = AuthScopeSnapshot( + serverId = "server-1", + profileId = delegate.getProfileId(), + serverUrl = "https://one.example", + profileToken = delegate.getProfileToken(), + identityGeneration = barrier.generation.value, + ) + } + + /** + * A verification captured against one identity must not be applied to + * whoever is active by the time it lands — the remote-playback overlay + * case, where committing would put this profile's proof in someone + * else's session. + */ + @Test + fun `a commit whose scope moved is discarded`() = runTest { + val barrier = DefaultIdentityTransitionBarrier() + val tokenManager = ScopedTokenManager(barrier) + val repo = repository(tokenManager, barrier) + repo.selectProfile(profileId = "profile-a", profileToken = "token-for-a") + + val captured = repo.captureIdentityScope() + // Something else moves the identity while verification is in flight — + // a remote-playback overlay, a server switch, a sign-out. + barrier.changing(IdentityTransitionKind.PROFILE_SWITCH) { } + + val result = repo.selectProfile( + profileId = "profile-x", + profileToken = "token-for-x", + expectedScope = captured, + ) + + assertEquals(ProfileCommitResult.ScopeChanged, result) + assertEquals("profile-a", tokenManager.getProfileId(), "identity must be untouched") + assertEquals("token-for-a", tokenManager.getProfileToken()) + } + + /** Same manager, unmoved scope: the ordinary path must still commit. */ + @Test + fun `a commit whose scope held is applied`() = runTest { + val barrier = DefaultIdentityTransitionBarrier() + val tokenManager = ScopedTokenManager(barrier) + val repo = repository(tokenManager, barrier) + + val captured = repo.captureIdentityScope() + val result = repo.selectProfile( + profileId = "profile-b", + profileToken = "token-for-b", + expectedScope = captured, + ) + + assertEquals(ProfileCommitResult.Committed, result) + assertEquals("profile-b", tokenManager.getProfileId()) + assertEquals("token-for-b", tokenManager.getProfileToken()) + } + + /** + * A remote-playback overlay owns identity while it exists, and an + * unprotected selection carries no scope to compare — so the repository + * refuses the commit outright rather than relying on the token manager to + * absorb it. Both layers now decline: without the repository check the + * managers would no-op the write but the caller would be told `Committed` + * and would run the downstream side effects (registry write, cache resets, + * navigation) for a switch that never happened. + */ + @Test + fun `no profile commits while a temporary overlay owns identity`() = runTest { + val tokenManager = TokenManagerImpl() + val repo = repository(tokenManager) + // Seed a real persistent identity so we can prove it survives intact. + repo.selectProfile(profileId = "profile-a", profileToken = "token-for-a") + tokenManager.beginTemporaryScope( + org.siloserver.silo.network.TemporaryAuthScope( + generationId = "overlay-1", + serverId = "server-1", + serverUrl = "https://one.example", + accessToken = "overlay-access", + refreshToken = "overlay-refresh", + profileId = "overlay-profile", + profileToken = "overlay-token", + expiresAtEpochMs = Long.MAX_VALUE, + ), + ) + + val result = repo.selectProfile(profileId = "profile-b", profileToken = null) + + assertEquals(ProfileCommitResult.ScopeChanged, result) + // The overlay's own identity is untouched... + assertEquals("overlay-profile", tokenManager.getProfileId()) + assertEquals("overlay-token", tokenManager.getProfileToken()) + + // ...and so is the persistent identity underneath it, which is what + // the user returns to when the handoff ends. + tokenManager.endTemporaryScope() + assertEquals("profile-a", tokenManager.getProfileId()) + assertEquals("token-for-a", tokenManager.getProfileToken()) + } + + /** No captured scope means the guard stays inert rather than failing closed. */ + @Test + fun `a commit with no captured scope still applies`() = runTest { + val tokenManager = TokenManagerImpl() + val repo = repository(tokenManager) + + val result = repo.selectProfile( + profileId = "profile-b", + profileToken = "token-for-b", + expectedScope = null, + ) + + assertEquals(ProfileCommitResult.Committed, result) + assertEquals("profile-b", tokenManager.getProfileId()) + } + + /** + * The profile list is identity-bound. A response fetched under an identity + * that has since been replaced must be dropped, or the grid offers profiles + * from a session the app no longer holds — and an unprotected tap on one of + * those carries no scope to reject it. + */ + @Test + fun `a list fetched under a replaced identity is not accepted`() = runTest { + val barrier = DefaultIdentityTransitionBarrier() + val tokenManager = ScopedTokenManager(barrier) + val repo = repository(tokenManager, barrier) + + val captured = repo.captureIdentityScope() + barrier.changing(IdentityTransitionKind.SIGN_OUT) { } + + assertFalse(repo.identityScopeUnchanged(captured)) + } + + @Test + fun `a list fetched under the current identity is accepted`() = runTest { + val barrier = DefaultIdentityTransitionBarrier() + val tokenManager = ScopedTokenManager(barrier) + val repo = repository(tokenManager, barrier) + + val captured = repo.captureIdentityScope() + + assertTrue(repo.identityScopeUnchanged(captured)) + } + + /** + * `valid` alone was the old gate. A 200 carrying no usable proof let the + * client enter a protected profile holding nothing to present, which + * surfaced much later as a confusing 403 on an unrelated action. + */ + @Test + fun `a verification without a usable token does not authorize`() { + assertNull(VerifyPinResponse(valid = true, profileToken = null).authorizedProfileToken()) + assertNull(VerifyPinResponse(valid = true, profileToken = "").authorizedProfileToken()) + assertNull(VerifyPinResponse(valid = true, profileToken = " ").authorizedProfileToken()) + assertNull(VerifyPinResponse(valid = false, profileToken = "token").authorizedProfileToken()) + } + + @Test + fun `a valid verification authorizes with its token`() { + assertEquals( + "token-for-a", + VerifyPinResponse(valid = true, profileToken = "token-for-a").authorizedProfileToken(), + ) + } +} From 0a1556432abcecbf7bb52043e472f17a0ed46a6a Mon Sep 17 00:00:00 2001 From: rxwatcher Date: Wed, 5 Aug 2026 20:38:28 +0200 Subject: [PATCH 2/6] fix(nav): stop launchSingleTop silently replacing detail and player entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `launchSingleTop` matches the destination NODE, not its arguments, and every item-detail route shares one node. So navigating from detail A to a related item B reused A's entry instead of pushing, and Back from B skipped A entirely. The comment at the call site asserted the opposite. - Item detail now collapses only an exact repeat (same contentId AND season), which is what that comment claimed launchSingleTop did. Applied to related items, series, seasons, person detail, both collection screens, the shell, and the silo://item deep link. Season paging keeps its deliberate popUpTo replace. - Playback navigation is worse under single-top: AndroidX reuses the entry, so its ViewModelStore survives and the previous title's player ViewModel can live on beside the new one. Home Play had no guard at all while detail Play had one, so a fast double Select stacked two players and two sessions. All playback now goes through one helper that suppresses only when the entry THIS request created is still on top, and otherwise takes over the current player rather than stacking. Weaker keys were tried and rejected: the route alone ignores what is on top, and route+contentId collides when another path puts up the same title with different arguments. - Watch Together's player target now shares that bookkeeping instead of single-topping the current player in place, which preserved the entry id and could make a stale record look current. - Cast launch replaces whichever player is on top, not only the video one; it stacked over an audiobook and Back resurrected it. - Content ids are percent-encoded in routes on both clients, and the phone deep-link parser reads rawPath and decodes exactly one segment instead of interpolating an already-decoded value back into a route — silo://item/ abc%3FseasonNumber%3D9 injected a route argument. playerRouteIntentOrNull decodes back so an already-showing player still matches. - Phone route encoding uses java.net rather than android.net.Uri, which is stubbed under plain JVM tests and silently yields "item/null"; that had already broken two existing suites mid-change. - Duplicate-nav guards added to the remaining argument-free destinations, and two dead launchSingleTop options removed (popUpTo is evaluated first, so they could never match). Seven review rounds; five returned NO-GO on the playback suppression key before it was keyed on back-stack entry identity with arrival confirmation. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW --- .../ui/navigation/ContentDeepLinkRoutes.kt | 24 +- .../ui/navigation/ExternalRouteNavigation.kt | 8 + .../silo/android/ui/navigation/Routes.kt | 34 +- .../navigation/ContentDeepLinkEncodingTest.kt | 77 ++++ .../silo/tv/ui/navigation/TvAppNavigation.kt | 344 ++++++++++++++---- .../silo/tv/ui/navigation/TvRoute.kt | 8 +- .../navigation/TvItemDetailNavigationTest.kt | 263 +++++++++++++ .../TvWatchTogetherSurfaceSourceTest.kt | 7 +- 8 files changed, 686 insertions(+), 79 deletions(-) create mode 100644 androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.kt create mode 100644 androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvItemDetailNavigationTest.kt diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkRoutes.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkRoutes.kt index 670f740a0..c3525d0a4 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkRoutes.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkRoutes.kt @@ -24,14 +24,22 @@ internal fun contentDeepLinkRouteOrNull(rawUri: String?): String? { ?: return null if (!uri.scheme.equals("silo", ignoreCase = true)) return null - val contentId = uri.path.orEmpty() + // Read the RAW path and decode exactly one segment. `URI.path` is already + // percent-decoded, so taking it and interpolating the result back into a + // route re-parsed the decoded bytes as route syntax: an id containing an + // encoded `?` or `/` could truncate the id or inject an argument. The + // route constructors below re-encode, so this must hand them the decoded + // id exactly once. + val contentId = uri.rawPath.orEmpty() .trim('/') .substringBefore('/') + .let(::decodePathSegment) + .orEmpty() .trim() return when (uri.host?.lowercase()) { "downloads" -> Route.Downloads.route - "item" -> contentId.takeIf { it.isNotBlank() }?.let { "item/$it" } + "item" -> contentId.takeIf { it.isNotBlank() }?.let { Route.ItemDetail(it).route } "play" -> contentId.takeIf { it.isNotBlank() }?.let { Route.Player( contentId = it, @@ -56,3 +64,15 @@ private fun URI.queryParameter(name: String): String? = rawQuery private fun decodeQueryComponent(value: String): String? = runCatching { URLDecoder.decode(value, StandardCharsets.UTF_8.name()) }.getOrNull() + +/** + * Percent-decoding for a PATH segment. Deliberately not [decodeQueryComponent]: + * `URLDecoder` implements form encoding, where `+` means space — but in a path + * a `+` is a literal plus, so an id containing one would be corrupted. + */ +private fun decodePathSegment(value: String): String? = runCatching { + // Escape `+` first: URLDecoder implements form encoding where `+` means + // space, but in a path a `+` is a literal plus. android.net.Uri.decode + // would do this correctly, but it is stubbed in plain JVM unit tests. + URLDecoder.decode(value.replace("+", "%2B"), StandardCharsets.UTF_8.name()) +}.getOrNull() diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt index 917af1b03..442a1be43 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt @@ -1,5 +1,7 @@ package org.siloserver.silo.android.ui.navigation +import java.net.URLDecoder +import java.nio.charset.StandardCharsets import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.first import org.siloserver.silo.android.ui.screens.player.MobilePlayerRouteIntent @@ -37,10 +39,16 @@ internal fun shouldReplaceCurrentPlayer( /** Parses the canonical in-app player route carried by an external request. */ internal fun playerRouteIntentOrNull(route: String): MobilePlayerRouteIntent? { if (!route.startsWith("player/")) return null + // Route.Player percent-encodes the content id, so decode it back here — + // this value is compared against the live player's target, and an encoded + // id would never match a decoded one, making an already-showing player look + // like a different request and restart it. val contentId = route .substringAfter("player/") .substringBefore('?') .takeIf { it.isNotBlank() } + ?.let { runCatching { URLDecoder.decode(it.replace("+", "%2B"), StandardCharsets.UTF_8.name()) }.getOrNull() } + ?.takeIf { it.isNotBlank() } ?: return null val query = route .substringAfter('?', "") diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt index 4e8d2a5cb..b8fd00f56 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt @@ -1,6 +1,8 @@ package org.siloserver.silo.android.ui.navigation import android.net.Uri +import java.net.URLEncoder +import java.nio.charset.StandardCharsets import org.siloserver.silo.common.player.video.VideoPlayerRouteArgs /** @@ -112,7 +114,11 @@ sealed class Route(val route: String) { val contentId: String, val seasonNumber: Int? = null, ) : Route( - if (seasonNumber != null) "item/$contentId?seasonNumber=$seasonNumber" else "item/$contentId" + if (seasonNumber != null) { + "item/${contentId.routeEncode()}?seasonNumber=$seasonNumber" + } else { + "item/${contentId.routeEncode()}" + } ) { companion object { const val ROUTE = "item/{contentId}?seasonNumber={seasonNumber}" @@ -138,7 +144,11 @@ sealed class Route(val route: String) { val collectionId: String, val libraryId: Int? = null, ) : Route( - if (libraryId != null) "collection/$collectionId?libraryId=$libraryId" else "collection/$collectionId" + if (libraryId != null) { + "collection/${collectionId.routeEncode()}?libraryId=$libraryId" + } else { + "collection/${collectionId.routeEncode()}" + } ) { companion object { const val ROUTE = "collection/{collectionId}?libraryId={libraryId}" @@ -158,7 +168,7 @@ sealed class Route(val route: String) { val roomId: String? = null, ) : Route( buildString { - append("player/$contentId") + append("player/${contentId.routeEncode()}") val queryParams = listOfNotNull( fileId?.let { "fileId=$it" }, // normalizeQuality is a closed wire-value set, so no URI @@ -199,7 +209,7 @@ sealed class Route(val route: String) { // resolves which part contains it; null resumes from the stored position. val startPosition: Double? = null, ) : Route( - "audiobook/$contentId" + + "audiobook/${contentId.routeEncode()}" + listOfNotNull( fileId?.let { "fileId=$it" }, if (fromStart) "fromStart=true" else null, @@ -218,7 +228,7 @@ sealed class Route(val route: String) { // --- Book reader (fullscreen, dispatches by BookFormat) --- data class BookReader(val contentId: String, val fileId: Int? = null) : Route( - "reader/$contentId" + fileId?.let { "?fileId=$it" }.orEmpty(), + "reader/${contentId.routeEncode()}" + fileId?.let { "?fileId=$it" }.orEmpty(), ) { companion object { const val ROUTE = "reader/{contentId}?fileId={fileId}" @@ -247,3 +257,17 @@ sealed class Route(val route: String) { } } + +/** + * Percent-encode a value for use as a route path segment. + * + * Deliberately `java.net.URLEncoder` rather than `android.net.Uri.encode`: + * routes are built in plain JVM unit tests, where `android.net.Uri` is stubbed + * and silently returns null — a route would become "item/null" and the test + * would assert against nonsense. Mirrors the TV app's `routeEncode`. + * + * `URLEncoder` is form encoding, where a space becomes `+`; a path segment + * needs `%20`, hence the fixup. + */ +private fun String.routeEncode(): String = + URLEncoder.encode(this, StandardCharsets.UTF_8.toString()).replace("+", "%20") diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.kt new file mode 100644 index 000000000..e1d0b6d83 --- /dev/null +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ContentDeepLinkEncodingTest.kt @@ -0,0 +1,77 @@ +package org.siloserver.silo.android.ui.navigation + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * `URI.path` is already percent-decoded. The parser used to take that decoded + * value and interpolate it straight back into a route string, so the decoded + * bytes were re-read as route syntax: an id carrying an encoded `?` became a + * query argument, and one carrying an encoded `/` truncated. + * + * Plain JVM: both sides use java.net encoding precisely so routes stay + * testable without Robolectric — `android.net.Uri` is stubbed here and would + * silently yield "item/null". + */ +class ContentDeepLinkEncodingTest { + + @Test + fun `an id containing an encoded separator cannot inject a route argument`() { + val route = contentDeepLinkRouteOrNull("silo://item/abc%3FseasonNumber%3D9") + + assertEquals( + Route.ItemDetail("abc?seasonNumber=9").route, + route, + "the whole decoded id must stay one path segment", + ) + // The literal injection the old code produced. + assertNotEquals("item/abc?seasonNumber=9", route) + } + + @Test + fun `an ordinary id is unchanged through the round trip`() { + assertEquals( + Route.ItemDetail("tt0111161").route, + contentDeepLinkRouteOrNull("silo://item/tt0111161"), + ) + } + + @Test + fun `a plus in an id survives as a plus`() { + // URLDecoder would turn this into a space; a path segment must not. + val route = contentDeepLinkRouteOrNull("silo://item/a%2Bb") + assertEquals(Route.ItemDetail("a+b").route, route) + } + + @Test + fun `a play link round trips through the player route`() { + assertEquals( + Route.Player(contentId = "abc/def").route, + contentDeepLinkRouteOrNull("silo://play/abc%2Fdef"), + ) + } + + @Test + fun `a non silo scheme is not a deep link`() { + assertNull(contentDeepLinkRouteOrNull("https://example.com/item/abc")) + } + + /** + * The player route is parsed back to compare against the live player. An + * encoded id would never equal the decoded target, so an already-showing + * player would look like a new request and restart. + */ + @Test + fun `the player route parses back to the original id`() { + val route = Route.Player(contentId = "abc?x=1").route + + assertEquals("abc?x=1", playerRouteIntentOrNull(route)?.contentId) + } + + private fun assertNotEquals(unexpected: String, actual: String?) { + if (unexpected == actual) { + throw AssertionError("expected not to equal <$unexpected>") + } + } +} diff --git a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt index 70baa377e..2b5eb1eff 100644 --- a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt +++ b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvAppNavigation.kt @@ -7,7 +7,9 @@ import androidx.compose.animation.fadeIn import androidx.compose.animation.fadeOut import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.MutableState import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.collectAsState import androidx.compose.foundation.layout.Box @@ -15,6 +17,7 @@ import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.runtime.produceState import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.ui.Modifier +import androidx.navigation.NavHostController import androidx.navigation.NavType import androidx.navigation.compose.NavHost import androidx.navigation.compose.composable @@ -49,6 +52,9 @@ import org.siloserver.silo.tv.ui.screens.settings.diagnostics.TvDiagnosticsSetti import org.siloserver.silo.tv.ui.screens.settings.diagnostics.TvDiagnosticsViewModel import org.siloserver.silo.tv.ui.screens.watchtogether.TvWatchTogetherLobbyScreen import org.siloserver.silo.tv.ui.screens.watchtogether.tvWatchTogetherDestination +import org.siloserver.silo.model.watchtogether.RoomSnapshot +import org.siloserver.silo.watchtogether.WatchTogetherEntryTarget +import org.siloserver.silo.watchtogether.watchTogetherEntryTarget import org.siloserver.silo.common.overlays.ProvideCardOverlays import org.siloserver.silo.common.diagnostics.DiagnosticsLifecycleLogger import org.siloserver.silo.common.settings.LibraryPlaybackPrefsStore @@ -75,6 +81,202 @@ internal fun tvShouldShowDiagnosticsPrompt(currentRoute: String?): Boolean = */ private const val MAX_DEEP_LINK_NAV_ATTEMPTS = 3 +/** + * True when item detail for exactly [contentId]/[seasonNumber] is already the + * top of the stack. + * + * `launchSingleTop` cannot answer this: it matches on the destination NODE, and + * every item-detail route shares one node. Using it here meant navigating from + * detail A to related item B reused A's entry instead of pushing, so Back from + * B skipped A entirely. Comparing the concrete arguments keeps the double-Select + * protection that was actually wanted while letting a different item push. + * + * Navigation decodes path arguments, so these compare against the decoded id the + * callers pass — not the percent-encoded form in the route string. + */ +internal fun tvIsAlreadyShowingItemDetail( + currentRoute: String?, + currentContentId: String?, + currentSeasonNumber: Int?, + contentId: String, + seasonNumber: Int?, +): Boolean = + currentRoute == TvRoute.ItemDetail.ROUTE && + currentContentId == contentId && + currentSeasonNumber == seasonNumber + +/** Destinations that own an active playback session. */ +private val tvPlayerRoutes = setOf(TvRoute.Player.ROUTE, TvRoute.AudiobookPlayer.ROUTE) + +/** + * What to do with a playback request given what is already on top. + * + * `launchSingleTop` is the wrong tool here for the same reason it was wrong for + * item detail — it matches the destination node, not its arguments — and it is + * worse for a player: AndroidX implements single-top by reusing the existing + * back-stack entry with new arguments, so the entry's ViewModelStore survives + * and the previous title's player ViewModel (and its session) can live on beside + * the new one. + * + * Suppression means: *the entry this exact request created is still the top + * one*. [TvPlaybackNavigation] records the requested route together with the id + * of the entry it produced, and both must still hold. Note this identifies the + * entry, not its current arguments — see below for why nothing is allowed to + * rewrite a player entry in place. + * + * Weaker keys were tried and are wrong. The route alone ignores what is + * actually on top — cast launches and auto-advance navigate to players without + * coming through here, so it can name a player that is long gone. Route plus + * content id still collides when one of those puts up the SAME title with + * different arguments (an auto-advance handoff), suppressing a real request. + * + * The entry id closes both, because those paths pop and push. It is NOT + * self-sufficient: a `launchSingleTop` navigation mutates an entry's arguments + * while keeping its id, which would leave a stale record looking current. That + * is why Watch Together — the one player-bound path that did this — now routes + * through here too, and why no playback navigation uses `launchSingleTop`. + * + * It is recorded only when the navigation actually produced a NEW entry, so a + * navigation dropped during teardown leaves nothing behind to suppress its own + * retry — including when the player already up happens to be the same title. + */ +internal data class TvPlaybackNavigation(val destination: String, val entryId: String) + +internal enum class TvPlaybackNavAction { Push, ReplaceCurrentPlayer, Suppress } + +internal fun tvPlaybackNavAction( + currentRoute: String?, + currentEntryId: String?, + lastPlaybackNavigation: TvPlaybackNavigation?, + destination: String, +): TvPlaybackNavAction = when { + currentRoute !in tvPlayerRoutes -> TvPlaybackNavAction.Push + // A double Select whose second press landed while the first navigation was + // still animating: same request, same entry, nothing to do. + lastPlaybackNavigation != null && + lastPlaybackNavigation.destination == destination && + lastPlaybackNavigation.entryId == currentEntryId -> TvPlaybackNavAction.Suppress + // A genuinely different playback request while a player is up: take over + // the entry rather than stacking players Back would walk back through. + else -> TvPlaybackNavAction.ReplaceCurrentPlayer +} + +/** + * The navigation to remember after a playback request, or null if nothing + * usable arrived. + * + * [entryIdBefore] is what was on top before navigating. Requiring a different + * id afterwards is what distinguishes "our request landed" from "the navigation + * was dropped and the player already there happens to match" — the latter would + * otherwise be recorded as ours and suppress the retry. + */ +internal fun tvRecordedPlaybackNavigation( + destination: String, + contentId: String, + entryIdBefore: String?, + arrivedEntryId: String?, + arrivedContentId: String?, +): TvPlaybackNavigation? = + if (arrivedEntryId != null && arrivedEntryId != entryIdBefore && arrivedContentId == contentId) { + TvPlaybackNavigation(destination = destination, entryId = arrivedEntryId) + } else { + null + } + +/** The content id argument for whichever player destination [route] is. */ +private fun tvPlayerContentIdArg(route: String?): String? = when (route) { + TvRoute.Player.ROUTE -> TvRoute.Player.ARG_CONTENT_ID + TvRoute.AudiobookPlayer.ROUTE -> TvRoute.AudiobookPlayer.ARG_CONTENT_ID + else -> null +} + +/** Navigates to a playback destination, collapsing an identical repeat. */ +private fun NavHostController.navigateToTvPlayback( + destination: String, + contentId: String, + lastPlaybackNavigation: MutableState, +) { + val top = currentBackStackEntry + val topRoute = top?.destination?.route + when ( + tvPlaybackNavAction( + currentRoute = topRoute, + currentEntryId = top?.id, + lastPlaybackNavigation = lastPlaybackNavigation.value, + destination = destination, + ) + ) { + TvPlaybackNavAction.Suppress -> return + TvPlaybackNavAction.Push -> navigate(destination) + TvPlaybackNavAction.ReplaceCurrentPlayer -> + navigate(destination) { topRoute?.let { popUpTo(it) { inclusive = true } } } + } + // Record only what actually arrived. `navigate` can be dropped (see the + // deep-link collector), and remembering a request that never landed would + // let it suppress its own retry. + // + // A NEW entry id is the load-bearing part. Checking only the destination and + // content id would accept the player that was already there — replacing + // `A?roomId=x` with solo `A` and having the navigation dropped would record + // the untouched Watch Together entry as if it were ours, and the retry would + // then suppress. + val arrived = currentBackStackEntry + lastPlaybackNavigation.value = tvRecordedPlaybackNavigation( + destination = destination, + contentId = contentId, + entryIdBefore = top?.id, + arrivedEntryId = arrived?.id, + arrivedContentId = tvPlayerContentIdArg(arrived?.destination?.route) + ?.let { arg -> arrived?.arguments?.getString(arg) }, + ) +} + +/** + * Watch Together enters either a lobby or a player. The player case is an + * ordinary playback navigation and must go through [navigateToTvPlayback] — + * it used `launchSingleTop`, which mutates the existing player entry's + * arguments while PRESERVING its id, so a recorded solo request could still + * look current afterwards and suppress the user's next real request. + */ +private fun NavHostController.navigateToTvWatchTogether( + room: RoomSnapshot, + lastPlaybackNavigation: MutableState, +) { + val destination = tvWatchTogetherDestination(room) + val contentId = room.selectedContentId + if (watchTogetherEntryTarget(room) == WatchTogetherEntryTarget.Player && contentId != null) { + navigateToTvPlayback( + destination = destination, + contentId = contentId, + lastPlaybackNavigation = lastPlaybackNavigation, + ) + } else { + navigate(destination) { launchSingleTop = true } + } +} + +/** Pushes item detail, collapsing only an exact repeat of the current page. */ +private fun NavHostController.navigateToTvItemDetail( + contentId: String, + seasonNumber: Int? = null, +) { + val top = currentBackStackEntry + if ( + tvIsAlreadyShowingItemDetail( + currentRoute = top?.destination?.route, + currentContentId = top?.arguments?.getString(TvRoute.ItemDetail.ARG_CONTENT_ID), + currentSeasonNumber = top?.arguments + ?.getString(TvRoute.ItemDetail.ARG_SEASON_NUMBER) + ?.toIntOrNull(), + contentId = contentId, + seasonNumber = seasonNumber, + ) + ) { + return + } + navigate(TvRoute.ItemDetail(contentId, seasonNumber).route) +} + /** * Top-level TV navigation graph. * @@ -114,6 +316,9 @@ fun TvAppNavigation( ) { val navController = rememberNavController() val scope = rememberCoroutineScope() + // The playback request [navigateToTvPlayback] last put on the stack, and + // the entry it produced. Only used to collapse an immediate repeat. + val lastPlaybackNavigation = remember { mutableStateOf(null) } val tokenManager: TokenManager = koinInject() val authRepository: AuthRepository = koinInject() val profileRepository: ProfileRepository = koinInject() @@ -137,11 +342,17 @@ fun TvAppNavigation( audioTrackIndex = playback.audioTrackIndex, subtitleTrackIndex = playback.subtitleTrackIndex, ).route - val replaceCurrentPlayer = navController.currentDestination?.route == TvRoute.Player.ROUTE + // Replace whichever player is on top, not just the video one. This + // only knew about TvRoute.Player, so a cast launch during an + // audiobook stacked over it and Back resurrected the audiobook + // player — restoring a session the viewer thought they had left. + val replacedPlayerRoute = navController.currentDestination?.route + ?.takeIf { it == TvRoute.Player.ROUTE || it == TvRoute.AudiobookPlayer.ROUTE } + // No launchSingleTop: popUpTo is evaluated first, so once the + // player entry is popped there is nothing left for single-top to + // match. Every Launch request deliberately (re)starts playback. navController.navigate(destination) { - if (replaceCurrentPlayer) { - popUpTo(TvRoute.Player.ROUTE) { inclusive = true } - } + replacedPlayerRoute?.let { popUpTo(it) { inclusive = true } } } } } @@ -239,7 +450,7 @@ fun TvAppNavigation( "deep link navigating (attempt $attempts): ${uri.host}/$contentId from $route", ) when (uri.host) { - "item" -> navController.navigate(TvRoute.ItemDetail(contentId).route) + "item" -> navController.navigateToTvItemDetail(contentId) "play" -> { // The Watch Next mapper tags play intents with the item type // (`silo://play/?type=`) so audiobook tiles @@ -250,8 +461,10 @@ fun TvAppNavigation( // behavior for movie/episode tiles. val itemType = uri.getQueryParameter("type") val playbackArgs = parseTvPlaybackDeepLinkArgs(uri::getQueryParameter) - navController.navigate( - tvPlayDestinationFor( + // A retried link (arrival-gated above) must not stack a + // second player over one already being created. + navController.navigateToTvPlayback( + destination = tvPlayDestinationFor( itemType = itemType, contentId = contentId, fileId = playbackArgs.fileId, @@ -260,11 +473,9 @@ fun TvAppNavigation( subtitleTrackIndex = playbackArgs.subtitleTrackIndex, quality = playbackArgs.quality, ), - ) { - // A retried link (arrival-gated above) must not stack a - // second player over one already being created. - launchSingleTop = true - } + contentId = contentId, + lastPlaybackNavigation = lastPlaybackNavigation, + ) } } } @@ -437,12 +648,16 @@ fun TvAppNavigation( watchNextSeeder.seedNow() watchNextSeeder.enqueuePeriodic() }, - onAddProfile = { navController.navigate(TvRoute.CreateProfile.route) }, + onAddProfile = { + navController.navigate(TvRoute.CreateProfile.route) { launchSingleTop = true } + }, onEditProfile = { profileId -> - navController.navigate(TvRoute.EditProfile(profileId).route) + navController.navigate(TvRoute.EditProfile(profileId).route) { + launchSingleTop = true + } }, onChangeServer = { - navController.navigate(TvRoute.ServerList.route) + navController.navigate(TvRoute.ServerList.route) { launchSingleTop = true } }, onSignOut = { scope.launch { @@ -488,24 +703,16 @@ fun TvAppNavigation( }, onManageServers = { mainEntry.savedStateHandle[RETURN_TO_MANAGE_SERVERS_KEY] = true - navController.navigate(TvRoute.ServerList.route) + navController.navigate(TvRoute.ServerList.route) { launchSingleTop = true } }, onOpenDiagnostics = { - navController.navigate(TvRoute.Diagnostics.route) + navController.navigate(TvRoute.Diagnostics.route) { launchSingleTop = true } }, onOpenItemDetail = { contentId -> - // launchSingleTop collapses a double-OK on the same card into - // one ItemDetail entry (consecutive identical contentId), so - // Back doesn't appear inert against a duplicate. Distinct - // pushes are unaffected — their route args differ. - navController.navigate(TvRoute.ItemDetail(contentId).route) { - launchSingleTop = true - } + navController.navigateToTvItemDetail(contentId) }, onOpenWatchTogether = { room -> - navController.navigate(tvWatchTogetherDestination(room)) { - launchSingleTop = true - } + navController.navigateToTvWatchTogether(room, lastPlaybackNavigation) }, onOpenLibraryCollectionDetail = { libraryId, collectionId, title -> navController.navigate( @@ -513,7 +720,9 @@ fun TvAppNavigation( ) }, onOpenCollectionDetail = { collectionId, title -> - navController.navigate(TvRoute.CollectionDetail(collectionId, title).route) + navController.navigate(TvRoute.CollectionDetail(collectionId, title).route) { + launchSingleTop = true + } }, onSignedOut = { scope.launch { @@ -570,7 +779,9 @@ fun TvAppNavigation( // Server" opens the server list; the user picks an existing // saved server or chooses Add to enter a new URL. onSwitchServer = { - navController.navigate(TvRoute.ServerList.route) + navController.navigate(TvRoute.ServerList.route) { + launchSingleTop = true + } }, onPairDevice = { navController.navigate(TvRoute.PairDevice().route) { @@ -578,17 +789,23 @@ fun TvAppNavigation( } }, onPlayItem = { playContentId, itemType, resumePositionSeconds -> - navController.navigate( - tvPlayDestinationFor( + // A fast double Select otherwise stacks a second player, + // starting two sessions and leaving Back on a duplicate. + navController.navigateToTvPlayback( + destination = tvPlayDestinationFor( itemType = itemType, contentId = playContentId, fileId = null, resumePositionSeconds = resumePositionSeconds, ), + contentId = playContentId, + lastPlaybackNavigation = lastPlaybackNavigation, ) }, onOpenPersonDetail = { personId -> - navController.navigate(TvRoute.PersonDetail(personId).route) + navController.navigate(TvRoute.PersonDetail(personId).route) { + launchSingleTop = true + } }, ) } @@ -597,7 +814,9 @@ fun TvAppNavigation( TvDiagnosticsSettingsScreen( onBack = { navController.popBackStack() }, onReportSelected = { reportId -> - navController.navigate(TvRoute.DiagnosticsReport(reportId).route) + navController.navigate(TvRoute.DiagnosticsReport(reportId).route) { + launchSingleTop = true + } }, ) } @@ -643,8 +862,12 @@ fun TvAppNavigation( // to the server's first listed file (which for multi-version // titles is often the lower-resolution encode). onPlay = { playContentId, fileId, audioTrackIndex, audioPicked, subtitleTrackIndex, itemType, resumePositionSeconds -> - navController.navigate( - tvPlayDestinationFor( + // A fast Select after entering detail can overlap the route + // transition. Collapse an identical second Play request + // instead of creating two player ViewModels and two + // concurrent playback-session starts. + navController.navigateToTvPlayback( + destination = tvPlayDestinationFor( itemType = itemType, contentId = playContentId, fileId = fileId, @@ -653,47 +876,38 @@ fun TvAppNavigation( audioPickedThisSession = audioPicked, subtitleTrackIndex = subtitleTrackIndex, ), - ) { - // A fast Select after entering detail can overlap the - // route transition. Collapse an identical second Play - // request instead of creating two player ViewModels and - // two concurrent playback-session starts. - launchSingleTop = true - } + contentId = playContentId, + lastPlaybackNavigation = lastPlaybackNavigation, + ) }, onItemDetail = { itemContentId -> - // launchSingleTop suppresses the exact double-tap dupe; a - // distinct related item (always a different contentId) still - // pushes normally. - navController.navigate(TvRoute.ItemDetail(itemContentId).route) { - launchSingleTop = true - } + navController.navigateToTvItemDetail(itemContentId) }, // Season switching replaces the current detail entry so paging // through seasons never stacks pages — one Back returns to the // screen the user arrived from. onItemDetailReplace = { itemContentId -> val current = navController.currentBackStackEntry?.destination?.route + // No launchSingleTop: popUpTo is evaluated first, so once + // the current page is popped there is nothing left for + // single-top to match. navController.navigate(TvRoute.ItemDetail(itemContentId).route) { - launchSingleTop = true current?.let { popUpTo(it) { inclusive = true } } } }, onSeriesClick = { seriesId -> - navController.navigate(TvRoute.ItemDetail(seriesId).route) { - launchSingleTop = true - } + navController.navigateToTvItemDetail(seriesId) }, onSeasonClick = { seriesId, selectedSeason -> - navController.navigate(TvRoute.ItemDetail(seriesId, selectedSeason).route) { - launchSingleTop = true - } + navController.navigateToTvItemDetail(seriesId, selectedSeason) }, onWatchTogether = { snapshot -> - navController.navigate(tvWatchTogetherDestination(snapshot)) + navController.navigateToTvWatchTogether(snapshot, lastPlaybackNavigation) }, onOpenPerson = { personId -> - navController.navigate(TvRoute.PersonDetail(personId).route) + navController.navigate(TvRoute.PersonDetail(personId).route) { + launchSingleTop = true + } }, onBack = { navController.popBackStack() }, ) @@ -709,9 +923,7 @@ fun TvAppNavigation( TvPersonDetailScreen( personId = personId, onOpenItemDetail = { itemContentId -> - navController.navigate(TvRoute.ItemDetail(itemContentId).route) { - launchSingleTop = true - } + navController.navigateToTvItemDetail(itemContentId) }, onBack = { navController.popBackStack() }, ) @@ -955,9 +1167,7 @@ fun TvAppNavigation( collectionId = collectionId, title = title, onItemClick = { contentId -> - navController.navigate(TvRoute.ItemDetail(contentId).route) { - launchSingleTop = true - } + navController.navigateToTvItemDetail(contentId) }, onBack = { navController.popBackStack() }, ) @@ -983,9 +1193,7 @@ fun TvAppNavigation( collectionId = collectionId, title = title, onItemClick = { contentId -> - navController.navigate(TvRoute.ItemDetail(contentId).route) { - launchSingleTop = true - } + navController.navigateToTvItemDetail(contentId) }, onBack = { navController.popBackStack() }, ) @@ -1002,7 +1210,9 @@ fun TvAppNavigation( ?.let { prompt -> TvDiagnosticsPromptScreen( prompt = prompt, - onReview = { navController.navigate(TvRoute.Diagnostics.route) }, + onReview = { + navController.navigate(TvRoute.Diagnostics.route) { launchSingleTop = true } + }, onSend = { diagnosticsViewModel.uploadPrompt(prompt) }, onAlwaysSend = { diagnosticsViewModel.alwaysSendPrompt(prompt) }, onDontSend = { diagnosticsViewModel.declinePrompt(prompt) }, diff --git a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.kt b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.kt index 85a079da1..4cafc2c39 100644 --- a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.kt +++ b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/navigation/TvRoute.kt @@ -63,9 +63,9 @@ sealed class TvRoute(val route: String) { data class ItemDetail(val contentId: String, val seasonNumber: Int? = null) : TvRoute( if (seasonNumber != null) { - "item/$contentId?seasonNumber=$seasonNumber" + "item/${contentId.routeEncode()}?seasonNumber=$seasonNumber" } else { - "item/$contentId" + "item/${contentId.routeEncode()}" }, ) { companion object { @@ -104,7 +104,7 @@ sealed class TvRoute(val route: String) { val episodeSelectionHandoffNonce: String? = null, ) : TvRoute( buildString { - append("player/$contentId") + append("player/${contentId.routeEncode()}") val query = buildList { if (fileId != null) add("fileId=$fileId") VideoPlayerRouteArgs.normalizeQuality(quality)?.let { value -> @@ -155,7 +155,7 @@ sealed class TvRoute(val route: String) { val startPositionSeconds: Double? = null, ) : TvRoute( buildString { - append("audiobook/$contentId") + append("audiobook/${contentId.routeEncode()}") val query = buildList { if (fileId != null) add("fileId=$fileId") VideoPlayerRouteArgs.encodeResumePosition(startPositionSeconds)?.let { value -> diff --git a/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvItemDetailNavigationTest.kt b/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvItemDetailNavigationTest.kt new file mode 100644 index 000000000..00244c2a8 --- /dev/null +++ b/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/navigation/TvItemDetailNavigationTest.kt @@ -0,0 +1,263 @@ +package org.siloserver.silo.tv.ui.navigation + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Item detail used `launchSingleTop`, which matches the destination NODE rather + * than its arguments. Every item-detail route shares one node, so navigating + * from detail A to related item B reused A's entry instead of pushing — Back + * from B skipped A and returned to whatever was underneath. + * + * The replacement collapses only an EXACT repeat, which is what the original + * comment claimed `launchSingleTop` did. + */ +class TvItemDetailNavigationTest { + + + @Test + fun `a different item is not the current page`() { + assertFalse( + tvIsAlreadyShowingItemDetail( + currentRoute = TvRoute.ItemDetail.ROUTE, + currentContentId = "item-a", + currentSeasonNumber = null, + contentId = "item-b", + seasonNumber = null, + ), + "a related item must push its own entry so Back returns to the item it came from", + ) + } + + @Test + fun `the same item is the current page`() { + assertTrue( + tvIsAlreadyShowingItemDetail( + currentRoute = TvRoute.ItemDetail.ROUTE, + currentContentId = "item-a", + currentSeasonNumber = null, + contentId = "item-a", + seasonNumber = null, + ), + "a double Select on one card must not stack a duplicate page", + ) + } + + @Test + fun `the same series at a different season is not the current page`() { + assertFalse( + tvIsAlreadyShowingItemDetail( + currentRoute = TvRoute.ItemDetail.ROUTE, + currentContentId = "series-a", + currentSeasonNumber = 1, + contentId = "series-a", + seasonNumber = 2, + ), + ) + } + + @Test + fun `the same series at the same season is the current page`() { + assertTrue( + tvIsAlreadyShowingItemDetail( + currentRoute = TvRoute.ItemDetail.ROUTE, + currentContentId = "series-a", + currentSeasonNumber = 3, + contentId = "series-a", + seasonNumber = 3, + ), + ) + } + + @Test + fun `the identical request on the same entry is suppressed`() { + val destination = TvRoute.Player(contentId = "movie-a").route + assertEquals( + TvPlaybackNavAction.Suppress, + tvPlaybackNavAction( + currentRoute = TvRoute.Player.ROUTE, + currentEntryId = "entry-1", + lastPlaybackNavigation = TvPlaybackNavigation(destination, "entry-1"), + destination = destination, + ), + "a double Select must not start a second session on the same title", + ) + } + + /** + * The same title at another version is a different request; dropping it + * would make the version picker silently do nothing. + */ + @Test + fun `the same title at a different version still navigates`() { + assertEquals( + TvPlaybackNavAction.ReplaceCurrentPlayer, + tvPlaybackNavAction( + currentRoute = TvRoute.Player.ROUTE, + currentEntryId = "entry-1", + lastPlaybackNavigation = TvPlaybackNavigation( + TvRoute.Player(contentId = "movie-a", fileId = 1).route, + "entry-1", + ), + destination = TvRoute.Player(contentId = "movie-a", fileId = 2).route, + ), + ) + } + + @Test + fun `a different title takes over the player entry`() { + assertEquals( + TvPlaybackNavAction.ReplaceCurrentPlayer, + tvPlaybackNavAction( + currentRoute = TvRoute.Player.ROUTE, + currentEntryId = "entry-1", + lastPlaybackNavigation = TvPlaybackNavigation( + TvRoute.Player(contentId = "movie-a").route, + "entry-1", + ), + destination = TvRoute.Player(contentId = "movie-b").route, + ), + "stacking players leaves Back walking through dead sessions", + ) + } + + @Test + fun `playback arriving over an audiobook takes over that entry too`() { + assertEquals( + TvPlaybackNavAction.ReplaceCurrentPlayer, + tvPlaybackNavAction( + currentRoute = TvRoute.AudiobookPlayer.ROUTE, + currentEntryId = "entry-1", + lastPlaybackNavigation = TvPlaybackNavigation( + TvRoute.AudiobookPlayer(contentId = "book-a").route, + "entry-1", + ), + destination = TvRoute.Player(contentId = "movie-b").route, + ), + "Back must not resurrect the audiobook the viewer replaced", + ) + } + + @Test + fun `playing from a non player screen pushes`() { + val destination = TvRoute.Player(contentId = "movie-a").route + assertEquals( + TvPlaybackNavAction.Push, + tvPlaybackNavAction( + currentRoute = TvRoute.ItemDetail.ROUTE, + currentEntryId = "entry-1", + lastPlaybackNavigation = TvPlaybackNavigation(destination, "entry-1"), + destination = destination, + ), + "Play from detail must push a player, not be mistaken for a repeat", + ) + } + + /** + * The case that broke every weaker key: cast and auto-advance navigate + * without going through the helper, and can put up the SAME title with + * different arguments (an auto-advance handoff, another file). Keying on + * route + content id suppressed the user's real request; keying on the entry + * the request produced does not, because those paths pop and push, so the id + * differs. (Watch Together used to belong on this list; it now routes + * through the helper precisely because it single-topped the player entry in + * place, preserving the id.) + */ + @Test + fun `a bypass path putting up the same title does not suppress`() { + val destination = TvRoute.Player(contentId = "movie-a").route + assertEquals( + TvPlaybackNavAction.ReplaceCurrentPlayer, + tvPlaybackNavAction( + currentRoute = TvRoute.Player.ROUTE, + // A cast launch replaced our entry with its own for the same title. + currentEntryId = "entry-2", + lastPlaybackNavigation = TvPlaybackNavigation(destination, "entry-1"), + destination = destination, + ), + ) + } + + /** With nothing recorded there is nothing to collide with. */ + @Test + fun `a request that never arrived can be retried`() { + val destination = TvRoute.Player(contentId = "movie-a").route + assertEquals( + TvPlaybackNavAction.ReplaceCurrentPlayer, + tvPlaybackNavAction( + currentRoute = TvRoute.Player.ROUTE, + currentEntryId = "entry-1", + lastPlaybackNavigation = null, + destination = destination, + ), + ) + } + + @Test + fun `a matching id on some other destination is not the current page`() { + assertFalse( + tvIsAlreadyShowingItemDetail( + currentRoute = TvRoute.Player.ROUTE, + currentContentId = "item-a", + currentSeasonNumber = null, + contentId = "item-a", + seasonNumber = null, + ), + "playing an item must not suppress opening its detail page", + ) + } + + // --- what gets recorded --- + + /** + * The case that made the previous key unsound: replacing a Watch Together + * player for the SAME title with a solo request, where the navigation is + * dropped during teardown. Confirming arrival on route + content id alone + * would adopt the untouched Watch Together entry as ours, and the retry + * would then be suppressed — the user presses Play and nothing happens. + */ + @Test + fun `a dropped navigation records nothing even when the same title is up`() { + assertNull( + tvRecordedPlaybackNavigation( + destination = TvRoute.Player(contentId = "movie-a").route, + contentId = "movie-a", + entryIdBefore = "entry-1", + // Unchanged: navigate() was dropped. + arrivedEntryId = "entry-1", + arrivedContentId = "movie-a", + ), + ) + } + + @Test + fun `a landed navigation records its own entry`() { + val destination = TvRoute.Player(contentId = "movie-a").route + assertEquals( + TvPlaybackNavigation(destination, "entry-2"), + tvRecordedPlaybackNavigation( + destination = destination, + contentId = "movie-a", + entryIdBefore = "entry-1", + arrivedEntryId = "entry-2", + arrivedContentId = "movie-a", + ), + ) + } + + @Test + fun `landing somewhere other than the requested content records nothing`() { + assertNull( + tvRecordedPlaybackNavigation( + destination = TvRoute.Player(contentId = "movie-a").route, + contentId = "movie-a", + entryIdBefore = "entry-1", + arrivedEntryId = "entry-2", + arrivedContentId = null, + ), + ) + } +} diff --git a/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.kt b/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.kt index 8643b8791..3c8863df5 100644 --- a/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.kt +++ b/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/watchtogether/TvWatchTogetherSurfaceSourceTest.kt @@ -37,7 +37,12 @@ class TvWatchTogetherSurfaceSourceTest { fun aResolvedRoomReachesTheNavigationCallback() { assertTrue(itemDetailScreen.contains("onWatchTogether(room)")) assertTrue(itemDetailScreen.contains("watchTogetherViewModel.consumeResult()")) - assertTrue(appNavigation.contains("tvWatchTogetherDestination(snapshot)")) + // The resolved room now goes through navigateToTvWatchTogether, which + // is what builds the destination — a Watch Together PLAYER target is an + // ordinary playback navigation and has to share the player back-stack + // bookkeeping instead of single-topping the current player in place. + assertTrue(appNavigation.contains("navigateToTvWatchTogether(snapshot")) + assertTrue(appNavigation.contains("tvWatchTogetherDestination(room)")) } @Test From c780ee37ab32eef255d7db3c6040c3eb53218f94 Mon Sep 17 00:00:00 2001 From: rxwatcher Date: Wed, 5 Aug 2026 21:59:45 +0200 Subject: [PATCH 3/6] fix(nav): close the deep-link, tab-root and identity-read gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remaining findings from the navigation and profile audits. - Bottom-nav tab switching popped to a hard-coded Home. An offline launch with downloads starts on Downloads, and popping to a route that is not on the stack pops nothing, so every tab stacked and Back walked back through previously visited tabs instead of leaving. The anchor is now read from the live back stack, because neither a hard-coded route nor the graph's declared start survives that tab later disappearing. - A tab that can no longer be shown is now removed rather than left at the bottom of the stack, where Back revealed it and its own effect bounced straight back. This also covers the anchor vanishing while the user is on a different tab, which nothing previously noticed. No saveState on that path: restoreState is evaluated before launchSingleTop, so saving the vanishing tab and restoring on the way out immediately restored the subtree that had just been popped. - Cold device links were the graph's start destination, so a signed-out user got Pair Device at the root; its Sign In pushed Login, and the successful login cleared the stack and lost the pairing request. They are queued as pending external routes instead, so the normal gates run and pairing arrives on an authenticated stack with somewhere to go back to. The PairDevice destination's navDeepLinks are gone — Navigation matched the launch Intent itself when the graph was installed, which was the same bypass. - HTTPS device links carry the origin that issued the pairing request and it was discarded, so the code was looked up against whichever server happened to be active. The origin is now parsed (typed, default ports normalized), checked, and carried on the pending request so it survives the wait through setup and login and is re-checked at delivery. A link naming a different configured server is refused; that refusal is silent, which is recorded as a known gap needing a product decision. - The launch Intent was re-parsed on every Activity recreation, pulling the user back to a link they had already followed. Delivery is now recorded on the Intent for in-process recreation and in saved state for process death. - Profile id and token are read as one identity in the three places that assemble both into a request. The write was already atomic; separate reads could still pair an old id with a new token across a switch. Test doubles delegating TokenManager needed getProfileIdentity overrides: interface delegation forwards a default method to the delegate, so they were silently exercising the wrong identity while staying green. Eleven review rounds. The last five were all one root cause — save/restore state on a tab that can no longer be shown — and each fix exposed the next. Unrelated: androidApp PlayerViewModelLoadOwnershipIntegrationTest > exitDuringDeferredLoadRejectsAndStopsLateReady is a pre-existing flake, timing out at 5s on roughly two runs in three, including on a clean HEAD. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW --- .../silo/common/player/MediaAuthSession.kt | 5 +- .../WatchTogetherRealtimeWebSocketTest.kt | 5 + .../siloserver/silo/android/MainActivity.kt | 110 ++++++++++++-- .../android/ui/navigation/AppNavigation.kt | 56 ++++++-- .../android/ui/navigation/BottomNavBar.kt | 49 +++++++ .../ui/navigation/DeviceLoginRouteParser.kt | 97 +++++++++++++ .../ui/navigation/ExternalRouteNavigation.kt | 26 +++- .../silo/android/ui/navigation/Routes.kt | 7 +- .../silo/android/ui/screens/MainScreen.kt | 67 +++++++-- .../navigation/DeviceLoginRouteParserTest.kt | 135 ++++++++++++++++++ .../navigation/ExternalRouteNavigationTest.kt | 63 ++++++++ .../silo/network/EncryptedTokenManagerImpl.kt | 8 ++ .../silo/network/AuthInterceptorImpl.kt | 7 +- .../silo/network/PlaybackRealtimeClient.kt | 5 +- .../siloserver/silo/network/TokenManager.kt | 23 +++ .../silo/network/TokenManagerImpl.kt | 7 + .../silo/network/SiloAuthPluginPinTest.kt | 9 ++ 17 files changed, 640 insertions(+), 39 deletions(-) diff --git a/android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/MediaAuthSession.kt b/android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/MediaAuthSession.kt index d2c02f622..66ab6593b 100644 --- a/android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/MediaAuthSession.kt +++ b/android-shared/src/androidMain/kotlin/org/siloserver/silo/common/player/MediaAuthSession.kt @@ -69,8 +69,9 @@ class MediaAuthSession( return MediaAuthSnapshot(null, null, null, tokenManager.getCurrentServerId(), "") } val accessToken = tokenManager.getAccessToken() - val profileId = tokenManager.getProfileId() - val profileToken = tokenManager.getProfileToken() + val profileIdentity = tokenManager.getProfileIdentity() + val profileId = profileIdentity.profileId + val profileToken = profileIdentity.profileToken val serverIdAfter = tokenManager.getCurrentServerId() val serverUrlAfter = tokenManager.getServerUrl() diff --git a/android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/network/WatchTogetherRealtimeWebSocketTest.kt b/android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/network/WatchTogetherRealtimeWebSocketTest.kt index d0b48c384..d49a1b826 100644 --- a/android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/network/WatchTogetherRealtimeWebSocketTest.kt +++ b/android-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/network/WatchTogetherRealtimeWebSocketTest.kt @@ -22,6 +22,7 @@ import org.siloserver.silo.network.CleartextOriginNotApprovedException import org.siloserver.silo.network.DefaultWatchTogetherRealtimeClient import org.siloserver.silo.network.RoomRealtimeEvent import org.siloserver.silo.network.SiloJson +import org.siloserver.silo.network.ProfileIdentity import org.siloserver.silo.network.TokenManager import org.siloserver.silo.network.TokenManagerImpl import org.siloserver.silo.network.canonicalHttpOrigin @@ -314,6 +315,10 @@ class WatchTogetherRealtimeWebSocketTest { override suspend fun getProfileId(): String = if (activeB) "profile-b" else "profile-a" + // See SiloAuthPluginPinTest: the delegated default would bypass these. + override suspend fun getProfileIdentity(): ProfileIdentity = + ProfileIdentity(getProfileId(), getProfileToken()) + override suspend fun getProfileToken(): String { val token = if (activeB) "PROFILE_B" else "PROFILE_A" activeB = true diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt index 2e0c70b5a..664596e23 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt @@ -38,7 +38,9 @@ import org.siloserver.silo.android.ui.navigation.ExternalRouteRequestFactory import org.siloserver.silo.android.ui.navigation.Route import org.siloserver.silo.android.ui.navigation.clearConsumedExternalRouteRequest import org.siloserver.silo.android.ui.navigation.contentDeepLinkRouteOrNull +import org.siloserver.silo.android.ui.navigation.deviceLoginLinkIsForActiveServer import org.siloserver.silo.android.ui.navigation.deviceLoginPairRouteOrNull +import org.siloserver.silo.android.ui.navigation.deviceLoginRequiredOrigin import org.siloserver.silo.android.ui.navigation.hasLocalDownloadsForScope import org.siloserver.silo.android.ui.navigation.inviteClaimRouteOrNull import org.siloserver.silo.android.ui.navigation.notificationNavigationRouteOrNull @@ -77,6 +79,21 @@ class MainActivity : ComponentActivity() { // start. Mirrors the TV-side flag in MainTvActivity. @Volatile private var hasShownColdSplash = false + + /** + * Set on the launch Intent once its external route has been delivered. + * `putExtra` mutates the process-local Intent, which covers ordinary + * in-process Activity recreation but NOT process death — the system may + * rebuild the task from the original launch Intent, without this. The + * saved-state route below is what covers that case; this is the fast + * path. + */ + private const val EXTRA_EXTERNAL_ROUTE_CONSUMED = + "org.siloserver.silo.EXTERNAL_ROUTE_CONSUMED" + + /** Saved-state key for [consumedExternalRoute]. */ + private const val STATE_CONSUMED_EXTERNAL_ROUTE = + "org.siloserver.silo.CONSUMED_EXTERNAL_ROUTE" } private val externalRouteRequestFactory = ExternalRouteRequestFactory() @@ -85,6 +102,14 @@ class MainActivity : ComponentActivity() { // A replay-free SharedFlow can silently drop exactly that warm delivery. private val pendingExternalRouteRequests = MutableStateFlow(null) + /** + * The external route already delivered for the Intent this Activity was + * launched with, carried across process death in saved state so a restored + * task cannot replay a link the user already followed and navigated away + * from. + */ + private var consumedExternalRoute: String? = null + // POST_NOTIFICATIONS is required on Android 13+ for any notification — // download progress / completion notifications silently never appear // without it. @@ -95,6 +120,7 @@ class MainActivity : ComponentActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) + consumedExternalRoute = savedInstanceState?.getString(STATE_CONSUMED_EXTERNAL_ROUTE) enableEdgeToEdge() maybeRequestNotificationPermission() maybeRequestLegacyPublicDownloadPermission() @@ -112,10 +138,35 @@ class MainActivity : ComponentActivity() { // its target after auth instead of being silently dropped. // The pending route is only consumed once the main graph is // showing, so pre-auth starts just hold it. - (notificationRouteOrNull(intent) ?: contentDeepLinkRouteOrNull(intent?.dataString)) - ?.let { route -> - pendingExternalRouteRequests.value = externalRouteRequestFactory.create(route) - } + // Skip an Intent whose route was already delivered: it is only + // still here because the Activity retains it. + if (intent?.getBooleanExtra(EXTRA_EXTERNAL_ROUTE_CONSUMED, false) != true) { + val activeServerUrl = get(ServerRegistry::class.java) + .activeEntry.value?.url + val deviceRoute = deviceLoginPairRouteOrNull(intent?.dataString) + ?.takeIf { + deviceLoginLinkIsForActiveServer(intent?.dataString, activeServerUrl) + } + val externalRoute = notificationRouteOrNull(intent) + ?: contentDeepLinkRouteOrNull(intent?.dataString) + ?: deviceRoute + externalRoute + ?.takeIf { it != consumedExternalRoute } + ?.let { route -> + pendingExternalRouteRequests.value = + externalRouteRequestFactory.create( + route = route, + // Only a device link is server-scoped, and it + // may wait here through setup/login onto a + // server that did not issue it. + requiredServerOrigin = if (route === deviceRoute) { + deviceLoginRequiredOrigin(intent?.dataString) + } else { + null + }, + ) + } + } launchAuthenticatedStartupWarmup(route) } @@ -152,6 +203,15 @@ class MainActivity : ComponentActivity() { startDestination = resolvedRoute, pendingExternalRoute = pendingExternalRoute, onExternalRouteConsumed = { consumedRequest -> + // Record the delivery in two places. The Intent + // extra covers in-process Activity recreation, + // which re-parses the retained Intent in + // onCreate and would otherwise yank the user + // back to a link they already followed. It is + // process-local, so the saved-state route below + // is what covers process death. + intent?.putExtra(EXTRA_EXTERNAL_ROUTE_CONSUMED, true) + consumedExternalRoute = consumedRequest.route pendingExternalRouteRequests.update { pendingRequest -> clearConsumedExternalRouteRequest( pendingRequest = pendingRequest, @@ -175,14 +235,36 @@ class MainActivity : ComponentActivity() { lifecycleScope.launch(Dispatchers.IO) { refresher.refreshIfStale() } } + override fun onSaveInstanceState(outState: Bundle) { + super.onSaveInstanceState(outState) + consumedExternalRoute?.let { outState.putString(STATE_CONSUMED_EXTERNAL_ROUTE, it) } + } + override fun onNewIntent(intent: Intent) { super.onNewIntent(intent) + // A genuinely new Intent has not been consumed, whatever the old one + // carried. + intent.removeExtra(EXTRA_EXTERNAL_ROUTE_CONSUMED) + consumedExternalRoute = null setIntent(intent) - val route = deviceLoginPairRouteOrNull(intent.dataString) + val activeServerUrl = get(ServerRegistry::class.java) + .activeEntry.value?.url + val deviceRoute = deviceLoginPairRouteOrNull(intent.dataString) + ?.takeIf { deviceLoginLinkIsForActiveServer(intent.dataString, activeServerUrl) } + val route = deviceRoute ?: inviteClaimRouteOrNull(intent.dataString) ?: notificationRouteOrNull(intent) ?: contentDeepLinkRouteOrNull(intent.dataString) - route?.let { pendingExternalRouteRequests.value = externalRouteRequestFactory.create(it) } + route?.let { + pendingExternalRouteRequests.value = externalRouteRequestFactory.create( + route = it, + requiredServerOrigin = if (it === deviceRoute) { + deviceLoginRequiredOrigin(intent.dataString) + } else { + null + }, + ) + } } /** @@ -250,11 +332,23 @@ class MainActivity : ComponentActivity() { * - All set → `Home` */ private suspend fun resolveStartDestination(): String { - deviceLoginPairRouteOrNull(intent?.dataString)?.let { return it } - val registry = get(ServerRegistry::class.java) val tokenManager = get(TokenManager::class.java) + // KNOWN GAP: a device link naming a configured-but-inactive server is + // refused silently — the user scans and nothing visible happens. Fixing + // that properly means a "this pairing request belongs to server X" + // surface with a switch action, which is a product decision. + // + // NOTE: a device link is deliberately NOT returned as the start + // destination. It used to be, which put Pair Device at the root of a + // signed-out app: its "Sign In" pushed Login, and the successful login + // then cleared the whole stack with popUpTo(0), losing the pairing + // request entirely. It is queued as a pending external route instead, + // so the normal server/token/profile gates run first and the pairing + // screen arrives on top of an authenticated stack — which also means + // its Back/Done has somewhere real to return to. + val activeEntry = registry.activeEntry.value ?: return Route.ServerSetup.route diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt index d2b0b27aa..88c777470 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt @@ -117,6 +117,7 @@ fun AppNavigation( onExternalRouteConsumed: (ExternalRouteRequest) -> Unit = {}, ) { val tokenManager: TokenManager = koinInject() + val serverRegistry: org.siloserver.silo.network.ServerRegistry = koinInject() val overlayPrefsStore: OverlayPrefsStore = koinInject() val siloCastController: SiloCastController = koinInject() val diagnosticsViewModel = koinViewModel() @@ -162,17 +163,38 @@ fun AppNavigation( ) }, navigate = { route -> - val replaceCurrentPlayer = shouldReplaceCurrentPlayer( - currentDestinationRoute = navController.currentBackStackEntry?.destination?.route, - targetRoute = route, - ) - navController.navigate(route) { - if (replaceCurrentPlayer) { - popUpTo(Route.Player.ROUTE) { inclusive = true } + // An external link to a TAB (silo://downloads) must switch tabs, + // not push a second copy of that tab. A duplicate tab entry also + // makes the tab anchor ambiguous: popUpTo(route) resolves to the + // NEWEST match, so the older anchor entry would survive and Back + // could loop through a hidden tab. + if (tabForRoute(route) != null) { + navController.navigate(route) { + tabSwitchNavOptions(navController.bottomMostTabRoute()) + } + } else { + val replaceCurrentPlayer = shouldReplaceCurrentPlayer( + currentDestinationRoute = navController.currentBackStackEntry + ?.destination?.route, + targetRoute = route, + ) + navController.navigate(route) { + if (replaceCurrentPlayer) { + popUpTo(Route.Player.ROUTE) { inclusive = true } + } + launchSingleTop = true } - launchSingleTop = true } }, + isStillValidForActiveServer = { requiredOrigin -> + // The user may have configured a DIFFERENT server while this + // request waited through setup/login. A pairing code looked up + // against the wrong server is worse than not looked up at all. + deviceLoginOriginMatchesServer( + requiredOrigin = requiredOrigin, + activeServerUrl = serverRegistry.activeEntry.value?.url, + ) + }, onConsumed = onExternalRouteConsumed, ) } @@ -327,10 +349,13 @@ fun AppNavigation( defaultValue = null }, ), - deepLinks = listOf( - navDeepLink { uriPattern = "silo://device?token={token}" }, - navDeepLink { uriPattern = "silo://device?code={code}" }, - ), + // Deliberately NO navDeepLink registrations. While they existed, + // Navigation matched the Activity's launch Intent itself when the + // graph was installed and landed Pair Device before any + // server/token/profile gate had run — the exact bypass + // MainActivity's pending-route queue exists to prevent. The + // manifest filter still delivers the Intent; MainActivity parses + // and queues it. ) { backStackEntry -> val token = backStackEntry.arguments?.getString("token") val code = backStackEntry.arguments?.getString("code") @@ -458,6 +483,13 @@ fun AppNavigation( LaunchedEffect(Unit) { navController.navigate(Route.Home.route) { popUpTo(legacyRoute) { inclusive = true } + // A restored stack can already hold Home IMMEDIATELY + // below the legacy entry; without this the redirect adds + // a second one, and a duplicate tab route makes the tab + // anchor ambiguous (popUpTo resolves to the newest + // match). Home further down is not collapsed — this + // checks the new top after the alias is popped. + launchSingleTop = true } } } diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/BottomNavBar.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/BottomNavBar.kt index 6761a0ea5..a234c3525 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/BottomNavBar.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/BottomNavBar.kt @@ -19,6 +19,8 @@ import androidx.compose.material3.NavigationBarItemDefaults import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.compositionLocalOf +import androidx.navigation.NavHostController +import androidx.navigation.NavOptionsBuilder import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.Brush import androidx.compose.ui.graphics.Color @@ -61,6 +63,53 @@ enum class Tab( ), } +/** + * The tab destination sitting lowest on the back stack — the anchor tab + * switching pops to. + * + * Derived from the live stack rather than remembered: `MainScreen` is composed + * per tab destination, so a remembered anchor gave every tab its own copy, and + * the graph's declared start destination keeps naming a tab even after that tab + * is removed. Popping to a route that is not on the stack pops nothing, so + * every tab tap stacked and Back walked back through previously visited tabs. + * + * This finds the oldest tab entry deterministically; what needs care is turning + * it back into a route string, because `popUpTo(route)` resolves to the NEWEST + * matching entry. The result is therefore unambiguous only while a tab route + * appears at most once. Every path in this build that can add a tab entry keeps + * that true: tab switching and the disappearing-tab cleanup both collapse to + * the anchor before pushing, external tab links switch rather than push, and the + * legacy-route aliases pop themselves inclusively before navigating, so their + * `launchSingleTop` sees Home on top when Home sat immediately below them. + * Duplicate tab routes are otherwise unsupported — a back stack restored from an + * older build could arrive holding them, and this does not repair that, so older + * tab entries may be left underneath. + */ +internal fun NavHostController.bottomMostTabRoute(): String? { + val tabRoutes = Tab.entries.mapTo(mutableSetOf()) { it.route } + return currentBackStack.value + .firstOrNull { entry -> entry.destination.route in tabRoutes } + ?.destination + ?.route +} + +/** The route's tab, if it is one. */ +internal fun tabForRoute(route: String): Tab? = Tab.entries.firstOrNull { it.route == route } + +/** + * Standard tab-switch options: replace the current tab rather than stack it, + * preserving each tab's own state. + * + * External links to a tab use these too, so `silo://downloads` behaves exactly + * like tapping Downloads — one definition of what entering a tab means, rather + * than two that drift. + */ +internal fun NavOptionsBuilder.tabSwitchNavOptions(anchorRoute: String?) { + anchorRoute?.let { popUpTo(it) { saveState = true } } + launchSingleTop = true + restoreState = true +} + /** * Material 3 bottom navigation bar themed for Silo's dark-first design. */ diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt index 4f7650e3c..67ed321d1 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt @@ -28,6 +28,103 @@ internal fun deviceLoginPairRouteOrNull(rawUri: String?): String? { return buildPairDeviceRoute(token = token, code = if (token == null) code else null) } +/** + * What server, if any, a device link names. + * + * The three cases must stay distinct. Collapsing "names no server" and "names + * something unparseable" into one null meant a malformed link such as + * `https:///device?code=...` — which still satisfies the device-path check but + * has no host — was treated as unscoped and accepted against whichever server + * was active, which is the behaviour this guard exists to remove. + */ +internal sealed interface DeviceLoginScope { + /** An app-scheme link (`silo://device`): names no server. */ + data object Unscoped : DeviceLoginScope + + /** An http(s) link naming [origin], already normalized. */ + data class Origin(val origin: String) : DeviceLoginScope + + /** Not a device link, or an http(s) one whose origin cannot be read. */ + data object Invalid : DeviceLoginScope +} + +internal fun deviceLoginScope(rawUri: String?): DeviceLoginScope { + val uri = rawUri + ?.takeIf { it.isNotBlank() } + ?.let { runCatching { URI(it) }.getOrNull() } + ?: return DeviceLoginScope.Invalid + if (!uri.isDeviceLoginUri()) return DeviceLoginScope.Invalid + val scheme = uri.scheme?.lowercase() ?: return DeviceLoginScope.Invalid + if (scheme != "http" && scheme != "https") return DeviceLoginScope.Unscoped + return uri.normalizedOrigin()?.let(DeviceLoginScope::Origin) ?: DeviceLoginScope.Invalid +} + +/** + * `scheme://host[:port]`, with the scheme's default port dropped so + * `https://silo.example` and `https://silo.example:443` compare equal. + */ +private fun URI.normalizedOrigin(): String? { + val scheme = scheme?.lowercase() ?: return null + val host = host?.lowercase()?.takeIf { it.isNotBlank() } ?: return null + val defaultPort = if (scheme == "https") 443 else 80 + // URI reports -1 for "omitted". Anything else must be a real port: 0 is not + // a valid origin and must not quietly compare equal to the default one. + val port = port + if (port != -1 && port !in 1..65535) return null + val explicitPort = port.takeIf { it != -1 && it != defaultPort } + return if (explicitPort != null) "$scheme://$host:$explicitPort" else "$scheme://$host" +} + +/** Whether [requiredOrigin] is the origin of [activeServerUrl]. */ +internal fun deviceLoginOriginMatchesServer( + requiredOrigin: String, + activeServerUrl: String?, +): Boolean { + val active = activeServerUrl + ?.takeIf { it.isNotBlank() } + ?.let { runCatching { URI(it) }.getOrNull() } + ?.normalizedOrigin() + ?: return false + return active == requiredOrigin +} + +/** The origin a device link requires, or null when it names no server. */ +internal fun deviceLoginRequiredOrigin(rawUri: String?): String? = + (deviceLoginScope(rawUri) as? DeviceLoginScope.Origin)?.origin + +/** + * True when a device link may be acted on for the currently active server. + * + * An http(s) device link identifies the server that issued the pairing request, + * but that origin used to be dropped on the floor: the code was then looked up + * against whatever server happened to be active, which normally reports the + * request as invalid or expired even though it is perfectly valid on the server + * the user actually scanned. Approving against the wrong server is worse than + * not approving, so a link naming a different origin is refused. + * + * With NO server configured yet there is nothing to contradict — the user is + * about to set one up, and having just scanned a server's code they are + * overwhelmingly likely to set up that one — so the link is allowed through the + * normal setup/login gates rather than dropped on the floor. + * + * Pairing against a configured-but-INACTIVE server would mean switching the + * active server from a link, which is a product decision rather than a parser + * one. It is refused here; see the caller for the known gap that this refusal + * is currently silent. + */ +internal fun deviceLoginLinkIsForActiveServer(rawUri: String?, activeServerUrl: String?): Boolean = + when (val scope = deviceLoginScope(rawUri)) { + DeviceLoginScope.Unscoped -> true + DeviceLoginScope.Invalid -> false + is DeviceLoginScope.Origin -> { + val active = activeServerUrl + ?.takeIf { it.isNotBlank() } + ?.let { runCatching { URI(it) }.getOrNull() } + ?.normalizedOrigin() + active == null || active == scope.origin + } + } + private fun URI.isDeviceLoginUri(): Boolean { val scheme = scheme?.lowercase() return when (scheme) { diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt index 442a1be43..bb58c78c1 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt @@ -12,15 +12,27 @@ import org.siloserver.silo.common.player.video.VideoPlayerRouteArgs class ExternalRouteRequest internal constructor( val generation: Long, val route: String, + /** + * The server origin this request is only valid against, or null if it is + * server-agnostic. + * + * A pairing link can be queued while NO server is configured, then wait + * through setup and login. Whatever server the user ends up on may not be + * the one that issued the code, so the origin has to survive the wait and + * be re-checked at delivery — dropping it here is how a code ends up looked + * up against the wrong server. + */ + val requiredServerOrigin: String? = null, ) internal class ExternalRouteRequestFactory { private var latestGeneration = 0L - fun create(route: String): ExternalRouteRequest = + fun create(route: String, requiredServerOrigin: String? = null): ExternalRouteRequest = ExternalRouteRequest( generation = ++latestGeneration, route = route, + requiredServerOrigin = requiredServerOrigin, ) } @@ -155,6 +167,11 @@ internal suspend fun consumeExternalRouteOnce( pendingExternalRoute: ExternalRouteRequest?, currentDestinationRoutes: Flow, isAlreadyAtRoute: (String) -> Boolean = { false }, + /** + * Whether [ExternalRouteRequest.requiredServerOrigin] still matches the + * active server. Evaluated after the wait, not before it. + */ + isStillValidForActiveServer: suspend (String) -> Boolean = { true }, navigate: (String) -> Unit, onConsumed: (ExternalRouteRequest) -> Unit, ) { @@ -165,8 +182,13 @@ internal suspend fun consumeExternalRouteOnce( currentDestinationRoutes.first { currentRoute -> isPreAuthenticationTarget || currentRoute !in preAuthenticationDestinationRoutes } - if (!isAlreadyAtRoute(route)) { + val originStillValid = request.requiredServerOrigin + ?.let { origin -> isStillValidForActiveServer(origin) } + ?: true + if (originStillValid && !isAlreadyAtRoute(route)) { navigate(route) } + // Consumed either way: a request whose server no longer matches must not + // sit in the queue waiting to fire at some later, equally wrong moment. onConsumed(request) } diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt index b8fd00f56..1bd51dccf 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt @@ -90,8 +90,11 @@ sealed class Route(val route: String) { } } - // Canonical tab routes — Home is the start destination and the bottom-nav / - // popUpTo anchor; Libraries and Recommendations back the other media tabs. + // Canonical tab routes. Home is the USUAL start destination, but not + // always: an offline launch with downloads starts on Downloads instead, so + // the bottom-nav popUpTo anchor is read from the live back stack + // ([bottomMostTabRoute]) rather than assumed to be Home. Libraries and + // Recommendations back the other media tabs. data object Home : Route("home") data object Libraries : Route("libraries") data object Recommendations : Route("recommendations") diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/MainScreen.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/MainScreen.kt index fa6171727..e689b31b6 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/MainScreen.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/MainScreen.kt @@ -40,6 +40,9 @@ import org.siloserver.silo.android.ui.navigation.LocalBottomChromeInset import org.siloserver.silo.android.ui.navigation.SiloBottomNavBar import org.siloserver.silo.android.ui.navigation.Route import org.siloserver.silo.android.ui.navigation.Tab +import org.siloserver.silo.android.ui.navigation.tabForRoute +import org.siloserver.silo.android.ui.navigation.tabSwitchNavOptions +import org.siloserver.silo.android.ui.navigation.bottomMostTabRoute import org.siloserver.silo.android.ui.navigation.fallbackMobileTab import org.siloserver.silo.android.ui.navigation.scopedLocalDownloadBytes import org.siloserver.silo.android.ui.navigation.shouldShowDownloadsTab @@ -190,15 +193,56 @@ fun MainScreen( // If the user is on a tab no longer supported by their libraries (or // Downloads disappears), move them to the nearest visible media tab. + // A tab that can no longer be shown must not be left on the stack: no entry + // at the bottom for Back to reveal (its own effect would bounce straight + // back, trapping the user), and no saved subtree for a later reappearance to + // restore into. This can only act while a tab is composed — with a detail + // page covering it, cleanup waits until Back returns here. + // + // Deliberately no saveState/restoreState on this path. Saving the vanishing + // tab and then restoring on the way to the replacement is self-defeating: + // restoreState is evaluated before launchSingleTop, so navigating to Home + // immediately restored the Downloads subtree that had just been popped. LaunchedEffect(currentTab, visibleTabs) { - if (currentTab !in visibleTabs) { - val fallback = fallbackMobileTab(visibleTabs, currentTab) ?: Tab.Home - navController.navigate(fallback.route) { - popUpTo(Route.Home.route) { saveState = true } - launchSingleTop = true - restoreState = true + val anchorRoute = navController.bottomMostTabRoute() + val anchorTab = anchorRoute?.let(::tabForRoute) + val vanished = when { + currentTab !in visibleTabs -> currentTab + // The ANCHOR can vanish while the user is on some other tab. Nothing + // above it changed, so this is the only chance to notice. + anchorTab != null && anchorTab !in visibleTabs -> anchorTab + else -> null + } ?: return@LaunchedEffect + + val target = if (vanished == currentTab) { + fallbackMobileTab(visibleTabs, currentTab) ?: Tab.Home + } else { + // Re-rooting onto the tab in use also destroys its entry, losing + // scroll position. Accepted: the alternative leaves an unreachable + // root that Back can surface. + currentTab + } + + navController.navigate(target.route) { + // Pop to the ANCHOR, not merely to the vanished tab. Popping just + // the vanished one leaves any other tab entries below it in place, + // and pushing the target then adds a SECOND copy of a tab already + // down there — the duplicate that makes the anchor ambiguous. + // Collapsing to the anchor first keeps at most one entry per tab, + // and launchSingleTop absorbs the case where the target IS the + // anchor. + if (vanished == anchorTab) { + popUpTo(vanished.route) { inclusive = true } + } else { + anchorRoute?.let { popUpTo(it) { inclusive = false } } } + launchSingleTop = true } + // Drop any subtree saved for it by an earlier ordinary tab switch — + // popping without saveState does not clear existing mappings, and a + // reappearing Downloads would otherwise restore a stale stack and land + // the user on a different tab entirely. + navController.clearBackStack(vanished.route) } LaunchedEffect(activeEntry?.id, activeEntry?.profileId, headerState.activeProfile?.id) { @@ -251,9 +295,14 @@ fun MainScreen( homeScrollToTopTick += 1 } else { navController.navigate(tab.route) { - popUpTo(Route.Home.route) { saveState = true } - launchSingleTop = true - restoreState = true + // Pop to the tab stack's live anchor, not a + // hard-coded Home and not the graph's declared + // start (which can name a tab that has since + // been removed). Popping to a route that is not + // on the stack pops nothing — every tab then + // stacked, so Back walked back through + // previously visited tabs instead of leaving. + tabSwitchNavOptions(navController.bottomMostTabRoute()) } } }, diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.kt index ff4a1f4ee..c3b8bebe3 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.kt @@ -1,6 +1,8 @@ package org.siloserver.silo.android.ui.navigation import kotlin.test.Test +import kotlin.test.assertTrue +import kotlin.test.assertFalse import kotlin.test.assertEquals import kotlin.test.assertNull @@ -57,4 +59,137 @@ class DeviceLoginRouteParserTest { assertNull(deviceLoginPairRouteOrNull("")) assertNull(deviceLoginPairRouteOrNull("silo://device?token=&code=")) } + + // --- origin matching --- + + /** + * An HTTPS device link names the server that issued the pairing request. + * That origin used to be dropped, so the code was looked up against + * whichever server happened to be active — normally reporting a perfectly + * valid request as invalid, and at worst approving against the wrong + * server. + */ + @Test + fun `an https device link for another server is not for the active one`() { + assertFalse( + deviceLoginLinkIsForActiveServer( + rawUri = "https://server-b.example/auth/device?code=ABCD", + activeServerUrl = "https://server-a.example", + ), + ) + } + + @Test + fun `an https device link for the active server is accepted`() { + assertTrue( + deviceLoginLinkIsForActiveServer( + rawUri = "https://server-a.example/auth/device?code=ABCD", + activeServerUrl = "https://server-a.example", + ), + ) + } + + @Test + fun `a port difference is a different origin`() { + assertFalse( + deviceLoginLinkIsForActiveServer( + rawUri = "https://silo.example:8443/device?code=ABCD", + activeServerUrl = "https://silo.example", + ), + ) + } + + @Test + fun `matching host and port is the same origin`() { + assertTrue( + deviceLoginLinkIsForActiveServer( + rawUri = "https://silo.example:8443/device?code=ABCD", + activeServerUrl = "https://silo.example:8443/", + ), + ) + } + + /** An app-scheme link names no server, so it is about the active one. */ + @Test + fun `an app scheme device link names no server`() { + assertTrue( + deviceLoginLinkIsForActiveServer( + rawUri = "silo://device?code=ABCD", + activeServerUrl = "https://server-a.example", + ), + ) + assertEquals(DeviceLoginScope.Unscoped, deviceLoginScope("silo://device?code=ABCD")) + } + + /** + * With no server configured there is nothing to contradict, and the user is + * about to set one up — almost certainly the one they just scanned. Dropping + * the link here would turn a valid action into silence. + */ + @Test + fun `an https device link with no active server is allowed through setup`() { + assertTrue( + deviceLoginLinkIsForActiveServer( + rawUri = "https://server-b.example/device?code=ABCD", + activeServerUrl = null, + ), + ) + } + + /** Default ports are equivalent to omitting them. */ + @Test + fun `an explicit default port is the same origin`() { + assertTrue( + deviceLoginLinkIsForActiveServer( + rawUri = "https://silo.example:443/device?code=ABCD", + activeServerUrl = "https://silo.example", + ), + ) + } + + /** + * A device-shaped link with no readable host must not fall back to "use + * whichever server is active" — that is the behaviour being removed. + */ + @Test + fun `an https device link with no host is refused`() { + assertEquals(DeviceLoginScope.Invalid, deviceLoginScope("https:///device?code=ABCD")) + assertFalse( + deviceLoginLinkIsForActiveServer( + rawUri = "https:///device?code=ABCD", + activeServerUrl = "https://server-a.example", + ), + ) + } + + /** Port 0 is not a valid origin and must not pass as "default port". */ + @Test + fun `port zero is not an origin`() { + assertEquals(DeviceLoginScope.Invalid, deviceLoginScope("https://silo.example:0/device?code=A")) + assertFalse( + deviceLoginLinkIsForActiveServer( + rawUri = "https://silo.example:0/device?code=A", + activeServerUrl = "https://silo.example", + ), + ) + } + + /** + * The origin has to survive the wait through setup/login: a pairing request + * queued with no server configured must not fire against whatever server + * the user happens to end up on. + */ + @Test + fun `a required origin only matches its own server`() { + val origin = deviceLoginRequiredOrigin("https://server-b.example/device?code=A") + assertEquals("https://server-b.example", origin) + assertTrue(deviceLoginOriginMatchesServer(origin!!, "https://server-b.example")) + assertFalse(deviceLoginOriginMatchesServer(origin, "https://server-a.example")) + assertFalse(deviceLoginOriginMatchesServer(origin, null)) + } + + @Test + fun `an app scheme link requires no origin`() { + assertNull(deviceLoginRequiredOrigin("silo://device?code=A")) + } } diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt index 16533a0c8..9fb6f80d0 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt @@ -293,4 +293,67 @@ class ExternalRouteNavigationTest { subtitleTrackIndex = subtitleTrackIndex, resumePositionSeconds = resumePositionSeconds, ) + + /** + * A pairing link can be queued with NO server configured and then wait + * through setup and login. Whatever server the user lands on may not be the + * one that issued the code, so the origin is re-checked at delivery — and a + * mismatch must not navigate. + */ + @Test + fun aRequestWhoseServerNoLongerMatchesIsNotDelivered() = runTest { + val request = ExternalRouteRequestFactory() + .create(route = "pair_device?code=ABCD", requiredServerOrigin = "https://server-b") + var navigated: String? = null + var consumed = 0 + + consumeExternalRouteOnce( + pendingExternalRoute = request, + currentDestinationRoutes = flowOf("home"), + isStillValidForActiveServer = { false }, + navigate = { navigated = it }, + onConsumed = { consumed++ }, + ) + + assertNull(navigated, "a code must never be looked up against the wrong server") + // Still consumed: leaving it queued would only let it fire later, at an + // equally wrong moment. + assertEquals(1, consumed) + } + + @Test + fun aRequestWhoseServerStillMatchesIsDelivered() = runTest { + val request = ExternalRouteRequestFactory() + .create(route = "pair_device?code=ABCD", requiredServerOrigin = "https://server-b") + var navigated: String? = null + var consumed = 0 + + consumeExternalRouteOnce( + pendingExternalRoute = request, + currentDestinationRoutes = flowOf("home"), + isStillValidForActiveServer = { true }, + navigate = { navigated = it }, + onConsumed = { consumed++ }, + ) + + assertEquals("pair_device?code=ABCD", navigated) + assertEquals(1, consumed) + } + + /** An unscoped request must not be gated on any server. */ + @Test + fun anUnscopedRequestIgnoresTheServerCheck() = runTest { + val request = ExternalRouteRequestFactory().create(route = "item/abc") + var navigated: String? = null + + consumeExternalRouteOnce( + pendingExternalRoute = request, + currentDestinationRoutes = flowOf("home"), + isStillValidForActiveServer = { error("must not be consulted for an unscoped route") }, + navigate = { navigated = it }, + onConsumed = { }, + ) + + assertEquals("item/abc", navigated) + } } diff --git a/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt b/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt index 5e38fc56d..64b6b97f9 100644 --- a/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt +++ b/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt @@ -250,6 +250,14 @@ class EncryptedTokenManagerImpl( } } + override suspend fun getProfileIdentity(): ProfileIdentity = mutex.withLock { + ensureCacheMatchesRegistryLocked() + temporaryScope?.let { scope -> + return@withLock ProfileIdentity(scope.profileId, scope.profileToken) + } + ProfileIdentity(profileId, profileToken) + } + /** * One lock, one preferences edit, so the PERSISTED id and token cannot * disagree even if the process dies immediately after. Concurrent readers diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt index 353ec1c4d..e173bdcd6 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt @@ -134,8 +134,11 @@ val SiloAuthPlugin = createClientPlugin("SiloAuthPlugin", ::SiloAuthConfig) { if (isRefreshRequest) return@onRequest val accessToken = tokenManager.getAccessToken() - val profileId = tokenManager.getProfileId() - val profileToken = tokenManager.getProfileToken() + // One read: taking these separately could pair the old profile id with + // the new profile's token across a switch. + val profileIdentity = tokenManager.getProfileIdentity() + val profileId = profileIdentity.profileId + val profileToken = profileIdentity.profileToken val activeServerIdAfter = tokenManager.getCurrentServerId() val activeServerUrlAfter = tokenManager.getServerUrl() if ( diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/network/PlaybackRealtimeClient.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/network/PlaybackRealtimeClient.kt index bc69a6664..6814a6e93 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/network/PlaybackRealtimeClient.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/network/PlaybackRealtimeClient.kt @@ -112,8 +112,9 @@ class DefaultPlaybackRealtimeClient( close() return@callbackFlow } - val profileId = tokenManager.getProfileId() - val profileToken = tokenManager.getProfileToken() + val profileIdentity = tokenManager.getProfileIdentity() + val profileId = profileIdentity.profileId + val profileToken = profileIdentity.profileToken val url = buildString { append("/api/v1/playback/sessions/") append(sessionId.encodeURLParameter()) diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt index 0551248b3..6ba1a08fd 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.kt @@ -19,6 +19,9 @@ data class TemporaryAuthScope( "profileToken=, expiresAtEpochMs=$expiresAtEpochMs)" } +/** A profile id and the token that proves it, read together. */ +data class ProfileIdentity(val profileId: String?, val profileToken: String?) + /** * Manages JWT access and refresh tokens. * Implementation provided by Agent 2 in TokenManagerImpl.kt. @@ -76,6 +79,26 @@ interface TokenManager { suspend fun getProfileToken(): String? suspend fun setProfileToken(token: String?) + /** + * Read the profile id and its token as ONE identity. + * + * [setProfileIdentity] makes the write atomic, but a reader taking the two + * getters separately can still interleave with a switch and pair the old id + * with the new token — sending headers that claim one profile while + * presenting another's proof, which is exactly what that write fixed. + * Anything assembling both into a request must use this. + * + * The default is the non-atomic pair so simple/test managers keep working; + * managers with real locking override it to read under one lock. + * + * A wrapper using `TokenManager by delegate` MUST override this too: Kotlin + * interface delegation forwards default methods to the delegate, so + * overriding only [getProfileId]/[getProfileToken] leaves this reading the + * delegate's identity instead — silently, with tests still green. + */ + suspend fun getProfileIdentity(): ProfileIdentity = + ProfileIdentity(profileId = getProfileId(), profileToken = getProfileToken()) + /** * Commit a profile id and its matching profile token as ONE identity. * diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.kt index 072bbdb75..885c45bd4 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.kt @@ -135,6 +135,13 @@ class TokenManagerImpl( } } + override suspend fun getProfileIdentity(): ProfileIdentity = mutex.withLock { + temporaryScope?.let { scope -> + return@withLock ProfileIdentity(scope.profileId, scope.profileToken) + } + ProfileIdentity(profileId, profileToken) + } + /** Single lock so the stored pair is written together; see [TokenManager]. */ override suspend fun setProfileIdentity(profileId: String?, profileToken: String?) { mutex.withLock { diff --git a/shared/src/commonTest/kotlin/org/siloserver/silo/network/SiloAuthPluginPinTest.kt b/shared/src/commonTest/kotlin/org/siloserver/silo/network/SiloAuthPluginPinTest.kt index 4dae65339..cd6ba2b2e 100644 --- a/shared/src/commonTest/kotlin/org/siloserver/silo/network/SiloAuthPluginPinTest.kt +++ b/shared/src/commonTest/kotlin/org/siloserver/silo/network/SiloAuthPluginPinTest.kt @@ -693,6 +693,12 @@ class SiloAuthPluginPinTest { override suspend fun getProfileId(): String = "server-b-profile" override suspend fun getProfileToken(): String = "server-b-profile-token" + + // Interface delegation forwards the DEFAULT getProfileIdentity() to the + // delegate, silently bypassing the two overrides above — so anything + // reading the identity as a pair would test the wrong values. + override suspend fun getProfileIdentity(): ProfileIdentity = + ProfileIdentity(getProfileId(), getProfileToken()) } private class InFlightSwitchingTokenManager( @@ -716,6 +722,9 @@ class SiloAuthPluginPinTest { override suspend fun getProfileId(): String = "$activeServer-profile" + override suspend fun getProfileIdentity(): ProfileIdentity = + ProfileIdentity(getProfileId(), getProfileToken()) + override suspend fun getProfileToken(): String = "$activeServer-profile-token" override suspend fun invalidateSession() { From 161f0eafa374008e56e972f85dbd82f4f7c76eaa Mon Sep 17 00:00:00 2001 From: rxwatcher Date: Thu, 6 Aug 2026 00:03:56 +0200 Subject: [PATCH 4/6] fix(nav): attribute external routes to the identity that created them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the three items left open after the navigation work. The flaky player test. It was not load — it failed 2 in 6 on an idle machine. Its wait helper hopped to Dispatchers.Default and polled in real time while the work ran on UnconfinedTestDispatcher, which resumes inline or queues on the thread's unconfined event loop; the poller could win that race and time out with the continuation still queued. Every test in the class was exposed. It now uses StandardTestDispatcher, waits on replayable StateFlow signals instead of polling, and keeps a real-time deadline only to turn a hang into a failure. That deadline is 30s, not 5s: five was tight enough that a full-suite run could blow it while the work was merely slow, which looks exactly like the race being removed. Also fixed a test whose final wait was already true when called, so it asserted nothing. Mutation-verified twice; 12 consecutive runs clean. Notifications acted on whoever was signed in when they were tapped, which for a PendingIntent can be days and several profile switches later. They now carry the identity that generated them, established from the fetched row and a scope that held across the fetch, or not at all — a partial identity is worse than none, because a missing component is a wildcard at delivery. Unattributable ones still post so the user knows something happened, but carry no route, no identity and no item text. Delivery independently requires a complete identity, so a notification from an older build or an Intent crafted against the exported Activity cannot navigate. Content deep links are server-local, so they pin to the identity active when the link arrives; arriving signed-out pins nothing, which is what still lets a link opened before login work after it. Invite claims stay unpinned — they carry their own target server and are meant to work pre-auth. A pairing link naming a server other than the active one was refused silently: the user scanned a code and nothing happened. The route now carries its issuing origin and the screen says "This pairing request is for " with a switch action, or offers to add an unknown one. Refusing to look a code up against the wrong server is preserved — the screen owns that check now. The request survives the sign-in the switch may require, on all three paths that can trigger one. Two production bugs found while doing it: snapshotCurrentScope was the one identity read that did not reconcile with the registry first, so straight after a registry-driven switch it described the previous server; and a device-shaped link whose origin could not be read parsed into a route with no origin, which downstream reads as "names no server" and pairs against whatever is active. Known limit, recorded in the code: the push payload carries no issuing server and the FCM token stays registered with previously-active servers, so a push from A arriving while B is active cannot be attributed at all and posts non-navigable. Fixing that needs issuer fields in the push protocol — server-side work. Seven review rounds. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW --- .../siloserver/silo/android/MainActivity.kt | 157 +++++++++++----- .../silo/android/di/AndroidModule.kt | 1 + .../android/push/PushNotificationPresenter.kt | 109 ++++++++++- .../android/ui/navigation/AppNavigation.kt | 176 +++++++++++++++--- .../ui/navigation/DeviceLoginRouteParser.kt | 70 +++---- .../ui/navigation/DeviceLoginServerMatch.kt | 46 +++++ .../ui/navigation/ExternalRouteNavigation.kt | 66 ++++--- .../navigation/NotificationExternalRoute.kt | 25 +++ .../silo/android/ui/navigation/Routes.kt | 10 +- .../auth/DevicePairingWrongServerScreen.kt | 116 ++++++++++++ .../push/PushNotificationAttributionTest.kt | 120 ++++++++++++ .../navigation/DeviceLoginRouteParserTest.kt | 138 +++----------- .../navigation/DeviceLoginServerMatchTest.kt | 93 +++++++++ .../navigation/ExternalRouteNavigationTest.kt | 72 +++++-- .../NotificationExternalRouteTest.kt | 71 +++++++ ...erViewModelLoadOwnershipIntegrationTest.kt | 68 +++++-- .../silo/network/EncryptedTokenManagerImpl.kt | 8 + ...ncryptedTokenManagerScopeGenerationTest.kt | 48 +++++ 18 files changed, 1113 insertions(+), 281 deletions(-) create mode 100644 androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatch.kt create mode 100644 androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRoute.kt create mode 100644 androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/DevicePairingWrongServerScreen.kt create mode 100644 androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationAttributionTest.kt create mode 100644 androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatchTest.kt create mode 100644 androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.kt diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt index 664596e23..1597e1d7a 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt @@ -38,9 +38,9 @@ import org.siloserver.silo.android.ui.navigation.ExternalRouteRequestFactory import org.siloserver.silo.android.ui.navigation.Route import org.siloserver.silo.android.ui.navigation.clearConsumedExternalRouteRequest import org.siloserver.silo.android.ui.navigation.contentDeepLinkRouteOrNull -import org.siloserver.silo.android.ui.navigation.deviceLoginLinkIsForActiveServer +import org.siloserver.silo.android.ui.navigation.ExternalRouteScope +import org.siloserver.silo.android.ui.navigation.notificationExternalRouteOrNull import org.siloserver.silo.android.ui.navigation.deviceLoginPairRouteOrNull -import org.siloserver.silo.android.ui.navigation.deviceLoginRequiredOrigin import org.siloserver.silo.android.ui.navigation.hasLocalDownloadsForScope import org.siloserver.silo.android.ui.navigation.inviteClaimRouteOrNull import org.siloserver.silo.android.ui.navigation.notificationNavigationRouteOrNull @@ -91,6 +91,12 @@ class MainActivity : ComponentActivity() { private const val EXTRA_EXTERNAL_ROUTE_CONSUMED = "org.siloserver.silo.EXTERNAL_ROUTE_CONSUMED" + /** + * Stands in for an active server whose identity could not be read, so a + * scope built from it matches nothing instead of everything. + */ + private const val UNRESOLVED_IDENTITY = "silo:unresolved-identity" + /** Saved-state key for [consumedExternalRoute]. */ private const val STATE_CONSUMED_EXTERNAL_ROUTE = "org.siloserver.silo.CONSUMED_EXTERNAL_ROUTE" @@ -141,31 +147,7 @@ class MainActivity : ComponentActivity() { // Skip an Intent whose route was already delivered: it is only // still here because the Activity retains it. if (intent?.getBooleanExtra(EXTRA_EXTERNAL_ROUTE_CONSUMED, false) != true) { - val activeServerUrl = get(ServerRegistry::class.java) - .activeEntry.value?.url - val deviceRoute = deviceLoginPairRouteOrNull(intent?.dataString) - ?.takeIf { - deviceLoginLinkIsForActiveServer(intent?.dataString, activeServerUrl) - } - val externalRoute = notificationRouteOrNull(intent) - ?: contentDeepLinkRouteOrNull(intent?.dataString) - ?: deviceRoute - externalRoute - ?.takeIf { it != consumedExternalRoute } - ?.let { route -> - pendingExternalRouteRequests.value = - externalRouteRequestFactory.create( - route = route, - // Only a device link is server-scoped, and it - // may wait here through setup/login onto a - // server that did not issue it. - requiredServerOrigin = if (route === deviceRoute) { - deviceLoginRequiredOrigin(intent?.dataString) - } else { - null - }, - ) - } + queueExternalRouteFrom(intent) } launchAuthenticatedStartupWarmup(route) } @@ -202,6 +184,15 @@ class MainActivity : ComponentActivity() { AppNavigation( startDestination = resolvedRoute, pendingExternalRoute = pendingExternalRoute, + onRequeueExternalRoute = { route -> + // A fresh request: clear the consumed marker so + // this re-delivery is not mistaken for the + // already-followed original. + consumedExternalRoute = null + intent?.removeExtra(EXTRA_EXTERNAL_ROUTE_CONSUMED) + pendingExternalRouteRequests.value = + externalRouteRequestFactory.create(route) + }, onExternalRouteConsumed = { consumedRequest -> // Record the delivery in two places. The Intent // extra covers in-process Activity recreation, @@ -247,24 +238,7 @@ class MainActivity : ComponentActivity() { intent.removeExtra(EXTRA_EXTERNAL_ROUTE_CONSUMED) consumedExternalRoute = null setIntent(intent) - val activeServerUrl = get(ServerRegistry::class.java) - .activeEntry.value?.url - val deviceRoute = deviceLoginPairRouteOrNull(intent.dataString) - ?.takeIf { deviceLoginLinkIsForActiveServer(intent.dataString, activeServerUrl) } - val route = deviceRoute - ?: inviteClaimRouteOrNull(intent.dataString) - ?: notificationRouteOrNull(intent) - ?: contentDeepLinkRouteOrNull(intent.dataString) - route?.let { - pendingExternalRouteRequests.value = externalRouteRequestFactory.create( - route = it, - requiredServerOrigin = if (it === deviceRoute) { - deviceLoginRequiredOrigin(intent.dataString) - } else { - null - }, - ) - } + lifecycleScope.launch { queueExternalRouteFrom(intent) } } /** @@ -310,6 +284,94 @@ class MainActivity : ComponentActivity() { requestLegacyPublicDownloadPermission.launch(LEGACY_PUBLIC_DOWNLOAD_PERMISSION) } + + /** + * Parses an Intent into a pending external route, tagged with the identity + * it is only meaningful under. + * + * Everything that can wait through authentication has to declare its scope, + * because "wait" can mean days for a notification PendingIntent and several + * profile switches: + * - a pairing link names its issuing SERVER ORIGIN; + * - a notification was generated for one profile's inbox on one server, so + * it carries the identity stamped on it at post time; + * - a content link (`silo://item`, `silo://play`) carries no identity of + * its own, but its ids are server-local — so it is pinned to whoever is + * signed in when the link arrives. Arriving signed-out pins nothing, + * which is what lets a link opened before login still work after it. + */ + private suspend fun queueExternalRouteFrom(intent: Intent?) { + if (intent?.getBooleanExtra(EXTRA_EXTERNAL_ROUTE_CONSUMED, false) == true) return + + // NOT gated on the active server. The route carries its issuing origin + // and the pairing destination refuses — and explains — a mismatch, with + // a switch action. Dropping it here was silent: the user scanned a code + // and nothing happened. A link whose origin cannot be read does not + // parse into a route at all. + val deviceRoute = deviceLoginPairRouteOrNull(intent?.dataString) + // Rejected outright unless it says whose it is — see + // [notificationExternalRouteOrNull]. + val notification = notificationExternalRouteOrNull( + route = notificationRouteOrNull(intent), + serverId = intent?.getStringExtra(PushNotificationPresenter.EXTRA_SERVER_ID), + profileId = intent?.getStringExtra(PushNotificationPresenter.EXTRA_PROFILE_ID), + ) + val notificationRoute = notification?.first + val contentRoute = contentDeepLinkRouteOrNull(intent?.dataString) + val inviteRoute = inviteClaimRouteOrNull(intent?.dataString) + + val route = notificationRoute ?: contentRoute ?: deviceRoute ?: inviteRoute ?: return + if (route == consumedExternalRoute) return + + val scope = when { + // Unscoped for DELIVERY: the pairing screen owns the server check, + // so the request must actually arrive for it to be explained. + route === deviceRoute -> ExternalRouteScope.Unscoped + // Non-null by construction: `route` is only this when `notification` + // produced it, and that requires a complete identity. + route === notificationRoute -> checkNotNull(notification).second + route === contentRoute -> currentIdentityScope() + // An invite claim carries its own target server and is designed to + // work before authentication, so it must NOT be pinned to the + // current identity. + else -> ExternalRouteScope.Unscoped + } + + pendingExternalRouteRequests.value = + externalRouteRequestFactory.create(route = route, scope = scope) + } + + /** + * One cohesive read of the live identity. + * + * Reading the server and profile through separate getters could tear across + * a switch — the cached server id from before it, the profile id from after + * — producing a hybrid identity that belongs to nobody, which then either + * consumes a valid one-shot route or weakens it with a null wildcard. + */ + private suspend fun currentIdentityScope(): ExternalRouteScope { + val scope = get(TokenManager::class.java).snapshotCurrentScope() + if (scope != null) { + return ExternalRouteScope.Identity( + serverId = scope.serverId, + profileId = scope.profileId, + ) + } + // A null snapshot means "no active server" — nothing to pin to, and the + // link must survive setup and login. But it ALSO means "snapshotting + // failed" or "this manager does not model scopes", and turning those + // into a wildcard would quietly unpin a link that should have been + // pinned. Only an actually-absent server is allowed to be unpinned. + val registry = get(ServerRegistry::class.java) + return if (registry.activeServerId.value == null) { + ExternalRouteScope.Identity(serverId = null, profileId = null) + } else { + // An active server we cannot describe: pin to something nothing + // matches rather than to everything. + ExternalRouteScope.Identity(serverId = UNRESOLVED_IDENTITY, profileId = null) + } + } + private fun notificationRouteOrNull(intent: Intent?): String? = notificationNavigationRouteOrNull( intent?.getStringExtra(PushNotificationPresenter.EXTRA_NAV_ROUTE), @@ -335,11 +397,6 @@ class MainActivity : ComponentActivity() { val registry = get(ServerRegistry::class.java) val tokenManager = get(TokenManager::class.java) - // KNOWN GAP: a device link naming a configured-but-inactive server is - // refused silently — the user scans and nothing visible happens. Fixing - // that properly means a "this pairing request belongs to server X" - // surface with a switch action, which is a product decision. - // // NOTE: a device link is deliberately NOT returned as the start // destination. It used to be, which put Pair Device at the root of a // signed-out app: its "Sign In" pushed Login, and the successful login diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.kt index afc34ede7..a247465a6 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.kt @@ -216,6 +216,7 @@ val androidModule = module { PushNotificationPresenter( context = androidContext(), notificationsRepository = get(), + tokenManager = get(), ) } single { PushMessageHandler(presenter = get()) } diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushNotificationPresenter.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushNotificationPresenter.kt index 58eff1d44..81f0c2254 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushNotificationPresenter.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushNotificationPresenter.kt @@ -16,17 +16,26 @@ import org.siloserver.silo.android.ui.navigation.Route import org.siloserver.silo.model.notifications.NotificationRow import org.siloserver.silo.model.notifications.NotificationType import org.siloserver.silo.network.ApiResult +import org.siloserver.silo.network.TokenManager import org.siloserver.silo.repository.NotificationsRepository class PushNotificationPresenter( private val context: Context, private val notificationsRepository: NotificationsRepository, + private val tokenManager: TokenManager, ) { suspend fun present( deliveryId: String, fallbackTitle: String? = null, fallbackBody: String? = null, ) { + // Captured BEFORE the fetch, which can take seconds: reading identity + // afterwards attributed the notification to whatever the user had + // switched to in the meantime. The scope snapshot (not just the server + // id) is what detects an A→B→A round trip across the fetch, which a + // plain id comparison reports as "unchanged". + val scopeBeforeFetch = tokenManager.snapshotCurrentScope() + // Fetch before the permission check: on a direct-lookup miss the fallback // refreshes the inbox, which keeps the in-app badge current even for a // profile that has denied POST_NOTIFICATIONS. @@ -34,15 +43,61 @@ class PushNotificationPresenter( if (!canPostNotifications()) return ensureChannel() + // Stamp the identity this notification belongs to. A notification is + // generated for one profile's inbox on one server, and its route (an + // item id, or the inbox itself) means something different — or nothing + // — under another. Without this the tap acted on whoever was signed in + // when it was opened, which for a PendingIntent can be days later and + // several profile switches away. + // + // The issuer must be established COMPLETELY or not at all. A partial + // identity is worse than none: a null component is treated as a + // wildcard at delivery, so a half-attributed notification can act under + // an identity that never generated it. + // + // The fetched row is authoritative for the profile — it IS the row from + // that profile's inbox. The server is only trusted if the scope did not + // move across the fetch. + // + // KNOWN LIMIT: the push payload carries no issuing server, and the FCM + // token stays registered with previously-active servers, so a push from + // server A arriving while B is active simply misses its lookup — it + // cannot be attributed to A at all, and is posted non-navigable below. + // Attributing it to B is what this used to do, and is wrong. Fixing it + // properly needs issuer fields in the push protocol: server-side work. + val scopeAfterFetch = tokenManager.snapshotCurrentScope() + val attribution = pushNotificationAttribution( + rowProfileId = row?.profileId, + serverIdBefore = scopeBeforeFetch?.serverId, + identityGenerationBefore = scopeBeforeFetch?.identityGeneration, + serverIdAfter = scopeAfterFetch?.serverId, + identityGenerationAfter = scopeAfterFetch?.identityGeneration, + ) + val issuingServerId = attribution?.serverId + val issuingProfileId = attribution?.profileId + val attributable = attribution != null + val content = notificationContentFor( - row = row, - fallbackTitle = fallbackTitle, - fallbackBody = fallbackBody, + // Both sources of text are withheld when we cannot say whose this + // is. The ROW matters as much as the payload: if the identity moved + // across the fetch, its series/episode details belong to whoever we + // just stopped being. An unattributable notification is generic as + // well as non-navigable. + row = row.takeIf { attributable }, + fallbackTitle = fallbackTitle.takeIf { attributable }, + fallbackBody = fallbackBody.takeIf { attributable }, ) val contentIntent = Intent(context, MainActivity::class.java).apply { flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP putExtra(EXTRA_DELIVERY_ID, deliveryId) - putExtra(EXTRA_NAV_ROUTE, content.route) + // Only navigable when we know whose it is. Unattributable ones still + // post — the user should see the event — but tapping just opens the + // app rather than acting on someone else's library. + if (attributable) { + putExtra(EXTRA_NAV_ROUTE, content.route) + putExtra(EXTRA_SERVER_ID, issuingServerId) + putExtra(EXTRA_PROFILE_ID, issuingProfileId) + } } val notification = NotificationCompat.Builder(context, CHANNEL_ID) .setSmallIcon(R.mipmap.ic_launcher) @@ -97,7 +152,10 @@ class PushNotificationPresenter( if (row == null) { return PushNotificationContent( title = fallbackTitle?.takeIf { it.isNotBlank() } ?: "Silo notification", - body = fallbackBody?.takeIf { it.isNotBlank() } ?: "Open Silo to view it.", + // Deliberately does not promise this specific event is visible + // under the active identity — it may not be ours to show. + body = fallbackBody?.takeIf { it.isNotBlank() } + ?: "Open Silo to check notifications.", route = Route.Inbox.route, ) } @@ -164,6 +222,10 @@ class PushNotificationPresenter( const val CHANNEL_ID = "silo_notifications" const val EXTRA_DELIVERY_ID = "silo_notification_delivery_id" const val EXTRA_NAV_ROUTE = "silo_notification_nav_route" + + /** Identity the notification was generated for; see [present]. */ + const val EXTRA_SERVER_ID = "silo_notification_server_id" + const val EXTRA_PROFILE_ID = "silo_notification_profile_id" } } @@ -172,3 +234,40 @@ private data class PushNotificationContent( val body: String, val route: String, ) + +/** A notification's established issuer, or null when it cannot be attributed. */ +data class PushNotificationAttribution(val serverId: String, val profileId: String) + +/** + * Establishes who a notification belongs to — completely, or not at all. + * + * A partial identity is worse than none: a missing component is a wildcard at + * delivery, so a half-attributed notification can act under an identity that + * never generated it. + * + * The profile comes from the fetched ROW or nowhere. Falling back to the active + * profile is the original misattribution: a push issued by server A that + * arrives while B is active misses its lookup, and the fallback stamped it as + * B's and navigated into B's library. + * + * The scope must also have held across the fetch, which can take seconds. + * Deliberately compares `serverId + identityGeneration` and NOT + * `credentialEpoch`: the epoch moves on persistent credential writes, which are + * not identity changes, so including it would let ordinary token churn make a + * legitimate notification generic. Comparing generations rather than ids is + * what catches an A→B→A round trip. + */ +fun pushNotificationAttribution( + rowProfileId: String?, + serverIdBefore: String?, + identityGenerationBefore: Long?, + serverIdAfter: String?, + identityGenerationAfter: Long?, +): PushNotificationAttribution? { + val profileId = rowProfileId?.takeIf { it.isNotBlank() } ?: return null + val serverId = serverIdBefore?.takeIf { it.isNotBlank() } ?: return null + if (serverIdAfter == null || identityGenerationBefore == null) return null + if (serverId != serverIdAfter) return null + if (identityGenerationBefore != identityGenerationAfter) return null + return PushNotificationAttribution(serverId = serverId, profileId = profileId) +} diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt index 88c777470..c7686f173 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt @@ -11,6 +11,10 @@ import androidx.compose.foundation.layout.navigationBarsPadding import androidx.compose.foundation.layout.padding import androidx.compose.runtime.Composable import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.runtime.rememberCoroutineScope +import kotlinx.coroutines.launch +import org.siloserver.silo.android.ui.screens.auth.DevicePairingWrongServerScreen +import org.siloserver.silo.android.ui.screens.auth.DevicePairingUnknownServerScreen import androidx.compose.runtime.collectAsState import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.LaunchedEffect @@ -115,6 +119,12 @@ fun AppNavigation( startDestination: String = Route.Login.route, pendingExternalRoute: ExternalRouteRequest? = null, onExternalRouteConsumed: (ExternalRouteRequest) -> Unit = {}, + /** + * Re-queues [route] as a pending external request. Used when an action + * inside a destination is about to send the user through authentication, + * which clears the back stack and would otherwise lose that destination. + */ + onRequeueExternalRoute: (String) -> Unit = {}, ) { val tokenManager: TokenManager = koinInject() val serverRegistry: org.siloserver.silo.network.ServerRegistry = koinInject() @@ -186,14 +196,22 @@ fun AppNavigation( } } }, - isStillValidForActiveServer = { requiredOrigin -> - // The user may have configured a DIFFERENT server while this - // request waited through setup/login. A pairing code looked up - // against the wrong server is worse than not looked up at all. - deviceLoginOriginMatchesServer( - requiredOrigin = requiredOrigin, - activeServerUrl = serverRegistry.activeEntry.value?.url, - ) + isStillValidForScope = { scope -> + // Checked AFTER the wait: the identity can move while a request + // sits through setup, login and profile selection. + when (scope) { + ExternalRouteScope.Unscoped -> true + is ExternalRouteScope.Identity -> { + // One snapshot, for the same reason the capture side + // takes one: separate getters can tear across a switch + // and validate against an identity that never existed. + val live = tokenManager.snapshotCurrentScope() + scope.matches( + serverId = live?.serverId, + profileId = live?.profileId, + ) + } + } }, onConsumed = onExternalRouteConsumed, ) @@ -348,6 +366,11 @@ fun AppNavigation( nullable = true defaultValue = null }, + navArgument("serverOrigin") { + type = NavType.StringType + nullable = true + defaultValue = null + }, ), // Deliberately NO navDeepLink registrations. While they existed, // Navigation matched the Activity's launch Intent itself when the @@ -359,20 +382,114 @@ fun AppNavigation( ) { backStackEntry -> val token = backStackEntry.arguments?.getString("token") val code = backStackEntry.arguments?.getString("code") - DevicePairingScreen( - token = token, - code = code, - onDone = { - if (!navController.popBackStack()) { - navController.navigate(Route.Home.route) { - popUpTo(0) { inclusive = true } + val requiredOrigin = backStackEntry.arguments?.getString("serverOrigin") + val knownServers by serverRegistry.entries.collectAsState() + val activeServer by serverRegistry.activeEntry.collectAsState() + val match = remember(requiredOrigin, activeServer, knownServers) { + deviceLoginServerMatch( + requiredOrigin = requiredOrigin, + activeServerUrl = activeServer?.url, + entries = knownServers, + ) + } + val pairingScope = rememberCoroutineScope() + when (val resolved = match) { + is DeviceLoginServerMatch.SwitchRequired -> + DevicePairingWrongServerScreen( + serverName = resolved.entry.displayName, + onSwitch = { + pairingScope.launch { + serverRegistry.switchTo(resolved.entry.id) + // Re-queue ONLY if the target server will send + // the user through auth: that flow ends at + // profile selection, whose popUpTo(0) wipes this + // destination and the code would have to be + // scanned again. Re-queueing unconditionally was + // worse — with no sign-in needed the request just + // waited for this screen to close and then + // reopened it. + val authRoute = pairingAuthRouteOrNull( + tokenManager = tokenManager, + activeEntryProfileId = serverRegistry.activeEntry.value + ?.profileId, + ) + if (authRoute != null) { + onRequeueExternalRoute( + Route.PairDevice( + token = token, + code = code, + serverOrigin = requiredOrigin, + ).route, + ) + // Requeueing alone left the user sitting on + // a pairing screen for a server they are not + // signed in to; the queued request only + // fires once something else takes them + // somewhere authenticated. Send them. + navController.navigate(authRoute) { + popUpTo(0) { inclusive = true } + } + } + } + }, + onCancel = { + if (!navController.popBackStack()) { + navController.navigate(Route.Home.route) { + popUpTo(0) { inclusive = true } + } + } + }, + ) + is DeviceLoginServerMatch.UnknownServer -> + DevicePairingUnknownServerScreen( + origin = resolved.origin, + onAddServer = { + // Adding a server always runs setup and login, which + // clear this destination — so this one always + // re-queues. + onRequeueExternalRoute( + Route.PairDevice( + token = token, + code = code, + serverOrigin = requiredOrigin, + ).route, + ) + navController.navigate(Route.ServerSetup.route) + }, + onCancel = { + if (!navController.popBackStack()) { + navController.navigate(Route.Home.route) { + popUpTo(0) { inclusive = true } + } + } + }, + ) + DeviceLoginServerMatch.Active -> DevicePairingScreen( + token = token, + code = code, + onDone = { + if (!navController.popBackStack()) { + navController.navigate(Route.Home.route) { + popUpTo(0) { inclusive = true } + } } - } - }, - onSignIn = { - navController.navigate(Route.Login.route) - }, - ) + }, + onSignIn = { + // Same preservation as the switch path: signing in ends + // at profile selection, whose popUpTo(0) wipes this + // destination, and the code would have to be scanned + // again. + onRequeueExternalRoute( + Route.PairDevice( + token = token, + code = code, + serverOrigin = requiredOrigin, + ).route, + ) + navController.navigate(Route.Login.route) + }, + ) + } } // ---- Server list (multi-server management) ---- @@ -1055,3 +1172,20 @@ private fun NavHostController.isDisplayingExactPlayerRoute( val requestedTarget = playerRouteIntentOrNull(route) ?: return false return currentPlayerTarget?.let(requestedTarget::matches) == true } + +/** + * The route the newly active server must pass through before pairing is + * possible, or null when it can pair immediately. + * + * Same credential check `ServerListViewModel` uses to pick a switch + * destination, including its preference for the registry entry's profile id + * over the token manager's cached one. + */ +private suspend fun pairingAuthRouteOrNull( + tokenManager: TokenManager, + activeEntryProfileId: String?, +): String? { + if (tokenManager.getAccessToken().isNullOrBlank()) return Route.Login.route + val profileId = activeEntryProfileId ?: tokenManager.getProfileId() + return if (profileId.isNullOrBlank()) Route.ProfileSelection.route else null +} diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt index 67ed321d1..fa5df86ff 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt @@ -20,12 +20,24 @@ internal fun deviceLoginPairRouteOrNull(rawUri: String?): String? { if (!uri.isDeviceLoginUri()) return null + // A device-SHAPED http(s) link whose origin cannot be read is not a usable + // pairing request. Letting it through produced a route with no + // `serverOrigin`, which downstream reads as "names no server" and pairs + // against whichever server is active — exactly what the origin check + // exists to stop. + val scope = deviceLoginScope(rawUri) + if (scope == DeviceLoginScope.Invalid) return null + val params = uri.queryParameters() val token = params["token"]?.takeIf { it.isNotBlank() } val code = params["code"]?.takeIf { it.isNotBlank() } if (token == null && code == null) return null - return buildPairDeviceRoute(token = token, code = if (token == null) code else null) + return buildPairDeviceRoute( + token = token, + code = if (token == null) code else null, + serverOrigin = (scope as? DeviceLoginScope.Origin)?.origin, + ) } /** @@ -75,56 +87,27 @@ private fun URI.normalizedOrigin(): String? { return if (explicitPort != null) "$scheme://$host:$explicitPort" else "$scheme://$host" } -/** Whether [requiredOrigin] is the origin of [activeServerUrl]. */ +/** + * Whether [requiredOrigin] is the origin of [activeServerUrl]. + * + * BOTH sides are normalized. Comparing a caller-supplied origin verbatim made + * `https://h:443` a different server from `https://h`, so a valid link was + * refused. + */ internal fun deviceLoginOriginMatchesServer( requiredOrigin: String, activeServerUrl: String?, ): Boolean { + val required = runCatching { URI(requiredOrigin) }.getOrNull()?.normalizedOrigin() + ?: return false val active = activeServerUrl ?.takeIf { it.isNotBlank() } ?.let { runCatching { URI(it) }.getOrNull() } ?.normalizedOrigin() ?: return false - return active == requiredOrigin + return active == required } -/** The origin a device link requires, or null when it names no server. */ -internal fun deviceLoginRequiredOrigin(rawUri: String?): String? = - (deviceLoginScope(rawUri) as? DeviceLoginScope.Origin)?.origin - -/** - * True when a device link may be acted on for the currently active server. - * - * An http(s) device link identifies the server that issued the pairing request, - * but that origin used to be dropped on the floor: the code was then looked up - * against whatever server happened to be active, which normally reports the - * request as invalid or expired even though it is perfectly valid on the server - * the user actually scanned. Approving against the wrong server is worse than - * not approving, so a link naming a different origin is refused. - * - * With NO server configured yet there is nothing to contradict — the user is - * about to set one up, and having just scanned a server's code they are - * overwhelmingly likely to set up that one — so the link is allowed through the - * normal setup/login gates rather than dropped on the floor. - * - * Pairing against a configured-but-INACTIVE server would mean switching the - * active server from a link, which is a product decision rather than a parser - * one. It is refused here; see the caller for the known gap that this refusal - * is currently silent. - */ -internal fun deviceLoginLinkIsForActiveServer(rawUri: String?, activeServerUrl: String?): Boolean = - when (val scope = deviceLoginScope(rawUri)) { - DeviceLoginScope.Unscoped -> true - DeviceLoginScope.Invalid -> false - is DeviceLoginScope.Origin -> { - val active = activeServerUrl - ?.takeIf { it.isNotBlank() } - ?.let { runCatching { URI(it) }.getOrNull() } - ?.normalizedOrigin() - active == null || active == scope.origin - } - } - private fun URI.isDeviceLoginUri(): Boolean { val scheme = scheme?.lowercase() return when (scheme) { @@ -153,11 +136,16 @@ private fun URI.queryParameters(): Map = private fun String.urlDecode(): String = URLDecoder.decode(this, Charsets.UTF_8.name()) -private fun buildPairDeviceRoute(token: String?, code: String?): String = buildString { +private fun buildPairDeviceRoute( + token: String?, + code: String?, + serverOrigin: String?, +): String = buildString { append("pair_device") val params = listOfNotNull( token?.takeIf { it.isNotBlank() }?.let { "token=${it.routeEncode()}" }, code?.takeIf { it.isNotBlank() }?.let { "code=${it.routeEncode()}" }, + serverOrigin?.takeIf { it.isNotBlank() }?.let { "serverOrigin=${it.routeEncode()}" }, ) if (params.isNotEmpty()) { append("?") diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatch.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatch.kt new file mode 100644 index 000000000..57d6b54b8 --- /dev/null +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatch.kt @@ -0,0 +1,46 @@ +package org.siloserver.silo.android.ui.navigation + +import org.siloserver.silo.model.server.ServerEntry + +/** + * What to do with a pairing request that names the server which issued it. + * + * A pairing code is only meaningful on its own server. The origin used to be + * discarded and the code looked up against whichever server happened to be + * active, which normally reported a perfectly valid request as invalid or + * expired. Refusing that is right — but refusing it *silently* just moves the + * confusion, so the request is still delivered and the screen explains itself. + */ +sealed interface DeviceLoginServerMatch { + /** Proceed: the link names this server, or names none. */ + data object Active : DeviceLoginServerMatch + + /** The link belongs to [entry], which the user has but is not using. */ + data class SwitchRequired(val entry: ServerEntry) : DeviceLoginServerMatch + + /** The link names [origin], which is not a server the user has. */ + data class UnknownServer(val origin: String) : DeviceLoginServerMatch +} + +/** + * Resolves [requiredOrigin] against the known servers. + * + * [activeServerUrl] null means no server is configured yet — the user is on + * their way to adding one, so there is nothing to contradict and pairing + * proceeds through the normal setup gates. + */ +fun deviceLoginServerMatch( + requiredOrigin: String?, + activeServerUrl: String?, + entries: List, +): DeviceLoginServerMatch { + if (requiredOrigin == null) return DeviceLoginServerMatch.Active + if (activeServerUrl == null) return DeviceLoginServerMatch.Active + if (deviceLoginOriginMatchesServer(requiredOrigin, activeServerUrl)) { + return DeviceLoginServerMatch.Active + } + val known = entries.firstOrNull { deviceLoginOriginMatchesServer(requiredOrigin, it.url) } + return known + ?.let(DeviceLoginServerMatch::SwitchRequired) + ?: DeviceLoginServerMatch.UnknownServer(requiredOrigin) +} diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt index bb58c78c1..bb30f225d 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt @@ -8,31 +8,57 @@ import org.siloserver.silo.android.ui.screens.player.MobilePlayerRouteIntent import org.siloserver.silo.android.ui.screens.player.MobilePlayerRouteTarget import org.siloserver.silo.common.player.video.VideoPlayerRouteArgs +/** + * The identity an external request is only meaningful against. + * + * External requests wait: a link can arrive before sign-in and sit through + * setup, login and profile selection, and the identity it was created for may + * not be the one active when it finally fires. Every route that means something + * different under a different server or profile has to say so and be re-checked + * at delivery, or it acts on whoever happens to be signed in by then. + */ +sealed interface ExternalRouteScope { + /** Meaningful under any identity — e.g. "open the Downloads tab". */ + data object Unscoped : ExternalRouteScope + + /** + * Valid only under this server and profile. Used by notifications (which + * are generated for one profile's inbox) and by content links (whose ids + * are server-local). Null components mean "was not signed in when this was + * created", which constrains nothing. + */ + data class Identity(val serverId: String?, val profileId: String?) : ExternalRouteScope { + /** + * Each component constrains only if it was known. A link that arrived + * with a server but no profile yet — configured server, nobody signed + * in — must still deliver once a profile IS chosen; requiring the + * profile to still be null would drop exactly the link the user was + * signing in to open. + */ + fun matches(serverId: String?, profileId: String?): Boolean = + (this.serverId == null || this.serverId == serverId) && + (this.profileId == null || this.profileId == profileId) + } +} + /** A single external-navigation delivery, distinct even when its route repeats. */ class ExternalRouteRequest internal constructor( val generation: Long, val route: String, - /** - * The server origin this request is only valid against, or null if it is - * server-agnostic. - * - * A pairing link can be queued while NO server is configured, then wait - * through setup and login. Whatever server the user ends up on may not be - * the one that issued the code, so the origin has to survive the wait and - * be re-checked at delivery — dropping it here is how a code ends up looked - * up against the wrong server. - */ - val requiredServerOrigin: String? = null, + val scope: ExternalRouteScope = ExternalRouteScope.Unscoped, ) internal class ExternalRouteRequestFactory { private var latestGeneration = 0L - fun create(route: String, requiredServerOrigin: String? = null): ExternalRouteRequest = + fun create( + route: String, + scope: ExternalRouteScope = ExternalRouteScope.Unscoped, + ): ExternalRouteRequest = ExternalRouteRequest( generation = ++latestGeneration, route = route, - requiredServerOrigin = requiredServerOrigin, + scope = scope, ) } @@ -168,10 +194,10 @@ internal suspend fun consumeExternalRouteOnce( currentDestinationRoutes: Flow, isAlreadyAtRoute: (String) -> Boolean = { false }, /** - * Whether [ExternalRouteRequest.requiredServerOrigin] still matches the - * active server. Evaluated after the wait, not before it. + * Whether the request's [ExternalRouteScope] still matches the live + * identity. Evaluated AFTER the wait, not before it. */ - isStillValidForActiveServer: suspend (String) -> Boolean = { true }, + isStillValidForScope: suspend (ExternalRouteScope) -> Boolean = { true }, navigate: (String) -> Unit, onConsumed: (ExternalRouteRequest) -> Unit, ) { @@ -182,13 +208,11 @@ internal suspend fun consumeExternalRouteOnce( currentDestinationRoutes.first { currentRoute -> isPreAuthenticationTarget || currentRoute !in preAuthenticationDestinationRoutes } - val originStillValid = request.requiredServerOrigin - ?.let { origin -> isStillValidForActiveServer(origin) } - ?: true - if (originStillValid && !isAlreadyAtRoute(route)) { + val scopeStillValid = isStillValidForScope(request.scope) + if (scopeStillValid && !isAlreadyAtRoute(route)) { navigate(route) } - // Consumed either way: a request whose server no longer matches must not + // Consumed either way: a request whose identity no longer matches must not // sit in the queue waiting to fire at some later, equally wrong moment. onConsumed(request) } diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRoute.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRoute.kt new file mode 100644 index 000000000..80179cd92 --- /dev/null +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRoute.kt @@ -0,0 +1,25 @@ +package org.siloserver.silo.android.ui.navigation + +/** + * A notification's navigation request, accepted only when it says whose it is. + * + * The identity is validated HERE as well as stamped at post time, because the + * delivery side must not trust an Intent it merely received. Two ways an + * unattributed one can arrive: a notification posted by a build before the + * extras existed, and an explicit Intent crafted against the exported Activity. + * Both used to produce `Identity(null, null)`, which matches every identity — + * so the route ran against whoever happened to be signed in. + */ +fun notificationExternalRouteOrNull( + route: String?, + serverId: String?, + profileId: String?, +): Pair? { + val usableRoute = route?.takeIf { it.isNotBlank() } ?: return null + val usableServerId = serverId?.takeIf { it.isNotBlank() } ?: return null + val usableProfileId = profileId?.takeIf { it.isNotBlank() } ?: return null + return usableRoute to ExternalRouteScope.Identity( + serverId = usableServerId, + profileId = usableProfileId, + ) +} diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt index 1bd51dccf..b8028d1a8 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/Routes.kt @@ -39,12 +39,20 @@ sealed class Route(val route: String) { data class PairDevice( val token: String? = null, val code: String? = null, + /** + * Origin of the server that issued this pairing request, when the link + * named one. Carried so the screen can refuse — and explain — rather + * than looking the code up against whichever server is active. + */ + val serverOrigin: String? = null, ) : Route( buildString { append("pair_device") val params = listOfNotNull( token?.takeIf { it.isNotBlank() }?.let { "token=${Uri.encode(it)}" }, code?.takeIf { it.isNotBlank() }?.let { "code=${Uri.encode(it)}" }, + serverOrigin?.takeIf { it.isNotBlank() } + ?.let { "serverOrigin=${Uri.encode(it)}" }, ) if (params.isNotEmpty()) { append("?") @@ -53,7 +61,7 @@ sealed class Route(val route: String) { }, ) { companion object { - const val ROUTE = "pair_device?token={token}&code={code}" + const val ROUTE = "pair_device?token={token}&code={code}&serverOrigin={serverOrigin}" } } diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/DevicePairingWrongServerScreen.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/DevicePairingWrongServerScreen.kt new file mode 100644 index 000000000..f45301ee5 --- /dev/null +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/auth/DevicePairingWrongServerScreen.kt @@ -0,0 +1,116 @@ +package org.siloserver.silo.android.ui.screens.auth + +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.padding +import androidx.compose.material3.Button +import androidx.compose.material3.ButtonDefaults +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.OutlinedButton +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.text.font.FontWeight +import androidx.compose.ui.text.style.TextAlign +import androidx.compose.ui.unit.dp + +/** + * Shown when a pairing link belongs to a server the user has, but is not + * currently using. + * + * The alternative — looking the code up against whichever server is active — + * reports a perfectly valid request as invalid or expired, which is a confusing + * dead end. So the request is still delivered, and the mismatch is stated with + * the one action that resolves it. + */ +@Composable +fun DevicePairingWrongServerScreen( + serverName: String, + onSwitch: () -> Unit, + onCancel: () -> Unit, +) { + DevicePairingNoticeStage( + title = "Different server", + body = "This pairing request is for $serverName. Switch to it to continue.", + primaryLabel = "Switch to $serverName", + onPrimary = onSwitch, + onCancel = onCancel, + ) +} + +/** Shown when a pairing link names a server the user has not added. */ +@Composable +fun DevicePairingUnknownServerScreen( + origin: String, + onAddServer: () -> Unit, + onCancel: () -> Unit, +) { + DevicePairingNoticeStage( + title = "Unknown server", + body = "This pairing request is for $origin, which isn't one of your servers. " + + "Add it to continue.", + primaryLabel = "Add server", + onPrimary = onAddServer, + onCancel = onCancel, + ) +} + +@Composable +private fun DevicePairingNoticeStage( + title: String, + body: String, + primaryLabel: String, + onPrimary: () -> Unit, + onCancel: () -> Unit, +) { + AuthStage { + Column( + modifier = Modifier + .fillMaxSize() + .padding(horizontal = 24.dp), + horizontalAlignment = Alignment.CenterHorizontally, + verticalArrangement = Arrangement.Center, + ) { + SiloLogo() + + Spacer(modifier = Modifier.height(18.dp)) + + Text( + text = title, + style = MaterialTheme.typography.headlineMedium, + color = AuthColors.OnBackground, + fontWeight = FontWeight.SemiBold, + ) + + Spacer(modifier = Modifier.height(12.dp)) + + Text( + text = body, + style = MaterialTheme.typography.bodyMedium, + color = AuthColors.OnSurfaceVariant, + textAlign = TextAlign.Center, + ) + + Spacer(modifier = Modifier.height(28.dp)) + + Button( + onClick = onPrimary, + modifier = Modifier.fillMaxWidth(), + colors = ButtonDefaults.buttonColors(containerColor = AuthColors.Primary), + ) { + Text(text = primaryLabel) + } + + Spacer(modifier = Modifier.height(12.dp)) + + OutlinedButton(onClick = onCancel, modifier = Modifier.fillMaxWidth()) { + Text(text = "Cancel") + } + } + } +} diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationAttributionTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationAttributionTest.kt new file mode 100644 index 000000000..1a413b05c --- /dev/null +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationAttributionTest.kt @@ -0,0 +1,120 @@ +package org.siloserver.silo.android.push + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * A notification must be attributed to its issuer completely or not at all — a + * missing component is a wildcard at delivery, so a half-attributed + * notification can act under an identity that never generated it. + */ +class PushNotificationAttributionTest { + + @Test + fun `a stable identity with a fetched row attributes`() { + assertEquals( + PushNotificationAttribution(serverId = "server-a", profileId = "kids"), + pushNotificationAttribution( + rowProfileId = "kids", + serverIdBefore = "server-a", + identityGenerationBefore = 7L, + serverIdAfter = "server-a", + identityGenerationAfter = 7L, + ), + ) + } + + /** + * The original misattribution: a push issued by A arriving while B is active + * misses its lookup, and falling back to the active profile stamped it as + * B's and navigated into B's library. + */ + @Test + fun `a lookup miss does not fall back to the active profile`() { + assertNull( + pushNotificationAttribution( + rowProfileId = null, + serverIdBefore = "server-b", + identityGenerationBefore = 7L, + serverIdAfter = "server-b", + identityGenerationAfter = 7L, + ), + ) + } + + @Test + fun `a server switch during the fetch abandons attribution`() { + assertNull( + pushNotificationAttribution( + rowProfileId = "kids", + serverIdBefore = "server-a", + identityGenerationBefore = 7L, + serverIdAfter = "server-b", + identityGenerationAfter = 8L, + ), + ) + } + + /** + * A→B→A leaves the server id looking untouched, which is why generations + * are compared rather than ids alone. + */ + @Test + fun `an A to B to A round trip is detected`() { + assertNull( + pushNotificationAttribution( + rowProfileId = "kids", + serverIdBefore = "server-a", + identityGenerationBefore = 7L, + serverIdAfter = "server-a", + identityGenerationAfter = 9L, + ), + ) + } + + /** + * Persistent credential writes move `credentialEpoch` without changing who + * the user is. This pins that the predicate ignores the epoch entirely — + * only the server and the identity generation decide. + */ + @Test + fun `credential churn does not abandon attribution`() { + assertEquals( + PushNotificationAttribution(serverId = "server-a", profileId = "kids"), + pushNotificationAttribution( + rowProfileId = "kids", + serverIdBefore = "server-a", + identityGenerationBefore = 7L, + serverIdAfter = "server-a", + identityGenerationAfter = 7L, + ), + ) + } + + @Test + fun `no identity at all does not attribute`() { + assertNull( + pushNotificationAttribution( + rowProfileId = "kids", + serverIdBefore = null, + identityGenerationBefore = null, + serverIdAfter = null, + identityGenerationAfter = null, + ), + ) + } + + @Test + fun `a blank profile is not an identity`() { + assertNull( + pushNotificationAttribution( + rowProfileId = " ", + serverIdBefore = "server-a", + identityGenerationBefore = 7L, + serverIdAfter = "server-a", + identityGenerationAfter = 7L, + ), + ) + } +} diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.kt index c3b8bebe3..b9219d2cb 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParserTest.kt @@ -26,8 +26,10 @@ class DeviceLoginRouteParserTest { @Test fun serverHttpsDeviceTokenUrlRoutesToPairDevice() { + // The issuing origin rides along so the pairing screen can refuse a + // code that belongs to a server the user is not currently on. assertEquals( - "pair_device?token=t1", + "pair_device?token=t1&serverOrigin=https%3A%2F%2Fsilo.example", deviceLoginPairRouteOrNull("https://silo.example/device?token=t1"), ) } @@ -35,7 +37,7 @@ class DeviceLoginRouteParserTest { @Test fun serverHttpsAuthDeviceCodeUrlRoutesToPairDevice() { assertEquals( - "pair_device?code=ABCD", + "pair_device?code=ABCD&serverOrigin=https%3A%2F%2Fsilo.example", deviceLoginPairRouteOrNull("https://silo.example/auth/device?code=ABCD"), ) } @@ -60,136 +62,42 @@ class DeviceLoginRouteParserTest { assertNull(deviceLoginPairRouteOrNull("silo://device?token=&code=")) } - // --- origin matching --- - - /** - * An HTTPS device link names the server that issued the pairing request. - * That origin used to be dropped, so the code was looked up against - * whichever server happened to be active — normally reporting a perfectly - * valid request as invalid, and at worst approving against the wrong - * server. - */ - @Test - fun `an https device link for another server is not for the active one`() { - assertFalse( - deviceLoginLinkIsForActiveServer( - rawUri = "https://server-b.example/auth/device?code=ABCD", - activeServerUrl = "https://server-a.example", - ), - ) - } - - @Test - fun `an https device link for the active server is accepted`() { - assertTrue( - deviceLoginLinkIsForActiveServer( - rawUri = "https://server-a.example/auth/device?code=ABCD", - activeServerUrl = "https://server-a.example", - ), - ) - } - - @Test - fun `a port difference is a different origin`() { - assertFalse( - deviceLoginLinkIsForActiveServer( - rawUri = "https://silo.example:8443/device?code=ABCD", - activeServerUrl = "https://silo.example", - ), - ) - } - - @Test - fun `matching host and port is the same origin`() { - assertTrue( - deviceLoginLinkIsForActiveServer( - rawUri = "https://silo.example:8443/device?code=ABCD", - activeServerUrl = "https://silo.example:8443/", - ), - ) - } + // --- origin --- - /** An app-scheme link names no server, so it is about the active one. */ @Test fun `an app scheme device link names no server`() { - assertTrue( - deviceLoginLinkIsForActiveServer( - rawUri = "silo://device?code=ABCD", - activeServerUrl = "https://server-a.example", - ), - ) assertEquals(DeviceLoginScope.Unscoped, deviceLoginScope("silo://device?code=ABCD")) + // No origin in the route either. + assertEquals("pair_device?code=ABCD", deviceLoginPairRouteOrNull("silo://device?code=ABCD")) } - /** - * With no server configured there is nothing to contradict, and the user is - * about to set one up — almost certainly the one they just scanned. Dropping - * the link here would turn a valid action into silence. - */ @Test - fun `an https device link with no active server is allowed through setup`() { - assertTrue( - deviceLoginLinkIsForActiveServer( - rawUri = "https://server-b.example/device?code=ABCD", - activeServerUrl = null, - ), - ) - } - - /** Default ports are equivalent to omitting them. */ - @Test - fun `an explicit default port is the same origin`() { - assertTrue( - deviceLoginLinkIsForActiveServer( - rawUri = "https://silo.example:443/device?code=ABCD", - activeServerUrl = "https://silo.example", - ), + fun `an https device link carries its issuing origin`() { + assertEquals( + DeviceLoginScope.Origin("https://server-b.example"), + deviceLoginScope("https://server-b.example/device?code=ABCD"), ) } /** - * A device-shaped link with no readable host must not fall back to "use - * whichever server is active" — that is the behaviour being removed. + * A device-SHAPED link whose origin cannot be read must not parse at all. + * It used to produce a route with no origin, which downstream reads as + * "names no server" and pairs against whichever server is active — the + * exact bypass the origin check exists to stop. */ @Test - fun `an https device link with no host is refused`() { + fun `an https device link with no host does not parse`() { assertEquals(DeviceLoginScope.Invalid, deviceLoginScope("https:///device?code=ABCD")) - assertFalse( - deviceLoginLinkIsForActiveServer( - rawUri = "https:///device?code=ABCD", - activeServerUrl = "https://server-a.example", - ), - ) + assertNull(deviceLoginPairRouteOrNull("https:///device?code=ABCD")) } - /** Port 0 is not a valid origin and must not pass as "default port". */ + /** Port 0 is not a valid origin, so the link is not usable either. */ @Test - fun `port zero is not an origin`() { - assertEquals(DeviceLoginScope.Invalid, deviceLoginScope("https://silo.example:0/device?code=A")) - assertFalse( - deviceLoginLinkIsForActiveServer( - rawUri = "https://silo.example:0/device?code=A", - activeServerUrl = "https://silo.example", - ), + fun `a device link with an invalid port does not parse`() { + assertEquals( + DeviceLoginScope.Invalid, + deviceLoginScope("https://silo.example:0/device?code=A"), ) - } - - /** - * The origin has to survive the wait through setup/login: a pairing request - * queued with no server configured must not fire against whatever server - * the user happens to end up on. - */ - @Test - fun `a required origin only matches its own server`() { - val origin = deviceLoginRequiredOrigin("https://server-b.example/device?code=A") - assertEquals("https://server-b.example", origin) - assertTrue(deviceLoginOriginMatchesServer(origin!!, "https://server-b.example")) - assertFalse(deviceLoginOriginMatchesServer(origin, "https://server-a.example")) - assertFalse(deviceLoginOriginMatchesServer(origin, null)) - } - - @Test - fun `an app scheme link requires no origin`() { - assertNull(deviceLoginRequiredOrigin("silo://device?code=A")) + assertNull(deviceLoginPairRouteOrNull("https://silo.example:0/device?code=A")) } } diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatchTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatchTest.kt new file mode 100644 index 000000000..72e604f5b --- /dev/null +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginServerMatchTest.kt @@ -0,0 +1,93 @@ +package org.siloserver.silo.android.ui.navigation + +import org.siloserver.silo.model.server.ServerEntry +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * A pairing code is only meaningful on the server that issued it. Looking it up + * against whichever server happens to be active reports a valid request as + * invalid; refusing it silently is just as bad a dead end. These pin the third + * option — deliver it, and say which server it belongs to. + */ +class DeviceLoginServerMatchTest { + + private val serverA = ServerEntry(id = "a", url = "https://a.example", fetchedName = "Server A") + private val serverB = ServerEntry(id = "b", url = "https://b.example", fetchedName = "Server B") + private val entries = listOf(serverA, serverB) + + @Test + fun `a link for the active server proceeds`() { + assertEquals( + DeviceLoginServerMatch.Active, + deviceLoginServerMatch( + requiredOrigin = "https://a.example", + activeServerUrl = "https://a.example", + entries = entries, + ), + ) + } + + @Test + fun `a link for another configured server offers the switch`() { + assertEquals( + DeviceLoginServerMatch.SwitchRequired(serverB), + deviceLoginServerMatch( + requiredOrigin = "https://b.example", + activeServerUrl = "https://a.example", + entries = entries, + ), + ) + } + + @Test + fun `a link for a server the user does not have says so`() { + assertEquals( + DeviceLoginServerMatch.UnknownServer("https://c.example"), + deviceLoginServerMatch( + requiredOrigin = "https://c.example", + activeServerUrl = "https://a.example", + entries = entries, + ), + ) + } + + /** An app-scheme link names no server, so it is about the active one. */ + @Test + fun `a link naming no server proceeds`() { + assertEquals( + DeviceLoginServerMatch.Active, + deviceLoginServerMatch( + requiredOrigin = null, + activeServerUrl = "https://a.example", + entries = entries, + ), + ) + } + + /** Nothing to contradict yet — the user is on their way to adding one. */ + @Test + fun `a link proceeds when no server is configured`() { + assertEquals( + DeviceLoginServerMatch.Active, + deviceLoginServerMatch( + requiredOrigin = "https://b.example", + activeServerUrl = null, + entries = emptyList(), + ), + ) + } + + /** Default ports must not make the same server look like a different one. */ + @Test + fun `an explicit default port still matches`() { + assertEquals( + DeviceLoginServerMatch.Active, + deviceLoginServerMatch( + requiredOrigin = "https://a.example:443", + activeServerUrl = "https://a.example", + entries = entries, + ), + ) + } +} diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt index 9fb6f80d0..be07221b0 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt @@ -295,27 +295,30 @@ class ExternalRouteNavigationTest { ) /** - * A pairing link can be queued with NO server configured and then wait - * through setup and login. Whatever server the user lands on may not be the - * one that issued the code, so the origin is re-checked at delivery — and a - * mismatch must not navigate. + * A notification PendingIntent can be tapped days after it was posted, and + * several profile switches later. Its route means something different — or + * nothing — under another identity, so the scope is re-checked at delivery + * and a mismatch must not navigate. */ @Test fun aRequestWhoseServerNoLongerMatchesIsNotDelivered() = runTest { val request = ExternalRouteRequestFactory() - .create(route = "pair_device?code=ABCD", requiredServerOrigin = "https://server-b") + .create( + route = "inbox", + scope = ExternalRouteScope.Identity(serverId = "server-b", profileId = "kids"), + ) var navigated: String? = null var consumed = 0 consumeExternalRouteOnce( pendingExternalRoute = request, currentDestinationRoutes = flowOf("home"), - isStillValidForActiveServer = { false }, + isStillValidForScope = { false }, navigate = { navigated = it }, onConsumed = { consumed++ }, ) - assertNull(navigated, "a code must never be looked up against the wrong server") + assertNull(navigated, "a notification must never act on a different profile's session") // Still consumed: leaving it queued would only let it fire later, at an // equally wrong moment. assertEquals(1, consumed) @@ -324,19 +327,22 @@ class ExternalRouteNavigationTest { @Test fun aRequestWhoseServerStillMatchesIsDelivered() = runTest { val request = ExternalRouteRequestFactory() - .create(route = "pair_device?code=ABCD", requiredServerOrigin = "https://server-b") + .create( + route = "inbox", + scope = ExternalRouteScope.Identity(serverId = "server-b", profileId = "kids"), + ) var navigated: String? = null var consumed = 0 consumeExternalRouteOnce( pendingExternalRoute = request, currentDestinationRoutes = flowOf("home"), - isStillValidForActiveServer = { true }, + isStillValidForScope = { true }, navigate = { navigated = it }, onConsumed = { consumed++ }, ) - assertEquals("pair_device?code=ABCD", navigated) + assertEquals("inbox", navigated) assertEquals(1, consumed) } @@ -349,11 +355,55 @@ class ExternalRouteNavigationTest { consumeExternalRouteOnce( pendingExternalRoute = request, currentDestinationRoutes = flowOf("home"), - isStillValidForActiveServer = { error("must not be consulted for an unscoped route") }, + isStillValidForScope = { scope -> + assertEquals(ExternalRouteScope.Unscoped, scope) + true + }, navigate = { navigated = it }, onConsumed = { }, ) assertEquals("item/abc", navigated) } + + // --- identity scope matching --- + + /** + * A link that arrived with a server but no profile — configured server, + * nobody signed in — must still deliver once a profile IS chosen. Requiring + * the profile to still be null dropped exactly the link the user was + * signing in to open. + */ + @Test + fun aScopeCapturedBeforeSignInStillMatchesAfterIt() { + val scope = ExternalRouteScope.Identity(serverId = "server-a", profileId = null) + + assertTrue(scope.matches(serverId = "server-a", profileId = "profile-1")) + assertTrue(scope.matches(serverId = "server-a", profileId = null)) + } + + @Test + fun aScopeDoesNotMatchAnotherServer() { + val scope = ExternalRouteScope.Identity(serverId = "server-a", profileId = null) + + assertFalse(scope.matches(serverId = "server-b", profileId = "profile-1")) + } + + /** A fully-specified notification scope must match both components. */ + @Test + fun aFullyPinnedScopeRequiresBothComponents() { + val scope = ExternalRouteScope.Identity(serverId = "server-a", profileId = "kids") + + assertTrue(scope.matches(serverId = "server-a", profileId = "kids")) + assertFalse(scope.matches(serverId = "server-a", profileId = "adults")) + assertFalse(scope.matches(serverId = "server-b", profileId = "kids")) + } + + /** Nothing known constrains nothing — the signed-out arrival case. */ + @Test + fun anEmptyScopeMatchesAnything() { + val scope = ExternalRouteScope.Identity(serverId = null, profileId = null) + + assertTrue(scope.matches(serverId = "server-a", profileId = "kids")) + } } diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.kt new file mode 100644 index 000000000..098fab5ba --- /dev/null +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRouteTest.kt @@ -0,0 +1,71 @@ +package org.siloserver.silo.android.ui.navigation + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * A notification route is only honoured if it says whose it is. + * + * Missing extras used to produce `Identity(null, null)`, which matches every + * identity — so the route ran against whoever happened to be signed in. Two + * ways that arrives: a notification posted by a build from before the extras + * existed, and an explicit Intent crafted against the exported Activity. + */ +class NotificationExternalRouteTest { + + @Test + fun `a fully attributed notification is accepted`() { + assertEquals( + "item/abc" to ExternalRouteScope.Identity(serverId = "server-a", profileId = "kids"), + notificationExternalRouteOrNull( + route = "item/abc", + serverId = "server-a", + profileId = "kids", + ), + ) + } + + @Test + fun `a notification with no identity is rejected`() { + assertNull( + notificationExternalRouteOrNull(route = "item/abc", serverId = null, profileId = null), + ) + } + + /** Half an identity is worse than none — the missing half is a wildcard. */ + @Test + fun `a half attributed notification is rejected`() { + assertNull( + notificationExternalRouteOrNull( + route = "item/abc", + serverId = "server-a", + profileId = null, + ), + ) + assertNull( + notificationExternalRouteOrNull( + route = "item/abc", + serverId = null, + profileId = "kids", + ), + ) + } + + @Test + fun `blank is not an identity`() { + assertNull( + notificationExternalRouteOrNull(route = "item/abc", serverId = " ", profileId = "kids"), + ) + assertNull( + notificationExternalRouteOrNull(route = " ", serverId = "server-a", profileId = "kids"), + ) + } + + @Test + fun `no route is nothing to deliver`() { + assertNull( + notificationExternalRouteOrNull(route = null, serverId = "server-a", profileId = "kids"), + ) + } +} diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt index 079288d20..58cc0be5b 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/player/PlayerViewModelLoadOwnershipIntegrationTest.kt @@ -38,6 +38,10 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.isActive import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.update import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.runTest @@ -104,7 +108,14 @@ class PlayerViewModelLoadOwnershipIntegrationTest { @get:Rule val tmp = TemporaryFolder() - private val dispatcher = UnconfinedTestDispatcher() + // StandardTestDispatcher, NOT Unconfined. Unconfined resumes continuations + // inline on whichever thread completed the suspending call, and reentrant + // resumptions land in that thread's internal unconfined event loop — a queue + // the test scheduler cannot reach. Waiting for such a continuation from + // another dispatcher was a genuine race: measured 2 failures in 6 idle runs. + // A standard dispatcher gives every continuation an explicit scheduler queue + // that `runTest` drains while the test body is suspended. + private val dispatcher = StandardTestDispatcher() private lateinit var db: SiloDatabase @BeforeTest @@ -206,7 +217,13 @@ class PlayerViewModelLoadOwnershipIntegrationTest { message = "stale failure", ), ) - fixture.viewModel.awaitState { it.sessionId == "new-session" } + // Drain, do not wait on a predicate. `awaitState { sessionId == + // "new-session" }` was already true the moment it was called, so it + // returned without the stale error having been handled at all — the + // assertions below then proved nothing. Draining the scheduler makes + // "the stale error was processed AND still did not overwrite" the + // thing actually under test. + advanceUntilIdle() val state = fixture.viewModel.uiState.value assertEquals("new", state.contentId) @@ -610,10 +627,14 @@ private class DeferredNonCooperativeStarter : VideoPlaybackStarter { private val pending = mutableListOf() + /** Replayable so a request that lands before the wait begins is still seen. */ + private val requestCount = MutableStateFlow(0) + override suspend fun start(request: VideoPlaybackStartRequest): VideoPlaybackStartResult = suspendCoroutine { continuation -> - synchronized(pending) { + requestCount.value = synchronized(pending) { pending += Pending(request, continuation) + pending.size } } @@ -631,9 +652,7 @@ private class DeferredNonCooperativeStarter : VideoPlaybackStarter { } suspend fun awaitRequestCount(count: Int) { - awaitCondition { - synchronized(pending) { pending.size >= count } - } + awaitRealTime { requestCount.first { it >= count } } } } @@ -646,6 +665,7 @@ private class RecordingPlaybackSessionManager( ) { private val stopped = mutableListOf() private val stopActiveContexts = mutableListOf() + private val stoppedSignal = MutableStateFlow>(emptySet()) val stoppedSessions: List get() = synchronized(stopped) { stopped.toList() } @@ -659,11 +679,12 @@ private class RecordingPlaybackSessionManager( stopped += sessionId stopActiveContexts += contextActive } + stoppedSignal.update { it + sessionId } return ApiResult.Success(Unit) } suspend fun awaitStopped(sessionId: String) { - awaitCondition { sessionId in stoppedSessions } + awaitRealTime { stoppedSignal.first { sessionId in it } } } } @@ -822,18 +843,33 @@ private fun noOpClient(): HttpClient = private suspend fun PlayerViewModel.awaitState( predicate: (PlayerViewModel.PlayerUiState) -> Boolean, ) { - awaitCondition { predicate(uiState.value) } + awaitRealTime { uiState.first(predicate) } } -private suspend fun awaitCondition(predicate: () -> Boolean) { - withContext(Dispatchers.Default.limitedParallelism(1)) { - withTimeout(5_000) { - while (!predicate()) { - delay(5) - } - } +/** + * Runs [block] under a REAL, generous deadline. + * + * The deadline has to be real: this load path does unavoidable work on + * `Dispatchers.IO` before it ever reaches the fake starter — the offline + * preflight in `PlayerViewModel.tryLocalPlayback` and, beneath it, + * `LegacyDownloadImporter` both hard-code that dispatcher — and virtual time + * cannot advance a real thread. A purely virtual timeout raced straight past + * that work and failed every test in this class. + * + * What must NOT come back is polling. Waiters here suspend on a signal, so a + * result that arrives before the wait begins is still seen, and a waiter can no + * longer give up on work that simply had not been dispatched yet. + */ +private suspend fun awaitRealTime(block: suspend () -> T): T = + withContext(Dispatchers.Default) { + // The deadline exists to turn a hang into a failure, not to police + // latency — a passing test signals in milliseconds and never waits. + // Five seconds was tight enough that a full-suite run, with dozens of + // Robolectric classes competing for the same JVM, could blow it while + // the work was merely slow. That looked exactly like the race this + // helper was written to remove, which is worse than useless. + withTimeout(30_000) { block() } } -} private const val SERVER_ID = "server" private const val PROFILE_ID = "profile" diff --git a/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt b/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt index 64b6b97f9..bb2762bf2 100644 --- a/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt +++ b/shared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.kt @@ -364,6 +364,14 @@ class EncryptedTokenManagerImpl( identityGeneration = identityTransitions.generation.value, ) } + // Reconcile with the registry FIRST. The registry observer is + // asynchronous, so immediately after a `switchTo(B)` this cached id can + // still be A — and every guard that trusts the snapshot then decides + // against a server the app has already left. The token reads + // (getAccessToken/getRefreshToken/getProfileId) reconcile; the snapshot + // did not, which made it disagree with them. Note getCurrentServerId + // still reads the cache directly. + ensureCacheMatchesRegistryLocked() val serverId = activeServerId ?: return@withLock null // Resolve the URL for *this* serverId from the registry entries so the // snapshot is internally consistent. Do NOT fall back to activeEntry — diff --git a/shared/src/androidUnitTest/kotlin/org/siloserver/silo/network/EncryptedTokenManagerScopeGenerationTest.kt b/shared/src/androidUnitTest/kotlin/org/siloserver/silo/network/EncryptedTokenManagerScopeGenerationTest.kt index f927b4e17..bf7ed2714 100644 --- a/shared/src/androidUnitTest/kotlin/org/siloserver/silo/network/EncryptedTokenManagerScopeGenerationTest.kt +++ b/shared/src/androidUnitTest/kotlin/org/siloserver/silo/network/EncryptedTokenManagerScopeGenerationTest.kt @@ -77,6 +77,54 @@ class EncryptedTokenManagerScopeGenerationTest { assertNull(manager.getAccessTokenForScope(staleScope)) } + /** + * The snapshot was the ONE identity read that did not reconcile with the + * registry first, so immediately after a registry-driven switch it still + * described the previous server — and every guard built on it then decided + * against a server the app had already left. Deliberately no intervening + * `getAccessToken()`: that read reconciles as a side effect and hid this. + */ + @Test + fun snapshotReportsTheNewServerImmediatelyAfterARegistryFirstSwitch() = runTest { + val registry = FakeServerRegistry() + val manager = EncryptedTokenManagerImpl( + prefs = inMemoryPreferences(), + registry = registry, + ) + manager.saveTokens("server-a-access", "server-a-refresh", 3600) + + registry.switchExternally("server-b") + + assertEquals("server-b", manager.snapshotCurrentScope()?.serverId) + } + + /** An overlay owns identity outright; a switch underneath must not retarget it. */ + @Test + fun aTemporaryOverlaySurvivesARegistryFirstSwitch() = runTest { + val registry = FakeServerRegistry() + val manager = EncryptedTokenManagerImpl( + prefs = inMemoryPreferences(), + registry = registry, + ) + manager.saveTokens("server-a-access", "server-a-refresh", 3600) + manager.beginTemporaryScope( + TemporaryAuthScope( + generationId = "overlay-1", + serverId = "overlay-server", + serverUrl = "https://overlay.example", + accessToken = "overlay-access", + refreshToken = "overlay-refresh", + profileId = "overlay-profile", + profileToken = "overlay-token", + expiresAtEpochMs = Long.MAX_VALUE, + ), + ) + + registry.switchExternally("server-b") + + assertEquals("overlay-server", manager.snapshotCurrentScope()?.serverId) + } + private class FakeServerRegistry : ServerRegistry { private val serverA = ServerEntry(id = "server-a", url = "https://server-a.example") private val serverB = ServerEntry(id = "server-b", url = "https://server-b.example") From adf3589349bdd38348d4c42b3c5166eda6e29f7f Mon Sep 17 00:00:00 2001 From: rxwatcher Date: Thu, 6 Aug 2026 08:39:45 +0200 Subject: [PATCH 5/6] fix(nav): close the four review findings on this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex reviewed this branch against its rebased base and returned NO-GO on four counts. All four are addressed. Unprotected profile selection was not scope-guarded. Both clients passed expectedScope = null for a profile with no PIN, which deliberately disables the guard — leaving the one path with no PIN round trip to re-establish identity committing unguarded. If another account became active between the grid being accepted and the commit entering the barrier, one account's profile id was written into the other's token slot. The scope the displayed grid was fetched under is now retained and passed for every selection, protected or not. An external tab link could retain a live player session indefinitely. tabSwitchNavOptions saves state so a tab keeps its stack, which is right for a tab — but a saved player entry keeps its ViewModelStore alive, so onCleared never runs and the session it owns is never stopped. The save is keyed to the LOWEST popped destination, so a later clearBackStack on the player route would not even find it. External tab routes now pop the player without saving first, so its ordinary teardown runs. This is the same class of leak the session-ownership work exists to prevent. External item links still used unconditional launchSingleTop, which is the exact defect this branch fixes for in-app navigation: AndroidX matches the destination node, not its arguments, so a notification for item B while item A's detail was showing reused A's entry and Back skipped A. Single-top now requires the arguments to agree. ExternalRouteScope did not carry identityGeneration, so signing out and back into the same account — or A to B back to A — was accepted. Ids alone cannot tell a new session from the old one. The generation is now captured, compared, and constrains only when it was known, the same rule the ids follow. Deliberately not credentialEpoch, which moves on ordinary token writes and would kill legitimate routes after a routine refresh. The PR description claimed this comparison already happened. It did not; that claim was wrong and is now true. Tests added for the generation rule and for argument-aware external item links. Full suite green on all four modules. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW --- .../siloserver/silo/android/MainActivity.kt | 1 + .../android/ui/navigation/AppNavigation.kt | 31 ++++++- .../ui/navigation/ExternalRouteNavigation.kt | 59 ++++++++++++- .../profiles/ProfileSelectionViewModel.kt | 18 +++- .../navigation/ExternalRouteNavigationTest.kt | 82 +++++++++++++++++-- .../profiles/TvProfileSelectionViewModel.kt | 18 +++- 6 files changed, 196 insertions(+), 13 deletions(-) diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt index 1597e1d7a..7e17ce387 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt @@ -355,6 +355,7 @@ class MainActivity : ComponentActivity() { return ExternalRouteScope.Identity( serverId = scope.serverId, profileId = scope.profileId, + identityGeneration = scope.identityGeneration, ) } // A null snapshot means "no active server" — nothing to pin to, and the diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt index c7686f173..a7e09cb83 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt @@ -179,6 +179,20 @@ fun AppNavigation( // NEWEST match, so the older anchor entry would survive and Back // could loop through a hidden tab. if (tabForRoute(route) != null) { + // Tear the player down BEFORE the tab switch, and without + // saving it. tabSwitchNavOptions saves state so a tab keeps + // its stack, which is right for a tab — but a saved player + // entry keeps its ViewModelStore alive, so onCleared never + // runs and the playback session it owns is never stopped. + // The save is also keyed to the LOWEST popped destination, + // so a later clearBackStack on the player route would not + // even find it. Popping first means the player's teardown + // runs the ordinary way. + navController.popBackStack( + route = Route.Player.ROUTE, + inclusive = true, + saveState = false, + ) navController.navigate(route) { tabSwitchNavOptions(navController.bottomMostTabRoute()) } @@ -188,11 +202,25 @@ fun AppNavigation( ?.destination?.route, targetRoute = route, ) + // Single-top only when the arguments agree it really is the + // same screen. AndroidX matches the destination NODE, so an + // external link to item B while item A's detail is showing + // reused A's entry and Back skipped A entirely — the same + // defect this branch fixes for in-app navigation. + val sameItemDetail = isSameItemDetail( + currentDestinationRoute = navController.currentBackStackEntry + ?.destination?.route, + currentContentId = navController.currentBackStackEntry + ?.arguments + ?.getString("contentId"), + targetRoute = route, + ) navController.navigate(route) { if (replaceCurrentPlayer) { popUpTo(Route.Player.ROUTE) { inclusive = true } } - launchSingleTop = true + launchSingleTop = replaceCurrentPlayer || sameItemDetail || + !route.startsWith("item/") } } }, @@ -209,6 +237,7 @@ fun AppNavigation( scope.matches( serverId = live?.serverId, profileId = live?.profileId, + identityGeneration = live?.identityGeneration, ) } } diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt index bb30f225d..774ec8656 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt @@ -27,17 +27,39 @@ sealed interface ExternalRouteScope { * are server-local). Null components mean "was not signed in when this was * created", which constrains nothing. */ - data class Identity(val serverId: String?, val profileId: String?) : ExternalRouteScope { + data class Identity( + val serverId: String?, + val profileId: String?, + /** + * The identity generation this route was created under. + * + * Ids alone cannot tell "still the same session" from "signed out and + * back into the same account", nor A -> B -> A. Both re-authenticate, + * and a route authored for the earlier session should not act on the + * later one. Null means the generation was unknown at creation and + * constrains nothing, same as the ids. + */ + val identityGeneration: Long? = null, + ) : ExternalRouteScope { /** * Each component constrains only if it was known. A link that arrived * with a server but no profile yet — configured server, nobody signed * in — must still deliver once a profile IS chosen; requiring the * profile to still be null would drop exactly the link the user was * signing in to open. + * + * The generation is deliberately NOT credentialEpoch: that moves on + * ordinary token writes, and pinning to it would kill legitimate routes + * after a routine refresh. */ - fun matches(serverId: String?, profileId: String?): Boolean = + fun matches( + serverId: String?, + profileId: String?, + identityGeneration: Long?, + ): Boolean = (this.serverId == null || this.serverId == serverId) && - (this.profileId == null || this.profileId == profileId) + (this.profileId == null || this.profileId == profileId) && + (this.identityGeneration == null || this.identityGeneration == identityGeneration) } } @@ -68,6 +90,37 @@ internal fun clearConsumedExternalRouteRequest( ): ExternalRouteRequest? = if (pendingRequest?.generation == consumedRequest.generation) null else pendingRequest +/** + * True when [targetRoute] is the item detail already on top. + * + * launchSingleTop matches the destination NODE, not its arguments, so an + * external link to item B while item A's detail is showing reuses A's entry — + * and its ViewModelStore — leaving Back to skip A entirely. Single-top is only + * correct here when the arguments say it really is the same screen. + */ +internal fun isSameItemDetail( + currentDestinationRoute: String?, + currentContentId: String?, + targetRoute: String, +): Boolean { + if (currentDestinationRoute != Route.ItemDetail.ROUTE) return false + if (!targetRoute.startsWith("item/")) return false + val targetContentId = targetRoute + .substringAfter("item/") + .substringBefore('?') + .takeIf { it.isNotBlank() } + // Same decode as the player intent: the route percent-encodes the id, + // and an encoded id never equals the decoded one held by the entry. + ?.let { + runCatching { + URLDecoder.decode(it.replace("+", "%2B"), StandardCharsets.UTF_8.name()) + }.getOrNull() + } + ?.takeIf { it.isNotBlank() } + ?: return false + return targetContentId == currentContentId +} + internal fun shouldReplaceCurrentPlayer( currentDestinationRoute: String?, targetRoute: String, diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt index 362a4288f..d5b32243a 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt @@ -54,6 +54,17 @@ class ProfileSelectionViewModel( * delete's explanation) visible across the follow-up list refresh, which * would otherwise silently swallow it. */ + /** + * The identity the displayed grid was fetched under. + * + * Every selection is qualified with it, protected or not. Passing null for + * unprotected picks disabled the guard for exactly the case with no PIN + * round trip to re-establish scope — so if another account became active + * between the grid being accepted and the commit entering the barrier, one + * account's profile id was written into the other's token slot. + */ + private var gridScope: AuthScopeSnapshot? = null + fun loadProfiles(clearError: Boolean = true) { val load = ++loadAttempt viewModelScope.launch { @@ -71,6 +82,8 @@ class ProfileSelectionViewModel( _uiState.update { it.copy(isLoading = false, profiles = emptyList()) } return@launch } + // The grid the viewer is about to pick from belongs to this scope. + gridScope = scope when (result) { is ApiResult.Success -> { @@ -126,7 +139,10 @@ class ProfileSelectionViewModel( ) } } else { - selectProfile(profile.id) + // Qualified by the grid's scope. An unprotected pick has no PIN + // round trip to re-establish identity, so without this it was the + // one path that committed unguarded. + selectProfile(profile.id, expectedScope = gridScope) } } diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt index be07221b0..b7c97e343 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt @@ -378,15 +378,15 @@ class ExternalRouteNavigationTest { fun aScopeCapturedBeforeSignInStillMatchesAfterIt() { val scope = ExternalRouteScope.Identity(serverId = "server-a", profileId = null) - assertTrue(scope.matches(serverId = "server-a", profileId = "profile-1")) - assertTrue(scope.matches(serverId = "server-a", profileId = null)) + assertTrue(scope.matches(serverId = "server-a", profileId = "profile-1", identityGeneration = null)) + assertTrue(scope.matches(serverId = "server-a", profileId = null, identityGeneration = null)) } @Test fun aScopeDoesNotMatchAnotherServer() { val scope = ExternalRouteScope.Identity(serverId = "server-a", profileId = null) - assertFalse(scope.matches(serverId = "server-b", profileId = "profile-1")) + assertFalse(scope.matches(serverId = "server-b", profileId = "profile-1", identityGeneration = null)) } /** A fully-specified notification scope must match both components. */ @@ -394,9 +394,9 @@ class ExternalRouteNavigationTest { fun aFullyPinnedScopeRequiresBothComponents() { val scope = ExternalRouteScope.Identity(serverId = "server-a", profileId = "kids") - assertTrue(scope.matches(serverId = "server-a", profileId = "kids")) - assertFalse(scope.matches(serverId = "server-a", profileId = "adults")) - assertFalse(scope.matches(serverId = "server-b", profileId = "kids")) + assertTrue(scope.matches(serverId = "server-a", profileId = "kids", identityGeneration = null)) + assertFalse(scope.matches(serverId = "server-a", profileId = "adults", identityGeneration = null)) + assertFalse(scope.matches(serverId = "server-b", profileId = "kids", identityGeneration = null)) } /** Nothing known constrains nothing — the signed-out arrival case. */ @@ -404,6 +404,74 @@ class ExternalRouteNavigationTest { fun anEmptyScopeMatchesAnything() { val scope = ExternalRouteScope.Identity(serverId = null, profileId = null) - assertTrue(scope.matches(serverId = "server-a", profileId = "kids")) + assertTrue(scope.matches(serverId = "server-a", profileId = "kids", identityGeneration = null)) + } + /** + * Signing out and back into the SAME account is a new session, and a route + * authored for the old one must not act on it. Ids alone cannot see that; + * only the generation can. + */ + @Test + fun aScopePinnedToAGenerationDoesNotMatchALaterSession() { + val scope = ExternalRouteScope.Identity( + serverId = "server-a", + profileId = "kids", + identityGeneration = 7L, + ) + + assertTrue( + scope.matches(serverId = "server-a", profileId = "kids", identityGeneration = 7L), + ) + assertFalse( + scope.matches(serverId = "server-a", profileId = "kids", identityGeneration = 8L), + ) + } + + /** An unknown generation constrains nothing, exactly like an unknown id. */ + @Test + fun aScopeWithNoGenerationIgnoresIt() { + val scope = ExternalRouteScope.Identity(serverId = "server-a", profileId = "kids") + + assertTrue( + scope.matches(serverId = "server-a", profileId = "kids", identityGeneration = 99L), + ) + } + /** + * The defect this branch exists to fix, on the external-link path: a + * notification for item B while item A's detail is showing must not reuse + * A's entry, or Back skips A. + */ + @Test + fun anExternalItemLinkIsSingleTopOnlyForTheSameItem() { + assertTrue( + isSameItemDetail( + currentDestinationRoute = Route.ItemDetail.ROUTE, + currentContentId = "movie-1", + targetRoute = "item/movie-1", + ), + ) + assertFalse( + isSameItemDetail( + currentDestinationRoute = Route.ItemDetail.ROUTE, + currentContentId = "movie-1", + targetRoute = "item/movie-2", + ), + ) + // Encoded ids must still compare equal to the decoded entry argument. + assertTrue( + isSameItemDetail( + currentDestinationRoute = Route.ItemDetail.ROUTE, + currentContentId = "tt 1/2", + targetRoute = "item/tt%201%2F2", + ), + ) + // Not on a detail screen at all. + assertFalse( + isSameItemDetail( + currentDestinationRoute = Route.Player.ROUTE, + currentContentId = "movie-1", + targetRoute = "item/movie-1", + ), + ) } } diff --git a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt index b02018ff8..d6cad9408 100644 --- a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt +++ b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt @@ -52,6 +52,17 @@ class TvProfileSelectionViewModel( loadProfiles() } + /** + * The identity the displayed grid was fetched under. + * + * Every selection is qualified with it, protected or not. Passing null for + * unprotected picks disabled the guard for exactly the case with no PIN + * round trip to re-establish scope — so if another account became active + * between the grid being accepted and the commit entering the barrier, one + * account's profile id was written into the other's token slot. + */ + private var gridScope: AuthScopeSnapshot? = null + fun loadProfiles() { val load = ++loadAttempt viewModelScope.launch { @@ -65,6 +76,8 @@ class TvProfileSelectionViewModel( _uiState.update { it.copy(isLoading = false, profiles = emptyList()) } return@launch } + // The grid the viewer is about to pick from belongs to this scope. + gridScope = scope when (val result = listed) { is ApiResult.Success -> { _uiState.update { @@ -120,7 +133,10 @@ class TvProfileSelectionViewModel( } return } - commitSelection(profile) + // Qualified by the grid's scope. An unprotected pick has no PIN round + // trip to re-establish identity, so without this it was the one path + // that committed unguarded. + commitSelection(profile, expectedScope = gridScope) } fun onPinDialogDismissed() { From 07f48d0bfc9e7edd51b50472de1330f85cda4643 Mon Sep 17 00:00:00 2001 From: rxwatcher Date: Thu, 6 Aug 2026 09:16:58 +0200 Subject: [PATCH 6/6] fix(nav, profiles): close the second review round on this branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex reviewed this branch twice and fixed the remaining findings itself. Three of the four problems on the second pass were introduced by my own remediation, and a fourth turned up here. The grid scope now moves only with a grid the app actually accepted. Assigning it before the result meant a reload that FAILED under a new identity left the old grid on screen qualified by the new scope, so a profile from the old session committed as belonging to the new one — worse than the unguarded commit it replaced. Nulling it on a scope mismatch then opened a second hole: a tap dispatched after the grid cleared committed unguarded precisely because the scope was null. Both are closed, and a cleared grid can no longer select at all. External-route single-top is an explicit per-destination policy rather than punctuation matching. My version keyed on whether the route contained '?' or '/', which happened to work for the cases I had in mind and not for others; the policy now names each destination an external request can produce — inbox, item, player, downloads, pair_device, invite_claim — so a second invitation gets its own entry instead of collapsing onto the first. The player pop stays conditional on the player being the current destination. Popping a player that sits below newer history would take that history with it, which is a worse trade than leaving the rarer case saved. Notifications still carry no identity generation, deliberately: that counter restarts at zero every process, so persisting it into a PendingIntent would refuse a legitimate notification tapped after the app was killed. Closing that needs a durable epoch, not a process-local count. The KDoc says so rather than implying coverage it does not have. Tests added for every path above, including the ones that had none: a reload that fails under a new scope, a tap after a cleared grid, the player pop with the player absent, on top, and below other entries, the full single-top decision including invite_claim, and notification extras through to the delivered scope. Full suite green on all four modules, verified here across repeated --rerun-tasks runs rather than taken on trust: Codex's sandbox could not run Gradle and correctly declined to claim it had. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW --- .../android/ui/navigation/AppNavigation.kt | 19 +-- .../ui/navigation/ExternalRouteNavigation.kt | 27 ++++ .../profiles/ProfileSelectionViewModel.kt | 16 +- ...PushNotificationDeliveryIntegrationTest.kt | 39 +++++ .../navigation/ExternalRouteNavigationTest.kt | 45 ++++++ .../profiles/ProfileSelectionGridScopeTest.kt | 152 ++++++++++++++++++ .../profiles/TvProfileSelectionViewModel.kt | 5 + .../TvProfileSelectionGridScopeTest.kt | 150 +++++++++++++++++ 8 files changed, 437 insertions(+), 16 deletions(-) create mode 100644 androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationDeliveryIntegrationTest.kt create mode 100644 androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionGridScopeTest.kt create mode 100644 androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionGridScopeTest.kt diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt index 5ca195a11..eb235395f 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.kt @@ -195,8 +195,9 @@ fun AppNavigation( // and silently discard state the viewer expected back. // Leaving that rarer case saved is the pre-existing // behaviour; destroying history to fix it is worse. - if (navController.currentBackStackEntry?.destination?.route == - Route.Player.ROUTE + if (shouldPopPlayerBeforeExternalTab( + navController.currentBackStackEntry?.destination?.route, + ) ) { navController.popBackStack( route = Route.Player.ROUTE, @@ -218,7 +219,7 @@ fun AppNavigation( // external link to item B while item A's detail is showing // reused A's entry and Back skipped A entirely — the same // defect this branch fixes for in-app navigation. - val sameItemDetail = isSameItemDetail( + val useSingleTop = shouldLaunchExternalRouteSingleTop( currentDestinationRoute = navController.currentBackStackEntry ?.destination?.route, currentContentId = navController.currentBackStackEntry @@ -230,14 +231,10 @@ fun AppNavigation( if (replaceCurrentPlayer) { popUpTo(Route.Player.ROUTE) { inclusive = true } } - // Affirmative, not "everything except item". A route - // carrying arguments identifies a specific thing, so - // reusing the node collapses two different requests into - // one — invite_claim with a different token being the - // case that bit: a second invitation replaced the first - // instead of getting its own entry. - launchSingleTop = replaceCurrentPlayer || sameItemDetail || - !route.contains('?') && !route.contains('/') + // Decided from the finite external-route producer set, + // not punctuation. A route's spelling does not say + // whether its arguments identify a distinct request. + launchSingleTop = useSingleTop } } }, diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt index 0e55d52f1..c63a06156 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigation.kt @@ -133,6 +133,33 @@ internal fun shouldReplaceCurrentPlayer( ): Boolean = currentDestinationRoute == Route.Player.ROUTE && targetRoute.startsWith("player/") +/** + * Whether an external tab switch may remove the player without also removing + * newer history above it. An inclusive route pop removes the target and every + * entry above it, so only the current player is a safe target. + */ +internal fun shouldPopPlayerBeforeExternalTab(currentDestinationRoute: String?): Boolean = + currentDestinationRoute == Route.Player.ROUTE + +/** + * Whether AndroidX may reuse the current destination node for [targetRoute]. + * + * External requests currently produce only Inbox, item detail, player, + * pairing, invitation, and the Downloads tab (handled before this function). + * Inbox has no arguments. Item detail is reusable only for the same decoded + * content id. A player target is replaced explicitly when a player is on top. + * Pairing and invitation routes carry one-shot arguments, so each delivery + * must retain its own entry. + */ +internal fun shouldLaunchExternalRouteSingleTop( + currentDestinationRoute: String?, + currentContentId: String?, + targetRoute: String, +): Boolean = + shouldReplaceCurrentPlayer(currentDestinationRoute, targetRoute) || + isSameItemDetail(currentDestinationRoute, currentContentId, targetRoute) || + (currentDestinationRoute == Route.Inbox.route && targetRoute == Route.Inbox.route) + /** Parses the canonical in-app player route carried by an external request. */ internal fun playerRouteIntentOrNull(route: String): MobilePlayerRouteIntent? { if (!route.startsWith("player/")) return null diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt index fbf94fd74..149ad6982 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionViewModel.kt @@ -49,11 +49,6 @@ class ProfileSelectionViewModel( loadProfiles() } - /** - * @param clearError false keeps an existing error banner (e.g. a failed - * delete's explanation) visible across the follow-up list refresh, which - * would otherwise silently swallow it. - */ /** * The identity the displayed grid was fetched under. * @@ -65,6 +60,11 @@ class ProfileSelectionViewModel( */ private var gridScope: AuthScopeSnapshot? = null + /** + * @param clearError false keeps an existing error banner (e.g. a failed + * delete's explanation) visible across the follow-up list refresh, which + * would otherwise silently swallow it. + */ fun loadProfiles(clearError: Boolean = true) { val load = ++loadAttempt viewModelScope.launch { @@ -133,6 +133,12 @@ class ProfileSelectionViewModel( // In manage mode, tapping opens edit -- handled by the screen composable. return } + // A click can already be queued when a scope mismatch clears the grid. + // gridScope is null then, and passing it through would disable the + // repository guard. Accept only profiles in the grid that is still + // displayed; use the id because refreshed model instances need not be + // referentially identical to the card's captured value. + if (_uiState.value.profiles.none { it.id == profile.id }) return // Bump before branching: ANY accepted selection supersedes a // verification still in flight, including picking an unprotected diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationDeliveryIntegrationTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationDeliveryIntegrationTest.kt new file mode 100644 index 000000000..b3b490ae2 --- /dev/null +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/PushNotificationDeliveryIntegrationTest.kt @@ -0,0 +1,39 @@ +package org.siloserver.silo.android.push + +import org.siloserver.silo.android.ui.navigation.ExternalRouteScope +import org.siloserver.silo.android.ui.navigation.notificationExternalRouteOrNull +import kotlin.test.Test +import kotlin.test.assertEquals + +class PushNotificationDeliveryIntegrationTest { + @Test + fun `posted notification retains its identity through route delivery`() { + // Model the extras contract shared by PushNotificationPresenter and + // MainActivity without starting Android, the Application, or its Koin + // graph. + val postedExtras = mapOf( + PushNotificationPresenter.EXTRA_NAV_ROUTE to "item/episode-1", + PushNotificationPresenter.EXTRA_SERVER_ID to "server-a", + PushNotificationPresenter.EXTRA_PROFILE_ID to "kids", + ) + + val (route, scope) = requireNotNull( + notificationExternalRouteOrNull( + route = postedExtras[PushNotificationPresenter.EXTRA_NAV_ROUTE], + serverId = postedExtras[PushNotificationPresenter.EXTRA_SERVER_ID], + profileId = postedExtras[PushNotificationPresenter.EXTRA_PROFILE_ID], + ), + ) + + assertEquals("item/episode-1", route) + assertEquals( + ExternalRouteScope.Identity( + serverId = "server-a", + profileId = "kids", + // Process-local generations cannot be persisted in PendingIntents. + identityGeneration = null, + ), + scope, + ) + } +} diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt index b7c97e343..defa84116 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/navigation/ExternalRouteNavigationTest.kt @@ -168,6 +168,51 @@ class ExternalRouteNavigationTest { ) } + @Test + fun externalTabOnlyPopsAPlayerThatIsOnTop() { + val playerAbsent = listOf(Route.Home.route, Route.ItemDetail.ROUTE) + val playerOnTop = listOf(Route.Home.route, Route.Player.ROUTE) + val playerBelowAnotherEntry = listOf(Route.Home.route, Route.Player.ROUTE, Route.ItemDetail.ROUTE) + + assertFalse(shouldPopPlayerBeforeExternalTab(playerAbsent.lastOrNull())) + assertTrue(shouldPopPlayerBeforeExternalTab(playerOnTop.lastOrNull())) + // A player below an item entry is deliberately left in history: an + // inclusive pop to the player would also discard the item above it. + assertFalse(shouldPopPlayerBeforeExternalTab(playerBelowAnotherEntry.lastOrNull())) + } + + @Test + fun externalSingleTopPolicyCoversEveryProducedRoute() { + val cases = listOf( + // Notification producers. + Triple(Route.Inbox.route, Route.Inbox.route, true), + Triple(Route.Home.route, Route.Inbox.route, false), + Triple(Route.ItemDetail.ROUTE, "item/movie-1", true), + Triple(Route.ItemDetail.ROUTE, "item/movie-2", false), + // Content-link producers. Downloads takes the separate tab path; + // item and player still exercise this policy. + Triple(Route.Home.route, "item/movie-1", false), + Triple(Route.Home.route, "player/movie-1", false), + Triple(Route.Player.ROUTE, "player/movie-1?quality=original", true), + // Device-login and invitation producers. Distinct argument sets + // must get distinct entries even though they share a graph node. + Triple(Route.PairDevice.ROUTE, "pair_device?code=123&serverOrigin=https%3A%2F%2Fa", false), + Triple(Route.InviteClaim.ROUTE, "invite_claim?server=https%3A%2F%2Fa&token=one", false), + ) + + cases.forEach { (currentRoute, targetRoute, expected) -> + assertEquals( + expected, + shouldLaunchExternalRouteSingleTop( + currentDestinationRoute = currentRoute, + currentContentId = if (currentRoute == Route.ItemDetail.ROUTE) "movie-1" else null, + targetRoute = targetRoute, + ), + "$currentRoute -> $targetRoute", + ) + } + } + @Test fun canonicalPlayerTargetParsesEveryPlaybackChoice() { assertEquals( diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionGridScopeTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionGridScopeTest.kt new file mode 100644 index 000000000..e37f31ac5 --- /dev/null +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/profiles/ProfileSelectionGridScopeTest.kt @@ -0,0 +1,152 @@ +package org.siloserver.silo.android.ui.screens.profiles + +import io.ktor.client.HttpClient +import io.ktor.client.engine.mock.MockEngine +import io.ktor.client.engine.mock.respond +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.siloserver.silo.model.profile.Profile +import org.siloserver.silo.network.ApiResult +import org.siloserver.silo.network.AuthScopeSnapshot +import org.siloserver.silo.network.DefaultIdentityTransitionBarrier +import org.siloserver.silo.network.IdentityTransitionKind +import org.siloserver.silo.network.ProfileIdentity +import org.siloserver.silo.network.TokenManager +import org.siloserver.silo.network.TokenManagerImpl +import org.siloserver.silo.network.api.ProfileApi +import org.siloserver.silo.repository.ProfileRepository +import kotlin.test.AfterTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +@OptIn(ExperimentalCoroutinesApi::class) +class ProfileSelectionGridScopeTest { + @AfterTest + fun tearDown() { + Dispatchers.resetMain() + } + + @Test + fun `failed reload under a new scope does not qualify the old grid as new`() = runTest { + Dispatchers.setMain(UnconfinedTestDispatcher(testScheduler)) + val barrier = DefaultIdentityTransitionBarrier() + val tokens = ScopeTokenManager(barrier) + val oldProfile = Profile(id = "old-profile", name = "Old profile") + val repository = QueueProfileRepository( + tokenManager = tokens, + barrier = barrier, + results = ArrayDeque( + listOf( + ApiResult.Success(listOf(oldProfile)), + ApiResult.Error(500, "failed", "Reload failed"), + ApiResult.Error(500, "failed", "Reload still failed"), + ), + ), + ) + val viewModel = ProfileSelectionViewModel(repository) + advanceUntilIdle() + + // Establish the init load before changing identity. Otherwise the + // Unconfined dispatcher can let the init response observe the switch + // below and correctly reject what the test intended as its old grid. + assertEquals(listOf(oldProfile), viewModel.uiState.value.profiles) + + barrier.changing(IdentityTransitionKind.SERVER_SWITCH) { + tokens.serverId = "server-b" + } + viewModel.loadProfiles() + advanceUntilIdle() + + // A failed reload leaves the previous grid visible, but must not move + // that grid's scope to server-b. + assertEquals(listOf(oldProfile), viewModel.uiState.value.profiles) + + viewModel.onProfileTapped(oldProfile) + advanceUntilIdle() + + // The retained card is still qualified by server-a. Selecting it under + // server-b therefore fails closed and drops the now-stale grid. + assertEquals(emptyList(), viewModel.uiState.value.profiles) + assertEquals(emptyList(), tokens.committedProfiles) + assertNull(viewModel.uiState.value.selectedProfileId) + } + + @Test + fun `tap dispatched after a scope mismatch cannot select from the cleared grid`() = runTest { + Dispatchers.setMain(UnconfinedTestDispatcher(testScheduler)) + val barrier = DefaultIdentityTransitionBarrier() + val tokens = ScopeTokenManager(barrier) + val oldProfile = Profile(id = "old-profile", name = "Old profile") + val repository = QueueProfileRepository( + tokenManager = tokens, + barrier = barrier, + results = ArrayDeque( + listOf( + ApiResult.Success(listOf(oldProfile)), + ApiResult.Success(emptyList()), + ), + ), + beforeResult = { call -> + if (call == 2) { + barrier.changing(IdentityTransitionKind.SERVER_SWITCH) { + tokens.serverId = "server-b" + } + } + }, + ) + val viewModel = ProfileSelectionViewModel(repository) + + viewModel.loadProfiles() + viewModel.onProfileTapped(oldProfile) + + assertEquals(emptyList(), viewModel.uiState.value.profiles) + assertEquals(emptyList(), tokens.committedProfiles) + } +} + +private class QueueProfileRepository( + tokenManager: TokenManager, + barrier: DefaultIdentityTransitionBarrier, + private val results: ArrayDeque>>, + private val beforeResult: suspend (Int) -> Unit = {}, +) : ProfileRepository( + profileApi = ProfileApi(HttpClient(MockEngine { respond("{}") })), + tokenManager = tokenManager, + identityTransitions = barrier, +) { + private var calls = 0 + + override suspend fun listProfiles(): ApiResult> { + beforeResult(++calls) + return results.removeFirst() + } + + override suspend fun getActiveProfileId(): String? = null +} + +private class ScopeTokenManager( + private val barrier: DefaultIdentityTransitionBarrier, +) : TokenManager by TokenManagerImpl(barrier) { + var serverId: String = "server-a" + val committedProfiles = mutableListOf() + + override suspend fun snapshotCurrentScope(): AuthScopeSnapshot = AuthScopeSnapshot( + serverId = serverId, + profileId = null, + serverUrl = "https://$serverId", + profileToken = null, + identityGeneration = barrier.generation.value, + ) + + override suspend fun getCurrentServerId(): String = serverId + + override suspend fun setProfileIdentity(profileId: String?, profileToken: String?) { + committedProfiles += ProfileIdentity(profileId, profileToken) + } +} diff --git a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt index 13b3c71b7..67e586854 100644 --- a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt +++ b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionViewModel.kt @@ -126,6 +126,11 @@ class TvProfileSelectionViewModel( // Manage-mode taps open edit, handled by the screen composable. return } + // Focus/Select can already be dispatched when a scope mismatch clears + // the grid. A null gridScope deliberately means no grid metadata, but + // it also disables the repository guard, so reject cards that are no + // longer part of the accepted grid. + if (_uiState.value.profiles.none { it.id == profile.id }) return // Bump before branching: ANY accepted selection supersedes a // verification still in flight, including choosing an unprotected // profile while a protected one is mid-verify. diff --git a/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionGridScopeTest.kt b/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionGridScopeTest.kt new file mode 100644 index 000000000..7d763f98a --- /dev/null +++ b/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/profiles/TvProfileSelectionGridScopeTest.kt @@ -0,0 +1,150 @@ +package org.siloserver.silo.tv.ui.screens.profiles + +import io.ktor.client.HttpClient +import io.ktor.client.engine.mock.MockEngine +import io.ktor.client.engine.mock.respond +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.siloserver.silo.model.profile.Profile +import org.siloserver.silo.network.ApiResult +import org.siloserver.silo.network.AuthScopeSnapshot +import org.siloserver.silo.network.DefaultIdentityTransitionBarrier +import org.siloserver.silo.network.IdentityTransitionKind +import org.siloserver.silo.network.ProfileIdentity +import org.siloserver.silo.network.TokenManager +import org.siloserver.silo.network.TokenManagerImpl +import org.siloserver.silo.network.api.ProfileApi +import org.siloserver.silo.repository.ProfileRepository +import kotlin.test.AfterTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +@OptIn(ExperimentalCoroutinesApi::class) +class TvProfileSelectionGridScopeTest { + @AfterTest + fun tearDown() { + Dispatchers.resetMain() + } + + @Test + fun `failed reload under a new scope does not qualify the old grid as new`() = runTest { + Dispatchers.setMain(UnconfinedTestDispatcher(testScheduler)) + val barrier = DefaultIdentityTransitionBarrier() + val tokens = TvScopeTokenManager(barrier) + val oldProfile = Profile(id = "old-profile", name = "Old profile") + val repository = TvQueueProfileRepository( + tokenManager = tokens, + barrier = barrier, + results = ArrayDeque( + listOf( + ApiResult.Success(listOf(oldProfile)), + ApiResult.Error(500, "failed", "Reload failed"), + ApiResult.Error(500, "failed", "Reload still failed"), + ), + ), + ) + val viewModel = TvProfileSelectionViewModel(repository) + advanceUntilIdle() + + // Establish the init load before changing identity. Otherwise the + // Unconfined dispatcher can let the init response observe the switch + // below and correctly reject what the test intended as its old grid. + assertEquals(listOf(oldProfile), viewModel.uiState.value.profiles) + + barrier.changing(IdentityTransitionKind.SERVER_SWITCH) { + tokens.serverId = "server-b" + } + viewModel.loadProfiles() + advanceUntilIdle() + + // A failed reload leaves the previous grid visible, but must not move + // that grid's scope to server-b. + assertEquals(listOf(oldProfile), viewModel.uiState.value.profiles) + + viewModel.onProfileSelected(oldProfile) + advanceUntilIdle() + + // The retained card is still qualified by server-a. Selecting it under + // server-b therefore fails closed and drops the now-stale grid. + assertEquals(emptyList(), viewModel.uiState.value.profiles) + assertEquals(emptyList(), tokens.committedProfiles) + assertNull(viewModel.uiState.value.selectedProfileId) + } + + @Test + fun `selection dispatched after a scope mismatch cannot use the cleared grid`() = runTest { + Dispatchers.setMain(UnconfinedTestDispatcher(testScheduler)) + val barrier = DefaultIdentityTransitionBarrier() + val tokens = TvScopeTokenManager(barrier) + val oldProfile = Profile(id = "old-profile", name = "Old profile") + val repository = TvQueueProfileRepository( + tokenManager = tokens, + barrier = barrier, + results = ArrayDeque( + listOf( + ApiResult.Success(listOf(oldProfile)), + ApiResult.Success(emptyList()), + ), + ), + beforeResult = { call -> + if (call == 2) { + barrier.changing(IdentityTransitionKind.SERVER_SWITCH) { + tokens.serverId = "server-b" + } + } + }, + ) + val viewModel = TvProfileSelectionViewModel(repository) + + viewModel.loadProfiles() + viewModel.onProfileSelected(oldProfile) + + assertEquals(emptyList(), viewModel.uiState.value.profiles) + assertEquals(emptyList(), tokens.committedProfiles) + } +} + +private class TvQueueProfileRepository( + tokenManager: TokenManager, + barrier: DefaultIdentityTransitionBarrier, + private val results: ArrayDeque>>, + private val beforeResult: suspend (Int) -> Unit = {}, +) : ProfileRepository( + profileApi = ProfileApi(HttpClient(MockEngine { respond("{}") })), + tokenManager = tokenManager, + identityTransitions = barrier, +) { + private var calls = 0 + + override suspend fun listProfiles(): ApiResult> { + beforeResult(++calls) + return results.removeFirst() + } +} + +private class TvScopeTokenManager( + private val barrier: DefaultIdentityTransitionBarrier, +) : TokenManager by TokenManagerImpl(barrier) { + var serverId: String = "server-a" + val committedProfiles = mutableListOf() + + override suspend fun snapshotCurrentScope(): AuthScopeSnapshot = AuthScopeSnapshot( + serverId = serverId, + profileId = null, + serverUrl = "https://$serverId", + profileToken = null, + identityGeneration = barrier.generation.value, + ) + + override suspend fun getCurrentServerId(): String = serverId + + override suspend fun setProfileIdentity(profileId: String?, profileToken: String?) { + committedProfiles += ProfileIdentity(profileId, profileToken) + } +}