Introduce separate UpdateRegistrationContact & UpdateRegistrationKey methods in RA & SA

ra/proto/ra.proto

@@ -10,6 +10,8 @@ import "google/protobuf/empty.proto";
 service RegistrationAuthority {
   rpc NewRegistration(core.Registration) returns (core.Registration) {}
   rpc UpdateRegistration(UpdateRegistrationRequest) returns (core.Registration) {}
+  rpc UpdateRegistrationContact(UpdateRegistrationContactRequest) returns (core.Registration) {}
+  rpc UpdateRegistrationKey(UpdateRegistrationKeyRequest) returns (core.Registration) {}
   rpc PerformValidation(PerformValidationRequest) returns (core.Authorization) {}
   rpc DeactivateRegistration(core.Registration) returns (google.protobuf.Empty) {}
   rpc DeactivateAuthorization(core.Authorization) returns (google.protobuf.Empty) {}
@@ -33,6 +35,16 @@ message UpdateRegistrationRequest {
   core.Registration update = 2;
 }
 
+message UpdateRegistrationContactRequest {
+  int64 registrationID = 1;
+  repeated string contacts = 2;
+}
+
+message UpdateRegistrationKeyRequest {
+  int64 registrationID = 1;
+  bytes jwk = 2;
+}
+
 message UpdateAuthorizationRequest {
   core.Authorization authz = 1;
   int64 challengeIndex = 2;

ra/ra.go

@@ -1620,7 +1620,8 @@ func (ra *RegistrationAuthorityImpl) checkNewOrderLimits(ctx context.Context, na
 // UpdateRegistration updates an existing Registration with new values. Caller
 // is responsible for making sure that update.Key is only different from base.Key
 // if it is being called from the WFE key change endpoint.
-// TODO(#5554): Split this into separate methods for updating Contacts vs Key.
+//
+// Deprecated: Use UpdateRegistrationContact or UpdateRegistrationKey instead.
 func (ra *RegistrationAuthorityImpl) UpdateRegistration(ctx context.Context, req *rapb.UpdateRegistrationRequest) (*corepb.Registration, error) {
 	// Error if the request is nil, there is no account key or IP address
 	if req.Base == nil || len(req.Base.Key) == 0 || len(req.Base.InitialIP) == 0 || req.Base.Id == 0 {
@@ -1659,6 +1660,46 @@ func (ra *RegistrationAuthorityImpl) UpdateRegistration(ctx context.Context, req
 	return update, nil
 }
 
+// UpdateRegistrationContact updates an existing Registration's contact.
+// The updated contacts field may be empty.
+func (ra *RegistrationAuthorityImpl) UpdateRegistrationContact(ctx context.Context, req *rapb.UpdateRegistrationContactRequest) (*corepb.Registration, error) {
+	if core.IsAnyNilOrZero(req.RegistrationID) {
+		return nil, errIncompleteGRPCRequest
+	}
+
+	err := ra.validateContacts(req.Contacts)
+	if err != nil {
+		return nil, fmt.Errorf("invalid contact: %w", err)
+	}
+
+	update, err := ra.SA.UpdateRegistrationContact(ctx, &sapb.UpdateRegistrationContactRequest{
+		RegistrationID: req.RegistrationID,
+		Contacts:       req.Contacts,
+	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to update registration contact: %w", err)
+	}
+
+	return update, nil
+}
+
+// UpdateRegistrationKey updates an existing Registration's key.
+func (ra *RegistrationAuthorityImpl) UpdateRegistrationKey(ctx context.Context, req *rapb.UpdateRegistrationKeyRequest) (*corepb.Registration, error) {
+	if core.IsAnyNilOrZero(req.RegistrationID, req.Jwk) {
+		return nil, errIncompleteGRPCRequest
+	}
+
+	update, err := ra.SA.UpdateRegistrationKey(ctx, &sapb.UpdateRegistrationKeyRequest{
+		RegistrationID: req.RegistrationID,
+		Jwk:            req.Jwk,
+	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to update registration key: %w", err)
+	}
+
+	return update, nil
+}
+
 func contactsEqual(a []string, b []string) bool {
 	if len(a) != len(b) {
 		return false

ra/ra_test.go

@@ -671,10 +671,21 @@ type NoUpdateSA struct {
 	sapb.StorageAuthorityClient
 }
 
-func (sa NoUpdateSA) UpdateRegistration(_ context.Context, _ *corepb.Registration, _ ...grpc.CallOption) (*emptypb.Empty, error) {
+// Deprecated: When this function is removed, the NoUpdateSA should be moved
+// down to join the other mocks and tests for UpdateRegistrationContact & Key,
+// where it's also used.
+func (sa *NoUpdateSA) UpdateRegistration(_ context.Context, _ *corepb.Registration, _ ...grpc.CallOption) (*emptypb.Empty, error) {
 	return nil, fmt.Errorf("UpdateRegistration() is mocked to always error")
 }
 
+func (sa *NoUpdateSA) UpdateRegistrationContact(_ context.Context, _ *sapb.UpdateRegistrationContactRequest, _ ...grpc.CallOption) (*corepb.Registration, error) {
+	return nil, fmt.Errorf("UpdateRegistrationContact() is mocked to always error")
+}
+
+func (sa *NoUpdateSA) UpdateRegistrationKey(_ context.Context, _ *sapb.UpdateRegistrationKeyRequest, _ ...grpc.CallOption) (*corepb.Registration, error) {
+	return nil, fmt.Errorf("UpdateRegistrationKey() is mocked to always error")
+}
+
 func TestUpdateRegistrationSame(t *testing.T) {
 	_, _, ra, _, cleanUp := initAuthorities(t)
 	defer cleanUp()
@@ -4483,3 +4494,136 @@ func TestGetAuthorization(t *testing.T) {
 	test.AssertNotError(t, err, "should not fail")
 	test.AssertEquals(t, len(authz.Challenges), 0)
 }
+
+// mockSARecordingRegistration tests UpdateRegistrationContact and UpdateRegistrationKey.
+type mockSARecordingRegistration struct {
+	sapb.StorageAuthorityClient
+	providedRegistrationID int64
+	providedContacts       []string
+	providedJwk            []byte
+}
+
+// UpdateRegistrationContact records the registration ID and updated contacts
+// (optional) provided.
+func (sa *mockSARecordingRegistration) UpdateRegistrationContact(ctx context.Context, req *sapb.UpdateRegistrationContactRequest, _ ...grpc.CallOption) (*corepb.Registration, error) {
+	sa.providedRegistrationID = req.RegistrationID
+	sa.providedContacts = req.Contacts
+
+	return &corepb.Registration{
+		Id:      req.RegistrationID,
+		Contact: req.Contacts,
+	}, nil
+}
+
+// UpdateRegistrationKey records the registration ID and updated key provided.
+func (sa *mockSARecordingRegistration) UpdateRegistrationKey(ctx context.Context, req *sapb.UpdateRegistrationKeyRequest, _ ...grpc.CallOption) (*corepb.Registration, error) {
+	sa.providedRegistrationID = req.RegistrationID
+	sa.providedJwk = req.Jwk
+
+	return &corepb.Registration{
+		Id:  req.RegistrationID,
+		Key: req.Jwk,
+	}, nil
+}
+
+// TestUpdateRegistrationContact tests that the RA's UpdateRegistrationContact
+// method correctly: requires a registration ID; validates the contact provided;
+// does not require a contact; passes the requested registration ID and contact
+// to the SA; passes the updated Registration back to the caller; and can return
+// an error.
+func TestUpdateRegistrationContact(t *testing.T) {
+	_, _, ra, _, cleanUp := initAuthorities(t)
+	defer cleanUp()
+
+	expectRegID := int64(1)
+	expectContacts := []string{"mailto:[email protected]"}
+	mockSA := mockSARecordingRegistration{}
+	ra.SA = &mockSA
+
+	_, err := ra.UpdateRegistrationContact(context.Background(), &rapb.UpdateRegistrationContactRequest{})
+	test.AssertError(t, err, "should not have been able to update registration contact without a registration ID")
+	test.AssertContains(t, err.Error(), "incomplete gRPC request message")
+
+	_, err = ra.UpdateRegistrationContact(context.Background(), &rapb.UpdateRegistrationContactRequest{
+		RegistrationID: expectRegID,
+		Contacts:       []string{"tel:+44123"},
+	})
+	test.AssertError(t, err, "should not have been able to update registration contact to an invalid contact")
+	test.AssertContains(t, err.Error(), "invalid contact")
+
+	res, err := ra.UpdateRegistrationContact(context.Background(), &rapb.UpdateRegistrationContactRequest{
+		RegistrationID: expectRegID,
+	})
+	test.AssertNotError(t, err, "should have been able to update registration with a blank contact")
+	test.AssertEquals(t, res.Id, expectRegID)
+	test.AssertEquals(t, mockSA.providedRegistrationID, expectRegID)
+	test.AssertDeepEquals(t, res.Contact, []string(nil))
+	test.AssertDeepEquals(t, mockSA.providedContacts, []string(nil))
+
+	res, err = ra.UpdateRegistrationContact(context.Background(), &rapb.UpdateRegistrationContactRequest{
+		RegistrationID: expectRegID,
+		Contacts:       expectContacts,
+	})
+	test.AssertNotError(t, err, "should have been able to update registration with a populated contact")
+	test.AssertEquals(t, res.Id, expectRegID)
+	test.AssertEquals(t, mockSA.providedRegistrationID, expectRegID)
+	test.AssertDeepEquals(t, res.Contact, expectContacts)
+	test.AssertDeepEquals(t, mockSA.providedContacts, expectContacts)
+
+	// Switch to a mock SA that will always error if UpdateRegistrationContact()
+	// is called.
+	ra.SA = &NoUpdateSA{}
+	_, err = ra.UpdateRegistrationContact(context.Background(), &rapb.UpdateRegistrationContactRequest{
+		RegistrationID: expectRegID,
+		Contacts:       expectContacts,
+	})
+	test.AssertError(t, err, "should have received an error from the SA")
+	test.AssertContains(t, err.Error(), "failed to update registration contact")
+	test.AssertContains(t, err.Error(), "mocked to always error")
+}
+
+// TestUpdateRegistrationKey tests that the RA's UpdateRegistrationKey method
+// correctly requires a registration ID and key, passes them to the SA, and
+// passes the updated Registration back to the caller.
+func TestUpdateRegistrationKey(t *testing.T) {
+	_, _, ra, _, cleanUp := initAuthorities(t)
+	defer cleanUp()
+
+	expectRegID := int64(1)
+	expectJwk := AccountKeyJSONA
+	mockSA := mockSARecordingRegistration{}
+	ra.SA = &mockSA
+
+	_, err := ra.UpdateRegistrationKey(context.Background(), &rapb.UpdateRegistrationKeyRequest{})
+	test.AssertError(t, err, "should not have been able to update registration key without a registration ID or key")
+	test.AssertContains(t, err.Error(), "incomplete gRPC request message")
+
+	_, err = ra.UpdateRegistrationKey(context.Background(), &rapb.UpdateRegistrationKeyRequest{RegistrationID: expectRegID})
+	test.AssertError(t, err, "should not have been able to update registration key without a key")
+	test.AssertContains(t, err.Error(), "incomplete gRPC request message")
+
+	_, err = ra.UpdateRegistrationKey(context.Background(), &rapb.UpdateRegistrationKeyRequest{Jwk: expectJwk})
+	test.AssertError(t, err, "should not have been able to update registration key without a registration ID")
+	test.AssertContains(t, err.Error(), "incomplete gRPC request message")
+
+	res, err := ra.UpdateRegistrationKey(context.Background(), &rapb.UpdateRegistrationKeyRequest{
+		RegistrationID: expectRegID,
+		Jwk:            expectJwk,
+	})
+	test.AssertNotError(t, err, "should have been able to update registration key")
+	test.AssertEquals(t, res.Id, expectRegID)
+	test.AssertEquals(t, mockSA.providedRegistrationID, expectRegID)
+	test.AssertDeepEquals(t, res.Key, expectJwk)
+	test.AssertDeepEquals(t, mockSA.providedJwk, expectJwk)
+
+	// Switch to a mock SA that will always error if UpdateRegistrationKey() is
+	// called.
+	ra.SA = &NoUpdateSA{}
+	_, err = ra.UpdateRegistrationKey(context.Background(), &rapb.UpdateRegistrationKeyRequest{
+		RegistrationID: expectRegID,
+		Jwk:            expectJwk,
+	})
+	test.AssertError(t, err, "should have received an error from the SA")
+	test.AssertContains(t, err.Error(), "failed to update registration key")
+	test.AssertContains(t, err.Error(), "mocked to always error")
+}