eloquence / lib.reviews

A free/libre code and information platform for reviews of anything
Creative Commons Zero v1.0 Universal
171 stars 13 forks source link

added signupLanguage query support for registration #195

Closed pnoll1 closed 6 years ago

pnoll1 commented 6 years ago

added support for signupLanguage for register and register with code routes. register with code route has not been tested.

Sets locale cookie if signupLanguage query is present and is a valid language, otherwise does nothing.

All tests passing.

pnoll1 commented 6 years ago

corrected per comments. put code in function and added check for signed in user.

manual testing shows code still works as intended and user check works.

all tests passing.

eloquence commented 6 years ago

Thanks, @pnoll1, this looks pretty solid!

Thinking about it a bit more, the risk of having a malicious page contain a <script> tag which in turn loads a resource like http://localhost/register?returnTo=/&signupLanguage=de is still too high. While we're protecting logged in users now, this would set the language for a logged out user who is unaware of this change.

How do we avoid this? I would suggest that we don't set the cookie until the form is submitted. If the user submits, regardless of whether successful or not, we can reasonably assume that they're OK with the language change. So in the POST handler, we could set the cookie - we'd just have to pass it along in the form, as we do with returnTo.

Does that make sense? I can also do that part, if you prefer.

pnoll1 commented 6 years ago

I understand what you're saying. In addition to moving the code already written, I'd need to add code to render the registration page in signupLanguage since we're not setting the cookie until the POST.

So when done, first page will load in signupLanguage for logged out users unless they register.

Definitely doable.

eloquence commented 6 years ago

Yup, I think we can even safely get rid of the req.user check if the language change is initially not persistent. It may be useful for logged in folks to check that the link they're giving to someone else is producing the expected result.

pnoll1 commented 6 years ago

Language is now set directly in render route if signupLang present and passes variables to POST registration request using same method as returnTo. Cookie is set if user completes POST.

Removed user sign in check, if we decide to add later, it should go in render to ensure signup query has no impact.

All tests passing.

pnoll1 commented 6 years ago

Fixed per comments.

Should probably add a test for registration with code. I don't have a way to test that on my dev site and it's currently how everyone registers.