Bug 33880 - "Enable two-factor authentication" fails if patron's library branchname is too long
Summary: "Enable two-factor authentication" fails if patron's library branchname is to...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jake Deery
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 32011
Blocks:
  Show dependency treegraph
 
Reported: 2023-06-01 16:38 UTC by Pedro Amorim
Modified: 2023-12-28 20:47 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02,22.11.08


Attachments
Bug 33880: Remove Version param from GB::Barcode->new (1.51 KB, patch)
2023-06-02 13:16 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33880: Remove Version param from GB::Barcode->new (1.59 KB, patch)
2023-06-06 08:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33880: Remove Version param from GB::Barcode->new (1.65 KB, patch)
2023-06-08 13:50 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-06-01 16:38:46 UTC
To reproduce, on ktd:

1) Enable TwoFactorAuthentication system preference
2) Go to "Administration->Libraries"
edit the library's name 
"Institut Protestant de Théologie"
to
"Institut Protestant de Théologie and another word"
3) Go to "My account" in the top right corner menu
4) Click the "Edit" button in the "Library use" box and set the library to the above library
5) Go back to "My account" and click "More" button, select "Manage two-factor authentication"
6) Click "Enable two-factor authentication" button
7) It explodes

Error logs:
[ERROR] POST /api/v1/auth/two-factor/registration: unhandled exception (Mojo::Exception)<<Overflow error. version 10
total bits: 2036  max bits: 1728>>

This is the same (or very similar) issue discussed in bug 32011
Bumping the version from 10 to something higher in GD::Barcode::new on Koha/Auth/TwoFactorAuth.pm works around this issue, but I think we should discuss and come up with a permanent solution.
Comment 1 Jonathan Druart 2023-06-02 05:30:46 UTC
What about using branchcode instead of branchname?
Comment 2 Marcel de Rooy 2023-06-02 06:17:22 UTC
(In reply to Jonathan Druart from comment #1)
> What about using branchcode instead of branchname?

10 characters. Should be fine, I guess.
Kind of weird that this CPAN module stumbles over such a thing..
Thought that issuer was just a label?
Comment 3 Katrin Fischer 2023-06-02 07:09:15 UTC
I think the length and characters in branchname could quickly become an issue for our libraries. But the branchcode is often nothing the user would connect with the library. Looking at my Google Authenticator app, there are not many clues to what the login is for. 

I think the best solution could be a configuration option in branches, an abbreviated library name. Often institutions will have an abbreviation that is well known or could create one for this use case. 

And maybe we could also show the options as a hint on the form?
Comment 4 Jake Deery 2023-06-02 08:36:44 UTC
Hello folks,

Having a quick read of GD::Barcode::QRcode, it looks like setting version to automatic allows the library to determine which QRcode to pick – https://metacpan.org/pod/GD::Barcode::QRcode#Version (note: it appears Version => 1 is automatic mode, based on the source code).

Do we need to specify which QRcode to use, or should we change it to automatic / 1? I'd imagine most authenticator apps will read any old code (that is what QRcodes were invented for, after all!)

Many thanks,
Jake
Comment 5 Jake Deery 2023-06-02 08:43:55 UTC
Update to my previous comment:

https://metacpan.org/release/KWITKNR/GD-Barcode-1.15/source/Barcode/QRcode.pm#L349 

It actually looks like ommitting the version entirely causes the QRcode generator to automatically iterate up until it finds the right place to stop. Cool, eh?

(Thanks Pedro!)
Jake
Comment 6 Jonathan Druart 2023-06-02 08:56:18 UTC
If that works, it's certainly the easiest and long term solution!
Comment 7 Pedro Amorim 2023-06-02 13:16:01 UTC
Created attachment 151981 [details] [review]
Bug 33880: Remove Version param from GB::Barcode->new

Looking at the code here
     if (Koha/Auth/TwoFactorAuth.pmoSelf->{Version}){        #--- auto version select
        for(->{Version}=1; ->{Version} <= 40; ++->{Version}) {
            last if ({->{Ecc}}->[->{Version}]
                        >=  + ->[->{Version}]);
        }
   }

in https://metacpan.org/release/KWITKNR/GD-Barcode-1.15/source/Barcode/QRcode.pm#L349

It appears the version parameter is not required, and will default to whatever version has the necessary bits for the input words given

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 8 Pedro Amorim 2023-06-02 13:17:48 UTC
Submitting this in Jake's behalf with my signature on it after looking at the code and testing.

Keeping in "Needs sign-off" as would be great to have others looking into this!
Comment 9 Jonathan Druart 2023-06-06 08:03:34 UTC
Created attachment 152037 [details] [review]
Bug 33880: Remove Version param from GB::Barcode->new

Looking at the code here
     if (Koha/Auth/TwoFactorAuth.pmoSelf->{Version}){        #--- auto version select
        for(->{Version}=1; ->{Version} <= 40; ++->{Version}) {
            last if ({->{Ecc}}->[->{Version}]
                        >=  + ->[->{Version}]);
        }
   }

in https://metacpan.org/release/KWITKNR/GD-Barcode-1.15/source/Barcode/QRcode.pm#L349

It appears the version parameter is not required, and will default to whatever version has the necessary bits for the input words given

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2023-06-06 08:39:47 UTC
Found bug 33904 when testing this.
Comment 11 Martin Renvoize 2023-06-08 13:50:55 UTC
Created attachment 152194 [details] [review]
Bug 33880: Remove Version param from GB::Barcode->new

Looking at the code here
     if (Koha/Auth/TwoFactorAuth.pmoSelf->{Version}){        #--- auto version select
        for(->{Version}=1; ->{Version} <= 40; ++->{Version}) {
            last if ({->{Ecc}}->[->{Version}]
                        >=  + ->[->{Version}]);
        }
   }

in https://metacpan.org/release/KWITKNR/GD-Barcode-1.15/source/Barcode/QRcode.pm#L349

It appears the version parameter is not required, and will default to whatever version has the necessary bits for the input words given

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2023-06-08 13:51:39 UTC
Trivial fix for a nasty problem.. Passing QA
Comment 13 Tomás Cohen Arazi 2023-06-09 11:28:00 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 14 Martin Renvoize 2023-07-12 07:01:44 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 15 Matt Blenkinsop 2023-07-12 09:36:19 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x