Match logged-in GAE user ID with registrar POC user ID (#511)

* Match logged-in GAE user ID with registrar POC user ID

The reasoning for this is thus:
We wish to have the users log in using Google-managed addresses--this is
so that we can manage enforcement of things like 2FA, as well as generic
account management. However, we wish for the registry-lock confirmation
emails to go to their standard non-Google email addresses--e.g.
johndoe@theregistrar.com, rather than johndoe@registry.google.

As a result, for registry lock, we will enable it on
the johndoe@registry.google account, but we will alter the email address
of the corresponding Registrar POC account to contain
johndoe@theregistrar.com. By doing this, the user will still be logging
in using the @registry.google account but we'll match to their actual
contact email.

* fix up comments and messages

* Error if >1 matching contact

* include email addresses

* set default optional

* fix tests
This commit is contained in:
gbrodman 2020-03-16 11:38:05 -04:00 committed by GitHub
parent 0545375eba
commit d09fc7ee05
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 148 additions and 68 deletions

View file

@ -249,7 +249,7 @@ public final class AppEngineRule extends ExternalResource {
.setEmailAddress("Marla.Singer@crr.com")
.setPhoneNumber("+1.2128675309")
.setTypes(ImmutableSet.of(RegistrarContact.Type.TECH))
.setGaeUserId(THE_REGISTRAR_GAE_USER_ID)
.setGaeUserId("12345")
.setAllowedToSetRegistryLockPassword(true)
.setRegistryLockPassword("hi")
.build();

View file

@ -31,6 +31,7 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.gson.Gson;
import google.registry.model.registrar.RegistrarContact;
import google.registry.request.Action.Method;
import google.registry.request.auth.AuthLevel;
import google.registry.request.auth.AuthResult;
@ -67,14 +68,15 @@ public final class RegistryLockGetActionTest {
@Rule public final MockitoRule mocks = MockitoJUnit.rule();
private final FakeResponse response = new FakeResponse();
private final User user = new User("Marla.Singer@crr.com", "gmail.com", "12345");
private User user;
private AuthResult authResult;
private AuthenticatedRegistrarAccessor accessor;
private RegistryLockGetAction action;
@Before
public void setup() {
user = userFromRegistrarContact(AppEngineRule.makeRegistrarContact3());
fakeClock.setTo(DateTime.parse("2000-06-08T22:00:00.0Z"));
authResult = AuthResult.create(AuthLevel.USER, UserAuthInfo.create(user, false));
accessor =
@ -309,6 +311,29 @@ public final class RegistryLockGetActionTest {
ImmutableList.of())));
}
@Test
public void testSuccess_linkedToContactEmail() {
// Even though the user is some.email@gmail.com the contact is still Marla Singer
user = new User("some.email@gmail.com", "gmail.com", user.getUserId());
authResult = AuthResult.create(AuthLevel.USER, UserAuthInfo.create(user, false));
action =
new RegistryLockGetAction(
Method.GET, response, accessor, authResult, Optional.of("TheRegistrar"));
action.run();
assertThat(GSON.fromJson(response.getPayload(), Map.class).get("results"))
.isEqualTo(
ImmutableList.of(
ImmutableMap.of(
"lockEnabledForContact",
true,
"email",
"Marla.Singer@crr.com",
"clientId",
"TheRegistrar",
"locks",
ImmutableList.of())));
}
@Test
public void testFailure_lockNotAllowedForRegistrar() {
// The UI shouldn't be making requests where lock isn't enabled for this registrar
@ -337,4 +362,9 @@ public final class RegistryLockGetActionTest {
action.run();
assertThat(response.getStatus()).isEqualTo(SC_FORBIDDEN);
}
static User userFromRegistrarContact(RegistrarContact registrarContact) {
return new User(
registrarContact.getEmailAddress(), "gmail.com", registrarContact.getGaeUserId());
}
}

View file

@ -23,6 +23,7 @@ import static google.registry.testing.DatastoreHelper.persistResource;
import static google.registry.testing.SqlHelper.getRegistryLockByVerificationCode;
import static google.registry.testing.SqlHelper.saveRegistryLock;
import static google.registry.tools.LockOrUnlockDomainCommand.REGISTRY_LOCK_STATUSES;
import static google.registry.ui.server.registrar.RegistryLockGetActionTest.userFromRegistrarContact;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
@ -78,11 +79,8 @@ public final class RegistryLockPostActionTest {
@Rule public final MockitoRule mocks = MockitoJUnit.rule();
private final User userWithoutPermission =
new User("johndoe@theregistrar.com", "gmail.com", "31337");
// Marla Singer has registry lock auth permissions
private final User userWithLockPermission =
new User("Marla.Singer@crr.com", "gmail.com", "31337");
private User userWithoutPermission;
private User userWithLockPermission;
private InternetAddress outgoingAddress;
private DomainBase domain;
@ -93,6 +91,8 @@ public final class RegistryLockPostActionTest {
@Before
public void setup() throws Exception {
userWithLockPermission = userFromRegistrarContact(AppEngineRule.makeRegistrarContact3());
userWithoutPermission = userFromRegistrarContact(AppEngineRule.makeRegistrarContact2());
createTld("tld");
domain = persistResource(newDomainBase("example.tld"));
outgoingAddress = new InternetAddress("domain-registry@example.com");
@ -133,6 +133,18 @@ public final class RegistryLockPostActionTest {
assertSuccess(response, "unlock", "johndoe@theregistrar.com");
}
@Test
public void testSuccess_linkedToContactEmail() throws Exception {
// Even though the user is some.email@gmail.com the contact is still Marla Singer
userWithLockPermission =
new User("some.email@gmail.com", "gmail.com", userWithLockPermission.getUserId());
action =
createAction(
AuthResult.create(AuthLevel.USER, UserAuthInfo.create(userWithLockPermission, false)));
Map<String, ?> response = action.handleJsonRequest(lockRequest());
assertSuccess(response, "lock", "Marla.Singer@crr.com");
}
@Test
public void testFailure_unlock_noLock() {
persistResource(domain.asBuilder().setStatusValues(REGISTRY_LOCK_STATUSES).build());
@ -233,7 +245,7 @@ public final class RegistryLockPostActionTest {
persistResource(
loadRegistrar("TheRegistrar").asBuilder().setRegistryLockAllowed(false).build());
Map<String, ?> response = action.handleJsonRequest(lockRequest());
assertFailureWithMessage(response, "Registry lock not allowed for this registrar");
assertFailureWithMessage(response, "Registry lock not allowed for registrar TheRegistrar");
}
@Test
@ -244,7 +256,7 @@ public final class RegistryLockPostActionTest {
"clientId", "TheRegistrar",
"fullyQualifiedDomainName", "example.tld",
"isLock", true));
assertFailureWithMessage(response, "Missing key for password");
assertFailureWithMessage(response, "Incorrect registry lock password for contact");
}
@Test
@ -386,9 +398,10 @@ public final class RegistryLockPostActionTest {
}
private RegistryLockPostAction createAction(AuthResult authResult) {
Role role = authResult.userAuthInfo().get().isUserAdmin() ? Role.ADMIN : Role.OWNER;
AuthenticatedRegistrarAccessor registrarAccessor =
AuthenticatedRegistrarAccessor.createForTesting(
ImmutableSetMultimap.of("TheRegistrar", Role.OWNER, "NewRegistrar", Role.OWNER));
ImmutableSetMultimap.of("TheRegistrar", role, "NewRegistrar", role));
JsonActionRunner jsonActionRunner =
new JsonActionRunner(ImmutableMap.of(), new JsonResponse(new ResponseImpl(mockResponse)));
DomainLockUtils domainLockUtils =

View file

@ -60,7 +60,8 @@ public class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
route("/registry-lock-verify", FrontendServlet.class))
.setFilters(ObjectifyFilter.class, OfyFilter.class)
.setFixtures(BASIC)
.setEmail("Marla.Singer@crr.com")
.setEmail("Marla.Singer@crr.com") // from AppEngineRule.makeRegistrarContact3
.setGaeUserId("12345") // from AppEngineRule.makeRegistrarContact3
.build();
@Test
@ -452,11 +453,12 @@ public class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
createTld("tld");
// expired unlock request
DomainBase expiredUnlockRequestDomain = persistActiveDomain("expiredunlock.tld");
saveRegistryLock(createRegistryLock(expiredUnlockRequestDomain)
.asBuilder()
.setLockCompletionTimestamp(START_OF_TIME.minusDays(1))
.setUnlockRequestTimestamp(START_OF_TIME.minusDays(1))
.build());
saveRegistryLock(
createRegistryLock(expiredUnlockRequestDomain)
.asBuilder()
.setLockCompletionTimestamp(START_OF_TIME.minusDays(1))
.setUnlockRequestTimestamp(START_OF_TIME.minusDays(1))
.build());
DomainBase domain = persistActiveDomain("example.tld");
saveRegistryLock(createRegistryLock(domain).asBuilder().isSuperuser(true).build());
DomainBase otherDomain = persistActiveDomain("otherexample.tld");

View file

@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
import static google.registry.testing.AppEngineRule.THE_REGISTRAR_GAE_USER_ID;
import static google.registry.util.NetworkUtils.getExternalAddressOfLocalSystem;
import static google.registry.util.NetworkUtils.pickUnusedPort;
import static google.registry.util.PreconditionsUtils.checkArgumentNotNull;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@ -32,6 +33,7 @@ import google.registry.testing.UserInfo;
import java.net.URL;
import java.net.UnknownHostException;
import java.nio.file.Path;
import java.util.Optional;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.Callable;
import java.util.concurrent.FutureTask;
@ -66,7 +68,8 @@ public final class TestServerRule extends ExternalResource {
ImmutableList<Route> routes,
ImmutableList<Class<? extends Filter>> filters,
ImmutableList<Fixture> fixtures,
String email) {
String email,
Optional<String> gaeUserId) {
this.runfiles = runfiles;
this.routes = routes;
this.filters = filters;
@ -79,7 +82,8 @@ public final class TestServerRule extends ExternalResource {
.withLocalModules()
.withUrlFetch()
.withTaskQueue()
.withUserService(UserInfo.createAdmin(email, THE_REGISTRAR_GAE_USER_ID))
.withUserService(
UserInfo.createAdmin(email, gaeUserId.orElse(THE_REGISTRAR_GAE_USER_ID)))
.build();
}
@ -149,8 +153,8 @@ public final class TestServerRule extends ExternalResource {
/**
* Runs arbitrary code inside server event loop thread.
*
* <p>You should use this method when you want to do things like change Datastore, because the
* App Engine testing environment is thread-local.
* <p>You should use this method when you want to do things like change Datastore, because the App
* Engine testing environment is thread-local.
*/
public <T> T runInAppEngineEnvironment(Callable<T> callback) throws Throwable {
FutureTask<T> job = new FutureTask<>(callback);
@ -211,7 +215,6 @@ public final class TestServerRule extends ExternalResource {
*
* <p>This builder has three required fields: {@link #setRunfiles}, {@link #setRoutes}, and {@link
* #setFilters}.
*
*/
public static final class Builder {
private ImmutableMap<String, Path> runfiles;
@ -219,6 +222,7 @@ public final class TestServerRule extends ExternalResource {
ImmutableList<Class<? extends Filter>> filters;
private ImmutableList<Fixture> fixtures = ImmutableList.of();
private String email;
private Optional<String> gaeUserId = Optional.empty();
/** Sets the directories containing the static files for {@link TestServer}. */
public Builder setRunfiles(ImmutableMap<String, Path> runfiles) {
@ -256,6 +260,13 @@ public final class TestServerRule extends ExternalResource {
return this;
}
/** Optionally, sets the GAE user ID for the logged in user. */
public Builder setGaeUserId(String gaeUserId) {
this.gaeUserId =
Optional.of(checkArgumentNotNull(gaeUserId, "Must specify a non-null GAE user ID"));
return this;
}
/** Returns a new {@link TestServerRule} instance. */
public TestServerRule build() {
return new TestServerRule(
@ -263,7 +274,8 @@ public final class TestServerRule extends ExternalResource {
checkNotNull(this.routes),
checkNotNull(this.filters),
checkNotNull(this.fixtures),
checkNotNull(this.email));
checkNotNull(this.email),
this.gaeUserId);
}
}
}

View file

@ -13,7 +13,7 @@ contacts:
ADDED:
{parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Extra Terrestrial, emailAddress=etphonehome@example.com, phoneNumber=+1.2345678901, faxNumber=null, types=[ADMIN, BILLING, TECH, WHOIS], gaeUserId=null, visibleInWhoisAsAdmin=true, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
REMOVED:
{parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Marla Singer, emailAddress=Marla.Singer@crr.com, phoneNumber=+1.2128675309, faxNumber=null, types=[TECH], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
{parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Marla Singer, emailAddress=Marla.Singer@crr.com, phoneNumber=+1.2128675309, faxNumber=null, types=[TECH], gaeUserId=12345, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
{parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=John Doe, emailAddress=johndoe@theregistrar.com, phoneNumber=+1.1234567890, faxNumber=null, types=[ADMIN], gaeUserId=31337, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false},
{parent=Key<?>(EntityGroupRoot("cross-tld")/Registrar("TheRegistrar")), name=Jian-Yang, emailAddress=jyang@bachman.accelerator, phoneNumber=+1.1234567890, faxNumber=null, types=[TECH], gaeUserId=null, visibleInWhoisAsAdmin=false, visibleInWhoisAsTech=false, visibleInDomainWhoisAsAbuse=false, allowedToSetRegistryLockPassword=false}
FINAL CONTENTS: