fix(rbac+auth): system-group PATCH accepts description; Google sync preserves memberships on empty
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).
This commit is contained in:
ZdenekSrotyr
parent
5c54320f75
commit
0c963b55ef
3 changed files with 42 additions and 26 deletions
|
|
@ -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 `<link>` 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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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 "<none>",
|
||||
)
|
||||
# `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
|
||||
|
|
|
|||
Loading…
Reference in a new issue