1
0
mirror of https://github.com/google/nomulus synced 2026-07-18 05:52:30 +00:00

Update security checks around registry lock verification (#3137)

1. make it a POST instead of a GET
2. pass the user into DomainLockUtils so we can check permissions to
   make sure they have permissions on the registrar
3. update all the tests
This commit is contained in:
gbrodman
2026-07-16 13:12:34 -04:00
committed by GitHub
parent 192a7e6c4e
commit 21421726e0
7 changed files with 159 additions and 51 deletions
@@ -288,8 +288,9 @@ export class BackendService {
verifyRegistryLockRequest(
lockVerificationCode: string
): Observable<RegistryLockVerificationResponse> {
return this.http.get<RegistryLockVerificationResponse>(
`/console-api/registry-lock-verify?lockVerificationCode=${lockVerificationCode}`
return this.http.post<RegistryLockVerificationResponse>(
`/console-api/registry-lock-verify?lockVerificationCode=${lockVerificationCode}`,
{}
);
}
@@ -28,6 +28,9 @@ import google.registry.config.RegistryConfig.Config;
import google.registry.model.ForeignKeyUtils;
import google.registry.model.billing.BillingBase.Reason;
import google.registry.model.billing.BillingEvent;
import google.registry.model.console.ConsolePermission;
import google.registry.model.console.User;
import google.registry.model.console.UserRoles;
import google.registry.model.domain.Domain;
import google.registry.model.domain.DomainHistory;
import google.registry.model.domain.RegistryLock;
@@ -100,16 +103,16 @@ public final class DomainLockUtils {
*
* <p>This assumes that the lock object / domain in question has a pending lock or unlock.
*/
public RegistryLock verifyVerificationCode(String verificationCode, boolean isAdmin) {
public RegistryLock verifyVerificationCode(String verificationCode, User user) {
RegistryLock result =
tm().transact(
() -> {
RegistryLock lock = getByVerificationCode(verificationCode);
if (lock.getLockCompletionTime().isEmpty()) {
return verifyAndApplyLock(lock, isAdmin);
return verifyAndApplyLock(lock, user);
} else if (lock.getUnlockRequestTime().isPresent()
&& lock.getUnlockCompletionTime().isEmpty()) {
return verifyAndApplyUnlock(lock, isAdmin);
return verifyAndApplyUnlock(lock, user);
} else {
throw new IllegalArgumentException(
String.format(
@@ -203,11 +206,18 @@ public final class DomainLockUtils {
countdown));
}
private RegistryLock verifyAndApplyLock(RegistryLock lock, boolean isAdmin) {
private RegistryLock verifyAndApplyLock(RegistryLock lock, User user) {
Instant now = tm().getTxTime();
checkArgument(
!lock.isLockRequestExpired(now), "The pending lock has expired; please try again");
UserRoles userRoles = user.getUserRoles();
boolean isAdmin = userRoles.isAdmin();
checkArgument(
userRoles.hasPermission(lock.getRegistrarId(), ConsolePermission.REGISTRY_LOCK),
"User %s does not have registry lock permission on registrar %s",
user.getEmailAddress(),
lock.getRegistrarId());
checkArgument(!lock.isSuperuser() || isAdmin, "Non-admin user cannot complete admin lock");
RegistryLock newLock =
@@ -217,12 +227,19 @@ public final class DomainLockUtils {
return newLock;
}
private RegistryLock verifyAndApplyUnlock(RegistryLock lock, boolean isAdmin) {
private RegistryLock verifyAndApplyUnlock(RegistryLock lock, User user) {
Instant now = tm().getTxTime();
checkArgument(
!lock.isUnlockRequestExpired(now), "The pending unlock has expired; please try again");
checkArgument(isAdmin || !lock.isSuperuser(), "Non-admin user cannot complete admin unlock");
UserRoles userRoles = user.getUserRoles();
boolean isAdmin = userRoles.isAdmin();
checkArgument(
userRoles.hasPermission(lock.getRegistrarId(), ConsolePermission.REGISTRY_LOCK),
"User %s does not have registry lock permission on registrar %s",
user.getEmailAddress(),
lock.getRegistrarId());
checkArgument(!lock.isSuperuser() || isAdmin, "Non-admin user cannot complete admin unlock");
RegistryLock newLock =
RegistryLockDao.save(lock.asBuilder().setUnlockCompletionTime(now).build());
@@ -15,7 +15,7 @@
package google.registry.ui.server.console;
import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
import static google.registry.request.Action.Method.GET;
import static google.registry.request.Action.Method.POST;
import com.google.common.base.Ascii;
import com.google.gson.annotations.Expose;
@@ -34,7 +34,7 @@ import jakarta.servlet.http.HttpServletResponse;
@Action(
service = Service.CONSOLE,
path = ConsoleRegistryLockVerifyAction.PATH,
method = {GET},
method = {POST},
auth = Auth.AUTH_PUBLIC_LOGGED_IN)
public class ConsoleRegistryLockVerifyAction extends ConsoleApiAction {
@@ -54,9 +54,8 @@ public class ConsoleRegistryLockVerifyAction extends ConsoleApiAction {
}
@Override
protected void getHandler(User user) {
RegistryLock lock =
domainLockUtils.verifyVerificationCode(lockVerificationCode, user.getUserRoles().isAdmin());
protected void postHandler(User user) {
RegistryLock lock = domainLockUtils.verifyVerificationCode(lockVerificationCode, user);
RegistryLockAction action =
lock.getUnlockCompletionTime().isPresent()
? RegistryLockAction.UNLOCKED
@@ -65,20 +64,19 @@ public class ConsoleRegistryLockVerifyAction extends ConsoleApiAction {
new RegistryLockVerificationResponse(
Ascii.toLowerCase(action.toString()), lock.getDomainName(), lock.getRegistrarId());
tm().transact(
() -> {
finishAndPersistConsoleUpdateHistory(
new ConsoleUpdateHistory.Builder()
.setType(
action == RegistryLockAction.LOCKED
? ConsoleUpdateHistory.Type.REGISTRY_LOCK
: ConsoleUpdateHistory.Type.REGISTRY_UNLOCK)
.setDescription(
String.format(
"%s%s%s",
lock.getRegistrarId(),
ConsoleUpdateHistory.DESCRIPTION_SEPARATOR,
lockResponse)));
});
() ->
finishAndPersistConsoleUpdateHistory(
new ConsoleUpdateHistory.Builder()
.setType(
action == RegistryLockAction.LOCKED
? ConsoleUpdateHistory.Type.REGISTRY_LOCK
: ConsoleUpdateHistory.Type.REGISTRY_UNLOCK)
.setDescription(
String.format(
"%s%s%s",
lock.getRegistrarId(),
ConsoleUpdateHistory.DESCRIPTION_SEPARATOR,
lockResponse))));
consoleApiParams.response().setPayload(consoleApiParams.gson().toJson(lockResponse));
consoleApiParams.response().setStatus(HttpServletResponse.SC_OK);
}
@@ -31,10 +31,14 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
import com.google.cloud.tasks.v2.HttpMethod;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import google.registry.batch.RelockDomainAction;
import google.registry.model.billing.BillingBase;
import google.registry.model.billing.BillingBase.Reason;
import google.registry.model.billing.BillingEvent;
import google.registry.model.console.RegistrarRole;
import google.registry.model.console.User;
import google.registry.model.console.UserRoles;
import google.registry.model.domain.Domain;
import google.registry.model.domain.DomainHistory;
import google.registry.model.domain.RegistryLock;
@@ -67,6 +71,21 @@ public final class DomainLockUtilsTest {
private static final String DOMAIN_NAME = "example.tld";
private static final String POC_ID = "marla.singer@example.com";
private static final User NON_ADMIN_USER =
new User.Builder()
.setEmailAddress("user@theregistrar.com")
.setUserRoles(
new UserRoles.Builder()
.setRegistrarRoles(ImmutableMap.of("TheRegistrar", RegistrarRole.PRIMARY_CONTACT))
.build())
.build();
private static final User ADMIN_USER =
new User.Builder()
.setEmailAddress("admin@theregistrar.com")
.setUserRoles(new UserRoles.Builder().setIsAdmin(true).build())
.build();
private final FakeClock clock = new FakeClock(Instant.now());
private DomainLockUtils domainLockUtils;
private CloudTasksHelper cloudTasksHelper = new CloudTasksHelper(clock);
@@ -122,7 +141,7 @@ public final class DomainLockUtilsTest {
clock.advanceBy(Duration.ofDays(1));
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), NON_ADMIN_USER);
verifyProperlyLockedDomain(false);
}
@@ -135,7 +154,7 @@ public final class DomainLockUtilsTest {
RegistryLock unlockRequest =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
domainLockUtils.verifyVerificationCode(unlockRequest.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(unlockRequest.getVerificationCode(), NON_ADMIN_USER);
assertThat(loadByEntity(domain).getStatusValues()).containsNoneIn(REGISTRY_LOCK_STATUSES);
}
@@ -143,7 +162,7 @@ public final class DomainLockUtilsTest {
void testSuccess_applyLockDomain() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), NON_ADMIN_USER);
verifyProperlyLockedDomain(false);
}
@@ -153,7 +172,7 @@ public final class DomainLockUtilsTest {
RegistryLock unlock =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), NON_ADMIN_USER);
verifyProperlyUnlockedDomain(false);
}
@@ -161,7 +180,7 @@ public final class DomainLockUtilsTest {
void testSuccess_applyAdminLock_onlyHistoryEntry() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", null, true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), ADMIN_USER);
verifyProperlyLockedDomain(true);
}
@@ -169,11 +188,11 @@ public final class DomainLockUtilsTest {
void testSuccess_applyAdminUnlock_onlyHistoryEntry() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", null, true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), ADMIN_USER);
RegistryLock unlock =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", true, Optional.empty());
domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), true);
domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), ADMIN_USER);
verifyProperlyUnlockedDomain(true);
}
@@ -194,7 +213,7 @@ public final class DomainLockUtilsTest {
void testSuccess_administrativelyUnlock_nonAdmin() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), NON_ADMIN_USER);
domainLockUtils.administrativelyApplyUnlock(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
verifyProperlyUnlockedDomain(false);
@@ -204,7 +223,7 @@ public final class DomainLockUtilsTest {
void testSuccess_administrativelyUnlock_admin() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", null, true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), ADMIN_USER);
domainLockUtils.administrativelyApplyUnlock(
DOMAIN_NAME, "TheRegistrar", true, Optional.empty());
verifyProperlyUnlockedDomain(true);
@@ -218,7 +237,7 @@ public final class DomainLockUtilsTest {
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
RegistryLock newLock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
newLock = domainLockUtils.verifyVerificationCode(newLock.getVerificationCode(), false);
newLock = domainLockUtils.verifyVerificationCode(newLock.getVerificationCode(), NON_ADMIN_USER);
assertThat(
getRegistryLockByRevisionId(oldLock.getRevisionId()).get().getRelock().getRevisionId())
.isEqualTo(newLock.getRevisionId());
@@ -252,7 +271,7 @@ public final class DomainLockUtilsTest {
RegistryLock lock =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.of(Duration.ofHours(6)));
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), NON_ADMIN_USER);
cloudTasksHelper.assertTasksEnqueued(
QUEUE_ASYNC_ACTIONS,
new TaskMatcher()
@@ -302,7 +321,7 @@ public final class DomainLockUtilsTest {
void testFailure_createUnlock_alreadyPendingUnlock() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), NON_ADMIN_USER);
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
@@ -321,7 +340,7 @@ public final class DomainLockUtilsTest {
void testFailure_createUnlock_nonAdminUnlockingAdmin() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", null, true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), ADMIN_USER);
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
@@ -385,12 +404,13 @@ public final class DomainLockUtilsTest {
void testFailure_applyLock_alreadyApplied() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), NON_ADMIN_USER);
domain = loadByEntity(domain);
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false));
() ->
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), NON_ADMIN_USER));
assertThat(thrown)
.hasMessageThat()
.isEqualTo("Lock/unlock with code 123456789ABCDEFGHJKLMNPQRSTUVWXY is already completed");
@@ -405,7 +425,7 @@ public final class DomainLockUtilsTest {
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true));
() -> domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), ADMIN_USER));
assertThat(thrown).hasMessageThat().isEqualTo("The pending lock has expired; please try again");
assertNoDomainChanges();
}
@@ -417,25 +437,75 @@ public final class DomainLockUtilsTest {
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false));
() ->
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), NON_ADMIN_USER));
assertThat(thrown).hasMessageThat().isEqualTo("Non-admin user cannot complete admin lock");
assertNoDomainChanges();
}
@Test
void testFailure_applyLock_noPermission() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
User userWithoutPermission =
new User.Builder()
.setEmailAddress("unauthorized@example.com")
.setUserRoles(new UserRoles.Builder().build())
.build();
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() ->
domainLockUtils.verifyVerificationCode(
lock.getVerificationCode(), userWithoutPermission));
assertThat(thrown)
.hasMessageThat()
.isEqualTo(
"User unauthorized@example.com does not have registry lock permission on registrar"
+ " TheRegistrar");
assertNoDomainChanges();
}
@Test
void testFailure_applyUnlock_noPermission() {
domainLockUtils.administrativelyApplyLock(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
RegistryLock unlock =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
User userWithoutPermission =
new User.Builder()
.setEmailAddress("unauthorized@example.com")
.setUserRoles(new UserRoles.Builder().build())
.build();
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() ->
domainLockUtils.verifyVerificationCode(
unlock.getVerificationCode(), userWithoutPermission));
assertThat(thrown)
.hasMessageThat()
.isEqualTo(
"User unauthorized@example.com does not have registry lock permission on registrar"
+ " TheRegistrar");
}
@Test
void testFailure_applyUnlock_alreadyUnlocked() {
RegistryLock lock =
domainLockUtils.saveNewRegistryLockRequest(DOMAIN_NAME, "TheRegistrar", POC_ID, false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), NON_ADMIN_USER);
RegistryLock unlock =
domainLockUtils.saveNewRegistryUnlockRequest(
DOMAIN_NAME, "TheRegistrar", false, Optional.empty());
domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), false);
domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), NON_ADMIN_USER);
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> domainLockUtils.verifyVerificationCode(unlock.getVerificationCode(), false));
() ->
domainLockUtils.verifyVerificationCode(
unlock.getVerificationCode(), NON_ADMIN_USER));
assertThat(thrown)
.hasMessageThat()
.isEqualTo("Lock/unlock with code Zabcdefghijkmnopqrstuvwxyz123456 is already completed");
@@ -453,7 +523,7 @@ public final class DomainLockUtilsTest {
IllegalArgumentException thrown =
assertThrows(
IllegalArgumentException.class,
() -> domainLockUtils.verifyVerificationCode(verificationCode, false));
() -> domainLockUtils.verifyVerificationCode(verificationCode, NON_ADMIN_USER));
assertThat(thrown).hasMessageThat().isEqualTo("Domain example.tld is already locked");
// Failure during the lock acquisition portion shouldn't affect the SQL object
@@ -27,6 +27,8 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import google.registry.model.console.User;
import google.registry.model.console.UserRoles;
import google.registry.model.domain.Domain;
import google.registry.model.domain.RegistryLock;
import google.registry.model.registrar.Registrar.Type;
@@ -56,11 +58,17 @@ class UnlockDomainCommandTest extends CommandTestCase<UnlockDomainCommand> {
command.printStream = System.out;
}
private static final User ADMIN_USER =
new User.Builder()
.setEmailAddress("admin@theregistrar.com")
.setUserRoles(new UserRoles.Builder().setIsAdmin(true).build())
.build();
private Domain persistLockedDomain(String domainName, String registrarId) {
Domain domain = persistResource(DatabaseHelper.newDomain(domainName));
RegistryLock lock =
command.domainLockUtils.saveNewRegistryLockRequest(domainName, registrarId, null, true);
command.domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), true);
command.domainLockUtils.verifyVerificationCode(lock.getVerificationCode(), ADMIN_USER);
return reloadResource(domain);
}
@@ -182,6 +182,20 @@ public class ConsoleRegistryLockVerifyActionTest extends ConsoleActionBaseTestCa
.containsAtLeastElementsIn(REGISTRY_LOCK_STATUSES);
}
@Test
void testFailure_noPermission() {
saveRegistryLock(createDefaultLockBuilder().build());
user = user.asBuilder().setUserRoles(new UserRoles.Builder().build()).build();
action = createAction(DEFAULT_CODE);
action.run();
assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_BAD_REQUEST);
assertThat(response.getPayload())
.isEqualTo(
"User user@theregistrar.com does not have registry lock permission on registrar"
+ " TheRegistrar");
assertThat(loadByEntity(defaultDomain).getStatusValues()).containsExactly(StatusValue.INACTIVE);
}
private RegistryLock.Builder createDefaultLockBuilder() {
return new RegistryLock.Builder()
.setRepoId(defaultDomain.getRepoId())
@@ -194,7 +208,7 @@ public class ConsoleRegistryLockVerifyActionTest extends ConsoleActionBaseTestCa
private ConsoleRegistryLockVerifyAction createAction(String verificationCode) {
AuthResult authResult = AuthResult.createUser(user);
ConsoleApiParams params = ConsoleApiParamsUtils.createFake(authResult);
when(params.request().getMethod()).thenReturn("GET");
when(params.request().getMethod()).thenReturn("POST");
when(params.request().getServerName()).thenReturn("registrarconsole.tld");
DomainLockUtils domainLockUtils =
new DomainLockUtils(
@@ -82,7 +82,7 @@ CONSOLE /console-api/password-reset-verify PasswordResetVerifyA
CONSOLE /console-api/registrar ConsoleUpdateRegistrarAction POST n USER PUBLIC
CONSOLE /console-api/registrars RegistrarsAction GET,POST n USER PUBLIC
CONSOLE /console-api/registry-lock ConsoleRegistryLockAction GET,POST n USER PUBLIC
CONSOLE /console-api/registry-lock-verify ConsoleRegistryLockVerifyAction GET n USER PUBLIC
CONSOLE /console-api/registry-lock-verify ConsoleRegistryLockVerifyAction POST n USER PUBLIC
CONSOLE /console-api/settings/contacts ContactAction GET,POST,DELETE,PUT n USER PUBLIC
CONSOLE /console-api/settings/rdap-fields RdapRegistrarFieldsAction POST n USER PUBLIC
CONSOLE /console-api/settings/security SecurityAction POST n USER PUBLIC