From eb0f7924312c62bc2c65df2d1bc859ce2147b0d0 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Tue, 9 Apr 2024 03:12:02 -0300 Subject: [PATCH] Make sure attribute metadata from user storage providers are added only for the provider associated with a federated user (#150) Closes #28248 Signed-off-by: Pedro Igor Conflicts: docs/documentation/upgrading/topics/changes/changes-24_0_3.adoc --- .../topics/changes/changes-24_0_3.adoc | 17 +++++ .../kerberos/KerberosFederationProvider.java | 17 ++--- .../storage/ldap/LDAPStorageProvider.java | 60 ++++++---------- .../sssd/SSSDFederationProvider.java | 28 +++----- .../cache/infinispan/UserCacheSession.java | 6 +- .../keycloak/storage/UserStorageManager.java | 19 +++-- .../userprofile/DefaultAttributes.java | 33 ++++++++- .../keycloak/userprofile/UserProfileUtil.java | 32 ++++++--- .../userprofile/UserProfileDecorator.java | 17 +++-- .../DeclarativeUserProfileProvider.java | 17 ----- .../testsuite/util/LDAPTestUtils.java | 8 +++ .../ldap/LDAPAccountRestApiTest.java | 5 ++ .../federation/ldap/LDAPTestContext.java | 6 +- .../federation/ldap/LDAPUserProfileTest.java | 71 +++++++++++++++++-- .../base/src/test/resources/ldap/users.ldif | 5 ++ .../testsuite/sssd/SSSDUserProfileTest.java | 3 +- 16 files changed, 226 insertions(+), 118 deletions(-) diff --git a/docs/documentation/upgrading/topics/changes/changes-24_0_3.adoc b/docs/documentation/upgrading/topics/changes/changes-24_0_3.adoc index 27265c6c4ce..f61284a3d18 100644 --- a/docs/documentation/upgrading/topics/changes/changes-24_0_3.adoc +++ b/docs/documentation/upgrading/topics/changes/changes-24_0_3.adoc @@ -5,3 +5,20 @@ Because of security concerns, the redirect URI verification now performs a exact The full wildcard `*` can still be used as a valid redirect in development for http(s) URIs with those characteristics. In production environments a exact valid redirect URI without wildcard needs to be configured for any URI of that type. Please note that wildcard valid redirect URIs are not recommended for production and not covered by the OAuth 2.0 specification. + +ifeval::[{project_community}==true] += Changes to the `org.keycloak.userprofile.UserProfileDecorator` interface + +To properly support multiple user storage providers within a realm, the `org.keycloak.userprofile.UserProfileDecorator` +interface has changed. + +The `decorateUserProfile` method is no longer invoked when parsing the user profile configuration for the first time (and caching it), +but everytime a user is being managed through the user profile provider. As a result, the method changed its contract to: + +```java +List decorateUserProfile(String providerId, UserProfileMetadata metadata) +``` + +Differently than the previous contract and behavior, this method is only invoked for the user storage provider from where the user +was loaded from. +endif::[] \ No newline at end of file diff --git a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java index 137449a0794..362945f69e0 100755 --- a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java +++ b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java @@ -42,16 +42,16 @@ import org.keycloak.storage.UserStorageProvider; import org.keycloak.storage.UserStorageProviderModel; import org.keycloak.storage.user.ImportedUserValidation; import org.keycloak.storage.user.UserLookupProvider; -import org.keycloak.userprofile.AttributeContext; import org.keycloak.userprofile.AttributeGroupMetadata; import org.keycloak.userprofile.AttributeMetadata; import org.keycloak.userprofile.UserProfileDecorator; import org.keycloak.userprofile.UserProfileMetadata; import org.keycloak.userprofile.UserProfileUtil; +import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; -import java.util.function.Predicate; import java.util.stream.Stream; import javax.security.auth.login.LoginException; @@ -302,22 +302,13 @@ public class KerberosFederationProvider implements UserStorageProvider, } @Override - public void decorateUserProfile(RealmModel realm, UserProfileMetadata metadata) { - Predicate kerberosUsersSelector = (attributeContext -> { - UserModel user = attributeContext.getUser(); - if (user == null) { - return false; - } - - return model.getId().equals(user.getFederationLink()); - }); - + public List decorateUserProfile(String providerId, UserProfileMetadata metadata) { int guiOrder = (int) metadata.getAttributes().stream() .map(AttributeMetadata::getName) .distinct() .count(); AttributeGroupMetadata metadataGroup = UserProfileUtil.lookupUserMetadataGroup(session); - UserProfileUtil.addMetadataAttributeToUserProfile(KerberosConstants.KERBEROS_PRINCIPAL, metadata, metadataGroup, kerberosUsersSelector, guiOrder++, model.getName()); + return Collections.singletonList(UserProfileUtil.createAttributeMetadata(KerberosConstants.KERBEROS_PRINCIPAL, metadata, metadataGroup, guiOrder++, model.getName())); } } diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java index fbb5da27d1e..dea8cab938f 100755 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java @@ -23,7 +23,6 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -1104,47 +1103,27 @@ public class LDAPStorageProvider implements UserStorageProvider, } @Override - public void decorateUserProfile(RealmModel realm, UserProfileMetadata metadata) { - Predicate ldapUsersSelector = (attributeContext -> { - UserModel user = attributeContext.getUser(); - if (user == null) { - return false; - } - - if (model.isImportEnabled()) { - return getModel().getId().equals(user.getFederationLink()); - } else { - return getModel().getId().equals(new StorageId(user.getId()).getProviderId()); - } - }); - - Predicate onlyAdminCondition = context -> metadata.getContext().isAdminContext(); - + public List decorateUserProfile(String providerId, UserProfileMetadata metadata) { int guiOrder = (int) metadata.getAttributes().stream() .map(AttributeMetadata::getName) .distinct() .count(); - + RealmModel realm = session.getContext().getRealm(); // 1 - get configured attributes from LDAP mappers and add them to the user profile (if they not already present) - Set attributes = new LinkedHashSet<>(); - realm.getComponentsStream(model.getId(), LDAPStorageMapper.class.getName()) + List attributes = realm.getComponentsStream(model.getId(), LDAPStorageMapper.class.getName()) .sorted(ldapMappersComparator.sortAsc()) - .forEachOrdered(mapperModel -> { + .flatMap(mapperModel -> { LDAPStorageMapper ldapMapper = mapperManager.getMapper(mapperModel); - attributes.addAll(ldapMapper.getUserAttributes()); - }); + return ldapMapper.getUserAttributes().stream(); + }).toList(); + + List metadatas = new ArrayList<>(); + for (String attrName : attributes) { - // In case that attributes from LDAP mappers are explicitly defined on user profile, we can prefer defined configuration - if (!metadata.getAttribute(attrName).isEmpty()) { - logger.debugf("Ignore adding attribute '%s' to user profile by LDAP provider '%s' as attribute is already defined on user profile.", attrName, getModel().getName()); - } else { - logger.debugf("Adding attribute '%s' to user profile by LDAP provider '%s' for user profile context '%s'.", attrName, getModel().getName(), metadata.getContext().toString()); - // Writable and readable only by administrators by default. Applied only for LDAP users - AttributeMetadata attributeMetadata = metadata.addAttribute(attrName, guiOrder++, Collections.emptyList()) - .addWriteCondition(onlyAdminCondition) - .addReadCondition(onlyAdminCondition) - .setRequired(AttributeMetadata.ALWAYS_FALSE); - attributeMetadata.setSelector(ldapUsersSelector); + AttributeMetadata attributeMetadata = UserProfileUtil.createAttributeMetadata(attrName, metadata, guiOrder++, getModel().getName()); + + if (attributeMetadata != null) { + metadatas.add(attributeMetadata); } } @@ -1157,17 +1136,20 @@ public class LDAPStorageProvider implements UserStorageProvider, AttributeGroupMetadata metadataGroup = UserProfileUtil.lookupUserMetadataGroup(session); for (String attrName : metadataAttributes) { - boolean attributeAdded = UserProfileUtil.addMetadataAttributeToUserProfile(attrName, metadata, metadataGroup, ldapUsersSelector, guiOrder++, getModel().getName()); - if (!attributeAdded) { + AttributeMetadata attributeAdded = UserProfileUtil.createAttributeMetadata(attrName, metadata, metadataGroup, guiOrder++, getModel().getName()); + if (attributeAdded == null) { guiOrder--; + } else { + metadatas.add(attributeAdded); } } // 3 - make all attributes read-only for LDAP users in case that LDAP itself is read-only if (getEditMode() == EditMode.READ_ONLY) { - for (AttributeMetadata attrMetadata : metadata.getAttributes()) { - attrMetadata.addWriteCondition(ldapUsersSelector.negate()); - } + Stream.concat(metadata.getAttributes().stream(), metadatas.stream()) + .forEach(attrMetadata -> attrMetadata.addWriteCondition(AttributeMetadata.ALWAYS_FALSE)); } + + return metadatas; } } diff --git a/federation/sssd/src/main/java/org/keycloak/federation/sssd/SSSDFederationProvider.java b/federation/sssd/src/main/java/org/keycloak/federation/sssd/SSSDFederationProvider.java index e56176b96aa..cdc0bb48b2a 100755 --- a/federation/sssd/src/main/java/org/keycloak/federation/sssd/SSSDFederationProvider.java +++ b/federation/sssd/src/main/java/org/keycloak/federation/sssd/SSSDFederationProvider.java @@ -33,16 +33,15 @@ import org.keycloak.storage.UserStorageProvider; import org.keycloak.storage.UserStorageProviderModel; import org.keycloak.storage.user.ImportedUserValidation; import org.keycloak.storage.user.UserLookupProvider; -import org.keycloak.userprofile.AttributeContext; import org.keycloak.userprofile.AttributeMetadata; import org.keycloak.userprofile.UserProfileDecorator; import org.keycloak.userprofile.UserProfileMetadata; +import org.keycloak.userprofile.UserProfileUtil; -import java.util.Collections; +import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; -import java.util.function.Predicate; import java.util.stream.Stream; /** @@ -221,38 +220,29 @@ public class SSSDFederationProvider implements UserStorageProvider, } @Override - public void decorateUserProfile(RealmModel realm, UserProfileMetadata metadata) { - // selector by sssd - Predicate sssdUsersSelector = attributeContext -> - attributeContext.getUser() != null && model.getId().equals(attributeContext.getUser().getFederationLink()); - - // condition to view only by admin - Predicate onlyAdminCondition = context -> metadata.getContext().isAdminContext(); - + public List decorateUserProfile(String providerId, UserProfileMetadata metadata) { // guiOrder if new attributes are needed int guiOrder = (int) metadata.getAttributes().stream() .map(AttributeMetadata::getName) .distinct() .count(); + List metadatas = new ArrayList<>(); + // firstName, lastName, username and email should be read-only for (String attrName : List.of(UserModel.FIRST_NAME, UserModel.LAST_NAME, UserModel.EMAIL, UserModel.USERNAME)) { List attrMetadatas = metadata.getAttribute(attrName); if (attrMetadatas.isEmpty()) { logger.debugf("Adding user profile attribute '%s' for sssd provider and context '%s'.", attrName, metadata.getContext()); - AttributeMetadata sssdAttrMetadata = metadata.addAttribute(attrName, guiOrder++, Collections.emptyList()) - .addWriteCondition(AttributeMetadata.ALWAYS_FALSE) - .addReadCondition(onlyAdminCondition) - .setRequired(AttributeMetadata.ALWAYS_FALSE); - sssdAttrMetadata.setSelector(sssdUsersSelector); + metadatas.add(UserProfileUtil.createAttributeMetadata(attrName, metadata, null, guiOrder++, model.getName())); } else { for (AttributeMetadata attrMetadata : attrMetadatas) { logger.debugf("Cloning attribute '%s' as read-only for sssd provider and context '%s'.", attrName, metadata.getContext()); - AttributeMetadata sssdAttrMetadata = metadata.addAttribute(attrMetadata.clone()) - .addWriteCondition(AttributeMetadata.ALWAYS_FALSE); - sssdAttrMetadata.setSelector(sssdUsersSelector); + metadatas.add(attrMetadata.clone().addWriteCondition(AttributeMetadata.ALWAYS_FALSE)); } } } + + return metadatas; } } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java index 91df0c7de9b..4ad8ed3e438 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java @@ -63,6 +63,7 @@ import org.keycloak.storage.StorageId; import org.keycloak.storage.UserStorageProvider; import org.keycloak.storage.UserStorageProviderModel; import org.keycloak.storage.client.ClientStorageProvider; +import org.keycloak.userprofile.AttributeMetadata; import org.keycloak.userprofile.UserProfileDecorator; import org.keycloak.userprofile.UserProfileMetadata; @@ -950,9 +951,10 @@ public class UserCacheSession implements UserCache, OnCreateComponent, OnUpdateC } @Override - public void decorateUserProfile(RealmModel realm, UserProfileMetadata metadata) { + public List decorateUserProfile(String providerId, UserProfileMetadata metadata) { if (getDelegate() instanceof UserProfileDecorator) { - ((UserProfileDecorator) getDelegate()).decorateUserProfile(realm, metadata); + return ((UserProfileDecorator) getDelegate()).decorateUserProfile(providerId, metadata); } + return List.of(); } } diff --git a/model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java b/model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java index c6f21fcaa23..e3dcb4ebdc2 100755 --- a/model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java +++ b/model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java @@ -21,6 +21,8 @@ import static org.keycloak.models.utils.KeycloakModelUtils.runJobInTransaction; import static org.keycloak.utils.StreamsUtil.distinctByKey; import static org.keycloak.utils.StreamsUtil.paginatedStream; +import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -71,6 +73,7 @@ import org.keycloak.storage.user.UserLookupProvider; import org.keycloak.storage.user.UserQueryMethodsProvider; import org.keycloak.storage.user.UserQueryProvider; import org.keycloak.storage.user.UserRegistrationProvider; +import org.keycloak.userprofile.AttributeMetadata; import org.keycloak.userprofile.UserProfileDecorator; import org.keycloak.userprofile.UserProfileMetadata; @@ -857,10 +860,18 @@ public class UserStorageManager extends AbstractStorageManager decorateUserProfile(String providerId, UserProfileMetadata metadata) { + RealmModel realm = session.getContext().getRealm(); + UserStorageProviderModel providerModel = getStorageProviderModel(realm, providerId); + + if (providerModel != null) { + UserProfileDecorator decorator = getStorageProviderInstance(providerModel, UserProfileDecorator.class); + + if (decorator != null) { + return decorator.decorateUserProfile(providerId, metadata); + } } + + return Collections.emptyList(); } } diff --git a/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java b/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java index 321411e3426..f9cb740cdac 100644 --- a/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java +++ b/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java @@ -30,6 +30,7 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Consumer; +import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -39,9 +40,11 @@ import org.keycloak.models.Constants; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; +import org.keycloak.models.UserProvider; import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.representations.userprofile.config.UPConfig; import org.keycloak.representations.userprofile.config.UPConfig.UnmanagedAttributePolicy; +import org.keycloak.storage.StorageId; import org.keycloak.utils.StringUtil; import org.keycloak.validate.ValidationContext; import org.keycloak.validate.ValidationError; @@ -84,7 +87,7 @@ public class DefaultAttributes extends HashMap> implements this.context = context; this.user = user; this.session = session; - this.metadataByAttribute = configureMetadata(profileMetadata.getAttributes()); + this.metadataByAttribute = configureMetadata(profileMetadata.getAttributes(), profileMetadata); this.upConfig = session.getProvider(UserProfileProvider.class).getConfiguration(); putAll(Collections.unmodifiableMap(normalizeAttributes(attributes))); } @@ -324,7 +327,7 @@ public class DefaultAttributes extends HashMap> implements return createAttributeContext(createAttribute(metadata.getName()), metadata); } - private Map configureMetadata(List attributes) { + private Map configureMetadata(List attributes, UserProfileMetadata profileMetadata) { Map metadatas = new HashMap<>(); for (AttributeMetadata metadata : attributes) { @@ -334,9 +337,35 @@ public class DefaultAttributes extends HashMap> implements } } + metadatas.putAll(getUserStorageProviderMetadata(profileMetadata)); + return metadatas; } + private Map getUserStorageProviderMetadata(UserProfileMetadata profileMetadata) { + if (user == null || (StorageId.isLocalStorage(user.getId()) && user.getFederationLink() == null)) { + // new user or not a user from a storage provider other than local + return Collections.emptyMap(); + } + + String providerId = user.getFederationLink(); + + if (providerId == null) { + providerId = StorageId.providerId(user.getId()); + } + + UserProvider userProvider = session.users(); + + if (userProvider instanceof UserProfileDecorator) { + // query the user provider from the source user storage provider for additional attribute metadata + UserProfileDecorator decorator = (UserProfileDecorator) userProvider; + return decorator.decorateUserProfile(providerId, profileMetadata).stream() + .collect(Collectors.toMap(AttributeMetadata::getName, Function.identity())); + } + + return Collections.emptyMap(); + } + private SimpleImmutableEntry> createAttribute(String name) { return new SimpleImmutableEntry>(name, null) { @Override diff --git a/server-spi-private/src/main/java/org/keycloak/userprofile/UserProfileUtil.java b/server-spi-private/src/main/java/org/keycloak/userprofile/UserProfileUtil.java index 0b716608d04..6c28ddd784e 100644 --- a/server-spi-private/src/main/java/org/keycloak/userprofile/UserProfileUtil.java +++ b/server-spi-private/src/main/java/org/keycloak/userprofile/UserProfileUtil.java @@ -47,6 +47,8 @@ public class UserProfileUtil { public static final String USER_METADATA_GROUP = "user-metadata"; + public static final Predicate ONLY_ADMIN_CONDITION = context -> context.getContext().isAdminContext(); + /** * Find the metadata group "user-metadata" * @@ -69,30 +71,38 @@ public class UserProfileUtil { * @param attrName attribute name * @param metadata user-profile metadata where attribute would be added * @param metadataGroup metadata group in user-profile - * @param userFederationUsersSelector used to recognize if user belongs to this user-storage provider or not * @param guiOrder guiOrder to where to put the attribute * @param storageProviderName storageProviderName (just for logging purposes) - * @return true if attribute was added. False otherwise + * @return the attribute metadata if attribute was created. False otherwise */ - public static boolean addMetadataAttributeToUserProfile(String attrName, UserProfileMetadata metadata, AttributeGroupMetadata metadataGroup, Predicate userFederationUsersSelector, int guiOrder, String storageProviderName) { - // In case that attributes like LDAP_ID, KERBEROS_PRINCIPAL are explicitly defined on user profile, we can prefer defined configuration + public static AttributeMetadata createAttributeMetadata(String attrName, UserProfileMetadata metadata, AttributeGroupMetadata metadataGroup, int guiOrder, String storageProviderName) { + return createAttributeMetadata(attrName, metadata, metadataGroup, ONLY_ADMIN_CONDITION, AttributeMetadata.ALWAYS_FALSE, guiOrder, storageProviderName); + } + + public static AttributeMetadata createAttributeMetadata(String attrName, UserProfileMetadata metadata, int guiOrder, String storageProviderName) { + return createAttributeMetadata(attrName, metadata, null, ONLY_ADMIN_CONDITION, ONLY_ADMIN_CONDITION, guiOrder, storageProviderName); + } + + private static AttributeMetadata createAttributeMetadata(String attrName, UserProfileMetadata metadata, AttributeGroupMetadata metadataGroup, Predicate readCondition, Predicate writeCondition, int guiOrder, String storageProviderName) { if (!metadata.getAttribute(attrName).isEmpty()) { logger.tracef("Ignore adding metadata attribute '%s' to user profile by user storage provider '%s' as attribute is already defined on user profile.", attrName, storageProviderName); - return false; } else { logger.tracef("Adding metadata attribute '%s' to user profile by user storage provider '%s' for user profile context '%s'.", attrName, storageProviderName, metadata.getContext().toString()); - Predicate onlyAdminCondition = context -> metadata.getContext().isAdminContext(); - AttributeMetadata attributeMetadata = metadata.addAttribute(attrName, guiOrder, Collections.emptyList()) - .addWriteCondition(AttributeMetadata.ALWAYS_FALSE) // Not writable for anyone - .addReadCondition(onlyAdminCondition) // Read-only for administrators + + AttributeMetadata attributeMetadata = new AttributeMetadata(attrName, guiOrder) + .setValidators(Collections.emptyList()) + .addWriteCondition(writeCondition) + .addReadCondition(readCondition) .setRequired(AttributeMetadata.ALWAYS_FALSE); if (metadataGroup != null) { attributeMetadata.setAttributeGroupMetadata(metadataGroup); } - attributeMetadata.setSelector(userFederationUsersSelector); - return true; + + return attributeMetadata; } + + return null; } /** diff --git a/server-spi/src/main/java/org/keycloak/userprofile/UserProfileDecorator.java b/server-spi/src/main/java/org/keycloak/userprofile/UserProfileDecorator.java index acf7b5508f3..0675d769b7d 100644 --- a/server-spi/src/main/java/org/keycloak/userprofile/UserProfileDecorator.java +++ b/server-spi/src/main/java/org/keycloak/userprofile/UserProfileDecorator.java @@ -19,18 +19,27 @@ package org.keycloak.userprofile; +import java.util.List; + import org.keycloak.models.RealmModel; /** + *

This interface allows user storage providers to customize the user profile configuration and its attributes for realm + * on a per-user storage provider basis. + * * @author Pedro Igor */ public interface UserProfileDecorator { /** - * Decorates user profile with additional metadata. For instance, metadata attributes, which are available just for your user-storage - * provider can be added there, so they are available just for the users coming from your provider + *

Decorates user profile with additional metadata. For instance, metadata attributes, which are available just for your user-storage + * provider can be added there, so they are available just for the users coming from your provider. * - * @param metadata to decorate + *

This method is invoked every time a user is being managed through a user profile provider. + * + * @param providerId the id of the user storage provider to which the user is associated with + * @param metadata the current {@link UserProfileMetadata} for the current realm + * @return a list of attribute metadata.The {@link AttributeMetadata} returned from this method overrides any other metadata already set in {@code metadata} for a given attribute. */ - void decorateUserProfile(RealmModel realm, UserProfileMetadata metadata); + List decorateUserProfile(String providerId, UserProfileMetadata metadata); } diff --git a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java index 78e45124383..65503a9fb68 100644 --- a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java +++ b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java @@ -176,13 +176,9 @@ public class DeclarativeUserProfileProvider implements UserProfileProvider { protected UserProfileMetadata configureUserProfile(UserProfileMetadata metadata, KeycloakSession session) { UserProfileContext context = metadata.getContext(); UserProfileMetadata decoratedMetadata = metadata.clone(); - RealmModel realm = session.getContext().getRealm(); - ComponentModel component = getComponentModel().orElse(null); if (component == null) { - // makes sure user providers can override metadata for any attribute - decorateUserProfileMetadataWithUserStorage(realm, decoratedMetadata); return decoratedMetadata; } @@ -411,23 +407,10 @@ public class DeclarativeUserProfileProvider implements UserProfileProvider { } } - if (session != null) { - // makes sure user providers can override metadata for any attribute - decorateUserProfileMetadataWithUserStorage(session.getContext().getRealm(), decoratedMetadata); - } - return decoratedMetadata; } - private void decorateUserProfileMetadataWithUserStorage(RealmModel realm, UserProfileMetadata userProfileMetadata) { - // makes sure user providers can override metadata for any attribute - UserProvider users = session.users(); - if (users instanceof UserProfileDecorator) { - ((UserProfileDecorator) users).decorateUserProfile(realm, userProfileMetadata); - } - } - private Map asHashMap(List groups) { return groups.stream().collect(Collectors.toMap(g -> g.getName(), g -> g)); } diff --git a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java index b1adb22bcbf..e9a62b15841 100644 --- a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java +++ b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/util/LDAPTestUtils.java @@ -187,6 +187,14 @@ public class LDAPTestUtils { .orElse(null); } + public static ComponentModel getLdapProviderModel(RealmModel realm, String providerName) { + return realm.getComponentsStream(realm.getId(), UserStorageProvider.class.getName()) + .filter(component -> Objects.equals(component.getProviderId(), LDAPStorageProviderFactory.PROVIDER_NAME)) + .filter(component -> providerName == null || component.getName().equals(providerName)) + .findFirst() + .orElse(null); + } + public static LDAPStorageProvider getLdapProvider(KeycloakSession keycloakSession, ComponentModel ldapFedModel) { return (LDAPStorageProvider)keycloakSession.getProvider(UserStorageProvider.class, ldapFedModel); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPAccountRestApiTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPAccountRestApiTest.java index e66f0b40831..8f89449c2ab 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPAccountRestApiTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPAccountRestApiTest.java @@ -123,6 +123,11 @@ public class LDAPAccountRestApiTest extends AbstractLDAPTest { List origLdapEntryDn = adminRestUserRep.getAttributes().get(LDAPConstants.LDAP_ENTRY_DN); Assert.assertNotNull(origLdapId.get(0)); Assert.assertNotNull(origLdapEntryDn.get(0)); + adminRestUserRep = testRealm().users().get(adminRestUserRep.getId()).toRepresentation(); + origLdapId = adminRestUserRep.getAttributes().get(LDAPConstants.LDAP_ID); + origLdapEntryDn = adminRestUserRep.getAttributes().get(LDAPConstants.LDAP_ENTRY_DN); + Assert.assertNotNull(origLdapId.get(0)); + Assert.assertNotNull(origLdapEntryDn.get(0)); // Trying to add KERBEROS_PRINCIPAL (Adding attribute, which was not yet present). Request does not fail, but attribute is not updated user.setFirstName("JohnUpdated"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPTestContext.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPTestContext.java index e724296e2d8..a7f263fe2c6 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPTestContext.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPTestContext.java @@ -34,8 +34,12 @@ public class LDAPTestContext { private final LDAPStorageProvider ldapProvider; public static LDAPTestContext init(KeycloakSession session) { + return init(session, null); + } + + public static LDAPTestContext init(KeycloakSession session, String providerName) { RealmModel testRealm = session.realms().getRealmByName(AbstractLDAPTest.TEST_REALM_NAME); - ComponentModel ldapCompModel = LDAPTestUtils.getLdapProviderModel(testRealm); + ComponentModel ldapCompModel = LDAPTestUtils.getLdapProviderModel(testRealm, providerName); UserStorageProviderModel ldapModel = new UserStorageProviderModel(ldapCompModel); LDAPStorageProvider ldapProvider = LDAPTestUtils.getLdapProvider(session, ldapModel); return new LDAPTestContext(testRealm, ldapModel, ldapProvider); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPUserProfileTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPUserProfileTest.java index b99d2c23298..dd0ecf15d87 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPUserProfileTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPUserProfileTest.java @@ -29,6 +29,8 @@ import org.junit.FixMethodOrder; import org.junit.Test; import org.junit.runners.MethodSorters; import org.keycloak.admin.client.resource.UserResource; +import org.keycloak.component.ComponentModel; +import org.keycloak.component.PrioritizedComponentModel; import org.keycloak.models.LDAPConstants; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; @@ -38,6 +40,8 @@ import org.keycloak.representations.userprofile.config.UPAttribute; import org.keycloak.representations.userprofile.config.UPAttributePermissions; import org.keycloak.representations.userprofile.config.UPConfig; import org.keycloak.storage.UserStorageProvider; +import org.keycloak.storage.UserStorageProviderModel; +import org.keycloak.storage.ldap.LDAPStorageProvider; import org.keycloak.storage.ldap.idm.model.LDAPObject; import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.pages.LoginUpdateProfilePage; @@ -68,7 +72,7 @@ public class LDAPUserProfileTest extends AbstractLDAPTest { @Override protected void afterImportTestRealm() { testingClient.server().run(session -> { - LDAPTestContext ctx = LDAPTestContext.init(session); + LDAPTestContext ctx = LDAPTestContext.init(session, "test-ldap"); RealmModel appRealm = ctx.getRealm(); UserModel user = LDAPTestUtils.addLocalUser(session, appRealm, "marykeycloak", "mary@test.com", "Password1"); @@ -218,7 +222,20 @@ public class LDAPUserProfileTest extends AbstractLDAPTest { @Test public void testUserProfileWithoutImport() { setLDAPImportDisabled(); + UPConfig origConfig = testRealm().users().userProfile().getConfiguration(); try { + UPConfig config = testRealm().users().userProfile().getConfiguration(); + // Set postal code + UPAttribute postalCode = new UPAttribute(); + postalCode.setName("postal_code"); + postalCode.setDisplayName("Postal Code"); + + UPAttributePermissions permissions = new UPAttributePermissions(); + permissions.setView(Set.of(UPConfigUtils.ROLE_USER, UPConfigUtils.ROLE_ADMIN)); + permissions.setEdit(Set.of(UPConfigUtils.ROLE_USER, UPConfigUtils.ROLE_ADMIN)); + postalCode.setPermissions(permissions); + config.getAttributes().add(postalCode); + testRealm().users().userProfile().update(config); // Test local user is writable and has only attributes defined explicitly in user-profile // Test user profile of user johnkeycloak in admin API UserResource johnResource = ApiUtil.findUserByUsernameId(testRealm(), "johnkeycloak2"); @@ -229,12 +246,56 @@ public class LDAPUserProfileTest extends AbstractLDAPTest { assertProfileAttributes(john, USER_METADATA_GROUP, true, LDAPConstants.LDAP_ID, LDAPConstants.LDAP_ENTRY_DN); } finally { setLDAPImportEnabled(); + testRealm().users().userProfile().update(origConfig); + } + } + + @Test + public void testMultipleLDAPProviders() { + testingClient.server().run(session -> { + RealmModel testRealm = session.realms().getRealmByName(AbstractLDAPTest.TEST_REALM_NAME); + ComponentModel ldapCompModel = LDAPTestUtils.getLdapProviderModel(testRealm); + UserStorageProviderModel ldapModel = new UserStorageProviderModel(ldapCompModel); + ldapModel.setId(null); + ldapModel.setParentId(null); + ldapModel.setName("other-ldap"); + ldapModel.put(LDAPConstants.USERS_DN, ldapModel.getConfig().getFirst(LDAPConstants.USERS_DN).replace("People", "OtherPeople")); + ldapCompModel.put(PrioritizedComponentModel.PRIORITY, "100"); + testRealm.addComponentModel(ldapModel); + LDAPStorageProvider ldapProvider = LDAPTestUtils.getLdapProvider(session, ldapModel); + LDAPObject john = LDAPTestUtils.addLDAPUser(ldapProvider, testRealm, "anotherjohn", "AnotherJohn", "AnotherDoe", "anotherjohn@email.org", null, "1234"); + LDAPTestUtils.updateLDAPPassword(ldapProvider, john, "Password1"); + }); + + // the provider for this user does not have postal_code mapper + UserResource userResource = ApiUtil.findUserByUsernameId(testRealm(), "anotherjohn"); + UserRepresentation userRep = userResource.toRepresentation(true); + Assert.assertNull(userRep.getAttributes().get("postal_code")); + + // the provider for this user does have postal_code mapper + userResource = ApiUtil.findUserByUsernameId(testRealm(), "johnkeycloak"); + userRep = userResource.toRepresentation(true); + Assert.assertNotNull(userRep.getAttributes().get("postal_code")); + + setLDAPReadOnly(); + try { + // the second provider is not readonly + userResource = ApiUtil.findUserByUsernameId(testRealm(), "anotherjohn"); + userRep = userResource.toRepresentation(true); + assertProfileAttributes(userRep, null, false, "username", "email", "firstName", "lastName"); + + // the original provider is readonly + userResource = ApiUtil.findUserByUsernameId(testRealm(), "johnkeycloak"); + userRep = userResource.toRepresentation(true); + assertProfileAttributes(userRep, null, true, "username", "email", "firstName", "lastName", "postal_code"); + } finally { + setLDAPWritable(); } } private void setLDAPReadOnly() { testingClient.server().run(session -> { - LDAPTestContext ctx = LDAPTestContext.init(session); + LDAPTestContext ctx = LDAPTestContext.init(session, "test-ldap"); RealmModel appRealm = ctx.getRealm(); ctx.getLdapModel().getConfig().putSingle(LDAPConstants.EDIT_MODE, UserStorageProvider.EditMode.READ_ONLY.toString()); @@ -244,7 +305,7 @@ public class LDAPUserProfileTest extends AbstractLDAPTest { private void setLDAPWritable() { testingClient.server().run(session -> { - LDAPTestContext ctx = LDAPTestContext.init(session); + LDAPTestContext ctx = LDAPTestContext.init(session, "test-ldap"); RealmModel appRealm = ctx.getRealm(); ctx.getLdapModel().getConfig().putSingle(LDAPConstants.EDIT_MODE, UserStorageProvider.EditMode.WRITABLE.toString()); @@ -254,7 +315,7 @@ public class LDAPUserProfileTest extends AbstractLDAPTest { private void setLDAPImportDisabled() { testingClient.server().run(session -> { - LDAPTestContext ctx = LDAPTestContext.init(session); + LDAPTestContext ctx = LDAPTestContext.init(session, "test-ldap"); RealmModel appRealm = ctx.getRealm(); ctx.getLdapModel().getConfig().putSingle(IMPORT_ENABLED, "false"); @@ -264,7 +325,7 @@ public class LDAPUserProfileTest extends AbstractLDAPTest { private void setLDAPImportEnabled() { testingClient.server().run(session -> { - LDAPTestContext ctx = LDAPTestContext.init(session); + LDAPTestContext ctx = LDAPTestContext.init(session, "test-ldap"); RealmModel appRealm = ctx.getRealm(); ctx.getLdapModel().getConfig().putSingle(IMPORT_ENABLED, "true"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/resources/ldap/users.ldif b/testsuite/integration-arquillian/tests/base/src/test/resources/ldap/users.ldif index 4df8b5e61a6..388686add06 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/resources/ldap/users.ldif +++ b/testsuite/integration-arquillian/tests/base/src/test/resources/ldap/users.ldif @@ -23,3 +23,8 @@ dn: ou=Groups,dc=keycloak,dc=org objectclass: top objectclass: organizationalUnit ou: Groups + +dn: ou=OtherPeople,dc=keycloak,dc=org +objectclass: top +objectclass: organizationalUnit +ou: People diff --git a/testsuite/integration-arquillian/tests/other/sssd/src/test/java/org/keycloak/testsuite/sssd/SSSDUserProfileTest.java b/testsuite/integration-arquillian/tests/other/sssd/src/test/java/org/keycloak/testsuite/sssd/SSSDUserProfileTest.java index 9d9ea49274e..81965f78e2a 100644 --- a/testsuite/integration-arquillian/tests/other/sssd/src/test/java/org/keycloak/testsuite/sssd/SSSDUserProfileTest.java +++ b/testsuite/integration-arquillian/tests/other/sssd/src/test/java/org/keycloak/testsuite/sssd/SSSDUserProfileTest.java @@ -184,7 +184,8 @@ public class SSSDUserProfileTest extends AbstractBaseSSSDTest { String sssdId = getSssdProviderId(); UserResource userResource = ApiUtil.findUserByUsernameId(testRealm(), username); UserRepresentation user = userResource.toRepresentation(true); - assertUser(user, username, getEmail(username), getFirstName(username), getLastName(username), sssdId); + // first and last names are removed from the UP config (unmanaged) and are not available from the representation + assertUser(user, username, getEmail(username), null, null, sssdId); assertProfileAttributes(user, null, true, UserModel.USERNAME, UserModel.EMAIL, UserModel.FIRST_NAME, UserModel.LAST_NAME); assertProfileAttributes(user, null, false, "postal_code");