Repository: lestrrat-go/jwx
Pull request #718 (2022-04-27 21:23:01)

Implement custom validation errors for invalid `aud` and `iss` claims
As discussed in #706, I'm adding two new custom validation errors for when the `aud` and `iss` claims are invalid. Do you think that the `Validate()` functions should always return the specific error, even in case the claim does not exists?

lestrrat: >  Do you think that the Validate() functions should always return the specific error, even in case the claim does not exists?

There's a nuance to this statement that (I don't know if you intended to be that way) that I don't agree with.
Specifically, "Validate() ... should _always_ return the specific error" doesn't sound right. But for the purposes of this PR, yes, I think it should return the specific error.

It's not really that `Validate()` needs to _always_ return a particular value, but it has more to do with the user's expectation. If I were a user and asked my `iss` claim to be validated, and wrote `jwt.Validate(tok, jwt.WithIssuer("foobar")`, I would expect `iss == "foobar"` to be checked all the time. Therefore, empty values should error out.

There may be cases where empty/non-existent values can be ignored, but not in this case.




ste93cry: >But for the purposes of this PR, yes, I think it should return the specific error

What I meant is that I had the doubt if a generic `ValidationError` had to be returned for empty/non-existent values or if the specific `ErrXXX` had to be (for these particular cases of course), but you answered to me in https://github.com/lestrrat-go/jwx/pull/718#discussion_r860274458 and I agree with your thoughts

Patch:
diff --git a/jwt/options.go b/jwt/options.go
index ce132f67..a83f4768 100644
--- a/jwt/options.go
+++ b/jwt/options.go
@@ -167,7 +167,7 @@ func WithKeySet(set jwk.Set, options ...interface{}) ParseOption {
 // WithIssuer specifies that expected issuer value. If not specified,
 // the value of issuer is not verified at all.
 func WithIssuer(s string) ValidateOption {
-	return WithValidator(ClaimValueIs(IssuerKey, s))
+	return WithValidator(issuerClaimValueIs(s))
 }
 
 // WithSubject specifies that expected subject value. If not specified,
@@ -187,7 +187,7 @@ func WithJwtID(s string) ValidateOption {
 // matches this value.  If not specified, the value of issuer is not
 // verified at all.
 func WithAudience(s string) ValidateOption {
-	return WithValidator(ClaimContainsString(AudienceKey, s))
+	return WithValidator(audienceClaimContainsString(s))
 }
 
 // WithClaimValue specifies the expected value for a given claim
diff --git a/jwt/validate.go b/jwt/validate.go
index f4a4aa67..f030f65b 100644
--- a/jwt/validate.go
+++ b/jwt/validate.go
@@ -175,6 +175,48 @@ func (err *missingRequiredClaimError) Is(target error) bool {
 func (err *missingRequiredClaimError) isValidationError() {}
 func (*missingRequiredClaimError) Unwrap() error          { return nil }
 
+type invalidAudienceError struct {
+	error
+}
+
+func (err *invalidAudienceError) Is(target error) bool {
+	_, ok := target.(*invalidAudienceError)
+	return ok
+}
+
+func (err *invalidAudienceError) isValidationError() {}
+func (err *invalidAudienceError) Unwrap() error {
+	return err.error
+}
+
+func (err *invalidAudienceError) Error() string {
+	if err.error == nil {
+		return `"aud" not satisfied`
+	}
+	return err.error.Error()
+}
+
+type invalidIssuerError struct {
+	error
+}
+
+func (err *invalidIssuerError) Is(target error) bool {
+	_, ok := target.(*invalidIssuerError)
+	return ok
+}
+
+func (err *invalidIssuerError) isValidationError() {}
+func (err *invalidIssuerError) Unwrap() error {
+	return err.error
+}
+
+func (err *invalidIssuerError) Error() string {
+	if err.error == nil {
+		return `"iss" not satisfied`
+	}
+	return err.error.Error()
+}
+
 var errTokenExpired = NewValidationError(fmt.Errorf(`"exp" not satisfied`))
 var errInvalidIssuedAt = NewValidationError(fmt.Errorf(`"iat" not satisfied`))
 var errTokenNotYetValid = NewValidationError(fmt.Errorf(`"nbf" not satisfied`))
@@ -195,6 +237,14 @@ func ErrTokenNotYetValid() ValidationError {
 	return errTokenNotYetValid
 }
 
+func ErrInvalidAudience() ValidationError {
+	return &invalidAudienceError{}
+}
+
+func ErrInvalidIssuer() ValidationError {
+	return &invalidIssuerError{}
+}
+
 // ErrMissingRequiredClaim creates a new error for missing required claims.
 func ErrMissingRequiredClaim(name string) ValidationError {
 	return &missingRequiredClaimError{claim: name}
@@ -335,6 +385,10 @@ func IsValidationError(err error) bool {
 		switch err.(type) {
 		case *validationError:
 			return true
+		case *invalidAudienceError:
+			return true
+		case *invalidIssuerError:
+			return true
 		default:
 			return false
 		}
@@ -352,17 +406,23 @@ func (ccs claimContainsString) Validate(_ context.Context, t Token) ValidationEr
 		return NewValidationError(fmt.Errorf(`claim %q must be a []string (got %T)`, ccs.name, v))
 	}
 
-	var found bool
 	for _, v := range list {
 		if v == ccs.value {
-			found = true
-			break
+			return nil
 		}
 	}
-	if !found {
-		return NewValidationError(fmt.Errorf(`%q not satisfied`, ccs.name))
-	}
-	return nil
+	return NewValidationError(fmt.Errorf(`%q not satisfied`, ccs.name))
+}
+
+// audienceClaimContainsString can be used to check if the audience claim, which is
+// expected to be a list of strings, contains `value`.
+func audienceClaimContainsString(value string) Validator {
+	return ValidatorFunc(func(ctx context.Context, token Token) ValidationError {
+		if err := ClaimContainsString(AudienceKey, value).Validate(ctx, token); err != nil {
+			return &invalidAudienceError{error: err.Unwrap()}
+		}
+		return nil
+	})
 }
 
 type claimValueIs struct {
@@ -389,6 +449,17 @@ func (cv *claimValueIs) Validate(_ context.Context, t Token) ValidationError {
 	return nil
 }
 
+// issuerClaimValueIs creates a Validator that checks if the issuer claim
+// matches `value`.
+func issuerClaimValueIs(value string) Validator {
+	return ValidatorFunc(func(ctx context.Context, token Token) ValidationError {
+		if err := ClaimValueIs(IssuerKey, value).Validate(ctx, token); err != nil {
+			return &invalidIssuerError{error: err.Unwrap()}
+		}
+		return nil
+	})
+}
+
 // IsRequired creates a Validator that checks if the required claim `name`
 // exists in the token
 func IsRequired(name string) Validator {


Test patch:
diff --git a/jwt/validate_test.go b/jwt/validate_test.go
index 6c5873ee..d4241407 100644
--- a/jwt/validate_test.go
+++ b/jwt/validate_test.go
@@ -21,16 +21,6 @@ func TestGHIssue10(t *testing.T) {
 		OptionFunc func(string) jwt.ValidateOption
 		BuildFunc  func(v string) (jwt.Token, error)
 	}{
-		{
-			ClaimName:  jwt.IssuerKey,
-			ClaimValue: `github.com/lestrrat-go/jwx/v2`,
-			OptionFunc: jwt.WithIssuer,
-			BuildFunc: func(v string) (jwt.Token, error) {
-				return jwt.NewBuilder().
-					Issuer(v).
-					Build()
-			},
-		},
 		{
 			ClaimName:  jwt.JwtIDKey,
 			ClaimValue: `my-sepcial-key`,
@@ -77,6 +67,39 @@ func TestGHIssue10(t *testing.T) {
 			}
 		})
 	}
+	t.Run(jwt.IssuerKey, func(t *testing.T) {
+		t.Parallel()
+		t1, err := jwt.NewBuilder().
+			Issuer("github.com/lestrrat-go/jwx/v2").
+			Build()
+		if !assert.NoError(t, err, `jwt.NewBuilder should succeed`) {
+			return
+		}
+
+		// This should succeed, because WithIssuer is not provided in the
+		// optional parameters
+		if !assert.NoError(t, jwt.Validate(t1), "jwt.Validate should succeed") {
+			return
+		}
+
+		// This should succeed, because WithIssuer is provided with same value
+		if !assert.NoError(t, jwt.Validate(t1, jwt.WithIssuer(t1.Issuer())), "jwt.Validate should succeed") {
+			return
+		}
+
+		err = jwt.Validate(t1, jwt.WithIssuer("poop"))
+		if !assert.Error(t, err, "jwt.Validate should fail") {
+			return
+		}
+
+		if !assert.ErrorIs(t, err, jwt.ErrInvalidIssuer(), "error should be jwt.ErrInvalidIssuer") {
+			return
+		}
+
+		if !assert.True(t, jwt.IsValidationError(err), "error should be a validation error") {
+			return
+		}
+	})
 	t.Run(jwt.IssuedAtKey, func(t *testing.T) {
 		t.Parallel()
 		t1 := jwt.New()
@@ -134,7 +157,7 @@ func TestGHIssue10(t *testing.T) {
 			if !assert.Error(t, err, "token.Validate should fail") {
 				return
 			}
-			if !assert.False(t, errors.Is(err, jwt.ErrTokenNotYetValid()), `error should be not ErrNotYetValid`) {
+			if !assert.ErrorIs(t, err, jwt.ErrInvalidAudience(), `error should be ErrInvalidAudience`) {
 				return
 			}
 			if !assert.True(t, jwt.IsValidationError(err), `error should be a validation error`) {