fix(permissions): File Asset Containers inherit from their folder, not the Site

[jcastro-dotcms]

Proposed Changes

This PR fixes: #37331

A File Asset Container is the container.vtl file under /application/containers/<name>/. It already reports Contentlet as its permission type, so both views of the file share one permission identity — but its parent Permissionable was still the Container one (the Site), while the Contentlet view of the same file resolves the container folder.

Because permission_reference is keyed by asset_id alone (unique (asset_id)), whichever view was loaded last overwrote the row written by the other. Once the Container view won, every grant made on the Container folder was replaced by whatever the Site inherits, and roles holding View only on the folder stopped seeing the Container in the layout editor.

• FileAssetContainer.getParentPermissionable() — new override resolving the Container folder from the instance's own Site and path, so both identities agree and the reference row is stable. Instances carrying neither Site nor path (the stub built while container.vtl is being deleted) keep the previous behavior.
• ContainerPaginator.hostname() — was Host.class.cast(container.getParentPermissionable()) while sorting. That cast now receives a Folder, and the resulting ClassCastException escapes the whole GET /api/v1/containers request, so the Template Builder lists no Containers at all. It now asks the Container for its own Site name via getHostName(), which both Container types answer correctly, and falls back to an empty sort key rather than failing the request.
• PageAPIGraphQLTypesProvider — the Container's parentPermissionable GraphQL field is declared as a Site, so it now resolves the Site from the Container's own host id instead of from its permission parent.
• FileAssetContainerPermissionInheritanceTest — new integration test, registered in MainSuite3a.

The main review question for this change is "does any caller expect a Host back from getParentPermissionable()?". Every consumer in dotCMS/src/main was audited; the two above were the only ones affected. Container.getHostId()/getHostName() also cast, but FileAssetContainer overrides both from its own Site field.

Behavior change worth calling out: once the reference points at the folder and that folder carries its own inheritable permissions, the folder is the nearest ancestor with an inheritable Contentlet set, so Site-level grants no longer reach the Container. A role that could see a Container purely through a Site-level inheritable content grant, with nothing granted on the folder, loses it. That is the rule container.vtl already obeys when read as a file — which is precisely why the two identities must agree — but it is user-visible for environments relying on the previous behavior.

Checklist

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

This is a permission-resolution change, so it was treated as security-relevant throughout. It makes the effective permissions of a File Asset Container match the folder that administrators actually configured, rather than silently substituting the Site's. No permission is widened: the folder's own grants become authoritative, and the Site's inheritable grants stop leaking past a folder that has broken inheritance. The behavior change noted above is a narrowing, not a widening.

Additional Info

Three integration test methods, all failing before the fix and passing after it:

Test method	What it proves
permissionReferenceForFileAssetContainerMustResolveToContainerFolder	The same asset resolved to two different parents depending on how it was loaded.
limitedUserMustKeepSeeingContainerAfterReferenceIsRebuiltAsContainer	The user-visible consequence: the Container vanished from the picker.
folderGrantedRoleMustNotLoseViewToSiteLevelGrantOfAnotherRole	The reported role split, including the case where the Site holds an inheritable grant of its own — which is what produces the Site id rather than SYSTEM_HOST.

The test sets PERMISSION_REFERENCES_UPDATE_ASYNC to false, since the reference write is otherwise performed on a background thread and the assertions would race it.

Manual reproduction steps for QA, verified on an unfixed build, are in #37331.

Screenshots

N/A — the defect and the fix are both observable through GET /api/v1/containers/ and the permission_reference table; there is no visual change.

🤖 Generated with Claude Code

https://claude.ai/code/session_012CsKDCNudZcysg4sPKHijU

---

Generated by Claude Code

[claude]

Claude finished @jcastro-dotcms's task in 1m 44s —— View job

---

Code Review: File Asset Container permission inheritance

• Read changed files
• Analyze permission-resolution logic
• Check prior review findings
• Post review

I reviewed the diff (git diff origin/main...HEAD) against origin/main, traced the callers of getParentPermissionable()/getHostName()/getHostId(), and verified the two prior bot findings plus Daniel's inline comment.

New Issues

No issues found.

The change is sound and internally consistent:

• FileAssetContainer.getParentPermissionable() correctly resolves the Container folder and only falls back to super (the Site) for the genuinely-stubbed case (null/inode-less/SYSTEM_FOLDER). Letting DotDataException propagate on a real lookup failure — rather than persisting a Site-pointing permission_reference row — is the right call for a permission resolver; the method already declared throws DotDataException, so no signature churn.
• ContainerPaginator.hostname() — the Host.class.cast(...) → ClassCastException that took down the whole GET /api/v1/containers request is fixed. container.getHostName() works for both types: FileAssetContainer overrides it (:252), and the base Container.getHostName() (Container.java:320) still casts to Host correctly for DB containers. The catch (Exception) + StringPool.BLANK fallback means a single unresolvable Site can no longer drop a Container or fail the list.
• PageAPIGraphQLTypesProvider — resolving the parentPermissionable Site field via getHostId() + HostAPI.find(...) keeps the field type (Site) honest now that the permission parent is a Folder. Behavior stays equivalent for DB containers (getHostId() → findParentHost), and the FileAssetContainer.getHostId() NPE-on-null-host is contained inside Try → null.

Blast radius verified: the only other (Host) getParentPermissionable() cast in dotCMS/src/main is TemplatePaginator:94 (operates on Template, unaffected) and Container.getHostId/getHostName (:313/:321), both overridden in FileAssetContainer. The PR's audit checks out.

Resolved

• ✅ FileAssetContainer.java:133 — transient-failure path no longer silently persists a Site-pointing reference row; DotDataException now propagates (prior finding, fixed in bf637b07a1).
• ✅ FileAssetContainerPermissionInheritanceTest.java — PERMISSION_REFERENCES_UPDATE_ASYNC is now captured in @BeforeClass and restored in @AfterClass, so it no longer leaks synchronous upserts onto later classes in MainSuite3a (prior finding, fixed in f1dfff3c90).
• ✅ ContainerPaginator.java:183 — Daniel Solis's request to use Logger.warn instead of Logger.debug is already applied.

Test coverage is thorough: three failing-before/passing-after methods cover the reference-parent mismatch, the user-visible picker regression, and the role-split case, and the new class is registered in MainSuite3a (satisfies the CI-gate rule). The documented narrowing (Site-level inheritable grants no longer leak past a folder with broken inheritance) is a correctness improvement, not a widening — worth keeping the release-notes callout for environments that relied on the old behavior.

LGTM.

· branch issue-37331-file-asset-container-permissions-resolve-to-site

[jcastro-dotcms]

Valid finding — fixed in bf637b0.

I verified all three factual claims:

1. Try.of(...).getOrNull() catches Throwable, logs nothing.
2. The fallback is super.getParentPermissionable() → the Site.
3. The wrong parent is persisted. PermissionBitFactoryImpl guards the cache write with if (!permissionList.isEmpty()) (:1726), but the upsertPermissionReferences at Step 4 (:1735-1747) is unconditional. So a transient failure writes a Site-pointing permission_reference row that survives until something clears it.

On the assumption you flagged for verification — a transient failure being realistic rather than only "folder missing": yes. folderFactory.findFolderByPath hits cache then DB, so a pool timeout mid-request is an ordinary production event.

I went further than the suggested remedy. Logging alone would make the failure diagnosable but still persist the wrong parent. The two cases turn out to be already separable in the API — FolderAPIImpl.findFolderByPath (:138-141):

final Folder folder = folderFactory.findFolderByPath(path, host);
if (folder == null || UtilMethods.isEmpty(folder.getInode()) || permissionAPI.doesUserHavePermission(...)) {
    return folder;
}

A genuinely absent folder comes back as null or inode-less — it does not throw. It only throws on a real failure. The Try was collapsing those two into one silent null.

So the fix keeps the null / inode-less / SYSTEM_FOLDER path falling back to super (the legitimate stub case, e.g. the instance built while container.vtl is being deleted) and lets DotDataException propagate. getParentPermissionable() already declares it via Permissionable, and _loadParentPermissions is inside a throws DotDataException method, so no signature change was needed. DotSecurityException — unreachable for the System User — is rethrown as DotDataException with the folder path and Site name rather than ignored, so it can never become a silent downgrade either.

Net effect: a failed lookup now fails loudly, nothing is written, and the next request resolves the reference again. That matches the principle already stated three lines above the upsert in the #35680 comment — "a failed walk-up must be retried, not persisted" — applied to the reference row rather than the cache.

One trade-off worth naming for human reviewers: propagating turns a transient DB blip into a failed page render instead of a silent degradation to Site permissions. For a permission resolver I think loud-and-retryable beats quietly-wrong-and-durable, but it's a judgment call someone may want to take the other way.

---

Generated by Claude Code

[jcastro-dotcms]

Valid finding — fixed in f1dfff3.

Confirmed: the class had only @BeforeClass (:77) and @After (:91), no @AfterClass. PERMISSION_REFERENCES_UPDATE_ASYNC was pinned to false and never put back, so with the class registered in MainSuite3a every class scheduled after it inherited synchronous permission-reference upserts.

Also confirmed both cited precedents do capture-and-restore: ContentDriveLinksTest:368-370 / :438-440 and MenuLinkAPITest:113-115 / :143-145, each in a try/finally.

The fix captures the old value in @BeforeClass and restores it in a new @AfterClass. One deliberate deviation from the precedents: they scope the flag per test method with try/finally, whereas all three methods here need it, so it stays class-scoped. That is exactly what makes the restore mandatory rather than optional, and the comment on prepare() now says so — so the next person to add a method to this class does not "simplify" the @AfterClass away.

The property name is also lifted to a constant now that it appears three times.

---

Generated by Claude Code

[jcastro-dotcms]

Closing this PR as several commits are not verified. New PR: #37402