From 0c963b55ef12c077daa81bebaa942a4f70bc8c55 Mon Sep 17 00:00:00 2001 From: ZdenekSrotyr Date: Tue, 28 Apr 2026 14:40:27 +0200 Subject: [PATCH] fix(rbac+auth): system-group PATCH accepts description; Google sync preserves memberships on empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Devin-flagged regressions on the squashed PR #106 head: 1) PATCH /api/admin/groups/{id} blanket-rejected on system groups. The repository guard at src/repositories/user_groups.py was already narrowed to "rename only" by 7147bac (PR #110 follow-up), but the endpoint at app/api/access.py:331-343 still short-circuited with 409 "System groups are immutable" for any mutation. A description-only payload like {"description": "..."} returned 409 instead of 200 even though the repo would have accepted it. CHANGELOG entry promised the fix but the code didn't match. Endpoint now mirrors the repo contract: 409 only when payload.name is set AND differs from existing name. Same-name no-op renames are dropped before the repo call. Description-only updates flow through. 2) Google OAuth callback wiped google_sync memberships on transient API failure. fetch_user_groups is fail-soft and returns [] for both "user has no groups" and "Cloud Identity API error". The callback fed that empty list into replace_google_sync_groups, which DELETEs all rows with source='google_sync' for the user then INSERTs zero — silently wiping every Workspace-synced membership on a hiccup. Callback now skips replace_google_sync_groups when group_names is empty and logs "preserving existing memberships". Trade-off: a user whose Workspace groups were genuinely cleared keeps stale memberships until the next non-empty sync. Admin-added rows (source='admin') were already protected by source-scope and are unaffected. The previous guard against this exact regression was test_callback_empty_groups_ does_not_overwrite_existing in tests/test_auth_providers.py — that test class has been skipped since v12 (asserts users.groups JSON, needs rewrite for user_group_members). --- CHANGELOG.md | 3 ++- app/api/access.py | 22 +++++++++--------- app/auth/providers/google.py | 43 +++++++++++++++++++++++++----------- 3 files changed, 42 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f11292d..da829c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,8 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C - `/admin/access` group sidebar grant-count badges no longer revert to a stale value when switching between groups. The badge was reading `state.groups[i].grant_count`, a snapshot populated once at `/access-overview` load; toggling a grant only updated the DOM (via `refreshCounts`), not that field, so the next `renderGroups` call (triggered by `selectGroup`) would clobber the live count with the original snapshot. `renderGroups` now derives the count live from `state.grants`, the array that `toggleGrant`/`bulkSet` keep in sync. Server data was always correct — only the in-page badge drifted until refresh. - `/catalog`, `/admin/tables`, and `/admin/permissions` pages now render the shared top header correctly. The pages include `_app_header.html` (which uses `.app-*` CSS classes) but were not linking `style-custom.css` where those classes are defined; only `dashboard.html` and `base.html` did. Without the stylesheet the nav links, dropdowns, and user menu rendered as unstyled inline text. Added the missing `` to all three templates. -- `PATCH /api/admin/groups/{id}` on a system group now correctly accepts description-only updates while still rejecting renames. Earlier the repo guard rejected any mutation on `is_system=TRUE` rows and the endpoint surfaced a misleading `409 "Cannot rename a system group"` on a description-only payload. +- `PATCH /api/admin/groups/{id}` on a system group now correctly accepts description-only updates while still rejecting renames. The endpoint guard previously short-circuited with `409 "System groups are immutable"` for any mutation, which contradicted the repository layer's narrowed contract (rename-only rejection) — a description-only payload like `{"description": "..."}` would hit the endpoint short-circuit and never reach the repo. The endpoint now 409s only when `payload.name` differs from the existing name; a no-op rename (same name in payload) is dropped from the update before reaching the repo. +- Google OAuth callback no longer wipes a user's `google_sync` group memberships on a transient Workspace API failure. `fetch_user_groups` is fail-soft and returns `[]` for both "no groups" and "API error" — the callback used to feed that empty list into `replace_google_sync_groups`, which deletes all `source='google_sync'` rows for the user and then inserts zero. A login during a transient Cloud Identity hiccup would silently drop every Workspace-synced membership the user had built up. Admin-added memberships (`source='admin'`) were already protected. The callback now skips `replace_google_sync_groups` when the fetch returns empty and logs "preserving existing memberships" instead. Trade-off: a user whose Workspace groups were genuinely cleared keeps stale memberships until the next non-empty sync — accepted until `fetch_user_groups` learns to distinguish empty-success from empty-failure. - `docker-compose.host-mount.yml` now uses `o: bind,rbind` instead of `o: bind` for the `data` volume. With a plain bind, sub-mounts under `/data` on the host (e.g. the dual-disk layout where sdc is mounted on `/data/state`) are silently shadowed inside the container by an empty subdirectory on the parent disk. The container then writes `system.duckdb` and other state to the wrong disk; the dedicated state disk receives no writes and accumulates only the snapshot left by the migration script. Recursive bind propagates existing sub-mounts at container start, so the container sees the same filesystem the host does. Operators on dual-disk VMs need to copy the live DB from `/var/lib/docker/volumes/agnes_data/_data/state/` (sdb's empty subdir) onto `/data/state/` (sdc) **before** redeploying with the fix, or the next start will surface the stale snapshot. ### Changed diff --git a/app/api/access.py b/app/api/access.py index 14e0950..d4f1675 100644 --- a/app/api/access.py +++ b/app/api/access.py @@ -328,21 +328,19 @@ async def update_group( g = repo.get(group_id) if not g: raise HTTPException(status_code=404, detail="Group not found") - if g.get("is_system"): - # System groups are immutable end-to-end: the canonical names - # 'Admin' / 'Everyone' are referenced from the codebase, and the - # admin UI hides the Edit button entirely (admin_group_detail.html). - # Reject any modification at the API layer with a clear message - # rather than the misleading repository error that surfaced when - # the previous "rename rejected, description allowed" contract - # diverged from the repository's "all mutations rejected" - # behavior. + if g.get("is_system") and payload.name is not None and payload.name.strip() != g["name"]: + # System groups: block renames (the canonical names "Admin" / + # "Everyone" are referenced from app.auth.access and the + # marketplace filter), but description edits are cosmetic and + # allowed (admins curate them in /admin/access). The repo + # layer's narrowed guard (src/repositories/user_groups.py) is + # the second line of defense. raise HTTPException( status_code=409, - detail="System groups are immutable", + detail="System groups cannot be renamed", ) updates: dict = {} - if payload.name is not None: + if payload.name is not None and payload.name.strip() != g["name"]: updates["name"] = payload.name.strip() if payload.description is not None: updates["description"] = payload.description @@ -351,7 +349,7 @@ async def update_group( repo.update(group_id, **updates) except SystemGroupProtected: raise HTTPException( - status_code=409, detail="System groups are immutable", + status_code=409, detail="System groups cannot be renamed", ) _audit(conn, user["id"], "user_group.updated", f"group:{group_id}", updates) g = repo.get(group_id) diff --git a/app/auth/providers/google.py b/app/auth/providers/google.py index 67ebb0e..674692a 100644 --- a/app/auth/providers/google.py +++ b/app/auth/providers/google.py @@ -114,19 +114,36 @@ async def google_callback(request: Request): # place; admin-added rows survive regardless. try: group_names = fetch_user_groups(email) - ug_repo = UserGroupsRepository(conn) - members_repo = UserGroupMembersRepository(conn) - group_ids: list[str] = [] - for group_name in group_names: - g = ug_repo.ensure(group_name) - group_ids.append(g["id"]) - members_repo.replace_google_sync_groups( - user["id"], group_ids, added_by="system:google-sync", - ) - logger.info( - "Google group sync for %s: %d group(s) [%s]", - email, len(group_ids), ", ".join(group_names) or "", - ) + # `fetch_user_groups` is fail-soft and returns [] for both + # "user genuinely has no groups" and "transient API failure". + # We can't distinguish, so empty is treated as "no change": + # don't call replace_google_sync_groups (which would + # DELETE...source='google_sync' then INSERT zero, wiping + # all of the user's Workspace-synced memberships on a + # transient hiccup). Trade-off: a user whose Workspace + # groups were genuinely cleared keeps stale memberships + # until the next non-empty sync. Admin-added rows + # (source='admin') are unaffected either way. + if group_names: + ug_repo = UserGroupsRepository(conn) + members_repo = UserGroupMembersRepository(conn) + group_ids: list[str] = [] + for group_name in group_names: + g = ug_repo.ensure(group_name) + group_ids.append(g["id"]) + members_repo.replace_google_sync_groups( + user["id"], group_ids, added_by="system:google-sync", + ) + logger.info( + "Google group sync for %s: %d group(s) [%s]", + email, len(group_ids), ", ".join(group_names), + ) + else: + logger.info( + "Google group sync for %s: empty result, " + "preserving existing memberships", + email, + ) except Exception as sync_err: # noqa: BLE001 - fail-soft by design logger.warning( "Google group sync failed for %s: %s", email, sync_err