[Security] Fix event propagation for globally registered security events #39621
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
scheb
changed the title
wouterj
reviewed
Dec 23, 2020
...tyBundle/Tests/DependencyInjection/Compiler/RegisterGlobalSecurityEventListenersPassTest.php
Outdated
Show resolved
Hide resolved
nicolas-grekas
force-pushed
the
security-event-propagation
branch
from
5f144f2
to
1675864
Compare
nicolas-grekas
approved these changes
Dec 23, 2020
|
Thank you @scheb. |
|
PR welcome on 5.2! |
wouterj
added a commit
that referenced
this pull request
Dec 27, 2020
…opagated (scheb) This PR was merged into the 5.1 branch. Discussion ---------- [Security] Add test to ensure all security events are propagated | Q | A | ------------- | --- | Branch? | 5.1 | Bug fix? | no | New feature? | no | Deprecations? | no | License | MIT Follow-up to #39621. As requested by @wouterj I'm adding a dedicated test case to ensure the security events are propagated from global to firewall-level event dispatcher. I'll file another PR to add `AuthenticationTokenCreatedEvent` as soon as this has been merged and copied to the 5.2 branch, that I need to target for the `AuthenticationTokenCreatedEvent` change. Happy holidays! Commits ------- e78adf7 Add test case to ensure all security events are propagated
fabpot
added a commit
that referenced
this pull request
Dec 29, 2020
…eatedEvent when globally registered (scheb) This PR was merged into the 5.2 branch. Discussion ---------- [Security] Fix event propagation for AuthenticationTokenCreatedEvent when globally registered | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | License | MIT As promised, here's the follow-up to #39621, fixing `AuthenticationTokenCreatedEvent` to be propated from the global event dispatcher to firewall-specific event dispatchers. The event was added in Symfony 5.2, therefore the separate PR targeting the 5.2 branch. Commits ------- 68aaf4f Add AuthenticationTokenCreatedEvent to be propagated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When new authenticator security is enabled, the
AuthenticatorManageris using its own firewall-specific event dispatcher. To allow security events being listened to on the global level,RegisterGlobalSecurityEventListenersPassis there to automatically add globally registered event listeners to the firewall-specific event dispatchers.RegisterGlobalSecurityEventListenersPasscontains a list of events that are propagated, but unfortunately this list is incomplete as there are other events inAuthenticatorManagerthat would need too be propagated. So I added the missing (older) security events. These older events may also be registered by their name, rather than the FQN of the class, so I've also added those.As this is targeting 5.1, I'll file another PR for the
AuthenticationTokenCreatedEventthat was introduced in 5.2, as soon as this change was merged into 5.x.On a note, I feel this "whitelist" approach to propagate security events to the global dispatcher isn't that great, because it's prone to error. Additional security events may be added in the future and adding these to
RegisterGlobalSecurityEventListenersPasscan easily be missed. When I addedAuthenticationTokenCreatedEventin PR #37359 I wasn't aware of this propagation mechanic existed and also no one reviewing the PR noticed it.Additional changes:
array_uintersectinRegisterGlobalSecurityEventListenersPassTestwasn't implemented correctly ** That function's behavior is really odd and easy to be used in the wrong way. The callback function isn't intended to return true/false for matching items, but return -1/0/1 like sorting functions. The tests seemingly only worked by chance as returning true/false is doing pretty much the opposite of what the callback function is supposed to do.