vitalik / django-ninja

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

Multiple authentication/authorizations #360

Closed gabrielfgularte closed 2 years ago

gabrielfgularte commented 2 years ago

Hi there, wondering how does one auhenticate and authorize a route. I'm doing this way:

@router.get('auth/', auth=[AuthBearer(), account_required], response=UserSchema)
def user_detail(request):
    return request.user

Where AuthBearer is a class that extends HttpBearer and has an authenticate method that checks for the token received (as explained in the docs: https://django-ninja.rest-framework.com/tutorial/authentication/#http-bearer). And account_required is a method that checks if user has an account (hasattr(request.user, 'account') and request.user.account is not None) (docs: https://django-ninja.rest-framework.com/tutorial/authentication/#custom-function)

When I hit this endpoint, code is touched on AuthBearer.authenticate but does not on account_required. So I assume that, for multiple auth, I must have done something wrong. Since for single auth (auth=account_required), the account_required method is touched.

What am I doing wrong? Thank you, and congrats for this amazing library!

gabrielfgularte commented 2 years ago

Just noticed that this could be due to an OR condition. In this doc https://django-ninja.rest-framework.com/tutorial/authentication/#multiple-authenticators, we have:

In this case Django Ninja will first check the API key GET, and if not set or invalid will check the header key. If both are invalid, it will raise an authentication error to the response.

Wouldn't it be a good idea to verify that all checks in this list are true (using AND instead of OR)?

gabrielfgularte commented 2 years ago

I have found a workaround to this. I created my own authorization decorators and I'm decorating the routes like this:

def account_required(f):
    @wraps(f)
    def wrapper(*args, **kwds):
        request = args[0]
        if hasattr(request.user, 'accounts'):
            return f(*args, **kwds)
        raise HttpError(401, 'Unauthorized')

@router.get('auth/', auth=AuthBearer(), response=UserSchema)
@account_required
def user_detail(request):
    return request.user
SmileyChris commented 2 years ago

The or is fine since the purpose of a multiple authenticators is to allow authentication using any of the multiple methods, not to block if they don't all match.

In your case, you'd use a custom single authenticator which does both checks.

The docs should be fleshed out a bit to explain this purpose.

gabrielfgularte commented 2 years ago

@SmileyChris I see your point and kind of agree with that.

If that auth param is meant to handle only authentication, then I think you're absolutelly right. But if is meant to handle both authorizarion and authentication, I think the and approach is more accurate, once a user should be properly authenticated AND authorized to access the given route.

IMHO authentication and authorization should not be handled in the same way. So, as I don't know if django-ninja wants to have builtin authorizations, maybe the best solution is do authorization decorators as I did and decorate the routes instead of trying to authorize user in the auth param. Let this param to handle authentication only.

And of course, if authorizations could be covered in the documentation it would be amazing. Thanks.

vitalik commented 2 years ago

@gabrielfgularte

well...

in your specific case you just extend HttpBearer (authentication) with your access logic:


class AccountAuth(AuthBearer):
    def authenticate(self, request, token):
           if magic(token) != 'secret':
                return None
           account = Accounts.objects.get(token=token)
           if account.does_not_have_permission():
                  raise PermissionDenied() # or return None
           return account

this works if your entire app or router need a same way of authorization

but if each individual view need different permissions check - then I would use decorators or some mixin classes for Auth

as a third option you can implement some sort of chain function that will execute all authenticators passed to it:

@router.get('auth/', auth=chain(AuthBearer(), account_required))
gabrielfgularte commented 2 years ago

Yeah @vitalik , as I said before, I think decorators can handle this and they are doing a great job so far for me.

So, I'm closing this issue. Thanks.

sauron136 commented 1 year ago

Hi vitaliy, I am stuck on how to create authentication and authorization for this. I have a user model, how do I implement register, login, logout ...functionality please?

baseplate-admin commented 1 year ago

Hi @KrystianMaccs,

Please use the discussion tab for this. Don't grave dig old issues

sauron136 commented 1 year ago

Hi @KrystianMaccs,

Please use the discussion tab for this. Dont grave dig old issues

Alright. New to github discussions and haven't figured out how to use it. sorry.