Unverified Commit 2bd84726 authored by Schmidt's avatar Schmidt Committed by GitHub
Browse files

Bug 2045032 - logins: don't count update() as a password use (#7398) 

Login's `update()` incremented `times_used` and set `time_last_used`
on every edit. Only touch() should update those fields.
parent ef278213

CHANGELOG.md

+6 −0
Original line number Diff line number Diff line 
# v153.0 (In progress)



## 🔧 What's Fixed 🔧



### Logins



- `update()` no longer bumps `times_used` or `time_last_used`, this is done only via `touch()`. Verified that both Firefox Android and iOS already track password use via `touch()` and call `update()` only for explicit edits. ([bug 2045032](https://bugzilla.mozilla.org/show_bug.cgi?id=2045032))



### Nimbus



- Enrollment change events (visible to the UDL) now include the feature IDs of the features involved when possible. ([#7391](https://github.com/mozilla/application-services/pull/7391/))

components/logins/src/db.rs

+45 −4
Original line number Diff line number Diff line
@@ -552,6 +552,7 @@ impl LoginDb { 


    fn update_existing_login(&self, login: &EncryptedLogin) -> Result<()> {

        // assumes the "local overlay" exists, so the guid must too.

        let now_ms = util::system_time_ms_i64(SystemTime::now());

        let sql = format!(

            "UPDATE loginsL

             SET local_modified                           = :now_millis,
@@ -583,8 +584,7 @@ impl LoginDb { 
                ":time_password_changed": login.meta.time_password_changed,

                ":sec_fields": login.sec_fields,

                ":guid": &login.meta.id,

                // time_last_used has been set to now.

                ":now_millis": login.meta.time_last_used,

                ":now_millis": now_ms,

            },

        )?;

        Ok(())
@@ -755,8 +755,9 @@ impl LoginDb { 
                id: existing.id,

                time_created: existing.time_created,

                time_password_changed,

                time_last_used: now_ms,

                times_used: existing.times_used + 1,

                // An edit is not a use (see bug 2045032)

                time_last_used: existing.time_last_used,

                times_used: existing.times_used,

                time_last_breach_alert_dismissed: None,

            },

            fields: LoginFields {
@@ -1956,6 +1957,46 @@ mod tests { 
        assert_eq!(login2.meta.times_used, login.meta.times_used + 1);

    }



    #[test]

    fn test_update_does_not_count_as_use() {

        // A plain update is not a password use.

        // It must not bump `times_used` or `time_last_used`. Only `touch()` is

        // allowed to do that.

        ensure_initialized();

        let db = LoginDb::open_in_memory();

        let login = db

            .add(

                LoginEntry {

                    origin: "https://www.example.com".into(),

                    http_realm: Some("https://www.example.com".into()),

                    username: "user1".into(),

                    password: "password1".into(),

                    ..Default::default()

                },

                &*TEST_ENCDEC,

            )

            .unwrap();

        // Make sure the "now" an update would use differs from the add time.

        thread::sleep(time::Duration::from_millis(50));

        db.update(

            &login.meta.id,

            LoginEntry {

                origin: "https://www.example.com".into(),

                http_realm: Some("https://www.example.com".into()),

                username: "user1".into(),

                password: "password2".into(),

                ..Default::default()

            },

            &*TEST_ENCDEC,

        )

        .unwrap();

        let updated = db.get_by_id(&login.meta.id).unwrap().unwrap();

        // An edit is not a use: times_used must stay unchanged.

        assert_eq!(updated.meta.times_used, login.meta.times_used);

        // An edit is not a use: time_last_used must stay unchanged.

        assert_eq!(updated.meta.time_last_used, login.meta.time_last_used);

    }



    #[test]

    fn test_breach_alert_dismissal() {

        ensure_initialized();

components/logins/src/store.rs

+3 −3
Original line number Diff line number Diff line
@@ -499,9 +499,9 @@ mod tests { 
        assert!(b_after_update.time_created >= start_us);

        assert!(b_after_update.time_created <= now_us);

        assert!(b_after_update.time_password_changed >= now_us);

        assert!(b_after_update.time_last_used >= now_us);

        // Should be two even though we updated twice

        assert_eq!(b_after_update.times_used, 2);

        // An edit is not a use: usage stats are unchanged by update().

        assert_eq!(b_after_update.time_last_used, b_from_db.time_last_used);

        assert_eq!(b_after_update.times_used, 1);

    }



    #[test]