Skip to content

Commit

Permalink
Merge pull request #211 from OpenConext/feature/log-correlation-id-ac…
Browse files Browse the repository at this point in the history
…cesslog

Add the session id derived id to polling requests
  • Loading branch information
johanib authored Nov 14, 2024
2 parents 145fcb4 + de6a189 commit ffa280b
Show file tree
Hide file tree
Showing 15 changed files with 83 additions and 27 deletions.
10 changes: 7 additions & 3 deletions assets/typescript/Client/StatusClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,18 @@ 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);
const url = this.apiUrl + (this.apiUrl.includes('?') ? '&' : '?') + 'correlation-id=' + this.correlationLoggingId;

return jQuery.get(url, callback).fail(errorCallback);
}
}
6 changes: 3 additions & 3 deletions assets/typescript/authentication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
10 changes: 7 additions & 3 deletions assets/typescript/registration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 4 additions & 2 deletions src/Controller/AuthenticationController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
) {
}
Expand Down Expand Up @@ -145,8 +147,8 @@ public function __invoke(Request $request): Response
$logger->info('Return authentication page with QR code');

return $this->render('default/authentication.html.twig', [
// TODO: Add something identifying the authentication session to the authenticateUrl
'authenticateUrl' => $this->tiqrService->authenticationUrl()
'authenticateUrl' => $this->tiqrService->authenticationUrl(),
'correlationLoggingId' => $this->correlationIdService->generateCorrelationId(),
]);
}

Expand Down
3 changes: 3 additions & 0 deletions src/Controller/RegistrationController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
) {
}
Expand Down Expand Up @@ -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
]
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@
private string $sessionName;

public function __construct(
private LoggerInterface $logger,
private LoggerInterface $logger,
private SessionCorrelationIdService $sessionCorrelationIdService,
/** @var array<string, string> */
private array $sessionOptions,
) {
if (!array_key_exists('name', $this->sessionOptions)) {
Expand Down
1 change: 1 addition & 0 deletions src/EventSubscriber/SessionStateListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
public function __construct(
private LoggerInterface $logger,
private SessionCorrelationIdService $sessionCorrelationIdService,
/** @var array<string, string> */
private array $sessionOptions,
) {
if (!array_key_exists('name', $this->sessionOptions)) {
Expand Down
1 change: 1 addition & 0 deletions src/Service/SessionCorrelationIdService.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

public function __construct(
private RequestStack $requestStack,
/** @var array<string, string> */
private array $sessionOptions,
private string $sessionCorrelationSalt,
) {
Expand Down
1 change: 0 additions & 1 deletion src/Tiqr/Legacy/TiqrService.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
use Tiqr_Service;
use Tiqr_StateStorage_StateStorageInterface;


/**
* Wrapper around the legacy Tiqr service.
*
Expand Down
3 changes: 2 additions & 1 deletion templates/default/authentication.html.twig
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}"
);
</script>
{% endblock %}
Expand Down
3 changes: 2 additions & 1 deletion templates/default/registration.html.twig
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
<script>
var registrationStateMachine = window.bootstrapRegistration(
"{{ path('app_identity_registration_status') | escape('js') }}",
"{{ path('app_identity_registration') | escape('js') }}"
"{{ path('app_identity_registration') | escape('js') }}",
"{{ correlationLoggingId }}"
);
</script>
{% endblock %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,14 @@ public function testControllersWithoutTheRequiresActiveSessionAttributeAreIgnore
$mockLogger = Mockery::mock(LoggerInterface::class);
$mockLogger->shouldNotReceive('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);
Expand Down Expand Up @@ -103,7 +110,11 @@ public function testItDeniesAccessWhenThereIsNoActiveSession(): void
['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);
Expand Down Expand Up @@ -144,7 +155,11 @@ public function testItDeniesAccessWhenThereIsNoSessionCookie(): void
['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);
Expand Down Expand Up @@ -185,7 +200,11 @@ public function testItDeniesAccessWhenTheActiveSessionDoesNotMatchTheSessionCook
['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', '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);
Expand Down Expand Up @@ -219,7 +238,11 @@ public function testItDoesNotThrowWhenTheActiveSessionMatchesTheSessionCookie():
$mockLogger = Mockery::mock(LoggerInterface::class);
$mockLogger->shouldNotReceive('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);
Expand Down
24 changes: 20 additions & 4 deletions tests/Unit/EventSubscriber/SessionStateListenerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ public function testItLogsWhenUserHasNoSessionCookie(): void
->once()
->with(LogLevel::INFO, 'User made a request without a session cookie.', ['correlationId' => '', 'route' => '/route']);

$listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack));
$listener = new SessionStateListener(
$mockLogger,
new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'),
['name' => 'PHPSESSIONID'],
);

$dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']);
$dispatcher->dispatch($event, KernelEvents::REQUEST);
Expand Down Expand Up @@ -85,7 +89,11 @@ public function testItLogsWhenUserHasNoSession(): void
->once()
->with(LogLevel::INFO, 'Session not found.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']);

$listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack));
$listener = new SessionStateListener(
$mockLogger,
new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'),
['name' => 'PHPSESSID'],
);

$dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']);
$dispatcher->dispatch($event, KernelEvents::REQUEST);
Expand Down Expand Up @@ -121,7 +129,11 @@ public function testItLogsAnErrorWhenTheSessionIdDoesNotMatchTheSessionCookie():
->once()
->with(LogLevel::ERROR, 'The session cookie does not match the session id.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']);

$listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack));
$listener = new SessionStateListener(
$mockLogger,
new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'),
['name' => 'PHPSESSID'],
);

$dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']);
$dispatcher->dispatch($event, KernelEvents::REQUEST);
Expand Down Expand Up @@ -158,7 +170,11 @@ public function testTheUserSessionMatchesTheSessionCookie(): void
->once()
->with(LogLevel::INFO, 'User session matches the session cookie.', ['correlationId' => 'f6e7cfb6f0861f577c48f171e27542236b1184f7a599dde82aca1640d86da961', 'route' => '/route']);

$listener = new SessionStateListener($mockLogger, new SessionCorrelationIdService($requestStack));
$listener = new SessionStateListener(
$mockLogger,
new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'),
['name' => 'PHPSESSID'],
);

$dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'onKernelRequest']);
$dispatcher->dispatch($event, KernelEvents::REQUEST);
Expand Down
4 changes: 2 additions & 2 deletions tests/Unit/Service/SessionCorrelationIdServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand All @@ -41,7 +41,7 @@ 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());
}
Expand Down
2 changes: 1 addition & 1 deletion tests/Unit/Session/LoggingSessionFactoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public function testItLogsWheneverASessionIsCreated(): void
$requestStack,
$this->createStub(SessionStorageFactoryInterface::class),
$mockLogger,
new SessionCorrelationIdService($requestStack),
new SessionCorrelationIdService($requestStack, ['name' => 'PHPSESSID'], 'Mr6LpJYtuWRDdVR2_7VgTChFhzQ'),
);

$this->assertInstanceOf(SessionInterface::class, $sessionFactory->createSession());
Expand Down

0 comments on commit ffa280b

Please sign in to comment.