Skip to content

Exchange id_token when it is present#1974

Closed
jduff wants to merge 1 commit intoShopify:mainfrom
jduff:exchange_token_when_present
Closed

Exchange id_token when it is present#1974
jduff wants to merge 1 commit intoShopify:mainfrom
jduff:exchange_token_when_present

Conversation

@jduff
Copy link
Contributor

@jduff jduff commented May 6, 2025

What this PR does

Fixes #1886

Reviewer's guide to testing

There are a couple of cases myself and others have ran into where the access token or scopes are not updated when using token exchange:

  • When the user uninstalls and reinstalls the app (and you do not hard delete the shop for data retention reasons)
  • When managed app install scopes are updated

In my testing when these cases occur we get an id_token back in the url params and this is a good indication that something has changed. Based on #1886 it seems like others have found this works for them as well. Triggering retrieve_session_from_token_exchange in this case causes all session storage and (I believe but didn't test) after authenticate triggers to run.

I haven't looked too much into the authentication header but it's possible that checking shopify_id_token is present would be a better approach to take into account that header having the id_token. I'm not sure what situations that header is present in through and my assumption is that most of those cases we don't necessarily need to update the scopes/access_token and the cases where we do want to update them it will be included in the url params.

Things to focus on

  1. Make sure my assumptions about when this should trigger are correct.
  2. Check if it would make sense to trigger this when the authentication header is present as well.

Checklist

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.

@jduff jduff requested a review from a team as a code owner May 6, 2025 14:30
@kirillplatonov
Copy link
Contributor

I don't think it's a good permanent solution. id_token is always included in params when the app is opened in Shopify Admin, regardless of installation/scope changes. With these changes, we'll end up making token exchange every time when the app is opened. It will slow down app loading by 200-300ms, which will impact perf metrics and our abilities to maintain Build for Shopify status.

As mentioned in #1886, I still believe we need some proper indication from App Bridge, that the app needs to update token after installation/scopes changes. The easiest solution would be new parameter (eg reload_token=1). This way, we could make token exchange only when it's really needed.

@jduff
Copy link
Contributor Author

jduff commented May 6, 2025

@kirillplatonov Thanks, ya, I'm seeing what you mean now as well. I'll close this.

@jduff jduff closed this May 6, 2025
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.

Token exchange not triggered after app reinstall

2 participants