Skip to content
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

Tracking AAL attribute value differences #11817

Merged
merged 4 commits into from
Feb 4, 2025
Merged

Tracking AAL attribute value differences #11817

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f224557
Add method to derive AAL value to AuthnContextResolver
Sgtpluck Jan 28, 2025
61942f2
Add analytics event and track SAML aal differences
Sgtpluck Jan 28, 2025
6c1a157
changelog: Internal, Analytics, Add tracking for response AAL fix
Sgtpluck Jan 29, 2025
d09e2a9
Adds acr_values to Identity Factory to fix tests
Sgtpluck Feb 4, 2025
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
18 changes: 17 additions & 1 deletion app/presenters/openid_connect_user_info_presenter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def user_info
info[:verified_at] = verified_at if scoper.verified_at_requested?
if identity.vtr.nil?
info[:ial] = authn_context_resolver.asserted_ial_acr
info[:aal] = identity.requested_aal_value
info[:aal] = requested_aal_value
else
info[:vot] = vot_values
info[:vtm] = IdentityConfig.store.vtm_url
Expand All @@ -41,6 +41,22 @@ def url_options

private

def analytics
Analytics.new(user: identity.user, request: nil, session: {}, sp: nil)
end
Comment on lines +44 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like our typical pattern is to log analytics from the controller level. is there a way that we can have this class expose an analytics_attributes or something that we can leverage from the controller? then we can attribute the right request, SP, session info as needed as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i found some examples of this pattern, so thought that it would be okay in this scenario, especially since it's a short-term event that will be removed when it's safe to implement the actual fix.

but i don't feel that strongly about it, and can try to figure out how to expose the needed data at the controller level -- just means it will have to wait until after i'm back!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think some of the context such as SP is valuable, maybe as another option we could popular sp: from that data here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree that we could populate sp data -- i'm not sure what "as another option" means exactly here? do you mean keeping this event in the presenter with the included data?

Copy link
Contributor

@mitchellhenke mitchellhenke Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think the suggestion is to use identity and pass in sp: identity.service_provider or similar. In this case, it is probably less useful since we pass in client_id: service_provider.issuer, to the event itself.

Copy link
Contributor Author

@Sgtpluck Sgtpluck Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i know that the Analytics object adds some extra details if the sp value is there, so i can add it in, in case any of those details end up being useful! actually, when i look a little closer, i think that's only the case if there's also a session object. so maybe it's not that useful


def requested_aal_value
if identity.requested_aal_value != authn_context_resolver.asserted_aal_acr
analytics.asserted_aal_different_from_response_aal(
asserted_aal_value: authn_context_resolver.asserted_aal_acr,
client_id: identity&.service_provider_record&.issuer,
response_aal_value: identity.requested_aal_value,
)
end

identity.requested_aal_value
end

def vot_values
AuthnContextResolver.new(
user: identity.user,
Expand Down
21 changes: 21 additions & 0 deletions app/services/analytics_events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,27 @@ def add_phone_setup_visit
)
end

# Temporary event:
# Tracks when the AAL value that we are returning to the integration
# is different from the actual asserted value
# @param [String] asserted_aal_value The actual AAL value the IdP asserts
# @param [String] client_id
# @param [String] response_aal_value The AAL value the IdP returns via attributes
def asserted_aal_different_from_response_aal(
asserted_aal_value:,
client_id:,
response_aal_value:,
**extra
)
track_event(
:asserted_aal_different_from_response_aal,
asserted_aal_value:,
client_id:,
response_aal_value:,
**extra,
)
end

# @identity.idp.previous_event_name TOTP: User Disabled
# Tracks when a user deletes their auth app from account
# @param [Boolean] success
Expand Down
17 changes: 17 additions & 0 deletions app/services/attribute_asserter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ def build
:decrypted_pii,
:user_session

def analytics
Analytics.new(user:, request: nil, session: {}, sp: nil)
end
Comment on lines +62 to +64
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto about logging from the controller layer if possible! SAML idp controller is a mess but is there a way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is actually way messier than the OIDC code path, since the AttributeAsserter object is used in private methods in the SamlIdpAuthConcern and is comingled with methods in the SamlIdp gem so i think it's not worth it here, especially for a temporary event.


def should_add_proofed_attributes?
return false if !user.active_profile.present?
resolved_authn_context_result.identity_proofing_or_ialmax?
Expand Down Expand Up @@ -150,9 +154,22 @@ def add_aal(attrs)
requested_aal_level = Saml::Idp::Constants::AUTHN_CONTEXT_CLASSREF_TO_AAL[requested_context]
aal_level = requested_aal_level || service_provider.default_aal || ::Idp::Constants::DEFAULT_AAL
context = Saml::Idp::Constants::AUTHN_CONTEXT_AAL_TO_CLASSREF[aal_level]

if context != asserted_aal_value
analytics.asserted_aal_different_from_response_aal(
asserted_aal_value:,
client_id: service_provider.issuer,
response_aal_value: context,
)
end

attrs[:aal] = { getter: aal_getter_function(context) } if context
end

def asserted_aal_value
authn_context_resolver.asserted_aal_acr
end

def add_ial(attrs)
asserted_ial = authn_context_resolver.asserted_ial_acr
attrs[:ial] = { getter: ial_getter_function(asserted_ial) } if asserted_ial
Expand Down
12 changes: 12 additions & 0 deletions app/services/authn_context_resolver.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,18 @@ def asserted_ial_acr
end
end

def asserted_aal_acr
if result.hspd12?
Saml::Idp::Constants::AAL2_HSPD12_AUTHN_CONTEXT_CLASSREF
elsif result.phishing_resistant?
Saml::Idp::Constants::AAL2_PHISHING_RESISTANT_AUTHN_CONTEXT_CLASSREF
elsif result.aal2?
Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF
else
Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF
end
end

private

def selected_vtr_parser_result_from_vtr_list
Expand Down
124 changes: 83 additions & 41 deletions spec/presenters/openid_connect_user_info_presenter_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,12 @@
)
end

let(:fake_analytics) { FakeAnalytics.new(user: identity.user) }

before do
OutOfBandSessionAccessor.new(rails_session_id).put_locale(locale, 5.minutes.in_seconds)
allow(Analytics).to receive(:new).and_return(fake_analytics)
stub_analytics
end

subject(:presenter) { OpenidConnectUserInfoPresenter.new(identity) }
Expand Down Expand Up @@ -127,27 +131,64 @@
context 'with ACR values' do
let(:vtr) { nil }

context 'no identity proofing' do
let(:acr_values) do
[
Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF,
Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
].join(' ')
context 'without identity proofing' do
context 'with an AAL value requested' do
let(:acr_values) do
[
Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF,
Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
].join(' ')
end
let(:requested_aal_value) { Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF }
let(:scope) { 'openid email all_emails locale' }

it 'includes the correct attributes' do
aggregate_failures do
expect(user_info[:sub]).to eq(identity.uuid)
expect(user_info[:iss]).to eq(root_url)
expect(user_info[:email]).to eq(identity.user.email_addresses.first.email)
expect(user_info[:email_verified]).to eq(true)
expect(user_info[:all_emails]).to eq([identity.user.email_addresses.first.email])
expect(user_info[:locale]).to eq(locale)
expect(user_info[:ial]).to eq(Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF)
expect(user_info[:aal]).to eq(Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF)
expect(user_info).to_not have_key(:vot)
end
end

it 'does not track an AAL mismatch' do
user_info

expect(@analytics).to_not have_logged_event(
:asserted_aal_different_from_response_aal,
)
end
end
let(:requested_aal_value) { Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF }
let(:scope) { 'openid email all_emails locale' }

it 'includes the correct attributes' do
aggregate_failures do
expect(user_info[:sub]).to eq(identity.uuid)
expect(user_info[:iss]).to eq(root_url)
expect(user_info[:email]).to eq(identity.user.email_addresses.first.email)
expect(user_info[:email_verified]).to eq(true)
expect(user_info[:all_emails]).to eq([identity.user.email_addresses.first.email])
expect(user_info[:locale]).to eq(locale)
expect(user_info[:ial]).to eq(Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF)
expect(user_info[:aal]).to eq(Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF)
expect(user_info).to_not have_key(:vot)
context 'without an AAL value requested' do
let(:acr_values) do
[
Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF,
].join(' ')
end
let(:requested_aal_value) { Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF }

context 'with a requested_aal_value that does not match the request' do
it 'includes an aal value based on ' do
expect(user_info[:aal]).to eq(
Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF,
)
end

it 'tracks an AAL mismatch' do
user_info
expect(@analytics).to have_logged_event(
:asserted_aal_different_from_response_aal,
asserted_aal_value: Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF,
client_id: service_provider.issuer,
response_aal_value: Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF,
)
end
end
end
end
Expand Down Expand Up @@ -328,16 +369,25 @@
)
end

let(:acr_values) do
[
Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF,
Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
].join(' ')
end

let(:scope) do
'openid email x509'
end
let(:user) { create(:user, :with_piv_or_cac) }

let(:identity) do
build(
:service_provider_identity,
rails_session_id: rails_session_id,
user: create(:user, :with_piv_or_cac),
scope: scope,
rails_session_id:,
user:,
scope:,
acr_values:,
)
end

Expand Down Expand Up @@ -366,14 +416,7 @@
end

context 'when the user does not have an associated piv/cac' do
let(:identity) do
build(
:service_provider_identity,
rails_session_id: rails_session_id,
user: create(:user, :fully_registered),
scope: scope,
)
end
let(:user) { create(:user, :fully_registered) }

it 'includes blank x509 claims' do
aggregate_failures do
Expand All @@ -386,12 +429,20 @@
end

context 'with a deleted email' do
let(:acr_values) do
[
Saml::Idp::Constants::AAL2_AUTHN_CONTEXT_CLASSREF,
Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF,
].join(' ')
end

let(:identity) do
build(
:service_provider_identity,
rails_session_id: rails_session_id,
rails_session_id:,
user: create(:user, :fully_registered, :with_multiple_emails),
scope: scope,
scope:,
acr_values:,
)
end

Expand All @@ -409,15 +460,6 @@
end

context 'with nil email id' do
let(:identity) do
build(
:service_provider_identity,
rails_session_id: rails_session_id,
user: create(:user, :fully_registered),
scope: scope,
)
end

before do
identity.email_address_id = nil
end
Expand Down
Loading