Skip to content

Dev: force password reset functionality #522

Closed

Description

@kenjis

Only auth_identities.force_reset exists.

Activity

  1. sammyskills commented on Jan 2, 2023

    @sammyskills
    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?
  2. datamweb commented on Jan 2, 2023

    @datamweb
    Collaborator
    • 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.

  3. lonnieezell commented on Jan 3, 2023

    @lonnieezell
    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.
  4. sammyskills commented on Jan 3, 2023

    @sammyskills
    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.

  5. sammyskills commented on Jan 6, 2023

    @sammyskills
    Contributor

    Hi all,

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

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

  6. kenjis commented on Jan 6, 2023

    @kenjis
    MemberAuthor

    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.

  7. sammyskills commented on Jan 6, 2023

    @sammyskills
    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.

  8. lonnieezell commented on Jan 6, 2023

    @lonnieezell
    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.

  9. datamweb commented on Jan 8, 2023

    @datamweb
    Collaborator

    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 ?

  10. kenjis commented on Jan 8, 2023

    @kenjis
    MemberAuthor

    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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions