Skip to content

feat: Add clientid to add-clientid-to-securityevents#20138

Closed
clouserw wants to merge 1 commit intomainfrom
add-clientid-to-securityevents
Closed

feat: Add clientid to add-clientid-to-securityevents#20138
clouserw wants to merge 1 commit intomainfrom
add-clientid-to-securityevents

Conversation

@clouserw
Copy link
Member

@clouserw clouserw commented Mar 4, 2026

Because:

  • It would be helpful to see relying parties associated with logs

This commit:

  • Adds the client id when it is available

@clouserw clouserw requested a review from a team as a code owner March 4, 2026 17:08
@clouserw clouserw requested a review from Copilot March 4, 2026 17:08
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds optional client_id attribution to recorded security events so logs/metrics can be correlated with relying parties when the originating service is known and allowed.

Changes:

  • Extend recordSecurityEvent to extract service from request payload/query, validate it against an allowlist (or legacy "sync"), and include it as additionalInfo.client_id.
  • Emit client_id as a StatsD tag for AccountEventsManager.recordSecurityEvent write/error metrics when present.
  • Add/update local tests to cover client_id inclusion/exclusion behavior and metric tagging.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
packages/fxa-auth-server/lib/routes/utils/security-event.ts Extracts/validates service and attaches client_id into security event additionalInfo.
packages/fxa-auth-server/lib/account-events.ts Adds optional client_id to security event additionalInfo type and tags StatsD metrics with it.
packages/fxa-auth-server/test/local/routes/utils/security-event.ts Adds unit tests for recordSecurityEvent client_id allowlist behavior and request source precedence.
packages/fxa-auth-server/test/local/account-events.js Updates/adds tests asserting StatsD tagging behavior for security events with/without client_id.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@clouserw clouserw force-pushed the add-clientid-to-securityevents branch from a115d65 to 20da018 Compare March 4, 2026 17:39
@clouserw clouserw requested a review from Copilot March 4, 2026 17:40
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@clouserw clouserw force-pushed the add-clientid-to-securityevents branch 2 times, most recently from d166d39 to 151ac46 Compare March 4, 2026 18:03
@clouserw clouserw requested a review from Copilot March 4, 2026 18:09
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@clouserw clouserw force-pushed the add-clientid-to-securityevents branch from 151ac46 to bc3c3b0 Compare March 4, 2026 19:54
Because:

* It would be helpful to see relying parties associated with logs

This commit:

* Adds the client id when it is available
@clouserw clouserw force-pushed the add-clientid-to-securityevents branch from bc3c3b0 to 9321dee Compare March 4, 2026 20:34
@dschom
Copy link
Contributor

dschom commented Mar 5, 2026

@clouserw Do you just want this info on security events and statsd metrics reported from security events? Or do you want client_id on logs and statsd metrics in general?

@clouserw
Copy link
Member Author

@dschom I don't remember opening this PR 😬 I thought we filed an issue. But yes, everywhere would be ideal but anything is better than nothing.

@clouserw
Copy link
Member Author

yeah, the real fix for this is #20213 . closing

@clouserw clouserw closed this Mar 18, 2026
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.

3 participants