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

Creating a user and immediately logging in does not work #60

Open
amenk opened this issue Oct 28, 2020 · 9 comments
Open

Creating a user and immediately logging in does not work #60

amenk opened this issue Oct 28, 2020 · 9 comments

Comments

@amenk
Copy link

amenk commented Oct 28, 2020

I have auto create on and creating the user is working.

But the direct userkey login isn't.

If I put a delay of 10 seconds between creation an calling the moodle link, it works

Edit:

  • Moodle version is 3.9 (Build: 20200615)
@amenk amenk changed the title Creating a user and directly logging in does not work Creating a user and immediately logging in does not work Oct 28, 2020
@amenk
Copy link
Author

amenk commented Oct 28, 2020

I am getting this error logged

[Wed Oct 28 15:40:32.845628 2020] [proxy_fcgi:error] [pid 266706] [client 127.0.0.1:48322] AH01071: Got error 'PHP message: PHP Warning: session_regenerate_id(): Cannot regenerate session id - session is not active in public/lib/classes/session/manager.php on line 581', referer: https://shop.com.pub.lh/

Not sure if it is a timing problem or something random

@amenk
Copy link
Author

amenk commented Oct 28, 2020

Findings during debugging:

I am deleting the user in mdl_user for testing and then call my external application.
In around 50% of the cases the login then does not work.

I can enforce this problem when I patch the module like this

public function user_login_userkey() {
    global $SESSION, $CFG, $USER;

    require_logout();

I digged that down to this code being called:

    if (isloggedin()) {
        if ($USER->id != $key->userid) {
            // Logout the current user if it's different to one that associated to the valid key.
            require_logout();
        } else {
            // Don't process further if the user is already logged in.
            $this->userkeymanager->delete_keys($key->userid);
            $this->redirect($redirecturl);
        }
    }

so whenever require_logout is called, the session is not properly established afterwards, I believe

amenk added a commit to amenk/moodle-auth_userkey that referenced this issue Oct 28, 2020
We do not call require_logout, in cases where the user is logged in anyways afterwards.
Additionally, in case of an error also we do not destroy the existing session
@dmitriim
Copy link
Member

dmitriim commented Oct 28, 2020

Hi @amenk !
Will you be able to provide clear steps to replicate? From the description it's not 100% clear what the issue is and unfortunately without clear understanding it's quite hard to fix (or check if the provided patch actually fixes the issue).

@amenk
Copy link
Author

amenk commented Oct 29, 2020

Hi @dmitriim ,

unfortunately I cannot really clearly reproduce.

Basically we have a Magento shop which request the token from the userkey endpoint, autocreate is on.
After the token is requested, the Magento shop forwards to the userkey login script with the token. This works many times, but more often than not, we are just not logged in on the Moodle side.

When using debug mode I get the "Cannot regenerate session id - session is not active" error as stated above.

I assume this happens always when require_logout is called before the login. As stated above, for testing purpose I called require_logout in all of the times, and it was 100% reproducible.

I had a hard time tracing it, because with xdebug and step-by-step debugging it most of the time does not appear, so it might be somewhat timing related as well.

In the end of the day I think my fix makes sense, because we don't really have to log out, when we log in anyways.

@dmitriim
Copy link
Member

@amenk

do you use a system web service user for generating a token?

> In the end of the day I think my fix makes sense, because we don't really have to log out, when we log in anyways.

This could be true in your case, but that logic was added to fix a bug when we actually have an active session already and a different user is trying to login using the same browser. So we check if the user is actually different and then logout the current user and them log in a new one. This logic must stay in place.

You may want to spend a little bit more time trying to replicate the issue. Huge benefit would be if you can replicate it using unit tests. So in this case we could be confident when we fix it.

Also would be good know other prerequisites like Moodle version, session storage, the way WS is configured and etc.

@amenk
Copy link
Author

amenk commented Oct 30, 2020

do you use a system web service user for generating a token?

What do you mean with system Webservice?
I added the token generating permission to an existing service, yes. I did not create a new webservice.

Do you think this makes a difference? Why?

I added the moodle version at the top post.

@janbucher
Copy link

This issue still persists, I can reproduce it with a vanilla Moodle 4, and the example script from the plugin.

I think this is mainly a problem for developers: New users will not be logged in, if an existing user is logged in for the same browser session. In this case, the old user is logged out first, but the link expired and therefore login fails.

Therefore, unless you need to have multiple users in the same session, it should not be a problem "in the wild". Still would be good to fix, for robustness...

@dmitriim
Copy link
Member

dmitriim commented Sep 18, 2022

Hi @janbucher

I think now I understand a scenario you are facing.

  1. User tries to login, but there is another active session for another user.
  2. Currently active is forced to loge out.
  3. However, the one that tires to log in never gets logged in as his key already expired/deleted and hence login is broken.

Can you please confirm?

If it's so, then it's 100% a bug as I would expect initial user should be logged in.

@dmitriim dmitriim added the bug label Sep 18, 2022
@janbucher
Copy link

@dmitriim yes, this is the behavior we observe.

There are two unexpected scenarios for the user: If an active session is present, she will be prompted to login, if she reuses the link, it will be expired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants