diff --git a/CHANGELOG.md b/CHANGELOG.md index fc7228284..38a28d4a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ [Full Changelog](https://github.com/mozilla/application-services/compare/v0.14.0...master) +## Logins + +### What's Fixed + +- Fix an issue where unexpected errors would become panics. ([#593](https://github.com/mozilla/application-services/pull/593)) +- Fix an issue where syncing with invalid credentials would be reported as the wrong kind of error (and cause a panic because of the previous issue). ([#593](https://github.com/mozilla/application-services/pull/593)) + ## Places ### What's New diff --git a/Cargo.lock b/Cargo.lock index 53c11453b..6493bfe3c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -369,7 +369,7 @@ dependencies = [ [[package]] name = "ffi-support" -version = "0.1.4" +version = "0.1.5" dependencies = [ "backtrace 0.3.13 (registry+https://github.com/rust-lang/crates.io-index)", "failure 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", @@ -443,7 +443,7 @@ dependencies = [ "base64 0.9.3 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 1.2.7 (registry+https://github.com/rust-lang/crates.io-index)", "failure 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", - "ffi-support 0.1.4", + "ffi-support 0.1.5", "hawk 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)", "hex 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -465,7 +465,7 @@ name = "fxaclient_ffi" version = "0.1.0" dependencies = [ "android_logger 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", - "ffi-support 0.1.4", + "ffi-support 0.1.5", "fxa-client 0.1.0", "lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", @@ -704,7 +704,7 @@ dependencies = [ "clap 2.32.0 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.5.13 (registry+https://github.com/rust-lang/crates.io-index)", "failure 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", - "ffi-support 0.1.4", + "ffi-support 0.1.5", "fxa-client 0.1.0", "lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", @@ -726,7 +726,7 @@ version = "0.1.0" dependencies = [ "android_logger 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", "base16 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", - "ffi-support 0.1.4", + "ffi-support 0.1.5", "lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "logins 0.1.0", @@ -989,7 +989,7 @@ dependencies = [ "clap 2.32.0 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.5.13 (registry+https://github.com/rust-lang/crates.io-index)", "failure 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", - "ffi-support 0.1.4", + "ffi-support 0.1.5", "find-places-db 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "fxa-client 0.1.0", "idna 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1017,7 +1017,7 @@ name = "places-ffi" version = "0.1.0" dependencies = [ "android_logger 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", - "ffi-support 0.1.4", + "ffi-support 0.1.5", "lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", "places 0.1.0", @@ -1197,7 +1197,7 @@ dependencies = [ name = "rc_log_ffi" version = "0.1.0" dependencies = [ - "ffi-support 0.1.4", + "ffi-support 0.1.5", "lazy_static 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)", ] diff --git a/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt b/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt index 9c4706568..26fa3a298 100644 --- a/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt +++ b/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt @@ -71,5 +71,23 @@ class DatabaseLoginsStorageTest: LoginsStorageTest() { finishAndClose(store) } + + @Test + fun testSyncException() { + val test = getTestStore() + test.ensureUnlocked(encryptionKey) + // Make sure we throw the right exception for invalid info. + expectException(SyncAuthInvalidException::class.java) { + // Provide a real URL for the tokenserver, or we give back an unexpected error about it being an invalid URL + test.sync(SyncUnlockInfo( + kid = "", + fxaAccessToken = "", + syncKey = "", + tokenserverURL = "https://asdf.com" + )) + } + + finishAndClose(test) + } } diff --git a/components/logins/android/src/test/java/mozilla/appservices/logins/LoginsStorageTest.kt b/components/logins/android/src/test/java/mozilla/appservices/logins/LoginsStorageTest.kt index 30b502fd6..6a52d48ab 100644 --- a/components/logins/android/src/test/java/mozilla/appservices/logins/LoginsStorageTest.kt +++ b/components/logins/android/src/test/java/mozilla/appservices/logins/LoginsStorageTest.kt @@ -10,9 +10,9 @@ abstract class LoginsStorageTest { abstract fun createTestStore(): LoginsStorage - private val encryptionKey = "testEncryptionKey" + protected val encryptionKey = "testEncryptionKey" - private fun getTestStore(): LoginsStorage { + protected fun getTestStore(): LoginsStorage { val store = createTestStore() store.unlock(encryptionKey) @@ -44,12 +44,12 @@ abstract class LoginsStorageTest { store.close() } - private inline fun expectException(klass: Class, callback: () -> T) { + protected inline fun expectException(klass: Class, callback: () -> T) { try { callback() fail("Expected exception!") } catch (e: Throwable) { - assert(klass.isInstance(e), { "Expected ${klass} but got exception of type ${e.javaClass}" }) + assert(klass.isInstance(e), { "Expected ${klass} but got exception of type ${e.javaClass}: ${e}" }) } } diff --git a/components/logins/src/ffi.rs b/components/logins/src/ffi.rs index c207ecfc5..58d6b2076 100644 --- a/components/logins/src/ffi.rs +++ b/components/logins/src/ffi.rs @@ -43,7 +43,7 @@ fn get_code(err: &Error) -> ErrorCode { ErrorKind::SyncAdapterError(e) => { log::error!("Sync error {:?}", e); match e.kind() { - Sync15ErrorKind::TokenserverHttpError(401) => { + Sync15ErrorKind::TokenserverHttpError(401) | Sync15ErrorKind::BadKeyLength(..) => { ErrorCode::new(error_codes::AUTH_INVALID) } Sync15ErrorKind::RequestError(_) => ErrorCode::new(error_codes::NETWORK), diff --git a/components/support/ffi/Cargo.toml b/components/support/ffi/Cargo.toml index f60270c61..a78ecdfac 100644 --- a/components/support/ffi/Cargo.toml +++ b/components/support/ffi/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "ffi-support" edition = "2018" -version = "0.1.4" +version = "0.1.5" authors = ["Thom Chiovoloni "] description = "A crate to help expose Rust functions over the FFI." repository = "https://github.com/mozilla/application-services" diff --git a/components/support/ffi/src/error.rs b/components/support/ffi/src/error.rs index 82104bf30..1fca45adb 100644 --- a/components/support/ffi/src/error.rs +++ b/components/support/ffi/src/error.rs @@ -245,7 +245,7 @@ impl ErrorCode { /// that's what you want), or -1 (reserved for panics, but you can use `ErrorCode::PANIC` if /// that's what you want). pub fn new(code: i32) -> Self { - assert!(code > ErrorCode::PANIC.0 && code != ErrorCode::PANIC.0 && code != ErrorCode::SUCCESS.0, + assert!(code > ErrorCode::INVALID_HANDLE.0 && code != ErrorCode::PANIC.0 && code != ErrorCode::SUCCESS.0, "Error: The ErrorCodes `{success}`, `{panic}`, and all error codes less than or equal \ to `{reserved}` are reserved (got {code}). You may use the associated constants on this \ type (`ErrorCode::PANIC`, etc) if you'd like instances of those error codes.", @@ -299,6 +299,12 @@ mod test { ErrorCode::new(-1043); } + #[test] + fn test_code_new_allowed() { + // Should not panic + ErrorCode::new(-2); + } + #[test] fn test_code() { assert!(!ErrorCode::PANIC.is_success());