Feature/issue 1971 correlation id logging by kayjoosten · Pull Request #1981 · OpenConext/OpenConext-engineblock

kayjoosten · 2026-04-16T12:42:00Z

No description provided.

johanib

The general approach is good! Still, I think some rework to cleanup the architecture & improve the behat tests will add a lot of value.

johanib · 2026-04-20T12:10:43Z

+    private ?string $correlationId = null;
+
+    public function get(): ?string
+    {
+        return $this->correlationId;
+    }
+
+    public function set(string $correlationId): void
+    {
+        $this->correlationId = $correlationId;
+    }


Suggested change

private ?string $correlationId = null;

public function get(): ?string

{

return $this->correlationId;

}

public function set(string $correlationId): void

{

$this->correlationId = $correlationId;

}

public function __construct(public readonly string $correlationId)

{

}

johanib · 2026-04-20T12:11:59Z

+    private const SESSION_KEY = 'CorrelationIds';
+
+    public function __construct(
+        private readonly CorrelationId $correlationId,


Regarding the $correlationId: Should the repository have state? Maybe as memoization? But not in the constructor?

johanib · 2026-04-20T12:15:30Z

+     * Generates and stores a correlation ID for $requestId if none exists yet.
+     * Safe to call multiple times for the same ID (back-button guard).
+     */
+    public function mint(string $requestId): void


Mint seems like a factory(method). Repository should save/read from session?

Probably nicer to add a mint or fromRandomBytes method to the CorrelationId.

johanib · 2026-04-20T12:15:35Z

+        $ids = $session->get(self::SESSION_KEY, []);
+
+        if (!isset($ids[$requestId])) {
+            $ids[$requestId] = bin2hex(random_bytes(16));


I assume the bin2hex is copied from the samlbundle? Otherwise doublecheck with Pieter.

johanib · 2026-04-20T12:21:40Z

    }
+
+    /**
+     * @return \OpenConext\EngineBlock\Request\CorrelationIdRepository


Useless comment & move to the non-deprecated DiContainer

johanib · 2026-04-20T12:33:04Z

+
+    /**
+     * Looks up the correlation ID for $requestId and pushes it into the
+     * CorrelationId DI service so all subsequent log entries carry it.


I'm not sure how to tackle this. The current implementation is not wrong. But it prevents the CorrelationId from functioning as a value object.

Maybe define a new class 'CurrentCorrelationId` and define it as a Singleton in Symfony?

This repository then read/writes to both the session and the CurrentCorrelationId.

johanib · 2026-04-20T12:43:31Z

+     * CorrelationId DI service so all subsequent log entries carry it.
+     * No-op when $requestId is null or not found.
+     */
+    public function resolve(?string $requestId): void


This function now has two responsibilities: Read the cid from session and update the singleton state.

I think separating these responsibilities in the classic Service <> Repository pattern helps keep things untangled.

So introduce a CorrelationIdService, and inject this Repository so it can write to the session.
This repository class can than be responsible for the interaction with the session.

The service can then track the $correlationId in a static for example.

johanib · 2026-04-20T13:02:32Z

+  # not bleed into each other.  Both flows must complete successfully and land
+  # on the correct SP ACS URL.
+  # Requires the @functional tag to use the Chrome driver (browser tabs need JS).
+  @functional


🔥 Very nice that this actually tests the feature in chrome.

Not sure if I'm missing something, but I think the logs are not actually asserted, like in https://github.com/OpenConext/Stepup-gssp-bundle/blob/main/src/Features/registration.feature ?

johanib · 2026-04-20T13:15:47Z

        $application = EngineBlock_ApplicationSingleton::getInstance();
+
+        $correlationIdRepository = $application->getDiContainer()->getCorrelationIdRepository();
+        $correlationIdRepository->resolve($receivedResponse->getInResponseTo());


This goes for all changes in the Corto\Module\Service dir:

Have you considered adding the resolve inside the getReceivedRequestFromResponse function? This very roughly seems to map with the points in code where you added the resolve.
It does get called from some more places. With your current solution, we do have more fine grained control. On the other hand, that function gets called from the SRAM flow, which might fall outside the boat in your current implementation.

Introduces components to address issue #1971: - CorrelationId: immutable value object wrapping a hex correlation ID - CurrentCorrelationId: mutable DI singleton holding the active correlation ID for the current HTTP request; read by the Monolog processor - CorrelationIdRepository: session-backed store with store/find/link methods; no-ops safely when no session is available (CLI, unit tests) - CorrelationIdService: orchestrator with mint/link/resolve used by Corto: mint(requestId) — generate a new ID if none exists (back-button safe) link(target, src) — copy an ID to a second SAML request ID resolve(requestId) — look up and push into CurrentCorrelationId - CorrelationIdProcessor: Monolog processor stamping correlation_id on every log record via CurrentCorrelationId - TestLogHandler: in-memory Monolog handler for Behat log assertions, registered in ci and test monolog config DI wiring: services.yml registers all services; logging.yml registers the processor. DiContainer exposes getCorrelationIdService() as the bridge from legacy Corto code into Symfony.

Migrates AuthnRequestSessionRepository from \$_SESSION to the Symfony session (via RequestStack) and registers it as a DI service, so all call sites use DiContainer instead of constructing it with a logger. Each HTTP leg resolves the correlation ID at the top of its handler: Leg 1 SSO — mint() + resolve() in SingleSignOn (WAYF path); mint() + link() + resolve() in ProxyServer (direct path) Leg 2 ContinueToIdp — resolve() so log lines in this leg carry the ID; ProxyServer also calls link() to tie the IdP request ID to the SP request ID (idempotent second resolve) Leg 3 ACS — resolve() via InResponseTo (IdP request ID) Leg 4 Consent — resolve() via SP request ID in ProvideConsent and ProcessConsent

Unit tests: - CorrelationIdRepositoryTest: store/find/link + SessionNotFoundException safety - CorrelationIdServiceTest: mint idempotency, link, resolve, null safety - CorrelationIdFlowTest: end-to-end simulation of all four SAML legs (WAYF, direct, concurrent flows, back-button replay guard) - CorrelationIdProcessorTest: stamps correlation_id; null when not set - AuthnRequestSessionRepositoryTest: updated to inject RequestStack + MockArraySessionStorage (logger constructor removed) - ProcessConsentTest / ProvideConsentTest: inject RequestStack-backed repository; stub getReceivedRequestFromResponse for isolation Behat: - CorrelationId.feature: WAYF and direct path scenarios assert every log record carries a non-null correlation_id field - LoggingContext: @BeforeScenario reset + "each log record should contain a :field field" step - TestLogHandler wired into behat.yml default suite contexts

…rver

…erty

kayjoosten force-pushed the feature/issue-1971-correlation-id-logging branch from 8ee83d0 to bf1e8e1 Compare April 16, 2026 14:50

kayjoosten requested a review from johanib April 17, 2026 09:24

johanib requested changes Apr 20, 2026

View reviewed changes

kayjoosten force-pushed the feature/issue-1971-correlation-id-logging branch from bf1e8e1 to 73b3503 Compare April 21, 2026 15:30

kayjoosten added 3 commits April 21, 2026 17:46

kayjoosten force-pushed the feature/issue-1971-correlation-id-logging branch from 73b3503 to 7e17a65 Compare April 21, 2026 16:08

kayjoosten added 4 commits April 21, 2026 18:25

fix: restore accidentally removed services.yml arguments

daf0c2c

fix: remove log-assertion steps that cannot work with external web se…

fd8f802

…rver

style: remove inline docblocks, fix copyright year to 2026 on new files

3c4b4f2

refactor: replace CurrentCorrelationId getter/setter with public prop…

e84276d

…erty

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/issue 1971 correlation id logging#1981

Feature/issue 1971 correlation id logging#1981
kayjoosten wants to merge 7 commits intomainfrom
feature/issue-1971-correlation-id-logging

kayjoosten commented Apr 16, 2026

Uh oh!

johanib left a comment

Uh oh!

johanib Apr 20, 2026

Uh oh!

johanib Apr 20, 2026

Uh oh!

johanib Apr 20, 2026

Uh oh!

johanib Apr 20, 2026

Uh oh!

johanib Apr 20, 2026

Uh oh!

johanib Apr 20, 2026

Uh oh!

johanib Apr 20, 2026

Uh oh!

johanib Apr 20, 2026

Uh oh!

johanib Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kayjoosten commented Apr 16, 2026

Uh oh!

johanib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants