LG 14123 Include associated user_id in event disavowal CloudWatch logs by kevinsmaster5 · Pull Request #11140 · 18F/identity-idp

kevinsmaster5 · 2024-08-22T19:58:37Z

🎫 Ticket

Link to the relevant ticket:
LG-14123

🛠 Summary of changes

Passes the disavowed_event.user.id value along to the respective Analytics events.

📜 Testing Plan

In a console run make watch_events
Login and MFA as a user in a private browser window to trigger the New Device email
After receiving the new device notification click to reset password there
Observe in the watch_events feed "Event disavowal visited" and "Event disavowal password reset" will show the user_id

👀 Screenshots

app/controllers/event_disavowal_controller.rb

…event disavowal

aduth · 2024-08-26T15:55:56Z

app/services/analytics_events.rb

I don't think we want any changes in this file. user_id is not meant to be interpreted as a parameter for an individual analytics event, it's available for every analytics event (as properties.user_id). Properties documented in this file are logged as properties.event_properties.[property_name], which is not how we'd want user_id to be logged.

I can't tell, but if this is a logged-out event, passing in user_id like that makes sense because user.uuid would be anonymous-uuid or something

I think it makes sense to pass it, but I didn't think we'd want to document it here.

That being said, I did a quick audit of if we document it for other methods. It looks like we're not consistent, but document it more often than we don't. (Inconsistency expected since we don't enforce it)

Documented:

remote_logout_completed

user_registration_email_confirmation

email_and_password_auth

openid_connect_token

password_creation

security_event_received

idv_doc_auth_submitted_image_upload_form

idv_gpo_expired

Undocumented:

add_email_request

password_reset_email

password_changed

idv_doc_auth_failed_image_resubmitted

Also, I'd guess if we ever wanted to get rid of our **extra handling, we'd need to explicitly list all the keyword arguments the method could receive.

tl;dr These changes make sense after all 👍

aduth · 2024-08-26T15:56:55Z

spec/controllers/event_disavowal_controller_spec.rb

If you have issues in this spec file after making the changes in my previous comment, I'd consider looking at using the user argument of stub_analytics, which was implemented to support asserting a specific user being associated with the logged event.

aduth · 2024-08-26T15:58:58Z

app/controllers/event_disavowal_controller.rb

      extra: EventDisavowal::BuildDisavowedEventAnalyticsAttributes.call(disavowed_event),
    )
-    analytics.event_disavowal(**result.to_h)
+    analytics.event_disavowal(user_id: disavowed_event.user_id, **result.to_h)


Should we implement this in BuildDisavowedEventAnalyticsAttributes instead? That service class pattern is not very conventional, but at least it gives us some consistency in how we build the analytics hash, and should require only updating in one place (vs. two here).

That seems to work great.
In regards to above, should user_id be documented in analytics_events? I took it out.

I think based on discussion it makes sense to keep the documentation.

aduth · 2024-08-27T11:54:08Z

app/controllers/event_disavowal_controller.rb

  def create
    result = password_reset_from_disavowal_form.submit(password_reset_params)
-    analytics.event_disavowal_password_reset(**result.to_h)
+    analytics.event_disavowal_password_reset(user_id: disavowed_event.user_id, **result.to_h)


PasswordResetFromDisavowalForm internally uses BuildDisavowedEventAnalyticsAttributes for its extra attributes, so we don't need to include this here again. One of the nice things about reusing BuildDisavowedEventAnalyticsAttributes.

Suggested change

analytics.event_disavowal_password_reset(user_id: disavowed_event.user_id, **result.to_h)

analytics.event_disavowal_password_reset(**result.to_h)

aduth · 2024-08-27T11:56:42Z

app/services/event_disavowal/build_disavowed_event_analytics_attributes.rb

        event_type: event.event_type,
        event_created_at: event.created_at,
        event_ip: event.ip,
+        user_id: event.user_id,


I missed it before, but confusingly user_id in the context of the Analytics class is actually referring to the user's UUID (a string), not the internal ID.

So we'd want:

Suggested change

user_id: event.user_id,

user_id: event.user.uuid,

(And update documentation to refer to the value being a String, not an Int)

Yes. The test caught that. Updated it

aduth

Couple comments, but tested and works well, LGTM 👍

aduth · 2024-08-27T12:35:42Z

app/services/analytics_events.rb

    event_id: nil,
    event_type: nil,
    event_ip: nil,
+    user_id: nil,


Minor: I'm generally not a fan of nil defaults in the analytics keyword arguments unless there's a legitimate case for arguments not always being passed. Since we know that all of our callsites will pass user_id, and all of those user IDs will not nil, we should avoid the default, and document it as always being a string.

aduth · 2024-08-27T12:38:22Z

spec/controllers/event_disavowal_controller_spec.rb

        expect(@analytics).to have_logged_event(
          'Event disavowal visited',
-          build_analytics_hash,
+          build_analytics_hash(user_id: event.user_id),


Hopefully the build will catch it, but we need to update these spec assertions to reference event.user.uuid as well.

Suggested change

build_analytics_hash(user_id: event.user_id),

build_analytics_hash(user_id: event.user.uuid),

aduth reviewed Aug 23, 2024

View reviewed changes

app/controllers/event_disavowal_controller.rb Outdated Show resolved Hide resolved

kevinsmaster5 added 2 commits August 23, 2024 08:50

changelog: Internal, Fraud prevention, include associated user_id in …

8e30343

…event disavowal

change user.id to use associated user_id

c6f1543

kevinsmaster5 force-pushed the kmas-lg-14123-include-user-id-event-disavowal branch from 63cac4f to c6f1543 Compare August 23, 2024 12:50

kevinsmaster5 marked this pull request as ready for review August 23, 2024 13:27

kevinsmaster5 requested a review from a team August 23, 2024 13:27

aduth reviewed Aug 26, 2024

View reviewed changes

kevinsmaster5 added 3 commits August 26, 2024 16:33

move user_id catching to the build_disavowal class

273dd38

add user_id documentation to event

b0f8fdb

fix lint

73289ac

kevinsmaster5 requested review from a team and aduth August 26, 2024 21:29

aduth reviewed Aug 27, 2024

View reviewed changes

pass user id as uuid

5df4904

aduth approved these changes Aug 27, 2024

View reviewed changes

kevinsmaster5 added 3 commits August 27, 2024 09:07

update specs to use uuid, add nil safe for disavowal error events

fa5fe08

remove nil default

2796413

move not-optional keywords

a8d8661

kevinsmaster5 merged commit 4d02b06 into main Aug 27, 2024

kevinsmaster5 deleted the kmas-lg-14123-include-user-id-event-disavowal branch August 27, 2024 14:28

jmhooper mentioned this pull request Aug 27, 2024

Deploy RC 409 to Production #11151

Merged

	analytics.event_disavowal_password_reset(user_id: disavowed_event.user_id, **result.to_h)
	analytics.event_disavowal_password_reset(**result.to_h)

	build_analytics_hash(user_id: event.user_id),
	build_analytics_hash(user_id: event.user.uuid),

Conversation

kevinsmaster5 commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

Uh oh!

Uh oh!

aduth Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevinsmaster5 commented Aug 22, 2024 •

edited

Loading

aduth Aug 26, 2024 •

edited

Loading