Fix NoMethodError in lti_launch_link_course action#2308
Fix NoMethodError in lti_launch_link_course action#2308dpaceoffice wants to merge 1 commit intoautolab:masterfrom
Conversation
The lti_launch_link_course action was incorrectly included in the authenticate_for_action exception list, causing authenticate_for_action to run. However, since UsersController skips both set_course and authorize_user_for_course, the @CUD instance variable is never set, resulting in 'undefined method has_auth_level? for nil:NilClass'. This fix: 1. Removes lti_launch_link_course from the authenticate_for_action exception list (so it gets skipped like other actions) 2. Adds inline authorization check within the action to verify the current user is an instructor for the target course This maintains security while fixing the nil reference error. Fixes: NoMethodError in authenticate_for_action when linking LTI course
📝 WalkthroughWalkthroughSecurity enhancement to the lti_launch_link_course endpoint requiring user authentication and verifying instructor permissions before creating course LTI data. Uses the loaded course object instead of re-fetching from parameters. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (1)
app/controllers/users_controller.rb (1)
340-349: Consider verifyingLtiCourseDatum.createsucceeded before showing success message.
createreturns the object regardless of whether it was persisted. If validation or a unique constraint fails (e.g., duplicatecontext_id), the user sees a success flash while the record wasn't actually created.♻️ Suggested fix to handle create failure
- LtiCourseDatum.create( + lcd = LtiCourseDatum.create( course_id: course.id, context_id: params[:context_id], membership_url: params[:course_memberships_url], platform: params[:platform], last_synced: DateTime.current ) + unless lcd.persisted? + flash[:error] = "Failed to link course: #{lcd.errors.full_messages.join(', ')}" + redirect_to(root_path) && return + end + flash[:success] = "#{course.name} successfully linked." redirect_to(course)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/users_controller.rb
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-09-20T20:46:52.775Z
Learnt from: evanyeyeye
Repo: autolab/Autolab PR: 2209
File: app/controllers/autograders_controller.rb:0-0
Timestamp: 2024-09-20T20:46:52.775Z
Learning: In the `AutogradersController`, ensure that the `download_file` method has `action_auth_level :download_file, :instructor` to restrict access to instructors.
Applied to files:
app/controllers/users_controller.rb
📚 Learning: 2024-06-10T19:38:42.782Z
Learnt from: jlge
Repo: autolab/Autolab PR: 2011
File: app/controllers/assessments_controller.rb:28-29
Timestamp: 2024-06-10T19:38:42.782Z
Learning: `destroy_no_redirect` in `app/controllers/assessments_controller.rb` is an internal method used within the controller and not a public endpoint. It should not be included in the `before_action :set_assessment` exclusion list.
Applied to files:
app/controllers/users_controller.rb
🧬 Code graph analysis (1)
app/controllers/users_controller.rb (1)
app/models/course_user_datum.rb (2)
find_cud_for_course(217-230)has_auth_level?(152-165)
🔇 Additional comments (2)
app/controllers/users_controller.rb (2)
4-5: LGTM!Removing
lti_launch_link_coursefrom the exception list correctly ensuresauthenticate_for_actionis skipped for this action, consistent with other actions in this controller that handle their own authorization. This fixes theNoMethodErrorcaused by@cudbeing nil.
332-338: Authorization check looks correct.The inline authorization properly verifies that
current_user(not@userfrom params) has instructor privileges for the target course before allowing the link operation. Using safe navigation handles the nil case appropriately.
1 participant
Title: Fix NoMethodError in lti_launch_link_course when linking LTI course
Description:
The lti_launch_link_course action was incorrectly included in the authenticate_for_action exception list, causing authenticate_for_action to run. However, since UsersController skips both set_course and authorize_user_for_course, the @CUD instance variable is never set, resulting in 'undefined method has_auth_level? for nil:NilClass'.
This fix:
This maintains security while fixing the nil reference error.
Description
When users attempt to link a Canvas course via LTI at
/users/:id/lti_launch_link_course, a 500 Internal Server Error occurs because@cud.has_auth_level?(:instructor)is called on a nil object inauthenticate_for_action.Motivation and Context
LTI course linking is completely broken in the current codebase. Any user attempting to link a Canvas course to Autolab encounters this crash, preventing LTI integration from working.
How Has This Been Tested?
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther details
This bug was introduced in commit 301689a (October 24, 2024) which added lti_launch_link_course to the authenticate_for_action exception list. This change was part of a PR to add admin checks for password-related actions, but lti_launch_link_course, it seems, was incorrectly included.