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

Dev: force password reset functionality #522

Closed
kenjis opened this issue Nov 11, 2022 · 10 comments · Fixed by #601
Closed

Dev: force password reset functionality #522

kenjis opened this issue Nov 11, 2022 · 10 comments · Fixed by #601
Labels

Comments

@kenjis
Copy link
Member

kenjis commented Nov 11, 2022

Only auth_identities.force_reset exists.

@kenjis kenjis added the dev label Nov 11, 2022
@sammyskills
Copy link
Contributor

I will like to work on this, but I have a few questions...

  • Is Shield just an Auth library or a complete User Management library?

For this feature to be complete, I believe there should be a "view" where users can enter the new password.

  • Based on the answer to the first question, should the view be outside (no need to be logged in) OR will it require that a user is logged in before the change of password is made?

@datamweb
Copy link
Collaborator

datamweb commented Jan 2, 2023

  • Is Shield just an Auth library or a complete User Management library?

Shield is Authentication and Authorization library. That's all for now.

This means that Shield strives to create the necessary facilities for implementation in security, speed and less time for developers. Shield tries to be flexible.

This means that the necessary tools for project developers are officially provided by Codeigniter.
To complete the discussion, please refer to #531.

In this regard, I disagree with you. For example, if we are going to have such a view, why don't we have a password recover?
In PR #413 , lonnieezell provided the necessary facilities to implement password change, but did not add any views, controllers, etc.
In this way, the necessary tools were provided to implement password recovery by magicLogin event and magicLogin session.
Any developer can create it or not according to the need.

So it seems to be enough to create the necessary tool for developers to use, if it doesn't really exist.

@lonnieezell
Copy link
Member

Is Shield just an Auth library or a complete User Management library?

Shield is an auth library, not a complete User Management library, as that would imply we provide the admin pages to manage the users, and possibly even the front-end pages for a user to manage their own account. Both of which are highly dependent on the application itself, the front-end tech used, etc.

For this feature to be complete, I believe there should be a "view" where users can enter the new password.

Agree - much like we have login and register pages, this would require front-end views.

Based on the answer to the first question, should the view be outside (no need to be logged in) OR will it require that a user is logged in before the change of password is made?

For our purposes we first need to actually build out password reset functionality first. This wasn't included in the original code but was always thought that it might be needed and the magic link would not be enough.

So for password reset we should provide something along the following lines:

  • config setting to allow password reset.
  • If the setting is true, should show a link within the Login view.
  • Would need a view to ask for the user's email address.
  • Record a temporary user identity with the email and the hashed version of the password reset code. A user should be able to have multiple of these identities at once so that they could request it a couple of times and see the first email and still use that code. Use the Passwords::hash function for the hashing here so the reset code is treated with the same care as a password.
  • config setting to set how long the reset code is valid for, in minutes - default of 60 is probably fine
  • When the user clicks the link in the email it should verify the code and the email address. If it cannot find a match should redirect to the first view with a message telling them why. If it can verify the code and email against what is sent in the link, it should present a form asking for a new password and and password confirmation.
  • Once that has been validated and the user updated with the new password, ALL password reset identities for this user should be deleted.
  • Log the user in automatically.

All views and emails should use lang strings so they are easily translated.
The views should be added to the list of views within the Auth config file so devs can easily change them.
This needs tests on every step of the pathway. We can provide guidance when you get there if you need it.

References:

That should be a single PR to get that flow working correctly.

Once that is in place, then we can focus on the force reset functionality, which would:

  • Check for the force reset flag within the session and access tokens filters. This way it doesn't wait until the user logs in again to force a reset, but takes place immediately.
  • If they are flagged, then it should automatically create the password identity and send them the email, then display a page telling them they must reset their password and to look for the email.
  • The rest of the process is handled by the above password reset flow.

@sammyskills
Copy link
Contributor

Thanks @lonnieezell for the breakdown, but I think I like the point raised by @datamweb:

In this regard, I disagree with you. For example, if we are going to have such a view, why don't we have a password recover? In PR #413 , lonnieezell provided the necessary facilities to implement password change, but did not add any views, controllers, etc. In this way, the necessary tools were provided to implement password recovery by magicLogin event and magicLogin session. Any developer can create it or not according to the need.

So it seems to be enough to create the necessary tool for developers to use, if it doesn't really exist.

In a bid to maintain the goal of shield, which is to remain flexible, not imposing stuffs on developers, while not compromising on security, I think it will be better we follow the same process of the magicLogin. Here are some points I think we should consider:

  • Forcing users to reset their passwords should be an internal call, i.e., the check should be only when the user is successfully logged in.
  • This can be like a flag within the session or event or both, that a developer can use to do whatever he/she deems necessary.
  • With this, we are still maintaining the magicLogin if a user cannot remember their password.
  • The actual view to change a password will (most likely) be within the application's user profile settings area, which should be provided (if not already available) by the developer.

Again, these are all my personal opinion as I am not in any way a security expert. So let me know what you think @lonnieezell @datamweb @kenjis @MGatner.

@sammyskills
Copy link
Contributor

sammyskills commented Jan 6, 2023

Hi all,

I'm still expecting a response regarding the appropriate format to follow: #522 (comment) or #522 (comment)

cc: @kenjis, @lonnieezell, @datamweb, @MGatner

@kenjis
Copy link
Member Author

kenjis commented Jan 6, 2023

Forcing users to reset their passwords should be an internal call, i.e., the check should be only when the user is successfully logged in.

What do you mean? It seems different from what lonnie says:

Check for the force reset flag within the session and access tokens filters. This way it doesn't wait until the user logs in again to force a reset, but takes place immediately.

@sammyskills
Copy link
Contributor

What do you mean? It seems different from what lonnie says:

Yes, it is different from what @lonnieezell said.

My idea is that, forcing users to reset their passwords should come after the user has been successfully logged in, i.e., the force reset flag should only be called after a user has been successfully authenticated.

@lonnieezell
Copy link
Member

I'm ok with do a smaller implementation, like we do with Magic Links, at the moment. I have a feeling we'll need the rest at some point, though.

Checking the force reset flag within the filters would only happen if a user was authenticated. Doing it within the filters ensures it is checked on every page view by a user. This helps avoid scenarios like:

  • user logs in, selects remember me, and they now have a valid session for 30 days, or whatever.
  • a possible security breach happens a week later. The admins do a global force reset on all users to help protect their accounts.
  • The user visits the site again. They don't have to login so any checks at the login process would be skipped. However, the filters do run and see they now need to reset their password.
  • Ideally, it should log them out at this point and not let them in until the password has been reset, but if we're going the simpler route and leave it to developers to implement, then we would just need to fire an event or something to let them know. My worry there is that they might be able to skip it, though I suppose at some point the message they receive, or redirects would be enough of an annoyance they probably would take it seriously. :)

I would push harder for a non-authenticated password reset flow, but at some point soon I'd like to dig into biometric, password-less authentication, which seems like a better, more forward-facing use of effort than a manual password reset.

@datamweb
Copy link
Collaborator

datamweb commented Jan 8, 2023

lonnieezell explanation was logical and complete enough. I think because of his myth-auth&Bonfire2 experience, he knows more than us what the codeigniter community needs.

@sammyskills Can you implement according to the description of lonnieezell ?

@kenjis
Copy link
Member Author

kenjis commented Jan 8, 2023

A smaller PR is better.
Larger PRs are less likely to be merged because no one can review them, and they take longer to get merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants