Simplify login/signup form validation #2095
Conversation
There was a problem hiding this comment.
When I give an invalid email then go to password it turns red as expected. Then when I enter an invalid password and go back to email field, the password field does not turn red.
I was unable to submit the form with the following 64 char password:
NF{ejLDxvLvgPiCGrecpufoPFsFTz8irtigJVBibgT)HBwRLaupvPwWfGzb6gyAu
| @@ -69,11 +65,15 @@ export class Auth extends Component<Props> { | |||
| ? 'Could not create account. Please try again.' | |||
| : 'Could not log in with the provided email address and password.'; | |||
|
|
|||
| const mainClasses = classNames('login', { | |||
There was a problem hiding this comment.
I'm not sure this is needed since it will only ever be electron.
There was a problem hiding this comment.
It avoids applying the class for local development 🤷
| onBlur = (event) => { | ||
| // nothing has been entered yet, don't validate | ||
| if ( | ||
| event.currentTarget.value === '' && |
There was a problem hiding this comment.
In a future PR we should be using local state or useRef to manipulate the class names. Interacting directly with the DOM is not reccomended.
I can't reproduce this with the steps provided, on either the login or signup form. Can you post the exact steps? Is it possible you were on the login form and it was only requiring a minimum of 4 characters (so the password was actually valid)?
Whoops, the regex lost its curly braces somewhere. Fixed in this PR. Do we want to patch the release branch to allow those characters? |
codebykat
force-pushed
the
update/signup-form-validation
branch
from
e533f62
to
d28bb05
Compare
|
Fixed regex separately in #2114 . This will need a rebase |
codebykat
force-pushed
the
update/signup-form-validation
branch
from
f8ff119
to
97df7bc
Compare
|
Rebased 👍 |
|
|
||
| // login - slightly more relaxed password rules | ||
| else { | ||
| if (!this.passwordInput.validity.valid) { |
There was a problem hiding this comment.
why are we allowing relaxed passwords?
There was a problem hiding this comment.
We haven't implemented the check for forcing-reset-on-login yet... but even so, we first need to submit the auth, which means we need to allow existing passwords to go through
There was a problem hiding this comment.
Ahh, yes. I thought we do have the check: #2088
There was a problem hiding this comment.
Oh yeah I got confused about what was merged back into develop. Let's make sure to test this flow
There was a problem hiding this comment.
Yeah okay I still stand by this code. #2088 uses authorize(username, password).then(... which means the form needs to accept the insecure password in order to submit it for authorization, at which point we prompt them to reset. Confusing but I think we do need to do it, otherwise we don't know if it's the correct password to begin with.
There was a problem hiding this comment.
codebykat
force-pushed
the
update/signup-form-validation
branch
from
f97e085
to
f6cba5c
Compare
Fix
Simplifies and cleans up form validation for the signup/login form. Also takes advantage of the HTML form validation API.
These changes echo the updates made on the GAE side in https://github.com/Simperium/simplenote-gae/pull/280 and https://github.com/Simperium/simplenote-gae/pull/294
Test
Try to sign up and log in on Electron. See if form validation behaves in a reasonable way (correct error messages, blocking form submission, etc).
Some specific things to try:
Release
Not updated: Simplified login/signup form validation