From 6b8023596a44ca8aab050deb51cb000767fe0d83 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Thu, 9 Nov 2023 13:22:25 +0100 Subject: [PATCH 1/5] Revert "Revert "Setup local reproduction"" This reverts commit a09090a9224f4653e6f4583e315b668431643452. --- app/controllers/concerns/set_lti_message.rb | 33 ++++++++++++++++++++- config/database.yml | 2 +- db/schema.rb | 2 +- 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/app/controllers/concerns/set_lti_message.rb b/app/controllers/concerns/set_lti_message.rb index 817d6f2fdf..78f606ecc1 100644 --- a/app/controllers/concerns/set_lti_message.rb +++ b/app/controllers/concerns/set_lti_message.rb @@ -8,7 +8,38 @@ module SetLtiMessage include LTI::Messages def set_lti_message - @lti_message = parse_message(params[:id_token], params[:provider_id]) + # @lti_message = parse_message(params[:id_token], params[:provider_id]) + provider = Provider::Lti.find(params[:provider_id]) + payload = { + iss: provider.issuer, + aud: provider.client_id, + exp: Time.now.to_i + 600, + iat: Time.now.to_i, + sub: 'test-user-123', + nonce: 'nonce', + 'https://purl.imsglobal.org/spec/lti/claim/message_type': 'target', + 'https://purl.imsglobal.org/spec/lti/claim/version': '1.3.0', + 'https://purl.imsglobal.org/spec/lti/claim/deployment_id': 'c5899818-7062-44d1-b377-5a08097daeb3', + 'https://purl.imsglobal.org/spec/lti/claim/target_link_uri': 'LtiDeepLinkingRequest', + 'https://purl.imsglobal.org/spec/lti/claim/resource_link': { + id: '5B0748E6-E75C-4A93-8875-E034639B31CD-513799_107172', + title: 'Oef 3-2 - vierkantsvergelijking', + description: nil + }, + 'https://purl.imsglobal.org/spec/lti-dl/claim/deep_linking_settings': { + accept_types: %w[link file html ltiResourceLink image], + accept_media_types: 'image/*,text/html', + accept_presentation_document_targets: %w[iframe window embed], + accept_multiple: true, + auto_create: true, + title: 'This is the default title', + text: 'This is the default text', + data: 'Some random opaque data that MUST be sent back', + deep_link_return_url: 'https://www.example.com/deep_links' + } + }.with_indifferent_access + @lti_message = ::LTI::Messages::Types::DeepLinkingRequest.new(payload) + @lti_launch = @lti_message.is_a?(LTI::Messages::Types::ResourceLaunchRequest) helpers.locale = @lti_message&.launch_presentation_locale if @lti_message&.launch_presentation_locale.present? rescue JSON::JWK::Set::KidNotFound => _e diff --git a/config/database.yml b/config/database.yml index 85bd914a3a..0e579d3b9b 100644 --- a/config/database.yml +++ b/config/database.yml @@ -19,7 +19,7 @@ default: &default development: <<: *default - database: dodona + database: dodona_backup staging: <<: *default diff --git a/db/schema.rb b/db/schema.rb index c7a60a2e37..508dc2f4b4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -105,8 +105,8 @@ t.integer "series_id_non_nil", null: false t.index ["accepted", "user_id", "series_id"], name: "index_activity_statuses_on_accepted_and_user_id_and_series_id" t.index ["activity_id"], name: "index_activity_statuses_on_activity_id" + t.index ["series_id", "started", "user_id", "last_submission_id"], name: "index_as_on_series_and_started_and_user_and_last_submission" t.index ["series_id"], name: "fk_rails_1bc42c2178" - t.index ["started", "user_id", "last_submission_id"], name: "index_as_on_started_and_user_and_last_submission" t.index ["started", "user_id", "series_id"], name: "index_activity_statuses_on_started_and_user_id_and_series_id" t.index ["user_id", "series_id", "last_submission_id"], name: "index_as_on_user_and_series_and_last_submission" t.index ["user_id", "series_id_non_nil", "activity_id"], name: "index_on_user_id_series_id_non_nil_activity_id", unique: true From 1184a31376184fa949c74ed62d7079ca9c3d5316 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Thu, 9 Nov 2023 13:33:29 +0100 Subject: [PATCH 2/5] Revert "Revert "Revert "Setup local reproduction""" This reverts commit 6b8023596a44ca8aab050deb51cb000767fe0d83. --- app/controllers/concerns/set_lti_message.rb | 33 +-------------------- config/database.yml | 2 +- db/schema.rb | 2 +- 3 files changed, 3 insertions(+), 34 deletions(-) diff --git a/app/controllers/concerns/set_lti_message.rb b/app/controllers/concerns/set_lti_message.rb index 78f606ecc1..817d6f2fdf 100644 --- a/app/controllers/concerns/set_lti_message.rb +++ b/app/controllers/concerns/set_lti_message.rb @@ -8,38 +8,7 @@ module SetLtiMessage include LTI::Messages def set_lti_message - # @lti_message = parse_message(params[:id_token], params[:provider_id]) - provider = Provider::Lti.find(params[:provider_id]) - payload = { - iss: provider.issuer, - aud: provider.client_id, - exp: Time.now.to_i + 600, - iat: Time.now.to_i, - sub: 'test-user-123', - nonce: 'nonce', - 'https://purl.imsglobal.org/spec/lti/claim/message_type': 'target', - 'https://purl.imsglobal.org/spec/lti/claim/version': '1.3.0', - 'https://purl.imsglobal.org/spec/lti/claim/deployment_id': 'c5899818-7062-44d1-b377-5a08097daeb3', - 'https://purl.imsglobal.org/spec/lti/claim/target_link_uri': 'LtiDeepLinkingRequest', - 'https://purl.imsglobal.org/spec/lti/claim/resource_link': { - id: '5B0748E6-E75C-4A93-8875-E034639B31CD-513799_107172', - title: 'Oef 3-2 - vierkantsvergelijking', - description: nil - }, - 'https://purl.imsglobal.org/spec/lti-dl/claim/deep_linking_settings': { - accept_types: %w[link file html ltiResourceLink image], - accept_media_types: 'image/*,text/html', - accept_presentation_document_targets: %w[iframe window embed], - accept_multiple: true, - auto_create: true, - title: 'This is the default title', - text: 'This is the default text', - data: 'Some random opaque data that MUST be sent back', - deep_link_return_url: 'https://www.example.com/deep_links' - } - }.with_indifferent_access - @lti_message = ::LTI::Messages::Types::DeepLinkingRequest.new(payload) - + @lti_message = parse_message(params[:id_token], params[:provider_id]) @lti_launch = @lti_message.is_a?(LTI::Messages::Types::ResourceLaunchRequest) helpers.locale = @lti_message&.launch_presentation_locale if @lti_message&.launch_presentation_locale.present? rescue JSON::JWK::Set::KidNotFound => _e diff --git a/config/database.yml b/config/database.yml index 0e579d3b9b..85bd914a3a 100644 --- a/config/database.yml +++ b/config/database.yml @@ -19,7 +19,7 @@ default: &default development: <<: *default - database: dodona_backup + database: dodona staging: <<: *default diff --git a/db/schema.rb b/db/schema.rb index 508dc2f4b4..c7a60a2e37 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -105,8 +105,8 @@ t.integer "series_id_non_nil", null: false t.index ["accepted", "user_id", "series_id"], name: "index_activity_statuses_on_accepted_and_user_id_and_series_id" t.index ["activity_id"], name: "index_activity_statuses_on_activity_id" - t.index ["series_id", "started", "user_id", "last_submission_id"], name: "index_as_on_series_and_started_and_user_and_last_submission" t.index ["series_id"], name: "fk_rails_1bc42c2178" + t.index ["started", "user_id", "last_submission_id"], name: "index_as_on_started_and_user_and_last_submission" t.index ["started", "user_id", "series_id"], name: "index_activity_statuses_on_started_and_user_id_and_series_id" t.index ["user_id", "series_id", "last_submission_id"], name: "index_as_on_user_and_series_and_last_submission" t.index ["user_id", "series_id_non_nil", "activity_id"], name: "index_on_user_id_series_id_non_nil_activity_id", unique: true From 00f760dc93f70f0217c196a9fe952b481d17f6ef Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Thu, 9 Nov 2023 13:44:28 +0100 Subject: [PATCH 3/5] Only return admistarting courses --- app/controllers/lti_controller.rb | 2 +- app/policies/lti_policy.rb | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 app/policies/lti_policy.rb diff --git a/app/controllers/lti_controller.rb b/app/controllers/lti_controller.rb index e7b3fb9daf..0501680f35 100644 --- a/app/controllers/lti_controller.rb +++ b/app/controllers/lti_controller.rb @@ -37,7 +37,7 @@ def do_redirect def content_selection @supported = @lti_message.accept_types.include?(LTI::Messages::Types::DeepLinkingResponse::LtiResourceLink::TYPE) - @grouped_courses = policy_scope(Course.all).group_by(&:year) + @grouped_courses = current_user.administrating_courses.group_by(&:year) @multiple = @lti_message.accept_multiple end diff --git a/app/policies/lti_policy.rb b/app/policies/lti_policy.rb new file mode 100644 index 0000000000..2d4cd9821d --- /dev/null +++ b/app/policies/lti_policy.rb @@ -0,0 +1,23 @@ +class LabelPolicy < ApplicationPolicy + class Scope < Scope + def resolve + scope + end + end + + def content_selection? + current_user&.a_course_admin? + end + + def redirect? + true + end + + def do_redirect? + true + end + + def series_and_activities? + Course.find_by(id: params[:id]).each { |c| current_user&.admin_of?(c) } + end +end From 3ac28cb3e5ec0d5ddb8c3479343a5bc91c1cc9aa Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Thu, 9 Nov 2023 14:15:07 +0100 Subject: [PATCH 4/5] Limit content selection to admistrating courses --- app/controllers/lti_controller.rb | 5 ++++ app/policies/lti_policy.rb | 23 --------------- test/controllers/lti_controller_test.rb | 39 +++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 23 deletions(-) delete mode 100644 app/policies/lti_policy.rb diff --git a/app/controllers/lti_controller.rb b/app/controllers/lti_controller.rb index 0501680f35..32460f37b1 100644 --- a/app/controllers/lti_controller.rb +++ b/app/controllers/lti_controller.rb @@ -36,6 +36,8 @@ def do_redirect end def content_selection + return head :unauthorized unless current_user&.a_course_admin? + @supported = @lti_message.accept_types.include?(LTI::Messages::Types::DeepLinkingResponse::LtiResourceLink::TYPE) @grouped_courses = current_user.administrating_courses.group_by(&:year) @multiple = @lti_message.accept_multiple @@ -44,6 +46,9 @@ def content_selection def series_and_activities # Eager load the activities @course = Course.includes(series: [:activities]).find_by(id: params[:id]) + + return head :unauthorized unless current_user&.admin_of?(@course) + @series = policy_scope(@course.series) @multiple = ActiveModel::Type::Boolean.new.cast(params[:multiple]) end diff --git a/app/policies/lti_policy.rb b/app/policies/lti_policy.rb deleted file mode 100644 index 2d4cd9821d..0000000000 --- a/app/policies/lti_policy.rb +++ /dev/null @@ -1,23 +0,0 @@ -class LabelPolicy < ApplicationPolicy - class Scope < Scope - def resolve - scope - end - end - - def content_selection? - current_user&.a_course_admin? - end - - def redirect? - true - end - - def do_redirect? - true - end - - def series_and_activities? - Course.find_by(id: params[:id]).each { |c| current_user&.admin_of?(c) } - end -end diff --git a/test/controllers/lti_controller_test.rb b/test/controllers/lti_controller_test.rb index 9cda2d1bc3..e5f23a277e 100644 --- a/test/controllers/lti_controller_test.rb +++ b/test/controllers/lti_controller_test.rb @@ -18,10 +18,14 @@ def setup end test 'content selection shows courses' do + user = create :staff courses = create_list :course, 2 + user.administrating_courses << courses payload = lti_payload('nonce', 'target', 'LtiDeepLinkingRequest') id_token = encode_jwt(payload) + sign_in user + get content_selection_path, params: { id_token: id_token, provider_id: @provider.id @@ -33,6 +37,41 @@ def setup end end + test 'content selection should not show courses that are not administrated by the user' do + user = create :staff + courses = create_list :course, 2 + payload = lti_payload('nonce', 'target', 'LtiDeepLinkingRequest') + id_token = encode_jwt(payload) + + sign_in user + + get content_selection_path, params: { + id_token: id_token, + provider_id: @provider.id + } + + assert_response :ok + courses.each do |course| + assert_select 'option', { text: course.name, count: 0 } + end + end + + test 'content selection should return unauthorized if the user is not administrating any courses' do + user = create :student + courses = create_list :course, 2 + payload = lti_payload('nonce', 'target', 'LtiDeepLinkingRequest') + id_token = encode_jwt(payload) + + sign_in user + + get content_selection_path, params: { + id_token: id_token, + provider_id: @provider.id + } + + assert_response :unauthorized + end + test 'content selection payload is correct' do series = create :series, exercise_count: 1 payload = lti_payload('nonce', 'target', 'LtiDeepLinkingRequest') From 3b619e67ef3428d7232b0692c7a2021570348218 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Thu, 9 Nov 2023 14:18:02 +0100 Subject: [PATCH 5/5] fix linting --- test/controllers/lti_controller_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/lti_controller_test.rb b/test/controllers/lti_controller_test.rb index e5f23a277e..057ea44152 100644 --- a/test/controllers/lti_controller_test.rb +++ b/test/controllers/lti_controller_test.rb @@ -58,7 +58,7 @@ def setup test 'content selection should return unauthorized if the user is not administrating any courses' do user = create :student - courses = create_list :course, 2 + create_list :course, 2 payload = lti_payload('nonce', 'target', 'LtiDeepLinkingRequest') id_token = encode_jwt(payload)