Skip to content

SAK-52543 LTI 1.3: Declare supported scopes - #14591

Merged
csev merged 1 commit into
sakaiproject:masterfrom
hornersa:SAK-52543
May 24, 2026
Merged

SAK-52543 LTI 1.3: Declare supported scopes#14591
csev merged 1 commit into
sakaiproject:masterfrom
hornersa:SAK-52543

Conversation

@hornersa

@hornersa hornersa commented May 13, 2026

Copy link
Copy Markdown
Contributor

Jira: https://sakaiproject.atlassian.net/browse/SAK-52543

While the set of scopes supported is certainly changeable, I simply assembled the set that already had static fields defined in the constants class and added this string array to the only place in the lti code where I observed that scopes_supported was defined. In testing this, the External Tool checkboxes during dynamic registration for services were checked, urls exchanged, etc.

Summary by CodeRabbit

  • Refactor
    • Centralized LTI scope definitions for consistency across the platform.
    • OpenID provider defaults now advertise the full aggregated set of supported scopes by default.
  • Tests
    • Updated provider serialization tests to expect the expanded list of advertised scopes.

Review Change Stack

@hornersa
hornersa requested a review from csev May 13, 2026 22:46
@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 832ecb04-f53e-4050-8499-54793667df77

📥 Commits

Reviewing files that changed from the base of the PR and between f6fa57f and 34aaf92.

📒 Files selected for processing (3)
  • lti/tsugi-util/src/java/org/tsugi/lti13/LTI13ConstantsUtil.java
  • lti/tsugi-util/src/java/org/tsugi/lti13/objects/OpenIDProviderConfiguration.java
  • lti/tsugi-util/src/test/org/tsugi/lti13/LTI13ObjectTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
  • lti/tsugi-util/src/java/org/tsugi/lti13/objects/OpenIDProviderConfiguration.java
  • lti/tsugi-util/src/java/org/tsugi/lti13/LTI13ConstantsUtil.java

Walkthrough

Adds a public List constant SCOPES_SUPPORTED in LTI13ConstantsUtil aggregating LTI scope URIs; OpenIDProviderConfiguration constructor now adds those scopes into its scopes_supported list. Tests updated to expect the expanded scopes_supported array.

Changes

LTI 1.3 Scope Constants Consolidation

Layer / File(s) Summary
Define SCOPES_SUPPORTED constant
lti/tsugi-util/src/java/org/tsugi/lti13/LTI13ConstantsUtil.java
Adds import java.util.List and a public static final List<String> SCOPES_SUPPORTED = List.of(...) aggregating existing scope URI constants.
Use SCOPES_SUPPORTED in provider defaults and update test
lti/tsugi-util/src/java/org/tsugi/lti13/objects/OpenIDProviderConfiguration.java, lti/tsugi-util/src/test/org/tsugi/lti13/LTI13ObjectTest.java
Constructor calls scopes_supported.addAll(LTI13ConstantsUtil.SCOPES_SUPPORTED) before adding "openid" and other defaults; testTwo expected JSON updated to include the expanded scopes_supported array.

Suggested reviewers

  • csev
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically describes the main change: declaring supported scopes in LTI 1.3 by adding a SCOPES_SUPPORTED constant and integrating it into the OpenIDProviderConfiguration.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lti/tsugi-util/src/java/org/tsugi/lti13/LTI13ConstantsUtil.java`:
- Line 97: Replace the public mutable array SUPPORTED_SCOPES in
LTI13ConstantsUtil with an immutable List to prevent callers from modifying
internal state; change the declaration from "public static final String[]
SUPPORTED_SCOPES" to a "public static final List<String> SUPPORTED_SCOPES =
List.of(...)" using the same scope constants (SCOPE_RESULT_READONLY,
SCOPE_SCORE, SCOPE_LINEITEM, SCOPE_LINEITEM_READONLY, SCOPE_NAMES_AND_ROLES,
SCOPE_CONTEXTGROUP_READONLY), and update any code that references the array to
use the List API or convert to an array locally if needed.

In
`@lti/tsugi-util/src/java/org/tsugi/lti13/objects/OpenIDProviderConfiguration.java`:
- Line 4: OpenIDProviderConfiguration currently uses Collections.addAll(...)
with LTI13ConstantsUtil.SUPPORTED_SCOPES (an array); if SUPPORTED_SCOPES becomes
a List<String> remove the now-unneeded import of java.util.Collections and
change the call using Collections.addAll(...) to use the List.addAll(...) form
(e.g., scopes.addAll(LTI13ConstantsUtil.SUPPORTED_SCOPES)); update the reference
in OpenIDProviderConfiguration so it calls addAll on the target List instead of
Collections.addAll.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fd835f96-3139-4ca1-bae2-f842f4b16c50

📥 Commits

Reviewing files that changed from the base of the PR and between 3ddf100 and 7b93813.

📒 Files selected for processing (2)
  • lti/tsugi-util/src/java/org/tsugi/lti13/LTI13ConstantsUtil.java
  • lti/tsugi-util/src/java/org/tsugi/lti13/objects/OpenIDProviderConfiguration.java

Comment thread lti/tsugi-util/src/java/org/tsugi/lti13/LTI13ConstantsUtil.java Outdated
Comment thread lti/tsugi-util/src/java/org/tsugi/lti13/objects/OpenIDProviderConfiguration.java Outdated
@hornersa
hornersa marked this pull request as ready for review May 13, 2026 23:26
@csev

csev commented May 13, 2026

Copy link
Copy Markdown
Contributor

So @hornersa - is it the case we didn't advertise the scopes? If this is the case - that is so cool to catch it.

@csev

csev commented May 13, 2026

Copy link
Copy Markdown
Contributor

@hornersa It looks like unit tests are breaking..

@hornersa

Copy link
Copy Markdown
Contributor Author

@hornersa It looks like unit tests are breaking..

Oops. I'll take a look tomorrow.

@hornersa
hornersa marked this pull request as draft May 13, 2026 23:59
@hornersa

Copy link
Copy Markdown
Contributor Author

@hornersa It looks like unit tests are breaking..

Sorry about that. Looks like one test needed fixing which I've since force pushed. (Apparently my zeal in running maven w/ the skip tests options for expediting build/deploy times came back to bite me in this instance.)

@ottenhoff

Copy link
Copy Markdown
Contributor

@coderabbitai why are we advertising LTI 1.3 scopes in OpenIDProviderConfiguration?

@coderabbitai

coderabbitai Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@ottenhoff Great question! Here's the rationale:

Sakai is the LTI Platform (OpenID Provider) in this context, not the tool. OpenIDProviderConfiguration is the OIDC Discovery document that Sakai exposes so that LTI tools can learn about the platform's capabilities during LTI Dynamic Registration.

The OpenID Connect Discovery spec defines scopes_supported as a JSON array containing the OAuth 2.0 scope values that the server supports, and the server MUST support the openid scope value. Beyond openid, the lti-platform-configuration exposes the current LTI capabilities of the platform and may be used by the Tool to tailor its registration process.

In the LTI Dynamic Registration flow, scopes_supported lists the supported services available on the instance. When a tool reads this document, it uses scopes_supported to know which LTI Advantage services (Assignment & Grade Services, Names & Roles Provisioning, etc.) the platform offers. The tool then sends a scope (comma-separated list of scopes it would like enabled), which drives which services are made available in the tool's registration.

Before this PR, Sakai's OpenIDProviderConfiguration only advertised openid — it was missing all the LTI-specific scope URIs. This meant tools performing dynamic registration had no way to know Sakai supported services like AGS or NRPS, so service checkboxes wouldn't be pre-populated. As the PR description and @hornersa confirmed in testing, adding these scopes caused "External Tool checkboxes during dynamic registration for services to be checked and allowed URLs to be exchanged."

For comparison, Canvas's openid_configuration already includes scopes_supported with entries like https://purl.imsglobal.org/spec/lti-ags/scope/lineitem, https://purl.imsglobal.org/spec/lti-ags/scope/result.readonly, https://purl.imsglobal.org/spec/lti-nrps/scope/contextmembership.readonly, etc. — this PR brings Sakai in line with that behavior.

@hornersa
hornersa marked this pull request as ready for review May 14, 2026 16:13
@csev
csev merged commit dccbfa5 into sakaiproject:master May 24, 2026
4 of 5 checks passed
ern pushed a commit that referenced this pull request May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants