-
Notifications
You must be signed in to change notification settings - Fork 985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Increase password check to 8 characters and add some basic weakness checks. #13150
Conversation
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. |
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? |
f627346
to
73b10ce
Compare
Found the build documentation and was able to fix my issue. Tested on android. |
Shoot, let me check on that |
7bc5843
to
18dae95
Compare
looks good to me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
18dae95
to
ad597a9
Compare
@qoqobolo @cammellos Rebased, thanks! |
44% of end-end tests have passed
Failed tests (29)Click to expand
Passed tests (23)Click to expand |
44% of end-end tests have passed
Failed 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. |
22db84a
to
2cd00d1
Compare
yeah, this is known issue. we will update test data and then re-run failed tests. |
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 passed
Failed 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 passed
Failed tests (9)Click to expand
Passed tests (2)Click to expand
|
11% of end-end tests have passed
Failed tests (8)Click to expand
Passed tests (1) |
75% of end-end tests have passed
Failed tests (2)Click to expand
Passed tests (6)Click to expand
|
0% of end-end tests have passed
Failed tests (2)Click to expand
|
0% of end-end tests have passed
Failed tests (2)Click to expand
|
0% of end-end tests have passed
Failed tests (2)Click to expand
|
50% of end-end tests have passed
Failed tests (1)Click to expand
Passed tests (1)Click to expand
|
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
a184b9c
to
855da78
Compare
Issue 1 resolved, still working on issue 2. |
97% of end-end tests have passed
Failed 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 |
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