Skip to content

fix: the ACL answers the actions the controllers ask about - #877

Merged
blaipr merged 1 commit into
mainfrom
fix/the-acl-answers-the-actions-the-controllers-ask-about
Aug 26, 2026
Merged

fix: the ACL answers the actions the controllers ask about#877
blaipr merged 1 commit into
mainfrom
fix/the-acl-answers-the-actions-the-controllers-ask-about

Conversation

@blaipr

@blaipr blaipr commented Aug 24, 2026

Copy link
Copy Markdown
Member

Acl::checkUserAccess() is one switch ending in a deny, so an action id with no case is
refused for everyone except an application administrator, who short-circuits at the top.
Nothing reports that the id was simply not listed — it looks exactly like a permission
somebody has not been granted.

Three were in that state, each the only unlisted action in a family whose other members
are granted normally, and each with a button its grid renders unconditionally:

  • PLUGIN_DELETE, beside five plugin actions granted by isConfigGeneral().
  • ACCOUNTMGR_HISTORY_RESTORE and ACCOUNTMGR_HISTORY_DELETE, beside the history tab and its
    search, both granted by isAdminAcc or isMgmAccounts().

So a user holding the profile bit that grants the page could open it, see the actions, and
be refused by every one of them. Fail-closed, so these are broken features rather than
holes — which is why they had gone unnoticed.

They join their siblings. The test is the more useful half: it reads every
checkUserAccess(AclActionsInterface::X) in the entry-point tree and asserts Acl has a case
for X, so a fourth cannot arrive by being forgotten.

Three notification actions — create, edit and delete — stay unlisted, and the test asserts
they are still unlisted rather than ignoring them. Adding an arm hands the action to
whoever holds some profile bit, and which bit that should be is a decision about the
product rather than a defect; the absence has been looked at before, when
NOTIFICATION_DELETE was mistakenly described as reachable by any signed-in user.

The other direction — an action id no controller checks — is RoutesAreDispatchableTest's
business, and this says nothing about it: plenty of ids exist only so permissions can be
named. This asks only that what the code checks is what the ACL answers.

Checked by removing the PLUGIN_DELETE case again: the test fails, naming it.

`Acl::checkUserAccess()` is one switch ending in a deny, so an action id with no case is
refused for everyone except an application administrator, who short-circuits at the top.
Nothing reports that the id was simply not listed — it looks exactly like a permission
somebody has not been granted.

Three were in that state, each the only unlisted action in a family whose other members
are granted normally, and each with a button its grid renders unconditionally:

- PLUGIN_DELETE, beside five plugin actions granted by isConfigGeneral().
- ACCOUNTMGR_HISTORY_RESTORE and ACCOUNTMGR_HISTORY_DELETE, beside the history tab and its
  search, both granted by isAdminAcc or isMgmAccounts().

So a user holding the profile bit that grants the page could open it, see the actions, and
be refused by every one of them. Fail-closed, so these are broken features rather than
holes — which is why they had gone unnoticed.

They join their siblings. The test is the more useful half: it reads every
checkUserAccess(AclActionsInterface::X) in the entry-point tree and asserts Acl has a case
for X, so a fourth cannot arrive by being forgotten.

Three notification actions — create, edit and delete — stay unlisted, and the test asserts
they are still unlisted rather than ignoring them. Adding an arm hands the action to
whoever holds some profile bit, and which bit that should be is a decision about the
product rather than a defect; the absence has been looked at before, when
NOTIFICATION_DELETE was mistakenly described as reachable by any signed-in user.

The other direction — an action id no controller checks — is RoutesAreDispatchableTest's
business, and this says nothing about it: plenty of ids exist only so permissions can be
named. This asks only that what the code checks is what the ACL answers.

Checked by removing the PLUGIN_DELETE case again: the test fails, naming it.
@blaipr
blaipr force-pushed the fix/the-acl-answers-the-actions-the-controllers-ask-about branch from 03ef93e to 00ebf7c Compare August 26, 2026 11:37
@blaipr
blaipr merged commit d2f956b into main Aug 26, 2026
8 checks passed
@blaipr
blaipr deleted the fix/the-acl-answers-the-actions-the-controllers-ask-about branch August 26, 2026 11:46
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