Skip to content
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

Update jwt#shopify_user_id to return an integer #1103

Merged
merged 1 commit into from
Nov 10, 2020

Conversation

rezaansyed
Copy link
Contributor

@rezaansyed rezaansyed commented Nov 6, 2020

It seems that we've introduced a slight bug for developers who are developing with the shopify_app gem.

Now that core returns the sub field of a jwt as a string, the following validation fails for any app that is developing with online access tokens using shopify_app:

auth_hash && jwt_shopify_domain == shop_name && jwt_shopify_user_id == associated_user_id

In this scenario, the associated_user_id parsed by omniauth remains an Integer, but the jwt_shopify_user_id parsed by our middleware is now a String. There is a type mismatch. The easiest solution for this is to cast the sub field as an integer in our middleware to ensure that everything still works with omniauth as intended (I think this is fine because it's the developer's prerogative to decide what to do with the sub field).

This issue was flagged by Sneha here: https://shopify.slack.com/archives/CG71G24BZ/p1604674034310200

Before submitting the PR, please consider if any of the following are needed:

  • Update CHANGELOG.md if the changes would impact users
  • Update README.md, if appropriate.
  • Update any relevant pages in docs/, if necessary
  • For security fixes, the Disclosure Policy must be followed.

@rezaansyed rezaansyed merged commit e926d14 into master Nov 10, 2020
@jgray7019 jgray7019 temporarily deployed to rubygems December 10, 2020 20:54 Inactive
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.

None yet

5 participants