SP136: expose subsystem RBAC access via GET/PUT .../access - #1523
SP136: expose subsystem RBAC access via GET/PUT .../access#1523ikethecoder wants to merge 1 commit into
Conversation
Add GET/PUT /organizations/{org}/subsystems/{name}/access to
OrgSubsystemController, wiring existing but previously unreachable
group-membership logic (GroupAccessService.getGroupMembership /
SysGroupAccessService.createOrUpdateGroupAccess) to a dedicated route,
so subsystem RBAC (system-owner, tech-lead, access-manager) can be
viewed and changed after the initial grant made at gateway registration.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Looks good.
Couple comments to consider:
- No activity logging, not sure when this will be implemented
- Probably just a weird edge case, but any chance someone would configure subsystem access before registering the gateway (which would overwrite the configured access)?
| */ | ||
| @Put('/{name}/access') | ||
| @OperationId('putSubsystemAccess') | ||
| @Security('jwt', ['System.Manage']) |
There was a problem hiding this comment.
Could you please confirm the intended authorization boundary for changing subsystem membership? The existing organization access PUT uses GroupAccess.Manage, while this RBAC mutation uses System.Manage. Is System.Manage intentionally sufficient to grant or revoke the system-owner, tech-lead, and access-manager roles?
| public async putAccess( | ||
| @Path() org: string, | ||
| @Path() name: string, | ||
| @Body() body: { members: GroupMember[] }, |
There was a problem hiding this comment.
Could the accepted role names be validated before this full-membership sync? GroupMember.roles is a string[], while SysGroupAccessService only processes the predefined system roles. An unrecognized or misspelled role is therefore ignored while absent recognized roles are synchronized as empty, which can revoke current assignments. Would rejecting unknown role names protect against accidental destructive updates?
| envConfig.clientSecret | ||
| ); | ||
|
|
||
| await sysGroupAccessService.createOrUpdateGroupAccess( |
There was a problem hiding this comment.
Could you clarify whether subsystem access changes should produce activity or audit records? The existing organization-access PUT uses the returned granted/revoked diff to call logOrganizationAccessChanges, while the diff from this RBAC update is discarded. If that omission is intentional, some context here would be helpful.
There was a problem hiding this comment.
createOrUpdateGroupAccess performs a full membership sync, but silently drops emails that cannot be resolved in Keycloak. As a result, a request containing an unknown email returns 204 while revoking the existing members for all three roles.
Could we validate that every requested member resolves before starting the sync, and return 400 if any do not?
4 participants
Summary
GET /organizations/{org}/subsystems/{name}/accessandPUT /organizations/{org}/subsystems/{name}/accesstoOrgSubsystemController, so a subsystem's RBAC role assignments (System Owner, Technical Lead, Access Manager) can be viewed and changed directly.SysGroupAccessService.createOrUpdateGroupAccess('subsystem', ...)) and read path (GroupAccessService.getGroupMembership(clientId)) already existed and are already exercised internally (at gateway-registration time, and for catalogaccessenrichment respectively) — this just exposes them as a dedicated route, mirroring the existingGET/PUT /organizations/{org}/accesspattern./accessroutes.Test plan
e2e/cypress/tests/99-sp136/01-subsystem-access.tsdemonstrates the fix: registers a subsystem gateway (which auto-grants the acting user all three roles), confirmsGET .../accessreturns that membership, thenPUT .../accessto replace membership with a second user holding onlytech-lead, and confirms the change via a follow-upGET.404against the unfixed controller (temporarily reverted just the controller change, rebuilt, reran) and passes once the fix is restored.e2e/cypress/tests/21-sdx-api/v1/01-subsystems.ts(11 passing) ande2e/cypress/tests/19-api-v3/02-organization.ts(9 passing) both green against the fixed branch.tsc --noEmitclean onsrc/.🤖 Generated with Claude Code