diff --git a/docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc b/docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc index edac8284c4..c62800de3c 100644 --- a/docs/documentation/upgrading/topics/changes/changes-26_0_0.adoc +++ b/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. diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIDPProvider.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIDPProvider.java index 32faf4ffff..c976ed5cba 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIDPProvider.java +++ b/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 getAllStream(Map attrs, Integer first, Integer max) { + public Stream getAllStream(Map attrs, Integer first, Integer max) { return idpDelegate.getAllStream(attrs, first, max); } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaIDPProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaIDPProvider.java index eff7b38814..75df2f7eb7 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaIDPProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaIDPProvider.java @@ -224,7 +224,7 @@ public class JpaIDPProvider implements IDPProvider { } @Override - public Stream getAllStream(Map attrs, Integer first, Integer max) { + public Stream getAllStream(Map attrs, Integer first, Integer max) { CriteriaBuilder builder = em.getCriteriaBuilder(); CriteriaQuery query = builder.createQuery(IdentityProviderEntity.class); Root 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 entry : attrs.entrySet()) { + for (Map.Entry 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)); diff --git a/server-spi/src/main/java/org/keycloak/models/IDPProvider.java b/server-spi/src/main/java/org/keycloak/models/IDPProvider.java index f6906b0311..8894356ca8 100644 --- a/server-spi/src/main/java/org/keycloak/models/IDPProvider.java +++ b/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. - * + *

* 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 getAllStream(Map attrs, Integer first, Integer max); + Stream getAllStream(Map 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 getForLogin(FETCH_MODE mode, String organizationId) { + default Stream getForLogin(FetchMode mode, String organizationId) { Stream 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 searchOptions = getBasicSearchOptionsForLogin(); + Map 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 searchOptions = getBasicSearchOptionsForLogin(); + Map 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 getBasicSearchOptionsForLogin() { - Map 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 filter; + + LoginFilter(String key, String value, java.util.function.Predicate filter) { + this.key = key; + this.value = value; + this.filter = filter; + } + + public String getKey() { + return key; + } + + public String getValue() { + return value; + } + + public Predicate getFilter() { + return filter; + } + + public static Map getLoginSearchOptions() { + return Stream.of(values()).collect(Collectors.toMap(LoginFilter::getKey, LoginFilter::getValue, (v1, v2) -> v1, LinkedHashMap::new)); + } + + public static Predicate getLoginPredicate() { + return ((Predicate) Objects::nonNull) + .and(Stream.of(values()).map(LoginFilter::getFilter).reduce(Predicate::and).get()); + } + } } diff --git a/server-spi/src/main/java/org/keycloak/models/IdentityProviderModel.java b/server-spi/src/main/java/org/keycloak/models/IdentityProviderModel.java index da6041d34f..53360fcbe4 100755 --- a/server-spi/src/main/java/org/keycloak/models/IdentityProviderModel.java +++ b/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"; diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java b/services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java index 160ff86339..0e2d15d294 100755 --- a/services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java +++ b/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) { diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java index 18077a1df4..f667ce4600 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderConfig.java +++ b/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); } @@ -375,7 +381,7 @@ public class SAMLIdentityProviderConfig extends IdentityProviderModel { public void setSignSpMetadata(boolean signSpMetadata) { getConfig().put(SIGN_SP_METADATA, String.valueOf(signSpMetadata)); } - + public boolean isAllowCreate() { return Boolean.valueOf(getConfig().get(ALLOW_CREATE)); } @@ -448,6 +454,6 @@ public class SAMLIdentityProviderConfig extends IdentityProviderModel { //transient name id format is not accepted together with principaltype SubjectnameId if (JBossSAMLURIConstants.NAMEID_FORMAT_TRANSIENT.get().equals(getNameIDPolicyFormat()) && SamlPrincipalType.SUBJECT == getPrincipalType()) throw new IllegalArgumentException("Can not have Transient NameID Policy Format together with SUBJECT Principal Type"); - + } } diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java index cd919920b7..66d7b500cb 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java +++ b/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); } } diff --git a/services/src/main/java/org/keycloak/forms/login/freemarker/model/IdentityProviderBean.java b/services/src/main/java/org/keycloak/forms/login/freemarker/model/IdentityProviderBean.java index 357d9ad188..bf5203b94b 100755 --- a/services/src/main/java/org/keycloak/forms/login/freemarker/model/IdentityProviderBean.java +++ b/services/src/main/java/org/keycloak/forms/login/freemarker/model/IdentityProviderBean.java @@ -70,7 +70,7 @@ public class IdentityProviderBean { public List getProviders() { if (this.providers == null) { String existingIDP = this.getExistingIDP(session, context); - Set federatedIdentities = this.getFederatedIdentities(session, realm, context); + Set 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. *

@@ -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 getFederatedIdentities(KeycloakSession session, RealmModel realm, AuthenticationFlowContext context) { + protected Set getLinkedBrokerAliases(KeycloakSession session, RealmModel realm, AuthenticationFlowContext context) { Set 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 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 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(); diff --git a/services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java b/services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java index f351382a33..fd20ea7376 100644 --- a/services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java +++ b/services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java @@ -52,7 +52,7 @@ public class OrganizationAwareIdentityProviderBean extends IdentityProviderBean protected List 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(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/IdentityProviderTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/IdentityProviderTest.java index 874d793f21..8eca003b4e 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/IdentityProviderTest.java +++ b/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 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 config, String expectedSigningCertificates, boolean enabled, boolean postBindingResponse) {