From dbf9afa7d6753246eb25686968454cbd0ed8d13c Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Thu, 25 Aug 2016 16:28:58 -0700 Subject: [PATCH] Review fixes pt. 1 --- core/interfaces.go | 2 +- mocks/mocks.go | 8 +++++++- ra/ra.go | 11 +++++++++-- ra/ra_test.go | 6 +++++- rpc/rpc-wrappers.go | 12 ++++++------ sa/_db/migrations/20160818140745_AddRegStatus.sql | 2 +- wfe/wfe.go | 10 +++------- wfe/wfe_test.go | 2 +- 8 files changed, 33 insertions(+), 20 deletions(-) diff --git a/core/interfaces.go b/core/interfaces.go index ccdd35fc8..d7c6c05f6 100644 --- a/core/interfaces.go +++ b/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 diff --git a/mocks/mocks.go b/mocks/mocks.go index 2d2784a55..1049b32c0 100644 --- a/mocks/mocks.go +++ b/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) { diff --git a/ra/ra.go b/ra/ra.go index 487b418fe..40b382031 100644 --- a/ra/ra.go +++ b/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 { - return ra.SA.DeactivateRegistration(ctx, id) +func (ra *RegistrationAuthorityImpl) DeactivateRegistration(ctx context.Context, reg core.Registration) error { + 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 diff --git a/ra/ra_test.go b/ra/ra_test.go index b557b41c6..792edd68f 100644 --- a/ra/ra_test.go +++ b/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") diff --git a/rpc/rpc-wrappers.go b/rpc/rpc-wrappers.go index 4806bd086..84d45ea56 100644 --- a/rpc/rpc-wrappers.go +++ b/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 - err = json.Unmarshal(req, &drReq) + var reg core.Registration + err = json.Unmarshal(req, ®) 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 -func (rac RegistrationAuthorityClient) DeactivateRegistration(ctx context.Context, id int64) error { - data, err := json.Marshal(deactivateRegistrationRequest{id}) +// DeactivateRegistration deactivates a currently valid registration +func (rac RegistrationAuthorityClient) DeactivateRegistration(ctx context.Context, reg core.Registration) error { + data, err := json.Marshal(reg) if err != nil { return err } diff --git a/sa/_db/migrations/20160818140745_AddRegStatus.sql b/sa/_db/migrations/20160818140745_AddRegStatus.sql index 154204f42..962af664a 100644 --- a/sa/_db/migrations/20160818140745_AddRegStatus.sql +++ b/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 diff --git a/wfe/wfe.go b/wfe/wfe.go index 137c55b41..5c3a4a053 100644 --- a/wfe/wfe.go +++ b/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 { - wfe.sendError(response, logEvent, probs.Malformed("Only valid registrations can be deactivated"), nil) - return - } - err := wfe.RA.DeactivateRegistration(ctx, reg.ID) + err := wfe.RA.DeactivateRegistration(ctx, reg) 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 diff --git a/wfe/wfe_test.go b/wfe/wfe_test.go index 2e58cf8c2..4033605a0 100644 --- a/wfe/wfe_test.go +++ b/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 }