From 22aba8ec3e369ad7e23967d19bde4cddf4f91feb Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Tue, 20 Dec 2022 12:32:53 +0100 Subject: [PATCH 1/4] Add required scopes for file proxing --- lms/views/api/d2l/authorize.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/views/api/d2l/authorize.py b/lms/views/api/d2l/authorize.py index 47fd14099a..e57574d16e 100644 --- a/lms/views/api/d2l/authorize.py +++ b/lms/views/api/d2l/authorize.py @@ -8,7 +8,7 @@ from lms.validation.authentication import OAuthCallbackSchema GROUPS_SCOPES = ("groups:*:*",) -FILES_SCOPES = ("content:toc:read",) +FILES_SCOPES = ("content:toc:read", "content:topics:read") @view_config( From f52278b03ec311f6e47e932b942f330cb1507ee0 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Tue, 20 Dec 2022 12:33:42 +0100 Subject: [PATCH 2/4] Via url endpoint for D2L files --- lms/resources/_js_config/__init__.py | 11 +++++++ lms/routes.py | 3 ++ lms/views/api/d2l/files.py | 48 ++++++++++++++++++++++++++-- 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/lms/resources/_js_config/__init__.py b/lms/resources/_js_config/__init__.py index 541ad389a0..bc768a4ee5 100644 --- a/lms/resources/_js_config/__init__.py +++ b/lms/resources/_js_config/__init__.py @@ -7,6 +7,7 @@ from lms.models import Assignment, GroupInfo, Grouping, HUser from lms.product.blackboard import Blackboard from lms.product.canvas import Canvas +from lms.product.d2l import D2L from lms.resources._js_config.file_picker_config import FilePickerConfig from lms.services import HAPIError, JSTORService, VitalSourceService from lms.validation.authentication import BearerTokenSchema @@ -68,6 +69,16 @@ def add_document_url(self, document_url): resource_link_id=self._request.lti_params["resource_link_id"], ), } + elif document_url.startswith("d2l://"): + self._config["api"]["viaUrl"] = { + "authUrl": self._request.route_url(D2L.route.oauth2_authorize), + "path": self._request.route_path( + "d2l_api.courses.files.via_url", + course_id=self._request.lti_params["context_id"], + _query={"document_url": document_url}, + ), + } + elif document_url.startswith("vitalsource://"): svc: VitalSourceService = self._request.find_service(VitalSourceService) diff --git a/lms/routes.py b/lms/routes.py index 33a7457240..2651ddc4b4 100644 --- a/lms/routes.py +++ b/lms/routes.py @@ -53,6 +53,9 @@ def includeme(config): # pylint:disable=too-many-statements "d2l_api.courses.group_sets.list", "/api/d2l/courses/{course_id}/group_sets" ) config.add_route("d2l_api.courses.files.list", "/api/d2l/courses/{course_id}/files") + config.add_route( + "d2l_api.courses.files.via_url", "/api/d2l/courses/{course_id}/via_url" + ) config.add_route( "blackboard_api.oauth.authorize", diff --git a/lms/views/api/d2l/files.py b/lms/views/api/d2l/files.py index d14eb14a9f..7b16529f23 100644 --- a/lms/views/api/d2l/files.py +++ b/lms/views/api/d2l/files.py @@ -1,9 +1,27 @@ +import re from pyramid.view import view_config +from lms.views import helpers from lms.security import Permissions from lms.services.d2l_api import D2LAPIClient +DOCUMENT_URL_REGEX = re.compile( + r"d2l:\/\/file\/course\/(?P[^\/]*)\/file_id\/(?P[^\/]*)\/" +) + + +def _find_files(modules): + """Recursively find files in modules.""" + for module in modules: + for topic in module.get("topics", []): + if topic.get("type") == "File": + yield topic + + # Find submodules + yield from _find_files(module.get("modules", [])) + + @view_config( request_method="GET", route_name="d2l_api.courses.files.list", @@ -12,6 +30,30 @@ ) def list_files(_context, request): """Return the list of files in the given course.""" - return request.find_service(D2LAPIClient).list_files( - org_unit=request.matchdict["course_id"] - ) + course_id = request.matchdict["course_id"] + results = request.find_service(D2LAPIClient).list_files(course_id) + + for result in results: + result["id"] = f"d2l://file/course/{course_id}/file_id/{result['id']}/" + + return results + + +@view_config( + request_method="GET", + route_name="d2l_api.courses.files.via_url", + renderer="json", + permission=Permissions.API, +) +def via_url(_context, request): + course_id = request.matchdict["course_id"] + document_url = request.params["document_url"] + file_id = DOCUMENT_URL_REGEX.search(document_url)["file_id"] + + public_url = request.find_service(D2LAPIClient).public_url(course_id, file_id) + + access_token = request.find_service(name="oauth2_token").get().access_token + headers = {"Authorization": f"Bearer {access_token}"} + + via_url = helpers.via_url(request, public_url, content_type="pdf", headers=headers) + return {"via_url": via_url} From 0df63bff9a0a0d2e137c3ea474319e116d8ebfc6 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Tue, 20 Dec 2022 12:34:23 +0100 Subject: [PATCH 3/4] Public URL endpoint for D2L files --- lms/services/d2l_api/client.py | 34 ++++++++++++++-- lms/views/api/d2l/files.py | 19 +-------- .../lms/resources/_js_config/__init___test.py | 40 +++++++++++-------- .../unit/lms/services/d2l_api/client_test.py | 20 +++++++++- tests/unit/lms/views/api/d2l/files_test.py | 27 ++++++++++++- 5 files changed, 99 insertions(+), 41 deletions(-) diff --git a/lms/services/d2l_api/client.py b/lms/services/d2l_api/client.py index bc6aac530b..7a302cfcf4 100644 --- a/lms/services/d2l_api/client.py +++ b/lms/services/d2l_api/client.py @@ -117,7 +117,33 @@ def list_files(self, org_unit) -> List[dict]: """Get a nested list of files and folders for the given `org_unit`.""" modules = self._get_course_modules(org_unit) - return list(self._find_files(modules)) + return list(self._find_files(org_unit, modules)) + + def public_url(self, org_unit, file_id) -> str: + """ + Return the URL to download the given file. + + As opposed to other LMS's that return a one time signed URL that we can then pass along to Via + D2L requires us to send the API authentication header to get the file contents. + + To make sure the API authentication header is not expired we'll make a API + request here so if it needs to be refreshed we follow the standard token refresh procedure. + + https://docs.valence.desire2learn.com/res/content.html#get--d2l-api-le-(version)-(orgUnitId)-content-topics-(topicId) + https://docs.valence.desire2learn.com/res/content.html#get--d2l-api-le-(version)-(orgUnitId)-content-topics-(topicId)-file + """ + + # We don't need the data from this call. + # We are only interested on the potential side effect of needing + # a new access token and/or refreshing an existing one. + _ = self._api.request( + "GET", + self._api.api_url(f"/{org_unit}/content/topics/{file_id}", product="le"), + ) + + return self._api.api_url( + f"/{org_unit}/content/topics/{file_id}/file?stream=1", product="le" + ) @staticmethod def get_api_user_id(user_id: str): @@ -145,21 +171,21 @@ def _get_course_modules(self, org_unit) -> List[dict]: ) return D2LTableOfContentsSchema(response).parse().get("modules", []) - def _find_files(self, modules): + def _find_files(self, course_id, modules): """Recursively find files in modules.""" for module in modules: module_files = [ { "type": "File", "display_name": topic["display_name"], - "id": topic["id"], + "id": f"d2l://file/course/{course_id}/file_id/{topic['id']}/", "updated_at": topic["updated_at"], } for topic in module.get("topics", []) if topic.get("type") == "File" ] - module_children = self._find_files(module.get("modules", [])) + module_children = self._find_files(course_id, module.get("modules", [])) yield { "type": "Folder", "display_name": module["display_name"], diff --git a/lms/views/api/d2l/files.py b/lms/views/api/d2l/files.py index 7b16529f23..ec3bb1e5e5 100644 --- a/lms/views/api/d2l/files.py +++ b/lms/views/api/d2l/files.py @@ -11,17 +11,6 @@ ) -def _find_files(modules): - """Recursively find files in modules.""" - for module in modules: - for topic in module.get("topics", []): - if topic.get("type") == "File": - yield topic - - # Find submodules - yield from _find_files(module.get("modules", [])) - - @view_config( request_method="GET", route_name="d2l_api.courses.files.list", @@ -30,13 +19,7 @@ def _find_files(modules): ) def list_files(_context, request): """Return the list of files in the given course.""" - course_id = request.matchdict["course_id"] - results = request.find_service(D2LAPIClient).list_files(course_id) - - for result in results: - result["id"] = f"d2l://file/course/{course_id}/file_id/{result['id']}/" - - return results + return request.find_service(D2LAPIClient).list_files(request.matchdict["course_id"]) @view_config( diff --git a/tests/unit/lms/resources/_js_config/__init___test.py b/tests/unit/lms/resources/_js_config/__init___test.py index 857466e5a7..45e1974b3f 100644 --- a/tests/unit/lms/resources/_js_config/__init___test.py +++ b/tests/unit/lms/resources/_js_config/__init___test.py @@ -166,6 +166,30 @@ def test_it_adds_the_viaUrl_api_config_for_Blackboard_documents(self, js_config) "path": "/api/blackboard/courses/test_course_id/via_url?document_url=blackboard%3A%2F%2Fcontent-resource%2Fxyz123", } + def test_it_adds_the_viaUrl_api_config_for_Canvas_documents( + self, js_config, pyramid_request + ): + course_id, file_id = "125", "100" + pyramid_request.params["custom_canvas_course_id"] = course_id + pyramid_request.params["file_id"] = file_id + + js_config.add_document_url( + f"canvas://file/course/{course_id}/file_id/{file_id}" + ) + + assert js_config.asdict()["api"]["viaUrl"] == { + "authUrl": "http://example.com/api/canvas/oauth/authorize", + "path": "/api/canvas/assignments/TEST_RESOURCE_LINK_ID/via_url", + } + + def test_it_adds_the_viaUrl_api_config_for_D2L_documents(self, js_config): + js_config.add_document_url("d2l://file/course/125/file_id/100") + + assert js_config.asdict()["api"]["viaUrl"] == { + "authUrl": "http://example.com/api/d2l/oauth/authorize", + "path": "/api/d2l/courses/test_course_id/via_url?document_url=d2l%3A%2F%2Ffile%2Fcourse%2F125%2Ffile_id%2F100", + } + def test_vitalsource_sets_config_with_sso( self, js_config, pyramid_request, vitalsource_service ): @@ -215,22 +239,6 @@ def test_jstor_sets_config(self, js_config, jstor_service, pyramid_request): } assert js_config.asdict()["viaUrl"] == jstor_service.via_url.return_value - def test_it_adds_the_viaUrl_api_config_for_Canvas_documents( - self, js_config, pyramid_request - ): - course_id, file_id = "125", "100" - pyramid_request.params["custom_canvas_course_id"] = course_id - pyramid_request.params["file_id"] = file_id - - js_config.add_document_url( - f"canvas://file/course/{course_id}/file_id/{file_id}" - ) - - assert js_config.asdict()["api"]["viaUrl"] == { - "authUrl": "http://example.com/api/canvas/oauth/authorize", - "path": "/api/canvas/assignments/TEST_RESOURCE_LINK_ID/via_url", - } - class TestAddCanvasSpeedgraderSettings: @pytest.mark.parametrize("group_set", (sentinel.group_set, None)) diff --git a/tests/unit/lms/services/d2l_api/client_test.py b/tests/unit/lms/services/d2l_api/client_test.py index 02e79a3371..b6a73c7759 100644 --- a/tests/unit/lms/services/d2l_api/client_test.py +++ b/tests/unit/lms/services/d2l_api/client_test.py @@ -104,7 +104,7 @@ def test_group_set_groups_with_user_id(self, svc, d2l_groups_schema, groups): "children": [ { "display_name": "TITLE 1", - "id": "FILE 1", + "id": "d2l://file/course/COURSE_ID/file_id/FILE 1/", "type": "File", "updated_at": "DATE 1", }, @@ -116,7 +116,7 @@ def test_group_set_groups_with_user_id(self, svc, d2l_groups_schema, groups): "children": [ { "display_name": "TITLE 2", - "id": "FILE 2", + "id": "d2l://file/course/COURSE_ID/file_id/FILE 2/", "type": "File", "updated_at": "DATE 2", }, @@ -148,6 +148,22 @@ def test_list_files( assert files == returned_files + def test_public_url(self, svc, basic_client): + public_url = svc.public_url("COURSE_ID", "FILE_ID") + + basic_client.api_url.assert_any_call( + "/COURSE_ID/content/topics/FILE_ID", product="le" + ) + basic_client.request.assert_called_once_with( + "GET", basic_client.api_url.return_value + ) + + basic_client.api_url.assert_called_with( + "/COURSE_ID/content/topics/FILE_ID/file?stream=1", product="le" + ) + + assert public_url == basic_client.api_url.return_value + @pytest.mark.parametrize( "user_id,api_user_id", [ diff --git a/tests/unit/lms/views/api/d2l/files_test.py b/tests/unit/lms/views/api/d2l/files_test.py index 166b0c0538..445e71dd06 100644 --- a/tests/unit/lms/views/api/d2l/files_test.py +++ b/tests/unit/lms/views/api/d2l/files_test.py @@ -1,6 +1,8 @@ from unittest.mock import sentinel -from lms.views.api.d2l.files import list_files +import pytest + +from lms.views.api.d2l.files import list_files, via_url def test_course_group_sets(pyramid_request, d2l_api_client): @@ -10,3 +12,26 @@ def test_course_group_sets(pyramid_request, d2l_api_client): d2l_api_client.list_files.assert_called_once_with("test_course_id") assert result == d2l_api_client.list_files.return_value + + +def test_via_url(helpers, pyramid_request, d2l_api_client, oauth2_token_service): + pyramid_request.matchdict = {"course_id": "COURSE_ID"} + pyramid_request.params[ + "document_url" + ] = "d2l://file/course/COURSE_ID/file_id/FILE_ID/" + + response = via_url(sentinel.context, pyramid_request) + + d2l_api_client.public_url.assert_called_once_with("COURSE_ID", "FILE_ID") + helpers.via_url.assert_called_once_with( + pyramid_request, + d2l_api_client.public_url.return_value, + content_type="pdf", + headers={"Authorization": f"Bearer {oauth2_token_service.get().access_token}"}, + ) + assert response == {"via_url": helpers.via_url.return_value} + + +@pytest.fixture +def helpers(patch): + return patch("lms.views.api.d2l.files.helpers") From 8294040cb75ccbf7fb415967226106f8b6ecee2f Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Tue, 20 Dec 2022 12:34:32 +0100 Subject: [PATCH 4/4] Pass headers along to via --- lms/views/api/d2l/files.py | 11 +++++++---- lms/views/helpers/_via.py | 4 +++- tests/unit/lms/views/api/d2l/authorize_test.py | 4 ++-- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/lms/views/api/d2l/files.py b/lms/views/api/d2l/files.py index ec3bb1e5e5..166d2ab474 100644 --- a/lms/views/api/d2l/files.py +++ b/lms/views/api/d2l/files.py @@ -1,10 +1,10 @@ import re + from pyramid.view import view_config -from lms.views import helpers from lms.security import Permissions from lms.services.d2l_api import D2LAPIClient - +from lms.views import helpers DOCUMENT_URL_REGEX = re.compile( r"d2l:\/\/file\/course\/(?P[^\/]*)\/file_id\/(?P[^\/]*)\/" @@ -38,5 +38,8 @@ def via_url(_context, request): access_token = request.find_service(name="oauth2_token").get().access_token headers = {"Authorization": f"Bearer {access_token}"} - via_url = helpers.via_url(request, public_url, content_type="pdf", headers=headers) - return {"via_url": via_url} + return { + "via_url": helpers.via_url( + request, public_url, content_type="pdf", headers=headers + ) + } diff --git a/lms/views/helpers/_via.py b/lms/views/helpers/_via.py index b799a53d1d..60ca882e7c 100644 --- a/lms/views/helpers/_via.py +++ b/lms/views/helpers/_via.py @@ -4,7 +4,7 @@ __all__ = ["via_url"] -def via_url(request, document_url, content_type=None, options=None): +def via_url(request, document_url, content_type=None, options=None, headers=None): """ Return the Via URL for annotating the given ``document_url``. @@ -17,6 +17,7 @@ def via_url(request, document_url, content_type=None, options=None): :param document_url: Document URL to present in Via :param content_type: Either "pdf" or "html" if known, None if not :param options: Any extra options for the url + :param headers: Headers for the request to `document_url` :return: A URL string """ if not options: @@ -37,4 +38,5 @@ def via_url(request, document_url, content_type=None, options=None): content_type, blocked_for="lms", options=options, + headers=headers, ) diff --git a/tests/unit/lms/views/api/d2l/authorize_test.py b/tests/unit/lms/views/api/d2l/authorize_test.py index 9af7f04609..0fa2aeabe9 100644 --- a/tests/unit/lms/views/api/d2l/authorize_test.py +++ b/tests/unit/lms/views/api/d2l/authorize_test.py @@ -20,9 +20,9 @@ class TestAuthorize: "groups,files,scopes", [ (False, False, "core:*:*"), - (False, True, "core:*:* content:toc:read"), + (False, True, "core:*:* content:toc:read content:topics:read"), (True, False, "core:*:* groups:*:*"), - (True, True, "core:*:* content:toc:read groups:*:*"), + (True, True, "core:*:* content:toc:read content:topics:read groups:*:*"), ], ) def test_it(