Bug 32066 - 2FA: User could get stuck temporarily on login screen when disabling pref
Summary: 2FA: User could get stuck temporarily on login screen when disabling pref
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 30588
Blocks:
  Show dependency treegraph
 
Reported: 2022-11-02 09:35 UTC by Jonathan Druart
Modified: 2023-06-08 22:28 UTC (History)
4 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.11.00


Attachments
Bug 32066: Check 2FA pref in check_cookie_auth (1.89 KB, patch)
2022-11-03 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32066: Check 2FA pref in check_cookie_auth (1.91 KB, patch)
2022-11-03 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32066: Add unit test to Auth.t (1.86 KB, patch)
2022-11-03 10:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32066: Check 2FA pref in check_cookie_auth (1.95 KB, patch)
2022-11-03 10:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32066: Add unit test to Auth.t (1.88 KB, patch)
2022-11-04 18:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32066: Add unit test to Auth.t (1.88 KB, patch)
2022-11-04 18:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32066: Check 2FA pref in check_cookie_auth (1.96 KB, patch)
2022-11-04 18:03 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-11-02 09:35:18 UTC
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.
Comment 1 Jonathan Druart 2022-11-02 09:35:59 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 {
Comment 2 Marcel de Rooy 2022-11-03 07:43:21 UTC
(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.
Comment 3 Marcel de Rooy 2022-11-03 08:16:25 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2022-11-03 08:18:24 UTC
I will still have a look at unit testing now.
Comment 5 Marcel de Rooy 2022-11-03 08:20:18 UTC
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>
Comment 6 Marcel de Rooy 2022-11-03 09:01:31 UTC
(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.
Comment 7 Marcel de Rooy 2022-11-03 10:29:28 UTC
(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.
Comment 8 Marcel de Rooy 2022-11-03 10:52:14 UTC
(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?
Comment 9 Marcel de Rooy 2022-11-03 10:57:56 UTC
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>
Comment 10 Marcel de Rooy 2022-11-03 10:58:00 UTC
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>
Comment 11 Marcel de Rooy 2022-11-03 10:59:21 UTC
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.
Comment 12 Kyle M Hall 2022-11-04 18:00:49 UTC
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>
Comment 13 Kyle M Hall 2022-11-04 18:02:49 UTC
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>
Comment 14 Kyle M Hall 2022-11-04 18:03:01 UTC
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>
Comment 15 Tomás Cohen Arazi 2022-11-04 22:14:58 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!