Launch Canvas Studio assignments using an admin account

[robertknight]

The Canvas Studio API has a constraint that only the owner of a video or an admin can use the APIs for getting a video download/playback URL or the transcript. Before this PR, only the instructor who uploaded a video could actually launch the assignment.

In #6160 a new application instance setting canvas_studio.admin_email was added, which specifies the email address of a user with admin permissions in Canvas Studio. In this PR, when an assignment is launched, an API token belonging to this user is used to fetch the video and transcript. In order for this to work, the admin user must authenticate Canvas Studio once by configuring or launching an assignment. The user does not have to be an administrator within Canvas itself, they just need to have the necessary permissions with Canvas Studio.

This solution is not ideal because it requires extra work for end-users to set up, and if the admin user ever revokes API access in Canvas Studio, launching assignments will break for everyone, until the admin user re-authenticates.

Summary of changes:

• Add the ability to construct instances of OAuthHTTPService which use a specified user ID, instead of the user ID associated with the current request
• Change CanvasStudioService to create two instances of OAuthHTTPService, one for the current user and one for the admin user
• Change the requests in CanvasStudioService which fetch the transcript and video to use the admin user instance of OAuthHTTPService, if the current user is not the admin user

Testing:

1. Create a Canvas Studio assignment locally
2. Attempt to launch it. This should fail with an error that no admin account is configured
3. Go into the settings for the application instance and add credentials for a user under "Canvas Studio admin account email". For testing purposes this user doesn't have to be an admin - the email of the "teacher" account you are using (who owns the video) will work
4. Launch the assignment using whichever account you have configured as the admin account
5. In a different browser, launch the assignment as a student. This should now work.

There are a variety of error scenarios that we need to handle:

• The canvas_studio.admin_email setting is not configured
• The current user is not an admin, and the admin user has never launched Hypothesis, so we can't look up their LTI user ID
• The current user is not an admin, the the admin user has never authenticated Canvas Studio, so we don't have a token we can use
• The current user is not an admin, and the admin user token refresh fails

See test cases for CanvasStudioService for the messages that each of these should generate.

[marcospri]

What this PR does is a bit hacky but it's a necessary work around to work with the Canvas Studio API.

As I mentioned elsewhere that we might even apply the same workaround (using a token for a user other than the current one ) in the future, for example in course copy.

Pointed to a possible refactor to avoid making queries to User from outside UserSerivce. That relatively straightforward and we use that pattern elsewhere. We could in any case do that in follow up PR

I also mention the possibility of a refactor in the OAuth2Service factory, avoid passing there the current requests details and move those to explicit parameters in .get/.save. That's probably something to discuss beforehand as it might be a bigger change.

[marcospri]

IMO this should be refactored into UserService

That could be done in a number of way, my prefered option would be:

public UserService.search taking optional:

• application_instance
• user_id
• email

Update the existing private _user_search_query to support, conditally, all of these parameters.

Some examples:

• https://github.com/hypothesis/lms/blob/main/lms/services/organization.py#L86

• https://github.com/hypothesis/lms/blob/main/lms/services/application_instance.py#L161

[robertknight]

A difference from the search APIs is that here I'm using one_or_none both to get a single item instead of a list, and as an assert that we never have ambiguity over the results. This could still go in UserService, but it would be more of a "fetch"/"lookup" API.

[marcospri]

You can do one_or_none in a search type interface (but it will be uglier) but you can't do search in a fetch/lookup interface.

One alternative we haven't used yet would be to expose the search query method returning a query object instead of one or a list of models.

That way the centralization benefits of having it over UserService will still be there (eg I we wanted to exclude an hypothetical deleted=True it will be a one line change) but you still get the flexibility of other services executing the query themselves (ie they can use .one() .one_or_none() or even use the query as a subquery...)

[marcospri]

Arguably this could look nicer with a @property

[marcospri]

@property are a bit more tricky to mock so that could be good reason to prefer not to use them.

[robertknight]

Do we have strong conventions in the codebase about when to use properties and when to use methods without arguments? On the JS side we rarely use getters (although they are certainly "allowed"), which I think is mostly a convention arising from developers' backgrounds / preferences.

[marcospri]

Nah, mostly personal preference, I'd say obj.value() looks ugly but again, personal preference.

[marcospri]

Same here, @property would look nicer

[marcospri]

This is IMO a very extended attipattern in the LMS code base

The service factories create services that are only useful in one context, here only useful to fetch the current user in LTI launch context.

Saving one method parameter complicates things everywhere else.

I reckon we should aim to take these explicitly as method parameters making the LTI use case just a tight more complex (ie passing a value that's readily available everywhere as request.lti_user) but making more complex use cases like this one possible without having to come up with new patterns and heachaches.

That being said, depending on the service that could be costly refactor.

That being said, parametrizing the factory as one here also works 👍

[marcospri]

That being said, depending on the service that could be costly refactor.

At first glance it looks mostly like a tedious refactor rather than complex one.

[robertknight]

The service factories are automatically invoked by the pyramid_services machinery when calling request.find_service, so that presumably means they have to be able to work without required extra arguments?

[marcospri]

My point is that these should not be service arguments but extra service method parameters, ie making this not the "Current user token service" but "User token service".

I reckon in this case, as this is the first time we have this need the refactor will look much more verbose as this ties into the HTTPService as well. Calling the service factory does the trick thought.