From 2adf1e501760743193b201ec1bdb51ecb18ee665 Mon Sep 17 00:00:00 2001
From: ravi-signal <99042880+ravi-signal@users.noreply.github.com>
Date: Wed, 28 Feb 2024 15:18:12 -0600
Subject: [PATCH] Avoid modification of Account from `@ReadOnly` endpoint

---
 .../push/PushNotificationManager.java            | 16 +++++++++++++++-
 .../push/PushNotificationManagerTest.java        |  4 +++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotificationManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotificationManager.java
index 03dca8555..76fafd5af 100644
--- a/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotificationManager.java
+++ b/service/src/main/java/org/whispersystems/textsecuregcm/push/PushNotificationManager.java
@@ -10,6 +10,7 @@ import static org.whispersystems.textsecuregcm.metrics.MetricsUtil.name;
 import com.google.common.annotations.VisibleForTesting;
 import io.micrometer.core.instrument.Metrics;
 import io.micrometer.core.instrument.Tags;
+import java.util.Optional;
 import java.util.function.BiConsumer;
 import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
@@ -167,7 +168,20 @@ public class PushNotificationManager {
   private void handleDeviceUnregistered(final Account account, final Device device) {
     if (StringUtils.isNotBlank(device.getGcmId())) {
       if (device.getUninstalledFeedbackTimestamp() == 0) {
-        accountsManager.updateDevice(account, device.getId(), d ->
+        // Reread the account to avoid marking the caller's account as stale. The consumers of this class tend to
+        // promise not to modify accounts. There's no need to force the caller to be considered mutable just for
+        // updating an uninstalled feedback timestamp though.
+        final Optional<Account> rereadAccount = accountsManager.getByAccountIdentifier(account.getUuid());
+        if (rereadAccount.isEmpty()) {
+          // don't bother adding the uninstalled timestamp, the account is gone
+          return;
+        }
+        final Optional<Device> rereadDevice = rereadAccount.get().getDevice(device.getId());
+        if (rereadDevice.map(Device::getUninstalledFeedbackTimestamp).orElse(-1L) != 0) {
+          // don't bother adding the uninstalled timestamp, the device is gone or already updated
+          return;
+        }
+        accountsManager.updateDevice(rereadAccount.get(), device.getId(), d ->
             d.setUninstalledFeedbackTimestamp(Util.todayInMillis()));
       }
     } else {
diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java
index dee011d2c..00b7020fe 100644
--- a/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java
+++ b/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java
@@ -220,10 +220,12 @@ class PushNotificationManagerTest {
   void testSendNotificationUnregisteredFcm() {
     final Account account = mock(Account.class);
     final Device device = mock(Device.class);
-
+    final UUID aci = UUID.randomUUID();
     when(device.getId()).thenReturn(Device.PRIMARY_ID);
     when(device.getGcmId()).thenReturn("token");
     when(account.getDevice(Device.PRIMARY_ID)).thenReturn(Optional.of(device));
+    when(account.getUuid()).thenReturn(aci);
+    when(accountsManager.getByAccountIdentifier(aci)).thenReturn(Optional.of(account));
 
     final PushNotification pushNotification = new PushNotification(
         "token", PushNotification.TokenType.FCM, PushNotification.NotificationType.NOTIFICATION, null, account, device, true);