Throwing AccessDeniedException when getCredentials() in GuardAuthenticator are null #23253

nospor · 2017-06-21T14:56:47Z

Q	A
Bug report?	yes/no
Feature request?	no
BC Break report?	no
RFC?	no
Symfony version	3.3

Hi, I upgraded to Symfony3.3 from 3.2. I am using GuardAuthenticator to authenticate my users. I also have ExceptionListener to provide my own responses to different exceptions depending on is it AJAX or not.

In class below there are two methods

class SsoAuthenticator extends AbstractGuardAuthenticator {
.....

    public function getCredentials(Request $request)
    {
        //it returns credentials or NULL
    }
.....
    public function start(Request $request, AuthenticationException $authException = null)
    {
        $url = $this->container->getParameter('sso.server_url');
        $url .= '/login?redirect=' . urlencode(
                $request->getScheme() . '://' .
                $request->getHttpHost() .
                $request->getRequestUri()
            );

        return new RedirectResponse($url);
    }
}

According to documentation methos start() is called when method getCredentials() returns null;
It was working fine with symfony3.2. But when I upgraded to symfony3.3 it stopped working. Method start() was not called anymore. After long investigating it appeard, that in symfony3.3 you throw AccessDeniedException which you didn't in Symfony3.2. And now, when I have my ExceptionListener yours AccessDeniedException goes first to my ExceptionListener and then I return some Response and method start() is not called anylonger. I had to add some conditions in my ExceptionListener that when is raised AccessDeniedException i must do return; and then method start() is finally called.

Well, IMHO it is not a good solution. I think you shouldn't throw exception when getCredentials() returns NULL. I think you should do what you said you will do which is call start() method directly. Now I, and maybe others need to play with this strange situation.

Robert

The text was updated successfully, but these errors were encountered:

chalasr · 2017-06-21T17:23:37Z

After long investigating it appeard, that in symfony3.3 you throw AccessDeniedException which you didn't in Symfony3.2.

Would you be able to point out this exception or the PR/commit in which it has been introduced? The guard behavior seems unchanged.

nospor · 2017-06-22T10:20:55Z

Well, maybe it was throw in Symfony3.2 as well but it was catched by something in the middle? All I know is now (Symfony 3.3) this exception goes directly to my ExceptionListener which didn't happen in Symfony3.2.
Below is the exception trace which I can see when it goes to my ExceptionListener. Maybe it will tell you something more.

#0 /var/www/html/brad/vendor/symfony/symfony/src/Symfony/Component/Security/Http/Firewall.php(69): Symfony\Component\Security\Http\Firewall\AccessListener->handle(Object(Symfony\Component\HttpKernel\Event\GetResponseEvent))
#1 /var/www/html/brad/vendor/symfony/symfony/src/Symfony/Bundle/SecurityBundle/EventListener/FirewallListener.php(48): Symfony\Component\Security\Http\Firewall->onKernelRequest(Object(Symfony\Component\HttpKernel\Event\GetResponseEvent))
#2 [internal function]: Symfony\Bundle\SecurityBundle\EventListener\FirewallListener->onKernelRequest(Object(Symfony\Component\HttpKernel\Event\GetResponseEvent), 'kernel.request', Object(Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher))
#3 /var/www/html/brad/vendor/symfony/symfony/src/Symfony/Component/EventDispatcher/Debug/WrappedListener.php(106): call_user_func(Array, Object(Symfony\Component\HttpKernel\Event\GetResponseEvent), 'kernel.request', Object(Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher))
#4 [internal function]: Symfony\Component\EventDispatcher\Debug\WrappedListener->__invoke(Object(Symfony\Component\HttpKernel\Event\GetResponseEvent), 'kernel.request', Object(Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher))
#5 /var/www/html/brad/vendor/symfony/symfony/src/Symfony/Component/EventDispatcher/EventDispatcher.php(212): call_user_func(Object(Symfony\Component\EventDispatcher\Debug\WrappedListener), Object(Symfony\Component\HttpKernel\Event\GetResponseEvent), 'kernel.request', Object(Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher))
#6 /var/www/html/brad/vendor/symfony/symfony/src/Symfony/Component/EventDispatcher/EventDispatcher.php(44): Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(Array, 'kernel.request', Object(Symfony\Component\HttpKernel\Event\GetResponseEvent))
#7 /var/www/html/brad/vendor/symfony/symfony/src/Symfony/Component/EventDispatcher/Debug/TraceableEventDispatcher.php(146): Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.request', Object(Symfony\Component\HttpKernel\Event\GetResponseEvent))
#8 /var/www/html/brad/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php(129): Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher->dispatch('kernel.request', Object(Symfony\Component\HttpKernel\Event\GetResponseEvent))
#9 /var/www/html/brad/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#10 /var/www/html/brad/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php(171): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#11 /var/www/html/brad/web/app_dev.php(18): Symfony\Component\HttpKernel\Kernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#12 {main}

ussuritiger · 2017-06-22T11:22:25Z

This is from the documentation (version 3.3):

start(Request $request, AuthenticationException $authException = null)
This is called if the client accesses a URI/resource that requires authentication, but no authentication details were sent (i.e. you returned null from getCredentials()). Your job is to return a Response object that helps the user authenticate (e.g. a 401 response that says "token is missing!").

My getCredentials method returns null but the start method is never called. An exception is thrown instead.

nospor · 2017-06-22T11:27:26Z

@ussuritiger exactly. It is not a problem if you do not have your own ExceptionListener - then symfony catch that exception and call start() method. If you have your own ExceptionListener, then you have a problem as I have described.

wimpog · 2017-06-22T15:40:51Z

Same issues here! AccessDeniedException instead of AuthenticationException in start(), and the start() method can't be type-hinted with AccessDeniedException and is never called!

chalasr · 2017-06-22T18:32:37Z

Status: reviewed

I'm on it.

Edit: culprit found, working on the fix.

chalasr · 2017-06-25T10:04:19Z

Can someone try #23291 and confirm it fixes the issue?
This is a BC break in the event dispatcher component.

nospor · 2017-06-25T10:36:49Z

@chalasr thank you, I've just checked your fix and seems to be working fine now :)

chalasr · 2017-06-25T10:38:37Z

thanks for the quick confirmation @nospor

nospor · 2017-06-25T10:41:45Z

You are welcome. But it is you who did the most job, so well done and thx once more for your fix :)
I am not familiar with Symfony acceptance process but when we can expect this in official Symfony3.3.x version?

chalasr · 2017-06-25T10:50:12Z

The next maintenance releases should occur in ~10 days (once a month).
Given I think we can expect it to be reviewed quickly (your confirm helps a lot) as this behaviour change may impact more than only the security component, I hope the patch will be part of the next release set.

This PR was merged into the 3.3 branch. Discussion ---------- [Security] Fix Firewall ExceptionListener priority | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23253 | License | MIT | Doc PR | n/a When making EventDispatcher able to lazy load listeners, we stopped using `ContainerAwareEventDispatcher::addListenerService/addSubcriberService`, we use `EventDispatcher::addListener()` instead. This change makes that the order of listeners is different than before, because `ContainerAwareEventDispatcher` calls `addListener()` tardily so that factories are never stored in `EventDispatcher::$listeners`. Example diff due to the behavior change in 3.3 (registering an `AppBundle\ExceptionListener::doCatch()` exception listener in the fullstack): 3.2 ---- ```php array:5 0 => "Symfony\Component\Security\Http\Firewall\ExceptionListener::onKernelException" 1 => "AppBundle\ExceptionListener::doCatch" 2 => "Symfony\Component\HttpKernel\EventListener\ProfilerListener::onKernelException" 3 => "Symfony\Bundle\SwiftmailerBundle\EventListener\EmailSenderListener::onException" 4 => "Symfony\Component\HttpKernel\EventListener\ExceptionListener::onKernelException" ] ``` 3.3 ---- ```php array:5 [ 0 => "AppBundle\ExceptionListener::doCatch" 1 => "Symfony\Component\HttpKernel\EventListener\ProfilerListener::onKernelException" 2 => "Symfony\Bundle\SwiftmailerBundle\EventListener\EmailSenderListener::onException" 3 => "Symfony\Component\Security\Http\Firewall\ExceptionListener::onKernelException" 4 => "Symfony\Component\HttpKernel\EventListener\ExceptionListener::onKernelException" ] ``` (that is what breaks #23253, the lazy listener is called before the runtime firewall exception listener on dispatch). This fixes the order by increasing the security exception listener priority. Commits ------- 8014b38 [Security] Fix Firewall ExceptionListener priority

xabbuh added Bug Security labels Jun 21, 2017

carsonbot added the Status: Needs Review label Jun 21, 2017

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 22, 2017

chalasr mentioned this issue Jun 25, 2017

[Security] Fix Firewall ExceptionListener priority #23291

Merged

weaverryan added the BC Break label Jun 26, 2017

fabpot closed this as completed Jul 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throwing AccessDeniedException when getCredentials() in GuardAuthenticator are null #23253

Throwing AccessDeniedException when getCredentials() in GuardAuthenticator are null #23253

nospor commented Jun 21, 2017 •

edited

chalasr commented Jun 21, 2017 •

edited

nospor commented Jun 22, 2017

ussuritiger commented Jun 22, 2017

nospor commented Jun 22, 2017

wimpog commented Jun 22, 2017 •

edited

chalasr commented Jun 22, 2017 •

edited

chalasr commented Jun 25, 2017

nospor commented Jun 25, 2017

chalasr commented Jun 25, 2017

nospor commented Jun 25, 2017 •

edited

chalasr commented Jun 25, 2017

Throwing AccessDeniedException when getCredentials() in GuardAuthenticator are null #23253

Throwing AccessDeniedException when getCredentials() in GuardAuthenticator are null #23253

Comments

nospor commented Jun 21, 2017 • edited

chalasr commented Jun 21, 2017 • edited

nospor commented Jun 22, 2017

ussuritiger commented Jun 22, 2017

nospor commented Jun 22, 2017

wimpog commented Jun 22, 2017 • edited

chalasr commented Jun 22, 2017 • edited

chalasr commented Jun 25, 2017

nospor commented Jun 25, 2017

chalasr commented Jun 25, 2017

nospor commented Jun 25, 2017 • edited

chalasr commented Jun 25, 2017

nospor commented Jun 21, 2017 •

edited

chalasr commented Jun 21, 2017 •

edited

wimpog commented Jun 22, 2017 •

edited

chalasr commented Jun 22, 2017 •

edited

nospor commented Jun 25, 2017 •

edited