SAK-52543 LTI 1.3: Declare supported scopes - #14591
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds 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. ChangesLTI 1.3 Scope Constants Consolidation
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
lti/tsugi-util/src/java/org/tsugi/lti13/LTI13ConstantsUtil.javalti/tsugi-util/src/java/org/tsugi/lti13/objects/OpenIDProviderConfiguration.java
|
So @hornersa - is it the case we didn't advertise the scopes? If this is the case - that is so cool to catch it. |
|
@hornersa It looks like unit tests are breaking.. |
Oops. I'll take a look tomorrow. |
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.) |
|
@coderabbitai why are we advertising LTI 1.3 scopes in OpenIDProviderConfiguration? |
|
Tip For best results, initiate chat on the files or code changes.
Sakai is the LTI Platform (OpenID Provider) in this context, not the tool. The OpenID Connect Discovery spec defines In the LTI Dynamic Registration flow, Before this PR, Sakai's For comparison, Canvas's |
(cherry picked from commit dccbfa5)
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