diff --git a/app/api/authentication_api.rb b/app/api/authentication_api.rb index e81de94d1f..6bcf3c024a 100644 --- a/app/api/authentication_api.rb +++ b/app/api/authentication_api.rb @@ -13,6 +13,7 @@ class AuthenticationApi < Grape::API helpers AuthenticationHelpers helpers AuthorisationHelpers helpers LtiHelper + helpers FederatedIdentityHelper # # Sign in - only mounted if AAF and SAML auth is NOT used (database auth) @@ -79,7 +80,7 @@ class AuthenticationApi < Grape::API token = user.generate_authentication_token! # Return user details - present :user, user, with: Entities::UserEntity + present :user, user, with: Entities::UserEntity, theme_owner_id: user.id present :auth_token, token.authentication_token present :auth_token_expiry, token.auth_token_expiry set_refresh_cookie_in_response(remember) @@ -108,12 +109,11 @@ class AuthenticationApi < Grape::API logger.info "Authenticate #{user_id_data[:email]} from #{request.ip}" - # Lookup using login_id if it exists - # Lookup using email otherwise and set login_id - # Otherwise create new - user = User.find_by(login_id: user_id_data[:login_id]) || - User.find_by(username: user_id_data[:username]) || - User.find_by(email: user_id_data[:email]) || + # Lookup on what the identity provider asserted, otherwise create new + user = user_for_asserted_identity(login_id: user_id_data[:login_id], + email: user_id_data[:email], + derived_username: user_id_data[:username], + source: request.ip) || User.create do |new_user| # Update new user with details from the SAML response Doubtfire::Application.config.institution_settings.update_user_from_saml_response( @@ -154,7 +154,11 @@ class AuthenticationApi < Grape::API protocol = Rails.env.development? ? 'http' : 'https' host = "#{protocol}://#{host}" end - redirect "#{host}/sign_in?authToken=#{onetime_token.authentication_token}&username=#{user.username}" + redirect AuthenticationHelpers.frontend_sign_in_url( + host: host, + auth_token: onetime_token.authentication_token, + username: user.username + ) end # Saml 2 logout callback @@ -215,12 +219,11 @@ class AuthenticationApi < Grape::API logger.info "Authenticate #{user_id_data[:email]} from #{request.ip}" - # Lookup using login_id if it exists - # Lookup using email otherwise and set login_id - # Otherwise create new - user = User.find_by(login_id: user_id_data[:login_id]) || - User.find_by(username: user_id_data[:username]) || - User.find_by(email: user_id_data[:email]) || + # Lookup on what the identity provider asserted, otherwise create new + user = user_for_asserted_identity(login_id: user_id_data[:login_id], + email: user_id_data[:email], + derived_username: user_id_data[:username], + source: request.ip) || User.create do |new_user| # Update new user with details from the LTI response Doubtfire::Application.config.institution_settings.update_user_from_lti_response( @@ -294,12 +297,11 @@ class AuthenticationApi < Grape::API logger.info "Authenticate #{email} from #{request.ip}" - # Lookup using login_id if it exists - # Lookup using email otherwise and set login_id - # Otherwise create new - user = User.find_by(login_id: login_id) || - User.find_by(username: email[/(.*)@/, 1]) || - User.find_by(email: email) || + # Lookup on what the identity provider asserted, otherwise create new + user = user_for_asserted_identity(login_id: login_id, + email: email, + derived_username: email[/(.*)@/, 1], + source: request.ip) || User.find_or_create_by(login_id: login_id) do |new_user| role = Role.aaf_affiliation_to_role_id(attrs[:edupersonscopedaffiliation]) first_name = (attrs[:givenname] || attrs[:cn]).capitalize @@ -344,7 +346,11 @@ class AuthenticationApi < Grape::API protocol = Rails.env.development? ? 'http' : 'https' host = "#{protocol}://#{host}" end - redirect "#{host}/sign_in?authToken=#{onetime_token.authentication_token}&username=#{user.username}" + redirect AuthenticationHelpers.frontend_sign_in_url( + host: host, + auth_token: onetime_token.authentication_token, + username: user.username + ) end end @@ -375,7 +381,7 @@ class AuthenticationApi < Grape::API logger.info "Login #{params[:username]} from #{request.ip}" # Respond user details with new auth token - present :user, user, with: Entities::UserEntity + present :user, user, with: Entities::UserEntity, theme_owner_id: user.id present :auth_token, token.authentication_token present :auth_token_expiry, token.auth_token_expiry set_refresh_cookie_in_response(params[:remember]) @@ -502,7 +508,7 @@ class AuthenticationApi < Grape::API end # Return user details token = current_user.generate_authentication_token!(token_type: :general, force_new: false) - present :user, current_user, with: Entities::UserEntity + present :user, current_user, with: Entities::UserEntity, theme_owner_id: current_user.id present :auth_token, token.authentication_token present :auth_token_expiry, token.auth_token_expiry else diff --git a/app/api/d2l_integration_api/d2l_api.rb b/app/api/d2l_integration_api/d2l_api.rb index 66f7ec635b..00378d91d0 100644 --- a/app/api/d2l_integration_api/d2l_api.rb +++ b/app/api/d2l_integration_api/d2l_api.rb @@ -53,11 +53,11 @@ class D2lApi < Grape::API d2l = unit.d2l_assessment_mapping - if d2l.id != params[:id].to_i + if d2l.nil? || d2l.id != params[:id].to_i error!({ error: 'D2L details not found' }, 404) end - d2l.destroy if d2l.present? + d2l.destroy status 204 end @@ -75,7 +75,7 @@ class D2lApi < Grape::API d2l = unit.d2l_assessment_mapping - if d2l.id != params[:id].to_i + if d2l.nil? || d2l.id != params[:id].to_i error!({ error: 'D2L details not found' }, 404) end diff --git a/app/api/learning_outcomes_api.rb b/app/api/learning_outcomes_api.rb index d7752777e2..25dd832d9b 100644 --- a/app/api/learning_outcomes_api.rb +++ b/app/api/learning_outcomes_api.rb @@ -5,6 +5,7 @@ class LearningOutcomesApi < Grape::API helpers AuthorisationHelpers helpers MimeCheckHelpers helpers CsvHelper + helpers ContextModelHelpers before do authenticated? @@ -26,8 +27,8 @@ class LearningOutcomesApi < Grape::API end post '/:context_type_plural/:context_id/outcomes' do # find context model dynamically - context_type = params[:context_type_plural].singularize.camelize - context_model = context_type.classify.constantize.find(params[:context_id]) + context_type = context_type_for(params[:context_type_plural]) + context_model = context_model_for(params[:context_type_plural], params[:context_id]) unless authorise? current_user, context_model, :update error!({ error: 'You are not authorised to create outcomes in this context.' }, 403) @@ -77,8 +78,7 @@ class LearningOutcomesApi < Grape::API end put '/:context_type_plural/:context_id/outcomes/:id' do # find context model dynamically - context_type = params[:context_type_plural].singularize.camelize - context_model = context_type.classify.constantize.find(params[:context_id]) + context_model = context_model_for(params[:context_type_plural], params[:context_id]) unless authorise? current_user, context_model, :update error!({ error: 'You are not authorised to update outcomes in this context.' }, 403) @@ -130,8 +130,7 @@ class LearningOutcomesApi < Grape::API end delete '/:context_type_plural/:context_id/outcomes/:id' do # find context model dynamically - context_type = params[:context_type_plural].singularize.camelize - context_model = context_type.classify.constantize.find(params[:context_id]) + context_model = context_model_for(params[:context_type_plural], params[:context_id]) unless authorise? current_user, context_model, :update error!({ error: 'You are not authorised to delete outcomes in this context.' }, 403) @@ -169,8 +168,7 @@ class LearningOutcomesApi < Grape::API get '/:context_type_plural/:context_id/outcomes/csv' do # find context model dynamically include_tlos = params[:includes_tlos] || false - context_type = params[:context_type_plural].singularize.camelize - context_model = context_type.classify.constantize.find(params[:context_id]) + context_model = context_model_for(params[:context_type_plural], params[:context_id]) unless authorise? current_user, context_model, :update error!({ error: 'You are not authorised to download outcomes for this context.' }, 403) @@ -197,8 +195,7 @@ class LearningOutcomesApi < Grape::API ensure_csv!(params[:file][:tempfile]) # find context model dynamically - context_type = params[:context_type_plural].singularize.camelize - context_model = context_type.classify.constantize.find(params[:context_id]) + context_model = context_model_for(params[:context_type_plural], params[:context_id]) unless authorise? current_user, context_model, :upload_csv error!({ error: 'Not authorised to upload CSV of outcomes' }, 403) diff --git a/app/api/lti_api.rb b/app/api/lti_api.rb index 7e9bbd4e23..c3a4b6dac9 100644 --- a/app/api/lti_api.rb +++ b/app/api/lti_api.rb @@ -71,30 +71,54 @@ class LtiApi < Grape::API error!({ error: "Missing required fields: #{missing.join(', ')}" }, 400) end - # if current_user.role_id != Role.student_id - # return status 204 - # end + # The token names the person the launch was issued for, and its roles decide + # what that person gets. Apply it to that person, and only while that person + # is the one holding the session. + unless lti_member_is?(member, current_user) + error!({ error: 'This LTI token was not issued for the signed in user.' }, 403) + end - # role = unit.role_for(current_user) - # if !role.nil? && role != Role.student - # # error!({ error: 'Failed to enrol, user is already staff.' }, 400) - # return status 204 - # end + subject = current_user unit_role = Doubtfire::Application.config.institution_settings.should_employ_lti_member(member) - unless unit_role.nil? - unit.employ_staff(current_user, unit_role) - end - - unless Doubtfire::Application.config.institution_settings.should_enrol_lti_member(member) + enrol_member = Doubtfire::Application.config.institution_settings.should_enrol_lti_member(member) + + project = nil + consumed = ConsumedLtiToken.find_by(jti: token['jti']) + + if consumed.present? + # The web client carries the one launch token for the whole session and + # calls this route on every mount of the dashboard, so the subject + # presenting their own token again is not a replay. The token is spent + # either way, so nothing is applied a second time. + unless consumed.spent_by?(subject) + error!({ error: 'This LTI token has already been used.' }, 403) + end + + project = unit.projects.find_by(user_id: subject.id) if enrol_member + else + begin + ActiveRecord::Base.transaction do + # Spend the token before anything is applied. A concurrent replay + # loses on the unique index and rolls the whole enrolment back. + ConsumedLtiToken.consume!(token, user: subject) + + unit.employ_staff(subject, unit_role) unless unit_role.nil? + + # TODO: which campus? + project = unit.enrol_student(subject, nil) if enrol_member + end + rescue ConsumedLtiToken::AlreadyUsed + error!({ error: 'This LTI token has already been used.' }, 403) + end + end + + if project.nil? # error!({ error: 'User can not be enrolled into this unit.' }, 404) - return status 204 + status 204 + else + present project, with: Entities::ProjectEntity, user: subject, for_student: true, in_project: true end - - # TODO: which campus? - project = unit.enrol_student(current_user, nil) - - present project, with: Entities::ProjectEntity, user: current_user, for_student: true, in_project: true end desc 'Enrol a list of students into a linked Lti unit' diff --git a/app/api/settings_public_api.rb b/app/api/settings_public_api.rb new file mode 100644 index 0000000000..c3a5c34dac --- /dev/null +++ b/app/api/settings_public_api.rb @@ -0,0 +1,27 @@ +require 'grape' + +class SettingsPublicApi < Grape::API + # This endpoint is required before sign-in. + # Keep this response explicitly allowlisted. + desc 'Return public branding details for the Doubtfire front end' + get '/settings/public' do + response = { + externalName: Doubtfire::Application.config.institution[:product_name], + hasLogo: Doubtfire::Application.config.institution[:has_logo], + logoUrl: Doubtfire::Application.config.institution[:logo_url], + logoLinkUrl: Doubtfire::Application.config.institution[:logo_link_url] + } + + present response, with: Grape::Presenters::Presenter + end + + desc 'Return public privacy policy details' + get '/settings/privacy' do + response = { + privacy: Doubtfire::Application.config.institution[:privacy], + plagiarism: Doubtfire::Application.config.institution[:plagiarism] + } + + present response, with: Grape::Presenters::Presenter + end +end diff --git a/app/helpers/authentication_helpers.rb b/app/helpers/authentication_helpers.rb index 1f81dbe848..ca6c50d538 100644 --- a/app/helpers/authentication_helpers.rb +++ b/app/helpers/authentication_helpers.rb @@ -1,4 +1,5 @@ require 'onelogin/ruby-saml' +require 'uri' # # The AuthenticationHelpers include functions to check if the user @@ -31,7 +32,7 @@ def user_auth_token_type(user_param, auth_param, token_type) if user.present? && token.present? # has the tolken not expired? if token.auth_token_expiry > Time.zone.now - logger.info("Authenticated #{user.username} from #{request.ip}") + logger.info("Authenticated user_id=#{user.id} from #{request.ip}") :valid else # Token is timed out - destroy it and return error @@ -40,7 +41,10 @@ def user_auth_token_type(user_param, auth_param, token_type) :token_expired end elsif token.present? - logger.info("Error logging in for #{user_param} / #{auth_param} from #{request.ip}") + # Never echo the presented credential. This branch is reached for invalid + # and expired tokens, which are exactly the values an attacker may try to + # force into application logs. + logger.info("Error logging in with an invalid one-time token from #{request.ip}") :error else :missing_details @@ -52,6 +56,19 @@ def user_auth_token_type(user_param, auth_param, token_type) # module_function + # Keep one-time sign-in credentials out of the query string. Query strings + # are routinely captured by reverse-proxy access logs, browser history, and + # telemetry. A URI fragment is not sent in the HTTP request; the web client + # consumes and removes it before initialising telemetry. + def frontend_sign_in_url(host:, auth_token:, username:) + callback_fragment = URI.encode_www_form( + authToken: auth_token, + username: username + ) + + "#{host.to_s.delete_suffix('/')}/sign_in##{callback_fragment}" + end + def authenticated_via_refresh_token? auth_param = cookies['refresh_token'] user_param = cookies['username'] @@ -220,7 +237,7 @@ def set_refresh_cookie_in_response(remember) token = current_user.auth_tokens.where(token_type: :refresh_token).last # Generate a new token when the old one is absent or getting close to expiring - if token.nil? || token.auth_token_expiry <= Time.zone.now - 12.hours + if token.nil? || token.auth_token_expiry <= Time.zone.now + 12.hours token = current_user.generate_authentication_token!(token_type: :refresh_token) end diff --git a/app/helpers/context_model_helpers.rb b/app/helpers/context_model_helpers.rb new file mode 100644 index 0000000000..4b67e62d8a --- /dev/null +++ b/app/helpers/context_model_helpers.rb @@ -0,0 +1,20 @@ +module ContextModelHelpers + CONTEXT_MODELS = { + 'units' => Unit, + 'task_definitions' => TaskDefinition + }.freeze + + def context_model_for(context_type_plural, context_id) + context_class_for(context_type_plural).find(context_id) + end + + def context_type_for(context_type_plural) + context_class_for(context_type_plural).name + end + + private + + def context_class_for(context_type_plural) + CONTEXT_MODELS.fetch(context_type_plural.to_s) + end +end diff --git a/app/helpers/federated_identity_helper.rb b/app/helpers/federated_identity_helper.rb new file mode 100644 index 0000000000..40aa05d923 --- /dev/null +++ b/app/helpers/federated_identity_helper.rb @@ -0,0 +1,57 @@ +# +# Resolves the user that a federated assertion is about. +# +# The identity provider asserts a login_id and an email, and those are the only +# two things a federated sign in may be matched on. The username is derived from +# the local part of the email, so two people at different domains derive the +# same one and an account found that way is not necessarily the person the +# assertion is about. +# +module FederatedIdentityHelper + include LogHelper + + # + # Find the existing user this assertion is about, or nil so the caller creates + # one. Source is what a near miss gets logged against, the request ip for a + # sign in and the job context for a background import. + # + def user_for_asserted_identity(login_id:, email:, derived_username:, source:) + if login_id.present? + user = User.find_by(login_id: login_id) + return user unless user.nil? + end + + if email.present? + user = User.find_by(email: email) + + # A pre-federation account with no login id can be adopted on its asserted + # email. Once both the assertion and the account carry a login id, though, + # a mismatch settles the question: falling through to email would hand a + # shared or reused address to the wrong federated identity. + return user if user.present? && (login_id.blank? || user.login_id.blank?) + + unless user.nil? + logger.info "Refused email fallback for #{login_id} from #{source}" + return nil + end + end + + log_refused_username_match(login_id, derived_username, source) + nil + end + + private + + # + # An account already holds the username this assertion derives, and nothing + # the provider asserted matched it. Somebody investigating a duplicate account + # or a failed sign in later needs to see the near miss. The assertion itself + # is never logged. + # + def log_refused_username_match(login_id, derived_username, source) + return if derived_username.blank? + return unless User.exists?(username: derived_username) + + logger.info "Refused username match for #{login_id} from #{source}" + end +end diff --git a/app/helpers/lti_helper.rb b/app/helpers/lti_helper.rb index 335b33d805..97a6de1477 100644 --- a/app/helpers/lti_helper.rb +++ b/app/helpers/lti_helper.rb @@ -7,7 +7,9 @@ def decode_lti_token(token) jti = response['jti'] exp = response['exp'] - raise "Missing jti" if jti.nil? + # An empty jti is no more usable than a missing one, it cannot be + # recorded and so it cannot be spent. + raise "Missing jti" if jti.blank? raise "Missing exp" if exp.nil? rescue JWT::DecodeError => e logger.debug "Failed to validate Lti Token: #{e}" @@ -24,4 +26,41 @@ def valid_lti_member?(member) missing = required_fields.select { |f| member[f].nil? || member[f].to_s.strip.empty? } [missing.empty?, missing] end + + # + # The identity fields an LTI member maps onto a Doubtfire user. This is the + # mapping the LTI sign in already uses. + # + def lti_member_user_id_data(member) + { + login_id: member['ext_user_username'] || member['user_id'], + email: member['email'], + username: member['email']&.split('@')&.first + } + end + + # + # Is this signed in user the person the LTI member describes? + # + # Only the login_id and the email are asserted by the platform. The username + # is derived from the local part of the email, which is not unique across + # domains, so it is never enough on its own to say a token belongs to + # somebody. + # + def lti_member_is?(member, user) + return false if user.nil? + + id_data = lti_member_user_id_data(member) + + # The login_id is the strongest thing the platform asserts about the person, + # so when both sides carry one it settles the question by itself. Falling + # through to the email after a mismatch would let a token issued for somebody + # else bind to this user on a shared or reused address. + if id_data[:login_id].present? && user.login_id.present? + return user.login_id == id_data[:login_id] + end + + # No login_id on one side or the other, so the email is all that is left. + id_data[:email].present? && user.email.present? && user.email.casecmp?(id_data[:email]) + end end diff --git a/app/middleware/sentry_tunnel_middleware.rb b/app/middleware/sentry_tunnel_middleware.rb index 8556130117..aac126171f 100644 --- a/app/middleware/sentry_tunnel_middleware.rb +++ b/app/middleware/sentry_tunnel_middleware.rb @@ -3,6 +3,7 @@ class SentryTunnelMiddleware PATH = '/api/client-reports'.freeze + MAX_ENVELOPE_BYTES = 256 * 1024 def initialize(app) @app = app @@ -11,17 +12,37 @@ def initialize(app) def call(env) return @app.call(env) unless env['REQUEST_METHOD'] == 'POST' && env['PATH_INFO'] == PATH - forward_envelope(env) + return payload_too_large_response if declared_body_too_large?(env) + + body = read_envelope(env) + return payload_too_large_response if body.bytesize > MAX_ENVELOPE_BYTES + + forward_envelope(env, body) [204, {}, []] end private - def forward_envelope(env) + def declared_body_too_large?(env) + length = Integer(env['CONTENT_LENGTH'], exception: false) + length && length > MAX_ENVELOPE_BYTES + end + + def read_envelope(env) + input = env.fetch('rack.input') + input.read(MAX_ENVELOPE_BYTES + 1).to_s + ensure + input.rewind if input.respond_to?(:rewind) + end + + def payload_too_large_response + [413, { 'content-length' => '0' }, []] + end + + def forward_envelope(env, body) envelope_url = sentry_envelope_url return if envelope_url.blank? - body = env['rack.input'].read return if body.blank? RestClient::Request.execute( @@ -36,8 +57,6 @@ def forward_envelope(env) Rails.logger.warn "Unable to forward Sentry envelope: #{e.class} #{e.response&.code}" rescue RestClient::Exception, SocketError, Timeout::Error => e Rails.logger.warn "Unable to forward Sentry envelope: #{e.class}" - ensure - env['rack.input'].rewind if env['rack.input'].respond_to?(:rewind) end def sentry_headers(env) diff --git a/app/models/consumed_lti_token.rb b/app/models/consumed_lti_token.rb new file mode 100644 index 0000000000..c2bbfa8b6c --- /dev/null +++ b/app/models/consumed_lti_token.rb @@ -0,0 +1,47 @@ +# +# Records the id of an LTI token that has been used, and the user it was spent +# by, so the roles it carries are applied exactly once. +# +class ConsumedLtiToken < ApplicationRecord + # + # Raised when the jti of a token is already recorded. Only the insert of the + # record raises this, so it can never be confused with an unrelated unique + # index failure somewhere else in the enrolment. + # + class AlreadyUsed < StandardError; end + + belongs_to :user, inverse_of: :consumed_lti_tokens + + validates :jti, presence: true + validates :expires_at, presence: true + + scope :expired, -> { where('expires_at < ?', Time.zone.now) } + + # + # Record the use of a decoded LTI token. The unique index on jti means a + # replay, including a concurrent one, fails on the insert rather than on a + # read. + # + def self.consume!(token, user:) + create!(jti: token['jti'], user: user, expires_at: Time.zone.at(token['exp'].to_i)) + rescue ActiveRecord::RecordNotUnique + raise AlreadyUsed + end + + # + # Was this token spent by this user? A launch session presents the one token + # on every mount of the LTI dashboard, so the same person presenting it again + # is not a replay. Anybody else is. + # + def spent_by?(user) + !user.nil? && user_id == user.id + end + + # + # A token that has passed its expiry can no longer be replayed, so the row + # recording it can go. Called from the maintenance:cleanup rake task. + # + def self.destroy_expired_tokens + expired.destroy_all + end +end diff --git a/app/models/similarity/unit_similarity_module.rb b/app/models/similarity/unit_similarity_module.rb index 819dc288c6..3a5567bc63 100644 --- a/app/models/similarity/unit_similarity_module.rb +++ b/app/models/similarity/unit_similarity_module.rb @@ -107,8 +107,9 @@ def check_jplag_similarity(force: false) pwd = FileUtils.pwd completed_all_checks = true - # making temp directory for unit - jplag - root_work_dir = Rails.root.join("tmp", "jplag", "#{code}-#{id}") + # Unit codes are editable and may contain path or shell metacharacters. Keep + # the transient workspace derived only from database integer identifiers. + root_work_dir = Rails.root.join('tmp', 'jplag', "unit-#{id.to_i}") begin logger.info "Checking plagiarsm for unit #{code} - #{name} (id=#{id})" @@ -134,7 +135,7 @@ def check_jplag_similarity(force: false) FileUtils.mkdir_p(root_work_dir) # Init work directory for each task definition - tasks_dir = root_work_dir.join(td.id.to_s) + tasks_dir = root_work_dir.join(td.id.to_i.to_s) FileUtils.mkdir_p(tasks_dir) # There are new tasks, check these with JPLAG @@ -161,6 +162,8 @@ def check_jplag_similarity(force: false) end ensure FileUtils.chdir(pwd) if FileUtils.pwd != pwd + logger.info "Deleting JPlag work directory for unit #{id}: #{root_work_dir}" + FileUtils.rm_rf(root_work_dir) end self @@ -173,9 +176,6 @@ def update_moss_plagiarism_stats moss = MossRuby.new(moss_key) task_definitions.where(plagiarism_updated: true).find_each do |td| - td.plagiarism_updated = false - td.save - # Get results url = td.plagiarism_report_url logger.debug "Processing MOSS results #{url}" @@ -184,13 +184,18 @@ def update_moss_plagiarism_stats results = moss.extract_results(url, warn_pct, ->(line) { puts line }) + # Track whether every match linked cleanly. A match that fails is logged and + # skipped so the rest still process, but the definition is left flagged for + # the next scan instead of being silently marked done. + completed = true + # Use results results.each do |match| task_id1 = %r{.*/(\d+)/$}.match(match[0][:filename])[1] task_id2 = %r{.*/(\d+)/$}.match(match[1][:filename])[1] - t1 = Task.find(task_id1) - t2 = Task.find(task_id2) + t1 = Task.find_by(id: task_id1) + t2 = Task.find_by(id: task_id2) if t1.nil? || t2.nil? logger.error "Could not find tasks #{task_id1} or #{task_id2} for plagiarism stats check!" @@ -210,6 +215,22 @@ def update_moss_plagiarism_stats else # just link the individuals... create_moss_plagiarism_link(t1, t2, match, warn_pct) end + rescue StandardError => e + # One bad match must not abort the rest of the import for this definition, + # but the definition must be retried, so remember that it did not complete. + completed = false + logger.error "Failed to process MOSS match for task definition #{td.id}: #{e.message}" + next + end + + # Clear the flag only after a clean pass, and only while the report we just + # processed is still the current one. A concurrent scan that produced a newer + # report writes a new url and re-flags, so matching on url leaves that newer + # flag intact, and a partial failure is retried rather than dropped. + if completed + # rubocop:disable Rails/SkipsModelValidations + TaskDefinition.where(id: td.id, plagiarism_report_url: url).update_all(plagiarism_updated: false) + # rubocop:enable Rails/SkipsModelValidations end end @@ -241,12 +262,15 @@ def run_jplag_on_done_files(task_definition, tasks_dir, tasks_with_files, report similarity_pct = task_definition.plagiarism_warn_pct return if similarity_pct.nil? - # Check if the directory exists and create it if it doesn't - results_dir = File.dirname(report_path) - system("docker exec -i jplag sh -c 'if [ ! -d \"#{results_dir}\" ]; then mkdir -p \"#{results_dir}\"; fi'") || raise('Failed to create JPlag results directory') + # Pass every derived path as its own argv entry. Do not put unit, task, or + # report data through a shell in the API container or the JPlag container. + results_dir = File.dirname(report_path).to_s + system('docker', 'exec', '-i', 'jplag', 'mkdir', '-p', results_dir) || + raise('Failed to create JPlag results directory') - # Remove existing result file if it exists - system("docker exec -i jplag sh -c 'if [ -f \"#{report_path}\" ]; then rm \"#{report_path}\"; fi'") || raise('Failed to remove previous JPlag report') + # rm -f is already successful when the old report is absent. + system('docker', 'exec', '-i', 'jplag', 'rm', '-f', report_path.to_s) || + raise('Failed to remove previous JPlag report') # Extract task resources for base code use_base_code = false @@ -295,51 +319,49 @@ def run_jplag_on_done_files(task_definition, tasks_dir, tasks_with_files, report end logger.info "Starting JPLAG container to run on #{tasks_dir}" - root_dir = Rails.root.to_s - tasks_dir_split = tasks_dir.to_s.split(root_dir)[1] + tasks_dir_in_container = Pathname.new('/').join(tasks_dir.relative_path_from(Rails.root)).to_s file_lang = task_definition.similarity_language.to_s # Convert pct to decimal similarity_threshold = similarity_pct.to_f / 100 min_tokens = Doubtfire::Application.config.jplag_min_tokens.to_i - # If empty, let JPlag set the default per-language - min_token_string = min_tokens <= 0 ? "" : "--min-tokens=#{min_tokens}" - - base_code_string = use_base_code ? "--base-code=#{tasks_dir_split}/base" : "" - skip_cluster_check = Doubtfire::Application.config.jplag_skip_cluster_check - skip_cluster_string = skip_cluster_check ? '--cluster-skip' : '' max_shown_comparisons = Doubtfire::Application.config.jplag_max_shown_comparisons max_shown_comparisons = 2500 if max_shown_comparisons.nil? # Run JPLAG on the extracted files. JPlag container should already be in the /jplag/ workdir. docker_command = [ - "docker exec -i jplag", - "java -jar jplag-jar-with-dependencies.jar", - "--skip-version-check", - "#{tasks_dir_split}/submissions", - base_code_string, - "-l #{file_lang}", + 'docker', 'exec', '-i', 'jplag', + 'java', '-jar', 'jplag-jar-with-dependencies.jar', + '--skip-version-check', + File.join(tasks_dir_in_container, 'submissions') + ] + docker_command << "--base-code=#{File.join(tasks_dir_in_container, 'base')}" if use_base_code + docker_command.push( + '-l', file_lang, "--similarity-threshold=#{similarity_threshold}", - "--shown-comparisons=#{max_shown_comparisons}", - min_token_string, - skip_cluster_string, - "-M RUN", - "-r #{report_path.delete_suffix('.jplag')}", - "--overwrite" - ].join(" ") - - logger.debug "Executing command: #{docker_command}" - system(docker_command) - - # Delete the extracted code files from tmp - tmp_dir = Rails.root.join("tmp/jplag") - logger.info "Deleting files in: #{tmp_dir}" - logger.info "Files to delete: #{Dir.glob("#{tmp_dir}/*")}" - FileUtils.rm_rf(Dir.glob("#{tmp_dir}/*")) + "--shown-comparisons=#{max_shown_comparisons}" + ) + docker_command << "--min-tokens=#{min_tokens}" if min_tokens.positive? + docker_command << '--cluster-skip' if skip_cluster_check + docker_command.push( + '-M', 'RUN', + '-r', report_path.to_s.delete_suffix('.jplag'), + '--overwrite' + ) + + logger.debug "Executing command argv: #{docker_command.inspect}" + system(*docker_command) || raise('Failed to run JPlag similarity check') + self + ensure + # Each unit has its own root work directory. Only remove this task + # definition's extracted files here: another unit may be running in + # parallel under tmp/jplag and its workspace must remain untouched. + logger.info "Deleting JPlag task work directory: #{tasks_dir}" + FileUtils.rm_rf(tasks_dir) end def process_jplag_plagiarism_report(path, warn_pct, is_group) @@ -374,8 +396,8 @@ def process_jplag_plagiarism_report(path, warn_pct, is_group) task2_id = entry.name.split('/')[2].to_i end end - first_submission = Task.find(task1_id) if task1_id - second_submission = Task.find(task2_id) if task2_id + first_submission = Task.find_by(id: task1_id) if task1_id + second_submission = Task.find_by(id: task2_id) if task2_id if first_submission.nil? || second_submission.nil? logger.error "Could not find tasks #{comparison[:first_submission]} or #{comparison[:second_submission]} for plagiarism stats check!" diff --git a/config/deakin.rb b/config/deakin.rb index c62ef0a292..e48cb92f0e 100644 --- a/config/deakin.rb +++ b/config/deakin.rb @@ -154,8 +154,7 @@ def sync_streams_from_star(unit) activityData.each do |activity| # Make sure units match - subject_match = /.*?(?=_)/.match(activity["subject_code"]) - unit_code = subject_match.nil? ? nil : subject_match[0] + unit_code = value_before_delimiter(activity['subject_code'], '_') unless unit_code == unit.code logger.error "Failed to sync #{unit.code} - response had unit code #{enrolmentData['unitCode']}" return @@ -185,11 +184,15 @@ def sync_streams_from_star(unit) end end + def value_before_delimiter(value, delimiter) + string = value.to_s + delimiter_index = string.index(delimiter) + string[0...delimiter_index] unless delimiter_index.nil? + end + def fetch_star_row(row, unit) - email_match = /(.*)(?=@)/.match(row["email_address"]) - subject_match = /.*?(?=_)/.match(row["subject_code"]) - username = email_match.nil? ? nil : email_match[0] - unit_code = subject_match.nil? ? nil : subject_match[0] + username = value_before_delimiter(row['email_address'], '@') + unit_code = value_before_delimiter(row['subject_code'], '_') tutorial_code = fetch_tutorial unit, row diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb new file mode 100644 index 0000000000..d483f94916 --- /dev/null +++ b/config/initializers/rack_attack.rb @@ -0,0 +1,76 @@ +require 'digest' +require 'json' + +# Rack::Attack 6.8 uses Rack::Request, whose params do not parse JSON request +# bodies. Use Rails' parser, which rewinds and caches rack.input for the app. +module Rack + class Attack + class Request + AUTH_PATH = %r{\A/api/auth(?:\.json)?\z} + + def password_authentication_request? + post? && path.match?(AUTH_PATH) + end + + def authentication_username_digest + username = + if media_type == 'application/json' + ActionDispatch::Request.new(env).request_parameters['username'] + else + params['username'] + end + return unless username.is_a?(String) + + normalized_username = username.downcase.strip + Digest::SHA256.hexdigest(normalized_username) if normalized_username.present? + rescue ActionDispatch::Http::Parameters::ParseError + nil + end + end + end +end + +# Prefer the application's dedicated Redis cache and fall back to the mandatory +# Sidekiq Redis service. Process-local stores do not enforce a deployment-wide +# throttle when the API is replicated. +shared_redis_url = + ENV.fetch('DF_REDIS_CACHE_URL', nil).presence || + ENV.fetch('DF_REDIS_SIDEKIQ_URL', nil).presence + +Rack::Attack.cache.store = + if shared_redis_url.present? && !Rails.env.test? + ActiveSupport::Cache::RedisCacheStore.new( + url: shared_redis_url, + namespace: 'doubtfire:rack-attack' + ) + elsif Rails.env.local? + Rails.cache + else + raise 'Set DF_REDIS_CACHE_URL or DF_REDIS_SIDEKIQ_URL to enable authentication rate limiting' + end + +Rack::Attack.throttled_response_retry_after_header = true +Rack::Attack.throttled_responder = lambda do |request| + match_data = request.env.fetch('rack.attack.match_data') + retry_after = match_data[:period] - (match_data[:epoch_time] % match_data[:period]) + + [ + 429, + { + 'content-type' => 'application/json', + 'retry-after' => retry_after.to_s + }, + [JSON.generate(error: 'Too many authentication attempts. Please try again later.')] + ] +end + +# Limit authentication attempts from a single IP. +Rack::Attack.throttle('auth/ip', limit: 5, period: 1.minute) do |req| + req.ip if req.password_authentication_request? +end + +# Limit attempts against a single username without storing that username in the +# rate-limit cache key. +Rack::Attack.throttle('auth/username', limit: 5, period: 1.minute) do |req| + req.authentication_username_digest if req.password_authentication_request? +end diff --git a/db/migrate/20260827013000_create_consumed_lti_tokens.rb b/db/migrate/20260827013000_create_consumed_lti_tokens.rb new file mode 100644 index 0000000000..9e2eb58a3c --- /dev/null +++ b/db/migrate/20260827013000_create_consumed_lti_tokens.rb @@ -0,0 +1,14 @@ +class CreateConsumedLtiTokens < ActiveRecord::Migration[8.0] + def change + create_table :consumed_lti_tokens do |t| + t.string :jti, null: false + t.references :user, null: false, foreign_key: true + t.datetime :expires_at, null: false + + t.timestamps + end + + add_index :consumed_lti_tokens, :jti, unique: true + add_index :consumed_lti_tokens, :expires_at + end +end diff --git a/deployApi.Dockerfile b/deployApi.Dockerfile index f5ae9529df..2e55d70046 100644 --- a/deployApi.Dockerfile +++ b/deployApi.Dockerfile @@ -1,23 +1,14 @@ -# -# deployApi.Dockerfile - the container used to host the API only -# -FROM ruby:3.4-bookworm +# Production API image. Refresh the exact base digest only through a reviewed +# dependency update and rebuild both API/app-worker images from the same commit. +FROM ruby:3.4.10-bookworm@sha256:56e0c9fdbf64d090e45072d32f0d3be7f2e392e733444f7d176a50881e6c325a -# Setup dependencies ARG DEBIAN_FRONTEND=noninteractive RUN apt-get update \ - && apt-get install -y apt-transport-https ca-certificates curl gnupg2 software-properties-common \ - && install -m 0755 -d /etc/apt/keyrings \ - && curl -fsSL https://download.docker.com/linux/debian/gpg -o /etc/apt/keyrings/docker.asc \ - && chmod a+r /etc/apt/keyrings/docker.asc \ - && echo "deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.asc] https://download.docker.com/linux/debian $(. /etc/os-release && echo "$VERSION_CODENAME") stable" | tee /etc/apt/sources.list.d/docker.list \ - && curl -fsSL https://packages.redis.io/gpg | gpg --dearmor -o /usr/share/keyrings/redis-archive-keyring.gpg \ - && echo "deb [signed-by=/usr/share/keyrings/redis-archive-keyring.gpg] https://packages.redis.io/deb $(lsb_release -cs) main" | tee /etc/apt/sources.list.d/redis.list - -RUN apt-get update \ - && apt-get install -y \ + && apt-get install -y --no-install-recommends \ bc \ + ca-certificates \ + curl \ ffmpeg \ ghostscript \ imagemagick \ @@ -25,29 +16,24 @@ RUN apt-get update \ libmagickwand-dev \ libmariadb-dev \ tzdata \ - redis \ - docker-ce \ - docker-ce-cli \ - containerd.io \ - && apt-get clean + && rm -rf /var/lib/apt/lists/* -# Setup the folder where we will deploy the code WORKDIR /doubtfire -# Copy doubtfire-api source -COPY . /doubtfire/ +ENV RAILS_ENV=production \ + BUNDLE_WITHOUT=development:test:staging -# Install bundler -RUN gem install bundler -v '2.6.6' -RUN bundle config set --global without development test staging +RUN gem install bundler -v 2.6.6 --no-document -# Install the Gems -RUN bundle install +# Keep dependency installation cacheable and require the committed lockfile. +COPY Gemfile Gemfile.lock ./ +RUN bundle config set deployment true \ + && bundle install --jobs 4 --retry 3 -EXPOSE 3000 +COPY . ./ -# Set default to production -ENV RAILS_ENV production +EXPOSE 3000 -# Run migrate and server on launch -CMD bundle exec rake db:migrate && bundle exec rails s -b 0.0.0.0 +# Migrations are a separate one-shot deployment service. API startup must never +# race or silently repeat them. +CMD ["bundle", "exec", "rails", "server", "-b", "0.0.0.0"] diff --git a/deployAppSvr.Dockerfile b/deployAppSvr.Dockerfile index 5ef8f15815..d8e402bb8d 100644 --- a/deployAppSvr.Dockerfile +++ b/deployAppSvr.Dockerfile @@ -1,56 +1,47 @@ -# -# deployAppSrc.Dockerfile - the container used for back end processing -# -FROM ruby:3.4-bookworm +# Docker CLI only: workers use a constrained remote Docker API for TexLive and +# JPlag. Never ship a daemon or containerd in this application image. +FROM docker:28.5.2-cli@sha256:625d9431a9f54c5a2bc90f24f0e1c3d55b1349fd857dd85035f98c2c9acbdd4d AS docker_cli -# Setup dependencies -ARG DEBIAN_FRONTEND=noninteractive +# Build the app-worker from the same exact Ruby base and API source as the API. +FROM ruby:3.4.10-bookworm@sha256:56e0c9fdbf64d090e45072d32f0d3be7f2e392e733444f7d176a50881e6c325a -RUN apt-get update \ - && apt-get install -y apt-transport-https ca-certificates curl gnupg2 software-properties-common \ - && install -m 0755 -d /etc/apt/keyrings \ - && curl -fsSL https://download.docker.com/linux/debian/gpg -o /etc/apt/keyrings/docker.asc \ - && chmod a+r /etc/apt/keyrings/docker.asc \ - && echo "deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.asc] https://download.docker.com/linux/debian $(. /etc/os-release && echo "$VERSION_CODENAME") stable" | tee /etc/apt/sources.list.d/docker.list \ - && curl -fsSL https://packages.redis.io/gpg | gpg --dearmor -o /usr/share/keyrings/redis-archive-keyring.gpg \ - && echo "deb [signed-by=/usr/share/keyrings/redis-archive-keyring.gpg] https://packages.redis.io/deb $(lsb_release -cs) main" | tee /etc/apt/sources.list.d/redis.list +ARG DEBIAN_FRONTEND=noninteractive RUN apt-get update \ - && apt-get install -y \ + && apt-get install -y --no-install-recommends \ bc \ + bsd-mailx \ + ca-certificates \ + cron \ ffmpeg \ - ghostscript qpdf \ + ghostscript \ imagemagick \ libmagic-dev \ libmagickwand-dev \ libmariadb-dev \ + msmtp-mta \ python3-pygments \ + qpdf \ tzdata \ - cron \ - msmtp-mta bsd-mailx \ - redis \ - docker-ce \ - docker-ce-cli \ - containerd.io \ - && apt-get clean - -# Setup the folder where we will deploy the code + && rm -rf /var/lib/apt/lists/* + +COPY --from=docker_cli /usr/local/bin/docker /usr/local/bin/docker + WORKDIR /doubtfire -# Install bundler -RUN gem install bundler -v '2.6.6' -RUN bundle config set --global without development test staging +ENV RAILS_ENV=production \ + BUNDLE_WITHOUT=development:test:staging -# Install the Gems -COPY ./Gemfile ./Gemfile.lock /doubtfire/ -RUN bundle install +RUN gem install bundler -v 2.6.6 --no-document -# Copy doubtfire-api source -COPY . /doubtfire/ +COPY Gemfile Gemfile.lock ./ +RUN bundle config set deployment true \ + && bundle install --jobs 4 --retry 3 -# Crontab file copied to cron.d directory. -COPY ./.ci-setup/crontab /etc/cron.d/container_cronjob +COPY . ./ +COPY .ci-setup/crontab /etc/cron.d/container_cronjob -RUN touch /var/log/cron.log +RUN touch /var/log/cron.log \ + && chmod 0644 /etc/cron.d/container_cronjob -CMD /doubtfire/lib/shell/pdfgen_entry_point.sh +CMD ["/doubtfire/lib/shell/pdfgen_entry_point.sh"] diff --git a/lib/helpers/find_or_create_students.rb b/lib/helpers/find_or_create_students.rb index 19995782a3..e9a583caae 100644 --- a/lib/helpers/find_or_create_students.rb +++ b/lib/helpers/find_or_create_students.rb @@ -13,11 +13,9 @@ def find_or_create_student(username) email: "#{username}@doubtfire.com", username: username } - unless AuthenticationHelpers.aaf_auth? - profile[:password] = 'password' - profile[:password_confirmation] = 'password' - end - user_created = User.create!(profile) + user_created = User.new(profile) + user_created.password = 'password' unless AuthenticationHelpers.aaf_auth? + user_created.save! @user_cache[username] = user_created if using_cache else user_created = User.find_by(username: username) diff --git a/test/api/authentication_api_test.rb b/test/api/authentication_api_test.rb new file mode 100644 index 0000000000..caf70871f5 --- /dev/null +++ b/test/api/authentication_api_test.rb @@ -0,0 +1,170 @@ +require 'test_helper' +require 'securerandom' + +# +# Tests that a federated sign in resolves to the person the assertion is +# actually about. The LTI callback is the federated path that is mounted in the +# test environment, so it stands in for the SAML and AAF callbacks and for the +# LTI membership import job, which share the same lookup helper. +# +class AuthenticationApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def lti_token_for(member) + JWT.encode({ + member: member, + exp: Time.now.to_i + 30, + jti: SecureRandom.uuid + }, Doubtfire::Application.config.lti_api_secret, 'HS256') + end + + def lti_member(login_id:, email:) + { + user_id: SecureRandom.uuid, + name: 'Nickname', + given_name: 'First name', + family_name: 'Last name', + email: email, + ext_user_username: login_id, + roles: ['Learner'] + } + end + + # An assertion whose login_id matches an existing account resolves to it. + def test_assertion_resolves_on_matching_login_id + user = FactoryBot.create(:user, username: 'sec07-known', email: 'sec07-known@example.com') + user.update(login_id: 'sec07-known-login') + + user_count = User.count + + post '/api/auth/lti', { ltik: lti_token_for(lti_member(login_id: 'sec07-known-login', email: 'sec07-known@example.com')) } + + assert_equal 201, last_response.status, last_response_body + assert_equal user.username, last_response_body['username'] + assert_equal user_count, User.count, 'Matching on login_id must not create a user' + end + + # Once an account and an assertion both carry a login id, that identifier has + # to decide the match on its own. Falling through to a shared or reused email + # address after a mismatch would issue a login token for the wrong account. + def test_assertion_does_not_fall_back_to_email_after_a_login_id_mismatch + account = FactoryBot.create(:user, username: 'sec07-email-owner', email: 'sec07-shared@example.com') + account.update(login_id: 'sec07-email-owner-login') + + user_count = User.count + + post '/api/auth/lti', { + ltik: lti_token_for(lti_member(login_id: 'sec07-other-login', email: 'sec07-shared@example.com')) + } + + assert_equal 500, last_response.status, 'A mismatched asserted login id must not be rescued by the email' + assert_nil last_response_body['auth_token'], 'No token may be issued for the email owner' + assert_equal user_count, User.count, 'A refused assertion must not create an account' + + account.reload + assert_equal 'sec07-email-owner-login', account.login_id + assert_nil account.auth_tokens.first, 'No token may be issued for the unrelated account' + end + + # An assertion whose derived username collides with an unrelated account must + # not resolve to that account. The derived username is the local part of the + # asserted email, so two people at different domains derive the same one. + # Nothing the provider asserted matches, so the callback tries to create an + # account and the username the assertion derives is already taken. + def test_assertion_does_not_resolve_on_a_colliding_derived_username + unrelated = FactoryBot.create(:user, username: 'sec07-shared', email: 'sec07-shared@one.example.com') + unrelated.update(login_id: 'sec07-unrelated-login') + + user_count = User.count + + post '/api/auth/lti', { ltik: lti_token_for(lti_member(login_id: 'sec07-other-login', email: 'sec07-shared@two.example.com')) } + + assert_equal 500, last_response.status, 'A colliding assertion must be refused, not resolved' + assert_nil last_response_body['auth_token'], 'No token may be issued on a refused assertion' + assert_equal user_count, User.count, 'A refused assertion must not create an account' + + unrelated.reload + assert_equal 'sec07-unrelated-login', unrelated.login_id, 'The unrelated account must not be taken over' + assert_equal 'sec07-shared@one.example.com', unrelated.email, 'The unrelated account must not be rewritten' + assert_nil unrelated.auth_tokens.first, 'No token may be issued for the unrelated account' + end + + # An account created before the institution had an identity provider is + # adopted at its first federated sign in on the asserted email, so removing + # the username lookup does not orphan it. + def test_pre_federation_account_is_adopted_on_the_asserted_email + legacy = FactoryBot.create(:user, username: 'sec07-legacy', email: 'sec07-legacy@example.com') + legacy.update(login_id: nil) + + user_count = User.count + + post '/api/auth/lti', { ltik: lti_token_for(lti_member(login_id: 'sec07-legacy-login', email: 'sec07-legacy@example.com')) } + + assert_equal 201, last_response.status, last_response_body + assert_equal legacy.username, last_response_body['username'] + assert_equal user_count, User.count, 'A pre federation account must be adopted, not duplicated' + + legacy.reload + assert_equal 'sec07-legacy-login', legacy.login_id + end + + # A pre federation account whose stored email is not the asserted one is no + # longer adopted on its username, because the username is not asserted. The + # sign in is refused and an administrator has to correct the stored email or + # login_id before that person can sign in. + def test_pre_federation_account_with_a_different_stored_email_is_not_adopted + legacy = FactoryBot.create(:user, username: 'sec07-moved', email: 'sec07-moved@old.example.com') + legacy.update(login_id: nil) + + user_count = User.count + + post '/api/auth/lti', { ltik: lti_token_for(lti_member(login_id: 'sec07-moved-login', email: 'sec07-moved@new.example.com')) } + + assert_equal 500, last_response.status, 'An unasserted username must not resolve the account' + assert_equal user_count, User.count + + legacy.reload + assert_nil legacy.login_id, 'The account must not have a login_id installed on it' + assert_equal 'sec07-moved@old.example.com', legacy.email + end + + # A legacy account with no email recorded holds a username but nothing the + # provider can assert, so an assertion that derives that username must not + # pick it up either. Matching it would hand the account to whoever registers + # the same local part at any domain. + def test_legacy_account_with_a_blank_email_is_not_taken_over + legacy = FactoryBot.create(:user, username: 'sec07-blank', email: 'sec07-blank@example.com') + legacy.update(login_id: nil) + legacy.update_column(:email, '') + + user_count = User.count + + post '/api/auth/lti', { ltik: lti_token_for(lti_member(login_id: 'sec07-attacker-login', email: 'sec07-blank@attacker.example.com')) } + + assert_equal 500, last_response.status, 'A blank email account must not be matched on its username' + assert_nil last_response_body['auth_token'], 'No token may be issued on a refused assertion' + assert_equal user_count, User.count + + legacy.reload + assert_nil legacy.login_id, 'The blank email account must not be taken over' + assert_nil legacy.auth_tokens.first, 'No token may be issued for the blank email account' + end + + # The normal path. A first time user is still created. + def test_first_time_user_is_still_created + user_count = User.count + + post '/api/auth/lti', { ltik: lti_token_for(lti_member(login_id: 'sec07-new-login', email: 'sec07-new@example.com')) } + + assert_equal 201, last_response.status, last_response_body + assert_equal 'sec07-new', last_response_body['username'] + assert_equal user_count + 1, User.count + + created = User.find_by(username: 'sec07-new') + assert_not_nil created + assert_equal 'sec07-new-login', created.login_id + assert_equal 'sec07-new@example.com', created.email + end +end diff --git a/test/api/authentication_refresh_cookie_test.rb b/test/api/authentication_refresh_cookie_test.rb new file mode 100644 index 0000000000..48f79c4616 --- /dev/null +++ b/test/api/authentication_refresh_cookie_test.rb @@ -0,0 +1,84 @@ +require 'test_helper' + +class AuthenticationRefreshCookieTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::JsonHelper + + def app + Rails.application + end + + setup do + Rack::Attack.reset! + end + + def test_remembered_login_rotates_refresh_token_at_renewal_boundary + travel_to Time.zone.parse('2026-08-28 12:00:00 UTC') do + user = FactoryBot.create(:user) + renewal_boundary = Time.zone.now + 12.hours + old_token = user.generate_authentication_token!( + expiry: renewal_boundary, + token_type: :refresh_token + ) + + post_remembered_login(user) + + refresh_tokens = user.auth_tokens.where(token_type: :refresh_token).order(:id) + new_token = refresh_tokens.last + + assert_equal 2, refresh_tokens.count + assert_not_equal old_token.id, new_token.id + assert_operator new_token.auth_token_expiry, :>, renewal_boundary + assert_match(/refresh_token=#{new_token.authentication_token};/, last_response.cookies['refresh_token'].to_s) + end + end + + def test_remembered_login_reuses_refresh_token_outside_renewal_window + travel_to Time.zone.parse('2026-08-28 12:00:00 UTC') do + user = FactoryBot.create(:user) + old_token = user.generate_authentication_token!( + expiry: Time.zone.now + 12.hours + 1.second, + token_type: :refresh_token + ) + + post_remembered_login(user) + + refresh_tokens = user.auth_tokens.where(token_type: :refresh_token).order(:id) + + assert_equal [old_token.id], refresh_tokens.pluck(:id) + assert_match(/refresh_token=#{old_token.authentication_token};/, last_response.cookies['refresh_token'].to_s) + end + end + + def test_remembered_login_rotates_expired_refresh_token + travel_to Time.zone.parse('2026-08-28 12:00:00 UTC') do + user = FactoryBot.create(:user) + old_token = user.generate_authentication_token!( + expiry: Time.zone.now - 1.second, + token_type: :refresh_token + ) + + post_remembered_login(user) + + refresh_tokens = user.auth_tokens.where(token_type: :refresh_token).order(:id) + new_token = refresh_tokens.last + + assert_equal 2, refresh_tokens.count + assert_not_equal old_token.id, new_token.id + assert_operator new_token.auth_token_expiry, :>, Time.zone.now + assert_match(/refresh_token=#{new_token.authentication_token};/, last_response.cookies['refresh_token'].to_s) + end + end + + private + + def post_remembered_login(user) + post_json '/api/auth.json', { + username: user.username, + password: 'password', + remember: true + } + + assert_equal 201, last_response.status + end +end diff --git a/test/api/d2l_test.rb b/test/api/d2l_test.rb index 37eadc7ab3..8587772cb3 100644 --- a/test/api/d2l_test.rb +++ b/test/api/d2l_test.rb @@ -109,6 +109,38 @@ def test_can_update_d2l_details_for_unit assert_equal '54321', unit.d2l_assessment_mapping.org_unit_id end + # A unit with no mapping used to reach d2l.id on nil and answer 500. Both routes + # must now report 404 instead. + def test_delete_d2l_without_mapping_returns_404 + unit = FactoryBot.create(:unit, with_students: false) + add_auth_header_for(user: unit.main_convenor_user) + + delete "/api/units/#{unit.id}/d2l/1" + assert_equal 404, last_response.status, last_response.inspect + assert_nil unit.reload.d2l_assessment_mapping + end + + def test_update_d2l_without_mapping_returns_404 + unit = FactoryBot.create(:unit, with_students: false) + add_auth_header_for(user: unit.main_convenor_user) + + put "/api/units/#{unit.id}/d2l/1", { org_unit_id: '54321' } + assert_equal 404, last_response.status, last_response.inspect + assert_nil unit.reload.d2l_assessment_mapping + end + + def test_delete_d2l_with_mismatched_id_returns_404 + unit = FactoryBot.create(:unit, with_students: false) + d2l = D2lAssessmentMapping.create(unit: unit, org_unit_id: '12345') + add_auth_header_for(user: unit.main_convenor_user) + + initial_count = D2lAssessmentMapping.count + + delete "/api/units/#{unit.id}/d2l/#{d2l.id + 1}" + assert_equal 404, last_response.status, last_response.inspect + assert_equal initial_count, D2lAssessmentMapping.count + end + def test_can_login_to_d2l user = FactoryBot.create(:user, :convenor) add_auth_header_for(user: user) diff --git a/test/api/lti_api_test.rb b/test/api/lti_api_test.rb index 607c30a3b8..c31404becd 100644 --- a/test/api/lti_api_test.rb +++ b/test/api/lti_api_test.rb @@ -1,4 +1,5 @@ require 'test_helper' +require 'minitest/mock' require 'securerandom' require 'json' @@ -7,6 +8,36 @@ class LtiApiTest < ActiveSupport::TestCase include TestHelpers::AuthHelper include TestHelpers::JsonHelper + # Build the LTI member block that describes an existing Doubtfire user, using + # the identity fields the LTI routes map onto a user. + def lti_member_for(user, roles:) + { + user_id: user.id.to_s, + name: user.nickname || user.first_name, + given_name: user.first_name, + family_name: user.last_name, + email: user.email, + ext_user_username: user.login_id, + roles: roles + } + end + + def lti_user(trait) + FactoryBot.create(:user, trait, login_id: "lti-#{SecureRandom.hex(6)}") + end + + # Pass jti: nil to build a token that carries no JWT id at all. + def lti_enrol_token(unit, member, jti: SecureRandom.uuid) + payload = { + unit_id: unit.id, + member: member, + exp: Time.now.to_i + 30 + } + payload[:jti] = jti unless jti.nil? + + JWT.encode(payload, Doubtfire::Application.config.lti_api_secret, 'HS256') + end + def test_ensure_jwt_secret_is_valid # Simply validate that our ENV var is not nil secret_key = Doubtfire::Application.config.lti_api_secret @@ -231,14 +262,6 @@ def test_convenor_can_link_requested_unit end def test_correct_roles_are_enrolled - users = [ - FactoryBot.create(:user, :student), - FactoryBot.create(:user, :admin), - FactoryBot.create(:user, :convenor), - FactoryBot.create(:user, :auditor), - FactoryBot.create(:user, :tutor) - ] - roles_can_be_enrolled = %w[ Student Learner @@ -251,51 +274,304 @@ def test_correct_roles_are_enrolled unit = FactoryBot.create(:unit, with_students: false) - payload = { - unit_id: unit.id, - member: { - user_id: '2', - name: 'Nickname', - given_name: 'First name', - family_name: 'Last name', - email: 'email@doubtfire.com', - ext_user_username: 'student_test_lti', - roles: ['Learner'] - }, - exp: Time.now.to_i + 30, - jti: SecureRandom.uuid - } - - secret_key = Doubtfire::Application.config.lti_api_secret - token = JWT.encode(payload, secret_key, 'HS256') - + # Each launch is presented by the person it was issued for, and each one + # carries its own token id. roles_cant_be_enrolled.each do |role| - payload[:member][:roles] = [role] - - token = JWT.encode(payload, secret_key, 'HS256') + user = lti_user(:student) + token = lti_enrol_token(unit, lti_member_for(user, roles: [role])) - add_auth_header_for(user: users.sample) + add_auth_header_for(user: user) post '/api/lti/enrol', { ltik: token } - assert_equal 204, last_response.status + assert_equal 204, last_response.status, last_response.body end roles_can_be_enrolled.each do |role| - payload[:member][:roles] = [role] + user = lti_user(:student) + token = lti_enrol_token(unit, lti_member_for(user, roles: [role])) - token = JWT.encode(payload, secret_key, 'HS256') - - add_auth_header_for(user: users.sample) # or whichever user you want as caller + add_auth_header_for(user: user) post '/api/lti/enrol', { ltik: token } - assert_equal 201, last_response.status + assert_equal 201, last_response.status, last_response.body id = last_response_body['id'] assert_not_nil id, "Expected project ID in response" project = Project.find(id) assert project.valid?, "Expected project to be created" assert_equal unit.id, project.unit.id + assert_equal user.id, project.user_id + end + end + + # The launch subject enrolling themselves is the ordinary path and must keep + # working. + def test_lti_enrol_binds_a_token_to_its_subject + unit = FactoryBot.create(:unit, with_students: false) + student = lti_user(:student) + + token = lti_enrol_token(unit, lti_member_for(student, roles: ['Learner'])) + + add_auth_header_for(user: student) + post '/api/lti/enrol', { ltik: token } + + assert_equal 201, last_response.status, last_response.body + + project = Project.find(last_response_body['id']) + assert_equal student.id, project.user_id + assert_equal unit.id, project.unit_id + end + + # A token issued for a member of staff must not give its bearer that staff + # role in the unit. + def test_lti_enrol_rejects_a_token_presented_by_another_user + unit = FactoryBot.create(:unit, with_students: false) + staff = lti_user(:tutor) + # Tutor capable, so without the check the token's Instructor role would + # actually land on them. + bearer = lti_user(:tutor) + + token = lti_enrol_token(unit, lti_member_for(staff, roles: ['Instructor'])) + + add_auth_header_for(user: bearer) + post '/api/lti/enrol', { ltik: token } + + unit.reload + assert_nil unit.unit_role_for(bearer), "Bearer of the token gained a unit role" + assert_nil unit.unit_role_for(staff), "Subject of the token gained a unit role" + assert_equal 0, unit.projects.where(user_id: bearer.id).count + + assert_equal 403, last_response.status, last_response.body + end + + # The web client carries the one launch token for the whole session, so the + # subject presenting it again has to keep working, and has to give the same + # enrolment back rather than a second one. + def test_lti_enrol_lets_the_launch_subject_present_the_same_token_again + unit = FactoryBot.create(:unit, with_students: false) + student = lti_user(:student) + jti = SecureRandom.uuid + + token = lti_enrol_token(unit, lti_member_for(student, roles: ['Learner']), jti: jti) + + add_auth_header_for(user: student) + + post '/api/lti/enrol', { ltik: token } + assert_equal 201, last_response.status, last_response.body + project_id = last_response_body['id'] + + post '/api/lti/enrol', { ltik: token } + assert_equal 201, last_response.status, last_response.body + assert_equal project_id, last_response_body['id'] + + assert_equal 1, unit.projects.where(user_id: student.id).count + assert_equal 1, ConsumedLtiToken.where(jti: jti).count + end + + # A token already spent by somebody else must not be spendable again, even by + # a caller the member fields now resolve to. + def test_lti_enrol_rejects_a_token_already_spent_by_another_user + unit = FactoryBot.create(:unit, with_students: false) + student = lti_user(:student) + other = lti_user(:student) + jti = SecureRandom.uuid + + ConsumedLtiToken.create!(jti: jti, user: other, expires_at: 1.minute.from_now) + + token = lti_enrol_token(unit, lti_member_for(student, roles: ['Learner']), jti: jti) + + add_auth_header_for(user: student) + post '/api/lti/enrol', { ltik: token } + + assert_equal 403, last_response.status, last_response.body + assert_equal 'This LTI token has already been used.', last_response_body['error'] + assert_equal 0, unit.projects.where(user_id: student.id).count + end + + # The replay record belongs to the account whose launch spent it. It must not + # turn the new foreign key into a reason an otherwise unused account cannot be + # deleted. + def test_consumed_lti_token_is_removed_with_its_user + user = lti_user(:student) + consumed = ConsumedLtiToken.create!(jti: SecureRandom.uuid, user: user, expires_at: 1.minute.from_now) + + user.destroy! + + assert_not ConsumedLtiToken.exists?(consumed.id) + end + + # The loser of a race on the unique index saw nothing recorded when it + # started, and still must not spend the token a second time. + def test_lti_enrol_rejects_a_concurrent_replay + unit = FactoryBot.create(:unit, with_students: false) + student = lti_user(:student) + + token = lti_enrol_token(unit, lti_member_for(student, roles: ['Learner'])) + + add_auth_header_for(user: student) + + duplicate = ->(*_args, **_kwargs) { raise ActiveRecord::RecordNotUnique, 'Duplicate entry' } + ConsumedLtiToken.stub(:create!, duplicate) do + post '/api/lti/enrol', { ltik: token } + end + + assert_equal 403, last_response.status, last_response.body + assert_equal 'This LTI token has already been used.', last_response_body['error'] + assert_equal 0, unit.projects.where(user_id: student.id).count + end + + # A unique index failure from anywhere else in the enrolment is not a replay + # and must not be reported as one. + def test_lti_enrol_does_not_report_an_unrelated_unique_failure_as_a_replay + unit = FactoryBot.create(:unit, with_students: false) + student = lti_user(:student) + + token = lti_enrol_token(unit, lti_member_for(student, roles: ['Learner'])) + + add_auth_header_for(user: student) + + duplicate = lambda do |*_args, **_kwargs| + raise ActiveRecord::RecordNotUnique, "Duplicate entry for key 'index_projects_on_unit_id_and_user_id'" + end + + Project.stub(:create!, duplicate) do + post '/api/lti/enrol', { ltik: token } end + + assert_not_equal 403, last_response.status, last_response.body + assert_not_equal 'This LTI token has already been used.', last_response_body['error'] + end + + # A token whose member only lines up with the local part of somebody's email + # address names nobody. The platform asserts a login id and an email, and + # neither of those is the caller here. + def test_lti_enrol_rejects_a_token_matching_only_a_derived_username + unit = FactoryBot.create(:unit, with_students: false) + + local_part = "alex-#{SecureRandom.hex(4)}" + # Tutor capable, so the token's Instructor role would actually land on them. + caller_user = FactoryBot.create( + :user, + :tutor, + username: local_part, + login_id: "lti-#{SecureRandom.hex(6)}", + email: "#{local_part}@another.example" + ) + + jti = SecureRandom.uuid + member = { + user_id: "unseen-#{SecureRandom.hex(4)}", + name: 'New Staff', + given_name: 'New', + family_name: 'Staff', + email: "#{local_part}@provider.example", + ext_user_username: "new-staff-#{SecureRandom.hex(4)}", + roles: ['Instructor'] + } + + token = lti_enrol_token(unit, member, jti: jti) + + add_auth_header_for(user: caller_user) + post '/api/lti/enrol', { ltik: token } + + assert_equal 403, last_response.status, last_response.body + + unit.reload + assert_nil unit.unit_role_for(caller_user), "Caller gained the token's staff role" + assert_nil ConsumedLtiToken.find_by(jti: jti) + end + + # A token that names a login_id has said who it is for. If that does not match, + # a shared or reused email address must not let it bind anyway. + def test_lti_enrol_rejects_a_token_whose_login_id_does_not_match + unit = FactoryBot.create(:unit, with_students: false) + + shared_email = "shared-#{SecureRandom.hex(4)}@provider.example" + caller_user = FactoryBot.create( + :user, + :tutor, + username: "alex-#{SecureRandom.hex(4)}", + login_id: "lti-#{SecureRandom.hex(6)}", + email: shared_email + ) + + jti = SecureRandom.uuid + member = { + user_id: "unseen-#{SecureRandom.hex(4)}", + name: 'New Staff', + given_name: 'New', + family_name: 'Staff', + # Same address, but the platform is naming a different person. + email: shared_email, + ext_user_username: "new-staff-#{SecureRandom.hex(4)}", + roles: ['Instructor'] + } + + token = lti_enrol_token(unit, member, jti: jti) + + add_auth_header_for(user: caller_user) + post '/api/lti/enrol', { ltik: token } + + assert_equal 403, last_response.status, last_response.body + + unit.reload + assert_nil unit.unit_role_for(caller_user), "Caller gained the token's staff role on an email match alone" + assert_nil ConsumedLtiToken.find_by(jti: jti) + end + + # Recording token ids must not let a token through that carries no id. + def test_lti_enrol_rejects_a_token_without_a_jti + unit = FactoryBot.create(:unit, with_students: false) + student = lti_user(:student) + + token = lti_enrol_token(unit, lti_member_for(student, roles: ['Learner']), jti: nil) + + add_auth_header_for(user: student) + post '/api/lti/enrol', { ltik: token } + + assert_equal 403, last_response.status, last_response.body + assert_equal "Invalid LTI token.", last_response_body['error'] + assert_equal 0, unit.projects.where(user_id: student.id).count + end + + # An empty token id is no more recordable than a missing one, so it has to be + # turned away in the same place rather than blowing up on the insert. + def test_lti_enrol_rejects_a_token_with_an_empty_jti + unit = FactoryBot.create(:unit, with_students: false) + student = lti_user(:student) + + token = lti_enrol_token(unit, lti_member_for(student, roles: ['Learner']), jti: '') + + add_auth_header_for(user: student) + post '/api/lti/enrol', { ltik: token } + + assert_equal 403, last_response.status, last_response.body + assert_equal "Invalid LTI token.", last_response_body['error'] + assert_equal 0, unit.projects.where(user_id: student.id).count + end + + # An ordinary staff launch still employs the person it was issued for, with + # the role the token asks for. + def test_lti_enrol_employs_the_launch_subject_as_staff + unit = FactoryBot.create(:unit, with_students: false) + tutor = lti_user(:tutor) + + token = lti_enrol_token(unit, lti_member_for(tutor, roles: ['Instructor'])) + + add_auth_header_for(user: tutor) + post '/api/lti/enrol', { ltik: token } + + assert_equal 204, last_response.status, last_response.body + + # The dashboard mounts again with the same token after the unit is linked. + post '/api/lti/enrol', { ltik: token } + assert_equal 204, last_response.status, last_response.body + + unit.reload + assert_equal 1, unit.unit_roles.where(user_id: tutor.id).count + unit_role = unit.unit_role_for(tutor) + assert_not_nil unit_role, "Expected the launch subject to be employed" + assert_equal Role.tutor.id, unit_role.role_id end def test_enrol_students_bulk diff --git a/test/config/deakin_config_test.rb b/test/config/deakin_config_test.rb index 25da492eec..c4f4f81583 100644 --- a/test/config/deakin_config_test.rb +++ b/test/config/deakin_config_test.rb @@ -24,6 +24,14 @@ def teardown Doubtfire::Application.config.institution_settings = @@backup end + def test_value_before_delimiter_uses_a_bounded_string_search + settings = Doubtfire::Application.config.institution_settings + + assert_equal 'student', settings.value_before_delimiter('student@example.edu.au', '@') + assert_equal 'SIT999', settings.value_before_delimiter('SIT999_CLASS', '_') + assert_nil settings.value_before_delimiter('a' * 100_000, '_') + end + def test_sync_deakin_unit WebMock.reset_executed_requests! diff --git a/test/models/authorisation_helpers_test.rb b/test/models/authorisation_helpers_test.rb new file mode 100644 index 0000000000..8f28496c1b --- /dev/null +++ b/test/models/authorisation_helpers_test.rb @@ -0,0 +1,58 @@ +require 'test_helper' +require 'minitest/mock' + +class AuthorisationHelpersTest < ActiveSupport::TestCase + # Capture Rails.logger.warn for the duration of the block, tolerating any + # arity so an unrelated warn elsewhere does not blow up the test. + def capture_warnings(&block) + warnings = [] + Rails.logger.stub(:warn, ->(*args, &warn_block) { warnings << (args.first || warn_block&.call) }) do + block.call + end + warnings + end + + def test_denial_when_user_has_no_role_is_logged + unit = FactoryBot.create(:unit) + outsider = FactoryBot.create(:user, :student) + + warnings = capture_warnings do + assert_not AuthorisationHelpers.authorise?(outsider, unit, :get) + end + + assert warnings.any? { |m| m.to_s.include?('authorisation denied') && m.to_s.include?('get') }, + "expected a denial warning, got: #{warnings.inspect}" + end + + def employed_convenor(unit) + convenor = FactoryBot.create :user, :convenor + unit.employ_staff(convenor, Role.convenor) + convenor + end + + def test_denial_when_action_is_absent_from_the_permission_hash_is_logged + unit = FactoryBot.create(:unit) + convenor = employed_convenor(unit) + perms = ->(_role, _hash, _other) { [:some_allowed_action] } + + warnings = capture_warnings do + assert_not AuthorisationHelpers.authorise?(convenor, unit, :a_denied_action, perms) + end + + assert warnings.any? { |m| m.to_s.include?('authorisation denied') && m.to_s.include?('a_denied_action') }, + "expected a denial warning, got: #{warnings.inspect}" + end + + def test_granted_authorisation_logs_nothing + unit = FactoryBot.create(:unit) + convenor = employed_convenor(unit) + perms = ->(_role, _hash, _other) { [:some_allowed_action] } + + warnings = capture_warnings do + assert AuthorisationHelpers.authorise?(convenor, unit, :some_allowed_action, perms) + end + + assert_not warnings.any? { |m| m.to_s.include?('authorisation denied') }, + "expected no denial warning, got: #{warnings.inspect}" + end +end diff --git a/test/models/context_model_helpers_test.rb b/test/models/context_model_helpers_test.rb new file mode 100644 index 0000000000..1b604e324d --- /dev/null +++ b/test/models/context_model_helpers_test.rb @@ -0,0 +1,16 @@ +require 'test_helper' + +class ContextModelHelpersTest < ActiveSupport::TestCase + include ContextModelHelpers + + def test_context_models_are_explicitly_allowlisted + assert_equal Unit, send(:context_class_for, 'units') + assert_equal TaskDefinition, send(:context_class_for, 'task_definitions') + end + + def test_arbitrary_constants_cannot_be_selected + assert_raises(KeyError) do + send(:context_class_for, 'Kernel') + end + end +end diff --git a/test/models/task_similarity_test.rb b/test/models/task_similarity_test.rb index 2ec972a113..501d9e31e1 100644 --- a/test/models/task_similarity_test.rb +++ b/test/models/task_similarity_test.rb @@ -273,7 +273,7 @@ def test_fetch_viewer_url get "/api/tasks/#{task.id}/similarities/#{sim.id}/viewer_url" assert_equal 200, last_response.status - assert last_response.body.include? "https://viewer.url" + assert_equal 'https://viewer.url', JSON.parse(last_response.body) add_auth_header_for(user: task.project.student) get "/api/tasks/#{task.id}/similarities/#{sim.id}/viewer_url" @@ -283,4 +283,155 @@ def test_fetch_viewer_url sim.destroy! task.destroy! end + + # A MOSS match that points at a task destroyed between the scan and this run + # used to raise ActiveRecord::RecordNotFound from Task.find and abort the whole + # import, losing every later match. find_by returns nil so the guard skips it + # and the remaining matches are still linked. + def test_moss_import_skips_a_missing_task_and_keeps_processing + unit = FactoryBot.create(:unit, with_students: false, stream_count: 0) + td = unit.task_definitions.first + td.update!(plagiarism_updated: true, plagiarism_warn_pct: 10) + + task_a = FactoryBot.create(:project, unit: unit).task_for_task_definition(td) + task_b = FactoryBot.create(:project, unit: unit).task_for_task_definition(td) + missing_id = Task.maximum(:id).to_i + 100_000 + + results = [ + [{ filename: "u/#{missing_id}/" }, { filename: "u/#{task_a.id}/" }], # one side deleted + [{ filename: "u/#{task_a.id}/" }, { filename: "u/#{task_b.id}/" }] # both present + ] + + linked = [] + run_moss_stats(unit, results) do + unit.stub(:create_moss_plagiarism_link, ->(t1, t2, _m, _w) { linked << [t1.id, t2.id] }) do + unit.update_moss_plagiarism_stats + end + end + + assert_equal [[task_a.id, task_b.id]], linked, 'the valid pair is linked, the deleted-task pair is skipped' + assert_not td.reload.plagiarism_updated, 'the flag is cleared after a clean pass' + end + + # If the results cannot be read at all the definition must stay flagged so the + # next scan retries it. It used to be un-flagged at the top of the loop, before + # the results were touched, so a mid-run failure lost the definition silently. + def test_moss_import_keeps_the_flag_when_results_cannot_be_read + unit = FactoryBot.create(:unit, with_students: false, stream_count: 0) + td = unit.task_definitions.first + td.update!(plagiarism_updated: true) + + boom = Object.new + boom.define_singleton_method(:extract_results) { |*_args| raise 'moss unavailable' } + + credentials = Object.new + credentials.define_singleton_method(:secret_key_moss) { 'test-moss-key' } + + Doubtfire::Application.stub(:credentials, credentials) do + MossRuby.stub(:new, boom) do + assert_raises(RuntimeError) { unit.update_moss_plagiarism_stats } + end + end + + assert td.reload.plagiarism_updated, 'the flag stays set so the scan is retried' + end + + # A match that fails to link must be logged and skipped so the remaining matches + # still process, but the definition must stay flagged so it is retried rather than + # silently marked done. + def test_moss_import_retries_the_definition_when_a_match_fails + unit = FactoryBot.create(:unit, with_students: false, stream_count: 0) + td = unit.task_definitions.first + td.update!(plagiarism_updated: true, plagiarism_warn_pct: 10) + + task_a = FactoryBot.create(:project, unit: unit).task_for_task_definition(td) + task_b = FactoryBot.create(:project, unit: unit).task_for_task_definition(td) + + results = [ + [{ filename: "u/#{task_a.id}/" }, { filename: "u/#{task_b.id}/" }], + [{ filename: "u/#{task_b.id}/" }, { filename: "u/#{task_a.id}/" }] + ] + + attempts = 0 + linker = lambda do |_t1, _t2, _match, _warn| + attempts += 1 + raise 'link failed' if attempts == 1 + end + + run_moss_stats(unit, results) do + unit.stub(:create_moss_plagiarism_link, linker) do + unit.update_moss_plagiarism_stats # must not raise + end + end + + assert_equal 2, attempts, 'the second match is still attempted after the first fails' + assert td.reload.plagiarism_updated, 'the flag stays set so the failed definition is retried' + end + + # The JPlag report maps zip entries back to tasks. A comparison that points at a + # task destroyed since the scan used to raise RecordNotFound and abort the whole + # report; find_by returns nil so the guard skips it and the rest still link. + def test_jplag_report_skips_a_missing_task_and_keeps_processing + unit = FactoryBot.create(:unit, with_students: false, stream_count: 0) + td = unit.task_definitions.first + + task_a = FactoryBot.create(:project, unit: unit).task_for_task_definition(td) + task_b = FactoryBot.create(:project, unit: unit).task_for_task_definition(td) + missing_id = Task.maximum(:id).to_i + 100_000 + + zip_path = build_jplag_report( + comparisons: [ + { first: 'subC', second: 'subD', max: 0.8 }, # subD was deleted - processed first + { first: 'subA', second: 'subB', max: 0.9 } # both present - must still be reached + ], + files: { 'subA' => task_a.id, 'subB' => task_b.id, 'subC' => task_a.id, 'subD' => missing_id } + ) + + linked = [] + unit.stub(:create_jplag_plagiarism_link, ->(t1, t2, _warn, _max) { linked << [t1.id, t2.id] }) do + assert_nothing_raised do + unit.send(:process_jplag_plagiarism_report, zip_path, 25, false) + end + end + + assert_equal [[task_a.id, task_b.id]], linked, 'the present pair links, the deleted-task pair is skipped' + ensure + File.delete(zip_path) if zip_path && File.exist?(zip_path) + end + + private + + # Builds a minimal JPlag report zip: a topComparisons.json plus one files/// + # entry per submission, which is how process_jplag_plagiarism_report maps a comparison + # back to its task ids. + def build_jplag_report(comparisons:, files:) + path = Rails.root.join('tmp', "jplag-report-#{SecureRandom.hex(4)}.zip").to_s + FileUtils.mkdir_p(File.dirname(path)) + top = comparisons.map do |c| + { 'firstSubmission' => c[:first], 'secondSubmission' => c[:second], 'similarities' => { 'MAX' => c[:max] } } + end + Zip::File.open(path, Zip::File::CREATE) do |zip| + zip.get_output_stream('topComparisons.json') { |f| f.write(top.to_json) } + files.each do |submission, task_id| + zip.get_output_stream("files/#{submission}/#{task_id}/src.java") { |f| f.write('// code') } + end + end + path + end + + # Runs the block with credentials and MossRuby stubbed so update_moss_plagiarism_stats + # reads the given results without a real MOSS key or network call. + def run_moss_stats(_unit, results) + fake_moss = Object.new + fake_moss.define_singleton_method(:extract_results) { |*_args| results } + + credentials = Object.new + credentials.define_singleton_method(:secret_key_moss) { 'test-moss-key' } + + Doubtfire::Application.stub(:credentials, credentials) do + MossRuby.stub(:new, fake_moss) do + yield + end + end + end end diff --git a/test/models/unit_similarity_cleanup_test.rb b/test/models/unit_similarity_cleanup_test.rb new file mode 100644 index 0000000000..ccc695a29d --- /dev/null +++ b/test/models/unit_similarity_cleanup_test.rb @@ -0,0 +1,198 @@ +# frozen_string_literal: true + +require 'test_helper' + +class UnitSimilarityCleanupTest < ActiveSupport::TestCase + def test_jplag_cleanup_preserves_another_units_workspace + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + current_root = Rails.root.join('tmp', 'jplag', "unit-#{unit.id}") + tasks_dir = current_root.join('task-definition') + other_root = Rails.root.join('tmp', 'jplag', "other-unit-#{SecureRandom.hex(6)}") + sentinel = other_root.join('in-progress.sentinel') + + FileUtils.mkdir_p(tasks_dir) + FileUtils.mkdir_p(other_root) + FileUtils.touch(sentinel) + + unit.stub(:system, true) do + unit.send( + :run_jplag_on_done_files, + jplag_task_definition, + tasks_dir, + [], + Rails.root.join('tmp/jplag-results/report.jplag').to_s + ) + end + + assert_not Dir.exist?(tasks_dir), 'the completed task workspace should be removed' + assert File.exist?(sentinel), "another unit's in-progress workspace must survive cleanup" + ensure + FileUtils.rm_rf(current_root) if current_root + FileUtils.rm_rf(other_root) if other_root + end + + def test_jplag_cleanup_runs_when_the_container_command_fails + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + current_root = Rails.root.join('tmp', 'jplag', "unit-#{unit.id}") + tasks_dir = current_root.join('task-definition') + other_root = Rails.root.join('tmp', 'jplag', "other-unit-#{SecureRandom.hex(6)}") + sentinel = other_root.join('in-progress.sentinel') + + FileUtils.mkdir_p(tasks_dir) + FileUtils.mkdir_p(other_root) + FileUtils.touch(sentinel) + + system_calls = 0 + run_command = lambda do |*_command| + system_calls += 1 + system_calls < 3 + end + + error = assert_raises(RuntimeError) do + unit.stub(:system, run_command) do + unit.send( + :run_jplag_on_done_files, + jplag_task_definition, + tasks_dir, + [], + Rails.root.join('tmp/jplag-results/report.jplag').to_s + ) + end + end + + assert_equal 'Failed to run JPlag similarity check', error.message + assert_not Dir.exist?(tasks_dir), 'the failed task workspace should be removed' + assert File.exist?(sentinel), "another unit's in-progress workspace must survive failed cleanup" + ensure + FileUtils.rm_rf(current_root) if current_root + FileUtils.rm_rf(other_root) if other_root + end + + def test_jplag_unit_root_cleanup_runs_after_a_top_level_failure + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + current_root = Rails.root.join('tmp', 'jplag', "unit-#{unit.id}") + other_root = Rails.root.join('tmp', 'jplag', "other-unit-#{SecureRandom.hex(6)}") + sentinel = other_root.join('in-progress.sentinel') + + FileUtils.mkdir_p(current_root) + FileUtils.mkdir_p(other_root) + FileUtils.touch(sentinel) + + failing_definitions = Object.new + failing_definitions.define_singleton_method(:each) { raise 'simulated scan setup failure' } + + error = assert_raises(RuntimeError) do + unit.stub(:task_definitions, failing_definitions) do + unit.check_jplag_similarity(force: true) + end + end + + assert_equal 'simulated scan setup failure', error.message + assert_not Dir.exist?(current_root), 'the failed unit workspace should be removed' + assert File.exist?(sentinel), "another unit's in-progress workspace must survive unit cleanup" + ensure + FileUtils.rm_rf(current_root) if current_root + FileUtils.rm_rf(other_root) if other_root + end + + def test_hostile_unit_code_cannot_escape_workspace_or_reach_a_shell + unit = FactoryBot.create(:unit, with_students: false, task_count: 0) + token = SecureRandom.hex(6) + shell_marker = Rails.root.join("jplag-shell-marker-#{token}") + hostile_code = "../escaped-#{token};touch #{shell_marker.basename};#" + unit.update!(code: hostile_code) + + task_definition = hostile_jplag_task_definition(unit.id + 9_000_000) + current_root = Rails.root.join('tmp', 'jplag', "unit-#{unit.id}") + expected_tasks_dir = current_root.join(task_definition.id.to_s) + legacy_escape_root = Rails.root.join('tmp', 'jplag', "#{hostile_code}-#{unit.id}") + other_root = Rails.root.join('tmp', 'jplag', "unit-#{unit.id + 8_000_000}") + sentinel = other_root.join('in-progress.sentinel') + extracted_to = [] + docker_calls = [] + tasks = hostile_tasks(extracted_to) + + FileUtils.mkdir_p(other_root) + FileUtils.touch(sentinel) + + capture_system = lambda do |*argv| + docker_calls << argv + true + end + + unit.stub(:task_definitions, [task_definition]) do + unit.stub(:tasks_for_definition, tasks) do + unit.stub(:process_jplag_plagiarism_report, true) do + unit.stub(:system, capture_system) do + unit.check_jplag_similarity(force: true) + end + end + end + end + + assert_equal [expected_tasks_dir, expected_tasks_dir], extracted_to + assert_equal 3, docker_calls.length + assert docker_calls.all? { |argv| argv.length > 1 }, 'derived paths must never be passed through a command string' + assert(docker_calls.all? { |argv| argv.first == 'docker' }) + assert_includes docker_calls.last, "/tmp/jplag/unit-#{unit.id}/#{task_definition.id}/submissions" + assert_not File.exist?(shell_marker), 'unit code shell metacharacters must never execute' + assert_not Dir.exist?(legacy_escape_root), 'unit code path traversal must not create a workspace' + assert_not Dir.exist?(current_root), 'the hostile-code unit workspace should be cleaned' + assert File.exist?(sentinel), "another unit's workspace must survive hostile-code cleanup" + ensure + FileUtils.rm_rf(current_root) if current_root + FileUtils.rm_rf(legacy_escape_root) if legacy_escape_root + FileUtils.rm_rf(other_root) if other_root + FileUtils.rm_f(shell_marker) if shell_marker + end + + private + + def jplag_task_definition + task_definition = Struct.new(:plagiarism_warn_pct, :upload_requirements, :similarity_language) + .new(50, [], 'java') + task_definition.define_singleton_method(:has_task_resources?) { false } + task_definition + end + + def hostile_jplag_task_definition(id) + task_definition = Struct.new( + :id, + :similarity_language, + :upload_requirements, + :updated_at, + :name, + :plagiarism_warn_pct, + :group_set, + :abbreviation + ).new( + id, + 'java', + [{ 'type' => 'code', 'tii_check' => true, 'name' => 'source' }], + Time.zone.now, + 'Hostile code task', + 50, + nil, + 'HOSTILE' + ) + task_definition.define_singleton_method(:has_task_resources?) { false } + task_definition.define_singleton_method(:glob_for_upload_requirement) { |_index| '*' } + task_definition + end + + def hostile_tasks(extracted_to) + task_list = Array.new(2) do + task = Object.new + task.define_singleton_method(:has_pdf) { true } + task.define_singleton_method(:extract_file_from_done) do |to_path, _pattern, _destination| + extracted_to << Pathname(to_path) + end + task + end + + tasks = Object.new + tasks.define_singleton_method(:select) { |&block| task_list.select(&block) } + tasks.define_singleton_method(:where) { |_query, _time| tasks } + tasks + end +end