Description
Jonathan Druart
2023-06-06 08:39:32 UTC
Created attachment 152038 [details] [review] Bug 33904: Add tests 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. If someone else signs this one off, I'll QA it. Created attachment 152196 [details] [review] Bug 33904: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> 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. Tests passing and UX also passing.. signing off (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. 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> 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> 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> The perltidy patch does include a fair bit of whitespace changes. We might want to leave it off as Jonathan suggested. Pushed to master for 23.11. Nice work everyone, thanks! Pushed to 23.05.x for 23.05.01 Nice work everyone! Pushed to oldstable for 22.11.x |