ra.NewRegistration: error ContactsPresent mismatch (#5399)

Generate error if ra.NewRegistration receives RPC with
ContactsPresent: false and non-empty Contacts list

Fixes #5396

ra/ra.go

@@ -360,6 +360,10 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, reques
 		return nil, err
 	}
 
+	if err := validateContactsPresent(request.Contact, request.ContactsPresent); err != nil {
+		return nil, err
+	}
+
 	reg := core.Registration{
 		Key:    &key,
 		Status: core.StatusValid,
@@ -2155,3 +2159,17 @@ func wildcardOverlap(dnsNames []string) error {
 	}
 	return nil
 }
+
+// validateContactsPresent will return an error if the contacts []string
+// len is greater than zero and the contactsPresent bool is false. We
+// don't care about any other cases. If the length of the contacts is zero
+// and contactsPresent is true, it seems like a mismatch but we have to
+// assume that the client is requesting to update the contacts field with
+// by removing the existing contacts value so we don't want to return an
+// error here.
+func validateContactsPresent(contacts []string, contactsPresent bool) error {
+	if len(contacts) > 0 && !contactsPresent {
+		return berrors.InternalServerError("account contacts present but contactsPresent false")
+	}
+	return nil
+}

ra/ra_test.go

@@ -462,6 +462,70 @@ func TestNewRegistration(t *testing.T) {
 	test.Assert(t, core.KeyDigestEquals(reg.Key, AccountKeyB), "Retrieved registration differed.")
 }
 
+func TestNewRegistrationContactsPresent(t *testing.T) {
+	_, _, ra, _, cleanUp := initAuthorities(t)
+	defer cleanUp()
+	testCases := []struct {
+		Name        string
+		Reg         *corepb.Registration
+		ExpectedErr error
+	}{
+		{
+			Name: "No contacts provided by client ContactsPresent false",
+			Reg: &corepb.Registration{
+				Key:       newAcctKey(t),
+				InitialIP: parseAndMarshalIP(t, "7.6.6.5"),
+			},
+			ExpectedErr: nil,
+		},
+		{
+			Name: "Empty contact provided by client ContactsPresent true",
+			Reg: &corepb.Registration{
+				Contact:         []string{},
+				ContactsPresent: true,
+				Key:             newAcctKey(t),
+				InitialIP:       parseAndMarshalIP(t, "7.6.6.4"),
+			},
+			ExpectedErr: nil,
+		},
+		{
+			Name: "Valid contact provided by client ContactsPresent true",
+			Reg: &corepb.Registration{
+				Contact:         []string{"mailto:foo@letsencrypt.org"},
+				ContactsPresent: true,
+				Key:             newAcctKey(t),
+				InitialIP:       parseAndMarshalIP(t, "7.6.4.3"),
+			},
+			ExpectedErr: nil,
+		},
+		{
+			Name: "Valid contact provided by client ContactsPresent false",
+			Reg: &corepb.Registration{
+				Contact:         []string{"mailto:foo@letsencrypt.org"},
+				ContactsPresent: false,
+				Key:             newAcctKey(t),
+				InitialIP:       parseAndMarshalIP(t, "7.6.6.2"),
+			},
+			ExpectedErr: fmt.Errorf("account contacts present but contactsPresent false"),
+		},
+	}
+	// For each test case we check that the NewRegistration works as
+	// intended with variations of Contact and ContactsPresent fields
+	for _, tc := range testCases {
+		t.Run(tc.Name, func(t *testing.T) {
+			// Create new registration
+			_, err := ra.NewRegistration(ctx, tc.Reg)
+			// Check error output
+			if tc.ExpectedErr == nil {
+				test.AssertNotError(t, err, "expected no error for NewRegistration")
+			} else {
+				test.AssertError(t, err, "expected error for NewRegistration")
+				test.AssertEquals(t, err.Error(), tc.ExpectedErr.Error())
+			}
+		})
+	}
+}
+
 type mockSAFailsNewRegistration struct {
 	mocks.StorageAuthority
 }