[mbiuki]

Summary

Follow-on to #36344. The RoleAjax DWR methods that change role layouts, layouts and role permissions now require a CMS Administrator, the same bar addUserToRole / removeUsersFromRole already use. Rejected calls are written to the security log.

Method	Before	After
saveRolePermission	Roles portlet only	Roles portlet + CMS Administrator
saveRoleLayouts	Roles portlet + Users portlet (getAdminUser()), checked after two DB reads	Roles portlet + CMS Administrator, checked before any DB read
updateLayout	Roles portlet + Users portlet (getAdminUser())	Roles portlet + CMS Administrator
deleteLayout	Roles portlet + Users portlet (getAdminUser())	Roles portlet + CMS Administrator
addUserToRole / removeUsersFromRole	Roles portlet + CMS Administrator (inline, from #36344)	Same behaviour, now through the shared helper

getAdminUser() checks PortletAPI.hasUserAdminRights, which is Users portlet access, not CMS Administrator, so it is no longer used as the gate for these four methods.

Changes

RoleAjax.java (the only production change)

• New private validateCMSAdmin(action): Roles portlet check, then isAdmin(); a non-admin caller gets a SecurityLogger entry naming the operation and user, and a DotSecurityException.
• Used by saveRoleLayouts, updateLayout, deleteLayout and saveRolePermission, and replaces the inline copies in addUserToRole / removeUsersFromRole.

Behaviour change

Non-admin users who hold the Roles and Users portlets could previously save role layouts and update or delete layouts, and anyone with the Roles portlet could save role permissions. They now get DotSecurityException. CMS Administrators are unaffected. The only callers of these four DWR methods are the legacy Roles & Tools screens (view_roles_js_inc.jsp, view_role_permissions_js_inc.jsp).

Scope

• ToolGroupResource is intentionally not changed. An earlier revision added .requiredRoles(CMS_ADMINISTRATOR_ROLE) to addToolGroupToUser; it was reverted because requiredRoles runs before the method body and would block non-admin Getting Started onboarding (PUT /api/v1/toolgroups/gettingstarted/_addtouser). Those endpoints are handled by sec: enforce admin check on layout assignment and role grant endpoints #36344. ToolGroupResource matches main.
• The other getAdminUser() callers (addNewRole, updateRole, deleteRole, lockRole, unlockRole, addNewLayout) are not changed here.

Tests

RoleAjaxSecurityTest (integration, MainSuite2b) runs as a non-admin backend user holding both the Roles and the Users portlets, so the Users portlet check would pass and only the CMS Administrator check can reject the call:

• test_addUserToRole_nonAdmin_throwsDotSecurityException
• test_saveRolePermission_nonAdmin_throwsDotSecurityException
• test_saveRoleLayouts_nonAdmin_throwsDotSecurityException
• test_updateLayout_nonAdmin_throwsDotSecurityException
• test_deleteLayout_nonAdmin_throwsDotSecurityException

RoleAjaxTest (unit) adds *_rejectsNonAdmin for the four methods, asserting DotSecurityException and a SecurityLogger entry. Run locally: 7/7 pass, and the four new tests fail against the previous getAdminUser() gate. The system-user DWR callers in LayoutAPITest / PermissionAPITest are unaffected: the system user always holds the CMS Administrator role.

Test plan

• Non-admin with Roles + Users portlet access gets DotSecurityException from saveRolePermission, saveRoleLayouts, updateLayout and deleteLayout via DWR, with an entry in the security log
• CMS Administrator can save role layouts, update and delete layouts, and save role permissions in Roles & Tools
• Getting Started onboarding (_addtouser / _removefromuser of gettingstarted) still works
• LayoutAPITest and PermissionAPITest still pass in MainSuite 2b

References

• Closes: dotCMS/private-issues#643
• Related: sec: enforce admin check on layout assignment and role grant endpoints #36344

🤖 Generated with Claude Code

[claude]

Claude finished @mbiuki's task in 1m 44s —— View job

---

SDK Compatibility Check

• Read docs/core/SDK_BREAKING_CHANGE_CATEGORIES.md
• Diffed c444a240...fbf894f0 (full PR range)
• Checked diff against every category (G-1/G-2/G-3, R-1, U-1/U-2, H-1)
• Determination: no SDK breaking change

Scope of the diff: all four changed files are server-side Java/integration-test code —
dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java (new private
validateCMSAdmin(action) gate used by saveRolePermission, saveRoleLayouts, updateLayout,
deleteLayout, addUserToRole, removeUsersFromRole), dotCMS/src/test/java/.../RoleAjaxTest.java,
dotcms-integration/src/test/java/com/dotcms/MainSuite2b.java, and the new
dotcms-integration/src/test/java/.../RoleAjaxSecurityTest.java.

None of it touches an SDK-reachable surface:

• No GraphQL schema/resolver changes (graphql.page / graphql.content) — G-1/G-2/G-3 n/a.
• No /api/v1/nav, /api/v1/content, or /api/v1/page/* response shape changes — R-1 n/a.
• No UVE postMessage protocol changes (__DOTCMS_UVE_EVENT__ / DotCMSUVEAction) — U-1/U-2 n/a.
• No changes to SdkVersionWebInterceptor, X-DotCMS-Version/X-DotCMS-Min-SDK, or
  compareVersions()/sdk-compatibility.ts — H-1 n/a.

The only runtime-reachable surface here is the legacy DWR endpoint RoleAjax, which backs the
Roles & Tools JSPs (view_roles_js_inc.jsp, view_role_permissions_js_inc.jsp), not any
@dotcms/* SDK. This matches the "admin-UI only change" / "internal refactor, no wire-format
change" non-breaking examples in the reference doc.

No PR comment or SDK Breaking Change label was added, per the check's instructions for a
non-breaking result.

[github-actions]

🤖 dotBot Review (Bedrock)

Reviewed 5 file(s); 3 candidate(s) → 2 confirmed, 0 uncertain (unverified, kept for review).

Confirmed findings

• 🔴 Critical dotCMS/src/main/java/com/dotmarketing/business/ajax/RoleAjax.java:573 — Admin check bypass in role layout updates
  Removal of 'User user = getAdminUser()' assignment causes subsequent permission check to use logged-in user instead of admin, allowing non-admins with roles portlet access to modify role layouts. The permission check at line 573 now references an undefined 'user' variable, which likely resolves to the current user rather than an admin.
• 🟡 Medium dotcms-integration/src/test/java/com/dotcms/rest/api/v1/portlet/ToolGroupResourceTest.java:60 — Silent exception swallowing in test cleanup
  In ToolGroupResourceTest.java line 60, exceptions during test role cleanup are caught but not logged or reported. This could mask failures in test teardown that might lead to data leaks between tests. While test cleanup is often best-effort, silent swallowing makes debugging harder when dependencies exist between tests.

---

_{us.deepseek.r1-v1:0 · Run: #28672054880 · tokens: in: 21925 · out: 6849 · total: 28774 · calls: 10 · est. ~$0.067}

[mbiuki]

Versions Affected

Exploitable range: v21.02 → v26.06.22-03 (current latest)

Gap	Introduced	First vulnerable release
RoleAjax.saveRolePermission (portlet-only gate, no admin check)	March 2012	v3.0
ToolGroupResource._addtouser (enabling step)	December 2020	v21.02

No patched release has been cut yet — this PR is part of the fix.

[mbiuki]

CI Re-run Results ✅

All checks passed on the re-run (run #28331839580).

The previous failures (QuartzUtils DB deadlock, StoryBlockMap NPE, E2E/Postman timeouts) were confirmed as pre-existing flaky infrastructure issues unrelated to this PR's changes.

This PR is clear to merge.

Suite	Result
Integration Tests - MainSuite 1a	✅ pass
Integration Tests - MainSuite 1b	✅ pass
Integration Tests - MainSuite 2a/2b/3a	✅ pass
Integration Tests - Junit5 Suite 1	✅ pass
Integration Tests - OpenSearch Upgrade	✅ pass
E2E Tests - Nx Playwright	✅ pass
Postman Tests - GraphQL	✅ pass
All other Postman/Karate/CLI/JVM suites	✅ pass

[wezell]

@mbiuki @sfreudenthaler we should do a pass over all the DWR endpoints to make sure there are no other vulnerabilities. These all predate any of our security hardening efforts. Here is a list of these endpoints.

https://github.com/dotCMS/core/blob/main/dotCMS/src/main/webapp/WEB-INF/dwr.xml

[github-actions]

⚠️ AI review failed

Claude did not produce a review — the backend call errored before generating any output (provider: anthropic-bedrock, model: global.anthropic.claude-sonnet-5). This usually means the model has no Bedrock access grant in the target account, or the model ID is invalid — not a problem with this PR.

_{Run: #37488524063}

[github-actions]

⚠️ AI review failed

Claude did not produce a review — the backend call errored before generating any output (provider: anthropic-bedrock, model: global.anthropic.claude-sonnet-5). This usually means the model has no Bedrock access grant in the target account, or the model ID is invalid — not a problem with this PR.

_{Run: #37507833153}

[ihoffmann-dot]

Approved ✅

This hardening change looks good.

What it does

• Restores the admin-only check on saveRolePermission, which had been dropped by mistake. Users with only roles-portlet access can no longer change role permissions.
• Applies the same admin check in saveRoleLayouts before anything is loaded, so unauthorized callers are rejected right away.

Why I'm comfortable approving

• It matches how the other role-modifying endpoints in RoleAjax are already protected.
• The change is small and contained, with no side effects on the rest of the class.
• The new RoleAjaxSecurityTest covers the three affected paths with a non-admin user who only has roles-portlet access, and it is registered in MainSuite2b, so it runs in CI.
• The test cleans up after itself, so it won't interfere with other tests in the suite.

Non-blocking note: the read-only role endpoints (e.g. getRole, getUserRole, getRolePermissions) still use the portlet-only check. That looks intentional and is outside the scope of this PR.

[mbiuki]

@oidacra review again

[mbiuki]

@ihoffmann-dot review again

[swicken]

Pulled the branch and ran RoleAjaxTest locally: 7 of 7 green. The new helper runs before any of the four methods read or write anything, the REST counterparts in RoleResource and LayoutResource already apply the same CMS Administrator bar, and the only DWR callers are the two Roles & Tools JSPs, as the description says. A merge-tree against current main is clean; of the four files only MainSuite2b has moved on main, in unrelated lines, so the rebase is straightforward.

Two things in the integration test worth fixing before merge, and a couple of nits, on the lines below. I'll follow up separately on the PR wording and history before this goes in.

[swicken]

Approving. The administrator check is in the right place and the behaviour change is documented; the two test comments above are suggestions, not blockers. When merging, please squash with a single plain commit message rather than the default concatenation.

[swicken]

Re-reviewed at fbf894f. The rejection tests now target a throwaway role and layout, the class saves the DWR context that was attached before it ran and puts that back afterwards, refusals log at WARN with the remote host, and the new administrator case actually exercises update and delete rather than only asserting that nothing was thrown. The merge from main kept main's additions to MainSuite2b, and the only production change since my last pass is the log line in validateCMSAdmin. Approving again.