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