https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #30 from Jonathan Druart <jonathan.druart+koha@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.