diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java index 6bb287d454..63c345f2fd 100755 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java @@ -60,6 +60,7 @@ import org.keycloak.storage.ldap.mappers.LDAPMappersComparator; import org.keycloak.storage.ldap.mappers.LDAPStorageMapper; import org.keycloak.storage.ldap.mappers.UserAttributeLDAPStorageMapper; import org.keycloak.storage.ldap.mappers.UserAttributeLDAPStorageMapperFactory; +import org.keycloak.storage.ldap.mappers.msad.MSADUserAccountControlStorageMapper; import org.keycloak.storage.ldap.mappers.msad.MSADUserAccountControlStorageMapperFactory; import org.keycloak.storage.user.ImportSynchronization; import org.keycloak.storage.user.SynchronizationResult; @@ -434,7 +435,8 @@ public class LDAPStorageProviderFactory implements UserStorageProviderFactory 0) { - // Merge KC and MSAD - return kcEnabled && !getUserAccountControl(ldapUser).has(UserAccountControl.ACCOUNTDISABLE); - } else { - // If new MSAD user is created and pwdLastSet is still 0, MSAD account is in disabled state. So read just from Keycloak DB. User is not able to login via MSAD anyway - return kcEnabled; + if (isAlwaysReadEnabledFromLdap) { + return !getUserAccountControl(ldapUser).has(UserAccountControl.ACCOUNTDISABLE); } + return super.isEnabled(); } @Override public void setEnabled(boolean enabled) { - // Always update DB - super.setEnabled(enabled); - if (ldapProvider.getEditMode() == UserStorageProvider.EditMode.WRITABLE && getPwdLastSet() > 0) { MSADUserAccountControlStorageMapper.logger.debugf("Going to propagate enabled=%s for ldapUser '%s' to MSAD", enabled, ldapUser.getDn().toString()); @@ -266,6 +262,8 @@ public class MSADUserAccountControlStorageMapper extends AbstractLDAPStorageMapp updateUserAccountControl(false, ldapUser, control); } + // Always update DB + super.setEnabled(enabled); } @Override diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapperFactory.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapperFactory.java index 81566f7765..a4785d8648 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapperFactory.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapperFactory.java @@ -22,12 +22,10 @@ import org.keycloak.models.LDAPConstants; import org.keycloak.models.RealmModel; import org.keycloak.provider.ProviderConfigProperty; import org.keycloak.provider.ProviderConfigurationBuilder; -import org.keycloak.storage.UserStorageProvider; -import org.keycloak.storage.ldap.LDAPConfig; +import org.keycloak.storage.UserStorageProviderModel; import org.keycloak.storage.ldap.LDAPStorageProvider; import org.keycloak.storage.ldap.mappers.AbstractLDAPStorageMapper; import org.keycloak.storage.ldap.mappers.AbstractLDAPStorageMapperFactory; -import org.keycloak.storage.ldap.mappers.FullNameLDAPStorageMapper; import java.util.ArrayList; import java.util.List; @@ -44,17 +42,25 @@ public class MSADUserAccountControlStorageMapperFactory extends AbstractLDAPStor configProperties = getConfigProps(null); } - private static List getConfigProps(ComponentModel parent) { - return ProviderConfigurationBuilder.create() + private static List getConfigProps(ComponentModel parentModel) { + UserStorageProviderModel parent = parentModel != null ? new UserStorageProviderModel(parentModel) : new UserStorageProviderModel(); + + ProviderConfigurationBuilder config = ProviderConfigurationBuilder.create() .property().name(MSADUserAccountControlStorageMapper.LDAP_PASSWORD_POLICY_HINTS_ENABLED) .label("Password Policy Hints Enabled") .helpText("Applicable just for writable MSAD. If on, then updating password of MSAD user will use LDAP_SERVER_POLICY_HINTS_OID " + "extension, which means that advanced MSAD password policies like 'password history' or 'minimal password age' will be applied. This extension works just for MSAD 2008 R2 or newer.") .type(ProviderConfigProperty.BOOLEAN_TYPE) .defaultValue("false") - .add() - .build(); + .add(); + if (parent.isImportEnabled()) { + config + .property().name(MSADUserAccountControlStorageMapper.ALWAYS_READ_ENABLED_VALUE_FROM_LDAP).label("Always Read Enabled Value From LDAP") + .helpText("If on, the user enabled/disabled state will always be read from MSAD by checking the proper userAccountControl") + .type(ProviderConfigProperty.BOOLEAN_TYPE).defaultValue("false").add(); + } + return config.build(); } @Override diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msadlds/MSADLDSUserAccountControlStorageMapper.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msadlds/MSADLDSUserAccountControlStorageMapper.java index 3bae11aacd..092297e24d 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msadlds/MSADLDSUserAccountControlStorageMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msadlds/MSADLDSUserAccountControlStorageMapper.java @@ -32,6 +32,7 @@ import org.keycloak.storage.ldap.idm.query.internal.LDAPQuery; import org.keycloak.storage.ldap.mappers.AbstractLDAPStorageMapper; import org.keycloak.storage.ldap.mappers.LDAPOperationDecorator; import org.keycloak.storage.ldap.mappers.PasswordUpdateCallback; +import org.keycloak.storage.ldap.mappers.msad.UserAccountControl; import javax.naming.AuthenticationException; import java.util.Objects; @@ -39,6 +40,8 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Stream; +import static org.keycloak.storage.ldap.mappers.msad.MSADUserAccountControlStorageMapper.ALWAYS_READ_ENABLED_VALUE_FROM_LDAP; + /** * Mapper specific to MSAD LDS. It's able to read the msDS-UserAccountDisabled, msDS-UserPasswordExpired and pwdLastSet attributes and set actions in Keycloak based on that. * It's also able to handle exception code from LDAP user authentication (See http://www-01.ibm.com/support/docview.wss?uid=swg21290631 ) @@ -105,7 +108,7 @@ public class MSADLDSUserAccountControlStorageMapper extends AbstractLDAPStorageM @Override public UserModel proxy(LDAPObject ldapUser, UserModel delegate, RealmModel realm) { - return new MSADUserModelDelegate(delegate, ldapUser); + return new MSADUserModelDelegate(delegate, ldapUser, parseBooleanParameter(mapperModel, ALWAYS_READ_ENABLED_VALUE_FROM_LDAP)); } @Override @@ -115,7 +118,8 @@ public class MSADLDSUserAccountControlStorageMapper extends AbstractLDAPStorageM @Override public void onImportUserFromLDAP(LDAPObject ldapUser, UserModel user, RealmModel realm, boolean isCreate) { - + // check if user is enabled in MSAD or not. + user.setEnabled(!Boolean.parseBoolean(ldapUser.getAttributeAsString(LDAPConstants.MSDS_USER_ACCOUNT_DISABLED))); } @Override @@ -174,32 +178,24 @@ public class MSADLDSUserAccountControlStorageMapper extends AbstractLDAPStorageM public class MSADUserModelDelegate extends UserModelDelegate { private final LDAPObject ldapUser; + private final boolean isAlwaysReadEnabledFromLdap; - public MSADUserModelDelegate(UserModel delegate, LDAPObject ldapUser) { + public MSADUserModelDelegate(UserModel delegate, LDAPObject ldapUser, boolean isAlwaysReadEnabledFromLdap) { super(delegate); this.ldapUser = ldapUser; + this.isAlwaysReadEnabledFromLdap = isAlwaysReadEnabledFromLdap; } @Override public boolean isEnabled() { - boolean kcEnabled = super.isEnabled(); - - // getPwdLastSet() == -1 when is set but not commit in AD LDS (-1 set pwdLastSet time to now) - if (getPwdLastSet() > 0 - || getPwdLastSet() == -1) { - // Merge KC and MSAD LDS - return kcEnabled && !Boolean.parseBoolean(ldapUser.getAttributeAsString(LDAPConstants.MSDS_USER_ACCOUNT_DISABLED)); - } else { - // If new MSAD LDS user is created and pwdLastSet is still 0, MSAD account is in disabled state. So read just from Keycloak DB. User is not able to login via MSAD anyway - return kcEnabled; + if (isAlwaysReadEnabledFromLdap) { + return !Boolean.parseBoolean(ldapUser.getAttributeAsString(LDAPConstants.MSDS_USER_ACCOUNT_DISABLED)); } + return super.isEnabled(); } @Override public void setEnabled(boolean enabled) { - // Always update DB - super.setEnabled(enabled); - if (ldapProvider.getEditMode() == UserStorageProvider.EditMode.WRITABLE && getPwdLastSet() > 0) { if (enabled) { logger.debugf("Removing msDS-UserAccountDisabled of user '%s'", ldapUser.getDn().toString()); @@ -209,9 +205,10 @@ public class MSADLDSUserAccountControlStorageMapper extends AbstractLDAPStorageM logger.debugf("Setting msDS-UserAccountDisabled of user '%s' to value 'TRUE'", ldapUser.getDn().toString()); ldapUser.setSingleAttribute(LDAPConstants.MSDS_USER_ACCOUNT_DISABLED, "TRUE"); } - ldapProvider.getLdapIdentityStore().update(ldapUser); } + // Always update DB + super.setEnabled(enabled); } @Override diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msadlds/MSADLDSUserAccountControlStorageMapperFactory.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msadlds/MSADLDSUserAccountControlStorageMapperFactory.java index 5f5d12e965..189c9c47f0 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msadlds/MSADLDSUserAccountControlStorageMapperFactory.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msadlds/MSADLDSUserAccountControlStorageMapperFactory.java @@ -21,9 +21,12 @@ import org.keycloak.component.ComponentModel; import org.keycloak.models.LDAPConstants; import org.keycloak.models.RealmModel; import org.keycloak.provider.ProviderConfigProperty; +import org.keycloak.provider.ProviderConfigurationBuilder; +import org.keycloak.storage.UserStorageProviderModel; import org.keycloak.storage.ldap.LDAPStorageProvider; import org.keycloak.storage.ldap.mappers.AbstractLDAPStorageMapper; import org.keycloak.storage.ldap.mappers.AbstractLDAPStorageMapperFactory; +import org.keycloak.storage.ldap.mappers.msad.MSADUserAccountControlStorageMapper; import java.util.ArrayList; import java.util.List; @@ -35,9 +38,23 @@ import java.util.List; public class MSADLDSUserAccountControlStorageMapperFactory extends AbstractLDAPStorageMapperFactory { public static final String PROVIDER_ID = LDAPConstants.MSADLDS_USER_ACCOUNT_CONTROL_MAPPER; - protected static final List configProperties = new ArrayList<>(); + protected static final List configProperties; static { + configProperties = getConfigProps(null); + } + + private static List getConfigProps(ComponentModel parentModel) { + UserStorageProviderModel parent = parentModel != null ? new UserStorageProviderModel(parentModel) : new UserStorageProviderModel(); + if (parent.isImportEnabled()) { + ProviderConfigurationBuilder config = ProviderConfigurationBuilder.create() + .property().name(MSADUserAccountControlStorageMapper.ALWAYS_READ_ENABLED_VALUE_FROM_LDAP).label("Always Read Enabled Value From LDAP") + .helpText("If on, the user enabled/disabled state will always be read from MSAD LDS by checking the msDS-UserAccountDisabled attribute") + .type(ProviderConfigProperty.BOOLEAN_TYPE).defaultValue("false").add(); + + return config.build(); + } + return new ArrayList<>(); } @Override @@ -51,6 +68,11 @@ public class MSADLDSUserAccountControlStorageMapperFactory extends AbstractLDAPS return configProperties; } + @Override + public List getConfigProperties(RealmModel realm, ComponentModel parent) { + return getConfigProps(parent); + } + @Override public String getId() { return PROVIDER_ID; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPMSADMapperTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPMSADMapperTest.java index 764c97df3d..5cb0e35f15 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPMSADMapperTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPMSADMapperTest.java @@ -28,13 +28,18 @@ import org.junit.Ignore; import org.junit.Test; import org.junit.runners.MethodSorters; import org.keycloak.admin.client.resource.UserResource; +import org.keycloak.component.ComponentModel; import org.keycloak.models.LDAPConstants; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.storage.UserStorageProvider; +import org.keycloak.storage.UserStorageProviderModel; import org.keycloak.storage.ldap.LDAPStorageProvider; import org.keycloak.storage.ldap.idm.model.LDAPObject; +import org.keycloak.storage.ldap.mappers.LDAPStorageMapper; +import org.keycloak.storage.ldap.mappers.msad.MSADUserAccountControlStorageMapper; +import org.keycloak.storage.ldap.mappers.msad.MSADUserAccountControlStorageMapperFactory; import org.keycloak.storage.ldap.mappers.msad.UserAccountControl; import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.pages.AppPage; @@ -262,7 +267,7 @@ public class LDAPMSADMapperTest extends AbstractLDAPTest { johnRep.setRequiredActions(Collections.singletonList(UserModel.RequiredAction.UPDATE_PASSWORD.name())); john.update(johnRep); - // Check in LDAP, that johnkeycloak has pwdLastSet set attribute set in MSAD to bigger value than 0. Previous update of requiredAction did not updated LDAP + // Check in LDAP, that johnkeycloak has pwdLastSet set attribute set in MSAD to bigger value than 0. Previous update of requiredAction did not update LDAP long pwdLastSetFromLDAP = getPwdLastSetOfJohn(); assertThat(pwdLastSetFromLDAP, Matchers.greaterThan(0L)); @@ -384,6 +389,15 @@ public class LDAPMSADMapperTest extends AbstractLDAPTest { ctx.getLdapModel().getConfig().putSingle(LDAPConstants.EDIT_MODE, UserStorageProvider.EditMode.UNSYNCED.toString()); appRealm.updateComponent(ctx.getLdapModel()); + + // change MSAD mapper config "ALWAYS_READ_ENABLED_VALUE_FROM_LDAP" to false, so that local db has priority. + ComponentModel msadMapperComponent = appRealm.getComponentsStream(ctx.getLdapModel().getId(), LDAPStorageMapper.class.getName()) + .filter(c -> MSADUserAccountControlStorageMapperFactory.PROVIDER_ID.equals(c.getProviderId())) + .findFirst().orElse(null); + if (msadMapperComponent != null) { + msadMapperComponent.getConfig().putSingle(MSADUserAccountControlStorageMapper.ALWAYS_READ_ENABLED_VALUE_FROM_LDAP, "false"); + appRealm.updateComponent(msadMapperComponent); + } }); // Disable user johnkeycloak through Keycloak admin API. Due UNSYNCED mode, this should update Keycloak DB, but not MSAD @@ -401,6 +415,7 @@ public class LDAPMSADMapperTest extends AbstractLDAPTest { Assert.assertEquals("Account is disabled, contact your administrator.", loginPage.getError()); // Enable johnkeycloak in admin REST API + johnRep = john.toRepresentation(); johnRep.setEnabled(true); john.update(johnRep); @@ -419,9 +434,103 @@ public class LDAPMSADMapperTest extends AbstractLDAPTest { ctx.getLdapModel().getConfig().putSingle(LDAPConstants.EDIT_MODE, UserStorageProvider.EditMode.WRITABLE.toString()); appRealm.updateComponent(ctx.getLdapModel()); + + // reset MSAD mapper config "ALWAYS_READ_ENABLED_VALUE_FROM_LDAP" to true. + ComponentModel msadMapperComponent = appRealm.getComponentsStream(ctx.getLdapModel().getId(), LDAPStorageMapper.class.getName()) + .filter(c -> MSADUserAccountControlStorageMapperFactory.PROVIDER_ID.equals(c.getProviderId())) + .findFirst().orElse(null); + if (msadMapperComponent != null) { + msadMapperComponent.getConfig().putSingle(MSADUserAccountControlStorageMapper.ALWAYS_READ_ENABLED_VALUE_FROM_LDAP, "true"); + appRealm.updateComponent(msadMapperComponent); + } + }); } + @Test + public void test09DisableUserImportDisabled() { + testingClient.server().run(session -> { + // set import enabled to false - in this case only attributes known to LDAP (via one of the mappers) are written + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + ctx.getLdapModel().getConfig().putSingle(UserStorageProviderModel.IMPORT_ENABLED, "false"); + appRealm.updateComponent(ctx.getLdapModel()); + }); + + // check user is enabled both locally and on MSAD. + UserResource john = ApiUtil.findUserByUsernameId(adminClient.realm("test"), "johnkeycloak"); + UserRepresentation johnRep = john.toRepresentation(); + Assert.assertTrue(johnRep.isEnabled()); + Assert.assertTrue(isJohnEnabledInMSAD()); + + // disable user johnkeycloak - it should disable both locally and on MSAD. + johnRep.setEnabled(false); + john.update(johnRep); + + // Login as johnkeycloak and see the user is disabled. + loginPage.open(); + loginPage.login("johnkeycloak", "Password1"); + Assert.assertEquals("Account is disabled, contact your administrator.", loginPage.getError()); + + // check user is disabled in all places. + johnRep = john.toRepresentation(); + Assert.assertFalse(johnRep.isEnabled()); + Assert.assertFalse(isJohnEnabledInMSAD()); + + // restore john to enabled state. + johnRep.setEnabled(true); + john.update(johnRep); + + // Login again. User should be enabled. + loginPage.open(); + loginPage.login("johnkeycloak", "Password1"); + Assert.assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType()); + + testingClient.server().run(session -> { + // restore import enabled mode in the storage provider. + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + ctx.getLdapModel().getConfig().putSingle(UserStorageProviderModel.IMPORT_ENABLED, "true"); + appRealm.updateComponent(ctx.getLdapModel()); + }); + } + + @Test + public void test10DisabledUserSwitchedToEnabledOnMSAD() { + // disable user johnkeycloak via REST API - should be disabled in MSAD as well. + UserResource john = ApiUtil.findUserByUsernameId(adminClient.realm("test"), "johnkeycloak"); + UserRepresentation johnRep = john.toRepresentation(); + johnRep.setEnabled(false); + john.update(johnRep); + + Assert.assertFalse(isJohnEnabledInMSAD()); + + // Login as johnkeycloak and see the user is disabled. + loginPage.open(); + loginPage.login("johnkeycloak", "Password1"); + Assert.assertEquals("Account is disabled, contact your administrator.", loginPage.getError()); + + // enable user johnkeycloak in MSAD only + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + + LDAPObject ldapJohn = ctx.getLdapProvider().loadLDAPUserByUsername(appRealm, "johnkeycloak"); + String userAccountControlStr = ldapJohn.getAttributeAsString(LDAPConstants.USER_ACCOUNT_CONTROL); + UserAccountControl control = new UserAccountControl(Long.parseLong(userAccountControlStr)); + control.remove(UserAccountControl.ACCOUNTDISABLE); + ldapJohn.setSingleAttribute(LDAPConstants.USER_ACCOUNT_CONTROL, String.valueOf(control.getValue())); + ctx.getLdapProvider().getLdapIdentityStore().update(ldapJohn); + }); + + Assert.assertTrue(isJohnEnabledInMSAD()); + + // Login again. User should be enabled. + loginPage.open(); + loginPage.login("johnkeycloak", "Password1"); + Assert.assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType()); + } + private long getPwdLastSetOfJohn() { String pwdLastSett = testingClient.server().fetchString(session -> { LDAPTestContext ctx = LDAPTestContext.init(session);