Add WrongAuthorizationState error code for UpdateAuthorization (#3053)

This commit adds a new boulder error type WrongAuthorizationState.
This error type is returned by the SA when UpdateAuthorization is
provided an authz that isn't pending. The RA and WFE are updated
accordingly such that this error percolates back through the API to the
user as a Malformed problem with a sensible description. Previously this
behaviour resulted in a ServerInternal error.

Resolves #3032

errors/errors.go

@@ -15,6 +15,7 @@ const (
 	RejectedIdentifier
 	InvalidEmail
 	ConnectionFailure
+	WrongAuthorizationState
 )
 
 // BoulderError represents internal Boulder errors
@@ -75,3 +76,7 @@ func InvalidEmailError(msg string, args ...interface{}) error {
 func ConnectionFailureError(msg string, args ...interface{}) error {
 	return New(ConnectionFailure, msg, args...)
 }
+
+func WrongAuthorizationStateError(msg string, args ...interface{}) error {
+	return New(WrongAuthorizationState, msg, args...)
+}

ra/ra.go

@@ -1219,7 +1219,7 @@ func (ra *RegistrationAuthorityImpl) UpdateAuthorization(ctx context.Context, ba
 	if err = ra.SA.UpdatePendingAuthorization(ctx, authz); err != nil {
 		ra.log.Warning(fmt.Sprintf(
 			"Error calling ra.SA.UpdatePendingAuthorization: %s\n", err.Error()))
-		return core.Authorization{}, berrors.InternalServerError("could not update pending authorization")
+		return core.Authorization{}, err
 	}
 	ra.stats.Inc("NewPendingAuthorizations", 1)
 

ra/ra_test.go

@@ -852,6 +852,36 @@ func TestUpdateAuthorizationExpired(t *testing.T) {
 	test.AssertError(t, err, "Updated expired authorization")
 }
 
+func TestUpdateAuthorizationAlreadyValid(t *testing.T) {
+	va, sa, ra, _, cleanUp := initAuthorities(t)
+	defer cleanUp()
+
+	// Create a finalized authorization
+	finalAuthz := AuthzInitial
+	finalAuthz.Status = "valid"
+	exp := ra.clk.Now().Add(365 * 24 * time.Hour)
+	finalAuthz.Expires = &exp
+	finalAuthz.Challenges[0].Status = "valid"
+	finalAuthz.RegistrationID = Registration.ID
+	finalAuthz, err := sa.NewPendingAuthorization(ctx, finalAuthz)
+	test.AssertNotError(t, err, "Could not create pending authorization")
+	err = sa.FinalizeAuthorization(ctx, finalAuthz)
+	test.AssertNotError(t, err, "Could not finalize pending authorization")
+
+	response, err := makeResponse(finalAuthz.Challenges[ResponseIndex])
+	test.AssertNotError(t, err, "Unable to construct response to challenge")
+	finalAuthz.Challenges[ResponseIndex].Type = core.ChallengeTypeDNS01
+	finalAuthz.Challenges[ResponseIndex].Status = core.StatusPending
+	va.RecordsReturn = []core.ValidationRecord{
+		{Hostname: "example.com"}}
+	va.ProblemReturn = nil
+
+	// A subsequent call to update the authorization should return the expected error
+	_, err = ra.UpdateAuthorization(ctx, finalAuthz, ResponseIndex, response)
+	test.Assert(t, berrors.Is(err, berrors.WrongAuthorizationState),
+		"FinalizeAuthorization of valid authz didn't return a berrors.WrongAuthorizationState")
+}
+
 func TestUpdateAuthorizationNewRPC(t *testing.T) {
 	va, sa, ra, _, cleanUp := initAuthorities(t)
 	defer cleanUp()

sa/sa.go

@@ -762,12 +762,12 @@ func (ssa *SQLStorageAuthority) UpdatePendingAuthorization(ctx context.Context,
 	}
 
 	if !statusIsPending(authz.Status) {
-		err = berrors.InternalServerError("authorization is not pending")
+		err = berrors.WrongAuthorizationStateError("authorization is not pending")
 		return Rollback(tx, err)
 	}
 
 	if existingFinal(tx, authz.ID) {
-		err = berrors.InternalServerError("cannot update a finalized authorization")
+		err = berrors.WrongAuthorizationStateError("cannot update a finalized authorization")
 		return Rollback(tx, err)
 	}
 

wfe/probs.go

@@ -25,6 +25,8 @@ func problemDetailsForBoulderError(err *berrors.BoulderError, msg string) *probs
 		return probs.RejectedIdentifier(fmt.Sprintf("%s :: %s", msg, err))
 	case berrors.InvalidEmail:
 		return probs.InvalidEmail(fmt.Sprintf("%s :: %s", msg, err))
+	case berrors.WrongAuthorizationState:
+		return probs.Malformed(fmt.Sprintf("%s :: %s", msg, err))
 	default:
 		// Internal server error messages may include sensitive data, so we do
 		// not include it.

wfe/wfe_test.go

@@ -1123,6 +1123,41 @@ func TestChallenge(t *testing.T) {
 
 }
 
+// MockRAStrictUpdateAuthz is a mock RA that enforces authz status in `UpdateAuthorization`
+type MockRAStrictUpdateAuthz struct {
+	MockRegistrationAuthority
+}
+
+// UpdateAuthorization for a MockRAStrictUpdateAuthz returns a
+// berrors.WrongAuthorizationStateError when told to update a non-pending authz.
+// It returns the authz unchanged for all other cases.
+func (ra *MockRAStrictUpdateAuthz) UpdateAuthorization(_ context.Context, authz core.Authorization, _ int, _ core.Challenge) (core.Authorization, error) {
+	if authz.Status != core.StatusPending {
+		return core.Authorization{}, berrors.WrongAuthorizationStateError("authorization is not pending")
+	}
+	return authz, nil
+}
+
+// TestUpdateChallengeFinalizedAuthz tests that POSTing a challenge associated
+// with an already valid authorization returns the expected Malformed problem.
+func TestUpdateChallengeFinalizedAuthz(t *testing.T) {
+	wfe, _ := setupWFE(t)
+	wfe.RA = &MockRAStrictUpdateAuthz{}
+	responseWriter := httptest.NewRecorder()
+
+	path := "valid/23"
+	wfe.Challenge(ctx, newRequestEvent(), responseWriter,
+		makePostRequestWithPath(path,
+			signRequest(t, `{"resource":"challenge"}`, wfe.nonceService)))
+
+	body := responseWriter.Body.String()
+	test.AssertUnmarshaledEquals(t, body, `{
+  "type": "`+probs.V1ErrorNS+`malformed",
+  "detail": "Unable to update challenge :: authorization is not pending",
+  "status": 400
+}`)
+}
+
 func TestBadNonce(t *testing.T) {
 	wfe, _ := setupWFE(t)
 

wfe2/probs.go

@@ -25,6 +25,8 @@ func problemDetailsForBoulderError(err *berrors.BoulderError, msg string) *probs
 		return probs.RejectedIdentifier(fmt.Sprintf("%s :: %s", msg, err))
 	case berrors.InvalidEmail:
 		return probs.InvalidEmail(fmt.Sprintf("%s :: %s", msg, err))
+	case berrors.WrongAuthorizationState:
+		return probs.Malformed(fmt.Sprintf("%s :: %s", msg, err))
 	default:
 		// Internal server error messages may include sensitive data, so we do
 		// not include it.

wfe2/wfe_test.go

@@ -979,6 +979,41 @@ func TestChallenge(t *testing.T) {
 	}
 }
 
+// MockRAStrictUpdateAuthz is a mock RA that enforces authz status in `UpdateAuthorization`
+type MockRAStrictUpdateAuthz struct {
+	MockRegistrationAuthority
+}
+
+// UpdateAuthorization for a MockRAStrictUpdateAuthz returns a
+// berrors.WrongAuthorizationStateError when told to update a non-pending authz. It
+// returns the authz unchanged for all other cases.
+func (ra *MockRAStrictUpdateAuthz) UpdateAuthorization(_ context.Context, authz core.Authorization, _ int, _ core.Challenge) (core.Authorization, error) {
+	if authz.Status != core.StatusPending {
+		return core.Authorization{}, berrors.WrongAuthorizationStateError("authorization is not pending")
+	}
+	return authz, nil
+}
+
+// TestUpdateChallengeFinalizedAuthz tests that POSTing a challenge associated
+// with an already valid authorization returns the expected Malformed problem.
+func TestUpdateChallengeFinalizedAuthz(t *testing.T) {
+	wfe, _ := setupWFE(t)
+	wfe.RA = &MockRAStrictUpdateAuthz{}
+	responseWriter := httptest.NewRecorder()
+
+	signedURL := "http://localhost/valid/23"
+	_, _, jwsBody := signRequestKeyID(t, 1, nil, signedURL, `{}`, wfe.nonceService)
+	request := makePostRequestWithPath("valid/23", jwsBody)
+	wfe.Challenge(ctx, newRequestEvent(), responseWriter, request)
+
+	body := responseWriter.Body.String()
+	test.AssertUnmarshaledEquals(t, body, `{
+  "type": "`+probs.V2ErrorNS+`malformed",
+  "detail": "Unable to update challenge :: authorization is not pending",
+  "status": 400
+}`)
+}
+
 func TestBadNonce(t *testing.T) {
 	wfe, _ := setupWFE(t)