From b27f18c38048ee275557fde090595f222f42acf9 Mon Sep 17 00:00:00 2001 From: mposolda Date: Tue, 2 Jun 2015 14:56:27 +0200 Subject: [PATCH 1/2] KEYCLOAK-838 per-request cache in UserFederationManager --- .../keycloak/models/UserFederationManager.java | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/model/api/src/main/java/org/keycloak/models/UserFederationManager.java b/model/api/src/main/java/org/keycloak/models/UserFederationManager.java index bd07b772bf..e6ac518461 100755 --- a/model/api/src/main/java/org/keycloak/models/UserFederationManager.java +++ b/model/api/src/main/java/org/keycloak/models/UserFederationManager.java @@ -20,6 +20,9 @@ public class UserFederationManager implements UserProvider { protected KeycloakSession session; + // Set of already validated/proxied users during this session. Key is user ID + private Map managedUsers = new HashMap<>(); + public UserFederationManager(KeycloakSession session) { this.session = session; } @@ -47,7 +50,9 @@ public class UserFederationManager implements UserProvider { UserFederationProvider fed = getFederationProvider(federation); if (fed.synchronizeRegistrations()) { user.setFederationLink(federation.getId()); - return fed.register(realm, user); + UserModel registered = fed.register(realm, user); + managedUsers.put(registered.getId(), registered); + return registered; } } return user; @@ -70,6 +75,7 @@ public class UserFederationManager implements UserProvider { boolean fedRemoved = link.removeUser(realm, user); if (fedRemoved) { boolean localRemoved = session.userStorage().removeUser(realm, user); + managedUsers.remove(user.getId()); if (!localRemoved) { logger.warn("User removed from federation provider, but failed to remove him from keycloak model"); } @@ -84,6 +90,10 @@ public class UserFederationManager implements UserProvider { } protected void validateUser(RealmModel realm, UserModel user) { + if (managedUsers.containsKey(user.getId())) { + return; + } + UserFederationProvider link = getFederationLink(realm, user); if (link != null && !link.isValid(realm, user)) { deleteInvalidUser(realm, user); @@ -109,10 +119,16 @@ public class UserFederationManager implements UserProvider { protected UserModel validateAndProxyUser(RealmModel realm, UserModel user) { + UserModel managed = managedUsers.get(user.getId()); + if (managed != null) { + return managed; + } + UserFederationProvider link = getFederationLink(realm, user); if (link != null) { UserModel validatedProxyUser = link.validateAndProxy(realm, user); if (validatedProxyUser != null) { + managedUsers.put(user.getId(), user); return validatedProxyUser; } else { deleteInvalidUser(realm, user); From 496062ef0c33afc7db1ec41bf336df07aa40c6e3 Mon Sep 17 00:00:00 2001 From: mposolda Date: Tue, 2 Jun 2015 16:58:32 +0200 Subject: [PATCH 2/2] KEYCLOAK-1359 LDAP tests update --- .../main/java/org/keycloak/federation/ldap/LDAPConfig.java | 2 -- .../federation/ldap/mappers/RoleLDAPFederationMapper.java | 2 -- .../src/main/java/org/keycloak/models/LDAPConstants.java | 1 + .../java/org/keycloak/models/UserFederationManager.java | 2 +- .../keycloak/testsuite/federation/FederationTestUtils.java | 6 ++++-- .../org/keycloak/testsuite/ldap/LDAPTestConfiguration.java | 6 ++---- .../src/test/resources/ldap/ldap-connection.properties | 2 -- 7 files changed, 8 insertions(+), 13 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPConfig.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPConfig.java index 983de98ee3..4ebde77865 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPConfig.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPConfig.java @@ -1,8 +1,6 @@ package org.keycloak.federation.ldap; -import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.HashSet; import java.util.Map; import java.util.Properties; diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/RoleLDAPFederationMapper.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/RoleLDAPFederationMapper.java index 763ca3c172..83e8d66721 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/RoleLDAPFederationMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/RoleLDAPFederationMapper.java @@ -108,7 +108,6 @@ public class RoleLDAPFederationMapper extends AbstractLDAPFederationMapper { String roleName = ldapRole.getAttributeAsString(rolesRdnAttr); if (roleContainer.getRole(roleName) == null) { - // TODO: rather change to debug logger.infof("Syncing role [%s] from LDAP to keycloak DB", roleName); roleContainer.addRole(roleName); } @@ -210,7 +209,6 @@ public class RoleLDAPFederationMapper extends AbstractLDAPFederationMapper { roleDn.addFirst(roleNameAttribute, roleName); ldapObject.setDn(roleDn); - // TODO: debug logger.infof("Creating role to [%s] to LDAP with DN [%s]", roleName, roleDn.toString()); ldapProvider.getLdapIdentityStore().add(ldapObject); return ldapObject; diff --git a/model/api/src/main/java/org/keycloak/models/LDAPConstants.java b/model/api/src/main/java/org/keycloak/models/LDAPConstants.java index c0c24a8757..3b9de4a68d 100644 --- a/model/api/src/main/java/org/keycloak/models/LDAPConstants.java +++ b/model/api/src/main/java/org/keycloak/models/LDAPConstants.java @@ -19,6 +19,7 @@ public class LDAPConstants { public static final String CONNECTION_URL = "connectionUrl"; public static final String SECURITY_PROTOCOL = "securityProtocol"; + public static final String BASE_DN = "baseDn"; // used for tests only public static final String USERS_DN = "usersDn"; public static final String BIND_DN = "bindDn"; public static final String BIND_CREDENTIAL = "bindCredential"; diff --git a/model/api/src/main/java/org/keycloak/models/UserFederationManager.java b/model/api/src/main/java/org/keycloak/models/UserFederationManager.java index e6ac518461..93045c1f19 100755 --- a/model/api/src/main/java/org/keycloak/models/UserFederationManager.java +++ b/model/api/src/main/java/org/keycloak/models/UserFederationManager.java @@ -110,7 +110,7 @@ public class UserFederationManager implements UserProvider { if (realmModel == null) return; UserModel deletedUser = tx.userStorage().getUserById(user.getId(), realmModel); tx.userStorage().removeUser(realmModel, deletedUser); - logger.debugf("Removed invalid user '%s'", user.getUsername()); + logger.infof("Removed invalid user '%s'", user.getUsername()); tx.getTransaction().commit(); } finally { tx.close(); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationTestUtils.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationTestUtils.java index 80bf539de8..0207b0a79f 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationTestUtils.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationTestUtils.java @@ -108,8 +108,9 @@ class FederationTestUtils { mapperModel.getConfig().put(RoleLDAPFederationMapper.MODE, mode.toString()); realm.updateUserFederationMapper(mapperModel); } else { + String baseDn = providerModel.getConfig().get(LDAPConstants.BASE_DN); mapperModel = KeycloakModelUtils.createUserFederationMapperModel("realmRolesMapper", providerModel.getId(), RoleLDAPFederationMapperFactory.PROVIDER_ID, - RoleLDAPFederationMapper.ROLES_DN, "ou=RealmRoles,dc=keycloak,dc=org", + RoleLDAPFederationMapper.ROLES_DN, "ou=RealmRoles," + baseDn, RoleLDAPFederationMapper.USE_REALM_ROLES_MAPPING, "true", RoleLDAPFederationMapper.MODE, mode.toString()); realm.addUserFederationMapper(mapperModel); @@ -120,8 +121,9 @@ class FederationTestUtils { mapperModel.getConfig().put(RoleLDAPFederationMapper.MODE, mode.toString()); realm.updateUserFederationMapper(mapperModel); } else { + String baseDn = providerModel.getConfig().get(LDAPConstants.BASE_DN); mapperModel = KeycloakModelUtils.createUserFederationMapperModel("financeRolesMapper", providerModel.getId(), RoleLDAPFederationMapperFactory.PROVIDER_ID, - RoleLDAPFederationMapper.ROLES_DN, "ou=FinanceRoles,dc=keycloak,dc=org", + RoleLDAPFederationMapper.ROLES_DN, "ou=FinanceRoles," + baseDn, RoleLDAPFederationMapper.USE_REALM_ROLES_MAPPING, "false", RoleLDAPFederationMapper.CLIENT_ID, "finance", RoleLDAPFederationMapper.MODE, mode.toString()); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/ldap/LDAPTestConfiguration.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/ldap/LDAPTestConfiguration.java index 8b96bc7c9a..078a09efdb 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/ldap/LDAPTestConfiguration.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/ldap/LDAPTestConfiguration.java @@ -28,8 +28,7 @@ public class LDAPTestConfiguration { static { PROP_MAPPINGS.put(LDAPConstants.CONNECTION_URL, "idm.test.ldap.connection.url"); - PROP_MAPPINGS.put("rolesDnSuffix", "idm.test.ldap.roles.dn.suffix"); - PROP_MAPPINGS.put("groupDnSuffix", "idm.test.ldap.group.dn.suffix"); + PROP_MAPPINGS.put(LDAPConstants.BASE_DN, "idm.test.ldap.base.dn"); PROP_MAPPINGS.put(LDAPConstants.USERS_DN, "idm.test.ldap.user.dn.suffix"); PROP_MAPPINGS.put(LDAPConstants.BIND_DN, "idm.test.ldap.bind.dn"); PROP_MAPPINGS.put(LDAPConstants.BIND_CREDENTIAL, "idm.test.ldap.bind.credential"); @@ -52,8 +51,7 @@ public class LDAPTestConfiguration { PROP_MAPPINGS.put(KerberosConstants.USE_KERBEROS_FOR_PASSWORD_AUTHENTICATION, "idm.test.kerberos.use.kerberos.for.password.authentication"); DEFAULT_VALUES.put(LDAPConstants.CONNECTION_URL, "ldap://localhost:10389"); - DEFAULT_VALUES.put("rolesDnSuffix", "ou=Roles,dc=keycloak,dc=org"); - DEFAULT_VALUES.put("groupDnSuffix", "ou=Groups,dc=keycloak,dc=org"); + DEFAULT_VALUES.put(LDAPConstants.BASE_DN, "dc=keycloak,dc=org"); DEFAULT_VALUES.put(LDAPConstants.USERS_DN, "ou=People,dc=keycloak,dc=org"); DEFAULT_VALUES.put(LDAPConstants.BIND_DN, "uid=admin,ou=system"); DEFAULT_VALUES.put(LDAPConstants.BIND_CREDENTIAL, "secret"); diff --git a/testsuite/integration/src/test/resources/ldap/ldap-connection.properties b/testsuite/integration/src/test/resources/ldap/ldap-connection.properties index c759f4a0ac..c279f49966 100644 --- a/testsuite/integration/src/test/resources/ldap/ldap-connection.properties +++ b/testsuite/integration/src/test/resources/ldap/ldap-connection.properties @@ -1,7 +1,5 @@ idm.test.ldap.connection.url=ldap\://localhost\:10389 idm.test.ldap.base.dn=dc\=keycloak,dc\=org -idm.test.ldap.roles.dn.suffix=ou\=Roles,dc\=keycloak,dc\=org -idm.test.ldap.group.dn.suffix=ou\=Groups,dc\=keycloak,dc\=org idm.test.ldap.user.dn.suffix=ou\=People,dc\=keycloak,dc\=org idm.test.ldap.start.embedded.ldap.server=true idm.test.ldap.bind.dn=uid\=admin,ou\=system