Skip to content

fix(folders): require EDIT on the source folder when moving it - #38113

Merged
swicken merged 2 commits into
mainfrom
folder-move-edit-permission
Oct 21, 2026
Merged

swicken merged 2 commits into
mainfrom
folder-move-edit-permission

Conversation

@swicken

@swicken swicken commented Oct 20, 2026

Copy link
Copy Markdown
Member

Proposed Changes

  • FolderAPI.move(Folder, Folder, User, boolean) and FolderAPI.move(Folder, Host, User, boolean) now require EDIT permission on the folder being moved, in addition to READ on it and CAN_ADD_CHILDREN on the destination. This matches move(String, String, ...) (used by the Site Browser), renameFolder and delete, which already require EDIT/WRITE.
  • A move refused for missing EDIT on the folder logs one WARN line naming the user and folder; every refused move throws DotSecurityException.
  • The Javadoc on both interface methods states the required permissions.

Checklist

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

Additional Info

Compatibility: a user with READ but not EDIT on a folder can no longer move it through the legacy folder action, WebDAV, or a plugin calling these methods as that user. The Site Browser move is unchanged because it already required WRITE. Administrators and the system user (push publishing) are unaffected. There is no schema, index or API signature change, and no configuration switch.

Tests: four cases in FolderAPITest (already in MainSuite1a) move a folder into another folder and to a site root, once with READ only on the source (expects DotSecurityException and no change) and once with READ and EDIT (expects the move). FolderAPITest: 107 run, 0 failures.

Fixes dotCMS/private-issues#882

FolderAPI.move(Folder, Folder, ...) and FolderAPI.move(Folder, Host, ...)
now require EDIT permission on the folder being moved, in addition to
READ on it and CAN_ADD_CHILDREN on the destination. This matches the
String-id overload, folder rename and folder delete. A move refused for
missing EDIT on the folder logs a WARN line naming the user and folder,
and every refused move throws DotSecurityException.
@github-actions github-actions Bot added the Area : Backend PR changes Java/Maven backend code label Oct 20, 2026
@claude

claude Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @swicken's task in 1m 9s —— View job


Code Review

I reviewed the diff against the three changed files (FolderAPI.java, FolderAPIImpl.java, FolderAPITest.java).

Checklist

  • Read PR diff and surrounding code
  • Verified permission semantics and consistency with existing overloads
  • Reviewed test coverage
  • Assessed blast radius / callers

New Issues

No issues found.

Notes

The change is correct and consistent with the rest of FolderAPIImpl:

  • checkCanMove (FolderAPIImpl.java:1154) enforces PERMISSION_READ then PERMISSION_EDIT on the source folder, matching the move(String, String, …) path, which already requires PERMISSION_WRITE on the source via moveWhenDestinationDoesNotExists/moveToExistingDestination (FolderAPIImpl.java:1192-1193). PERMISSION_EDIT and PERMISSION_WRITE are both value 2 (PermissionAPI.java:39-40), so the two paths now require the same grant.
  • Both move overloads throw DotSecurityException before any write (folderFactory.move), so the refused case leaves no partial state — the tests' assertTrue(folderExists(sourcePath, site)) + assertFalse(...) assertions confirm this.
  • Blast radius is contained: admins and the system user short-circuit doesUserHavePermission to true, so push-publish is unaffected, as the PR body states. The internal move(String,String,…) callers already gate on WRITE, so the added source-folder check is redundant-but-harmless there, not a new rejection.
  • Tests cover all four meaningful combinations (folder→folder and folder→site, each with and without EDIT) and are in FolderAPITest, which is registered in MainSuite1a — so they will actually run in CI.

Minor, non-blocking observation (not worth changing on its own): the WARN line is logged only on the EDIT failure in checkCanMove (FolderAPIImpl.java:1161), not on the READ failure — a user denied for READ gets a DotSecurityException with no log line, same as before. Consistent with prior behavior; just an asymmetry to be aware of.

LGTM.
· folder-move-edit-permission

@swicken
swicken added this pull request to the merge queue Oct 21, 2026
Merged via the queue into main with commit 808e4d8 Oct 21, 2026
72 checks passed
@swicken
swicken deleted the folder-move-edit-permission branch October 21, 2026 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants