Summary: | 2FA: Generate QR code without exposing secret via HTTP GET | ||
---|---|---|---|
Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
Component: | Authentication | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | dpavlin, jonathan.druart, kyle, martin.renvoize, mtj |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
Instead of calling the (deprecated) Google Charts API and exposing our secret, we create the image ourself and push it as a data uri in the src attribute of the image (inline image).
|
Version(s) released in: |
22.05.00
|
Circulation function: | |||
Bug Depends on: | 28786 | ||
Bug Blocks: | 29894, 32011 | ||
Attachments: |
Bug 29873: Create QR code
Bug 29873: Create QR code Bug 29873: Add unit test Bug 29873: Adjust QR image size Bug 29873: Add unit test Bug 29873: Adjust QR image size Bug 29873: Update cpanfile Bug 29873: (follow-up) Rename qr_dataurl Bug 29873: Create QR code Bug 29873: Add unit test Bug 29873: Create QR code Bug 29873: Add unit test Bug 29873: Create QR code Bug 29873: Add unit test |
Description
Marcel de Rooy
2022-01-13 12:32:45 UTC
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] |