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

Token exchange not triggered after app reinstall #1886

Open
kirillplatonov opened this issue Jul 26, 2024 · 5 comments
Open

Token exchange not triggered after app reinstall #1886

kirillplatonov opened this issue Jul 26, 2024 · 5 comments

Comments

@kirillplatonov
Copy link
Contributor

I'm using token exchange auth and managed installs.

When I install the app for the first time - the new Shop record is created and ShopifyApp::Auth::TokenExchange is called.

When I re-install the app - the Shop record is already present in DB and ShopifyApp::Auth::TokenExchange not called automatically so shopify_token is not updated and post_authenticate_jobs are not called.

Expected behavior

ShopifyApp::Auth::TokenExchange should be called after app re-install.

Actual behavior

ShopifyApp::Auth::TokenExchange not called after re-install.

Demo

CleanShot.2024-07-26.at.15.45.07.mp4
@Arkham
Copy link
Contributor

Arkham commented Jul 26, 2024

Hey Kirill,

thanks for reporting this issue, I've tagged the team working on this!

@kirillplatonov
Copy link
Contributor Author

Current workaround

To fix this issue in my app I end up adding controller callback that triggers token exchange on the first app loading from admin using pretty complex condition:

class AuthenticatedController < ApplicationController
  before_action :perform_token_exchange

  def perform_token_exchange
    return if params[:embedded] != "1" || params[:id_token].blank?
    return unless request.path == "/home"

    new_session = ShopifyApp::Auth::TokenExchange.perform(shopify_id_token)
    current_shopify_session.copy_attributes_from(new_session)
  end
end

And I now have to trigger AfterAuthenticateJob manually from Shop model to be sure it will fire only after install/re-install, when shopify_token is updated:

class Shop < ActiveRecord::Base
  after_save_commit :check_shopify_token_change

  def check_shopify_token_change
    return unless shopify_token_previously_changed?

    AfterAuthenticateJob.perform_now(shop_domain: shopify_domain)
  end
end

It works well but it's a pretty hacky solution.

Proper fix

To properly fix the issue, we need to have some indication from ShopifyAdmin/AppBridge that the request is coming to the app after managed installation. Right now there's no difference in regular app visit and app installation. The best fix would be providing a parameter (eg installed=1) to the first app visit after managed installation. This way we could trigger token exchange when we detect this parameter.

@zzooeeyy
Copy link
Contributor

Hey @kirillplatonov, I'll bring this up to the team to see how we'd like to proceed for this. We didn't have a way to handle this because our template app defaults to destroying the record upon uninstallation. In the mean time, I've seen apps implement this in a less hacky way:

  1. New column in Shop table for uninstalled
  2. Upon receiving uninstalled webhook, set Shop.uninstalled = true
  3. In AuthenticatedController perform token exchange if uninstalled, and set uninstalled=false

I hope this helps in the meantime. Thanks!

@kirillplatonov
Copy link
Contributor Author

Destroying Shop is not suitable for production apps, unfortunately. We need to keep data during grace period until fully erasing them. And even after erasing app data, we still need to keep Shop record without personal data to track possible app reinstalls in the future and offer support. We need a better default for handling uninstalls in shopify_app template.

Regarding suggested workaround with uninstalled column. In my apps I'm using uninstalled_at column to detect uninstalls too. However, this solely relies on webhooks and it won't work in cases when the app re-installed right away after uninstall. This is not common among case among merchants, but this is specific case that Shopify App Store review team check apps for. So we have to handle it properly just to pass regular app review. And we can't solely rely on webhooks - we need to perform exchange properly after each app reinstall automatically.

@uurcank
Copy link
Contributor

uurcank commented Jul 29, 2024

@kirillplatonov @zzooeeyy i have this workaround which might be good for reinstallation

  1. upon uninstall replace shop token with 'na' (also override model validations to enable this)
  2. override construct_session in shop model to return early if token is 'na'
def self.def self.construct_session(shop)
    return unless shop
    return if shop.token == "na"

    ShopifyAPI::Auth::Session.new(
      shop: shop.shopify_domain,
      access_token: shop.shopify_token,
      scope: shop.access_scopes || ShopifyApp.configuration.scope
    )
  end 

this forces a token exchange from what i see so you would not need to override the controller.

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

No branches or pull requests

4 participants