Feature/issue 1971 correlation id logging#1981
Open
kayjoosten wants to merge 11 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.
kayjoosten
force-pushed
the
feature/issue-1971-correlation-id-logging
branch
2 times, most recently
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
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
johanib
force-pushed
the
feature/issue-1971-correlation-id-logging
branch
from
3557fe0 to
464f9e4
Compare
kayjoosten
commented
May 7, 2026
| And I give my consent | ||
| And I pass through EngineBlock | ||
| Then the url should match "functional-testing/CorrId-SP/acs" | ||
| # And I dump the log records |
Contributor
Author
There was a problem hiding this comment.
can this be removed? Or do you want to let it stay so we know later on thats a option?
| use Monolog\Level; | ||
| use Monolog\LogRecord; | ||
|
|
||
| final class TestLogHandler extends AbstractProcessingHandler |
Contributor
Author
There was a problem hiding this comment.
Is this class still needed now we write to files?
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.