From 8f81f7b77be6b58deaa0f2213ea42c9f3e5bcd59 Mon Sep 17 00:00:00 2001 From: Antti Leinonen Date: Fri, 3 Jul 2026 10:53:56 +0300 Subject: [PATCH] Migrate existing users to courses.mooc.fi on login and password changes --- app/controllers/api/v8/users_controller.rb | 4 ++ .../password_reset_keys_controller.rb | 2 + app/controllers/settings_controller.rb | 2 + app/controllers/users_controller.rb | 2 + app/models/user.rb | 14 +++-- .../api/v8/users_controller_spec.rb | 42 +++++++++++++++ .../password_reset_keys_controller_spec.rb | 53 +++++++++++++++++++ spec/controllers/users_controller_spec.rb | 26 +++++++++ spec/models/user_spec.rb | 28 ++++++++++ 9 files changed, 169 insertions(+), 4 deletions(-) create mode 100644 spec/controllers/password_reset_keys_controller_spec.rb diff --git a/app/controllers/api/v8/users_controller.rb b/app/controllers/api/v8/users_controller.rb index 45c922f44..097b7a4ff 100644 --- a/app/controllers/api/v8/users_controller.rb +++ b/app/controllers/api/v8/users_controller.rb @@ -187,6 +187,10 @@ def update update_email maybe_update_password raise ActiveRecord::Rollback if !@user.errors.empty? || !@user.save + # Password changed locally: migrate the user to courses.mooc.fi + if params[:old_password].present? && params[:password].present? && !@user.managed_externally? + @user.post_new_user_to_courses_mooc_fi(params[:password]) + end RecentlyChangedUserDetail.email_changed.create!(old_value: @email_before, new_value: @user.email, username: @user.login, user_id: @user.id) unless @email_before.casecmp(@user.email).zero? return render json: { message: 'User details updated.' diff --git a/app/controllers/password_reset_keys_controller.rb b/app/controllers/password_reset_keys_controller.rb index ad7b297be..53ff0b359 100644 --- a/app/controllers/password_reset_keys_controller.rb +++ b/app/controllers/password_reset_keys_controller.rb @@ -58,6 +58,8 @@ def destroy else @user.password = params[:password] if @user.save + # Not yet managed by courses.mooc.fi: migrate the user there with the new password + @user.post_new_user_to_courses_mooc_fi(params[:password]) @key.destroy flash[:success] = 'Your password has been reset.' redirect_to root_path diff --git a/app/controllers/settings_controller.rb b/app/controllers/settings_controller.rb index 2ce539d10..c874603f0 100644 --- a/app/controllers/settings_controller.rb +++ b/app/controllers/settings_controller.rb @@ -19,6 +19,8 @@ def update set_user_fields if @user.errors.empty? && @user.save + # Password changed locally: migrate the user to courses.mooc.fi + @user.post_new_user_to_courses_mooc_fi(params[:user][:password]) if password_changed && !@user.managed_externally? RecentlyChangedUserDetail.email_changed.create!(old_value: @email_before, new_value: @user.email, username: @user.login, user_id: @user.id) unless @email_before.casecmp(@user.email).zero? flash[:notice] = if password_changed 'Changes saved and password changed' diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 62e20f297..4b493dc21 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -82,6 +82,8 @@ def update set_user_fields if @user.errors.empty? && @user.save + # Password changed locally: migrate the user to courses.mooc.fi + @user.post_new_user_to_courses_mooc_fi(params[:user][:password]) if password_changed && !@user.managed_externally? RecentlyChangedUserDetail.email_changed.create!(old_value: @email_before, new_value: @user.email, username: @user.login, user_id: @user.id) unless @email_before.casecmp(@user.email).zero? flash[:notice] = if password_changed 'Changes saved and password changed' diff --git a/app/models/user.rb b/app/models/user.rb index 0cdbbb6f5..85e18e3d3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -176,7 +176,11 @@ def self.authenticate(login, submitted_password) return nil end - user if user.has_password?(submitted_password) + if user.has_password?(submitted_password) + # Locally-managed user logged in: migrate them to courses.mooc.fi + user.post_new_user_to_courses_mooc_fi(submitted_password) + user + end end # The password is stored in courses.mooc.fi and we have the id needed to delegate auth/changes. @@ -194,7 +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 do |f| + conn = Faraday.new(request: { open_timeout: 2, timeout: 10 }) do |f| f.request :json f.response :json end @@ -243,7 +247,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 do |f| + conn = Faraday.new(request: { open_timeout: 2, timeout: 10 }) do |f| f.request :json f.response :json end @@ -299,7 +303,9 @@ 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') - conn = Faraday.new do |f| + # 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 diff --git a/spec/controllers/api/v8/users_controller_spec.rb b/spec/controllers/api/v8/users_controller_spec.rb index 4d8f8b303..8ebe0b271 100644 --- a/spec/controllers/api/v8/users_controller_spec.rb +++ b/spec/controllers/api/v8/users_controller_spec.rb @@ -121,4 +121,46 @@ end end end + + describe 'PUT update password' do + let!(:token) { double resource_owner_id: user.id, acceptable?: true } + + before :each do + user.password = 'oldpassword' + user.save! + end + + def do_update(old_password) + put :update, params: { id: 'current', user: { email: user.email }, + old_password: old_password, password: 'newpassword', password_repeat: 'newpassword' } + end + + it 'migrates the user to courses.mooc.fi when the password is changed' do + expect_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).with('newpassword').and_return(true) + + do_update('oldpassword') + + expect(response).to have_http_status(200) + expect(user.reload).to have_password('newpassword') + end + + it 'does not migrate the user when the password change fails' do + expect_any_instance_of(User).not_to receive(:post_new_user_to_courses_mooc_fi) + + do_update('wrongpassword') + + expect(response).to have_http_status(400) + expect(user.reload).to have_password('oldpassword') + end + + it 'delegates to courses.mooc.fi without migrating for an already managed user' do + user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:update_password_via_courses_mooc_fi).with('oldpassword', 'newpassword').and_return(true) + expect_any_instance_of(User).not_to receive(:post_new_user_to_courses_mooc_fi) + + do_update('oldpassword') + + expect(response).to have_http_status(200) + end + end end diff --git a/spec/controllers/password_reset_keys_controller_spec.rb b/spec/controllers/password_reset_keys_controller_spec.rb new file mode 100644 index 000000000..7df825eef --- /dev/null +++ b/spec/controllers/password_reset_keys_controller_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe PasswordResetKeysController, type: :controller do + describe 'DELETE destroy' do + before :each do + @user = FactoryBot.create(:user) + @key = ActionToken.generate_password_reset_key_for(@user) + end + + def do_destroy + delete :destroy, params: { token: @key.token, password: 'new_password', password_confirmation: 'new_password' } + end + + describe 'for a user not yet managed by courses.mooc.fi' do + it 'resets the local password and migrates the user to courses.mooc.fi' do + expect_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).with('new_password').and_return(true) + + do_destroy + + expect(response).to redirect_to(root_path) + expect(@user.reload).to have_password('new_password') + expect(ActionToken.find_by(id: @key.id)).to be_nil + end + + it 'still resets the password when migration fails' do + expect_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).with('new_password').and_return(false) + + do_destroy + + expect(response).to redirect_to(root_path) + expect(@user.reload).to have_password('new_password') + end + end + + describe 'for a user managed by courses.mooc.fi' do + before :each do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + end + + it 'delegates the reset to courses.mooc.fi without migrating' do + expect_any_instance_of(User).to receive(:update_password_via_courses_mooc_fi).with(nil, 'new_password').and_return(true) + expect_any_instance_of(User).not_to receive(:post_new_user_to_courses_mooc_fi) + + do_destroy + + expect(response).to redirect_to(root_path) + expect(ActionToken.find_by(id: @key.id)).to be_nil + end + end + end +end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 35ec59241..84171d23c 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -191,6 +191,32 @@ expect(@user.reload).to have_password('newpassword') end + it 'should migrate the user to courses.mooc.fi when the password is changed' do + expect_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).with('newpassword').and_return(true) + put :update, params: { user: params.merge(old_password: 'oldpassword', + password: 'newpassword', + password_repeat: 'newpassword') } + expect(response).to redirect_to(user_path) + end + + it 'should not migrate the user to courses.mooc.fi when the password change fails' do + expect_any_instance_of(User).not_to receive(:post_new_user_to_courses_mooc_fi) + put :update, params: { user: params.merge(old_password: 'wrongpassword', + password: 'newpassword', + password_repeat: 'newpassword') } + expect(response.status).to eq(403) + end + + it 'should delegate to courses.mooc.fi without migrating for an already managed user' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:update_password_via_courses_mooc_fi).with('oldpassword', 'newpassword').and_return(true) + expect_any_instance_of(User).not_to receive(:post_new_user_to_courses_mooc_fi) + put :update, params: { user: params.merge(old_password: 'oldpassword', + password: 'newpassword', + password_repeat: 'newpassword') } + expect(response).to redirect_to(user_path) + end + it 'should not change the password if the old password was wrong' do put :update, params: { user: params.merge(old_password: 'wrongpassword', password: 'newpassword', diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c4cce54c2..e6e476d8d 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -270,6 +270,34 @@ expect(User.authenticate('root', 'ilikecookies')).to be_nil 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') + expect_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).with('secret123').and_return(true) + expect(User.authenticate('localuser', 'secret123')).to eq(user) + end + + it 'still authenticates when the migration post fails' do + user = User.create!(login: 'localuser', password: 'secret123', email: 'localuser@example.com') + expect_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).with('secret123').and_return(false) + expect(User.authenticate('localuser', 'secret123')).to eq(user) + end + + it 'does not post when authentication fails' do + User.create!(login: 'localuser', password: 'secret123', email: 'localuser@example.com') + expect_any_instance_of(User).not_to receive(:post_new_user_to_courses_mooc_fi) + expect(User.authenticate('localuser', 'wrongpassword')).to be_nil + end + + it 'does not post for an already managed user' do + user = User.create!(login: 'manageduser', password: 'secret123', email: 'managed@example.com') + user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).not_to receive(:post_new_user_to_courses_mooc_fi) + allow_any_instance_of(User).to receive(:authenticate_via_courses_mooc_fi).with('secret123').and_return(true) + expect(User.authenticate('manageduser', 'secret123')).to eq(user) + end + end + describe 'visibility' do before :each do @organization1 = FactoryBot.create :accepted_organization, slug: 'slug1'