From 5b4003125181854213f9bccdce73fc1026d54598 Mon Sep 17 00:00:00 2001 From: mposolda Date: Fri, 5 Jun 2015 08:51:56 +0200 Subject: [PATCH] KEYCLOAK-1359 more Active Directory fixes --- .../ldap/LDAPFederationProviderFactory.java | 42 ++++++++++++++++--- .../ldap/LDAPIdentityStoreRegistry.java | 6 +-- .../idm/store/ldap/LDAPIdentityStore.java | 3 +- .../mappers/RoleLDAPFederationMapper.java | 4 ++ .../org/keycloak/models/LDAPConstants.java | 2 +- .../FederationProvidersIntegrationTest.java | 11 ++--- .../federation/LDAPRoleMappingsTest.java | 13 ++++-- .../federation/SyncProvidersTest.java | 5 +++ 8 files changed, 66 insertions(+), 20 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProviderFactory.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProviderFactory.java index c020f5139b..f0e9dafc04 100755 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProviderFactory.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProviderFactory.java @@ -94,12 +94,42 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi UserAttributeLDAPFederationMapper.READ_ONLY, readOnly); realm.addUserFederationMapper(mapperModel); - // For AD deployments with sAMAccountName is probably more common to map "cn" to full name of user - if (activeDirectory && usernameLdapAttribute.equalsIgnoreCase(LDAPConstants.SAM_ACCOUNT_NAME)) { - mapperModel = KeycloakModelUtils.createUserFederationMapperModel("full name", newProviderModel.getId(), FullNameLDAPFederationMapperFactory.PROVIDER_ID, - FullNameLDAPFederationMapper.LDAP_FULL_NAME_ATTRIBUTE, LDAPConstants.CN, - UserAttributeLDAPFederationMapper.READ_ONLY, readOnly); - realm.addUserFederationMapper(mapperModel); + // CN is typically used as RDN for Active Directory deployments + if (ldapConfig.getRdnLdapAttribute().equalsIgnoreCase(LDAPConstants.CN)) { + + if (usernameLdapAttribute.equalsIgnoreCase(LDAPConstants.CN)) { + + // For AD deployments with "cn" as username, we will map "givenName" to first name + mapperModel = KeycloakModelUtils.createUserFederationMapperModel("first name", newProviderModel.getId(), UserAttributeLDAPFederationMapperFactory.PROVIDER_ID, + UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.FIRST_NAME, + UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, LDAPConstants.GIVENNAME, + UserAttributeLDAPFederationMapper.READ_ONLY, readOnly); + realm.addUserFederationMapper(mapperModel); + + } else { + if (editMode == UserFederationProvider.EditMode.WRITABLE) { + + // For AD deployments with "sAMAccountName" as username and writable, we need to map "cn" as username as well (this is needed so we can register new users from KC into LDAP) and we will map "givenName" to first name. + mapperModel = KeycloakModelUtils.createUserFederationMapperModel("first name", newProviderModel.getId(), UserAttributeLDAPFederationMapperFactory.PROVIDER_ID, + UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.FIRST_NAME, + UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, LDAPConstants.GIVENNAME, + UserAttributeLDAPFederationMapper.READ_ONLY, readOnly); + realm.addUserFederationMapper(mapperModel); + + mapperModel = KeycloakModelUtils.createUserFederationMapperModel("username2", newProviderModel.getId(), UserAttributeLDAPFederationMapperFactory.PROVIDER_ID, + UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.USERNAME, + UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, LDAPConstants.CN, + UserAttributeLDAPFederationMapper.READ_ONLY, readOnly); + realm.addUserFederationMapper(mapperModel); + } else { + + // For read-only LDAP, we map "cn" as full name + mapperModel = KeycloakModelUtils.createUserFederationMapperModel("full name", newProviderModel.getId(), FullNameLDAPFederationMapperFactory.PROVIDER_ID, + FullNameLDAPFederationMapper.LDAP_FULL_NAME_ATTRIBUTE, LDAPConstants.CN, + UserAttributeLDAPFederationMapper.READ_ONLY, readOnly); + realm.addUserFederationMapper(mapperModel); + } + } } else { mapperModel = KeycloakModelUtils.createUserFederationMapperModel("first name", newProviderModel.getId(), UserAttributeLDAPFederationMapperFactory.PROVIDER_ID, UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.FIRST_NAME, diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPIdentityStoreRegistry.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPIdentityStoreRegistry.java index 97f347b80c..c737266a03 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPIdentityStoreRegistry.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPIdentityStoreRegistry.java @@ -24,7 +24,7 @@ public class LDAPIdentityStoreRegistry { // Ldap config might have changed for the realm. In this case, we must re-initialize Map config = model.getConfig(); if (context == null || !config.equals(context.config)) { - logLDAPConfig(model.getId(), config); + logLDAPConfig(model.getDisplayName(), config); LDAPIdentityStore store = createLdapIdentityStore(config); context = new LDAPIdentityStoreContext(config, store); @@ -34,10 +34,10 @@ public class LDAPIdentityStoreRegistry { } // Don't log LDAP password - private void logLDAPConfig(String fedProviderId, Map ldapConfig) { + private void logLDAPConfig(String fedProviderDisplayName, Map ldapConfig) { Map copy = new HashMap(ldapConfig); copy.remove(LDAPConstants.BIND_CREDENTIAL); - logger.infof("Creating new LDAP based partition manager for the Federation provider: " + fedProviderId + ", LDAP Configuration: " + copy); + logger.infof("Creating new LDAP based partition manager for the Federation provider: " + fedProviderDisplayName + ", LDAP Configuration: " + copy); } /** diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java index c2477e8144..03b23a31bb 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java @@ -75,7 +75,8 @@ public class LDAPIdentityStore implements IdentityStore { } String entryDN = ldapObject.getDn().toString(); - this.operationManager.createSubContext(entryDN, extractAttributes(ldapObject, true)); + BasicAttributes ldapAttributes = extractAttributes(ldapObject, true); + this.operationManager.createSubContext(entryDN, ldapAttributes); ldapObject.setUuid(getEntryIdentifier(ldapObject)); if (logger.isTraceEnabled()) { 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 c992f1bd50..2a78169c04 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 @@ -94,6 +94,9 @@ public class RoleLDAPFederationMapper extends AbstractLDAPFederationMapper { // Sync roles from LDAP tree and create them in local Keycloak DB (if they don't exist here yet) protected void syncRolesFromLDAP(UserFederationMapperModel mapperModel, LDAPFederationProvider ldapProvider, RealmModel realm) { if (!rolesSyncedModels.contains(mapperModel.getId())) { + // TODO: debug + logger.infof("Syncing roles from LDAP into Keycloak DB. Mapper is [%s], LDAP provider is [%s]", mapperModel.getName(), ldapProvider.getModel().getDisplayName()); + LDAPIdentityQuery ldapQuery = createRoleQuery(mapperModel, ldapProvider); // Send query @@ -105,6 +108,7 @@ public class RoleLDAPFederationMapper extends AbstractLDAPFederationMapper { String roleName = ldapRole.getAttributeAsString(rolesRdnAttr); if (roleContainer.getRole(roleName) == null) { + // TODO: debug logger.infof("Syncing role [%s] from LDAP to keycloak DB", roleName); roleContainer.addRole(roleName); } 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 f284c22bb8..22f8979021 100644 --- a/model/api/src/main/java/org/keycloak/models/LDAPConstants.java +++ b/model/api/src/main/java/org/keycloak/models/LDAPConstants.java @@ -52,7 +52,7 @@ public class LDAPConstants { public static final String CONFIG_DIVIDER = ":::"; // Those are forked from Picketlink - public static final String GIVENNAME = "givenname"; + public static final String GIVENNAME = "givenName"; public static final String CN = "cn"; public static final String SN = "sn"; public static final String SAM_ACCOUNT_NAME = "sAMAccountName"; diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java index b6a77b5185..c384651fcd 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java @@ -279,15 +279,16 @@ public class FederationProvidersIntegrationTest { LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel); FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "fullname", "James Dee", "Dee", "fullname@email.org", "4578"); - // add fullname mapper to the provider and remove "firstNameMapper" + // add fullname mapper to the provider and remove "firstNameMapper". For this test, we will simply map full name to the LDAP attribute, which was before firstName ( "givenName" on active directory, "cn" on other LDAP servers) + firstNameMapper = appRealm.getUserFederationMapperByName(ldapModel.getId(), "first name"); + String ldapFirstNameAttributeName = firstNameMapper.getConfig().get(UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE); + appRealm.removeUserFederationMapper(firstNameMapper); + UserFederationMapperModel fullNameMapperModel = KeycloakModelUtils.createUserFederationMapperModel("full name", ldapModel.getId(), FullNameLDAPFederationMapperFactory.PROVIDER_ID, - FullNameLDAPFederationMapper.LDAP_FULL_NAME_ATTRIBUTE, LDAPConstants.CN, + FullNameLDAPFederationMapper.LDAP_FULL_NAME_ATTRIBUTE, ldapFirstNameAttributeName, UserAttributeLDAPFederationMapper.READ_ONLY, "false"); appRealm.addUserFederationMapper(fullNameMapperModel); - firstNameMapper = appRealm.getUserFederationMapperByName(ldapModel.getId(), "first name"); - appRealm.removeUserFederationMapper(firstNameMapper); - // Assert user is successfully imported in Keycloak DB now with correct firstName and lastName FederationTestUtils.assertUserImported(session.users(), appRealm, "fullname", "James", "Dee", "fullname@email.org", "4578"); } finally { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/LDAPRoleMappingsTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/LDAPRoleMappingsTest.java index 3342d2ff62..f8a6944232 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/LDAPRoleMappingsTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/LDAPRoleMappingsTest.java @@ -67,14 +67,14 @@ public class LDAPRoleMappingsTest { LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel); FederationTestUtils.removeAllLDAPUsers(ldapFedProvider, appRealm); + // Add sample application + ClientModel finance = appRealm.addClient("finance"); + // Delete all LDAP roles FederationTestUtils.addOrUpdateRoleLDAPMappers(appRealm, ldapModel, RoleLDAPFederationMapper.Mode.LDAP_ONLY); FederationTestUtils.removeAllLDAPRoles(manager.getSession(), appRealm, ldapModel, "realmRolesMapper"); FederationTestUtils.removeAllLDAPRoles(manager.getSession(), appRealm, ldapModel, "financeRolesMapper"); - // Add sample application - ClientModel finance = appRealm.addClient("finance"); - // Add some users for testing LDAPObject john = FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "johnkeycloak", "John", "Doe", "john@email.org", "1234"); ldapFedProvider.getLdapIdentityStore().updatePassword(john, "Password1"); @@ -133,7 +133,12 @@ public class LDAPRoleMappingsTest { RoleModel realmRole2 = appRealm.getRole("realmRole2"); mary.grantRole(realmRole2); - RoleModel realmRole3 = appRealm.addRole("realmRole3"); + // This role may already exists from previous test (was imported from LDAP), but may not + RoleModel realmRole3 = appRealm.getRole("realmRole3"); + if (realmRole3 == null) { + realmRole3 = appRealm.addRole("realmRole3"); + } + john.grantRole(realmRole3); mary.grantRole(realmRole3); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/SyncProvidersTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/SyncProvidersTest.java index ad8eee9f70..bab44c146c 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/SyncProvidersTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/SyncProvidersTest.java @@ -75,6 +75,11 @@ public class SyncProvidersTest { .outerRule(ldapRule) .around(keycloakRule); +// @Test +// public void test01runit() throws Exception { +// Thread.sleep(10000000); +// } + @Test public void testLDAPSync() { UsersSyncManager usersSyncManager = new UsersSyncManager();