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

Change password on first login #9547

Open
Tracked by #190
latin-panda opened this issue Oct 16, 2024 · 13 comments · May be fixed by #9581
Open
Tracked by #190

Change password on first login #9547

latin-panda opened this issue Oct 16, 2024 · 13 comments · May be fixed by #9581
Assignees
Labels
Type: Security Affects security
Milestone

Comments

@latin-panda
Copy link
Contributor

latin-panda commented Oct 16, 2024

Is your feature request related to a problem? Please describe.
System admin users create accounts for CHWs and then share the password with them. To enhance the security of these accounts, there should be a way to prompt a password change on the first login.

Describe the solution you'd like
On the login page, create a feature to change the password with the following considerations:

  • All UI texts should be translatable in CHT's supported languages
  • This feature is enabled based on permissions (change_password_first_login)
  • Only new users will be prompted to change their password on the first login when the permission is enabled.
  • Subsequent logins won't require a password change.
  • Changing the password in this step is an online action (they need internet)
  • If the CHW can't change the password for some reason, they can't access the app
  • If CHW logins for first login and closes the app before changing password, the next time they login (even though it is not technically first login) they are prompted to change password.

Figma design

The scope of this work is to enable changing the password on the first login. It doesn't include changing the password rules or current authentication mechanisms.

@latin-panda latin-panda added Type: Feature Add something new Type: Security Affects security labels Oct 16, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Care Teams Oct 16, 2024
@latin-panda latin-panda moved this from Todo to Next Week's Commitments in Product Team Activities Oct 16, 2024
@latin-panda latin-panda moved this from Backlog to Planned in Care Teams Oct 16, 2024
@garethbowen
Copy link
Member

This feature is enabled based on permissions

Once the MVP is proven then reset password will be mandatory for all projects so we can ensure all users on all instances are not compromised in future. Until then it's fine to use a feature flag so projects can opt-in to try it out.

Subsequent logins won't require a password change.

One addition to this, is the user loses their phone, or forgets their password and the administrator resets it for them, then the user will be required to change their password again. This is because the password has almost certainly been shared in plaintext so it's once again vulnerable to future leaks.

@Benmuiruri
Copy link
Contributor

Hi @garethbowen

I'll be starting this tomorrow (Oct 23). Probably start with conceptualizing the data flow then work on the UI first. Just checking whether you have any comment / clarification before I start ..(anything to add other that what we have in this thread) ?

cc @n-orlowski are we going with the single password input design or confirm password design in Figma?

@garethbowen
Copy link
Member

@Benmuiruri Nothing to add regarding the UX. Potentially explore ways to keep it simple for the MVP, for example, is the password strength bar strictly necessary or something we can do in a separate issue?

From an implementation point of view, I think it's a fairly simple process of flagging the user with password_change_required: true (or something). This gives us flexibility to implement further features like forgotten password and admin reset easily using the same logic. We need to make sure this flag is set when users are created or have passwords changed whether that's done using the user management app, the admin app, direct to the db, or any other way. Make sure everyone is using the API or we'll have to intercept the POST request...

Finally, because this is in the authentication system we'll have to be very careful not to accidentally break anything. For example, if the feature flag is enabled once users are already logged in they should not be forced to change their password.

@Benmuiruri Benmuiruri moved this from Next Week's Commitments to In Progress in Product Team Activities Oct 23, 2024
@Benmuiruri Benmuiruri moved this from Planned to In Progress in Care Teams Oct 23, 2024
@n-orlowski
Copy link

@Benmuiruri let's do with the confirmation input. Also happy to drop the progress bar for MVP (but we should keep the written prompts)

@latin-panda
Copy link
Contributor Author

Adding this to 4.16

@jkuester
Copy link
Contributor

Okay, sorry for the last-minute design questions here, but I am a bit concerned about how the MVP functionality is going to work from a Programs perspective. (Started this discussion in this PR thread, but wanted to continue it here at a higher level for better visibility....)

  • My primary concern is that the can_skip_password_change permission is essentially an "opt-out". By default, users will have to reset their password whenever it is changed by an admin. Unless a project updates their permissions, this password reset behavior will automatically take effect for all users.
    • This seems to be the opposite of the behavior @garethbowen described above "Until then it's fine to use a feature flag so projects can opt-in to try it out." Perhaps I missed more conversations that happened elsewhere....
  • If we are going to make this default behavior, then it really feels like our MVP needs to contain a more sophisticated "out-out" mechanism than a permission. As things are, it really feels like we could seriously complicate things for admins in the field (where getting users logged in has historically already been a problem). Other user management tools I have used (e.g. Keycloak) have included a check-box when modifying a user's password to "Require user to reset password on next login".
    • I understand the security posture is much better with the automatic password reset. However one consequence of this change that seems likely to me is that, for many user accounts, no one will actually remember the password after it gets reset by the user. (I know I struggle with this, myself, if I don't immediately take the time to record a new password that I set.) This will likely result in increased churn in passwords as users will need admins to reset their password each time they need to log in again.
    • If we have decided that we don't want programs to be able to manage passwords for users (for security purposes) it seems like token login links would be less burdensome to both the admins and the end user....
  • Have we gotten feedback from Programs folks on possible impacts that this change will have on various user management workflows? (Maybe this feature originated from talking with programs and is exactly what they need to improve their user management, but I don't want to just assume that...)

Hopefully this has all been previously discussed in threads that I have just missed. If the answer is "yeah, we took all of this into consideration already and decided the current approach is best", then great! I will be happy to move forwards with this, as is.

@jkuester
Copy link
Contributor

Another design question that has come up on the PR is how we should handle basic auth requests from a user that has "password_change_required": true.

The current implementation has added an extra lookup for the _user doc of the user in context to check the password_change_required property on every request to the server. This is needed because a simple _session check will succeed when you use the new password (the one the admin set) and is not sufficient to force you to change your password.

I am wondering if we could be content with not forcing a password reset for basic-auth requests like this. Instead, we only trigger the password-reset flow from the actual login flow. For normal users with a session cookie, when the admin resets their password, they will be forced to login again (and so will be funneled to the password-reset flow). I am just not sure it is worth adding an extra round-trip for every request just to address this basic-auth edge case....

@Benmuiruri
Copy link
Contributor

You raise very pertinent issues Josh. I was concerned about the Programs disruptions as well, but I proceeded with the implementation believing prior discussions had taken place with all stakeholders about how it would work in terms of requiring password reset whenever the password is changed.

Given the situation we are in, time constraints, and this being an MVP that (to the best of my knowledge) none of the partners is particularly waiting for it, how can we create a functional MVP that is not too disruptive? I see two paths here.

Option 1: Put the PR on hold / Close it to resume it next year after further discussions.

Option 2:

  • Change the solution to be permissions based, but make it opt in.
  • Only require password reset on first time login, not when password is changed by admin every time
  • Only enforce the password reset flow in the webapp login without enforcing it in the basic auth for now.

@dianabarsan
Copy link
Member

I agree with Josh on opting-in vs opting-out for the MVP. Especially because we don't have a password reset mechanism put in place.

As for the 2nd point of Basic Auth requests, making this opt-in means that users intentionally created to be used with integrations can be kept opt-out. I'm not super happy with what is happening now though, getting that message "password change required" doesn't really give an indication of what should be done. If I didn't know how this worked, I would just edit the password through the admin console just to get the message again. That's a horrible user experience. I think we really need to come up with something else if we wish to keep this.

Additionally, for the concern that we make an additional request to get the user, for most requests we already need the full user doc, and we get it in subsequent middleware: https://github.com/medic/cht-core/blob/master/api/src/middleware/authorization.js#L66 through getUserSettings.
If we could coalesce these so that we never end up getting _user doc twice, that would involve no performance concerns.

@latin-panda
Copy link
Contributor Author

This feature came up after an unfortunate event where a project got a document with user passwords leaked. It was designed as an opt-out feature because CHT should be secure-first and already have these features by default. It should not disrupt existing users; only new users or those getting a new password from the system admin will go through the process of changing passwords. Like other stuff (old UI design to new design), the opt-out capability is temporary, then removed, and that feature becomes "native".

The token login has been an option but not popular (I don't remember of any project using it right now). This feature remains, and no changes are planned there.

We understand the struggle to learn setting up a password, and that this will require user training. As a second phase to this feature, we thought of added a password suggestion mechanism.

Image

Programs has been in contact with one MoH to start using this feature as soon as it's available. I think it's best to follow up with them to get the latest details. I understand that Programs was very engaged in conversations with this partner about implementing this additional security mechanism, and avoid password leaks.

@latin-panda
Copy link
Contributor Author

About the basic auth question -
I agree with Diana about avoiding getting the doc twice by using that function.
It's okay to keep this Change Password flow to the login page flow as MVP, then straighten the basic auth later. The main intention is to protect CHWs. The basic auth is not less essential but another scope of work since we need to think of the best way to avoid disrupting tools / integrations using the basic auth.

@jkuester
Copy link
Contributor

Okay, so after discussing this more with @latin-panda I now better understand the motivations for making this functionality "opt-out" via a permission instead of "opt-in". This would follow the same pattern we have used for feature flagging other functionality that we intend to eventually be the default experience in the CHT (e.g. the FAB button). This "opt-out" approach will simplify the code maintenance going forward (making it easier to eventually remove the permission) while still allowing deployments the flexibility to avoid taking this behavior immediately if they need to update their operating procedures to work with the new approach. With that in mind, I think we should keep the can_skip_password_change permission in the PR with the understanding that we intend to remove it in the future.

However, I do also think that our base create/update user functionality (e.g. via the REST apis) needs to accept a password_change_required: false property (or something similar) so that it is possible to avoid this functionality for a particular user (regardless of the can_skip_password_change permission. Let the default be password_change_required: true when nothing is set. I am also okay if we do not immediately support any way from the admin app to actually set this flag to false, but from a platform perspective, this feels like an important thing to be able to control (e.g. the cht-user-management tool might need more complex/custom logic to determine who should have to reset their passwords....).

I also want to second Jenni's proposal of pushing the basic auth flows into a new ticket. I think Diana is correct that we might not actually need an additional round-trip to the DB to make this work, but the details on how we want to handle erroring/redirecting a basic-auth request are things that we can sort out in a future ticket and do not need to be in the MVP PR.

@andrablaj andrablaj moved this to In Progress in CHT Stewardship Dec 19, 2024
@latin-panda
Copy link
Contributor Author

The documentation in cht-docs for this feature is here: medic/cht-docs#1711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Security Affects security
Projects
Status: In Progress
Status: In Progress
Status: In Progress
Development

Successfully merging a pull request may close this issue.

6 participants