fix(categories): allow reparenting existing categories via REST API (#33989)

[dsilvam]

Proposed Changes

• Fix CategoryAPIImpl.save(...) so PUT /api/v1/categories actually re-parents an existing category. Previously the parent/child tree relationship was only written for brand-new categories (gated by isANewCategory), so a supplied "parent" was silently ignored on updates and the category stayed under its original parent.
• When a differing parent is supplied for an existing category, the category is detached from its previous parent(s) and linked to the new one (removeParent + addChild). An omitted/null parent leaves the current relationship untouched, so an update never accidentally detaches a category.
• The move is guarded by an EDIT (add-children) permission check on the target parent.
• Adds integration test CategoryAPITest#reParentExistingCategory and a Postman regression folder Reparent (issue #33989) in the Category collection.

Root cause

CategoryAPIImpl.save wired the tree relationship only inside if (isANewCategory && parent != null). For an update, isANewCategory is false, so addChild was never called and no existing link was rewritten — the category row updated but its tree position did not.

Checklist

• Tests
• Translations
• Security Implications Contemplated (add notes if applicable)

Additional Info

Security note: re-parenting now requires PERMISSION_EDIT on the target parent, mirroring the create-time check. Permissions on the moved category itself are intentionally left untouched (no copyPermissions on move) to avoid silently overwriting an existing permission set — flag if inheritance-on-move is the desired product behavior.

Behavior change: PUT /api/v1/categories now actually moves categories where it previously ignored parent. Clients sending a stale parent on update will now relocate the category.

Out of scope: moving a category to top level via an explicit-null parent — the API cannot currently distinguish "omitted parent" from "make top-level" (both arrive as null); would need a dedicated flag on CategoryForm (cf. the activeProvided() pattern from #35501).

Verification

• CategoryAPITest#reParentExistingCategory — passed (isolated core build).
• Postman Category → Reparent (issue #33989) run against a live dotCMS: 10 requests / 16 assertions, 0 failures — confirms the child moves under the new parent and is removed from the original.

Refs: #33989

This PR fixes: #33989

[claude]

Claude finished @dsilvam's task in 6m 28s —— View job

---

🔍 dotCMS Backend Review

Re-reviewed after the latest commit (f7348e62 — source-parent permission + cycle guard). The two 🟠 High findings from the prior review are now resolved. Remaining items are non-blocking.

New Issues

• 🟡 Medium: CategoryAPIImpl.java:284-296 — Multi-parent categories are silently collapsed to a single parent on update. applyReParent detaches the category from every current parent except newParent. dotCMS's category tree permits a category to sit under more than one parent (getParents returns a List). Previously an update never touched the tree, so a category under parents X and Y stayed under both. Now any PUT that supplies parent=X will detach it from Y. Clients that echo back only one parent in the update form will unintentionally sever the others.

  • Assumption: multi-parented categories exist in practice (the schema allows them).
  • What to verify: whether the product intends "move" to mean "detach from all other parents", or only "add/relocate under the supplied parent". If the former, this is by-design — worth a comment in the code and the API docs. Fix this →

• 🟡 Medium: dotcms-postman/.../Category.postman_collection.json:576-648 — Cleanup leaves the moved child category orphaned. The Reparent | Cleanup step deletes only reparentNewParentId and reparentOriginalParentId. categoryFactory.delete removes the parents' tree rows and permissions but does not cascade-delete child category rows, so reparentChildId (reparent-child / var reparentChild) survives every run. On a re-run the fixed key/categoryVelocityVarName collide, so the create step may fail. Add the child inode to the delete batch. Fix this →

Existing

• 🟡 Medium: CategoriesResource.java:539-543 — The @Operation description for updateCategory still doesn't document the new re-parent semantics (supplying a different parent moves the category, subject to EDIT on the new parent; omitting parent leaves the relationship untouched). Per repo convention, behavior changes belong in the annotation, and openapi.yaml should be regenerated.
• 🟡 Medium: CategoryHelper.java:54-66 — toCategoryView / toCategoryWithChildCountView still never populate CategoryView.parent, so a PUT response gives no way to confirm which parent the category ended up under. (Pre-existing, but more relevant now that PUT moves categories.)
• 🟡 Medium: CategoryAPIImpl.java:285-290 — applyReParent issues one removeParent (SELECT + DELETE) per old parent rather than a single batched tree delete. Negligible for the common single-parent case; only matters for many-parent categories.

Resolved

• ✅ CategoryAPIImpl.java:254-264 — EDIT permission is now enforced on each current parent being detached, not just the new parent (validateReParent).
• ✅ CategoryAPIImpl.java:233-239, 309-329 — Cycle guard (wouldCreateCycle, ancestor walk with a visited set) rejects moving a category under itself or a descendant; covered by reParentRejectsCycles.
• ✅ CategoryAPIImpl.java:175-180 — The reparent validation (permissions + cycle) now runs before categoryFactory.save, so a denied/cyclic request no longer pays for a DB write + cluster-wide cache flush.
• ✅ CategoriesResource.java:790-793 — A supplied-but-unknown parent inode now throws DoesNotExistException (400/404) instead of silently returning 200 with no move.

The core logic is sound and the two blocking security findings are addressed. Remaining items are documentation/test-hygiene and one behavioral clarification — none block merge.
· issue-33989-category-reparenting

[github-actions]

🤖 dotBot Review (Bedrock)

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

Confirmed findings

• 🟠 High dotCMS/src/main/java/com/dotmarketing/portlets/categories/business/CategoryAPIImpl.java:200 — Missing EDIT permission checks on original parents during category detachment
  The code removes the category from current parents without checking EDIT permission on those parents, allowing unauthorized users to detach categories from parents they can't modify. The current implementation checks permissions on the target parent but neglects the original parents, violating access control requirements.
• 🟡 Medium dotcms-postman/src/main/resources/postman/Category.postman_collection.json:644 — Test cleanup leaves child category undeleted
  The Postman test's cleanup step deletes parent categories 'Parent 1' and 'Parent 2' but does not delete the moved child category created during testing. This leaves orphaned categories that could cause ID collisions or state pollution in subsequent test runs.

---

_{us.deepseek.r1-v1:0 · Run: #28815147223 · tokens: in: 24132 · out: 5231 · total: 29363 · calls: 8 · est. ~$0.061}

[claude]

🔍 dotCMS Backend Review

🟠 High dotCMS/src/main/java/com/dotmarketing/portlets/categories/business/CategoryAPIImpl.java:223-228

reParent() detaches the category from every current parent it no longer belongs to, but only checks PERMISSION_EDIT on the new parent — never on the original parent(s) being modified. Prior to this PR, an existing category's tree position was immutable, so no code path could sever a category from a parent without permission on that parent. This is the first path that restructures the shared category tree using the child-oriented removeParent permission convention (EDIT-on-child only), but that convention was previously only used for tagging content with categories (Categorizable child = Contentlet), a self-service operation on the caller's own content. Here it's being applied to mutate another category's children set. A user with EDIT only on category X (no permission on P1, X's current parent) can call PUT /api/v1/categories with inode=X, parent=P2 (a parent they do control) and silently remove X from P1's subtree with zero authorization check on P1. The correct analogy is removeChild(parent, child, ...), which requires EDIT on the parent being modified (line 498) — not removeParent.

for (final Category currentParent : currentParents) {
    if (!newParent.getInode().equals(currentParent.getInode())) {
        categoryFactory.removeParent(category, currentParent);
    }
}

💡 Require permissionAPI.doesUserHavePermission(currentParent, PermissionAPI.PERMISSION_EDIT, user, respectFrontendRoles) for each currentParent before detaching, throwing DotSecurityException if not satisfied — mirroring removeChild's parent-oriented check.

---

🟠 High dotCMS/src/main/java/com/dotmarketing/portlets/categories/business/CategoryAPIImpl.java:200-234

reParent() never validates that newParent isn't category itself or a descendant of category. Before this PR the category tree was structurally guaranteed acyclic (parent links were set once, at creation). This method removes that guarantee: a user with EDIT on two categories they control can re-parent an ancestor under its own descendant, creating a cycle. Several tree-traversal routines used broadly across category browsing, content-type category fields, and permission filtering (CategoryFactoryImpl.getAllChildren, CategoryAPIImpl.isParent, getCategoryTree) have no cycle protection and will loop forever or stack-overflow once a cycle exists — a low-privilege, authenticated-only DoS against category functionality platform-wide.

private void reParent(final Category category, final Category newParent, final User user,
        final boolean respectFrontendRoles) throws DotDataException, DotSecurityException {
    final List<Category> currentParents = categoryFactory.getParents(category);
    // no check that newParent != category or that category is an ancestor of newParent

💡 Reject the move if newParent equals category or category is an ancestor of newParent (e.g. via isParent(newParent, category, user, respectFrontendRoles)), throwing on a cyclic move.

---

🟡 Medium dotCMS/src/main/java/com/dotmarketing/portlets/categories/business/CategoryAPIImpl.java:171-172,213-221

The EDIT-on-newParent permission check runs only inside reParent, which is called after categoryFactory.save(category, parent) has already executed the UPDATE and triggered a full catCache.clearCache(). Unlike the new-category branch (permissions validated at lines 134-158 before any write), a denied reparent attempt still pays for a DB write (rolled back by @WrapInTransaction, so no data-integrity issue) and an unconditional cluster-wide cache flush. A client repeatedly attempting an unauthorized reparent can cheaply force repeated full category-cache invalidations.

category.setModDate(new Date());
categoryFactory.save(category, parent);   // update + full catCache.clearCache()
...
if (!permissionAPI.doesUserHavePermission(newParent, PermissionAPI.PERMISSION_EDIT, user,
        respectFrontendRoles)) {
    throw new DotSecurityException(errorMsg);
}

💡 Hoist the newParent EDIT check to before categoryFactory.save(category, parent), alongside the existing upfront permission checks.

---

🟡 Medium dotCMS/src/main/java/com/dotcms/rest/api/v1/categories/CategoriesResource.java:784-787

When a PUT payload's parent field is an inode that doesn't resolve to an existing category, categoryAPI.find(id, ...) returns null instead of throwing, so parentCategory ends up null and CategoryAPIImpl.save()'s else if (parent != null) branch silently skips the reparent — the endpoint still returns 200 OK with no error and no move. Contrast with the same method's handling of an unknown category inode, which explicitly throws DoesNotExistException.

if (UtilMethods.isSet(categoryForm.getParent())) {
    parentCategory = this.categoryAPI.find(categoryForm.getParent(), user,
            pageMode.respectAnonPerms);
}

💡 If categoryForm.getParent() was set but the lookup returns null, throw DoesNotExistException instead of silently no-oping.

---

🟡 Medium dotCMS/src/main/java/com/dotcms/rest/api/v1/categories/CategoryHelper.java:54-66

CategoryHelper.toCategoryView(...) never populates CategoryView.parent, even though the view declares the field. Now that PUT can actually move a category, callers have no way to confirm from the response body which parent the category ended up under — especially given the silent-no-op issue above, where a successful-looking response could mean the reparent was skipped entirely.

return new CategoryView.Builder()
         .inode(category.getInode())
         .description(category.getDescription())
         ...
         .build(); // .parent(...) is never called

💡 Populate .parent(...) from categoryAPI.getParents(category, ...) in toCategoryView/toCategoryWithChildCountView.

---

🟡 Medium dotCMS/src/main/java/com/dotcms/rest/api/v1/categories/CategoriesResource.java:539-552

The @Operation Swagger description for updateCategory doesn't mention that supplying a different parent now moves the category, nor that omitting parent preserves the existing relationship (asymmetric create/update semantics). Per repo convention, @Operation/@Parameter docs must accurately describe behavior — this is a real documentation gap given the PR's own stated behavior change.
💡 Extend the @Operation description to document that changing parent re-parents the category (subject to EDIT on the new parent) and that omitting it leaves the relationship untouched.

---

🟡 Medium dotCMS/src/main/java/com/dotmarketing/portlets/categories/business/CategoryAPIImpl.java:223-228

currentParents is fetched in one query, but each categoryFactory.removeParent call in the loop re-issues its own SELECT + DELETE against the tree table, costing 2N extra round-trips for a category with N parents instead of a single batched delete. Low impact for the common single-parent case, but avoidable overhead for categories with many parents.

for (final Category currentParent : currentParents) {
    if (!newParent.getInode().equals(currentParent.getInode())) {
        categoryFactory.removeParent(category, currentParent);
    }
}

💡 Collect the old-parent inodes and issue one batched delete against tree (e.g. adapt TreeFactory's existing deleteTreesByChildAndParentsAndRelationType-style helper).

---

Next steps

• 🟠 Fix locally and push — these need your judgment
• 🟡 You can ask me to handle mechanical fixes inline: @claude fix <issue description> in <File.java>
• Every new push triggers a fresh review automatically

[mergify]

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

• Queue this pull request