diff --git a/ra/ra.go b/ra/ra.go index 803d78c63b2..10f343d0444 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1669,7 +1669,7 @@ func (ra *RegistrationAuthorityImpl) UpdateRegistrationContact(ctx context.Conte err := ra.validateContacts(req.Contacts) if err != nil { - return nil, err + return nil, fmt.Errorf("invalid contact: %w", err) } update, err := ra.SA.UpdateRegistrationContact(ctx, &sapb.UpdateRegistrationContactRequest{ @@ -1677,8 +1677,7 @@ func (ra *RegistrationAuthorityImpl) UpdateRegistrationContact(ctx context.Conte Contacts: req.Contacts, }) if err != nil { - err = fmt.Errorf("failed to update registration contact: %w", err) - return nil, err + return nil, fmt.Errorf("failed to update registration contact: %w", err) } return update, nil @@ -1695,8 +1694,7 @@ func (ra *RegistrationAuthorityImpl) UpdateRegistrationKey(ctx context.Context, Jwk: req.Jwk, }) if err != nil { - err = fmt.Errorf("failed to update registration key: %w", err) - return nil, err + return nil, fmt.Errorf("failed to update registration key: %w", err) } return update, nil diff --git a/ra/ra_test.go b/ra/ra_test.go index 28c878ba1e2..ea77c22ecb9 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -674,15 +674,15 @@ type NoUpdateSA struct { // 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) { +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) { +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) { +func (sa *NoUpdateSA) UpdateRegistrationKey(_ context.Context, _ *sapb.UpdateRegistrationKeyRequest, _ ...grpc.CallOption) (*corepb.Registration, error) { return nil, fmt.Errorf("UpdateRegistrationKey() is mocked to always error") } @@ -4495,25 +4495,31 @@ func TestGetAuthorization(t *testing.T) { test.AssertEquals(t, len(authz.Challenges), 0) } -// An authority for testing UpdateRegistrationContact and UpdateRegistrationKey. -type mockSAWithRegistration struct { +// mockSARecordingRegistration tests UpdateRegistrationContact and UpdateRegistrationKey. +type mockSARecordingRegistration struct { sapb.StorageAuthorityClient - expectRegistrationID int64 - expectContacts []string - expectJwk []byte + providedRegistrationID int64 + providedContacts []string + providedJwk []byte } -// Mocked UpdateRegistrationContact returns the registration ID and updated -// contacts (optional) provided. -func (sa *mockSAWithRegistration) UpdateRegistrationContact(ctx context.Context, req *sapb.UpdateRegistrationContactRequest, _ ...grpc.CallOption) (*corepb.Registration, error) { +// 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 } -// Mocked UpdateRegistrationKey returns the updated key provided. -func (sa *mockSAWithRegistration) UpdateRegistrationKey(ctx context.Context, req *sapb.UpdateRegistrationKeyRequest, _ ...grpc.CallOption) (*corepb.Registration, error) { +// 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, @@ -4531,34 +4537,38 @@ func TestUpdateRegistrationContact(t *testing.T) { expectRegID := int64(1) expectContacts := []string{"mailto:test@contoso.com"} - mockSA := mockSAWithRegistration{ - expectRegistrationID: expectRegID, - expectContacts: expectContacts, - } + 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.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.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.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.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. @@ -4567,7 +4577,9 @@ func TestUpdateRegistrationContact(t *testing.T) { RegistrationID: expectRegID, Contacts: expectContacts, }) - test.AssertError(t, err, "Should have received an error from the SA") + 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 @@ -4579,22 +4591,39 @@ func TestUpdateRegistrationKey(t *testing.T) { expectRegID := int64(1) expectJwk := AccountKeyJSONA - mockSA := mockSAWithRegistration{expectRegistrationID: expectRegID, expectJwk: expectJwk} + 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.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: 1}) - test.AssertError(t, err, "Should not have been able to update registration key without a key") + _, 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: AccountKeyJSONA}) - test.AssertError(t, err, "Should not have been able to update registration key without a registration ID") + _, 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: 1, - Jwk: AccountKeyJSONA, + RegistrationID: expectRegID, + Jwk: expectJwk, }) - test.AssertNotError(t, err, "Should have been able to update registration key") + 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") } diff --git a/sa/sa.go b/sa/sa.go index 18320f767a9..2d2ce1b2148 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -180,7 +180,7 @@ func (ssa *SQLStorageAuthority) UpdateRegistrationContact(ctx context.Context, r if len(req.Contacts) != 0 { jsonContact, err = json.Marshal(req.Contacts) if err != nil { - return nil, err + return nil, fmt.Errorf("serializing contacts: %w", err) } } @@ -216,12 +216,7 @@ func (ssa *SQLStorageAuthority) UpdateRegistrationContact(ctx context.Context, r return nil, overallError } - updatedRegistration, ok := result.(*corepb.Registration) - if !ok { - return nil, fmt.Errorf("failed to assert result as *corepb.Registration in UpdateRegistrationContact") - } - - return updatedRegistration, nil + return result.(*corepb.Registration), nil } // UpdateRegistrationKey stores an updated key in a Registration. @@ -236,11 +231,11 @@ func (ssa *SQLStorageAuthority) UpdateRegistrationKey(ctx context.Context, req * var jwk jose.JSONWebKey err := jwk.UnmarshalJSON(req.Jwk) if err != nil { - return nil, err + return nil, fmt.Errorf("parsing JWK: %w", err) } sha, err := core.KeyDigestB64(jwk.Key) if err != nil { - return nil, err + return nil, fmt.Errorf("computing key digest: %w", err) } result, overallError := db.WithTransaction(ctx, ssa.dbMap, func(tx db.Executor) (interface{}, error) { @@ -281,12 +276,7 @@ func (ssa *SQLStorageAuthority) UpdateRegistrationKey(ctx context.Context, req * return nil, overallError } - updatedRegistration, ok := result.(*corepb.Registration) - if !ok { - return nil, fmt.Errorf("failed to assert result as *corepb.Registration in UpdateRegistrationKey") - } - - return updatedRegistration, nil + return result.(*corepb.Registration), nil } // AddSerial writes a record of a serial number generation to the DB. diff --git a/sa/sa_test.go b/sa/sa_test.go index b0998abd076..8295a1bff38 100644 --- a/sa/sa_test.go +++ b/sa/sa_test.go @@ -4936,41 +4936,39 @@ func TestUpdateRegistrationContact(t *testing.T) { exampleContact, _ := json.Marshal("test@example.com") twoExampleContacts, _ := json.Marshal([]string{"test1@example.com", "test2@example.com"}) + _, err := sa.UpdateRegistrationContact(ctx, &sapb.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") + tests := []struct { name string oldContactsJSON []string newContacts []string - expectedError error }{ { name: "update a valid registration from no contacts to one email address", oldContactsJSON: []string{string(noContact)}, newContacts: []string{"mailto:test@example.com"}, - expectedError: nil, }, { name: "update a valid registration from no contacts to two email addresses", oldContactsJSON: []string{string(noContact)}, newContacts: []string{"mailto:test1@example.com", "mailto:test2@example.com"}, - expectedError: nil, }, { name: "update a valid registration from one email address to no contacts", oldContactsJSON: []string{string(exampleContact)}, newContacts: []string{}, - expectedError: nil, }, { name: "update a valid registration from one email address to two email addresses", oldContactsJSON: []string{string(exampleContact)}, newContacts: []string{"mailto:test1@example.com", "mailto:test2@example.com"}, - expectedError: nil, }, { name: "update a valid registration from two email addresses to no contacts", oldContactsJSON: []string{string(twoExampleContacts)}, newContacts: []string{}, - expectedError: nil, }, } for _, tt := range tests { @@ -4984,13 +4982,19 @@ func TestUpdateRegistrationContact(t *testing.T) { }) test.AssertNotError(t, err, "creating new registration") - reg, err = sa.UpdateRegistrationContact(ctx, &sapb.UpdateRegistrationContactRequest{ + updatedReg, err := sa.UpdateRegistrationContact(ctx, &sapb.UpdateRegistrationContactRequest{ RegistrationID: reg.Id, Contacts: tt.newContacts, }) - test.AssertNotError(t, err, "Unexpected error for UpdateRegistrationContact()") + test.AssertNotError(t, err, "unexpected error for UpdateRegistrationContact()") + test.AssertEquals(t, updatedReg.Id, reg.Id) + test.AssertDeepEquals(t, updatedReg.Contact, tt.newContacts) - test.AssertDeepEquals(t, reg.Contact, tt.newContacts) + refetchedReg, err := sa.GetRegistration(ctx, &sapb.RegistrationID{ + Id: reg.Id, + }) + test.AssertNotError(t, err, "retrieving registration") + test.AssertDeepEquals(t, refetchedReg.Contact, tt.newContacts) }) } } @@ -4999,19 +5003,65 @@ func TestUpdateRegistrationKey(t *testing.T) { sa, _, cleanUp := initSA(t) defer cleanUp() + _, err := sa.UpdateRegistrationKey(ctx, &sapb.UpdateRegistrationKeyRequest{}) + 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") + initialIP, _ := net.ParseIP("43.34.43.34").MarshalText() - reg, err := sa.NewRegistration(ctx, &corepb.Registration{ + + existingReg, err := sa.NewRegistration(ctx, &corepb.Registration{ Key: newAcctKey(t), InitialIP: initialIP, }) test.AssertNotError(t, err, "creating new registration") - newJwk := newAcctKey(t) - reg, err = sa.UpdateRegistrationKey(ctx, &sapb.UpdateRegistrationKeyRequest{ - RegistrationID: reg.Id, - Jwk: newJwk, - }) - test.AssertNotError(t, err, "Unexpected error for UpdateRegistrationKey()") + tests := []struct { + name string + newJwk []byte + expectedError string + }{ + { + name: "update a valid registration with a new account key", + newJwk: newAcctKey(t), + }, + { + name: "update a valid registration with a duplicate account key", + newJwk: existingReg.Key, + expectedError: "key is already in use for a different account", + }, + { + name: "update a valid registration with a malformed account key", + newJwk: []byte("Eat at Joe's"), + expectedError: "parsing JWK", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + initialIP, _ := net.ParseIP("43.34.43.34").MarshalText() + reg, err := sa.NewRegistration(ctx, &corepb.Registration{ + Key: newAcctKey(t), + InitialIP: initialIP, + }) + test.AssertNotError(t, err, "creating new registration") - test.AssertDeepEquals(t, reg.Key, newJwk) + updatedReg, err := sa.UpdateRegistrationKey(ctx, &sapb.UpdateRegistrationKeyRequest{ + RegistrationID: reg.Id, + Jwk: tt.newJwk, + }) + if tt.expectedError != "" { + test.AssertError(t, err, "should have errored") + test.AssertContains(t, err.Error(), tt.expectedError) + } else { + test.AssertNotError(t, err, "unexpected error for UpdateRegistrationKey()") + test.AssertEquals(t, updatedReg.Id, reg.Id) + test.AssertDeepEquals(t, updatedReg.Key, tt.newJwk) + + refetchedReg, err := sa.GetRegistration(ctx, &sapb.RegistrationID{ + Id: reg.Id, + }) + test.AssertNotError(t, err, "retrieving registration") + test.AssertDeepEquals(t, refetchedReg.Key, tt.newJwk) + } + }) + } }