Fix dubious error handling in logins and bug in ffi_support

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

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)",
 ]

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)
+    }
 }
 

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 <T: Any?, reified E: Throwable> expectException(klass: Class<E>, callback: () -> T) {
+    protected inline fun <T: Any?, reified E: Throwable> expectException(klass: Class<E>, 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}" })
         }
     }
 

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),

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 <tchiovoloni@mozilla.com>"]
 description = "A crate to help expose Rust functions over the FFI."
 repository = "https://github.com/mozilla/application-services"

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());