[SecurityBundle] fix(security): user login programmatically with dedicated user checker on a firewall #54287

94noni · 2024-03-14T14:33:08Z

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix internal security user checker code call
License	MIT

Original PR at #41274 as of Symfony v6.2 => so lets target v6.4 here

When defining an user checker on a firewall
This user checker logic for checkPreAuth is not called at all when using the Symfony/Bundle/SecurityBundle/Security::login feature
=> this can lead to unwanted security issue

In the reproducer, check the https://github.com/94noni/userchecker/blob/master/src/Security/UserChecker.php (logged in user must be enabled)
The controller https://github.com/94noni/userchecker/blob/master/src/Controller/AnonymousController.php get the user by its id and log
=> the user checker is not called and the user is logged in even if not enabled

OskarStark · 2024-03-15T07:40:35Z

Sounds like a bugfix to me

94noni · 2024-03-15T13:40:08Z

Sounds like a bugfix to me

yes

94noni · 2024-03-15T18:44:39Z

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

@@ -98,6 +98,7 @@ public function load(array $configs, ContainerBuilder $container)
    {
        if (!array_filter($configs)) {
            trigger_deprecation('symfony/security-bundle', '6.3', 'Enabling bundle "%s" and not configuring it is deprecated.', SecurityBundle::class);
+


fabbot cs fixer issue

94noni · 2024-03-15T18:45:06Z

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

@@ -424,6 +425,14 @@ private function createFirewall(ContainerBuilder $container, string $id, array $
        $container->register('security.user_checker.chain.'.$id, ChainUserChecker::class)
            ->addArgument(new TaggedIteratorArgument('security.user_checker.'.$id));

+        // Register Firewall-specific user checker for 'security.helper' login()
+        $securityUserCheckerLocator = $container->getDefinition('security.user_checker_locator');


i "copied" such things for another locator
please tell me if its the right thing to do

94noni · 2024-03-15T18:46:14Z

src/Symfony/Bundle/SecurityBundle/Security.php

@@ -127,7 +127,11 @@ public function login(UserInterface $user, ?string $authenticatorName = null, ?s

        $authenticator = $this->getAuthenticator($authenticatorName, $firewallName);

-        $this->container->get('security.user_checker')->checkPreAuth($user);
+        if ($this->container->get('security.user_checker_locator')->has('security.user_checker.'.$firewallName)) {
+            $this->container->get('security.user_checker_locator')->get('security.user_checker.'.$firewallName)->checkPreAuth($user);


this is the PR fix: calling the proper firewall user checker if any

smnandre · 2024-03-16T04:04:37Z

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

+        $securityUserCheckerLocator = $container->getDefinition('security.user_checker_locator');
+        $securityUserCheckerLocator
+            ->replaceArgument(0, array_merge($securityUserCheckerLocator->getArgument(0), [
+                'security.user_checker.'.$id => new ServiceClosureArgument(new Reference('security.user_checker.'.$id)),


You should reuse the chain defined just before

security.user_checker.chain.'.$id

Thx for your comment
I fear not getting it, you mean i should add it in the locator or i should not use locator at all but inject this chained instead and use it in the security helper?

I'm not sure a locator is needed. There are already "ChainChecker" registered per firewall, that have all the checkers you reference there.

The problem is -imho- that no Event is triggered during this execution (there is one for the standard behaviour (see UserCheckerListener) login or SwitchUser) So the checkPreAuth is called only with the "main" UserChecker and not the one registered, as you already pointed.

I'm thinking instead of adding services there, it would maybe be prefereable to add a new Event ("manualLogin" -- with a better name haha) and plug the checkers in the same way they are called in the authentication events.

@smnandre thx for detailing this, I got it more
I know @chalasr and @wouterj are a lot of security subject so I would like to have more hints

my PR just want to fix the bug/security issue

Thanks for the ping.

I'm thinking instead of adding services there, it would maybe be prefereable to add a new Event

Indeed, relying on UserCheckerListener sounds better. Maybe make it listen to a CheckUserEvent in addition to CheckPassportEvent? As this is a bugfix, that new event class should be internal at first. On 7.x we'll be able to make it public, dispatch it in the regular authentication flow and deprecate listening on CheckPassportEvent in UserCheckerListener.

I will try to find some time this week to address both of your reviews
Thx

94noni · 2024-04-02T12:11:35Z

@smnandre @chalasr I've push something related to our discussion

please come back to me if it is what you had in mind, so I can continue if its the legit path
thx a lot

…cated user checker on a firewall

94noni · 2024-04-29T09:00:10Z

friendly ping 👋🏻
just before investing more time on CIs resolution
thx!

94noni · 2024-05-21T08:19:46Z

up please ? I really think this is important
thank you

smnandre · 2024-05-26T22:42:36Z

up please ? I really think this is important thank you

Sorry, had a lot going on those last weeks .. seems good to me, but my opinion will have no importance here.

I think you should maybe add some tests for this new feature, and check the previous ones still pass :)

oleg-andreyev · 2024-07-12T10:57:02Z

src/Symfony/Bundle/SecurityBundle/Security.php

@@ -127,7 +128,7 @@ public function login(UserInterface $user, ?string $authenticatorName = null, ?s

        $authenticator = $this->getAuthenticator($authenticatorName, $firewallName);

-        $this->container->get('security.user_checker')->checkPreAuth($user);


not sure that this will solve #57706, security.user_checker is still bound to InMemoryUserChecker as default.

We probably need create service that will be aware about all user_checker's per firewall and based on firewall name, all required service.

thanks for commenting
to be franc I've tested several things, see all PR force push
so for now I am quite waiting core devs hints

i do also have created a workaround on my project until core is fixed so for now it really dont bother much in a security POV on my side

xabbuh · 2024-07-17T10:32:50Z

@94noni I suggest trying another approach that doesn't require a new event. Can you try if #57748 fixes the issue you experience?

94noni · 2024-07-17T17:19:05Z

Is it like my first approach on this PR (prior to reviews/force push) with a locator @xabbuh ?
In all ways, i will try this ok, but not at the moment as not having any computer
Thanks

xabbuh · 2024-07-18T09:42:05Z

Is it like my first approach on this PR (prior to reviews/force push) with a locator @xabbuh ?

It really looks like that. Can you push your initial approach as I think you should then get the credit for the bugfix.

94noni · 2024-07-18T10:12:32Z

Its not about the credit but more confirming my first understanding of this core code :)
As much as the root issue is fixed i will be happy
But really i would not be able to test on a machine before end of month so perhaps better merge your PR so it can be released asap :)

xabbuh · 2024-07-19T08:15:27Z

closing in favor of #57748 then 👍

chalasr · 2024-07-19T21:57:03Z

Is it like my first approach on this PR (prior to reviews/force push) with a locator @xabbuh ?

@94noni Indeed. Looking again at the code, your original approach was correct.

The ChainUserchecker is what confused us, but actually there is no need to care about it given it's a userland class, not a core service. When it's used, it ends up being set as the firewall's user_checker so it would ultimately be part of the service locator you first proposed.

My apologies for the confusion and thanks a lot for your work here.

94noni requested a review from chalasr as a code owner March 14, 2024 14:33

carsonbot added Status: Needs Review Bug labels Mar 14, 2024

carsonbot added this to the 7.1 milestone Mar 14, 2024

94noni changed the title ~~fix(security): user login programmatically with dedicated user checker on a firewall~~ [SecurityBundle] fix(security): user login programmatically with dedicated user checker on a firewall Mar 15, 2024

94noni force-pushed the security-login-prog-user-checker branch from a93196b to be547b5 Compare March 15, 2024 13:39

94noni force-pushed the security-login-prog-user-checker branch 2 times, most recently from 961db7e to 2527d07 Compare March 15, 2024 13:53

94noni changed the base branch from 7.1 to 6.4 March 15, 2024 13:53

94noni force-pushed the security-login-prog-user-checker branch 2 times, most recently from d5db9d8 to e4ea9f8 Compare March 15, 2024 14:11

94noni commented Mar 15, 2024

View reviewed changes

GromNaN added the SecurityBundle label Mar 15, 2024

smnandre reviewed Mar 16, 2024

View reviewed changes

symfony deleted a comment from carsonbot Mar 16, 2024

94noni force-pushed the security-login-prog-user-checker branch 2 times, most recently from 9d29a40 to 29a65f9 Compare April 2, 2024 12:10

[SecurityBundle] fix(security): user login programmatically with dedi…

6d1cf68

…cated user checker on a firewall

94noni force-pushed the security-login-prog-user-checker branch from 29a65f9 to 6d1cf68 Compare April 2, 2024 12:24

xabbuh modified the milestones: 7.1, 6.4 May 15, 2024

MatTheCat mentioned this pull request Jul 12, 2024

[Security] Security::login does not take into account custom user_checker #57706

Closed

oleg-andreyev reviewed Jul 12, 2024

View reviewed changes

chalasr mentioned this pull request Jul 18, 2024

[SecurityBundle] use firewall-specific user checkers when manually logging in users #57748

Merged

xabbuh closed this Jul 19, 2024

94noni deleted the security-login-prog-user-checker branch August 7, 2024 10:10

		@@ -127,7 +128,7 @@ public function login(UserInterface $user, ?string $authenticatorName = null, ?s

		$authenticator = $this->getAuthenticator($authenticatorName, $firewallName);

		$this->container->get('security.user_checker')->checkPreAuth($user);

Uh oh!

[SecurityBundle] fix(security): user login programmatically with dedicated user checker on a firewall #54287

[SecurityBundle] fix(security): user login programmatically with dedicated user checker on a firewall #54287

Uh oh!

Conversation

94noni commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OskarStark commented Mar 15, 2024

Uh oh!

94noni commented Mar 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

94noni Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Mar 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

94noni commented Apr 2, 2024

Uh oh!

94noni commented Apr 29, 2024

Uh oh!

94noni commented May 21, 2024

Uh oh!

smnandre commented May 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Jul 17, 2024

Uh oh!

94noni commented Jul 17, 2024

Uh oh!

xabbuh commented Jul 18, 2024

Uh oh!

94noni commented Jul 18, 2024

Uh oh!

xabbuh commented Jul 19, 2024

Uh oh!

chalasr commented Jul 19, 2024

Uh oh!

Uh oh!

94noni commented Mar 14, 2024 •

edited

Loading

94noni Mar 16, 2024 •

edited

Loading

chalasr Mar 31, 2024 •

edited

Loading