-
Notifications
You must be signed in to change notification settings - Fork 684
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
Remove JWT middleware #1861
Remove JWT middleware #1861
Conversation
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.
Very nice! This LGTM, though others should review too.
Admittedly, I'm having troubles following the code that actually ensures a shopify app verifies the ID token is present and valid. The code I'm reading seems to indicate we do some kind of token exchange - but I thought we fully relied on the JWT, without any external calls.
Could you please walk me through the flow?
Thanks @tgwizard! Will walk you through it according to my understanding:
|
end | ||
|
||
def jwt_payload | ||
@jwt_payload ||= shopify_id_token.present? ? ShopifyAPI::Auth::JwtPayload.new(shopify_id_token) : nil |
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.
Is there some way we can skip parsing the JWT token twice, once here, and once in https://github.com/Shopify/shopify-api-ruby/blob/a64f7dd391306033ca58399b42fa32207ba6dc34/lib/shopify_api/utils/session_utils.rb#L48?
(That could be a follow-up optimization).
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 idea. Looks like that would be a change in the shopify-api-ruby
repo, so will leave it as a follow-up.
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.
The changes makes sense to me, have you tried top-hatting with a rails app ?
@@ -266,6 +265,7 @@ PLATFORMS | |||
|
|||
DEPENDENCIES | |||
byebug | |||
jwt (>= 2.2.3) |
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'm not sure what impact this might have? how did this line get moved?
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.
This was a result of updating the gemspec as explained in the PR description:
The jwt gem has been changed from runtime dependency to dev dependency (it's still used to create JWTs in unit tests)
0ee1edd
to
8695f06
Compare
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.
LGTM!
252fc80
to
f97f022
Compare
I tested this using a new test app created with https://github.com/Shopify/shopify-app-template-ruby and it's working as expected 👍 |
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.
Looks good to me! Thanks for top-hatting!
f97f022
to
a53b692
Compare
What this PR does
Closes #1744
Removes the JWT middleware, since JWT authentication was moved to
shopify-api-ruby
in v19 and then the relatedShopifyApp::JWT
class was deprecated here (see upgrading guide).request.env["jwt.shopify_domain"]
), notably inShopifyApp::WithShopifyIdToken
ShopifyApp::WithShopifyIdToken
has been updated to no longer check values inrequest.env["jwt.*"]
, and instead only check header and URL param valuesrequest.env["jwt.*"]
have been updated to use the respective methods inShopifyApp::WithShopifyIdToken
.jwt
gem has been changed from runtime dependency to dev dependency (it's still used to create JWTs in unit tests)Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.md
if the changes would impact usersREADME.md
, if appropriate./docs
, if necessary