Skip to content

[Security\Core] Fix NoopAuthenticationManager::authenticate() return value #36805

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
May 16, 2020

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented May 13, 2020

Q A
Branch? master
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #36804
License MIT
Doc PR -

@wouterj
Copy link
Member

wouterj commented May 13, 2020

Imho, we should do both suggestions of #36804: The always_authenticate_before_granting setting is completely ignored in the new system. It's better to make this explicit by not allowing it to be true when the new system is used.

Rationale: This option is heavily misused to reload user roles on the fly, which is not the intention of this option. This use-case has to be addressed by a new feature in Symfony Security, fixing all related issues with changing roles on the fly (such as being logged out since 4.4). As this was considered out of scope for the authenticator system PR, it was decided that if one needs to reload on the fly, one needs to stick with the old system in 5.1.

@chalasr chalasr force-pushed the noop-auth-retval branch from bfaf4bc to c7a04d7 Compare May 13, 2020 13:35
@chalasr
Copy link
Member Author

chalasr commented May 13, 2020

@wouterj Makes sense, exception added. Thanks.
Suggestions welcome to improve the error message

@chalasr chalasr force-pushed the noop-auth-retval branch 2 times, most recently from 7664179 to 39af372 Compare May 13, 2020 16:27
@nicolas-grekas
Copy link
Member

Thank you @chalasr.

@nicolas-grekas nicolas-grekas merged commit a86058c into symfony:master May 16, 2020
@fabpot fabpot mentioned this pull request May 16, 2020
@chalasr chalasr deleted the noop-auth-retval branch May 17, 2020 18:49
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.

Authenticators and always_authenticate_before_granting causes null TypeError
5 participants