Bug 30588 - Add the option to require 2FA setup on first staff login
Summary: Add the option to require 2FA setup on first staff login
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low new feature (vote)
Assignee: Jonathan Druart
QA Contact: Nick Clemens
URL:
Keywords:
: 32404 (view as bug list)
Depends on: 28786 28787 31244
Blocks: 20476 31912 32010 32066 32376
  Show dependency treegraph
 
Reported: 2022-04-21 16:34 UTC by Martin Renvoize
Modified: 2023-12-28 20:43 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/667
Text to go in the release notes:
This adds a third option 'enforce' to the TwoFactorAuthentication system preference. If chosen, staff will no longer be able to log into the staff client without setting up and using 2-factor authentication.
Version(s) released in:
22.11.00


Attachments
Bug 30588: Add 'enforced' option to TwoFactorAuthentication (2.39 KB, patch)
2022-07-27 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Adjust existing occurrences of TwoFactorAuthentication (8.13 KB, patch)
2022-07-27 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add the option to require 2FA setup on first staff login (25.86 KB, patch)
2022-07-27 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Adjust existing selenium tests (5.41 KB, patch)
2022-07-27 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add tests for the REST API routes (5.77 KB, patch)
2022-07-27 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add selenium tests (4.53 KB, patch)
2022-07-27 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add tests for C4::checkauth (1.41 KB, patch)
2022-07-27 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add missing atomic update (1.16 KB, patch)
2022-07-28 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add missing include file (2.18 KB, patch)
2022-07-28 08:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add 'enforced' option to TwoFactorAuthentication (2.39 KB, patch)
2022-08-10 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Adjust existing occurrences of TwoFactorAuthentication (8.12 KB, patch)
2022-08-10 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add the option to require 2FA setup on first staff login (25.86 KB, patch)
2022-08-10 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Adjust existing selenium tests (5.41 KB, patch)
2022-08-10 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add tests for the REST API routes (5.77 KB, patch)
2022-08-10 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add selenium tests (4.53 KB, patch)
2022-08-10 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add tests for C4::checkauth (1.45 KB, patch)
2022-08-10 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add missing atomic update (1.16 KB, patch)
2022-08-10 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add missing include file (2.18 KB, patch)
2022-08-10 08:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: FIX serial claims filters (1.09 KB, patch)
2022-08-10 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Typos in sysprefs.sql (1.68 KB, patch)
2022-09-09 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Missing shebang in TwoFactorAuth.t (830 bytes, patch)
2022-09-09 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Remove erroneous FIXME (1.14 KB, patch)
2022-09-09 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Auth - remove two warns and second logout (2.15 KB, patch)
2022-09-09 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Adjust flags for two_factor_auth.pl (1.07 KB, patch)
2022-09-09 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: Add 'enforced' option to TwoFactorAuthentication (2.48 KB, patch)
2022-09-12 09:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: Adjust existing occurrences of TwoFactorAuthentication (8.20 KB, patch)
2022-09-12 09:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: Add the option to require 2FA setup on first staff login (25.94 KB, patch)
2022-09-12 09:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: Adjust existing selenium tests (5.51 KB, patch)
2022-09-12 09:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: Add tests for the REST API routes (5.86 KB, patch)
2022-09-12 09:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: Add selenium tests (4.62 KB, patch)
2022-09-12 09:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: Add tests for C4::checkauth (1.54 KB, patch)
2022-09-12 09:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: Add missing atomic update (1.25 KB, patch)
2022-09-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: Add missing include file (2.27 KB, patch)
2022-09-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Typos in sysprefs.sql (1.68 KB, patch)
2022-09-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Missing shebang in TwoFactorAuth.t (830 bytes, patch)
2022-09-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Remove erroneous FIXME (1.14 KB, patch)
2022-09-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Auth - remove two warns and second logout (2.15 KB, patch)
2022-09-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Adjust flags for two_factor_auth.pl (1.07 KB, patch)
2022-09-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Clear waiting-for-2FA-setup in session (1.33 KB, patch)
2022-09-12 09:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30588: Add 'enforced' option to TwoFactorAuthentication (2.39 KB, patch)
2022-10-17 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Adjust existing occurrences of TwoFactorAuthentication (8.12 KB, patch)
2022-10-17 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add the option to require 2FA setup on first staff login (25.85 KB, patch)
2022-10-17 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Adjust existing selenium tests (5.41 KB, patch)
2022-10-17 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add tests for the REST API routes (5.77 KB, patch)
2022-10-17 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add selenium tests (4.53 KB, patch)
2022-10-17 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add tests for C4::checkauth (1.45 KB, patch)
2022-10-17 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add missing atomic update (1.16 KB, patch)
2022-10-17 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add missing include file (2.18 KB, patch)
2022-10-17 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Typos in sysprefs.sql (1.64 KB, patch)
2022-10-17 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Missing shebang in TwoFactorAuth.t (792 bytes, patch)
2022-10-17 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Remove erroneous FIXME (1.11 KB, patch)
2022-10-17 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Auth - remove two warns and second logout (2.12 KB, patch)
2022-10-17 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Adjust flags for two_factor_auth.pl (1.03 KB, patch)
2022-10-17 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Clear waiting-for-2FA-setup in session (1.29 KB, patch)
2022-10-17 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30588: Add 'enforced' option to TwoFactorAuthentication (2.44 KB, patch)
2022-10-21 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: Adjust existing occurrences of TwoFactorAuthentication (8.16 KB, patch)
2022-10-21 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: Add the option to require 2FA setup on first staff login (25.89 KB, patch)
2022-10-21 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: Adjust existing selenium tests (5.47 KB, patch)
2022-10-21 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: Add tests for the REST API routes (5.82 KB, patch)
2022-10-21 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: Add selenium tests (4.58 KB, patch)
2022-10-21 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: Add tests for C4::checkauth (1.50 KB, patch)
2022-10-21 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: Add missing atomic update (1.21 KB, patch)
2022-10-21 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: Add missing include file (2.23 KB, patch)
2022-10-21 12:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Typos in sysprefs.sql (1.69 KB, patch)
2022-10-21 12:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Missing shebang in TwoFactorAuth.t (847 bytes, patch)
2022-10-21 12:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Remove erroneous FIXME (1.16 KB, patch)
2022-10-21 12:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Auth - remove two warns and second logout (2.17 KB, patch)
2022-10-21 12:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Adjust flags for two_factor_auth.pl (1.09 KB, patch)
2022-10-21 12:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30588: (QA follow-up) Clear waiting-for-2FA-setup in session (1.35 KB, patch)
2022-10-21 12:28 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2022-04-21 16:34:24 UTC
We should add the option to require 2FA configuration at first login for staff users when 2FA is enabled on the system.
Comment 1 Jonathan Druart 2022-07-27 08:05:18 UTC
Created attachment 138160 [details] [review]
Bug 30588: Add 'enforced' option to TwoFactorAuthentication

Sponsored-by: Rijksmuseum, Netherlands
Comment 2 Jonathan Druart 2022-07-27 08:05:22 UTC
Created attachment 138161 [details] [review]
Bug 30588: Adjust existing occurrences of TwoFactorAuthentication

We need to replace 0 with 'disabled', and 1 with 'enabled'

Sponsored-by: Rijksmuseum, Netherlands
Comment 3 Jonathan Druart 2022-07-27 08:05:27 UTC
Created attachment 138162 [details] [review]
Bug 30588: Add the option to require 2FA setup on first staff login

Bug 28786 added the ability to turn on a two-factor authentication,
using a One Time Password (OTP).
Once enabled on the system, librarian had the choice to enable or
disable it for themselves.
For security reason an administrator could decide to force the
librarians to use this second authentication step.

This patch adds a third option to the existing syspref, 'Enforced', for
that purpose.

QA notes: the code we had in the members/two_factor_auth.pl controller
has been moved to REST API controller methods (with their tests and
swagger specs), for reusability reason. Code from template has been
moved to an include file for the same reason.

Test plan:
A. Regression tests
As we modified the code we need first to confirm the existing features
are still working as expected.
1. Turn off TwoFactorAuthentication (disabled) and confirm that you are not able to
enable and access the second authentication step
2. Turn it on (enabled) and confirm that you are able to enable it in your account
3. Logout and confirm then that you are able to login into Koha

B. The new option
1. Set the pref to "enforced"
2. You are not logged out, logged in users stay logged in
3. Pick a user that does not have 2FA setup, login
4. Notice the new screen (UI is a bit ugly, suggestions welcomed)
5. Try to access Koha without enabling 2FA, you shouldn't be able to
access any pages
6. Setup 2FA and confirm that you are redirected to the login screen
7. Login, send the correct pin code
=> You are fully logged in!

Note that at 6 we could redirect to the mainpage, without the need to
login again, but I think it's preferable to reduce the change to
C4::Auth. If it's considered mandatory by QA I could have a look on
another bug report.

Sponsored-by: Rijksmuseum, Netherlands
Comment 4 Jonathan Druart 2022-07-27 08:05:31 UTC
Created attachment 138163 [details] [review]
Bug 30588: Adjust existing selenium tests

Sponsored-by: Rijksmuseum, Netherlands
Comment 5 Jonathan Druart 2022-07-27 08:05:35 UTC
Created attachment 138164 [details] [review]
Bug 30588: Add tests for the REST API routes

Sponsored-by: Rijksmuseum, Netherlands
Comment 6 Jonathan Druart 2022-07-27 08:05:39 UTC
Created attachment 138165 [details] [review]
Bug 30588: Add selenium tests

Sponsored-by: Rijksmuseum, Netherlands
Comment 7 Jonathan Druart 2022-07-27 08:05:43 UTC
Created attachment 138166 [details] [review]
Bug 30588: Add tests for C4::checkauth

Sponsored-by: Rijksmuseum, Netherlands
Comment 8 David Cook 2022-07-28 01:23:26 UTC
Ohh I'm curious about this one. 

I wonder if "Bug 28980 - Add temporary passwords which must be reset after first login" would be able to make use of any hooks from this.
Comment 9 Jonathan Druart 2022-07-28 08:05:20 UTC
Created attachment 138191 [details] [review]
Bug 30588: Add missing atomic update
Comment 10 Jonathan Druart 2022-07-28 08:32:07 UTC
Created attachment 138193 [details] [review]
Bug 30588: Add missing include file
Comment 11 Martin Renvoize 2022-08-10 06:52:59 UTC
Sorry Jonathan, I get sha1 missing of useless problems when trying to apply this patch series :(.
Comment 12 Jonathan Druart 2022-08-10 08:24:21 UTC
Created attachment 138953 [details] [review]
Bug 30588: Add 'enforced' option to TwoFactorAuthentication

Sponsored-by: Rijksmuseum, Netherlands
Comment 13 Jonathan Druart 2022-08-10 08:24:25 UTC
Created attachment 138954 [details] [review]
Bug 30588: Adjust existing occurrences of TwoFactorAuthentication

We need to replace 0 with 'disabled', and 1 with 'enabled'

Sponsored-by: Rijksmuseum, Netherlands
Comment 14 Jonathan Druart 2022-08-10 08:24:30 UTC
Created attachment 138955 [details] [review]
Bug 30588: Add the option to require 2FA setup on first staff login

Bug 28786 added the ability to turn on a two-factor authentication,
using a One Time Password (OTP).
Once enabled on the system, librarian had the choice to enable or
disable it for themselves.
For security reason an administrator could decide to force the
librarians to use this second authentication step.

This patch adds a third option to the existing syspref, 'Enforced', for
that purpose.

QA notes: the code we had in the members/two_factor_auth.pl controller
has been moved to REST API controller methods (with their tests and
swagger specs), for reusability reason. Code from template has been
moved to an include file for the same reason.

Test plan:
A. Regression tests
As we modified the code we need first to confirm the existing features
are still working as expected.
1. Turn off TwoFactorAuthentication (disabled) and confirm that you are not able to
enable and access the second authentication step
2. Turn it on (enabled) and confirm that you are able to enable it in your account
3. Logout and confirm then that you are able to login into Koha

B. The new option
1. Set the pref to "enforced"
2. You are not logged out, logged in users stay logged in
3. Pick a user that does not have 2FA setup, login
4. Notice the new screen (UI is a bit ugly, suggestions welcomed)
5. Try to access Koha without enabling 2FA, you shouldn't be able to
access any pages
6. Setup 2FA and confirm that you are redirected to the login screen
7. Login, send the correct pin code
=> You are fully logged in!

Note that at 6 we could redirect to the mainpage, without the need to
login again, but I think it's preferable to reduce the change to
C4::Auth. If it's considered mandatory by QA I could have a look on
another bug report.

Sponsored-by: Rijksmuseum, Netherlands
Comment 15 Jonathan Druart 2022-08-10 08:24:35 UTC
Created attachment 138956 [details] [review]
Bug 30588: Adjust existing selenium tests

Sponsored-by: Rijksmuseum, Netherlands
Comment 16 Jonathan Druart 2022-08-10 08:24:39 UTC
Created attachment 138957 [details] [review]
Bug 30588: Add tests for the REST API routes

Sponsored-by: Rijksmuseum, Netherlands
Comment 17 Jonathan Druart 2022-08-10 08:24:44 UTC
Created attachment 138958 [details] [review]
Bug 30588: Add selenium tests

Sponsored-by: Rijksmuseum, Netherlands
Comment 18 Jonathan Druart 2022-08-10 08:24:48 UTC
Created attachment 138959 [details] [review]
Bug 30588: Add tests for C4::checkauth

Sponsored-by: Rijksmuseum, Netherlands
Comment 19 Jonathan Druart 2022-08-10 08:24:53 UTC
Created attachment 138960 [details] [review]
Bug 30588: Add missing atomic update
Comment 20 Jonathan Druart 2022-08-10 08:24:58 UTC
Created attachment 138961 [details] [review]
Bug 30588: Add missing include file
Comment 21 Jonathan Druart 2022-08-10 08:30:21 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2022-09-09 09:55:22 UTC
Created attachment 140365 [details] [review]
Bug 30588: (QA follow-up) Typos in sysprefs.sql

The codebase uses disabled and enforced which is imo better too.
So we need to fix this sql file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2022-09-09 09:55:26 UTC
Created attachment 140366 [details] [review]
Bug 30588: (QA follow-up) Missing shebang in TwoFactorAuth.t

Test plan:
Try t/db_dependent/Koha/Auth/TwoFactorAuth.t on cmd line.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2022-09-09 09:55:30 UTC
Created attachment 140367 [details] [review]
Bug 30588: (QA follow-up) Remove erroneous FIXME

Comment speaks for itself. Look at surrounding code.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2022-09-09 09:55:34 UTC
Created attachment 140368 [details] [review]
Bug 30588: (QA follow-up) Auth - remove two warns and second logout

Resolves:
Use of uninitialized value $request_method in string eq at /usr/share/koha/C4/Auth.pm line 1122.
Use of uninitialized value $return in numeric gt (>) at /usr/share/koha/C4/Auth.pm line 1155.

We also remove the double logout from Auth.t

Test plan:
Run t/db_dependent/Auth.t
Check if you do not see the warns anymore.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2022-09-09 09:55:37 UTC
Created attachment 140369 [details] [review]
Bug 30588: (QA follow-up) Adjust flags for two_factor_auth.pl

Anyone with staff access (catalogue) should be able to go
to account and enable 2FA.

Test plan:
Have a staff user with minimum staff permission.
Access account, manage 2FA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2022-09-09 09:57:41 UTC
Still testing actually. Having some issues when changing users in Enforced state. But not sure yet what's going on.

If you wanna test too, please do !
Comment 28 Marcel de Rooy 2022-09-12 09:14:41 UTC
(In reply to Marcel de Rooy from comment #27)
> Still testing actually. Having some issues when changing users in Enforced
> state. But not sure yet what's going on.
> 
> If you wanna test too, please do !

Found it. Adding a last follow-up.
Comment 29 Marcel de Rooy 2022-09-12 09:15:32 UTC
Created attachment 140416 [details] [review]
Bug 30588: Add 'enforced' option to TwoFactorAuthentication

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2022-09-12 09:15:36 UTC
Created attachment 140417 [details] [review]
Bug 30588: Adjust existing occurrences of TwoFactorAuthentication

We need to replace 0 with 'disabled', and 1 with 'enabled'

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2022-09-12 09:15:40 UTC
Created attachment 140418 [details] [review]
Bug 30588: Add the option to require 2FA setup on first staff login

Bug 28786 added the ability to turn on a two-factor authentication,
using a One Time Password (OTP).
Once enabled on the system, librarian had the choice to enable or
disable it for themselves.
For security reason an administrator could decide to force the
librarians to use this second authentication step.

This patch adds a third option to the existing syspref, 'Enforced', for
that purpose.

QA notes: the code we had in the members/two_factor_auth.pl controller
has been moved to REST API controller methods (with their tests and
swagger specs), for reusability reason. Code from template has been
moved to an include file for the same reason.

Test plan:
A. Regression tests
As we modified the code we need first to confirm the existing features
are still working as expected.
1. Turn off TwoFactorAuthentication (disabled) and confirm that you are not able to
enable and access the second authentication step
2. Turn it on (enabled) and confirm that you are able to enable it in your account
3. Logout and confirm then that you are able to login into Koha

B. The new option
1. Set the pref to "enforced"
2. You are not logged out, logged in users stay logged in
3. Pick a user that does not have 2FA setup, login
4. Notice the new screen (UI is a bit ugly, suggestions welcomed)
5. Try to access Koha without enabling 2FA, you shouldn't be able to
access any pages
6. Setup 2FA and confirm that you are redirected to the login screen
7. Login, send the correct pin code
=> You are fully logged in!

Note that at 6 we could redirect to the mainpage, without the need to
login again, but I think it's preferable to reduce the change to
C4::Auth. If it's considered mandatory by QA I could have a look on
another bug report.

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2022-09-12 09:15:44 UTC
Created attachment 140419 [details] [review]
Bug 30588: Adjust existing selenium tests

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2022-09-12 09:15:48 UTC
Created attachment 140420 [details] [review]
Bug 30588: Add tests for the REST API routes

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2022-09-12 09:15:54 UTC
Created attachment 140421 [details] [review]
Bug 30588: Add selenium tests

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2022-09-12 09:15:59 UTC
Created attachment 140422 [details] [review]
Bug 30588: Add tests for C4::checkauth

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2022-09-12 09:16:03 UTC
Created attachment 140423 [details] [review]
Bug 30588: Add missing atomic update

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2022-09-12 09:16:08 UTC
Created attachment 140424 [details] [review]
Bug 30588: Add missing include file

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2022-09-12 09:16:12 UTC
Created attachment 140425 [details] [review]
Bug 30588: (QA follow-up) Typos in sysprefs.sql

The codebase uses disabled and enforced which is imo better too.
So we need to fix this sql file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2022-09-12 09:16:16 UTC
Created attachment 140426 [details] [review]
Bug 30588: (QA follow-up) Missing shebang in TwoFactorAuth.t

Test plan:
Try t/db_dependent/Koha/Auth/TwoFactorAuth.t on cmd line.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Marcel de Rooy 2022-09-12 09:16:21 UTC
Created attachment 140427 [details] [review]
Bug 30588: (QA follow-up) Remove erroneous FIXME

Comment speaks for itself. Look at surrounding code.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2022-09-12 09:16:25 UTC
Created attachment 140428 [details] [review]
Bug 30588: (QA follow-up) Auth - remove two warns and second logout

Resolves:
Use of uninitialized value $request_method in string eq at /usr/share/koha/C4/Auth.pm line 1122.
Use of uninitialized value $return in numeric gt (>) at /usr/share/koha/C4/Auth.pm line 1155.

We also remove the double logout from Auth.t

Test plan:
Run t/db_dependent/Auth.t
Check if you do not see the warns anymore.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2022-09-12 09:16:29 UTC
Created attachment 140429 [details] [review]
Bug 30588: (QA follow-up) Adjust flags for two_factor_auth.pl

Anyone with staff access (catalogue) should be able to go
to account and enable 2FA.

Test plan:
Have a staff user with minimum staff permission.
Access account, manage 2FA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2022-09-12 09:16:34 UTC
Created attachment 140430 [details] [review]
Bug 30588: (QA follow-up) Clear waiting-for-2FA-setup in session

If we do not clear this session, the first login directly after setup
does not really enhances user experience ;)

Test plan:
Make sure 2FA is enforced.
Test the above. Disable your 2FA, logout and login.
Verify that you can access pages with this patch now. Without this
patch you could not.
Run these tests to provide more confidence:
t/db_dependent/Auth.t
t/db_dependent/api/v1/two_factor_auth.t
t/db_dependent/Koha/Auth/TwoFactorAuth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Jonathan Druart 2022-10-17 15:05:11 UTC
Created attachment 141978 [details] [review]
Bug 30588: Add 'enforced' option to TwoFactorAuthentication

Sponsored-by: Rijksmuseum, Netherlands
Comment 45 Jonathan Druart 2022-10-17 15:05:15 UTC
Created attachment 141979 [details] [review]
Bug 30588: Adjust existing occurrences of TwoFactorAuthentication

We need to replace 0 with 'disabled', and 1 with 'enabled'

Sponsored-by: Rijksmuseum, Netherlands
Comment 46 Jonathan Druart 2022-10-17 15:05:22 UTC
Created attachment 141980 [details] [review]
Bug 30588: Add the option to require 2FA setup on first staff login

Bug 28786 added the ability to turn on a two-factor authentication,
using a One Time Password (OTP).
Once enabled on the system, librarian had the choice to enable or
disable it for themselves.
For security reason an administrator could decide to force the
librarians to use this second authentication step.

This patch adds a third option to the existing syspref, 'Enforced', for
that purpose.

QA notes: the code we had in the members/two_factor_auth.pl controller
has been moved to REST API controller methods (with their tests and
swagger specs), for reusability reason. Code from template has been
moved to an include file for the same reason.

Test plan:
A. Regression tests
As we modified the code we need first to confirm the existing features
are still working as expected.
1. Turn off TwoFactorAuthentication (disabled) and confirm that you are not able to
enable and access the second authentication step
2. Turn it on (enabled) and confirm that you are able to enable it in your account
3. Logout and confirm then that you are able to login into Koha

B. The new option
1. Set the pref to "enforced"
2. You are not logged out, logged in users stay logged in
3. Pick a user that does not have 2FA setup, login
4. Notice the new screen (UI is a bit ugly, suggestions welcomed)
5. Try to access Koha without enabling 2FA, you shouldn't be able to
access any pages
6. Setup 2FA and confirm that you are redirected to the login screen
7. Login, send the correct pin code
=> You are fully logged in!

Note that at 6 we could redirect to the mainpage, without the need to
login again, but I think it's preferable to reduce the change to
C4::Auth. If it's considered mandatory by QA I could have a look on
another bug report.

Sponsored-by: Rijksmuseum, Netherlands
Comment 47 Jonathan Druart 2022-10-17 15:05:29 UTC
Created attachment 141981 [details] [review]
Bug 30588: Adjust existing selenium tests

Sponsored-by: Rijksmuseum, Netherlands
Comment 48 Jonathan Druart 2022-10-17 15:05:35 UTC
Created attachment 141982 [details] [review]
Bug 30588: Add tests for the REST API routes

Sponsored-by: Rijksmuseum, Netherlands
Comment 49 Jonathan Druart 2022-10-17 15:05:41 UTC
Created attachment 141983 [details] [review]
Bug 30588: Add selenium tests

Sponsored-by: Rijksmuseum, Netherlands
Comment 50 Jonathan Druart 2022-10-17 15:05:47 UTC
Created attachment 141984 [details] [review]
Bug 30588: Add tests for C4::checkauth

Sponsored-by: Rijksmuseum, Netherlands
Comment 51 Jonathan Druart 2022-10-17 15:05:53 UTC
Created attachment 141985 [details] [review]
Bug 30588: Add missing atomic update
Comment 52 Jonathan Druart 2022-10-17 15:05:58 UTC
Created attachment 141986 [details] [review]
Bug 30588: Add missing include file
Comment 53 Jonathan Druart 2022-10-17 15:06:05 UTC
Created attachment 141987 [details] [review]
Bug 30588: (QA follow-up) Typos in sysprefs.sql

The codebase uses disabled and enforced which is imo better too.
So we need to fix this sql file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Jonathan Druart 2022-10-17 15:06:11 UTC
Created attachment 141988 [details] [review]
Bug 30588: (QA follow-up) Missing shebang in TwoFactorAuth.t

Test plan:
Try t/db_dependent/Koha/Auth/TwoFactorAuth.t on cmd line.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Jonathan Druart 2022-10-17 15:06:17 UTC
Created attachment 141989 [details] [review]
Bug 30588: (QA follow-up) Remove erroneous FIXME

Comment speaks for itself. Look at surrounding code.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 56 Jonathan Druart 2022-10-17 15:06:22 UTC
Created attachment 141990 [details] [review]
Bug 30588: (QA follow-up) Auth - remove two warns and second logout

Resolves:
Use of uninitialized value $request_method in string eq at /usr/share/koha/C4/Auth.pm line 1122.
Use of uninitialized value $return in numeric gt (>) at /usr/share/koha/C4/Auth.pm line 1155.

We also remove the double logout from Auth.t

Test plan:
Run t/db_dependent/Auth.t
Check if you do not see the warns anymore.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 57 Jonathan Druart 2022-10-17 15:06:27 UTC
Created attachment 141991 [details] [review]
Bug 30588: (QA follow-up) Adjust flags for two_factor_auth.pl

Anyone with staff access (catalogue) should be able to go
to account and enable 2FA.

Test plan:
Have a staff user with minimum staff permission.
Access account, manage 2FA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 58 Jonathan Druart 2022-10-17 15:06:33 UTC
Created attachment 141992 [details] [review]
Bug 30588: (QA follow-up) Clear waiting-for-2FA-setup in session

If we do not clear this session, the first login directly after setup
does not really enhances user experience ;)

Test plan:
Make sure 2FA is enforced.
Test the above. Disable your 2FA, logout and login.
Verify that you can access pages with this patch now. Without this
patch you could not.
Run these tests to provide more confidence:
t/db_dependent/Auth.t
t/db_dependent/api/v1/two_factor_auth.t
t/db_dependent/Koha/Auth/TwoFactorAuth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 59 Jonathan Druart 2022-10-17 15:06:51 UTC
Just a rebase.
Comment 60 Nick Clemens 2022-10-21 12:27:11 UTC
Created attachment 142344 [details] [review]
Bug 30588: Add 'enforced' option to TwoFactorAuthentication

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 61 Nick Clemens 2022-10-21 12:27:15 UTC
Created attachment 142345 [details] [review]
Bug 30588: Adjust existing occurrences of TwoFactorAuthentication

We need to replace 0 with 'disabled', and 1 with 'enabled'

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 62 Nick Clemens 2022-10-21 12:27:20 UTC
Created attachment 142346 [details] [review]
Bug 30588: Add the option to require 2FA setup on first staff login

Bug 28786 added the ability to turn on a two-factor authentication,
using a One Time Password (OTP).
Once enabled on the system, librarian had the choice to enable or
disable it for themselves.
For security reason an administrator could decide to force the
librarians to use this second authentication step.

This patch adds a third option to the existing syspref, 'Enforced', for
that purpose.

QA notes: the code we had in the members/two_factor_auth.pl controller
has been moved to REST API controller methods (with their tests and
swagger specs), for reusability reason. Code from template has been
moved to an include file for the same reason.

Test plan:
A. Regression tests
As we modified the code we need first to confirm the existing features
are still working as expected.
1. Turn off TwoFactorAuthentication (disabled) and confirm that you are not able to
enable and access the second authentication step
2. Turn it on (enabled) and confirm that you are able to enable it in your account
3. Logout and confirm then that you are able to login into Koha

B. The new option
1. Set the pref to "enforced"
2. You are not logged out, logged in users stay logged in
3. Pick a user that does not have 2FA setup, login
4. Notice the new screen (UI is a bit ugly, suggestions welcomed)
5. Try to access Koha without enabling 2FA, you shouldn't be able to
access any pages
6. Setup 2FA and confirm that you are redirected to the login screen
7. Login, send the correct pin code
=> You are fully logged in!

Note that at 6 we could redirect to the mainpage, without the need to
login again, but I think it's preferable to reduce the change to
C4::Auth. If it's considered mandatory by QA I could have a look on
another bug report.

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 63 Nick Clemens 2022-10-21 12:27:26 UTC
Created attachment 142347 [details] [review]
Bug 30588: Adjust existing selenium tests

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 64 Nick Clemens 2022-10-21 12:27:30 UTC
Created attachment 142348 [details] [review]
Bug 30588: Add tests for the REST API routes

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 65 Nick Clemens 2022-10-21 12:27:34 UTC
Created attachment 142349 [details] [review]
Bug 30588: Add selenium tests

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 66 Nick Clemens 2022-10-21 12:27:39 UTC
Created attachment 142350 [details] [review]
Bug 30588: Add tests for C4::checkauth

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 67 Nick Clemens 2022-10-21 12:27:48 UTC
Created attachment 142351 [details] [review]
Bug 30588: Add missing atomic update

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 68 Nick Clemens 2022-10-21 12:28:21 UTC
Created attachment 142352 [details] [review]
Bug 30588: Add missing include file

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 69 Nick Clemens 2022-10-21 12:28:26 UTC
Created attachment 142353 [details] [review]
Bug 30588: (QA follow-up) Typos in sysprefs.sql

The codebase uses disabled and enforced which is imo better too.
So we need to fix this sql file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 70 Nick Clemens 2022-10-21 12:28:40 UTC
Created attachment 142354 [details] [review]
Bug 30588: (QA follow-up) Missing shebang in TwoFactorAuth.t

Test plan:
Try t/db_dependent/Koha/Auth/TwoFactorAuth.t on cmd line.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 71 Nick Clemens 2022-10-21 12:28:45 UTC
Created attachment 142355 [details] [review]
Bug 30588: (QA follow-up) Remove erroneous FIXME

Comment speaks for itself. Look at surrounding code.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 72 Nick Clemens 2022-10-21 12:28:49 UTC
Created attachment 142356 [details] [review]
Bug 30588: (QA follow-up) Auth - remove two warns and second logout

Resolves:
Use of uninitialized value $request_method in string eq at /usr/share/koha/C4/Auth.pm line 1122.
Use of uninitialized value $return in numeric gt (>) at /usr/share/koha/C4/Auth.pm line 1155.

We also remove the double logout from Auth.t

Test plan:
Run t/db_dependent/Auth.t
Check if you do not see the warns anymore.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 73 Nick Clemens 2022-10-21 12:28:54 UTC
Created attachment 142357 [details] [review]
Bug 30588: (QA follow-up) Adjust flags for two_factor_auth.pl

Anyone with staff access (catalogue) should be able to go
to account and enable 2FA.

Test plan:
Have a staff user with minimum staff permission.
Access account, manage 2FA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 74 Nick Clemens 2022-10-21 12:28:59 UTC
Created attachment 142358 [details] [review]
Bug 30588: (QA follow-up) Clear waiting-for-2FA-setup in session

If we do not clear this session, the first login directly after setup
does not really enhances user experience ;)

Test plan:
Make sure 2FA is enforced.
Test the above. Disable your 2FA, logout and login.
Verify that you can access pages with this patch now. Without this
patch you could not.
Run these tests to provide more confidence:
t/db_dependent/Auth.t
t/db_dependent/api/v1/two_factor_auth.t
t/db_dependent/Koha/Auth/TwoFactorAuth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 75 Nick Clemens 2022-10-21 12:32:30 UTC
This all works nicely, minor requests:
When a user is configuring 2FA and it is enforced, it would be nice to have a message telling them they must set this up - yes, it doesn't let you in without, but still nice to say so

I wondered at first why a user can disable 2FA when it is enforced, but I suppose it is so they can setup a new authentication method? It should also state when enforced on this page, so the user knows they cannot truly disable

Neither blockers, moving forward for RM review
Comment 76 Tomás Cohen Arazi 2022-10-21 14:46:39 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 77 Nick Clemens 2022-12-07 17:00:46 UTC
*** Bug 32404 has been marked as a duplicate of this bug. ***