Skip to content

Conversation

@jo
Copy link
Contributor

@jo jo commented Dec 12, 2025

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 Login properties

  • time_of_last_breach
    System time in milliseconds corresponding to the breach’s BreachDate.

  • time_last_breach_alert_dismissed
    System time in milliseconds indicating when the user dismissed the breach alert.

Additional LoginStore API methods

  • record_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_breach to null).

  • 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

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

@jo jo force-pushed the breach-alerts branch 12 times, most recently from 65b3c0b to 1924a2e Compare December 18, 2025 13:22
@jo jo changed the title WIP Logins: breach alert dismissal support Logins: Breach Alerts Dec 18, 2025
jo added 2 commits December 19, 2025 09:38
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`.
@jo jo requested review from bendk and groovecoder December 19, 2025 09:11
Copy link
Contributor

@bendk bendk left a 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();
Copy link
Contributor

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:

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",
Copy link
Contributor

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.

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