Skip to content

Commit c7f2db1

Browse files
gbrodmangbrodman
authored
Forbid contacts earlier in the domain EPP parsing process (#2962)
This will make testing easier, as well as allow us to remove contact code from other parts of the codebase.
1 parent 6747cc8 commit c7f2db1

File tree

7 files changed

+119
-88
lines changed

7 files changed

+119
-88
lines changed

‎core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1252,7 +1252,7 @@ public InvalidIdnDomainLabelException() {
12521252
}
12531253

12541254
/** Having a registrant is prohibited by registry policy. */
1255-
static class RegistrantProhibitedException extends ParameterValuePolicyErrorException {
1255+
public static class RegistrantProhibitedException extends ParameterValuePolicyErrorException {
12561256
public RegistrantProhibitedException() {
12571257
super("Having a registrant is prohibited by registry policy");
12581258
}

‎core/src/main/java/google/registry/model/domain/DomainCommand.java‎

Lines changed: 32 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -16,20 +16,18 @@
1616

1717
import static com.google.common.base.MoreObjects.firstNonNull;
1818
import static com.google.common.base.Preconditions.checkNotNull;
19-
import static com.google.common.collect.Iterables.getOnlyElement;
2019
import static com.google.common.collect.Sets.difference;
2120
import static google.registry.util.CollectionUtils.difference;
22-
import static google.registry.util.CollectionUtils.forceEmptyToNull;
21+
import static google.registry.util.CollectionUtils.isNullOrEmpty;
2322
import static google.registry.util.CollectionUtils.nullSafeImmutableCopy;
24-
import static google.registry.util.CollectionUtils.nullToEmpty;
2523
import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy;
26-
import static google.registry.util.CollectionUtils.union;
2724

2825
import com.google.common.base.MoreObjects;
29-
import com.google.common.base.Strings;
3026
import com.google.common.collect.ImmutableMap;
3127
import com.google.common.collect.ImmutableSet;
32-
import google.registry.model.EppResource;
28+
import google.registry.flows.EppException.ParameterValuePolicyErrorException;
29+
import google.registry.flows.domain.DomainFlowUtils.RegistrantProhibitedException;
30+
import google.registry.flows.exceptions.ContactsProhibitedException;
3331
import google.registry.model.ForeignKeyUtils;
3432
import google.registry.model.ImmutableObject;
3533
import google.registry.model.contact.Contact;
@@ -67,7 +65,8 @@ public class DomainCommand {
6765
*/
6866
public interface CreateOrUpdate<T extends CreateOrUpdate<T>> extends SingleResourceCommand {
6967
/** Creates a copy of this command with hard links to hosts and contacts. */
70-
T cloneAndLinkReferences(DateTime now) throws InvalidReferencesException;
68+
T cloneAndLinkReferences(DateTime now)
69+
throws InvalidReferencesException, ParameterValuePolicyErrorException;
7170
}
7271

7372
/** The fields on "chgType" from <a href="http://tools.ietf.org/html/rfc5731">RFC5731</a>. */
@@ -171,26 +170,15 @@ public DomainAuthInfo getAuthInfo() {
171170

172171
/** Creates a copy of this {@link Create} with hard links to hosts and contacts. */
173172
@Override
174-
public Create cloneAndLinkReferences(DateTime now) throws InvalidReferencesException {
173+
public Create cloneAndLinkReferences(DateTime now)
174+
throws InvalidReferencesException, ParameterValuePolicyErrorException {
175175
Create clone = clone(this);
176176
clone.nameservers = linkHosts(clone.nameserverHostNames, now);
177-
if (registrantContactId == null) {
178-
clone.contacts = linkContacts(clone.foreignKeyedDesignatedContacts, now);
179-
} else {
180-
// Load the registrant and contacts in one shot.
181-
ForeignKeyedDesignatedContact registrantPlaceholder = new ForeignKeyedDesignatedContact();
182-
registrantPlaceholder.contactId = clone.registrantContactId;
183-
registrantPlaceholder.type = DesignatedContact.Type.REGISTRANT;
184-
Set<DesignatedContact> contacts = linkContacts(
185-
union(nullToEmpty(clone.foreignKeyedDesignatedContacts), registrantPlaceholder),
186-
now);
187-
for (DesignatedContact contact : contacts) {
188-
if (DesignatedContact.Type.REGISTRANT.equals(contact.getType())) {
189-
clone.registrant = contact.getContactKey();
190-
clone.contacts = forceEmptyToNull(difference(contacts, contact));
191-
break;
192-
}
193-
}
177+
if (registrantContactId != null) {
178+
throw new RegistrantProhibitedException();
179+
}
180+
if (!isNullOrEmpty(foreignKeyedDesignatedContacts)) {
181+
throw new ContactsProhibitedException();
194182
}
195183
return clone;
196184
}
@@ -369,27 +357,25 @@ public ImmutableSet<DesignatedContact> getContacts() {
369357
}
370358

371359
/** Creates a copy of this {@link AddRemove} with hard links to hosts and contacts. */
372-
private AddRemove cloneAndLinkReferences(DateTime now) throws InvalidReferencesException {
360+
private AddRemove cloneAndLinkReferences(DateTime now)
361+
throws InvalidReferencesException, ContactsProhibitedException {
373362
AddRemove clone = clone(this);
374363
clone.nameservers = linkHosts(clone.nameserverHostNames, now);
375-
clone.contacts = linkContacts(clone.foreignKeyedDesignatedContacts, now);
364+
if (!isNullOrEmpty(foreignKeyedDesignatedContacts)) {
365+
throw new ContactsProhibitedException();
366+
}
376367
return clone;
377368
}
378369
}
379370

380371
/** The inner change type on a domain update command. */
381372
@XmlType(propOrder = {"registrantContactId", "authInfo"})
382373
public static class Change extends DomainCreateOrChange<Domain.Builder> {
383-
/** Creates a copy of this {@link Change} with hard links to hosts and contacts. */
384-
Change cloneAndLinkReferences(DateTime now) throws InvalidReferencesException {
374+
Change cloneAndLinkReferences() throws RegistrantProhibitedException {
385375
Change clone = clone(this);
386-
clone.registrant =
387-
Strings.isNullOrEmpty(clone.registrantContactId)
388-
? null
389-
: getOnlyElement(
390-
loadByForeignKeysCached(
391-
ImmutableSet.of(clone.registrantContactId), Contact.class, now)
392-
.values());
376+
if (clone.registrantContactId != null) {
377+
throw new RegistrantProhibitedException();
378+
}
393379
return clone;
394380
}
395381
}
@@ -401,11 +387,12 @@ Change cloneAndLinkReferences(DateTime now) throws InvalidReferencesException {
401387
* of those classes, which is harmless because the getters do that anyways.
402388
*/
403389
@Override
404-
public Update cloneAndLinkReferences(DateTime now) throws InvalidReferencesException {
390+
public Update cloneAndLinkReferences(DateTime now)
391+
throws InvalidReferencesException, ParameterValuePolicyErrorException {
405392
Update clone = clone(this);
406393
clone.innerAdd = clone.getInnerAdd().cloneAndLinkReferences(now);
407394
clone.innerRemove = clone.getInnerRemove().cloneAndLinkReferences(now);
408-
clone.innerChange = clone.getInnerChange().cloneAndLinkReferences(now);
395+
clone.innerChange = clone.getInnerChange().cloneAndLinkReferences();
409396
return clone;
410397
}
411398
}
@@ -415,37 +402,17 @@ private static Set<VKey<Host>> linkHosts(Set<String> hostNames, DateTime now)
415402
if (hostNames == null) {
416403
return null;
417404
}
418-
return ImmutableSet.copyOf(loadByForeignKeysCached(hostNames, Host.class, now).values());
405+
return ImmutableSet.copyOf(loadByForeignKeysCached(hostNames, now).values());
419406
}
420407

421-
private static Set<DesignatedContact> linkContacts(
422-
Set<ForeignKeyedDesignatedContact> contacts, DateTime now) throws InvalidReferencesException {
423-
if (contacts == null) {
424-
return null;
425-
}
426-
ImmutableSet.Builder<String> foreignKeys = new ImmutableSet.Builder<>();
427-
for (ForeignKeyedDesignatedContact contact : contacts) {
428-
foreignKeys.add(contact.contactId);
429-
}
430-
ImmutableMap<String, VKey<Contact>> loadedContacts =
431-
loadByForeignKeysCached(foreignKeys.build(), Contact.class, now);
432-
ImmutableSet.Builder<DesignatedContact> linkedContacts = new ImmutableSet.Builder<>();
433-
for (ForeignKeyedDesignatedContact contact : contacts) {
434-
linkedContacts.add(
435-
DesignatedContact.create(contact.type, loadedContacts.get(contact.contactId)));
436-
}
437-
return linkedContacts.build();
438-
}
439-
440-
/** Loads keys to cached EPP resources by their foreign keys. */
441-
private static <T extends EppResource> ImmutableMap<String, VKey<T>> loadByForeignKeysCached(
442-
final Set<String> foreignKeys, final Class<T> clazz, final DateTime now)
443-
throws InvalidReferencesException {
444-
ImmutableMap<String, VKey<T>> fks =
445-
ForeignKeyUtils.loadKeysByCacheIfEnabled(clazz, foreignKeys, now);
408+
/** Loads host keys to cached EPP resources by their foreign keys. */
409+
private static ImmutableMap<String, VKey<Host>> loadByForeignKeysCached(
410+
final Set<String> foreignKeys, final DateTime now) throws InvalidReferencesException {
411+
ImmutableMap<String, VKey<Host>> fks =
412+
ForeignKeyUtils.loadKeysByCacheIfEnabled(Host.class, foreignKeys, now);
446413
if (!fks.keySet().equals(foreignKeys)) {
447414
throw new InvalidReferencesException(
448-
clazz, ImmutableSet.copyOf(difference(foreignKeys, fks.keySet())));
415+
Host.class, ImmutableSet.copyOf(difference(foreignKeys, fks.keySet())));
449416
}
450417
return fks;
451418
}

‎core/src/test/java/google/registry/model/domain/DomainCommandTest.java‎

Lines changed: 33 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,11 @@
1414

1515
package google.registry.model.domain;
1616

17-
import static google.registry.testing.DatabaseHelper.persistActiveContact;
1817
import static google.registry.testing.DatabaseHelper.persistActiveHost;
18+
import static org.junit.Assert.assertThrows;
1919

20+
import google.registry.flows.domain.DomainFlowUtils.RegistrantProhibitedException;
21+
import google.registry.flows.exceptions.ContactsProhibitedException;
2022
import google.registry.model.ResourceCommandTestCase;
2123
import google.registry.model.eppinput.EppInput;
2224
import google.registry.model.eppinput.EppInput.ResourceCommandWrapper;
@@ -29,6 +31,7 @@ class DomainCommandTest extends ResourceCommandTestCase {
2931

3032
@Test
3133
void testCreate() throws Exception {
34+
// This EPP command wouldn't be allowed for policy reasons, but should marshal/unmarshal fine.
3235
doXmlRoundtripTest("domain_create.xml");
3336
}
3437

@@ -64,42 +67,48 @@ void testCreate_fee() throws Exception {
6467

6568
@Test
6669
void testCreate_emptyCommand() throws Exception {
67-
// This EPP command wouldn't be allowed for policy reasons, but should marshal/unmarshal fine.
6870
doXmlRoundtripTest("domain_create_empty.xml");
6971
}
7072

7173
@Test
7274
void testCreate_missingNonRegistrantContacts() throws Exception {
73-
// This EPP command wouldn't be allowed for policy reasons, but should marshal/unmarshal fine.
7475
doXmlRoundtripTest("domain_create_missing_non_registrant_contacts.xml");
7576
}
7677

7778
@Test
78-
void testCreate_cloneAndLinkReferences() throws Exception {
79+
void testCreate_cloneAndLinkReferences_withHosts() throws Exception {
7980
persistActiveHost("ns1.example.net");
8081
persistActiveHost("ns2.example.net");
81-
persistActiveContact("sh8013");
82-
persistActiveContact("jd1234");
8382
DomainCommand.Create create =
8483
(DomainCommand.Create) loadEppResourceCommand("domain_create.xml");
8584
create.cloneAndLinkReferences(fakeClock.nowUtc());
8685
}
8786

8887
@Test
89-
void testCreate_emptyCommand_cloneAndLinkReferences() throws Exception {
90-
// This EPP command wouldn't be allowed for policy reasons, but should clone-and-link fine.
88+
void testCreate_cloneAndLinkReferences_failsWithContacts() throws Exception {
89+
persistActiveHost("ns1.example.net");
90+
persistActiveHost("ns2.example.net");
9191
DomainCommand.Create create =
92-
(DomainCommand.Create) loadEppResourceCommand("domain_create_empty.xml");
93-
create.cloneAndLinkReferences(fakeClock.nowUtc());
92+
(DomainCommand.Create) loadEppResourceCommand("domain_create_with_contacts.xml");
93+
assertThrows(
94+
RegistrantProhibitedException.class,
95+
() -> create.cloneAndLinkReferences(fakeClock.nowUtc()));
9496
}
9597

9698
@Test
97-
void testCreate_missingNonRegistrantContacts_cloneAndLinkReferences() throws Exception {
98-
persistActiveContact("jd1234");
99-
// This EPP command wouldn't be allowed for policy reasons, but should clone-and-link fine.
99+
void testCreate_cloneAndLinkReferences_failsWithRegistrant() throws Exception {
100100
DomainCommand.Create create =
101101
(DomainCommand.Create)
102102
loadEppResourceCommand("domain_create_missing_non_registrant_contacts.xml");
103+
assertThrows(
104+
RegistrantProhibitedException.class,
105+
() -> create.cloneAndLinkReferences(fakeClock.nowUtc()));
106+
}
107+
108+
@Test
109+
void testCreate_emptyCommand_cloneAndLinkReferences() throws Exception {
110+
DomainCommand.Create create =
111+
(DomainCommand.Create) loadEppResourceCommand("domain_create_empty.xml");
103112
create.cloneAndLinkReferences(fakeClock.nowUtc());
104113
}
105114

@@ -120,16 +129,24 @@ void testUpdate_emptyCommand() throws Exception {
120129
}
121130

122131
@Test
123-
void testUpdate_cloneAndLinkReferences() throws Exception {
132+
void testUpdate_cloneAndLinkReferences_hosts() throws Exception {
124133
persistActiveHost("ns1.example.com");
125134
persistActiveHost("ns2.example.com");
126-
persistActiveContact("mak21");
127-
persistActiveContact("sh8013");
128135
DomainCommand.Update update =
129136
(DomainCommand.Update) loadEppResourceCommand("domain_update.xml");
130137
update.cloneAndLinkReferences(fakeClock.nowUtc());
131138
}
132139

140+
@Test
141+
void testUpdate_cloneAndLinkReferences_failsWithContacts() throws Exception {
142+
persistActiveHost("ns1.example.com");
143+
persistActiveHost("ns2.example.com");
144+
DomainCommand.Update update =
145+
(DomainCommand.Update) loadEppResourceCommand("domain_update_with_contacts.xml");
146+
assertThrows(
147+
ContactsProhibitedException.class, () -> update.cloneAndLinkReferences(fakeClock.nowUtc()));
148+
}
149+
133150
@Test
134151
void testUpdate_emptyCommand_cloneAndLinkReferences() throws Exception {
135152
// This EPP command wouldn't be allowed for policy reasons, but should clone-and-link fine.

‎core/src/test/resources/google/registry/model/domain/domain_create.xml‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,6 @@
99
<domain:hostObj>ns1.example.net</domain:hostObj>
1010
<domain:hostObj>ns2.example.net</domain:hostObj>
1111
</domain:ns>
12-
<domain:registrant>jd1234</domain:registrant>
13-
<domain:contact type="admin">sh8013</domain:contact>
14-
<domain:contact type="tech">sh8013</domain:contact>
1512
<domain:authInfo>
1613
<domain:pw>2fooBAR</domain:pw>
1714
</domain:authInfo>

‎core/src/test/resources/google/registry/model/domain/domain_create_with_contacts.xml‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
2+
<command>
3+
<create>
4+
<domain:create
5+
xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
6+
<domain:name>example.com</domain:name>
7+
<domain:period unit="y">2</domain:period>
8+
<domain:ns>
9+
<domain:hostObj>ns1.example.net</domain:hostObj>
10+
<domain:hostObj>ns2.example.net</domain:hostObj>
11+
</domain:ns>
12+
<domain:registrant>jd1234</domain:registrant>
13+
<domain:contact type="admin">sh8013</domain:contact>
14+
<domain:contact type="tech">sh8013</domain:contact>
15+
<domain:authInfo>
16+
<domain:pw>2fooBAR</domain:pw>
17+
</domain:authInfo>
18+
</domain:create>
19+
</create>
20+
<clTRID>ABC-12345</clTRID>
21+
</command>
22+
</epp>

‎core/src/test/resources/google/registry/model/domain/domain_update.xml‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,15 @@
88
<domain:ns>
99
<domain:hostObj>ns2.example.com</domain:hostObj>
1010
</domain:ns>
11-
<domain:contact type="tech">mak21</domain:contact>
1211
<domain:status s="clientHold"/>
1312
</domain:add>
1413
<domain:rem>
1514
<domain:ns>
1615
<domain:hostObj>ns1.example.com</domain:hostObj>
1716
</domain:ns>
18-
<domain:contact type="tech">sh8013</domain:contact>
1917
<domain:status s="clientUpdateProhibited"/>
2018
</domain:rem>
2119
<domain:chg>
22-
<domain:registrant>sh8013</domain:registrant>
2320
<domain:authInfo>
2421
<domain:pw>2BARfoo</domain:pw>
2522
</domain:authInfo>

‎core/src/test/resources/google/registry/model/domain/domain_update_with_contacts.xml‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<epp xmlns="urn:ietf:params:xml:ns:epp-1.0">
2+
<command>
3+
<update>
4+
<domain:update
5+
xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
6+
<domain:name>example.com</domain:name>
7+
<domain:add>
8+
<domain:ns>
9+
<domain:hostObj>ns2.example.com</domain:hostObj>
10+
</domain:ns>
11+
<domain:contact type="tech">mak21</domain:contact>
12+
<domain:status s="clientHold"/>
13+
</domain:add>
14+
<domain:rem>
15+
<domain:ns>
16+
<domain:hostObj>ns1.example.com</domain:hostObj>
17+
</domain:ns>
18+
<domain:contact type="tech">sh8013</domain:contact>
19+
<domain:status s="clientUpdateProhibited"/>
20+
</domain:rem>
21+
<domain:chg>
22+
<domain:registrant>sh8013</domain:registrant>
23+
<domain:authInfo>
24+
<domain:pw>2BARfoo</domain:pw>
25+
</domain:authInfo>
26+
</domain:chg>
27+
</domain:update>
28+
</update>
29+
<clTRID>ABC-12345</clTRID>
30+
</command>
31+
</epp>

0 commit comments

Comments
 (0)