feat(api): add group management surface - #545
Open
abdulrafey1 wants to merge 7 commits into
Open
Conversation
abdulrafey1
changed the title
group.create/read/update/delete gate the group-management surface; granted to the seeded admin role by data migration so existing admins can manage groups. The group engine functions join the sparkth.lib.permissions facade for the API and CLI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CRUD for user groups under /api/v1/permissions/groups, gated by the group.* permissions at the global scope; domain exceptions mapped centrally (404/409). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
groups add-member and groups assign-role-to-group manage the principal/grant edges, mirroring the CLI-only posture of user-role assignment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
abdulrafey1
force-pushed
the
rafey/feat/permission-groups-api
branch
from
July 27, 2026 12:20
bd2a2b0 to
2a54f17
Compare
…rmission-groups-api
hamza-56
reviewed
Jul 28, 2026
…rmission-groups-api
…rmission-groups-api # Conflicts: # frontend/lib/api/generated.ts # sparkth/api/v1/permissions/routes/__init__.py
- add "groups remove-member" and "groups revoke-role-from-group" so membership and group-role grants are no longer one-way doors (the engine functions existed but nothing was wired to them) - extract _resolve_user/_resolve_group_id helpers, removing the copy-pasted lookup/not-found/exit blocks and the dead id-None branch - note the known PATCH limitation on GroupUpdate (a description cannot be cleared back to null because None means "unchanged") Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
regisb
approved these changes
Jul 30, 2026
regisb
left a comment
regisb
left a comment
Member
There was a problem hiding this comment.
Super clean PR. Just one minor comment (feel free to ignore) and question.
| ) | ||
| async def create_group(payload: GroupCreate, session: AsyncSession = Depends(get_async_session)) -> GroupResponse: | ||
| """Create a group. Returns the created GroupResponse (id, name, description).""" | ||
| return _group_to_response(await group_engine.create_group(payload.name, payload.description, session)) |
Member
There was a problem hiding this comment.
Are we not catching exceptions thrown by group_engine.create_group in these endpoints? E.g: I believe we should raise 40x if a group already exists?
| @@ -0,0 +1,82 @@ | |||
| """Group sub-router (mounted at ``/groups``): CRUD for user groups (issue #519). | |||
Member
There was a problem hiding this comment.
I like how this router is really clean.
| permission_scope = get_permission_scope(scope) | ||
| except PermissionScopeNotFound: | ||
| typer.secho(f"Unknown scope kind: '{scope}'", fg=typer.colors.RED) | ||
| raise typer.Exit(code=1) from None |
Member
There was a problem hiding this comment.
nit: I'm not too confortable running typer.Exit in non-CLI functions. I understand this specific function is only called in CLI, but it might be a future footgun during a refactoring. Feel free to ignore this comment.
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
3 participants
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: Permission system needs groups to grant roles to users in bulk
Part 2 of 3 stacked PRs — stacked on #544 (engine). Management surface for the group tables the engine PR introduced.
What
Adds the management surface for permission groups: REST CRUD for group structure, CLI for membership and group-role grants, and the
group.*permissions gating it — following the split already established for roles (structure via REST, principal/grant edges via CLI).Changes
group.create/group.read/group.update/group.deletepermissions and export the group engine surface throughsparkth.lib.permissions5c6ccf073bb7grants the fourgroup.*permissions to the seededadminrole (no-op when the role isn't seeded)/api/v1/permissions/groups(list / create / get / patch / delete), gated at the global scope;GroupNotFound → 404,GroupAlreadyExists/GroupInUse → 409via the central exception registrygroups add-member <user> <group>andgroups assign-role-to-group <group> <role> [--scope] [--scope-object-id]group.*rows in the permissions guide's shipped-permissions tablefrontend/lib/api/generated.tsfrom the OpenAPI schemaHow to Test
make test.backend.pytest— full suite green (1512 passed); new coverage intests/api/v1/test_permission_groups.pyandtests/cli/test_groups.py.make migrations— applies5c6ccf073bb7(verified up and down against PostgreSQL).POST /api/v1/permissions/groups {"name": "cs-staff"}→ 201; duplicate name → 409 with exact detail body.make cli -- groups add-member <username> cs-staffthenmake cli -- groups assign-role-to-group cs-staff <role>— every member now passescan()checks for that role's permissions at the granted scope.Notes
5c6ccf073bb7(admin grants only, no schema change).This PR description was written with the assistance of an LLM (Claude).