vitalik / django-ninja

💨 Fast, Async-ready, Openapi, type hints based framework for building APIs
https://django-ninja.dev
MIT License
6.96k stars 420 forks source link

Multiple authentication backends with conditional csrf #283

Open asaff1 opened 2 years ago

asaff1 commented 2 years ago

Obviously cookie based auth needs the csrf, but how can I exclude the csrf for authorization header based auth? This is my configuration:

router = NinjaAPI(auth=(AuthBearer(), django_auth), csrf=True)

So the point is, the csrf check needs to be based on the authentication backend that is in use. Now it is always enforced. (django-rest-framework does that correctly)

vitalik commented 2 years ago

you can set global csrf=False

and then extend django_auth and add csrf check inside your own authenticator

asaff1 commented 2 years ago

Good idea, I think it should be the design by default. how do I call django's csrf check logic?

vitalik commented 2 years ago

@asaff1 you can check here - https://github.com/vitalik/django-ninja/blob/master/ninja/utils.py#L17

Photonios commented 2 years ago

you can set global csrf=False

and then extend django_auth and add csrf check inside your own authenticator

You can't really do the CSRF check in the authenticator since you do not have access to the view function. I ended up solving this with a custom middleware that exempts views from CSRF if non-session authentication is used.

I'd be highly in favour of being able to control which authentication methods require CSRF and which don't natively in Django Ninja.

from ninja.operation import PathView

class ExemptAPIKeyAuthFromCSRFMiddleware:
    """Exempts API views from CSRF if not authenticated with
    Django session authentication.

    This allows API calls with an API  key to pass without
    having to pass a CSRF token."""

    def __init__(self, get_response):
        self.get_response = get_response

    def __call__(self, request):
        return self.get_response(request)

    def process_view(self, request, view_func, view_args, view_kwargs):
        if request.user.is_authenticated: # this check might not be enough if your other auth methods set `request.user`.
            return

        klass = getattr(view_func, '__self__', None)
        if not klass:
            return

        if not isinstance(klass, PathView):
            return

        for operation in klass.operations:
            setattr(operation.view_func, 'csrf_exempt', True)
Skelmis commented 2 years ago

I can plus 1 Photonios here, although I have taken a slightly different approach in fixing it myself in the mean time. Using a class which is passed as auth and contains a variable which is used for bool calls, this can then be set in each auth provider accordingly to do CSRF dependent on the authentication provider.

class Mockcls:
    def __init__(self):
        self.do_csrf = True
    def __bool__(self):
        return self.do_csrf

instance = Mockcls()

NinjaAPI(auth=instance)

Which allows us to modify the do_csrf attribute in auth classes to toggle CSRF. It does feel very hacky and I would love a way to control this natively, possibly going as far as having the csrf parameter be on authentication classes rather then the NinjaAPI instance

shughes-uk commented 2 years ago

Thank you @Photonios for this but i've spotted an issue

class ExemptAPIKeyAuthFromCSRFMiddleware:

The view_func instances seem to be reused between multiple requests, so by setting csrf_exempt you're disabling CSRF checks for all future requests.

You could toggle it back to False on a per request basis, but I think that might run into race conditions when you have django serving requests in a concurrent fashion (async/gevent).

The CSRF middleware checks request._dont_enforce_csrf_check and will exit early if set https://github.com/django/django/blob/fac3dd7f390d372736e05974cc5c3ef1a3768fbb/django/middleware/csrf.py#L433-L438

We can use this to 'trick' it into thinking CSRF has already been checked just for specific requests. The modified version looks like this

class ExemptAPIKeyAuthFromCSRFMiddleware:
    def __init__(self, get_response):
        self.get_response = get_response

    def __call__(self, request):
        return self.get_response(request)

    def process_view(self, request, view_func, view_args, view_kwargs):
        if request.user.is_authenticated:
            return
        klass = getattr(view_func, "__self__", None)
        if not klass:
            return
        if isinstance(klass, PathView):
            request._dont_enforce_csrf_checks = True

Definitely need a mechanism to avoid having to do these hacks, API's that deal with browser based auth and some other auth where CSRF is not required seem to be fairly common.

skokado commented 1 year ago

Is it better approach to set csrf_exempt=True if the view klass includes ninja.security.django_auth in auth callbacks or not ?

Like this:

from ninja.security import django_auth

class ExemptCSRFOtherThanDjangoAuthMiddleware:
    def __init__(self, get_response):
        self.get_response = get_response

    def __call__(self, request):
        return self.get_response(request)

    def process_view(self, request, view_func, view_args, view_kwargs):
        klass = getattr(view_func, "__self__", None)
        if not klass:
            return

        for operation in klass.operations:
            if django_auth not in operation.auth_callbacks:
                setattr(operation.view_func, 'csrf_exempt', True)