sec: enforce admin check on layout assignment and role grant endpoints

[mbiuki]

Summary

Fixes a privilege-escalation chain (reported via responsible disclosure) that allowed any authenticated backend user to reach CMS Administrator and subsequently achieve remote code execution via OSGi bundle upload.

Attack chain closed by this PR:

1. PUT /api/v1/toolgroups/{id}/_addtouser — any backend user could self-assign the admin Settings layout (containing the roles portlet) with no privilege check
2. POST /dwr/call/plaincall/RoleAjax.addUserToRole.dwr — with the roles portlet now accessible, the user could grant themselves the CMS Administrator role; the only gate was portlet-access, which step 1 satisfied
3. As CMS Administrator, attacker uploads a malicious OSGi bundle → arbitrary OS command execution

Changes

ToolGroupResource.java

• _addtouser: require loggedInUser.isAdmin() for all layouts except gettingstarted (the onboarding layout — intentionally self-assignable)
• _removefromuser: same admin guard applied symmetrically

RoleAjax.java

• addUserToRole: add caller.isAdmin() check after the existing portlet-access check; log unauthorized attempts via SecurityLogger
• removeUsersFromRole: same admin guard applied symmetrically

Test plan

• Verify a low-privilege backend user receives 403 on PUT /api/v1/toolgroups/{layout_id}/_addtouser (non-gettingStarted layout)
• Verify gettingStarted layout self-assignment still works for non-admin users (onboarding flow must not break)
• Verify a low-privilege backend user with roles-portlet access receives DotSecurityException on RoleAjax.addUserToRole via DWR
• Verify a CMS Administrator can still assign layouts and manage roles normally
• Run the researcher's PoC script — it must fail at step 1 and never reach role escalation

References

Closes dotCMS/private-issues#642

• CWE-269: Improper Privilege Management

🤖 Generated with Claude Code

[claude]

Claude finished @mbiuki's task in 2m 43s —— View job

---

Code Review — PR #36344

• Read PR diff (ToolGroupResource, RoleAjax, tests)
• Verify admin gates and self-service exemption logic
• Recheck prior findings (dotBot userid validation, dsilvam Getting Started regression)
• Review test coverage

---

I reviewed the diff against origin/main. This is a clean, well-scoped security fix. The guard logic is correct on all four endpoints and the tests exercise both the reject and allow paths plus the self-service boundary.

New Issues

No issues found.

The guard !isSelfServiceGettingStarted(layoutId, userid, loggedInUser) && !loggedInUser.isAdmin() throws unless the caller is either an admin or self-servicing the gettingstarted layout — which is exactly the intended allow-set. Verified:

• RoleAjax.addUserToRole/removeUsersFromRole (RoleAjax.java:287,322) — isAdmin() fires after validateRolesPortletPermissions but before any loadRoleById/loadUserById, so portlet access alone no longer grants role mutation and unauthorized callers are rejected before DB reads. SecurityLogger.logInfo records the attempt. isAdmin() resolves to the CMS Administrator role check (User.java:327), which is stricter than the old portlet-based getAdminUser() gate — the correct fix for step 2 of the chain.
• The isSelfServiceGettingStarted helper (ToolGroupResource.java:196) is equalsIgnoreCase on the layout id and requires userid == null || userid.equals(loggedInUser.getUserId()), so cross-user ?userid= assignment/removal still requires admin.

Resolved

• ✅ ToolGroupResource.java:124 — dotBot's "missing userid validation on gettingstarted assignment" is closed: the self-service check now rejects a non-admin passing another user's userid (test addToolGroupToUser_rejectsNonAdminAssigningGettingStartedToOtherUser).
• ✅ ToolGroupResource.java:79 — dsilvam's Getting Started dismiss regression is fixed: _removefromuser now mirrors the gettingstarted exemption, so a non-admin can dismiss their own starter page (test deleteToolGroupFromUser_allowsGettingStartedForNonAdmin).

Notes (non-blocking)

• The pre-existing getUsersByRole unauthenticated enumeration (RoleAjax.java:211) flagged in the earlier red-team review is not in this diff and remains open — worth a follow-up issue, but out of scope here.
• Test coverage is solid: 11 ToolGroupResourceTest cases + 2 RoleAjaxTest cases cover reject/allow, admin/non-admin, self vs. other-user, case-insensitivity, and the "no side effects on rejection" ordering guarantee.
  · sec/priv-esc-layout-role-auth

[github-actions]

🤖 dotBot Review (Bedrock)

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

Confirmed findings

• 🟡 Medium dotCMS/src/main/java/com/dotcms/rest/api/v1/portlet/ToolGroupResource.java:124 — Missing user ID validation in gettingstarted layout assignment
  The code allows non-admin users to assign the 'gettingstarted' layout to arbitrary user IDs via the userId parameter. While the admin check is bypassed for this layout type, there's no validation that the target userId matches the logged-in user's ID, enabling potential privilege escalation if the layout contains sensitive portlets.

---

_{us.deepseek.r1-v1:0 · Run: #28671877655 · tokens: in: 8448 · out: 2919 · total: 11367 · calls: 4 · est. ~$0.027}

[mbiuki]

This PR fixes https://github.com/dotCMS/private-issues/issues/642

[github-actions]

PRs linked to this issue

• sec: harden portlet-only gated methods in RoleAjax by @mbiuki

[mbiuki]

Versions Affected

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

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

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

[mbiuki]

Vulnerability Introduction Timeline

ToolGroupResource.addToolGroupToUser — admin check was never present

Date	Commit	Author	Event
2020-12-16	8aa87f7b87	Will Ezell	Method created without any admin check (PR #19680, issue #19581) — only requiredBackendUser(true) enforced
2021-03-22	890feffa94	Nollymar Longa	?userid= parameter added — widened exploit surface from self-assignment to assigning layouts to any user
2025-11-04	3babaf0d17	—	Partial portlet-level mitigation added (private-issues#482) — insufficient alone
PR #36344	—	—	Full fix: admin-only gate added — not yet in any release

Affected Versions

• First vulnerable release: 21.02 (December 2020 commit shipped in the Feb 2021 release cut)
• Widened attack surface (other-user assignment): ~21.03–21.04 (March 2021 commit)
• All affected: 21.02 → v26.06.22-03 (current latest), including all LTS lines:
  • 23.10 LTS ✗
  • 24.04 LTS ✗
  • 24.12 LTS ✗
  • 25.07 LTS ✗
• Partial mitigation from: v25.11.07-1 (still exploitable via chain with sec: enforce admin check on layout assignment and role grant endpoints #36344)
• Fully fixed in: not yet released

[mbiuki]

🔴 Red Team Review — PR #36344

Three independent reviewers examined this PR across security, performance, and test coverage.

---

Verdicts

Dimension	Verdict
🔐 Security	NEEDS_CHANGES
⚡ Performance	MINOR_IMPROVEMENT
🧪 Test Coverage	NEEDS_IMPROVEMENT

---

🔐 Security

The _addtouser admin guard is correctly implemented. However, three issues require remediation before merge.

🚨 Critical — _removefromuser has no admin guard

ToolGroupResource.java:66–92

The PR description states _removefromuser received the same admin guard symmetrically — this is false in current HEAD. The endpoint only requires requiredBackendUser(true). Any authenticated backend user can call PUT /api/v1/toolgroups/{layoutId}/_removefromuser?userid={anyUserId} to strip any layout from any user's role, including CMS Administrators.

Fix: Apply .requiredRoles(Role.CMS_ADMINISTRATOR_ROLE) to _removefromuser identically to _addtouser.

⚠️ High — addUserToRole / removeUsersFromRole use portlet-based check, not role-based

RoleAjax.java:282–345

getAdminUser() checks whether the user has a layout containing the "users" portlet — not whether they hold the CMS Administrator role. An attacker who obtains the "users" portlet via any other mechanism could bypass this guard. The correct check is isAdmin() → doesUserHaveRole(user, loadCMSAdminRole()), which is a direct role membership check. requiredRoles(CMS_ADMINISTRATOR_ROLE) (used correctly in _addtouser) is stricter than getAdminUser().

Additionally, both methods do DB reads (loadRoleById, loadUserById) before the admin guard fires — wasted work for unauthorized callers, inconsistent with the fail-fast pattern applied in saveRoleLayouts (PR #36345).

Fix: Replace getAdminUser() in both methods with an explicit isAdmin() check at the top of the method body, before any DB reads.

⚠️ High — SecurityLogger coverage removed

RoleAjax.java:282–345

Explicit SecurityLogger.logInfo() calls on unauthorized role-assignment attempts were present in an earlier commit but dropped in the current HEAD. The getAdminUser() fallback logs "unauthorized attempt to call getUserById" — a misleading message that won't surface in forensic searches for role-escalation incidents.

Fix: Add explicit log entries before throwing:

SecurityLogger.logInfo(getClass(),
    "Unauthorized attempt to assign role [roleId=" + roleId + "] by user " + caller.getUserId());

🔵 Low — gettingStarted self-assignment exception removed

The original commit allowed non-admin users to self-assign the onboarding gettingStarted layout. The current HEAD applies requiredRoles(CMS_ADMINISTRATOR_ROLE) to all layouts uniformly, breaking the documented onboarding flow. This is an over-restriction, not a security regression — but needs product validation.

ℹ️ Info (pre-existing) — getUsersByRole has no auth guard

RoleAjax.java:211 — any session-authenticated user can enumerate all users in any role including CMS Admin. Not introduced by this PR.

---

⚡ Performance

Net positive change.

• _addtouser guard fires inside InitBuilder.init() before any layout/user DB reads — correct fail-fast ordering
• isAdmin() and getAdminUser() both resolve from RoleCache on the warm path — negligible overhead
• Pre-existing anti-pattern in addUserToRole: loadRoleById + loadUserById fire before the admin check — not introduced by this PR
• Minor redundancy: WebResource.init() calls both checkAdminPermissions and checkRolePermissions for the same CMS Admin role check — both are cache hits, pre-existing issue

---

🧪 Test Coverage

Scenario	Status
Non-admin blocked on _addtouser	✅
Admin succeeds on _addtouser	✅
Non-admin blocked on addUserToRole (with portlet access)	✅
gettingStarted non-admin self-assignment allowed	❌ Missing
Non-admin blocked on _removefromuser	❌ Missing
Admin succeeds on _removefromuser	❌ Missing
Non-admin blocked on removeUsersFromRole	❌ Missing
Admin succeeds on addUserToRole / removeUsersFromRole	❌ Missing
Cross-user ?userid= assignment blocked for non-admin	❌ Missing
SecurityLogger invoked on unauthorized attempts	❌ Missing

Recommended additions:

// ToolGroupResourceTest
test_removeToolGroupFromUser_lowPrivilegeUser_throwsSecurityException()
test_removeToolGroupFromUser_adminUser_succeeds()
test_addToolGroupToUser_gettingStartedLayout_nonAdmin_succeeds()  // if exception is restored
test_addToolGroupToUser_crossUserAssignment_lowPrivilegeUser_throwsSecurityException()

// RoleAjaxSecurityTest
test_removeUsersFromRole_lowPrivilegeUser_throwsSecurityException()
test_addUserToRole_adminUser_succeeds()
test_removeUsersFromRole_adminUser_succeeds()

---

Summary

The _addtouser fix is correct and closes Step 1 of the escalation chain. Three blockers must be resolved before merge:

1. 🚨 Add admin guard to _removefromuser
2. ⚠️ Replace getAdminUser() with isAdmin() in addUserToRole + removeUsersFromRole
3. ⚠️ Restore SecurityLogger with accurate action descriptions on unauthorized role-assignment attempts

[mbiuki]

Correction to the Red Team Review above

After reading the actual PR commit (4d0d4977ff), the three blockers I flagged are already addressed:

1. ✅ _removefromuser — isAdmin() check is present (added by this PR)
2. ✅ addUserToRole / removeUsersFromRole — isAdmin() check + SecurityLogger are both present (added by this PR)
3. ✅ SecurityLogger — accurate log messages on unauthorized attempts are already in place

The review was based on pre-PR baseline code rather than the PR branch — apologies for the noise.

One real issue was found and fixed: both _removefromuser and _addtouser were calling loadUserById() before the isAdmin() check, so a non-admin caller with a ?userid= param triggered a needless DB read before being rejected. Commit ff86ac22f7 reorders those two blocks so the admin check fires first.

[mbiuki]

✅ Test Results

Added unit tests for all four admin gates in this PR (commit 61527b0) and ran them locally:

Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS

com.dotcms.rest.api.v1.portlet.ToolGroupResourceTest   6/6 ✅
com.dotmarketing.business.ajax.RoleAjaxTest            2/2 ✅

Test	Verifies
addUserToRole_rejectsNonAdmin	Non-admin with roles-portlet permission still gets DotSecurityException
removeUsersFromRole_rejectsNonAdmin	Same for role removal
deleteToolGroupFromUser_rejectsNonAdmin	Non-admin cannot remove layouts
addToolGroupToUser_rejectsNonAdmin	Non-admin cannot assign layouts
addToolGroupToUser_allowsGettingStartedForNonAdmin	The gettingstarted exemption still works (case-insensitive) and the layout is actually added
addToolGroupToUser_allowsAdmin / deleteToolGroupFromUser_allowsAdmin	Admins pass the gates with the expected LayoutAPI/RoleAPI side effects
deleteToolGroupFromUser_nonAdminCausesNoSideEffects	The rejection happens before any role mutation

Run locally with: ./mvnw test -pl :dotcms-core -Dtest="ToolGroupResourceTest,RoleAjaxTest"

Also: the link-issue check now passes on this PR since #36458 merged. 🎉

🤖 Generated with Claude Code

[dsilvam]

Findings

🔴 ToolGroupResource.java:79 — _removefromuser gains an unconditional admin gate, but _addtouser exempts gettingstarted; this breaks the non-admin "Getting Started" toggle.

The add path is guarded as !layoutId.equalsIgnoreCase("gettingstarted") && !loggedInUser.isAdmin(), deliberately letting non-admins self-assign the onboarding layout. The remove path, however, is if (!loggedInUser.isAdmin()) throw ... with no
gettingstarted exemption.

Two non-admin-reachable call sites hit _removefromuser/gettingstarted via DotAccountService.removeStarterPage():

• dot-my-account.component.ts:209 — the My Account dialog (available to every backend user) toggling "Show Getting Started" off.
• onboarding-author.component.ts:108 — the Getting Started portlet's own hide/dismiss toggle.

Failure scenario: A non-admin backend user enables the Getting Started page (succeeds — add exempts gettingstarted), then toggles it off. deleteToolGroupFromUser throws DotSecurityException → the httpErrorManagerService catch in removeStarterPage()
surfaces an error notification, and the layout is never removed. The user can add the starter page but can never dismiss it. Fix: mirror the add guard — exempt gettingstarted from the admin check in deleteToolGroupFromUser.

🟠 ToolGroupResourceTest.java:132 — the new test codifies the regression rather than catching it. deleteToolGroupFromUser_rejectsNonAdmin asserts a non-admin is rejected for "someLayout", but there is no test for a non-admin removing gettingstarted (the
symmetric counterpart to addToolGroupToUser_allowsGettingStartedForNonAdmin). Adding that case would have exposed the asymmetry above; once the fix lands, it should assert removal succeeds for gettingstarted.

[mbiuki]

@dsilvam you're right — validated and fixed in 0edd733. Thank you, this was a real regression.

Confirmed your analysis end to end:

• _addtouser exempted gettingstarted (line 124) but _removefromuser had an unconditional !isAdmin() gate (line 79).
• dot-account-service.ts maps removeStarterPage() → /toolgroups/gettingstarted/_removefromuser, called from dot-my-account.component.ts:209 and onboarding-author.component.ts:108 — both reachable by any backend user. A non-admin could enable the Getting Started page but not dismiss it.

Fix: mirrored the exemption on the remove path. I also tightened it a bit beyond a pure mirror — the exemption is now self-service only (userid null or your own), so assigning/removing gettingstarted for another user still requires admin. That closes the dotBot "missing userid validation" finding on the add path in the same change. Extracted into a shared isSelfServiceGettingStarted(layoutId, userid, loggedInUser) helper.

Tests (your second point — the suite now covers the symmetry):

• deleteToolGroupFromUser_allowsGettingStartedForNonAdmin — non-admin can dismiss their own starter page (the case that would have caught this)
• deleteToolGroupFromUser_rejectsNonAdminRemovingGettingStartedFromOtherUser — self-service boundary on remove
• addToolGroupToUser_rejectsNonAdminAssigningGettingStartedToOtherUser — self-service boundary on add

Tests run: 11, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS

🤖 Generated with Claude Code

[mergify]

Queued — the merge queue status continues in this comment ↓.

[mergify]

Merge Queue Status

• ✅ Entered queue — 2026-07-15 15:38 UTC · Rule: default · triggered by @mbiuki with the merge queue checkbox
• ✅ Checks skipped · PR is already up-to-date
• 🚫 Left the queue — 2026-07-15 15:39 UTC · at 79abba7325a43d986d3414597eb670df2f8a0f12

This pull request spent 1 minute 20 seconds in the queue, including 5 seconds running CI.

Required conditions to merge

• #review-threads-unresolved = 0 [🛡 GitHub branch protection]
  • sec: enforce admin check on layout assignment and role grant endpoints #36344
• github-review-approved [🛡 GitHub branch protection]
  • sec: enforce admin check on layout assignment and role grant endpoints #36344
• github-review-decision = APPROVED [🛡 GitHub branch protection]
  • sec: enforce admin check on layout assignment and role grant endpoints #36344
• any of [🛡 GitHub branch protection]:
  • check-success = Finalize / Final Status
  • check-neutral = Finalize / Final Status
  • check-skipped = Finalize / Final Status
• any of [🛡 GitHub branch protection]:
  • check-success = Initialize / Initialize
  • check-neutral = Initialize / Initialize
  • check-skipped = Initialize / Initialize

Reason

Pull request #36344 has been dequeued

GitHub refused to merge the pull request. Pull Request is in the merge queue. This is usually enforced by a branch protection or ruleset rule.

Hint

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

• Requeue this pull request