docs(spec): Content Drive folder tree hides readable folders from non-admin users

[rjvelazco]

Spec-Kit PR 1 of 2: this PR carries spec.md alone. It needs another dev's approval (not a merge) before /speckit-plan runs. PR 2 will branch from here with the implementation.

Spec for #37749.

Proposed Changes

• Adds specs/37749-content-drive-folder-search-permissions/spec.md, the issue-resolution spec for Fix Content Drive folder tree hiding readable folders from non-admin users #37749.
• No code changes. No implementation.

The defect, in short

Content Drive's folder tree hides folders that a non-admin user can read, while the Site Browser shows them. CMS Administrators never see it. The folders come back once something checks them one at a time, so it looks intermittent. Every folder made by Content Drive's bulk duplicate is hidden this way until something checks it.

The server response is already missing the folders. The batch permission filter behind GET /api/v1/folder/search grants READ only when a folder has a cached link to the parent it inherits from (a permission_reference row) or permissions of its own. That cache is filled lazily and cleared on permission changes above the folder. A folder that inherits and isn't cached yet is dropped. The one-at-a-time check walks up to the parent instead, which is why the Site Browser shows the folder.

Decisions recorded in the spec (Clarifications, 2026-10-16)

• The first load may be slower when folders aren't cached yet. Folders the batch query misses fall back to the one-at-a-time check, with no cap. That check fills the cache, so the next request is one query again. The batch SQL is not rewritten to resolve inheritance itself.
• The fix goes in the shared batch filter, not only in the folder search. Its three other callers change behavior too, each with its own criterion and test: the related-content READ check during content import (AC-011), the Publishing Queue's Pending tab (AC-013) and the folder bulk duplicate (AC-014).

Worth checking before approving

• Worst-case cost: right after a permission change on a site, a recursive folder search can run one check per inheriting folder on the site in a single request, because the READ filter runs before paging. The plan measures this on a site with a few thousand folders and sets an acceptable latency; the "no cap" decision is revisited if it's too slow.
• Behavior changes outside Content Drive: content import may accept related items it wrongly rejected; the Publishing Queue may show pending bundles it wrongly hid; the bulk duplicate may accept folders it wrongly refused. None of them allows anything the one-at-a-time check denies.
• Agreement rule: an uncached item gets the same answer it gets once cached (AC-003, AC-004). Apart from three known differences, that matches doesUserHavePermission. The differences stay as they are and go to a follow-up issue: the folder-owner grant, the Liferay isAdmin() bypass, and PUBLISH on folders (which the one-at-a-time check treats as EDIT).
• Out of scope: how the permission cache is written or cleared, the legacy list filter, the Content Drive frontend, and the code of the bulk duplicate and Publishing Queue.

🤖 Generated with Claude Code

[claude]

Claude finished @rjvelazco's task in 1m 29s —— View job

---

Spec review

• Read current spec (5d666761)
• Verify code references against main
• Post review

This PR carries specs/37749-content-drive-folder-search-permissions/spec.md alone — no code. I checked the spec's code claims against the current tree rather than hunting for runtime bugs, since there's nothing executable to break.

The claims that matter all hold up:

• Four callers of the batch overload, and only four. The spec scopes the fix to filterCollection(Collection, int, User, boolean) — PermissionBitAPIImpl.java:1415, which routes through getPermittedIds (:1456). The four callers it names use that overload: FolderAPIImpl.java:818/888/975, RelationshipUtil.java:70, PublishQueuePermissionFilter.java:138/146, FolderBulkDuplicateProcessor.java:416. The dozens of other filterCollection calls in the tree use the legacy (List, int, boolean, User) overload (:1348), which is correctly listed as a non-goal.

• The non-goal on the legacy filter is self-consistent, and now I can say why. The legacy overload (:1368-1380) already loops doesUserHavePermission per item — it is the slow path, so it never had the bug and needs no change. The gap lives only in the batch getPermittedIds route. Worth stating plainly in the spec: the two overloads are independent methods, which is what makes "fix the batch filter, leave the legacy list filter alone" coherent.

• The three known differences are real and match the code.

  • Liferay admin / CMS-Admin-only bypass: :1428-1434 bypasses for the system user and the CMS Admin role and deliberately avoids user.isAdmin(). Matches the Liferay-admin non-goal and AC-009.
  • Folder owner: getPermittedIds is driven by the user's own roleIds (:1439-1445), so a CMS-Owner grant via ownership is never seen by the batch path. Matches the folder-owner non-goal.
  • PUBLISH-on-folders → EDIT: confirmed in PublishQueuePermissionFilter.java:118-151, which splits folders out and asks for EDIT. Matches the non-goal and AC-004's "publish flag keeps following PUBLISH."

• The worst-case performance claim is accurate. FolderAPIImpl.java:815-820 loads all matching folders with "no LIMIT — pagination happens in Java" and runs the READ filter before paginate (:823/:850), so a recursive search can run one fallback check per inheriting folder in a single request. The per-folder flags (:888) run only on the post-pagination page, so page size bounds them — exactly as the Performance section states.

No blocking issues. The spec is internally consistent and its code references check out. The two prior reviewer threads (owner / PUBLISH / Liferay-admin from @dario-daza and @nollymar, the four-caller enumeration, the background-write caveat) are all reflected in the current text.

One optional nicety, not blocking: the "legacy list filter … out of scope" non-goal would be stronger if it said the legacy overload is a separate method that already checks per item, so it carries no risk from this change — right now a reader has to infer that.
· issue-37749-content-drive-folder-search-permissions

[rjvelazco]

Thanks for the review. All three points are addressed in 5d955be.

Denied vs uncached: agreed, and the spec now says so explicitly.

• Only items with no cached row and no permissions of their own may take the one-at-a-time check. A denied item does not (AC-007).
• The Performance section now says today's batch query returns only permitted ids, so the fix needs a way to tell uncached items from denied ones. The plan picks the mechanism.
• New AC-012: with a filled cache, a user denied most folders runs no one-at-a-time checks on repeated recursive searches.

One correction: a denied folder does gain a permission_reference row. loadPermissions writes the row after the parent walk-up, whatever the user's access turns out to be (PermissionBitFactoryImpl.java:1736-1746). So an uncached denied folder takes the slow path at most once; after that the batch query resolves it as denied. The real steady-state risk is the one you described: re-checking every miss instead of only the uncached ones, which the spec now rules out.

Edge case added: when the one-at-a-time check is answered from the in-memory permission cache, it returns early and does not write the row. Such an item could take the slow path again on later requests, as a memory lookup rather than a query. The spec leaves it to the plan whether the fix must also make sure the row is written.

respectFrontendRoles: AC-003 and AC-004 now require the same user, permission type and respectFrontendRoles value on both sides, and note that a missing user counts as anonymous.

AC order: fixed; the ACs now run AC-001 to AC-012 in order.

Comment written by Claude (Claude Code) on behalf of @rjvelazco.

[nollymar]

Spec review: suggested changes before approval

I checked the spec at 5d955be against main at e972c83. The root cause and the fallback approach both match the code. I'd approve once the points below are in the spec.

Points 1 and 4 repeat the two gaps in the automated review above, which are still open. I've added the detail I'd want to see in the spec. Points 2 and 3 are new.

• Before approval: 1 (name all four callers) and 2 (AC-003/AC-004 ask for more than the fix delivers).
• Nice to have: 3 (put a number on the worst case) and 4 (say the cache row is written in the background).

---

1. Name all four callers of the batch filter, with a criterion for each

Regression Risk lists two callers: folder search and the related-content check in content import. The PR description also says content import is the filter's only other caller ("Its other caller, the related-content READ check during content import, is fixed too"). On main, four places call the shared batch filter filterCollection(Collection, int, User, boolean):

Caller	What it decides	In the spec?
Content Drive folder search (FolderAPIImpl.java:818, :888, :975)	Which folders the tree lists, their edit / publish / edit-permissions / add-children flags, and "has subfolders"	Yes
Content import (RelationshipUtil.java:70)	Which related items an import accepts (READ)	Yes, AC-011
Publishing Queue, Pending tab (PublishQueuePermissionFilter.java:138, :146, used by view_publish_queue_list.jsp:299)	Which pending bundles a non-admin sees, and so can select for delete. It checks PUBLISH on each bundle's first asset, or EDIT when that asset is a folder	No
Folder bulk duplicate (FolderBulkDuplicateProcessor.java:409, called at :309 and :328)	Whether each folder can be duplicated. It checks READ on the source folder and CAN_ADD_CHILDREN on the folder the copy goes into	No

Both of the unlisted callers will change behavior. The change is in the right direction in both cases, but the spec doesn't mention it and nothing checks it:

• Publishing Queue. Today a non-admin doesn't see a pending bundle when its first asset inherits its permissions and has no permission_reference row. After the fix, the bundle appears, with its delete checkbox. That matches what the page did before Publishing Queue → Pending tab "Delete" is a silent no-op when an asset is queued in two bundles (duplicate dijit widget id aborts dojo parse) #36861, when it ran doesUserHavePermission on each bundle.
• Bulk duplicate. Today a duplicate can be refused as PERMISSION_DENIED ("no permission to read the folder") or PARENT_PERMISSION_DENIED when the source or destination folder has no row. After the fix, it goes through. A likely repro, from reading the code but not tried: duplicate a folder, then duplicate the copy from the Content Drive listing before anything else checks it. The spec lists the bulk duplicate as out of scope. That's true of its code, but not of its behavior.
• Coverage gap. AC-003 covers folders, and AC-011 covers READ on related content. No criterion covers PUBLISH on content items, which is what the Publishing Queue checks. Yet the Assumptions section says non-folder items have the same gap.

Suggested changes:

• Scope of Investigation and Regression Risk: list all four callers. Note that the bulk duplicate's behavior changes even though its code doesn't.
• AC-013: In the Publishing Queue's Pending tab, a non-admin sees a bundle when doesUserHavePermission says they can publish its first asset (or edit it, if it's a folder), even when that asset inherits its permissions and has no cached row. A bundle the one-at-a-time check denies stays hidden.
• AC-014: The folder bulk duplicate accepts a source folder the user can read and a destination they can add children to, even when either has no cached row. It still reports PERMISSION_DENIED or PARENT_PERMISSION_DENIED when the one-at-a-time check denies.
• Verification: both callers already have integration tests that can take an "uncached" case:
  • PublisherAPIImplTest already compares PublishQueuePermissionFilter with the one-at-a-time check on each bundle.
  • FolderBulkDuplicateProcessorIT covers the bulk duplicate.
• PR description: replace "Its other caller" with the three other callers.

2. AC-003 and AC-004 ask for more than the fix delivers

AC-003 says the batch filter returns exactly what doesUserHavePermission allows, "for any set of folders", with "no missing folders and no extra ones". AC-004 says the same for the per-folder flags. But the two checks also disagree in two ways that have nothing to do with the cache:

• Folder owner. The one-at-a-time check grants a permission given to the CMS Owner role when the user owns the folder (PermissionBitAPIImpl.java:397-402). The batch SQL only matches the user's own role ids, so it never sees that grant (getPermittedIds, PermissionBitFactoryImpl.java:2727).
• PUBLISH on folders. The one-at-a-time check treats PUBLISH on a folder as EDIT ("Folders do not have PUBLISH, use EDIT instead", PermissionBitAPIImpl.java:373). Folder search asks the batch filter for PERMISSION_PUBLISH on folders as-is (FolderAPIImpl.java:875). So Content Drive's publish flag follows the PUBLISH permission, not EDIT. PublishQueuePermissionFilter already works around this by asking for EDIT on folders.

The fix only sends items with no row and no own permissions to the one-at-a-time check. These two cases stay on the batch path, so the checks keep disagreeing. The planned comparison test uses four kinds of items: own permissions, cached and inheriting, uncached and inheriting, and denied. None of them hits these cases, so the test would pass even though AC-003 and AC-004, as written, are not met.

Pick one:

• Narrow the criteria (smaller change; my preference). For example: "For any set of folders that inherit their permissions and have no cached row, the batch filter returns exactly what doesUserHavePermission allows…". Then list the owner grant and the folder PUBLISH difference under Non-goals, as known separate gaps that are worth their own issue.
• Or widen the scope. Put both differences in scope, and add a folder-owner case and an EDIT-without-PUBLISH case to the comparison test. Note that this changes the publish flag Content Drive shows today.

3. Put a number on the worst case

The spec accepts that the first request after a permission change can run many one-at-a-time checks, but gives no limit or estimate. Paging doesn't limit it. Folder search loads every matching folder and runs the READ filter on all of them before it pages (FolderAPIImpl.java:815-820, "no LIMIT — pagination happens in Java"). So a recursive search right after a permission change on a site can run one check for every inheriting folder on that site, in a single request. The per-folder flags are only checked for the current page, so they're limited by page size.

Suggestion: have the plan measure that case on a large site, for example a recursive search right after a permission change on a site with a few thousand folders. Then state an acceptable latency, or add a criterion for it. If the result is too slow, the "no cap" decision in Clarifications can be revisited with data behind it.

4. Say that the cache row is written in the background

AC-010 says "once the cache row is written" and AC-012 says "until its row is written", which is correct. What the spec doesn't say is that the write happens later, on a background thread. loadPermissions puts the result into the in-memory permission cache right away. When PERMISSION_REFERENCES_UPDATE_ASYNC is true (the default), it hands the permission_reference insert to a background thread (PermissionBitFactoryImpl.java:1725-1747). If the background write fails, the error is only logged.

This has two consequences:

• Repeat checks. Requests that arrive before the row is saved take the one-at-a-time check again. The tree's parallel requests are one example. Those repeats are answered from the in-memory caches rather than by walking up to the parent again, so they cost little. "At most once" holds for the walk up to the parent, not for the number of one-at-a-time checks.
• Unreliable tests. The run-twice test for AC-010, and any counting test for AC-012, will likely send the second request before the row exists. They'd fail, or pass only sometimes.

Suggested text:

• Regression Risk: "The reference row is written on a background thread by default (PERMISSION_REFERENCES_UPDATE_ASYNC). Requests that arrive before it is saved still take the one-at-a-time check, answered from the in-memory permission cache."
• Verification method: "Tests for AC-010 and AC-012 set PERMISSION_REFERENCES_UPDATE_ASYNC=false, or wait for the write, before the second request."

Comment written by Claude (Claude Code) on behalf of @nollymar.

[rjvelazco]

@nollymar thanks, all four points checked out against main and are in the spec as of 403b9ff. The PR description is updated too.

1. All four callers. Scope of Investigation and Regression Risk now list the Publishing Queue's Pending tab and the folder bulk duplicate, with what each one decides and how its behavior changes. The bulk duplicate's non-goal now says its code is out of scope, not its behavior.

• AC-013: Publishing Queue, using your wording. Verified by an uncached case in PublisherAPIImplTest.
• AC-014: bulk duplicate, using your wording. Verified by an uncached source/destination case in FolderBulkDuplicateProcessorIT.

2. AC-003/AC-004. I took your preferred option and narrowed them to inheriting folders with no cached row. The folder-owner grant and PUBLISH on folders are now named non-goals, described as separate gaps that deserve their own issue.

One addition from following that through: once uncached folders go to the one-at-a-time check, that check would answer the publish flag with EDIT, while the same folder once cached gets PUBLISH. AC-004 now says a folder's publish flag must not depend on whether it's cached, so the fallback can't make the flag flip.

3. Worst case. Performance now says paging doesn't limit it (the READ filter runs before paging, while the per-folder flags are limited by page size). The plan measures a recursive search right after a site-level permission change on a site with a few thousand folders and states an acceptable latency; if it's too slow, the "no cap" clarification is revisited with that data.

4. Background write. Performance now says the row is written on a background thread by default, a failed write is only logged, and early repeats are answered from the in-memory cache. "At most once" now refers to the walk up to the parent (AC-012 too). Verification says the AC-010 and AC-012 tests set PERMISSION_REFERENCES_UPDATE_ASYNC=false or wait for the write.

Comment written by Claude (Claude Code) on behalf of @rjvelazco.