We should add the option to require 2FA configuration at first login for staff users when 2FA is enabled on the system.
Created attachment 138160 [details] [review] Bug 30588: Add 'enforced' option to TwoFactorAuthentication Sponsored-by: Rijksmuseum, Netherlands
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
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
Created attachment 138163 [details] [review] Bug 30588: Adjust existing selenium tests Sponsored-by: Rijksmuseum, Netherlands
Created attachment 138164 [details] [review] Bug 30588: Add tests for the REST API routes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 138165 [details] [review] Bug 30588: Add selenium tests Sponsored-by: Rijksmuseum, Netherlands
Created attachment 138166 [details] [review] Bug 30588: Add tests for C4::checkauth Sponsored-by: Rijksmuseum, Netherlands
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.
Created attachment 138191 [details] [review] Bug 30588: Add missing atomic update
Created attachment 138193 [details] [review] Bug 30588: Add missing include file
Sorry Jonathan, I get sha1 missing of useless problems when trying to apply this patch series :(.
Created attachment 138953 [details] [review] Bug 30588: Add 'enforced' option to TwoFactorAuthentication Sponsored-by: Rijksmuseum, Netherlands
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
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
Created attachment 138956 [details] [review] Bug 30588: Adjust existing selenium tests Sponsored-by: Rijksmuseum, Netherlands
Created attachment 138957 [details] [review] Bug 30588: Add tests for the REST API routes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 138958 [details] [review] Bug 30588: Add selenium tests Sponsored-by: Rijksmuseum, Netherlands
Created attachment 138959 [details] [review] Bug 30588: Add tests for C4::checkauth Sponsored-by: Rijksmuseum, Netherlands
Created attachment 138960 [details] [review] Bug 30588: Add missing atomic update
Created attachment 138961 [details] [review] Bug 30588: Add missing include file
Created attachment 138962 [details] [review] Bug 30588: FIX serial claims filters
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>
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>
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>
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>
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>
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 !
(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.
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>
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>
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>
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>
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>
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>
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>
Created attachment 140423 [details] [review] Bug 30588: Add missing atomic update Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 140424 [details] [review] Bug 30588: Add missing include file Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
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>
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>
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>
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>
Created attachment 141978 [details] [review] Bug 30588: Add 'enforced' option to TwoFactorAuthentication Sponsored-by: Rijksmuseum, Netherlands
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
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
Created attachment 141981 [details] [review] Bug 30588: Adjust existing selenium tests Sponsored-by: Rijksmuseum, Netherlands
Created attachment 141982 [details] [review] Bug 30588: Add tests for the REST API routes Sponsored-by: Rijksmuseum, Netherlands
Created attachment 141983 [details] [review] Bug 30588: Add selenium tests Sponsored-by: Rijksmuseum, Netherlands
Created attachment 141984 [details] [review] Bug 30588: Add tests for C4::checkauth Sponsored-by: Rijksmuseum, Netherlands
Created attachment 141985 [details] [review] Bug 30588: Add missing atomic update
Created attachment 141986 [details] [review] Bug 30588: Add missing include file
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>
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>
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>
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>
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>
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>
Just a rebase.
Created attachment 142344 [details] [review] Bug 30588: Add 'enforced' option to TwoFactorAuthentication Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
Created attachment 142347 [details] [review] Bug 30588: Adjust existing selenium tests Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
Created attachment 142349 [details] [review] Bug 30588: Add selenium tests Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 142350 [details] [review] Bug 30588: Add tests for C4::checkauth Sponsored-by: Rijksmuseum, Netherlands Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 142351 [details] [review] Bug 30588: Add missing atomic update Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 142352 [details] [review] Bug 30588: Add missing include file Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
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>
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>
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>
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>
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
Pushed to master for 22.11. Nice work everyone, thanks!
*** Bug 32404 has been marked as a duplicate of this bug. ***