Add XSRF protection to legacy authentication mechanism

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148689952

javatests/google/registry/request/RequestHandlerTest.java

@@ -241,7 +241,7 @@ public final class RequestHandlerTest {
                 ImmutableSet.of("https://www.googleapis.com/auth/userinfo.email"),
                 ImmutableSet.of("https://www.googleapis.com/auth/userinfo.email"),
                 ImmutableSet.of("proxy-client-id", "regtool-client-id"))),
-        new LegacyAuthenticationMechanism(userService));
+        new LegacyAuthenticationMechanism(userService, xsrfTokenManager));
 
     // Initialize here, not inline, so that we pick up the mocked UserService.
     handler = RequestHandler.<Component>createForTest(
@@ -382,7 +382,8 @@ public final class RequestHandlerTest {
   public void testXsrfProtection_validTokenProvided_runsAction() throws Exception {
     userService.setUser(testUser,  false);
     when(req.getMethod()).thenReturn("POST");
-    when(req.getHeader("X-CSRF-Token")).thenReturn(xsrfTokenManager.generateToken("vampire"));
+    when(req.getHeader("X-CSRF-Token"))
+        .thenReturn(xsrfTokenManager.generateTokenWithCurrentUser("vampire"));
     when(req.getRequestURI()).thenReturn("/safe-sloth");
     handler.handleRequest(req, rsp);
     verify(safeSlothTask).run();
@@ -392,7 +393,8 @@ public final class RequestHandlerTest {
   public void testXsrfProtection_tokenWithInvalidScopeProvided_returns403() throws Exception {
     userService.setUser(testUser,  false);
     when(req.getMethod()).thenReturn("POST");
-    when(req.getHeader("X-CSRF-Token")).thenReturn(xsrfTokenManager.generateToken("blood"));
+    when(req.getHeader("X-CSRF-Token"))
+        .thenReturn(xsrfTokenManager.generateTokenWithCurrentUser("blood"));
     when(req.getRequestURI()).thenReturn("/safe-sloth");
     handler.handleRequest(req, rsp);
     verify(rsp).sendError(403, "Invalid X-CSRF-Token");

javatests/google/registry/request/auth/BUILD

@@ -14,6 +14,7 @@ java_library(
     deps = [
         "//java/google/registry/request",
         "//java/google/registry/request/auth",
+        "//java/google/registry/security",
         "//javatests/google/registry/testing",
         "//third_party/java/objectify:objectify-v4_1",
         "@com_google_appengine_api_1_0_sdk//:testonly",

javatests/google/registry/request/auth/RequestAuthenticatorTest.java

@@ -26,11 +26,14 @@ import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import google.registry.request.Action;
+import google.registry.security.XsrfTokenManager;
 import google.registry.testing.AppEngineRule;
 import google.registry.testing.ExceptionRule;
+import google.registry.testing.FakeClock;
 import google.registry.testing.FakeOAuthService;
 import google.registry.testing.FakeUserService;
 import javax.servlet.http.HttpServletRequest;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -125,6 +128,8 @@ public class RequestAuthenticatorTest {
 
   private final User testUser = new User("test@google.com", "test@google.com");
   private final FakeUserService fakeUserService = new FakeUserService();
+  private final XsrfTokenManager xsrfTokenManager =
+      new XsrfTokenManager(new FakeClock(), fakeUserService);
   private final FakeOAuthService fakeOAuthService = new FakeOAuthService(
       false /* isOAuthEnabled */,
       testUser,
@@ -132,6 +137,11 @@ public class RequestAuthenticatorTest {
       "test-client-id",
       ImmutableList.of("test-scope1", "test-scope2", "nontest-scope"));
 
+  @Before
+  public void before() throws Exception {
+    when(req.getMethod()).thenReturn("POST");
+  }
+
   private RequestAuthenticator createRequestAuthenticator(UserService userService) {
     return new RequestAuthenticator(
         new AppEngineInternalAuthenticationMechanism(),
@@ -141,7 +151,7 @@ public class RequestAuthenticatorTest {
                 ImmutableSet.of("test-scope1", "test-scope2", "test-scope3"),
                 ImmutableSet.of("test-scope1", "test-scope2"),
                 ImmutableSet.of("test-client-id", "other-test-client-id"))),
-        new LegacyAuthenticationMechanism(userService));
+        new LegacyAuthenticationMechanism(userService, xsrfTokenManager));
   }
 
   private Optional<AuthResult> runTest(UserService userService, Class<?> clazz) {
@@ -152,19 +162,20 @@ public class RequestAuthenticatorTest {
   @Test
   public void testNoAuthNeeded_noneFound() throws Exception {
     Optional<AuthResult> authResult = runTest(mockUserService, AuthNone.class);
+
     verifyZeroInteractions(mockUserService);
     assertThat(authResult).isPresent();
-    assertThat(authResult.get()).isNotNull();
     assertThat(authResult.get().authLevel()).isEqualTo(AuthLevel.NONE);
   }
 
   @Test
   public void testNoAuthNeeded_internalFound() throws Exception {
     when(req.getHeader("X-AppEngine-QueueName")).thenReturn("__cron");
+
     Optional<AuthResult> authResult = runTest(mockUserService, AuthNone.class);
+
     verifyZeroInteractions(mockUserService);
     assertThat(authResult).isPresent();
-    assertThat(authResult.get()).isNotNull();
     assertThat(authResult.get().authLevel()).isEqualTo(AuthLevel.APP);
     assertThat(authResult.get().userAuthInfo()).isAbsent();
   }
@@ -172,6 +183,7 @@ public class RequestAuthenticatorTest {
   @Test
   public void testInternalAuth_notInvokedInternally() throws Exception {
     Optional<AuthResult> authResult = runTest(mockUserService, AuthInternalOnly.class);
+
     verifyZeroInteractions(mockUserService);
     assertThat(authResult).isAbsent();
   }
@@ -179,10 +191,11 @@ public class RequestAuthenticatorTest {
   @Test
   public void testInternalAuth_success() throws Exception {
     when(req.getHeader("X-AppEngine-QueueName")).thenReturn("__cron");
+
     Optional<AuthResult> authResult = runTest(mockUserService, AuthInternalOnly.class);
+
     verifyZeroInteractions(mockUserService);
     assertThat(authResult).isPresent();
-    assertThat(authResult.get()).isNotNull();
     assertThat(authResult.get().authLevel()).isEqualTo(AuthLevel.APP);
     assertThat(authResult.get().userAuthInfo()).isAbsent();
   }
@@ -190,15 +203,28 @@ public class RequestAuthenticatorTest {
   @Test
   public void testAnyUserAnyMethod_notLoggedIn() throws Exception {
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserAnyMethod.class);
+
+    assertThat(authResult).isAbsent();
+  }
+
+  @Test
+  public void testAnyUserAnyMethod_xsrfFailure() throws Exception {
+    fakeUserService.setUser(testUser, false);
+
+    Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserAnyMethod.class);
+
     assertThat(authResult).isAbsent();
   }
 
   @Test
   public void testAnyUserAnyMethod_success() throws Exception {
     fakeUserService.setUser(testUser, false /* isAdmin */);
+    when(req.getHeader(XsrfTokenManager.X_CSRF_TOKEN))
+        .thenReturn(xsrfTokenManager.generateTokenWithCurrentUser("console"));
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserAnyMethod.class);
+
     assertThat(authResult).isPresent();
-    assertThat(authResult.get()).isNotNull();
     assertThat(authResult.get().authLevel()).isEqualTo(AuthLevel.USER);
     assertThat(authResult.get().userAuthInfo()).isPresent();
     assertThat(authResult.get().userAuthInfo().get().user()).isEqualTo(testUser);
@@ -206,23 +232,53 @@ public class RequestAuthenticatorTest {
     assertThat(authResult.get().userAuthInfo().get().oauthTokenInfo()).isAbsent();
   }
 
+  @Test
+  public void testAnyUserAnyMethod_xsrfNotRequiredForGet() throws Exception {
+    fakeUserService.setUser(testUser, false);
+    when(req.getMethod()).thenReturn("GET");
+
+    Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserAnyMethod.class);
+
+    assertThat(authResult).isPresent();
+    assertThat(authResult.get().authLevel()).isEqualTo(AuthLevel.USER);
+    assertThat(authResult.get().userAuthInfo()).isPresent();
+    assertThat(authResult.get().userAuthInfo().get().user()).isEqualTo(testUser);
+    assertThat(authResult.get().userAuthInfo().get().oauthTokenInfo()).isAbsent();
+  }
+
   @Test
   public void testAdminUserAnyMethod_notLoggedIn() throws Exception {
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAdminUserAnyMethod.class);
+
     assertThat(authResult).isAbsent();
   }
 
   @Test
   public void testAdminUserAnyMethod_notAdminUser() throws Exception {
     fakeUserService.setUser(testUser, false /* isAdmin */);
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAdminUserAnyMethod.class);
+
+    assertThat(authResult).isAbsent();
+  }
+
+  @Test
+  public void testAdminUserAnyMethod_xsrfFailure() throws Exception {
+    fakeUserService.setUser(testUser, true);
+
+    Optional<AuthResult> authResult = runTest(fakeUserService, AuthAdminUserAnyMethod.class);
+
     assertThat(authResult).isAbsent();
   }
 
   @Test
   public void testAdminUserAnyMethod_success() throws Exception {
     fakeUserService.setUser(testUser, true /* isAdmin */);
+    when(req.getHeader(XsrfTokenManager.X_CSRF_TOKEN))
+        .thenReturn(xsrfTokenManager.generateTokenWithCurrentUser("console"));
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAdminUserAnyMethod.class);
+
     assertThat(authResult).isPresent();
     assertThat(authResult.get().authLevel()).isEqualTo(AuthLevel.USER);
     assertThat(authResult.get().userAuthInfo()).isPresent();
@@ -236,7 +292,9 @@ public class RequestAuthenticatorTest {
     fakeOAuthService.setUser(testUser);
     fakeOAuthService.setOAuthEnabled(true);
     when(req.getHeader(AUTHORIZATION)).thenReturn("Bearer TOKEN");
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserNoLegacy.class);
+
     assertThat(authResult).isPresent();
     assertThat(authResult.get().authLevel()).isEqualTo(AuthLevel.USER);
     assertThat(authResult.get().userAuthInfo()).isPresent();
@@ -257,7 +315,9 @@ public class RequestAuthenticatorTest {
     fakeOAuthService.setUserAdmin(true);
     fakeOAuthService.setOAuthEnabled(true);
     when(req.getHeader(AUTHORIZATION)).thenReturn("Bearer TOKEN");
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserNoLegacy.class);
+
     assertThat(authResult).isPresent();
     assertThat(authResult.get().authLevel()).isEqualTo(AuthLevel.USER);
     assertThat(authResult.get().userAuthInfo()).isPresent();
@@ -276,7 +336,9 @@ public class RequestAuthenticatorTest {
   public void testOAuthMissingAuthenticationToken_failure() throws Exception {
     fakeOAuthService.setUser(testUser);
     fakeOAuthService.setOAuthEnabled(true);
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserNoLegacy.class);
+
     assertThat(authResult).isAbsent();
   }
 
@@ -286,7 +348,9 @@ public class RequestAuthenticatorTest {
     fakeOAuthService.setOAuthEnabled(true);
     fakeOAuthService.setClientId("wrong-client-id");
     when(req.getHeader(AUTHORIZATION)).thenReturn("Bearer TOKEN");
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserNoLegacy.class);
+
     assertThat(authResult).isAbsent();
   }
 
@@ -296,7 +360,9 @@ public class RequestAuthenticatorTest {
     fakeOAuthService.setOAuthEnabled(true);
     fakeOAuthService.setAuthorizedScopes();
     when(req.getHeader(AUTHORIZATION)).thenReturn("Bearer TOKEN");
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserNoLegacy.class);
+
     assertThat(authResult).isAbsent();
   }
 
@@ -306,7 +372,9 @@ public class RequestAuthenticatorTest {
     fakeOAuthService.setOAuthEnabled(true);
     fakeOAuthService.setAuthorizedScopes("test-scope1", "test-scope3");
     when(req.getHeader(AUTHORIZATION)).thenReturn("Bearer TOKEN");
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserNoLegacy.class);
+
     assertThat(authResult).isAbsent();
   }
 
@@ -316,7 +384,9 @@ public class RequestAuthenticatorTest {
     fakeOAuthService.setOAuthEnabled(true);
     fakeOAuthService.setAuthorizedScopes("test-scope1", "test-scope2", "test-scope3");
     when(req.getHeader(AUTHORIZATION)).thenReturn("Bearer TOKEN");
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserNoLegacy.class);
+
     assertThat(authResult).isPresent();
     assertThat(authResult.get().authLevel()).isEqualTo(AuthLevel.USER);
     assertThat(authResult.get().userAuthInfo()).isPresent();
@@ -334,13 +404,16 @@ public class RequestAuthenticatorTest {
   @Test
   public void testAnyUserNoLegacy_failureWithLegacyUser() throws Exception {
     fakeUserService.setUser(testUser, false /* isAdmin */);
+
     Optional<AuthResult> authResult = runTest(fakeUserService, AuthAnyUserNoLegacy.class);
+
     assertThat(authResult).isAbsent();
   }
 
   @Test
   public void testNoMethods_failure() throws Exception {
     thrown.expect(IllegalArgumentException.class, "Must specify at least one auth method");
+
     runTest(fakeUserService, AuthNoMethods.class);
   }
 
@@ -348,6 +421,7 @@ public class RequestAuthenticatorTest {
   public void testMissingInternal_failure() throws Exception {
     thrown.expect(IllegalArgumentException.class,
         "Auth method INTERNAL must always be specified, and as the first auth method");
+
     runTest(fakeUserService, AuthMissingInternal.class);
   }
 
@@ -355,6 +429,7 @@ public class RequestAuthenticatorTest {
   public void testWrongMethodOrdering_failure() throws Exception {
     thrown.expect(IllegalArgumentException.class,
         "Auth methods must be unique and strictly in order - INTERNAL, API, LEGACY");
+
     runTest(fakeUserService, AuthWrongMethodOrdering.class);
   }
 
@@ -362,6 +437,7 @@ public class RequestAuthenticatorTest {
   public void testDuplicateMethods_failure() throws Exception {
     thrown.expect(IllegalArgumentException.class,
         "Auth methods must be unique and strictly in order - INTERNAL, API, LEGACY");
+
     runTest(fakeUserService, AuthDuplicateMethods.class);
   }
 
@@ -369,6 +445,7 @@ public class RequestAuthenticatorTest {
   public void testInternalWithUser_failure() throws Exception {
     thrown.expect(IllegalArgumentException.class,
         "Actions with only INTERNAL auth may not require USER auth level");
+
     runTest(fakeUserService, AuthInternalWithUser.class);
   }
 
@@ -376,6 +453,7 @@ public class RequestAuthenticatorTest {
   public void testWronglyIgnoringUser_failure() throws Exception {
     thrown.expect(IllegalArgumentException.class,
         "Actions with auth methods beyond INTERNAL must not specify the IGNORED user policy");
+
     runTest(fakeUserService, AuthWronglyIgnoringUser.class);
   }
 }

javatests/google/registry/security/XsrfTokenManagerTest.java

@@ -54,7 +54,9 @@ public class XsrfTokenManagerTest {
 
   @Test
   public void testSuccess() {
-    assertThat(xsrfTokenManager.validateToken(xsrfTokenManager.generateToken("console"), "console"))
+    assertThat(
+            xsrfTokenManager.validateToken(
+                xsrfTokenManager.generateTokenWithCurrentUser("console"), "console"))
         .isTrue();
   }
 
@@ -70,7 +72,7 @@ public class XsrfTokenManagerTest {
 
   @Test
   public void testExpired() {
-    String token = xsrfTokenManager.generateToken("console");
+    String token = xsrfTokenManager.generateTokenWithCurrentUser("console");
     clock.setTo(START_OF_TIME.plusDays(2));
     assertThat(xsrfTokenManager.validateToken(token, "console")).isFalse();
   }
@@ -78,7 +80,9 @@ public class XsrfTokenManagerTest {
   @Test
   public void testTimestampTamperedWith() {
     String encodedPart =
-        Splitter.on(':').splitToList(xsrfTokenManager.generateToken("console")).get(0);
+        Splitter.on(':')
+            .splitToList(xsrfTokenManager.generateTokenWithCurrentUser("console"))
+            .get(0);
     long tamperedTimestamp = clock.nowUtc().plusMillis(1).getMillis();
     assertThat(xsrfTokenManager.validateToken(encodedPart + ":" + tamperedTimestamp, "console"))
         .isFalse();
@@ -93,7 +97,26 @@ public class XsrfTokenManagerTest {
 
   @Test
   public void testDifferentScope() {
-    assertThat(xsrfTokenManager.validateToken(xsrfTokenManager.generateToken("console"), "foobar"))
+    assertThat(
+            xsrfTokenManager.validateToken(
+                xsrfTokenManager.generateTokenWithCurrentUser("console"), "foobar"))
         .isFalse();
   }
+
+  @Test
+  public void testNullScope() {
+    assertThat(
+            xsrfTokenManager.validateToken(
+                xsrfTokenManager.generateTokenWithCurrentUser(null), null))
+        .isTrue();
+  }
+
+  // This test checks that the server side will pass when we switch the client to use a null scope.
+  @Test
+  public void testNullScopePassesWhenTestedWithNonNullScope() {
+    assertThat(
+            xsrfTokenManager.validateToken(
+                xsrfTokenManager.generateTokenWithCurrentUser(null), "console"))
+        .isTrue();
+  }
 }

javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java

@@ -113,7 +113,7 @@ public class RegistrarSettingsActionTestCase {
     when(req.getMethod()).thenReturn("POST");
     when(rsp.getWriter()).thenReturn(new PrintWriter(writer));
     when(req.getContentType()).thenReturn("application/json");
-    when(req.getHeader(eq("X-CSRF-Token"))).thenReturn(xsrfTokenManager.generateToken("console"));
+    when(req.getHeader(eq("X-CSRF-Token"))).thenReturn(xsrfTokenManager.generateTokenWithCurrentUser("console"));
     when(req.getReader()).thenReturn(createJsonPayload(ImmutableMap.of("op", "read")));
     when(sessionUtils.isLoggedIn()).thenReturn(true);
     when(sessionUtils.checkRegistrarConsoleLogin(req)).thenReturn(true);