From cf92007f668ac6813c1596f499380ae19210013f Mon Sep 17 00:00:00 2001 From: Sergey Skrobotov Date: Thu, 20 Jul 2023 11:12:44 -0700 Subject: [PATCH] Moving Account serialization logic to storage-specific classes --- .../textsecuregcm/storage/Account.java | 60 +++++++------------ .../textsecuregcm/storage/Accounts.java | 44 +++++++++----- .../storage/AccountsManager.java | 32 +++++----- .../textsecuregcm/util/SystemMapper.java | 27 +++++++++ .../textsecuregcm/storage/AccountTest.java | 16 +++++ .../textsecuregcm/storage/AccountsTest.java | 19 ++++++ .../textsecuregcm/util/SystemMapperTest.java | 25 +++++++- 7 files changed, 154 insertions(+), 69 deletions(-) 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 844e5da7a..b4f439fc2 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Account.java @@ -5,6 +5,7 @@ package org.whispersystems.textsecuregcm.storage; +import com.fasterxml.jackson.annotation.JsonFilter; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; @@ -29,12 +30,12 @@ import org.whispersystems.textsecuregcm.util.ByteArrayBase64UrlAdapter; import org.whispersystems.textsecuregcm.util.IdentityKeyAdapter; import org.whispersystems.textsecuregcm.util.Util; +@JsonFilter("Account") public class Account { - @JsonIgnore private static final Logger logger = LoggerFactory.getLogger(Account.class); - @JsonIgnore + @JsonProperty private UUID uuid; @JsonProperty("pni") @@ -55,7 +56,7 @@ public class Account { @Nullable private byte[] reservedUsernameHash; - @JsonIgnore + @JsonProperty @Nullable private UUID usernameLinkHandle; @@ -103,16 +104,13 @@ public class Account { @JsonIgnore private boolean stale; - @JsonIgnore - private boolean canonicallyDiscoverable; - public UUID getUuid() { // this is the one method that may be called on a stale account return uuid; } - public void setUuid(UUID uuid) { + public void setUuid(final UUID uuid) { requireNotStale(); this.uuid = uuid; @@ -140,7 +138,7 @@ public class Account { return number; } - public void setNumber(String number, UUID phoneNumberIdentifier) { + public void setNumber(final String number, final UUID phoneNumberIdentifier) { requireNotStale(); this.number = number; @@ -203,14 +201,14 @@ public class Account { this.usernameLinkHandle = usernameLinkHandle; } - public void addDevice(Device device) { + public void addDevice(final Device device) { requireNotStale(); removeDevice(device.getId()); this.devices.add(device); } - public void removeDevice(long deviceId) { + public void removeDevice(final long deviceId) { requireNotStale(); this.devices.removeIf(device -> device.getId() == deviceId); @@ -228,7 +226,7 @@ public class Account { return getDevice(Device.MASTER_ID); } - public Optional getDevice(long deviceId) { + public Optional getDevice(final long deviceId) { requireNotStale(); return devices.stream().filter(device -> device.getId() == deviceId).findFirst(); @@ -278,7 +276,7 @@ public class Account { return allEnabledDevicesHaveCapability(DeviceCapabilities::isPaymentActivation); } - private boolean allEnabledDevicesHaveCapability(Predicate predicate) { + private boolean allEnabledDevicesHaveCapability(final Predicate predicate) { requireNotStale(); return devices.stream() @@ -309,25 +307,13 @@ public class Account { int count = 0; - for (Device device : devices) { + for (final Device device : devices) { if (device.isEnabled()) count++; } return count; } - public boolean isCanonicallyDiscoverable() { - requireNotStale(); - - return canonicallyDiscoverable; - } - - public void setCanonicallyDiscoverable(boolean canonicallyDiscoverable) { - requireNotStale(); - - this.canonicallyDiscoverable = canonicallyDiscoverable; - } - public void setIdentityKey(final IdentityKey identityKey) { requireNotStale(); @@ -362,7 +348,7 @@ public class Account { return Optional.ofNullable(currentProfileVersion); } - public void setCurrentProfileVersion(String currentProfileVersion) { + public void setCurrentProfileVersion(final String currentProfileVersion) { requireNotStale(); this.currentProfileVersion = currentProfileVersion; @@ -374,7 +360,7 @@ public class Account { return badges; } - public void setBadges(Clock clock, List badges) { + public void setBadges(final Clock clock, final List badges) { requireNotStale(); this.badges = badges; @@ -382,11 +368,11 @@ public class Account { purgeStaleBadges(clock); } - public void addBadge(Clock clock, AccountBadge badge) { + public void addBadge(final Clock clock, final AccountBadge badge) { requireNotStale(); boolean added = false; for (int i = 0; i < badges.size(); i++) { - AccountBadge badgeInList = badges.get(i); + final AccountBadge badgeInList = badges.get(i); if (Objects.equals(badgeInList.getId(), badge.getId())) { if (added) { badges.remove(i); @@ -405,7 +391,7 @@ public class Account { purgeStaleBadges(clock); } - public void makeBadgePrimaryIfExists(Clock clock, String badgeId) { + public void makeBadgePrimaryIfExists(final Clock clock, final String badgeId) { requireNotStale(); // early exit if it's already the first item in the list @@ -429,28 +415,28 @@ public class Account { purgeStaleBadges(clock); } - public void removeBadge(Clock clock, String id) { + public void removeBadge(final Clock clock, final String id) { requireNotStale(); badges.removeIf(accountBadge -> Objects.equals(accountBadge.getId(), id)); purgeStaleBadges(clock); } - private void purgeStaleBadges(Clock clock) { + private void purgeStaleBadges(final Clock clock) { final Instant now = clock.instant(); badges.removeIf(accountBadge -> now.isAfter(accountBadge.getExpiration())); } public void setRegistrationLockFromAttributes(final AccountAttributes attributes) { if (!Util.isEmpty(attributes.getRegistrationLock())) { - SaltedTokenHash credentials = SaltedTokenHash.generateFor(attributes.getRegistrationLock()); + final SaltedTokenHash credentials = SaltedTokenHash.generateFor(attributes.getRegistrationLock()); setRegistrationLock(credentials.hash(), credentials.salt()); } else { setRegistrationLock(null, null); } } - public void setRegistrationLock(String registrationLock, String registrationLockSalt) { + public void setRegistrationLock(final String registrationLock, final String registrationLockSalt) { requireNotStale(); this.registrationLock = registrationLock; @@ -469,7 +455,7 @@ public class Account { return Optional.ofNullable(unidentifiedAccessKey); } - public void setUnidentifiedAccessKey(byte[] unidentifiedAccessKey) { + public void setUnidentifiedAccessKey(final byte[] unidentifiedAccessKey) { requireNotStale(); this.unidentifiedAccessKey = unidentifiedAccessKey; @@ -481,7 +467,7 @@ public class Account { return unrestrictedUnidentifiedAccess; } - public void setUnrestrictedUnidentifiedAccess(boolean unrestrictedUnidentifiedAccess) { + public void setUnrestrictedUnidentifiedAccess(final boolean unrestrictedUnidentifiedAccess) { requireNotStale(); this.unrestrictedUnidentifiedAccess = unrestrictedUnidentifiedAccess; @@ -511,7 +497,7 @@ public class Account { return version; } - public void setVersion(int version) { + public void setVersion(final int version) { requireNotStale(); this.version = version; 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 8018f68da..f8e485448 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/Accounts.java @@ -8,6 +8,7 @@ import static com.codahale.metrics.MetricRegistry.name; import static java.util.Objects.requireNonNull; import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectWriter; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Throwables; import io.micrometer.core.instrument.Metrics; @@ -31,7 +32,6 @@ import java.util.function.Predicate; import java.util.stream.Collectors; import javax.annotation.Nonnull; import javax.annotation.Nullable; - import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.whispersystems.textsecuregcm.util.AsyncTimerUtil; @@ -63,11 +63,25 @@ import software.amazon.awssdk.services.dynamodb.model.Update; import software.amazon.awssdk.services.dynamodb.model.UpdateItemRequest; import software.amazon.awssdk.utils.CompletableFutureUtils; +/** + * "Accounts" DDB table's structure doesn't match 1:1 the {@link Account} class: most of the class fields are serialized + * and stored in the {@link Accounts#ATTR_ACCOUNT_DATA} attribute, however there are certain fields that are stored only as DDB attributes + * (e.g. if indexing or lookup by field is required), and there are also fields that stored in both places. + * This class contains all the logic that decides whether or not a field of the {@link Account} class should be + * added as an attribute, serialized as a part of {@link Accounts#ATTR_ACCOUNT_DATA}, or both. To skip serialization, + * make sure attribute name is listed in {@link Accounts#ACCOUNT_FIELDS_TO_EXCLUDE_FROM_SERIALIZATION}. If serialization is skipped, + * make sure the field is stored in a DDB attribute and then put back into the account object in {@link Accounts#fromItem(Map)}. + */ @SuppressWarnings("OptionalUsedAsFieldOrParameterType") public class Accounts extends AbstractDynamoDbStore { private static final Logger log = LoggerFactory.getLogger(Accounts.class); + static final List ACCOUNT_FIELDS_TO_EXCLUDE_FROM_SERIALIZATION = List.of("uuid", "usernameLinkHandle"); + + private static final ObjectWriter ACCOUNT_DDB_JSON_WRITER = SystemMapper.jsonMapper() + .writer(SystemMapper.excludingField(Account.class, ACCOUNT_FIELDS_TO_EXCLUDE_FROM_SERIALIZATION)); + private static final Timer CREATE_TIMER = Metrics.timer(name(Accounts.class, "create")); private static final Timer CHANGE_NUMBER_TIMER = Metrics.timer(name(Accounts.class, "changeNumber")); private static final Timer SET_USERNAME_TIMER = Metrics.timer(name(Accounts.class, "setUsername")); @@ -207,7 +221,7 @@ public class Accounts extends AbstractDynamoDbStore { final Account existingAccount = getByAccountIdentifier(account.getUuid()).orElseThrow(); // It's up to the client to delete this username hash if they can't retrieve and decrypt the plaintext username from storage service - existingAccount.getUsernameHash().ifPresent(existingUsernameHash -> account.setUsernameHash(existingUsernameHash)); + existingAccount.getUsernameHash().ifPresent(account::setUsernameHash); account.setNumber(existingAccount.getNumber(), existingAccount.getPhoneNumberIdentifier()); account.setVersion(existingAccount.getVersion()); @@ -281,7 +295,7 @@ public class Accounts extends AbstractDynamoDbStore { "#version", ATTR_VERSION)) .expressionAttributeValues(Map.of( ":number", numberAttr, - ":data", AttributeValues.fromByteArray(SystemMapper.jsonMapper().writeValueAsBytes(account)), + ":data", accountDataAttributeValue(account), ":cds", AttributeValues.fromBool(account.shouldBeVisibleInDirectory()), ":pni", pniAttr, ":version", AttributeValues.fromInt(account.getVersion()), @@ -324,7 +338,7 @@ public class Accounts extends AbstractDynamoDbStore { final long expirationTime = clock.instant().plus(ttl).getEpochSecond(); // Use account UUID as a "reservation token" - by providing this, the client proves ownership of the hash - UUID uuid = account.getUuid(); + final UUID uuid = account.getUuid(); try { final List writeItems = new ArrayList<>(); @@ -352,7 +366,7 @@ public class Accounts extends AbstractDynamoDbStore { .conditionExpression("#version = :version") .expressionAttributeNames(Map.of("#data", ATTR_ACCOUNT_DATA, "#version", ATTR_VERSION)) .expressionAttributeValues(Map.of( - ":data", AttributeValues.fromByteArray(SystemMapper.jsonMapper().writeValueAsBytes(account)), + ":data", accountDataAttributeValue(account), ":version", AttributeValues.fromInt(account.getVersion()), ":version_increment", AttributeValues.fromInt(1))) .build()) @@ -427,7 +441,7 @@ public class Accounts extends AbstractDynamoDbStore { final StringBuilder updateExpr = new StringBuilder("SET #data = :data, #username_hash = :username_hash"); final Map expressionAttributeValues = new HashMap<>(Map.of( - ":data", AttributeValues.fromByteArray(SystemMapper.jsonMapper().writeValueAsBytes(account)), + ":data", accountDataAttributeValue(account), ":username_hash", AttributeValues.fromByteArray(usernameHash), ":version", AttributeValues.fromInt(account.getVersion()), ":version_increment", AttributeValues.fromInt(1))); @@ -503,7 +517,7 @@ public class Accounts extends AbstractDynamoDbStore { "#username_hash", ATTR_USERNAME_HASH, "#version", ATTR_VERSION)) .expressionAttributeValues(Map.of( - ":data", AttributeValues.fromByteArray(SystemMapper.jsonMapper().writeValueAsBytes(account)), + ":data", accountDataAttributeValue(account), ":version", AttributeValues.fromInt(account.getVersion()), ":version_increment", AttributeValues.fromInt(1))) .build()) @@ -547,8 +561,9 @@ public class Accounts extends AbstractDynamoDbStore { "#data", ATTR_ACCOUNT_DATA, "#cds", ATTR_CANONICALLY_DISCOVERABLE, "#version", ATTR_VERSION)); + final Map attrValues = new HashMap<>(Map.of( - ":data", AttributeValues.fromByteArray(SystemMapper.jsonMapper().writeValueAsBytes(account)), + ":data", accountDataAttributeValue(account), ":cds", AttributeValues.fromBool(account.shouldBeVisibleInDirectory()), ":version", AttributeValues.fromInt(account.getVersion()), ":version_increment", AttributeValues.fromInt(1))); @@ -861,7 +876,7 @@ public class Accounts extends AbstractDynamoDbStore { KEY_ACCOUNT_UUID, uuidAttr, ATTR_ACCOUNT_E164, numberAttr, ATTR_PNI_UUID, pniUuidAttr, - ATTR_ACCOUNT_DATA, AttributeValues.fromByteArray(SystemMapper.jsonMapper().writeValueAsBytes(account)), + ATTR_ACCOUNT_DATA, accountDataAttributeValue(account), ATTR_VERSION, AttributeValues.fromInt(account.getVersion()), ATTR_CANONICALLY_DISCOVERABLE, AttributeValues.fromBool(account.shouldBeVisibleInDirectory()))); @@ -970,10 +985,10 @@ public class Accounts extends AbstractDynamoDbStore { @VisibleForTesting @Nonnull static Account fromItem(final Map item) { - // TODO: eventually require ATTR_CANONICALLY_DISCOVERABLE if (!item.containsKey(ATTR_ACCOUNT_DATA) || !item.containsKey(ATTR_ACCOUNT_E164) - || !item.containsKey(KEY_ACCOUNT_UUID)) { + || !item.containsKey(KEY_ACCOUNT_UUID) + || !item.containsKey(ATTR_CANONICALLY_DISCOVERABLE)) { throw new RuntimeException("item missing values"); } try { @@ -994,9 +1009,6 @@ public class Accounts extends AbstractDynamoDbStore { account.setUsernameHash(AttributeValues.getByteArray(item, ATTR_USERNAME_HASH, null)); account.setUsernameLinkHandle(AttributeValues.getUUID(item, ATTR_USERNAME_LINK_UUID, null)); account.setVersion(Integer.parseInt(item.get(ATTR_VERSION).n())); - account.setCanonicallyDiscoverable(Optional.ofNullable(item.get(ATTR_CANONICALLY_DISCOVERABLE)) - .map(AttributeValue::bool) - .orElse(false)); return account; @@ -1005,6 +1017,10 @@ public class Accounts extends AbstractDynamoDbStore { } } + private static AttributeValue accountDataAttributeValue(final Account account) throws JsonProcessingException { + return AttributeValues.fromByteArray(ACCOUNT_DDB_JSON_WRITER.writeValueAsBytes(account)); + } + private static boolean conditionalCheckFailed(final CancellationReason reason) { return CONDITIONAL_CHECK_FAILED.equals(reason.code()); } 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 27899b94b..4e2aeb293 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/storage/AccountsManager.java @@ -12,7 +12,7 @@ import com.codahale.metrics.MetricRegistry; import com.codahale.metrics.SharedMetricRegistries; import com.codahale.metrics.Timer; import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.ObjectWriter; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import io.lettuce.core.RedisException; @@ -112,7 +112,8 @@ public class AccountsManager { private final RegistrationRecoveryPasswordsManager registrationRecoveryPasswordsManager; private final Clock clock; - private static final ObjectMapper mapper = SystemMapper.jsonMapper(); + private static final ObjectWriter ACCOUNT_REDIS_JSON_WRITER = SystemMapper.jsonMapper() + .writer(SystemMapper.excludingField(Account.class, List.of("uuid"))); // An account that's used at least daily will get reset in the cache at least once per day when its "last seen" // timestamp updates; expiring entries after two days will help clear out "zombie" cache entries that are read @@ -454,7 +455,7 @@ public class AccountsManager { /** * Reserve a username hash so that no other accounts may take it. *

- * The reserved hash can later be set with {@link #confirmReservedUsernameHash(Account, byte[])}. The reservation + * The reserved hash can later be set with {@link #confirmReservedUsernameHash(Account, byte[], byte[])}. The reservation * will eventually expire, after which point confirmReservedUsernameHash may fail if another account has taken the * username hash. * @@ -657,7 +658,7 @@ public class AccountsManager { final Supplier retriever, final AccountChangeValidator changeValidator) throws UsernameHashNotAvailableException { - Account originalAccount = cloneAccount(account); + Account originalAccount = cloneAccountAsNotStale(account); if (!updater.apply(account)) { return account; @@ -671,7 +672,7 @@ public class AccountsManager { try { persister.persistAccount(account); - final Account updatedAccount = cloneAccount(account); + final Account updatedAccount = cloneAccountAsNotStale(account); account.markStale(); changeValidator.validateChange(originalAccount, updatedAccount); @@ -681,7 +682,7 @@ public class AccountsManager { tries++; account = retriever.get(); - originalAccount = cloneAccount(account); + originalAccount = cloneAccountAsNotStale(account); if (!updater.apply(account)) { return account; @@ -699,7 +700,7 @@ public class AccountsManager { final AccountChangeValidator changeValidator, final int remainingTries) { - final Account originalAccount = cloneAccount(account); + final Account originalAccount = cloneAccountAsNotStale(account); if (!updater.apply(account)) { return CompletableFuture.completedFuture(account); @@ -708,7 +709,7 @@ public class AccountsManager { if (remainingTries > 0) { return persister.apply(account) .thenApply(ignored -> { - final Account updatedAccount = cloneAccount(account); + final Account updatedAccount = cloneAccountAsNotStale(account); account.markStale(); changeValidator.validateChange(originalAccount, updatedAccount); @@ -728,13 +729,10 @@ public class AccountsManager { return CompletableFuture.failedFuture(new OptimisticLockRetryLimitExceededException()); } - private static Account cloneAccount(final Account account) { + private static Account cloneAccountAsNotStale(final Account account) { try { - final Account clone = mapper.readValue(mapper.writeValueAsBytes(account), Account.class); - clone.setUuid(account.getUuid()); - clone.setUsernameLinkHandle(account.getUsernameLinkHandle()); - - return clone; + return SystemMapper.jsonMapper().readValue( + SystemMapper.jsonMapper().writeValueAsBytes(account), Account.class); } catch (final IOException e) { // this should really, truly, never happen throw new IllegalArgumentException(e); @@ -901,7 +899,7 @@ public class AccountsManager { private void redisSet(Account account) { try (Timer.Context ignored = redisSetTimer.time()) { - final String accountJson = mapper.writeValueAsString(account); + final String accountJson = ACCOUNT_REDIS_JSON_WRITER.writeValueAsString(account); cacheCluster.useCluster(connection -> { final RedisAdvancedClusterCommands commands = connection.sync(); @@ -922,7 +920,7 @@ public class AccountsManager { final String accountJson; try { - accountJson = mapper.writeValueAsString(account); + accountJson = ACCOUNT_REDIS_JSON_WRITER.writeValueAsString(account); } catch (final JsonProcessingException e) { throw new UncheckedIOException(e); } @@ -1036,7 +1034,7 @@ public class AccountsManager { private static Optional parseAccountJson(@Nullable final String accountJson, final UUID uuid) { try { if (StringUtils.isNotBlank(accountJson)) { - Account account = mapper.readValue(accountJson, Account.class); + Account account = SystemMapper.jsonMapper().readValue(accountJson, Account.class); account.setUuid(uuid); if (account.getPhoneNumberIdentifier() == null) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/util/SystemMapper.java b/service/src/main/java/org/whispersystems/textsecuregcm/util/SystemMapper.java index 52bec59d6..e055c716f 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/util/SystemMapper.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/util/SystemMapper.java @@ -6,12 +6,19 @@ package org.whispersystems.textsecuregcm.util; import com.fasterxml.jackson.annotation.JsonAutoDetect; +import com.fasterxml.jackson.annotation.JsonFilter; import com.fasterxml.jackson.annotation.PropertyAccessor; import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.ser.FilterProvider; +import com.fasterxml.jackson.databind.ser.impl.SimpleBeanPropertyFilter; +import com.fasterxml.jackson.databind.ser.impl.SimpleFilterProvider; import com.fasterxml.jackson.dataformat.yaml.YAMLMapper; import com.fasterxml.jackson.datatype.jdk8.Jdk8Module; import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; +import java.util.Arrays; +import java.util.List; +import java.util.Objects; import javax.annotation.Nonnull; import org.whispersystems.textsecuregcm.configuration.secrets.SecretsModule; @@ -34,6 +41,7 @@ public class SystemMapper { public static ObjectMapper configureMapper(final ObjectMapper mapper) { return mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false) + .setFilterProvider(new SimpleFilterProvider().setDefaultFilter(SimpleBeanPropertyFilter.serializeAll())) .setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE) .setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY) .setVisibility(PropertyAccessor.CREATOR, JsonAutoDetect.Visibility.PUBLIC_ONLY) @@ -42,4 +50,23 @@ public class SystemMapper { new JavaTimeModule(), new Jdk8Module()); } + + public static FilterProvider excludingField(final Class clazz, final List fieldsToExclude) { + final String filterId = clazz.getSimpleName(); + + // validate that the target class is annotated with @JsonFilter, + final List jsonFilterAnnotations = Arrays.stream(clazz.getAnnotations()) + .map(a -> a instanceof JsonFilter jsonFilter ? jsonFilter : null) + .filter(Objects::nonNull) + .toList(); + if (jsonFilterAnnotations.size() != 1 || !jsonFilterAnnotations.get(0).value().equals(filterId)) { + throw new IllegalStateException(""" + Class `%1$s` must have a single annotation of type `JsonFilter` + with the value equal to the name of the class itself: `@JsonFilter("%1$s")` + """.formatted(filterId)); + } + + return new SimpleFilterProvider() + .addFilter(filterId, SimpleBeanPropertyFilter.serializeAllExcept(fieldsToExclude.toArray(new String[0]))); + } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountTest.java index 45b24fd04..68ff5e33c 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountTest.java @@ -16,11 +16,15 @@ import static org.mockito.Mockito.when; import static org.whispersystems.textsecuregcm.tests.util.DevicesHelper.createDevice; import static org.whispersystems.textsecuregcm.tests.util.DevicesHelper.setEnabled; +import com.fasterxml.jackson.annotation.JsonFilter; +import java.lang.annotation.Annotation; import java.nio.charset.StandardCharsets; import java.time.Clock; import java.time.Instant; +import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Optional; import java.util.UUID; import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.BeforeEach; @@ -428,4 +432,16 @@ class AccountTest { assertThat(badge.isVisible()).isTrue(); }); } + + @Test + public void testAccountClassJsonFilterIdMatchesClassName() throws Exception { + // Some logic relies on the @JsonFilter name being equal to the class name. + // This test is just making sure that annotation is there and that the ID matches class name. + final Optional maybeJsonFilterAnnotation = Arrays.stream(Account.class.getAnnotations()) + .filter(a -> a.annotationType().equals(JsonFilter.class)) + .findFirst(); + assertTrue(maybeJsonFilterAnnotation.isPresent()); + final JsonFilter jsonFilterAnnotation = (JsonFilter) maybeJsonFilterAnnotation.get(); + assertEquals(Account.class.getSimpleName(), jsonFilterAnnotation.value()); + } } 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 c50445f0d..b5278f40b 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/storage/AccountsTest.java @@ -11,6 +11,7 @@ import static org.assertj.core.api.Assertions.assertThatNoException; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -989,6 +990,24 @@ class AccountsTest { assertThat(account.getUsernameHash()).isEmpty(); } + @Test + public void testIgnoredFieldsNotAddedToDataAttribute() throws Exception { + final Account account = generateAccount("+18005551234", UUID.randomUUID(), UUID.randomUUID()); + account.setUsernameHash(RandomUtils.nextBytes(32)); + account.setUsernameLinkDetails(UUID.randomUUID(), RandomUtils.nextBytes(32)); + accounts.create(account); + final Map accountRecord = DYNAMO_DB_EXTENSION.getDynamoDbClient() + .getItem(GetItemRequest.builder() + .tableName(Tables.ACCOUNTS.tableName()) + .key(Map.of(Accounts.KEY_ACCOUNT_UUID, AttributeValues.fromUUID(account.getUuid()))) + .build()) + .item(); + final Map dataMap = SystemMapper.jsonMapper() + .readValue(accountRecord.get(Accounts.ATTR_ACCOUNT_DATA).b().asByteArray(), Map.class); + Accounts.ACCOUNT_FIELDS_TO_EXCLUDE_FROM_SERIALIZATION + .forEach(field -> assertFalse(dataMap.containsKey(field))); + } + private static Device generateDevice(long id) { return DevicesHelper.createDevice(id); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/util/SystemMapperTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/util/SystemMapperTest.java index 93b6501cd..dfb73551e 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/util/SystemMapperTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/util/SystemMapperTest.java @@ -6,13 +6,19 @@ package org.whispersystems.textsecuregcm.util; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import com.fasterxml.jackson.annotation.JsonFilter; import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.ObjectWriter; +import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.stream.Stream; +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; @@ -68,7 +74,6 @@ class SystemMapperTest { } } - @ParameterizedTest @ValueSource(classes = {DataClass.class, DataRecord.class}) public void testOptionalField(final Class clazz) throws Exception { @@ -96,4 +101,22 @@ class SystemMapperTest { Arguments.of(new DataRecord(null), JSON_NO_FIELD) ); } + + public record NotAnnotatedWithJsonFilter(String data) { + } + + @JsonFilter("AnnotatedWithJsonFilter") + public record AnnotatedWithJsonFilter(String data, String excluded) { + } + + @Test + public void testFiltering() throws Exception { + assertThrows(IllegalStateException.class, () -> SystemMapper.excludingField(NotAnnotatedWithJsonFilter.class, List.of("data"))); + final ObjectWriter writer = SystemMapper.jsonMapper() + .writer(SystemMapper.excludingField(AnnotatedWithJsonFilter.class, List.of("excluded"))); + final AnnotatedWithJsonFilter obj = new AnnotatedWithJsonFilter("valData", "valExcluded"); + final String json = writer.writeValueAsString(obj); + final Map serializedFields = SystemMapper.jsonMapper().readValue(json, Map.class); + assertEquals(Map.of("data", "valData"), serializedFields); + } }