Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[SecurityBundle] fix(security): user login programmatically with dedicated user checker on a firewall#54287
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
OskarStark commentedMar 15, 2024
Sounds like a bugfix to me |
a93196b tobe547b5Compare94noni commentedMar 15, 2024
yes |
961db7e to2527d07Compared5db9d8 toe4ea9f8Compare| { | ||
| if (!array_filter($configs)) { | ||
| trigger_deprecation('symfony/security-bundle','6.3','Enabling bundle "%s" and not configuring it is deprecated.', SecurityBundle::class); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fabbot cs fixer issue
| ->addArgument(newTaggedIteratorArgument('security.user_checker.'.$id)); | ||
| // Register Firewall-specific user checker for 'security.helper' login() | ||
| $securityUserCheckerLocator =$container->getDefinition('security.user_checker_locator'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i "copied" such things for another locator
please tell me if its the right thing to do
| $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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is the PR fix: calling the proper firewall user checker if any
| $securityUserCheckerLocator =$container->getDefinition('security.user_checker_locator'); | ||
| $securityUserCheckerLocator | ||
| ->replaceArgument(0,array_merge($securityUserCheckerLocator->getArgument(0), [ | ||
| 'security.user_checker.'.$id =>newServiceClosureArgument(newReference('security.user_checker.'.$id)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should reuse the chain defined just before
security.user_checker.chain.'.$idThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 (seeUserCheckerListener) 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the ping.
I'm thinking instead of adding services there, it would maybe be prefereable to add a new Event
Indeed, relying onUserCheckerListener sounds better. Maybe make it listen to aCheckUserEvent in addition toCheckPassportEvent? 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 onCheckPassportEvent inUserCheckerListener.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I will try to find some time this week to address both of your reviews
Thx
9d29a40 to29a65f9Compare94noni commentedApr 2, 2024
@smnandre@chalasr I've push something related toour discussion please come back to me if it is what you had in mind, so I can continue if its the legit path |
…cated user checker on a firewall
29a65f9 to6d1cf68Compare94noni commentedApr 29, 2024
friendly ping 👋🏻 |
94noni commentedMay 21, 2024
up please ? I really think this is important |
smnandre commentedMay 26, 2024
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 :) |
| $authenticator =$this->getAuthenticator($authenticatorName,$firewallName); | ||
| $this->container->get('security.user_checker')->checkPreAuth($user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not sure that this will solve#57706,security.user_checker is still bound toInMemoryUserChecker 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedJul 17, 2024
94noni commentedJul 17, 2024
Is it like my first approach on this PR (prior to reviews/force push) with a locator@xabbuh ? |
xabbuh commentedJul 18, 2024
It really looks like that. Can you push your initial approach as I think you should then get the credit for the bugfix. |
94noni commentedJul 18, 2024
Its not about the credit but more confirming my first understanding of this core code :) |
xabbuh commentedJul 19, 2024
closing in favor of#57748 then 👍 |
chalasr commentedJul 19, 2024
@94noni Indeed. Looking again at the code, your original approach was correct. The My apologies for the confusion and thanks a lot for your work here. |
Uh oh!
There was an error while loading.Please reload this page.
Original PR at#41274 as of Symfony v6.2 => so lets target v6.4 here
When defining anuser checker on a firewall
This user checker logic for
checkPreAuthis not called at all when using theSymfony/Bundle/SecurityBundle/Security::loginfeature=> this can lead to unwanted security issue
In the reproducer, check thehttps://github.com/94noni/userchecker/blob/master/src/Security/UserChecker.php (logged in user must be enabled)
The controllerhttps://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