From 237e588d6ca21652d2a24c3806d60aff702489e0 Mon Sep 17 00:00:00 2001
From: mountford <mountford@google.com>
Date: Fri, 30 Sep 2016 14:49:27 -0700
Subject: [PATCH] Do not require fee extension on free updates

This CL fixes a bug introduced in [] which caused an exception to be thrown when an attempt was made to update a domain without a fee extension, even if the update was free, as it usually is. The fee extension should only be required if the update is not free.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=134830250
---
 .../flows/domain/DomainUpdateFlow.java        | 14 ++-
 .../flows/domain/DomainUpdateFlowTest.java    | 93 +++++++++++++++++--
 .../domain_update_addremove_flags.xml         |  2 +-
 .../domain_update_addremove_flags_fee.xml     |  2 +-
 4 files changed, 98 insertions(+), 13 deletions(-)

diff --git a/java/google/registry/flows/domain/DomainUpdateFlow.java b/java/google/registry/flows/domain/DomainUpdateFlow.java
index c2588c173..db0024702 100644
--- a/java/google/registry/flows/domain/DomainUpdateFlow.java
+++ b/java/google/registry/flows/domain/DomainUpdateFlow.java
@@ -39,6 +39,7 @@ import google.registry.model.registry.Registry;
 import google.registry.model.reporting.HistoryEntry;
 import java.util.Set;
 import javax.inject.Inject;
+import org.joda.money.Money;
 import org.joda.time.DateTime;
 
 /**
@@ -150,12 +151,17 @@ public class DomainUpdateFlow extends BaseDomainUpdateFlow<DomainResource, Build
         getClientId(),
         now,
         eppInput);
-    // The fee extension must be present if the update is not free.
-    if ((feeUpdate == null) && !commandOperations.getTotalCost().isZero()) {
+
+    // If the fee extension is present, validate it (even if the cost is zero, to check for price
+    // mismatches). Don't rely on the the validateFeeChallenge check for feeUpdate nullness, because
+    // it throws an error if the name is premium, and we don't want to do that here.
+    Money totalCost = commandOperations.getTotalCost();
+    if (feeUpdate != null) {
+      validateFeeChallenge(targetId, existingResource.getTld(), now, feeUpdate, totalCost);
+    // If it's not present but the cost is not zero, throw an exception.
+    } else if (!totalCost.isZero()) {
       throw new FeesRequiredForNonFreeUpdateException();
     }
-    validateFeeChallenge(
-        targetId, existingResource.getTld(), now, feeUpdate, commandOperations.getTotalCost());
   }
 
   @Override
diff --git a/javatests/google/registry/flows/domain/DomainUpdateFlowTest.java b/javatests/google/registry/flows/domain/DomainUpdateFlowTest.java
index 3762a0665..1a871729c 100644
--- a/javatests/google/registry/flows/domain/DomainUpdateFlowTest.java
+++ b/javatests/google/registry/flows/domain/DomainUpdateFlowTest.java
@@ -30,6 +30,7 @@ import static google.registry.testing.DatastoreHelper.persistActiveDomain;
 import static google.registry.testing.DatastoreHelper.persistActiveHost;
 import static google.registry.testing.DatastoreHelper.persistActiveSubordinateHost;
 import static google.registry.testing.DatastoreHelper.persistDeletedDomain;
+import static google.registry.testing.DatastoreHelper.persistPremiumList;
 import static google.registry.testing.DatastoreHelper.persistResource;
 import static google.registry.testing.DomainResourceSubject.assertAboutDomains;
 import static google.registry.testing.HistoryEntrySubject.assertAboutHistoryEntries;
@@ -107,6 +108,14 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
   @Before
   public void initDomainTest() {
     createTlds("tld", "flags");
+    // Super-hack premium list: The domain name has to be of the form feetype-amount for the
+    // test extra logic manager to be happy. So we use domain name "renew-0" to designate a premium
+    // name with a zero-cost update. This has nothing to do with renewals -- we just need to use a
+    // valid fee type.
+    persistResource(
+        Registry.get("flags").asBuilder()
+            .setPremiumList(persistPremiumList("flags", "renew-0,USD 100"))
+            .build());
     // For flags extension tests.
     RegistryExtraFlowLogicProxy.setOverride("flags", TestExtraLogicManager.class);
   }
@@ -1182,18 +1191,21 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
     runFlow();
   }
 
+  // This test should work, because the fee extension is not required when the fee is zero.
   @Test
-  public void testAddAndRemoveFlags_noFee() throws Exception {
-    setEppInput("domain_update_addremove_flags.xml");
+  public void testAddAndRemoveFlags_free_noFee() throws Exception {
+    setEppInput("domain_update_addremove_flags.xml", ImmutableMap.of("DOMAIN", "update-0"));
     persistReferencedEntities();
     persistDomain();
-    thrown.expect(FeesRequiredForNonFreeUpdateException.class);
+    thrown.expect(IllegalArgumentException.class, "add:flag1,flag2;remove:flag3,flag4");
     runFlow();
   }
 
   @Test
-  public void testAddAndRemoveFlags_wrongFee() throws Exception {
-    setEppInput("domain_update_addremove_flags_fee.xml", ImmutableMap.of("FEE", "11.00"));
+  public void testAddAndRemoveFlags_free_wrongFee() throws Exception {
+    setEppInput(
+        "domain_update_addremove_flags_fee.xml",
+        ImmutableMap.of("DOMAIN", "update-0", "FEE", "1.00"));
     persistReferencedEntities();
     persistDomain();
     thrown.expect(FeesMismatchException.class);
@@ -1201,8 +1213,75 @@ public class DomainUpdateFlowTest extends ResourceFlowTestCase<DomainUpdateFlow,
   }
 
   @Test
-  public void testAddAndRemoveFlags_correctFee() throws Exception {
-    setEppInput("domain_update_addremove_flags_fee.xml", ImmutableMap.of("FEE", "13.00"));
+  public void testAddAndRemoveFlags_free_correctFee() throws Exception {
+    setEppInput(
+        "domain_update_addremove_flags_fee.xml",
+        ImmutableMap.of("DOMAIN", "update-0", "FEE", "0.00"));
+    persistReferencedEntities();
+    persistDomain();
+    thrown.expect(IllegalArgumentException.class, "add:flag1,flag2;remove:flag3,flag4");
+    runFlow();
+  }
+
+  // This should also work, even though the domain is premium (previously, a bug caused it to fail).
+  @Test
+  public void testAddAndRemoveFlags_freePremium_noFee() throws Exception {
+    setEppInput("domain_update_addremove_flags.xml", ImmutableMap.of("DOMAIN", "renew-0"));
+    persistReferencedEntities();
+    persistDomain();
+    thrown.expect(IllegalArgumentException.class, "add:flag1,flag2;remove:flag3,flag4");
+    runFlow();
+  }
+
+  @Test
+  public void testAddAndRemoveFlags_freePremium_wrongFee() throws Exception {
+    setEppInput(
+        "domain_update_addremove_flags_fee.xml",
+        ImmutableMap.of("DOMAIN", "renew-0", "FEE", "1.00"));
+    persistReferencedEntities();
+    persistDomain();
+    thrown.expect(FeesMismatchException.class);
+    runFlow();
+  }
+
+  @Test
+  public void testAddAndRemoveFlags_freePremium_correctFee() throws Exception {
+    setEppInput(
+        "domain_update_addremove_flags_fee.xml",
+        ImmutableMap.of("DOMAIN", "renew-0", "FEE", "0.00"));
+    persistReferencedEntities();
+    persistDomain();
+    thrown.expect(IllegalArgumentException.class, "add:flag1,flag2;remove:flag3,flag4");
+    runFlow();
+  }
+
+  // This test should throw an exception, because the fee extension is required when the fee is not
+  // zero.
+  @Test
+  public void testAddAndRemoveFlags_paid_noFee() throws Exception {
+    setEppInput("domain_update_addremove_flags.xml", ImmutableMap.of("DOMAIN", "update-13"));
+    persistReferencedEntities();
+    persistDomain();
+    thrown.expect(FeesRequiredForNonFreeUpdateException.class);
+    runFlow();
+  }
+
+  @Test
+  public void testAddAndRemoveFlags_paid_wrongFee() throws Exception {
+    setEppInput(
+        "domain_update_addremove_flags_fee.xml",
+        ImmutableMap.of("DOMAIN", "update-13", "FEE", "11.00"));
+    persistReferencedEntities();
+    persistDomain();
+    thrown.expect(FeesMismatchException.class);
+    runFlow();
+  }
+
+  @Test
+  public void testAddAndRemoveFlags_paid_correctFee() throws Exception {
+    setEppInput(
+        "domain_update_addremove_flags_fee.xml",
+        ImmutableMap.of("DOMAIN", "update-13", "FEE", "13.00"));
     persistReferencedEntities();
     persistDomain();
     thrown.expect(IllegalArgumentException.class, "add:flag1,flag2;remove:flag3,flag4");
diff --git a/javatests/google/registry/flows/domain/testdata/domain_update_addremove_flags.xml b/javatests/google/registry/flows/domain/testdata/domain_update_addremove_flags.xml
index 3eb86d5c9..9c1734e0d 100644
--- a/javatests/google/registry/flows/domain/testdata/domain_update_addremove_flags.xml
+++ b/javatests/google/registry/flows/domain/testdata/domain_update_addremove_flags.xml
@@ -3,7 +3,7 @@
     <update>
       <domain:update
        xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
-        <domain:name>update-13.flags</domain:name>
+        <domain:name>%DOMAIN%.flags</domain:name>
       </domain:update>
     </update>
     <extension>
diff --git a/javatests/google/registry/flows/domain/testdata/domain_update_addremove_flags_fee.xml b/javatests/google/registry/flows/domain/testdata/domain_update_addremove_flags_fee.xml
index f57e67c8f..19b3f43e4 100644
--- a/javatests/google/registry/flows/domain/testdata/domain_update_addremove_flags_fee.xml
+++ b/javatests/google/registry/flows/domain/testdata/domain_update_addremove_flags_fee.xml
@@ -3,7 +3,7 @@
     <update>
       <domain:update
        xmlns:domain="urn:ietf:params:xml:ns:domain-1.0">
-        <domain:name>update-13.flags</domain:name>
+        <domain:name>%DOMAIN%.flags</domain:name>
       </domain:update>
     </update>
     <extension>