Do not automatically re-import users if they already exist locally when searching by attributes
Closes #32870 Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
This commit is contained in:
parent
22f9d2077e
commit
92e435f192
2 changed files with 64 additions and 11 deletions
|
@ -94,7 +94,6 @@ import org.keycloak.storage.user.ImportedUserValidation;
|
|||
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;
|
||||
|
@ -297,7 +296,15 @@ public class LDAPStorageProvider implements UserStorageProvider,
|
|||
}
|
||||
}
|
||||
|
||||
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() {
|
||||
|
@ -566,16 +573,16 @@ public class LDAPStorageProvider implements UserStorageProvider,
|
|||
}
|
||||
|
||||
/**
|
||||
* 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) {
|
||||
|
@ -1070,11 +1077,11 @@ public class LDAPStorageProvider implements UserStorageProvider,
|
|||
/**
|
||||
* 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) {
|
||||
|
@ -1097,7 +1104,7 @@ public class LDAPStorageProvider implements UserStorageProvider,
|
|||
}
|
||||
}
|
||||
|
||||
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 {
|
||||
|
@ -1108,7 +1115,7 @@ public class LDAPStorageProvider implements UserStorageProvider,
|
|||
throw new ModelException("Querying of LDAP failed " + query, e);
|
||||
}
|
||||
return query.getPaginationContext().hasNextPage();
|
||||
},
|
||||
},
|
||||
query -> query
|
||||
).flatMap(query -> {
|
||||
query.setLimit(limit);
|
||||
|
|
|
@ -38,7 +38,11 @@ import org.junit.runners.MethodSorters;
|
|||
import org.keycloak.models.LDAPConstants;
|
||||
import org.keycloak.models.KeycloakSession;
|
||||
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.admin.ApiUtil;
|
||||
import org.keycloak.testsuite.util.LDAPRule;
|
||||
import org.keycloak.testsuite.util.LDAPTestUtils;
|
||||
|
@ -205,6 +209,48 @@ public class LDAPSearchForUsersPaginationTest extends AbstractLDAPTest {
|
|||
Assert.assertTrue("Duplicated user created", adminClient.realm(TEST_REALM_NAME).users().search("john", true).isEmpty());
|
||||
}
|
||||
|
||||
@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", "bwayne@waynecorp.com", "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").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 setLDAPEnabled(final boolean enabled) {
|
||||
testingClient.server().run((KeycloakSession session) -> {
|
||||
LDAPTestContext ctx = LDAPTestContext.init(session);
|
||||
|
|
Loading…
Reference in a new issue