Skip to content

[SecurityBundle] Do not replace authenticators service by their traceable version #59278

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
Dec 30, 2024

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Dec 21, 2024

Q A
Branch? 7.2
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #59071, fix #59091
License MIT

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.

Unfortunate but 👍 Thanks. Hope you checked everything keeps working as expected in the profiler 😇

@MatTheCat
Copy link
Contributor Author

MatTheCat commented Dec 23, 2024

Hm there shouldn’t be anything unfortunate here; did I miss something? 😅

The AuthenticatorManager will still receive TraceableAuthenticators, the only difference is that the original authenticators will still exist in the container.

@chalasr
Copy link
Member

chalasr commented Dec 24, 2024

Just the fact we cannot stick with regular service decoration.

@MatTheCat MatTheCat force-pushed the fix_traceable_authenticators branch from e4c66eb to ffbd82c Compare December 27, 2024 16:39
@MatTheCat MatTheCat force-pushed the fix_traceable_authenticators branch from c9c19ab to 111913e Compare December 29, 2024 15:37
@fabpot fabpot force-pushed the fix_traceable_authenticators branch from 111913e to d44b7af Compare December 30, 2024 18:55
@fabpot
Copy link
Member

fabpot commented Dec 30, 2024

Thank you @MatTheCat.

@fabpot fabpot merged commit 5280da9 into symfony:7.2 Dec 30, 2024
10 of 11 checks passed
@MatTheCat MatTheCat deleted the fix_traceable_authenticators branch December 30, 2024 18:57
@fabpot fabpot mentioned this pull request Dec 31, 2024
fabpot added a commit that referenced this pull request Jan 2, 2025
…security.helper` (MatTheCat)

This PR was merged into the 7.2 branch.

Discussion
----------

[SecurityBundle] Do not pass traceable authenticators to `security.helper`

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #59341
| License       | MIT

Since #59278 authenticators are no longer aliases for their traceable version, which means calling `Security::login` with an authenticator ID won’t match its traceable ID, and fail.

Plus, `Security::login` using the traceable authenticators meant the profiler could show them as successful while not supporting the request:

![](https://github.com/user-attachments/assets/dc36ff28-93ba-4adf-ba16-9ed7742f3fd4)

This PR fixes these issues by passing the original authenticators to `security.helper`, using their ID as name.

Commits
-------

c5a2360 [SecurityBundle] Do not pass traceable authenticators to `security.helper`
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