open-lms-open-source / moodle-theme_snap

GNU General Public License v3.0
79 stars 75 forks source link

SAML2 vs Snap theme coding error thrown on redirect #201

Closed MattV50 closed 3 years ago

MattV50 commented 3 years ago

With SAML2 and Snap installed and enabled, authentication occurs but a redirect from Active Directory Federated Services to any other page except the 'Home' page results in a coding error.

In a 3.8 version, with debugging on, the redirect from Federated Services to Home results in,

"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" (there is more information listing errors on line numbers and filenames. Please let me know if you need these)

...but otherwise does not affect the loading of the page for the user (when debug is turned off).

Otherwise a redirect to any other page will stop the user with the message,

"Coding error detected, it must be fixed by a programmer: The theme has already been set up for this page ready for output. Therefore, you can no longer change the theme, or anything that might affect what the current theme is, for example, the course. More information about this error"

Debug gives the additional information,

Debug info: Stack trace when the theme was set up:

Error code: codingerror

Stack trace:

line 1912 of /lib/pagelib.php: coding_exception thrown line 1011 of /lib/pagelib.php: call to moodle_page->ensure_theme_not_set() line 2819 of /lib/moodlelib.php: call to moodle_page->set_course() line 58 of /course/view.php: call to require_login()

Output buffer:

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 1542 of /lib/pagelib.php: call to moodle_page->magic_get_context()
  • line 204 of /theme/snap/config.php: call to moodle_page->initialise_theme_and_output()
  • line 2239 of /lib/outputlib.php: call to include()
  • line 679 of /lib/outputlib.php: call to theme_config::find_theme_config()
  • line 7440 of /lib/moodlelib.php: call to theme_config::load()
  • line 702 of /lib/classes/user.php: call to get_list_of_themes()
  • line 863 of /lib/classes/user.php: call to core_user::fill_properties_cache()
  • line 832 of /lib/classes/user.php: call to core_user::get_property_type()
  • line 158 of /user/lib.php: call to core_user::clean_field()
  • line 767 of /auth/saml2/auth.php: call to user_update_user()
  • line 563 of /auth/saml2/auth.php: call to auth_plugin_saml2->update_user_profile_fields()
  • line 339 of /auth/saml2/auth.php: call to auth_plugin_saml2->saml_login()
  • line 321 of /auth/saml2/auth.php: call to auth_plugin_saml2->loginpage_hook()
  • line 2731 of /lib/moodlelib.php: call to auth_plugin_saml2->pre_loginpage_hook()
  • line 58 of /course/view.php: call to require_login()

This is not an issue once cookies have been set.

Thank you for any help with this issue.

dvdcastro commented 3 years ago

Hi @MattV50, thanks for reporting.

We are aware of this issue, the problem happens because we have some things in Snap that we need to initialize within the theme and not leave to core in config.php, especially these lines:

    if (empty($CFG->snappageinit) && !empty($PAGE)) {
        $CFG->snappageinit = true;
        $PAGE->initialise_theme_and_output();

        // Modify $PAGE to use snap requirements manager.
        $snappman = new snap_page_requirements_manager();
        $snappman->copy_page_requirements();
    }

So, we had to patch Saml2 so it doesn't fail with Snap:

Index: auth.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/auth.php b/auth.php
--- a/auth.php  (revision c1cd9379f36d486603337ace74eb079d010a0a24)
+++ b/auth.php  (revision 2b55e91c179428e27bee379af31d534ffeb64c3c)
@@ -661,7 +661,11 @@
                 // If set to true - don't update based on data from this call.
                 unset($user->description);
             }
+            // Snap is initializing the theme on the config file and causing a conflict.
+            // This is to avoid the config file call the initializing function.
+            $CFG->snappageinit = true;
             user_update_user($user, false, false);
+            $CFG->snappageinit = false;
             // Save custom profile fields.
             profile_save_data($user);
         }

We haven't gotten around to avoid making calls to \moodle_page::initialise_theme_and_output when initializing Snap, so we suggest you add this patch to the auth_saml2 plugin.

MattV50 commented 3 years ago

Thanks, @dvdcastro I have included the patch and can report that it works fine. Thank you for your assistance.