From dfbe6dd96fca98608fb681b1c96f027e73e2bd2b Mon Sep 17 00:00:00 2001 From: Baptiste Mispelon Date: Sun, 28 Jan 2024 14:22:06 +0100 Subject: [PATCH] Used a plain HTML form for DjangoProject login (no more basic auth) Fixes #95, #61 and #60 Also possibly #100 and #64 --- .github/workflows/tests.yml | 14 ++ DjangoPlugin/tracdjangoplugin/djangoauth.py | 101 -------------- DjangoPlugin/tracdjangoplugin/plugins.py | 51 ++++++- DjangoPlugin/tracdjangoplugin/settings.py | 11 +- .../tracdjangoplugin/settings_tests.py | 14 ++ DjangoPlugin/tracdjangoplugin/tests.py | 129 ++++++++++++++++++ DjangoPlugin/tracdjangoplugin/wsgi.py | 9 +- trac-env/conf/trac.ini | 1 + trac-env/templates/django_theme.html | 3 +- trac-env/templates/plainlogin.html | 36 +++++ 10 files changed, 256 insertions(+), 113 deletions(-) delete mode 100644 DjangoPlugin/tracdjangoplugin/djangoauth.py create mode 100644 DjangoPlugin/tracdjangoplugin/settings_tests.py create mode 100644 DjangoPlugin/tracdjangoplugin/tests.py create mode 100644 trac-env/templates/plainlogin.html diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index bb96383..46e65fd 100644 --- 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 deleted file mode 100644 index 2af7f91..0000000 --- a/DjangoPlugin/tracdjangoplugin/djangoauth.py +++ /dev/null @@ -1,101 +0,0 @@ -""" -WSGI middleware that authenticates against a Django user database. - -DJANGO_SETTINGS_MODULE should point to a valid Django settings module. - -In addition, the following settings are available: - -- BASIC_AUTH_LOGIN_URL: DjangoAuth will trigger basic authentication on this - URL. Since browsers only propagate auth to resources on the same level or - below, this URL will usually be '/' without a trailing slash. - Defaults to '/login'. -- BASIC_AUTH_MESSAGE: Content of the 401 error page. Defaults to - "Authorization required.". -- BASIC_AUTH_MESSAGE_TYPE: Content type of the message. Defaults to - 'text/plain'. -- BASIC_AUTH_REALM: Authentication realm. Defaults to "Authenticate". -- BASIC_AUTH_REDIRECT_URL: DjangoAuth will redirect to this URL after login if - it isn't empty and to the HTTP Referer otherwise. If provided, it should be - an absolute URL including the domain name. Defaults to ''. - -If the user authenticates successfully, the REMOTE_USER variable is set in the -WSGI environment. - -See http://tools.ietf.org/html/rfc2617#section-2 for details on basic auth. -""" - -from base64 import b64decode - -import django -from django.conf import settings -from django.contrib.auth import authenticate -from django.core.handlers.wsgi import get_path_info -from django.db import close_old_connections -from django.utils import six - - -django.setup() - - -class DjangoAuth: - login_url = getattr(settings, "BASIC_AUTH_LOGIN_URL", "/login") - message = getattr(settings, "BASIC_AUTH_MESSAGE", "Authorization required.") - message_type = getattr(settings, "BASIC_AUTH_MESSAGE_TYPE", "text/plain") - realm = getattr(settings, "BASIC_AUTH_REALM", "Authenticate") - - def __init__(self, application): - self.application = application - - def __call__(self, environ, start_response): - try: - if get_path_info(environ) == self.login_url: - username = self.process_authorization(environ) - if username is None: - start_response( - "401 Unauthorized", - [ - ("Content-Type", self.message_type), - ("WWW-Authenticate", 'Basic realm="%s"' % self.realm), - ], - ) - return [self.message] - finally: - close_old_connections() - - return self.application(environ, start_response) - - @staticmethod - def process_authorization(environ): - # Don't override authentication information set by another component. - remote_user = environ.get("REMOTE_USER") - if remote_user is not None: - return - - authorization = environ.get("HTTP_AUTHORIZATION") - if authorization is None: - return - - if six.PY3: # because fuck you PEP 3333. - authorization = authorization.encode("iso-8859-1").decode("utf-8") - - method, _, credentials = authorization.partition(" ") - if not method.lower() == "basic": - return - - try: - credentials = b64decode(credentials.strip()) - username, _, password = credentials.partition(":") - except Exception: - return - - if authenticate(username=username, password=password) is None: - return - - remote_user = username - - if six.PY3: # because fuck you PEP 3333. - remote_user = remote_user.encode("utf-8").decode("iso-8859-1") - - environ["REMOTE_USER"] = remote_user - - return username diff --git a/DjangoPlugin/tracdjangoplugin/plugins.py b/DjangoPlugin/tracdjangoplugin/plugins.py index 52a08f3..0b1c2a3 100644 --- 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 index 436d815..b0f6246 100644 --- 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 new file mode 100644 index 0000000..1c784ae --- /dev/null +++ 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 new file mode 100644 index 0000000..0731e2a --- /dev/null +++ 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 index afcfc68..ab5403e 100644 --- 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 index 06f47ae..ed17c18 100644 --- 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 index 60860c0..a491b3f 100644 --- a/trac-env/templates/django_theme.html +++ b/trac-env/templates/django_theme.html @@ -28,8 +28,7 @@ # else -
  • GitHub Login
    • -
  • DjangoProject Login
    • +
  • Login
    • # endif
  • Preferences
    • # if req.perm.has_permission('XML_RPC'): diff --git a/trac-env/templates/plainlogin.html b/trac-env/templates/plainlogin.html new file mode 100644 index 0000000..b0f24f9 --- /dev/null +++ b/trac-env/templates/plainlogin.html @@ -0,0 +1,36 @@ +# extends 'layout.html' + +# block title + Login ${ super() } +# endblock title + +# block content +

    Choose how you want to log in

    + +