Skip to content

Commit

Permalink
Address feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@jprenken
jprenken committed Nov 5, 2024
1 parent 2b7a13a commit a547c52
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 68 deletions.
8 changes: 3 additions & 5 deletions ra/ra.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1669,16 +1669,15 @@ 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{
RegistrationID: req.RegistrationID,
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
Expand All @@ -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
Expand Down
91 changes: 60 additions & 31 deletions ra/ra_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down Expand Up @@ -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,
Expand All @@ -4531,34 +4537,38 @@ func TestUpdateRegistrationContact(t *testing.T) {

expectRegID := int64(1)
expectContacts := []string{"mailto:[email protected]"}
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.
Expand All @@ -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
Expand All @@ -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")
}
20 changes: 5 additions & 15 deletions sa/sa.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down
84 changes: 67 additions & 17 deletions sa/sa_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4936,41 +4936,39 @@ func TestUpdateRegistrationContact(t *testing.T) {
exampleContact, _ := json.Marshal("[email protected]")
twoExampleContacts, _ := json.Marshal([]string{"[email protected]", "[email protected]"})

_, 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:[email protected]"},
expectedError: nil,
},
{
name: "update a valid registration from no contacts to two email addresses",
oldContactsJSON: []string{string(noContact)},
newContacts: []string{"mailto:[email protected]", "mailto:[email protected]"},
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:[email protected]", "mailto:[email protected]"},
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 {
Expand All @@ -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)
})
}
}
Expand All @@ -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)
}
})
}
}

0 comments on commit a547c52

Please sign in to comment.