Skip to content

#36937 #36938: feat(roles): add role-membership endpoints (grant user, bulk remove users) - #37077

Merged
hassandotcms merged 11 commits into
mainfrom
36937-36938-roles-api-role-membership
Aug 19, 2026
Merged

#36937 #36938: feat(roles): add role-membership endpoints (grant user, bulk remove users)#37077
hassandotcms merged 11 commits into
mainfrom
36937-36938-roles-api-role-membership

Conversation

@hassandotcms

@hassandotcms hassandotcms commented Aug 16, 2026 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

Adds the two role-membership mutations for the Angular Roles & Tools portlet migration (epic #36909), replacing DWR RoleAjax#addUserToRole / #removeUsersFromRole.

Closes #36937, closes #36938. #36939

POST /v1/roles/{roleid}/users/{userId} — grant user to role

Grants a direct membership. Returns {granted, roleId, user: {userId, email, fullName}}.

  • Idempotent (legacy parity): granting an already-held role — direct or inherited — returns 200 and changes nothing. Membership inherits down the role tree, so granting an inherited role does not create a direct membership; documented in the OpenAPI description.
  • editUsers=false role → 403 (covers workflow/system roles). Missing role/user → 404.

DELETE /v1/roles/{roleid}/users — bulk remove members

JSON body {userIds: [...]} (same pattern as bulk category delete).

  • Partial success: {removedUserIds, skipped: [{userId, reason: not_found|inherited|error}]}. inherited reports what legacy silently no-ops.
  • Per-user commits, no batch transaction — a batch wrapper would roll back earlier removals when a later entry fails.
  • Empty/null/blank userIds → 400. Missing role → 404.

Both

  • Auth: backend user + Roles portlet + CMS admin (same gate as PUT/DELETE role).
  • Audit logs per mutation; typed immutable views; openapi.yaml regenerated per commit.

Testing

22 new integration tests in RoleResourceIntegrationTest (MainSuite3a), TDD red→green.
Security review: no critical/high findings.

This PR fixes: #36937

@hassandotcms
#36936: feat(roles): add PUT /v1/roles/{roleId} for role update and r…
a5465c0 
…eparent

* New v1 REST endpoint replacing DWR RoleAjax#updateRole for the Angular
  Roles & Tools portlet (epic #36909): updates name, key, description,
  can-grant flags and parent; null parentRoleId reparents to root (DWR parity)
* Guards: 404 missing role/parent, 403 system/locked roles, 400 invalid name
  and reparent cycles (net-new corruption guard), 409 duplicate key/name
* Auth: backend user + roles portlet + CMS admin, shared gate extracted and
  reused by POST /v1/roles (behavior unchanged, regression-tested)
* RoleHelper promoted to @ApplicationScoped CDI with the update logic under
  @WrapInTransaction; response reuses RoleView/ResponseEntityRoleDetailView
  (same shape as GET /v1/roles/{roleid})
* 15 integration tests (TDD red->green), registered in MainSuite3a
* Regenerated openapi.yaml
@hassandotcms
Merge branch 'main' into 36936-roles-api-put-role-update
d2b55cf
@hassandotcms
#36936: docs(roles): state PUT full-replace semantics and pin them wi…
3a604e0 
…th a test

* OpenAPI description now spells out that PUT overwrites every field:
  omitted booleans reset to false, omitted roleKey/description are cleared,
  omitted parentRoleId reparents to root (DWR parity)
* New IT testUpdateRole_fullReplace_omittedFieldsAreReset pins the contract
  so drift to merge/PATCH semantics is a deliberate, test-breaking change
* Addresses claude[bot] review finding on PR #37012
@hassandotcms
#36936: fix(roles): apply role updates to a detached copy so rejected…
e22d5f0 
… saves cannot poison the role cache

Review fixes for PR #37012 (fabrizzio-dotCMS):

* loadRoleById returns the cache-resident Role instance; the update previously
  mutated it in place before validation, so a rejected save (duplicate key/name,
  invalid name, cycle, missing parent) left phantom values in the local cache.
  Now: validate first, then copy the role (BeanUtils.copyProperties — the same
  mechanism RoleFactoryImpl.save uses) and save the copy
* Rejection tests now pre-warm the cache (production-warm case: entries cached
  inside the failing transaction are rollback-evicted by
  CommitListenerCacheWrapper and mask the bug) and assert post-rejection that
  name/key/description survived — all five failed before the fix, 16/16 after
* Replace hand-rolled ancestor walk with roleAPI.isParentRole
* Drop dead CDI annotations on RoleHelper (nothing injects it; transactions are
  ByteBuddy-woven, not CDI-intercepted)
* Align path template with sibling GET: {roleId} -> {roleid}; regenerated openapi
* MainSuite3a: import instead of fully-qualified suite entry
@hassandotcms
#36939: feat(roles): add DELETE /v1/roles/{roleId} for role deletion
5fa9827 
Cascading delete (legacy RoleAPIImpl.delete parity): removes the role from
all users, strips its permissions, detaches its layouts. Blocks only where
legacy blocks: children (409), workflow-action Assign To references (409,
pre-checked because delete() re-wraps the check into a generic failure),
system/locked roles (403). Response reports usersAffected as the cascade
blast radius via a new typed RoleDeletionView.
@hassandotcms
#36937: feat(roles): add POST /v1/roles/{roleid}/users/{userId} to gr…
305f83d 
…ant user

Grants a role to a user as a direct membership, replacing the DWR RoleAjax#addUserToRole path for the Angular Roles & Tools portlet (epic #36909).

- Idempotent: granting an already-held role (direct or inherited) returns 200 and changes nothing (legacy RoleAPIImpl.addRoleToUser parity)
- Already-holds check runs BEFORE the editUsers gate (legacy order), so re-grants stay 200 no-ops even after the role's membership is frozen (editUsers flipped to false)
- Single grant gate: role's editUsers flag false -> 403 (the only failure legacy has; the issue's hierarchy-409 case does not exist in code)
- Inherited-grant quirk documented in OpenAPI: membership inherits DOWN the role tree, so granting a role the user already inherits via a parent role is a no-op that still returns 200
- 404 via DoesNotExistException for missing role/user; auth via shared roles-portlet + CMS-admin gate
- Path template {roleid} matching the convention set by the sibling GET/PUT endpoints
- Typed immutable views (RoleUserGrantView with minimal user payload) returned directly per v2 REST patterns
- Truthful audit logging: success lines only on actual grant, error lines on failure (deviation from DWR, which always logged success)
@hassandotcms
#36938: feat(roles): add DELETE /v1/roles/{roleid}/users for bulk mem…
2425453 
…ber removal

Bulk-removes direct role memberships, replacing the DWR RoleAjax#removeUsersFromRole path for the Angular Roles & Tools portlet (epic #36909). DELETE with JSON body {userIds: [...]} — same pattern as the bulk category delete endpoint.

- Partial-success semantics: once the role resolves the batch never fails as a whole; response reports removedUserIds plus skipped entries with reasons not_found / inherited / error (constants on the view, single source of truth with the OpenAPI enum)
- skipped 'inherited' reports what legacy silently no-ops (0-row membership DELETE; the Dojo grid only offered checkboxes on direct rows)
- userIds entries validated non-null/non-blank -> 400 (Jackson accepts null elements in a Set<String>; unchecked they would 500 mid-batch)
- Direct-membership check per user via the role cache (loadRolesForUser) — scales with batch size, not role membership size, and reads at removal time
- Deliberately no batch-level transaction: partial success requires per-user commits (each roleAPI.removeRoleFromUser is already transactional); documented in the helper javadoc
- jdkOnly immutables so the view's list properties inline in the OpenAPI schema instead of binding to the shared ImmutableListString component (which would clobber other endpoints' docs)
- Path template {roleid} matching the convention set by the sibling GET/PUT endpoints
- Per-removal ActivityLogger/AdminLogger/SecurityLogger lines; error-path audit lines; missing role -> 404; auth via shared roles-portlet + CMS-admin gate
@hassandotcms
Merge remote-tracking branch 'upstream/main' into 36936-roles-api-put…
44970ef 
…-role-update

# Conflicts:
#	dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java
@hassandotcms hassandotcms mentioned this pull request Aug 17, 2026
Open
9 tasks
hassandotcms added a commit that referenced this pull request Aug 17, 2026
@hassandotcms
refactor(roles): use lowercase {roleid} path template on the users en…
9549595 
…dpoint

Aligns GET /{roleid}/users with the existing GET /{roleid}, PUT
/{roleid} (PR #37012, where review feedback settled on lowercase) and
the membership endpoints POST /{roleid}/users/{userId} and DELETE
/{roleid}/users (PR #37077). Without this, the regenerated openapi.yaml
would end up with /v1/roles/{roleId}/users and /v1/roles/{roleid}/users
as two entries for the same real path once both branches merge, which
OpenAPI treats as ambiguous.

Template rename only: real URLs, clients and tests are unaffected.
openapi.yaml regenerated.
@hassandotcms
hassandotcms marked this pull request as ready for review August 17, 2026 16:57
@hassandotcms
hassandotcms changed the base branch from main to 36936-roles-api-put-role-update August 17, 2026 19:32
@hassandotcms
Merge remote-tracking branch 'upstream/36936-roles-api-put-role-updat…
c770b9e 
…e' into 36937-36938-roles-api-role-membership
@claude

claude Bot commented Aug 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @hassandotcms's task in 2m 29s —— View job

Code Review — role-membership endpoints

Reviewed RoleHelper, RoleResource, RoleUsersForm, and the immutable views. The membership methods (addUserToRole, removeUsersFromRole) and their forms/views are well-structured, correctly gated behind initRequireRolesPortletAndCmsAdmin, audit-logged, and the legacy-parity semantics are clearly documented. Verified specific points:

  • RoleUsersForm.checkValid() — the null userIds case is safe: @NotNull is validated by super.checkValid() (a standard nullness constraint, not the collection constraint the comment flags as unreliable) before userIds.isEmpty() runs, so {}/{"userIds":null} → 400, not NPE. ✅
  • removeUsersFromRole direct-membership check uses loadRolesForUser(userId, false) → includeImplicitRoles=false, correctly matching only direct memberships so inherited holders are reported as inherited. ✅
  • Deliberate absence of @WrapInTransaction on the bulk remove (per-user commits for partial success) and its presence on the single grant are both correct. ✅
  • Partial-success error paths: NoSuchUserException → not_found, other per-user exceptions caught → error and logged; the batch never fails as a whole once the role resolves. ✅

New Issues

  • 🟡 Medium: RoleHelper.java:360 — removeUsersFromRole has no editUsers/system-role gate, unlike the grant path. addUserToRole (line 311) rejects editUsers=false roles with 403, but bulk-remove only resolves the role and then removes any direct membership — so an admin can strip direct members off a workflow/system role that disallows user grants. Assumption: legacy RoleAjax#removeUsersFromRole had no such gate and this is intended parity (removal is de-escalation). What to verify: confirm legacy imposed no gate here; if it did, mirror it, otherwise this is fine as-is and no change is needed.

Nothing blocking — the two prior review threads (@WrapInTransaction on updateRole/addUserToRole) were addressed by the author and the PR is already approved.
· 36937-36938-roles-api-role-membership

Base automatically changed from 36936-roles-api-put-role-update to main August 17, 2026 20:39
@hassandotcms
Merge remote-tracking branch 'upstream/main' into 36937-36938-roles-a…
b4cb1dc 
…pi-role-membership

# Conflicts:
#	dotCMS/src/main/java/com/dotcms/rest/api/v1/system/role/RoleHelper.java
#	dotCMS/src/main/java/com/dotcms/rest/api/v1/system/role/RoleResource.java
#	dotcms-integration/src/test/java/com/dotcms/rest/api/v1/system/role/RoleResourceIntegrationTest.java
@github-actions github-actions Bot added Area : Backend PR changes Java/Maven backend code and removed AI: Safe To Rollback labels Aug 18, 2026
@github-actions github-actions Bot mentioned this pull request Aug 18, 2026
Closed
6 tasks
@hassandotcms
#36939: refactor(roles): align DELETE path template with the {roleid}…
8ee3a62 
… convention

PUT and GET on the same segment already use {roleid} (aligned during PR #37012 review); this makes DELETE consistent so all /v1/roles/{roleid} operations share one path entry in the OpenAPI spec. URL shape is unchanged - template name only. Regenerated openapi.yaml.
@hassandotcms
hassandotcms enabled auto-merge August 18, 2026 21:22
@hassandotcms
hassandotcms added this pull request to the merge queue Aug 18, 2026
Merged via the queue into main with commit 51a1db2 Aug 19, 2026
68 checks passed
@hassandotcms
hassandotcms deleted the 36937-36938-roles-api-role-membership branch August 19, 2026 00:14
@hassandotcms hassandotcms mentioned this pull request Aug 19, 2026
Closed
@hassandotcms hassandotcms linked an issue Aug 19, 2026 that may be closed by this pull request
[Task] Roles API: add DELETE /v1/roles/{roleId} for role deletion #36939
Closed
7 tasks
This was referenced Aug 20, 2026
Closed
Closed
@hassandotcms hassandotcms mentioned this pull request Aug 20, 2026
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcastro-dotcms jcastro-dotcms jcastro-dotcms approved these changes

Assignees

No one assigned

Labels

Area : Backend PR changes Java/Maven backend code

Projects

dotCMS - Product Planning
Status: No status

Milestone

No milestone

2 participants

@hassandotcms @jcastro-dotcms