Fix bits from pullreq review.

7 files changed, 92 insertions, 61 deletions

diff --git a/client/src/leap/soledad/client/__init__.py b/client/src/leap/soledad/client/__init__.py
index 0b72be27..c76e4a4a 100644
--- a/client/src/leap/soledad/client/__init__.py
+++ b/client/src/leap/soledad/client/__init__.py
@@ -755,7 +755,7 @@ class Soledad(object):
     @property
     def remote_storage_secret(self):
         """
-        Return the secret used for encryption of remotelly stored data.
+        Return the secret used for encryption of remotely stored data.
         """
         return self._secrets.remote_storage_secret
 
diff --git a/client/src/leap/soledad/client/crypto.py b/client/src/leap/soledad/client/crypto.py
index a24f2053..5e3760b3 100644
--- a/client/src/leap/soledad/client/crypto.py
+++ b/client/src/leap/soledad/client/crypto.py
@@ -863,7 +863,7 @@ class SyncDecrypterPool(SyncEncryptDecryptPool):
 
         :param encrypted: If not None, only return documents with encrypted
                           field equal to given parameter.
-        :type encrypted: bool
+        :type encrypted: bool or None
 
         :return: list of doc_id, rev, generation, gen, trans_id
         :rtype: list
@@ -878,16 +878,23 @@ class SyncDecrypterPool(SyncEncryptDecryptPool):
 
     def get_insertable_docs_by_gen(self):
         """
-        Return a list of documents ready to be inserted.
+        Return a list of non-encrypted documents ready to be inserted.
         """
+        # here, we compare the list of all available docs with the list of
+        # decrypted docs and find the longest common prefix between these two
+        # lists. Note that the order of lists fetch matters: if instead we
+        # first fetch the list of decrypted docs and then the list of all
+        # docs, then some document might have been decrypted between these two
+        # calls, and if it is just the right doc then it might not be caught
+        # by the next loop.
         all_docs = self.get_docs_by_generation()
         decrypted_docs = self.get_docs_by_generation(encrypted=False)
         insertable = []
         for doc_id, rev, _, gen, trans_id, encrypted in all_docs:
             try:
-                next_decrypted = decrypted_docs.next()
-                if doc_id == next_decrypted[0]:
-                    content = next_decrypted[2]
+                next_doc_id, _, next_content, _, _, _ = decrypted_docs.next()
+                if doc_id == next_doc_id:
+                    content = next_content
                     insertable.append((doc_id, rev, content, gen, trans_id))
                 else:
                     break
@@ -901,7 +908,7 @@ class SyncDecrypterPool(SyncEncryptDecryptPool):
 
         :param encrypted: If not None, return count of documents with
                           encrypted field equal to given parameter.
-        :type encrypted: bool
+        :type encrypted: bool or None
 
         :return: The count of documents.
         :rtype: int
diff --git a/client/src/leap/soledad/client/mp_safe_db.py b/client/src/leap/soledad/client/mp_safe_db.py
index 2c6b7e24..780b7153 100644
--- a/client/src/leap/soledad/client/mp_safe_db.py
+++ b/client/src/leap/soledad/client/mp_safe_db.py
@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-# crypto.py
+# mp_safe_db.py
 # Copyright (C) 2014 LEAP
 #
 # This program is free software: you can redistribute it and/or modify
diff --git a/client/src/leap/soledad/client/secrets.py b/client/src/leap/soledad/client/secrets.py
index 55580692..b1c22371 100644
--- a/client/src/leap/soledad/client/secrets.py
+++ b/client/src/leap/soledad/client/secrets.py
@@ -69,21 +69,28 @@ logger = logging.getLogger(name=__name__)
 # Exceptions
 #
 
-class NoStorageSecret(Exception):
+
+class SecretsException(Exception):
+    """
+    Generic exception type raised by this module.
+    """
+
+
+class NoStorageSecret(SecretsException):
     """
     Raised when trying to use a storage secret but none is available.
     """
     pass
 
 
-class PassphraseTooShort(Exception):
+class PassphraseTooShort(SecretsException):
     """
     Raised when trying to change the passphrase but the provided passphrase is
     too short.
     """
 
 
-class BootstrapSequenceError(Exception):
+class BootstrapSequenceError(SecretsException):
     """
     Raised when an attempt to generate a secret and store it in a recovery
     document on server failed.
@@ -107,34 +114,35 @@ class SoledadSecrets(object):
 
     LOCAL_STORAGE_SECRET_LENGTH = 512
     """
-    The length of the secret used to derive a passphrase for the SQLCipher
-    database.
+    The length, in bytes, of the secret used to derive a passphrase for the
+    SQLCipher database.
     """
 
     REMOTE_STORAGE_SECRET_LENGTH = 512
     """
-    The length of the secret used to derive an encryption key and a MAC auth
-    key for remote storage.
+    The length, in bytes, of the secret used to derive an encryption key and a
+    MAC auth key for remote storage.
     """
 
     SALT_LENGTH = 64
     """
-    The length of the salt used to derive the key for the storage secret
-    encryption.
+    The length, in bytes, of the salt used to derive the key for the storage
+    secret encryption.
     """
 
     GEN_SECRET_LENGTH = LOCAL_STORAGE_SECRET_LENGTH \
         + REMOTE_STORAGE_SECRET_LENGTH \
         + SALT_LENGTH  # for sync db
     """
-    The length of the secret to be generated. This includes local and remote
-    secrets, and the salt for deriving the sync db secret.
+    The length, in bytes, of the secret to be generated. This includes local
+    and remote secrets, and the salt for deriving the sync db secret.
     """
 
     MINIMUM_PASSPHRASE_LENGTH = 6
     """
-    The minimum length for a passphrase. The passphrase length is only checked
-    when the user changes her passphrase, not when she instantiates Soledad.
+    The minimum length, in bytes, for a passphrase. The passphrase length is
+    only checked when the user changes her passphrase, not when she
+    instantiates Soledad.
     """
 
     IV_SEPARATOR = ":"
@@ -288,7 +296,7 @@ class SoledadSecrets(object):
             self._secrets[self._secret_id] += new_piece
             enlarged = True
         # store and save in shared db if needed
-        if mac is False or enlarged is True:
+        if not mac or enlarged:
             self._store_secrets()
             self._put_secrets_in_shared_db()
 
@@ -443,13 +451,17 @@ class SoledadSecrets(object):
                 raise WrongMac('Could not authenticate recovery document\'s '
                                'contents.')
         # include secrets in the secret pool.
-        secrets = 0
+        secret_count = 0
         for secret_id, encrypted_secret in data[self.STORAGE_SECRETS_KEY].items():
             if secret_id not in self._secrets:
-                secrets += 1
-                self._secrets[secret_id] = \
-                    self._decrypt_storage_secret(encrypted_secret)
-        return secrets, mac
+                try:
+                    self._secrets[secret_id] = \
+                        self._decrypt_storage_secret(encrypted_secret)
+                    secret_count += 1
+                except SecretsException as e:
+                    logger.error("Failed to decrypt storage secret: %s"
+                                 % str(e))
+        return secret_count, mac
 
     def _get_secrets_from_shared_db(self):
         """
@@ -512,10 +524,13 @@ class SoledadSecrets(object):
 
         :return: The decrypted storage secret.
         :rtype: str
+
+        :raise SecretsException: Raised in case the decryption of the storage
+                                 secret fails for some reason.
         """
         # calculate the encryption key
         if encrypted_secret_dict[self.KDF_KEY] != self.KDF_SCRYPT:
-            raise Exception("Unknown KDF in stored secret.")
+            raise SecretsException("Unknown KDF in stored secret.")
         key = scrypt.hash(
             self._passphrase_as_string(),
             # the salt is stored base64 encoded
@@ -524,16 +539,16 @@ class SoledadSecrets(object):
             buflen=32,  # we need a key with 256 bits (32 bytes).
         )
         if encrypted_secret_dict[self.KDF_LENGTH_KEY] != len(key):
-            raise Exception("Wrong length of decryption key.")
+            raise SecretsException("Wrong length of decryption key.")
         if encrypted_secret_dict[self.CIPHER_KEY] != self.CIPHER_AES256:
-            raise Exception("Unknown cipher in stored secret.")
+            raise SecretsException("Unknown cipher in stored secret.")
         # recover the initial value and ciphertext
         iv, ciphertext = encrypted_secret_dict[self.SECRET_KEY].split(
             self.IV_SEPARATOR, 1)
         ciphertext = binascii.a2b_base64(ciphertext)
         decrypted_secret = self._crypto.decrypt_sym(ciphertext, key, iv=iv)
         if encrypted_secret_dict[self.LENGTH_KEY] != len(decrypted_secret):
-            raise Exception("Wrong length of decrypted secret.")
+            raise SecretsException("Wrong length of decrypted secret.")
         return decrypted_secret
 
     def _encrypt_storage_secret(self, decrypted_secret):
@@ -729,8 +744,8 @@ class SoledadSecrets(object):
         :rtype: str
         """
         return scrypt.hash(
-            self._get_local_storage_secret(),  # the password
-            self._get_local_storage_salt(),    # the salt
+            password=self._get_local_storage_secret(),
+            salt=self._get_local_storage_salt(),
             buflen=32,  # we need a key with 256 bits (32 bytes)
         )
 
@@ -755,7 +770,7 @@ class SoledadSecrets(object):
         :rtype: str
         """
         return scrypt.hash(
-            self._get_local_storage_secret(),  # the password
-            self._get_sync_db_salt(),    # the salt
+            password=self._get_local_storage_secret(),
+            salt=self._get_sync_db_salt(),
             buflen=32,  # we need a key with 256 bits (32 bytes)
         )
diff --git a/client/src/leap/soledad/client/sqlcipher.py b/client/src/leap/soledad/client/sqlcipher.py
index a7ddab24..b7de2fba 100644
--- a/client/src/leap/soledad/client/sqlcipher.py
+++ b/client/src/leap/soledad/client/sqlcipher.py
@@ -63,6 +63,7 @@ from leap.soledad.client.target import SoledadSyncTarget
 from leap.soledad.client.target import PendingReceivedDocsSyncError
 from leap.soledad.client.sync import SoledadSynchronizer
 from leap.soledad.client.mp_safe_db import MPSafeSQLiteDB
+from leap.soledad.common import soledad_assert
 from leap.soledad.common.document import SoledadDocument
 
 
@@ -262,13 +263,16 @@ class SQLCipherDatabase(sqlite_backend.SQLitePartialExpandDatabase):
             self._crypto = crypto
 
         # define sync-db attrs
+        self._sqlcipher_file = sqlcipher_file
+        self._sync_db_key = sync_db_key
         self._sync_db = None
         self._sync_db_write_lock = None
         self._sync_enc_pool = None
-        self._init_sync_db(sqlcipher_file, sync_db_key)
+        self.sync_queue = None
 
         if self.defer_encryption:
             # initialize sync db
+            self._init_sync_db()
             # initialize syncing queue encryption pool
             self._sync_enc_pool = SyncEncrypterPool(
                 self._crypto, self._sync_db, self._sync_db_write_lock)
@@ -471,6 +475,8 @@ class SQLCipherDatabase(sqlite_backend.SQLitePartialExpandDatabase):
         res = None
         # the following context manager blocks until the syncing lock can be
         # acquired.
+        if defer_decryption:
+            self._init_sync_db()
         with self.syncer(url, creds=creds) as syncer:
             # XXX could mark the critical section here...
             try:
@@ -564,28 +570,27 @@ class SQLCipherDatabase(sqlite_backend.SQLitePartialExpandDatabase):
             'ALTER TABLE document '
             'ADD COLUMN syncable BOOL NOT NULL DEFAULT TRUE')
 
-    def _init_sync_db(self, sqlcipher_file, sync_db_password):
+    def _init_sync_db(self):
         """
         Initialize the Symmetrically-Encrypted document to be synced database,
         and the queue to communicate with subprocess workers.
-
-        :param sqlcipher_file: The path for the SQLCipher file.
-        :type sqlcipher_file: str
         """
-        sync_db_path = None
-        if sqlcipher_file != ":memory:":
-            sync_db_path = "%s-sync" % sqlcipher_file
-        else:
-            sync_db_path = ":memory:"
-        self._sync_db = MPSafeSQLiteDB(sync_db_path)
-        # protect the sync db with a password
-        if sync_db_password is not None:
-            self._set_crypto_pragmas(
-                self._sync_db, sync_db_password, True,
-                'aes-256-cbc', 4000, 1024)
-        self._sync_db_write_lock = threading.Lock()
-        self._create_sync_db_tables()
-        self.sync_queue = multiprocessing.Queue()
+        if self._sync_db is None:
+            soledad_assert(self._sync_db_key is not None)
+            sync_db_path = None
+            if self._sqlcipher_file != ":memory:":
+                sync_db_path = "%s-sync" % self._sqlcipher_file
+            else:
+                sync_db_path = ":memory:"
+            self._sync_db = MPSafeSQLiteDB(sync_db_path)
+            # protect the sync db with a password
+            if self._sync_db_key is not None:
+                self._set_crypto_pragmas(
+                    self._sync_db, self._sync_db_key, False,
+                    'aes-256-cbc', 4000, 1024)
+            self._sync_db_write_lock = threading.Lock()
+            self._create_sync_db_tables()
+            self.sync_queue = multiprocessing.Queue()
 
     def _create_sync_db_tables(self):
         """
@@ -1106,24 +1111,30 @@ class SQLCipherDatabase(sqlite_backend.SQLitePartialExpandDatabase):
         """
         Close db_handle and close syncer.
         """
-        logger.debug("Sqlcipher backend: closing")
+        if logger is not None:  # logger might be none if called from __del__
+            logger.debug("Sqlcipher backend: closing")
         # stop the sync watcher for deferred encryption
         if self._sync_watcher is not None:
             self._sync_watcher.stop()
             self._sync_watcher.shutdown()
+            self._sync_watcher = None
         # close all open syncers
         for url in self._syncers:
             _, syncer = self._syncers[url]
             syncer.close()
+        self._syncers = []
         # stop the encryption pool
         if self._sync_enc_pool is not None:
             self._sync_enc_pool.close()
+            self._sync_enc_pool = None
         # close the actual database
         if self._db_handle is not None:
             self._db_handle.close()
+            self._db_handle = None
         # close the sync database
         if self._sync_db is not None:
             self._sync_db.close()
+            self._sync_db = None
         # close the sync queue
         if self.sync_queue is not None:
             self.sync_queue.close()
diff --git a/client/src/leap/soledad/client/target.py b/client/src/leap/soledad/client/target.py
index 1cb02856..ae2010a6 100644
--- a/client/src/leap/soledad/client/target.py
+++ b/client/src/leap/soledad/client/target.py
@@ -807,12 +807,9 @@ class SoledadSyncTarget(HTTPSyncTarget, TokenBasedAuth):
             self._sync_db = sync_db
             self._sync_db_write_lock = sync_db_write_lock
 
-    def _setup_sync_decr_pool(self, last_known_generation):
+    def _setup_sync_decr_pool(self):
         """
         Set up the SyncDecrypterPool for deferred decryption.
-
-        :param last_known_generation: Target's last known generation.
-        :type last_known_generation: int
         """
         if self._sync_decr_pool is None:
             # initialize syncing queue decryption pool
@@ -1133,7 +1130,7 @@ class SoledadSyncTarget(HTTPSyncTarget, TokenBasedAuth):
 
         if defer_decryption and self._sync_db is not None:
             self._sync_exchange_lock.acquire()
-            self._setup_sync_decr_pool(last_known_generation)
+            self._setup_sync_decr_pool()
             self._setup_sync_watcher()
             self._defer_decryption = True
         else:
diff --git a/common/src/leap/soledad/common/tests/test_sync_deferred.py b/common/src/leap/soledad/common/tests/test_sync_deferred.py
index 7643b27c..07a9742b 100644
--- a/common/src/leap/soledad/common/tests/test_sync_deferred.py
+++ b/common/src/leap/soledad/common/tests/test_sync_deferred.py
@@ -73,7 +73,8 @@ class BaseSoledadDeferredEncTest(SoledadWithCouchServerMixin):
         self.db1 = open_sqlcipher(self.db1_file, DBPASS, create=True,
                                   document_factory=SoledadDocument,
                                   crypto=self._soledad._crypto,
-                                  defer_encryption=True)
+                                  defer_encryption=True,
+                                  sync_db_key=DBPASS)
         self.db2 = couch.CouchDatabase.open_database(
             urljoin(
                 'http://localhost:' + str(self.wrapper.port), 'test'),