Bug 28786 - Two-factor authentication for staff client - TOTP
Summary: Two-factor authentication for staff client - TOTP
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords: dependency
Depends on: 28785
Blocks: 20476 28787 28998
  Show dependency treegraph
 
Reported: 2021-07-30 12:02 UTC by Jonathan Druart
Modified: 2021-10-22 08:18 UTC (History)
8 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Text to go in the release notes:
This enhancement adds an initial optional implementation of two-factor authentication (2FA) to improve security when logging into the staff interface. This implementation uses time-based, one-time passwords (TOTP) as the second factor, letting librarians use an application to handle it and provide them the code they need when logging in. It is enabled using the new system preference "TwoFactorAuthentication". Librarians can then enable 2FA for their account from More > Manage Two-Factor authentication. To setup: 1) Scan the QR code with an authenticator app. 2) Enter the one time code generated. For future logins, librarians are prompted to enter the authenticator code after entering their normal login credentials. Any authenticator app, such as Google Authenticator, andOTP, and many others can be used. Applications that enable backup of their 2FA accounts (either cloud-based or automatic) are recommended.
Version(s) released in:


Attachments
Bug 28786: Add new syspref TwoFactorAuthentication (2.95 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.secret (3.37 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.auth_method (3.41 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add controller script and template (10.15 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (25.98 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.59 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: DBIC schema changes (3.44 KB, patch)
2021-08-02 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add new syspref TwoFactorAuthentication (2.99 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.secret (3.42 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: Add new DB column borrowers.auth_method (3.46 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: Add controller script and template (10.19 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (26.01 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.64 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 28786: DBIC schema changes (3.49 KB, patch)
2021-08-02 19:53 UTC, David Nind
Details | Diff | Splinter Review
Example UI (108.87 KB, image/png)
2021-09-07 09:40 UTC, Tomás Cohen Arazi
Details
Bug 28786: Two-factor authentication for staff client - TOTP (27.10 KB, patch)
2021-09-10 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.64 KB, patch)
2021-09-10 15:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Two-factor authentication for staff client - TOTP (27.11 KB, patch)
2021-09-13 20:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Improve readability in C4::Auth::checkauth (5.64 KB, patch)
2021-09-13 20:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Correctly inherit from Auth::GoogleAuth (2.61 KB, patch)
2021-09-13 21:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28786: Add tests for checkauth (5.12 KB, patch)
2021-09-13 21:21 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-07-30 12:02:36 UTC
This enhancement will provide a first step for Two-factor authentication (2FA).

This implementation will use time-based, one-time passwords (TOTP) as the second factor, letting the librarians use an application to handle it and provide them the code they need to provide at a second login step.

The goal is to let librarians choose if then want to enable the feature, not to force them.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password
Comment 1 Jonathan Druart 2021-08-02 13:14:15 UTC
Created attachment 123375 [details] [review]
Bug 28786: Add new syspref TwoFactorAuthentication

Sponsored-by: Orex Digital
Comment 2 Jonathan Druart 2021-08-02 13:14:19 UTC
Created attachment 123376 [details] [review]
Bug 28786: Add new DB column borrowers.secret

Sponsored-by: Orex Digital
Comment 3 Jonathan Druart 2021-08-02 13:14:22 UTC
Created attachment 123377 [details] [review]
Bug 28786: Add new DB column borrowers.auth_method

Sponsored-by: Orex Digital
Comment 4 Jonathan Druart 2021-08-02 13:14:26 UTC
Created attachment 123378 [details] [review]
Bug 28786: Add controller script and template

Sponsored-by: Orex Digital
Comment 5 Jonathan Druart 2021-08-02 13:14:30 UTC
Created attachment 123379 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital
Comment 6 Jonathan Druart 2021-08-02 13:14:34 UTC
Created attachment 123380 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital
Comment 7 Jonathan Druart 2021-08-02 13:14:38 UTC
Created attachment 123381 [details] [review]
Bug 28786: DBIC schema changes

Sponsored-by: Orex Digital
Comment 8 David Nind 2021-08-02 19:53:05 UTC
Created attachment 123397 [details] [review]
Bug 28786: Add new syspref TwoFactorAuthentication

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2021-08-02 19:53:09 UTC
Created attachment 123398 [details] [review]
Bug 28786: Add new DB column borrowers.secret

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2021-08-02 19:53:14 UTC
Created attachment 123399 [details] [review]
Bug 28786: Add new DB column borrowers.auth_method

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2021-08-02 19:53:20 UTC
Created attachment 123400 [details] [review]
Bug 28786: Add controller script and template

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2021-08-02 19:53:25 UTC
Created attachment 123401 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2021-08-02 19:53:30 UTC
Created attachment 123402 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2021-08-02 19:53:35 UTC
Created attachment 123403 [details] [review]
Bug 28786: DBIC schema changes

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 David Nind 2021-08-02 19:58:12 UTC
Testing notes (koha-testing-docker):
- Tested with Google Authenticator app
- Tested with andOTP app (https://github.com/andOTP/andOTP)
- Everything work as per the test plan
Comment 16 Tomás Cohen Arazi 2021-09-03 14:27:09 UTC
Can we add a new table for the 2FA secrets please?
Comment 17 Jonathan Druart 2021-09-06 09:19:00 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> Can we add a new table for the 2FA secrets please?

Why?
Adding it means that the code needs to be adjusted as well, to deal with 1-N relationship at DB level. IMO the DB structure should reflect the UI. If we need it later it will be easy to move it to a separate table.
Comment 18 Tomás Cohen Arazi 2021-09-06 12:16:46 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Tomás Cohen Arazi from comment #16)
> > Can we add a new table for the 2FA secrets please?
> 
> Why?
> Adding it means that the code needs to be adjusted as well, to deal with 1-N
> relationship at DB level. IMO the DB structure should reflect the UI. If we
> need it later it will be easy to move it to a separate table.

2FA secrets (and the same for overdrive tokens, etc) are not part of the 'patron' entity. Changing the model like this to avoid a new table has another side effects: you will need to remove it in Koha::Patron::to_api_mapping, and if we want to add routes for dealing with them, the code will look weird.
Comment 19 Jonathan Druart 2021-09-07 09:33:15 UTC
Are you suggesting a one-column table with a list of secrets or a ('secret', 'auth_type') that would contain ('my_secret_passphrase', 'totp') with a uniq on auth_type?
Comment 20 Tomás Cohen Arazi 2021-09-07 09:40:11 UTC
Created attachment 124569 [details]
Example UI

I'm not sure the 'app' should be only one or not, I guess the answer is yes. But  I like Github's page, and it points us towards this 'several 2FA methods' thinking.
Comment 21 Jonathan Druart 2021-09-08 09:56:33 UTC
(In reply to Tomás Cohen Arazi from comment #20)
> Created attachment 124569 [details]
> Example UI
> 
> I'm not sure the 'app' should be only one or not, I guess the answer is yes.
> But  I like Github's page, and it points us towards this 'several 2FA
> methods' thinking.

The "Security keys" is not implemented in this development, I can understand the 1-N you would need in this case (you can have several keys), but not with the TOTP. You have only one TOTP secret that you could regenerate, but that will invalidate the existing one. How is the secret different than the password? Are you suggesting we should have the password in this new table?
Comment 24 David Cook 2021-09-09 02:16:50 UTC
If we look at Red Hat's FOSS Identity Management system Keycloak, we can see that they do store credentials separately to the user_entity table.

https://www.keycloak.org/docs/latest/server_admin/#_user-credentials

http://htmlpreview.github.io/?https://gist.githubusercontent.com/thomasdarimont/b1c19da5e8df747b8596e6ddcda7e36f/raw/29309467f4ea07519cf614fd74943272e7d939f4/keycloak_db_overview_4.0.0.CR1-SNAPSHOT.svg

They store both passwords and one-time passwords using that table.

I don't know about Keycloak for sure, but it's common for applications to store multiple OTPs to give users a margin of error. For instance, with AWS and Google Authenticator, you can use the current code or the past 1-2 codes I believe. Using a separate credential table would make that easier.
Comment 25 David Cook 2021-09-09 04:15:26 UTC
Note having a credential table could also make it easier to add "temporary" passwords. 

For instance, it would be great if staff could reset a password, tell the patron the password, and then the patron would be forced to reset that password on first login. I'm going to raise a ticket for that and link it to here...
Comment 26 David Cook 2021-09-09 04:18:23 UTC
Note that Keycloak db schema view is very old. It's just the first visualization I could find while I was quickly looking.
Comment 27 Martin Renvoize 2021-09-09 13:57:46 UTC
I've finally found a moment to look at this.

Whilst I like Tomas's proposal to normalise the database by having a distinct table.. I think that can certainly come as a followup later.

I do wonder why you've chosen a super new cpan module for this, Auth::GoogleAuth. I had considered Authen::OAth myself, because although it's a little smaller (you'de need something to generate QR codes on top), it's further up the CPAN river and is written by a trusted author... might even already have a debian package.

Finally, and what I would consider a QA failure.. the secret is stored in plain text in the database. I feel this should be stored encrypted using the a passphrase stored in the config file (we could re-use api_secret_passphrase or add a new field to the config.
Comment 28 Jonathan Druart 2021-09-10 09:08:58 UTC
(In reply to Martin Renvoize from comment #27)
> I've finally found a moment to look at this.
> 
> Whilst I like Tomas's proposal to normalise the database by having a
> distinct table.. I think that can certainly come as a followup later.

If we are talking about 3NF, I don't think having a separate table with a single column (different than the FK) is what can be called normalisation.

If that is what you have in mind, I think it's wrong:
borrowers < 1-1 > secrets
                  - borrowernumber (FK borrowers.borrowernumber)
                  - secret


> I do wonder why you've chosen a super new cpan module for this,
> Auth::GoogleAuth. I had considered Authen::OAth myself, because although
> it's a little smaller (you'de need something to generate QR codes on top),
> it's further up the CPAN river and is written by a trusted author... might
> even already have a debian package.

I've compared them all.
There is a recent version of Auth::GoogleAuth, which show it's still an active project:
https://metacpan.org/pod/Auth::GoogleAuth
1.03      2021-01-10
1.02      2017-12-26
1.01      2015-08-03
1.00      2015-08-03

Note that 1.03 is only in bullseye.

https://metacpan.org/pod/Authen::OATH
2.0.1     2017-04-12
1.0.0     2010-05-25

The author is active in different other projects: https://metacpan.org/author/GRYPHON

Also, it can generate QR codes out-of-the-box.

> Finally, and what I would consider a QA failure.. the secret is stored in
> plain text in the database. I feel this should be stored encrypted using the
> a passphrase stored in the config file (we could re-use
> api_secret_passphrase or add a new field to the config.

How would you do that?
You encrypt when you don't want to know the secret, and you will never know it again.
Here we need the secret (and not one of its hashed version) as it's an external module that is doing the comparison job. We could eventually generate a new secret containing borrowers.secret, like we concatenate bcrypt_settings (or part of it) with borrowers.secret. That could work but feel weird.
Comment 29 Martin Renvoize 2021-09-10 10:46:39 UTC
Not all encryption is one way ;). I was suggesting for these fields we use encrypt/decrypt where we have the key stored outside of the database, but on the server.  That way, we can decrypt the secret as needed on the server to validate the top. But the key secret itself can't easily be leaked via reports.
Comment 30 Jonathan Druart 2021-09-10 15:16:18 UTC
This will be done on bug 28998.
Comment 31 Jonathan Druart 2021-09-10 15:29:54 UTC
Created attachment 124770 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 32 Jonathan Druart 2021-09-10 15:30:14 UTC
Created attachment 124771 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 33 Martin Renvoize 2021-09-10 16:05:12 UTC
QA looking here.

So far I'm reasonably happy. I think I would have prefered more isolation of the verification step myself, but I can follow on with that in a follow-up bug.

So, personally, I would pass around a 'varified' state linked to the session (as you do I believe). Then, for any get_template_and_user calls I'd have checked the verification status and redirected to a self-contained verification controller for the MFA check... rather than folding the check into Auth.pm and the login pages themselves.  In this way you open up the option to invalidate the verification without invalidating the session entirely for things like patron modification for example (when we add this to the opac.. I can see it being most helpful to not require the verification step at first login but rather upon taking higher privilege actions).

Anywho.. I'll continue down the QA route but wanted to flag it in case you had any feedback as to why you took this particular route rather than any others?
Comment 34 Marcel de Rooy 2021-09-13 09:31:43 UTC
+package Koha::Auth::TwoFactorAuth;
+=head1 NAME
+
+Koha::Encryption - Koha class to encrypt or decrypt strings

Hmm
Comment 35 Marcel de Rooy 2021-09-13 09:44:19 UTC
Template process failed: undef error - The method Koha::Patron->auth_method is not covered by tests!
Comment 36 Marcel de Rooy 2021-09-13 09:46:16 UTC
(In reply to Marcel de Rooy from comment #35)
> Template process failed: undef error - The method Koha::Patron->auth_method
> is not covered by tests!

No DBIx schema change.
Comment 37 Marcel de Rooy 2021-09-13 12:05:56 UTC
Not a complete QA, but at least some remarks:

Nice development! Would like to see 2FA in Koha.

There was discussion about moving the secret to another table. I tend to follow Tomas here. Two factor authentication now only includes TOTP, but we could extend that. If we have several methods, they would (probably) have their own secrets. So yes a separate table would be better.

In terms of security I wonder if we should let the user choose to enable 2FA. If the library switches 2FA on, I would opt for enforcing it. How would you let a user register at that point? Might be that you need some verification mail mechanism here to allow access to the register page exposing the shared key (QR).

As for code, Koha/Auth/TwoFactorAuth.pm should be a folder or base class. And the TOTP code should move deeper then?
There is a Selenium test, but not a regular one?

The "Improve readability" patch triggers this remark ;) The code in C4::Auth is very essential, but already a pain. The maintenance of it by adding the 2FA will be even harder. No one volunteers to rewrite it, but wouldnt this be a great opportunity? Just hoping.. The current changes with a nice "ugly trick" are not the greatest base for confidence.
Comment 38 Jonathan Druart 2021-09-13 20:03:59 UTC
Martin and Marcel, thank you for your interest in this patch set!

As you noted, C4::Auth is not an easy place, and we all know that. It was hard to write code that followed the different "check auth" methods, I reworked the code to make the code isolated enough to be located in only one place (checkauth). And I wrote it, it was the best solution I found without having to rewrite more parts.

There are more to implement but, as said in the commit message, the idea was to provide a first step, without reworking the whole C4::Auth code.

I would like to get more funding to work on follow-up bugs, but as of today I have already exploded the time I could dedicate to this bug.

(In reply to Martin Renvoize from comment #33)
> So, personally, I would pass around a 'varified' state linked to the session
> (as you do I believe). Then, for any get_template_and_user calls I'd have
> checked the verification status and redirected to a self-contained
> verification controller for the MFA check... rather than folding the check
> into Auth.pm and the login pages themselves.  In this way you open up the
> option to invalidate the verification without invalidating the session
> entirely for things like patron modification for example (when we add this
> to the opac.. I can see it being most helpful to not require the
> verification step at first login but rather upon taking higher privilege
> actions).
> 
> Anywho.. I'll continue down the QA route but wanted to flag it in case you
> had any feedback as to why you took this particular route rather than any
> others?

I don't follow what you suggest. A check in get_template_and_user is not enough, we need to catch other auth calls as well.

(In reply to Marcel de Rooy from comment #37)
> There was discussion about moving the secret to another table. I tend to
> follow Tomas here. Two factor authentication now only includes TOTP, but we
> could extend that. If we have several methods, they would (probably) have
> their own secrets. So yes a separate table would be better.

IMO it's out of the scope. That would put this bug in a dead-end situation.

> In terms of security I wonder if we should let the user choose to enable
> 2FA. If the library switches 2FA on, I would opt for enforcing it. How would
> you let a user register at that point? Might be that you need some
> verification mail mechanism here to allow access to the register page
> exposing the shared key (QR).

It's part of the enhancement I suggested in the commit message :)
"* Force 2FA for librarians"

> As for code, Koha/Auth/TwoFactorAuth.pm should be a folder or base class.
> And the TOTP code should move deeper then?

It's a first step, no need to make it more complex. We can still create the base class we will need it.

> There is a Selenium test, but not a regular one?

What tests would make you happy? I could add tests for Koha::Auth::TwoFactorAuth but it's based on Auth::GoogleAuth and only overwrite the constructor.
Or are you asking for C4::Auth::checkauth tests?

With selenium tests we are testing the whole workflow, they are robust and I am very happy with them as they helped me a lot during the development process.

> The "Improve readability" patch triggers this remark ;) The code in C4::Auth
> is very essential, but already a pain. The maintenance of it by adding the
> 2FA will be even harder. No one volunteers to rewrite it, but wouldnt this
> be a great opportunity? Just hoping.. The current changes with a nice "ugly
> trick" are not the greatest base for confidence.

I am open to suggestions to improve the code I've added, and isolate more the code. But I spent a lot of time and tried different approaches before ending with that one.
Comment 39 Jonathan Druart 2021-09-13 20:06:25 UTC
Created attachment 124844 [details] [review]
Bug 28786: Two-factor authentication for staff client - TOTP

This patchset introduces the Two-factor authentication (2FA) idea in
Koha.

It is far for complete, and only implement one way of doing it, but at
least it's a first step.
The idea here is to offer the librarian user the ability to
enable/disable 2FA when logging in to Koha.

It will use time-based, one-time passwords (TOTP) as the second factor,
an application to handle that will be required.

https://en.wikipedia.org/wiki/Time-based_One-Time_Password

More developements are possible on top of this:
* Send a notice (sms or email) with the code
* Force 2FA for librarians
* Implementation for OPAC
* WebAuthn, FIDO2, etc. - https://fidoalliance.org/category/intro-fido/

Test plan:
 0.
  a. % apt install -y libauth-googleauth-perl && updatedatabase && restart_all
  b. To test this you will need an app to generate the TOTP token, you can
 use FreeOTP that is open source and easy to use.
 1. Turn on TwoFactorAuthentication
 2. Go to your account, click 'More' > 'Manage Two-Factor authentication'
 3. Click Enable, scan the QR code with the app, insert the pin code and
 register
 4. Your account now requires 2FA to login!
 5. Notice that you can browse until you logout
 6. Logout
 7. Enter the credential and the pincode provided by the app
 8. Logout
 9. Enter the credential, no pincode
10. Confirm that you are stuck on the second auth form (ie. you cannot
access other Koha pages)
11. Click logout => First login form
12. Enter the credential and the pincode provided by the app

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 40 Jonathan Druart 2021-09-13 20:06:29 UTC
Created attachment 124845 [details] [review]
Bug 28786: Improve readability in C4::Auth::checkauth

Sponsored-by: Orex Digital

Signed-off-by: David Nind <david@davidnind.com>
Comment 41 Jonathan Druart 2021-09-13 20:06:46 UTC
(In reply to Marcel de Rooy from comment #34)
> +package Koha::Auth::TwoFactorAuth;
> +=head1 NAME
> +
> +Koha::Encryption - Koha class to encrypt or decrypt strings
> 
> Hmm

Oops, fixed!
Comment 42 Jonathan Druart 2021-09-13 21:21:07 UTC
Created attachment 124846 [details] [review]
Bug 28786: Correctly inherit from Auth::GoogleAuth
Comment 43 Jonathan Druart 2021-09-13 21:21:14 UTC
Created attachment 124847 [details] [review]
Bug 28786: Add tests for checkauth
Comment 44 Katrin Fischer 2021-10-21 19:50:54 UTC
I am sure I am missing something obvious, but:

sudo apt-get install libauth-googleauth-perl
Reading package lists... Done
Building dependency tree       
Reading state information... Done
E: Unable to locate package libauth-googleauth-perl

:(
Comment 45 David Cook 2021-10-21 22:13:39 UTC
(In reply to Katrin Fischer from comment #44)
> I am sure I am missing something obvious, but:
> 
> sudo apt-get install libauth-googleauth-perl
> Reading package lists... Done
> Building dependency tree       
> Reading state information... Done
> E: Unable to locate package libauth-googleauth-perl
> 
> :(

Looks like it was added in Buster so maybe you're using an older OS?

https://packages.debian.org/buster/libauth-googleauth-perl
Comment 46 Jonathan Druart 2021-10-22 07:29:55 UTC
Indeed, it's not in D9
"The current LTS version is Debian 9 stretch and will be supported from 2020-07-06 to 2022-06-30"

My bad I was sure I checked the availability of the package.

So, some options:
- wait one more release to push (22.05 released one month before EOL of D9 and we drop D9 support)
- Try to package it for D9 (Mason, how ease would it be?)
- Flag the module has optional and replace use with require
Comment 47 Katrin Fischer 2021-10-22 07:32:06 UTC
I'd be ok with 2 or 3.

I am not sure I can easily update my kohadevbox to a newer Debian - if someone can help me get the module sorted, I am happy to have another go at this.
Comment 48 Jonathan Druart 2021-10-22 07:52:24 UTC
(In reply to Katrin Fischer from comment #47)
> I'd be ok with 2 or 3.
> 
> I am not sure I can easily update my kohadevbox to a newer Debian - if
> someone can help me get the module sorted, I am happy to have another go at
> this.

Install via CPAN.
Comment 49 Mason James 2021-10-22 08:18:52 UTC
(In reply to Jonathan Druart from comment #46)
> Indeed, it's not in D9
> "The current LTS version is Debian 9 stretch and will be supported from
> 2020-07-06 to 2022-06-30"
> 
> My bad I was sure I checked the availability of the package.
> 
> So, some options:
> - wait one more release to push (22.05 released one month before EOL of D9
> and we drop D9 support)
> - Try to package it for D9 (Mason, how ease would it be?)
> - Flag the module has optional and replace use with require

i've added libauth-googleauth-perl to the kc.org staging repo, for testing

root@kohadevbox:/kohadevbox#  apt-cache policy libauth-googleauth-perl
libauth-googleauth-perl:
  Installed: 1.03-1
  Candidate: 1.03-1
  Version table:
 *** 1.03-1 500
        500 http://debian.koha-community.org/koha-staging dev/main amd64 Packages
        100 /var/lib/dpkg/status

root@kohadevbox:/kohadevbox# lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description:    Debian GNU/Linux 9.13 (stretch)
Release:        9.13
Codename:       stretch