See bug 28786. The Google Charts API shows the secret in the src of the image.
Will give it a go
Created attachment 129402 [details] [review] Bug 29873: Create QR code
Work in progress. But should give you an idea. Works for me. But needs some POD, unit testing, dependency registering. It uses Imager::QRCode, packaged for Debian in libimager-qrcode-perl.
Created attachment 129576 [details] [review] Bug 29873: Create QR code Instead of using deprecated Google Charts API, and exposing our secret in a GET parameter, we generate QR data ourselves. Test plan: [1] Enable two factor authentication in the prefs. [2] Login in staff. Go to account. Select Manage 2FA. [3] Verify that QR code is displayed. [4] Register the QR in your authenticator app and test 2FA by logging in again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested with Google Authenticator and FreeOTP.
Created attachment 129577 [details] [review] Bug 29873: Add unit test Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t
Still looking at size of QR image (but it works). And impact on Selenium test.
Created attachment 129578 [details] [review] Bug 29873: Adjust QR image size Size parameter 8 results in too big images. Size parameter 4 seems fine. Although I have seen variations in the number of pixels from about 116x116 to 180x180. The documentation is a bit obsure. Test plan: Register again with smaller QR code and logout/login.
(In reply to Marcel de Rooy from comment #6) > Still looking at size of QR image (but it works). And impact on Selenium > test. t/db_dependent/selenium/authentication_2fa.t seems fine to me still. Could you check, Jonathan?
Created attachment 129579 [details] [review] Bug 29873: Add unit test Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129580 [details] [review] Bug 29873: Adjust QR image size Size parameter 8 results in too big images. Size parameter 4 seems fine. Although I have seen variations in the number of pixels from about 116x116 to 180x180. The documentation is a bit obsure. Test plan: Register again with smaller QR code and logout/login. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129581 [details] [review] Bug 29873: Update cpanfile Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I would rename ->qr_dataurl with ->qr_code as it actually generates an image.
Created attachment 129605 [details] [review] Bug 29873: (follow-up) Rename qr_dataurl As requested by a QA team member. We're moving to qr_code as method name. This is the same name as the method in the underlying base class. Apart from one sed statement, changing to self->SUPER on one line. Test plan: Can you still register, logout and login? Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #12) > I would rename ->qr_dataurl with ->qr_code as it actually generates an image. Renamed. Added a few POD lines now too.
Hmm. Should we have done something like: $image = GD::Barcode->new('QRcode', $barcode, { Ecc => "M", ModuleSize => $qrcode_modulesize } )->plot->png();
GD::Barcode::QRcode
Back to ASSIGNED. If all the stuff that comes before this one, gets clearer, I will resubmit.
Second try
Created attachment 133216 [details] [review] Bug 29873: Create QR code Instead of using deprecated Google Charts API, and exposing our secret in a GET parameter, we generate QR data ourselves. Test plan: [1] Enable two factor authentication in the prefs. [2] Login in staff. Go to account. Select Manage 2FA. [3] Verify that QR code is displayed. [4] Register the QR in your authenticator app and test 2FA by logging in again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested with Google Authenticator and FreeOTP. Bug 29873: (follow-up) Rename qr_dataurl As requested by a QA team member. We're moving to qr_code as method name. This is the same name as the method in the underlying base class. Apart from one sed statement, changing to self->SUPER on one line. Test plan: Can you still register, logout and login? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 29873: (follow-up) Switch to GD We do not need a new module, we could use GD instead. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133217 [details] [review] Bug 29873: Add unit test Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended to reflect requested changes.
Created attachment 133558 [details] [review] Bug 29873: Create QR code Instead of using deprecated Google Charts API, and exposing our secret in a GET parameter, we generate QR data ourselves. Test plan: [1] Enable two factor authentication in the prefs. [2] Login in staff. Go to account. Select Manage 2FA. [3] Verify that QR code is displayed. [4] Register the QR in your authenticator app and test 2FA by logging in again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested with Google Authenticator and FreeOTP. Bug 29873: (follow-up) Rename qr_dataurl As requested by a QA team member. We're moving to qr_code as method name. This is the same name as the method in the underlying base class. Apart from one sed statement, changing to self->SUPER on one line. Test plan: Can you still register, logout and login? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 29873: (follow-up) Switch to GD We do not need a new module, we could use GD instead. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133559 [details] [review] Bug 29873: Add unit test Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended to reflect requested changes. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Good call.. works as expected. Signing of.. Must admit, I still wonder if we aught to have gone with Authen::OATH rather than Auth::GoogleAuth for 2FA.. but that's another question entirely.
(In reply to Martin Renvoize from comment #23) > Must admit, I still wonder if we aught to have gone with Authen::OATH rather > than Auth::GoogleAuth for 2FA.. but that's another question entirely. Discussed extensively on bug 28786 already. tldr; With this patch there is no bad reason to not switch (but do we have a good reason to switch?).
(In reply to Jonathan Druart from comment #24) > (In reply to Martin Renvoize from comment #23) > > Must admit, I still wonder if we aught to have gone with Authen::OATH rather > > than Auth::GoogleAuth for 2FA.. but that's another question entirely. > > Discussed extensively on bug 28786 already. > > tldr; With this patch there is no bad reason to not switch (but do we have a > good reason to switch?). Lets not do that now anymore. Go ahead, Joubu. QA time!
Created attachment 133659 [details] [review] Bug 29873: Create QR code Instead of using deprecated Google Charts API, and exposing our secret in a GET parameter, we generate QR data ourselves. Test plan: [1] Enable two factor authentication in the prefs. [2] Login in staff. Go to account. Select Manage 2FA. [3] Verify that QR code is displayed. [4] Register the QR in your authenticator app and test 2FA by logging in again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested with Google Authenticator and FreeOTP. Bug 29873: (follow-up) Rename qr_dataurl As requested by a QA team member. We're moving to qr_code as method name. This is the same name as the method in the underlying base class. Apart from one sed statement, changing to self->SUPER on one line. Test plan: Can you still register, logout and login? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 29873: (follow-up) Switch to GD We do not need a new module, we could use GD instead. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 133660 [details] [review] Bug 29873: Add unit test Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended to reflect requested changes. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 22.05, thanks to everybody involved [U+1F984]