From 58a8c340dfbbe9ea6850f4cf8a6dc2d854b84802 Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Thu, 16 Apr 2026 16:04:17 +0200 Subject: [PATCH 1/6] Add support for preferred IdPs in WAYF display Prior to this change, there was no way to configure priority IdPs. This change adds a `wayf.preferred_idp_entity_ids` parameter to configure IdPs that should show prominent in the wayf. IdPs in this list are shown on top of the wayf, outside of the regular list. Resolves #1970 --- CHANGELOG.md | 3 + config/packages/parameters.yml.dist | 5 + config/services/services.yml | 8 + docs/testing.md | 52 ++++++ .../Corto/Module/Service/SingleSignOn.php | 46 +++-- .../EngineBlock/Service/Wayf/IdpSplitter.php | 58 ++++++ .../Bridge/DiContainerRuntime.php | 13 +- .../Bridge/EngineBlockBootstrapper.php | 7 +- .../Service/WayfViewModelFactory.php | 75 ++++++++ .../Twig/Extensions/Extension/Locale.php | 8 +- .../ViewModel/WayfViewModel.php | 59 ++++++ .../Controllers/WayfController.php | 54 ++++-- .../skeune/wayf/wayf.general.spec.js | 7 + .../skeune/wayf/wayf.preferred.spec.js | 98 ++++++++++ .../Test/Corto/Module/BindingsTest.php | 4 +- .../Service/Wayf/IdpSplitterTest.php | 170 ++++++++++++++++++ .../DiContainerRuntimeTest.php | 53 ++++++ .../Twig/Extensions/Extension/WayfTest.php | 3 +- theme/base/javascripts/selectors.js | 2 + .../wayf/utility/switchIdpSection.js | 6 +- theme/base/stylesheets/pages/wayf.scss | 1 + .../stylesheets/pages/wayf/preferredIdps.scss | 14 ++ .../stylesheets/pages/wayf/remainingIdps.scss | 4 + .../Proxy/Partials/WAYF/backLink.html.twig | 4 +- .../Proxy/Partials/WAYF/idp/idp.html.twig | 11 +- .../Proxy/Partials/WAYF/idp/idpForm.html.twig | 4 +- .../Proxy/Partials/WAYF/idp/idpItem.html.twig | 9 +- .../Proxy/Partials/WAYF/idp/idpList.html.twig | 22 ++- .../Partials/WAYF/noAccess/noAccess.html.twig | 6 +- .../WAYF/noAccess/requestForm.html.twig | 2 +- .../Partials/WAYF/preferredIdps.html.twig | 7 + .../Partials/WAYF/preselection.html.twig | 14 +- .../Partials/WAYF/remainingIdps.html.twig | 44 +++-- .../Authentication/View/Proxy/wayf.html.twig | 57 +++++- .../View/Proxy/wayf_content.html.twig | 19 -- .../Authentication/View/Proxy/wayf.html.twig | 3 - theme/skeune/translations/messages.en.php | 1 + theme/skeune/translations/messages.nl.php | 1 + theme/skeune/translations/messages.pt.php | 1 + 39 files changed, 853 insertions(+), 102 deletions(-) create mode 100644 docs/testing.md create mode 100644 src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php create mode 100644 src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php create mode 100644 src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php create mode 100644 tests/e2e/cypress/integration/skeune/wayf/wayf.preferred.spec.js create mode 100644 tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php create mode 100644 tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php create mode 100644 theme/base/stylesheets/pages/wayf/preferredIdps.scss create mode 100644 theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/preferredIdps.html.twig delete mode 100644 theme/base/templates/modules/Authentication/View/Proxy/wayf_content.html.twig diff --git a/CHANGELOG.md b/CHANGELOG.md index ff38a49149..f64afb8b51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,10 @@ Changes: * The `0000-00-00 00:00:00` is added for clarity/consistency, as this is probably the default behaviour of your database already. * Removed unused index `consent.deleted_at`. Delete this from your production database if it's there. * Added a specific error page for unsolicited SAML responses (IdP-initiated SSO without a prior AuthnRequest). +* A new parameter `wayf.preferred_idp_entity_ids` must be added to `parameters.yml`. To display a set of IdPs prominent at the top of the WAYF, add the entityId's of those IdPs to this parameter. + * To keep the old behaviour, set the value to `[]` +**Default behaviour (no change):** when the parameter is absent or empty, the WAYF behaves exactly as before. * Stabilized consent checks * In order to make the consent hashes more robust, a more consistent way of hashing the user attributes has been introduced * This feature automatically migrates from the old hashes to the new hashes upon login. diff --git a/config/packages/parameters.yml.dist b/config/packages/parameters.yml.dist index 0eef642d67..0a8c02350b 100644 --- a/config/packages/parameters.yml.dist +++ b/config/packages/parameters.yml.dist @@ -187,6 +187,11 @@ parameters: wayf.display_default_idp_banner_on_wayf: true wayf.default_idp_entity_id: https://default-idp.dev.openconext.local + ## Ordered list of IdP entity IDs to feature prominently at the top of the WAYF. + ## These IdPs appear above the search field and are excluded from the regular searchable list. + ## An empty list means no behaviour change. + wayf.preferred_idp_entity_ids: [] + ## Toggle display & content of global site notice global.site_notice.show: false global.site_notice.allowed.tags: '

    1. ' diff --git a/config/services/services.yml b/config/services/services.yml index 2d695a5829..30a80c1943 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -2,9 +2,17 @@ services: _defaults: public: true + OpenConext\EngineBlock\Service\Wayf\IdpSplitter: + + OpenConext\EngineBlockBundle\Service\WayfViewModelFactory: + arguments: + $wayfExtension: '@OpenConext\EngineBlockBundle\Twig\Extensions\Extension\Wayf' + OpenConext\EngineBlockBundle\Bridge\EngineBlockBootstrapper: autowire: true autoconfigure: true + arguments: + $preferredIdpEntityIds: '%wayf.preferred_idp_entity_ids%' tags: - { name: kernel.event_subscriber } diff --git a/docs/testing.md b/docs/testing.md new file mode 100644 index 0000000000..4059a7eabf --- /dev/null +++ b/docs/testing.md @@ -0,0 +1,52 @@ +# Testing + +## WAYF functional-testing page + +The functional-testing route renders the WAYF page with synthetic IdP data, controllable via query parameters. Use it for manual verification and as the base URL for Cypress tests. + +**Base URL:** `https://engine.dev.openconext.local/functional-testing/wayf` + +### Query parameters + +| Parameter | Type | Default | Description | +|---|---|---|---| +| `lang` | string | `en` | Locale (`en` or `nl`) | +| `connectedIdps` | int | `5` | Number of connected IdPs to generate | +| `unconnectedIdps` | int | `0` | Number of unconnected IdPs to generate | +| `randomIdps` | int | `0` | Generate N IdPs with random (Faker) names instead; overrides connected/unconnected | +| `addDiscoveries` | bool | `true` | Add discovery entries to IdP 1 (gives it 3 list entries instead of 1) | +| `preferredIdpEntityIds[]` | string[] | `[]` | Entity IDs to feature in the preferred section (array syntax required) | +| `defaultIdpEntityId` | string | - | Entity ID of the default IdP (shows banner) | +| `showIdPBanner` | bool | `true` | Whether to show the default IdP banner | +| `displayUnconnectedIdpsWayf` | bool | `false` | Show unconnected IdPs with a "Request access" button | +| `backLink` | bool | `false` | Show "Return to service provider" back link | +| `rememberChoiceFeature` | bool | `false` | Show "Remember my choice" checkbox | +| `cutoffPointForShowingUnfilteredIdps` | int | `100` | Hide the IdP list until the user searches when list length exceeds this value | + +#### Baseline +- [Default (5 connected IdPs)](https://engine.dev.openconext.local/functional-testing/wayf) +- [Dutch locale](https://engine.dev.openconext.local/functional-testing/wayf?lang=nl) +- [10 IdPs](https://engine.dev.openconext.local/functional-testing/wayf?connectedIdps=10&addDiscoveries=false) +- [Random IdPs (Faker names)](https://engine.dev.openconext.local/functional-testing/wayf?randomIdps=8) + +#### Cutoff / search +- [Cutoff active - list hidden until search](https://engine.dev.openconext.local/functional-testing/wayf?connectedIdps=6&cutoffPointForShowingUnfilteredIdps=5) + +#### Unconnected IdPs / request access +- [Unconnected IdPs visible, no request access](https://engine.dev.openconext.local/functional-testing/wayf?unconnectedIdps=3) +- [Unconnected IdPs + request access button](https://engine.dev.openconext.local/functional-testing/wayf?unconnectedIdps=3&displayUnconnectedIdpsWayf=true) + +#### UI features +- [Back link](https://engine.dev.openconext.local/functional-testing/wayf?backLink=true) +- [Remember my choice](https://engine.dev.openconext.local/functional-testing/wayf?rememberChoiceFeature=true) +- [Default IdP banner](https://engine.dev.openconext.local/functional-testing/wayf?defaultIdpEntityId=https%3A%2F%2Fexample.com%2FentityId%2F3&showIdPBanner=true&addDiscoveries=false) + +#### Preferred IdPs +- [1 preferred IdP](https://engine.dev.openconext.local/functional-testing/wayf?preferredIdpEntityIds%5B%5D=https%3A%2F%2Fexample.com%2FentityId%2F1&addDiscoveries=false) +- [2 preferred IdPs](https://engine.dev.openconext.local/functional-testing/wayf?preferredIdpEntityIds%5B%5D=https%3A%2F%2Fexample.com%2FentityId%2F1&preferredIdpEntityIds%5B%5D=https%3A%2F%2Fexample.com%2FentityId%2F2&addDiscoveries=false) +- [Preferred = default IdP > banner suppressed](https://engine.dev.openconext.local/functional-testing/wayf?preferredIdpEntityIds%5B%5D=https%3A%2F%2Fexample.com%2FentityId%2F1&defaultIdpEntityId=https%3A%2F%2Fexample.com%2FentityId%2F1&showIdPBanner=true&addDiscoveries=false) +- [Preferred ≠ default IdP > both visible](https://engine.dev.openconext.local/functional-testing/wayf?preferredIdpEntityIds%5B%5D=https%3A%2F%2Fexample.com%2FentityId%2F1&defaultIdpEntityId=https%3A%2F%2Fexample.com%2FentityId%2F2&showIdPBanner=true&addDiscoveries=false) +- [Preferred IdP with discoveries (1 IdP > 3 entries)](https://engine.dev.openconext.local/functional-testing/wayf?preferredIdpEntityIds%5B%5D=https%3A%2F%2Fexample.com%2FentityId%2F1) + + +- [All features enabled](https://engine.dev.openconext.local/functional-testing/wayf?connectedIdps=8&unconnectedIdps=2&displayUnconnectedIdpsWayf=true&preferredIdpEntityIds%5B%5D=https%3A%2F%2Fexample.com%2FentityId%2F1&defaultIdpEntityId=https%3A%2F%2Fexample.com%2FentityId%2F2&showIdPBanner=true&backLink=true&rememberChoiceFeature=true&addDiscoveries=false) diff --git a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php index 54e2e6233a..eb0e728a96 100644 --- a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php +++ b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php @@ -462,8 +462,6 @@ protected function _showWayf(EngineBlock_Saml2_AuthnRequestAnnotationDecorator $ $currentLocale = $container->getLocaleProvider()->getLocale(); - $cookies = $container->getSymfonyRequest()->cookies->all(); - if ($request->isDebugRequest()) { $serviceProvider = $this->getEngineSpRole($this->_server); } else { @@ -478,27 +476,39 @@ protected function _showWayf(EngineBlock_Saml2_AuthnRequestAnnotationDecorator $ ); $defaultIdPInIdPList = $this->isDefaultIdPPresent($idpList); - $showDefaultIdpBanner = (bool) $container->shouldDisplayDefaultIdpBannerOnWayf() && $defaultIdPInIdPList; + + $diContainerRuntime = $application->getDiContainerRuntime(); + $preferredIdpEntityIds = $diContainerRuntime->getPreferredIdpEntityIds(); + $split = $diContainerRuntime->idpSplitter->split($idpList, $preferredIdpEntityIds); + $showPreferredIdps = !empty($split['preferred']); + $isDefaultIdpPreferred = in_array($container->getDefaultIdPEntityId(), $preferredIdpEntityIds, true); + $showDefaultIdpBanner = (bool) $container->shouldDisplayDefaultIdpBannerOnWayf() + && $defaultIdPInIdPList + && (!$showPreferredIdps || !$isDefaultIdpPreferred); $rememberChoiceFeature = $container->getRememberChoice(); + $viewModel = $diContainerRuntime->wayfViewModelFactory->create( + idpList: $idpList, + regularIdpList: $split['regular'], + preferredIdpList: $split['preferred'], + showPreferredIdps: $showPreferredIdps, + action: $action, + greenHeader: $serviceProvider->getDisplayName($currentLocale), + helpLink: '/authentication/idp/help-discover?lang=' . $currentLocale, + backLink: $container->isUiOptionReturnToSpActive(), + cutoffPointForShowingUnfilteredIdps: $container->getCutoffPointForShowingUnfilteredIdps(), + showIdPBanner: $showDefaultIdpBanner, + rememberChoiceFeature: $rememberChoiceFeature, + showRequestAccess: $serviceProvider->getCoins()->displayUnconnectedIdpsWayf(), + requestId: $request->getId(), + serviceProvider: $serviceProvider, + showRequestAccessContainer: true, + ); + $output = $this->twig->render( '@theme/Authentication/View/Proxy/wayf.html.twig', - [ - 'action' => $action, - 'greenHeader' => $serviceProvider->getDisplayName($currentLocale), - 'helpLink' => '/authentication/idp/help-discover?lang=' . $currentLocale, - 'backLink' => $container->isUiOptionReturnToSpActive(), - 'cutoffPointForShowingUnfilteredIdps' => $container->getCutoffPointForShowingUnfilteredIdps(), - 'showIdPBanner' => $showDefaultIdpBanner, - 'rememberChoiceFeature' => $rememberChoiceFeature, - 'showRequestAccess' => $serviceProvider->getCoins()->displayUnconnectedIdpsWayf(), - 'requestId' => $request->getId(), - 'serviceProvider' => $serviceProvider, - 'idpList' => $idpList, - 'cookies' => $cookies, - 'showRequestAccessContainer' => true, - ] + $viewModel->toArray() ); $this->_server->sendOutput($output); } diff --git a/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php b/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php new file mode 100644 index 0000000000..e9e16a7088 --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php @@ -0,0 +1,58 @@ + [], 'regular' => $idpList]; + } + + $orderMap = array_flip($preferredEntityIds); + $preferredBuckets = array_fill(0, count($preferredEntityIds), []); + $regular = []; + + foreach ($idpList as $idp) { + $entityId = $idp['EntityID']; + if (isset($orderMap[$entityId])) { + if ($idp['Access'] === '1') { + $preferredBuckets[$orderMap[$entityId]][] = $idp; + } + // Unconnected preferred IdPs are excluded from both sections. + } else { + $regular[] = $idp; + } + } + + $mergeArgs = array_values(array_filter($preferredBuckets)); + $preferred = empty($mergeArgs) ? [] : array_merge(...$mergeArgs); + + return ['preferred' => $preferred, 'regular' => $regular]; + } +} diff --git a/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php b/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php index 77498409d4..7c03242bc8 100644 --- a/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php +++ b/src/OpenConext/EngineBlockBundle/Bridge/DiContainerRuntime.php @@ -18,6 +18,8 @@ namespace OpenConext\EngineBlockBundle\Bridge; +use OpenConext\EngineBlock\Service\Wayf\IdpSplitter; +use OpenConext\EngineBlockBundle\Service\WayfViewModelFactory; use Twig\Environment; /** @@ -29,7 +31,16 @@ final readonly class DiContainerRuntime { - public function __construct(public Environment $twig) + public function __construct( + public Environment $twig, + public IdpSplitter $idpSplitter, + public WayfViewModelFactory $wayfViewModelFactory, + private array $preferredIdpEntityIds = [], + ) { + } + + public function getPreferredIdpEntityIds(): array { + return $this->preferredIdpEntityIds; } } diff --git a/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php b/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php index bbba86ab23..36919835d6 100644 --- a/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php +++ b/src/OpenConext/EngineBlockBundle/Bridge/EngineBlockBootstrapper.php @@ -19,6 +19,8 @@ namespace OpenConext\EngineBlockBundle\Bridge; use EngineBlock_ApplicationSingleton; +use OpenConext\EngineBlock\Service\Wayf\IdpSplitter; +use OpenConext\EngineBlockBundle\Service\WayfViewModelFactory; use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpKernel\KernelEvents; use Twig\Environment; @@ -29,8 +31,11 @@ class EngineBlockBootstrapper implements EventSubscriberInterface public function __construct( Environment $twig, + IdpSplitter $idpSplitter, + WayfViewModelFactory $wayfViewModelFactory, + array $preferredIdpEntityIds = [], ) { - $this->diContainerRuntime = new DiContainerRuntime($twig); + $this->diContainerRuntime = new DiContainerRuntime($twig, $idpSplitter, $wayfViewModelFactory, $preferredIdpEntityIds); } public function onKernelRequest(): void diff --git a/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php b/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php new file mode 100644 index 0000000000..bf5a8bc2e8 --- /dev/null +++ b/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php @@ -0,0 +1,75 @@ +wayfExtension->getConnectedIdps($idpList), + regularConnectedIdps: $this->wayfExtension->getConnectedIdps($regularIdpList), + preferredConnectedIdps: $this->wayfExtension->getConnectedIdps($preferredIdpList), + showPreferredIdps: $showPreferredIdps, + idpList: $idpList, + regularIdpList: $regularIdpList, + preferredIdpList: $preferredIdpList, + ); + } +} diff --git a/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Locale.php b/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Locale.php index 1e10ebc6e1..126f5bfdaf 100644 --- a/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Locale.php +++ b/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Locale.php @@ -75,13 +75,9 @@ public function getQueryStringFor($locale) $params ); - $query = ''; - foreach ($params as $key => $value) { - $query .= (strlen($query) == 0) ? '?' : '&' ; - $query .= $key. '=' .urlencode($value); - } + $query = http_build_query($params); - return $query; + return strlen($query) > 0 ? '?' . $query : ''; } #[AsTwigFunction(name: 'locale')] diff --git a/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php b/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php new file mode 100644 index 0000000000..8f9b151043 --- /dev/null +++ b/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php @@ -0,0 +1,59 @@ +twig = $twig; + public function __construct( + private readonly Environment $twig, + private readonly IdpSplitter $idpSplitter, + private readonly WayfViewModelFactory $wayfViewModelFactory, + ) { } public function wayfAction(Request $request) @@ -49,6 +51,7 @@ public function wayfAction(Request $request) $cutoffPointForShowingUnfilteredIdps = $request->query->getInt('cutoffPointForShowingUnfilteredIdps', 100); $showIdPBanner = $request->query->getBoolean('showIdPBanner', true); $defaultIdpEntityId = $request->query->get('defaultIdpEntityId'); + $preferredIdpEntityIds = $request->query->all('preferredIdpEntityIds'); $connectedIdps = $request->query->getInt('connectedIdps', 5); $unconnectedIdps = $request->query->getInt('unconnectedIdps'); @@ -58,22 +61,35 @@ public function wayfAction(Request $request) ? TestEntitySeeder::buildIdps($connectedIdps, $unconnectedIdps, $currentLocale, $defaultIdpEntityId, $addDiscoveries) : TestEntitySeeder::buildRandomIdps($randomIdps, $currentLocale, $defaultIdpEntityId); + $split = $this->idpSplitter->split($idpList, $preferredIdpEntityIds); + $preferredIdpList = $split['preferred']; + $regularIdpList = $split['regular']; + $showPreferredIdps = !empty($preferredIdpList); + + $isDefaultIdpPreferred = in_array($defaultIdpEntityId, $preferredIdpEntityIds, true); + $showIdPBanner = $showIdPBanner && (!$showPreferredIdps || !$isDefaultIdpPreferred); + + $viewModel = $this->wayfViewModelFactory->create( + idpList: $idpList, + regularIdpList: $regularIdpList, + preferredIdpList: $preferredIdpList, + showPreferredIdps: $showPreferredIdps, + action: $this->generateUrl('functional_testing_handle_wayf'), + greenHeader: $currentLocale, + helpLink: '/authentication/idp/help-discover?lang=' . $currentLocale, + backLink: $backLink, + cutoffPointForShowingUnfilteredIdps: $cutoffPointForShowingUnfilteredIdps, + showIdPBanner: $showIdPBanner, + rememberChoiceFeature: $rememberChoiceFeature, + showRequestAccess: $displayUnconnectedIdpsWayf, + requestId: 'bogus-request-id', + serviceProvider: TestEntitySeeder::buildSp(), + showRequestAccessContainer: true, + ); + return new Response($this->twig->render( '@theme/Authentication/View/Proxy/wayf.html.twig', - [ - 'action' => $this->generateUrl('functional_testing_handle_wayf'), - 'greenHeader' => $currentLocale, - 'helpLink' => '/authentication/idp/help-discover?lang='.$currentLocale, - 'backLink' => $backLink, - 'cutoffPointForShowingUnfilteredIdps' => $cutoffPointForShowingUnfilteredIdps, - 'showIdPBanner' => $showIdPBanner, - 'rememberChoiceFeature' => $rememberChoiceFeature, - 'showRequestAccess' => $displayUnconnectedIdpsWayf, - 'requestId' => 'bogus-request-id', - 'serviceProvider' => TestEntitySeeder::buildSp(), - 'idpList' => $idpList, - 'showRequestAccessContainer' => true, - ] + $viewModel->toArray() )); } diff --git a/tests/e2e/cypress/integration/skeune/wayf/wayf.general.spec.js b/tests/e2e/cypress/integration/skeune/wayf/wayf.general.spec.js index 5165d75973..76632774a2 100644 --- a/tests/e2e/cypress/integration/skeune/wayf/wayf.general.spec.js +++ b/tests/e2e/cypress/integration/skeune/wayf/wayf.general.spec.js @@ -202,6 +202,13 @@ context('WAYF behaviour not tied to mouse / keyboard navigation', () => { }); }); + describe('Preferred IdPs section heading', () => { + it('Should show the preferred IdPs section with the correct heading when preferred IdPs are configured', () => { + cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?preferredIdpEntityIds%5B%5D=https%3A%2F%2Fexample.com%2FentityId%2F1'); + cy.get('.wayf__preferredIdps').should('be.visible'); + }); + }); + describe('Should show the return to service link when configured', () => { it('Load the page & check if the page is there', () => { cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?connectedIdps=5&backLink=true'); diff --git a/tests/e2e/cypress/integration/skeune/wayf/wayf.preferred.spec.js b/tests/e2e/cypress/integration/skeune/wayf/wayf.preferred.spec.js new file mode 100644 index 0000000000..9fa4620b9e --- /dev/null +++ b/tests/e2e/cypress/integration/skeune/wayf/wayf.preferred.spec.js @@ -0,0 +1,98 @@ +import { + addAccountButtonSelector, + defaultIdpInformational, + preferredIdpsSectionSelector, + remainingIdpsSearchLabelSelector, +} from '../../../../../../theme/base/javascripts/selectors'; + +const WAYF = 'https://engine.dev.openconext.local/functional-testing/wayf'; +const ENTITY_1 = 'https://example.com/entityId/1'; +const ENTITY_2 = 'https://example.com/entityId/2'; + +const withPreferred = `${WAYF}?preferredIdpEntityIds%5B%5D=${encodeURIComponent(ENTITY_1)}`; +const preferredEqualsDefault = `${WAYF}?defaultIdpEntityId=${encodeURIComponent(ENTITY_1)}&preferredIdpEntityIds%5B%5D=${encodeURIComponent(ENTITY_1)}&showIdPBanner=1`; +const preferredDiffersFromDefault = `${WAYF}?defaultIdpEntityId=${encodeURIComponent(ENTITY_2)}&preferredIdpEntityIds%5B%5D=${encodeURIComponent(ENTITY_1)}&showIdPBanner=1`; + +describe('Search label above regular IdP list', () => { + it('shows search label when preferred IdPs are configured', () => { + cy.visit(withPreferred); + cy.get(remainingIdpsSearchLabelSelector).should('be.visible'); + }); + + it('does not show search label when no preferred IdPs are configured', () => { + cy.visit(WAYF); + cy.get(remainingIdpsSearchLabelSelector).should('not.exist'); + }); +}); + +describe('Preferred IdPs section visibility', () => { + it('shows the preferred section when preferred IdPs are configured', () => { + cy.visit(withPreferred); + cy.get(preferredIdpsSectionSelector).should('be.visible'); + }); + + it('does not show preferred section when no preferred IdPs are configured', () => { + cy.visit(WAYF); + cy.get(preferredIdpsSectionSelector).should('not.exist'); + }); + + it('shows preferred IdP in preferred section and not in remaining list', () => { + cy.visit(withPreferred); + + // entityId/1 appears inside the preferred section + cy.get(`${preferredIdpsSectionSelector} [data-entityid="${ENTITY_1}"]`).should('exist'); + + // entityId/1 must NOT appear in the remaining (non-preferred) IdP list + cy.get(`.wayf__remainingIdps .wayf__idpList:not(.wayf__idpList--preferred) [data-entityid="${ENTITY_1}"]`) + .should('not.exist'); + }); + + it('shows exactly one item in the preferred section when one preferred IdP is configured', () => { + // addDiscoveries=false prevents discovery entries from inflating the preferred count + cy.visit(`${withPreferred}&addDiscoveries=false`); + cy.get(`${preferredIdpsSectionSelector} li`).should('have.length', 1); + }); + + it('shows exactly two items in the preferred section when two preferred IdPs are configured', () => { + const withTwoPreferred = `${WAYF}?preferredIdpEntityIds%5B%5D=${encodeURIComponent(ENTITY_1)}&preferredIdpEntityIds%5B%5D=${encodeURIComponent(ENTITY_2)}&addDiscoveries=false`; + cy.visit(withTwoPreferred); + cy.get(`${preferredIdpsSectionSelector} li`).should('have.length', 2); + }); +}); + +describe('Preferred IdPs with previous selection', () => { + beforeEach(() => { + cy.addOnePreviouslySelectedIdp(false, withPreferred); + }); + + it('hides preferred section when a previous selection is present', () => { + cy.visit(withPreferred); + cy.get(preferredIdpsSectionSelector).should('not.be.visible'); + }); + + it('reveals preferred section after clicking "Use another account"', () => { + cy.visit(withPreferred); + cy.get(addAccountButtonSelector).click({ force: true }); + cy.get(preferredIdpsSectionSelector).should('be.visible'); + }); +}); + +describe('Three display scenarios', () => { + it('scenario 1: no preferred IdPs: banner visible, preferred section absent', () => { + cy.visit(`${WAYF}?showIdPBanner=1`); + cy.get(defaultIdpInformational).should('be.visible'); + cy.get(preferredIdpsSectionSelector).should('not.exist'); + }); + + it('scenario 3: default IdP is preferred: preferred section visible, banner suppressed', () => { + cy.visit(preferredEqualsDefault); + cy.get(preferredIdpsSectionSelector).should('be.visible'); + cy.get(defaultIdpInformational).should('not.exist'); + }); + + it('scenario 4: preferred IdP differs from default: both preferred section and banner visible', () => { + cy.visit(preferredDiffersFromDefault); + cy.get(preferredIdpsSectionSelector).should('be.visible'); + cy.get(defaultIdpInformational).should('be.visible'); + }); +}); diff --git a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php index 13cc57dc64..d8b421cf88 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php @@ -20,7 +20,9 @@ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; +use OpenConext\EngineBlock\Service\Wayf\IdpSplitter; use OpenConext\EngineBlockBundle\Bridge\DiContainerRuntime; +use OpenConext\EngineBlockBundle\Service\WayfViewModelFactory; use PHPUnit\Framework\TestCase; use SAML2\Assertion; use SAML2\Assertion\Validation\ConstraintValidator\NotBefore; @@ -59,7 +61,7 @@ public function setUp(): void Phake::when($this->proxyServer)->getConfig('WantsAuthnRequestsSigned')->thenReturn(false); $engineBlock = \EngineBlock_ApplicationSingleton::getInstance(); - $engineBlock->setDiContainerRuntime(new DiContainerRuntime(Phake::mock(Twig\Environment::class))); + $engineBlock->setDiContainerRuntime(new DiContainerRuntime(Phake::mock(Twig\Environment::class), new IdpSplitter(), Phake::mock(WayfViewModelFactory::class))); $this->bindings = new EngineBlock_Corto_Module_Bindings($this->proxyServer); } diff --git a/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php b/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php new file mode 100644 index 0000000000..ff8ec2030b --- /dev/null +++ b/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php @@ -0,0 +1,170 @@ +splitter = new IdpSplitter(); + } + + private function split(array $idpList, array $preferredEntityIds): array + { + return $this->splitter->split($idpList, $preferredEntityIds); + } + + private function idp(string $entityId, string $access = '1', string $discoveryHash = ''): array + { + return [ + 'EntityID' => $entityId, + 'Access' => $access, + 'Name' => $entityId, + 'Keywords' => [], + 'Logo' => '', + 'isDefaultIdp' => false, + 'DiscoveryHash' => $discoveryHash, + ]; + } + + public function testEmptyPreferredEntityIdsReturnsFullListAsRegular(): void + { + $idpList = [$this->idp('https://idp1.example.org'), $this->idp('https://idp2.example.org')]; + + $result = $this->split($idpList, []); + + $this->assertSame([], $result['preferred']); + $this->assertSame($idpList, $result['regular']); + } + + public function testPreferredIdpIsMovedOutOfRegularList(): void + { + $idp1 = $this->idp('https://idp1.example.org'); + $idp2 = $this->idp('https://idp2.example.org'); + + $result = $this->split([$idp1, $idp2], ['https://idp1.example.org']); + + $this->assertCount(1, $result['preferred']); + $this->assertSame('https://idp1.example.org', $result['preferred'][0]['EntityID']); + $this->assertCount(1, $result['regular']); + $this->assertSame('https://idp2.example.org', $result['regular'][0]['EntityID']); + } + + public function testConfiguredOrderIsPreservedInPreferredList(): void + { + $idp1 = $this->idp('https://idp1.example.org'); + $idp2 = $this->idp('https://idp2.example.org'); + $idp3 = $this->idp('https://idp3.example.org'); + + $result = $this->split( + [$idp1, $idp2, $idp3], + ['https://idp3.example.org', 'https://idp1.example.org'] + ); + + $this->assertCount(2, $result['preferred']); + $this->assertSame('https://idp3.example.org', $result['preferred'][0]['EntityID']); + $this->assertSame('https://idp1.example.org', $result['preferred'][1]['EntityID']); + $this->assertCount(1, $result['regular']); + $this->assertSame('https://idp2.example.org', $result['regular'][0]['EntityID']); + } + + public function testDisconnectedPreferredIdpIsExcludedFromBothLists(): void + { + $idpConnected = $this->idp('https://idp1.example.org', '1'); + $idpDisconnected = $this->idp('https://idp2.example.org', '0'); + + $result = $this->split( + [$idpConnected, $idpDisconnected], + ['https://idp2.example.org'] + ); + + $this->assertSame([], $result['preferred']); + // idp1 is not preferred so it stays in regular; idp2 is preferred but disconnected > excluded from both + $this->assertCount(1, $result['regular']); + $this->assertSame('https://idp1.example.org', $result['regular'][0]['EntityID']); + } + + public function testMultipleDiscoveryEntriesForSameEntityIdAreGroupedInOrder(): void + { + $idpMain = $this->idp('https://idp1.example.org', '1', ''); + $idpDiscovery = $this->idp('https://idp1.example.org', '1', 'discovery-hash'); + $idpOther = $this->idp('https://idp2.example.org', '1', ''); + + $result = $this->split( + [$idpMain, $idpDiscovery, $idpOther], + ['https://idp1.example.org'] + ); + + $this->assertCount(2, $result['preferred']); + $this->assertSame('https://idp1.example.org', $result['preferred'][0]['EntityID']); + $this->assertSame('https://idp1.example.org', $result['preferred'][1]['EntityID']); + $this->assertCount(1, $result['regular']); + $this->assertSame('https://idp2.example.org', $result['regular'][0]['EntityID']); + } + + public function testPreferredEntityIdNotInIdpListIsIgnored(): void + { + $idp1 = $this->idp('https://idp1.example.org'); + + $result = $this->split([$idp1], ['https://nonexistent.example.org']); + + $this->assertSame([], $result['preferred']); + $this->assertCount(1, $result['regular']); + } + + public static function fiveScenarioProvider(): array + { + return [ + 'scenario 1: no preferred, default connected' => [[], true, 'https://default.example.org', false, true], + 'scenario 2: no preferred, default not connected' => [[], false, 'https://default.example.org', false, false], + 'scenario 3: preferred includes default' => [['https://default.example.org'], true, 'https://default.example.org', true, false], + 'scenario 4: preferred does not include default' => [['https://idp1.example.org'], true, 'https://default.example.org', true, true], + 'scenario 5: preferred, default not connected' => [['https://idp1.example.org'], false, 'https://default.example.org', true, false], + ]; + } + + #[DataProvider('fiveScenarioProvider')] + public function testFiveScenarioBannerAndPreferredVisibility( + array $preferredEntityIds, + bool $defaultIdpConnected, + string $defaultIdpEntityId, + bool $expectShowPreferred, + bool $expectShowBanner + ): void { + $idpList = [ + $this->idp('https://idp1.example.org', '1'), + $this->idp('https://default.example.org', $defaultIdpConnected ? '1' : '0'), + ]; + + $split = $this->split($idpList, $preferredEntityIds); + + $showPreferred = !empty($split['preferred']); + $isDefaultPreferred = in_array($defaultIdpEntityId, $preferredEntityIds, true); + $showBanner = $defaultIdpConnected && (!$showPreferred || !$isDefaultPreferred); + + $this->assertSame($expectShowPreferred, $showPreferred, 'showPreferredIdps mismatch'); + $this->assertSame($expectShowBanner, $showBanner, 'showIdPBanner mismatch'); + } +} diff --git a/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php b/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php new file mode 100644 index 0000000000..e8877d801f --- /dev/null +++ b/tests/unit/OpenConext/EngineBlockBundle/DiContainerRuntimeTest.php @@ -0,0 +1,53 @@ +createStub(Environment::class), + new IdpSplitter(), + $this->createStub(WayfViewModelFactory::class), + ); + + $this->assertSame([], $runtime->getPreferredIdpEntityIds()); + } + + public function testGetPreferredIdpEntityIdsReturnsConfiguredList(): void + { + $entityIds = ['https://idp1.example.org', 'https://idp2.example.org']; + + $runtime = new DiContainerRuntime( + $this->createStub(Environment::class), + new IdpSplitter(), + $this->createStub(WayfViewModelFactory::class), + $entityIds, + ); + + $this->assertSame($entityIds, $runtime->getPreferredIdpEntityIds()); + } +} diff --git a/tests/unit/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/WayfTest.php b/tests/unit/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/WayfTest.php index a346324a71..b996f139e4 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/WayfTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/WayfTest.php @@ -203,7 +203,8 @@ public function testGetWayfJsonConfig() $this->translator->method('trans') ->willReturnMap([ ['more_idp_results', [], null, null, 'More results'], - ['request_access', [], null, null, 'Request Access'] + ['request_access', [], null, null, 'Request Access'], + ['wayf_remaining_idps_search_label', [], null, null, 'Or search for a Dutch institution from the list'], ]); // Test with showRequestAccess = true diff --git a/theme/base/javascripts/selectors.js b/theme/base/javascripts/selectors.js index d597029c68..d5bfdca7c0 100644 --- a/theme/base/javascripts/selectors.js +++ b/theme/base/javascripts/selectors.js @@ -77,6 +77,8 @@ export const idpDeleteDisabledSelector = `.${idpDeleteDisabledClass}`; export const remainingIdpSectionSelector = '.wayf__remainingIdps'; export const remainingIdpListSelector = '.wayf__remainingIdps .wayf__idpList'; export const remainingIdpSelector = '.wayf__remainingIdps .wayf__idpList .wayf__idp'; +export const preferredIdpsSectionSelector = '.wayf__preferredIdps'; +export const remainingIdpsSearchLabelSelector = '.remainingIdps__searchLabel'; export const searchFieldClass = 'search__field'; export const searchFieldSelector = `.${searchFieldClass}`; export const searchResetClass = 'search__reset'; diff --git a/theme/base/javascripts/wayf/utility/switchIdpSection.js b/theme/base/javascripts/wayf/utility/switchIdpSection.js index 7d90a55e60..90ee9d6cbe 100644 --- a/theme/base/javascripts/wayf/utility/switchIdpSection.js +++ b/theme/base/javascripts/wayf/utility/switchIdpSection.js @@ -1,6 +1,6 @@ import {toggleVisibility} from '../../utility/toggleVisibility'; import {focusOn} from "../../utility/focusOn"; -import {noAccessSectionSelector, noResultSectionSelector, remainingIdpSectionSelector, searchFieldSelector, selectedIdpsSectionSelector} from '../../selectors'; +import {noAccessSectionSelector, noResultSectionSelector, preferredIdpsSectionSelector, remainingIdpSectionSelector, searchFieldSelector, selectedIdpsSectionSelector} from '../../selectors'; import {isVisibleElement} from '../../utility/isVisibleElement'; export const switchIdpSection = () => { @@ -8,10 +8,14 @@ export const switchIdpSection = () => { const previousIdps = document.querySelector(selectedIdpsSectionSelector); const noResults = document.querySelector(noResultSectionSelector); const noAccess = document.querySelector(noAccessSectionSelector); + const preferredIdps = document.querySelector(preferredIdpsSectionSelector); const ariaHidden = 'aria-hidden'; toggleVisibility(previousIdps); toggleVisibility(remainingIdps); + if (preferredIdps) { + toggleVisibility(preferredIdps); + } remainingIdps.removeAttribute(ariaHidden); diff --git a/theme/base/stylesheets/pages/wayf.scss b/theme/base/stylesheets/pages/wayf.scss index 8668f88a15..2c6cca75a1 100644 --- a/theme/base/stylesheets/pages/wayf.scss +++ b/theme/base/stylesheets/pages/wayf.scss @@ -10,3 +10,4 @@ @use 'wayf/noResults'; @use 'wayf/previousSelection'; @use 'wayf/remainingIdps'; +@use 'wayf/preferredIdps'; diff --git a/theme/base/stylesheets/pages/wayf/preferredIdps.scss b/theme/base/stylesheets/pages/wayf/preferredIdps.scss new file mode 100644 index 0000000000..54e46f029d --- /dev/null +++ b/theme/base/stylesheets/pages/wayf/preferredIdps.scss @@ -0,0 +1,14 @@ +@use "../../variables"; + +.wayf__preferredIdps { + margin-bottom: 0; + margin-top: 2rem; +} + +.remainingIdps__searchLabel { + color: variables.$brownishGray; + font-family: Nunito, sans-serif; + font-size: variables.$f-xl; + font-weight: variables.$bolder; + margin: 2rem 0 0; +} diff --git a/theme/base/stylesheets/pages/wayf/remainingIdps.scss b/theme/base/stylesheets/pages/wayf/remainingIdps.scss index 5cb65f41d7..38196c01c9 100644 --- a/theme/base/stylesheets/pages/wayf/remainingIdps.scss +++ b/theme/base/stylesheets/pages/wayf/remainingIdps.scss @@ -68,6 +68,10 @@ } } + > .remainingIdps__searchLabel + .wayf__search { + margin-top: 0.5rem; + } + > .remainingIdps__defaultIdp { @include mixins-skeune.informational; margin: 1.25rem 0 1.25rem; diff --git a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/backLink.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/backLink.html.twig index 65b88aecd6..5af6e094d8 100644 --- a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/backLink.html.twig +++ b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/backLink.html.twig @@ -1,9 +1,9 @@ -{% if backLink is defined and backLink == true %} +{% if backLink %}
      {% include '@theme/Default/Partials/buttonLink.html.twig' with { buttonUrl: 'javascript:window.history.back(-2)', className: 'wayf__backLink', text: 'return_to_sp'|trans - } %} + } only %}
      {% endif %} diff --git a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idp.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idp.html.twig index 76ea916c62..a59b655bc4 100644 --- a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idp.html.twig +++ b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idp.html.twig @@ -30,9 +30,16 @@ id="idp__title{{ listName }}{{ loop.index }}" class="idp__title" >{{ idp['displayTitle'] }}
      - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpForm.html.twig' %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpForm.html.twig' with { + idp: idp, + action: action, + requestId: requestId, + } only %} {% if idp['connected'] is defined and not idp['connected'] or delete %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpDeleteDisable.html.twig' %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpDeleteDisable.html.twig' with { + idp: idp, + delete: delete, + } only %} {% endif %}
      diff --git a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpForm.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpForm.html.twig index 61a6c140b3..2eb57a719f 100644 --- a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpForm.html.twig +++ b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpForm.html.twig @@ -2,8 +2,8 @@ - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpSubmitButton.html.twig' with { hidden: true } %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpSubmitButton.html.twig' with { hidden: true } only %} diff --git a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpItem.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpItem.html.twig index cc222b0ed3..2a53813333 100644 --- a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpItem.html.twig +++ b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpItem.html.twig @@ -22,5 +22,12 @@ data-connectable="{{ connectable }}" data-url="{{ logoUrl }}" > - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idp.html.twig' with { idp: idp } %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idp.html.twig' with { + idp: idp, + loop: loop, + listName: listName, + delete: delete, + action: action, + requestId: requestId, + } only %}
      2. diff --git a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpList.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpList.html.twig index b21ba191a0..d0608a2bd3 100644 --- a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpList.html.twig +++ b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/idp/idpList.html.twig @@ -10,15 +10,27 @@ {# First show all connected Idps #} {% for idp in idpList %} {% if idp['connected'] is defined and idp['connected'] %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpItem.html.twig' with { idp: idp } - %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpItem.html.twig' with { + idp: idp, + loop: loop, + delete: delete, + listName: listName, + action: action, + requestId: requestId, + } only %} {% endif %} {% endfor %} - {# Next show all unconnnected Idps #} + {# Next show all unconnected Idps #} {% for idp in idpList %} {% if showRequestAccess and idp['connected'] is defined and not idp['connected'] %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpItem.html.twig' with { idp: idp } - %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpItem.html.twig' with { + idp: idp, + loop: loop, + delete: delete, + listName: listName, + action: action, + requestId: requestId, + } only %} {% endif %} {% endfor %}
diff --git a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/noAccess/noAccess.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/noAccess/noAccess.html.twig index f0341bfc0b..183f06d79d 100644 --- a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/noAccess/noAccess.html.twig +++ b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/noAccess/noAccess.html.twig @@ -5,6 +5,8 @@ {# placeholder for the idp without access #} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noAccess/helpdesk.html.twig' %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noAccess/requestForm.html.twig' %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noAccess/helpdesk.html.twig' only %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noAccess/requestForm.html.twig' with { + serviceProvider: serviceProvider, + } only %} diff --git a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/noAccess/requestForm.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/noAccess/requestForm.html.twig index 238e28be63..8f5a873123 100644 --- a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/noAccess/requestForm.html.twig +++ b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/noAccess/requestForm.html.twig @@ -56,5 +56,5 @@ - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noAccess/ctas.html.twig' %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noAccess/ctas.html.twig' only %} diff --git a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/preferredIdps.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/preferredIdps.html.twig new file mode 100644 index 0000000000..c5b25cc0b3 --- /dev/null +++ b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/preferredIdps.html.twig @@ -0,0 +1,7 @@ +
+
    + {% for idp in preferredConnectedIdps.formattedIdpList %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpItem.html.twig' with { idp: idp, delete: false, listName: 'preferred', loop: loop, action: action, requestId: requestId } only %} + {% endfor %} +
+
diff --git a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/preselection.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/preselection.html.twig index f8489a3134..24d382df43 100644 --- a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/preselection.html.twig +++ b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/preselection.html.twig @@ -13,10 +13,20 @@ {% endapply %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpList.html.twig' with { idpList: previousSelectionList, delete: true, listName: 'preselection', id: 'previousSelection__title' } %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpList.html.twig' with { + idpList: previousSelectionList, + delete: true, + listName: 'preselection', + id: 'previousSelection__title', + showIdPBanner: false, + showRequestAccess: showRequestAccess, + cutoffPointForShowingUnfilteredIdps: cutoffPointForShowingUnfilteredIdps, + action: action, + requestId: requestId, + } only %} diff --git a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/remainingIdps.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/remainingIdps.html.twig index 8ac292aa30..5c59895c7e 100644 --- a/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/remainingIdps.html.twig +++ b/theme/base/templates/modules/Authentication/View/Proxy/Partials/WAYF/remainingIdps.html.twig @@ -6,10 +6,16 @@ {{ 'wayf_select_account_screenreader'|trans }} {% endif %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/search.html.twig' %} + {% if showPreferredIdps %} +

{{ 'wayf_remaining_idps_search_label'|trans }}

+ {% endif %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/search.html.twig' with { + action: action, + requestId: requestId, + } only %} - {% set idpListSorted = connectedIdps.formattedIdpList|sort((a, b) => a.displayTitle|lower <=> b.displayTitle|lower) %} - {% if showIdPBanner is defined and showIdPBanner %} + {% set idpListSorted = regularConnectedIdps.formattedIdpList|sort((a, b) => a.displayTitle|lower <=> b.displayTitle|lower) %} + {% if showIdPBanner %} {% set requestUri %} {% if '?' in app.request.requestUri %} {{ app.request.requestUri|replace({'?': '#defaultIdp?'}) }} @@ -25,16 +31,32 @@ class: 'remainingIdps__defaultIdp', text: defaultIdpText, id: 'defaultIdpDescription', - } %} - {% endif %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/rememberChoice.html.twig' %} - {% if showIdPBanner is defined and showIdPBanner %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpList.html.twig' with { idpList: idpListSorted, delete: false, listName: 'remaining', id: 'remainingIdps__title', showIdPBanner: showIdPBanner } %} - {% else %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpList.html.twig' with { idpList: idpListSorted, delete: false, listName: 'remaining', id: 'remainingIdps__title', showIdPBanner: false } %} + } only %} {% endif %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/rememberChoice.html.twig' with { + action: action, + rememberChoiceFeature: rememberChoiceFeature, + } only %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/idp/idpList.html.twig' with { + idpList: idpListSorted, + delete: false, + listName: 'remaining', + id: 'remainingIdps__title', + showIdPBanner: showIdPBanner, + showRequestAccess: showRequestAccess, + cutoffPointForShowingUnfilteredIdps: cutoffPointForShowingUnfilteredIdps, + action: action, + requestId: requestId, + } only %} diff --git a/theme/base/templates/modules/Authentication/View/Proxy/wayf.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/wayf.html.twig index ebda547252..89735794de 100644 --- a/theme/base/templates/modules/Authentication/View/Proxy/wayf.html.twig +++ b/theme/base/templates/modules/Authentication/View/Proxy/wayf.html.twig @@ -6,11 +6,60 @@ {% set organisationNoun = 'organisation_noun'|trans %} {% set pageTitle = 'log_in_to'|trans({'%arg1%': spName, '%organisationNoun%': organisationNoun}) %} -{# Data object containing the formatted IdP's #} -{% set connectedIdps = connectedIdps(idpList) %} - {% block title %}{{ parent() }} - {{ pageTitle }} {% endblock %} {% block content %} - {% include '@theme/Authentication/View/Proxy/wayf_content.html.twig' %} + {% include '@theme/Default/Partials/LoginBar.html.twig' with { loginName: 'suite_name'|trans } only %} +
+ {% include '@theme/Authentication/View/Proxy/Partials/Shared/header.html.twig' with { pageTitle: pageTitle } only %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noScript.html.twig' only %} + + {% block wayf_content %} +
+ {% include '@theme/Authentication/View/Proxy/Partials/Shared/site-notice.html.twig' only %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noAccess/successMessage.html.twig' only %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/preselection.html.twig' with { + connectedIdps: connectedIdps, + action: action, + requestId: requestId, + showRequestAccess: showRequestAccess, + cutoffPointForShowingUnfilteredIdps: cutoffPointForShowingUnfilteredIdps, + } only %} + {% if showPreferredIdps %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/preferredIdps.html.twig' with { + connectedIdps: connectedIdps, + preferredConnectedIdps: preferredConnectedIdps, + action: action, + requestId: requestId, + } only %} + {% endif %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/remainingIdps.html.twig' with { + connectedIdps: connectedIdps, + regularConnectedIdps: regularConnectedIdps, + showIdPBanner: showIdPBanner, + showPreferredIdps: showPreferredIdps, + action: action, + requestId: requestId, + rememberChoiceFeature: rememberChoiceFeature, + cutoffPointForShowingUnfilteredIdps: cutoffPointForShowingUnfilteredIdps, + showRequestAccess: showRequestAccess, + } only %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/backLink.html.twig' with { + backLink: backLink, + } only %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noAccess/noAccess.html.twig' with { + serviceProvider: serviceProvider, + } only %} + {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noResults.html.twig' only %} +
+ {% endblock %} +
+ {% include '@theme/Authentication/View/Proxy/Partials/WAYF/scriptConfig.html.twig' with { + connectedIdps: connectedIdps, + serviceProvider: serviceProvider, + showRequestAccess: showRequestAccess, + rememberChoiceFeature: rememberChoiceFeature, + cutoffPointForShowingUnfilteredIdps: cutoffPointForShowingUnfilteredIdps, + } only %} + {% include '@theme/Default/Partials/footer.html.twig' with { helpLink: helpLink } only %} {% endblock %} diff --git a/theme/base/templates/modules/Authentication/View/Proxy/wayf_content.html.twig b/theme/base/templates/modules/Authentication/View/Proxy/wayf_content.html.twig deleted file mode 100644 index 8c835488e8..0000000000 --- a/theme/base/templates/modules/Authentication/View/Proxy/wayf_content.html.twig +++ /dev/null @@ -1,19 +0,0 @@ -{% include '@theme/Default/Partials/LoginBar.html.twig' with { loginName: 'suite_name'|trans } %} -
- {% include '@theme/Authentication/View/Proxy/Partials/Shared/header.html.twig' %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noScript.html.twig' %} - - {% block wayf_content %} -
- {% include '@theme/Authentication/View/Proxy/Partials/Shared/site-notice.html.twig' %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noAccess/successMessage.html.twig' %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/preselection.html.twig' %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/remainingIdps.html.twig' %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/backLink.html.twig' %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noAccess/noAccess.html.twig' %} - {% include '@theme/Authentication/View/Proxy/Partials/WAYF/noResults.html.twig' %} -
- {% endblock %} -
-{% include '@theme/Authentication/View/Proxy/Partials/WAYF/scriptConfig.html.twig' %} -{% include '@theme/Default/Partials/footer.html.twig' %} diff --git a/theme/openconext/templates/modules/Authentication/View/Proxy/wayf.html.twig b/theme/openconext/templates/modules/Authentication/View/Proxy/wayf.html.twig index 7f713190b7..03dfeeb744 100644 --- a/theme/openconext/templates/modules/Authentication/View/Proxy/wayf.html.twig +++ b/theme/openconext/templates/modules/Authentication/View/Proxy/wayf.html.twig @@ -3,9 +3,6 @@ {# Prepare the page title #} {% set pageTitle = 'log_in_to'|trans %} -{# Data object containing the formatted IdP's #} -{% set connectedIdps = connectedIdps(idpList) %} - {% block title %}{{ parent() }} - {{ pageTitle }} {% endblock %} {% block pageHeading %}{{ parent() }} - {{ pageTitle }}{% endblock %} diff --git a/theme/skeune/translations/messages.en.php b/theme/skeune/translations/messages.en.php index 5554b44af0..39f4ae13d6 100644 --- a/theme/skeune/translations/messages.en.php +++ b/theme/skeune/translations/messages.en.php @@ -59,6 +59,7 @@ 'wayf_noaccess_form_announcement_screenreader' => 'Some required fields are not filled in, or not correctly filled in.', 'wayf_defaultIdp_start' => 'If your %organisation_noun% is not listed,', 'wayf_defaultIdp_linkText' => '%defaultIdpName% is available as an alternative.', + 'wayf_remaining_idps_search_label' => 'Or search for a Dutch institution from the list', 'wayf_idp_title_screenreader' => 'Login with ', 'wayf_idp_title_noaccess_screenreader' => 'No access with', diff --git a/theme/skeune/translations/messages.nl.php b/theme/skeune/translations/messages.nl.php index f3739827d2..f76f9940e1 100644 --- a/theme/skeune/translations/messages.nl.php +++ b/theme/skeune/translations/messages.nl.php @@ -58,6 +58,7 @@ 'wayf_noaccess_form_announcement_screenreader' => 'Er zijn verplichte velden niet, of niet goed ingevuld.', 'wayf_defaultIdp_start' => 'Als je %organisation_noun% niet in de lijst staat,', 'wayf_defaultIdp_linkText' => 'is %defaultIdpName% beschikbaar als alternatief.', + 'wayf_remaining_idps_search_label' => 'Of zoek een Nederlandse instelling uit de lijst', 'wayf_idp_title_screenreader' => 'Inloggen met ', 'wayf_idp_title_noaccess_screenreader' => 'Geen toegang met', diff --git a/theme/skeune/translations/messages.pt.php b/theme/skeune/translations/messages.pt.php index 7d08062275..60b21b8141 100644 --- a/theme/skeune/translations/messages.pt.php +++ b/theme/skeune/translations/messages.pt.php @@ -59,6 +59,7 @@ 'wayf_noaccess_form_announcement_screenreader' => 'Some required fields are not filled in, or not correctly filled in.', 'wayf_defaultIdp_start' => 'If your %organisation_noun% is not listed,', 'wayf_defaultIdp_linkText' => '%defaultIdpName% is available as an alternative.', + 'wayf_remaining_idps_search_label' => 'Ou procure uma instituição neerlandesa na lista', 'wayf_idp_title_screenreader' => 'Login with ', 'wayf_idp_title_noaccess_screenreader' => 'No access with', From 31c41a8c082ee71f3e6feeca231cda98523d4825 Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Tue, 21 Apr 2026 16:39:09 +0200 Subject: [PATCH 2/6] Extract WAYF rendering logic from Corto into Symfony WayfRenderer service Prior to this change, much of the wayf rendering happened in Corto. This change moves that logic to the Symfony workspaces. Benefits: - Business logic is now in a testable Symfony service with proper DI - Production (SingleSignOn) and the functional test (WayfController) share the exact same rendering path, eliminating logic duplication - DiContainerRuntime bridge is thinner --- CHANGELOG.md | 1 - config/services/ci/controllers.yml | 2 +- config/services/services.yml | 3 + .../Corto/Module/Service/SingleSignOn.php | 43 +--- .../EngineBlock/Service/Wayf/IdpSplitter.php | 2 +- .../Bridge/DiContainerRuntime.php | 6 +- .../Bridge/EngineBlockBootstrapper.php | 8 +- .../Service/WayfRenderer.php | 86 +++++++ .../Service/WayfViewModelFactory.php | 2 +- .../ViewModel/WayfViewModel.php | 2 +- .../Controllers/WayfController.php | 40 +--- .../skeune/wayf/wayf.keyboard.spec.js | 6 +- .../skeune/wayf/wayf.mouse.spec.js | 2 +- .../skeune/wayf/wayf.preferred.spec.js | 2 +- .../Test/Corto/Module/BindingsTest.php | 5 +- .../Service/Wayf/IdpSplitterTest.php | 2 +- .../DiContainerRuntimeTest.php | 11 +- .../Service/WayfRendererTest.php | 220 ++++++++++++++++++ 18 files changed, 348 insertions(+), 95 deletions(-) create mode 100644 src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php create mode 100644 tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index f64afb8b51..feff76a898 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,7 +36,6 @@ Changes: * A new parameter `wayf.preferred_idp_entity_ids` must be added to `parameters.yml`. To display a set of IdPs prominent at the top of the WAYF, add the entityId's of those IdPs to this parameter. * To keep the old behaviour, set the value to `[]` -**Default behaviour (no change):** when the parameter is absent or empty, the WAYF behaves exactly as before. * Stabilized consent checks * In order to make the consent hashes more robust, a more consistent way of hashing the user attributes has been introduced * This feature automatically migrates from the old hashes to the new hashes upon login. diff --git a/config/services/ci/controllers.yml b/config/services/ci/controllers.yml index 27a303cbc7..2113eabfcf 100644 --- a/config/services/ci/controllers.yml +++ b/config/services/ci/controllers.yml @@ -19,7 +19,7 @@ services: engineblock.functional_test.controller.wayf: class: OpenConext\EngineBlockFunctionalTestingBundle\Controllers\WayfController arguments: - - '@twig' + - '@OpenConext\EngineBlockBundle\Service\WayfRenderer' engineblock.functional_test.controller.feedback: class: OpenConext\EngineBlockFunctionalTestingBundle\Controllers\FeedbackController diff --git a/config/services/services.yml b/config/services/services.yml index 30a80c1943..d7613ec3f4 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -8,6 +8,9 @@ services: arguments: $wayfExtension: '@OpenConext\EngineBlockBundle\Twig\Extensions\Extension\Wayf' + OpenConext\EngineBlockBundle\Service\WayfRenderer: + autowire: true + OpenConext\EngineBlockBundle\Bridge\EngineBlockBootstrapper: autowire: true autoconfigure: true diff --git a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php index eb0e728a96..3589bd5260 100644 --- a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php +++ b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php @@ -475,40 +475,21 @@ protected function _showWayf(EngineBlock_Saml2_AuthnRequestAnnotationDecorator $ $container->getDefaultIdPEntityId() ); - $defaultIdPInIdPList = $this->isDefaultIdPPresent($idpList); - $diContainerRuntime = $application->getDiContainerRuntime(); - $preferredIdpEntityIds = $diContainerRuntime->getPreferredIdpEntityIds(); - $split = $diContainerRuntime->idpSplitter->split($idpList, $preferredIdpEntityIds); - $showPreferredIdps = !empty($split['preferred']); - $isDefaultIdpPreferred = in_array($container->getDefaultIdPEntityId(), $preferredIdpEntityIds, true); - $showDefaultIdpBanner = (bool) $container->shouldDisplayDefaultIdpBannerOnWayf() - && $defaultIdPInIdPList - && (!$showPreferredIdps || !$isDefaultIdpPreferred); - - $rememberChoiceFeature = $container->getRememberChoice(); - $viewModel = $diContainerRuntime->wayfViewModelFactory->create( + $output = $diContainerRuntime->wayfRenderer->render( idpList: $idpList, - regularIdpList: $split['regular'], - preferredIdpList: $split['preferred'], - showPreferredIdps: $showPreferredIdps, + preferredIdpEntityIds: $diContainerRuntime->getPreferredIdpEntityIds(), action: $action, - greenHeader: $serviceProvider->getDisplayName($currentLocale), - helpLink: '/authentication/idp/help-discover?lang=' . $currentLocale, + currentLocale: $currentLocale, + defaultIdpEntityId: $container->getDefaultIdPEntityId(), + shouldDisplayBanner: (bool) $container->shouldDisplayDefaultIdpBannerOnWayf(), backLink: $container->isUiOptionReturnToSpActive(), - cutoffPointForShowingUnfilteredIdps: $container->getCutoffPointForShowingUnfilteredIdps(), - showIdPBanner: $showDefaultIdpBanner, - rememberChoiceFeature: $rememberChoiceFeature, + cutoffPoint: $container->getCutoffPointForShowingUnfilteredIdps(), + rememberChoice: $container->getRememberChoice(), showRequestAccess: $serviceProvider->getCoins()->displayUnconnectedIdpsWayf(), requestId: $request->getId(), serviceProvider: $serviceProvider, - showRequestAccessContainer: true, - ); - - $output = $this->twig->render( - '@theme/Authentication/View/Proxy/wayf.html.twig', - $viewModel->toArray() ); $this->_server->sendOutput($output); } @@ -685,14 +666,4 @@ protected function getEngineSpRole(EngineBlock_Corto_ProxyServer $proxyServer) $serviceProvider = $this->_serviceProviderFactory->createEngineBlockEntityFrom($keyId); return ServiceProvider::fromServiceProviderEntity($serviceProvider); } - - private function isDefaultIdPPresent(array $idpList): bool - { - foreach ($idpList as $idp) { - if ($idp[self::IS_DEFAULT_IDP_KEY] === true) { - return true; - } - } - return false; - } } diff --git a/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php b/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php index e9e16a7088..94dd75e144 100644 --- a/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php +++ b/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php @@ -1,7 +1,7 @@ diContainerRuntime = new DiContainerRuntime($twig, $idpSplitter, $wayfViewModelFactory, $preferredIdpEntityIds); + $this->diContainerRuntime = new DiContainerRuntime($twig, $wayfRenderer, $preferredIdpEntityIds); } public function onKernelRequest(): void diff --git a/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php b/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php new file mode 100644 index 0000000000..7fd56bbb29 --- /dev/null +++ b/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php @@ -0,0 +1,86 @@ +splitter->split($idpList, $preferredIdpEntityIds); + $showPreferredIdps = !empty($split['preferred']); + $isDefaultIdpPreferred = in_array($defaultIdpEntityId, $preferredIdpEntityIds, true); + + $showIdPBanner = $shouldDisplayBanner + && $this->isDefaultIdpPresent($idpList) + && (!$showPreferredIdps || !$isDefaultIdpPreferred); + + $viewModel = $this->factory->create( + idpList: $idpList, + regularIdpList: $split['regular'], + preferredIdpList: $split['preferred'], + showPreferredIdps: $showPreferredIdps, + action: $action, + greenHeader: $serviceProvider->getDisplayName($currentLocale), + helpLink: '/authentication/idp/help-discover?lang=' . $currentLocale, + backLink: $backLink, + cutoffPointForShowingUnfilteredIdps: $cutoffPoint, + showIdPBanner: $showIdPBanner, + rememberChoiceFeature: $rememberChoice, + showRequestAccess: $showRequestAccess, + requestId: $requestId, + serviceProvider: $serviceProvider, + showRequestAccessContainer: true, + ); + + return $this->twig->render('@theme/Authentication/View/Proxy/wayf.html.twig', $viewModel->toArray()); + } + + private function isDefaultIdpPresent(array $idpList): bool + { + return array_any($idpList, fn($idp) => ($idp['isDefaultIdp'] ?? false) === true); + } +} diff --git a/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php b/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php index bf5a8bc2e8..1e9af2fa63 100644 --- a/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php +++ b/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php @@ -1,7 +1,7 @@ query->getBoolean('rememberChoiceFeature'); $cutoffPointForShowingUnfilteredIdps = $request->query->getInt('cutoffPointForShowingUnfilteredIdps', 100); $showIdPBanner = $request->query->getBoolean('showIdPBanner', true); - $defaultIdpEntityId = $request->query->get('defaultIdpEntityId'); + $defaultIdpEntityId = $request->query->get('defaultIdpEntityId', ''); $preferredIdpEntityIds = $request->query->all('preferredIdpEntityIds'); $connectedIdps = $request->query->getInt('connectedIdps', 5); @@ -61,36 +57,22 @@ public function wayfAction(Request $request) ? TestEntitySeeder::buildIdps($connectedIdps, $unconnectedIdps, $currentLocale, $defaultIdpEntityId, $addDiscoveries) : TestEntitySeeder::buildRandomIdps($randomIdps, $currentLocale, $defaultIdpEntityId); - $split = $this->idpSplitter->split($idpList, $preferredIdpEntityIds); - $preferredIdpList = $split['preferred']; - $regularIdpList = $split['regular']; - $showPreferredIdps = !empty($preferredIdpList); - - $isDefaultIdpPreferred = in_array($defaultIdpEntityId, $preferredIdpEntityIds, true); - $showIdPBanner = $showIdPBanner && (!$showPreferredIdps || !$isDefaultIdpPreferred); - - $viewModel = $this->wayfViewModelFactory->create( + $output = $this->wayfRenderer->render( idpList: $idpList, - regularIdpList: $regularIdpList, - preferredIdpList: $preferredIdpList, - showPreferredIdps: $showPreferredIdps, + preferredIdpEntityIds: $preferredIdpEntityIds, action: $this->generateUrl('functional_testing_handle_wayf'), - greenHeader: $currentLocale, - helpLink: '/authentication/idp/help-discover?lang=' . $currentLocale, + currentLocale: $currentLocale, + defaultIdpEntityId: $defaultIdpEntityId, + shouldDisplayBanner: $showIdPBanner, backLink: $backLink, - cutoffPointForShowingUnfilteredIdps: $cutoffPointForShowingUnfilteredIdps, - showIdPBanner: $showIdPBanner, - rememberChoiceFeature: $rememberChoiceFeature, + cutoffPoint: $cutoffPointForShowingUnfilteredIdps, + rememberChoice: $rememberChoiceFeature, showRequestAccess: $displayUnconnectedIdpsWayf, requestId: 'bogus-request-id', serviceProvider: TestEntitySeeder::buildSp(), - showRequestAccessContainer: true, ); - return new Response($this->twig->render( - '@theme/Authentication/View/Proxy/wayf.html.twig', - $viewModel->toArray() - )); + return new Response($output); } public function handleWayfAction(Request $request) diff --git a/tests/e2e/cypress/integration/skeune/wayf/wayf.keyboard.spec.js b/tests/e2e/cypress/integration/skeune/wayf/wayf.keyboard.spec.js index e0da11ad3f..f86e9eed6d 100644 --- a/tests/e2e/cypress/integration/skeune/wayf/wayf.keyboard.spec.js +++ b/tests/e2e/cypress/integration/skeune/wayf/wayf.keyboard.spec.js @@ -63,7 +63,7 @@ context('WAYF when using the keyboard', () => { // todo if html spec is changed, or cypress fixes bug 6207, get rid of the manual focus on search. See https://github.com/cypress-io/cypress/issues/6207 describe('Should be able to traverse the remaining idp section with arrow keys', () => { it('check if pressing down works as expected', () => { - cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?showIdpBanner=1'); + cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?showIdpBanner=1&defaultIdpEntityId=https://example.com/entityId/1'); cy.get(searchFieldSelector).focus(); cy.pressArrowOnIdpList('down', searchFieldClass); cy.pressArrowOnIdpList('down', defaultIdpClass); @@ -78,7 +78,7 @@ context('WAYF when using the keyboard', () => { }); it('check if pressing up works as expected', () => { - cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?showIdpBanner=1'); + cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?showIdpBanner=1&defaultIdpEntityId=https://example.com/entityId/1'); cy.get(searchFieldSelector).focus(); cy.pressArrowOnIdpList('up', searchFieldClass); cy.pressArrowOnIdpList('up', idpClass, '7'); @@ -183,7 +183,7 @@ context('WAYF when using the keyboard', () => { describe('Should have a working default Idp Banner', () => { it('Should have a default Idp banner visible', () => { - cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?showIdpBanner=1'); + cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?showIdpBanner=1&defaultIdpEntityId=https://example.com/entityId/1'); cy.beVisible(defaultIdpSelector); }); diff --git a/tests/e2e/cypress/integration/skeune/wayf/wayf.mouse.spec.js b/tests/e2e/cypress/integration/skeune/wayf/wayf.mouse.spec.js index bb018989de..0df782c1d0 100644 --- a/tests/e2e/cypress/integration/skeune/wayf/wayf.mouse.spec.js +++ b/tests/e2e/cypress/integration/skeune/wayf/wayf.mouse.spec.js @@ -85,7 +85,7 @@ context('WAYF when using the mouse', () => { describe('Should have a working default Idp Banner', () => { it('Should have a default Idp banner visible', () => { - cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?showIdpBanner=1'); + cy.visit('https://engine.dev.openconext.local/functional-testing/wayf?showIdpBanner=1&defaultIdpEntityId=https://example.com/entityId/1'); cy.get(defaultIdpSelector).should('be.visible'); }); diff --git a/tests/e2e/cypress/integration/skeune/wayf/wayf.preferred.spec.js b/tests/e2e/cypress/integration/skeune/wayf/wayf.preferred.spec.js index 9fa4620b9e..81e51742a4 100644 --- a/tests/e2e/cypress/integration/skeune/wayf/wayf.preferred.spec.js +++ b/tests/e2e/cypress/integration/skeune/wayf/wayf.preferred.spec.js @@ -79,7 +79,7 @@ describe('Preferred IdPs with previous selection', () => { describe('Three display scenarios', () => { it('scenario 1: no preferred IdPs: banner visible, preferred section absent', () => { - cy.visit(`${WAYF}?showIdPBanner=1`); + cy.visit(`${WAYF}?showIdPBanner=1&defaultIdpEntityId=${encodeURIComponent(ENTITY_1)}`); cy.get(defaultIdpInformational).should('be.visible'); cy.get(preferredIdpsSectionSelector).should('not.exist'); }); diff --git a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php index d8b421cf88..438fb8a14d 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php @@ -20,9 +20,8 @@ use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; -use OpenConext\EngineBlock\Service\Wayf\IdpSplitter; use OpenConext\EngineBlockBundle\Bridge\DiContainerRuntime; -use OpenConext\EngineBlockBundle\Service\WayfViewModelFactory; +use OpenConext\EngineBlockBundle\Service\WayfRenderer; use PHPUnit\Framework\TestCase; use SAML2\Assertion; use SAML2\Assertion\Validation\ConstraintValidator\NotBefore; @@ -61,7 +60,7 @@ public function setUp(): void Phake::when($this->proxyServer)->getConfig('WantsAuthnRequestsSigned')->thenReturn(false); $engineBlock = \EngineBlock_ApplicationSingleton::getInstance(); - $engineBlock->setDiContainerRuntime(new DiContainerRuntime(Phake::mock(Twig\Environment::class), new IdpSplitter(), Phake::mock(WayfViewModelFactory::class))); + $engineBlock->setDiContainerRuntime(new DiContainerRuntime(Phake::mock(Twig\Environment::class), Phake::mock(WayfRenderer::class))); $this->bindings = new EngineBlock_Corto_Module_Bindings($this->proxyServer); } diff --git a/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php b/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php index ff8ec2030b..b1cb86df6a 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php @@ -1,7 +1,7 @@ createStub(Environment::class), - new IdpSplitter(), - $this->createStub(WayfViewModelFactory::class), + $this->createStub(WayfRenderer::class), ); $this->assertSame([], $runtime->getPreferredIdpEntityIds()); @@ -43,8 +41,7 @@ public function testGetPreferredIdpEntityIdsReturnsConfiguredList(): void $runtime = new DiContainerRuntime( $this->createStub(Environment::class), - new IdpSplitter(), - $this->createStub(WayfViewModelFactory::class), + $this->createStub(WayfRenderer::class), $entityIds, ); diff --git a/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php b/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php new file mode 100644 index 0000000000..c104dcf30f --- /dev/null +++ b/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php @@ -0,0 +1,220 @@ +factory = $this->createMock(WayfViewModelFactory::class); + $this->twig = $this->createMock(Environment::class); + } + + private function renderer(): WayfRenderer + { + return new WayfRenderer( + factory: $this->factory, + splitter: new IdpSplitter(), + twig: $this->twig, + ); + } + + private function buildViewModel(bool $showIdPBanner = false, bool $showPreferredIdps = false): WayfViewModel + { + $emptyIdps = new ConnectedIdps([], []); + $sp = $this->createStub(ServiceProvider::class); + + return new WayfViewModel( + action: '/sso', + greenHeader: 'SP', + helpLink: '/help', + backLink: false, + cutoffPointForShowingUnfilteredIdps: 100, + showIdPBanner: $showIdPBanner, + rememberChoiceFeature: false, + showRequestAccess: false, + showRequestAccessContainer: true, + requestId: 'req-1', + serviceProvider: $sp, + connectedIdps: $emptyIdps, + regularConnectedIdps: $emptyIdps, + preferredConnectedIdps: $emptyIdps, + showPreferredIdps: $showPreferredIdps, + idpList: [], + regularIdpList: [], + preferredIdpList: [], + ); + } + + #[DataProvider('bannerConditionProvider')] + public function testBannerConditionPassedToFactory( + bool $shouldDisplayBanner, + bool $defaultIdpInList, + array $preferredIdpEntityIds, + string $defaultIdpEntityId, + bool $expectedShowBanner, + ): void { + $idpList = $defaultIdpInList + ? [['EntityID' => $defaultIdpEntityId, 'isDefaultIdp' => true, 'Access' => '1']] + : [['EntityID' => 'https://other.example.org', 'isDefaultIdp' => false, 'Access' => '1']]; + + $capturedShowIdPBanner = null; + + $this->factory->expects($this->once()) + ->method('create') + ->willReturnCallback(function () use (&$capturedShowIdPBanner): WayfViewModel { + $namedArgs = func_get_args(); + $capturedShowIdPBanner = $namedArgs[9]; + return $this->buildViewModel($namedArgs[9]); + }); + + $this->twig->method('render')->willReturn(''); + + $sp = $this->createStub(ServiceProvider::class); + $sp->method('getDisplayName')->willReturn('Test SP'); + + $this->renderer()->render( + idpList: $idpList, + preferredIdpEntityIds: $preferredIdpEntityIds, + action: '/sso', + currentLocale: 'en', + defaultIdpEntityId: $defaultIdpEntityId, + shouldDisplayBanner: $shouldDisplayBanner, + backLink: false, + cutoffPoint: 100, + rememberChoice: false, + showRequestAccess: false, + requestId: 'req-1', + serviceProvider: $sp, + ); + + $this->assertSame($expectedShowBanner, $capturedShowIdPBanner); + } + + public static function bannerConditionProvider(): array + { + $defaultId = 'https://default.example.org'; + $otherId = 'https://other.example.org'; + + return [ + 'banner off by config' => [false, true, [], $defaultId, false], + 'banner on, default not in list' => [true, false, [], $defaultId, false], + 'banner on, no preferred IdPs' => [true, true, [], $defaultId, true], + 'banner on, default is preferred (suppressed)' => [true, true, [$defaultId], $defaultId, false], + 'banner on, preferred shown but default is not one of them' => [true, true, [$otherId], $defaultId, true], + ]; + } + + public function testSplitsIdpListBeforePassingToFactory(): void + { + $preferredId = 'https://preferred.example.org'; + $regularId = 'https://regular.example.org'; + + $idpList = [ + ['EntityID' => $preferredId, 'isDefaultIdp' => false, 'Access' => '1'], + ['EntityID' => $regularId, 'isDefaultIdp' => false, 'Access' => '1'], + ]; + + $capturedRegular = null; + $capturedPreferred = null; + + $this->factory->expects($this->once()) + ->method('create') + ->willReturnCallback(function ( + array $idpList, + array $regularIdpList, + array $preferredIdpList, + ) use ( + &$capturedRegular, + &$capturedPreferred + ): WayfViewModel { + $capturedRegular = $regularIdpList; + $capturedPreferred = $preferredIdpList; + return $this->buildViewModel(); + }); + + $this->twig->method('render')->willReturn(''); + + $sp = $this->createStub(ServiceProvider::class); + $sp->method('getDisplayName')->willReturn('Test SP'); + + $this->renderer()->render( + idpList: $idpList, + preferredIdpEntityIds: [$preferredId], + action: '/sso', + currentLocale: 'en', + defaultIdpEntityId: '', + shouldDisplayBanner: false, + backLink: false, + cutoffPoint: 100, + rememberChoice: false, + showRequestAccess: false, + requestId: 'req-1', + serviceProvider: $sp, + ); + + $this->assertCount(1, $capturedPreferred); + $this->assertSame($preferredId, $capturedPreferred[0]['EntityID']); + $this->assertCount(1, $capturedRegular); + $this->assertSame($regularId, $capturedRegular[0]['EntityID']); + } + + public function testReturnsRenderedHtml(): void + { + $expectedHtml = 'WAYF'; + + $this->factory->method('create')->willReturn($this->buildViewModel()); + $this->twig->method('render')->willReturn($expectedHtml); + + $sp = $this->createStub(ServiceProvider::class); + $sp->method('getDisplayName')->willReturn('SP'); + + $result = $this->renderer()->render( + idpList: [], + preferredIdpEntityIds: [], + action: '/sso', + currentLocale: 'en', + defaultIdpEntityId: '', + shouldDisplayBanner: false, + backLink: false, + cutoffPoint: 100, + rememberChoice: false, + showRequestAccess: false, + requestId: 'req-1', + serviceProvider: $sp, + ); + + $this->assertSame($expectedHtml, $result); + } +} From daef5e494108488b6c94d3233f999279eec0ef22 Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Tue, 28 Apr 2026 10:27:08 +0200 Subject: [PATCH 3/6] DX: Introduce WayfIdp value object Prior to this change, tracing the IdP logic of the Wayf was difficult because it was just arrays. This change introduces the WayfIdp so its easy to keep track of where the object is used. --- .../Corto/Module/Service/SingleSignOn.php | 23 +++--- .../EngineBlock/Service/Wayf/IdpSplitter.php | 10 +-- .../EngineBlock/Service/Wayf/WayfIdp.php | 51 ++++++++++++ .../Service/WayfRenderer.php | 3 +- .../Twig/Extensions/Extension/Wayf.php | 34 +++----- .../ViewModel/WayfViewModel.php | 12 ++- .../Helper/TestEntitySeeder.php | 48 ++++++------ .../Service/Wayf/IdpSplitterTest.php | 40 +++++----- .../Service/WayfRendererTest.php | 13 ++-- .../Twig/Extensions/Extension/WayfTest.php | 77 ++++++++++--------- 10 files changed, 183 insertions(+), 128 deletions(-) create mode 100644 src/OpenConext/EngineBlock/Service/Wayf/WayfIdp.php diff --git a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php index 3589bd5260..3fa431c70c 100644 --- a/library/EngineBlock/Corto/Module/Service/SingleSignOn.php +++ b/library/EngineBlock/Corto/Module/Service/SingleSignOn.php @@ -20,6 +20,7 @@ use OpenConext\EngineBlock\Metadata\Factory\Factory\ServiceProviderFactory; use OpenConext\EngineBlock\Metadata\Discovery; use OpenConext\EngineBlock\Metadata\X509\KeyPairFactory; +use OpenConext\EngineBlock\Service\Wayf\WayfIdp; use OpenConext\EngineBlockBundle\Service\DiscoverySelectionService; use SAML2\AuthnRequest; use SAML2\Constants; @@ -537,21 +538,21 @@ protected function _transformIdpsForWayf(array $idpEntityIds, $isDebugRequest, $ private function buildIdp( ?string $name, string $logo, - $keywords, + array $keywords, string $entityId, bool $isAccessible, bool $isDefaultIdP, ?string $discoveryHash - ): array { - return array( - 'Name' => $name, - 'Logo' => $logo, - 'Keywords' => $keywords, - 'Access' => $isAccessible ? '1' : '0', - 'ID' => md5($entityId), - 'EntityID' => $entityId, - self::IS_DEFAULT_IDP_KEY => $isDefaultIdP, - 'DiscoveryHash' => $discoveryHash, + ): WayfIdp { + return new WayfIdp( + name: $name, + logo: $logo, + keywords: $keywords, + accessible: $isAccessible, + id: md5($entityId), + entityId: $entityId, + isDefaultIdp: $isDefaultIdP, + discoveryHash: $discoveryHash, ); } /** diff --git a/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php b/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php index 94dd75e144..1beb9b99f3 100644 --- a/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php +++ b/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php @@ -24,9 +24,9 @@ final class IdpSplitter * Splits the full IdP list into preferred (connected, in configured order) and regular (everything else). * Preferred IdPs that are not connected are excluded from both sections. * - * @param array $idpList Full transformed IdP list - * @param array $preferredEntityIds Ordered list of entity IDs to feature at the top - * @return array{preferred: array, regular: array} + * @param WayfIdp[] $idpList Full transformed IdP list + * @param array $preferredEntityIds Ordered list of entity IDs to feature at the top + * @return array{preferred: WayfIdp[], regular: WayfIdp[]} */ public function split(array $idpList, array $preferredEntityIds): array { @@ -39,9 +39,9 @@ public function split(array $idpList, array $preferredEntityIds): array $regular = []; foreach ($idpList as $idp) { - $entityId = $idp['EntityID']; + $entityId = $idp->entityId; if (isset($orderMap[$entityId])) { - if ($idp['Access'] === '1') { + if ($idp->accessible) { $preferredBuckets[$orderMap[$entityId]][] = $idp; } // Unconnected preferred IdPs are excluded from both sections. diff --git a/src/OpenConext/EngineBlock/Service/Wayf/WayfIdp.php b/src/OpenConext/EngineBlock/Service/Wayf/WayfIdp.php new file mode 100644 index 0000000000..284ae43689 --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/Wayf/WayfIdp.php @@ -0,0 +1,51 @@ + $this->name, + 'Logo' => $this->logo, + 'Keywords' => $this->keywords, + 'Access' => $this->accessible ? '1' : '0', + 'ID' => $this->id, + 'EntityID' => $this->entityId, + 'isDefaultIdp' => $this->isDefaultIdp, + 'DiscoveryHash' => $this->discoveryHash, + ]; + } +} diff --git a/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php b/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php index 7fd56bbb29..6334e2e8f1 100644 --- a/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php +++ b/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php @@ -22,6 +22,7 @@ use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\Wayf\IdpSplitter; +use OpenConext\EngineBlock\Service\Wayf\WayfIdp; use Twig\Environment; class WayfRenderer @@ -81,6 +82,6 @@ public function render( private function isDefaultIdpPresent(array $idpList): bool { - return array_any($idpList, fn($idp) => ($idp['isDefaultIdp'] ?? false) === true); + return array_any($idpList, fn(WayfIdp $idp) => $idp->isDefaultIdp); } } diff --git a/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Wayf.php b/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Wayf.php index e66737a8eb..7ba2a4c4b7 100644 --- a/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Wayf.php +++ b/src/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/Wayf.php @@ -19,6 +19,7 @@ namespace OpenConext\EngineBlockBundle\Twig\Extensions\Extension; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; +use OpenConext\EngineBlock\Service\Wayf\WayfIdp; use OpenConext\EngineBlockBundle\Service\IdpHistoryService; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Contracts\Translation\TranslatorInterface; @@ -29,8 +30,6 @@ class Wayf const PREVIOUS_SELECTION_COOKIE_NAME = 'selectedidps'; const REMEMBER_CHOICE_COOKIE_NAME = 'rememberchoice'; - private const ACCESS_ENABLED = '1'; - /** * @var \Symfony\Contracts\Translation\TranslatorInterface */ @@ -80,35 +79,26 @@ private function indexPreviousSelection(): array return array_column($this->previousSelection, null, 'idp'); } - private function formatIdpEntry(array $idp): array + private function formatIdpEntry(WayfIdp $idp): array { - $keywords = $idp['Keywords'] === 'Undefined' - ? [] - : array_values((array)$idp['Keywords']); - - $connected = false; - if (isset($idp['Access']) && $idp['Access'] === self::ACCESS_ENABLED) { - $connected = true; - } + $keywords = $idp->keywords; - // In SingleSignOn.php, the IDP is transformed into an array for the frontend - // Then, here, the array is transformed into another array for the frontend which is actually used in twig return [ - 'entityId' => $idp['EntityID'] ?? null, - 'connected' => $connected, - 'displayTitle' => $idp['Name'] ?? null, - 'title' => strtolower($idp['Name'] ?? ''), - 'keywords' => strtolower(implode('|', $keywords)), - 'logo' => $idp['Logo'] ?? null, - 'isDefaultIdp' => (bool) ($idp['isDefaultIdp'] ?? null), - 'discoveryHash' => $idp['DiscoveryHash'] ?? null, + 'entityId' => $idp->entityId, + 'connected' => $idp->accessible, + 'displayTitle' => $idp->name, + 'title' => strtolower($idp->name ?? ''), + 'keywords' => strtolower(implode('|', $keywords)), + 'logo' => $idp->logo, + 'isDefaultIdp' => $idp->isDefaultIdp, + 'discoveryHash' => $idp->discoveryHash, ]; } private function formatIdpList(array $idpList): array { return array_map( - $this->formatIdpEntry(...), + fn(WayfIdp $idp) => $this->formatIdpEntry($idp), $idpList ); } diff --git a/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php b/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php index e0651330d9..c57a4726ed 100644 --- a/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php +++ b/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php @@ -21,6 +21,7 @@ namespace OpenConext\EngineBlockBundle\ViewModel; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; +use OpenConext\EngineBlock\Service\Wayf\WayfIdp; use OpenConext\EngineBlockBundle\Twig\Extensions\Extension\ConnectedIdps; final readonly class WayfViewModel @@ -44,16 +45,23 @@ public function __construct( public ConnectedIdps $regularConnectedIdps, public ConnectedIdps $preferredConnectedIdps, public bool $showPreferredIdps, - // These Raw arrays kept for backward compatibility with custom theme overrides. + // These WayfIdp arrays kept for backward compatibility with custom theme overrides. // The base / skeune theme do not use them, but potentially downstream themes may rely on them so we keep them in. + /** @var WayfIdp[] */ public array $idpList, + /** @var WayfIdp[] */ public array $regularIdpList, + /** @var WayfIdp[] */ public array $preferredIdpList, ) { } public function toArray(): array { - return get_object_vars($this); + $vars = get_object_vars($this); + $vars['idpList'] = array_map(static fn(WayfIdp $idp) => $idp->toArray(), $this->idpList); + $vars['regularIdpList'] = array_map(static fn(WayfIdp $idp) => $idp->toArray(), $this->regularIdpList); + $vars['preferredIdpList'] = array_map(static fn(WayfIdp $idp) => $idp->toArray(), $this->preferredIdpList); + return $vars; } } diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Helper/TestEntitySeeder.php b/src/OpenConext/EngineBlockFunctionalTestingBundle/Helper/TestEntitySeeder.php index 45785bea7d..0c17641a8a 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Helper/TestEntitySeeder.php +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Helper/TestEntitySeeder.php @@ -23,6 +23,7 @@ use OpenConext\EngineBlock\Metadata\Entity\IdentityProvider; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Metadata\Logo; +use OpenConext\EngineBlock\Service\Wayf\WayfIdp; use OpenConext\EngineBlockBundle\Service\DiscoverySelectionService; use Webmozart\Assert\Assert; @@ -188,47 +189,42 @@ public static function buildRandomIdps($numberOfIdps, $locale, $defaultIdpEntity /** * @param array $idpEntityIds * @param string $currentLocale - * @return array[] + * @return WayfIdp[] */ private static function transformIdpsForWayf(array $idpEntityIds, $currentLocale) { $discoveryService = new DiscoverySelectionService(); $identityProviders = self::findIdentityProvidersByEntityId($idpEntityIds); - $wayfIdps = array(); + $wayfIdps = []; foreach ($identityProviders as $identityProvider) { $name = 'name' . ucfirst($currentLocale); - $wayfIdp = array( - 'Name' => $identityProvider->$name, - 'Logo' => $identityProvider->logo ? $identityProvider->logo->url : '/images/placeholder.png', - 'Keywords' => $identityProvider->keywordsEn, - 'Access' => ($identityProvider->enabledInWayf) ? '1' : '0', - 'ID' => md5($identityProvider->entityId), - 'EntityID' => $identityProvider->entityId, - 'isDefaultIdp' => $idpEntityIds[$identityProvider->entityId]['isDefaultIdp'], + $wayfIdps[] = new WayfIdp( + name: $identityProvider->$name, + logo: $identityProvider->logo ? $identityProvider->logo->url : '/images/placeholder.png', + keywords: (array) $identityProvider->keywordsEn, + accessible: $identityProvider->enabledInWayf, + id: md5($identityProvider->entityId), + entityId: $identityProvider->entityId, + isDefaultIdp: $idpEntityIds[$identityProvider->entityId]['isDefaultIdp'], + discoveryHash: null, ); - $wayfIdps[] = $wayfIdp; foreach ($identityProvider->getDiscoveries() as $discovery) { - $wayfIdps[] = array( - 'Name' => $discovery->getName($currentLocale), - 'Logo' => $discovery->getLogo() ? $discovery->getLogo()->url : '/images/placeholder.png', - 'Keywords' => $discovery->getKeywords('en'), - 'Access' => ($identityProvider->enabledInWayf) ? '1' : '0', - 'ID' => md5($identityProvider->entityId), - 'EntityID' => $identityProvider->entityId, - 'isDefaultIdp' => $idpEntityIds[$identityProvider->entityId]['isDefaultIdp'], - 'DiscoveryHash' => $discoveryService->hash($discovery), + $wayfIdps[] = new WayfIdp( + name: $discovery->getName($currentLocale), + logo: $discovery->getLogo() ? $discovery->getLogo()->url : '/images/placeholder.png', + keywords: $discovery->getKeywordsArray($currentLocale), + accessible: $identityProvider->enabledInWayf, + id: md5($identityProvider->entityId), + entityId: $identityProvider->entityId, + isDefaultIdp: $idpEntityIds[$identityProvider->entityId]['isDefaultIdp'], + discoveryHash: $discoveryService->hash($discovery), ); } } - $nameSort = static function ($a, $b) { - return strcmp(strtolower($a['Name']), strtolower($b['Name'])); - }; - - // Sort the IdP entries by name - usort($wayfIdps, $nameSort); + usort($wayfIdps, static fn(WayfIdp $a, WayfIdp $b) => strcmp(strtolower($a->name ?? ''), strtolower($b->name ?? ''))); return $wayfIdps; } diff --git a/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php b/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php index b1cb86df6a..011fa54748 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php @@ -19,6 +19,7 @@ namespace Tests\OpenConext\EngineBlock\Service\Wayf; use OpenConext\EngineBlock\Service\Wayf\IdpSplitter; +use OpenConext\EngineBlock\Service\Wayf\WayfIdp; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -36,17 +37,18 @@ private function split(array $idpList, array $preferredEntityIds): array return $this->splitter->split($idpList, $preferredEntityIds); } - private function idp(string $entityId, string $access = '1', string $discoveryHash = ''): array + private function idp(string $entityId, string $access = '1', string $discoveryHash = ''): WayfIdp { - return [ - 'EntityID' => $entityId, - 'Access' => $access, - 'Name' => $entityId, - 'Keywords' => [], - 'Logo' => '', - 'isDefaultIdp' => false, - 'DiscoveryHash' => $discoveryHash, - ]; + return new WayfIdp( + name: $entityId, + logo: '', + keywords: [], + accessible: $access === '1', + id: md5($entityId), + entityId: $entityId, + isDefaultIdp: false, + discoveryHash: $discoveryHash !== '' ? $discoveryHash : null, + ); } public function testEmptyPreferredEntityIdsReturnsFullListAsRegular(): void @@ -67,9 +69,9 @@ public function testPreferredIdpIsMovedOutOfRegularList(): void $result = $this->split([$idp1, $idp2], ['https://idp1.example.org']); $this->assertCount(1, $result['preferred']); - $this->assertSame('https://idp1.example.org', $result['preferred'][0]['EntityID']); + $this->assertSame('https://idp1.example.org', $result['preferred'][0]->entityId); $this->assertCount(1, $result['regular']); - $this->assertSame('https://idp2.example.org', $result['regular'][0]['EntityID']); + $this->assertSame('https://idp2.example.org', $result['regular'][0]->entityId); } public function testConfiguredOrderIsPreservedInPreferredList(): void @@ -84,10 +86,10 @@ public function testConfiguredOrderIsPreservedInPreferredList(): void ); $this->assertCount(2, $result['preferred']); - $this->assertSame('https://idp3.example.org', $result['preferred'][0]['EntityID']); - $this->assertSame('https://idp1.example.org', $result['preferred'][1]['EntityID']); + $this->assertSame('https://idp3.example.org', $result['preferred'][0]->entityId); + $this->assertSame('https://idp1.example.org', $result['preferred'][1]->entityId); $this->assertCount(1, $result['regular']); - $this->assertSame('https://idp2.example.org', $result['regular'][0]['EntityID']); + $this->assertSame('https://idp2.example.org', $result['regular'][0]->entityId); } public function testDisconnectedPreferredIdpIsExcludedFromBothLists(): void @@ -103,7 +105,7 @@ public function testDisconnectedPreferredIdpIsExcludedFromBothLists(): void $this->assertSame([], $result['preferred']); // idp1 is not preferred so it stays in regular; idp2 is preferred but disconnected > excluded from both $this->assertCount(1, $result['regular']); - $this->assertSame('https://idp1.example.org', $result['regular'][0]['EntityID']); + $this->assertSame('https://idp1.example.org', $result['regular'][0]->entityId); } public function testMultipleDiscoveryEntriesForSameEntityIdAreGroupedInOrder(): void @@ -118,10 +120,10 @@ public function testMultipleDiscoveryEntriesForSameEntityIdAreGroupedInOrder(): ); $this->assertCount(2, $result['preferred']); - $this->assertSame('https://idp1.example.org', $result['preferred'][0]['EntityID']); - $this->assertSame('https://idp1.example.org', $result['preferred'][1]['EntityID']); + $this->assertSame('https://idp1.example.org', $result['preferred'][0]->entityId); + $this->assertSame('https://idp1.example.org', $result['preferred'][1]->entityId); $this->assertCount(1, $result['regular']); - $this->assertSame('https://idp2.example.org', $result['regular'][0]['EntityID']); + $this->assertSame('https://idp2.example.org', $result['regular'][0]->entityId); } public function testPreferredEntityIdNotInIdpListIsIgnored(): void diff --git a/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php b/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php index c104dcf30f..e49214ea6e 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php @@ -22,6 +22,7 @@ use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; use OpenConext\EngineBlock\Service\Wayf\IdpSplitter; +use OpenConext\EngineBlock\Service\Wayf\WayfIdp; use OpenConext\EngineBlockBundle\Service\WayfRenderer; use OpenConext\EngineBlockBundle\Service\WayfViewModelFactory; use OpenConext\EngineBlockBundle\Twig\Extensions\Extension\ConnectedIdps; @@ -86,8 +87,8 @@ public function testBannerConditionPassedToFactory( bool $expectedShowBanner, ): void { $idpList = $defaultIdpInList - ? [['EntityID' => $defaultIdpEntityId, 'isDefaultIdp' => true, 'Access' => '1']] - : [['EntityID' => 'https://other.example.org', 'isDefaultIdp' => false, 'Access' => '1']]; + ? [new WayfIdp(name: null, logo: '', keywords: [], accessible: true, id: md5($defaultIdpEntityId), entityId: $defaultIdpEntityId, isDefaultIdp: true, discoveryHash: null)] + : [new WayfIdp(name: null, logo: '', keywords: [], accessible: true, id: md5('other'), entityId: 'https://other.example.org', isDefaultIdp: false, discoveryHash: null)]; $capturedShowIdPBanner = null; @@ -142,8 +143,8 @@ public function testSplitsIdpListBeforePassingToFactory(): void $regularId = 'https://regular.example.org'; $idpList = [ - ['EntityID' => $preferredId, 'isDefaultIdp' => false, 'Access' => '1'], - ['EntityID' => $regularId, 'isDefaultIdp' => false, 'Access' => '1'], + new WayfIdp(name: null, logo: '', keywords: [], accessible: true, id: md5($preferredId), entityId: $preferredId, isDefaultIdp: false, discoveryHash: null), + new WayfIdp(name: null, logo: '', keywords: [], accessible: true, id: md5($regularId), entityId: $regularId, isDefaultIdp: false, discoveryHash: null), ]; $capturedRegular = null; @@ -185,9 +186,9 @@ public function testSplitsIdpListBeforePassingToFactory(): void ); $this->assertCount(1, $capturedPreferred); - $this->assertSame($preferredId, $capturedPreferred[0]['EntityID']); + $this->assertSame($preferredId, $capturedPreferred[0]->entityId); $this->assertCount(1, $capturedRegular); - $this->assertSame($regularId, $capturedRegular[0]['EntityID']); + $this->assertSame($regularId, $capturedRegular[0]->entityId); } public function testReturnsRenderedHtml(): void diff --git a/tests/unit/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/WayfTest.php b/tests/unit/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/WayfTest.php index b996f139e4..3b732a43d4 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/WayfTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Twig/Extensions/Extension/WayfTest.php @@ -19,6 +19,7 @@ namespace Tests\OpenConext\EngineBlockBundle\Twig\Extensions\Extension; use OpenConext\EngineBlock\Metadata\Entity\ServiceProvider; +use OpenConext\EngineBlock\Service\Wayf\WayfIdp; use OpenConext\EngineBlockBundle\Twig\Extensions\Extension\ConnectedIdps; use OpenConext\EngineBlockBundle\Twig\Extensions\Extension\Wayf; use PHPUnit\Framework\Attributes\DataProvider; @@ -43,24 +44,26 @@ protected function setUp(): void public function testGetConnectedIdpsWithEmptyPreviousSelection() { $idpList = [ - [ - 'EntityID' => 'https://idp1.example.org', - 'Access' => '1', - 'Name' => 'IDP One', - 'Keywords' => ['university', 'education'], - 'Logo' => 'logo1.png', - 'isDefaultIdp' => false, - 'DiscoveryHash' => 'hash1' - ], - [ - 'EntityID' => 'https://idp2.example.org', - 'Access' => '0', - 'Name' => 'IDP Two', - 'Keywords' => 'Undefined', - 'Logo' => 'logo2.png', - 'isDefaultIdp' => true, - 'DiscoveryHash' => 'hash2' - ] + new WayfIdp( + name: 'IDP One', + logo: 'logo1.png', + keywords: ['university', 'education'], + accessible: true, + id: md5('https://idp1.example.org'), + entityId: 'https://idp1.example.org', + isDefaultIdp: false, + discoveryHash: 'hash1', + ), + new WayfIdp( + name: 'IDP Two', + logo: 'logo2.png', + keywords: [], + accessible: false, + id: md5('https://idp2.example.org'), + entityId: 'https://idp2.example.org', + isDefaultIdp: true, + discoveryHash: 'hash2', + ), ]; $result = $this->wayf->getConnectedIdps($idpList); @@ -118,24 +121,26 @@ public function testGetConnectedIdpsWithPreviousSelection(string $storedCookieVa $wayf = new Wayf($requestStack, $this->translator); $idpList = [ - [ - 'EntityID' => 'https://idp1.example.org', - 'Access' => '1', - 'Name' => 'IDP One', - 'Keywords' => ['university', 'education'], - 'Logo' => 'logo1.png', - 'isDefaultIdp' => false, - 'DiscoveryHash' => '' - ], - [ - 'EntityID' => 'https://idp1.example.org', - 'Access' => '1', - 'Name' => 'IDP One Discovery', - 'Keywords' => [], - 'Logo' => 'logo2.png', - 'isDefaultIdp' => false, - 'DiscoveryHash' => 'hash1' - ] + new WayfIdp( + name: 'IDP One', + logo: 'logo1.png', + keywords: ['university', 'education'], + accessible: true, + id: md5('https://idp1.example.org'), + entityId: 'https://idp1.example.org', + isDefaultIdp: false, + discoveryHash: null, + ), + new WayfIdp( + name: 'IDP One Discovery', + logo: 'logo2.png', + keywords: [], + accessible: true, + id: md5('https://idp1.example.org'), + entityId: 'https://idp1.example.org', + isDefaultIdp: false, + discoveryHash: 'hash1', + ), ]; $result = $wayf->getConnectedIdps($idpList); From 93c0e0b8b56c5fc906c46f16019204a562d153bd Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Tue, 28 Apr 2026 12:07:59 +0200 Subject: [PATCH 4/6] Fix theoretical issue with unconnected defaultIdp and the banner not showing on the functional-testing route. --- .../Service/WayfRenderer.php | 3 +- .../Helper/TestEntitySeeder.php | 11 ++++- .../Service/WayfRendererTest.php | 45 +++++++++++++++++++ 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php b/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php index 6334e2e8f1..78892e68c4 100644 --- a/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php +++ b/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php @@ -53,7 +53,8 @@ public function render( ): string { $split = $this->splitter->split($idpList, $preferredIdpEntityIds); $showPreferredIdps = !empty($split['preferred']); - $isDefaultIdpPreferred = in_array($defaultIdpEntityId, $preferredIdpEntityIds, true); + $preferredEntityIdsShown = array_map(static fn(WayfIdp $idp) => $idp->entityId, $split['preferred']); + $isDefaultIdpPreferred = in_array($defaultIdpEntityId, $preferredEntityIdsShown, true); $showIdPBanner = $shouldDisplayBanner && $this->isDefaultIdpPresent($idpList) diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Helper/TestEntitySeeder.php b/src/OpenConext/EngineBlockFunctionalTestingBundle/Helper/TestEntitySeeder.php index 0c17641a8a..d301effeeb 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Helper/TestEntitySeeder.php +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Helper/TestEntitySeeder.php @@ -198,6 +198,13 @@ private static function transformIdpsForWayf(array $idpEntityIds, $currentLocale $wayfIdps = []; foreach ($identityProviders as $identityProvider) { + $isDefaultIdp = $idpEntityIds[$identityProvider->entityId]['isDefaultIdp']; + + // Mirror the production guard: do not show the default IdP in the disconnected section. + if (!$identityProvider->enabledInWayf && $isDefaultIdp) { + continue; + } + $name = 'name' . ucfirst($currentLocale); $wayfIdps[] = new WayfIdp( name: $identityProvider->$name, @@ -206,7 +213,7 @@ private static function transformIdpsForWayf(array $idpEntityIds, $currentLocale accessible: $identityProvider->enabledInWayf, id: md5($identityProvider->entityId), entityId: $identityProvider->entityId, - isDefaultIdp: $idpEntityIds[$identityProvider->entityId]['isDefaultIdp'], + isDefaultIdp: $isDefaultIdp, discoveryHash: null, ); @@ -218,7 +225,7 @@ private static function transformIdpsForWayf(array $idpEntityIds, $currentLocale accessible: $identityProvider->enabledInWayf, id: md5($identityProvider->entityId), entityId: $identityProvider->entityId, - isDefaultIdp: $idpEntityIds[$identityProvider->entityId]['isDefaultIdp'], + isDefaultIdp: $isDefaultIdp, discoveryHash: $discoveryService->hash($discovery), ); } diff --git a/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php b/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php index e49214ea6e..6f602ed4af 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php @@ -137,6 +137,51 @@ public static function bannerConditionProvider(): array ]; } + public function testBannerShownWhenDefaultIdpIsInPreferredConfigButDroppedBySpitter(): void + { + $defaultId = 'https://default.example.org'; + $otherId = 'https://other.example.org'; + + // Default IdP is unconnected (splitter will drop it); other preferred IdP is connected. + $idpList = [ + new WayfIdp(name: null, logo: '', keywords: [], accessible: false, id: md5($defaultId), entityId: $defaultId, isDefaultIdp: true, discoveryHash: null), + new WayfIdp(name: null, logo: '', keywords: [], accessible: true, id: md5($otherId), entityId: $otherId, isDefaultIdp: false, discoveryHash: null), + ]; + + $capturedShowIdPBanner = null; + + $this->factory->expects($this->once()) + ->method('create') + ->willReturnCallback(function () use (&$capturedShowIdPBanner): WayfViewModel { + $namedArgs = func_get_args(); + $capturedShowIdPBanner = $namedArgs[9]; + return $this->buildViewModel($namedArgs[9]); + }); + + $this->twig->method('render')->willReturn(''); + + $sp = $this->createStub(ServiceProvider::class); + $sp->method('getDisplayName')->willReturn('Test SP'); + + $this->renderer()->render( + idpList: $idpList, + preferredIdpEntityIds: [$defaultId, $otherId], + action: '/sso', + currentLocale: 'en', + defaultIdpEntityId: $defaultId, + shouldDisplayBanner: true, + backLink: false, + cutoffPoint: 100, + rememberChoice: false, + showRequestAccess: false, + requestId: 'req-1', + serviceProvider: $sp, + ); + + $this->assertTrue($capturedShowIdPBanner, 'Banner should show: default IdP was in preferred config but dropped by splitter, so it is not visible anywhere'); + } + + public function testSplitsIdpListBeforePassingToFactory(): void { $preferredId = 'https://preferred.example.org'; From 142fd65b7793f65f5f11328a2a01a704d3dc1626 Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Tue, 28 Apr 2026 12:24:25 +0200 Subject: [PATCH 5/6] Use value object as return in IdPSplitter Prior to this change, bugs could occur because the array returned rom the IdP Splitter exposed all data. This change adds a value object in the return, so logic can be better encapsulated. --- .../EngineBlock/Service/Wayf/IdpSplitter.php | 7 ++- .../Service/Wayf/WayfSplitResult.php | 44 +++++++++++++++++ .../Service/WayfRenderer.php | 9 ++-- .../Service/Wayf/IdpSplitterTest.php | 49 ++++++++++--------- 4 files changed, 76 insertions(+), 33 deletions(-) create mode 100644 src/OpenConext/EngineBlock/Service/Wayf/WayfSplitResult.php diff --git a/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php b/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php index 1beb9b99f3..56b6ebc41b 100644 --- a/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php +++ b/src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php @@ -26,12 +26,11 @@ final class IdpSplitter * * @param WayfIdp[] $idpList Full transformed IdP list * @param array $preferredEntityIds Ordered list of entity IDs to feature at the top - * @return array{preferred: WayfIdp[], regular: WayfIdp[]} */ - public function split(array $idpList, array $preferredEntityIds): array + public function split(array $idpList, array $preferredEntityIds): WayfSplitResult { if (empty($preferredEntityIds)) { - return ['preferred' => [], 'regular' => $idpList]; + return new WayfSplitResult(preferred: [], regular: $idpList); } $orderMap = array_flip($preferredEntityIds); @@ -53,6 +52,6 @@ public function split(array $idpList, array $preferredEntityIds): array $mergeArgs = array_values(array_filter($preferredBuckets)); $preferred = empty($mergeArgs) ? [] : array_merge(...$mergeArgs); - return ['preferred' => $preferred, 'regular' => $regular]; + return new WayfSplitResult(preferred: $preferred, regular: $regular); } } diff --git a/src/OpenConext/EngineBlock/Service/Wayf/WayfSplitResult.php b/src/OpenConext/EngineBlock/Service/Wayf/WayfSplitResult.php new file mode 100644 index 0000000000..dd238d79b6 --- /dev/null +++ b/src/OpenConext/EngineBlock/Service/Wayf/WayfSplitResult.php @@ -0,0 +1,44 @@ +preferred); + } + + public function containsInPreferred(string $entityId): bool + { + return array_any($this->preferred, static fn($idp) => $idp->entityId === $entityId); + } +} diff --git a/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php b/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php index 78892e68c4..b85f14e5e5 100644 --- a/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php +++ b/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php @@ -52,9 +52,8 @@ public function render( ServiceProvider $serviceProvider, ): string { $split = $this->splitter->split($idpList, $preferredIdpEntityIds); - $showPreferredIdps = !empty($split['preferred']); - $preferredEntityIdsShown = array_map(static fn(WayfIdp $idp) => $idp->entityId, $split['preferred']); - $isDefaultIdpPreferred = in_array($defaultIdpEntityId, $preferredEntityIdsShown, true); + $showPreferredIdps = $split->hasPreferred(); + $isDefaultIdpPreferred = $split->containsInPreferred($defaultIdpEntityId); $showIdPBanner = $shouldDisplayBanner && $this->isDefaultIdpPresent($idpList) @@ -62,8 +61,8 @@ public function render( $viewModel = $this->factory->create( idpList: $idpList, - regularIdpList: $split['regular'], - preferredIdpList: $split['preferred'], + regularIdpList: $split->regular, + preferredIdpList: $split->preferred, showPreferredIdps: $showPreferredIdps, action: $action, greenHeader: $serviceProvider->getDisplayName($currentLocale), diff --git a/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php b/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php index 011fa54748..eeebfe86c6 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php @@ -20,6 +20,7 @@ use OpenConext\EngineBlock\Service\Wayf\IdpSplitter; use OpenConext\EngineBlock\Service\Wayf\WayfIdp; +use OpenConext\EngineBlock\Service\Wayf\WayfSplitResult; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; @@ -32,7 +33,7 @@ protected function setUp(): void $this->splitter = new IdpSplitter(); } - private function split(array $idpList, array $preferredEntityIds): array + private function split(array $idpList, array $preferredEntityIds): WayfSplitResult { return $this->splitter->split($idpList, $preferredEntityIds); } @@ -57,8 +58,8 @@ public function testEmptyPreferredEntityIdsReturnsFullListAsRegular(): void $result = $this->split($idpList, []); - $this->assertSame([], $result['preferred']); - $this->assertSame($idpList, $result['regular']); + $this->assertSame([], $result->preferred); + $this->assertSame($idpList, $result->regular); } public function testPreferredIdpIsMovedOutOfRegularList(): void @@ -68,10 +69,10 @@ public function testPreferredIdpIsMovedOutOfRegularList(): void $result = $this->split([$idp1, $idp2], ['https://idp1.example.org']); - $this->assertCount(1, $result['preferred']); - $this->assertSame('https://idp1.example.org', $result['preferred'][0]->entityId); - $this->assertCount(1, $result['regular']); - $this->assertSame('https://idp2.example.org', $result['regular'][0]->entityId); + $this->assertCount(1, $result->preferred); + $this->assertSame('https://idp1.example.org', $result->preferred[0]->entityId); + $this->assertCount(1, $result->regular); + $this->assertSame('https://idp2.example.org', $result->regular[0]->entityId); } public function testConfiguredOrderIsPreservedInPreferredList(): void @@ -85,11 +86,11 @@ public function testConfiguredOrderIsPreservedInPreferredList(): void ['https://idp3.example.org', 'https://idp1.example.org'] ); - $this->assertCount(2, $result['preferred']); - $this->assertSame('https://idp3.example.org', $result['preferred'][0]->entityId); - $this->assertSame('https://idp1.example.org', $result['preferred'][1]->entityId); - $this->assertCount(1, $result['regular']); - $this->assertSame('https://idp2.example.org', $result['regular'][0]->entityId); + $this->assertCount(2, $result->preferred); + $this->assertSame('https://idp3.example.org', $result->preferred[0]->entityId); + $this->assertSame('https://idp1.example.org', $result->preferred[1]->entityId); + $this->assertCount(1, $result->regular); + $this->assertSame('https://idp2.example.org', $result->regular[0]->entityId); } public function testDisconnectedPreferredIdpIsExcludedFromBothLists(): void @@ -102,10 +103,10 @@ public function testDisconnectedPreferredIdpIsExcludedFromBothLists(): void ['https://idp2.example.org'] ); - $this->assertSame([], $result['preferred']); + $this->assertSame([], $result->preferred); // idp1 is not preferred so it stays in regular; idp2 is preferred but disconnected > excluded from both - $this->assertCount(1, $result['regular']); - $this->assertSame('https://idp1.example.org', $result['regular'][0]->entityId); + $this->assertCount(1, $result->regular); + $this->assertSame('https://idp1.example.org', $result->regular[0]->entityId); } public function testMultipleDiscoveryEntriesForSameEntityIdAreGroupedInOrder(): void @@ -119,11 +120,11 @@ public function testMultipleDiscoveryEntriesForSameEntityIdAreGroupedInOrder(): ['https://idp1.example.org'] ); - $this->assertCount(2, $result['preferred']); - $this->assertSame('https://idp1.example.org', $result['preferred'][0]->entityId); - $this->assertSame('https://idp1.example.org', $result['preferred'][1]->entityId); - $this->assertCount(1, $result['regular']); - $this->assertSame('https://idp2.example.org', $result['regular'][0]->entityId); + $this->assertCount(2, $result->preferred); + $this->assertSame('https://idp1.example.org', $result->preferred[0]->entityId); + $this->assertSame('https://idp1.example.org', $result->preferred[1]->entityId); + $this->assertCount(1, $result->regular); + $this->assertSame('https://idp2.example.org', $result->regular[0]->entityId); } public function testPreferredEntityIdNotInIdpListIsIgnored(): void @@ -132,8 +133,8 @@ public function testPreferredEntityIdNotInIdpListIsIgnored(): void $result = $this->split([$idp1], ['https://nonexistent.example.org']); - $this->assertSame([], $result['preferred']); - $this->assertCount(1, $result['regular']); + $this->assertSame([], $result->preferred); + $this->assertCount(1, $result->regular); } public static function fiveScenarioProvider(): array @@ -162,8 +163,8 @@ public function testFiveScenarioBannerAndPreferredVisibility( $split = $this->split($idpList, $preferredEntityIds); - $showPreferred = !empty($split['preferred']); - $isDefaultPreferred = in_array($defaultIdpEntityId, $preferredEntityIds, true); + $showPreferred = $split->hasPreferred(); + $isDefaultPreferred = $split->containsInPreferred($defaultIdpEntityId); $showBanner = $defaultIdpConnected && (!$showPreferred || !$isDefaultPreferred); $this->assertSame($expectShowPreferred, $showPreferred, 'showPreferredIdps mismatch'); From 4f20aa35ab47041a77ca7b2e0066710dc45c4cc0 Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Tue, 28 Apr 2026 15:36:02 +0200 Subject: [PATCH 6/6] Cleanup unused showRequestAccessContainer --- src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php | 1 - .../EngineBlockBundle/Service/WayfViewModelFactory.php | 2 -- src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php | 1 - .../OpenConext/EngineBlockBundle/Service/WayfRendererTest.php | 3 +-- theme/openconext/templates/layouts/scripts/default.html.twig | 4 +--- 5 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php b/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php index b85f14e5e5..7e56308ceb 100644 --- a/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php +++ b/src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php @@ -74,7 +74,6 @@ public function render( showRequestAccess: $showRequestAccess, requestId: $requestId, serviceProvider: $serviceProvider, - showRequestAccessContainer: true, ); return $this->twig->render('@theme/Authentication/View/Proxy/wayf.html.twig', $viewModel->toArray()); diff --git a/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php b/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php index 1e9af2fa63..67ae26b195 100644 --- a/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php +++ b/src/OpenConext/EngineBlockBundle/Service/WayfViewModelFactory.php @@ -49,7 +49,6 @@ public function create( bool $showRequestAccess, string $requestId, ServiceProvider $serviceProvider, - bool $showRequestAccessContainer, ): WayfViewModel { return new WayfViewModel( action: $action, @@ -60,7 +59,6 @@ public function create( showIdPBanner: $showIdPBanner, rememberChoiceFeature: $rememberChoiceFeature, showRequestAccess: $showRequestAccess, - showRequestAccessContainer: $showRequestAccessContainer, requestId: $requestId, serviceProvider: $serviceProvider, connectedIdps: $this->wayfExtension->getConnectedIdps($idpList), diff --git a/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php b/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php index c57a4726ed..14afdb409c 100644 --- a/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php +++ b/src/OpenConext/EngineBlockBundle/ViewModel/WayfViewModel.php @@ -38,7 +38,6 @@ public function __construct( public bool $showIdPBanner, public bool $rememberChoiceFeature, public bool $showRequestAccess, - public bool $showRequestAccessContainer, public string $requestId, public ServiceProvider $serviceProvider, public ConnectedIdps $connectedIdps, diff --git a/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php b/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php index 6f602ed4af..abef75c547 100644 --- a/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php +++ b/tests/unit/OpenConext/EngineBlockBundle/Service/WayfRendererTest.php @@ -65,7 +65,6 @@ private function buildViewModel(bool $showIdPBanner = false, bool $showPreferred showIdPBanner: $showIdPBanner, rememberChoiceFeature: false, showRequestAccess: false, - showRequestAccessContainer: true, requestId: 'req-1', serviceProvider: $sp, connectedIdps: $emptyIdps, @@ -181,7 +180,7 @@ public function testBannerShownWhenDefaultIdpIsInPreferredConfigButDroppedBySpit $this->assertTrue($capturedShowIdPBanner, 'Banner should show: default IdP was in preferred config but dropped by splitter, so it is not visible anywhere'); } - + public function testSplitsIdpListBeforePassingToFactory(): void { $preferredId = 'https://preferred.example.org'; diff --git a/theme/openconext/templates/layouts/scripts/default.html.twig b/theme/openconext/templates/layouts/scripts/default.html.twig index 9119bc008a..6510d51438 100644 --- a/theme/openconext/templates/layouts/scripts/default.html.twig +++ b/theme/openconext/templates/layouts/scripts/default.html.twig @@ -111,9 +111,7 @@ {% endif %} -{% if showRequestAccessContainer is defined %} -
-{% endif %} +