feat(stepup): add service display name to Stepup callout AuthnRequest - #2034
feat(stepup): add service display name to Stepup callout AuthnRequest#2034kayjoosten wants to merge 2 commits into
Conversation
b0887cb to
168d03d
Compare
860bfac to
c71e61d
Compare
| final class StepupServiceNameExtension | ||
| { | ||
| private const MDUI_NS = 'urn:oasis:names:tc:SAML:metadata:ui'; | ||
| private const SUPPORTED_LOCALES = ['en', 'nl', 'pt']; |
There was a problem hiding this comment.
Instead of hardcoding here, read symfony values or values?
| public static function add(Message $message, ServiceProvider $sp, string $locale): void | ||
| { | ||
| if (!in_array($locale, self::SUPPORTED_LOCALES, true)) { | ||
| $locale = 'en'; |
There was a problem hiding this comment.
fallback to global/symfony default?
| $locale = 'en'; | ||
| } | ||
| $result = self::resolveName($sp, $locale); | ||
| if ($result === null && $locale !== 'en') { |
There was a problem hiding this comment.
This assumes the default locale === 'en'. Would be better to do $locale !== $defaultLocale
If applied, this commit will make the Stepup service-display-name
extension use EngineBlock's configured default/available locales
instead of a hardcoded locale set, and drop a dynamic property lookup
in favor of an explicit match.
Why is this change needed?
StepupServiceNameExtension hardcoded SUPPORTED_LOCALES = ['en', 'nl',
'pt'] and fell back to the literal string 'en', independent of how the
instance is actually configured. It also read the localized name via
$sp->{'name' . ucfirst($locale)}, a dynamic property access that
duplicates logic already present in
ServiceProvider::getDisplayName().
How does it address the issue?
Adds getDefaultLocale() and getAvailableLocales() to LocaleProvider,
and passes both into StepupServiceNameExtension::add() from
ProxyServer, replacing the hardcoded whitelist and 'en' literal.
Replaces the dynamic property read with an explicit match over the
three schema-backed name columns. Adds a CHANGELOG entry for the
eb.stepup.send_service_name feature flag.
Links / references:
#2034
Stepup currently receives a callout AuthnRequest without the requesting service's display name. That leaves the Stepup UI showing a generic proxy label instead of the application the user is logging in to, which makes the step-up prompt less clear. Add a dedicated Stepup AuthnRequest extension that emits the service DisplayName in the SAML extensions payload when the feature flag is enabled. The value follows the current EngineBlock locale, falls back to English when needed, and now validates the locale before resolving dynamic name fields so unsupported locales cannot break lookup. Resolves: #2011
If applied, this commit will make the Stepup service-display-name
extension use EngineBlock's configured default/available locales
instead of a hardcoded locale set, and drop a dynamic property lookup
in favor of an explicit match.
Why is this change needed?
StepupServiceNameExtension hardcoded SUPPORTED_LOCALES = ['en', 'nl',
'pt'] and fell back to the literal string 'en', independent of how the
instance is actually configured. It also read the localized name via
$sp->{'name' . ucfirst($locale)}, a dynamic property access that
duplicates logic already present in
ServiceProvider::getDisplayName().
How does it address the issue?
Adds getDefaultLocale() and getAvailableLocales() to LocaleProvider,
and passes both into StepupServiceNameExtension::add() from
ProxyServer, replacing the hardcoded whitelist and 'en' literal.
Replaces the dynamic property read with an explicit match over the
three schema-backed name columns. Adds a CHANGELOG entry for the
eb.stepup.send_service_name feature flag.
Links / references:
#2034
cc099f9 to
fde41c1
Compare
| } | ||
|
|
||
| $isSendServiceNameConfigured = $features->hasFeature('eb.stepup.send_service_name'); | ||
| $isSendServiceNameEnabled = $features->isEnabled('eb.stepup.send_service_name'); |
There was a problem hiding this comment.
move to inline if, so it doesnt get called if feature flag is not set
johanib
left a comment
johanib
left a comment
There was a problem hiding this comment.
Maybe check dots on i, looks good, no regression 👍
|
and check commit message |
johanib
left a comment
johanib
left a comment
There was a problem hiding this comment.
On second thought, we discussed applying the constraints on the servicename from the RFC at the source. So, here in EB.
Can we add the stepup bundle that supplies the sanitization logic so EB can sanitize it beforehand?
Tbh i dont see the benefit, it gets sanitized or overwritten in gateway either way. I know this is something we said in the standup but i think to leave it up to gateway as there the final value will be send out. What do you think about that? |
If these were official saml specs, we should apply it here. But, as long as we keep within the saml extension fields limit, it should be ok. Applying this only in gateway seems sensible. |
2 participants
Summary
mdui:UIInfo/mdui:DisplayNameto<samlp:Extensions>of the Stepup Gateway SFO callout AuthnRequest, so Stepup can surface the correct service name in its 2FA UI instead of the generic proxy nameeb.stepup.send_service_name(disabled by default)name*fieldsTechnical notes
StepupServiceNameExtensionclass insrc/OpenConext/EngineBlock/Stepup/follows the same pattern asStepupGsspUserAttributeExtensionxml:langattribute set viasetAttributeNS(XML namespace) so XPath and SAML consumers resolve it correctlyLocaleProvider::getLocale()available throughDiContainerTest plan
StepupServiceNameExtensionTest)Stepup.feature(includes 2 new scenarios: feature enabled / disabled)phpcsanddocheaderclean on new files