Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistent inner errors for validation errors #135

Closed
oxisto opened this issue Nov 30, 2021 · 2 comments · Fixed by #136
Closed

Consistent inner errors for validation errors #135

oxisto opened this issue Nov 30, 2021 · 2 comments · Fixed by #136

Comments

@oxisto
Copy link
Collaborator

oxisto commented Nov 30, 2021

With PR #125 we can finally unwrap the Inner error of an ValidationError, for example to check for its underlying error, such as expiration or mis-formed token, e.g. using error.Is

However, we are not consistent in setting our inner errors. For example, for an expired map claim we set it to:

vErr.Inner = errors.New("Token is expired")

However, for our RegisteredClaims and StandardClaims we are setting it to:

jwt/claims.go

Line 59 in c435f38

vErr.Inner = fmt.Errorf("token is expired by %v", delta)

In other cases we are actually using constant error strings, such as:

ErrInvalidKey = errors.New("key is invalid")

I would propose to harmonize this, possibly introducing an ErrExpiredClaim as a constant error string in this example to allow something like

token, err := jwt.Parse(...)
if err != nil && errors.Is(err, ErrExpiredClaim) {
  fmt.Printf("Your token is expired")
}

We could also potentially override Is(err) on validation error to check for the error using our flags rather than the error string.

I know that there is already a PR #100 open, but it introduces a lot of other changes and seems to be stale.

@mfridman
Copy link
Member

Great job, thanks for putting this together. A few questions,

  1. Can we introduce Go 1.13-style errors in a backwards compatible way?
  2. Do we need a third-party library such as go-multierror or uber-go/multierr?

My biggest worry, is breaking existing behaviour.

@oxisto
Copy link
Collaborator Author

oxisto commented Nov 30, 2021

Great job, thanks for putting this together. A few questions,

  1. Can we introduce Go 1.13-style errors in a backwards compatible way?
  2. Do we need a third-party library such as go-multierror or uber-go/multierr?

My biggest worry, is breaking existing behaviour.

I think we can do it backwards compatible in the following way:

  1. Introduce a new ErrTokenExpired which is identically to the existing errors.New("Token is expired") of map claims and replace it in the map claims.
  2. Override Is of ValidationError to check, for example if an error is ErrTokenExpired by comparing it against the flags
  3. (Potentially breaking, see below). Replace the error strings in RegisteredClaims and StandardClaims with ErrTokenExpired.

One could even argue that we could change the error strings of RegisteredClaims and StandardClaims since it does not change the API, however if one used the existing strings to compare the error (which is highly unlikely), this would break existing behaviour for them, so 3.) is a judgment call and not necessarily needed in the first place.

Update: To add on that, we already did something similar to 3.) already in #97

The approach works without any new dependencies.

oxisto added a commit that referenced this issue Nov 30, 2021
Fixes #135 by implementing `Is(err) bool` for the `ValidationError`. It both checks
for the actual inner error message as well as our error flags for maximum backwards compatibility
oxisto added a commit that referenced this issue Nov 30, 2021
Fixes #135 by implementing `Is(err) bool` for the `ValidationError`. It both checks
for the actual inner error message as well as our error flags for maximum backwards compatibility
oxisto added a commit that referenced this issue Nov 30, 2021
Fixes #135 by implementing `Is(err) bool` for the `ValidationError`. It both checks
for the actual inner error message as well as our error flags for maximum backwards compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants