Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/controllers/openid_connect/logout_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 5 additions & 0 deletions app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down Expand Up @@ -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? &&
Expand Down
1 change: 1 addition & 0 deletions config/initializers/rack_attack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions lib/custom_devise_failure_app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +12 to +14
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this works is we assume that a concurrent session logout throw will be handled by Devise's failure app responder. By default, this would redirect to the new_user_session_url. We're intercepting it to redirect them to the same place they originally intended, with the expectation that the user would be fully logged out at that point. It's not ideal that this incurs an additional redirect hop, but this gives the best assurance that Warden has a nil user object, since our concurrent session logout behavior only occurs after Warden has already loaded and assigned the user who's being signed out.


private

def build_message(message)
Expand Down
8 changes: 8 additions & 0 deletions spec/controllers/openid_connect/logout_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
6 changes: 6 additions & 0 deletions spec/controllers/saml_idp_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
49 changes: 49 additions & 0 deletions spec/features/openid_connect/openid_connect_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
RSpec.describe 'OpenID Connect' do
include IdvHelper
include OidcAuthHelper
include SamlAuthHelper
include DocAuthHelper

it 'sets the sp_issuer cookie' do
Expand Down Expand Up @@ -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
Expand Down
35 changes: 35 additions & 0 deletions spec/features/saml/saml_logout_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
31 changes: 31 additions & 0 deletions spec/lib/custom_devise_failure_app_spec.rb
Original file line number Diff line number Diff line change
@@ -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
9 changes: 9 additions & 0 deletions spec/requests/rack_attack_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down