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

Custom AccessDeniedHandler didn't work #28229

Closed
y4roc opened this issue Aug 19, 2018 · 28 comments
Closed

Custom AccessDeniedHandler didn't work #28229

y4roc opened this issue Aug 19, 2018 · 28 comments

Comments

@y4roc
Copy link

y4roc commented Aug 19, 2018

Symfony version(s) affected: 4.1.3

Description
Symfony ignores the custom AccessDeniedHandler. It never will called.

How to reproduce
Read (http://symfony.com/doc/current/security/access_denied_handler.html) and build it in a clean Symfony project. Secure an URL an called it. You will see, this your handler will never called.

@xabbuh
Copy link
Member

xabbuh commented Aug 19, 2018

Can you please create a small example project that allows to reproduce your issue?

@seferov
Copy link
Contributor

seferov commented Aug 19, 2018

Access denied handler is working in \Symfony\Component\Security\Http\Firewall\ExceptionListener::handleAccessDeniedException. However, if current user is not fully fledged instead of AccessDeniedException, InsufficientAuthenticationException is thrown and return before the handler. I guess this is the problem that @thHAN is facing. I don't know if this is expected behaviour but if so there should be a note about this in the documentation.

screenshot_2018-08-20 full authentication is required to access this resource 500 internal server error

On the error page it is also seen that first AccessDeniedException, then InsufficientAuthenticationException is thrown.

@nicoweb
Copy link
Contributor

nicoweb commented Aug 26, 2018

Same problem, if I follow the doc (http://symfony.com/doc/current/security/access_denied_handler.html),
my custom Access Denied Handler Handler is never called

@xabbuh
Copy link
Member

xabbuh commented Aug 26, 2018

@seferov @nicoweb Can one of you create a small example application that allows to reproduce your issue?

@nicoweb
Copy link
Contributor

nicoweb commented Aug 27, 2018

@xabbuh Here a small example: https://github.com/nicoweb/symfony-issue_28229
If I delete the return here, my custom AccessDeniedHandler is executed.

@y4roc
Copy link
Author

y4roc commented Sep 3, 2018

@nicoweb reproduce my problem. I can't give more informations about this issue.

@WestFR
Copy link

WestFR commented Dec 2, 2018

Same problem for me when i use this following : https://symfony.com/doc/current/security/access_denied_handler.html

@Floeig
Copy link

Floeig commented Dec 18, 2018

Same problem here, I followed the @nicoweb, to delete the return and worked as expected. I found another way to have the same result here multiple_guard_authenticators

@otatarintseva
Copy link

otatarintseva commented Mar 22, 2019

Did you try to change app/config/services.yml? Mayby it is just documentation issue? I used AccessDeniedHandler in Symfony 2.8 and it works for me. I configured it according to https://symfony.com/doc/2.8/security/access_denied_handler.html.

@curry684
Copy link
Contributor

curry684 commented Apr 7, 2019

No the issue is real and #30423 is in itself the proper fix.

Status: reviewed

@fabpot fabpot closed this as completed Apr 10, 2019
fabpot added a commit that referenced this issue Apr 10, 2019
This PR was merged into the 3.4 branch.

Discussion
----------

[Security] Rework firewall's access denied rule

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~~#30099~~, #28229
| License       | MIT
| Doc PR        |

Follow tickets provided above to reproduce bugs. (there are also some project examples)

~~In addition, I'm looking for someone who knows an answer to [this](#30099 (comment)) regarding rework in this PR.~~

Commits
-------

5790859 Rework firewall access denied rule
@chalasr
Copy link
Member

chalasr commented Apr 17, 2019

Patch reverted, reopening.

@chalasr chalasr reopened this Apr 17, 2019
@clevercodenl
Copy link

when is this getting fixed ???

@xabbuh
Copy link
Member

xabbuh commented May 3, 2019

@GreenLeewayDesignCastle Would you like to create a pull request?

@clevercodenl
Copy link

@GreenLeewayDesignCastle Would you like to create a pull request?

I'll try to, keep you posted

@nesk
Copy link
Contributor

nesk commented May 22, 2019

Patch reverted, reopening.

Why was it reverted? What was wrong with the patch provided by fabpot?

@DariuszLuber
Copy link

Exactly the same situation - steps taken from doc: https://symfony.com/doc/current/security/access_denied_handler.html
Symfony version: 4.2.7
Any sugestion to get it work?
image

@nesk
Copy link
Contributor

nesk commented Jun 21, 2019

@chalasr Sorry to insist, but why the patch has been reverted? What was wrong with it and what can we do to help fix this really annoying issue?

@walva
Copy link

walva commented Jul 1, 2019

Hi everyone, I am also having an issue with Symfony\Component\Security\Core\Exception\ AccessDeniedException.
If a user tries to access an entity for which he has no access, the @IsGranted("PARTNER_VIEW", subject="center") annotation throws an AccessDeniedException.
Even if the user is logged in, symfony will redirect him to the login page instead of having an error page.

Is this the expected behavior of symfony: redirect a logged user to the login page when an AccessDeniedException is thrown?

I created an AccessDeniedHandler in my application that will display an error page if there is a user logged by following this documentation: https://symfony.com/doc/master/security/access_denied_handler.html.
It works. I'm using 4.2.8.

Btw, in the accessDeniedHandler, the call $request->getUser() returns null. Is this the expected behavior? To access the user, I have to inject the TokenStorageInterface service.

@nesk
Copy link
Contributor

nesk commented Jul 1, 2019

I've managed to mitigate the bug by creating a subscriber listening for thrown AccessDeniedException. I check the stack trace of the exception, if the firewall threw it then I do nothing, otherwise I return my own response with a 403 status code.

namespace App\EventSubscriber;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
use Symfony\Component\Security\Http\Firewall\AccessListener;

class AccessDeniedSubscriber implements EventSubscriberInterface
{
    public function onKernelException(GetResponseForExceptionEvent $event): void
    {
        $exception = $event->getException();

        if ($exception instanceof AccessDeniedException && !self::isThrownByFirewall($exception)) {
             // Create your own response like in a custom access denied handler
             $response = new Response('blablabla', 403);

            $event->setResponse($response);
            $event->stopPropagation();
        }
    }

    public static function getSubscribedEvents()
    {
        return [
            // Define the priority to execute our subscriber before the one from the security component
            KernelEvents::EXCEPTION => ['onKernelException', 1],
        ];
    }

    /**
     * Determines, by analyzing the stack trace, if an exception has been thrown by the firewall.
     */
    private static function isThrownByFirewall(\Throwable $exception): bool
    {
        foreach ($exception->getTrace() as $stackItem) {
            $class = $stackItem['class'] ?? null;
            if ($class === AccessListener::class) {
                return true;
            }
        }

        return false;
    }
}

Once the bug is fixed by Symfony devs, you will have to use a real custom access denied handler and delete this subscriber (it could break easily with a future Symfony update).

@rostandnj
Copy link

rostandnj commented Jul 22, 2019

I have solved the problem by changing
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
with
use Symfony\Component\Security\Core\Exception\AccessDeniedException;

in the AccessDeniedHandler class

@nesk
Copy link
Contributor

nesk commented Oct 1, 2019

Patch reverted, reopening.

Why was it reverted? What was wrong with the patch provided by fabpot?

Asking the same question again… If you want people to help, you have to explain why the patch created by @fabpot was reverted, otherwise we will never be able to make progress on this issue.

@xabbuh
Copy link
Member

xabbuh commented Oct 1, 2019

see #31136 for the context of why the fix was reverted

@nesk
Copy link
Contributor

nesk commented Oct 1, 2019

So, if I understand correctly, this patch brought BC issues to the 3.X and 4.X branches? I understand why it was reverted, but why the patch has not been merged in the master branch? Should I create a PR for this?

(And thank you for the answer 🙂)

@michaelKaefer
Copy link
Contributor

michaelKaefer commented Apr 5, 2020

Based on the workaroung of @nesk for Symfony 5 you can use this workaround until this issue is solved. As soon as it is solved it should be enough to simply delete the file EventSubscriber.php:

<?php

namespace App\EventSubscriber;

use App\Security\AccessDeniedHandler;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
use Symfony\Component\Security\Http\Firewall\AccessListener;

/**
 * This is a temporary workaround until https://github.com/symfony/symfony/issues/28229 is solved.
 *
 * @package App\EventSubscriber
 */
class AccessDeniedSubscriber implements EventSubscriberInterface
{
    private $handler;

    public function __construct(AccessDeniedHandler $handler)
    {
        $this->handler = $handler;
    }

    public function onKernelException(ExceptionEvent $event)
    {
        $exception = $event->getThrowable();

        if ($exception instanceof AccessDeniedException && !self::isThrownByFirewall($exception)) {
            $response = $this->handler->handle($event->getRequest(), $exception);

            $event->setResponse($response);
            $event->stopPropagation();
        }
    }

    public static function getSubscribedEvents()
    {
        return [
            'kernel.exception' => ['onKernelException', 1],
        ];
    }

    /**
    * Determines, by analyzing the stack trace, if an exception has been thrown by the firewall.
    *
    * @param AccessDeniedException $exception
    *
    * @return bool
    */
    private static function isThrownByFirewall(AccessDeniedException $exception): bool
    {
        foreach ($exception->getTrace() as $stackItem) {
            $class = $stackItem['class'] ?? null;
            if ($class === AccessListener::class) {
                return true;
            }
        }

        return false;
    }
}

@HamHamFonFon
Copy link

I'have the same problem but i fixed it by throwing

Symfony\Component\Security\Core\Exception\AccessDeniedException

And not Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException or other AccessDeniedException

@elevado
Copy link

elevado commented May 12, 2020

Once the bug is fixed by Symfony devs, you will have to use a real custom access denied
handler and delete this subscriber (it could break easily with a future Symfony update).

Thanks, it works!

@eerison
Copy link

eerison commented Jul 17, 2020

I'm using Symfony 4.4.10, and It works if I put statusCode=403 in @IsGranted

e.g

@IsGranted("ROLE_ADMIN", statusCode=403)

@wouterj
Copy link
Member

wouterj commented Aug 9, 2020

I've tested this in a real Symfony app and I think this issue is mostly due to confusion about the purpose of the AccessDeniedHandler. It's only meant to be called when a user is authenticated (thus not anonymous), but does not have enough permissions to access the resource. An authentication entry point is meant to catch the anonymous users trying to access a protected resource.

Based on my experiments, I've created a PR to better document this: symfony/symfony-docs#14045

I think this issue can be closed, the access denied handler is working like expected in my tests.

@fabpot fabpot closed this as completed Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests