Improve public error type and properly validate input length

[leotaku]

This addresses some of the issues raised by in #4.

@ThinkChaos feel free to provide feedback on what you think about these changes. I know on public error variant still contains a Box<dyn std::error::Error>, but in my opinion this is the best way to deal with the opaque error type provided by jwt_simple, while also keeping the header/authentication provider system very generic.

[leotaku]

Hey @ThinkChaos, if you want you can give me some feedback on these changes and if they would help your usecase. Otherwise, I will probably merge this PR as-is soon.

[ThinkChaos]

Sorry haven't had much time to spend on the computer. Will get back to you by the weekend!

[ThinkChaos]

I agree the type should be changed in jwt_simple too, and I might propose that there too depending on the result here.
jwt_simple::Error is actually an alias for anyhow::Error and that's explicitly not recommended for libs (see anyhow's README).

Coincidentally, my use case is getting this crate to work well with anyhow.
Basically the issue is that anyhow requires errors to be Send + Sync + 'static (docs), which Box<dyn std::error::Error> isn't.

A quick and easy fix to make your patch work for me is to add those bounds to Error::Extension and AddHeaders::Error.
I'm not a fan of this solution, since exposing a boxed error kind of sucks for users of this crate.

The solution I prefer is exposing the jwt_simple::Error, which still sucks as it's also not a concrete error type, but at least sucks a bit less since there's less boxing and wrapping. And using that type in this crate will make it easier to update this crate once jwt_simple::Error is fixed.
Also that has the nice side effect of hiding more code behind the vapid feature, which is a bit more correct, and users that disable it won't have to deal with Error::Extension. See #6 for the implementation.

P.S. Thanks for looking into this quickly and your patience!

[leotaku]

Thanks for the feedback!

I agree that if this PR were to be merged, the Send + Sync + 'static trait bounds should definitely be added to the boxed Error. I will push a commit to this branch that adds them.

However, I do not fully understand why you are against exposing a boxed/non-concrete error in the API. In my opinion, exposing boxed errors is only bad when a user would realistically want to implement custom error recovery based on the value of the returned error. I do not think any user of this library would want to implement custom error recovery for vapid authentication, even a future version of jwt_simple made this possible. I expect most users to simply want to get a good error message, which the new error handling code in this PR should provide.

In other some other scenarios, I might also not want to box errors because this could create additional overhead, but because jwt_simple::Error is already a boxed error, I would expect the "conversion" to Box<std::error::Error> to essentially be a no-op.

Additionally, I'm not sure that removing the Extension error variant based on features is something that I would like to do. I could imagine users being confused by such a change when enabling or disabling crate features. Exposing jwt_simple::Error directly would force me to remove the error variant depending on the vapid feature.

In essence, from my perspective exposing jwt_simple::Error directly instead of Box<dyn std::error::Error + Send + Sync + 'static> would require some non-obvious behavior and more code for supporting optional features, while providing no real benefits. However, I acknowledge that I am not an expert when it comes to the finer details of Rust and there might well be a good reason to avoid Box<dyn std::error::Error + Send + Sync + 'static>. If you know such a reason, please tell me!

[ThinkChaos]

I agree that I don't "want to implement custom error recovery based on the value of the returned error", but IMO it's not up to crates to decide that for the user. At least that's my understanding of the consensus in Rust's ecosystem.

I would expect the "conversion" to Box to essentially be a no-op

Yeah you're right, I had missed that there's a From<Error> for Box in anyhow (docs), which does seem smarter than "re-box".

Anyways for my use case this PR works since the latest commit, so I'm ok with this outcome.

[leotaku]

I agree that I don't "want to implement custom error recovery based on the value of the returned error", but IMO it's not up to crates to decide that for the user. At least that's my understanding of the consensus in Rust's ecosystem.

That seems fair. I might reconsider my approach if jwt_simple starts exposing a concrete error type.

Thanks again for the feedback, I have added you as a co-author of this PR. You can expect a release incorporating these changes soon.