grnet / djnro

DjNRO hits the decks of eduroam database management
http://djnro.grnet.gr/
Other
10 stars 21 forks source link

Fix broken language selection for non-https #34

Closed zmousm closed 7 years ago

zmousm commented 7 years ago

Override django.views.i18n.set_language() to fix broken language selection via session storage for non-https when SESSION_COOKIE_SECURE=True

zmousm commented 7 years ago

@vladimir-mencl-eresearch please check

vladimir-mencl-eresearch commented 7 years ago

Hi Zenon,

Thanks - I've just had a look. I did not run into this issue in my setups, as I'm also configuring Apache to redirect all plain HTTP requests to HTTPS.

But, I understand exposing plain HTTP may also be desired.

I'm not sure how serious this issue was: for plain HTTP, was language selection not working at all, or was it just not propagating from HTTPS?

This fix looks OK to me. It has a downside it sets the language in two places - that's a potential source for confusing behavior. (E.g., a user sets a language over https, it gets set in both cookie and session. Then reverts this selection over plain http - it reverts it only in cookie. Then goes back to https and sees the old language selection there. This might be an undesired side-effect).

I've just checked: get_language_from_request in django/utils/translation/trans_real.py picks the language in the session first, cookie comes next (and HTTP_ACCEPT_LANGUAGE after that). So the above scenario could happen - but given the browser preferred language is in the game as well anyway, there would be sources of confusion regardless, so this might not be any issue.

On the technical side, looks all good to me - and the only concerns I have are with possible user confusion as per above - but fine with me.

Cheers, Vlad

zmousm commented 7 years ago

Thanks - I've just had a look. I did not run into this issue in my setups, as I'm also configuring Apache to redirect all plain HTTP requests to HTTPS.

Yes I suppose this is quite common nowadays, however I would not want to impose https-only due to something as simple as language selection.

I'm not sure how serious this issue was: for plain HTTP, was language selection not working at all, or was it just not propagating from HTTPS?

It is not working at all for plain http. The session cookie is created but it is marked secure, so a browser won't send it back if the request is not https.

This fix looks OK to me. It has a downside it sets the language in two places - that's a potential source for confusing behavior. (E.g., a user sets a language over https, it gets set in both cookie and session. Then reverts this selection over plain http - it reverts it only in cookie. Then goes back to https and sees the old language selection there. This might be an undesired side-effect).

I also pondered about this, as I first wanted and tried to do either session or language cookie (like flipping a switch) but I soon realized the inconsistent state problems that could entail. The revert you describe won't happen now as this view will always set the chosen language in both places (btw going back to http from https is quite difficult if you implement HSTS); furthermore such parts of session contents are carried over even when you log out (Django apparently creates a new session).

It is somewhat redundant though so we could opt to just ditch session storage in favor of the language cookie. I preferred not to go that way as apparently Django seems to be moving towards consolidating such things in the user session (the preference towards the session storage is one cause of breakage in Django 1.8), so maybe in some (distant) future the language cookie is deprecated and we'll just have to live with that. However if you prefer to only use the language cookie for now, I'm also happy with that.

zmousm commented 7 years ago

@vladimir-mencl-eresearch I am merging this as-is; if you believe we should ditch session storage, please follow up.

vladimir-mencl-eresearch commented 7 years ago

Hi Zenon,

Sorry about going silent - I was dragged into other projects. I think this is OK for the situation.

Cheers, Vlad