Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for client_credentials grant type #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brandon-miles
Copy link

This adds support for the client credentials grant as found in section 4.4 of the RFC.

@eculver
Copy link
Contributor

eculver commented Nov 6, 2013

Hi @brandon-miles. Thanks for contributing this. I have a few questions/concerns though. I don't see any provisions made for authenticating the client. The specification makes it pretty clear in Section 3.2.1 that the authorization server must authenticate the client:

Confidential clients or other clients issued client credentials MUST
authenticate with the authorization server as described in
Section 2.3 when making requests to the token endpoint.

The specification does suggest a few means for doing this in Section 2.3 which we should probably explore.

So, while client_credentials is certainly a valid grant_type, the use of it implies other provisions have been made to guarantee the authentication of the client. @flashingpumpkin can correct me if I'm wrong, but this is outside of the original scope of the authentication provider. However, this is not to say that this shouldn't necessarily be supported. I personally feel that this should be supported though. We do need to start the discussion of how the client should and shouldn't be authenticated though if we do. I think this is the real burden of this PR.

Also, you don't have tests :(

Let's get this started though. If you're implementing this grant type, what is your use case? Do you have a real world implementation that you're accommodating that we could reference?

My initial thoughts are to provide some sort of authentication backend for authenticating the client based on upon credentials that are agreed upon up front (username+password, public key, etc.) and then extending the access token view to require authentication for this case. Just a thought...

@brandon-miles
Copy link
Author

Hi @eculver,

Thanks for taking a look. In our implementation we allow a user to register a client (obtaining a client_id and client_secret) once they have authenticated via our corporate auth. Since we initially have to redirect the user to a corporate auth URL, we never have a chance to accept their username + password, so the password grant simply won't work for us.

We also don't have a choice as to what the corporate auth sends us back as a response (they just send us back an LDAP entry), so the code grant won't work for us either.

The client_credentials grant works well for us since a user has to be authenticated in order to register a client. I think that while this isn't the preferred grant type, it works well for scenario like ours where we have no control over the authorization server or what it sends us back.

As for the tests, sorry for not including those. I'll get those added to this PR when I get a chance.

Let me know if I'm missing anything here, or if you have any questions on our particular use case.

Thanks again!

-Brandon

@brandon-miles
Copy link
Author

Ok, tests have been added. Let me know if there's anything else I missed.

@brandon-miles
Copy link
Author

Hi @eculver,

The tests have been added a while back. Can you have a look at this again when you get a chance? Thanks!

@nvillalva
Copy link

Hi @brandon-miles,

We're using this package and are really hoping to get client credentials support in, so I pulled a copy of your branch and started to explore.

With Client Credentials the user to associate with the issued AccessToken and the stored Grant comes into play (No RefreshToken should be issued per the spec). Currently, Client allows a null user and, when authenticated through Client Credentials, the associated access token should be for operating on behalf of the client (or the user that owns the client) and not an end user. Not requiring a user to own a Client makes sense but we'll see an exception when a client with no associated user tries to get a token through this grant type. The way I see it there are two options:

  • Modify the AccessToken, RefreshToken and Grant models to accept empty users, or
  • Add a default user object (maybe as a settings option) and apps using the provider could default to this (hopefully anonymous) user instead of None

As far as authentication, since we don't necessarily have a client.user or request.user those options aren't as viable. The only additional verification I can think of is to check the request origin against client.url -- basically treating client.url as a url slug for valid requests. I'm not sure if that would be necessary or even helpful.

Hopefully we can get this PR merged in soon :)

-Nick

@andrefsp
Copy link

What's the status of this PR?
My personal opinion is that user fields on AccessToken, RequestToken and Grant models should be nullable. This way it is possible to differentiate a user access token from an app/client access token.

With the current changes and with the user nullable I don't think there will be any violation of the OAuth2.0 protocol when requesting an access token.

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

@ryanisnan
Copy link

If I could add to this discussion; I've been reviewing this lib and django-oauth-toolkit, and this is one of the more annoying points in that library as well. Tokens have to be bound to users, but this design decision seems to be an oversight when looking at the client_credentials grant type in particular.

Tokens should not be required to be bound to users, as they can be assigned to an application/client directly.

One other consideration is that client_credentials is one of the major grant types defined in the specification; this library should be considered incomplete until its support has been included.

@cambridgemike
Copy link

@eculver Is merging this PR on the roadmap?

@clintonb
Copy link

Any update on this issue? I'd really like to make use of this functionality for my server-to-server APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants