From 582dfb2710b44eed77be56689cc091ed1626552a Mon Sep 17 00:00:00 2001 From: Oren Kanner Date: Wed, 23 Feb 2022 14:42:23 -0500 Subject: [PATCH] Correctly invalidate session for SAML Remote Logout **Why:** the way this currently worked led to the user's session being cleared on subsequent visits, which meant that any authentication request information was lost. This correctly deletes the session from the Redis cache. changelog: Bug Fixes, Authentication, Fixed issue with SAML remote logout causing subsequent authentication requests not to redirect correctly --- .../concerns/saml_idp_logout_concern.rb | 5 +- app/services/out_of_band_session_accessor.rb | 46 +++++++++++++++++++ app/services/pii/session_store.rb | 33 +++---------- spec/controllers/saml_idp_controller_spec.rb | 9 +++- .../out_of_band_session_accessor_spec.rb | 39 ++++++++++++++++ 5 files changed, 102 insertions(+), 30 deletions(-) create mode 100644 app/services/out_of_band_session_accessor.rb create mode 100644 spec/services/out_of_band_session_accessor_spec.rb diff --git a/app/controllers/concerns/saml_idp_logout_concern.rb b/app/controllers/concerns/saml_idp_logout_concern.rb index df33eeb5c40..eef271be8ad 100644 --- a/app/controllers/concerns/saml_idp_logout_concern.rb +++ b/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' diff --git a/app/services/out_of_band_session_accessor.rb b/app/services/out_of_band_session_accessor.rb new file mode 100644 index 00000000000..fc735706d08 --- /dev/null +++ b/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 diff --git a/app/services/pii/session_store.rb b/app/services/pii/session_store.rb index b6afefc0864..8c11883c2cd 100644 --- a/app/services/pii/session_store.rb +++ b/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 diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 458a9b2424b..d5c819cda73 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/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 diff --git a/spec/services/out_of_band_session_accessor_spec.rb b/spec/services/out_of_band_session_accessor_spec.rb new file mode 100644 index 00000000000..0948f8291fe --- /dev/null +++ b/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