Skip to content

bug(securityEvent): Add missing security event names to db#19483

Merged
nshirley merged 1 commit intomainfrom
bug/missing-migrations
Sep 22, 2025
Merged

bug(securityEvent): Add missing security event names to db#19483
nshirley merged 1 commit intomainfrom
bug/missing-migrations

Conversation

@nshirley
Copy link
Copy Markdown
Contributor

@nshirley nshirley commented Sep 19, 2025

Because:

  • We're missing some security event names from some events in the db
  • And we're missing some security event names in the model
  • And some names have mismatching id

This commit:

  • Updates all security event names in 'fxa.securityEventNames' that are missing
  • Aligns all values on the model to match the db as source of truth

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

@nshirley nshirley force-pushed the bug/missing-migrations branch from 3937b97 to 877378f Compare September 19, 2025 16:08
'account.recovery_codes_created': 33,
'account.recovery_codes_signin_complete': 34,
'account.must_reset': 35,
'account.recovery_phone_reset_password_success': 36,
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.

This was not found in code, but the new recovery_phone_reset_password_complete is.

'account.password_upgrade_success': 38,
'account.password_upgraded': 39,
'account.recovery_phone_setup_failed': 40,
'account.recovery_phone_replace_failed': 41,
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.

here, _failed is what was in code, but the db had it as _failure. I'm unsure if we should keep the db as the source of truth or update it to match the model here so I went with the former given the comment at the top

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.

I was just digging into how events are written and, since they're written with whatever name is passed to recordSecurityEvent then this is probably not a great idea to change. We'd have some events with the old _failed and then all new events would have the _failure name.

Maybe it makes more sense to update the event name in the db instead of code/model?

Because:
 - We're missing some security event names from some events in the db
 - And we're missing some security event names in the model
 - And some names have mismatching id

This commit:
 - Updates all security event names in 'fxa.securityEventNames' that are
   missing
 - Aligns all values on the model to match the db as source of truth
@nshirley nshirley force-pushed the bug/missing-migrations branch from 877378f to 9f7d150 Compare September 19, 2025 16:15
@nshirley
Copy link
Copy Markdown
Contributor Author

I also noticed that several of the recent values are missing from the PageRecentActivity/SecurityEvent.tsx mapping to display them. I see there's L10n values in there - how do I go about getting those that are missing added so we don't just show Other account activity on them?

@nshirley
Copy link
Copy Markdown
Contributor Author

I also found that the securityEvents table records the event name as the ID so changing the ID's in the model is probably not a good idea 🤔 I'll see if I can dig in further to find the best solution that won't require updating the securityEvents table

@nshirley
Copy link
Copy Markdown
Contributor Author

Okay, after a bit more review of how securityEvents are recorded, I think this is a safe change. Since we look up the nameId from EVENT_NAMES with bracket notation, we'll get undefined if the name doesn't exist on the object. That will fail to write since there's a FK constraint on the table (this also prevents writing of IDs beyond what is in the table)

So, we've just dropped these events that don't exist/map correctly. This should fix that!

@nshirley nshirley marked this pull request as ready for review September 22, 2025 15:24
@nshirley nshirley requested a review from a team as a code owner September 22, 2025 15:24
Copy link
Copy Markdown
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

:shipit:

@nshirley nshirley merged commit f442efe into main Sep 22, 2025
19 checks passed
@nshirley nshirley deleted the bug/missing-migrations branch September 22, 2025 16:10
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