Skip to content

[Security] Add passport to AuthenticationTokenCreatedEvent #40840

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
merged 1 commit into from
Apr 16, 2021

Conversation

scheb
Copy link
Contributor

@scheb scheb commented Apr 16, 2021

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 Document how to bypass 2FA entirely in a given context 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".

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks.

@chalasr chalasr added this to the 5.x milestone Apr 16, 2021
@chalasr chalasr added the Ready label Apr 16, 2021
@wouterj wouterj force-pushed the authentication-token-created-event branch from 7f0fdb0 to 74196e0 Compare April 16, 2021 19:36
@wouterj
Copy link
Member

wouterj commented Apr 16, 2021

I agree, thank you Christian! (also thanks for the precise PR descriptions as always, that makes it a breeze to review the need for features you introduce!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants