Handle duplicate logins on insertion/update

CHANGES_UNRELEASED.md

@@ -3,3 +3,9 @@
 # Unreleased Changes
 
 [Full Changelog](https://github.com/mozilla/application-services/compare/v0.43.1...master)
+
+## Logins
+
+### What's new
+
+- Added ability to prevent insertion/updates from creating dupes via `LoginsStorage.ensureValid`. ([#2101](https://github.com/mozilla/application-services/pull/2101))

components/logins/android/src/main/java/mozilla/appservices/logins/DatabaseLoginsStorage.kt

@@ -207,6 +207,14 @@ class DatabaseLoginsStorage(private val dbPath: String) : AutoCloseable, LoginsS
         }
     }
 
+    @Throws(InvalidRecordException::class)
+    override fun ensureValid(login: ServerPassword) {
+        val s = login.toJSON().toString()
+        rustCallWithLock { raw, error ->
+            PasswordSyncAdapter.INSTANCE.sync15_passwords_check_valid(raw, s, error)
+        }
+    }
+
     @Synchronized
     @Throws(LoginsStorageException::class)
     override fun close() {

components/logins/android/src/main/java/mozilla/appservices/logins/LoginsStorage.kt

@@ -219,4 +219,20 @@ interface LoginsStorage : AutoCloseable {
      * Note: handles do not remain valid after locking / unlocking the logins database.
      */
     fun getHandle(): Long
+
+    /**
+     * Checks if login already exists and is valid. Throws a [InvalidRecordException] if it is not.
+     *
+     * ```
+     * try {
+     *     db.ensureValid(record)
+     * } catch (e: InvalidRecordException) {
+     *     // The reason the record is invalid is stored in `e.reason`.
+     * }
+     * ```
+     *
+     * @throws [InvalidRecordException] On unexpected errors (IO failure, rust panics, etc)
+     */
+    @Throws(InvalidRecordException::class)
+    fun ensureValid(login: ServerPassword)
 }

components/logins/android/src/main/java/mozilla/appservices/logins/LoginsStorageException.kt

@@ -31,13 +31,10 @@ class IdCollisionException(msg: String) : LoginsStorageException(msg)
 
 /**
  * This is thrown on attempts to insert or update a record so that it
- * is no longer valid. Valid records have:
- *
- * - non-empty hostnames
- * - non-empty passwords
- * - and exactly one of `httpRealm` or `formSubmitUrl` is non-null.
+ * is no longer valid. See [InvalidLoginReason] for a list of reasons
+ * a record may be considered invalid
  */
-class InvalidRecordException(msg: String) : LoginsStorageException(msg)
+class InvalidRecordException(msg: String, public val reason: InvalidLoginReason) : LoginsStorageException(msg)
 
 /**
  * This error is emitted in two cases:
@@ -56,3 +53,19 @@ class RequestFailedException(msg: String) : LoginsStorageException(msg)
  * This error is emitted if a sync or other operation is interrupted.
  */
 class InterruptedException(msg: String) : LoginsStorageException(msg)
+
+/**
+ * A reason a login may be invalid
+ */
+enum class InvalidLoginReason {
+    /** Hostnames may not be empty */
+    EMPTY_HOSTNAME,
+    /** Passwords may not be empty */
+    EMPTY_PASSWORD,
+    /** The login already exists */
+    DUPLICATE_LOGIN,
+    /** Both `httpRealm` and `formSubmitUrl` are non-null */
+    BOTH_TARGETS,
+    /** Both `httpRealm` and `formSubmitUrl` are null */
+    NO_TARGET,
+}

components/logins/android/src/main/java/mozilla/appservices/logins/MemoryLoginsStorage.kt

@@ -171,7 +171,7 @@ class MemoryLoginsStorage(private var list: List<ServerPassword>) : AutoCloseabl
             timePasswordChanged = System.currentTimeMillis()
         )
 
-        checkValid(toInsert)
+        checkValidWithNoDupes(toInsert)
 
         val sp = list.find { it.id == toInsert.id }
         if (sp != null) {
@@ -191,7 +191,7 @@ class MemoryLoginsStorage(private var list: List<ServerPassword>) : AutoCloseabl
         for (login in logins) {
             val toInsert = login.copy(id = UUID.randomUUID().toString())
             try {
-                checkValid(toInsert)
+                checkValidWithNoDupes(toInsert)
                 list += toInsert
             } catch (e: InvalidRecordException) {
                 numErrors += 1
@@ -217,7 +217,7 @@ class MemoryLoginsStorage(private var list: List<ServerPassword>) : AutoCloseabl
                     System.currentTimeMillis()
                 })
 
-        checkValid(newRecord)
+        checkValidWithNoDupes(newRecord)
 
         list = list.filter { it.id != login.id }
 
@@ -256,18 +256,45 @@ class MemoryLoginsStorage(private var list: List<ServerPassword>) : AutoCloseabl
     @Suppress("ThrowsCount")
     private fun checkValid(login: ServerPassword) {
         if (login.hostname == "") {
-            throw InvalidRecordException("Invalid login: Hostname is empty")
+            throw InvalidRecordException("Invalid login: Hostname is empty", InvalidLoginReason.EMPTY_HOSTNAME)
         }
         if (login.password == "") {
-            throw InvalidRecordException("Invalid login: Password is empty")
+            throw InvalidRecordException("Invalid login: Password is empty", InvalidLoginReason.EMPTY_PASSWORD)
         }
         if (login.formSubmitURL != null && login.httpRealm != null) {
             throw InvalidRecordException(
-                    "Invalid login: Both `formSubmitUrl` and `httpRealm` are present")
+                    "Invalid login: Both `formSubmitUrl` and `httpRealm` are present",
+                    InvalidLoginReason.BOTH_TARGETS)
         }
         if (login.formSubmitURL == null && login.httpRealm == null) {
             throw InvalidRecordException(
-                    "Invalid login: Neither `formSubmitUrl` and `httpRealm` are present")
+                    "Invalid login: Neither `formSubmitUrl` and `httpRealm` are present",
+                    InvalidLoginReason.NO_TARGET)
+        }
+    }
+
+    override fun ensureValid(login: ServerPassword) {
+        checkValidWithNoDupes(login)
+    }
+
+    @Suppress("ThrowsCount")
+    private fun checkValidWithNoDupes(login: ServerPassword) {
+        checkValid(login)
+
+        val hasDupe = list.any {
+            it.id != login.id &&
+            it.hostname == login.hostname &&
+            it.username == login.username &&
+            (
+                it.formSubmitURL == login.formSubmitURL ||
+                it.httpRealm == login.httpRealm
+            )
+        }
+
+        if (hasDupe) {
+            throw InvalidRecordException(
+                    "Invalid login: Login already exists",
+                    InvalidLoginReason.DUPLICATE_LOGIN)
         }
     }
 }

components/logins/android/src/main/java/mozilla/appservices/logins/rust/PasswordSyncAdapter.kt

@@ -61,6 +61,9 @@ internal interface PasswordSyncAdapter : Library {
     fun sync15_passwords_reset(handle: LoginsDbHandle, error: RustError.ByReference)
 
     fun sync15_passwords_touch(handle: LoginsDbHandle, id: String, error: RustError.ByReference)
+
+    fun sync15_passwords_check_valid(handle: LoginsDbHandle, json: String, error: RustError.ByReference)
+
     // This is 1 for true and 0 for false, it would be a boolean but we need to return a value with
     // a known size.
     fun sync15_passwords_delete(handle: LoginsDbHandle, id: String, error: RustError.ByReference): Byte

components/logins/android/src/main/java/mozilla/appservices/logins/rust/RustError.kt

@@ -9,6 +9,7 @@ import com.sun.jna.Structure
 import mozilla.appservices.logins.IdCollisionException
 import mozilla.appservices.logins.InvalidKeyException
 import mozilla.appservices.logins.InvalidRecordException
+import mozilla.appservices.logins.InvalidLoginReason
 import mozilla.appservices.logins.LoginsStorageException
 import mozilla.appservices.logins.NoSuchRecordException
 import mozilla.appservices.logins.RequestFailedException
@@ -47,10 +48,16 @@ open class RustError : Structure() {
             1 -> return SyncAuthInvalidException(message)
             2 -> return NoSuchRecordException(message)
             3 -> return IdCollisionException(message)
-            4 -> return InvalidRecordException(message)
-            5 -> return InvalidKeyException(message)
-            6 -> return RequestFailedException(message)
-            7 -> return InterruptedException(message)
+            4 -> return InvalidKeyException(message)
+            5 -> return RequestFailedException(message)
+            6 -> return InterruptedException(message)
+
+            64 -> return InvalidRecordException(message, InvalidLoginReason.EMPTY_HOSTNAME)
+            65 -> return InvalidRecordException(message, InvalidLoginReason.EMPTY_PASSWORD)
+            66 -> return InvalidRecordException(message, InvalidLoginReason.DUPLICATE_LOGIN)
+            67 -> return InvalidRecordException(message, InvalidLoginReason.BOTH_TARGETS)
+            68 -> return InvalidRecordException(message, InvalidLoginReason.NO_TARGET)
+
             else -> return LoginsStorageException(message)
         }
     }

components/logins/android/src/test/java/mozilla/appservices/logins/LoginsStorageTest.kt

@@ -226,6 +226,32 @@ abstract class LoginsStorageTest {
         finishAndClose(test)
     }
 
+    @Test
+    fun testEnsureValid() {
+        val test = getTestStore()
+        test.unlock(encryptionKey)
+
+        test.add(ServerPassword(
+                id = "bbbbb",
+                hostname = "https://www.foo.org",
+                httpRealm = "Some Realm",
+                password = "MyPassword",
+                username = "MyUsername"))
+
+        val dupeLogin = ServerPassword(
+                id = "",
+                hostname = "https://www.foo.org",
+                httpRealm = "Some Realm",
+                password = "MyPassword",
+                username = "MyUsername")
+
+        expectException(InvalidRecordException::class.java) {
+            test.ensureValid(dupeLogin)
+        }
+
+        test.delete("bbbbb")
+    }
+
     @Test
     fun testUpdate() {
         val test = getTestStore()

components/logins/ffi/src/lib.rs

@@ -129,6 +129,19 @@ pub extern "C" fn sync15_passwords_touch(handle: u64, id: FfiStr<'_>, error: &mu
     })
 }
 
+#[no_mangle]
+pub extern "C" fn sync15_passwords_check_valid(
+    handle: u64,
+    record_json: FfiStr<'_>,
+    error: &mut ExternError,
+) {
+    log::debug!("sync15_passwords_check_valid");
+    ENGINES.call_with_result(error, handle, |state| {
+        let parsed: Login = serde_json::from_str(record_json.as_str())?;
+        state.lock().unwrap().check_valid_with_no_dupes(&parsed)
+    })
+}
+
 #[no_mangle]
 pub extern "C" fn sync15_passwords_delete(
     handle: u64,

components/logins/ios/Logins/Errors/LoginStoreError.swift

@@ -25,12 +25,8 @@ public enum LoginsStoreError: LocalizedError {
     case duplicateGuid(message: String)
 
     /// This is thrown on attempts to insert or update a record so that it
-    /// is no longer valid. Valid records have:
-    ///
-    /// - non-empty hostnames
-    /// - non-empty passwords
-    /// - and exactly one of `httpRealm` or `formSubmitUrl` is non-null.
-    case invalidLogin(message: String)
+    /// is no longer valid. See InvalidLoginReason for list of reasons.
+    case invalidLogin(message: String, reason: InvalidLoginReason)
 
     /// This error is emitted in two cases:
     ///
@@ -94,8 +90,20 @@ public enum LoginsStoreError: LocalizedError {
         case Sync15Passwords_DuplicateGuid:
             return .duplicateGuid(message: String(freeingRustString: message!))
 
-        case Sync15Passwords_InvalidLogin:
-            return .invalidLogin(message: String(freeingRustString: message!))
+        case Sync15Passwords_InvalidLogin_EmptyHostname:
+            return .invalidLogin(message: String(freeingRustString: message!), reason: .emptyHostname)
+
+        case Sync15Passwords_InvalidLogin_EmptyPassword:
+            return .invalidLogin(message: String(freeingRustString: message!), reason: .emptyPassword)
+
+        case Sync15Passwords_InvalidLogin_DuplicateLogin:
+            return .invalidLogin(message: String(freeingRustString: message!), reason: .duplicateLogin)
+
+        case Sync15Passwords_InvalidLogin_BothTargets:
+            return .invalidLogin(message: String(freeingRustString: message!), reason: .bothTargets)
+
+        case Sync15Passwords_InvalidLogin_NoTarget:
+            return .invalidLogin(message: String(freeingRustString: message!), reason: .noTarget)
 
         case Sync15Passwords_InvalidKeyError:
             return .invalidKey(message: String(freeingRustString: message!))
@@ -145,3 +153,12 @@ public enum LoginsStoreError: LocalizedError {
         return result
     }
 }
+
+/// Indicates a record is invalid
+public enum InvalidLoginReason {
+    case emptyHostname
+    case emptyPassword
+    case duplicateLogin
+    case bothTargets
+    case noTarget
+}

components/logins/ios/Logins/LoginsStorage.swift

@@ -205,6 +205,17 @@ open class LoginsStorage {
         }
     }
 
+    /// Ensure that the record is valid and a duplicate record doesn't exist.
+    open func ensureValid(login: LoginRecord) throws {
+        let json = try login.toJSON()
+        return try queue.sync {
+            let engine = try self.getUnlocked()
+            try LoginsStoreError.unwrap { err in
+                sync15_passwords_check_valid(engine, json, err)
+            }
+        }
+    }
+
     /// Bump the usage count for the record with the given id.
     ///
     /// Throws `LoginStoreError.NoSuchRecord` if there was no such record.

components/logins/ios/Logins/RustPasswordAPI.h

@@ -12,10 +12,16 @@ typedef enum Sync15PasswordsErrorCode {
     Sync15Passwords_AuthInvalidError = 1,
     Sync15Passwords_NoSuchRecord     = 2,
     Sync15Passwords_DuplicateGuid    = 3,
-    Sync15Passwords_InvalidLogin     = 4,
-    Sync15Passwords_InvalidKeyError  = 5,
-    Sync15Passwords_NetworkError     = 6,
-    Sync15Passwords_InterruptedError = 7,
+    Sync15Passwords_InvalidKeyError  = 4,
+    Sync15Passwords_NetworkError     = 5,
+    Sync15Passwords_InterruptedError = 6,
+
+    Sync15Passwords_InvalidLogin_EmptyHostname = 64 + 0,
+    Sync15Passwords_InvalidLogin_EmptyPassword = 64 + 1,
+    Sync15Passwords_InvalidLogin_DuplicateLogin = 64 + 2,
+    Sync15Passwords_InvalidLogin_BothTargets = 64 + 3,
+    Sync15Passwords_InvalidLogin_NoTarget = 64 + 4,
+
 } Sync15PasswordsErrorCode;
 
 typedef struct Sync15PasswordsError {
@@ -79,6 +85,10 @@ uint8_t sync15_passwords_delete(Sync15PasswordEngineHandle handle,
                                 char const *_Nonnull id,
                                 Sync15PasswordsError *_Nonnull error);
 
+void sync15_passwords_check_valid(Sync15PasswordEngineHandle handle,
+                                char const *_Nonnull record_json,
+                                Sync15PasswordsError *_Nonnull error);
+
 char *_Nullable sync15_passwords_add(Sync15PasswordEngineHandle handle,
                                      char const *_Nonnull json,
                                      Sync15PasswordsError *_Nonnull error);

components/logins/src/db.rs

@@ -340,7 +340,7 @@ impl LoginDb {
     }
 
     pub fn add(&self, mut login: Login) -> Result<Login> {
-        login.check_valid()?;
+        self.check_valid_with_no_dupes(&login)?;
 
         let tx = self.unchecked_transaction()?;
         let now_ms = util::system_time_ms_i64(SystemTime::now());
@@ -474,7 +474,7 @@ impl LoginDb {
         );
         let mut num_failed = 0;
         for login in logins {
-            if let Err(e) = login.check_valid() {
+            if let Err(e) = self.check_valid_with_no_dupes(&login) {
                 log::warn!("Skipping login {} as it is invalid ({}).", login.guid, e);
                 num_failed += 1;
                 continue;
@@ -515,7 +515,7 @@ impl LoginDb {
     }
 
     pub fn update(&self, login: Login) -> Result<()> {
-        login.check_valid()?;
+        self.check_valid_with_no_dupes(&login)?;
         let tx = self.unchecked_transaction()?;
         // Note: These fail with DuplicateGuid if the record doesn't exist.
         self.ensure_local_overlay_exists(login.guid_str())?;
@@ -565,6 +565,56 @@ impl LoginDb {
         Ok(())
     }
 
+    pub fn check_valid_with_no_dupes(&self, login: &Login) -> Result<()> {
+        login.check_valid()?;
+        let _dupe_exists = self.dupe_exists(login)?;
+
+        if _dupe_exists {
+            throw!(InvalidLogin::DuplicateLogin);
+        }
+        Ok(())
+    }
+
+    pub fn dupe_exists(&self, login: &Login) -> Result<bool> {
+        // Note: the query below compares the guids of the given login with existing logins
+        //  to prevent a login from being considered a duplicate of itself (e.g. during updates).
+        Ok(self.db.query_row_named(
+            "SELECT EXISTS(
+                SELECT 1 FROM loginsL
+                WHERE is_deleted = 0
+                    AND guid <> :guid
+                    AND hostname = :hostname
+                    AND NULLIF(username, '') = :username
+                    AND (
+                        formSubmitURL = :form_submit
+                        OR
+                        httpRealm = :http_realm
+                    )
+
+                UNION ALL
+
+                SELECT 1 FROM loginsM
+                WHERE is_overridden = 0
+                    AND guid <> :guid
+                    AND hostname = :hostname
+                    AND NULLIF(username, '') = :username
+                    AND (
+                        formSubmitURL = :form_submit
+                        OR
+                        httpRealm = :http_realm
+                    )
+             )",
+            named_params! {
+                ":guid": &login.guid,
+                ":hostname": &login.hostname,
+                ":username": &login.username,
+                ":http_realm": login.http_realm.as_ref(),
+                ":form_submit": login.form_submit_url.as_ref(),
+            },
+            |row| row.get(0),
+        )?)
+    }
+
     pub fn exists(&self, id: &str) -> Result<bool> {
         Ok(self.db.query_row_named(
             "SELECT EXISTS(
@@ -1089,4 +1139,66 @@ mod tests {
         assert_eq!(res[0].guid, "dummy_000001");
         assert_eq!(res[1].guid, "dummy_000003");
     }
+
+    #[test]
+    fn test_check_valid_with_no_dupes_with_dupe() {
+        let db = LoginDb::open_in_memory(Some("testing")).unwrap();
+        let login = db
+            .add(Login {
+                guid: "dummy_000001".into(),
+                form_submit_url: Some("https://www.example.com/submit".into()),
+                hostname: "https://www.example.com".into(),
+                http_realm: None,
+                username: "test".into(),
+                password: "test".into(),
+                ..Login::default()
+            })
+            .unwrap();
+
+        let duplicate_login_check = db.check_valid_with_no_dupes(&Login {
+            guid: Guid::empty(),
+            form_submit_url: Some("https://www.example.com/submit".into()),
+            hostname: "https://www.example.com".into(),
+            http_realm: None,
+            username: "test".into(),
+            password: "test2".into(),
+            ..Login::default()
+        });
+
+        db.delete(login.guid_str()).unwrap();
+        assert!(&duplicate_login_check.is_err());
+        assert_eq!(
+            &duplicate_login_check.unwrap_err().to_string(),
+            "Invalid login: Login already exists"
+        )
+    }
+
+    #[test]
+    fn test_check_valid_with_no_dupes_with_unique_login() {
+        let db = LoginDb::open_in_memory(Some("testing")).unwrap();
+        let login = db
+            .add(Login {
+                guid: "dummy_000001".into(),
+                form_submit_url: Some("https://www.example.com/submit".into()),
+                hostname: "https://www.example.com".into(),
+                http_realm: None,
+                username: "test".into(),
+                password: "test".into(),
+                ..Login::default()
+            })
+            .unwrap();
+
+        let unique_login_check = db.check_valid_with_no_dupes(&Login {
+            guid: Guid::empty(),
+            form_submit_url: None,
+            hostname: "https://www.example.com".into(),
+            http_realm: Some("https://www.example.com".into()),
+            username: "test".into(),
+            password: "test".into(),
+            ..Login::default()
+        });
+
+        db.delete(login.guid_str()).unwrap();
+        assert!(&unique_login_check.is_ok())
+    }
 }

components/logins/src/engine.rs

@@ -137,6 +137,10 @@ impl PasswordEngine {
             Some(Err(e)) => Err(e.into()),
         }
     }
+
+    pub fn check_valid_with_no_dupes(&self, login: &Login) -> Result<()> {
+        self.db.check_valid_with_no_dupes(login)
+    }
 }
 
 #[cfg(test)]

components/logins/src/error.rs

@@ -69,6 +69,8 @@ pub enum InvalidLogin {
     EmptyHostname,
     #[fail(display = "Password is empty")]
     EmptyPassword,
+    #[fail(display = "Login already exists")]
+    DuplicateLogin,
     #[fail(display = "Both `formSubmitUrl` and `httpRealm` are present")]
     BothTargets,
     #[fail(display = "Neither `formSubmitUrl` and `httpRealm` are present")]

components/logins/src/ffi.rs

@@ -4,7 +4,7 @@
 
 // This module implement the traits that make the FFI code easier to manage.
 
-use crate::{Error, ErrorKind, Login};
+use crate::{Error, ErrorKind, InvalidLogin, Login};
 use ffi_support::{implement_into_ffi_by_json, ErrorCode, ExternError};
 use sync15::ErrorKind as Sync15ErrorKind;
 
@@ -25,18 +25,26 @@ pub mod error_codes {
     /// already existed.
     pub const DUPLICATE_GUID: i32 = 3;
 
-    /// Attempted to insert or update a record so that it is invalid
-    pub const INVALID_LOGIN: i32 = 4;
-
     /// Either the file is not a database, or it is not encrypted with the
     /// provided encryption key.
-    pub const INVALID_KEY: i32 = 5;
+    pub const INVALID_KEY: i32 = 4;
 
     /// A request to the sync server failed.
-    pub const NETWORK: i32 = 6;
+    pub const NETWORK: i32 = 5;
 
     /// An operation has been interrupted.
-    pub const INTERRUPTED: i32 = 7;
+    pub const INTERRUPTED: i32 = 6;
+
+    // Skip a bunch of spaces to make it clear these are part of a group,
+    // even as more and more errors get added. We're only exposing the
+    // InvalidLogin items that can actually be triggered, the others
+    // (if they happen accidentally) will come through as unexpected.
+
+    pub const INVALID_LOGIN_EMPTY_HOSTNAME: i32 = 64;
+    pub const INVALID_LOGIN_EMPTY_PASSWORD: i32 = 64 + 1;
+    pub const INVALID_LOGIN_DUPLICATE_LOGIN: i32 = 64 + 2;
+    pub const INVALID_LOGIN_BOTH_TARGETS: i32 = 64 + 3;
+    pub const INVALID_LOGIN_NO_TARGET: i32 = 64 + 4;
 }
 
 fn get_code(err: &Error) -> ErrorCode {
@@ -61,7 +69,13 @@ fn get_code(err: &Error) -> ErrorCode {
         }
         ErrorKind::InvalidLogin(desc) => {
             log::error!("Invalid login: {}", desc);
-            ErrorCode::new(error_codes::INVALID_LOGIN)
+            ErrorCode::new(match desc {
+                InvalidLogin::EmptyHostname => error_codes::INVALID_LOGIN_EMPTY_HOSTNAME,
+                InvalidLogin::EmptyPassword => error_codes::INVALID_LOGIN_EMPTY_PASSWORD,
+                InvalidLogin::DuplicateLogin => error_codes::INVALID_LOGIN_DUPLICATE_LOGIN,
+                InvalidLogin::BothTargets => error_codes::INVALID_LOGIN_BOTH_TARGETS,
+                InvalidLogin::NoTarget => error_codes::INVALID_LOGIN_NO_TARGET,
+            })
         }
         // We can't destructure `err` without bringing in the libsqlite3_sys crate
         // (and I'd really rather not) so we can't put this in the match.

megazords/ios/MozillaAppServicesTests/LoginsTests.swift

@@ -85,4 +85,41 @@ class LoginsTests: XCTestCase {
         XCTAssertNil(record1.formSubmitURL)
         XCTAssertEqual(record1.httpRealm, "Something Something")
     }
+
+    func testLoginEnsureValid() {
+        let storage = getTestStorage()
+        try! storage.unlock(withEncryptionKey: "test123")
+
+        let id0 = try! storage.add(login: LoginRecord(
+            id: "",
+            password: "hunter5",
+            hostname: "https://www.example5.com",
+            username: "cooluser55",
+            formSubmitURL: "https://www.example5.com/login",
+            httpRealm: nil,
+            timesUsed: nil,
+            timeLastUsed: nil,
+            timeCreated: nil,
+            timePasswordChanged: nil,
+            usernameField: nil,
+            passwordField: nil
+        ))
+
+        let dupeLogin = LoginRecord(
+            id: "",
+            password: "hunter3",
+            hostname: "https://www.example5.com",
+            username: "cooluser55",
+            formSubmitURL: "https://www.example5.com/login",
+            httpRealm: nil,
+            timesUsed: nil,
+            timeLastUsed: nil,
+            timeCreated: nil,
+            timePasswordChanged: nil,
+            usernameField: nil,
+            passwordField: nil
+        )
+
+        XCTAssertThrowsError(try storage.ensureValid(login: dupeLogin))
+    }
 }