Implement per-SP WAYF remember-choice core (#2065) by kayjoosten · Pull Request #2091 · OpenConext/OpenConext-engineblock

kayjoosten · 2026-09-14T10:54:05Z

Summary

Implements the EngineBlock backend core for per-SP WAYF remembered IdP choices, the first vertical slice of #2065 (part of epic #1956 — "Remember my Choice for this Service" feature in WAYF).

This PR is intentionally scoped to a safe, independently testable backend slice. It preserves all existing behavior (one-IdP shortcut, SSO-notification shortcut, and the existing global rememberchoice cookie feature) and adds a new, disabled-by-default, opt-in per-SP mode.

What this PR adds

Configuration: feature_enable_wayf_remember_choice_per_idp (default false), wayf.remember_choice_per_idp_lifetime (default 7776000s / 90 days), wayf.remember_choice_per_idp_max (default 16).
Metadata gate: new SP metadata coin coin:wayf_remember_choice (Manage metadata:coin:wayf_remember_choice), defaulting to false, with strict boolean parsing so only true/"1" opt an SP in.
RememberedIdpCookie service (src/OpenConext/EngineBlock/Service/Wayf/): a deterministic codec/policy service for the new rememberedidps cookie — JSON entries keyed by SP entity ID, deflated + base64 encoded, with strict validation (invalid data is always treated as absent and cleared, never used for IdP selection), expiry cleanup on every read, and over-limit pruning only when a new choice is written (never during passive reads).
Runtime bridge: the cookie service and per-SP feature flag are exposed via DiContainerRuntime (not DiContainer), matching the legacy/modern bridge convention for new dependencies.
Read-side bypass (SingleSignOn): when the per-SP flag and the SP's metadata coin are enabled (and the request is not ForceAuthn/debug), a valid remembered IdP that is still a candidate IdP skips the WAYF automatically. The existing one-IdP shortcut and SSO-notification shortcut remain unconditionally first. The legacy global rememberchoice cookie path is now only used when per-SP mode is disabled.
Write-side persistence (ContinueToIdp): when a user has explicitly checked "remember my choice" and the per-SP flag and the SP metadata coin are enabled, the (SP, IdP) pair is persisted in the cookie with a 90-day expiry. Cookie hygiene (invalid/expired cleanup) always runs regardless of whether the checkbox was checked.
Minimal frontend signal: a hidden rememberChoice field is added to the WAYF's per-IdP form. In per-SP mode, checking the existing checkbox sets this field instead of creating the legacy client-side cookie; the checkbox itself is visually unchanged.
Cookie attributes: the new cookie is Secure, HttpOnly, SameSite=None, scoped to the EngineBlock host only (not the wildcard locale-cookie domain), with the configured lifetime.

Mutually exclusive modes

The global mode (wayf.remember_choice) and the per-SP mode (feature_enable_wayf_remember_choice_per_idp) are mutually exclusive. The per-SP mode no longer requires wayf.remember_choice. If both are true, a compiler pass fails the container build (cache:clear / cache:warmup) with an InvalidConfigurationException. See docs/wayf_remember_choice.md.

What this PR does not include (tracked as follow-up work)

Redesign "Remember my choice" checkbox in WAYF #2066 — checkbox/tooltip visual redesign (existing checkbox markup/wording is unchanged here)
Add endpoint to remove "remember my choice" cookie #2085 — EngineBlock endpoint to reset/remove the cookie
Support global reset of all per-SP "remember my choice" entries for the WAYF OpenConext-profile#345 and Add "cookies have been removed" feedback to profile OpenConext-profile#350 — Profile integration (reset action + feedback)
Cypress / behat tests for remember my choice epic #2084 — full Cypress/Behat acceptance coverage for the epic (this PR adds focused unit/integration coverage only)
Support per-SP reset "remember my choice" entries for the WAYF OpenConext-profile#346 and Improve UI for global reset "remember my choice" cookies OpenConext-profile#348 remain on hold per the epic discussion

Recommended merge order after this PR: #2085 → #2066 → Profile #345/#350 → #2084.

Testing

All run inside the project's PHP 8.5 Docker container as www-data:

phpmd, phpcs (both src and legacy library rulesets), docheader: clean
PHPUnit eb4: 244/244 passed
PHPUnit unit: 998/998 passed
PHPUnit integration: 105/105 passed
PHPUnit functional (APP_ENV=test): 119/119 passed
Behat default suite: 300/300 scenarios, 5557/5557 steps passed
lint:twig theme/: 114/114 valid
theme && yarn lint: passed

New focused test coverage includes:

RememberedIdpCookieTest — codec round-trip, all invalid-input cases, expiry cleanup, pruning-only-on-add, candidate-safe lookup, cookie write/clear, and the shared cookie-hygiene/gating helpers
WayfScriptConfigRenderTest — a real-Twig-render regression test ensuring the per-SP flag actually reaches the rendered WAYF page (this caught and fixed a template-context bug during development)
ContinueToIdpTest — write-gate coverage for the persistence decision
Updated DiContainerRuntimeTest, WayfRendererTest, WayfTest, PushMetadataAssemblerTest, ServiceProviderTest, BindingsTest

Behavior preserved

One-candidate-IdP shortcut and SSO-notification shortcut remain first, unconditionally
Existing global rememberchoice behavior is byte-for-byte unchanged when feature_enable_wayf_remember_choice_per_idp is false
An SP without the new metadata coin never uses the new per-SP cookie and never silently falls back to the old global cookie once per-SP mode is enabled

# If applied, this commit will Configure the rememberedidps cookie to omit the Domain attribute. # Why is this change needed? Prior to this change, the rememberedidps cookie was scoped to the configured hostname domain and shared with subdomains. # How does it address the issue? This change passes an empty cookie domain so PHP emits a host-only cookie. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2091.

# If applied, this commit will Log an error and skip success logging when writing the rememberedidps cookie fails. # Why is this change needed? Prior to this change, a failed rememberedidps cookie write was still logged as a successful remember-choice update. # How does it address the issue? This change checks the cookie write result, logs an error for failures, and covers the behavior with a regression test. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2091.

johanib

WIP review..

I feel I'm only at 20%..
Next time, ill start with understanding $request->getForceAuthn()

johanib · 2026-09-28T09:22:56Z

+    {
+        set_error_handler(static fn(): bool => true);
+        try {
+            $inflated = gzinflate($decoded);


ext-zlib is not yet an explicit dependency in composer.json
Is that a new requirement? Or are we now using it ourselves, but was it already a implicit dep?

If not: Can we prevent making it a requirement?
(it seems this is almost always enabled by default, so just making it explicit seems the way to go).

# If applied, this commit will... Rename the wayf.remember_choice_per_idp parameter to match the feature_enable_ naming convention used by other boolean toggles in this file. # Why is this change needed? Prior to this change, this flag used a bare wayf.* key while sibling feature flags later in the same file follow a feature_enable_* convention, which was raised in review. # How does it address the issue? This change renames the parameter key in parameters.yml.dist and the service binding in services.yml. PHP-side property and method names are unchanged. # Provide links to any relevant tickets, articles or other resources #2091

# If applied, this commit will... Remove the COIN_WAYF_REMEMBER_CHOICE constant and use the literal string directly. # Why is this change needed? Prior to this change, this constant had exactly one production call site and one test assertion, which was flagged in review as not earning its keep as a constant. # How does it address the issue? This change inlines the literal 'wayf_remember_choice' at both usage sites and removes the constant declaration. # Provide links to any relevant tickets, articles or other resources #2091

# If applied, this commit will... Add RememberedIdpCookie::loadValidEntriesFromRequest() and use it at both call sites that previously read the cookie directly. # Why is this change needed? Prior to this change, ContinueToIdp and SingleSignOn both plucked the rememberedidps cookie off the Symfony request and passed it to loadValidEntries() themselves, duplicating the same few lines and leaking cookie-reading details into legacy Corto code, which was raised in review. # How does it address the issue? This change adds a loadValidEntriesFromRequest() method on RememberedIdpCookie that reads the cookie from the request itself, and updates both call sites to use it instead of plucking the cookie directly. # Provide links to any relevant tickets, articles or other resources #2091

# If applied, this commit will... Add an explicit 64KB max_length bound to the gzinflate() call used to decode the rememberedidps cookie. # Why is this change needed? Prior to this change, inflate() called gzinflate() with no explicit output-size bound, which was flagged in review: a small, highly compressible attacker-crafted cookie should never be allowed to decompress into an unbounded buffer. # How does it address the issue? This change passes a 65536-byte max_length to gzinflate(), so an oversized or maliciously compressed cookie is rejected as invalid instead of being decompressed without limit. # Provide links to any relevant tickets, articles or other resources #2091

…eSignOn # If applied, this commit will... Extract two multi-line boolean conditions in SingleSignOn into named local variables. # Why is this change needed? Prior to this change, both conditions mixed a multi-line function call or comparison with trailing || continuation lines indented deeper than the if(, which was flagged in review as hard to read ('broken indent?' and 'rewrite to human readable ifs'). # How does it address the issue? This change assigns each condition to a clearly named boolean ($legacyRememberChoiceAllowed, $notEligibleForRememberedIdp) before the if statement, with no change in behavior. # Provide links to any relevant tickets, articles or other resources #2091

# If applied, this commit will... Make $rememberChoicePerIdp public on DiContainerRuntime and remove the isRememberChoicePerIdpEnabled() getter. # Why is this change needed? Prior to this change, this was the only property on this readonly class guarded by a private property plus getter; every sibling property is accessed directly, which was raised in review as redundant given the class is already readonly. # How does it address the issue? This change makes the property public, matching the rest of the class, and updates the four call sites and two unit test assertions to read $runtime->rememberChoicePerIdp directly. # Provide links to any relevant tickets, articles or other resources #2091

@SuppressWarnings

# If applied, this commit will... Add a class-level PHPMD suppression for ExcessiveClassComplexity on RememberedIdpCookie. # Why is this change needed? Prior to this change, phpmd was clean on this class; the loadValidEntriesFromRequest() helper added earlier in this branch pushes the class's weighted method count to exactly the default threshold of 50, failing the ci/qa-config/phpmd.xml check. # How does it address the issue? This change adds a targeted @SuppressWarnings(PHPMD.ExcessiveClassComplexity) docblock on the class, the same pattern already used for other PHPMD exceptions in this codebase (e.g. DiContainerRuntime), rather than splitting the class or reverting the encapsulation improvement. # Provide links to any relevant tickets, articles or other resources #2091

johanib

99% there 💯 .
Lets merge after last iteration is done!

johanib

merge after moving gzip to reqs instead of dev reqs

Introduce the backend core for remembering a user's chosen IdP on a per-SP basis, gated behind two independent opt-ins: an EngineBlock-wide config flag and the SP's own Manage metadata coin. This replaces, for opted-in SPs, the existing single global "remember choice" cookie with a signed-per-entry cookie keyed by SP entityId, so different SPs can have independent (and independently expiring) remembered IdPs. Only true/"1" values for the SP metadata coin enable the feature for that SP; any other value (including "false" or "-1") leaves it disabled, so a malformed Manage value can never silently turn on choice-remembering behaviour. - Coins/ServiceProvider/PushMetadataAssembler gain the wayf_remember_choice coin (metadata:coin:wayf_remember_choice), defaulting to false and parsed with strict true-only semantics. - RememberedIdpCookie encodes/decodes the "rememberedidps" cookie (deflate + base64 JSON, one {idp, expires} entry per SP entityId), validates entry shape defensively, drops expired entries on every read, and prunes down to a configurable maximum (keeping the highest-expiry entries) only when a new entry is added. - DiContainerRuntime/EngineBlockBootstrapper bridge the new cookie service and the per-SP feature flag into the legacy Corto layer. - SingleSignOn bypasses WAYF using a remembered IdP when the SP has opted in and the choice is still valid for one of the candidate IdPs, preserving the existing zero/one-candidate and SSO-notification shortcuts; the legacy global "rememberchoice" cookie path is now gated off for SPs using the new per-SP mode instead of double-firing. - ContinueToIdp persists a checked "remember this choice" selection into the cookie and normalizes/prunes it on every request regardless of whether this SP is enrolled, so stale entries age out naturally. - The WAYF view model, Twig templates and rememberChoice.js are wired so the per-SP checkbox submits a hidden form field instead of writing a client-side cookie, leaving the legacy global-cookie behaviour untouched for SPs that haven't opted in. Refs: #2065

# If applied, this commit will... Make the rememberedidps cookie host-only and rename the per-SP WAYF remember-choice flag to match the feature_enable_ naming convention. # Why is this change needed? Prior to this change, the rememberedidps cookie was scoped to the configured hostname domain and shared with subdomains, and the wayf.remember_choice_per_idp parameter used a bare wayf.* key while sibling feature flags in the same file follow a feature_enable_* convention. Both were raised in review. # How does it address the issue? This change passes an empty cookie domain so PHP emits a host-only cookie, and renames the parameter key in parameters.yml.dist and the service binding in services.yml. PHP-side property and method names are unchanged. # Provide links to any relevant tickets, articles or other resources #2091

# If applied, this commit will... Add RememberedIdpCookie::loadValidEntriesFromRequest() for both read call sites, log an error instead of silently succeeding when a cookie write fails, and suppress the resulting PHPMD complexity warning. # Why is this change needed? Prior to this change, ContinueToIdp and SingleSignOn both plucked the rememberedidps cookie off the Symfony request directly, duplicating the same lines and leaking cookie-reading details into legacy Corto code; a failed cookie write was still logged as a successful remember-choice update. Both were raised in review. # How does it address the issue? This change adds loadValidEntriesFromRequest() on RememberedIdpCookie and updates both call sites to use it, checks the cookie write result and logs an error on failure (covered by a regression test), and adds a targeted PHPMD suppression for the resulting class complexity, matching the existing pattern used for DiContainerRuntime. # Provide links to any relevant tickets, articles or other resources #2091

# If applied, this commit will... Add an explicit 64KB max_length bound to the gzinflate() call used to decode the rememberedidps cookie. # Why is this change needed? Prior to this change, inflate() called gzinflate() with no explicit output-size bound, which was flagged in review: a small, highly compressible attacker-crafted cookie should never be allowed to decompress into an unbounded buffer. # How does it address the issue? This change passes a 65536-byte max_length to gzinflate(), so an oversized or maliciously compressed cookie is rejected as invalid instead of being decompressed without limit. # Provide links to any relevant tickets, articles or other resources #2091

# If applied, this commit will... Extract two multi-line boolean conditions in SingleSignOn into named local variables, and make rememberChoicePerIdp a public property on DiContainerRuntime instead of a private property plus getter. # Why is this change needed? Prior to this change, both conditions mixed a multi-line function call or comparison with trailing || continuation lines indented deeper than the if(, and rememberChoicePerIdp was the only property on this readonly class guarded by a getter while every sibling property is accessed directly. Both were raised in review. # How does it address the issue? This change assigns each condition to a clearly named boolean ($legacyRememberChoiceAllowed, $notEligibleForRememberedIdp) with no change in behavior, and makes the property public, updating the four call sites and two unit test assertions to read $runtime->rememberChoicePerIdp directly. # Provide links to any relevant tickets, articles or other resources #2091

# If applied, this commit will... Make the per-SP and global WAYF remember-my-choice modes mutually exclusive, and make the cookie removal page available in per-SP mode. # Why is this change needed? Prior to this change, the per-SP remember-my-choice mode only worked when the global wayf.remember_choice was also enabled, so both features were intertwined, and the business wants exactly one of the two modes active. Additionally, /authentication/idp/remove-cookies only worked when wayf.remember_choice was true, so a deployment using only the per-SP mode had no cookie removal page at all and the rememberedidps cookie was never listed on it. # How does it address the issue? This change makes the per-SP mode independent of wayf.remember_choice and adds a compiler pass that fails the container build when both flags are true, so a misconfiguration is caught during cache:clear/cache:warmup at deploy time. The cookie removal page is now shown when either mode is enabled and lists the rememberedidps cookie, clearing it through RememberedIdpCookie so the configured path and domain are honoured. The modes, their parameters, the exclusivity rule and the new page behaviour are documented in docs/wayf_remember_choice.md, parameters.yml.dist, the theme README and the CHANGELOG. # Provide links to any relevant tickets, articles or other resources #2091

# If applied, this commit will... Route feature_enable_wayf_remember_choice_per_idp through the existing FeatureConfiguration mechanism instead of injecting the raw parameter directly, reuse it for the mutual-exclusivity check in the compiler pass, and guard against the key being unknown to a given FeatureConfiguration instance. # Why is this change needed? Prior to this change, the per-SP remember-choice flag was wired into EngineBlockBootstrapper as a plain boolean parameter, and RememberChoiceModeCompilerPass duplicated its own ad hoc boolean parsing for the mutual-exclusivity check, which did not follow the generic custom_features/FeatureConfiguration mechanism already used by every other feature toggle. EngineBlockBootstrapper also runs on every request via kernel.request, and several existing functional tests override FeatureConfiguration with a partial mock containing only their own scenario's keys, so calling isEnabled() directly on the new key threw when it was missing, causing a 302 redirect instead of the expected 404 on seven unrelated controller tests. # How does it address the issue? This change registers eb.feature_enable_wayf_remember_choice_per_idp in custom_features, moves its parameter declaration into the feature settings section of parameters.yml.dist, and has EngineBlockBootstrapper resolve it via FeatureConfigurationInterface::hasFeature()/isEnabled() rather than a raw parameter binding, treating a missing key as disabled instead of throwing. RememberChoiceModeCompilerPass now builds a small FeatureConfiguration instance from the two resolved parameters and checks both through isEnabled(). Public constants, the exception type and message stay unchanged, and the bootstrapper's unit tests are updated/extended to cover feature resolution including the missing-key case. # Provide links to any relevant tickets, articles or other resources #2091

# If applied, this commit will... Move the ext-zlib declaration from require-dev to require in composer.json. # Why is this change needed? Prior to this change, ext-zlib was only declared as a dev dependency, but RememberedIdpCookie uses gzdeflate()/gzinflate() directly in production code, which was raised in review. # How does it address the issue? This change moves ext-zlib to the require section so Composer enforces it as a hard production dependency, and refreshes composer.lock's content hash and platform metadata accordingly. # Provide links to any relevant tickets, articles or other resources #2091

kayjoosten force-pushed the wayf-remember-choice-core branch from bc590c8 to 5ab851e Compare September 15, 2026 08:03

This was referenced Sep 15, 2026

Add endpoint to reset the per-SP WAYF remember-choice cookie #2094

Merged

Redesign remember-my-choice checkbox in WAYF #2095

Merged

kayjoosten requested a review from johanib September 24, 2026 08:30

johanib reviewed Sep 28, 2026

View reviewed changes

Comment thread src/OpenConext/EngineBlock/Service/Wayf/RememberedIdpCookie.php Outdated

johanib reviewed Oct 6, 2026

View reviewed changes

Comment thread src/OpenConext/EngineBlock/Metadata/Entity/Assembler/PushMetadataAssembler.php

Comment thread src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php Outdated

johanib reviewed Oct 6, 2026

View reviewed changes

Comment thread library/EngineBlock/Corto/Module/Service/SingleSignOn.php Outdated

johanib reviewed Oct 6, 2026

View reviewed changes

Comment thread library/EngineBlock/Corto/Module/Service/SingleSignOn.php

johanib reviewed Oct 8, 2026

View reviewed changes

Comment thread config/packages/parameters.yml.dist Outdated

johanib reviewed Oct 8, 2026

View reviewed changes

Comment thread ...OpenConext/EngineBlockBundle/DependencyInjection/Compiler/RememberChoiceModeCompilerPass.php

johanib requested changes Oct 8, 2026

View reviewed changes

johanib approved these changes Oct 12, 2026

View reviewed changes

kayjoosten added 8 commits October 12, 2026 14:07

kayjoosten force-pushed the wayf-remember-choice-core branch from 9e96c15 to 31c4f4f Compare October 12, 2026 12:31

kayjoosten merged commit 6e1ff7a into main Oct 12, 2026
2 checks passed

kayjoosten deleted the wayf-remember-choice-core branch October 12, 2026 12:59

Navigation Menu

Repository navigation

Implement per-SP WAYF remember-choice core (#2065) - #2091

Implement per-SP WAYF remember-choice core (#2065)#2091
kayjoosten merged 8 commits into
mainfrom
wayf-remember-choice-core

kayjoosten commented Sep 14, 2026 •

edited

Loading

Uh oh!

johanib left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johanib Sep 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johanib left a comment

Uh oh!

johanib left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kayjoosten commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What this PR adds

Mutually exclusive modes

What this PR does not include (tracked as follow-up work)

Testing

Behavior preserved

Uh oh!

johanib left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johanib Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johanib left a comment

Choose a reason for hiding this comment

Uh oh!

johanib left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kayjoosten commented Sep 14, 2026 •

edited

Loading