A few suggestions for the next major version

[bellebaum]

First off: I really like this gem :)
Having worked with it in my projects and recently even contributed the tiniest amount, it offers many features and foresight which are really helpful and appreciated.
At the same time, it has no required dependencies besides the standard library, which is one of my main motivations for using this gem.

Nonetheless, there are a few suggestions (and really: Just suggestions) I have for this gem, should a major refactoring and/or deprecation of a good amount of code one day be necessary:

Structure of the gem

Currently, the gem is built around two functions: JWT.encode and JWT.decode.
This is great, as it provides a simple-to-understand interface for everyone using JWTs. There are a few problems with this though.

First, occasionally there are people who are confused about the difference between JWT and JWS. Currently, this gem assumes that every JWT is a JWS and is built around this assumption. While there seem to have been issues raised around this and even some implementations provided, this is not an easy addition.

Secondly, the two functions seem a bit overloaded. Just understanding every single option (e.g. to add a custom algorithm in the encode step) can be quite hard, and the code to handle all options is sometimes hard to locate. Also in recent times, the approach of only exposing and documenting these two functions seems to have been dropped. The documentation now suggests using JWT::JWK to manage JSON Web Keys.

This latter point also makes the code more complicated, since there are a lot of conversions between formats in some places (this is partially my fault) and generic code concerns itself with features specific to e.g. OpenSSL.

For a future version of this gem, I propose the following module structure:

• JWT (including the main encode/decode functions, as well as anything RFC 7519 specific. Cryptographic operations are not performed here)
• JWT::JWK (including stuff from RFC 7517, as well as optionally any extensions (e.g. the OCT kty)
• JWT::JWA (including what is now algos, c.t. RFC 7518)
• JWT::JWS (including the encoding/signing and decoding/verification functions, c.t. RFC 7515)
• JWT::JWE (optional, for RFC 7516)
• JWT::Configuration (for configuration stuff)

This also sets up the gem for future extensions to JOSE, like the proposed JSON Proof Tokens :)

Decoding Example

A decoding could be invoked like this:

key = OpenSSL::PKey::RSA.new(2048).public_key
JWT.decode token, key

The decode function would first turn the key (which may also be a JWT::JWK or JWT::JWK::Set) into a JWT::JWK::Set to not have to handle different formats.
Then it would figure out whether token is a JWS or JWE by looking at the number of . characters in the compact serialization (2 for JWS, 4 for JWE) and hand the JWKS and token to either JWT::JWS.decode or JWT::JWE.decode depending on the outcome.
These functions would have a look at the JOSE header and filter the JWKS according to the kid and alg members in the header and the use claim in the key,
and try to verify/decrypt the token using the remaining keys, with the algorithm being chosen according to their alg,
and the corresponding verify/decrypt function in JWT::JWA is called with each JWK. An optional block can still be given to the first call to add additional keys to the queue based on the unverified header/content (JWS) or header alone (JWE), and when a list of trusted certificates is configured in JWT::Configuration, further keys from the x5c header are taken into consideration, if the header is valid.

Note: We never specified algorithms in the call to decode. This is done implicitly by the JWKS which is constructed before even looking at the token. When we did have a look at alg in the token, we just used it to eliminate keys. No alteration of alg could increase the chances of forging a valid signature. We could of course keep the algorithms parameter optional, to allow to further filter the keys.

Once the token has been decoded and we are back in JWT.decode, we may need to repeat the procedure for nested JWTs.
Once this is done, the JWT-specific claims are checked and the token content along any headers returned.
If anything goes wrong, errors are raised.

Extensibility of JWKs and JWAs ("custom algorithms") is given by the possibility to extend JWT::JWK::Base (currently KeyBase) and JWT::JWA::Base.

Encoding example

Ideally, we would not have JWT.encode, but several functions:

• JWT.sign (aliased as JWT.encode)
• JWT.encrypt
• JWT.sign_then_encrypt
• JWT.encrypt_then_sign

They would be invoked like this:

key = JWT::JWK.new OpenSSL::PKey::RSA.new(2048), alg: 'RS256' # one could add a default algorithm to RSA keys for unexperienced users
JWT.sign content, key, optional_header

Of course, one could also just invoke something like JWT::JWS.sign, which would have a similar interface, but the above function might offer additional benefits like setting standard claims (e.g. iat) for you. One could also extend the configuration to set an issuer or default audience :)

In any case, the kid, typ and alg can be automatically added to the header unless configuration or options say otherwise.

Getting rid of indirection

Some parts of this gem seem to be superfluous, like JWT::Base64, which offers no functionality over the std-lib Base64 (Note that it can also perform base64url encoding), or the wrapper around JSON. These tend to make the code base seem larger and more complicated than it actually is.

Likewise, but this is a code style argument and opinions differ, some functions seem quite blown up.
Take the private function validate_claims! from encode.rb. It wants to do the following:

return unless @payload.is_a?(Hash)

@payload.transform_keys(&:to_sym)

# Check numeric claims
violating_claims = %i[exp iat nbf].filter { |claim| @payload[claim] && !@payload[claim].is_a?(Numeric) }
return if violating_claims.empty?

raise InvalidPayload, "#{violating_claims[0]} claim must be a Numeric value but it is a #{@payload[violating_claims[0]].class}"

The actual implementation however dedicates an entire file just to this functionality, which is also used nowhere else.
Even if JWT::ClaimsValidator is supposed to be part of the public API of this gem, spreading the functionality of five lines checking three claims for their type over five functions slows down readers of this gem who just want to get a quick glance at the code of their dependencies.

At least this was my first impression. One does get fluently at reading this gem's code after a while :)

Getting rid of backwards compatibility code

I guess this is the point of a new major version :)
One suggestion here would include testing with without OpenSSL, since OpenSSL is now part of the standard library and for some/most operating systems the openssl gem is a dependency of the ruby package.

Conclusion

This gem is great. Please do not take any suggestions I left here personally or too seriously. I am not a seasoned ruby programmer nor am I an expert on "good code". This was just some ideas I had and I wanted to at least communicate them :)
Since my opinions tend to change at least hourly, I will probably pour them here when that happens. That way, the issues section stays somewhat clean :D

Thanks for reading all of this!

[anakinj]

Good points and suggestions. Without going into much details I share some of the points and views you have.

I'm still a little unsure if this gem is the correct place for all of this or would a set of gems with isolated responsibilities be a better approach and then some "collection" gems that glue parts together. For example JWKs could be used as a separate thing just for representing keys in some context, no need to bring in all the different signing algorithms in that mix.

I think we could do some bigger changes to the structure of everything and dropping some quirks from the past while keeping somewhat good backwards compatibility in the APIs so upgrading would not be that big of a hassle.

Would like to continue the discussion and maybe even come up with some actionable plan to reach a great JSON toolkit in the future.