From 2ff1026cfd8c0fc92af87b06b714879765a60015 Mon Sep 17 00:00:00 2001
From: gbrodman <gbrodman@google.com>
Date: Thu, 26 Mar 2020 14:29:05 -0400
Subject: [PATCH] Handle null GAE user IDs gracefully (for non-admins) (#531)

Unfortunately in our testing environments, we're all admins so it's easy
to miss things like this.
---
 .../registry/ui/server/registrar/RegistryLockGetAction.java | 4 +++-
 .../test/java/google/registry/testing/AppEngineRule.java    | 1 -
 .../google/registry/tools/RegistrarContactCommandTest.java  | 6 +++++-
 .../ui/server/registrar/RegistryLockGetActionTest.java      | 3 +++
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/core/src/main/java/google/registry/ui/server/registrar/RegistryLockGetAction.java b/core/src/main/java/google/registry/ui/server/registrar/RegistryLockGetAction.java
index 247f865ff..0e895edb5 100644
--- a/core/src/main/java/google/registry/ui/server/registrar/RegistryLockGetAction.java
+++ b/core/src/main/java/google/registry/ui/server/registrar/RegistryLockGetAction.java
@@ -43,6 +43,7 @@ import google.registry.request.auth.AuthenticatedRegistrarAccessor;
 import google.registry.request.auth.AuthenticatedRegistrarAccessor.RegistrarAccessDeniedException;
 import google.registry.schema.domain.RegistryLock;
 import google.registry.security.JsonResponseHelper;
+import java.util.Objects;
 import java.util.Optional;
 import javax.inject.Inject;
 import org.joda.time.DateTime;
@@ -119,7 +120,8 @@ public final class RegistryLockGetAction implements JsonGetAction {
   static Optional<RegistrarContact> getContactMatchingLogin(User user, Registrar registrar) {
     ImmutableList<RegistrarContact> matchingContacts =
         registrar.getContacts().stream()
-            .filter(contact -> contact.getGaeUserId().equals(user.getUserId()))
+            .filter(contact -> contact.getGaeUserId() != null)
+            .filter(contact -> Objects.equals(contact.getGaeUserId(), user.getUserId()))
             .collect(toImmutableList());
     if (matchingContacts.size() > 1) {
       ImmutableList<String> matchingEmails =
diff --git a/core/src/test/java/google/registry/testing/AppEngineRule.java b/core/src/test/java/google/registry/testing/AppEngineRule.java
index cb7c4dab1..52af8442f 100644
--- a/core/src/test/java/google/registry/testing/AppEngineRule.java
+++ b/core/src/test/java/google/registry/testing/AppEngineRule.java
@@ -224,7 +224,6 @@ public final class AppEngineRule extends ExternalResource {
         .setEmailAddress("janedoe@theregistrar.com")
         .setPhoneNumber("+1.1234567890")
         .setTypes(ImmutableSet.of(RegistrarContact.Type.ADMIN))
-        .setGaeUserId(NEW_REGISTRAR_GAE_USER_ID)
         .build();
   }
 
diff --git a/core/src/test/java/google/registry/tools/RegistrarContactCommandTest.java b/core/src/test/java/google/registry/tools/RegistrarContactCommandTest.java
index 8fd847786..45919fbf6 100644
--- a/core/src/test/java/google/registry/tools/RegistrarContactCommandTest.java
+++ b/core/src/test/java/google/registry/tools/RegistrarContactCommandTest.java
@@ -127,7 +127,11 @@ public class RegistrarContactCommandTest extends CommandTestCase<RegistrarContac
         "--email=jane.doe@example.com",
         "--allow_console_access=true",
         "NewRegistrar");
-    RegistrarContact registrarContact = loadRegistrar("NewRegistrar").getContacts().asList().get(1);
+    RegistrarContact registrarContact =
+        loadRegistrar("NewRegistrar").getContacts().stream()
+            .filter(rc -> rc.getEmailAddress().equals("jane.doe@example.com"))
+            .findFirst()
+            .get();
     assertThat(registrarContact.getGaeUserId()).matches("-?[0-9]+");
   }
 
diff --git a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockGetActionTest.java b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockGetActionTest.java
index afe7b4771..d120d845e 100644
--- a/core/src/test/java/google/registry/ui/server/registrar/RegistryLockGetActionTest.java
+++ b/core/src/test/java/google/registry/ui/server/registrar/RegistryLockGetActionTest.java
@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
 import static google.registry.request.auth.AuthenticatedRegistrarAccessor.Role.ADMIN;
 import static google.registry.request.auth.AuthenticatedRegistrarAccessor.Role.OWNER;
 import static google.registry.testing.AppEngineRule.makeRegistrar2;
+import static google.registry.testing.AppEngineRule.makeRegistrarContact2;
 import static google.registry.testing.AppEngineRule.makeRegistrarContact3;
 import static google.registry.testing.DatastoreHelper.persistResource;
 import static google.registry.testing.SqlHelper.saveRegistryLock;
@@ -287,6 +288,8 @@ public final class RegistryLockGetActionTest {
   public void testSuccess_lockAllowedForAdmin() {
     // Locks are allowed for admins even when they're not enabled for the registrar
     persistResource(makeRegistrar2().asBuilder().setRegistryLockAllowed(false).build());
+    // disallow the other user
+    persistResource(makeRegistrarContact2().asBuilder().setGaeUserId(null).build());
     authResult = AuthResult.create(AuthLevel.USER, UserAuthInfo.create(user, true));
     accessor =
         AuthenticatedRegistrarAccessor.createForTesting(