Bug 28787

Summary: Send a notice with the TOTP token
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: AuthenticationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: enhancement    
Priority: P5 - low CC: caroline.cyr-la-rose, dcook, dpavlin, kyle, lucas, m.de.rooy, martin.renvoize, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30842
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31119
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Caroline Cyr La Rose Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/632
Text to go in the release notes:
Add the ability to send an email containing the token to the patron once it's authenticated The new notice template is '2FA_OTP_TOKEN'.
Version(s) released in:
22.11.00
Bug Depends on: 28786    
Bug Blocks: 20476, 31118, 30588, 31243    
Attachments: Bug 28787: Send a notice with the TOTP token
Bug 28787: Send a notice with the TOTP token
Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters
Bug 28787: Typo authenticaction
Bug 28787: Fix t/db_dependent/api/v1/two_factor_auth.t
Bug 28787: Rename the REST API route to /auth/otp/token_delivery
Bug 28787: Don't send the notice if we are not waiting for 2FA
Bug 28787: Mock send_or_die
Bug 28787: Comment selenium test that will fail if no SMTP server is defined
Bug 28787: Don't send the notice if we are not waiting for 2FA
Bug 28787: Don't request a token if no email address defined
Bug 28787: Mock send_or_die
Bug 28787: Comment selenium test that will fail if no SMTP server is defined
Bug 28787: Send a notice with the TOTP token
Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters
Bug 28787: Typo authenticaction
Bug 28787: Fix t/db_dependent/api/v1/two_factor_auth.t
Bug 28787: Rename the REST API route to /auth/otp/token_delivery
Bug 28787: Don't send the notice if we are not waiting for 2FA
Bug 28787: Don't request a token if no email address defined
Bug 28787: Mock send_or_die
Bug 28787: Comment selenium test that will fail if no SMTP server is defined
Bug 28787: (follow-up) Typo and additional test
Bug 28787: Fix misleading tests in two_factor_auth.t
Bug 28787: Send a notice with the TOTP token
Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters
Bug 28787: Typo authenticaction
Bug 28787: Fix t/db_dependent/api/v1/two_factor_auth.t
Bug 28787: Rename the REST API route to /auth/otp/token_delivery
Bug 28787: Don't send the notice if we are not waiting for 2FA
Bug 28787: Don't request a token if no email address defined
Bug 28787: Mock send_or_die
Bug 28787: Comment selenium test that will fail if no SMTP server is defined
Bug 28787: (follow-up) Typo and additional test
Bug 28787: Fix misleading tests in two_factor_auth.t
Bug 28787: (QA follow-up) Remove unused variable
Bug 28787: Send a notice with the TOTP token
Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters
Bug 28787: Typo authenticaction
Bug 28787: Fix t/db_dependent/api/v1/two_factor_auth.t
Bug 28787: Rename the REST API route to /auth/otp/token_delivery
Bug 28787: Don't send the notice if we are not waiting for 2FA
Bug 28787: Don't request a token if no email address defined
Bug 28787: Mock send_or_die
Bug 28787: Comment selenium test that will fail if no SMTP server is defined
Bug 28787: (follow-up) Typo and additional test
Bug 28787: Fix misleading tests in two_factor_auth.t
Bug 28787: (QA follow-up) Remove unused variable
Bug 28787: (QA follow-up) Add spec description
Bug 28787: (QA follow-up) Add spec description

Description Jonathan Druart 2021-07-30 12:05:21 UTC
Bug 28786 will let librarians enable a Two-factor authentication but will force them to use an application to generate the TOTP token.
It would be nice to have the ability to send a notice with the code, via email or SMS.
Comment 1 Jonathan Druart 2022-04-08 07:16:17 UTC
Will start this once bug 28786 is pushed.
Comment 2 Jonathan Druart 2022-05-23 21:04:20 UTC
Created attachment 135287 [details] [review]
Bug 28787: Send a notice with the TOTP token

Bug 28786 let librarians enable a Two-factor authentication but force them to use
an application to generate the TOTP token.

This new enhancement add the ability to send an email containing the token to the
patron once it's authenticaed

The new notice template has the code '2FA_OTP_TOKEN'

Test plan:
- Setup the two-factor authentication (you need the config entry and the
syspref ON)
- Enable it for your logged in patron
- Logout
- Login and notice the new link "Send the code by email"
- Click on it and confirm that you received an email with the code
- Use the code to be fully logged in

QA question: Is 400 the correct error code to tell the email has not
been sent?
Comment 3 Jonathan Druart 2022-05-23 21:07:48 UTC
TODO - I am not sure about the following line, so I commented it but let it in the patch
+            #|| $c->req->url->to_abs->path eq '/api/v1/auth/send_otp_token' ) {
Comment 4 Martin Renvoize 2022-05-24 14:50:05 UTC
Hmm, I'm not so sure about this.. whilst I understand TOTP over SMS delivery makes sense as SMS is in theory immediate delivery.. Email has lots of caveats around delivery speed and so it's more common to send an HOTP or even a simple random string OTP in the email case due to the timeout factor?
Comment 5 David Cook 2022-05-25 00:01:47 UTC
Comment on attachment 135287 [details] [review]
Bug 28787: Send a notice with the TOTP token

Review of attachment 135287 [details] [review]:
-----------------------------------------------------------------

::: C4/Letters.pm
@@ +1605,5 @@
>  
> +    if ( $content =~ m|\[% otp_token %\]| ) {
> +        my $patron = Koha::Patrons->find(C4::Context->userenv->{number});
> +        $tt_params->{otp_token} = Koha::Auth::TwoFactorAuth->new({patron => $patron})->code;
> +    }

This looks like a hack. We should pass the code in via a public method/function. That said, it looks like this OTP will wind up in the message_queue table?
Comment 6 David Cook 2022-05-25 02:23:34 UTC
(In reply to Martin Renvoize from comment #4)
> Hmm, I'm not so sure about this.. whilst I understand TOTP over SMS delivery
> makes sense as SMS is in theory immediate delivery.. Email has lots of
> caveats around delivery speed and so it's more common to send an HOTP or
> even a simple random string OTP in the email case due to the timeout factor?

I agree that a 30 second time window is probably too short for email. 

I suppose alternatively you could set a longer interval when using email TOTPs. (I did a little bit of a deep dive into Auth::GoogleAuth and it's actually kind of interesting how simple the mathematical mechanism is for establishing time windows for TOTPs.)

Another thing we could do is add the range parameter to the verify() function I believe. At the moment, it looks like we're not following the recommendations of rfc6238 to allow additional backwards steps. (Typically, with a TOTP, you can usually use up to 2-3 old codes and still work to allow for clock drift and slow users.)
Comment 7 Jonathan Druart 2022-05-25 08:00:58 UTC
(In reply to Martin Renvoize from comment #4)
> Hmm, I'm not so sure about this.. whilst I understand TOTP over SMS delivery
> makes sense as SMS is in theory immediate delivery.. Email has lots of
> caveats around delivery speed and so it's more common to send an HOTP or
> even a simple random string OTP in the email case due to the timeout factor?

Well, the description was clear enough and the bug has been flagged as sponsored. I don't think it's fair to ask for a rewrite once the code has been written.

(In reply to David Cook from comment #5)
> Comment on attachment 135287 [details] [review] [review]
> > +    if ( $content =~ m|\[% otp_token %\]| ) {
> > +        my $patron = Koha::Patrons->find(C4::Context->userenv->{number});
> > +        $tt_params->{otp_token} = Koha::Auth::TwoFactorAuth->new({patron => $patron})->code;
> > +    }
> 
> This looks like a hack. We should pass the code in via a public
> method/function. That said, it looks like this OTP will wind up in the
> message_queue table?

Hum yes, maybe. It seemed weird to add a pattern/variable that would be available for a single notice template.

(In reply to David Cook from comment #6)
> Another thing we could do is add the range parameter to the verify()
> function I believe. At the moment, it looks like we're not following the
> recommendations of rfc6238 to allow additional backwards steps. (Typically,
> with a TOTP, you can usually use up to 2-3 old codes and still work to allow
> for clock drift and slow users.)

Yes, that's a bug. I was pretty sure it was allowing at least 1 old code.
It's in the POD of ->verify, and members/two_factor_auth.pl, but C4/Auth.pm
Comment 8 Jonathan Druart 2022-05-25 08:07:42 UTC
> (In reply to David Cook from comment #6)
> > Another thing we could do is add the range parameter to the verify()
> > function I believe. At the moment, it looks like we're not following the
> > recommendations of rfc6238 to allow additional backwards steps. (Typically,
> > with a TOTP, you can usually use up to 2-3 old codes and still work to allow
> > for clock drift and slow users.)
> 
> Yes, that's a bug. I was pretty sure it was allowing at least 1 old code.
> It's in the POD of ->verify, and members/two_factor_auth.pl, but C4/Auth.pm

Fixed on bug 30842.
Comment 9 Marcel de Rooy 2022-05-25 11:20:27 UTC
(In reply to David Cook from comment #5)

> This looks like a hack. We should pass the code in via a public
> method/function. That said, it looks like this OTP will wind up in the
> message_queue table?

How vulnerable is that? Surely, the token will be expired very quickly but can we get back to the originating secret? And that said, would an attack on the email not have a higher chance of success ?

https://security.stackexchange.com/questions/42671/is-oath-totp-and-or-google-authenticator-vulnerable-if-an-attacker-has-n-pre
Comment 10 David Cook 2022-05-26 00:08:01 UTC
(In reply to Jonathan Druart from comment #7)
> Well, the description was clear enough and the bug has been flagged as
> sponsored. I don't think it's fair to ask for a rewrite once the code has
> been written.
 
In that case, I should mark more bugs as sponsored heh. 

(In reply to Jonathan Druart from comment #8)
> > Yes, that's a bug. I was pretty sure it was allowing at least 1 old code.
> > It's in the POD of ->verify, and members/two_factor_auth.pl, but C4/Auth.pm
> 
> Fixed on bug 30842.

Between bug 30842 and bug 30843, that might be enough to cover off the delivery delay of email.
Comment 11 David Cook 2022-05-26 00:23:38 UTC
(In reply to Marcel de Rooy from comment #9)
> (In reply to David Cook from comment #5)
> 
> > This looks like a hack. We should pass the code in via a public
> > method/function. That said, it looks like this OTP will wind up in the
> > message_queue table?
> 
> How vulnerable is that? Surely, the token will be expired very quickly but
> can we get back to the originating secret? And that said, would an attack on
> the email not have a higher chance of success ?
> 
> https://security.stackexchange.com/questions/42671/is-oath-totp-and-or-
> google-authenticator-vulnerable-if-an-attacker-has-n-pre

I'm not an expert on the topic, but in theory you could try an offline brute force attack that could potentially reveal the secret eventually, although I imagine we're using complex enough secrets that it would probably be computationally improbable at this time. 

Technically, I suppose we could encrypt the email contents at rest (like https://docs.aws.amazon.com/sns/latest/dg/sns-server-side-encryption.html), but I think the risk is small enough that can be a future enhancement...
Comment 12 David Cook 2022-06-08 03:04:57 UTC
(In reply to Martin Renvoize from comment #4)
> Hmm, I'm not so sure about this.. whilst I understand TOTP over SMS delivery
> makes sense as SMS is in theory immediate delivery.. Email has lots of
> caveats around delivery speed and so it's more common to send an HOTP or
> even a simple random string OTP in the email case due to the timeout factor?

I just signed up for a new system that uses OTP over email and it looks like they've set their code expiry to 5 minutes. (They tell the user in the email of that expiration time.)
Comment 13 Marcel de Rooy 2022-06-27 11:52:17 UTC
Created attachment 136571 [details] [review]
Bug 28787: Send a notice with the TOTP token

Bug 28786 let librarians enable a Two-factor authentication but force them to use
an application to generate the TOTP token.

This new enhancement add the ability to send an email containing the token to the
patron once it's authenticaed

The new notice template has the code '2FA_OTP_TOKEN'

Test plan:
- Setup the two-factor authentication (you need the config entry and the
syspref ON)
- Enable it for your logged in patron
- Logout
- Login and notice the new link "Send the code by email"
- Click on it and confirm that you received an email with the code
- Use the code to be fully logged in

QA question: Is 400 the correct error code to tell the email has not
been sent?
Comment 14 Marcel de Rooy 2022-06-27 11:52:21 UTC
Created attachment 136572 [details] [review]
Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters
Comment 15 Marcel de Rooy 2022-06-27 11:52:25 UTC
Created attachment 136573 [details] [review]
Bug 28787: Typo authenticaction

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2022-06-27 11:59:47 UTC
Generally, I have some doubts about the API path api/v1/auth/send_otp_token. Sending a token is not a normally expected API action; it sounds like a 'misused verb'. You could think of creating a OTP code as an API action, although we do not really add it as entity.
Apart from that it works. See some details hereunder.

[1] Your TODO - I am not sure about the following line, so I commented it but 
let it in the patch
+            #|| $c->req->url->to_abs->path eq '/api/v1/auth/send_otp_token' ) {
The otp path should go thru the chain. So this line should not be here although commented. Removed it.

[2] Code segment from Koha/REST/V1/Auth.pm
    if ( !$authorization and
         ( $params->{is_public} and
          ( C4::Context->preference('RESTPublicAnonymousRequests') or
            $user) or $params->{is_plugin} )
        or $pending_auth
This does not look good to me. Do we need pending_auth here ? If so, at least we need parentheses etc. My follow-up removes the line now.

[3] This segment is incomplete:
        elsif ($status eq "additional-auth-needed") {
            if ( $c->req->url->to_abs->path eq '/api/v1/auth/send_otp_token' ) {
                $user = Koha::Patrons->find( $session->param('number') );
                $cookie_auth = 1;
                $pending_auth = 1;
            }
I think we should raise an exception if we have this status and the api path does not match (so add an else). Removed pending_auth. Added a simple exception in my follow-up.

[4] When I tested this API path via API keys, I got no authorization. I added a permission catalogue (staff access) to get around that. If you dont have that permission, we should not even send a code.

[5] Letters:
+    if ( $content =~ m|\[% otp_token %\]| ) {
+        my $patron = Koha::Patrons->find(C4::Context->userenv->{number});
+        $tt_params->{otp_token} = Koha::Auth::TwoFactorAuth->new({patron => $patron})->code;
+    }
This seems quite hacky. Why not pass it to Letters from the api module? Moved it now.
This still needs updating the notice stuff.

[6] QA question: Is 400 the correct error code to tell the email has not been sent?
I guess it is not. The client did nothing wrong. Maybe just plain 500? But having some doubts about that too.
Or always 200/201 and refer for details to JSON body?

[7] TODO Hardcoded phrase: It is valid one minute.

[8] Functional question:
When you want to enable 2FA without a mobile phone, what should you do? There is no link to send the code on that form.

[9] Current code:
C4::Context->config('encryption_key')
<encryption_key>__ENCRYPTION_KEY__</encryption_key>
Do we still need to replace it in koha-create by the actual key ?
Enable 2FA: Form text: Can't scan the code?  To add the entry manually, provide the following details to the application on your phone.  Account: BRANCH Key: BRANCH_EMAIL Time based: Yes
But the form does not show the Secret. So telling the user to enter the details on their phone is useless?

Let me know if you agree with the follow-up.
Comment 17 Marcel de Rooy 2022-06-27 12:00:32 UTC
Tomas, what do you think? See prev comment.
Comment 18 Jonathan Druart 2022-06-27 16:04:54 UTC
(In reply to Marcel de Rooy from comment #16)
> [2] Code segment from Koha/REST/V1/Auth.pm
>     if ( !$authorization and
>          ( $params->{is_public} and
>           ( C4::Context->preference('RESTPublicAnonymousRequests') or
>             $user) or $params->{is_plugin} )
>         or $pending_auth
> This does not look good to me. Do we need pending_auth here ? If so, at
> least we need parentheses etc. My follow-up removes the line now.


Why? Can you explain? If the user is not fully authenticated they shouldn't be allowed to access REST API route.
With your follow-up patch the tests are failing now.
Comment 19 Jonathan Druart 2022-06-27 16:10:17 UTC
Note that the tests need an SMTP server configured to pass (which is wrong - TODO).

They can be proved anyway with: apt install python && python -m smtpd -n -c DebuggingServer localhost:25
Comment 20 Jonathan Druart 2022-07-06 15:14:51 UTC
Failing QA for comment 18. Marcel I waiting for an answer from you.
Comment 21 Tomás Cohen Arazi 2022-07-06 15:53:14 UTC
I agree with Marcel that the endpoint route doesn't feel correct. In our API we stick to using nouns instead of verbs (more RPC-ish).

So I'd suggest

POST /api/v1/auth/otp/token_delivery

I haven't read the patches yet, but I'd like to mention that, to me, OTP should be generated in very specific cases and so our handling in V1/Auth.pm needs to be very careful. Somehow, we need to identify a session that is in an intermediate state: it already identified correctly but still needs a specific action. So not any active session should be able to request a OTP.

As I said, I haven't reviewed this completely, so take it with a grain of salt.
Comment 22 Marcel de Rooy 2022-07-07 06:35:09 UTC
(In reply to Jonathan Druart from comment #18)
> (In reply to Marcel de Rooy from comment #16)
> > [2] Code segment from Koha/REST/V1/Auth.pm
> >     if ( !$authorization and
> >          ( $params->{is_public} and
> >           ( C4::Context->preference('RESTPublicAnonymousRequests') or
> >             $user) or $params->{is_plugin} )
> >         or $pending_auth
> > This does not look good to me. Do we need pending_auth here ? If so, at
> > least we need parentheses etc. My follow-up removes the line now.
> 
> 
> Why? Can you explain? If the user is not fully authenticated they shouldn't
> be allowed to access REST API route.
> With your follow-up patch the tests are failing now.

With a bit of delay.. Thx for your patience :)

First, the !A and (B and (C or D)) or E expression seems to be wrong. If D should be true now, we are putting the door open! So the first impression is: we need some brackets here, but looking further we dont need this stage here.

If the cookie tells us additional-auth-needed, we should only allow the otp_token path and set the user. Another path should be revoked (follow-up). If you set the user, and this statement is therefore fine for API, there is no need for pending auth. You can just run this call. The current checks for user are enough.

Note the relation with x-koha-authorization too. The follow-up adds this line. We should jump to the else branch and check for $user now.

If you read this code carefully, I think you should understand the point?

About the test, I will submit another comment or fix it. Please wait.
Comment 23 Marcel de Rooy 2022-07-07 06:51:29 UTC
Created attachment 137251 [details] [review]
Bug 28787: Fix t/db_dependent/api/v1/two_factor_auth.t

Changing flags to 20 to include the required Staff access for
intranet login.
This changes the http status. The API raises an exception now.

Test plan:
Run t/db_dependent/api/v1/two_factor_auth.t
Comment 24 Marcel de Rooy 2022-07-07 06:53:51 UTC
There is also t/db_dependent/selenium/authentication_2fa.t
I am not sure if it fails now. Cant run it.
But as you can see in the last patch, it should be a trivial fix.
Comment 25 Marcel de Rooy 2022-07-07 07:05:21 UTC
(In reply to Tomás Cohen Arazi from comment #21)
> I haven't read the patches yet, but I'd like to mention that, to me, OTP
> should be generated in very specific cases and so our handling in V1/Auth.pm
> needs to be very careful. Somehow, we need to identify a session that is in
> an intermediate state: it already identified correctly but still needs a
> specific action. So not any active session should be able to request a OTP.

Currently, we do allow it. I am not sure if we should need to revoke it when the session allows it.
We could easily revoke for status==ok, but the code wont improve imo.
Comment 26 Marcel de Rooy 2022-07-07 07:13:14 UTC
(In reply to Marcel de Rooy from comment #22)
> First, the !A and (B and (C or D)) or E expression seems to be wrong. If D
> should be true now, we are putting the door open! 

I shuffled my letters here. Last line should if E is true..
Comment 27 Jonathan Druart 2022-07-07 07:18:16 UTC
Selenium tests are passing. I do agree with your patch after a second look.
Comment 28 Jonathan Druart 2022-07-07 07:26:32 UTC
(In reply to Jonathan Druart from comment #27)
> Selenium tests are passing. I do agree with your patch after a second look.

Well, this change looks wrong, we should keep 403 IMO.

     # Patron is not authenticated yet
-    $t->request_ok($tx)->status_is(403);
+    $t->request_ok($tx)->status_is(500); # FIXME Check the exception instead?
Comment 29 Marcel de Rooy 2022-07-07 07:48:35 UTC
(In reply to Jonathan Druart from comment #28)
> (In reply to Jonathan Druart from comment #27)
> > Selenium tests are passing. I do agree with your patch after a second look.
> 
> Well, this change looks wrong, we should keep 403 IMO.
> 
>      # Patron is not authenticated yet
> -    $t->request_ok($tx)->status_is(403);
> +    $t->request_ok($tx)->status_is(500); # FIXME Check the exception
> instead?

Yes, I agree with that. But cause lies in current code. We could solve it on a new report too. No big deal.
Comment 30 Jonathan Druart 2022-07-07 17:28:31 UTC
(In reply to Marcel de Rooy from comment #16)
> Generally, I have some doubts about the API path api/v1/auth/send_otp_token.
> Sending a token is not a normally expected API action; it sounds like a
> 'misused verb'. You could think of creating a OTP code as an API action,
> although we do not really add it as entity.
> Apart from that it works. See some details hereunder.

Done, renamed with Tomas's suggestion: /auth/otp/token_delivery

> [1] Your TODO - I am not sure about the following line, so I commented it
> but 
> let it in the patch
> +            #|| $c->req->url->to_abs->path eq '/api/v1/auth/send_otp_token'
> ) {
> The otp path should go thru the chain. So this line should not be here
> although commented. Removed it.

Yes. Thanks!

> [2] Code segment from Koha/REST/V1/Auth.pm
>     if ( !$authorization and
>          ( $params->{is_public} and
>           ( C4::Context->preference('RESTPublicAnonymousRequests') or
>             $user) or $params->{is_plugin} )
>         or $pending_auth
> This does not look good to me. Do we need pending_auth here ? If so, at
> least we need parentheses etc. My follow-up removes the line now.

Agreed.

> [3] This segment is incomplete:
>         elsif ($status eq "additional-auth-needed") {
>             if ( $c->req->url->to_abs->path eq '/api/v1/auth/send_otp_token'
> ) {
>                 $user = Koha::Patrons->find( $session->param('number') );
>                 $cookie_auth = 1;
>                 $pending_auth = 1;
>             }
> I think we should raise an exception if we have this status and the api path
> does not match (so add an else). Removed pending_auth. Added a simple
> exception in my follow-up.

I reworked this part to take into account Tomas's remark. Requesting the token should only be done when a full authentication is pending.

> [4] When I tested this API path via API keys, I got no authorization. I
> added a permission catalogue (staff access) to get around that. If you dont
> have that permission, we should not even send a code.

You did it in your follow-up patch.

> [5] Letters:
> +    if ( $content =~ m|\[% otp_token %\]| ) {
> +        my $patron = Koha::Patrons->find(C4::Context->userenv->{number});
> +        $tt_params->{otp_token} = Koha::Auth::TwoFactorAuth->new({patron =>
> $patron})->code;
> +    }
> This seems quite hacky. Why not pass it to Letters from the api module?
> Moved it now.
> This still needs updating the notice stuff.

The idea was to make it available from other templates, but that indeed seems useless.

> [6] QA question: Is 400 the correct error code to tell the email has not
> been sent?
> I guess it is not. The client did nothing wrong. Maybe just plain 500? But
> having some doubts about that too.
> Or always 200/201 and refer for details to JSON body?

It can be a configuration error: the SMTP error is not setup correctly, or the patron does not have an email address. I guess we should improve the UX.

> [7] TODO Hardcoded phrase: It is valid one minute.

Well, it's hardcoded but it's true so far. It will need to be modified after bug 30843.

> [8] Functional question:
> When you want to enable 2FA without a mobile phone, what should you do?
> There is no link to send the code on that form.

Yes, that is a good idea to add it there as well. I am going to open a separate bug report (bug 31118).

> [9] Current code:
> C4::Context->config('encryption_key')
> <encryption_key>__ENCRYPTION_KEY__</encryption_key>
> Do we still need to replace it in koha-create by the actual key ?

Are you asking if we should setup a key for new installs?

> Enable 2FA: Form text: Can't scan the code?  To add the entry manually,
> provide the following details to the application on your phone.  Account:
> BRANCH Key: BRANCH_EMAIL Time based: Yes
> But the form does not show the Secret. So telling the user to enter the
> details on their phone is useless?

Yes, we should show the secret. Opening a new bug report (bug 31119).

> Let me know if you agree with the follow-up.

Almost, follow-up coming ;)

Still TODO (will have a look tomorrow):
* Make the tests pass if the SMTP server is not set (hum are we doing that already somewhere in other tests?)
* Improve user feedback messages if the email has not been sent

TODO on another bug report as well: Force 2FA for the REST API routes when not using Basic auth (this needs bug 29836).
Comment 31 Jonathan Druart 2022-07-07 17:30:36 UTC
Created attachment 137330 [details] [review]
Bug 28787: Rename the REST API route to /auth/otp/token_delivery
Comment 32 Jonathan Druart 2022-07-07 17:30:41 UTC
Created attachment 137331 [details] [review]
Bug 28787: Don't send the notice if we are not waiting for 2FA

If we are fully logged-in or haven't pass the password phase, don't send
the notice!
Comment 33 Jonathan Druart 2022-07-08 07:10:16 UTC
Created attachment 137369 [details] [review]
Bug 28787: Mock send_or_die

To make the tests pass even if no SMTP server is defined.
Comment 34 Jonathan Druart 2022-07-08 07:10:21 UTC
Created attachment 137370 [details] [review]
Bug 28787: Comment selenium test that will fail if no SMTP server is defined

This is a deadlock I think. Any ideas how we could continue to test that
in any conditions?
Comment 35 Jonathan Druart 2022-07-08 07:11:56 UTC
Created attachment 137371 [details] [review]
Bug 28787: Don't send the notice if we are not waiting for 2FA

If we are fully logged-in or haven't pass the password phase, don't send
the notice!
Comment 36 Jonathan Druart 2022-07-08 07:12:12 UTC
(In reply to Jonathan Druart from comment #30)
> Still TODO (will have a look tomorrow):
> * Make the tests pass if the SMTP server is not set (hum are we doing that
> already somewhere in other tests?)

Done, but I had to remove a selenium test. I don't think we can find a solution to make it pass without SMTP server.

> * Improve user feedback messages if the email has not been sent

Done. Not how I imagined it yesterday, but I think it's simpler and efficient enough.
Comment 37 Marcel de Rooy 2022-07-08 07:57:10 UTC
Applying: Bug 28787: Mock send_or_die
error: sha1 information is lacking or useless (t/db_dependent/api/v1/two_factor_auth.t).
Comment 38 Marcel de Rooy 2022-07-08 08:00:04 UTC
Fixed it manually. Dont worry
Comment 39 Marcel de Rooy 2022-07-08 08:04:01 UTC
time prove t/db_dependent/api/v1/two_factor_auth.t
t/db_dependent/api/v1/two_factor_auth.t .. ok
All tests successful.
Files=1, Tests=1,  6 wallclock secs ( 0.04 usr  0.00 sys +  5.22 cusr  0.38 csys =  5.64 CPU)
Result: PASS

real    0m5.824s
user    0m5.334s
sys     0m0.393s

Why is it so slow ?
Comment 40 Jonathan Druart 2022-07-08 08:34:16 UTC
Created attachment 137379 [details] [review]
Bug 28787: Don't request a token if no email address defined
Comment 41 Jonathan Druart 2022-07-08 08:34:21 UTC
Created attachment 137380 [details] [review]
Bug 28787: Mock send_or_die

To make the tests pass even if no SMTP server is defined.
Comment 42 Jonathan Druart 2022-07-08 08:34:26 UTC
Created attachment 137381 [details] [review]
Bug 28787: Comment selenium test that will fail if no SMTP server is defined

This is a deadlock I think. Any ideas how we could continue to test that
in any conditions?
Comment 43 Marcel de Rooy 2022-07-08 09:44:00 UTC
Created attachment 137391 [details] [review]
Bug 28787: Send a notice with the TOTP token

Bug 28786 let librarians enable a Two-factor authentication but force them to use
an application to generate the TOTP token.

This new enhancement add the ability to send an email containing the token to the
patron once it's authenticaed

The new notice template has the code '2FA_OTP_TOKEN'

Test plan:
- Setup the two-factor authentication (you need the config entry and the
syspref ON)
- Enable it for your logged in patron
- Logout
- Login and notice the new link "Send the code by email"
- Click on it and confirm that you received an email with the code
- Use the code to be fully logged in

QA question: Is 400 the correct error code to tell the email has not
been sent?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2022-07-08 09:44:06 UTC
Created attachment 137392 [details] [review]
Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2022-07-08 09:44:10 UTC
Created attachment 137393 [details] [review]
Bug 28787: Typo authenticaction

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2022-07-08 09:44:15 UTC
Created attachment 137394 [details] [review]
Bug 28787: Fix t/db_dependent/api/v1/two_factor_auth.t

Changing flags to 20 to include the required Staff access for
intranet login.
This changes the http status. The API raises an exception now.

Test plan:
Run t/db_dependent/api/v1/two_factor_auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Marcel de Rooy 2022-07-08 09:44:20 UTC
Created attachment 137395 [details] [review]
Bug 28787: Rename the REST API route to /auth/otp/token_delivery

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Marcel de Rooy 2022-07-08 09:44:25 UTC
Created attachment 137396 [details] [review]
Bug 28787: Don't send the notice if we are not waiting for 2FA

If we are fully logged-in or haven't pass the password phase, don't send
the notice!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Marcel de Rooy 2022-07-08 09:44:29 UTC
Created attachment 137397 [details] [review]
Bug 28787: Don't request a token if no email address defined

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 50 Marcel de Rooy 2022-07-08 09:44:36 UTC
Created attachment 137398 [details] [review]
Bug 28787: Mock send_or_die

To make the tests pass even if no SMTP server is defined.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Marcel de Rooy 2022-07-08 09:44:42 UTC
Created attachment 137399 [details] [review]
Bug 28787: Comment selenium test that will fail if no SMTP server is defined

This is a deadlock I think. Any ideas how we could continue to test that
in any conditions?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Marcel de Rooy 2022-07-08 09:44:47 UTC
Created attachment 137400 [details] [review]
Bug 28787: (follow-up) Typo and additional test

Adding a test for a 403 status when lowering authorization.
The Auth mock for check_cookie_auth is not needed here.
Reading back the session after flush either.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2022-07-08 10:00:24 UTC
Few small notes for QA:

+            elsif ($status eq "additional-auth-needed") {
+            }
=> Should we add an exception here?

            elsif ( $status eq 'ok' ) {
                Koha::Exceptions::Authentication->throw(
                    error => 'Cannot request a new token.' );
=> This is theoretical. But in this case you are authorized, but we (..) do no longer want to send another token, so it is actually a bad request. So 400 ?
Comment 54 Jonathan Druart 2022-07-27 07:04:22 UTC
Created attachment 138146 [details] [review]
Bug 28787: Fix misleading tests in two_factor_auth.t

We were having a "Patron is not authenticated yet" comment, but it was not correct,
we set 'number' and 'id' in session, and waiting-for-2FA was not set => the patron is fully authenticated.
The test returned 401 because we fully authenticated user cannot request
an otp token when not waiting for the second auth step.

This situation is already covered (last test of the subtest).

Test plan:
  prove t/db_dependent/api/v1/two_factor_auth.t
must return green
Comment 55 Jonathan Druart 2022-07-27 08:03:22 UTC
Created attachment 138149 [details] [review]
Bug 28787: Send a notice with the TOTP token

Bug 28786 let librarians enable a Two-factor authentication but force them to use
an application to generate the TOTP token.

This new enhancement add the ability to send an email containing the token to the
patron once it's authenticaed

The new notice template has the code '2FA_OTP_TOKEN'

Test plan:
- Setup the two-factor authentication (you need the config entry and the
syspref ON)
- Enable it for your logged in patron
- Logout
- Login and notice the new link "Send the code by email"
- Click on it and confirm that you received an email with the code
- Use the code to be fully logged in

QA question: Is 400 the correct error code to tell the email has not
been sent?

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

Sponsored-by: Rijksmuseum, Netherlands
Comment 56 Jonathan Druart 2022-07-27 08:03:26 UTC
Created attachment 138150 [details] [review]
Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters

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

Sponsored-by: Rijksmuseum, Netherlands
Comment 57 Jonathan Druart 2022-07-27 08:03:31 UTC
Created attachment 138151 [details] [review]
Bug 28787: Typo authenticaction

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

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

Sponsored-by: Rijksmuseum, Netherlands
Comment 58 Jonathan Druart 2022-07-27 08:03:35 UTC
Created attachment 138152 [details] [review]
Bug 28787: Fix t/db_dependent/api/v1/two_factor_auth.t

Changing flags to 20 to include the required Staff access for
intranet login.
This changes the http status. The API raises an exception now.

Test plan:
Run t/db_dependent/api/v1/two_factor_auth.t

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

Sponsored-by: Rijksmuseum, Netherlands
Comment 59 Jonathan Druart 2022-07-27 08:03:40 UTC
Created attachment 138153 [details] [review]
Bug 28787: Rename the REST API route to /auth/otp/token_delivery

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

Sponsored-by: Rijksmuseum, Netherlands
Comment 60 Jonathan Druart 2022-07-27 08:03:44 UTC
Created attachment 138154 [details] [review]
Bug 28787: Don't send the notice if we are not waiting for 2FA

If we are fully logged-in or haven't pass the password phase, don't send
the notice!

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

Sponsored-by: Rijksmuseum, Netherlands
Comment 61 Jonathan Druart 2022-07-27 08:03:49 UTC
Created attachment 138155 [details] [review]
Bug 28787: Don't request a token if no email address defined

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

Sponsored-by: Rijksmuseum, Netherlands
Comment 62 Jonathan Druart 2022-07-27 08:03:54 UTC
Created attachment 138156 [details] [review]
Bug 28787: Mock send_or_die

To make the tests pass even if no SMTP server is defined.

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

Sponsored-by: Rijksmuseum, Netherlands
Comment 63 Jonathan Druart 2022-07-27 08:03:59 UTC
Created attachment 138157 [details] [review]
Bug 28787: Comment selenium test that will fail if no SMTP server is defined

This is a deadlock I think. Any ideas how we could continue to test that
in any conditions?

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

Sponsored-by: Rijksmuseum, Netherlands
Comment 64 Jonathan Druart 2022-07-27 08:04:04 UTC
Created attachment 138158 [details] [review]
Bug 28787: (follow-up) Typo and additional test

Adding a test for a 403 status when lowering authorization.
The Auth mock for check_cookie_auth is not needed here.
Reading back the session after flush either.

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

Sponsored-by: Rijksmuseum, Netherlands
Comment 65 Jonathan Druart 2022-07-27 08:04:10 UTC
Created attachment 138159 [details] [review]
Bug 28787: Fix misleading tests in two_factor_auth.t

We were having a "Patron is not authenticated yet" comment, but it was not correct,
we set 'number' and 'id' in session, and waiting-for-2FA was not set => the patron is fully authenticated.
The test returned 401 because we fully authenticated user cannot request
an otp token when not waiting for the second auth step.

This situation is already covered (last test of the subtest).

Test plan:
  prove t/db_dependent/api/v1/two_factor_auth.t
must return green

Sponsored-by: Rijksmuseum, Netherlands
Comment 66 Kyle M Hall 2022-08-05 12:36:21 UTC
Created attachment 138689 [details] [review]
Bug 28787: (QA follow-up) Remove unused variable
Comment 67 Kyle M Hall 2022-08-05 14:24:20 UTC
Created attachment 138696 [details] [review]
Bug 28787: Send a notice with the TOTP token

Bug 28786 let librarians enable a Two-factor authentication but force them to use
an application to generate the TOTP token.

This new enhancement add the ability to send an email containing the token to the
patron once it's authenticaed

The new notice template has the code '2FA_OTP_TOKEN'

Test plan:
- Setup the two-factor authentication (you need the config entry and the
syspref ON)
- Enable it for your logged in patron
- Logout
- Login and notice the new link "Send the code by email"
- Click on it and confirm that you received an email with the code
- Use the code to be fully logged in

QA question: Is 400 the correct error code to tell the email has not
been sent?

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

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 68 Kyle M Hall 2022-08-05 14:24:38 UTC
Created attachment 138697 [details] [review]
Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters

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

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 69 Kyle M Hall 2022-08-05 14:24:42 UTC
Created attachment 138698 [details] [review]
Bug 28787: Typo authenticaction

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

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

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 70 Kyle M Hall 2022-08-05 14:24:46 UTC
Created attachment 138699 [details] [review]
Bug 28787: Fix t/db_dependent/api/v1/two_factor_auth.t

Changing flags to 20 to include the required Staff access for
intranet login.
This changes the http status. The API raises an exception now.

Test plan:
Run t/db_dependent/api/v1/two_factor_auth.t

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

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 71 Kyle M Hall 2022-08-05 14:24:50 UTC
Created attachment 138700 [details] [review]
Bug 28787: Rename the REST API route to /auth/otp/token_delivery

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

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 72 Kyle M Hall 2022-08-05 14:24:55 UTC
Created attachment 138701 [details] [review]
Bug 28787: Don't send the notice if we are not waiting for 2FA

If we are fully logged-in or haven't pass the password phase, don't send
the notice!

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

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 73 Kyle M Hall 2022-08-05 14:24:58 UTC
Created attachment 138702 [details] [review]
Bug 28787: Don't request a token if no email address defined

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

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 74 Kyle M Hall 2022-08-05 14:25:03 UTC
Created attachment 138703 [details] [review]
Bug 28787: Mock send_or_die

To make the tests pass even if no SMTP server is defined.

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

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Kyle M Hall 2022-08-05 14:25:09 UTC
Created attachment 138704 [details] [review]
Bug 28787: Comment selenium test that will fail if no SMTP server is defined

This is a deadlock I think. Any ideas how we could continue to test that
in any conditions?

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

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 76 Kyle M Hall 2022-08-05 14:25:13 UTC
Created attachment 138705 [details] [review]
Bug 28787: (follow-up) Typo and additional test

Adding a test for a 403 status when lowering authorization.
The Auth mock for check_cookie_auth is not needed here.
Reading back the session after flush either.

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

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 77 Kyle M Hall 2022-08-05 14:25:17 UTC
Created attachment 138706 [details] [review]
Bug 28787: Fix misleading tests in two_factor_auth.t

We were having a "Patron is not authenticated yet" comment, but it was not correct,
we set 'number' and 'id' in session, and waiting-for-2FA was not set => the patron is fully authenticated.
The test returned 401 because we fully authenticated user cannot request
an otp token when not waiting for the second auth step.

This situation is already covered (last test of the subtest).

Test plan:
  prove t/db_dependent/api/v1/two_factor_auth.t
must return green

Sponsored-by: Rijksmuseum, Netherlands

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 78 Kyle M Hall 2022-08-05 14:25:21 UTC
Created attachment 138707 [details] [review]
Bug 28787: (QA follow-up) Remove unused variable

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 79 Tomás Cohen Arazi 2022-08-08 16:48:59 UTC
Hey, can you please check this:

Error while executing command: unexpected alert open: Dismissed user prompt dialog: Cannot send the notice, you don't have an email address defined. at /usr/share/perl5/Selenium/Remote/Driver.pm line 411.
 at /usr/share/perl5/Selenium/Remote/Driver.pm line 356.
 # Looks like your test exited with 255 just after 3.
 /kohadevbox/koha/t/db_dependent/selenium/authentication_2fa.t .. 
Dubious, test returned 255 (wstat 65280, 0xff00)
 Failed 2/4 subtests 


Just in case, I'm running on:

$ ktd --selenium --es7 up -d
Comment 80 Jonathan Druart 2022-08-09 06:31:36 UTC
Tests are passing for me, can you retry please?

Also can you provide the screenshot (if one is generated) and the full error (with the line number)?
Comment 81 Tomás Cohen Arazi 2022-08-09 16:49:53 UTC Comment hidden (obsolete)
Comment 82 Tomás Cohen Arazi 2022-08-09 16:53:27 UTC
Created attachment 138932 [details] [review]
Bug 28787: (QA follow-up) Add spec description

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 83 Tomás Cohen Arazi 2022-08-09 16:57:05 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 84 Lucas Gass 2022-09-30 22:45:49 UTC
Do we need/want this is 22.05.x? I am skipping unless told otherwise.
Comment 85 Caroline Cyr La Rose 2023-03-01 20:21:38 UTC
Hi all!

I'm trying to document this but even with the test plan, there is a part I don't understand... The bug description says "Bug 28786 let librarians enable a Two-factor authentication but force them to use an application to generate the TOTP token." This lets me believe that an app is not necessary at all? But you still need an app to enable it, don't you? (sorry, maybe I misunderstand because of the jargon...)

Is it only to actually log in that you don't need the app? But you still need it for the rest of the setup?
Comment 86 Jonathan Druart 2023-03-02 07:41:38 UTC
(In reply to Caroline Cyr La Rose from comment #85)
> Hi all!
> 
> I'm trying to document this but even with the test plan, there is a part I
> don't understand... The bug description says "Bug 28786 let librarians
> enable a Two-factor authentication but force them to use an application to
> generate the TOTP token." This lets me believe that an app is not necessary
> at all? But you still need an app to enable it, don't you? (sorry, maybe I
> misunderstand because of the jargon...)
> 
> Is it only to actually log in that you don't need the app? But you still
> need it for the rest of the setup?

Yes, you still need an app to enable the feature.
Comment 87 Caroline Cyr La Rose 2023-03-02 14:53:01 UTC
(In reply to Jonathan Druart from comment #86)
> (In reply to Caroline Cyr La Rose from comment #85)
> > Hi all!
> > 
> > I'm trying to document this but even with the test plan, there is a part I
> > don't understand... The bug description says "Bug 28786 let librarians
> > enable a Two-factor authentication but force them to use an application to
> > generate the TOTP token." This lets me believe that an app is not necessary
> > at all? But you still need an app to enable it, don't you? (sorry, maybe I
> > misunderstand because of the jargon...)
> > 
> > Is it only to actually log in that you don't need the app? But you still
> > need it for the rest of the setup?
> 
> Yes, you still need an app to enable the feature.

Ok thanks!