https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787
--- Comment #30 from Jonathan Druart <[email protected]> --- (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. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
