From c745cb31d437dce20646d418b1defba41849cd34 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 18 Aug 2026 15:34:04 +0300 Subject: [PATCH 1/5] Add admin controls for courses.mooc.fi migration status --- app/controllers/participants_controller.rb | 18 +++++++ app/models/user.rb | 50 +++++++++++++++++++ app/views/participants/show.html.erb | 10 ++++ config/routes.rb | 1 + config/site.defaults.yml | 1 + .../participants_controller_spec.rb | 48 ++++++++++++++++++ spec/models/user_spec.rb | 21 ++++++++ 7 files changed, 149 insertions(+) diff --git a/app/controllers/participants_controller.rb b/app/controllers/participants_controller.rb index 65d44bf8c..ed4243b7e 100644 --- a/app/controllers/participants_controller.rb +++ b/app/controllers/participants_controller.rb @@ -131,6 +131,24 @@ def password_reset_link @password_reset_link = @user.generate_password_reset_link end + def force_migrate_to_courses_mooc_fi + @user = User.find(params[:id]) + authorize! :view_participant_information, @user + return respond_forbidden('This feature is only available to admins') unless current_user.administrator? + return respond_forbidden('This feature is disabled for admin accounts') if @user.administrator? + + if @user.managed_externally? + return redirect_to participant_path(@user), alert: 'User is already managed by courses.mooc.fi.' + end + + result = @user.force_migrate_to_courses_mooc_fi + if result[:success] + redirect_to participant_path(@user), notice: 'User force-migrated to courses.mooc.fi.' + else + redirect_to participant_path(@user), alert: "Force migration to courses.mooc.fi failed: #{result[:error]}" + end + end + private def index_json_data result = [] diff --git a/app/models/user.rb b/app/models/user.rb index 85e18e3d3..2d9af0976 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -363,6 +363,56 @@ def post_new_user_to_courses_mooc_fi(password) end end + # Admin-triggered override of the normal login/password-change-triggered migration: creates + # the user on courses.mooc.fi with a throwaway random password nobody needs to know, since the + # user gets a real one later via password reset once courses.mooc.fi confirms the account + # (see set_password_managed_by_courses_mooc_fi). Returns the raw status/body on failure rather + # than a guessed message, since we don't control courses.mooc.fi's error schema. + def force_migrate_to_courses_mooc_fi + create_url = SiteSetting.value('courses_mooc_fi_create_user_url') + password = SecureRandom.hex(24) + + conn = Faraday.new(request: { open_timeout: 2, timeout: 10 }) do |f| + f.request :json + f.response :json + end + + response = conn.post(create_url) do |req| + req.headers['Content-Type'] = 'application/json' + req.headers['Accept'] = 'application/json' + req.headers['Authorization'] = Rails.application.secrets.tmc_server_secret_for_communicating_to_secret_project + req.body = { upstream_id: id, password: normalize_password(password) } + end + + data = response.body + if response.status == 200 && data.is_a?(Hash) && data['user'].present? + Rails.logger.info("User #{self.email} force-migrated to courses.mooc.fi by an admin") + { success: true } + else + Rails.logger.error("Force migration to courses.mooc.fi failed for user #{self.email}: status=#{response.status}, body=#{data.inspect}") + { success: false, error: "status=#{response.status}, body=#{data.inspect}" } + end + + rescue Faraday::ClientError => e + status = e.response&.dig(:status) + body = e.response&.dig(:body) + Rails.logger.error("Force migration to courses.mooc.fi errored for user #{self.email}: status=#{status}, body=#{body.inspect}") + { success: false, error: "status=#{status}, body=#{body.inspect}" } + + rescue => e + Rails.logger.error("Force migration to courses.mooc.fi unexpectedly failed for user #{self.email}: #{e.message}") + { success: false, error: e.message } + end + + def courses_mooc_fi_profile_url + return nil if courses_mooc_fi_user_id.blank? + + base_url = SiteSetting.value('courses_mooc_fi_manage_user_url') + return nil if base_url.blank? + + "#{base_url}/#{courses_mooc_fi_user_id}" + end + def password_reset_key action_tokens.find { |t| t.action == 'reset_password' } end diff --git a/app/views/participants/show.html.erb b/app/views/participants/show.html.erb index 2bc75c8d5..1f3a0cd2a 100644 --- a/app/views/participants/show.html.erb +++ b/app/views/participants/show.html.erb @@ -20,6 +20,16 @@ <% if current_user.administrator? && !@user.administrator? %>
  • <%= link_to 'Generate password reset link', password_reset_link_participant_path, class: 'btn btn-primary' %> (shown to you because you're an admin)
    • <% end %> + <% if current_user.administrator? %> +
  • Courses.mooc.fi managed: <%= @user.password_managed_by_courses_mooc_fi ? 'yes' : 'no' %> (shown to you because you're an admin)
    • +
  • Courses.mooc.fi user id: <%= @user.courses_mooc_fi_user_id.presence || 'not set' %> (shown to you because you're an admin)
    • + <% end %> + <% if current_user.administrator? && !@user.administrator? && !@user.managed_externally? %> +
  • <%= button_to 'Force migrate to courses.mooc.fi', force_migrate_to_courses_mooc_fi_participant_path, method: :post, class: 'btn btn-primary', data: { confirm: "Force migrate #{@user.email} to courses.mooc.fi now?" } %> (shown to you because you're an admin)
    • + <% end %> + <% if current_user.administrator? && @user.courses_mooc_fi_profile_url %> +
  • <%= link_to 'View on courses.mooc.fi', @user.courses_mooc_fi_profile_url, target: '_blank', rel: 'noopener', class: 'btn btn-primary' %> (shown to you because you're an admin)
    • + <% end %> diff --git a/config/routes.rb b/config/routes.rb index 77da25dff..3dc83e7a7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -276,6 +276,7 @@ end member do get 'password_reset_link', to: 'participants#password_reset_link' + post 'force_migrate_to_courses_mooc_fi', to: 'participants#force_migrate_to_courses_mooc_fi' end end diff --git a/config/site.defaults.yml b/config/site.defaults.yml index 36ea5a787..deaa523b2 100644 --- a/config/site.defaults.yml +++ b/config/site.defaults.yml @@ -143,3 +143,4 @@ teacher_manual_url: http://testmycode.github.io/tmc-server/usermanual/ courses_mooc_fi_auth_url: courses_mooc_fi_update_password_url: courses_mooc_fi_create_user_url: +courses_mooc_fi_manage_user_url: diff --git a/spec/controllers/participants_controller_spec.rb b/spec/controllers/participants_controller_spec.rb index 518007ba7..310aba3fc 100644 --- a/spec/controllers/participants_controller_spec.rb +++ b/spec/controllers/participants_controller_spec.rb @@ -27,4 +27,52 @@ end end end + + describe 'POST /force_migrate_to_courses_mooc_fi' do + describe 'when logged in as a non-admin' do + before :each do + controller.current_user = @user + end + + it 'is forbidden' do + post :force_migrate_to_courses_mooc_fi, params: { id: @user.id } + expect(response.code.to_i).to eq(403) + end + end + + describe 'when logged in as an admin' do + before :each do + controller.current_user = FactoryBot.create(:admin) + end + + it 'is forbidden when the target user is an admin' do + admin_target = FactoryBot.create(:admin) + expect_any_instance_of(User).not_to receive(:force_migrate_to_courses_mooc_fi) + post :force_migrate_to_courses_mooc_fi, params: { id: admin_target.id } + expect(response.code.to_i).to eq(403) + end + + it 'refuses when the user is already managed externally' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).not_to receive(:force_migrate_to_courses_mooc_fi) + post :force_migrate_to_courses_mooc_fi, params: { id: @user.id } + expect(response).to redirect_to(participant_path(@user)) + expect(flash[:alert]).to match(/already managed/) + end + + it 'migrates the user when they are not yet managed externally and the migration succeeds' do + expect_any_instance_of(User).to receive(:force_migrate_to_courses_mooc_fi).and_return({ success: true }) + post :force_migrate_to_courses_mooc_fi, params: { id: @user.id } + expect(response).to redirect_to(participant_path(@user)) + expect(flash[:notice]).to match(/force-migrated/) + end + + it 'shows the exact error when the migration fails' do + expect_any_instance_of(User).to receive(:force_migrate_to_courses_mooc_fi).and_return({ success: false, error: 'status=422, body={"error"=>"boom"}' }) + post :force_migrate_to_courses_mooc_fi, params: { id: @user.id } + expect(response).to redirect_to(participant_path(@user)) + expect(flash[:alert]).to match(/status=422/) + end + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e6e476d8d..fa196c5d6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -270,6 +270,27 @@ expect(User.authenticate('root', 'ilikecookies')).to be_nil end + describe 'courses_mooc_fi_profile_url' do + it 'is nil when the user has no courses.mooc.fi id' do + user = User.create!(login: 'localuser', password: 'secret123', email: 'localuser@example.com') + SiteSetting.all_settings['courses_mooc_fi_manage_user_url'] = 'https://courses.mooc.fi/manage/users' + expect(user.courses_mooc_fi_profile_url).to be_nil + end + + it 'is nil when courses_mooc_fi_manage_user_url is not configured' do + user = User.create!(login: 'manageduser', password: 'secret123', email: 'managed@example.com', courses_mooc_fi_user_id: SecureRandom.uuid) + SiteSetting.all_settings['courses_mooc_fi_manage_user_url'] = nil + expect(user.courses_mooc_fi_profile_url).to be_nil + end + + it 'builds the profile url from the configured base url and the courses.mooc.fi id' do + id = SecureRandom.uuid + user = User.create!(login: 'manageduser', password: 'secret123', email: 'managed@example.com', courses_mooc_fi_user_id: id) + SiteSetting.all_settings['courses_mooc_fi_manage_user_url'] = 'https://courses.mooc.fi/manage/users' + expect(user.courses_mooc_fi_profile_url).to eq("https://courses.mooc.fi/manage/users/#{id}") + end + end + describe 'migrating to courses.mooc.fi on login' do it 'posts a locally-managed user on successful authentication' do user = User.create!(login: 'localuser', password: 'secret123', email: 'localuser@example.com') From 09e914baa8b2d27d0c58d232b253982e22ffb730 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 18 Aug 2026 19:37:20 +0300 Subject: [PATCH 2/5] Rework force migrate and show live migration status --- app/controllers/api/v8/users_controller.rb | 20 +++- app/controllers/participants_controller.rb | 13 ++- app/models/user.rb | 94 +++++++++++++------ app/views/participants/show.html.erb | 18 +++- config/site.defaults.yml | 2 + .../api/v8/users_controller_spec.rb | 29 ++++++ .../participants_controller_spec.rb | 53 ++++++++++- 7 files changed, 194 insertions(+), 35 deletions(-) diff --git a/app/controllers/api/v8/users_controller.rb b/app/controllers/api/v8/users_controller.rb index 097b7a4ff..d1adbd208 100644 --- a/app/controllers/api/v8/users_controller.rb +++ b/app/controllers/api/v8/users_controller.rb @@ -58,11 +58,19 @@ class UsersController < Api::V8::BaseController swagger_path '/api/v8/users/{user_id}/set_password_managed_by_courses_mooc_fi' do operation :post do - key :description, 'Sets the boolean password_managed_by_courses_mooc_fi for the user with the given id to true.' + key :description, 'Sets the boolean password_managed_by_courses_mooc_fi for the user with the given id to true and records the courses.mooc.fi user id.' key :operationId, 'setPasswordManagedByCoursesMoocFi' key :produces, ['application/json'] key :tags, ['user'] parameter '$ref': '#/parameters/user_id' + parameter do + key :name, :courses_mooc_fi_user_id + key :in, :formData + key :description, "The user's id on courses.mooc.fi" + key :required, true + key :type, :string + end + response 400, '$ref': '#/responses/error' response 403, '$ref': '#/responses/error' response 404, '$ref': '#/responses/error' response 200 do @@ -228,8 +236,14 @@ def destroy def set_password_managed_by_courses_mooc_fi only_admins! + if params[:courses_mooc_fi_user_id].blank? + return render json: { + errors: { courses_mooc_fi_user_id: ['must be present'] } + }, status: :bad_request + end + + user = User.find_by!(id: params[:id]) User.transaction do - user = User.find_by!(id: params[:id]) user.password_managed_by_courses_mooc_fi = true user.password_hash = nil user.salt = nil @@ -241,7 +255,7 @@ def set_password_managed_by_courses_mooc_fi } end render json: { - errors: @user.errors + errors: user.errors }, status: :bad_request end diff --git a/app/controllers/participants_controller.rb b/app/controllers/participants_controller.rb index ed4243b7e..a7fb61fcb 100644 --- a/app/controllers/participants_controller.rb +++ b/app/controllers/participants_controller.rb @@ -81,6 +81,8 @@ def show add_breadcrumb 'Participants', :participants_path add_breadcrumb @user.username, participant_path(@user) @app_data = JSON.pretty_generate(JSON.parse(@user.user_app_data.to_json)) + @courses_mooc_fi_status = @user.courses_mooc_fi_migration_status + @courses_mooc_fi_status_label = courses_mooc_fi_status_label(@user, @courses_mooc_fi_status) else add_breadcrumb 'My stats', participant_path(@user) end @@ -143,13 +145,22 @@ def force_migrate_to_courses_mooc_fi result = @user.force_migrate_to_courses_mooc_fi if result[:success] - redirect_to participant_path(@user), notice: 'User force-migrated to courses.mooc.fi.' + redirect_to participant_path(@user), notice: + "User force-migrated to courses.mooc.fi (id: #{result[:courses_mooc_fi_user_id]}). " \ + "They have no password yet — use 'Generate password reset link' below to give them one." else redirect_to participant_path(@user), alert: "Force migration to courses.mooc.fi failed: #{result[:error]}" end end private + def courses_mooc_fi_status_label(user, status) + return 'Fully migrated' if user.managed_externally? + return 'Broken: flagged as migrated locally but missing the target id' if user.externally_managed_without_target? + return 'Inconsistent: courses.mooc.fi already has a password, but it isn\'t linked locally' if status&.dig(:password_set) + + 'Not migrated' + end def index_json_data result = [] @participants.each do |user| diff --git a/app/models/user.rb b/app/models/user.rb index 2d9af0976..6969a824f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -198,10 +198,7 @@ def externally_managed_without_target? def authenticate_via_courses_mooc_fi(submitted_password) auth_url = SiteSetting.value('courses_mooc_fi_auth_url') - conn = Faraday.new(request: { open_timeout: 2, timeout: 10 }) do |f| - f.request :json - f.response :json - end + conn = courses_mooc_fi_connection response = conn.post(auth_url) do |req| req.headers['Content-Type'] = 'application/json' @@ -247,10 +244,7 @@ def authenticate_via_courses_mooc_fi(submitted_password) def update_password_via_courses_mooc_fi(old_password, new_password) update_url = SiteSetting.value('courses_mooc_fi_update_password_url') - conn = Faraday.new(request: { open_timeout: 2, timeout: 10 }) do |f| - f.request :json - f.response :json - end + conn = courses_mooc_fi_connection begin response = conn.post(update_url) do |req| @@ -305,10 +299,7 @@ def post_new_user_to_courses_mooc_fi(password) # Best-effort call made inline during logins/password changes: tight timeouts so a hung # courses.mooc.fi can't stall authentication (migration retries on the next attempt). - conn = Faraday.new(request: { open_timeout: 2, timeout: 10 }) do |f| - f.request :json - f.response :json - end + conn = courses_mooc_fi_connection begin response = conn.post(create_url) do |req| @@ -363,31 +354,32 @@ def post_new_user_to_courses_mooc_fi(password) end end - # Admin-triggered override of the normal login/password-change-triggered migration: creates - # the user on courses.mooc.fi with a throwaway random password nobody needs to know, since the - # user gets a real one later via password reset once courses.mooc.fi confirms the account - # (see set_password_managed_by_courses_mooc_fi). Returns the raw status/body on failure rather - # than a guessed message, since we don't control courses.mooc.fi's error schema. + # Ensures the courses.mooc.fi shadow account exists (reusing the get-or-create lookup, a no-op + # if it's already there), then hands off password ownership locally -- never sends a password, + # so the user is passwordless until generate_password_reset_link runs. Local state is only + # updated after a confirmed remote success, so a failure (e.g. re-creating a soft-deleted linked + # id collides on the primary key) never leaves the account half-migrated. def force_migrate_to_courses_mooc_fi - create_url = SiteSetting.value('courses_mooc_fi_create_user_url') - password = SecureRandom.hex(24) + ensure_url = SiteSetting.value('courses_mooc_fi_users_by_upstream_id_url') - conn = Faraday.new(request: { open_timeout: 2, timeout: 10 }) do |f| - f.request :json - f.response :json - end + conn = courses_mooc_fi_connection - response = conn.post(create_url) do |req| - req.headers['Content-Type'] = 'application/json' + response = conn.get("#{ensure_url}/#{id}") do |req| req.headers['Accept'] = 'application/json' req.headers['Authorization'] = Rails.application.secrets.tmc_server_secret_for_communicating_to_secret_project - req.body = { upstream_id: id, password: normalize_password(password) } end data = response.body - if response.status == 200 && data.is_a?(Hash) && data['user'].present? - Rails.logger.info("User #{self.email} force-migrated to courses.mooc.fi by an admin") - { success: true } + if response.status == 200 && data.is_a?(Hash) && data['id'].present? + update!( + password_managed_by_courses_mooc_fi: true, + courses_mooc_fi_user_id: data['id'], + argon_hash: nil, + salt: nil, + password_hash: nil + ) + Rails.logger.info("User #{self.email} force-migrated to courses.mooc.fi by an admin (id=#{data['id']})") + { success: true, courses_mooc_fi_user_id: data['id'] } else Rails.logger.error("Force migration to courses.mooc.fi failed for user #{self.email}: status=#{response.status}, body=#{data.inspect}") { success: false, error: "status=#{response.status}, body=#{data.inspect}" } @@ -404,6 +396,43 @@ def force_migrate_to_courses_mooc_fi { success: false, error: e.message } end + # Live, display-only read of courses.mooc.fi's view of this user -- never gates any action. + # nil means genuinely unknown (unconfigured, network error, unexpected response), not "not migrated". + def courses_mooc_fi_migration_status + status_url = SiteSetting.value('courses_mooc_fi_user_status_url') + return nil if status_url.blank? + + conn = courses_mooc_fi_connection + + response = conn.get("#{status_url}/#{id}/status") do |req| + req.headers['Accept'] = 'application/json' + req.headers['Authorization'] = Rails.application.secrets.tmc_server_secret_for_communicating_to_secret_project + end + + data = response.body + unless response.status == 200 && data.is_a?(Hash) + Rails.logger.error("Fetching courses.mooc.fi migration status failed for user #{self.email}: status=#{response.status}, body=#{data.inspect}") + return nil + end + + { + shadow_user_exists: data['shadow_user_exists'], + courses_mooc_fi_user_id: data['courses_mooc_fi_user_id'], + password_set: data['password_set'], + deleted_at: data['deleted_at'] + } + + rescue Faraday::ClientError => e + status = e.response&.dig(:status) + body = e.response&.dig(:body) + Rails.logger.error("Fetching courses.mooc.fi migration status errored for user #{self.email}: status=#{status}, body=#{body.inspect}") + nil + + rescue => e + Rails.logger.error("Fetching courses.mooc.fi migration status unexpectedly failed for user #{self.email}: #{e.message}") + nil + end + def courses_mooc_fi_profile_url return nil if courses_mooc_fi_user_id.blank? @@ -550,6 +579,13 @@ def processing_submissions_count_for_exercise(exercise_name, course_id) end private + def courses_mooc_fi_connection + Faraday.new(request: { open_timeout: 2, timeout: 10 }) do |f| + f.request :json + f.response :json + end + end + def course_ids_arel courses = Course.arel_table submissions = Submission.arel_table diff --git a/app/views/participants/show.html.erb b/app/views/participants/show.html.erb index 1f3a0cd2a..ea5218d85 100644 --- a/app/views/participants/show.html.erb +++ b/app/views/participants/show.html.erb @@ -23,9 +23,25 @@ <% if current_user.administrator? %>
  • Courses.mooc.fi managed: <%= @user.password_managed_by_courses_mooc_fi ? 'yes' : 'no' %> (shown to you because you're an admin)
  • Courses.mooc.fi user id: <%= @user.courses_mooc_fi_user_id.presence || 'not set' %> (shown to you because you're an admin)
    • +
  • Courses.mooc.fi status: <%= @courses_mooc_fi_status_label %> (shown to you because you're an admin)
    • + <% if @courses_mooc_fi_status %> +
  • + Courses.mooc.fi says: account exists=<%= @courses_mooc_fi_status[:shadow_user_exists] %>, + id=<%= @courses_mooc_fi_status[:courses_mooc_fi_user_id] || 'none' %>, + password set=<%= @courses_mooc_fi_status[:password_set] %><% if @courses_mooc_fi_status[:deleted_at] %>, deleted at <%= @courses_mooc_fi_status[:deleted_at] %><% end %> + (shown to you because you're an admin) +
    • + <% else %> +
  • Could not reach courses.mooc.fi to confirm live status. (shown to you because you're an admin)
    • + <% end %> <% end %> <% if current_user.administrator? && !@user.administrator? && !@user.managed_externally? %> -
  • <%= button_to 'Force migrate to courses.mooc.fi', force_migrate_to_courses_mooc_fi_participant_path, method: :post, class: 'btn btn-primary', data: { confirm: "Force migrate #{@user.email} to courses.mooc.fi now?" } %> (shown to you because you're an admin)
    • + <% confirm_text = if @courses_mooc_fi_status&.dig(:password_set) + "#{@user.email} already has a working password on courses.mooc.fi. Force migrating will still wipe their local tmc-server password and relink them to that existing account. Continue?" + else + "Force migrate #{@user.email} to courses.mooc.fi now?" + end %> +
  • <%= button_to 'Force migrate to courses.mooc.fi', force_migrate_to_courses_mooc_fi_participant_path, method: :post, class: 'btn btn-primary', data: { confirm: confirm_text } %> (shown to you because you're an admin)
    • <% end %> <% if current_user.administrator? && @user.courses_mooc_fi_profile_url %>
  • <%= link_to 'View on courses.mooc.fi', @user.courses_mooc_fi_profile_url, target: '_blank', rel: 'noopener', class: 'btn btn-primary' %> (shown to you because you're an admin)
    • diff --git a/config/site.defaults.yml b/config/site.defaults.yml index deaa523b2..1614414a4 100644 --- a/config/site.defaults.yml +++ b/config/site.defaults.yml @@ -144,3 +144,5 @@ courses_mooc_fi_auth_url: courses_mooc_fi_update_password_url: courses_mooc_fi_create_user_url: courses_mooc_fi_manage_user_url: +courses_mooc_fi_users_by_upstream_id_url: +courses_mooc_fi_user_status_url: diff --git a/spec/controllers/api/v8/users_controller_spec.rb b/spec/controllers/api/v8/users_controller_spec.rb index 8ebe0b271..fee8b6c12 100644 --- a/spec/controllers/api/v8/users_controller_spec.rb +++ b/spec/controllers/api/v8/users_controller_spec.rb @@ -163,4 +163,33 @@ def do_update(old_password) expect(response).to have_http_status(200) end end + + describe 'POST set_password_managed_by_courses_mooc_fi' do + before :each do + controller.current_user = admin + end + + it 'rejects a blank courses_mooc_fi_user_id with a clean 400 instead of erroring' do + post :set_password_managed_by_courses_mooc_fi, params: { id: user.id } + + expect(response).to have_http_status(400) + expect(user.reload.password_managed_by_courses_mooc_fi).to eq(false) + end + + it 'marks the user as managed and clears the local password on success' do + user.password = 'oldpassword' + user.save! + moocfi_id = SecureRandom.uuid + + post :set_password_managed_by_courses_mooc_fi, params: { id: user.id, courses_mooc_fi_user_id: moocfi_id } + + expect(response).to have_http_status(200) + user.reload + expect(user.password_managed_by_courses_mooc_fi).to eq(true) + expect(user.courses_mooc_fi_user_id).to eq(moocfi_id) + expect(user.argon_hash).to be_nil + expect(user.salt).to be_nil + expect(user.password_hash).to be_nil + end + end end diff --git a/spec/controllers/participants_controller_spec.rb b/spec/controllers/participants_controller_spec.rb index 310aba3fc..a50bcfda4 100644 --- a/spec/controllers/participants_controller_spec.rb +++ b/spec/controllers/participants_controller_spec.rb @@ -61,10 +61,11 @@ end it 'migrates the user when they are not yet managed externally and the migration succeeds' do - expect_any_instance_of(User).to receive(:force_migrate_to_courses_mooc_fi).and_return({ success: true }) + expect_any_instance_of(User).to receive(:force_migrate_to_courses_mooc_fi).and_return({ success: true, courses_mooc_fi_user_id: 'abc-123' }) post :force_migrate_to_courses_mooc_fi, params: { id: @user.id } expect(response).to redirect_to(participant_path(@user)) expect(flash[:notice]).to match(/force-migrated/) + expect(flash[:notice]).to match(/abc-123/) end it 'shows the exact error when the migration fails' do @@ -75,4 +76,54 @@ end end end + + describe 'GET /show' do + describe 'when logged in as an admin' do + before :each do + controller.current_user = FactoryBot.create(:admin) + end + + it 'shows the migration status when courses.mooc.fi confirms the user is not migrated' do + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return( + { shadow_user_exists: false, courses_mooc_fi_user_id: nil, password_set: false, deleted_at: nil } + ) + get :show, params: { id: @user.id } + expect(response).to be_successful + expect(assigns(:courses_mooc_fi_status_label)).to eq('Not migrated') + end + + it 'flags an inconsistency when courses.mooc.fi already has a password but the user is not linked locally' do + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return( + { shadow_user_exists: true, courses_mooc_fi_user_id: SecureRandom.uuid, password_set: true, deleted_at: nil } + ) + get :show, params: { id: @user.id } + expect(response).to be_successful + expect(assigns(:courses_mooc_fi_status_label)).to match(/Inconsistent/) + end + + it 'shows fully migrated when the user is already managed externally, regardless of the live status' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return(nil) + get :show, params: { id: @user.id } + expect(response).to be_successful + expect(assigns(:courses_mooc_fi_status_label)).to eq('Fully migrated') + end + + it 'flags the broken state when managed locally but missing the target id' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: nil) + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return(nil) + get :show, params: { id: @user.id } + expect(response).to be_successful + expect(assigns(:courses_mooc_fi_status_label)).to match(/Broken/) + end + + it 'degrades gracefully when courses.mooc.fi cannot be reached' do + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return(nil) + get :show, params: { id: @user.id } + expect(response).to be_successful + expect(assigns(:courses_mooc_fi_status)).to be_nil + expect(assigns(:courses_mooc_fi_status_label)).to eq('Not migrated') + end + end + end end From 1a4e67ca511cc9aae067021381ca340ab61d13a5 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 18 Aug 2026 20:16:36 +0300 Subject: [PATCH 3/5] Allow force migrate to fix drifted courses.mooc.fi state --- app/controllers/participants_controller.rb | 14 +++- app/views/participants/show.html.erb | 6 +- .../participants_controller_spec.rb | 72 ++++++++++++++++++- 3 files changed, 86 insertions(+), 6 deletions(-) diff --git a/app/controllers/participants_controller.rb b/app/controllers/participants_controller.rb index a7fb61fcb..c4a738ebc 100644 --- a/app/controllers/participants_controller.rb +++ b/app/controllers/participants_controller.rb @@ -83,6 +83,7 @@ def show @app_data = JSON.pretty_generate(JSON.parse(@user.user_app_data.to_json)) @courses_mooc_fi_status = @user.courses_mooc_fi_migration_status @courses_mooc_fi_status_label = courses_mooc_fi_status_label(@user, @courses_mooc_fi_status) + @courses_mooc_fi_force_migrate_available = !@user.managed_externally? || courses_mooc_fi_account_missing?(@courses_mooc_fi_status) else add_breadcrumb 'My stats', participant_path(@user) end @@ -139,7 +140,7 @@ def force_migrate_to_courses_mooc_fi return respond_forbidden('This feature is only available to admins') unless current_user.administrator? return respond_forbidden('This feature is disabled for admin accounts') if @user.administrator? - if @user.managed_externally? + if @user.managed_externally? && !courses_mooc_fi_account_missing?(@user.courses_mooc_fi_migration_status) return redirect_to participant_path(@user), alert: 'User is already managed by courses.mooc.fi.' end @@ -154,9 +155,18 @@ def force_migrate_to_courses_mooc_fi end private + # nil means the live status is unknown (unreachable/unconfigured) -- trust the local flag + # instead of treating the account as missing. + def courses_mooc_fi_account_missing?(status) + status.present? && (!status[:shadow_user_exists] || status[:deleted_at].present?) + end + def courses_mooc_fi_status_label(user, status) - return 'Fully migrated' if user.managed_externally? return 'Broken: flagged as migrated locally but missing the target id' if user.externally_managed_without_target? + if user.managed_externally? + return "Broken: flagged as migrated locally, but courses.mooc.fi doesn't have a live account for this user" if courses_mooc_fi_account_missing?(status) + return 'Fully migrated' + end return 'Inconsistent: courses.mooc.fi already has a password, but it isn\'t linked locally' if status&.dig(:password_set) 'Not migrated' diff --git a/app/views/participants/show.html.erb b/app/views/participants/show.html.erb index ea5218d85..f88e9938a 100644 --- a/app/views/participants/show.html.erb +++ b/app/views/participants/show.html.erb @@ -35,8 +35,10 @@
  • Could not reach courses.mooc.fi to confirm live status. (shown to you because you're an admin)
    • <% end %> <% end %> - <% if current_user.administrator? && !@user.administrator? && !@user.managed_externally? %> - <% confirm_text = if @courses_mooc_fi_status&.dig(:password_set) + <% if current_user.administrator? && !@user.administrator? && @courses_mooc_fi_force_migrate_available %> + <% confirm_text = if @user.managed_externally? + "#{@user.email} is flagged as migrated locally, but courses.mooc.fi doesn't have a live account for them. Force migrating will (re)create the account on courses.mooc.fi and relink them to it. Continue?" + elsif @courses_mooc_fi_status&.dig(:password_set) "#{@user.email} already has a working password on courses.mooc.fi. Force migrating will still wipe their local tmc-server password and relink them to that existing account. Continue?" else "Force migrate #{@user.email} to courses.mooc.fi now?" diff --git a/spec/controllers/participants_controller_spec.rb b/spec/controllers/participants_controller_spec.rb index a50bcfda4..e64c613c0 100644 --- a/spec/controllers/participants_controller_spec.rb +++ b/spec/controllers/participants_controller_spec.rb @@ -52,14 +52,48 @@ expect(response.code.to_i).to eq(403) end - it 'refuses when the user is already managed externally' do + it 'refuses when the user is already managed externally and courses.mooc.fi confirms the account is live' do @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return( + { shadow_user_exists: true, courses_mooc_fi_user_id: @user.courses_mooc_fi_user_id, password_set: true, deleted_at: nil } + ) expect_any_instance_of(User).not_to receive(:force_migrate_to_courses_mooc_fi) post :force_migrate_to_courses_mooc_fi, params: { id: @user.id } expect(response).to redirect_to(participant_path(@user)) expect(flash[:alert]).to match(/already managed/) end + it 'refuses when the user is already managed externally and the live status is unknown' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return(nil) + expect_any_instance_of(User).not_to receive(:force_migrate_to_courses_mooc_fi) + post :force_migrate_to_courses_mooc_fi, params: { id: @user.id } + expect(response).to redirect_to(participant_path(@user)) + expect(flash[:alert]).to match(/already managed/) + end + + it 'allows re-migrating when managed externally locally but courses.mooc.fi confirms the account is gone' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return( + { shadow_user_exists: false, courses_mooc_fi_user_id: nil, password_set: false, deleted_at: nil } + ) + expect_any_instance_of(User).to receive(:force_migrate_to_courses_mooc_fi).and_return({ success: true, courses_mooc_fi_user_id: 'new-id' }) + post :force_migrate_to_courses_mooc_fi, params: { id: @user.id } + expect(response).to redirect_to(participant_path(@user)) + expect(flash[:notice]).to match(/force-migrated/) + end + + it 'allows re-migrating when managed externally locally but the linked courses.mooc.fi account was deleted' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return( + { shadow_user_exists: true, courses_mooc_fi_user_id: @user.courses_mooc_fi_user_id, password_set: false, deleted_at: Time.current } + ) + expect_any_instance_of(User).to receive(:force_migrate_to_courses_mooc_fi).and_return({ success: true, courses_mooc_fi_user_id: 'new-id' }) + post :force_migrate_to_courses_mooc_fi, params: { id: @user.id } + expect(response).to redirect_to(participant_path(@user)) + expect(flash[:notice]).to match(/force-migrated/) + end + it 'migrates the user when they are not yet managed externally and the migration succeeds' do expect_any_instance_of(User).to receive(:force_migrate_to_courses_mooc_fi).and_return({ success: true, courses_mooc_fi_user_id: 'abc-123' }) post :force_migrate_to_courses_mooc_fi, params: { id: @user.id } @@ -101,12 +135,46 @@ expect(assigns(:courses_mooc_fi_status_label)).to match(/Inconsistent/) end - it 'shows fully migrated when the user is already managed externally, regardless of the live status' do + it 'shows fully migrated when the user is already managed externally and courses.mooc.fi confirms the account is live' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return( + { shadow_user_exists: true, courses_mooc_fi_user_id: @user.courses_mooc_fi_user_id, password_set: true, deleted_at: nil } + ) + get :show, params: { id: @user.id } + expect(response).to be_successful + expect(assigns(:courses_mooc_fi_status_label)).to eq('Fully migrated') + expect(assigns(:courses_mooc_fi_force_migrate_available)).to eq(false) + end + + it 'shows fully migrated when the user is already managed externally and the live status is unknown' do @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return(nil) get :show, params: { id: @user.id } expect(response).to be_successful expect(assigns(:courses_mooc_fi_status_label)).to eq('Fully migrated') + expect(assigns(:courses_mooc_fi_force_migrate_available)).to eq(false) + end + + it 'flags drift and re-enables force migrate when locally managed but courses.mooc.fi has no live account' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return( + { shadow_user_exists: false, courses_mooc_fi_user_id: nil, password_set: false, deleted_at: nil } + ) + get :show, params: { id: @user.id } + expect(response).to be_successful + expect(assigns(:courses_mooc_fi_status_label)).to match(/Broken/) + expect(assigns(:courses_mooc_fi_force_migrate_available)).to eq(true) + end + + it 'flags drift and re-enables force migrate when the linked courses.mooc.fi account was deleted' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:courses_mooc_fi_migration_status).and_return( + { shadow_user_exists: true, courses_mooc_fi_user_id: @user.courses_mooc_fi_user_id, password_set: false, deleted_at: Time.current } + ) + get :show, params: { id: @user.id } + expect(response).to be_successful + expect(assigns(:courses_mooc_fi_status_label)).to match(/Broken/) + expect(assigns(:courses_mooc_fi_force_migrate_available)).to eq(true) end it 'flags the broken state when managed locally but missing the target id' do From 6da84e0a76b7e1f818a0073eaf75a23aa190da49 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 18 Aug 2026 20:30:05 +0300 Subject: [PATCH 4/5] Consolidate courses.mooc.fi urls into a single base url --- app/models/user.rb | 25 ++++++++++++------------- config/site.defaults.yml | 9 ++------- spec/models/user_spec.rb | 8 ++++---- 3 files changed, 18 insertions(+), 24 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 6969a824f..a152b6ae3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -196,7 +196,7 @@ def externally_managed_without_target? def authenticate_via_courses_mooc_fi(submitted_password) - auth_url = SiteSetting.value('courses_mooc_fi_auth_url') + auth_url = courses_mooc_fi_url('/api/v0/tmc-server/users/authenticate') conn = courses_mooc_fi_connection @@ -242,7 +242,7 @@ def authenticate_via_courses_mooc_fi(submitted_password) def update_password_via_courses_mooc_fi(old_password, new_password) - update_url = SiteSetting.value('courses_mooc_fi_update_password_url') + update_url = courses_mooc_fi_url('/api/v0/tmc-server/users/change-password') conn = courses_mooc_fi_connection @@ -295,7 +295,7 @@ def update_password_via_courses_mooc_fi(old_password, new_password) def post_new_user_to_courses_mooc_fi(password) Rails.logger.info("Posting new user #{self.email} to courses.mooc.fi") - create_url = SiteSetting.value('courses_mooc_fi_create_user_url') + create_url = courses_mooc_fi_url('/api/v0/tmc-server/users/create') # Best-effort call made inline during logins/password changes: tight timeouts so a hung # courses.mooc.fi can't stall authentication (migration retries on the next attempt). @@ -360,11 +360,9 @@ def post_new_user_to_courses_mooc_fi(password) # updated after a confirmed remote success, so a failure (e.g. re-creating a soft-deleted linked # id collides on the primary key) never leaves the account half-migrated. def force_migrate_to_courses_mooc_fi - ensure_url = SiteSetting.value('courses_mooc_fi_users_by_upstream_id_url') - conn = courses_mooc_fi_connection - response = conn.get("#{ensure_url}/#{id}") do |req| + response = conn.get(courses_mooc_fi_url("/api/v0/tmc-server/users-by-upstream-id/#{id}")) do |req| req.headers['Accept'] = 'application/json' req.headers['Authorization'] = Rails.application.secrets.tmc_server_secret_for_communicating_to_secret_project end @@ -399,12 +397,11 @@ def force_migrate_to_courses_mooc_fi # Live, display-only read of courses.mooc.fi's view of this user -- never gates any action. # nil means genuinely unknown (unconfigured, network error, unexpected response), not "not migrated". def courses_mooc_fi_migration_status - status_url = SiteSetting.value('courses_mooc_fi_user_status_url') - return nil if status_url.blank? + return nil if SiteSetting.value('courses_mooc_fi_base_url').blank? conn = courses_mooc_fi_connection - response = conn.get("#{status_url}/#{id}/status") do |req| + response = conn.get(courses_mooc_fi_url("/api/v0/tmc-server/users-by-upstream-id/#{id}/status")) do |req| req.headers['Accept'] = 'application/json' req.headers['Authorization'] = Rails.application.secrets.tmc_server_secret_for_communicating_to_secret_project end @@ -435,11 +432,9 @@ def courses_mooc_fi_migration_status def courses_mooc_fi_profile_url return nil if courses_mooc_fi_user_id.blank? + return nil if SiteSetting.value('courses_mooc_fi_base_url').blank? - base_url = SiteSetting.value('courses_mooc_fi_manage_user_url') - return nil if base_url.blank? - - "#{base_url}/#{courses_mooc_fi_user_id}" + courses_mooc_fi_url("/manage/users/#{courses_mooc_fi_user_id}") end def password_reset_key @@ -586,6 +581,10 @@ def courses_mooc_fi_connection end end + def courses_mooc_fi_url(path) + "#{SiteSetting.value('courses_mooc_fi_base_url')}#{path}" + end + def course_ids_arel courses = Course.arel_table submissions = Submission.arel_table diff --git a/config/site.defaults.yml b/config/site.defaults.yml index 1614414a4..d4c737d59 100644 --- a/config/site.defaults.yml +++ b/config/site.defaults.yml @@ -139,10 +139,5 @@ course_instruction_page: http://mooc.fi/courses/general/ohjelmointi/ # Teacher manual link teacher_manual_url: http://testmycode.github.io/tmc-server/usermanual/ -# URLs for password management via courses.mooc.fi -courses_mooc_fi_auth_url: -courses_mooc_fi_update_password_url: -courses_mooc_fi_create_user_url: -courses_mooc_fi_manage_user_url: -courses_mooc_fi_users_by_upstream_id_url: -courses_mooc_fi_user_status_url: +# Base URL for password management and admin lookups via courses.mooc.fi, e.g. https://courses.mooc.fi +courses_mooc_fi_base_url: diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fa196c5d6..571f2ae03 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -273,20 +273,20 @@ describe 'courses_mooc_fi_profile_url' do it 'is nil when the user has no courses.mooc.fi id' do user = User.create!(login: 'localuser', password: 'secret123', email: 'localuser@example.com') - SiteSetting.all_settings['courses_mooc_fi_manage_user_url'] = 'https://courses.mooc.fi/manage/users' + SiteSetting.all_settings['courses_mooc_fi_base_url'] = 'https://courses.mooc.fi' expect(user.courses_mooc_fi_profile_url).to be_nil end - it 'is nil when courses_mooc_fi_manage_user_url is not configured' do + it 'is nil when courses_mooc_fi_base_url is not configured' do user = User.create!(login: 'manageduser', password: 'secret123', email: 'managed@example.com', courses_mooc_fi_user_id: SecureRandom.uuid) - SiteSetting.all_settings['courses_mooc_fi_manage_user_url'] = nil + SiteSetting.all_settings['courses_mooc_fi_base_url'] = nil expect(user.courses_mooc_fi_profile_url).to be_nil end it 'builds the profile url from the configured base url and the courses.mooc.fi id' do id = SecureRandom.uuid user = User.create!(login: 'manageduser', password: 'secret123', email: 'managed@example.com', courses_mooc_fi_user_id: id) - SiteSetting.all_settings['courses_mooc_fi_manage_user_url'] = 'https://courses.mooc.fi/manage/users' + SiteSetting.all_settings['courses_mooc_fi_base_url'] = 'https://courses.mooc.fi' expect(user.courses_mooc_fi_profile_url).to eq("https://courses.mooc.fi/manage/users/#{id}") end end From 5b1e1406e353f5f1a3f488473af3a916c618d0b1 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 18 Aug 2026 20:39:34 +0300 Subject: [PATCH 5/5] Fix order-dependent assertion in course assistants spec --- spec/controllers/setup/course_assistans_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/setup/course_assistans_controller_spec.rb b/spec/controllers/setup/course_assistans_controller_spec.rb index 2f5f9023b..7ea9f7d74 100644 --- a/spec/controllers/setup/course_assistans_controller_spec.rb +++ b/spec/controllers/setup/course_assistans_controller_spec.rb @@ -24,7 +24,7 @@ user3 = FactoryBot.create(:user) @course.assistants << [user1, user2, user3] get :index, params: { organization_id: @organization.slug, course_id: @course.id } - expect(assigns(:assistants)).to eq([user1, user2, user3]) + expect(assigns(:assistants).sort_by(&:id)).to eq([user1, user2, user3].sort_by(&:id)) end end