-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: replace custom middleware with Django's LoginRequiredMiddleware #16956
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
Conversation
Replace Weblate's custom RequireLoginMiddleware with Django 5.1's built-in LoginRequiredMiddleware to simplify authentication handling. - Remove weblate.accounts.middleware.RequireLoginMiddleware class - Use django.contrib.auth.middleware.LoginRequiredMiddleware when REQUIRE_LOGIN=True - Add @login_not_required decorator to git_export view for HTTP Basic Auth - Remove obsolete LOGIN_REQUIRED_URLS and LOGIN_REQUIRED_URLS_EXCEPTIONS settings - Update tests to verify Django middleware behavior - Deprecate Docker environment variables for URL exceptions All public views already use @login_not_required decorator as of Django 5.1. Closes WeblateOrg#16934
for more information, see https://pre-commit.ci
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
nijel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good once the ruff and mypy errors are fixed.
| def public_view(self, request) -> str: | ||
| """A public view marked with @login_not_required decorator.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def public_view(self, request) -> str: | |
| """A public view marked with @login_not_required decorator.""" | |
| def public_view(self, request: HttpRequest) -> str: | |
| """View not requiring login.""" |
| def protected_view(self, request) -> str: | ||
| """A protected view without the @login_not_required decorator.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def protected_view(self, request) -> str: | |
| """A protected view without the @login_not_required decorator.""" | |
| def protected_view(self, request: HttpRequest) -> str: | |
| """View requiring login.""" |
|
Did you actually test running with the middleware?
|
- Fix docstrings to use imperative mood (D401) - Add HttpRequest type hints to test methods - Add auth.E013 to SILENCED_SYSTEM_CHECKS for custom auth middleware - Add @login_not_required decorator to WeblateLoginView and SecondFactorLoginView to prevent redirect loops These changes fix the ruff linting errors and resolve the login redirect loop issue identified during testing.
for more information, see https://pre-commit.ci
|
The remaining bit is /admin/login/ which should also work without requiring login (it always allows local login even if it is disabled for user authentication). |
Add @login_not_required decorator to WeblateAdminSite.login method to ensure /admin/login/ is accessible even when REQUIRE_LOGIN=True. This allows local admin login to work as intended, supporting the Django admin's authentication flow.
Co-authored-by: Michal Čihař <[email protected]>
Removed documentation for URL exceptions related to authentication.
Updated documentation to reflect changes in version 5.15 regarding the reliance on Django middleware.
for more information, see https://pre-commit.ci
|
Merged, thanks for your contribution! I had to do some documentation adjustments before. |
Replace custom middleware with Django's LoginRequiredMiddleware
Description
Replace Weblate's custom
RequireLoginMiddlewarewith Django 5.1's built-inLoginRequiredMiddlewareto simplify authentication handling and reduce maintenance burden.Changes
weblate.accounts.middleware.RequireLoginMiddlewareclass (~75 lines)django.contrib.auth.middleware.LoginRequiredMiddlewarewhenREQUIRE_LOGIN=True@login_not_requireddecorator togit_exportview for HTTP Basic Auth compatibilityLOGIN_REQUIRED_URLSandLOGIN_REQUIRED_URLS_EXCEPTIONSsettingsweblate.accounts.tests.test_middlewareto verify Django middleware behaviorWEBLATE_LOGIN_REQUIRED_URLS_EXCEPTIONS,WEBLATE_ADD_LOGIN_REQUIRED_URLS_EXCEPTIONS, andWEBLATE_REMOVE_LOGIN_REQUIRED_URLS_EXCEPTIONSdocs/changes.rstwith compatibility notesImplementation Details
All public views throughout the codebase already use the
@login_not_requireddecorator (introduced in Django 5.1), making this migration straightforward. The middleware is conditionally added only whenREQUIRE_LOGIN=True, maintaining backward compatibility.The Git exporter view required special attention as it implements HTTP Basic Authentication independently. It now uses
@login_not_requiredto bypass the login requirement while maintaining its own authentication logic.Testing
LoginRequiredMiddlewarebehavior/healthz/,/api/,/widgets/, etc.)REQUIRE_LOGIN=TrueDocumentation
Closes #16934