[Security] Add ability for authenticators to explain why they didn’t support a request #60538
Conversation
a175f93 to
b0cdff7
Compare
| @@ -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 | |||
There was a problem hiding this comment.
| * 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?
There was a problem hiding this comment.
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.
12f9679 to
cc4097b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
541085e to
66af8a6
Compare
Why not; then what about
|
|
It'd say $requestDecision and $decisionReasons. |
It wouldn’t serve any purpose in the current state of this PR but yes. The problem I have with |
|
$requestDecisionReasons? |
f4ade34 to
0cecef5
Compare
|
Fine by me; just pushed the changes. |
0cecef5 to
9bdf856
Compare
| class RequestDecision | ||
| { | ||
| public bool $support; | ||
| public ?bool $lazy = null; |
There was a problem hiding this comment.
when is this used? what could be built with that?
| public ?bool $lazy = null; | |
| public bool $isLazy; |
There was a problem hiding this comment.
I planned to use this class for firewall listeners.
There was a problem hiding this comment.
Oh, so let's remove it from this PR and add it in the one about listeners then.
There was a problem hiding this comment.
Should the TraceableAuthenticator set them then?
| if ($this->supports === false) { | ||
| $requestDecision->support = false; | ||
| } else { | ||
| $requestDecision->support = true; | ||
| $requestDecision->lazy = null === $this->supports; | ||
| } |
There was a problem hiding this comment.
| 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; |
There was a problem hiding this comment.
I think isLazy should stay null if the request is not supported.
There was a problem hiding this comment.
the decision was not lazy, so why?
There was a problem hiding this comment.
Cause lazy only makes sense if the request is supported.
There was a problem hiding this comment.
BTW it is not the decision which is lazy but the request support; not sure about this naming then 🤔
9bdf856 to
f7520a5
Compare
f7520a5 to
50a70cb
Compare
Inspired by #59771 this PR allows authenticators to advertise why they didn’t support a request by passing a new
RequestSupportargument to theirsupportsmethod. Calling itsaddReasonmethod 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 theRequestSupportclass also holds$resultand$lazyproperties) andauthenticate, thus closing #36668.