diff --git a/core/src/main/java/google/registry/request/auth/AppEngineInternalAuthenticationMechanism.java b/core/src/main/java/google/registry/request/auth/AppEngineInternalAuthenticationMechanism.java index af79a48b739..35b44e26627 100644 --- a/core/src/main/java/google/registry/request/auth/AppEngineInternalAuthenticationMechanism.java +++ b/core/src/main/java/google/registry/request/auth/AppEngineInternalAuthenticationMechanism.java @@ -17,6 +17,7 @@ import static google.registry.request.auth.AuthLevel.APP; import static google.registry.request.auth.AuthLevel.NONE; +import com.google.appengine.api.users.UserService; import javax.inject.Inject; import javax.servlet.http.HttpServletRequest; @@ -24,29 +25,15 @@ * Authentication mechanism which uses the X-AppEngine-QueueName header set by App Engine for * internal requests. * - *

- * Task queue push task requests set this header value to the actual queue name. Cron requests set - * this header value to __cron, since that's actually the name of the hidden queue used for cron + *

Task queue push task requests set this header value to the actual queue name. Cron requests + * set this header value to __cron, since that's actually the name of the hidden queue used for cron * requests. Cron also sets the header X-AppEngine-Cron, which we could check, but it's simpler just * to check the one. * - *

- * App Engine allows app admins to set these headers for testing purposes. This means that this auth - * method is somewhat unreliable - any app admin can access any internal endpoint and pretend to be - * the app itself by setting these headers, which would circumvent any finer-grained authorization - * if we added it in the future (assuming we did not apply it to the app itself). And App Engine's - * concept of an "admin" includes all project owners, editors and viewers. So anyone with access to - * the project will be able to access anything the app itself can access. - * - *

- * For now, it's probably okay to allow this behavior, especially since it could indeed be - * convenient for testing. If we wanted to revisit this decision in the future, we have a couple - * options for locking this down: - * - *

+ *

App Engine allows app admins to set these headers for testing purposes. Because of this, we + * also need to verify that the current user is null, indicating that there is no user, to prevent + * access by someone with "admin" privileges. If the user is an admin, UserService presumably must + * return a User object. * *

See task @@ -57,12 +44,16 @@ public class AppEngineInternalAuthenticationMechanism implements AuthenticationM // As defined in the App Engine request header documentation. private static final String QUEUE_NAME_HEADER = "X-AppEngine-QueueName"; + private UserService userService; + @Inject - public AppEngineInternalAuthenticationMechanism() {} + public AppEngineInternalAuthenticationMechanism(UserService userService) { + this.userService = userService; + } @Override public AuthResult authenticate(HttpServletRequest request) { - if (request.getHeader(QUEUE_NAME_HEADER) == null) { + if (request.getHeader(QUEUE_NAME_HEADER) == null || userService.getCurrentUser() != null) { return AuthResult.create(NONE); } else { return AuthResult.create(APP); diff --git a/core/src/test/java/google/registry/request/auth/RequestAuthenticatorTest.java b/core/src/test/java/google/registry/request/auth/RequestAuthenticatorTest.java index 0cb443cbdaa..5211a67a811 100644 --- a/core/src/test/java/google/registry/request/auth/RequestAuthenticatorTest.java +++ b/core/src/test/java/google/registry/request/auth/RequestAuthenticatorTest.java @@ -117,7 +117,7 @@ void beforeEach() { private RequestAuthenticator createRequestAuthenticator(UserService userService) { return new RequestAuthenticator( - new AppEngineInternalAuthenticationMechanism(), + new AppEngineInternalAuthenticationMechanism(fakeUserService), ImmutableList.of( new OAuthAuthenticationMechanism( fakeOAuthService, @@ -173,6 +173,16 @@ void testInternalAuth_success() { assertThat(authResult.get().userAuthInfo()).isEmpty(); } + @Test + void testInternalAuth_failForAdminUser() { + when(req.getHeader("X-AppEngine-QueueName")).thenReturn("__cron"); + fakeUserService.setUser(testUser, true /* isAdmin */); + + Optional authResult = runTest(fakeUserService, AUTH_INTERNAL_OR_ADMIN); + + assertThat(authResult).isEmpty(); + } + @Test void testAnyUserAnyMethod_notLoggedIn() { Optional authResult = runTest(fakeUserService, AUTH_ANY_USER_ANY_METHOD);