From 303e56548c6162c8332b66aa799ac5ac2f53a9ac Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Tue, 17 Sep 2024 13:46:34 +0200 Subject: [PATCH] Add the session id derived id to polling requests The polling async requests should include the correlation id based on the session id of the user that is placing them. That way we can correlate between the access logs and the application logs. See: https://www.pivotaltracker.com/story/show/188205232 --- assets/typescript/Client/StatusClient.ts | 15 +- assets/typescript/authentication.ts | 6 +- assets/typescript/registration.ts | 10 +- ci/qa/phpunit.xml | 2 +- composer.json | 1 - composer.lock | 136 +--------------- config/openconext/parameters.yaml.dist | 8 +- config/packages/framework.yaml | 3 +- config/services.yaml | 2 +- dev/Command/AuthenticationCommand.php | 1 - src/Controller/AuthenticationController.php | 7 +- src/Controller/AuthenticationQrController.php | 1 - src/Controller/RegistrationController.php | 3 + ...RequiresActiveSessionAttributeListener.php | 3 +- src/EventSubscriber/SessionStateListener.php | 1 + src/Service/SessionCorrelationIdService.php | 23 +-- src/Tiqr/Legacy/TiqrService.php | 1 - templates/default/authentication.html.twig | 3 +- templates/default/registration.html.twig | 3 +- ...iresActiveSessionAttributeListenerTest.php | 67 +++++--- .../SessionStateListenerTest.php | 150 ++++++++++++------ .../SessionCorrelationIdServiceTest.php | 30 +++- .../Session/LoggingSessionFactoryTest.php | 16 +- 23 files changed, 239 insertions(+), 253 deletions(-) diff --git a/assets/typescript/Client/StatusClient.ts b/assets/typescript/Client/StatusClient.ts index a9babc3e..d2a90fdf 100644 --- a/assets/typescript/Client/StatusClient.ts +++ b/assets/typescript/Client/StatusClient.ts @@ -5,14 +5,23 @@ export interface PendingRequest { } export class StatusClient { - constructor(private apiUrl: string) { - + constructor( + private apiUrl: string, + private correlationLoggingId: string, + ) { } /** * Request status form the API. */ public request(callback: (status: string) => void, errorCallback: (error: unknown) => void): PendingRequest { - return jQuery.get(this.apiUrl, callback).fail(errorCallback); + let url; + if(this.correlationLoggingId !== ''){ + url = this.apiUrl + (this.apiUrl.includes('?') ? '&' : '?') + 'correlation-id=' + this.correlationLoggingId; + }else{ + url = this.apiUrl; + } + + return jQuery.get(url, callback).fail(errorCallback); } } diff --git a/assets/typescript/authentication.ts b/assets/typescript/authentication.ts index b38e13ed..e56e8751 100644 --- a/assets/typescript/authentication.ts +++ b/assets/typescript/authentication.ts @@ -8,12 +8,12 @@ import jQuery from 'jquery'; declare global { interface Window { - bootstrapAuthentication: (statusApiUrl: string, notificationApiUrl: string) => AuthenticationPageService; + bootstrapAuthentication: (statusApiUrl: string, notificationApiUrl: string, correlationLoggingId: string) => AuthenticationPageService; } } -window.bootstrapAuthentication = (statusApiUrl: string, notificationApiUrl: string) => { - const statusClient = new StatusClient(statusApiUrl); +window.bootstrapAuthentication = (statusApiUrl: string, notificationApiUrl: string, correlationLoggingId: string) => { + const statusClient = new StatusClient(statusApiUrl, correlationLoggingId); const notificationClient = new NotificationClient(notificationApiUrl); const pollingService = new StatusPollService(statusClient); diff --git a/assets/typescript/registration.ts b/assets/typescript/registration.ts index 98328d19..9588762c 100644 --- a/assets/typescript/registration.ts +++ b/assets/typescript/registration.ts @@ -7,12 +7,16 @@ import jQuery from 'jquery'; declare global { interface Window { - bootstrapRegistration: (statusApiUrl: string, notificationApiUrl: string) => RegistrationStateMachine; + bootstrapRegistration: ( + statusApiUrl: string, + notificationApiUrl: string, + correlationLoggingId: string + ) => RegistrationStateMachine; } } -window.bootstrapRegistration = (statusApiUrl: string, finalizedUrl: string) => { - const statusClient = new StatusClient(statusApiUrl); +window.bootstrapRegistration = (statusApiUrl: string, finalizedUrl: string, correlationLoggingId: string) => { + const statusClient = new StatusClient(statusApiUrl, correlationLoggingId); const pollingService = new StatusPollService(statusClient); const machine = new RegistrationStateMachine( pollingService, diff --git a/ci/qa/phpunit.xml b/ci/qa/phpunit.xml index 8d17dbb5..0f05cc07 100644 --- a/ci/qa/phpunit.xml +++ b/ci/qa/phpunit.xml @@ -17,7 +17,7 @@ - + diff --git a/composer.json b/composer.json index e269211f..862bbeb0 100644 --- a/composer.json +++ b/composer.json @@ -48,7 +48,6 @@ "khanamiryan/qrcode-detector-decoder": "^2.0", "league/csv": "^9.13", "malukenho/docheader": "^1", - "mockery/mockery": "^1.6", "overtrue/phplint": "*", "phpmd/phpmd": "^2.15", "phpstan/phpstan": "^1.10", diff --git a/composer.lock b/composer.lock index c94b49b7..dbc64042 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "be9d520404343ee4e12d3071c5bf78e1", + "content-hash": "364d7351b9f4930e8d29773e7330e973", "packages": [ { "name": "beberlei/assert", @@ -8400,57 +8400,6 @@ "abandoned": "guzzlehttp/guzzle", "time": "2014-01-28T22:29:15+00:00" }, - { - "name": "hamcrest/hamcrest-php", - "version": "v2.0.1", - "source": { - "type": "git", - "url": "https://github.com/hamcrest/hamcrest-php.git", - "reference": "8c3d0a3f6af734494ad8f6fbbee0ba92422859f3" - }, - "dist": { - "type": "zip", - "url": "https://api.github.com/repos/hamcrest/hamcrest-php/zipball/8c3d0a3f6af734494ad8f6fbbee0ba92422859f3", - "reference": "8c3d0a3f6af734494ad8f6fbbee0ba92422859f3", - "shasum": "" - }, - "require": { - "php": "^5.3|^7.0|^8.0" - }, - "replace": { - "cordoval/hamcrest-php": "*", - "davedevelopment/hamcrest-php": "*", - "kodova/hamcrest-php": "*" - }, - "require-dev": { - "phpunit/php-file-iterator": "^1.4 || ^2.0", - "phpunit/phpunit": "^4.8.36 || ^5.7 || ^6.5 || ^7.0" - }, - "type": "library", - "extra": { - "branch-alias": { - "dev-master": "2.1-dev" - } - }, - "autoload": { - "classmap": [ - "hamcrest" - ] - }, - "notification-url": "https://packagist.org/downloads/", - "license": [ - "BSD-3-Clause" - ], - "description": "This is the PHP port of Hamcrest Matchers", - "keywords": [ - "test" - ], - "support": { - "issues": "https://github.com/hamcrest/hamcrest-php/issues", - "source": "https://github.com/hamcrest/hamcrest-php/tree/v2.0.1" - }, - "time": "2020-07-09T08:09:16+00:00" - }, { "name": "instaclick/php-webdriver", "version": "1.4.19", @@ -8850,89 +8799,6 @@ }, "time": "2024-03-31T07:05:07+00:00" }, - { - "name": "mockery/mockery", - "version": "1.6.12", - "source": { - "type": "git", - "url": "https://github.com/mockery/mockery.git", - "reference": "1f4efdd7d3beafe9807b08156dfcb176d18f1699" - }, - "dist": { - "type": "zip", - "url": "https://api.github.com/repos/mockery/mockery/zipball/1f4efdd7d3beafe9807b08156dfcb176d18f1699", - "reference": "1f4efdd7d3beafe9807b08156dfcb176d18f1699", - "shasum": "" - }, - "require": { - "hamcrest/hamcrest-php": "^2.0.1", - "lib-pcre": ">=7.0", - "php": ">=7.3" - }, - "conflict": { - "phpunit/phpunit": "<8.0" - }, - "require-dev": { - "phpunit/phpunit": "^8.5 || ^9.6.17", - "symplify/easy-coding-standard": "^12.1.14" - }, - "type": "library", - "autoload": { - "files": [ - "library/helpers.php", - "library/Mockery.php" - ], - "psr-4": { - "Mockery\\": "library/Mockery" - } - }, - "notification-url": "https://packagist.org/downloads/", - "license": [ - "BSD-3-Clause" - ], - "authors": [ - { - "name": "Pádraic Brady", - "email": "padraic.brady@gmail.com", - "homepage": "https://github.com/padraic", - "role": "Author" - }, - { - "name": "Dave Marshall", - "email": "dave.marshall@atstsolutions.co.uk", - "homepage": "https://davedevelopment.co.uk", - "role": "Developer" - }, - { - "name": "Nathanael Esayeas", - "email": "nathanael.esayeas@protonmail.com", - "homepage": "https://github.com/ghostwriter", - "role": "Lead Developer" - } - ], - "description": "Mockery is a simple yet flexible PHP mock object framework", - "homepage": "https://github.com/mockery/mockery", - "keywords": [ - "BDD", - "TDD", - "library", - "mock", - "mock objects", - "mockery", - "stub", - "test", - "test double", - "testing" - ], - "support": { - "docs": "https://docs.mockery.io/", - "issues": "https://github.com/mockery/mockery/issues", - "rss": "https://github.com/mockery/mockery/releases.atom", - "security": "https://github.com/mockery/mockery/security/advisories", - "source": "https://github.com/mockery/mockery" - }, - "time": "2024-05-16T03:13:13+00:00" - }, { "name": "myclabs/deep-copy", "version": "1.12.0", diff --git a/config/openconext/parameters.yaml.dist b/config/openconext/parameters.yaml.dist index f3d53eaf..555177c9 100644 --- a/config/openconext/parameters.yaml.dist +++ b/config/openconext/parameters.yaml.dist @@ -7,6 +7,9 @@ parameters: # A secret key that's used to generate certain security-related tokens app_secret: ThisTokenIsNotSoSecretChangeIt + # A secret salt used to hash the correlationId for logging based on the session_id + correlation_id_salt: 'changeMeToAtLeast16CharsOfRandomString' + # All locales supported by the application default_locale: en_GB locales: @@ -41,11 +44,6 @@ parameters: # PCRE as accepted by preg_match (http://php.net/preg_match). mobile_app_user_agent_pattern: "/^.*$/" - # When logging authentication related messages, having a reference to the session id of the user - # makes auditing the logs much easier. We do not want to log the session_id for obvious reasons, that's why - # a salt is used to hash a part of the session id. Ensuring we do not disclose sensitive data in the logs. - session_correlation_salt: 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ' - # Options for the tiqr library tiqr_library_options: general: diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index 6bf555e3..f04eec90 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -14,9 +14,10 @@ framework: # Remove or comment this section to explicitly disable session support. session: handler_id: null + cookie_secure: true + cookie_samesite: none name: sess_tiqr cookie_httponly: true - cookie_secure: true router: strict_requirements: null utf8: true diff --git a/config/services.yaml b/config/services.yaml index ffcdc91c..8e2963ef 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -20,7 +20,7 @@ services: $tiqrConfiguration: '%tiqr_library_options%' $appSecret: '%app_secret%' $sessionOptions: '%session.storage.options%' - $sessionCorrelationSalt: '%session_correlation_salt%' + $correlationIdSalt: '%correlation_id_salt%' # makes classes in src/ available to be used as services # this creates a service per class whose id is the fully-qualified class name diff --git a/dev/Command/AuthenticationCommand.php b/dev/Command/AuthenticationCommand.php index a981d8e7..64dbaf5f 100644 --- a/dev/Command/AuthenticationCommand.php +++ b/dev/Command/AuthenticationCommand.php @@ -174,7 +174,6 @@ protected function decorateResult(string $text): string protected function readAuthenticationLinkFromFile(string $file, OutputInterface $output): string { $qrcode = new QrReader(file_get_contents($file), QrReader::SOURCE_TYPE_BLOB); - /** @phpstan-var mixed $link */ $link = $qrcode->text(); if (!is_string($link)) { diff --git a/src/Controller/AuthenticationController.php b/src/Controller/AuthenticationController.php index 0fadc7bc..5eb2a5b8 100644 --- a/src/Controller/AuthenticationController.php +++ b/src/Controller/AuthenticationController.php @@ -29,6 +29,7 @@ use Surfnet\Tiqr\Exception\UserNotFoundException; use Surfnet\Tiqr\Exception\UserPermanentlyBlockedException; use Surfnet\Tiqr\Exception\UserTemporarilyBlockedException; +use Surfnet\Tiqr\Service\SessionCorrelationIdService; use Surfnet\Tiqr\Tiqr\AuthenticationRateLimitServiceInterface; use Surfnet\Tiqr\Tiqr\Exception\UserNotExistsException; use Surfnet\Tiqr\Tiqr\Response\AuthenticationResponse; @@ -53,6 +54,7 @@ public function __construct( private readonly TiqrServiceInterface $tiqrService, private readonly TiqrUserRepositoryInterface $userRepository, private readonly AuthenticationRateLimitServiceInterface $authenticationRateLimitService, + private readonly SessionCorrelationIdService $correlationIdService, private readonly LoggerInterface $logger ) { } @@ -72,7 +74,7 @@ public function __invoke(Request $request): Response $logger->info('Verifying if there is a pending authentication request from SP'); - // Do have a valid sample AuthnRequest? + // Do we have a valid GSSP authentication AuthnRequest in this session? if (!$this->authenticationService->authenticationRequired()) { $logger->error('There is no pending authentication request from SP'); @@ -145,7 +147,8 @@ public function __invoke(Request $request): Response $logger->info('Return authentication page with QR code'); return $this->render('default/authentication.html.twig', [ - 'authenticateUrl' => $this->tiqrService->authenticationUrl() + 'authenticateUrl' => $this->tiqrService->authenticationUrl(), + 'correlationLoggingId' => $this->correlationIdService->generateCorrelationId(), ]); } diff --git a/src/Controller/AuthenticationQrController.php b/src/Controller/AuthenticationQrController.php index b7260e03..78fc71a6 100644 --- a/src/Controller/AuthenticationQrController.php +++ b/src/Controller/AuthenticationQrController.php @@ -53,7 +53,6 @@ public function __invoke(): Response $logger = WithContextLogger::from($this->logger, ['nameId' => $nameId, 'sari' => $sari]); $logger->info('Client request QR image'); - // Do have a valid sample AuthnRequest?. if (!$this->authenticationService->authenticationRequired()) { $logger->error('There is no pending authentication request from SP'); diff --git a/src/Controller/RegistrationController.php b/src/Controller/RegistrationController.php index 67f0a3ba..69f7b1ad 100644 --- a/src/Controller/RegistrationController.php +++ b/src/Controller/RegistrationController.php @@ -25,6 +25,7 @@ use Surfnet\GsspBundle\Service\StateHandlerInterface; use Surfnet\Tiqr\Attribute\RequiresActiveSession; use Surfnet\Tiqr\Exception\NoActiveAuthenrequestException; +use Surfnet\Tiqr\Service\SessionCorrelationIdService; use Surfnet\Tiqr\Tiqr\Legacy\TiqrService; use Surfnet\Tiqr\Tiqr\TiqrServiceInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; @@ -38,6 +39,7 @@ public function __construct( private readonly RegistrationService $registrationService, private readonly StateHandlerInterface $stateHandler, private readonly TiqrServiceInterface $tiqrService, + private readonly SessionCorrelationIdService $correlationIdService, private readonly LoggerInterface $logger ) { } @@ -81,6 +83,7 @@ public function registration(Request $request): Response 'default/registration.html.twig', [ 'metadataUrl' => sprintf("tiqrenroll://%s", $metadataUrl), + 'correlationLoggingId' => $this->correlationIdService->generateCorrelationId(), 'enrollmentKey' => $key ] ); diff --git a/src/EventSubscriber/RequiresActiveSessionAttributeListener.php b/src/EventSubscriber/RequiresActiveSessionAttributeListener.php index beb7a5a3..4c6c5af7 100644 --- a/src/EventSubscriber/RequiresActiveSessionAttributeListener.php +++ b/src/EventSubscriber/RequiresActiveSessionAttributeListener.php @@ -41,8 +41,9 @@ private string $sessionName; public function __construct( - private LoggerInterface $logger, + private LoggerInterface $logger, private SessionCorrelationIdService $sessionCorrelationIdService, + /** @var array */ private array $sessionOptions, ) { if (!array_key_exists('name', $this->sessionOptions)) { diff --git a/src/EventSubscriber/SessionStateListener.php b/src/EventSubscriber/SessionStateListener.php index a03a774b..1bc89e58 100644 --- a/src/EventSubscriber/SessionStateListener.php +++ b/src/EventSubscriber/SessionStateListener.php @@ -39,6 +39,7 @@ public function __construct( private LoggerInterface $logger, private SessionCorrelationIdService $sessionCorrelationIdService, + /** @var array */ private array $sessionOptions, ) { if (!array_key_exists('name', $this->sessionOptions)) { diff --git a/src/Service/SessionCorrelationIdService.php b/src/Service/SessionCorrelationIdService.php index 76e72698..638f9f16 100644 --- a/src/Service/SessionCorrelationIdService.php +++ b/src/Service/SessionCorrelationIdService.php @@ -26,32 +26,37 @@ final readonly class SessionCorrelationIdService { private string $sessionName; + private ?string $correlationIdSalt; + /** + * @param array $sessionOptions + */ public function __construct( private RequestStack $requestStack, - private array $sessionOptions, - private string $sessionCorrelationSalt, + array $sessionOptions, + ?string $correlationIdSalt = null, ) { - if (!array_key_exists('name', $this->sessionOptions)) { + if (!array_key_exists('name', $sessionOptions)) { throw new RuntimeException( 'The session name (PHP session cookie identifier) could not be found in the session configuration.' ); } - if (empty($this->sessionCorrelationSalt)) { - throw new RuntimeException('Please configure a non empty session correlation salt.'); - } - - $this->sessionName = $this->sessionOptions['name']; + $this->correlationIdSalt = is_null($correlationIdSalt) || strlen($correlationIdSalt) < 16 ? null : $correlationIdSalt; + $this->sessionName = $sessionOptions['name']; } public function generateCorrelationId(): ?string { + if ($this->correlationIdSalt === null) { + return null; + } + $sessionCookie = $this->requestStack->getMainRequest()?->cookies->get($this->sessionName); if ($sessionCookie === null) { return null; } - return hash('sha256', $this->sessionCorrelationSalt . substr($sessionCookie, 0, 10)); + return substr(hash('sha256', $sessionCookie.$this->correlationIdSalt), 0, 8); } } diff --git a/src/Tiqr/Legacy/TiqrService.php b/src/Tiqr/Legacy/TiqrService.php index 3e77d905..181bbb25 100644 --- a/src/Tiqr/Legacy/TiqrService.php +++ b/src/Tiqr/Legacy/TiqrService.php @@ -38,7 +38,6 @@ use Tiqr_Service; use Tiqr_StateStorage_StateStorageInterface; - /** * Wrapper around the legacy Tiqr service. * diff --git a/templates/default/authentication.html.twig b/templates/default/authentication.html.twig index 0cd8feac..04a64d68 100644 --- a/templates/default/authentication.html.twig +++ b/templates/default/authentication.html.twig @@ -16,7 +16,8 @@ */ var authenticationPageService = window.bootstrapAuthentication( "{{ path('app_identity_authentication_status') | escape('js') }}", - "{{ path('app_identity_authentication_notification') | escape('js') }}" + "{{ path('app_identity_authentication_notification') | escape('js') }}", + "{{ correlationLoggingId }}" ); {% endblock %} diff --git a/templates/default/registration.html.twig b/templates/default/registration.html.twig index 4bef79de..f04d8320 100644 --- a/templates/default/registration.html.twig +++ b/templates/default/registration.html.twig @@ -13,7 +13,8 @@ {% endblock %} diff --git a/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php b/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php index 0f310f0b..378707e4 100644 --- a/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php +++ b/tests/Unit/EventSubscriber/RequiresActiveSessionAttributeListenerTest.php @@ -17,7 +17,6 @@ namespace Unit\EventSubscriber; -use Mockery; use Psr\Log\LoggerInterface; use Psr\Log\LogLevel; use Surfnet\Tiqr\Attribute\RequiresActiveSession; @@ -42,8 +41,6 @@ final class RequiresActiveSessionAttributeListenerTest extends KernelTestCase public function testControllersWithoutTheRequiresActiveSessionAttributeAreIgnored(): void { - $this->expectNotToPerformAssertions(); - self::bootKernel(); $request = new Request(server: ['REQUEST_URI' => '/route']); @@ -63,10 +60,17 @@ public function testControllersWithoutTheRequiresActiveSessionAttributeAreIgnore $dispatcher = new EventDispatcher(); - $mockLogger = Mockery::mock(LoggerInterface::class); - $mockLogger->shouldNotReceive('log'); + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->never())->method('log'); - $listener = new RequiresActiveSessionAttributeListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService( + $requestStack, + ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ' + ), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -94,16 +98,19 @@ public function testItDeniesAccessWhenThereIsNoActiveSession(): void $dispatcher = new EventDispatcher(); - $mockLogger = Mockery::mock(LoggerInterface::class); - $mockLogger->shouldReceive('log') - ->once() + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->once()) + ->method('log') ->with( LogLevel::ERROR, 'Route requires active session. Active session wasn\'t found.', ['correlationId' => '', 'route' => '/route'] ); - - $listener = new RequiresActiveSessionAttributeListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -135,16 +142,20 @@ public function testItDeniesAccessWhenThereIsNoSessionCookie(): void $dispatcher = new EventDispatcher(); - $mockLogger = Mockery::mock(LoggerInterface::class); - $mockLogger->shouldReceive('log') - ->once() + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->once()) + ->method('log') ->with( LogLevel::ERROR, 'Route requires active session. Active session wasn\'t found. No session cookie was set.', ['correlationId' => '', 'route' => '/route'] ); - $listener = new RequiresActiveSessionAttributeListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -176,16 +187,20 @@ public function testItDeniesAccessWhenTheActiveSessionDoesNotMatchTheSessionCook $dispatcher = new EventDispatcher(); - $mockLogger = Mockery::mock(LoggerInterface::class); - $mockLogger->shouldReceive('log') - ->once() + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->once()) + ->method('log') ->with( LogLevel::ERROR, 'Route requires active session. Session does not match session cookie.', - ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route'] + ['correlationId' => 'f02614d0', 'route' => '/route'] ); - $listener = new RequiresActiveSessionAttributeListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -193,8 +208,6 @@ public function testItDeniesAccessWhenTheActiveSessionDoesNotMatchTheSessionCook public function testItDoesNotThrowWhenTheActiveSessionMatchesTheSessionCookie(): void { - $this->expectNotToPerformAssertions(); - self::bootKernel(); $session = new Session(new MockArraySessionStorage()); @@ -216,10 +229,14 @@ public function testItDoesNotThrowWhenTheActiveSessionMatchesTheSessionCookie(): $dispatcher = new EventDispatcher(); - $mockLogger = Mockery::mock(LoggerInterface::class); - $mockLogger->shouldNotReceive('log'); + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->never())->method('log'); - $listener = new RequiresActiveSessionAttributeListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $listener = new RequiresActiveSessionAttributeListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelControllerArguments']); $dispatcher->dispatch($event, KernelEvents::REQUEST); diff --git a/tests/Unit/EventSubscriber/SessionStateListenerTest.php b/tests/Unit/EventSubscriber/SessionStateListenerTest.php index 580bddbc..98df41c2 100644 --- a/tests/Unit/EventSubscriber/SessionStateListenerTest.php +++ b/tests/Unit/EventSubscriber/SessionStateListenerTest.php @@ -17,7 +17,6 @@ namespace Unit\EventSubscriber; -use Mockery; use Psr\Log\LoggerInterface; use Psr\Log\LogLevel; use Surfnet\Tiqr\EventSubscriber\SessionStateListener; @@ -38,8 +37,6 @@ final class SessionStateListenerTest extends KernelTestCase public function testItLogsWhenUserHasNoSessionCookie(): void { - $this->expectNotToPerformAssertions(); - self::bootKernel(); $request = new Request(server: ['REQUEST_URI' => '/route']); @@ -51,12 +48,19 @@ public function testItLogsWhenUserHasNoSessionCookie(): void $dispatcher = new EventDispatcher(); - $mockLogger = Mockery::mock(LoggerInterface::class); - $mockLogger->shouldReceive('log') - ->once() - ->with(LogLevel::INFO, 'User made a request without a session cookie.', ['correlationId' => '', 'route' => '/route']); - - $listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->once()) + ->method('log') + ->with( + LogLevel::INFO, + 'User made a request without a session cookie.', + ['correlationId' => '', 'route' => '/route'] + ); + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSIONID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -64,8 +68,6 @@ public function testItLogsWhenUserHasNoSessionCookie(): void public function testItLogsWhenUserHasNoSession(): void { - $this->expectNotToPerformAssertions(); - self::bootKernel(); $request = new Request(server: ['REQUEST_URI' => '/route'], cookies: ['PHPSESSID' => self::SESSION_ID]); @@ -77,15 +79,37 @@ public function testItLogsWhenUserHasNoSession(): void $dispatcher = new EventDispatcher(); - $mockLogger = Mockery::mock(LoggerInterface::class); - $mockLogger->shouldReceive('log') - ->once() - ->with(LogLevel::INFO, 'User made a request with a session cookie.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - $mockLogger->shouldReceive('log') - ->once() - ->with(LogLevel::INFO, 'Session not found.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - - $listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $mockLogger = $this->createMock(LoggerInterface::class); + + $mockLogger->expects($this->exactly(2)) + ->method('log') + ->willReturnCallback(function ($level, $message, $context) { + static $calledMessages = []; + + if (isset($calledMessages[$message])) { + $this->fail('Log message "' . $message . '" was called more than once.'); + } + $calledMessages[$message] = true; + + switch($message) { + case 'Session not found.': + case 'User made a request with a session cookie.': + $this->assertSame(LogLevel::INFO, $level); + $this->assertSame('f02614d0', $context['correlationId']); + $this->assertSame('/route', $context['route']); + break; + default: + $this->fail('Unexpected log message'); + } + + }) + ; + + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -93,8 +117,6 @@ public function testItLogsWhenUserHasNoSession(): void public function testItLogsAnErrorWhenTheSessionIdDoesNotMatchTheSessionCookie(): void { - $this->expectNotToPerformAssertions(); - self::bootKernel(); $session = new Session(new MockArraySessionStorage()); @@ -110,18 +132,35 @@ public function testItLogsAnErrorWhenTheSessionIdDoesNotMatchTheSessionCookie(): $dispatcher = new EventDispatcher(); - $mockLogger = Mockery::mock(LoggerInterface::class); - $mockLogger->shouldReceive('log') - ->once() - ->with(LogLevel::INFO, 'User made a request with a session cookie.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - $mockLogger->shouldReceive('log') - ->once() - ->with(LogLevel::INFO, 'User has a session.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - $mockLogger->shouldReceive('log') - ->once() - ->with(LogLevel::ERROR, 'The session cookie does not match the session id.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - - $listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->exactly(3)) + ->method('log') + ->willReturnCallback(function ($level, $message, $context) { + static $calledMessages = []; + + if (isset($calledMessages[$message])) { + $this->fail('Log message "' . $message . '" was called more than once.'); + } + $calledMessages[$message] = true; + + switch($message) { + case 'User made a request with a session cookie.': + case 'User has a session.': + case 'The session cookie does not match the session id.': + $this->assertSame($level === LogLevel::ERROR ? LogLevel::ERROR : LogLevel::INFO, $level); + $this->assertSame('f02614d0', $context['correlationId']); + $this->assertSame('/route', $context['route']); + break; + default: + $this->fail('Unexpected log message'); + } + }); + + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); $dispatcher->dispatch($event, KernelEvents::REQUEST); @@ -129,8 +168,6 @@ public function testItLogsAnErrorWhenTheSessionIdDoesNotMatchTheSessionCookie(): public function testTheUserSessionMatchesTheSessionCookie(): void { - $this->expectNotToPerformAssertions(); - self::bootKernel(); $session = new Session(new MockArraySessionStorage()); @@ -147,18 +184,35 @@ public function testTheUserSessionMatchesTheSessionCookie(): void $dispatcher = new EventDispatcher(); - $mockLogger = Mockery::mock(LoggerInterface::class); - $mockLogger->shouldReceive('log') - ->once() - ->with(LogLevel::INFO, 'User made a request with a session cookie.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - $mockLogger->shouldReceive('log') - ->once() - ->with(LogLevel::INFO, 'User has a session.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - $mockLogger->shouldReceive('log') - ->once() - ->with(LogLevel::INFO, 'User session matches the session cookie.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']); - - $listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack)); + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->exactly(3)) + ->method('log') + ->willReturnCallback(function ($level, $message, $context) { + static $calledMessages = []; + + if (isset($calledMessages[$message])) { + $this->fail('Log message "' . $message . '" was called more than once.'); + } + $calledMessages[$message] = true; + + switch($message) { + case 'User made a request with a session cookie.': + case 'User has a session.': + case 'User session matches the session cookie.': + $this->assertSame(LogLevel::INFO, $level); + $this->assertSame('f02614d0', $context['correlationId']); + $this->assertSame('/route', $context['route']); + break; + default: + $this->fail('Unexpected log message'); + } + }); + + $listener = new SessionStateListener( + $mockLogger, + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), + ['name' => 'PHPSESSID'], + ); $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']); $dispatcher->dispatch($event, KernelEvents::REQUEST); diff --git a/tests/Unit/Service/SessionCorrelationIdServiceTest.php b/tests/Unit/Service/SessionCorrelationIdServiceTest.php index 8ff8c178..b3f11afa 100644 --- a/tests/Unit/Service/SessionCorrelationIdServiceTest.php +++ b/tests/Unit/Service/SessionCorrelationIdServiceTest.php @@ -30,7 +30,7 @@ public function testItHasNoCorrelationIdWhenThereIsNoSessionCookie(): void $requestStack = new RequestStack(); $requestStack->push($request); - $service = new SessionCorrelationIdService($requestStack); + $service = new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'); $this->assertNull($service->generateCorrelationId()); } @@ -41,8 +41,32 @@ public function testItGeneratesACorrelationIdBasedOnTheSessionCookie(): void $requestStack = new RequestStack(); $requestStack->push($request); - $service = new SessionCorrelationIdService($requestStack); + $service = new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'); - $this->assertSame('f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', $service->generateCorrelationId()); + $this->assertSame('f02614d0', $service->generateCorrelationId()); + } + + /** + * @dataProvider saltProvider + */ + public function testItWillNotGenerateACorrelationIdWithoutAdequateSalt(?string $salt): void + { + $request = new Request(cookies: ['PHPSESSID' => 'session-id']); + $requestStack = new RequestStack(); + $requestStack->push($request); + + $service = new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], $salt); + + $this->assertNull($service->generateCorrelationId()); + } + + public function saltProvider(): array + { + return [ + 'empty salt' => [''], + 'null salt' => [null], + 'short salt' => ['abc'], + 'almost_long_enough salt' => ['1234567890ABCDE'], + ]; } } diff --git a/tests/Unit/Session/LoggingSessionFactoryTest.php b/tests/Unit/Session/LoggingSessionFactoryTest.php index 43d625f5..50e91980 100644 --- a/tests/Unit/Session/LoggingSessionFactoryTest.php +++ b/tests/Unit/Session/LoggingSessionFactoryTest.php @@ -17,7 +17,6 @@ namespace Unit\Session; -use Mockery; use Psr\Log\LoggerInterface; use Psr\Log\LogLevel; use Surfnet\Tiqr\Service\SessionCorrelationIdService; @@ -36,16 +35,19 @@ public function testItLogsWheneverASessionIsCreated(): void $requestStack = new RequestStack(); $requestStack->push($request); - $mockLogger = Mockery::mock(LoggerInterface::class); - $mockLogger->shouldReceive('log') - ->once() - ->with(LogLevel::INFO, 'Created new session.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961']); - + $mockLogger = $this->createMock(LoggerInterface::class); + $mockLogger->expects($this->once()) + ->method('log') + ->with( + LogLevel::INFO, + 'Created new session.', + ['correlationId' => 'f02614d0'] + ); $sessionFactory = new LoggingSessionFactory( $requestStack, $this->createStub(SessionStorageFactoryInterface::class), $mockLogger, - new SessionCorrelationIdService($requestStack), + new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'), ); $this->assertInstanceOf(SessionInterface::class, $sessionFactory->createSession());