Skip to content

Conversation

jeroendelau
Copy link
Contributor

Checkout UI Extensions have session tokens, but they vary slightly from the token provided by Shopify Apps.

App Session Tokens
Checkout Extension Session Tokens

Key differences:

  • iss, sid are not part of the token
  • sub, optional
  • dest: has no protocol prefix

By merging this PR people should be able to use session tokens obtained through useSessionToken, to interact with the back-end without modification.

@jeroendelau jeroendelau changed the title modify SessionToken to work with from Checkout UI Extension modify SessionToken to be compatible with Checkout UI Extension Jul 24, 2024
@jeroendelau jeroendelau force-pushed the app-extension-support branch from 9b59acd to 99bc6cd Compare July 24, 2024 09:16
@jeroendelau jeroendelau marked this pull request as ready for review July 24, 2024 09:16
@Kyon147
Copy link
Owner

Kyon147 commented Aug 2, 2024

@jeroendelau are you able to add a wiki page on how this would be use for the checkout extension vs the main laravel application?

@jeroendelau
Copy link
Contributor Author

absolutely, I will try to get something in there before the end of the week.

@Kyon147
Copy link
Owner

Kyon147 commented Sep 5, 2024

@jeroendelau great work on this.
Just checking on this, do you need anything from me to get this passed with the check "linting" and then we can get it merged in

@jeroendelau
Copy link
Contributor Author

No, sorry, simply had no time. Will fix

@jeroendelau
Copy link
Contributor Author

I think I fixed all linter issues, but I'm having trouble reproducing the same error locally.
Added a wiki page here: https://github.com/Kyon147/laravel-shopify/wiki/Working-with-App-Extensions

@jeroendelau
Copy link
Contributor Author

jeroendelau commented Sep 12, 2024

Added in separate PR #346

The reason I couldn't get the linter to fail was because the local version does not match the version of github actions.

github action 3.0.0
composer ^3.0 (which results in 3.59.3).

After setting composer to 3.0.0 the errors matched.

Suggest adjusting the lint action to use the composer version so mismatch can't happen.

name: PHP Lint

on: [ push, pull_request ]

jobs:
  lint:
    runs-on: ubuntu-latest

    steps:
      - name: Checkout code
        uses: actions/checkout@v2

      - name: Setup PHP
        uses: shivammathur/setup-php@v2
        with:
          php-version: '8.2'
          coverage: none

      - name: Get Composer cache directory
        id: cache-composer
        run: echo "dir=$(composer config cache-files-dir)" >> $GITHUB_OUTPUT

      - name: Restore Composer cache
        uses: actions/cache@v3
        with:
          path: ${{ steps.cache-composer.outputs.dir }}
          key: ${{ runner.os }}-${{ github.ref_name }}-composer-${{ hashFiles('**/composer.lock') }}

      - name: Install Composer dependencies
        run: |
          composer install --no-interaction --prefer-dist

      - name: Check for code style violation with PHP-CS-Fixer
        run: vendor/bin/php-cs-fixer fix --diff

Copy link
Owner

@Kyon147 Kyon147 left a comment

Choose a reason for hiding this comment

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

Looks great

@Kyon147 Kyon147 merged commit 1cc5029 into Kyon147:master Sep 24, 2024
6 checks passed
@blazerunner44
Copy link

@jeroendelau Thanks for this PR! I've been working on a Customer Account UI extension. I've noticed this code improperly classifies the session token from the Customer Account UI extension as an app session token because the iss is included in the body. I don't have a Checkout UI extension to test with right away, but wanted to get your thoughts.

https://shopify.dev/docs/api/customer-account-ui-extensions/latest

image

@jeroendelau
Copy link
Contributor Author

@blazerunner44 happy to hear someone is using it! I haven't worked with it for a while, so I didn't encounter this myself. But let's see if we can't resolve this.

So the original issue was that theAssert::thatAll([ .... ]) on line 182 failed. Since the checkout token did not include three of those fields consistently.

However, I didn't simply want to remove the check, so I tried to create a strict a scenario as I could to only skip checking it if specific fields were missing, and all missing.

Now that Shopify is sometimes adding these fields, it starts to become brittle.

So, from my perspective, We have two options:

  1. remove the iss from determineTokenSource`
  • Still attempt to communicate where the token might have come from
  • If later Shopify decides to also issue ['sub'], it fails again
  • Most backward compatible
  1. Remove the requirement for iss, sid and sub to be not_null
  • This will weaken checks for all tokens
  • makes it future proof

add. with option 2, the need to determine tokenSource goes out the window. It could be removed, but I can't be sure that nobody is relying on it. Maybe we can find a different method?

@blazerunner44
Copy link

@jeroendelau Appreciate you taking the time with a thoughtful response. This is my first dive into Shopify and am very much figuring things out as I go.

Shopify didn't issue the iss field back when this PR was being worked on, but now it is included. Is it safe to say this code doesn't work across the board right now, or could there be cases where Shopify still doesn't issue the iss field (older API versions, Checkout UI vs Account UI extensions, etc)?

I do like the method to determine the tokenSource. Seems useful for people who might want to determine how a request is received.

A quick dive into the Shopify documentation about the sid field on the token indicates:
sid: A unique session ID per user and app.
Shopify docs

To me it makes sense that a sid wouldn't be issued for a checkout/account UI extension as there's no authenticated Shopify user using the app. Unless they change the definition in the future, it seems safe to remove the check for iss from determineTokenSource and rely soely on the sid field, your option #1 above.

@jeroendelau
Copy link
Contributor Author

jeroendelau commented Sep 26, 2025 via email

@Kyon147
Copy link
Owner

Kyon147 commented Sep 26, 2025

I've been following your conversation, let me know when you want a review on this PR and thanks for your help 🙏

@blazerunner44
Copy link

@jeroendelau @Kyon147 Quick PR to remove the iss field check #452

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