params[:id_token] not being considered in this gem

[awd]

Issue summary

Shopify admin is including params[:id_token] when opening an already installed embedded app.

Unfortunately, this gem still only looks for the token in the auth header (ie: from a fetch request). Despite the id_token and the value returned from app bridge shopify.idToken() matching, this gem does not authenticate the param.

This causes unnecessary redirects in the embedded app iframe when opening the app.

This is most reproduceable when clicking a link which opens the app for the end user, like a link in their email, or a support tool opening a deeply nested page in the app itself.

ex: https://admin.shopify.com/store/test-store/apps/test-app/some/nested/route

The app will receive the GET request to /some/nested/route with a bunch of Shopify injected params (embedded, hmac, shop, id_token, etc). This is an authenticated link ,and should not trigger redirects (likely from ShopifyApp::EnsureAuthenticatedLinks).

Are there plans to support this ASAP?

[ragalie]

Thank you for the feature request! Yes, we'd like to implement this in the gem, but we haven't gotten to it in our backlog yet. I agree that it is a nice performance enhancement to utilize the id_token in the URL if we can.

[awd]

Earlier today @kirillplatonov shared this middleware to solve this, and @developit mentioned the id_token should be stable. Would be great to see updated docs in Shopify around this, and of course in the gem.

All of the token verification remains the same within the gem as well.

class AppBridgeMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    request = Rack::Request.new(env)

    if request.params.has_key?('id_token') && !env['HTTP_AUTHORIZATION']
      env['HTTP_AUTHORIZATION'] = %(Bearer #{request.params['id_token']})
    end

    @app.call(env)
  end
end

[jweslley]

I made an extra change to @kirillplatonov 's middleware suggestion since I'm using ShopifyApp::EnsureAuthenticatedLinks

class AppBridgeMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    request = Rack::Request.new(env)

    if request.params.has_key?('id_token') && !env['HTTP_AUTHORIZATION']
      env['HTTP_AUTHORIZATION'] = %(Bearer #{request.params['id_token']})
      jwt_payload = ShopifyAPI::Auth::JwtPayload.new(request.params['id_token'])
      env['jwt.shopify_domain'] = jwt_payload.shop
    end

    @app.call(env)
  end
end

[awd]

I made an extra change to @kirillplatonov 's middleware suggestion since I'm using ShopifyApp::EnsureAuthenticatedLinks

class AppBridgeMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    request = Rack::Request.new(env)

    if request.params.has_key?('id_token') && !env['HTTP_AUTHORIZATION']
      env['HTTP_AUTHORIZATION'] = %(Bearer #{request.params['id_token']})
      jwt_payload = ShopifyAPI::Auth::JwtPayload.new(request.params['id_token'])
      env['jwt.shopify_domain'] = jwt_payload.shop
    end

    @app.call(env)
  end
end

I think this is only needed depending where you've inserted the AppBridgeMiddleware. If before the ShopifyApp JWT - this should not be required.

something like: config.middleware.insert_before 0, AppBridgeMiddleware

[zzooeeyy]

Hey everyone, thanks for your patience on this issues -

We have added support for params[:id_token] in this PR, and it has been released as a part of v22.2.0

[hoppergee]

I found this line in method with_token_refetch which is using by the new token exchange module.

https://github.com/Shopify/shopify_app/blob/v22.2.0/lib/shopify_app/admin_api/with_token_refetch.rb#L15

Will this be too dangerous, and accidentally delete the shop in the database due to some accidents?

[zzooeeyy]

Hi @hoppergee, it only gets deleted if it encounters a 401 error, which means the access/session token isn't invalid anymore, in which case it would get deleted. But if the app is using token exchange auth method, the next time the user tries to start a session, it'll automatically exchange token and store it in the database again.

[kirillplatonov]

Destroying Shop on invalid token is terrible idea. We have data connected to Shop and they all will be lost. We keep data for 90 days to let merchants reinstall app and continue using it without interruption. And even after 90 days we delete all data except Shop record to be able to track merchant installation history in case of support queries.

All we need to do on 401 error is to remove shopify_token from Shop and populate it again on app reinstall. At it's current state this sounds like a breaking change without prior notice and major version bump. I think the best approach would be to remove this change and release patch version ASAP until it damages data for partners that accidentally upgraded to this minor version.

[awd]

100% Agree with @kirillplatonov on this one - this gem should not make any assumptions on how to manage data after the shop has been installed.

This is entirely an app concern and not a gem concern.

Please rollback immediately!!

Destroying Shop on invalid token is terrible idea. We have data connected to Shop and they all will be lost. We keep data for 90 days to let merchants reinstall app and continue using it without interruption. And even after 90 days we delete all data except Shop record to be able to track merchant installation history in case of support queries.

All we need to do on 401 error is to remove shopify_token from Shop and populate it again on app reinstall. At it's current state this sounds like a breaking change without prior notice and major version bump. I think the best approach would be to remove this change and release patch version ASAP until it damage data for partners that accidentally upgraded to this minor version.

[zzooeeyy]

Hi everyone, thanks for your raised concerns, we'll work on removing deletion of the Shop right away. This should not affect majority of the app that are upgrading since it's only used inside the Token Exchange auth strategy - it's a manual configuration to enable this. It won't affect existing app's OAuth flow. But we'll still work on a patch right away for apps migrating to token exchange. Thanks!

[kirillplatonov]

Thank you! 🙏