From 1a36de32670469a9bce62db4f23f1245401fd1ae Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Thu, 16 Apr 2026 16:04:17 +0200 Subject: [PATCH 1/2] 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 814dc53f21..4a45d9fa55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,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 126ecd31da..4c547f9eb5 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php @@ -19,7 +19,9 @@ use Mockery as m; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; 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; @@ -51,7 +53,7 @@ public function setUp(): void ); $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($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 a8115aa3e8ee5240d247b17e2dcd318dd9faac9d Mon Sep 17 00:00:00 2001 From: Johan Kromhout Date: Tue, 21 Apr 2026 16:39:09 +0200 Subject: [PATCH 2/2] 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 4a45d9fa55..71fa5382b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,7 +32,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 4c547f9eb5..af54a6431a 100644 --- a/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php +++ b/tests/library/EngineBlock/Test/Corto/Module/BindingsTest.php @@ -19,9 +19,8 @@ use Mockery as m; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; 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; @@ -53,7 +52,7 @@ public function setUp(): void ); $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($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); + } +}