WFE: Don't remove contacts on empty update-account request (#8049)

When we receive an update-account request which is not empty, but
doesn't contain the "contact" field, don't assume that they want to
remove their contacts. Only remove contacts if the "contact" field is
present, but empty.

Add a unit test and an integration test which will catch regressions in
this behavior.

Author: aarongable

test/integration/account_test.go

@@ -48,3 +48,32 @@ func TestAccountDeactivate(t *testing.T) {
 	// account object as it used to make the request, and a wholly missing
 	// contacts field doesn't overwrite whatever eggsampler was holding in memory.
 }
+
+func TestAccountUpdate_UnspecifiedContacts(t *testing.T) {
+	t.Parallel()
+
+	c, err := acme.NewClient("http://boulder.service.consul:4001/directory")
+	if err != nil {
+		t.Fatalf("failed to connect to acme directory: %s", err)
+	}
+
+	acctKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("failed to generate account key: %s", err)
+	}
+
+	acct, err := c.NewAccount(acctKey, false, true, "mailto:example@"+random_domain())
+	if err != nil {
+		t.Fatalf("failed to create initial account: %s", err)
+	}
+
+	// This request does not include the Contact field, meaning that the contacts
+	// should remain unchanged (i.e. not be removed).
+	acct, err = c.UpdateAccount(acct)
+	if err != nil {
+		t.Errorf("failed to no-op update account: %s", err)
+	}
+	if len(acct.Contact) != 1 {
+		t.Errorf("unexpected number of contacts: want 1, got %d", len(acct.Contact))
+	}
+}

wfe2/wfe.go

@@ -1441,12 +1441,16 @@ func (wfe *WebFrontEndImpl) updateAccount(
 		return nil, probs.Malformed("Invalid value provided for status field")
 	}
 
-	var contacts []string
-	if accountUpdateRequest.Contact != nil {
-		contacts = *accountUpdateRequest.Contact
+	if accountUpdateRequest.Contact == nil {
+		// We use a pointer-to-slice for the contacts field so that we can tell the
+		// difference between the request not including the contact field, and the
+		// request including an empty contact list. If the field was omitted
+		// entirely, they don't want us to update it, so there's no work to do here.
+		return currAcct, nil
 	}
 
-	updatedAcct, err := wfe.ra.UpdateRegistrationContact(ctx, &rapb.UpdateRegistrationContactRequest{RegistrationID: currAcct.ID, Contacts: contacts})
+	updatedAcct, err := wfe.ra.UpdateRegistrationContact(ctx, &rapb.UpdateRegistrationContactRequest{
+		RegistrationID: currAcct.ID, Contacts: *accountUpdateRequest.Contact})
 	if err != nil {
 		return nil, web.ProblemDetailsForError(err, "Unable to update account")
 	}

wfe2/wfe_test.go

@@ -21,6 +21,8 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"os"
+	"reflect"
+	"slices"
 	"sort"
 	"strconv"
 	"strings"
@@ -197,13 +199,17 @@ func (ra *MockRegistrationAuthority) NewRegistration(ctx context.Context, in *co
 
 func (ra *MockRegistrationAuthority) UpdateRegistrationContact(ctx context.Context, in *rapb.UpdateRegistrationContactRequest, _ ...grpc.CallOption) (*corepb.Registration, error) {
 	return &corepb.Registration{
+		Status:  string(core.StatusValid),
 		Contact: in.Contacts,
 		Key:     []byte(test1KeyPublicJSON),
 	}, nil
 }
 
 func (ra *MockRegistrationAuthority) UpdateRegistrationKey(ctx context.Context, in *rapb.UpdateRegistrationKeyRequest, _ ...grpc.CallOption) (*corepb.Registration, error) {
-	return &corepb.Registration{Key: in.Jwk}, nil
+	return &corepb.Registration{
+		Status: string(core.StatusValid),
+		Key:    in.Jwk,
+	}, nil
 }
 
 func (ra *MockRegistrationAuthority) PerformValidation(context.Context, *rapb.PerformValidationRequest, ...grpc.CallOption) (*corepb.Authorization, error) {
@@ -1773,15 +1779,6 @@ func TestAuthorizationChallengeHandlerNamespace(t *testing.T) {
 	responseWriter.Body.Reset()
 }
 
-func contains(s []string, e string) bool {
-	for _, a := range s {
-		if a == e {
-			return true
-		}
-	}
-	return false
-}
-
 func TestAccount(t *testing.T) {
 	wfe, _, signer := setupWFE(t)
 	mux := wfe.Handler(metrics.NoopRegisterer)
@@ -1836,7 +1833,7 @@ func TestAccount(t *testing.T) {
 	wfe.Account(ctx, newRequestEvent(), responseWriter, request)
 	test.AssertNotContains(t, responseWriter.Body.String(), probs.ErrorNS)
 	links := responseWriter.Header()["Link"]
-	test.AssertEquals(t, contains(links, "<"+agreementURL+">;rel=\"terms-of-service\""), true)
+	test.AssertEquals(t, slices.Contains(links, "<"+agreementURL+">;rel=\"terms-of-service\""), true)
 	responseWriter.Body.Reset()
 
 	// Test POST valid JSON with garbage in URL but valid account ID
@@ -1877,6 +1874,66 @@ func TestAccount(t *testing.T) {
 	}`)
 }
 
+func TestUpdateAccount(t *testing.T) {
+	t.Parallel()
+	wfe, _, _ := setupWFE(t)
+
+	for _, tc := range []struct {
+		name     string
+		req      string
+		wantAcct *core.Registration
+	}{
+		{
+			name:     "deactivate clears contact",
+			req:      `{"status": "deactivated"}`,
+			wantAcct: &core.Registration{Status: core.StatusDeactivated},
+		},
+		{
+			name:     "deactivate takes priority over contact change",
+			req:      `{"status": "deactivated", "contact": ["mailto:admin@example.com"]}`,
+			wantAcct: &core.Registration{Status: core.StatusDeactivated},
+		},
+		{
+			name:     "change contact",
+			req:      `{"contact": ["mailto:admin@example.com"]}`,
+			wantAcct: &core.Registration{Status: core.StatusValid, Contact: &[]string{"mailto:admin@example.com"}},
+		},
+		{
+			name:     "change contact with unchanged status",
+			req:      `{"status": "valid", "contact": ["mailto:admin@example.com"]}`,
+			wantAcct: &core.Registration{Status: core.StatusValid, Contact: &[]string{"mailto:admin@example.com"}},
+		},
+		{
+			name:     "unchanged status leaves contact untouched",
+			req:      `{"status": "valid"}`,
+			wantAcct: &core.Registration{Status: core.StatusValid, Contact: &[]string{"mailto:webmaster@example.com"}},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			acct := core.Registration{
+				Status:  core.StatusValid,
+				Contact: &[]string{"mailto:webmaster@example.com"},
+			}
+
+			gotAcct, gotProb := wfe.updateAccount(context.Background(), []byte(tc.req), &acct)
+			if gotProb != nil {
+				t.Fatalf("want success, got problem %s", gotProb)
+			}
+
+			if tc.wantAcct != nil {
+				if gotAcct.Status != tc.wantAcct.Status {
+					t.Errorf("want status %s, got %s", tc.wantAcct.Status, gotAcct.Status)
+				}
+				if !reflect.DeepEqual(gotAcct.Contact, tc.wantAcct.Contact) {
+					t.Errorf("want contact %v, got %v", tc.wantAcct.Contact, gotAcct.Contact)
+				}
+			}
+		})
+	}
+}
+
 type mockSAWithCert struct {
 	sapb.StorageAuthorityReadOnlyClient
 	cert   *x509.Certificate
@@ -2930,7 +2987,7 @@ func TestKeyRollover(t *testing.T) {
 			Payload: `{"oldKey":` + test1KeyPublicJSON + `,"account":"http://localhost/acme/acct/1"}`,
 			ExpectedResponse: `{
 		     "key": ` + string(newJWKJSON) + `,
-		     "status": ""
+		     "status": "valid"
 		   }`,
 			NewKey: newKeyPriv,
 		},