diff --git a/Classes/Annotations/ReadOnly.php b/Classes/Annotations/ReadOnly.php index ce2e966..24cc7db 100644 --- a/Classes/Annotations/ReadOnly.php +++ b/Classes/Annotations/ReadOnly.php @@ -1,10 +1,12 @@ bootstrap->getActiveRequestHandler(); - if (! $handler instanceof HttpRequestHandlerInterface) { - $this->logger->debug(\get_class($handler)); + $activeRequestHandler = $this->bootstrap->getActiveRequestHandler(); + if (!$activeRequestHandler instanceof HttpRequestHandlerInterface) { + $this->logger->debug('SessionLock: ' . \get_class($activeRequestHandler)); + return $joinPoint->getAdviceChain()->proceed($joinPoint); } - $componentContext = $handler->getComponentContext(); - /** @var Lock $lock */ - $lock = $componentContext->getParameter(SessionLockRequestComponent::class, 'sessionLock'); + $this->readOnly = true; - if ($lock) { - $this->logger->debug('SessionLock: Release, as this is marked read only'); + + /** @var Lock|null $lock */ + $lock = $activeRequestHandler->getComponentContext()->getParameter( + SessionLockRequestComponent::class, + SessionLockRequestComponent::PARAMETER_NAME + ); + if ($lock !== null) { + $this->logger->debug('SessionLock: Release, as this is marked read only.'); $lock->release(); } + return $joinPoint->getAdviceChain()->proceed($joinPoint); } /** * @Flow\Around("method(Neos\Flow\Session\Session->shutdownObject())") - * * @param JoinPointInterface $joinPoint + * @return mixed|void */ public function doNotSaveSession(JoinPointInterface $joinPoint) { if ($this->readOnly) { return; } - $joinPoint->getAdviceChain()->proceed($joinPoint); + + return $joinPoint->getAdviceChain()->proceed($joinPoint); } } diff --git a/Classes/Aspects/ReadOnlyFilter.php b/Classes/Aspects/ReadOnlyFilter.php index f7d7d04..751ea6e 100644 --- a/Classes/Aspects/ReadOnlyFilter.php +++ b/Classes/Aspects/ReadOnlyFilter.php @@ -1,12 +1,17 @@ configurationManager = $configurationManager; } + /** + * @param MethodTargetExpressionParser $methodTargetExpressionParser + */ public function injectMethodTargetExpressionParser(MethodTargetExpressionParser $methodTargetExpressionParser): void { $this->methodTargetExpressionParser = $methodTargetExpressionParser; @@ -36,30 +53,28 @@ class ReadOnlyFilter implements PointcutFilterInterface /** * @inheritDoc + * @throws InvalidConfigurationTypeException + * @throws InvalidPointcutExpressionException + * @throws NeosFlowAopException */ public function matches($className, $methodName, $methodDeclaringClassName, $pointcutQueryIdentifier): bool { - if ($this->filters === null) { - $this->buildPointcutFilters(); - } + $this->buildPointcutFilters(); - $matchingFilters = \array_filter( - $this->filters, - function (PointcutFilterInterface $filter) use ( - $className, - $methodName, - $methodDeclaringClassName, - $pointcutQueryIdentifier - ): bool { - return $filter->matches($className, $methodName, $methodDeclaringClassName, $pointcutQueryIdentifier); + foreach ($this->pointcutFilterComposites as $pointcutFilterComposite) { + if ( + $pointcutFilterComposite->matches( + $className, + $methodName, + $methodDeclaringClassName, + $pointcutQueryIdentifier + ) + ) { + return true; } - ); - - if ($matchingFilters === []) { - return false; } - return true; + return false; } /** @@ -80,33 +95,47 @@ class ReadOnlyFilter implements PointcutFilterInterface /** * @inheritDoc + * @throws InvalidConfigurationTypeException + * @throws InvalidPointcutExpressionException + * @throws NeosFlowAopException */ public function reduceTargetClassNames(ClassNameIndex $classNameIndex): ClassNameIndex { - if ($this->filters === null) { - $this->buildPointcutFilters(); - } + $this->buildPointcutFilters(); $result = new ClassNameIndex(); - foreach ($this->filters as $filter) { - $result->applyUnion($filter->reduceTargetClassNames($classNameIndex)); + + foreach ($this->pointcutFilterComposites as $pointcutFilterComposite) { + $result->applyUnion($pointcutFilterComposite->reduceTargetClassNames($classNameIndex)); } + return $result; } + /** + * @throws InvalidConfigurationTypeException + * @throws InvalidPointcutExpressionException + * @throws NeosFlowAopException + */ protected function buildPointcutFilters(): void { - $this->filters = []; - $readOnlyExpressions = $this->configurationManager->getConfiguration( - ConfigurationManager::CONFIGURATION_TYPE_SETTINGS, - 'DigiComp.FlowSessionLock.readOnlyExpressions' - ) ?? []; - foreach ($readOnlyExpressions as $key => $pointcut) { - if ($pointcut === null) { + if ($this->pointcutFilterComposites !== null) { + return; + } + + $this->pointcutFilterComposites = []; + foreach ( + $this->configurationManager->getConfiguration( + ConfigurationManager::CONFIGURATION_TYPE_SETTINGS, + 'DigiComp.FlowSessionLock.readOnlyExpressions' + ) as $key => $pointcutExpression + ) { + if ($pointcutExpression === null) { continue; } - $this->filters[] = $this->methodTargetExpressionParser->parse( - $pointcut, + + $this->pointcutFilterComposites[] = $this->methodTargetExpressionParser->parse( + $pointcutExpression, 'Settings.yaml at "DigiComp.FlowSessionLock.readOnlyExpressions", key: "' . $key . '"' ); } diff --git a/Classes/Http/SessionLockRequestComponent.php b/Classes/Http/SessionLockRequestComponent.php index 964e9e2..43109ad 100644 --- a/Classes/Http/SessionLockRequestComponent.php +++ b/Classes/Http/SessionLockRequestComponent.php @@ -1,26 +1,23 @@ sessionSettings['name']; - $request = $componentContext->getHttpRequest(); - $cookies = $request->getCookieParams(); + $cookies = $componentContext->getHttpRequest()->getCookieParams(); if (!isset($cookies[$sessionCookieName])) { return; } - $sessionIdentifier = $cookies[$sessionCookieName]; - - $key = new Key( - 'session-' . $sessionIdentifier - ); //TODO: sessionIdentifier might be wrong, probably it should probably be storage identifier + // TODO: sessionIdentifier might be wrong, probably it should probably be storage identifier + $key = new Key('session-' . $cookies[$sessionCookieName]); $lock = $this->lockFactory->createLockFromKey($key, $this->timeToLive, $this->autoRelease); - $componentContext->setParameter(SessionLockRequestComponent::class, 'sessionLock', $lock); + $componentContext->setParameter(SessionLockRequestComponent::class, static::PARAMETER_NAME, $lock); - $this->logger->debug('SessionLock: Get ' . $key); - $lock->acquire(true); - $this->logger->debug('SessionLock: Acquired ' . $key); + $this->logger->debug('SessionLock: Try to get "' . $key . '"'); + $timedOut = \time() + $this->secondsToWait; + while (!$lock->acquire()) { + if (\time() >= $timedOut) { + throw new LockAcquiringException( + 'Could not acquire the lock for "' . $key . '" in ' . $this->secondsToWait . ' seconds.', + 1652687960 + ); + } + \usleep(100000); + } + $this->logger->debug('SessionLock: Acquired "' . $key . '"'); } } diff --git a/Classes/Package.php b/Classes/Package.php deleted file mode 100644 index 66e97be..0000000 --- a/Classes/Package.php +++ /dev/null @@ -1,31 +0,0 @@ -getSignalSlotDispatcher(); - - $dispatcher->connect( - ConfigurationManager::class, - 'configurationManagerReady', - function (ConfigurationManager $configurationManager) { - $lockStoreDir = $configurationManager->getConfiguration( - ConfigurationManager::CONFIGURATION_TYPE_SETTINGS, - 'DigiComp.FlowSessionLock.lockStoreDir' - ); - if (is_string($lockStoreDir)) { - Files::createDirectoryRecursively($lockStoreDir); - } - } - ); - } -} diff --git a/Configuration/Development/Settings.yaml b/Configuration/Development/Settings.yaml index 0760c4d..23b7e23 100644 --- a/Configuration/Development/Settings.yaml +++ b/Configuration/Development/Settings.yaml @@ -1,3 +1,3 @@ DigiComp: FlowSessionLock: - lockStoreDir: '%FLOW_PATH_DATA%Temporary/Development/SessionLocks' + lockStoreConnection: "flock://%FLOW_PATH_DATA%Temporary/Development/SessionLocks/" diff --git a/Configuration/Objects.yaml b/Configuration/Objects.yaml index 6523a12..95f5480 100644 --- a/Configuration/Objects.yaml +++ b/Configuration/Objects.yaml @@ -1,12 +1,10 @@ DigiComp.FlowSessionLock:LockFactory: - className: 'Symfony\Component\Lock\LockFactory' + className: "Symfony\\Component\\Lock\\LockFactory" arguments: 1: - object: 'DigiComp.FlowSessionLock:LockStore' - -DigiComp.FlowSessionLock:LockStore: - className: 'Symfony\Component\Lock\Store\FlockStore' - scope: 'singleton' - arguments: - 1: - setting: 'DigiComp.FlowSessionLock.lockStoreDir' + object: + factoryObjectName: "Symfony\\Component\\Lock\\Store\\StoreFactory" + factoryMethodName: "createStore" + arguments: + 1: + setting: "DigiComp.FlowSessionLock.lockStoreConnection" diff --git a/Configuration/Settings.yaml b/Configuration/Settings.yaml index e49711d..f283d84 100644 --- a/Configuration/Settings.yaml +++ b/Configuration/Settings.yaml @@ -1,3 +1,11 @@ +DigiComp: + FlowSessionLock: + lockStoreConnection: "flock://%FLOW_PATH_DATA%Temporary/Production/SessionLocks/" + timeToLive: 300.0 + autoRelease: true + secondsToWait: 30 + readOnlyExpressions: {} + Neos: Flow: http: @@ -5,12 +13,5 @@ Neos: preprocess: chain: lockSession: - position: 'before getSessionCookieFromRequest' - component: 'DigiComp\FlowSessionLock\Http\SessionLockRequestComponent' - -DigiComp: - FlowSessionLock: - lockStoreDir: '%FLOW_PATH_DATA%Temporary/Production/SessionLocks' - readOnlyExpressions: [] - autoRelease: true - timeToLive: 300 + position: "before getSessionCookieFromRequest" + component: "DigiComp\\FlowSessionLock\\Http\\SessionLockRequestComponent" diff --git a/Configuration/Testing/Settings.yaml b/Configuration/Testing/Settings.yaml new file mode 100644 index 0000000..31d150d --- /dev/null +++ b/Configuration/Testing/Settings.yaml @@ -0,0 +1,5 @@ +DigiComp: + FlowSessionLock: + lockStoreConnection: "flock://%FLOW_PATH_DATA%Temporary/Testing/SessionLocks/" + readOnlyExpressions: + TestUnprotected: "method(DigiComp\\FlowSessionLock\\Tests\\Functional\\Fixtures\\Controller\\ExampleController->unprotectedByConfigurationAction())" diff --git a/README.md b/README.md index 781bc71..0717b21 100644 --- a/README.md +++ b/README.md @@ -1,13 +1,16 @@ DigiComp.FlowSessionLock ------------------------ -By default the session established by Flow is not "protected" in any way. -This package restricts every request to load the session only, if there are no other requests having it in access currently. -It allows to set custom pointcut which will set the session in "ReadOnly" mode, which allows concurrent requests to read, but disallows the current request to write the session. +By default, the session established by Flow is not "protected" in any way. This package restricts every request to load +the session only, if there are no other requests having it in access currently. It allows to set custom pointcut which +will set the session in "ReadOnly" mode, which allows concurrent requests to read, but disallows the current request to +write the session. -If you want to allow concurrent access somewhere, you can add your trigger pointcut in Settings.yaml like such: - - DigiComp: - FlowSessionLock: - readOnlyExpressions: - 'AcmeLock': 'method(Acme/SuperPackage/Controller/ConcurrentController->concurrentAction())' +If you want to allow concurrent access somewhere, you can add your trigger pointcut in `Settings.yaml` like such: + +```yaml +DigiComp: + FlowSessionLock: + readOnlyExpressions: + MyLock: "method(My\\Package\\Controller\\MyController->myAction())" +``` diff --git a/Tests/Functional/Fixtures/Controller/ExampleController.php b/Tests/Functional/Fixtures/Controller/ExampleController.php new file mode 100644 index 0000000..a5ecab9 --- /dev/null +++ b/Tests/Functional/Fixtures/Controller/ExampleController.php @@ -0,0 +1,43 @@ +serverRequestFactory = $this->objectManager->get(ServerRequestFactoryInterface::class); + $route = new Route(); + $route->setName('Functional Test - SessionRequestComponent::Restricted'); + $route->setUriPattern('test/sessionlock/{@action}'); + $route->setDefaults([ + '@package' => 'DigiComp.FlowSessionLock', + '@subpackage' => 'Tests\Functional\Fixtures', + '@controller' => 'Example', + '@action' => 'protected', + '@format' => 'html', + ]); + $route->setAppendExceedingArguments(true); + $this->router->addRoute($route); + } + + public function expectedDuration(): array + { + $parallelChecker = function ($allRequests, $oneRequest) { + self::assertGreaterThan(ExampleController::CONTROLLER_TIME, $oneRequest * 1000); + self::assertLessThan(ExampleController::CONTROLLER_TIME * 4, $allRequests * 1000); + }; + return [ + [ + 'http://localhost/test/sessionlock/protected', + function ($allRequests, $oneRequest) { + self::assertGreaterThan(ExampleController::CONTROLLER_TIME, $oneRequest * 1000); + self::assertGreaterThan(ExampleController::CONTROLLER_TIME * 4, $allRequests * 1000); + } + ], + [ + 'http://localhost/test/sessionlock/unprotectedbyannotation', + $parallelChecker + ], + [ + 'http://localhost/test/sessionlock/unprotectedbyconfiguration', + $parallelChecker + ] + ]; + } + + /** + * @dataProvider expectedDuration + * @test + */ + public function itDoesNotAllowToEnterMoreThanOneWithTheSameSession(string $url, \Closure $checker): void + { + $request = $this->serverRequestFactory + ->createServerRequest('GET', new Uri($url)); + $start = microtime(true); + $response = $this->browser->sendRequest($request); + $neededForOne = microtime(true) - $start; + + $sessionCookies = array_map(static function ($cookie) { + return Cookie::createFromRawSetCookieHeader($cookie); + }, $response->getHeader('Set-Cookie')); + self::assertNotEmpty($sessionCookies); + + $cookies = array_reduce($sessionCookies, static function ($out, $cookie) { + $out[$cookie->getName()] = $cookie->getValue(); + return $out; + }, []); + $nextRequest = $this->serverRequestFactory + ->createServerRequest('GET', new Uri($url)) + ->withCookieParams($cookies); + $childs = []; + $start = microtime(true); + for ($i = 0; $i < 4; $i++) { + $child = \pcntl_fork(); + if ($child === 0) { + $this->browser->sendRequest($nextRequest); + exit(); + } + $childs[] = $child; + } + foreach ($childs as $child) { + \pcntl_waitpid($child, $status); + } + $neededForAll = microtime(true) - $start; + + $checker($neededForAll, $neededForOne); + } +} diff --git a/composer.json b/composer.json index 758ea6f..8445f85 100644 --- a/composer.json +++ b/composer.json @@ -1,25 +1,14 @@ { "name": "digicomp/flowsessionlock", + "description": "Session locking for Neos Flow - it secures the session becoming corrupted by concurrent access to the same session by different requests", "type": "neos-package", - "description": "Sesion locking for Neos Flow - it secures the session becoming corrupted by concurrent access to the same session by different requests", - "keywords": [ - "flow", - "neos" - ], - "authors": [ - { - "name": "Ferdinand Kuhl", - "email": "f.kuhl@digital-competence.de", - "homepage": "http://www.digital-competence.de", - "role": "Developer" - } - ], - "license": "MIT", - "homepage": "https://github.com/digicomp/DigiComp.FlowSessionLock", "require": { - "neos/flow": "^6.2", - "php": "^7.4", - "symfony/lock": "^5.2" + "neos/flow": "^6.3.0", + "php": ">=7.4", + "symfony/lock": "^5.2.0" + }, + "require-dev": { + "ext-pcntl": "*" }, "autoload": { "psr-4": { @@ -34,5 +23,19 @@ "dev-develop": "3.0.x-dev", "dev-version/2.x-dev": "2.1.x-dev" } - } + }, + "authors": [ + { + "name": "Ferdinand Kuhl", + "email": "f.kuhl@digital-competence.de", + "homepage": "https://www.digital-competence.de", + "role": "Developer" + } + ], + "license": "MIT", + "homepage": "https://github.com/digital-competence/FlowSessionLock", + "keywords": [ + "Neos", + "Flow" + ] }