-
-
Notifications
You must be signed in to change notification settings - Fork 217
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
feat(#9547): add password change feature on first time login #9581
base: master
Are you sure you want to change the base?
Conversation
Hi @garethbowen I worked on the first part to understand the API side routing and display the password page.
My Thoughts
|
Unfortunately this does not solve the requirement. The requirement is that every time a password is set by anyone other than the user themselves (ie: admin), the user is required to set a new password on login. This should be enforced in the API somewhere to ensure this happens regardless of whether the password is set via the admin app, the user management app, or any other method. Therefore there must be no checkbox in the admin app to turn this off. You'll have to check how the user management app is written to make sure it applies there as well. Because this may be disruptive we will include a feature flag to turn it off, just like we do for the new UX features like the action bar -> floating action button. In some time if/when this has been proven to work we will remove the feature flag and make this the only option for all user password setting. All of this must default to true, otherwise we cannot claim the CHT is secure by default.
It's also possible to set up the user with scripts and so on, so I prefer the API approach so we can catch all cases where the password is set.
If the phone is lost then the administrator MUST change the password, not set the checkbox. You have to change the password in order to invalidate the session so anyone finding the phone is kicked out of the app. If they're changing the password then the process above (mandatory password reset) will mean you don't need the checkbox at all.
Thanks for thinking about this! That's an absolute requirement. |
- Add user.password_change_required flag - Add password-reset GET and POST routes - Add passwordResetGet and passwordResetPost functions - Add password-reset template with es translations - Refactor login and password reset to share translation logic
Hi @latin-panda this is ready for an early review even though it is not 100% done. A couple of things:
With those three points, I think the solution design is ready for an early review. |
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.
Nice job Ben! 🤩
Please test that password values don't appear in logs for weird error cases
Hi @garethbowen and @latin-panda I believe this is ready for a review. The password reset logic , unit tests and integration tests are ready for review. Unfortunately the e2e are a bit flaky with the password reset. Sometimes a test hangs on the password reset page causing the test to fail. I want to reach out to QA to get possibly a more robust way of doing the password reset. While I work on the e2e I think the review can go ahead. |
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.
Really extensive work! I haven't looked at the flaky test problem but left some comments on the other bits.
@Benmuiruri Another thing to think about is what happens if the user connects with basic auth, either through curl or directly in the browser. Can they bypass the password reset this way? |
update tests
Hi @garethbowen Thanks for your review.
This video shows the progress so farScreen.Recording.2024-11-18.at.10.45.46.movI'm currently updating failing e2e tests (almost done). As I continues, could I get your review of my implementation of your requested changes. Thanks |
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 progress! I've left a few smallish things to work on.
It's now a fairly big change - once you make these changes and fix the tests I'll come back to it with fresh eyes and give it a thorough look.
if (environment.isTesting) { | ||
return true; | ||
} |
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.
This is a really bad idea because it's complicating production code to make testing work. Furthermore this will block ever testing the following paths.
'password.weak': 'password-weak', //NoSONAR | ||
'password.length.minimum': 'password-short', //NoSONAR | ||
'password.current.incorrect': 'current-password-incorrect', //NoSONAR | ||
'password.same': 'password-same' //NoSonar |
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.
What is the sonar warning?
Description
Password reset for offline user
Screen.Recording.2024-11-18.at.10.45.46.mov
Closes #9547
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.