Skip to content

Commit

Permalink
Gracefully handle NotPushRegisteredException
Browse files Browse the repository at this point in the history
  • Loading branch information
@jon-signal
jon-signal committed Jun 25, 2024
1 parent 2619569 commit cb5cd64
Show file tree
Hide file tree
Showing 7 changed files with 68 additions and 156 deletions.
105 changes: 40 additions & 65 deletions service/src/main/java/org/whispersystems/textsecuregcm/controllers/MessageController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@
import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil;
import org.whispersystems.textsecuregcm.providers.MultiRecipientMessageProvider;
import org.whispersystems.textsecuregcm.push.MessageSender;
import org.whispersystems.textsecuregcm.push.NotPushRegisteredException;
import org.whispersystems.textsecuregcm.push.PushNotificationManager;
import org.whispersystems.textsecuregcm.push.ReceiptSender;
import org.whispersystems.textsecuregcm.spam.ReportSpamTokenProvider;
Expand Down Expand Up @@ -180,7 +179,6 @@ private record MultiRecipientDeliveryData(
private static final String RATE_LIMITED_MESSAGE_COUNTER_NAME = name(MessageController.class, "rateLimitedMessage");

private static final String REJECT_INVALID_ENVELOPE_TYPE = name(MessageController.class, "rejectInvalidEnvelopeType");
private static final String UNEXPECTED_MISSING_USER_COUNTER_NAME = name(MessageController.class, "unexpectedMissingDestinationForMultiRecipientMessage");
private static final Timer SEND_MESSAGE_LATENCY_TIMER =
Timer.builder(MetricsUtil.name(MessageController.class, "sendMessageLatency"))
.publishPercentileHistogram(true)
Expand Down Expand Up @@ -434,8 +432,6 @@ public Response sendMessage(@ReadOnly @Auth Optional<AuthenticatedAccount> sourc
}

return Response.ok(new SendMessageResponse(needsSync)).build();
} catch (NoSuchUserException e) {
throw new WebApplicationException(Response.status(404).build());
} catch (MismatchedDevicesException e) {
throw new WebApplicationException(Response.status(409)
.type(MediaType.APPLICATION_JSON_TYPE)
Expand Down Expand Up @@ -627,8 +623,6 @@ public Response sendMultiRecipientMessage(
.build();
}

List<ServiceIdentifier> uuids404 = Collections.synchronizedList(new ArrayList<>());

try {
CompletableFuture.allOf(
recipients.values().stream()
Expand All @@ -649,19 +643,12 @@ public Response sendMultiRecipientMessage(

// we asserted this must exist in validateCompleteDeviceList
final Device destinationDevice = destinationAccount.getDevice(deviceId).orElseThrow();
try {
sentMessageCounter.increment();
sendCommonPayloadMessage(
destinationAccount, destinationDevice, recipientData.serviceIdentifier(), timestamp,
online,
isStory, isUrgent, payload);
} catch (NoSuchUserException e) {
// this should never happen, because we already asserted the device is present and enabled
Metrics.counter(
UNEXPECTED_MISSING_USER_COUNTER_NAME,
Tags.of("isPrimary", String.valueOf(destinationDevice.isPrimary()))).increment();
uuids404.add(recipientData.serviceIdentifier());
}

sentMessageCounter.increment();
sendCommonPayloadMessage(
destinationAccount, destinationDevice, recipientData.serviceIdentifier(), timestamp,
online,
isStory, isUrgent, payload);
},
multiRecipientMessageExecutor));
})
Expand All @@ -677,7 +664,7 @@ public Response sendMultiRecipientMessage(
logger.error("partial failure while delivering multi-recipient messages", e.getCause());
return Response.serverError().entity("failure during delivery").build();
}
return Response.ok(new SendMultiRecipientMessageResponse(uuids404)).build();
return Response.ok(new SendMultiRecipientMessageResponse(Collections.emptyList())).build();
}

private void checkGroupSendToken(
Expand Down Expand Up @@ -858,32 +845,27 @@ private void sendIndividualMessage(
boolean urgent,
IncomingMessage incomingMessage,
String userAgentString,
Optional<byte[]> spamReportToken)
throws NoSuchUserException {
try {
final Envelope envelope;
Optional<byte[]> spamReportToken) {

try {
Account sourceAccount = source.map(AuthenticatedAccount::getAccount).orElse(null);
Byte sourceDeviceId = source.map(account -> account.getAuthenticatedDevice().getId()).orElse(null);
envelope = incomingMessage.toEnvelope(
destinationIdentifier,
sourceAccount,
sourceDeviceId,
timestamp == 0 ? System.currentTimeMillis() : timestamp,
story,
urgent,
spamReportToken.orElse(null));
} catch (final IllegalArgumentException e) {
logger.warn("Received bad envelope type {} from {}", incomingMessage.type(), userAgentString);
throw new BadRequestException(e);
}
final Envelope envelope;

messageSender.sendMessage(destinationAccount, destinationDevice, envelope, online);
} catch (NotPushRegisteredException e) {
if (destinationDevice.isPrimary()) throw new NoSuchUserException(e);
else logger.debug("Not registered", e);
try {
final Account sourceAccount = source.map(AuthenticatedAccount::getAccount).orElse(null);
final Byte sourceDeviceId = source.map(account -> account.getAuthenticatedDevice().getId()).orElse(null);
envelope = incomingMessage.toEnvelope(
destinationIdentifier,
sourceAccount,
sourceDeviceId,
timestamp == 0 ? System.currentTimeMillis() : timestamp,
story,
urgent,
spamReportToken.orElse(null));
} catch (final IllegalArgumentException e) {
logger.warn("Received bad envelope type {} from {}", incomingMessage.type(), userAgentString);
throw new BadRequestException(e);
}

messageSender.sendMessage(destinationAccount, destinationDevice, envelope, online);
}

private void sendCommonPayloadMessage(Account destinationAccount,
Expand All @@ -893,28 +875,21 @@ private void sendCommonPayloadMessage(Account destinationAccount,
boolean online,
boolean story,
boolean urgent,
byte[] payload) throws NoSuchUserException {
try {
Envelope.Builder messageBuilder = Envelope.newBuilder();
long serverTimestamp = System.currentTimeMillis();

messageBuilder
.setType(Type.UNIDENTIFIED_SENDER)
.setTimestamp(timestamp == 0 ? serverTimestamp : timestamp)
.setServerTimestamp(serverTimestamp)
.setContent(ByteString.copyFrom(payload))
.setStory(story)
.setUrgent(urgent)
.setDestinationUuid(serviceIdentifier.toServiceIdentifierString());

messageSender.sendMessage(destinationAccount, destinationDevice, messageBuilder.build(), online);
} catch (NotPushRegisteredException e) {
if (destinationDevice.isPrimary()) {
throw new NoSuchUserException(e);
} else {
logger.debug("Not registered", e);
}
}
byte[] payload) {

final Envelope.Builder messageBuilder = Envelope.newBuilder();
final long serverTimestamp = System.currentTimeMillis();

messageBuilder
.setType(Type.UNIDENTIFIED_SENDER)
.setTimestamp(timestamp == 0 ? serverTimestamp : timestamp)
.setServerTimestamp(serverTimestamp)
.setContent(ByteString.copyFrom(payload))
.setStory(story)
.setUrgent(urgent)
.setDestinationUuid(serviceIdentifier.toServiceIdentifierString());

messageSender.sendMessage(destinationAccount, destinationDevice, messageBuilder.build(), online);
}

private void checkMessageRateLimit(AuthenticatedAccount source, Account destination, String userAgent)
Expand Down
18 changes: 0 additions & 18 deletions service/src/main/java/org/whispersystems/textsecuregcm/controllers/NoSuchUserException.java
View file

This file was deleted.

10 changes: 3 additions & 7 deletions service/src/main/java/org/whispersystems/textsecuregcm/push/MessageSender.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ public MessageSender(ClientPresenceManager clientPresenceManager,
this.pushLatencyManager = pushLatencyManager;
}

public void sendMessage(final Account account, final Device device, final Envelope message, final boolean online)
throws NotPushRegisteredException {
public void sendMessage(final Account account, final Device device, final Envelope message, final boolean online) {

final String channel;

Expand All @@ -64,7 +63,7 @@ public void sendMessage(final Account account, final Device device, final Envelo
} else if (device.getFetchesMessages()) {
channel = "websocket";
} else {
throw new AssertionError();
channel = "none";
}

final boolean clientPresent;
Expand All @@ -89,10 +88,7 @@ public void sendMessage(final Account account, final Device device, final Envelo

final boolean useVoip = StringUtils.isNotBlank(device.getVoipApnId());
RedisOperation.unchecked(() -> pushLatencyManager.recordPushSent(account.getUuid(), device.getId(), useVoip, message.getUrgent()));
} catch (final NotPushRegisteredException e) {
if (!device.getFetchesMessages()) {
throw e;
}
} catch (final NotPushRegisteredException ignored) {
}
}
}
Expand Down
2 changes: 0 additions & 2 deletions service/src/main/java/org/whispersystems/textsecuregcm/push/ReceiptSender.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ public void sendReceipt(ServiceIdentifier sourceIdentifier, byte sourceDeviceId,
for (final Device destinationDevice : destinationAccount.getDevices()) {
try {
messageSender.sendMessage(destinationAccount, destinationDevice, message.build(), false);
} catch (final NotPushRegisteredException e) {
logger.debug("User no longer push registered for delivery receipt: {}", e.getMessage());
} catch (final Exception e) {
logger.warn("Could not send delivery receipt", e);
}
Expand Down
33 changes: 15 additions & 18 deletions service/src/main/java/org/whispersystems/textsecuregcm/storage/ChangeNumberManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,23 +130,20 @@ void sendMessageToSelf(
logger.debug("destination device not present");
return;
}
try {
long serverTimestamp = System.currentTimeMillis();
Envelope envelope = Envelope.newBuilder()
.setType(Envelope.Type.forNumber(message.type()))
.setTimestamp(serverTimestamp)
.setServerTimestamp(serverTimestamp)
.setDestinationUuid(new AciServiceIdentifier(sourceAndDestinationAccount.getUuid()).toServiceIdentifierString())
.setContent(ByteString.copyFrom(contents.get()))
.setSourceUuid(new AciServiceIdentifier(sourceAndDestinationAccount.getUuid()).toServiceIdentifierString())
.setSourceDevice((int) Device.PRIMARY_ID)
.setUpdatedPni(sourceAndDestinationAccount.getPhoneNumberIdentifier().toString())
.setUrgent(true)
.build();

messageSender.sendMessage(sourceAndDestinationAccount, destinationDevice.get(), envelope, false);
} catch (NotPushRegisteredException e) {
logger.debug("Not registered", e);
}

final long serverTimestamp = System.currentTimeMillis();
final Envelope envelope = Envelope.newBuilder()
.setType(Envelope.Type.forNumber(message.type()))
.setTimestamp(serverTimestamp)
.setServerTimestamp(serverTimestamp)
.setDestinationUuid(new AciServiceIdentifier(sourceAndDestinationAccount.getUuid()).toServiceIdentifierString())
.setContent(ByteString.copyFrom(contents.get()))
.setSourceUuid(new AciServiceIdentifier(sourceAndDestinationAccount.getUuid()).toServiceIdentifierString())
.setSourceDevice((int) Device.PRIMARY_ID)
.setUpdatedPni(sourceAndDestinationAccount.getPhoneNumberIdentifier().toString())
.setUrgent(true)
.build();

messageSender.sendMessage(sourceAndDestinationAccount, destinationDevice.get(), envelope, false);
}
}
46 changes: 0 additions & 46 deletions ...ice/src/test/java/org/whispersystems/textsecuregcm/controllers/MessageControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.anyBoolean;
import static org.mockito.Mockito.anyString;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.reset;
Expand Down Expand Up @@ -109,7 +108,6 @@
import org.whispersystems.textsecuregcm.metrics.MessageMetrics;
import org.whispersystems.textsecuregcm.providers.MultiRecipientMessageProvider;
import org.whispersystems.textsecuregcm.push.MessageSender;
import org.whispersystems.textsecuregcm.push.NotPushRegisteredException;
import org.whispersystems.textsecuregcm.push.PushNotificationManager;
import org.whispersystems.textsecuregcm.push.ReceiptSender;
import org.whispersystems.textsecuregcm.spam.ReportSpamTokenProvider;
Expand Down Expand Up @@ -1681,50 +1679,6 @@ private static Stream<Arguments> sendMultiRecipientMessageStaleDevices() {
Arguments.of(MULTI_DEVICE_PNI_ID));
}

@ParameterizedTest
@MethodSource
void sendMultiRecipientMessage404(final ServiceIdentifier serviceIdentifier, final int regId1, final int regId2)
throws NotPushRegisteredException {

final List<Recipient> recipients = List.of(
new Recipient(serviceIdentifier, MULTI_DEVICE_ID1, regId1, new byte[48]),
new Recipient(serviceIdentifier, MULTI_DEVICE_ID2, regId2, new byte[48]));

// initialize our binary payload and create an input stream
byte[] buffer = new byte[2048];
// InputStream stream = initializeMultiPayload(recipientUUID, buffer);
InputStream stream = initializeMultiPayload(recipients, buffer, true);

// set up the entity to use in our PUT request
Entity<InputStream> entity = Entity.entity(stream, MultiRecipientMessageProvider.MEDIA_TYPE);

// start building the request
final Invocation.Builder invocationBuilder = resources
.getJerseyTest()
.target("/v1/messages/multi_recipient")
.queryParam("online", false)
.queryParam("ts", System.currentTimeMillis())
.queryParam("story", true)
.queryParam("urgent", true)
.request()
.header(HttpHeaders.USER_AGENT, "FIXME")
.header(HeaderUtils.UNIDENTIFIED_ACCESS_KEY, Base64.getEncoder().encodeToString(UNIDENTIFIED_ACCESS_BYTES));

doThrow(NotPushRegisteredException.class)
.when(messageSender).sendMessage(any(), any(), any(), anyBoolean());

// make the PUT request
final SendMultiRecipientMessageResponse response = invocationBuilder.put(entity, SendMultiRecipientMessageResponse.class);

assertEquals(List.of(serviceIdentifier), response.uuids404());
}

private static Stream<Arguments> sendMultiRecipientMessage404() {
return Stream.of(
Arguments.of(MULTI_DEVICE_ACI_ID, MULTI_DEVICE_REG_ID1, MULTI_DEVICE_REG_ID2),
Arguments.of(MULTI_DEVICE_PNI_ID, MULTI_DEVICE_PNI_REG_ID1, MULTI_DEVICE_PNI_REG_ID2));
}

@Test
void sendMultiRecipientMessageStoryRateLimited() {
final List<Recipient> recipients = List.of(new Recipient(SINGLE_DEVICE_ACI_ID, SINGLE_DEVICE_ID1, SINGLE_DEVICE_REG_ID1, new byte[48]));
Expand Down
10 changes: 10 additions & 0 deletions service/src/test/java/org/whispersystems/textsecuregcm/push/MessageSenderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,16 @@ void testSendMessageFetchClientNotPresent() throws Exception {
verify(messagesManager).insert(ACCOUNT_UUID, DEVICE_ID, message);
}

@Test
void testSendMessageNoChannel() {
when(device.getGcmId()).thenReturn(null);
when(device.getApnId()).thenReturn(null);
when(device.getFetchesMessages()).thenReturn(false);

assertDoesNotThrow(() -> messageSender.sendMessage(account, device, message, false));
verify(messagesManager).insert(ACCOUNT_UUID, DEVICE_ID, message);
}

private MessageProtos.Envelope generateRandomMessage() {
return MessageProtos.Envelope.newBuilder()
.setTimestamp(System.currentTimeMillis())
Expand Down

0 comments on commit cb5cd64

Please sign in to comment.