Bug 33904 - 2FA registration fails if library name has non-latin characters
Summary: 2FA registration fails if library name has non-latin characters
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 28786
Blocks:
  Show dependency treegraph
 
Reported: 2023-06-06 08:39 UTC by Jonathan Druart
Modified: 2023-12-28 20:47 UTC (History)
4 users (show)

See Also:
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


Attachments
Bug 33904: Add tests (2.03 KB, patch)
2023-06-06 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33904: Fix 2FA registration when library name has non-latin characters (2.37 KB, patch)
2023-06-06 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33904: Add tests (2.09 KB, patch)
2023-06-08 13:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33904: Fix 2FA registration when library name has non-latin characters (2.43 KB, patch)
2023-06-08 13:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33904: (follow-up) Perltidy (2.97 KB, patch)
2023-06-08 13:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33904: Add tests (2.15 KB, patch)
2023-06-08 21:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33904: Fix 2FA registration when library name has non-latin characters (2.48 KB, patch)
2023-06-08 21:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33904: (follow-up) Perltidy (3.03 KB, patch)
2023-06-08 21:21 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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