Skip to content
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

Open
wants to merge 52 commits into
base: master
Choose a base branch
from

Conversation

Benmuiruri
Copy link
Contributor

@Benmuiruri Benmuiruri commented Oct 24, 2024

Description

Password reset for offline user
Screen.Recording.2024-11-18.at.10.45.46.mov

Closes #9547

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

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.

@Benmuiruri
Copy link
Contributor Author

Hi @garethbowen I worked on the first part to understand the API side routing and display the password page.
Now I'm updating the form to meet the Figma design and checking out the update password logic to use it in the password change form. As I do that, I'd like your input of my approach / understanding of this feature.

  • The approach I would like to take for this is to edit the add user form in the admin to add a checkbox option, which is unchecked by default (hence false / null). When checked it sets require_password_change to true in the user object.
  • In the login controller I have password_change_required = true;.
  • The login controller will check the permission (does the user's role have the permission can_change_password_first_login && is require_password_change = true for the user && is password_change_required = true; ) - If all those are true, then it is the user's first time login and require password change.
  • Once user changes password the password update function will be in the login controller so that it can update password_change_required = false; and possibly update user settings to update require_password_change flag to false That way subsequent logins will not require password change.

My Thoughts

  • I believe this would work with CHT User Management tool to add the checkbox option when creating single or multiple users.
  • I believe this would work if a user loses their phone because the admin can select the require_password_change checkbox in the admin.
  • I believe this would not be a breaking change for existing users because require_password_change would be null for them.
  • When making it mandatory for projects in future, we would make the require_password_change field to be mandatory true.
  • A forgotten password feature would also reuse the same require_password_change in the user object.

@garethbowen
Copy link
Member

The approach I would like to take for this is to edit the add user form in the admin to add a checkbox option, which is unchecked by default (hence false / null). When checked it sets require_password_change to true in the user object.

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.

I believe this would work with CHT User Management tool to add the checkbox option when creating single or multiple users.

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.

I believe this would work if a user loses their phone because the admin can select the require_password_change checkbox in the admin.

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.

I believe this would not be a breaking change for existing users because require_password_change would be null for them.

Thanks for thinking about this! That's an absolute requirement.

@Benmuiruri Benmuiruri marked this pull request as ready for review October 31, 2024 18:17
@Benmuiruri
Copy link
Contributor Author

Hi @latin-panda this is ready for an early review even though it is not 100% done. A couple of things:

  • I tried a couple of things to display the param for the translation, but I could not get the minimum to display, any idea how to pass it from the js file to the html file ?
  • I split the translation for login and password reset to api/src/public/login/auth-utils.js to avoid duplication. Do I change the eslint to allow compile app to run without the Parsing error: 'import' and 'export' may appear only with 'sourceType: module' error ?
  • I'll work on the styling of the password reset validation user feedback to match Figma on Monday.

With those three points, I think the solution design is ready for an early review.

Copy link
Contributor

@latin-panda latin-panda left a 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

api/resources/translations/messages-es.properties Outdated Show resolved Hide resolved
api/resources/translations/messages-en.properties Outdated Show resolved Hide resolved
api/src/auth.js Outdated Show resolved Hide resolved
api/src/controllers/login.js Outdated Show resolved Hide resolved
api/resources/translations/messages-en.properties Outdated Show resolved Hide resolved
api/src/public/login/auth-utils.js Show resolved Hide resolved
api/src/public/login/auth-utils.js Outdated Show resolved Hide resolved
api/src/public/login/auth-utils.js Outdated Show resolved Hide resolved
api/src/public/login/password-reset.js Outdated Show resolved Hide resolved
api/src/public/login/password-reset.js Outdated Show resolved Hide resolved
@Benmuiruri
Copy link
Contributor Author

Benmuiruri commented Nov 13, 2024

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.

Copy link
Member

@garethbowen garethbowen left a 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.

api/src/controllers/login.js Outdated Show resolved Hide resolved
api/src/controllers/login.js Outdated Show resolved Hide resolved
api/src/controllers/login.js Show resolved Hide resolved
api/src/controllers/login.js Outdated Show resolved Hide resolved
api/src/public/login/auth-utils.js Outdated Show resolved Hide resolved
api/src/public/login/script.js Outdated Show resolved Hide resolved
api/src/public/login/script.js Outdated Show resolved Hide resolved
api/src/public/login/password-reset.js Outdated Show resolved Hide resolved
api/src/controllers/login.js Outdated Show resolved Hide resolved
api/src/public/login/password-reset.js Outdated Show resolved Hide resolved
tests/e2e/default/login/login-logout.wdio-spec.js Outdated Show resolved Hide resolved
tests/e2e/default/login/login-logout.wdio-spec.js Outdated Show resolved Hide resolved
tests/e2e/default/login/login-logout.wdio-spec.js Outdated Show resolved Hide resolved
tests/e2e/default/login/login-logout.wdio-spec.js Outdated Show resolved Hide resolved
@garethbowen
Copy link
Member

@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?

@Benmuiruri
Copy link
Contributor Author

Hi @garethbowen Thanks for your review.

  • I refactored the auth-utils.js file to drop the use of modules
  • I changed the updatePassword in the login controller to safe guard against timeouts over a network
  • I added the currentPassword field in the password reset form
  • I added a check for password reset in the authorization middleware
This video shows the progress so far
Screen.Recording.2024-11-18.at.10.45.46.mov

I'm currently updating failing e2e tests (almost done). As I continues, could I get your review of my implementation of your requested changes. Thanks

Copy link
Member

@garethbowen garethbowen left a 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.

api/src/auth.js Show resolved Hide resolved
Comment on lines +91 to +93
if (environment.isTesting) {
return true;
}
Copy link
Member

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.

Comment on lines +24 to +27
'password.weak': 'password-weak', //NoSONAR
'password.length.minimum': 'password-short', //NoSONAR
'password.current.incorrect': 'current-password-incorrect', //NoSONAR
'password.same': 'password-same' //NoSonar
Copy link
Contributor

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?

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.

Change password on first login
4 participants