diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index fd549921441..a488159319d 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/app/controllers/openid_connect/logout_controller.rb @@ -5,6 +5,7 @@ class LogoutController < ApplicationController include SecureHeadersConcern include FullyAuthenticatable + before_action :set_devise_failure_redirect_for_concurrent_session_logout, only: [:index] before_action :confirm_two_factor_authenticated, only: [:delete] def index @@ -39,6 +40,10 @@ def delete private + def set_devise_failure_redirect_for_concurrent_session_logout + request.env['devise_session_limited_failure_redirect_url'] = request.url + end + def redirect_user(redirect_uri, user_uuid) redirect_method = IdentityConfig.store.openid_connect_redirect_uuid_override_map.fetch( user_uuid, diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index a9dc846e012..2387631005b 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -17,6 +17,7 @@ class SamlIdpController < ApplicationController skip_before_action :verify_authenticity_token before_action :require_path_year + before_action :set_devise_failure_redirect_for_concurrent_session_logout, only: :logout before_action :handle_banned_user before_action :bump_auth_count, only: :auth before_action :redirect_to_sign_in, only: :auth, unless: :user_signed_in? @@ -105,6 +106,10 @@ def prompt_for_password_if_ial2_request_and_pii_locked redirect_to capture_password_url end + def set_devise_failure_redirect_for_concurrent_session_logout + request.env['devise_session_limited_failure_redirect_url'] = request.url + end + def pii_requested_but_locked? if (sp_session && sp_session_ial > 1) || ial_context.ialmax_requested? current_user.identity_verified? && diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 4385cf76121..9c4ac5e881c 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -256,6 +256,7 @@ def headers 'rack.attack', ) do |_name, _start, _finish, _request_id, payload| req = payload[:request] + next if req.env['rack.attack.match_type'] == :safelist user = req.env['warden'].user || AnonymousUser.new sp = req.env.fetch('rack.session', {}).dig('sp', 'issuer') analytics = Analytics.new(user: user, request: req, session: {}, sp: sp) diff --git a/lib/custom_devise_failure_app.rb b/lib/custom_devise_failure_app.rb index 53a7c270a3b..4b725d7f175 100644 --- a/lib/custom_devise_failure_app.rb +++ b/lib/custom_devise_failure_app.rb @@ -9,6 +9,10 @@ def i18n_message(default = nil) message.is_a?(Symbol) ? build_message(message) : message.to_s end + def redirect_url + request.env["devise_#{warden_message}_failure_redirect_url"] || super + end + private def build_message(message) diff --git a/spec/controllers/openid_connect/logout_controller_spec.rb b/spec/controllers/openid_connect/logout_controller_spec.rb index a6cb848065c..f76e45e7ebb 100644 --- a/spec/controllers/openid_connect/logout_controller_spec.rb +++ b/spec/controllers/openid_connect/logout_controller_spec.rb @@ -33,6 +33,14 @@ ).id_token end + describe '#index' do + it 'assigns devise session limited failure redirect url' do + get :index + + expect(request.env['devise_session_limited_failure_redirect_url']).to eq(request.url) + end + end + context 'when accepting id_token_hint and client_id' do before do allow(IdentityConfig.store).to receive(:reject_id_token_hint_in_logout). diff --git a/spec/controllers/saml_idp_controller_spec.rb b/spec/controllers/saml_idp_controller_spec.rb index 2a29333ab46..9a5c3a8595e 100644 --- a/spec/controllers/saml_idp_controller_spec.rb +++ b/spec/controllers/saml_idp_controller_spec.rb @@ -8,6 +8,12 @@ let(:path_year) { SamlAuthHelper::PATH_YEAR } describe '/api/saml/logout' do + it 'assigns devise session limited failure redirect url' do + delete :logout, params: { path_year: path_year } + + expect(request.env['devise_session_limited_failure_redirect_url']).to eq(request.url) + end + it 'tracks the event when idp initiated' do stub_analytics stub_attempts_tracker diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index ec1e52df2e3..b7b56b2e59e 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -4,6 +4,7 @@ RSpec.describe 'OpenID Connect' do include IdvHelper include OidcAuthHelper + include SamlAuthHelper include DocAuthHelper it 'sets the sp_issuer cookie' do @@ -928,10 +929,58 @@ id_token_hint: id_token, ) + expect(oidc_redirect_url).to eq( + UriService.add_params('gov.gsa.openidconnect.test://result/signout', state:), + ) + visit account_path expect(page).to_not have_content(t('headings.account.login_info')) expect(page).to have_content(t('headings.sign_in_without_sp')) end + + context 'when signed out' do + it 'redirects back to the client app' do + visit openid_connect_logout_path( + client_id: 'urn:gov:gsa:openidconnect:test', + post_logout_redirect_uri: 'gov.gsa.openidconnect.test://result/signout', + ) + + expect(oidc_redirect_url).to eq('gov.gsa.openidconnect.test://result/signout') + end + end + + context 'when signed in with another browser' do + it 'redirects back to the client app after concurrent session logout' do + user = user_with_2fa + service_provider = ServiceProvider.find_by(issuer: OidcAuthHelper::OIDC_IAL1_ISSUER) + IdentityLinker.new(user, service_provider).link_identity( + verified_attributes: %w[openid email], + ) + + perform_in_browser(:one) do + visit_idp_from_sp_with_ial1(:oidc) + sign_in_live_with_2fa(user) + end + + perform_in_browser(:two) do + visit_idp_from_sp_with_ial1(:oidc) + sign_in_live_with_2fa(user) + end + + perform_in_browser(:one) do + visit openid_connect_logout_path( + client_id: OidcAuthHelper::OIDC_IAL1_ISSUER, + post_logout_redirect_uri: 'http://localhost:7654/auth/result', + ) + + expect(oidc_redirect_url).to eq('http://localhost:7654/auth/result') + + visit account_path + expect(page).to_not have_content(t('headings.account.login_info')) + expect(page).to have_content(t('headings.sign_in_without_sp')) + end + end + end end context 'canceling sign in with active identities present' do diff --git a/spec/features/saml/saml_logout_spec.rb b/spec/features/saml/saml_logout_spec.rb index ff655ff321e..a0d483596a1 100644 --- a/spec/features/saml/saml_logout_spec.rb +++ b/spec/features/saml/saml_logout_spec.rb @@ -106,6 +106,41 @@ end end + context 'when signed in with another browser' do + it 'redirects to the SP after concurrent session logout' do + user = user_with_2fa + service_provider = ServiceProvider.find_by(issuer: SamlAuthHelper::SP_ISSUER) + IdentityLinker.new(user, service_provider).link_identity( + verified_attributes: %w[openid email], + ) + + perform_in_browser(:one) do + visit_idp_from_sp_with_ial1(:saml) + sign_in_live_with_2fa(user) + end + + perform_in_browser(:two) do + visit_idp_from_sp_with_ial1(:saml) + sign_in_live_with_2fa(user) + end + + perform_in_browser(:one) do + visit_saml_logout_request_url( + overrides: { + issuer: SamlAuthHelper::SP_ISSUER, + }, + ) + + # It should contain a SAMLResponse + expect(page.find('#SAMLResponse', visible: false)).to be_truthy + + # The user should be signed out + visit account_path + expect(current_path).to eq root_path + end + end + end + context 'the saml request is invalid' do it 'renders an error' do sign_in_and_2fa_user(user) diff --git a/spec/lib/custom_devise_failure_app_spec.rb b/spec/lib/custom_devise_failure_app_spec.rb new file mode 100644 index 00000000000..7e37caa8660 --- /dev/null +++ b/spec/lib/custom_devise_failure_app_spec.rb @@ -0,0 +1,31 @@ +require 'rails_helper' +require 'custom_devise_failure_app' + +RSpec.describe CustomDeviseFailureApp do + subject(:failure_app) { CustomDeviseFailureApp.new } + + let(:message) { :invalid } + let(:env) { { 'warden' => OpenStruct.new(message:) } } + let(:request) { ActionDispatch::Request.new(env) } + + before do + failure_app.set_request!(request) + end + + describe '#redirect_url' do + it 'defers to to the default implementation' do + expect_any_instance_of(Devise::FailureApp).to receive(:redirect_url) + + failure_app.redirect_url + end + + context 'with custom redirect url assigned in request env' do + let(:custom_redirect_url) { '/redirect' } + let(:env) { super().merge('devise_invalid_failure_redirect_url' => custom_redirect_url) } + + it 'returns the custom redirect url' do + expect(failure_app.redirect_url).to eq(custom_redirect_url) + end + end + end +end diff --git a/spec/requests/rack_attack_spec.rb b/spec/requests/rack_attack_spec.rb index 075633b2af2..83800fa8419 100644 --- a/spec/requests/rack_attack_spec.rb +++ b/spec/requests/rack_attack_spec.rb @@ -14,6 +14,15 @@ expect(request.env['rack.attack.throttle_data']).to be_nil end + + it 'does not log an event' do + analytics = FakeAnalytics.new + allow(Analytics).to receive(:new).and_return(analytics) + + get '/' + + expect(analytics).not_to have_logged_event('Rate Limit Triggered') + end end describe 'high requests per ip' do