codeigniter4 / shield

Authentication and Authorization for CodeIgniter 4
https://shield.codeigniter.com
MIT License
351 stars 123 forks source link

Dev: force password reset functionality #522

Closed kenjis closed 1 year ago

kenjis commented 1 year ago

Only auth_identities.force_reset exists.

sammyskills commented 1 year ago

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

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

datamweb commented 1 year ago
  • 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 commented 1 year ago

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:

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:

sammyskills commented 1 year ago

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:

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 commented 1 year ago

Hi all,

I'm still expecting a response regarding the appropriate format to follow: https://github.com/codeigniter4/shield/issues/522#issuecomment-1369390599 or https://github.com/codeigniter4/shield/issues/522#issuecomment-1370006120

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

kenjis commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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.