[Security] Configuring a user checker per firewall #14721

linaori · 2015-05-22T08:52:55Z

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.

weaverryan · 2015-05-22T14:38:38Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/FormLoginFactory.php

@@ -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))


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.

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).

@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 · 2015-05-23T14:30:07Z

@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.

weaverryan · 2015-05-23T14:39:14Z

src/Symfony/Component/Security/Core/User/ChainUserChecker.php

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


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).

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/

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.

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.

linaori · 2015-07-16T14:37:23Z

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

fabpot · 2015-07-16T14:40:50Z

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

linaori · 2015-07-16T14:53:36Z

@fabpot done

weaverryan · 2015-07-16T16:55:40Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

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


Should this be here?

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

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

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

weaverryan · 2015-07-16T16:56:55Z

Other than my last comment, 👍

linaori · 2015-07-16T17:56:00Z

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

fabpot · 2015-07-23T03:33:22Z

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 · 2015-07-30T06:44:49Z

@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 · 2015-08-01T06:34:49Z

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 · 2015-08-01T09:01:44Z

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 · 2015-08-01T15:50:32Z

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 · 2015-09-28T09:31:42Z

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

linaori · 2015-09-28T10:15:27Z

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 · 2015-09-28T11:29:49Z

@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 · 2015-09-28T11:53:47Z

@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 · 2015-09-28T12:00:05Z

@iltar I mean places like https://github.com/symfony/symfony/blob/2.8/src/Symfony/Bundle/SecurityBundle/Resources/config/security_rememberme.xml#L31 which will be modified by the factories anyway.

linaori · 2015-09-28T12:01:18Z

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

xabbuh · 2015-09-28T12:11:46Z

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 · 2015-09-28T12:51:17Z

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

xabbuh · 2015-09-28T14:06:45Z

@iltar Yes, I think that's better.

weaverryan · 2015-09-28T23:30:50Z

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 · 2015-09-29T05:20:50Z

@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 · 2015-09-29T06:02:45Z

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

linaori · 2015-09-29T06:07:42Z

@fabpot correct

fabpot · 2015-09-29T06:09:05Z

👍

linaori · 2015-09-29T06:51:43Z

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.

weaverryan · 2015-09-30T23:27:56Z

I agree with the treatNullLike thing.

👍

weaverryan · 2015-09-30T23:30:36Z

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

Also added the chain_user_checker to freshly added authenticators: - LDAP - Guard

linaori · 2015-10-01T05:14:31Z

@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.

fabpot · 2015-10-01T05:18:17Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

@@ -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']);


Quick question: Why do we need to create aliases?

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.

linaori · 2015-10-01T06:37:10Z

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 · 2015-10-01T08:31:18Z

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

fabpot · 2015-10-02T12:49:45Z

Thank you @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 mentioned this pull request May 22, 2015

[Security] Allow multiple user checkers per firewall #14713

Closed

jakzal added Security SecurityBundle labels May 22, 2015

weaverryan reviewed May 22, 2015
View reviewed changes

weaverryan reviewed May 23, 2015
View reviewed changes

linaori mentioned this pull request Jul 16, 2015

[Cookbook, Security] Added user_checkers.rst symfony/symfony-docs#5530

Closed

weaverryan reviewed Jul 16, 2015
View reviewed changes

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

Iltar van der Berg and others added 9 commits October 1, 2015 07:13

Implemented the ChainUserChecker

b4b2e57

Added the possibility to register multiple user checkers per firewall

f031185

Security factories are now injecting the chain user checker per firewall

ef016fa

Added a test case to cover when an empty set is passed

315421e

Updated docs to briefly explain the exception thrown

d2468f4

Put some code back that was merged out by accident

9c35cbb

Thanks fabbot.io

75f766d

Removed bad merge

331a52a

Removed security.user_checker from services

c419603

Also added the chain_user_checker to freshly added authenticators: - LDAP - Guard

fabpot reviewed Oct 1, 2015
View reviewed changes

Removed ChainUser checker & simplified config

6189cf2

fabpot closed this in 1e0adf4 Oct 2, 2015

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

linaori deleted the feature/user-checkers branch April 15, 2016 13:33

[Security] Configuring a user checker per firewall #14721

[Security] Configuring a user checker per firewall #14721

Conversation

linaori commented May 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented May 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Jul 16, 2015

fabpot commented Jul 16, 2015

linaori commented Jul 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Jul 16, 2015

linaori commented Jul 16, 2015

fabpot commented Jul 23, 2015

linaori commented Jul 30, 2015

fabpot commented Aug 1, 2015

xabbuh commented Aug 1, 2015

linaori commented Aug 1, 2015

fabpot commented Sep 28, 2015

linaori commented Sep 28, 2015

xabbuh commented Sep 28, 2015

linaori commented Sep 28, 2015

xabbuh commented Sep 28, 2015

linaori commented Sep 28, 2015

xabbuh commented Sep 28, 2015

linaori commented Sep 28, 2015

xabbuh commented Sep 28, 2015

weaverryan commented Sep 28, 2015

linaori commented Sep 29, 2015

fabpot commented Sep 29, 2015

linaori commented Sep 29, 2015

fabpot commented Sep 29, 2015

linaori commented Sep 29, 2015

weaverryan commented Sep 30, 2015

weaverryan commented Sep 30, 2015

linaori commented Oct 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Oct 1, 2015

xabbuh commented Oct 1, 2015

fabpot commented Oct 2, 2015