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

Proposals to make the anonymous security config simpler #27610

Closed
javiereguiluz opened this Issue Jun 15, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@javiereguiluz
Copy link
Member

javiereguiluz commented Jun 15, 2018

Description
After having delivered some introductory Symfony workshops lately, I'd like to propose some changes to make anonymous-related security config easier to explain/understand.

Example
My first petition is a DX-related change. The IS_AUTHENTICATED_ANONYMOUSLY "role" is always strange for newcomers. It's not a role, but we use it in the roles option ... and its name is too long and easy to mistype:

Before:

security:
    access_control:
        - { path: "^/something/login", roles: IS_AUTHENTICATED_ANONYMOUSLY }

After:

security:
    access_control:
        - { path: "^/something/login", anonymous: true }

By the way, the Spring security system we took inspiration from, use allow to use ROLE_ANONYMOUS in addition to IS_AUTHENTICATED_ANONYMOUSLY. So maybe this can be enough:

security:
    access_control:
        - { path: "^/something/login", roles: ROLE_ANONYMOUS }

My second petition is about the anonymous setting in the firewall. It's always confusing to understand ... and it's not documented in the Symfony Security Config Reference.

As soon as some access_control URL allows to access anonymous users, you must set it to anonymous: ~. What's the point of not adding it? Users won't be able to access to the login form and nothing will work. Can we remove this option or make it smarter and auto-detect when you need to allow anonymous users for some URLs?

Thanks!

@alsar

This comment has been minimized.

Copy link

alsar commented Jun 15, 2018

In my opinion the IS_AUTHENTICATED_ANONYMOUSLY "role" should be removed from access_control. As you pointed out is not a role, but is treated like a role.
The anonymous: true approach is better as it expresses the attention to allow unauthenticated access.

@stof

This comment has been minimized.

Copy link
Member

stof commented Jun 15, 2018

there is already another way to configure this, thanks to the support for expression language: allow_if: 'true'. Do we want to add one more ?

@webnet-fr

This comment has been minimized.

Copy link
Contributor

webnet-fr commented Jun 15, 2018

IS_AUTHENTICATED_ANONYMOUSLY is not the only attribute you can require, there are also
IS_AUTHENTICATED_REMEMBERED, and IS_AUTHENTICATED_FULLY. Take a look
https://github.com/symfony/security/blob/master/Core/Authorization/Voter/AuthenticatedVoter.php#L28-L30. So we will have to add two more keys to access_control: remembered and fully which will encumber authorization rules.
These attributes could be started with 'ROLE_' prefix but IMHO they are not roles.

@webnet-fr

This comment has been minimized.

Copy link
Contributor

webnet-fr commented Jun 15, 2018

As for anonymous under firewall definition it could be naturally false. For example, consider http_basic authentification : there is no form so there is no page to expose to anonymous users.

@stof

This comment has been minimized.

Copy link
Member

stof commented Jun 15, 2018

So we will have to add two more keys to access_control: remembered and fully which will encumber authorization rules.
These attributes could be started with ROLE_ prefix but IMHO they are not roles.

roles in access_control is actually a bad naming. This config does not expect roles, but permission attributes (anything you can pass to isGranted).
Roles can be used a permission attribute (thanks to the RoleVoter supporting them), but lots of other things can as well. Using roles for the config here is very unfortunate.

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Jun 15, 2018

You can also add more IS_AUTHENTICATED_ rules if you over-write the authentication trust resolver: https://github.com/scheb/two-factor-bundle/blob/036666cfe7dd3716b28be964310cc1177fab62d5/Security/Authentication/AuthenticationTrustResolver.php#L9

I have pointed out a similar issue in: #23139

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

javiereguiluz commented Sep 21, 2018

Closing because this proposal didn't gain enough traction and the proposed solution is not much better than the current situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.