From bdbbd2959d2fbe2ea2f1a76fdaea8f2984acf29c Mon Sep 17 00:00:00 2001 From: vramik Date: Tue, 16 May 2023 17:38:15 +0200 Subject: [PATCH] User search with LDAP federation not consistent Closes #10195 --- .../storage/ldap/LDAPStorageProvider.java | 14 +++++-- .../ldap/LDAPProvidersIntegrationTest.java | 2 +- .../LDAPSearchForUsersPaginationTest.java | 37 +++++++++++++++++++ ...PSearchForUsersPaginationNoImportTest.java | 24 ++++++------ 4 files changed, 61 insertions(+), 16 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 index cb194d1ed4f..e7c3db36b5d 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 @@ -497,15 +497,23 @@ public class LDAPStorageProvider implements UserStorageProvider, try (LDAPQuery ldapQuery = LDAPUtils.createQueryForUserSearch(this, realm)) { LDAPQueryConditionsBuilder conditionsBuilder = new LDAPQueryConditionsBuilder(); - List conditions = new LinkedList<>(); for (String s : search.split("\\s+")) { + List conditions = new LinkedList<>(); + if (s.startsWith("\"") && s.endsWith("\"")) { + // exact search + s = s.substring(1, s.length() - 1); + } else if (!s.endsWith("*")) { + // default to prefix search + s += "*"; + } + conditions.add(conditionsBuilder.equal(UserModel.USERNAME, s.trim().toLowerCase(), EscapeStrategy.NON_ASCII_CHARS_ONLY)); conditions.add(conditionsBuilder.equal(UserModel.EMAIL, s.trim().toLowerCase(), EscapeStrategy.NON_ASCII_CHARS_ONLY)); conditions.add(conditionsBuilder.equal(UserModel.FIRST_NAME, s, EscapeStrategy.NON_ASCII_CHARS_ONLY)); conditions.add(conditionsBuilder.equal(UserModel.LAST_NAME, s, EscapeStrategy.NON_ASCII_CHARS_ONLY)); - } - ldapQuery.addWhereCondition(conditionsBuilder.orCondition(conditions.toArray(Condition[]::new))); + ldapQuery.addWhereCondition(conditionsBuilder.orCondition(conditions.toArray(Condition[]::new))); + } return paginatedSearchLDAP(ldapQuery, firstResult, maxResults); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java index bd154b58b8f..5e7fdd4bf77 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java @@ -1005,7 +1005,7 @@ public class LDAPProvidersIntegrationTest extends AbstractLDAPTest { Assert.assertNull(UserStoragePrivateUtil.userLocalStorage(session).getUserByUsername(appRealm, "username4")); // search by username (we use a terminal operation on the stream to ensure it is consumed) - Assert.assertEquals(1, session.users().searchForUserStream(appRealm, "username1").count()); + Assert.assertEquals(1, session.users().searchForUserStream(appRealm, "\"username1\"").count()); LDAPTestAsserts.assertUserImported(UserStoragePrivateUtil.userLocalStorage(session), appRealm, "username1", "John1", "Doel1", "user1@email.org", "121"); // search by email (we use a terminal operation on the stream to ensure it is consumed) diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSearchForUsersPaginationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSearchForUsersPaginationTest.java index 4fe74aa417e..f900f903dea 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSearchForUsersPaginationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPSearchForUsersPaginationTest.java @@ -25,6 +25,7 @@ import java.util.TreeSet; import java.util.stream.Collectors; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.hasItems; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.not; @@ -96,4 +97,40 @@ public class LDAPSearchForUsersPaginationTest extends AbstractLDAPTest { assertThat(search, not(contains(importedUsers.get(2)))); assertThat(search, hasItems(importedUsers.get(3), importedUsers.get(4))); } + + @Test + public void testSearchLDAPMatchesLocalDBTwoKeywords() { + assertLDAPSearchMatchesLocalDB("Some Some"); + } + + @Test + public void testSearchLDAPMatchesLocalDBExactSearch() { + assertLDAPSearchMatchesLocalDB("\"Some\""); + } + + @Test + public void testSearchLDAPMatchesLocalDBInfixSearch() { + assertLDAPSearchMatchesLocalDB("*ohn*"); + } + + @Test + public void testSearchLDAPMatchesLocalDBPrefixSearch() { + assertLDAPSearchMatchesLocalDB("john*"); + } + + @Test + public void testSearchLDAPMatchesLocalDBDefaultPrefixSearch() { + // default search is prefix search + assertLDAPSearchMatchesLocalDB("john"); + } + + private void assertLDAPSearchMatchesLocalDB(String searchString) { + //this call should import some users into local database + List importedUsers = adminClient.realm(TEST_REALM_NAME).users().search(searchString, null, null).stream().map(UserRepresentation::getUsername).collect(Collectors.toList()); + + //this should query local db + List search = adminClient.realm(TEST_REALM_NAME).users().search(searchString, null, null).stream().map(UserRepresentation::getUsername).collect(Collectors.toList()); + + assertThat(search, containsInAnyOrder(importedUsers.toArray())); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPSearchForUsersPaginationNoImportTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPSearchForUsersPaginationNoImportTest.java index 13356a6dcf6..cd9ac12b450 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPSearchForUsersPaginationNoImportTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPSearchForUsersPaginationNoImportTest.java @@ -32,8 +32,6 @@ import org.keycloak.testsuite.federation.ldap.LDAPTestContext; import org.keycloak.testsuite.util.LDAPRule; import org.keycloak.testsuite.util.LDAPTestUtils; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.contains; -import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.not; @@ -89,8 +87,8 @@ public class LDAPSearchForUsersPaginationNoImportTest extends AbstractLDAPTest { @Test public void testPagination() { //tests LDAPStorageProvider.searchLDAP(... - assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John Some Doe", 0, 15), hasSize(15)); - assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John Some Doe", 7, 10), hasSize(8)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("jo*", 0, 15), hasSize(15)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John Some Doe", 0, 15), hasSize(0)); assertThat(adminClient.realm(TEST_REALM_NAME).users().search("*", null, null), hasSize(15)); assertThat(adminClient.realm(TEST_REALM_NAME).users().search("*", null, null), hasSize(15)); assertThat(adminClient.realm(TEST_REALM_NAME).users().search("*", 10, 8), hasSize(5)); @@ -100,14 +98,16 @@ public class LDAPSearchForUsersPaginationNoImportTest extends AbstractLDAPTest { assertThat(adminClient.realm(TEST_REALM_NAME).users().search("*", 14, 2), hasSize(1)); //tests LDAPStorageProvider.searchLDAP(... - assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John", null, null), hasSize(11)); - assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John", 10, 8), hasSize(1)); - assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John", 0, 10), hasSize(10)); - assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John", 0, 5), hasSize(5)); - assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John", 2, 10), hasSize(9)); - assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John", 0, 8), hasSize(8)); - assertThat(adminClient.realm(TEST_REALM_NAME).users().search("Some", 0, 20), hasSize(10)); - assertThat(adminClient.realm(TEST_REALM_NAME).users().search("Some", 10, 20), hasSize(0)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John", null, null), hasSize(15)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("John*", null, null), hasSize(15)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("\"John\"", null, null), hasSize(11)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("\"John\"", 10, 8), hasSize(1)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("\"John\"", 0, 10), hasSize(10)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("\"John\"", 0, 5), hasSize(5)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("\"John\"", 2, 10), hasSize(9)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("\"John\"", 0, 8), hasSize(8)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("\"Some\"", 0, 20), hasSize(10)); + assertThat(adminClient.realm(TEST_REALM_NAME).users().search("\"Some\"", 10, 20), hasSize(0)); //tests LDAPStorageProvider.searchLDAPByAttributes(... assertThat(adminClient.realm(TEST_REALM_NAME).users().list(), hasSize(15));