Merge pull request #2980 from cloudfoundry/sortIdpLinksByLinkText

feat: Return a sorted IdP list
cloudfoundry · Aug 16, 2024 · b23a496 · b23a496
2 parents 5249786 + 22de6a6
commit b23a496
Show file tree

Hide file tree

Showing 2 changed files with 13 additions and 8 deletions.
diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java b/server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java
@@ -10,6 +10,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
@@ -450,12 +451,13 @@ private void updateLoginPageModel(
             boolean fieldUsernameShow,
             boolean linkCreateAccountShow
     ) {
+        Comparator<SamlIdentityProviderDefinition> sortingByLinkText = Comparator.comparing(SamlIdentityProviderDefinition::getLinkText, String.CASE_INSENSITIVE_ORDER);
         model.addAttribute(LINK_CREATE_ACCOUNT_SHOW, linkCreateAccountShow);
         model.addAttribute(FIELD_USERNAME_SHOW, fieldUsernameShow);
-        model.addAttribute(IDP_DEFINITIONS, samlIdentityProviders.values());
+        model.addAttribute(IDP_DEFINITIONS, samlIdentityProviders.values().stream().sorted(sortingByLinkText).toList());
         Map<String, String> oauthLinks = new HashMap<>();
         ofNullable(oauthIdentityProviders).orElse(emptyMap()).entrySet().stream()
-                .filter(e -> e.getValue().isShowLinkText())
+                .filter(e -> e.getValue() != null && e.getValue().isShowLinkText() && e.getKey() != null)
                 .forEach(e ->
                         oauthLinks.put(
                                 externalOAuthProviderConfigurator.getIdpAuthenticationUrl(
@@ -465,7 +467,7 @@ private void updateLoginPageModel(
                                 e.getValue().getLinkText()
                         )
                 );
-        model.addAttribute(OAUTH_LINKS, oauthLinks);
+        model.addAttribute(OAUTH_LINKS, oauthLinks.entrySet().stream().sorted(Map.Entry.comparingByValue(String::compareToIgnoreCase)).toList());
         model.addAttribute("clientName", clientName);
     }
 

diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java
@@ -827,18 +827,20 @@ void allowedIdpsforClientOIDCProvider() throws Exception {
         when(clientDetailsService.loadClientByClientId("client-id", "uaa")).thenReturn(clientDetails);
 
         List<IdentityProvider> clientAllowedIdps = new LinkedList<>();
-        clientAllowedIdps.add(createOIDCIdentityProvider("my-OIDC-idp1"));
-        clientAllowedIdps.add(createOIDCIdentityProvider("my-OIDC-idp2"));
         clientAllowedIdps.add(createOIDCIdentityProvider("my-OIDC-idp3"));
+        clientAllowedIdps.add(createOIDCIdentityProvider("my-OIDC-idp2"));
+        clientAllowedIdps.add(createOIDCIdentityProvider("my-OIDC-idp1"));
 
         when(mockIdentityProviderProvisioning.retrieveAll(eq(true), anyString())).thenReturn(clientAllowedIdps);
 
         LoginInfoEndpoint endpoint = getEndpoint(IdentityZoneHolder.get(), clientDetailsService);
 
         endpoint.loginForHtml(extendedModelMap, null, request, singletonList(MediaType.TEXT_HTML));
 
-        Map<String, AbstractExternalOAuthIdentityProviderDefinition> idpDefinitions = (Map<String, AbstractExternalOAuthIdentityProviderDefinition>) extendedModelMap.asMap().get("oauthLinks");
+        Collection<Map<String, String>> idpDefinitions = (Collection<Map<String, String>>) extendedModelMap.asMap().get("oauthLinks");
         assertEquals(2, idpDefinitions.size());
+        // Expect this always on top of list because of sorting
+        assertEquals("my-OIDC-idp1", ((Map.Entry<String, String>) idpDefinitions.iterator().next()).getValue());
     }
 
     @Test
@@ -1243,7 +1245,7 @@ public void testInvalidLoginHintLoginPageReturnsList() throws Exception {
 
         endpoint.loginForHtml(extendedModelMap, null, mockHttpServletRequest, Collections.singletonList(MediaType.TEXT_HTML));
 
-        assertFalse(((Map)extendedModelMap.get("oauthLinks")).isEmpty());
+        assertFalse(((Collection<Map<String, String>>)extendedModelMap.get("oauthLinks")).isEmpty());
     }
 
     @Test
@@ -1701,7 +1703,7 @@ void allowedProvidersLoginHintDoesKeepExternalProviders() throws Exception {
         assertEquals("{\"origin\":\"uaa\"}", extendedModelMap.get("login_hint"));
         assertEquals("login", redirect);
 
-        Map<String, String> oauthLinks = (Map<String, String>) extendedModelMap.get("oauthLinks");
+        Collection<Map<String, String>> oauthLinks = (Collection<Map<String, String>>) extendedModelMap.get("oauthLinks");
         assertEquals(1, oauthLinks.size());
     }
 
@@ -1812,6 +1814,7 @@ private static IdentityProvider createOIDCIdentityProvider(String originKey) thr
         OIDCIdentityProviderDefinition definition = new OIDCIdentityProviderDefinition();
         definition.setAuthUrl(new URL("https://" + originKey + ".com"));
         definition.setRelyingPartySecret("client-secret");
+        definition.setLinkText(originKey);
         oidcIdentityProvider.setConfig(definition);
 
         return oidcIdentityProvider;