Repository navigation
Commit 6e1ff7a
authored
* Add per-SP WAYF remember-choice core
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
* Tidy rememberedidps cookie and feature-flag configuration
# 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
* Encapsulate and harden rememberedidps cookie reading/writing
# 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
* Bound decompression of the client-controlled rememberedidps cookie
# 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
* Simplify remember-choice condition and property access in SSO flow
# 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
* Enforce and document remember-choice mode mutual exclusivity
# 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
* Route the per-SP remember-choice flag through FeatureConfiguration
# 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
* Require the zlib extension explicitly
# 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
1 parent d94160f commit 6e1ff7a
42 files changed
Lines changed: 2010 additions & 27 deletions
File tree
- config
- packages
- services
- docs
- library/EngineBlock/Corto/Module/Service
- src/OpenConext
- EngineBlockBundle
- Bridge
- Controller
- DependencyInjection/Compiler
- Service
- Twig/Extensions/Extension
- ViewModel
- EngineBlock
- Metadata
- Entity
- Assembler
- Service
- Wayf
- tests
- integration/OpenConext/EngineBlock/Metadata/Entity/Assembler
- library/EngineBlock/Test/Corto/Module
- Service
- unit/OpenConext
- EngineBlockBundle
- DependencyInjection/Compiler
- Service
- Twig
- Extensions/Extension
- EngineBlock
- Metadata/Entity
- Service/Wayf
- theme
- base
- javascripts/wayf
- templates/modules/Authentication/View/Proxy
- Partials/WAYF
- idp
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
16 | 27 | | |
17 | 28 | | |
18 | 29 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
| 25 | + | |
25 | 26 | | |
26 | 27 | | |
27 | 28 | | |
| |||
54 | 55 | | |
55 | 56 | | |
56 | 57 | | |
57 | | - | |
58 | 58 | | |
59 | 59 | | |
60 | 60 | | |
| |||
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
| 20 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
179 | 179 | | |
180 | 180 | | |
181 | 181 | | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
182 | 185 | | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
183 | 192 | | |
184 | 193 | | |
185 | 194 | | |
| |||
235 | 244 | | |
236 | 245 | | |
237 | 246 | | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
238 | 251 | | |
239 | 252 | | |
240 | 253 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
18 | 19 | | |
19 | 20 | | |
20 | 21 | | |
| |||
173 | 174 | | |
174 | 175 | | |
175 | 176 | | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
176 | 186 | | |
177 | 187 | | |
178 | 188 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
| 13 | + | |
13 | 14 | | |
14 | 15 | | |
15 | 16 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
16 | 16 | | |
17 | 17 | | |
18 | 18 | | |
19 | | - | |
20 | | - | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
21 | 22 | | |
22 | 23 | | |
23 | 24 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| 22 | + | |
22 | 23 | | |
23 | 24 | | |
24 | 25 | | |
| |||
113 | 114 | | |
114 | 115 | | |
115 | 116 | | |
| 117 | + | |
| 118 | + | |
116 | 119 | | |
117 | 120 | | |
118 | 121 | | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
119 | 184 | | |
120 | 185 | | |
121 | 186 | | |
| |||
0 commit comments