-
Notifications
You must be signed in to change notification settings - Fork 251
Logins: Breach Alerts #7127
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.
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
base: main
Are you sure you want to change the base?
Logins: Breach Alerts #7127
Conversation
65b3c0b to
1924a2e
Compare
Extend the Logins API with breach-related metadata and queries, migrate the database to version 3, and add primitives to record breaches, evaluate potential exposure, and track alert dismissals.
and ignore the enum size warning for the `BulkResultEntry`.
bendk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I had a couple suggestions, but nothing blocking. I don't need to re-review.
| db.execute_batch("SELECT timeLastBreachAlertDismissed FROM loginsL") | ||
| .unwrap(); | ||
| db.execute_batch("SELECT timeLastBreachAlertDismissed FROM loginsM") | ||
| .unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now's probably a good time to add a schema upgrade test like the one suggest has:
application-services/components/suggest/src/schema.rs
Lines 1013 to 1018 in eea8d80
| fn test_all_upgrades() { | |
| let db_file = | |
| MigratedDatabaseFile::new(SuggestConnectionInitializer::default(), V16_SCHEMA); | |
| db_file.run_all_upgrades(); | |
| db_file.assert_schema_matches_new_database(); | |
| } |
It should be fairly easy, the only real work is finding an older version of the schema to use as the base. We could just use v2 for that.
| "UPDATE loginsL | ||
| SET timeOfLastBreach = :now_millis | ||
| WHERE guid = :guid | ||
| AND is_deleted = 0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the is_deleted = 0 part could be removed. If somehow a deleted login gets merged with a non-deleted login and the merge code decides to go with the non-deleted version, then maybe we'd want to merge the timeOfLastBreach value. That's a lot of "ifs" and "maybes". I can't think of a real scenario where that would happen, but I can't think of any downsides either. I trust your judgement on this one, just wanted to flag it.
This PR adds Breach Alert support to the Logins component by extending the Logins API and introducing breach-related metadata and queries. It includes a database migration to version 3 and provides the necessary primitives for consumers (e.g. Firefox Desktop) to record breaches, evaluate potential exposure, and track alert dismissals.
New
Loginpropertiestime_of_last_breachSystem time in milliseconds corresponding to the breach’s
BreachDate.time_last_breach_alert_dismissedSystem time in milliseconds indicating when the user dismissed the breach alert.
Additional
LoginStoreAPI methodsrecord_breach(id, timestamp)Stores a known breach date for a login.
In Firefox Desktop this is updated once per session from Remote Settings.
reset_all_breaches()Removes all recorded breaches for all logins (i.e. sets
time_of_last_breachtonull).is_potentially_breached(id)Determines whether a login’s password is potentially breached, based on the breach date and the time of the last password change.
record_breach_alert_dismissal(id)Stores the time at which the user dismissed the breach alert for a login.
is_breach_alert_dismissed(id)Determines whether a breach alert has been dismissed, based on the breach date and the alert dismissal timestamp.
Pull Request checklist
[ci full]to the PR title.