From 8d38086d401caec1e77934dac8ca2123c7996e62 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Sat, 22 Aug 2020 09:13:55 -0400 Subject: [PATCH] Add check to prevent creating VKeys with incorrectly null parents (#774) * Add check to prevent creating VKeys with incorrectly null parents Datastore entities that are not the roots of entity groups are uniquely defined by an inheritance chain in addition to the id/name, not just by id/name. --- .../google/registry/persistence/VKey.java | 35 ++++++++++++++++++- .../model/contact/ContactResourceTest.java | 4 +-- .../model/domain/GracePeriodTest.java | 7 +++- .../registry/model/host/HostResourceTest.java | 4 --- .../model/transfer/TransferDataTest.java | 35 ++++++++++++++++--- .../google/registry/persistence/VKeyTest.java | 21 ++++++++++- 6 files changed, 91 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/google/registry/persistence/VKey.java b/core/src/main/java/google/registry/persistence/VKey.java index a06ced5ea..d12bfdd9d 100644 --- a/core/src/main/java/google/registry/persistence/VKey.java +++ b/core/src/main/java/google/registry/persistence/VKey.java @@ -14,10 +14,12 @@ package google.registry.persistence; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static google.registry.util.PreconditionsUtils.checkArgumentNotNull; import com.googlecode.objectify.Key; +import google.registry.model.BackupGroupRoot; import google.registry.model.ImmutableObject; import google.registry.model.translators.VKeyTranslatorFactory; import java.io.Serializable; @@ -75,11 +77,42 @@ public class VKey extends ImmutableObject implements Serializable { return new VKey(kind, ofyKey, sqlKey); } - /** Creates a symmetric {@link VKey} in which both sql and ofy keys are {@code id}. */ + /** + * Creates a symmetric {@link VKey} in which both sql and ofy keys are {@code id}. + * + *

IMPORTANT USAGE NOTE: Datastore entities that are not roots of entity groups (i.e. those + * that do not have a null parent in their Objectify keys) require the full entity group + * inheritance chain to be specified and thus cannot use this create method. You need to use + * {@link #create(Class, Object, Key)} instead and pass in the full, valid parent field in the + * Datastore key. + */ public static VKey create(Class kind, long id) { + checkArgument( + kind.isAssignableFrom(BackupGroupRoot.class), + "The kind %s is not a BackupGroupRoot and thus needs its entire entity group chain" + + " specified in a parent", + kind.getCanonicalName()); return new VKey(kind, Key.create(kind, id), id); } + /** + * Creates a symmetric {@link VKey} in which both sql and ofy keys are {@code name}. + * + *

IMPORTANT USAGE NOTE: Datastore entities that are not roots of entity groups (i.e. those + * that do not have a null parent in their Objectify keys) require the full entity group + * inheritance chain to be specified and thus cannot use this create method. You need to use + * {@link #create(Class, Object, Key)} instead and pass in the full, valid parent field in the + * Datastore key. + */ + public static VKey create(Class kind, String name) { + checkArgument( + kind.isAssignableFrom(BackupGroupRoot.class), + "The kind %s is not a BackupGroupRoot and thus needs its entire entity group chain" + + " specified in a parent", + kind.getCanonicalName()); + return new VKey(kind, Key.create(kind, name), name); + } + /** Returns the type of the entity. */ public Class getKind() { return this.kind; diff --git a/core/src/test/java/google/registry/model/contact/ContactResourceTest.java b/core/src/test/java/google/registry/model/contact/ContactResourceTest.java index 8979cfa00..f1037754e 100644 --- a/core/src/test/java/google/registry/model/contact/ContactResourceTest.java +++ b/core/src/test/java/google/registry/model/contact/ContactResourceTest.java @@ -31,7 +31,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import google.registry.model.EntityTestCase; import google.registry.model.ImmutableObjectSubject; -import google.registry.model.billing.BillingEvent; import google.registry.model.contact.Disclose.PostalInfoChoice; import google.registry.model.contact.PostalInfo.Type; import google.registry.model.eppcommon.AuthInfo.PasswordAuth; @@ -46,6 +45,7 @@ import org.junit.jupiter.api.Test; /** Unit tests for {@link ContactResource}. */ public class ContactResourceTest extends EntityTestCase { + private ContactResource originalContact; private ContactResource contactResource; @@ -113,8 +113,6 @@ public class ContactResourceTest extends EntityTestCase { .setGainingClientId("gaining") .setLosingClientId("losing") .setPendingTransferExpirationTime(fakeClock.nowUtc()) - .setServerApproveEntities( - ImmutableSet.of(VKey.create(BillingEvent.OneTime.class, 1))) .setTransferRequestTime(fakeClock.nowUtc()) .setTransferStatus(TransferStatus.SERVER_APPROVED) .setTransferRequestTrid(Trid.create("client-trid", "server-trid")) diff --git a/core/src/test/java/google/registry/model/domain/GracePeriodTest.java b/core/src/test/java/google/registry/model/domain/GracePeriodTest.java index a4eb11971..3ae94f976 100644 --- a/core/src/test/java/google/registry/model/domain/GracePeriodTest.java +++ b/core/src/test/java/google/registry/model/domain/GracePeriodTest.java @@ -98,6 +98,11 @@ public class GracePeriodTest { @Test void testFailure_createForRecurring_notAutoRenew() { + Key recurringKey = + Key.create( + Key.create(Key.create(DomainBase.class, "1-TEST"), HistoryEntry.class, 343L), + Recurring.class, + 12345); IllegalArgumentException thrown = assertThrows( IllegalArgumentException.class, @@ -107,7 +112,7 @@ public class GracePeriodTest { "1-TEST", now.plusDays(1), "TheRegistrar", - VKey.create(Recurring.class, 12345))); + VKey.create(Recurring.class, 12345, recurringKey))); assertThat(thrown).hasMessageThat().contains("autorenew"); } } diff --git a/core/src/test/java/google/registry/model/host/HostResourceTest.java b/core/src/test/java/google/registry/model/host/HostResourceTest.java index 78e49cfb9..9fac4423d 100644 --- a/core/src/test/java/google/registry/model/host/HostResourceTest.java +++ b/core/src/test/java/google/registry/model/host/HostResourceTest.java @@ -27,13 +27,11 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableSet; import com.google.common.net.InetAddresses; import google.registry.model.EntityTestCase; -import google.registry.model.billing.BillingEvent; import google.registry.model.domain.DomainBase; import google.registry.model.eppcommon.StatusValue; import google.registry.model.eppcommon.Trid; import google.registry.model.transfer.DomainTransferData; import google.registry.model.transfer.TransferStatus; -import google.registry.persistence.VKey; import org.joda.time.DateTime; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -62,8 +60,6 @@ class HostResourceTest extends EntityTestCase { .setGainingClientId("gaining") .setLosingClientId("losing") .setPendingTransferExpirationTime(fakeClock.nowUtc()) - .setServerApproveEntities( - ImmutableSet.of(VKey.create(BillingEvent.OneTime.class, 1))) .setTransferRequestTime(fakeClock.nowUtc()) .setTransferStatus(TransferStatus.SERVER_APPROVED) .setTransferRequestTrid(Trid.create("client-trid", "server-trid")) diff --git a/core/src/test/java/google/registry/model/transfer/TransferDataTest.java b/core/src/test/java/google/registry/model/transfer/TransferDataTest.java index 2ea01fd4b..d5a62cad9 100644 --- a/core/src/test/java/google/registry/model/transfer/TransferDataTest.java +++ b/core/src/test/java/google/registry/model/transfer/TransferDataTest.java @@ -18,10 +18,13 @@ import static com.google.common.truth.Truth.assertThat; import static org.joda.time.DateTimeZone.UTC; import com.google.common.collect.ImmutableSet; +import com.googlecode.objectify.Key; import google.registry.model.billing.BillingEvent; +import google.registry.model.domain.DomainBase; import google.registry.model.domain.Period; import google.registry.model.eppcommon.Trid; import google.registry.model.poll.PollMessage; +import google.registry.model.reporting.HistoryEntry; import google.registry.persistence.VKey; import google.registry.testing.AppEngineExtension; import org.joda.time.DateTime; @@ -46,11 +49,33 @@ public class TransferDataTest { @BeforeEach void beforeEach() { - transferBillingEventKey = VKey.create(BillingEvent.OneTime.class, 12345); - otherServerApproveBillingEventKey = VKey.create(BillingEvent.Cancellation.class, 2468); - recurringBillingEventKey = VKey.create(BillingEvent.Recurring.class, 13579); - autorenewPollMessageKey = VKey.create(PollMessage.Autorenew.class, 67890); - otherServerApprovePollMessageKey = VKey.create(PollMessage.OneTime.class, 314159); + Key historyEntryKey = + Key.create(Key.create(DomainBase.class, "4-TLD"), HistoryEntry.class, 1356L); + transferBillingEventKey = + VKey.create( + BillingEvent.OneTime.class, + 12345, + Key.create(historyEntryKey, BillingEvent.OneTime.class, 12345)); + otherServerApproveBillingEventKey = + VKey.create( + BillingEvent.Cancellation.class, + 2468, + Key.create(historyEntryKey, BillingEvent.Cancellation.class, 2468)); + recurringBillingEventKey = + VKey.create( + BillingEvent.Recurring.class, + 13579, + Key.create(historyEntryKey, BillingEvent.Recurring.class, 13579)); + autorenewPollMessageKey = + VKey.create( + PollMessage.Autorenew.class, + 67890, + Key.create(historyEntryKey, PollMessage.Autorenew.class, 67890)); + otherServerApprovePollMessageKey = + VKey.create( + PollMessage.OneTime.class, + 314159, + Key.create(historyEntryKey, PollMessage.OneTime.class, 314159)); } @Test diff --git a/core/src/test/java/google/registry/persistence/VKeyTest.java b/core/src/test/java/google/registry/persistence/VKeyTest.java index 9ec3f55bd..ca392cda1 100644 --- a/core/src/test/java/google/registry/persistence/VKeyTest.java +++ b/core/src/test/java/google/registry/persistence/VKeyTest.java @@ -14,8 +14,12 @@ package google.registry.persistence; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; import com.googlecode.objectify.Key; +import google.registry.model.billing.BillingEvent.OneTime; +import google.registry.model.registrar.RegistrarContact; import google.registry.testing.AppEngineExtension; import google.registry.testing.TestObject; import org.junit.jupiter.api.Test; @@ -37,7 +41,22 @@ class VKeyTest { VKey.create(TestObject.class, "foo", Key.create(TestObject.create("foo"))); assertThat(key.maybeGetSqlKey().isPresent()).isTrue(); assertThat(key.maybeGetOfyKey().isPresent()).isTrue(); + assertThat(VKey.createSql(TestObject.class, "foo").maybeGetSqlKey()).hasValue("foo"); + } - assertThat(VKey.createSql(TestObject.class, "foo").maybeGetSqlKey().get()).isEqualTo("foo"); + @Test + void testCreateById_failsWhenParentIsNullButShouldntBe() { + IllegalArgumentException thrown = + assertThrows(IllegalArgumentException.class, () -> VKey.create(OneTime.class, 134L)); + assertThat(thrown).hasMessageThat().contains("BackupGroupRoot"); + } + + @Test + void testCreateByName_failsWhenParentIsNullButShouldntBe() { + IllegalArgumentException thrown = + assertThrows( + IllegalArgumentException.class, + () -> VKey.create(RegistrarContact.class, "fake@example.com")); + assertThat(thrown).hasMessageThat().contains("BackupGroupRoot"); } }