Added support for client_credentials grant type #25

clintonb · 2016-02-04T22:09:45Z

I am adding this functionality so that our services can get access tokens for service accounts. This is preferable to hacking around the issue or creating a long-lived access token. Adapted from caffeinehit#47.

FYI @nasthagiri @jcdyer @douglashall @rlucioni @jimabramson

TODO

Test locally and on sandboxes
Security review (@nasthagiri you are our resident OAuth 2.0 expert. Any glaring issues here?)

clintonb · 2016-02-05T05:46:12Z

FYI @bderusha, this will aid in authentication for Course Discovery's refresh management command.

rlucioni · 2016-02-05T14:59:25Z

@clintonb given the nature of this change, I think it would be appropriate to bump the version of this package.

rlucioni · 2016-02-05T15:01:12Z

provider/views.py

+        scope = data.get('scope')
+
+        if constants.SINGLE_ACCESS_TOKEN:
+                at = self.get_access_token(request, client.user, scope, client)


Nitpick: Looks like this is indented too far.

rlucioni · 2016-02-05T16:02:12Z

👍 once version incremented.

clintonb · 2016-02-05T20:28:28Z

Having spent a couple hours trying to increase test coverage, specifically focusing on the case when the endpoint should not issue a refresh token, I am giving up. Given that we do use refresh tokens, there is no point in focusing on ensuring that path is covered. The source of the test failure has to do with the constants.SINGLE_ACCESS_TOKEN and the fact that I cannot seem to override its value in tests.

nasthagiri · 2016-02-05T20:44:19Z

provider/oauth2/tests.py

@@ -597,6 +597,25 @@ def test_access_token_response_valid_token_type(self):
        token = self._login_authorize_get_token()
        self.assertEqual(token['token_type'], constants.TOKEN_TYPE, token)

+    def test_client_credentials_grant(self):


Nit:

Separating the tests here into individual test methods.

For comprehensiveness, add a test for invalid client_id with client_credentials grant_type.

So, in the end, you would have: test_client_credentials_grant_success, test_client_credentials_grant_invalid_secret, test_client_credentials_grant_invalid_client_id.

clintonb · 2016-02-06T00:39:19Z

@nasthagiri it turns out refresh tokens were being created whenever access tokens were created, regardless of whether the refresh token was needed/requested. I have removed that functionality, and updated the tests appropriately. Please take another look.

@rlucioni please take another look as well.

nasthagiri · 2016-02-06T13:45:37Z

provider/oauth2/views.py

        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)


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.)

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.

I see we are making 2 assumptions here:

All operators of open edX will never enable constants.SINGLE_ACCESS_TOKEN.

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?

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

nasthagiri · 2016-02-06T13:56:41Z

@clintonb I have completed my latest pass. This is looking good! A few remaining comments.

rlucioni · 2016-02-07T04:09:03Z

Looks nice @clintonb, 👍.

nasthagiri · 2016-02-08T19:52:03Z

provider/views.py

+            'scope': data.get('scope'),
+            'client': client,
+            'create_refresh_token': False,
+        }


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

Yes. Added.

nasthagiri · 2016-02-08T20:41:48Z

👍
Thanks for taking this on @clintonb. Looks good. Pending one question.

- Removed creation of refresh token alongside access token - Mocking constants.SINGLE_ACCESS_TOKEN instead of changing its value, thereby breaking other tests

Added support for client_credentials grant type

clintonb changed the title ~~WIP: Added support for client_credentials grant type~~ Added support for client_credentials grant type Feb 5, 2016

rlucioni reviewed Feb 5, 2016
View reviewed changes

clintonb force-pushed the clintonb/client-credentials branch 5 times, most recently from cb116b6 to 05673d9 Compare February 5, 2016 19:53

nasthagiri reviewed Feb 5, 2016
View reviewed changes

clintonb force-pushed the clintonb/client-credentials branch 3 times, most recently from f7db444 to 199b0ed Compare February 6, 2016 00:37

nasthagiri reviewed Feb 6, 2016
View reviewed changes

clintonb force-pushed the clintonb/client-credentials branch 2 times, most recently from 887f3f6 to ccbeefa Compare February 8, 2016 16:29

clintonb mentioned this pull request Feb 8, 2016

Code Cleanup #27

Merged

clintonb force-pushed the clintonb/client-credentials branch from ccbeefa to 97abf4f Compare February 8, 2016 17:18

nasthagiri reviewed Feb 8, 2016
View reviewed changes

De-coupled refresh token creation from access token creation

5c146ab

- Removed creation of refresh token alongside access token - Mocking constants.SINGLE_ACCESS_TOKEN instead of changing its value, thereby breaking other tests

clintonb force-pushed the clintonb/client-credentials branch from 97abf4f to 5993f8d Compare February 8, 2016 21:09

Added support for client credentials grant

a636a72

clintonb force-pushed the clintonb/client-credentials branch from 5993f8d to a636a72 Compare February 8, 2016 21:35

clintonb added a commit that referenced this pull request Feb 8, 2016

Merge pull request #25 from edx/clintonb/client-credentials

e709d71

Added support for client_credentials grant type

clintonb merged commit e709d71 into edx Feb 8, 2016

clintonb deleted the clintonb/client-credentials branch February 8, 2016 22:00

nasthagiri mentioned this pull request Feb 10, 2016

Implement third party auth ID mapping API openedx/edx-platform#9842

Merged

clintonb mentioned this pull request Apr 13, 2016

Updated the default scopes of the client credentials grant #32

Merged

Added support for client_credentials grant type #25

Added support for client_credentials grant type #25

Uh oh!

Conversation

clintonb commented Feb 4, 2016

Uh oh!

clintonb commented Feb 5, 2016

Uh oh!

rlucioni commented Feb 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlucioni commented Feb 5, 2016

Uh oh!

clintonb commented Feb 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintonb commented Feb 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nasthagiri commented Feb 6, 2016

Uh oh!

rlucioni commented Feb 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nasthagiri commented Feb 8, 2016

Uh oh!

Uh oh!