10000 New Symfony Guard doesn't support user/token refresh/reload · Issue #23660 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

New Symfony Guard doesn't support user/token refresh/reload #23660

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
timwsuqld opened this issue Jul 25, 2017 · 8 comments
Closed

New Symfony Guard doesn't support user/token refresh/reload #23660

timwsuqld opened this issue Jul 25, 2017 · 8 comments
Labels
Bug Security Status: Needs Review

Comments

@timwsuqld
Copy link
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.3

The new Guard system doesn't support refreshing users when they change (i.e. reloading roles), instead forcing the user to be logged out when something changes.

authenticate function in GuardAuthenticationProvider.php, specifically detects we have a PostAuthenticationGuardToken, and throws an AuthenticationExpiredException. This doesn't allow us to update parts of the user, that should normally just cause the user to be refreshed in the security token (if I understand the code correctly).
Previously, when using Authenticators and not Guard, a change to the user would cause the security token to be refreshed. Now it causes an AuthenticationExpiredException exception which forces a logout.

Not all systems want, or can, use remember_me cookies to allow a logout to then reauthenticate the user when things change. This worked before Guard, so not having it working in Guard is really inconvenient.

Symfony\Component\Security\Core\Exception\AuthenticationExpiredException:

  at vendor/symfony/symfony/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php:86
  at Symfony\Component\Security\Guard\Provider\GuardAuthenticationProvider->authenticate(object(PostAuthenticationGuardToken))
     (vendor/symfony/symfony/src/Symfony/Component/Security/Core/Authentication/AuthenticationProviderManager.php:80)
  at Symfony\Component\Security\Core\Authentication\AuthenticationProviderManager->authenticate(object(PostAuthenticationGuardToken))
     (vendor/symfony/symfony/src/Symfony/Component/Security/Http/Firewall/AccessListener.php:65)
  at Symfony\Component\Security\Http\Firewall\AccessListener->handle(object(GetResponseEvent))
     (vendor/symfony/symfony/src/Symfony/Component/Security/Http/Firewall.php:69)
  at Symfony\Component\Security\Http\Firewall->onKernelRequest(object(GetResponseEvent))
     (vendor/symfony/symfony/src/Symfony/Bundle/SecurityBundle/EventListener/FirewallListener.php:48)
  at Symfony\Bundle\SecurityBundle\EventListener\FirewallListener->onKernelRequest(object(GetResponseEvent))
     (var/cache/dev/appDevDebugProjectContainer.php:1367)
  at appDevDebugProjectContainer->{closure}(object(GetResponseEvent), 'kernel.request', object(TraceableEventDispatcher))
  at call_user_func(object(Closure), object(GetResponseEvent), 'kernel.request', object(TraceableEventDispatcher))
     (vendor/symfony/symfony/src/Symfony/Component/EventDispatcher/Debug/WrappedListener.php:112)
  at Symfony\Component\EventDispatcher\Debug\WrappedListener->__invoke(object(GetResponseEvent), 'kernel.request', object(ContainerAwareEventDispatcher))
  at call_user_func(object(WrappedListener), object(GetResponseEvent), 'kernel.request', object(ContainerAwareEventDispatcher))
     (vendor/symfony/symfony/src/Symfony/Component/EventDispatcher/EventDispatcher.php:174)
  at Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(array(object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener)), 'kernel.request', object(GetResponseEvent))
     (vendor/symfony/symfony/src/Symfony/Component/EventDispatcher/EventDispatcher.php:43)
  at Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.request', object(GetResponseEvent))
     (vendor/symfony/symfony/src/Symfony/Component/EventDispatcher/Debug/TraceableEventDispatcher.php:146)
  at Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher->dispatch('kernel.request', object(GetResponseEvent))
     (vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php:129)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
     (vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php:68)
  at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
     (vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:171)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (app_dev.php:25)
@mvrhov
Copy link
mvrhov commented Jul 25, 2017

@javiereguiluz Please see #19033

@linaori
Copy link
Contributor
linaori commented Jul 25, 2017

The token being refreshed is afaik, a bug. It shouldn't be refreshed but logged out and Guard is the only one implementing this properly (as @mvrhov linked, my findings are in that ticket).

If you want to update your SecurityObject each request if data changes, don't also use it as security user: https://stovepipe.systems/post/decoupling-your-security-user

@timwsuqld
Copy link
Author

@iltar I understand that you think it's a bug. I also like the idea of a SecurityObject being different to your user entity. However, this doesn't solve the Roles being updated in a users session issue. Updating a users Roles should not require them to log back in. While Voters can be used in some areas, other times simple roles are perfect and work fine, we just need the ability to update the security token when a user's Roles change.
UserInterface specifically has getRoles, so it would need to be part of your SecurityUser object. In your system, how can you update the SecurityUser roles without logging the user out?

Maybe I just need to follow #12025 until a solution is found?

@linaori
Copy link
Contributor
linaori commented Jul 25, 2017

Updating a users Roles should not require them to log back in

How can you verify this without re-authenticating?

we just need the ability to update the security token when a user's Roles change.

That's what the EquatableInterface is for, however, this should trigger re-authentication as it should de-authenticate you. You're mixing User roles and Token roles in this scenario. A user is authenticated with a set of roles. If those roles change and you want to automatically re-authenticate, create a new token and store this in the TokenStorage. Symfony should not do this for you in my opinion, because it might create unwanted security issues in applications.

UserInterface specifically has getRoles, so it would need to be part of your SecurityUser object. In your system, how can you update the SecurityUser roles without logging the user out?

This is actually an issue that comes along from this interface. In fact, the majority of methods in that interface should not be used outside of the authentication process. The only thing you need during the life-cycle of your application, is the identifier. This identifier can be used to retrieve the corresponding entity (if there is any). See #23639 (comment) as of why.

@timwsuqld
Copy link
Author

we just need the ability to update the security token when a user's Roles change.

That's what the EquatableInterface is for, however, this should trigger re-authentication as it should de-authenticate you. You're mixing User roles and Token roles in this scenario. A user is authenticated with a set of roles. If those roles change and you want to automatically re-authenticate, create a new token and store this in the TokenStorage. Symfony should not do this for you in my opinion, because it might create unwanted security issues in applications.

This is starting to make more sense now. isEqualTo (EquatableInterface) is to ensure that if something changes that would invalidate the user, we force a logout. It's NOT for checking if the security token needs to be refreshed.

For Roles refresh, we should be writing custom logic, either at the point the roles change, or with a custom event/listener (i.e. onKernelController) to run custom code at each load, which should then detect if the security user needs to be reloaded.
Ideally, we should also decouple the SecurityUser from our User Entity which will prevent synchronisation issues. This also will remove an extra database call, as EntityUserProvider loads the user entity on each load (AFAIK), and instead we can make that call ourselves when we actually want to verify if the user needs reloading.

What this issue has highlighted for me most, is the misinformation surrounding isEqualTo and making that force a reload of the security token. https://coderwall.com/p/nptn5q/how-to-reload-your-user-after-changes-in-symfony2 actually has it backwards, His first solution of reloadUserPermissions was actually the correct way, and the use of isEqualTo probably works due to a remember_me cookie or similar. (That blog isn't the only place on the net that has this part wrong)

@linaori
Copy link
Contributor
linaori commented Jul 26, 2017

This also will remove an extra database call

In my scenario this is not the case, I will still query the database to query certain information for a refresh. This is primarily to trigger a de-authentication in certain scenarios, for example: is the user blocked or removed? You can simply return the existing user object, but this user object often contains the password (and salt), which I try to not store in the session whenever I can.

What this issue has highlighted for me most, is the misinformation surrounding isEqualTo and making that force a reload of the security token. https://coderwall.com/p/nptn5q/how-to-reload-your-user-after-changes-in-symfony2 actually has it backwards, His first solution of reloadUserPermissions was actually the correct way, and the use of isEqualTo probably works due to a remember_me cookie or similar. (That blog isn't the only place on the net that has this part wrong)

There's a lot of confusion regarding this. Some say it's working like A and consider B a bug, others say it's the other way around, that's why I linked the other post. I'm not sure what the original intention is, but I like the solution done by Guard, as it's explicit and lets developers customize the other scenarios.

@mvrhov
Copy link
mvrhov commented Jul 26, 2017 •
edited
Loading

AFAIR, the Johannes said that he modeled security after the one in Java. so IMO it should be looked there for a behavior.

@chalasr
Copy link
Member
chalasr commented Nov 3, 2017

In #23882 we decided to adopt the behavior described here (guard) no matter which listener is used. Closing here.

@chalasr chalasr closed this as completed Nov 3, 2017
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: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@javiereguiluz @mvrhov @linaori @timwsuqld @chalasr @carsonbot
0