NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

MatTheCat wants to merge2 commits intosymfony:7.4

base:7.4

Choose a base branch

fromMatTheCat:authenticator-support-reason

Open

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

MatTheCat wants to merge2 commits intosymfony:7.4fromMatTheCat:authenticator-support-reason

+233 −30

Conversation

Copy link

Contributor

MatTheCat commentedMay 25, 2025•
edited by OskarStark
Loading

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 newRequestSupport argument to theirsupports method. Calling itsaddReason 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 tosupports (this is why theRequestSupport class also holds$result and$lazy properties) andauthenticate, thus closing#36668.

MatTheCat requested a review fromchalasr as acode owner

May 25, 2025 13:18

carsonbot added Status: Needs Review Feature Security labels

May 25, 2025

carsonbot added this to the7.3 milestone

May 25, 2025

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 force-pushed theauthenticator-support-reason branch 2 times, most recently froma175f93 tob0cdff7Compare

May 25, 2025 13:20

OskarStark reviewed

May 25, 2025

View reviewed changes

src/Symfony/Component/Security/Http/CHANGELOG.md Outdated

		@@ -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

OskarStarkMay 25, 2025•
edited
Loading

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 theydo notsupport the request

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

Copy link

Member

chalasrMay 25, 2025

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

OskarStarkMay 25, 2025

Choose a reason for hiding this comment

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

Updated, thanks

Copy link

ContributorAuthor

MatTheCatMay 26, 2025

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 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 force-pushed theauthenticator-support-reason branch 6 times, most recently from12f9679 tocc4097bCompare

May 26, 2025 13:15

This comment has been minimized.

fabpot modified the milestones:7.3,7.4

May 26, 2025

MatTheCat force-pushed theauthenticator-support-reason branch 4 times, most recently from541085e to66af8a6Compare

May 30, 2025 10:01

nicolas-grekas reviewed

May 30, 2025

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Nice :)

I'm just wondering about the name. What aboutRequestDecision instead?

Copy link

ContributorAuthor

MatTheCat commentedMay 30, 2025

I'm just wondering about the name. What aboutRequestDecision instead?

Why not; then what about

the parameter’s name?$requestDecision? Or just$decision?
TraceableAuthenticator’s property? Still$supportReasons?

Copy link

Member

nicolas-grekas commentedMay 30, 2025•
edited
Loading

It'd say $requestDecision and $decisionReasons.
(One could also use addReason to explain why they decided to ACCEPT the request, isn't it?)

Copy link

ContributorAuthor

MatTheCat commentedMay 30, 2025•
edited
Loading

One could also use addReason to explain why they decided to ACCEPT the request, isn't it?

It wouldn’t serve any purpose in the current state of this PR but yes.

The problem I have with$decisionReasons is that it doesn’t mention the decision is about request support (could also be about authentication).

Copy link

Member

nicolas-grekas commentedMay 30, 2025

$requestDecisionReasons?

MatTheCat force-pushed theauthenticator-support-reason branch fromf4ade34 to0cecef5Compare

May 30, 2025 16:23

Copy link

ContributorAuthor

MatTheCat commentedMay 30, 2025

Fine by me; just pushed the changes.

MatTheCat force-pushed theauthenticator-support-reason branch from0cecef5 to9bdf856Compare

May 30, 2025 16:26

nicolas-grekas reviewed

May 30, 2025

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

One more iteration :)

src/Symfony/Component/Security/Http/CHANGELOG.md OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Http/RequestDecision.php Outdated

		class RequestDecision
		{
		publicbool$support;
		public ?bool$lazy =null;

Copy link

Member

nicolas-grekasMay 30, 2025

Choose a reason for hiding this comment

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

when is this used? what could be built with that?

Suggested change

	public?bool$lazy =null;
	public bool$isLazy;

Copy link

ContributorAuthor

MatTheCatMay 30, 2025

Choose a reason for hiding this comment

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

I planned to use this class for firewall listeners.

Copy link

Member

nicolas-grekasMay 30, 2025

Choose a reason for hiding this comment

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

Oh, so let's remove it from this PR and add it in the one about listeners then.

Copy link

ContributorAuthor

MatTheCatMay 30, 2025

Choose a reason for hiding this comment

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

Should theTraceableAuthenticator set them then?

src/Symfony/Component/Security/Http/RequestDecision.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Http/Authenticator/Debug/TraceableAuthenticator.php Outdated

Comment on lines 78 to 83

		if ($this->supports === false) {
		$requestDecision->support = false;
		} else {
		$requestDecision->support = true;
		$requestDecision->lazy = null === $this->supports;
		}

Copy link

Member

nicolas-grekasMay 30, 2025

Choose a reason for hiding this comment

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

Suggested change

	if ($this->supports ===false) {
	$requestDecision->support =false;
	}else {
	$requestDecision->support =true;
	$requestDecision->lazy =null ===$this->supports;
	}
	$requestDecision->isSupported =false !==$this->supports;
	$requestDecision->isLazy =null ===$this->supports;