Add support for OAuth2 authentication - #25
Conversation
| 'djangorestframework>=3.2.0,<3.4.0', | ||
| 'djangorestframework-jwt>=1.6.0,<=1.7.2', | ||
| 'edx-opaque-keys>=0.1.2,<1.0.0', | ||
| 'edx-djangorestframework-oauth==1.0.1', |
There was a problem hiding this comment.
@clintonb worth mentioning: if we get rid of the edx-djangorestframework-oauth fork as proposed in https://github.com/edx/edx-platform/pull/11559, we'll need to carefully order imports here, too, so that django-oauth2-provider is shadowed.
|
@rlucioni why no tests? |
ecf73f4 to
c207428
Compare
|
@jimabramson miss on my part. Tests added. |
| self.organization_list_url, | ||
| HTTP_AUTHORIZATION='Bearer {}'.format('nonexistent-access-token') | ||
| ) | ||
| self.assertEqual(response.status_code, 403) |
There was a problem hiding this comment.
if OAuth2Authentication is moved to the top of the authentication_classes list, I think this response code will become a 401, which is a more desirable (i.e. correct) result.
There was a problem hiding this comment.
I also thought re-ordering the authentication classes would yield the more correct 401, but it doesn't appear to. I'll poke around a little longer.
There was a problem hiding this comment.
Turns out re-ordering the classes does work as expected. I was being thrown off by some test cases. The view now returns a 401 when authentication fails.
|
@rlucioni apart from my proposal to try and get a 401 instead of a 403, LGTM 👍 |
c207428 to
2a55af2
Compare
Add support for OAuth2 authentication
3 participants
A management command in the programs app will make calls to the API provided by this package with an OAuth 2.0 access token. Requires changes from edx/django-rest-framework-oauth#2.
@jimabramson please review.