From 65dad53ffc6ce2023fda24f480cbaa265c786fe3 Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Tue, 1 Jul 2025 19:57:49 -0400 Subject: [PATCH] Allow Ninja to enforce CSRF on a per-auth method basis Globally enabling CSRF is no longer necessary, as newer versions of Ninja will now directly enforce it for any cookie auth. So, we can also remove the global disable `ExemptBearerAuthFromCSRFMiddleware`, since auth via bearer won't check CSRF (which is correct). --- isic/auth.py | 31 +++++++++++++++++++++++++++---- isic/middleware.py | 26 -------------------------- isic/settings/base.py | 2 -- isic/urls.py | 1 - isic/zip_download/api.py | 2 +- pyproject.toml | 1 - 6 files changed, 28 insertions(+), 35 deletions(-) diff --git a/isic/auth.py b/isic/auth.py index b3588086c..d91b861b7 100644 --- a/isic/auth.py +++ b/isic/auth.py @@ -1,6 +1,10 @@ from collections.abc import Callable -from ninja.security import HttpBearer, django_auth +from django.http import HttpRequest +from ninja.errors import HttpError +from ninja.operation import PathView +from ninja.security import HttpBearer, SessionAuth +from ninja.utils import check_csrf from oauth2_provider.oauth2_backends import get_oauthlib_core from isic.core.permissions import SessionAuthStaffUser @@ -8,6 +12,24 @@ ACCESS_PERMS = ["any", "is_authenticated", "is_staff"] +class CsrfFixedSessionAuth(SessionAuth): + def _get_key(self, request: HttpRequest) -> str | None: + if self.csrf: + # Work around https://github.com/vitalik/django-ninja/issues/1068 + # Maybe related https://github.com/vitalik/django-ninja/issues/1101 + path_view: PathView = request.resolver_match.func.__self__ + view_func = path_view._find_operation(request).view_func + # The upstream implementation doesn't send "view_func" to "check_csrf", so a + # "csrf_exempt" annotation can't be detected. + error_response = check_csrf(request, view_func) + if error_response: + raise HttpError(403, "CSRF check Failed") + return request.COOKIES.get(self.param_name) + + +django_auth = CsrfFixedSessionAuth() + + class OAuth2AuthBearer(HttpBearer): def __init__(self, perm: str): if perm not in ACCESS_PERMS: @@ -38,7 +60,8 @@ def authenticate(self, request, token): request.oauth2_error = getattr(r, "oauth2_error", {}) +# Always test OAuth2 before session auth, since OAuth2 doesn't have CSRF messiness. # The lambda _: True is to handle the case where a user doesn't pass any authentication. -allow_any: list[Callable] = [django_auth, OAuth2AuthBearer("any"), lambda _: True] -is_authenticated = [django_auth, OAuth2AuthBearer("is_authenticated")] -is_staff = [SessionAuthStaffUser(), OAuth2AuthBearer("is_staff")] +allow_any: list[Callable] = [OAuth2AuthBearer("any"), lambda _: True, django_auth] +is_authenticated = [OAuth2AuthBearer("is_authenticated"), django_auth] +is_staff = [OAuth2AuthBearer("is_staff"), SessionAuthStaffUser()] diff --git a/isic/middleware.py b/isic/middleware.py index 3588c21c0..b63e0291b 100644 --- a/isic/middleware.py +++ b/isic/middleware.py @@ -1,36 +1,10 @@ -from collections.abc import Callable import logging -from typing import Any -from django.http import HttpRequest, HttpResponseBase -from ninja.operation import PathView from sentry_sdk import set_tag logger = logging.getLogger(__name__) -# See https://github.com/vitalik/django-ninja/issues/283. -# This exists to allow the header based authentication to be exempt from CSRF checks, -# while still enforcing CSRF checks on the session based authentication. -class ExemptBearerAuthFromCSRFMiddleware: - def __init__(self, get_response): - self.get_response = get_response - - def __call__(self, request): - return self.get_response(request) - - def process_view( - self, - request: HttpRequest, - view_func: Callable[..., HttpResponseBase], - view_args: tuple[Any, ...], - view_kwargs: dict[str, Any], - ) -> None: - klass = getattr(view_func, "__self__", None) - if klass and isinstance(klass, PathView): - request._dont_enforce_csrf_checks = True # noqa: SLF001 - - class UserTypeTagMiddleware: def __init__(self, get_response): self.get_response = get_response diff --git a/isic/settings/base.py b/isic/settings/base.py index ecb347de1..29a5c4df7 100644 --- a/isic/settings/base.py +++ b/isic/settings/base.py @@ -82,8 +82,6 @@ "isic.middleware.UserTypeTagMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", "django.middleware.common.CommonMiddleware", - # Insert "ExemptBearerAuthFromCSRFMiddleware" just before the CsrfViewMiddleware - "isic.middleware.ExemptBearerAuthFromCSRFMiddleware", "django.middleware.csrf.CsrfViewMiddleware", "django.contrib.auth.middleware.AuthenticationMiddleware", "django.contrib.messages.middleware.MessageMiddleware", diff --git a/isic/urls.py b/isic/urls.py index fcd0f189d..bcbaac7e3 100644 --- a/isic/urls.py +++ b/isic/urls.py @@ -36,7 +36,6 @@ version="v2", docs_url=None, # we want to serve the docs next to the ninja root rather than under it auth=allow_any, - csrf=True, urls_namespace="api", ) swagger_view = partial(openapi_view, api=api) diff --git a/isic/zip_download/api.py b/isic/zip_download/api.py index 1e8cade01..d4b0128b4 100644 --- a/isic/zip_download/api.py +++ b/isic/zip_download/api.py @@ -93,8 +93,8 @@ def zip_api_auth(request: HttpRequest): return ZipDownloadTokenAuth()(request) -@csrf_exempt @zip_router.post("/url/", response=str, include_in_schema=False) +@csrf_exempt def create_zip_download_url(request: HttpRequest, payload: SearchQueryIn): url: ParseResult | None = settings.ISIC_ZIP_DOWNLOAD_SERVICE_URL if url is None: diff --git a/pyproject.toml b/pyproject.toml index fe5df7edd..200819544 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -265,7 +265,6 @@ filterwarnings = [ "ignore:unclosed