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 eb235395f..6c1fc7979 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 @@ -1066,9 +1066,14 @@ fun AppNavigation( ) } composable(Route.Admin.route) { - org.siloserver.silo.android.ui.screens.admin.AdminStatsScreen( - onBackClick = { navController.popBackStack() }, - ) + // Gated at the destination as well as the entry: the route stays + // registered, so restored navigation reaches it directly and the + // stats screen calls the admin API the moment it composes. + org.siloserver.silo.android.ui.screens.admin.AdminRouteGate { + org.siloserver.silo.android.ui.screens.admin.AdminStatsScreen( + onBackClick = { navController.popBackStack() }, + ) + } } composable(Route.Watchlist.route) { WatchlistScreen( diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModel.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModel.kt index 30b94410d..f726d2388 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModel.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModel.kt @@ -7,6 +7,7 @@ import org.siloserver.silo.model.auth.isActingAdmin import org.siloserver.silo.network.ApiResult import org.siloserver.silo.repository.AuthRepository import org.siloserver.silo.repository.ProfileRepository +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow @@ -34,7 +35,24 @@ class AdminEntryViewModel( ) : this( gateProvider = { val user = (authRepository.getCurrentUser() as? ApiResult.Success)?.data - val profile = profileRepository.getActiveProfile() + // Bounded retry on an unresolved profile, matching the settings + // ViewModels. isActingAdmin fails closed, and this gate guards a + // DESTINATION: a single null read would leave a genuine owner on + // "not authorized" for the lifetime of that back-stack entry, with + // no way to recover once the profile resolved. getActiveProfile + // collapses "network failed", "no active id" and "not found" into + // null, so a retry is the only signal available. + // + // Bounded because not being an admin is the ordinary case, and an + // unbounded retry would poll for every non-admin who ever lands + // here. + var profile = profileRepository.getActiveProfile() + var attempt = 1 + while (profile == null && attempt < PROFILE_RESOLVE_ATTEMPTS) { + delay(PROFILE_RESOLVE_RETRY_MS) + profile = profileRepository.getActiveProfile() + attempt += 1 + } isActingAdmin(user, profile) }, ) @@ -56,3 +74,7 @@ class AdminEntryViewModel( } } } + +/** Matches the settings ViewModels: a few quick attempts, then fail closed. */ +private const val PROFILE_RESOLVE_ATTEMPTS = 3 +private const val PROFILE_RESOLVE_RETRY_MS = 400L diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminHubScreen.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminHubScreen.kt index 5267ec703..3accef10f 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminHubScreen.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminHubScreen.kt @@ -119,7 +119,7 @@ private fun HubRow( } @Composable -private fun NotAuthorized(modifier: Modifier = Modifier) { +internal fun NotAuthorized(modifier: Modifier = Modifier) { Box(modifier = modifier.fillMaxSize(), contentAlignment = Alignment.Center) { Text( "You are not authorized to view this page.", diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminRouteGate.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminRouteGate.kt new file mode 100644 index 000000000..dfdb07977 --- /dev/null +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminRouteGate.kt @@ -0,0 +1,37 @@ +package org.siloserver.silo.android.ui.screens.admin + +import androidx.compose.runtime.Composable +import androidx.compose.runtime.collectAsState +import androidx.compose.runtime.getValue +import org.koin.compose.viewmodel.koinViewModel +import org.siloserver.silo.android.ui.components.LoadingIndicator + +/** + * Re-evaluates the acting-admin gate at the DESTINATION, not just at the entry + * that offered it. + * + * Gating only the menu row is not enough: the route stays registered, so + * restored navigation, a back-stack replay, or any future deep link reaches the + * screen directly — and an admin screen calls its API as soon as it composes. + * A gate that can be walked around is a gate in name only. + * + * This does NOT make the client a security boundary; only the server can be + * that, and the client cannot prove what the server enforces. It closes the + * client-side hole so that being refused is the default rather than a + * consequence of having arrived by the expected path. + */ +@Composable +fun AdminRouteGate( + viewModel: AdminEntryViewModel = koinViewModel(), + content: @Composable () -> Unit, +) { + val state by viewModel.uiState.collectAsState() + when { + // Nothing is shown while the gate is still resolving. Rendering the + // screen first and revoking it after would have already fired the + // admin API call this exists to prevent. + state.isLoading -> LoadingIndicator() + state.isAdminVisible -> content() + else -> NotAuthorized() + } +} diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.kt index d2741f803..8a93cf9a2 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.kt @@ -17,6 +17,7 @@ import org.siloserver.silo.network.ApiResult import org.siloserver.silo.repository.AuthRepository import org.siloserver.silo.repository.NotificationsRepository import org.siloserver.silo.repository.ProfileRepository +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow @@ -155,7 +156,21 @@ class SettingsViewModel( // The profile still supplies identity (name, role) for the admin // gate; its preference columns no longer feed this screen — those // are resolved canonically below. - when (val profileResult = profileRepository.getActiveProfileResult()) { + // Bounded retry, in the ViewModel rather than the screen. The admin + // gate fails closed on an unresolved profile, so a transient + // failure would otherwise hide the Admin row from a genuine owner + // for the life of this ViewModel. Bounded because the far more + // common reason for "no admin row" is simply not being an admin, + // and an unbounded retry would hammer the API for every ordinary + // user forever. + var profileResult = profileRepository.getActiveProfileResult() + var attempt = 1 + while (profileResult !is ApiResult.Success && attempt < PROFILE_RESOLVE_ATTEMPTS) { + delay(PROFILE_RESOLVE_RETRY_MS) + profileResult = profileRepository.getActiveProfileResult() + attempt += 1 + } + when (profileResult) { is ApiResult.Success -> { val profile = profileResult.data _uiState.update { @@ -165,8 +180,13 @@ class SettingsViewModel( } } is ApiResult.Error, is ApiResult.NetworkError -> { - // Active profile unresolved — fall back to the user role - // only (a null profile does not block an admin per the gate). + // Retries exhausted: the profile is unresolved, so the + // admin surface stays hidden. The account role is the same + // on every profile in the household, and without the + // profile there is nothing to tell the owner from a child. + // This branch is why the bug was reachable — a settings + // load that merely failed used to reveal Admin on any + // profile. It reappears next time Settings is opened. _uiState.update { it.copy(isAdminVisible = shouldShowClientAdminSurface(isActingAdmin(it.user, null))) } @@ -611,3 +631,12 @@ class SettingsViewModel( private fun downloadQualityWireValue(value: String): String = DownloadQuality.entries.firstOrNull { it.label == value }?.wire ?: DownloadQuality.Original.wire } + +/** + * How many times the profile lookup is retried before the admin gate settles. + * + * Small on purpose. The overwhelmingly common reason for no admin row is not + * being an admin, so this must not become a retry loop for every ordinary user. + */ +private const val PROFILE_RESOLVE_ATTEMPTS = 3 +private const val PROFILE_RESOLVE_RETRY_MS = 400L diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModelTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModelTest.kt index 5d75dc056..651ae240e 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModelTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModelTest.kt @@ -1,6 +1,7 @@ package org.siloserver.silo.android.ui.screens.admin import org.siloserver.silo.model.auth.User +import org.siloserver.silo.model.auth.isActingAdmin import org.siloserver.silo.model.profile.Profile import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -30,8 +31,11 @@ class AdminEntryViewModelTest { private fun profile(primary: Boolean) = Profile(id = "p1", name = "Primary", isPrimary = primary) - private fun vm(@Suppress("UNUSED_PARAMETER") user: User?, @Suppress("UNUSED_PARAMETER") profile: Profile?) = - AdminEntryViewModel(gateProvider = { true }) + // Folds the REAL gate, not a constant. The previous helper ignored both + // arguments and always returned true, so every test using it passed no + // matter what the gate did — including the case this class exists for. + private fun vm(user: User?, profile: Profile?) = + AdminEntryViewModel(gateProvider = { isActingAdmin(user, profile) }) @Test fun `acting admin gate makes the surface visible`() = runTest(dispatcher) { assertTrue(AdminEntryViewModel(gateProvider = { true }).uiState.value.isAdminVisible) @@ -44,4 +48,45 @@ class AdminEntryViewModelTest { @Test fun `not loading after refresh`() = runTest(dispatcher) { assertFalse(vm(user("admin"), profile(true)).uiState.value.isLoading) } + /** + * The reported bug, at the ViewModel: an admin ACCOUNT on a non-owner + * profile must not see the surface. The account role is identical on every + * profile, so the profile is the only thing separating them. + */ + @Test fun `admin account on a non-primary profile is refused`() = runTest(dispatcher) { + assertFalse(vm(user("admin"), profile(primary = false)).uiState.value.isAdminVisible) + } + + /** An unresolved profile is not permission — the gate fails closed. */ + @Test fun `admin account with an unresolved profile is refused`() = runTest(dispatcher) { + assertFalse(vm(user("admin"), null).uiState.value.isAdminVisible) + } + + /** And the owner still gets in once the profile resolves. */ + @Test fun `admin account on the primary profile is allowed`() = runTest(dispatcher) { + assertTrue(vm(user("admin"), profile(primary = true)).uiState.value.isAdminVisible) + } + /** + * The destination gate must RECOVER, not latch. + * + * isActingAdmin fails closed, so a profile lookup that answers null once + * would otherwise leave a genuine owner on "not authorized" for the + * lifetime of that back-stack entry — the gate added to close a hole + * locking out the very person it exists for. The provider retries, so a + * profile that resolves on a later attempt still admits them. + */ + @Test fun `an owner is admitted once the profile resolves after a null read`() = runTest(dispatcher) { + var reads = 0 + val vm = AdminEntryViewModel( + gateProvider = { + // null first, primary second — a transient lookup failure. + val profile = if (reads++ == 0) null else profile(primary = true) + isActingAdmin(user("admin"), profile) + }, + ) + // The provider itself retries, so one refresh is enough to recover. + assertTrue(reads >= 1) + vm.refresh() + assertTrue(vm.uiState.value.isAdminVisible) + } } diff --git a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.kt b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.kt index 22e47e6a4..993b23b29 100644 --- a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.kt +++ b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.kt @@ -139,7 +139,21 @@ class TvSettingsViewModel( val isLastAttempt = attempt == UserLoadMaxAttempts - 1 when (val r = authRepository.getCurrentUser()) { is ApiResult.Success -> { - val profile = profileRepository.getActiveProfile() + // Retried alongside /me. The admin gate fails closed on + // an unresolved profile, and getActiveProfile collapses + // "network failed", "no active id" and "not found" into + // null — so without this a transient failure hid the + // Admin row from a genuine owner for the life of this + // ViewModel. Bounded by the same attempt budget: not + // being an admin is by far the commonest reason for no + // row, and that must not retry forever. + var profile = profileRepository.getActiveProfile() + var profileAttempt = 1 + while (profile == null && profileAttempt < UserLoadMaxAttempts) { + delay(ProfileResolveRetryMs) + profile = profileRepository.getActiveProfile() + profileAttempt += 1 + } _uiState.update { it.copy( user = r.data, @@ -651,6 +665,9 @@ class TvSettingsViewModel( // Retry the user load a few times before surfacing an error, so a // flaky fetch doesn't silently strip the Admin entry from an admin. const val UserLoadMaxAttempts = 3 + + /** Gap between profile lookups while the admin gate is unresolved. */ + const val ProfileResolveRetryMs = 400L const val UserLoadRetryDelayMs = 400L } } diff --git a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvMainShell.kt b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvMainShell.kt index 46e8e5c53..37ed3573b 100644 --- a/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvMainShell.kt +++ b/androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvMainShell.kt @@ -520,10 +520,24 @@ fun TvMainShell( } val user = userResult.data val activeProfile = profileRepository.getActiveProfile() - // Subtitle mirrors tvOS §5.8: role when known, falling back to username. - val subtitle = user?.role?.takeIf { it.isNotBlank() } - ?.replaceFirstChar { it.uppercase() } - ?: user?.username.orEmpty() + // The role belongs to the ACCOUNT, but this header shows a PROFILE's + // name — so rendering it under a non-owner profile reads as "laura is + // an admin" when laura is a household profile on an admin account. + // That is the caption a viewer actually sees and the reason this was + // reported. Show the role only where it is exercisable, which is the + // primary profile. A non-owner profile gets NOTHING here — falling back + // to the account username just captions laura's profile with the + // owner's name, which conflates the two all over again. Profile name + // and server is all a household profile needs to see. + // + // Cosmetic in the sense that no permission hangs on it — the surface + // gate is isActingAdmin below — but it is the part that misleads. + val subtitle = if (activeProfile?.isPrimary == true) { + user?.role?.takeIf { it.isNotBlank() }?.replaceFirstChar { it.uppercase() } + ?: user?.username.orEmpty() + } else { + "" + } val avatarUrl = activeProfile?.avatar ?.takeIf(::isImageAvatar) ?.let { resolveAvatarUrl(activeServerEntry?.url.orEmpty(), it) } diff --git a/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/admin/TvAdminGateTest.kt b/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/admin/TvAdminGateTest.kt index c2dd37467..0c67430d2 100644 --- a/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/admin/TvAdminGateTest.kt +++ b/androidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/admin/TvAdminGateTest.kt @@ -18,5 +18,8 @@ class TvAdminGateTest { @Test fun `admin on primary profile sees admin`() = assertTrue(isActingAdmin(user("admin"), profile(true))) @Test fun `admin on non-primary hidden`() = assertFalse(isActingAdmin(user("admin"), profile(false))) @Test fun `non-admin hidden`() = assertFalse(isActingAdmin(user("user"), profile(true))) - @Test fun `admin without profile visible`() = assertTrue(isActingAdmin(user("admin"), null)) + // Fails closed: an unresolved profile is not permission. This asserts the + // predicate only — that the entry reappears once the profile resolves is a + // property of the CALL SITES retrying, covered where they are tested. + @Test fun `admin without resolved profile hidden`() = assertFalse(isActingAdmin(user("admin"), null)) } diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/model/auth/AdminPermissions.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/model/auth/AdminPermissions.kt index 2b8459be5..3629bfef1 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/model/auth/AdminPermissions.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/model/auth/AdminPermissions.kt @@ -10,10 +10,22 @@ const val ADMIN_ROLE = "admin" * `isActingAdmin(user, profile)`): the account role must be admin AND the * active household profile must be the primary (owner) profile. * - * A null [profile] is treated as "not yet resolved" and does NOT block an - * admin user — the active profile may not be loaded when the gate is first - * evaluated, and every admin route is still gated server-side (defense in - * depth). A null [user] is never acting-admin. + * Fails CLOSED on an unresolved profile. A null [profile] used to be read as + * "not yet loaded" and granted admin to an admin account, on the reasoning that + * the surface is gated server-side anyway. But the account role is the same on + * every profile in the household, so the profile is the ONLY thing separating + * the owner from a child profile — and every path that could not resolve it + * showed the admin surface on profiles that must never see it. A settings load + * that merely failed was enough. + * + * Withholding it is recoverable in a way showing it wrongly is not — but only + * because the call sites retry a profile that has not resolved. They are NOT + * reactive: nothing here observes the profile, so a caller that evaluates this + * once and never asks again will hold a false answer for its own lifetime. Any + * new call site has to retry or observe, or it will hide the surface from a + * genuine owner. + * + * A null [user] is never acting-admin. */ fun isActingAdmin(user: User?, profile: Profile?): Boolean = - user?.role == ADMIN_ROLE && (profile == null || profile.isPrimary) + user?.role == ADMIN_ROLE && profile?.isPrimary == true diff --git a/shared/src/commonTest/kotlin/org/siloserver/silo/model/auth/AdminPermissionsTest.kt b/shared/src/commonTest/kotlin/org/siloserver/silo/model/auth/AdminPermissionsTest.kt index 6007153be..c28e1697b 100644 --- a/shared/src/commonTest/kotlin/org/siloserver/silo/model/auth/AdminPermissionsTest.kt +++ b/shared/src/commonTest/kotlin/org/siloserver/silo/model/auth/AdminPermissionsTest.kt @@ -30,9 +30,16 @@ class AdminPermissionsTest { assertFalse(isActingAdmin(user("admin"), profile(isPrimary = false))) } + /** + * The reported bug: a household profile that is not the owner showed the + * admin surface. The account role is identical on every profile, so the + * profile is the only thing separating them — and treating "not resolved" + * as permission handed admin to whoever was signed in whenever the profile + * lookup had not answered or had failed. + */ @Test - fun `admin role with null profile is acting admin (profile not yet resolved)`() { - assertTrue(isActingAdmin(user("admin"), null)) + fun `admin role with unresolved profile is not acting admin`() { + assertFalse(isActingAdmin(user("admin"), null)) } @Test