From 95c3f73da67589a3f6c825add30f28d70215867d Mon Sep 17 00:00:00 2001 From: Shinichi Maeshima Date: Fri, 10 Apr 2026 17:06:04 +0900 Subject: [PATCH] Expand controller coverage for Phase 1.3 Cover Sorcery::Controller core and controller submodule edge cases so the refactor roadmap has stronger regression protection. Mark Phase 1.3 complete after verifying the full spec suite passes. --- V1_ROADMAP.md | 18 +++--- ...orcery_controller_activity_logging_spec.rb | 7 +++ ..._controller_brute_force_protection_spec.rb | 8 +++ ...sorcery_controller_http_basic_auth_spec.rb | 18 ++++++ .../sorcery_controller_oauth_spec.rb | 61 +++++++++++++++++++ .../sorcery_controller_remember_me_spec.rb | 12 +++- ...sorcery_controller_session_timeout_spec.rb | 14 +++++ spec/controllers/sorcery_controller_spec.rb | 41 +++++++++++++ 8 files changed, 168 insertions(+), 11 deletions(-) diff --git a/V1_ROADMAP.md b/V1_ROADMAP.md index 7f4ee4a0..198c217d 100644 --- a/V1_ROADMAP.md +++ b/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 diff --git a/spec/controllers/sorcery_controller_activity_logging_spec.rb b/spec/controllers/sorcery_controller_activity_logging_spec.rb index 1ea49e1c..8ff9e0bf 100644 --- a/spec/controllers/sorcery_controller_activity_logging_spec.rb +++ b/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 diff --git a/spec/controllers/sorcery_controller_brute_force_protection_spec.rb b/spec/controllers/sorcery_controller_brute_force_protection_spec.rb index b2bc402b..28ce0338 100644 --- a/spec/controllers/sorcery_controller_brute_force_protection_spec.rb +++ b/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(['bla@example.com', '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!, ['bla@example.com', 'blabla']) + end end end diff --git a/spec/controllers/sorcery_controller_http_basic_auth_spec.rb b/spec/controllers/sorcery_controller_http_basic_auth_spec.rb index 3cc04771..564e1204 100644 --- a/spec/controllers/sorcery_controller_http_basic_auth_spec.rb +++ b/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 diff --git a/spec/controllers/sorcery_controller_oauth_spec.rb b/spec/controllers/sorcery_controller_oauth_spec.rb index 15ce29e8..e6fda0f4 100644 --- a/spec/controllers/sorcery_controller_oauth_spec.rb +++ b/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 diff --git a/spec/controllers/sorcery_controller_remember_me_spec.rb b/spec/controllers/sorcery_controller_remember_me_spec.rb index 33602bbe..364f1d0e 100644 --- a/spec/controllers/sorcery_controller_remember_me_spec.rb +++ b/spec/controllers/sorcery_controller_remember_me_spec.rb @@ -33,11 +33,15 @@ expect(response.cookies[:remember_me_token]).to be_nil end - it 'clears cookie on force_forget_me!' do - request.cookies[:remember_me_token] = { value: 'asd54234dsfsd43534', expires: 3600 } + it 'clears cookie and token on force_forget_me!' do + session[:user_id] = user.id.to_s + user.remember_me! + expect(user.reload.remember_me_token).not_to be_nil + request.cookies[:remember_me_token] = { value: user.remember_me_token, expires: 3600 } get :test_logout_with_force_forget_me expect(response.cookies[:remember_me_token]).to be_nil + expect(user.reload.remember_me_token).to be_nil end it 'login(email,password,remember_me) logs user in and remembers' do @@ -77,6 +81,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: 'bla@example.com', password: 'secret', remember: '0' } diff --git a/spec/controllers/sorcery_controller_session_timeout_spec.rb b/spec/controllers/sorcery_controller_session_timeout_spec.rb index 890f0255..55300880 100644 --- a/spec/controllers/sorcery_controller_session_timeout_spec.rb +++ b/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 diff --git a/spec/controllers/sorcery_controller_spec.rb b/spec/controllers/sorcery_controller_spec.rb index 7ba5f0f5..0a37deda 100644 --- a/spec/controllers/sorcery_controller_spec.rb +++ b/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('bla@example.com', 'wrong').and_yield(nil, :invalid_password) + + result = controller.login('bla@example.com', '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!, ['bla@example.com', 'wrong']) + + expect(yielded_credentials).to eq(['bla@example.com', '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