Add support for preferred IdPs in WAYF display

[johanib]

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.

Also make the twig templates more explicit by not enabling the global variable scope.

Resolves #1970

[kayjoosten]

The banner is suppressed when the default IdP is in $preferredIdpEntityIds, but this is the raw config list it doesn't know whether the IdP is actually connected to this SP. IdpSplitter silently drops disconnected preferred IdPs, so the default IdP can be "on the guest list" but never show up to the party.

When this happens the default IdP is invisible everywhere: not in the preferred section (dropped by splitter), not in the regular list (preferred IdPs are excluded from regular), and the banner is suppressed too.

// checks the config — doesn't know if the IdP is connected
$isDefaultIdpPreferred = in_array($defaultIdpEntityId, $preferredIdpEntityIds, true);

// fix: check what the splitter actually produced
$preferredEntityIdsShown = array_column($split['preferred'], 'EntityID');
$isDefaultIdpPreferred = in_array($defaultIdpEntityId, $preferredEntityIdsShown, true);

[johanib]

🤯

This took some time to figure out.

If the default IdP (EduID) was not connected, the EduID banner would not show because the splitter correctly dropped EduID from the perferred section, but the banner logic checked the raw config list instead of what the splitter actually produced and incorrectly concluded the banner was not needed.

I verified and applied your fix because it makes sense.

But do note, this scenario could not actually occur on production, because the

            // Do not show the default IdP in the disconnected IdPs section.
            if (!$isAccessible && $isDefaultIdP) {
                continue;
            }

prevents that scenario.

What would happen in this scenario
Scenario

After the fix:

But after by commit, that fixed scenario is still not visible, because I also prevented this scenario from forming on the functional-testing route, to accurately mirror the production scenarios.

[johanib]

One reason this situation/bug could occur was that the datastructure was just arrays, providing no encapsulation of internal data.

So in line with the rest of the PR, I also improved the IdpSplitter to return a value object instead of a array.

[kayjoosten]

remainingIdps.html.twig has a visually-hidden <h2> so screenreader users know where they are. preferredIdps.html.twig has none a screenreader user landing in this section gets no context.

<section class="wayf__preferredIdps {% if connectedIdps.formattedPreviousSelectionList is not empty %}hidden{% endif %}">
    <h2 class="visually-hidden">{{ 'wayf_preferred_idps_title_screenreader'|trans }}</h2>
    <ul class="wayf__idpList wayf__idpList--preferred">

Or is it not needed here? What do you think?

[johanib]

The main header kinda says all there is to say. "Select an account to login".
So I dont think an extra title is needed for screenreaders. This would create asymmetry. If a header is needed for screen readers, it should be added visually as well.
So I don't think it adds anything in this case.

[kayjoosten]

showRequestAccessContainer is hardcoded true the parameter does nothing

WayfRenderer::render() always passes showRequestAccessContainer: true to the factory. It is carried through WayfViewModelFactory and stored in WayfViewModel, but no caller can ever change it.

If it should always be true, can you not hardcode it in the factory?

[johanib]

Ah, this was a leftover from the openconext theme. The theme actually uses an is defined check. Which is always I deleted the parameter.