Totara 17.3+ patchless support.

[LiamKearn]

Changes

This patch attempts to watch the newly introduced Totara hooks:

• \core\hook\after_require_login
• \core\hook\after_config

It will forward any calls to them along to the existing lib.php callbacks (tool_mfa_after_config & tool_mfa_after_require_login) that check for authentication.

These hooks were introduced as part of TL-35168. If you are not familar with Totara hooks documentation is available here: https://help.totaralearning.com/display/DEV/Hooks+developer+documentation

This will authenticate users on Totara (17.3+) without the need for core patches.

All changes in this patch are gated with a class_exists check for \totara_core\hook\base to ensure MOODLE compatibility.

---

Three things to consider

1. It seems the plan in the future is for these hooks to automatically search for functions in lib.php as normal to preserve functionality with MOODLE plugins. That future plan will hopefully render this change obsolete and make this plugin work with Totara without any changes.

   The source for that plan is a code comment:
   TODO - PLATFORM-117 to create a watcher to enable those functions via this hook in one of the
   newly introduced hooks.

2. I'm sure the \class_exists() in hooks.php is not needed but it can't hurt to be explicit.

3. With this patch lib.php callbacks could potentially be called before the plugin is installed or enabled. This is different to the MOODLE hook style with get_plugins_with_function which does something like:

   if (!isset($installedplugins[$plugin])) {
       // Plugin code is still present on disk but it is not installed.
       unset($pluginfunctions[$plugintype][$plugin]);
       continue;
   }

   At the moment the tool_mfa\manager::require_auth invoked via the lib.php callbacks handles this in it's call to: \tool_mfa\manager::is_ready. However if anything new was introduced directly to lib.php callbacks that requires installed plugin functionality it may create issues.

   I think it's safe to ignore for now especially since Totara seemingly plans to make this obsolete in the future.

[LiamKearn]

I'm defeated by CI's required parameter documentation in MOODLE 4+. I don't develop enough MOODLE/Totara at the moment to justify setting codesniffs up locally in VIM as I had it in vscode. I'm not entirely sure why it fails with (no function documentation) and only after I add it does it complain about missing parameters (especially when those parameters are typed). IMO CI checks should be declarative not imperative.

It was nice that it picked up on a missing define || die however!

Might come back to this another day but hoping I can be a cheeky boy and leave it to you to fix as maintainers if want to include this 😉

[LiamKearn]

@danmarsden is there any chance you/someone could take a look at this in the next few weeks?

I saw you created an issue around T17 in this plugin recently.

At this point the CI linting failures seem unrelated to files touched in this patch:

[Peterburnett]

@LiamKearn Thanks for the contributions, and the very detailed description. Ill try and squeeze this onto my plate in the next week or two to get it polished off and merged. Cant say Im overly thrilled with the direction totara is taking, but happy to match to it.

[LiamKearn]

@Peterburnett

@LiamKearn Thanks for the contributions, and the very detailed description. Ill try and squeeze this onto my plate in the next week or two to get it polished off and merged. Cant say Im overly thrilled with the direction totara is taking, but happy to match to it.

Yes completely agree, I'm not a fan of hooks, Totara feels like a wheel design shop in some regards. I'd much rather see upstreamed work here.

If you're commenting on the fact that we have to manually watch these hooks as of now, that should change in the future. I think they're doing it as a two step plan:

1. Add the hooks <- We're here now
2. Call the hooks to preserve moodle compat without developers having to watch them

---

@keevan
Thank you for taking the time to review, I've improved everything a bit based on your feedback, let me know if there is anything further.

[LiamKearn]

I intentionally added that typo to test something earlier didn't realise it was commited, glad it was picked up before a merge :)

[LiamKearn]

Hi All

Apologies for bump, any chance someone can review/trigger CI again (I think failures are unrelated).

It would be really nice to no longer have to patch for such a standard security measure.

[danmarsden]

Thanks Liam, now that Moodle is bringing this into their core release we've created a Totara specific branch for Totara support - If you got a chance to rebase and fix the conflict it would be great, otherwise we'll try to get to it ourselves sometime.