Do not automatically re-import users if they already exist locally wh…

…en searching by attributes (keycloak#32887) Closes keycloak#32870 Signed-off-by: Alexander Schwartz <[email protected]> Co-authored-by: Stefan Guilhen <[email protected]>
ssilvert · Sep 13, 2024 · e655b90 · e655b90
1 parent 073e773
commit e655b90
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 11 deletions.
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
@@ -93,7 +93,6 @@
 import org.keycloak.storage.user.UserLookupProvider;
 import org.keycloak.storage.user.UserQueryMethodsProvider;
 import org.keycloak.storage.user.UserRegistrationProvider;
-import org.keycloak.userprofile.AttributeContext;
 import org.keycloak.userprofile.AttributeGroupMetadata;
 import org.keycloak.userprofile.AttributeMetadata;
 import org.keycloak.userprofile.UserProfileDecorator;
@@ -296,7 +295,15 @@ public Stream<UserModel> searchForUserByUserAttributeStream(RealmModel realm, St
             }
         }
 
-        return ldapObjects.stream().map(ldapUser -> importUserFromLDAP(session, realm, ldapUser));
+        return ldapObjects.stream().map(ldapUser -> {
+            String ldapUsername = LDAPUtils.getUsername(ldapUser, this.ldapIdentityStore.getConfig());
+            UserModel localUser = UserStoragePrivateUtil.userLocalStorage(session).getUserByUsername(realm, ldapUsername);
+            if (localUser == null) {
+                return importUserFromLDAP(session, realm, ldapUser);
+            } else {
+                return proxy(realm, localUser, ldapUser, false);
+            }
+        });
     }
 
     public boolean synchronizeRegistrations() {
@@ -565,16 +572,16 @@ private Stream<LDAPObject> searchLDAPByAttributes(RealmModel realm, Map<String,
     }
 
     /**
-     * Searches LDAP using logical disjunction of params. It supports 
+     * Searches LDAP using logical disjunction of params. It supports
      * <ul>
      *     <li>{@link UserModel#FIRST_NAME}</li>
      *     <li>{@link UserModel#LAST_NAME}</li>
      *     <li>{@link UserModel#EMAIL}</li>
      *     <li>{@link UserModel#USERNAME}</li>
      * </ul>
-     * 
+     *
      * It uses multiple LDAP calls and results are combined together with respect to firstResult and maxResults
-     * 
+     *
      * This method serves for {@code search} param of {@link org.keycloak.services.resources.admin.UsersResource#getUsers}
      */
     private Stream<LDAPObject> searchLDAP(RealmModel realm, String search, Integer firstResult, Integer maxResults) {
@@ -1044,11 +1051,11 @@ private Predicate<LDAPObject> filterLocalUsers(RealmModel realm) {
     /**
      * This method leverages existing pagination support in {@link LDAPQuery#getResultList()}. It sets the limit for the query
      * based on {@code firstResult}, {@code maxResults} and {@link LDAPConfig#getBatchSizeForSync()}.
-     * 
+     *
      * <p/>
-     * Internally it uses {@link Stream#iterate(java.lang.Object, java.util.function.Predicate, java.util.function.UnaryOperator)} 
-     * to ensure there will be obtained required number of users considering a fact that some of the returned ldap users could be 
-     * filtered out (as they might be already imported in local storage). The returned {@code Stream<LDAPObject>} will be filled 
+     * Internally it uses {@link Stream#iterate(java.lang.Object, java.util.function.Predicate, java.util.function.UnaryOperator)}
+     * to ensure there will be obtained required number of users considering a fact that some of the returned ldap users could be
+     * filtered out (as they might be already imported in local storage). The returned {@code Stream<LDAPObject>} will be filled
      * "on demand".
      */
     private Stream<LDAPObject> paginatedSearchLDAP(LDAPQuery ldapQuery, Integer firstResult, Integer maxResults) {
@@ -1071,7 +1078,7 @@ private Stream<LDAPObject> paginatedSearchLDAP(LDAPQuery ldapQuery, Integer firs
                 }
             }
 
-            return Stream.iterate(ldapQuery, 
+            return Stream.iterate(ldapQuery,
                     query -> {
                         //the very 1st page - Pagination context might not yet be present
                         if (query.getPaginationContext() == null) try {
@@ -1082,7 +1089,7 @@ private Stream<LDAPObject> paginatedSearchLDAP(LDAPQuery ldapQuery, Integer firs
                             throw new ModelException("Querying of LDAP failed " + query, e);
                         }
                         return query.getPaginationContext().hasNextPage();
-                    }, 
+                    },
                     query -> query
             ).flatMap(query -> {
                         query.setLimit(limit);

diff --git a/...rc/test/java/org/keycloak/testsuite/federation/ldap/LDAPSearchForUsersPaginationTest.java b/...rc/test/java/org/keycloak/testsuite/federation/ldap/LDAPSearchForUsersPaginationTest.java
@@ -37,7 +37,11 @@
 import org.junit.runners.MethodSorters;
 import org.keycloak.models.LDAPConstants;
 import org.keycloak.models.RealmModel;
+import org.keycloak.models.UserModel;
+import org.keycloak.models.UserProvider;
 import org.keycloak.representations.idm.UserRepresentation;
+import org.keycloak.storage.DatastoreProvider;
+import org.keycloak.storage.datastore.DefaultDatastoreProvider;
 import org.keycloak.testsuite.util.LDAPRule;
 import org.keycloak.testsuite.util.LDAPTestUtils;
 
@@ -180,6 +184,48 @@ public void testSearchLDAPLdapEntryDn() {
         Assert.assertEquals(Set.of("john"), usernames);
     }
 
+    @Test
+    public void testSearchByUserAttributeDoesNotTriggerUserReimport() {
+
+        testingClient.server().run(session -> {
+            // add a new user for testing that searching by attributes should not cause the user to be re-imported.
+            LDAPTestContext ctx = LDAPTestContext.init(session);
+            RealmModel appRealm = ctx.getRealm();
+            LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "bwayne", "Bruce", "Wayne", "[email protected]", "Gotham Avenue", "666");
+        });
+
+        testingClient.server(TEST_REALM_NAME).run(session -> {
+            // check the user doesn't yet exist in Keycloak
+            UserProvider localProvider = ((DefaultDatastoreProvider) session.getProvider(DatastoreProvider.class)).userLocalStorage();
+            UserModel user = localProvider.getUserByUsername(session.getContext().getRealm(), "bwayne");
+            Assert.assertNull(user);
+
+            // import the user by searching for its username, and check it has the timestamp set by one of the LDAP mappers.
+            user = session.users().getUserByUsername(session.getContext().getRealm(), "bwayne");
+            Assert.assertNotNull(user);
+            Assert.assertNotNull(user.getAttributes().get("createTimestamp"));
+
+            // remove the create timestamp from the user.
+            user.removeAttribute("createTimestamp");
+            user = localProvider.getUserByUsername(session.getContext().getRealm(), "bwayne");
+            Assert.assertNull(user.getAttributes().get("createTimestamp"));
+        });
+
+        testingClient.server(TEST_REALM_NAME).run(session -> {
+            // search users by user attribute - the existing user SHOULD NOT be re-imported (GHI #32870)
+            List<UserModel> users = session.users().searchForUserByUserAttributeStream(session.getContext().getRealm(), "street", "Gotham Avenue").collect(Collectors.toList());
+            Assert.assertEquals(1, users.size());
+            UserModel user = users.get(0);
+            // create timestamp won't be null because it is provided directly from the LDAP mapper, so it should still be visible.
+            Assert.assertNotNull(user.getAttributes().get("createTimestamp"));
+
+            // however, the local stored attribute should not have been updated (i.e. user should not have been fully re-imported).
+            UserProvider localProvider = ((DefaultDatastoreProvider) session.getProvider(DatastoreProvider.class)).userLocalStorage();
+            user = localProvider.getUserByUsername(session.getContext().getRealm(), "bwayne");
+            Assert.assertNull(user.getAttributes().get("createTimestamp"));
+        });
+    }
+
     private void assertLDAPSearchMatchesLocalDB(String searchString) {
         //this call should import some users into local database
         List<String> importedUsers = adminClient.realm(TEST_REALM_NAME).users().search(searchString, null, null).stream().map(UserRepresentation::getUsername).collect(Collectors.toList());