fix(admin, tv): stop a household profile being labelled and treated as admin - #179
Merged
RXWatcher merged 5 commits intoAug 6, 2026
Merged
Conversation
Reported from a device: a household profile that is not the owner showed
the admin surface.
isActingAdmin read
user?.role == ADMIN_ROLE && (profile == null || profile.isPrimary)
so an UNRESOLVED profile granted admin. The account role is identical on
every profile in the household, which makes the profile the only thing
separating the owner from a child — and every path that could not resolve
it therefore revealed admin. Settings passes null explicitly on a failed
profile lookup, so a load that merely errored was enough. The gate now
requires the primary profile.
Failing closed introduces the opposite risk, and the call sites did not
survive it as first written: three of them evaluate once and hold, so a
transient failure would have hidden the surface from a genuine owner for
the life of the ViewModel — worse than the bug being fixed. Both settings
ViewModels now retry an unresolved profile, bounded, because the ordinary
reason for no admin row is simply not being an admin and an unbounded
retry would hammer the API for every user. An earlier attempt at this
retried in the screen on "admin not visible", which for a non-admin is
always true — an infinite request loop. It is bounded in the ViewModel
instead.
Gating the entry was also not enough. The phone admin route stayed
registered and its screen calls the API as it composes, so restored or
direct navigation reached it regardless of the menu row. AdminRouteGate
re-evaluates at the destination and refuses.
This does NOT make the client a security boundary. Admin calls are not
separately authorised here, and this repository cannot show what the
server enforces: if the server requires role AND primary profile the
incident was UI exposure, and if it authorises on role alone it was not.
That is worth establishing server-side rather than assuming, and the
comments no longer assert it.
Also fixes a test helper that ignored its arguments and always returned
true, so every test through it passed regardless of the gate — including
the case this class exists for.
Reviewed by Codex, which caught the latching, the ungated destinations,
and the overclaiming comments.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
📝 WalkthroughWalkthroughAdmin authorization now fails closed for unresolved or non-primary profiles. Android and TV settings retry profile resolution, and the Android admin route gates screen composition by authorization state. ChangesAdmin authorization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AdminRouteGate
participant AdminEntryViewModel
participant AdminStatsScreen
participant NotAuthorized
AdminRouteGate->>AdminEntryViewModel: collect uiState
AdminEntryViewModel-->>AdminRouteGate: authorization state
alt loading
AdminRouteGate-->>AdminRouteGate: render loading indicator
else authorized
AdminRouteGate->>AdminStatsScreen: render admin screen
else unauthorized
AdminRouteGate->>NotAuthorized: render unauthorized screen
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@coderabbitai review |
|
The header shows a PROFILE's name with the ACCOUNT's role beneath it, so a household profile on an admin account read as "laura — ADMIN". That is the caption the viewer actually sees, and it is what was reported from a device. The role is now shown only on the primary profile, where it can actually be exercised. Everyone else gets the account username, which is true of them without implying powers they do not have. No permission hangs on this label — the surface gate is isActingAdmin, and the server independently refuses admin work from a non-primary profile. But the earlier work in this branch gated the Admin entry and destinations without touching the one thing on screen that said ADMIN, so the reported symptom would have survived it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.kt (1)
641-642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse camelCase names for Kotlin properties.
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.kt#L641-L642: RenamePROFILE_RESOLVE_ATTEMPTSandPROFILE_RESOLVE_RETRY_MSand update their references.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.kt#L669-L670: RenameProfileResolveRetryMsand update its references.As per coding guidelines,
**/*.{kt,kts}requirescamelCasefor Kotlin functions and properties.🤖 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/settings/SettingsViewModel.kt` around lines 641 - 642, Rename PROFILE_RESOLVE_ATTEMPTS and PROFILE_RESOLVE_RETRY_MS to camelCase property names in SettingsViewModel.kt and update every reference. Also rename ProfileResolveRetryMs to camelCase in TvSettingsViewModel.kt and update all of its references.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminRouteGate.kt`:
- 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.
---
Nitpick comments:
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.kt`:
- Around line 641-642: Rename PROFILE_RESOLVE_ATTEMPTS and
PROFILE_RESOLVE_RETRY_MS to camelCase property names in SettingsViewModel.kt and
update every reference. Also rename ProfileResolveRetryMs to camelCase in
TvSettingsViewModel.kt and update all of its references.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ecfe13a7-b679-45e6-8ddd-65de052a7481
📒 Files selected for processing (10)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/navigation/AppNavigation.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminHubScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminRouteGate.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModelTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvMainShell.ktandroidTvApp/src/androidUnitTest/kotlin/org/siloserver/silo/tv/ui/screens/admin/TvAdminGateTest.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/auth/AdminPermissions.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/auth/AdminPermissionsTest.kt
| */ | ||
| @Composable | ||
| fun AdminRouteGate( | ||
| viewModel: AdminEntryViewModel = koinViewModel(), |
There was a problem hiding this comment.
🎯 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.
Falling back to the account username captioned laura's profile with the owner's name — conflating profile and account exactly as the ADMIN label had. A non-owner profile now shows its name and the server, and nothing about whose account it belongs to or what that account can do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
RXWatcher
changed the title
AdminEntryViewModel read the active profile once. Since isActingAdmin fails closed, a single null read left AdminRouteGate showing "not authorized" for the lifetime of that back-stack entry, with no way back once the profile resolved — so the gate added to close a hole could lock out the owner it exists for. On a restored or directly-navigated route that is the whole session. Bounded retry, matching the two settings ViewModels. getActiveProfile collapses "network failed", "no active id" and "not found" into null, so retrying 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 lands here. I had documented this requirement on isActingAdmin, fixed the two settings call sites, and then built a new gate on the third without applying it. CodeRabbit quoted the KDoc back at me. Test covers unresolved -> primary while the destination is still active. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Collaborator
Author
|
Fixed — and a fair catch, since you quoted my own KDoc back at me. I documented on
Test added for unresolved → primary while the destination is still active, as suggested. Full suite green on all four modules. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModel.kt (1)
49-55: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip profile retries for non-admin accounts.
isActingAdmincannot returntruefor a null or non-admin user. The current code can still perform two 400 ms delays before returningfalsewhen the profile is unresolved. Gate the retry block on the account role, then keep the finalisActingAdmin(user, profile)check.🤖 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/AdminEntryViewModel.kt` around lines 49 - 55, Update the profile retry logic in the surrounding ViewModel flow to execute only when the account is already identified as an admin; skip all retry delays for null or non-admin users. Preserve the final isActingAdmin(user, profile) check after the conditional retry block.androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModelTest.kt (1)
69-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the recovery test execute the bounded retry.
The injected
gateProviderreturnsnullduring the initial refresh and a primary profile only after the explicit secondrefresh(). It does not execute the repository-backed retry loop. The test therefore passes if the new retry loop is removed. Construct the ViewModel with fake repositories that returnnulland then a primary profile during one gate evaluation. Use one refresh and assert the exact read count and final visibility.🤖 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/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModelTest.kt` around lines 69 - 91, Update the test `an owner is admitted once the profile resolves after a null read` to use fake repositories that return null on the first read and a primary profile on the bounded retry within a single gate evaluation, rather than injecting a self-retrying `gateProvider`. Construct `AdminEntryViewModel` with those fakes, perform one `refresh()`, assert the exact repository read count, and verify `uiState.value.isAdminVisible` is true.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In
`@androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModel.kt`:
- Around line 49-55: Update the profile retry logic in the surrounding ViewModel
flow to execute only when the account is already identified as an admin; skip
all retry delays for null or non-admin users. Preserve the final
isActingAdmin(user, profile) check after the conditional retry block.
In
`@androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModelTest.kt`:
- Around line 69-91: Update the test `an owner is admitted once the profile
resolves after a null read` to use fake repositories that return null on the
first read and a primary profile on the bounded retry within a single gate
evaluation, rather than injecting a self-retrying `gateProvider`. Construct
`AdminEntryViewModel` with those fakes, perform one `refresh()`, assert the
exact repository read count, and verify `uiState.value.isAdminVisible` is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fd7f5d00-dfc0-4bd5-990d-2b0a5beea73a
📒 Files selected for processing (3)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModel.ktandroidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/ui/screens/admin/AdminEntryViewModelTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/shell/TvMainShell.kt
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported from a device: a household profile that is not the owner was marked as an admin.
That turned out to be two separate things, and the visible one was not where I first looked.
1. The label — what was actually on screen
The TV profile menu shows a profile's name with the account's role beneath it, so a household profile on an admin account rendered as
laura · ADMIN. It never consulted the admin gate at all:Technically true — it is an admin account — but it reads as "laura is an admin", which is the thing that was reported.
The role now shows only on the primary profile, where it can actually be exercised. A non-owner profile shows its name and the server and nothing else: falling back to the account username just captioned laura's profile with the owner's name, conflating profile and account all over again.
No permission hangs on this label. It is the part that misleads.
2. The gate — what was actually exploitable
isActingAdminreadso an unresolved profile granted admin. The account role is identical on every profile, which makes the profile the only thing separating owner from child — and Settings passes
nullexplicitly when the profile lookup fails, so a load that merely errored revealed admin. The gate now requires the primary profile.Failing closed had to be done carefully. Three call sites evaluate once and hold, so failing closed as first written would have hidden Admin from a genuine owner for the life of the ViewModel — worse than the bug. Both settings ViewModels now retry an unresolved profile, bounded: not being an admin is by far the commonest reason for no admin row, and an earlier attempt that retried on "admin not visible" would have looped on the API forever for every ordinary user.
Gating the entry was not enough. The phone admin route stays registered and its screen calls the API as it composes, so restored or direct navigation reached it regardless of the menu row.
AdminRouteGatere-evaluates at the destination and refuses, rendering nothing while the gate resolves.Security impact
The server does enforce this.
RequireActingAdminchecks the admin role and that the declared profile is the account's primary, and fails closed on a profile it cannot resolve — explicitly so a non-primary session cannot regain admin with a bogusX-Profile-Id.So the reported incident was UI exposure, not privilege escalation.
One narrow exception, worth recording: the server allows admin on role alone when no profile is declared, and the client omits
X-Profile-Idwhen the profile id is null — the same unresolved-profile condition. In that window a non-primary profile on an admin account could have reached admin data. This PR closes it client-side at both the entry and the destination.The server is also candid that this is a policy boundary for well-behaved clients, not isolation between people sharing an account.
Also
A test helper ignored both its arguments and always returned
true, so every test through it passed regardless of the gate — including the case the class exists for. Fixed, with coverage for an admin account on a non-primary profile, on an unresolved profile, and on the primary profile.Testing
Full suite green on all four modules. Verified on a Google TV Streamer with a release build: the profile menu now reads
laura · WAVE-NINJAinstead oflaura · ADMIN · WAVE-NINJA.Reviewed by Codex, which caught the latching, the ungated destinations and the overclaiming comments. The label fix came out of looking at the actual screen — no amount of gate review would have found it, because the label never touched the gate.
🤖 Generated with Claude Code
https://claude.ai/code/session_0115uaQ6FTQZK8KYvazjefaW
Summary by CodeRabbit
Bug Fixes
Tests