feat(auth): group-membership-only authorization, remove roles, require >=1 group (closes #907) - #912
Merged
Conversation
…e >=1 group (closes #907) Collapse the dual authorization model (role + groups) down to group membership only. Every authorization decision now derives from the union of the user's groups' permissions; the users.role and sessions.role columns are removed. Role -> group mapping (migration 000057): - admin -> Administrators (00000000-0000-5000-8000-000000000001) - user -> Standard Users (00000000-0000-5000-8000-000000000005) - readonly -> Read-Only Users (00000000-0000-5000-8000-000000000006) Migration 000057 seeds the two role-mirror groups, backfills group_ids from role for any zero-group user, nets any still-zero-group user into Read-Only Users (fail-safe), then enforces group_ids NOT NULL + cardinality(group_ids) >= 1 via a CHECK constraint before dropping the role columns last. The DB can no longer represent a zero-group user. How the role short-circuits were replaced: - session.Role == "admin" checks -> HasPermissionAPI(admin, *) or the specific verb (cancel-any/own, retry-any/own, approve-any) so admins resolve capabilities through the Administrators group, not a literal role. requireAdmin now checks HasPermissionAPI(admin, *) and fails closed (missing auth service / invalid session / lookup error -> deny). - The stateless bootstrap admin API key (a configured secret with no user row) keeps its established full-access bypass; user-backed sessions and API keys resolve permissions purely from group membership. - Registration reviewer resolution derives admins from Administrators-group membership instead of role == "admin". Service-layer guards (issue #907): - CreateUser/UpdateUser reject zero-group membership (ErrNoGroups, 400). - Last-admin protection: cannot remove the last Administrators-group member via update or delete (ErrLastAdmin). - Self-escalation guard: a non-privileged actor editing their own groups cannot add a group they lack manage-users permission for (ErrSelfEscalation, 403). Internal callers (empty actor) are trusted. Security tests (service_group_only_authz_test.go): admin-equivalence, non-admin denial, zero-group fail-closed, lookup-error fail-closed, create/update reject-zero-groups, last-admin protection (update + delete), self-escalation denied, privileged-self-edit allowed. Frontend (role selector -> required group multi-select, permission-derived UI gating) is tracked as a follow-up; this PR is backend + migration only and MUST NOT be deployed before the frontend follow-up lands, because the API stops returning user.role and the current UI gates on it.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBackend removes Role fields and short-circuits; permissions now derive solely from group memberships. Added group invariants and guards (no-zero-groups, last-admin, self-escalation), updated DB migration/backfill, changed UpdateUserAPI to accept actor, adjusted middleware/handlers (admin API key sentinel), and rewired tests/mocks. ChangesRole-to-Groups authorization refactor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
6 tasks
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
cristim
added a commit
that referenced
this pull request
Jun 2, 2026
…ization (#914) * feat(auth/frontend): migrate UI from roles to group-membership authorization Pairs with #912 (backend). The API no longer returns user.role; all authorization derives from group membership. Administrators group UUID (00000000-0000-5000-8000-000000000001) replaces the admin-role check. Key changes: - api/types.ts: User/APIUser drop role; CreateUserRequest groups required (>=1) - permissions.ts: isAdmin() checks Administrators group membership instead of role; canAccess() returns true only for Administrators-group members until the /me/permissions endpoint lands (deferred follow-up) - auth.ts, history.ts: replace user.role === 'admin' with isAdmin() - index.html: remove role selector from user modal; groups multi-select is now required (HTML required attr + JS validation: at least 1 group required) - users/userModals.ts: saveUser rejects zero-group submissions with a clear validation message mirroring the backend DB CHECK constraint - users/userList.ts: admin count uses ADMINISTRATORS_GROUP_ID membership; effectivePermissions derives from group permissions only (no role defaults) - users/handlers.ts, userActions.ts: remove bulkChangeRole (role concept gone) - users/filters.ts: role filter maps to Administrators group membership - All test fixtures updated from role strings to groups arrays - Permissions tests rewritten for group-based model with new isAdmin tests Frontend half of #907 (backend: #912). Both PRs must land together. * refactor(test): replace hardcoded admin group UUID with ADMINISTRATORS_GROUP_ID constant Replace literal '00000000-0000-5000-8000-000000000001' with imported ADMINISTRATORS_GROUP_ID from permissions.ts in 8 test files where top-level imports can reach the factory; leave the literal (with an explanatory comment) in the 2 jest.mock() factories where permissions.ts cannot be required due to a circular import through the mocked state module (permissions.test.ts pins the UUID value as a regression guard). Fixes CodeRabbit nitpicks on PR #914.
… APIKey to fit gocyclo budget Extract checkLastAdminConstraint from guardGroupChange (service_user.go) and a shared authorizeAPIKeyAccess helper used by both RevokeAPIKey and DeleteAPIKey (service_apikeys.go). All three functions drop from 11 to 9. No behaviour change: validation order, error messages, and audit-log writes are identical.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/app.go (1)
861-869:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep a temporary compatibility shim for
user.role.These response mappers remove
Rolefrom the auth/user payloads even though the frontend follow-up is explicitly not part of this PR. That makes this branch backwards-incompatible by itself and turns safe rollout into a manual deployment-order guarantee. Please keep emitting a derived legacyrolefield until#913lands, or gate this backend rollout so old frontends can never hit it.Also applies to: 898-906, 943-944
🤖 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 `@internal/server/app.go` around lines 861 - 869, The response mappers removed the legacy `role` field causing a breaking change; restore a temporary compatibility shim by adding a Role field to api.UserInfo in the LoginResponse mapping (and any other mappings that build api.UserInfo such as the other user mappers), filling it from resp.User.Role when present or deriving a sensible fallback (e.g., the primary entry from resp.User.Groups or a default like "user") so old frontends continue to receive `role` until `#913` lands; update the api.UserInfo construction in the functions that create LoginResponse and the other user response builders to include this derived Role value.
🧹 Nitpick comments (2)
internal/database/postgres/migrations/000057_drop_user_role_to_groups_test.go (1)
55-81: ⚡ Quick winCover the unknown-role fallback here.
Line 57 says this test exercises the fail-safe net, but it only seeds
admin,user, andreadonly. Adding one unexpected legacy role would lock down theRead-Onlyfallback that prevents migrated users from ending up with zero groups.Suggested test addition
seed("admin@test.example", "admin") seed("user@test.example", "user") seed("readonly@test.example", "readonly") + seed("unknown@test.example", "legacy-custom") @@ assert.Equal(t, []string{readOnlyUsersGroupIDTest}, queryGroupIDs(t, ctx, pool, "readonly@test.example"), "readonly must be mapped to the Read-Only Users group") + assert.Equal(t, []string{readOnlyUsersGroupIDTest}, + queryGroupIDs(t, ctx, pool, "unknown@test.example"), + "unknown legacy roles must fall back to the Read-Only Users group")Based on learnings, migration tests in
internal/database/postgres/migrationsshould reuse shared helpers and lock down migration behavior in-package rather than relying on ad hoc setup.🤖 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 `@internal/database/postgres/migrations/000057_drop_user_role_to_groups_test.go` around lines 55 - 81, Add a seeded user with an unexpected legacy role to this migration test to exercise the unknown-role fallback: in the same seed helper used (seed function) insert one more user (e.g., "unknown@test.example" with role "unexpected_role") before calling migrations.RunMigrations, then after RunMigrations assert that queryGroupIDs(t, ctx, pool, "unknown@test.example") returns []string{readOnlyUsersGroupIDTest} (the Read-Only fallback). Reuse the existing seed helper and queryGroupIDs to keep the test consistent with other migration tests and ensure the fallback is locked down.internal/api/handler.go (1)
184-189: 💤 Low valueSentinel constant is safe but consider defensive validation.
The sentinel
"admin-api-key"won't collide with real user IDs because they're UUIDs, so the risk is minimal. However, for defense-in-depth, consider adding a startup check that verifies this exact string doesn't exist in the users table (e.g., due to manual DB manipulation).🤖 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 `@internal/api/handler.go` around lines 184 - 189, Add a defensive startup validation to ensure the sentinel constant apiKeyAdminUserID ("admin-api-key") does not exist as an actual user row: implement a small check (e.g., verifyNoSentinelUser or validateApiKeyAdminSentinel) that queries the users table for that ID during application initialization (call it from your main/init path) and fail-fast (log error and exit) if any matching user is found; reference apiKeyAdminUserID and the users auth store access method used elsewhere to perform the lookup.
🤖 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 `@internal/server/adapter_test.go`:
- Around line 306-311: The test currently discards the error from
adapter.UpdateUserAPI and never verifies mock expectations, so change the test
to assert the call succeeded and the mock was exercised: replace the ignored err
with an assertion such as require.NoError(t, err) (or
require.EqualError/expected error if the test expects failure) immediately after
calling adapter.UpdateUserAPI, and then call mockStore.AssertExpectations(t) to
ensure the mock received the forwarded actorUserID; ensure you reference the
UpdateUserAPI call and mockStore.AssertExpectations(t) in the updated
assertions.
---
Outside diff comments:
In `@internal/server/app.go`:
- Around line 861-869: The response mappers removed the legacy `role` field
causing a breaking change; restore a temporary compatibility shim by adding a
Role field to api.UserInfo in the LoginResponse mapping (and any other mappings
that build api.UserInfo such as the other user mappers), filling it from
resp.User.Role when present or deriving a sensible fallback (e.g., the primary
entry from resp.User.Groups or a default like "user") so old frontends continue
to receive `role` until `#913` lands; update the api.UserInfo construction in the
functions that create LoginResponse and the other user response builders to
include this derived Role value.
---
Nitpick comments:
In `@internal/api/handler.go`:
- Around line 184-189: Add a defensive startup validation to ensure the sentinel
constant apiKeyAdminUserID ("admin-api-key") does not exist as an actual user
row: implement a small check (e.g., verifyNoSentinelUser or
validateApiKeyAdminSentinel) that queries the users table for that ID during
application initialization (call it from your main/init path) and fail-fast (log
error and exit) if any matching user is found; reference apiKeyAdminUserID and
the users auth store access method used elsewhere to perform the lookup.
In
`@internal/database/postgres/migrations/000057_drop_user_role_to_groups_test.go`:
- Around line 55-81: Add a seeded user with an unexpected legacy role to this
migration test to exercise the unknown-role fallback: in the same seed helper
used (seed function) insert one more user (e.g., "unknown@test.example" with
role "unexpected_role") before calling migrations.RunMigrations, then after
RunMigrations assert that queryGroupIDs(t, ctx, pool, "unknown@test.example")
returns []string{readOnlyUsersGroupIDTest} (the Read-Only fallback). Reuse the
existing seed helper and queryGroupIDs to keep the test consistent with other
migration tests and ensure the fallback is locked down.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7486a4f1-d952-4d3c-bd3e-40d694999700
📒 Files selected for processing (64)
internal/api/coverage_extras_test.gointernal/api/coverage_gaps_test.gointernal/api/handler.gointernal/api/handler_accounts_router_test.gointernal/api/handler_accounts_test.gointernal/api/handler_analytics_test.gointernal/api/handler_apikeys_test.gointernal/api/handler_auth.gointernal/api/handler_auth_test.gointernal/api/handler_config_test.gointernal/api/handler_coverage_test.gointernal/api/handler_dashboard_test.gointernal/api/handler_federation_test.gointernal/api/handler_groups_test.gointernal/api/handler_history_test.gointernal/api/handler_inventory_test.gointernal/api/handler_per_account_perms_test.gointernal/api/handler_plans_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/api/handler_registrations.gointernal/api/handler_registrations_recipients_test.gointernal/api/handler_ri_exchange.gointernal/api/handler_ri_exchange_test.gointernal/api/handler_router_test.gointernal/api/handler_test.gointernal/api/handler_users.gointernal/api/handler_users_test.gointernal/api/middleware.gointernal/api/middleware_test.gointernal/api/mocks_test.gointernal/api/router_660_permission_flips_test.gointernal/api/router_authuser_test.gointernal/api/router_handlers_test.gointernal/api/types.gointernal/auth/errors.gointernal/auth/interfaces.gointernal/auth/service.gointernal/auth/service_api.gointernal/auth/service_api_test.gointernal/auth/service_apikeys.gointernal/auth/service_apikeys_api_test.gointernal/auth/service_apikeys_test.gointernal/auth/service_group.gointernal/auth/service_group_only_authz_test.gointernal/auth/service_group_test.gointernal/auth/service_helpers.gointernal/auth/service_lockout_test.gointernal/auth/service_password_callback_test.gointernal/auth/service_test.gointernal/auth/service_user.gointernal/auth/service_user_test.gointernal/auth/store_postgres.gointernal/auth/store_postgres_test.gointernal/auth/test_helpers.gointernal/auth/types.gointernal/database/postgres/migrations/000057_drop_user_role_to_groups.down.sqlinternal/database/postgres/migrations/000057_drop_user_role_to_groups.up.sqlinternal/database/postgres/migrations/000057_drop_user_role_to_groups_test.gointernal/database/postgres/migrations/backfill_admin_group_ids_test.gointernal/mocks/stores.gointernal/server/adapter_test.gointernal/server/app.gointernal/server/health_test.go
💤 Files with no reviewable changes (6)
- internal/auth/service_password_callback_test.go
- internal/api/middleware_test.go
- internal/auth/service_lockout_test.go
- internal/api/handler_per_account_perms_test.go
- internal/auth/service_helpers.go
- internal/auth/service.go
CodeRabbit flagged that the adapter test ignored its error and skipped mock expectation verification, so it silently passed even if the adapter stopped forwarding actorUserID to s.UpdateUser (the #907 self-escalation guard hook). It also passed a map[string]interface{} payload that fails the APIUpdateUserRequest type assertion in UpdateUserAPI, so the test only ever exercised the early-error path. Switch to a properly typed auth.APIUpdateUserRequest with a group change, add require.NoError + mockStore.AssertExpectations, and document why neither the self-escalation nor the last-admin guard fires for this shape (actor != target, group change does not remove Administrators). A regression in the actor-forwarding wiring now breaks this test instead of going unnoticed. Refs #907.
…912) CodeRabbit noted the migration test seeded only the three known legacy roles (admin/user/readonly), so the fail-safe net at step 3 of the .up.sql (any user STILL with zero groups -> Read-Only Users) was never asserted. A bug in that step could land in production silently. Add one more seed row with a non-canonical role value to exercise the fallback path. The valid_role CHECK has to be dropped first because it restricts the column to {admin,user,readonly}; this matches the realistic shape of legacy databases where the CHECK may have been relaxed before hitting this migration. After re-applying 000057, assert the row landed in the Read-Only Users group, locking down the migration's promise that no migrated user can end up with zero groups regardless of their prior role value. Refs #907.
Member
Author
|
Addressed CR pass on
@coderabbitai review |
Contributor
|
Triggering a fresh review now. ✅ Actions performedReview triggered.
|
This was referenced Jun 2, 2026
6 tasks
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
) * feat(auth): add GET /api/auth/me/permissions + wire frontend canAccess (closes #917) Adds a new authenticated endpoint that returns the effective permission set for the current user (union of all group permissions) so the frontend can gate UI elements based on real group-derived access instead of the admin-or-nothing stub that has been blocking every non-admin user since the group-only authz migration (#912/#914). Backend: - Add GET /api/auth/me/permissions (AuthUser level, mirrors /api/auth/me) - Handler delegates to GetUserPermissionsAPI -> GetUserPermissions (same union path the server enforces with), returns {permissions, is_admin} - Add GetUserPermissionsAPI to AuthServiceInterface + implementation in service_api.go via the existing GetUserPermissions union logic - Register route in router.go; add adapter in server/app.go; add mock method + 6 handler tests covering multi-group user, admin wildcard, and 401 paths Frontend: - Add getUserPermissions() in api/auth.ts and re-export via api/index.ts - Extend User type with optional effectivePermissions field - Rewrite canAccess() to consult effectivePermissions when present; falls back to admin group-membership check while loading (fails closed) - Fetch permissions in app.ts bootstrap after getCurrentUser(), merge onto current-user state; best-effort (errors do not block app loading) - Update permissions.test.ts: add canAccess tests for the effective- permissions path (multi-group union, admin wildcard, empty set); keep fallback-path tests with updated comments * fix(api/auth): honor AuthUser auth + fail loudly on bad permissions payload CR #922 findings F1 + F2 on GET /api/auth/me/permissions, addressed together because they share the handler body: F1 — Unchecked type assertion on GetUserPermissionsAPI's `any` return swallowed adapter/type mismatches and degraded to an empty PermissionEntry slice. The frontend then renders the caller as having lost all access, hiding the real server bug. Switch to a checked assertion + 500 with a logged "GetUserPermissionsAPI returned %T, want []auth.APIPermission" line. F2 — The route is registered with Auth: AuthUser, which admits the stateless admin API key, any user API key, OR a bearer-token session (see router.go AuthLevel doc). The previous body re-required a bearer session after the router had already admitted the caller, so a valid admin / user API-key request would still 401 on this endpoint. Resolve the user ID from any of the three credentials via a new resolveAuthenticatedUserID helper, then look up permissions. The admin API key has no backing user row, so it short-circuits to {admin, *} + is_admin=true (matches the requireAdmin / requirePermission convention). Regression tests: - TestHandler_getCurrentUserPermissions_UnexpectedPayload (F1) - TestHandler_getCurrentUserPermissions_AdminAPIKey (F2) - TestHandler_getCurrentUserPermissions_UserAPIKey (F2) * fix(frontend/auth): runtime-validate /me/permissions response shape CR #922 finding F3. getUserPermissions() previously cast the response directly to UserPermissionsResponse with no shape check. canAccess() later iterates user.effectivePermissions with `for (const p of ...)` and reads p.action / p.resource — a non-array `permissions`, a null entry, or a non-string action/resource throws synchronously outside the app.ts fetch/merge try/catch and crashes the bootstrap path. Align with the existing pattern in this file (getResetTokenStatus, setupMFA, enableMFA, regenerateMFARecoveryCodes): validate the response is an object, permissions is an array, every entry is an object with string action and string resource, and is_admin is a boolean. Throw a descriptive error on any mismatch so the caller in app.ts falls back to the safe group-membership gating. Adds frontend/src/__tests__/api-permissions.test.ts covering the happy path plus each rejection branch (non-object response, null response, non-array permissions, null entry, missing action, non-string resource, missing is_admin, non-boolean is_admin).
9 tasks
This was referenced Jun 3, 2026
Closed
Merged
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
…kUser (closes #935) (#936) The mockUser helper was still supplying `role: 'admin'` after the group-membership authz migration (#912/#917). permissions.ts isAdmin() now reads user.groups, so the old fixture always yielded canEdit=false, the lookback change listener was never attached, and 11 assertions failed on the base branch. Mirror the pattern already used in recommendations-permissions.test.ts: import ADMINISTRATORS_GROUP_ID and set groups: [ADMINISTRATORS_GROUP_ID] for admin, groups: [] for non-admin roles.
cristim
added a commit
that referenced
this pull request
Jun 4, 2026
…ect gate (closes #940) (#941) * fix(api/purchases): drop removed session.Role shortcut in execute-direct gate (closes #940) PR #912 (#907) removed the Role field from api.Session, leaving authorizeSessionExecuteDirect with a dead `if session.Role == "admin"` reference that prevented the base branch from compiling. Remove the three-line shortcut and update the gate-logic doc comment to match the sibling authorizeSessionApprove/authorizeSessionCancel pattern: admins pass via the execute-any HasPermissionAPI check because the Administrators group carries the {admin,*} wildcard permission. Also fix all test Session struct literals that still set Role (now an unknown field), update the admin-role-shortcut test to properly stub the HasPermissionAPI path, and add three regression tests: - AdminGroupViaExecuteAny: confirms admin users still pass via execute-any - NoGrant: confirms non-admin without execute-any/own gets 403 - NilAuth: confirms nil auth component returns 500 (fail-closed) * fix(db): renumber duplicate migration 000058_seed_purchaser_group to 000059 (base collision from #803+#924) The base feat/multicloud-web-frontend merged two 000058 migrations from the recent wave, which the check-migration-conflicts pre-commit hook rejects: - 000058_purchase_executions_direct_execute_audit (#803, schema change) - 000058_seed_purchaser_group (#924, data seed) Keep the schema-change audit migration at 000058 and renumber the seed migration to the next free slot, 000059 (base jumps 000058 -> 000063, so 000059-000062 are open). golang-migrate discovers migrations by filename glob, so no embed list needs updating; only two self-references were adjusted: - auth/types.go GroupPurchaser doc comment pointing at the filename - the self-referencing error message inside the up.sql Safe to renumber without applied-state reconciliation: the base has not compiled since the #803/#907 break, so neither 000058 migration has been applied to any database yet.
This was referenced Jun 4, 2026
cristim
added a commit
that referenced
this pull request
Jun 4, 2026
…ple bootstrap from migration step (closes #945) (#947) * fix(api/purchases): drop removed session.Role shortcut in execute-direct gate (closes #940) PR #912 (#907) removed the Role field from api.Session, leaving authorizeSessionExecuteDirect with a dead `if session.Role == "admin"` reference that prevented the base branch from compiling. Remove the three-line shortcut and update the gate-logic doc comment to match the sibling authorizeSessionApprove/authorizeSessionCancel pattern: admins pass via the execute-any HasPermissionAPI check because the Administrators group carries the {admin,*} wildcard permission. Also fix all test Session struct literals that still set Role (now an unknown field), update the admin-role-shortcut test to properly stub the HasPermissionAPI path, and add three regression tests: - AdminGroupViaExecuteAny: confirms admin users still pass via execute-any - NoGrant: confirms non-admin without execute-any/own gets 403 - NilAuth: confirms nil auth component returns 500 (fail-closed) * fix(db): renumber duplicate migration 000058_seed_purchaser_group to 000059 (base collision from #803+#924) The base feat/multicloud-web-frontend merged two 000058 migrations from the recent wave, which the check-migration-conflicts pre-commit hook rejects: - 000058_purchase_executions_direct_execute_audit (#803, schema change) - 000058_seed_purchaser_group (#924, data seed) Keep the schema-change audit migration at 000058 and renumber the seed migration to the next free slot, 000059 (base jumps 000058 -> 000063, so 000059-000062 are open). golang-migrate discovers migrations by filename glob, so no embed list needs updating; only two self-references were adjusted: - auth/types.go GroupPurchaser doc comment pointing at the filename - the self-referencing error message inside the up.sql Safe to renumber without applied-state reconciliation: the base has not compiled since the #803/#907 break, so neither 000058 migration has been applied to any database yet. * fix(db/migrate): admin-upsert SQL parity with post-057 schema + decouple bootstrap from migration step (closes #945) Migration 000057 dropped the users.role column and sessions.role column (PR #912). The Go bootstrap functions in migrate.go were not updated at the time, leaving three SQL statements that reference the dropped column: - ensureAdminUser: INSERT INTO users included `role` in column list / VALUES - ensureAdminUserWithPassword: same INSERT pattern - assignAdminGroupAndWarn: two queries used WHERE role = 'admin' This caused every Lambda cold start to fail the admin-upsert step with "column role of relation users does not exist (SQLSTATE 42703)". The admin-upsert failure also masked a second deployment gap: migration 000058 (purchase_executions direct-execute audit columns) was missing from the deployed image, causing all purchase-execution reads/writes to fail with column-not-found errors. The image rebuild (ops action) will apply 000058 on the next cold start. Changes: - Remove `role` from INSERT column list and VALUES in both ensureAdminUser and ensureAdminUserWithPassword - Replace `WHERE role = 'admin'` in assignAdminGroupAndWarn with `WHERE (group_ids IS NULL OR cardinality(group_ids) = 0)`; post-057, the users_min_one_group CHECK makes this a permanent no-op so the backfill path is defence-in-depth for rollback scenarios only - Decouple admin-bootstrap failure from RunMigrations return value: if m.Up() succeeds but ensureAdminUser fails, RunMigrations now logs a WARNING and returns nil so the health endpoint correctly reports migration success and the app boots cleanly - Update ensure_admin_user_test.go: the pre-057 drift scenario (empty group_ids on an existing admin row) is structurally impossible post-057 due to the CHECK constraint; replace with an idempotency test that verifies re-running RunMigrations on an already-correct admin row is a no-op - Update migrate_security_integration_test.go: the same drift simulation (INSERT with role + empty group_ids after migrations) is impossible post-057; test now exercises the "Admin user created" log path instead * fix(db/migrate): address CR feedback on PR #947 - Scope assignAdminGroupAndWarn to the bootstrap admin email only; the previous UPDATE/COUNT targeted all users with empty group_ids, which would assign the Administrators group to non-admin users in pre-057 or rollback states (CR finding, major severity) - Remove dropped `role = 'admin'` predicate from test SQL in the operator-customisation sub-case; the column was dropped by migration 000057 so the WHERE clause would always return zero rows on the current schema (CR outside-diff finding) - Deferred: frontend 000058->000059 comment update is out of PR scope; filed as issue #955
This was referenced Jun 5, 2026
cristim
added a commit
that referenced
this pull request
Jun 5, 2026
…ionAPI for admin gate, assert SourceIdentity admin-only - Remove Session.Role field from all Session literals in handler_config_test.go (Role was removed in #907/#912; role-based short-circuit replaced by group membership via HasPermissionAPI) - Add mockAuth.grantAdmin() to TestHandler_getConfig so the admin permission check in requireAdmin is satisfied - Add explicit HasPermissionAPI stub for "admin-user" in TestHandler_getConfig_SourceIdentity_AdminOnly (required now that the admin path goes through HasPermissionAPI, not a Role field) - Replace no-op `_ = result.SourceIdentity` with require.NotNil assertion (admin sessions must receive SourceIdentity per issue #407) - Non-admin subtest already asserts SourceIdentity == nil via assert.Nil
3 tasks
cristim
added a commit
that referenced
this pull request
Jun 5, 2026
…#974 Render a bulk-actions toolbar inside the Users fieldset that becomes visible when one or more user rows are checked and hides again when the selection is cleared. The toolbar contains: - a selection counter ("N users selected") - a Delete selected button wired to the existing bulkDeleteUsers() - an "Add to group..." select populated with available groups, wired to the existing bulkAddToGroup(); resets to the placeholder after each operation completes Implementation notes: - toolbar HTML added to frontend/src/index.html (id=bulk-actions-bar, hidden class by default; matches the idiom already used by updateBulkActionsBar in userList.ts) - populateBulkGroupSelect() added to userList.ts (alongside updateBulkActionsBar) and called by loadUsers() so options stay in sync after every refresh - handlers.ts wires the bulk-group-select change event and calls populateBulkGroupSelect() at setup time - bulk-change-role button intentionally absent (removed in PR #912) - 8 new tests covering: toolbar hidden/shown/count on selection changes, unchecking all hides toolbar, delete btn calls API, select populates with groups, select change calls bulkAddToGroup, placeholder selection is a no-op
cristim
added a commit
that referenced
this pull request
Jun 5, 2026
…#974 (#977) Render a bulk-actions toolbar inside the Users fieldset that becomes visible when one or more user rows are checked and hides again when the selection is cleared. The toolbar contains: - a selection counter ("N users selected") - a Delete selected button wired to the existing bulkDeleteUsers() - an "Add to group..." select populated with available groups, wired to the existing bulkAddToGroup(); resets to the placeholder after each operation completes Implementation notes: - toolbar HTML added to frontend/src/index.html (id=bulk-actions-bar, hidden class by default; matches the idiom already used by updateBulkActionsBar in userList.ts) - populateBulkGroupSelect() added to userList.ts (alongside updateBulkActionsBar) and called by loadUsers() so options stay in sync after every refresh - handlers.ts wires the bulk-group-select change event and calls populateBulkGroupSelect() at setup time - bulk-change-role button intentionally absent (removed in PR #912) - 8 new tests covering: toolbar hidden/shown/count on selection changes, unchecking all hides toolbar, delete btn calls API, select populates with groups, select change calls bulkAddToGroup, placeholder selection is a no-op
cristim
added a commit
that referenced
this pull request
Jun 6, 2026
closes #407) (#530) * sec(api): restrict SourceIdentity to admin sessions in GET /api/config (closes #407) GET /api/config returned SourceIdentity (AWS account ID, Azure tenant ID / subscription ID, GCP project ID) to any authenticated user. Non-admin roles should not have access to the host cloud identity. Change getConfig to accept the request so it can call requireAdmin. Only populate SourceIdentity when the caller is an admin; non-admin responses get SourceIdentity: nil. SourceCloud (the cloud name string "aws"/"azure") is still returned to all authenticated users as it is needed by the dashboard for feature gating. Update all call sites (router wrapper, existing tests). Add regression tests confirming non-admin sessions receive a nil SourceIdentity. * test(api/config): rebase + drop removed Session.Role, mock HasPermissionAPI for admin gate, assert SourceIdentity admin-only - Remove Session.Role field from all Session literals in handler_config_test.go (Role was removed in #907/#912; role-based short-circuit replaced by group membership via HasPermissionAPI) - Add mockAuth.grantAdmin() to TestHandler_getConfig so the admin permission check in requireAdmin is satisfied - Add explicit HasPermissionAPI stub for "admin-user" in TestHandler_getConfig_SourceIdentity_AdminOnly (required now that the admin path goes through HasPermissionAPI, not a Role field) - Replace no-op `_ = result.SourceIdentity` with require.NotNil assertion (admin sessions must receive SourceIdentity per issue #407) - Non-admin subtest already asserts SourceIdentity == nil via assert.Nil
This was referenced Jun 9, 2026
9 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.
Closes #907.
Collapses the dual authorization model (
role+group_ids) down to group membership only. Every authorization decision now derives from the union of the user's groups' permissions; theusers.roleandsessions.rolecolumns are removed. Backend + migration only — the frontend follow-up is tracked separately (see Frontend below).Role -> group mapping (migration
000057)admin00000000-0000-5000-8000-000000000001user00000000-0000-5000-8000-000000000005readonly00000000-0000-5000-8000-000000000006000057_drop_user_role_to_groupsis load-bearing in order:group_idsfromrolefor any zero-group user.group_ids NOT NULL DEFAULT '{}'+CHECK (cardinality(group_ids) >= 1), then drop therolecolumns last.The DB can no longer represent a zero-group user. The down migration restores
rolefrom group membership.How each
session.Role == "admin"short-circuit was replacedrequirePermissionalready routed throughHasPermissionAPI(action, resource)-> group-union permissions.requireAdmin(coarse admin routes) now checksHasPermissionAPI(admin, *)— true only for Administrators-group members. Fails closed: missing auth service / invalid session / lookup error -> deny.cancel-any|cancel-own,retry-any|retry-own,approve-anyverbs resolved from groups instead of a role bypass.requireAdmin.gatherAdminEmails/isAdminGroupMember), notrole == "admin".Service-layer guards
CreateUser/UpdateUserreject zero-group membership (ErrNoGroups, surfaced as 400; also enforced by the DB CHECK).ErrLastAdmin).ErrSelfEscalation, 403). Internal callers (empty actor) are trusted.UpdateUserAPInow takes the trusted actor user ID from the validated session.Security tests (
internal/auth/service_group_only_authz_test.go)admin-equivalence, non-admin denial, zero-group fail-closed, lookup-error fail-closed, create reject-zero-groups, update reject-zero-groups, last-admin protection (update + delete), self-escalation denied, privileged-self-edit allowed.
Frontend
Not in this PR — tracked as a follow-up (role selector -> required group multi-select; viewer/operator UI gating off effective group permissions). The backend is fully migrated and self-consistent. This PR must not be deployed before the frontend follow-up lands, because the API stops returning
user.roleand the current UI gates on it.Verification
go build ./...,go vet ./...,gofmtclean;go test ./...-> 5011 passed (38 packages);go test ./pkg/...-> 401 passed; migration tests pass.Summary by CodeRabbit
New Features
Bug Fixes
Refactor