Bug 30842 - Two-factor authentication code should be valid longer
Summary: Two-factor authentication code should be valid longer
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 28786
Blocks: 30843
  Show dependency treegraph
 
Reported: 2022-05-25 08:03 UTC by Jonathan Druart
Modified: 2023-06-08 22:26 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This extends the time a two-factor authentication code is valid for, in case it is not entered quickly enough. (Example: wait for the code to change, then enter the previous code - this should still work, but will not work when the code changes again.)
Version(s) released in:
22.11.00, 22.05.01


Attachments
Bug 30842: 2FA - Allow at least one old TOTP (1.13 KB, patch)
2022-05-25 08:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30842: 2FA - Allow at least one old TOTP (1.17 KB, patch)
2022-05-26 23:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 30842: 2FA - Allow at least one old TOTP (1.27 KB, patch)
2022-05-27 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-05-25 08:03:39 UTC
So far it's valid only 30 second, it should be extended.
Comment 1 Jonathan Druart 2022-05-25 08:07:28 UTC
Created attachment 135333 [details] [review]
Bug 30842: 2FA - Allow at least one old TOTP

We allow one old token when we are setting the two-factor auth, we
should reuse the same settings when validation the authentication
itself.

Test plan:
Setup 2FA for your logged-in user
Logout/Login
Have a look at the code and wait for 30 sec before using it (< 1min
however)
Comment 2 David Nind 2022-05-26 23:30:02 UTC
Created attachment 135414 [details] [review]
Bug 30842: 2FA - Allow at least one old TOTP

We allow one old token when we are setting the two-factor auth, we
should reuse the same settings when validation the authentication
itself.

Test plan:
Setup 2FA for your logged-in user
Logout/Login
Have a look at the code and wait for 30 sec before using it (< 1min
however)

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2022-05-26 23:40:14 UTC
Testing notes (koha-testing-docker).

Enabling two-factor authentication (from test plan for Bug 28786):
1. Enable two faction authentication: TwoFactorAuthentication system preference.
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!

Notes:
- Used andOTP to test.
- Waited for code to change, then entered old code: success!
- Waited for code to change twice, and then entered old code: fail (as expected).
- Entered code before it changed: success (as expected)!
- Disabled 2FA and setup again: worked as expected.

From my experience with 2FA, I've never had any problems with the short period of time (for other services, not Koha).
Comment 4 Marcel de Rooy 2022-05-27 06:05:28 UTC
I dont think that this oneliner patch should go on its own.
It is just hardcoding a 1 in the verify call in Auth.

This 1 should be a parameter from koha-conf (imo).
We should merge this with 30843 and add a parameter too for the interval that GoogleAuth gets from us in ->code and ->verify.
Comment 5 Jonathan Druart 2022-05-27 08:15:27 UTC
It's fixing a bug, an inconsistency. Bug 30843 is an enhancement. IMO it makes sense to have it pushed now and backported.
Comment 6 Marcel de Rooy 2022-05-27 08:55:06 UTC
(In reply to Jonathan Druart from comment #5)
> It's fixing a bug, an inconsistency. Bug 30843 is an enhancement. IMO it
> makes sense to have it pushed now and backported.

The backport argument wins.
Comment 7 Marcel de Rooy 2022-05-27 08:56:13 UTC
The title of this report could be more specific btw.
Comment 8 Marcel de Rooy 2022-05-27 08:56:59 UTC
Created attachment 135417 [details] [review]
Bug 30842: 2FA - Allow at least one old TOTP

We allow one old token when we are setting the two-factor auth, we
should reuse the same settings when validation the authentication
itself.

Test plan:
Setup 2FA for your logged-in user
Logout/Login
Have a look at the code and wait for 30 sec before using it (< 1min
however)

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Tomás Cohen Arazi 2022-06-01 19:23:36 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 10 Lucas Gass 2022-06-10 15:02:14 UTC
Pushed to 22.05.x for 22.05.01
Comment 11 Arthur Suzuki 2022-06-21 12:15:05 UTC
depends on 28786 which is an enhancement. not backporting to 21.11.