eadwinCode/django-ninja-extra

Permissions are verified before authentication.

Closed this issue · 15 comments

How to reproduce.

from typing import Any

from django.http import HttpRequest
from ninja_extra import permissions, NinjaExtraAPI, api_controller, http_get
from ninja_jwt.authentication import AsyncJWTAuth
from ninja_jwt.controller import NinjaJWTDefaultController

api = NinjaExtraAPI()
api.register_controllers(NinjaJWTDefaultController)


class MyAuth(AsyncJWTAuth):
    async def authenticate(self, request: HttpRequest, token: str) -> Any:
        # This method will be called after MyPerm.has_permission calling.
        # Expected behavior: Authenticate should be called firstly.
        return await self.async_jwt_authenticate(request, token)


class MyPerm(permissions.BasePermission):
    def has_permission(self, request, controller):
        return request.user.groups.filter(name="MyGroup").exists()


@api_controller('', tags=['My Operations'], auth=MyAuth(), permissions=[MyPerm])
class MyAPIController:

    @http_get("/path")
    async def get_operation(self):
        return {"ok": "ok"}

api.register_controllers(MyAPIController)

Issue:

MyPerm.has_permissions is called before AsyncJWTAuth.authenticate
so when MyPerm.has_permissions is called, the request.user will be always AnonymousUser because AsyncJWTAuth.authenticate has not been called yet.

Expected behavior

AsyncJWTAuth.authenticate is called first to initialize request.user, and then BasePermission.has_permission

@airddm Thanks🙏 for bringing this to my notice. It was an oversight

I have also published a new release, please let me know if the problem is resolved at your end.

Hi @eadwinCode 👋

In my case, after upgrading to the last version I have:

{
    "detail": "user_id: Input should be a valid UUID, invalid character: expected an optional prefix of `urn:uuid:` followed by [0-9a-fA-F-], found `m` at 1"
}
@api_controller("/users", tags=["User"])
class UserAPI:
    @http_get("/me")
    def get_current_user(self, request):
        return {"debug": "ok"}
from ninja.security import django_auth

api = NinjaExtraAPI(auth=django_auth)
MIDDLEWARE = [
    "django.middleware.security.SecurityMiddleware",
    "corsheaders.middleware.CorsMiddleware",
    "django.contrib.sessions.middleware.SessionMiddleware",
    "django.middleware.locale.LocaleMiddleware",
    "django.middleware.common.CommonMiddleware",
    "django.middleware.csrf.CsrfViewMiddleware",
    "main.middlewares.auth_cached.CachedAuthenticationMiddleware",
    "main.middlewares.tenant.XTenantMiddleware",
    "main.middlewares.cache_control.CacheControlMiddleware",
    "django.contrib.messages.middleware.MessageMiddleware",
    "django.middleware.clickjacking.XFrameOptionsMiddleware",
    "django_structlog.middlewares.RequestMiddleware",
]

@Tragio can you give an example source code that resulted to the error? Your current example is not showing anything that relates to user_id.

@eadwinCode interesting, so the problem it is not picking GET /me only if I remove the other dynamic ones 🤔

import uuid

from ninja_extra import (
    api_controller,
    http_delete,
    http_get,
    status,
)


@api_controller("/users", tags=["User"])
class UserAPI:
    @http_get("/me")
    def get_current_user(self, request):
        return {"debug": "ok"}

    @http_get("/{user_id}")
    def get_user(self, request, user_id: uuid.UUID):
        return {"debug": "ok"}

    @http_delete("/{user_id}", response={status.HTTP_204_NO_CONTENT: None})
    def delete_user_from_clinic(self, request, user_id: uuid.UUID, token: str):
        return {"debug": "ok"}

@Tragio you have two endpoints with there same path configuration, get_current_user and get_user has the same path configuration. so when you call get_current_user, django will call get_user cause in set up hierarchy get_user comes last.

My solution for your

@api_controller("/users", tags=["User"])
class UserAPI:
    @http_get("/")
    def get_current_user(self, request):
        return {"debug": "ok"}

    @http_get("/{user_id}")
    def get_user(self, request, user_id: uuid.UUID):
        return {"debug": "ok"}
    
    @http_delete("/{user_id}", response={status.HTTP_204_NO_CONTENT: None})
    def delete_user_from_clinic(self, request, user_id: uuid.UUID, token: str):
        return {"debug": "ok"}

Also, was this working before the upgrade?

I have also published a new release, please let me know if the problem is resolved at your end.

It works ! Thank you very much 👍

@eadwinCode it was working before, that is why I was confused with the behaviour 😅 I thought it respected the order that I had 🤔

@Tragio that's interesting I will look into it again, I can't remember making changes to the route models.

But what version were you on before the upgrade?

@eadwinCode I was on the version 0.21.4, when I upgraded to 0.21.6 it started with that problem 🤔

Edit: I can also change the route. My question is what is the intended behaviour, as I initially thought it was in the order I had written.

@Tragio I will create a fix for this shortly, I have seen what caused it

@Tragio You can pull the latest release in 15 minutes, that should solve it.

@eadwinCode already tested, it is working. You're amazing, thank you very much ❤️

Awesome... Thanks for reporting this