-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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] Added condition to always return the real Authenticator from security events #49015
[Security] Added condition to always return the real Authenticator from security events #49015
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
This is a bugfix to me. I would merge on 5.4 no matter if it's breaking some instanceof checks which should anyway be falling back to the real authenticator already if it's not traceable, in order for the code to work in non debug mode. |
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.
For 5.4
I think this is a feature, under the reasons we always apply to ensure the best stability for LTS versions. It's easy to workaround this in userland code and returning Given this, and the possibility that it might (although unlikely) break code in dev relying on |
I'd add that in the TraceableAuthenticator, the |
Being forced to call some internal API is indeed quite unfortunate and not a valid workaround. We are not adding any new feature nor making an improvement here. So the only remaining criteria is the breakage potential, which looks very low given it's impacting the debug mode only. |
In this PR, using the internal API is perfectly fines as usages are in the same package than the definition (and so they are internal usages). |
@stof what I meant is that merging this as a feature implies to force end users using 5.4 to call that internal method on their own. |
I'm forced to use the internal method on 6.2 so... 😒 I agree that this feels like a bugfix rather than a feature. But I get that this was not taken into account when creating the TraceableAuthenticator. |
While I understand the need behind this PR (I found myself fighting with the Traceable decorators quite a few times), I think this is also quite unprecedented. I have yet to see a Symfony event (or service?) that by design hides the Traceable decorators with no alternative other than reflection. Please, correct me if I'm wrong. Getting the TraceableAuthenticator is an actual feature. I value predictability over magic so, naturally, when calling getAuthenticator() I expect to get the authenticator being used, even if that means it's a decorator (Traceable, or otherwise). This issue is easily fixable in user-land, while the PR opens a philosophical can of worms whether Symfony should hide profiler decorators or not. While on the topic, I would also like to suggest removing There are indeed cases where in user-land you need to bypass the Traceable decorator and get to the real implementation, but having the events do it in a piecemeal fashion seems uninspired. |
I don't know in what use case you would use the traceableAuthenticator in production so I would assume it's for development. Given that, wouldn't an explicit I don't have a very strong opinion on that. My take is that I have the same code not behaving the same in dev and un prod and I find this unprecedented in the symfony framework. Edit: agree on the |
There is no reason to have the Profiler's Traceable decorators in production (though, a bit off-topic, you could have something else, like Sentry, decorating things to collect data). That's another reason against adding that ternary in an Event class -- it would run on all the production environments out there, whether it's needed or not. If Symfony must include a workaround for this issue, I'd rather try and find a different way. Some examples (with various pros and cons) I can think of are
That's all I could think of. More ideas are definitely welcome. |
Changing traceable authenticators to use inheritance rather than decoration is a no-go. Earlier versions of Symfony used to rely on inheritance to implement those traceable classes and this caused a bunch of issues (maintenance of those parts was a huge pain due to having to preserve inheritance support, an dit was entirely incompatible with other usages of decoration). |
There should be no need to call that method, as one is supposed to rely on the interface rather than the concrete implementation. That is part of the motivation for using decoration. Anyway this patch is fine, we just need to agree about whether it qualifies as a bugfix or a feature. |
@chalasr the use case for having the authenticator in the event is to be able to customize the logic done in the listener based on the authenticator that performed the authentication, not to call the authenticator again. And for that, the only tool we have is |
What about just dropping the |
If it's not obvious, it's a feature. Still good to merge? If yes, let's go! |
Thanks @florentdestremau! I've merged this as a feature in 6.3, given the "when in doubt, it's a feature" guideline used by Symfony. In any case, this is something we want, so we better merge it than stall it for another release :) About |
I'll make a PR for the removal of |
…ator::getAuthenticator()` (florentdestremau) This PR was merged into the 5.4 branch. Discussion ---------- [Security] Remove ``@internal`` tag on `TraceableAuthenticator::getAuthenticator()` | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | See #49015 | License | MIT Following the [discussion](#49015 (comment)) in #49015 I made this PR Commits ------- 0a8ba93 Removed `@internal` tag on TraceableAuthenticator::getAuthenticator()
…ator::getAuthenticator()` (florentdestremau) This PR was merged into the 5.4 branch. Discussion ---------- [Security] Remove ``@internal`` tag on `TraceableAuthenticator::getAuthenticator()` | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | See #49015 | License | MIT Following the [discussion](symfony/symfony#49015 (comment)) in #49015 I made this PR Commits ------- 0a8ba937b7 Removed `@internal` tag on TraceableAuthenticator::getAuthenticator()
This PR aims to uniformise the
getAuthenticator
method of several security Events when using the profiler in dev environement.