From 5cd89619fd9a137dc14d642f216a7a39ea6e5fe4 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Thu, 17 Sep 2026 13:14:39 +0300 Subject: [PATCH] Fix account deletion dead ends and misleading login errors --- app/controllers/api/v8/users_controller.rb | 22 +++-- app/controllers/sessions_controller.rb | 25 +++--- app/controllers/users_controller.rb | 17 +++- app/models/course_template_refresh.rb | 2 +- app/models/user.rb | 57 ++++++++----- app/views/courses/show.html.erb | 7 +- ..._null_user_on_course_template_refreshes.rb | 13 +++ db/schema.rb | 6 +- .../api/v8/users_controller_spec.rb | 81 +++++++++++++++++++ spec/controllers/sessions_controller_spec.rb | 55 ++++++++++++- spec/controllers/users_controller_spec.rb | 58 +++++++++++++ spec/models/user_spec.rb | 51 +++++++++++- 12 files changed, 340 insertions(+), 54 deletions(-) create mode 100644 db/migrate/20260917120000_allow_null_user_on_course_template_refreshes.rb diff --git a/app/controllers/api/v8/users_controller.rb b/app/controllers/api/v8/users_controller.rb index d1adbd208..5b0de1977 100644 --- a/app/controllers/api/v8/users_controller.rb +++ b/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] diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 4c59aac65..57d2b3034 100644 --- a/app/controllers/sessions_controller.rb +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 4b493dc21..518a28c73 100644 --- a/app/controllers/users_controller.rb +++ b/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] diff --git a/app/models/course_template_refresh.rb b/app/models/course_template_refresh.rb index c0d8a0e17..beb0e7fd1 100644 --- a/app/models/course_template_refresh.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index a152b6ae3..86485036e 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/views/courses/show.html.erb b/app/views/courses/show.html.erb index cc79b1cbe..04ae43861 100644 --- a/app/views/courses/show.html.erb +++ b/app/views/courses/show.html.erb @@ -201,9 +201,10 @@ <% if @course.refreshed_at %>
  • 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 %>
    • <% end %> diff --git a/db/migrate/20260917120000_allow_null_user_on_course_template_refreshes.rb b/db/migrate/20260917120000_allow_null_user_on_course_template_refreshes.rb new file mode 100644 index 000000000..e7b4bf510 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index af328f129..9d62cab13 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/controllers/api/v8/users_controller_spec.rb b/spec/controllers/api/v8/users_controller_spec.rb index fee8b6c12..8a0f8305f 100644 --- a/spec/controllers/api/v8/users_controller_spec.rb +++ b/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 diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index dbbbe4946..14fc2e242 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -5,8 +5,12 @@ describe SessionsController, type: :controller do before :each do @user = mock_model(User, administrator?: true) - allow(User).to receive(:authenticate) do |login, pwd| - @user if login == 'instructor' && pwd == 'correct_password' + allow(User).to receive(:authenticate_with_status) do |login, pwd| + if login == 'instructor' && pwd == 'correct_password' + [@user, :accepted] + else + [nil, :rejected] + end end end @@ -61,7 +65,7 @@ def post_create describe 'when authentication fails' do before :each do - allow(User).to receive_messages(authenticate: nil) + allow(User).to receive(:authenticate_with_status).and_return([nil, :rejected]) end it 'should not set current_user' do @@ -81,6 +85,51 @@ def post_create expect(response).to redirect_to(login_path) end end + + describe 'when courses.mooc.fi cannot be reached' do + before :each do + allow(User).to receive(:authenticate_with_status).and_return([nil, :unavailable]) + end + + it 'should not set current_user' do + post_create + expect(controller.send(:current_user)).to be_guest + end + + it 'should say login is unavailable instead of blaming the credentials' do + post_create + expect(flash[:alert]).to include('temporarily unavailable') + end + end + + describe 'when the account cannot be delegated to courses.mooc.fi at all' do + before :each do + allow(User).to receive(:authenticate_with_status).and_return([nil, :misconfigured]) + end + + it 'should not set current_user' do + post_create + expect(controller.send(:current_user)).to be_guest + end + + it 'should tell the user to contact support instead of to wait' do + post_create + expect(flash[:alert]).to include('contact support') + expect(flash[:alert]).not_to include('try again') + end + end + + describe 'when the status is one this controller does not know' do + before :each do + allow(User).to receive(:authenticate_with_status).and_return([@user, :some_future_status]) + end + + it 'should not sign the user in' do + post_create + expect(controller.send(:current_user)).to be_guest + expect(flash[:alert]).to eq('Invalid credentials. Try again.') + end + end end describe 'DELETE destroy' do diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 84171d23c..0a7971bcf 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -242,4 +242,62 @@ end end end + + describe 'DELETE destroy_user' do + let!(:user) { FactoryBot.create(:user, password: 'secret123') } + let!(:verification_token) { VerificationToken.create!(user: user, type: :delete_user) } + + before :each do + controller.current_user = user + end + + def do_destroy_user(password) + delete :destroy_user, params: { user_id: user.id, id: verification_token.token, im_sure: '1', + user: { password: password } } + end + + it 'should destroy the account when the password is correct' do + allow_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).and_return(true) + + do_destroy_user('secret123') + + expect(User.find_by(id: user.id)).to be_nil + end + + it 'should blame the password when it was actually rejected' do + do_destroy_user('wrongpassword') + + expect(flash[:alert]).to eq('The password was incorrect.') + expect(User.find_by(id: user.id)).not_to be_nil + end + + it 'should not blame the password when courses.mooc.fi could not be reached' do + user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + allow_any_instance_of(User).to receive(:courses_mooc_fi_authentication_status).and_return(:unavailable) + + do_destroy_user('secret123') + + expect(flash[:alert]).to include('could not reach') + expect(User.find_by(id: user.id)).not_to be_nil + end + + it 'should tell the user to contact support when the courses.mooc.fi id is missing' do + user.update_columns(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: nil) + + do_destroy_user('secret123') + + expect(flash[:alert]).to include('contact support') + expect(flash[:alert]).not_to include('try again') + expect(User.find_by(id: user.id)).not_to be_nil + end + + it 'should not destroy the account on a status this controller does not know' do + allow(User).to receive(:authenticate_with_status).and_return([user, :some_future_status]) + + do_destroy_user('secret123') + + expect(User.find_by(id: user.id)).not_to be_nil + expect(flash[:alert]).to eq('The password was incorrect.') + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 571f2ae03..2a08edd17 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -314,11 +314,60 @@ 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) + allow_any_instance_of(User).to receive(:courses_mooc_fi_authentication_status).with('secret123').and_return(:accepted) expect(User.authenticate('manageduser', 'secret123')).to eq(user) end end + describe '.authenticate_with_status' do + let!(: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) + user + end + + def stub_courses_mooc_fi_authentication(status:, body:) + response = double(success?: (200..299).cover?(status), status: status, body: body, headers: {}) + connection = double + allow(connection).to receive(:post).and_return(response) + allow(Faraday).to receive(:new).and_return(connection) + end + + it 'rejects a wrong local password' do + User.create!(login: 'localuser', password: 'secret123', email: 'localuser@example.com') + expect(User.authenticate_with_status('localuser', 'wrongpassword').last).to eq(:rejected) + end + + it 'rejects a password that courses.mooc.fi refuses' do + stub_courses_mooc_fi_authentication(status: 200, body: false) + expect(User.authenticate_with_status('manageduser', 'secret123').last).to eq(:rejected) + end + + it 'accepts a password that courses.mooc.fi confirms' do + stub_courses_mooc_fi_authentication(status: 200, body: true) + user, status = User.authenticate_with_status('manageduser', 'secret123') + expect(status).to eq(:accepted) + expect(user).to eq(managed_user) + end + + it 'reports an error from courses.mooc.fi as unavailable, not as a wrong password' do + stub_courses_mooc_fi_authentication(status: 500, body: 'oops') + expect(User.authenticate_with_status('manageduser', 'secret123').last).to eq(:unavailable) + expect(User.authenticate('manageduser', 'secret123')).to be_nil + end + + it 'reports an unreachable courses.mooc.fi as unavailable' do + allow(Faraday).to receive(:new).and_raise(Faraday::ConnectionFailed.new('boom')) + expect(User.authenticate_with_status('manageduser', 'secret123').last).to eq(:unavailable) + end + + it 'reports a missing courses_mooc_fi_user_id as misconfigured, not as a passing outage' do + managed_user.update_column(:courses_mooc_fi_user_id, nil) + expect(User.authenticate_with_status('manageduser', 'secret123').last).to eq(:misconfigured) + expect(User.authenticate('manageduser', 'secret123')).to be_nil + end + end + describe 'visibility' do before :each do @organization1 = FactoryBot.create :accepted_organization, slug: 'slug1'