fix: project-scoped members API + S1 security + RC-C notifications - #1407
Open
ptone wants to merge 11 commits into
Open
fix: project-scoped members API + S1 security + RC-C notifications#1407ptone wants to merge 11 commits into
ptone wants to merge 11 commits into
Conversation
added 11 commits
August 31, 2026 13:29
Implement GET/POST/PATCH/DELETE for /api/v1/projects/{id}/members,
replacing the admin role-bindings stopgap for project member management.
Key changes:
- New handlers_project_members.go with project-scoped member CRUD
- GET returns enriched bindings with roleName, source, display names
- POST/PATCH/DELETE authorize via project.manage + CanDelegate checks
- CanDelegate uses both ProjectMembership and RoleBinding grant types
to prevent escalation (project-admin cannot mint project-owner)
- Last-owner guard preserved on PATCH (role change) and DELETE
- PATCH performs atomic role change (delete+create, not create-then-delete)
- Route dispatched via handleProjectRoutes under /members path
- Updated stale CanDelegate wiring comment in authz_candelegate.go
Closes RC-A (D1, D2) and RC-B (D3, D4, D5) backend components.
Remove role_binding.read from hubMemberPermissionIDs and
hubViewerPermissionIDs to prevent any authenticated hub member from
enumerating all role bindings hub-wide.
This is now safe because the project members UI uses the new
project-scoped /api/v1/projects/{id}/members endpoint, which
authorizes via project.read instead of role_binding.read.
Role revision bumped from 1 → 2 for both hub-member and hub-viewer
per the PG1 contract (startup reconciliation will update permissions).
Updated scoped_admin_test.go and seed_roles_test.go to reflect that
hub members are now denied access to /api/v1/admin/role-bindings.
Rewrite the frontend project members editor to use the new
project-scoped /api/v1/projects/{id}/members endpoints instead of
the generic hub-admin /api/v1/admin/role-bindings stopgap.
Key changes:
- loadData() now hits GET /api/v1/projects/{id}/members
- Add member uses POST /api/v1/projects/{id}/members
- Change role uses atomic PATCH /api/v1/projects/{id}/members/{id}
(replaces the non-atomic create-then-delete dance)
- Remove member uses DELETE /api/v1/projects/{id}/members/{id}
- Removed roleBindingApiAvailable flag and loadDataFallback() method
- Removed the PM1 TODO comment — the PM1 API is now implemented
- Server now returns enriched roleName and source fields
Closes RC-A and RC-B frontend components (D1-D5).
Three changes to fix the duplicate toast+inline error on 403 responses:
1. api.ts: Add suppressAccessDeniedToast option to apiFetch.
When set, the global scion:access-denied event is not dispatched,
allowing components with local error handling to prevent double
notifications.
2. api.ts: Fix dead detail extraction in the 403 handler. The backend
error envelope is {error: {code, message}}, not {resource, action,
reason}. Now reads from the actual shape so the toast message is
specific when it fires.
3. app-shell.ts + chat-shell.ts: Guard against double-toast when both
shells are mounted. The first handler marks the event detail as
_handled; the second skips if already handled.
4. project-members-editor.ts: Set suppressAccessDeniedToast on all
mutation calls that have local error handling (add, change role,
remove member).
12 tests covering the project-scoped members endpoints: - GET returns project-scoped bindings with enriched roleName, source, and principalDisplayName fields - GET denied for non-project-members (403) - POST works for project-owner adding a member - POST denied for non-project-members (403) - POST escalation prevented: project-admin cannot mint project-owner - POST rejects non-project-scoped roles (400) - PATCH atomically changes member role - PATCH enforces last-owner guard (409 LAST_OWNER) - DELETE removes member binding - DELETE denied for project-member (403, lacks project.manage) - DELETE enforces last-owner guard (409 LAST_OWNER) - S1 regression: hub-member cannot list all role bindings (403)
…y validation) R1: Reverse PATCH handler's delete-then-create to create-then-delete, preventing data loss if create fails after delete. A brief duplicate-binding window is harmless; a missing-binding window would silently demote the user. R2: Add handler-level validation for direct-user-only roles (project-owner, project-admin) in both POST and PATCH handlers, returning 400 instead of letting the store's ErrDirectUserOnly surface as 500. Add test for group principal assignment rejection.
…ship Two issues surfaced in QA: (1) "my projects" filter missed RoleBinding- based membership because scope=mine only queried the legacy Project.OwnerID field, and (2) pre-existing projects showed "no members" because they lacked project-owner RoleBindings. Fixes: - Add backfillProjectOwnerRoleBindings to seed.go: iterates all projects at startup and creates project-owner RoleBindings from Project.CreatedBy. Idempotent (skips existing bindings). - Add resolveUserRBProjectIDs helper: queries user's project-scoped RoleBindings and returns ScopeIDs for membership resolution. - Add mergeProjectIDs helper: deduplicates project IDs from multiple sources (legacy groups + RoleBindings). - Update scope=mine, scope=shared, and mine=true filters in both project and agent list handlers to merge group-based and RoleBinding-based membership sources. - Fix gofmt alignment in handlers_project_members.go.
… check createProject and handleProjectRegister had no authorization enforcement. Any authenticated user — including hub-viewers — could create projects and, by cascading into project-owner role, create agents. Add CheckAccess for project.create at the top of both handlers, matching the authorize() pattern used by other mutation endpoints.
Suspended users could continue operating with valid JWT or UAT tokens because neither path checked user.Status. Add suspension enforcement: - UAT: ValidateToken already fetches the user — add Status check and return ErrUserSuspended, surfaced as 403 in the middleware. - JWT: tokens are self-contained, so add a per-request user lookup in UnifiedAuthMiddleware to check current status from the store. - Wire UserStore into AuthConfig so the middleware can query user state. The full kernel-level suspension restriction (B1 scope) will be wired through the Decide() pipeline on scion/constraint-uiux.
The S1 fix bumped hub-member to revision 2. Update the legacy integer marker reconciliation test to expect the current revision.
The JWT auth path silently continued when UserStore.GetUser() returned a non-ErrNotFound error (e.g. transient DB outage), allowing suspended users with valid JWTs to bypass the suspension gate. Return 503 Service Unavailable on store errors instead, making the JWT path fail-closed like the UAT path. Only ErrNotFound (deleted user) falls through, where downstream handlers enforce the missing-record invariant. Addresses R1 from review round 4 (policy-fix-review-r4).
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.
Summary
Fixes all 6 QA defects (D1-D6) and the S1 security finding from the authorization foundation refactor (PR #1435). The root causes were:
/api/v1/admin/role-bindingsendpoint, which returned unfiltered system-wide bindings with no role name or provenance, and required system-scopedrole_binding.*permissions that project owners cannot hold.apiFetchdispatched a global 403 toast unconditionally while components also rendered the error inline, causing duplicate notifications.Changes
Backend — new project-scoped members API:
GET /api/v1/projects/{id}/members— lists project-scoped bindings with enrichedroleName,source, and display namesPOST /api/v1/projects/{id}/members— adds a member with CanDelegate escalation controlPATCH /api/v1/projects/{id}/members/{bindingID}— atomic role changeDELETE /api/v1/projects/{id}/members/{bindingID}— removes member with last-owner guardproject.manage/project.readat project scopeBackend — S1 security fix:
role_binding.readfromhubMemberPermissionIDs()andhubViewerPermissionIDs()Frontend — project-members-editor rewrite:
roleBindingApiAvailablefallback machinery andloadDataFallback()Frontend — RC-C fix (duplicate 403 notifications):
suppressAccessDeniedToastoption toapiFetch{error: {code, message}}envelope)app-shell.tsandchat-shell.tsTests:
scoped_admin_test.goandseed_roles_test.gofor S1 changesCloses #1406
Test plan