-
-
Notifications
You must be signed in to change notification settings - Fork 218
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
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.
Hey @jkuester 👋 With @garethbowen out of office this week, I was wondering if you could review this PR, we work together to get it merged. I addressed the feedback, but I'd appreciate a fresh pair of eyes given it was my first time in the API side of things. The PR description has a video of the state of the feature. Happy to work with you on this. |
👍 Sounds good @Benmuiruri. I can have a look at things and let you know if it would be helpful to sync on the details. Unfortunately, I am pretty stacked up this week with PRs. I should be able to get to this by EOW, but if you need it sooner, feel free to replace me with a different reviewer. |
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.