Updating behavior to not continue after an authenticator has set the response #15925
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…response This mirrors the behavior in core: *if* a listener sets a response (on success or failure), then the other listeners are not called. But if a response is *not* set (which is sometimes the case for success, like in BasicAuthenticationListener), then the other listeners are called, and can even fail.
| @@ -75,6 +75,12 @@ public function handle(GetResponseEvent $event) | |||
| $uniqueGuardKey = $this->providerKey.'_'.$key; | |||
|
|
|||
| $this->executeGuardAuthenticator($uniqueGuardKey, $guardAuthenticator, $event); | |||
|
|
|||
| if ($event->hasResponse()) { | |||
| $this->logger->info(sprintf('The "%s" authenticator set the response. Any later authenticator will not be called', get_class($guardAuthenticator))); | |||
There was a problem hiding this comment.
this looks like a debug message, not an info one IMO. You only care about it if you need to debug something going wrong.
There was a problem hiding this comment.
That's a good point - we use info() everywhere in the other security listeners already though.
There was a problem hiding this comment.
and part of them don't make sense at the info level IMO.
Only the "auth success" and "auth failed" messages remain at info. That's consistent with AbstractAuthenticationListener
| @@ -66,7 +66,7 @@ public function __construct(GuardAuthenticatorHandler $guardHandler, Authenticat | |||
| public function handle(GetResponseEvent $event) | |||
| { | |||
| if (null !== $this->logger) { | |||
| $this->logger->info('Checking for guard authentication credentials.', array('firewall_key' => $this->providerKey, 'authenticators' => count($this->guardAuthenticators))); | |||
| $this->logger->debug('Checking for guard authentication credentials.', array('firewall_key' => $this->providerKey, 'authenticators' => count($this->guardAuthenticators))); | |||
There was a problem hiding this comment.
This is unrelated to this PR, but it's so minor I snuck it in here. As Stof mentioned, most of these messages are really debug messages. I've only left the "auth success" and "auth failed" messages as info(), which is consistent with AbstractAuthenticationListener.
|
👍 Status: reviewed |
|
👍 |
|
Thank you @weaverryan. |
fabpot
added a commit
that referenced
this pull request
Sep 27, 2015
…as set the response (weaverryan) This PR was merged into the 2.8 branch. Discussion ---------- Updating behavior to not continue after an authenticator has set the response | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | https://github.com/symfony/symfony/pull/14673/files#r40492765 | License | MIT | Doc PR | n/a This mirrors the behavior in core: *if* a listener sets a response (on success or failure), then the other listeners are not called. But if a response is *not* set (which is sometimes the case for success, like in BasicAuthenticationListener), then the other listeners are called, and can even fail. It's all a bit of an edge-case, as only one authenticator (like authentication listener) would normally be doing any work on a request, but I think matching the other listeners (since I'm not aware of anyone having issues with its behavior) is best. Commits ------- 5fa2684 Making all "debug" messages use the debug router f403444 Updating behavior to not continue after an authenticator has set the response
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This mirrors the behavior in core: if a listener sets a response (on success or failure),
then the other listeners are not called. But if a response is not set
(which is sometimes the case for success, like in BasicAuthenticationListener),
then the other listeners are called, and can even fail.
It's all a bit of an edge-case, as only one authenticator (like authentication listener) would normally be doing any work on a request, but I think matching the other listeners (since I'm not aware of anyone having issues with its behavior) is best.