laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

[Proposal] Convert LoginController/AuthenticatesUsers process to pipeline #1620

Closed jrmadsen67 closed 3 years ago

jrmadsen67 commented 5 years ago

I would like to propose changing the current system of traits in AuthenticatesUsers, et al to a simple pipeline much along the lines of what Middleware does now.

This is very much just the idea stage, not proposed code.

Overall plan would be to convert AuthenticatesUsers::login() from the step-by-step procedural code to a loop/pipeline that checked a group of classes. The would be registered (in the LoginController? config/auth?) and so all of the login conditions could be easily added/swapped out just like middleware.

I think this would make the authentication system much easier to understand and extend, and make the rules themselves reusable.


Actual implementation:

I think the easiest first step would be do make a contract & add the looping code directly above the incrementLoginAttempts(); this would serve as a hook for moving forward while remaining backward compatible.

The next part would be to shift the existing checks onto new classes while working out how to capture customized methods currently being overwritten via $this->something() in the LoginController


Leave that there for thoughts, ideas, and definitely anything I may have overlooked.

sisve commented 5 years ago

I don't understand what you're trying to accomplish. You're talking about changing the code, but it's not clear what new scenarios you're attempting to support. Are there not enough extension points for AuthenticatesUsers::login? It already calls the RateLimiter and the Guard classes so you can already change a lot of behavior of the method, and you can override the existing methods too.

jrmadsen67 commented 5 years ago

it's not so much that it does anything new, it is that it does it in a cleaner, easier to work with way.

Here's an Elixir library that works similar to what I'm suggesting:

https://github.com/ueberauth/guardian#pipelines

there are a lot of small rules that people commonly add ("email confirmed" for example) that mean they always have to override the existing functions. I think the idea of being able to encapsulate those rules into reusable classes and simply include in them an array like the Middleware's kernel would be much nicer to work with.

to expand - you may always like to change the way the new sign up emails go out, or the password resets work (neither of those directly in this first request, but an obvious next step). currently it is a cumbersome process to keep setting up things the way you like them; with this, they become composer libraries you pull in & add to the "kernel" array

I think it is better code when you can "add to" rather than overwrite.

martinbean commented 5 years ago

I’d be very much in favour of this.

I recently worked on a project where I needed to do something slightly different during the login process, but to do so I had to copy and paste the entire login() method (event dispatching, authenticating after validation, etc) to my application’s LoginController. If the login process was pipeline-like, then I’d have been able to change the “slice” in the pipeline I needed to.

It also means my application’s login controller won’t break if the implementation in the AuthenticatesUsers trait is changed, as that’s something I now have a dependency on, need to be vigilant of, and make sure my implementation stays inline with the framework’s implementation.

jrmadsen67 commented 3 years ago

Given the re-writes of the authentication section, I'd suggest this prolly isn't valid any more, so going to close