Skip to content
This repository has been archived by the owner on Aug 15, 2024. It is now read-only.

Fixes #2 - Initial commit to add OAuth support #98

Merged
merged 7 commits into from
Apr 11, 2022

Conversation

jonespm
Copy link
Member

@jonespm jonespm commented Oct 12, 2021

This adds support for OAuth. There is some configuration needed for this to work.

Testing this app for LTI locally if you were doing that is kind of a pain. If you use the development mode. You need ngrok with 2 URL's as described on https://github.com/tl-its-umich-edu/canvas-app-explorer/wiki/Configuring-LTI-1.3. If you restart Ngrok you'll have to both change Canvas and the value in the .env. Alternatively you can use the docker-compose-openshift-test-yml which only needs a single port but doesn't auto reload.

To test the OAuth you'll have to generate an API key (scopes don't matter yet) and have the redirect URL setup as
https://<ngrok.address>/oauth/oauth-callback

Then add the values of the developer key to settings the env in

# CANVAS_OAUTH_CLIENT_ID=

# (required) The client secret is the random string (secret) value of your Canvas developer key.
# CANVAS_OAUTH_CLIENT_SECRET=

# (required) The domain of your canvas instance (e.g. canvas.instructure.com)
# CANVAS_OAUTH_CANVAS_DOMAIN=

There are at least 2 big improvements than can be made on this

  1. Write a custom AUTHENTICATION_BACKEND. Currently this uses the default ['django.contrib.auth.backends.ModelBackend'] and logs in the user through this backend. We should have a custom one for LTI. This will likely be a separate issue and is one we've wanted for other apps like MYLA as well. Write Custom AUTHENTICATION_BACKEND for PyLTI13 #119

This was a sample from one LTI project. https://github.com/wachjose88/django-lti-provider-auth/blob/master/lti_provider/backends.py
Here was another: https://github.com/ccnmtl/django-lti-provider/blob/master/lti_provider/auth.py

  1. Store the OAuth configuration in the database along with the LTI Tools. This app's LTI config in the database can support multi-tenant, however the OAuth config is only a single value defined in the settings. For this to be multi-tenant this will need to extend and reference the LTI's tool config model. OAuth config should be stored in a table #120

@jonespm jonespm linked an issue Nov 11, 2021 that may be closed by this pull request
@jonespm jonespm force-pushed the issue_2 branch 3 times, most recently from 3f55b12 to de8887c Compare January 21, 2022 18:41
@jonespm jonespm marked this pull request as ready for review February 24, 2022 20:13
@ssciolla
Copy link
Contributor

I think I'll have some time to look at this tomorrow.

@jonespm
Copy link
Member Author

jonespm commented Feb 25, 2022

Thanks @ssciolla ! We're frozen for new features for user testing next week so no rush, whenever you have time. I'm available most of tomorrow too if you wanted to chat about anything. This looks simple but it was a lot of trial and error to get here and it very likely has a few adjustments beyond the issues I mentioned.

The next step of this is going to be using the token against the API #32 which I'm going to be working on next week.

@zqian
Copy link
Member

zqian commented Feb 25, 2022

@jonespm l'd like to join the chat on this. I had some problem getting the custom field values in #32

.env.sample Show resolved Hide resolved
Copy link
Contributor

@ssciolla ssciolla left a comment

Choose a reason for hiding this comment

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

I was able to get this to work, and I think it's okay to merge. I made some comments as appropriate.

I did notice that there was message about model changes not reflected in migrations related to canvas_oauth; my guess is it's likely the library owners changed the Meta without creating a new migration. Probably won't affect performance of our app, but we could open an issue in their repo. I'm a little (but perhaps not overly) concerned with relying on this package (no updates since 2020). If things get out of date, we may need to contribute.

backend/canvas_app_explorer/lti1p3.py Outdated Show resolved Hide resolved
backend/canvas_app_explorer/lti1p3.py Outdated Show resolved Hide resolved
backend/canvas_app_explorer/lti1p3.py Outdated Show resolved Hide resolved
backend/canvas_app_explorer/urls.py Outdated Show resolved Hide resolved
backend/settings.py Outdated Show resolved Hide resolved
backend/settings.py Outdated Show resolved Hide resolved
backend/views.py Outdated Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
backend/settings.py Show resolved Hide resolved
@ssciolla ssciolla added enhancement New feature or request backend labels Mar 29, 2022
@jonespm jonespm merged commit baf83ca into tl-its-umich-edu:main Apr 11, 2022
@jonespm jonespm deleted the issue_2 branch April 11, 2022 19:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backend enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OAuth to allow usage of Canvas API
3 participants