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

Convert InsufficientAuthenticationException to HttpAccessDeniedException #8467

Closed
Koc opened this issue Jul 10, 2013 · 54 comments
Closed

Convert InsufficientAuthenticationException to HttpAccessDeniedException #8467

Koc opened this issue Jul 10, 2013 · 54 comments

Comments

@Koc
Copy link
Contributor

Koc commented Jul 10, 2013

    firewalls:
        dev:
            pattern:  ^/(_(profiler|wdt|fragment)|css|images|js)/
            security: false

        main:
            pattern:    /
            ololo: ~
            provider: users_entity
            anonymous: ~

    access_control:
        - { path: ^/admin, roles: ROLE_ADMIN }

open as anonymous something like /admin/dashboard and got 500. Expected 403.

[2013-07-10 17:27:49] security.INFO: Populated SecurityContext with an anonymous Token [] []
[2013-07-10 17:27:49] security.INFO: No expression found; abstaining from voting. [] []
[2013-07-10 17:27:49] security.DEBUG: Access is denied (user is not fully authenticated) by "/vhosts/www.ololo.com/new/vendor/symfony/symfony/src/Symfony/Component/Security/Http/Firewall/AccessListener.php" at line 73; redirecting to authentication entry point [] []
[2013-07-10 17:27:49] request.CRITICAL: Uncaught PHP Exception Symfony\Component\Security\Core\Exception\InsufficientAuthenticationException: "Full authentication is required to access this resource." at /vhosts/www.ololo.com/new/vendor/symfony/symfony/src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php line 109 {"exception":"[object] (Symfony\\Component\\Security\\Core\\Exception\\InsufficientAuthenticationException: Full authentication is required to access this resource. at /vhosts/www.ololo.com/new/vendor/symfony/symfony/src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php:109, Symfony\\Component\\Security\\Core\\Exception\\AccessDeniedException: Access Denied at /vhosts/www.ololo.com/new/vendor/symfony/symfony/src/Symfony/Component/Security/Http/Firewall/AccessListener.php:73)"} []
@stof
Copy link
Member

stof commented Jul 10, 2013

This is weird. It should be handled by the ExceptionController of the security component already

@Koc
Copy link
Contributor Author

Koc commented Jul 10, 2013

Please explain a little bit particularly why.

@stof
Copy link
Member

stof commented Jul 10, 2013

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php is the exception listener I'm talking about. The Firewall is registering on the kernel.exception event to handle these exceptions.

@Koc
Copy link
Contributor Author

Koc commented Jul 10, 2013

Oh, I see. It causes because my configuration hasn't login_form. But security component hasn't ExceptionController.

@jfsimon
Copy link
Contributor

jfsimon commented Jul 15, 2013

@Koc can you confirm there is no framework problem here and close this issue?

@Koc
Copy link
Contributor Author

Koc commented Jul 15, 2013

I cann't confitm that.

@jfsimon
Copy link
Contributor

jfsimon commented Jul 15, 2013

@Koc then can you point the real issue and write a failing test?

@Koc
Copy link
Contributor Author

Koc commented Jul 15, 2013

I think that better ask @schmittjoh is this fw problem or no.

@jakzal
Copy link
Contributor

jakzal commented Dec 19, 2013

Might be related: #9823

fabpot added a commit that referenced this issue Dec 29, 2013
…eniedException if is not first exception (fabpot)

This PR was merged into the 2.3 branch.

Discussion
----------

[Security] Fix ExceptionListener to catch correctly AccessDeniedException if is not first exception

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9544, #8467?, #9823
| License       | MIT
| Doc PR        |

Same as #9823 but with some refactoring of the code and with some unit tests.

When merging to 2.4, the unit tests can be simplified a bit.

Commits
-------

172fd63 [Security] made code easier to understand, added some missing unit tests
616b6c5 [Security] fixed error 500 instead of 403 if previous exception is provided to AccessDeniedException
@fabpot
Copy link
Member

fabpot commented Dec 29, 2013

@Koc #9879 should have fixed this issue. Can you confirm?

@Koc
Copy link
Contributor Author

Koc commented Dec 29, 2013

No, this issue isn't fixed. I got

Full authentication is required to access this resource.
500 Internal Server Error - InsufficientAuthenticationException
1 linked Exception:  AccessDeniedException

@Koc
Copy link
Contributor Author

Koc commented Dec 29, 2013

Tryed on the 0285bfd revision

@lwojciechowski
Copy link

I can confirm this with Symfony 2.4.2 and using simple_preauth.

@pkruithof
Copy link
Contributor

Also confirming for 2.4.2, I think I know how this occurs in my setup: I have an Api which uses a token supplied via a header to authenticate. There is no authentication entry point: if there's no token supplied, a 401 response should be returned directly.

This line shows that the exception is re-thrown when this is the case.

@Koc
Copy link
Contributor Author

Koc commented Mar 26, 2014

Other problem: I'm logged via rememberme. Throwing AccessDeniedException from controller and got redirected to the login. Of course login redirects me to the frontpage because I'm already authenticated. And all this madness goes instead of displaying simle page with http 403 code.

@stof
Copy link
Member

stof commented Mar 27, 2014

@Koc this is an issue in your login page. If you are authenticated through remember_me only, you should still be able to access the login form to login again as a fully authenticated user

@lwojciechowski
Copy link

@pkruithof did you find workaround for this?

@pkruithof
Copy link
Contributor

@nostrzak yes I did: I added an exception listener that checks for these exceptions and sets the right response: https://github.com/financial-media/FMKeystoneBundle/blob/master/src/FM/KeystoneBundle/EventListener/ExceptionListener.php

The listener must have a high priority, else it's picked up by the firewall listener (IIRC).

@lwojciechowski
Copy link

I don't know if I was blind before but in the cookbok entry there is section exactly about this. Just for reference.

Ping: @pkruithof

@pkruithof
Copy link
Contributor

Seems it was added in 2.4, I never noticed it, thanks for the link!

@fabpot
Copy link
Member

fabpot commented Apr 28, 2014

Can we close this one?

@Koc
Copy link
Contributor Author

Koc commented Apr 28, 2014

@fabpot no, please see my comment #8467 (comment) above

@samsamm777
Copy link

@fabpot Can confirm this is an issue when using simple_preauth and the anonymous firewall flag.

@pkruithof
Copy link
Contributor

To get this issue moving again, I set up a symfony-standard branch with a failing test:

https://github.com/pkruithof/symfony-standard/tree/issue-8467-invalid-status-codes

Basically, I've set up two firewalls with two authentication mechanisms. The first one is stateless username/password authentication via a POST request with a JSON body, like this:

curl -XPOST -d '{"username":"foo","password":"bar"}' http://localhost/api/tokens

This returns a token which can be used in the second authentication mechanism, using it as a query parameter. The second mechanism is set up exactly as described in this cookbook article.

The difference between the two is that the second uses the simple_preauth key in the firewall, while the first one has a custom authentication provider. I've written two tests which do the same for either firewalls: authenticate with valid and invalid credentials. The simple_preauth firewall returns proper 401/403 responses, but the other one returns 500 responses:

$> ./bin/phpunit -c app --filter ApiKeyTest                                                                                               PHPUnit 4.1.4 by Sebastian Bergmann.

Configuration read from /Users/peter/projects/symfony-standard/app/phpunit.xml.dist

....

Time: 646 ms, Memory: 20.00Mb

OK (4 tests, 4 assertions)
$> ./bin/phpunit -c app --filter TokenTest                                                                                                PHPUnit 4.1.4 by Sebastian Bergmann.

Configuration read from /Users/peter/projects/symfony-standard/app/phpunit.xml.dist

.FF

Time: 795 ms, Memory: 20.25Mb

There were 2 failures:

1) Acme\ApiBundle\Tests\Functional\TokenTest::testNoAuthentication
Failed asserting that 500 matches expected 401.

/Users/peter/projects/symfony-standard/src/Acme/ApiBundle/Tests/Functional/TokenTest.php:35

2) Acme\ApiBundle\Tests\Functional\TokenTest::testBadCredentials
Failed asserting that 500 matches expected 401.

/Users/peter/projects/symfony-standard/src/Acme/ApiBundle/Tests/Functional/TokenTest.php:50

FAILURES!                            
Tests: 3, Assertions: 4, Failures: 2.

So hopefully with this failing test we can actually fix this issue. As I stated earlier, I think this is because the failing firewall does not have an entry point, which is a design choice (there is no login form or anything in this example). And I guess the simple_preauth firewall has this covered. If this is the case, something like lexik/LexikJWTAuthenticationBundle#7 will fix it, but I'm not sure if that's the way Symfony should handle it.

@dmitrybelyakov
Copy link

Confirming. Same behaviour for me. InsufficientAuthenticationException is returned with 500 code.
Having onAuthenticationFailure() handler doesn't help either because it never arrives there.

@nicolas-grekas nicolas-grekas changed the title Convert InsufficientAuthenticationException to HttpAccessDinedException Convert InsufficientAuthenticationException to HttpAccessDeniedException Nov 19, 2015
@jakzal jakzal removed the Unconfirmed label Dec 8, 2015
@jakzal
Copy link
Contributor

jakzal commented Dec 10, 2015

@pkruithof thank you for providing code that would reproduce this issue. Your case could be easily solved by doing what was previously suggested by @dmaicher and the docs - catching the AuthenticationException and setting a 401/403 response on the event (one of the exceptions was actually a PHPUnit_Framework_Error).

I'm assuming @Koc's problem has the same root cause as @pkruithof (please provide some code if this is not the case).

To summarise options to solve this issue (already mentioned in the comments above), we could:

From the end-user's perspective option 2 would be the best, as he wouldn't have to do anything to get a correct response. It is a behaviour change though, and we need to consider if this is a bug fix or could it be a BC break (could anyone rely on this behaviour)?

If option 2 is not viable, then I'd vote for option 1.

@pkruithof
Copy link
Contributor

@jakzal we've since implemented the third option (entry point). However I do still think this is a workaround, rather than a solution. As I mentioned before:

I think this is because the failing firewall does not have an entry point, which is a design choice (there is no login form or anything in this example).

The entry point interface clearly states that it's:

used to start the authentication scheme

So when returning a 401 other than http basic auth (which provides a login form in the browser), the response does not start any form of authentication, but rather denies access.

If option 2 is not viable, then I'd vote for option 1.

I agree.

@COil
Copy link
Contributor

COil commented Feb 11, 2016

Also having the same issue, I'm giving ROLES depending on the current application theme (using the LiipThemeBundle) to anonymous users. I don't have login form at all. I have several actions that should be only be accessed for a give theme.
Using:

 * @Security("has_role('ROLE_THEME1')")

When browsing THEME2 returns the same error 500:

Full authentication is required to access this resource.
500 Internal Server Error - InsufficientAuthenticationException
1 linked Exception: AccessDeniedException »

@merk
Copy link
Contributor

merk commented Sep 19, 2016

I am encountering this issue with the FrameworkExtraBundle's @Security annotation. When the expression fails, the SecurityListener throws an AccessDeniedException as expected, but it is not handled because I do not have form_login or an entry point defined.

Because the exceptions are not occurring in the authentication listeners for me, I am not able to do points 1 or 2 proposed by @jakzal. The suggested EntryPoint implementation added to my firewall fixed the problem.

security:
        api:
            anonymous: true
            entry_point: "system.api_entry"
<?php

use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Security\Core\Exception\AuthenticationException;
use Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface;

class ApiEntryPoint implements AuthenticationEntryPointInterface
{
    public function start(Request $request, AuthenticationException $authException = null)
    {
        return new JsonResponse([
            'error' => JsonResponse::HTTP_FORBIDDEN,
            'message' => $authException ? $authException->getMessage() : 'Access Denied'
        ], JsonResponse::HTTP_FORBIDDEN);
    }
}

@patrickallaert
Copy link
Contributor

Any update on this?

@goetas
Copy link
Contributor

goetas commented Jun 21, 2017

@maarekj solution works well :)
@merk solution works well :)

@maarekj
Copy link
Contributor

maarekj commented Jun 21, 2017

@goetas Sorry, but which solution ?

@goetas
Copy link
Contributor

goetas commented Jun 21, 2017

@maarekj sorry typo... i meant @merk

@vpassapera
Copy link

I'm still seeing this issue in 3.3 using LexikJWTAuthenticationBundle

@dmaicher
Copy link
Contributor

dmaicher commented Aug 21, 2017

@weaverryan @chalasr @stof as you are mergers of the security component: What do you think?

@jakzal proposed some options here: #8467 (comment)

One more idea would be to throw an AccessDeniedHttpException here instead of re-throwing the AuthenticationException:

https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Security/Http/Firewall/ExceptionListener.php#L183

I could work on a patch if we agree on a solution 😊

@Th3Mouk
Copy link
Contributor

Th3Mouk commented Nov 11, 2017

I had the same issue on 3.4.0-BETA3
I resolved it putting the backend before the public firewall:

Pattern always must be restrictive at the start, and be degressive

    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false

        backend:
            anonymous: ~
            pattern:    ^/admin
            provider: admin

            form_login:
                login_path: admin_login
                check_path: admin_login
                default_target_path: admin_dashboard

        public:
            anonymous: ~
            pattern:    ^/
            provider: customer

    access_control:
        - { path: ^/admin/login, roles: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/admin, roles: ROLE_ADMIN }

@Rebolon
Copy link

Rebolon commented Jan 11, 2018

I get the same kind of error with Symfony4 + Flex (this is a proof of concept project)
A route protected by a firewall and grant access return a 500 AccessDeniedException instead of a 403.

A sample route that return the 500 :
http://localhost:85/demo/login/json/isloggedin

Here is my security.yaml

security:
    encoders:
        # @todo should use password encoding, more info here: https://symfony.com/doc/current/security.html#c-encoding-the-user-s-password
        Symfony\Component\Security\Core\User\User: plaintext
    # https://symfony.com/doc/current/book/security.html#where-do-users-come-from-user-providers
    providers:
        in_memory:
            memory:
                users:
                    test:
                        password:           test
                        roles:              ROLE_USER
                    admin:
                        password:           admin
                        roles:              [ROLE_USER, ROLE_ADMIN]

    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        vuejs:
            pattern: ^/(demo/(vuejs|form|login/(json|json/isloggedin|json/logout))|api)
            anonymous: ~
            json_login:
                check_path: /demo/login/json
                # this doesn't work, see in the security.yaml:12 for more explanation
                #check_path: demo_login_json_check
            logout:
                path:   demo_login_json_logout
                target: index
                invalidate_session: true
        main:
            pattern: ^/demo/login/(secured|standard|authenticate|logout)
            anonymous: ~

            # activate different ways to authenticate

            # http_basic: ~
            # https://symfony.com/doc/current/book/security.html#a-configuring-how-your-users-will-authenticate

            # form_login: ~
            # https://symfony.com/doc/current/cookbook/security/form_login_setup.html

            form_login:
                default_target_path: demo_secured_page
                login_path: demo_login_standard
                check_path: demo_login_standard_check
                # field names for the username and password fields
                username_parameter: login_username
                password_parameter: login_password

                # csrf token options
                csrf_parameter:       "%csrf_token_parameter%"
                csrf_token_generator: security.csrf.token_manager
                csrf_token_id:        "%csrf_token_id%"

            logout:
                path:   demo_login_standard_logout
                target: index
                invalidate_session: true

Here is the action controller:

    /**
     * Try to test this security when the one on the bottom works Security("is_granted('IS_AUTHENTICATED_FULLY')")
     *
     * call it with .json extension and check if you have a 200
     *
     * @todo: should we let it as is, or always return a 200 and in the Json content set the isLoggedIn to 0 or 1 ?
     * For instance i stay on my first choice
     *
     * @Security("is_granted('IS_AUTHENTICATED_FULLY')")
     * @Route(
     *     "/demo/login/json/isloggedin",
     *     name="demo_secured_page_is_logged_in",
     *     )
     * @Method({"GET"})
     */
    public function isLoggedIn() {
        // will be usefull if we decide to return always 200 + the real Json content represented by isLoggedIn: 0|1
        $authenticated = $this->isGranted('IS_AUTHENTICATED_FULLY');

        return new JsonResponse(['isLoggedIn' => (int)$authenticated, ]);
    }

And finally the composer.json:

{
    "type": "project",
    "license": "proprietary",
    "require": {
        "php": "^7.0",
        "ext-curl": "*",
        "ext-pdo_sqlite": "*",
        "doctrine/doctrine-migrations-bundle": "^1.3",
        "javiereguiluz/easyadmin-bundle": "^1.17",
        "php-http/httplug-pack": "^1.0",
        "rebolon/api-pack": "^1.0.2",
        "sensio/framework-extra-bundle": "^5.1.0",
        "symfony/console": "^4.0.0",
        "symfony/framework-bundle": "^4.0.0",
        "symfony/monolog-bundle": "^3.1.0",
        "symfony/profiler-pack": "^1.0",
        "symfony/yaml": "^4.0.0",
        "webonyx/graphql-php": "^0.11.5"
    },
    "require-dev": {
        "doctrine/doctrine-fixtures-bundle": "^3.0",
        "symfony/dotenv": "^4.0.0",
        "symfony/flex": "^1.0",
        "symfony/webpack-encore-pack": "^1.0"
    },
    "minimum-stability": "dev",
    "prefer-stable": true,
    "repositories": [
        {
            "type": "vcs",
            "url":  "git@github.com:Rebolon/api-pack.git"
        }
    ],
    "config": {
        "preferred-install": {
            "*": "dist"
        },
        "sort-packages": true
    },
    "autoload": {
        "psr-4": {
            "App\\": "src/"
        }
    },
    "autoload-dev": {
        "psr-4": {
            "App\\Tests\\": "tests/"
        }
    },
    "scripts": {
        "auto-scripts": {
            "cache:clear": "symfony-cmd",
            "assets:install --symlink --relative %PUBLIC_DIR%": "symfony-cmd"
        },
        "post-install-cmd": [
            "@auto-scripts"
        ],
        "post-update-cmd": [
            "@auto-scripts"
        ]
    },
    "conflict": {
        "symfony/symfony": "*",
        "symfony/twig-bundle": "<3.3",
        "symfony/debug": "<3.3"
    },
    "extra": {
        "symfony": {
            "id": "01BXPDHBAP11MNR97CR7E97D3F",
            "allow-contrib": false
        }
    }
}

And maybe the way i installed the app:

composer create-project symfony/skeleton sf-flex-encore-vuejs
cd sf-flex-encore-vuejs
composer req encore annotations twig api http profiler log doctrine-migrations admin webonyx/graphql-php
composer require --dev doctrine/doctrine-fixtures-bundle

Here is the project uri: https://github.com/Rebolon/php-sf-flex-webpack-encore-vuejs

@Rebolon
Copy link

Rebolon commented Jan 15, 2018

when i look at the Security component documentation https://symfony.com/doc/master/bundles/SensioFrameworkExtraBundle/annotations/security.html

I can see that we have to specify the status_code if we want an HTTP Exception instead of an AccessDeniedException

For my configuration it seems that the json_login is the explanation of the problem because this is its own behavior (why ? i don't know the code always throw this exception, not an HttpException)

@devpetrov
Copy link

If this would help.

I'm encountering this issue with the following configuration:

providers:
    myProvider:
        id: myProviderService

firewalls:
    dev:
        pattern: ^/(_(profiler|wdt)|css|images|js)/
        security: false

    main:
        anonymous: ~

    application_main:
        pattern: ^/
        provider: myProvider:
        anonymous: ~
        myAuthenticator:
	login_path: login_route
        logout:
            path: /logout
            target: index
            invalidate_session: false
            handlers: [myLogoutHandler]

The problem is the presence main firewall and may be the way it is configured. When I remove it and leave only dev and application_main everything works as normal.

@xabbuh
Copy link
Member

xabbuh commented Feb 2, 2018

@chaos-drone your main firewall catches all requests that do not match the pattern of the dev firewall. Thus the application_main firewall is never reached.

fabpot added a commit that referenced this issue Oct 17, 2018
…on with 401 status code (vincentchalamon)

This PR was merged into the 2.8 branch.

Discussion
----------

Convert InsufficientAuthenticationException to HttpException with 401 status code

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed ticket | #8467
| License       | MIT

I was trying to implement the `json_login` authentication and test it with an API Platform project. When I call a secured endpoint without authentication, an InsufficientAuthenticationException is thrown with a 500 status code instead of a 401.

After some researches with @dunglas, there is no default `entrypoint` on the security firewall. As one already exists for `form_login` in the FormLoginFactory, this component might need a default one to convert this 500 exception to a correct 401 HTTP error.

This fixes #25806 (comment).

Commits
-------

4503ac8 Convert InsufficientAuthenticationException to HttpException
@Koc
Copy link
Contributor Author

Koc commented Oct 17, 2018

I hope #28801 fixes this issue

@Koc Koc closed this as completed Oct 17, 2018
@Rebolon
Copy link

Rebolon commented Oct 18, 2018

@Koc your fix is only for 2.8 or also for 3.4+ and 4+ ?

@dmaicher
Copy link
Contributor

@Rebolon it will be merged up in the other branches 😉

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