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
9 changes: 9 additions & 0 deletions docs/changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -4367,3 +4367,12 @@ users(id)`, so the decider handler passing the decider's email violated the
and the FIPS host image's `cryptography` relink builds from a
sha256-pinned GitHub tag tarball when PyPI publishes no source release
for the locked version.

- **Login and `/admin` navigation wait for permission data (#3540).**
The router's admin gates now hold their decision while the session's
permission data is still loading: a login redirect or a direct
`/admin` navigation that arrives before `/my-permissions` answers
stays on its page until the fetch settles, then routes with live
permissions. Previously such a redirect tested the admin gate against
empty permissions and landed the user on the workspaces page instead;
the requested admin page was never re-attempted.
10 changes: 8 additions & 2 deletions src/frontend/e2e-tests/fmtk/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,15 @@ def at_login(harness, app) -> None:

def open_admin_users(app) -> None:
"""Route to the admin page and wait for the Users tab's toolbar
(the 'Handle' sort chip is unique to it)."""
(the 'Handle' sort chip is unique to it). A first navigation can
lose the race with the login redirect chain (#3540) — one retry
re-runs the hash route and absorbs the loss."""
app.navigate("/admin/users")
app.wait_for_text("Handle")
try:
app.wait_for_text("Handle")
except FmtkError:
app.navigate("/admin/users")
app.wait_for_text("Handle")


def admin_login(harness, app) -> None:
Expand Down
5 changes: 5 additions & 0 deletions src/frontend/lib/admin/admin_users_page.dart
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,11 @@ class _AdminUsersPageState extends State<AdminUsersPage> {
Widget build(BuildContext context) {
// Re-resolve on each build: AuthService loads permissions asynchronously,
// so the first build (before they arrive) would otherwise show no tabs.
// #3540: the router's admin gate can hold a login on this page while
// /my-permissions is still in flight — the page then mounts with empty
// permissions, and the fetch's settle notification must rebuild it.
// Watching the auth service provides exactly that rebuild.
context.watch<AuthService>();
_resolvePermissions();
final (tabs, views) = _buildTabsAndViews();

Expand Down
1 change: 1 addition & 0 deletions src/frontend/lib/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ class _KlangkAppState extends State<KlangkApp> {
publicRoutes: routes,
featurePaths: featurePaths,
canAccessAdmin: auth.canAdminSection,
permissionsLoaded: auth.permissionsLoaded,
);
},
routes: [
Expand Down
38 changes: 36 additions & 2 deletions src/frontend/lib/app_guards.dart
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,18 @@ String? guardAuth({
/// Feature routes are excluded: they are public but a logged-in user may
/// legitimately navigate to them.
///
/// While [permissionsLoaded] is false — the session's token just landed
/// and `/my-permissions` is still in flight — the gate HOLDS (returns
/// null) instead of bouncing (#3540). Deciding the target at that
/// instant tests `canAdminSection` against empty permissions, so an
/// admin's stashed `/admin`-prefixed target would take the
/// non-admin fallback to `/workspaces`, and once permissions arrive
/// nothing re-attempts the stash — the user is parked there. The
/// permission fetch's completion fires the notification that re-parses
/// this same location, and the gate then decides with live data. A hold
/// is safe: the gate holds on whatever public page the user is on for
/// the fraction of a second the fetch takes.
///
/// The target is permission-checked against the *current* session: an
/// `/admin`-prefixed target (e.g. stashed by an admin's logout or expiry,
/// then inherited by whoever logs in next on this browser) falls back to
Expand All @@ -115,12 +127,14 @@ String? guardAuth({
/// Returns the redirect target, or null to allow.
String? guardLoggedInPublicRoute({
required bool isLoggedIn,
required bool permissionsLoaded,
required String loc,
required Set<String> publicRoutes,
required Set<String> featurePaths,
required bool canAccessAdmin,
}) {
if (isLoggedIn && publicRoutes.contains(loc) && !featurePaths.contains(loc)) {
if (!permissionsLoaded) return null;
final target = pendingRedirect;
if (target == null) return '/workspaces';
if (target.startsWith('/admin') && !canAccessAdmin) {
Expand All @@ -139,6 +153,21 @@ String? guardLoggedInPublicRoute({
/// redirect even though the app-bar admin icon is gated, and its page is
/// a dead end ("No admin sections available") for them.
///
/// While [permissionsLoaded] is false — the session's token just landed
/// and `/my-permissions` is still in flight — the gate HOLDS (returns
/// null) instead of bouncing (#3540): `canAccessAdmin` reads empty
/// permissions at that instant, so a mid-login navigation to a stashed
/// or typed `/admin` URL would bounce an admin to `/workspaces`, and
/// once the fetch completes nothing re-attempts the route — the user
/// stays parked there. The permission fetch's completion fires the
/// notification that re-parses this same location: an admin stays put,
/// a non-admin is bounced then. The transient hold leaves a non-admin
/// on the admin page for the fetch's duration — its data fetches are
/// refused, and the settle bounce lands them on `/workspaces` — the
/// cost of not parking admins permanently. The fetch itself has no
/// client-side timeout: a hung request holds the page (and the login
/// loading state) until the connection gives up.
///
/// Fires only for *logged-in* users: a logged-out visitor must keep the
/// `guardAuth` flow (stash + `/login`), and `guardLoggedInPublicRoute`
/// already rejects `/admin`-prefixed stashes for non-admins on login
Expand All @@ -151,11 +180,13 @@ String? guardLoggedInPublicRoute({
/// every refreshListenable notification) all agree.
String? guardAdminRoute({
required bool isLoggedIn,
required bool permissionsLoaded,
required bool canAccessAdmin,
required String loc,
}) {
if (isLoggedIn && !canAccessAdmin && loc.startsWith('/admin')) {
return '/workspaces';
if (isLoggedIn && loc.startsWith('/admin')) {
if (!permissionsLoaded) return null;
if (!canAccessAdmin) return '/workspaces';
}
return null;
}
Expand Down Expand Up @@ -206,6 +237,7 @@ String? evaluateGuards({
required Set<String> publicRoutes,
required Set<String> featurePaths,
required bool canAccessAdmin,
required bool permissionsLoaded,
}) {
if (bannerRequired) {
return guardBanner(bannerRequired: true, isLoggedIn: isLoggedIn, loc: loc);
Expand All @@ -224,13 +256,15 @@ String? evaluateGuards({
) ??
guardLoggedInPublicRoute(
isLoggedIn: isLoggedIn,
permissionsLoaded: permissionsLoaded,
loc: loc,
publicRoutes: publicRoutes,
featurePaths: featurePaths,
canAccessAdmin: canAccessAdmin,
) ??
guardAdminRoute(
isLoggedIn: isLoggedIn,
permissionsLoaded: permissionsLoaded,
canAccessAdmin: canAccessAdmin,
loc: loc,
) ??
Expand Down
25 changes: 25 additions & 0 deletions src/frontend/lib/auth/auth_service.dart
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,23 @@ class AuthService extends ChangeNotifier {
List<Map<String, dynamic>> _groups = [];
bool _isAdmin = false;

/// True once this session's permission fetch has settled (a 200, a
/// refusal, or a network error — anything but still-in-flight or a
/// cleared session). The login-page redirect guard holds while it is
/// false: a mid-login re-parse would otherwise test `canAdminSection`
/// against empty permissions and bounce an admin's stashed
/// `/admin`-prefixed target to `/workspaces` (#3540).
bool _permissionsLoaded = false;

Map<String, List<String>> get permissions => _permissions;
List<Map<String, dynamic>> get groups => _groups;

/// Whether a permission fetch has settled for the CURRENT token —
/// during a token refresh the flag is false while [permissions] still
/// holds the previous fetch's data. Read by the router's redirect
/// guards (see [_permissionsLoaded]).
bool get permissionsLoaded => _permissionsLoaded;

/// Instance-admin status: the explicit `is_admin` flag from
/// /my-permissions, derived server-side from `admins`-group
/// membership (#2995) — not an ACL permission. The old `/admin`
Expand Down Expand Up @@ -418,11 +432,17 @@ class AuthService extends ChangeNotifier {
_isAdmin = data['is_admin'] == true;
} else if (resp.statusCode == 401) {
await _clearToken(preserveRedirect: true);
return;
}
} catch (e) {
// coverage:ignore-start
debugPrint('[AuthService] fetch permissions failed: $e');
} // coverage:ignore-end
// Every settled outcome except the 401 (which cleared the session
// and with it this flag) marks the fetch resolved: a failed or
// refused fetch leaves empty permissions, and the guards must not
// hold the login page forever on its account (#3540).
_permissionsLoaded = true;
}

/// Refresh permissions from the server (call after group changes).
Expand Down Expand Up @@ -454,6 +474,10 @@ class AuthService extends ChangeNotifier {

Future<void> _saveToken(String token) async {
_token = token;
// A fresh token starts a permission-fetch cycle; until it settles,
// the guards hold instead of bouncing (see _permissionsLoaded,
// #3540).
_permissionsLoaded = false;
await writeToken(token);
// Bind the fresh token to the browser's non-extractable key right
// away (#3218): the unbound token exists in JS-readable form only
Expand Down Expand Up @@ -527,6 +551,7 @@ class AuthService extends ChangeNotifier {
_refreshTimer = null;
_token = null;
_permissions = {};
_permissionsLoaded = false;
_groups = [];
_isAdmin = false;
_mustChangePassword = false;
Expand Down
50 changes: 50 additions & 0 deletions src/frontend/test/admin_users_page_test.dart
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import 'dart:async';
import 'dart:convert';

import 'package:flutter/material.dart';
Expand Down Expand Up @@ -272,6 +273,55 @@ void main() {
}

group('AdminUsersPage', () {
testWidgets(
'mounts mid-permission-fetch and rebuilds when they settle (#3540)',
(tester) async {
// The router's admin gate holds a session on this page while the
// permission fetch is in flight — the page then builds with empty
// permissions, and AuthService's settle notification must rebuild
// it (the page re-resolves permissions in build, so it needs the
// auth dependency to receive that rebuild).
final permissions = Completer<http.Response>();
testAuthHttpClientOverride = MockClient((request) async {
if (request.url.path.contains('/api/v1/my-permissions')) {
return permissions.future;
}
if (request.url.path.contains('/api/v1/users') &&
request.method == 'GET') {
return http.Response(_usersEnvelope([_user('a@example.com')]), 200);
}
return http.Response('{}', 200);
});

await tester.binding.setSurfaceSize(const Size(1280, 900));
await tester.pumpWidget(buildPage());
await tester.pump();
await tester.pump(const Duration(milliseconds: 100));

// Mounted while the boot fetch is pending: permission-gated tabs
// are absent.
expect(find.text('No admin sections available'), findsOneWidget);
expect(find.text('Handle'), findsNothing);

permissions.complete(http.Response(
jsonEncode({
'user_id': 'admin-user',
'email': 'admin@example.com',
'is_admin': true,
'permissions': {
'/users': ['manage-users'],
},
'groups': <Map<String, dynamic>>[],
}),
200,
));
await tester.pumpAndSettle();

expect(find.text('No admin sections available'), findsNothing);
expect(find.text('Handle'), findsOneWidget);
},
);

testWidgets('renders users from the paged envelope', (tester) async {
serveUsers(
(page, pageSize, sort, order, q) => [
Expand Down
Loading
Loading