[Koha-bugs] [Bug 28787] Send a notice with the TOTP token

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787

--- Comment #30 from Jonathan Druart <jonathan.druart+koha at gmail.com> ---
(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).

-- 
You are receiving this mail because:
You are watching all bug changes.