From ecd1ea6c61e909d75ab26c6237ee9863d301f196 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 23 May 2019 22:41:55 -0400 Subject: [PATCH] Implement suberrors & subproblems (#4227) Updates #4193 Updating relevant Boulder locations to use WithSubErrors and WithSubProblems will be done in a separate follow-up PR. --- errors/errors.go | 28 +++++++++++++++++++++--- errors/errors_test.go | 50 ++++++++++++++++++++++++++++++++++++++++++ probs/probs.go | 24 ++++++++++++++++++++ probs/probs_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 errors/errors_test.go diff --git a/errors/errors.go b/errors/errors.go index 4a781a5d1..eafc8a347 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -1,6 +1,10 @@ package errors -import "fmt" +import ( + "fmt" + + "github.com/letsencrypt/boulder/identifier" +) // ErrorType provides a coarse category for BoulderErrors type ErrorType int @@ -24,14 +28,32 @@ const ( // BoulderError represents internal Boulder errors type BoulderError struct { - Type ErrorType - Detail string + Type ErrorType + Detail string + SubErrors []SubBoulderError +} + +// SubBoulderError represents sub-errors specific to an identifier that are +// related to a top-level internal Boulder error. +type SubBoulderError struct { + *BoulderError + Identifier identifier.ACMEIdentifier } func (be *BoulderError) Error() string { return be.Detail } +// WithSubErrors returns a new BoulderError instance created by adding the +// provided subErrs to the existing BoulderError. +func (be *BoulderError) WithSubErrors(subErrs []SubBoulderError) *BoulderError { + return &BoulderError{ + Type: be.Type, + Detail: be.Detail, + SubErrors: append(be.SubErrors, subErrs...), + } +} + // New is a convenience function for creating a new BoulderError func New(errType ErrorType, msg string, args ...interface{}) error { return &BoulderError{ diff --git a/errors/errors_test.go b/errors/errors_test.go new file mode 100644 index 000000000..b461addbc --- /dev/null +++ b/errors/errors_test.go @@ -0,0 +1,50 @@ +package errors + +import ( + "testing" + + "github.com/letsencrypt/boulder/identifier" + "github.com/letsencrypt/boulder/test" +) + +// TestWithSubErrors tests that a boulder error can be created by adding +// suberrors to an existing top level boulder error +func TestWithSubErrors(t *testing.T) { + topErr := &BoulderError{ + Type: RateLimit, + Detail: "don't you think you have enough certificates already?", + } + + subErrs := []SubBoulderError{ + SubBoulderError{ + Identifier: identifier.DNSIdentifier("example.com"), + BoulderError: &BoulderError{ + Type: RateLimit, + Detail: "everyone uses this example domain", + }, + }, + SubBoulderError{ + Identifier: identifier.DNSIdentifier("what about example.com"), + BoulderError: &BoulderError{ + Type: RateLimit, + Detail: "try a real identifier value next time", + }, + }, + } + + outResult := topErr.WithSubErrors(subErrs) + // The outResult should be a new, distinct error + test.AssertNotEquals(t, topErr, outResult) + // The outResult error should have the correct sub errors + test.AssertDeepEquals(t, outResult.SubErrors, subErrs) + // Adding another suberr shouldn't squash the original sub errors + anotherSubErr := SubBoulderError{ + Identifier: identifier.DNSIdentifier("another ident"), + BoulderError: &BoulderError{ + Type: RateLimit, + Detail: "another rate limit err", + }, + } + outResult = outResult.WithSubErrors([]SubBoulderError{anotherSubErr}) + test.AssertDeepEquals(t, outResult.SubErrors, append(subErrs, anotherSubErr)) +} diff --git a/probs/probs.go b/probs/probs.go index 2c3a69a83..d3f3f0159 100644 --- a/probs/probs.go +++ b/probs/probs.go @@ -3,6 +3,8 @@ package probs import ( "fmt" "net/http" + + "github.com/letsencrypt/boulder/identifier" ) // Error types that can be used in ACME payloads @@ -40,12 +42,34 @@ type ProblemDetails struct { // HTTPStatus is the HTTP status code the ProblemDetails should probably be sent // as. HTTPStatus int `json:"status,omitempty"` + // SubProblems are optional additional per-identifier problems. See + // RFC 8555 Section 6.7.1: https://tools.ietf.org/html/rfc8555#section-6.7.1 + SubProblems []SubProblemDetails `json:"subproblems,omitempty"` +} + +// SubProblemDetails represents sub-problems specific to an identifier that are +// related to a top-level ProblemDetails. +// See RFC 8555 Section 6.7.1: https://tools.ietf.org/html/rfc8555#section-6.7.1 +type SubProblemDetails struct { + ProblemDetails + Identifier identifier.ACMEIdentifier } func (pd *ProblemDetails) Error() string { return fmt.Sprintf("%s :: %s", pd.Type, pd.Detail) } +// WithSubProblems returns a new ProblemsDetails instance created by adding the +// provided subProbs to the existing ProblemsDetail. +func (pd *ProblemDetails) WithSubProblems(subProbs []SubProblemDetails) *ProblemDetails { + return &ProblemDetails{ + Type: pd.Type, + Detail: pd.Detail, + HTTPStatus: pd.HTTPStatus, + SubProblems: append(pd.SubProblems, subProbs...), + } +} + // statusTooManyRequests is the HTTP status code meant for rate limiting // errors. It's not currently in the net/http library so we add it here. const statusTooManyRequests = 429 diff --git a/probs/probs_test.go b/probs/probs_test.go index 43dc10996..040af219f 100644 --- a/probs/probs_test.go +++ b/probs/probs_test.go @@ -5,6 +5,7 @@ import ( "net/http" + "github.com/letsencrypt/boulder/identifier" "github.com/letsencrypt/boulder/test" ) @@ -77,5 +78,55 @@ func TestProblemDetailsConvenience(t *testing.T) { if c.pb.Detail != c.detail { t.Errorf("Incorrect detail message. Expected %s got %s", c.detail, c.pb.Detail) } + + if subProbLen := len(c.pb.SubProblems); subProbLen != 0 { + t.Errorf("Incorrect SubProblems. Expected 0, found %d", subProbLen) + } } } + +// TestWithSubProblems tests that a new problem can be constructed by adding +// subproblems. +func TestWithSubProblems(t *testing.T) { + topProb := &ProblemDetails{ + Type: RateLimitedProblem, + Detail: "don't you think you have enough certificates already?", + HTTPStatus: statusTooManyRequests, + } + subProbs := []SubProblemDetails{ + SubProblemDetails{ + Identifier: identifier.DNSIdentifier("example.com"), + ProblemDetails: ProblemDetails{ + Type: RateLimitedProblem, + Detail: "don't you think you have enough certificates already?", + HTTPStatus: statusTooManyRequests, + }, + }, + SubProblemDetails{ + Identifier: identifier.DNSIdentifier("what about example.com"), + ProblemDetails: ProblemDetails{ + Type: MalformedProblem, + Detail: "try a real identifier value next time", + HTTPStatus: http.StatusConflict, + }, + }, + } + + outResult := topProb.WithSubProblems(subProbs) + + // The outResult should be a new, distinct problem details instance + test.AssertNotEquals(t, topProb, outResult) + // The outResult problem details should have the correct sub problems + test.AssertDeepEquals(t, outResult.SubProblems, subProbs) + // Adding another sub problem shouldn't squash the original sub problems + anotherSubProb := SubProblemDetails{ + Identifier: identifier.DNSIdentifier("another ident"), + ProblemDetails: ProblemDetails{ + Type: RateLimitedProblem, + Detail: "yet another rate limit err", + HTTPStatus: statusTooManyRequests, + }, + } + outResult = outResult.WithSubProblems([]SubProblemDetails{anotherSubProb}) + test.AssertDeepEquals(t, outResult.SubProblems, append(subProbs, anotherSubProb)) +}