From 12d965abf37fb4f594c61b05d48374de5a3eb899 Mon Sep 17 00:00:00 2001 From: mposolda Date: Thu, 27 Feb 2020 08:51:47 +0100 Subject: [PATCH] KEYCLOAK-13047 LDAP no-import fixes. Avoid lost updates - dont allow update attributes, which are not mapped to LDAP --- .../storage/ldap/LDAPStorageProvider.java | 31 +-- .../ldap/LDAPStorageProviderFactory.java | 5 + .../ldap/LDAPWritesOnlyUserModelDelegate.java | 155 +++++++++++++++ .../ldap/ReadonlyLDAPUserModelDelegate.java | 57 ------ .../ldap/UnsyncedLDAPUserModelDelegate.java | 37 ---- .../ldap/WritableLDAPUserModelDelegate.java | 41 ---- .../idm/store/ldap/LDAPIdentityStore.java | 5 +- .../mappers/AbstractLDAPStorageMapper.java | 2 - .../mappers/FullNameLDAPStorageMapper.java | 23 ++- .../storage/ldap/mappers/LDAPTransaction.java | 42 +++- .../mappers/TxAwareLDAPUserModelDelegate.java | 12 ++ .../UserAttributeLDAPStorageMapper.java | 9 +- .../MSADUserAccountControlStorageMapper.java | 6 +- .../org/keycloak/models/LDAPConstants.java | 4 +- .../utils/ReadOnlyUserModelDelegate.java | 54 +++-- .../storage/adapter/InMemoryUserAdapter.java | 4 +- .../UpdateOnlyChangeUserModelDelegate.java | 185 ++++++++++++++++++ .../admin/ClientRoleMappingsResource.java | 28 ++- .../resources/admin/RoleMapperResource.java | 28 ++- .../resources/admin/UserResource.java | 4 +- .../federation/ldap/LDAPGroupMapperTest.java | 114 ++++++++--- .../LDAPProvidersIntegrationNoImportTest.java | 91 ++++++++- .../testsuite/oauth/ServiceAccountTest.java | 2 +- .../admin/messages/messages_en.properties | 1 + 24 files changed, 697 insertions(+), 243 deletions(-) create mode 100644 federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPWritesOnlyUserModelDelegate.java delete mode 100755 federation/ldap/src/main/java/org/keycloak/storage/ldap/ReadonlyLDAPUserModelDelegate.java delete mode 100755 federation/ldap/src/main/java/org/keycloak/storage/ldap/UnsyncedLDAPUserModelDelegate.java delete mode 100755 federation/ldap/src/main/java/org/keycloak/storage/ldap/WritableLDAPUserModelDelegate.java create mode 100644 server-spi-private/src/main/java/org/keycloak/storage/adapter/UpdateOnlyChangeUserModelDelegate.java diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java index 8eac6d6c2b..172480436a 100755 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java @@ -50,6 +50,7 @@ import org.keycloak.storage.StorageId; import org.keycloak.storage.UserStorageProvider; import org.keycloak.storage.UserStorageProviderModel; import org.keycloak.storage.adapter.InMemoryUserAdapter; +import org.keycloak.storage.adapter.UpdateOnlyChangeUserModelDelegate; import org.keycloak.storage.ldap.idm.model.LDAPObject; import org.keycloak.storage.ldap.idm.query.Condition; import org.keycloak.storage.ldap.idm.query.EscapeStrategy; @@ -149,10 +150,10 @@ public class LDAPStorageProvider implements UserStorageProvider, return null; } - return proxy(realm, local, ldapObject); + return proxy(realm, local, ldapObject, false); } - protected UserModel proxy(RealmModel realm, UserModel local, LDAPObject ldapObject) { + protected UserModel proxy(RealmModel realm, UserModel local, LDAPObject ldapObject, boolean newUser) { UserModel existing = userManager.getManagedProxiedUser(local.getId()); if (existing != null) { return existing; @@ -174,17 +175,17 @@ public class LDAPStorageProvider implements UserStorageProvider, switch (editMode) { case READ_ONLY: - if (model.isImportEnabled()) { - proxied = new ReadonlyLDAPUserModelDelegate(local, this); - } else { proxied = new ReadOnlyUserModelDelegate(local); - } break; case WRITABLE: - proxied = new WritableLDAPUserModelDelegate(local, this, ldapObject); - break; case UNSYNCED: - proxied = new UnsyncedLDAPUserModelDelegate(local, this); + // Any attempt to write data, which are not supported by the LDAP schema, should fail + // This check is skipped when register new user as there are many "generic" attributes always written (EG. enabled, emailVerified) and those are usually unsupported by LDAP schema + if (!model.isImportEnabled() && !newUser) { + UserModel readOnlyDelegate = new ReadOnlyUserModelDelegate(local, ModelException::new); + proxied = new LDAPWritesOnlyUserModelDelegate(readOnlyDelegate, this); + } + break; } List mappers = realm.getComponents(model.getId(), LDAPStorageMapper.class.getName()); @@ -194,6 +195,10 @@ public class LDAPStorageProvider implements UserStorageProvider, proxied = ldapMapper.proxy(ldapObject, proxied, realm); } + if (!model.isImportEnabled()) { + proxied = new UpdateOnlyChangeUserModelDelegate(proxied); + } + userManager.setManagedProxiedUser(proxied, ldapObject); return proxied; @@ -272,7 +277,7 @@ public class LDAPStorageProvider implements UserStorageProvider, user.setSingleAttribute(LDAPConstants.LDAP_ENTRY_DN, ldapUser.getDn().toString()); // Add the user to the default groups and add default required actions - UserModel proxy = proxy(realm, user, ldapUser); + UserModel proxy = proxy(realm, user, ldapUser, true); DefaultRoles.addDefaultRoles(realm, proxy); for (GroupModel g : realm.getDefaultGroups()) { @@ -534,7 +539,7 @@ public class LDAPStorageProvider implements UserStorageProvider, } logger.debugf("Imported new user from LDAP to Keycloak DB. Username: [%s], Email: [%s], LDAP_ID: [%s], LDAP Entry DN: [%s]", imported.getUsername(), imported.getEmail(), ldapUser.getUuid(), userDN); - UserModel proxy = proxy(realm, imported, ldapUser); + UserModel proxy = proxy(realm, imported, ldapUser, false); return proxy; } @@ -566,7 +571,7 @@ public class LDAPStorageProvider implements UserStorageProvider, LDAPUtils.checkUuid(ldapUser, ldapIdentityStore.getConfig()); // If email attribute mapper is set to "Always Read Value From LDAP" the user may be in Keycloak DB with an old email address if (ldapUser.getUuid().equals(user.getFirstAttribute(LDAPConstants.LDAP_ID))) { - return proxy(realm, user, ldapUser); + return proxy(realm, user, ldapUser, false); } throw new ModelDuplicateException("User with username '" + ldapUsername + "' already exists in Keycloak. It conflicts with LDAP user with email '" + email + "'"); } @@ -757,7 +762,7 @@ public class LDAPStorageProvider implements UserStorageProvider, } else { LDAPObject ldapObject = loadAndValidateUser(realm, user); if (ldapObject != null) { - return proxy(realm, user, ldapObject); + return proxy(realm, user, ldapObject, false); } else { logger.warnf("User with username [%s] aready exists and is linked to provider [%s] but is not valid. Stale LDAP_ID on local user is: %s", username, model.getName(), user.getFirstAttribute(LDAPConstants.LDAP_ID)); 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 2bdbb3b89b..80263d57cd 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 @@ -253,6 +253,7 @@ public class LDAPStorageProviderFactory implements UserStorageProviderFactoryMarek Posolda + */ +public class LDAPWritesOnlyUserModelDelegate extends UserModelDelegate { + + private final LDAPStorageProvider provider; + + public LDAPWritesOnlyUserModelDelegate(UserModel delegate, LDAPStorageProvider provider) { + super(delegate); + this.provider = provider; + } + + @Override + public void setUsername(String username) { + if (!isAttributeUpdatedInLDAP(UserModel.USERNAME)) { + super.setUsername(username); + } + } + + @Override + public void setEnabled(boolean enabled) { + if (!isAttributeUpdatedInLDAP(LDAPConstants.ENABLED)) { + super.setEnabled(enabled); + } + } + + @Override + public void setSingleAttribute(String name, String value) { + if (!isAttributeUpdatedInLDAP(name)) { + super.setSingleAttribute(name, value); + } + } + + @Override + public void setAttribute(String name, List values) { + if (!isAttributeUpdatedInLDAP(name)) { + super.setAttribute(name, values); + } + } + + @Override + public void removeAttribute(String name) { + if (!isAttributeUpdatedInLDAP(name)) { + super.removeAttribute(name); + } + } + + @Override + public void addRequiredAction(String action) { + if (!isRequiredActionUpdatedInLDAP(action)) { + super.addRequiredAction(action); + } + } + + @Override + public void removeRequiredAction(String action) { + if (!isRequiredActionUpdatedInLDAP(action)) { + super.removeRequiredAction(action); + } + } + + @Override + public void addRequiredAction(RequiredAction action) { + if (!isRequiredActionUpdatedInLDAP(action.toString())) { + super.addRequiredAction(action); + } + } + + @Override + public void removeRequiredAction(RequiredAction action) { + if (!isRequiredActionUpdatedInLDAP(action.toString())) { + super.removeRequiredAction(action); + } + } + + @Override + public void setFirstName(String firstName) { + if (!isAttributeUpdatedInLDAP(UserModel.FIRST_NAME)) { + super.setFirstName(firstName); + } + } + + @Override + public void setLastName(String lastName) { + if (!isAttributeUpdatedInLDAP(UserModel.LAST_NAME)) { + super.setLastName(lastName); + } + } + + @Override + public void setEmail(String email) { + if (!isAttributeUpdatedInLDAP(UserModel.EMAIL)) { + super.setEmail(email); + } + } + + @Override + public void setEmailVerified(boolean verified) { + if (!isAttributeUpdatedInLDAP("emailVerified")) { + super.setEmailVerified(verified); + } + } + + // Checks if attribute was updated in LDAP in this transaction + protected boolean isAttributeUpdatedInLDAP(String attributeName) { + LDAPTransaction transaction = provider.getUserManager().getTransaction(getId()); + if (transaction == null) return false; + return transaction.isAttributeUpdated(attributeName); + } + + // Checks if requiredAction was updated in LDAP in this transaction + protected boolean isRequiredActionUpdatedInLDAP(String requiredActionName) { + LDAPTransaction transaction = provider.getUserManager().getTransaction(getId()); + if (transaction == null) return false; + return transaction.isRequiredActionUpdated(requiredActionName); + } + + + +} diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/ReadonlyLDAPUserModelDelegate.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/ReadonlyLDAPUserModelDelegate.java deleted file mode 100755 index cea4c94f5b..0000000000 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/ReadonlyLDAPUserModelDelegate.java +++ /dev/null @@ -1,57 +0,0 @@ -/* - * Copyright 2016 Red Hat, Inc. and/or its affiliates - * and other contributors as indicated by the @author tags. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.keycloak.storage.ldap; - -import org.keycloak.models.UserModel; -import org.keycloak.models.utils.UserModelDelegate; -import org.keycloak.storage.ReadOnlyException; - -/** - * @author Bill Burke - * @version $Revision: 1 $ - */ -public class ReadonlyLDAPUserModelDelegate extends UserModelDelegate implements UserModel { - - protected LDAPStorageProvider provider; - - public ReadonlyLDAPUserModelDelegate(UserModel delegate, LDAPStorageProvider provider) { - super(delegate); - this.provider = provider; - } - - @Override - public void setUsername(String username) { - throw new ReadOnlyException("Federated storage is not writable"); - } - - @Override - public void setLastName(String lastName) { - throw new ReadOnlyException("Federated storage is not writable"); - } - - @Override - public void setFirstName(String first) { - throw new ReadOnlyException("Federated storage is not writable"); - } - - @Override - public void setEmail(String email) { - throw new ReadOnlyException("Federated storage is not writable"); - } - -} diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/UnsyncedLDAPUserModelDelegate.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/UnsyncedLDAPUserModelDelegate.java deleted file mode 100755 index e26104c823..0000000000 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/UnsyncedLDAPUserModelDelegate.java +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright 2016 Red Hat, Inc. and/or its affiliates - * and other contributors as indicated by the @author tags. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.keycloak.storage.ldap; - -import org.jboss.logging.Logger; -import org.keycloak.models.UserModel; -import org.keycloak.models.utils.UserModelDelegate; - -/** - * @author Bill Burke - * @version $Revision: 1 $ - */ -public class UnsyncedLDAPUserModelDelegate extends UserModelDelegate implements UserModel { - private static final Logger logger = Logger.getLogger(UnsyncedLDAPUserModelDelegate.class); - - protected LDAPStorageProvider provider; - - public UnsyncedLDAPUserModelDelegate(UserModel delegate, LDAPStorageProvider provider) { - super(delegate); - this.provider = provider; - } -} diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/WritableLDAPUserModelDelegate.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/WritableLDAPUserModelDelegate.java deleted file mode 100755 index 6b87bb80c3..0000000000 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/WritableLDAPUserModelDelegate.java +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright 2016 Red Hat, Inc. and/or its affiliates - * and other contributors as indicated by the @author tags. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.keycloak.storage.ldap; - -import org.jboss.logging.Logger; -import org.keycloak.models.UserModel; -import org.keycloak.models.utils.UserModelDelegate; -import org.keycloak.storage.ldap.idm.model.LDAPObject; - -/** - * @author Bill Burke - * @version $Revision: 1 $ - */ -public class WritableLDAPUserModelDelegate extends UserModelDelegate implements UserModel { - private static final Logger logger = Logger.getLogger(WritableLDAPUserModelDelegate.class); - - protected LDAPStorageProvider provider; - protected LDAPObject ldapObject; - - public WritableLDAPUserModelDelegate(UserModel delegate, LDAPStorageProvider provider, LDAPObject ldapObject) { - super(delegate); - this.provider = provider; - this.ldapObject = ldapObject; - } - -} diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java index 1d1f8e9bd2..9c3682e48c 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPIdentityStore.java @@ -218,10 +218,7 @@ public class LDAPIdentityStore implements IdentityStore { String newDn = newLdapDn.toString(); - // TODO:mposolda - if (logger.isInfoEnabled()) { - logger.infof("Renaming LDAP Object. Old DN: [%s], New DN: [%s]", oldDn, newDn); - } + logger.debugf("Renaming LDAP Object. Old DN: [%s], New DN: [%s]", oldDn, newDn); // In case, that there is conflict (For example already existing "CN=John Anthony"), the different DN is returned newDn = this.operationManager.renameEntry(oldDn, newDn, true); diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/AbstractLDAPStorageMapper.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/AbstractLDAPStorageMapper.java index dedf8d83e6..dde5371481 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/AbstractLDAPStorageMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/AbstractLDAPStorageMapper.java @@ -22,10 +22,8 @@ import org.keycloak.models.GroupModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; -import org.keycloak.storage.ldap.LDAPConfig; import org.keycloak.storage.ldap.LDAPStorageProvider; import org.keycloak.storage.ldap.idm.model.LDAPObject; -import org.keycloak.storage.ldap.idm.query.internal.LDAPQuery; import org.keycloak.storage.user.SynchronizationResult; import javax.naming.AuthenticationException; diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/FullNameLDAPStorageMapper.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/FullNameLDAPStorageMapper.java index 223a1151de..a7dd2ac8ee 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/FullNameLDAPStorageMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/FullNameLDAPStorageMapper.java @@ -90,16 +90,32 @@ public class FullNameLDAPStorageMapper extends AbstractLDAPStorageMapper { TxAwareLDAPUserModelDelegate txDelegate = new TxAwareLDAPUserModelDelegate(delegate, ldapProvider, ldapUser) { + // Per-transaction state. Useful due the fact that "setFirstName" and "setLastName" called within same transaction + private String firstName; + private String lastName; + + @Override + public String getFirstName() { + return firstName != null? firstName : super.getFirstName(); + } + + @Override + public String getLastName() { + return lastName != null ? lastName : super.getLastName(); + } + @Override public void setFirstName(String firstName) { - super.setFirstName(firstName); + this.firstName = firstName; setFullNameToLDAPObject(); + super.setFirstName(firstName); } @Override public void setLastName(String lastName) { - super.setLastName(lastName); + this.lastName = lastName; setFullNameToLDAPObject(); + super.setLastName(lastName); } private void setFullNameToLDAPObject() { @@ -108,7 +124,8 @@ public class FullNameLDAPStorageMapper extends AbstractLDAPStorageMapper { logger.tracef("Pushing full name attribute to LDAP. Full name: %s", fullName); } - ensureTransactionStarted(); + markUpdatedAttributeInTransaction(UserModel.FIRST_NAME); + markUpdatedAttributeInTransaction(UserModel.LAST_NAME); String ldapFullNameAttrName = getLdapFullNameAttrName(); ldapUser.setSingleAttribute(ldapFullNameAttrName, fullName); diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/LDAPTransaction.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/LDAPTransaction.java index 3cf91b9485..82bf6dddaf 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/LDAPTransaction.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/LDAPTransaction.java @@ -17,9 +17,11 @@ package org.keycloak.storage.ldap.mappers; +import java.util.HashSet; +import java.util.Set; + import org.jboss.logging.Logger; import org.keycloak.models.AbstractKeycloakTransaction; -import org.keycloak.models.KeycloakTransaction; import org.keycloak.storage.ldap.LDAPStorageProvider; import org.keycloak.storage.ldap.idm.model.LDAPObject; @@ -33,6 +35,9 @@ public class LDAPTransaction extends AbstractKeycloakTransaction { private final LDAPStorageProvider ldapProvider; private final LDAPObject ldapUser; + // Tracks the attributes updated in this transaction + private final Set updatedAttributes = new HashSet<>(); + public LDAPTransaction(LDAPStorageProvider ldapProvider, LDAPObject ldapUser) { this.ldapProvider = ldapProvider; this.ldapUser = ldapUser; @@ -54,5 +59,40 @@ public class LDAPTransaction extends AbstractKeycloakTransaction { logger.warn("Transaction rollback! Ignoring LDAP updates for object " + ldapUser.getDn().toString()); } + /** + * Add attribute, which will be updated in LDAP in this transaction + * + * @param attributeName model attribute name (For example "firstName", "lastName", "street") + */ + public void addUpdatedAttribute(String attributeName) { + updatedAttributes.add(attributeName); + } + + /** + * @param attributeName model attribute name (For example "firstName", "lastName", "street") + * @return true if attribute was updated in this transaction + */ + public boolean isAttributeUpdated(String attributeName) { + return updatedAttributes.contains(attributeName); + } + + /** + * Add required action, which will be updated in LDAP in this transaction + * + * @param requiredActionName + */ + public void addUpdatedRequiredAction(String requiredActionName) { + updatedAttributes.add("requiredAction(" + requiredActionName + ")"); + } + + /** + * + * @param requiredActionName + * @return true if requiredAction was updated in this transaction + */ + public boolean isRequiredActionUpdated(String requiredActionName) { + return updatedAttributes.contains("requiredAction(" + requiredActionName + ")"); + } + } diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/TxAwareLDAPUserModelDelegate.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/TxAwareLDAPUserModelDelegate.java index 09f4051e36..ec7ca64e48 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/TxAwareLDAPUserModelDelegate.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/TxAwareLDAPUserModelDelegate.java @@ -50,4 +50,16 @@ public abstract class TxAwareLDAPUserModelDelegate extends UserModelDelegate { } } + protected void markUpdatedAttributeInTransaction(String modelAttributeName) { + ensureTransactionStarted(); + LDAPTransaction transaction = provider.getUserManager().getTransaction(getId()); + transaction.addUpdatedAttribute(modelAttributeName); + } + + protected void markUpdatedRequiredActionInTransaction(String requiredActionName) { + ensureTransactionStarted(); + LDAPTransaction transaction = provider.getUserManager().getTransaction(getId()); + transaction.addUpdatedRequiredAction(requiredActionName); + } + } diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/UserAttributeLDAPStorageMapper.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/UserAttributeLDAPStorageMapper.java index c9beabc4ca..3a21eaab91 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/UserAttributeLDAPStorageMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/UserAttributeLDAPStorageMapper.java @@ -27,17 +27,13 @@ import org.keycloak.models.UserModel; import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.UserModelDelegate; import org.keycloak.models.utils.reflection.Property; -import org.keycloak.models.utils.reflection.PropertyCriteria; -import org.keycloak.models.utils.reflection.PropertyQueries; import org.keycloak.storage.UserStorageProvider; import org.keycloak.storage.ldap.LDAPStorageProvider; import org.keycloak.storage.ldap.LDAPUtils; import org.keycloak.storage.ldap.idm.model.LDAPObject; import org.keycloak.storage.ldap.idm.query.Condition; import org.keycloak.storage.ldap.idm.query.internal.LDAPQuery; -import org.keycloak.storage.ldap.idm.store.ldap.LDAPUtil; -import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -46,6 +42,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; + import org.keycloak.models.ModelException; /** @@ -208,7 +205,7 @@ public class UserAttributeLDAPStorageMapper extends AbstractLDAPStorageMapper { @Override public void removeAttribute(String name) { - if ( setLDAPAttribute(name, null)) { + if (setLDAPAttribute(name, null)) { super.removeAttribute(name); } } @@ -246,7 +243,7 @@ public class UserAttributeLDAPStorageMapper extends AbstractLDAPStorageMapper { UserAttributeLDAPStorageMapper.logger.tracef("Pushing user attribute to LDAP. username: %s, Model attribute name: %s, LDAP attribute name: %s, Attribute value: %s", getUsername(), modelAttrName, ldapAttrName, value); } - ensureTransactionStarted(); + markUpdatedAttributeInTransaction(modelAttrName); if (value == null) { if (isMandatoryInLdap) { diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapper.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapper.java index f991fec724..a8e0476023 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/msad/MSADUserAccountControlStorageMapper.java @@ -241,7 +241,7 @@ public class MSADUserAccountControlStorageMapper extends AbstractLDAPStorageMapp control.add(UserAccountControl.ACCOUNTDISABLE); } - ensureTransactionStarted(); + markUpdatedAttributeInTransaction(LDAPConstants.ENABLED); updateUserAccountControl(false, ldapUser, control); } @@ -266,7 +266,7 @@ public class MSADUserAccountControlStorageMapper extends AbstractLDAPStorageMapp ldapUser.setSingleAttribute(LDAPConstants.PWD_LAST_SET, "0"); - ensureTransactionStarted(); + markUpdatedRequiredActionInTransaction(action); } } @@ -293,7 +293,7 @@ public class MSADUserAccountControlStorageMapper extends AbstractLDAPStorageMapp ldapUser.setSingleAttribute(LDAPConstants.PWD_LAST_SET, "-1"); - ensureTransactionStarted(); + markUpdatedRequiredActionInTransaction(action); } } } diff --git a/server-spi-private/src/main/java/org/keycloak/models/LDAPConstants.java b/server-spi-private/src/main/java/org/keycloak/models/LDAPConstants.java index c631a60a2f..f001ba9090 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/LDAPConstants.java +++ b/server-spi-private/src/main/java/org/keycloak/models/LDAPConstants.java @@ -119,7 +119,9 @@ public class LDAPConstants { public static final String EMPTY_ATTRIBUTE_VALUE = " "; public static final String EMPTY_MEMBER_ATTRIBUTE_VALUE = "cn=empty-membership-placeholder"; - public static final String CUSTOM_ATTRIBUTE_ENABLED = "enabled"; + public static final String ENABLED = "enabled"; + + public static final String CUSTOM_ATTRIBUTE_CREATE_DATE = "createDate"; public static final String CUSTOM_ATTRIBUTE_EXPIRY_DATE = "expiryDate"; public static final String ENTRY_UUID = "entryUUID"; diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/ReadOnlyUserModelDelegate.java b/server-spi-private/src/main/java/org/keycloak/models/utils/ReadOnlyUserModelDelegate.java index ddc613e6f1..8ad38ee3f4 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/ReadOnlyUserModelDelegate.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/ReadOnlyUserModelDelegate.java @@ -22,113 +22,127 @@ import org.keycloak.models.UserModel; import org.keycloak.storage.ReadOnlyException; import java.util.List; +import java.util.function.Function; /** * @author Bill Burke * @version $Revision: 1 $ */ public class ReadOnlyUserModelDelegate extends UserModelDelegate { + + private final Function exceptionCreator; + public ReadOnlyUserModelDelegate(UserModel delegate) { + this(delegate, ReadOnlyException::new); + } + + public ReadOnlyUserModelDelegate(UserModel delegate, Function exceptionCreator) { super(delegate); + this.exceptionCreator = exceptionCreator; } @Override public void setUsername(String username) { - throw new ReadOnlyException(); + throw readOnlyException("username"); } @Override public void setEnabled(boolean enabled) { - throw new ReadOnlyException(); + throw readOnlyException("enabled"); } @Override public void setSingleAttribute(String name, String value) { - throw new ReadOnlyException(); + throw readOnlyException("attribute(" + name + ")"); } @Override public void setAttribute(String name, List values) { - throw new ReadOnlyException(); + throw readOnlyException("attribute(" + name + ")"); } @Override public void removeAttribute(String name) { - throw new ReadOnlyException(); + throw readOnlyException("attribute(" + name + ")"); } @Override public void addRequiredAction(String action) { - throw new ReadOnlyException(); + throw readOnlyException("required action " + action); } @Override public void removeRequiredAction(String action) { - throw new ReadOnlyException(); + throw readOnlyException("required action " + action); } @Override public void addRequiredAction(RequiredAction action) { - throw new ReadOnlyException(); + throw readOnlyException("required action " + action); } @Override public void removeRequiredAction(RequiredAction action) { - throw new ReadOnlyException(); + throw readOnlyException("required action " + action); } @Override public void setFirstName(String firstName) { - throw new ReadOnlyException(); + throw readOnlyException("firstName"); } @Override public void setLastName(String lastName) { - throw new ReadOnlyException(); + throw readOnlyException("lastName"); } @Override public void setEmail(String email) { - throw new ReadOnlyException(); + throw readOnlyException("email"); } @Override public void setEmailVerified(boolean verified) { - throw new ReadOnlyException(); + throw readOnlyException("emailVerified"); } @Override public void deleteRoleMapping(RoleModel role) { - throw new ReadOnlyException(); + throw readOnlyException("role mapping for role " + role.getName()); } @Override public void setFederationLink(String link) { - throw new ReadOnlyException(); + throw readOnlyException("federationLink"); } @Override public void setServiceAccountClientLink(String clientInternalId) { - throw new ReadOnlyException(); + throw readOnlyException("serviceAccountClientLink"); } @Override public void setCreatedTimestamp(Long timestamp) { - throw new ReadOnlyException(); + throw readOnlyException("createdTimestamp"); } @Override public void joinGroup(GroupModel group) { - throw new ReadOnlyException(); + throw readOnlyException("group mapping for group " + group.getName()); } @Override public void leaveGroup(GroupModel group) { - throw new ReadOnlyException(); + throw readOnlyException("group mapping for group " + group.getName()); } @Override public void grantRole(RoleModel role) { - throw new ReadOnlyException(); + throw readOnlyException("role mapping for role " + role.getName()); + } + + private RuntimeException readOnlyException(String detail) { + String message = String.format("Not possible to write '%s' when updating user '%s'", detail, getUsername()); + return exceptionCreator.apply(message); } } diff --git a/server-spi-private/src/main/java/org/keycloak/storage/adapter/InMemoryUserAdapter.java b/server-spi-private/src/main/java/org/keycloak/storage/adapter/InMemoryUserAdapter.java index 4be9e26021..f4fca16d9c 100644 --- a/server-spi-private/src/main/java/org/keycloak/storage/adapter/InMemoryUserAdapter.java +++ b/server-spi-private/src/main/java/org/keycloak/storage/adapter/InMemoryUserAdapter.java @@ -251,7 +251,7 @@ public class InMemoryUserAdapter implements UserModel { @Override public Set getGroups() { - if (groupIds.isEmpty()) return Collections.emptySet(); + if (groupIds.isEmpty()) return new HashSet<>(); Set groups = new HashSet<>(); for (String id : groupIds) { groups.add(realm.getGroupById(id)); @@ -347,7 +347,7 @@ public class InMemoryUserAdapter implements UserModel { @Override public Set getRoleMappings() { - if (roleIds.isEmpty()) return Collections.emptySet(); + if (roleIds.isEmpty()) return new HashSet<>(); Set roles = new HashSet<>(); for (String id : roleIds) { roles.add(realm.getRoleById(id)); diff --git a/server-spi-private/src/main/java/org/keycloak/storage/adapter/UpdateOnlyChangeUserModelDelegate.java b/server-spi-private/src/main/java/org/keycloak/storage/adapter/UpdateOnlyChangeUserModelDelegate.java new file mode 100644 index 0000000000..0de7dc8148 --- /dev/null +++ b/server-spi-private/src/main/java/org/keycloak/storage/adapter/UpdateOnlyChangeUserModelDelegate.java @@ -0,0 +1,185 @@ +/* + * Copyright 2019 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.keycloak.storage.adapter; + +import java.util.List; +import org.keycloak.models.GroupModel; +import org.keycloak.models.RoleModel; +import org.keycloak.models.UserModel; +import org.keycloak.models.utils.UserModelDelegate; + +import static org.keycloak.common.util.ObjectUtil.isEqualOrBothNull; + +/** + * This will perform update operation for particular attribute/property just if the existing value is not already same. + * In other words, just "real updates" will be passed to the delegate. + * + * @author Marek Posolda + */ +public class UpdateOnlyChangeUserModelDelegate extends UserModelDelegate { + + public UpdateOnlyChangeUserModelDelegate(UserModel delegate) { + super(delegate); + } + + @Override + public void setUsername(String username) { + if (!isEqualOrBothNull(getUsername(), username)) { + delegate.setUsername(username); + } + } + + @Override + public void setEnabled(boolean enabled) { + if (!isEqualOrBothNull(isEnabled(), enabled)) { + delegate.setEnabled(enabled); + } + } + + @Override + public void setSingleAttribute(String name, String value) { + if (!isEqualOrBothNull(getFirstAttribute(name), value)) { + delegate.setSingleAttribute(name, value); + } + } + + @Override + public void setAttribute(String name, List values) { + if (!isEqualOrBothNull(getAttribute(name), values)) { + delegate.setAttribute(name, values); + } + } + + @Override + public void removeAttribute(String name) { + List currentVal = getAttribute(name); + if (currentVal != null && !currentVal.isEmpty()) { + delegate.removeAttribute(name); + } + } + + @Override + public void addRequiredAction(String action) { + if (!getRequiredActions().contains(action)) { + delegate.addRequiredAction(action); + } + } + + @Override + public void removeRequiredAction(String action) { + if (getRequiredActions().contains(action)) { + delegate.removeRequiredAction(action); + } + } + + @Override + public void addRequiredAction(RequiredAction action) { + String actionName = action.name(); + addRequiredAction(actionName); + } + + @Override + public void removeRequiredAction(RequiredAction action) { + String actionName = action.name(); + removeRequiredAction(actionName); + } + + + @Override + public void setFirstName(String firstName) { + if (!isEqualOrBothNull(getFirstName(), firstName)) { + delegate.setFirstName(firstName); + } + } + + @Override + public void setLastName(String lastName) { + if (!isEqualOrBothNull(getLastName(), lastName)) { + delegate.setLastName(lastName); + } + } + + @Override + public void setEmail(String email) { + if (!isEqualOrBothNull(getEmail(), email)) { + delegate.setEmail(email); + } + } + + + + @Override + public void setEmailVerified(boolean verified) { + if (!isEqualOrBothNull(isEmailVerified(), verified)) { + delegate.setEmailVerified(verified); + } + } + + @Override + public void grantRole(RoleModel role) { + if (!hasRole(role)) { + delegate.grantRole(role); + } + } + + @Override + public void deleteRoleMapping(RoleModel role) { + if (hasRole(role)) { + delegate.deleteRoleMapping(role); + } + } + + @Override + public void setFederationLink(String link) { + if (!isEqualOrBothNull(getFederationLink(), link)) { + delegate.setFederationLink(link); + } + } + + @Override + public void setServiceAccountClientLink(String clientInternalId) { + if (!isEqualOrBothNull(getServiceAccountClientLink(), clientInternalId)) { + delegate.setServiceAccountClientLink(clientInternalId); + } + } + + @Override + public void setCreatedTimestamp(Long timestamp){ + if (!isEqualOrBothNull(getCreatedTimestamp(), timestamp)) { + delegate.setCreatedTimestamp(timestamp); + } + } + + @Override + public void joinGroup(GroupModel group) { + if (!isMemberOf(group)) { + delegate.joinGroup(group); + } + + } + + @Override + public void leaveGroup(GroupModel group) { + if (isMemberOf(group)) { + delegate.leaveGroup(group); + } + } + + +} diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ClientRoleMappingsResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ClientRoleMappingsResource.java index 3855cb4ce0..f0c2cc1e64 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/ClientRoleMappingsResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/ClientRoleMappingsResource.java @@ -18,6 +18,8 @@ package org.keycloak.services.resources.admin; import org.jboss.logging.Logger; import org.jboss.resteasy.annotations.cache.NoCache; + +import javax.ws.rs.BadRequestException; import javax.ws.rs.NotFoundException; import org.keycloak.services.resources.admin.permissions.AdminPermissionEvaluator; import org.keycloak.events.admin.OperationType; @@ -31,6 +33,7 @@ import org.keycloak.models.RoleModel; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.services.ErrorResponseException; +import org.keycloak.storage.ReadOnlyException; import javax.ws.rs.Consumes; import javax.ws.rs.DELETE; @@ -168,14 +171,20 @@ public class ClientRoleMappingsResource { public void addClientRoleMapping(List roles) { managePermission.require(); - for (RoleRepresentation role : roles) { - RoleModel roleModel = client.getRole(role.getName()); - if (roleModel == null || !roleModel.getId().equals(role.getId())) { - throw new NotFoundException("Role not found"); + try { + for (RoleRepresentation role : roles) { + RoleModel roleModel = client.getRole(role.getName()); + if (roleModel == null || !roleModel.getId().equals(role.getId())) { + throw new NotFoundException("Role not found"); + } + auth.roles().requireMapRole(roleModel); + user.grantRole(roleModel); } - auth.roles().requireMapRole(roleModel); - user.grantRole(roleModel); + } catch (ModelException | ReadOnlyException me) { + logger.warn(me.getMessage(), me); + throw new ErrorResponseException("invalid_request", "Could not add user role mappings!", Response.Status.BAD_REQUEST); } + adminEvent.operation(OperationType.CREATE).resourcePath(uriInfo).representation(roles).success(); } @@ -214,10 +223,9 @@ public class ClientRoleMappingsResource { auth.roles().requireMapRole(roleModel); try { user.deleteRoleMapping(roleModel); - } catch (ModelException me) { - Properties messages = AdminRoot.getMessages(session, realm, auth.adminAuth().getToken().getLocale()); - throw new ErrorResponseException(me.getMessage(), MessageFormat.format(messages.getProperty(me.getMessage(), me.getMessage()), me.getParameters()), - Response.Status.BAD_REQUEST); + } catch (ModelException | ReadOnlyException me) { + logger.warn(me.getMessage(), me); + throw new ErrorResponseException("invalid_request", "Could not remove user role mappings!", Response.Status.BAD_REQUEST); } } } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java b/services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java index fc311fc341..c4c5cd1d4e 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/RoleMapperResource.java @@ -18,6 +18,8 @@ package org.keycloak.services.resources.admin; import org.jboss.logging.Logger; import org.jboss.resteasy.annotations.cache.NoCache; + +import javax.ws.rs.BadRequestException; import javax.ws.rs.NotFoundException; import org.keycloak.common.ClientConnection; import org.keycloak.events.admin.OperationType; @@ -33,8 +35,10 @@ import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.idm.ClientMappingsRepresentation; import org.keycloak.representations.idm.MappingsRepresentation; import org.keycloak.representations.idm.RoleRepresentation; +import org.keycloak.services.ErrorResponse; import org.keycloak.services.ErrorResponseException; import org.keycloak.services.resources.admin.permissions.AdminPermissionEvaluator; +import org.keycloak.storage.ReadOnlyException; import javax.ws.rs.Consumes; import javax.ws.rs.DELETE; @@ -220,13 +224,18 @@ public class RoleMapperResource { logger.debugv("** addRealmRoleMappings: {0}", roles); - for (RoleRepresentation role : roles) { - RoleModel roleModel = realm.getRole(role.getName()); - if (roleModel == null || !roleModel.getId().equals(role.getId())) { - throw new NotFoundException("Role not found"); + try { + for (RoleRepresentation role : roles) { + RoleModel roleModel = realm.getRole(role.getName()); + if (roleModel == null || !roleModel.getId().equals(role.getId())) { + throw new NotFoundException("Role not found"); + } + auth.roles().requireMapRole(roleModel); + roleMapper.grantRole(roleModel); } - auth.roles().requireMapRole(roleModel); - roleMapper.grantRole(roleModel); + } catch (ModelException | ReadOnlyException me) { + logger.warn(me.getMessage(), me); + throw new ErrorResponseException("invalid_request", "Could not add user role mappings!", Response.Status.BAD_REQUEST); } adminEvent.operation(OperationType.CREATE).resourcePath(session.getContext().getUri()).representation(roles).success(); @@ -263,10 +272,9 @@ public class RoleMapperResource { auth.roles().requireMapRole(roleModel); try { roleMapper.deleteRoleMapping(roleModel); - } catch (ModelException me) { - Properties messages = AdminRoot.getMessages(session, realm, auth.adminAuth().getToken().getLocale()); - throw new ErrorResponseException(me.getMessage(), MessageFormat.format(messages.getProperty(me.getMessage(), me.getMessage()), me.getParameters()), - Response.Status.BAD_REQUEST); + } catch (ModelException | ReadOnlyException me) { + logger.warn(me.getMessage(), me); + throw new ErrorResponseException("invalid_request", "Could not remove user role mappings!", Response.Status.BAD_REQUEST); } } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java index 95664a509f..8fff8eaacd 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java @@ -185,12 +185,12 @@ public class UserResource { return ErrorResponse.exists("User is read only!"); } catch (ModelException me) { logger.warn("Could not update user!", me); - return ErrorResponse.exists("Could not update user!"); + return ErrorResponse.error("Could not update user!", Status.BAD_REQUEST); } catch (ForbiddenException fe) { throw fe; } catch (Exception me) { // JPA logger.warn("Could not update user!", me);// may be committed by JTA which can't - return ErrorResponse.exists("Could not update user!"); + return ErrorResponse.error("Could not update user!", Status.BAD_REQUEST); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java index dbb45de3ab..2f8dfe23e7 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPGroupMapperTest.java @@ -241,11 +241,8 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { LDAPTestUtils.updateGroupMapperConfigOptions(mapperModel, GroupMapperConfig.MODE, LDAPGroupMapperMode.READ_ONLY.toString()); appRealm.updateComponent(mapperModel); - UserModel mary = session.users().getUserByUsername("marykeycloak", appRealm); - GroupModel group1 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1"); GroupModel group11 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group11"); - GroupModel group12 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group12"); // Add some group mappings directly into LDAP GroupLDAPStorageMapper groupMapper = LDAPTestUtils.getGroupMapper(mapperModel, ctx.getLdapProvider(), appRealm); @@ -253,33 +250,94 @@ public class LDAPGroupMapperTest extends AbstractLDAPTest { LDAPObject maryLdap = ctx.getLdapProvider().loadLDAPUserByUsername(appRealm, "marykeycloak"); groupMapper.addGroupMappingInLDAP(appRealm, group1, maryLdap); groupMapper.addGroupMappingInLDAP(appRealm, group11, maryLdap); - - // Add some group mapping to model - mary.joinGroup(group12); - - // Assert that mary has both LDAP and DB mapped groups - Set maryGroups = mary.getGroups(); - Assert.assertEquals(5, maryGroups.size()); - Assert.assertTrue(maryGroups.contains(group1)); - Assert.assertTrue(maryGroups.contains(group11)); - Assert.assertTrue(maryGroups.contains(group12)); - - long groupCount = mary.getGroupsCount(); - Assert.assertEquals(5, groupCount); - - Set maryGroupsWithGr = mary.getGroups("gr", 0, 10); - Assert.assertEquals(5, maryGroupsWithGr.size()); - - Set maryGroupsWithGr2 = mary.getGroups("gr", 1, 10); - Assert.assertEquals(4, maryGroupsWithGr2.size()); - - Set maryGroupsWithGr3 = mary.getGroups("gr", 0, 1); - Assert.assertEquals(1, maryGroupsWithGr3.size()); - - Set maryGroupsWith12 = mary.getGroups("12", 0, 10); - Assert.assertEquals(2, maryGroupsWith12.size()); }); + if (importEnabled) { + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + + UserModel mary = session.users().getUserByUsername("marykeycloak", appRealm); + GroupModel group1 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1"); + GroupModel group11 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group11"); + GroupModel group12 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group12"); + + // Add some group mapping to model + mary.joinGroup(group12); + + // Assert that mary has both LDAP and DB mapped groups + Set maryGroups = mary.getGroups(); + Assert.assertEquals(5, maryGroups.size()); + Assert.assertTrue(maryGroups.contains(group1)); + Assert.assertTrue(maryGroups.contains(group11)); + Assert.assertTrue(maryGroups.contains(group12)); + + long groupCount = mary.getGroupsCount(); + Assert.assertEquals(5, groupCount); + + Set maryGroupsWithGr = mary.getGroups("gr", 0, 10); + Assert.assertEquals(5, maryGroupsWithGr.size()); + + Set maryGroupsWithGr2 = mary.getGroups("gr", 1, 10); + Assert.assertEquals(4, maryGroupsWithGr2.size()); + + Set maryGroupsWithGr3 = mary.getGroups("gr", 0, 1); + Assert.assertEquals(1, maryGroupsWithGr3.size()); + + Set maryGroupsWith12 = mary.getGroups("12", 0, 10); + Assert.assertEquals(2, maryGroupsWith12.size()); + }); + } else { + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + + UserModel mary = session.users().getUserByUsername("marykeycloak", appRealm); + GroupModel group12 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group12"); + + // Add some group mapping to model. This should fail with no-import mode for LDAP provider READ_ONLY mode for the group mapper + // as it is not allowed to update group mappings in LDAP nor in the DB + try { + mary.joinGroup(group12); + Assert.fail("Not expected to successfully add group12 in no-import mode and READ_ONLY mode of the group mapper"); + } catch (ModelException me) { + // Ignore + } + }); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + + UserModel mary = session.users().getUserByUsername("marykeycloak", appRealm); + GroupModel group1 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1"); + GroupModel group11 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group11"); + GroupModel group12 = KeycloakModelUtils.findGroupByPath(appRealm, "/group1/group12"); + + // Assert that mary has both LDAP and DB mapped groups + Set maryGroups = mary.getGroups(); + Assert.assertEquals(4, maryGroups.size()); + Assert.assertTrue(maryGroups.contains(group1)); + Assert.assertTrue(maryGroups.contains(group11)); + Assert.assertFalse(maryGroups.contains(group12)); + + long groupCount = mary.getGroupsCount(); + Assert.assertEquals(4, groupCount); + + Set maryGroupsWithGr = mary.getGroups("gr", 0, 10); + Assert.assertEquals(4, maryGroupsWithGr.size()); + + Set maryGroupsWithGr2 = mary.getGroups("gr", 1, 10); + Assert.assertEquals(3, maryGroupsWithGr2.size()); + + Set maryGroupsWithGr3 = mary.getGroups("gr", 0, 1); + Assert.assertEquals(1, maryGroupsWithGr3.size()); + + Set maryGroupsWith12 = mary.getGroups("12", 0, 10); + Assert.assertEquals(1, maryGroupsWith12.size()); + }); + } + // Assert that access through DB will have just DB mapped groups if (importEnabled) { testingClient.server().run(session -> { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java index f105f05d81..605bb5f617 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/noimport/LDAPProvidersIntegrationNoImportTest.java @@ -17,6 +17,9 @@ package org.keycloak.testsuite.federation.ldap.noimport; +import java.util.Collections; + +import javax.ws.rs.BadRequestException; import javax.ws.rs.core.Response; import org.junit.Assert; @@ -24,6 +27,8 @@ import org.junit.FixMethodOrder; import org.junit.Ignore; import org.junit.Test; import org.junit.runners.MethodSorters; +import org.keycloak.admin.client.resource.ComponentResource; +import org.keycloak.admin.client.resource.UserResource; import org.keycloak.component.ComponentModel; import org.keycloak.models.LDAPConstants; import org.keycloak.models.RealmModel; @@ -33,15 +38,18 @@ import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.idm.ComponentRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.storage.StorageId; +import org.keycloak.storage.UserStorageProvider; import org.keycloak.storage.ldap.LDAPStorageProvider; import org.keycloak.storage.ldap.mappers.FullNameLDAPStorageMapper; import org.keycloak.storage.ldap.mappers.FullNameLDAPStorageMapperFactory; import org.keycloak.storage.ldap.mappers.LDAPStorageMapper; import org.keycloak.storage.ldap.mappers.UserAttributeLDAPStorageMapper; +import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.federation.ldap.LDAPProvidersIntegrationTest; import org.keycloak.testsuite.federation.ldap.LDAPTestAsserts; import org.keycloak.testsuite.federation.ldap.LDAPTestContext; import org.keycloak.testsuite.util.LDAPTestUtils; +import org.keycloak.testsuite.util.WaitUtils; /** @@ -155,8 +163,25 @@ public class LDAPProvidersIntegrationNoImportTest extends LDAPProvidersIntegrati @Test @Override - @Ignore // Unsynced mode doesn't have much sense in no-import + // Unsynced mode doesn't have much sense in no-import. So it is not allowed at the configuration level public void testUnsynced() throws Exception { + ComponentResource ldapProviderResource = testRealm().components().component(ldapModelId); + ComponentRepresentation ldapProviderRep = ldapProviderResource.toRepresentation(); + String currentEditMode = ldapProviderRep.getConfig().getFirst(LDAPConstants.EDIT_MODE); + Assert.assertEquals(UserStorageProvider.EditMode.WRITABLE.toString(), currentEditMode); + + // Try update editMode to UNSYNCED. It should not work as UNSYNCED with no-import is not allowed + ldapProviderRep.getConfig().putSingle(LDAPConstants.EDIT_MODE, UserStorageProvider.EditMode.UNSYNCED.toString()); + try { + ldapProviderResource.update(ldapProviderRep); + Assert.fail("Not expected to successfully update provider"); + } catch (BadRequestException bre) { + // Expected + } + + // Try to set editMode to WRITABLE should work + ldapProviderRep.getConfig().putSingle(LDAPConstants.EDIT_MODE, currentEditMode); + ldapProviderResource.update(ldapProviderRep); } @@ -219,7 +244,6 @@ public class LDAPProvidersIntegrationNoImportTest extends LDAPProvidersIntegrati fullnameUser.setLastName("Dee2"); }); - // Assert changed user available in Keycloak, but his firstName is null (due the fullnameMapper is write-only and firstName mapper is removed) testingClient.server().run(session -> { LDAPTestContext ctx = LDAPTestContext.init(session); @@ -243,4 +267,67 @@ public class LDAPProvidersIntegrationNoImportTest extends LDAPProvidersIntegrati response.close(); } + // Tests that attempt to change some user attributes, which are not mapped to LDAP, will fail + @Test + public void testImpossibleToChangeNonLDAPMappedAttributes() { + UserResource john = ApiUtil.findUserByUsernameId(testRealm(), "johnkeycloak"); + + UserRepresentation johnRep = john.toRepresentation(); + String firstNameOrig = johnRep.getFirstName(); + String lastNameOrig = johnRep.getLastName(); + String postalCodeOrig = johnRep.getAttributes().get("postal_code").get(0); + + try { + // Attempt to disable user should fail + try { + johnRep.setFirstName("John2"); + johnRep.setLastName("Doe2"); + johnRep.setEnabled(false); + + john.update(johnRep); + Assert.fail("Not supposed to successfully update 'enabled' state of the user"); + } catch (BadRequestException bre) { + // Expected + } + + // Attempt to set requiredAction to the user should fail + try { + johnRep = john.toRepresentation(); + johnRep.setRequiredActions(Collections.singletonList(UserModel.RequiredAction.CONFIGURE_TOTP.toString())); + john.update(johnRep); + Assert.fail("Not supposed to successfully add requiredAction to the user"); + } catch (BadRequestException bre) { + // Expected + } + + // Attempt to add some new attribute should fail + try { + johnRep = john.toRepresentation(); + johnRep.singleAttribute("foo", "bar"); + john.update(johnRep); + Assert.fail("Not supposed to successfully add attribute to the user"); + } catch (BadRequestException bre) { + // Expected + } + + // Attempt to update firstName, lastName and postal_code should be successful. All those attributes are mapped to LDAP + johnRep = john.toRepresentation(); + johnRep.setFirstName("John2"); + johnRep.setLastName("Doe2"); + johnRep.singleAttribute("postal_code", "654321"); + john.update(johnRep); + + johnRep = john.toRepresentation(); + Assert.assertEquals("John2", johnRep.getFirstName()); + Assert.assertEquals("Doe2", johnRep.getLastName()); + Assert.assertEquals("654321", johnRep.getAttributes().get("postal_code").get(0)); + } finally { + // Revert + johnRep.setFirstName(firstNameOrig); + johnRep.setLastName(lastNameOrig); + johnRep.singleAttribute("postal_code", postalCodeOrig); + john.update(johnRep); + } + } + } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountTest.java index 879a2d9058..322dcd8ed5 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/ServiceAccountTest.java @@ -304,7 +304,7 @@ public class ServiceAccountTest extends AbstractKeycloakTest { representation.setCredentials(Arrays.asList(password)); this.expectedException.expect(Matchers.allOf(Matchers.instanceOf(ClientErrorException.class), - Matchers.hasProperty("response", Matchers.hasProperty("status", Matchers.is(409))))); + Matchers.hasProperty("response", Matchers.hasProperty("status", Matchers.is(400))))); this.expectedException.reportMissingExceptionWithMessage("Should fail, should not be possible to manage credentials for service accounts"); serviceAccount.update(representation); diff --git a/themes/src/main/resources/theme/base/admin/messages/messages_en.properties b/themes/src/main/resources/theme/base/admin/messages/messages_en.properties index c14a013f71..3efbf4ae09 100644 --- a/themes/src/main/resources/theme/base/admin/messages/messages_en.properties +++ b/themes/src/main/resources/theme/base/admin/messages/messages_en.properties @@ -17,6 +17,7 @@ ldapErrorCantPreserveGroupInheritanceWithUIDMembershipType=Not possible to prese ldapErrorCantWriteOnlyForReadOnlyLdap=Can not set write only when LDAP provider mode is not WRITABLE ldapErrorCantWriteOnlyAndReadOnly=Can not set write-only and read-only together ldapErrorCantEnableStartTlsAndConnectionPooling=Can not enable both StartTLS and connection pooling. +ldapErrorCantEnableUnsyncedAndImportOff=Can not disable Importing users when LDAP provider mode is UNSYNCED clientRedirectURIsFragmentError=Redirect URIs must not contain an URI fragment clientRootURLFragmentError=Root URL must not contain an URL fragment