From 4ea7278c6ff304d29c1895a9f225c3cbe263dffb Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Mon, 6 Dec 2021 15:50:27 -0500 Subject: [PATCH] Remove unversioned profile properties from `Account` entities --- .../controllers/ProfileController.java | 19 ++++-------- .../textsecuregcm/storage/Account.java | 30 ------------------- ...ConcurrentModificationIntegrationTest.java | 10 ++----- .../storage/AccountsManagerTest.java | 18 ++++------- .../textsecuregcm/storage/AccountsTest.java | 5 ---- .../controllers/ProfileControllerTest.java | 21 +++---------- 6 files changed, 18 insertions(+), 85 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java index 2263b5891..ebc191d4d 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java @@ -168,11 +168,6 @@ public class ProfileController { currentAvatar = Optional.of(currentProfile.get().getAvatar()); } - if (currentAvatar.isEmpty() && auth.getAccount().getAvatar() != null && auth.getAccount().getAvatar() - .startsWith("profiles/")) { - currentAvatar = Optional.of(auth.getAccount().getAvatar()); - } - currentAvatar.ifPresent(s -> s3client.deleteObject(DeleteObjectRequest.builder() .bucket(bucket) .key(s) @@ -186,8 +181,6 @@ public class ProfileController { .orElseGet(() -> auth.getAccount().getBadges()); accountsManager.update(auth.getAccount(), a -> { - a.setProfileName(request.getName()); - a.setAvatar(avatar); a.setBadges(clock, updatedBadges); a.setCurrentProfileVersion(request.getVersion()); }); @@ -261,10 +254,10 @@ public class ProfileController { Optional username = accountProfile.flatMap(Account::getUsername); Optional profile = profilesManager.get(uuid, version); - String name = profile.map(VersionedProfile::getName).orElse(accountProfile.get().getProfileName()); + String name = profile.map(VersionedProfile::getName).orElse(null); String about = profile.map(VersionedProfile::getAbout).orElse(null); String aboutEmoji = profile.map(VersionedProfile::getAboutEmoji).orElse(null); - String avatar = profile.map(VersionedProfile::getAvatar).orElse(accountProfile.get().getAvatar()); + String avatar = profile.map(VersionedProfile::getAvatar).orElse(null); Optional currentProfileVersion = accountProfile.get().getCurrentProfileVersion(); // Allow requests where either the version matches the latest version on Account or the latest version on Account @@ -339,10 +332,10 @@ public class ProfileController { final boolean isSelf = auth.getAccount().getUuid().equals(accountProfile.getUuid()); return new Profile( - accountProfile.getProfileName(), null, null, - accountProfile.getAvatar(), + null, + null, null, accountProfile.getIdentityKey(), UnidentifiedAccessChecksum.generateFor(accountProfile.getUnidentifiedAccessKey()), @@ -418,10 +411,10 @@ public class ProfileController { Optional username = accountProfile.flatMap(Account::getUsername); return new Profile( - accountProfile.get().getProfileName(), null, null, - accountProfile.get().getAvatar(), + null, + null, null, accountProfile.get().getIdentityKey(), UnidentifiedAccessChecksum.generateFor(accountProfile.get().getUnidentifiedAccessKey()), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java index 3f5ad4354..93a06b178 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java @@ -55,12 +55,6 @@ public class Account { @JsonProperty("cpv") private String currentProfileVersion; - @JsonProperty - private String name; - - @JsonProperty - private String avatar; - @JsonProperty private List badges = new ArrayList<>(); @@ -340,30 +334,6 @@ public class Account { this.currentProfileVersion = currentProfileVersion; } - public String getProfileName() { - requireNotStale(); - - return name; - } - - public void setProfileName(String name) { - requireNotStale(); - - this.name = name; - } - - public String getAvatar() { - requireNotStale(); - - return avatar; - } - - public void setAvatar(String avatar) { - requireNotStale(); - - this.avatar = avatar; - } - public List getBadges() { requireNotStale(); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java index fed2c6e35..66a00b0ab 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerConcurrentModificationIntegrationTest.java @@ -189,8 +189,6 @@ class AccountsManagerConcurrentModificationIntegrationTest { uuid = account.getUuid(); } - final String profileName = "name"; - final String avatar = "avatar"; final boolean discoverableByPhoneNumber = false; final String currentProfileVersion = "cpv"; final String identityKey = "ikey"; @@ -202,8 +200,6 @@ class AccountsManagerConcurrentModificationIntegrationTest { final long lastSeen = Instant.now().getEpochSecond(); CompletableFuture.allOf( - modifyAccount(uuid, account -> account.setProfileName(profileName)), - modifyAccount(uuid, account -> account.setAvatar(avatar)), modifyAccount(uuid, account -> account.setDiscoverableByPhoneNumber(discoverableByPhoneNumber)), modifyAccount(uuid, account -> account.setCurrentProfileVersion(currentProfileVersion)), modifyAccount(uuid, account -> account.setIdentityKey(identityKey)), @@ -224,7 +220,7 @@ class AccountsManagerConcurrentModificationIntegrationTest { new Pair<>("dynamo", dynamoAccount), new Pair<>("redis", redisAccount) ).forEach(pair -> - verifyAccount(pair.first(), pair.second(), profileName, avatar, discoverableByPhoneNumber, + verifyAccount(pair.first(), pair.second(), discoverableByPhoneNumber, currentProfileVersion, identityKey, unidentifiedAccessKey, pin, registrationLock, unrestrictedUnidentifiedAccess, lastSeen)); } @@ -237,11 +233,9 @@ class AccountsManagerConcurrentModificationIntegrationTest { return JsonHelpers.fromJson(redisSetArgumentCapture.getValue(), Account.class); } - private void verifyAccount(final String name, final Account account, final String profileName, final String avatar, final boolean discoverableByPhoneNumber, final String currentProfileVersion, final String identityKey, final byte[] unidentifiedAccessKey, final String pin, final String clientRegistrationLock, final boolean unrestrictedUnidentifiedAcces, final long lastSeen) { + private void verifyAccount(final String name, final Account account, final boolean discoverableByPhoneNumber, final String currentProfileVersion, final String identityKey, final byte[] unidentifiedAccessKey, final String pin, final String clientRegistrationLock, final boolean unrestrictedUnidentifiedAcces, final long lastSeen) { assertAll(name, - () -> assertEquals(profileName, account.getProfileName()), - () -> assertEquals(avatar, account.getAvatar()), () -> assertEquals(discoverableByPhoneNumber, account.isDiscoverableByPhoneNumber()), () -> assertEquals(currentProfileVersion, account.getCurrentProfileVersion().orElseThrow()), () -> assertEquals(identityKey, account.getIdentityKey()), diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java index c13136d88..7ef596f7a 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsManagerTest.java @@ -68,8 +68,6 @@ class AccountsManagerTest { private RedisAdvancedClusterCommands commands; private AccountsManager accountsManager; - private static final UUID RESERVED_FOR_UUID = UUID.randomUUID(); - private static final Answer ACCOUNT_UPDATE_ANSWER = (answer) -> { // it is implicit in the update() contract is that a successful call will // result in an incremented version @@ -150,13 +148,12 @@ class AccountsManagerTest { UUID uuid = UUID.randomUUID(); when(commands.get(eq("AccountMap::+14152222222"))).thenReturn(uuid.toString()); - when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\"}"); + when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\"}"); Optional account = accountsManager.getByE164("+14152222222"); assertTrue(account.isPresent()); assertEquals(account.get().getNumber(), "+14152222222"); - assertEquals(account.get().getProfileName(), "test"); assertEquals(UUID.fromString("de24dc73-fbd8-41be-a7d5-764c70d9da7e"), account.get().getPhoneNumberIdentifier()); verify(commands, times(1)).get(eq("AccountMap::+14152222222")); @@ -170,14 +167,13 @@ class AccountsManagerTest { void testGetAccountByUuidInCache() { UUID uuid = UUID.randomUUID(); - when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\"}"); + when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\"}"); Optional account = accountsManager.getByAccountIdentifier(uuid); assertTrue(account.isPresent()); assertEquals(account.get().getNumber(), "+14152222222"); assertEquals(account.get().getUuid(), uuid); - assertEquals(account.get().getProfileName(), "test"); assertEquals(UUID.fromString("de24dc73-fbd8-41be-a7d5-764c70d9da7e"), account.get().getPhoneNumberIdentifier()); verify(commands, times(1)).get(eq("Account3::" + uuid)); @@ -192,13 +188,12 @@ class AccountsManagerTest { UUID pni = UUID.randomUUID(); when(commands.get(eq("AccountMap::" + pni))).thenReturn(uuid.toString()); - when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\"}"); + when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\"}"); Optional account = accountsManager.getByPhoneNumberIdentifier(pni); assertTrue(account.isPresent()); assertEquals(account.get().getNumber(), "+14152222222"); - assertEquals(account.get().getProfileName(), "test"); assertEquals(UUID.fromString("de24dc73-fbd8-41be-a7d5-764c70d9da7e"), account.get().getPhoneNumberIdentifier()); verify(commands).get(eq("AccountMap::" + pni)); @@ -214,13 +209,12 @@ class AccountsManagerTest { String username = "test"; when(commands.get(eq("AccountMap::" + username))).thenReturn(uuid.toString()); - when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"name\": \"test\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\", \"username\": \"test\"}"); + when(commands.get(eq("Account3::" + uuid))).thenReturn("{\"number\": \"+14152222222\", \"pni\": \"de24dc73-fbd8-41be-a7d5-764c70d9da7e\", \"username\": \"test\"}"); Optional account = accountsManager.getByUsername(username); assertTrue(account.isPresent()); assertEquals(account.get().getNumber(), "+14152222222"); - assertEquals(account.get().getProfileName(), "test"); assertEquals(UUID.fromString("de24dc73-fbd8-41be-a7d5-764c70d9da7e"), account.get().getPhoneNumberIdentifier()); assertEquals(Optional.of(username), account.get().getUsername()); @@ -451,10 +445,10 @@ class AccountsManagerTest { .doAnswer(ACCOUNT_UPDATE_ANSWER) .when(accounts).update(any()); - account = accountsManager.update(account, a -> a.setProfileName("name")); + account = accountsManager.update(account, a -> a.setIdentityKey("identity-key")); assertEquals(1, account.getVersion()); - assertEquals("name", account.getProfileName()); + assertEquals("identity-key", account.getIdentityKey()); verify(accounts, times(1)).getByAccountIdentifier(uuid); verify(accounts, times(2)).update(any()); 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 cd7ba22e3..85d8b21d0 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -294,8 +294,6 @@ class AccountsTest { assertPhoneNumberConstraintExists("+14151112222", firstUuid); assertPhoneNumberIdentifierConstraintExists(firstPni, firstUuid); - account.setProfileName("name"); - accounts.update(account); UUID secondUuid = UUID.randomUUID(); @@ -348,8 +346,6 @@ class AccountsTest { assertThatThrownBy(() -> accounts.update(unknownAccount)).isInstanceOfAny(ConditionalCheckFailedException.class); - account.setProfileName("name"); - accounts.update(account); assertThat(account.getVersion()).isEqualTo(2); @@ -361,7 +357,6 @@ class AccountsTest { assertThatThrownBy(() -> accounts.update(account)).isInstanceOfAny(ContestedOptimisticLockException.class); account.setVersion(2); - account.setProfileName("name2"); accounts.update(account); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java index 02c024a90..a398770bb 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/tests/controllers/ProfileControllerTest.java @@ -164,8 +164,6 @@ class ProfileControllerTest { profileAccount = mock(Account.class); when(profileAccount.getIdentityKey()).thenReturn("bar"); - when(profileAccount.getProfileName()).thenReturn("baz"); - when(profileAccount.getAvatar()).thenReturn("profiles/bang"); when(profileAccount.getUuid()).thenReturn(AuthHelper.VALID_UUID_TWO); when(profileAccount.isEnabled()).thenReturn(true); when(profileAccount.isGroupsV2Supported()).thenReturn(false); @@ -179,8 +177,6 @@ class ProfileControllerTest { Account capabilitiesAccount = mock(Account.class); when(capabilitiesAccount.getIdentityKey()).thenReturn("barz"); - when(capabilitiesAccount.getProfileName()).thenReturn("bazz"); - when(capabilitiesAccount.getAvatar()).thenReturn("profiles/bangz"); when(capabilitiesAccount.isEnabled()).thenReturn(true); when(capabilitiesAccount.isGroupsV2Supported()).thenReturn(true); when(capabilitiesAccount.isGv1MigrationSupported()).thenReturn(true); @@ -220,8 +216,8 @@ class ProfileControllerTest { .get(Profile.class); assertThat(profile.getIdentityKey()).isEqualTo("bar"); - assertThat(profile.getName()).isEqualTo("baz"); - assertThat(profile.getAvatar()).isEqualTo("profiles/bang"); + assertThat(profile.getName()).isNull(); + assertThat(profile.getAvatar()).isNull(); assertThat(profile.getUsername()).isEqualTo("n00bkiller"); assertThat(profile.getBadges()).hasSize(1).element(0).has(new Condition<>( badge -> "Test Badge".equals(badge.getName()), "has badge with expected name")); @@ -239,8 +235,8 @@ class ProfileControllerTest { .get(Profile.class); assertThat(profile.getIdentityKey()).isEqualTo("bar"); - assertThat(profile.getName()).isEqualTo("baz"); - assertThat(profile.getAvatar()).isEqualTo("profiles/bang"); + assertThat(profile.getName()).isNull(); + assertThat(profile.getAvatar()).isNull(); assertThat(profile.getUsername()).isEqualTo("n00bkiller"); assertThat(profile.getUuid()).isEqualTo(AuthHelper.VALID_UUID_TWO); assertThat(profile.getBadges()).hasSize(1).element(0).has(new Condition<>( @@ -383,9 +379,6 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq("anotherversion")); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verify(AuthHelper.VALID_ACCOUNT_TWO).setProfileName("123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678"); - verify(AuthHelper.VALID_ACCOUNT_TWO).setAvatar(null); - verifyNoMoreInteractions(s3client); assertThat(profileArgumentCaptor.getValue().getCommitment()).isEqualTo(commitment.serialize()); @@ -472,9 +465,6 @@ class ProfileControllerTest { verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq("anotherversion")); verify(profilesManager, times(1)).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verify(AuthHelper.VALID_ACCOUNT_TWO).setProfileName(name); - verify(AuthHelper.VALID_ACCOUNT_TWO).setAvatar(null); - verifyNoMoreInteractions(s3client); final VersionedProfile profile = profileArgumentCaptor.getValue(); @@ -510,9 +500,6 @@ class ProfileControllerTest { verify(profilesManager).get(eq(AuthHelper.VALID_UUID_TWO), eq("yetanotherversion")); verify(profilesManager).set(eq(AuthHelper.VALID_UUID_TWO), profileArgumentCaptor.capture()); - verify(AuthHelper.VALID_ACCOUNT_TWO).setProfileName(eq(name)); - verify(AuthHelper.VALID_ACCOUNT_TWO).setAvatar(null); - verifyNoMoreInteractions(s3client); final VersionedProfile profile = profileArgumentCaptor.getValue();