8000 @Security("has_role('ROLE_ADMIN')") "delayed start" when reloading roles? · Issue #21861 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

@Security("has_role('ROLE_ADMIN')") "delayed start" when reloading roles? #21861

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.

Sign up for GitHub

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

Jump to bottom
Closed
ThePeterMick opened this issue Mar 4, 2017 · 9 comments
Closed

@Security("has_role('ROLE_ADMIN')") "delayed start" when reloading roles? #21861

ThePeterMick opened this issue Mar 4, 2017 · 9 comments
Labels
Bug Security Status: Reviewed

Comments

@ThePeterMick
Copy link
ThePeterMick commented Mar 4, 2017 •
edited
Loading
Q A
Bug report? maybe
Feature request? no
BC Break report? no
RFC? yes
Symfony version 2.8.17

Let's say you have an admin page that lists users.

On the controller you have @Security("has_role('ROLE_ADMIN')") and you load the page while logged in as admin user with the role, all nice.

Behind the scenes, remove the association of ROLE_ADMIN to the user (many-to-many table) in the database and in order to force roles to be reloaded let's say change the username from i.e. admin (or whatever you logged in with) to admin1 (or whatever property in order to trigger the hasUserChanged detection and as a result trigger role reload)

Reload the page, roles will get reloaded and it will still let you access the page (!) even though you don't have ROLE_ADMIN anymore. Refresh the page again and it will only now become effective. So you have to refresh twice hence giving you this "last shot" at access (this could be both GET/POST requests).

Now, when you use @Security("is_granted('ROLE_ADMIN')") or control via security.yml the change is effective immediately and access is denied - without this "one last shot" as has_role offers.

Any ideas?
@xabbuh
Copy link
Member
xabbuh commented Mar 4, 2017

Can you fork the Symfony Standard Edition and make the changes that are necessary to reproduce your issue?

By the way, the @Security annotation is part of the SensioFrameworkExtraBundle. So your issue is probably to be fixed there. However, we can leave this issue open until we know for sure that this is no issue in the Symfony core.

@ThePeterMick
Copy link
Author

@xabbuh I have illustrated how you can reproduce the has_role "misbehaving" on the symfony-demo fork at: https://github.com/ptrm04/symfony-demo

If you want to just use the standard symfony-demo then please do the changes I've made in these two commits:

  1. Login to the backend as anna_admin.
  2. Access the post list page (http://127.0.0.1:8000/en/admin/post/).
  3. Edit the SQLLite DB (lives in %sf_app_home%/var/data/blog.sqlite).
  4. Change anna_admin to anna_admin1 (or whatever)
  5. Change her role from ROLE_ADMIN to ROLE_ADMIN1
  6. Make sure you click "Write Changes" to the SQL Lite file if using "DB Browser for SQLite"
  7. Reload the post list page. It'll let you access it. Inspect your current roles, it'll say ROLE_ADMIN1
  8. Reload the post list page, it won't let you access it.
  • Thanks to the is_grantedthe roles are getting reloaded while has_role still looking at the old ones. This may be because is_granted call takes place in the template and controller runs with has_role annotation before that takes place.
  • If you were to remove the template calls to is_granted I made in commit at ThePeterMick/symfony-demo@6f672de then has_role will never ever trigger a role reload
  • Conclusion: has_role will never trigger role reload and therefore will always look at the "old" roles.

To play with this from scratch and see how has_role is not triggering a role reload same as is_granted does:

  1. Get a fresh symfony-demo master and kill access_control line as in ThePeterMick/symfony-demo@1aeb745 as we want to be purely looking at has_role.
  2. Login to the backend as anna_admin.
  3. Access the post list page (http://127.0.0.1:8000/en/admin/post/).
  4. Edit the SQLLite DB (lives in %sf_app_home%/var/data/blog.sqlite).
  5. Change anna_admin to anna_admin1 (or whatever)
  6. Change her role from ROLE_ADMIN to ROLE_ADMIN1
  7. Make sure you click "Write Changes" to the SQL Lite file if using "DB Browser for SQLite"
  8. Reload the post list page.
  9. You will notice that the Authenticated line in debugger will say No and the role will stay as it was (that's because there is no is_granted call anywhere)
  10. At this point it'll let you access the page no matter how many times you reload the page, it wouldn't have let you access it anymore if the roles were reloaded.

2 ways to stop giving you access at this point:

  1. Incorporate a call to is_granted in any of the twig templates that get loaded when requesting http://127.0.0.1:8000/en/admin/post/ (same as I did in my fork)

OR

  1. Go to http://127.0.0.1:8000/en/blog/ - this one uses a template in which is_granted is used for the menu and will reload the roles.
  2. Go back to http://127.0.0.1:8000/en/admin/post/ and has_role will deny you access.

@xabbuh
Copy link
Member
xabbuh commented Mar 5, 2017

Thanks for providing the example. I looked into it and found two things:

  1. The User class from the Symfony Demo is not considered to have changed when roles are edited. I am not completely sure if that should be done inside the User entity (by implementing the EquatableInterface) or inside the hasUserChanged() method of the AbstractToken class (I don't know if there was a good reason only to compare the other attributes of the UserInterface and ignore the roles there.

  2. The built-in AuthorizationChecker (which is used when is_granted() called in a Twig template) reauthenticates the current token when it isn't authenticated and thereby triggering a reload of the token's roles. This does not happen inside the SecurityListener from the SensioFrameworkExtraBundle. Thus, the listener will only see the roles that were present in the token during the previous request.

@xabbuh
Copy link
Member
xabbuh commented Mar 5, 2017

@fabpot @stof @HeahDude Maybe you can share your thoughts here because of your insights in the Security component and SensioFrameworkExtraBundle?

@HeahDude
Copy link
Contributor
HeahDude commented Mar 5, 2017

Isn't it related to #12025?

@xabbuh
Copy link
Member
xabbuh commented Mar 5, 2017

@HeahDude Partly, the first part of my comment basically sums up what is described in #12025. However, that's something you could work around by letting your custom user class implement the EquatableInterface. However, the fact that the SecurityListener from SensioFrameworkExtraBundle doesn't trigger authentication of the token (like the authorization checker does) still stands which means that the ExpressionLanguage doesn't act on the updated roles.

@ThePeterMick ThePeterMick mentioned this issue Mar 5, 2017
Closed
@ThePeterMick
Copy link
Author

Thanks for looking into this @xabbuh

Based on your findings:

  1. Just to re-iterate my observation as described above, the roles will get reloaded if you additionally modify one of the serialized properties i.e. username and trigger is_granted call or require a role using access_control in security.yml so this does not check if the roles themselves has changed and instead the roles are reloaded anyway when any of the props change. I guess one could implement their own isEqualTo and do whatever checks they want there and as long as they don't use has_role and instead use access_control / is_granted the roles will get reloaded.

This is issue is purely to do with has_role not reloading the roles. #12025 seems to cover what you've described where a simple solution of checking if roles have changed in isEqualTo or utilising updatedAt property (if changed it will trigger a role reload) would do.

  1. If you call is_granted as part of the @Security annotation it will have the same effect so not bound directly to a call from within Twig template (it is in the example though indeed).

@xabbuh
Copy link
Member
xabbuh commented Mar 6, 2017 •
edited
Loading

The is_granted() inside the @Security annotation uses the authorization checker internally like the Twig function. That's why you don't see a difference there.

@xabbuh xabbuh mentioned this issue Nov 13, 2018
Closed
@xabbuh
Copy link
Member
xabbuh commented Nov 14, 2018

When has_role() is used as part of the ExpressionVoter, it's already called inside the authorization checker. So this is no issue there. And the SensioFrameworkExtraBundle now ships with an @IsGranted annotation that should be used instead of the @Security annotation. Thus, I am closing here.

@xabbuh xabbuh closed this as completed Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Security Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@xabbuh @ThePeterMick @HeahDude @carsonbot
0