Skip to content

Commit

Permalink
Preserve request_id param across multiple visits
Browse files Browse the repository at this point in the history
**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.
  • Loading branch information
monfresh authored and Peter Karman committed Apr 20, 2017
1 parent f92bd0e commit 52e6baf
Show file tree
Hide file tree
Showing 7 changed files with 58 additions and 5 deletions.
4 changes: 4 additions & 0 deletions app/controllers/concerns/fully_authenticatable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/sign_up/registrations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions spec/controllers/sign_up/registrations_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
30 changes: 30 additions & 0 deletions spec/features/openid_connect/openid_connect_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
14 changes: 14 additions & 0 deletions spec/features/saml/loa1_sso_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down

0 comments on commit 52e6baf

Please sign in to comment.