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..7e17ce387 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/MainActivity.kt @@ -38,6 +38,8 @@ 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.ExternalRouteScope +import org.siloserver.silo.android.ui.navigation.notificationExternalRouteOrNull import org.siloserver.silo.android.ui.navigation.deviceLoginPairRouteOrNull import org.siloserver.silo.android.ui.navigation.hasLocalDownloadsForScope import org.siloserver.silo.android.ui.navigation.inviteClaimRouteOrNull @@ -77,6 +79,27 @@ 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" + + /** + * 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" } private val externalRouteRequestFactory = ExternalRouteRequestFactory() @@ -85,6 +108,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 +126,7 @@ class MainActivity : ComponentActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) + consumedExternalRoute = savedInstanceState?.getString(STATE_CONSUMED_EXTERNAL_ROUTE) enableEdgeToEdge() maybeRequestNotificationPermission() maybeRequestLegacyPublicDownloadPermission() @@ -112,10 +144,11 @@ 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) { + queueExternalRouteFrom(intent) + } launchAuthenticatedStartupWarmup(route) } @@ -151,7 +184,25 @@ 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, + // 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 +226,19 @@ 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) - ?: inviteClaimRouteOrNull(intent.dataString) - ?: notificationRouteOrNull(intent) - ?: contentDeepLinkRouteOrNull(intent.dataString) - route?.let { pendingExternalRouteRequests.value = externalRouteRequestFactory.create(it) } + lifecycleScope.launch { queueExternalRouteFrom(intent) } } /** @@ -228,6 +284,95 @@ 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, + identityGeneration = scope.identityGeneration, + ) + } + // 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), @@ -250,11 +395,18 @@ 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) + // 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/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 d2b0b27aa..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 @@ -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,8 +119,15 @@ 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() val overlayPrefsStore: OverlayPrefsStore = koinInject() val siloCastController: SiloCastController = koinInject() val diagnosticsViewModel = koinViewModel() @@ -162,15 +173,87 @@ 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) { + // 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. + // ONLY when the player is the current destination. An + // inclusive pop also removes everything above its target, + // so a player sitting BELOW other entries — an external + // item link pushed over it, say — would take those with it + // 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 (shouldPopPlayerBeforeExternalTab( + navController.currentBackStackEntry?.destination?.route, + ) + ) { + navController.popBackStack( + route = Route.Player.ROUTE, + inclusive = true, + saveState = false, + ) + } + navController.navigate(route) { + tabSwitchNavOptions(navController.bottomMostTabRoute()) + } + } else { + val replaceCurrentPlayer = shouldReplaceCurrentPlayer( + currentDestinationRoute = navController.currentBackStackEntry + ?.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 useSingleTop = shouldLaunchExternalRouteSingleTop( + currentDestinationRoute = navController.currentBackStackEntry + ?.destination?.route, + currentContentId = navController.currentBackStackEntry + ?.arguments + ?.getString("contentId"), + targetRoute = route, + ) + navController.navigate(route) { + if (replaceCurrentPlayer) { + popUpTo(Route.Player.ROUTE) { inclusive = true } + } + // 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 + } + } + }, + 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, + identityGeneration = live?.identityGeneration, + ) } - launchSingleTop = true } }, onConsumed = onExternalRouteConsumed, @@ -326,28 +409,130 @@ fun AppNavigation( nullable = true defaultValue = null }, + navArgument("serverOrigin") { + type = NavType.StringType + nullable = true + 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") - 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) ---- @@ -458,6 +643,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 } } } @@ -1023,3 +1215,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/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/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/DeviceLoginRouteParser.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/DeviceLoginRouteParser.kt index 4f7650e3c..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,92 @@ 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, + ) +} + +/** + * 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]. + * + * 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 == required } private fun URI.isDeviceLoginUri(): Boolean { @@ -56,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 917af1b03..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 @@ -1,24 +1,92 @@ 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 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?, + /** + * 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. + * + * Set for routes captured in-process. NOT set for notifications: the + * counter restarts at zero in every process, so persisting it into a + * PendingIntent would refuse a legitimate notification tapped after the + * app was killed. Notifications therefore keep only server+profile + * pinning — see [notificationExternalRouteOrNull]. + */ + 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?, + identityGeneration: Long?, + ): Boolean = + (this.serverId == null || this.serverId == serverId) && + (this.profileId == null || this.profileId == profileId) && + (this.identityGeneration == null || this.identityGeneration == identityGeneration) + } +} + /** A single external-navigation delivery, distinct even when its route repeats. */ class ExternalRouteRequest internal constructor( val generation: Long, val route: String, + val scope: ExternalRouteScope = ExternalRouteScope.Unscoped, ) internal class ExternalRouteRequestFactory { private var latestGeneration = 0L - fun create(route: String): ExternalRouteRequest = + fun create( + route: String, + scope: ExternalRouteScope = ExternalRouteScope.Unscoped, + ): ExternalRouteRequest = ExternalRouteRequest( generation = ++latestGeneration, route = route, + scope = scope, ) } @@ -28,19 +96,83 @@ 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, ): 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 + // 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('?', "") @@ -147,6 +279,11 @@ internal suspend fun consumeExternalRouteOnce( pendingExternalRoute: ExternalRouteRequest?, currentDestinationRoutes: Flow, isAlreadyAtRoute: (String) -> Boolean = { false }, + /** + * Whether the request's [ExternalRouteScope] still matches the live + * identity. Evaluated AFTER the wait, not before it. + */ + isStillValidForScope: suspend (ExternalRouteScope) -> Boolean = { true }, navigate: (String) -> Unit, onConsumed: (ExternalRouteRequest) -> Unit, ) { @@ -157,8 +294,11 @@ internal suspend fun consumeExternalRouteOnce( currentDestinationRoutes.first { currentRoute -> isPreAuthenticationTarget || currentRoute !in preAuthenticationDestinationRoutes } - if (!isAlreadyAtRoute(route)) { + val scopeStillValid = isStillValidForScope(request.scope) + if (scopeStillValid && !isAlreadyAtRoute(route)) { navigate(route) } + // 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..b99b65009 --- /dev/null +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/NotificationExternalRoute.kt @@ -0,0 +1,37 @@ +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. + * + * The identity GENERATION is deliberately not carried, unlike an in-process + * route captured while the app is running. That counter restarts at zero in + * every process, so a notification tapped after the app has been killed would + * be compared against a generation that means something different — and a + * perfectly legitimate notification would be refused. Notifications are + * therefore pinned by server and profile only, and remain deliverable across a + * sign-out and back in to the same account. Closing that would need a durable + * identity epoch rather than a process-local counter. + */ +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, + // Explicit: see the note above on why a process-local generation must + // not be persisted into a PendingIntent. + identityGeneration = null, + ) +} 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..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 @@ -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 /** @@ -37,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("?") @@ -51,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}" } } @@ -88,8 +98,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") @@ -112,7 +125,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 +155,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 +179,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 +220,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 +239,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 +268,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/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/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/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..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 @@ -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,22 +39,63 @@ 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() } + /** + * 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 + /** * @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 { _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)) { + // The displayed grid is gone, so its scope must go with it. + // Leaving a scope behind for an empty grid is stale metadata + // that a later selection could be qualified against. + gridScope = null + _uiState.update { it.copy(isLoading = false, profiles = emptyList()) } + return@launch + } + + when (result) { is ApiResult.Success -> { + // The scope moves with the grid, and ONLY with it. Assigning + // it before this point meant a reload that failed under a + // NEW identity left the OLD grid on screen qualified by the + // NEW scope — so picking a profile from the old session was + // accepted as belonging to the new one. That is worse than + // the unguarded commit this was meant to fix. + gridScope = scope _uiState.update { it.copy(isLoading = false, profiles = result.data, activeProfileId = activeId) } @@ -89,6 +133,17 @@ 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 + // profile while a protected one is mid-verify. + pinAttempt++ if (profile.hasPin) { _uiState.update { @@ -99,7 +154,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) } } @@ -108,15 +166,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 +213,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 +267,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/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/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/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/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..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 @@ -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 @@ -24,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"), ) } @@ -33,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"), ) } @@ -57,4 +61,43 @@ class DeviceLoginRouteParserTest { assertNull(deviceLoginPairRouteOrNull("")) assertNull(deviceLoginPairRouteOrNull("silo://device?token=&code=")) } + + // --- origin --- + + @Test + fun `an app scheme device link names no server`() { + assertEquals(DeviceLoginScope.Unscoped, deviceLoginScope("silo://device?code=ABCD")) + // No origin in the route either. + assertEquals("pair_device?code=ABCD", deviceLoginPairRouteOrNull("silo://device?code=ABCD")) + } + + @Test + 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 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 does not parse`() { + assertEquals(DeviceLoginScope.Invalid, deviceLoginScope("https:///device?code=ABCD")) + assertNull(deviceLoginPairRouteOrNull("https:///device?code=ABCD")) + } + + /** Port 0 is not a valid origin, so the link is not usable either. */ + @Test + fun `a device link with an invalid port does not parse`() { + assertEquals( + DeviceLoginScope.Invalid, + deviceLoginScope("https://silo.example:0/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 16533a0c8..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( @@ -293,4 +338,185 @@ class ExternalRouteNavigationTest { subtitleTrackIndex = subtitleTrackIndex, resumePositionSeconds = resumePositionSeconds, ) + + /** + * 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 = "inbox", + scope = ExternalRouteScope.Identity(serverId = "server-b", profileId = "kids"), + ) + var navigated: String? = null + var consumed = 0 + + consumeExternalRouteOnce( + pendingExternalRoute = request, + currentDestinationRoutes = flowOf("home"), + isStillValidForScope = { false }, + navigate = { navigated = it }, + onConsumed = { consumed++ }, + ) + + 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) + } + + @Test + fun aRequestWhoseServerStillMatchesIsDelivered() = runTest { + val request = ExternalRouteRequestFactory() + .create( + route = "inbox", + scope = ExternalRouteScope.Identity(serverId = "server-b", profileId = "kids"), + ) + var navigated: String? = null + var consumed = 0 + + consumeExternalRouteOnce( + pendingExternalRoute = request, + currentDestinationRoutes = flowOf("home"), + isStillValidForScope = { true }, + navigate = { navigated = it }, + onConsumed = { consumed++ }, + ) + + assertEquals("inbox", 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"), + 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", 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", identityGeneration = null)) + } + + /** 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", 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. */ + @Test + fun anEmptyScopeMatchesAnything() { + val scope = ExternalRouteScope.Identity(serverId = null, profileId = null) + + 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/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 0b1f13a80..c40155f5e 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,35 +843,34 @@ private fun noOpClient(): HttpClient = private suspend fun PlayerViewModel.awaitState( predicate: (PlayerViewModel.PlayerUiState) -> Boolean, ) { - awaitCondition { predicate(uiState.value) } + awaitRealTime { uiState.first(predicate) } } /** - * Polls [predicate] off the test scheduler, on a dispatcher that cannot be - * starved. + * Runs [block] under a REAL, generous deadline. * - * The deadline is wall-clock, so it has to outlast the machine being busy. This - * used to poll on a single-parallelism slice of [Dispatchers.Default], which is - * sized to CPU count and fully subscribed when the Gradle suite runs its - * modules in parallel — the polling coroutine got almost no time while the - * timeout kept counting real seconds, and the test failed on a loaded machine - * while passing alone. [Dispatchers.IO] is elastic, so the poll keeps running - * under load. + * 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. * - * The timeout is a backstop against a genuine hang, not an assertion about - * latency, so it is generous. A real hang still fails, just later. + * 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 awaitCondition(predicate: () -> Boolean) { - withContext(Dispatchers.IO) { - withTimeout(AWAIT_CONDITION_TIMEOUT_MS) { - while (!predicate()) { - delay(5) - } - } +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 AWAIT_CONDITION_TIMEOUT_MS = 30_000L private const val SERVER_ID = "server" private const val PROFILE_ID = "profile" 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/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/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..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 @@ -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,15 +42,49 @@ 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() } + /** + * 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 { _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)) { + // The displayed grid is gone, so its scope must go with it. + gridScope = null + _uiState.update { it.copy(isLoading = false, profiles = emptyList()) } + return@launch + } + when (val result = listed) { is ApiResult.Success -> { + // The scope moves with the grid, and ONLY with it. See the + // phone ViewModel: assigning it before the result meant a + // failed reload under a NEW identity left the OLD grid + // qualified by the NEW scope. + gridScope = scope _uiState.update { it.copy(isLoading = false, profiles = result.data) } @@ -89,6 +126,16 @@ 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. + pinAttempt++ + if (profile.hasPin) { // Open the PIN dialog; actual selection happens in onPinEntered. _uiState.update { @@ -96,10 +143,15 @@ 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() { + // Abandon any in-flight verification for the dismissed profile. + pinAttempt++ _uiState.update { it.copy(pinProfile = null, pinError = null, isVerifyingPin = false) } @@ -107,16 +159,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 +202,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/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/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) + } +} 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 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..bb2762bf2 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,45 @@ 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 + * 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() } @@ -325,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") 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/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 ed8de9456..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. @@ -75,6 +78,47 @@ interface TokenManager { suspend fun setProfileId(profileId: String?) 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. + * + * 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..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,24 @@ 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 { + // 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/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() { 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(), + ) + } +}