Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions app/controllers/concerns/saml_idp_auth_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ def validate_service_provider_and_authn_context
return if result.success?

capture_analytics
track_integration_errors(
event: :saml_auth_request,
errors: result.errors.values.flatten,
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.

question: Is there any chance that result.errors will be nil or empty? If so, does it make sense for this event to still get logged?

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.

no chance -- return if result.success? indicates that any flow that does not include an error will return before getting here.

)

render 'saml_idp/auth/error', status: :bad_request
end

Expand Down Expand Up @@ -248,4 +253,14 @@ def request_url
url.query = Rack::Utils.build_query(query_params).presence
url.to_s
end

def track_integration_errors(event:, errors: nil)
analytics.sp_integration_errors_present(
error_details: errors || saml_request.errors.uniq,
error_types: [:saml_request_errors],
event:,
integration_exists: saml_request_service_provider.present?,
request_issuer: saml_request&.issuer,
)
end
end
36 changes: 25 additions & 11 deletions app/controllers/openid_connect/authorization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def pending_profile_policy
end

def check_sp_active
return if @authorize_form.service_provider&.active?
return if service_provider&.active?
redirect_to sp_inactive_error_url
end

Expand Down Expand Up @@ -100,7 +100,7 @@ def email_address_id
def ial_context
IalContext.new(
ial: resolved_authn_context_int_ial,
service_provider: @authorize_form.service_provider,
service_provider:,
user: current_user,
)
end
Expand All @@ -121,7 +121,6 @@ def handle_successful_handoff

redirect_user(
@authorize_form.success_redirect_uri,
@authorize_form.service_provider.issuer,
current_user.uuid,
)

Expand Down Expand Up @@ -153,13 +152,13 @@ def build_authorize_form_from_params

def secure_headers_override
return if form_action_csp_disabled_and_not_server_side_redirect?(
issuer: @authorize_form.service_provider.issuer,
issuer: issuer,
user_uuid: current_user&.uuid,
)

csp_uris = SecureHeadersAllowList.csp_with_sp_redirect_uris(
@authorize_form.redirect_uri,
@authorize_form.service_provider.redirect_uris,
service_provider.redirect_uris,
)
override_form_action_csp(csp_uris)
end
Expand All @@ -172,27 +171,34 @@ def pre_validate_authorize_form
result = @authorize_form.submit

analytics.openid_connect_request_authorization(
**result.to_h.except(:redirect_uri, :code_digest).merge(
**result.to_h.except(:redirect_uri, :code_digest, :integration_errors).merge(
user_fully_authenticated: user_fully_authenticated?,
referer: request.referer,
vtr_param: params[:vtr],
unknown_authn_contexts:,
),
)
return if result.success?

if result.extra[:integration_errors].present?
analytics.sp_integration_errors_present(
**result.to_h[:integration_errors],
)
end

redirect_uri = result.extra[:redirect_uri]

if redirect_uri.nil?
render :error
else
redirect_user(redirect_uri, @authorize_form.service_provider.issuer, current_user&.uuid)
redirect_user(redirect_uri, current_user&.uuid)
end
end

def sign_out_if_prompt_param_is_login_and_user_is_signed_in
if @authorize_form.prompt != 'login'
set_issuer_forced_reauthentication(
issuer: @authorize_form.service_provider.issuer,
issuer:,
is_forced_reauthentication: false,
)
end
Expand All @@ -204,7 +210,7 @@ def sign_out_if_prompt_param_is_login_and_user_is_signed_in
unless sp_session[:request_url] == request.original_url
sign_out
set_issuer_forced_reauthentication(
issuer: @authorize_form.service_provider.issuer,
issuer:,
is_forced_reauthentication: true,
)
end
Expand Down Expand Up @@ -236,8 +242,8 @@ def track_events
track_billing_events
end

def redirect_user(redirect_uri, issuer, user_uuid)
case oidc_redirect_method(issuer: issuer, user_uuid: user_uuid)
def redirect_user(redirect_uri, user_uuid)
case oidc_redirect_method(issuer:, user_uuid: user_uuid)
when 'client_side'
@oidc_redirect_uri = redirect_uri
render(
Expand All @@ -258,6 +264,14 @@ def redirect_user(redirect_uri, issuer, user_uuid)
end
end

def service_provider
@authorize_form.service_provider
end

def issuer
service_provider&.issuer
end

def sp_handoff_bouncer
@sp_handoff_bouncer ||= SpHandoffBouncer.new(sp_session)
end
Expand Down
16 changes: 15 additions & 1 deletion app/controllers/openid_connect/logout_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ def show
if result.success? && redirect_uri
handle_successful_logout_request(result, redirect_uri)
else
track_integration_errors(result:, event: :oidc_logout_requested)

render :error
end
end
Expand All @@ -48,6 +50,8 @@ def delete
if result.success? && redirect_uri
handle_logout(result, redirect_uri)
else
track_integration_errors(result:, event: :oidc_logout_submitted)

render :error
end
end
Expand Down Expand Up @@ -141,11 +145,21 @@ def handle_logout(result, redirect_uri)
# Convert FormResponse into loggable analytics event
# @param [FormResponse] result
def to_event(result)
result.to_h.except(:redirect_uri)
result.to_h.except(:redirect_uri, :integration_errors)
end

def logout_params
params.permit(:client_id, :id_token_hint, :post_logout_redirect_uri, :state)
end

def track_integration_errors(result:, event:)
if result.extra[:integration_errors].present?
analytics.sp_integration_errors_present(
**result.
to_h[:integration_errors].
merge(event:),
)
end
end
end
end
8 changes: 7 additions & 1 deletion app/controllers/openid_connect/token_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,13 @@ def create
analytics_attributes = result.to_h
analytics_attributes[:expires_in] = response[:expires_in]

analytics.openid_connect_token(**analytics_attributes)
analytics.openid_connect_token(**analytics_attributes.except(:integration_errors))

if !result.success? && analytics_attributes[:integration_errors].present?
analytics.sp_integration_errors_present(
**analytics_attributes[:integration_errors],
)
end

render json: response,
status: (result.success? ? :ok : :bad_request)
Expand Down
4 changes: 3 additions & 1 deletion app/controllers/openid_connect/user_info_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ def show
def authenticate_identity_via_bearer_token
verifier = AccessTokenVerifier.new(request.env['HTTP_AUTHORIZATION'])
response, identity = verifier.submit
analytics.openid_connect_bearer_token(**response)
attributes = response.to_h
analytics.openid_connect_bearer_token(**attributes.except(:integration_errors))

if response.success?
@current_identity = identity
else
analytics.sp_integration_errors_present(**attributes[:integration_errors])
render json: { error: verifier.errors[:access_token].join(' ') },
status: :unauthorized
end
Expand Down
20 changes: 17 additions & 3 deletions app/controllers/saml_idp_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,11 @@ def logout

track_logout_event

return head(:bad_request) unless valid_saml_request?
unless valid_saml_request?
track_integration_errors(event: :saml_logout_request)

return head(:bad_request)
end

handle_valid_sp_logout_request
end
Expand All @@ -75,11 +79,21 @@ def remotelogout

track_remote_logout_event(issuer)

return head(:bad_request) unless valid_saml_request?
unless valid_saml_request?
track_integration_errors(event: :saml_remote_logout_request)

return head(:bad_request)
end

user_id = find_user_from_session_index

return head(:bad_request) unless user_id.present?
unless user_id.present?
track_integration_errors(
event: :saml_remote_logout_request,
errors: [:no_user_found_from_session_index],
)
return head(:bad_request)
end

handle_valid_sp_remote_logout_request(user_id: user_id, issuer: issuer)
end
Expand Down
13 changes: 13 additions & 0 deletions app/forms/openid_connect_authorize_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ def extra_analytics_attributes
code_digest: code ? Digest::SHA256.hexdigest(code) : nil,
code_challenge_present: code_challenge.present?,
service_provider_pkce: service_provider&.pkce,
integration_errors:,
}
end

Expand Down Expand Up @@ -350,6 +351,18 @@ def ialmax_requested?
Saml::Idp::Constants::AUTHN_CONTEXT_CLASSREF_TO_IAL[ial_values.sort.max] == 0
end

def integration_errors
return nil if @success || client_id.blank?

{
error_details: errors.full_messages,
error_types: errors.attribute_names,
event: :oidc_request_authorization,
integration_exists: service_provider.present?,
request_issuer: client_id,
}
end

def facial_match_ial_requested?
ial_values.any? { |ial| Saml::Idp::Constants::FACIAL_MATCH_IAL_CONTEXTS.include? ial }
end
Expand Down
11 changes: 11 additions & 0 deletions app/forms/openid_connect_logout_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,16 @@ def id_token_hint_or_client_id_present
)
end

def integration_errors
return nil if valid?
{
error_details: errors.full_messages,
error_types: errors.attribute_names,
integration_exists: service_provider.present?,
request_issuer: client_id || service_provider&.issuer,
}
end

def valid_client_id
return unless client_id.present? && id_token_hint.blank?
return if service_provider.present?
Expand Down Expand Up @@ -144,6 +154,7 @@ def extra_analytics_attributes
redirect_uri: redirect_uri,
sp_initiated: true,
oidc: true,
integration_errors:,
}
end

Expand Down
13 changes: 13 additions & 0 deletions app/forms/openid_connect_token_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,19 @@ def extra_analytics_attributes
code_verifier_present: code_verifier.present?,
service_provider_pkce: service_provider&.pkce,
ial: identity&.ial,
integration_errors:,
}
end

def integration_errors
return nil if valid? || client_id.blank?

{
error_details: errors.full_messages,
error_types: errors.attribute_names,
event: :oidc_token_request,
integration_exists: service_provider.present?,
request_issuer: client_id,
}
end

Expand Down
11 changes: 11 additions & 0 deletions app/services/access_token_verifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ def submit
extra: {
client_id: @identity&.service_provider,
ial: @identity&.ial,
integration_errors:,
},
)

Expand Down Expand Up @@ -69,4 +70,14 @@ def extract_access_token(header)

access_token
end

def integration_errors
{
error_details: errors.full_messages,
error_types: errors.attribute_names,
event: :oidc_bearer_token_auth,
integration_exists: @identity&.service_provider.present?,
request_issuer: @identity&.service_provider,
}
end
end
26 changes: 26 additions & 0 deletions app/services/analytics_events.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6915,6 +6915,32 @@ def sp_inactive_visit
track_event('SP inactive visited')
end

# @param [Array] error_details Full messages of the errors
# @param [Hash] error_types Types of errors that are surfaced
# @param [Symbol] event What part of the workflow the error occured in
# @param [Boolean] integration_exists Whether the requesting issuer maps to an SP
# @param [String] request_issuer The issuer in the request
# Monitoring service-provider specific integration errors
def sp_integration_errors_present(
error_details:,
error_types:,
event:,
integration_exists:,
request_issuer: nil,
**extra
)
types = error_types.index_with { |_type| true }
track_event(
:sp_integration_errors_present,
error_details:,
error_types: types,
event:,
integration_exists:,
request_issuer:,
**extra,
)
end

# Tracks when a user is redirected back to the service provider
# @param [Integer] ial
# @param [Integer] billed_ial
Expand Down
Loading