-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Claims Validation #287
Claims Validation #287
Conversation
Hello, @jamesstonehill! 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. |
lib/jwt/claims.rb
Outdated
@@ -0,0 +1,11 @@ | |||
module JWT | |||
CLAIMS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use %i
or %I
for an array of symbols.
lib/jwt/claims.rb
Outdated
@@ -0,0 +1,11 @@ | |||
module JWT | |||
CLAIMS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Freeze mutable objects assigned to constants.
5937047
to
2aed55d
Compare
2aed55d
to
ef273be
Compare
ef273be
to
80b326c
Compare
@excpt seems like the specs are failing for 2.2 because of a dependency mismatch with openssl |
80b326c
to
d6d5722
Compare
Ebert has finished reviewing this Pull Request and has found:
But beware that this branch is 13 commits behind the You can see more details about this review at https://ebertapp.io/github/jwt/ruby-jwt/pulls/287. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be a nice addition, to separate the validation from the encoding logic.
Added one minor issue with the validation, might be worth checking out.
end | ||
|
||
def validate | ||
validate_exp if @payload[:exp] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to check that the key actually is present in the payload, this row is now checking if the value returned from the hash is thruthy.
I think this will not validate cases where exp is false or nil eg:
{ exp: false }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
Switching branches to this one for a cleaner commit history. #295 |
In an effort to make the claim validation a little more extendable and to fix this issue, I've added a
ClaimsValidator
class that is responsible for validating validatable claims.I thought that it is a little inconsistent that we only validate the
exp
claim is an integer even though the JWT RFC mandates that all time (iat, nbf, and exp) claims "MUST be a number containing a NumericDate value".