Fixed bug: Legacy hashed passwords (like sha2) will get rehashed when set.

This happens when a user password should get set without transfering it in plain like e.g. the REST API, an import or the auto_wizard.json file. Therefore Zammad should accept legacy passwords when set but should convert them on first login to the internal format (as already implemented).

Additionally there should be no exclusion for the import mode when setting/ensuring a password since Zammand couldn't handle it later.

app/models/user.rb

@@ -980,7 +980,6 @@ raise 'Minimum one user need to have admin permissions'
 
   def ensure_password
     return if password_empty?
-    return if Setting.get('import_mode')
     return if PasswordHash.crypted?(password)
     self.password = PasswordHash.crypt(password)
   end

lib/password_hash.rb

@@ -17,24 +17,37 @@ module PasswordHash
   end
 
   def crypted?(pw_hash)
-    return if !pw_hash
-    # taken from: https://github.com/technion/ruby-argon2/blob/7e1f4a2634316e370ab84150e4f5fd91d9263713/lib/argon2.rb#L33
-    return if pw_hash !~ /^\$argon2i\$.{,112}/
-    true
+    return false if !pw_hash
+    return true if hashed_argon2?(pw_hash)
+    return true if hashed_sha2?(pw_hash)
+    false
   end
 
   def legacy?(pw_hash, password)
-    return if pw_hash.blank?
-    return if !password
-    legacy_sha2?(pw_hash, password)
+    return false if pw_hash.blank?
+    return false if !password
+    sha2?(pw_hash, password)
   end
 
-  private
+  def hashed_sha2?(pw_hash)
+    pw_hash.start_with?('{sha2}')
+  end
 
-  def legacy_sha2?(pw_hash, password)
-    return if !pw_hash.start_with?('{sha2}')
+  def hashed_argon2?(pw_hash)
+    # taken from: https://github.com/technion/ruby-argon2/blob/7e1f4a2634316e370ab84150e4f5fd91d9263713/lib/argon2.rb#L33
+    pw_hash =~ /^\$argon2i\$.{,112}/
+  end
+
+  def sha2(password)
     crypted = Digest::SHA2.hexdigest(password)
-    pw_hash == "{sha2}#{crypted}"
+    "{sha2}#{crypted}"
+  end
+
+  private
+
+  def sha2?(pw_hash, password)
+    return false if !hashed_sha2?(pw_hash)
+    pw_hash == sha2(password)
   end
 
   def argon2

spec/factories/user.rb

@@ -25,9 +25,4 @@ FactoryGirl.define do
   factory :user_login_failed, parent: :user do
     login_failed { (Setting.get('password_max_login_failed').to_i || 10) + 1 }
   end
-
-  factory :user_legacy_password_sha2, parent: :user do
-    after(:build) { |user| user.class.skip_callback(:validation, :before, :ensure_password, if: -> { password && password.start_with?('{sha2}') }) }
-    password '{sha2}dd9c764fa7ea18cd992c8600006d3dc3ac983d1ba22e9ba2d71f6207456be0ba' # zammad
-  end
 end

spec/lib/auth/internal_spec.rb

@@ -20,13 +20,18 @@ RSpec.describe Auth::Internal do
     end
 
     it 'converts legacy sha2 passwords' do
-      user = create(:user_legacy_password_sha2)
 
-      expect(PasswordHash.crypted?(user.password)).to be_falsy
+      pw_plain = 'zammad'
+      sha2_pw  = PasswordHash.sha2(pw_plain)
+      user     = create(:user, password: sha2_pw)
 
-      result = instance.valid?(user, 'zammad')
+      expect(PasswordHash.crypted?(user.password)).to be true
+      expect(PasswordHash.legacy?(user.password, pw_plain)).to be true
+
+      result = instance.valid?(user, pw_plain)
       expect(result).to be true
 
+      expect(PasswordHash.legacy?(user.password, pw_plain)).to be false
       expect(PasswordHash.crypted?(user.password)).to be true
     end
   end

spec/lib/password_hash_spec.rb

@@ -20,6 +20,18 @@ RSpec.describe PasswordHash do
     it 'responds to legacy?' do
       expect(described_class).to respond_to(:legacy?)
     end
+
+    it 'responds to sha2' do
+      expect(described_class).to respond_to(:sha2)
+    end
+
+    it 'responds to hashed_sha2?' do
+      expect(described_class).to respond_to(:hashed_sha2?)
+    end
+
+    it 'responds to hashed_argon2?' do
+      expect(described_class).to respond_to(:hashed_argon2?)
+    end
   end
 
   context 'encryption' do
@@ -56,5 +68,25 @@ RSpec.describe PasswordHash do
     it 'detects sha2 hashes' do
       expect(described_class.legacy?(zammad_sha2, pw_plain)).to be true
     end
+
+    it 'detects crypted passwords' do
+      expect(described_class.crypted?(zammad_sha2)).to be true
+    end
+
+    describe '::sha2' do
+
+      it 'creates sha2 hashes' do
+        hashed = described_class.sha2(pw_plain)
+        expect(hashed).to eq zammad_sha2
+      end
+    end
+
+    describe '::hashed_sha2?' do
+
+      it 'detects sha2 hashes' do
+        expect(described_class.hashed_sha2?(zammad_sha2)).to be true
+      end
+    end
   end
+
 end