Skip to content

Allow decode options to specify required claims#430

Merged
excpt merged 3 commits intojwt:masterfrom
andyjdavis:224_required_claims
Jul 16, 2021
Merged

Allow decode options to specify required claims#430
excpt merged 3 commits intojwt:masterfrom
andyjdavis:224_required_claims

Conversation

@andyjdavis
Copy link
Copy Markdown
Contributor

Hi, this is a potential solution for #244

When decoding, the caller can optionally supply an array of claims that must be present for the jwt to be valid. For example requiring the 'exp' claim to both be present and valid.

I wasnt sure whether to put the check within Verify.verify_claims and ultimately added Verify.verify_required_claims alongside. Let me know if you want any changes to how this is structured.

@sourcelevel-bot
Copy link
Copy Markdown

Hello, @andyjdavis! This is your first Pull Request that will be reviewed by SourceLevel, 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.

Comment thread lib/jwt/error.rb
class InvalidSubError < DecodeError; end
class InvalidJtiError < DecodeError; end
class InvalidPayload < DecodeError; end
class MissingRequiredClaim < DecodeError; end
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JWT::MissingRequiredClaim has no descriptive comment

Read more about it here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these error classes have a descriptive comment so I'm not sure I should add one.

@andyjdavis andyjdavis force-pushed the 224_required_claims branch from 7d422e9 to 498d632 Compare July 2, 2021 12:11
@sourcelevel-bot
Copy link
Copy Markdown

SourceLevel has finished reviewing this Pull Request and has found:

  • 1 possible new issue (including those that may have been commented here).

See more details about this review.

@anakinj
Copy link
Copy Markdown
Member

anakinj commented Jul 9, 2021

Looks good.

Would it make sense to have one "integration" test for this, testing the public JWT.decode interface for the required_claims parameter. For example some of the examples in the readme is also tested in readme_examples_spec.rb.

@andyjdavis
Copy link
Copy Markdown
Contributor Author

Just letting you know that I have been on a short holiday but I'm back now. I will get back to this in the next 2-3 days :)

@andyjdavis andyjdavis force-pushed the 224_required_claims branch from 498d632 to 4cb4751 Compare July 15, 2021 13:45
@andyjdavis
Copy link
Copy Markdown
Contributor Author

@anakinj Ive pushed a commit with an integration test. Let me know if anything needs more work.

@anakinj
Copy link
Copy Markdown
Member

anakinj commented Jul 15, 2021

Looks great. A few minor adjustments to get RuboCop happy and we are ready to merge. https://github.com/jwt/ruby-jwt/pull/430/checks?check_run_id=3077229521

@andyjdavis
Copy link
Copy Markdown
Contributor Author

@anakinj I have fixed those rubocop warnings.

@excpt excpt merged commit 3b4a1ab into jwt:master Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants