GFSB-1389 validating encrypted_password using given encryptor strategy with fallbacks (#5)

Author: Mike Li

.github/workflows/test.yml

@@ -1,130 +1,21 @@
 name: Test
 on: [push, pull_request]
+
 jobs:
   test:
+    runs-on: ubuntu-latest
     strategy:
-      fail-fast: false
       matrix:
-        gemfile:
-          - Gemfile
-          - gemfiles/Gemfile-rails-main
-          - gemfiles/Gemfile-rails-6-1
-          - gemfiles/Gemfile-rails-6-0
-          - gemfiles/Gemfile-rails-5-2
-          - gemfiles/Gemfile-rails-5-1
-          - gemfiles/Gemfile-rails-5-0
-          - gemfiles/Gemfile-rails-4-2
-          - gemfiles/Gemfile-rails-4-1
-        ruby:
-          - '3.1'
-          - '3.0'
-          - '2.7'
-          - '2.6'
-          - '2.5'
-          - '2.4'
-          - '2.3'
-          - '2.2'
-          - '2.1'
-        exclude:
-          - gemfile: Gemfile
-            ruby: '2.6'
-          - gemfile: Gemfile
-            ruby: '2.5'
-          - gemfile: Gemfile
-            ruby: '2.4'
-          - gemfile: Gemfile
-            ruby: '2.3'
-          - gemfile: Gemfile
-            ruby: '2.2'
-          - gemfile: Gemfile
-            ruby: '2.1'
-          - gemfile: gemfiles/Gemfile-rails-main
-            ruby: '2.6'
-          - gemfile: gemfiles/Gemfile-rails-main
-            ruby: '2.5'
-          - gemfile: gemfiles/Gemfile-rails-main
-            ruby: '2.4'
-          - gemfile: gemfiles/Gemfile-rails-main
-            ruby: '2.3'
-          - gemfile: gemfiles/Gemfile-rails-main
-            ruby: '2.2'
-          - gemfile: gemfiles/Gemfile-rails-main
-            ruby: '2.1'
-          - gemfile: gemfiles/Gemfile-rails-6-1
-            ruby: '2.4'
-          - gemfile: gemfiles/Gemfile-rails-6-1
-            ruby: '2.3'
-          - gemfile: gemfiles/Gemfile-rails-6-1
-            ruby: '2.2'
-          - gemfile: gemfiles/Gemfile-rails-6-1
-            ruby: '2.1'
-          - gemfile: gemfiles/Gemfile-rails-6-0
-            ruby: '3.1'
-          - gemfile: gemfiles/Gemfile-rails-6-0
-            ruby: '2.4'
-          - gemfile: gemfiles/Gemfile-rails-6-0
-            ruby: '2.3'
-          - gemfile: gemfiles/Gemfile-rails-6-0
-            ruby: '2.2'
-          - gemfile: gemfiles/Gemfile-rails-6-0
-            ruby: '2.1'
-          - gemfile: gemfiles/Gemfile-rails-5-2
-            ruby: '3.1'
-          - gemfile: gemfiles/Gemfile-rails-5-2
-            ruby: '3.0'
-          - gemfile: gemfiles/Gemfile-rails-5-2
-            ruby: '2.7'
-          - gemfile: gemfiles/Gemfile-rails-5-2
-            ruby: '2.2'
-          - gemfile: gemfiles/Gemfile-rails-5-2
-            ruby: '2.1'
-          - gemfile: gemfiles/Gemfile-rails-5-1
-            ruby: '3.1'
-          - gemfile: gemfiles/Gemfile-rails-5-1
-            ruby: '3.0'
-          - gemfile: gemfiles/Gemfile-rails-5-1
-            ruby: '2.7'
-          - gemfile: gemfiles/Gemfile-rails-5-1
-            ruby: '2.1'
-          - gemfile: gemfiles/Gemfile-rails-5-0
-            ruby: '3.1'
-          - gemfile: gemfiles/Gemfile-rails-5-0
-            ruby: '3.0'
-          - gemfile: gemfiles/Gemfile-rails-5-0
-            ruby: '2.7'
-          - gemfile: gemfiles/Gemfile-rails-5-0
-            ruby: '2.1'
-          - gemfile: gemfiles/Gemfile-rails-4-2
-            ruby: '3.1'
-          - gemfile: gemfiles/Gemfile-rails-4-2
-            ruby: '3.0'
-          - gemfile: gemfiles/Gemfile-rails-4-2
-            ruby: '2.7'
-          - gemfile: gemfiles/Gemfile-rails-4-2
-            ruby: '2.6'
-          - gemfile: gemfiles/Gemfile-rails-4-1
-            ruby: '3.1'
-          - gemfile: gemfiles/Gemfile-rails-4-1
-            ruby: '3.0'
-          - gemfile: gemfiles/Gemfile-rails-4-1
-            ruby: '2.7'
-          - gemfile: gemfiles/Gemfile-rails-4-1
-            ruby: '2.6'
-          - gemfile: gemfiles/Gemfile-rails-4-1
-            ruby: '2.5'
-          - gemfile: gemfiles/Gemfile-rails-4-1
-            ruby: '2.4'
-    runs-on: ubuntu-latest
-    env: # $BUNDLE_GEMFILE must be set at the job level, so it is set for all steps
-      BUNDLE_GEMFILE: ${{ matrix.gemfile }}
+        ruby-version: ['3.2', '3.1']
+
     steps:
-      - uses: actions/checkout@v2
-      - name: Setup Bundler 1.x for Rails 4.x
-        if: ${{ matrix.gemfile == 'gemfiles/Gemfile-rails-4-1' || matrix.gemfile == 'gemfiles/Gemfile-rails-4-2' }}
-        run: echo "BUNDLER_VERSION=1.17.3" >> $GITHUB_ENV
-      - uses: ruby/setup-ruby@v1
+      - uses: actions/checkout@v3
+      - name: Set up Ruby ${{ matrix.ruby-version }}
+        uses: ruby/setup-ruby@v1
         with:
-          ruby-version: ${{ matrix.ruby }}
-          bundler-cache: true # runs bundle install and caches installed gems automatically
-          bundler: ${{ env.BUNDLER_VERSION || 'latest' }}
-      - run: bundle exec rake
+          ruby-version: ${{ matrix.ruby-version }}
+          bundler-cache: true
+      - name: Install dependencies
+        run: bundle install
+      - name: Run tests
+        run: bundle exec rake

Gemfile

@@ -7,3 +7,5 @@ gem 'rails', '~> 7.0.0'
 gem 'sqlite3'
 
 gem 'mocha', '~> 1.0', require: false
+
+gem 'simplecov', require: false, group: :test

Gemfile.lock

@@ -83,6 +83,7 @@ GEM
       responders
       warden (~> 1.2.3)
     digest (3.1.0)
+    docile (1.4.0)
     erubi (1.10.0)
     globalid (1.0.0)
       activesupport (>= 5.0)
@@ -154,6 +155,12 @@ GEM
     responders (3.0.1)
       actionpack (>= 5.0)
       railties (>= 5.0)
+    simplecov (0.22.0)
+      docile (~> 1.1)
+      simplecov-html (~> 0.11)
+      simplecov_json_formatter (~> 0.1)
+    simplecov-html (0.12.3)
+    simplecov_json_formatter (0.1.4)
     sqlite3 (1.4.2)
     strscan (3.0.1)
     thor (1.2.1)
@@ -175,6 +182,7 @@ DEPENDENCIES
   devise-encryptable!
   mocha (~> 1.0)
   rails (~> 7.0.0)
+  simplecov
   sqlite3
 
 BUNDLED WITH

lib/devise/migratable/encryptors/pbkdf2_sha512.rb

@@ -19,6 +19,13 @@ def self.digest(password, stretches, salt, pepper)
 
             format_hash(STRATEGY, stretches, salt, checksum)
           end
+
+          def self.valid_hash?(pass)
+            split_digest(pass)
+            true
+          rescue StandardError
+            false
+          end
 
           private_class_method def self.sha512_checksum(password, stretches, salt, pepper)
             hash = OpenSSL::Digest.new('SHA512')
@@ -66,4 +73,4 @@ def self.digest(password, stretches, salt, pepper)
       end
     end
   end
-  
+  

lib/devise/migratable/migratable.rb

@@ -3,20 +3,20 @@ module Devise
   mattr_accessor :encryptor
   @@encryptor = nil
 
-  # block to be evaluated to determine wether to validate using encryptor
-  mattr_accessor :enable_validation
-  @@enable_validation = nil
+  mattr_accessor :override_existing_password_hash
+  @@override_existing_password_hash = nil
 
-  # Sets enable_validation block
+
+  # Sets override_existing_password_hash block
   #
   #  Devise.setup do |config|
   #
-  #    config.validate_using_encryptor do |user|
-  #      Features.active?(:enable_pbkdf2_validation, user)
+  #    config.override_existing_password_hash_check do |user|
+  #      Features.active?(:override_existing_password_hash_using_encryptor, user)
   #    end
   #  end
-  def self.validate_using_encryptor(&block)
-    @@enable_validation = block
+  def self.override_existing_password_hash_check(&block)
+    @@override_existing_password_hash = block
   end
 
   module Migratable

lib/devise/migratable/model.rb

@@ -1,4 +1,5 @@
 require 'devise/strategies/database_authenticatable'
+require 'bcrypt'
 
 module Devise
   module Models
@@ -50,26 +51,29 @@ def self.required_fields(_klass)
       # Generates new password for the new encryptor
       # generate password of the old one using super
       def password=(new_password)
-        # everytime we store the new one with the given encryptor
-        self.encrypted_password_migrate_to = generate_digest_for_password(new_password)
-        # this will set the original password to the database, nothing changed
+        unless override_existing_password_hash_enabled?
+          self.encrypted_password_migrate_to = generate_digest_for_password(new_password)
+        end
         super
       end
 
       # Validates the password considering the salt.
       def valid_password?(password)
         return false if encrypted_password.blank?
 
-        # only if feature enabled? then we do the work here
-        if encryptor_validation_enabled? && encrypted_password_migrate_to.present?
+        if valid_encryptor_hash?
           valid_password_using_encryptor?(password)
         else
           result = super
-          update_encrypted_password_migrate_to(password) if result && encrypted_password_migrate_to.nil?
+          update_encrypted_password_hash(password) if result
           result
         end
       end
 
+      def valid_encryptor_hash?
+        encryptor_class.valid_hash?(encrypted_password)
+      end
+
       # redefine serializable_hash to prevent encrypted_password_migrate_to leaking
       def serializable_hash(options = {})
         options[:except] ||= []
@@ -79,12 +83,17 @@ def serializable_hash(options = {})
 
       protected
 
-      def update_encrypted_password_migrate_to(password)
+      def password_digest(password)
+        override_existing_password_hash_enabled? ? generate_digest_for_password(password) : super
+      end
+
+      def update_encrypted_password_hash(password)
         return if new_record?
 
-        update_column(:encrypted_password_migrate_to, generate_digest_for_password(password))
+        column_name = override_existing_password_hash_enabled? ? :encrypted_password : :encrypted_password_migrate_to
+        update_column(column_name, generate_digest_for_password(password))
       rescue StandardError => e # capture StandardError instead of ActiveRecordError to play safe
-        log_error('Failed to update_column encrypted_password_migrate_to', e)
+        log_error("Failed to update_column #{column_name}", e)
       end
 
       def generate_digest_for_password(password)
@@ -94,13 +103,13 @@ def generate_digest_for_password(password)
                                self.class.pepper)
       end
 
-      def encryptor_validation_enabled?
-        self.class.enable_validation&.call(self)
+      def override_existing_password_hash_enabled?
+        self.class.override_existing_password_hash&.call(self)
       end
 
       def valid_password_using_encryptor?(password)
         encryptor_arguments = [
-          encrypted_password_migrate_to,
+          encrypted_password,
           password,
           self.class.stretches,
           self.class.password_salt,
@@ -119,7 +128,7 @@ def encryptor_class
 
       # class method get injected into Devise module
       module ClassMethods
-        Devise::Models.config(self, :encryptor, :enable_validation)
+        Devise::Models.config(self, :encryptor, :override_existing_password_hash)
 
         # Returns the class for the configured encryptor.
         def encryptor_class
@@ -135,9 +144,9 @@ def password_salt
         def compute_encryptor_class(encryptor)
           case encryptor
           when :bcrypt
-            raise 'In order to use bcrypt as encryptor, simply remove :encryptable from your devise model'
+            raise 'In order to use bcrypt as encryptor, simply remove :migratable from your devise model'
           when nil
-            raise 'You need to give an :encryptor as option in order to use :encryptable'
+            raise 'You need to give an :encryptor as option in order to use :migratable'
           else
             Devise::Migratable::Encryptors.const_get(encryptor.to_s.classify)
           end