fix(permissions): enforce contentlet-level WRITE check on Copy and Edit (#34215)

[gortiz-dotcms]

Problem

When a user only has View permission on a contentlet instance, they could still use the Copy and Edit button in the Page Edit screen to create a copy of that contentlet. Additionally, a user with Read-only access on a page could call the _deepcopy endpoint and receive a fully editable duplicate of that page — same class of bug, different object.

Both code paths called contentletAPI.copyContentlet() directly, which only enforces READ, not WRITE.

Root Cause

1. Copy and Edit (contentlet): PageResourceHelper.copyContent() fetched the source contentlet and immediately invoked contentletAPI.copyContentlet() without verifying the user had WRITE permission on the specific contentlet instance. Permission was only evaluated at the content type level.

2. Deep copy (page): PageResourceHelper.copyPage() called contentletAPI.copyContentlet(page, ...) without a WRITE check on the page itself. A READ-only user could create a page copy before the child-contentlet WRITE checks caused a rollback — wrong failure mode, wrong place.

Fix

Added explicit permissionAPI.doesUserHavePermission(..., PERMISSION_WRITE, ...) checks in both code paths before any copy is attempted. Both throw DotSecurityException (→ 403) if the user lacks WRITE on the instance.

Changes

• dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResourceHelper.java
  • copyContent() — added instance-level WRITE check before copying a contentlet (Copy and Edit)
  • copyPage() — added WRITE check on the page before deep-copying it

Test Plan

• Log in as a user with View-only on a contentlet but Publish on the content type → "Copy and Edit" should return 403
• Log in as a user with WRITE on the contentlet → "Copy and Edit" should succeed as before
• Log in as a user with Read-only on a page → PUT /page/{pageId}/_deepcopy should return 403
• Log in as a user with WRITE on the page → deep copy should succeed as before
• Run: ./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=PageResourceTest

Fixes: #34215

This PR fixes: #34215

[claude]

Claude finished @gortiz-dotcms's task in 12m 32s —— View job

---

🔍 dotCMS Backend Review

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResourceHelper.java:640 (inside copyContent(), invoked once per MultiTree from the loop in copyPage(), lines 341–356)

copyPage() iterates every MultiTree on the page and, for each one, now runs an additional scalar permissionAPI.doesUserHavePermission(currentContentlet, PERMISSION_WRITE, user, ...) call — all inside the single @WrapInTransaction connection opened by copyPage(). Since each iteration checks a different contentlet identifier, cache hits are unlikely, so this adds one extra DB round trip per placed contentlet held open inside the page-copy transaction — the per-item doesUserHavePermission loop pattern this repo's own conventions call out in favor of the batch permissionAPI.filterCollection(...) overload. Non-blocking: the loop already performs per-item findContentletByIdentifier, copyContentlet, and two MultiTree writes regardless of this change, so this doesn't change the transaction's asymptotic shape, and page content counts are typically small.

if (!permissionAPI.doesUserHavePermission(currentContentlet, PermissionAPI.PERMISSION_WRITE, user, pageMode.respectAnonPerms)) {
    throw new DotSecurityException(...);
}

💡 Optional: before the for (final MultiTree multiTree : multiTrees) loop in copyPage(), resolve the referenced contentlets and run one batched permissionAPI.filterCollection(contentlets, PermissionAPI.PERMISSION_WRITE, user, pageMode.respectAnonPerms) call instead of re-checking per item inside copyContent().

---

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResourceHelper.java:332-335 and dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResourceHelper.java:640-643

The two new permission checks are near-identical (same condition shape, same exception type, same message template, differing only by "Page"/"Contentlet" and the target object). Worth extracting into a shared private helper to avoid message-format drift and give a single place to adjust the permission logic later.

if (!permissionAPI.doesUserHavePermission(HTMLPageAsset.class.cast(page), PermissionAPI.PERMISSION_WRITE, user, pageMode.respectAnonPerms)) {
    throw new DotSecurityException(String.format("User '%s' does not have WRITE permission on Page '%s'",
            user.getUserId(), page.getIdentifier()));
}

💡 Extract a private helper, e.g. checkWritePermission(Permissionable permissionable, String entityLabel, String identifier, User user, boolean respectAnonPerms), and call it from both sites.

---

Next steps

• 🟡 Both findings above are non-blocking cleanup/perf notes — you can ask me to handle them inline: @claude extract the duplicated WRITE-permission-check into a shared helper in PageResourceHelper.java
• Every new push updates this comment automatically

[claude]

🔍 dotCMS Backend Review

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResourceHelper.java:342-356 and :640-643

copyPage iterates every MultiTree on the page and calls copyContentlet(...) → copyContent() per entry. The new WRITE check added in this PR now runs a scalar permissionAPI.doesUserHavePermission(...) call once per iteration, inside the single outer @WrapInTransaction started by copyPage. For a page with N placed contentlets, that's up to N additional permission lookups (cache-miss triggers a DB round trip) held open inside the same transaction/connection — the per-item doesUserHavePermission loop pattern this repo's own CLAUDE.md guidance calls out to avoid in favor of permissionAPI.filterCollection(...).

for (final MultiTree multiTree : multiTrees) {
    this.copyContentlet(new CopyContentletForm.Builder()...build(), user, pageMode, language);
    // -> copyContent() -> doesUserHavePermission(currentContentlet, WRITE, ...) each time
}

💡 This is a pre-existing per-item-cost loop pattern (each iteration already does findContentletByIdentifier, checkin, multitree writes), so it's non-blocking. If page-copy performance for large pages becomes a concern, consider batch-checking WRITE permission for the referenced contentlets up front rather than one scalar check per loop iteration — note the batch check would need to fail-fast on the first unauthorized item rather than silently filtering, since a security violation must still throw DotSecurityException.

---

Next steps

• 🟡 Non-blocking cleanup/perf note — you can ask me to handle it inline: @claude batch the per-item WRITE permission check in PageResourceHelper.copyPage into a single up-front check
• Every new push updates this comment automatically

[claude]

🚫 Critical or High severity findings must be resolved before merging. See the review comment above for details.

[claude]

✅ No Critical or High severity issues found.

[mergify]

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

• Queue this pull request

[dsilvam]

@gortiz-dotcms is it possible to include a test here?

[claude]

✅ No Critical or High severity issues found.

[claude]

✅ No Critical or High severity issues found.