This is an automated email from the ASF dual-hosted git repository. andreapatricelli pushed a commit to branch 2_1_X in repository https://gitbox.apache.org/repos/asf/syncope.git
The following commit(s) were added to refs/heads/2_1_X by this push: new 0084592 [SYNCOPE-1666] added security answer hashing (#319) 0084592 is described below commit 008459240094e7d021553b484d47aa1569626250 Author: Andrea Patricelli <andreapatrice...@apache.org> AuthorDate: Thu Mar 3 17:34:55 2022 +0100 [SYNCOPE-1666] added security answer hashing (#319) [SYNCOPE-1666] added security answer hashing --- .../org/apache/syncope/core/logic/UserLogic.java | 31 +++--- .../core/persistence/api/entity/user/Account.java | 6 +- .../core/persistence/api/entity/user/User.java | 4 + .../jpa/entity/user/JPALinkedAccount.java | 21 +++- .../core/persistence/jpa/entity/user/JPAUser.java | 49 +++++++-- .../persistence/jpa/inner/MultitenancyTest.java | 3 +- .../core/persistence/jpa/inner/UserTest.java | 61 +++++++++-- .../core/persistence/jpa/outer/UserTest.java | 3 +- .../core/provisioning/api/data/UserDataBinder.java | 2 - .../core/provisioning/java/MappingManagerImpl.java | 2 +- .../provisioning/java/data/UserDataBinderImpl.java | 119 +++++++++++++-------- .../java/propagation/DeletingLinkedAccount.java | 9 +- .../provisioning/java/MappingManagerImplTest.java | 7 +- .../core/spring/policy/DefaultPasswordRule.java | 2 +- .../spring/policy/HaveIBeenPwnedPasswordRule.java | 2 +- .../syncope/core/logic/UserRequestLogic.java | 2 +- .../fit/core/reference/TestPasswordRule.java | 2 +- .../configurationparameters.adoc | 3 +- 18 files changed, 236 insertions(+), 92 deletions(-) diff --git a/core/logic/src/main/java/org/apache/syncope/core/logic/UserLogic.java b/core/logic/src/main/java/org/apache/syncope/core/logic/UserLogic.java index ad59396..dd5ec19 100644 --- a/core/logic/src/main/java/org/apache/syncope/core/logic/UserLogic.java +++ b/core/logic/src/main/java/org/apache/syncope/core/logic/UserLogic.java @@ -60,6 +60,7 @@ import org.apache.syncope.core.provisioning.api.data.UserDataBinder; import org.apache.syncope.core.provisioning.api.serialization.POJOHelper; import org.apache.syncope.core.provisioning.api.utils.RealmUtils; import org.apache.syncope.core.spring.security.AuthContextUtils; +import org.apache.syncope.core.spring.security.Encryptor; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.stereotype.Component; @@ -107,14 +108,14 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { return Triple.of( POJOHelper.serialize(AuthContextUtils.getAuthorizations()), POJOHelper.serialize(delegationDAO.findValidDelegating(authenticatedUser.getKey())), - binder.returnUserTO(authenticatedUser)); + authenticatedUser); } @PreAuthorize("hasRole('" + StandardEntitlement.USER_READ + "')") @Transactional(readOnly = true) @Override public UserTO read(final String key) { - return binder.returnUserTO(binder.getUserTO(key)); + return binder.getUserTO(key); } @PreAuthorize("hasRole('" + StandardEntitlement.USER_SEARCH + "')") @@ -137,7 +138,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { List<User> matching = searchDAO.search(authRealms, effectiveCond, page, size, orderBy, AnyTypeKind.USER); List<UserTO> result = matching.stream(). - map(user -> binder.returnUserTO(binder.getUserTO(user, details))). + map(user -> binder.getUserTO(user, details)). collect(Collectors.toList()); return Pair.of(count, result); @@ -189,8 +190,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { Pair<String, List<PropagationStatus>> created = provisioningManager.create(before.getLeft(), storePassword, nullPriorityAsync); - return afterCreate( - binder.returnUserTO(binder.getUserTO(created.getKey())), created.getRight(), before.getRight()); + return afterCreate(binder.getUserTO(created.getKey()), created.getRight(), before.getRight()); } @PreAuthorize("isAuthenticated() " @@ -246,7 +246,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { provisioningManager.update(before.getLeft(), nullPriorityAsync); ProvisioningResult<UserTO> result = afterUpdate( - binder.returnUserTO(binder.getUserTO(after.getLeft().getKey())), + binder.getUserTO(after.getLeft().getKey()), after.getRight(), before.getRight()); @@ -297,7 +297,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { Pair<String, List<PropagationStatus>> updated = setStatusOnWfAdapter(statusPatch, nullPriorityAsync); return afterUpdate( - binder.returnUserTO(binder.getUserTO(updated.getKey())), + binder.getUserTO(updated.getKey()), updated.getRight(), Collections.emptyList()); } @@ -308,7 +308,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { Pair<String, List<PropagationStatus>> updated = setStatusOnWfAdapter(statusPatch, nullPriorityAsync); return afterUpdate( - binder.returnUserTO(binder.getUserTO(updated.getKey())), + binder.getUserTO(updated.getKey()), updated.getRight(), Collections.emptyList()); } @@ -334,8 +334,9 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { } if (syncopeLogic.isPwdResetRequiringSecurityQuestions() - && (securityAnswer == null || !securityAnswer.equals(user.getSecurityAnswer()))) { - + && (securityAnswer == null + || !Encryptor.getInstance().verify(securityAnswer, user.getCipherAlgorithm(), + user.getSecurityAnswer()))) { throw SyncopeClientException.build(ClientExceptionType.InvalidSecurityAnswer); } @@ -399,7 +400,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { deletedTO = binder.getUserTO(before.getLeft().getKey()); } - return afterDelete(binder.returnUserTO(deletedTO), statuses, before.getRight()); + return afterDelete(deletedTO, statuses, before.getRight()); } protected void updateChecks(final String key) { @@ -428,7 +429,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { map(r -> new StringPatchItem.Builder().operation(PatchOperation.DELETE).value(r).build()). collect(Collectors.toList())); - return binder.returnUserTO(binder.getUserTO(provisioningManager.unlink(patch))); + return binder.getUserTO(provisioningManager.unlink(patch)); } @PreAuthorize("hasRole('" + StandardEntitlement.USER_UPDATE + "')") @@ -442,7 +443,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { map(r -> new StringPatchItem.Builder().operation(PatchOperation.ADD_REPLACE).value(r).build()). collect(Collectors.toList())); - return binder.returnUserTO(binder.getUserTO(provisioningManager.link(patch))); + return binder.getUserTO(provisioningManager.link(patch)); } @PreAuthorize("hasRole('" + StandardEntitlement.USER_UPDATE + "')") @@ -496,7 +497,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { List<PropagationStatus> statuses = provisioningManager.deprovision(key, resources, nullPriorityAsync); ProvisioningResult<UserTO> result = new ProvisioningResult<>(); - result.setEntity(binder.returnUserTO(binder.getUserTO(key))); + result.setEntity(binder.getUserTO(key)); result.getPropagationStatuses().addAll(statuses); return result; } @@ -516,7 +517,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserPatch> { nullPriorityAsync); ProvisioningResult<UserTO> result = new ProvisioningResult<>(); - result.setEntity(binder.returnUserTO(binder.getUserTO(key))); + result.setEntity(binder.getUserTO(key)); result.getPropagationStatuses().addAll(statuses); return result; } diff --git a/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/Account.java b/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/Account.java index bbfe93e..dc34148 100644 --- a/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/Account.java +++ b/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/Account.java @@ -28,14 +28,16 @@ public interface Account { CipherAlgorithm getCipherAlgorithm(); - boolean canDecodePassword(); + boolean canDecodeSecrets(); String getPassword(); void setEncodedPassword(String password, CipherAlgorithm cipherAlgoritm); - void setPassword(String password, CipherAlgorithm cipherAlgoritm); + void setPassword(String password); + void setCipherAlgorithm(CipherAlgorithm cipherAlgorithm); + Boolean isSuspended(); void setSuspended(Boolean suspended); diff --git a/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java b/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java index 458d415..8406759 100644 --- a/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java +++ b/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java @@ -54,7 +54,11 @@ public interface User extends Account, GroupableRelatable<User, UMembership, UPl void setSecurityQuestion(SecurityQuestion securityQuestion); String getSecurityAnswer(); + + String getClearSecurityAnswer(); + void setEncodedSecurityAnswer(String securityAnswer); + void setSecurityAnswer(String securityAnswer); Integer getFailedLogins(); diff --git a/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPALinkedAccount.java b/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPALinkedAccount.java index dbd47e8..ef48ffa 100644 --- a/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPALinkedAccount.java +++ b/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPALinkedAccount.java @@ -39,6 +39,7 @@ import javax.persistence.UniqueConstraint; import javax.validation.Valid; import javax.validation.constraints.NotNull; import org.apache.syncope.common.lib.types.CipherAlgorithm; +import org.apache.syncope.core.persistence.api.dao.ConfDAO; import org.apache.syncope.core.persistence.api.entity.Privilege; import org.apache.syncope.core.persistence.api.entity.resource.ExternalResource; import org.apache.syncope.core.persistence.api.entity.user.LAPlainAttr; @@ -47,6 +48,7 @@ import org.apache.syncope.core.persistence.api.entity.user.User; import org.apache.syncope.core.persistence.jpa.entity.AbstractGeneratedKeyEntity; import org.apache.syncope.core.persistence.jpa.entity.JPAPrivilege; import org.apache.syncope.core.persistence.jpa.entity.resource.JPAExternalResource; +import org.apache.syncope.core.spring.ApplicationContextProvider; import org.apache.syncope.core.spring.security.Encryptor; @Entity @@ -141,7 +143,16 @@ public class JPALinkedAccount extends AbstractGeneratedKeyEntity implements Link } @Override - public boolean canDecodePassword() { + public void setCipherAlgorithm(final CipherAlgorithm cipherAlgorithm) { + if (this.cipherAlgorithm == null || cipherAlgorithm == null) { + this.cipherAlgorithm = cipherAlgorithm; + } else { + throw new IllegalArgumentException("Cannot override existing cipher algorithm"); + } + } + + @Override + public boolean canDecodeSecrets() { return this.cipherAlgorithm != null && this.cipherAlgorithm.isInvertible(); } @@ -157,10 +168,12 @@ public class JPALinkedAccount extends AbstractGeneratedKeyEntity implements Link } @Override - public void setPassword(final String password, final CipherAlgorithm cipherAlgoritm) { + public void setPassword(final String password) { try { - this.password = ENCRYPTOR.encode(password, cipherAlgoritm); - this.cipherAlgorithm = cipherAlgoritm; + this.password = ENCRYPTOR.encode(password, cipherAlgorithm == null + ? CipherAlgorithm.valueOf(ApplicationContextProvider.getBeanFactory().getBean(ConfDAO.class). + find("password.cipher.algorithm", CipherAlgorithm.AES.name())) + : cipherAlgorithm); } catch (Exception e) { LOG.error("Could not encode password", e); this.password = null; diff --git a/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java b/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java index 3c93202..1e59265 100644 --- a/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java +++ b/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java @@ -47,6 +47,7 @@ import javax.persistence.UniqueConstraint; import javax.validation.Valid; import javax.validation.constraints.NotNull; import org.apache.syncope.common.lib.types.CipherAlgorithm; +import org.apache.syncope.core.persistence.api.dao.ConfDAO; import org.apache.syncope.core.persistence.api.entity.user.SecurityQuestion; import org.apache.syncope.core.persistence.api.entity.user.UPlainAttr; import org.apache.syncope.core.persistence.api.entity.user.User; @@ -185,6 +186,9 @@ public class JPAUser @Column(nullable = true) private String securityAnswer; + @Transient + private String clearSecurityAnswer; + @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true, mappedBy = "owner") @Valid private List<JPALinkedAccount> linkedAccounts = new ArrayList<>(); @@ -241,21 +245,23 @@ public class JPAUser } @Override - public void setEncodedPassword(final String password, final CipherAlgorithm cipherAlgoritm) { + public void setEncodedPassword(final String password, final CipherAlgorithm cipherAlgorithm) { this.clearPassword = null; this.password = password; - this.cipherAlgorithm = cipherAlgoritm; + this.cipherAlgorithm = cipherAlgorithm; setMustChangePassword(false); } @Override - public void setPassword(final String password, final CipherAlgorithm cipherAlgoritm) { + public void setPassword(final String password) { this.clearPassword = password; try { - this.password = ENCRYPTOR.encode(password, cipherAlgoritm); - this.cipherAlgorithm = cipherAlgoritm; + this.password = ENCRYPTOR.encode(password, cipherAlgorithm == null + ? CipherAlgorithm.valueOf(ApplicationContextProvider.getBeanFactory().getBean(ConfDAO.class). + find("password.cipher.algorithm", CipherAlgorithm.AES.name())) + : cipherAlgorithm); setMustChangePassword(false); } catch (Exception e) { LOG.error("Could not encode password", e); @@ -269,7 +275,16 @@ public class JPAUser } @Override - public boolean canDecodePassword() { + public void setCipherAlgorithm(final CipherAlgorithm cipherAlgorithm) { + if (this.cipherAlgorithm == null || cipherAlgorithm == null) { + this.cipherAlgorithm = cipherAlgorithm; + } else { + throw new IllegalArgumentException("Cannot override existing cipher algorithm"); + } + } + + @Override + public boolean canDecodeSecrets() { return this.cipherAlgorithm != null && this.cipherAlgorithm.isInvertible(); } @@ -425,8 +440,30 @@ public class JPAUser } @Override + public String getClearSecurityAnswer() { + return clearSecurityAnswer; + } + + @Override + public void setEncodedSecurityAnswer(final String securityAnswer) { + this.clearSecurityAnswer = null; + + this.securityAnswer = securityAnswer; + } + + @Override public void setSecurityAnswer(final String securityAnswer) { this.securityAnswer = securityAnswer; + + try { + this.securityAnswer = ENCRYPTOR.encode(securityAnswer, cipherAlgorithm == null + ? CipherAlgorithm.valueOf(ApplicationContextProvider.getBeanFactory().getBean(ConfDAO.class). + find("password.cipher.algorithm", CipherAlgorithm.AES.name())) + : cipherAlgorithm); + } catch (Exception e) { + LOG.error("Could not encode security answer", e); + this.securityAnswer = null; + } } @Override diff --git a/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/MultitenancyTest.java b/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/MultitenancyTest.java index 565b3b8..4f06a73 100644 --- a/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/MultitenancyTest.java +++ b/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/MultitenancyTest.java @@ -94,7 +94,8 @@ public class MultitenancyTest extends AbstractTest { User user = entityFactory.newEntity(User.class); user.setRealm(realmDAO.getRoot()); - user.setPassword("password", CipherAlgorithm.SHA256); + user.setPassword("password"); + user.setCipherAlgorithm(CipherAlgorithm.SHA256); user.setUsername("username"); User actual = userDAO.save(user); diff --git a/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/UserTest.java b/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/UserTest.java index a7272da..2b7e6e1 100644 --- a/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/UserTest.java +++ b/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/UserTest.java @@ -22,6 +22,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -33,11 +34,13 @@ import org.apache.syncope.core.persistence.api.attrvalue.validation.InvalidEntit import org.apache.syncope.core.persistence.api.dao.DerSchemaDAO; import org.apache.syncope.core.persistence.api.dao.ExternalResourceDAO; import org.apache.syncope.core.persistence.api.dao.PlainSchemaDAO; +import org.apache.syncope.core.persistence.api.dao.SecurityQuestionDAO; import org.apache.syncope.core.persistence.api.dao.UserDAO; import org.apache.syncope.core.persistence.api.entity.user.UPlainAttrValue; import org.apache.syncope.core.persistence.api.entity.user.User; import org.apache.syncope.core.persistence.jpa.AbstractTest; import org.apache.syncope.core.spring.policy.InvalidPasswordRuleConf; +import org.apache.syncope.core.spring.security.Encryptor; import org.apache.syncope.core.spring.security.PasswordGenerator; import org.apache.syncope.core.persistence.api.dao.RealmDAO; import org.apache.syncope.core.persistence.api.entity.PlainSchema; @@ -68,6 +71,9 @@ public class UserTest extends AbstractTest { @Autowired private DerSchemaDAO derSchemaDAO; + @Autowired + private SecurityQuestionDAO securityQuestionDAO; + @Test public void find() { User user = userDAO.find("823074dc-d280-436d-a7dd-07399fae48ec"); @@ -202,7 +208,8 @@ public class UserTest extends AbstractTest { user.setRealm(realmDAO.findByFullPath("/even/two")); user.setCreator("admin"); user.setCreationDate(new Date()); - user.setPassword("pass", CipherAlgorithm.SHA256); + user.setCipherAlgorithm(CipherAlgorithm.SHA256); + user.setPassword("pass"); try { userDAO.save(user); @@ -219,7 +226,8 @@ public class UserTest extends AbstractTest { user.setRealm(realmDAO.findByFullPath("/even/two")); user.setCreator("admin"); user.setCreationDate(new Date()); - user.setPassword("password123", CipherAlgorithm.SHA256); + user.setCipherAlgorithm(CipherAlgorithm.SHA256); + user.setPassword("password123"); try { userDAO.save(user); @@ -236,7 +244,8 @@ public class UserTest extends AbstractTest { user.setRealm(realmDAO.findByFullPath("/even/two")); user.setCreator("admin"); user.setCreationDate(new Date()); - user.setPassword("password123", CipherAlgorithm.SHA256); + user.setCipherAlgorithm(CipherAlgorithm.SHA256); + user.setPassword("password123"); User actual = userDAO.save(user); assertNotNull(actual); @@ -263,7 +272,8 @@ public class UserTest extends AbstractTest { user.setCreator("admin"); user.setCreationDate(new Date()); - user.setPassword("password123", CipherAlgorithm.AES); + user.setCipherAlgorithm(CipherAlgorithm.AES); + user.setPassword("password123"); User actual = userDAO.save(user); assertNotNull(actual); @@ -273,7 +283,8 @@ public class UserTest extends AbstractTest { public void issueSYNCOPE391() { User user = entityFactory.newEntity(User.class); user.setUsername("username"); - user.setPassword(null, CipherAlgorithm.AES); + user.setCipherAlgorithm(CipherAlgorithm.AES); + user.setPassword(null); user.setRealm(realmDAO.findByFullPath("/even/two")); User actual = userDAO.save(user); @@ -292,7 +303,45 @@ public class UserTest extends AbstractTest { assertNotNull(password); User user = userDAO.find("c9b2dec2-00a7-4855-97c0-d854842b4b24"); - user.setPassword(password, CipherAlgorithm.SHA); + user.setPassword(password); userDAO.save(user); } + + @Test + public void passwordGeneratorFailing() { + assertThrows(IllegalArgumentException.class, () -> { + String password = ""; + try { + password = passwordGenerator.generate(resourceDAO.find("ws-target-resource-nopropagation")); + } catch (InvalidPasswordRuleConf e) { + fail(e.getMessage()); + } + assertNotNull(password); + + User user = userDAO.find("c9b2dec2-00a7-4855-97c0-d854842b4b24"); + // SYNCOPE-1666 fail because cipherAlgorithm is already set + user.setCipherAlgorithm(CipherAlgorithm.SHA); + user.setPassword(password); + userDAO.save(user); + }); + } + + @Test + public void issueSYNCOPE1666() { + User user = entityFactory.newEntity(User.class); + user.setUsername("username"); + user.setRealm(realmDAO.findByFullPath("/even/two")); + user.setCreator("admin"); + user.setCreationDate(new Date()); + user.setCipherAlgorithm(CipherAlgorithm.SSHA256); + user.setPassword("password123"); + user.setSecurityQuestion(securityQuestionDAO.find("887028ea-66fc-41e7-b397-620d7ea6dfbb")); + String securityAnswer = "my complex answer to @ $complex question è ? £12345"; + user.setSecurityAnswer(securityAnswer); + + User actual = userDAO.save(user); + assertNotNull(actual); + assertNotNull(actual.getSecurityAnswer()); + assertTrue(Encryptor.getInstance().verify(securityAnswer, CipherAlgorithm.SSHA256, actual.getSecurityAnswer())); + } } diff --git a/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/outer/UserTest.java b/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/outer/UserTest.java index e3093ac..03e498d 100644 --- a/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/outer/UserTest.java +++ b/core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/outer/UserTest.java @@ -256,7 +256,8 @@ public class UserTest extends AbstractTest { account.add(applicationDAO.findPrivilege("getMighty")); account.setUsername(UUID.randomUUID().toString()); - account.setPassword("Password123", CipherAlgorithm.AES); + account.setCipherAlgorithm(CipherAlgorithm.AES); + account.setPassword("Password123"); AnyUtils anyUtils = anyUtilsFactory.getLinkedAccountInstance(); LAPlainAttr attr = anyUtils.newPlainAttr(); diff --git a/core/provisioning-api/src/main/java/org/apache/syncope/core/provisioning/api/data/UserDataBinder.java b/core/provisioning-api/src/main/java/org/apache/syncope/core/provisioning/api/data/UserDataBinder.java index 61bf2ff..6877eca 100644 --- a/core/provisioning-api/src/main/java/org/apache/syncope/core/provisioning/api/data/UserDataBinder.java +++ b/core/provisioning-api/src/main/java/org/apache/syncope/core/provisioning/api/data/UserDataBinder.java @@ -28,8 +28,6 @@ import org.apache.syncope.core.persistence.api.entity.user.User; public interface UserDataBinder { - UserTO returnUserTO(UserTO userTO); - UserTO getAuthenticatedUserTO(); UserTO getUserTO(String key); diff --git a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/MappingManagerImpl.java b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/MappingManagerImpl.java index 79b5d59..afaa464 100644 --- a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/MappingManagerImpl.java +++ b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/MappingManagerImpl.java @@ -497,7 +497,7 @@ public class MappingManagerImpl implements MappingManager { } else { if (StringUtils.isNotBlank(defaultValue)) { passwordAttrValue = defaultValue; - } else if (account.canDecodePassword()) { + } else if (account.canDecodeSecrets()) { passwordAttrValue = decodePassword(account); } else { passwordAttrValue = null; diff --git a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java index e116afd..058557d 100644 --- a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java +++ b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java @@ -50,6 +50,7 @@ import org.apache.syncope.core.persistence.api.dao.AccessTokenDAO; import org.apache.syncope.core.persistence.api.dao.ConfDAO; import org.apache.syncope.core.persistence.api.dao.SecurityQuestionDAO; import org.apache.syncope.core.persistence.api.entity.group.Group; +import org.apache.syncope.core.persistence.api.entity.user.Account; import org.apache.syncope.core.persistence.api.entity.user.SecurityQuestion; import org.apache.syncope.core.persistence.api.entity.user.User; import org.apache.syncope.core.provisioning.api.PropagationByResource; @@ -112,16 +113,6 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat @Transactional(readOnly = true) @Override - public UserTO returnUserTO(final UserTO userTO) { - if (!confDAO.find("return.password.value", false)) { - userTO.setPassword(null); - userTO.getLinkedAccounts().forEach(account -> account.setPassword(null)); - } - return userTO; - } - - @Transactional(readOnly = true) - @Override public UserTO getAuthenticatedUserTO() { UserTO authUserTO; @@ -144,17 +135,42 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat private void setPassword(final User user, final String password, final SyncopeClientCompositeException scce) { try { - String algorithm = confDAO.find("password.cipher.algorithm", CipherAlgorithm.AES.name()); - user.setPassword(password, CipherAlgorithm.valueOf(algorithm)); + setCipherAlgorithm(user); + user.setPassword(password); } catch (IllegalArgumentException e) { - SyncopeClientException invalidCiperAlgorithm = SyncopeClientException.build(ClientExceptionType.NotFound); - invalidCiperAlgorithm.getElements().add(e.getMessage()); - scce.addException(invalidCiperAlgorithm); + throw aggregateException(scce, e, ClientExceptionType.NotFound); + } + } - throw scce; + private void setSecurityAnswer( + final User user, + final String securityAnswer, + final SyncopeClientCompositeException scce) { + try { + setCipherAlgorithm(user); + user.setSecurityAnswer(securityAnswer); + } catch (IllegalArgumentException e) { + throw aggregateException(scce, e, ClientExceptionType.NotFound); } } + private void setCipherAlgorithm(final Account account) { + if (account.getCipherAlgorithm() == null) { + account.setCipherAlgorithm( + CipherAlgorithm.valueOf(confDAO.find("password.cipher.algorithm", CipherAlgorithm.AES.name()))); + } + } + + private RuntimeException aggregateException( + final SyncopeClientCompositeException scce, + final RuntimeException e, + final ClientExceptionType clientExceptionType) { + SyncopeClientException sce = SyncopeClientException.build(clientExceptionType); + sce.getElements().add(e.getMessage()); + scce.addException(sce); + return scce; + } + private void linkedAccount( final User user, final LinkedAccountTO accountTO, @@ -188,7 +204,8 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat if (StringUtils.isBlank(accountTO.getPassword())) { account.setEncodedPassword(null, null); } else if (!accountTO.getPassword().equals(account.getPassword())) { - account.setPassword(accountTO.getPassword(), CipherAlgorithm.AES); + setCipherAlgorithm(account); + account.setPassword(accountTO.getPassword()); } account.setSuspended(accountTO.isSuspended()); @@ -225,6 +242,28 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat } } + private LinkedAccountTO getLinkedAccountTO(final LinkedAccount account, final boolean returnPasswordValue) { + LinkedAccountTO accountTO = new LinkedAccountTO.Builder( + account.getKey(), account.getResource().getKey(), account.getConnObjectKeyValue()). + username(account.getUsername()). + suspended(BooleanUtils.isTrue(account.isSuspended())). + build(); + if (returnPasswordValue) { + accountTO.setPassword(account.getPassword()); + } + + account.getPlainAttrs().forEach(plainAttr -> { + accountTO.getPlainAttrs().add(new AttrTO.Builder(). + schema(plainAttr.getSchema().getKey()). + values(plainAttr.getValuesAsStrings()).build()); + }); + + accountTO.getPrivileges().addAll(account.getPrivileges().stream(). + map(Entity::getKey).collect(Collectors.toList())); + + return accountTO; + } + @Override public void create(final User user, final UserTO userTO, final boolean storePassword) { SyncopeClientCompositeException scce = SyncopeClientException.buildComposite(); @@ -249,7 +288,7 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat user.setSecurityQuestion(securityQuestion); } } - user.setSecurityAnswer(userTO.getSecurityAnswer()); + setSecurityAnswer(user, userTO.getSecurityAnswer(), scce); // roles userTO.getRoles().forEach(roleKey -> { @@ -440,7 +479,7 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat securityQuestionDAO.find(userPatch.getSecurityQuestion().getValue()); if (securityQuestion != null) { user.setSecurityQuestion(securityQuestion); - user.setSecurityAnswer(userPatch.getSecurityAnswer().getValue()); + setSecurityAnswer(user, userPatch.getSecurityAnswer().getValue(), scce); } } } @@ -594,7 +633,7 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat // SYNCOPE-686: if password is invertible and we are adding resources with password mapping, // ensure that they are counted for password propagation - if (toBeUpdated.canDecodePassword()) { + if (toBeUpdated.canDecodeSecrets()) { if (userPatch.getPassword() == null) { userPatch.setPassword(new PasswordPatch()); } @@ -681,32 +720,21 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat @Transactional(readOnly = true) @Override public LinkedAccountTO getLinkedAccountTO(final LinkedAccount account) { - LinkedAccountTO accountTO = new LinkedAccountTO.Builder( - account.getKey(), account.getResource().getKey(), account.getConnObjectKeyValue()). - username(account.getUsername()). - password(account.getPassword()). - suspended(BooleanUtils.isTrue(account.isSuspended())). - build(); - - account.getPlainAttrs().forEach(plainAttr -> { - accountTO.getPlainAttrs().add(new AttrTO.Builder(). - schema(plainAttr.getSchema().getKey()). - values(plainAttr.getValuesAsStrings()).build()); - }); - - accountTO.getPrivileges().addAll(account.getPrivileges().stream(). - map(Entity::getKey).collect(Collectors.toList())); - - return accountTO; + return getLinkedAccountTO(account, true); } @Transactional(readOnly = true) @Override public UserTO getUserTO(final User user, final boolean details) { + boolean returnPasswordValue = confDAO.find("return.password.value", false); + UserTO userTO = new UserTO(); userTO.setKey(user.getKey()); userTO.setUsername(user.getUsername()); - userTO.setPassword(user.getPassword()); + if (returnPasswordValue) { + userTO.setPassword(user.getPassword()); + userTO.setSecurityAnswer(user.getSecurityAnswer()); + } userTO.setType(user.getType().getKey()); userTO.setCreationDate(user.getCreationDate()); userTO.setCreator(user.getCreator()); @@ -756,20 +784,21 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat // memberships userTO.getMemberships().addAll( user.getMemberships().stream().map(membership -> getMembershipTO( - user.getPlainAttrs(membership), - derAttrHandler.getValues(user, membership), - virAttrHandler.getValues(user, membership), - membership)).collect(Collectors.toList())); + user.getPlainAttrs(membership), + derAttrHandler.getValues(user, membership), + virAttrHandler.getValues(user, membership), + membership)).collect(Collectors.toList())); // dynamic memberships userTO.getDynMemberships().addAll( userDAO.findDynGroups(user.getKey()).stream().map(group -> new MembershipTO.Builder(). - group(group.getKey(), group.getName()). - build()).collect(Collectors.toList())); + group(group.getKey(), group.getName()). + build()).collect(Collectors.toList())); // linked accounts userTO.getLinkedAccounts().addAll( - user.getLinkedAccounts().stream().map(this::getLinkedAccountTO).collect(Collectors.toList())); + user.getLinkedAccounts().stream().map(a -> getLinkedAccountTO(a, returnPasswordValue)) + .collect(Collectors.toList())); // delegations userTO.getDelegatingDelegations().addAll( diff --git a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/propagation/DeletingLinkedAccount.java b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/propagation/DeletingLinkedAccount.java index c96eebc..c456da7 100644 --- a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/propagation/DeletingLinkedAccount.java +++ b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/propagation/DeletingLinkedAccount.java @@ -108,7 +108,12 @@ public class DeletingLinkedAccount implements LinkedAccount { } @Override - public boolean canDecodePassword() { + public void setCipherAlgorithm(final CipherAlgorithm cipherAlgorithm) { + // unsupported + } + + @Override + public boolean canDecodeSecrets() { return false; } @@ -123,7 +128,7 @@ public class DeletingLinkedAccount implements LinkedAccount { } @Override - public void setPassword(final String password, final CipherAlgorithm cipherAlgoritm) { + public void setPassword(final String password) { // unsupported } diff --git a/core/provisioning-java/src/test/java/org/apache/syncope/core/provisioning/java/MappingManagerImplTest.java b/core/provisioning-java/src/test/java/org/apache/syncope/core/provisioning/java/MappingManagerImplTest.java index f18a116..527ace7 100644 --- a/core/provisioning-java/src/test/java/org/apache/syncope/core/provisioning/java/MappingManagerImplTest.java +++ b/core/provisioning-java/src/test/java/org/apache/syncope/core/provisioning/java/MappingManagerImplTest.java @@ -41,6 +41,7 @@ import org.identityconnectors.framework.common.objects.AttributeUtil; import org.identityconnectors.framework.common.objects.OperationalAttributes; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.util.ReflectionTestUtils; import org.springframework.transaction.annotation.Transactional; @Transactional("Master") @@ -116,7 +117,8 @@ public class MappingManagerImplTest extends AbstractTest { assertNull(AttributeUtil.getPasswordValue(attrs.getRight())); // 5. with no clear-text password, random password generation disabled but AES - bellini.setPassword("newPassword123", CipherAlgorithm.AES); + ReflectionTestUtils.setField(bellini, "cipherAlgorithm", CipherAlgorithm.AES); + bellini.setPassword("newPassword123"); userDAO.save(bellini); entityManager().flush(); @@ -142,7 +144,8 @@ public class MappingManagerImplTest extends AbstractTest { account.setResource(ldap); account.setOwner(vivaldi); account.setSuspended(false); - account.setPassword("Password321", CipherAlgorithm.AES); + account.setCipherAlgorithm(CipherAlgorithm.AES); + account.setPassword("Password321"); vivaldi.add(account); vivaldi = userDAO.save(vivaldi); diff --git a/core/spring/src/main/java/org/apache/syncope/core/spring/policy/DefaultPasswordRule.java b/core/spring/src/main/java/org/apache/syncope/core/spring/policy/DefaultPasswordRule.java index 4326075..cd8d60f 100644 --- a/core/spring/src/main/java/org/apache/syncope/core/spring/policy/DefaultPasswordRule.java +++ b/core/spring/src/main/java/org/apache/syncope/core/spring/policy/DefaultPasswordRule.java @@ -207,7 +207,7 @@ public class DefaultPasswordRule implements PasswordRule { if (account.getPassword() != null) { String clear = null; - if (account.canDecodePassword()) { + if (account.canDecodeSecrets()) { try { clear = ENCRYPTOR.decode(account.getPassword(), account.getCipherAlgorithm()); } catch (Exception e) { diff --git a/core/spring/src/main/java/org/apache/syncope/core/spring/policy/HaveIBeenPwnedPasswordRule.java b/core/spring/src/main/java/org/apache/syncope/core/spring/policy/HaveIBeenPwnedPasswordRule.java index 4a2bd31..a843b0c 100644 --- a/core/spring/src/main/java/org/apache/syncope/core/spring/policy/HaveIBeenPwnedPasswordRule.java +++ b/core/spring/src/main/java/org/apache/syncope/core/spring/policy/HaveIBeenPwnedPasswordRule.java @@ -109,7 +109,7 @@ public class HaveIBeenPwnedPasswordRule implements PasswordRule { public void enforce(final LinkedAccount account) { if (account.getPassword() != null) { String clear = null; - if (account.canDecodePassword()) { + if (account.canDecodeSecrets()) { try { clear = ENCRYPTOR.decode(account.getPassword(), account.getCipherAlgorithm()); } catch (Exception e) { diff --git a/ext/flowable/logic/src/main/java/org/apache/syncope/core/logic/UserRequestLogic.java b/ext/flowable/logic/src/main/java/org/apache/syncope/core/logic/UserRequestLogic.java index 507b5d9..16a75be 100644 --- a/ext/flowable/logic/src/main/java/org/apache/syncope/core/logic/UserRequestLogic.java +++ b/ext/flowable/logic/src/main/java/org/apache/syncope/core/logic/UserRequestLogic.java @@ -237,7 +237,7 @@ public class UserRequestLogic extends AbstractTransactionalLogic<EntityTO> { } else { userTO = binder.getUserTO(wfResult.getResult().getKey()); } - result.setEntity(binder.returnUserTO(userTO)); + result.setEntity(userTO); return result; } diff --git a/fit/core-reference/src/main/java/org/apache/syncope/fit/core/reference/TestPasswordRule.java b/fit/core-reference/src/main/java/org/apache/syncope/fit/core/reference/TestPasswordRule.java index df61508..2a3eb71 100644 --- a/fit/core-reference/src/main/java/org/apache/syncope/fit/core/reference/TestPasswordRule.java +++ b/fit/core-reference/src/main/java/org/apache/syncope/fit/core/reference/TestPasswordRule.java @@ -66,7 +66,7 @@ public class TestPasswordRule implements PasswordRule { public void enforce(final LinkedAccount account) { if (account.getPassword() != null) { String clear = null; - if (account.canDecodePassword()) { + if (account.canDecodeSecrets()) { try { clear = ENCRYPTOR.decode(account.getPassword(), account.getCipherAlgorithm()); } catch (Exception e) { diff --git a/src/main/asciidoc/reference-guide/workingwithapachesyncope/systemadministration/configurationparameters.adoc b/src/main/asciidoc/reference-guide/workingwithapachesyncope/systemadministration/configurationparameters.adoc index 5dedb84..d621d04 100644 --- a/src/main/asciidoc/reference-guide/workingwithapachesyncope/systemadministration/configurationparameters.adoc +++ b/src/main/asciidoc/reference-guide/workingwithapachesyncope/systemadministration/configurationparameters.adoc @@ -56,7 +56,8 @@ mechanism to work properly; [WARNING] Suspended Users are anyway not allowed to authenticate. * `log.lastlogindate` - whether the system updates the `lastLoginDate` field of users upon authentication; -* `return.password.value` - whether the hashed password value shall be returned when reading users; +* `return.password.value` - whether the hashed password value and the hashed security answer (if any) value shall be +returned when reading users; * `connector.test.timeout` - timeout (in seconds) to check connector connection in <<Admin Console>>; `0` to skip any check;