Skip to content

[Security] Add ability for authenticators to explain why they didn’t support a request #60538

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

Open
wants to merge 1 commit into
base: 7.4
Choose a base branch
from

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented May 25, 2025

Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Issues N/A
License MIT

Inspired by #59771 this PR allows authenticators to advertise why they didn’t support a request by passing a new RequestSupport argument to their supports method. Calling its addReason method will cause the profiler to display them:

If this is accepted, this will pave the way for firewall listeners to expose informations about their calls to supports (this is why the RequestSupport class also holds $result and $lazy properties) and authenticate, thus closing #36668.

@MatTheCat MatTheCat requested a review from chalasr as a code owner May 25, 2025 13:18
@carsonbot carsonbot added this to the 7.3 milestone May 25, 2025
@MatTheCat MatTheCat changed the title [Security] Add ability for authenticators to explain why they support the request or not [Security] Add ability for authenticators to explain why they supported the request or not May 25, 2025
@MatTheCat MatTheCat force-pushed the authenticator-support-reason branch 2 times, most recently from a175f93 to b0cdff7 Compare May 25, 2025 13:20
@@ -11,6 +11,7 @@ CHANGELOG
* Add support for closures in `#[IsGranted]`
* Add `OAuth2TokenHandler` with OAuth2 Token Introspection support for `AccessTokenAuthenticator`
* Add discovery support to `OidcTokenHandler` and `OidcUserInfoTokenHandler`
* Add ability for authenticators to explain why they support the request or not
Copy link
Contributor

@OskarStark OskarStark May 25, 2025

Choose a reason for hiding this comment

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

Suggested change
* Add ability for authenticators to explain why they support the request or not
* Add ability for authenticators to explain why they do not support the request

If they support it, no reason is set, right?

Copy link
Member

Choose a reason for hiding this comment

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

s/does/do

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they support it, no reason is set, right?

I was not sure if the userland could benefit from it, but since it is only displayed when the authenticator doesn’t support the request let’s not advertise it indeed.

@MatTheCat MatTheCat changed the title [Security] Add ability for authenticators to explain why they supported the request or not [Security] Add ability for authenticators to explain why they didn’t support a request May 26, 2025
@MatTheCat MatTheCat force-pushed the authenticator-support-reason branch 6 times, most recently from 12f9679 to cc4097b Compare May 26, 2025 13:15
@alexandre-daubois
Copy link
Member

You may want to update your PR description to target 7.4, as well as create a new UPGRADE file to target 7.4 as well

@MatTheCat
Copy link
Contributor Author

Yep I’ll do it while rebasing on the 7.4 branch when it’ll be created.

@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
@MatTheCat MatTheCat force-pushed the authenticator-support-reason branch from cc4097b to 18bdbc1 Compare May 27, 2025 08:28
@MatTheCat MatTheCat force-pushed the authenticator-support-reason branch from 18bdbc1 to fa94c26 Compare May 27, 2025 08:33
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.

6 participants