Canvas Integration #116
Canvas Integration #116
Conversation
|
Hello @bradley-erickson, please help me review this new PR. This one has fewer and only relevant files in it and it now matches the up-to-date code with PMSS being used. However, I still used a Other than that, I have applied the review comments you made on the previous PR. |
| token_uri: {canvas-token-uri} | ||
| client_id: {canvas-client-id} | ||
| client_secret: {canvas-client-secret} | ||
| refresh_token: {canvas-refresh-token} |
There was a problem hiding this comment.
@bradley-erickson When is the time to move this to PSS?
|
|
||
| return decorator | ||
|
|
||
| def make_ajax_raw_handler(raw_ajax_function, remote_url): |
There was a problem hiding this comment.
At the very least, the docstring should explain enough to be able to use this securely.
|
|
||
| return caller | ||
|
|
||
| def api_docs_handler(endpoints): |
There was a problem hiding this comment.
What is the security model here? Shouldn't these check if the user is authenticated or similar?
| return aiohttp.web.json_response(response) | ||
| return ajax_passthrough | ||
|
|
||
| def make_cleaner_handler(raw_function, cleaner_function, name=None): |
There was a problem hiding this comment.
Same comments going down. Think through docstrings, security, etc.
|
|
||
| # Ensure Google requests are authenticated | ||
| if lms_name == constants.GOOGLE and constants.AUTH_HEADERS not in request: | ||
| raise aiohttp.web.HTTPUnauthorized(text="Please log in") |
There was a problem hiding this comment.
I am glad you're handling errors, but this might be an incorrect user error depending on what's going on. For example, if a user is logged in with Canvas auth, but is trying to do a Google AJAX call....
Many users will use neither, either, or both, and we should make sure all those use-cases work fine.
There was a problem hiding this comment.
You should not be doing this type of low-level stuff here. This should all be calls to generic functions.
- Step through and make sure every externally-facing call checks that the user has the right permissions to access the resource
- Use the generic stuff functions, ideally, as decorators to do this
In general, be very, very careful not to reimplement security code. If we change header names, add an auth scheme, etc. the existing code will break, and it could break in a way which leads to an exploitable hole.
|
Holistically, this looks very good and quite close. I left many comments throughout the code, mostly focused on:
Aside from understanding security model, I have no major comments; just the kinds of polish generally needed for a final merge. Thank you for the contribution. |
| required=True | ||
| ) | ||
|
|
||
| cache = None |
There was a problem hiding this comment.
This is used, but I can't find it being set anywhere. If it's set somewhere, that should be documented. If not, it should be removed.
You should document why we have / need a cache, and what is stored in it.
There was a problem hiding this comment.
We reviewed. This is used for off-line demos and development. It saves calls to Google, and allows us to pretend Google exists in an off-line setting. I can masquerade as if I were on the real server.
This doesn't work in the new code. It should be:
- Fixed
- Tested
- Properly documented
What it did was confusing in the existing code (pre-PR).
| "grant_type": "refresh_token", | ||
| 'client_id': settings.pmss_settings.client_id(types=['lms', 'canvas_oauth']), | ||
| 'client_secret': settings.pmss_settings.client_secret(types=['lms', 'canvas_oauth']), | ||
| "refresh_token": settings.pmss_settings.refresh_token(types=['lms', 'canvas_oauth']) |
There was a problem hiding this comment.
From my understanding of the Oauth workflow, the refresh_token should be tied to the user signing in and not generically tied to the system. This token is used to fetch new access_tokens when needed without requiring another sign-on.
The Google API code in LO did not implement the refresh_token functionality, though it should do something with it.
| assert 'access_token' in data, data | ||
|
|
||
| # get user profile | ||
| canvas_headers = {'Authorization': 'Bearer ' + data['access_token']} |
There was a problem hiding this comment.
We ought to be storing more than just the access_token for each user. We should include the refresh_token if available and some additional metadata about when the sign-in occurred.
| session_user = session.get(constants.USER, None) | ||
| if constants.AUTH_HEADERS in session: | ||
| request[constants.AUTH_HEADERS] = session[constants.AUTH_HEADERS] | ||
| header_keys = [constants.AUTH_HEADERS, constants.CANVAS_AUTH_HEADERS] |
There was a problem hiding this comment.
Code like this won't scale easily to more LMSs. We ought to have a better auth system that can handle multiple auths at once.
Example: a teacher might need their roster from Canvas, but also needs to sign into Google to get updated document text.
| ) | ||
|
|
||
| user = await _google(request) | ||
| user = await _handle_google_authorization(request) |
There was a problem hiding this comment.
This function ought to handle a single option of signing in for a given API.
When we create the endpoints for Oauth, we ought to initialized them separately.
For example in Routes, where we initialize the google oauth route, we should use
# Copied from learning_observer/routes.py:239
if 'google_oauth' in settings.settings['auth']:
debug_log("Running with Google authentication")
app.add_routes([
aiohttp.web.get(
'/auth/login/{provider:google}',
# the below call to `social_handler` ought to pass a parameter "google"
handler=learning_observer.auth.social_handler),
])For the canvas oauth routes, we should add a similar route but with
handler=learning_observer.auth.social_handler('canvas') or similar.
The passed method should call the appropriate authorization function instead of always calling Google and sometimes calling Canvas/other LMSs
No description provided.