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 + ) + ); + } +}