Review fixes pt. 1

core/interfaces.go

@@ -70,7 +70,7 @@ type RegistrationAuthority interface {
 	RevokeCertificateWithReg(ctx context.Context, cert x509.Certificate, code revocation.Reason, regID int64) error
 
 	// [WebFrontEnd]
-	DeactivateRegistration(ctx context.Context, id int64) error
+	DeactivateRegistration(ctx context.Context, reg Registration) error
 
 	// [WebFrontEnd]
 	DeactivateAuthorization(ctx context.Context, auth Authorization) error

mocks/mocks.go

@@ -118,7 +118,13 @@ func (sa *StorageAuthority) GetRegistrationByKey(_ context.Context, jwk jose.Jso
 	contacts := []string{"mailto:person@mail.com"}
 
 	if core.KeyDigestEquals(jwk, test1KeyPublic) {
-		return core.Registration{ID: 1, Key: jwk, Agreement: agreementURL, Contact: &contacts, Status: core.StatusValid}, nil
+		return core.Registration{
+			ID:        1,
+			Key:       jwk,
+			Agreement: agreementURL,
+			Contact:   &contacts,
+			Status:    core.StatusValid,
+		}, nil
 	}
 
 	if core.KeyDigestEquals(jwk, test2KeyPublic) {

ra/ra.go

@@ -1081,8 +1081,15 @@ func (ra *RegistrationAuthorityImpl) onValidationUpdate(ctx context.Context, aut
 }
 
 // DeactivateRegistration deactivates a valid registration
-func (ra *RegistrationAuthorityImpl) DeactivateRegistration(ctx context.Context, id int64) error {
+func (ra *RegistrationAuthorityImpl) DeactivateRegistration(ctx context.Context, reg core.Registration) error {
-	return ra.SA.DeactivateRegistration(ctx, id)
+	if reg.Status != core.StatusValid {
+		return core.MalformedRequestError("Only vaid registrations can be deactivated")
+	}
+	err := ra.SA.DeactivateRegistration(ctx, reg.ID)
+	if err != nil {
+		return core.InternalServerError(err.Error())
+	}
+	return nil
 }
 
 // DeactivateAuthorization deactivates a currently valid authorization

ra/ra_test.go

@@ -1271,7 +1271,11 @@ func TestDeactivateRegistration(t *testing.T) {
 	_, _, ra, _, cleanUp := initAuthorities(t)
 	defer cleanUp()
 
-	err := ra.DeactivateRegistration(context.Background(), 1)
+	err := ra.DeactivateRegistration(context.Background(), core.Registration{ID: 1})
+	test.AssertError(t, err, "DeactivateRegistration failed with a non-valid registration")
+	err = ra.DeactivateRegistration(context.Background(), core.Registration{ID: 1, Status: core.StatusDeactivated})
+	test.AssertError(t, err, "DeactivateRegistration failed with a non-valid registration")
+	err = ra.DeactivateRegistration(context.Background(), core.Registration{ID: 1, Status: core.StatusValid})
 	test.AssertNotError(t, err, "DeactivateRegistration failed")
 	dbReg, err := ra.SA.GetRegistration(context.Background(), 1)
 	test.AssertNotError(t, err, "GetRegistration failed")

rpc/rpc-wrappers.go

@@ -395,13 +395,13 @@ func NewRegistrationAuthorityServer(rpc Server, impl core.RegistrationAuthority,
 	})
 
 	rpc.Handle(MethodDeactivateRegistration, func(ctx context.Context, req []byte) (response []byte, err error) {
-		var drReq deactivateRegistrationRequest
+		var reg core.Registration
-		err = json.Unmarshal(req, &drReq)
+		err = json.Unmarshal(req, &reg)
 		if err != nil {
 			errorCondition(MethodDeactivateRegistration, err, req)
 			return
 		}
-		err = impl.DeactivateRegistration(ctx, drReq.ID)
+		err = impl.DeactivateRegistration(ctx, reg)
 		if err != nil {
 			errorCondition(MethodDeactivateRegistration, err, req)
 			return
@@ -560,9 +560,9 @@ func (rac RegistrationAuthorityClient) DeactivateAuthorization(ctx context.Conte
 	return err
 }
 
-// DeactivateAuthorization deactivates a currently valid registration
+// DeactivateRegistration deactivates a currently valid registration
-func (rac RegistrationAuthorityClient) DeactivateRegistration(ctx context.Context, id int64) error {
+func (rac RegistrationAuthorityClient) DeactivateRegistration(ctx context.Context, reg core.Registration) error {
-	data, err := json.Marshal(deactivateRegistrationRequest{id})
+	data, err := json.Marshal(reg)
 	if err != nil {
 		return err
 	}

sa/_db/migrations/20160818140745_AddRegStatus.sql

@@ -2,7 +2,7 @@
 -- +goose Up
 -- SQL in section 'Up' is executed when this migration is applied
 
-ALTER TABLE `registrations` ADD COLUMN (`status` varchar(255));
+ALTER TABLE `registrations` ADD COLUMN (`status` varchar(255) DEFAULT NULL);
 UPDATE `registrations` SET `status` = 'valid';
 
 -- +goose Down

wfe/wfe.go

@@ -455,7 +455,6 @@ func (wfe *WebFrontEndImpl) verifyPOST(ctx context.Context, logEvent *requestEve
 	}
 
 	if wfe.AllowAccountDeactivation && reg.Status != core.StatusValid {
-		fmt.Println(reg)
 		return nil, nil, reg, probs.Unauthorized("Cannot use a non-valid registration")
 	}
 
@@ -1389,13 +1388,10 @@ func (wfe *WebFrontEndImpl) setCORSHeaders(response http.ResponseWriter, request
 }
 
 func (wfe *WebFrontEndImpl) deactivateRegistration(ctx context.Context, reg core.Registration, response http.ResponseWriter, request *http.Request, logEvent *requestEvent) {
-	if reg.Status != core.StatusValid {
+	err := wfe.RA.DeactivateRegistration(ctx, reg)
-		wfe.sendError(response, logEvent, probs.Malformed("Only valid registrations can be deactivated"), nil)
-		return
-	}
-	err := wfe.RA.DeactivateRegistration(ctx, reg.ID)
 	if err != nil {
-		wfe.sendError(response, logEvent, probs.ServerInternal("Failed to deactivate registration"), err)
+		logEvent.AddError("unable to deactivate registration", err)
+		wfe.sendError(response, logEvent, core.ProblemDetailsForError(err, "Error deactivating registration"), err)
 		return
 	}
 	reg.Status = core.StatusDeactivated

wfe/wfe_test.go

@@ -182,7 +182,7 @@ func (ra *MockRegistrationAuthority) DeactivateAuthorization(ctx context.Context
 	return nil
 }
 
-func (ra *MockRegistrationAuthority) DeactivateRegistration(ctx context.Context, _ int64) error {
+func (ra *MockRegistrationAuthority) DeactivateRegistration(ctx context.Context, _ core.Registration) error {
 	return nil
 }