Refactors errors to use go 1.13 style

[chanced]

This isn't done as the tests haven't been verified but this moves errors to go 1.13. I can finish it but I don't want to burn a lot of time if you aren't interested in the changes.

There may be too many errors, especially around signing methods, but maybe not. It definitely helps to be able to narrow down what the problem is.

This pull request also:

• adds accessors to MapClaims
• changes the way signing methods are accessed. I changed the RWMutex guarding the global map of signingMethods to a Mutex. It is only locked when RegisterSigningMethod is called. The signingMethods map is then copied, modified, and then assigned over the existing map. Doing so removes the need to have a mutex checks on reads.

[oxisto]

Thanks for your contribution. There are several things to consider when moving forward in a direction like that

• We would really like to avoid any dependencies on third-party projects, meaning we would only consider the standard library's way of handling errors
• The changes to claim verification are interesting, but I am not sure if we should de-couple them from the error handling. I would propose first designing an appropriate API first. See Bring over ValidationHelper functionality from v4 branch #16
• All of this is API breaking, so we would need to consider doing this in a possible v5 branch. This is not a deal-breaker, we just need to be aware of it

[chanced]

The changes to claim verification are interesting, but I am not sure if we should de-couple them from the error handling. I would propose first designing an appropriate API first.

Which changes? I don't think I changed the logic behind the validation. I merely changed the way errors were composed. I did add accessors to MapClaims but that was just for my connivence.

We would really like to avoid any dependencies on third-party projects, meaning we would only consider the standard library's way of handling errors

Hashicorp's multierror is using the standard Is/ Unwrap under the hood. The used code from multierror is about 100LOC, between multierror.MultiError and multierror.Append. I guess you could just copy it? It is an incredibly clean way to handle multiple errors.

All of this is API breaking, so we would need to consider doing this in a possible v5 branch. This is not a deal-breaker, we just need to be aware of it

Yep. No worries there. Going to fork it for my use. Feel free to hit me up here if you have any questions or need me to clarify anything.

[mfridman]

Thank you for submitting a PR, we should discuss large (breaking) changes like this in an issue.

Indeed errors is something this package could improve upon, esp. since there is a well-defined way to handle errors nowadays. This will likely be a /v5 release with proper documentation on error handling. cc @oxisto do you recall if we have a issue / discussion tracking improving error handling? I suggest we add proposals (like this one) to a tracking issue.

I think this package should avoided external dependencies, if possible. But multierror is the right tool for the job.

[chanced]

So since validation errors are fairly isolated, you may not need it.

You could do something like:

type ValidationError struct {
    // maybe include a RWMutex? I don't know if it is warranted.
   errors []error
}

func (err *ValidationError) Errors() []error {
    res := make([]error, len(err.errors))
    copy(res, err.errors)
    return res
}

func (err *ValidationError) Error() string {
    res := fmt.Sprintf("%d validation errors occurred", len(err))
    for _, e := range err.errors {
        res += "\n\t* " + e.Error()
    }
    return res
}

func (err *ValidationError) Is(target error) bool {
    if ve, ok := (target).(*ValidationError); ok {
        return true
    }
    for _, e := range err.errors {
        if errors.Is(e, target) {
            return true
        }
    }
    return false
}

func(err *ValidationError) As(target interface{}) bool {
    // ref: https://cs.opensource.google/go/go/+/master:src/errors/wrap.go;l=95-97
    for _, e := range err.errors {
        if errors.As(e, &target) {
            return true
         }
    }
    return false
}
func (err *ValidationError) Add(target error) {
    if(target != nil) {
        err.errors = append(err.errors, target)
    }
}

func (err *ValidationError) ErrorOrNil() error {
    if(len(err.errors) == 0) {
        return nil
    }
    return nil
}

Then use it like:

    t, err := p.Parse("token", keyFunc)
    if err != nil {
        var vErr *jwt.ValidationError
        if errors.As(err, &vErr){ // this could also be if errors.Is(err, ErrValidation) or something
                // err is a validation error.

                // to check for specific validation errors, you could then either use errors.Is or errors.As:
                // (both of which could be done on err rather than vErr)

                if errors.Is(vErr, ErrTokenExpired) {
                        fmt.Println("token is expired")
                }
                // or, alternatively
                var ubErr *UsedBeforeIssuedError
                if errors.As(vErr, &ubErr) {
                    fmt.Println("token issued at", ubErr.IssuedAt)
                    fmt.Println("token attempted at, ubErr.AttemptedAt)
                    fmt.Println("time between issue and attempt", ubErr.Delta())
               }
        }
    }

Outside of validation errors, I suspect you're going to want to fail fast and return the first error encountered.

I'd also encourage a change in the interface for the Verify methods. I'd have those return an error instead of bool.

That way you could do:

func (c Claims) IsValid() bool {
    return c.Validate() == nil
}
func (c Claims) Validate() error {
    err := &ValidationError{ errors: []error{} }
    err.Add(c.VerifyExpiresAt(now, expiresRequired))
    err.Add(c.VerifyIssuedAt(now, issuedRequired))
    err.Add(c.VerifyNotBefore(now, notBeforeRequired))
    return err.ErrorOrNil()
}

[chanced]

Edited my reply above.

[lggomez]

I have to take a better look at this but contract-wise this goes a similar way to #35 and #64, error handling improvements are very much welcome in the context of the 1.13 model but the fact that any changes or style fixes also constitute a breaking change on the package API contract contract.

Seconding @mfridman, this warrants a major version bump before merging any of these, as consumers of the package rely heavily on the errors returned by the package (at least by my previous experience using dgrijalva/jwt-go, error handling was integral for the test suites in our secuirity code).

On a sidenote, the v3 branch is in maintenance mode as main is already the v4 upstream (please correct me if I'm wrong), so I think we may be ready to create a v5 branch from main and start merging and iterating these improvements

[chanced]

If you guys are okay with the approach above, I'll implement them in the pull request. I understand the desire not to have external dependencies and since validation errors are probably the only errors that will need to be multiple, it isn't that warranted.

I'm not sure if I went overboard on the amount of errors I created though.

[oxisto]

Is there still any interest in progressing with? With #125 we can at least support errors.Is.

[oxisto]

Closing this for now.