[WIP] Increase password check to 8 characters and add some basic weakness checks. #13150
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey @dangarbri, and thank you so much for making your first pull request in status-react! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2 |
Jenkins BuildsClick to see older builds (27)
|
|
How do I properly create a namespace? I'm getting this error: The required namespace "status-im.utils.password-resources" is not available, it was required by "status_im/ui/screens/onboarding/password/views.cljs". I have (ns status-im.utils.password-resources) in my password_utils.cljs file. |
dangarbri
force-pushed
the
password-checks
branch
from
318cd9e
to
f627346
Compare
|
My all-same function is failing by saying the password is not ISeqable, it should just be a string though, right? |
dangarbri
force-pushed
the
password-checks
branch
from
f627346
to
73b10ce
Compare
|
Found the build documentation and was able to fix my issue. Tested on android. |
|
The word "phenomenon" fits the requirements that you've listed, but it won't let me create an account. Adding a unicode character (Chinese, Emoji, Symbols) to the end doesn't allow me to create the account either.
|
|
Shoot, let me check on that |
dangarbri
force-pushed
the
password-checks
branch
from
7bc5843
to
18dae95
Compare
|
@ducheng0 thanks for catching that, I must have tested without my latest changes, my mistake. It's working now. |
|
looks good to me! |
cammellos
approved these changes
Mar 18, 2022
There was a problem hiding this comment.
Great job, thanks @dangarbri .
I will approve you to work on the bounty on gitcoin and as soon as it's tested ok I will release the bounty.
Thanks again!
|
Hello @dangarbri, thank you for the contribution! |
dangarbri
force-pushed
the
password-checks
branch
from
18dae95
to
ad597a9
Compare
|
@qoqobolo @cammellos Rebased, thanks! |
44% of end-end tests have passedFailed tests (29)Click to expand
Passed tests (23)Click to expand |
44% of end-end tests have passedFailed tests (29)Click to expand
Passed tests (23)Click to expand |
|
Ah, the test is failing because it can't find the 2nd password input. |
dangarbri
force-pushed
the
password-checks
branch
from
22db84a
to
2cd00d1
Compare
yeah, this is known issue. we will update test data and then re-run failed tests. |
dangarbri
force-pushed
the
password-checks
branch
from
2cd00d1
to
d319186
Compare
|
I updated where I think it was getting qwerty from to qwerty1234. If that's the right place to update it then this should work now. |
86% of end-end tests have passedFailed tests (11)Click to expand
Passed tests (68)Click to expand
|
|
@dangarbri Thanx for your contribution! Please, take a look at following issue. ISSUE 1 Unable to login into account after app upgrade if initial account password does not meet new validation rulesSteps:
Actual result: Sign in button is disabled. Expected result: user with old passwords (< 8 char, no basic weakness validation) should be able to Sign in their accounts. |
|
Mmmm, good catch. I'll work on that. |
18% of end-end tests have passedFailed tests (9)Click to expand
Passed tests (2)Click to expand
|
ISSUE 2 No validation error is shown to user when he tries to reset password to value that does not meet basic weakness checks (e.g. "aaaaaaaa", "12345678", "abcdefghi" etc)Steps:
Actual result: no validation errors are shown that could provide user with info why he is unable to proceed with password reset. The text below password fields do not contain any info about basic weakness reqs, only about minimum 8 chars and type of chars (unicode/emoji). Expected result: user must be shown validation error just like he is shown during initial password set. |
11% of end-end tests have passedFailed tests (8)Click to expand
Passed tests (1) |
75% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (6)Click to expand
|
0% of end-end tests have passedFailed tests (2)Click to expand
|
0% of end-end tests have passedFailed tests (2)Click to expand
|
0% of end-end tests have passedFailed tests (2)Click to expand
|
50% of end-end tests have passedFailed tests (1)Click to expand
Passed tests (1)Click to expand
|
dangarbri
force-pushed
the
password-checks
branch
from
d319186
to
a184b9c
Compare
Fix problem where signing in requires 8 chars Add description that emojis are supported. Update minimum password length for reset password popup add password utility Update onboarding and reset password views to share password checking code Update to support masked data Add error when password validation fails Formatting fix Fix is-sequential? function
dangarbri
force-pushed
the
password-checks
branch
from
a184b9c
to
855da78
Compare
dangarbri
changed the title
|
Issue 1 resolved, still working on issue 2. |
97% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (77)Click to expand
|
|
hey @dangarbri are you still working on this issue ? |
|
Hey, yes and no. If someone else wants to wrap up the last issue here, that
would be good. I haven't had time to work on this lately.
…On Tue, May 3, 2022, 4:57 AM flexsurfer ***@***.***> wrote:
hey @dangarbri <https://github.com/dangarbri> are you still working on
this issue ?
—
Reply to this email directly, view it on GitHub
<#13150 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABSKTLYEVU7CXWF4Z5MXG5TVIDS77ANCNFSM5QBLT7LQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
feel free to reopen if needed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #13137
Summary
Password limit should be increased to 8 characters and we should have some
checks to make sure users are not entering weak passwords.
In this PR, I've updated the minimum password length to 8 characters and added 2 weakness checks:
I've also created a password utility so that the onboarding screen and password reset screens can share
the same password validation code.
Testing notes
Tested on android by inputting passwords to test failing conditions.
Platforms
Areas that maybe impacted
Functional
Steps to test
status: Ready