Implement JWT callback endpoint #951

ragalie · 2020-04-13T12:58:53Z

Goals

When we receive an OAuth callback with a JWT in the headers, we want to do two things:

Validate that the omniauth data matches the JWT data.
Persist the token information to the session store.

Implementation

It's a bit of a pain to have two separate callback paths in omniauth. There is a callback_path option, and it does take a block, but I'm a little unclear about how it knows how to start the normal OAuth flow if you use a block for this option. I elected to incorporate the JWT flow into the existing callback path in a backwards-compatible fashion.
Omniauth tries to check the state param as part of the callback action and throws an error if the param doesn't exist or doesn't match the param in the session. We won't have a param in the session in this context. So we need to conditionally disable the state check, based on whether we have a valid JWT. This is safe, I believe, because the goal of the state check is to avoid CSRF, and we can trust the JWT as evidence that the user is doing this from the Shopify context.

To make this work, I moved the JWT logic into a Rack middleware. This has two benefits: (1) it means that other Rack middleware (such as omniauth) can make decisions based on the JWT data, and (2) it allows any Rack application to decode our JWT, even if they're not using Rails or the shopify_app controller stuff.

theundeadmonk · 2020-04-15T19:37:10Z

Is there a reason we prefer not to override the old callback endpoint to handle both cases based on whether enable_jwt_auth is set in the config?

Would that be easier since we then wouldn't have to keep track of both urls.

ragalie · 2020-04-15T19:40:54Z

I don't think we can do that because the existing callback endpoint has to run for the initial install (to get the shop token), even if allow_jwt_authentication is true.

theundeadmonk · 2020-04-16T13:17:55Z

app/controllers/shopify_app/callback_controller.rb

+    def jwt_callback
+      if valid_jwt_auth?
+        create_user_session_from_jwt_callback
+        head(:ok)


Should we use head(:no_content) instead?

I'd rather stick with OK because I think we're likely to add some content later, and I don't want apps to break when the status code switches from 204 => 200.

rezaansyed · 2020-04-16T14:23:45Z

app/controllers/shopify_app/callback_controller.rb

@@ -24,8 +24,31 @@ def callback
      end
    end

+    def jwt_callback


Since this is only for user tokens, should we call this user_callback?

I think it's more tightly coupled to the jwt aspect than the user aspect, since the other callback endpoint can also handle user tokens.

ragalie · 2020-04-20T14:21:01Z

Ok, so there are some issues with the current approach :) Here's what I've discovered:

It's a bit of a pain to have two separate callback paths in omniauth. There is a callback_path option, and it does take a block, but I'm a little unclear about how it knows how to start the normal OAuth flow if you use a block for this option. I'll probably switch to using /callback for both (as Adi originally suggested) since I'll be fighting less with omniauth.
Omniauth tries to check the state param as part of the callback action and throws an error if the param doesn't exist or doesn't match the param in the session. We won't have a param in the session in this context. So we need to conditionally disable the state check, based on whether we have a valid JWT. This is safe, I believe, because the goal of the state check is to avoid CSRF, and we can trust the JWT as evidence that the user is doing this from the Shopify context. cc @leleabhinav
We'll need to update the logic in the omniauth gem to support three settings for what kind of tokens are expected: only shop, only user, shop or user. Right now it supports either shop or user, and we use a hacky session thing to work around this, and that isn't viable for our use case. Since it's quite common to use both shop and user tokens, we should just natively support this.

I'll work on this later today and we can see how it looks and if we like it.

theundeadmonk · 2020-04-22T13:43:06Z

LGTM.
I was wondering if we could perhaps reject the request in the middleware itself if the JWT is invalid. Not sure if we have the Shop or the User info there, but we could check for things like expiry, etc. Just a thought.

ragalie · 2020-04-22T13:46:05Z

I'm reluctant to do that because some apps might use other bearer tokens in parts of their app. I think rejecting the request is more of a controller concern.

greatestape · 2020-04-22T14:23:14Z

app/controllers/shopify_app/callback_controller.rb

+        jwt_callback
+        return
+      end
+


Should this be if/elsif/else blocks rather than having the early return?

greatestape · 2020-04-22T14:25:44Z

app/controllers/shopify_app/callback_controller.rb

        login_shop



Should this block of code be extracted out for single-level-of-abstraction reasons? It looks like we're doing that with jwt_callback above, and it seems nice.

I tried cleaning this up for clarity, take a look and see what you think!

app/controllers/shopify_app/callback_controller.rb

lib/shopify_app/middleware/jwt_middleware.rb

app/controllers/shopify_app/callback_controller.rb

rezaansyed

Tophatted it with Adi and works as expected 👍

ragalie · 2020-05-05T13:08:35Z

@leleabhinav are you OK with where this ended up?

OAuth returns 'id' not 'shopify_user_id'

This decouples retrieving the JWT data from the Rails controller layer so that we can use this data when making decisions in the omniauth middleware.

It's a bit of a pain to have two separate callback paths in omniauth. There is a callback_path option, and it does take a block, but I'm a little unclear about how it knows how to start the normal OAuth flow if you use a block for this option. To keep things simple I went with the single callback endpoint.

Technically it does more than we need, but we protect against the other paths elsewhere, and this simplies the logic overall.

leleabhinav · 2020-05-05T15:01:15Z

@leleabhinav are you OK with where this ended up?
👍 🚢

ragalie · 2020-05-05T16:45:43Z

@Shopify/platform-dev-tools-education I'm ready for a review on this when you have a chance :) Thanks!

tanema

This looks good and an interesting approach. I wonder if in the future, the middleware could be extracted so that we could support more than just rails.

ragalie · 2020-05-06T16:50:13Z

For sure, I'm thinking we could pretty easily make that move if/when we think it would be useful.

ragalie self-assigned this Apr 13, 2020

ragalie mentioned this pull request Apr 14, 2020

Implement Session#== Shopify/shopify-api-ruby#714

Merged

ragalie force-pushed the jwt-callback branch from 2bb9e9f to 06e125d Compare April 15, 2020 15:43

ragalie marked this pull request as ready for review April 15, 2020 16:03

ragalie requested a review from a team as a code owner April 15, 2020 16:03

ragalie requested review from rezaansyed and greatestape April 15, 2020 16:03

theundeadmonk reviewed Apr 16, 2020

View reviewed changes

rezaansyed reviewed Apr 16, 2020

View reviewed changes

ragalie force-pushed the jwt-callback branch from 06e125d to 1eba818 Compare April 20, 2020 18:32

theundeadmonk approved these changes Apr 22, 2020

View reviewed changes

greatestape reviewed Apr 22, 2020

View reviewed changes

app/controllers/shopify_app/callback_controller.rb Outdated Show resolved Hide resolved

greatestape reviewed Apr 22, 2020

View reviewed changes

app/controllers/shopify_app/callback_controller.rb Show resolved Hide resolved

greatestape reviewed Apr 22, 2020

View reviewed changes

lib/shopify_app/middleware/jwt_middleware.rb Show resolved Hide resolved

greatestape reviewed Apr 22, 2020

View reviewed changes

lib/shopify_app/middleware/jwt_middleware.rb Outdated Show resolved Hide resolved

greatestape reviewed Apr 22, 2020

View reviewed changes

app/controllers/shopify_app/callback_controller.rb Show resolved Hide resolved

rezaansyed approved these changes Apr 23, 2020

View reviewed changes

ragalie force-pushed the jwt-callback branch from 48b35de to d279f6b Compare April 24, 2020 13:27

ragalie added 4 commits May 5, 2020 09:09

Correct data shape

5fe0adb

OAuth returns 'id' not 'shopify_user_id'

Define expected behaviour

f649ede

Use updated ShopifyAPI with equality

d302de7

Implement jwt_callback

602c3f5

ragalie added 8 commits May 5, 2020 09:09

Refactor to single layer of abstraction

8245f20

Extract JWT data in Rack middleware

58690e4

This decouples retrieving the JWT data from the Rails controller layer so that we can use this data when making decisions in the omniauth middleware.

Always return a valid response

4e3d7fa

Execute next middleware _after_ setting jwt variables

3831487

Use set_shopify_session for JWT callback

c62dc87

Technically it does more than we need, but we protect against the other paths elsewhere, and this simplies the logic overall.

Rename method

313b20b

Refactor

f7db777

ragalie force-pushed the jwt-callback branch from d279f6b to f7db777 Compare May 5, 2020 13:11

leleabhinav approved these changes May 5, 2020

View reviewed changes

tanema approved these changes May 6, 2020

View reviewed changes

ragalie merged commit 914f992 into master May 6, 2020

ragalie deleted the jwt-callback branch May 6, 2020 16:51

rezaansyed temporarily deployed to rubygems May 7, 2020 16:16 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement JWT callback endpoint #951

Implement JWT callback endpoint #951

ragalie commented Apr 13, 2020 •

edited

Loading

theundeadmonk commented Apr 15, 2020

ragalie commented Apr 15, 2020

theundeadmonk Apr 16, 2020

ragalie Apr 20, 2020

rezaansyed Apr 16, 2020

ragalie Apr 21, 2020

ragalie commented Apr 20, 2020

theundeadmonk commented Apr 22, 2020

ragalie commented Apr 22, 2020

greatestape Apr 22, 2020

greatestape Apr 22, 2020

ragalie Apr 24, 2020

rezaansyed left a comment

ragalie commented May 5, 2020

leleabhinav commented May 5, 2020

ragalie commented May 5, 2020

tanema left a comment

ragalie commented May 6, 2020

Implement JWT callback endpoint #951

Implement JWT callback endpoint #951

Conversation

ragalie commented Apr 13, 2020 • edited Loading

Goals

Implementation

theundeadmonk commented Apr 15, 2020

ragalie commented Apr 15, 2020

theundeadmonk Apr 16, 2020

Choose a reason for hiding this comment

ragalie Apr 20, 2020

Choose a reason for hiding this comment

rezaansyed Apr 16, 2020

Choose a reason for hiding this comment

ragalie Apr 21, 2020

Choose a reason for hiding this comment

ragalie commented Apr 20, 2020

theundeadmonk commented Apr 22, 2020

ragalie commented Apr 22, 2020

greatestape Apr 22, 2020

Choose a reason for hiding this comment

greatestape Apr 22, 2020

Choose a reason for hiding this comment

ragalie Apr 24, 2020

Choose a reason for hiding this comment

rezaansyed left a comment

Choose a reason for hiding this comment

ragalie commented May 5, 2020

leleabhinav commented May 5, 2020

ragalie commented May 5, 2020

tanema left a comment

Choose a reason for hiding this comment

ragalie commented May 6, 2020

ragalie commented Apr 13, 2020 •

edited

Loading