diff --git a/Changelog.md b/Changelog.md index d48cae06f5..a4ef12cae8 100644 --- a/Changelog.md +++ b/Changelog.md @@ -33,6 +33,7 @@ - Fixed the "Fix" link being clipped off the screen for long QR-scan error messages on the exam scan log table (#8070) - Fixed Assign Scans returning a raw 404 instead of redirecting back to the Groups page once all groups have been assigned (#8059) - Allow new instructors without existing courses to create a course via LTI launch (#8061) +- Enforce privileged LTI launch role for course linking and creation- (#8130) ### 📚 Documentation changes - Made documentation site table-of-contents/section listings consistent (#8062) diff --git a/app/controllers/lti_deployments_controller.rb b/app/controllers/lti_deployments_controller.rb index 5125a4b1bd..584fd141cd 100644 --- a/app/controllers/lti_deployments_controller.rb +++ b/app/controllers/lti_deployments_controller.rb @@ -1,11 +1,12 @@ class LtiDeploymentsController < ApplicationController skip_verify_authorized except: [:choose_course] - skip_forgery_protection except: [:choose_course] + skip_forgery_protection except: [:choose_course, :create_course] before_action :authenticate, :check_course_switch, :check_record, except: [:get_config, :launch, :public_jwk, :redirect_login] before_action(except: [:get_config, :launch, :public_jwk, :redirect_login]) { authorize! } before_action :check_host, only: [:launch, :redirect_login] + before_action :check_lti_launch_role, only: [:choose_course, :create_course] USE_SECURE_COOKIES = !Rails.env.local? @@ -151,6 +152,7 @@ def redirect_login has_privileged_role = lti_data[:user_roles].intersect?(LtiDeployment::LTI_PRIVILEGED_ROLES) has_ta_role = lti_data[:user_roles].include?(LtiDeployment::LTI_ROLES[:ta]) if has_privileged_role && !has_ta_role + session[:lti_privileged_deployments] = Array(session[:lti_privileged_deployments]) | [lti_deployment.id] redirect_to choose_course_lti_deployment_path(lti_deployment) else redirect_to course_not_set_up_lti_deployment_path(lti_deployment) @@ -254,4 +256,15 @@ def construct_redirect_with_port(url, endpoint: nil) referer_host = referer_host_with_port if referer.to_s.start_with?(referer_host_with_port) URI("#{referer_host}#{endpoint}") end + + # Only allow linking or creating a course for LTI deployments that the current + # user launched from the LMS with a privileged (non-TA) role. The launch role + # is recorded in the session by #redirect_login. + def check_lti_launch_role + return if Array(session[:lti_privileged_deployments]).include?(record&.id) + + @title = I18n.t('lti.launch_required_title') + @message = I18n.t('lti.launch_required') + render 'message', status: :forbidden + end end diff --git a/config/locales/common/en.yml b/config/locales/common/en.yml index b6944f41cb..943fef0202 100644 --- a/config/locales/common/en.yml +++ b/config/locales/common/en.yml @@ -38,6 +38,8 @@ en: grade_sync_complete: Grade Sync Complete grade_sync_instructions: This will only synchronize grades that have been released. launch: Launch MarkUs + launch_required: To link or create a course, please launch MarkUs from your LMS course. + launch_required_title: Launch Required line_item_created: Gradebook item created. lti_configuration: LTI configuration lti_course_link_html: Course linked to external course diff --git a/spec/controllers/lti_deployments_controller_spec.rb b/spec/controllers/lti_deployments_controller_spec.rb index 391ad43a92..8da4a364e7 100644 --- a/spec/controllers/lti_deployments_controller_spec.rb +++ b/spec/controllers/lti_deployments_controller_spec.rb @@ -14,6 +14,10 @@ let(:test_rlid) { 'a-unique-resource-link-id-12345' } let!(:lti) { create(:lti_deployment, resource_link_id: test_rlid) } + before do + session[:lti_privileged_deployments] = [lti.id] + end + describe 'get' do it 'is inaccessible unless logged in' do get :choose_course, params: { id: lti.id } @@ -21,17 +25,28 @@ end it 'is accessible when logged in' do - session[:lti_deployment_id] = lti.id get_as instructor, :choose_course, params: { id: lti.id } expect(response).to have_http_status(:ok) end - end - describe 'post' do - before do - session[:lti_deployment_id] = lti.id + context 'when the user did not launch this deployment with a privileged LMS role' do + before do + session[:lti_privileged_deployments] = [] + end + + it 'responds with forbidden' do + get_as instructor, :choose_course, params: { id: lti.id } + expect(response).to have_http_status(:forbidden) + end + + it 'renders the launch required message' do + get_as instructor, :choose_course, params: { id: lti.id } + expect(response).to render_template('message') + end end + end + describe 'post' do context 'when picking a course' do it 'redirects to a course on success' do post_as instructor, :choose_course, params: { id: lti.id, course: course.id } @@ -55,6 +70,23 @@ expect(lti.resource_link_id).to eq(test_rlid) end + context 'when the user did not launch this deployment with a privileged LMS role' do + before do + session[:lti_privileged_deployments] = [] + end + + it 'does not link the course' do + post_as instructor, :choose_course, params: { id: lti.id, course: course.id } + lti.reload + expect(lti.course).to be_nil + end + + it 'responds with forbidden' do + post_as instructor, :choose_course, params: { id: lti.id, course: course.id } + expect(response).to have_http_status(:forbidden) + end + end + context 'when the user does not have permission to link' do let(:course2) { create(:course) } let(:instructor2) { create(:instructor, course: course2) } @@ -79,9 +111,12 @@ { id: lti_deployment.id, display_name: 'Introduction to Computer Science', name: lti_deployment.lms_course_name } end + before do + session[:lti_privileged_deployments] = [lti_deployment.id] + end + context 'as an instructor with a standard term' do before do - session[:lti_deployment_id] = lti_deployment.id post_as instructor, :create_course, params: course_params end @@ -118,10 +153,6 @@ create(:lti_deployment, lms_term_name: 'Default Term', lms_course_name: 'csc108') end - before do - session[:lti_deployment_id] = lti_deployment.id - end - it 'slugifies the term name as the suffix' do post_as instructor, :create_course, params: { id: lti_deployment.id, @@ -134,7 +165,6 @@ context 'as an admin user' do before do - session[:lti_deployment_id] = lti_deployment.id post_as admin_user, :create_course, params: course_params end @@ -155,7 +185,6 @@ context 'when a course already exists' do before do create(:course, name: expected_name) - session[:lti_deployment_id] = lti_deployment.id end it 'does not create a new course' do @@ -174,10 +203,6 @@ create(:lti_deployment, lms_course_name: 'csc108 fall 3000!', lms_term_name: 'Fall 2026') end - before do - session[:lti_deployment_id] = lti_deployment.id - end - it 'creates a new course with a sanitized name and appends suffix' do post_as instructor, :create_course, params: course_params expect(Course.exists?(name: 'CSC108-FALL-3000-20269')).not_to be_nil @@ -188,10 +213,6 @@ # NOTE: the default filter in config/dummy_lti_config.rb only accepts course names starting with 'csc' let!(:lti_deployment) { create(:lti_deployment, lms_course_name: 'sta130', lms_term_name: 'Fall 2026') } - before do - session[:lti_deployment_id] = lti_deployment.id - end - it 'does not create a new course' do post_as instructor, :create_course, params: course_params expect(Course.count).to eq(1) @@ -219,6 +240,44 @@ expect(Role.find_by(user: new_instructor_user, course: course, type: 'Instructor')).not_to be_nil end end + + context 'when the user did not launch this deployment with a privileged LMS role' do + before do + session[:lti_privileged_deployments] = [] + end + + it 'does not create a course' do + post_as instructor, :create_course, params: course_params + expect(Course.find_by(name: expected_name)).to be_nil + end + + it 'responds with forbidden' do + post_as instructor, :create_course, params: course_params + expect(response).to have_http_status(:forbidden) + end + + it 'renders the launch required message' do + post_as instructor, :create_course, params: course_params + expect(response).to render_template('message') + end + end + + context 'when the deployment is already linked to a course' do + let(:linked_course) { create(:course) } + + before do + lti_deployment.update!(course: linked_course) + post_as instructor, :create_course, params: course_params + end + + it 'responds with not found' do + expect(response).to have_http_status(:not_found) + end + + it 'does not create a new course' do + expect(Course.find_by(name: expected_name)).to be_nil + end + end end describe '#public_jwk' do