Skip to content

LG-12261 Stop reading from sp_session[:ial] #10091

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
jmhooper merged 1 commit into from
Feb 22, 2024
Merged

LG-12261 Stop reading from sp_session[:ial] #10091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions app/controllers/application_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,8 +387,11 @@ def set_locale
I18n.locale = LocaleChooser.new(params[:locale], request).locale
end

def sp_session_ial
sp_session[:ial].presence || 1
def pii_requested_but_locked?
if resolved_authn_context_result.identity_proofing? || resolved_authn_context_result.ialmax?
current_user.identity_verified? &&
!Pii::Cacher.new(current_user, user_session).exists_in_session?
end
end

def mfa_policy
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/concerns/billable_event_trackable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def mark_current_session_billed
def session_has_been_billed_flag_key
issuer = sp_session[:issuer]

if sp_session_ial == 1
if !resolved_authn_context_result.identity_proofing?
"auth_counted_#{issuer}ial1"
else
"auth_counted_#{issuer}"
Expand Down
8 changes: 1 addition & 7 deletions app/controllers/concerns/idv_session_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,7 @@ def redirect_unless_idv_session_user
def redirect_unless_sp_requested_verification
return if !IdentityConfig.store.idv_sp_required
return if idv_session_user.profiles.any?

ial_context = IalContext.new(
ial: sp_session_ial,
service_provider: sp_from_sp_session,
user: idv_session_user,
)
return if ial_context.ial2_or_greater?
return if resolved_authn_context_result.identity_proofing?

redirect_to account_url
end
Expand Down
12 changes: 11 additions & 1 deletion app/controllers/concerns/verify_sp_attributes_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def update_verified_attributes
current_user,
current_sp,
).link_identity(
ial: sp_session_ial,
ial: linked_identity_ial,
verified_attributes: sp_session[:requested_attributes],
last_consented_at: Time.zone.now,
clear_deleted_at: true,
Expand Down Expand Up @@ -62,6 +62,16 @@ def verified_after_consent?(last_estimated_consent)
verification_timestamp.present? && last_estimated_consent < verification_timestamp
end

def linked_identity_ial
if resolved_authn_context_result.ialmax?
0
elsif resolved_authn_context_result.identity_proofing?
2
else
1
end
Comment on lines 66 to 72
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Feb 15, 2024

Choose a reason for hiding this comment

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

maybe idea for future us would be to migrate this to be an enum column so we can name these?

Copy link
Copy Markdown
Contributor Author

@jmhooper jmhooper Feb 15, 2024

Choose a reason for hiding this comment

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

We have another story for sending the VoT back to the partner. @theabrad is working on it. For that we'll likely need to add the acr_values and vtr to the identity column and start reading from those instead of the ial and aal columns. Once we do that we can remove this portion entirely.

end

def find_sp_session_identity
current_user&.identities&.find_by(service_provider: sp_session[:issuer])
end
Expand Down
6 changes: 0 additions & 6 deletions app/controllers/openid_connect/authorization_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,12 +189,6 @@ def store_request
).call
end

def pii_requested_but_locked?
sp_session && sp_session_ial > 1 &&
current_user.identity_verified? &&
!Pii::Cacher.new(current_user, user_session).exists_in_session?
end

def track_events
event_ial_context = IalContext.new(
ial: @authorize_form.ial,
Expand Down
7 changes: 0 additions & 7 deletions app/controllers/saml_idp_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,6 @@ def set_devise_failure_redirect_for_concurrent_session_logout
request.env['devise_session_limited_failure_redirect_url'] = request.url
end

def pii_requested_but_locked?
if resolved_authn_context_result.identity_proofing_or_ialmax?
current_user.identity_verified? &&
!Pii::Cacher.new(current_user, user_session).exists_in_session?
end
end

def capture_analytics
analytics_payload = result.to_h.merge(
endpoint: api_saml_auth_path(path_year: params[:path_year]),
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/sign_up/completions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ def pii
end

def send_in_person_completion_survey
return unless sp_session_ial == ::Idp::Constants::IAL2
return unless resolved_authn_context_result.identity_proofing?

Idv::InPerson::CompletionSurveySender.send_completion_survey(
current_user,
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/users/passwords_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ class PasswordsController < ApplicationController
include ReauthenticationRequiredConcern

before_action :confirm_two_factor_authenticated
before_action :capture_password_if_pii_requested_but_locked
before_action :capture_password_if_pii_present_but_locked
before_action :confirm_recently_authenticated_2fa

def edit
Expand Down Expand Up @@ -33,7 +33,7 @@ def update

private

def capture_password_if_pii_requested_but_locked
def capture_password_if_pii_present_but_locked
return unless current_user.identity_verified? &&
!Pii::Cacher.new(current_user, user_session).exists_in_session?
user_session[:stored_location] = request.url
Expand Down
11 changes: 1 addition & 10 deletions app/controllers/users/piv_cac_login_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ def process_valid_submission
end

def next_step
if ial_context.ial2_requested? && current_user.identity_verified? &&
!Pii::Cacher.new(current_user, user_session).exists_in_session?
if pii_requested_but_locked?
capture_password_url
elsif !current_user.accepted_rules_of_use_still_valid?
rules_of_use_path
Expand All @@ -89,14 +88,6 @@ def next_step
end
end

def ial_context
@ial_context ||= IalContext.new(
ial: sp_session_ial,
service_provider: current_sp,
user: piv_cac_login_form.user,
)
end

def process_invalid_submission
session[:needs_to_setup_piv_cac_after_sign_in] = true if piv_cac_login_form.valid_token?

Expand Down
4 changes: 2 additions & 2 deletions spec/controllers/concerns/billable_event_trackable_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
:request_id,
:user_session,
:sp_session,
:sp_session_ial,
:resolved_authn_context_result,
:session,
) do
include BillableEventTrackable
Expand All @@ -33,7 +33,7 @@
current_user:,
request_id:,
user_session: {},
sp_session_ial: 1,
resolved_authn_context_result: double(identity_proofing?: false),
sp_session: {
issuer: current_sp.issuer,
},
Expand Down
8 changes: 4 additions & 4 deletions spec/controllers/idv_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,13 +194,13 @@

describe 'SP for IdV requirement' do
let(:current_sp) { create(:service_provider) }
let(:ial) { 2 }
let(:acr_values) { Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF }
let(:user) { build(:user, password: ControllerHelper::VALID_PASSWORD) }

before do
stub_sign_in(user)
if current_sp.present?
session[:sp] = { issuer: current_sp.issuer, ial: ial }
session[:sp] = { issuer: current_sp.issuer, acr_values: acr_values }
else
session[:sp] = {}
end
Expand Down Expand Up @@ -237,7 +237,7 @@
end

context 'with an SP context that does not require IdV' do
let(:ial) { 1 }
let(:acr_values) { Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF }

context 'when an SP is required' do
let(:idv_sp_required) { true }
Expand Down Expand Up @@ -266,7 +266,7 @@
end

context 'with an SP context that requires IdV' do
let(:ial) { 2 }
let(:acr_values) { Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF }

context 'when an SP is required' do
let(:idv_sp_required) { true }
Expand Down
19 changes: 15 additions & 4 deletions spec/controllers/sign_up/completions_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
stub_sign_in(user)
subject.session[:sp] = {
issuer: current_sp.issuer,
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
}
get :show

Expand All @@ -31,6 +32,7 @@
stub_sign_in(user)
subject.session[:sp] = {
issuer: current_sp.issuer,
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
ial2: false,
requested_attributes: [:email],
request_url: 'http://localhost:3000',
Expand Down Expand Up @@ -67,6 +69,7 @@
stub_sign_in(user)
subject.session[:sp] = {
issuer: current_sp.issuer,
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
ial2: true,
requested_attributes: [:email],
request_url: 'http://localhost:3000',
Expand Down Expand Up @@ -141,6 +144,7 @@
stub_sign_in(user)
subject.session[:sp] = {
issuer: current_sp.issuer,
acr_values: Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF,
ial2: false,
ialmax: true,
requested_attributes: [:email],
Expand Down Expand Up @@ -220,6 +224,7 @@
stub_sign_in(user)
subject.session[:sp] = { issuer: sp.issuer,
ial2: false,
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
requested_attributes: [:email],
request_url: 'http://localhost:3000' }
get :show
Expand All @@ -245,6 +250,7 @@
it 'tracks analytics' do
stub_sign_in(user)
subject.session[:sp] = {
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
ial2: false,
issuer: 'foo',
request_url: 'http://example.com',
Expand All @@ -271,7 +277,7 @@
stub_sign_in(user)
subject.session[:sp] = {
issuer: 'foo',
ial: 1,
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
request_url: 'http://example.com',
requested_attributes: ['email'],
}
Expand All @@ -290,6 +296,7 @@
it 'redirects to account page if the session request_url is removed' do
stub_sign_in(user)
subject.session[:sp] = {
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
ial2: false,
issuer: 'foo',
requested_attributes: ['email'],
Expand All @@ -306,6 +313,7 @@
DisposableEmailDomain.create(name: 'temporary.com')
stub_sign_in(user)
subject.session[:sp] = {
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
ial2: false,
issuer: 'foo',
request_url: 'http://example.com',
Expand Down Expand Up @@ -343,6 +351,7 @@
sp = create(:service_provider, issuer: 'https://awesome')
subject.session[:sp] = {
issuer: sp.issuer,
acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF,
ial2: true,
request_url: 'http://example.com',
requested_attributes: ['email'],
Expand Down Expand Up @@ -371,7 +380,7 @@
sp = create(:service_provider, issuer: 'https://awesome')
subject.session[:sp] = {
issuer: sp.issuer,
ial: 2,
acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF,
request_url: 'http://example.com',
requested_attributes: %w[email first_name verified_at],
}
Expand All @@ -395,12 +404,11 @@
sp = create(:service_provider, issuer: 'https://awesome')
subject.session[:sp] = {
issuer: sp.issuer,
ial: 2,
acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF,
request_url: 'http://example.com',
requested_attributes: %w[email first_name verified_at],
}
allow(@linker).to receive(:link_identity).with(
ial: 2,
verified_attributes: %w[email first_name verified_at],
last_consented_at: now,
clear_deleted_at: true,
Expand Down Expand Up @@ -428,6 +436,7 @@
ial2: false,
issuer: 'foo',
request_url: 'http://example.com',
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
}
patch :update
end
Expand All @@ -441,6 +450,7 @@
ial2: false,
issuer: 'foo',
request_url: 'http://example.com',
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
}

expect(original_profile.activated_at).to be_present
Expand Down Expand Up @@ -468,6 +478,7 @@
subject.session[:sp] = {
ial2: false,
request_url: 'http://example.com',
acr_values: Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
}

patch :update
Expand Down
31 changes: 26 additions & 5 deletions spec/controllers/users/piv_cac_login_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,12 @@

context 'with a valid token' do
let(:service_provider) { create(:service_provider) }
let(:sp_session) { { ial: 1, issuer: service_provider.issuer } }
let(:sp_session) do
{
acr_values: Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF,
issuer: service_provider.issuer,
}
end
let(:nonce) { SecureRandom.base64(20) }
let(:data) do
{
Expand Down Expand Up @@ -175,7 +180,12 @@
end

context 'ial2 service_level' do
let(:sp_session) { { ial: Idp::Constants::IAL2, issuer: service_provider.issuer } }
let(:sp_session) do
{
acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF,
issuer: service_provider.issuer,
}
end

it 'redirects to account' do
expect(response).to redirect_to(account_url)
Expand All @@ -184,7 +194,10 @@

context 'ial_max service level' do
let(:sp_session) do
{ ial: Idp::Constants::IAL_MAX, issuer: service_provider.issuer }
{
acr_values: Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF,
issuer: service_provider.issuer,
}
end

it 'redirects to the after_sign_in_path_for' do
Expand All @@ -203,7 +216,12 @@
end

context 'ial2 service_level' do
let(:sp_session) { { ial: Idp::Constants::IAL2, issuer: service_provider.issuer } }
let(:sp_session) do
{
acr_values: Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF,
issuer: service_provider.issuer,
}
end

it 'redirects to the capture_password_url' do
expect(response).to redirect_to(capture_password_url)
Expand All @@ -212,7 +230,10 @@

context 'ial_max service_level' do
let(:sp_session) do
{ ial: Idp::Constants::IAL_MAX, issuer: service_provider.issuer }
{
acr_values: Saml::Idp::Constants::IALMAX_AUTHN_CONTEXT_CLASSREF,
issuer: service_provider.issuer,
}
end

it 'redirects to the capture_password_url' do
Expand Down