Redirect authenticated with RememberMeToken user to login form after access check in controller #16026

wiistriker · 2015-09-30T15:58:04Z

I have custom voter for check access to admin of Company entity

public function mainAction(Request $request, $id)
{
    ....

    $this->denyAccessUnlessGranted('admin', $company);

    ....
}

If AccessDeniedException is thrown i get redirected to login page even if user is logged in (with RememberMeToken). Is this expected behaviour?
I want get 403 error page for authenticated user and redirect to login page for anonymous user. What is right way to do this? Throw AccessDeniedHttpException instead?

https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php#L122 this part of code check that user should be not "anonymous" and not "remember me". I am not sure why we need remember me check here?

linaori · 2015-10-01T05:45:54Z

@wiistriker my guess is that both anonymous and rememberme are considered to require an additional login if permissions are missing, elevating their permission level.

wiistriker · 2015-10-01T10:08:36Z

@iltar In my case user doesnt have rights to admin $company even if he fill username and password again. After he fill form, he get proper 403 page. It's very strange behaviour.

wiistriker · 2015-10-01T11:16:03Z

Code which represent flow which i want

         if (!$this->isGranted('admin', $company)) {
            if (!$this->isGranted('IS_AUTHENTICATED_REMEMBERED')) {
                throw $this->createAccessDeniedException();
            } else {
                throw new \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException('Access denied');
            }
        }

If user is logged in he get 403 error page and will be redirected to login form if user is anonymous.

Method $this->denyAccessUnlessGranted('admin', $company); become really useless

aromer · 2015-10-10T20:54:36Z

I've been getting frustrated with this too.

In Symfony/Component/Security/Http/Firewall/ExceptionListener.php it checks for fullyFledged and then begins the authentication process. The default AuthenticationTrustResolver will always return false if the user is only "remembered".

private function handleAccessDeniedException(GetResponseForExceptionEvent $event, AccessDeniedException $exception)
{
    $event->setException(new AccessDeniedHttpException($exception->getMessage(), $exception));

    $token = $this->tokenStorage->getToken();
    if (!$this->authenticationTrustResolver->isFullFledged($token)) {
        if (null !== $this->logger) {
            $this->logger->debug('Access denied, the user is not fully authenticated; redirecting to authentication entry point.', array('exception' => $exception));
        }

        try {
            $insufficientAuthenticationException = new InsufficientAuthenticationException('Full authentication is required to access this resource.', 0, $exception);
            $insufficientAuthenticationException->setToken($token);

            $event->setResponse($this->startAuthentication($event->getRequest(), $insufficientAuthenticationException));
        } catch (\Exception $e) {
            $event->setException($e);
        }

        return;
    }

gharlan · 2016-06-28T09:26:18Z

I've been getting frustrated with this too.

Same here. But don't know how to fix. How to decide when redirection is correct, and when not.

SuperToma · 2018-07-12T20:25:26Z

This strange behaviour is still up to date in Symfony4.
I used something like this instead :

if (!$this->isGranted('ROLE_USER')) {
    return $this->json(['message' => 'Please sign up before'], 403);
}

or :

if (!$this->isGranted('ROLE_USER')) {
    return new Response('Please sign up before', Response::HTTP_FORBIDDEN);
}

ahmadmayahi · 2019-01-17T06:26:35Z

Same for me here, I use Symfony 4.2. I even tried to handle the denyAccessException but it didn't work for me.

chris-k-k · 2020-06-19T22:46:37Z

<- new to Symfony

Stumbled across the same issue as OP. My solution was writing a custom ExceptionListener.

app/config/services.yaml:
App\Security\CustomExceptionListener: tags: - { name: 'kernel.event_listener', event: 'kernel.exception', method: 'onKernelException', priority: 100 }

app/src/security/CustomExceptionListener.php

public $container;
private $tokenStorage;

public function __construct(ContainerInterface $container, TokenStorageInterface $tokenStorage)
{
    $this->container = $container;
    $this->tokenStorage = $tokenStorage;
}

/**
 * Handles access denied exception if RememberMeToken is set
 */
public function onKernelException(ExceptionEvent $event)
{
    $exception = $event->getThrowable();
    do {
        if ($exception instanceof AccessDeniedException) {
            $this->handleAccessDeniedException($event, $exception);

            return;
        }
    } while (null !== $exception = $exception->getPrevious());
}

private function handleAccessDeniedException(ExceptionEvent $event, AccessDeniedException $exception)
{
    $event->setThrowable(new AccessDeniedHttpException($exception->getMessage(), $exception));

    $authenticationTrustResolver = $this->container->get('security.authentication.trust_resolver');
    $token = $this->tokenStorage->getToken();
    if ($authenticationTrustResolver->isRememberMe($token) == true) {
        $event->stopPropagation();
    }
}

If you don't want to inject the container, you can alias the AuthenticationTrustResolver and inject it into the class constructor.

app/config/services.yaml:
Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolverInterface: '@security.authentication.trust_resolver'

This way, the 'isGranted' and 'denyAccessUnlessGranted' functionality is available again and does not redirect to the login page.

danieldenbraven · 2020-07-03T09:03:35Z

You can just create your own AuthenticationTrustResolver and decide yourself which are the conditions that are to be considered if a user isFullFledged or not, so the user will just get the access denied page instead of a redirect.

I do have the feeling that this feature assumes a lot of things about the workflows that people use. Maybe I didn't read or find the documentation for this, but we were throwing AccessDenied exceptions (or bundles that we use) for the purpose of showing an access denied page and be done with it (we upgraded from an older version of Symfony), but now the framework takes over this and redirects the user to a login page.

LaterEdit: I worked on way too old versions on Symfony up until recently. The documentation actually is quite clear on what you can do: https://symfony.com/doc/4.4/security/access_denied_handler.html

carsonbot · 2021-02-18T13:02:00Z

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

carsonbot · 2021-03-04T13:29:29Z

Friendly ping? Should this still be open? I will close if I don't hear anything.

aromer · 2021-03-04T14:06:45Z

I can't confirm. I had reported my issue 5 years ago and ended up resolving it by extending the AuthenticationTrustResolver and providing the desired functionality within my application code, essentially the solution that @daniel-iwaniec suggested. That particular application is no longer under active development so I'm unsure how newer versions of Symfony may have affected that solution.

ghost · 2021-03-16T13:05:45Z

@aromer I can confirm, it's still an issue sadly.

carsonbot · 2021-09-17T13:01:04Z

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

carsonbot · 2021-10-01T13:01:26Z

Just a quick reminder to make a comment on this. If I don't hear anything I'll close this.

wiistriker · 2021-10-01T13:04:12Z

8000

wouterj · 2021-10-01T16:01:21Z

I see mostly problems in this issue, but no proposed solutions on how to "fix" this issue in Symfony.

The only way I can see this issue move forward and no longer get these stalled questions is if people proposed solutions, maybe even as a pull request.

Note that a rememberme cookie is considered to be a weak authentication mechanism. As such, it is not considered "full fledged". That logic still makes sense to me.

chris-k-k · 2021-10-02T04:09:46Z

I didn't work on the project that showed this behaviour for over a year (symfony 4.x), but I tried to replicate it in my current project (symfony 5.3).
The behaviour is still the same: If a user returns to the site with a remember-me cookie set and tries to access a page he/she is not authorized to view, instead of being redirected to the 403 page he/she is redirected to the login page.

@danieldenbraven is right: the solution can be found in the documentation
https://symfony.com/doc/current/security/access_denied_handler.html#customizing-all-access-denied-responses

Not setting a custom response and stopping event propagation in case of a remember-me token redirects to the desired 'Access denied' page. The exception is no longer 'overwritten' by a any further exceptions. (E.g. a 401 response with a redirect to the login page.)

namespace App\EventListener;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\ExceptionEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\Security\Core\Authentication\AuthenticationTrustResolver;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;

class AccessDeniedListener implements EventSubscriberInterface
{
    private $tokenStorage;

    public function __construct(TokenStorageInterface $tokenStorage)
    {
        $this->tokenStorage = $tokenStorage;
    }

    public static function getSubscribedEvents(): array
    {
        return [
            // the priority must be greater than the Security HTTP
            // ExceptionListener, to make sure it's called before
            // the default exception listener
            KernelEvents::EXCEPTION => ['onKernelException', 2],
        ];
    }

    public function onKernelException(ExceptionEvent $event): void
    {
        $exception = $event->getThrowable();
        if (!$exception instanceof AccessDeniedException) {
            return;
        }

        // ... perform some action (e.g. logging)

        // optionally set the custom response
        //$event->setResponse(new Response(null, 403)); <-- *** no custom response! *** default behaviour: redirect to login page!

        // or stop propagation (prevents the next exception listeners from being called)
        $authenticationTrustResolver = new AuthenticationTrustResolver();
        $token = $this->tokenStorage->getToken();
        if ($authenticationTrustResolver->isRememberMe($token)) {
            $event->stopPropagation(); // <-- *** stop propagation *** if user is logged in via remember-me cookie => redirect to 'Access denied' page!
        }
    }
}

Whether this opens up any security issues I'm not sure... access to any other 'secured' pages the user ought to have access to (e.g. to a 'my profile' page) is granted as if he/she was authorized via a regular login.

ytilotti · 2021-11-30T17:11:32Z

Same issue here on 5.3. There is solution using @isGranted for exemple but not solve the issue:

use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted;

**
 * @IsGranted("admin", subject="company", statusCode=403)
 */
public function mainAction(Request $request, Company $company)
{
    ....
}

In my case, I don't use denyAccessUnlessGranted() but:

if(!$this->isGranted('admin', $company)) {
    throw new \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException('Access denied');
}

carsonbot · 2022-05-31T13:00:58Z

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

carsonbot · 2022-06-14T13:05:57Z

Just a quick reminder to make a comment on this. If I don't hear anything I'll close this.

uncaught · 2022-06-20T07:14:38Z

This is still present and needs to be resolved.

Symfony assumes too much when changing an AccessDeniedException into an InsufficientAuthenticationException. There is no basis to do so. If the developer expects the user to be fully authenticated, they can query for IS_AUTHENTICATED_FULLY themselves.

uncaught · 2022-06-21T07:13:38Z

@wouterj in regards to your query for a solution, I would suggest the following:

Remove the conversion into an InsufficientAuthenticationException in handleAccessDeniedException
Instead create a handleInsufficientAuthenticationException that sets the startAuthentication response
Add a denyAccessUnlessAuthenticatedFullyFledged() to the controller trait that checks for isGranted(IS_AUTHENTICATED_FULLY) and then throws InsufficientAuthenticationException if false.

That way developers have all the control they need and an example in the code if they wish to programmatically kick the user to the authentication start.

chris-k-k · 2022-06-22T08:19:39Z

@uncaught just for my understanding:
This would mean that a check for isGranted('ROLE_NAME') will evaluate to true regardless of whether the user logged in via form or remember_me token? (Provided the respective role is asigned to the user.) Only the explicit check for isGranted(IS_AUTHENTICATED_FULLY) will throw an exception and redirect to the login page (per default) if the user is authenticated via remember_me token?
If so, I believe this is the most elegant way to resolve this issue...

uncaught · 2022-06-22T08:49:53Z

Yes and no. isGranted should never throw anything, just return a bool.

The isGranted('ROLE_NAME') result should be deterministic to the user and not the authentication method. Authentication is not the same as authorization. Mixing these concepts got us into this problem.

If you think it cleaner, we could make denyAccessUnlessGranted(IS_AUTHENTICATED_FULLY) throw the exception. Then we won't need an additional method in the controller trait. But I'm unsure if that would be a good design pattern to behave differently for exactly one argument.

An alternative very simple fix would be to explicitly check the roles before jumping to conclusions:

src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php

private function handleAccessDeniedException(GetResponseForExceptionEvent $event, AccessDeniedException $exception)
    {
        $event->setThrowable(new AccessDeniedHttpException($exception->getMessage(), $exception));

        $token = $this->tokenStorage->getToken();
-       if (!$this->authenticationTrustResolver->isFullFledged($token)) {
+       if (in_array(IS_AUTHENTICATED_FULLY, $exception->getAttributes()) && !$this->authenticationTrustResolver->isFullFledged($token)) {

chris-k-k · 2022-06-23T07:59:07Z

@uncaught tyvm for your response! :)
To my (rather limited) understanding, your simple fix doesn't - as far as I can see, I'm not sure :) - address the issue, namely that a user authenticated with a remember_me token always gets redirected to the login page whenever he/she tries to access protected pages, regardless of whether he/she would have access with a full-fledged token. Due to this line which is only circumvented with a full-fledged token:

$event->setResponse($this->startAuthentication($event->getRequest(), $insufficientAuthenticationException));

Which makes no sense to me: assuming the user tries to access a page he/she wouldn't have access to anyway isn't mitigated by a redirect to the login page (only to get a "Access denied 403" response immediately after logging in).

This minor change in src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php has the desired effect:

- if (!$this->authenticationTrustResolver->isFullFledged($token)) {
+ if (!$this->authenticationTrustResolver->isFullFledged($token) && !$this->authenticationTrustResolver->isRememberMe($token)) {

With a major drawback: the possibilty for a user to upgarde his/her token from remember_me to full-fledged is removed completely. To avoid this from happening, I wrote a custom AccessDeniedListener when I ran into this issue a few years back (Symfony 5.3.3):

public function onKernelException(ExceptionEvent $event): void
    {
        $exception = $event->getThrowable();
        if (!$exception instanceof AccessDeniedException) {
            return;
        }

        // stop propagation (prevents the next exception listeners from being called)
        $authenticationTrustResolver = new AuthenticationTrustResolver();
        $token = $this->tokenStorage->getToken();
        if ($authenticationTrustResolver->isRememberMe($token)) {
            // stop propagation if user is logged in via remember-me cookie => redirect to 'Access denied' page!
            $event->stopPropagation(); 
        }

        // else, default behaviour: redirect to login page!
    }

Since there are always 2 exceptions queued up when access is denied (the initial AccessDeniedException and the AccessDeniedHttpException from method handleAuthenticationException), stopping the event propagation forces the framework to only handle the first one i.e. the user gets to see the "Access denied 403" page and is not redirected, regardless of the whether full-fledged or remembered. An anonymous user still gets redirected to the login page.

TL;DR Though I may very well be wrong about what your simple fix does, I nethertheless prefer your initial proposal of essentially splitting the exception in two i.e. not converting one exception into another:

a (regular) AccessDeniedHttpException that gets thrown when isGranted evaluates to false without the startAuthentication response unless the user is anonymous.
a InsufficientAuthenticationException that is only thrown when the developer explicitly asks for isGranted('IS_AUTHENTICATED_FULLY') which would set the startAuthentication response in case of a remember_me token present or an anonymous user. This way one would be able to mitigate the fact that a remember_me token is considered a weak protection, but it's the developers responsibility to make sure that sensitive data can only be accessed by full-fledged users.

@wouterj : Note that a rememberme cookie is considered to be a weak authentication mechanism. As such, it is not considered "full fledged". That logic still makes sense to me.

Your initial proposal - though a little bit more 'verbous' and not a 'simple fix' - I'd consider to be very 'clean' and the best solution:

That way developers have all the control they need and an example in the code if they wish to programmatically kick the user to the authentication start.

stof · 2022-06-23T08:36:31Z

Offering to get a full-fledged token before getting an error is a feature we have since Symfony 2.0, because it is totally possible for any voter to include a check on IS_AUTHENTICATED_FULLY as part of a more complex decision.

uncaught · 2022-06-23T09:32:04Z

@stof what is your point?

It's not an offer, it's a forced redirect to a 401 response (or login page). I find that very inconvenient when I already know that changing the authentication method will not change the end result.

@chris-k-k let me explain what the simple fix would do:

if (in_array(IS_AUTHENTICATED_FULLY, $exception->getAttributes()) && !$this->authenticationTrustResolver->isFullFledged($token)) {

This would mean, that only if you threw an AccessDeniedException and set the attributes to include IS_AUTHENTICATED_FULLY, you would be redirected to the login if you were not fully authenticated.

I found this very simple because denyAccessUnlessGranted() already fills these attributes.

So lets say you are logged in via remember me:

denyAccessUnlessGranted(['ROLE_X', IS_AUTHENTICATED_FULLY]) will redirect you to the login (or 401 resposne)
denyAccessUnlessGranted('ROLE_X') will give you 403 response

chris-k-k · 2022-06-23T10:40:12Z

@uncaught ty for the explanation! :)
So your quick fix achieves the same thing as what I did a few years ago if the additional attribute 'IS_AUTHENTICATED_FULLY' is set: it prevents the forced redirect when authenticated via the remember_me token. With your solution having the added benefit of making it optional instead of hard-coded, which is much more preferable...

I'd still prefer the separation of authorization and authentication from your initial proposal:

The isGranted('ROLE_NAME') result should be deterministic to the user and not the authentication method. Authentication is not the same as authorization. Mixing these concepts got us into this problem.

This still applies, no? Given that 'ROLE_NAME' is the authorization (or 'IS_ALLOWED_TO_EDIT' or w/e) and 'IS_AUTHENTICATED_FULLY' would be the authentication.

$this->isGranted(['ROLE_ADMIN', 'IS_AUTHENTICATED_FULLY']);
or
$this->denyAccessUnlessGranted(['ROLE_ADMIN', 'IS_AUTHENTICATED_FULLY']);

EDIT: Just found out that the support for an array of attributes has been removed a long, long time ago ;) #33584
Internally, it's still an array due to backwards-compatibility issues, I believe.

wiistriker · 2022-06-24T07:44:13Z

btw i am still using simple code:

if (!$this->isGranted('admin', $company)) {
            if (!$this->isGranted('IS_AUTHENTICATED_REMEMBERED')) {
                throw $this->createAccessDeniedException();
            } else {
                throw new \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException('Access denied');
            }
        }

chris-k-k · 2022-06-24T10:32:55Z

Pretty inconvenient if this needs to be repeated in every controller.

After trying to implement @uncaught's solution (which unfortunately didn't work due to isGranted() & denyAccessUnlessGranted() not allowing an array of attributes), this is what I did to get the desired result (in an old project - Symfony 5.3.3):

// Symfony\Component\Security\Http\Firewall\ExceptionListener
- if (!$this->authenticationTrustResolver->isFullFledged($token)) { ... }
+ if (null === $this->tokenStorage->getToken() || in_array('IS_AUTHENTICATED_FULLY', $exception->getAttributes())) { ... }

If there is no token set (i.e. the user is not authenticted at all) or the exception attribute is set to 'IS_AUTHENTICATED_FULLY', the authentication process will start (e.g. redirecting to the login page). Else, a simple access denied exception is thrown.

I'm now able to explicitly secure resources (with controller attributes for example):

#[Route('/admin', name: 'admin_index')]
#[isGranted('IS_AUTHENTICATED_FULLY')]
public function indexAction(): Response { ... }

I can customize the behaviour of the authentication process by overriding the start() method of class Symfony\Component\Security\Http\Authenticator\AbstractLoginFormAuthenticator (documentation):

class LoginFormAuthenticator extends AbstractLoginFormAuthenticator {

    ...

    public function start(Request $request, AuthenticationException $authException = null): Response
    {
        $authenticationTrustResolver = new AuthenticationTrustResolver();
        if ($authenticationTrustResolver->isRememberMe($authException->getToken())) {
            // user is remembered - add flash message that being remembered won't suffice for accessing this resource
            dd('insufficient authentication');
        }

        if (!$authException->getToken()) {
            // visitor is not authenticated at all - add flash message that he/she needs to login, so access rights can be evaluated
            dd('no authentication at all');
        }

        // redirect to login page where the appropriate flash message will be displayed
        $url = $this->getLoginUrl($request);

        return new RedirectResponse($url);
    }

    ...

}

Here, it would be cleaner to be able to differentiate between a no authentication exception and an insufficient authentication exception (as per uncaught's initial proposal). It would make it possible to check which kind of exception was thrown instead of examining the token. But regardless, it works...

uncaught · 2022-06-24T11:22:47Z

I'd still prefer the separation of authorization and authentication

With the changes the separation will at least be a bit more clear. If you wanted to split hairs, the whole idea of IS_AUTHENTICATED_FULLY as role check is weird in the first place. You basically ask the authorization system whether the user is authenticated or not. But then again, without authentication, you can't have authorization, so triggering it, is still somewhat fine.

After trying to implement @uncaught's solution (which unfortunately doesn't work due to isGranted() & denyAccessUnlessGranted() not allowing an array of attributes)

Oh, I didn't know that was dropped with Symfony 5. I'm still on 4, but never used arrays anyway, so I guess I've never seen the deprecation notices about it. Weird that the AccessDescisionManager keeps using arrays though.

And now I see that with Symfony 6 the AuthorizationChecker no longer authenticates the user if they are not authenticated. So I suppose I am a bit outdated. I don't know at which point the user gets authenticated anymore. Is that code in the ExceptionListener the only thing left that starts the authentication? Then your suggestion to check for a null token seems logical.

Although the fact that they left the attributes in there as array makes me a bit nervous because your code assumes that the IS_AUTHENTICATED_FULLY was in fact the attribute that failed. So I'd still leave the isFullyFledged check in there to be safe:

// Symfony\Component\Security\Http\Firewall\ExceptionListener
- if (!$this->authenticationTrustResolver->isFullFledged($token)) { ... }
+ if (null === $this->tokenStorage->getToken() || in_array('IS_AUTHENTICATED_FULLY', $exception->getAttributes()) && !$this->authenticationTrustResolver->isFullFledged($token)) { ... }

Or I might be paranoid.

stof · 2022-06-24T12:08:58Z

Well, your authorization layer is perfectly legitimate to grant or reject a permission based on the level of trust of the authentication.

And my answer was about logic that you might implement in custom voters. ->isGranted('ACCESS_ADMINISTRATION_AREA') might rely on the trust level to take a decision.

carsonbot · 2022-12-30T12:46:09Z

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

uncaught · 2023-02-22T07:46:21Z

We've just upgraded to 5.4 and the behavior is still an issue.

Overriding the AuthenticationTrustResolver is not an option because we want to use those trust levels in the future.

Luckily we have a custom authenticator with an entry point now, so this is my new 5.4 work around, basically what @chris-k-k suggested:

  public function start(Request $request, AuthenticationException $authException = null): Response {
    if ($authException instanceof InsufficientAuthenticationException && $authException->getToken()?->getUser()) {
      return new Response('Access denied', 403);
    }
    return new Response('Not logged in', 401);
  }

carsonbot · 2023-09-04T09:08:18Z

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

carsonbot · 2023-09-22T10:41:10Z

Just a quick reminder to make a comment on this. If I don't hear anything I'll close this.

carsonbot · 2023-10-06T19:25:58Z

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

pauljura · 2024-03-12T00:29:31Z

Hi all, I am struggling with this at the moment and I think this issue deserves to be re-opened.

Here's how a typical project goes for me:

I build the main section of the website, whatever it happens to be.
I add a login page
I protect most of the site with #[IsGranted('IS_AUTHENTICATED_FULLY')] in my controllers.
Some parts might need a custom voter (e.g. user can only edit their own blog posts, or whatever), so I protect those routes with $this->denyAccessUnlessGranted
Now I have a properly working site that has a login page and working security. Great.
Some time later I decide to add "remember me". No problem, I just add that checkbox to the login page, and I replace IS_AUTHENTICATED_FULLY with IS_AUTHENTICATED_REMEMBERED all across my site. Easy!

I expect my site to just work as normal after adding "remember me".

Except it doesn't. Because everywhere that I have a route protected with a custom voter and $this->denyAccessUnlessGranted I get different behaviour. Instead of a 403 Forbidden response, as it was, and as I expect it to still be, users are being redirected to login. Not only is this not what I wanted, but it's also confusing for users too.

Now this is not some bizarre edge case. This is a typical project with typical security. But the behaviour I get is not what I expect, and I think that it's not what most developers expect.

So now I'm forced to write a custom event handler to get the kind of behaviour that I think is "normal" and should come out of the box.

Look, I get why it was written the way it was written. The code that checks access can't make assumptions about the level of trust required to make a decision. Sure. But you also need to recognise that the way it is implemented at the moment produces behaviour that is not what most developers and users would expect.

The solution? I don't know... The problem seems to be that we have one entry point for access checks, but two different outcomes (redirect vs forbidden). Having one entry point is great and convenient, but when the developer doesn't have control over the outcome, it breaks down. Actually, it's not that there are two different outcomes, it's that the outcomes are determined by the state of the user, instead of by the check itself. If I'm checking for IS_AUTHENTICATED_FULLY and that fails, then the response should always be redirect to login. But if I'm checking for "can this user edit this blog post" and that fails, then the outcome should always be 403 Forbidden. Of course this gets tricky if the developer wants to do both in a single check. So maybe don't allow that?

End of the day, I want to have a way to say "if this check fails, I want to return a 403 Forbidden, even if the user is authenticated via remember me", as well as "if this check fails, then redirect to login". Maybe add a method called forbidUnlessGranted that always returns a 403 Forbidden response? Or just change how access denied is handled, so that users with "remember me" get treated the same as a fully authenticated user (meaning they get a forbidden response instead of a redirect to login), and if a redirect to login is actually required then leave it up to the developer to enforce that by making a separate check for IS_AUTHENTICATED_FULLY.

uncaught · 2024-03-12T05:21:02Z

I agree 100%. This wouldn't be a problem if the authorization system wasn't abused to check for authentication states in the first place.

I would remove those IS_AUTHENTICATED* from there, but that would probably be too much of change.

As a compromise, the system should only assume a 401 response is the right answer if one of the authentication attributes was checked in the authorization system.

fabpot added the Security label Oct 5, 2015

javiereguiluz added the Enhancement label Feb 6, 2016

carsonbot added the Stalled label Feb 18, 2021

carsonbot removed the Stalled label Mar 4, 2021

carsonbot added the Stalled label Sep 17, 2021

carsonbot removed the Stalled label Oct 1, 2021

wouterj added the Status: Waiting feedback label Oct 1, 2021

carsonbot added the Stalled label May 31, 2022

carsonbot removed the Stalled label Jun 20, 2022

carsonbot added the Stalled label Dec 30, 2022

carsonbot removed the Stalled label Feb 22, 2023

carsonbot added the Stalled label Sep 4, 2023

carsonbot closed this as completed Oct 6, 2023

pauljura mentioned this issue Mar 18, 2024

Improve behaviour for Access Denied with Remember Me #54318

Open

eltharin mentioned this issue Jun 30, 2024

Using IsCsrfTokenValid attribute with invalid token redirects user to login page #57343

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redirect authenticated with RememberMeToken user to login form after access check in controller #16026

Redirect authenticated with RememberMeToken user to login form after access check in controller #16026

Redirect authenticated with RememberMeToken user to login form after access check in controller #16026

Redirect authenticated with RememberMeToken user to login form after access check in controller #16026

Comments