Make not logged in errors take precedence over extension errors (#1483)

* Make not logged in errors take precedence over extension errors

This is the right order to do the checks in, because if the registrar isn't
logged in (or their login failed) then they will have an empty set of declared
extensions, so any attempt to use an extension will throw a "Service
extension(s) must be declared at login" error. This is potentially misleading
because the actual error in this situation is that the registrar isn't logged
in at all.

This also fixes some flows that weren't declared final (but should be), or
methods declared final on final classes, which is superfluous.
This commit is contained in:
Ben McIlwain 2021-12-30 17:23:14 -05:00 committed by GitHub
parent 78494c4ac7
commit a246eeea82
34 changed files with 80 additions and 69 deletions

View file

@ -56,9 +56,9 @@ public final class ContactCheckFlow implements Flow {
@Inject ContactCheckFlow() {}
@Override
public final EppResponse run() throws EppException {
extensionManager.validate(); // There are no legal extensions for this flow.
public EppResponse run() throws EppException {
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate(); // There are no legal extensions for this flow.
ImmutableList<String> targetIds = ((Check) resourceCommand).getTargetIds();
verifyTargetIdCount(targetIds, maxChecks);
ImmutableSet<String> existingIds =

View file

@ -70,10 +70,10 @@ public final class ContactCreateFlow implements TransactionalFlow {
@Inject ContactCreateFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
Create command = (Create) resourceCommand;
DateTime now = tm().getTransactionTime();
verifyResourceDoesNotExist(ContactResource.class, targetId, now, registrarId);

View file

@ -90,10 +90,10 @@ public final class ContactDeleteFlow implements TransactionalFlow {
ContactDeleteFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
checkLinkedDomains(targetId, now, ContactResource.class, DomainBase::getReferencedContacts);
ContactResource existingContact = loadAndVerifyExistence(ContactResource.class, targetId, now);

View file

@ -65,10 +65,10 @@ public final class ContactInfoFlow implements Flow {
ContactInfoFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
DateTime now = clock.nowUtc();
extensionManager.validate(); // There are no legal extensions for this flow.
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate(); // There are no legal extensions for this flow.
ContactResource contact = loadAndVerifyExistence(ContactResource.class, targetId, now);
if (!isSuperuser) {
verifyResourceOwnership(registrarId, contact);

View file

@ -74,14 +74,14 @@ public final class ContactTransferApproveFlow implements TransactionalFlow {
@Inject ContactTransferApproveFlow() {}
/**
* <p>The logic in this flow, which handles client approvals, very closely parallels the logic in
* The logic in this flow, which handles client approvals, very closely parallels the logic in
* {@link ContactResource#cloneProjectedAtTime} which handles implicit server approvals.
*/
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
ContactResource existingContact = loadAndVerifyExistence(ContactResource.class, targetId, now);
verifyOptionalAuthInfo(authInfo, existingContact);

View file

@ -76,8 +76,8 @@ public final class ContactTransferCancelFlow implements TransactionalFlow {
@Override
public final EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
ContactResource existingContact = loadAndVerifyExistence(ContactResource.class, targetId, now);
verifyOptionalAuthInfo(authInfo, existingContact);

View file

@ -63,9 +63,9 @@ public final class ContactTransferQueryFlow implements Flow {
@Inject ContactTransferQueryFlow() {}
@Override
public final EppResponse run() throws EppException {
extensionManager.validate(); // There are no legal extensions for this flow.
public EppResponse run() throws EppException {
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate(); // There are no legal extensions for this flow.
ContactResource contact =
loadAndVerifyExistence(ContactResource.class, targetId, clock.nowUtc());
verifyOptionalAuthInfo(authInfo, contact);

View file

@ -72,10 +72,10 @@ public final class ContactTransferRejectFlow implements TransactionalFlow {
@Inject ContactTransferRejectFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
ContactResource existingContact = loadAndVerifyExistence(ContactResource.class, targetId, now);
verifyOptionalAuthInfo(authInfo, existingContact);

View file

@ -92,10 +92,10 @@ public final class ContactTransferRequestFlow implements TransactionalFlow {
@Inject ContactTransferRequestFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(gainingClientId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
ContactResource existingContact = loadAndVerifyExistence(ContactResource.class, targetId, now);
verifyAuthInfoPresentForResourceTransfer(authInfo);

View file

@ -89,10 +89,10 @@ public final class ContactUpdateFlow implements TransactionalFlow {
@Inject ContactUpdateFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
Update command = (Update) resourceCommand;
DateTime now = tm().getTransactionTime();
ContactResource existingContact = loadAndVerifyExistence(ContactResource.class, targetId, now);

View file

@ -135,8 +135,8 @@ public final class DomainCheckFlow implements Flow {
extensionManager.register(
FeeCheckCommandExtension.class, LaunchCheckExtension.class, AllocationTokenExtension.class);
flowCustomLogic.beforeValidation();
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
ImmutableList<String> domainNames = ((Check) resourceCommand).getTargetIds();
verifyTargetIdCount(domainNames, maxChecks);
DateTime now = clock.nowUtc();

View file

@ -82,8 +82,8 @@ public final class DomainClaimsCheckFlow implements Flow {
@Override
public EppResponse run() throws EppException {
extensionManager.register(LaunchCheckExtension.class, AllocationTokenExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
if (eppInput.getSingleExtension(AllocationTokenExtension.class).isPresent()) {
throw new DomainClaimsCheckNotAllowedWithAllocationTokens();
}

View file

@ -198,7 +198,7 @@ import org.joda.time.Duration;
* @error {@link DomainPricingLogic.AllocationTokenInvalidForPremiumNameException}
*/
@ReportingSpec(ActivityReportField.DOMAIN_CREATE)
public class DomainCreateFlow implements TransactionalFlow {
public final class DomainCreateFlow implements TransactionalFlow {
/** Anchor tenant creates should always be for 2 years, since they get 2 years free. */
private static final int ANCHOR_TENANT_CREATE_VALID_YEARS = 2;
@ -219,7 +219,7 @@ public class DomainCreateFlow implements TransactionalFlow {
@Inject DomainCreateFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(
FeeCreateCommandExtension.class,
SecDnsCreateExtension.class,
@ -227,9 +227,9 @@ public class DomainCreateFlow implements TransactionalFlow {
LaunchCreateExtension.class,
AllocationTokenExtension.class);
flowCustomLogic.beforeValidation();
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
verifyRegistrarIsActive(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
DomainCommand.Create command = cloneAndLinkReferences((Create) resourceCommand, now);
Period period = command.getPeriod();

View file

@ -138,12 +138,12 @@ public final class DomainDeleteFlow implements TransactionalFlow {
@Inject DomainDeleteFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(
MetadataExtension.class, SecDnsCreateExtension.class, DomainDeleteSuperuserExtension.class);
flowCustomLogic.beforeValidation();
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
// Loads the target resource if it exists
DomainBase existingDomain = loadAndVerifyExistence(DomainBase.class, targetId, now);

View file

@ -91,11 +91,11 @@ public final class DomainInfoFlow implements Flow {
DomainInfoFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(FeeInfoCommandExtensionV06.class);
flowCustomLogic.beforeValidation();
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = clock.nowUtc();
DomainBase domain = verifyExistence(
DomainBase.class, targetId, loadByForeignKey(DomainBase.class, targetId, now));

View file

@ -135,12 +135,12 @@ public final class DomainRenewFlow implements TransactionalFlow {
@Inject DomainRenewFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(FeeRenewCommandExtension.class, MetadataExtension.class);
flowCustomLogic.beforeValidation();
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
verifyRegistrarIsActive(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
Renew command = (Renew) resourceCommand;
// Loads the target resource if it exists

View file

@ -128,14 +128,14 @@ public final class DomainRestoreRequestFlow implements TransactionalFlow {
@Inject DomainRestoreRequestFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(
FeeUpdateCommandExtension.class,
MetadataExtension.class,
RgpUpdateExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
verifyRegistrarIsActive(registrarId);
extensionManager.validate();
Update command = (Update) resourceCommand;
DateTime now = tm().getTransactionTime();
DomainBase existingDomain = loadAndVerifyExistence(DomainBase.class, targetId, now);

View file

@ -99,14 +99,14 @@ public final class DomainTransferApproveFlow implements TransactionalFlow {
@Inject DomainTransferApproveFlow() {}
/**
* <p>The logic in this flow, which handles client approvals, very closely parallels the logic in
* The logic in this flow, which handles client approvals, very closely parallels the logic in
* {@link DomainBase#cloneProjectedAtTime} which handles implicit server approvals.
*/
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
DomainBase existingDomain = loadAndVerifyExistence(DomainBase.class, targetId, now);
verifyOptionalAuthInfo(authInfo, existingDomain);

View file

@ -86,10 +86,10 @@ public final class DomainTransferCancelFlow implements TransactionalFlow {
@Inject DomainTransferCancelFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
DomainBase existingDomain = loadAndVerifyExistence(DomainBase.class, targetId, now);
verifyOptionalAuthInfo(authInfo, existingDomain);

View file

@ -67,9 +67,9 @@ public final class DomainTransferQueryFlow implements Flow {
@Inject DomainTransferQueryFlow() {}
@Override
public final EppResponse run() throws EppException {
extensionManager.validate(); // There are no legal extensions for this flow.
public EppResponse run() throws EppException {
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate(); // There are no legal extensions for this flow.
DateTime now = clock.nowUtc();
DomainBase domain = loadAndVerifyExistence(DomainBase.class, targetId, now);
verifyOptionalAuthInfo(authInfo, domain);

View file

@ -88,10 +88,10 @@ public final class DomainTransferRejectFlow implements TransactionalFlow {
@Inject DomainTransferRejectFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
DomainBase existingDomain = loadAndVerifyExistence(DomainBase.class, targetId, now);
Registry registry = Registry.get(existingDomain.getTld());

View file

@ -139,14 +139,14 @@ public final class DomainTransferRequestFlow implements TransactionalFlow {
@Inject DomainTransferRequestFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(
DomainTransferRequestSuperuserExtension.class,
FeeTransferCommandExtension.class,
MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(gainingClientId);
verifyRegistrarIsActive(gainingClientId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
DomainBase existingDomain = loadAndVerifyExistence(DomainBase.class, targetId, now);
Optional<DomainTransferRequestSuperuserExtension> superuserExtension =

View file

@ -168,8 +168,8 @@ public final class DomainUpdateFlow implements TransactionalFlow {
SecDnsUpdateExtension.class,
DomainUpdateSuperuserExtension.class);
flowCustomLogic.beforeValidation();
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
Update command = cloneAndLinkReferences((Update) resourceCommand, now);
DomainBase existingDomain = loadAndVerifyExistence(DomainBase.class, targetId, now);

View file

@ -56,9 +56,9 @@ public final class HostCheckFlow implements Flow {
@Inject HostCheckFlow() {}
@Override
public final EppResponse run() throws EppException {
extensionManager.validate(); // There are no legal extensions for this flow.
public EppResponse run() throws EppException {
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate(); // There are no legal extensions for this flow.
ImmutableList<String> hostnames = ((Check) resourceCommand).getTargetIds();
verifyTargetIdCount(hostnames, maxChecks);
ImmutableSet<String> existingIds =

View file

@ -100,10 +100,10 @@ public final class HostCreateFlow implements TransactionalFlow {
HostCreateFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
Create command = (Create) resourceCommand;
DateTime now = tm().getTransactionTime();
verifyResourceDoesNotExist(HostResource.class, targetId, now, registrarId);

View file

@ -92,10 +92,10 @@ public final class HostDeleteFlow implements TransactionalFlow {
HostDeleteFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
DateTime now = tm().getTransactionTime();
validateHostName(targetId);
checkLinkedDomains(targetId, now, HostResource.class, DomainBase::getNameservers);

View file

@ -62,8 +62,8 @@ public final class HostInfoFlow implements Flow {
@Override
public EppResponse run() throws EppException {
extensionManager.validate(); // There are no legal extensions for this flow.
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate(); // There are no legal extensions for this flow.
validateHostName(targetId);
DateTime now = clock.nowUtc();
HostResource host = loadAndVerifyExistence(HostResource.class, targetId, now);

View file

@ -125,10 +125,10 @@ public final class HostUpdateFlow implements TransactionalFlow {
@Inject HostUpdateFlow() {}
@Override
public final EppResponse run() throws EppException {
public EppResponse run() throws EppException {
extensionManager.register(MetadataExtension.class);
extensionManager.validate();
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate();
Update command = (Update) resourceCommand;
Change change = command.getInnerChange();
String suppliedNewHostName = change.getFullyQualifiedHostName();

View file

@ -45,17 +45,17 @@ import org.joda.time.DateTime;
/**
* An EPP flow for acknowledging {@link PollMessage}s.
*
* <p>Registrars refer to poll messages using an externally visible id generated by
* {@link PollMessageExternalKeyConverter}. One-time poll messages are deleted from Datastore once
* they are ACKed, whereas autorenew poll messages are simply marked as read, and won't be delivered
* again until the next year of their recurrence.
* <p>Registrars refer to poll messages using an externally visible id generated by {@link
* PollMessageExternalKeyConverter}. One-time poll messages are deleted from Datastore once they are
* ACKed, whereas autorenew poll messages are simply marked as read, and won't be delivered again
* until the next year of their recurrence.
*
* @error {@link PollAckFlow.InvalidMessageIdException}
* @error {@link PollAckFlow.MessageDoesNotExistException}
* @error {@link PollAckFlow.MissingMessageIdException}
* @error {@link PollAckFlow.NotAuthorizedToAckMessageException}
*/
public class PollAckFlow implements TransactionalFlow {
public final class PollAckFlow implements TransactionalFlow {
@Inject ExtensionManager extensionManager;
@Inject @RegistrarId String registrarId;
@ -65,9 +65,9 @@ public class PollAckFlow implements TransactionalFlow {
@Inject PollAckFlow() {}
@Override
public final EppResponse run() throws EppException {
extensionManager.validate(); // There are no legal extensions for this flow.
public EppResponse run() throws EppException {
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate(); // There are no legal extensions for this flow.
if (messageId.isEmpty()) {
throw new MissingMessageIdException();
}

View file

@ -47,7 +47,7 @@ import org.joda.time.DateTime;
*
* @error {@link PollRequestFlow.UnexpectedMessageIdException}
*/
public class PollRequestFlow implements Flow {
public final class PollRequestFlow implements Flow {
@Inject ExtensionManager extensionManager;
@Inject @RegistrarId String registrarId;
@ -57,9 +57,9 @@ public class PollRequestFlow implements Flow {
@Inject PollRequestFlow() {}
@Override
public final EppResponse run() throws EppException {
extensionManager.validate(); // There are no legal extensions for this flow.
public EppResponse run() throws EppException {
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate(); // There are no legal extensions for this flow.
if (!messageId.isEmpty()) {
throw new UnexpectedMessageIdException();
}

View file

@ -27,7 +27,7 @@ import javax.inject.Inject;
*
* @error {@link google.registry.flows.FlowUtils.GenericXmlSyntaxErrorException}
*/
public class HelloFlow implements Flow {
public final class HelloFlow implements Flow {
@Inject ExtensionManager extensionManager;
@Inject Clock clock;

View file

@ -90,7 +90,7 @@ public class LoginFlow implements Flow {
}
/** Run the flow without bothering to log errors. The {@link #run} method will do that for us. */
private final EppResponse runWithoutLogging() throws EppException {
private EppResponse runWithoutLogging() throws EppException {
extensionManager.validate(); // There are no legal extensions for this flow.
Login login = (Login) eppInput.getCommandWrapper().getCommand();
if (!registrarId.isEmpty()) {

View file

@ -30,7 +30,7 @@ import javax.inject.Inject;
*
* @error {@link google.registry.flows.FlowUtils.NotLoggedInException}
*/
public class LogoutFlow implements Flow {
public final class LogoutFlow implements Flow {
@Inject ExtensionManager extensionManager;
@Inject @RegistrarId String registrarId;
@ -39,9 +39,9 @@ public class LogoutFlow implements Flow {
@Inject LogoutFlow() {}
@Override
public final EppResponse run() throws EppException {
extensionManager.validate(); // There are no legal extensions for this flow.
public EppResponse run() throws EppException {
validateRegistrarIsLoggedIn(registrarId);
extensionManager.validate(); // There are no legal extensions for this flow.
sessionMetadata.invalidate();
return responseBuilder.setResultFromCode(SUCCESS_AND_CLOSE).build();
}

View file

@ -130,6 +130,17 @@ class DomainCheckFlowTest extends ResourceCheckFlowTestCase<DomainCheckFlow, Dom
assertAboutEppExceptions().that(thrown).marshalsToXml();
}
@TestOfyAndSql
void testNotLoggedIn_takesPrecedenceOverUndeclaredExtensions() {
// Attempt to use the fee extension, but there is no login session and no supported extensions.
setEppInput("domain_check_fee_v06.xml", ImmutableMap.of("CURRENCY", "USD"));
sessionMetadata.setRegistrarId(null);
sessionMetadata.setServiceExtensionUris(ImmutableSet.of());
// NotLoggedIn should be thrown, not UndeclaredServiceExtensionException.
EppException thrown = assertThrows(NotLoggedInException.class, this::runFlow);
assertAboutEppExceptions().that(thrown).marshalsToXml();
}
@TestOfyAndSql
void testSuccess_nothingExists() throws Exception {
doCheckTest(