|
Description
Marcel de Rooy
2022-01-17 14:02:41 UTC
I was thinking about if we should pass in the patron's secret and not make it an automatic default. But decided to leave it as-is now. I do like to have only a secret parameter and handle the secret/secret32 stuff inside the module only. Expirementing a bit now. Throw some exceptions on missing parameters. Trivial edits. Still thinking about what we do now in registering: create secret, pass it out, get it back, etc. Easy way to build the qr but not pass the secret in that process? (In reply to Marcel de Rooy from comment #4) > Still thinking about what we do now in registering: create secret, pass it > out, get it back, etc. > Easy way to build the qr but not pass the secret in that process? Obviously the secret is in the qr code. I know.. Add some additional tests Created attachment 129538 [details] [review] Bug 29894: Module changes (preliminary) Created attachment 129539 [details] [review] Bug 29894: First draft for testing (In reply to Marcel de Rooy from comment #2) > I do like to have only a secret parameter and handle the secret/secret32 > stuff inside the module only. Expirementing a bit now. I am skipping this. Too much shuffling. But will improve validation. (In reply to Marcel de Rooy from comment #4) > Still thinking about what we do now in registering: create secret, pass it > out, get it back, etc. > Easy way to build the qr but not pass the secret in that process? The CSRF token protects the secret. Fine enough probably. Especially if we would send a notice when we register or deregister 2FA to inform the user about such a change. That notice might be a simple pragmatic additional precaution. Created attachment 129669 [details] [review] Bug 29894: Add some exceptions to TwoFactorAuth module Test updated accordingly. Adding utf8 flag to CGI in staff script. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 129670 [details] [review] Bug 29894: Clear secret when disabling 2FA Test plan: Deregister 2FA for patron. Check if secret is empty in borrowers.secret. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 129689 [details] [review] Bug 29894: Add some exceptions to TwoFactorAuth module Test updated accordingly. Adding utf8 flag to CGI in staff script. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 129690 [details] [review] Bug 29894: Clear secret when disabling 2FA Test plan: Deregister 2FA for patron. Check if secret is empty in borrowers.secret. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 129691 [details] [review] Bug 29894: Db rev for adding notices This dbrev adds two notices: 2FA_REGISTER and 2FA_DEREGISTER Test plan: Run the dbrev. Check if you see them on Tools/Notices. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 129692 [details] [review] Bug 29894: Send a confirmation notice When registering or deregistering, send a confirmation. Test plan: Register or deregister with patron having email address. Verify that you got a confirmation mail. Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> FAIL Koha/Auth/TwoFactorAuth.pm
FAIL valid
Constant subroutine Koha::Auth::TwoFactorAuth::CONFIRM_NOTICE_REG redefined
Constant subroutine Koha::Auth::TwoFactorAuth::CONFIRM_NOTICE_DEREG redefined
Dont let this bother you. No problem with these constants.
Created attachment 133286 [details] [review] Bug 29894: Add some exceptions to TwoFactorAuth module Test updated accordingly. Adding utf8 flag to CGI in staff script. Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 133287 [details] [review] Bug 29894: Clear secret when disabling 2FA Test plan: Deregister 2FA for patron. Check if secret is empty in borrowers.secret. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 133288 [details] [review] Bug 29894: Db rev for adding notices This dbrev adds two notices: 2FA_REGISTER and 2FA_DEREGISTER Test plan: Run the dbrev. Check if you see them on Tools/Notices. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 133289 [details] [review] Bug 29894: Send a confirmation notice When registering or deregistering, send a confirmation. Test plan: Register or deregister with patron having email address. Verify that you got a confirmation mail. Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased Created attachment 133562 [details] [review] Bug 29894: Add some exceptions to TwoFactorAuth module Test updated accordingly. Adding utf8 flag to CGI in staff script. Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 133563 [details] [review] Bug 29894: Clear secret when disabling 2FA Test plan: Deregister 2FA for patron. Check if secret is empty in borrowers.secret. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 133564 [details] [review] Bug 29894: Db rev for adding notices This dbrev adds two notices: 2FA_REGISTER and 2FA_DEREGISTER Test plan: Run the dbrev. Check if you see them on Tools/Notices. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 133565 [details] [review] Bug 29894: Send a confirmation notice When registering or deregistering, send a confirmation. Test plan: Register or deregister with patron having email address. Verify that you got a confirmation mail. Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 133566 [details] [review] Bug 29894: (follow-up) Update notice terms Register and Derigister didn't sound right, in reality we we enabling and disabling 2FA for the user so I think those terms are more understandable. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> All works well so signing off.. I added a follow-up to use what I feel are clearer terms in the notices. My one caveat is something doesn't feel quite right about the introduction of 'send_confirm_notice' as a method in Koha::Auth::TwoFactorAuth, but I can see why you do it so it's testable.. I'll leave that query to QA (In reply to Martin Renvoize from comment #28) > My one caveat is something doesn't feel quite right about the introduction > of 'send_confirm_notice' as a method in Koha::Auth::TwoFactorAuth, but I can > see why you do it so it's testable.. I'll leave that query to QA That's something I noticed as well, it feels misplaced. (In reply to Jonathan Druart from comment #29) > (In reply to Martin Renvoize from comment #28) > > My one caveat is something doesn't feel quite right about the introduction > > of 'send_confirm_notice' as a method in Koha::Auth::TwoFactorAuth, but I can > > see why you do it so it's testable.. I'll leave that query to QA > > That's something I noticed as well, it feels misplaced. Yeah. This was kind of expected. Will try to find another home for him. Created attachment 133618 [details] [review] Bug 29894: (QA follow-up) Get rid of send_confirm_notice Chose here to fall back to $patron->queue_notice. Which is tested already, so removing the additional test code. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Martin Renvoize from comment #28) > All works well so signing off.. I added a follow-up to use what I feel are > clearer terms in the notices. > > My one caveat is something doesn't feel quite right about the introduction > of 'send_confirm_notice' as a method in Koha::Auth::TwoFactorAuth, but I can > see why you do it so it's testable.. I'll leave that query to QA Thanks, Martin. Addressed the misplaced method now. Joubu, go ahead! Created attachment 133661 [details] [review] Bug 29894: Add some exceptions to TwoFactorAuth module Test updated accordingly. Adding utf8 flag to CGI in staff script. Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t 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 133662 [details] [review] Bug 29894: Clear secret when disabling 2FA Test plan: Deregister 2FA for patron. Check if secret is empty in borrowers.secret. 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 133663 [details] [review] Bug 29894: Db rev for adding notices This dbrev adds two notices: 2FA_REGISTER and 2FA_DEREGISTER Test plan: Run the dbrev. Check if you see them on Tools/Notices. 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 133664 [details] [review] Bug 29894: Send a confirmation notice When registering or deregistering, send a confirmation. Test plan: Register or deregister with patron having email address. Verify that you got a confirmation mail. Run t/db_dependent/Koha/Auth/TwoFactorAuth.t 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 133665 [details] [review] Bug 29894: (follow-up) Update notice terms Register and Derigister didn't sound right, in reality we we enabling and disabling 2FA for the user so I think those terms are more understandable. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 133666 [details] [review] Bug 29894: (QA follow-up) Get rid of send_confirm_notice Chose here to fall back to $patron->queue_notice. Which is tested already, so removing the additional test code. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 133667 [details] [review] Bug 29894: Add Selenium tests for disable 2FA Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master for 22.05, thanks to everybody involved [U+1F984] |