ilgrosso commented on a change in pull request #319:
URL: https://github.com/apache/syncope/pull/319#discussion_r818702717
##########
File path:
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPALinkedAccount.java
##########
@@ -157,10 +166,9 @@ public void setEncodedPassword(final String password,
final CipherAlgorithm ciph
}
@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, this.cipherAlgorithm);
Review comment:
What happens if `this.cipherAlgorithm` is `null`? it should take the
default value from conf IIRC
##########
File path:
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java
##########
@@ -241,21 +244,20 @@ public void removeClearPassword() {
}
@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);
Review comment:
What happens if `this.cipherAlgorithm` is `null`? it should take the
default value from conf IIRC
##########
File path:
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/MappingManagerImpl.java
##########
@@ -479,6 +479,7 @@ private String getIntValue(final Realm realm, final Item
orgUnitItem) {
protected String decodePassword(final Account account) {
try {
+ LOG.info("Decode password {} in {}", account.getPassword(),
account.getCipherAlgorithm());
Review comment:
Avoid logging secrets, even if encoded
##########
File path:
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java
##########
@@ -755,21 +783,20 @@ public UserTO getUserTO(final User user, final boolean
details) {
// 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.getMemberships().stream().map(membership ->
getMembershipTO(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()));
Review comment:
Please revert this formatting-only change
##########
File path:
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java
##########
@@ -144,17 +135,42 @@ public UserTO getAuthenticatedUserTO() {
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(
Review comment:
how many methods are calling this new one? If it's just one, remove the
new method and place its statements in the caller
##########
File path:
core/persistence-jpa/src/test/java/org/apache/syncope/core/persistence/jpa/inner/UserTest.java
##########
@@ -292,7 +303,45 @@ public void testPasswordGenerator() {
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 testPasswordGeneratorFailing() {
Review comment:
Please avoid naming test methods as `test...`
##########
File path:
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java
##########
@@ -755,21 +783,20 @@ public UserTO getUserTO(final User user, final boolean
details) {
// 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.getMemberships().stream().map(membership ->
getMembershipTO(user.getPlainAttrs(membership),
Review comment:
Please revert this formatting-only change
##########
File path:
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java
##########
@@ -424,9 +435,28 @@ public String getSecurityAnswer() {
return securityAnswer;
}
+ @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);
Review comment:
What happens if `this.cipherAlgorithm` is `null`? it should take the
default value from conf IIRC
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
