From 90e622b307e591ffa76fdbea778d9b2db279d04c Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Mon, 24 Jun 2024 16:35:19 -0400 Subject: [PATCH] Require that message bundles include all linked devices --- .../util/DestinationDeviceValidator.java | 6 -- .../controllers/MessageControllerTest.java | 80 ++++++++++--------- .../util/DestinationDeviceValidatorTest.java | 25 +++--- .../current_message_multi_device.json | 9 ++- ...rrent_message_multi_device_not_urgent.json | 7 ++ .../current_message_multi_device_pni.json | 9 ++- .../current_message_registration_id.json | 9 ++- .../fixtures/missing_device_response.json | 4 +- 8 files changed, 89 insertions(+), 60 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/util/DestinationDeviceValidator.java b/service/src/main/java/org/whispersystems/textsecuregcm/util/DestinationDeviceValidator.java index ad26b0ec4..3e6524471 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/util/DestinationDeviceValidator.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/util/DestinationDeviceValidator.java @@ -96,12 +96,6 @@ public class DestinationDeviceValidator { final Set missingDeviceIds = new HashSet<>(accountDeviceIds); missingDeviceIds.removeAll(messageDeviceIds); - // Temporarily "excuse" missing devices if they're missing a message delivery channel as a transitional measure - missingDeviceIds.removeAll(account.getDevices().stream() - .filter(device -> !device.hasMessageDeliveryChannel()) - .map(Device::getId) - .collect(Collectors.toSet())); - final Set extraDeviceIds = new HashSet<>(messageDeviceIds); extraDeviceIds.removeAll(accountDeviceIds); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java index e09e94a08..e8699ea5b 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java @@ -562,7 +562,7 @@ class MessageControllerTest { final ArgumentCaptor envelopeCaptor = ArgumentCaptor.forClass(Envelope.class); - verify(messageSender, times(2)) + verify(messageSender, times(3)) .sendMessage(any(Account.class), any(Device.class), envelopeCaptor.capture(), eq(false)); envelopeCaptor.getAllValues().forEach(envelope -> assertTrue(envelope.getUrgent())); @@ -584,7 +584,7 @@ class MessageControllerTest { final ArgumentCaptor envelopeCaptor = ArgumentCaptor.forClass(Envelope.class); - verify(messageSender, times(2)) + verify(messageSender, times(3)) .sendMessage(any(Account.class), any(Device.class), envelopeCaptor.capture(), eq(false)); envelopeCaptor.getAllValues().forEach(envelope -> assertFalse(envelope.getUrgent())); @@ -604,7 +604,7 @@ class MessageControllerTest { assertThat("Good Response Code", response.getStatus(), is(equalTo(200))); - verify(messageSender, times(2)) + verify(messageSender, times(3)) .sendMessage(any(Account.class), any(Device.class), any(Envelope.class), eq(false)); } } @@ -1220,21 +1220,22 @@ class MessageControllerTest { } private static Map> multiRecipientTargetMap() { - return + return Map.of( + SINGLE_DEVICE_ACI_ID, Map.of(SINGLE_DEVICE_ID1, SINGLE_DEVICE_REG_ID1), + SINGLE_DEVICE_PNI_ID, Map.of(SINGLE_DEVICE_ID1, SINGLE_DEVICE_PNI_REG_ID1), + MULTI_DEVICE_ACI_ID, Map.of( - SINGLE_DEVICE_ACI_ID, Map.of(SINGLE_DEVICE_ID1, SINGLE_DEVICE_REG_ID1), - SINGLE_DEVICE_PNI_ID, Map.of(SINGLE_DEVICE_ID1, SINGLE_DEVICE_PNI_REG_ID1), - MULTI_DEVICE_ACI_ID, - Map.of( - MULTI_DEVICE_ID1, MULTI_DEVICE_REG_ID1, - MULTI_DEVICE_ID2, MULTI_DEVICE_REG_ID2), - MULTI_DEVICE_PNI_ID, - Map.of( - MULTI_DEVICE_ID1, MULTI_DEVICE_PNI_REG_ID1, - MULTI_DEVICE_ID2, MULTI_DEVICE_PNI_REG_ID2), - NONEXISTENT_ACI_ID, Map.of(SINGLE_DEVICE_ID1, SINGLE_DEVICE_REG_ID1), - NONEXISTENT_PNI_ID, Map.of(SINGLE_DEVICE_ID1, SINGLE_DEVICE_PNI_REG_ID1) - ); + MULTI_DEVICE_ID1, MULTI_DEVICE_REG_ID1, + MULTI_DEVICE_ID2, MULTI_DEVICE_REG_ID2, + MULTI_DEVICE_ID3, MULTI_DEVICE_REG_ID3), + MULTI_DEVICE_PNI_ID, + Map.of( + MULTI_DEVICE_ID1, MULTI_DEVICE_PNI_REG_ID1, + MULTI_DEVICE_ID2, MULTI_DEVICE_PNI_REG_ID2, + MULTI_DEVICE_ID3, MULTI_DEVICE_PNI_REG_ID3), + NONEXISTENT_ACI_ID, Map.of(SINGLE_DEVICE_ID1, SINGLE_DEVICE_REG_ID1), + NONEXISTENT_PNI_ID, Map.of(SINGLE_DEVICE_ID1, SINGLE_DEVICE_PNI_REG_ID1) + ); } private record MultiRecipientMessageTestCase( @@ -1273,9 +1274,9 @@ class MessageControllerTest { return ArgumentSets .argumentsForFirstParameter( new MultiRecipientMessageTestCase(singleDeviceAci, unauth, story, 200, 1), - new MultiRecipientMessageTestCase(multiDeviceAci, unauth, story, 200, 2), - new MultiRecipientMessageTestCase(bothAccountsAci, unauth, story, 200, 3), - new MultiRecipientMessageTestCase(realAndFakeAci, unauth, story, 200, 3), + new MultiRecipientMessageTestCase(multiDeviceAci, unauth, story, 200, 3), + new MultiRecipientMessageTestCase(bothAccountsAci, unauth, story, 200, 4), + new MultiRecipientMessageTestCase(realAndFakeAci, unauth, story, 200, 4), new MultiRecipientMessageTestCase(singleDeviceAci, unauth, notStory, 401, 0), new MultiRecipientMessageTestCase(multiDeviceAci, unauth, notStory, 401, 0), @@ -1283,13 +1284,13 @@ class MessageControllerTest { new MultiRecipientMessageTestCase(realAndFakeAci, unauth, notStory, 404, 0), new MultiRecipientMessageTestCase(singleDeviceAci, auth, story, 200, 1), - new MultiRecipientMessageTestCase(multiDeviceAci, auth, story, 200, 2), - new MultiRecipientMessageTestCase(bothAccountsAci, auth, story, 200, 3), - new MultiRecipientMessageTestCase(realAndFakeAci, auth, story, 200, 3), + new MultiRecipientMessageTestCase(multiDeviceAci, auth, story, 200, 3), + new MultiRecipientMessageTestCase(bothAccountsAci, auth, story, 200, 4), + new MultiRecipientMessageTestCase(realAndFakeAci, auth, story, 200, 4), new MultiRecipientMessageTestCase(singleDeviceAci, auth, notStory, 200, 1), - new MultiRecipientMessageTestCase(multiDeviceAci, auth, notStory, 200, 2), - new MultiRecipientMessageTestCase(bothAccountsAci, auth, notStory, 200, 3), + new MultiRecipientMessageTestCase(multiDeviceAci, auth, notStory, 200, 3), + new MultiRecipientMessageTestCase(bothAccountsAci, auth, notStory, 200, 4), new MultiRecipientMessageTestCase(realAndFakeAci, auth, notStory, 404, 0)) .argumentsForNextParameter(false, true) // urgent .argumentsForNextParameter(false, true); // explicitIdentifiers @@ -1326,9 +1327,9 @@ class MessageControllerTest { .argumentsForFirstParameter( new MultiRecipientMessageTestCase(singleDevicePni, unauth, story, 200, 1), new MultiRecipientMessageTestCase(singleDeviceAciAndPni, unauth, story, 200, 2), - new MultiRecipientMessageTestCase(multiDevicePni, unauth, story, 200, 2), - new MultiRecipientMessageTestCase(bothAccountsMixed, unauth, story, 200, 3), - new MultiRecipientMessageTestCase(realAndFakeMixed, unauth, story, 200, 3), + new MultiRecipientMessageTestCase(multiDevicePni, unauth, story, 200, 3), + new MultiRecipientMessageTestCase(bothAccountsMixed, unauth, story, 200, 4), + new MultiRecipientMessageTestCase(realAndFakeMixed, unauth, story, 200, 4), new MultiRecipientMessageTestCase(singleDevicePni, unauth, notStory, 401, 0), new MultiRecipientMessageTestCase(singleDeviceAciAndPni, unauth, notStory, 401, 0), @@ -1338,14 +1339,14 @@ class MessageControllerTest { new MultiRecipientMessageTestCase(singleDevicePni, auth, story, 200, 1), new MultiRecipientMessageTestCase(singleDeviceAciAndPni, auth, story, 200, 2), - new MultiRecipientMessageTestCase(multiDevicePni, auth, story, 200, 2), - new MultiRecipientMessageTestCase(bothAccountsMixed, auth, story, 200, 3), - new MultiRecipientMessageTestCase(realAndFakeMixed, auth, story, 200, 3), + new MultiRecipientMessageTestCase(multiDevicePni, auth, story, 200, 3), + new MultiRecipientMessageTestCase(bothAccountsMixed, auth, story, 200, 4), + new MultiRecipientMessageTestCase(realAndFakeMixed, auth, story, 200, 4), new MultiRecipientMessageTestCase(singleDevicePni, auth, notStory, 200, 1), new MultiRecipientMessageTestCase(singleDeviceAciAndPni, unauth, story, 200, 2), - new MultiRecipientMessageTestCase(multiDevicePni, auth, notStory, 200, 2), - new MultiRecipientMessageTestCase(bothAccountsMixed, auth, notStory, 200, 3), + new MultiRecipientMessageTestCase(multiDevicePni, auth, notStory, 200, 3), + new MultiRecipientMessageTestCase(bothAccountsMixed, auth, notStory, 200, 4), new MultiRecipientMessageTestCase(realAndFakeMixed, auth, notStory, 404, 0)) .argumentsForNextParameter(false, true); // urgent } @@ -1355,7 +1356,8 @@ class MessageControllerTest { final List recipients = List.of( new Recipient(SINGLE_DEVICE_ACI_ID, SINGLE_DEVICE_ID1, SINGLE_DEVICE_REG_ID1, new byte[48]), new Recipient(MULTI_DEVICE_ACI_ID, MULTI_DEVICE_ID1, MULTI_DEVICE_REG_ID1, new byte[48]), - new Recipient(MULTI_DEVICE_ACI_ID, MULTI_DEVICE_ID2, MULTI_DEVICE_REG_ID2, new byte[48])); + new Recipient(MULTI_DEVICE_ACI_ID, MULTI_DEVICE_ID2, MULTI_DEVICE_REG_ID2, new byte[48]), + new Recipient(MULTI_DEVICE_ACI_ID, MULTI_DEVICE_ID3, MULTI_DEVICE_REG_ID3, new byte[48])); // initialize our binary payload and create an input stream byte[] buffer = new byte[2048]; @@ -1378,7 +1380,7 @@ class MessageControllerTest { assertThat("Unexpected response", response.getStatus(), is(equalTo(200))); verify(messageSender, - exactly(3)) + exactly(4)) .sendMessage( any(), any(), @@ -1527,8 +1529,9 @@ class MessageControllerTest { Recipient r2 = new Recipient(MULTI_DEVICE_ACI_ID, MULTI_DEVICE_ID1, MULTI_DEVICE_REG_ID1, new byte[48]); Recipient r3 = new Recipient(MULTI_DEVICE_ACI_ID, MULTI_DEVICE_ID2, MULTI_DEVICE_REG_ID2, new byte[48]); + Recipient r4 = new Recipient(MULTI_DEVICE_ACI_ID, MULTI_DEVICE_ID3, MULTI_DEVICE_REG_ID3, new byte[48]); - List recipients = List.of(r1, r2, r3); + List recipients = List.of(r1, r2, r3, r4); byte[] buffer = new byte[2048]; InputStream stream = initializeMultiPayload(recipients, buffer, useExplicitIdentifier); @@ -1635,7 +1638,8 @@ class MessageControllerTest { void sendMultiRecipientMessageStaleDevices(final ServiceIdentifier serviceIdentifier) throws JsonProcessingException { final List recipients = List.of( new Recipient(serviceIdentifier, MULTI_DEVICE_ID1, MULTI_DEVICE_REG_ID1 + 1, new byte[48]), - new Recipient(serviceIdentifier, MULTI_DEVICE_ID2, MULTI_DEVICE_REG_ID2 + 1, new byte[48])); + new Recipient(serviceIdentifier, MULTI_DEVICE_ID2, MULTI_DEVICE_REG_ID2 + 1, new byte[48]), + new Recipient(serviceIdentifier, MULTI_DEVICE_ID3, MULTI_DEVICE_REG_ID3 + 1, new byte[48])); // initialize our binary payload and create an input stream byte[] buffer = new byte[2048]; @@ -1668,7 +1672,7 @@ class MessageControllerTest { assertEquals(1, staleDevices.size()); assertEquals(serviceIdentifier, staleDevices.getFirst().uuid()); - assertEquals(Set.of(MULTI_DEVICE_ID1, MULTI_DEVICE_ID2), + assertEquals(Set.of(MULTI_DEVICE_ID1, MULTI_DEVICE_ID2, MULTI_DEVICE_ID3), new HashSet<>(staleDevices.getFirst().devices().staleDevices())); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/util/DestinationDeviceValidatorTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/util/DestinationDeviceValidatorTest.java index 63d89f570..7f28f12de 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/util/DestinationDeviceValidatorTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/util/DestinationDeviceValidatorTest.java @@ -123,18 +123,21 @@ class DestinationDeviceValidatorTest { final byte id1 = 1; final byte id2 = 2; final byte id3 = 3; + + final Account account = mockAccountWithDeviceAndEnabled(Map.of(id1, true, id2, false, id3, true)); + return Stream.of( // Device IDs provided for all enabled devices arguments( - mockAccountWithDeviceAndEnabled(Map.of(id1, true, id2, false, id3, true)), + account, Set.of(id1, id3), - null, + Set.of(id2), null, Collections.emptySet()), // Device ID provided for disabled device arguments( - mockAccountWithDeviceAndEnabled(Map.of(id1, true, id2, false, id3, true)), + account, Set.of(id1, id2, id3), null, null, @@ -142,15 +145,15 @@ class DestinationDeviceValidatorTest { // Device ID omitted for enabled device arguments( - mockAccountWithDeviceAndEnabled(Map.of(id1, true, id2, false, id3, true)), + account, Set.of(id1), - Set.of(id3), + Set.of(id2, id3), null, Collections.emptySet()), // Device ID included for disabled device, omitted for enabled device arguments( - mockAccountWithDeviceAndEnabled(Map.of(id1, true, id2, false, id3, true)), + account, Set.of(id1, id2), Set.of(id3), null, @@ -158,16 +161,16 @@ class DestinationDeviceValidatorTest { // Device ID omitted for enabled device, included for device in excluded list arguments( - mockAccountWithDeviceAndEnabled(Map.of(id1, true, id2, false, id3, true)), + account, Set.of(id1), - Set.of(id3), + Set.of(id2, id3), Set.of(id1), Set.of(id1) ), // Device ID omitted for enabled device, included for disabled device, omitted for excluded device arguments( - mockAccountWithDeviceAndEnabled(Map.of(id1, true, id2, false, id3, true)), + account, Set.of(id2), Set.of(id3), null, @@ -176,9 +179,9 @@ class DestinationDeviceValidatorTest { // Device ID included for enabled device, omitted for excluded device arguments( - mockAccountWithDeviceAndEnabled(Map.of(id1, true, id2, false, id3, true)), + account, Set.of(id3), - null, + Set.of(id2), null, Set.of(id1) ) diff --git a/service/src/test/resources/fixtures/current_message_multi_device.json b/service/src/test/resources/fixtures/current_message_multi_device.json index 4236372fe..f5c8b6ba8 100644 --- a/service/src/test/resources/fixtures/current_message_multi_device.json +++ b/service/src/test/resources/fixtures/current_message_multi_device.json @@ -12,5 +12,12 @@ "destinationRegistrationId" : 333, "content" : "Zm9vYmFyego", "timestamp" : 1234 - }] + }, + { + "type" : 1, + "destinationDeviceId" : 3, + "destinationRegistrationId" : 444, + "content" : "Zm9vYmFyego", + "timestamp" : 1234 + }] } diff --git a/service/src/test/resources/fixtures/current_message_multi_device_not_urgent.json b/service/src/test/resources/fixtures/current_message_multi_device_not_urgent.json index c07ca93a2..6fe8b8f49 100644 --- a/service/src/test/resources/fixtures/current_message_multi_device_not_urgent.json +++ b/service/src/test/resources/fixtures/current_message_multi_device_not_urgent.json @@ -14,6 +14,13 @@ "destinationRegistrationId": 333, "content": "Zm9vYmFyego", "timestamp": 1234 + }, + { + "type": 1, + "destinationDeviceId": 3, + "destinationRegistrationId": 444, + "content": "Zm9vYmFyego", + "timestamp": 1234 } ] } diff --git a/service/src/test/resources/fixtures/current_message_multi_device_pni.json b/service/src/test/resources/fixtures/current_message_multi_device_pni.json index 0be6b7832..fb894d16b 100644 --- a/service/src/test/resources/fixtures/current_message_multi_device_pni.json +++ b/service/src/test/resources/fixtures/current_message_multi_device_pni.json @@ -12,5 +12,12 @@ "destinationRegistrationId" : 3333, "content" : "Zm9vYmFyego", "timestamp" : 1234 - }] + }, + { + "type" : 1, + "destinationDeviceId" : 3, + "destinationRegistrationId" : 4444, + "content" : "Zm9vYmFyego", + "timestamp" : 1234 + }] } diff --git a/service/src/test/resources/fixtures/current_message_registration_id.json b/service/src/test/resources/fixtures/current_message_registration_id.json index 1b7b03ad5..699b58cfb 100644 --- a/service/src/test/resources/fixtures/current_message_registration_id.json +++ b/service/src/test/resources/fixtures/current_message_registration_id.json @@ -12,5 +12,12 @@ "destinationRegistrationId" : 999, "content" : "Zm9vYmFyego", "timestamp" : 1234 - }] + }, + { + "type" : 1, + "destinationDeviceId" : 3, + "destinationRegistrationId" : 444, + "content" : "Zm9vYmFyego", + "timestamp" : 1234 + }] } diff --git a/service/src/test/resources/fixtures/missing_device_response.json b/service/src/test/resources/fixtures/missing_device_response.json index a0ad77d79..c3f9aabcc 100644 --- a/service/src/test/resources/fixtures/missing_device_response.json +++ b/service/src/test/resources/fixtures/missing_device_response.json @@ -1,4 +1,4 @@ { - "missingDevices" : [2], + "missingDevices" : [2, 3], "extraDevices" : [] -} \ No newline at end of file +}