feat: Implement an upgrade plugin to update passwords hashing algorithm - EXO-65151 - Meeds-io/MIPs#69 #161

hakermi · 2023-07-26T16:18:10Z

Prior to this change, As a part of the upgrade mechanism of old hashed passwords we need to create an UP to re-hash old passwords by the new hash algo. This PR creates an UP which has to update the old passwords hash by the new algo by re-hashing and adding the needed salt attribute.

data-upgrade-users/src/main/java/org/exoplatform/migration/UserPasswordHashMigration.java

rdenarie · 2023-07-27T10:20:04Z

data-upgrade-users/src/main/java/org/exoplatform/migration/UserPasswordHashMigration.java

+
+    @Override
+    public void afterUpgrade() {
+        settingService.set(Context.GLOBAL.id(USER_PASSWORD_HASH_MIGRATION),


This should be set only if success.

In your case, if there are 10k users to update, and if I stop the server before the end, it will set it as finished. but it is not

I didn't understand, if you stop the server this will not be called, thus i added it

I thought that afterUpgrade is called, even the upgrade is not finished or failed.

Other question : imagine that during the execution, there is a problem for some users (not all).

Then the execution finished.
Does the migration is considered as finished or not ?
Does it will restart at next startup ?

In addition, you configure upgradePlugin as executed once, so why do we need to store a setting to know if the execution is finished ?

In fact, in other upgrade plugin, there are 2 option

UP finish with an exeception : so it is considered as not executed and will relaunched at next startup

UP finish without exception : so it is considered as executed, and will not relaunched at next startup
With this assumption, do we really need a setting to mark it as finished ?

I thought that afterUpgrade is called, even the upgrade is not finished or failed.

Other question : imagine that during the execution, there is a problem for some users (not all).

Then the execution finished. Does the migration is considered as finished or not ? Does it will restart at next startup ?

In addition, you configure upgradePlugin as executed once, so why do we need to store a setting to know if the execution is finished ?

In fact, in other upgrade plugin, there are 2 option

UP finish with an exeception : so it is considered as not executed and will relaunched at next startup

UP finish without exception : so it is considered as executed, and will not relaunched at next startup
With this assumption, do we really need a setting to mark it as finished ?

@rdenarie I hope this replies to your comments
https://github.com/exoplatform/data-upgrade/pull/161/files#diff-1a9349cb98114358fe6ed02277b76ad326d706337cec3efe4e732b4f6ed39b41R119-R128

no sure. Need to speak about that. Can you ping me ?

data-upgrade-users/src/test/java/org/exoplatform/migration/UserPasswordHashMigrationTest.java

rdenarie · 2023-07-27T10:21:46Z

data-upgrade-users/src/test/java/org/exoplatform/migration/UserPasswordHashMigrationTest.java

+    proceedToUpgrade = userPasswordHashMigration.shouldProceedToUpgrade(null, null, null);
+    assertFalse(proceedToUpgrade);
+  }
+}


can you add a test with an error during process upgrade, to check that the upgrade correctly restart ?

already exists by not calling the afterUpgrade, the shouldProceedToUpgrade is true, so when inttrepring server start or stop the server during the UP execution , the afterUpgrade will not be called, so the next restart the UP will be launched again .
https://github.com/exoplatform/data-upgrade/pull/161/files#diff-1a9349cb98114358fe6ed02277b76ad326d706337cec3efe4e732b4f6ed39b41R100

In fact, my test would be

boolean upgrade = processUpgrade
//with mock find a way to fail it
assertFalse(upgrade)

//restart processUpgrade
assert(process upgrade correctly restart and finish)

rdenarie · 2023-07-27T12:24:07Z

data-upgrade-users/src/test/java/org/exoplatform/migration/UserPasswordHashMigrationTest.java

+    proceedToUpgrade = userPasswordHashMigration.shouldProceedToUpgrade(null, null, null);
+    assertFalse(proceedToUpgrade);
+  }
+}


In fact, my test would be

boolean upgrade = processUpgrade
//with mock find a way to fail it
assertFalse(upgrade)

//restart processUpgrade
assert(process upgrade correctly restart and finish)

rdenarie · 2023-07-27T12:27:59Z

data-upgrade-users/src/main/java/org/exoplatform/migration/UserPasswordHashMigration.java

+
+    @Override
+    public void afterUpgrade() {
+        settingService.set(Context.GLOBAL.id(USER_PASSWORD_HASH_MIGRATION),


I thought that afterUpgrade is called, even the upgrade is not finished or failed.

Other question : imagine that during the execution, there is a problem for some users (not all).

Then the execution finished.
Does the migration is considered as finished or not ?
Does it will restart at next startup ?

In addition, you configure upgradePlugin as executed once, so why do we need to store a setting to know if the execution is finished ?

In fact, in other upgrade plugin, there are 2 option

UP finish with an exeception : so it is considered as not executed and will relaunched at next startup

UP finish without exception : so it is considered as executed, and will not relaunched at next startup
With this assumption, do we really need a setting to mark it as finished ?

hakermi · 2023-07-28T09:23:01Z

@rdenarie PR updated

rdenarie

Can you also fix sonar feedbacks.

rdenarie · 2023-07-28T09:49:59Z

data-upgrade-users/src/main/java/org/exoplatform/migration/UserPasswordHashMigration.java

+
+  private String getDefaultCredentialEncoderName() {
+    Map<String, String> props = new HashMap<>();
+    picketLinkIDMService.getConfigMD().getRealms().get(0).getOptions().forEach((k, v) -> props.put(k, v.get(0)));


Can you explain this line ?
There is twice get(0), are we ok with the fact that there is only one realm ?
and that the option we search is really in first position ?

In addition, instead of creating a map<String, String>, could we use a filter/map operation to directly have the correct value to return ?

It is the only way to do that, and it is one realm so it is in the first position,
if you have another way could you suggest it ?

and i don't see how to use filter/map here and what difference will make.
Do you know that options is a map and not a list or array ? beside that the value of the map is another list.

There is a solution to get default credential with proper way and is to declare a public function: ExtendedAttributeManager.getDefaultCrdentialEncoder().
inside the function we will just add : return getCredentialEncoder();

I didn't do that because i will need this function only in the UP

I think that getDefaultCredential is clearer, and safer.
WDYT ?

OK i will do it.

hakermi · 2023-07-28T10:33:53Z

Can you also fix sonar feedbacks.

Done

exo-swf · 2023-07-28T10:36:03Z

Your PR triggers too many exo-ci builds! Please finish your work and then, set your PR ready! Thank you

…hm - EXO-65151 - Meeds-io/MIPs#69 Prior to this change, As a part of the upgrade mechanism of old hashed passwords we need to create an UP to re-hash old passwords by the new hash algo. This PR creates an UP which has to update the old passwords hash by the new algo by re-hashing and adding the needed salt attribute.

sonarcloud · 2023-07-31T14:22:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

92.2% Coverage
0.0% Duplication

…hm - EXO-65151 - Meeds-io/MIPs#69 (#161) Prior to this change, As a part of the upgrade mechanism of old hashed passwords we need to create an UP to re-hash old passwords by the new hash algo. This PR creates an UP which has to update the old passwords hash by the new algo by re-hashing and adding the needed salt attribute.

hakermi marked this pull request as draft July 27, 2023 07:58

hakermi force-pushed the TASK-65151 branch 2 times, most recently from def7e0d to 1ee0c79 Compare July 27, 2023 09:10

hakermi marked this pull request as ready for review July 27, 2023 09:20

hakermi requested a review from rdenarie July 27, 2023 09:27

rdenarie requested changes Jul 27, 2023

View reviewed changes

hakermi force-pushed the TASK-65151 branch from 1ee0c79 to 7a8697c Compare July 27, 2023 11:26

hakermi requested a review from rdenarie July 27, 2023 11:38

rdenarie requested changes Jul 27, 2023

View reviewed changes

hakermi force-pushed the TASK-65151 branch from 7a8697c to 64d13b7 Compare July 27, 2023 13:19

hakermi requested a review from rdenarie July 27, 2023 13:21

hakermi force-pushed the TASK-65151 branch 2 times, most recently from 4065a47 to d909a19 Compare July 28, 2023 09:07

rdenarie requested changes Jul 28, 2023

View reviewed changes

hakermi requested a review from rdenarie July 28, 2023 10:32

hakermi force-pushed the TASK-65151 branch from d909a19 to d777645 Compare July 28, 2023 10:33

exo-swf marked this pull request as draft July 28, 2023 10:36

hakermi marked this pull request as ready for review July 28, 2023 10:36

hakermi force-pushed the TASK-65151 branch 3 times, most recently from baf47f4 to f581d94 Compare July 28, 2023 17:19

hakermi force-pushed the TASK-65151 branch from f581d94 to 48c41c4 Compare July 31, 2023 10:14

rdenarie approved these changes Jul 31, 2023

View reviewed changes

hakermi merged commit 30d3070 into feature/devx Jul 31, 2023
2 checks passed

hakermi deleted the TASK-65151 branch July 31, 2023 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement an upgrade plugin to update passwords hashing algorithm - EXO-65151 - Meeds-io/MIPs#69 #161

feat: Implement an upgrade plugin to update passwords hashing algorithm - EXO-65151 - Meeds-io/MIPs#69 #161

hakermi commented Jul 26, 2023

rdenarie Jul 27, 2023

hakermi Jul 27, 2023

rdenarie Jul 27, 2023

hakermi Jul 27, 2023

rdenarie Jul 27, 2023

rdenarie Jul 27, 2023

hakermi Jul 27, 2023

rdenarie Jul 27, 2023

rdenarie Jul 27, 2023

rdenarie Jul 27, 2023

hakermi commented Jul 28, 2023

rdenarie left a comment

rdenarie Jul 28, 2023

hakermi Jul 28, 2023

hakermi Jul 28, 2023 •

edited

Loading

rdenarie Jul 28, 2023

hakermi Jul 28, 2023

hakermi commented Jul 28, 2023

exo-swf commented Jul 28, 2023

sonarcloud bot commented Jul 31, 2023

feat: Implement an upgrade plugin to update passwords hashing algorithm - EXO-65151 - Meeds-io/MIPs#69 #161

feat: Implement an upgrade plugin to update passwords hashing algorithm - EXO-65151 - Meeds-io/MIPs#69 #161

Conversation

hakermi commented Jul 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakermi commented Jul 28, 2023

rdenarie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakermi Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakermi commented Jul 28, 2023

exo-swf commented Jul 28, 2023

sonarcloud bot commented Jul 31, 2023

hakermi Jul 28, 2023 •

edited

Loading