Skip to content

LG 14123 Include associated user_id in event disavowal CloudWatch logs #11140

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

Merged
kevinsmaster5 merged 9 commits into from
Aug 27, 2024
Merged

LG 14123 Include associated user_id in event disavowal CloudWatch logs #11140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8e30343
changelog: Internal, Fraud prevention, include associated user_id in …
kevinsmaster5 Aug 22, 2024
c6f1543
change user.id to use associated user_id
kevinsmaster5 Aug 23, 2024
273dd38
move user_id catching to the build_disavowal class
kevinsmaster5 Aug 26, 2024
b0f8fdb
add user_id documentation to event
kevinsmaster5 Aug 26, 2024
73289ac
fix lint
kevinsmaster5 Aug 26, 2024
5df4904
pass user id as uuid
kevinsmaster5 Aug 27, 2024
fa5fe08
update specs to use uuid, add nil safe for disavowal error events
kevinsmaster5 Aug 27, 2024
2796413
remove nil default
kevinsmaster5 Aug 27, 2024
a8d8661
move not-optional keywords
kevinsmaster5 Aug 27, 2024
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
6 changes: 6 additions & 0 deletions app/services/analytics_events.rb
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@aduth aduth Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Aug 26, 2024

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@aduth aduth Aug 26, 2024

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@aduth aduth Aug 26, 2024

Choose a reason for hiding this comment

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

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 👍

Original file line number Diff line number Diff line change
Expand Up @@ -521,10 +521,12 @@ def email_sent(action:, ses_message_id:, email_address_id:, **extra)
# @param [Integer, nil] event_id events table id
# @param [String, nil] event_type (see Event#event_type)
# @param [String, nil] event_ip ip address for the event
# @param [String, nil] user_id UUID of the user
# Tracks disavowed event
def event_disavowal(
success:,
errors:,
user_id:,
error_details: nil,
event_created_at: nil,
disavowed_device_last_used_at: nil,
Expand All @@ -547,6 +549,7 @@ def event_disavowal(
event_id:,
event_type:,
event_ip:,
user_id:,
**extra,
)
end
Expand All @@ -561,10 +564,12 @@ def event_disavowal(
# @param [Integer, nil] event_id events table id
# @param [String, nil] event_type (see Event#event_type)
# @param [String, nil] event_ip ip address for the event
# @param [String, nil] user_id UUID of the user
# Event disavowal password reset was performed
def event_disavowal_password_reset(
success:,
errors:,
user_id:,
error_details: nil,
event_created_at: nil,
disavowed_device_last_used_at: nil,
Expand All @@ -587,6 +592,7 @@ def event_disavowal_password_reset(
event_id:,
event_type:,
event_ip:,
user_id:,
**extra,
)
end
Expand Down
1 change: 1 addition & 0 deletions app/services/event_disavowal/build_disavowed_event_analytics_attributes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ def self.call(event)
event_type: event.event_type,
event_created_at: event.created_at,
event_ip: event.ip,
user_id: event.user&.uuid,
disavowed_device_user_agent: device&.user_agent,
disavowed_device_last_ip: device&.last_ip,
disavowed_device_last_used_at: device&.last_used_at,
Expand Down
9 changes: 5 additions & 4 deletions spec/controllers/event_disavowal_controller_spec.rb
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@aduth aduth Aug 26, 2024

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

expect(@analytics).to have_logged_event(
'Event disavowal visited',
build_analytics_hash,
build_analytics_hash(user_id: event.user.uuid),
)
end

Expand All @@ -31,7 +31,7 @@

expect(@analytics).to have_logged_event(
'Event disavowal visited',
build_analytics_hash,
build_analytics_hash(user_id: event.user.uuid),
)
expect(assigns(:forbidden_passwords)).to all(be_a(String))
end
Expand Down Expand Up @@ -79,7 +79,7 @@

expect(@analytics).to have_logged_event(
'Event disavowal password reset',
build_analytics_hash,
build_analytics_hash(user_id: event.user.uuid),
)
end
end
Expand Down Expand Up @@ -172,7 +172,7 @@
end
end

def build_analytics_hash(success: true, errors: {})
def build_analytics_hash(success: true, errors: {}, user_id: nil)
hash_including(
{
event_created_at: event.created_at,
Expand All @@ -184,6 +184,7 @@ def build_analytics_hash(success: true, errors: {})
event_ip: event.ip,
disavowed_device_user_agent: event.device.user_agent,
disavowed_device_last_ip: event.device.last_ip,
user_id: user_id,
}.compact,
)
end
Expand Down