feature: implement password policies to avoid weak passwords by WillardHu · Pull Request #4008 · apolloconfig/apollo

WillardHu · 2021-09-30T02:46:29Z

Signed-off-by: WillardHu [email protected]

What's the purpose of this PR

Closes #3948

Which issue(s) this PR fixes:

Add password verification rule in UserInfoController.createOrUpdateUser(..) method;
- Password needs a number and lowercase letter and between 8~20 characters;
- Password cannot be a commonly used;
Add random password generation button in user-manage.html;

Brief changelog

Implement password policies to avoid weak passwords

Follow this checklist to help us incorporate your contribution quickly and easily:

Read the Contributing Guide before making this pull request.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit tests to verify the code.
Run mvn clean test to make sure this pull request doesn't break anything.
Update the CHANGES log.

codecov-commenter · 2021-09-30T02:51:04Z

Codecov Report

Merging #4008 (c7dc03f) into master (348bee5) will increase coverage by 0.15%.
The diff coverage is 92.59%.

@@             Coverage Diff              @@
##             master    #4008      +/-   ##
============================================
+ Coverage     51.31%   51.46%   +0.15%     
- Complexity     2504     2524      +20     
============================================
  Files           482      484       +2     
  Lines         14772    14798      +26     
  Branches       1528     1532       +4     
============================================
+ Hits           7580     7616      +36     
+ Misses         6662     6652      -10     
  Partials        530      530

Impacted Files	Coverage Δ
...o/portal/util/checker/AuthUserPasswordChecker.java	`87.50% <87.50%> (ø)`
...k/apollo/portal/controller/UserInfoController.java	`55.00% <100.00%> (+23.75%)`	⬆️
...mework/apollo/portal/util/checker/CheckResult.java	`100.00% <100.00%> (ø)`
.../apollo/internals/RemoteConfigLongPollService.java	`78.31% <0.00%> (+0.60%)`	⬆️
...ervice/service/ReleaseMessageServiceWithCache.java	`87.05% <0.00%> (+1.17%)`	⬆️
...work/apollo/biz/message/ReleaseMessageScanner.java	`85.52% <0.00%> (+2.63%)`	⬆️
...trip/framework/apollo/portal/entity/po/UserPO.java	`29.16% <0.00%> (+25.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 348bee5...c7dc03f. Read the comment docs.

CHANGES.md

...lo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java

apollo-portal/src/main/resources/static/i18n/zh-CN.json

apollo-portal/src/main/resources/static/scripts/controller/UserController.js

kezhenxu94 · 2021-09-30T02:59:50Z

...ortal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java

+    user.setUsername("username");
+    user.setPassword("9sivg8hg3wjz8");
+    userInfoController.createOrUpdateUser(user);
+    Assert.assertTrue(true);


This is pointless

Done, this test case has been removed. Thanks

Hi @WillardHu , I don't mean to remove the case, I mean the assertTrue(true) is pointless, removing this line should be enough

...-portal/src/test/java/com/ctrip/framework/apollo/portal/util/CommonlyUsedPwdCheckerTest.java

kezhenxu94 · 2021-09-30T05:43:31Z

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserPasswordChecker.java

+  }
+
+  /**
+   * @return The passwrod contains code fragment or is blank.


Suggested change

* @return The passwrod contains code fragment or is blank.

* @return The password contains code fragment or is blank.

Done, thanks.

kezhenxu94 · 2021-09-30T05:51:12Z

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserPasswordChecker.java

+  public static boolean notMatchRegex(String password) {
+    return !PWD_PATTERN.matcher(password).matches();
+  }


Suggested change

public static boolean notMatchRegex(String password) {

return !PWD_PATTERN.matcher(password).matches();

}

public static boolean isWeakPassword(String password) {

return !PWD_PATTERN.matcher(password).matches() || isCommonlyUsed(password);

}

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserPasswordChecker.java

kezhenxu94 · 2021-09-30T05:53:39Z

apollo-portal/src/main/resources/static/scripts/controller/UserController.js

+    var alpha_chars =[ 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l',
+               'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'];
+
+    function randomAlphanumeric(len) {


I'm not sure whether we should provide this functionality, because most modern browsers (like Safari) or password manager (like 1password) has this ability to generate strong random password, this function defined here is rather naive IMO

I agree with you if it's self-registration, but in this scenario, it is the administrator manage the account. This is just a convenience for administrators to generate default passwords for members if he wanted. And the browser's password generation extension may only work for the password component. If you don't want it, I'll remove it. Thanks.

@WillardHu Let's wait for other reviewers' opinions

I would remove that functionality since strong password generation can be done by the browser.
Additionally a custom password generator would be harder to maintain (for example if the password policy changes etc) with little to none benefit.

I would also suggest we don't provide this functionality as it is not a core function of apollo and there are many plugins or browsers doing this job much better. What we need to do though is to make sure the password form is compatible with those plugins or browsers.

Ok, the frontend modifications have been rollback.

DiegoKrupitza

The rest looks good to me.

DiegoKrupitza · 2021-09-30T17:14:12Z

...lo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java

+    if (UserPasswordChecker.isWeakPassword(user.getPassword())) {
+      throw new BadRequestException(
+          "Password needs a number and lowercase letter and between 8~20 characters. "
+              + "And cannot be a weak password.");


I think you should adapt the message. Makes it more useable and clear to the user what the real reason is.
It can be not matching to the pattern, weak password or both.
If it is just one and you display both reasons in the error message the user might not understand it why you get both.

PS: you should also explain what a weak password is in the exception

Ok, I've split the error message, and wrap it with CheckResult.

DiegoKrupitza · 2021-09-30T17:17:19Z

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserPasswordChecker.java

+
+public class UserPasswordChecker {
+
+  private static final Pattern PWD_PATTERN = Pattern


Would be nice if you could write few test cases that check if this pattern really works as intended. Regex is often a reason for bugs.

nobodyiam · 2021-10-03T01:06:51Z

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserPasswordChecker.java

+      "1q2w", "2w3e", "3e4r", "5t6y", "abcd", "qwer", "asdf", "zxcv"
+  );
+
+  public static boolean isWeakPassword(String password) {


I would suggest we use instance method instead of static method so that we could mock this method easily.

Ok, I used Spring IOC to register the bean instance of UserPasswordChecker interface’s implementation class AuthUserPasswordChecker.

nobodyiam · 2021-10-03T01:06:57Z

apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/UserPasswordChecker.java

+  /**
+   * @return The password contains code fragment or is blank.
+   */
+  private static boolean isCommonlyUsed(String password) {


I would suggest we use instance method instead of static method so that we could mock this method easily.

nobodyiam · 2021-10-03T01:09:36Z

...ortal/src/test/java/com/ctrip/framework/apollo/portal/controller/UserInfoControllerTest.java

+      try {
+        userInfoController.createOrUpdateUser(user);
+      } catch (BadRequestException e) {
+        Assert.assertEquals(


It's better we mock password checker here instead of testing the password logic here, as it is not core logic of UserInfoController.

nobodyiam · 2021-10-03T01:11:37Z

...-portal/src/test/java/com/ctrip/framework/apollo/portal/util/CommonlyUsedPwdCheckerTest.java

+import org.junit.Assert;
+import org.junit.Test;
+
+public class CommonlyUsedPwdCheckerTest {


This test should be renamed as something like UserPasswordCheckerTest so that we will test all kinds of weak password scenarios.

DiegoKrupitza · 2021-10-03T08:12:07Z

...lo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java

+    CheckResult pwdCheckRes;
+    if (!(pwdCheckRes = passwordChecker.checkWeakPassword(user.getPassword())).isSuccess()) {
+      throw new BadRequestException(pwdCheckRes.getMessage());
+    }


Is there a reason why you assign pwdCheckRes inside the if and not before when you declare the var pwdCheckRes?

Otherwise to make the code more readable I would suggest to change it to

CheckResult pwdCheckRes = passwordChecker.checkWeakPassword(user.getPassword()); if (!pwdCheckRes.isSuccess()) { throw new BadRequestException(pwdCheckRes.getMessage()); }

Signed-off-by: WillardHu <[email protected]>

DiegoKrupitza · 2021-10-03T13:01:10Z

Good work! 👍🏽:shipit:

nobodyiam

LGTM

kezhenxu94 requested changes Sep 30, 2021

View reviewed changes

kezhenxu94 added the enhancement label Sep 30, 2021

WillardHu force-pushed the feat/issue-3948 branch 2 times, most recently from 70f1d0c to edc6de5 Compare September 30, 2021 05:26

WillardHu requested a review from kezhenxu94 September 30, 2021 05:31

WillardHu force-pushed the feat/issue-3948 branch from edc6de5 to cc1cb83 Compare September 30, 2021 05:40

kezhenxu94 reviewed Sep 30, 2021

View reviewed changes

WillardHu force-pushed the feat/issue-3948 branch from cc1cb83 to f9312ba Compare September 30, 2021 06:41

WillardHu requested a review from kezhenxu94 September 30, 2021 06:46

kezhenxu94 requested a review from nobodyiam September 30, 2021 06:59

WillardHu force-pushed the feat/issue-3948 branch from f9312ba to a732f6c Compare September 30, 2021 07:00

DiegoKrupitza reviewed Sep 30, 2021

View reviewed changes

nobodyiam reviewed Oct 3, 2021

View reviewed changes

WillardHu force-pushed the feat/issue-3948 branch from a732f6c to 465491c Compare October 3, 2021 04:19

WillardHu requested a review from nobodyiam October 3, 2021 04:50

DiegoKrupitza reviewed Oct 3, 2021

View reviewed changes

implement password policies to avoid weak passwords

9b9281e

Signed-off-by: WillardHu <[email protected]>

WillardHu force-pushed the feat/issue-3948 branch from 465491c to 9b9281e Compare October 3, 2021 12:44

Merge branch 'master' into feat/issue-3948

c7dc03f

nobodyiam approved these changes Oct 4, 2021

View reviewed changes

kezhenxu94 approved these changes Oct 4, 2021

View reviewed changes

kezhenxu94 merged commit 4a28fbd into apolloconfig:master Oct 4, 2021

github-actions bot locked and limited conversation to collaborators Oct 4, 2021

nobodyiam added this to the 1.10.0 milestone Oct 6, 2021

WillardHu deleted the feat/issue-3948 branch October 8, 2021 23:49

	* @return The passwrod contains code fragment or is blank.
	* @return The password contains code fragment or is blank.


		public class UserPasswordChecker {

		private static final Pattern PWD_PATTERN = Pattern

Uh oh!

Conversation

WillardHu commented Sep 30, 2021 • edited by kezhenxu94 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's the purpose of this PR

Which issue(s) this PR fixes:

Brief changelog

Uh oh!

codecov-commenter commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kezhenxu94 Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiegoKrupitza left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiegoKrupitza Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

WillardHu commented Sep 30, 2021 •

edited by kezhenxu94

Loading

codecov-commenter commented Sep 30, 2021 •

edited

Loading

kezhenxu94 Sep 30, 2021 •

edited

Loading

DiegoKrupitza Sep 30, 2021 •

edited

Loading