To recreate: 1. Set the syspref TwoFactorAuthentication (enforce or enabled) 2. Configure 2FA for a patron 3. Logout 4. Authenticate but don't enter the 2FA code 5. Switch off the syspref (disabled) => Patron is stuck on the login screen (actually can access Same can happens when the patron needs to setup 2FA.
This change should fix the problem. diff --git a/C4/Auth.pm b/C4/Auth.pm index b6933e4cc60..8bd13e88e1f 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1784,11 +1784,13 @@ sub check_cookie_auth { $session->param('desk_id'), $session->param('desk_name'), $session->param('register_id'), $session->param('register_name') ); - return ( "additional-auth-needed", $session ) - if $session->param('waiting-for-2FA'); + if ( C4::Context->preference('TwoFactorAuthentication') ne 'disabled' ) { + return ( "additional-auth-needed", $session ) + if $session->param('waiting-for-2FA'); . - return ( "setup-additional-auth-needed", $session ) - if $session->param('waiting-for-2FA-setup'); + return ( "setup-additional-auth-needed", $session ) + if $session->param('waiting-for-2FA-setup'); + } . return ( "ok", $session ); } else {
(In reply to Jonathan Druart from comment #1) > This change should fix the problem. > > diff --git a/C4/Auth.pm b/C4/Auth.pm > index b6933e4cc60..8bd13e88e1f 100644 > --- a/C4/Auth.pm > +++ b/C4/Auth.pm > @@ -1784,11 +1784,13 @@ sub check_cookie_auth { > $session->param('desk_id'), > $session->param('desk_name'), > $session->param('register_id'), > $session->param('register_name') > ); > - return ( "additional-auth-needed", $session ) > - if $session->param('waiting-for-2FA'); > + if ( C4::Context->preference('TwoFactorAuthentication') ne > 'disabled' ) { > + return ( "additional-auth-needed", $session ) > + if $session->param('waiting-for-2FA'); > . > - return ( "setup-additional-auth-needed", $session ) > - if $session->param('waiting-for-2FA-setup'); > + return ( "setup-additional-auth-needed", $session ) > + if $session->param('waiting-for-2FA-setup'); > + } > . > return ( "ok", $session ); > } else { Note that this diff contains a few strange dots that block applying it directly. No problem. I will have a look now.
Created attachment 143066 [details] [review] Bug 32066: Check 2FA pref in check_cookie_auth Test plan: Without this patch: 1. Set the syspref TwoFactorAuthentication (enforce or enabled) 2. Configure 2FA for a patron 3. Logout 4. Authenticate but don't enter the 2FA code 5. Switch off the syspref (disabled) [via another browser or so] 6. Patron is stuck on the [original] login screen. [Only removing the session cookie would resolve it.] With this patch: 1. Follow the steps above again. But note that you can refresh your browser window to get in now. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I will still have a look at unit testing now.
Created attachment 143067 [details] [review] Bug 32066: Check 2FA pref in check_cookie_auth Test plan: Without this patch: 1. Set the syspref TwoFactorAuthentication (enforce or enabled) 2. Configure 2FA for a patron 3. Logout 4. Authenticate but don't enter the 2FA code 5. Switch off the syspref (disabled) [via another browser or so] 6. Patron is stuck on the [original] login screen. [Only removing the session cookie would resolve it.] With this patch: 1. Follow the steps above again. But note that you can refresh your browser window to get in now. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #1) > - return ( "setup-additional-auth-needed", $session ) > - if $session->param('waiting-for-2FA-setup'); > + return ( "setup-additional-auth-needed", $session ) > + if $session->param('waiting-for-2FA-setup'); > + } I am still thinking about this second case btw. Not sure yet.
(In reply to Marcel de Rooy from comment #6) > (In reply to Jonathan Druart from comment #1) > > - return ( "setup-additional-auth-needed", $session ) > > - if $session->param('waiting-for-2FA-setup'); > > + return ( "setup-additional-auth-needed", $session ) > > + if $session->param('waiting-for-2FA-setup'); > > + } > > I am still thinking about this second case btw. Not sure yet. The change actually seems to be unneeded. But it is hard to catch why checkauth allows the login if cookie_auth returned: [2022/11/03 10:20:14] [WARN] L853:setup-additional-auth-needed: at /usr/share/koha/C4/Auth.pm line 853.
(In reply to Marcel de Rooy from comment #7) > (In reply to Marcel de Rooy from comment #6) > > (In reply to Jonathan Druart from comment #1) > > > - return ( "setup-additional-auth-needed", $session ) > > > - if $session->param('waiting-for-2FA-setup'); > > > + return ( "setup-additional-auth-needed", $session ) > > > + if $session->param('waiting-for-2FA-setup'); > > > + } > > > > I am still thinking about this second case btw. Not sure yet. > > The change actually seems to be unneeded. But it is hard to catch why > checkauth allows the login if cookie_auth returned: > [2022/11/03 10:20:14] [WARN] L853:setup-additional-auth-needed: at > /usr/share/koha/C4/Auth.pm line 853. Hmm. What happens is that the q_userid and password are checked again with checkpw and are fine. So login is granted. It seems that checkauth might need more attention in responding to the setup-additional-auth-needed response of cookie_auth?
Created attachment 143071 [details] [review] Bug 32066: Add unit test to Auth.t Test plan: Run without next patch. Should fail. Run with next patch. Should pass. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 143072 [details] [review] Bug 32066: Check 2FA pref in check_cookie_auth Test plan: Without this patch: 1. Set the syspref TwoFactorAuthentication (enforce or enabled) 2. Configure 2FA for a patron 3. Logout 4. Authenticate but don't enter the 2FA code 5. Switch off the syspref (disabled) [via another browser or so] 6. Patron is stuck on the [original] login screen. [Only removing the session cookie would resolve it.] With this patch: 1. Follow the steps above again. But note that you can refresh your browser window to get in now. 2. Verify that Auth.t passes now too. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
See my earlier remarks about checkauth and the setup flag. But I agree with this patch. Further refinements might be possible on follow-up reports.
Created attachment 143263 [details] [review] Bug 32066: Add unit test to Auth.t Test plan: Run without next patch. Should fail. Run with next patch. Should pass. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 143264 [details] [review] Bug 32066: Add unit test to Auth.t Test plan: Run without next patch. Should fail. Run with next patch. Should pass. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 143265 [details] [review] Bug 32066: Check 2FA pref in check_cookie_auth Test plan: Without this patch: 1. Set the syspref TwoFactorAuthentication (enforce or enabled) 2. Configure 2FA for a patron 3. Logout 4. Authenticate but don't enter the 2FA code 5. Switch off the syspref (disabled) [via another browser or so] 6. Patron is stuck on the [original] login screen. [Only removing the session cookie would resolve it.] With this patch: 1. Follow the steps above again. But note that you can refresh your browser window to get in now. 2. Verify that Auth.t passes now too. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 22.11. Nice work everyone, thanks!