return reason as a special error (#97)

* return reason as a special error

* update changelog

* better errors, testing from code review comments

* improved some other unit tests too, added more explanation

* added short unit test for special error

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Removed emperror package dependency. [#94](https://github.com/xmidt-org/bascule/pull/94)
 - Converted basculechecks to use touchstone metrics. [#95](https://github.com/xmidt-org/bascule/pull/95)
 - Added method label to metric validator. [#96](https://github.com/xmidt-org/bascule/pull/96)
+- Update errors to include reason used by metric validator. [#97](https://github.com/xmidt-org/bascule/pull/97)
 
 ## [v0.9.0]
 - added helper function for building basic auth map [#59](https://github.com/xmidt-org/bascule/pull/59)

basculechecks/capabilitiesmap.go

@@ -26,7 +26,10 @@ import (
 
 var (
 	ErrNilDefaultChecker = errors.New("default checker cannot be nil")
-	ErrEmptyEndpoint     = errors.New("endpoint provided is empty")
+	ErrEmptyEndpoint     = errWithReason{
+		err:    errors.New("endpoint provided is empty"),
+		reason: EmptyParsedURL,
+	}
 )
 
 // CapabilitiesMap runs a capability check based on the value of the parsedURL,
@@ -43,22 +46,22 @@ type CapabilitiesMap struct {
 // EndpointChecker for the endpoint, the default is used.  As long as one
 // capability is found to be authorized by the EndpointChecker, no error is
 // returned.
-func (c CapabilitiesMap) CheckAuthentication(auth bascule.Authentication, vs ParsedValues) (string, error) {
+func (c CapabilitiesMap) CheckAuthentication(auth bascule.Authentication, vs ParsedValues) error {
 	if auth.Token == nil {
-		return MissingValues, ErrNoToken
+		return ErrNoToken
 	}
 
 	if auth.Request.URL == nil {
-		return MissingValues, ErrNoURL
+		return ErrNoURL
 	}
 
 	if vs.Endpoint == "" {
-		return EmptyParsedURL, ErrEmptyEndpoint
+		return ErrEmptyEndpoint
 	}
 
-	capabilities, reason, err := getCapabilities(auth.Token.Attributes())
+	capabilities, err := getCapabilities(auth.Token.Attributes())
 	if err != nil {
-		return reason, err
+		return err
 	}
 
 	// determine which EndpointChecker to use.
@@ -72,19 +75,19 @@ func (c CapabilitiesMap) CheckAuthentication(auth bascule.Authentication, vs Par
 	// if the checker is nil, we treat it like a checker that always returns
 	// false.
 	if checker == nil {
-		return NoCapabilitiesMatch, fmt.Errorf("%w in [%v] with nil endpoint checker",
+		// ErrNoValidCapabilityFound is a Reasoner.
+		return fmt.Errorf("%w in [%v] with nil endpoint checker",
 			ErrNoValidCapabilityFound, capabilities)
 	}
 
 	// if one of the capabilities is good, then the request is authorized
 	// for this endpoint.
 	for _, capability := range capabilities {
 		if checker.Authorized(capability, reqURL, method) {
-			return "", nil
+			return nil
 		}
 	}
 
-	return NoCapabilitiesMatch, fmt.Errorf("%w in [%v] with %v endpoint checker",
+	return fmt.Errorf("%w in [%v] with %v endpoint checker",
 		ErrNoValidCapabilityFound, capabilities, checker.Name())
-
 }

basculechecks/capabilitiesmap_test.go

@@ -18,6 +18,8 @@
 package basculechecks
 
 import (
+	"errors"
+	"fmt"
 	"net/url"
 	"testing"
 
@@ -57,13 +59,12 @@ func TestCapabilitiesMapCheck(t *testing.T) {
 		bascule.NewAttributes(map[string]interface{}{CapabilityKey: defaultCapabilities}))
 	badToken := bascule.NewToken("", "", nil)
 	tests := []struct {
-		description    string
-		cm             CapabilitiesMap
-		token          bascule.Token
-		includeURL     bool
-		endpoint       string
-		expectedReason string
-		expectedErr    error
+		description string
+		cm          CapabilitiesMap
+		token       bascule.Token
+		includeURL  bool
+		endpoint    string
+		expectedErr error
 	}{
 		{
 			description: "Success",
@@ -87,65 +88,58 @@ func TestCapabilitiesMapCheck(t *testing.T) {
 			endpoint:    "fallback",
 		},
 		{
-			description:    "No Match Error",
-			cm:             cm,
-			token:          goodToken,
-			includeURL:     true,
-			endpoint:       "b",
-			expectedReason: NoCapabilitiesMatch,
-			expectedErr:    ErrNoValidCapabilityFound,
+			description: "No Match Error",
+			cm:          cm,
+			token:       goodToken,
+			includeURL:  true,
+			endpoint:    "b",
+			expectedErr: ErrNoValidCapabilityFound,
 		},
 		{
-			description:    "No Match with Default Checker Error",
-			cm:             cm,
-			token:          defaultToken,
-			includeURL:     true,
-			endpoint:       "bcedef",
-			expectedReason: NoCapabilitiesMatch,
-			expectedErr:    ErrNoValidCapabilityFound,
+			description: "No Match with Default Checker Error",
+			cm:          cm,
+			token:       defaultToken,
+			includeURL:  true,
+			endpoint:    "bcedef",
+			expectedErr: ErrNoValidCapabilityFound,
 		},
 		{
-			description:    "No Match Nil Default Checker Error",
-			cm:             nilCM,
-			token:          defaultToken,
-			includeURL:     true,
-			endpoint:       "bcedef",
-			expectedReason: NoCapabilitiesMatch,
-			expectedErr:    ErrNoValidCapabilityFound,
+			description: "No Match Nil Default Checker Error",
+			cm:          nilCM,
+			token:       defaultToken,
+			includeURL:  true,
+			endpoint:    "bcedef",
+			expectedErr: ErrNoValidCapabilityFound,
 		},
 		{
-			description:    "No Token Error",
-			cm:             cm,
-			token:          nil,
-			includeURL:     true,
-			expectedReason: MissingValues,
-			expectedErr:    ErrNoToken,
+			description: "No Token Error",
+			cm:          cm,
+			token:       nil,
+			includeURL:  true,
+			expectedErr: ErrNoToken,
 		},
 		{
-			description:    "No Request URL Error",
-			cm:             cm,
-			token:          goodToken,
-			includeURL:     false,
-			expectedReason: MissingValues,
-			expectedErr:    ErrNoURL,
+			description: "No Request URL Error",
+			cm:          cm,
+			token:       goodToken,
+			includeURL:  false,
+			expectedErr: ErrNoURL,
 		},
 		{
-			description:    "Empty Endpoint Error",
-			cm:             cm,
-			token:          goodToken,
-			includeURL:     true,
-			endpoint:       "",
-			expectedReason: EmptyParsedURL,
-			expectedErr:    ErrEmptyEndpoint,
+			description: "Empty Endpoint Error",
+			cm:          cm,
+			token:       goodToken,
+			includeURL:  true,
+			endpoint:    "",
+			expectedErr: ErrEmptyEndpoint,
 		},
 		{
-			description:    "Get Capabilities Error",
-			cm:             cm,
-			token:          badToken,
-			includeURL:     true,
-			endpoint:       "b",
-			expectedReason: UndeterminedCapabilities,
-			expectedErr:    ErrNilAttributes,
+			description: "Get Capabilities Error",
+			cm:          cm,
+			token:       badToken,
+			includeURL:  true,
+			endpoint:    "b",
+			expectedErr: ErrNilAttributes,
 		},
 	}
 	for _, tc := range tests {
@@ -163,13 +157,18 @@ func TestCapabilitiesMapCheck(t *testing.T) {
 					Method: "GET",
 				}
 			}
-			reason, err := tc.cm.CheckAuthentication(auth, ParsedValues{Endpoint: tc.endpoint})
-			assert.Equal(tc.expectedReason, reason)
-			if err == nil || tc.expectedErr == nil {
-				assert.Equal(tc.expectedErr, err)
+			err := tc.cm.CheckAuthentication(auth, ParsedValues{Endpoint: tc.endpoint})
+			if tc.expectedErr == nil {
+				assert.NoError(err)
 				return
 			}
-			assert.Contains(err.Error(), tc.expectedErr.Error())
+			assert.True(errors.Is(err, tc.expectedErr),
+				fmt.Errorf("error [%v] doesn't contain error [%v] in its err chain",
+					err, tc.expectedErr),
+			)
+			// every error should be a reasoner.
+			var r Reasoner
+			assert.True(errors.As(err, &r), "expected error to be a Reasoner")
 		})
 	}
 }

basculechecks/capabilitiesvalidator.go

@@ -27,13 +27,39 @@ import (
 )
 
 var (
-	ErrNoVals                 = errors.New("expected at least one value")
-	ErrNoAuth                 = errors.New("couldn't get request info: authorization not found")
-	ErrNoToken                = errors.New("no token found in Auth")
-	ErrNoValidCapabilityFound = errors.New("no valid capability for endpoint")
-	ErrNilAttributes          = errors.New("nil attributes interface")
-	ErrNoMethod               = errors.New("no method found in Auth")
-	ErrNoURL                  = errors.New("invalid URL found in Auth")
+	ErrNoAuth = errors.New("couldn't get request info: authorization not found")
+	ErrNoVals = errWithReason{
+		err:    errors.New("expected at least one value"),
+		reason: EmptyCapabilitiesList,
+	}
+	ErrNoToken = errWithReason{
+		err:    errors.New("no token found in Auth"),
+		reason: MissingValues,
+	}
+	ErrNoValidCapabilityFound = errWithReason{
+		err:    errors.New("no valid capability for endpoint"),
+		reason: NoCapabilitiesMatch,
+	}
+	ErrNilAttributes = errWithReason{
+		err:    errors.New("nil attributes interface"),
+		reason: MissingValues,
+	}
+	ErrNoMethod = errWithReason{
+		err:    errors.New("no method found in Auth"),
+		reason: MissingValues,
+	}
+	ErrNoURL = errWithReason{
+		err:    errors.New("invalid URL found in Auth"),
+		reason: MissingValues,
+	}
+	ErrGettingCapabilities = errWithReason{
+		err:    errors.New("couldn't get capabilities from attributes"),
+		reason: UndeterminedCapabilities,
+	}
+	ErrCapabilityNotStringSlice = errWithReason{
+		err:    errors.New("expected a string slice"),
+		reason: UndeterminedCapabilities,
+	}
 )
 
 const (
@@ -76,7 +102,7 @@ func (c CapabilitiesValidator) Check(ctx context.Context, _ bascule.Token) error
 		return nil
 	}
 
-	_, err := c.CheckAuthentication(auth, ParsedValues{})
+	err := c.CheckAuthentication(auth, ParsedValues{})
 	if err != nil && c.ErrorOut {
 		return fmt.Errorf("endpoint auth for %v on %v failed: %v",
 			auth.Request.Method, auth.Request.URL.EscapedPath(), err)
@@ -90,28 +116,24 @@ func (c CapabilitiesValidator) Check(ctx context.Context, _ bascule.Token) error
 // iterating through each capability and calling the EndpointChecker.  If no
 // capability authorizes the client for the given endpoint and method, it is
 // unauthorized.
-func (c CapabilitiesValidator) CheckAuthentication(auth bascule.Authentication, _ ParsedValues) (string, error) {
+func (c CapabilitiesValidator) CheckAuthentication(auth bascule.Authentication, _ ParsedValues) error {
 	if auth.Token == nil {
-		return MissingValues, ErrNoToken
+		return ErrNoToken
 	}
 	if len(auth.Request.Method) == 0 {
-		return MissingValues, ErrNoMethod
+		return ErrNoMethod
 	}
-	vals, reason, err := getCapabilities(auth.Token.Attributes())
+	vals, err := getCapabilities(auth.Token.Attributes())
 	if err != nil {
-		return reason, err
+		return err
 	}
 
 	if auth.Request.URL == nil {
-		return MissingValues, ErrNoURL
+		return ErrNoURL
 	}
 	reqURL := auth.Request.URL.EscapedPath()
 	method := auth.Request.Method
-	err = c.checkCapabilities(vals, reqURL, method)
-	if err != nil {
-		return NoCapabilitiesMatch, err
-	}
-	return "", nil
+	return c.checkCapabilities(vals, reqURL, method)
 }
 
 // checkCapabilities uses a EndpointChecker to check if each capability
@@ -130,25 +152,27 @@ func (c CapabilitiesValidator) checkCapabilities(capabilities []string, reqURL s
 
 // getCapabilities runs some error checks while getting the list of
 // capabilities from the attributes.
-func getCapabilities(attributes bascule.Attributes) ([]string, string, error) {
+func getCapabilities(attributes bascule.Attributes) ([]string, error) {
 	if attributes == nil {
-		return []string{}, UndeterminedCapabilities, ErrNilAttributes
+		return []string{}, ErrNilAttributes
 	}
 
 	val, ok := attributes.Get(CapabilityKey)
 	if !ok {
-		return []string{}, UndeterminedCapabilities, fmt.Errorf("couldn't get capabilities using key %v", CapabilityKey)
+		return []string{}, fmt.Errorf("%w using key path %v",
+			ErrGettingCapabilities, CapabilityKey)
 	}
 
 	vals, err := cast.ToStringSliceE(val)
 	if err != nil {
-		return []string{}, UndeterminedCapabilities, fmt.Errorf("capabilities \"%v\" not the expected string slice: %v", val, err)
+		return []string{}, fmt.Errorf("%w for capabilities \"%v\": %v",
+			ErrCapabilityNotStringSlice, val, err)
 	}
 
 	if len(vals) == 0 {
-		return []string{}, EmptyCapabilitiesList, ErrNoVals
+		return []string{}, ErrNoVals
 	}
 
-	return vals, "", nil
+	return vals, nil
 
 }