Skip to content
Merged
8 changes: 8 additions & 0 deletions app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ def pre_validate_authorize_form
user_fully_authenticated: user_fully_authenticated?,
referer: request.referer,
vtr_param: params[:vtr],
unknown_authn_contexts:,
),
)
return if result.success?
Expand Down Expand Up @@ -258,5 +259,12 @@ def redirect_user(redirect_uri, issuer, user_uuid)
def sp_handoff_bouncer
@sp_handoff_bouncer ||= SpHandoffBouncer.new(sp_session)
end

def unknown_authn_contexts
return nil if params[:vtr].present? || params[:acr_values].blank?

(params[:acr_values].split - Saml::Idp::Constants::VALID_AUTHN_CONTEXTS).
join(' ').presence
end
end
end
25 changes: 24 additions & 1 deletion app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ def capture_analytics
request_signed: saml_request.signed?,
matching_cert_serial:,
requested_nameid_format: saml_request.name_id_format,
unknown_authn_contexts:,
)

if result.success? && saml_request.signed?
Expand All @@ -151,12 +152,13 @@ def log_external_saml_auth_request

analytics.saml_auth_request(
requested_ial: requested_ial,
authn_context: saml_request&.requested_authn_contexts,
authn_context: requested_authn_contexts,
requested_aal_authn_context: FederatedProtocols::Saml.new(saml_request).aal,
requested_vtr_authn_contexts: saml_request&.requested_vtr_authn_contexts.presence,
force_authn: saml_request&.force_authn?,
final_auth_request: sp_session[:final_auth_request],
service_provider: saml_request&.issuer,
unknown_authn_contexts:,
user_fully_authenticated: user_fully_authenticated?,
)
end
Expand Down Expand Up @@ -227,4 +229,25 @@ def resolved_authn_context_int_ial
def require_path_year
render_not_found if params[:path_year].blank?
end

def unknown_authn_contexts
return nil if saml_request.requested_vtr_authn_contexts.present?
return nil if requested_authn_contexts.blank?

unmatched_authn_contexts.reject do |authn_context|
authn_context.match(req_attrs_regexp)
end.join(' ').presence
end

def unmatched_authn_contexts
requested_authn_contexts - Saml::Idp::Constants::VALID_AUTHN_CONTEXTS
end

def requested_authn_contexts
@request_authn_contexts || saml_request&.requested_authn_contexts
end

def req_attrs_regexp
Regexp.escape(Saml::Idp::Constants::REQUESTED_ATTRIBUTES_CLASSREF)
end
end
9 changes: 9 additions & 0 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5526,6 +5526,7 @@ def openid_connect_bearer_token(success:, ial:, client_id:, errors:, error_detai
# @param [String, nil] vtr_param
# @param [Boolean] unauthorized_scope
# @param [Boolean] user_fully_authenticated
# @param [String] unknown_authn_contexts space separated list of unknown contexts
def openid_connect_request_authorization(
success:,
errors:,
Expand All @@ -5542,6 +5543,7 @@ def openid_connect_request_authorization(
unauthorized_scope:,
user_fully_authenticated:,
error_details: nil,
unknown_authn_contexts: nil,
**extra
)
track_event(
Expand All @@ -5561,6 +5563,7 @@ def openid_connect_request_authorization(
vtr_param:,
unauthorized_scope:,
user_fully_authenticated:,
unknown_authn_contexts:,
**extra,
)
end
Expand Down Expand Up @@ -6337,6 +6340,7 @@ def rules_of_use_visit
# matches the request certificate in a successful, signed request
# @param [Hash] cert_error_details Details for errors that occurred because of an invalid
# signature
# @param [String] unknown_authn_contexts space separated list of unknown contexts
def saml_auth(
success:,
errors:,
Expand All @@ -6353,6 +6357,7 @@ def saml_auth(
matching_cert_serial:,
error_details: nil,
cert_error_details: nil,
unknown_authn_contexts: nil,
**extra
)
track_event(
Expand All @@ -6372,6 +6377,7 @@ def saml_auth(
request_signed:,
matching_cert_serial:,
cert_error_details:,
unknown_authn_contexts:,
**extra,
)
end
Expand All @@ -6383,6 +6389,7 @@ def saml_auth(
# @param [Boolean] force_authn
# @param [Boolean] final_auth_request
# @param [String] service_provider
# @param [String] unknown_authn_contexts space separated list of unknown contexts
# @param [Boolean] user_fully_authenticated
# An external request for SAML Authentication was received
def saml_auth_request(
Expand All @@ -6393,6 +6400,7 @@ def saml_auth_request(
force_authn:,
final_auth_request:,
service_provider:,
unknown_authn_contexts:,
user_fully_authenticated:,
**extra
)
Expand All @@ -6405,6 +6413,7 @@ def saml_auth_request(
force_authn:,
final_auth_request:,
service_provider:,
unknown_authn_contexts:,
user_fully_authenticated:,
**extra,
)
Expand Down
5 changes: 4 additions & 1 deletion app/services/saml_request_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@ def valid_authn_context?
next true if classref.match?(SamlIdp::Request::VTR_REGEXP) &&
IdentityConfig.store.use_vot_in_sp_requests
end
authn_contexts.all? do |classref|
# SAML requests are allowed to "default" to the integration's IAL default.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🙌

return true if authn_contexts.empty?

authn_contexts.any? do |classref|
valid_contexts.include?(classref)
end
end
Expand Down
15 changes: 1 addition & 14 deletions app/services/vot/parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ module Vot
class Parser
class ParseException < StandardError; end

class UnsupportedComponentsException < ParseException; end

class DuplicateComponentsException < ParseException; end

Result = Data.define(
Expand Down Expand Up @@ -87,8 +85,7 @@ def initial_components
@initial_components ||= component_string.split(component_separator).map do |component_name|
component_map.fetch(component_name)
rescue KeyError
raise_unsupported_component_exception(component_name)
end
end.compact
end

def component_separator
Expand All @@ -113,16 +110,6 @@ def validate_component_uniqueness!(component_values)
end
end

def raise_unsupported_component_exception(component_value_name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll remark here that this may be a deviation from the spec since we are accepting vectors we do not recognize that are not described by our trustmark. However, that may not be a huge deal since this API should be going away soon.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks for that note! i could probably update it to only raise when vectors of trust are passed in, if you have any feelings about it! (although i think we are deviating from the spec in some ways anyways, like having implied vectors, and as you said, it's hopefully not long for our code base.)

if vector_of_trust.present?
raise UnsupportedComponentsException,
"'#{vector_of_trust}' contains unknown component '#{component_value_name}'"
else
raise UnsupportedComponentsException,
"'#{acr_values}' contains unknown acr value '#{component_value_name}'"
end
end

def raise_duplicate_component_exception
if vector_of_trust.present?
raise DuplicateComponentsException, "'#{vector_of_trust}' contains duplicate components"
Expand Down
27 changes: 26 additions & 1 deletion spec/controllers/application_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ def index
let(:vtr) { nil }
let(:acr_values) do
[
'http://idmanagement.gov/ns/assurance/aal/1',
Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF,
].join(' ')
end

Expand All @@ -486,6 +486,31 @@ def index
expect(result.identity_proofing?).to eq(true)
end

context 'when an unknown acr value is passed in' do
let(:acr_values) { 'unknown-acr-value' }

it 'raises an exception' do
expect { result }.to raise_exception(
Vot::Parser::ParseException,
'VoT parser called without VoT or ACR values',
)
end

context 'with a known acr value' do
let(:acr_values) do
[
Saml::Idp::Constants::DEFAULT_AAL_AUTHN_CONTEXT_CLASSREF,
'unknown-acr-value',
].join(' ')
end

it 'returns a resolved authn context result' do
expect(result.aal2?).to eq(true)
expect(result.identity_proofing?).to eq(true)
end
end
end

context 'without an SP' do
let(:sp) { nil }
let(:sp_session) { nil }
Expand Down
59 changes: 59 additions & 0 deletions spec/controllers/openid_connect/authorization_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2029,6 +2029,65 @@
end
end

context 'when there are unknown acr_values params' do
let(:unknown_value) { 'unknown-acr-value' }
let(:acr_values) { unknown_value }

context 'when there is only an unknown acr_value' do
it 'tracks the event with errors' do
stub_analytics

action

expect(@analytics).to have_logged_event(
'OpenID Connect: authorization request',
success: false,
client_id:,
prompt:,
allow_prompt_login: true,
unauthorized_scope: false,
errors: hash_including(:acr_values),
error_details: hash_including(:acr_values),
user_fully_authenticated: true,
acr_values: '',
code_challenge_present: false,
scope: 'openid profile',
unknown_authn_contexts: unknown_value,
)
end

context 'when there is also a valid acr_value' do
let(:known_value) { Saml::Idp::Constants::IAL2_AUTHN_CONTEXT_CLASSREF }
let(:acr_values) do
[
unknown_value,
known_value,
].join(' ')
end

it 'tracks the event' do
stub_analytics

action
expect(@analytics).to have_logged_event(
'OpenID Connect: authorization request',
success: true,
client_id:,
prompt:,
allow_prompt_login: true,
unauthorized_scope: false,
user_fully_authenticated: true,
acr_values: known_value,
code_challenge_present: false,
scope: 'openid profile',
unknown_authn_contexts: unknown_value,
errors: {},
)
end
end
end
end

context 'vtr with invalid params that do not interfere with the redirect_uri' do
let(:acr_values) { nil }
let(:vtr) { ['C1'].to_json }
Expand Down
53 changes: 50 additions & 3 deletions spec/controllers/saml_idp_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -970,15 +970,22 @@ def name_id_version(format_urn)
end

context 'authn_context is invalid' do
it 'renders an error page' do
let(:unknown_value) do
'http://idmanagement.gov/ns/assurance/loa/5'
end
let(:authn_context) { unknown_value }

before do
stub_analytics

saml_get_auth(
saml_settings(
overrides: { authn_context: 'http://idmanagement.gov/ns/assurance/loa/5' },
overrides: { authn_context: },
),
)
end

it 'renders an error page' do
expect(controller).to render_template('saml_idp/auth/error')
expect(response.status).to eq(400)
expect(response.body).to include(t('errors.messages.unauthorized_authn_context'))
Expand All @@ -989,7 +996,7 @@ def name_id_version(format_urn)
errors: { authn_context: [t('errors.messages.unauthorized_authn_context')] },
error_details: { authn_context: { unauthorized_authn_context: true } },
nameid_format: Saml::Idp::Constants::NAME_ID_FORMAT_PERSISTENT,
authn_context: ['http://idmanagement.gov/ns/assurance/loa/5'],
authn_context: [unknown_value],
authn_context_comparison: 'exact',
service_provider: 'http://localhost:3000',
request_signed: true,
Expand All @@ -998,9 +1005,49 @@ def name_id_version(format_urn)
idv: false,
finish_profile: false,
matching_cert_serial: saml_test_sp_cert_serial,
unknown_authn_contexts: unknown_value,
),
)
end

context 'there is also a valid authn_context' do
let(:authn_context) do
[
Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
unknown_value,
]
end

it 'logs the unknown authn_context value' do
expect(response.status).to eq(302)
expect(@analytics).to have_logged_event(
'SAML Auth Request',
hash_including(
unknown_authn_contexts: unknown_value,
),
)
end

context 'when it includes the ReqAttributes AuthnContext' do
let(:authn_context) do
[
Saml::Idp::Constants::REQUESTED_ATTRIBUTES_CLASSREF,
Saml::Idp::Constants::IAL1_AUTHN_CONTEXT_CLASSREF,
unknown_value,
]
end

it 'logs the unknown authn_context value' do
expect(response.status).to eq(302)
expect(@analytics).to have_logged_event(
'SAML Auth Request',
hash_including(
unknown_authn_contexts: unknown_value,
),
)
end
end
end
end

context 'authn_context scenarios' do
Expand Down
Loading