When logging in, if session already exists, the login is attached to the old user/session

[jtsafran]

This seems to have happened sometime between 2016092600 and 2017051503.

Steps to reproduce: In the 2016092600 version of the plugin installed to Moodle 3.1: 1) Log User A into Moodle (GUI or via auth_userkey, both work) 2) Call auth_userkey_request_login_url with username = User B 3) Follow URL returned in step 2 4) You are logged in as User B, as expected

In the 2017051503 version of the plugin installed to Moodle 3.1 OR the 2018050200 version on Moodle 3.5: 1) Log User A into Moodle (GUI or via auth_userkey, both work) 2) Call auth_userkey_request_login_url with username = User B 3) Follow URL returned in step 2 4) You are logged in as User A instead of User B, without the standard prompt you would get if visiting the login page when a user is already logged in within Moodle: "You are already logged in as User A, you need to log out before logging in as different user."

[jtsafran]

The culprit is 3 lines in the public function user_login_userkey():

if (isloggedin()) { $this->redirect($redirecturl); }

Looks to be looking for a login to already exist and if it does, it just redirects to the redirect URL instead of warning or logging in the new user. Confirmed if I remove those, the new user gets logged in, as expected

[dmitriim]

Hi @jtsafran

Thank you for reporting this issue.

It doesn't look like security issue with the plugin. The plugin implements very simple SSO model. Your application needs to manage this scenario and make sure that you logged out users from moodle when they are no longer logged in in your application. That's why I remove your label "SECURITY ISSUE" and will treat this issue as Enhancement. I agree that the plugin could be improved for better managing the described edge case.

Feel free to submit a patch for that. It will be much appreciated.

Cheers. Dmitrii

[dmitriim]

One of the option is a logout webservice which is in TODO list https://github.com/catalyst/moodle-auth_userkey/issues/12

[jtsafran]

Dmitrii,

Without a logout call available (yet, as noted above with TODO https://github.com/catalyst/moodle-auth_userkey/issues/12), or a call to check for an existing session, I can't see a good way to have a 3rd party app could manage this scenario.

I don't think the expected behavior of a login call should be a successful login if you are logging the passed user into another profile. It either needs to fail (or at least warn) or it needs to login the new user, not pass a user into a wrong account.

At the very least, it might make sense to alert folks to this behavior in the documentation of the API call as that is where most 3rd party devs are going to look!

Thanks for taking a look and responding! -Jesse

[dmitriim]

@jtsafran thanks for your comment.

As I said before I agree that the plugin could be improved, but adding extra text to documentation is not a good way to do that.

Feel free to submit a patch to help to improve the plugin. I'm ok with both "logout call back" or "to fail (or at least warn) or it needs to login". To be honest I recon they both need to be in the code.

Anyway if you don't treat logout properly in your application you can have following scenario.

Having User A logs in to you your application and to Moodle at some point. User A logs out from your application, then User B using the same browser just navigates to moodle site URL (without being logged in ti your application). As the moodle session is still alive, the user will be logged in as User A.

Cheers, Dmitrii

[jsanchezieu]

Hi, I've tried to send a little change, this helps to check if exists a previous session and if session userid corresponds to the userid from the loginurl. And working on a kind of logout. How can I send a push request? Thanks.

[lukepass]

Hello, unfortunately this is quite a problem for our application since the users need to be logged out before logging in again. Is there any band-aid solution?

Thanks.

[dmitriim]

@lukepass there is an open pull request to fix this issue https://github.com/catalyst/moodle-auth_userkey/pull/39 However it needs some more work until we can integrate it. Please feel free to pick it up and finish it.

[dmitriim]

That should be fixed now as part of https://github.com/catalyst/moodle-auth_userkey/pull/46