openwebwork / webwork2

Course management front end for WeBWorK
http://webwork.maa.org/wiki/Main_Page
Other
143 stars 165 forks source link

Add two factor authentication. #2335

Closed drgrice1 closed 5 months ago

drgrice1 commented 6 months ago

This uses the TOTP (time-based one-time password) protocol. Any authenticator app on a mobile device that supports this protocol can be used (for example, Google Authenticator, Microsoft Authenticator, Twilio Authy, etc.).

Whether this is enabled or not is controlled by new course environment variable $twoFA{enabled}. If that is set to 0, then two factor authentication is disabled for all courses. If that is 1 (the default), then two factor authentication is enabled for all courses that use password authentication (of course two factor authentication does not apply to courses that use external authentication methods like LTI, CAS, Shibboleth, etc.). If that is a string course name like 'admin', then two factor authentication is enabled only for that course. If that is an array of string course names, then two factor authentication is enabled only for those courses listed. This can also be set in a course's course.conf file. Note that only the values of 0 and 1 make sense there.

There are two methods that can be used to setup two factor authentication when a user signs in for the first time. The setup information can be emailed to the user, or can be directly displayed in the browser on the next page that is shown after password verification succeeds.

This is controlled by the new course environment variable $twoFA{email_sender}. If that is set, then the email approach will be used. In this case, after a user signs in and the password is verified, the user will be sent an email containing a QR code and instructions on how to set up a OTP generator app. This is probably a more secure way to set up two factor authentication, as it ensures the user setting it up is the correct user. Note that if a user does not have an email address, then the browser method below will be used as a fallback.

If $twoFA{email_sender} is not set, then after a user signs in and the password is verified, the QR code, OTP link, and instructions will be displayed directly on the page in the browser. This is potentially less secure because a hacker could guess a username and password before a user has setup two factor authentication (particularly if the username and password are initially the same), and then the hacker would gain access to that user's account, and the actual user would be locked out. Note that you will need to use this option if your server can not send emails. Also note that no-reply addresses may be blocked by the email server or marked as spam. So it may be better to find a valid email address to use for this.

This requires a new database column otp_secret that was added to the password table (for lack of a better place to put it).

There is a new wwsh script (bin/reset2fa) that can be used to reset two factor authentication for a user if a user somehow loses their setup in an authentactor app on their mobile device. That just removes the OTP secret from the database. This means that the user will need to go through the two factor authentication setup process again. To use it execute: wwsh courseID /opt/webwork/webwork2/bin/reset2fa userID. Multiple user ids can be listed if you want to reset more than one user at a time. Note that this script is the only way that an admin user can reset their own two factor authentication (and there should never be another way for admin users to do this for themselves). Perhaps a page in the admin course could be added for resetting this for instructors.

A form has been added to the Accounts Manager for resetting two factor authentication for students. This form does not allow the user to reset their own two factor authentication secret, but that of other users at equal or lesser permission level to their own. Note that in the admin course if there are multiple admin users, then one admin user can reset two factor authentication for another. Also there is some clean up and issue fixes in the htdocs/js/UserList/userlist.js file with form validation. The "change" event handler was being added multiple times to the users list table. More clean up is needed though (with this and the other pages with action forms). There is a lot of redundancy with this form validation implementation.

There is an option to skip two factor authentication on trusted devices. A checkbox is on the two factor verification page. If that is checked, then a signed cookie (separate from the session cookie) is set. If that cookie is set, then two factor verification is skipped for sign in attempts.

This builds on #2333 and #2334, and is the final part of the authentication system revamp.

drgrice1 commented 5 months ago

I really don't want to use a command line utility. A perl package would be much better. We are trying to remove dependencies on command line utilities. They are rarely portable.

drgrice1 commented 5 months ago

The GD::Barcode::QRcode module is a currently maintained package (the last release was in September of last year), and should work well. I think the current parameters should work without errors or warnings on all linux distributions. The image is just a bit larger than is desired.

somiaj commented 5 months ago

Tested this out and everything seems to work. Tested with the 'Duo' app since that is what I'm using here. It worked fine to get into the admin course. I had to setup a second account to log into a test course (as expected), and the first time it didn't work, but the second time it did (so unsure if there was an error scanning the QR code or something else, but the second time worked just fine). But outside of having to setup one of my test courses twice, works well.

Out of curiosity I went into the database and manually copied the otp_secret from my admin course to my test course and I was able to use the same authentication account as the admin course to login. This would make it so I don't need to have a different 'account' for each course I log into. Would it be useful to have an option in the admin course when copying users from the admin admin course to also copy their otp_secret? What about allowing admins to copy a users otp_secret from one course to another, so users don't have to keep track of multiple accounts for each course they are in?

somiaj commented 5 months ago

Further tests, I tried to login again, and the code generated by my app was invalid. I tested both the courses I had 2fa setup multiple times with the same result (thought maybe it had to do with my copying of the otp_secret). I was about to delete the otp_secret and start over, and I tried one more time, and the code worked. After that I logged in and out a few more times, and the 2fa code work just fine for both courses.

Could there be some connectivity issue or timing that would make it so sometimes the 2fa code works and some times doesn't? I didn't actually change anything between when it worked and when it didn't.

drgrice1 commented 5 months ago

@somiaj: I have not tested with the 'Duo' app. The fact that you needed to set up a course twice, and that your later logins did not work might have something to do with that. Unfortunately the TOTP protocol is not standardized (or just not everyone follows the standards -- not sure which there). At one point I found a site explaining the different expectations of the more well known authenticator apps (including the three I have been listing and a few others). I know that some of the parameters are not supported by some apps. For example, Microsoft Authenticator only supports the SHA1 algorithm, as I discovered earlier. Supposedly the period option actually means something and can be either 30 or 60 seconds, but none of the authenticator apps that I tested with honored that option.

Note that this has nothing to do with connectivity. There is never any network interaction between the authenticator apps and the webwork2 server. The TOTP protocol is purely algorithmic. Once the authenticator app has the parameters in the otpauth link that is embedded in the QR code, then it can generate OTP codes that in theory will be valid for any given time interval. It could be a timing issue. The WeBWorK::Utils::TOTP module is based on the Authen::TOTP package from cpan. I didn't use that cpan package because it had some clear design issues, and as you can see from the code in the WeBWorK::Utils::TOTP module, the protocol isn't hard to encode.

You could experiment with the parameters (digits, period, algorithm, and tolerance) and see if there are parameters that work better for DUO. Note that tolerance is not an actual OTP parameter, but is a number of periods to check surrounding the current time interval as specified by the period. I have that set to 0. So it only checks precisely the current period. If that is set to 1. then it will check 1 period on both sides of the current period as well for a match.

Copying the otp_secret will work. Note that if you are creating a course and copy an admin user into the course, then the otp_secret will already be copied with it. The same is true if you are copying from another course (by checking to copy non-student users).

drgrice1 commented 5 months ago

I just tested this with DUO. I tried to scan the given QR code to set up the account twice. Both times the codes that DUO was giving failed. I added tolerance => 1 to line 475 of WeBWork::Authen and the codes started working.

drgrice1 commented 5 months ago

I added the tolerance => 1 option to this pull request. So test again, and see if that works better.

somiaj commented 5 months ago

Thanks. I didn't realize this was purely algorithmic, and the tolerance option does sound like why it would sometimes work and sometimes not. So far in my tests this is working just fine.

somiaj commented 5 months ago

Also good to know that copying users will copy their otp_secret, so once a user has setup 2fa, they can continue to use the same app/setup. I overlooked that @Alex-Jordan changes also allowed copying non-student users from another course when creating a new course.

somiaj commented 5 months ago

I had a thought on the Skip two factor verification on this device. option. Some sites will require that 2fa is used every X amount of time instead of having an indefinite skip. What about adding an option for admins to set a time interval which the skip lasts? That way it could be required that 2fa is used once every 7 days, once every 30 days, require 2fa is used every time, or as it currently stands an indefinite time.

drgrice1 commented 5 months ago

This was discussed, and I mentioned that an option could be added. I left it for now, but if you are asking, I suppose I will implement it.

somiaj commented 5 months ago

I missed the discussion, thanks for letting me know. You don't need to add it on my account, I just thought this was a feature some admins may want.

drgrice1 commented 5 months ago

The time interval for which the effect of checking the Skip two factor verification on this device. check box will last is now configurable. The setting is $twoFA{skip_verification_code_interval}. It is set to one year by default.

somiaj commented 5 months ago

Should the skip 2fa checkbox state the time interval? It still states Skip two factor verification on this device..

drgrice1 commented 5 months ago

I'd rather leave it! It will be a bit of a pain to translate the value in seconds into a human readable time interval.

somiaj commented 5 months ago

It will be a bit of a pain to translate the value in seconds into a human readable time interval.

I think there is code for this, what about moving sub format_interval ($c, $seconds) { in LTIUpdate.pm to Utils.pm so it can be used by other locations?

somiaj commented 5 months ago

Though I think that method should be improved too, It does assume that all languages list time in days, hours, minutes, seconds order (I think I tried to simplify it to much). I thought you had a version which was a bit more robust for ProblemSet.pm, but not finding it.

drgrice1 commented 5 months ago

I don't think that method quite covers it. I don't think the message should say something about 365 days (for the default value). The method could be extended to include months or even years, but the complexity of the message if the interval is set to something that isn't a nice even multiple of a day or even of an hour becomes rather intense.

I type to slow. You beat me to it.

somiaj commented 5 months ago

Yea, you have a variation in templates/ContentGenerator/ProblemSet/version_list.html.ep, might be worth standardizing this, but I agree, it will be a pain, so leave it for now is fine.

drgrice1 commented 5 months ago

If this were javascript we could use moment.js for this (we already use that library with flatpickr for the date/time picker). It has a duration.humanize method with support for translations. I bet perl doesn't have something so convenient.

drgrice1 commented 5 months ago

There is the Time::Duration module, but it doesn't offer translation (at least not directly).

drgrice1 commented 5 months ago

Hmm, maybe the Time::Duration::LocaleObject will work?

somiaj commented 5 months ago

That does seem reasonable, but also seems like we would need to install a module for every language we support, as that is just a wrapper for Time::Duration language modules.

drgrice1 commented 5 months ago

Unfortunately, it doesn't look like there are modules for all of the languages that we support. I think I will try to extend what is in version_list.html.ep and move it into a Utils module. I don't really want anything more to go into Utils.pm itself. In fact I have a pull request coming soon that pulls out much of that massive file into Utils submodules. I guess for this pull request I could add it to Utils.pm, and then move it to the already written Utils/DateTime.pm submodule in that pull request.

Alex-Jordan commented 5 months ago

A cautionary tale about measuring time duration in months. One time I had my car's emissions tested on July 1. This is required here before getting your tags renewed. I was told I had "6 months" to actually get the tags before my testing expired. Lots of things came up and I didn't get around to it, but I knew it was July 1 so I had until Jan 1. It got down to the wire and I went in on Dec 30 to get the new tags, but the testing had expired. "6 months" was a lie. It was actually 180 days and I was a day or two late.

On Sat, Mar 16, 2024, 3:17 PM Glenn Rice @.***> wrote:

Unfortunately, it doesn't look like there are modules for all of the languages that we support. I think I will try to extend what is in version_list.html.ep and move it into a Utils module. I don't really want anything more to go into Utils.pm itself. In fact I have a pull request coming soon that pulls out much of that massive file into Utils submodules. I guess for this pull request I could add it to Utils.pm, and then move it to the already written Utils/DateTime.pm submodule in that pull request.

— Reply to this email directly, view it on GitHub https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-cc14702df862a4d3&q=1&e=379d028d-6053-40f5-a7da-c02f0e1eae3d&u=https%3A%2F%2Fgithub.com%2Fopenwebwork%2Fwebwork2%2Fpull%2F2335%23issuecomment-2002157513, or unsubscribe https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-fe6283f86dd88cf3&q=1&e=379d028d-6053-40f5-a7da-c02f0e1eae3d&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABEDOADTHZIAWJDNSJ5Y2RDYYTAHHAVCNFSM6AAAAABDW7FTQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBSGE2TONJRGM . You are receiving this because you were mentioned.Message ID: @.***>

drgrice1 commented 5 months ago

Fortunately, in our case the consequences of rounding aren't so dire! It just means that the user will need to enter the TOTP code a little sooner than the message might have initially said. Note that the user will only see this message when they are entering the code and checking the box. They won't see it again until the time period has expired. So if it says 6 months and it is really 180 days, it probably isn't to big of a deal. Unless the user makes a note in their calendar of the day 6 months from now, and then comes hunting for the developers because they had to enter the TOTP code a few days earlier than noted in the calendar!

Alex-Jordan commented 5 months ago

Yeah, I just let my tags be expired for an additional year, and the state lost my tag renewal payment that year. Around here cops don't look for expired tags. Parking meter monitors do, but I never park in those parts of town so...

drgrice1 commented 5 months ago

In any case, I was working on coding this, and months are a hassle. I will do years, days, hours, minutes, and seconds. I plan to do something like only showing years and days (and omitting minutes and seconds) if there are years involved, and it will cascade down in that way. So there is some rounding to avoid messy long statements. We don't really need that kind of precision here. The time left that is shown for tests (in version_list.html.ep) does this sort of thing (although there are no days or years in that case).

drgrice1 commented 5 months ago

In thinking about this, and after working on trying to make a nice human readable duration that isn't a total kludge for to long now, I decided there really is no reason to specify the time duration at all in the message. It really is much harder than you think to give a nice duration message. Even using a library that handles this for you is unlikely to actually give a good message for any given duration.

The message will be changed to say that "two factor verification will be skipped for a limited time" but will not specify the time duration. The user will just have to enter the code again when that unspecified time ends. That is really all that they need to know. If they really want to know more, then they can talk to their system administrator.

Alex-Jordan commented 5 months ago

I've been googling about GD::Barcode::QRcode.pm. This post (really the comment at the bottom) suggests that once upon a time, the auto-selection of Version was unintentionally defaulting to version 1. The current code for the module, with Ecc => 'M' at Version => 1 would have max bytes 128 which matches exactly what @dlglin's error says. My takeaway is that @dlglin's error posted here earlier comes from using an old version of GD::Barcode::QRcode.pm that was packaged.

I'm about to open a PR to your branch that reduces size of these QR codes significantly. It works for me but of course we have reason to believe it needs to be tested on many systems.

Alex-Jordan commented 5 months ago

Well, not quite yet tonight. Tomorrow or Monday. I am still getting the warnings:

Use of uninitialized value within @aMask in bitwise xor (^) at /usr/local/share/perl5/GD/Barcode/QRcode.pm line 218.
Use of uninitialized value within @aMatrixX in array element at /usr/local/share/perl5/GD/Barcode/QRcode.pm line 218.
Use of uninitialized value within @aMatrixY in array element at /usr/local/share/perl5/GD/Barcode/QRcode.pm line 218.

any time I try to use Ecc => 'L'. I want to look into that more, but also can do a version of the PR I have ready using 'M'. It will still reduce the image size, just not as much.

Alex-Jordan commented 5 months ago

So, there is something wrong with the version of this package that I got using cpan that is leading to those warnings. I suspect that you would see them too if you try { Ecc => 'L', ModuleSize => 4, Version => 6 }. Note that for me the URL being encoded is still short enough that at 'L' (low error correction), I can get away with Version 6. I'm guessing you can too because there shouldn't be too much variation on the lengths of these URLs.

See if you get those warnings. I looked into QRcode.pm and basically the warnings are saying a certain array is constructed with some needed entries missing.

I can't tell if this is an issue that might also happen at Version higher than 6 if the URL happens to be just the right length.

drgrice1 commented 5 months ago

My takeaway is that @dlglin's error posted here earlier comes from using an old version of GD::Barcode::QRcode.pm that was packaged.

It would be good to get feedback from @dlglin on this. My understanding is that he was using the latest version of the package from cpan.

So, there is something wrong with the version of this package that I got using cpan that is leading to those warnings. I suspect that you would see them too if you try { Ecc => 'L', ModuleSize => 4, Version => 6 }. Note that for me the URL being encoded is still short enough that at 'L' (low error correction), I can get away with Version 6. I'm guessing you can too because there shouldn't be too much variation on the lengths of these URLs.

So there is a change that affects the choice of these parameters considerably. I tested the parameters you give here with this pull request and it worked fine with no errors or warnings, but then I put them into my script and it failed with an overflow error. I then copied the link from the browser into the script, and it worked fine. The link that I had in the script was from before switching from the SHA512 algorithm to the SHA1 algorithm. I noticed that now the link is considerably shorter. I will need to retest everything, but the rules have changed from my earlier testing.

See if you get those warnings. I looked into QRcode.pm and basically the warnings are saying a certain array is constructed with some needed entries missing.

I haven't seen these warning at all in any of my testing. I either get an image generated with no warnings or I get the overflow error and no image.

drgrice1 commented 5 months ago

I did find a platform that gives the warnings you mention now. I used the perl:5.34 docker image which seems to be using debian 11.8. If I use the options { Ecc => 'L', ModuleSize => 4, Version => 6 } on that it gives the warnings you show. If I tweak that to { Ecc => 'L', ModuleSize => 4, Version => 7 } though the warnings go away.

In general using too low of a version is the problem. We need to use a higher version to ensure the image generates and doesn't cause warnings. We need a margin of error so that variation in link length doesn't cause an issue. I suggest using { Ecc => 'L', ModuleSize => 4, Version => 8 } for this. I haven't tested this extensively, but in my initial testing this gives a smaller image and doesn't cause warnings or errors.

drgrice1 commented 5 months ago

I have tested further, and using { Ecc => 'L', ModuleSize => 4, Version => 8 } seems to work well without warnings on all platforms. It is also worth noting that there is not much difference in the resulting image if you use Version 7 or 8.

Using { Ecc => 'L', ModuleSize => 4, Version => 6 } generates images, but gives warnings on Fedora, Oracle Linux, Debian (perl docker image), and Redhat. On Ubuntu I have never seen the warnings yet.

drgrice1 commented 5 months ago

So I though about this a little more. Version 8 even will not be enough. The primary factors that will cause the link length to change (and thus affect which version is needed) are the user id and course id. The maximum user id length is 100 characters, and the maximum course id length is 40. I don't think the base 32 encoded sha1 hash of the secret which is 20 randomly generated characters will change. I believe that will always be 32 characters. This comes to a maximum link length of 237 characters. Testing with a maximum length user id and course id, I see that a version of 8 fails. A version of 9 also fails. Version 10 does work. So in the end, I think that we will need to use version 10.

Alex-Jordan commented 5 months ago

Is anything in the URL case sensitive? You will get better (lower but count) if all letters are made capital and if you URL escape the URL to replace the ? and = signs. This is because if all letters are: 0–9, A–Z (upper-case only), space, $, %, *, +, -, ., /, : Then QR code generation uses a different algorithm.

That was one thing I did to reduce size. The other is that once you do that, you can manually calculate the optimal version number using a table.

But then it turns out the optimal version number triggers those perl warnings. I think maybe you could do all that I just outlined but choose the maximum between 7 and the calculated version number.

On Sun, Mar 17, 2024, 4:17 AM Glenn Rice @.***> wrote:

I have tested further, and using { Ecc => 'L', ModuleSize => 4, Version => 8 } seems to work well without warnings on all platforms. It is also worth noting that there is not much difference in the resulting image if you use Version 7 or 8.

Using { Ecc => 'L', ModuleSize => 4, Version => 6 } generates images, but gives warnings on Fedora, Oracle Linux, Debian (perl docker image), and Redhat. On Ubuntu I have never seen the warnings yet.

— Reply to this email directly, view it on GitHub https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-a1712eb8b25a2f19&q=1&e=915d61a4-6825-4ffa-8fa0-b1cddcf93f80&u=https%3A%2F%2Fgithub.com%2Fopenwebwork%2Fwebwork2%2Fpull%2F2335%23issuecomment-2002414125, or unsubscribe https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-fa639a039f656a73&q=1&e=915d61a4-6825-4ffa-8fa0-b1cddcf93f80&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABEDOACL5QKE4NLYBB6XTJTYYV3VNAVCNFSM6AAAAABDW7FTQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBSGQYTIMJSGU . You are receiving this because you were mentioned.Message ID: @.***>

Alex-Jordan commented 5 months ago

Even if the letters can't be made all capital, there is still a way to calculate the optimal version ahead of time, rather than lock it in at 10 or whatever.

On Sun, Mar 17, 2024, 8:16 AM Alex Jordan @.***> wrote:

Is anything in the URL case sensitive? You will get better (lower but count) if all letters are made capital and if you URL escape the URL to replace the ? and = signs. This is because if all letters are: 0–9, A–Z (upper-case only), space, $, %, *, +, -, ., /, : Then QR code generation uses a different algorithm.

That was one thing I did to reduce size. The other is that once you do that, you can manually calculate the optimal version number using a table.

But then it turns out the optimal version number triggers those perl warnings. I think maybe you could do all that I just outlined but choose the maximum between 7 and the calculated version number.

On Sun, Mar 17, 2024, 4:17 AM Glenn Rice @.***> wrote:

I have tested further, and using { Ecc => 'L', ModuleSize => 4, Version => 8 } seems to work well without warnings on all platforms. It is also worth noting that there is not much difference in the resulting image if you use Version 7 or 8.

Using { Ecc => 'L', ModuleSize => 4, Version => 6 } generates images, but gives warnings on Fedora, Oracle Linux, Debian (perl docker image), and Redhat. On Ubuntu I have never seen the warnings yet.

— Reply to this email directly, view it on GitHub https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-a1712eb8b25a2f19&q=1&e=915d61a4-6825-4ffa-8fa0-b1cddcf93f80&u=https%3A%2F%2Fgithub.com%2Fopenwebwork%2Fwebwork2%2Fpull%2F2335%23issuecomment-2002414125, or unsubscribe https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-fa639a039f656a73&q=1&e=915d61a4-6825-4ffa-8fa0-b1cddcf93f80&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABEDOACL5QKE4NLYBB6XTJTYYV3VNAVCNFSM6AAAAABDW7FTQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBSGQYTIMJSGU . You are receiving this because you were mentioned.Message ID: @.***>

drgrice1 commented 5 months ago

The characters are case sensitive. You certainly can't just convert the link all to uppercase. You might be able to auto detect a version a little better, but I have to ask ... what is your real objective? A QR code is not designed to really be a visually appealing thing. This is really a lot of effort to obtain a minimal gain in appearance. It really doesn't make a lot of sense.

drgrice1 commented 5 months ago

URL escaping the link also fails.

Alex-Jordan commented 5 months ago

Yes, I'm fine with whatever you want to do. I am just brainstorming how to overcome the bug(s) with this package.

On Sun, Mar 17, 2024, 9:31 AM Glenn Rice @.***> wrote:

URL escaping the link also fails.

— Reply to this email directly, view it on GitHub https://protect2.fireeye.com/v1/url?k=31323334-501d2dca-3132feb7-454455534531-0e241ec20716d417&q=1&e=69618edc-d613-4c41-aca5-ee2010055f4f&u=https%3A%2F%2Fgithub.com%2Fopenwebwork%2Fwebwork2%2Fpull%2F2335%23issuecomment-2002531432, or unsubscribe https://protect2.fireeye.com/v1/url?k=31323334-501d2dca-3132feb7-454455534531-15412648154107ef&q=1&e=69618edc-d613-4c41-aca5-ee2010055f4f&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABEDOAALGAMRFPICUE5DHPLYYXAPJAVCNFSM6AAAAABDW7FTQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBSGUZTCNBTGI . You are receiving this because you were mentioned.Message ID: @.***>

Alex-Jordan commented 5 months ago

Just for reference where I learned more about how QR codes work: if the characters are all treated as "binary" (a misnomer that applies to characters outside of that list I gave before) then the number of bits in the QR code is 4+8+8*length(string being encoded). With Ecc => 'L', this translates to { 1856 => 9, 1552 => 8, 1248 => 7 }. If the bit count is for example 1300, you "round up" to 1552 and use version 8.

If the bit count ends up more than 1856, then you have to redo the bit count: 4+16+8*length(string being encoded). Now the table is { 2192 => 10, 2592 => 11, ... }.

This is all stuff worked out from https://www.qrcode.com/en/about/version.html.

drgrice1 commented 5 months ago

Test with my latest changes.

Alex-Jordan commented 5 months ago

That's a nice way to do it in the last commit!

drgrice1 commented 5 months ago

Yeah, it seems that the warnings when using Version => 0 are inconsequential. So why not just suppress them?

Alex-Jordan commented 5 months ago

I tested all the way through everything, including the use_two_factor_auth permission, and it works for me. This is with Oracle 8.7, using the system perl 5.26, with GD::Barcode::QRcode installed using cpan.

Alex-Jordan commented 5 months ago

Two approvals, but I think we should wait to hear from @dlglin.

pstaabp commented 5 months ago

Just retested after the past couple of days of updates.

  1. Tested using Duo, Microsoft Authenticator and the built-in iOS authenticator. All with success.
  2. Tested the permission level.
dlglin commented 5 months ago

In Fedora switching from the dnf packaged version of Mojolicious to cpan fixed the Base64 encoded image issue.

Both Fedora 39 and RHEL 8 have GD::Barcode version 1.15 packaged. In both cases I had to uninstall the packaged version and install from CPAN (version 2.0). With the CPAN version it works on both.

I did a quick test with both the Microsoft and Google Authenticator Apps and was able to log in with 2FA. One observation: The name of the entry in the authenticator app is the course name. How about prefixing this with one of the following to make it more descriptive:

I didn't test the permission level.

Alex-Jordan commented 5 months ago

You'll have multiple items in the authenticator app for the multiple courses you need to authenticate into as a WW admin (or even as an instructor or student). So it helps if the items in the app name the particular course too.