From 52e6baf1ee60dcd8397f57f0163a36837482c320 Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Fri, 14 Apr 2017 12:04:16 -0400 Subject: [PATCH] Preserve request_id param across multiple visits **Why**: Before, we were passing in the request_id param to the landing page via the `@request_id` variable, which does not get set on each request unless `sp_session[:request_id]` is blank. If you visit the IdP via the SP, then go back to the SP and visit the IdP again within the session validity period, `@request_id` will be blank on the second request, and so the param will be lost. **How**: Read the value from the session instead. --- .../concerns/fully_authenticatable.rb | 4 +++ .../authorization_controller.rb | 2 +- app/controllers/saml_idp_controller.rb | 2 +- .../sign_up/registrations_controller.rb | 2 +- .../sign_up/registrations_controller_spec.rb | 9 ++++-- .../openid_connect/openid_connect_spec.rb | 30 +++++++++++++++++++ spec/features/saml/loa1_sso_spec.rb | 14 +++++++++ 7 files changed, 58 insertions(+), 5 deletions(-) diff --git a/app/controllers/concerns/fully_authenticatable.rb b/app/controllers/concerns/fully_authenticatable.rb index c07f9e3670d..f5e7eb0665c 100644 --- a/app/controllers/concerns/fully_authenticatable.rb +++ b/app/controllers/concerns/fully_authenticatable.rb @@ -11,4 +11,8 @@ def delete_branded_experience ServiceProviderRequest.from_uuid(sp_session[:request_id]).delete session.delete(:sp) end + + def request_id + sp_session[:request_id] + end end diff --git a/app/controllers/openid_connect/authorization_controller.rb b/app/controllers/openid_connect/authorization_controller.rb index 6a2a8d3867b..0173d1b624f 100644 --- a/app/controllers/openid_connect/authorization_controller.rb +++ b/app/controllers/openid_connect/authorization_controller.rb @@ -10,7 +10,7 @@ class AuthorizationController < ApplicationController before_action :load_authorize_form_from_session, only: %i[create destroy] def index - return confirm_two_factor_authenticated(@request_id) unless user_fully_authenticated? + return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated? return redirect_to verify_url if identity_needs_verification? track_index_action_analytics diff --git a/app/controllers/saml_idp_controller.rb b/app/controllers/saml_idp_controller.rb index 76108f2e260..5ac27ccd809 100644 --- a/app/controllers/saml_idp_controller.rb +++ b/app/controllers/saml_idp_controller.rb @@ -12,7 +12,7 @@ class SamlIdpController < ApplicationController skip_before_action :handle_two_factor_authentication, only: :logout def auth - return confirm_two_factor_authenticated(@request_id) unless user_fully_authenticated? + return confirm_two_factor_authenticated(request_id) unless user_fully_authenticated? process_fully_authenticated_user do |needs_idv| return store_location_and_redirect_to_verify_url if needs_idv end diff --git a/app/controllers/sign_up/registrations_controller.rb b/app/controllers/sign_up/registrations_controller.rb index 020ccc2a055..b1c2f16539e 100644 --- a/app/controllers/sign_up/registrations_controller.rb +++ b/app/controllers/sign_up/registrations_controller.rb @@ -8,7 +8,7 @@ class RegistrationsController < ApplicationController prepend_before_action :disable_account_creation, only: %i[new create] def show - return redirect_to sign_up_email_path if session[:sp].blank? + return redirect_to sign_up_email_path if params[:request_id].blank? analytics.track_event(Analytics::USER_REGISTRATION_INTRO_VISIT) end diff --git a/spec/controllers/sign_up/registrations_controller_spec.rb b/spec/controllers/sign_up/registrations_controller_spec.rb index 1f825e39973..b06bd3084c9 100644 --- a/spec/controllers/sign_up/registrations_controller_spec.rb +++ b/spec/controllers/sign_up/registrations_controller_spec.rb @@ -118,12 +118,11 @@ describe '#show' do it 'tracks page visit' do stub_analytics - session[:sp] = { loa3: true } expect(@analytics).to receive(:track_event). with(Analytics::USER_REGISTRATION_INTRO_VISIT) - get :show + get :show, request_id: 'foo' end it 'cannot be viewed by signed in users' do @@ -133,5 +132,11 @@ expect(response).to redirect_to profile_path end + + it 'redirects to sign_up_email_path if request_id param is missing' do + get :show + + expect(response).to redirect_to sign_up_email_path + end end end diff --git a/spec/features/openid_connect/openid_connect_spec.rb b/spec/features/openid_connect/openid_connect_spec.rb index fdd8cd52e97..29704ffccb9 100644 --- a/spec/features/openid_connect/openid_connect_spec.rb +++ b/spec/features/openid_connect/openid_connect_spec.rb @@ -336,6 +336,36 @@ end end + context 'visiting IdP via SP, then going back to SP and visiting IdP again' do + it 'maintains the request_id in the params' do + visit_idp_from_sp_with_loa1 + sp_request_id = ServiceProviderRequest.last.uuid + + expect(current_url).to eq sign_up_start_url(request_id: sp_request_id) + + visit_idp_from_sp_with_loa1 + + expect(current_url).to eq sign_up_start_url(request_id: sp_request_id) + end + end + + def visit_idp_from_sp_with_loa1 + client_id = 'urn:gov:gsa:openidconnect:sp:server' + state = SecureRandom.hex + nonce = SecureRandom.hex + + visit openid_connect_authorize_path( + client_id: client_id, + response_type: 'code', + acr_values: Saml::Idp::Constants::LOA1_AUTHN_CONTEXT_CLASSREF, + scope: 'openid email', + redirect_uri: 'http://localhost:7654/auth/result', + state: state, + prompt: 'select_account', + nonce: nonce + ) + end + def sp_public_key page.driver.get api_openid_connect_certs_path diff --git a/spec/features/saml/loa1_sso_spec.rb b/spec/features/saml/loa1_sso_spec.rb index ff785283c25..85ee65115c7 100644 --- a/spec/features/saml/loa1_sso_spec.rb +++ b/spec/features/saml/loa1_sso_spec.rb @@ -117,6 +117,20 @@ end end + context 'visiting IdP via SP, then going back to SP and visiting IdP again' do + it 'maintains the request_id in the params' do + authn_request = auth_request.create(saml_settings) + visit authn_request + sp_request_id = ServiceProviderRequest.last.uuid + + expect(current_url).to eq sign_up_start_url(request_id: sp_request_id) + + visit authn_request + + expect(current_url).to eq sign_up_start_url(request_id: sp_request_id) + end + end + def sign_in_and_require_viewing_personal_key(user) login_as(user, scope: :user, run_callbacks: false) Warden.on_next_request do |proxy|