jmikola / JmikolaAutoLoginBundle

Authenticate users in your Symfony app via a single query parameter (e.g. email and newsletter links).
MIT License
84 stars 16 forks source link

Add support for redirecting to login_path #9

Closed bertoost closed 9 years ago

bertoost commented 9 years ago

When a token can not be found it should be nice to return false or throw an exception and that 'the system' redirects the visitor back to the 'login_path' inside your firewall setup. Now the token key stays in your URL and would like to redirect users to remove that from the URL.

jmikola commented 9 years ago

I'm not sure I understand the request here. The AutoLogin listener is URL agnostic and simply inspects each request for a token (see: AutoLoginListener::isTokenInRequest()). This is a passive approach intended to operate on any path within the application (like RememberMeListener), unlike the firewall listeners that kick in for a specific path (AbstractAuthenticationListener and its children).

bertoost commented 9 years ago

That's not exactly what I mean indeed.. Let me try to explain it a bit more...

When you configured the token to appear as ?_al=abcdef .. The request listener handles the auto login correctly but ?_al stays in the URL. It should be nice if the auto login is redirected to the login_path configured in the config file :-)

henrikbjorn commented 9 years ago

@bertoost that is not always what you want. If you use a autologin url, you proberly want the user to visit a specific url and not have dem redirected.

jmikola commented 9 years ago

Since the library already dispatches a InteractiveLoginEvent (event name: SecurityEvents::INTERACTIVE_LOGIN), which includes the Request object, couldn't you listen on and issue a redirect? I concur with @henrikbjorn that the original purpose of this library was to seamlessly authenticate users as they were accessing a page (e.g. from a mailing list link). I can understand why someone would want the token removed to keep a clean URL, but I believe the necessary tools are already there to facilitate it.

bertoost commented 9 years ago

Sure! But that's where a configuration key can be used for. No problem. Of course I can fix it myself, but it would be nice to have more complete bundles right?

jmikola commented 9 years ago

it would be nice to have more complete bundles

While exposing a boolean flag on the bundle is trivial, this is a matter of adding additional complexity to the core library -- and for a narrow use case (i.e. always redirect after the user becomes authenticated). As is, both the INTERACTIVE_LOGIN and ALREADY_AUTHENTICATED events are passed a GetResponseEvent instance, which I believe makes it trivial to redirect on either event by populating the event with a response object. The event implementation is also more flexible in allowing for custom logic (unlike a boolean flag).

I can revisit this issue if events cannot be used to achieve redirects; however, I'd like to err on the side of not introducing new features unless they are absolutely necessary. For contrast, the introduction of the already authenticated event provided an extension point for the library, and the override behavior was about providing control over the library's essential function.