libre.sh/keycloak-scim
2
0
Fork 0
Code Issues 14 Pull requests Releases Packages Activity Actions 6

Align isEnabled in MSAD mappers to how other properties are processed in UserAttributeLDAPStorageMapper

Browse source 
- user model is updated by onImport with the enabled/disabled status of the LDAP user
- a config option always.read.enabled.value.from.ldap was introduced, in synch to what we have in UserAttributeLDAPStorageMapper
- isEnabled checks the flag to decide if it should always retrieve the value from LDAP, or return the local value.
- setEnabled first updates the LDAP tx, and then calls the delegate to avoid issue #24201

Closes #26695
Closed #24201

Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
This commit is contained in:
Stefan Guilhen 2024-03-28 11:18:10 -03:00 committed by Pedro Igor
parent e9ad9d0564
commit 2ca59d4141
6 changed files with 175 additions and 41 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java
View file

@ -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<LD
// MSAD specific mapper for account state propagation
if (activeDirectory) {
mapperModel = KeycloakModelUtils.createComponentModel("MSAD account controls", model.getId(), MSADUserAccountControlStorageMapperFactory.PROVIDER_ID,LDAPStorageMapper.class.getName());
mapperModel = KeycloakModelUtils.createComponentModel("MSAD account controls", model.getId(), MSADUserAccountControlStorageMapperFactory.PROVIDER_ID,LDAPStorageMapper.class.getName(),
MSADUserAccountControlStorageMapper.ALWAYS_READ_ENABLED_VALUE_FROM_LDAP, alwaysReadValueFromLDAP);
realm.addComponentModel(mapperModel);
}

26
federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapper.java
View file

@ -49,6 +49,7 @@ import java.util.stream.Stream;
public class MSADUserAccountControlStorageMapper extends AbstractLDAPStorageMapper implements PasswordUpdateCallback {
public static final String LDAP_PASSWORD_POLICY_HINTS_ENABLED = "ldap.password.policy.hints.enabled";
public static final String ALWAYS_READ_ENABLED_VALUE_FROM_LDAP = "always.read.enabled.value.from.ldap";
private static final Logger logger = Logger.getLogger(MSADUserAccountControlStorageMapper.class);
@ -112,7 +113,7 @@ public class MSADUserAccountControlStorageMapper extends AbstractLDAPStorageMapp
@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
@ -122,7 +123,8 @@ public class MSADUserAccountControlStorageMapper extends AbstractLDAPStorageMapp
@Override
public void onImportUserFromLDAP(LDAPObject ldapUser, UserModel user, RealmModel realm, boolean isCreate) {
// check if user is enabled in MSAD or not.
user.setEnabled(!getUserAccountControl(ldapUser).has(UserAccountControl.ACCOUNTDISABLE));
}
@Override
@ -228,30 +230,24 @@ public class MSADUserAccountControlStorageMapper extends AbstractLDAPStorageMapp
public class MSADUserModelDelegate extends TxAwareLDAPUserModelDelegate {
private final LDAPObject ldapUser;
private final boolean isAlwaysReadEnabledFromLdap;
public MSADUserModelDelegate(UserModel delegate, LDAPObject ldapUser) {
public MSADUserModelDelegate(UserModel delegate, LDAPObject ldapUser, boolean isAlwaysReadEnabledFromLdap) {
super(delegate, ldapProvider, ldapUser);
this.ldapUser = ldapUser;
this.isAlwaysReadEnabledFromLdap = isAlwaysReadEnabledFromLdap;
}
@Override
public boolean isEnabled() {
boolean kcEnabled = super.isEnabled();
if (getPwdLastSet() > 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

20
federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapperFactory.java
View file

@ -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<ProviderConfigProperty> getConfigProps(ComponentModel parent) {
return ProviderConfigurationBuilder.create()
private static List<ProviderConfigProperty> 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

31
federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msadlds/MSADLDSUserAccountControlStorageMapper.java
View file

@ -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

24
federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msadlds/MSADLDSUserAccountControlStorageMapperFactory.java
View file

@ -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<ProviderConfigProperty> configProperties = new ArrayList<>();
protected static final List<ProviderConfigProperty> configProperties;
static {
configProperties = getConfigProps(null);
}
private static List<ProviderConfigProperty> 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<ProviderConfigProperty> getConfigProperties(RealmModel realm, ComponentModel parent) {
return getConfigProps(parent);
}
@Override
public String getId() {
return PROVIDER_ID;

111
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPMSADMapperTest.java
View file

@ -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);