Fix registrar security console

The registrar security console failed because it assumed the email is a
required field for the registrar, but it isn't (at least - create_registrar
doesn't require an email, and update_registrar lets you remove the email).

Fixed by allowing it to *remain* unset if it was unset originally, but if it was set - it's required.

There are more fixes needed, but they aren't related to the email, so they will wait for the next CL

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=191623034
This commit is contained in:
guyben 2018-04-04 11:45:13 -07:00 committed by Ben McIlwain
parent e816913c61
commit ea891001d9
7 changed files with 44 additions and 14 deletions

View file

@ -77,7 +77,7 @@ registry.json.Response.prototype.results;
* ianaIdentifier: (number?|undefined),
* icannReferralEmail: string,
* ipAddressWhitelist: !Array<string>,
* emailAddress: string,
* emailAddress: (string?|undefined),
* phonePasscode: (string?|undefined),
* phoneNumber: (string?|undefined),
* faxNumber: (string?|undefined),

View file

@ -71,12 +71,17 @@ public final class RegistrarFormFields {
.required()
.build();
public static final FormField<String, String> EMAIL_ADDRESS_FIELD =
public static final FormField<String, String> EMAIL_ADDRESS_FIELD_REQUIRED =
FormFields.EMAIL.asBuilderNamed("emailAddress")
.matches(ASCII_PATTERN, ASCII_ERROR)
.required()
.build();
public static final FormField<String, String> EMAIL_ADDRESS_FIELD_OPTIONAL =
FormFields.EMAIL.asBuilderNamed("emailAddress")
.matches(ASCII_PATTERN, ASCII_ERROR)
.build();
public static final FormField<String, String> ICANN_REFERRAL_EMAIL_FIELD =
FormFields.EMAIL.asBuilderNamed("icannReferralEmail")
.matches(ASCII_PATTERN, ASCII_ERROR)

View file

@ -20,6 +20,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.common.base.Strings;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@ -125,14 +126,14 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
initialRegistrar.getClientId(),
args);
return JsonResponseHelper.createFormFieldError(e.getMessage(), e.getFieldName());
} catch (FormException ee) {
} catch (FormException e) {
logger.warningfmt(
ee,
e,
"Failed to perform operation '%s' on registrar '%s' for args %s",
op,
initialRegistrar.getClientId(),
args);
return JsonResponseHelper.create(ERROR, ee.getMessage());
return JsonResponseHelper.create(ERROR, e.getMessage());
}
}
@ -196,7 +197,12 @@ public class RegistrarSettingsAction implements Runnable, JsonActionRunner.JsonA
RegistrarFormFields.WHOIS_SERVER_FIELD.extractUntyped(args).orElse(null));
builder.setReferralUrl(
RegistrarFormFields.REFERRAL_URL_FIELD.extractUntyped(args).orElse(null));
RegistrarFormFields.EMAIL_ADDRESS_FIELD
// If the email is already null / empty - we can keep it so. But if it's set - it's required to
// remain set.
(Strings.isNullOrEmpty(existingRegistrarObj.getEmailAddress())
? RegistrarFormFields.EMAIL_ADDRESS_FIELD_OPTIONAL
: RegistrarFormFields.EMAIL_ADDRESS_FIELD_REQUIRED)
.extractUntyped(args)
.ifPresent(builder::setEmailAddress);
builder.setPhoneNumber(

View file

@ -25,7 +25,7 @@
{@param? whoisServer: string}
{@param? referralUrl: string}
// Passed to .contactInfo_
{@param emailAddress: string}
{@param? emailAddress: string}
{@param? localizedAddress: ?}
{@param? phoneNumber: string}
{@param? faxNumber: string}
@ -79,7 +79,7 @@
* Contact info.
*/
{template .contactInfo_ visibility="private"}
{@param emailAddress: string}
{@param? emailAddress: string}
{@param readonly: bool}
{@param? localizedAddress: ?}
{@param? phoneNumber: string}
@ -107,11 +107,13 @@
value="{$faxNumber}">
<div class="{css('contact-fax-number')}">{$faxNumber} (Fax)</div>
{/if}
<input type="hidden"
name="emailAddress"
id="emailAddress"
value="{$emailAddress}">
<div class="{css('contact-fax-number')}">{$emailAddress}</div>
{if isNonnull($emailAddress)}
<input type="hidden"
name="emailAddress"
id="emailAddress"
value="{$emailAddress}">
<div class="{css('contact-fax-number')}">{$emailAddress}</div>
{/if}
</td>
{else}
{call registry.soy.forms.inputFieldRowWithValue}

View file

@ -88,6 +88,8 @@ public final class RegistryTestServer {
route("/registrar", google.registry.module.frontend.FrontendServlet.class),
route("/registrar-settings",
google.registry.module.frontend.FrontendServlet.class),
route("/registrar-premium-price-ack",
google.registry.module.frontend.FrontendServlet.class),
route("/registrar-payment",
google.registry.module.frontend.FrontendServlet.class),
route("/registrar-payment-setup",

View file

@ -16,6 +16,7 @@ package google.registry.ui.server.registrar;
import static com.google.common.truth.Truth.assertThat;
import static google.registry.testing.DatastoreHelper.loadRegistrar;
import static google.registry.testing.DatastoreHelper.persistResource;
import static google.registry.testing.JUnitBackports.assertThrows;
import static google.registry.testing.TaskQueueHelper.assertNoTasksEnqueued;
import static google.registry.testing.TaskQueueHelper.assertTasksEnqueued;
@ -98,6 +99,16 @@ public class RegistrarSettingsActionTest extends RegistrarSettingsActionTestCase
assertNoTasksEnqueued("sheet");
}
@Test
public void testUpdate_emptyJsonObject_emailFieldNotRequiredWhenEmpty() throws Exception {
persistResource(loadRegistrar(CLIENT_ID).asBuilder().setEmailAddress(null).build());
Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(
"op", "update",
"args", ImmutableMap.of()));
assertThat(response).containsEntry("status", "SUCCESS");
}
@Test
public void testUpdate_badEmail_errorEmailField() throws Exception {
Map<String, Object> response = action.handleJsonRequest(ImmutableMap.of(

View file

@ -106,8 +106,12 @@ public class RegistrarSettingsActionTestCase {
when(rsp.getWriter()).thenReturn(new PrintWriter(writer));
when(req.getContentType()).thenReturn("application/json");
when(req.getReader()).thenReturn(createJsonPayload(ImmutableMap.of("op", "read")));
// We mock getRegistrarForAuthResult to reload the registrar each time it's called. Otherwise -
// the result is out of date after mutations.
// (for example, if someone wants to change the registrar to prepare for a test, the function
// would still return the old value)
when(sessionUtils.getRegistrarForAuthResult(req, action.authResult))
.thenReturn(loadRegistrar(CLIENT_ID));
.thenAnswer(x -> loadRegistrar(CLIENT_ID));
when(modulesService.getVersionHostname("backend", null)).thenReturn("backend.hostname");
}