From 8188bbdebe4a791e24205d639aa498c72487576e Mon Sep 17 00:00:00 2001 From: Karen Rasmussen Date: Tue, 21 Jul 2026 12:05:59 -0300 Subject: [PATCH 01/12] Sync checkpoints with the document's h identity --- ...b531_add_assignment_document_uri_column.py | 23 ++ lms/models/assignment.py | 3 + lms/resources/_js_config/__init__.py | 24 +- lms/services/assignment.py | 9 + lms/services/document_uri.py | 329 ++++++++++++++++ lms/services/jstor/service.py | 24 +- lms/services/lti_h.py | 11 +- lms/services/youtube.py | 23 ++ lms/views/api/checkpoint.py | 10 +- lms/views/lti/basic_launch.py | 7 + .../lms/resources/_js_config/__init___test.py | 14 +- tests/unit/lms/services/assignment_test.py | 79 +++- tests/unit/lms/services/document_uri_test.py | 371 ++++++++++++++++++ tests/unit/lms/services/lti_h_test.py | 45 ++- tests/unit/lms/views/api/checkpoint_test.py | 21 +- tests/unit/lms/views/api/sync_test.py | 10 +- 16 files changed, 937 insertions(+), 66 deletions(-) create mode 100644 lms/migrations/versions/fa62e42cb531_add_assignment_document_uri_column.py create mode 100644 lms/services/document_uri.py create mode 100644 tests/unit/lms/services/document_uri_test.py diff --git a/lms/migrations/versions/fa62e42cb531_add_assignment_document_uri_column.py b/lms/migrations/versions/fa62e42cb531_add_assignment_document_uri_column.py new file mode 100644 index 0000000000..44a02254ce --- /dev/null +++ b/lms/migrations/versions/fa62e42cb531_add_assignment_document_uri_column.py @@ -0,0 +1,23 @@ +"""Add assignment document_uri column. + +Revision ID: fa62e42cb531 +Revises: 2a45f5cb8e25 +""" + +import sqlalchemy as sa +from alembic import op + +revision = "fa62e42cb531" +down_revision = "2a45f5cb8e25" + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.add_column("assignment", sa.Column("document_uri", sa.Unicode(), nullable=True)) + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column("assignment", "document_uri") + # ### end Alembic commands ### diff --git a/lms/models/assignment.py b/lms/models/assignment.py index 5ea276950b..2888bbb3eb 100644 --- a/lms/models/assignment.py +++ b/lms/models/assignment.py @@ -95,6 +95,9 @@ class Assignment(CreatedUpdatedMixin, Base): document_url: Mapped[str] = mapped_column(sa.Unicode, nullable=False) """The URL of the document to be annotated for this assignment.""" + document_uri: Mapped[str | None] = mapped_column(sa.Unicode, nullable=True) + """The URI that identifies this assignment's document in h.""" + extra: Mapped[MutableDict] = mapped_column( MutableDict.as_mutable(JSONB()), server_default=sa.text("'{}'::jsonb"), diff --git a/lms/resources/_js_config/__init__.py b/lms/resources/_js_config/__init__.py index 82102be179..64a299115f 100644 --- a/lms/resources/_js_config/__init__.py +++ b/lms/resources/_js_config/__init__.py @@ -3,7 +3,6 @@ from datetime import UTC, timedelta from enum import Enum, StrEnum from typing import Any -from urllib.parse import urlparse from lms.error_code import ErrorCode from lms.events import LTIEvent @@ -24,29 +23,10 @@ VitalSourceService, YouTubeService, ) +from lms.services.youtube import video_id_from_url from lms.validation.authentication import BearerTokenSchema from lms.views.helpers import via_url -# Regex to extract YouTube video ID (same URL patterns as frontend utils/youtube.ts) -_YOUTUBE_VIDEO_ID_RE = re.compile( - r"(?:youtu\.be/|v/|u/\w/|embed/|shorts/|live/|watch\?v=|&v=)([^#&?]*)", - re.IGNORECASE, -) - - -def _youtube_video_id_from_url(url: str) -> str | None: - """Return the YouTube video ID if url is a YouTube URL, else None.""" - try: - parsed = urlparse(url) - if parsed.scheme not in ("http", "https"): - return None - if parsed.netloc.lower() not in ("www.youtube.com", "youtube.com", "youtu.be"): - return None - match = _YOUTUBE_VIDEO_ID_RE.search(url) - return match.group(1) if match and match.group(1) else None - except (ValueError, AttributeError): - return None - class JSConfig: """The config for the app's JavaScript code.""" @@ -191,7 +171,7 @@ def add_document_url( # pylint: disable=too-complex,too-many-branches,useless-s else: self._config["viaUrl"] = via_url(self._request, document_url) youtube_service = self._request.find_service(iface=YouTubeService) - if youtube_service.enabled and _youtube_video_id_from_url(document_url): + if youtube_service.enabled and video_id_from_url(document_url): self._hypothesis_client["youtubeAssignment"] = True def _update_focus_config(self, updates: dict): diff --git a/lms/services/assignment.py b/lms/services/assignment.py index 7e18c8b59a..88583a3228 100644 --- a/lms/services/assignment.py +++ b/lms/services/assignment.py @@ -19,6 +19,7 @@ User, ) from lms.services.course import CourseService +from lms.services.document_uri import initial_document_uri from lms.services.upsert import bulk_upsert LOG = logging.getLogger(__name__) @@ -121,7 +122,15 @@ def update_assignment( # noqa: PLR0913 # https://github.com/instructure/canvas-lms/issues/1952 return assignment + document_url_changed = assignment.document_url != document_url assignment.document_url = document_url + if document_url_changed or not assignment.document_uri: + # The h document identity follows the document. File content gets + # None here: its PDF fingerprint is computed at launch (see + # `ensure_checkpoint_fingerprint`). + assignment.document_uri = initial_document_uri( + request, document_url, course.application_instance + ) assignment.extra["group_set_id"] = group_set_id # Metadata based on the launch diff --git a/lms/services/document_uri.py b/lms/services/document_uri.py new file mode 100644 index 0000000000..3148eef2ec --- /dev/null +++ b/lms/services/document_uri.py @@ -0,0 +1,329 @@ +""" +Maintain Assignment.document_uri: the h document identity of an assignment. + +h resolves documents by URI (``Document.find_by_uris``), so anything we tell h +about an assignment's document — e.g. a Hide & Reveal checkpoint — must use +the URI the Hypothesis client uses as the document's *identity*, not our +internal ``document_url``. That identity depends on the content type: + +* http(s) URLs: the client annotates the URL itself (Via preserves it), so + ``document_url`` is already the right URI. + +* LMS files (Canvas, Blackboard, D2L, Moodle) and JSTOR: the viewer loads the + file from a per-launch download URL (usually signed and short-lived), so no + URL is a usable identity. These are all PDFs, and the client attaches a + stable ``urn:x-pdf:`` claim computed from the file's bytes — + we compute the same fingerprint server-side and use that. + +* Canvas pages: the client annotates the canonical page URL that our page + proxy injects as ````. + +* VitalSource: the client uses a stable per-book URL + (``VitalSourceContentIntegration.uri()`` in the client). + +* Canvas Studio: Via's video player saves the annotations with the video's + canonical REST URL (``CanvasStudioService.get_canonical_video_url()``). + +* Moodle pages: the proxy's canonical link has no scheme, so annotations get + the href resolved against the page proxy's URL + +`initial_document_uri` covers the cases derivable from ``document_url`` alone +and is applied when an assignment is (re)configured; the PDF fingerprint needs +the file's bytes and is filled in lazily by `ensure_checkpoint_fingerprint`. +""" + +import hashlib +import logging +import re +from urllib.parse import quote_plus, urljoin + +from lms.models import ApplicationInstance, Assignment, Course +from lms.services.canvas import CanvasService +from lms.services.d2l_api import D2LAPIClient +from lms.services.jstor.service import JSTORService +from lms.services.moodle import MoodleAPIClient +from lms.services.vitalsource.model import VSBookLocation +from lms.services.youtube import video_id_from_url + +LOG = logging.getLogger(__name__) + +#: Content annotated at its own URL: document_url is already the h identity. +_HTTP_URL_REGEX = re.compile(r"^https?://", re.IGNORECASE) + +# Keep in sync with lms/views/api/canvas/pages.py::DOCUMENT_URL_REGEX. +_CANVAS_PAGE_REGEX = re.compile( + r"canvas:\/\/page\/course\/(?P[^\/]*)\/page_id\/(?P[^\/]*)" +) + +# The file regexes below are kept in sync with each LMS's +# views/api/*/files.py::DOCUMENT_URL_REGEX. +_CANVAS_FILE_REGEX = re.compile( + r"canvas:\/\/file\/course\/(?P[^\/]*)\/file_id\/(?P[^\/]*)" +) +_BLACKBOARD_FILE_REGEX = re.compile( + r"blackboard:\/\/content-resource\/(?P[^\/]*)\/" +) +_D2L_FILE_REGEX = re.compile( + r"d2l:\/\/file\/course\/(?P[^\/]*)\/file_id\/(?P[^\/]*)\/" +) +_MOODLE_FILE_REGEX = re.compile( + r"moodle:\/\/file\/course\/(?P[^\/]*)\/url\/(?P.*)" +) + +# Keep in sync with CanvasStudioService.media_id()'s parsing. +_CANVAS_STUDIO_REGEX = re.compile(r"canvas-studio:\/\/media\/(?P.+)") + +# Keep in sync with lms/views/api/moodle/pages.py::DOCUMENT_URL_REGEX. +_MOODLE_PAGE_REGEX = re.compile( + r"moodle:\/\/page\/course\/(?P[^\/]*)\/page_id\/(?P[^\/]*)" +) + + +def initial_document_uri( # noqa: PLR0911 + request, document_url: str, application_instance: ApplicationInstance +) -> str | None: + """ + Return the h document URI derivable from `document_url`, or None. + + None means the identity can't be derived from the URL alone: file content + needs its PDF fingerprint computed from the file's bytes (see + `ensure_checkpoint_fingerprint`), and some content types aren't supported + yet. + """ + if _HTTP_URL_REGEX.match(document_url): + settings = application_instance.settings + if (video_id := video_id_from_url(document_url)) and settings.get_setting( + settings.fields[settings.Settings.YOUTUBE_ENABLED] + ): + # Via's YouTube player canonicalizes the video URL before the + # client annotates it (canonical_video_url() in via), so e.g. a + # youtu.be/X document_url isn't what the annotations get. + return f"https://www.youtube.com/watch?v={quote_plus(video_id)}" + return document_url + + if match := _CANVAS_PAGE_REGEX.search(document_url): + # The same URL as CanvasPage.canonical_url(), which our page proxy + # injects as and the client uses as the URI. + # + # NB: the ids come from document_url, i.e. the course the assignment + # was configured in. In a copied course the canonical URL uses the new + # course's ids, so this won't match there until the assignment is + # reconfigured. + lms_host = application_instance.lms_host() + return ( + f"https://{lms_host}/courses/{match['course_id']}/pages/{match['page_id']}" + ) + + if document_url.startswith("vitalsource://"): + try: + book_id = VSBookLocation.from_document_url(document_url).book_id + except ValueError: + return None + # The same URL as the client's VitalSourceContentIntegration.uri(). + return f"https://bookshelf.vitalsource.com/reader/books/{book_id}" + + if match := _CANVAS_STUDIO_REGEX.search(document_url): + # The same URL as CanvasStudioService.get_canonical_video_url(), + # which Via's video player saves with the annotations. + domain = application_instance.settings.get("canvas_studio", "domain") + if not domain: + return None + return f"https://{domain}/api/public/v1/media/{match['media_id']}" + + if match := _MOODLE_PAGE_REGEX.search(document_url): + canonical_href = ( + f"{application_instance.lms_host()}/mod/page/view.php?id={match['page_id']}" + ) + return urljoin(request.route_url("moodle_api.pages.proxy"), canonical_href) + + # LMS files and jstor:// are PDFs: nothing derivable from the URL — their + # fingerprint is computed at launch (see ensure_checkpoint_fingerprint). + return None + + +def ensure_checkpoint_fingerprint(request, assignment: Assignment, course: Course): + """ + Fill in document_uri for a Hide & Reveal file assignment. + + File content is identified in h by its PDF fingerprint, which requires + downloading the file — so it can't be derived at configure time like the + other document_uri cases and is computed here, on launch. + + Best-effort: any failure (expired API token, unreachable file...) is + logged and swallowed — a launch must never break over this. Until the + fingerprint is stored the checkpoint sync degrades to being skipped. + """ + if assignment.document_uri: + return + + if not request.lti_user.is_instructor: + # Instructors are the only users guaranteed to have authorized us (they picked + # the file), and they always launch first (they configure via one). + return + + try: + pdf = _download_file_content(request, assignment, course) + except Exception: + LOG.exception( + "Couldn't compute the checkpoint PDF fingerprint for assignment %s", + assignment.id, + ) + return + + if pdf is not None: + assignment.document_uri = f"urn:x-pdf:{pdf_fingerprint(pdf)}" + + +def _download_file_content(request, assignment: Assignment, course: Course): # noqa: PLR0911 + """ + Download the assignment's file content, or return None. + + None means document_url isn't LMS file content. Each branch resolves the + download URL the same way the LMS's files.py::via_url view does for the + viewer, minus the course-copy repair fallbacks (we only use already-stored + mappings: this is best-effort and re-runs on every launch until it works). + """ + document_url = assignment.document_url + http = request.find_service(name="http") + + if match := _CANVAS_FILE_REGEX.search(document_url): + public_url = request.find_service(CanvasService).public_url_for_file( + assignment, + match["file_id"], + course.extra["canvas"]["custom_canvas_course_id"], + ) + return http.get(public_url).content + + if match := _BLACKBOARD_FILE_REGEX.search(document_url): + public_url = request.find_service(name="blackboard_api_client").public_url( + course.lms_id, course.get_mapped_file_id(match["file_id"]) + ) + return http.get(public_url).content + + if match := _D2L_FILE_REGEX.search(document_url): + public_url = request.find_service(D2LAPIClient).public_url( + course.lms_id, course.get_mapped_file_id(match["file_id"]) + ) + access_token = request.find_service(name="oauth2_token").get().access_token + return http.get( + public_url, headers={"Authorization": f"Bearer {access_token}"} + ).content + + if match := _MOODLE_FILE_REGEX.search(document_url): + token = request.find_service(MoodleAPIClient).token + return http.get( + course.get_mapped_file_id(match["url"]), params={"token": token} + ).content + + if document_url.startswith("jstor://"): + jstor = request.find_service(iface=JSTORService) + if not jstor.enabled: + return None + return http.get(jstor.public_url(document_url)).content + + return None + + +# The fingerprint algorithm below matches the `fingerprints` getter in the +# PDF.js build bundled in Via (via/static/vendor/pdfjs-2/build/pdf.worker.js), +# which is what the client reads to build its urn:x-pdf: claims. + +_FINGERPRINT_FIRST_BYTES = 1024 +_EMPTY_ID = b"\x00" * 16 + +#: A PDF trailer /ID array: two strings, each either hex (<...>) or literal +#: ((...) with backslash escapes). We only need the first (the "original" ID). +_PDF_ID_REGEX = re.compile( + rb"/ID\s*\[\s*(?:<(?P[0-9A-Fa-f\s]*)>|\((?P(?:\\.|[^\\)])*)\))" +) + + +def pdf_fingerprint(pdf: bytes) -> str: + """ + Return PDF.js's fingerprint for `pdf`. + + This is the value the client puts in annotations' urn:x-pdf: + document claims: the hex of the PDF trailer's original /ID when present + (and not empty/zeroed), else the MD5 of the first 1024 bytes. + """ + if original_id := _pdf_original_id(pdf): + return original_id.hex() + return hashlib.md5(pdf[:_FINGERPRINT_FIRST_BYTES]).hexdigest() # noqa: S324 + + +def _pdf_original_id(pdf: bytes) -> bytes | None: + """Return the original (first) /ID string of `pdf`'s latest trailer.""" + matches = list(_PDF_ID_REGEX.finditer(pdf)) + if not matches: + return None + + # Incremental updates append a new trailer at the end of the file, and + # PDF.js reads the latest one — so take the last /ID in the file. (The + # original ID is required by spec to be the same in every trailer anyway.) + match = matches[-1] + + if (hex_id := match["hex"]) is not None: + # Whitespace is allowed anywhere inside a PDF hex string. + hex_str = re.sub(r"\s", "", hex_id.decode("ascii")) + if len(hex_str) % 2: + # Per PDF spec, a hex string with an odd number of digits gets a + # trailing zero appended. + hex_str += "0" + try: + original_id = bytes.fromhex(hex_str) + except ValueError: + return None + else: + original_id = _decode_pdf_literal(match["literal"]) + + # Same validation as PDF.js: a missing, empty or all-zeroes ID falls back + # to the MD5 fingerprint. + if original_id and original_id != _EMPTY_ID: + return original_id + return None + + +_LITERAL_ESCAPES = { + ord("n"): b"\n", + ord("r"): b"\r", + ord("t"): b"\t", + ord("b"): b"\b", + ord("f"): b"\f", + ord("("): b"(", + ord(")"): b")", + ord("\\"): b"\\", +} + + +def _decode_pdf_literal(data: bytes) -> bytes: + """Decode a PDF literal string's backslash escapes.""" + out = bytearray() + i = 0 + while i < len(data): + byte = data[i] + if byte != ord("\\"): + out.append(byte) + i += 1 + continue + + i += 1 + if i >= len(data): + break + escaped = data[i] + + if escaped in _LITERAL_ESCAPES: + out += _LITERAL_ESCAPES[escaped] + i += 1 + elif ord("0") <= escaped <= ord("7"): + # Octal escape: up to three octal digits. + digits = bytearray() + while len(digits) < 3 and i < len(data) and ord("0") <= data[i] <= ord("7"): + digits.append(data[i]) + i += 1 + out.append(int(digits, 8) & 0xFF) + else: + # An unknown escape means the backslash is ignored. + out.append(escaped) + i += 1 + + return bytes(out) diff --git a/lms/services/jstor/service.py b/lms/services/jstor/service.py index b782a98e67..68893dda6c 100644 --- a/lms/services/jstor/service.py +++ b/lms/services/jstor/service.py @@ -46,18 +46,14 @@ def enabled(self) -> bool: return bool(self._enabled and self._api_url and self._site_code) - def via_url(self, request, document_url): + def public_url(self, document_url) -> str: """ - Get a VIA url for a document. + Get a signed S3 URL for the PDF of a jstor:// document. - :param request: Pyramid request - :param document_url: The URL to annotate - :return: A URL for Via configured to launch the requested document + :param document_url: The jstor:// URL of the document :raises ExternalRequestError: If we get a value which doesn't look like a public URL from JSTOR """ - - # Get a signed S3 URL for the given JSTOR URL. s3_url = self._api_request( "/pdf/{doi}", doi=document_url.replace("jstor://", "") ).text @@ -67,9 +63,21 @@ def via_url(self, request, document_url): f"Expected to get an S3 URL but got: '{s3_url}' instead" # noqa: EM102 ) + return s3_url + + def via_url(self, request, document_url): + """ + Get a VIA url for a document. + + :param request: Pyramid request + :param document_url: The URL to annotate + :return: A URL for Via configured to launch the requested document + :raises ExternalRequestError: If we get a value which doesn't look like + a public URL from JSTOR + """ return via_url( request, - document_url=s3_url, + document_url=self.public_url(document_url), content_type="pdf", # Show content partner banner in client for JSTOR. options={"via.client.contentPartner": "jstor"}, diff --git a/lms/services/lti_h.py b/lms/services/lti_h.py index 1e3d56aad8..e17ac25e0d 100644 --- a/lms/services/lti_h.py +++ b/lms/services/lti_h.py @@ -7,20 +7,21 @@ def checkpoint_sync_data(assignment: Assignment | None, lti_user) -> dict | None: """Build the checkpoint payload to sync to h for a Hide & Reveal assignment. - Returns None when the assignment is missing or doesn't have checkpoint - enabled, so callers can pass the result straight through to - `LTIHService.sync(..., checkpoint_data=...)`. + Returns None when the assignment is missing, doesn't have checkpoint + enabled, or its h document identity isn't known yet + (assignment.document_uri is None), so callers can pass the result straight + through to `LTIHService.sync(..., checkpoint_data=...)`. reveal_date is not sent — h is the source of truth for the reveal state. h's upsert uses coalesce to preserve an existing reveal_date when NULL is sent. """ - if not (assignment and assignment.checkpoint_enabled): + if not (assignment and assignment.checkpoint_enabled and assignment.document_uri): return None role = "instructor" if lti_user.is_instructor else "student" return { - "document_uri": assignment.document_url, + "document_uri": assignment.document_uri, "user": { "username": lti_user.h_user.username, "role": role, diff --git a/lms/services/youtube.py b/lms/services/youtube.py index 93ef3aa679..bc02eb1dd9 100644 --- a/lms/services/youtube.py +++ b/lms/services/youtube.py @@ -1,9 +1,32 @@ +import re +from urllib.parse import urlparse + from lms.services.exceptions import SerializableError from lms.services.http import HTTPService YOUTUBE_API_URL = "https://www.googleapis.com/youtube/v3" """YouTube's API base URL""" +# Regex to extract YouTube video ID (same URL patterns as frontend utils/youtube.ts) +_YOUTUBE_VIDEO_ID_RE = re.compile( + r"(?:youtu\.be/|v/|u/\w/|embed/|shorts/|live/|watch\?v=|&v=)([^#&?]*)", + re.IGNORECASE, +) + + +def video_id_from_url(url: str) -> str | None: + """Return the YouTube video ID if url is a YouTube URL, else None.""" + try: + parsed = urlparse(url) + if parsed.scheme not in ("http", "https"): + return None + if parsed.netloc.lower() not in ("www.youtube.com", "youtube.com", "youtu.be"): + return None + match = _YOUTUBE_VIDEO_ID_RE.search(url) + return match.group(1) if match and match.group(1) else None + except (ValueError, AttributeError): + return None + class VideoNotFound(SerializableError): # noqa: N818 def __init__(self, video_id): diff --git a/lms/views/api/checkpoint.py b/lms/views/api/checkpoint.py index 30ed79a668..109313b748 100644 --- a/lms/views/api/checkpoint.py +++ b/lms/views/api/checkpoint.py @@ -41,6 +41,14 @@ def reveal_checkpoint(request): message = "Assignment or checkpoint not found" raise HTTPNotFound(message) + # The checkpoint in h is keyed by the document's identity there + # (assignment.document_uri), not by our internal document_url. If we + # haven't resolved one, no checkpoint can have been synced, so there's + # nothing to reveal. + if not assignment.document_uri: + message = "Assignment or checkpoint not found" + raise HTTPNotFound(message) + # Reveal directly in h — h is the source of truth for reveal state. h_api = request.find_service(HAPI) # If the assignment has section/group groupings, only reveal those — @@ -56,7 +64,7 @@ def reveal_checkpoint(request): checkpoints = [ { "group_authority_provided_id": grouping.authority_provided_id, - "document_uri": assignment.document_url, + "document_uri": assignment.document_uri, } for grouping in reveal_groupings ] diff --git a/lms/views/lti/basic_launch.py b/lms/views/lti/basic_launch.py index 21432fd7b4..866195dab6 100644 --- a/lms/views/lti/basic_launch.py +++ b/lms/views/lti/basic_launch.py @@ -22,6 +22,7 @@ from lms.security import Permissions from lms.services import LTIGradingService, UserService, VitalSourceService from lms.services.assignment import AssignmentService # noqa: TC001 +from lms.services.document_uri import ensure_checkpoint_fingerprint from lms.services.lti_h import checkpoint_sync_data from lms.validation import BasicLTILaunchSchema, ConfigureAssignmentSchema @@ -197,6 +198,12 @@ def _show_document(self, assignment): # noqa: C901, PLR0912 ): assignment.extra["ext_lti_assignment_id"] = ext_lti_assignment_id + # For file-based Hide & Reveal assignments the h document identity is + # the file's PDF fingerprint: make sure it's computed and stored + # before we build the checkpoint sync data below. + if assignment.checkpoint_enabled: + ensure_checkpoint_fingerprint(self.request, assignment, self.course) + # Determine the grouping type to decide whether to sync checkpoint # data for the course group. When the assignment uses sections/groups, # the checkpoint sync happens in the client-side sync (POST /api/sync) diff --git a/tests/unit/lms/resources/_js_config/__init___test.py b/tests/unit/lms/resources/_js_config/__init___test.py index 3b4a8ec41f..b28b3eb736 100644 --- a/tests/unit/lms/resources/_js_config/__init___test.py +++ b/tests/unit/lms/resources/_js_config/__init___test.py @@ -7,9 +7,10 @@ from lms.models import Grouping, LTIParams from lms.product.product import Routes from lms.resources import LTILaunchResource, OAuth2RedirectResource -from lms.resources._js_config import JSConfig, _youtube_video_id_from_url +from lms.resources._js_config import JSConfig from lms.security import Identity, Permissions from lms.services import HAPIError +from lms.services.youtube import video_id_from_url from lms.views.api.sync import APISyncSchema from tests import factories from tests.conftest import TEST_SETTINGS @@ -559,16 +560,13 @@ def test_non_youtube_url_does_not_set_client_flag( def test_youtube_video_id_from_url_returns_none_on_parse_error(self): """Cover the except (ValueError, AttributeError) branch.""" - with patch("lms.resources._js_config.urlparse", side_effect=ValueError): - assert ( - _youtube_video_id_from_url("https://www.youtube.com/watch?v=abc") - is None - ) + with patch("lms.services.youtube.urlparse", side_effect=ValueError): + assert video_id_from_url("https://www.youtube.com/watch?v=abc") is None def test_youtube_video_id_from_url_is_case_insensitive_for_host(self): """Host is normalized so YouTube.com / YOUTUBE.COM work like the frontend.""" - assert _youtube_video_id_from_url("https://YouTube.com/watch?v=xyz") == "xyz" - assert _youtube_video_id_from_url("https://YOUTU.BE/xyz") == "xyz" + assert video_id_from_url("https://YouTube.com/watch?v=xyz") == "xyz" + assert video_id_from_url("https://YOUTU.BE/xyz") == "xyz" class TestAddCanvasSpeedgraderSettings: diff --git a/tests/unit/lms/services/assignment_test.py b/tests/unit/lms/services/assignment_test.py index 49a03cd955..311599587a 100644 --- a/tests/unit/lms/services/assignment_test.py +++ b/tests/unit/lms/services/assignment_test.py @@ -136,24 +136,73 @@ def test_update_assignment( assignment = svc.update_assignment( pyramid_request, factories.Assignment(), - sentinel.document_url, + "https://example.com/document", sentinel.group_set_id, course, ) if is_speed_grader: assert assignment.extra == {} - assert assignment.document_url != sentinel.document_url + assert assignment.document_url != "https://example.com/document" assert not assignment.lis_outcome_service_url assert not assignment.lti_v13_resource_link_id else: - assert assignment.document_url == sentinel.document_url + assert assignment.document_url == "https://example.com/document" assert assignment.extra["group_set_id"] == sentinel.group_set_id assert assignment.title == title assert assignment.course_id == course.id assert assignment.lis_outcome_service_url == "GRADING URL" assert assignment.lti_v13_resource_link_id == v13_resource_link_id + def test_update_assignment_sets_document_uri(self, svc, pyramid_request, course): + assignment = svc.update_assignment( + pyramid_request, + factories.Assignment(), + "https://example.com/document", + sentinel.group_set_id, + course, + ) + + assert assignment.document_uri == "https://example.com/document" + + def test_update_assignment_resets_document_uri_when_the_document_changes( + self, svc, pyramid_request, course + ): + assignment = factories.Assignment( + document_url="canvas://file/course/1/file_id/2", + document_uri="urn:x-pdf:FINGERPRINT", + ) + + assignment = svc.update_assignment( + pyramid_request, + assignment, + "canvas://file/course/1/file_id/3", + sentinel.group_set_id, + course, + ) + + # The new file's fingerprint isn't known yet: it gets computed on + # launch (see ensure_checkpoint_fingerprint). + assert assignment.document_uri is None + + def test_update_assignment_keeps_document_uri_when_the_document_is_unchanged( + self, svc, pyramid_request, course + ): + assignment = factories.Assignment( + document_url="canvas://file/course/1/file_id/2", + document_uri="urn:x-pdf:FINGERPRINT", + ) + + assignment = svc.update_assignment( + pyramid_request, + assignment, + "canvas://file/course/1/file_id/2", + sentinel.group_set_id, + course, + ) + + assert assignment.document_uri == "urn:x-pdf:FINGERPRINT" + @pytest.mark.parametrize("with_existing", [True, False]) def test_update_assignment_with_auto_grading_config( self, svc, pyramid_request, course, with_existing @@ -171,7 +220,7 @@ def test_update_assignment_with_auto_grading_config( assignment = svc.update_assignment( pyramid_request, assignment, - sentinel.document_url, + "https://example.com/document", sentinel.group_set_id, course, auto_grading_config={ @@ -219,7 +268,7 @@ def test_update_assignment_with_checkpoint(self, svc, pyramid_request, course): assignment = svc.update_assignment( pyramid_request, assignment, - sentinel.document_url, + "https://example.com/document", sentinel.group_set_id, course, checkpoint_enabled=True, @@ -233,7 +282,7 @@ def test_update_assignment_without_checkpoint(self, svc, pyramid_request, course assignment = svc.update_assignment( pyramid_request, assignment, - sentinel.document_url, + "https://example.com/document", sentinel.group_set_id, course, checkpoint_enabled=False, @@ -249,7 +298,7 @@ def test_update_assignment_keeps_existing_checkpoint( assignment = svc.update_assignment( pyramid_request, assignment, - sentinel.document_url, + "https://example.com/document", sentinel.group_set_id, course, checkpoint_enabled=True, @@ -275,7 +324,7 @@ def test_update_assignment_with_due_date( assignment = svc.update_assignment( pyramid_request, factories.Assignment(), - sentinel.document_url, + "https://example.com/document", sentinel.group_set_id, course, due_date=due_date, @@ -328,7 +377,7 @@ def test_get_assignment_for_launch_existing( course, ): misc_plugin.get_assignment_configuration.return_value = { - "document_url": sentinel.document_url, + "document_url": "https://example.com/document", "group_set_id": sentinel.group_set_id, } get_assignment.return_value = factories.Assignment() @@ -342,7 +391,7 @@ def test_get_assignment_for_launch_existing( misc_plugin.is_assignment_gradable.assert_called_once_with( pyramid_request.lti_params ) - assert assignment.document_url == sentinel.document_url + assert assignment.document_url == "https://example.com/document" assert assignment.extra["group_set_id"] == sentinel.group_set_id assert assignment.title == pyramid_request.lti_params.get("resource_link_title") @@ -362,7 +411,7 @@ def test_get_assignment_for_launch_sets_due_date( course, ): misc_plugin.get_assignment_configuration.return_value = { - "document_url": sentinel.document_url, + "document_url": "https://example.com/document", "group_set_id": sentinel.group_set_id, "due_date": "2026-07-01T12:00:00+00:00", } @@ -392,7 +441,7 @@ def test_get_assignment_creates_assignment( course, ): misc_plugin.get_assignment_configuration.return_value = { - "document_url": sentinel.document_url, + "document_url": "https://example.com/document", "group_set_id": group_set_id, } create_assignment.return_value = factories.Assignment() @@ -405,7 +454,7 @@ def test_get_assignment_creates_assignment( create_assignment.assert_called_once_with( "TEST_TOOL_CONSUMER_INSTANCE_GUID", "TEST_RESOURCE_LINK_ID" ) - assert assignment.document_url == sentinel.document_url + assert assignment.document_url == "https://example.com/document" assert assignment.course_id == course.id if group_set_id: assignment.extra["group_set_id"] = group_set_id @@ -422,7 +471,7 @@ def test_get_assignment_created_assignments_point_to_copy( course, ): misc_plugin.get_assignment_configuration.return_value = { - "document_url": sentinel.document_url + "document_url": "https://example.com/document" } get_assignment.return_value = None _get_copied_from_assignment.return_value = sentinel.original_assignment @@ -434,7 +483,7 @@ def test_get_assignment_created_assignments_point_to_copy( "TEST_TOOL_CONSUMER_INSTANCE_GUID", "TEST_RESOURCE_LINK_ID" ) assert assignment.copied_from == sentinel.original_assignment - assert assignment.document_url == sentinel.document_url + assert assignment.document_url == "https://example.com/document" @pytest.mark.parametrize("with_lti11_grading_id", [True, False]) def test_upsert_assignment_membership( diff --git a/tests/unit/lms/services/document_uri_test.py b/tests/unit/lms/services/document_uri_test.py new file mode 100644 index 0000000000..68ee82c642 --- /dev/null +++ b/tests/unit/lms/services/document_uri_test.py @@ -0,0 +1,371 @@ +import hashlib + +import pytest + +from lms.services.document_uri import ( + ensure_checkpoint_fingerprint, + initial_document_uri, + pdf_fingerprint, +) +from tests import factories + + +class TestInitialDocumentURI: + @pytest.mark.parametrize( + "document_url", + [ + "https://example.com/article", + "http://example.com/doc.pdf", + "HTTPS://EXAMPLE.COM/ARTICLE", + ], + ) + def test_http_urls_are_returned_as_is( + self, pyramid_request, application_instance, document_url + ): + assert ( + initial_document_uri(pyramid_request, document_url, application_instance) + == document_url + ) + + @pytest.mark.parametrize( + "document_url", + [ + "https://youtu.be/VIDEO_ID", + "https://www.youtube.com/watch?v=VIDEO_ID&t=30s", + "https://www.youtube.com/shorts/VIDEO_ID", + ], + ) + def test_youtube_urls_return_the_canonical_video_url( + self, pyramid_request, application_instance, document_url + ): + # Via's YouTube player canonicalizes the URL before the client + # annotates it, whatever form the instructor pasted. + document_uri = initial_document_uri( + pyramid_request, document_url, application_instance + ) + + assert document_uri == "https://www.youtube.com/watch?v=VIDEO_ID" + + def test_youtube_urls_are_returned_as_is_when_youtube_is_disabled( + self, pyramid_request, application_instance + ): + application_instance.settings.set("youtube", "enabled", False) # noqa: FBT003 + + document_uri = initial_document_uri( + pyramid_request, "https://youtu.be/VIDEO_ID", application_instance + ) + + assert document_uri == "https://youtu.be/VIDEO_ID" + + def test_canvas_pages_return_the_canonical_url( + self, pyramid_request, application_instance + ): + document_uri = initial_document_uri( + pyramid_request, "canvas://page/course/42/page_id/314", application_instance + ) + + assert document_uri == "https://uni.instructure.com/courses/42/pages/314" + + def test_vitalsource_returns_the_bookshelf_url( + self, pyramid_request, application_instance + ): + document_uri = initial_document_uri( + pyramid_request, + "vitalsource://book/bookID/BOOK-ID/cfi//6/8", + application_instance, + ) + + assert document_uri == "https://bookshelf.vitalsource.com/reader/books/BOOK-ID" + + def test_invalid_vitalsource_urls_return_None( + self, pyramid_request, application_instance + ): + assert ( + initial_document_uri( + pyramid_request, "vitalsource://nonsense", application_instance + ) + is None + ) + + def test_canvas_studio_returns_the_canonical_video_url( + self, pyramid_request, application_instance + ): + application_instance.settings.set( + "canvas_studio", "domain", "uni.instructuremedia.com" + ) + + document_uri = initial_document_uri( + pyramid_request, "canvas-studio://media/55", application_instance + ) + + assert document_uri == "https://uni.instructuremedia.com/api/public/v1/media/55" + + def test_canvas_studio_without_a_domain_returns_None( + self, pyramid_request, application_instance + ): + assert ( + initial_document_uri( + pyramid_request, "canvas-studio://media/55", application_instance + ) + is None + ) + + def test_moodle_pages_return_the_proxy_resolved_url( + self, pyramid_request, application_instance + ): + document_uri = initial_document_uri( + pyramid_request, "moodle://page/course/42/page_id/860", application_instance + ) + + proxy_url = pyramid_request.route_url("moodle_api.pages.proxy") + assert document_uri == proxy_url.replace( + "/proxy", "/uni.instructure.com/mod/page/view.php?id=860" + ) + + @pytest.mark.parametrize( + "document_url", + [ + # PDF content: the identity is the fingerprint, which can't be + # derived from the URL (see ensure_checkpoint_fingerprint). + "canvas://file/course/42/file_id/99", + "blackboard://content-resource/FILE_ID/", + "d2l://file/course/42/file_id/99/", + "moodle://file/course/42/url/https%3A%2F%2Fmoodle.com%2Ffile.pdf", + "jstor://10.2307/1234", + ], + ) + def test_urls_with_no_derivable_identity_return_None( + self, pyramid_request, application_instance, document_url + ): + assert ( + initial_document_uri(pyramid_request, document_url, application_instance) + is None + ) + + @pytest.fixture + def application_instance(self): + return factories.ApplicationInstance(lms_url="https://uni.instructure.com") + + +@pytest.mark.usefixtures("canvas_service", "http_service") +class TestEnsureCheckpointFingerprint: + @pytest.mark.usefixtures("user_is_instructor") + def test_it_computes_and_stores_the_fingerprint( + self, pyramid_request, assignment, course, canvas_service, http_service + ): + http_service.get.return_value.content = PDF_WITH_HEX_ID + + ensure_checkpoint_fingerprint(pyramid_request, assignment, course) + + canvas_service.public_url_for_file.assert_called_once_with( + assignment, "99", "CANVAS_COURSE_ID" + ) + http_service.get.assert_called_once_with( + canvas_service.public_url_for_file.return_value + ) + assert ( + assignment.document_uri == f"urn:x-pdf:{pdf_fingerprint(PDF_WITH_HEX_ID)}" + ) + + @pytest.mark.usefixtures("user_is_instructor") + def test_it_does_nothing_if_document_uri_is_already_set( + self, pyramid_request, assignment, course, canvas_service + ): + assignment.document_uri = "urn:x-pdf:already-there" + + ensure_checkpoint_fingerprint(pyramid_request, assignment, course) + + canvas_service.public_url_for_file.assert_not_called() + assert assignment.document_uri == "urn:x-pdf:already-there" + + @pytest.mark.usefixtures("user_is_learner") + def test_it_does_nothing_for_non_instructors( + self, pyramid_request, assignment, course, canvas_service + ): + ensure_checkpoint_fingerprint(pyramid_request, assignment, course) + + canvas_service.public_url_for_file.assert_not_called() + assert assignment.document_uri is None + + @pytest.mark.usefixtures("user_is_instructor") + def test_it_computes_the_fingerprint_for_blackboard_files( + self, pyramid_request, assignment, course, blackboard_api_client, http_service + ): + assignment.document_url = "blackboard://content-resource/FILE_ID/" + http_service.get.return_value.content = PDF_WITH_HEX_ID + + ensure_checkpoint_fingerprint(pyramid_request, assignment, course) + + blackboard_api_client.public_url.assert_called_once_with( + course.lms_id, "FILE_ID" + ) + assert ( + assignment.document_uri == f"urn:x-pdf:{pdf_fingerprint(PDF_WITH_HEX_ID)}" + ) + + @pytest.mark.usefixtures("user_is_instructor", "oauth2_token_service") + def test_it_computes_the_fingerprint_for_d2l_files( + self, + pyramid_request, + assignment, + course, + d2l_api_client, + http_service, + oauth_token, + ): + assignment.document_url = "d2l://file/course/42/file_id/99/" + http_service.get.return_value.content = PDF_WITH_HEX_ID + + ensure_checkpoint_fingerprint(pyramid_request, assignment, course) + + d2l_api_client.public_url.assert_called_once_with(course.lms_id, "99") + http_service.get.assert_called_once_with( + d2l_api_client.public_url.return_value, + headers={"Authorization": f"Bearer {oauth_token.access_token}"}, + ) + assert ( + assignment.document_uri == f"urn:x-pdf:{pdf_fingerprint(PDF_WITH_HEX_ID)}" + ) + + @pytest.mark.usefixtures("user_is_instructor") + def test_it_computes_the_fingerprint_for_moodle_files( + self, pyramid_request, assignment, course, moodle_api_client, http_service + ): + assignment.document_url = ( + "moodle://file/course/42/url/https://moodle.com/file.pdf" + ) + http_service.get.return_value.content = PDF_WITH_HEX_ID + + ensure_checkpoint_fingerprint(pyramid_request, assignment, course) + + http_service.get.assert_called_once_with( + "https://moodle.com/file.pdf", params={"token": moodle_api_client.token} + ) + assert ( + assignment.document_uri == f"urn:x-pdf:{pdf_fingerprint(PDF_WITH_HEX_ID)}" + ) + + @pytest.mark.usefixtures("user_is_instructor") + def test_it_computes_the_fingerprint_for_jstor( + self, pyramid_request, assignment, course, jstor_service, http_service + ): + assignment.document_url = "jstor://10.2307/1234" + jstor_service.enabled = True + http_service.get.return_value.content = PDF_WITH_HEX_ID + + ensure_checkpoint_fingerprint(pyramid_request, assignment, course) + + jstor_service.public_url.assert_called_once_with("jstor://10.2307/1234") + http_service.get.assert_called_once_with(jstor_service.public_url.return_value) + assert ( + assignment.document_uri == f"urn:x-pdf:{pdf_fingerprint(PDF_WITH_HEX_ID)}" + ) + + @pytest.mark.usefixtures("user_is_instructor") + def test_it_does_nothing_for_jstor_when_disabled( + self, pyramid_request, assignment, course, jstor_service, http_service + ): + assignment.document_url = "jstor://10.2307/1234" + jstor_service.enabled = False + + ensure_checkpoint_fingerprint(pyramid_request, assignment, course) + + http_service.get.assert_not_called() + assert assignment.document_uri is None + + @pytest.mark.usefixtures("user_is_instructor") + def test_it_does_nothing_for_non_file_urls( + self, pyramid_request, assignment, course, canvas_service, http_service + ): + assignment.document_url = "moodle://page/course/42/page_id/314" + + ensure_checkpoint_fingerprint(pyramid_request, assignment, course) + + canvas_service.public_url_for_file.assert_not_called() + http_service.get.assert_not_called() + assert assignment.document_uri is None + + @pytest.mark.usefixtures("user_is_instructor") + def test_it_swallows_errors( + self, pyramid_request, assignment, course, canvas_service + ): + canvas_service.public_url_for_file.side_effect = RuntimeError("API is down") + + ensure_checkpoint_fingerprint(pyramid_request, assignment, course) + + assert assignment.document_uri is None + + @pytest.fixture + def assignment(self): + return factories.Assignment( + document_url="canvas://file/course/42/file_id/99", checkpoint_enabled=True + ) + + @pytest.fixture + def course(self): + course = factories.Course() + course.extra = {"canvas": {"custom_canvas_course_id": "CANVAS_COURSE_ID"}} + return course + + +# A minimal classic-trailer PDF tail with a hex /ID. +PDF_WITH_HEX_ID = ( + b"%PDF-1.4\nsome pdf content here\n" + b"trailer\n<< /Size 10 /Root 1 0 R /ID [" + b"] >>\nstartxref\n123\n%%EOF\n" +) + + +class TestPDFFingerprint: + def test_it_uses_the_original_id_when_present(self): + assert pdf_fingerprint(PDF_WITH_HEX_ID) == "deadbeefdeadbeefdeadbeefdeadbeef" + + def test_it_uses_the_last_id_in_the_file(self): + # Incremental updates append a new trailer: the last one wins. + pdf = ( + b"%PDF-1.4\n" + b"trailer\n<< /ID [<11111111111111111111111111111111>" + b"<11111111111111111111111111111111>] >>\n%%EOF\n" + b"trailer\n<< /ID [<22222222222222222222222222222222>" + b"<33333333333333333333333333333333>] >>\n%%EOF\n" + ) + + assert pdf_fingerprint(pdf) == "22222222222222222222222222222222" + + def test_it_allows_whitespace_inside_hex_ids(self): + pdf = b"/ID [ <00> ]" + + assert pdf_fingerprint(pdf) == "deadbeefdeadbeefdeadbeefdeadbeef" + + def test_it_pads_odd_length_hex_ids(self): + # Per the PDF spec an odd number of hex digits gets a trailing 0. + pdf = b"/ID [<00>]" + + assert pdf_fingerprint(pdf) == "deadbee0" + + def test_it_decodes_literal_string_ids(self): + pdf = rb"/ID [(AB\\C\)D\101) (other)]" + + assert pdf_fingerprint(pdf) == b"AB\\C)DA".hex() + + def test_it_falls_back_to_md5_when_there_is_no_id(self): + pdf = b"%PDF-1.4\nno id in this file\n" * 100 + + assert ( + pdf_fingerprint(pdf) == hashlib.md5(pdf[:1024]).hexdigest() # noqa: S324 + ) + + def test_it_falls_back_to_md5_when_the_id_is_all_zeroes(self): + # Same validation as PDF.js: an all-NUL 16-byte ID is "empty". + pdf = b"/ID [<00000000000000000000000000000000><00>]" + + assert ( + pdf_fingerprint(pdf) == hashlib.md5(pdf[:1024]).hexdigest() # noqa: S324 + ) + + def test_it_falls_back_to_md5_when_the_id_is_empty(self): + pdf = b"/ID [<><>]" + + assert ( + pdf_fingerprint(pdf) == hashlib.md5(pdf[:1024]).hexdigest() # noqa: S324 + ) diff --git a/tests/unit/lms/services/lti_h_test.py b/tests/unit/lms/services/lti_h_test.py index 6986955bde..3b070701d2 100644 --- a/tests/unit/lms/services/lti_h_test.py +++ b/tests/unit/lms/services/lti_h_test.py @@ -5,7 +5,7 @@ from lms.models import Grouping from lms.services import HAPIError -from lms.services.lti_h import LTIHService +from lms.services.lti_h import LTIHService, checkpoint_sync_data from tests import factories @@ -100,3 +100,46 @@ def h_user(self, pyramid_request): @pytest.fixture def grouping(self): return create_autospec(Grouping, instance=True, spec_set=True) + + +class TestCheckpointSyncData: + def test_it(self, lti_user): + assignment = factories.Assignment( + checkpoint_enabled=True, document_uri="https://example.com/doc" + ) + + assert checkpoint_sync_data(assignment, lti_user) == { + "document_uri": "https://example.com/doc", + "user": { + "username": lti_user.h_user.username, + "role": "student", + }, + } + + @pytest.mark.usefixtures("user_is_instructor") + def test_it_with_instructor(self, lti_user): + assignment = factories.Assignment( + checkpoint_enabled=True, document_uri="https://example.com/doc" + ) + + assert checkpoint_sync_data(assignment, lti_user)["user"]["role"] == ( + "instructor" + ) + + def test_it_returns_None_without_an_assignment(self, lti_user): + assert checkpoint_sync_data(None, lti_user) is None + + def test_it_returns_None_when_checkpoint_is_not_enabled(self, lti_user): + assignment = factories.Assignment( + checkpoint_enabled=False, document_uri="https://example.com/doc" + ) + + assert checkpoint_sync_data(assignment, lti_user) is None + + def test_it_returns_None_when_the_document_uri_is_not_known(self, lti_user): + # E.g. a file assignment whose PDF fingerprint hasn't been computed + # yet: syncing our internal document_url instead would create an h + # document no annotation ever matches. + assignment = factories.Assignment(checkpoint_enabled=True, document_uri=None) + + assert checkpoint_sync_data(assignment, lti_user) is None diff --git a/tests/unit/lms/views/api/checkpoint_test.py b/tests/unit/lms/views/api/checkpoint_test.py index f8fa080e12..f502c3419d 100644 --- a/tests/unit/lms/views/api/checkpoint_test.py +++ b/tests/unit/lms/views/api/checkpoint_test.py @@ -173,6 +173,25 @@ def test_it_reports_not_revealed_when_h_returns_no_results( assert result == {"revealed": False, "reveal_date": None} + @pytest.mark.usefixtures("user_is_instructor") + def test_it_returns_404_when_the_document_uri_is_not_known( + self, pyramid_request, assignment_service, h_api + ): + # If we never resolved an h document identity (e.g. a file assignment + # whose PDF fingerprint hasn't been computed), no checkpoint can have + # been synced, so there's nothing to reveal. + assignment = self._assignment_with_checkpoint( + pyramid_request.lti_user.application_instance_id + ) + assignment.document_uri = None + assignment_service.get_by_id.return_value = assignment + pyramid_request.matchdict = {"assignment_id": "1"} + + with pytest.raises(HTTPNotFound): + reveal_checkpoint(pyramid_request) + + h_api.reveal_checkpoints.assert_not_called() + @pytest.mark.usefixtures("user_is_instructor") def test_it_returns_404_when_no_groupings( self, pyramid_request, assignment_service @@ -190,7 +209,7 @@ def test_it_returns_404_when_no_groupings( def _assignment_with_checkpoint(self, application_instance_id=None): assignment = MagicMock() assignment.checkpoint_enabled = True - assignment.document_url = "https://example.com/doc" + assignment.document_uri = "https://example.com/doc" if application_instance_id is not None: assignment.course.application_instance_id = application_instance_id grouping = MagicMock() diff --git a/tests/unit/lms/views/api/sync_test.py b/tests/unit/lms/views/api/sync_test.py index 1c08a58e3d..0a5e4947b4 100644 --- a/tests/unit/lms/views/api/sync_test.py +++ b/tests/unit/lms/views/api/sync_test.py @@ -196,7 +196,7 @@ def test_it_syncs_checkpoint_data_with_sections( ): assignment = assignment_service.get_assignment.return_value assignment.checkpoint_enabled = True - assignment.document_url = "https://example.com/doc" + assignment.document_uri = "https://example.com/doc" sync(pyramid_request) @@ -222,7 +222,7 @@ def test_it_syncs_checkpoint_data_with_instructor( ): assignment = assignment_service.get_assignment.return_value assignment.checkpoint_enabled = True - assignment.document_url = "https://example.com/doc" + assignment.document_uri = "https://example.com/doc" sync(pyramid_request) @@ -249,7 +249,7 @@ def test_it_syncs_checkpoint_data_with_groups( pyramid_request.parsed_params["group_set_id"] = sentinel.group_set_id assignment = assignment_service.get_assignment.return_value assignment.checkpoint_enabled = True - assignment.document_url = "https://example.com/doc" + assignment.document_uri = "https://example.com/doc" sync(pyramid_request) @@ -274,7 +274,7 @@ def test_it_returns_checkpoint_state_from_h( ): assignment = assignment_service.get_assignment.return_value assignment.checkpoint_enabled = True - assignment.document_url = "https://example.com/doc" + assignment.document_uri = "https://example.com/doc" lti_h_service.sync.return_value = [ {"revealed": True, "reveal_date": "2026-07-01T12:00:00"} ] @@ -295,7 +295,7 @@ def test_it_omits_checkpoint_state_when_h_returns_no_results( ): assignment = assignment_service.get_assignment.return_value assignment.checkpoint_enabled = True - assignment.document_url = "https://example.com/doc" + assignment.document_uri = "https://example.com/doc" lti_h_service.sync.return_value = None result = sync(pyramid_request) From 54873e189c69d449fad8e44932bdad5157078cba Mon Sep 17 00:00:00 2001 From: Elim Pizza Date: Thu, 23 Jul 2026 10:03:48 -0300 Subject: [PATCH 02/12] coverage --- lms/services/document_uri.py | 5 ++++- tests/unit/lms/services/document_uri_test.py | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/lms/services/document_uri.py b/lms/services/document_uri.py index 3148eef2ec..337834777d 100644 --- a/lms/services/document_uri.py +++ b/lms/services/document_uri.py @@ -271,7 +271,10 @@ def _pdf_original_id(pdf: bytes) -> bytes | None: hex_str += "0" try: original_id = bytes.fromhex(hex_str) - except ValueError: + except ValueError: # pragma: no cover + # Unreachable: the /ID hex group is [0-9A-Fa-f\s]* and we've + # stripped whitespace and padded to even length, so fromhex can't + # fail. Kept as a defensive guard on the low-level decode. return None else: original_id = _decode_pdf_literal(match["literal"]) diff --git a/tests/unit/lms/services/document_uri_test.py b/tests/unit/lms/services/document_uri_test.py index 68ee82c642..efdb9c071e 100644 --- a/tests/unit/lms/services/document_uri_test.py +++ b/tests/unit/lms/services/document_uri_test.py @@ -3,6 +3,7 @@ import pytest from lms.services.document_uri import ( + _decode_pdf_literal, ensure_checkpoint_fingerprint, initial_document_uri, pdf_fingerprint, @@ -369,3 +370,18 @@ def test_it_falls_back_to_md5_when_the_id_is_empty(self): assert ( pdf_fingerprint(pdf) == hashlib.md5(pdf[:1024]).hexdigest() # noqa: S324 ) + + def test_it_ignores_the_backslash_of_an_unknown_literal_escape(self): + # An unrecognized escape (not in the escape table and not octal) drops + # the backslash and keeps the following character, matching PDF.js. + pdf = rb"/ID [(A\qB) (other)]" + + assert pdf_fingerprint(pdf) == b"AqB".hex() + + +class TestDecodePDFLiteral: + def test_a_trailing_backslash_is_dropped(self): + # A lone backslash at the very end of a literal has nothing to escape, + # so it's ignored. (The /ID regex never yields this, but the decoder + # guards against it anyway.) + assert _decode_pdf_literal(b"AB\\") == b"AB" From 30ca2e788f49f7c237950d3b06a179bc40364366 Mon Sep 17 00:00:00 2001 From: Karen Rasmussen Date: Tue, 28 Jul 2026 11:39:29 -0300 Subject: [PATCH 03/12] Use the client-reported document identity for Hide & Reveal checkpoints --- lms/resources/_js_config/__init__.py | 101 +++--- lms/services/assignment.py | 12 +- lms/services/document_uri.py | 332 ------------------ lms/services/lti_h.py | 7 +- .../components/BasicLTILaunchApp.tsx | 35 ++ .../frontend_apps/services/client-rpc.ts | 25 ++ lms/views/api/checkpoint.py | 7 +- lms/views/api/sync.py | 32 +- lms/views/lti/basic_launch.py | 7 - 9 files changed, 151 insertions(+), 407 deletions(-) delete mode 100644 lms/services/document_uri.py diff --git a/lms/resources/_js_config/__init__.py b/lms/resources/_js_config/__init__.py index 64a299115f..d81d994021 100644 --- a/lms/resources/_js_config/__init__.py +++ b/lms/resources/_js_config/__init__.py @@ -506,6 +506,7 @@ def enable_toolbar_checkpoint( toolbar_config["assignmentDueDate"] = due_date_iso toolbar_config["assignmentCheckpointEnabled"] = True self._config["instructorToolbar"] = toolbar_config + self._enable_document_info_reporting() def enable_student_checkpoint(self, assignment, *, h_revealed=False): due_date_iso = ( @@ -521,6 +522,10 @@ def enable_student_checkpoint(self, assignment, *, h_revealed=False): "assignmentDueDate": due_date_iso, "assignmentCheckpointEnabled": True, } + self._enable_document_info_reporting() + + def _enable_document_info_reporting(self): + self._hypothesis_client["reportDocumentInfo"] = True def enable_toolbar_editing(self): toolbar_config = self._get_toolbar_config() @@ -761,7 +766,10 @@ def _configure_groups(self, course, assignment): self._config["hypothesisClient"]["services"][0]["groups"] = [ course.groupid(self._authority) ] - self._config["api"]["sync"] = None + if assignment and assignment.checkpoint_enabled: + self._config["api"]["sync"] = self._sync_api_config(course, assignment) + else: + self._config["api"]["sync"] = None else: # If not using the default COURSE grouping point the FE @@ -769,53 +777,56 @@ def _configure_groups(self, course, assignment): self._config["hypothesisClient"]["services"][0]["groups"] = ( "$rpc:requestGroups" ) + self._config["api"]["sync"] = self._sync_api_config(course, assignment) - req = self._request - self._config["api"]["sync"] = { - "authUrl": ( - req.route_url(req.product.route.oauth2_authorize) - if req.product.route.oauth2_authorize - else None + def _sync_api_config(self, course, assignment): # noqa: ARG002 + """Build the `api.sync` config the frontend uses to POST to /api/sync.""" + req = self._request + return { + "authUrl": ( + req.route_url(req.product.route.oauth2_authorize) + if req.product.route.oauth2_authorize + else None + ), + "path": req.route_path("api.sync"), + # This data is consumed by the view in `lms.views.api.sync` which + # defines the arguments it expects. We need to match that + # description. Anything we add here should be echoed back by the + # frontend. + "data": { + "resource_link_id": assignment.resource_link_id, + "context_id": self._request.lti_params["context_id"], + "group_set_id": self._request.product.plugin.grouping.get_group_set_id( + self._request, assignment, historical_assignment=None ), - "path": req.route_path("api.sync"), - # This data is consumed by the view in `lms.views.api.sync` which - # defines the arguments it expects. We need to match that - # description. Anything we add here should be echoed back by the - # frontend. - "data": { - "resource_link_id": assignment.resource_link_id, - "context_id": self._request.lti_params["context_id"], - "group_set_id": self._request.product.plugin.grouping.get_group_set_id( - self._request, assignment, historical_assignment=None - ), - "group_info": { - key: value - for key, value in self._request.lti_params.items() - if key - in { - # Most (all) of these are duplicated elsewhere, we'll keep updating for now - # because external analytics query rely on this table. - "context_id", - "context_title", - "context_label", - "tool_consumer_info_product_family_code", - "tool_consumer_info_version", - "tool_consumer_instance_name", - "tool_consumer_instance_description", - "tool_consumer_instance_url", - "tool_consumer_instance_contact_email", - "tool_consumer_instance_guid", - "custom_canvas_api_domain", - "custom_canvas_course_id", - } - }, - # The student we are currently grading. In the case of Canvas - # this will be present in the SpeedGrader launch URL and - # available at launch time. When using our own grading bar this - # will be passed by the frontend - "gradingStudentId": req.params.get("learner_canvas_user_id"), + "group_info": { + key: value + for key, value in self._request.lti_params.items() + if key + in { + # Most (all) of these are duplicated elsewhere, we'll keep updating for now + # because external analytics query rely on this table. + "context_id", + "context_title", + "context_label", + "tool_consumer_info_product_family_code", + "tool_consumer_info_version", + "tool_consumer_instance_name", + "tool_consumer_instance_description", + "tool_consumer_instance_url", + "tool_consumer_instance_contact_email", + "tool_consumer_instance_guid", + "custom_canvas_api_domain", + "custom_canvas_course_id", + } }, - } + # The student we are currently grading. In the case of Canvas + # this will be present in the SpeedGrader launch URL and + # available at launch time. When using our own grading bar this + # will be passed by the frontend + "gradingStudentId": req.params.get("learner_canvas_user_id"), + }, + } def _get_user_info(self) -> User: if self._request.has_permission(Permissions.STAFF): diff --git a/lms/services/assignment.py b/lms/services/assignment.py index 88583a3228..2f16c94e81 100644 --- a/lms/services/assignment.py +++ b/lms/services/assignment.py @@ -19,7 +19,6 @@ User, ) from lms.services.course import CourseService -from lms.services.document_uri import initial_document_uri from lms.services.upsert import bulk_upsert LOG = logging.getLogger(__name__) @@ -124,13 +123,10 @@ def update_assignment( # noqa: PLR0913 document_url_changed = assignment.document_url != document_url assignment.document_url = document_url - if document_url_changed or not assignment.document_uri: - # The h document identity follows the document. File content gets - # None here: its PDF fingerprint is computed at launch (see - # `ensure_checkpoint_fingerprint`). - assignment.document_uri = initial_document_uri( - request, document_url, course.application_instance - ) + if document_url_changed: + # If the document changed, any previously reported identity is stale, so + # clear it and let the client re-report it on this launch. + assignment.document_uri = None assignment.extra["group_set_id"] = group_set_id # Metadata based on the launch diff --git a/lms/services/document_uri.py b/lms/services/document_uri.py deleted file mode 100644 index 337834777d..0000000000 --- a/lms/services/document_uri.py +++ /dev/null @@ -1,332 +0,0 @@ -""" -Maintain Assignment.document_uri: the h document identity of an assignment. - -h resolves documents by URI (``Document.find_by_uris``), so anything we tell h -about an assignment's document — e.g. a Hide & Reveal checkpoint — must use -the URI the Hypothesis client uses as the document's *identity*, not our -internal ``document_url``. That identity depends on the content type: - -* http(s) URLs: the client annotates the URL itself (Via preserves it), so - ``document_url`` is already the right URI. - -* LMS files (Canvas, Blackboard, D2L, Moodle) and JSTOR: the viewer loads the - file from a per-launch download URL (usually signed and short-lived), so no - URL is a usable identity. These are all PDFs, and the client attaches a - stable ``urn:x-pdf:`` claim computed from the file's bytes — - we compute the same fingerprint server-side and use that. - -* Canvas pages: the client annotates the canonical page URL that our page - proxy injects as ````. - -* VitalSource: the client uses a stable per-book URL - (``VitalSourceContentIntegration.uri()`` in the client). - -* Canvas Studio: Via's video player saves the annotations with the video's - canonical REST URL (``CanvasStudioService.get_canonical_video_url()``). - -* Moodle pages: the proxy's canonical link has no scheme, so annotations get - the href resolved against the page proxy's URL - -`initial_document_uri` covers the cases derivable from ``document_url`` alone -and is applied when an assignment is (re)configured; the PDF fingerprint needs -the file's bytes and is filled in lazily by `ensure_checkpoint_fingerprint`. -""" - -import hashlib -import logging -import re -from urllib.parse import quote_plus, urljoin - -from lms.models import ApplicationInstance, Assignment, Course -from lms.services.canvas import CanvasService -from lms.services.d2l_api import D2LAPIClient -from lms.services.jstor.service import JSTORService -from lms.services.moodle import MoodleAPIClient -from lms.services.vitalsource.model import VSBookLocation -from lms.services.youtube import video_id_from_url - -LOG = logging.getLogger(__name__) - -#: Content annotated at its own URL: document_url is already the h identity. -_HTTP_URL_REGEX = re.compile(r"^https?://", re.IGNORECASE) - -# Keep in sync with lms/views/api/canvas/pages.py::DOCUMENT_URL_REGEX. -_CANVAS_PAGE_REGEX = re.compile( - r"canvas:\/\/page\/course\/(?P[^\/]*)\/page_id\/(?P[^\/]*)" -) - -# The file regexes below are kept in sync with each LMS's -# views/api/*/files.py::DOCUMENT_URL_REGEX. -_CANVAS_FILE_REGEX = re.compile( - r"canvas:\/\/file\/course\/(?P[^\/]*)\/file_id\/(?P[^\/]*)" -) -_BLACKBOARD_FILE_REGEX = re.compile( - r"blackboard:\/\/content-resource\/(?P[^\/]*)\/" -) -_D2L_FILE_REGEX = re.compile( - r"d2l:\/\/file\/course\/(?P[^\/]*)\/file_id\/(?P[^\/]*)\/" -) -_MOODLE_FILE_REGEX = re.compile( - r"moodle:\/\/file\/course\/(?P[^\/]*)\/url\/(?P.*)" -) - -# Keep in sync with CanvasStudioService.media_id()'s parsing. -_CANVAS_STUDIO_REGEX = re.compile(r"canvas-studio:\/\/media\/(?P.+)") - -# Keep in sync with lms/views/api/moodle/pages.py::DOCUMENT_URL_REGEX. -_MOODLE_PAGE_REGEX = re.compile( - r"moodle:\/\/page\/course\/(?P[^\/]*)\/page_id\/(?P[^\/]*)" -) - - -def initial_document_uri( # noqa: PLR0911 - request, document_url: str, application_instance: ApplicationInstance -) -> str | None: - """ - Return the h document URI derivable from `document_url`, or None. - - None means the identity can't be derived from the URL alone: file content - needs its PDF fingerprint computed from the file's bytes (see - `ensure_checkpoint_fingerprint`), and some content types aren't supported - yet. - """ - if _HTTP_URL_REGEX.match(document_url): - settings = application_instance.settings - if (video_id := video_id_from_url(document_url)) and settings.get_setting( - settings.fields[settings.Settings.YOUTUBE_ENABLED] - ): - # Via's YouTube player canonicalizes the video URL before the - # client annotates it (canonical_video_url() in via), so e.g. a - # youtu.be/X document_url isn't what the annotations get. - return f"https://www.youtube.com/watch?v={quote_plus(video_id)}" - return document_url - - if match := _CANVAS_PAGE_REGEX.search(document_url): - # The same URL as CanvasPage.canonical_url(), which our page proxy - # injects as and the client uses as the URI. - # - # NB: the ids come from document_url, i.e. the course the assignment - # was configured in. In a copied course the canonical URL uses the new - # course's ids, so this won't match there until the assignment is - # reconfigured. - lms_host = application_instance.lms_host() - return ( - f"https://{lms_host}/courses/{match['course_id']}/pages/{match['page_id']}" - ) - - if document_url.startswith("vitalsource://"): - try: - book_id = VSBookLocation.from_document_url(document_url).book_id - except ValueError: - return None - # The same URL as the client's VitalSourceContentIntegration.uri(). - return f"https://bookshelf.vitalsource.com/reader/books/{book_id}" - - if match := _CANVAS_STUDIO_REGEX.search(document_url): - # The same URL as CanvasStudioService.get_canonical_video_url(), - # which Via's video player saves with the annotations. - domain = application_instance.settings.get("canvas_studio", "domain") - if not domain: - return None - return f"https://{domain}/api/public/v1/media/{match['media_id']}" - - if match := _MOODLE_PAGE_REGEX.search(document_url): - canonical_href = ( - f"{application_instance.lms_host()}/mod/page/view.php?id={match['page_id']}" - ) - return urljoin(request.route_url("moodle_api.pages.proxy"), canonical_href) - - # LMS files and jstor:// are PDFs: nothing derivable from the URL — their - # fingerprint is computed at launch (see ensure_checkpoint_fingerprint). - return None - - -def ensure_checkpoint_fingerprint(request, assignment: Assignment, course: Course): - """ - Fill in document_uri for a Hide & Reveal file assignment. - - File content is identified in h by its PDF fingerprint, which requires - downloading the file — so it can't be derived at configure time like the - other document_uri cases and is computed here, on launch. - - Best-effort: any failure (expired API token, unreachable file...) is - logged and swallowed — a launch must never break over this. Until the - fingerprint is stored the checkpoint sync degrades to being skipped. - """ - if assignment.document_uri: - return - - if not request.lti_user.is_instructor: - # Instructors are the only users guaranteed to have authorized us (they picked - # the file), and they always launch first (they configure via one). - return - - try: - pdf = _download_file_content(request, assignment, course) - except Exception: - LOG.exception( - "Couldn't compute the checkpoint PDF fingerprint for assignment %s", - assignment.id, - ) - return - - if pdf is not None: - assignment.document_uri = f"urn:x-pdf:{pdf_fingerprint(pdf)}" - - -def _download_file_content(request, assignment: Assignment, course: Course): # noqa: PLR0911 - """ - Download the assignment's file content, or return None. - - None means document_url isn't LMS file content. Each branch resolves the - download URL the same way the LMS's files.py::via_url view does for the - viewer, minus the course-copy repair fallbacks (we only use already-stored - mappings: this is best-effort and re-runs on every launch until it works). - """ - document_url = assignment.document_url - http = request.find_service(name="http") - - if match := _CANVAS_FILE_REGEX.search(document_url): - public_url = request.find_service(CanvasService).public_url_for_file( - assignment, - match["file_id"], - course.extra["canvas"]["custom_canvas_course_id"], - ) - return http.get(public_url).content - - if match := _BLACKBOARD_FILE_REGEX.search(document_url): - public_url = request.find_service(name="blackboard_api_client").public_url( - course.lms_id, course.get_mapped_file_id(match["file_id"]) - ) - return http.get(public_url).content - - if match := _D2L_FILE_REGEX.search(document_url): - public_url = request.find_service(D2LAPIClient).public_url( - course.lms_id, course.get_mapped_file_id(match["file_id"]) - ) - access_token = request.find_service(name="oauth2_token").get().access_token - return http.get( - public_url, headers={"Authorization": f"Bearer {access_token}"} - ).content - - if match := _MOODLE_FILE_REGEX.search(document_url): - token = request.find_service(MoodleAPIClient).token - return http.get( - course.get_mapped_file_id(match["url"]), params={"token": token} - ).content - - if document_url.startswith("jstor://"): - jstor = request.find_service(iface=JSTORService) - if not jstor.enabled: - return None - return http.get(jstor.public_url(document_url)).content - - return None - - -# The fingerprint algorithm below matches the `fingerprints` getter in the -# PDF.js build bundled in Via (via/static/vendor/pdfjs-2/build/pdf.worker.js), -# which is what the client reads to build its urn:x-pdf: claims. - -_FINGERPRINT_FIRST_BYTES = 1024 -_EMPTY_ID = b"\x00" * 16 - -#: A PDF trailer /ID array: two strings, each either hex (<...>) or literal -#: ((...) with backslash escapes). We only need the first (the "original" ID). -_PDF_ID_REGEX = re.compile( - rb"/ID\s*\[\s*(?:<(?P[0-9A-Fa-f\s]*)>|\((?P(?:\\.|[^\\)])*)\))" -) - - -def pdf_fingerprint(pdf: bytes) -> str: - """ - Return PDF.js's fingerprint for `pdf`. - - This is the value the client puts in annotations' urn:x-pdf: - document claims: the hex of the PDF trailer's original /ID when present - (and not empty/zeroed), else the MD5 of the first 1024 bytes. - """ - if original_id := _pdf_original_id(pdf): - return original_id.hex() - return hashlib.md5(pdf[:_FINGERPRINT_FIRST_BYTES]).hexdigest() # noqa: S324 - - -def _pdf_original_id(pdf: bytes) -> bytes | None: - """Return the original (first) /ID string of `pdf`'s latest trailer.""" - matches = list(_PDF_ID_REGEX.finditer(pdf)) - if not matches: - return None - - # Incremental updates append a new trailer at the end of the file, and - # PDF.js reads the latest one — so take the last /ID in the file. (The - # original ID is required by spec to be the same in every trailer anyway.) - match = matches[-1] - - if (hex_id := match["hex"]) is not None: - # Whitespace is allowed anywhere inside a PDF hex string. - hex_str = re.sub(r"\s", "", hex_id.decode("ascii")) - if len(hex_str) % 2: - # Per PDF spec, a hex string with an odd number of digits gets a - # trailing zero appended. - hex_str += "0" - try: - original_id = bytes.fromhex(hex_str) - except ValueError: # pragma: no cover - # Unreachable: the /ID hex group is [0-9A-Fa-f\s]* and we've - # stripped whitespace and padded to even length, so fromhex can't - # fail. Kept as a defensive guard on the low-level decode. - return None - else: - original_id = _decode_pdf_literal(match["literal"]) - - # Same validation as PDF.js: a missing, empty or all-zeroes ID falls back - # to the MD5 fingerprint. - if original_id and original_id != _EMPTY_ID: - return original_id - return None - - -_LITERAL_ESCAPES = { - ord("n"): b"\n", - ord("r"): b"\r", - ord("t"): b"\t", - ord("b"): b"\b", - ord("f"): b"\f", - ord("("): b"(", - ord(")"): b")", - ord("\\"): b"\\", -} - - -def _decode_pdf_literal(data: bytes) -> bytes: - """Decode a PDF literal string's backslash escapes.""" - out = bytearray() - i = 0 - while i < len(data): - byte = data[i] - if byte != ord("\\"): - out.append(byte) - i += 1 - continue - - i += 1 - if i >= len(data): - break - escaped = data[i] - - if escaped in _LITERAL_ESCAPES: - out += _LITERAL_ESCAPES[escaped] - i += 1 - elif ord("0") <= escaped <= ord("7"): - # Octal escape: up to three octal digits. - digits = bytearray() - while len(digits) < 3 and i < len(data) and ord("0") <= data[i] <= ord("7"): - digits.append(data[i]) - i += 1 - out.append(int(digits, 8) & 0xFF) - else: - # An unknown escape means the backslash is ignored. - out.append(escaped) - i += 1 - - return bytes(out) diff --git a/lms/services/lti_h.py b/lms/services/lti_h.py index e17ac25e0d..9c71fbfb43 100644 --- a/lms/services/lti_h.py +++ b/lms/services/lti_h.py @@ -7,9 +7,10 @@ def checkpoint_sync_data(assignment: Assignment | None, lti_user) -> dict | None: """Build the checkpoint payload to sync to h for a Hide & Reveal assignment. - Returns None when the assignment is missing, doesn't have checkpoint - enabled, or its h document identity isn't known yet - (assignment.document_uri is None), so callers can pass the result straight + Syncs against `assignment.document_uri` — the h document identity. + Returns None when the assignment is missing, doesn't have + checkpoint enabled, or the client hasn't reported an identity yet + (`assignment.document_uri` is None), so callers can pass the result straight through to `LTIHService.sync(..., checkpoint_data=...)`. reveal_date is not sent — h is the source of truth for the reveal state. diff --git a/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx b/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx index 52f2a72d36..5d60396cd7 100644 --- a/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx +++ b/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.tsx @@ -78,10 +78,19 @@ export default function BasicLTILaunchApp() { // Content URL to show in the iframe. viaUrl: viaURL, canvas, + instructorToolbar, + studentToolbar, } = useConfig(['api', 'hypothesisClient']); const clientRPC = useService(ClientRPC); + // Whether this is a Hide & Reveal assignment. Only then do we need to wait + // for the client to report the document's h identity and sync a checkpoint. + const checkpointEnabled = !!( + instructorToolbar?.assignmentCheckpointEnabled ?? + studentToolbar?.assignmentCheckpointEnabled + ); + // Canvas only: The presence of a value for this configuration property // indicates that an empty grading submission should be made only after this // (student) user performs qualifying annotation activity. Otherwise, a @@ -245,6 +254,32 @@ export default function BasicLTILaunchApp() { fetchGroups(); }, [fetchContentURL, fetchGroups]); + /** + * For Hide & Reveal assignments, report the document's h identity to the + * backend once the Hypothesis client computes it. + */ + useEffect(() => { + if (!syncAPICallInfo || !checkpointEnabled) { + return; + } + + clientRPC.getDocumentUri().then(async documentUri => { + if (!documentUri) { + return; + } + try { + const { checkpoint } = await apiCall({ + authToken, + path: syncAPICallInfo.path, + data: { ...syncAPICallInfo.data, document_uri: documentUri }, + }); + setSyncCheckpoint(checkpoint ?? null); + } catch { + // The group sync surfaces sync failures already. + } + }); + }, [clientRPC, syncAPICallInfo, authToken, checkpointEnabled]); + /** * Report a submission to the LMS, with the LMS-provided metadata needed for * later grading of the assignment. diff --git a/lms/static/scripts/frontend_apps/services/client-rpc.ts b/lms/static/scripts/frontend_apps/services/client-rpc.ts index 1df2e820e9..04eba32f66 100644 --- a/lms/static/scripts/frontend_apps/services/client-rpc.ts +++ b/lms/static/scripts/frontend_apps/services/client-rpc.ts @@ -102,8 +102,15 @@ export type ClientRPCOptions = { * - Updating the Hypothesis client configuration in response to input * in the LMS frontend, such as changing the focused user in grading mode. */ +/** Argument for the `reportDocumentInfo` message from the client. */ +type DocumentInfo = { + uri: string; +}; + export class ClientRPC extends TinyEmitter { private _resolveGroups: (groups: string[]) => void; + private _documentUri: Promise; + private _resolveDocumentUri: (uri: string) => void; private _server: Server; /** @@ -173,6 +180,14 @@ export class ClientRPC extends TinyEmitter { // Expose current auth token via RPC this._server.register('requestAuthToken', () => authToken); + this._resolveDocumentUri = () => {}; + this._documentUri = new Promise(resolve => { + this._resolveDocumentUri = resolve; + }); + this._server.register('reportDocumentInfo', (info: DocumentInfo) => { + this._resolveDocumentUri(info.uri); + }); + this._resolveGroups = () => {}; const groups = new Promise(resolve => { this._resolveGroups = resolve; @@ -200,6 +215,16 @@ export class ClientRPC extends TinyEmitter { this._resolveGroups(groups); } + /** + * Resolve with the loaded document's h identity, once the client reports it. + * + * The client reports this asynchronously after the document loads + * — the URI is only required for Hide & Reveal assignments. + */ + getDocumentUri(): Promise { + return this._documentUri; + } + /** * Set which user is focused in the client or none if `user` is `null`. * diff --git a/lms/views/api/checkpoint.py b/lms/views/api/checkpoint.py index 109313b748..9f76ae6aeb 100644 --- a/lms/views/api/checkpoint.py +++ b/lms/views/api/checkpoint.py @@ -41,10 +41,9 @@ def reveal_checkpoint(request): message = "Assignment or checkpoint not found" raise HTTPNotFound(message) - # The checkpoint in h is keyed by the document's identity there - # (assignment.document_uri), not by our internal document_url. If we - # haven't resolved one, no checkpoint can have been synced, so there's - # nothing to reveal. + # The checkpoint in h is keyed by the document's identity there — the URI + # the client reports (`assignment.document_uri`). If the client hasn't reported + # one, no checkpoint can have been synced, so there's nothing to reveal. if not assignment.document_uri: message = "Assignment or checkpoint not found" raise HTTPNotFound(message) diff --git a/lms/views/api/sync.py b/lms/views/api/sync.py index b1cbe2a41b..a66959918c 100644 --- a/lms/views/api/sync.py +++ b/lms/views/api/sync.py @@ -1,6 +1,7 @@ from pyramid.view import view_config from webargs import fields +from lms.models import Grouping from lms.product.plugin.grouping import GroupError from lms.security import Permissions from lms.services.lti_h import checkpoint_sync_data @@ -13,6 +14,7 @@ class APISyncSchema(PyramidRequestSchema): group_set_id = fields.Str(required=False, allow_none=True) group_info = fields.Dict(required=True) gradingStudentId = fields.Str(required=False, allow_none=True) # noqa: N815 + document_uri = fields.Str(required=False, allow_none=True) @view_config( @@ -30,7 +32,28 @@ def sync(request): ) grading_student_id = request.parsed_params.get("gradingStudentId") - if group_set_id := request.parsed_params.get("group_set_id"): + assignment = assignment_service.get_assignment( + course.application_instance.tool_consumer_instance_guid, + request.parsed_params["resource_link_id"], + ) + + if ( + (reported_document_uri := request.parsed_params.get("document_uri")) + and assignment + and assignment.checkpoint_enabled + ): + assignment.document_uri = reported_document_uri + + grouping_type = grouping_service.get_launch_grouping_type( + request, course, assignment + ) + + if grouping_type == Grouping.Type.COURSE: + # Course-grouping assignments have no dynamic groupings to fetch. The + # client only calls /api/sync here to report the document identity and + # have us sync the checkpoint against the course group. + groupings = [course] + elif group_set_id := request.parsed_params.get("group_set_id"): course_copy_plugin = request.product.plugin.course_copy # For course copy we might have stored a mapping for this `group_set_id` group_set_id = course.get_mapped_group_set_id(group_set_id) @@ -68,13 +91,6 @@ def sync(request): grading_student_id=grading_student_id, ) - # Look up the assignment so we can sync checkpoint data for the actual - # groupings (sections or canvas groups), not just the course group. - assignment = assignment_service.get_assignment( - course.application_instance.tool_consumer_instance_guid, - request.parsed_params["resource_link_id"], - ) - # Sync the groups over to H so they are ready to be annotated against. # Also sync checkpoint data if the assignment has checkpoint enabled. h_checkpoint_results = request.find_service(name="lti_h").sync( diff --git a/lms/views/lti/basic_launch.py b/lms/views/lti/basic_launch.py index 866195dab6..21432fd7b4 100644 --- a/lms/views/lti/basic_launch.py +++ b/lms/views/lti/basic_launch.py @@ -22,7 +22,6 @@ from lms.security import Permissions from lms.services import LTIGradingService, UserService, VitalSourceService from lms.services.assignment import AssignmentService # noqa: TC001 -from lms.services.document_uri import ensure_checkpoint_fingerprint from lms.services.lti_h import checkpoint_sync_data from lms.validation import BasicLTILaunchSchema, ConfigureAssignmentSchema @@ -198,12 +197,6 @@ def _show_document(self, assignment): # noqa: C901, PLR0912 ): assignment.extra["ext_lti_assignment_id"] = ext_lti_assignment_id - # For file-based Hide & Reveal assignments the h document identity is - # the file's PDF fingerprint: make sure it's computed and stored - # before we build the checkpoint sync data below. - if assignment.checkpoint_enabled: - ensure_checkpoint_fingerprint(self.request, assignment, self.course) - # Determine the grouping type to decide whether to sync checkpoint # data for the course group. When the assignment uses sections/groups, # the checkpoint sync happens in the client-side sync (POST /api/sync) From 277abab52bc198c8dd93918088c332ab7135fc24 Mon Sep 17 00:00:00 2001 From: Elim Pizza Date: Tue, 28 Jul 2026 11:59:57 -0300 Subject: [PATCH 04/12] Remove stale document_uri tests --- tests/unit/lms/services/assignment_test.py | 15 +- tests/unit/lms/services/document_uri_test.py | 387 ------------------- 2 files changed, 2 insertions(+), 400 deletions(-) delete mode 100644 tests/unit/lms/services/document_uri_test.py diff --git a/tests/unit/lms/services/assignment_test.py b/tests/unit/lms/services/assignment_test.py index 311599587a..dd18062d89 100644 --- a/tests/unit/lms/services/assignment_test.py +++ b/tests/unit/lms/services/assignment_test.py @@ -154,17 +154,6 @@ def test_update_assignment( assert assignment.lis_outcome_service_url == "GRADING URL" assert assignment.lti_v13_resource_link_id == v13_resource_link_id - def test_update_assignment_sets_document_uri(self, svc, pyramid_request, course): - assignment = svc.update_assignment( - pyramid_request, - factories.Assignment(), - "https://example.com/document", - sentinel.group_set_id, - course, - ) - - assert assignment.document_uri == "https://example.com/document" - def test_update_assignment_resets_document_uri_when_the_document_changes( self, svc, pyramid_request, course ): @@ -181,8 +170,8 @@ def test_update_assignment_resets_document_uri_when_the_document_changes( course, ) - # The new file's fingerprint isn't known yet: it gets computed on - # launch (see ensure_checkpoint_fingerprint). + # The new document's identity isn't known yet: the client re-reports it + # on the next launch (via /api/sync). assert assignment.document_uri is None def test_update_assignment_keeps_document_uri_when_the_document_is_unchanged( diff --git a/tests/unit/lms/services/document_uri_test.py b/tests/unit/lms/services/document_uri_test.py deleted file mode 100644 index efdb9c071e..0000000000 --- a/tests/unit/lms/services/document_uri_test.py +++ /dev/null @@ -1,387 +0,0 @@ -import hashlib - -import pytest - -from lms.services.document_uri import ( - _decode_pdf_literal, - ensure_checkpoint_fingerprint, - initial_document_uri, - pdf_fingerprint, -) -from tests import factories - - -class TestInitialDocumentURI: - @pytest.mark.parametrize( - "document_url", - [ - "https://example.com/article", - "http://example.com/doc.pdf", - "HTTPS://EXAMPLE.COM/ARTICLE", - ], - ) - def test_http_urls_are_returned_as_is( - self, pyramid_request, application_instance, document_url - ): - assert ( - initial_document_uri(pyramid_request, document_url, application_instance) - == document_url - ) - - @pytest.mark.parametrize( - "document_url", - [ - "https://youtu.be/VIDEO_ID", - "https://www.youtube.com/watch?v=VIDEO_ID&t=30s", - "https://www.youtube.com/shorts/VIDEO_ID", - ], - ) - def test_youtube_urls_return_the_canonical_video_url( - self, pyramid_request, application_instance, document_url - ): - # Via's YouTube player canonicalizes the URL before the client - # annotates it, whatever form the instructor pasted. - document_uri = initial_document_uri( - pyramid_request, document_url, application_instance - ) - - assert document_uri == "https://www.youtube.com/watch?v=VIDEO_ID" - - def test_youtube_urls_are_returned_as_is_when_youtube_is_disabled( - self, pyramid_request, application_instance - ): - application_instance.settings.set("youtube", "enabled", False) # noqa: FBT003 - - document_uri = initial_document_uri( - pyramid_request, "https://youtu.be/VIDEO_ID", application_instance - ) - - assert document_uri == "https://youtu.be/VIDEO_ID" - - def test_canvas_pages_return_the_canonical_url( - self, pyramid_request, application_instance - ): - document_uri = initial_document_uri( - pyramid_request, "canvas://page/course/42/page_id/314", application_instance - ) - - assert document_uri == "https://uni.instructure.com/courses/42/pages/314" - - def test_vitalsource_returns_the_bookshelf_url( - self, pyramid_request, application_instance - ): - document_uri = initial_document_uri( - pyramid_request, - "vitalsource://book/bookID/BOOK-ID/cfi//6/8", - application_instance, - ) - - assert document_uri == "https://bookshelf.vitalsource.com/reader/books/BOOK-ID" - - def test_invalid_vitalsource_urls_return_None( - self, pyramid_request, application_instance - ): - assert ( - initial_document_uri( - pyramid_request, "vitalsource://nonsense", application_instance - ) - is None - ) - - def test_canvas_studio_returns_the_canonical_video_url( - self, pyramid_request, application_instance - ): - application_instance.settings.set( - "canvas_studio", "domain", "uni.instructuremedia.com" - ) - - document_uri = initial_document_uri( - pyramid_request, "canvas-studio://media/55", application_instance - ) - - assert document_uri == "https://uni.instructuremedia.com/api/public/v1/media/55" - - def test_canvas_studio_without_a_domain_returns_None( - self, pyramid_request, application_instance - ): - assert ( - initial_document_uri( - pyramid_request, "canvas-studio://media/55", application_instance - ) - is None - ) - - def test_moodle_pages_return_the_proxy_resolved_url( - self, pyramid_request, application_instance - ): - document_uri = initial_document_uri( - pyramid_request, "moodle://page/course/42/page_id/860", application_instance - ) - - proxy_url = pyramid_request.route_url("moodle_api.pages.proxy") - assert document_uri == proxy_url.replace( - "/proxy", "/uni.instructure.com/mod/page/view.php?id=860" - ) - - @pytest.mark.parametrize( - "document_url", - [ - # PDF content: the identity is the fingerprint, which can't be - # derived from the URL (see ensure_checkpoint_fingerprint). - "canvas://file/course/42/file_id/99", - "blackboard://content-resource/FILE_ID/", - "d2l://file/course/42/file_id/99/", - "moodle://file/course/42/url/https%3A%2F%2Fmoodle.com%2Ffile.pdf", - "jstor://10.2307/1234", - ], - ) - def test_urls_with_no_derivable_identity_return_None( - self, pyramid_request, application_instance, document_url - ): - assert ( - initial_document_uri(pyramid_request, document_url, application_instance) - is None - ) - - @pytest.fixture - def application_instance(self): - return factories.ApplicationInstance(lms_url="https://uni.instructure.com") - - -@pytest.mark.usefixtures("canvas_service", "http_service") -class TestEnsureCheckpointFingerprint: - @pytest.mark.usefixtures("user_is_instructor") - def test_it_computes_and_stores_the_fingerprint( - self, pyramid_request, assignment, course, canvas_service, http_service - ): - http_service.get.return_value.content = PDF_WITH_HEX_ID - - ensure_checkpoint_fingerprint(pyramid_request, assignment, course) - - canvas_service.public_url_for_file.assert_called_once_with( - assignment, "99", "CANVAS_COURSE_ID" - ) - http_service.get.assert_called_once_with( - canvas_service.public_url_for_file.return_value - ) - assert ( - assignment.document_uri == f"urn:x-pdf:{pdf_fingerprint(PDF_WITH_HEX_ID)}" - ) - - @pytest.mark.usefixtures("user_is_instructor") - def test_it_does_nothing_if_document_uri_is_already_set( - self, pyramid_request, assignment, course, canvas_service - ): - assignment.document_uri = "urn:x-pdf:already-there" - - ensure_checkpoint_fingerprint(pyramid_request, assignment, course) - - canvas_service.public_url_for_file.assert_not_called() - assert assignment.document_uri == "urn:x-pdf:already-there" - - @pytest.mark.usefixtures("user_is_learner") - def test_it_does_nothing_for_non_instructors( - self, pyramid_request, assignment, course, canvas_service - ): - ensure_checkpoint_fingerprint(pyramid_request, assignment, course) - - canvas_service.public_url_for_file.assert_not_called() - assert assignment.document_uri is None - - @pytest.mark.usefixtures("user_is_instructor") - def test_it_computes_the_fingerprint_for_blackboard_files( - self, pyramid_request, assignment, course, blackboard_api_client, http_service - ): - assignment.document_url = "blackboard://content-resource/FILE_ID/" - http_service.get.return_value.content = PDF_WITH_HEX_ID - - ensure_checkpoint_fingerprint(pyramid_request, assignment, course) - - blackboard_api_client.public_url.assert_called_once_with( - course.lms_id, "FILE_ID" - ) - assert ( - assignment.document_uri == f"urn:x-pdf:{pdf_fingerprint(PDF_WITH_HEX_ID)}" - ) - - @pytest.mark.usefixtures("user_is_instructor", "oauth2_token_service") - def test_it_computes_the_fingerprint_for_d2l_files( - self, - pyramid_request, - assignment, - course, - d2l_api_client, - http_service, - oauth_token, - ): - assignment.document_url = "d2l://file/course/42/file_id/99/" - http_service.get.return_value.content = PDF_WITH_HEX_ID - - ensure_checkpoint_fingerprint(pyramid_request, assignment, course) - - d2l_api_client.public_url.assert_called_once_with(course.lms_id, "99") - http_service.get.assert_called_once_with( - d2l_api_client.public_url.return_value, - headers={"Authorization": f"Bearer {oauth_token.access_token}"}, - ) - assert ( - assignment.document_uri == f"urn:x-pdf:{pdf_fingerprint(PDF_WITH_HEX_ID)}" - ) - - @pytest.mark.usefixtures("user_is_instructor") - def test_it_computes_the_fingerprint_for_moodle_files( - self, pyramid_request, assignment, course, moodle_api_client, http_service - ): - assignment.document_url = ( - "moodle://file/course/42/url/https://moodle.com/file.pdf" - ) - http_service.get.return_value.content = PDF_WITH_HEX_ID - - ensure_checkpoint_fingerprint(pyramid_request, assignment, course) - - http_service.get.assert_called_once_with( - "https://moodle.com/file.pdf", params={"token": moodle_api_client.token} - ) - assert ( - assignment.document_uri == f"urn:x-pdf:{pdf_fingerprint(PDF_WITH_HEX_ID)}" - ) - - @pytest.mark.usefixtures("user_is_instructor") - def test_it_computes_the_fingerprint_for_jstor( - self, pyramid_request, assignment, course, jstor_service, http_service - ): - assignment.document_url = "jstor://10.2307/1234" - jstor_service.enabled = True - http_service.get.return_value.content = PDF_WITH_HEX_ID - - ensure_checkpoint_fingerprint(pyramid_request, assignment, course) - - jstor_service.public_url.assert_called_once_with("jstor://10.2307/1234") - http_service.get.assert_called_once_with(jstor_service.public_url.return_value) - assert ( - assignment.document_uri == f"urn:x-pdf:{pdf_fingerprint(PDF_WITH_HEX_ID)}" - ) - - @pytest.mark.usefixtures("user_is_instructor") - def test_it_does_nothing_for_jstor_when_disabled( - self, pyramid_request, assignment, course, jstor_service, http_service - ): - assignment.document_url = "jstor://10.2307/1234" - jstor_service.enabled = False - - ensure_checkpoint_fingerprint(pyramid_request, assignment, course) - - http_service.get.assert_not_called() - assert assignment.document_uri is None - - @pytest.mark.usefixtures("user_is_instructor") - def test_it_does_nothing_for_non_file_urls( - self, pyramid_request, assignment, course, canvas_service, http_service - ): - assignment.document_url = "moodle://page/course/42/page_id/314" - - ensure_checkpoint_fingerprint(pyramid_request, assignment, course) - - canvas_service.public_url_for_file.assert_not_called() - http_service.get.assert_not_called() - assert assignment.document_uri is None - - @pytest.mark.usefixtures("user_is_instructor") - def test_it_swallows_errors( - self, pyramid_request, assignment, course, canvas_service - ): - canvas_service.public_url_for_file.side_effect = RuntimeError("API is down") - - ensure_checkpoint_fingerprint(pyramid_request, assignment, course) - - assert assignment.document_uri is None - - @pytest.fixture - def assignment(self): - return factories.Assignment( - document_url="canvas://file/course/42/file_id/99", checkpoint_enabled=True - ) - - @pytest.fixture - def course(self): - course = factories.Course() - course.extra = {"canvas": {"custom_canvas_course_id": "CANVAS_COURSE_ID"}} - return course - - -# A minimal classic-trailer PDF tail with a hex /ID. -PDF_WITH_HEX_ID = ( - b"%PDF-1.4\nsome pdf content here\n" - b"trailer\n<< /Size 10 /Root 1 0 R /ID [" - b"] >>\nstartxref\n123\n%%EOF\n" -) - - -class TestPDFFingerprint: - def test_it_uses_the_original_id_when_present(self): - assert pdf_fingerprint(PDF_WITH_HEX_ID) == "deadbeefdeadbeefdeadbeefdeadbeef" - - def test_it_uses_the_last_id_in_the_file(self): - # Incremental updates append a new trailer: the last one wins. - pdf = ( - b"%PDF-1.4\n" - b"trailer\n<< /ID [<11111111111111111111111111111111>" - b"<11111111111111111111111111111111>] >>\n%%EOF\n" - b"trailer\n<< /ID [<22222222222222222222222222222222>" - b"<33333333333333333333333333333333>] >>\n%%EOF\n" - ) - - assert pdf_fingerprint(pdf) == "22222222222222222222222222222222" - - def test_it_allows_whitespace_inside_hex_ids(self): - pdf = b"/ID [ <00> ]" - - assert pdf_fingerprint(pdf) == "deadbeefdeadbeefdeadbeefdeadbeef" - - def test_it_pads_odd_length_hex_ids(self): - # Per the PDF spec an odd number of hex digits gets a trailing 0. - pdf = b"/ID [<00>]" - - assert pdf_fingerprint(pdf) == "deadbee0" - - def test_it_decodes_literal_string_ids(self): - pdf = rb"/ID [(AB\\C\)D\101) (other)]" - - assert pdf_fingerprint(pdf) == b"AB\\C)DA".hex() - - def test_it_falls_back_to_md5_when_there_is_no_id(self): - pdf = b"%PDF-1.4\nno id in this file\n" * 100 - - assert ( - pdf_fingerprint(pdf) == hashlib.md5(pdf[:1024]).hexdigest() # noqa: S324 - ) - - def test_it_falls_back_to_md5_when_the_id_is_all_zeroes(self): - # Same validation as PDF.js: an all-NUL 16-byte ID is "empty". - pdf = b"/ID [<00000000000000000000000000000000><00>]" - - assert ( - pdf_fingerprint(pdf) == hashlib.md5(pdf[:1024]).hexdigest() # noqa: S324 - ) - - def test_it_falls_back_to_md5_when_the_id_is_empty(self): - pdf = b"/ID [<><>]" - - assert ( - pdf_fingerprint(pdf) == hashlib.md5(pdf[:1024]).hexdigest() # noqa: S324 - ) - - def test_it_ignores_the_backslash_of_an_unknown_literal_escape(self): - # An unrecognized escape (not in the escape table and not octal) drops - # the backslash and keeps the following character, matching PDF.js. - pdf = rb"/ID [(A\qB) (other)]" - - assert pdf_fingerprint(pdf) == b"AqB".hex() - - -class TestDecodePDFLiteral: - def test_a_trailing_backslash_is_dropped(self): - # A lone backslash at the very end of a literal has nothing to escape, - # so it's ignored. (The /ID regex never yields this, but the decoder - # guards against it anyway.) - assert _decode_pdf_literal(b"AB\\") == b"AB" From 2de065549277fd4af951d2658e29d7f516b40fa9 Mon Sep 17 00:00:00 2001 From: Elim Pizza Date: Tue, 28 Jul 2026 12:21:18 -0300 Subject: [PATCH 05/12] Format fixes --- lms/services/lti_h.py | 2 +- lms/views/api/checkpoint.py | 2 +- lms/views/api/sync.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lms/services/lti_h.py b/lms/services/lti_h.py index 9c71fbfb43..d0f122f087 100644 --- a/lms/services/lti_h.py +++ b/lms/services/lti_h.py @@ -7,7 +7,7 @@ def checkpoint_sync_data(assignment: Assignment | None, lti_user) -> dict | None: """Build the checkpoint payload to sync to h for a Hide & Reveal assignment. - Syncs against `assignment.document_uri` — the h document identity. + Syncs against `assignment.document_uri` — the h document identity. Returns None when the assignment is missing, doesn't have checkpoint enabled, or the client hasn't reported an identity yet (`assignment.document_uri` is None), so callers can pass the result straight diff --git a/lms/views/api/checkpoint.py b/lms/views/api/checkpoint.py index 9f76ae6aeb..25b2f61309 100644 --- a/lms/views/api/checkpoint.py +++ b/lms/views/api/checkpoint.py @@ -42,7 +42,7 @@ def reveal_checkpoint(request): raise HTTPNotFound(message) # The checkpoint in h is keyed by the document's identity there — the URI - # the client reports (`assignment.document_uri`). If the client hasn't reported + # the client reports (`assignment.document_uri`). If the client hasn't reported # one, no checkpoint can have been synced, so there's nothing to reveal. if not assignment.document_uri: message = "Assignment or checkpoint not found" diff --git a/lms/views/api/sync.py b/lms/views/api/sync.py index a66959918c..41cea88e36 100644 --- a/lms/views/api/sync.py +++ b/lms/views/api/sync.py @@ -47,7 +47,7 @@ def sync(request): grouping_type = grouping_service.get_launch_grouping_type( request, course, assignment ) - + if grouping_type == Grouping.Type.COURSE: # Course-grouping assignments have no dynamic groupings to fetch. The # client only calls /api/sync here to report the document identity and From e7beb4025abc8c828fffd6d654577a0b94d4393e Mon Sep 17 00:00:00 2001 From: Elim Pizza Date: Tue, 28 Jul 2026 12:27:51 -0300 Subject: [PATCH 06/12] Cover client-reported document identity in frontend tests --- .../components/test/BasicLTILaunchApp-test.js | 64 +++++++++++++++++++ .../services/test/client-rpc-test.js | 13 ++++ 2 files changed, 77 insertions(+) diff --git a/lms/static/scripts/frontend_apps/components/test/BasicLTILaunchApp-test.js b/lms/static/scripts/frontend_apps/components/test/BasicLTILaunchApp-test.js index f3aeb3636d..ad61df7877 100644 --- a/lms/static/scripts/frontend_apps/components/test/BasicLTILaunchApp-test.js +++ b/lms/static/scripts/frontend_apps/components/test/BasicLTILaunchApp-test.js @@ -67,6 +67,7 @@ describe('BasicLTILaunchApp', () => { on: sinon.stub(), off: sinon.stub(), setGroups: sinon.stub(), + getDocumentUri: sinon.stub().resolves(''), }; $imports.$mock(mockImportedComponents()); @@ -162,6 +163,69 @@ describe('BasicLTILaunchApp', () => { }); }); + context('when the assignment has Hide & Reveal checkpoints enabled', () => { + beforeEach(() => { + fakeConfig.api.sync = { + data: { course: { context_id: '12345' } }, + path: '/api/sync', + }; + fakeConfig.instructorToolbar = { assignmentCheckpointEnabled: true }; + }); + + it('reports the document identity and syncs the checkpoint', async () => { + fakeRpcServer.getDocumentUri.resolves('urn:x-pdf:FINGERPRINT'); + const checkpoint = { revealed: false, revealDate: null }; + fakeApiCall.callsFake(async ({ data }) => + data.document_uri ? { checkpoint } : { groups: ['group1'] }, + ); + + const wrapper = renderLTILaunchApp(); + + await waitFor(() => + fakeApiCall + .getCalls() + .some( + call => call.args[0].data.document_uri === 'urn:x-pdf:FINGERPRINT', + ), + ); + assert.calledWith(fakeApiCall, { + authToken: 'dummyAuthToken', + path: '/api/sync', + data: { + course: { context_id: '12345' }, + document_uri: 'urn:x-pdf:FINGERPRINT', + }, + }); + + await waitFor(() => { + wrapper.update(); + return ( + wrapper.find('InstructorToolbar').prop('syncCheckpoint') !== null + ); + }); + assert.deepEqual( + wrapper.find('InstructorToolbar').prop('syncCheckpoint'), + checkpoint, + ); + }); + + it('does not sync a checkpoint when the client reports no document identity', async () => { + fakeRpcServer.getDocumentUri.resolves(''); + fakeApiCall.resolves({ groups: ['group1'] }); + + renderLTILaunchApp(); + await waitFor(() => fakeApiCall.called); + // Let the getDocumentUri promise settle before asserting. + await delay(0); + + assert.isFalse( + fakeApiCall + .getCalls() + .some(call => 'document_uri' in call.args[0].data), + ); + }); + }); + it('renders the instructor and student toolbars', () => { const wrapper = renderLTILaunchApp(); assert.isTrue(wrapper.exists('InstructorToolbar')); diff --git a/lms/static/scripts/frontend_apps/services/test/client-rpc-test.js b/lms/static/scripts/frontend_apps/services/test/client-rpc-test.js index 23109d659f..9745de246d 100644 --- a/lms/static/scripts/frontend_apps/services/test/client-rpc-test.js +++ b/lms/static/scripts/frontend_apps/services/test/client-rpc-test.js @@ -227,6 +227,19 @@ describe('ClientRPC', () => { }); }); + describe('getDocumentUri', () => { + it('resolves with the URI reported via the "reportDocumentInfo" RPC handler', async () => { + const clientRPC = createClientRPC(); + + const [, callback] = fakeServerInstance.register.args.find( + ([method]) => method === 'reportDocumentInfo', + ); + callback({ uri: 'https://example.com/doc' }); + + assert.equal(await clientRPC.getDocumentUri(), 'https://example.com/doc'); + }); + }); + describe('setFocusedUser', () => { it('sets focused user in client when user is passed', async () => { const clientRPC = createClientRPC(); From 0aeab1c88c2aedaeafd9c4984ee1c18fcf719786 Mon Sep 17 00:00:00 2001 From: Karen Rasmussen Date: Tue, 28 Jul 2026 13:02:18 -0300 Subject: [PATCH 07/12] Inline back YouTube and JSTOR helpers no longer shared --- lms/resources/_js_config/__init__.py | 24 ++++++++++++++++++++++-- lms/services/jstor/service.py | 24 ++++++++---------------- lms/services/youtube.py | 23 ----------------------- 3 files changed, 30 insertions(+), 41 deletions(-) diff --git a/lms/resources/_js_config/__init__.py b/lms/resources/_js_config/__init__.py index d81d994021..816126c066 100644 --- a/lms/resources/_js_config/__init__.py +++ b/lms/resources/_js_config/__init__.py @@ -3,6 +3,7 @@ from datetime import UTC, timedelta from enum import Enum, StrEnum from typing import Any +from urllib.parse import urlparse from lms.error_code import ErrorCode from lms.events import LTIEvent @@ -23,10 +24,29 @@ VitalSourceService, YouTubeService, ) -from lms.services.youtube import video_id_from_url from lms.validation.authentication import BearerTokenSchema from lms.views.helpers import via_url +# Regex to extract YouTube video ID (same URL patterns as frontend utils/youtube.ts) +_YOUTUBE_VIDEO_ID_RE = re.compile( + r"(?:youtu\.be/|v/|u/\w/|embed/|shorts/|live/|watch\?v=|&v=)([^#&?]*)", + re.IGNORECASE, +) + + +def _youtube_video_id_from_url(url: str) -> str | None: + """Return the YouTube video ID if url is a YouTube URL, else None.""" + try: + parsed = urlparse(url) + if parsed.scheme not in ("http", "https"): + return None + if parsed.netloc.lower() not in ("www.youtube.com", "youtube.com", "youtu.be"): + return None + match = _YOUTUBE_VIDEO_ID_RE.search(url) + return match.group(1) if match and match.group(1) else None + except (ValueError, AttributeError): + return None + class JSConfig: """The config for the app's JavaScript code.""" @@ -171,7 +191,7 @@ def add_document_url( # pylint: disable=too-complex,too-many-branches,useless-s else: self._config["viaUrl"] = via_url(self._request, document_url) youtube_service = self._request.find_service(iface=YouTubeService) - if youtube_service.enabled and video_id_from_url(document_url): + if youtube_service.enabled and _youtube_video_id_from_url(document_url): self._hypothesis_client["youtubeAssignment"] = True def _update_focus_config(self, updates: dict): diff --git a/lms/services/jstor/service.py b/lms/services/jstor/service.py index 68893dda6c..b782a98e67 100644 --- a/lms/services/jstor/service.py +++ b/lms/services/jstor/service.py @@ -46,14 +46,18 @@ def enabled(self) -> bool: return bool(self._enabled and self._api_url and self._site_code) - def public_url(self, document_url) -> str: + def via_url(self, request, document_url): """ - Get a signed S3 URL for the PDF of a jstor:// document. + Get a VIA url for a document. - :param document_url: The jstor:// URL of the document + :param request: Pyramid request + :param document_url: The URL to annotate + :return: A URL for Via configured to launch the requested document :raises ExternalRequestError: If we get a value which doesn't look like a public URL from JSTOR """ + + # Get a signed S3 URL for the given JSTOR URL. s3_url = self._api_request( "/pdf/{doi}", doi=document_url.replace("jstor://", "") ).text @@ -63,21 +67,9 @@ def public_url(self, document_url) -> str: f"Expected to get an S3 URL but got: '{s3_url}' instead" # noqa: EM102 ) - return s3_url - - def via_url(self, request, document_url): - """ - Get a VIA url for a document. - - :param request: Pyramid request - :param document_url: The URL to annotate - :return: A URL for Via configured to launch the requested document - :raises ExternalRequestError: If we get a value which doesn't look like - a public URL from JSTOR - """ return via_url( request, - document_url=self.public_url(document_url), + document_url=s3_url, content_type="pdf", # Show content partner banner in client for JSTOR. options={"via.client.contentPartner": "jstor"}, diff --git a/lms/services/youtube.py b/lms/services/youtube.py index bc02eb1dd9..93ef3aa679 100644 --- a/lms/services/youtube.py +++ b/lms/services/youtube.py @@ -1,32 +1,9 @@ -import re -from urllib.parse import urlparse - from lms.services.exceptions import SerializableError from lms.services.http import HTTPService YOUTUBE_API_URL = "https://www.googleapis.com/youtube/v3" """YouTube's API base URL""" -# Regex to extract YouTube video ID (same URL patterns as frontend utils/youtube.ts) -_YOUTUBE_VIDEO_ID_RE = re.compile( - r"(?:youtu\.be/|v/|u/\w/|embed/|shorts/|live/|watch\?v=|&v=)([^#&?]*)", - re.IGNORECASE, -) - - -def video_id_from_url(url: str) -> str | None: - """Return the YouTube video ID if url is a YouTube URL, else None.""" - try: - parsed = urlparse(url) - if parsed.scheme not in ("http", "https"): - return None - if parsed.netloc.lower() not in ("www.youtube.com", "youtube.com", "youtu.be"): - return None - match = _YOUTUBE_VIDEO_ID_RE.search(url) - return match.group(1) if match and match.group(1) else None - except (ValueError, AttributeError): - return None - class VideoNotFound(SerializableError): # noqa: N818 def __init__(self, video_id): From 205d8a3cb279a2982b390996aa8d8c9c3eae59cd Mon Sep 17 00:00:00 2001 From: Elim Pizza Date: Tue, 28 Jul 2026 13:05:04 -0300 Subject: [PATCH 08/12] Coverage --- .../lms/resources/_js_config/__init___test.py | 16 +++++++++ tests/unit/lms/views/api/sync_test.py | 33 +++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/tests/unit/lms/resources/_js_config/__init___test.py b/tests/unit/lms/resources/_js_config/__init___test.py index b28b3eb736..a2751fa7b1 100644 --- a/tests/unit/lms/resources/_js_config/__init___test.py +++ b/tests/unit/lms/resources/_js_config/__init___test.py @@ -273,6 +273,22 @@ def test_configures_the_client_with_course_group( ] assert not config["api"]["sync"] + def test_configures_the_client_with_course_group_and_checkpoints( + self, js_config, grouping_service, course, assignment + ): + grouping_service.get_launch_grouping_type.return_value = Grouping.Type.COURSE + assignment.checkpoint_enabled = True + + js_config.enable_lti_launch_mode(course, assignment) + config = js_config.asdict() + + assert config["hypothesisClient"]["services"][0]["groups"] == [ + Any.string.matching("^group:.*@lms.hypothes.is") + ] + # Hide & Reveal assignments still need the sync API so the client can + # report the document identity and receive the checkpoint state. + assert config["api"]["sync"]["path"] == "/api/sync" + @pytest.mark.usefixtures("grouping_plugin") @pytest.mark.parametrize( "grouping_type", [Grouping.Type.SECTION, Grouping.Type.GROUP] diff --git a/tests/unit/lms/views/api/sync_test.py b/tests/unit/lms/views/api/sync_test.py index 0a5e4947b4..bab463b61d 100644 --- a/tests/unit/lms/views/api/sync_test.py +++ b/tests/unit/lms/views/api/sync_test.py @@ -2,6 +2,7 @@ import pytest +from lms.models import Grouping from lms.product.plugin.grouping import GroupError from lms.views.api.sync import sync from tests import factories @@ -302,6 +303,38 @@ def test_it_omits_checkpoint_state_when_h_returns_no_results( assert "checkpoint" not in result + @pytest.mark.usefixtures("grouping_service", "course_service", "lti_h_service") + def test_it_stores_the_client_reported_document_uri( + self, pyramid_request, assignment_service + ): + assignment = assignment_service.get_assignment.return_value + assignment.checkpoint_enabled = True + pyramid_request.parsed_params["document_uri"] = "https://example.com/reported" + + sync(pyramid_request) + + assert assignment.document_uri == "https://example.com/reported" + + def test_it_with_course_grouping( + self, + pyramid_request, + grouping_service, + course_service, + assignment_service, # noqa: ARG002 + lti_h_service, + ): + grouping_service.get_launch_grouping_type.return_value = Grouping.Type.COURSE + course = course_service.get_by_context_id.return_value + + returned_ids = sync(pyramid_request) + + lti_h_service.sync.assert_called_once_with( + [course], + sentinel.group_info, + checkpoint_data=None, + ) + assert returned_ids["groups"] == [course.groupid.return_value] + @pytest.fixture def assignment_service(self, assignment_service): assignment_service.get_assignment.return_value.checkpoint_enabled = False From d6988f588e2e682eab7151da9aef5d98ccc896ba Mon Sep 17 00:00:00 2001 From: Elim Pizza Date: Tue, 28 Jul 2026 13:16:38 -0300 Subject: [PATCH 09/12] Fix _js_config test import after YouTube helper inlined --- tests/unit/lms/resources/_js_config/__init___test.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/unit/lms/resources/_js_config/__init___test.py b/tests/unit/lms/resources/_js_config/__init___test.py index a2751fa7b1..5cffd48586 100644 --- a/tests/unit/lms/resources/_js_config/__init___test.py +++ b/tests/unit/lms/resources/_js_config/__init___test.py @@ -7,10 +7,9 @@ from lms.models import Grouping, LTIParams from lms.product.product import Routes from lms.resources import LTILaunchResource, OAuth2RedirectResource -from lms.resources._js_config import JSConfig +from lms.resources._js_config import JSConfig, _youtube_video_id_from_url from lms.security import Identity, Permissions from lms.services import HAPIError -from lms.services.youtube import video_id_from_url from lms.views.api.sync import APISyncSchema from tests import factories from tests.conftest import TEST_SETTINGS @@ -576,13 +575,13 @@ def test_non_youtube_url_does_not_set_client_flag( def test_youtube_video_id_from_url_returns_none_on_parse_error(self): """Cover the except (ValueError, AttributeError) branch.""" - with patch("lms.services.youtube.urlparse", side_effect=ValueError): - assert video_id_from_url("https://www.youtube.com/watch?v=abc") is None + with patch("lms.resources._js_config.urlparse", side_effect=ValueError): + assert _youtube_video_id_from_url("https://youtube.com/watch?v=abc") is None def test_youtube_video_id_from_url_is_case_insensitive_for_host(self): """Host is normalized so YouTube.com / YOUTUBE.COM work like the frontend.""" - assert video_id_from_url("https://YouTube.com/watch?v=xyz") == "xyz" - assert video_id_from_url("https://YOUTU.BE/xyz") == "xyz" + assert _youtube_video_id_from_url("https://YouTube.com/watch?v=xyz") == "xyz" + assert _youtube_video_id_from_url("https://YOUTU.BE/xyz") == "xyz" class TestAddCanvasSpeedgraderSettings: From 8fcd63b26a9bb40623fac9d3ba9fcce183e9a277 Mon Sep 17 00:00:00 2001 From: Elim Pizza Date: Tue, 28 Jul 2026 15:46:52 -0300 Subject: [PATCH 10/12] Fold document_uri check into reveal_checkpoint guard --- lms/views/api/checkpoint.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/lms/views/api/checkpoint.py b/lms/views/api/checkpoint.py index 25b2f61309..1ae19937d2 100644 --- a/lms/views/api/checkpoint.py +++ b/lms/views/api/checkpoint.py @@ -27,9 +27,12 @@ def reveal_checkpoint(request): # application instance (guards cross-institution reveal). # - Membership scope: the caller must be a member of the assignment (guards # an instructor of a different course within the same institution). + # - Identity scope: without a client-reported `document_uri` no checkpoint + # can have been synced to h, so there is nothing to reveal. if ( not assignment or not assignment.checkpoint_enabled + or not assignment.document_uri or not assignment.course or assignment.course.application_instance_id != request.lti_user.application_instance_id @@ -41,13 +44,6 @@ def reveal_checkpoint(request): message = "Assignment or checkpoint not found" raise HTTPNotFound(message) - # The checkpoint in h is keyed by the document's identity there — the URI - # the client reports (`assignment.document_uri`). If the client hasn't reported - # one, no checkpoint can have been synced, so there's nothing to reveal. - if not assignment.document_uri: - message = "Assignment or checkpoint not found" - raise HTTPNotFound(message) - # Reveal directly in h — h is the source of truth for reveal state. h_api = request.find_service(HAPI) # If the assignment has section/group groupings, only reveal those — From c3be256a7a084b6667e2c814e1f36e0db3af1b2c Mon Sep 17 00:00:00 2001 From: Elim Pizza Date: Tue, 28 Jul 2026 15:54:16 -0300 Subject: [PATCH 11/12] Remove migration --- ...b531_add_assignment_document_uri_column.py | 23 ------------------- 1 file changed, 23 deletions(-) delete mode 100644 lms/migrations/versions/fa62e42cb531_add_assignment_document_uri_column.py diff --git a/lms/migrations/versions/fa62e42cb531_add_assignment_document_uri_column.py b/lms/migrations/versions/fa62e42cb531_add_assignment_document_uri_column.py deleted file mode 100644 index 44a02254ce..0000000000 --- a/lms/migrations/versions/fa62e42cb531_add_assignment_document_uri_column.py +++ /dev/null @@ -1,23 +0,0 @@ -"""Add assignment document_uri column. - -Revision ID: fa62e42cb531 -Revises: 2a45f5cb8e25 -""" - -import sqlalchemy as sa -from alembic import op - -revision = "fa62e42cb531" -down_revision = "2a45f5cb8e25" - - -def upgrade() -> None: - # ### commands auto generated by Alembic - please adjust! ### - op.add_column("assignment", sa.Column("document_uri", sa.Unicode(), nullable=True)) - # ### end Alembic commands ### - - -def downgrade() -> None: - # ### commands auto generated by Alembic - please adjust! ### - op.drop_column("assignment", "document_uri") - # ### end Alembic commands ### From 30965c951a323f7cce6a2ebfd7827f5d90094ee9 Mon Sep 17 00:00:00 2001 From: Elim Pizza Date: Wed, 29 Jul 2026 11:15:10 -0300 Subject: [PATCH 12/12] Revert unnecessary sentinel.document_url replacements in tests --- tests/unit/lms/services/assignment_test.py | 30 +++++++++++----------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/unit/lms/services/assignment_test.py b/tests/unit/lms/services/assignment_test.py index dd18062d89..aff194029a 100644 --- a/tests/unit/lms/services/assignment_test.py +++ b/tests/unit/lms/services/assignment_test.py @@ -136,18 +136,18 @@ def test_update_assignment( assignment = svc.update_assignment( pyramid_request, factories.Assignment(), - "https://example.com/document", + sentinel.document_url, sentinel.group_set_id, course, ) if is_speed_grader: assert assignment.extra == {} - assert assignment.document_url != "https://example.com/document" + assert assignment.document_url != sentinel.document_url assert not assignment.lis_outcome_service_url assert not assignment.lti_v13_resource_link_id else: - assert assignment.document_url == "https://example.com/document" + assert assignment.document_url == sentinel.document_url assert assignment.extra["group_set_id"] == sentinel.group_set_id assert assignment.title == title assert assignment.course_id == course.id @@ -209,7 +209,7 @@ def test_update_assignment_with_auto_grading_config( assignment = svc.update_assignment( pyramid_request, assignment, - "https://example.com/document", + sentinel.document_url, sentinel.group_set_id, course, auto_grading_config={ @@ -257,7 +257,7 @@ def test_update_assignment_with_checkpoint(self, svc, pyramid_request, course): assignment = svc.update_assignment( pyramid_request, assignment, - "https://example.com/document", + sentinel.document_url, sentinel.group_set_id, course, checkpoint_enabled=True, @@ -271,7 +271,7 @@ def test_update_assignment_without_checkpoint(self, svc, pyramid_request, course assignment = svc.update_assignment( pyramid_request, assignment, - "https://example.com/document", + sentinel.document_url, sentinel.group_set_id, course, checkpoint_enabled=False, @@ -287,7 +287,7 @@ def test_update_assignment_keeps_existing_checkpoint( assignment = svc.update_assignment( pyramid_request, assignment, - "https://example.com/document", + sentinel.document_url, sentinel.group_set_id, course, checkpoint_enabled=True, @@ -313,7 +313,7 @@ def test_update_assignment_with_due_date( assignment = svc.update_assignment( pyramid_request, factories.Assignment(), - "https://example.com/document", + sentinel.document_url, sentinel.group_set_id, course, due_date=due_date, @@ -366,7 +366,7 @@ def test_get_assignment_for_launch_existing( course, ): misc_plugin.get_assignment_configuration.return_value = { - "document_url": "https://example.com/document", + "document_url": sentinel.document_url, "group_set_id": sentinel.group_set_id, } get_assignment.return_value = factories.Assignment() @@ -380,7 +380,7 @@ def test_get_assignment_for_launch_existing( misc_plugin.is_assignment_gradable.assert_called_once_with( pyramid_request.lti_params ) - assert assignment.document_url == "https://example.com/document" + assert assignment.document_url == sentinel.document_url assert assignment.extra["group_set_id"] == sentinel.group_set_id assert assignment.title == pyramid_request.lti_params.get("resource_link_title") @@ -400,7 +400,7 @@ def test_get_assignment_for_launch_sets_due_date( course, ): misc_plugin.get_assignment_configuration.return_value = { - "document_url": "https://example.com/document", + "document_url": sentinel.document_url, "group_set_id": sentinel.group_set_id, "due_date": "2026-07-01T12:00:00+00:00", } @@ -430,7 +430,7 @@ def test_get_assignment_creates_assignment( course, ): misc_plugin.get_assignment_configuration.return_value = { - "document_url": "https://example.com/document", + "document_url": sentinel.document_url, "group_set_id": group_set_id, } create_assignment.return_value = factories.Assignment() @@ -443,7 +443,7 @@ def test_get_assignment_creates_assignment( create_assignment.assert_called_once_with( "TEST_TOOL_CONSUMER_INSTANCE_GUID", "TEST_RESOURCE_LINK_ID" ) - assert assignment.document_url == "https://example.com/document" + assert assignment.document_url == sentinel.document_url assert assignment.course_id == course.id if group_set_id: assignment.extra["group_set_id"] = group_set_id @@ -460,7 +460,7 @@ def test_get_assignment_created_assignments_point_to_copy( course, ): misc_plugin.get_assignment_configuration.return_value = { - "document_url": "https://example.com/document" + "document_url": sentinel.document_url } get_assignment.return_value = None _get_copied_from_assignment.return_value = sentinel.original_assignment @@ -472,7 +472,7 @@ def test_get_assignment_created_assignments_point_to_copy( "TEST_TOOL_CONSUMER_INSTANCE_GUID", "TEST_RESOURCE_LINK_ID" ) assert assignment.copied_from == sentinel.original_assignment - assert assignment.document_url == "https://example.com/document" + assert assignment.document_url == sentinel.document_url @pytest.mark.parametrize("with_lti11_grading_id", [True, False]) def test_upsert_assignment_membership(