Create endpoint for public LTI JWKs #2235
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
config/routes.rb (1)
Line range hint
1-214: Consider namespacing LTI routes in the future.While the current implementation is correct and consistent, as you continue to expand LTI functionality, it might be beneficial to consider grouping all LTI-related routes under a namespace in the future. This could improve organization and maintainability as the number of LTI routes grows. For example:
namespace :lti do get 'launch/jwks', to: "launch#jwks" post 'launch/oidc_login', to: "launch#oidc_login" get 'launch/oidc_login', to: "launch#oidc_login" post 'launch/launch', to: "launch#launch" get 'launch/launch', to: "launch#launch" post 'nrps/sync_roster', to: "nrps#sync_roster" endThis is not a critical change for the current PR but could be considered for future refactoring.
app/controllers/lti_launch_controller.rb (1)
239-262: LGTM with suggestions for improvement.The
jwksmethod successfully implements the core functionality of returning public JWKs for LTI authentication. It includes proper error handling and logging, which is commendable.Consider the following improvements:
- Replace
File.size?withFile.exist?for checking file existence:-unless File.size?("#{Rails.configuration.config_location}/lti_tool_jwk.json") +unless File.exist?("#{Rails.configuration.config_location}/lti_tool_jwk.json")
- Implement caching to improve performance for frequent requests:
def jwks Rails.cache.fetch("lti_jwks", expires_in: 1.hour) do # Existing implementation here end end
- Consider moving the file path to a constant or configuration variable for easier maintenance:
LTI_TOOL_JWK_PATH = "#{Rails.configuration.config_location}/lti_tool_jwk.json"These changes will enhance the method's performance and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/controllers/lti_launch_controller.rb (2 hunks)
- config/routes.rb (1 hunks)
🔇 Additional comments (4)
config/routes.rb (2)
9-9: LGTM: New route for LTI JWKs endpoint added correctly.The new route
get 'lti_launch/jwks', to: "lti_launch#jwks"is well-placed and correctly implemented. It aligns perfectly with the PR objective of creating an endpoint for public LTI JWKs. The route follows RESTful conventions, maintains consistency with existing LTI routes, and adheres to Rails naming conventions.
Line range hint
1-214: New route integrates well with existing structure.The addition of the new route for LTI JWKs is well-integrated into the existing routing structure. It maintains consistency with other LTI-related routes and doesn't disrupt the overall organization of the routes file. This minimal impact approach is commendable as it reduces the risk of unintended side effects on other parts of the application.
app/controllers/lti_launch_controller.rb (2)
Line range hint
1-362: Overall, the changes successfully implement the PR objectives.The modifications to the
LtiLaunchControllereffectively introduce the new public endpoint for JWKs and adjust the authentication flow as required. The implementation includes proper error handling and logging, which is crucial for maintainability and debugging.A few minor suggestions have been made to improve performance and adhere to Ruby best practices. Once these are addressed, the code will be in excellent shape.
Don't forget to update the documentation as mentioned in the PR description, particularly regarding the changes to the Canvas flow and other LTI-related modifications.
7-7: LGTM, but consider security implications.The addition of
skip_before_action :authenticate_user!for thelaunch,oidc_login, andjwksactions aligns with the PR objectives. It allows external LTI providers to access the public JWKs without authentication.However, to ensure security, please verify that these actions don't expose any sensitive information or operations. Consider implementing rate limiting or other security measures to prevent potential abuse of these endpoints.
✅ Verification successful
Security implications verified
The
launch,oidc_login, andjwksactions handle errors appropriately without exposing sensitive information. The use of genericLtiErrormessages ensures that internal details remain concealed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any sensitive information or operations in the specified actions # Test: Search for potential sensitive operations in the launch, oidc_login, and jwks methods rg --type ruby -A 10 -B 10 'def (launch|oidc_login|jwks)' app/controllers/lti_launch_controller.rbLength of output: 2781
Description
Create a way for other LTI providers to discover LTI's public JWKs so that they don't need to be manually uploaded to the tool provider, which simplifies setup and makes switching out the JWT convenient.
documentation to be updated after we update the canvas flow / other LTI changes.
Motivation and Context
Part of upgrades to support better LTI integration with Canvas. This should also make setting up Autolab on Canvas easier.
There is now a public endpoint
/lti_launch/jwksto support querying Autolab for its public JWKs.How Has This Been Tested?
This testing was done on Nightly since it seems that the LTI testing tool really doesn't like http requests for making a request to get the public JWK.
Use the following platform: https://lti-ri.imsglobal.org/platforms/5276
Verify that platform has correct JWK url:
Use the following configuration:
Upload whatever JWK keypair you want on nightly. Then go to the following link:
https://nightly.autolabproject.com/lti_launch/jwks
See that you see the public JWK (not the private one):
Also see that you can be logged out and still view the JWK (as it should be public-facing)
Then try synchronizing the course roster here (for now the LTI link is here): https://nightly.autolabproject.com/courses/hackingsession-david/users
See that link succeeds
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for linting