Skip to content

fix: let whoever may manage files search them - #904

Merged
blaipr merged 1 commit into
mainfrom
fix/the-files-page-is-reachable-by-those-who-may-manage-files
Sep 3, 2026
Merged

fix: let whoever may manage files search them#904
blaipr merged 1 commit into
mainfrom
fix/the-files-page-is-reachable-by-those-who-may-manage-files

Conversation

@blaipr

@blaipr blaipr commented Sep 3, 2026

Copy link
Copy Markdown
Member

The Files tab in Items Management is shown when FILE is granted — isMgmFiles(). The grid inside
it searches and pages under a different action id, ACCOUNT_FILE_SEARCH, and that id had no arm
at all
in Acl::checkUserAccess(), so it fell through to the deny at the end.

isAdminApp short-circuits at the top of that method, so an application administrator never saw it.
Everyone else with isMgmFiles() saw the Files tab, saw the list of files in it, and got "You
don't have permission to do this operation"
from every search or page inside it. Two action ids
authorising one feature, disagreeing.

The arm joins the FILE/FILE_SEARCH group it belongs with and answers isMgmFiles(), which is
what showed the tab in the first place.

Why the guard test did not catch it

AclAnswersEveryActionCheckedTest exists for exactly this failure — it was written after three
actions reached the same state at once — and it passed. It scans the controllers for
checkUserAccess(AclActionsInterface::FOO).

AccountFile\SearchController does not contain that call. SearchGridControllerBase::searchAction()
calls checkUserAccess($this->getAclAction()), and the id is returned from a method in the
subclass. Six controllers are authorised that way, and the id was never within a regex's reach of
the call.

Scanning getAclAction() bodies as well brings all six in. Of those six — ACCOUNTMGR_SEARCH,
ACCOUNTMGR_HISTORY_SEARCH, AUTHTOKEN_SEARCH, CATEGORY_SEARCH, CLIENT_SEARCH and
ACCOUNT_FILE_SEARCH — the one the guard could not see is the one that was broken.

Checked and deliberately not changed

ACCOUNT_FILE_VIEW, _UPLOAD, _DOWNLOAD, _DELETE and _LIST still have no arm, and that is
correct rather than an oversight: nothing passes them to checkUserAccess(). The web controllers
enforce object-level access through AccountFileAcl::requireView()/requireEdit(), the grid does
not filter its buttons by ACL, and the API authorises by the calling token's bound action id —
Api::setup() calls getTokenByToken($actionId, $token) and never consults this ACL, so no API
endpoint was affected. Adding arms for ids nothing checks would be inventing policy; the guard
test's own docblock makes the same point about ids that exist only so permissions can be named.

Tests

  • AclTest gains the behavioural case: mgmFiles grants ACCOUNT_FILE_SEARCH, and a profile
    without it does not.
  • AclAnswersEveryActionCheckedTest now covers 88 actions instead of the direct-call subset.

Mutation-verified: with the arm removed the guard fails naming ACCOUNT_FILE_SEARCH and quoting
what it costs.

The Files tab in Items Management is shown when FILE is granted (isMgmFiles), and the grid
inside it searches and pages under ACCOUNT_FILE_SEARCH — which had no arm at all in
Acl::checkUserAccess() and fell through to the deny. isAdminApp short-circuits at the top,
so an application administrator never saw it; everyone else with isMgmFiles saw the tab,
saw the list, and was refused by every search inside it. Two ids authorising one feature,
disagreeing.

AclAnswersEveryActionCheckedTest exists for exactly this failure and passed, because it
scans for the literal checkUserAccess(AclActionsInterface::FOO) and
SearchGridControllerBase calls checkUserAccess($this->getAclAction()) — the id is returned
from a method in the subclass, never within a regex's reach of the call. Six controllers
are authorised that way, and the one the guard could not see is the one that was broken.
It now scans getAclAction() bodies too.

ACCOUNT_FILE_VIEW, _UPLOAD, _DOWNLOAD, _DELETE and _LIST still have no arm, which is
correct: nothing passes them to checkUserAccess(). The web enforces object-level access
through AccountFileAcl, the grid does not filter its buttons by ACL, and Api::setup()
authorises by the calling token's bound action id without consulting this ACL.
@blaipr
blaipr merged commit 2655b53 into main Sep 3, 2026
8 checks passed
@blaipr
blaipr deleted the fix/the-files-page-is-reachable-by-those-who-may-manage-files branch September 3, 2026 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant