diff --git a/app/controllers/api/v8/users_controller.rb b/app/controllers/api/v8/users_controller.rb index ec5300e34..45c922f44 100644 --- a/app/controllers/api/v8/users_controller.rb +++ b/app/controllers/api/v8/users_controller.rb @@ -299,8 +299,14 @@ def set_password def maybe_update_password if params[:old_password].present? && params[:password].present? - if @user.password_managed_by_courses_mooc_fi && @user.courses_mooc_fi_user_id.present? - return @user.update_password_via_courses_mooc_fi(params[:old_password], params[:password]) + if @user.managed_externally? + unless @user.update_password_via_courses_mooc_fi(params[:old_password], params[:password]) + @user.errors.add(:password, 'could not be changed. Please check your current password and try again.') + end + return + elsif @user.externally_managed_without_target? + @user.errors.add(:base, 'This account is being migrated; password changes are temporarily unavailable.') + return end if !@user.has_password?(params[:old_password]) @user.errors.add(:old_password, 'incorrect') diff --git a/app/controllers/password_reset_keys_controller.rb b/app/controllers/password_reset_keys_controller.rb index ee5ecb8fd..ad7b297be 100644 --- a/app/controllers/password_reset_keys_controller.rb +++ b/app/controllers/password_reset_keys_controller.rb @@ -39,7 +39,7 @@ def destroy return render action: :show, status: :forbidden end - if @user.password_managed_by_courses_mooc_fi + if @user.managed_externally? success = @user.update_password_via_courses_mooc_fi(nil, params[:password]) if success @key.destroy @@ -49,6 +49,12 @@ def destroy flash.now[:alert] = 'Failed to reset password.' render action: :show, status: :forbidden end + elsif @user.externally_managed_without_target? + # Flagged as managed by courses.mooc.fi but with no id to delegate to. Do not fall back to a + # local reset (authentication would ignore it); surface the misconfiguration instead. + Rails.logger.error("Password reset for user #{@user.id}: managed by courses.mooc.fi but missing courses_mooc_fi_user_id") + flash.now[:alert] = 'This account cannot be reset right now. Please contact support.' + render action: :show, status: :forbidden else @user.password = params[:password] if @user.save diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 08cd71bdb..4c59aac65 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -18,8 +18,13 @@ def create rescue StandardError end - user = User.authenticate(params[:session][:login], - params[:session][:password]) + 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 redirect_params = {} if user.nil? diff --git a/app/controllers/settings_controller.rb b/app/controllers/settings_controller.rb index fed89abd1..2ce539d10 100644 --- a/app/controllers/settings_controller.rb +++ b/app/controllers/settings_controller.rb @@ -87,8 +87,15 @@ def set_email def maybe_update_password(user, user_params) if user_params[:old_password].present? || user_params[:password].present? - if user.password_managed_by_courses_mooc_fi && user.courses_mooc_fi_user_id.present? - return user.update_password_via_courses_mooc_fi(user_params[:old_password], user_params[:password]) + if user.managed_externally? + unless user.update_password_via_courses_mooc_fi(user_params[:old_password], user_params[:password]) + user.errors.add(:password, 'could not be changed. Please check your current password and try again.') + return false + end + return true + elsif user.externally_managed_without_target? + user.errors.add(:base, 'This account is being migrated; password changes are temporarily unavailable.') + return false end if !user.has_password?(user_params[:old_password]) user.errors.add(:old_password, 'incorrect') diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index e75b78c30..62e20f297 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -186,8 +186,15 @@ def set_password def maybe_update_password(user, user_params) if user_params[:old_password].present? || user_params[:password].present? - if user.password_managed_by_courses_mooc_fi && user.courses_mooc_fi_user_id.present? - return user.update_password_via_courses_mooc_fi(user_params[:old_password], user_params[:password]) + if user.managed_externally? + unless user.update_password_via_courses_mooc_fi(user_params[:old_password], user_params[:password]) + user.errors.add(:password, 'could not be changed. Please check your current password and try again.') + return false + end + return true + elsif user.externally_managed_without_target? + user.errors.add(:base, 'This account is being migrated; password changes are temporarily unavailable.') + return false end if !user.has_password?(user_params[:old_password]) user.errors.add(:old_password, 'incorrect') diff --git a/app/models/user.rb b/app/models/user.rb index 8442e3812..8506ca447 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -45,6 +45,17 @@ class User < ApplicationRecord message: 'does not look like an email' } + # Guard the courses.mooc.fi delegation id: it must be a valid UUID and unique. A malformed id + # set here would otherwise be persisted while the local password hash is nulled, locking the + # user out (they could neither log in locally nor be delegated to courses.mooc.fi). + validates :courses_mooc_fi_user_id, + uniqueness: true, + format: { + with: /\A\h{8}-\h{4}-\h{4}-\h{4}-\h{12}\z/, + message: 'must be a valid UUID' + }, + allow_blank: true + validate :reject_common_login_mistakes, on: :create scope :legitimate_students, -> { where(legitimate_student: true) } @@ -129,7 +140,15 @@ def guest? def has_password?(submitted_password) if !salt - return Argon2::Password.verify_password(submitted_password, argon_hash) + normalized = normalize_password(submitted_password) + # When normalization changes nothing (e.g. an ASCII password) there is only one form to + # check, so verify once. Only when it changes the input do we also try the raw bytes, to + # stay compatible with argon hashes created before normalization existed. + if normalized == submitted_password + return Argon2::Password.verify_password(submitted_password, argon_hash) + end + return Argon2::Password.verify_password(normalized, argon_hash) || + Argon2::Password.verify_password(submitted_password, argon_hash) end result = Argon2::Password.verify_password(old_encrypt(submitted_password), argon_hash) if result && salt @@ -147,14 +166,30 @@ def self.authenticate(login, submitted_password) user ||= find_by('lower(email) = ?', login.downcase) return nil if user.nil? - if user.password_managed_by_courses_mooc_fi && user.courses_mooc_fi_user_id.present? + 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. + 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 end user if user.has_password?(submitted_password) end + # The password is stored in courses.mooc.fi and we have the id needed to delegate auth/changes. + def managed_externally? + password_managed_by_courses_mooc_fi && courses_mooc_fi_user_id.present? + end + + # Flagged as managed by courses.mooc.fi but missing the delegation id: a misconfigured state in + # which local credentials must never be used as a fallback. + def externally_managed_without_target? + password_managed_by_courses_mooc_fi && courses_mooc_fi_user_id.blank? + end + def authenticate_via_courses_mooc_fi(submitted_password) auth_url = SiteSetting.value('courses_mooc_fi_auth_url') @@ -171,11 +206,12 @@ def authenticate_via_courses_mooc_fi(submitted_password) req.body = { user_id: courses_mooc_fi_user_id, - password: submitted_password + password: normalize_password(submitted_password) } end if response.body == true + clear_stale_local_password return true end @@ -220,8 +256,8 @@ def update_password_via_courses_mooc_fi(old_password, new_password) req.body = { user_id: self.courses_mooc_fi_user_id, - old_password: old_password, - new_password: new_password + old_password: normalize_password(old_password), + new_password: normalize_password(new_password) } end @@ -231,6 +267,7 @@ def update_password_via_courses_mooc_fi(old_password, new_password) raise "Updating password via courses.mooc.fi failed for user #{self.email}" end + clear_stale_local_password true rescue Faraday::ClientError => e @@ -275,7 +312,7 @@ def post_new_user_to_courses_mooc_fi(password) req.body = { upstream_id: id, - password: password, + password: normalize_password(password), } end @@ -519,6 +556,24 @@ def reject_common_login_mistakes end def generate_argon(input) - Argon2::Password.new(t_cost: 4, m_cost: 15).create(input) + Argon2::Password.new(t_cost: 4, m_cost: 15).create(normalize_password(input)) + end + + # Normalize a password to Unicode NFC before hashing or verifying so that the same password + # entered on different platforms/forms hashes to identical bytes. Argon2 hashes raw bytes, so + # a composed "ä" (U+00E4) and a decomposed "ä" (U+0061 U+0308) would otherwise produce + # different hashes. nil is passed through unchanged (e.g. a missing old_password on reset). + def normalize_password(password) + return password unless password.is_a?(String) + password.unicode_normalize(:nfc) + end + + # courses.mooc.fi owns this user's password, so any local hash is dead weight that must never + # be used as a fallback. Drop it if present. This self-heals users whose managed flag was set + # by a direct SQL backfill that left the old hash behind (see also the one-time cleanup task). + # update_columns writes directly, skipping validations and callbacks. + def clear_stale_local_password + return unless argon_hash.present? || salt.present? || password_hash.present? + update_columns(argon_hash: nil, salt: nil, password_hash: nil, updated_at: Time.now) end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 5f1537b3e..c4cce54c2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -227,6 +227,33 @@ expect(user).not_to have_password('ihatecookies') end + it 'should authenticate regardless of the Unicode normalization form of the password' do + nfc = "p\u{00E4}ssword" # "pässword" with a composed ä (U+00E4) + nfd = "pa\u{0308}ssword" # "pässword" with a decomposed ä (a + combining U+0308) + expect(nfc.bytes).not_to eq(nfd.bytes) # sanity check: the raw inputs really differ + + # Stored from the NFD form (as one platform/form might submit it)... + User.create!(login: 'umlaut', password: nfd, email: 'umlaut@example.com') + user = User.find_by!(login: 'umlaut') + + # ...must still verify against the NFC form (as another platform/form would submit it). + expect(user).to have_password(nfc) + expect(user).to have_password(nfd) + expect(user).not_to have_password('different') + end + + it 'authenticates an argon hash that predates normalization (raw bytes)' do + nfd = "pa\u{0308}ssword" # decomposed "pässword" + user = User.create!(login: 'legacynorm', password: 'placeholder', email: 'legacynorm@example.com') + # Simulate a hash stored before normalization existed: argon over the raw NFD bytes. + user.update_column(:argon_hash, Argon2::Password.new(t_cost: 4, m_cost: 15).create(nfd)) + user.update_column(:salt, nil) + + user = User.find_by!(login: 'legacynorm') + expect(user).to have_password(nfd) # raw-bytes fallback keeps the existing user logging in + expect(user).not_to have_password('different') + end + it 'should not reset the password when saved without changing the password' do user = User.create!(login: 'instructor', password: 'ihatecookies', email: 'instructor@example.com') user.login = 'funny_person'