fix(admin): hide administration without cached permission evidence - #487
Conversation
Obiente previewNC Native · Obiente updates this comment as the preview changes. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4eb5190863
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) { mutableStateOf(NextcloudDestination.Home) } | ||
| val administration = rememberAdministrationAccess( | ||
| session, platformCapabilityRefreshRequest, | ||
| active = destination == NextcloudDestination.Settings || screen == Screen.AdminApps, |
There was a problem hiding this comment.
Restrict the Settings case to screen == Screen.Root && destination == NextcloudDestination.Settings. Opening Offline Center, Transfers, Project News, or an installed app from Server apps changes screen but leaves destination set to Settings, so this expression keeps the five-minute ForceNetwork catalog loop active across unrelated screens. Long browsing or transfer sessions will therefore repeatedly download the administrator catalog despite neither Settings nor Server apps being visible, wasting bandwidth and battery.
AGENTS.md reference: AGENTS.md:L256-L258
Useful? React with 👍 / 👎.
| if (access.state.checking) { | ||
| LoadingMessage("Checking administrator access...") | ||
| } else { | ||
| LaunchedEffect(access.state) { onBack() } |
There was a problem hiding this comment.
When an already-open Server apps screen reaches the five-minute expiry, the repository clears the old result while checking and then publishes Forbidden, Unavailable, or InvalidResponse if revalidation fails. This effect immediately calls onBack() for every non-checking, non-admin result, so the result-specific error and retry branches below can never render; permission loss, an offline failure, or a malformed response silently ejects the user to Settings without explaining what happened or what is safe to do next. Keep the catalog fail-closed, but make the typed failure state visible before requiring navigation away.
AGENTS.md reference: AGENTS.md:L447-L449
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9b3816d40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while (active && isActive) { | ||
| repository.refresh { request -> currentExecute(request) } | ||
| val age = repository.state.value.checkedAt?.elapsedNow() ?: AdministrationAccessTtl | ||
| delay((AdministrationAccessTtl - age).coerceAtLeast(kotlin.time.Duration.ZERO)) |
There was a problem hiding this comment.
On Android when the activity is stopped, and on desktop when the window is minimized or hidden, the composition and its coroutine can remain active; because active reflects only the current route, leaving the app on Settings or Server apps continues fetching the administrator catalog every five minutes in the background. Include application/window visibility in the polling condition, or move lifecycle-aware scheduling outside this composable, so a screen that is no longer actually visible does not consume network and battery.
AGENTS.md reference: AGENTS.md:L256-L258
Useful? React with 👍 / 👎.
| isDesktop = isDesktop, | ||
| hasDeviceSettings = platformCapabilities.isNotEmpty(), | ||
| hasDesktopAppSettings = hasDesktopAppSettings, | ||
| canAdminister = canAdminister, |
There was a problem hiding this comment.
On a two-pane Settings layout, every five-minute revalidation temporarily makes canAdminister false because the repository clears its prior result while checking. Filtering Administration here then resolves the saved selection to Account, and the existing LaunchedEffect at lines 268-271 writes that fallback into selectedSectionName; after a successful check, the administrator is therefore left on Account instead of returning to the section they were using. The same overwrite loses a restored Administration selection during the initial post-recreation check, so retain the requested selection while permission is merely unknown/checking.
AGENTS.md reference: AGENTS.md:L457-L459
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
Outcome
Regular users no longer see Settings > Administration, Server apps, or its installed-workspace summary. Restored admin routes and action callbacks never expose the catalog or mutation controls without current permission evidence. The ordinary Apps workspace remains available.
Automatic checks run only while the root Settings screen or Server apps is visible and its platform window is visible. Android stop and desktop hide/minimize cancel polling, including an in-flight check. The requested Settings section survives permission revalidation and restoration while admin controls remain hidden. Failed revalidation keeps admin controls hidden and shows a typed recovery state with retry and Back.
A session-scoped repository caches allowed and denied catalog results for five minutes, reuses the verified catalog when opening Server apps, and retires its state on account changes. Revalidation bypasses persisted transport responses. Missing or malformed OCS success metadata cannot grant access.
Advances #187.
Verification
bash tools/check-repository.shpasses in fullPassed with JDK 21 on Windows:
:ui:compileKotlinDesktop,:ui:compileDebugKotlinAndroid, and:androidApp:compileDebugKotlin.:ui:createDistributableand:androidApp:assembleDebug.bash tools/check-kotlin-architecture.shandgit diff --check.The full repository check could not finish locally: its Linux package metadata test requires
dpkg-deb, and the available WSL environment could not start. Release-promotion and desktop-manifest checks were verified separately after adapting the local Windows jq invocation.Compatibility and risk
Uses the existing app-store OCS catalog and provisioning inventory fallback. Tests use synthetic responses; no live Nextcloud server or installed-app version was validated. The cache is in memory and is cleared on session replacement or process restart. A permission change may take up to the five-minute validity window to affect visibility. Mutations still require the existing authenticated browser handoff and server authorization.
Visual changes
Administration is omitted on compact and desktop Settings layouts without permission. Visibility and restoration policies are covered by deterministic tests. Rendered Compose tests exercise denied, unavailable, and malformed-response recovery at 390x844 and 1280x800, including retry and Back. Android emulator visual and lifecycle validation has not been performed. Platform visibility cancellation and Settings selection restoration are covered by deterministic Compose tests.