vttranlina commented on a change in pull request #753:
URL: https://github.com/apache/james-project/pull/753#discussion_r751982329
##########
File path:
server/data/data-library/src/main/java/org/apache/james/user/lib/model/DefaultUser.java
##########
@@ -86,7 +87,8 @@ public boolean verifyPassword(String pass) {
@Override
public boolean setPassword(String newPass) {
try {
- hashedPassword = DigestUtil.digestString(newPass, algorithm);
+ String newCredentials = algorithm.isSalted() ? userName.asString()
+ newPass : newPass;
Review comment:
I don't think when `salt` is a `username` value will make sense. I
thought it should be a random string, that only the server site knows.
##########
File path:
server/data/data-library/src/main/java/org/apache/james/user/lib/model/DefaultUser.java
##########
@@ -76,7 +76,8 @@ public Username getUserName() {
@Override
public boolean verifyPassword(String pass) {
try {
- String hashGuess = DigestUtil.digestString(pass, algorithm);
+ String credentials = algorithm.isSalted() ? userName.asString() +
pass : pass;
Review comment:
I like `Ternary Operator` too, but I don't see it in James before
##########
File path: server/data/data-memory/pom.xml
##########
@@ -93,6 +93,11 @@
<artifactId>cucumber-picocontainer</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.awaitility</groupId>
Review comment:
I don't see where using it?
##########
File path:
server/data/data-cassandra/src/main/java/org/apache/james/user/cassandra/CassandraUsersDAO.java
##########
@@ -185,7 +183,7 @@ public int countUsers() {
@Override
public void addUser(Username username, String password) throws
UsersRepositoryException {
- DefaultUser user = new DefaultUser(username,
algorithmFactory.of(DEFAULT_ALGO_VALUE));
+ DefaultUser user = new DefaultUser(username,
Algorithm.of(DEFAULT_ALGO_VALUE));
Review comment:
Should we move `Algorithm.of(DEFAULT_ALGO_VALUE)` to outside and make it
is a static final variable?
##########
File path:
server/data/data-library/src/test/java/org/apache/james/user/lib/model/DefaultUserTest.java
##########
@@ -0,0 +1,41 @@
+package org.apache.james.user.lib.model;
+
+import org.apache.james.core.Username;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class DefaultUserTest {
+
+ private DefaultUser user;
+
+ @Before
+ public void setUp() {
+ user = new DefaultUser(
+ Username.of("joe"),
+ "5en6G6MezRroT3XKqkdPOmY/", // SHA-1 legacy hash of "secret"
+ Algorithm.of("SHA-1", "legacy"),
+ Algorithm.of("SHA-256", "plain"));
+ }
+
+ @Test
+ public void shouldYieldVerifyAlgorithm() {
+ assertThat(user.getHashAlgorithm().asString()).isEqualTo("SHA-1");
+
assertThat(user.getHashAlgorithm().getHashingMode()).isEqualTo("LEGACY");
+ }
+
+ @Test
+ public void shouldVerifyPasswordUsingVerifyAlgorithm() {
+ assertThat(user.verifyPassword("secret")).isTrue();
+ }
+
+ @Test
+ public void shouldSetPasswordUsingUpdateAlgorithm() {
+ user.setPassword("secret2");
+ assertThat(user.getHashAlgorithm().asString()).isEqualTo("SHA-256");
+
assertThat(user.getHashAlgorithm().getHashingMode()).isEqualTo("PLAIN");
+
+ assertThat(user.verifyPassword("secret2")).isTrue();
Review comment:
+ `assertThat(user.verifyPassword("secret")).isFalse();`
// not really necessary
##########
File path: server/data/data-cassandra/pom.xml
##########
@@ -119,6 +119,11 @@
<groupId>org.apache.commons</groupId>
<artifactId>commons-configuration2</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.awaitility</groupId>
Review comment:
I don't see where using it?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
