Address review comments

Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>

docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc

@@ -155,3 +155,5 @@ New indexes were added to the `IDENTITY_PROVIDER` table to improve the performan
 If the table currently contains more than 300.000 entries,
 {project_name} will skip the creation of the indexes by default during the automatic schema migration, and will instead log the SQL statements
 on the console during migration. In this case, the statements must be run manually in the DB after {project_name}'s startup.
+
+Also, the `kc.org` and `hideOnLoginPage` configuration attributes were migrated to the identity provider itself, to allow for more efficient queries when searching for providers. As such, API clients should use the `getOrganizationId/setOrganizationId` and `isHideOnLogin/setHideOnLogin` methods in the `IdentityProviderRepresentation`, and avoid setting these properties using the legacy config attributes that are now deprecated.

model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIDPProvider.java

@@ -143,7 +143,7 @@ public class InfinispanIDPProvider implements IDPProvider {
     }
 
     @Override
-    public Stream<IdentityProviderModel> getAllStream(Map<String, Object> attrs, Integer first, Integer max) {
+    public Stream<IdentityProviderModel> getAllStream(Map<String, String> attrs, Integer first, Integer max) {
         return idpDelegate.getAllStream(attrs, first, max);
     }
 

model/jpa/src/main/java/org/keycloak/models/jpa/JpaIDPProvider.java

@@ -224,7 +224,7 @@ public class JpaIDPProvider implements IDPProvider {
     }
 
     @Override
-    public Stream<IdentityProviderModel> getAllStream(Map<String, Object> attrs, Integer first, Integer max) {
+    public Stream<IdentityProviderModel> getAllStream(Map<String, String> attrs, Integer first, Integer max) {
         CriteriaBuilder builder = em.getCriteriaBuilder();
         CriteriaQuery<IdentityProviderEntity> query = builder.createQuery(IdentityProviderEntity.class);
         Root<IdentityProviderEntity> idp = query.from(IdentityProviderEntity.class);
@@ -233,9 +233,9 @@ public class JpaIDPProvider implements IDPProvider {
         predicates.add(builder.equal(idp.get("realmId"), getRealm().getId()));
 
         if (attrs != null) {
-            for (Map.Entry<String, Object> entry : attrs.entrySet()) {
+            for (Map.Entry<String, String> entry : attrs.entrySet()) {
                 String key = entry.getKey();
-                Object value = entry.getValue();
+                String value = entry.getValue();
                 if (StringUtil.isBlank(key)) {
                     continue;
                 }
@@ -244,7 +244,7 @@ public class JpaIDPProvider implements IDPProvider {
                     case ENABLED:
                     case HIDE_ON_LOGIN:
                     case LINK_ONLY: {
-                        if (Boolean.parseBoolean(value.toString())) {
+                        if (Boolean.parseBoolean(value)) {
                             predicates.add(builder.isTrue(idp.get(key)));
                         } else {
                             predicates.add(builder.isFalse(idp.get(key)));
@@ -253,7 +253,7 @@ public class JpaIDPProvider implements IDPProvider {
                     }
                     case FIRST_BROKER_LOGIN_FLOW_ID:
                     case ORGANIZATION_ID: {
-                        if (value == null || value.toString().isEmpty()) {
+                        if (StringUtil.isBlank(value)) {
                             predicates.add(builder.isNull(idp.get(key)));
                         } else {
                             predicates.add(builder.equal(idp.get(key), value));

server-spi/src/main/java/org/keycloak/models/IDPProvider.java

@@ -16,8 +16,12 @@
  */
 package org.keycloak.models;
 
+import java.util.Arrays;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.Objects;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import org.keycloak.provider.Provider;
@@ -25,7 +29,7 @@ import org.keycloak.provider.Provider;
 /**
  * The {@code IDPProvider} is concerned with the storage/retrieval of the configured identity providers in Keycloak. In
  * other words, it is a provider of identity providers (IDPs) and, as such, handles the CRUD operations for IDPs.
- *
+ * </p>
  * It is not to be confused with the {@code IdentityProvider} found in server-spi-private as that provider is meant to be
  * implemented by actual identity providers that handle the logic of authenticating users with third party brokers, such
  * as Microsoft, Google, Github, LinkedIn, etc.
@@ -119,7 +123,7 @@ public interface IDPProvider extends Provider {
      * @param max the maximum number of results to be returned. Ignored if negative or {@code null}.
      * @return a non-null stream of {@link IdentityProviderModel}s that match the search criteria.
      */
-    Stream<IdentityProviderModel> getAllStream(Map<String, Object> attrs, Integer first, Integer max);
+    Stream<IdentityProviderModel> getAllStream(Map<String, String> attrs, Integer first, Integer max);
 
     /**
      * Returns all identity providers associated with the organization with the provided id.
@@ -163,17 +167,17 @@ public interface IDPProvider extends Provider {
      *                       that only IDPs associated with the specified organization are to be returned.
      * @return a non-null stream of {@link IdentityProviderModel}s that are suitable for being displayed in the login pages.
      */
-    default Stream<IdentityProviderModel> getForLogin(FETCH_MODE mode, String organizationId) {
+    default Stream<IdentityProviderModel> getForLogin(FetchMode mode, String organizationId) {
         Stream<IdentityProviderModel> result = Stream.of();
-        if (mode == FETCH_MODE.REALM_ONLY || mode == FETCH_MODE.ALL) {
+        if (mode == FetchMode.REALM_ONLY || mode == FetchMode.ALL) {
             // fetch all realm-only IDPs - i.e. those not associated with orgs.
-            Map<String, Object> searchOptions = getBasicSearchOptionsForLogin();
+            Map<String, String> searchOptions = LoginFilter.getLoginSearchOptions();
             searchOptions.put(IdentityProviderModel.ORGANIZATION_ID, null);
             result = Stream.concat(result, getAllStream(searchOptions, null, null));
         }
-        if (mode == FETCH_MODE.ORG_ONLY || mode == FETCH_MODE.ALL) {
+        if (mode == FetchMode.ORG_ONLY || mode == FetchMode.ALL) {
             // fetch IDPs associated with organizations.
-            Map<String, Object> searchOptions = getBasicSearchOptionsForLogin();
+            Map<String, String> searchOptions = LoginFilter.getLoginSearchOptions();
             if (organizationId != null) {
                 // we want the IDPs associated with a specific org.
                 searchOptions.put(IdentityProviderModel.ORGANIZATION_ID, organizationId);
@@ -184,15 +188,6 @@ public interface IDPProvider extends Provider {
         return result;
     }
 
-    private static Map<String, Object> getBasicSearchOptionsForLogin() {
-        Map<String, Object> searchOptions = new LinkedHashMap<>();
-        searchOptions.put(IdentityProviderModel.ENABLED, "true");
-        searchOptions.put(IdentityProviderModel.LINK_ONLY, "false");
-        searchOptions.put(IdentityProviderModel.HIDE_ON_LOGIN, "false");
-        return searchOptions;
-    }
-
-
     /**
      * Returns the number of IDPs in the realm.
      *
@@ -210,5 +205,58 @@ public interface IDPProvider extends Provider {
         return count() > 0;
     }
 
-    enum FETCH_MODE {REALM_ONLY, ORG_ONLY, ALL}
+    /**
+     * Enum to control how login identity providers should be fetched.
+     */
+    enum FetchMode {
+        /** only realm-level providers should be fetched (not linked to any organization) **/
+        REALM_ONLY,
+        /** only providers linked to organizations should be fetched **/
+        ORG_ONLY,
+        /** all providers should fetched, regardless of being linked to an organization or not **/
+        ALL
+    }
+
+    /**
+     * Enum that contains all fields that are considered when deciding if a provider should be available for login or not.
+     */
+    enum LoginFilter {
+
+        ENABLED(IdentityProviderModel.ENABLED, Boolean.TRUE.toString(), IdentityProviderModel::isEnabled),
+
+        LINK_ONLY(IdentityProviderModel.LINK_ONLY, Boolean.FALSE.toString(), Predicate.not(IdentityProviderModel::isLinkOnly)),
+
+        HIDE_ON_LOGIN(IdentityProviderModel.HIDE_ON_LOGIN, Boolean.FALSE.toString(), Predicate.not(IdentityProviderModel::isHideOnLogin));
+
+        private final String key;
+        private final String value;
+        private final Predicate<IdentityProviderModel> filter;
+
+        LoginFilter(String key, String value, java.util.function.Predicate<IdentityProviderModel> filter) {
+            this.key = key;
+            this.value = value;
+            this.filter = filter;
+        }
+
+        public String getKey() {
+            return key;
+        }
+
+        public String getValue() {
+            return value;
+        }
+
+        public Predicate<IdentityProviderModel> getFilter() {
+            return filter;
+        }
+
+        public static Map<String, String> getLoginSearchOptions() {
+            return Stream.of(values()).collect(Collectors.toMap(LoginFilter::getKey, LoginFilter::getValue, (v1, v2) -> v1, LinkedHashMap::new));
+        }
+
+        public static Predicate<IdentityProviderModel> getLoginPredicate() {
+            return ((Predicate<IdentityProviderModel>) Objects::nonNull)
+                    .and(Stream.of(values()).map(LoginFilter::getFilter).reduce(Predicate::and).get());
+        }
+    }
 }

server-spi/src/main/java/org/keycloak/models/IdentityProviderModel.java

@@ -42,6 +42,7 @@ public class IdentityProviderModel implements Serializable {
     public static final String FILTERED_BY_CLAIMS = "filteredByClaim";
     public static final String FIRST_BROKER_LOGIN_FLOW_ID = "firstBrokerLoginFlowId";
     public static final String HIDE_ON_LOGIN = "hideOnLogin";
+    @Deprecated
     public static final String LEGACY_HIDE_ON_LOGIN_ATTR = "hideOnLoginPage";
     public static final String LINK_ONLY = "linkOnly";
     public static final String LOGIN_HINT = "loginHint";

services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java

@@ -38,7 +38,7 @@ public final class UsernameForm extends UsernamePasswordForm {
     public void authenticate(AuthenticationFlowContext context) {
         if (context.getUser() != null) {
             // We can skip the form when user is re-authenticating. Unless current user has some IDP set, so he can re-authenticate with that IDP
-            if (!this.contextUserHasFederatedIDPs(context)) {
+            if (!this.hasLinkedBrokers(context)) {
                 context.success();
                 return;
             }
@@ -79,7 +79,7 @@ public final class UsernameForm extends UsernamePasswordForm {
      * @param context a reference to the {@link AuthenticationFlowContext}
      * @return {@code true} if the context user has federated IDPs that can be used for authentication; {@code false} otherwise.
      */
-    private boolean contextUserHasFederatedIDPs(AuthenticationFlowContext context) {
+    private boolean hasLinkedBrokers(AuthenticationFlowContext context) {
         KeycloakSession session = context.getSession();
         UserModel user = context.getUser();
         if (user == null) {

services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java

@@ -71,6 +71,12 @@ public class SAMLIdentityProviderConfig extends IdentityProviderModel {
     public SAMLIdentityProviderConfig() {
     }
 
+    @Override
+    public void setHideOnLogin(boolean hideOnLogin) {
+        super.setHideOnLogin(hideOnLogin);
+        getConfig().put(LEGACY_HIDE_ON_LOGIN_ATTR, String.valueOf(hideOnLogin));
+    }
+
     public SAMLIdentityProviderConfig(IdentityProviderModel identityProviderModel) {
         super(identityProviderModel);
     }

services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java

@@ -160,7 +160,7 @@ public class SAMLIdentityProviderFactory extends AbstractIdentityProviderFactory
                     for (AttributeType attribute : entityType.getExtensions().getEntityAttributes().getAttribute()) {
                         if (MACEDIR_ENTITY_CATEGORY.equals(attribute.getName())
                                 && attribute.getAttributeValue().contains(REFEDS_HIDE_FROM_DISCOVERY)) {
-                            samlIdentityProviderConfig.getConfig().put(IdentityProviderModel.LEGACY_HIDE_ON_LOGIN_ATTR, Boolean.TRUE.toString());
+                            samlIdentityProviderConfig.setHideOnLogin(true);
                         }
                     }
 

services/src/main/java/org/keycloak/forms/login/freemarker/model/IdentityProviderBean.java

@@ -70,7 +70,7 @@ public class IdentityProviderBean {
     public List<IdentityProvider> getProviders() {
         if (this.providers == null) {
             String existingIDP = this.getExistingIDP(session, context);
-            Set<String> federatedIdentities = this.getFederatedIdentities(session, realm, context);
+            Set<String> federatedIdentities = this.getLinkedBrokerAliases(session, realm, context);
             if (federatedIdentities != null) {
                 this.providers = getFederatedIdentityProviders(federatedIdentities, existingIDP);
             } else {
@@ -167,7 +167,7 @@ public class IdentityProviderBean {
     }
 
     /**
-     * Returns the list of IDPs associated with the user's federated identities, if any. In case these IDPs exist, the login
+     * Returns the list of IDPs linked with the user's federated identities, if any. In case these IDPs exist, the login
      * page should show only the IDPs already linked to the user. Returning {@code null} indicates that all public enabled IDPs
      * should be available.
      * </p>
@@ -179,7 +179,7 @@ public class IdentityProviderBean {
      * @return a {@link Set} containing the aliases of the IDPs that should be available for login. An empty set indicates
      * that no IDPs should be available.
      */
-    protected Set<String> getFederatedIdentities(KeycloakSession session, RealmModel realm, AuthenticationFlowContext context) {
+    protected Set<String> getLinkedBrokerAliases(KeycloakSession session, RealmModel realm, AuthenticationFlowContext context) {
         Set<String> result = null;
         if (context != null) {
             UserModel currentUser = context.getUser();
@@ -223,7 +223,7 @@ public class IdentityProviderBean {
      * @return the custom {@link Predicate} used as a last filter before conversion into {@link IdentityProvider}
      */
     protected Predicate<IdentityProviderModel> federatedProviderPredicate() {
-        return idp -> Objects.nonNull(idp) && idp.isEnabled() && !idp.isLinkOnly() && !idp.isHideOnLogin();
+        return IDPProvider.LoginFilter.getLoginPredicate();
     }
 
     /**
@@ -235,7 +235,7 @@ public class IdentityProviderBean {
      * @return a {@link List} containing the constructed {@link IdentityProvider}s.
      */
     protected List<IdentityProvider> searchForIdentityProviders(String existingIDP) {
-        return session.identityProviders().getForLogin(IDPProvider.FETCH_MODE.REALM_ONLY, null)
+        return session.identityProviders().getForLogin(IDPProvider.FetchMode.REALM_ONLY, null)
                 .filter(idp -> !Objects.equals(existingIDP, idp.getAlias()))
                 .map(idp -> createIdentityProvider(this.realm, this.baseURI, idp))
                 .sorted(IDP_COMPARATOR_INSTANCE).toList();

services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java

@@ -52,7 +52,7 @@ public class OrganizationAwareIdentityProviderBean extends IdentityProviderBean
     protected List<IdentityProvider> searchForIdentityProviders(String existingIDP) {
         if (onlyRealmBrokers) {
             // we only want the realm-level IDPs - i.e. those not associated with any orgs.
-            return session.identityProviders().getForLogin(IDPProvider.FETCH_MODE.REALM_ONLY, null)
+            return session.identityProviders().getForLogin(IDPProvider.FetchMode.REALM_ONLY, null)
                     .filter(idp -> !Objects.equals(existingIDP, idp.getAlias()))
                     .map(idp -> createIdentityProvider(this.realm, this.baseURI, idp))
                     .sorted(IDP_COMPARATOR_INSTANCE).toList();
@@ -63,16 +63,17 @@ public class OrganizationAwareIdentityProviderBean extends IdentityProviderBean
                 return organization.getIdentityProviders()
                         .filter(idp -> idp.isEnabled() && !idp.isLinkOnly() && !idp.isHideOnLogin()
                                 && Boolean.parseBoolean(idp.getConfig().get(OrganizationModel.BROKER_PUBLIC)))
+                        .filter(idp -> !Objects.equals(existingIDP, idp.getAlias()))
                         .map(idp -> createIdentityProvider(super.realm, super.baseURI, idp))
-                        .toList();
+                        .sorted(IDP_COMPARATOR_INSTANCE).toList();
             }
             // we don't have a specific organization - fetch public enabled IDPs linked to any org.
-            return session.identityProviders().getForLogin(IDPProvider.FETCH_MODE.ORG_ONLY, null)
+            return session.identityProviders().getForLogin(IDPProvider.FetchMode.ORG_ONLY, null)
                     .filter(idp -> !Objects.equals(existingIDP, idp.getAlias()))
                     .map(idp -> createIdentityProvider(this.realm, this.baseURI, idp))
                     .sorted(IDP_COMPARATOR_INSTANCE).toList();
         }
-        return session.identityProviders().getForLogin(IDPProvider.FETCH_MODE.ALL, this.organization != null ? this.organization.getId() : null)
+        return session.identityProviders().getForLogin(IDPProvider.FetchMode.ALL, this.organization != null ? this.organization.getId() : null)
                 .filter(idp -> !Objects.equals(existingIDP, idp.getAlias()))
                 .map(idp -> createIdentityProvider(this.realm, this.baseURI, idp))
                 .sorted(IDP_COMPARATOR_INSTANCE).toList();

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/IdentityProviderTest.java

@@ -1068,7 +1068,7 @@ public class IdentityProviderTest extends AbstractAdminTest {
         // import endpoint simply converts IDPSSODescriptor into key value pairs.
         // check that saml-idp-metadata.xml was properly converted into key value pairs
         //System.out.println(config);
-        List<String> configKeys = new ArrayList<>(List.of(
+        assertThat(config.keySet(), containsInAnyOrder(
                 "syncMode",
                 "validateSignature",
                 "singleLogoutServiceUrl",
@@ -1083,12 +1083,9 @@ public class IdentityProviderTest extends AbstractAdminTest {
                 "signingCertificate",
                 "addExtensionsElementWithKeyInfo",
                 "loginHint",
+                "hideOnLoginPage",
                 "idpEntityId"
         ));
-        if (hasHideOnLoginPage) {
-            configKeys.add("hideOnLoginPage");
-        }
-        assertThat(config.keySet(), containsInAnyOrder(configKeys.toArray()));
         assertThat(config, hasEntry("validateSignature", "true"));
         assertThat(config, hasEntry("singleLogoutServiceUrl", "http://localhost:8080/auth/realms/master/protocol/saml"));
         assertThat(config, hasEntry("artifactResolutionServiceUrl", "http://localhost:8080/auth/realms/master/protocol/saml/resolve"));
@@ -1099,11 +1096,9 @@ public class IdentityProviderTest extends AbstractAdminTest {
         assertThat(config, hasEntry("wantAuthnRequestsSigned", "true"));
         assertThat(config, hasEntry("addExtensionsElementWithKeyInfo", "false"));
         assertThat(config, hasEntry("nameIDPolicyFormat", "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"));
+        assertThat(config, hasEntry("hideOnLoginPage", "true"));
         assertThat(config, hasEntry("idpEntityId", "http://localhost:8080/auth/realms/master"));
         assertThat(config, hasEntry(is("signingCertificate"), notNullValue()));
-        if (hasHideOnLoginPage) {
-            assertThat(config, hasEntry("hideOnLoginPage", "true"));
-        }
     }
 
     private void assertSamlImport(Map<String, String> config, String expectedSigningCertificates, boolean enabled, boolean postBindingResponse) {