ra, pb: Don't expect or validate contactsPresent (#7933)

Part of #7920

There will be a followup removing the remaining places that set
`contactsPresent`.

---------

Co-authored-by: Jacob Hoffman-Andrews <[email protected]>

grpc/pb-marshalling.go

@@ -263,18 +263,8 @@ func PbToRegistration(pb *corepb.Registration) (core.Registration, error) {
 		createdAt = &c
 	}
 	var contacts *[]string
-	if pb.ContactsPresent {
-		if len(pb.Contact) != 0 {
-			contacts = &pb.Contact
-		} else {
-			// When gRPC creates an empty slice it is actually a nil slice. Since
-			// certain things boulder uses, like encoding/json, differentiate between
-			// these we need to de-nil these slices. Without this we are unable to
-			// properly do registration updates as contacts would always be removed
-			// as we use the difference between a nil and empty slice in ra.mergeUpdate.
-			empty := []string{}
-			contacts = &empty
-		}
+	if len(pb.Contact) != 0 {
+		contacts = &pb.Contact
 	}
 	return core.Registration{
 		ID:        pb.Id,

grpc/pb-marshalling_test.go

@@ -206,7 +206,9 @@ func TestRegistration(t *testing.T) {
 	test.AssertNotError(t, err, "registrationToPB failed")
 	outReg, err = PbToRegistration(pbReg)
 	test.AssertNotError(t, err, "PbToRegistration failed")
-	test.Assert(t, *outReg.Contact != nil, "Empty slice was converted to a nil slice")
+	if outReg.Contact != nil {
+		t.Errorf("Empty contacts should be a nil slice")
+	}
 
 	inRegNilCreatedAt := core.Registration{
 		ID:        1,

ra/ra.go

@@ -384,22 +384,17 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, reques
 	}
 
 	// Check that contacts conform to our expectations.
-	err = validateContactsPresent(request.Contact, request.ContactsPresent)
-	if err != nil {
-		return nil, err
-	}
 	err = ra.validateContacts(request.Contact)
 	if err != nil {
 		return nil, err
 	}
 
 	// Don't populate ID or CreatedAt because those will be set by the SA.
 	req := &corepb.Registration{
-		Key:             request.Key,
-		Contact:         request.Contact,
-		ContactsPresent: request.ContactsPresent,
-		Agreement:       request.Agreement,
-		Status:          string(core.StatusValid),
+		Key:       request.Key,
+		Contact:   request.Contact,
+		Agreement: request.Agreement,
+		Status:    string(core.StatusValid),
 	}
 
 	// Store the registration object, then return the version that got stored.
@@ -2318,20 +2313,6 @@ 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
-}
-
 // UnpauseAccount receives a validated account unpause request from the SFE and
 // instructs the SA to unpause that account. If the account cannot be unpaused,
 // an error is returned.

ra/ra_test.go

@@ -258,14 +258,6 @@ var (
 
 var ctx = context.Background()
 
-func newAcctKey(t *testing.T) []byte {
-	key, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-	jwk := &jose.JSONWebKey{Key: key.Public()}
-	acctKey, err := jwk.MarshalJSON()
-	test.AssertNotError(t, err, "failed to marshal account key")
-	return acctKey
-}
-
 func initAuthorities(t *testing.T) (*DummyValidationAuthority, sapb.StorageAuthorityClient, *RegistrationAuthorityImpl, ratelimits.Source, clock.FakeClock, func()) {
 	err := json.Unmarshal(AccountKeyJSONA, &AccountKeyA)
 	test.AssertNotError(t, err, "Failed to unmarshal public JWK")
@@ -464,66 +456,6 @@ func TestNewRegistration(t *testing.T) {
 	test.AssertByteEquals(t, reg.Key, acctKeyB)
 }
 
-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),
-			},
-			ExpectedErr: nil,
-		},
-		{
-			Name: "Empty contact provided by client ContactsPresent true",
-			Reg: &corepb.Registration{
-				Contact:         []string{},
-				ContactsPresent: true,
-				Key:             newAcctKey(t),
-			},
-			ExpectedErr: nil,
-		},
-		{
-			Name: "Valid contact provided by client ContactsPresent true",
-			Reg: &corepb.Registration{
-				Contact:         []string{"mailto:[email protected]"},
-				ContactsPresent: true,
-				Key:             newAcctKey(t),
-			},
-			ExpectedErr: nil,
-		},
-		{
-			Name: "Valid contact provided by client ContactsPresent false",
-			Reg: &corepb.Registration{
-				Contact:         []string{"mailto:[email protected]"},
-				ContactsPresent: false,
-				Key:             newAcctKey(t),
-			},
-			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 {
 	sapb.StorageAuthorityClient
 }