Allow a list of algorithms for decode #241
Conversation
Hard-coding only one algorithms makes rolling out changes harder. Both ends need to be deployed simultaneously or retry logic needs to be implemented in app. Allowing the algorithms to be a list containing potentially multiple entries enables flexibility when changing algorithms.
|
Hello, @lautis! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
|
@lautis Thanks for the PR. I left you a comment to improve the code. |
|
@excpt should that be visible somewhere? |
spec/jwt_spec.rb
Outdated
| token = JWT.encode payload, data[:secret], 'HS512' | ||
|
|
||
| expect do | ||
| JWT.decode token, data[:secret], true, algorithm: 'HS384' | ||
| JWT.decode token, data[:secret], true, algorithms: 'HS384' |
There was a problem hiding this comment.
I'd duplicate the tests in that case, since :algorithm is still supported. I prefer to be a little more paranoid.
There was a problem hiding this comment.
That's a typo, should be left as algorithm there.
There was a problem hiding this comment.
Simply push a fix for that and I'll merge it.
|
@lautis Now it should be visible. I need to press the "submit review"-button in order to make my review visible. |
Do not combine both options. As algorithms (plural) option contains default value, using algorithm (singular) should not include the default value.
|
I added one more change: removed the combination of option because it could lead into the default algorithms array being used in addition to the value given with algorithm option. |
|
Thanks again for your contribution. 🎉 |
|
Thank you for maintaining JWT! 🙏 |
Hard-coding only one algorithms makes rolling out changes harder. Both ends need to be deployed simultaneously or retry logic needs to be implemented in app.
Allowing the algorithms to be a list containing potentially multiple entries enables flexibility when changing algorithms. The default value is left unchanged and giving
:algorithmoption instead of:algorithmsstill works.