Skip to content

Commit

Permalink
When remotely deleting messages, keep metadata around until user loca…
Browse files Browse the repository at this point in the history 
…lly deletes message
  • Loading branch information
@oxtoacart
oxtoacart committed Jun 2, 2021
1 parent 8bb93a5 commit 283a6e1
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 64 deletions.
75 changes: 51 additions & 24 deletions messaging/src/androidTest/java/io/lantern/messaging/MessagingTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ import kotlin.time.seconds

private val logger = KotlinLogging.logger {}

private const val smileyFace = "\uD83D\uDE04"

@ExperimentalTime
class MessagingTest : BaseMessagingTest() {

Expand Down Expand Up @@ -460,7 +462,7 @@ class MessagingTest : BaseMessagingTest() {
dog.close()
// wait a couple of seconds
delay(2000)
newMessaging(dogDB, "dog").with { dog2 ->
newMessaging(dogDB, "dog").with {
assertFalse(
File(attachment.encryptedFilePath).exists(),
"orphaned attachment file should have been deleted"
Expand Down Expand Up @@ -516,7 +518,6 @@ class MessagingTest : BaseMessagingTest() {

@Test
fun testReactions() {
val smileyFace = "\uD83D\uDE04"
testInCoroutine {
newDB.use { dogDB ->
newDB.use { catDB ->
Expand Down Expand Up @@ -618,21 +619,18 @@ class MessagingTest : BaseMessagingTest() {
"cat responds to dog",
cat,
dog,
"howdie",
"howdy",
replyToId = initialMsgs.sent.id,
attachments = arrayOf(
dog.createAttachment(
"text/plain",
"attachment for dog".length.toLong(),
ByteArrayInputStream(
"attachment for dog".toByteArray(
Charsets.UTF_8
)
)
)
dog.createAttachment(assetToFile("image.jpg"))
)
)

dog.react(
replyMsgs.received.dbPath,
smileyFace
)

// globally delete cat's reply
cat.deleteGlobally(replyMsgs.sent.dbPath)
assertNull(
Expand Down Expand Up @@ -664,16 +662,30 @@ class MessagingTest : BaseMessagingTest() {
)
}

dog.waitForNull(
val deletedMessage = dog.waitFor<Model.StoredMessage>(
replyMsgs.received.dbPath,
"message should have been deleted for dog too"
)
"message should have been marked deleted for dog too"
) {
it.deletedBySender
}
assertEquals("", deletedMessage.text)
assertEquals(0, deletedMessage.thumbnailsCount)
assertEquals(0, deletedMessage.attachmentsCount)
assertEquals(0, deletedMessage.reactionsCount)

replyMsgs.received.attachmentsMap.values.forEach { storedAttachment ->
assertFalse(
File(storedAttachment.encryptedFilePath).exists(),
"attachment file should have been deleted for dog too"
)
}

// dog locally deletes cat's message to clear remaining metadata
dog.deleteLocally(deletedMessage.dbPath)
dog.waitForNull(
replyMsgs.received.dbPath,
"message should have been completely deleted for dog"
)
dog.db.get<Model.Contact>(replyMsgs.received.contactId.contactPath)!!
.let { catContact ->
assertEquals(
Expand Down Expand Up @@ -887,7 +899,7 @@ class MessagingTest : BaseMessagingTest() {
}

// send a message
val senderStoredMsg = from.sendToDirectContact(
var senderStoredMsg = from.sendToDirectContact(
toId,
text,
attachments = attachments,
Expand Down Expand Up @@ -932,6 +944,17 @@ class MessagingTest : BaseMessagingTest() {
testCase
)

// wait for attachments to finish sending
senderStoredMsg = from.waitFor(
senderStoredMsg.dbPath,
"wait for attachments to send"
) { storedMsg ->
storedMsg.attachmentsMap.values.find {
(it.status != Model.StoredAttachment.Status.DONE) ||
(it.hasThumbnail() && it.thumbnail.status != Model.StoredAttachment.Status.DONE)
} == null
}

if (attachments != null) {
assertEquals(attachments.size, senderStoredMsg.attachmentsCount, testCase)
assertEquals(
Expand All @@ -949,9 +972,9 @@ class MessagingTest : BaseMessagingTest() {
testCase
)
assertEquals(
File(attachment.encryptedFilePath).length(),
File(storedAttachment.encryptedFilePath).length(),
AttachmentCipherOutputStream
.getCiphertextLength(attachment.attachment.plaintextLength),
.getCiphertextLength(storedAttachment.attachment.plaintextLength),
testCase
)
}
Expand Down Expand Up @@ -1010,24 +1033,28 @@ class MessagingTest : BaseMessagingTest() {
storedContact.mostRecentAttachmentMimeType,
testCase
)
// wait for all attachments to download
// wait for all attachments and thumbnails to download
recipientStoredMsg =
to.waitFor(recipientStoredMsg.dbPath, testCase) { storedMsg ->
storedMsg.attachmentsMap.values.count {
it.status != Model.StoredAttachment.Status.DONE
} == 0
storedMsg.attachmentsMap.values.find {
(it.status != Model.StoredAttachment.Status.DONE) ||
(
it.hasThumbnail() &&
it.thumbnail.status != Model.StoredAttachment.Status.DONE
)
} == null
}
recipientStoredMsg.attachmentsMap.forEach { (id, attachment) ->
// make sure metadata matches expected
assertEquals(
attachments[id].attachment.metadataMap,
senderStoredMsg.getAttachmentsOrThrow(id).attachment.metadataMap,
attachment.attachment.metadataMap
)
// make sure decrypted content matches expected
assertTrue(
Util.streamsEqual(
attachment.inputStream,
attachments[id].inputStream,
senderStoredMsg.getAttachmentsOrThrow(id).inputStream,
),
testCase
)
Expand Down
19 changes: 11 additions & 8 deletions messaging/src/main/java/io/lantern/messaging/CryptoWorker.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,9 @@ internal class CryptoWorker(
)
}
} catch (e: Exception) {
logger.error("unexpected problem decrypting and storing message, dropping: ${e.message}")
logger.error(
"unexpected problem decrypting and storing message, dropping: ${e.message}"
)
}
}

Expand All @@ -614,7 +616,8 @@ internal class CryptoWorker(
Model.Reaction.parseFrom(transferMsg.reaction)
)
Model.TransferMessage.ContentCase.DELETEMESSAGEID -> messaging.deleteLocally(
senderId.storedMessagePath(transferMsg.deleteMessageId.base32)
senderId.storedMessagePath(transferMsg.deleteMessageId.base32),
keepMetadata = true
)
Model.TransferMessage.ContentCase.DISAPPEARSETTINGS -> storeDisappearSettings(
tx,
Expand Down Expand Up @@ -649,7 +652,9 @@ internal class CryptoWorker(
val inboundThumbnail =
Model.InboundAttachment.newBuilder().setSenderId(senderId)
.setTs(storedMsgBuilder.ts)
.setMessageId(storedMsgBuilder.id).setAttachmentId(id).build()
.setIsThumbnail(true)
.setMessageId(storedMsgBuilder.id)
.setAttachmentId(id).build()
tx.put(inboundThumbnail.dbPath, inboundThumbnail)
val thumbnail =
messaging.newStoredAttachment
Expand Down Expand Up @@ -748,9 +753,7 @@ internal class CryptoWorker(
Model.StoredAttachment.Status.DONE
}
val updatedMsgBuilder = msg.toBuilder()
val fullSizeId =
updatedMsgBuilder.thumbnailsMap[inbound.attachmentId]
if (fullSizeId == null) {
if (!inbound.isThumbnail) {
// this is a regular attachment
val updatedAttachment = msg
.attachmentsMap[inbound.attachmentId]!!.toBuilder()
Expand All @@ -762,14 +765,14 @@ internal class CryptoWorker(
} else {
// this is a thumbnail
// associate thumbnail with the corresponding full-size attachment
updatedMsgBuilder.attachmentsMap[fullSizeId]
updatedMsgBuilder.attachmentsMap[inbound.attachmentId]
?.let { fullSizeAttachment ->
val updatedAttachment = fullSizeAttachment
.thumbnail
.toBuilder()
.setStatus(status).build()
updatedMsgBuilder.putAttachments(
fullSizeId,
inbound.attachmentId,
fullSizeAttachment.toBuilder()
.setThumbnail(updatedAttachment).build()
)
Expand Down
89 changes: 57 additions & 32 deletions messaging/src/main/java/io/lantern/messaging/Messaging.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -490,17 +490,21 @@ class Messaging(
* Deletes the message locally only.
*
* @param msgPath the path identifying the message to delete.
* @param keepMetadata if true, we retain a record of the message and basic metadata, but delete
* everything else
*/
fun deleteLocally(msgPath: String): Model.StoredMessage? {
fun deleteLocally(msgPath: String, keepMetadata: Boolean = false): Model.StoredMessage? {
return db.mutate { tx ->
doDeleteLocally(tx, msgPath)
doDeleteLocally(tx, msgPath, keepMetadata)
}
}

private fun doDeleteLocally(tx: Transaction, msgPath: String): Model.StoredMessage? {
private fun doDeleteLocally(
tx: Transaction,
msgPath: String,
keepMetadata: Boolean = false
): Model.StoredMessage? {
return db.get<Model.StoredMessage>(msgPath)?.let { msg ->
tx.delete(msgPath)

// Delete attachments on disk
// Note - we only delete attachments locally, not in the cloud, because clients
// aren't authorized to modify files. They will naturally be deleted once they hit
Expand All @@ -511,23 +515,40 @@ class Messaging(
}
}

// Delete index entries for messages under this Contact's conversation
tx.delete(msg.contactMessagePath)

// Update the Contact metadata based on the most recent remaining message
tx.listDetails<Model.StoredMessage>(
msg.contactMessagesQuery,
count = 1,
reverseSort = true
).let { storedMessages ->
val mostRecentMsg = storedMessages.firstOrNull()
if (mostRecentMsg != null) {
updateContactMetaData(tx, mostRecentMsg.value, force = true)
} else {
clearContactMetaData(tx, msg.contactId)
if (keepMetadata) {
// don't actually physically delete the message yet, just clear the message content
// and mark it as deleted
tx.put(
msg.dbPath,
msg.toBuilder()
.setDeletedBySender(true)
.clearText()
.clearThumbnails()
.clearAttachments()
.clearReactions()
.build()
)
} else {
// Delete the message
tx.delete(msgPath)

// Delete index entries for messages under this Contact's conversation
tx.delete(msg.contactMessagePath)

// Update the Contact metadata based on the most recent remaining message
tx.listDetails<Model.StoredMessage>(
msg.contactMessagesQuery,
count = 1,
reverseSort = true
).let { storedMessages ->
val mostRecentMsg = storedMessages.firstOrNull()
if (mostRecentMsg != null) {
updateContactMetaData(tx, mostRecentMsg.value, force = true)
} else {
clearContactMetaData(tx, msg.contactId)
}
}
}

return@let msg
}
}
Expand Down Expand Up @@ -742,7 +763,7 @@ class Messaging(

private fun deleteOrphanedAttachments() {
// first build a set of all known attachment paths
var knownAttachmentPaths = db
val knownAttachmentPaths = db

This comment has been minimized.

Sign in to view

Copy link
@oxtoacart

oxtoacart Jun 2, 2021

Author Contributor

drive-by linter fix
.list<Model.StoredMessage>(Schema.PATH_MESSAGES.path("%"))
.flatMap { msg ->
msg.value.attachmentsMap.values.map { attachment -> attachment.encryptedFilePath }
Expand All @@ -764,17 +785,21 @@ class Messaging(
dir: File
) {
if (dir.exists()) {
val files = dir.listFiles()
for (i in files.indices) {
val file = files[i]
if (file.isDirectory) {
deleteOrphanedAttachments(now, cutoffMillis, knownAttachmentPaths, file)
} else {
if (!knownAttachmentPaths.contains(file.absolutePath)) {
val fileOldEnough = now - file.lastModified() > cutoffMillis
if (fileOldEnough) {
logger.debug("deleting orphaned attachment ${file.absolutePath}")
file.delete()
dir.listFiles().let { files ->

This comment has been minimized.

Sign in to view

Copy link
@oxtoacart

oxtoacart Jun 2, 2021

Author Contributor

drive-by linter fix
for (i in files.indices) {
files[i].let { file ->
if (file.isDirectory) {
deleteOrphanedAttachments(now, cutoffMillis, knownAttachmentPaths, file)
} else {
if (!knownAttachmentPaths.contains(file.absolutePath)) {
val fileOldEnough = now - file.lastModified() > cutoffMillis
if (fileOldEnough) {
logger.debug(
"deleting orphaned attachment ${file.absolutePath}"
)
file.delete()
}
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions messaging/src/main/protos/Model.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ message StoredMessage {
DeliveryStatus status = 12; // for outgoing messages, the status of its sending and delivery
int64 firstViewedAt = 13; // the unix timestamp in milliseconds of when this message was first viewed on the current device
int64 disappearAt = 14; // the unix timestamp in milliseconds of when this message will automatically disappear
bool deletedBySender = 16; // message was deleted by sender, most data will be null but metadata remains

This comment has been minimized.

Sign in to view

Copy link
@oxtoacart

oxtoacart Jun 2, 2021

Author Contributor

@Kallirroi This new flag indicates whether the message was remotely deleted by the sender

This comment has been minimized.

Sign in to view

Copy link
@cosmicespresso

cosmicespresso Jun 2, 2021

Contributor

💯 I was wondering if it makes sense to also add a deletion timestamp? We don't need it right now, but we might in the future?

This comment has been minimized.

Sign in to view

Copy link
@oxtoacart

oxtoacart Jun 2, 2021

Author Contributor

I like it! I'll replace the boolean with a timestamp.

This comment has been minimized.

Sign in to view

Copy link
@oxtoacart

oxtoacart Jun 2, 2021

Author Contributor

done
}

// A reaction to a message
Expand Down

0 comments on commit 283a6e1

Please sign in to comment.