Skip to content

Commit

Permalink
Refactor some registry lock verification code (#2434)
Browse files Browse the repository at this point in the history 
The user, on the front end, should not be required to provide whether or
not they're trying to verify a lock or an unlock. They should only need
the verification code. We can inspect the lock object itself (and the
domain in question) to see whether or not we're verifying a lock or an
unlock.
  • Loading branch information
@gbrodman
gbrodman committed May 14, 2024
1 parent 6ca3cc2 commit d09bb4f
Show file tree
Hide file tree
Showing 9 changed files with 134 additions and 149 deletions.
115 changes: 54 additions & 61 deletions core/src/main/java/google/registry/tools/DomainLockUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public DomainLockUtils(
/**
* Creates and persists a lock request when requested by a user.
*
* <p>The lock will not be applied until {@link #verifyAndApplyLock} is called.
* <p>The lock will not be applied until {@link #verifyVerificationCode} is called.
*/
public RegistryLock saveNewRegistryLockRequest(
String domainName, String registrarId, @Nullable String registrarPocId, boolean isAdmin) {
Expand All @@ -84,7 +84,7 @@ public RegistryLock saveNewRegistryLockRequest(
/**
* Creates and persists an unlock request when requested by a user.
*
* <p>The unlock will not be applied until {@link #verifyAndApplyUnlock} is called.
* <p>The unlock will not be applied until {@link #verifyVerificationCode} is called.
*/
public RegistryLock saveNewRegistryUnlockRequest(
String domainName, String registrarId, boolean isAdmin, Optional<Duration> relockDuration) {
Expand All @@ -94,62 +94,32 @@ public RegistryLock saveNewRegistryUnlockRequest(
createUnlockBuilder(domainName, registrarId, isAdmin, relockDuration).build()));
}

/** Verifies and applies the lock request previously requested by a user. */
public RegistryLock verifyAndApplyLock(String verificationCode, boolean isAdmin) {
return tm().transact(
() -> {
DateTime now = tm().getTransactionTime();
RegistryLock lock = getByVerificationCode(verificationCode);

checkArgument(
lock.getLockCompletionTime().isEmpty(),
"Domain %s is already locked",
lock.getDomainName());

checkArgument(
!lock.isLockRequestExpired(now),
"The pending lock has expired; please try again");

checkArgument(
!lock.isSuperuser() || isAdmin, "Non-admin user cannot complete admin lock");

RegistryLock newLock =
RegistryLockDao.save(lock.asBuilder().setLockCompletionTime(now).build());
setAsRelock(newLock);
tm().transact(() -> applyLockStatuses(newLock, now, isAdmin));
return newLock;
});
}

/** Verifies and applies the unlock request previously requested by a user. */
public RegistryLock verifyAndApplyUnlock(String verificationCode, boolean isAdmin) {
RegistryLock lock =
/**
* Verifies and applies the lock/unlock request previously requested given a verification code.
*
* <p>This assumes that the lock object / domain in question has a pending lock or unlock.
*/
public RegistryLock verifyVerificationCode(String verificationCode, boolean isAdmin) {
RegistryLock result =
tm().transact(
() -> {
DateTime now = tm().getTransactionTime();
RegistryLock previousLock = getByVerificationCode(verificationCode);
checkArgument(
previousLock.getUnlockCompletionTime().isEmpty(),
"Domain %s is already unlocked",
previousLock.getDomainName());

checkArgument(
!previousLock.isUnlockRequestExpired(now),
"The pending unlock has expired; please try again");

checkArgument(
isAdmin || !previousLock.isSuperuser(),
"Non-admin user cannot complete admin unlock");

RegistryLock newLock =
RegistryLockDao.save(
previousLock.asBuilder().setUnlockCompletionTime(now).build());
tm().transact(() -> removeLockStatuses(newLock, isAdmin, now));
return newLock;
RegistryLock lock = getByVerificationCode(verificationCode);
if (lock.getLockCompletionTime().isEmpty()) {
return verifyAndApplyLock(lock, isAdmin);
} else if (lock.getUnlockRequestTime().isPresent()
&& lock.getUnlockCompletionTime().isEmpty()) {
return verifyAndApplyUnlock(lock, isAdmin);
} else {
throw new IllegalArgumentException(
String.format(
"Lock/unlock with code %s is already completed", verificationCode));
}
});
// Submit relock outside the transaction to make sure that it fully succeeded
submitRelockIfNecessary(lock);
return lock;
if (result.getUnlockCompletionTime().isPresent()) {
// Submit relock outside the transaction to make sure that it fully succeeded
submitRelockIfNecessary(result);
}
return result;
}

/**
Expand Down Expand Up @@ -232,13 +202,36 @@ public void enqueueDomainRelock(Duration countdown, long lockRevisionId, int pre
countdown));
}

private RegistryLock verifyAndApplyLock(RegistryLock lock, boolean isAdmin) {
DateTime now = tm().getTransactionTime();
checkArgument(
!lock.isLockRequestExpired(now), "The pending lock has expired; please try again");

checkArgument(!lock.isSuperuser() || isAdmin, "Non-admin user cannot complete admin lock");

RegistryLock newLock =
RegistryLockDao.save(lock.asBuilder().setLockCompletionTime(now).build());
setAsRelock(newLock);
applyLockStatuses(newLock, now, isAdmin);
return newLock;
}

private RegistryLock verifyAndApplyUnlock(RegistryLock lock, boolean isAdmin) {
DateTime now = tm().getTransactionTime();
checkArgument(
!lock.isUnlockRequestExpired(now), "The pending unlock has expired; please try again");

checkArgument(isAdmin || !lock.isSuperuser(), "Non-admin user cannot complete admin unlock");

RegistryLock newLock =
RegistryLockDao.save(lock.asBuilder().setUnlockCompletionTime(now).build());
removeLockStatuses(newLock, isAdmin, now);
return newLock;
}

private void setAsRelock(RegistryLock newLock) {
tm().transact(
() ->
RegistryLockDao.getMostRecentVerifiedUnlockByRepoId(newLock.getRepoId())
.ifPresent(
oldLock ->
RegistryLockDao.save(oldLock.asBuilder().setRelock(newLock).build())));
RegistryLockDao.getMostRecentVerifiedUnlockByRepoId(newLock.getRepoId())
.ifPresent(oldLock -> RegistryLockDao.save(oldLock.asBuilder().setRelock(newLock).build()));
}

private RegistryLock.Builder createLockBuilder(
Expand Down
1 change: 0 additions & 1 deletion core/src/main/java/google/registry/ui/server/console/ConsoleRegistryLockAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,6 @@ private void sendVerificationEmail(RegistryLock lock, String userEmail, boolean
// TODO: replace this with the PATH in ConsoleRegistryLockVerifyAction once it exists
.setPath("/console-api/registry-lock-verify")
.setParameter("lockVerificationCode", lock.getVerificationCode())
.setParameter("isLock", String.valueOf(isLock))
.build()
.toString();
String body = String.format(VERIFICATION_EMAIL_TEMPLATE, lock.getDomainName(), url);
Expand Down
1 change: 0 additions & 1 deletion core/src/main/java/google/registry/ui/server/registrar/RegistryLockPostAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ private void sendVerificationEmail(RegistryLock lock, String userEmail, boolean
.setHost(req.getServerName())
.setPath("registry-lock-verify")
.setParameter("lockVerificationCode", lock.getVerificationCode())
.setParameter("isLock", String.valueOf(isLock))
.build()
.toString();
String body = String.format(VERIFICATION_EMAIL_TEMPLATE, lock.getDomainName(), url);
Expand Down
15 changes: 4 additions & 11 deletions core/src/main/java/google/registry/ui/server/registrar/RegistryLockVerifyAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,29 +50,22 @@ public final class RegistryLockVerifyAction extends HtmlAction {

private final DomainLockUtils domainLockUtils;
private final String lockVerificationCode;
private final Boolean isLock;

@Inject
public RegistryLockVerifyAction(
DomainLockUtils domainLockUtils,
@Parameter("lockVerificationCode") String lockVerificationCode,
@Parameter("isLock") Boolean isLock) {
@Parameter("lockVerificationCode") String lockVerificationCode) {
this.domainLockUtils = domainLockUtils;
this.lockVerificationCode = lockVerificationCode;
this.isLock = isLock;
}

@Override
public void runAfterLogin(Map<String, Object> data) {
try {
boolean isAdmin = authResult.userAuthInfo().get().isUserAdmin();
final RegistryLock resultLock;
if (isLock) {
resultLock = domainLockUtils.verifyAndApplyLock(lockVerificationCode, isAdmin);
} else {
resultLock = domainLockUtils.verifyAndApplyUnlock(lockVerificationCode, isAdmin);
}
data.put("isLock", isLock);
RegistryLock resultLock =
domainLockUtils.verifyVerificationCode(lockVerificationCode, isAdmin);
data.put("isLock", resultLock.getUnlockCompletionTime().isEmpty());
data.put("success", true);
data.put("domainName", resultLock.getDomainName());
} catch (Throwable t) {
Expand Down
50 changes: 27 additions & 23 deletions core/src/test/java/google/registry/tools/DomainLockUtilsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ void testSuccess_createLock_previousLockExpired() {
clock.advanceBy(standardDays(1));
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
verifyProperlyLockedDomain(false);
}

Expand All @@ -137,15 +137,15 @@ void testSuccess_createUnlock_previousUnlockRequestExpired() {
RegistryLock unlockRequest =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
domainLockUtils.verifyAndApplyUnlock(unlockRequest.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(unlockRequest.getVerificationCode(), false);
assertThat(loadByEntity(domain).getStatusValues()).containsNoneIn(REGISTRY_LOCK_STATUSES);
}

@Test
void testSuccess_applyLockDomain() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
verifyProperlyLockedDomain(false);
}

Expand All @@ -155,27 +155,27 @@ void testSuccess_applyUnlockDomain() {
RegistryLock unlock =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
domainLockUtils.verifyAndApplyUnlock(unlock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), false);
verifyProperlyUnlockedDomain(false);
}

@Test
void testSuccess_applyAdminLock_onlyHistoryEntry() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", null, true);
domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true);
verifyProperlyLockedDomain(true);
}

@Test
void testSuccess_applyAdminUnlock_onlyHistoryEntry() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", null, true);
domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true);
RegistryLock unlock =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", true, Optional.empty());
domainLockUtils.verifyAndApplyUnlock(unlock.getVerificationCode(), true);
domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), true);
verifyProperlyUnlockedDomain(true);
}

Expand All @@ -196,7 +196,7 @@ void testSuccess_administrativelyLock_admin() {
void testSuccess_administrativelyUnlock_nonAdmin() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
domainLockUtils.administrativelyApplyUnlock(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
verifyProperlyUnlockedDomain(false);
Expand All @@ -206,7 +206,7 @@ void testSuccess_administrativelyUnlock_nonAdmin() {
void testSuccess_administrativelyUnlock_admin() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", null, true);
domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true);
domainLockUtils.administrativelyApplyUnlock(
DOMAIN_NAME, "TheRegistrar", true, Optional.empty());
verifyProperlyUnlockedDomain(true);
Expand All @@ -220,7 +220,7 @@ void testSuccess_regularLock_relockSet() {
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
RegistryLock newLock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
newLock = domainLockUtils.verifyAndApplyLock(newLock.getVerificationCode(), false);
newLock = domainLockUtils.verifyVerificationCode(newLock.getVerificationCode(), false);
assertThat(
getRegistryLockByRevisionId(oldLock.getRevisionId()).get().getRelock().getRevisionId())
.isEqualTo(newLock.getRevisionId());
Expand Down Expand Up @@ -254,7 +254,7 @@ void testSuccess_unlock_relockSubmitted() {
RegistryLock lock =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.of(standardHours(6)));
domainLockUtils.verifyAndApplyUnlock(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
cloudTasksHelper.assertTasksEnqueued(
QUEUE_ASYNC_ACTIONS,
new TaskMatcher()
Expand Down Expand Up @@ -304,7 +304,7 @@ void testSuccess_adminCanLockUnlockedDomain_withSavedLock() {
void testFailure_createUnlock_alreadyPendingUnlock() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());

Expand All @@ -323,7 +323,7 @@ void testFailure_createUnlock_alreadyPendingUnlock() {
void testFailure_createUnlock_nonAdminUnlockingAdmin() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", null, true);
domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true);
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
Expand Down Expand Up @@ -387,13 +387,15 @@ void testFailure_createUnlock_alreadyUnlocked() {
void testFailure_applyLock_alreadyApplied() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
domain = loadByEntity(domain);
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), false));
assertThat(thrown).hasMessageThat().isEqualTo("Domain example.tld is already locked");
() -> domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false));
assertThat(thrown)
.hasMessageThat()
.isEqualTo("Lock/unlock with code 123456789ABCDEFGHJKLMNPQRSTUVWXY is already completed");
assertNoDomainChanges();
}

Expand All @@ -405,7 +407,7 @@ void testFailure_applyLock_expired() {
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), true));
() -> domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true));
assertThat(thrown).hasMessageThat().isEqualTo("The pending lock has expired; please try again");
assertNoDomainChanges();
}
Expand All @@ -417,7 +419,7 @@ void testFailure_applyLock_nonAdmin_applyAdminLock() {
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), false));
() -> domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false));
assertThat(thrown).hasMessageThat().isEqualTo("Non-admin user cannot complete admin lock");
assertNoDomainChanges();
}
Expand All @@ -426,17 +428,19 @@ void testFailure_applyLock_nonAdmin_applyAdminLock() {
void testFailure_applyUnlock_alreadyUnlocked() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
RegistryLock unlock =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
domainLockUtils.verifyAndApplyUnlock(unlock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), false);

IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> domainLockUtils.verifyAndApplyUnlock(unlock.getVerificationCode(), false));
assertThat(thrown).hasMessageThat().isEqualTo("Domain example.tld is already unlocked");
() -> domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), false));
assertThat(thrown)
.hasMessageThat()
.isEqualTo("Lock/unlock with code Zabcdefghijkmnopqrstuvwxyz123456 is already completed");
assertNoDomainChanges();
}

Expand All @@ -451,7 +455,7 @@ void testFailure_applyLock_alreadyLocked() {
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> domainLockUtils.verifyAndApplyLock(verificationCode, false));
() -> domainLockUtils.verifyVerificationCode(verificationCode, false));
assertThat(thrown).hasMessageThat().isEqualTo("Domain example.tld is already locked");

// Failure during the lock acquisition portion shouldn't affect the SQL object
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/google/registry/tools/UnlockDomainCommandTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ private Domain persistLockedDomain(String domainName, String registrarId) {
Domain domain = persistResource(DatabaseHelper.newDomain(domainName));
RegistryLock lock =
command.domainLockUtils.saveNewRegistryLockRequest(domainName, registrarId, null, true);
command.domainLockUtils.verifyAndApplyLock(lock.getVerificationCode(), true);
command.domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true);
return reloadResource(domain);
}

Expand Down

0 comments on commit d09bb4f

Please sign in to comment.