isImpersonator() method not working properly

KGE commented 6 years ago

\Auth::isImpersonator() return false when impersonating a user ( and you are not already logged in as a frontend user )

Steps to reproduce: ( Best is to do it in incognito mode to be sure we are in a clean state )

Login as a backend user
Impersonate a frontend user
\Auth::isImpersonator() return false

Expected behaviour:

\Auth::isImpersonator() return true after impersonating a frontend user

I think it happens because if you call the \Auth::impersonate method and your are not already logged as a frontend user, the user_auth_impersonate session var will be stored as null so it will not be considered as set and consequently the isImpersonator() will return false.

LukeTowers commented 6 years ago

@KGE feel free to make a PR to fix this :)

KGE commented 6 years ago

The logic I can see here seems to work properly only with a backend user that impersonate another backend user OR a frontend user that impersonate another frontend user.

A backend user that impersonate a frontend user basically works but \Auth::isImpersonator() and \Auth::getImpersonator() methods can't work properly since they store and use the $oldSession searching for one of the same kind of the auth manager we are using ( Backend or Frontend ).

At the moment I can't make a PR since I cant see what was the real idea behind this feature.

@LukeTowers, @daftspunk Feel free to let me know if I am missing something or if I have misunderstood the usage.

manuelbua commented 6 years ago

I can confirm that on my updated local installation (latest stable) the behavior i'm seeing is the same as the one @KGE is describing.

By looking at the actual source code, it's very difficult to grasp the original intent or the reasoning behind the current implementation, but obviously touching such sensitive code without the know-how behind the nitty-gritty of its inner workings could also produce undesired security impacts.

Are you still working on it? Is this feature's implementation supposed to be complete at this point? If you could shed some light on it and its usage then it would be very much appreciated: i'm all for contributing to the project (although this looks like a basic, mandatory functionality), but i'm personally pretty much unable to help since it looks soooo archane :stuck_out_tongue_closed_eyes:

LukeTowers commented 6 years ago

@KGE @manuelbua could you explain what exactly is going on a bit more in depth?

KGE commented 6 years ago

@LukeTowers, I already explained all I can at my best..but I can be more precise on the steps to reproduce

Steps to reproduce:

install october cms
install user plugin and create a frontend user
create a frontend page where check for the \Auth::isImpersonator() value
Login as a backend user
Go the user plugin user list and Impersonate the created user
Go to the previously created page anche check for the \Auth::isImpersonator() value

\Auth::isImpersonator() method should return true but it return false

LukeTowers commented 6 years ago

@KGE are you able to explain why that happens?

KGE commented 6 years ago

I already explained it here and since there is no obvious fix ( But i can see a few possible ), before making a PR I need to have some feedback from you about the idea and limitations about this feature.

LukeTowers commented 6 years ago

If I'm asking you to explain why it happens, then it's because your existing explanation isn't detailed enough to understand what's happening. I can't provide any feedback if I don't know what the problem is.

Please try to explain it again as detailed as possible step by step. Any possible fixes you might be thinking of would be welcome as well.

manuelbua commented 6 years ago

I think it happens because if you call the \Auth::impersonate method and your are not already logged as a frontend user, the user_auth_impersonate session var will be stored as null so it will not be considered as set and consequently the isImpersonator() will return false.

Mmmm i'm not a native speaker but if i'm not too dumb, it looks like the last paragraph of the original issue already tries to describe a possible cause.

@LukeTowers, as project leaders, shouldn't investigating these issues be one of your tasks as well? If you are too busy to handle it, then no problem and let it known, but we really are not asking to solve it for us, we are asking for more information to be able to grasp the reasoning behind the feature itself, explaining the behavior we see.

It looks like the auth manager there tries to consolidate the impersonate feature in the base class, but the real intent behind this choice is not clear at all..

LukeTowers commented 6 years ago

@manuelbua as my time is freely given to this project, and there are significant amounts of requests clamouring for attention across all the various support channels and code repositories my tasks are whatever I choose to grant my limited time on. There is not enough time for every issue, so to improve your chances of success it's important that you provide as much information as possible in the initial requests and follow up so that the amount of back and forth can be minimized.

I have very little time to dedicate to reading issues and processing them, so when I ask for a more detailed explanation it's because the existing one will require too much time to try and think through what the person is saying vs what they mean and what is actually happening.

I would like to see links directly to the lines of code affecting the issue and the thought process behind what's going on exactly, along with (ideally) any potential solutions that the person can think of to resolve the issue.

I did not create the AuthManager, so I have no idea what the original logic behind the impersonate() methods were, but I can tell you that the AuthManager for both backend users and frontend users extends the October\Rain Auth manager base class, which was a purposeful choice since they are both similar systems using similar core code.

KGE commented 6 years ago

An that's why i linked to the October\Rain\Auth\Manager some comments above.

Backend and Frontend auth do not ovveride those impersonate methods.. and since there is a store/restore session flow to preserve the current session auth ( admin_auth or user_auth ) when impersonating, the code assumes you are already logged in as a user of the same kind ( Backend or Frontend ).

If it cant store ( For than restore it when stop impersonating ) a current session auth of the same kind because there isnt one, the isImpersonator() will not work properly since it search for that stored session. ( And the getImpersonator() will not work consequently ).

From what I can see, at this time the isImpersonator() and getImpersonator() methods can work properly only with a Backend user that impersonate another backend user ( Or a frontend user that impersonate another frontend user ) I cant see how they can works when a backend user impersonate a frontend user ( Unless a backend user is logged as frontend user too at the same time )

When impersonating a frontend user we only look for an existing frontend user session to preserve ( And the isImpersonator() search for this stored session ) but if there isnt we should at least check if the impersonator is the logged backend user

LukeTowers commented 6 years ago

@KGE I'm still not seeing how come this wouldn't work as it is right now.

Backend user logs in, session is created
Backend user impersonates frontend user, old session is stored in $sessionKey-impersonate
Auth manager checks if it is currently impersonating by checking for the existence of $sessionKey-impersonate.

What part of that doesn't work? Is the sessionKey property different between the RainLab auth manager and the Backend auth manager?

KGE commented 6 years ago

Yes, they use different session keys ( since you can be logged in as frontend user ( Rainlab auth ) and backend user at the same time ). Both classes inherits from the October\Rain\Auth\Manager but both override the $sessionKey variable

LukeTowers commented 6 years ago

@KGE so what would you suggest as an implementation to properly determine whether or not a particular user is being impersonated?

AugmentBLU commented 6 years ago

I'm having issues with this as well, except I'm using this for backend users only. After calling the impersonate function, the admin_auth and admin_auth_impersonate sessions are the same. So when you come to stopImpersonate or getImpersonator, it's just defaulting back to the same person that is being impersonated rather than the original impersonator/your admin account. Seems broken.

LukeTowers commented 6 years ago

@AugmentBLU are you able to implement a fix in a PR?

yapsr commented 5 years ago

I would suggest the special impersonation session value should also be set if a backend user is impersonating, like:

See https://github.com/octobercms/library/blob/develop/src/Auth/Manager.php

public function impersonate($user)
{
    $impersonation = $this->getImpersonation();
    if (!$this->isActualImpersonation($impersonation)) {
        // User is not logged in as regular user, may be impersonating as a backend user
        $impersonation = false;
    }

    // Login and set new sessionKey
    $this->login($user, false);

    // To indicate user is impersonating, set special key with impersonator data
    $this->setImpersonation($impersonation);
}

public function isActualImpersonation($impersonation)
{
    return is_array($impersonation) && count($impersonation) === 2;
}

public function getImpersonation()
{
    return Session::get($this->getImpersonationSessionKey());
}

public function setImpersonation($impersonation)
{
    return Session::put($this->getImpersonationSessionKey(), $impersonation);
}

public function getImpersonationSessionKey()
{
    return $this->sessionKey . '_impersonate';
}

bennothommo commented 5 years ago

Closing as it has been over a month since any activity on this occurred and we are trying to figure out what issues are still relevant. If this is still something that you would like to see through to fruition please respond and we can get the ball rolling.

yapsr commented 5 years ago

@bennothommo Yes, I still would like this to be fixed.

LukeTowers commented 5 years ago

@yapsr are you able to submit a PR to fix the issue so that we can discuss the fix more there? Please include a summary of the problem and how your PR fixes it.

yapsr commented 5 years ago

@LukeTowers I will try and find some time to do that.

rajakhoury commented 3 years ago

I noticed the same problem mentioned by @KGE

What @yapsr proposed doesn't solve the problem it is actually the same one. Auth::isImpersonator() will also return false because the associated session is invalid / there was not previous session when it was created.

Follwing these steps the problem is in step 2 and I believe is related to impersonate()

You're logged in as a Backend User only and there's no previous frontend auth session or a "user_auth" session key.

// this will return null - doesn't exist yet
$oldSession = Session::get($this->sessionKey);
// this also is false
$oldUser = !empty($oldSession[0]) ? $this->findUserById($oldSession[0]) : false;

then after the Impersonator is logged in $this->login($user, false) the session will be created with an empty value. AuthManager assumes it is the actual user :)

if (!$this->isImpersonator()) {
    Session::put($this->sessionKey.'_impersonate', $oldSession);  // $oldSession = null
}

a workaround could be to set the old session to the current one ?

if (!$this->isImpersonator()) {
      Session::put($this->sessionKey.'_impersonate', $oldSession ?? Session::get($this->sessionKey));
 }

What I don't like here is that you will need to logout twice for example see the Session Component onStopImpersonating()

The user session has 2 parts id + persist code.

Is it bad practise to add the impersonator flag to the same key instead of creating and deleting sessions? also the checks would be easier and features can be more accurate. For example I may have events that fire on Login and Logout but only should run when is not an impersonator and with my current workaround won't work properly.

public function impersonate($user)
{
    $oldSession = Session::get($this->sessionKey);
    $oldUser = !empty($oldSession[0]) ? $this->findUserById($oldSession[0]) : false;

    if($oldUser)
    {
        $user->fireEvent('model.auth.beforeImpersonate', [$oldUser]);
    }

    $this->login($user, false);

    if (!$this->isImpersonator()) {
        Session::put($this->sessionKey.'_impersonate', $oldSession ?? Session::get($this->sessionKey));
    }
}

public function stopImpersonate()
{
    $currentSession = Session::get($this->sessionKey);
    $currentUser = !empty($currentSession[0]) ? $this->findUserById($currentSession[0]) : false;
    $oldSession = Session::pull($this->sessionKey.'_impersonate');
    $oldUser = !empty($oldSession[0]) ? $this->findUserById($oldSession[0]) : false;

    if ($currentUser && $oldUser) {
        $currentUser->fireEvent('model.auth.afterImpersonate', [$oldUser]);
    }

    Session::put($this->sessionKey, $oldSession);
}

bennothommo commented 3 years ago

I've gone over the history of comments here, and I am of the opinion that the functionality is correct at this stage (although, admittedly, a little misleading).

The thing is - the Backend Users and the Front-end Users should be considered completely different authentication "streams", if you will. They don't share sessions, they operate independently. @LukeTowers has mentioned they both extend the core AuthManager class, but that's to share common functionality between the two - not to share sessions. We cannot, and should not, share sessions because this leaves Backend Users more open to exploits from access levels that are supposed to be only available on the Front-end (the reverse is also true, if you are not careful).

I know many people blur the lines between the two, or would like to share authentication between the Front-end Users and Backend Users - I understand this, but it will be a lot easier for management and accountability if you consider them separate. Perhaps a good idea for a plugin would be to provide components like those provided by the User plugin but tailored towards looking at the authenticated Backend user, as opposed to a Front-end user, if there is not a plugin like that already.

This leaves the misleading apsect to it - in that this plugin provides a way to impersonate a user from the Backend. I instead believe it should be changed to say that you "Log in as user" instead. It promotes the correct action here in that you are not impersonating another user, but are instead logging in as the user in the User plugin authentication "stream". The functionality of this plugin should perhaps be changed to suit this concept.

daftspunk commented 3 years ago

This is a platform bug and is fixed in v2.1.5

rainlab / user-plugin

isImpersonator() method not working properly #298