Introducing functional-style options for the Parser type #108
Conversation
|
I was also thinking about getting rid of the |
|
If there is a configurable parser with functional options (via variadic arguments) this opens the opportunity to add behaviour in a backwards compatible way in the current branch. (I know I sound like a broken record w.r.t backwards compatibility, but that is the one thing above all that I care about in this package) Do you think we should build a breaking but "nicer" API in a The |
I think we can sort of have both. In my opinion, the approach implemented now is completely backwards compatible and it offers a quite nice way of configuring the options
I will keep it (for now) and I also added variadic arguments to it, so it can be fed with the same |
|
|
||
| // WithValidMethods is an option to supply algorithm methods that the parser will check. Only those methods will be considered valid. | ||
| // It is heavily encouraged to use this option in order to prevent attacks such as https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/. | ||
| func WithValidMethods(methods []string) ParserOption { |
There was a problem hiding this comment.
It would be good to have a unit test.
| // If populated, only these methods will be considered valid. | ||
| // | ||
| // In future releases, this field will not be exported anymore and should be set with an option to NewParser instead. | ||
| ValidMethods []string |
There was a problem hiding this comment.
Could we live in a world where algorithms are well defined, instead of strings?
There was a problem hiding this comment.
Probably, at some future point. We should keep it in mind when de-exporting the field in the future.
I added the deprecation notice to the field. Please re-approve ;) |
This PR follows up the discussion in #107 and provides an easier way to configure the parser. This is of utmost importance, because we want people to actually use
ValidMethodsand currently, the shortcut functionParseintoken.gowas just using a newParsertype, without any possibility to change its properties.Note: While this technically does change the function signature of two exported functions
ParseandParseWithClaims, it only adds an variadic argument. This means, that for a user of this library, this change is completely transparent, as the minimum number of variadic arguments is 0, meaning that the function can be called without any additional options. So in my opinion, this would be ok-ish to be considered API stable in terms of semver.This is heavily inspired by the original release4 branch of @dgrijalva
cc @sebastien-rosset