From 726e925b4af1469188a64b00416f0e585dea3ea0 Mon Sep 17 00:00:00 2001 From: nickfelt Date: Tue, 28 Feb 2017 11:17:35 -0800 Subject: [PATCH] Refactor a few new XsrfTokenManager methods Followup to comments on [] ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=148792464 --- .../registry/security/XsrfTokenManager.java | 26 ++-------- .../registry/request/RequestHandlerTest.java | 4 +- .../auth/RequestAuthenticatorTest.java | 4 +- .../security/XsrfTokenManagerTest.java | 50 +++++++------------ .../RegistrarSettingsActionTestCase.java | 5 -- 5 files changed, 28 insertions(+), 61 deletions(-) diff --git a/java/google/registry/security/XsrfTokenManager.java b/java/google/registry/security/XsrfTokenManager.java index 4d9796c12..653069fea 100644 --- a/java/google/registry/security/XsrfTokenManager.java +++ b/java/google/registry/security/XsrfTokenManager.java @@ -68,24 +68,13 @@ public final class XsrfTokenManager { .asBytes()); } - /** - * Generate an xsrf token for a given scope using the email of the logged in user or else no user. - * - *

If there is no user, the entire xsrf check becomes basically a no-op, but that's ok because - * any callback that doesn't have a user shouldn't be able to access any per-user resources - * anyways. - * - *

The scope (or lack thereof) is passed to {@link #encodeToken}. Use of a scope in xsrf tokens - * is deprecated; instead, use the no-argument version. - */ - @Deprecated - public String generateTokenWithCurrentUser(@Nullable String scope) { - return generateTokenSub(scope, getLoggedInEmailOrEmpty()); - } - /** * Generate an xsrf token for a given scope and user. * + *

If there is no user (email is an empty string), the entire xsrf check becomes basically a + * no-op, but that's ok because any callback that doesn't have a user shouldn't be able to access + * any per-user resources anyways. + * *

The scope (or lack thereof) is passed to {@link #encodeToken}. Use of a scope in xsrf tokens * is deprecated; instead, use the no-argument version. */ @@ -97,18 +86,13 @@ public final class XsrfTokenManager { /** Generate an xsrf token for a given user. */ public String generateToken(String email) { - return generateTokenSub(null, email); + return generateToken(null, email); } private String getLoggedInEmailOrEmpty() { return userService.isUserLoggedIn() ? userService.getCurrentUser().getEmail() : ""; } - private String generateTokenSub(@Nullable String scope, String email) { - long now = clock.nowUtc().getMillis(); - return Joiner.on(':').join(encodeToken(now, scope, email), now); - } - /** * Validate an xsrf token, given the scope it was used for. * diff --git a/javatests/google/registry/request/RequestHandlerTest.java b/javatests/google/registry/request/RequestHandlerTest.java index e04e13287..cb5ff6ffe 100644 --- a/javatests/google/registry/request/RequestHandlerTest.java +++ b/javatests/google/registry/request/RequestHandlerTest.java @@ -383,7 +383,7 @@ public final class RequestHandlerTest { userService.setUser(testUser, false); when(req.getMethod()).thenReturn("POST"); when(req.getHeader("X-CSRF-Token")) - .thenReturn(xsrfTokenManager.generateTokenWithCurrentUser("vampire")); + .thenReturn(xsrfTokenManager.generateToken("vampire", testUser.getEmail())); when(req.getRequestURI()).thenReturn("/safe-sloth"); handler.handleRequest(req, rsp); verify(safeSlothTask).run(); @@ -394,7 +394,7 @@ public final class RequestHandlerTest { userService.setUser(testUser, false); when(req.getMethod()).thenReturn("POST"); when(req.getHeader("X-CSRF-Token")) - .thenReturn(xsrfTokenManager.generateTokenWithCurrentUser("blood")); + .thenReturn(xsrfTokenManager.generateToken("blood", testUser.getEmail())); when(req.getRequestURI()).thenReturn("/safe-sloth"); handler.handleRequest(req, rsp); verify(rsp).sendError(403, "Invalid X-CSRF-Token"); diff --git a/javatests/google/registry/request/auth/RequestAuthenticatorTest.java b/javatests/google/registry/request/auth/RequestAuthenticatorTest.java index 735df8096..da05cb5e7 100644 --- a/javatests/google/registry/request/auth/RequestAuthenticatorTest.java +++ b/javatests/google/registry/request/auth/RequestAuthenticatorTest.java @@ -220,7 +220,7 @@ public class RequestAuthenticatorTest { public void testAnyUserAnyMethod_success() throws Exception { fakeUserService.setUser(testUser, false /* isAdmin */); when(req.getHeader(XsrfTokenManager.X_CSRF_TOKEN)) - .thenReturn(xsrfTokenManager.generateTokenWithCurrentUser("console")); + .thenReturn(xsrfTokenManager.generateToken("console", testUser.getEmail())); Optional authResult = runTest(fakeUserService, AuthAnyUserAnyMethod.class); @@ -275,7 +275,7 @@ public class RequestAuthenticatorTest { public void testAdminUserAnyMethod_success() throws Exception { fakeUserService.setUser(testUser, true /* isAdmin */); when(req.getHeader(XsrfTokenManager.X_CSRF_TOKEN)) - .thenReturn(xsrfTokenManager.generateTokenWithCurrentUser("console")); + .thenReturn(xsrfTokenManager.generateToken("console", testUser.getEmail())); Optional authResult = runTest(fakeUserService, AuthAdminUserAnyMethod.class); diff --git a/javatests/google/registry/security/XsrfTokenManagerTest.java b/javatests/google/registry/security/XsrfTokenManagerTest.java index efbbd02bd..b9660f520 100644 --- a/javatests/google/registry/security/XsrfTokenManagerTest.java +++ b/javatests/google/registry/security/XsrfTokenManagerTest.java @@ -17,12 +17,12 @@ package google.registry.security; import static com.google.common.truth.Truth.assertThat; import static google.registry.util.DateTimeUtils.START_OF_TIME; +import com.google.appengine.api.users.User; import com.google.common.base.Splitter; import google.registry.testing.AppEngineRule; import google.registry.testing.FakeClock; import google.registry.testing.FakeUserService; import google.registry.testing.InjectRule; -import google.registry.testing.UserInfo; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -33,31 +33,30 @@ import org.mockito.runners.MockitoJUnitRunner; @RunWith(MockitoJUnitRunner.class) public class XsrfTokenManagerTest { - FakeClock clock = new FakeClock(START_OF_TIME); - FakeUserService userService = new FakeUserService(); - - XsrfTokenManager xsrfTokenManager = new XsrfTokenManager(clock, userService); - @Rule public final AppEngineRule appEngine = AppEngineRule.builder() .withDatastore() - .withUserService(UserInfo.createAdmin("a@example.com", "user1")) .build(); @Rule public InjectRule inject = new InjectRule(); + private final User testUser = new User("test@example.com", "test@example.com"); + private final FakeClock clock = new FakeClock(START_OF_TIME); + private final FakeUserService userService = new FakeUserService(); + private final XsrfTokenManager xsrfTokenManager = new XsrfTokenManager(clock, userService); + + String realToken; + @Before public void init() { - // inject.setStaticField(XsrfTokenManager.class, "clock", clock); + userService.setUser(testUser, false); + realToken = xsrfTokenManager.generateToken("console", testUser.getEmail()); } @Test public void testSuccess() { - assertThat( - xsrfTokenManager.validateToken( - xsrfTokenManager.generateTokenWithCurrentUser("console"), "console")) - .isTrue(); + assertThat(xsrfTokenManager.validateToken(realToken, "console")).isTrue(); } @Test @@ -72,17 +71,13 @@ public class XsrfTokenManagerTest { @Test public void testExpired() { - String token = xsrfTokenManager.generateTokenWithCurrentUser("console"); clock.setTo(START_OF_TIME.plusDays(2)); - assertThat(xsrfTokenManager.validateToken(token, "console")).isFalse(); + assertThat(xsrfTokenManager.validateToken(realToken, "console")).isFalse(); } @Test public void testTimestampTamperedWith() { - String encodedPart = - Splitter.on(':') - .splitToList(xsrfTokenManager.generateTokenWithCurrentUser("console")) - .get(0); + String encodedPart = Splitter.on(':').splitToList(realToken).get(0); long tamperedTimestamp = clock.nowUtc().plusMillis(1).getMillis(); assertThat(xsrfTokenManager.validateToken(encodedPart + ":" + tamperedTimestamp, "console")) .isFalse(); @@ -91,32 +86,25 @@ public class XsrfTokenManagerTest { @Test public void testDifferentUser() { assertThat(xsrfTokenManager - .validateToken(xsrfTokenManager.generateToken("console", "b@example.com"), "console")) + .validateToken(xsrfTokenManager.generateToken("console", "eve@example.com"), "console")) .isFalse(); } @Test public void testDifferentScope() { - assertThat( - xsrfTokenManager.validateToken( - xsrfTokenManager.generateTokenWithCurrentUser("console"), "foobar")) - .isFalse(); + assertThat(xsrfTokenManager.validateToken(realToken, "foobar")).isFalse(); } @Test public void testNullScope() { - assertThat( - xsrfTokenManager.validateToken( - xsrfTokenManager.generateTokenWithCurrentUser(null), null)) - .isTrue(); + String tokenWithNullScope = xsrfTokenManager.generateToken(null, testUser.getEmail()); + assertThat(xsrfTokenManager.validateToken(tokenWithNullScope, 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(); + String tokenWithNullScope = xsrfTokenManager.generateToken(null, testUser.getEmail()); + assertThat(xsrfTokenManager.validateToken(tokenWithNullScope, "console")).isTrue(); } } diff --git a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java index 1504dd3b5..f1e7ee1fc 100644 --- a/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java +++ b/javatests/google/registry/ui/server/registrar/RegistrarSettingsActionTestCase.java @@ -19,7 +19,6 @@ import static google.registry.config.RegistryConfig.getGSuiteOutgoingEmailDispla import static google.registry.security.JsonHttpTestUtils.createJsonPayload; import static google.registry.security.JsonHttpTestUtils.createJsonResponseSupplier; import static google.registry.util.ResourceUtils.readResourceUtf8; -import static org.mockito.Matchers.eq; import static org.mockito.Mockito.when; import com.google.appengine.api.modules.ModulesService; @@ -32,10 +31,8 @@ import google.registry.model.registrar.Registrar; import google.registry.request.JsonActionRunner; import google.registry.request.JsonResponse; import google.registry.request.ResponseImpl; -import google.registry.security.XsrfTokenManager; import google.registry.testing.AppEngineRule; import google.registry.testing.FakeClock; -import google.registry.testing.FakeUserService; import google.registry.testing.InjectRule; import google.registry.util.SendEmailService; import java.io.PrintWriter; @@ -92,7 +89,6 @@ public class RegistrarSettingsActionTestCase { final StringWriter writer = new StringWriter(); final Supplier> json = createJsonResponseSupplier(writer); final FakeClock clock = new FakeClock(DateTime.parse("2014-01-01T00:00:00Z")); - final XsrfTokenManager xsrfTokenManager = new XsrfTokenManager(clock, new FakeUserService()); @Before public void setUp() throws Exception { @@ -113,7 +109,6 @@ 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.generateTokenWithCurrentUser("console")); when(req.getReader()).thenReturn(createJsonPayload(ImmutableMap.of("op", "read"))); when(sessionUtils.isLoggedIn()).thenReturn(true); when(sessionUtils.checkRegistrarConsoleLogin(req)).thenReturn(true);