Skip to content
This repository was archived by the owner on Jun 6, 2019. It is now read-only.

Conversation

jimabramson
Copy link

@clintonb @rlucioni Please review.
@ormsbee @nasthagiri FYI.

This adds JWT authentication (and only JWT authentication) for the REST API. A little redirection trick when using the oidc login via browser makes the browseable API usable enough (though it doesn't handle refreshing expired tokens).

I have decided to leave the user model in place, even though it is theoretically not needed. The basic reasoning is that it helps to use django's APIs for groups and permissions, and perhaps more significantly, doesn't require a major overhaul of the social-auth pipeline / oidc client, which is a piece I've found rather difficult to debug.

At present one can only login to the API if a properly configured LMS is running to provision the id_tokens. In a separate PR we can modify auto-auth to generate self-issued tokens with long expirations which should make various kinds of manual/automated testing easier.

@jimabramson jimabramson force-pushed the jsa/ecom-2186-auth branch 2 times, most recently from 8d60ce4 to 96d6d51 Compare September 28, 2015 16:27
Copy link
Contributor

Choose a reason for hiding this comment

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

Use .get() here to avoid KeyErrors. If the key is missing, raise DRF's AuthenticationFailed exception.

@jimabramson
Copy link
Author

@rlucioni thanks for feedback - addressed your comments in a fixup.

@rlucioni
Copy link
Contributor

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Make these the default authentication class and permissions in settings.

http://www.django-rest-framework.org/api-guide/settings/

Copy link
Author

Choose a reason for hiding this comment

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

good idea, done

@clintonb
Copy link
Contributor

Why not use session authentication for the browseable API?

@jimabramson
Copy link
Author

@clintonb main reason to disable session auth is to avoid the need for CSRF protection.

@jimabramson
Copy link
Author

@clintonb updated with minor improvements to authentication settings.

@clintonb
Copy link
Contributor

clintonb commented Oct 1, 2015

@jimabramson what's the harm in CSRF? What, if anything, do we gain from trying to work around de-facto standard behavior?

@jimabramson
Copy link
Author

@clintonb do you have a suggestion for how to pass / use csrf tokens when the client is using JWT authentication?

@clintonb
Copy link
Contributor

clintonb commented Oct 1, 2015

Session authentication is only used for the browseable API, and all of
those pages already know how to handle CSRF tokens. You can continue using
JWT authentication for all other requests.

Clinton Blackburn

@jimabramson
Copy link
Author

@clintonb last fixup adds session authentication and removes the query string / redirection hackery. i've verified that requests using session auth require a csrf token, but jwt auth requests do not. hooray, less code. thanks for the help!

Copy link
Contributor

Choose a reason for hiding this comment

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

Move both of the authentication and permission declarations to the defaults in settings. The (very few?) classes that need to override them can, but I'd imagine almost all of the endpoints will use the same authentication and permissions.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is an easier way to create custom permissions: https://courses.stage.edx.org/api/credit/v1/courses/.

Also, the add permission should have been created when the model was created, along with the change and delete permissions.

Copy link
Author

Choose a reason for hiding this comment

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

@clintonb i added a comment in the last commit explaining why the permission is being created explicitly during the migration.

@jimabramson
Copy link
Author

@clintonb cleaned up.

@clintonb
Copy link
Contributor

clintonb commented Oct 1, 2015

👍

jimabramson pushed a commit that referenced this pull request Oct 1, 2015
Added JWT authentication, initial roles and permissions
@jimabramson jimabramson merged commit 633090e into master Oct 1, 2015
@jimabramson jimabramson deleted the jsa/ecom-2186-auth branch October 1, 2015 21:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants