Fix error when deactivating an account (#7899)

The RA's DeactivateAccount method expects the account provided to it by
the WFE to still have status Valid. The new WFE deactivation code was
hardcoding the status to Deactivated. Fix the WFE to pass the account's
current status instead.

Add an integration test to confirm both the breakage and the fix. Also
leave behind some TODOs to simplify this codepath further, and not
require the status to be provided at all.

Part of #5554

ra/ra.go

@@ -2442,6 +2442,9 @@ func (ra *RegistrationAuthorityImpl) DeactivateRegistration(ctx context.Context,
 	if reg == nil || reg.Id == 0 {
 		return nil, errIncompleteGRPCRequest
 	}
+	// TODO(#5554): Remove this check: this is only enforcing that the WFE has
+	// told us the correct status. The SA will enforce that the current status is
+	// valid during its database update.
 	if reg.Status != string(core.StatusValid) {
 		return nil, berrors.MalformedError("only valid registrations can be deactivated")
 	}
@@ -2449,6 +2452,8 @@ func (ra *RegistrationAuthorityImpl) DeactivateRegistration(ctx context.Context,
 	if err != nil {
 		return nil, err
 	}
+
+	// TODO(#5554): Return the updated account object.
 	return &emptypb.Empty{}, nil
 }
 

sa/sa.go

@@ -549,6 +549,8 @@ func (ssa *SQLStorageAuthority) DeactivateRegistration(ctx context.Context, req
 	if err != nil {
 		return nil, err
 	}
+
+	// TODO(#5554): Return the updated account object.
 	return &emptypb.Empty{}, nil
 }
 

test/integration/account_test.go

@@ -0,0 +1,50 @@
+//go:build integration
+
+package integration
+
+import (
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
+	"testing"
+
+	"github.com/eggsampler/acme/v3"
+
+	"github.com/letsencrypt/boulder/core"
+)
+
+// TestAccountDeactivate tests that account deactivation works. It does not test
+// that we reject requests for other account statuses, because eggsampler/acme
+// wisely does not allow us to construct such malformed requests.
+func TestAccountDeactivate(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)
+	}
+
+	account, err := c.NewAccount(acctKey, false, true, "mailto:hello@blackhole.net")
+	if err != nil {
+		t.Fatalf("failed to create initial account: %s", err)
+	}
+
+	got, err := c.DeactivateAccount(account)
+	if err != nil {
+		t.Errorf("unexpected error while deactivating account: %s", err)
+	}
+
+	if got.Status != string(core.StatusDeactivated) {
+		t.Errorf("account deactivation should have set status to %q, instead got %q", core.StatusDeactivated, got.Status)
+	}
+
+	// TODO(#5554): Check that the contacts have been cleared. We can't do this
+	// today because eggsampler/acme unmarshals the WFE's response into the same
+	// account object as it used to make the request, and a wholly missing
+	// contacts field doesn't overwrite whatever eggsampler was holding in memory.
+}

wfe2/wfe.go

@@ -1466,12 +1466,17 @@ func (wfe *WebFrontEndImpl) updateAccount(
 	// their contacts, the deactivation will take place and return before an
 	// update would be performed. Deactivation deletes the contacts field.
 	if accountUpdateRequest.Status == core.StatusDeactivated {
-		_, err = wfe.ra.DeactivateRegistration(ctx, &corepb.Registration{Id: currAcct.ID, Status: string(core.StatusDeactivated)})
+		// TODO(#5554): Remove the need to pass Status here: we wouldn't have reached
+		// this point unless the requesting account was valid.
+		_, err = wfe.ra.DeactivateRegistration(ctx, &corepb.Registration{Id: currAcct.ID, Status: string(currAcct.Status)})
 		if err != nil {
 			return nil, web.ProblemDetailsForError(err, "Unable to deactivate account")
 		}
 
+		// TODO(#5554): Have DeactivateRegistration return the updated account
+		// object, so we don't have to modify it ourselves.
 		currAcct.Status = core.StatusDeactivated
+		currAcct.Contact = nil
 		return currAcct, nil
 	}
 

wfe2/wfe_test.go

@@ -2756,9 +2756,6 @@ func TestDeactivateAccount(t *testing.T) {
 		    "n": "yNWVhtYEKJR21y9xsHV-PD_bYwbXSeNuFal46xYxVfRL5mqha7vttvjB_vc7Xg2RvgCxHPCqoxgMPTzHrZT75LjCwIW2K_klBYN8oYvTwwmeSkAz6ut7ZxPv-nZaT5TJhGk0NT2kh_zSpdriEJ_3vW-mqxYbbBmpvHqsa1_zx9fSuHYctAZJWzxzUZXykbWMWQZpEiE0J4ajj51fInEzVn7VxV-mzfMyboQjujPh7aNJxAWSq4oQEJJDgWwSh9leyoJoPpONHxh5nEE5AjE01FkGICSxjpZsF-w8hOTI3XXohUdu29Se26k2B0PolDSuj0GIQU6-W9TdLXSjBb2SpQ",
 		    "e": "AQAB"
 		  },
-		  "contact": [
-		    "mailto:person@mail.com"
-		  ],
 		  "status": "deactivated"
 		}`)
 
@@ -2775,9 +2772,6 @@ func TestDeactivateAccount(t *testing.T) {
 		    "n": "yNWVhtYEKJR21y9xsHV-PD_bYwbXSeNuFal46xYxVfRL5mqha7vttvjB_vc7Xg2RvgCxHPCqoxgMPTzHrZT75LjCwIW2K_klBYN8oYvTwwmeSkAz6ut7ZxPv-nZaT5TJhGk0NT2kh_zSpdriEJ_3vW-mqxYbbBmpvHqsa1_zx9fSuHYctAZJWzxzUZXykbWMWQZpEiE0J4ajj51fInEzVn7VxV-mzfMyboQjujPh7aNJxAWSq4oQEJJDgWwSh9leyoJoPpONHxh5nEE5AjE01FkGICSxjpZsF-w8hOTI3XXohUdu29Se26k2B0PolDSuj0GIQU6-W9TdLXSjBb2SpQ",
 		    "e": "AQAB"
 		  },
-		  "contact": [
-		    "mailto:person@mail.com"
-		  ],
 		  "status": "deactivated"
 		}`)