Feature/issue 1971 correlation id logging#1981
Open
kayjoosten wants to merge 8 commits intomainfrom
Open
Conversation
kayjoosten
force-pushed
the
feature/issue-1971-correlation-id-logging
branch
from
8ee83d0 to
bf1e8e1
Compare
johanib
requested changes
Apr 20, 2026
Contributor
johanib
left a comment
johanib
left a comment
There was a problem hiding this comment.
The general approach is good! Still, I think some rework to cleanup the architecture & improve the behat tests will add a lot of value.
| } | ||
|
|
||
| /** | ||
| * @return \OpenConext\EngineBlock\Request\CorrelationIdRepository |
Contributor
There was a problem hiding this comment.
Useless comment & move to the non-deprecated DiContainer
| $application = EngineBlock_ApplicationSingleton::getInstance(); | ||
|
|
||
| $correlationIdRepository = $application->getDiContainer()->getCorrelationIdRepository(); | ||
| $correlationIdRepository->resolve($receivedResponse->getInResponseTo()); |
Contributor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
After discussion: I think your approach makes sense: At key entry points, set the id to session. From then on, load into extra logger context at the earliest possible moment.
I'll dive into this before approving.
kayjoosten
force-pushed
the
feature/issue-1971-correlation-id-logging
branch
from
bf1e8e1 to
73b3503
Compare
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
kayjoosten
force-pushed
the
feature/issue-1971-correlation-id-logging
branch
from
73b3503 to
7e17a65
Compare
kayjoosten
force-pushed
the
feature/issue-1971-correlation-id-logging
branch
from
e84276d to
e5e4d1f
Compare
johanib
reviewed
Apr 23, 2026
| { | ||
| $records = $this->logHandler->getRecords(); | ||
|
|
||
| Assert::assertNotEmpty($records, 'No log records were captured during this scenario.'); |
Contributor
There was a problem hiding this comment.
This does not check the actual logs during eb execution, just the logs that behat generates 🤓
Contributor
There was a problem hiding this comment.
I pushed the change we just discussed.
| $application = EngineBlock_ApplicationSingleton::getInstance(); | ||
|
|
||
| $correlationIdRepository = $application->getDiContainer()->getCorrelationIdRepository(); | ||
| $correlationIdRepository->resolve($receivedResponse->getInResponseTo()); |
Contributor
There was a problem hiding this comment.
After discussion: I think your approach makes sense: At key entry points, set the id to session. From then on, load into extra logger context at the earliest possible moment.
I'll dive into this before approving.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.