User foreign key is required in Application model about django-oauth-toolkit HOT 33 CLOSED

I had to work around this a while ago. I ended up with a class that overrode the one from this project but grabbed the user from the token:

from oauth2_provider.ext.rest_framework import (
    OAuth2Authentication as BaseOAuth2Authentication,
)

class OAuth2Authentication(BaseOAuth2Authentication):
    """
    Authenticator that forces request.user to be present even if the
    oauth2_provider package doesn't want it to be.

    Works around the change introduced in:
    https://github.com/evonove/django-oauth-toolkit/commit/628f9e6ba98007d2940bb1a4c28136c03d81c245

    Reference:
    https://github.com/evonove/django-oauth-toolkit/issues/38

    """
    def authenticate(self, request):
        super_result = super(OAuth2Authentication, self).authenticate(request)

        if super_result:
            # The request was found to be authentic.
            user, token = super_result
            if user is None:
                user = token.application.user
            result = user, token
        else:
            result = super_result
        return result

Originally I thought that perhaps DRF should change how they detect 'authenticated' requests. Perhaps checking if request.user is set is not correct and they should do something else.

However, after reading the Django documentation you see:

Either way, authenticate should check the credentials it gets, and it should return a User object that matches those credentials, if the credentials are valid. If they’re not valid, it should return None.

Django then places that returned object on the request, so I think it's entirely reasonable for DRF to use that as a test and I think this PR is not following the interface as laid out in the Django docs.

All that being said, I don't think my hack above is a great long-term fix. I would suggest creating a sentinal object (as I think @masci is suggesting) and return that. It could be a ClientCredentialUser very similar to the Django AnonymousUser object. This object wouldn't represent a User in the normal sense (so it shouldn't have a username or other user-like things) but would instead be an assertion to the Auth backends that the request was authenticated and the request came from a source that is in fact whom they claim to be.

from django-oauth-toolkit.

Came across this issue which was causing errors in my usage of the project. Perhaps I can serve as a useful usecase, so I'll describe my setup.

I am writing an intermediate server with (at the moment) 2 server-based consumers. They will speak to my django-rest-framework endpoints authenticating via oauth. They each have an application generated, linked to a 'User' object which identifies their server. In this case, the users are 'edx' and 'teachersportal'.

Each of these servers get their own set of application ids and secrets. They use them to log in. Unfortunately, with the change as it exists (specifically this line: 628f9e6#diff-2c5f9ca04fff2537c181592cc1e6c7c3R296 ) I have no way of knowing which of those servers is authenticated, which means all incoming requests return access denied errors.

If I manually set a user_id on the AccessToken instances, I can successfully authenticate with them.

It seems like the goal is to have Application.user_id be optional. Is it reasonable to say "If you have a client_credentials endpoint, and the Application has a user set, set that user on the AccessTokens. If the Application does NOT have a user, don't set the user on AccessTokens." ?

from django-oauth-toolkit.

Your example presumes that a developer who owns the application is giving his API id/secret to a user that he wouldn't want to make protected calls. This seems to go against the very nature of id/secret pairs, I think.

And you can still access the very same user instance through token.application.user. Isn't this ok?
This is a matter of where you do this. token.application.user isn't available in my views where I need to check this.

via: https://tools.ietf.org/html/rfc6749#section-4.4

The client can request an access token using only its client
credentials (or other supported means of authentication) when the
client is requesting access to the protected resources under its
control, or those of another resource owner that have been previously
arranged with the authorization server (the method of which is beyond
the scope of this specification).

It seems like what I'm describing is "requesting access to the protected resources under it's control".

I'll just use the approach I'm using already. I think this API isn't quite right, but there's an easy enough alternative.

from django-oauth-toolkit.

We discussed about the topic and we agree that in some cases (above all, Client Credential workflow) having the access token tied to an user is not correct. We have some ideas to decouple tokens and users and a solution is on the way, thus I cannot give you an ETA.
Let's use this issue to track this feature's progress.

from django-oauth-toolkit.

Thank you for the feedback. I think with the current code it's possible to associate an inactive user to the token because the application user is not validated.

from django-oauth-toolkit.

This is an old draft, should be ok as a starting point for the discussion.
The basic goal is to remove user reference in the application so we can support client credential flow.

         +----------------+                             
         |                |                             
         |     Owner      +---------------------+       
         |                |                     |       
         +-+------------+-+         References  |       
           |            |                       |       
           |  Inherits  |                       |       
           |            |                       |       
+----------+----+   +---+-----------+   +-------+------+
|               |   |               |   |              |
|  User owner   |   |Client Creden. |   |  Token       |
|               |   |   Owner       |   |              |
+-+-------------+   +-+-------------+   +--------------+
  |                   |                                 
  |  References       |                                 
  |                   |                                 
+-+-------------+   +-+-------------+                   
|               |   |               |                   
|  User         |   |Application    |                   
|               |   |               |                   
+---------------+   +---------------+      

from django-oauth-toolkit.

In the draft above, you have introduced the "owner" concept. Wouldn't it be sufficient to simply make User an optional property on Application, and on Token? I may be missing something here, but could it be that simple?

from django-oauth-toolkit.

I extracted the draft above from a more general design that has never been implemented, so do not take it in much consideration.

@ryanisnan actually it could. Application.user is not involved in the OAuth2 process and its usage could be left in users' hands according to their use cases. AccessToken.user has to be made optional to adhere the rfc and support client credential flow.

The change is trivial and I don't see any apparent drawback, I whish I noticed this earlier.

from django-oauth-toolkit.

Sounds great @masci. Would you like me to put together a PR for this one?

from django-oauth-toolkit.

@ryanisnan It'd be great!

from django-oauth-toolkit.

The above commit fixes the client credential flow by creating access tokens not bound to a user instance.

I think that the Application.user field should never be null because it represents the owner of the client application which is going to consume the API and this is an information you do not want to omit.
Maybe there are usecases where the field is useless but I can't think of any right now

from django-oauth-toolkit.

I've updated the library on my installation.
I may miss something, but this change break Django Rest Framework permissions.
628f9e6#diff-2c5f9ca04fff2537c181592cc1e6c7c3R296

Now with request.user = None, the permissions.IsAuthenticatedOrReadOnly always return False

https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/permissions.py#L65-L69

Is there any workaround?

from django-oauth-toolkit.

This is a problem because we cannot distinguish unauthenticated requests from "server2server" authenticated requests.
@synasius do you think we could assign request.user to something related to the OAuth2 client (application id, token, whatever) instead of None?

from django-oauth-toolkit.

@poswald creating another type of user sounds like a bad idea to me. Problem is that 3rd party code is not ready to receive anything beside User and AnonymousUser in request.user...

I'm really surprised that Application is tied to a user. There are a lot of cases when Application does not necessary have any user it belongs to (think of applications of your own control).

from django-oauth-toolkit.

if anyone is interested not having mandaratory User in your apps use this gist:

# taken from https://gist.github.com/specialunderwear/9d917ddacf3547b646ba
def AbstractClassWithoutFieldsNamed(cls, *excl):
    """
    Removes unwanted fields from abstract base classes.

    Usage::
    >>> from oscar.apps.address.abstract_models import AbstractBillingAddress

    >>> from koe.meta import AbstractClassWithoutFieldsNamed as without
    >>> class BillingAddress(without(AbstractBillingAddress, 'phone_number')):
    ...     pass
    """
    if cls._meta.abstract:
        remove_fields = [f for f in cls._meta.local_fields if f.name in excl]
        for f in remove_fields:
            cls._meta.local_fields.remove(f)
        return cls
    else:
        raise Exception("Not an abstract model")

and setup your custom Application this way:

from django.db import models
from django.conf import settings

from oauth2_provider.models import AbstractApplication
from core.models.utils import AbstractClassWithoutFieldsNamed as without


class Application(without(AbstractApplication, 'user')):
    user = models.ForeignKey(
        settings.AUTH_USER_MODEL,
        related_name="%(app_label)s_%(class)s",
        blank=True,
        null=True
    )

then migrate as usual.

from django-oauth-toolkit.

@pySilver The problem is that it isn't tied to a user anymore (which is why this is a closed PR) but that fix has now broken expectations of 3rd party libs who assume that a request without a user is unauthenticated. This discussion would perhaps be better as a new PR.

While I agree that introducing a new type of user object is not ideal and may cause breakage in 3rd party apps, I feel like it best follows the documentation of the django backend system and how DRF expect things to work. With Django now supporting custom user objects, there is probably a lot less instanceof checking out there in the wild. If the new object introduced follows roughly the same API as User and AnonymousUser then it should be ok.

The user object is more like a container for an authenticated identity. To work in a way that is compatible with how 3rd party libs that depend on django backends, it has to return something (not None) and I feel like a new user object best represents what is really going on.

from django-oauth-toolkit.

@poswald hm, Application.user is still a required field. Why do you want to introduce something that is not required by specification? It's much easier to leave fields optional and let end user to define validation logic.

from django-oauth-toolkit.

@poswald, I agree that the fix posted by @synasius is bad. It is not solving directly any of the issues opened by this ticket~~, and also introduces a side effect.~~*

• The issue was originally about if the User reference is mandatory on an Application, not on the AccessToken
• The side effect of the actual commit is that the user information is cleared from the request completely, instead of just not storing it with the generated Tokens*

The reason why I had to dive into this problem is that I would like to allow for a use case when a token generated by a confidential client can be used in places protected by standard User bound permissions, i.e. that this token can be used to manage User resources, as if it was a User bound token.

For now, @poswald 's solution is the only generic approach

I believe that as a toolkit django-oauth should be permissive of different use cases. When storing an AccessToken as a result of a client_credentials grant type it should take the User info from the Application class. At the same time, the User on Application should be made optional. This way, any workflow will be possible, and when User association will not be wanted, it is going to be reflected both at the Token and Application levels.

** the side effect was discussed separately, and is not relevant any more

from django-oauth-toolkit.

@leonid-s-usov the other solution I've seen is to write your own validator that basically undoes this commit (link).

Also agreed with @pySilver that for applications you control having Application tied to user doesn't make a lot of sense.

from django-oauth-toolkit.

Hi @justinabrahms
if you have a client_credential application you can still access the application's user instance through the application field of the AccessToken model.

user = token.application.user

The user field in the AccessToken model should only contain the user on whose behalf the app is authenticated. The client_credential flow only authenticates applications, not users, therefore should not be bound to any user instance. And this is the main reason why we set request.user to None here 628f9e6#diff-2c5f9ca04fff2537c181592cc1e6c7c3R296

from django-oauth-toolkit.

What access does an AccessToken provide? What things can they request? If they don't have a backing user.. how do you know how to handle authorization? Are you doing this entirely via scopes?

I have additional state that I store about incoming requesters. I need a mechanism to figure out which application is making the request so I can lookup the proper information there. The clients won't know this information. As such, I've given them all a backing user and have dangled the additional state on that user's profile object.

It seems to me that just because client_credential doesn't NEED to be backed by a user doesn't mean it shouldn't be able to be backed by a user. Accessing the "who is querying me?" via request.user is a strong API with lots of built up community expectation in Django land. For now, I'm using @poswald's patch above (and trying to figure out a reasonable testing mechanism), but I'd really like it if this was a built-in option.

from django-oauth-toolkit.

What access does an AccessToken provide? What things can they request? If they don't have a backing user.. how do you know how to handle authorization? Are you doing this entirely via scopes?

Yes, we usually use scopes to define the resources that can be accessed with an access token.

I have additional state that I store about incoming requesters. I need a mechanism to figure out which application is making the request so I can lookup the proper information there. The clients won't know this information. As such, I've given them all a backing user and have dangled the additional state on that user's profile object.

Help me to understand. You have one backing user for every application?
If this is the case an alternative could be
to use a custom Application model (or OneToOne relation similar to a user profile) and add the infos you need there.

It seems to me that just because client_credential doesn't NEED to be backed by a user doesn't mean it shouldn't be able to be backed by a user.

Ok, I can agree with that. But I think that the AccessModel should work the way I described for consistency with RFC. And you can still access the very same user instance through token.application.user. Isn't this ok?

Accessing the "who is querying me?" via request.user is a strong API with lots of built up community expectation in Django land. For now, I'm using @poswald's patch above (and trying to figure out a reasonable testing mechanism), but I'd really like it if this was a built-in option.

I think that @poswald patch is the way to go, if you need it. It isn't a builtin option because I still think that we're not authenticating a user but an application. It's a sane default to keep request.user to None and this default can help developers to avoid data leaks on a protected resource if endpoints are not configured properly.
For instance, let's say we have a user developer which owns the client_credential application A.
Let's say we have a /api/profile endpoint protected with IsAuthenticated permission, which returns information for the authenticated user account (the one bound to the token).
If an access_token issued by the A application is bound to the developer user, that access token can be used with success to retrieve information about the developer account from the /api/profile/ endpoint.

Hope this helps!

from django-oauth-toolkit.

In case django rest framework is used, the token is available at request.auth.

from django-oauth-toolkit.

However, @synasius, using token.appilcation.user is a weak alternative to having the user set automatically, since additional middleware and plugins will not be aware of the authentication.

from django-oauth-toolkit.

@leonid-s-usov yes, I understand! But, as explained in the the previous comment, I think that DOT should behave as safe as possible by default. @poswald patch is fine, but just can't be the default.

As you noticed, one can also use request.auth to create a custom permission to use with Rest framework. Something like

class IsClientCredential(BasePermission):
    def has_permission(self, request, view):
        return request.auth and request.auth.application.authorization_grant_type == 'client-credentials'

BTW, If the interest around @poswald fix increase, we can also merge it into the codebase, as an optional backend.
Oh, and we're going to drop not null constraint on user fk for the application model ;)

from django-oauth-toolkit.

For those who plan to use @poswald's stuff, here's a test case (written for py.test) that exercises at least the happy path.

"""
Test for custom django-rest-framework authentication class.
"""
# pylint: disable=no-self-use
from datetime import timedelta
from django.core.urlresolvers import reverse
from django.contrib.auth.models import User
from django.utils import timezone
from django.test import TestCase, RequestFactory
from .oauth2_auth import OAuth2Authentication
from oauth2_provider.models import Application, AccessToken

class OAuth2AuthenticationTests(TestCase):
    """
    Tests for OAuth2Authentication
    """
    def test_returns_proper_user_on_request_object(self):
        """
        Returns proper user on request object.
        """
        user = User.objects.create_user('test')
        req_factory = RequestFactory()
        app = Application.objects.create(name='test app', user=user)
        token = AccessToken.objects.create(
            token='test-token',
            application=app,
            expires=timezone.now() + timedelta(days=1))
        request = req_factory.get(reverse('course-list'), **{
            'HTTP_AUTHORIZATION': 'Bearer {}'.format(token.token),
        })
        subject = OAuth2Authentication()

        result = subject.authenticate(request)

        assert result, "Expected request to be a valid authentication call"
        assert result[0] == user, "Expected user to be the oauth app's user"

from django-oauth-toolkit.

@synasius
OK, imagine there is this possibility to create an Application without User association (NULL). Having that, can you please give me a use case when an Application with the User set should not be automatically associated with that User upon client authentication?

from django-oauth-toolkit.

the same use case I described at the end of this comment #38 (comment)
The issued access_token is going to have permission that shouldn't have. You can access endpoints related to user's resources and not application's resources. So, as I said before, it's just a sane default.

I think that if you need something different you can plug your custom class into DOT. That should be easy. If it isn't, we can work on the library to make it easier.

from django-oauth-toolkit.

Unfortunately, I don't see any problem in your use case :). Why shouldn't the confidential client get access to the resource owner's profile? If I didn't want it, I wouldn't set the mapping between those entities

There is a reason behind this thread having multiple participants. A direct 1-1, or even a 0-1 mapping between Application and a User is something DOT has introduced, it has nothing to do with the standard. The way clients (Applications in DOT) are added to the system, and the way they are associated with protected resources is completely application specific.

The actual mapping between clients and resources will in many cases be much more complicated than the one you can express with a single relationship. So this should not be treated as "the way" of managing Applications.

Having that said, I see the User field in Application as a neat free addition to the otherwise transparent (RFC wise) OAuth implementation. So, first of all, it has to be optional (since it is not a part of the standard). And secondly, as long as the field is there, it seems useful to automatically authenticate the Application as the User in case this mapping is set . Because otherwise this field has no apparent (to myself) meaning, and is trying to be what it can't (see the above paragraph).

Honestly, I would even think of adding "allowed scopes" to the application definition - should we continue on the "toolkit over the RFC" approach... However the same result may be achieved with an unbound Application and a scoped refresh token authorized by a resource owner for the client represented by the Application. Which again, puts to question the rationale behind this field being at all defined at the level of an abstract Application.

from django-oauth-toolkit.

Actually, I find this draft diagram posted here by @masci quite appealing. I would just say that it lacks a permission system to close the loop and bind an Owner to a resource, with the RFC scopes representing the permission grants.

from django-oauth-toolkit.

Unfortunately, I don't see any problem in your use case :). Why shouldn't the confidential client get access to the resource owner's profile? If I didn't want it, I wouldn't set the mapping between those entities

I agree, the confidential client should access resource owner data if you need it, but this is is not the default behavior. This is DOT choice.

The actual mapping between clients and resources will in many cases be much more complicated than the one you can express with a single relationship. So this should not be treated as "the way" of managing Applications.

It's not "the way". It's just the default choice in DOT. You can customize it through the settings. Maybe we can make it more easy, but you can already customize pretty much everything in DOT.

Having that said, I see the User field in Application as a neat free addition to the otherwise transparent (RFC wise) OAuth implementation. So, first of all, it has to be optional (since it is not a part of the standard).

👍

And secondly, as long as the field is there, it seems useful to automatically authenticate the Application as the User in case this mapping is set. Because otherwise this field has no apparent (to myself) meaning, and is trying to be what it can't (see the above paragraph).

Not true, the user field says who owns the Application. For example, it is used in the application's management views to filter the queryset that shows the managed application.

Honestly, I would even think of adding "allowed scopes" to the application definition - should we continue on the "toolkit over the RFC" approach...

👍

Also consider that it is difficult to find a default behavior that pleases everyone.

from django-oauth-toolkit.

Also consider that it is difficult to find a default behavior that pleases everyone.

This is why the default should be following the standard, then there will be no questions

from django-oauth-toolkit.

For several hours I was looking for a solution about this problem
Thanks to @poswald I was able to fix it.

from django-oauth-toolkit.