Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

16995 Move language handling to Middleware #17122

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

m2martin
Copy link
Contributor

Fixes: #16995

Decouples language activation from login form to have it handled by CoreMiddleware which will also trigger language activation on any type of login (e.g. SSO).

Comment on lines +50 to +43
if (
request.user.is_authenticated and
settings.LANGUAGE_COOKIE_NAME not in request.COOKIES and
hasattr(request.user, 'config')
):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this is needed? IIRC the first authenticated response should always be a redirect, so I'd expect the language cookie should already be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your assumption is true when the login form or even some types of SSO is used, but not for netbox.authentication.RemoteUserBackend.

If you're using header-based authentication without that code fragment, you'll end up with the first page being displayed in the browser's default language, despite having a correct cookie.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right.

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Aug 30, 2024
@netbox-community netbox-community deleted a comment from m2martin Sep 3, 2024
@bctiemann
Copy link
Contributor

bctiemann commented Sep 10, 2024

@m2martin Could you please rebase this PR? I tried to resolve what seemed like a trivial merge conflict and found that there are a large number of tests failing due to references to settings.AUTH_EXEMPT_PATHS which does not exist, as well as missing imports (i.e. parse).

@jeremystretch
Copy link
Member

I've rebased this against develop, but it's still not clear to me what the recommended convention is for setting and renewing the language cookie.

@jeremystretch jeremystretch removed the pending closure Requires immediate attention to avoid being closed for inactivity label Sep 23, 2024
@m2martin
Copy link
Contributor Author

What exactly are you missing? Do you mean the "Django recommended way"?

@jeremystretch
Copy link
Member

Do you mean the "Django recommended way"?

Yeah. It's not clear to me what Django's recommended practice is for directly managing the language cookie. It feels like we're reinventing the wheel a bit.

It also has me wondering if we should even be using a separate cookie for the language, as opposed to session data. IIRC Django uses a separate cookie to remove the session middleware as a dependency, but obviously that doesn't apply here.

@m2martin
Copy link
Contributor Author

m2martin commented Oct 2, 2024

Hmm, I understand your concerns but I think I can't provide any satisfying answer...

AFAIK, Django has had language and sessions coupled in the past but now tries to de-couple them (remove language from sessions and fully rely on the cookie as detection mechanism).

I've found no other way to implement all possible login cases.
There are two code points where we can kick in - before and after get_response(). Because of this, there are two code blocks. On the latter, we can't change the language anymore because it has already been set and activated by other instances.
On the first, we can follow the Django-recommended way of using translation.activate().

It could be simplified if it is acceptable to have the first page-load being displayed in the wrong language.

Copy link
Contributor

github-actions bot commented Nov 2, 2024

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending closure Requires immediate attention to avoid being closed for inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Language setting not applied when signing in via SSO
3 participants