ilgrosso commented on a change in pull request #319:
URL: https://github.com/apache/syncope/pull/319#discussion_r818370864
##########
File path:
core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java
##########
@@ -54,8 +55,14 @@
void setSecurityQuestion(SecurityQuestion securityQuestion);
String getSecurityAnswer();
+
+ String getClearSecurityAnswer();
- void setSecurityAnswer(String securityAnswer);
+ void setEncodedSecurityAnswer(String securityAnswer);
+
+ void setSecurityAnswer(String securityAnswer, CipherAlgorithm
cipherAlgoritm);
Review comment:
This method would allow to set a different `cipherAlgorithm` for the
user than the one set for password.
The risk is that `user#getCipherAlgorith` can return the wrong value when
used to check password or security question values via `Encryptor#verify`.
I think we should change `User` interface to:
1. remove `cipherAlgorithm` param from `setPassword()` and
`setSecurityAnswer()` methods
1. add a separate `setCipherAlgorithm()` method, failing if
`cipherAlgorithm` is already set, unless the provided parameter is `null`
1. have `setPassword()` and `setSecurityAnswer()` to internally set
`cipherAlgorithm` to the value from conf
##########
File path:
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java
##########
@@ -756,20 +775,21 @@ 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.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()).
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
##########
@@ -681,32 +711,21 @@ private boolean isPasswordMapped(final ExternalResource
resource) {
@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);
Review comment:
Is there any call for `getLinkedAccountTO(account, false)`?
##########
File path:
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java
##########
@@ -756,20 +775,21 @@ 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.getPlainAttrs(membership),
Review comment:
please revert this formatting-only change
##########
File path:
ext/flowable/logic/src/main/java/org/apache/syncope/core/logic/UserRequestLogic.java
##########
@@ -126,7 +126,7 @@ public UserRequest startRequest(
protected void securityChecks(final String username, final String
entitlement, final String errorMessage) {
if (!AuthContextUtils.getUsername().equals(username)
&& !AuthContextUtils.getAuthorities().stream().
- anyMatch(auth ->
entitlement.equals(auth.getAuthority()))) {
+ anyMatch(auth -> entitlement.equals(auth.getAuthority()))) {
Review comment:
please revert this formatting-only change
##########
File path:
core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java
##########
@@ -54,8 +55,14 @@
void setSecurityQuestion(SecurityQuestion securityQuestion);
String getSecurityAnswer();
+
+ String getClearSecurityAnswer();
- void setSecurityAnswer(String securityAnswer);
+ void setEncodedSecurityAnswer(String securityAnswer);
+
+ void setSecurityAnswer(String securityAnswer, CipherAlgorithm
cipherAlgoritm);
+
+ boolean canDecodeSecurityAnswer();
Review comment:
Is this method really needed? Isn't just a copy of `canDecodePassword()`?
We might unify the two methods in one `canDecodeSecrets()`
--
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]
