Merge pull request #255 from codez/multiple-user-sessions

Store IDP session index in session to allow multiple sessions per user

Author: adamstegman

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
 

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
 

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

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

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

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('[email protected]', 'sessionindex')
+          controller.current_user = Struct.new(:email).new('[email protected]')
+          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

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!

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("[email protected]")
-    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)