[GitHub] [james-project] vttranlina commented on a change in pull request #753: JAMES-3674 : Support password salting and hash scheme upgrading

[GitBox <gi...@apache.org>]

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: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org