Summary: | 2FA: User could get stuck temporarily on login screen when disabling pref | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Authentication | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Kyle M Hall (khall) <kyle> |
Severity: | normal | ||
Priority: | P5 - low | CC: | dcook, dpavlin, kyle, m.de.rooy |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
22.11.00
|
|
Circulation function: | |||
Bug Depends on: | 30588 | ||
Bug Blocks: | |||
Attachments: |
Bug 32066: Check 2FA pref in check_cookie_auth
Bug 32066: Check 2FA pref in check_cookie_auth Bug 32066: Add unit test to Auth.t Bug 32066: Check 2FA pref in check_cookie_auth Bug 32066: Add unit test to Auth.t Bug 32066: Add unit test to Auth.t Bug 32066: Check 2FA pref in check_cookie_auth |
Description
Jonathan Druart
2022-11-02 09:35:18 UTC
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! |