diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java index e7916531c..09b411217 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/WhisperServerService.java @@ -253,7 +253,6 @@ import org.whispersystems.textsecuregcm.workers.CertificateCommand; import org.whispersystems.textsecuregcm.workers.CheckDynamicConfigurationCommand; import org.whispersystems.textsecuregcm.workers.DeleteUserCommand; import org.whispersystems.textsecuregcm.workers.MessagePersisterServiceCommand; -import org.whispersystems.textsecuregcm.workers.ProcessPushNotificationFeedbackCommand; import org.whispersystems.textsecuregcm.workers.RemoveExpiredAccountsCommand; import org.whispersystems.textsecuregcm.workers.RemoveExpiredBackupsCommand; import org.whispersystems.textsecuregcm.workers.RemoveExpiredLinkedDevicesCommand; @@ -314,7 +313,6 @@ public class WhisperServerService extends Application rereadAccount = accountsManager.getByAccountIdentifier(account.getUuid()); - if (rereadAccount.isEmpty()) { - // don't bother adding the uninstalled timestamp, the account is gone - return; - } - final Optional 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())); + final String originalFcmId = device.getGcmId(); + + // 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 rereadAccount = accountsManager.getByAccountIdentifier(account.getUuid()); + if (rereadAccount.isEmpty()) { + // Don't bother removing the token; the account is gone + return; } + + rereadAccount.get().getDevice(device.getId()).ifPresent(rereadDevice -> + accountsManager.updateDevice(rereadAccount.get(), device.getId(), d -> { + // Don't clear the token if it's already changed + if (originalFcmId.equals(d.getGcmId())) { + d.setGcmId(null); + } + })); } else { apnPushNotificationScheduler.cancelScheduledNotifications(account, device).whenComplete(logErrors()); } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Device.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Device.java index 24c5d1073..f2f428d0a 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Device.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Device.java @@ -56,9 +56,6 @@ public class Device { @JsonProperty private long pushTimestamp; - @JsonProperty - private long uninstalledFeedback; - @JsonProperty private boolean fetchesMessages; @@ -101,14 +98,6 @@ public class Device { this.voipApnId = voipApnId; } - public void setUninstalledFeedbackTimestamp(long uninstalledFeedback) { - this.uninstalledFeedback = uninstalledFeedback; - } - - public long getUninstalledFeedbackTimestamp() { - return uninstalledFeedback; - } - public void setLastSeen(long lastSeen) { this.lastSeen = lastSeen; } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommand.java deleted file mode 100644 index ffdb7ab4f..000000000 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommand.java +++ /dev/null @@ -1,160 +0,0 @@ -/* - * Copyright 2023 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.whispersystems.textsecuregcm.workers; - -import com.google.common.annotations.VisibleForTesting; -import io.micrometer.core.instrument.Metrics; -import io.micrometer.core.instrument.Tags; -import java.time.Clock; -import java.time.Duration; -import java.time.Instant; -import java.util.Optional; -import net.sourceforge.argparse4j.inf.Subparser; -import org.apache.commons.lang3.StringUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import org.whispersystems.textsecuregcm.metrics.MetricsUtil; -import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; -import org.whispersystems.textsecuregcm.storage.Account; -import org.whispersystems.textsecuregcm.storage.Device; -import reactor.core.publisher.Flux; -import reactor.core.publisher.Mono; - -public class ProcessPushNotificationFeedbackCommand extends AbstractSinglePassCrawlAccountsCommand { - - private final Clock clock; - - @VisibleForTesting - static final Duration MAX_TOKEN_REFRESH_DELAY = Duration.ofDays(14); - - @VisibleForTesting - static final String DRY_RUN_ARGUMENT = "dry-run"; - - private static final int MAX_CONCURRENCY = 16; - - private static final String EXPIRED_DEVICE_COUNTER_NAME = - MetricsUtil.name(ProcessPushNotificationFeedbackCommand.class, "expiredDevice"); - - private static final String RECOVERED_DEVICE_COUNTER_NAME = - MetricsUtil.name(ProcessPushNotificationFeedbackCommand.class, "recoveredDevice"); - - private static final Logger log = LoggerFactory.getLogger(ProcessPushNotificationFeedbackCommand.class); - - public ProcessPushNotificationFeedbackCommand(final Clock clock) { - super("process-push-notification-feedback", ""); - - this.clock = clock; - } - - @Override - public void configure(final Subparser subparser) { - super.configure(subparser); - - subparser.addArgument("--dry-run") - .type(Boolean.class) - .dest(DRY_RUN_ARGUMENT) - .required(false) - .setDefault(true) - .help("If true, don't actually modify accounts with stale devices"); - } - - @Override - protected void crawlAccounts(final Flux accounts) { - final boolean isDryRun = getNamespace().getBoolean(DRY_RUN_ARGUMENT); - - accounts - .filter(account -> account.getDevices().stream().anyMatch(this::deviceNeedsUpdate)) - .flatMap(account -> { - account.getDevices().stream() - .filter(this::deviceNeedsUpdate) - .forEach(device -> { - final Tags tags = Tags.of( - UserAgentTagUtil.PLATFORM_TAG, getPlatform(device), - "dryRun", String.valueOf(isDryRun)); - - if (deviceExpired(device)) { - Metrics.counter(EXPIRED_DEVICE_COUNTER_NAME, tags).increment(); - } else { - Metrics.counter(RECOVERED_DEVICE_COUNTER_NAME, tags).increment(); - } - }); - - if (isDryRun) { - return Mono.just(account); - } else { - return Mono.fromFuture(() -> getCommandDependencies().accountsManager().updateAsync(account, - a -> a.getDevices().stream() - .filter(this::deviceNeedsUpdate) - .forEach(device -> { - if (deviceExpired(device)) { - getUserAgent(device).ifPresent(device::setUserAgent); - - device.setGcmId(null); - device.setApnId(null); - device.setVoipApnId(null); - device.setFetchesMessages(false); - } else { - device.setUninstalledFeedbackTimestamp(0); - } - }))) - .onErrorResume(throwable -> { - log.warn("Failed to process push notification feedback for account {}", account.getUuid(), throwable); - return Mono.empty(); - }); - } - }, MAX_CONCURRENCY) - .then() - .block(); - } - - @VisibleForTesting - boolean pushFeedbackIntervalElapsed(final Device device) { - // After we get an indication that a device may have uninstalled the Signal app (`uninstalledFeedbackTimestamp` is - // non-zero), check back in after a few days to see what ultimately happened. - return device.getUninstalledFeedbackTimestamp() != 0 && - Instant.ofEpochMilli(device.getUninstalledFeedbackTimestamp()).plus(MAX_TOKEN_REFRESH_DELAY) - .isBefore(clock.instant()); - } - - @VisibleForTesting - boolean deviceExpired(final Device device) { - // If we received an indication that a device may have uninstalled the Signal app and we haven't seen that device in - // a few days since that event, we consider the device "expired" and should clean up its push tokens. If we have - // seen the device recently, though, we assume that the "uninstalled" hint was either incorrect or the device has - // since reinstalled the app and provided new push tokens. - return Instant.ofEpochMilli(device.getLastSeen()).plus(MAX_TOKEN_REFRESH_DELAY).isBefore(clock.instant()); - } - - @VisibleForTesting - boolean deviceNeedsUpdate(final Device device) { - return pushFeedbackIntervalElapsed(device) && (device.hasMessageDeliveryChannel() || device.getLastSeen() > device.getUninstalledFeedbackTimestamp()); - } - - @VisibleForTesting - static Optional getUserAgent(final Device device) { - if (StringUtils.isNotBlank(device.getApnId())) { - if (device.isPrimary()) { - return Optional.of("OWI"); - } else { - return Optional.of("OWP"); - } - } else if (StringUtils.isNotBlank(device.getGcmId())) { - return Optional.of("OWA"); - } - - return Optional.empty(); - } - - private static String getPlatform(final Device device) { - if (StringUtils.isNotBlank(device.getApnId())) { - return "ios"; - } else if (StringUtils.isNotBlank(device.getGcmId())) { - return "android"; - } - - return "unrecognized"; - } -} 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 00b7020fe..5b2665002 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/push/PushNotificationManagerTest.java @@ -26,7 +26,6 @@ import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.Device; import org.whispersystems.textsecuregcm.tests.util.AccountsHelper; -import org.whispersystems.textsecuregcm.util.Util; class PushNotificationManagerTest { @@ -153,7 +152,7 @@ class PushNotificationManagerTest { verify(fcmSender).sendNotification(pushNotification); verifyNoInteractions(apnSender); verify(accountsManager, never()).updateDevice(eq(account), eq(Device.PRIMARY_ID), any()); - verify(device, never()).setUninstalledFeedbackTimestamp(Util.todayInMillis()); + verify(device, never()).setGcmId(any()); verifyNoInteractions(apnPushNotificationScheduler); } @@ -211,7 +210,7 @@ class PushNotificationManagerTest { verifyNoInteractions(fcmSender); verify(accountsManager, never()).updateDevice(eq(account), eq(Device.PRIMARY_ID), any()); - verify(device, never()).setUninstalledFeedbackTimestamp(Util.todayInMillis()); + verify(device, never()).setGcmId(any()); verify(apnPushNotificationScheduler).scheduleRecurringVoipNotification(account, device); verify(apnPushNotificationScheduler, never()).scheduleBackgroundNotification(any(), any()); } @@ -236,7 +235,7 @@ class PushNotificationManagerTest { pushNotificationManager.sendNotification(pushNotification); verify(accountsManager).updateDevice(eq(account), eq(Device.PRIMARY_ID), any()); - verify(device).setUninstalledFeedbackTimestamp(Util.todayInMillis()); + verify(device).setGcmId(null); verifyNoInteractions(apnSender); verifyNoInteractions(apnPushNotificationScheduler); } @@ -262,7 +261,7 @@ class PushNotificationManagerTest { verifyNoInteractions(fcmSender); verify(accountsManager, never()).updateDevice(eq(account), eq(Device.PRIMARY_ID), any()); - verify(device, never()).setUninstalledFeedbackTimestamp(Util.todayInMillis()); + verify(device, never()).setGcmId(any()); verify(apnPushNotificationScheduler).cancelScheduledNotifications(account, device); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommandTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommandTest.java deleted file mode 100644 index 95eee6b32..000000000 --- a/service/src/test/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommandTest.java +++ /dev/null @@ -1,277 +0,0 @@ -/* - * Copyright 2023 Signal Messenger, LLC - * SPDX-License-Identifier: AGPL-3.0-only - */ - -package org.whispersystems.textsecuregcm.workers; - -import net.sourceforge.argparse4j.inf.Namespace; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.Arguments; -import org.junit.jupiter.params.provider.MethodSource; -import org.junit.jupiter.params.provider.ValueSource; -import org.whispersystems.textsecuregcm.storage.Account; -import org.whispersystems.textsecuregcm.storage.AccountsManager; -import org.whispersystems.textsecuregcm.storage.Device; -import reactor.core.publisher.Flux; - -import java.time.Clock; -import java.time.Instant; -import java.time.ZoneId; -import java.util.ArrayList; -import java.util.List; -import java.util.Optional; -import java.util.concurrent.CompletableFuture; -import java.util.function.Consumer; - -import static org.junit.jupiter.api.Assertions.*; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -class ProcessPushNotificationFeedbackCommandTest { - - private AccountsManager accountsManager; - private Clock clock; - - private ProcessPushNotificationFeedbackCommand processPushNotificationFeedbackCommand; - - private static final Instant CURRENT_TIME = Instant.now(); - - private static class TestProcessPushNotificationFeedbackCommand extends ProcessPushNotificationFeedbackCommand { - - private final CommandDependencies commandDependencies; - private final Namespace namespace; - - public TestProcessPushNotificationFeedbackCommand(final Clock clock, final AccountsManager accountsManager, final boolean isDryRun) { - super(clock); - - commandDependencies = mock(CommandDependencies.class); - when(commandDependencies.accountsManager()).thenReturn(accountsManager); - - namespace = mock(Namespace.class); - when(namespace.getBoolean(RemoveExpiredAccountsCommand.DRY_RUN_ARGUMENT)).thenReturn(isDryRun); - } - - @Override - protected CommandDependencies getCommandDependencies() { - return commandDependencies; - } - - @Override - protected Namespace getNamespace() { - return namespace; - } - } - - @BeforeEach - void setUpBeforeEach() { - accountsManager = mock(AccountsManager.class); - - when(accountsManager.updateAsync(any(), any())) - .thenAnswer(invocation -> { - final Account account = invocation.getArgument(0); - final Consumer accountUpdater = invocation.getArgument(1); - - accountUpdater.accept(account); - - return CompletableFuture.completedFuture(account); - }); - - clock = Clock.fixed(CURRENT_TIME, ZoneId.systemDefault()); - - processPushNotificationFeedbackCommand = - new TestProcessPushNotificationFeedbackCommand(clock, accountsManager, true); - } - - @ParameterizedTest - @ValueSource(booleans = {true, false}) - void crawlAccounts(final boolean isDryRun) { - processPushNotificationFeedbackCommand = - new TestProcessPushNotificationFeedbackCommand(clock, accountsManager, isDryRun); - - final Account accountWithActiveDevice = mock(Account.class); - { - final Device device = mock(Device.class); - when(device.hasMessageDeliveryChannel()).thenReturn(true); - - when(accountWithActiveDevice.getDevices()).thenReturn(List.of(device)); - } - - final Account accountWithUninstalledDevice = mock(Account.class); - { - final Device uninstalledDevice = mock(Device.class); - when(uninstalledDevice.hasMessageDeliveryChannel()).thenReturn(true); - when(uninstalledDevice.getUninstalledFeedbackTimestamp()) - .thenReturn(clock.instant().minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.multipliedBy(2)).toEpochMilli()); - - when(accountWithUninstalledDevice.getDevices()).thenReturn(List.of(uninstalledDevice)); - } - - final Account accountWithAlreadyDisabledUninstalledDevice = mock(Account.class); - { - final Device previouslyDisabledUninstalledDevice = mock(Device.class); - when(previouslyDisabledUninstalledDevice.hasMessageDeliveryChannel()).thenReturn(false); - when(previouslyDisabledUninstalledDevice.getUninstalledFeedbackTimestamp()) - .thenReturn(clock.instant().minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.multipliedBy(2)).toEpochMilli()); - - when(accountWithAlreadyDisabledUninstalledDevice.getDevices()).thenReturn(List.of(previouslyDisabledUninstalledDevice)); - } - - processPushNotificationFeedbackCommand.crawlAccounts( - Flux.just(accountWithActiveDevice, accountWithUninstalledDevice, accountWithAlreadyDisabledUninstalledDevice)); - - if (isDryRun) { - verify(accountsManager, never()).updateAsync(any(), any()); - } else { - verify(accountsManager, never()).updateAsync(eq(accountWithActiveDevice), any()); - verify(accountsManager).updateAsync(eq(accountWithUninstalledDevice), any()); - verify(accountsManager, never()).updateAsync(eq(accountWithAlreadyDisabledUninstalledDevice), any()); - } - } - - @Test - void pushFeedbackIntervalElapsed() { - { - final Device deviceWithMaturePushNotificationFeedback = mock(Device.class); - when(deviceWithMaturePushNotificationFeedback.getUninstalledFeedbackTimestamp()) - .thenReturn(clock.instant().minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.multipliedBy(2)).toEpochMilli()); - - assertTrue(processPushNotificationFeedbackCommand.pushFeedbackIntervalElapsed(deviceWithMaturePushNotificationFeedback)); - } - - { - final Device deviceWithRecentPushNotificationFeedback = mock(Device.class); - when(deviceWithRecentPushNotificationFeedback.getUninstalledFeedbackTimestamp()) - .thenReturn(clock.instant().minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.dividedBy(2)).toEpochMilli()); - - assertFalse(processPushNotificationFeedbackCommand.pushFeedbackIntervalElapsed(deviceWithRecentPushNotificationFeedback)); - } - - { - final Device deviceWithoutPushNotificationFeedback = mock(Device.class); - - assertFalse(processPushNotificationFeedbackCommand.pushFeedbackIntervalElapsed(deviceWithoutPushNotificationFeedback)); - } - } - - @Test - void deviceExpired() { - { - final Device expiredDevice = mock(Device.class); - when(expiredDevice.getLastSeen()) - .thenReturn( - clock.instant().minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.multipliedBy(2)) - .toEpochMilli()); - - assertTrue(processPushNotificationFeedbackCommand.deviceExpired(expiredDevice)); - } - - { - final Device activeDevice = mock(Device.class); - when(activeDevice.getLastSeen()).thenReturn(clock.instant().toEpochMilli()); - - assertFalse(processPushNotificationFeedbackCommand.deviceExpired(activeDevice)); - } - } - - @ParameterizedTest - @MethodSource - void deviceNeedsUpdate(final Device device, final boolean expectNeedsUpdate) { - assertEquals(expectNeedsUpdate, processPushNotificationFeedbackCommand.deviceNeedsUpdate(device)); - } - - private static List deviceNeedsUpdate() { - final long maturePushFeedbackTimestamp = - CURRENT_TIME.minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.multipliedBy(2)) - .toEpochMilli(); - - final List arguments = new ArrayList<>(); - - { - // Device is active, enabled, and has no push feedback - final Device device = mock(Device.class); - when(device.hasMessageDeliveryChannel()).thenReturn(true); - when(device.getUninstalledFeedbackTimestamp()).thenReturn(0L); - when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); - - arguments.add(Arguments.of(device, false)); - } - - { - // Device is active, but not enabled, and has no push feedback - final Device device = mock(Device.class); - when(device.hasMessageDeliveryChannel()).thenReturn(false); - when(device.getUninstalledFeedbackTimestamp()).thenReturn(0L); - when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); - - arguments.add(Arguments.of(device, false)); - } - - { - // Device is active, enabled, and has "mature" push feedback - final Device device = mock(Device.class); - when(device.hasMessageDeliveryChannel()).thenReturn(true); - when(device.getUninstalledFeedbackTimestamp()).thenReturn(maturePushFeedbackTimestamp); - when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); - - arguments.add(Arguments.of(device, true)); - } - - { - // Device is active, but not enabled, and has "mature" push feedback - final Device device = mock(Device.class); - when(device.hasMessageDeliveryChannel()).thenReturn(false); - when(device.getUninstalledFeedbackTimestamp()).thenReturn(maturePushFeedbackTimestamp); - when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); - - arguments.add(Arguments.of(device, true)); - } - - { - // Device is inactive, not enabled, and has "mature" push feedback - final Device device = mock(Device.class); - when(device.hasMessageDeliveryChannel()).thenReturn(false); - when(device.getUninstalledFeedbackTimestamp()).thenReturn(maturePushFeedbackTimestamp); - when(device.getLastSeen()).thenReturn(maturePushFeedbackTimestamp); - - arguments.add(Arguments.of(device, false)); - } - - return arguments; - } - - @SuppressWarnings("OptionalUsedAsFieldOrParameterType") - @ParameterizedTest - @MethodSource - void getUserAgent(final Device device, final Optional expectedUserAgentString) { - assertEquals(expectedUserAgentString, ProcessPushNotificationFeedbackCommand.getUserAgent(device)); - } - - private static List getUserAgent() { - final Device iosPrimaryDevice = mock(Device.class); - when(iosPrimaryDevice.isPrimary()).thenReturn(true); - when(iosPrimaryDevice.getApnId()).thenReturn("apns-token"); - - final Device iosLinkedDevice = mock(Device.class); - when(iosLinkedDevice.isPrimary()).thenReturn(false); - when(iosLinkedDevice.getApnId()).thenReturn("apns-token"); - - final Device androidDevice = mock(Device.class); - when(androidDevice.getGcmId()).thenReturn("gcm-id"); - - final Device deviceWithoutTokens = mock(Device.class); - - return List.of( - Arguments.of(iosPrimaryDevice, Optional.of("OWI")), - Arguments.of(iosLinkedDevice, Optional.of("OWP")), - Arguments.of(androidDevice, Optional.of("OWA")), - Arguments.of(deviceWithoutTokens, Optional.empty()) - ); - } -}