diff --git a/java/google/registry/ui/server/registrar/BUILD b/java/google/registry/ui/server/registrar/BUILD index 2723a052f..e815fa505 100644 --- a/java/google/registry/ui/server/registrar/BUILD +++ b/java/google/registry/ui/server/registrar/BUILD @@ -26,11 +26,13 @@ java_library( "//java/google/registry/util", "//third_party/objectify:objectify-v4_1", "@com_google_appengine_api_1_0_sdk", + "@com_google_auto_value", "@com_google_code_findbugs_jsr305", "@com_google_dagger", "@com_google_flogger", "@com_google_flogger_system_backend", "@com_google_guava", + "@com_google_monitoring_client_metrics", "@com_google_re2j", "@io_bazel_rules_closure//closure/templates", "@javax_inject", diff --git a/java/google/registry/ui/server/registrar/ConsoleUiAction.java b/java/google/registry/ui/server/registrar/ConsoleUiAction.java index 38e5b9c76..56842985a 100644 --- a/java/google/registry/ui/server/registrar/ConsoleUiAction.java +++ b/java/google/registry/ui/server/registrar/ConsoleUiAction.java @@ -75,6 +75,7 @@ public final class ConsoleUiAction implements Runnable { @Inject HttpServletRequest req; @Inject Response response; + @Inject RegistrarConsoleMetrics registrarConsoleMetrics; @Inject AuthenticatedRegistrarAccessor registrarAccessor; @Inject UserService userService; @Inject XsrfTokenManager xsrfTokenManager; @@ -134,15 +135,19 @@ public final class ConsoleUiAction implements Runnable { data.put("username", user.getNickname()); data.put("logoutUrl", userService.createLogoutURL(PATH)); data.put("xsrfToken", xsrfTokenManager.generateToken(user.getEmail())); + ImmutableSetMultimap roleMap = registrarAccessor.getAllClientIdWithRoles(); + ImmutableSetMultimap roleMapInverse = roleMap.inverse(); + // TODO(guyben):just return all the clientIDs in a single list, and add an "isAdmin" or "roles" + // item + data.put("ownerClientIds", roleMapInverse.get(OWNER)); + data.put( + "adminClientIds", Sets.difference(roleMapInverse.get(ADMIN), roleMapInverse.get(OWNER))); + // We set the initual value to the value that will show if guessClientId throws. + String clientId = ""; try { - String clientId = paramClientId.orElse(registrarAccessor.guessClientId()); + clientId = paramClientId.orElse(registrarAccessor.guessClientId()); data.put("clientId", clientId); - ImmutableSetMultimap roleMap = - registrarAccessor.getAllClientIdWithRoles().inverse(); - data.put("ownerClientIds", roleMap.get(OWNER)); - data.put("adminClientIds", Sets.difference(roleMap.get(ADMIN), roleMap.get(OWNER))); - // We want to load the registrar even if we won't use it later (even if we remove the // requireFeeExtension) - to make sure the user indeed has access to the guessed registrar. // @@ -162,7 +167,13 @@ public final class ConsoleUiAction implements Runnable { .setCssRenamingMap(CSS_RENAMING_MAP_SUPPLIER.get()) .setData(data) .render()); + registrarConsoleMetrics.registerConsoleRequest( + clientId, paramClientId.isPresent(), roleMap.get(clientId), "FORBIDDEN"); return; + } catch (Exception e) { + registrarConsoleMetrics.registerConsoleRequest( + clientId, paramClientId.isPresent(), roleMap.get(clientId), "UNEXPECTED ERROR"); + throw e; } String payload = TOFU_SUPPLIER.get() @@ -171,5 +182,7 @@ public final class ConsoleUiAction implements Runnable { .setData(data) .render(); response.setPayload(payload); + registrarConsoleMetrics.registerConsoleRequest( + clientId, paramClientId.isPresent(), roleMap.get(clientId), "SUCCESS"); } } diff --git a/java/google/registry/ui/server/registrar/RegistrarConsoleMetrics.java b/java/google/registry/ui/server/registrar/RegistrarConsoleMetrics.java new file mode 100644 index 000000000..338a595e4 --- /dev/null +++ b/java/google/registry/ui/server/registrar/RegistrarConsoleMetrics.java @@ -0,0 +1,69 @@ +// 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.ui.server.registrar; + +import com.google.common.collect.ImmutableSet; +import com.google.monitoring.metrics.IncrementableMetric; +import com.google.monitoring.metrics.LabelDescriptor; +import com.google.monitoring.metrics.MetricRegistryImpl; +import google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role; +import javax.inject.Inject; + +final class RegistrarConsoleMetrics { + + private static final ImmutableSet CONSOLE_LABEL_DESCRIPTORS = + ImmutableSet.of( + LabelDescriptor.create("clientId", "target registrar client ID"), + LabelDescriptor.create("explicitClientId", "whether the client ID is set explicitly"), + LabelDescriptor.create("role", "Role[s] of the user making the request"), + LabelDescriptor.create("status", "whether the request is successful")); + + static final IncrementableMetric consoleRequestMetric = + MetricRegistryImpl.getDefault() + .newIncrementableMetric( + "/console/registrar/console_requests", + "Count of /registrar requests", + "count", + CONSOLE_LABEL_DESCRIPTORS); + + private static final ImmutableSet SETTINGS_LABEL_DESCRIPTORS = + ImmutableSet.of( + LabelDescriptor.create("clientId", "target registrar client ID"), + LabelDescriptor.create("action", "action performed"), + LabelDescriptor.create("role", "Role[s] of the user making the request"), + LabelDescriptor.create("status", "whether the request is successful")); + + static final IncrementableMetric settingsRequestMetric = + MetricRegistryImpl.getDefault() + .newIncrementableMetric( + "/console/registrar/setting_requests", + "Count of /registrar-settings requests", + "count", + SETTINGS_LABEL_DESCRIPTORS); + + @Inject + public RegistrarConsoleMetrics() {} + + void registerConsoleRequest( + String clientId, boolean explicitClientId, ImmutableSet roles, String status) { + consoleRequestMetric.increment( + clientId, String.valueOf(explicitClientId), String.valueOf(roles), status); + } + + void registerSettingsRequest( + String clientId, String action, ImmutableSet roles, String status) { + settingsRequestMetric.increment(clientId, action, String.valueOf(roles), status); + } +} diff --git a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java index a4b09ec5b..412d03fa1 100644 --- a/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java +++ b/java/google/registry/ui/server/registrar/RegistrarSettingsAction.java @@ -21,6 +21,7 @@ import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.security.JsonResponseHelper.Status.ERROR; import static google.registry.security.JsonResponseHelper.Status.SUCCESS; +import com.google.auto.value.AutoValue; import com.google.common.base.Ascii; import com.google.common.base.Strings; import com.google.common.collect.HashMultimap; @@ -80,6 +81,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA @Inject JsonActionRunner jsonActionRunner; @Inject AppEngineServiceUtils appEngineServiceUtils; + @Inject RegistrarConsoleMetrics registrarConsoleMetrics; @Inject SendEmailUtils sendEmailUtils; @Inject AuthenticatedRegistrarAccessor registrarAccessor; @Inject AuthResult authResult; @@ -114,34 +116,55 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA @SuppressWarnings("unchecked") Map args = (Map) Optional.ofNullable(input.get(ARGS_PARAM)).orElse(ImmutableMap.of()); + logger.atInfo().log("Received request '%s' on registrar '%s' with args %s", op, clientId, args); + String status = "SUCCESS"; try { switch (op) { case "update": - return update(args, clientId); + return update(args, clientId).toJsonResponse(); case "read": - return read(clientId); + return read(clientId).toJsonResponse(); default: - return JsonResponseHelper.create(ERROR, "Unknown or unsupported operation: " + op); + throw new IllegalArgumentException("Unknown or unsupported operation: " + op); } - } catch (FormFieldException e) { - logger.atWarning().withCause(e).log( - "Failed to perform operation '%s' on registrar '%s' for args %s", op, clientId, args); - return JsonResponseHelper.createFormFieldError(e.getMessage(), e.getFieldName()); } catch (Throwable e) { logger.atWarning().withCause(e).log( "Failed to perform operation '%s' on registrar '%s' for args %s", op, clientId, args); + status = "ERROR: " + e.getClass().getSimpleName(); + if (e instanceof FormFieldException) { + FormFieldException formFieldException = (FormFieldException) e; + return JsonResponseHelper.createFormFieldError( + formFieldException.getMessage(), formFieldException.getFieldName()); + } return JsonResponseHelper.create( ERROR, Optional.ofNullable(e.getMessage()).orElse("Unspecified error")); + } finally { + registrarConsoleMetrics.registerSettingsRequest( + clientId, op, registrarAccessor.getAllClientIdWithRoles().get(clientId), status); } } - Map read(String clientId) { - return JsonResponseHelper.create( - SUCCESS, "Success", registrarAccessor.getRegistrar(clientId).toJsonMap()); + @AutoValue + abstract static class RegistrarResult { + abstract String message(); + + abstract Registrar registrar(); + + Map toJsonResponse() { + return JsonResponseHelper.create(SUCCESS, message(), registrar().toJsonMap()); + } + + static RegistrarResult create(String message, Registrar registrar) { + return new AutoValue_RegistrarSettingsAction_RegistrarResult(message, registrar); + } } - Map update(final Map args, String clientId) { + private RegistrarResult read(String clientId) { + return RegistrarResult.create("Success", registrarAccessor.getRegistrar(clientId)); + } + + private RegistrarResult update(final Map args, String clientId) { return ofy() .transact( () -> { @@ -196,8 +219,7 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA // Email and return update. sendExternalUpdatesIfNecessary( registrar, contacts, updatedRegistrar, updatedContacts); - return JsonResponseHelper.create( - SUCCESS, "Saved " + clientId, updatedRegistrar.toJsonMap()); + return RegistrarResult.create("Saved " + clientId, updatedRegistrar); }); } diff --git a/javatests/google/registry/ui/server/registrar/BUILD b/javatests/google/registry/ui/server/registrar/BUILD index d3137c720..e7a04c602 100644 --- a/javatests/google/registry/ui/server/registrar/BUILD +++ b/javatests/google/registry/ui/server/registrar/BUILD @@ -28,6 +28,7 @@ java_library( "@com_google_flogger_system_backend", "@com_google_guava", "@com_google_guava_testlib", + "@com_google_monitoring_client_contrib", "@com_google_truth", "@com_google_truth_extensions_truth_java8_extension", "@com_googlecode_json_simple", diff --git a/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java b/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java index 705655ed4..5cfb05ac9 100644 --- a/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java +++ b/javatests/google/registry/ui/server/registrar/ConsoleUiActionTest.java @@ -16,6 +16,7 @@ package google.registry.ui.server.registrar; import static com.google.common.net.HttpHeaders.LOCATION; import static com.google.common.truth.Truth.assertThat; +import static com.google.monitoring.metrics.contrib.LongMetricSubject.assertThat; import static google.registry.testing.DatastoreHelper.loadRegistrar; import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.ADMIN; import static google.registry.ui.server.registrar.AuthenticatedRegistrarAccessor.Role.OWNER; @@ -38,6 +39,7 @@ import google.registry.testing.FakeResponse; import google.registry.testing.UserInfo; import java.util.Optional; import javax.servlet.http.HttpServletRequest; +import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -73,6 +75,7 @@ public class ConsoleUiActionTest { action.technicalDocsUrl = "http://example.com/technical-docs"; action.req = request; action.response = response; + action.registrarConsoleMetrics = new RegistrarConsoleMetrics(); action.registrarAccessor = registrarAccessor; action.userService = UserServiceFactory.getUserService(); action.xsrfTokenManager = new XsrfTokenManager(new FakeClock(), action.userService); @@ -91,24 +94,39 @@ public class ConsoleUiActionTest { when(registrarAccessor.guessClientId()).thenCallRealMethod(); // Used for error message in guessClientId registrarAccessor.authResult = authResult; + RegistrarConsoleMetrics.consoleRequestMetric.reset(); + } + + @After + public void tearDown() throws Exception { + assertThat(RegistrarConsoleMetrics.consoleRequestMetric).hasNoOtherValues(); + } + + public void assertMetric(String clientId, String explicitClientId, String roles, String status) { + assertThat(RegistrarConsoleMetrics.consoleRequestMetric) + .hasValueForLabels(1, clientId, explicitClientId, roles, status); + RegistrarConsoleMetrics.consoleRequestMetric.reset(clientId, explicitClientId, roles, status); } @Test public void testWebPage_disallowsIframe() { action.run(); assertThat(response.getHeaders()).containsEntry("X-Frame-Options", "SAMEORIGIN"); + assertMetric("TheRegistrar", "false", "[OWNER]", "SUCCESS"); } @Test public void testWebPage_setsHtmlUtf8ContentType() { action.run(); assertThat(response.getContentType()).isEqualTo(MediaType.HTML_UTF_8); + assertMetric("TheRegistrar", "false", "[OWNER]", "SUCCESS"); } @Test public void testWebPage_containsUserNickname() { action.run(); assertThat(response.getPayload()).contains("marla.singer"); + assertMetric("TheRegistrar", "false", "[OWNER]", "SUCCESS"); } @Test @@ -116,6 +134,7 @@ public class ConsoleUiActionTest { action.run(); assertThat(response.getPayload()).contains("Registrar Console"); assertThat(response.getPayload()).contains("reg-content-and-footer"); + assertMetric("TheRegistrar", "false", "[OWNER]", "SUCCESS"); } @Test @@ -131,6 +150,7 @@ public class ConsoleUiActionTest { action.run(); assertThat(response.getPayload()).contains("

You need permission

"); assertThat(response.getPayload()).contains("not associated with Nomulus."); + assertMetric("", "false", "[]", "FORBIDDEN"); } @Test @@ -153,22 +173,25 @@ public class ConsoleUiActionTest { @Test public void testSettingClientId_notAllowed_showsNeedPermissionPage() { - action.paramClientId = Optional.of("OtherClientId"); - when(registrarAccessor.getRegistrar("OtherClientId")) + // Behaves the same way if fakeRegistrar exists, but we don't have access to it + action.paramClientId = Optional.of("fakeRegistrar"); + when(registrarAccessor.getRegistrar("fakeRegistrar")) .thenThrow(new ForbiddenException("forbidden")); action.run(); assertThat(response.getPayload()).contains("

You need permission

"); - assertThat(response.getPayload()).contains("not associated with the registrar OtherClientId."); + assertThat(response.getPayload()).contains("not associated with the registrar fakeRegistrar."); + assertMetric("fakeRegistrar", "true", "[]", "FORBIDDEN"); } @Test public void testSettingClientId_allowed_showsRegistrarConsole() { - action.paramClientId = Optional.of("OtherClientId"); - when(registrarAccessor.getRegistrar("OtherClientId")) + action.paramClientId = Optional.of("OtherRegistrar"); + when(registrarAccessor.getRegistrar("OtherRegistrar")) .thenReturn(loadRegistrar("TheRegistrar")); action.run(); assertThat(response.getPayload()).contains("Registrar Console"); assertThat(response.getPayload()).contains("reg-content-and-footer"); + assertMetric("OtherRegistrar", "true", "[OWNER, ADMIN]", "SUCCESS"); } @Test @@ -177,5 +200,6 @@ public class ConsoleUiActionTest { assertThat(response.getPayload()).contains("