Fix account deletion dead ends and misleading login errors (#598)

Author: nygrenh

app/controllers/api/v8/users_controller.rb

@@ -110,7 +110,7 @@ class UsersController < Api::V8::BaseController
         end
       end
 
-      skip_authorization_check only: %i[set_password_managed_by_courses_mooc_fi]
+      skip_authorization_check only: %i[set_password_managed_by_courses_mooc_fi destroy]
 
       def show
         unauthorize_guest! if current_user.guest?
@@ -209,11 +209,14 @@ def update
         }, status: :bad_request
       end
 
+      # courses.mooc.fi retries account deletion and keys off success/already_deleted/code, so a
+      # user that is already gone must answer 200, never 404.
       def destroy
-        unauthorize_guest! if current_user.guest?
+        return render_destroy_failure('not_authorized', 'Authentication required', :unauthorized) if current_user.guest?
 
-        user = User.find(params[:id])
-        authorize! :destroy, user
+        user = User.find_by(id: params[:id])
+        return render json: { success: true, already_deleted: true } if user.nil?
+        return render_destroy_failure('not_authorized', 'Forbidden', :forbidden) unless can?(:destroy, user)
 
         User.transaction do
           if user.destroy
@@ -226,11 +229,14 @@ def destroy
             )
             Doorkeeper::AccessToken.where(resource_owner_id: user.id).delete_all
 
-            render json: { success: true, message: 'User deleted.' }
+            render json: { success: true, already_deleted: false, message: 'User deleted.' }
           else
-            render json: { success: false, errors: user.errors }, status: :bad_request
+            render_destroy_failure('destroy_failed', user.errors.full_messages, :bad_request)
           end
         end
+      rescue StandardError => e
+        Rails.logger.error("Deleting user #{params[:id]} failed: #{e.class}: #{e.message}")
+        render_destroy_failure('internal_error', 'User deletion failed.', :internal_server_error)
       end
 
       def set_password_managed_by_courses_mooc_fi
@@ -277,6 +283,10 @@ def get_user_with_email
       end
 
       private
+        def render_destroy_failure(code, messages, status)
+          render json: { success: false, code: code, errors: [*messages] }, status: status
+        end
+
         def set_email
           user_params = params[:user]
 

app/controllers/sessions_controller.rb

@@ -18,24 +18,19 @@ def create
     rescue StandardError
     end
 
-    user = begin
-      User.authenticate(params[:session][:login], params[:session][:password])
-    rescue Faraday::Error => e
-      # courses.mooc.fi delegated authentication is unreachable; fail gracefully instead of 500.
-      Rails.logger.error("Login temporarily unavailable due to courses.mooc.fi error: #{e.class}: #{e.message}")
-      return try_to_redirect_incorrect_login(alert: 'Login is temporarily unavailable. Please try again shortly.')
-    end
+    user, status = User.authenticate_with_status(params[:session][:login], params[:session][:password])
 
-    redirect_params = {}
-    if user.nil?
-      msg = 'Invalid credentials. Try again.'
-      redirect_params = { alert: msg }
-      return try_to_redirect_incorrect_login(redirect_params)
-    else
+    case status
+    when :accepted
       sign_in user
+      try_to_redirect_back
+    when :unavailable
+      try_to_redirect_incorrect_login(alert: 'Login is temporarily unavailable. Please try again shortly.')
+    when :misconfigured
+      try_to_redirect_incorrect_login(alert: 'Your account needs manual attention before you can log in. Please contact support.')
+    else
+      try_to_redirect_incorrect_login(alert: 'Invalid credentials. Try again.')
     end
-
-    try_to_redirect_back(redirect_params)
   end
 
   def destroy

app/controllers/users_controller.rb

@@ -129,9 +129,9 @@ def destroy_user
       return
     end
     user = authenticate_current_user_destroy
-    user_authentication = User.authenticate(user.login, params[:user][:password])
-    if user_authentication.nil?
-      redirect_to verify_destroying_user_url, alert: 'The password was incorrect.'
+    _authenticated_user, status = User.authenticate_with_status(user.login, params[:user][:password])
+    unless status == :accepted
+      redirect_to verify_destroying_user_url, alert: failed_authentication_alert(status)
       return
     end
     VerificationToken.delete_user.find_by!(user: user, token: params[:id])
@@ -159,6 +159,17 @@ def authenticate_current_user_destroy
       user
     end
 
+    def failed_authentication_alert(status)
+      case status
+      when :unavailable
+        'We could not reach the system that holds your password. Please try again in a few minutes, or contact support if this keeps happening.'
+      when :misconfigured
+        'Your account needs manual attention before it can be deleted. Please contact support.'
+      else
+        'The password was incorrect.'
+      end
+    end
+
     def set_email
       user_params = params[:user]
 

app/models/course_template_refresh.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class CourseTemplateRefresh < ApplicationRecord
-  belongs_to :user
+  belongs_to :user, optional: true
   belongs_to :course_template
   has_many :course_template_refresh_phases, dependent: :delete_all
   has_one :course_template_refresh_report, dependent: :destroy

app/models/user.rb

@@ -23,6 +23,7 @@ class User < ApplicationRecord
   has_many :unlocks, dependent: :delete_all
   has_many :uncomputed_unlocks, dependent: :delete_all
   has_many :reviews, foreign_key: :reviewer_id, inverse_of: :reviewer, dependent: :nullify
+  has_many :course_template_refreshes, dependent: :nullify
   has_many :course_notifications
   has_many :comments
   has_many :certificates
@@ -159,28 +160,37 @@ def has_password?(submitted_password)
     result
   end
 
+  # The user when the password is accepted, nil otherwise. Use .authenticate_with_status when the
+  # outcome is shown to the user, so that a delegation failure is not reported as a wrong password.
   def self.authenticate(login, submitted_password)
-    return nil unless login
+    user, status = authenticate_with_status(login, submitted_password)
+    status == :accepted ? user : nil
+  end
+
+  # [user, status] where status is :accepted, :rejected when the password was judged wrong,
+  # :unavailable when courses.mooc.fi could not be reached, or :misconfigured when the account
+  # cannot be delegated at all and only an admin can repair it. A found user is returned whatever
+  # the status, so sign in only on :accepted.
+  def self.authenticate_with_status(login, submitted_password)
+    return [nil, :rejected] unless login
     login = login.strip
     user = find_by(login: login)
     user ||= find_by('lower(email) = ?', login.downcase)
-    return nil if user.nil?
+    return [nil, :rejected] if user.nil?
 
-    if user.managed_externally?
-      return user if user.authenticate_via_courses_mooc_fi(submitted_password)
-      return nil
-    elsif user.externally_managed_without_target?
-      # Half-migrated/misconfigured: flagged as managed by courses.mooc.fi but with no id to
-      # delegate to. Fail closed instead of silently authenticating against the stale local hash.
+    return [user, user.courses_mooc_fi_authentication_status(submitted_password)] if user.managed_externally?
+
+    if user.externally_managed_without_target?
+      # Fail closed instead of silently authenticating against the stale local hash.
       Rails.logger.error("User #{user.id} is password_managed_by_courses_mooc_fi but has no courses_mooc_fi_user_id; refusing local fallback authentication")
-      return nil
+      return [user, :misconfigured]
     end
 
-    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
+    return [user, :rejected] unless 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, :accepted]
   end
 
   # The password is stored in courses.mooc.fi and we have the id needed to delegate auth/changes.
@@ -195,7 +205,9 @@ def externally_managed_without_target?
   end
 
 
-  def authenticate_via_courses_mooc_fi(submitted_password)
+  # :accepted, :rejected when courses.mooc.fi judged the password wrong, or :unavailable when it
+  # could not be consulted. Raises nothing, so that an outage is never mistaken for a rejection.
+  def courses_mooc_fi_authentication_status(submitted_password)
     auth_url = courses_mooc_fi_url('/api/v0/tmc-server/users/authenticate')
 
     conn = courses_mooc_fi_connection
@@ -211,22 +223,29 @@ def authenticate_via_courses_mooc_fi(submitted_password)
       }
     end
 
+    unless response.success?
+      Rails.logger.error(
+        "Authentication via courses.mooc.fi failed for user #{self.email}: status=#{response.status}, request-id=#{response.headers['request-id']}, body=#{response.body.inspect}"
+      )
+      return :unavailable
+    end
+
     if response.body == true
       clear_stale_local_password
-      return true
+      return :accepted
     end
 
     Rails.logger.warn(
       "Authentication via courses.mooc.fi rejected for user #{self.email}: status=#{response.status}, request-id=#{response.headers['request-id']}, body=#{response.body.inspect}"
     )
-    false
+    :rejected
 
   rescue Faraday::ClientError => e
     status = e.response&.dig(:status)
     request_id = e.response&.dig(:headers, 'request-id')
     body = e.response&.dig(:body)
     Rails.logger.error("Authentication via courses.mooc.fi error for user #{self.email}: status=#{status}, request-id=#{request_id}, body=#{body.inspect}")
-    raise
+    :unavailable
 
   rescue => e
     if defined?(response) && response
@@ -236,7 +255,7 @@ def authenticate_via_courses_mooc_fi(submitted_password)
     else
       Rails.logger.error("Unexpected error during authentication via courses.mooc.fi for user #{self.email} (no response): #{e.message}")
     end
-    raise
+    :unavailable
   end
 
 

app/views/courses/show.html.erb

@@ -201,9 +201,10 @@
           <% if @course.refreshed_at %>
             <li>
               Last refreshed at <%= @course.refreshed_at.strftime("%d.%m.%Y %H:%M:%S") %> 
-              <% if @course.course_template.course_template_refreshes.any? %>
-                by <%= @course.course_template.course_template_refreshes.last.user.login %>
-                <%= link_to 'Show last report', organization_course_path(@organization, @course, generate_report: @course.course_template.course_template_refreshes.last.id), class: "btn btn-link" %>
+              <% last_refresh = @course.course_template.course_template_refreshes.last %>
+              <% if last_refresh %>
+                <% if last_refresh.user %>by <%= last_refresh.user.login %><% end %>
+                <%= link_to 'Show last report', organization_course_path(@organization, @course, generate_report: last_refresh.id), class: "btn btn-link" %>
               <% end %>
             </li>
           <% end %>

db/migrate/20260917120000_allow_null_user_on_course_template_refreshes.rb

@@ -0,0 +1,13 @@
+class AllowNullUserOnCourseTemplateRefreshes < ActiveRecord::Migration[7.1]
+  def up
+    change_column_null :course_template_refreshes, :user_id, true
+    remove_foreign_key :course_template_refreshes, :users
+    add_foreign_key :course_template_refreshes, :users, on_delete: :nullify
+  end
+
+  def down
+    remove_foreign_key :course_template_refreshes, :users
+    change_column_null :course_template_refreshes, :user_id, false
+    add_foreign_key :course_template_refreshes, :users
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2025_07_23_122117) do
+ActiveRecord::Schema[7.1].define(version: 2026_09_17_120000) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
 
@@ -129,7 +129,7 @@
     t.integer "status", default: 0, null: false
     t.decimal "percent_done", precision: 10, scale: 4, default: "0.0", null: false
     t.jsonb "langs_refresh_output"
-    t.integer "user_id", null: false
+    t.integer "user_id"
     t.integer "course_template_id", null: false
     t.index ["course_template_id"], name: "index_course_template_refreshes_on_course_template_id"
     t.index ["user_id"], name: "index_course_template_refreshes_on_user_id"
@@ -564,7 +564,7 @@
   add_foreign_key "course_template_refresh_phases", "course_template_refreshes"
   add_foreign_key "course_template_refresh_reports", "course_template_refreshes"
   add_foreign_key "course_template_refreshes", "course_templates"
-  add_foreign_key "course_template_refreshes", "users"
+  add_foreign_key "course_template_refreshes", "users", on_delete: :nullify
   add_foreign_key "courses", "organizations"
   add_foreign_key "exercises", "courses", on_delete: :cascade
   add_foreign_key "feedback_answers", "feedback_questions", on_delete: :cascade

spec/controllers/api/v8/users_controller_spec.rb

@@ -164,6 +164,87 @@ def do_update(old_password)
     end
   end
 
+  describe 'DELETE destroy' do
+    before :each do
+      controller.current_user = admin
+    end
+
+    it 'deletes the user' do
+      delete :destroy, params: { id: user.id }
+
+      expect(response).to have_http_status(200)
+      body = JSON.parse(response.body)
+      expect(body['success']).to eq(true)
+      expect(body['already_deleted']).to eq(false)
+      expect(User.find_by(id: user.id)).to be_nil
+    end
+
+    it 'reports an already deleted user as a success instead of a 404' do
+      deleted_id = user.id
+      user.destroy!
+
+      delete :destroy, params: { id: deleted_id }
+
+      expect(response).to have_http_status(200)
+      body = JSON.parse(response.body)
+      expect(body['success']).to eq(true)
+      expect(body['already_deleted']).to eq(true)
+    end
+
+    it 'deletes a user who has refreshed a course template' do
+      course_template = FactoryBot.create(:course_template)
+      refresh = CourseTemplateRefresh.create!(user_id: user.id, course_template_id: course_template.id)
+
+      delete :destroy, params: { id: user.id }
+
+      expect(response).to have_http_status(200)
+      expect(User.find_by(id: user.id)).to be_nil
+      expect(refresh.reload.user_id).to be_nil
+    end
+
+    it 'answers a guest with a not_authorized code' do
+      controller.current_user = Guest.new
+
+      delete :destroy, params: { id: user.id }
+
+      expect(response).to have_http_status(401)
+      body = JSON.parse(response.body)
+      expect(body['success']).to eq(false)
+      expect(body['code']).to eq('not_authorized')
+      expect(User.find_by(id: user.id)).not_to be_nil
+    end
+
+    it "answers another user's deletion attempt with a not_authorized code" do
+      controller.current_user = other_user
+
+      delete :destroy, params: { id: user.id }
+
+      expect(response).to have_http_status(403)
+      expect(JSON.parse(response.body)['code']).to eq('not_authorized')
+      expect(User.find_by(id: user.id)).not_to be_nil
+    end
+
+    it 'answers a refused destroy with a destroy_failed code' do
+      allow_any_instance_of(User).to receive(:destroy).and_return(false)
+
+      delete :destroy, params: { id: user.id }
+
+      expect(response).to have_http_status(400)
+      expect(JSON.parse(response.body)['code']).to eq('destroy_failed')
+    end
+
+    it 'answers an unexpected failure with an internal_error code' do
+      allow_any_instance_of(User).to receive(:destroy).and_raise(ActiveRecord::InvalidForeignKey.new('boom'))
+
+      delete :destroy, params: { id: user.id }
+
+      expect(response).to have_http_status(500)
+      body = JSON.parse(response.body)
+      expect(body['success']).to eq(false)
+      expect(body['code']).to eq('internal_error')
+    end
+  end
+
   describe 'POST set_password_managed_by_courses_mooc_fi' do
     before :each do
       controller.current_user = admin