Move login breach fields into meta record and add password reuse detection for breach alerts #7155
Conversation
jo
force-pushed
the
breach-alert-meta
branch
9 times, most recently
from
b3c471f to
03586ff
Compare
jo
changed the title
jo
force-pushed
the
breach-alert-meta
branch
2 times, most recently
from
0fe7fa6 to
0564fe4
Compare
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
Looks good, just minor comments.
| pub fn are_potentially_vulnerable_passwords(&self, ids: Vec<String>) -> ApiResult<Vec<String>> { | ||
| // Note: Vec<&str> is not supported with UDL, so we receive Vec<String> and convert | ||
| let db = self.lock_db()?; | ||
| let ids: Vec<&str> = ids.iter().map(|id| &**id).collect(); |
There was a problem hiding this comment.
I think you could eliminate the mapping by having the method input &[String]. I'm not sure if it's worth it or not, just wanted to point that out.
There was a problem hiding this comment.
I'd like that (also for delete_many), but how does this work with uniffi, esp. what do I need to specify instead of sequence<string>? If I change above, I get a
error[E0308]: mismatched types
--> /home/jo/moz/application-services/target/debug/build/logins-608812cf418f2f0d/out/logins.uniffi.rs:363:1
|
363 | #[::uniffi::export_for_udl]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&[String]`, found `Vec<String>`
364 | impl r#LoginStore {
365 | pub fn r#delete_many(
| ------------- arguments to this method are incorrect
|
= note: expected reference `&[std::string::String]`
found struct `Vec<std::string::String>`
There was a problem hiding this comment.
I thought that &ids would work, but I'm not totally sure. I'd try that and if it doesn't work stay with the code you have.
| )?), | ||
|
|
||
| 3 => Ok(db.execute_batch(CREATE_LOCAL_BREACHES_TABLE_SQL)?), | ||
|
|
There was a problem hiding this comment.
I would expect this to populate the breachesL table. Maybe you can skip it since the feature hasn't been used in the wild? If so, please add a comment explaining that.
There was a problem hiding this comment.
I don't quite understand 🤔
It doesn't populate the table, it creates it. This changes introduces a new database version, so the table has to be created first, right?
The vulnerable passwords feature already exists on the desktop (this table is stored in a parallel structure within logins.json, in .potentiallyVulnerablePasswords), and in the course of migrating to AS logins, I will also need this feature in AS logins.
There was a problem hiding this comment.
Yes you definitely need to create the table.
I think you might also want to populate the table by iterating through the loginsL table and finding rows that have been flagged as breached. It seems like there's an invariant that's not being upheld. For example:
- Login A is flagged as breached on desktop
- Login A is synced
- This update runs
- Login B is created on mobile with the same password
- Would
is_potentially_vulnerable_passwordreturn true for login B?
Also, I'm now seeing that this isn't hooked up to syncing logic, so maybe the same thing would happen if the update ran before the first step.
Is that analysis correct? If so it seems like we'll need to change the approach somehow. Sorry I missed this in the first review.
There was a problem hiding this comment.
This PR prepares AS logins to store the vulnerable password data only on desktop. This list of breaches is ingested regularly from remote settings and consumed by the desktop itself, and is not synchronized (yet). The list of vulnerable passwords (including historical data) is also maintained by desktop. So the separate table breachesL is intentional, as we need to track historically compromised passwords even after a change by the user.
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
I'm going to re-flag this as requested changes based on the issues raised in my last comment.
Move the previously introduced login breach-related fields `time_of_last_breach` and `time_last_breach_alert_dismissed` from LoginFields into LoginMeta struct, to group internally handled fields which are not directly updateable.
Add breachesL table (schema v4) to track breached passwords and enable cross-domain password reuse detection. New APIs: - are_potentially_vulnerable_passwords(ids) - Batch check, returns GUIDs - is_potentially_vulnerable_password(id) - Single login check Batch API decrypts breachesL once and uses HashSet for efficient lookups (O(M + N) vs O(M * N) for repeated single checks).
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
Thanks for the explanation, I think this is all ready to land.
2 participants
Move the previously introduced login breach-related fields
time_of_last_breachandtime_last_breach_alert_dismissedfrom LoginFields into LoginMeta struct, to group internally handled fields which are not directly updateable.Add breachesL table (schema v4) to track breached passwords and enable cross-domain password reuse detection.
New APIs:
are_potentially_vulnerable_passwords(ids)- Batch check, returns GUIDsis_potentially_vulnerable_password(id)- Single login checkBatch API decrypts breachesL once and uses HashSet for efficient lookups (O(M + N) vs O(M * N) for repeated single checks).
This is the Phabricator patch for Desktop support.
Pull Request checklist
[ci full]to the PR title.