add IncludesAny and IncludesAll methods to ValidationError

[thetorpedodog]

Adds IncludesAny and IncludesAll convenience methods to test whether
an error contains some (IncludesAny) or all (IncludesAll) of a set of
error conditions.

Adds tests for IncludesAny and IncludesAll.

Updates doc comments to follow standard Go conventions and changes
ValidationError.valid() to use a value receiver so that all methods
have the same kind of receiver.

[oxisto]

Thanks for this PR. I am still a little bit torn whether we want to push additional functionality into the struct. My long term plan would actually to refactor this alltogether with the new error wrapping possibilities that were introduced in Go 1.13. This way we could rely on the stdlib's approach of using errors.Is and so on and not replicate this behaviour with custom code.

[lggomez]

Thanks for this PR. I am still a little bit torn whether we want to push additional functionality into the struct. My long term plan would actually to refactor this alltogether with the new error wrapping possibilities that were introduced in Go 1.13. This way we could rely on the stdlib's approach of using errors.Is and so on and not replicate this behaviour with custom code.

I kinda agree with this as discussed on #64. Do we want to keep going the both the bitflag and 1.13 error route? in any case, I can see these refactored error types working both ways but I don't know if such complexity should be warranted here

[thetorpedodog]

Fair concerns—something like this (specifically, “IncludesAny”) would make some code in my work nicer and I did this implementation as a “throw it at the wall and see what sticks” first version.

It looks like this could be implemented in terms of errors.Is in a few steps:

1. Make each of the ValidationError___ constants some type that implements error. Something like

   type InvalidReason uint32
   func (r InvalidReason) Error() string { return "TODO" } // maybe go generate a stringer?
   
   const (
     ValidationErrorMalformed InvalidReason = 1 << iota
     // ...
   )

   (Would this constitute an API break? Maybe create new, identically-named and -valued InvalidReason___ constants?)
2. Make ValidationError.Unwrap return the internal InvalidReason as the cause (i.e., the ValidationError only provides the details string).

This seems like it could coexist with the bitmasked error forms (e.g. InvalidReason.Is acts like the original IncludesAll). You might also choose to provide something like an AnyOf type:

type AnyOf InvalidReason

// AnyReason is a convenience constructor so that you don't have to think about the bitmasking operations.
func AnyReason(...of InvalidReason) AnyOf {
  out := AnyOf(0)
  for _, r := range of {
    out |= r
  }
  return out
}

func (ar AnyReason) Error() string { return "TODO" }

func (ar AnyReason) Is(err error) bool { /* do the intersection-check */ }

or you could choose to abandon them and go for more conventional sequentially-enumerated errors (which would definitely be an API break).

These are just some ideas after a little pondering.

[buddy-sandidge]

@thetorpedodog, regarding the comment

Make ValidationError.Unwrap return the internal InvalidReason as the cause (i.e., the ValidationError only provides the details string).

I would assume the error returned from Unwrap to be the error inside the validation error. The reason is to allow checking the error returned as part of a custom claim validation. For example, here's a simplified use case I would expect to work.

func (s *Server) ClaimFromToken(token string) (*CustomClaim, error) {
	claim := CustomClaim{}
	if _, err := jwt.ParseWithClaims(token, &claim, s.withKey); err != nil {
		var validationError *CustomValidationError
		if errors.As(err, &validationError) {
			fmt.Println("got invalid claim %d",  validationError.UserID)
			// do custom error handling here
		}
		return err
	}
	return &claim, nil
}

func (s *Server) withKey(token *jwt.Token) (interface{}, error) { return s.Key, nil }

type CustomClaim struct { UserID int }

func (c CustomClaim) Valid() error {
	if c.UserID <= 0 {
		return &CustomValidationError{c.UserID}
	}
	return nil
}

type CustomValidationError struct { UserID int }

func (e *CustomValidationError) Error() string {
	return fmt.Sprintf("invalid user id %d", e.UserID)
}

To get the above to work, I think the only change needed would be to add an Unwrap method that would look something like what I have below. I don't know this code base in-depth, there may be reasons something like what I have below wouldn't work.

func (e *InvalidClaimError) Unwrap() error {
	return e.Inner
}

@lggomez, regarding the go 1.13 error API support and error type bitfield support, would the pseudo-code I have below address both issues? We could inspect the value of the error passed in against the current error code. I don't know how idiomatic it is, but it should work. I can make a PR if this approach sounds good. I have some custom code I wrote to work around the lack of the go 1.13 error support and I would love to get rid of it. I'm happy to contribute if it would help.

func (e *InvalidClaimError) Is(err error) bool {
	claimError, ok := err.(*InvalidClaimError)
	if !ok {
		return false
	}
	// perhaps a different bitwise check?
	return claimError.Errors & e.Errors == 0
}

func SomeExampleFunction() {
	_, err := jwt.ParseWithClaims(token, &claim, s.withKey)
	if errors.Is(err, &jwt.InvalidClaimError{}) {
		// handle case when the type of error is an jwt.InvalidClaimError
	}
	if errors.Is(err, &jwt.InvalidClaimError{Errors: jwt.ValidationErrorExpired}) {
		// handle case when the error is a jwt.ValidationErrorExpired
	}
	if errors.Is(err, &jwt.InvalidClaimError{Errors: jwt.ValidationErrorExpired | jwt.ValidationErrorIssuedAt}) {
		// handle case with multiple errors
	}

}

[oxisto]

I think this functionality should now be possible with #136

You can find an example in the examples:

jwt/example_test.go

Lines 101 to 114 in b44ad0a

	token, err := jwt.Parse(tokenString, func(token *jwt.Token) (interface{}, error) {
	return []byte("AllYourBase"), nil
	})
	if token.Valid {
	fmt.Println("You look nice today")
	} else if errors.Is(err, jwt.ErrTokenMalformed) {
	fmt.Println("That's not even a token")
	} else if errors.Is(err, jwt.ErrTokenExpired) || errors.Is(err, jwt.ErrTokenNotValidYet) {
	// Token is either expired or not active yet
	fmt.Println("Timing is everything")
	} else {
	fmt.Println("Couldn't handle this token:", err)
	}

[oxisto]

Closing this as stale and possibly superseded by #136.