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

Inability to update a user's roles after login #12025

Closed
andyvenus opened this issue Sep 24, 2014 · 43 comments

Comments

@andyvenus
Copy link

@andyvenus andyvenus commented Sep 24, 2014

I'm implementing the security component in my app and was surprised to find that a user's roles are decided at login time and cannot be easily updated after that. While the chosen UserInterface object can be refreshed on each load to check for changes (like the password changing or any other details), there is no way to check to see if the user's roles have changed since login. My roles are coming from the UserInterface getRoles method.

This means if I have an admin user for example and I remove their admin abilities, they remain an admin until they decide to log out. Or moving a user to a "banned" role would have no effect.

Ideally, a UsernamePasswordToken or any other token that contains a UserInterface object would check the UserInterface object's getRoles method to see if the roles have changed. But even just a setRoles method on AbstractToken would be useful so that the option is there to keep user's roles fresh.

Of course I may be missing something that allows you to make sure a user's roles are kept fresh so I'd love to hear about it :)

@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Sep 25, 2014

If your roles can dynamically change, you can create a listener that (somehow decides to) reload the token in your security component. You can implement this in your refresh user as well. If there's a solution for this, build-in, I would like to find out too. When your roles change, your token should change.

@stof

This comment has been minimized.

Copy link
Member

@stof stof commented Sep 25, 2014

@fabpot should we take roles into account in AbstractToken::hasUserChanged to force re-authenticating when roles are changed ?

@stof stof added the Security label Sep 25, 2014
@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Sep 25, 2014

@stof I don't think that's desired in every situation. We have the same case but we don't want to have the user login again, that's why manually re-authenticate the user in a listener. Is it possible to add an event that's fired every time the AbstractToken::hasUserChanged()? People can hook on to that and manually determine what they want to have done. For example: #11320 would really help in this case.

@origaminal

This comment has been minimized.

Copy link
Contributor

@origaminal origaminal commented Aug 20, 2015

@fabpot The problem is still actual.
http://stackoverflow.com/questions/31986324/symfony2-authentication-roles-not-updating-in-twig-template

+1 to @stof 's idea of adding to AbstractToken::hasUserChanged a roles comparison: getRoles is part on UserInterface and it looks logically.

@Kobzol

This comment has been minimized.

Copy link

@Kobzol Kobzol commented May 1, 2016

Is there any way to reload the roles dynamically in Symfony 3.x?
I load them from DB and they can change dynamically.
They change from an outside source, so I can't simply reload the user after the change, because I don't know when it happens.
I tried using both EquatableInterfaceand the always_authenticate_before_granting setting, but neither of them worked, when I used them, no roles were loaded at all (I load them using a User repository).

@troymccabe

This comment has been minimized.

Copy link
Contributor

@troymccabe troymccabe commented May 17, 2016

@Kobzol RE: "Is there any way to reload the roles dynamically in Symfony 3.x?" I think so?

Here's how I went about it & it seems to be working, though maybe @stof or someone more involved would be able to shed some light on how much of a nasty bastardization it may be.

You're able to register an subscriber listening to the KernelEvents::CONTROLLER event (I tried KernelEvents::REQUEST, though that seemed a bit early in the bootstrapping process--$container->get('security.token_storage')->getToken() was null.)

Your event subscriber should be injected with (at a minimum) @security.token_storage, though injecting things like Doctrine to load roles would be beneficial as well.

With this, the @Security annotation from extra bundle evaluates has_role properly after calling setToken, and the roles available in templates and such down the request pipeline are the ones set in the YourRoleListener.

Long story short--you're able to modify the stored token before your controller action is executed (& annotations are evaluated) with an event subscriber.

So, in terms of code, here's what an abstract version of what we've got:

services.yml

    app.security.role_change_listener:
        class: YourRoleListener
        tags:
            - { name: kernel.event_subscriber }
        arguments:
            - '@doctrine.orm.default_entity_manager'
            - '@security.token_storage'

YourRoleListener.php

class YourRoleListener implements EventSubscriberInterface
{
    /**
     * @var EntityManager
     */
    protected $entityManager;

    /**
     * @var TokenStorage
     */
    protected $tokenStorage;

    /**
     * YourRoleListener constructor
     *
     * @param EntityManager $entityManager The entity manager to use to load roles
     * @param TokenStorage $tokenStorage The token storage for the user
     */
    public function __construct(EntityManager $entityManager, TokenStorage $tokenStorage)
    {
        $this->entityManager = $entityManager;
        $this->tokenStorage = $tokenStorage;
    }

    /**
     * Handles the kernel event
     *
     * @param FilterControllerEvent $event The dispatched event
     */
    public function onKernelController(FilterControllerEvent $event)
    {
        if ($this->tokenStorage && $this->tokenStorage->getToken()) {
            $token = $this->tokenStorage->getToken();
            $user = $token->getUser();
            // This check can be just `is_object` like in symfony core
            // we're explicit about the class used
            if ($user instanceof YourUserClass) {
                // there didn't seem to be an easier way to grab the provider key, 
                // so using bound closure to retrieve it
                $providerKeyGetter = function(TokenInterface $token) {
                    return $token->providerKey;
                };
                $boundProviderKeyGetter = Closure::bind($providerKeyGetter, null, $token);

                // check & load roles for user here if necessary

                $this->tokenStorage->setToken(
                    new UsernamePasswordToken(
                        $user,
                        $token->getCredentials(),
                        $boundProviderKeyGetter($token),
                        // loaded roles array goes here
                    )
                );
            }
        }
    }

    /** {@inheritdoc} */
    public static function getSubscribedEvents()
    {
        return [KernelEvents::CONTROLLER => ['onKernelController', 99]];
    }
}
@Kobzol

This comment has been minimized.

Copy link

@Kobzol Kobzol commented May 17, 2016

I did something like that, but it is indeed a big hack and I didn't like it. It also caused some issues with authentication and other parts of the app (it also didn't show the right roles in the Symfony debug toolbar, but that's not that important).
After a advice received elsewhere I solved it in a different way, which is way simpler and works so far, maybe it will be useful to others.
You can create your own voter and extract the roles to be checked not from the token, but from the user object (and reload the user's roles on each request in his repository or somewhere else).
services.yml:

login.user_voter:
        class: AppBundle\Security\UserVoter
        tags:
            - { name: security.voter, priority: 245 } # priority is important to override the default
        public: false         # small performance boost

and the UserVoter looks like this:

class UserVoter extends RoleVoter
{
    protected function extractRoles(TokenInterface $token)
    {
        $user = $token->getUser();
        return $user instanceof UserInterface ? array_map(function(string $role)
        {
            return new Role($role);
        }, $user->getRoles()) : [];
    }
}

I had to create a simple Role class that implements the Symfony\Component\Security\Core\Role\RoleInterface and returns the stored role string in the getRole method. This was required because the authentication system expected that I return Role objects (not strings) from the extractRoles method.

With this the roles are extracted from the user and thus are dynamically refreshed on every request. So far it seems that it works and doesn't interfere with the authentication system in any way.

@troymccabe

This comment has been minimized.

Copy link
Contributor

@troymccabe troymccabe commented May 17, 2016

Ah, nice! Good to have another approach--thanks!

@stof -- would the custom voter be the preferred approach, or is there some officially sanctioned way to achieve this?

@weaverryan

This comment has been minimized.

Copy link
Member

@weaverryan weaverryan commented Feb 17, 2017

For the most part, I think the voter approach will work (but definitely test to make sure I'm not missing some edge case). So for now, I'd say this is the way to go. Make sure you haven't changed the access_decision_manager.strategy configuration in security (i.e. make sure this configuration does not appear in your security.yml) - that's important for this to work.

You will still have a few weird issues, however. First, role_hierarchy probably won't work. And also, your web debug toolbar will still probably show the old roles.

@vudaltsov

This comment has been minimized.

Copy link
Contributor

@vudaltsov vudaltsov commented Mar 3, 2017

I used a different approach. First, I made Token think that User changes when roles change:

class User implements \Serializable, \Symfony\Component\Security\Core\User\EquatableInterface
{
    // ...

    public function serialize()
    {
        return serialize([
            // ...
            $this->roles,
        ]);
    }

    public function unserialize($serialized)
    {
        list (
            // ...
            $this->roles
            ) = unserialize($serialized);
    }

    public function isEqualTo(UserInterface $user)
    {
        // ...

        if (array_diff($this->getRoles(), $user->getRoles())) {
            return false;
        }

        return true;
    }
}

I also have the remember_me functionality enabled.

When I change the user's roles, he get's reauthenticated with the RememberMeToken and new roles instead of being logged out.

@ptrm04

This comment has been minimized.

Copy link

@ptrm04 ptrm04 commented Mar 5, 2017

@andyvenus one way you can trigger role reload is to implement the EquatableInterface and do a custom check for example:

  • Extend the User class with an updatedAt property.
  • Once you change user roles, update the updatedAt value at that time (you'd need to include updatedAt in the serialize / unserialize call)
  • Please note that writing your own isEqualTo (EquatableInterface) function will need to cover for any checks that typically would be covered by out of the box when / if you are using AdvancedUserInterface / UserInterface (those can be viewed here https://github.com/symfony/symfony/blob/3.2/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php#L244-L287 so you'd need to take of it yourself (please let me know if you need a snippet). On L250 you can see that if your user is an instance of EquatableInterface it will relay any checks to it.

For all this to work, you'd need to use access_control in security.yml or use is_granted call if using @Security annotation as has_role won't trigger a role reload as described here: #21861

If you follow all this any sessions that a user might have will be updated with the new roles.

Alternatively you can serialize the roles and compare them as in the comment above.


If you want to ban a user, you could consider disabling their account (so that isEnabled returns false) and this will actually log the user out in any session they might have as I've described here long time ago: #17023 and #13870

Changing username / password or any other serialized properties will not log the user out (contrary to what the docs state: http://symfony.com/doc/current/security/entity_provider.html#security-serialize-equatable "For example, if the username on the 2 User objects doesn't match for some reason, then the user will be logged out for security reasons." ). There is a potential fix for this kindly made by @xaben here: #19033 but given many people already might rely on the exisiting functionality hopefully this can be made parameterized / optional in terms of which fields actually do trigger a logout.

@ptrm04

This comment has been minimized.

Copy link

@ptrm04 ptrm04 commented Mar 5, 2017

@vudaltsov what if you didn't have the remember_me functionality enabled? Would it actually log you out completely when you change the roles?

Or let's say if you were to not implement isEqualTo and just change username / password (whatever is serialized as "standard" as per the docs. Does it log you out completely? There is an issue for this at #17023 as this won't log you out, only disabling the account will log the user out completely i.e. changing the isEnabled to false.

@mahono

This comment has been minimized.

Copy link

@mahono mahono commented Mar 24, 2017

It should really be part of AbstractToken::hasUserChanged – anything else seems like a hack to me.

@timwsuqld

This comment has been minimized.

Copy link

@timwsuqld timwsuqld commented Jul 25, 2017

We really need a "hasUserChanged" and "doesUserNeedReloading". Using hasUserChanged for changes that should invalidate an entire session (and potentially case a logout when not using remember_me), and doesUserNeedReloading that allows modifying the user in the security token, without having to reauthenticate (remember_me).

@weaverryan

This comment has been minimized.

Copy link
Member

@weaverryan weaverryan commented Sep 26, 2017

PR open to address this issue: #24331

@gjuric

This comment has been minimized.

Copy link
Contributor

@gjuric gjuric commented Jun 18, 2018

Can someone point me into the right direction to get a better understanding of the issue. I would like to help out. What exactly changed between v3.3 and v3.4 that caused this?

In my case, Symfony 3.3 and previous versions always call the UserAuthenticationProvider::authenticate() method that would create a new UsernamePasswordToken with the roles loaded from the User object that was fetched from the database.

With 3.4 this is not executed unless I turn on security.always_authenticate_before_granting but even then it is being called for all IS_GRANTED calls but not before the Firewall denies access because the User still has the roles from the old UsernamePasswordToken that was loaded from the session.

@RealJTG

This comment has been minimized.

Copy link

@RealJTG RealJTG commented Jul 17, 2018

I have the same issue: my UserProvider currently retrieves a fresh user on each request, but RoleVoter seems to do its checks against "old" roles list obtained from token.

@vudaltsov solution not working for me.
First: it's not safe to use array_diff here.

    public function isEqualTo(UserInterface $user)
    {
        // ...

        if (array_diff($this->getRoles(), $user->getRoles())) {
            return false;
        }

        return true;
    }

because:

$a = [1,2,3];
$b = [1,2,3,4];
var_dump(array_diff($a, $b));
> array(0) {
> }
var_dump(array_diff($b, $a));
> array(1) {
>   [3] => int(4)
> }

Second: when I added a proper check to isEqualTo, after changing roles on the fly I'm getting back to authenticateToken in "simple_form" authenticator

    public function authenticateToken(TokenInterface $token, UserProviderInterface $userProvider, $providerKey)
    {
        try {
            $user = $userProvider->loadUserByUsername($token->getUsername());
        } catch (UsernameNotFoundException $exception) {            
            throw new CustomUserMessageAuthenticationException('Invalid login or password');
        }

        $isPasswordValid = $this->encoder->isPasswordValid($user, $token->getCredentials());

        if ($isPasswordValid) {
            return new UsernamePasswordToken(
                $user,
                $user->getPassword(),
                $providerKey,
                $user->getRoles()
            );
        }

        throw new CustomUserMessageAuthenticationException('Invalid login or password');
    }

but this time $token->getCredentials() returns NULL and leads to endless login loop :\

@Kobzol solution seems to work, however:

  • you either have to change the default prefix from "ROLE_" to something other (so that default RoleVoter - which still do its checks against token - won't vote and grant access), or set access_decision_manager.strategy to unanimous
  • this don't refresh token and you will still see already revoked roles in Symfony Profiler until logout
@RealJTG

This comment has been minimized.

Copy link

@RealJTG RealJTG commented Aug 9, 2018

Upd, so finally it works:

  • Your User class must implement EquatableInterface and isEqualTo must treat users with non-equal roles as different users
  • UserProvider's refreshUser method must implement some logic to return $this->loadUserByUsername() under some conditions (elapsed time, requests count, flag etc.) or return cached User to prevent high database I/O caused by user refresh on each request
  • security.firewalls.[...].logout_on_user_change must be set to false (would be a problem in 4.0 though)
  • security.erase_credentials must be set to false (in order to re-authenticate user transparently - this was my problem)
  • security.always_authenticate_before_granting should be set to false (to prevent user refresh on each request)
@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Aug 9, 2018

The easiest solution would be to leave the identity alone until the next login. There doesn't seem to be a nice solution to reload roles, but that's okay in my opinion. Roles are part of your identity and if your identity changes, you should either be logged out or not updated til your next login. This ensures the visitor is going through the proper identification (authentication) process when it changes and possibly gets blocked by a user checker (or not).

@RealJTG that won't work in 4.0 indeed and sounds like a step in the wrong direction.

@ptrm04

This comment has been minimized.

Copy link

@ptrm04 ptrm04 commented Oct 3, 2018

@iltar agree roles are part of identity but from the UX point of view it is not so okay to reload roles by logging out/back in. Let's say you're logged in and on your free plan, you upgrade to premium, then it logs you out and asks you to log back in because you've just upgraded.

@Grafikart

This comment has been minimized.

Copy link

@Grafikart Grafikart commented Mar 5, 2019

I would argue against the "roles are part of identity". If the administrator chooses to disable an account or restrict it while the user is logged in, I have no way to detect it.
I'm forced to refresh the token on every page using the guardHandler.

$this->guardHandler->authenticateUserAndHandleSuccess(
  $user,
  $request,
  $this->authenticator,
  'main'
);

I don't get why user infos are refreshed but not the roles on every request.

@gjuric

This comment has been minimized.

Copy link
Contributor

@gjuric gjuric commented Mar 5, 2019

This does not make sense to me as well. I don't see a reason why this was changed in v3.4 and not sure how other people are handling this. A few of us can't be the only ones with this issue.

It looks to me like the only sane solution at the moment (since it has been over 4 years since this issue was filed) is to not rely on Symfony roles, firewall or security component but to roll your own.

@ptrm04

This comment has been minimized.

Copy link

@ptrm04 ptrm04 commented Mar 6, 2019

Security used to be taken somewhat lightly in symfony, it used to do the complete opposite from what was in the docs (symfony/symfony-docs#5419 it used to say it will log you out but it didn't etc., now that section has been removed so try for yourself how it behaves in each scenario), then you've got the almost 5 year old roles issues and recently the symfony-checker's security.sensiolabs.org host was decommissioned with very little comms - afaik just twitter - because who isn't always on twitter ;-)

I'd say use with caution, don't roll your own (do people still do that?). Test, test and test (you're doing automated tests, right?)

Other components including Guard are superb (however Guard still needs to deal with the legacy issues, Ryan has done a tremendous job to make things better with Guard).

@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Mar 6, 2019

Let's say you're logged in and on your free plan, you upgrade to premium, then it logs you out and asks you to log back in because you've just upgraded.

@ptrm04 Being part of a premium plan of sorts, sounds more like authorization than authentication. This is most likely to be handled by voters, not authentication.

I would argue against the "roles are part of identity". If the administrator chooses to disable an account or restrict it while the user is logged in, I have no way to detect it.

@Grafikart Make it a property in your UserInterface implementation, as soon as the user is blocked, you can trigger a logout via the EquatableInterface. Why do you think they are not part of the identity?

I don't get why user infos are refreshed but not the roles on every request.

Because they are part of the identity, the user info is not, it just tags along because you most likely use an entity instead of a DTO as UserInterface implementation. I have an idea to make some changes to the security component to solve this issue, to only store a unique identifier in the token and make it easy to (anywhere) fetch the domain object (entity) based on this.

This does not make sense to me as well. I don't see a reason why this was changed in v3.4 and not sure how other people are handling this. A few of us can't be the only ones with this issue.

@gjuric Previously the "logout when a user changes" was broken, it didn't work. This was changed (read fixed) in 3.4, unless it's not this part you're talking about.

and recently the symfony-checker's security.sensiolabs.org host was decommissioned with very little comms - afaik just twitter - because who isn't always on twitter ;-)

I've never used the service so I can't say what the problem was in communications, but it has nothing to do with the security component in Symfony.

Other components including Guard are superb (however Guard still needs to deal with the legacy issues, Ryan has done a tremendous job to make things better with Guard).

Guard runs on the security component, you can't run it without. We're currently discussing the Security Component behavior, not the guard authenticators, they actually have very little to do with this.

@gjuric

This comment has been minimized.

Copy link
Contributor

@gjuric gjuric commented Mar 6, 2019

Thank you for taking the time to explan this.

@ptrm04 Being part of a premium plan of sorts, sounds more like authorization than authentication. This is most likely to be handled by voters, not authentication.

To me it sounds like roles should be a part of authorization as well. They are being used by the voters after all. I don't see a reason on why wouldn't the UsernamePasswordToken be updated on every request as it was done before. The Equatable interface (or the data that gets serialized/deserialized) could be still used to trigger a logout if something changes, but roles don't have to be a part of that check if one doesn't want them to be.

This would allow the app to dynamically update roles as they change without triggering a logout if one is not needed.

@Grafikart Make it a property in your UserInterface implementation, as soon as the user is blocked, you can trigger a logout via the EquatableInterface. Why do you think they are not part of the identity?

What if you want to refresh the current state without triggering a logout?

Because they are part of the identity, the user info is not, it just tags along because you most likely use an entity instead of a DTO as UserInterface implementation. I have an idea to make some changes to the security component to solve this issue, to only store a unique identifier in the token and make it easy to (anywhere) fetch the domain object (entity) based on this.

Please make it fetch roles as well :) This would solve the issue.

@gjuric Previously the "logout when a user changes" was broken, it didn't work. This was changed (read fixed) in 3.4, unless it's not this part you're talking about.

IMHO, using the Equatable interface to fix it would have been enough.

@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Mar 6, 2019

To me it sounds like roles should be a part of authorization as well.

They shouldn't, they help identify to which groups you belong. They can be used in authorization, the same way as a username or IP can be used.

What if you want to refresh the current state without triggering a logout?

I don't see a reason on why wouldn't the UsernamePasswordToken be updated on every request as it was done before.

If a token changes (token = state), it has to be re-authenticated

Please make it fetch roles as well :) This would solve the issue.

Roles are stored inside the token, so that wouldn't change anything. If you want to have roles in your token, you have to somehow get them in there. Changing the token requires a re-authentication

IMHO, using the Equatable interface to fix it would have been enough.

That's what was broken and is fixed, nothing more, nothing less.


My suggestion is to not use roles if you want permissions to change. Use the authorization system for this. You can solve all of these issues by writing voters: https://stovepipe.systems/post/symfony-security-roles-vs-voters

@gjuric

This comment has been minimized.

Copy link
Contributor

@gjuric gjuric commented Mar 6, 2019

If a token changes (token = state), it has to be re-authenticated

Creating a new UsernamePasswordTokes is re-authentication AFAIK.

Roles are stored inside the token, so that wouldn't change anything. If you want to have roles in your token, you have to somehow get them in there. Changing the token requires a re-authentication

I don't mind that happening.

IMHO, using the Equatable interface to fix it would have been enough.

That's what was broken and is fixed, nothing more, nothing less.

Not true. You could have generated a new UsernamePasswordToken with the data provided by the UserProvider if Equatable says that they are not the same.

My suggestion is to not use roles if you want permissions to change. Use the authorization system for this. You can solve all of these issues by writing voters: https://stovepipe.systems/post/symfony-security-roles-vs-voters

That doesn't work in the firewall since it happens early in the request. Actually, the firewall blocking of stuff based on the role that was stored in the session is the only issue.

The other issue being that I am working on a 7 year old app :) We do use voters for other stuff, but firewall is causing issues for us.

@quazardous

This comment has been minimized.

Copy link

@quazardous quazardous commented Mar 7, 2019

What I understand is that for now Symfony Security has a binary approach: you can be white (identity is OK) or black (identity is not OK). And the response is you can stay or you are kicked out. What we (?) need is some gray area : identity has changed but meh you can stay in (authentication stays true) ! But symfony should trigger again post authentication security stuff (roles, voters, etc).

Equatable could be associated with some SecurityUserStatusSomethingService that could expose 2 methods hasChanged() and isStillSafe()

If user hasChanged() and not isStillSafe() you go out.

But if isStillSafe() the Symfony security stack should be ran again against the new identity.

EDIT: since it's working with always_remember_me: true another approach could be to create a special oneshot RememberMeToken with the correct RememberMeAuthenticationProvider

@arturslogins

This comment has been minimized.

Copy link

@arturslogins arturslogins commented Mar 12, 2019

What I understand is that for now Symfony Security has a binary approach: you can be white (identity is OK) or black (identity is not OK). And the response is you can stay or you are kicked out. What we (?) need is some gray area : identity has changed but meh you can stay in (authentication stays true) ! But symfony should trigger again post authentication security stuff (roles, voters, etc).

Equatable could be associated with some SecurityUserStatusSomethingService that could expose 2 methods hasChanged() and isStillSafe()

If user hasChanged() and not isStillSafe() you go out.

But if isStillSafe() the Symfony security stack should be ran again against the new identity.

EDIT: since it's working with always_remember_me: true another approach could be to create a special oneshot RememberMeToken with the correct RememberMeAuthenticationProvider

Is there any solution for this post?

@quazardous

This comment has been minimized.

Copy link

@quazardous quazardous commented Mar 12, 2019

@arturslogins I've no solution if it's what you mean. but as said it's working if you allow always_remember_me: true. But I find it not very satifaying becuse you have to rely on a very optimistic feature...

@arturslogins

This comment has been minimized.

Copy link

@arturslogins arturslogins commented Mar 12, 2019

@arturslogins I've no solution if it's what you mean. but as said it's working if you allow always_remember_me: true. But I find it not very satifaying becuse you have to rely on a very optimistic feature...

For me, i'm searching solution for multiple user profiles for one single symfony app (Session)
User Story for that case:
I log in with my app account and in header i have select field where i can choose use/user profile

When i change profile, i would like to update user session with selected user data object.
But when i update session user data doesn't change :/

@ptrm04

This comment has been minimized.

Copy link

@ptrm04 ptrm04 commented Mar 30, 2019

Let's say you're logged in and on your free plan, you upgrade to premium, then it logs you out and asks you to log back in because you've just upgraded.

@ptrm04 Being part of a premium plan of sorts, sounds more like authorization than authentication. This is most likely to be handled by voters, not authentication.

@linaori what I meant there is that the behaviour of being logged out and asked to log back in after upgrading to premium is wrong - they should be part of authorization but as the roles are not reloaded hence the need for re-authentication. Guess if you injected a repository in a voter to check the live data that would work, but still in an ideal world (?) shouldn't need to do that on each request post-upgrade to "premium".

@gjuric

This comment has been minimized.

Copy link
Contributor

@gjuric gjuric commented Apr 1, 2019

Injecting a repository in the voter would work, but not with the firewall.

In the end, we managed to solve our issue by implementing Guard and extending the AbstractFormLoginAuthenticator.

I still think this could be an easy fix in Symfony, after checking if the stored Token is equal to the freshly obtained User information setRoles() should be called on the old token.

Authentication did not change (it is still the same user) and authorization should be handled from the freshly obtained user information. After all, that is why we have the Equatable interface, to be able to handle ourselves if the user should be logged out or not.

Although this was probably made to fix some other issues what we have now is pretty broken. The object you get when calling ->getUser() could differ from the properties used in the token for firewall and other stuff.

I could make a pull request for this in a week or so (it's a simple change), but not sure if it would be accepted.

@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Apr 1, 2019

I could make a pull request for this in a week or so (it's a simple change), but not sure if it would be accepted.

Next weekend we have a hackathon and we will be having a brainstorm session related to the UserInterface in Symfony and the Security token, so it might be a conflict on this part.

My point of view remains though, the security token should be immutable, thus no roles change. If you need to have authorization, use voters, which query if the user can do something. Don't rely on the identity for this.

@gjuric

This comment has been minimized.

Copy link
Contributor

@gjuric gjuric commented Apr 1, 2019

How does one implement something like that with the Symfony firewall?

@ptrm04

This comment has been minimized.

Copy link

@ptrm04 ptrm04 commented Apr 1, 2019

Injecting a repository in the voter would work, but not with the firewall.

@gjuric what's the problem with firewalls? They don't support voters?

@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Apr 1, 2019

You don't implement it in the firewall, you implement it in a voter so you can call isGranted (one way or another). The firewall is for authentication, not authorization

@ptrm04

This comment has been minimized.

Copy link

@ptrm04 ptrm04 commented Apr 1, 2019

Are voters supported in access_control in security.yaml?

@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Apr 1, 2019

Yes, it's confusing as it's called "roles", they are actually the "attributes" that are passed to voters.

@ptrm04

This comment has been minimized.

Copy link

@ptrm04 ptrm04 commented Apr 1, 2019

Thanks @linaori - I've searched for this in the docs but no mention of access_control (https://symfony.com/doc/current/security/voters.html, https://symfony.com/doc/current/best_practices/security.html#security-voters)
So you can have something like:

    access_control:
        - { path: '^/post/edit/[0-9]+', roles: CAN_EDIT_POST }
@linaori

This comment has been minimized.

Copy link
Contributor

@linaori linaori commented Apr 1, 2019

Correct! I've proposed to change it to "attributes" before, but isn't much clearer. Perhaps we should name it "is_granted" instead?

@vudaltsov

This comment has been minimized.

Copy link
Contributor

@vudaltsov vudaltsov commented Apr 2, 2019

requires_granted?

@curry684

This comment has been minimized.

Copy link
Contributor

@curry684 curry684 commented Apr 6, 2019

We are centralizing discussions on authentication at #30914. As in the proposed changes the Roles would no longer be an integral part of the UserInterface it would be easier to handle situations like this, and this issue itself is no longer a structural problem. As fixing it in 4.x would be a potentially breaking change in authorization I don't think we need to keep this issue open separately.

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