[Koha-bugs] [Bug 28786] Two-factor authentication for staff client - TOTP

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28786

--- Comment #38 from Jonathan Druart <jonathan.druart+koha at 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.