From 4a8f2ef2eacf3cdd0a431b577f8b6477b0878034 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 2 Nov 2023 17:02:16 -0500 Subject: [PATCH 01/27] Add support for client-side OIDC redirect changelog: Internal, OpenID Connect, Add support for client-side OIDC redirect --- .../concerns/secure_headers_concern.rb | 1 + .../authorization_controller.rb | 25 ++++++++++++++++--- .../packs/openid-connect-redirect.js | 4 +++ .../authorization/redirect.html.erb | 25 +++++++++++++++++++ config/application.yml.default | 2 ++ lib/identity_config.rb | 1 + tsconfig.json | 3 ++- 7 files changed, 56 insertions(+), 5 deletions(-) create mode 100644 app/javascript/packs/openid-connect-redirect.js create mode 100644 app/views/openid_connect/authorization/redirect.html.erb diff --git a/app/controllers/concerns/secure_headers_concern.rb b/app/controllers/concerns/secure_headers_concern.rb index d7e7e3496b1..9f56526b647 100644 --- a/app/controllers/concerns/secure_headers_concern.rb +++ b/app/controllers/concerns/secure_headers_concern.rb @@ -3,6 +3,7 @@ module SecureHeadersConcern def apply_secure_headers_override return if stored_url_for_user.blank? + return if IdentityConfig.store.openid_connect_redirect_interstitial_enabled authorize_form = OpenidConnectAuthorizeForm.new(authorize_params) return unless authorize_form.valid? diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index de08e231166..366315801f5 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -73,7 +73,16 @@ def ial_context def handle_successful_handoff track_events SpHandoffBounce::AddHandoffTimeToSession.call(sp_session) - redirect_to @authorize_form.success_redirect_uri, allow_other_host: true + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + @oidc_redirect_uri = @authorize_form.success_redirect_uri + render( + 'redirect', + layout: false, + ) + else + redirect_to @authorize_form.success_redirect_uri, allow_other_host: true + end + delete_branded_experience end @@ -97,6 +106,7 @@ def build_authorize_form_from_params end def secure_headers_override + return if IdentityConfig.store.openid_connect_redirect_interstitial_enabled csp_uris = SecureHeadersAllowList.csp_with_sp_redirect_uris( @authorize_form.redirect_uri, @authorize_form.service_provider.redirect_uris, @@ -117,11 +127,18 @@ def pre_validate_authorize_form ), ) return if result.success? + redirect_uri = result.extra[:redirect_uri] - if (redirect_uri = result.extra[:redirect_uri]) - redirect_to redirect_uri, allow_other_host: true - else + if redirect_uri.nil? render :error + elsif IdentityConfig.store.openid_connect_redirect_interstitial_enabled + @oidc_redirect_uri = redirect_uri + render( + 'redirect', + layout: false, + ) + else + redirect_to redirect_uri, allow_other_host: true end end diff --git a/app/javascript/packs/openid-connect-redirect.js b/app/javascript/packs/openid-connect-redirect.js new file mode 100644 index 00000000000..4c343fa5855 --- /dev/null +++ b/app/javascript/packs/openid-connect-redirect.js @@ -0,0 +1,4 @@ +document.addEventListener('DOMContentLoaded', function () { + document.body.className += ' usa-sr-only'; + document.getElementById('openid-connect-redirect').click(); +}); diff --git a/app/views/openid_connect/authorization/redirect.html.erb b/app/views/openid_connect/authorization/redirect.html.erb new file mode 100644 index 00000000000..a77abb192a4 --- /dev/null +++ b/app/views/openid_connect/authorization/redirect.html.erb @@ -0,0 +1,25 @@ + + + + + <%= t('saml_idp.shared.saml_post_binding.redirecting') %> | <%= APP_NAME %> + <%= stylesheet_link_tag 'application', media: 'all' %> + <%= render_stylesheet_once_tags %> + <%= render_javascript_pack_once_tags 'openid-connect-redirect' %> + + +
+
+
+ <%= render PageHeadingComponent.new.with_content(t('saml_idp.shared.saml_post_binding.heading')) %> + +

+ <%= t('saml_idp.shared.saml_post_binding.no_js') %> +

+ + <%= link_to(t('forms.buttons.continue'), @oidc_redirect_uri, class: 'usa-button usa-button--wide usa-button--big', id: 'openid-connect-redirect') %> +
+
+
+ + diff --git a/config/application.yml.default b/config/application.yml.default index 00d196751b0..8ee8cf70ef7 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -211,6 +211,7 @@ multi_region_kms_migration_jobs_profile_timeout: 120 multi_region_kms_migration_jobs_user_count: 1000 multi_region_kms_migration_jobs_user_timeout: 120 mx_timeout: 3 +openid_connect_redirect_interstitial_enabled: true otp_delivery_blocklist_maxretry: 10 otp_valid_for: 10 otp_expiration_warning_seconds: 150 @@ -543,6 +544,7 @@ test: otp_min_attempts_remaining_warning_count: 1 newrelic_license_key: '' nonessential_email_banlist: '["banned_email@gmail.com"]' + openid_connect_redirect_interstitial_enabled: false otp_delivery_blocklist_findtime: 1 otp_delivery_blocklist_maxretry: 2 otps_per_ip_limit: 3 diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 31a7ed683c3..33987a86bf4 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -322,6 +322,7 @@ def self.build_store(config_map) config.add(:mx_timeout, type: :integer) config.add(:newrelic_license_key, type: :string) config.add(:nonessential_email_banlist, type: :json) + config.add(:openid_connect_redirect_interstitial_enabled, type: :boolean) config.add(:otp_delivery_blocklist_findtime, type: :integer) config.add(:otp_delivery_blocklist_maxretry, type: :integer) config.add(:otp_expiration_warning_seconds, type: :integer) diff --git a/tsconfig.json b/tsconfig.json index 1c56906d480..dd5532d663d 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -29,6 +29,7 @@ "**/fixtures", "**/*.spec.js", "app/javascript/packs/pw-strength.js", - "app/javascript/packs/saml-post.js" + "app/javascript/packs/saml-post.js", + "app/javascript/packs/openid-connect-redirect.js" ] } From b2df8f5664ac40e4d1ce22af4719c9cc7a8fe9c7 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Tue, 28 Nov 2023 16:18:57 -0600 Subject: [PATCH 02/27] meta refresh --- app/javascript/packs/openid-connect-redirect.js | 4 ---- .../authorization/redirect.html.erb | 15 +-------------- tsconfig.json | 3 +-- 3 files changed, 2 insertions(+), 20 deletions(-) delete mode 100644 app/javascript/packs/openid-connect-redirect.js diff --git a/app/javascript/packs/openid-connect-redirect.js b/app/javascript/packs/openid-connect-redirect.js deleted file mode 100644 index 4c343fa5855..00000000000 --- a/app/javascript/packs/openid-connect-redirect.js +++ /dev/null @@ -1,4 +0,0 @@ -document.addEventListener('DOMContentLoaded', function () { - document.body.className += ' usa-sr-only'; - document.getElementById('openid-connect-redirect').click(); -}); diff --git a/app/views/openid_connect/authorization/redirect.html.erb b/app/views/openid_connect/authorization/redirect.html.erb index a77abb192a4..8736adcbf20 100644 --- a/app/views/openid_connect/authorization/redirect.html.erb +++ b/app/views/openid_connect/authorization/redirect.html.erb @@ -5,21 +5,8 @@ <%= t('saml_idp.shared.saml_post_binding.redirecting') %> | <%= APP_NAME %> <%= stylesheet_link_tag 'application', media: 'all' %> <%= render_stylesheet_once_tags %> - <%= render_javascript_pack_once_tags 'openid-connect-redirect' %> + -
-
-
- <%= render PageHeadingComponent.new.with_content(t('saml_idp.shared.saml_post_binding.heading')) %> - -

- <%= t('saml_idp.shared.saml_post_binding.no_js') %> -

- - <%= link_to(t('forms.buttons.continue'), @oidc_redirect_uri, class: 'usa-button usa-button--wide usa-button--big', id: 'openid-connect-redirect') %> -
-
-
diff --git a/tsconfig.json b/tsconfig.json index dd5532d663d..1c56906d480 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -29,7 +29,6 @@ "**/fixtures", "**/*.spec.js", "app/javascript/packs/pw-strength.js", - "app/javascript/packs/saml-post.js", - "app/javascript/packs/openid-connect-redirect.js" + "app/javascript/packs/saml-post.js" ] } From 3f475e71d4ab3075c0c195634e2fa838746586bf Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 09:16:33 -0600 Subject: [PATCH 03/27] logout --- .../openid_connect/logout_controller.rb | 28 +++++++++++++------ .../openid_connect/logout/redirect.html.erb | 12 ++++++++ 2 files changed, 32 insertions(+), 8 deletions(-) create mode 100644 app/views/openid_connect/logout/redirect.html.erb diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index c438f3420a2..c77c3509cd0 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/app/controllers/openid_connect/logout_controller.rb @@ -29,10 +29,15 @@ def delete irs_attempts_api_tracker.logout_initiated(success: result.success?) sign_out - redirect_to( - redirect_uri, - allow_other_host: true, - ) + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + @oidc_redirect_uri = redirect_uri + render :redirect + else + redirect_to( + redirect_uri, + allow_other_host: true, + ) + end else render :error end @@ -42,6 +47,7 @@ def delete def apply_logout_secure_headers_override(redirect_uri, service_provider) return if service_provider.nil? || redirect_uri.nil? + return if IdentityConfig.store.openid_connect_redirect_interstitial_enabled uris = SecureHeadersAllowList.csp_with_sp_redirect_uris( redirect_uri, @@ -82,10 +88,16 @@ def handle_successful_logout_request(result, redirect_uri) irs_attempts_api_tracker.logout_initiated(success: result.success?) sign_out - redirect_to( - redirect_uri, - allow_other_host: true, - ) + + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + @oidc_redirect_uri = redirect_uri + render :redirect + else + redirect_to( + redirect_uri, + allow_other_host: true, + ) + end end end diff --git a/app/views/openid_connect/logout/redirect.html.erb b/app/views/openid_connect/logout/redirect.html.erb new file mode 100644 index 00000000000..8736adcbf20 --- /dev/null +++ b/app/views/openid_connect/logout/redirect.html.erb @@ -0,0 +1,12 @@ + + + + + <%= t('saml_idp.shared.saml_post_binding.redirecting') %> | <%= APP_NAME %> + <%= stylesheet_link_tag 'application', media: 'all' %> + <%= render_stylesheet_once_tags %> + + + + + From f07941f9225448d00ed90c96ea10164019debfeb Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 09:17:05 -0600 Subject: [PATCH 04/27] split interstitial and CSP config --- app/controllers/concerns/secure_headers_concern.rb | 2 +- app/controllers/openid_connect/authorization_controller.rb | 2 +- app/controllers/openid_connect/logout_controller.rb | 2 +- config/application.yml.default | 4 ++++ lib/identity_config.rb | 1 + 5 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/controllers/concerns/secure_headers_concern.rb b/app/controllers/concerns/secure_headers_concern.rb index 9f56526b647..e0aecab4451 100644 --- a/app/controllers/concerns/secure_headers_concern.rb +++ b/app/controllers/concerns/secure_headers_concern.rb @@ -3,7 +3,7 @@ module SecureHeadersConcern def apply_secure_headers_override return if stored_url_for_user.blank? - return if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + return unless IdentityConfig.store.openid_connect_content_security_form_action_enabled authorize_form = OpenidConnectAuthorizeForm.new(authorize_params) return unless authorize_form.valid? diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 366315801f5..7f1acf87a1e 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -106,7 +106,7 @@ def build_authorize_form_from_params end def secure_headers_override - return if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + return unless IdentityConfig.store.openid_connect_content_security_form_action_enabled csp_uris = SecureHeadersAllowList.csp_with_sp_redirect_uris( @authorize_form.redirect_uri, @authorize_form.service_provider.redirect_uris, diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index c77c3509cd0..69cf195896d 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/app/controllers/openid_connect/logout_controller.rb @@ -47,7 +47,7 @@ def delete def apply_logout_secure_headers_override(redirect_uri, service_provider) return if service_provider.nil? || redirect_uri.nil? - return if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + return unless IdentityConfig.store.openid_connect_content_security_form_action_enabled uris = SecureHeadersAllowList.csp_with_sp_redirect_uris( redirect_uri, diff --git a/config/application.yml.default b/config/application.yml.default index 8ee8cf70ef7..3a44ebddebb 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -212,6 +212,7 @@ multi_region_kms_migration_jobs_user_count: 1000 multi_region_kms_migration_jobs_user_timeout: 120 mx_timeout: 3 openid_connect_redirect_interstitial_enabled: true +openid_connect_content_security_form_action_enabled: false otp_delivery_blocklist_maxretry: 10 otp_valid_for: 10 otp_expiration_warning_seconds: 150 @@ -471,6 +472,8 @@ production: logins_per_ip_track_only_mode: true newrelic_license_key: '' nonessential_email_banlist: '[]' + openid_connect_redirect_interstitial_enabled: false + openid_connect_content_security_form_action_enabled: true otp_delivery_blocklist_findtime: 5 participate_in_dap: true password_pepper: @@ -545,6 +548,7 @@ test: newrelic_license_key: '' nonessential_email_banlist: '["banned_email@gmail.com"]' openid_connect_redirect_interstitial_enabled: false + openid_connect_content_security_form_action_enabled: true otp_delivery_blocklist_findtime: 1 otp_delivery_blocklist_maxretry: 2 otps_per_ip_limit: 3 diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 33987a86bf4..87f6f4f9b93 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -323,6 +323,7 @@ def self.build_store(config_map) config.add(:newrelic_license_key, type: :string) config.add(:nonessential_email_banlist, type: :json) config.add(:openid_connect_redirect_interstitial_enabled, type: :boolean) + config.add(:openid_connect_content_security_form_action_enabled, type: :boolean) config.add(:otp_delivery_blocklist_findtime, type: :integer) config.add(:otp_delivery_blocklist_maxretry, type: :integer) config.add(:otp_expiration_warning_seconds, type: :integer) From 1b4a41d71e1bb7cf667a87af236ce47fe3088cad Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 12:10:13 -0600 Subject: [PATCH 05/27] trying some testing --- .../openid_connect/logout_controller.rb | 10 ++- .../authorization_controller_spec.rb | 88 ++++++++++++++++--- .../openid_connect/openid_connect_spec.rb | 38 +++++--- .../redirect_uri_validation_spec.rb | 7 +- spec/support/oidc_auth_helper.rb | 19 ++++ 5 files changed, 132 insertions(+), 30 deletions(-) diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index 69cf195896d..a8cea2e1536 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/app/controllers/openid_connect/logout_controller.rb @@ -31,7 +31,10 @@ def delete sign_out if IdentityConfig.store.openid_connect_redirect_interstitial_enabled @oidc_redirect_uri = redirect_uri - render :redirect + render( + :redirect, + layout: false, + ) else redirect_to( redirect_uri, @@ -91,7 +94,10 @@ def handle_successful_logout_request(result, redirect_uri) if IdentityConfig.store.openid_connect_redirect_interstitial_enabled @oidc_redirect_uri = redirect_uri - render :redirect + render( + :redirect, + layout: false, + ) else redirect_to( redirect_uri, diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 25e49e3a381..84c20383bd2 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -52,9 +52,18 @@ user.identities.last.update!(verified_attributes: %w[given_name family_name birthdate]) action - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + else + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end - redirect_params = UriService.params(response.location) + redirect_params = if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + UriService.params(assigns(:oidc_redirect_uri)) + else + UriService.params(response.location) + end expect(redirect_params[:code]).to be_present expect(redirect_params[:state]).to eq(params[:state]) @@ -116,7 +125,12 @@ allow(controller).to receive(:pii_requested_but_locked?).and_return(false) action - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + else + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end end it 'redirects to the password capture url when pii is locked' do @@ -280,7 +294,12 @@ allow(controller).to receive(:pii_requested_but_locked?).and_return(false) action - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + else + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end end it 'redirects to the password capture url when pii is locked' do @@ -343,7 +362,13 @@ ) action - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + else + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end end it 'tracks IAL1 authentication event' do @@ -396,7 +421,12 @@ ) action - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + else + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end end it 'tracks IAL1 authentication event' do @@ -463,9 +493,18 @@ it 'redirects back to the client app with a code' do action - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + else + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end - redirect_params = UriService.params(response.location) + redirect_params = if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + UriService.params(assigns(:oidc_redirect_uri)) + else + UriService.params(response.location) + end expect(redirect_params[:code]).to be_present expect(redirect_params[:state]).to eq(params[:state]) @@ -479,9 +518,18 @@ it 'redirects to the redirect_uri immediately with an invalid_request' do action - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + else + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end - redirect_params = UriService.params(response.location) + redirect_params = if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + UriService.params(assigns(:oidc_redirect_uri)) + else + UriService.params(response.location) + end expect(redirect_params[:error]).to eq('invalid_request') expect(redirect_params[:error_description]).to be_present @@ -554,7 +602,12 @@ it 'handles the error and does not blow up' do action - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + else + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end end end @@ -575,9 +628,18 @@ it 'redirect the user' do action - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + else + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end - redirect_params = UriService.params(response.location) + redirect_params = if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + UriService.params(assigns(:oidc_redirect_uri)) + else + UriService.params(response.location) + end expect(redirect_params[:error]).to eq('invalid_request') expect(redirect_params[:error_description]).to be_present diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index ee196f1b695..0c67a13d3c7 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -65,7 +65,7 @@ it 'succeeds in returning back to sp with prompt select_account and prior session' do user = oidc_end_client_secret_jwt(prompt: 'select_account') oidc_end_client_secret_jwt(prompt: 'select_account', user: user, redirs_to: '/auth/result') - expect(current_url).to include('?code=') + expect(oidc_redirect_url).to include('?code=') end it 'succeeds with no prompt and no prior session like select_account' do @@ -75,7 +75,7 @@ it 'succeeds in returning back to sp with no prompt and prior session like select_account' do user = oidc_end_client_secret_jwt oidc_end_client_secret_jwt(user: user, redirs_to: '/auth/result') - expect(current_url).to include('?code=') + expect(oidc_redirect_url).to include('?code=') end it 'succeeds with prompt login and no prior session' do @@ -111,15 +111,17 @@ it 'returns invalid request with bad prompt parameter' do oidc_end_client_secret_jwt(prompt: 'aaa', redirs_to: '/auth/result') - expect(current_url).to include('?error=invalid_request') + expect(oidc_redirect_url).to include('?error=invalid_request') end it 'returns invalid request with a blank prompt parameter' do oidc_end_client_secret_jwt(prompt: '', redirs_to: '/auth/result') - expect(current_url).to include('?error=invalid_request') + expect(oidc_redirect_url).to include('?error=invalid_request') end it 'auto-allows with a second authorization and includes redirect_uris in CSP headers' do + allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). + and_return(true) client_id = 'urn:gov:gsa:openidconnect:sp:server' service_provider = build(:service_provider, issuer: client_id) user = user_with_2fa @@ -141,6 +143,9 @@ end it 'auto-allows and includes redirect_uris in CSP headers after an incorrect OTP' do + allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). + and_return(true) + client_id = 'urn:gov:gsa:openidconnect:sp:server' service_provider = build(:service_provider, issuer: client_id) user = user_with_2fa @@ -280,6 +285,9 @@ context 'when sending client_id' do it 'logout destroys the session when confirming logout' do + allow(IdentityConfig.store).to( + receive(:openid_connect_content_security_form_action_enabled).and_return(true), + ) service_provider = ServiceProvider.find_by(issuer: 'urn:gov:gsa:openidconnect:test') sign_in_get_id_token(client_id: service_provider.issuer) @@ -527,7 +535,7 @@ verified_within: '1w', ) - redirect_params = UriService.params(current_url) + redirect_params = UriService.params(oidc_redirect_url) expect(redirect_params[:error]).to eq('invalid_request') expect(redirect_params[:error_description]). @@ -655,7 +663,7 @@ _user = sign_in_live_with_2fa(user) expect(page.html).to_not include(code_challenge) - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) redirect_params = Rack::Utils.parse_query(redirect_uri.query).with_indifferent_access expect(redirect_uri.to_s).to start_with('gov.gsa.openidconnect.test://result') @@ -721,7 +729,7 @@ sign_in_live_with_2fa(user) continue_as(user.email) - redirect_uri2 = URI(current_url) + redirect_uri2 = URI(oidc_redirect_url) expect(redirect_uri2.to_s).to start_with('gov.gsa.openidconnect.test://result') redirect_params2 = Rack::Utils.parse_query(redirect_uri2.query).with_indifferent_access @@ -793,7 +801,7 @@ click_agree_and_continue continue_as(email) - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('gov.gsa.openidconnect.test://result') expect(page.get_rack_session.keys).to include('sp') end @@ -831,12 +839,12 @@ ) click_agree_and_continue - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') visit sign_out_url visit oidc_path sign_in_live_with_2fa(user) - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -983,7 +991,11 @@ def sign_in_get_token_response( proofing_steps&.call handoff_page_steps&.call - redirect_uri = URI(current_url) + redirect_uri = if current_path.start_with?('/openid_connect/') + URI(oidc_redirect_url) + else + URI(current_url) + end redirect_params = Rack::Utils.parse_query(redirect_uri.query).with_indifferent_access code = redirect_params[:code] expect(code).to be_present @@ -1026,7 +1038,7 @@ def oidc_end_client_secret_jwt(prompt: nil, user: nil, redirs_to: nil) visit_idp_from_ial2_oidc_sp(prompt: prompt, state: state, nonce: nonce, client_id: client_id) continue_as(user.email) if user if redirs_to - expect(current_path).to eq(redirs_to) + expect(URI(oidc_redirect_url).path).to eq(redirs_to) return end expect(current_path).to eq('/') @@ -1038,7 +1050,7 @@ def oidc_end_client_secret_jwt(prompt: nil, user: nil, redirs_to: nil) sign_in_live_with_2fa(user) click_agree_and_continue_optional - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) redirect_params = Rack::Utils.parse_query(redirect_uri.query).with_indifferent_access expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') diff --git a/spec/features/openid_connect/redirect_uri_validation_spec.rb b/spec/features/openid_connect/redirect_uri_validation_spec.rb index 660c4d41217..9815881d9d1 100644 --- a/spec/features/openid_connect/redirect_uri_validation_spec.rb +++ b/spec/features/openid_connect/redirect_uri_validation_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' RSpec.describe 'redirect_uri validation' do + include OidcAuthHelper + context 'when the redirect_uri in the request does not match one that is registered' do it 'displays error instead of branded landing page' do visit_idp_from_sp_with_ial1_with_disallowed_redirect_uri @@ -148,8 +150,9 @@ click_submit_default click_agree_and_continue - redirect_host = URI.parse(current_url).host - redirect_scheme = URI.parse(current_url).scheme + redirect_uri = URI.parse(oidc_redirect_url) + redirect_host = redirect_uri.host + redirect_scheme = redirect_uri.scheme expect(redirect_host).to eq('example.com') expect(redirect_scheme).to eq('https') diff --git a/spec/support/oidc_auth_helper.rb b/spec/support/oidc_auth_helper.rb index 36181e7812c..2f9fce8446a 100644 --- a/spec/support/oidc_auth_helper.rb +++ b/spec/support/oidc_auth_helper.rb @@ -103,4 +103,23 @@ def include_aal3(params) params[:acr_values] = "#{params[:acr_values]} " + Saml::Idp::Constants::AAL3_AUTHN_CONTEXT_CLASSREF end + + # We rely on client-side redirects in some cases using: + # + # This method checks that the url contains the right url + def extract_meta_refresh_url + content = page.find("meta[http-equiv='refresh']", visible: false)['content'] + timeout, url_value = content.split(';') + expect(timeout).to eq '0' + _, url = url_value.split('url=') + url + end + + def oidc_redirect_url + if IdentityConfig.store.openid_connect_redirect_interstitial_enabled + extract_meta_refresh_url + else + current_url + end + end end From 111c6ba1f26140d2911c94bcf9aa31a78081dbac Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 12:25:16 -0600 Subject: [PATCH 06/27] enable interstitial in tests --- config/application.yml.default | 2 -- 1 file changed, 2 deletions(-) diff --git a/config/application.yml.default b/config/application.yml.default index 3a44ebddebb..55bbb2f81b5 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -547,8 +547,6 @@ test: otp_min_attempts_remaining_warning_count: 1 newrelic_license_key: '' nonessential_email_banlist: '["banned_email@gmail.com"]' - openid_connect_redirect_interstitial_enabled: false - openid_connect_content_security_form_action_enabled: true otp_delivery_blocklist_findtime: 1 otp_delivery_blocklist_maxretry: 2 otps_per_ip_limit: 3 From b1d9cfd9ce284786cb42e4cab6e05581ac69aa5a Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 16:40:31 -0600 Subject: [PATCH 07/27] fix spec --- .../account_creation/multiple_browsers_spec.rb | 1 + .../remember_device/session_expiration_spec.rb | 6 ++---- spec/support/shared_examples/account_creation.rb | 14 +++++++------- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/spec/features/account_creation/multiple_browsers_spec.rb b/spec/features/account_creation/multiple_browsers_spec.rb index 7995c447d0f..73080c84394 100644 --- a/spec/features/account_creation/multiple_browsers_spec.rb +++ b/spec/features/account_creation/multiple_browsers_spec.rb @@ -3,6 +3,7 @@ RSpec.feature 'account creation across multiple browsers' do include SpAuthHelper include SamlAuthHelper + include OidcAuthHelper it_behaves_like 'creating two accounts during the same session', :oidc it_behaves_like 'creating two accounts during the same session', :saml diff --git a/spec/features/remember_device/session_expiration_spec.rb b/spec/features/remember_device/session_expiration_spec.rb index 1fa3c1b6551..1fea732eb4d 100644 --- a/spec/features/remember_device/session_expiration_spec.rb +++ b/spec/features/remember_device/session_expiration_spec.rb @@ -2,6 +2,7 @@ RSpec.describe 'signing in with remember device and idling on the sign in page' do include SamlAuthHelper + include OidcAuthHelper it 'redirects to the OIDC SP even though session is deleted' do allow(IdentityConfig.store).to receive(:otp_delivery_blocklist_maxretry).and_return(1000) @@ -37,14 +38,11 @@ # Simulate refreshing the page with JS to avoid a CSRF error visit new_user_session_url(request_id: request_id) - expect(page.response_headers['Content-Security-Policy']). - to(include('form-action \'self\' http://localhost:7654')) - fill_in_credentials_and_submit(user.email, user.password) continue_as(user.email, user.password) - expect(current_url).to start_with('http://localhost:7654') + expect(oidc_redirect_url).to start_with('http://localhost:7654') end end end diff --git a/spec/support/shared_examples/account_creation.rb b/spec/support/shared_examples/account_creation.rb index 5ad098ed863..67795cfcbfe 100644 --- a/spec/support/shared_examples/account_creation.rb +++ b/spec/support/shared_examples/account_creation.rb @@ -13,7 +13,7 @@ if :sp == :saml expect(current_url).to eq UriService.add_params(@saml_authn_request, locale: :es) elsif sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -34,7 +34,7 @@ expect(current_url).to eq complete_saml_url if sp == :saml if sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -59,7 +59,7 @@ expect(current_path).to eq test_saml_decode_assertion_path if sp == :saml if sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -80,7 +80,7 @@ expect(current_url).to eq complete_saml_url if sp == :saml if sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -110,7 +110,7 @@ expect(current_path).to eq test_saml_decode_assertion_path if sp == :saml if sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -135,7 +135,7 @@ expect(current_url).to eq complete_saml_url if sp == :saml if sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -156,7 +156,7 @@ expect(current_url).to eq complete_saml_url if sp == :saml if sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end From 7f8e10add29419e89753b94e88ff395445ceed3d Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 16:47:10 -0600 Subject: [PATCH 08/27] add case for form-action config being disabled --- spec/requests/csp_spec.rb | 108 ++++++++++++++++++++++++++++---------- 1 file changed, 80 insertions(+), 28 deletions(-) diff --git a/spec/requests/csp_spec.rb b/spec/requests/csp_spec.rb index 5cfd4a927e9..476717e53d1 100644 --- a/spec/requests/csp_spec.rb +++ b/spec/requests/csp_spec.rb @@ -2,40 +2,92 @@ RSpec.describe 'content security policy' do context 'on endpoints that will redirect to an SP' do - it 'includes a CSP with a form action that will allow a redirect to the CSP' do - visit_password_form_with_sp - follow_redirect! + context 'when openid_connect_content_security_form_action_enabled is enabled' do + before do + allow(IdentityConfig.store).to( + receive(:openid_connect_content_security_form_action_enabled), + ).and_return(true) + end - content_security_policy = parse_content_security_policy + it 'includes a CSP with a form action that will allow a redirect to the CSP' do + visit_password_form_with_sp + follow_redirect! - expect(content_security_policy['default-src']).to eq("'self'") - expect(content_security_policy['base-uri']).to eq("'self'") - expect(content_security_policy['child-src']).to eq("'self'") - expect(content_security_policy['connect-src']).to eq("'self'") - expect(content_security_policy['font-src']).to eq("'self' data:") - expect(content_security_policy['form-action']).to eq( - "'self' http://localhost:7654 https://example.com http://www.example.com", - ) - expect(content_security_policy['img-src']).to eq( - "'self' data: login.gov https://s3.us-west-2.amazonaws.com", - ) - expect(content_security_policy['media-src']).to eq("'self'") - expect(content_security_policy['object-src']).to eq("'none'") - expect(content_security_policy['script-src']).to match( - /'self' 'unsafe-eval' 'nonce-[\w\d=\/+]+'/, - ) - expect(content_security_policy['style-src']).to eq("'self'") + content_security_policy = parse_content_security_policy + + expect(content_security_policy['default-src']).to eq("'self'") + expect(content_security_policy['base-uri']).to eq("'self'") + expect(content_security_policy['child-src']).to eq("'self'") + expect(content_security_policy['connect-src']).to eq("'self'") + expect(content_security_policy['font-src']).to eq("'self' data:") + expect(content_security_policy['form-action']).to eq( + "'self' http://localhost:7654 https://example.com http://www.example.com", + ) + expect(content_security_policy['img-src']).to eq( + "'self' data: login.gov https://s3.us-west-2.amazonaws.com", + ) + expect(content_security_policy['media-src']).to eq("'self'") + expect(content_security_policy['object-src']).to eq("'none'") + expect(content_security_policy['script-src']).to match( + /'self' 'unsafe-eval' 'nonce-[\w\d=\/+]+'/, + ) + expect(content_security_policy['style-src']).to eq("'self'") + end + + it 'uses logout SP to override CSP form action that will allow a redirect to the CSP' do + visit_password_form_with_sp + visit_logout_form_with_sp + + content_security_policy = parse_content_security_policy + + expect(content_security_policy['form-action']).to eq( + "'self' gov.gsa.openidconnect.test:", + ) + end end - it 'uses logout SP to override CSP form action that will allow a redirect to the CSP' do - visit_password_form_with_sp - visit_logout_form_with_sp + context 'when openid_connect_content_security_form_action_enabled is disabled' do + before do + allow(IdentityConfig.store).to( + receive(:openid_connect_content_security_form_action_enabled), + ).and_return(false) + end - content_security_policy = parse_content_security_policy + it 'includes a CSP without SP hosts in form-action' do + visit_password_form_with_sp + follow_redirect! - expect(content_security_policy['form-action']).to eq( - "'self' gov.gsa.openidconnect.test:", - ) + content_security_policy = parse_content_security_policy + + expect(content_security_policy['default-src']).to eq("'self'") + expect(content_security_policy['base-uri']).to eq("'self'") + expect(content_security_policy['child-src']).to eq("'self'") + expect(content_security_policy['connect-src']).to eq("'self'") + expect(content_security_policy['font-src']).to eq("'self' data:") + expect(content_security_policy['form-action']).to eq( + "'self'", + ) + expect(content_security_policy['img-src']).to eq( + "'self' data: login.gov https://s3.us-west-2.amazonaws.com", + ) + expect(content_security_policy['media-src']).to eq("'self'") + expect(content_security_policy['object-src']).to eq("'none'") + expect(content_security_policy['script-src']).to match( + /'self' 'unsafe-eval' 'nonce-[\w\d=\/+]+'/, + ) + expect(content_security_policy['style-src']).to eq("'self'") + end + + it 'uses logout SP to override CSP form action that will allow a redirect to the CSP' do + visit_password_form_with_sp + visit_logout_form_with_sp + + content_security_policy = parse_content_security_policy + + expect(content_security_policy['form-action']).to eq( + "'self'", + ) + end end end From 49eb81029c5ec7e6cbfad3c153fb87ce22bd07b0 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 16:53:14 -0600 Subject: [PATCH 09/27] fix more specs --- spec/features/users/sign_in_spec.rb | 1 + spec/support/shared_examples/sign_in.rb | 19 +++++++------------ 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 86280070532..059f983eae9 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -5,6 +5,7 @@ include ActionView::Helpers::DateHelper include PersonalKeyHelper include SamlAuthHelper + include OidcAuthHelper include SpAuthHelper include IdvHelper include DocAuthHelper diff --git a/spec/support/shared_examples/sign_in.rb b/spec/support/shared_examples/sign_in.rb index 4dfddbf1964..2fde5dbe2a3 100644 --- a/spec/support/shared_examples/sign_in.rb +++ b/spec/support/shared_examples/sign_in.rb @@ -7,11 +7,6 @@ fill_in_credentials_and_submit(user.email, user.password) continue_as(user.email) - if sp == :oidc - expect(page.response_headers['Content-Security-Policy']). - to(include('form-action \'self\' http://localhost:7654')) - end - fill_in_code_with_last_phone_otp sp == :saml ? click_submit_default_twice : click_submit_default @@ -22,7 +17,7 @@ if sp == :saml expect(current_url).to eq UriService.add_params(complete_saml_url, locale: 'es') elsif sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -60,7 +55,7 @@ expect(current_url).to eq complete_saml_url if sp == :saml if sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -120,7 +115,7 @@ expect(current_url).to eq complete_saml_url if sp == :saml if sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -156,7 +151,7 @@ expect(current_url).to eq complete_saml_url if sp == :saml if sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -311,7 +306,7 @@ def ial1_sign_in_with_personal_key_goes_to_sp(sp) return unless sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -326,7 +321,7 @@ def ial1_sign_in_with_piv_cac_goes_to_sp(sp) click_submit_default if sp == :saml click_agree_and_continue return unless sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -352,7 +347,7 @@ def ial2_sign_in_with_piv_cac_goes_to_sp(sp) expect(current_url).to include(@saml_authn_request) end elsif sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end From b2fc00a01343fdeed5f12a004da2917f34474f70 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 16:58:19 -0600 Subject: [PATCH 10/27] fix more specs --- spec/support/shared_examples/remember_device.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/support/shared_examples/remember_device.rb b/spec/support/shared_examples/remember_device.rb index 6a4948935a8..79719d93e92 100644 --- a/spec/support/shared_examples/remember_device.rb +++ b/spec/support/shared_examples/remember_device.rb @@ -1,4 +1,6 @@ RSpec.shared_examples 'remember device' do + include OidcAuthHelper + it 'does not require 2FA on sign in' do user = remember_device_and_sign_out_user sign_in_user(user) @@ -56,12 +58,9 @@ visit oidc_url - expect(page.response_headers['Content-Security-Policy']). - to(include('form-action \'self\' http://localhost:7654')) - sign_in_user(user) click_continue - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') end def expect_mfa_to_be_required_for_user(user) From 99a5a2dfb7182b63a2fe8e84c1f32c50695bd95e Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 17:01:53 -0600 Subject: [PATCH 11/27] fix more specs --- spec/features/reports/sp_active_users_report_spec.rb | 5 +++-- spec/features/visitors/password_recovery_spec.rb | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/spec/features/reports/sp_active_users_report_spec.rb b/spec/features/reports/sp_active_users_report_spec.rb index f5586511427..caae13a6856 100644 --- a/spec/features/reports/sp_active_users_report_spec.rb +++ b/spec/features/reports/sp_active_users_report_spec.rb @@ -2,6 +2,7 @@ RSpec.feature 'sp active users report' do include SamlAuthHelper + include OidcAuthHelper include IdvHelper it 'reports a user as ial1 active for an ial1 sign in' do @@ -11,7 +12,7 @@ fill_in_code_with_last_phone_otp click_submit_default click_agree_and_continue - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') results = [{ issuer: OidcAuthHelper::OIDC_IAL1_ISSUER, app_id: nil, @@ -30,7 +31,7 @@ fill_in_code_with_last_phone_otp click_submit_default click_agree_and_continue - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') results = [{ issuer: 'urn:gov:gsa:openidconnect:sp:server', app_id: nil, diff --git a/spec/features/visitors/password_recovery_spec.rb b/spec/features/visitors/password_recovery_spec.rb index ccda0376641..80ce19fb844 100644 --- a/spec/features/visitors/password_recovery_spec.rb +++ b/spec/features/visitors/password_recovery_spec.rb @@ -4,6 +4,7 @@ include IdvHelper include PersonalKeyHelper include SamlAuthHelper + include OidcAuthHelper context 'user enters valid email in forgot password form', email: true do it 'redirects to forgot_password path and sends an email to the user' do @@ -203,7 +204,7 @@ click_submit_default click_agree_and_continue - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') end end From ffb828372fc17759f737143174b670a12ae3f930 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 17:08:36 -0600 Subject: [PATCH 12/27] fix more specs --- spec/features/sign_in/banned_users_spec.rb | 2 +- .../features/users/password_recovery_via_recovery_code_spec.rb | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/features/sign_in/banned_users_spec.rb b/spec/features/sign_in/banned_users_spec.rb index 4e333e815ea..aecb624ae3a 100644 --- a/spec/features/sign_in/banned_users_spec.rb +++ b/spec/features/sign_in/banned_users_spec.rb @@ -38,7 +38,7 @@ visit_idp_from_sp_with_ial1(:oidc) sign_in_live_with_2fa(user) click_agree_and_continue - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') end end diff --git a/spec/features/users/password_recovery_via_recovery_code_spec.rb b/spec/features/users/password_recovery_via_recovery_code_spec.rb index 76c78856de4..f4ac1cf8fa8 100644 --- a/spec/features/users/password_recovery_via_recovery_code_spec.rb +++ b/spec/features/users/password_recovery_via_recovery_code_spec.rb @@ -4,6 +4,7 @@ include PersonalKeyHelper include IdvStepHelper include SamlAuthHelper + include OidcAuthHelper include SpAuthHelper let(:user) { create(:user, :fully_registered) } @@ -107,7 +108,7 @@ click_agree_and_continue - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') end end From d4b32b617adfbf073190fe3e9d14a81aaae572d1 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 17:18:14 -0600 Subject: [PATCH 13/27] add test for both cases --- .../openid_connect/openid_connect_spec.rb | 62 +++++++++++++++++-- 1 file changed, 58 insertions(+), 4 deletions(-) diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index 0c67a13d3c7..ec1e52df2e3 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -119,7 +119,7 @@ expect(oidc_redirect_url).to include('?error=invalid_request') end - it 'auto-allows with a second authorization and includes redirect_uris in CSP headers' do + it 'auto-allows with a second authorization and redirect_uris in CSP headers if enabled' do allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). and_return(true) client_id = 'urn:gov:gsa:openidconnect:sp:server' @@ -138,11 +138,34 @@ fill_in_code_with_last_phone_otp click_submit_default - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') + expect(page.get_rack_session.keys).to include('sp') + end + + it 'auto-allows with a second authorization and blank CSP headers if not enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). + and_return(false) + client_id = 'urn:gov:gsa:openidconnect:sp:server' + service_provider = build(:service_provider, issuer: client_id) + user = user_with_2fa + + IdentityLinker.new(user, service_provider).link_identity + user.identities.last.update!(verified_attributes: ['email']) + + visit_idp_from_ial1_oidc_sp(client_id: client_id, prompt: 'select_account') + sign_in_user(user) + + expect(page.response_headers['Content-Security-Policy']). + to(include('form-action \'self\'')) + + fill_in_code_with_last_phone_otp + click_submit_default + + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') expect(page.get_rack_session.keys).to include('sp') end - it 'auto-allows and includes redirect_uris in CSP headers after an incorrect OTP' do + it 'auto-allows and includes redirect_uris in CSP headers if enabled after an incorrect OTP' do allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). and_return(true) @@ -169,7 +192,38 @@ fill_in_code_with_last_phone_otp click_submit_default - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') + expect(page.get_rack_session.keys).to include('sp') + end + + it 'auto-allows and blank CSP headers if disabled after an incorrect OTP' do + allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). + and_return(true) + + client_id = 'urn:gov:gsa:openidconnect:sp:server' + service_provider = build(:service_provider, issuer: client_id) + user = user_with_2fa + + IdentityLinker.new(user, service_provider).link_identity + user.identities.last.update!(verified_attributes: ['email']) + + visit_idp_from_ial1_oidc_sp(client_id: client_id, prompt: 'select_account') + sign_in_user(user) + + expect(page.response_headers['Content-Security-Policy']). + to(include('form-action \'self\'')) + + fill_in :code, with: 'wrong otp' + click_submit_default + + expect(page).to have_content(t('two_factor_authentication.invalid_otp')) + expect(page.response_headers['Content-Security-Policy']). + to(include('form-action \'self\'')) + + fill_in_code_with_last_phone_otp + click_submit_default + + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') expect(page.get_rack_session.keys).to include('sp') end From 59c7bdd5b5cbc5a23c47e7f47e7002c782d77e81 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 17:23:56 -0600 Subject: [PATCH 14/27] fix more specs --- spec/features/users/sign_in_spec.rb | 16 ++++++++-------- spec/support/shared_examples/sign_in.rb | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/features/users/sign_in_spec.rb b/spec/features/users/sign_in_spec.rb index 059f983eae9..461a8303e3c 100644 --- a/spec/features/users/sign_in_spec.rb +++ b/spec/features/users/sign_in_spec.rb @@ -95,7 +95,7 @@ expect(current_url).to eq rules_of_use_url accept_rules_of_use_and_continue_if_displayed - expect(current_url).to start_with service_provider.redirect_uris.first + expect(oidc_redirect_url).to start_with service_provider.redirect_uris.first end scenario 'user with old terms of use can accept and continue to IAL2 SP' do @@ -129,7 +129,7 @@ expect(current_url).to eq rules_of_use_url accept_rules_of_use_and_continue_if_displayed - expect(current_url).to start_with service_provider.redirect_uris.first + expect(oidc_redirect_url).to start_with service_provider.redirect_uris.first end scenario 'user opts to add piv/cac card but gets an error' do @@ -191,7 +191,7 @@ click_submit_default skip_second_mfa_prompt click_agree_and_continue - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') end scenario 'user cannot sign in with certificate none error' do @@ -722,7 +722,7 @@ click_submit_default click_agree_and_continue - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -785,7 +785,7 @@ click_agree_and_continue - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') end it 'returns ial2 info for a verified user' do @@ -803,7 +803,7 @@ click_agree_and_continue - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') end end @@ -812,7 +812,7 @@ create(:user, :fully_registered) visit_idp_from_oidc_sp_with_ialmax - expect(page).to have_content 'The page you were looking for doesn’t exist' + expect(oidc_redirect_url).to include('error=invalid_request') end end end @@ -956,7 +956,7 @@ action_url = agree_and_continue_button.ancestor('form')[:action] agree_and_continue_button.click - expect(current_url).to start_with('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to start_with('http://localhost:7654/auth/result') response = page.driver.post(action_url) expect(response).to be_redirect diff --git a/spec/support/shared_examples/sign_in.rb b/spec/support/shared_examples/sign_in.rb index 2fde5dbe2a3..5030d2c2def 100644 --- a/spec/support/shared_examples/sign_in.rb +++ b/spec/support/shared_examples/sign_in.rb @@ -347,7 +347,7 @@ def ial2_sign_in_with_piv_cac_goes_to_sp(sp) expect(current_url).to include(@saml_authn_request) end elsif sp == :oidc - redirect_uri = URI(oidc_redirect_url) + redirect_uri = URI(current_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end From 9ab26d0317120c6f7b978893bfb7d371a325ffde Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Thu, 30 Nov 2023 17:31:42 -0600 Subject: [PATCH 15/27] fix more specs --- spec/features/multiple_emails/sp_sign_in_spec.rb | 3 ++- .../openid_connect/authorization_confirmation_spec.rb | 8 ++++---- spec/support/shared_examples/account_creation.rb | 6 +++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/spec/features/multiple_emails/sp_sign_in_spec.rb b/spec/features/multiple_emails/sp_sign_in_spec.rb index 5179fe24c74..dbae13eb376 100644 --- a/spec/features/multiple_emails/sp_sign_in_spec.rb +++ b/spec/features/multiple_emails/sp_sign_in_spec.rb @@ -2,6 +2,7 @@ RSpec.feature 'signing into an SP with multiple emails enabled' do include SamlAuthHelper + include OidcAuthHelper context 'with the email scope' do scenario 'signing in with OIDC sends the email address used to sign in' do @@ -109,7 +110,7 @@ def visit_idp_from_oidc_sp(scope:) end def fetch_oidc_id_token_info - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) redirect_params = Rack::Utils.parse_query(redirect_uri.query).with_indifferent_access code = redirect_params[:code] diff --git a/spec/features/openid_connect/authorization_confirmation_spec.rb b/spec/features/openid_connect/authorization_confirmation_spec.rb index 86e3ec3123f..25bafb60992 100644 --- a/spec/features/openid_connect/authorization_confirmation_spec.rb +++ b/spec/features/openid_connect/authorization_confirmation_spec.rb @@ -39,7 +39,7 @@ def create_user_and_remember_device expect(page).to have_content second_email.email continue_as(second_email.email) - expect(current_url).to match('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to match('http://localhost:7654/auth/result') end it 'it allows the user to switch accounts prior to continuing to the SP' do @@ -53,7 +53,7 @@ def create_user_and_remember_device fill_in_code_with_last_phone_otp click_submit_default - expect(current_url).to match('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to match('http://localhost:7654/auth/result') end it 'does not render the confirmation screen on a return visit to the SP by default' do @@ -66,7 +66,7 @@ def create_user_and_remember_device # second visit visit_idp_from_ial1_oidc_sp - expect(current_url).to match('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to match('http://localhost:7654/auth/result') end it 'does not render an error if a user goes back after opting to switch accounts' do @@ -100,7 +100,7 @@ def create_user_and_remember_device click_agree_and_continue - expect(current_url).to match('http://localhost:7654/auth/result') + expect(oidc_redirect_url).to match('http://localhost:7654/auth/result') expect(page.get_rack_session.keys).to include('sp') end end diff --git a/spec/support/shared_examples/account_creation.rb b/spec/support/shared_examples/account_creation.rb index 67795cfcbfe..76656cc07cb 100644 --- a/spec/support/shared_examples/account_creation.rb +++ b/spec/support/shared_examples/account_creation.rb @@ -13,7 +13,7 @@ if :sp == :saml expect(current_url).to eq UriService.add_params(@saml_authn_request, locale: :es) elsif sp == :oidc - redirect_uri = URI(oidc_redirect_url) + redirect_uri = URI(current_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -59,7 +59,7 @@ expect(current_path).to eq test_saml_decode_assertion_path if sp == :saml if sp == :oidc - redirect_uri = URI(oidc_redirect_url) + redirect_uri = URI(current_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -110,7 +110,7 @@ expect(current_path).to eq test_saml_decode_assertion_path if sp == :saml if sp == :oidc - redirect_uri = URI(oidc_redirect_url) + redirect_uri = URI(current_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end From 0934450ec469f6c3a8ef41acf8fc4ba72b52122c Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 09:30:34 -0600 Subject: [PATCH 16/27] fix more specs --- .../users/rules_of_use_controller_spec.rb | 29 ++++++++++++++++++- spec/support/shared_examples/sign_in.rb | 2 ++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/spec/controllers/users/rules_of_use_controller_spec.rb b/spec/controllers/users/rules_of_use_controller_spec.rb index 6769a87944f..cc4f7fed6e7 100644 --- a/spec/controllers/users/rules_of_use_controller_spec.rb +++ b/spec/controllers/users/rules_of_use_controller_spec.rb @@ -122,7 +122,10 @@ action end - it 'includes service provider URIs in form action content security policy header' do + it 'includes service provider URIs in form-action CSP header when enabled' do + allow(IdentityConfig.store).to( + receive(:openid_connect_content_security_form_action_enabled).and_return(true), + ) sp = create(:service_provider, issuer: 'example-issuer', redirect_uris: ['https://example.com']) params = { client_id: sp.issuer, @@ -142,6 +145,30 @@ csp_array = ["'self'", 'https://example.com'] expect(form_action).to match_array(csp_array) end + + it 'does not include service provider URIs in form-action CSP header when disabled' do + allow(IdentityConfig.store).to( + receive(:openid_connect_content_security_form_action_enabled).and_return(false), + ) + sp = create(:service_provider, issuer: 'example-issuer', redirect_uris: ['https://example.com']) + params = { + client_id: sp.issuer, + response_type: 'code', + acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF, + scope: 'openid email', + redirect_uri: sp.redirect_uris.first, + state: '1234567890123456789012', + nonce: '1234567890123456789012', + } + session[:sp] = { + issuer: sp.issuer, + request_url: "http://test.com?#{URI.encode_www_form(params)}", + } + action + form_action = response.request.content_security_policy.form_action + csp_array = ["'self'"] + expect(form_action).to match_array(csp_array) + end end context 'when the user needs to accept the rules of use and does not accept them' do diff --git a/spec/support/shared_examples/sign_in.rb b/spec/support/shared_examples/sign_in.rb index 5030d2c2def..fd875aae6ae 100644 --- a/spec/support/shared_examples/sign_in.rb +++ b/spec/support/shared_examples/sign_in.rb @@ -45,6 +45,8 @@ end RSpec.shared_examples 'visiting 2fa when fully authenticated' do |sp| + include OidcAuthHelper + it 'redirects to SP after visiting a 2fa screen when fully authenticated', email: true do ial1_sign_in_with_personal_key_goes_to_sp(sp) From 7a1b2645f3b57797a833a000cad8acca76693226 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 09:53:29 -0600 Subject: [PATCH 17/27] fix more specs --- spec/features/users/sign_up_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/features/users/sign_up_spec.rb b/spec/features/users/sign_up_spec.rb index 26d0b4dcb41..8ac814fe613 100644 --- a/spec/features/users/sign_up_spec.rb +++ b/spec/features/users/sign_up_spec.rb @@ -2,6 +2,7 @@ RSpec.feature 'Sign Up' do include SamlAuthHelper + include OidcAuthHelper include DocAuthHelper include ActionView::Helpers::DateHelper @@ -315,7 +316,7 @@ def clipboard_text skip_second_mfa_prompt click_agree_and_continue - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end From dbcd16baf0b6fffc03485a8a4925b3773ec5528e Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 11:00:27 -0600 Subject: [PATCH 18/27] add spec --- .../shared_examples/account_creation.rb | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/spec/support/shared_examples/account_creation.rb b/spec/support/shared_examples/account_creation.rb index 76656cc07cb..dcc460cc75f 100644 --- a/spec/support/shared_examples/account_creation.rb +++ b/spec/support/shared_examples/account_creation.rb @@ -21,7 +21,9 @@ end RSpec.shared_examples 'creating an account using authenticator app for 2FA' do |sp| - it 'redirects to the SP', email: true do + it 'redirects to the SP with SP URIs in form-action CSP if enabled', email: true do + allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). + and_return(true) visit_idp_from_sp_with_ial1(sp) register_user_with_authenticator_app @@ -39,6 +41,27 @@ expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end end + + it 'redirects to the SP without SP URIs in form-action CSP if disabled', email: true do + allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). + and_return(false) + visit_idp_from_sp_with_ial1(sp) + register_user_with_authenticator_app + + if sp == :oidc + expect(page.response_headers['Content-Security-Policy']). + to(include('form-action \'self\'')) + end + + click_agree_and_continue + expect(current_url).to eq complete_saml_url if sp == :saml + + if sp == :oidc + redirect_uri = URI(oidc_redirect_url) + + expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') + end + end end RSpec.shared_examples 'creating an IAL2 account using authenticator app for 2FA' do |sp| From ac472d9a6419bc626cc22bdff8cc1f91d2fb4afc Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 11:53:05 -0600 Subject: [PATCH 19/27] update logout specs --- .../openid_connect/logout_controller_spec.rb | 107 ++++++++++++++++-- 1 file changed, 98 insertions(+), 9 deletions(-) diff --git a/spec/controllers/openid_connect/logout_controller_spec.rb b/spec/controllers/openid_connect/logout_controller_spec.rb index 9bc34dc04e4..5f9317970c8 100644 --- a/spec/controllers/openid_connect/logout_controller_spec.rb +++ b/spec/controllers/openid_connect/logout_controller_spec.rb @@ -62,12 +62,23 @@ action end - it 'redirects back to the client' do + it 'redirects back to the client if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) end + it 'renders client-side redirect if client-side redirect is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + action + + expect(controller).to render_template('openid_connect/logout/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) + end + it 'tracks events' do stub_analytics expect(@analytics).to receive(:track_event). @@ -173,11 +184,22 @@ end context 'user is not signed in' do - it 'redirects back with an error' do + it 'renders client-side redirect if client-side redirect is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) end + + it 'redirects back to the client if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + action + + expect(controller).to render_template('openid_connect/logout/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) + end end end @@ -276,11 +298,22 @@ end context 'user is not signed in' do - it 'redirects back to client' do + it 'redirects back to the client if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) end + + it 'renders client-side redirect if client-side redirect is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + action + + expect(controller).to render_template('openid_connect/logout/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) + end end end end @@ -297,16 +330,32 @@ end context 'user is signed in' do + let(:user) { create(:user) } before { stub_sign_in(user) } - it 'destroys the session' do + + it 'destroys the session and redirects to client if client-side redirect is disabled' do + expect(controller).to receive(:sign_out) + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) end + + it 'destroys session and renders client-side redirect if enabled' do + expect(controller).to receive(:sign_out) + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + action + + expect(controller).to render_template('openid_connect/logout/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) + end end context 'user is not signed in' do - it 'destroys the session' do + it 'redirects to new session path' do + expect(controller).to_not receive(:sign_out) action expect(response).to redirect_to(new_user_session_path) @@ -327,15 +376,29 @@ context 'user is signed in' do before { stub_sign_in(user) } - it 'destroys the session' do + it 'destroys the session and redirects if client-side redirect is disabled' do + expect(controller).to receive(:sign_out) + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) end + + it 'destroys the session and renders client-side redirect if enabled' do + expect(controller).to receive(:sign_out) + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + action + + expect(controller).to render_template('openid_connect/logout/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) + end end context 'user is not signed in' do - it 'destroys the session' do + it 'redirects to new session path' do + expect(controller).to_not receive(:sign_out) action expect(response).to redirect_to(new_user_session_path) @@ -489,11 +552,24 @@ end context 'user is not signed in' do - it 'redirects back to client' do + it 'redirects back to the client if client-side redirect is disabled' do + expect(controller).to receive(:sign_out) + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) end + + it 'renders client-side redirect if client-side redirect is enabled' do + expect(controller).to receive(:sign_out) + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + action + + expect(controller).to render_template('openid_connect/logout/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) + end end end @@ -510,12 +586,25 @@ context 'user is signed in' do before { stub_sign_in(user) } - it 'destroys the session' do + it 'destroys session and redirects to client if client-side redirect is disabled' do + expect(controller).to receive(:sign_out) + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action expect(response).to redirect_to(/^#{post_logout_redirect_uri}/) end + it 'destroys the session and renders client-side redirect if enabled' do + expect(controller).to receive(:sign_out) + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + action + + expect(controller).to render_template('openid_connect/logout/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) + end + it 'tracks events' do stub_analytics From 626cc09ff749e02d0c70f8fb2e7e30e793c270bd Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 12:10:02 -0600 Subject: [PATCH 20/27] add/fix specs --- .../shared_examples/account_creation.rb | 53 +++++++++++++++++-- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/spec/support/shared_examples/account_creation.rb b/spec/support/shared_examples/account_creation.rb index dcc460cc75f..38625da6863 100644 --- a/spec/support/shared_examples/account_creation.rb +++ b/spec/support/shared_examples/account_creation.rb @@ -1,5 +1,7 @@ RSpec.shared_examples 'creating an account with the site in Spanish' do |sp| - it 'redirects to the SP', email: true do + it 'redirects to the SP with SP URIs in form-action CSP if enabled', email: true do + allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). + and_return(true) Capybara.current_session.driver.header('Accept-Language', 'es') visit_idp_from_sp_with_ial1(sp) register_user @@ -13,7 +15,29 @@ if :sp == :saml expect(current_url).to eq UriService.add_params(@saml_authn_request, locale: :es) elsif sp == :oidc - redirect_uri = URI(current_url) + redirect_uri = URI(oidc_redirect_url) + + expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') + end + end + + it 'redirects to the SP without SP URIs in form-action CSP if disabled', email: true do + allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). + and_return(false) + Capybara.current_session.driver.header('Accept-Language', 'es') + visit_idp_from_sp_with_ial1(sp) + register_user + + if sp == :oidc + expect(page.response_headers['Content-Security-Policy']). + to(include('form-action \'self\'')) + end + + click_agree_and_continue + if :sp == :saml + expect(current_url).to eq UriService.add_params(@saml_authn_request, locale: :es) + elsif sp == :oidc + redirect_uri = URI(oidc_redirect_url) expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end @@ -90,7 +114,9 @@ end RSpec.shared_examples 'creating an account using PIV/CAC for 2FA' do |sp| - it 'redirects to the SP', email: true do + it 'redirects to the SP with SP URIs in form-action CSP if enabled', email: true do + allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). + and_return(true) visit_idp_from_sp_with_ial1(sp) register_user_with_piv_cac @@ -108,6 +134,27 @@ expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') end end + + it 'redirects to the SP without SP URIs in form-action CSP if disabled', email: true do + allow(IdentityConfig.store).to receive(:openid_connect_content_security_form_action_enabled). + and_return(false) + visit_idp_from_sp_with_ial1(sp) + register_user_with_piv_cac + + if sp == :oidc + expect(page.response_headers['Content-Security-Policy']). + to(include('form-action \'self\'')) + end + + click_agree_and_continue + expect(current_url).to eq complete_saml_url if sp == :saml + + if sp == :oidc + redirect_uri = URI(oidc_redirect_url) + + expect(redirect_uri.to_s).to start_with('http://localhost:7654/auth/result') + end + end end RSpec.shared_examples 'creating an IAL2 account using webauthn for 2FA' do |sp| From 3cfa5a3794ba9a42881586dbf59deaf2b232834b Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 14:29:16 -0600 Subject: [PATCH 21/27] add oidc authorization specs --- .../authorization_controller_spec.rb | 253 ++++++++++++------ 1 file changed, 170 insertions(+), 83 deletions(-) diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index 84c20383bd2..b8092b66e75 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -1,3 +1,4 @@ +# rubocop:disable Layout/LineLength require 'rails_helper' RSpec.describe OpenidConnect::AuthorizationController do @@ -47,23 +48,32 @@ end context 'with valid params' do - it 'redirects back to the client app with a code' do + it 'redirects back to the client app with a code if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) IdentityLinker.new(user, service_provider).link_identity(ial: 1) user.identities.last.update!(verified_attributes: %w[given_name family_name birthdate]) action - if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - expect(controller).to render_template('openid_connect/authorization/redirect') - expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - else - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - end + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + + redirect_params = UriService.params(response.location) + + expect(redirect_params[:code]).to be_present + expect(redirect_params[:state]).to eq(params[:state]) + end + + it 'renders a client-side redirect back to the client app with a code if it is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + IdentityLinker.new(user, service_provider).link_identity(ial: 1) + user.identities.last.update!(verified_attributes: %w[given_name family_name birthdate]) + action + + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - redirect_params = if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - UriService.params(assigns(:oidc_redirect_uri)) - else - UriService.params(response.location) - end + redirect_params = UriService.params(assigns(:oidc_redirect_uri)) expect(redirect_params[:code]).to be_present expect(redirect_params[:state]).to eq(params[:state]) @@ -117,7 +127,9 @@ ).user end - it 'redirects to the redirect_uri immediately when pii is unlocked' do + it 'redirects to the redirect_uri immediately when pii is unlocked if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) IdentityLinker.new(user, service_provider).link_identity(ial: 3) user.identities.last.update!( verified_attributes: %w[given_name family_name birthdate verified_at], @@ -125,12 +137,21 @@ allow(controller).to receive(:pii_requested_but_locked?).and_return(false) action - if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - expect(controller).to render_template('openid_connect/authorization/redirect') - expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - else - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - end + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end + + it 'renders a client-side redirect back to the client app immediately if it is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + IdentityLinker.new(user, service_provider).link_identity(ial: 3) + user.identities.last.update!( + verified_attributes: %w[given_name family_name birthdate verified_at], + ) + allow(controller).to receive(:pii_requested_but_locked?).and_return(false) + action + + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) end it 'redirects to the password capture url when pii is locked' do @@ -286,7 +307,9 @@ ).user end - it 'redirects to the redirect_uri immediately when pii is unlocked' do + it 'redirects to the redirect_uri immediately when pii is unlocked if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) IdentityLinker.new(user, service_provider).link_identity(ial: 3) user.identities.last.update!( verified_attributes: %w[given_name family_name birthdate verified_at], @@ -294,12 +317,22 @@ allow(controller).to receive(:pii_requested_but_locked?).and_return(false) action - if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - expect(controller).to render_template('openid_connect/authorization/redirect') - expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - else - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - end + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end + + it 'renders client-side redirect to the client app immediately if PII is unlocked and it is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + + IdentityLinker.new(user, service_provider).link_identity(ial: 3) + user.identities.last.update!( + verified_attributes: %w[given_name family_name birthdate verified_at], + ) + allow(controller).to receive(:pii_requested_but_locked?).and_return(false) + action + + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) end it 'redirects to the password capture url when pii is locked' do @@ -355,7 +388,9 @@ end context 'account is not already verified' do - it 'redirects to the redirect_uri immediately without proofing' do + it 'redirects to the redirect_uri immediately without proofing if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) IdentityLinker.new(user, service_provider).link_identity(ial: 1) user.identities.last.update!( verified_attributes: %w[given_name family_name birthdate verified_at], @@ -363,12 +398,21 @@ action - if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - expect(controller).to render_template('openid_connect/authorization/redirect') - expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - else - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - end + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end + + it 'renders client-side redirect to the client app immediately if client-side redirect is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + + IdentityLinker.new(user, service_provider).link_identity(ial: 1) + user.identities.last.update!( + verified_attributes: %w[given_name family_name birthdate verified_at], + ) + + action + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) end it 'tracks IAL1 authentication event' do @@ -414,19 +458,32 @@ context 'profile is reset' do let(:user) { create(:profile, :verified, :password_reset).user } - it 'redirects to the redirect_uri immediately without proofing' do + it 'redirects to the redirect_uri immediately without proofing if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) + IdentityLinker.new(user, service_provider).link_identity(ial: 1) user.identities.last.update!( verified_attributes: %w[given_name family_name birthdate verified_at], ) action - if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - expect(controller).to render_template('openid_connect/authorization/redirect') - expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - else - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - end + + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end + + it 'renders client-side redirect to the client app immediately if client-side redirect is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + + IdentityLinker.new(user, service_provider).link_identity(ial: 1) + user.identities.last.update!( + verified_attributes: %w[given_name family_name birthdate verified_at], + ) + + action + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) end it 'tracks IAL1 authentication event' do @@ -490,46 +547,61 @@ user.identities.last.update!(verified_attributes: %w[given_name family_name birthdate]) end - it 'redirects back to the client app with a code' do + it 'redirects back to the client app with a code if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action - if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - expect(controller).to render_template('openid_connect/authorization/redirect') - expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - else - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - end + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - redirect_params = if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - UriService.params(assigns(:oidc_redirect_uri)) - else - UriService.params(response.location) - end + redirect_params = UriService.params(response.location) expect(redirect_params[:code]).to be_present expect(redirect_params[:state]).to eq(params[:state]) end + + it 'renders a client-side redirect back to the client app with a code if it is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + + action + + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + + redirect_params = UriService.params(assigns(:oidc_redirect_uri)) + expect(redirect_params[:code]).to be_present + expect(redirect_params[:state]).to eq(params[:state]) + end end end context 'with invalid params that do not interfere with the redirect_uri' do before { params[:prompt] = '' } - it 'redirects to the redirect_uri immediately with an invalid_request' do + it 'redirects the user with an invalid request if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action - if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - expect(controller).to render_template('openid_connect/authorization/redirect') - expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - else - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - end + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - redirect_params = if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - UriService.params(assigns(:oidc_redirect_uri)) - else - UriService.params(response.location) - end + redirect_params = UriService.params(response.location) + + expect(redirect_params[:error]).to eq('invalid_request') + expect(redirect_params[:error_description]).to be_present + expect(redirect_params[:state]).to eq(params[:state]) + end + + it 'renders client-side redirect with an invalid request if client-side redirect is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + action + + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) + + redirect_params = UriService.params(assigns(:oidc_redirect_uri)) expect(redirect_params[:error]).to eq('invalid_request') expect(redirect_params[:error_description]).to be_present @@ -599,15 +671,21 @@ context 'without valid acr_values' do before { params.delete(:acr_values) } - it 'handles the error and does not blow up' do + it 'handles the error and does not blow up when client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action - if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - expect(controller).to render_template('openid_connect/authorization/redirect') - expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - else - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - end + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + end + + it 'handles the error and does not blow up when client-side redirect is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + action + + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) end end @@ -625,21 +703,29 @@ params[:acr_values] = Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF end - it 'redirect the user' do + it 'redirects the user if client-side redirect is disabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(false) action - if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - expect(controller).to render_template('openid_connect/authorization/redirect') - expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - else - expect(response).to redirect_to(/^#{params[:redirect_uri]}/) - end + expect(response).to redirect_to(/^#{params[:redirect_uri]}/) + + redirect_params = UriService.params(response.location) + + expect(redirect_params[:error]).to eq('invalid_request') + expect(redirect_params[:error_description]).to be_present + expect(redirect_params[:state]).to eq(params[:state]) + end + + it 'renders a client-side redirect if client-side redirect is enabled' do + allow(IdentityConfig.store).to receive(:openid_connect_redirect_interstitial_enabled). + and_return(true) + action + + expect(controller).to render_template('openid_connect/authorization/redirect') + expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) - redirect_params = if IdentityConfig.store.openid_connect_redirect_interstitial_enabled - UriService.params(assigns(:oidc_redirect_uri)) - else - UriService.params(response.location) - end + redirect_params = UriService.params(assigns(:oidc_redirect_uri)) expect(redirect_params[:error]).to eq('invalid_request') expect(redirect_params[:error_description]).to be_present @@ -691,3 +777,4 @@ end end end +# rubocop:enable Layout/LineLength From 6756f833deb56f3f1faec6869786da13697a2434 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 15:40:25 -0600 Subject: [PATCH 22/27] move redirect template to shared location --- .../openid_connect/authorization_controller.rb | 4 ++-- .../openid_connect/logout_controller.rb | 4 ++-- .../openid_connect/logout/redirect.html.erb | 12 ------------ .../redirect.html.erb | 0 .../authorization_controller_spec.rb | 18 +++++++++--------- .../openid_connect/logout_controller_spec.rb | 14 +++++++------- 6 files changed, 20 insertions(+), 32 deletions(-) delete mode 100644 app/views/openid_connect/logout/redirect.html.erb rename app/views/openid_connect/{authorization => shared}/redirect.html.erb (100%) diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 7f1acf87a1e..14bca296cec 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -76,7 +76,7 @@ def handle_successful_handoff if IdentityConfig.store.openid_connect_redirect_interstitial_enabled @oidc_redirect_uri = @authorize_form.success_redirect_uri render( - 'redirect', + 'openid_connect/shared/redirect', layout: false, ) else @@ -134,7 +134,7 @@ def pre_validate_authorize_form elsif IdentityConfig.store.openid_connect_redirect_interstitial_enabled @oidc_redirect_uri = redirect_uri render( - 'redirect', + 'openid_connect/shared/redirect', layout: false, ) else diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index a8cea2e1536..2e123dee8ec 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/app/controllers/openid_connect/logout_controller.rb @@ -32,7 +32,7 @@ def delete if IdentityConfig.store.openid_connect_redirect_interstitial_enabled @oidc_redirect_uri = redirect_uri render( - :redirect, + 'openid_connect/shared/redirect', layout: false, ) else @@ -95,7 +95,7 @@ def handle_successful_logout_request(result, redirect_uri) if IdentityConfig.store.openid_connect_redirect_interstitial_enabled @oidc_redirect_uri = redirect_uri render( - :redirect, + 'openid_connect/shared/redirect', layout: false, ) else diff --git a/app/views/openid_connect/logout/redirect.html.erb b/app/views/openid_connect/logout/redirect.html.erb deleted file mode 100644 index 8736adcbf20..00000000000 --- a/app/views/openid_connect/logout/redirect.html.erb +++ /dev/null @@ -1,12 +0,0 @@ - - - - - <%= t('saml_idp.shared.saml_post_binding.redirecting') %> | <%= APP_NAME %> - <%= stylesheet_link_tag 'application', media: 'all' %> - <%= render_stylesheet_once_tags %> - - - - - diff --git a/app/views/openid_connect/authorization/redirect.html.erb b/app/views/openid_connect/shared/redirect.html.erb similarity index 100% rename from app/views/openid_connect/authorization/redirect.html.erb rename to app/views/openid_connect/shared/redirect.html.erb diff --git a/spec/controllers/openid_connect/authorization_controller_spec.rb b/spec/controllers/openid_connect/authorization_controller_spec.rb index b8092b66e75..0d4634cbeb5 100644 --- a/spec/controllers/openid_connect/authorization_controller_spec.rb +++ b/spec/controllers/openid_connect/authorization_controller_spec.rb @@ -70,7 +70,7 @@ user.identities.last.update!(verified_attributes: %w[given_name family_name birthdate]) action - expect(controller).to render_template('openid_connect/authorization/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) redirect_params = UriService.params(assigns(:oidc_redirect_uri)) @@ -150,7 +150,7 @@ allow(controller).to receive(:pii_requested_but_locked?).and_return(false) action - expect(controller).to render_template('openid_connect/authorization/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) end @@ -331,7 +331,7 @@ allow(controller).to receive(:pii_requested_but_locked?).and_return(false) action - expect(controller).to render_template('openid_connect/authorization/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) end @@ -411,7 +411,7 @@ ) action - expect(controller).to render_template('openid_connect/authorization/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) end @@ -482,7 +482,7 @@ ) action - expect(controller).to render_template('openid_connect/authorization/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) end @@ -566,7 +566,7 @@ action - expect(controller).to render_template('openid_connect/authorization/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) redirect_params = UriService.params(assigns(:oidc_redirect_uri)) @@ -598,7 +598,7 @@ and_return(true) action - expect(controller).to render_template('openid_connect/authorization/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) redirect_params = UriService.params(assigns(:oidc_redirect_uri)) @@ -684,7 +684,7 @@ and_return(true) action - expect(controller).to render_template('openid_connect/authorization/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) end end @@ -722,7 +722,7 @@ and_return(true) action - expect(controller).to render_template('openid_connect/authorization/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(params[:redirect_uri]) redirect_params = UriService.params(assigns(:oidc_redirect_uri)) diff --git a/spec/controllers/openid_connect/logout_controller_spec.rb b/spec/controllers/openid_connect/logout_controller_spec.rb index 5f9317970c8..c3f0901ed9a 100644 --- a/spec/controllers/openid_connect/logout_controller_spec.rb +++ b/spec/controllers/openid_connect/logout_controller_spec.rb @@ -75,7 +75,7 @@ and_return(true) action - expect(controller).to render_template('openid_connect/logout/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) end @@ -197,7 +197,7 @@ and_return(true) action - expect(controller).to render_template('openid_connect/logout/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) end end @@ -311,7 +311,7 @@ and_return(true) action - expect(controller).to render_template('openid_connect/logout/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) end end @@ -348,7 +348,7 @@ and_return(true) action - expect(controller).to render_template('openid_connect/logout/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) end end @@ -391,7 +391,7 @@ and_return(true) action - expect(controller).to render_template('openid_connect/logout/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) end end @@ -567,7 +567,7 @@ and_return(true) action - expect(controller).to render_template('openid_connect/logout/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) end end @@ -601,7 +601,7 @@ and_return(true) action - expect(controller).to render_template('openid_connect/logout/redirect') + expect(controller).to render_template('openid_connect/shared/redirect') expect(assigns(:oidc_redirect_uri)).to start_with(post_logout_redirect_uri) end From 63f99a007c1d1fb94707941335bfbb0a40d58a10 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 15:52:22 -0600 Subject: [PATCH 23/27] move redirecting to headings --- app/views/openid_connect/shared/redirect.html.erb | 2 +- app/views/saml_idp/shared/saml_post_binding.html.erb | 2 +- config/locales/headings/en.yml | 1 + config/locales/headings/es.yml | 1 + config/locales/headings/fr.yml | 1 + config/locales/saml_idp/en.yml | 1 - config/locales/saml_idp/es.yml | 1 - config/locales/saml_idp/fr.yml | 1 - 8 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/openid_connect/shared/redirect.html.erb b/app/views/openid_connect/shared/redirect.html.erb index 8736adcbf20..91b02f07472 100644 --- a/app/views/openid_connect/shared/redirect.html.erb +++ b/app/views/openid_connect/shared/redirect.html.erb @@ -2,7 +2,7 @@ - <%= t('saml_idp.shared.saml_post_binding.redirecting') %> | <%= APP_NAME %> + <%= t('headings.redirecting') %> | <%= APP_NAME %> <%= stylesheet_link_tag 'application', media: 'all' %> <%= render_stylesheet_once_tags %> diff --git a/app/views/saml_idp/shared/saml_post_binding.html.erb b/app/views/saml_idp/shared/saml_post_binding.html.erb index fad016f2439..eb827e26bf8 100644 --- a/app/views/saml_idp/shared/saml_post_binding.html.erb +++ b/app/views/saml_idp/shared/saml_post_binding.html.erb @@ -2,7 +2,7 @@ - <%= t('.redirecting') %> | <%= APP_NAME %> + <%= t('headings.redirecting') %> | <%= APP_NAME %> <%= csrf_meta_tags %> <%= stylesheet_link_tag 'application', media: 'all' %> <%= render_stylesheet_once_tags %> diff --git a/config/locales/headings/en.yml b/config/locales/headings/en.yml index 5066fd0a215..9459bc3a620 100644 --- a/config/locales/headings/en.yml +++ b/config/locales/headings/en.yml @@ -28,6 +28,7 @@ en: edit_info: password: Change your password phone: Manage your phone settings + redirecting: Redirecting passwords: change: Change your password confirm: Confirm your current password to continue diff --git a/config/locales/headings/es.yml b/config/locales/headings/es.yml index 301d7ebd5db..af3f579dd57 100644 --- a/config/locales/headings/es.yml +++ b/config/locales/headings/es.yml @@ -28,6 +28,7 @@ es: edit_info: password: Cambie su contraseña phone: Administrar la configuración de su teléfono + redirecting: Redirigiendo passwords: change: Cambie su contraseña confirm: Confirme la contraseña actual para continuar diff --git a/config/locales/headings/fr.yml b/config/locales/headings/fr.yml index 3f2100eebe1..d4af3f2589c 100644 --- a/config/locales/headings/fr.yml +++ b/config/locales/headings/fr.yml @@ -28,6 +28,7 @@ fr: edit_info: password: Changez votre mot de passe phone: Administrer les paramètres de votre téléphone + redirecting: Redirection passwords: change: Changez votre mot de passe confirm: Confirmez votre mot de passe actuel pour continuer diff --git a/config/locales/saml_idp/en.yml b/config/locales/saml_idp/en.yml index f273cd0af06..6c273e15a40 100644 --- a/config/locales/saml_idp/en.yml +++ b/config/locales/saml_idp/en.yml @@ -10,4 +10,3 @@ en: no_js: JavaScript seems to be turned off in your browser. Normally this step happens automatically, but because you have JavaScript turned off, please click the submit button to continue signing in or signing out. - redirecting: Redirecting diff --git a/config/locales/saml_idp/es.yml b/config/locales/saml_idp/es.yml index 9e57fd4c088..a408d15022e 100644 --- a/config/locales/saml_idp/es.yml +++ b/config/locales/saml_idp/es.yml @@ -11,4 +11,3 @@ es: paso se realiza automáticamente, pero debido a que tiene JavaScript desactivado, haga clic en el botón Enviar para continuar iniciando o cerrando la sesión. - redirecting: Redirigiendo diff --git a/config/locales/saml_idp/fr.yml b/config/locales/saml_idp/fr.yml index 4b31787ed91..7c10525b20d 100644 --- a/config/locales/saml_idp/fr.yml +++ b/config/locales/saml_idp/fr.yml @@ -11,4 +11,3 @@ fr: cette étape se déroule automatiquement, mais parce que vous avez désactivé le JavaScript, veuillez cliquer sur le lien « soumettre » pour continuer ou pour vous déconnecter. - redirecting: Redirection From 7c0a0dc331fe2d921f4b134ffd1ab1780cd7c769 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 15:52:58 -0600 Subject: [PATCH 24/27] freeze strings in openid controllers --- app/controllers/openid_connect/authorization_controller.rb | 2 ++ app/controllers/openid_connect/logout_controller.rb | 2 ++ 2 files changed, 4 insertions(+) diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 14bca296cec..e2113dda38d 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module OpenidConnect class AuthorizationController < ApplicationController include FullyAuthenticatable diff --git a/app/controllers/openid_connect/logout_controller.rb b/app/controllers/openid_connect/logout_controller.rb index 2e123dee8ec..5da023013ba 100644 --- a/app/controllers/openid_connect/logout_controller.rb +++ b/app/controllers/openid_connect/logout_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module OpenidConnect class LogoutController < ApplicationController include SecureHeadersConcern From d99dd743c5225b1c650396de4ed433e3511254c9 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 15:56:51 -0600 Subject: [PATCH 25/27] inline CSP test expectations Co-authored-by: Zach Margolis --- spec/controllers/users/rules_of_use_controller_spec.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/controllers/users/rules_of_use_controller_spec.rb b/spec/controllers/users/rules_of_use_controller_spec.rb index cc4f7fed6e7..012635be0b7 100644 --- a/spec/controllers/users/rules_of_use_controller_spec.rb +++ b/spec/controllers/users/rules_of_use_controller_spec.rb @@ -141,9 +141,8 @@ request_url: "http://test.com?#{URI.encode_www_form(params)}", } action - form_action = response.request.content_security_policy.form_action - csp_array = ["'self'", 'https://example.com'] - expect(form_action).to match_array(csp_array) + expect(response.request.content_security_policy.form_action). + to match_array(["'self'", 'https://example.com']) end it 'does not include service provider URIs in form-action CSP header when disabled' do @@ -165,9 +164,8 @@ request_url: "http://test.com?#{URI.encode_www_form(params)}", } action - form_action = response.request.content_security_policy.form_action - csp_array = ["'self'"] - expect(form_action).to match_array(csp_array) + expect(response.request.content_security_policy.form_action). + to match_array(["'self'"]) end end From 88b9fce8612bb12140680e5694caa3aaf4ab7dbf Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Fri, 1 Dec 2023 16:07:25 -0600 Subject: [PATCH 26/27] normalize yaml --- config/locales/headings/en.yml | 2 +- config/locales/headings/es.yml | 2 +- config/locales/headings/fr.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/locales/headings/en.yml b/config/locales/headings/en.yml index 9459bc3a620..f88741dd91d 100644 --- a/config/locales/headings/en.yml +++ b/config/locales/headings/en.yml @@ -28,7 +28,6 @@ en: edit_info: password: Change your password phone: Manage your phone settings - redirecting: Redirecting passwords: change: Change your password confirm: Confirm your current password to continue @@ -57,6 +56,7 @@ en: piv_cac_setup: already_associated: The PIV/CAC you presented is associated with another user. new: Use your PIV/CAC card to secure your account + redirecting: Redirecting residential_address: Current residential address session_timeout_warning: Need more time? sign_in_existing_users: Sign in for existing users diff --git a/config/locales/headings/es.yml b/config/locales/headings/es.yml index af3f579dd57..4b98a646142 100644 --- a/config/locales/headings/es.yml +++ b/config/locales/headings/es.yml @@ -28,7 +28,6 @@ es: edit_info: password: Cambie su contraseña phone: Administrar la configuración de su teléfono - redirecting: Redirigiendo passwords: change: Cambie su contraseña confirm: Confirme la contraseña actual para continuar @@ -57,6 +56,7 @@ es: piv_cac_setup: already_associated: La PIV/CAC que has presentado está asociada a otro usuario. new: Use su tarjeta PIV/CAC para asegurar su cuenta + redirecting: Redirigiendo residential_address: Dirección residencial actual session_timeout_warning: '¿Necesita más tiempo?' sign_in_existing_users: Iniciar sesión para usuarios existentes diff --git a/config/locales/headings/fr.yml b/config/locales/headings/fr.yml index d4af3f2589c..7353464d25e 100644 --- a/config/locales/headings/fr.yml +++ b/config/locales/headings/fr.yml @@ -28,7 +28,6 @@ fr: edit_info: password: Changez votre mot de passe phone: Administrer les paramètres de votre téléphone - redirecting: Redirection passwords: change: Changez votre mot de passe confirm: Confirmez votre mot de passe actuel pour continuer @@ -60,6 +59,7 @@ fr: already_associated: La carte PIV/CAC que vous avez présentée est associée à un autre utilisateur. new: Utilisez votre carte PIV/CAC pour sécuriser votre compte + redirecting: Redirection residential_address: Adresse de résidence actuelle session_timeout_warning: Vous avez besoin de plus de temps? sign_in_existing_users: S’identifier pour les utilisateurs existants From 9e60cb877605a83aaf3c20149102ca09a0330488 Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Mon, 4 Dec 2023 10:07:06 -0600 Subject: [PATCH 27/27] add background color to redirect page --- app/views/openid_connect/shared/redirect.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/openid_connect/shared/redirect.html.erb b/app/views/openid_connect/shared/redirect.html.erb index 91b02f07472..75531cb75e1 100644 --- a/app/views/openid_connect/shared/redirect.html.erb +++ b/app/views/openid_connect/shared/redirect.html.erb @@ -7,6 +7,6 @@ <%= render_stylesheet_once_tags %> - +