Merge pull request #109 from omniauth/validation-regression

fix: options[:on_validation] regression

Author: pboling

.rubocop_todo.yml

@@ -1,6 +1,6 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2021-02-14 08:34:14 UTC using RuboCop version 1.9.1.
+# on 2021-03-21 02:34:29 UTC using RuboCop version 1.11.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
@@ -34,7 +34,7 @@ Lint/NestedMethodDefinition:
 # Offense count: 1
 # Configuration parameters: IgnoredMethods, CountRepeatedAttributes.
 Metrics/AbcSize:
-  Max: 42
+  Max: 30
 
 # Offense count: 1
 # Configuration parameters: CountComments, CountAsOne, ExcludedMethods, IgnoredMethods.
@@ -45,22 +45,17 @@ Metrics/BlockLength:
 # Offense count: 1
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
-  Max: 140
-
-# Offense count: 1
-# Configuration parameters: IgnoredMethods.
-Metrics/CyclomaticComplexity:
-  Max: 9
+  Max: 145
 
 # Offense count: 7
 # Configuration parameters: CountComments, CountAsOne, ExcludedMethods, IgnoredMethods.
 Metrics/MethodLength:
-  Max: 21
+  Max: 20
 
 # Offense count: 2
 # Configuration parameters: IgnoredMethods.
 Metrics/PerceivedComplexity:
-  Max: 11
+  Max: 9
 
 # Offense count: 1
 Naming/AccessorMethodName:
@@ -131,14 +126,14 @@ RSpec/LeakyConstantDeclaration:
 RSpec/MessageSpies:
   EnforcedStyle: receive
 
-# Offense count: 8
+# Offense count: 9
 RSpec/MultipleExpectations:
   Max: 4
 
-# Offense count: 3
+# Offense count: 14
 # Configuration parameters: AllowSubject.
 RSpec/MultipleMemoizedHelpers:
-  Max: 6
+  Max: 8
 
 # Offense count: 44
 # Configuration parameters: IgnoreSharedExamples.
@@ -148,9 +143,9 @@ RSpec/NamedSubject:
     - 'spec/omniauth/identity/models/couch_potato_spec.rb'
     - 'spec/omniauth/identity/models/mongoid_spec.rb'
 
-# Offense count: 6
+# Offense count: 12
 RSpec/NestedGroups:
-  Max: 4
+  Max: 5
 
 # Offense count: 4
 RSpec/StubbedMock:

lib/omniauth/strategies/identity.rb

@@ -20,6 +20,11 @@ class Identity
       option :on_registration, nil        # See #registration_phase
       option :on_failed_registration, nil # See #registration_phase
       option :locate_conditions, ->(req) { { model.auth_key => req['auth_key'] } }
+      option :create_identity_link_text, 'Create an Identity'
+      option :registration_failure_message, 'One or more fields were invalid'
+      option :validation_failure_message, 'Validation failed'
+      option :title, 'Identity Verification' # Title for Login Form
+      option :registration_form_title, 'Register Identity' # Title for Registration Form
 
       def request_phase
         if options[:on_login]
@@ -71,14 +76,14 @@ def registration_phase
         if model.respond_to?(:column_names) && model.column_names.include?('provider')
           attributes.reverse_merge!(provider: 'identity')
         end
+        @identity = model.new(attributes)
         if saving_instead_of_creating?
-          @identity = model.new(attributes)
           env['omniauth.identity'] = @identity
           if !validating? || valid?
             @identity.save
             registration_result
           else
-            registration_failure('Validation failed')
+            registration_failure(options[:validation_failure_message])
           end
         else
           deprecated_registration(attributes)
@@ -114,19 +119,19 @@ def model
 
       def build_omniauth_login_form
         OmniAuth::Form.build(
-          title: (options[:title] || 'Identity Verification'),
+          title: options[:title],
           url: callback_path
         ) do |f|
           f.text_field 'Login', 'auth_key'
           f.password_field 'Password', 'password'
           if options[:enable_registration]
-            f.html "<p align='center'><a href='#{registration_path}'>Create an Identity</a></p>"
+            f.html "<p align='center'><a href='#{registration_path}'>#{options[:create_identity_link_text]}</a></p>"
           end
         end
       end
 
       def build_omniauth_registration_form(validation_message)
-        OmniAuth::Form.build(title: 'Register Identity') do |f|
+        OmniAuth::Form.build(title: options[:registration_form_title]) do |f|
           f.html "<p style='color:red'>#{validation_message}</p>" if validation_message
           options[:fields].each do |field|
             f.text_field field.to_s.capitalize, field.to_s
@@ -137,14 +142,14 @@ def build_omniauth_registration_form(validation_message)
       end
 
       def saving_instead_of_creating?
-        model.respond_to?(:save) && model.respond_to?(:persisted?)
+        @identity.respond_to?(:save) && @identity.respond_to?(:persisted?)
       end
 
       # Validates the model before it is persisted
       #
       # @return [truthy or falsey] :on_validation option is truthy or falsey
       def validating?
-        options[:on_validation]
+        !!options[:on_validation]
       end
 
       # Validates the model before it is persisted
@@ -169,7 +174,7 @@ def registration_result
           env['PATH_INFO'] = callback_path
           callback_phase
         else
-          registration_failure('One or more fields were invalid')
+          registration_failure(options[:registration_failure_message])
         end
       end
 

spec/omniauth/strategies/identity_spec.rb

@@ -3,8 +3,9 @@
 RSpec.describe OmniAuth::Strategies::Identity do
   attr_accessor :app
 
-  let(:auth_hash) { last_response.headers['env']['omniauth.auth'] }
-  let(:identity_hash) { last_response.headers['env']['omniauth.identity'] }
+  let(:env_hash) { last_response.headers['env'] }
+  let(:auth_hash) { env_hash['omniauth.auth'] }
+  let(:identity_hash) { env_hash['omniauth.identity'] }
   let(:identity_options) { {} }
   let(:anon_ar) do
     AnonymousActiveRecord.generate(
@@ -193,7 +194,7 @@ def set_app!(identity_options = {})
       end
     end
 
-    context 'with successful creation' do
+    context 'with good identity' do
       let(:properties) do
         {
           name: 'Awesome Dude',
@@ -209,9 +210,66 @@ def set_app!(identity_options = {})
         expect(auth_hash['uid']).to match(/\d+/)
         expect(auth_hash['provider']).to eq('identity')
       end
+
+      context 'with on_validation proc' do
+        let(:identity_options) do
+          { model: anon_ar, on_validation: on_validation_proc }
+        end
+        let(:on_validation_proc) do
+          lambda { |_env|
+            false
+          }
+        end
+
+        context 'when validation fails' do
+          it 'does not set the env hash' do
+            post '/auth/identity/register', properties
+            expect(env_hash).to eq(nil)
+          end
+
+          it 'renders registration form' do
+            post '/auth/identity/register', properties
+            expect(last_response.body).to be_include(described_class.default_options[:registration_form_title])
+          end
+
+          it 'displays validation failure message' do
+            post '/auth/identity/register', properties
+            expect(last_response.body).to be_include(described_class.default_options[:validation_failure_message])
+          end
+        end
+
+        context 'when validation succeeds' do
+          let(:on_validation_proc) do
+            lambda { |_env|
+              true
+            }
+          end
+
+          it 'sets the auth hash' do
+            post '/auth/identity/register', properties
+            expect(auth_hash['uid']).to match(/\d+/)
+            expect(auth_hash['provider']).to eq('identity')
+          end
+
+          it 'does not render registration form' do
+            post '/auth/identity/register', properties
+            expect(last_response.body).not_to be_include(described_class.default_options[:registration_form_title])
+          end
+
+          it 'does not display validation failure message' do
+            post '/auth/identity/register', properties
+            expect(last_response.body).not_to be_include(described_class.default_options[:validation_failure_message])
+          end
+
+          it 'does not display registration failure message' do
+            post '/auth/identity/register', properties
+            expect(last_response.body).not_to be_include(described_class.default_options[:registration_failure_message])
+          end
+        end
+      end
     end
 
-    context 'with invalid identity' do
+    context 'with bad identity' do
       let(:properties) do
         {
           name: 'Awesome Dude',
@@ -249,6 +307,62 @@ def set_app!(identity_options = {})
           expect(last_response.body).not_to be_include('One or more fields were invalid')
         end
       end
+
+      context 'with on_validation proc' do
+        let(:identity_options) do
+          { model: anon_ar, on_validation: on_validation_proc }
+        end
+        let(:on_validation_proc) do
+          lambda { |_env|
+            false
+          }
+        end
+
+        context 'when validation fails' do
+          it 'does not set the env hash' do
+            post '/auth/identity/register', properties
+            expect(env_hash).to eq(nil)
+          end
+
+          it 'renders registration form' do
+            post '/auth/identity/register', properties
+            expect(last_response.body).to be_include(described_class.default_options[:registration_form_title])
+          end
+
+          it 'displays validation failure message' do
+            post '/auth/identity/register', properties
+            expect(last_response.body).to be_include(described_class.default_options[:validation_failure_message])
+          end
+        end
+
+        context 'when validation succeeds' do
+          let(:on_validation_proc) do
+            lambda { |_env|
+              true
+            }
+          end
+
+          it 'does not set the env hash' do
+            post '/auth/identity/register', properties
+            expect(env_hash).to eq(nil)
+          end
+
+          it 'renders registration form' do
+            post '/auth/identity/register', properties
+            expect(last_response.body).to be_include(described_class.default_options[:registration_form_title])
+          end
+
+          it 'does not display validation failure message' do
+            post '/auth/identity/register', properties
+            expect(last_response.body).not_to be_include(described_class.default_options[:validation_failure_message])
+          end
+
+          it 'display registration failure message' do
+            post '/auth/identity/register', properties
+            expect(last_response.body).to be_include(described_class.default_options[:registration_failure_message])
+          end
+        end
+      end
     end
   end
 end