Skip to content

Conversation

@BaggioGiacomo
Copy link
Contributor

@BaggioGiacomo BaggioGiacomo commented Mar 28, 2025

Description

The generated session token on the checkout ui extensions doesn't contain:

  • The sid claim
  • The sub claim if the customer is not logged in

Having them mandatory causes an Expected type String, got type NilClass error

How has this been tested?

After adding the patch, I've made my app use my local version of the shopify-api gem:

gem "shopify_api", path: "/Users/jack/Documents/GitHub/shopify-api-ruby"

After that, I've tested my app and session works correctly

I've finally tested the checkout ui extension and no errors is thrown now. Also I'm now able to get the logged in customer by decoding the jwt

Checklist:

  • My commit message follow the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added a changelog line.

@BaggioGiacomo BaggioGiacomo force-pushed the make-jwt-sid-and-sub-claims-optional branch from fc02a05 to 96be21d Compare March 28, 2025 15:48
sig { returns(Integer) }
def shopify_user_id
@sub.to_i
end
Copy link
Contributor Author

@BaggioGiacomo BaggioGiacomo Mar 28, 2025

Choose a reason for hiding this comment

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

How can this work?

If the sub is gid://shopify/Customer/123456789, shouldn't this method return 123456789?
gid://shopify/Customer/123456789.to_i returns 0

@lizkenyon I think this is the answer to your question here: https://github.com/Shopify/shopify-api-ruby/pull/1346/files#r1831667330

@BaggioGiacomo BaggioGiacomo force-pushed the make-jwt-sid-and-sub-claims-optional branch from 96be21d to 300e713 Compare March 31, 2025 12:59
The generated session token on the checkout ui extensions doesn't contain:
- The `sid` claim
- The `sub` claim if the customer is not logged in

Making them mandatory causes an `Expected type String, got type NilClass` error
@BaggioGiacomo BaggioGiacomo force-pushed the make-jwt-sid-and-sub-claims-optional branch from 300e713 to 4bc8538 Compare March 31, 2025 13:48
@BaggioGiacomo BaggioGiacomo marked this pull request as ready for review March 31, 2025 13:48
@BaggioGiacomo BaggioGiacomo requested a review from a team as a code owner March 31, 2025 13:48
Copy link
Contributor

@lizkenyon lizkenyon left a comment

Choose a reason for hiding this comment

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

A few comments!

One question I have is how are you (or how are you planning to) currently validating your session token in your app? ie. Are you using a controller concern from shopify_app?

@BaggioGiacomo
Copy link
Contributor Author

One question I have is how are you (or how are you planning to) currently validating your session token in your app? ie. Are you using a controller concern from shopify_app?

Yes, I've an AuthenticatedController that includes ShopifyApp::EnsureHasSession concern. new_embedded_auth_strategy is set to true, token exchange is used

@BaggioGiacomo BaggioGiacomo requested a review from lizkenyon April 1, 2025 07:13
Copy link
Contributor

@lizkenyon lizkenyon left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! ⭐

@lizkenyon lizkenyon merged commit b8eee39 into Shopify:main Apr 1, 2025
5 checks passed
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.

2 participants