Expand controller coverage for Phase 1.3

V1_ROADMAP.md

@@ -53,16 +53,16 @@ internal structural changes.
 
 ### 1.3 — Improve Controller Submodule Test Coverage
 
-- [ ] Review and expand tests for `Sorcery::Controller` core (login, logout,
+- [x] Review and expand tests for `Sorcery::Controller` core (login, logout,
       session management).
-- [ ] Improve coverage for each controller submodule:
-  - [ ] `remember_me`
-  - [ ] `session_timeout`
-  - [ ] `brute_force_protection`
-  - [ ] `http_basic_auth`
-  - [ ] `activity_logging`
-  - [ ] `external` (OAuth)
-- [ ] Add integration tests that exercise multiple submodules working together.
+- [x] Improve coverage for each controller submodule:
+  - [x] `remember_me`
+  - [x] `session_timeout`
+  - [x] `brute_force_protection`
+  - [x] `http_basic_auth`
+  - [x] `activity_logging`
+  - [x] `external` (OAuth)
+- [x] Add integration tests that exercise multiple submodules working together.
 
 ### 1.4 — Improve Provider Test Coverage
 

spec/controllers/sorcery_controller_activity_logging_spec.rb

@@ -111,5 +111,12 @@
 
       expect(user.reload.last_login_from_ip_address).to be_nil
     end
+
+    it 'does not register last activity time when nobody is logged in' do
+      sorcery_controller_property_set(:register_last_activity_time, true)
+      expect(user).not_to receive(:set_last_activity_at)
+
+      controller.send(:register_last_activity_time_to_db)
+    end
   end
 end

spec/controllers/sorcery_controller_brute_force_protection_spec.rb

@@ -48,5 +48,13 @@ def request_test_login
       expect(controller).to receive(:after_login_lock!).once
       request_test_login
     end
+
+    it 'does not increment failed logins for an already locked user' do
+      allow(User.sorcery_adapter).to receive(:find_by_credentials).with(['[email protected]', 'blabla']).and_return(user)
+      allow(user).to receive(:login_locked?).and_return(true)
+      expect(user).not_to receive(:register_failed_login!)
+
+      controller.send(:update_failed_logins_count!, ['[email protected]', 'blabla'])
+    end
   end
 end

spec/controllers/sorcery_controller_http_basic_auth_spec.rb

@@ -59,5 +59,23 @@
 
       expect(session[:user_id]).to eq user.id.to_s
     end
+
+    it 'does not authenticate from http basic before it was explicitly requested' do
+      @request.env['HTTP_AUTHORIZATION'] = "Basic #{Base64.encode64("#{user.email}:secret")}"
+      expect(User).not_to receive(:authenticate)
+
+      expect(controller.send(:login_from_basic_auth)).to be false
+    end
+
+    it 'returns nil for realm lookup when no controller in the hierarchy is mapped' do
+      sorcery_controller_property_set(:controller_to_realm_map, {})
+      subclass = Class.new(SorceryController) do
+        def self.controller_name
+          'unmapped'
+        end
+      end
+
+      expect(subclass.new.send(:realm_name_by_controller)).to be_nil
+    end
   end
 end

spec/controllers/sorcery_controller_oauth_spec.rb

@@ -127,6 +127,39 @@ def response.body
         expect(response).to be_a_redirect
       end
     end
+
+    it 'returns nil for unsupported providers' do
+      expect(controller.send(:sorcery_get_provider, :unknown)).to be_nil
+    end
+
+    it 'returns nil when a provider has no callback flow' do
+      provider = Object.new
+      provider.singleton_class.attr_accessor :callback_url, :original_callback_url, :state
+      provider.callback_url = nil
+      provider.original_callback_url = nil
+      def provider.has_callback?
+        false
+      end
+
+      allow(controller).to receive(:sorcery_get_provider).with('twitter').and_return(provider)
+
+      expect(controller.send(:sorcery_login_url, 'twitter')).to be_nil
+    end
+
+    it 'uses https in callback urls behind a forwarded https proxy' do
+      sorcery_controller_external_property_set(:twitter, :callback_url, '/oauth/twitter/callback')
+      request.env['HTTP_X_FORWARDED_PROTO'] = 'https'
+
+      get :login_at_test
+
+      expect(response).to redirect_to('http://api.example.com/oauth/authorize?oauth_callback=https%3A%2F%2Ftest.host%2Foauth%2Ftwitter%2Fcallback&oauth_token=')
+    end
+
+    it 'exposes the fetched access token through the compatibility accessor' do
+      controller.send(:sorcery_fetch_user_hash, :twitter)
+
+      expect(controller.send(:access_token)).to be_a(OAuth::AccessToken)
+    end
   end
 
   describe SorceryController do
@@ -135,6 +168,25 @@ def response.body
         stub_all_oauth_requests!
       end
 
+      it 'stores incomplete external user data when validation fails' do
+        sorcery_controller_external_property_set(:twitter, :user_info_mapping, username: 'screen_name')
+        unsaved_user = User.new
+        expect(User).to receive(:create_and_validate_from_provider)
+          .with(:twitter, '123', { username: 'nbenari' })
+          .and_return([unsaved_user, false])
+
+        result = controller.send(:create_and_validate_from, :twitter)
+
+        expect(result).to eq(unsaved_user)
+        expect(session[:incomplete_user]).to eq(
+          provider: {
+            User.sorcery_config.provider_uid_attribute_name => '123',
+            User.sorcery_config.provider_attribute_name => :twitter
+          },
+          user_hash: { username: 'nbenari' }
+        )
+      end
+
       it 'creates a new user' do
         sorcery_controller_external_property_set(:twitter, :user_info_mapping, username: 'screen_name')
         expect(User).to receive(:load_from_provider).with('twitter', '123').and_return(nil)
@@ -182,6 +234,15 @@ def response.body
           get :test_create_from_provider_with_block, params: { provider: 'twitter' }
         end
       end
+
+      it 'builds a user from provider data' do
+        sorcery_controller_external_property_set(:twitter, :user_info_mapping, username: 'screen_name')
+        built_user = User.new
+        expect(User).to receive(:build_from_provider).with({ username: 'nbenari' }).and_return(built_user)
+
+        expect(controller.send(:build_from, :twitter)).to eq(built_user)
+        expect(controller.instance_variable_get(:@user)).to eq(built_user)
+      end
     end
   end
 

spec/controllers/sorcery_controller_remember_me_spec.rb

@@ -40,6 +40,15 @@
       expect(response.cookies[:remember_me_token]).to be_nil
     end
 
+    it 'force_forget_me! clears the cookie and token directly' do
+      controller.current_user = user
+      cookie_jar = controller.send(:cookies)
+      expect(user).to receive(:force_forget_me!)
+      expect(cookie_jar).to receive(:delete).with(:remember_me_token, domain: Sorcery::Controller::Config.cookie_domain)
+
+      controller.send(:force_forget_me!)
+    end
+
     it 'login(email,password,remember_me) logs user in and remembers' do
       expect(User).to receive(:authenticate).with('[email protected]', 'secret', '1').and_yield(user, nil)
       expect(user).to receive(:remember_me!)
@@ -77,6 +86,10 @@
       expect(assigns[:current_user]).to eq user
     end
 
+    it 'returns false when logging in from cookie without a cookie' do
+      expect(controller.send(:login_from_cookie)).to be false
+    end
+
     it 'doest not remember_me! when not asked to, even if third parameter is used' do
       post :test_login_with_remember_in_login, params: { email: '[email protected]', password: 'secret', remember: '0' }
 

spec/controllers/sorcery_controller_session_timeout_spec.rb

@@ -161,5 +161,19 @@
       expect(session[:login_time]).not_to be_nil
       expect(session[:last_action_time]).not_to be_nil
     end
+
+    it 'does nothing when invalidating active sessions is disabled' do
+      sorcery_controller_property_set(:session_timeout_invalidate_active_sessions_enabled, false)
+      controller.current_user = user
+      expect(user).not_to receive(:save)
+
+      controller.send(:invalidate_active_sessions!)
+    end
+
+    it 'does nothing when there is no current user to invalidate' do
+      sorcery_controller_property_set(:session_timeout_invalidate_active_sessions_enabled, true)
+
+      expect { controller.send(:invalidate_active_sessions!) }.not_to raise_error
+    end
   end
 end

spec/controllers/sorcery_controller_spec.rb

@@ -87,6 +87,18 @@
           expect(session[:user_id]).to be_nil
         end
       end
+
+      it 'yields the failure reason when authentication fails with a reason' do
+        yielded = nil
+        allow(User).to receive(:authenticate).with('[email protected]', 'wrong').and_yield(nil, :invalid_password)
+
+        result = controller.login('[email protected]', 'wrong') do |user, failure_reason|
+          yielded = [user, failure_reason]
+        end
+
+        expect(result).to be_nil
+        expect(yielded).to eq([nil, :invalid_password])
+      end
     end
 
     describe '#login!' do
@@ -229,6 +241,35 @@
 
         2.times { expect(controller.current_user).to be_nil } # memoized!
       end
+
+      it 'allows assigning current_user directly' do
+        controller.current_user = user
+
+        expect(controller.current_user).to eq(user)
+      end
+    end
+
+    describe 'after_login_lock callback' do
+      it 'runs configured callbacks' do
+        yielded_credentials = nil
+        sorcery_controller_property_set(:after_login_lock, [:capture_login_lock_credentials])
+        controller.define_singleton_method(:capture_login_lock_credentials) do |credentials|
+          yielded_credentials = credentials
+        end
+
+        controller.send(:after_login_lock!, ['[email protected]', 'wrong'])
+
+        expect(yielded_credentials).to eq(['[email protected]', 'wrong'])
+      end
+    end
+
+    describe '#user_class' do
+      it 'raises a helpful error when configured user_class is invalid' do
+        sorcery_controller_property_set(:user_class, 'MissingUserClass')
+
+        expect { controller.send(:user_class) }
+          .to raise_error(ArgumentError, /incorrectly defined user_class/)
+      end
     end
 
     it "calls the configured 'not_authenticated_action' when authenticate before_action fails" do