Correctly invalidate session for SAML Remote Logout

app/controllers/concerns/saml_idp_logout_concern.rb

@@ -22,8 +22,9 @@ def handle_valid_sp_logout_request
   end
 
   def handle_valid_sp_remote_logout_request(user_id)
-    # Remotely invalidate the user's current session, see config/initializers/session_limitable.rb
-    User.find(user_id).update!(unique_session_id: nil)
+    # Remotely delete the user's current session
+    session_id = User.find(user_id).unique_session_id
+    OutOfBandSessionAccessor.new(session_id).destroy
 
     # rubocop:disable Rails/RenderInline
     render inline: logout_response, content_type: 'text/xml'

app/services/out_of_band_session_accessor.rb

@@ -0,0 +1,46 @@
+# Provides a wrapper for accessing the redis cache out-of-band (using only the
+# session UUID) instead of having access to the user session from Devise/Warden.
+# Should only used outside of a normal browser session (such as the OpenID
+# Connect API or remote SAML Logout).
+
+class OutOfBandSessionAccessor
+  attr_reader :session_uuid
+
+  def initialize(session_uuid)
+    @session_uuid = session_uuid
+  end
+
+  def ttl
+    uuid = session_uuid
+    session_store.instance_eval { redis.ttl(prefixed(uuid)) }
+  end
+
+  def load
+    session_store.send(:load_session_from_redis, session_uuid) || {}
+  end
+
+  def destroy
+    session_store.send(:destroy_session_from_sid, session_uuid)
+  end
+
+  # @api private
+  # Only used for convenience in tests
+  # @param [Pii::Attributes] pii
+  def put(data, expiration = 5.minutes)
+    session_data = {
+      'warden.user.user.session' => data.to_h,
+    }
+
+    session_store.
+      send(:set_session, {}, session_uuid, session_data, expire_after: expiration.to_i)
+  end
+
+  private
+
+  def session_store
+    @session_store ||= begin
+      config = Rails.application.config
+      config.session_store.new({}, config.session_options)
+    end
+  end
+end

app/services/pii/session_store.rb

@@ -5,19 +5,16 @@
 # See Pii::Cacher for accessing PII inside of a normal browser session
 module Pii
   class SessionStore
-    attr_reader :session_uuid
+    attr_reader :session_accessor
 
-    def initialize(session_uuid)
-      @session_uuid = session_uuid
-    end
+    delegate :ttl, :destroy, to: :session_accessor
 
-    def ttl
-      uuid = session_uuid
-      session_store.instance_eval { redis.ttl(prefixed(uuid)) }
+    def initialize(session_uuid)
+      @session_accessor = OutOfBandSessionAccessor.new(session_uuid)
     end
 
     def load
-      session = session_store.send(:load_session_from_redis, session_uuid) || {}
+      session = session_accessor.load
       Pii::Attributes.new_from_json(session.dig('warden.user.user.session', :decrypted_pii))
     end
 
@@ -26,26 +23,10 @@ def load
     # @param [Pii::Attributes] pii
     def put(pii, expiration = 5.minutes)
       session_data = {
-        'warden.user.user.session' => {
-          decrypted_pii: pii.to_h.to_json,
-        },
+        decrypted_pii: pii.to_h.to_json,
       }
 
-      session_store.
-        send(:set_session, {}, session_uuid, session_data, expire_after: expiration.to_i)
-    end
-
-    # @api private
-    # Only used for convenience in tests
-    def destroy
-      session_store.send(:destroy_session_from_sid, session_uuid)
-    end
-
-    private
-
-    def session_store
-      config = Rails.application.config
-      config.session_store.new({}, config.session_options)
+      session_accessor.put(session_data, expiration)
     end
   end
 end

spec/controllers/saml_idp_controller_spec.rb

@@ -133,7 +133,8 @@
     end
     let(:other_sp) { create(:service_provider, active: true, agency_id: agency.id) }
 
-    let(:user) { create(:user, :signed_up, unique_session_id: 'abc123') }
+    let(:session_id) { 'abc123' }
+    let(:user) { create(:user, :signed_up, unique_session_id: session_id) }
     let(:other_user) { create(:user, :signed_up) }
 
     let!(:identity) do
@@ -216,6 +217,9 @@
     end
 
     it 'accepts requests with correct cert and correct session index and renders logout response' do
+      REDIS_POOL.with { |namespaced| namespaced.redis.flushdb }
+      session_accessor = OutOfBandSessionAccessor.new(session_id)
+      session_accessor.put(foo: 'bar')
       saml_request = OneLogin::RubySaml::Logoutrequest.new
       encoded_saml_request = UriService.params(
         saml_request.create(right_cert_settings),
@@ -245,11 +249,12 @@
       delete :remotelogout, params: payload.to_h.merge(Signature: Base64.encode64(signature))
 
       expect(response).to be_ok
-      expect(User.find(user.id).unique_session_id).to be_nil
+      expect(session_accessor.load).to eq({})
 
       logout_response = OneLogin::RubySaml::Logoutresponse.new(response.body)
       expect(logout_response.success?).to eq(true)
       expect(logout_response.in_response_to).to eq(saml_request.uuid)
+      REDIS_POOL.with { |namespaced| namespaced.redis.flushdb }
     end
 
     it 'rejects requests from a correct cert but no session index' do

spec/services/out_of_band_session_accessor_spec.rb

@@ -0,0 +1,39 @@
+require 'rails_helper'
+
+RSpec.describe OutOfBandSessionAccessor do
+  let(:session_uuid) { SecureRandom.uuid }
+
+  subject(:store) { described_class.new(session_uuid) }
+
+  around do |ex|
+    REDIS_POOL.with { |namespaced| namespaced.redis.flushdb }
+    ex.run
+    REDIS_POOL.with { |namespaced| namespaced.redis.flushdb }
+  end
+
+  describe '#ttl' do
+    it 'returns the remaining time-to-live of the session data in redis' do
+      store.put({ foo: 'bar' }, 5.minutes.to_i)
+
+      expect(store.ttl).to eq(5.minutes.to_i)
+    end
+  end
+
+  describe '#load' do
+    it 'loads the session' do
+      store.put({ foo: 'bar' }, 5.minutes.to_i)
+
+      session = store.load
+      expect(session.dig('warden.user.user.session')).to eq('foo' => 'bar')
+    end
+  end
+
+  describe '#destroy' do
+    it 'destroys the session' do
+      store.put({ foo: 'bar' }, 5.minutes.to_i)
+      store.destroy
+
+      expect(store.load).to eq({})
+    end
+  end
+end