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

MatTheCat · 2025-05-25T13:18:37Z

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.

OskarStark · 2025-05-25T13:43:51Z

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

 * 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


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?

Updated, thanks

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.

nicolas-grekas

Nice :)

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

MatTheCat · 2025-05-30T15:55:25Z

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

Why not; then what about

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

nicolas-grekas · 2025-05-30T16:03:07Z

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

MatTheCat · 2025-05-30T16:07:11Z

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).

nicolas-grekas · 2025-05-30T16:12:14Z

$requestDecisionReasons?

MatTheCat · 2025-05-30T16:24:08Z

Fine by me; just pushed the changes.

nicolas-grekas

One more iteration :)

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

nicolas-grekas · 2025-05-30T16:30:03Z

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

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


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

Suggested change

public ?bool $lazy = null;

public bool $isLazy;

I planned to use this class for firewall listeners.

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

Should the TraceableAuthenticator set them then?

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

nicolas-grekas · 2025-05-30T16:33:04Z

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

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


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;

I think isLazy should stay null if the request is not supported.

the decision was not lazy, so why?

Cause lazy only makes sense if the request is supported.

BTW it is not the decision which is lazy but the request support; not sure about this naming then 🤔

chalasr · 2025-06-04T06:47:27Z

I'm not sure it is worth complicating the authenticator API for this debug purpose. Complex setups involving many authenticators are pretty rare as opposed to voters, and logs work pretty well for this.

MatTheCat · 2025-06-17T13:29:30Z

I do think this would be useful whatever the number of configured authenticators. And isn’t one of the profiler’s interest avoiding to scan the logs? 😁

chalasr · 2025-06-17T13:43:51Z

Sure. It's a matter of balance and I feel like it's not worth it here 🤷 Maybe it's only me, let's see what others think. ping @wouterj

…support a request

MatTheCat requested a review from chalasr as a code owner May 25, 2025 13:18

carsonbot added Status: Needs Review Feature Security labels May 25, 2025

carsonbot added this to the 7.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 the authenticator-support-reason branch 2 times, most recently from a175f93 to b0cdff7 Compare May 25, 2025 13:20

OskarStark reviewed May 25, 2025

View reviewed changes

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 the authenticator-support-reason branch 6 times, most recently from 12f9679 to cc4097b Compare May 26, 2025 13:15

This comment has been minimized.

Sign in to view

fabpot modified the milestones: 7.3, 7.4 May 26, 2025

MatTheCat force-pushed the authenticator-support-reason branch 4 times, most recently from 541085e to 66af8a6 Compare May 30, 2025 10:01

nicolas-grekas reviewed May 30, 2025

View reviewed changes

MatTheCat force-pushed the authenticator-support-reason branch from f4ade34 to 0cecef5 Compare May 30, 2025 16:23

MatTheCat force-pushed the authenticator-support-reason branch from 0cecef5 to 9bdf856 Compare May 30, 2025 16:26

nicolas-grekas reviewed May 30, 2025

View reviewed changes

MatTheCat force-pushed the authenticator-support-reason branch 2 times, most recently from f7520a5 to 50a70cb Compare May 30, 2025 16:39

fabpot force-pushed the 7.4 branch from 65b7b8a to 1152edf Compare June 1, 2025 07:08

MatTheCat force-pushed the authenticator-support-reason branch from 50a70cb to 0d12051 Compare June 2, 2025 07:45

MatTheCat force-pushed the authenticator-support-reason branch from 0d12051 to 0f2834d Compare June 5, 2025 08:39

MatTheCat force-pushed the authenticator-support-reason branch from 0f2834d to 5dcb9ec Compare June 17, 2025 13:30

MatTheCat and others added 2 commits June 23, 2025 22:31

[Security] Add ability for authenticators to explain why they didn't …

e789bb1

…support a request

Renaming

0e1ee99

MatTheCat force-pushed the authenticator-support-reason branch from 5dcb9ec to 0e1ee99 Compare June 23, 2025 20:32

MatTheCat closed this Jul 26, 2025

MatTheCat deleted the authenticator-support-reason branch July 26, 2025 15:48

	* 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

Uh oh!

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

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

Uh oh!

Conversation

MatTheCat commented May 25, 2025 • edited by OskarStark Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OskarStark May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

MatTheCat commented May 30, 2025

Uh oh!

nicolas-grekas commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MatTheCat commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented May 30, 2025

Uh oh!

MatTheCat commented May 30, 2025

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commented Jun 4, 2025

Uh oh!

MatTheCat commented Jun 17, 2025

Uh oh!

chalasr commented Jun 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

MatTheCat commented May 25, 2025 •

edited by OskarStark

Loading

OskarStark May 25, 2025 •

edited

Loading

nicolas-grekas commented May 30, 2025 •

edited

Loading

MatTheCat commented May 30, 2025 •

edited

Loading