https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28786 --- Comment #38 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.