From e9ad1b6f7260aa330039cff9b78fad1f3993d69f Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Mon, 22 Jun 2020 15:24:36 -0400 Subject: [PATCH] Don't expect a renewal fee on restores when one isn't due (#637) * Don't expect a renewal fee on restores when one isn't due This is a fix on top of #632 so that domain restore commands don't require acking an illusory renewal fee for 1 year when that isn't actually happening (i.e. if the domain isn't yet past its original expiration). Unfortunately, there's still a problem remaining wherein the restore fee on a domain check will always include the additional year even if it's not necessary. We don't have a good solution to that. Also note that in versions of the fee extension more recent than 0.6, the fee extension cannot be passed on a domain info command at all, so the domain check command is the only way you have to determine what the restore fee should be. So we definitely do want to get that right so that the apparent restore fee on a check is the same as the actual restore fee when running the restore command. We're not quite there yet though and it's hard to say how we will get there, since we don't load domains during a domain check command for performance reasons yet we would need to do so in order to know the expiration date and thus whether the additional year of renewal should be charged. A problem for a future PR. --- .../flows/domain/DomainCheckFlow.java | 1 + .../flows/domain/DomainFlowUtils.java | 25 +++++++-- .../registry/flows/domain/DomainInfoFlow.java | 1 + .../flows/domain/DomainPricingLogic.java | 20 ++++---- .../domain/DomainRestoreRequestFlow.java | 40 +++++++++------ .../flows/domain/DomainInfoFlowTest.java | 33 ++++++++++++ .../domain/DomainRestoreRequestFlowTest.java | 20 +++++++- ...main_info_fee_restore_premium_response.xml | 1 - .../domain_info_fee_restore_response.xml | 1 - ...n_info_fee_restore_response_no_renewal.xml | 49 ++++++++++++++++++ ...info_fee_restore_response_with_renewal.xml | 51 +++++++++++++++++++ ..._update_restore_request_fee_no_renewal.xml | 21 ++++++++ ...ate_restore_request_premium_no_renewal.xml | 23 +++++++++ ...estore_request_response_fee_no_renewal.xml | 17 +++++++ 14 files changed, 269 insertions(+), 34 deletions(-) create mode 100644 core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response_no_renewal.xml create mode 100644 core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response_with_renewal.xml create mode 100644 core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_fee_no_renewal.xml create mode 100644 core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_premium_no_renewal.xml create mode 100644 core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_response_fee_no_renewal.xml diff --git a/core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java b/core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java index 8b42748da..939ff3e70 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainCheckFlow.java @@ -247,6 +247,7 @@ public final class DomainCheckFlow implements Flow { feeCheckItem, builder, domainNames.get(domainName), + Optional.empty(), feeCheck.getCurrency(), now, pricingLogic, diff --git a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java index b98451ba2..983c58dfc 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java @@ -551,7 +551,8 @@ public class DomainFlowUtils { static void handleFeeRequest( FeeQueryCommandExtensionItem feeRequest, FeeQueryResponseExtensionItem.Builder builder, - InternetDomainName domain, + InternetDomainName domainName, + Optional domain, @Nullable CurrencyUnit topLevelCurrency, DateTime currentDate, DomainPricingLogic pricingLogic, @@ -564,8 +565,8 @@ public class DomainFlowUtils { now = feeRequest.getEffectiveDate().get(); builder.setEffectiveDateIfSupported(now); } - String domainNameString = domain.toString(); - Registry registry = Registry.get(domain.parent().toString()); + String domainNameString = domainName.toString(); + Registry registry = Registry.get(domainName.parent().toString()); int years = verifyUnitIsYears(feeRequest.getPeriod()).getValue(); boolean isSunrise = (registry.getTldState(now) == START_DATE_SUNRISE); @@ -589,7 +590,7 @@ public class DomainFlowUtils { switch (feeRequest.getCommandName()) { case CREATE: // Don't return a create price for reserved names. - if (isReserved(domain, isSunrise) && !isAvailable) { + if (isReserved(domainName, isSunrise) && !isAvailable) { builder.setClass("reserved"); // Override whatever class we've set above. builder.setAvailIfSupported(false); builder.setReasonIfSupported("reserved"); @@ -606,11 +607,25 @@ public class DomainFlowUtils { fees = pricingLogic.getRenewPrice(registry, domainNameString, now, years).getFees(); break; case RESTORE: + // The minimum allowable period per the EPP spec is 1, so, strangely, 1 year still has to be + // passed in as the period for a restore even if the domain would *not* be renewed as part + // of a restore. This is fixed in RFC 8748 (which is a more recent version of the fee + // extension than we currently support), which does not allow the period to be passed at all + // on a restore fee check. if (years != 1) { throw new RestoresAreAlwaysForOneYearException(); } builder.setAvailIfSupported(true); - fees = pricingLogic.getRestorePrice(registry, domainNameString, now).getFees(); + // The domain object is present only on domain info commands, not on domain check commands, + // because check commands can query up to 50 domains and it isn't performant to load them + // all. So, only on info commands can we actually determine if we should include the renewal + // fee because the domain needs to have been loaded in order to know its expiration time. We + // default to including the renewal fee on domain checks because typically most domains are + // deleted during the autorenew grace period and thus if restored will require a renewal, + // but this is just a best guess. + boolean isExpired = + !domain.isPresent() || domain.get().getRegistrationExpirationTime().isBefore(now); + fees = pricingLogic.getRestorePrice(registry, domainNameString, now, isExpired).getFees(); break; case TRANSFER: if (years != 1) { diff --git a/core/src/main/java/google/registry/flows/domain/DomainInfoFlow.java b/core/src/main/java/google/registry/flows/domain/DomainInfoFlow.java index fd1e47a62..7beb3827b 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainInfoFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainInfoFlow.java @@ -158,6 +158,7 @@ public final class DomainInfoFlow implements Flow { feeInfo.get(), builder, InternetDomainName.from(targetId), + Optional.of(domain), null, now, pricingLogic, diff --git a/core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java b/core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java index abb3b62d5..988ef03e2 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java +++ b/core/src/main/java/google/registry/flows/domain/DomainPricingLogic.java @@ -124,23 +124,23 @@ public final class DomainPricingLogic { } /** Returns a new restore price for the pricer. */ - public FeesAndCredits getRestorePrice(Registry registry, String domainName, DateTime dateTime) + public FeesAndCredits getRestorePrice( + Registry registry, String domainName, DateTime dateTime, boolean isExpired) throws EppException { DomainPrices domainPrices = getPricesForDomainName(domainName, dateTime); - FeesAndCredits feesAndCredits = + FeesAndCredits.Builder feesAndCredits = new FeesAndCredits.Builder() .setCurrency(registry.getCurrency()) .addFeeOrCredit( - Fee.create( - domainPrices.getRenewCost().getAmount(), - FeeType.RENEW, - domainPrices.isPremium())) - .addFeeOrCredit( - Fee.create(registry.getStandardRestoreCost().getAmount(), FeeType.RESTORE, false)) - .build(); + Fee.create(registry.getStandardRestoreCost().getAmount(), FeeType.RESTORE, false)); + if (isExpired) { + feesAndCredits.addFeeOrCredit( + Fee.create( + domainPrices.getRenewCost().getAmount(), FeeType.RENEW, domainPrices.isPremium())); + } return customLogic.customizeRestorePrice( RestorePriceParameters.newBuilder() - .setFeesAndCredits(feesAndCredits) + .setFeesAndCredits(feesAndCredits.build()) .setRegistry(registry) .setDomainName(InternetDomainName.from(domainName)) .setAsOfDate(dateTime) diff --git a/core/src/main/java/google/registry/flows/domain/DomainRestoreRequestFlow.java b/core/src/main/java/google/registry/flows/domain/DomainRestoreRequestFlow.java index e6b3a5050..24606095c 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainRestoreRequestFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainRestoreRequestFlow.java @@ -136,20 +136,22 @@ public final class DomainRestoreRequestFlow implements TransactionalFlow { Update command = (Update) resourceCommand; DateTime now = tm().getTransactionTime(); DomainBase existingDomain = loadAndVerifyExistence(DomainBase.class, targetId, now); + boolean isExpired = existingDomain.getRegistrationExpirationTime().isBefore(now); FeesAndCredits feesAndCredits = - pricingLogic.getRestorePrice(Registry.get(existingDomain.getTld()), targetId, now); + pricingLogic.getRestorePrice( + Registry.get(existingDomain.getTld()), targetId, now, isExpired); Optional feeUpdate = eppInput.getSingleExtension(FeeUpdateCommandExtension.class); verifyRestoreAllowed(command, existingDomain, feeUpdate, feesAndCredits, now); HistoryEntry historyEntry = buildHistoryEntry(existingDomain, now); ImmutableSet.Builder entitiesToSave = new ImmutableSet.Builder<>(); + DateTime newExpirationTime = + existingDomain.getRegistrationExpirationTime().plusYears(isExpired ? 1 : 0); // Restore the expiration time on the deleted domain, except if that's already passed, then add // a year and bill for it immediately, with no grace period. - DateTime newExpirationTime = existingDomain.getRegistrationExpirationTime(); - if (newExpirationTime.isBefore(now)) { + if (isExpired) { entitiesToSave.add(createRenewBillingEvent(historyEntry, feesAndCredits.getRenewCost(), now)); - newExpirationTime = newExpirationTime.plusYears(1); } // Always bill for the restore itself. entitiesToSave.add( @@ -176,7 +178,7 @@ public final class DomainRestoreRequestFlow implements TransactionalFlow { ofy().delete().key(existingDomain.getDeletePollMessage()); dnsQueue.addDomainRefreshTask(existingDomain.getDomainName()); return responseBuilder - .setExtensions(createResponseExtensions(feesAndCredits, feeUpdate)) + .setExtensions(createResponseExtensions(feesAndCredits, feeUpdate, isExpired)) .build(); } @@ -263,23 +265,29 @@ public final class DomainRestoreRequestFlow implements TransactionalFlow { } private static ImmutableList createResponseExtensions( - FeesAndCredits feesAndCredits, Optional feeUpdate) { + FeesAndCredits feesAndCredits, + Optional feeUpdate, + boolean isExpired) { + ImmutableList.Builder fees = new ImmutableList.Builder<>(); + fees.add( + Fee.create( + feesAndCredits.getRestoreCost().getAmount(), + FeeType.RESTORE, + feesAndCredits.hasPremiumFeesOfType(FeeType.RESTORE))); + if (isExpired) { + fees.add( + Fee.create( + feesAndCredits.getRenewCost().getAmount(), + FeeType.RENEW, + feesAndCredits.hasPremiumFeesOfType(FeeType.RENEW))); + } return feeUpdate.isPresent() ? ImmutableList.of( feeUpdate .get() .createResponseBuilder() .setCurrency(feesAndCredits.getCurrency()) - .setFees( - ImmutableList.of( - Fee.create( - feesAndCredits.getRestoreCost().getAmount(), - FeeType.RESTORE, - feesAndCredits.hasPremiumFeesOfType(FeeType.RESTORE)), - Fee.create( - feesAndCredits.getRenewCost().getAmount(), - FeeType.RENEW, - feesAndCredits.hasPremiumFeesOfType(FeeType.RENEW)))) + .setFees(fees.build()) .build()) : ImmutableList.of(); } diff --git a/core/src/test/java/google/registry/flows/domain/DomainInfoFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainInfoFlowTest.java index 0405f19d6..0c5cf0972 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainInfoFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainInfoFlowTest.java @@ -659,6 +659,39 @@ public class DomainInfoFlowTest extends ResourceFlowTestCaseUSD restore 1 - 100.00 17.00 premium diff --git a/core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response.xml b/core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response.xml index 11616fc62..4095bef0a 100644 --- a/core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response.xml +++ b/core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response.xml @@ -35,7 +35,6 @@ USD restore 1 - 11.00 17.00 diff --git a/core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response_no_renewal.xml b/core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response_no_renewal.xml new file mode 100644 index 000000000..816caf4cc --- /dev/null +++ b/core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response_no_renewal.xml @@ -0,0 +1,49 @@ + + + + Command completed successfully + + + + example.tld + %ROID% + + jd1234 + sh8013 + sh8013 + + ns1.example.tld + ns1.example.net + + ns1.example.tld + ns2.example.tld + NewRegistrar + TheRegistrar + 1999-04-03T22:00:00.0Z + NewRegistrar + 1999-12-03T09:00:00.0Z + 2005-04-03T22:00:00.0Z + 2000-04-08T09:00:00.0Z + + 2fooBAR + + + + + + + + + USD + restore + 1 + 17.00 + + + + ABC-12345 + server-trid + + + diff --git a/core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response_with_renewal.xml b/core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response_with_renewal.xml new file mode 100644 index 000000000..4b1fa4142 --- /dev/null +++ b/core/src/test/resources/google/registry/flows/domain/domain_info_fee_restore_response_with_renewal.xml @@ -0,0 +1,51 @@ + + + + Command completed successfully + + + + rich.example + %ROID% + + jd1234 + sh8013 + sh8013 + + ns1.example.tld + ns1.example.net + + ns1.example.tld + ns2.example.tld + NewRegistrar + TheRegistrar + 1999-04-03T22:00:00.0Z + NewRegistrar + 1999-12-03T09:00:00.0Z + 2005-03-02T22:00:00.000Z + 2000-04-08T09:00:00.0Z + + 2fooBAR + + + + + + + + + USD + restore + 1 + 17.00 + 100.00 + premium + + + + ABC-12345 + server-trid + + + diff --git a/core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_fee_no_renewal.xml b/core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_fee_no_renewal.xml new file mode 100644 index 000000000..78a05ae98 --- /dev/null +++ b/core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_fee_no_renewal.xml @@ -0,0 +1,21 @@ + + + + + example.tld + + + + + + + + + %CURRENCY% + 17.00 + + + ABC-12345 + + diff --git a/core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_premium_no_renewal.xml b/core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_premium_no_renewal.xml new file mode 100644 index 000000000..5bbcf6bc6 --- /dev/null +++ b/core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_premium_no_renewal.xml @@ -0,0 +1,23 @@ + + + + + rich.example + + + + + + + + + USD + + 17.00 + + + + ABC-12345 + + diff --git a/core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_response_fee_no_renewal.xml b/core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_response_fee_no_renewal.xml new file mode 100644 index 000000000..05484e809 --- /dev/null +++ b/core/src/test/resources/google/registry/flows/domain/domain_update_restore_request_response_fee_no_renewal.xml @@ -0,0 +1,17 @@ + + + + Command completed successfully + + + <%FEE_NS%:updData xmlns:%FEE_NS%="urn:ietf:params:xml:ns:fee-%FEE_VERSION%"> + <%FEE_NS%:currency>USD + <%FEE_NS%:fee description="restore">17.00 + + + + ABC-12345 + server-trid + + +