Bug 32011 - 2FA - Problem with qr_code generation
Summary: 2FA - Problem with qr_code generation
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 29873
Blocks:
  Show dependency treegraph
 
Reported: 2022-10-27 08:50 UTC by Jonathan Druart
Modified: 2022-11-14 10:35 UTC (History)
5 users (show)

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


Attachments
Bug 32011: 2FA - Prevent qr_code to not be generated (1.92 KB, patch)
2022-10-27 08:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32011: 2FA - Prevent qr_code to not be generated (1.92 KB, patch)
2022-10-27 09:03 UTC, Jonathan Druart
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-10-27 08:50:27 UTC
Looks like an major bug is caught by this selenium test:

STRACE: /usr/share/perl5/Try/Tiny.pm:123 in Selenium::Remote::Driver::catch {...} 
        /usr/share/perl5/Selenium/Remote/Driver.pm:361 in Try::Tiny::try        (eval 541):1 in Selenium::Remote::Driver::__ANON__
        (eval 543):2 in Selenium::Remote::Driver::__ANON__
        /usr/share/perl5/Selenium/Remote/Driver.pm:1058 in Selenium::Remote::Driver::_execute_command        /kohadevbox/koha/t/lib/Selenium.pm:203 in Selenium::Remote::Driver::execute_script
        t/db_dependent/selenium/authentication_2fa.t:264 in t::lib::Selenium::wait_for_ajax
        /usr/share/perl/5.32/Test/Builder.pm:334 in main::__ANON__
        /usr/share/perl/5.32/Test/Builder.pm:334 in (eval)        /usr/share/perl/5.32/Test/More.pm:809 in Test::Builder::subtest
        t/db_dependent/selenium/authentication_2fa.t:291 in Test::More::subtest
    # Looks like you planned 7 tests but ran 2.t/db_dependent/selenium/authentication_2fa.t .. 4/5 
#   Failed test 'Enforce 2FA setup on first login'
#   at t/db_dependent/selenium/authentication_2fa.t line 291.Error while executing command: unexpected alert open: Dismissed user prompt dialog: [object Object] at /usr/share/perl5/Selenium/Remote/Driver.pm line 411.
 at /usr/share/perl5/Selenium/Remote/Driver.pm line 356.

There is a 500 in the logs:
 "POST /api/v1/app.pl/api/v1/auth/two-factor/registration HTTP/1.1" 500

That is caused by
[ERROR] POST /api/v1/auth/two-factor/registration: unhandled exception (Mojo::Exception)<<Overflow error. version 8
total bits: 1268  max bits: 1232>>

It's coming from GD::Barcode, in my understanding we should increase ModuleSize.
Comment 1 Jonathan Druart 2022-10-27 08:55:38 UTC
Created attachment 142696 [details] [review]
Bug 32011: 2FA - Prevent qr_code to not be generated

Caught by selenium/authentication_2fa.t

 #   Failed test 'Enforce 2FA setup on first login'
 #   at t/db_dependent/selenium/authentication_2fa.t line 291.Error while executing command: unexpected alert open: Dismissed user prompt dialog: [object Object] at /usr/share/perl5/Selenium/Remote/Driver.pm line 411.
 at /usr/share/perl5/Selenium/Remote/Driver.pm line 356.

There is a 500 in the logs:
 "POST /api/v1/app.pl/api/v1/auth/two-factor/registration HTTP/1.1" 500

That is caused by
[ERROR] POST /api/v1/auth/two-factor/registration: unhandled exception (Mojo::Exception)<<Overflow error. version 8
total bits: 1268  max bits: 1232>>

It's coming from GD::Barcode, in my understanding we should increase ModuleSize.

Test plan:
Remove all other subtests from the selenium to speed up its exec, then
run it in a loop. Without this patch the tests fail quite often (1/10),
now it should not fail with this error (maybe another one, see bug
32010)
Comment 2 Jonathan Druart 2022-10-27 09:03:15 UTC
Created attachment 142697 [details] [review]
Bug 32011: 2FA - Prevent qr_code to not be generated

Caught by selenium/authentication_2fa.t

 #   Failed test 'Enforce 2FA setup on first login'
 #   at t/db_dependent/selenium/authentication_2fa.t line 291.Error while executing command: unexpected alert open: Dismissed user prompt dialog: [object Object] at /usr/share/perl5/Selenium/Remote/Driver.pm line 411.
 at /usr/share/perl5/Selenium/Remote/Driver.pm line 356.

There is a 500 in the logs:
 "POST /api/v1/app.pl/api/v1/auth/two-factor/registration HTTP/1.1" 500

That is caused by
[ERROR] POST /api/v1/auth/two-factor/registration: unhandled exception (Mojo::Exception)<<Overflow error. version 8
total bits: 1268  max bits: 1232>>

It's coming from GD::Barcode, in my understanding we should increase 'Version'.

Test plan:
Remove all other subtests from the selenium to speed up its exec, then
run it in a loop. Without this patch the tests fail quite often (1/10),
now it should not fail with this error (maybe another one, see bug
32010)
Comment 3 Jonathan Druart 2022-10-27 09:04:32 UTC
*** Bug 32010 has been marked as a duplicate of this bug. ***
Comment 4 Jonathan Druart 2022-10-27 09:06:40 UTC
Sometimes seeing a 500 in Koha logs which is suspect:

Can't use string ("Koha::Patron") as a HASH ref while "strict refs" in use at /kohadevbox/koha/Koha/Object.pm line 830                                                                                                       Koha::Object::_columns('Koha::Patron') called at /kohadevbox/koha/Koha/Object.pm line 858                                                                                                                    
        Koha::Object::AUTOLOAD('Koha::Patron', 'HASH(0x5579840b17b8)') called at /kohadevbox/koha/C4/Auth.pm line 1772                                                                                                       C4::Auth::check_cookie_auth('b712cd069e4808fa8a5da0e29d098b30', undef, 'HASH(0x557983e6ac28)') called at /kohadevbox/koha/C4/Auth.pm line 852                                                                
        C4::Auth::checkauth('CGI=HASH(0x557986d44b18)', 0, 'HASH(0x557986cf0118)', 'intranet', undef, 'intranet-main.tt') called at /kohadevbox/koha/C4/Auth.pm line 189                                             
        C4::Auth::get_template_and_user('HASH(0x557986d0b648)') called at /kohadevbox/koha/mainpage.pl line 41  

But not related to this change.
Comment 5 Jonathan Druart 2022-10-27 09:07:40 UTC
Tests ran 50x without failure. Looks good!
Comment 6 Tomás Cohen Arazi 2022-10-27 21:22:14 UTC
I cannot run it on the laptop right now, but I've pushed it. Will continue tomorrow.
Comment 7 Marcel de Rooy 2022-10-28 06:11:18 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> I cannot run it on the laptop right now, but I've pushed it. Will continue
> tomorrow.

I dont understand that. I am looking now here. But this is not just a failing test. This is a module change.
Comment 8 Marcel de Rooy 2022-10-28 06:18:54 UTC
Without this patch on commit fd7bc8a9b8de5a8fdea36b046e4621760366dbf0, 2FA seems to work just fine on Debian 11 via the interface.

The Selenium stuff is another story. But it should not dictate us to make changes just to pass Selenium imo.

Jonathan: Please provide some more details on the system you are running on. And could you confirm that the interface does not work either anymore on your system? If not, this is not a major bug. But maybe just a Selenium issue?

I wouldnt mind not having these Selenium tests btw. But I could be biased :)
Comment 9 Marcel de Rooy 2022-10-28 06:23:12 UTC
Tested both Enable and Enforced btw.
Comment 10 Marcel de Rooy 2022-10-28 06:34:13 UTC
GD::Barcode::QRcode

elsif ($iTotalBits > $iMaxDatBits){
        die "Overflow error. version $oSelf->{Version}\n" . 
            "total bits: $iTotalBits  max bits: $iMaxDatBits\n";
    }
Comment 11 Jonathan Druart 2022-10-28 06:47:12 UTC
Yes, it's coming from GD::Barcode::QRcode

 310 sub _calcVersion($$$) {
 311     my ($oSelf, $iTtlBits, $raPlusWords) = @_;
 312 
 313     my %hMaxDatBits= (
 314         'M' =>[ 0,
 315           128,   224,   352,   512,   688,   864,   992,  1232,  1456,  1728,

So the patch jumps from 1232 to 1728 (in my tests the value was almost always < ~1350, but I saw it failed with 1456.
Comment 12 Jonathan Druart 2022-10-28 06:47:58 UTC
(In reply to Marcel de Rooy from comment #7)
> (In reply to Tomás Cohen Arazi from comment #6)
> > I cannot run it on the laptop right now, but I've pushed it. Will continue
> > tomorrow.
> 
> I dont understand that. I am looking now here. But this is not just a
> failing test. This is a module change.

Not in my understand, not a module change:
Version
    Version ie. size of barcode image (Default = auto select).
Comment 13 Jonathan Druart 2022-10-28 06:51:06 UTC
(In reply to Marcel de Rooy from comment #8)
> Without this patch on commit fd7bc8a9b8de5a8fdea36b046e4621760366dbf0, 2FA
> seems to work just fine on Debian 11 via the interface.
> 
> The Selenium stuff is another story. But it should not dictate us to make
> changes just to pass Selenium imo.

It's failing randomly, so you may not notice it via the interface with only a couple of tries.

> Jonathan: Please provide some more details on the system you are running on.
> And could you confirm that the interface does not work either anymore on
> your system? If not, this is not a major bug. But maybe just a Selenium
> issue?

I am pretty sure it's not a selenium issue, it's coming from the Koha::REST::V1::TwoFactorAuth->registration. If the selenium test make it fails, a normal browser can make it fail.

> I wouldnt mind not having these Selenium tests btw. But I could be biased :)

Lol, they are very strong and caught a bug here, so... We have some shitty selenium/www tests, but those ones are great IMO.
Comment 14 Marcel de Rooy 2022-10-28 07:04:14 UTC
Doing some debugging now with:

use Modern::Perl;
use Convert::Base32;
use GD::Barcode;
use Koha::AuthUtils;
my $duplicate_secret = 4;
foreach( 1.. 50) {
    my $secret = Koha::AuthUtils::generate_salt( 'weak', 16 );
    $secret = $secret x $duplicate_secret;
    my $otpauth = encode_base32($secret);
    print "$otpauth\n";
    GD::Barcode->new( 'QRcode', $otpauth, { Ecc => 'M', Version => 10, ModuleSize => 4 } );
}

The problem is the length of the secret. I think that you have a very long secret?
Comment 15 Marcel de Rooy 2022-10-28 07:05:27 UTC
If I am corrent, a normal secret will give values like:
Version 10 Module  4
L104: 228 1728
Version 8 Module 4
L104: 220 1232

where L104 is a debug line in QRcode.pm:
warn "L104: $iTotalBits $iMaxDatBits\n";
Comment 16 Marcel de Rooy 2022-10-28 07:06:24 UTC
(In reply to Jonathan Druart from comment #13)
> Lol, they are very strong and caught a bug here, so... We have some shitty
> selenium/www tests, but those ones are great IMO.

No reason to overreact imo.
Comment 17 Jonathan Druart 2022-10-28 07:09:03 UTC
Try with a very long userid.
Comment 18 Jonathan Druart 2022-10-28 07:10:40 UTC
(In reply to Jonathan Druart from comment #17)
> Try with a very long userid.


otpauth://totp/QcQ4WNT:QcQ4WNT_QAQDfB8?secret=3ml253y4p3er7axhrwdlz6by2e&issuer=QcQ4WNT

vs

otpauth://totp/BtwjEx:BtwjEx_wp9YPWXB91qaOkkwuxT9GydFBV9cMeUlHOLMDrn0?secret=qxiff74gn7jtdiengrfnpreywu&issuer=BtwjEx

And 
 78     my $issuer = $patron->library->branchname;
 79     my $key_id = sprintf "%s_%s",
 80       $issuer, ( $patron->email || $patron->userid );
Comment 19 Jonathan Druart 2022-10-28 07:12:20 UTC
(In reply to Marcel de Rooy from comment #16)
> (In reply to Jonathan Druart from comment #13)
> > Lol, they are very strong and caught a bug here, so... We have some shitty
> > selenium/www tests, but those ones are great IMO.
> 
> No reason to overreact imo.

On the fact that these selenium tests are great? Or that we have several bad ones? I don't understand what you mean..
Comment 20 Marcel de Rooy 2022-10-28 07:13:04 UTC
(In reply to Jonathan Druart from comment #17)
> Try with a very long userid.

Yeah, still looking for that spot in the code where we build the secret ;) But its length is the culprit.
Comment 21 Marcel de Rooy 2022-10-28 07:15:17 UTC
(In reply to Jonathan Druart from comment #18)

> otpauth://totp/BtwjEx:
> BtwjEx_wp9YPWXB91qaOkkwuxT9GydFBV9cMeUlHOLMDrn0?secret=qxiff74gn7jtdiengrfnpr
> eywu&issuer=BtwjEx

otpauth://totp/BtwjEx:BtwjEx_wp9YPWXB91qaOkkwuxT9GydFBV9cMeUlHOLMDrn0?secret=qxiff74gn7jtdiengrfnpreywu&issuer=BtwjEx
L104: 956 1728
Comment 22 Marcel de Rooy 2022-10-28 07:17:51 UTC
Okay, convinced :) This will probably get us out of trouble in most cases. Perhaps we should have a look at limiting the secret length at some point.

So Selenium is just fantastic :) Ugghh
Comment 23 Marcel de Rooy 2022-10-28 07:18:51 UTC
Status now reflects where we are.
Comment 24 Victor Grousset/tuxayo 2022-10-28 16:00:58 UTC
(In reply to Marcel de Rooy from comment #22)
> So Selenium is just fantastic :) Ugghh

XD, well in this case it helped spot a random wrong behavior in Koha IIUC. But for other thing tests themselves randomly fail. So Ugghh!

(the use of wait_for_ajax seems to help so thing get better!)
Comment 25 Tomás Cohen Arazi 2022-10-28 19:27:38 UTC
(In reply to Marcel de Rooy from comment #7)
> (In reply to Tomás Cohen Arazi from comment #6)
> > I cannot run it on the laptop right now, but I've pushed it. Will continue
> > tomorrow.
> 
> I dont understand that. I am looking now here. But this is not just a
> failing test. This is a module change.

I trusted Jonathan knows what he's doing and I don't run Selenium on ktd on the laptop because it always drains my battery (macOS).
Comment 26 Tomás Cohen Arazi 2022-10-28 19:28:14 UTC
(In reply to Marcel de Rooy from comment #23)
> Status now reflects where we are.

Thanks :-D

I was waiting for more reviews and debugging to take place today.
Comment 27 Lucas Gass 2022-10-31 17:21:10 UTC
Backported to 22.05.x for upcoming 22.05.07 release
Comment 28 Arthur Suzuki 2022-11-14 10:35:10 UTC
depends on 2FA auth, not present in 21.11.x branch.
Not backporting this one.