diff --git a/java/google/registry/request/auth/LegacyAuthenticationMechanism.java b/java/google/registry/request/auth/LegacyAuthenticationMechanism.java index 6b1d3f50d..a33018af8 100644 --- a/java/google/registry/request/auth/LegacyAuthenticationMechanism.java +++ b/java/google/registry/request/auth/LegacyAuthenticationMechanism.java @@ -14,9 +14,11 @@ package google.registry.request.auth; +import static com.google.common.base.Strings.emptyToNull; import static com.google.common.base.Strings.nullToEmpty; import static google.registry.request.auth.AuthLevel.NONE; import static google.registry.request.auth.AuthLevel.USER; +import static google.registry.security.XsrfTokenManager.P_CSRF_TOKEN; import static google.registry.security.XsrfTokenManager.X_CSRF_TOKEN; import com.google.appengine.api.users.UserService; @@ -52,8 +54,7 @@ public class LegacyAuthenticationMechanism implements AuthenticationMechanism { return AuthResult.create(NONE); } - if (!SAFE_METHODS.contains(request.getMethod()) - && !xsrfTokenManager.validateToken(nullToEmpty(request.getHeader(X_CSRF_TOKEN)))) { + if (!SAFE_METHODS.contains(request.getMethod()) && !validateXsrf(request)) { return AuthResult.create(NONE); } @@ -61,4 +62,27 @@ public class LegacyAuthenticationMechanism implements AuthenticationMechanism { USER, UserAuthInfo.create(userService.getCurrentUser(), userService.isUserAdmin())); } + + private boolean validateXsrf(HttpServletRequest request) { + String headerToken = emptyToNull(request.getHeader(X_CSRF_TOKEN)); + if (headerToken != null) { + return xsrfTokenManager.validateToken(headerToken); + } + // If we got here - the header didn't have the token. + // It might be in the POST data - however even checking whether the POST data has this entry + // could break the Action! + // + // Reason: if we do request.getParameter, any Action that injects @Payload or @JsonPayload + // would break since it uses request.getReader - and it's an error to call both getReader and + // getParameter! + // + // However, in this case it's acceptable since if we got here - the POST request didn't even + // have the XSRF header meaning if it doesn't have POST data - it's not from a valid source at + // all (a valid but outdated source would have a bad header value, but getting here means we had + // no value at all) + // + // TODO(b/120201577): Once we know from the @Action whether we can use getParameter or not - + // only check getParameter if that's how this @Action uses getParameters. + return xsrfTokenManager.validateToken(nullToEmpty(request.getParameter(P_CSRF_TOKEN))); + } } diff --git a/java/google/registry/security/XsrfTokenManager.java b/java/google/registry/security/XsrfTokenManager.java index 5681ddeb2..21318fcab 100644 --- a/java/google/registry/security/XsrfTokenManager.java +++ b/java/google/registry/security/XsrfTokenManager.java @@ -37,6 +37,9 @@ public final class XsrfTokenManager { /** HTTP header used for transmitting XSRF tokens. */ public static final String X_CSRF_TOKEN = "X-CSRF-Token"; + /** POST parameter used for transmitting XSRF tokens. */ + public static final String P_CSRF_TOKEN = "xsrfToken"; + /** Maximum age of an acceptable XSRF token. */ private static final Duration XSRF_VALIDITY = Duration.standardDays(1); diff --git a/javatests/google/registry/request/auth/LegacyAuthenticationMechanismTest.java b/javatests/google/registry/request/auth/LegacyAuthenticationMechanismTest.java new file mode 100644 index 000000000..738bf08e3 --- /dev/null +++ b/javatests/google/registry/request/auth/LegacyAuthenticationMechanismTest.java @@ -0,0 +1,173 @@ +// Copyright 2018 The Nomulus Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package google.registry.request.auth; + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.atLeast; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; + +import com.google.appengine.api.users.User; +import com.google.appengine.api.users.UserService; +import google.registry.security.XsrfTokenManager; +import google.registry.testing.AppEngineRule; +import google.registry.testing.FakeClock; +import google.registry.testing.MockitoJUnitRule; +import javax.servlet.http.HttpServletRequest; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.mockito.Mock; + +@RunWith(JUnit4.class) +public final class LegacyAuthenticationMechanismTest { + + + @Rule + public final AppEngineRule appEngine = AppEngineRule.builder() + .withDatastore() + .build(); + + @Rule public final MockitoJUnitRule mocks = MockitoJUnitRule.create(); + + @Mock private UserService userService; + @Mock private HttpServletRequest req; + + private final FakeClock clock = new FakeClock(); + private XsrfTokenManager xsrfTokenManager; + private LegacyAuthenticationMechanism legacyAuthenticationMechanism; + private String goodToken; + + @Before + public void setUp() { + xsrfTokenManager = new XsrfTokenManager(clock, userService); + legacyAuthenticationMechanism = + new LegacyAuthenticationMechanism(userService, xsrfTokenManager); + when(userService.getCurrentUser()).thenReturn(new User("email@example.com", "example.com")); + when(userService.isUserAdmin()).thenReturn(false); + goodToken = xsrfTokenManager.generateToken("email@example.com"); + } + + @After + public void tearDown() { + // Make sure we didn't use getParameter or getInputStream or any of the other "with side + // effects" getters unexpectedly. But allow "no side effect" getters. + // + // Unfortunately HttpServletRequest doesn't document well which getters "have side effects". It + // does explicitly state getReader and getInputStream, and that getParameter can also interfere + // with them, but it doesn't say anything about getParameterNames, getParameterValues, + // getParameterMap - even though I'm pretty sure they are similar to getParameter in that + // effect. + // + // Feel free to add other "no side effect" functions with atLeast(0) to exempt them from the + // verifyNoMoreInteractions + verify(req, atLeast(0)).getMethod(); + verify(req, atLeast(0)).getHeader(any()); + verifyNoMoreInteractions(req); + } + + @Test + public void testAuthenticate_notLoggedIn() { + when(userService.isUserLoggedIn()).thenReturn(false); + assertThat(legacyAuthenticationMechanism.authenticate(req).authLevel()) + .isEqualTo(AuthLevel.NONE); + } + + @Test + public void testAuthenticate_loggedInSafeMethod_get() { + when(userService.isUserLoggedIn()).thenReturn(true); + when(req.getMethod()).thenReturn("GET"); + assertThat(legacyAuthenticationMechanism.authenticate(req).authLevel()) + .isEqualTo(AuthLevel.USER); + } + + @Test + public void testAuthenticate_loggedInSafeMethod_head() { + when(userService.isUserLoggedIn()).thenReturn(true); + when(req.getMethod()).thenReturn("HEAD"); + assertThat(legacyAuthenticationMechanism.authenticate(req).authLevel()) + .isEqualTo(AuthLevel.USER); + } + + @Test + public void testAuthenticate_loggedInUnsafeMethod_post_noXsrfToken() { + when(userService.isUserLoggedIn()).thenReturn(true); + when(req.getMethod()).thenReturn("POST"); + assertThat(legacyAuthenticationMechanism.authenticate(req).authLevel()) + .isEqualTo(AuthLevel.NONE); + + // Make sure we looked for the token in all relevant places before giving up + verify(req).getHeader("X-CSRF-Token"); + verify(req).getParameter("xsrfToken"); + } + + @Test + public void testAuthenticate_loggedInUnsafeMethod_post_goodTokenInHeader() { + when(userService.isUserLoggedIn()).thenReturn(true); + when(req.getMethod()).thenReturn("POST"); + when(req.getHeader("X-CSRF-Token")).thenReturn(goodToken); + assertThat(legacyAuthenticationMechanism.authenticate(req).authLevel()) + .isEqualTo(AuthLevel.USER); + + // Make sure we didn't call getParameter (we already verify it in the @After, but we're doing it + // here explicitly as well for clarity, since this is important in this test) + verify(req, times(0)).getParameter(any()); + } + + @Test + public void testAuthenticate_loggedInUnsafeMethod_post_badTokenInHeader() { + when(userService.isUserLoggedIn()).thenReturn(true); + when(req.getMethod()).thenReturn("POST"); + when(req.getHeader("X-CSRF-Token")).thenReturn("bad"); + assertThat(legacyAuthenticationMechanism.authenticate(req).authLevel()) + .isEqualTo(AuthLevel.NONE); + + // Make sure we didn't call getParameter (we already verify it in the @After, but we're doing it + // here explicitly as well for clarity, since this is important in this test) + verify(req, times(0)).getParameter(any()); + } + + @Test + public void testAuthenticate_loggedInUnsafeMethod_post_goodTokenInParam() { + when(userService.isUserLoggedIn()).thenReturn(true); + when(req.getMethod()).thenReturn("POST"); + when(req.getParameter("xsrfToken")).thenReturn(goodToken); + assertThat(legacyAuthenticationMechanism.authenticate(req).authLevel()) + .isEqualTo(AuthLevel.USER); + + // we allow getParameter to be called in this case (we verify it so it's not caught in the + // @After's verifyNoMoreInteractions) + verify(req).getParameter("xsrfToken"); + } + + @Test + public void testAuthenticate_loggedInUnsafeMethod_post_badTokenInParam() { + when(userService.isUserLoggedIn()).thenReturn(true); + when(req.getMethod()).thenReturn("POST"); + when(req.getParameter("xsrfToken")).thenReturn("bad"); + assertThat(legacyAuthenticationMechanism.authenticate(req).authLevel()) + .isEqualTo(AuthLevel.NONE); + + // we allow getParameter to be called in this case (we verify it so it's not caught in the + // @After's verifyNoMoreInteractions) + verify(req).getParameter("xsrfToken"); + } +}