[Bug 28787] New: Send a notice with the TOTP token
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Bug ID: 28787 Summary: Send a notice with the TOTP token Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Notices Assignee: koha-bugs@lists.koha-community.org Reporter: jonathan.druart+koha@gmail.com QA Contact: testopia@bugs.koha-community.org Depends on: 28786 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28786 [Bug 28786] Two-factor authentication for staff client - TOTP -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Change sponsored?|--- |Seeking sponsor Assignee|koha-bugs@lists.koha-commun |jonathan.druart+koha@gmail. |ity.org |com -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|Notices |Authentication CC| |dpavlin@rot13.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Change sponsored?|Seeking sponsor |Sponsored Status|NEW |ASSIGNED CC| |martin.renvoize@ptfs-europe | |.com --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Will start this once bug 28786 is pushed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |20476 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20476 [Bug 20476] Two factor authentication for the staff client - omnibus -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Bug 28787 depends on bug 28786, which changed state. Bug 28786 Summary: Two-factor authentication for staff client - TOTP https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28786 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 135287 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135287&action=edit 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #3 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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' ) { -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- Comment on attachment 135287 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135287 Bug 28787: Send a notice with the TOTP token Review of attachment 135287: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=28787&attachment=135287) ----------------------------------------------------------------- ::: 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (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.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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]
+ 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |30842 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30842 [Bug 30842] Two-factor authentication code should be valid longer -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|30842 | See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30842 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30842 [Bug 30842] Two-factor authentication code should be valid longer -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #8 from Jonathan Druart <jonathan.druart+koha@gmail.com> ---
(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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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-googl... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #10 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #11 from David Cook <dcook@prosentient.com.au> --- (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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- (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.) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135287|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 136571 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136571&action=edit 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 136572 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136572&action=edit Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 136573 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=136573&action=edit Bug 28787: Typo authenticaction Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #16 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Tomas, what do you think? See prev comment. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #18 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #19 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #20 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Failing QA for comment 18. Marcel I waiting for an answer from you. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137251 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137251&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #27 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Selenium tests are passing. I do agree with your patch after a second look. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #28 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |31118 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31118 [Bug 31118] Allow to send the TOTP token when enabling 2FA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=31119 -- You are receiving this mail because: You are watching all bug changes.
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.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #31 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 137330 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137330&action=edit Bug 28787: Rename the REST API route to /auth/otp/token_delivery -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #32 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 137331 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137331&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #33 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 137369 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137369&action=edit Bug 28787: Mock send_or_die To make the tests pass even if no SMTP server is defined. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #34 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 137370 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137370&action=edit 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #137331|0 |1 is obsolete| | --- Comment #35 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 137371 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137371&action=edit 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #36 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #37 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Applying: Bug 28787: Mock send_or_die error: sha1 information is lacking or useless (t/db_dependent/api/v1/two_factor_auth.t). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #38 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Fixed it manually. Dont worry -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #39 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #40 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 137379 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137379&action=edit Bug 28787: Don't request a token if no email address defined -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #137369|0 |1 is obsolete| | --- Comment #41 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 137380 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137380&action=edit Bug 28787: Mock send_or_die To make the tests pass even if no SMTP server is defined. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #137370|0 |1 is obsolete| | --- Comment #42 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 137381 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137381&action=edit 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136571|0 |1 is obsolete| | --- Comment #43 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137391 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137391&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136572|0 |1 is obsolete| | --- Comment #44 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137392 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137392&action=edit Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #136573|0 |1 is obsolete| | --- Comment #45 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137393 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137393&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #137251|0 |1 is obsolete| | --- Comment #46 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137394 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137394&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #137330|0 |1 is obsolete| | --- Comment #47 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137395 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137395&action=edit Bug 28787: Rename the REST API route to /auth/otp/token_delivery Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #137371|0 |1 is obsolete| | --- Comment #48 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137396 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137396&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #137379|0 |1 is obsolete| | --- Comment #49 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137397 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137397&action=edit Bug 28787: Don't request a token if no email address defined Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #137380|0 |1 is obsolete| | --- Comment #50 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137398 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137398&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #137381|0 |1 is obsolete| | --- Comment #51 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137399 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137399&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #52 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 137400 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=137400&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #53 from 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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |31243 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31243 [Bug 31243] Misleading tests in api/v1/two_factor_auth.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #54 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138146 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138146&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |30588 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30588 [Bug 30588] Add the option to require 2FA setup on first staff login -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #137391|0 |1 is obsolete| | Attachment #137392|0 |1 is obsolete| | Attachment #137393|0 |1 is obsolete| | Attachment #137394|0 |1 is obsolete| | Attachment #137395|0 |1 is obsolete| | Attachment #137396|0 |1 is obsolete| | Attachment #137397|0 |1 is obsolete| | Attachment #137398|0 |1 is obsolete| | Attachment #137399|0 |1 is obsolete| | Attachment #137400|0 |1 is obsolete| | Attachment #138146|0 |1 is obsolete| | --- Comment #55 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138149 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138149&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #56 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138150 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138150&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #57 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138151 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138151&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #58 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138152 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138152&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #59 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138153 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138153&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #60 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138154 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138154&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #61 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138155 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138155&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #62 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138156 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138156&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #63 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138157 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138157&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #64 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138158 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138158&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #65 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 138159 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138159&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #66 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138689 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138689&action=edit Bug 28787: (QA follow-up) Remove unused variable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #138149|0 |1 is obsolete| | Attachment #138150|0 |1 is obsolete| | Attachment #138151|0 |1 is obsolete| | Attachment #138152|0 |1 is obsolete| | Attachment #138153|0 |1 is obsolete| | Attachment #138154|0 |1 is obsolete| | Attachment #138155|0 |1 is obsolete| | Attachment #138156|0 |1 is obsolete| | Attachment #138157|0 |1 is obsolete| | Attachment #138158|0 |1 is obsolete| | Attachment #138159|0 |1 is obsolete| | Attachment #138689|0 |1 is obsolete| | --- Comment #67 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138696 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138696&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #68 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138697 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138697&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #69 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138698 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138698&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #70 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138699 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138699&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #71 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138700 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138700&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #72 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138701 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138701&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #73 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138702 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138702&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #74 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138703 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138703&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #75 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138704 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138704&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #76 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138705 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138705&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #77 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138706 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138706&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #78 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 138707 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138707&action=edit Bug 28787: (QA follow-up) Remove unused variable Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #79 from Tomás Cohen Arazi <tomascohen@gmail.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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #80 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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)? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #81 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 138931 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138931&action=edit Bug 28787: (QA follow-up) Add spec description Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #138931|0 |1 is obsolete| | --- Comment #82 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 138932 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138932&action=edit Bug 28787: (QA follow-up) Add spec description Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |22.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #83 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 22.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #84 from Lucas Gass <lucas@bywatersolutions.com> --- Do we need/want this is 22.05.x? I am skipping unless told otherwise. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |Add the ability to send an release notes| |email containing the token | |to the | |patron once it's | |authenticated | | | |The new | |notice template is | |'2FA_OTP_TOKEN'. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |caroline.cyr-la-rose@inlibr | |o.com --- Comment #85 from Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 --- Comment #86 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28787 Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Documentation| |https://gitlab.com/koha-com submission| |munity/koha-manual/-/merge_ | |requests/632 Documentation| |Caroline Cyr La Rose contact| | --- Comment #87 from Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org