From 7da9a83debdda096a64ef0bd9597222ee282afba Mon Sep 17 00:00:00 2001 From: James Renken Date: Tue, 14 Jan 2025 15:58:56 -0800 Subject: [PATCH] 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 --- grpc/pb-marshalling.go | 14 ++------ grpc/pb-marshalling_test.go | 4 ++- ra/ra.go | 27 +++------------ ra/ra_test.go | 68 ------------------------------------- 4 files changed, 9 insertions(+), 104 deletions(-) diff --git a/grpc/pb-marshalling.go b/grpc/pb-marshalling.go index c62a5e1417d..c1bedd42357 100644 --- a/grpc/pb-marshalling.go +++ b/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, diff --git a/grpc/pb-marshalling_test.go b/grpc/pb-marshalling_test.go index 73ceff2b331..3273eeab33d 100644 --- a/grpc/pb-marshalling_test.go +++ b/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, diff --git a/ra/ra.go b/ra/ra.go index 396b5d23ff2..65cabc73794 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -384,10 +384,6 @@ 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 @@ -395,11 +391,10 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, reques // 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. diff --git a/ra/ra_test.go b/ra/ra_test.go index 66aa603a56e..98832a05648 100644 --- a/ra/ra_test.go +++ b/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:foo@letsencrypt.org"}, - ContactsPresent: true, - Key: newAcctKey(t), - }, - ExpectedErr: nil, - }, - { - Name: "Valid contact provided by client ContactsPresent false", - Reg: &corepb.Registration{ - Contact: []string{"mailto:foo@letsencrypt.org"}, - 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 }