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

WebAuthn not allowing security key registration on Totara #422

Closed
jamie-catalyst opened this issue Jul 26, 2023 · 6 comments
Closed

WebAuthn not allowing security key registration on Totara #422

jamie-catalyst opened this issue Jul 26, 2023 · 6 comments

Comments

@jamie-catalyst
Copy link

Affected branch: MOODLE_35_STABLE

When attempting to set up a security key on a Totara 16 we came across an issue with setting up security keys. The Register security key button doesn't function and the following error appears in the JS console:

require.js:65 TypeError: amd.init is not a function
    at action.php:1859:59
    at Object.execCb (require.js:117:193)
    at Module.check (require.js:63:341)
    at Module.<anonymous> (require.js:80:35)
    at require.js:16:51
    at require.js:82:75
    at each (require.js:8:77)
    at Module.emit (require.js:82:39)
    at Module.check (require.js:68:83)
    at Module.enable (require.js:81:280)

The problem is that Totara doesn't support the new esm modules that got introduced. In addition Totara and older Moodles can no longer build the JS files.

I highlighted these commits in the MOODLE_35_STABLE as being problematic:

factor_webauthn: Move JS into modules (5e28ce0670e87b9da811f062a885aafe14319c1f)
factor_webauthn: Copy andrewnicols JS fixes (8df3be56c65ca3b776e2f200636d3589f06568e2)
factor_webauthn: Disable submit button on registration page (3abf8e505183b31c6cc5a14097a431552f28101f)

When I reverted those commits I was able to register a security key in Totara without issue.

Are these commits something to remove/refactor in MOODLE_35_STABLE to maintain Totara compatibility?

@danmarsden
Copy link
Member

Thanks @jamie-catalyst - pull requests welcome! :-) - I don't think we'd want to revert all of those as they are useful for Moodle-based sites. I'm very tempted to create a TOTARA branch in the repo here though - especially as mfa is planned for inclusion in Moodle 4.3 core and higher and we'll only really be maintaining it for Totara (and older Moodle releases) when 4.3 comes out.

@jamie-catalyst
Copy link
Author

Perfect! I thought we'd need to go down that route. In that case I have refactored the JS to work with Totara now jamie-catalyst@39b9b57

If we get a Totara branch created from MOODLE_35_STABLE then I'll get a PR created with the fix.

@alexmorrisnz
Copy link
Member

@jamie-catalyst are you sure you are on the MOODLE_35_STABLE branch when testing on T16? I'm unable to replicate on the latest MOODLE_35_STABLE branch with T16

The problem is that Totara doesn't support the new esm modules that got introduced

The 35 branch isn't using ES modules, it is still using AMD
See https://github.com/catalyst/moodle-tool_mfa/blob/MOODLE_35_STABLE/factor/webauthn/amd/src/login.js#L25 (AMD) vs https://github.com/catalyst/moodle-tool_mfa/blob/MOODLE_400_STABLE/factor/webauthn/amd/src/login.js#L45 (ESM)

@jamie-catalyst
Copy link
Author

Hey @alexmorrisnz

Ah, interesting. I just retested on a fresh install of Totara 16 with the patches applied, and can see the error when setting up the security key. It's specifically a problem where newer ES functionality than Totara supports. I.e. aysnc/await.

Here's the error I see when setting up the key in the JS console:
mfa-t16-js-error

The buttons don't respond and the user is unable to set anything up.

Is that error not appearing when you go to that page?

Cheers

@alexmorrisnz
Copy link
Member

Ah my mistake sorry, I had cache js disabled so it was pulling from the source files.

+1 on a new Totara branch. Took a look at your refactored JS patch too @jamie-catalyst looks good to go when that branch is made 👍

@danmarsden
Copy link
Member

Thanks - I've created a new TOTARA_12 branch, updated readme files with branch details and cherry-picked Jamie's patch into this new branch.

Would be good if someone got a chance to check #373 and rebase it for this branch too.

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

No branches or pull requests

3 participants