fix: deep linking target_link_uri and AGS endpoint quirks by navinkarkera · Pull Request #643 · openedx/xblock-lti-consumer

navinkarkera · 2026-04-12T14:37:35Z

Description:

Deep linking launch uses correct target_link_uri so tools hit deep link endpoint
Scores API accepts blank comment per spec expectations
AGS result endpoint works with or without trailing slash
Uses correct key-secret pair based on config_store type

Supporting information:

Fixes following issues:

Private-ref: https://tasks.opencraft.com/browse/FAL-4346

Test instructions:

Only review last 4 commits

For #633

Create an LTI xblock and link it with saltire.
Enable deep linking and use a slightly different deep linking url like: https://saltire.lti.app/tool?test=1
Publish and click on Deep linking Launch - Configure tool in studio.
Verify that the https://purl.imsglobal.org/spec/lti/claim/target_link_uri field is set to the deep linking url in Message Claims (derived from message parameters) section

For #630

Use the same block and open it in LMS.
Go to Services -> OAuth 2 Access Token, select all scopes and request a new token.
Go to Message -> Services Available -> Result section
Under Result section, click on Read button. It should not fail.

For #637

Just below Result section, click on Score publish Service section
Fill in all fields but leave Comment field empty and click on Create/Update button. It should not fail.

For #620

Could not test this locally due.
TODO: test on sandbox.

openedx-webhooks · 2026-04-12T14:37:45Z

Thanks for the pull request, @navinkarkera!

This repository is currently maintained by @Faraz32123.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

codecov · 2026-04-12T14:49:13Z

Codecov Report

❌ Patch coverage is 98.37838% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.63%. Comparing base (7b1c939) to head (f1471d0).

Files with missing lines	Patch %	Lines
...ssport_context_key_lti1p3passport_name_and_more.py	76.92%	6 Missing ⚠️
lti_consumer/models.py	96.80%	3 Missing ⚠️
lti_consumer/lti_xblock.py	84.61%	2 Missing ⚠️
...onsumer/migrations/0021_create_lti_1p3_passport.py	90.90%	2 Missing ⚠️
lti_consumer/signals/signals.py	97.01%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #643      +/-   ##
==========================================
+ Coverage   97.59%   97.63%   +0.03%     
==========================================
  Files          79       84       +5     
  Lines        6871     7606     +735     
==========================================
+ Hits         6706     7426     +720     
- Misses        165      180      +15

Flag	Coverage Δ
unittests	`97.63% <98.37%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Introduce Lti1p3Passport model to centralize LTI 1.3 keys and credentials - Move lti_1p3_internal_private_key, lti_1p3_internal_private_key_id, lti_1p3_internal_public_jwk, lti_1p3_client_id, lti_1p3_tool_public_key, and lti_1p3_tool_keyset_url fields from LtiConfiguration to Lti1p3Passport - Add ForeignKey relationship from LtiConfiguration to Lti1p3Passport - Implement passport-based key generation and retrieval - Add clean() validation to Lti1p3Passport to ensure at least one of lti_1p3_tool_public_key or lti_1p3_tool_keyset_url is set - Update validation in LtiConfiguration.clean() to check for passport presence instead of tool key fields - Refactor get_or_create_local_lti_config() to handle passport creation and sync block/passport key configurations - Update API endpoints to work with passport ID instead of configuration ID - Add admin interface for Lti1p3Passport model - Refactor access_token_endpoint and public_keyset_endpoint to use passport ID - Update API and views to work with the new passport model - Generate migration to remove fields from LtiConfiguration table - Update data migration to copy existing configurations to the new Passport model - Update XBlock to store passport ID instead of config ID - Fix copy-paste issue in resource_link_id generation

* Add signal handlers to delete LTI configurations when xblocks or library blocks are deleted * Ensure LTI configurations are properly cleaned up when associated blocks are removed from the system * Update documentation for LTI 1.3 configuration changes to inform users about potential regeneration of client IDs and URLs when public keys are changed

• Fixed spelling errors (configurtion → configuration, url → URL) • Improved log messages to be more informative • Used more descriptive variable names (id_list → block_locations) • Maintained consistent code style and import organization

…_1p3_passport

- Added 'name' and 'context_key' fields to Lti1p3Passport model - Modified LtiConfiguration to populate these fields when creating passports - Updated admin interface to display new fields - Added migration to populate existing passports with name and context_key - Updated string representation of passport to include name - Made 'location' field in LtiConfiguration unique to prevent race conditions This fixes a race condition that occurred when get_or_create was called with the same location multiple times, which resulted in duplicate rows with identical locations being created simultaneously.

rpenido · 2026-04-13T18:40:11Z

Hi @navinkarkera! I checked the code and it LGTM!
But I didn't manage to test it manually.

I tried to configure with the following:

Do we have any docs on how to link that?

Adds tests covering possible cases

navinkarkera · 2026-04-15T14:28:28Z

@rpenido I'll create a sandbox once all bugs are ready to be tested, you can then try it out. It is bit tricky locally as you need to setup tunneling to make it work.

Fixes: openedx#628

Fixes: openedx#637

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Apr 12, 2026

openedx-webhooks added this to Contributions Apr 12, 2026

github-project-automation bot moved this to Needs Triage in Contributions Apr 12, 2026

navinkarkera added 24 commits April 13, 2026 16:50

fix: lint issues

3979b16

feat: install openedx-events

8a268b3

fix: model label

2e6ce8b

fix: handle no location

999156d

test: fix tests

de50bae

fix: lint issues

a5ce602

fixup! fix: lint issues

64d7aae

test: add signal tests

f31fa61

fix: coverage

b34d803

refactor(models): rename create_lti_1p3_passport to get_or_create_lti…

3ac8dd3

…_1p3_passport

fix: copy-paste bug when both public key and keyset url is specified

fb89f25

fix: lint issues

ce8ab7c

test: improve coverage

84c2e78

refactor(views): remove unnecessary db call

1057e62

fix: create name only if block is available

d96b929

fix: test

8c8df22

refactor: migration

a6f3807

chore: upgrade

17938cc

refactor: avoid duplicate signal triggers

8096344

refactor: api

2a2b337

navinkarkera added 10 commits April 13, 2026 16:52

refactor: rename

901d76b

fix: tests

114e208

chore: fix lint issues

d9af4dd

test: add some more

dfffc98

fix: tests

df6c7fe

fix: coverage issue

6bed593

chore: upgrade

aa034e6

fix: handle duplicate block explicitly

6269d91

fix: upgrade conflicts

01fb5bf

refactor: robust duplicate signal handler

c2c7ec7

navinkarkera force-pushed the navin/FAL-4346/bug-fix branch from 0f1d34f to 48a5afb Compare April 13, 2026 15:10

navinkarkera added 2 commits April 14, 2026 15:50

fix: migration for missing location field in configurations

3a41150

Adds tests covering possible cases

fix: lint issues

4831f17

navinkarkera force-pushed the navin/FAL-4346/bug-fix branch from 48a5afb to 64d301b Compare April 14, 2026 11:06

navinkarkera added 2 commits April 16, 2026 15:56

refactor: remove logic that update block fields in migration

cf4bd33

refactor: add passport id to xml

1ca3e5f

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Apr 16, 2026

navinkarkera added 4 commits April 17, 2026 12:41

fix: ags result endpoint to work with or without ending slash

9391906

Fixes: openedx#628

fix: allow blank comment in scores api endpoint

1234199

Fixes: openedx#637

fix: use correct deep linking launch uri as target_link_uri

d5dbe3f

fix: lint issues

f1471d0

navinkarkera force-pushed the navin/FAL-4346/bug-fix branch from 64d301b to f1471d0 Compare April 17, 2026 07:11

fix: use correct key-secret pair for lti 1.1

66c31f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: deep linking target_link_uri and AGS endpoint quirks#643

fix: deep linking target_link_uri and AGS endpoint quirks#643
navinkarkera wants to merge 43 commits intoopenedx:masterfrom
open-craft:navin/FAL-4346/bug-fix

navinkarkera commented Apr 12, 2026 •

edited

Loading

Uh oh!

openedx-webhooks commented Apr 12, 2026

Uh oh!

codecov bot commented Apr 12, 2026 •

edited

Loading

Uh oh!

rpenido commented Apr 13, 2026

Uh oh!

navinkarkera commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

navinkarkera commented Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Apr 12, 2026

Uh oh!

codecov bot commented Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rpenido commented Apr 13, 2026

Uh oh!

navinkarkera commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

navinkarkera commented Apr 12, 2026 •

edited

Loading

codecov bot commented Apr 12, 2026 •

edited

Loading