Skip to content

Conversation

@ThomasKalverda
Copy link

When the user is redirected to any view of HIdP, the preferred language should be taken into account. However, there are multiple ways and places where users can define their preferred language, so it's hard for us to determine what takes precedence.

The solution we came up with was to use the ui_locales query param as source of truth when it is available, as it likely comes from the browser. We check it in middleware and set the django_language cookie based on that.

An added benefit is that all of HIdP's views are also translated to comply with the browser language (if the language is supported)

There was also a related bug in Django Oauth Toolkit, that is fixed with this middleware.

@linear
Copy link

linear bot commented Aug 22, 2024

HIDP-51 Investigate bug regarding ui_locales querystring param

The AT project sends a param that indicates the preferred client locale, this makes DOT (or the underlying library) crash. Investigate why, report an issue and fix it locally/upstream.

@jaap3
Copy link

jaap3 commented Aug 26, 2024

I know this solution works, but I want to hold off with merging it. Two reasons:

  1. The issue should be reported upstream, so it can be referred to in the workaround. I haven't been able to reproduce it in a clean test setup, once I have I'll add a reference here.
  2. The potential clash between HIdP's use of the django_language cookie and "other" use is nagging me. I'll spend a bit more time thinking about it to see if it truly is an issue and if there are ways to work around it.

@jaap3
Copy link

jaap3 commented Aug 26, 2024

I was able to reproduce and have reported the issue upstream: django-oauth/django-oauth-toolkit#1468

@jaap3 jaap3 force-pushed the HIDP-51-add-ui-locales-middleware branch from c5f1c08 to 80b2d78 Compare August 26, 2024 14:42
@jaap3
Copy link

jaap3 commented Aug 27, 2024

Upstream PRs:

@jaap3 jaap3 force-pushed the HIDP-51-add-ui-locales-middleware branch from 80b2d78 to 33d71fb Compare August 27, 2024 12:08
@jaap3
Copy link

jaap3 commented Aug 27, 2024

I've updated the PR to make this middleware independent of Django's language cookie and LocaleMiddleware. I think it's better to just have a stand-alone HIdP language cookie that is only used for this specific purpose. It also makes configuration a bit easier as this middleware is no-longer depending on LocalMiddelware running after it.

@jaap3 jaap3 force-pushed the HIDP-51-add-ui-locales-middleware branch from 33d71fb to 4025505 Compare August 27, 2024 12:15
@bheesink
Copy link
Member

Will this PR be reverted when the two PR's in oAuthToolkit and oAuthLib are merged and released? Or do we always want to use the cookie hidp_language in HIdP?

@jaap3
Copy link

jaap3 commented Aug 28, 2024

The language selection/activation will remain. The workaround can be removed if things are fixed upstream.

We will probably keep the cookie around, as it’s the easiest way to make sure the translation stays active during the authentication/registration process (KeyCloak does something similar). We might find a better way to do this later, but for now I think this is the best option.

@jaap3 jaap3 merged commit 7980ecc into main Aug 28, 2024
@jaap3 jaap3 deleted the HIDP-51-add-ui-locales-middleware branch August 28, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants