8000 Why is security.authentication.success_handler abstract? · Issue #11926 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Why is security.authentication.success_handler abstract? #11926

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.

Sign up for GitHub

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

Jump to bottom
Closed
ftdysa opened this issue Sep 15, 2014 · 9 comments
Closed

Why is security.authentication.success_handler abstract? #11926

ftdysa opened this issue Sep 15, 2014 · 9 comments

Comments

@ftdysa
Copy link
ftdysa commented Sep 15, 2014

Why is this Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler class service definition declared as abstract? I am trying to decorate it with the following but am encountering the exception displayed below:

services:
    project.security.handler.login_success:
        public: false
        class: Project\WebBundle\Security\Authentication\Handler\LoginSuccessHandler
        decorates: security.authentication.simple_success_failure_handler
        arguments: ["@project.security.handler.login_success.inner", @project.manager.user]

This then throws:

RuntimeException: The definition "project.security.handler.login_success" has a reference to an abstract definition "project.security.handler.login_success.inner". Abstract definitions cannot be the target of references.

And here is the definition from the SecurityBundle

<service id="security.authentication.success_handler" class="%security.authentication.success_handler.class%" abstract="true" public="false">
    <argument type="service" id="security.http_utils" />
    <argument type="collection" /> <!-- Options -->
 </service>

My goal is to get the configured firewall map options (login_path, default_target_path, etc) in my own success handler. Is there a better way to do this?
@stof
Copy link
Member
stof commented Sep 15, 2014

because it is an incomplete definition. It is just a prototype which will be used by real success handler services (one per firewall needing it): https://github.com/symfony/symfony/blob/05c720734bae7f96edb5a3fb9644d360f6301336/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/AbstractFactory.php#L171-184

@stof
Copy link
Member
stof commented Sep 15, 2014

this means you should decorate the real service, not the parent prototype

@ftdysa
Copy link
Author
ftdysa commented Sep 15, 2014

Which is the real service? I'm having trouble locating it. I also realized I have the failure handler in the above example which is also incorrect, should have been security.authentication.success_handler.

Also, should my service be extending the decorated service or just implementing AuthenticationSuccessHandlerInterface?

services:
    project.security.handler.login_success:
        class: Project\WebBundle\Security\Authentication\Handler\LoginSuccessHandler
        decorates: security.authentication.success_handler
        arguments: ["@project.security.handler.login_success.inner", @project.manager.user]

Throws

RuntimeException: The definition "project.security.handler.login_success" has a reference to an abstract definition "project.security.handler.login_success.inner". Abstract definitions cannot be the target of references.

@jakzal
Copy link
Contributor
jakzal commented Sep 15, 2014

Closing as this is really a support question.

@jakzal jakzal closed this as completed Sep 15, 2014
@ftdysa
Copy link
Author
ftdysa commented Sep 16, 2014

This may very well be a support question, but no tips? No one in IRC has an idea as well.

None of the success handler services are non-abstract (that I've seen). I'd think it would be a simple compiler pass but calling the following is showing an empty array for the options when I would assume they would be populated with my configured options.

var_dump($container->getDefinition('security.authentication.success_handler')->getArguments());

I've obviously been able to make it work by hard coding the options but that is less than ideal.

@stof
Copy link
Member
stof commented Sep 16, 2014

See the code I linked. It is the place where the successHandler service is built, including its id (FYI, the îd argument contains the firewall name)

@stof
Copy link
Member
stof commented Sep 16, 2014

and no, the prototype definition does not get the options set in the arguments, as this would share them between all success handlers. the options are set only in the real service inheriting from the prototype

@ftdysa
Copy link
Author
ftdysa commented Sep 16, 2014

Thanks for your comments stof.

I'm still a bit lost on how I accomplish this. Do I need to create my own factory in my bundle that builds my service like in the link that you showed above? That seems a bit overkill but I'll try going down that route. I tried loading the SecurityBundle configuration in my Bundle's extension last night and that didn't go over so well.

It seems like it'd almost be easier to just use the yaml parser and parse out that array at this point, but again that seems hacky.

@ftdysa
Copy link
Author
ftdysa commented Sep 23, 2014

FYI, this #11993 is what I need. For now since I only have one success handler I just overwrote the security.authentication.success_handler service with my own and passed my other dependency.

This was referenced Sep 23, 2014
Merged
Closed
fabpot added a commit that referenced this issue Sep 25, 2014
@fabpot
feature #11993 [Security] make it possible to override the default su…
af0aa50 
…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
@silverbackdan silverbackdan mentioned this issue Jul 21, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jakzal @stof @ftdysa
0