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.
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)
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)
*** Bug 32010 has been marked as a duplicate of this bug. ***
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.
Tests ran 50x without failure. Looks good!
I cannot run it on the laptop right now, but I've pushed it. Will continue tomorrow.
(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.
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 :)
Tested both Enable and Enforced btw.
GD::Barcode::QRcode elsif ($iTotalBits > $iMaxDatBits){ die "Overflow error. version $oSelf->{Version}\n" . "total bits: $iTotalBits max bits: $iMaxDatBits\n"; }
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.
(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).
(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.
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?
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";
(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.
Try with a very long userid.
(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 );
(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..
(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.
(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
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
Status now reflects where we are.
(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!)
(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).
(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.
Backported to 22.05.x for upcoming 22.05.07 release
depends on 2FA auth, not present in 21.11.x branch. Not backporting this one.