Skip to content

fix(publishing): check permissions before deleting push history - #38121

Merged
fabrizzio-dotCMS merged 5 commits into
mainfrom
bundle-resource-push-history
Oct 21, 2026
Merged

fabrizzio-dotCMS merged 5 commits into
mainfrom
bundle-resource-push-history

Conversation

@zJaaal

@zJaaal zJaaal commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

Summary

Deleting push history now checks the user's permissions on the asset or environment. Administrators are unaffected.

Environment history is checked against access to the Configuration tool, where environments are managed.

Adds integration tests in BundleResourceTest.

Fixes dotCMS/private-issues#679

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the Area : Backend PR changes Java/Maven backend code label Oct 20, 2026
@dotCMS dotCMS deleted a comment from claude Bot Oct 20, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@fabrizzio-dotCMS fabrizzio-dotCMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid change — these two delete endpoints had no authorization check at all. Two things I'd like addressed before it goes in.

1. findPermissionable covers 6 of the 16 PusheableAsset types (blocking)

findPermissionable tries ContentType first, then delegates to AssetPermissionHelper.resolveAsset, which covers Folder, Host, Contentlet, Template and Container. For anything else it returns null — and checkCanEditAsset treats null exactly like "no permission".

Push history screens exist for types outside that set: html/categories/push_history.jsp for categories, and portlet/ext/links/edit_link.jsp (through edit_versions_inc.jsp) for menu links. Category extends Inode and Link extends WebAsset, so both are Permissionable and doesUserHavePermission would work fine if the lookup ever reached them.

Concretely: an editor who holds EDIT on a category opens its push history, clicks delete, and gets a 403 saying they are not allowed. Failing closed is the right default, but the message is wrong about the cause and the feature is gone for every non-admin on those screens.

Either extend the resolution to the remaining permissionable types, or at minimum separate "could not resolve the asset" from "resolved but not permitted", so the response and the log say which one actually happened.

2. "configuration-beta" doesn't exist anywhere (trivial)

CONFIGURATION_BETA_PORTLET_ID is a hardcoded "configuration-beta". That string doesn't appear anywhere else in the repo — Java, JSPs, frontend, SQL or config — so that branch is always false today.

If the beta portlet is planned, the established home for it is PortletID, alongside TOOLS_BETA("tools-beta"), which ships with a comment naming the issue that removes it.

Smaller points

  • PERMISSION_EDIT vs PERMISSION_PUBLISH: clearing push history is a publishing fact rather than an editing one. A user with EDIT and without PUBLISH can clear the record of something they could never have pushed themselves. Was PUBLISH considered?
  • The two checks use different notions of authority — a permission on the object for the asset path, portlet access for the environment path. The environment choice is defensible, since remotePublishing.jsp lives under portlet/ext/cmsconfig/ and is loaded from that portlet's own main.js. But Environment implements Permissionable and EnvironmentAPI.findEnvironmentsByRole already exists, so a line in the description saying the coarser check was deliberate would save the next reader the question.
  • AssetPermissionHelper is @ApplicationScoped with an @Inject constructor; new AssetPermissionHelper() bypasses the container and builds a fresh instance on every call.
  • The three new private methods have no Javadoc. The null contract of findPermissionable is the one that most needs it, given point 1.

Tests

Registered in MainSuite2b, so they will actually run in CI — good, that's the easy one to miss.

They do miss both branches above, though. There's no test with an asset type the resolver can't find, which is what would have surfaced point 1. And test_deleteEnvironmentPushHistory_admin_deletesHistory uses APILocator.systemUser(), so it short-circuits on user.isAdmin() and doesUserHaveAccessToPortlet is never the deciding call — a non-admin holding the Configuration portlet would cover it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dotCMS dotCMS deleted a comment from claude Bot Oct 20, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dotCMS dotCMS deleted a comment from claude Bot Oct 20, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dotCMS dotCMS deleted a comment from claude Bot Oct 20, 2026
@zJaaal zJaaal added the PR: docker image Build & push a per-PR test image to dotcms/dotcms-test label Oct 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🐳 PR Docker test image

Latest build for commit c658d98 pushed to dotcms/dotcms-test:

docker pull dotcms/dotcms-test:pr-38121-bundle-resource-push-history
docker pull dotcms/dotcms-test:pr-38121-bundle-resource-push-history_c658d98

@fabrizzio-dotCMS
fabrizzio-dotCMS added this pull request to the merge queue Oct 20, 2026
Merged via the queue into main with commit 8cb4d7d Oct 21, 2026
72 checks passed
@fabrizzio-dotCMS
fabrizzio-dotCMS deleted the bundle-resource-push-history branch October 21, 2026 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Backend PR changes Java/Maven backend code PR: docker image Build & push a per-PR test image to dotcms/dotcms-test

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants