Skip to content

Remove saml_internal_post feature flag #8180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduth merged 1 commit into from
Apr 12, 2023
Merged

Remove saml_internal_post feature flag #8180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 12 additions & 17 deletions app/controllers/application_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,23 +425,18 @@ def sp_session
# Conditionally sets the final_auth_request service provider session attribute
# when applicable (the original SP request is SAML)
def sp_session_request_url_with_updated_params
# Temporarily place SAML route update behind a feature flag
if IdentityConfig.store.saml_internal_post
return unless sp_session[:request_url].present?
request_url = URI(sp_session[:request_url])
url = if request_url.path.match?('saml')
sp_session[:final_auth_request] = true
complete_saml_url
else
# Login.gov redirects to the orginal request_url after a user authenticates
# replace prompt=login with prompt=select_account to prevent sign_out
# which should only ever occur once when the user
# lands on Login.gov with prompt=login
sp_session[:request_url]&.gsub('prompt=login', 'prompt=select_account')
end
else
url = sp_session[:request_url]&.gsub('prompt=login', 'prompt=select_account')
end
return unless sp_session[:request_url].present?
request_url = URI(sp_session[:request_url])
url = if request_url.path.match?('saml')
sp_session[:final_auth_request] = true
complete_saml_url
else
# Login.gov redirects to the orginal request_url after a user authenticates
# replace prompt=login with prompt=select_account to prevent sign_out
# which should only ever occur once when the user
# lands on Login.gov with prompt=login
sp_session[:request_url]&.gsub('prompt=login', 'prompt=select_account')
end

# If the user has changed the locale, we should preserve that as well
if url && locale_url_param && UriService.params(url)[:locale] != locale_url_param
Expand Down
8 changes: 2 additions & 6 deletions app/controllers/concerns/saml_idp_auth_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,8 @@ module SamlIdpAuthConcern
def sign_out_if_forceauthn_is_true_and_user_is_signed_in
return unless user_signed_in? && saml_request.force_authn?

if IdentityConfig.store.saml_internal_post
sign_out unless sp_session[:final_auth_request]
sp_session[:final_auth_request] = false
else
sign_out unless sp_session[:request_url] == request.original_url
end
sign_out unless sp_session[:final_auth_request]
sp_session[:final_auth_request] = false
end

def check_sp_active
Expand Down
2 changes: 0 additions & 2 deletions config/application.yml.default
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,6 @@ rules_of_use_horizon_years: 6
rules_of_use_updated_at: '2022-01-19T00:00:00Z' # Production has a newer timestamp than this, update directly in S3
s3_public_reports_enabled: false
s3_reports_enabled: false
saml_internal_post: true
saml_secret_rotation_enabled: false
seed_agreements_data: true
service_provider_request_ttl_hours: 24
Expand Down Expand Up @@ -570,7 +569,6 @@ test:
s3_report_bucket_prefix: ''
s3_report_public_bucket_prefix: ''
saml_endpoint_configs: '[{"suffix":"2023","secret_key_passphrase":"trust-but-verify"}]'
saml_internal_post: true
scrypt_cost: 800$8$1$
secret_key_base: test_secret_key_base
session_encryption_key: 27bad3c25711099429c1afdfd1890910f3b59f5a4faec1c85e945cb8b02b02f261ba501d99cfbb4fab394e0102de6fecf8ffe260f322f610db3e96b2a775c120
Expand Down
1 change: 0 additions & 1 deletion lib/identity_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,6 @@ def self.build_store(config_map)
config.add(:secret_key_base, type: :string)
config.add(:seed_agreements_data, type: :boolean)
config.add(:service_provider_request_ttl_hours, type: :integer)
config.add(:saml_internal_post, type: :boolean)
config.add(:session_check_delay, type: :integer)
config.add(:session_check_frequency, type: :integer)
config.add(:session_encryption_key, type: :string)
Expand Down
32 changes: 0 additions & 32 deletions spec/controllers/application_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,38 +436,6 @@ def index
expect(url_with_updated_params).to eq('/authorize?locale=es')
end
end

context 'with saml_internal_post feature flag set to false' do
before { allow(IdentityConfig.store).to receive(:saml_internal_post).and_return false }
context 'with a SAML request' do
let(:sp_session_request_url) { '/api/saml/auth2023?SAMLRequest=blah' }
it 'returns the original request url' do
expect(url_with_updated_params).to eq '/api/saml/auth2023?SAMLRequest=blah'
end
end

context 'with an OIDC request' do
let(:sp_session_request_url) { '/openid_connect/authorize' }
it 'returns the original request' do
expect(url_with_updated_params).to eq '/openid_connect/authorize'
end
end

context 'with a url that has prompt=login' do
let(:sp_session_request_url) { '/authorize?prompt=login' }
it 'changes it to prompt=select_account' do
expect(url_with_updated_params).to eq('/authorize?prompt=select_account')
end
end

context 'when the locale has been changed' do
before { I18n.locale = :es }
let(:sp_session_request_url) { '/authorize' }
it 'adds the locale to the url' do
expect(url_with_updated_params).to eq('/authorize?locale=es')
end
end
end
end

def expect_user_event_to_have_been_created(user, event_type)
Expand Down
19 changes: 1 addition & 18 deletions spec/controllers/saml_idp_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -873,7 +873,7 @@ def name_id_version(format_urn)
end
end

context 'saml_internal_post feature configuration is set to true with ForceAuthn' do
context 'with ForceAuthn' do
let(:user) { create(:user, :signed_up) }

it 'signs user out if a session is active and sp_session[:final_auth_request] is falsey' do
Expand Down Expand Up @@ -902,23 +902,6 @@ def name_id_version(format_urn)
end
end

context 'saml_internal_post feature configuration is set to false' do
let(:user) { create(:user, :signed_up) }

before { allow(IdentityConfig.store).to receive(:saml_internal_post).and_return(false) }

context 'ForceAuthn set to true' do
it 'signs the user out if a session is active' do
sign_in(user)
generate_saml_response(user, saml_settings(overrides: { force_authn: true }))
# would be 200 if the user's session persists
expect(response.status).to eq(302)
# implicit test of request storage since request_id would be missing otherwise
expect(response.location).to match(%r{#{root_url}\?request_id=.+})
end
end
end

context 'service provider is inactive' do
it 'responds with an error page' do
user = create(:user, :signed_up)
Expand Down