Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Success/failure handlers are not documented #4258

Closed
fabpot opened this issue Sep 23, 2014 · 11 comments
Closed

Success/failure handlers are not documented #4258

fabpot opened this issue Sep 23, 2014 · 11 comments
Labels
actionable Clear and specific issues ready for anyone to take them. hasPR A Pull Request has already been submitted for this issue. Security

Comments

@fabpot
Copy link
Member

fabpot commented Sep 23, 2014

Working on fixing symfony/symfony#10417, I wanted to update the documentation but there is no documentation for custom success/failure handlers for the firewall configuration. The success_handler and success_failure configuration settings are mentioned in configuration examples, but there is no explanation whatsoever.

I think we need to document them, but only as of Symfony 2.6 as they are less useful before. So, we need to document them in a new cookbook entry, based on the changes done in symfony/symfony#11993.

@wouterj
Copy link
Member

wouterj commented Sep 23, 2014

We have an open issue for this in #802 We had some issues finding good usecases for such listeners, do you know anything?

@fabpot
Copy link
Member Author

fabpot commented Sep 24, 2014

fabpot added a commit to symfony/symfony that referenced this issue Sep 25, 2014
…ccess/failure handler (fabpot)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[Security] make it possible to override the default success/failure handler

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #5432, #9272, #10417, #11926
| License       | MIT
| Doc PR        | symfony/symfony-docs#4258

Overriding the default success/failure handler of the security firewalls is possible via the `success_handler` and `failure_handler` setting but this approach is not flexible as it does not allow you to get the options/provider key.

To sum up the problem:

* Overriding the default success/failure handler is possible via a service;
* When not overridden, the default success/failure handler gets options and the provider key;
* Those options and the provider key are injected by the factory as they are dynamic (they depend on the firewall and the provider key), so getting those options/provider key is not possible for a custom service that is only configured via the container configuration;
* Extending the default handler does not help as the injection mechanism is only triggered when no custom provider is set;
* Wrapping the default handler is not possible as the service id is dynamic.

... and of course we need to keep BC and make it work for people extending the default handler but also for people just using the interface.

Instead of the current PR, I propose this slightly different approach. It's not perfect, but given the above constraint, I think this is an acceptable trade-of.

So, several use cases:

 * Using the default handler (no change);
 * Using a custom handler that implements `AuthenticationSuccessHandlerInterface` directly and does not need any options (no change);
 * Using a custom handler that needs the options/provider key (that's the new use case this PR supports).

This PR introduces 2 new classes that wrap custom handlers. If those classes define the `setOptions()` and/or `setProviderKey()` methods, they are automatically called with the correct arguments. Yours handler does not need to extend the default handler `DefaultAuthentication*Handler`, but doing so helps as the setters are already defined there.

Commits
-------

810eeaf [Security] made it possible to override the default success/failure handler (take 2)
36116fc [Security] made it possible to override the default success/failure handler
fabpot added a commit to symfony/security-http that referenced this issue Sep 25, 2014
…ccess/failure handler (fabpot)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[Security] make it possible to override the default success/failure handler

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #5432, #9272, #10417, #11926
| License       | MIT
| Doc PR        | symfony/symfony-docs#4258

Overriding the default success/failure handler of the security firewalls is possible via the `success_handler` and `failure_handler` setting but this approach is not flexible as it does not allow you to get the options/provider key.

To sum up the problem:

* Overriding the default success/failure handler is possible via a service;
* When not overridden, the default success/failure handler gets options and the provider key;
* Those options and the provider key are injected by the factory as they are dynamic (they depend on the firewall and the provider key), so getting those options/provider key is not possible for a custom service that is only configured via the container configuration;
* Extending the default handler does not help as the injection mechanism is only triggered when no custom provider is set;
* Wrapping the default handler is not possible as the service id is dynamic.

... and of course we need to keep BC and make it work for people extending the default handler but also for people just using the interface.

Instead of the current PR, I propose this slightly different approach. It's not perfect, but given the above constraint, I think this is an acceptable trade-of.

So, several use cases:

 * Using the default handler (no change);
 * Using a custom handler that implements `AuthenticationSuccessHandlerInterface` directly and does not need any options (no change);
 * Using a custom handler that needs the options/provider key (that's the new use case this PR supports).

This PR introduces 2 new classes that wrap custom handlers. If those classes define the `setOptions()` and/or `setProviderKey()` methods, they are automatically called with the correct arguments. Yours handler does not need to extend the default handler `DefaultAuthentication*Handler`, but doing so helps as the setters are already defined there.

Commits
-------

810eeaf [Security] made it possible to override the default success/failure handler (take 2)
36116fc [Security] made it possible to override the default success/failure handler
fabpot added a commit to symfony/security that referenced this issue Sep 25, 2014
…ccess/failure handler (fabpot)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[Security] make it possible to override the default success/failure handler

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #5432, #9272, #10417, #11926
| License       | MIT
| Doc PR        | symfony/symfony-docs#4258

Overriding the default success/failure handler of the security firewalls is possible via the `success_handler` and `failure_handler` setting but this approach is not flexible as it does not allow you to get the options/provider key.

To sum up the problem:

* Overriding the default success/failure handler is possible via a service;
* When not overridden, the default success/failure handler gets options and the provider key;
* Those options and the provider key are injected by the factory as they are dynamic (they depend on the firewall and the provider key), so getting those options/provider key is not possible for a custom service that is only configured via the container configuration;
* Extending the default handler does not help as the injection mechanism is only triggered when no custom provider is set;
* Wrapping the default handler is not possible as the service id is dynamic.

... and of course we need to keep BC and make it work for people extending the default handler but also for people just using the interface.

Instead of the current PR, I propose this slightly different approach. It's not perfect, but given the above constraint, I think this is an acceptable trade-of.

So, several use cases:

 * Using the default handler (no change);
 * Using a custom handler that implements `AuthenticationSuccessHandlerInterface` directly and does not need any options (no change);
 * Using a custom handler that needs the options/provider key (that's the new use case this PR supports).

This PR introduces 2 new classes that wrap custom handlers. If those classes define the `setOptions()` and/or `setProviderKey()` methods, they are automatically called with the correct arguments. Yours handler does not need to extend the default handler `DefaultAuthentication*Handler`, but doing so helps as the setters are already defined there.

Commits
-------

810eeaf [Security] made it possible to override the default success/failure handler (take 2)
36116fc [Security] made it possible to override the default success/failure handler
fabpot added a commit to symfony/security-bundle that referenced this issue Sep 25, 2014
…ccess/failure handler (fabpot)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[Security] make it possible to override the default success/failure handler

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #5432, #9272, #10417, #11926
| License       | MIT
| Doc PR        | symfony/symfony-docs#4258

Overriding the default success/failure handler of the security firewalls is possible via the `success_handler` and `failure_handler` setting but this approach is not flexible as it does not allow you to get the options/provider key.

To sum up the problem:

* Overriding the default success/failure handler is possible via a service;
* When not overridden, the default success/failure handler gets options and the provider key;
* Those options and the provider key are injected by the factory as they are dynamic (they depend on the firewall and the provider key), so getting those options/provider key is not possible for a custom service that is only configured via the container configuration;
* Extending the default handler does not help as the injection mechanism is only triggered when no custom provider is set;
* Wrapping the default handler is not possible as the service id is dynamic.

... and of course we need to keep BC and make it work for people extending the default handler but also for people just using the interface.

Instead of the current PR, I propose this slightly different approach. It's not perfect, but given the above constraint, I think this is an acceptable trade-of.

So, several use cases:

 * Using the default handler (no change);
 * Using a custom handler that implements `AuthenticationSuccessHandlerInterface` directly and does not need any options (no change);
 * Using a custom handler that needs the options/provider key (that's the new use case this PR supports).

This PR introduces 2 new classes that wrap custom handlers. If those classes define the `setOptions()` and/or `setProviderKey()` methods, they are automatically called with the correct arguments. Yours handler does not need to extend the default handler `DefaultAuthentication*Handler`, but doing so helps as the setters are already defined there.

Commits
-------

810eeaf [Security] made it possible to override the default success/failure handler (take 2)
36116fc [Security] made it possible to override the default success/failure handler
@weaverryan
Copy link
Member

The use-case I just had (which seems reasonable) is authenticating via AJAX, and so wanting a JSON response back in those cases. But, for non-AJAX, I want to have the normal redirect functionality.

class CustomAuthenticationFailureHandler extends DefaultAuthenticationFailureHandler
{
    /**
     * @var TranslatorInterface
     */
    private $translator;

    public function setTranslator(TranslatorInterface $translator)
    {
        $this->translator = $translator;
    }

    public function onAuthenticationFailure(Request $request, AuthenticationException $exception)
    {
        if ($request->isXmlHttpRequest()) {
            return new JsonResponse(array(
                'authenticated' => false,
                'error' => $this->translator->trans($exception->getMessageKey(), $exception->getMessageData()),
            ));
        }

        return parent::onAuthenticationFailure($request, $exception);
    }
}
services:
    security.custom_auth_failure_handler:
        class: Knp\UserBundle\Security\Authentication\CustomAuthenticationFailureHandler
        arguments:
            - "@http_kernel"
            - "@security.http_utils"
        calls:
            - [setTranslator, ["@translator"]]

And of course, failure_handler is set to this service id.

If you don't want to extend DefaultAuthenticationFailureHandler, then you can just implement AuthenticationFailureHandlerInterface. But the key is that if you have a setOptions(array $options) method on your failure handler, Symfony will call that and pass you all the things like failure_path, etc. That's the change that happened in symfony/symfony#11993 (which is also why extending DefaultAuthenticationFailureHandler works).

So, I think this can be added, though maybe we should just doc it for 2.6, and backport a few pieces to 2.3. Marking this as actionable :). Imo, we should work these into this article: http://symfony.com/doc/current/cookbook/security/form_login.html, and maybe rename it - it's really all about handling redirect behavior after login - maybe "How to control what happens after a user logs in (or fails login)"

@weaverryan weaverryan added good first issue Ideal for your first contribution! (some Symfony experience may be required) actionable Clear and specific issues ready for anyone to take them. labels Mar 17, 2015
@wouterj wouterj removed the good first issue Ideal for your first contribution! (some Symfony experience may be required) label May 2, 2015
@nicolas-san
Copy link

@wouterj I want to redirect my user on different route based on is role.
One have access to a dashboard, another just is profile page.

So, after some search, I use a custom handler:

https://github.com/nicolas-san/authSuccessHandlerRedirect/blob/master/MyBundle/Handler/LoginSuccessHandler.php

(I use Fos user bundle, my route for profile is fos_user_profile_show, for general purpose, I can change it to more basic route like "profile".)

Perhaps this usecase is ok for the doc ?
And if you have some comment on my handler, I'll take them :)

@ABM-Dan
Copy link

ABM-Dan commented Dec 13, 2016

This might be made easier to explain if we had interfaces exposing setOptions and setProviderKey.

javiereguiluz added a commit that referenced this issue Nov 6, 2018
This PR was squashed before being merged into the 2.8 branch (closes #10631).

Discussion
----------

Document logout success_handler configuration

The [Security docs](https://symfony.com/doc/current/security.html#logging-out) tell how to control what happens after logging out, but the security configuration reference is missing documentation on the logout success_handler.

_Partially_ related to #4258

Commits
-------

b93452a Document logout success_handler configuration
@ureimers
Copy link

ureimers commented Nov 22, 2018

As this issue is still open (because of lack of a good use case?) maybe this one will do:

My use case is that new user's need to confirm their email address first by receiving and following a standard double-opt-in mail. If a user then confirms her email address, a flag isEmailConfirmed is set to true.

When a user logs in a simple UserChecker checks the flag on post authentication:

final class UserChecker implements UserCheckerInterface
{
    public function checkPostAuth(UserInterface $user)
    {
        if (!$user->isEmailConfirmed()) {
            throw new EmailNotConfirmedException();
        }
    }

    public function checkPreAuth(UserInterface $user)
    {
        // Nothing to do before authentication. Yet.
    }
}

I then use a custom AuthenticationFailureHandler to handle that specific exception and redirect the user to a page where she can resend her confirmation email. As I still want to use the default logic for every other scenario, I extend the DefaultAuthenticationFailureHandler:

final class AuthenticationFailureHandler extends DefaultAuthenticationFailureHandler
{
    public function onAuthenticationFailure(Request $request, AuthenticationException $exception)
    {
        if ($exception instanceof EmailNotConfirmedException) {
            return $this->httpUtils->createRedirectResponse($request, 'app_resend_confirmation_email');
        }

        return parent::onAuthenticationFailure($request, $exception);
    }
}

Both of the mechanisms used show how easy it is to accomplish both, a post authentication check and redirecting the user because of what the post authentication request might have thrown. Just beautiful :-)

There are other scenarios like this where you want to present a user a dedicated page if a specific requirement for a successful login wasn't met. Without the custom failure handler, the EmailNotConfirmedException's message would simply be displayed as an error of the login form.

Additional notes:

It seems that with Symfony 3.4 the mentioning of the handler was removed entirely from the default security configuration documentation. Is there a reason for that? Without looking through the source code of the Security Component I wouldn't have found out about this very useful configuration method.

Furthermore this comment from Fabien should be mentioned in a possible documentation too. It's easy to miss so I'll add it here again:

There is one slight side-effect: as we are passing options and the provider key, you must not use the same custom handler in two different firewalls.

@carsonbot
Copy link
Collaborator

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@carsonbot
Copy link
Collaborator

Just a quick reminder to make a comment on this. If I don't hear anything I'll close this.

@wouterj
Copy link
Member

wouterj commented Jan 2, 2021

Success/failure handlers are still relevant in the new security system and definitely deserve some documentation.

@carsonbot carsonbot removed the Stalled label Jan 2, 2021
@javiereguiluz
Copy link
Member

Closing in favor of #15908, a meta-issue that groups all pending security-related issues so we can easily check them.

javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this issue Oct 4, 2023
…led authentication (alexandre-daubois)

This PR was merged into the 5.4 branch.

Discussion
----------

[Security] Mention customizing successful and failed authentication

Fix symfony#4258, part of symfony#15908

Commits
-------

3526742 [Security] Mention customizing successful and failed authentication
@javiereguiluz
Copy link
Member

Closing as fixed in #18931.

@xabbuh xabbuh added the hasPR A Pull Request has already been submitted for this issue. label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionable Clear and specific issues ready for anyone to take them. hasPR A Pull Request has already been submitted for this issue. Security
Projects
None yet
Development

No branches or pull requests

9 participants