From 117ea763b487fa006430df37054b137d6f379809 Mon Sep 17 00:00:00 2001 From: johnabass Date: Tue, 5 Mar 2024 16:09:31 -0800 Subject: [PATCH 1/5] added an Error interface to consistently expose information about failures --- error.go | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 error.go diff --git a/error.go b/error.go new file mode 100644 index 0000000..d82777d --- /dev/null +++ b/error.go @@ -0,0 +1,48 @@ +// SPDX-FileCopyrightText: 2021 Comcast Cable Communications Management, LLC +// SPDX-License-Identifier: Apache-2.0 + +package bascule + +// ErrorType is an enumeration type for various types of security errors. +// This type can be used to determine more detail about the context of an error. +type ErrorType int + +const ( + // ErrorTypeUnknown indicates an error that didn't specify an ErrorType, + // possibly because the error didn't implement the Error interface in this package. + ErrorTypeUnknown ErrorType = iota + + // ErrorTypeMissingCredentials indicates that no credentials could be found. + // For example, this is the type used when no credentials are present in an HTTP request. + ErrorTypeMissingCredentials + + // ErrorTypeBadCredentials indcates that credentials exist, but they were badly formatted. + // In other words, bascule could not parse the credentials. + ErrorTypeBadCredentials + + // ErrorTypeInvalidCredentials indicates that credentials exist and are properly formatted, + // but they failed validation. Typically, this is due to failed authentication. It can also + // mean that a token's fields are invalid, such as the exp field of a JWT. + ErrorTypeInvalidCredentials + + // ErrorTypeUnauthorized indicates that a token did not have sufficient privileges to + // perform an operation. + ErrorTypeUnauthorized +) + +// Error is an optional interface that errors may implement to expose security +// metadata about the error. +type Error interface { + // Type is the ErrorType describing this error. + Type() ErrorType +} + +// GetErrorType examines err to determine its associated metadata type. If err +// does not implement Error, this function returns ErrorTypeUnknown. +func GetErrorType(err error) ErrorType { + if e, ok := err.(Error); ok { + return e.Type() + } + + return ErrorTypeUnknown +} From a1d4530577bcb965856ad3944b5d41cda2d53bcc Mon Sep 17 00:00:00 2001 From: johnabass Date: Tue, 5 Mar 2024 20:09:50 -0800 Subject: [PATCH 2/5] refactored errors to better allow generic error handling --- basculehttp/credentials.go | 2 +- basculehttp/credentials_test.go | 2 +- credentials.go | 43 ---------------------------- credentials_test.go | 31 -------------------- error.go | 44 ++++++++++++++++++++++++++++- error_test.go | 50 +++++++++++++++++++++++++++++++++ 6 files changed, 95 insertions(+), 77 deletions(-) create mode 100644 error_test.go diff --git a/basculehttp/credentials.go b/basculehttp/credentials.go index fc1b46a..e654eee 100644 --- a/basculehttp/credentials.go +++ b/basculehttp/credentials.go @@ -28,7 +28,7 @@ var defaultCredentialsParser bascule.CredentialsParser = bascule.CredentialsPars Value: value, } } else { - err = &bascule.InvalidCredentialsError{ + err = &bascule.BadCredentialsError{ Raw: raw, } } diff --git a/basculehttp/credentials_test.go b/basculehttp/credentials_test.go index ef19078..73f8534 100644 --- a/basculehttp/credentials_test.go +++ b/basculehttp/credentials_test.go @@ -67,7 +67,7 @@ func (suite *CredentialsTestSuite) testDefaultCredentialsParserFailure() { suite.Require().Error(err) suite.Equal(bascule.Credentials{}, creds) - var ice *bascule.InvalidCredentialsError + var ice *bascule.BadCredentialsError if suite.ErrorAs(err, &ice) { suite.Equal(testCase, ice.Raw) } diff --git a/credentials.go b/credentials.go index 2ac2cce..c7f689f 100644 --- a/credentials.go +++ b/credentials.go @@ -3,49 +3,6 @@ package bascule -import "strings" - -// InvalidCredentialsError is returned typically by CredentialsParser.Parse -// to indicate that a raw, serialized credentials were badly formatted. -type InvalidCredentialsError struct { - // Cause represents any lower-level error that occurred, if any. - Cause error - - // Raw represents the raw credentials that couldn't be parsed. - Raw string -} - -func (err *InvalidCredentialsError) Unwrap() error { return err.Cause } - -func (err *InvalidCredentialsError) Error() string { - var o strings.Builder - o.WriteString(`Invalid credentials "`) - o.WriteString(err.Raw) - o.WriteString(`"`) - - if err.Cause != nil { - o.WriteString(": ") - o.WriteString(err.Cause.Error()) - } - - return o.String() -} - -// UnsupportedSchemeError indicates that a credential scheme was not -// supported via the particular way bascule was configured. -type UnsupportedSchemeError struct { - // Scheme is the authorization scheme that wasn't supported. - Scheme Scheme -} - -func (err *UnsupportedSchemeError) Error() string { - var o strings.Builder - o.WriteString(`Unsupported credential scheme: "`) - o.WriteString(string(err.Scheme)) - o.WriteString(`"`) - return o.String() -} - // Scheme represents how a security token should be parsed. For HTTP, examples // of a scheme are "Bearer" and "Basic". type Scheme string diff --git a/credentials_test.go b/credentials_test.go index 1afe63b..5aa25bd 100644 --- a/credentials_test.go +++ b/credentials_test.go @@ -14,37 +14,6 @@ type CredentialsTestSuite struct { suite.Suite } -func (suite *CredentialsTestSuite) TestInvalidCredentialsError() { - suite.Run("WithCause", func() { - cause := errors.New("cause") - err := InvalidCredentialsError{ - Cause: cause, - Raw: "raw", - } - - suite.Same(err.Unwrap(), cause) - suite.Contains(err.Error(), "cause") - suite.Contains(err.Error(), "raw") - }) - - suite.Run("NoCause", func() { - err := InvalidCredentialsError{ - Raw: "raw", - } - - suite.Nil(err.Unwrap()) - suite.Contains(err.Error(), "raw") - }) -} - -func (suite *CredentialsTestSuite) TestUnsupportedSchemeError() { - err := UnsupportedSchemeError{ - Scheme: Scheme("scheme"), - } - - suite.Contains(err.Error(), "scheme") -} - func (suite *CredentialsTestSuite) TestCredentialsParserFunc() { const expectedRaw = "expected raw credentials" expectedErr := errors.New("expected error") diff --git a/error.go b/error.go index d82777d..3c2e08b 100644 --- a/error.go +++ b/error.go @@ -3,6 +3,11 @@ package bascule +import ( + "errors" + "strings" +) + // ErrorType is an enumeration type for various types of security errors. // This type can be used to determine more detail about the context of an error. type ErrorType int @@ -40,9 +45,46 @@ type Error interface { // GetErrorType examines err to determine its associated metadata type. If err // does not implement Error, this function returns ErrorTypeUnknown. func GetErrorType(err error) ErrorType { - if e, ok := err.(Error); ok { + var e Error + if errors.As(err, &e) { return e.Type() } return ErrorTypeUnknown } + +// UnsupportedSchemeError indicates that a credentials scheme was not supported +// by a TokenParser. +type UnsupportedSchemeError struct { + // Scheme is the unsupported credential scheme. + Scheme Scheme +} + +// Type tags errors of this type as ErrorTypeBadCredentials. +func (err *UnsupportedSchemeError) Type() ErrorType { return ErrorTypeBadCredentials } + +func (err *UnsupportedSchemeError) Error() string { + var o strings.Builder + o.WriteString(`Unsupported scheme: "`) + o.WriteString(string(err.Scheme)) + o.WriteRune('"') + return o.String() +} + +// BadCredentialsError is a general-purpose error indicating that credentials +// could not be parsed. +type BadCredentialsError struct { + // Raw is the raw value of the credentials that could not be parsed. + Raw string +} + +// Type tags errors of this type as ErrorTypeBadCredentials. +func (err *BadCredentialsError) Type() ErrorType { return ErrorTypeBadCredentials } + +func (err *BadCredentialsError) Error() string { + var o strings.Builder + o.WriteString(`Bad credentials: "`) + o.WriteString(err.Raw) + o.WriteRune('"') + return o.String() +} diff --git a/error_test.go b/error_test.go new file mode 100644 index 0000000..fb4c274 --- /dev/null +++ b/error_test.go @@ -0,0 +1,50 @@ +package bascule + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/suite" +) + +type ErrorSuite struct { + suite.Suite +} + +func (suite *ErrorSuite) TestUnsupportedSchemeError() { + err := UnsupportedSchemeError{ + Scheme: Scheme("scheme"), + } + + suite.Contains(err.Error(), "scheme") + suite.Equal(ErrorTypeBadCredentials, err.Type()) +} + +func (suite *ErrorSuite) TestBadCredentialsError() { + err := BadCredentialsError{ + Raw: "these are an unparseable, raw credentials", + } + + suite.Contains(err.Error(), "these are an unparseable, raw credentials") + suite.Equal(ErrorTypeBadCredentials, err.Type()) +} + +func (suite *ErrorSuite) TestGetErrorType() { + suite.Run("Unknown", func() { + suite.Equal( + ErrorTypeUnknown, + GetErrorType(errors.New("this is an error that is unknown to bascule")), + ) + }) + + suite.Run("ImplementsError", func() { + suite.Equal( + ErrorTypeBadCredentials, + GetErrorType(new(BadCredentialsError)), + ) + }) +} + +func TestError(t *testing.T) { + suite.Run(t, new(ErrorSuite)) +} From 3dbe4611265b3dff0b01b0c493964c6b8bdf0762 Mon Sep 17 00:00:00 2001 From: johnabass Date: Wed, 6 Mar 2024 10:59:06 -0800 Subject: [PATCH 3/5] chore: delint --- basculehttp/credentials_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/basculehttp/credentials_test.go b/basculehttp/credentials_test.go index 73f8534..7b2e82c 100644 --- a/basculehttp/credentials_test.go +++ b/basculehttp/credentials_test.go @@ -17,7 +17,7 @@ type CredentialsTestSuite struct { func (suite *CredentialsTestSuite) testDefaultCredentialsParserSuccess() { const ( expectedScheme bascule.Scheme = "Test" - expectedValue = "credentialValue" + expectedValue string = "credentialValue" ) testCases := []string{ @@ -43,11 +43,6 @@ func (suite *CredentialsTestSuite) testDefaultCredentialsParserSuccess() { } func (suite *CredentialsTestSuite) testDefaultCredentialsParserFailure() { - const ( - expectedScheme bascule.Scheme = "Test" - expectedValue = "credentialValue" - ) - testCases := []string{ "", " ", From 5c1120ad85fbf4cf2ef2af6cc4390d7b860b7805 Mon Sep 17 00:00:00 2001 From: johnabass Date: Wed, 6 Mar 2024 11:15:00 -0800 Subject: [PATCH 4/5] feat: Error interface carries additional metadata --- error.go | 23 +++++++++++++++++++++-- error_test.go | 12 ++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/error.go b/error.go index 3c2e08b..7d29d6f 100644 --- a/error.go +++ b/error.go @@ -30,9 +30,9 @@ const ( // mean that a token's fields are invalid, such as the exp field of a JWT. ErrorTypeInvalidCredentials - // ErrorTypeUnauthorized indicates that a token did not have sufficient privileges to + // ErrorTypeForbidden indicates that a token did not have sufficient privileges to // perform an operation. - ErrorTypeUnauthorized + ErrorTypeForbidden ) // Error is an optional interface that errors may implement to expose security @@ -42,6 +42,25 @@ type Error interface { Type() ErrorType } +type typedError struct { + error + et ErrorType +} + +func (te *typedError) Unwrap() error { return te.error } + +func (te *typedError) Type() ErrorType { return te.et } + +// NewTypedError wraps a given error and associates an ErrorType with it. +// The returned error will implement the Error interface in this package, +// and will have an Unwrap method that returns err. +func NewTypedError(err error, et ErrorType) error { + return &typedError{ + error: err, + et: et, + } +} + // GetErrorType examines err to determine its associated metadata type. If err // does not implement Error, this function returns ErrorTypeUnknown. func GetErrorType(err error) ErrorType { diff --git a/error_test.go b/error_test.go index fb4c274..3169fe9 100644 --- a/error_test.go +++ b/error_test.go @@ -29,6 +29,18 @@ func (suite *ErrorSuite) TestBadCredentialsError() { suite.Equal(ErrorTypeBadCredentials, err.Type()) } +func (suite *ErrorSuite) TestNewTypedError() { + original := errors.New("original error") + typed := NewTypedError(original, ErrorTypeBadCredentials) + + suite.ErrorIs(typed, original) + suite.Require().Implements((*Error)(nil), typed) + suite.Equal( + ErrorTypeBadCredentials, + typed.(Error).Type(), + ) +} + func (suite *ErrorSuite) TestGetErrorType() { suite.Run("Unknown", func() { suite.Equal( From 0c5b72b5dc4f4f029911cc016367c3a2b581422a Mon Sep 17 00:00:00 2001 From: johnabass Date: Wed, 6 Mar 2024 14:57:45 -0800 Subject: [PATCH 5/5] chore: delint --- basculehttp/error.go | 22 ++++++++++++++++++++-- error_test.go | 5 ++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/basculehttp/error.go b/basculehttp/error.go index ac92447..b96d6f5 100644 --- a/basculehttp/error.go +++ b/basculehttp/error.go @@ -8,6 +8,8 @@ import ( "encoding/json" "errors" "net/http" + + "github.com/xmidt-org/bascule/v1" ) // ErrorStatusCoder is a strategy for determining the HTTP response code for an error. @@ -18,12 +20,28 @@ import ( type ErrorStatusCoder func(request *http.Request, defaultCode int, err error) int // DefaultErrorStatusCoder is the strategy used when no ErrorStatusCoder is supplied. -// This function examines err to see if it or any wrapped error provides a StatusCode() -// method. If found, Status() is used. Otherwise, this function returns the default code. +// This function first tries to see if the error implements bascule.Error, in which case +// the error's type will dictate the response code. Next, if the wrapper error provides +// a StatusCode() method, that code is used. Failing all of that, the defaultCode is +// returned. // // This function can also be decorated. Passing a sentinel value for defaultCode allows // a decorator to take further action. func DefaultErrorStatusCoder(_ *http.Request, defaultCode int, err error) int { + switch bascule.GetErrorType(err) { + case bascule.ErrorTypeMissingCredentials: + return http.StatusUnauthorized + + case bascule.ErrorTypeBadCredentials: + return http.StatusBadRequest + + case bascule.ErrorTypeInvalidCredentials: + return http.StatusForbidden + + case bascule.ErrorTypeForbidden: + return http.StatusForbidden + } + type statusCoder interface { StatusCode() int } diff --git a/error_test.go b/error_test.go index 3169fe9..5861d9a 100644 --- a/error_test.go +++ b/error_test.go @@ -35,9 +35,12 @@ func (suite *ErrorSuite) TestNewTypedError() { suite.ErrorIs(typed, original) suite.Require().Implements((*Error)(nil), typed) + + var e Error + suite.Require().ErrorAs(typed, &e) suite.Equal( ErrorTypeBadCredentials, - typed.(Error).Type(), + e.Type(), ) }