fix(permissions): require EDIT_PERMISSIONS before PermissionAjax changes an asset's permissions

[oidacra]

Proposed Changes

• PermissionAjax.resetAssetPermissions and PermissionAjax.saveAssetPermissions now check that the logged-in user has EDIT_PERMISSIONS on the asset before changing its permissions, the same check the REST permission endpoints apply (AssetPermissionHelper, front-end roles not counting). The check runs before the submitted list is read, so it covers every list that ends up removing the asset's permissions.
• Links, containers and other inodes, which retrievePermissionable loads without a user, now require READ before their permissions are returned or changed. A site the user can't read is now reported instead of being skipped by the lookup.
• An id that matches no asset is rejected when changing permissions (DotDataException); reading its permissions still returns an empty list, so the Permissions tab keeps rendering (e.g. a contentlet in a language it has no version in).
• Rejections are written to the security log with the user id and the asset's permission id, and resetAssetPermissions rolls back when it rejects a call.
• retrievePermissionable uses typed result lists instead of raw ArrayList/Map casts.

Checklist

• Tests
• Translations
• Security Implications Contemplated (permission checks only; no new endpoints, configuration or data changes)

Additional Info

Tests:

• PermissionAjaxTest (unit, dotCMS module, 16 cases): edit-permission checks on reset and on both save branches, front-end-only roles, READ on the inode lookup, site/contentlet/folder lookups without READ, unknown ids, rollback and the security-log line.
• PermissionAjaxIntegrationTest (dotcms-integration, registered in MainSuite2b, 8 cases): checks the stored permission rows after each call, with a back-end user that has READ only, no permission, or EDIT_PERMISSIONS.

Behavior to know about: a user whose EDIT_PERMISSIONS on an asset comes only from front-end roles can no longer change that asset's permissions from the Permissions tab, matching the REST endpoints.

Fixes dotCMS/private-issues#766

[claude]

Claude finished @oidacra's task in 3m 9s —— View job

---

Code Review

Reviewed the permission-check changes in PermissionAjax plus both test classes. The core change is sound: saveAssetPermissions and resetAssetPermissions now gate on EDIT_PERMISSIONS before reading the submitted list (so the empty-list → removePermissions branch is covered), the reset transaction now commits only on success, and retrievePermissionable adds a READ check for inode-only lookups and rethrows the site's DotSecurityException. Test coverage is strong — 16 unit cases + 8 integration cases that assert the stored permission rows, and the integration test is correctly registered in MainSuite2b (line 490).

New Issues

• 🟡 Medium: PermissionAjax.java:394 and :520 — When the DWR call arrives without an authenticated session, userWebAPI.getLoggedInUser(request) returns null (confirmed in UserWebAPIImpl.getLoggedInUser — it returns null, not the anonymous user, for a non-logged-in/anonymous/inactive request). On the rejection path, checkEditPermissions and the new READ check in retrievePermissionable build their message with user.getUserId(), which NPEs on a null user. The call is still refused either way (no permission change), but the failure surfaces as a NullPointerException instead of the intended DotSecurityException, and the SecurityLogger.logInfo(...) line that records the rejection never runs — so the one event you want logged is the one that's lost. Assumption: these DWR methods are reachable without a backend session. What to verify: whether DWR access to PermissionAjax requires an authenticated user; if it can be hit anonymously, guard user == null (treat as rejected) before composing the message. Fix this →

Notes (non-blocking)

• 🟡 Medium: PermissionAjax.java:97 (getAssetPermissions read path) — This is the documented, intended tightening, flagging only for UI confirmation: reading permissions now throws DotSecurityException for a site/container/inode the user can't READ, whereas before it returned data (or an empty list). This differs from the unknown-id case, which still returns an empty list so the Permissions tab renders. Worth confirming the tab's DWR error handler degrades gracefully (not a stack trace in the UI) when a user opens the tab on an asset they can't read — the two paths (DotSecurityException vs. empty list) now behave differently.

• 🟡 Medium: PermissionAjax.java:518 — The new READ check hardcodes respectFrontendRoles=false, while the finders above it were called with the caller's respectFrontendRoles. This matches the PR's stated "front-end roles don't count" design and the REST endpoints, so it's consistent for the backend Permissions tab; noting only that it's a deliberate asymmetry between the inode-lookup branch and the user-aware finders.

Nothing blocking. The EDIT_PERMISSIONS gate, the reset-transaction fix, and the test suite all look correct.

· permission-ajax-edit-check

[jcastro-dotcms]

Requesting changes. Four correctness issues, then four design and cleanup notes. Details are in the inline comments.

Correctness

1. permissionIndividually is the third DWR method that writes permissions, and it doesn't get the new checkEditPermissions guard. It still relies on getNewPermissions, which lets front-end roles count, so a user who is now blocked by the save and reset methods can still break an asset's inheritance through it.
2. The new READ check also runs on the generic Inode fallback. For ids that only resolve there (a contentlet passed by inode, or one with no default-language version opened without a languageId), it now throws DotSecurityException for non-admin users instead of returning an empty list.
3. PermissionAjaxIntegrationTest attaches a mocked DWR container globally and never removes it. Later tests in MainSuite2b inherit the mocked request.
4. The READ check always passes respectFrontendRoles=false, while the other finders in retrievePermissionable honour the flag. The same user gets different answers depending on the asset type.

Design and cleanup
5. The underlying gap is that PermissionAPI.removePermissions(asset) takes no user and checks nothing. A check inside the API would cover every caller, not only this DWR facade.
6. On the non-empty save path, EDIT_PERMISSIONS is now checked twice (checkEditPermissions and assignPermissions), with different front-end-role settings.
7. The check, SecurityLogger call and throw block is duplicated between checkEditPermissions and the READ check.
8. Every denied save now also writes a WARN with a full stack trace to the main log.

[jcastro-dotcms]

permissionIndividually doesn't get this guard. It is the third DWR method that writes permissions (line 529), and it still relies only on PermissionBitAPIImpl.getNewPermissions. That method checks EDIT_PERMISSIONS with the 3-argument doesUserHavePermission, which lets front-end roles count. So a user who has EDIT_PERMISSIONS on an asset only through a front-end role is now refused by saveAssetPermissions and resetAssetPermissions, but can still call permissionIndividually and break the asset's inheritance.

Unlike the two methods fixed here, it also never calls HibernateUtil.closeSession() in a finally. Suggest calling checkEditPermissions(asset, user) there as well and aligning its transaction handling.

[jcastro-dotcms]

This check also runs on the generic Inode fallback, so it breaks reads that used to return an empty list. Some ids only resolve through InodeFactory.getInode(assetId, Inode.class) (line 513), or through the class getClassByDBType returns for contentlet. Two examples: a contentlet passed by inode, or a contentlet with no version in the default language opened without a languageId. The resulting plain Inode uses the inode as its permission id, which has no permission rows and no parent to inherit from. doesUserHavePermission(READ, user, false) is therefore false for every non-admin user, and getAssetPermissions now throws DotSecurityException where it used to return an empty list, so the permissions tab fails.

Suggest skipping the check on this fallback, or resolving the real permissionable before checking.

[jcastro-dotcms]

respectFrontendRoles is ignored here. Every other finder in retrievePermissionable (sites, folders, contentlets, templates, categories) honours the flag, but this check always passes false. When isLoggedToBackend is false, the flag is true. In that case a folder passes READ through front-end roles while a link or container inside it (the foundWithoutUser path) throws. The same user can open a folder's permissions but not those of a link in it.

Either pass respectFrontendRoles through, or document why links and containers are meant to be stricter than the rest.

[jcastro-dotcms]

This mocked container is never removed. WebContextFactory.attach sets global state, and the class has no @AfterClass that restores or detaches it. Every later test in the same JVM (MainSuite2b) that calls WebContextFactory.get() gets this mocked request, logged in as backendUser. Those tests then depend on run order and can fail, or pass for the wrong reason, as a limited back-end user. Please restore the previous builder/container in an @AfterClass.

[jcastro-dotcms]

The underlying gap is in the API, not in this facade. PermissionAPI.removePermissions(asset) takes no user and checks nothing, so every other caller that removes permissions on a user's behalf still has the same missing-authorization hole. A user-aware removePermissions(asset, user, respectFrontendRoles) that checks EDIT_PERMISSIONS, as assignPermissions already does, would close it once for all callers. A follow-up issue is fine if that is out of scope here.

[jcastro-dotcms]

On the non-empty path this repeats a check assignPermissions already makes, with different semantics. permissionAPI.assignPermissions already checks EDIT_PERMISSIONS, with front-end roles counting (true). checkEditPermissions checks it again with them not counting (false). Each save now runs two lookups for the same bit, with two separately maintained messages, and the two checks have to be kept in step. Consider enforcing it in one place.

[jcastro-dotcms]

This block is duplicated. The check, SecurityLogger.logInfo and throw new DotSecurityException here are almost identical to the READ check at lines 517-524; only the permission bit and the verb in the message differ. PermissionBitAPIImpl.assignPermissions and getNewPermissions already build the same "does not have permission to alter permissions" message, so this is the third copy. A small private helper, e.g. requirePermission(permissionable, permissionType, user, action), would remove the duplication and keep the logging consistent.

[jcastro-dotcms]

Every denied save now also writes a WARN with a full stack trace. checkEditPermissions throws inside the try, so each unauthorized attempt reaches this catch and Logger.warn(this, e.getMessage(), e) on line 330. That is on top of the SecurityLogger line already written, and a user who keeps retrying can flood the main log. Consider letting DotSecurityException bypass this warn, or logging it without the stack trace.