Refactor password management for externally managed users

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

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

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?

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

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

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

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'