-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Add event to inspect authenticated token before it becomes effective #37359
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
Merged
Conversation
This file contains hidden or 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
cb254ae
to
37c6f0c
Compare
1192e19
to
c469334
Compare
c469334
to
bab908d
Compare
src/Symfony/Component/Security/Http/Event/InspectAuthenticatedTokenEvent.php
Outdated
Show resolved
Hide resolved
noniagriconomie
approved these changes
Aug 12, 2020
wouterj
approved these changes
Aug 12, 2020
fabpot
approved these changes
Aug 12, 2020
Thank you @scheb. |
363db1c
to
2030964
Compare
Merged
nicolas-grekas
added a commit
that referenced
this pull request
Dec 23, 2020
…ecurity events (scheb) This PR was squashed before being merged into the 5.1 branch. Discussion ---------- [Security] Fix event propagation for globally registered security events | Q | A | ------------- | --- | Branch? | 5.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | License | MIT When new authenticator security is enabled, the `AuthenticatorManager` is using its own firewall-specific event dispatcher. To allow security events being listened to on the global level, `RegisterGlobalSecurityEventListenersPass` is there to automatically add globally registered event listeners to the firewall-specific event dispatchers. `RegisterGlobalSecurityEventListenersPass` contains a list of events that are propagated, but unfortunately this list is incomplete as there are other events in `AuthenticatorManager` that 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 `AuthenticationTokenCreatedEvent` that 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 `RegisterGlobalSecurityEventListenersPass` can easily be missed. When I added `AuthenticationTokenCreatedEvent` in PR #37359 I wasn't aware of this propagation mechanic existed and also no one reviewing the PR noticed it. Additional changes: - Typo fix :) - The `array_uintersect` in `RegisterGlobalSecurityEventListenersPassTest` wasn'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. Commits ------- 1675864 [Security] Fix event propagation for globally registered security events
wouterj
added a commit
that referenced
this pull request
Apr 16, 2021
…vent (scheb) This PR was merged into the 5.3-dev branch. Discussion ---------- [Security] Add passport to AuthenticationTokenCreatedEvent | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | yes | Deprecations? | no | License | MIT This is a follow-up to my previous PR #37359, which added `AuthenticationTokenCreatedEvent` to the new authenticator-based security system to inspect the security token before it becomes effective to the security system. It **adds the passport** that was used to generate that token to the event, so that it can be inspected as well. Reasoning: 1) It makes the event more aligned with other security events (which are also providing the passport) 2) I see valid use-cases when you'd want to look into the passport/badges to decide if you'd want to make modifications to the security token. @Seldaek mentioned to me in scheb/2fa#74 that he'd like to have the ability to add a badge from his custom authenticator class, which then influences 2fa being triggered or not. Having the passport in the event would make that a straight forward task. I would like to add this to Symfony 5.3, since @wouterj plans to stabilize the authenticator security system for that release, so I believe this is worth adding it now rather than later. The constructor change could be considered a BC break, but since authenticator system is experimental, I believe it's fair to make that change now before declaring it "stable". Commits ------- 74196e0 Add passport to AuthenticationTokenCreatedEvent
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.
Hello there, I'm the author of
scheb/two-factor-bundle
, which extends Symfony's security layer with two-factor authentication. I've been closely following the recent changes by @wouterj to rework the security layer with "authenticators" (great work!). While I managed to make my bundle work with authenticators, I see some limitations in the security layer that I'd like to address to make such extensions easier to implement.This PR adds a new event, which is disapatched right after the authenticated token has been created by the authenticator, to "announce" it to the application before it becomes effective to the security system. The event works similar to
ResponseEvent
, but for security token. It allows listeners to inspect the new token before it becomes effective and - most importantly - apply modifications to it. So components other than the authenticator will be able to influence how the security token looks like, that will be set to the security layer on successful authentication.Why would you want to do this? Of course I'm looking at this from the 2fa perspective. To make 2fa work, it's necessary to prevent a newly created authenticated token from becoming visible to the security system and therefore exposing its privileges/roles. This is done by replacing the authenticated token with a temporary "TwoFactorToken". Currently I'm doing this through dependency injection, getting all the registered authenticators and decorating them with my own token-exchange logic. This is not very clean and overly complicated, but it works. Adding this event as a hook-in point would allow for a much cleaner integration for any component that wants to have a saying in how the security token should look like.