Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

Custom Password Resetting #12

Closed
browner12 opened this issue Mar 15, 2016 · 9 comments
Closed

Custom Password Resetting #12

browner12 opened this issue Mar 15, 2016 · 9 comments

Comments

@browner12
Copy link

While I think the scaffolding that Laravel provides for resetting passwords is great, and excellent work has been done to make it as customizable as possible, I think some tweaks could be made to give users full control over it.

There are two reason why I am bringing this up. First off, even though a lot of customization is built in, I think it's difficult or impossible to provide all of the customization an end user could want. The second reason is because most of us have a certain way of building our sites, and building our controllers, services, etc. With the built in password controller, now we have one spot in our app that behaves differently than every where else, which makes it a little harder on the developer (or future developers) to immediately know what's going on.

To reset a password, the programmers essentially cares about two things. Creating a unique token, and then checking that token. Emailing, updating the password, rendering views, redirecting, and the like are all more superfluous things that Laravel already makes it easy for us to do. So what I would propose is the PasswordBroker has 2 public methods for these things.

$broker->generateToken(CanResetPasswordContract $user); //returns a string

which would create the token and store it to the database, and

$broker->validateToken($credentials); //returns a boolean

which would validate the passed in token against the database and then delete it if valid.

While this way of handling password resets wouldn't be for everyone, it would give users who want full control these easy to use methods, and then they are required to handle everything else (emails, views, redirecting).

I just currently did basically this with a project of mine (I needed more control of the emailing), and although it was doable, it was not easy like I've grown accustomed to with Laravel. I'm assuming others may have done this as well, so looking for some feedback.

Just to be clear, I am not arguing to get rid of the current scaffolding, but rather just give tools to the users who wish to implement it themselves.

Thanks!

@jeroenherczeg
Copy link

I have to second this. It wasn't easy to implement the password reset the way I wanted to. I wanted to have some more control over the mailing of the token (using a template from campaign monitor) and the routing. I ended up writing my own but having these to functions available would have saved me some time.

for the second function I would opt for passing the user and the token separately.

@judgej
Copy link

judgej commented Mar 16, 2016

Would my issue be something that this proposal would help solve?

When requesting a password reset token, I need the system to check if there is already an active (non-expired) token already available, and use that in preference to creating a new one every single time. The current approach (from L4.2) which deletes any current tokens every single time a password reset request is sent, has resulted in many support issues for us, from users with home ISPs that happen to be slow (we are only talking minutes) in delivering the password reset emails.

@browner12
Copy link
Author

this proposal would not solve your issue. that would be a separate change to how Laravel internally handles the token generation and regeneration. this proposal wants to still be abstracted from that, but have public methods to generate and validate the token.

@judgej
Copy link

judgej commented Mar 16, 2016

Ah, okay, I just assumed the public method could be used in my own broker to check for an existing token to use, when being asked to generate one. Maybe I just misunderstood.

Edit: I've just implemented a solution for my site, by extending the ReminderServiceProvider to use an extended DatabaseReminderRepository with a slightly customised create() method. Personally I suspect the non-reuse of existing active password-reset tokens is an issue for many people but without being explicitly identified as a problem. Where would I raise this for discussion - internals or the the main repository?

@browner12
Copy link
Author

i mean, what your asking is technically possible. i just hadn't thought of it for this proposal, and i'm not sure if there was a reason they switched away from it in the first place

@judgej
Copy link

judgej commented Mar 17, 2016

I've raised it as a separate issue. We'll see what comes of it. I can do a PR if needed, or just blog the solution somewhere if not seen important enough to change (or a security risk). Ta.

@Rjs37
Copy link

Rjs37 commented Apr 6, 2016

I think this would be a really beneficial change. I already had a controller, views etc in place for resetting a password and I was hoping to simply switch from Sentinel to Laravel's own authorisation.

With the two publicly accessible functions described above, that would make that process a lot easier.

@browner12
Copy link
Author

my PR for this was merged into 5.2 today.

laravel/framework@0152bbf

the method signatures have changed slightly so, make sure you check them out. Also added a delete method, so validating doesn't automatically delete the token, you have to handle it yourself.

@jeroenherczeg
Copy link

Great job!

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

No branches or pull requests

4 participants