Skip to content

Commit

Permalink
Make LDAP searchForUsersStream consistent with other storages
Browse files Browse the repository at this point in the history
Co-authored-by: mhajas <[email protected]>

Closes keycloak#17294
  • Loading branch information
vramik authored and mhajas committed May 19, 2023
1 parent b6a4b0f commit fd6a6ec
Show file tree
Hide file tree
Showing 13 changed files with 427 additions and 124 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -195,3 +195,8 @@ Up to this version, the resolving of fallback messages was inconsistent across t
The implementation has now been unified for all themes. In general, the message for the most specific matching language tag has the highest priority. If there are both a realm localization message and a Theme 18n message, the realm localization message has the higher priority. Summarized, the priority of the messages is as follows (RL = realm localization, T = Theme i18n files): `RL <variant> > T <variant> > RL <region> > T <region> > RL <language> > T <language> > RL en > T en`.

Probably this can be better explained with an example: When the variant `de-CH-1996` is requested and there is a realm localization message for the variant, this message will be used. If such a realm localization message does not exist, the Theme i18n files are searched for a corresponding message for that variant. If such a message does not exist, a realm localization message for the region (`de-CH`) will be searched. If such a realm localization message does not exist, the Theme i18n files are searched for a message for that region. If still no message is found, a realm localization message for the language (`de`) will be searched. If there is no matching realm localization message, the Theme i18n files are be searched for a message for that language. As last fallback, the English (`en`) translation is used: First, an English realm localization will be searched - if not found, the Theme 18n files are searched for an English message.

= LDAPStorageProvider search changes

Starting with this release Keycloak uses a pagination mechanism when querying federated LDAP database.
Searching for users should be consistent with search in local database.
6 changes: 6 additions & 0 deletions federation/ldap/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@
<name>Keycloak LDAP UserStoreProvider</name>
<description />

<properties>
<maven.compiler.release>11</maven.compiler.release>
<maven.compiler.source>11</maven.compiler.source>
<maven.compiler.target>11</maven.compiler.target>
</properties>

<dependencies>
<dependency>
<groupId>org.keycloak</groupId>
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -51,20 +51,19 @@ public class LDAPQuery implements AutoCloseable {

private final LDAPStorageProvider ldapFedProvider;

private int offset;
private int limit;
private PaginationContext paginationContext;
private LDAPContextManager ldapContextManager;
private String searchDn;
private final Set<Condition> conditions = new LinkedHashSet<Condition>();
private final Set<Sort> ordering = new LinkedHashSet<Sort>();
private final Set<Condition> conditions = new LinkedHashSet<>();
private final Set<Sort> ordering = new LinkedHashSet<>();

private final Set<String> returningLdapAttributes = new LinkedHashSet<String>();
private final Set<String> returningLdapAttributes = new LinkedHashSet<>();

// Contains just those returningLdapAttributes, which are read-only. They will be marked as read-only in returned LDAPObject instances as well
// NOTE: names of attributes are lower-cased to avoid case sensitivity issues (LDAP searching is usually case-insensitive, so we want to be as well)
private final Set<String> returningReadOnlyLdapAttributes = new LinkedHashSet<String>();
private final Set<String> objectClasses = new LinkedHashSet<String>();
private final Set<String> returningReadOnlyLdapAttributes = new LinkedHashSet<>();
private final Set<String> objectClasses = new LinkedHashSet<>();

private final List<ComponentModel> mappers = new ArrayList<>();

Expand Down Expand Up @@ -146,10 +145,6 @@ public int getLimit() {
return limit;
}

public int getOffset() {
return offset;
}

public PaginationContext getPaginationContext() {
return paginationContext;
}
Expand All @@ -166,7 +161,7 @@ public List<LDAPObject> getResultList() {
fedMapper.beforeLDAPQuery(this);
}

List<LDAPObject> result = new ArrayList<LDAPObject>();
List<LDAPObject> result = new ArrayList<>();

try {
for (LDAPObject ldapObject : ldapFedProvider.getLdapIdentityStore().fetchQueryResults(this)) {
Expand Down Expand Up @@ -195,11 +190,6 @@ public int getResultCount() {
return ldapFedProvider.getLdapIdentityStore().countQueryResults(this);
}

public LDAPQuery setOffset(int offset) {
this.offset = offset;
return this;
}

public LDAPQuery setLimit(int limit) {
this.limit = limit;
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ private void createLdapContext() throws NamingException {
if (!LDAPConstants.AUTH_TYPE_NONE.equals(ldapConfig.getAuthType())) {
vaultCharSecret = getVaultSecret();

if (vaultCharSecret != null && !ldapConfig.isStartTls()) {
if (vaultCharSecret != null && !ldapConfig.isStartTls() && ldapConfig.getBindCredential() != null) {
connProp.put(SECURITY_CREDENTIALS, vaultCharSecret.getAsArray()
.orElse(ldapConfig.getBindCredential() != null? ldapConfig.getBindCredential().toCharArray() : null));
.orElse(ldapConfig.getBindCredential().toCharArray()));
}
}

Expand Down Expand Up @@ -140,7 +140,7 @@ private Hashtable<Object, Object> getConnectionProperties(LDAPConfig ldapConfig)
if(!ldapConfig.isStartTls()) {
String authType = ldapConfig.getAuthType();

env.put(Context.SECURITY_AUTHENTICATION, authType);
if (authType != null) env.put(Context.SECURITY_AUTHENTICATION, authType);

String bindDN = ldapConfig.getBindDN();

Expand All @@ -151,8 +151,8 @@ private Hashtable<Object, Object> getConnectionProperties(LDAPConfig ldapConfig)
}

if (!LDAPConstants.AUTH_TYPE_NONE.equals(authType)) {
env.put(Context.SECURITY_PRINCIPAL, bindDN);
env.put(Context.SECURITY_CREDENTIALS, bindCredential);
if (bindDN != null) env.put(Context.SECURITY_PRINCIPAL, bindDN);
if (bindCredential != null) env.put(Context.SECURITY_CREDENTIALS, bindCredential);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,15 +295,12 @@ public List<LDAPObject> fetchQueryResults(LDAPQuery identityQuery) {
@Override
public int countQueryResults(LDAPQuery identityQuery) {
int limit = identityQuery.getLimit();
int offset = identityQuery.getOffset();

identityQuery.setLimit(0);
identityQuery.setOffset(0);

int resultCount = identityQuery.getResultList().size();

identityQuery.setLimit(limit);
identityQuery.setOffset(offset);

return resultCount;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,13 @@ public void modifyAttribute(String dn, Attribute attribute) {
*/
public void modifyAttributes(String dn, NamingEnumeration<Attribute> attributes) {
try {
List<ModificationItem> modItems = new ArrayList<ModificationItem>();
List<ModificationItem> modItems = new ArrayList<>();
while (attributes.hasMore()) {
ModificationItem modItem = new ModificationItem(DirContext.REPLACE_ATTRIBUTE, attributes.next());
modItems.add(modItem);
}

modifyAttributes(dn, modItems.toArray(new ModificationItem[] {}), null);
modifyAttributes(dn, modItems.toArray(ModificationItem[]::new), null);
} catch (NamingException ne) {
throw new ModelException("Could not modify attributes on entry from DN [" + dn + "]", ne);
}
Expand Down Expand Up @@ -246,7 +246,7 @@ private String findNextDNForFallback(String newDn, int counter) {


public List<SearchResult> search(final String baseDN, final String filter, Collection<String> returningAttributes, int searchScope) throws NamingException {
final List<SearchResult> result = new ArrayList<SearchResult>();
final List<SearchResult> result = new ArrayList<>();
final SearchControls cons = getSearchControls(returningAttributes, searchScope);

try {
Expand Down Expand Up @@ -285,7 +285,7 @@ public String toString() {
}

public List<SearchResult> searchPaginated(final String baseDN, final String filter, final LDAPQuery identityQuery) throws NamingException {
final List<SearchResult> result = new ArrayList<SearchResult>();
final List<SearchResult> result = new ArrayList<>();
final SearchControls cons = getSearchControls(identityQuery.getReturningLdapAttributes(), identityQuery.getSearchScope());

// Very 1st page. Pagination context is not yet present
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@
import org.keycloak.testsuite.ProfileAssume;
import org.keycloak.testsuite.admin.ApiUtil;
import org.keycloak.testsuite.client.KeycloakTestingClient;
import org.keycloak.testsuite.util.RealmRepUtil;

import java.io.IOException;
import java.util.ArrayList;
Expand All @@ -78,6 +77,7 @@
import java.util.stream.Collectors;

import org.hamcrest.Matcher;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.keycloak.util.JsonSerialization;

Expand Down Expand Up @@ -107,7 +107,6 @@ public static void assertDataImportedInRealm(Keycloak adminClient, KeycloakTesti

RealmResource realmRsc = adminClient.realm(realm.getRealm());

/* See KEYCLOAK-3104*/
UserRepresentation user = findByUsername(realmRsc, "loginclient");
Assert.assertNotNull(user);

Expand Down Expand Up @@ -478,12 +477,10 @@ private static boolean hasClient(List<ClientRepresentation> clients, ClientRepre
return false;
}

// Workaround for KEYCLOAK-3104. For this realm, search() only works if username is null.
private static UserRepresentation findByUsername(RealmResource realmRsc, String username) {
for (UserRepresentation user : realmRsc.users().search(null, 0, -1)) {
if (user.getUsername().equalsIgnoreCase(username)) return user;
}
return null;
List<UserRepresentation> usersByUsername = realmRsc.users().search(username);
MatcherAssert.assertThat(usersByUsername, Matchers.hasSize(1));
return usersByUsername.get(0);
}

private static Set<RoleRepresentation> allScopeMappings(ClientResource client) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@
*/
public abstract class AbstractLDAPTest extends AbstractTestRealmKeycloakTest {

static final String TEST_REALM_NAME = "test";

protected static String ldapModelId;

@Rule
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright 2023 Red Hat, Inc. and/or its affiliates
* and other contributors as indicated by the @author tags.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package org.keycloak.testsuite.federation.ldap;

import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
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.hasItems;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.not;
import org.junit.ClassRule;
import org.junit.FixMethodOrder;
import org.junit.Test;
import org.junit.runners.MethodSorters;
import org.keycloak.models.RealmModel;
import org.keycloak.representations.idm.UserRepresentation;
import org.keycloak.testsuite.util.LDAPRule;
import org.keycloak.testsuite.util.LDAPTestUtils;

@FixMethodOrder(MethodSorters.NAME_ASCENDING)
public class LDAPSearchForUsersPaginationTest extends AbstractLDAPTest {

@ClassRule
public static LDAPRule ldapRule = new LDAPRule();

@Override
protected LDAPRule getLDAPRule() {
return ldapRule;
}

@Override
protected void afterImportTestRealm() {
testingClient.server().run(session -> {

LDAPTestContext ctx = LDAPTestContext.init(session);
RealmModel appRealm = ctx.getRealm();

// Delete all local users and add some new for testing
session.users().searchForUserStream(appRealm, new HashMap<>()).collect(Collectors.toList()).forEach(u -> session.users().removeUser(appRealm, u));

// Delete all LDAP users and add some new for testing
LDAPTestUtils.removeAllLDAPUsers(ctx.getLdapProvider(), appRealm);

LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john", "Some", "Some", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john00", "john", "Doe", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john01", "john", "Doe", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john02", "john", "Doe", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john03", "john", "Doe", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john04", "john", "Doe", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john05", "Some", "john", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john06", "Some", "john", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john07", "Some", "john", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john08", "Some", "john", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john09", "Some", "john", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john10", "Some", "Some", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john11", "Some", "Some", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john12", "Some", "Some", "[email protected]", null, "1234");
LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "john13", "Some", "Some", "[email protected]", null, "1234");
});
}

@Test
public void testPagination() {
//this call should import some users into local database
//collecting to TreeSet for ordering as users are orderd by username when querying from local database
@SuppressWarnings("unchecked")
LinkedList<String> importedUsers = new LinkedList(adminClient.realm(TEST_REALM_NAME).users().search("*", 0, 5).stream().map(UserRepresentation::getUsername).collect(Collectors.toCollection(TreeSet::new)));

//this call should ommit first 3 already imported users from local db
//it should return 2 local(imported) users and 8 users from ldap
List<String> search = adminClient.realm(TEST_REALM_NAME).users().search("*", 3, 10).stream().map(UserRepresentation::getUsername).collect(Collectors.toList());

assertThat(search, hasSize(10));
assertThat(search, not(contains(importedUsers.get(0))));
assertThat(search, not(contains(importedUsers.get(1))));
assertThat(search, not(contains(importedUsers.get(2))));
assertThat(search, hasItems(importedUsers.get(3), importedUsers.get(4)));
}
}
Loading

0 comments on commit fd6a6ec

Please sign in to comment.