From fe86e15d80e2d688a9f28a65c1b631bca4a90839 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Mon, 28 Feb 2022 17:33:54 -0500 Subject: [PATCH] Remove PNI repair code --- .../textsecuregcm/storage/Accounts.java | 28 ++------- .../textsecuregcm/storage/AccountsTest.java | 60 ------------------- 2 files changed, 6 insertions(+), 82 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java index c2cf1e80b..2cf36cfb2 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java @@ -520,7 +520,7 @@ public class Accounts extends AbstractDynamoDbStore { return Optional.ofNullable(response.item()) .map(item -> item.get(KEY_ACCOUNT_UUID)) .map(this::accountByUuid) - .map(this::fromItem); + .map(Accounts::fromItem); }); } @@ -535,7 +535,7 @@ public class Accounts extends AbstractDynamoDbStore { return Optional.ofNullable(response.item()) .map(item -> item.get(KEY_ACCOUNT_UUID)) .map(this::accountByUuid) - .map(this::fromItem); + .map(Accounts::fromItem); }); } @@ -550,7 +550,7 @@ public class Accounts extends AbstractDynamoDbStore { return Optional.ofNullable(response.item()) .map(item -> item.get(KEY_ACCOUNT_UUID)) .map(this::accountByUuid) - .map(this::fromItem); + .map(Accounts::fromItem); }); } @@ -566,7 +566,7 @@ public class Accounts extends AbstractDynamoDbStore { public Optional getByAccountIdentifier(UUID uuid) { return GET_BY_UUID_TIMER.record(() -> Optional.ofNullable(accountByUuid(AttributeValues.fromUUID(uuid))) - .map(this::fromItem)); + .map(Accounts::fromItem)); } public void delete(UUID uuid) { @@ -633,7 +633,7 @@ public class Accounts extends AbstractDynamoDbStore { final List accounts = timer.record(() -> scan(scanRequestBuilder.build(), maxCount) .stream() - .map(this::fromItem) + .map(Accounts::fromItem) .collect(Collectors.toList())); return new AccountCrawlChunk(accounts, accounts.size() > 0 ? accounts.get(accounts.size() - 1).getUuid() : null); @@ -645,9 +645,8 @@ public class Accounts extends AbstractDynamoDbStore { .collect(Collectors.joining(", ")); } - // TODO Make this static once PNI repairs are complete and the call to #update(Account) is no longer needed @VisibleForTesting - Account fromItem(Map item) { + static Account fromItem(Map item) { if (!item.containsKey(ATTR_ACCOUNT_DATA) || !item.containsKey(ATTR_ACCOUNT_E164) || // TODO: eventually require ATTR_CANONICALLY_DISCOVERABLE @@ -667,27 +666,12 @@ public class Accounts extends AbstractDynamoDbStore { accountIdentifier, account.getPhoneNumberIdentifier(), phoneNumberIdentifierFromAttribute); } - boolean shouldResetPni = false; - - if (account.getPhoneNumberIdentifier() == null && phoneNumberIdentifierFromAttribute != null) { - log.info("Account {} has a PNI in its attributes, but not JSON blob; will repair", accountIdentifier); - shouldResetPni = true; - } - account.setNumber(item.get(ATTR_ACCOUNT_E164).s(), phoneNumberIdentifierFromAttribute); account.setUuid(accountIdentifier); account.setUsername(AttributeValues.getString(item, ATTR_USERNAME, null)); account.setVersion(Integer.parseInt(item.get(ATTR_VERSION).n())); account.setCanonicallyDiscoverable(Optional.ofNullable(item.get(ATTR_CANONICALLY_DISCOVERABLE)).map(av -> av.bool()).orElse(false)); - if (shouldResetPni) { - try { - update(account); - } catch (final Exception e) { - log.warn("Failed to reset PNI for account {}", accountIdentifier, e); - } - } - return account; } catch (IOException e) { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java index 998ac1293..980bb504c 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -769,66 +769,6 @@ class AccountsTest { assertThat(account.getUsername()).hasValueSatisfying(u -> assertThat(u).isEqualTo(username)); } - // TODO Remove when PNI repair pass is complete - @Test - void testRepairMissingPni() throws JsonProcessingException { - final String number = "+18005551234"; - final UUID accountIdentifier = UUID.randomUUID(); - final UUID phoneNumberIdentifier = UUID.randomUUID(); - - { - final Account account = generateAccount(number, accountIdentifier, phoneNumberIdentifier); - accounts.create(account); - - // Artificially inject data with a null PNI in the JSON blob into the backing table - account.setNumber(number, null); - - dynamoDbExtension.getDynamoDbClient().updateItem(UpdateItemRequest.builder() - .tableName(ACCOUNTS_TABLE_NAME) - .key(Map.of(Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(accountIdentifier))) - .updateExpression("SET #data = :data") - .expressionAttributeNames(Map.of("#data", Accounts.ATTR_ACCOUNT_DATA)) - .expressionAttributeValues( - Map.of(":data", AttributeValues.fromByteArray(SystemMapper.getMapper().writeValueAsBytes(account)))) - .build()); - } - - { - final GetItemResponse response = dynamoDbExtension.getDynamoDbClient().getItem(GetItemRequest.builder() - .tableName(ACCOUNTS_TABLE_NAME) - .key(Map.of(Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(accountIdentifier))) - .consistentRead(true) - .build()); - - final String accountJson = new String( - AttributeValues.getByteArray(response.item(), Accounts.ATTR_ACCOUNT_DATA, null), StandardCharsets.UTF_8); - - final Account accountFromJson = SystemMapper.getMapper().readValue(accountJson, Account.class); - assertThat(accountFromJson.getPhoneNumberIdentifier()).isNull(); - } - - { - // Loading the account should auto-repair the missing PNI - final Account account = accounts.getByAccountIdentifier(accountIdentifier).orElseThrow(); - assertThat(account.getPhoneNumberIdentifier()).isEqualTo(phoneNumberIdentifier); - assertThat(account.isStale()).isFalse(); - } - - { - final GetItemResponse response = dynamoDbExtension.getDynamoDbClient().getItem(GetItemRequest.builder() - .tableName(ACCOUNTS_TABLE_NAME) - .key(Map.of(Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(accountIdentifier))) - .consistentRead(true) - .build()); - - final String accountJson = new String( - AttributeValues.getByteArray(response.item(), Accounts.ATTR_ACCOUNT_DATA, null), StandardCharsets.UTF_8); - - final Account accountFromJson = SystemMapper.getMapper().readValue(accountJson, Account.class); - assertThat(accountFromJson.getPhoneNumberIdentifier()).isEqualTo(phoneNumberIdentifier); - } - } - private Device generateDevice(long id) { Random random = new Random(System.currentTimeMillis()); SignedPreKey signedPreKey = new SignedPreKey(random.nextInt(), "testPublicKey-" + random.nextInt(),