Skip to content
This repository was archived by the owner on Mar 3, 2020. It is now read-only.

Added support for client_credentials grant type #25

Merged
merged 2 commits into from
Feb 8, 2016
Merged

Added support for client_credentials grant type #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c146ab
De-coupled refresh token creation from access token creation
clintonb Feb 6, 2016
a636a72
Added support for client credentials grant
clintonb Feb 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion provider/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
__version__ = '0.4.0'
__version__ = '0.5.0'
13 changes: 12 additions & 1 deletion provider/oauth2/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from django.utils.encoding import smart_unicode
from django.utils.translation import ugettext as _

from provider import scope
from provider import constants, scope
from provider.constants import RESPONSE_TYPE_CHOICES, SCOPES
from provider.forms import OAuthForm, OAuthValidationError
from provider.oauth2.models import Client, Grant, RefreshToken
Expand Down Expand Up @@ -336,3 +336,14 @@ def clean(self):

data['client'] = client
return data


class ClientCredentialsGrantForm(ScopeMixin, OAuthForm):
""" Validate a client credentials grant request. """

def clean(self):
cleaned_data = super(ClientCredentialsGrantForm, self).clean()
# We do not fully support scopes for this grant type; however, a scope is required
# in order to create an access token. Default to read-only access.
cleaned_data['scope'] = constants.READ
return cleaned_data
65 changes: 65 additions & 0 deletions provider/oauth2/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,71 @@ def test_access_token_response_valid_token_type(self):
self.assertEqual(token['token_type'], constants.TOKEN_TYPE, token)


@ddt.ddt
class ClientCredentialsAccessTokenTests(BaseOAuth2TestCase):
""" Tests for issuing access tokens using the client credentials grant. """
fixtures = ['test_oauth2.json']

def setUp(self):
super(ClientCredentialsAccessTokenTests, self).setUp()
AccessToken.objects.all().delete()

def request_access_token(self, client_id=None, client_secret=None):
""" Issues an access token request using the client credentials grant.

Arguments:
client_id (str): Optional override of the client ID credential.
client_secret (str): Optional override of the client secret credential.

Returns:
HttpResponse
"""
client = self.get_client()
data = {
'grant_type': 'client_credentials',
'client_id': client_id or client.client_id,
'client_secret': client_secret or client.client_secret,
}

return self.client.post(self.access_token_url(), data)

def assert_valid_access_token_response(self, access_token, response):
""" Verifies the content of the response contains a JSON representation of the access token.

Note:
The access token should NOT have an associated refresh token.
"""
expected = {
u'access_token': access_token.token,
u'token_type': constants.TOKEN_TYPE,
u'expires_in': access_token.get_expire_delta(),
u'scope': u' '.join(scope.names(access_token.scope)),
}

self.assertEqual(json.loads(response.content), expected)

def get_latest_access_token(self):
return AccessToken.objects.filter(client=self.get_client()).order_by('-id')[0]

def test_authorize_success(self):
""" Verify the endpoint successfully issues an access token using the client credentials grant. """
response = self.request_access_token()
self.assertEqual(200, response.status_code, response.content)
Copy link

@nasthagiri nasthagiri Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the success case, verify that:

  1. An access_token is returned (with an expires_in field).
  2. A refresh_token is NOT returned.

Copy link
Author

@clintonb clintonb Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my latest comment regarding testing for refresh tokens. This code base is...not ideal.

Copy link

@nasthagiri nasthagiri Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. Perhaps I missed an earlier conversation on this.
But the RFC specifically states in Section 4.4.3 for the Client Credentials Grant:

A refresh token SHOULD NOT be included.

So, if the Client Credentials grant is implemented correctly, then we should make sure in the test that we don't return a refresh_token.

Copy link

@nasthagiri nasthagiri Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it as simple as adding a line: self.assertNotIn('refresh_token', response.data)?

Copy link
Author

@clintonb clintonb Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. This is why I wanted someone intimately familiar with the spec reviewing. This code is being adapted from caffeinehit#47. I admin I haven't fully read the spec, so I was unaware of the need to omit the refresh token. That is great, because testing that has proved frustrating!

Copy link

@nasthagiri nasthagiri Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. It's an unmerged PR and still has issues.


access_token = self.get_latest_access_token()
self.assert_valid_access_token_response(access_token, response)

@ddt.data(
{'client_id': 'invalid'},
{'client_secret': 'invalid'},
)
def test_authorize_with_invalid_credentials(self, credentials_override):
""" Verify the endpoint returns HTTP 400 if the credentials are invalid. """
response = self.request_access_token(**credentials_override)
self.assertEqual(400, response.status_code, response.content)
self.assertDictEqual(json.loads(response.content), {'error': 'invalid_client'})


class AuthBackendTest(BaseOAuth2TestCase):
fixtures = ['test_oauth2']

Expand Down
9 changes: 7 additions & 2 deletions provider/oauth2/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from provider import constants
from provider.oauth2.backends import BasicClientBackend, RequestParamsClientBackend, PublicPasswordBackend
from provider.oauth2.forms import (AuthorizationCodeGrantForm, AuthorizationRequestForm, AuthorizationForm,
PasswordGrantForm, RefreshTokenGrantForm)
PasswordGrantForm, RefreshTokenGrantForm, ClientCredentialsGrantForm)
from provider.oauth2.models import Client, RefreshToken, AccessToken
from provider.utils import now
from provider.views import AccessToken as AccessTokenView, OAuthError, AccessTokenMixin, Capture, Authorize, Redirect
Copy link

@rlucioni rlucioni Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (i.e., import AccessToken as AccessTokenView) threw me off. It's especially strange considering that a second AccessTokenView is being defined in this module.

I understand wanting to be as surgical as possible when it comes to touching this codebase. However, since you've fixed the imports here, would you be opposed to also fixing this so that AccessToken isn't renamed on import?

Copy link
Author

@clintonb clintonb Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. I'm not touching any of that.

Copy link

@jcdyer jcdyer Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An easy fix would be to import views and use views.AccessToken, etc. Or you could not touch it.

Copy link
Author

@clintonb clintonb Feb 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take "not touching the code we want to kill" for 400, please, Alex.

I seriously don't even want to make this PR. I would much rather invest the time/energy in getting us to django-oauth-toolkit. This works. It's tested. I'm not touching it. If anyone feels strongly about renaming, submit another PR.

Expand All @@ -24,7 +24,6 @@ def get_access_token(self, request, user, scope, client):
except AccessToken.DoesNotExist:
# None found... make a new one!
at = self.create_access_token(request, user, scope, client)
self.create_refresh_token(request, user, scope, at, client)
return at
Copy link

@nasthagiri nasthagiri Feb 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense since not all grant types should create a refresh token.
However, what about the grant types that DO require a refresh token?
Specifically, authorization_code and the non-Public password grants also call get_access_token - expecting to have created a refresh token.

In fact, looking at the code now, there's an unintentional bug with Public clients using the Password Grant with constants.SINGLE_ACCESS_TOKEN enabled - the code generated refresh tokens for them.

My suggestion to you is to use the recommendation in the Wrong Abstraction article. Have each of the 3 callers of the get_access_token method handle the DoesNotExist exception themselves. So, authorization_code and password for private Clients will create refresh tokens, while client_credentials and password for public Clients won't. (Alternatively, pass a boolean into get_access_token - ignoring the article.)

Copy link
Author

@clintonb clintonb Feb 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_access_token is only used if constants.SINGLE_ACCESS_TOKEN. To my knowledge, we do not have that functionality enabled. All of the handlers have a conditional similar to that seen at https://github.com/edx/django-oauth2-provider/blob/edx/provider/views.py#L569-L573. I don't think we (edX) actually use get_access_token.

Copy link

@nasthagiri nasthagiri Feb 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we are making 2 assumptions here:

  1. All operators of open edX will never enable constants.SINGLE_ACCESS_TOKEN.
  2. We will be removing this library in favor of django-oauth-toolkit so this is a temporary fix, anyway.

Not knowing what future lies ahead of us, in the meantime, why have broken functionality in this common library? My concern is that (A) current (if any open edX instance has enabled constants.SINGLE_ACCESS_TOKEN) and (B) future uses of this library would not be doing the correct thing.

(By the way, the Mobile team was, in fact, considering to enable constants.SINGLE_ACCESS_TOKEN on edx.org since the access control table continues to grow in the multi-device and multi-session use cases.)

So either: (1) remove support for constants.SINGLE_ACCESS_TOKEN entirely - with notification to edx-code, (2) pass in a boolean to this method on whether or not a refresh_token is expected, or (3) refactor the abstraction as suggested above. I believe, 2 or 3 should be reasonable to implement. What do you say?

Copy link
Author

@clintonb clintonb Feb 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented a combination of 2 and 3. Please have a look at get_access_and_refresh_tokens().


def create_access_token(self, request, user, scope, client):
Expand Down Expand Up @@ -140,6 +139,12 @@ def get_password_grant(self, request, data, client):
raise OAuthError(form.errors)
return form.cleaned_data

def get_client_credentials_grant(self, request, data, client):
form = ClientCredentialsGrantForm(data, client=client)
if not form.is_valid():
raise OAuthError(form.errors)
return form.cleaned_data

def invalidate_grant(self, grant):
if constants.DELETE_EXPIRED:
grant.delete()
Expand Down
102 changes: 77 additions & 25 deletions provider/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,25 @@ def access_token_response_data(self, access_token, response_type=None):

return response_data

def get_access_and_refresh_tokens(self, request, user, scope, client, reuse_existing_access_token=False, create_refresh_token=True):
"""
Returns an AccessToken and RefreshToken for the given user, scope, and client combination.

Returns:
(AccessToken, RefreshToken)
If create_refresh_token is False, the second element of the tuple will be None.
"""
if reuse_existing_access_token:
at = self.get_access_token(request, user, scope, client)
else:
at = self.create_access_token(request, user, scope, client)

rt = None
if create_refresh_token and not reuse_existing_access_token:
rt = self.create_refresh_token(request, user, scope, at, client)

return at, rt


class OAuthView(TemplateView):
"""
Expand Down Expand Up @@ -333,15 +352,14 @@ def get_implicit_response(self, request, client):
data = self.get_data(request)

lookup_kwargs = {
"user": request.user,
"client": client,
"scope": scope.to_int(*data.get('scope', constants.SCOPES[0][1]).split())
'user': request.user,
'client': client,
'scope': scope.to_int(*data.get('scope', constants.SCOPES[0][1]).split()),
'reuse_existing_access_token': constants.SINGLE_ACCESS_TOKEN,
'create_refresh_token': False
}

if constants.SINGLE_ACCESS_TOKEN:
token = self.get_access_token(request, **lookup_kwargs)
else:
token = self.create_access_token(request, **lookup_kwargs)
token, __ = self.get_access_and_refresh_tokens(request, **lookup_kwargs)

response_data = self.access_token_response_data(token, data['response_type'])

Expand Down Expand Up @@ -503,7 +521,7 @@ class AccessToken(OAuthView, Mixin, AccessTokenMixin):
Authentication backends used to authenticate a particular client.
"""

grant_types = ['authorization_code', 'refresh_token', 'password']
grant_types = ['authorization_code', 'refresh_token', 'password', 'client_credentials']
"""
The default grant types supported by this view.
"""
Expand Down Expand Up @@ -532,6 +550,14 @@ def get_password_grant(self, request, data, client):
"""
raise NotImplementedError # pragma: no cover

def get_client_credentials_grant(self, request, data, client):
"""
Return the optional parameters (scope) associated with this request.

:return: ``tuple`` - ``(True or False, options)``
"""
raise NotImplementedError # pragma: no cover

def invalidate_grant(self, grant):
"""
Override to handle grant invalidation. A grant is invalidated right
Expand Down Expand Up @@ -564,13 +590,16 @@ def authorization_code(self, request, data, client):
Handle ``grant_type=authorization_code`` requests as defined in
:rfc:`4.1.3`.
"""
grant = self.get_authorization_code_grant(request, request.POST,
client)
if constants.SINGLE_ACCESS_TOKEN:
at = self.get_access_token(request, grant.user, grant.scope, client)
else:
at = self.create_access_token(request, grant.user, grant.scope, client)
rt = self.create_refresh_token(request, grant.user, grant.scope, at, client)
grant = self.get_authorization_code_grant(request, request.POST, client)

kwargs = {
'request': request,
'user': grant.user,
'scope': grant.scope,
'client': client,
'reuse_existing_access_token': constants.SINGLE_ACCESS_TOKEN,
}
at, rt = self.get_access_and_refresh_tokens(**kwargs)

self.invalidate_grant(grant)

Expand All @@ -586,8 +615,13 @@ def refresh_token(self, request, data, client):
self.invalidate_refresh_token(rt)
self.invalidate_access_token(rt.access_token)

at = self.create_access_token(request, rt.user, rt.access_token.scope, client)
rt = self.create_refresh_token(request, at.user, at.scope, at, client)
kwargs = {
'request': request,
'user': rt.user,
'scope': rt.access_token.scope,
'client': client,
}
at, rt = self.get_access_and_refresh_tokens(**kwargs)

return self.access_token_response(at)

Expand All @@ -597,16 +631,32 @@ def password(self, request, data, client):
"""

data = self.get_password_grant(request, data, client)
user = data.get('user')
scope = data.get('scope')
kwargs = {
'request': request,
'user': data.get('user'),
'scope': data.get('scope'),
'client': client,
'reuse_existing_access_token': constants.SINGLE_ACCESS_TOKEN,

if constants.SINGLE_ACCESS_TOKEN:
at = self.get_access_token(request, user, scope, client)
else:
at = self.create_access_token(request, user, scope, client)
# Public clients don't get refresh tokens
if client.client_type == constants.CONFIDENTIAL:
rt = self.create_refresh_token(request, user, scope, at, client)
'create_refresh_token': client.client_type == constants.CONFIDENTIAL
}
at, rt = self.get_access_and_refresh_tokens(**kwargs)

return self.access_token_response(at)

def client_credentials(self, request, data, client):
""" Handle ``grant_type=client_credentials`` requests as defined in :rfc:`4.4`. """
data = self.get_client_credentials_grant(request, data, client)
kwargs = {
'request': request,
'user': client.user,
'scope': data.get('scope'),
'client': client,
'reuse_existing_access_token': constants.SINGLE_ACCESS_TOKEN,
'create_refresh_token': False,
}
Copy link

@nasthagiri nasthagiri Feb 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this also need 'reuse_existing_access_token': constants.SINGLE_ACCESS_TOKEN?

Copy link
Author

@clintonb clintonb Feb 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Added.

at, rt = self.get_access_and_refresh_tokens(**kwargs)

return self.access_token_response(at)

Expand All @@ -622,6 +672,8 @@ def get_handler(self, grant_type):
return self.refresh_token
elif grant_type == 'password':
return self.password
elif grant_type == 'client_credentials':
return self.client_credentials
return None

def get(self, request):
Expand Down