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
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 28786
Blocks: 30843
  Show dependency tree / graph
 
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
Circulation function:

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
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

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.