Used a plain HTML form for DjangoProject login (no more basic auth)

bmispelon · bmispelon · commit dfbe6dd96fca · 2024-02-01T17:04:38.000+01:00
Fixes django#95, django#61 and django#60 Also possibly django#100 and django#64
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -24,3 +24,17 @@ jobs:
           python-version: '3.7'
       - run: pip install "tinycss2>=1.2.0"
       - run: python noshadows.py --tests
+
+  tracdjangoplugin:
+    runs-on: ubuntu-latest
+    container:
+      image: python:2.7.18-buster
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+      - name: Install requirements
+        run: pip install -r requirements.txt
+      - name: Run tests
+        run: python -m django test tracdjangoplugin.tests
+        env:
+          DJANGO_SETTINGS_MODULE: tracdjangoplugin.settings_tests
diff --git a/DjangoPlugin/tracdjangoplugin/djangoauth.py b/DjangoPlugin/tracdjangoplugin/djangoauth.py
diff --git a/DjangoPlugin/tracdjangoplugin/plugins.py b/DjangoPlugin/tracdjangoplugin/plugins.py
@@ -1,11 +1,18 @@
+from urlparse import urlparse
+
 from trac.core import Component, implements
 from trac.web.chrome import INavigationContributor
-from trac.web.api import IRequestFilter, IRequestHandler
+from trac.web.api import IRequestFilter, IRequestHandler, RequestDone
+from trac.web.auth import LoginModule
 from trac.wiki.web_ui import WikiModule
 from trac.util import Markup
 from trac.util.html import tag
 from tracext.github import GitHubBrowser
 
+from django.conf import settings
+from django.contrib.auth.forms import AuthenticationForm
+from django.utils.http import is_safe_url
+
 
 class CustomTheme(Component):
     implements(IRequestFilter)
@@ -91,3 +98,45 @@ def _format_changeset_link(self, formatter, ns, chgset, label, fullmatch=None):
         return super(GitHubBrowserWithSVNChangesets, self)._format_changeset_link(
             formatter, ns, chgset, label, fullmatch
         )
+
+
+class PlainLoginComponent(Component):
+    """
+    Enable login through a plain HTML form (no more HTTP basic auth)
+    """
+
+    implements(IRequestHandler)
+
+    def match_request(self, req):
+        return req.path_info == "/login"
+
+    def process_request(self, req):
+        if req.method == "POST":
+            return self.do_post(req)
+        elif req.method == "GET":
+            return self.do_get(req)
+        else:
+            req.send_response(405)
+            raise RequestDone
+
+    def do_get(self, req):
+        return "plainlogin.html", {
+            "form": AuthenticationForm(),
+            "next": req.args.get("next", ""),
+        }
+
+    def do_post(self, req):
+        form = AuthenticationForm(data=req.args)
+        if form.is_valid():
+            req.environ["REMOTE_USER"] = form.get_user().username
+            LoginModule(self.compmgr)._do_login(req)
+            req.redirect(self._get_safe_redirect_url(req))
+        return "plainlogin.html", {"form": form, "next": req.args.get("next", "")}
+
+    def _get_safe_redirect_url(self, req):
+        host = urlparse(req.base_url).hostname
+        redirect_url = req.args.get("next", "") or settings.LOGIN_REDIRECT_URL
+        if is_safe_url(redirect_url, allowed_hosts=[host]):
+            return redirect_url
+        else:
+            return settings.LOGIN_REDIRECT_URL
diff --git a/DjangoPlugin/tracdjangoplugin/settings.py b/DjangoPlugin/tracdjangoplugin/settings.py
@@ -1,8 +1,11 @@
 import json
 import os
 
-with open(os.environ.get("SECRETS_FILE")) as handle:
-    SECRETS = json.load(handle)
+if os.environ.get("SECRETS_FILE"):
+    with open(os.environ.get("SECRETS_FILE")) as handle:
+        SECRETS = json.load(handle)
+else:
+    SECRETS = {}
 
 DEBUG = False
 
@@ -23,6 +26,6 @@
 ]
 
 
-SECRET_KEY = str(SECRETS["secret_key"])
+SECRET_KEY = str(SECRETS.get("secret_key", ""))
 
-BASIC_AUTH_REALM = "Django's Trac"
+LOGIN_REDIRECT_URL = "/"
diff --git a/DjangoPlugin/tracdjangoplugin/settings_tests.py b/DjangoPlugin/tracdjangoplugin/settings_tests.py
@@ -0,0 +1,14 @@
+from .settings import *
+
+DATABASES = {
+    "default": {
+        "ENGINE": "django.db.backends.sqlite3",
+        "NAME": "djangoproject",
+    },
+}
+
+PASSWORD_HASHERS = [
+    "django.contrib.auth.hashers.MD5PasswordHasher",
+]
+
+SECRET_KEY = "test"
diff --git a/DjangoPlugin/tracdjangoplugin/tests.py b/DjangoPlugin/tracdjangoplugin/tests.py
@@ -0,0 +1,129 @@
+from functools import partial
+
+from django.contrib.auth.forms import AuthenticationForm
+from django.contrib.auth.models import User
+from django.test import TestCase
+
+from trac.test import EnvironmentStub, MockRequest
+from trac.web.api import RequestDone
+from trac.web.main import RequestDispatcher
+
+from tracdjangoplugin.plugins import PlainLoginComponent
+
+
+class PlainLoginComponentTestCase(TestCase):
+    def setUp(self):
+        self.env = EnvironmentStub()
+        self.component = PlainLoginComponent(self.env)
+        self.request_factory = partial(MockRequest, self.env)
+
+    def test_component_matches_correct_url(self):
+        request = self.request_factory(path_info="/login")
+        self.assertTrue(self.component.match_request(request))
+
+    def test_component_doesnt_match_another_url(self):
+        request = self.request_factory(path_info="/github/login")
+        self.assertFalse(self.component.match_request(request))
+
+    def test_component(self):
+        request = self.request_factory(path_info="/login")
+        template, context = self.component.process_request(request)
+        self.assertEqual(template, "plainlogin.html")
+        self.assertFalse(context["form"].is_bound)
+
+    def assertLoginSucceeds(
+        self, username, password, check_redirect=None, extra_data=None
+    ):
+        data = {"username": username, "password": password}
+        if extra_data is not None:
+            data.update(extra_data)
+        request = self.request_factory(method="POST", path_info="/login", args=data)
+        with self.assertRaises(RequestDone):
+            self.component.process_request(request)
+
+        self.assertEqual(request.authname, "test")
+        self.assertEqual(request.status_sent, ["303 See Other"])
+        if check_redirect is not None:
+            redirect_url = request.headers_sent["Location"]
+            self.assertEqual(redirect_url, check_redirect)
+
+    def test_login_valid_user(self):
+        User.objects.create_user(username="test", password="test")
+        self.assertLoginSucceeds(username="test", password="test")
+
+    def test_login_valid_default_redirect(self):
+        self.env.config.set("trac", "base_url", "")
+        User.objects.create_user(username="test", password="test")
+        with self.settings(LOGIN_REDIRECT_URL="/test"):
+            self.assertLoginSucceeds(
+                username="test", password="test", check_redirect="/test"
+            )
+
+    def test_login_valid_with_custom_redirection(self):
+        self.env.config.set("trac", "base_url", "")
+        User.objects.create_user(username="test", password="test")
+        self.assertLoginSucceeds(
+            username="test",
+            password="test",
+            check_redirect="/test",
+            extra_data={"next": "/test"},
+        )
+
+    def test_login_valid_with_custom_redirection_with_hostname(self):
+        self.env.config.set("trac", "base_url", "http://localhost")
+        User.objects.create_user(username="test", password="test")
+        self.assertLoginSucceeds(
+            username="test",
+            password="test",
+            check_redirect="http://localhost/test",
+            extra_data={"next": "http://localhost/test"},
+        )
+
+    def test_login_valid_with_malicious_redirection(self):
+        self.env.config.set("trac", "base_url", "http://localhost")
+        User.objects.create_user(username="test", password="test")
+        with self.settings(LOGIN_REDIRECT_URL="/test"):
+            self.assertLoginSucceeds(
+                username="test",
+                password="test",
+                check_redirect="http://localhost/test",
+                extra_data={"next": "http://example.com/evil"},
+            )
+
+    def assertLoginFails(self, username, password, error_message=None):
+        if error_message is None:
+            error_message = AuthenticationForm.error_messages["invalid_login"] % {
+                "username": "username"
+            }
+
+        request = self.request_factory(
+            method="POST",
+            path_info="/login",
+            args={"username": username, "password": password},
+        )
+        template, context = self.component.process_request(request)
+        self.assertEqual(template, "plainlogin.html")
+        self.assertEqual(context["form"].errors, {"__all__": [error_message]})
+
+    def test_login_invalid_no_users(self):
+        self.assertLoginFails(username="test", password="test")
+
+    def test_login_invalid_incorrect_username(self):
+        User.objects.create_user(username="test", password="test")
+        self.assertLoginFails(username="test123", password="test")
+
+    def test_login_invalid_incorrect_password(self):
+        User.objects.create_user(username="test", password="test")
+        self.assertLoginFails(username="test", password="test123")
+
+    def test_login_invalid_incorrect_username_and_password(self):
+        User.objects.create_user(username="test", password="test")
+        self.assertLoginFails(username="test123", password="test123")
+
+    def test_login_invalid_username_uppercased(self):
+        User.objects.create_user(username="test", password="test")
+        self.assertLoginFails(username="TEST", password="test")
+
+    def test_login_invalid_inactive_user(self):
+        User.objects.create_user(username="test", password="test", is_active=False)
+        self.assertLoginFails(username="test", password="test")
diff --git a/DjangoPlugin/tracdjangoplugin/wsgi.py b/DjangoPlugin/tracdjangoplugin/wsgi.py
@@ -4,18 +4,17 @@
 
 application = trac.web.main.dispatch_request
 
+import django
+
+django.setup()
+
 # Massive hack to make Trac fast, otherwise every git call tries to close ulimit -n (1e6) fds
 # Python 3 would perform better here, but we are still on 2.7 for Trac, so leak fds for now.
 from tracopt.versioncontrol.git import PyGIT
 
 PyGIT.close_fds = False
 
-from .djangoauth import DjangoAuth
-
-application = DjangoAuth(application)
-
 trac_dsn = os.getenv("SENTRY_DSN")
-
 if trac_dsn:
     import sentry_sdk
     from sentry_sdk.integrations.wsgi import SentryWsgiMiddleware
diff --git a/trac-env/conf/trac.ini b/trac-env/conf/trac.ini
@@ -22,6 +22,7 @@ trac.ticket.roadmap.roadmapmodule = disabled
 trac.versioncontrol.web_ui.browser.browsermodule = disabled
 trac.versioncontrol.web_ui.changeset.changesetmodule = disabled
 trac.versioncontrol.web_ui.log.logmodule = disabled
+trac.web.auth.loginmodule = disabled; replaced by djangoplugin.PlainLoginComponent
 trac.wiki.web_ui.wikimodule = disabled
 tracdjangoplugin.* = enabled
 tracext.github.githubloginmodule = enabled
diff --git a/trac-env/templates/django_theme.html b/trac-env/templates/django_theme.html
@@ -28,8 +28,7 @@
           </form>
         </li>
       # else
-        <li><a href="/github/login">GitHub Login</a></li>
-        <li><a href="/login">DjangoProject Login</a></li>
+        <li><a href="/login?next=${req.path_info|urlencode()}">Login</a></li>
       # endif
       <li><a href="${req.href.prefs()}">Preferences</a></li>
       # if req.perm.has_permission('XML_RPC'):
diff --git a/trac-env/templates/plainlogin.html b/trac-env/templates/plainlogin.html
