Add contact and transport keys in the same transaction.

This avoids a potential problem where the app crashes after adding the contact but before adding the transport keys, leaving the contact unusable.

briar-api/src/org/briarproject/api/contact/ContactManager.java

@@ -1,5 +1,6 @@
 package org.briarproject.api.contact;
 
+import org.briarproject.api.crypto.SecretKey;
 import org.briarproject.api.db.DbException;
 import org.briarproject.api.db.Transaction;
 import org.briarproject.api.identity.Author;
@@ -19,8 +20,8 @@ public interface ContactManager {
 	 * Stores a contact associated with the given local and remote pseudonyms,
 	 * and returns an ID for the contact.
 	 */
-	ContactId addContact(Author remote, AuthorId local, boolean active)
-			throws DbException;
+	ContactId addContact(Author remote, AuthorId local, SecretKey master,
+			long timestamp, boolean alice, boolean active) throws DbException;
 
 	/** Returns the contact with the given ID. */
 	Contact getContact(ContactId c) throws DbException;

briar-api/src/org/briarproject/api/transport/KeyManager.java

@@ -3,6 +3,8 @@ package org.briarproject.api.transport;
 import org.briarproject.api.TransportId;
 import org.briarproject.api.contact.ContactId;
 import org.briarproject.api.crypto.SecretKey;
+import org.briarproject.api.db.DbException;
+import org.briarproject.api.db.Transaction;
 
 /**
  * Responsible for managing transport keys and recognising the pseudo-random
@@ -16,8 +18,8 @@ public interface KeyManager {
 	 * {@link StreamContext StreamContexts} for the contact can be created
 	 * after this method has returned.
 	 */
-	void addContact(ContactId c, SecretKey master, long timestamp,
-			boolean alice);
+	void addContact(Transaction txn, ContactId c, SecretKey master,
+			long timestamp, boolean alice) throws DbException;
 
 	/**
 	 * Returns a {@link StreamContext} for sending a stream to the given

briar-core/src/org/briarproject/contact/ContactManagerImpl.java

@@ -5,6 +5,7 @@ import com.google.inject.Inject;
 import org.briarproject.api.contact.Contact;
 import org.briarproject.api.contact.ContactId;
 import org.briarproject.api.contact.ContactManager;
+import org.briarproject.api.crypto.SecretKey;
 import org.briarproject.api.db.DatabaseComponent;
 import org.briarproject.api.db.DbException;
 import org.briarproject.api.db.Transaction;
@@ -12,6 +13,7 @@ import org.briarproject.api.identity.Author;
 import org.briarproject.api.identity.AuthorId;
 import org.briarproject.api.identity.IdentityManager.RemoveIdentityHook;
 import org.briarproject.api.identity.LocalAuthor;
+import org.briarproject.api.transport.KeyManager;
 
 import java.util.ArrayList;
 import java.util.Collection;
@@ -22,12 +24,14 @@ import java.util.concurrent.CopyOnWriteArrayList;
 class ContactManagerImpl implements ContactManager, RemoveIdentityHook {
 
 	private final DatabaseComponent db;
+	private final KeyManager keyManager;
 	private final List<AddContactHook> addHooks;
 	private final List<RemoveContactHook> removeHooks;
 
 	@Inject
-	ContactManagerImpl(DatabaseComponent db) {
+	ContactManagerImpl(DatabaseComponent db, KeyManager keyManager) {
 		this.db = db;
+		this.keyManager = keyManager;
 		addHooks = new CopyOnWriteArrayList<AddContactHook>();
 		removeHooks = new CopyOnWriteArrayList<RemoveContactHook>();
 	}
@@ -43,12 +47,14 @@ class ContactManagerImpl implements ContactManager, RemoveIdentityHook {
 	}
 
 	@Override
-	public ContactId addContact(Author remote, AuthorId local, boolean active)
+	public ContactId addContact(Author remote, AuthorId local, SecretKey master,
+			long timestamp, boolean alice, boolean active)
 			throws DbException {
 		ContactId c;
 		Transaction txn = db.startTransaction();
 		try {
 			c = db.addContact(txn, remote, local, active);
+			keyManager.addContact(txn, c, master, timestamp, alice);
 			Contact contact = db.getContact(txn, c);
 			for (AddContactHook hook : addHooks)
 				hook.addingContact(txn, contact);

briar-core/src/org/briarproject/invitation/Connector.java

@@ -219,9 +219,7 @@ abstract class Connector extends Thread {
 			long timestamp, boolean alice) throws DbException {
 		// Add the contact to the database
 		contactId = contactManager.addContact(remoteAuthor,
-				localAuthor.getId(), true);
-		// Derive transport keys
-		keyManager.addContact(contactId, master, timestamp, alice);
+				localAuthor.getId(), master, timestamp, alice, true);
 		return contactId;
 	}
 

briar-core/src/org/briarproject/transport/KeyManagerImpl.java

@@ -88,10 +88,10 @@ class KeyManagerImpl implements KeyManager, Service, EventListener {
 		return true;
 	}
 
-	public void addContact(ContactId c, SecretKey master, long timestamp,
-			boolean alice) {
+	public void addContact(Transaction txn, ContactId c, SecretKey master,
+			long timestamp, boolean alice) throws DbException {
 		for (TransportKeyManager m : managers.values())
-			m.addContact(c, master, timestamp, alice);
+			m.addContact(txn, c, master, timestamp, alice);
 	}
 
 	public StreamContext getStreamContext(ContactId c, TransportId t) {

briar-core/src/org/briarproject/transport/TransportKeyManager.java

@@ -149,8 +149,8 @@ class TransportKeyManager {
 		timer.schedule(task, delay);
 	}
 
-	void addContact(ContactId c, SecretKey master, long timestamp,
-			boolean alice) {
+	void addContact(Transaction txn, ContactId c, SecretKey master,
+			long timestamp, boolean alice) throws DbException {
 		lock.lock();
 		try {
 			// Work out what rotation period the timestamp belongs to
@@ -164,15 +164,7 @@ class TransportKeyManager {
 			// Initialise mutable state for the contact
 			addKeys(c, new MutableTransportKeys(k));
 			// Write the keys back to the DB
-			Transaction txn = db.startTransaction();
-			try {
-				db.addTransportKeys(txn, c, k);
-				txn.setComplete();
-			} finally {
-				db.endTransaction(txn);
-			}
-		} catch (DbException e) {
-			if (LOG.isLoggable(WARNING)) LOG.log(WARNING, e.toString(), e);
+			db.addTransportKeys(txn, c, k);
 		} finally {
 			lock.unlock();
 		}

briar-tests/src/org/briarproject/sync/ConstantsTest.java

@@ -31,6 +31,7 @@ import org.briarproject.event.EventModule;
 import org.briarproject.forum.ForumModule;
 import org.briarproject.identity.IdentityModule;
 import org.briarproject.messaging.MessagingModule;
+import org.briarproject.transport.TransportModule;
 import org.junit.Test;
 
 import java.util.Random;
@@ -57,7 +58,8 @@ public class ConstantsTest extends BriarTestCase {
 				new TestLifecycleModule(), new TestSystemModule(),
 				new ContactModule(), new CryptoModule(), new DatabaseModule(),
 				new DataModule(), new EventModule(), new ForumModule(),
-				new IdentityModule(), new MessagingModule(), new SyncModule());
+				new IdentityModule(), new MessagingModule(), new SyncModule(),
+				new TransportModule());
 		crypto = i.getInstance(CryptoComponent.class);
 		authorFactory = i.getInstance(AuthorFactory.class);
 		privateMessageFactory = i.getInstance(PrivateMessageFactory.class);

briar-tests/src/org/briarproject/sync/SimplexMessagingIntegrationTest.java

@@ -135,9 +135,7 @@ public class SimplexMessagingIntegrationTest extends BriarTestCase {
 		Author bobAuthor = new Author(bobId, "Bob",
 				new byte[MAX_PUBLIC_KEY_LENGTH]);
 		ContactId contactId = contactManager.addContact(bobAuthor, aliceId,
-				true);
-		// Derive and store the transport keys
-		keyManager.addContact(contactId, master, timestamp, true);
+				master, timestamp, true, true);
 
 		// Send Bob a message
 		GroupId groupId = messagingManager.getConversationId(contactId);
@@ -206,9 +204,7 @@ public class SimplexMessagingIntegrationTest extends BriarTestCase {
 		Author aliceAuthor = new Author(aliceId, "Alice",
 				new byte[MAX_PUBLIC_KEY_LENGTH]);
 		ContactId contactId = contactManager.addContact(aliceAuthor, bobId,
-				true);
-		// Derive and store the transport keys
-		keyManager.addContact(contactId, master, timestamp, false);
+				master, timestamp, false, true);
 
 		// Set up an event listener
 		MessageListener listener = new MessageListener();

briar-tests/src/org/briarproject/transport/TransportKeyManagerTest.java

@@ -133,17 +133,15 @@ public class TransportKeyManagerTest extends BriarTestCase {
 				will(new EncodeTagAction());
 			}
 			// Save the keys
-			oneOf(db).startTransaction();
-			will(returnValue(txn));
 			oneOf(db).addTransportKeys(txn, contactId, rotated);
-			oneOf(db).endTransaction(txn);
 		}});
 
 		TransportKeyManager transportKeyManager = new TransportKeyManager(db,
 				crypto, timer, clock, transportId, maxLatency);
 		// The timestamp is 1 ms before the start of rotation period 1000
 		long timestamp = rotationPeriodLength * 1000 - 1;
-		transportKeyManager.addContact(contactId, masterKey, timestamp, alice);
+		transportKeyManager.addContact(txn, contactId, masterKey, timestamp,
+				alice);
 
 		context.assertIsSatisfied();
 	}
@@ -194,17 +192,15 @@ public class TransportKeyManagerTest extends BriarTestCase {
 			oneOf(crypto).rotateTransportKeys(transportKeys, 1000);
 			will(returnValue(transportKeys));
 			// Save the keys
-			oneOf(db).startTransaction();
-			will(returnValue(txn));
 			oneOf(db).addTransportKeys(txn, contactId, transportKeys);
-			oneOf(db).endTransaction(txn);
 		}});
 
 		TransportKeyManager transportKeyManager = new TransportKeyManager(db,
 				crypto, timer, clock, transportId, maxLatency);
 		// The timestamp is at the start of rotation period 1000
 		long timestamp = rotationPeriodLength * 1000;
-		transportKeyManager.addContact(contactId, masterKey, timestamp, alice);
+		transportKeyManager.addContact(txn, contactId, masterKey, timestamp,
+				alice);
 		assertNull(transportKeyManager.getStreamContext(contactId));
 
 		context.assertIsSatisfied();
@@ -240,10 +236,7 @@ public class TransportKeyManagerTest extends BriarTestCase {
 			oneOf(crypto).rotateTransportKeys(transportKeys, 1000);
 			will(returnValue(transportKeys));
 			// Save the keys
-			oneOf(db).startTransaction();
-			will(returnValue(txn));
 			oneOf(db).addTransportKeys(txn, contactId, transportKeys);
-			oneOf(db).endTransaction(txn);
 			// Increment the stream counter
 			oneOf(db).startTransaction();
 			will(returnValue(txn1));
@@ -256,7 +249,8 @@ public class TransportKeyManagerTest extends BriarTestCase {
 				crypto, timer, clock, transportId, maxLatency);
 		// The timestamp is at the start of rotation period 1000
 		long timestamp = rotationPeriodLength * 1000;
-		transportKeyManager.addContact(contactId, masterKey, timestamp, alice);
+		transportKeyManager.addContact(txn, contactId, masterKey, timestamp,
+				alice);
 		// The first request should return a stream context
 		StreamContext ctx = transportKeyManager.getStreamContext(contactId);
 		assertNotNull(ctx);
@@ -299,17 +293,15 @@ public class TransportKeyManagerTest extends BriarTestCase {
 			oneOf(crypto).rotateTransportKeys(transportKeys, 1000);
 			will(returnValue(transportKeys));
 			// Save the keys
-			oneOf(db).startTransaction();
-			will(returnValue(txn));
 			oneOf(db).addTransportKeys(txn, contactId, transportKeys);
-			oneOf(db).endTransaction(txn);
 		}});
 
 		TransportKeyManager transportKeyManager = new TransportKeyManager(db,
 				crypto, timer, clock, transportId, maxLatency);
 		// The timestamp is at the start of rotation period 1000
 		long timestamp = rotationPeriodLength * 1000;
-		transportKeyManager.addContact(contactId, masterKey, timestamp, alice);
+		transportKeyManager.addContact(txn, contactId, masterKey, timestamp,
+				alice);
 		assertNull(transportKeyManager.getStreamContext(new byte[TAG_LENGTH]));
 
 		context.assertIsSatisfied();
@@ -345,10 +337,7 @@ public class TransportKeyManagerTest extends BriarTestCase {
 			oneOf(crypto).rotateTransportKeys(transportKeys, 1000);
 			will(returnValue(transportKeys));
 			// Save the keys
-			oneOf(db).startTransaction();
-			will(returnValue(txn));
 			oneOf(db).addTransportKeys(txn, contactId, transportKeys);
-			oneOf(db).endTransaction(txn);
 			// Encode a new tag after sliding the window
 			oneOf(crypto).encodeTag(with(any(byte[].class)),
 					with(tagKey), with((long) REORDERING_WINDOW_SIZE));
@@ -365,7 +354,8 @@ public class TransportKeyManagerTest extends BriarTestCase {
 				crypto, timer, clock, transportId, maxLatency);
 		// The timestamp is at the start of rotation period 1000
 		long timestamp = rotationPeriodLength * 1000;
-		transportKeyManager.addContact(contactId, masterKey, timestamp, alice);
+		transportKeyManager.addContact(txn, contactId, masterKey, timestamp,
+				alice);
 		// Use the first tag (previous rotation period, stream number 0)
 		assertEquals(REORDERING_WINDOW_SIZE * 3, tags.size());
 		byte[] tag = tags.get(0);