From 5127aeafb576f43acdc28ef744260ffe6d7cc51c Mon Sep 17 00:00:00 2001 From: mountford Date: Thu, 6 Apr 2017 07:59:15 -0700 Subject: [PATCH] Enable authentication/authorization checks The code to authenticate and authorize incoming requests (including via OAuth) has been in the system. This CL actually turns it on, since we are satisfied from logging information that it is not unjustly denying access. Auth settings are also updated on a few commands missed earlier. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=152381820 --- .../registry/request/RequestHandler.java | 5 ++- .../tools/server/GenerateZoneFilesAction.java | 11 ++++-- .../tools/server/KillAllCommitLogsAction.java | 9 ++++- .../server/KillAllEppResourcesAction.java | 9 ++++- .../server/ResaveAllEppResourcesAction.java | 4 +++ .../javascrap/RefreshAllDomainsAction.java | 8 ++++- .../registry/request/RequestHandlerTest.java | 36 ++----------------- 7 files changed, 40 insertions(+), 42 deletions(-) diff --git a/java/google/registry/request/RequestHandler.java b/java/google/registry/request/RequestHandler.java index bcd2d3763..eba0bd11e 100644 --- a/java/google/registry/request/RequestHandler.java +++ b/java/google/registry/request/RequestHandler.java @@ -171,9 +171,8 @@ public class RequestHandler { Optional authResult = requestAuthenticator.authorize(route.get().action().auth(), req); if (!authResult.isPresent()) { - logger.warning("Request would not have been authorized"); - // TODO(b/28219927): Change this to call rsp.sendError(SC_FORBIDDEN) and return - authResult = Optional.of(AuthResult.NOT_AUTHENTICATED); + rsp.sendError(SC_FORBIDDEN, "Not authorized"); + return; } // Build a new request component using any modules we've constructed by this point. diff --git a/java/google/registry/tools/server/GenerateZoneFilesAction.java b/java/google/registry/tools/server/GenerateZoneFilesAction.java index 86cdcc33e..584b3e7e5 100644 --- a/java/google/registry/tools/server/GenerateZoneFilesAction.java +++ b/java/google/registry/tools/server/GenerateZoneFilesAction.java @@ -47,6 +47,8 @@ import google.registry.model.host.HostResource; import google.registry.request.Action; import google.registry.request.HttpException.BadRequestException; import google.registry.request.JsonActionRunner; +import google.registry.request.auth.Auth; +import google.registry.request.auth.AuthLevel; import google.registry.util.Clock; import java.io.IOException; import java.io.OutputStream; @@ -72,8 +74,13 @@ import org.joda.time.Duration; @Action( path = GenerateZoneFilesAction.PATH, method = POST, - xsrfProtection = true, - xsrfScope = "admin") + auth = + @Auth( + methods = {Auth.AuthMethod.INTERNAL, Auth.AuthMethod.API}, + minimumLevel = AuthLevel.APP, + userPolicy = Auth.UserPolicy.ADMIN + ) +) public class GenerateZoneFilesAction implements Runnable, JsonActionRunner.JsonAction { public static final String PATH = "/_dr/task/generateZoneFiles"; diff --git a/java/google/registry/tools/server/KillAllCommitLogsAction.java b/java/google/registry/tools/server/KillAllCommitLogsAction.java index 51a7d63a2..9012a8d51 100644 --- a/java/google/registry/tools/server/KillAllCommitLogsAction.java +++ b/java/google/registry/tools/server/KillAllCommitLogsAction.java @@ -35,7 +35,14 @@ import google.registry.request.Response; import java.util.Arrays; import javax.inject.Inject; -/** Deletes all commit logs in Datastore. */ +/** + * Deletes all commit logs in Datastore. + * + *

Because there are no auth settings in the {@link Action} annotation, this command can only be + * run internally, or by pretending to be internal by setting the X-AppEngine-QueueName header, + * which only admin users can do. That makes this command hard to use, which is appropriate, given + * the drastic consequences of accidental execution. + */ @Action(path = "/_dr/task/killAllCommitLogs", method = POST) public class KillAllCommitLogsAction implements Runnable { diff --git a/java/google/registry/tools/server/KillAllEppResourcesAction.java b/java/google/registry/tools/server/KillAllEppResourcesAction.java index 3924ab4b9..01240c25e 100644 --- a/java/google/registry/tools/server/KillAllEppResourcesAction.java +++ b/java/google/registry/tools/server/KillAllEppResourcesAction.java @@ -35,7 +35,14 @@ import google.registry.request.Action; import google.registry.request.Response; import javax.inject.Inject; -/** Deletes all {@link EppResource} objects in Datastore, including indices and descendants. */ +/** + * Deletes all {@link EppResource} objects in Datastore, including indices and descendants. + * + *

Because there are no auth settings in the {@link Action} annotation, this command can only be + * run internally, or by pretending to be internal by setting the X-AppEngine-QueueName header, + * which only admin users can do. That makes this command hard to use, which is appropriate, given + * the drastic consequences of accidental execution. + */ @Action(path = "/_dr/task/killAllEppResources", method = POST) public class KillAllEppResourcesAction implements Runnable { diff --git a/java/google/registry/tools/server/ResaveAllEppResourcesAction.java b/java/google/registry/tools/server/ResaveAllEppResourcesAction.java index 864839680..68bd149e2 100644 --- a/java/google/registry/tools/server/ResaveAllEppResourcesAction.java +++ b/java/google/registry/tools/server/ResaveAllEppResourcesAction.java @@ -34,6 +34,10 @@ import javax.inject.Inject; *

This is useful for completing data migrations on EppResource fields that are accomplished * with @OnSave or @OnLoad annotations, and also guarantees that all EppResources will get fresh * commit logs (for backup purposes). + * + *

Because there are no auth settings in the {@link Action} annotation, this command can only be + * run internally, or by pretending to be internal by setting the X-AppEngine-QueueName header, + * which only admin users can do. */ @Action(path = "/_dr/task/resaveAllEppResources") public class ResaveAllEppResourcesAction implements Runnable { diff --git a/java/google/registry/tools/server/javascrap/RefreshAllDomainsAction.java b/java/google/registry/tools/server/javascrap/RefreshAllDomainsAction.java index b9773dc86..c11a50559 100644 --- a/java/google/registry/tools/server/javascrap/RefreshAllDomainsAction.java +++ b/java/google/registry/tools/server/javascrap/RefreshAllDomainsAction.java @@ -30,7 +30,13 @@ import javax.inject.Inject; import org.joda.time.DateTime; import org.joda.time.DateTimeZone; -/** A mapreduce that enqueues publish tasks on all active domains. */ +/** + * A mapreduce that enqueues publish tasks on all active domains. + * + *

Because there are no auth settings in the {@link Action} annotation, this command can only be + * run internally, or by pretending to be internal by setting the X-AppEngine-QueueName header, + * which only admin users can do. + */ @Action(path = "/_dr/task/refreshAllDomains") public class RefreshAllDomainsAction implements Runnable { diff --git a/javatests/google/registry/request/RequestHandlerTest.java b/javatests/google/registry/request/RequestHandlerTest.java index e07c9463a..6f65e20b4 100644 --- a/javatests/google/registry/request/RequestHandlerTest.java +++ b/javatests/google/registry/request/RequestHandlerTest.java @@ -48,7 +48,6 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.junit.After; import org.junit.Before; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -458,8 +457,6 @@ public final class RequestHandlerTest { assertThat(providedAuthResult.userAuthInfo()).isAbsent(); } - // TODO(b/28219927): turn this on once we actually do authorization - @Ignore @Test public void testAuthNeeded_notLoggedIn() throws Exception { when(req.getMethod()).thenReturn("GET"); @@ -467,26 +464,11 @@ public final class RequestHandlerTest { handler.handleRequest(req, rsp); - verify(rsp).sendError(403); + verify(rsp).sendError(403, "Not authorized"); assertThat(providedAuthResult).isNull(); assertThat(providedAuthResult).isNull(); } - // TODO(b/28219927): remove this once we actually do authorization - @Test - public void testAuthNeeded_notLoggedIn_interim() throws Exception { - when(req.getMethod()).thenReturn("GET"); - when(req.getRequestURI()).thenReturn("/auth/adminUserAnyMethod"); - - handler.handleRequest(req, rsp); - - assertThat(providedAuthResult).isNotNull(); - assertThat(providedAuthResult.authLevel()).isEqualTo(AuthLevel.NONE); - assertThat(providedAuthResult.userAuthInfo()).isAbsent(); - } - - // TODO(b/28219927): turn this on once we actually do authorization - @Ignore @Test public void testAuthNeeded_notAuthorized() throws Exception { userService.setUser(testUser, false); @@ -495,24 +477,10 @@ public final class RequestHandlerTest { handler.handleRequest(req, rsp); - verify(rsp).sendError(403); + verify(rsp).sendError(403, "Not authorized"); assertThat(providedAuthResult).isNull(); } - // TODO(b/28219927): remove this once we actually do authorization - @Test - public void testAuthNeeded_notAuthorized_interim() throws Exception { - userService.setUser(testUser, false); - when(req.getMethod()).thenReturn("GET"); - when(req.getRequestURI()).thenReturn("/auth/adminUserAnyMethod"); - - handler.handleRequest(req, rsp); - - assertThat(providedAuthResult).isNotNull(); - assertThat(providedAuthResult.authLevel()).isEqualTo(AuthLevel.NONE); - assertThat(providedAuthResult.userAuthInfo()).isAbsent(); - } - @Test public void testAuthNeeded_success() throws Exception { userService.setUser(testUser, true);