diff --git a/README.md b/README.md index d4a6064..7c1110c 100644 --- a/README.md +++ b/README.md @@ -115,9 +115,9 @@ In `config/initializers/devise.rb`: # sure that the Authentication Response includes the attribute. config.saml_default_user_key = :email - # Optional. This stores the session index defined by the IDP during login. If provided it will be used as a salt - # for the user's session to facilitate an IDP initiated logout request. - config.saml_session_index_key = :session_index + # Optional. This stores the session index defined by the IDP during login. + # If provided it will be used to facilitate an IDP initiated logout request. + config.saml_session_index_key = :saml_session_index # You can set this value to use Subject or SAML assertion as info to which email will be compared. # If you don't set it then email will be extracted from SAML assertion attributes. @@ -303,7 +303,7 @@ Logout support is included by immediately terminating the local session and then Logout requests from the IDP are supported by the `idp_sign_out` endpoint. Directing logout requests to `users/saml/idp_sign_out` will log out the respective user by invalidating their current sessions. -`saml_session_index_key` must be configured to support this feature. +To disable this feature, set `saml_session_index_key` to `nil`. ## Signing and Encrypting Authentication Requests and Assertions diff --git a/app/controllers/devise/saml_sessions_controller.rb b/app/controllers/devise/saml_sessions_controller.rb index 02a512c..10d27b3 100644 --- a/app/controllers/devise/saml_sessions_controller.rb +++ b/app/controllers/devise/saml_sessions_controller.rb @@ -23,10 +23,11 @@ def metadata def idp_sign_out if params[:SAMLRequest] && Devise.saml_session_index_key + Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name) + session[Devise.saml_session_index_key] = nil + saml_config = saml_config(get_idp_entity_id(params), request) logout_request = OneLogin::RubySaml::SloLogoutrequest.new(params[:SAMLRequest], settings: saml_config) - resource_class.reset_session_key_for(logout_request.name_id) - redirect_to generate_idp_logout_response(saml_config, logout_request.id), allow_other_host: true elsif params[:SAMLResponse] # Currently Devise handles the session invalidation when the request is made. @@ -56,7 +57,7 @@ def store_info_for_sp_initiated_logout @name_identifier_value_for_sp_initiated_logout = Devise.saml_name_identifier_retriever.call(current_user) if Devise.saml_session_index_key - @sessionindex_for_sp_initiated_logout = current_user.public_send(Devise.saml_session_index_key) + @sessionindex_for_sp_initiated_logout = session[Devise.saml_session_index_key] end end diff --git a/lib/devise_saml_authenticatable.rb b/lib/devise_saml_authenticatable.rb index 4c75458..b24659c 100644 --- a/lib/devise_saml_authenticatable.rb +++ b/lib/devise_saml_authenticatable.rb @@ -30,7 +30,7 @@ module Devise # Add valid users to database # Can accept a Boolean value or a Proc that is called with the model class, the saml_response and auth_value - # Ex: + # Ex: # Devise.saml_create_user = Proc.new do |model_class, saml_response, auth_value| # model_class == Admin # end @@ -39,7 +39,7 @@ module Devise # Update user attributes after login # Can accept a Boolean value or a Proc that is called with the model class, the saml_response and auth_value - # Ex: + # Ex: # Devise.saml_update_user = Proc.new do |model_class, saml_response, auth_value| # model_class == User # end @@ -54,7 +54,7 @@ module Devise # Key used to index sessions for later retrieval mattr_accessor :saml_session_index_key - @@saml_session_index_key + @@saml_session_index_key ||= :saml_session_index # Redirect after signout (redirects to 'users/saml/sign_in' by default) mattr_accessor :saml_sign_out_success_url diff --git a/lib/devise_saml_authenticatable/model.rb b/lib/devise_saml_authenticatable/model.rb index c9b11e9..d76fb85 100644 --- a/lib/devise_saml_authenticatable/model.rb +++ b/lib/devise_saml_authenticatable/model.rb @@ -12,22 +12,6 @@ module SamlAuthenticatable attr_accessor :password_confirmation end - def after_saml_authentication(session_index) - if Devise.saml_session_index_key && self.respond_to?(Devise.saml_session_index_key) - self.update_attribute(Devise.saml_session_index_key, session_index) - end - end - - def authenticatable_salt - if Devise.saml_session_index_key && - self.respond_to?(Devise.saml_session_index_key) && - self.send(Devise.saml_session_index_key).present? - self.send(Devise.saml_session_index_key) - else - super - end - end - module ClassMethods def authenticate_with_saml(saml_response, relay_state) key = Devise.saml_default_user_key @@ -78,11 +62,6 @@ def authenticate_with_saml(saml_response, relay_state) resource end - def reset_session_key_for(name_id) - resource = find_by(Devise.saml_default_user_key => name_id) - resource.update_attribute(Devise.saml_session_index_key, nil) unless resource.nil? - end - def find_for_shibb_authentication(conditions) find_for_authentication(conditions) end diff --git a/lib/devise_saml_authenticatable/strategy.rb b/lib/devise_saml_authenticatable/strategy.rb index 340327d..620dfb5 100644 --- a/lib/devise_saml_authenticatable/strategy.rb +++ b/lib/devise_saml_authenticatable/strategy.rb @@ -19,7 +19,9 @@ def authenticate! parse_saml_response retrieve_resource unless self.halted? unless self.halted? - @resource.after_saml_authentication(@response.sessionindex) + if Devise.saml_session_index_key + request.session[Devise.saml_session_index_key] = @response.sessionindex + end success!(@resource) end end diff --git a/spec/controllers/devise/saml_sessions_controller_spec.rb b/spec/controllers/devise/saml_sessions_controller_spec.rb index 8bc56e4..bc3a325 100644 --- a/spec/controllers/devise/saml_sessions_controller_spec.rb +++ b/spec/controllers/devise/saml_sessions_controller_spec.rb @@ -31,6 +31,7 @@ def destroy describe Devise::SamlSessionsController, type: :controller do include RubySamlSupport + include Devise::Test::ControllerHelpers let(:idp_providers_adapter) { spy('Stub IDPSettings Adaptor') } @@ -256,7 +257,8 @@ def all_signed_out? end it 'includes a LogoutRequest with the name identifier and session index', :aggregate_failures do - controller.current_user = Struct.new(:email, :session_index).new('user@example.com', 'sessionindex') + controller.current_user = Struct.new(:email).new('user@example.com') + session[Devise.saml_session_index_key] = 'sessionindex' actual_settings = nil expect_any_instance_of(OneLogin::RubySaml::Logoutrequest).to receive(:create) do |_, settings| @@ -322,9 +324,10 @@ def self.entity_id(params) end it 'returns invalid request if SAMLRequest or SAMLResponse is not passed' do - expect(User).not_to receive(:reset_session_key_for) + session[Devise.saml_session_index_key] = 'sessionindex' post :idp_sign_out expect(response.status).to eq 500 + expect(session[Devise.saml_session_index_key]).to eq('sessionindex') end context 'when receiving a logout response from the IdP after redirecting an SP logout request' do @@ -367,13 +370,13 @@ def self.entity_id(params) let(:name_id) { '12312312' } before do allow(OneLogin::RubySaml::SloLogoutrequest).to receive(:new).and_return(saml_request) - allow(User).to receive(:reset_session_key_for) + session[Devise.saml_session_index_key] = 'sessionindex' end it 'direct the resource to reset the session key' do do_post expect(response).to redirect_to response_url - expect(User).to have_received(:reset_session_key_for).with(name_id) + expect(session[Devise.saml_session_index_key]).to be_nil end context 'with a specified idp' do @@ -387,6 +390,7 @@ def self.entity_id(params) expect(response.status).to eq 302 expect(idp_providers_adapter).to have_received(:settings).with(idp_entity_id, request) expect(response).to redirect_to 'http://localhost/logout_response' + expect(session[Devise.saml_session_index_key]).to be_nil end end @@ -407,7 +411,6 @@ def self.entity_id(params) end it 'returns invalid request' do - expect(User).not_to receive(:reset_session_key_for).with(name_id) do_post expect(response.status).to eq 500 end diff --git a/spec/devise_saml_authenticatable/strategy_spec.rb b/spec/devise_saml_authenticatable/strategy_spec.rb index ec4ed98..fbc6356 100644 --- a/spec/devise_saml_authenticatable/strategy_spec.rb +++ b/spec/devise_saml_authenticatable/strategy_spec.rb @@ -23,8 +23,10 @@ end let(:params) { {} } + let(:session) { {} } before do allow(strategy).to receive(:params).and_return(params) + allow_any_instance_of(ActionDispatch::Request).to receive(:session).and_return(session) end context "with a login SAMLResponse parameter" do @@ -37,10 +39,25 @@ it "authenticates with the response" do expect(OneLogin::RubySaml::Response).to receive(:new).with(params[:SAMLResponse], anything) expect(user_class).to receive(:authenticate_with_saml).with(response, nil) - expect(user).to receive(:after_saml_authentication).with(response.sessionindex) expect(strategy).to receive(:success!).with(user) strategy.authenticate! + expect(session).to eq(Devise.saml_session_index_key => response.sessionindex) + end + + context "when saml_session_index_key is not configured" do + before do + Devise.saml_session_index_key = nil + end + + it "authenticates with the response" do + expect(OneLogin::RubySaml::Response).to receive(:new).with(params[:SAMLResponse], anything) + expect(user_class).to receive(:authenticate_with_saml).with(response, nil) + + expect(strategy).to receive(:success!).with(user) + strategy.authenticate! + expect(session).to eq({}) + end end context "and a RelayState parameter" do @@ -95,10 +112,10 @@ def self.settings(idp_entity_id, request) expect(OneLogin::RubySaml::Response).to receive(:new).with(params[:SAMLResponse], anything) expect(idp_providers_adapter).to receive(:settings).with(idp_entity_id, anything) expect(user_class).to receive(:authenticate_with_saml).with(response, params[:RelayState]) - expect(user).to receive(:after_saml_authentication).with(response.sessionindex) expect(strategy).to receive(:success!).with(user) strategy.authenticate! + expect(session).to eq(Devise.saml_session_index_key => response.sessionindex) end end @@ -173,7 +190,6 @@ def handle(response, strategy) before do allow(Devise).to receive(:saml_validate_in_response_to).and_return(true) - allow_any_instance_of(ActionDispatch::Request).to receive(:session).and_return(session) end context "when the session has a saml_transaction_id" do @@ -193,8 +209,6 @@ def handle(response, strategy) end context "when the session is missing a saml_transaction_id" do - let(:session) { { } } - it "uses 'ID_MISSING' for matches_request_id so validation will fail" do expect(OneLogin::RubySaml::Response).to receive(:new).with(params[:SAMLResponse], hash_including(matches_request_id: "ID_MISSING")) strategy.authenticate! diff --git a/spec/support/saml_idp_controller.rb.erb b/spec/support/saml_idp_controller.rb.erb index ffde364..5aa8b55 100644 --- a/spec/support/saml_idp_controller.rb.erb +++ b/spec/support/saml_idp_controller.rb.erb @@ -103,10 +103,7 @@ class SamlIdpController < StubSamlIdp::IdpController def sp_sign_out idp_slo_authenticate(params[:name_id]) saml_slo_request = encode_SAML_SLO_Request("you@example.com") - uri = URI.parse("http://localhost:8020/users/saml/idp_sign_out") - require 'net/http' - Net::HTTP.post_form(uri, {"SAMLRequest" => saml_slo_request}) - head :no_content + redirect_to "http://localhost:8020/users/saml/idp_sign_out?SAMLRequest=#{URI.encode_www_form_component(saml_slo_request)}" end def idp_slo_authenticate(email)