From 190a6bff967e8b66bd3b0ad7bdbbc0c931b21d3e Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 21 Oct 2019 14:59:10 -0300 Subject: [PATCH 1/8] [core] Add method to ConversationClient that returns a set of MessageIds it is responsible for --- .../api/conversation/ConversationManager.java | 8 ++++++++ .../introduction/IntroductionManagerImpl.java | 3 ++- .../briar/messaging/MessagingManagerImpl.java | 17 +++++++++++++++++ .../invitation/GroupInvitationManagerImpl.java | 3 ++- .../briar/sharing/SharingManagerImpl.java | 3 ++- 5 files changed, 31 insertions(+), 3 deletions(-) diff --git a/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java b/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java index fb51b2574..8ab792c3e 100644 --- a/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java +++ b/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java @@ -12,6 +12,7 @@ import org.briarproject.briar.api.client.MessageTracker.GroupCount; import org.briarproject.briar.api.messaging.MessagingManager; import java.util.Collection; +import java.util.Set; @NotNullByDefault public interface ConversationManager { @@ -51,6 +52,13 @@ public interface ConversationManager { Collection getMessageHeaders(Transaction txn, ContactId contactId) throws DbException; + /** + * Returns all conversation {@link MessageId}s for the given contact + * this client is responsible for. + */ + Set getMessageIds(Transaction txn, ContactId contactId) + throws DbException; + GroupCount getGroupCount(Transaction txn, ContactId c) throws DbException; diff --git a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java index 128f73445..3be4536ca 100644 --- a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java @@ -660,7 +660,8 @@ class IntroductionManagerImpl extends ConversationClientImpl return allDeleted; } - private Set getMessageIds(Transaction txn, ContactId c) + @Override + public Set getMessageIds(Transaction txn, ContactId c) throws DbException { GroupId g = getContactGroup(db.getContact(txn, c)).getId(); BdfDictionary query = messageParser.getMessagesVisibleInUiQuery(); diff --git a/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java index 969af3864..938b67a39 100644 --- a/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java @@ -7,6 +7,7 @@ import org.briarproject.bramble.api.contact.Contact; import org.briarproject.bramble.api.contact.ContactId; import org.briarproject.bramble.api.contact.ContactManager.ContactHook; import org.briarproject.bramble.api.data.BdfDictionary; +import org.briarproject.bramble.api.data.BdfEntry; import org.briarproject.bramble.api.data.BdfList; import org.briarproject.bramble.api.data.MetadataParser; import org.briarproject.bramble.api.db.DatabaseComponent; @@ -47,6 +48,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.Set; import javax.annotation.concurrent.Immutable; import javax.inject.Inject; @@ -363,6 +365,21 @@ class MessagingManagerImpl implements MessagingManager, IncomingMessageHook, return headers; } + @Override + public Set getMessageIds(Transaction txn, ContactId c) + throws DbException { + GroupId g = getContactGroup(db.getContact(txn, c)).getId(); + BdfDictionary query = BdfDictionary.of( + new BdfEntry(MSG_KEY_MSG_TYPE, PRIVATE_MESSAGE)); + try { + Map results = + clientHelper.getMessageMetadataAsDictionary(txn, g, query); + return results.keySet(); + } catch (FormatException e) { + throw new DbException(e); + } + } + @Override public String getMessageText(MessageId m) throws DbException { try { diff --git a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java index ac660dd27..b8a8aed7b 100644 --- a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java @@ -729,7 +729,8 @@ class GroupInvitationManagerImpl extends ConversationClientImpl return allDeleted; } - private Set getMessageIds(Transaction txn, ContactId c) + @Override + public Set getMessageIds(Transaction txn, ContactId c) throws DbException { GroupId g = getContactGroup(db.getContact(txn, c)).getId(); BdfDictionary query = messageParser.getMessagesVisibleInUiQuery(); diff --git a/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java index 0f8848bc5..4ebadd4c6 100644 --- a/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java @@ -629,7 +629,8 @@ abstract class SharingManagerImpl return allDeleted; } - private Set getMessageIds(Transaction txn, ContactId c) + @Override + public Set getMessageIds(Transaction txn, ContactId c) throws DbException { GroupId g = getContactGroup(db.getContact(txn, c)).getId(); BdfDictionary query = messageParser.getMessagesVisibleInUiQuery(); From 124e2f99b008aa2f75026e232a662945fcef995f Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 21 Oct 2019 16:23:55 -0300 Subject: [PATCH 2/8] [core] Add method to ConversationClient for deleting a set of messages This also implements the method for MessagingManager (including integration tests) and adds no-op implementations for other clients. --- .../api/conversation/ConversationManager.java | 11 + .../introduction/IntroductionManagerImpl.java | 6 + .../briar/messaging/MessagingManagerImpl.java | 38 +++ .../GroupInvitationManagerImpl.java | 6 + .../briar/sharing/SharingManagerImpl.java | 6 + .../MessagingManagerIntegrationTest.java | 296 ++++++++++++++++++ .../briar/test/BriarIntegrationTest.java | 2 +- .../test/BriarIntegrationTestComponent.java | 6 + 8 files changed, 370 insertions(+), 1 deletion(-) create mode 100644 briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java diff --git a/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java b/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java index 8ab792c3e..e33faca9a 100644 --- a/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java +++ b/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java @@ -72,6 +72,17 @@ public interface ConversationManager { */ boolean deleteAllMessages(Transaction txn, ContactId c) throws DbException; + + /** + * Deletes the given set of messages associated with the given contact. + *

+ * The set of message IDs must only include message IDs returned by + * {@link #getMessageIds}. + * + * @return true if all messages could be deleted, false otherwise + */ + boolean deleteMessages(Transaction txn, ContactId c, + Set messageIds) throws DbException; } } diff --git a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java index 3be4536ca..60997da29 100644 --- a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java @@ -660,6 +660,12 @@ class IntroductionManagerImpl extends ConversationClientImpl return allDeleted; } + @Override + public boolean deleteMessages(Transaction txn, ContactId c, + Set messageIds) throws DbException { + return false; + } + @Override public Set getMessageIds(Transaction txn, ContactId c) throws DbException { diff --git a/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java index 938b67a39..f17becb04 100644 --- a/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java @@ -435,4 +435,42 @@ class MessagingManagerImpl implements MessagingManager, IncomingMessageHook, return true; } + @Override + public boolean deleteMessages(Transaction txn, ContactId c, + Set messageIds) throws DbException { + for (MessageId messageId : messageIds) { + db.deleteMessage(txn, messageId); + db.deleteMessageMetadata(txn, messageId); + } + GroupId g = getContactGroup(db.getContact(txn, c)).getId(); + recalculateGroupCount(txn, g); + return true; + } + + private void recalculateGroupCount(Transaction txn, GroupId g) + throws DbException { + BdfDictionary query = BdfDictionary.of( + new BdfEntry(MSG_KEY_MSG_TYPE, PRIVATE_MESSAGE)); + Map results; + try { + results = + clientHelper.getMessageMetadataAsDictionary(txn, g, query); + } catch (FormatException e) { + throw new DbException(e); + } + int msgCount = results.size(); + int unreadCount = 0; + for (Map.Entry entry : results.entrySet()) { + BdfDictionary meta = entry.getValue(); + boolean read; + try { + read = meta.getBoolean(MSG_KEY_READ); + } catch (FormatException e) { + throw new DbException(e); + } + if (!read) unreadCount++; + } + messageTracker.resetGroupCount(txn, g, msgCount, unreadCount); + } + } diff --git a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java index b8a8aed7b..78164d898 100644 --- a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java @@ -729,6 +729,12 @@ class GroupInvitationManagerImpl extends ConversationClientImpl return allDeleted; } + @Override + public boolean deleteMessages(Transaction txn, ContactId c, + Set messageIds) throws DbException { + return false; + } + @Override public Set getMessageIds(Transaction txn, ContactId c) throws DbException { diff --git a/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java index 4ebadd4c6..efc6b0b9f 100644 --- a/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java @@ -629,6 +629,12 @@ abstract class SharingManagerImpl return allDeleted; } + @Override + public boolean deleteMessages(Transaction txn, ContactId c, + Set messageIds) throws DbException { + return false; + } + @Override public Set getMessageIds(Transaction txn, ContactId c) throws DbException { diff --git a/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java b/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java new file mode 100644 index 000000000..f23bd3334 --- /dev/null +++ b/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java @@ -0,0 +1,296 @@ +package org.briarproject.briar.messaging; + +import org.briarproject.bramble.api.contact.ContactId; +import org.briarproject.bramble.api.db.DatabaseComponent; +import org.briarproject.bramble.api.db.MessageDeletedException; +import org.briarproject.bramble.api.sync.GroupId; +import org.briarproject.bramble.api.sync.MessageId; +import org.briarproject.bramble.test.TestDatabaseConfigModule; +import org.briarproject.briar.api.conversation.ConversationMessageHeader; +import org.briarproject.briar.api.messaging.AttachmentHeader; +import org.briarproject.briar.api.messaging.MessagingManager; +import org.briarproject.briar.api.messaging.PrivateMessage; +import org.briarproject.briar.api.messaging.PrivateMessageFactory; +import org.briarproject.briar.api.messaging.PrivateMessageHeader; +import org.briarproject.briar.test.BriarIntegrationTest; +import org.briarproject.briar.test.BriarIntegrationTestComponent; +import org.briarproject.briar.test.DaggerBriarIntegrationTestComponent; +import org.junit.Before; +import org.junit.Test; + +import java.io.ByteArrayInputStream; +import java.io.InputStream; +import java.util.Collection; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +import javax.annotation.Nullable; + +import static java.util.Collections.emptyList; +import static java.util.Collections.emptySet; +import static java.util.Collections.singletonList; +import static org.briarproject.bramble.test.TestUtils.getRandomBytes; +import static org.briarproject.bramble.util.StringUtils.getRandomString; +import static org.briarproject.briar.test.BriarTestUtils.assertGroupCount; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +public class MessagingManagerIntegrationTest + extends BriarIntegrationTest { + + private DatabaseComponent db0, db1; + private MessagingManager messagingManager0, messagingManager1; + private PrivateMessageFactory messageFactory; + private ContactId contactId; + + @Before + @Override + public void setUp() throws Exception { + super.setUp(); + + db0 = c0.getDatabaseComponent(); + db1 = c1.getDatabaseComponent(); + messagingManager0 = c0.getMessagingManager(); + messagingManager1 = c1.getMessagingManager(); + messageFactory = c0.getPrivateMessageFactory(); + assertEquals(contact0From1, contact1From0); + contactId = contactId0From1; + } + + @Override + protected void createComponents() { + BriarIntegrationTestComponent component = + DaggerBriarIntegrationTestComponent.builder().build(); + component.injectBriarEagerSingletons(); + component.inject(this); + + c0 = DaggerBriarIntegrationTestComponent.builder() + .testDatabaseConfigModule(new TestDatabaseConfigModule(t0Dir)) + .build(); + c0.injectBriarEagerSingletons(); + + c1 = DaggerBriarIntegrationTestComponent.builder() + .testDatabaseConfigModule(new TestDatabaseConfigModule(t1Dir)) + .build(); + c1.injectBriarEagerSingletons(); + + c2 = DaggerBriarIntegrationTestComponent.builder() + .testDatabaseConfigModule(new TestDatabaseConfigModule(t2Dir)) + .build(); + c2.injectBriarEagerSingletons(); + } + + @Test + public void testSimpleConversation() throws Exception { + // conversation start out empty + Collection messages0 = getMessages(c0); + Collection messages1 = getMessages(c1); + assertEquals(0, messages0.size()); + assertEquals(0, messages1.size()); + + // message is sent/displayed properly + String text = getRandomString(42); + sendMessage(c0, c1, text); + messages0 = getMessages(c0); + messages1 = getMessages(c1); + assertEquals(1, messages0.size()); + assertEquals(1, messages1.size()); + PrivateMessageHeader m0 = + (PrivateMessageHeader) messages0.iterator().next(); + PrivateMessageHeader m1 = + (PrivateMessageHeader) messages1.iterator().next(); + assertTrue(m0.hasText()); + assertTrue(m1.hasText()); + assertTrue(m0.isRead()); + assertFalse(m1.isRead()); + assertGroupCounts(c0, 1, 0); + assertGroupCounts(c1, 1, 1); + + // same for reply + String text2 = getRandomString(42); + sendMessage(c1, c0, text2); + messages0 = getMessages(c0); + messages1 = getMessages(c1); + assertEquals(2, messages0.size()); + assertEquals(2, messages1.size()); + assertGroupCounts(c0, 2, 1); + assertGroupCounts(c1, 2, 1); + } + + @Test + public void testAttachments() throws Exception { + // send message with attachment + AttachmentHeader h = addAttachment(c0); + sendMessage(c0, c1, null, singletonList(h)); + + // message with attachment is sent/displayed properly + Collection messages0 = getMessages(c0); + Collection messages1 = getMessages(c1); + assertEquals(1, messages0.size()); + assertEquals(1, messages1.size()); + PrivateMessageHeader m0 = + (PrivateMessageHeader) messages0.iterator().next(); + PrivateMessageHeader m1 = + (PrivateMessageHeader) messages1.iterator().next(); + assertFalse(m0.hasText()); + assertFalse(m1.hasText()); + assertEquals(1, m0.getAttachmentHeaders().size()); + assertEquals(1, m1.getAttachmentHeaders().size()); + assertGroupCounts(c0, 1, 0); + assertGroupCounts(c1, 1, 1); + } + + @Test + public void testDeleteAll() throws Exception { + // send 3 message (1 with attachment) + sendMessage(c0, c1, getRandomString(42)); + sendMessage(c0, c1, getRandomString(23)); + sendMessage(c0, c1, null, singletonList(addAttachment(c0))); + assertEquals(3, getMessages(c0).size()); + assertEquals(3, getMessages(c1).size()); + assertGroupCounts(c0, 3, 0); + assertGroupCounts(c1, 3, 3); + + // delete all messages on both sides (deletes all, because returns true) + assertTrue(db0.transactionWithResult(false, + txn -> messagingManager0.deleteAllMessages(txn, contactId))); + assertTrue(db1.transactionWithResult(false, + txn -> messagingManager1.deleteAllMessages(txn, contactId))); + + // all messages are gone + assertEquals(0, getMessages(c0).size()); + assertEquals(0, getMessages(c1).size()); + assertGroupCounts(c0, 0, 0); + assertGroupCounts(c1, 0, 0); + } + + @Test + public void testDeleteSubset() throws Exception { + // send 3 message (1 with attachment) + PrivateMessage m0 = sendMessage(c0, c1, getRandomString(42)); + PrivateMessage m1 = sendMessage(c0, c1, getRandomString(23)); + PrivateMessage m2 = + sendMessage(c0, c1, null, singletonList(addAttachment(c0))); + assertGroupCounts(c0, 3, 0); + assertGroupCounts(c1, 3, 3); + + // delete 2 messages on both sides (deletes all, because returns true) + Set toDelete = new HashSet<>(); + toDelete.add(m1.getMessage().getId()); + toDelete.add(m2.getMessage().getId()); + assertTrue(db0.transactionWithResult(false, txn -> + messagingManager0.deleteMessages(txn, contactId, toDelete))); + assertTrue(db1.transactionWithResult(false, txn -> + messagingManager1.deleteMessages(txn, contactId, toDelete))); + + // all messages except 1 are gone + assertEquals(1, getMessages(c0).size()); + assertEquals(1, getMessages(c1).size()); + assertEquals(m0.getMessage().getId(), + getMessages(c0).iterator().next().getId()); + assertEquals(m0.getMessage().getId(), + getMessages(c1).iterator().next().getId()); + assertGroupCounts(c0, 1, 0); + assertGroupCounts(c1, 1, 1); + + // remove also last message + toDelete.clear(); + toDelete.add(m0.getMessage().getId()); + assertTrue(db0.transactionWithResult(false, txn -> + messagingManager0.deleteMessages(txn, contactId, toDelete))); + assertEquals(0, getMessages(c0).size()); + assertGroupCounts(c0, 0, 0); + } + + @Test + public void testDeleteAttachment() throws Exception { + // send one message with attachment + AttachmentHeader h = addAttachment(c0); + sendMessage(c0, c1, getRandomString(42), singletonList(h)); + + // attachment exists on both devices + db0.transaction(true, txn -> db0.getMessage(txn, h.getMessageId())); + db1.transaction(true, txn -> db1.getMessage(txn, h.getMessageId())); + + // delete message on both sides (deletes all, because returns true) + assertTrue(db0.transactionWithResult(false, + txn -> messagingManager0.deleteAllMessages(txn, contactId))); + assertTrue(db1.transactionWithResult(false, + txn -> messagingManager1.deleteAllMessages(txn, contactId))); + + // attachment was deleted on both devices + try { + db0.transaction(true, txn -> db0.getMessage(txn, h.getMessageId())); + fail(); + } catch (MessageDeletedException e) { + // expected + } + try { + db1.transaction(true, txn -> db1.getMessage(txn, h.getMessageId())); + fail(); + } catch (MessageDeletedException e) { + // expected + } + } + + @Test + public void testDeletingEmptySet() throws Exception { + assertTrue(db0.transactionWithResult(false, txn -> + messagingManager0.deleteMessages(txn, contactId, emptySet()))); + } + + private PrivateMessage sendMessage(BriarIntegrationTestComponent from, + BriarIntegrationTestComponent to, String text) + throws Exception { + return sendMessage(from, to, text, emptyList()); + } + + private PrivateMessage sendMessage(BriarIntegrationTestComponent from, + BriarIntegrationTestComponent to, @Nullable String text, + List attachments) throws Exception { + GroupId g = from.getMessagingManager().getConversationId(contactId); + PrivateMessage m = messageFactory.createPrivateMessage(g, + clock.currentTimeMillis(), text, attachments); + from.getMessagingManager().addLocalMessage(m); + syncMessage(from, to, contactId, 1 + attachments.size(), true); + return m; + } + + private AttachmentHeader addAttachment(BriarIntegrationTestComponent c) + throws Exception { + GroupId g = c.getMessagingManager().getConversationId(contactId); + InputStream stream = new ByteArrayInputStream(getRandomBytes(42)); + return c.getMessagingManager().addLocalAttachment(g, + clock.currentTimeMillis(), "image/jpeg", stream); + } + + private Collection getMessages( + BriarIntegrationTestComponent c) + throws Exception { + Collection messages = + c.getDatabaseComponent().transactionWithResult(true, + txn -> c.getMessagingManager() + .getMessageHeaders(txn, contactId)); + Set ids = + c.getDatabaseComponent().transactionWithResult(true, + txn -> + c.getMessagingManager() + .getMessageIds(txn, contactId)); + assertEquals(messages.size(), ids.size()); + for (ConversationMessageHeader h : messages) { + assertTrue(ids.contains(h.getId())); + } + return messages; + } + + private void assertGroupCounts(BriarIntegrationTestComponent c, + long msgCount, long unreadCount) throws Exception { + GroupId g = c.getMessagingManager().getConversationId(contactId); + assertGroupCount(c.getMessageTracker(), g, msgCount, unreadCount); + } + + +} diff --git a/briar-core/src/test/java/org/briarproject/briar/test/BriarIntegrationTest.java b/briar-core/src/test/java/org/briarproject/briar/test/BriarIntegrationTest.java index fee6261db..c900410d2 100644 --- a/briar-core/src/test/java/org/briarproject/briar/test/BriarIntegrationTest.java +++ b/briar-core/src/test/java/org/briarproject/briar/test/BriarIntegrationTest.java @@ -354,7 +354,7 @@ public abstract class BriarIntegrationTest Date: Mon, 21 Oct 2019 17:36:22 -0300 Subject: [PATCH 3/8] [core] implement subset conversation message deletion for SharingManager --- .../briar/sharing/SharingManagerImpl.java | 101 +++++++++++---- .../sharing/ForumSharingIntegrationTest.java | 115 ++++++++++++++---- 2 files changed, 164 insertions(+), 52 deletions(-) diff --git a/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java index efc6b0b9f..b998d57b3 100644 --- a/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java @@ -539,13 +539,80 @@ abstract class SharingManagerImpl return m; } + @FunctionalInterface + private interface DeletableSessionRetriever { + Map getDeletableSessions( + GroupId contactGroup, Map metadata) + throws DbException; + } + + @FunctionalInterface + private interface MessageDeletionChecker { + /** + * This is called for all messages belonging to a session. + * It returns true if the given {@link MessageId} causes a problem + * so that the session can not be deleted. + */ + boolean causesProblem(MessageId messageId) throws DbException; + } + @Override public boolean deleteAllMessages(Transaction txn, ContactId c) throws DbException { + return deleteMessages(txn, c, (contactGroup, metadata) -> { + // get all sessions and their states + Map sessions = new HashMap<>(); + for (BdfDictionary d : metadata.values()) { + if (!sessionParser.isSession(d)) continue; + Session session; + try { + session = sessionParser.parseSession(contactGroup, d); + } catch (FormatException e) { + throw new DbException(e); + } + sessions.put(session.getShareableId(), + new DeletableSession(session.getState())); + } + return sessions; + }, messageId -> false); + } + + @Override + public boolean deleteMessages(Transaction txn, ContactId c, + Set messageIds) throws DbException { + return deleteMessages(txn, c, (g, metadata) -> { + // get only sessions from given messageIds + Map sessions = new HashMap<>(); + for (MessageId messageId : messageIds) { + BdfDictionary d = metadata.get(messageId); + if (d == null) continue; // throw new NoSuchMessageException() + try { + MessageMetadata messageMetadata = + messageParser.parseMetadata(d); + SessionId sessionId = + getSessionId(messageMetadata.getShareableId()); + StoredSession ss = getSession(txn, g, sessionId); + if (ss == null) throw new DbException(); + Session session = sessionParser + .parseSession(g, metadata.get(ss.storageId)); + sessions.put(session.getShareableId(), + new DeletableSession(session.getState())); + } catch (FormatException e) { + throw new DbException(e); + } + } + return sessions; + // don't delete sessions if a message is not part of messageIds + }, messageId -> !messageIds.contains(messageId)); + } + + private boolean deleteMessages(Transaction txn, ContactId c, + DeletableSessionRetriever retriever, MessageDeletionChecker checker) + throws DbException { // get ID of the contact group GroupId g = getContactGroup(db.getContact(txn, c)).getId(); - // get metadata for all messages in the + // get metadata for all messages in the group // (these are sessions *and* protocol messages) Map metadata; try { @@ -555,18 +622,8 @@ abstract class SharingManagerImpl } // get all sessions and their states - Map sessions = new HashMap<>(); - for (BdfDictionary d : metadata.values()) { - if (!sessionParser.isSession(d)) continue; - Session session; - try { - session = sessionParser.parseSession(g, d); - } catch (FormatException e) { - throw new DbException(e); - } - sessions.put(session.getShareableId(), - new DeletableSession(session.getState())); - } + Map sessions = + retriever.getDeletableSessions(g, metadata); // assign protocol messages to their sessions for (Entry entry : metadata.entrySet()) { @@ -585,7 +642,7 @@ abstract class SharingManagerImpl // add visible messages to session DeletableSession session = sessions.get(m.getShareableId()); - session.messages.add(entry.getKey()); + if (session != null) session.messages.add(entry.getKey()); } // get a set of all messages which were not ACKed by the contact @@ -593,15 +650,15 @@ abstract class SharingManagerImpl for (MessageStatus status : db.getMessageStatus(txn, c, g)) { if (!status.isSeen()) notAcked.add(status.getMessageId()); } - boolean allDeleted = - deleteCompletedSessions(txn, sessions.values(), notAcked); + boolean allDeleted = deleteCompletedSessions(txn, sessions.values(), + notAcked, checker); recalculateGroupCount(txn, g); return allDeleted; } private boolean deleteCompletedSessions(Transaction txn, - Collection sessions, Set notAcked) - throws DbException { + Collection sessions, Set notAcked, + MessageDeletionChecker checker) throws DbException { // find completed sessions to delete boolean allDeleted = true; for (DeletableSession session : sessions) { @@ -613,7 +670,7 @@ abstract class SharingManagerImpl // where delivery of all messages was confirmed (aka ACKed) boolean allAcked = true; for (MessageId m : session.messages) { - if (notAcked.contains(m)) { + if (notAcked.contains(m) || checker.causesProblem(m)) { allAcked = false; allDeleted = false; break; @@ -629,12 +686,6 @@ abstract class SharingManagerImpl return allDeleted; } - @Override - public boolean deleteMessages(Transaction txn, ContactId c, - Set messageIds) throws DbException { - return false; - } - @Override public Set getMessageIds(Transaction txn, ContactId c) throws DbException { diff --git a/briar-core/src/test/java/org/briarproject/briar/sharing/ForumSharingIntegrationTest.java b/briar-core/src/test/java/org/briarproject/briar/sharing/ForumSharingIntegrationTest.java index ee02f2c95..20084eb98 100644 --- a/briar-core/src/test/java/org/briarproject/briar/sharing/ForumSharingIntegrationTest.java +++ b/briar-core/src/test/java/org/briarproject/briar/sharing/ForumSharingIntegrationTest.java @@ -35,9 +35,12 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import java.util.Collection; +import java.util.HashSet; +import java.util.Set; import javax.annotation.Nullable; +import static java.util.Collections.emptySet; import static junit.framework.Assert.assertNotNull; import static org.briarproject.bramble.util.StringUtils.getRandomString; import static org.briarproject.briar.api.forum.ForumSharingManager.CLIENT_ID; @@ -187,9 +190,8 @@ public class ForumSharingIntegrationTest @Test public void testDeclinedSharing() throws Exception { // send invitation - forumSharingManager0 - .sendInvitation(forum.getId(), contactId1From0, null, - clock.currentTimeMillis()); + forumSharingManager0.sendInvitation(forum.getId(), contactId1From0, + null, clock.currentTimeMillis()); // sync request message sync0To1(1, true); @@ -256,9 +258,8 @@ public class ForumSharingIntegrationTest @Test public void testInviteeLeavesAfterFinished() throws Exception { // send invitation - forumSharingManager0 - .sendInvitation(forum.getId(), contactId1From0, "Hi!", - clock.currentTimeMillis()); + forumSharingManager0.sendInvitation(forum.getId(), contactId1From0, + "Hi!", clock.currentTimeMillis()); // sync request message sync0To1(1, true); @@ -320,9 +321,8 @@ public class ForumSharingIntegrationTest @Test public void testSharerLeavesAfterFinished() throws Exception { // send invitation - forumSharingManager0 - .sendInvitation(forum.getId(), contactId1From0, null, - clock.currentTimeMillis()); + forumSharingManager0.sendInvitation(forum.getId(), contactId1From0, + null, clock.currentTimeMillis()); // sync request message sync0To1(1, true); @@ -398,9 +398,8 @@ public class ForumSharingIntegrationTest @Test public void testSharerLeavesBeforeResponse() throws Exception { // send invitation - forumSharingManager0 - .sendInvitation(forum.getId(), contactId1From0, null, - clock.currentTimeMillis()); + forumSharingManager0.sendInvitation(forum.getId(), contactId1From0, + null, clock.currentTimeMillis()); // sharer un-subscribes from forum forumManager0.removeForum(forum); @@ -445,9 +444,8 @@ public class ForumSharingIntegrationTest @Test public void testSharingSameForumWithEachOther() throws Exception { // send invitation - forumSharingManager0 - .sendInvitation(forum.getId(), contactId1From0, "Hi!", - clock.currentTimeMillis()); + forumSharingManager0.sendInvitation(forum.getId(), contactId1From0, + "Hi!", clock.currentTimeMillis()); // sync request message sync0To1(1, true); @@ -486,9 +484,8 @@ public class ForumSharingIntegrationTest public void testSharingSameForumWithEachOtherBeforeAccept() throws Exception { // send invitation - forumSharingManager0 - .sendInvitation(forum.getId(), contactId1From0, "Hi!", - clock.currentTimeMillis()); + forumSharingManager0.sendInvitation(forum.getId(), contactId1From0, + "Hi!", clock.currentTimeMillis()); sync0To1(1, true); eventWaiter.await(TIMEOUT, 1); assertRequestReceived(listener1, contactId0From1); @@ -556,9 +553,8 @@ public class ForumSharingIntegrationTest @Test public void testContactRemoved() throws Exception { // send invitation - forumSharingManager0 - .sendInvitation(forum.getId(), contactId1From0, "Hi!", - clock.currentTimeMillis()); + forumSharingManager0.sendInvitation(forum.getId(), contactId1From0, + "Hi!", clock.currentTimeMillis()); // sync request message sync0To1(1, true); @@ -683,9 +679,8 @@ public class ForumSharingIntegrationTest @Test public void testSyncAfterReSharing() throws Exception { // send invitation - forumSharingManager0 - .sendInvitation(forum.getId(), contactId1From0, "Hi!", - clock.currentTimeMillis()); + forumSharingManager0.sendInvitation(forum.getId(), contactId1From0, + "Hi!", clock.currentTimeMillis()); // sync request message sync0To1(1, true); @@ -791,9 +786,8 @@ public class ForumSharingIntegrationTest @Test public void testSessionResetAfterAbort() throws Exception { // send invitation - forumSharingManager0 - .sendInvitation(forum.getId(), contactId1From0, "Hi!", - clock.currentTimeMillis()); + forumSharingManager0.sendInvitation(forum.getId(), contactId1From0, + "Hi!", clock.currentTimeMillis()); // sync request message sync0To1(1, true); @@ -981,6 +975,61 @@ public class ForumSharingIntegrationTest assertEquals(1, getMessages0From1().size()); } + @Test + public void testDeletingSomeMessages() throws Exception { + // send invitation + forumSharingManager0.sendInvitation(forum.getId(), contactId1From0, + null, clock.currentTimeMillis()); + sync0To1(1, true); + eventWaiter.await(TIMEOUT, 1); + + // deleting the invitation will fail + Collection m0 = getMessages1From0(); + assertEquals(1, m0.size()); + MessageId messageId = m0.iterator().next().getId(); + Set toDelete = new HashSet<>(); + toDelete.add(messageId); + assertFalse(deleteMessages1From0(toDelete)); + + // decline invitation + respondToRequest(contactId0From1, true); + sync1To0(1, true); + eventWaiter.await(TIMEOUT, 1); + + // both can still not delete the invitation, + // because the response was not selected for deletion as well + assertFalse(deleteMessages1From0(toDelete)); + assertFalse(deleteMessages0From1(toDelete)); + + // after selecting response, both messages can be deleted + m0 = getMessages1From0(); + assertEquals(2, m0.size()); + for (ConversationMessageHeader h : m0) { + if (!h.getId().equals(messageId)) toDelete.add(h.getId()); + } + assertTrue(deleteMessages1From0(toDelete)); + assertEquals(0, getMessages1From0().size()); + // a second time nothing happens + assertTrue(deleteMessages1From0(toDelete)); + + // 1 can still not delete the messages, as last one has not been ACKed + assertFalse(deleteMessages0From1(toDelete)); + + // 0 sends an ACK to their last message + sendAcks(c0, c1, contactId1From0, 1); + + // 1 can now delete all messages, as last one has been ACKed + assertTrue(deleteMessages0From1(toDelete)); + assertEquals(0, getMessages0From1().size()); + // a second time nothing happens + assertTrue(deleteMessages0From1(toDelete)); + } + + @Test + public void testDeletingEmptySet() throws Exception { + assertTrue(deleteMessages0From1(emptySet())); + } + private Collection getMessages1From0() throws DbException { return db0.transactionWithResult(true, txn -> forumSharingManager0 @@ -1003,6 +1052,18 @@ public class ForumSharingIntegrationTest .deleteAllMessages(txn, contactId0From1)); } + private boolean deleteMessages1From0(Set toDelete) + throws DbException { + return db0.transactionWithResult(false, txn -> forumSharingManager0 + .deleteMessages(txn, contactId1From0, toDelete)); + } + + private boolean deleteMessages0From1(Set toDelete) + throws DbException { + return db1.transactionWithResult(false, txn -> forumSharingManager1 + .deleteMessages(txn, contactId0From1, toDelete)); + } + private void respondToRequest(ContactId contactId, boolean accept) throws DbException { assertEquals(1, forumSharingManager1.getInvitations().size()); From ef04a26cfcdb95fa946991a8586c116ac8d1a679 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 22 Oct 2019 09:46:05 -0300 Subject: [PATCH 4/8] [core] implement subset conversation message deletion for GroupInvitationManager --- .../GroupInvitationManagerImpl.java | 124 +++++++++++------- .../invitation/SessionParser.java | 3 + .../invitation/SessionParserImpl.java | 15 +++ .../GroupInvitationIntegrationTest.java | 80 ++++++++++- 4 files changed, 173 insertions(+), 49 deletions(-) diff --git a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java index 78164d898..39472e684 100644 --- a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java @@ -612,29 +612,82 @@ class GroupInvitationManagerImpl extends ConversationClientImpl .getMessageMetadataAsDictionary(txn, contactGroupId, query); Map m = new HashMap<>(); for (BdfDictionary d : results.values()) { - Role role = sessionParser.getRole(d); - if (role == CREATOR) { - CreatorSession s = - sessionParser.parseCreatorSession(contactGroupId, d); - m.put(s.getPrivateGroupId(), s.getState().getVisibility()); - } else if (role == INVITEE) { - InviteeSession s = - sessionParser.parseInviteeSession(contactGroupId, d); - m.put(s.getPrivateGroupId(), s.getState().getVisibility()); - } else if (role == PEER) { - PeerSession s = - sessionParser.parsePeerSession(contactGroupId, d); - m.put(s.getPrivateGroupId(), s.getState().getVisibility()); - } else { - throw new AssertionError(); - } + Session s = sessionParser.parseSession(contactGroupId, d); + m.put(s.getPrivateGroupId(), s.getState().getVisibility()); } return m; } + @FunctionalInterface + private interface DeletableSessionRetriever { + Map getDeletableSessions( + GroupId contactGroup, Map metadata) + throws DbException; + } + + @FunctionalInterface + private interface MessageDeletionChecker { + /** + * This is called for all messages belonging to a session. + * It returns true if the given {@link MessageId} causes a problem + * so that the session can not be deleted. + */ + boolean causesProblem(MessageId messageId) throws DbException; + } + @Override public boolean deleteAllMessages(Transaction txn, ContactId c) throws DbException { + return deleteMessages(txn, c, (g, metadata) -> { + // get all sessions and their states + Map sessions = new HashMap<>(); + for (BdfDictionary d : metadata.values()) { + if (!sessionParser.isSession(d)) continue; + Session session; + try { + session = sessionParser.parseSession(g, d); + } catch (FormatException e) { + throw new DbException(e); + } + sessions.put(session.getPrivateGroupId(), + new DeletableSession(session.getState())); + } + return sessions; + }, messageId -> false); + } + + @Override + public boolean deleteMessages(Transaction txn, ContactId c, + Set messageIds) throws DbException { + return deleteMessages(txn, c, (g, metadata) -> { + // get only sessions from given messageIds + Map sessions = new HashMap<>(); + for (MessageId messageId : messageIds) { + BdfDictionary d = metadata.get(messageId); + if (d == null) continue; // throw new NoSuchMessageException() + try { + MessageMetadata messageMetadata = + messageParser.parseMetadata(d); + SessionId sessionId = + getSessionId(messageMetadata.getPrivateGroupId()); + StoredSession ss = getSession(txn, g, sessionId); + if (ss == null) throw new DbException(); + Session session = sessionParser + .parseSession(g, metadata.get(ss.storageId)); + sessions.put(session.getPrivateGroupId(), + new DeletableSession(session.getState())); + } catch (FormatException e) { + throw new DbException(e); + } + } + return sessions; + // don't delete sessions if a message is not part of messageIds + }, messageId -> !messageIds.contains(messageId)); + } + + private boolean deleteMessages(Transaction txn, ContactId c, + DeletableSessionRetriever retriever, MessageDeletionChecker checker) + throws DbException { // get ID of the contact group GroupId g = getContactGroup(db.getContact(txn, c)).getId(); @@ -648,25 +701,8 @@ class GroupInvitationManagerImpl extends ConversationClientImpl } // get all sessions and their states - Map sessions = new HashMap<>(); - for (BdfDictionary d : metadata.values()) { - if (!sessionParser.isSession(d)) continue; - Session session; - try { - Role role = sessionParser.getRole(d); - if (role == CREATOR) { - session = sessionParser.parseCreatorSession(g, d); - } else if (role == INVITEE) { - session = sessionParser.parseInviteeSession(g, d); - } else if (role == PEER) { - session = sessionParser.parsePeerSession(g, d); - } else throw new AssertionError("unknown role"); - } catch (FormatException e) { - throw new DbException(e); - } - sessions.put(session.getPrivateGroupId(), - new DeletableSession(session.getState())); - } + Map sessions = + retriever.getDeletableSessions(g, metadata); // assign protocol messages to their sessions for (Entry entry : metadata.entrySet()) { @@ -685,7 +721,7 @@ class GroupInvitationManagerImpl extends ConversationClientImpl // add visible messages to session DeletableSession session = sessions.get(m.getPrivateGroupId()); - session.messages.add(entry.getKey()); + if (session != null) session.messages.add(entry.getKey()); } // get a set of all messages which were not ACKed by the contact @@ -693,15 +729,15 @@ class GroupInvitationManagerImpl extends ConversationClientImpl for (MessageStatus status : db.getMessageStatus(txn, c, g)) { if (!status.isSeen()) notAcked.add(status.getMessageId()); } - boolean allDeleted = - deleteCompletedSessions(txn, sessions.values(), notAcked); + boolean allDeleted = deleteCompletedSessions(txn, sessions.values(), + notAcked, checker); recalculateGroupCount(txn, g); return allDeleted; } private boolean deleteCompletedSessions(Transaction txn, - Collection sessions, Set notAcked) - throws DbException { + Collection sessions, Set notAcked, + MessageDeletionChecker checker) throws DbException { // find completed sessions to delete boolean allDeleted = true; for (DeletableSession session : sessions) { @@ -713,7 +749,7 @@ class GroupInvitationManagerImpl extends ConversationClientImpl // where delivery of all messages was confirmed (aka ACKed) boolean allAcked = true; for (MessageId m : session.messages) { - if (notAcked.contains(m)) { + if (notAcked.contains(m) || checker.causesProblem(m)) { allAcked = false; allDeleted = false; break; @@ -729,12 +765,6 @@ class GroupInvitationManagerImpl extends ConversationClientImpl return allDeleted; } - @Override - public boolean deleteMessages(Transaction txn, ContactId c, - Set messageIds) throws DbException { - return false; - } - @Override public Set getMessageIds(Transaction txn, ContactId c) throws DbException { diff --git a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/SessionParser.java b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/SessionParser.java index 115f8531d..b6d0b61b9 100644 --- a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/SessionParser.java +++ b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/SessionParser.java @@ -17,6 +17,9 @@ interface SessionParser { boolean isSession(BdfDictionary d); + Session parseSession(GroupId contactGroupId, BdfDictionary d) + throws FormatException; + CreatorSession parseCreatorSession(GroupId contactGroupId, BdfDictionary d) throws FormatException; diff --git a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/SessionParserImpl.java b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/SessionParserImpl.java index dd848f57e..184c42636 100644 --- a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/SessionParserImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/SessionParserImpl.java @@ -53,6 +53,21 @@ class SessionParserImpl implements SessionParser { return d.getBoolean(SESSION_KEY_IS_SESSION, false); } + @Override + public Session parseSession(GroupId contactGroupId, BdfDictionary d) + throws FormatException { + Session session; + Role role = getRole(d); + if (role == CREATOR) { + session = parseCreatorSession(contactGroupId, d); + } else if (role == INVITEE) { + session = parseInviteeSession(contactGroupId, d); + } else if (role == PEER) { + session = parsePeerSession(contactGroupId, d); + } else throw new AssertionError("unknown role"); + return session; + } + @Override public CreatorSession parseCreatorSession(GroupId contactGroupId, BdfDictionary d) throws FormatException { diff --git a/briar-core/src/test/java/org/briarproject/briar/privategroup/invitation/GroupInvitationIntegrationTest.java b/briar-core/src/test/java/org/briarproject/briar/privategroup/invitation/GroupInvitationIntegrationTest.java index 96d668f09..70406c7eb 100644 --- a/briar-core/src/test/java/org/briarproject/briar/privategroup/invitation/GroupInvitationIntegrationTest.java +++ b/briar-core/src/test/java/org/briarproject/briar/privategroup/invitation/GroupInvitationIntegrationTest.java @@ -2,6 +2,7 @@ package org.briarproject.briar.privategroup.invitation; import org.briarproject.bramble.api.db.DbException; import org.briarproject.bramble.api.sync.Group; +import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.bramble.test.TestDatabaseConfigModule; import org.briarproject.briar.api.client.ProtocolStateException; import org.briarproject.briar.api.conversation.ConversationMessageHeader; @@ -19,9 +20,12 @@ import org.junit.Before; import org.junit.Test; import java.util.Collection; +import java.util.HashSet; +import java.util.Set; import javax.annotation.Nullable; +import static java.util.Collections.emptySet; import static org.briarproject.briar.test.BriarTestUtils.assertGroupCount; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -35,6 +39,7 @@ public class GroupInvitationIntegrationTest private PrivateGroupManager groupManager0, groupManager1; private GroupInvitationManager groupInvitationManager0, groupInvitationManager1; + private Group g1From0, g0From1; @Before @Override @@ -45,6 +50,8 @@ public class GroupInvitationIntegrationTest groupManager1 = c1.getPrivateGroupManager(); groupInvitationManager0 = c0.getGroupInvitationManager(); groupInvitationManager1 = c1.getGroupInvitationManager(); + g1From0 = groupInvitationManager0.getContactGroup(contact1From0); + g0From1 = groupInvitationManager1.getContactGroup(contact0From1); privateGroup = privateGroupFactory.createPrivateGroup("Testgroup", author0); @@ -467,8 +474,6 @@ public class GroupInvitationIntegrationTest sync1To0(1, true); // check group count - Group g1From0 = groupInvitationManager0.getContactGroup(contact1From0); - Group g0From1 = groupInvitationManager1.getContactGroup(contact0From1); assertGroupCount(messageTracker0, g1From0.getId(), 2, 1); assertGroupCount(messageTracker1, g0From1.getId(), 2, 1); @@ -563,6 +568,65 @@ public class GroupInvitationIntegrationTest assertGroupCount(messageTracker0, g1From0.getId(), 0, 0); } + @Test + public void testDeletingSomeMessages() throws Exception { + // send invitation + sendInvitation(clock.currentTimeMillis(), null); + sync0To1(1, true); + + // deleting the invitation will fail for both + Collection m0 = getMessages1From0(); + assertEquals(1, m0.size()); + MessageId messageId = m0.iterator().next().getId(); + Set toDelete = new HashSet<>(); + toDelete.add(messageId); + assertFalse(deleteMessages1From0(toDelete)); + assertFalse(deleteMessages0From1(toDelete)); + + // respond + groupInvitationManager1 + .respondToInvitation(contactId0From1, privateGroup, false); + sync1To0(1, true); + + // both can still not delete the invitation, + // because the response was not selected for deletion as well + assertFalse(deleteMessages1From0(toDelete)); + assertFalse(deleteMessages0From1(toDelete)); + + // after selecting response, both messages can be deleted by creator + m0 = getMessages1From0(); + assertEquals(2, m0.size()); + for (ConversationMessageHeader h : m0) { + if (!h.getId().equals(messageId)) toDelete.add(h.getId()); + } + assertGroupCount(messageTracker0, g1From0.getId(), 2, 1); + assertTrue(deleteMessages1From0(toDelete)); + assertEquals(0, getMessages1From0().size()); + // a second time nothing happens + assertTrue(deleteMessages1From0(toDelete)); + assertGroupCount(messageTracker0, g1From0.getId(), 0, 0); + + // 1 can still not delete the messages, as last one has not been ACKed + assertFalse(deleteMessages0From1(toDelete)); + assertEquals(2, getMessages0From1().size()); + assertGroupCount(messageTracker1, g0From1.getId(), 2, 1); + + // 0 sends an ACK to their last message + sendAcks(c0, c1, contactId1From0, 1); + + // 1 can now delete all messages, as last one has been ACKed + assertTrue(deleteMessages0From1(toDelete)); + assertEquals(0, getMessages0From1().size()); + assertGroupCount(messageTracker1, g0From1.getId(), 0, 0); + // a second time nothing happens + assertTrue(deleteMessages0From1(toDelete)); + } + + @Test + public void testDeletingEmptySet() throws Exception { + assertTrue(deleteMessages0From1(emptySet())); + } + private Collection getMessages1From0() throws DbException { return db0.transactionWithResult(true, txn -> groupInvitationManager0 @@ -585,6 +649,18 @@ public class GroupInvitationIntegrationTest .deleteAllMessages(txn, contactId0From1)); } + private boolean deleteMessages1From0(Set toDelete) + throws DbException { + return db0.transactionWithResult(false, txn -> groupInvitationManager0 + .deleteMessages(txn, contactId1From0, toDelete)); + } + + private boolean deleteMessages0From1(Set toDelete) + throws DbException { + return db1.transactionWithResult(false, txn -> groupInvitationManager1 + .deleteMessages(txn, contactId0From1, toDelete)); + } + private void sendInvitation(long timestamp, @Nullable String text) throws DbException { byte[] signature = groupInvitationFactory.signInvitation(contact1From0, From 5b515d7e18de3e5ac831c42c5d2f6cf2ff2f1a0d Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 22 Oct 2019 10:53:41 -0300 Subject: [PATCH 5/8] [core] implement subset conversation message deletion for IntroductionManager --- .../introduction/IntroductionManagerImpl.java | 82 +++++++--- .../IntroductionIntegrationTest.java | 147 ++++++++++++++++-- 2 files changed, 202 insertions(+), 27 deletions(-) diff --git a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java index 60997da29..3677faf9e 100644 --- a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java @@ -559,26 +559,77 @@ class IntroductionManagerImpl extends ConversationClientImpl } } + @FunctionalInterface + private interface MessageRetriever { + Map getMessages(GroupId contactGroup) + throws DbException; + } + + @FunctionalInterface + private interface MessageDeletionChecker { + /** + * This is called for all messages belonging to a session. + * It returns true if the given {@link MessageId} causes a problem + * so that the session can not be deleted. + */ + boolean causesProblem(MessageId messageId) throws DbException; + } + @Override public boolean deleteAllMessages(Transaction txn, ContactId c) throws DbException { + return deleteMessages(txn, c, g -> { + // get metadata for all messages in the group + Map messages; + try { + messages = clientHelper.getMessageMetadataAsDictionary(txn, g); + } catch (FormatException e) { + throw new DbException(e); + } + return messages; + }, messageId -> false); + } + + @Override + public boolean deleteMessages(Transaction txn, ContactId c, + Set messageIds) throws DbException { + return deleteMessages(txn, c, g -> { + // get metadata for messages that shall be deleted + Map messages = + new HashMap<>(messageIds.size()); + for (MessageId m : messageIds) { + BdfDictionary d; + try { + d = clientHelper.getMessageMetadataAsDictionary(txn, m); + } catch (FormatException e) { + throw new DbException(e); + } + // If message metadata does not exist, + // getMessageMetadataAsDictionary(txn, m) returns empty Metadata + if (!d.isEmpty()) messages.put(m, d); + } + return messages; + // don't delete sessions if a message is not part of messageIds + }, messageId -> !messageIds.contains(messageId)); + } + + private boolean deleteMessages(Transaction txn, ContactId c, + MessageRetriever retriever, MessageDeletionChecker checker) + throws DbException { // get ID of the contact group GroupId g = getContactGroup(db.getContact(txn, c)).getId(); - // get metadata for all messages in the group - Map messages; - try { - messages = clientHelper.getMessageMetadataAsDictionary(txn, g); - } catch (FormatException e) { - throw new DbException(e); - } + // get messages to be deleted + Map messages = retriever.getMessages(g); // assign protocol messages to their sessions Map sessions = new HashMap<>(); for (Entry entry : messages.entrySet()) { + BdfDictionary d = entry.getValue(); + if (d == null) continue; // throw new NoSuchMessageException() MessageMetadata m; try { - m = messageParser.parseMetadata(entry.getValue()); + m = messageParser.parseMetadata(d); } catch (FormatException e) { throw new DbException(e); } @@ -603,7 +654,8 @@ class IntroductionManagerImpl extends ConversationClientImpl for (MessageStatus status : db.getMessageStatus(txn, c, g)) { if (!status.isSeen()) notAcked.add(status.getMessageId()); } - boolean allDeleted = deleteCompletedSessions(txn, sessions, notAcked); + boolean allDeleted = + deleteCompletedSessions(txn, sessions, notAcked, checker); recalculateGroupCount(txn, g); return allDeleted; } @@ -628,8 +680,8 @@ class IntroductionManagerImpl extends ConversationClientImpl } private boolean deleteCompletedSessions(Transaction txn, - Map sessions, Set notAcked) - throws DbException { + Map sessions, Set notAcked, + MessageDeletionChecker checker) throws DbException { // find completed sessions to delete boolean allDeleted = true; for (DeletableSession session : sessions.values()) { @@ -641,7 +693,7 @@ class IntroductionManagerImpl extends ConversationClientImpl // where delivery of all messages was confirmed (aka ACKed) boolean allAcked = true; for (MessageId m : session.messages) { - if (notAcked.contains(m)) { + if (notAcked.contains(m) || checker.causesProblem(m)) { allAcked = false; allDeleted = false; break; @@ -660,12 +712,6 @@ class IntroductionManagerImpl extends ConversationClientImpl return allDeleted; } - @Override - public boolean deleteMessages(Transaction txn, ContactId c, - Set messageIds) throws DbException { - return false; - } - @Override public Set getMessageIds(Transaction txn, ContactId c) throws DbException { diff --git a/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java b/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java index 067f4b08d..beff1dac0 100644 --- a/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java +++ b/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java @@ -39,8 +39,11 @@ import org.junit.Before; import org.junit.Test; import java.util.Collection; +import java.util.HashSet; import java.util.Map; +import java.util.Set; +import static java.util.Collections.emptySet; import static org.briarproject.bramble.test.TestPluginConfigModule.SIMPLEX_TRANSPORT_ID; import static org.briarproject.bramble.test.TestUtils.getAgreementPublicKey; import static org.briarproject.bramble.test.TestUtils.getSecretKey; @@ -82,6 +85,8 @@ public class IntroductionIntegrationTest private IntroduceeListener listener1; private IntroduceeListener listener2; + private Group g1, g2; + interface StateVisitor { AcceptMessage visit(AcceptMessage response); } @@ -95,6 +100,9 @@ public class IntroductionIntegrationTest introductionManager1 = c1.getIntroductionManager(); introductionManager2 = c2.getIntroductionManager(); + g1 = introductionManager0.getContactGroup(contact1From0); + g2 = introductionManager0.getContactGroup(contact2From0); + // initialize waiter fresh for each test eventWaiter = new Waiter(); @@ -1210,7 +1218,6 @@ public class IntroductionIntegrationTest assertTrue(listener2.succeeded); // check that introducer messages are tracked properly - Group g1 = introductionManager0.getContactGroup(contact1From0); assertGroupCount(messageTracker0, g1.getId(), 2, 1); // introducer can now remove messages @@ -1224,8 +1231,7 @@ public class IntroductionIntegrationTest assertEquals(2, getMessages0From1().size()); // check that introducee1 messages are tracked properly - Group g0 = introductionManager1.getContactGroup(contact0From1); - assertGroupCount(messageTracker1, g0.getId(), 2, 1); + assertGroupCount(messageTracker1, g1.getId(), 2, 1); // ACK last message sendAcks(c0, c1, contactId1From0, 1); @@ -1234,17 +1240,16 @@ public class IntroductionIntegrationTest assertTrue(deleteAllMessages0From1()); assertEquals(0, getMessages0From1().size()); assertTrue(deleteAllMessages0From1()); // a second time returns true - assertGroupCount(messageTracker1, g0.getId(), 0, 0); + assertGroupCount(messageTracker1, g1.getId(), 0, 0); // check that introducee2 messages are tracked properly - Group g0From2 = introductionManager2.getContactGroup(contact0From2); - assertGroupCount(messageTracker2, g0From2.getId(), 2, 1); + assertGroupCount(messageTracker2, g2.getId(), 2, 1); // introducee2 can remove messages (last message was incoming) assertTrue(deleteAllMessages0From2()); assertEquals(0, getMessages0From2().size()); assertTrue(deleteAllMessages0From2()); // a second time returns true - assertGroupCount(messageTracker2, g0From2.getId(), 0, 0); + assertGroupCount(messageTracker2, g2.getId(), 0, 0); // a new introduction is still possible assertTrue(introductionManager0 @@ -1273,8 +1278,8 @@ public class IntroductionIntegrationTest // group counts get counted up again correctly assertGroupCount(messageTracker0, g1.getId(), 2, 1); - assertGroupCount(messageTracker1, g0.getId(), 2, 1); - assertGroupCount(messageTracker2, g0From2.getId(), 2, 1); + assertGroupCount(messageTracker1, g1.getId(), 2, 1); + assertGroupCount(messageTracker2, g2.getId(), 2, 1); } @Test @@ -1434,6 +1439,106 @@ public class IntroductionIntegrationTest assertFalse(listener2.aborted); } + @Test + public void testDeletingSomeMessages() throws Exception { + addListeners(false, false); + + // make introduction + long time = clock.currentTimeMillis(); + introductionManager0.makeIntroduction(contact1From0, contact2From0, + "Hi!", time); + + // deleting the introduction for introducee1 will fail + Collection m1From0 = getMessages1From0(); + assertEquals(1, m1From0.size()); + MessageId messageId1 = m1From0.iterator().next().getId(); + Set toDelete1 = new HashSet<>(); + toDelete1.add(messageId1); + assertFalse(deleteMessages1From0(toDelete1)); + + // deleting the introduction for introducee2 will fail as well + Collection m2From0 = getMessages2From0(); + assertEquals(1, m2From0.size()); + MessageId messageId2 = m2From0.iterator().next().getId(); + Set toDelete2 = new HashSet<>(); + toDelete2.add(messageId2); + assertFalse(deleteMessages2From0(toDelete2)); + + // sync REQUEST messages + sync0To1(1, true); + eventWaiter.await(TIMEOUT, 1); + sync0To2(1, true); + eventWaiter.await(TIMEOUT, 1); + + // deleting introduction also fails for introducees + assertFalse(deleteMessages0From1(toDelete1)); + assertFalse(deleteMessages0From2(toDelete2)); + + // add response of introducee1 for deletion + Collection m0From1 = getMessages0From1(); + assertEquals(2, m0From1.size()); + for (ConversationMessageHeader h : m0From1) { + if (!h.getId().equals(messageId1)) toDelete1.add(h.getId()); + } + + // add response of introducee2 for deletion + Collection m0From2 = getMessages0From2(); + assertEquals(2, m0From2.size()); + for (ConversationMessageHeader h : m0From2) { + if (!h.getId().equals(messageId2)) toDelete2.add(h.getId()); + } + + // sync first DECLINE message + sync1To0(1, true); + eventWaiter.await(TIMEOUT, 1); + + // introducer can not yet remove messages + assertFalse(deleteMessages1From0(toDelete1)); + // introducee1 can not yet remove messages + assertFalse(deleteMessages0From1(toDelete1)); + + // sync second DECLINE message + sync2To0(1, true); + eventWaiter.await(TIMEOUT, 1); + + // check group counts + assertGroupCount(messageTracker0, g1.getId(), 2, 1); + assertGroupCount(messageTracker0, g2.getId(), 2, 1); + assertGroupCount(messageTracker1, g1.getId(), 2, 1); + assertGroupCount(messageTracker2, g2.getId(), 2, 1); + + // introducer can now remove messages with both introducees + assertTrue(deleteMessages1From0(toDelete1)); + assertTrue(deleteMessages2From0(toDelete2)); + assertGroupCount(messageTracker0, g1.getId(), 0, 0); + assertGroupCount(messageTracker0, g2.getId(), 0, 0); + // introducee2 can not yet remove messages, missing the other response + assertFalse(deleteMessages0From1(toDelete1)); + + // forward first DECLINE message + sync0To2(1, true); + + // introducee2 can now remove messages + assertTrue(deleteMessages0From2(toDelete2)); + assertEquals(0, getMessages0From2().size()); + assertTrue(deleteMessages0From2(toDelete2)); // a second time nothing happens + assertGroupCount(messageTracker2, g2.getId(), 0, 0); + + // forward second DECLINE message + sync0To1(1, true); + + // introducee1 can now also remove messages + assertTrue(deleteMessages0From1(toDelete1)); + assertEquals(0, getMessages0From1().size()); + assertTrue(deleteMessages0From1(toDelete1)); // a second time nothing happens + assertGroupCount(messageTracker1, g1.getId(), 0, 0); + } + + @Test + public void testDeletingEmptySet() throws Exception { + assertTrue(deleteMessages0From1(emptySet())); + } + private boolean deleteAllMessages1From0() throws DbException { return db0.transactionWithResult(false, txn -> introductionManager0 .deleteAllMessages(txn, contactId1From0)); @@ -1454,6 +1559,30 @@ public class IntroductionIntegrationTest .deleteAllMessages(txn, contactId0From2)); } + private boolean deleteMessages1From0(Set toDelete) + throws DbException { + return db0.transactionWithResult(false, txn -> introductionManager0 + .deleteMessages(txn, contactId1From0, toDelete)); + } + + private boolean deleteMessages2From0(Set toDelete) + throws DbException { + return db0.transactionWithResult(false, txn -> introductionManager0 + .deleteMessages(txn, contactId2From0, toDelete)); + } + + private boolean deleteMessages0From1(Set toDelete) + throws DbException { + return db1.transactionWithResult(false, txn -> introductionManager1 + .deleteMessages(txn, contactId0From1, toDelete)); + } + + private boolean deleteMessages0From2(Set toDelete) + throws DbException { + return db2.transactionWithResult(false, txn -> introductionManager2 + .deleteMessages(txn, contactId0From2, toDelete)); + } + private void addTransportProperties() throws Exception { TransportPropertyManager tpm0 = c0.getTransportPropertyManager(); TransportPropertyManager tpm1 = c1.getTransportPropertyManager(); From f516dbe34f244bcfe0f5edecd6afbfe3a8998a27 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 22 Oct 2019 11:16:27 -0300 Subject: [PATCH 6/8] [core] add method to ConversationManager for deleting a set of messages --- .../api/conversation/ConversationManager.java | 8 ++++++++ .../briar/messaging/ConversationManagerImpl.java | 15 +++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java b/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java index e33faca9a..fee34cd6b 100644 --- a/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java +++ b/briar-api/src/main/java/org/briarproject/briar/api/conversation/ConversationManager.java @@ -44,6 +44,14 @@ public interface ConversationManager { */ boolean deleteAllMessages(ContactId c) throws DbException; + /** + * Deletes the given set of messages associated with the given contact. + * + * @return true if all given messages could be deleted, false otherwise + */ + boolean deleteMessages(ContactId c, Collection messageIds) + throws DbException; + @NotNullByDefault interface ConversationClient { diff --git a/briar-core/src/main/java/org/briarproject/briar/messaging/ConversationManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/messaging/ConversationManagerImpl.java index 957f7e2ad..7163e4c80 100644 --- a/briar-core/src/main/java/org/briarproject/briar/messaging/ConversationManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/messaging/ConversationManagerImpl.java @@ -5,6 +5,7 @@ import org.briarproject.bramble.api.db.DatabaseComponent; import org.briarproject.bramble.api.db.DbException; import org.briarproject.bramble.api.db.Transaction; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; +import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.briar.api.client.MessageTracker.GroupCount; import org.briarproject.briar.api.conversation.ConversationManager; import org.briarproject.briar.api.conversation.ConversationMessageHeader; @@ -84,4 +85,18 @@ class ConversationManagerImpl implements ConversationManager { }); } + @Override + public boolean deleteMessages(ContactId c, Collection toDelete) + throws DbException { + return db.transactionWithResult(false, txn -> { + boolean allDeleted = true; + for (ConversationClient client : clients) { + Set idSet = client.getMessageIds(txn, c); + idSet.retainAll(toDelete); + allDeleted = client.deleteMessages(txn, c, idSet) && allDeleted; + } + return allDeleted; + }); + } + } From c7200910c9e69c731f4e54c79fc492fb5a9fe7fc Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 28 Oct 2019 09:45:41 -0300 Subject: [PATCH 7/8] [core] address feedback for selective conversation message deletion --- .../introduction/IntroductionManagerImpl.java | 20 +++++++++---------- .../GroupInvitationManagerImpl.java | 14 ++++++------- .../briar/sharing/SharingManagerImpl.java | 12 +++++------ .../IntroductionIntegrationTest.java | 3 ++- .../MessagingManagerIntegrationTest.java | 2 +- 5 files changed, 25 insertions(+), 26 deletions(-) diff --git a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java index 3677faf9e..0fdb774ca 100644 --- a/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/introduction/IntroductionManagerImpl.java @@ -561,8 +561,8 @@ class IntroductionManagerImpl extends ConversationClientImpl @FunctionalInterface private interface MessageRetriever { - Map getMessages(GroupId contactGroup) - throws DbException; + Map getMessages(Transaction txn, + GroupId contactGroup) throws DbException; } @FunctionalInterface @@ -572,17 +572,17 @@ class IntroductionManagerImpl extends ConversationClientImpl * It returns true if the given {@link MessageId} causes a problem * so that the session can not be deleted. */ - boolean causesProblem(MessageId messageId) throws DbException; + boolean causesProblem(MessageId messageId); } @Override public boolean deleteAllMessages(Transaction txn, ContactId c) throws DbException { - return deleteMessages(txn, c, g -> { + return deleteMessages(txn, c, (txn1, g) -> { // get metadata for all messages in the group Map messages; try { - messages = clientHelper.getMessageMetadataAsDictionary(txn, g); + messages = clientHelper.getMessageMetadataAsDictionary(txn1, g); } catch (FormatException e) { throw new DbException(e); } @@ -593,14 +593,14 @@ class IntroductionManagerImpl extends ConversationClientImpl @Override public boolean deleteMessages(Transaction txn, ContactId c, Set messageIds) throws DbException { - return deleteMessages(txn, c, g -> { + return deleteMessages(txn, c, (txn1, g) -> { // get metadata for messages that shall be deleted Map messages = new HashMap<>(messageIds.size()); for (MessageId m : messageIds) { BdfDictionary d; try { - d = clientHelper.getMessageMetadataAsDictionary(txn, m); + d = clientHelper.getMessageMetadataAsDictionary(txn1, m); } catch (FormatException e) { throw new DbException(e); } @@ -620,16 +620,14 @@ class IntroductionManagerImpl extends ConversationClientImpl GroupId g = getContactGroup(db.getContact(txn, c)).getId(); // get messages to be deleted - Map messages = retriever.getMessages(g); + Map messages = retriever.getMessages(txn, g); // assign protocol messages to their sessions Map sessions = new HashMap<>(); for (Entry entry : messages.entrySet()) { - BdfDictionary d = entry.getValue(); - if (d == null) continue; // throw new NoSuchMessageException() MessageMetadata m; try { - m = messageParser.parseMetadata(d); + m = messageParser.parseMetadata(entry.getValue()); } catch (FormatException e) { throw new DbException(e); } diff --git a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java index 39472e684..fdacd865e 100644 --- a/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/privategroup/invitation/GroupInvitationManagerImpl.java @@ -620,7 +620,7 @@ class GroupInvitationManagerImpl extends ConversationClientImpl @FunctionalInterface private interface DeletableSessionRetriever { - Map getDeletableSessions( + Map getDeletableSessions(Transaction txn, GroupId contactGroup, Map metadata) throws DbException; } @@ -632,13 +632,13 @@ class GroupInvitationManagerImpl extends ConversationClientImpl * It returns true if the given {@link MessageId} causes a problem * so that the session can not be deleted. */ - boolean causesProblem(MessageId messageId) throws DbException; + boolean causesProblem(MessageId messageId); } @Override public boolean deleteAllMessages(Transaction txn, ContactId c) throws DbException { - return deleteMessages(txn, c, (g, metadata) -> { + return deleteMessages(txn, c, (txn1, g, metadata) -> { // get all sessions and their states Map sessions = new HashMap<>(); for (BdfDictionary d : metadata.values()) { @@ -659,7 +659,7 @@ class GroupInvitationManagerImpl extends ConversationClientImpl @Override public boolean deleteMessages(Transaction txn, ContactId c, Set messageIds) throws DbException { - return deleteMessages(txn, c, (g, metadata) -> { + return deleteMessages(txn, c, (txn1, g, metadata) -> { // get only sessions from given messageIds Map sessions = new HashMap<>(); for (MessageId messageId : messageIds) { @@ -670,7 +670,7 @@ class GroupInvitationManagerImpl extends ConversationClientImpl messageParser.parseMetadata(d); SessionId sessionId = getSessionId(messageMetadata.getPrivateGroupId()); - StoredSession ss = getSession(txn, g, sessionId); + StoredSession ss = getSession(txn1, g, sessionId); if (ss == null) throw new DbException(); Session session = sessionParser .parseSession(g, metadata.get(ss.storageId)); @@ -700,9 +700,9 @@ class GroupInvitationManagerImpl extends ConversationClientImpl throw new DbException(e); } - // get all sessions and their states + // get sessions and their states from retriever Map sessions = - retriever.getDeletableSessions(g, metadata); + retriever.getDeletableSessions(txn, g, metadata); // assign protocol messages to their sessions for (Entry entry : metadata.entrySet()) { diff --git a/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java index b998d57b3..4f7ce604a 100644 --- a/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/sharing/SharingManagerImpl.java @@ -541,7 +541,7 @@ abstract class SharingManagerImpl @FunctionalInterface private interface DeletableSessionRetriever { - Map getDeletableSessions( + Map getDeletableSessions(Transaction txn, GroupId contactGroup, Map metadata) throws DbException; } @@ -553,13 +553,13 @@ abstract class SharingManagerImpl * It returns true if the given {@link MessageId} causes a problem * so that the session can not be deleted. */ - boolean causesProblem(MessageId messageId) throws DbException; + boolean causesProblem(MessageId messageId); } @Override public boolean deleteAllMessages(Transaction txn, ContactId c) throws DbException { - return deleteMessages(txn, c, (contactGroup, metadata) -> { + return deleteMessages(txn, c, (txn1, contactGroup, metadata) -> { // get all sessions and their states Map sessions = new HashMap<>(); for (BdfDictionary d : metadata.values()) { @@ -580,7 +580,7 @@ abstract class SharingManagerImpl @Override public boolean deleteMessages(Transaction txn, ContactId c, Set messageIds) throws DbException { - return deleteMessages(txn, c, (g, metadata) -> { + return deleteMessages(txn, c, (txn1, g, metadata) -> { // get only sessions from given messageIds Map sessions = new HashMap<>(); for (MessageId messageId : messageIds) { @@ -591,7 +591,7 @@ abstract class SharingManagerImpl messageParser.parseMetadata(d); SessionId sessionId = getSessionId(messageMetadata.getShareableId()); - StoredSession ss = getSession(txn, g, sessionId); + StoredSession ss = getSession(txn1, g, sessionId); if (ss == null) throw new DbException(); Session session = sessionParser .parseSession(g, metadata.get(ss.storageId)); @@ -623,7 +623,7 @@ abstract class SharingManagerImpl // get all sessions and their states Map sessions = - retriever.getDeletableSessions(g, metadata); + retriever.getDeletableSessions(txn, g, metadata); // assign protocol messages to their sessions for (Entry entry : metadata.entrySet()) { diff --git a/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java b/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java index beff1dac0..b53b6465d 100644 --- a/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java +++ b/briar-core/src/test/java/org/briarproject/briar/introduction/IntroductionIntegrationTest.java @@ -1470,7 +1470,8 @@ public class IntroductionIntegrationTest sync0To2(1, true); eventWaiter.await(TIMEOUT, 1); - // deleting introduction also fails for introducees + // deleting introduction fails for introducees, + // because response is not yet selected for deletion assertFalse(deleteMessages0From1(toDelete1)); assertFalse(deleteMessages0From2(toDelete2)); diff --git a/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java b/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java index f23bd3334..aae06972c 100644 --- a/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java +++ b/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java @@ -56,7 +56,7 @@ public class MessagingManagerIntegrationTest messagingManager0 = c0.getMessagingManager(); messagingManager1 = c1.getMessagingManager(); messageFactory = c0.getPrivateMessageFactory(); - assertEquals(contact0From1, contact1From0); + assertEquals(contactId0From1, contactId1From0); contactId = contactId0From1; } From 6c489fbea3803bd4a4c503d9efe40428ab09d9cf Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 28 Oct 2019 10:22:04 -0300 Subject: [PATCH 8/8] [core] also delete attachments when deleting select messages --- .../briar/messaging/MessagingManagerImpl.java | 42 ++++++++++++-- .../MessagingManagerIntegrationTest.java | 55 +++++++++++++++++++ 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java index f17becb04..0003b8159 100644 --- a/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java @@ -13,6 +13,7 @@ import org.briarproject.bramble.api.data.MetadataParser; import org.briarproject.bramble.api.db.DatabaseComponent; import org.briarproject.bramble.api.db.DbException; import org.briarproject.bramble.api.db.Metadata; +import org.briarproject.bramble.api.db.NoSuchMessageException; import org.briarproject.bramble.api.db.Transaction; import org.briarproject.bramble.api.lifecycle.LifecycleManager.OpenDatabaseHook; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; @@ -55,6 +56,7 @@ import javax.inject.Inject; import static java.util.Collections.emptyList; import static org.briarproject.bramble.api.sync.SyncConstants.MAX_MESSAGE_BODY_LENGTH; +import static org.briarproject.bramble.api.sync.validation.MessageState.DELIVERED; import static org.briarproject.bramble.util.IoUtils.copyAndClose; import static org.briarproject.briar.client.MessageTrackerConstants.MSG_KEY_READ; import static org.briarproject.briar.messaging.MessageTypes.ATTACHMENT; @@ -438,13 +440,45 @@ class MessagingManagerImpl implements MessagingManager, IncomingMessageHook, @Override public boolean deleteMessages(Transaction txn, ContactId c, Set messageIds) throws DbException { - for (MessageId messageId : messageIds) { - db.deleteMessage(txn, messageId); - db.deleteMessageMetadata(txn, messageId); + boolean allDeleted = true; + for (MessageId m : messageIds) { + // get attachment headers + List headers; + try { + BdfDictionary meta = + clientHelper.getMessageMetadataAsDictionary(txn, m); + Long messageType = meta.getOptionalLong(MSG_KEY_MSG_TYPE); + if (messageType != null && messageType != PRIVATE_MESSAGE) + throw new AssertionError("not supported"); + headers = parseAttachmentHeaders(meta); + } catch (FormatException e) { + throw new DbException(e); + } + // check if all attachments have been delivered + boolean allAttachmentsDelivered = true; + try { + for (AttachmentHeader h : headers) { + if (db.getMessageState(txn, h.getMessageId()) != DELIVERED) + throw new NoSuchMessageException(); + } + } catch (NoSuchMessageException e) { + allAttachmentsDelivered = false; + } + // delete messages, if all attachments were delivered + if (allAttachmentsDelivered) { + for (AttachmentHeader h : headers) { + db.deleteMessage(txn, h.getMessageId()); + db.deleteMessageMetadata(txn, h.getMessageId()); + } + db.deleteMessage(txn, m); + db.deleteMessageMetadata(txn, m); + } else { + allDeleted = false; + } } GroupId g = getContactGroup(db.getContact(txn, c)).getId(); recalculateGroupCount(txn, g); - return true; + return allDeleted; } private void recalculateGroupCount(Transaction txn, GroupId g) diff --git a/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java b/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java index aae06972c..211e7c060 100644 --- a/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java +++ b/briar-core/src/test/java/org/briarproject/briar/messaging/MessagingManagerIntegrationTest.java @@ -29,7 +29,10 @@ import javax.annotation.Nullable; import static java.util.Collections.emptyList; import static java.util.Collections.emptySet; +import static java.util.Collections.singleton; import static java.util.Collections.singletonList; +import static org.briarproject.bramble.api.sync.validation.MessageState.DELIVERED; +import static org.briarproject.bramble.api.sync.validation.MessageState.PENDING; import static org.briarproject.bramble.test.TestUtils.getRandomBytes; import static org.briarproject.bramble.util.StringUtils.getRandomString; import static org.briarproject.briar.test.BriarTestUtils.assertGroupCount; @@ -236,6 +239,58 @@ public class MessagingManagerIntegrationTest } } + @Test + public void testDeleteSomeAttachment() throws Exception { + // send one message with attachment + AttachmentHeader h = addAttachment(c0); + PrivateMessage m = + sendMessage(c0, c1, getRandomString(42), singletonList(h)); + + // attachment exists on both devices, state is set to PENDING + db0.transaction(false, txn -> { + db0.getMessage(txn, h.getMessageId()); + db0.setMessageState(txn, h.getMessageId(), PENDING); + }); + db1.transaction(false, txn -> { + db1.getMessage(txn, h.getMessageId()); + db1.setMessageState(txn, h.getMessageId(), PENDING); + }); + + // deleting message fails (on both sides), + // because attachment is not yet delivered + Set toDelete = singleton(m.getMessage().getId()); + assertFalse(db0.transactionWithResult(false, txn -> + messagingManager0.deleteMessages(txn, contactId, toDelete))); + assertFalse(db1.transactionWithResult(false, txn -> + messagingManager1.deleteMessages(txn, contactId, toDelete))); + + // deliver attachment + db0.transaction(false, + txn -> db0.setMessageState(txn, h.getMessageId(), DELIVERED)); + db1.transaction(false, + txn -> db1.setMessageState(txn, h.getMessageId(), DELIVERED)); + + // deleting message and attachment on both sides works now + assertTrue(db0.transactionWithResult(false, txn -> + messagingManager0.deleteMessages(txn, contactId, toDelete))); + assertTrue(db1.transactionWithResult(false, txn -> + messagingManager1.deleteMessages(txn, contactId, toDelete))); + + // attachment was deleted on both devices + try { + db0.transaction(true, txn -> db0.getMessage(txn, h.getMessageId())); + fail(); + } catch (MessageDeletedException e) { + // expected + } + try { + db1.transaction(true, txn -> db1.getMessage(txn, h.getMessageId())); + fail(); + } catch (MessageDeletedException e) { + // expected + } + } + @Test public void testDeletingEmptySet() throws Exception { assertTrue(db0.transactionWithResult(false, txn ->