Migrate existing users to courses.mooc.fi on login and password changes (#595)

Author: Redande

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.'

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

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'

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'

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

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

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

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',

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'