Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nospor opened this issue Jun 21, 2017 · 11 comments
Closed

Comments

@nospor
Copy link

@nospor nospor commented Jun 21, 2017

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

@chalasr

This comment has been minimized.

Copy link
Member

@chalasr chalasr commented Jun 21, 2017

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

This comment has been minimized.

Copy link
Author

@nospor nospor commented Jun 22, 2017

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

This comment has been minimized.

Copy link

@ussuritiger ussuritiger commented Jun 22, 2017

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

This comment has been minimized.

Copy link
Author

@nospor nospor commented Jun 22, 2017

@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

This comment has been minimized.

Copy link

@wimpog wimpog commented Jun 22, 2017

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

This comment has been minimized.

Copy link
Member

@chalasr chalasr commented Jun 22, 2017

Status: reviewed

I'm on it.

Edit: culprit found, working on the fix.

@chalasr

This comment has been minimized.

Copy link
Member

@chalasr chalasr commented Jun 25, 2017

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

@nospor

This comment has been minimized.

Copy link
Author

@nospor nospor commented Jun 25, 2017

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

@chalasr

This comment has been minimized.

Copy link
Member

@chalasr chalasr commented Jun 25, 2017

thanks for the quick confirmation @nospor

@nospor

This comment has been minimized.

Copy link
Author

@nospor nospor commented Jun 25, 2017

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

This comment has been minimized.

Copy link
Member

@chalasr chalasr commented Jun 25, 2017

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.

@weaverryan weaverryan added the BC Break label Jun 26, 2017
fabpot added a commit that referenced this issue Jul 3, 2017
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
@fabpot fabpot closed this Jul 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.