|
Description
Jonathan Druart
2021-07-30 12:05:21 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? 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' ) {
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 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? (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.) (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 > (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. (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 (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. (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... (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.) 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? Created attachment 136572 [details] [review] Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters Created attachment 136573 [details] [review] Bug 28787: Typo authenticaction Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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.
Tomas, what do you think? See prev comment. (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. 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 Failing QA for comment 18. Marcel I waiting for an answer from you. 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. (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. 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 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. (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. (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.. Selenium tests are passing. I do agree with your patch after a second look. (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? (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. (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). Created attachment 137330 [details] [review] Bug 28787: Rename the REST API route to /auth/otp/token_delivery 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! Created attachment 137369 [details] [review] Bug 28787: Mock send_or_die To make the tests pass even if no SMTP server is defined. 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? 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! (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. Applying: Bug 28787: Mock send_or_die error: sha1 information is lacking or useless (t/db_dependent/api/v1/two_factor_auth.t). Fixed it manually. Dont worry 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 ? Created attachment 137379 [details] [review] Bug 28787: Don't request a token if no email address defined Created attachment 137380 [details] [review] Bug 28787: Mock send_or_die To make the tests pass even if no SMTP server is defined. 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? 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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 ?
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 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 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 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 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 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 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 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 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 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 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 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 Created attachment 138689 [details] [review] Bug 28787: (QA follow-up) Remove unused variable 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 138707 [details] [review] Bug 28787: (QA follow-up) Remove unused variable Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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 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)? Created attachment 138931 [details] [review] Bug 28787: (QA follow-up) Add spec description Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 138932 [details] [review] Bug 28787: (QA follow-up) Add spec description Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 22.11. Nice work everyone, thanks! Do we need/want this is 22.05.x? I am skipping unless told otherwise. 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? (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. (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! |