From 09843d2fb2427ccbd0527fb6894e6795bfef0bf4 Mon Sep 17 00:00:00 2001 From: Hugo Rodger-Brown Date: Mon, 22 Mar 2021 19:46:45 +0000 Subject: [PATCH] Remove custom token error handling (#49) Token errors now raise a PermissionDenied error and are handled by Django. --- .github/workflows/tox.yml | 2 +- CHANGELOG | 7 ++ pyproject.toml | 6 +- request_token/admin.py | 3 +- request_token/apps.py | 20 ----- request_token/middleware.py | 26 ++---- .../0012_delete_requesttokenerrorlog.py | 16 ++++ request_token/models.py | 90 ++----------------- request_token/settings.py | 28 +----- tests/settings.py | 11 ++- tests/templates/403.html | 3 - tests/test_apps.py | 16 ---- tests/test_middleware.py | 18 ++-- tests/test_models.py | 36 +------- tests/urls.py | 2 + tox.ini | 4 +- 16 files changed, 67 insertions(+), 221 deletions(-) create mode 100644 request_token/migrations/0012_delete_requesttokenerrorlog.py delete mode 100644 tests/templates/403.html delete mode 100644 tests/test_apps.py diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index b90af8c..27bf9c1 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -38,7 +38,7 @@ jobs: strategy: matrix: python: [3.8,3.9] - django: [31,master] + django: [31,main] env: TOXENV: py${{ matrix.python }}-django${{ matrix.django }} diff --git a/CHANGELOG b/CHANGELOG index 127a760..b7f1e9f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,13 @@ All notable changes to this project will be documented in this file. +## [0.2] - 2021-03-22 + +### Removed + +- Logging of token use errors - this has never worked +- Custom 403_TEMPLATE use (and setting) + ## [0.12] - 2020-08-17 No functional changes - just updating classifiers and CI support. diff --git a/pyproject.toml b/pyproject.toml index e3a456d..14d12a3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "django-request-token" -version = "0.13" +version = "0.2.b0" description = "JWT-backed Django app for managing querystring tokens." license = "MIT" authors = ["YunoJuno "] @@ -9,6 +9,7 @@ readme = "README.rst" homepage = "https://github.com/yunojuno/django-request-token" repository = "https://github.com/yunojuno/django-request-token" classifiers = [ + "Development Status :: 4 - Beta", "Environment :: Web Environment", "Framework :: Django :: 2.2", "Framework :: Django :: 3.0", @@ -25,7 +26,6 @@ packages = [{ include = "request_token" }] python = "^3.7" django = "^2.2 || ^3.0" pyjwt = "^2.0" -sqlparse = "*" [tool.poetry.dev-dependencies] tox = "*" @@ -41,7 +41,7 @@ flake8-docstrings = "*" isort = "*" mypy = "*" pre-commit = "*" -black = "==19.10b0" +black = { version="*", allow-prereleases=true } [build-system] requires = ["poetry>=0.12"] diff --git a/request_token/admin.py b/request_token/admin.py index 7733366..4606cac 100644 --- a/request_token/admin.py +++ b/request_token/admin.py @@ -7,7 +7,7 @@ from django.utils.safestring import mark_safe from django.utils.timezone import now as tz_now -from .models import RequestToken, RequestTokenErrorLog, RequestTokenLog +from .models import RequestToken, RequestTokenLog def pretty_print(data: Optional[dict]) -> Optional[str]: @@ -108,5 +108,4 @@ class RequestTokenErrorLogAdmin(admin.ModelAdmin): admin.site.register(RequestToken, RequestTokenAdmin) -admin.site.register(RequestTokenErrorLog, RequestTokenErrorLogAdmin) admin.site.register(RequestTokenLog, RequestTokenLogAdmin) diff --git a/request_token/apps.py b/request_token/apps.py index 4f21657..15b0ece 100644 --- a/request_token/apps.py +++ b/request_token/apps.py @@ -1,10 +1,6 @@ from __future__ import annotations from django.apps import AppConfig -from django.core.exceptions import ImproperlyConfigured -from django.template import TemplateDoesNotExist, loader - -from .settings import FOUR03_TEMPLATE class RequestTokenAppConfig(AppConfig): @@ -12,19 +8,3 @@ class RequestTokenAppConfig(AppConfig): name = "request_token" verbose_name = "JWT Request Tokens" - - def ready(self) -> None: - """Validate config and connect signals.""" - super(RequestTokenAppConfig, self).ready() - if FOUR03_TEMPLATE: - check_template(FOUR03_TEMPLATE) - - -def check_template(template: str) -> None: - """Check for the existance of the custom 403 page.""" - try: - loader.get_template(template) - except TemplateDoesNotExist: - raise ImproperlyConfigured( - f"Custom request token template does not exist: '{template}'" - ) diff --git a/request_token/middleware.py b/request_token/middleware.py index 69192dd..97037ad 100644 --- a/request_token/middleware.py +++ b/request_token/middleware.py @@ -2,17 +2,16 @@ import json import logging +import sys from typing import Callable -from django.core.exceptions import ImproperlyConfigured -from django.http import HttpResponseForbidden +from django.core.exceptions import ImproperlyConfigured, PermissionDenied from django.http.request import HttpRequest from django.http.response import HttpResponse -from django.template import loader from jwt.exceptions import InvalidTokenError from .models import RequestToken -from .settings import FOUR03_TEMPLATE, JWT_QUERYSTRING_ARG +from .settings import JWT_QUERYSTRING_ARG from .utils import decode logger = logging.getLogger(__name__) @@ -93,20 +92,5 @@ def process_exception( ) -> HttpResponse: """Handle all InvalidTokenErrors.""" if isinstance(exception, InvalidTokenError): - logger.exception("JWT request token error") - response = _403(request, exception) - if getattr(request, "token", None): - request.token.log(request, response, error=exception) - return response - - -def _403(request: HttpRequest, exception: Exception) -> HttpResponseForbidden: - """Render HttpResponseForbidden for exception.""" - if FOUR03_TEMPLATE: - html = loader.render_to_string( - template_name=FOUR03_TEMPLATE, - context={"token_error": str(exception), "exception": exception}, - request=request, - ) - return HttpResponseForbidden(html, reason=str(exception)) - return HttpResponseForbidden(reason=str(exception)) + logger.error("JWT request token error", exc_info=sys.exc_info()) + raise PermissionDenied("Invalid request token.") diff --git a/request_token/migrations/0012_delete_requesttokenerrorlog.py b/request_token/migrations/0012_delete_requesttokenerrorlog.py new file mode 100644 index 0000000..45167d0 --- /dev/null +++ b/request_token/migrations/0012_delete_requesttokenerrorlog.py @@ -0,0 +1,16 @@ +# Generated by Django 3.1.7 on 2021-03-22 18:56 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("request_token", "0011_update_model_meta_options"), + ] + + operations = [ + migrations.DeleteModel( + name="RequestTokenErrorLog", + ), + ] diff --git a/request_token/models.py b/request_token/models.py index c49bedc..6d5cf3b 100644 --- a/request_token/models.py +++ b/request_token/models.py @@ -11,11 +11,11 @@ from django.http import HttpRequest, HttpResponse from django.utils.timezone import now as tz_now from django.utils.translation import gettext_lazy as _lazy -from jwt.exceptions import InvalidAudienceError, InvalidTokenError +from jwt.exceptions import InvalidAudienceError from .compat import JSONField from .exceptions import MaxUseError -from .settings import DEFAULT_MAX_USES, JWT_SESSION_TOKEN_EXPIRY, LOG_TOKEN_ERRORS +from .settings import DEFAULT_MAX_USES, JWT_SESSION_TOKEN_EXPIRY from .utils import encode, to_seconds logger = logging.getLogger(__name__) @@ -302,28 +302,8 @@ def authenticate(self, request: HttpRequest) -> HttpRequest: return self._auth_is_authenticated(request) @transaction.atomic - def log( - self, - request: HttpRequest, - response: HttpResponse, - error: Optional[InvalidTokenError] = None, - ) -> RequestTokenLog: - """ - Record the use of a token. - - This is used by the decorator to log each time someone uses the token, - or tries to. Used for reporting, diagnostics. - - Args: - request: the HttpRequest object that used the token, from which the - user, ip and user-agenct are extracted. - response: the corresponding HttpResponse object, from which the status - code is extracted. - error: an InvalidTokenError that gets logged as a RequestTokenError. - - Returns a RequestTokenUse object. - - """ + def log(self, request: HttpRequest, response: HttpResponse) -> RequestTokenLog: + """Record the use of a token.""" def rmg(key: str, default: Any = None) -> Any: return request.META.get(key, default) @@ -332,16 +312,12 @@ def rmg(key: str, default: Any = None) -> Any: token=self, user=None if request.user.is_anonymous else request.user, user_agent=rmg("HTTP_USER_AGENT", "unknown"), - client_ip=parse_xff(rmg("HTTP_X_FORWARDED_FOR")) - or rmg("REMOTE_ADDR", None), + client_ip=( + parse_xff(rmg("HTTP_X_FORWARDED_FOR")) or rmg("REMOTE_ADDR", None) + ), status_code=response.status_code, ).save() - if error and LOG_TOKEN_ERRORS: - RequestTokenErrorLog.objects.create_error_log(log, error) - # NB this will include all error logs - which means that an error log - # may prohibit further use of the token. Is there a scenario in which - # this would be the wrong outcome? - self.used_to_date = self.logs.filter(error__isnull=True).count() + self.used_to_date = self.logs.count() self.save() return log @@ -427,53 +403,3 @@ def save(self, *args: Any, **kwargs: Any) -> RequestToken: self.timestamp = self.timestamp or tz_now() super(RequestTokenLog, self).save(*args, **kwargs) return self - - -class RequestTokenErrorLogQuerySet(models.query.QuerySet): - def create_error_log( - self, log: RequestTokenLog, error: Exception - ) -> RequestTokenErrorLog: - return RequestTokenErrorLog( - token=log.token, - log=log, - error_type=type(error).__name__, - error_message=str(error), - ) - - -class RequestTokenErrorLog(models.Model): - """Used to log errors that occur with the use of a RequestToken.""" - - token = models.ForeignKey( - RequestToken, - related_name="errors", - on_delete=models.CASCADE, - help_text="The RequestToken that was used.", - db_index=True, - ) - log = models.OneToOneField( - RequestTokenLog, - related_name="error", - on_delete=models.CASCADE, - help_text="The token use against which the error occurred.", - db_index=True, - ) - error_type = models.CharField( - max_length=50, help_text="The underlying type of error raised." - ) - error_message = models.CharField( - max_length=200, help_text="The error message supplied." - ) - - objects = RequestTokenErrorLogQuerySet().as_manager() - - class Meta: - verbose_name = "Error" - verbose_name_plural = "Errors" - - def __str__(self) -> str: - return self.error_message - - def save(self, *args: Any, **kwargs: Any) -> RequestTokenErrorLog: - super(RequestTokenErrorLog, self).save(*args, **kwargs) - return self diff --git a/request_token/settings.py b/request_token/settings.py index 77c3285..f56b193 100644 --- a/request_token/settings.py +++ b/request_token/settings.py @@ -1,31 +1,9 @@ -from os import getenv -from typing import Any, Optional - from django.conf import settings - -def _env_or_setting(key: str, default: Any) -> Any: - return getenv(key) or getattr(settings, key, default) - - # the name of GET argument to contain the token -JWT_QUERYSTRING_ARG = _env_or_setting("REQUEST_TOKEN_QUERYSTRING", "rt") # type: str +JWT_QUERYSTRING_ARG: str = getattr(settings, "REQUEST_TOKEN_QUERYSTRING", "rt") # the fixed expiration check on Session tokens -JWT_SESSION_TOKEN_EXPIRY = int(_env_or_setting("REQUEST_TOKEN_EXPIRY", 10)) # type: int - -# Set the default 403 template value -FOUR03_TEMPLATE = _env_or_setting( - "REQUEST_TOKEN_403_TEMPLATE", None -) # type: Optional[str] - -# log all InvalidTokenErrors -LOG_TOKEN_ERRORS = _env_or_setting( - "REQUEST_TOKEN_LOG_TOKEN_ERRORS", "True" -).lower() in ( - "true", - "1", -) # noqa - +JWT_SESSION_TOKEN_EXPIRY: int = getattr(settings, "REQUEST_TOKEN_EXPIRY", 10) -DEFAULT_MAX_USES = _env_or_setting("REQUEST_TOKEN_DEFAULT_MAX_USES", 10) +DEFAULT_MAX_USES: int = getattr(settings, "REQUEST_TOKEN_DEFAULT_MAX_USES", 10) diff --git a/tests/settings.py b/tests/settings.py index e1df09f..b0d112b 100644 --- a/tests/settings.py +++ b/tests/settings.py @@ -4,7 +4,8 @@ import django DJANGO_VERSION = StrictVersion(django.get_version()) -assert DJANGO_VERSION >= StrictVersion("2.2") +if not DJANGO_VERSION >= StrictVersion("2.2"): + raise RuntimeError("Invalid Django version") DEBUG = True @@ -12,7 +13,10 @@ from django.db.models import JSONField # noqa: F401 DATABASES = { - "default": {"ENGINE": "django.db.backends.sqlite3", "NAME": "test.db",} + "default": { + "ENGINE": "django.db.backends.sqlite3", + "NAME": "test.db", + } } except ImportError: DATABASES = { @@ -99,4 +103,5 @@ "loggers": {"request_token": {"handlers": ["console"], "level": "DEBUG"}}, } -assert DEBUG is True, "This project is only intended to be used for testing." +if not DEBUG: + raise Exception("This project is only intended to be used for testing.") diff --git a/tests/templates/403.html b/tests/templates/403.html deleted file mode 100644 index bd09580..0000000 --- a/tests/templates/403.html +++ /dev/null @@ -1,3 +0,0 @@ -Custom 403 template. {% if token_error %} -
{{token_error}}
-{% endif %} diff --git a/tests/test_apps.py b/tests/test_apps.py deleted file mode 100644 index cc025c1..0000000 --- a/tests/test_apps.py +++ /dev/null @@ -1,16 +0,0 @@ -from unittest import mock - -from django.template import TemplateDoesNotExist -from django.test import TestCase - -from request_token.apps import ImproperlyConfigured, check_template - - -class AppTests(TestCase): - - """Tests for request_token.apps functions.""" - - @mock.patch("django.template.loader.get_template") - def test_check_403(self, mock_loader): - mock_loader.side_effect = TemplateDoesNotExist("Template not found.") - self.assertRaises(ImproperlyConfigured, check_template, "foo.html") diff --git a/tests/test_middleware.py b/tests/test_middleware.py index 58e7f66..53e611d 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -3,7 +3,7 @@ from django.contrib.auth import get_user_model from django.contrib.auth.models import AnonymousUser -from django.core.exceptions import ImproperlyConfigured +from django.core.exceptions import ImproperlyConfigured, PermissionDenied from django.http import HttpResponse from django.test import RequestFactory, TestCase from jwt import exceptions @@ -116,18 +116,18 @@ def test_process_exception(self, mock_log): request = self.get_request() request.token = self.token exception = exceptions.InvalidTokenError("bar") - response = self.middleware.process_exception(request, exception) - mock_log.assert_called_once_with(request, response, error=exception) - self.assertEqual(response.status_code, 403) - self.assertEqual(response.reason_phrase, str(exception)) + with self.assertRaises(PermissionDenied): + response = self.middleware.process_exception(request, exception) + self.assertEqual(response.status_code, 403) + self.assertEqual(response.reason_phrase, str(exception)) # no request token = no error log del request.token mock_log.reset_mock() - response = self.middleware.process_exception(request, exception) - self.assertEqual(mock_log.call_count, 0) - self.assertEqual(response.status_code, 403) - self.assertEqual(response.reason_phrase, str(exception)) + with self.assertRaises(PermissionDenied): + response = self.middleware.process_exception(request, exception) + self.assertEqual(response.status_code, 403) + self.assertEqual(response.reason_phrase, str(exception)) # round it out with a non-token error response = self.middleware.process_exception(request, Exception("foo")) diff --git a/tests/test_models.py b/tests/test_models.py index 8e964cb..aef8615 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,7 +1,7 @@ import datetime from unittest import mock -from django.contrib.auth import get_user_model, logout +from django.contrib.auth import get_user_model from django.contrib.auth.models import AnonymousUser from django.contrib.sessions.middleware import SessionMiddleware from django.core.exceptions import ValidationError @@ -12,13 +12,7 @@ from jwt.exceptions import InvalidAudienceError from request_token.exceptions import MaxUseError -from request_token.models import ( - RequestToken, - RequestTokenErrorLog, - RequestTokenErrorLogQuerySet, - RequestTokenLog, - parse_xff, -) +from request_token.models import RequestToken, RequestTokenLog, parse_xff from request_token.settings import DEFAULT_MAX_USES, JWT_SESSION_TOKEN_EXPIRY from request_token.utils import decode, to_seconds @@ -204,15 +198,6 @@ def assertUsedToDate(expected): token.refresh_from_db(fields=["used_to_date"]) assertUsedToDate(4) - with mock.patch.object( - RequestTokenErrorLogQuerySet, "create_error_log" - ) as mock_log: - log = token.log(request, response, MaxUseError("foo")) - self.assertEqual(mock_log.call_count, 1) - self.assertEqual(log.user_agent, "test_agent") - token.refresh_from_db(fields=["used_to_date"]) - assertUsedToDate(5) - def test_jwt(self): token = RequestToken(id=1, scope="foo").save() jwt = token.jwt() @@ -356,23 +341,6 @@ def test_create_token(self): self.assertEqual(RequestToken.objects.get().scope, "foo") -class RequestTokenErrorLogQuerySetTests(TestCase): - def test_create_error_log(self): - user = get_user_model().objects.create_user( - "zoidberg", first_name=u"∂ƒ©˙∆", last_name=u"†¥¨^" - ) - token = RequestToken.objects.create_token( - scope="foo", user=user, login_mode=RequestToken.LOGIN_MODE_REQUEST - ) - log = RequestTokenLog(token=token, user=user).save() - elog = RequestTokenErrorLog.objects.create_error_log(log, MaxUseError("foo")) - self.assertEqual(elog.token, token) - self.assertEqual(elog.log, log) - self.assertEqual(elog.error_type, "MaxUseError") - self.assertEqual(elog.error_message, "foo") - self.assertEqual(str(elog), "foo") - - class RequestTokenLogTests(TestCase): """RequestTokenLog model property and method tests.""" diff --git a/tests/urls.py b/tests/urls.py index 2080278..8405cbb 100644 --- a/tests/urls.py +++ b/tests/urls.py @@ -2,12 +2,14 @@ from django.conf.urls.static import static from django.contrib import admin from django.urls import path +from django.views import debug from .views import decorated, roundtrip, undecorated app_name = "tests" urlpatterns = [ + path("", debug.default_urlconf), path("admin/", admin.site.urls), path("decorated/", decorated, name="decorated"), path("roundtrip/", roundtrip, name="roundtrip"), diff --git a/tox.ini b/tox.ini index 55d95ae..73175da 100644 --- a/tox.ini +++ b/tox.ini @@ -1,6 +1,6 @@ [tox] isolated_build = True -envlist = fmt, lint, mypy, py{3.7,3.8,3.9}-django{22,30,31,master} +envlist = fmt, lint, mypy, py{3.7,3.8,3.9}-django{22,30,31,main} [testenv] deps = @@ -12,7 +12,7 @@ deps = django22: Django>=2.2,<2.3 django30: Django>=3.0,<3.1 django31: Django>=3.1,<3.2 - djangomaster: https://github.com/django/django/archive/master.tar.gz + djangomain: https://github.com/django/django/archive/main.tar.gz commands = pytest --cov=request_token tests/