From b370f038f20628a449aca7c03e393d0a6002ac7e Mon Sep 17 00:00:00 2001 From: Krystian Szymukowicz Date: Sat, 3 Oct 2026 19:52:27 +0200 Subject: [PATCH] [!!!][TASK] Refactor expression evaluation in access checkers Build the expression language resolver once per checker without variables and pass the variables of each check to evaluate(), like ResponseCacheService already does. Add functional tests covering the expressions documented on the Security documentation page. Breaking: protected AbstractAccessChecker::getExpressionLanguageResolver() is removed, use evaluateExpression() instead. --- Classes/Security/AbstractAccessChecker.php | 31 +- Classes/Security/FilterAccessChecker.php | 2 +- Classes/Security/OperationAccessChecker.php | 4 +- Tests/Functional/Security/ExpressionTest.php | 352 +++++++++++++++++++ 4 files changed, 369 insertions(+), 20 deletions(-) create mode 100644 Tests/Functional/Security/ExpressionTest.php diff --git a/Classes/Security/AbstractAccessChecker.php b/Classes/Security/AbstractAccessChecker.php index d6ff0848..f2541737 100644 --- a/Classes/Security/AbstractAccessChecker.php +++ b/Classes/Security/AbstractAccessChecker.php @@ -16,25 +16,22 @@ public function __construct( protected readonly EventDispatcherInterface $eventDispatcher ) {} - protected function getExpressionLanguageResolver(array $additionalExpressionLanguageVariables = []): Resolver - { - static $expressionLanguageResolver; + /** + * Built once per checker without any variables - variables differ for every check (`object`, + * `t3apiOperation`, `t3apiFilter`, event-provided ones) and are passed to each evaluation. + */ + protected ?Resolver $expressionLanguageResolver = null; - if ($expressionLanguageResolver === null) { - $context = GeneralUtility::makeInstance(Context::class); + protected function evaluateExpression(string $expression, array $additionalExpressionLanguageVariables = []): bool + { + $this->expressionLanguageResolver ??= GeneralUtility::makeInstance(Resolver::class, 't3api', []); - $variables = array_merge( - ExpressionLanguageService::getUserVariables($context), + return (bool)$this->expressionLanguageResolver->evaluate( + $expression, + array_merge( + ExpressionLanguageService::getUserVariables(GeneralUtility::makeInstance(Context::class)), $additionalExpressionLanguageVariables - ); - - $expressionLanguageResolver = GeneralUtility::makeInstance( - Resolver::class, - 't3api', - $variables - ); - } - - return $expressionLanguageResolver; + ) + ); } } diff --git a/Classes/Security/FilterAccessChecker.php b/Classes/Security/FilterAccessChecker.php index 98c08fde..7b88e0ec 100644 --- a/Classes/Security/FilterAccessChecker.php +++ b/Classes/Security/FilterAccessChecker.php @@ -24,6 +24,6 @@ public function isGranted(ApiFilter $filter, array $expressionLanguageVariables $variables = array_merge($expressionLanguageVariables, ['t3apiFilter' => $filter]); - return $this->getExpressionLanguageResolver($variables)->evaluate($filter->getStrategy()->getCondition()); + return $this->evaluateExpression($filter->getStrategy()->getCondition(), $variables); } } diff --git a/Classes/Security/OperationAccessChecker.php b/Classes/Security/OperationAccessChecker.php index 2dfcb651..5c77e168 100644 --- a/Classes/Security/OperationAccessChecker.php +++ b/Classes/Security/OperationAccessChecker.php @@ -25,7 +25,7 @@ public function isGranted(OperationInterface $operation, array $expressionLangua $variables = array_merge($expressionLanguageVariables, ['t3apiOperation' => $operation]); - return $this->getExpressionLanguageResolver($variables)->evaluate($operation->getSecurity()); + return $this->evaluateExpression($operation->getSecurity(), $variables); } public function isGrantedPostDenormalize( @@ -45,6 +45,6 @@ public function isGrantedPostDenormalize( $variables = array_merge($expressionLanguageVariables, ['t3apiOperation' => $operation]); - return $this->getExpressionLanguageResolver($variables)->evaluate($operation->getSecurityPostDenormalize()); + return $this->evaluateExpression($operation->getSecurityPostDenormalize(), $variables); } } diff --git a/Tests/Functional/Security/ExpressionTest.php b/Tests/Functional/Security/ExpressionTest.php new file mode 100644 index 00000000..f5d2c037 --- /dev/null +++ b/Tests/Functional/Security/ExpressionTest.php @@ -0,0 +1,352 @@ +createOperationAccessChecker(); + $operation = $this->createOperation(securityPostDenormalize: 'object.title == "published"'); + + self::assertTrue($checker->isGrantedPostDenormalize($operation, ['object' => $this->createObject('published')])); + self::assertFalse($checker->isGrantedPostDenormalize($operation, ['object' => $this->createObject('draft')])); + } + + #[Test] + public function t3apiOperationVariableIsAvailableInOperationCheck(): void + { + $checker = $this->createOperationAccessChecker(); + + self::assertTrue($checker->isGranted($this->createOperation(key: 'get', security: 't3apiOperation.getKey() == "get"'))); + self::assertTrue($checker->isGranted($this->createOperation(key: 'post', security: 't3apiOperation.getKey() == "post"'))); + } + + #[Test] + public function collectionRequestEvaluatesOperationSecurityAndFilterCondition(): void + { + self::assertTrue($this->createOperationAccessChecker()->isGranted($this->createOperation(security: 'true'))); + self::assertTrue($this->createFilterAccessChecker()->isGranted( + $this->createFilter('t3apiFilter.getProperty() == "title"') + )); + } + + #[Test] + public function userVariablesFollowLoginState(): void + { + $checker = $this->createOperationAccessChecker(); + $operation = $this->createOperation(security: 'frontend.user.isLoggedIn'); + + self::assertFalse($checker->isGranted($operation)); + + $this->loginFrontendUser(12, [3, 5]); + + self::assertTrue($checker->isGranted($operation)); + } + + #[Test] + public function operationWithoutSecurityIsGranted(): void + { + $checker = $this->createOperationAccessChecker(); + $operation = $this->createOperation(); + + self::assertTrue($checker->isGranted($operation)); + self::assertTrue($checker->isGrantedPostDenormalize($operation)); + } + + #[Test] + public function filterWithoutConditionIsGranted(): void + { + self::assertTrue($this->createFilterAccessChecker()->isGranted( + new ApiFilter(SearchFilter::class, 'title', 'partial', []) + )); + } + + public static function userVariableExpressions(): array + { + return [ + // Examples from "User variables" section + 'logged in frontend user' => ['frontend.user.isLoggedIn', true, false], + 'frontend user from group 3' => ['3 in frontend.user.userGroupIds', true, false], + 'frontend user from group 3 or 4' => ['3 in frontend.user.userGroupIds || 4 in frontend.user.userGroupIds', true, false], + 'frontend user not from group 4' => ['4 in frontend.user.userGroupIds', false, false], + 'specific frontend user' => ['frontend.user.userId == 12', true, false], + 'other frontend user' => ['frontend.user.userId == 13', false, false], + 'anonymous frontend user id' => ['frontend.user.userId == 0', false, true], + 'frontend user group list' => ['like(frontend.user.userGroupList, "*3*")', true, false], + 'logged in backend user' => ['backend.user.isLoggedIn', true, false], + 'backend admin' => ['backend.user.isAdmin', true, false], + 'backend user from group 2' => ['2 in backend.user.userGroupIds', true, false], + 'specific backend user' => ['backend.user.userId == 1', true, false], + 'backend user group list' => ['backend.user.userGroupList == "2"', true, false], + ]; + } + + #[Test] + #[DataProvider('userVariableExpressions')] + public function userVariablesAreAvailable(string $expression, bool $expectedForLoggedInUsers, bool $expectedForAnonymous): void + { + $checker = $this->createOperationAccessChecker(); + $operation = $this->createOperation(security: $expression); + + self::assertSame($expectedForAnonymous, $checker->isGranted($operation), 'anonymous'); + + $this->loginFrontendUser(12, [3, 5]); + $this->loginBackendUser(1, [2], true); + + self::assertSame($expectedForLoggedInUsers, $checker->isGranted($operation), 'logged in'); + } + + #[Test] + public function groupMembershipCheckWithInDoesNotMatchSimilarGroupIds(): void + { + $this->loginFrontendUser(12, [13, 30]); + $checker = $this->createOperationAccessChecker(); + + // See "tip" in documentation: `in` checks exact ids, `like` on the list matches substrings + self::assertFalse($checker->isGranted($this->createOperation(security: '3 in frontend.user.userGroupIds'))); + self::assertTrue($checker->isGranted($this->createOperation(security: 'like(frontend.user.userGroupList, "*3*")'))); + } + + #[Test] + public function documentedVoteExampleRestrictsPatchToOwner(): void + { + $checker = $this->createOperationAccessChecker(); + $patch = $this->createOperation( + security: 'frontend.user.isLoggedIn && object.getUser().getUid() == frontend.user.userId', + securityPostDenormalize: 'object.getUser().getUid() == frontend.user.userId' + ); + $ownVote = $this->createVote(12); + + self::assertFalse($checker->isGranted($patch, ['object' => $ownVote]), 'anonymous'); + + $this->loginFrontendUser(12, [3]); + + self::assertTrue($checker->isGranted($patch, ['object' => $ownVote]), 'own vote'); + self::assertFalse($checker->isGranted($patch, ['object' => $this->createVote(99)]), 'vote of other user'); + self::assertTrue($checker->isGrantedPostDenormalize($patch, ['object' => $ownVote]), 'owner unchanged'); + self::assertFalse($checker->isGrantedPostDenormalize($patch, ['object' => $this->createVote(99)]), 'owner changed in payload'); + } + + #[Test] + public function documentedVoteExampleRestrictsPostToLoggedInAndDeleteToAdmins(): void + { + $checker = $this->createOperationAccessChecker(); + $post = $this->createOperation(security: 'frontend.user.isLoggedIn'); + $delete = $this->createOperation(security: 'backend.user.isAdmin'); + + self::assertFalse($checker->isGranted($post)); + self::assertFalse($checker->isGranted($delete)); + + $this->loginFrontendUser(12, [3]); + self::assertTrue($checker->isGranted($post)); + self::assertFalse($checker->isGranted($delete)); + + $this->loginBackendUser(1, [2], false); + self::assertFalse($checker->isGranted($delete), 'backend user without admin rights'); + + $this->loginBackendUser(1, [2], true); + self::assertTrue($checker->isGranted($delete)); + } + + #[Test] + public function documentedFilterConditionEnablesFilterForBackendUsersOnly(): void + { + $checker = $this->createFilterAccessChecker(); + $filter = $this->createFilter('backend.user.isLoggedIn'); + + self::assertFalse($checker->isGranted($filter)); + + $this->loginBackendUser(1, [2], false); + + self::assertTrue($checker->isGranted($filter)); + } + + public static function t3apiAndCoreExpressions(): array + { + return [ + // T3api variables + 't3apiOperation' => ['t3apiOperation.getPath() == "/votes/{id}"'], + 'context' => ['context.getPropertyFromAspect("workspace", "id") == 0'], + // TYPO3 core variables + 'applicationContext' => ['applicationContext matches "/^Testing/"'], + 'typo3.version' => ['typo3.version != ""'], + 'typo3.branch' => ['typo3.branch != ""'], + 'typo3.devIpMask' => ['typo3.devIpMask == typo3.devIpMask'], + 'date variable' => ['date.getDateTime().format("Y") >= "2024"'], + 'features' => ['features.isFeatureEnabled("not-existing-feature") == false'], + // TYPO3 core functions + 'compatVersion()' => ['compatVersion("12.4")'], + 'like()' => ['like("t3api", "t3*")'], + 'date() before deadline' => ['date("Y-m-d") < "2999-01-01"'], + 'feature()' => ['feature("not-existing-feature") == false'], + 'traverse()' => ['traverse(object, "nested/key") == "value"'], + // T3api functions + 'force_absolute_url()' => ['force_absolute_url("/fileadmin/a.jpg", "https://example.com") == "https://example.com/fileadmin/a.jpg"'], + ]; + } + + #[Test] + #[DataProvider('t3apiAndCoreExpressions')] + public function documentedVariablesAndFunctionsAreAvailable(string $expression): void + { + self::assertTrue($this->createOperationAccessChecker()->isGranted( + $this->createOperation(security: $expression, path: '/votes/{id}'), + ['object' => ['nested' => ['key' => 'value']]] + )); + } + + public static function pageRelatedVariables(): array + { + return [ + 'page' => ['page.uid > 0'], + 'tree' => ['tree.level == 0'], + 'request' => ['request.getQueryParams() == []'], + 'site' => ['site.getIdentifier() == ""'], + 'siteLanguage' => ['siteLanguage.getLanguageId() == 0'], + ]; + } + + #[Test] + #[DataProvider('pageRelatedVariables')] + public function pageRelatedVariablesAreNotAvailable(string $expression): void + { + $this->expectException(SyntaxError::class); + + $this->createOperationAccessChecker()->isGranted($this->createOperation(security: $expression)); + } + + #[Test] + public function variablesFromEventsAreAvailableInEveryCheck(): void + { + $eventDispatcher = new class () implements EventDispatcherInterface { + public function dispatch(object $event): object + { + $event->setExpressionLanguageVariable('eventVariable', $event::class); + + return $event; + } + }; + $operationChecker = new OperationAccessChecker($eventDispatcher); + $filterChecker = new FilterAccessChecker($eventDispatcher); + + self::assertTrue($operationChecker->isGranted( + $this->createOperation(security: 'eventVariable matches "/BeforeOperationAccessGrantedEvent$/"') + )); + self::assertTrue($operationChecker->isGrantedPostDenormalize( + $this->createOperation(securityPostDenormalize: 'eventVariable matches "/BeforeOperationAccessGrantedPostDenormalizeEvent$/"') + )); + self::assertTrue($filterChecker->isGranted( + $this->createFilter('eventVariable matches "/BeforeFilterAccessGrantedEvent$/"') + )); + } + + private function createOperationAccessChecker(): OperationAccessChecker + { + return new OperationAccessChecker($this->get(EventDispatcherInterface::class)); + } + + private function createFilterAccessChecker(): FilterAccessChecker + { + return new FilterAccessChecker($this->get(EventDispatcherInterface::class)); + } + + private function createOperation( + string $key = 'get', + string $security = '', + string $securityPostDenormalize = '', + string $path = '' + ): OperationInterface { + $operation = $this->createMock(OperationInterface::class); + $operation->method('getKey')->willReturn($key); + $operation->method('getPath')->willReturn($path); + $operation->method('getSecurity')->willReturn($security); + $operation->method('getSecurityPostDenormalize')->willReturn($securityPostDenormalize); + + return $operation; + } + + private function createFilter(string $condition): ApiFilter + { + return new ApiFilter(SearchFilter::class, 'title', ['name' => 'partial', 'condition' => $condition], []); + } + + private function createObject(string $title): object + { + $object = new \stdClass(); + $object->title = $title; + + return $object; + } + + private function createVote(int $userUid): object + { + return new class ($userUid) { + public function __construct(private readonly int $userUid) {} + + public function getUser(): object + { + return new class ($this->userUid) { + public function __construct(private readonly int $uid) {} + + public function getUid(): int + { + return $this->uid; + } + }; + } + }; + } + + /** + * @param int[] $groupIds + */ + private function loginFrontendUser(int $uid, array $groupIds): void + { + $frontendUser = $this->createMock(FrontendUserAuthentication::class); + $frontendUser->userid_column = 'uid'; + $frontendUser->user = ['uid' => $uid]; + $frontendUser->userGroups = array_fill_keys($groupIds, []); + + GeneralUtility::makeInstance(Context::class)->setAspect('frontend.user', new UserAspect($frontendUser)); + } + + /** + * @param int[] $groupIds + */ + private function loginBackendUser(int $uid, array $groupIds, bool $isAdmin): void + { + $backendUser = $this->createMock(BackendUserAuthentication::class); + $backendUser->method('isAdmin')->willReturn($isAdmin); + $backendUser->userid_column = 'uid'; + $backendUser->user = ['uid' => $uid]; + $backendUser->userGroupsUID = $groupIds; + + GeneralUtility::makeInstance(Context::class)->setAspect('backend.user', new UserAspect($backendUser)); + } +}