Bug 29894 - 2FA: Add few validations, clear secret, send register notice
Summary: 2FA: Add few validations, clear secret, send register notice
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 29873
Blocks: 20476 28998
  Show dependency treegraph
 
Reported: 2022-01-17 14:02 UTC by Marcel de Rooy
Modified: 2022-12-12 21:23 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 29894: Module changes (preliminary) (2.16 KB, patch)
2022-01-17 15:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: First draft for testing (2.67 KB, patch)
2022-01-17 15:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Add some exceptions to TwoFactorAuth module (6.79 KB, patch)
2022-01-20 15:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Clear secret when disabling 2FA (1.60 KB, patch)
2022-01-20 15:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Add some exceptions to TwoFactorAuth module (6.79 KB, patch)
2022-01-21 10:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Clear secret when disabling 2FA (1.60 KB, patch)
2022-01-21 10:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Db rev for adding notices (3.90 KB, patch)
2022-01-21 10:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Send a confirmation notice (7.07 KB, patch)
2022-01-21 10:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Add some exceptions to TwoFactorAuth module (6.85 KB, patch)
2022-04-14 06:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Clear secret when disabling 2FA (1.60 KB, patch)
2022-04-14 06:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Db rev for adding notices (3.95 KB, patch)
2022-04-14 06:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Send a confirmation notice (7.05 KB, patch)
2022-04-14 06:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Add some exceptions to TwoFactorAuth module (6.87 KB, patch)
2022-04-21 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29894: Clear secret when disabling 2FA (1.63 KB, patch)
2022-04-21 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29894: Db rev for adding notices (3.97 KB, patch)
2022-04-21 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29894: Send a confirmation notice (7.08 KB, patch)
2022-04-21 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29894: (follow-up) Update notice terms (6.06 KB, patch)
2022-04-21 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29894: (QA follow-up) Get rid of send_confirm_notice (6.27 KB, patch)
2022-04-22 06:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29894: Add some exceptions to TwoFactorAuth module (6.95 KB, patch)
2022-04-22 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29894: Clear secret when disabling 2FA (1.70 KB, patch)
2022-04-22 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29894: Db rev for adding notices (4.05 KB, patch)
2022-04-22 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29894: Send a confirmation notice (7.16 KB, patch)
2022-04-22 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29894: (follow-up) Update notice terms (6.14 KB, patch)
2022-04-22 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29894: (QA follow-up) Get rid of send_confirm_notice (6.31 KB, patch)
2022-04-22 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29894: Add Selenium tests for disable 2FA (2.17 KB, patch)
2022-04-22 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-01-17 14:02:41 UTC
Follow-up report on bug 28786
Comment 1 Marcel de Rooy 2022-01-17 15:04:43 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.
Comment 2 Marcel de Rooy 2022-01-17 15:05:26 UTC
I do like to have only a secret parameter and handle the secret/secret32 stuff inside the module only. Expirementing a bit now.
Comment 3 Marcel de Rooy 2022-01-17 15:05:56 UTC
Throw some exceptions on missing parameters. Trivial edits.
Comment 4 Marcel de Rooy 2022-01-17 15:08:22 UTC
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?
Comment 5 Marcel de Rooy 2022-01-17 15:08:53 UTC
(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..
Comment 6 Marcel de Rooy 2022-01-17 15:10:01 UTC
Add some additional tests
Comment 7 Marcel de Rooy 2022-01-17 15:29:43 UTC
Created attachment 129538 [details] [review]
Bug 29894: Module changes (preliminary)
Comment 8 Marcel de Rooy 2022-01-17 15:30:52 UTC
Created attachment 129539 [details] [review]
Bug 29894: First draft for testing
Comment 9 Marcel de Rooy 2022-01-20 15:13:26 UTC
(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.
Comment 10 Marcel de Rooy 2022-01-20 15:16:54 UTC
(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.
Comment 11 Marcel de Rooy 2022-01-20 15:20:58 UTC
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>
Comment 12 Marcel de Rooy 2022-01-20 15:21:01 UTC
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>
Comment 13 Marcel de Rooy 2022-01-21 10:29:13 UTC
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>
Comment 14 Marcel de Rooy 2022-01-21 10:29:17 UTC
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>
Comment 15 Marcel de Rooy 2022-01-21 10:29:20 UTC
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>
Comment 16 Marcel de Rooy 2022-01-21 10:29:24 UTC
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>
Comment 17 Marcel de Rooy 2022-01-21 11:01:28 UTC
 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.
Comment 18 Marcel de Rooy 2022-04-14 06:27:31 UTC
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>
Comment 19 Marcel de Rooy 2022-04-14 06:27:35 UTC
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>
Comment 20 Marcel de Rooy 2022-04-14 06:27:39 UTC
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>
Comment 21 Marcel de Rooy 2022-04-14 06:27:44 UTC
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>
Comment 22 Marcel de Rooy 2022-04-14 06:28:04 UTC
Rebased
Comment 23 Martin Renvoize 2022-04-21 13:57:40 UTC
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>
Comment 24 Martin Renvoize 2022-04-21 13:57:45 UTC
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>
Comment 25 Martin Renvoize 2022-04-21 13:57:50 UTC
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>
Comment 26 Martin Renvoize 2022-04-21 13:57:54 UTC
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>
Comment 27 Martin Renvoize 2022-04-21 13:57:59 UTC
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>
Comment 28 Martin Renvoize 2022-04-21 13:59:55 UTC
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
Comment 29 Jonathan Druart 2022-04-21 14:45:25 UTC
(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.
Comment 30 Marcel de Rooy 2022-04-22 06:13:29 UTC
(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.
Comment 31 Marcel de Rooy 2022-04-22 06:50:31 UTC
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>
Comment 32 Marcel de Rooy 2022-04-22 06:51:30 UTC
(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!
Comment 33 Jonathan Druart 2022-04-22 10:30:15 UTC
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>
Comment 34 Jonathan Druart 2022-04-22 10:30:21 UTC
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>
Comment 35 Jonathan Druart 2022-04-22 10:30:28 UTC
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>
Comment 36 Jonathan Druart 2022-04-22 10:30:35 UTC
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>
Comment 37 Jonathan Druart 2022-04-22 10:30:41 UTC
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>
Comment 38 Jonathan Druart 2022-04-22 10:30:45 UTC
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>
Comment 39 Jonathan Druart 2022-04-22 10:30:50 UTC
Created attachment 133667 [details] [review]
Bug 29894: Add Selenium tests for disable 2FA

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 40 Fridolin Somers 2022-05-02 21:24:18 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄