-
Notifications
You must be signed in to change notification settings - Fork 47
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
Adding Pact::Jwt object, to handle json web tokens in the pact definintion #32
Conversation
…tions. Note, this adds a dependency on gem jwt
Hey @cf-3box, thanks for the PR. I would definitely like it to be in a separate gem, as we don't want everyone to have to bring in the JWT gem just to use pact. I'll create a new repo for it in pact-foundation and give you access to it. The bigger issue is that you're using ruby specific syntax to serialise the token, and that is not used any more. Pact is a cross language technology, so we don't use any ruby specific code in the pact JSON. What we can do is work out how to specify in the matching rules that it is a JWT token, to handle it accordingly. |
Ok, I've created a repo, and invited you to it: https://github.com/pact-foundation/pact-jwt/settings/collaboration |
Hm... now I'm looking at the code, I've realised it's going to be tricky to extract. I'll give it some thought. |
@@ -39,6 +40,7 @@ def calculate_diff expected, actual, opts = {} | |||
when Regexp then regexp_diff(expected, actual, options) | |||
when Pact::SomethingLike then calculate_diff(expected.contents, actual, options.merge(:type => true)) | |||
when Pact::ArrayLike then array_like_diff(expected, actual, options) | |||
when Pact::Jwt then jwt_diff(expected,actual,options) |
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.
Dude, spacing! Gotta respect the formatting contentions of the codebase, otherwise your PRs will never get merged!
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.
Ok, I know it's just a conversation stater, but it's nice to make a good first impression ;)
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.
Oh oops, completely overlooked that.
# Specifies that the actual object should be considered a match if | ||
# it includes the same keys, and the values of the keys are of the same class. | ||
|
||
class Jwt |
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.
What do you think about calling this JWT? It looks funny to my eye, but I know it's completely subjective.
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 don't like Jwt very much, to be honest. I started with JWT, and changed it, to avoid confusion with JWT the gem itself.
Ok, I think I need to make some extension points in the matching code. |
I'll need some guidance there, I thought I simply copied what Pact.SomethingLike does (or is that Ruby specific as well?) - I have never looked into the v2 matching stuff. Hooks and separate gem, I like the approach. Anybody who wants to add their own special matcher can do that. |
I'm looking at all the places we need to update. And the bits you've already done... I'm going to go back on my original inclination to put it in a separate gem, because I don't currently have the time to make all these pieces of code extensible. However, I don't want to make the jwt gem a requirement for all pact users, so I think we'll do all the Pact::JWT stuff as part of the main gems, but not make jwt a runtime requirement of the pact-jwt gem. |
Hello @cf-3box , Apologies for the very late reply, would this still be useful to you at all? And would be want to pull anything out into https://github.com/pact-foundation/pact-jwt I'm not sure with the new rust core, if it changes the state of play, but thank you so much for your contributions |
We have a similar ask over in the pact reference project, which pact-ruby will ultimately end up leveraging pact-foundation/pact-reference#356 tracking progress in pact-foundation/pact-ruby#317 will close this off now, and it may revisited in the future. |
Creating PR to start discussion.
This adds support for a Pact::Jwt object. The matching is done on the content of the Jason Web Token, rather than on the encoded strings. Pact.terms, Pact.SomethingLike and other goodies can be included in the jwt.
If there is interest in getting this merged, I need a add a couple of tests on the jwt class itself.
Potentially, we should make the dependency on jwt gem only needed for people using Pact.Jwt