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

[Security] Configuring a user checker per firewall #14721

Closed
wants to merge 10 commits into from

Conversation

@linaori
Copy link
Contributor

commented May 22, 2015

Changed my base branch to avoid issues, closed old PR

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed ticket #11090 and helps #14673
License MIT
Doc PR symfony/symfony-docs/pull/5530

This pull request adds support for a configurable user checker per firewall. An example could be:

services:
    app.user_checker:
        class: App\Security\UserChecker
        arguments:
            - "@request_stack"

security:
    firewalls:
        secured_area:
            pattern: ^/
            anonymous: ~
            basic_auth: ~
            user_checker: app.user_checker

The above example will use the UserChecker defined as app.user_checker. If the user_checker option is left empty, security.user_checker will be used. If the user_checkers option is not defined, it will fall back to the original behavior to not break backwards compatibility and will validate using the existing UserChecker: security.user_checker.

I left the default argument in the service definitions to be security.user_checker to include backwards compatibility for people who for some reason don't have the extension executed. You can obtain the checker for a specific firewall by appending the firewall name to it. For the firewall secured_area, this would be security.user_checker.secured_area.

@@ -65,6 +65,7 @@ protected function createAuthProvider(ContainerBuilder $container, $id, $config,
$container
->setDefinition($provider, new DefinitionDecorator('security.authentication.provider.dao'))
->replaceArgument(0, new Reference($userProviderId))
->replaceArgument(1, new Reference('security.chain_user_checker.'.$id))

This comment has been minimized.

Copy link
@weaverryan

weaverryan May 22, 2015

Member

Won't this mean that the ChainUserChecker is always injected into this class? For BC, if the new user_checkers isn't specified, it should still inject the exact same UserChecker class as it did before.

This comment has been minimized.

Copy link
@linaori

linaori May 22, 2015

Author Contributor

That's correct, I did that on purpose. The 4 services using this are most likely not going to break BC, all of them are abstract and only created/defined through the 4 factories here. As far as I know, it's very, very difficult to properly extend those classes and even if you do, it's their own mistake if they use the UserChecker as interface instead of the interface as defined in all 4 classes.

All custom implementations will still use security.user_checker and decorating that will still work as it's simply added as default when not defined. I don't see anything break here, but maybe you can give an example which I might have missed.

If I don't inject the ChainUserProvider if it's not defined, I won't be able to add a default configuration (which will work as it's just the default user checker being used).

This comment has been minimized.

Copy link
@weaverryan

weaverryan May 23, 2015

Member

@iltar Very well explained. Fwiw, I agree with you: the only BC break would be if you overrode these core services, then somehow type-hinted something internally in your overridden class to the concrete UserProvider. And I don't believe that is really a BC break. So I agree with you - no issue here :)

@weaverryan

This comment has been minimized.

Copy link
Member

commented May 23, 2015

@iltar I think we should add a bit more docs to UserCheckerInterface - specifically, to explain (briefly) that checkPreAuth and checkPostAuth should throw AccountStatusException if there is any issue.

*
* @author Iltar van der Berg <ivanderberg@hostnet.nl>
*/
final class ChainUserChecker implements UserCheckerInterface

This comment has been minimized.

Copy link
@weaverryan

weaverryan May 23, 2015

Member

I don't think there's a precedence in the core for this to be final. We only use final on classes like FormEvents or classes that are basically nice factories to help you get started (e.g. PropertyAccess).

This comment has been minimized.

Copy link
@linaori

linaori May 23, 2015

Author Contributor

I designed this to not be an extension point. It's supposed to be a simple delegator. It also prevents people from looking for a way to extend it, while composition might be nicer (though, won't work for this particular case).

I found that this works rather nice for the core as it's not part of the public API: http://ocramius.github.io/blog/when-to-declare-classes-final/

This comment has been minimized.

Copy link
@weaverryan

weaverryan May 23, 2015

Member

well, it certainly can't hurt - someone could always ask us to not make it final in the future. Anyways, we'll see what others think.

This comment has been minimized.

Copy link
@linaori

linaori May 23, 2015

Author Contributor

I hope that by making it final, people will wonder why and ask (on IRC). We can then explain why it's final and help the developer with his design and make suggestions on what to do.

@@ -215,6 +215,11 @@ private function addFirewallsSection(ArrayNodeDefinition $rootNode, array $facto
->prototype('scalar')->end()
->end()
->booleanNode('security')->defaultTrue()->end()
->arrayNode('user_checkers')
->defaultValue(array('security.user_checker'))
->info('A list of user checkers reserved for this firewall.')

This comment has been minimized.

Copy link
@weaverryan

weaverryan May 23, 2015

Member

A list of user checker service ids to use when authenticating users in this firewall.

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2015

Docs are added, for some reason the tests timed out. Any idea how to rebuild without a push? :(

@fabpot

This comment has been minimized.

Copy link
Member

commented Jul 16, 2015

@iltar Can you rebase to get rid of the merge commit? That's a blocker for the merge.

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2015

@fabpot done

'Symfony\\Bundle\\SecurityBundle\\Security\\FirewallContext',
'Symfony\\Component\\HttpFoundation\\RequestMatcher',
'Symfony\Component\Security\Http\Firewall',
'Symfony\Component\Security\Core\SecurityContext',

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 16, 2015

Member

Should this be here?

This comment has been minimized.

Copy link
@linaori

linaori Jul 16, 2015

Author Contributor

As long as the SecurityContext is a service, IMO yes. The Interface is missing though..

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 16, 2015

Member

Does this cause a deprecation warning since that file will be included now?

This comment has been minimized.

Copy link
@linaori

linaori Jul 16, 2015

Author Contributor

I had a merge conflict here so I thought I merged it correct, but yes it will throw a notice now.

@weaverryan

This comment has been minimized.

Copy link
Member

commented Jul 16, 2015

Other than my last comment, 👍

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2015

@weaverryan I think it's fixed now, running gitbash with a non-syntax highlighting php $ git yolo.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jul 23, 2015

Re-read the whole PR and the associated issue discussion.

I'm 👍 on making the user checker configurable by firewall.

I'm -0 for the addition of the chain user checker (or any other way to allow more than one user checker). Being able to configure the service responsible for user checking should be enough for the vast majority of use cases. If you really want/need to provide more than one user checker, you can still do the chain yourself. Or do you envision third party bundle to come with user checkers that you will mix with your own.

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2015

@fabpot I think you're right that it will cover most cases, but configuring only 1 user-checker per firewall would be hard to extend for those who do want multiple user-checkers. They will have to start decorating the service, collect and inject their own user-checkers etc.

Providing a small extension point in the configuration with 1 additional class will save people with that specific case a lot of time, which includes myself.

@fabpot

This comment has been minimized.

Copy link
Member

commented Aug 1, 2015

I understand your point, but I'm still not convinced. It adds some complexity (and the security component does not need more complexity) for very edge cases. For the rare occasion where several user checkers are needed, the work is not that important, especially because you can now easily decorate a service in Symfony. Also, people will start asking for more, like being able to prioritize their user checkers and more. I'm still convinced that having more than one user checker is over-engineering we don't need.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Aug 1, 2015

I think it's okay to ship a chain user checker implementation with the Security component. However, I am not convinced if it is really necessary to configure it as the default user checker in the SecurityBundle.

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2015

It doesn't really matter if it's default or not, the end-user won't notice it. I just have the ChainChecker injected because it's less configuration to inject the default in there than make a conditional default service. I can change it but I don't see any benefit to add an extra conditional for that.

Regarding complexity, it won't really add a complex layer and has a nice documentation part in the book. It's merely another configuration option which people will find if they search for custom user checkers.

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

So, what's the status of this PR? For the record, I'm still against adding the chain user checker.
ping @symfony/deciders

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

For me this is as done as done can be. The ChainUserChecker won't be accessible/used-by the developer so it shouldn't bother them. This is merely to allow the developer to use multiple user-checkers without having to write code to allow this.

@fabpot if you know another way of achieving this without making it more complex for the developer, I'm more than happy to write it.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

@iltar What about setting the default user checker argument in the config resource files to null and add a comment explaining that this will be replaced by a user check chain? This would add some clarification to the configuration.

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

@xabbuh You mean this bit?

->arrayNode('user_checkers')
    ->defaultValue(array('security.user_checker')) #this default?
    ->info('A list of user checker service ids to use when authenticating users in this firewall.')
    ->prototype('scalar')->end()
->end()

If I set this to null by default, no checker is defined and I would have to set this as default if the array is empty and add it in the pass. I thought that was what the default value was for here, to save time in the extension/compiler pass.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

That can be done, but what would be the benefit of this?

@xabbuh

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

Imo it would increase readability for everyone trying to read and understand the code (you do not get the wrong impression that a single user checker is injected, for example). This can be handy when you try to debug and issue with the security system.

Anyway, I am 👍 for this change.

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

@xabbuh like this?
@weaverryan I've updated Guard as well now.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

@iltar Yes, I think that's better.

@weaverryan

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

I like this PR, and I've played around with the code.

But the chain user checker is a tough one: this feature won't be used by many people, and it's easily implementable on your own using normal OO practices. @iltar had once told me that a bundle could now provider different user checkers that you could opt into. If that's the case, then I think that bundle could also provider the chain user checker and configuration for you to put whatever you want into it.

@iltar I know you disagree with me, but I'm 👍 on this without the chain user checker.

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2015

@weaverryan This service is what's making it possible to provide an array of user-checkers, otherwise you would be limited to one checker. I can update the code to work without, but it would add a bit of extra work for developers who do want to implement it.

(A): Allow only 1 user-checker per firewall to be configured and remove the chain-user checker
(B): Leave as it

If I implement (A), I will change back the security.chain_user_checker to security.user_checker and use that service as a default, change the option to allow only a scalar value and I will update the docs PR to show the examples simplified with one option.

/cc @fabpot @weaverryan would this be an ok setup to complete this PR?

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 29, 2015

If I follow, (A) would just be about making the user_checker service configurable, right?

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2015

@fabpot correct

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 29, 2015

👍

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2015

I currently have added ->treatNullLike('security.user_checker'), but maybe in the future this could become nullable with a NullUserChecker.

Instead of creating a ChainUserChecker with a service definition, it will now create a private alias which is referenced in the factories instead of the security.chain_user_checker.

@linaori linaori changed the title [Security] Allow multiple user checkers per firewall [Security] Configuring a user checker per firewall Sep 29, 2015

@weaverryan

This comment has been minimized.

Copy link
Member

commented Sep 30, 2015

I agree with the treatNullLike thing.

👍

@weaverryan

This comment has been minimized.

Copy link
Member

commented Sep 30, 2015

And it looks like the test failures are unrelated and should be resolved with a rebase. So, a rebase might be good :)

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2015

@weaverryan yeah there were some issues with some tests hence I didn't rebase yet, good that you reminded me of it. Lets see how this turns out.

@@ -369,6 +370,8 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
// Exception listener
$exceptionListener = new Reference($this->createExceptionListener($container, $firewall, $id, $configuredEntryPoint ?: $defaultEntryPoint, $firewall['stateless']));
$container->setAlias(new Alias('security.user_checker.'.$id, false), $firewall['user_checker']);

This comment has been minimized.

Copy link
@fabpot

fabpot Oct 1, 2015

Member

Quick question: Why do we need to create aliases?

This comment has been minimized.

Copy link
@linaori

linaori Oct 1, 2015

Author Contributor

It was the only way I was able to get the user checker defined per firewall. I use an Alias because of the following configuration:

        security:
            firewalls:
                admin:
                    pattern: ^/admin
                    user_checker: app.admin_user_checker
                secured_area:
                    pattern: ^/
                    user_checker: app.user_checker

When the firewall part is being created, I couldn't get the user checker in there, I can only distinguish them based on $id which is the firewall name. By creating an alias per firewall, I can see which is which.

  • security.user_checker.admin -> app.admin_user_checker
  • security.user_checker.secured_area -> app.user_checker

If you have a better idea, I can see if I can make that work instead.

@@ -56,6 +56,8 @@ security:
logout: true
remember_me:
secret: TheSecret
user_checker:

This comment has been minimized.

Copy link
@fabpot

fabpot Oct 1, 2015

Member

missing ~

This comment has been minimized.

Copy link
@linaori

linaori Oct 1, 2015

Author Contributor

Ah nice catch! fixed.

@linaori

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2015

Any clues what the error in travis might be? hhvm locally for me doesn't run stable enough (more errors than travis) and I don't have php7 to test with.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Oct 1, 2015

The failing tests are not related to your changes (they are already failing on the 2.8 branch).

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 2, 2015

Thank you @iltar.

@fabpot fabpot closed this in 1e0adf4 Oct 2, 2015

@fabpot fabpot referenced this pull request Nov 16, 2015
xabbuh added a commit to symfony/symfony-docs that referenced this pull request Feb 7, 2016
feature #5530 [Cookbook, Security] Added user_checkers.rst (iltar)
This PR was squashed before being merged into the 2.8 branch (closes #5530).

Discussion
----------

[Cookbook, Security] Added user_checkers.rst

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes
| Applies to    | ^2.8
| Fixed tickets | symfony/symfony/pull/14721

This PR should provide sufficient docs to get started with custom user checkers as a new feature.

Commits
-------

89b20a8 [Cookbook, Security] Added user_checkers.rst

@linaori linaori deleted the linaori:feature/user-checkers branch Apr 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.