Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
},
)
Expand All @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down
Original file line number Diff line number Diff line change
@@ -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(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Retry unresolved profiles in the destination gate.

AdminRouteGate uses the default AdminEntryViewModel, whose production gate reads the active profile once. If that lookup returns null, refresh() completes and this route remains on NotAuthorized() for the lifetime of its back-stack entry.

This breaks the recovery requirement documented by isActingAdmin. A primary-profile owner who reaches a restored or direct route during a transient profile failure cannot regain access after the profile resolves.

Add bounded profile retry or profile observation to AdminEntryViewModel. Add a test where profile resolution changes from unresolved to primary while the destination remains active.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminRouteGate.kt`
at line 25, Update AdminEntryViewModel, used by AdminRouteGate, so unresolved
active-profile lookups are retried with a bounded strategy or observed until
resolution, allowing NotAuthorized state to recover to authorized while the
destination remains active. Preserve existing authorization behavior once a
profile resolves, and add a test covering a transition from unresolved to a
primary profile while the route remains active.

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()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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)))
}
Expand Down Expand Up @@ -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
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down