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 1 commit into
Conversation
b0887cb to
168d03d
Compare
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
860bfac to
c71e61d
Compare
| } | ||
| } | ||
|
|
||
| $isSendServiceNameConfigured = $features->hasFeature('eb.stepup.send_service_name'); |
There was a problem hiding this comment.
huh, why not only rely on isSendServiceNameEnabled?
| bool $isForceAuthn, | ||
| Assertion $originalAssertion | ||
| Assertion $originalAssertion, | ||
| ?ServiceProvider $sp = null, |
There was a problem hiding this comment.
Instead of passing the ServiceProvider down the line, consider passing the string $serviceName.
This makes the code more flexible and testable, as you do not need a instantiated $sp here. This also removes the $container access for the locale.
| 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
| $uiInfo->appendChild($displayName); | ||
|
|
||
| $ext = $message->getExtensions(); | ||
| $ext['mdui:UIInfo'] = new Chunk($uiInfo); |
There was a problem hiding this comment.
What if $ext already contains mdui:UIInfo.
I would guess this is never the case, and this is correct, so no change warranted.
But good to double check.
| /** | ||
| * @return array{string, string}|null | ||
| */ | ||
| private static function resolveName(ServiceProvider $sp, string $locale): ?array |
There was a problem hiding this comment.
This function is duplicated business logic?
| feature_enable_idp_initiated_flow: true | ||
| feature_stepup_sfo_override_engine_entityid: false | ||
| feature_stepup_send_user_attributes: false | ||
| feature_stepup_send_service_name: false |
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