From 23ca011ac1c1634fd01ac8c4818c0a59a8c0b600 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Fri, 16 Oct 2020 15:04:55 -0400 Subject: [PATCH] Record account deletion reasons. --- .../controllers/AccountController.java | 2 +- .../textsecuregcm/storage/AccountCleaner.java | 2 +- .../storage/AccountsManager.java | 24 +++++++++++++++---- .../workers/DeleteUserCommand.java | 2 +- .../controllers/AccountControllerTest.java | 2 +- .../tests/storage/AccountCleanerTest.java | 10 ++++---- 6 files changed, 30 insertions(+), 12 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index aface157b..9c5a620d2 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -604,7 +604,7 @@ public class AccountController { @DELETE @Path("/me") public void deleteAccount(@Auth Account account) { - accounts.delete(account); + accounts.delete(account, AccountsManager.DeletionReason.USER_REQUEST); } private boolean shouldAutoBlock(String requester) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountCleaner.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountCleaner.java index 61ef14f0c..0e9506f36 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountCleaner.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountCleaner.java @@ -68,7 +68,7 @@ public class AccountCleaner extends AccountDatabaseCrawlerListener { expiredAccountsMeter.mark(); if (accountUpdateCount < MAX_ACCOUNT_UPDATES_PER_CHUNK) { - accountsManager.delete(account); + accountsManager.delete(account, AccountsManager.DeletionReason.EXPIRED); accountUpdateCount++; } } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java index 2b3dcde1c..b65fb951c 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -56,8 +56,9 @@ public class AccountsManager { private static final Timer redisUuidGetTimer = metricRegistry.timer(name(AccountsManager.class, "redisUuidGet" )); private static final Timer redisDeleteTimer = metricRegistry.timer(name(AccountsManager.class, "redisDelete" )); - private static final String DELETE_COUNTER_NAME = name(AccountsManager.class, "deleteCounter"); - private static final String COUNTRY_CODE_TAG_NAME = "country"; + private static final String DELETE_COUNTER_NAME = name(AccountsManager.class, "deleteCounter"); + private static final String COUNTRY_CODE_TAG_NAME = "country"; + private static final String DELETION_REASON_TAG_NAME = "reason"; private final Logger logger = LoggerFactory.getLogger(AccountsManager.class); @@ -72,6 +73,18 @@ public class AccountsManager { private final ProfilesManager profilesManager; private final ObjectMapper mapper; + public enum DeletionReason { + ADMIN_DELETED("admin"), + EXPIRED ("expired"), + USER_REQUEST ("userRequest"); + + private final String tagValue; + + DeletionReason(final String tagValue) { + this.tagValue = tagValue; + } + } + public AccountsManager(Accounts accounts, DirectoryManager directory, FaultTolerantRedisCluster cacheCluster, final DirectoryQueue directoryQueue, final Keys keys, final MessagesManager messagesManager, final UsernamesManager usernamesManager, final ProfilesManager profilesManager) { this.accounts = accounts; this.directory = directory; @@ -143,7 +156,7 @@ public class AccountsManager { return accounts.getAllFrom(uuid, length); } - public void delete(final Account account) { + public void delete(final Account account, final DeletionReason deletionReason) { try (final Timer.Context ignored = deleteTimer.time()) { usernamesManager.delete(account.getUuid()); directoryQueue.deleteAccount(account); @@ -155,7 +168,10 @@ public class AccountsManager { databaseDelete(account); } - Metrics.counter(DELETE_COUNTER_NAME, COUNTRY_CODE_TAG_NAME, Util.getCountryCode(account.getNumber())).increment(); + Metrics.counter(DELETE_COUNTER_NAME, + COUNTRY_CODE_TAG_NAME, Util.getCountryCode(account.getNumber()), + DELETION_REASON_TAG_NAME, deletionReason.tagValue) + .increment(); } private void updateDirectory(Account account) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java index 9d1b163d6..4797d983e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/DeleteUserCommand.java @@ -106,7 +106,7 @@ public class DeleteUserCommand extends EnvironmentCommand account = accountsManager.get(user); if (account.isPresent()) { - accountsManager.delete(account.get()); + accountsManager.delete(account.get(), AccountsManager.DeletionReason.ADMIN_DELETED); logger.warn("Removed " + account.get().getNumber()); } else { logger.warn("Account not found"); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java index 344a53834..a2ee463fd 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/AccountControllerTest.java @@ -1187,6 +1187,6 @@ public class AccountControllerTest { .delete(); assertThat(response.getStatus()).isEqualTo(204); - verify(accountsManager).delete(AuthHelper.VALID_ACCOUNT); + verify(accountsManager).delete(AuthHelper.VALID_ACCOUNT, AccountsManager.DeletionReason.USER_REQUEST); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountCleanerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountCleanerTest.java index 962b140eb..009425bba 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountCleanerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/storage/AccountCleanerTest.java @@ -31,6 +31,8 @@ import java.util.Optional; import java.util.UUID; import java.util.concurrent.TimeUnit; +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.times; @@ -88,9 +90,9 @@ public class AccountCleanerTest { accountCleaner.timeAndProcessCrawlChunk(Optional.empty(), Arrays.asList(deletedDisabledAccount, undeletedDisabledAccount, undeletedEnabledAccount)); accountCleaner.onCrawlEnd(Optional.empty()); - verify(accountsManager, never()).delete(deletedDisabledAccount); - verify(accountsManager).delete(undeletedDisabledAccount); - verify(accountsManager, never()).delete(undeletedEnabledAccount); + verify(accountsManager, never()).delete(eq(deletedDisabledAccount), any()); + verify(accountsManager).delete(undeletedDisabledAccount, AccountsManager.DeletionReason.EXPIRED); + verify(accountsManager, never()).delete(eq(undeletedEnabledAccount), any()); verifyNoMoreInteractions(accountsManager); } @@ -112,7 +114,7 @@ public class AccountCleanerTest { accountCleaner.timeAndProcessCrawlChunk(Optional.empty(), accounts); accountCleaner.onCrawlEnd(Optional.empty()); - verify(accountsManager, times(AccountCleaner.MAX_ACCOUNT_UPDATES_PER_CHUNK)).delete(undeletedDisabledAccount); + verify(accountsManager, times(AccountCleaner.MAX_ACCOUNT_UPDATES_PER_CHUNK)).delete(undeletedDisabledAccount, AccountsManager.DeletionReason.EXPIRED); verifyNoMoreInteractions(accountsManager); }