Skip to content

[BC Break] Change of logout path access control behavior #28058

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

Closed
xelan opened this issue Jul 25, 2018 · 8 comments
Closed

[BC Break] Change of logout path access control behavior #28058

xelan opened this issue Jul 25, 2018 · 8 comments

Comments

@xelan
Copy link
Contributor

xelan commented Jul 25, 2018

Symfony version(s) affected: Reproduced with 2.7.47 (LTS) and 3.4.10 (LTS). Probably other versions where PR #24805 was merged are affected as well.

Description
Apparently, access control behavior for the logout path was changed in bugfix releases of Symfony, I suppose this is caused by #24805.

In Symfony 2.7.46, the logout path was not considered by the access_control rules defined in security.yml. Starting from 2.7.47 it is, so an "Access Denied" exception is thrown if the logout path is not allowed.

Also see hslavich/OneloginSamlBundle#53

How to reproduce

This example reproduces this issue with a minimal Symfony 2.7 application created with the Symfony installer.

git clone https://github.com/xelan/symfony-logout-break.git
cd symfony-logout-break
composer install
php app/console server:start 0.0.0.0:8000

If you change composer.json to "symfony/symfony": "2.7.46", run composer update and test again, the last step (logout) is successful.

Possible Solution
IMHO, two approaches would be possible:

  • Revert the access control behavior for the logout path (Retaining BC)
  • Consider it a fixed security flaw (as logout path was not considered by the access_control configuration before), but clearly document the change in the upgrade guides for all supported versions.

Additional context
app/config/security.yml:

security:
    encoders:
        Symfony\Component\Security\Core\User\User: plaintext

    providers:
        in_memory:
            memory:
                users:
                    user:  { password: pass, roles: [ 'ROLE_USER' ] }
    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false

        login:
            pattern:  ^/login$
            security: false

        secured_area:
            pattern:        ^/
            form_login:
                check_path: example_login_check
                login_path: example_login
                require_previous_session: false
            logout:
                path:       example_logout
                target:     /
            anonymous:      ~

    access_control:
        - { path: ^/$, roles: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/test$, roles: ROLE_USER }
        - { path: ^/, roles: ROLE_DUMMY_NO_ACCESS }
@stof
Copy link
Member

stof commented Jul 25, 2018

hmm, #24805 made the logout listener the last one. But the access listener is also special (it is not about authenticating, but about authorizing), so I think this one should have been kept after the logout one.

@nicolas-grekas
Copy link
Member

ping @MatTheCat maybe

@MatTheCat
Copy link
Contributor

Was it intentional to not take access control into account on logout? Seems like a bug to me.

@chalasr
Copy link
Member

chalasr commented Jul 30, 2018

I'm divided, we must specify an access rule for any login path so the original behavior for logout sounds inconsistent, but I'm not sure it deserves such a behavior change.
I'm up for a PR that restores the original behavior by making the access listener last, the same way we did for the logout one.

@xabbuh
Copy link
Member

xabbuh commented Aug 1, 2018

The current behaviour is a bug to me. What would be the point in not allowing the user to log out (and that's the only affect you could achieve by defining access control rules for this path, right?)? We cannot prevent the user from logging out in most cases anyway (they can delete cookies for example).

@chalasr
Copy link
Member

chalasr commented Aug 1, 2018

I'm on it.

@MatTheCat
Copy link
Contributor

@xabbuh the point would be to do what is actually expected. Everyone must ensure their login form is under a public firewall but it makes no sense authenticating someone on a login form. That's just the way it works and I don't see why the access control on logout would be an exception.

@chalasr
Copy link
Member

chalasr commented Aug 1, 2018

See #28100

chalasr pushed a commit that referenced this issue Aug 11, 2018
This PR was merged into the 2.8 branch.

Discussion
----------

[Security] Call AccessListener after LogoutListener

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28058
| License       | MIT
| Doc PR        | n/a

Commits
-------

44dbea6 [Security] Call AccessListener after LogoutListener
@chalasr chalasr closed this as completed Aug 11, 2018
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

6 participants