Skip to content

Remove sp_request_requested_attributes from completion events#10737

Merged
aduth merged 1 commit intomainfrom
aduth-rm-sp-request-requested-attributes
May 31, 2024
Merged

Remove sp_request_requested_attributes from completion events#10737
aduth merged 1 commit intomainfrom
aduth-rm-sp-request-requested-attributes

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented May 31, 2024

🛠 Summary of changes

Removes sp_request_requested_attributes from 'User registration: complete' and 'User registration: agency handoff visited' analytics events.

These values are never assigned (see #10736 (comment)), and are prone to confusion with the sp_session_requested_attributes analytics property.

Previous discussion: #10736 (comment)

📜 Testing Plan

Verify that this property is never logged in production analytics.

changelog: Internal, Analytics, Remove sp_request_requested_attributes from completion events
@aduth aduth requested a review from zachmargolis May 31, 2024 18:55
@@ -84,7 +84,6 @@ def analytics_attributes(page_occurence)
ialmax: resolved_authn_context_result.ialmax?,
service_provider_name: decorated_sp_session.sp_name,
sp_session_requested_attributes: sp_session[:requested_attributes],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to sift through the layers of abstraction, we could also do something like this, which still indirectly falls back to service_provider_request.requested_attributes (source):

Suggested change
sp_session_requested_attributes: sp_session[:requested_attributes],
sp_requested_attributes: decorated_sp_session.requested_attributes,

Still trying to understand if we really need service_provider_request at all though...

ialmax: resolved_authn_context_result.ialmax?,
service_provider_name: decorated_sp_session.sp_name,
sp_session_requested_attributes: sp_session[:requested_attributes],
sp_request_requested_attributes: service_provider_request.requested_attributes,
Copy link
Contributor

Choose a reason for hiding this comment

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

tracing this back looks like it should come from

requested_attributes: protocol.requested_attributes,

which comes from either

def requested_attributes
OpenidConnectAttributeScoper.new(request.scope).requested_attributes
end

or

def requested_attributes
@requested_attributes ||= SamlRequestedAttributesPresenter.new(
service_provider: current_service_provider,
ial: ial,
vtr: vtr,
authn_request_attribute_bundle: SamlRequestParser.new(request).requested_attributes,
).requested_attributes
end

which don't seem like they should be as empty as they are?

Copy link
Contributor Author

@aduth aduth May 31, 2024

Choose a reason for hiding this comment

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

The problem is ApplicationController#service_provider_request will always be NullServiceProviderRequest, because uuid is blank since there is no request_id query parameter at this controller.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh that explains a lot

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a next PR could clean up that helper? I know in at least one place use do pass request_id as a URL param for emails, but maybe it shouldn't be in a shared controller like it is now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's part of the trickiness, since there are a few places we do expect request_id query parameter to be supported (the entrypoints like password reset, sign-in), so we might rely on it there. But I'd think the way it should work is that we immediately store it all in the session and then refer to sp_session from that point forward.

@aduth aduth merged commit 6898b78 into main May 31, 2024
@aduth aduth deleted the aduth-rm-sp-request-requested-attributes branch May 31, 2024 19:31
@aduth aduth mentioned this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants