From ef04a26cfcdb95fa946991a8586c116ac8d1a679 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 22 Oct 2019 09:46:05 -0300 Subject: [PATCH] [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,