From 00ebf7c5c13ff82be65f11cf3dd7306705f967f9 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 09:00:19 +0200 Subject: [PATCH] fix: the ACL answers the actions the controllers ask about MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- src/Infrastructure/Acl/Acl.php | 10 ++ .../Acl/AclAnswersEveryActionCheckedTest.php | 143 ++++++++++++++++++ 2 files changed, 153 insertions(+) create mode 100644 tests/Unit/Infrastructure/Acl/AclAnswersEveryActionCheckedTest.php diff --git a/src/Infrastructure/Acl/Acl.php b/src/Infrastructure/Acl/Acl.php index a521c1aea..32ed2bc97 100644 --- a/src/Infrastructure/Acl/Acl.php +++ b/src/Infrastructure/Acl/Acl.php @@ -149,6 +149,10 @@ public function checkUserAccess(int $actionId, int $userId = 0): bool case self::PLUGIN_ENABLE: case self::PLUGIN_RESET: case self::PLUGIN_VIEW: + // Same shape: delete was the one plugin action with no arm, so the grid offered a + // button that answered "You don't have permission" to everyone but an application + // administrator, beside five that worked. + case self::PLUGIN_DELETE: return $userProfile->isConfigGeneral(); case self::CONFIG_IMPORT: return $userProfile->isConfigImport(); @@ -186,6 +190,12 @@ public function checkUserAccess(int $actionId, int $userId = 0): bool case self::ACCOUNTMGR_SEARCH: case self::ACCOUNTMGR_HISTORY: case self::ACCOUNTMGR_HISTORY_SEARCH: + // Restore and delete were absent, so they fell through to the deny at the end and only + // an application administrator ever reached them — while the history grid rendered + // both buttons unconditionally and the tab itself was granted by the two above. An + // account manager could open the page, see the actions, and be refused by every one. + case self::ACCOUNTMGR_HISTORY_RESTORE: + case self::ACCOUNTMGR_HISTORY_DELETE: return $userDto->isAdminAcc || $userProfile->isMgmAccounts(); case self::FILE: case self::FILE_SEARCH: diff --git a/tests/Unit/Infrastructure/Acl/AclAnswersEveryActionCheckedTest.php b/tests/Unit/Infrastructure/Acl/AclAnswersEveryActionCheckedTest.php new file mode 100644 index 000000000..e8342248b --- /dev/null +++ b/tests/Unit/Infrastructure/Acl/AclAnswersEveryActionCheckedTest.php @@ -0,0 +1,143 @@ +. + */ + +declare(strict_types=1); + +namespace SP\Tests\Unit\Infrastructure\Acl; + +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; + +/** + * Every action a controller asks the ACL about has an arm in the ACL. + * + * `Acl::checkUserAccess()` is one long switch ending in a deny. An action id that has no case + * falls off the end, so it is refused for everyone except an application administrator, who + * short-circuits at the top — and nothing anywhere reports that the id was simply not listed. It + * looks exactly like a permission somebody has not been granted. + * + * Three had reached that state at once: PLUGIN_DELETE, ACCOUNTMGR_HISTORY_RESTORE and + * ACCOUNTMGR_HISTORY_DELETE, each the only unlisted action in a family whose other members were + * granted normally, and each with a button the grid rendered unconditionally. A holder of the + * profile bit that grants the page could open it, see the action, and be refused by it. + * + * The other direction — an action id with no controller — is `RoutesAreDispatchableTest`'s + * business. Plenty of ids exist only so that permissions can be named, and this deliberately says + * nothing about them: it asks only that what the code *checks* is what the ACL *answers*. + */ +#[Group('unitary')] +class AclAnswersEveryActionCheckedTest extends TestCase +{ + private const CONTROLLERS = REAL_APP_ROOT . '/src/Infrastructure/Adapter/In'; + private const ACL = REAL_APP_ROOT . '/src/Infrastructure/Acl/Acl.php'; + + /** + * @return array + */ + public static function actionProvider(): array + { + $found = []; + + $files = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator(self::CONTROLLERS, \FilesystemIterator::SKIP_DOTS) + ); + + /** @var \SplFileInfo $file */ + foreach ($files as $file) { + if ($file->getExtension() !== 'php') { + continue; + } + + preg_match_all( + '/checkUserAccess\(\s*AclActionsInterface::([A-Z0-9_]+)/', + (string)file_get_contents($file->getPathname()), + $matches + ); + + foreach ($matches[1] as $action) { + $found[$action] = [$action]; + } + } + + ksort($found); + + return $found; + } + + /** + * Actions a controller checks that the ACL deliberately does not list. + * + * All three are the notification management actions, and the effect is that only an + * application administrator reaches them. That is what the feature is — a notification is + * issued by an administrator — and the absence has been looked at before, when + * NOTIFICATION_DELETE was mistakenly described as reachable by any signed-in user. It is not: + * it falls through to the deny like these two beside it. + * + * They are listed rather than granted because adding an arm here hands the action to whoever + * holds the profile bit, and which bit that should be is a decision about the product rather + * than a defect to fix. Listing them keeps the check honest: the test asserts these three are + * *still* unlisted, so a fourth cannot join them by being forgotten. + */ + private const DELIBERATELY_UNLISTED = [ + 'NOTIFICATION_CREATE', + 'NOTIFICATION_EDIT', + 'NOTIFICATION_DELETE', + ]; + + #[Test] + #[DataProvider('actionProvider')] + public function theAclHasAnArmForIt(string $action): void + { + $hasArm = preg_match( + '/^\s*case self::' . preg_quote($action, '/') . ':\s*$/m', + (string)file_get_contents(self::ACL) + ) === 1; + + if (in_array($action, self::DELIBERATELY_UNLISTED, true)) { + self::assertFalse( + $hasArm, + sprintf( + '%s is listed as deliberately unlisted but Acl now has a case for it. Remove ' + . 'it from DELIBERATELY_UNLISTED.', + $action + ) + ); + + return; + } + + self::assertTrue( + $hasArm, + sprintf( + 'A controller calls checkUserAccess(AclActionsInterface::%s), but Acl has no case ' + . 'for it — so it falls through to the deny at the end and only an application ' + . 'administrator can ever reach that action.', + $action + ) + ); + } +}