You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@syncope.apache.org by an...@apache.org on 2022/03/04 17:00:20 UTC
[syncope] branch master updated: [SYNCOPE-1666] added security answer hashing (#319) (#321)
This is an automated email from the ASF dual-hosted git repository.
andreapatricelli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/syncope.git
The following commit(s) were added to refs/heads/master by this push:
new b4a41e0 [SYNCOPE-1666] added security answer hashing (#319) (#321)
b4a41e0 is described below
commit b4a41e0e41f6c8c9b2ca1afc9ce7e19efd485bcf
Author: Andrea Patricelli <an...@apache.org>
AuthorDate: Fri Mar 4 18:00:12 2022 +0100
[SYNCOPE-1666] added security answer hashing (#319) (#321)
* [SYNCOPE-1666] added security answer hashing (#321)
---
.../org/apache/syncope/core/logic/UserLogic.java | 33 +++---
.../core/persistence/api/entity/user/Account.java | 6 +-
.../core/persistence/api/entity/user/User.java | 4 +
.../jpa/entity/user/JPALinkedAccount.java | 23 ++++-
.../core/persistence/jpa/entity/user/JPAUser.java | 52 ++++++++--
.../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 -
.../provisioning/java/DefaultMappingManager.java | 2 +-
.../provisioning/java/data/UserDataBinderImpl.java | 112 +++++++++++++--------
.../java/propagation/DeletingLinkedAccount.java | 9 +-
.../java/DefaultMappingManagerTest.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 +-
.../configuration/configurationparameters.adoc | 2 +-
18 files changed, 237 insertions(+), 90 deletions(-)
diff --git a/core/idrepo/logic/src/main/java/org/apache/syncope/core/logic/UserLogic.java b/core/idrepo/logic/src/main/java/org/apache/syncope/core/logic/UserLogic.java
index 29e475d..58e79bd 100644
--- a/core/idrepo/logic/src/main/java/org/apache/syncope/core/logic/UserLogic.java
+++ b/core/idrepo/logic/src/main/java/org/apache/syncope/core/logic/UserLogic.java
@@ -63,6 +63,7 @@ import org.apache.syncope.core.provisioning.api.serialization.POJOHelper;
import org.apache.syncope.core.provisioning.api.utils.RealmUtils;
import org.apache.syncope.core.provisioning.java.utils.TemplateUtils;
import org.apache.syncope.core.spring.security.AuthContextUtils;
+import org.apache.syncope.core.spring.security.Encryptor;
import org.springframework.security.access.prepost.PreAuthorize;
import org.springframework.transaction.annotation.Transactional;
@@ -124,15 +125,14 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
return Triple.of(
POJOHelper.serialize(AuthContextUtils.getAuthorizations()),
- POJOHelper.serialize(delegationDAO.findValidDelegating(authenticatedUser.getKey())),
- binder.returnUserTO(authenticatedUser));
+ POJOHelper.serialize(delegationDAO.findValidDelegating(authenticatedUser.getKey())), authenticatedUser);
}
@PreAuthorize("hasRole('" + IdRepoEntitlement.USER_READ + "')")
@Transactional(readOnly = true)
@Override
public UserTO read(final String key) {
- return binder.returnUserTO(binder.getUserTO(key));
+ return binder.getUserTO(key);
}
@PreAuthorize("hasRole('" + IdRepoEntitlement.USER_SEARCH + "')")
@@ -153,7 +153,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
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);
@@ -196,8 +196,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
Pair<String, List<PropagationStatus>> created = provisioningManager.create(
before.getLeft(), nullPriorityAsync, AuthContextUtils.getUsername(), REST_CONTEXT);
- return afterCreate(
- binder.returnUserTO(binder.getUserTO(created.getKey())), created.getRight(), before.getRight());
+ return afterCreate(binder.getUserTO(created.getKey()), created.getRight(), before.getRight());
}
@PreAuthorize("isAuthenticated() "
@@ -261,7 +260,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
before.getLeft(), nullPriorityAsync, AuthContextUtils.getUsername(), REST_CONTEXT);
ProvisioningResult<UserTO> result = afterUpdate(
- binder.returnUserTO(binder.getUserTO(after.getLeft().getKey())),
+ binder.getUserTO(after.getLeft().getKey()),
after.getRight(),
before.getRight());
@@ -315,7 +314,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
Pair<String, List<PropagationStatus>> updated = setStatusOnWfAdapter(statusR, nullPriorityAsync);
return afterUpdate(
- binder.returnUserTO(binder.getUserTO(updated.getKey())),
+ binder.getUserTO(updated.getKey()),
updated.getRight(),
List.of());
}
@@ -326,7 +325,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
Pair<String, List<PropagationStatus>> updated = setStatusOnWfAdapter(statusR, nullPriorityAsync);
return afterUpdate(
- binder.returnUserTO(binder.getUserTO(updated.getKey())),
+ binder.getUserTO(updated.getKey()),
updated.getRight(),
List.of());
}
@@ -352,7 +351,9 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
}
if (syncopeLogic.isPwdResetRequiringSecurityQuestions()
- && (securityAnswer == null || !securityAnswer.equals(user.getSecurityAnswer()))) {
+ && (securityAnswer == null
+ || !Encryptor.getInstance().verify(securityAnswer, user.getCipherAlgorithm(),
+ user.getSecurityAnswer()))) {
throw SyncopeClientException.build(ClientExceptionType.InvalidSecurityAnswer);
}
@@ -419,7 +420,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
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) {
@@ -448,8 +449,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
map(r -> new StringPatchItem.Builder().operation(PatchOperation.DELETE).value(r).build()).
collect(Collectors.toList()));
- return binder.returnUserTO(binder.getUserTO(
- provisioningManager.unlink(req, AuthContextUtils.getUsername(), REST_CONTEXT)));
+ return binder.getUserTO(provisioningManager.unlink(req, AuthContextUtils.getUsername(), REST_CONTEXT));
}
@PreAuthorize("hasRole('" + IdRepoEntitlement.USER_UPDATE + "')")
@@ -463,8 +463,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
map(r -> new StringPatchItem.Builder().operation(PatchOperation.ADD_REPLACE).value(r).build()).
collect(Collectors.toList()));
- return binder.returnUserTO(binder.getUserTO(
- provisioningManager.link(req, AuthContextUtils.getUsername(), REST_CONTEXT)));
+ return binder.getUserTO(provisioningManager.link(req, AuthContextUtils.getUsername(), REST_CONTEXT));
}
@PreAuthorize("hasRole('" + IdRepoEntitlement.USER_UPDATE + "')")
@@ -519,7 +518,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
key, resources, nullPriorityAsync, AuthContextUtils.getUsername(), REST_CONTEXT);
ProvisioningResult<UserTO> result = new ProvisioningResult<>();
- result.setEntity(binder.returnUserTO(binder.getUserTO(key)));
+ result.setEntity(binder.getUserTO(key));
result.getPropagationStatuses().addAll(statuses);
return result;
}
@@ -539,7 +538,7 @@ public class UserLogic extends AbstractAnyLogic<UserTO, UserCR, UserUR> {
key, changePwd, password, resources, nullPriorityAsync, AuthContextUtils.getUsername(), REST_CONTEXT);
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..cd5d894 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
@@ -38,6 +38,7 @@ import javax.persistence.Table;
import javax.persistence.UniqueConstraint;
import javax.validation.Valid;
import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.keymaster.client.api.ConfParamOps;
import org.apache.syncope.common.lib.types.CipherAlgorithm;
import org.apache.syncope.core.persistence.api.entity.Privilege;
import org.apache.syncope.core.persistence.api.entity.resource.ExternalResource;
@@ -47,6 +48,8 @@ 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.AuthContextUtils;
import org.apache.syncope.core.spring.security.Encryptor;
@Entity
@@ -141,7 +144,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 +169,13 @@ 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(ConfParamOps.class).
+ get(AuthContextUtils.getDomain(), "password.cipher.algorithm", CipherAlgorithm.AES.name(),
+ String.class))
+ : 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 d1692ed..655e64a 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
@@ -46,11 +46,13 @@ import javax.persistence.Transient;
import javax.persistence.UniqueConstraint;
import javax.validation.Valid;
import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.keymaster.client.api.ConfParamOps;
import org.apache.syncope.common.lib.types.CipherAlgorithm;
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;
import org.apache.syncope.core.persistence.jpa.entity.resource.JPAExternalResource;
+import org.apache.syncope.core.spring.security.AuthContextUtils;
import org.apache.syncope.core.spring.security.Encryptor;
import org.apache.syncope.core.spring.security.SecureRandomUtils;
import org.apache.syncope.core.spring.ApplicationContextProvider;
@@ -185,6 +187,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 +246,24 @@ 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(ConfParamOps.class).
+ get(AuthContextUtils.getDomain(), "password.cipher.algorithm", CipherAlgorithm.AES.name(),
+ String.class))
+ : cipherAlgorithm);
setMustChangePassword(false);
} catch (Exception e) {
LOG.error("Could not encode password", e);
@@ -269,7 +277,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();
}
@@ -414,8 +431,31 @@ 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(ConfParamOps.class).
+ get(AuthContextUtils.getDomain(), "password.cipher.algorithm", CipherAlgorithm.AES.name(),
+ String.class))
+ : 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 0fc1ea8..d214f6a 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 71f72b0..fa91f98 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 c0b1d9b..c7721f9 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
@@ -255,7 +255,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 81e3471..b364f74 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
@@ -29,8 +29,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/DefaultMappingManager.java b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/DefaultMappingManager.java
index 83fe8b0..e69fa4a 100644
--- a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/DefaultMappingManager.java
+++ b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/DefaultMappingManager.java
@@ -509,7 +509,7 @@ public class DefaultMappingManager 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 f81e8ac..d5a8b36 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
@@ -52,6 +52,7 @@ import org.apache.syncope.core.persistence.api.dao.AnyTypeClassDAO;
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.resource.Item;
+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;
@@ -169,16 +170,6 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat
@Transactional(readOnly = true)
@Override
- public UserTO returnUserTO(final UserTO userTO) {
- if (!confParamOps.get(AuthContextUtils.getDomain(), "return.password.value", false, Boolean.class)) {
- userTO.setPassword(null);
- userTO.getLinkedAccounts().forEach(account -> account.setPassword(null));
- }
- return userTO;
- }
-
- @Transactional(readOnly = true)
- @Override
public UserTO getAuthenticatedUserTO() {
UserTO authUserTO;
@@ -201,18 +192,42 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat
private void setPassword(final User user, final String password, final SyncopeClientCompositeException scce) {
try {
- String algorithm = confParamOps.get(AuthContextUtils.getDomain(),
- "password.cipher.algorithm", CipherAlgorithm.AES.name(), String.class);
- 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(confParamOps.get(AuthContextUtils.getDomain(),
+ "password.cipher.algorithm", CipherAlgorithm.AES.name(), String.class)));
}
}
+ 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,
@@ -246,7 +261,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());
@@ -283,6 +299,26 @@ 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()).
+ password(account.getPassword()).
+ suspended(BooleanUtils.isTrue(account.isSuspended())).
+ build();
+
+ account.getPlainAttrs().forEach(plainAttr -> {
+ accountTO.getPlainAttrs().add(
+ new Attr.Builder(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 UserCR userCR) {
SyncopeClientCompositeException scce = SyncopeClientException.buildComposite();
@@ -307,7 +343,7 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat
user.setSecurityQuestion(securityQuestion);
}
}
- user.setSecurityAnswer(userCR.getSecurityAnswer());
+ setSecurityAnswer(user, userCR.getSecurityAnswer(), scce);
// roles
userCR.getRoles().forEach(roleKey -> {
@@ -498,7 +534,7 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat
securityQuestionDAO.find(userUR.getSecurityQuestion().getValue());
if (securityQuestion != null) {
user.setSecurityQuestion(securityQuestion);
- user.setSecurityAnswer(userUR.getSecurityAnswer().getValue());
+ setSecurityAnswer(user, userUR.getSecurityAnswer().getValue(), scce);
}
}
}
@@ -652,7 +688,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 (userUR.getPassword() == null) {
userUR.setPassword(new PasswordPatch());
}
@@ -739,30 +775,24 @@ 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 Attr.Builder(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) {
- UserTO userTO = new UserTO();
+ Boolean returnPasswordValue = confParamOps.get(AuthContextUtils.getDomain(),
+ "return.password.value", Boolean.FALSE, Boolean.class);
+ UserTO userTO = new UserTO();
+ userTO.setKey(user.getKey());
+ userTO.setUsername(user.getUsername());
+ if (returnPasswordValue) {
+ userTO.setPassword(user.getPassword());
+ userTO.setSecurityAnswer(user.getSecurityAnswer());
+ }
+ userTO.setType(user.getType().getKey());
+ userTO.setCreationDate(user.getCreationDate());
userTO.setCreator(user.getCreator());
userTO.setCreationDate(user.getCreationDate());
userTO.setCreationContext(user.getCreationContext());
@@ -778,7 +808,6 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat
userTO.setKey(user.getKey());
userTO.setUsername(user.getUsername());
- userTO.setPassword(user.getPassword());
userTO.setType(user.getType().getKey());
userTO.setStatus(user.getStatus());
userTO.setSuspended(BooleanUtils.isTrue(user.isSuspended()));
@@ -831,7 +860,8 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements UserDat
// 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 3c9674a..ebc7cbd 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
@@ -107,7 +107,12 @@ public class DeletingLinkedAccount implements LinkedAccount {
}
@Override
- public boolean canDecodePassword() {
+ public void setCipherAlgorithm(final CipherAlgorithm cipherAlgorithm) {
+ // unsupported
+ }
+
+ @Override
+ public boolean canDecodeSecrets() {
return false;
}
@@ -122,7 +127,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/DefaultMappingManagerTest.java b/core/provisioning-java/src/test/java/org/apache/syncope/core/provisioning/java/DefaultMappingManagerTest.java
index c9363bd..920b4b1 100644
--- a/core/provisioning-java/src/test/java/org/apache/syncope/core/provisioning/java/DefaultMappingManagerTest.java
+++ b/core/provisioning-java/src/test/java/org/apache/syncope/core/provisioning/java/DefaultMappingManagerTest.java
@@ -50,6 +50,7 @@ import org.identityconnectors.framework.common.objects.Attribute;
import org.identityconnectors.framework.common.objects.AttributeUtil;
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;
import org.identityconnectors.framework.common.objects.OperationalAttributes;
@@ -141,7 +142,8 @@ public class DefaultMappingManagerTest 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();
@@ -167,7 +169,8 @@ public class DefaultMappingManagerTest extends AbstractTest {
account.setResource(ldap);
account.setOwner(vivaldi);
account.setSuspended(Boolean.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 70cc770..1bc5af6 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 188f321..bbe0ccb 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
@@ -245,7 +245,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/configuration/configurationparameters.adoc b/src/main/asciidoc/reference-guide/configuration/configurationparameters.adoc
index 98d268f..474a176 100644
--- a/src/main/asciidoc/reference-guide/configuration/configurationparameters.adoc
+++ b/src/main/asciidoc/reference-guide/configuration/configurationparameters.adoc
@@ -55,7 +55,7 @@ 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
* `connector.test.timeout` - timeout (in seconds) to check connector connection in <<Admin Console>>;
`0` to skip any check;