Bug 33904

Summary: 2FA registration fails if library name has non-latin characters
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: AuthenticationAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: major    
Priority: P5 - low CC: dcook, dpavlin, fridolin.somers, martin.renvoize
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33880
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.01,22.11.08
Bug Depends on: 28786    
Bug Blocks:    
Attachments: Bug 33904: Add tests
Bug 33904: Fix 2FA registration when library name has non-latin characters
Bug 33904: Add tests
Bug 33904: Fix 2FA registration when library name has non-latin characters
Bug 33904: (follow-up) Perltidy
Bug 33904: Add tests
Bug 33904: Fix 2FA registration when library name has non-latin characters
Bug 33904: (follow-up) Perltidy

Description Jonathan Druart 2023-06-06 08:39:32 UTC
If the library name has non-latin characters the two-factor authentication registration fails with a JS error and a 500 server-side.

POST /api/v1/auth/two-factor/registration: unhandled exception (Mojo::Exception)<<Can't escape \x{2714}, try uri_escape_utf8() instead at /usr/share/perl5/Auth/GoogleAuth.pm line     43.
Comment 1 Jonathan Druart 2023-06-06 08:47:13 UTC
Created attachment 152038 [details] [review]
Bug 33904: Add tests
Comment 2 Jonathan Druart 2023-06-06 08:47:16 UTC
Created attachment 152039 [details] [review]
Bug 33904: Fix 2FA registration when library name has non-latin characters

If the library name contains non-latin characters, the 2FA registration
process will fail with a JS alert coming from a 500 server-side.

The problem is that Auth::GoogleAuth is expecting an already UTF8 encoded string.
We should set the encoding correctly to make Auth::GoogleAuth deal with
the URL escaping internally correctly, then decode on our own (in the
REST API controller)

Test plan:
* Modify your logged in library name and add some non-latin characters
(eg. "my ❤ library")
* Turn on TwoFactorAuthentication
* Go to your account > More > Manage 2FA
* Click the enable button
=> Notice that you see the QR code and that both "issuer" and "key id"
entries display the library name correctly.
* Test the whole 2FA process, confirm that the library name is correctly
displayed on the app you are using.
Comment 3 David Cook 2023-06-07 00:22:51 UTC
If someone else signs this one off, I'll QA it.
Comment 4 Martin Renvoize 2023-06-08 13:54:39 UTC
Created attachment 152196 [details] [review]
Bug 33904: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2023-06-08 13:54:41 UTC
Created attachment 152197 [details] [review]
Bug 33904: Fix 2FA registration when library name has non-latin characters

If the library name contains non-latin characters, the 2FA registration
process will fail with a JS alert coming from a 500 server-side.

The problem is that Auth::GoogleAuth is expecting an already UTF8 encoded string.
We should set the encoding correctly to make Auth::GoogleAuth deal with
the URL escaping internally correctly, then decode on our own (in the
REST API controller)

Test plan:
* Modify your logged in library name and add some non-latin characters
(eg. "my ❤ library")
* Turn on TwoFactorAuthentication
* Go to your account > More > Manage 2FA
* Click the enable button
=> Notice that you see the QR code and that both "issuer" and "key id"
entries display the library name correctly.
* Test the whole 2FA process, confirm that the library name is correctly
displayed on the app you are using.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2023-06-08 13:54:43 UTC
Created attachment 152198 [details] [review]
Bug 33904: (follow-up) Perltidy

With the recent introduction of a community perltidyrc, I felt it
sensible to give the module a tidy with this patch.
Comment 7 Martin Renvoize 2023-06-08 13:55:05 UTC
Tests passing and UX also passing..  signing off
Comment 8 Jonathan Druart 2023-06-08 14:01:13 UTC
(In reply to Martin Renvoize from comment #6)
> Created attachment 152198 [details] [review] [review]
> Bug 33904: (follow-up) Perltidy
> 
> With the recent introduction of a community perltidyrc, I felt it
> sensible to give the module a tidy with this patch.

I don't think we should do that. The first 2 patches will need to be backported and we don't want to confuse RMaints with unnecessary conflicts to solve.
Comment 9 Katrin Fischer 2023-06-08 21:21:19 UTC
Created attachment 152218 [details] [review]
Bug 33904: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2023-06-08 21:21:22 UTC
Created attachment 152219 [details] [review]
Bug 33904: Fix 2FA registration when library name has non-latin characters

If the library name contains non-latin characters, the 2FA registration
process will fail with a JS alert coming from a 500 server-side.

The problem is that Auth::GoogleAuth is expecting an already UTF8 encoded string.
We should set the encoding correctly to make Auth::GoogleAuth deal with
the URL escaping internally correctly, then decode on our own (in the
REST API controller)

Test plan:
* Modify your logged in library name and add some non-latin characters
(eg. "my ❤ library")
* Turn on TwoFactorAuthentication
* Go to your account > More > Manage 2FA
* Click the enable button
=> Notice that you see the QR code and that both "issuer" and "key id"
entries display the library name correctly.
* Test the whole 2FA process, confirm that the library name is correctly
displayed on the app you are using.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2023-06-08 21:21:25 UTC
Created attachment 152220 [details] [review]
Bug 33904: (follow-up) Perltidy

With the recent introduction of a community perltidyrc, I felt it
sensible to give the module a tidy with this patch.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2023-06-08 21:24:26 UTC
The perltidy patch does include a fair bit of whitespace changes. We might want to leave it off as Jonathan suggested.
Comment 13 Tomás Cohen Arazi 2023-06-09 11:28:12 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 14 Fridolin Somers 2023-06-26 12:59:06 UTC
Pushed to 23.05.x for 23.05.01
Comment 15 Matt Blenkinsop 2023-07-12 09:10:42 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x