JWT::Encode refactorings, alg and exp related bugfixes #293
Conversation
anakinj
force-pushed
the
encode-refactoring
branch
from
880a780
to
43a08a4
Compare
|
I think this fixes #282 also |
| # Base64 helpers | ||
| class Base64 | ||
| class << self | ||
| def url_encode(str) |
There was a problem hiding this comment.
Do you really want to add two more methods to the public API of this gem that handle Base64 encoding? This kind of thing should probably be kept private.
| # JWT::Encode module | ||
| module JWT | ||
| # Encoding logic for JWT | ||
| class Encode | ||
| attr_reader :payload, :key, :algorithm, :header_fields, :segments | ||
| ALG_NONE = 'none'.freeze |
There was a problem hiding this comment.
Wouldn't nil be a better representation of none?
There was a problem hiding this comment.
This is something related to creating a JWT without a signature. none is the algorithm the JWA spec defines as "Unsecured JWS"
| ALG_NONE = 'none'.freeze | ||
| ALG_KEY = 'alg'.freeze | ||
| EXP_KEY = 'exp'.freeze | ||
| EXP_KEYS = [EXP_KEY, EXP_KEY.to_sym].freeze |
There was a problem hiding this comment.
Might be better off refactoring to use a HashWithIndifferentAccess
There was a problem hiding this comment.
Did not spend that much time focusing on this payload validation, think that it belongs somewhere else than the Encode class. I think #287 is a more elegant way to handle this.
|
Sorry to be late to the comments. My main concern here is that this adds methods to the public API of this gem that then need to be maintained to adhere to semantic versioning. Base64 encoding and JSON parsing might might be necessary parts of the JWT generation process, but it's better if you don't give users a way to rely on this behaviour. |
|
Thanks for the feedback. The idea was not to add more public APIs for the GEM. It was more to have the Base64 and JSON related methods used in this gem in the same place. This would for example allow easier monkey patching the JSON handling to use some other JSON framework than the default, not saying monkey patching is the way to go:) The helper methods could as well be in a AFAIK there is no good way in Ruby to hide GEM internals, in theory all the classes and methods in a gem is free to be used anywhere, but i suppose they are not considered public because you can access them? What would you suggest doing to make this clearer? |
|
@anakinj yeah totally agree, Ruby doesn't have a good way of handling these situations. I guess we could take it as a given that only methods on the "JWT" constant are part of the API but I think that's a bit limiting and inevitably someone will ignore that and if they do they are susceptible to breaking changes in minor or even patch updates. If you wanted to make these private your options would be to use You're comment about monkey patching is interesting. However, as you also hint at, I think that if this becomes a feature requirement there are better ways to implement this like a |
Spent a little time fixing and simplifying (at least in my mind) things related to the encoding process of the JWT token. Also base64 and JSON operations were moved to its own class.
Comments are appreciated!