feat(#9547): add password change feature on first time login #9581

Benmuiruri · 2024-10-24T16:08:23Z

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 · 2024-10-25T07:28:05Z

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 · 2024-10-25T07:43:06Z

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.

api/src/controllers/login.js

- 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

Benmuiruri · 2024-11-01T04:24:50Z

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.

…assword

latin-panda

Nice job Ben! 🤩

Please test that password values don't appear in logs for weird error cases

api/resources/translations/messages-es.properties

api/resources/translations/messages-en.properties

api/src/auth.js

api/src/controllers/login.js

api/resources/translations/messages-en.properties

api/src/public/login/auth-utils.js

api/src/public/login/password-reset.js

webapp/src/js/bootstrapper/index.js

api/src/controllers/login.js

update tests

…assword

Benmuiruri · 2024-11-18T07:52:06Z

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

garethbowen

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

api/tests/mocha/middleware/authorization.spec.js

api/src/controllers/login.js

…assword

Benmuiruri · 2024-12-02T15:23:53Z

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.

jkuester · 2024-12-02T22:47:06Z

👍 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.

jkuester · 2024-12-05T23:18:06Z

FYI, got dug in on this and started reviewing it today, but was not able to make it all the way through. I will be OOO on Fri/Mon, but will pick this back up next Tues. 👍

Benmuiruri · 2024-12-09T09:33:55Z

Thanks for the heads up @jkuester

jkuester

Alright, going to go ahead and post what I have reviewed so far. You have done a TON of great work here! (Hard to believe how much code it took to implement this feature... 😅 )

I have tried to focus just on important issues/questions and/or easy code suggestions. But, knowing the time constraints we have here, I will definitely accept "This works fine as it is" as a response to any of my refactoring comments!

shared-libs/user-management/src/index.js

shared-libs/user-management/src/users.js

shared-libs/user-management/test/unit/users.spec.js

jkuester · 2024-12-11T22:53:10Z

api/src/controllers/login.js

+      }
+
+      const userDoc = await db.users.get(`org.couchdb.user:${username}`);
+      await updatePassword(userDoc, password);


question: Actually, wait, is there a reason we cannot use the users.updateUser functionality here instead of manually getting/putting the _user doc? Not sure what the 401 retry logic is about in this updatePassword function, though. If we need that, then I guess we cannot use users.updateUser...

The longer I look at this, the more confused I am by that retry logic. Why would we get a 401 on the put, but not here on the get? I am probably missing something important here....

It's because of a race condition, when testing it out, in some instances requesting session immediately after changing a password was giving back a 401. Hence I safe guarded it with the retry to avoid that.

So, yeah, the createSessionRetry logic makes sense to me (and I guess already existed for token login). But here is where we are actually changing the password. Seems like we should not be getting a 401 from the db.users.put(updatedUser); call. If I understand things correctly, the user session that is actually used to make that update would be the api server's admin user (COUCH_USER) anyway since we have not yet created a session for the current user trying to login....

api/src/controllers/login.js

api/tests/mocha/controllers/login.spec.js

api/src/controllers/login.js

jkuester

Okay, made it through the rest of the implementation code and unit tests.

For the e2e/integration tests, I don't like how all the random tests that need a non-admin user have to deal with the password change complexity. Ideally we would have dedicated tests for the password reset workflow and then none of the rest of the tests woud have to worry about it.

Now that is, of course, easier said than done. I guess the most naive way to avoid the password reset would be to add the roles to can_skip_password_change in config/default/app_settings.json, but that is not super elegant.

The main idea I have come up with so far involves updaing the createUsers function in utils/index.js to do either of these two things:

Manually update the user doc after creating the user to remove the password_change_required field. This is nice and simple. The downside is that it is a bit artificial, but I am not sure it rises to the level of affecting the integrety of the tests.
The other alternative would be to have the createUsers function write the initial user with with a different password than the one provided in the parameter. Then, after the user has been created the createUsers function could do a login and run the reset-password flow to set the password to the original password value provided for the user. The end result after calling createUsers would be that the user exists and has the specified password and a reset will not be triggered the next time the user logs in.

Ideally the deafult behavior of createUsers would result in the user existing in the db with the provided password and no password reset would happen the next time the user logs in. For the actual password-reset tests, we could either parameterize the createUsers function to be able to flag off that behavior (so the test can do the reset) or we could just not use this function for creating users for those tests...

jkuester · 2024-12-12T19:01:53Z

api/src/middleware/authorization.js

@@ -40,7 +40,9 @@ module.exports = {
      return serverUtils.error('Authentication error', req, res);
    }

-    next();
+    return auth.checkPasswordChange(req)


Hmmm, do we really need to force this check here? If we only set the password_change_required when actually changing the user's password, then the user's session should always be invalidated. This would require them to re-login and then they will hit the password_change_required logic in the login controller. Are there other cases I am missing here?

The reason for adding this was to block users who require password change from bypassing the check via basic auth or curl.

The first result is with the guard

The second result is without it

Ahh, I see. 🤔 Honestly, I wonder if we actually care about the basic auth case... 😬 They are giving is a valid password, so it is not like this is a security hole. Maybe it is acceptable to say that the password-reset flow is only triggered by the login-flow (and not when making random requests with basic auth...). IMHO that would be preferable to adding an extra round-trip to _users for each server request....

Going to push this question up to the main issue too!

api/tests/mocha/middleware/authorization.spec.js

api/src/auth.js

api/src/services/cookie.js

jkuester · 2024-12-12T20:47:31Z

webapp/tests/mocha/unit/bootstrapper.spec.js

@@ -99,6 +99,12 @@ describe('bootstrapper', () => {
      return promise;
    });

+    global.localStorage = {


suggestion: can we also get a few tests to cover the new logic?

jkuester · 2024-12-12T20:52:04Z

tests/e2e/default/admin/admin-access.wdio-spec.js

@@ -31,7 +31,8 @@ describe('Acessing the admin app', () => {
    const error = '{"code":403,"error":"forbidden","details":"Offline users are not allowed access to this endpoint"}';
    await utils.saveDocs([parent]);
    await utils.createUsers([offlineUser]);
-    await loginPage.cookieLogin({ ...offlineUser, createUser: false });
+
+    await loginPage.login({ username: offlineUser.username, password: offlineUser.password });


question: why did this need to change?

jkuester · 2024-12-12T21:06:24Z

api/src/controllers/login.js

+  return !user?.password_change_required ||
+    user?.token_login?.active ||
+    roles.isDbAdmin(userCtx) ||
+    await auth.hasAllPermissions(userCtx, 'can_skip_password_change');


thought: referencing back to my previous comment about how this feature feels like a bit of a breaking change to have enabled by default, I initially expected this behavior to be something toggled ON by a permission. E.g. the system would trigger the reset password flow only for users with the can_reset_password_assigned_by_admin permission, or something like that...

Great observation, I initially thought that as well. Even started working on it from that approach. However, Gareth raised a fair point. Our purpose for this feature is to improve security to safe guard against a password leak.

If we leave it as an opt in feature, because people are generally "lazy" when it comes to security steps, a project will not turn it on. Therefore, in the event of a password leak, the feature won't really help them.

But having it as an opt out then that way the system is designed to safe guard, but you can opt out knowingly at your own risk.

chore: test password change routing

d7173a6

garethbowen reviewed Oct 25, 2024

View reviewed changes

api/src/controllers/login.js Outdated Show resolved Hide resolved

garethbowen reviewed Oct 25, 2024

View reviewed changes

api/src/controllers/login.js Outdated Show resolved Hide resolved

Benmuiruri added 9 commits October 29, 2024 12:04

Add password reset flag to API

523fc30

- 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

chore: add password validation

1ef9996

chore: add password validation error msgs

efe3a72

chore: prevent unauthorized access before password change

057782c

chore: redirect back to login if not password reset

63017ca

chore: use cookie to prevent access

bfa9485

chore: try cookie to show UI

c9b570d

chore: show password success UI

f1dea9a

sonar reduce cognitive

7e15098

Benmuiruri marked this pull request as ready for review October 31, 2024 18:17

chore: sonar fixes

d2d18e9

Benmuiruri requested a review from latin-panda November 1, 2024 04:16

Benmuiruri added 2 commits November 4, 2024 09:10

Merge branch 'master' of github.com:medic/cht-core into 9547-change-p…

9d1c055

…assword

chore: interpolate translations

8fee9e8

latin-panda requested changes Nov 4, 2024

View reviewed changes

chore: first round of feedback

0323df5

github-advanced-security bot found potential problems Nov 4, 2024

View reviewed changes

webapp/src/js/bootstrapper/index.js Fixed Show fixed Hide fixed

chore: additional feedback

935c721

garethbowen reviewed Nov 5, 2024

View reviewed changes

api/src/controllers/login.js Outdated Show resolved Hide resolved

Benmuiruri added 5 commits November 6, 2024 11:15

chore: refactor password validation

f4f4de0

chore: refactor toggle password and legacy code

04b3f97

chore: remove passwordUpdate cookie and set minimal cookie without Auth

1a4e971

chore: refactor setting basic Cookie

f6d9949

chore: clean password reset

d693b75

Benmuiruri added 7 commits November 15, 2024 08:04

chore: try iife pattern

71ccda5

chore: sonar

cf77f9f

validate password reset in auth middleware

6c7d976

update tests

update e2e

e0806e0

Merge branch 'master' of github.com:medic/cht-core into 9547-change-p…

95d5f9c

…assword

chore: update breaking e2e

40bee0a

chore: sonar

87170ff

Benmuiruri requested a review from garethbowen November 18, 2024 07:52

chore: update e2e tests

c23d3e2

garethbowen requested changes Nov 18, 2024

View reviewed changes

api/src/auth.js Outdated Show resolved Hide resolved

api/src/auth.js Outdated Show resolved Hide resolved

api/tests/mocha/middleware/authorization.spec.js Outdated Show resolved Hide resolved

latin-panda reviewed Nov 18, 2024

View reviewed changes

api/src/controllers/login.js Outdated Show resolved Hide resolved

Benmuiruri added 9 commits November 19, 2024 07:47

chore: lint

4312722

Merge branch 'master' of github.com:medic/cht-core into 9547-change-p…

424fe87

…assword

chore: sonar

fc9e645

update e2e and fix sonar

874fafc

chore: sonar

6d72df4

refactor auth::checkPasswordChange

d68e8e9

chore: update integration tests

b0ef429

Merge branch 'master' of github.com:medic/cht-core into 9547-change-p…

3a5fd7c

…assword

chore: update sentinel integration tests

f3690ad

chore: test, trying to fix repeat e2e test (#9681)

4bf749a

jkuester self-requested a review December 5, 2024 22:08

jkuester requested changes Dec 12, 2024

View reviewed changes

jkuester mentioned this pull request Dec 13, 2024

Change password on first login #9547

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#9547): add password change feature on first time login #9581

feat(#9547): add password change feature on first time login #9581

Benmuiruri commented Oct 24, 2024 •

edited

Loading

Benmuiruri commented Oct 25, 2024

garethbowen commented Oct 25, 2024

Benmuiruri commented Nov 1, 2024

latin-panda left a comment

Benmuiruri commented Nov 18, 2024

garethbowen left a comment

Benmuiruri commented Dec 2, 2024

jkuester commented Dec 2, 2024

jkuester commented Dec 5, 2024

Benmuiruri commented Dec 9, 2024

jkuester left a comment

jkuester Dec 11, 2024

jkuester Dec 11, 2024

Benmuiruri Dec 13, 2024

jkuester Dec 13, 2024

jkuester left a comment

jkuester Dec 12, 2024

Benmuiruri Dec 13, 2024

jkuester Dec 13, 2024

jkuester Dec 12, 2024

jkuester Dec 12, 2024

jkuester Dec 12, 2024

Benmuiruri Dec 13, 2024

feat(#9547): add password change feature on first time login #9581

Are you sure you want to change the base?

feat(#9547): add password change feature on first time login #9581

Conversation

Benmuiruri commented Oct 24, 2024 • edited Loading

Description

Code review checklist

Compose URLs

License

Benmuiruri commented Oct 25, 2024

garethbowen commented Oct 25, 2024

Benmuiruri commented Nov 1, 2024

latin-panda left a comment

Choose a reason for hiding this comment

Benmuiruri commented Nov 18, 2024

garethbowen left a comment

Choose a reason for hiding this comment

Benmuiruri commented Dec 2, 2024

jkuester commented Dec 2, 2024

jkuester commented Dec 5, 2024

Benmuiruri commented Dec 9, 2024

jkuester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkuester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benmuiruri commented Oct 24, 2024 •

edited

Loading