catalyst / moodle-local_cohortauto

Automatically add users into cohorts. (Previously moodle-auth_mcae.)
https://moodle.org/plugins/local_cohortauto
11 stars 14 forks source link

"$PAGE->context was not set" when performing OAuth2 login #11

Closed lukecarr closed 3 years ago

lukecarr commented 4 years ago

After installing this plugin, I'm getting an error when I try to login with an OAuth2 account. I'm not sure if this plugin is causing the error, but it is referenced in the error output:

Coding problem: $PAGE->context was not set. You may have forgotten to call require_login() or $PAGE->set_context(). The page may not display correctly as a result
line 503 of /lib/pagelib.php: call to debugging()
line 830 of /lib/pagelib.php: call to moodle_page->magic_get_context()
line 1456 of /lib/weblib.php: call to moodle_page->__get()
line 214 of /local/cohortauto/lib.php: call to format_string()
line 60 of /local/cohortauto/classes/observer.php: call to local_cohortauto_handler->user_profile_hook()
line ? of unknownfile: call to local_cohortauto_observer::user_updated()
line 155 of /lib/classes/event/manager.php: call to call_user_func()
line 75 of /lib/classes/event/manager.php: call to core\event\manager::process_buffers()
line 834 of /lib/classes/event/base.php: call to core\event\manager::dispatch()
line 209 of /user/lib.php: call to core\event\base->trigger()
line 352 of /auth/oauth2/classes/auth.php: call to user_update_user()
line 480 of /auth/oauth2/classes/auth.php: call to auth_oauth2\auth->update_user()
line 49 of /auth/oauth2/login.php: call to auth_oauth2\auth->complete_login()

I get this error message immediately after logging in, however, I can click 'Continue' and access the site normally.

The URL of the page this error appears on is: /auth/oauth2/login.php?wantsurl=...&sesskey=...&id=...&oauth2code=... (I've removed the actual values of the query string)

danmarsden commented 4 years ago

Thanks @lukecarr - I've got a site using oauth that is about to use this plugin pretty soon - feel free to submit a PR otherwise I'm likely to need to look at this at some point soon.

lukecarr commented 4 years ago

@danmarsden I'm not too well versed in Moodle plugin development, so I wouldn't know where to start!

Do you think you'd be able to have a fix ready by October 9th? I've got a Moodle site currently succumbing to this error that I'd like to push to production on that date.

DrCuriosity commented 4 years ago

Looking at this now. It appears that the issue might be to do with format_string() requiring a context that it is not able to pick up automatically when an event is triggered during OAuth2 negotiation.

Unfortunately I'm not able to replicate this error on the 3.9 site I have available to me (Version 3.9.2+ (Build: 20200929) (2020061502.03))

I think we could maybe fix this by explicitly passing the system context through to the format_string() calls that are initiated by the event observer. E.g. in the user_profile_hook() function, changing the line:

$cohortslist[$cohort->id] = format_string($cohort->name);;

to:

$cohortslist[$cohort->id] = format_string($cohort->name, true, array('context' => $context));

This should pick up the $context = context_system::instance(); earlier in the function.

@lukecarr, are you in a position to make this change locally, and see if it helps on your site?

lukecarr commented 4 years ago

Thanks for the reply @DrCuriosity. Unfortunately, after implementing your proposed fix locally, the issue still persists. The error message hasn't changed at all. I can still click 'Continue' to log in successfully, the same as before.

DrCuriosity commented 4 years ago

Curses. Same stack trace and everything?

Unfortunately I still haven't been able to replicate the issue locally. I'll see if I can find another approach to try on (UTC+13) Monday.

DrCuriosity commented 4 years ago

No luck, I'm afraid. Even testing the event trigger via a CLI script, it seems to work fine here.

The only thing I can think of is that somewhere along the line, the format_string() function is trying to do something else that relies on an implicit context being set outside of the explicit system context we're giving it, when the event is triggered via OAuth2. If this is the case, it might be best to try the "dumb" context-free fallback option of using strip_tags(), as per this snippet from weblib.php:

    if (!$options['context']) {
        // We did not find any context? weird.
        return $string = strip_tags($string);
    }

i.e.:

$cohortslist[$cohort->id] = strip_tags($cohort->name);

It's not an elegant fix but it might be worth trying out, if only to see if the problem goes away or not.

lukecarr commented 3 years ago

Just chiming in to mention that I believe this issue was actually a result of me having set Moodle's message debugging level to DEVELOPER. Turning that off, I no longer had the errors, and sign in functionality worked as intended, leading me to believe that the "errors" I had were mere warnings.