Skip to content

RA-1553: Added client side verification for password input#49

Open
anuar2k wants to merge 2 commits intoopenmrs:masterfrom
anuar2k:RA-1553
Open

RA-1553: Added client side verification for password input#49
anuar2k wants to merge 2 commits intoopenmrs:masterfrom
anuar2k:RA-1553

Conversation

@anuar2k
Copy link
Copy Markdown

@anuar2k anuar2k commented Dec 9, 2018

Description of what I changed

Although the issue I've reported wasn't in no way reviewed, I believe it is valid. I've come up with a possible solution, which seems to fix the problem.

I have added the functionality to the pages which verificates the entered passwords if they are valid. Code (regex) which verificates passwords is generated dynamically (together with a message), based on the state of appropriate GlobalProperties. Currently, the only check done on the client size is minimum length verification, so if our password passes client side check and fails on the server side, an error is displayed with "Validation errors found" message, which wasn't really helpful when I was new to the system. I've wasted a lot of time trying to figure out what's wrong; why I couldn't change the password, so imagine non-tech-savvy clerks trying to do so.

Note: Although it seems that a long warning message breaks the layout on Add New Account page, the same problem exists if only the password length warning is displayed. This problem was described with another ticket, therefore shouldn't be addressed in this PR.

Issue I worked on

see https://issues.openmrs.org/browse/RA-1553

@djazayeri
Copy link
Copy Markdown
Member

I agree about improving the UX.
It looks like the logic you've added here must be duplicated from openmrs-core (right?), so is it possible to instead have core generate the error message and display it, to minimize the amount of duplicated logic?

@anuar2k
Copy link
Copy Markdown
Author

anuar2k commented Dec 10, 2018

@djazayeri This new code isn't really about displaying the error message after when user tries to change his password (pushes a button to save it), but rather about verifying the input on client side, dynamically using ng-pattern property of the input to not even allow the user to send incorrect password (which will not be accepted by the server).

The message displayed after a trial is different from the exception messages thrown from OpenmrsUtil.validatePassword which are in form "Please choose a password that [...]", but it's rather in a form of a checklist displayed until the password meets the requirements (similiar to what you can see when registering a new account on any modern website). What that checklist contains AND how the verification regex looks is based on what OpenmrsUtil.validatePassword utility demands from the passwords (based on the state of appropriate GlobalProperties, to be precise).

It's simply does this:

  • controller reads the state of appropriate GlobalProperties and "tells" the PageModel what rules must the password meet
  • the page using the information from the controller compiles a regex used to verify the password on client-side and compiles a "checklist" message in form "This password must have: [...], [...], [...]" which is displayed until user's input meets the requirements.

Unfortunately I don't see how we could prevent code duplication here, because the logic is a bit different between what server and client does.

Thanks for the attention.

PS: Sorry for sounding very repetetive but I've tried to explain it in a bit different way.

@anuar2k
Copy link
Copy Markdown
Author

anuar2k commented Dec 10, 2018

@djazayeri I have refactored the code (moved pattern and patternErrorMessage generation to a Java utility class instead of repeating the code in the GSPs) and for now I'm keeping both commits (so the changes are visible) - when we'll eventually get to merging in the future I can squash them.

@dkayiwa
Copy link
Copy Markdown
Member

dkayiwa commented Feb 22, 2019

@anuar2k are you still available to add some tests for the PasswordValidation class?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants