Store session expiry and check it to trigger a re-auth

[gbzodek]

What this PR does

Embedded apps using user tokens currently have to wait for a 401 from Shopify to know that the user token is not valid anymore. By storing the token expiry date, we are able to detect an expired session without a round trip to Shopify and to trigger the Oauth flow to get a fresh token.

Goals of this PR are:

• to store the user token expiry date and to check it during the session check. This allows to trigger the Oauth flow early and has the nice side effect of removing useless calls to Shopify
• to align shopify_app with shopify-app-js which already stores the expiry date in the data model (Prisma one for example) and checks it here.
• to prepare for the integration of the new token exchange flow which will also be able to leverage that date

For that, the PR:

• updates the User model with an expires_at attribute. A DB migration is created with a prompt when running rails generate shopify_app:user_model
• creates a new UserSessionStorageWithScopesAndExpiry concern that will store/retrieve the session expiry date (which is already set during Oauth here). This will be included in the User model for new apps and be an opt-in for existing apps.
• check if the session is expired in the login protection to trigger a re-auth

Reviewer's guide to testing

• create a new Shopify Rails app with user sessions leveraging UserSessionStorageWithScopes (or use an existing one)
• update the Gemfile to use that branch
  • gem "shopify_app", git: 'https://github.com/Shopify/shopify_app.git', branch: 'store-session-expiry'
  • bundle install

Backward compatibility (config flag off and no expiry date column)

• Check that sessions can still be saved and retrieved

user = ShopifyAPI::Auth::AssociatedUser.new(id: 1, first_name: "first name", last_name: "last name", email: "my.email@email.com", email_verified: true, account_owner: true, locale: "en", collaborator: true)
session = ShopifyAPI::Auth::Session.new(shop: "shop1.myshopify.com", access_token: "token1", scope: ["read_products"], associated_user: user, expires: Time.now + 1.day)
ShopifyApp::SessionRepository.store_session(session)
ShopifyApp::SessionRepository.load_session("shop1.myshopify.com_1")

backward-compatibility.mp4

Config flag on and no expiry date column

• set the new config flag check_session_expiry_date to true in shopify_app.rb
• restart the rails server (rails restart)
• check that you get an error when trying to save or retrieve sessions as the flag is on but the model has no expiry date

user = ShopifyAPI::Auth::AssociatedUser.new(id: 2, first_name: "first name", last_name: "last name", email: "my.email@email.com", email_verified: true, account_owner: true, locale: "en", collaborator: true)
session = ShopifyAPI::Auth::Session.new(shop: "shop1.myshopify.com", access_token: "token2", scope: ["read_products"], associated_user: user, expires: Time.now + 1.day)
ShopifyApp::SessionRepository.store_session(session)
ShopifyApp::SessionRepository.load_session("shop1.myshopify.com_1")

model-with-no-expiry-date.mp4

Config flag on and expiry date column

• check the User model is correctly generated:
  • rails generate shopify_app:user_model --skip -> answer Y to the prompt to include the expiry_date migration
    -> check that a DB migration was created to add the expires_at column to the users table
  • rails db:migrate
• check that the session expiry date is well stored and retrieved. The @expires attribute should not be nil when retrieving the session

user = ShopifyAPI::Auth::AssociatedUser.new(id: 3, first_name: "first name", last_name: "last name", email: "my.email@email.com", email_verified: true, account_owner: true, locale: "en", collaborator: true)
session = ShopifyAPI::Auth::Session.new(shop: "shop1.myshopify.com", access_token: "token3", scope: ["read_products"], associated_user: user, expires: Time.now + 1.day)
ShopifyApp::SessionRepository.store_session(session)
ShopifyApp::SessionRepository.load_session("shop1.myshopify.com_3")

model-with-expiry.mp4

Expiry date check

• on an app with the updated User model and check_session_expiry_date set to true, check that the user session expiry date is stored in the users table (you need to restart the app after updating)
• manually change the expiry date to be in the past
• hit the app and check the login flow is triggered before any call to the Shopify API is done

Things to focus on

1. Could this have any bad side effect?

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.

[nelsonwittwer]

Thanks for your contribution here! This idea makes a lot of sense to me! I've got this on my list to review and triage, but it might take a day or two. I appreciate your patience!

[nelsonwittwer]

👋 Thanks so much for this contribution! I ❤️ the direction for sure, just have a few thoughts to chat through before we merge this in:

Introducing a new controller concern

One of the more frustrating aspects of maintaining this gem is managing what concern does what. I'm nervous to add another concern into the mix for us to maintain 😬

Since I love this as a default option, I wonder if we can add this functionality to our existing UserSessionStorageWithScopes concern (and arguably UserSessionStorage) and find another way to opt in to this feature without introducing another concern into the mix. Maybe we could look to see if the user's session model has the new column name you introduced or we could look at adding a flag that we add to the config for this gem and if it is enabled we add this new functionality.

What happens when Shopify revokes a user(online) token?

One of the more frustrating use cases for user tokens is handling the use case of Shopify revoking a user access token and handling that messaging in a graceful manner. I'm curious if you have thought through that use case here at all. The bad stuff for that use case isn't terrible, since that user was likely deleted or had other security concerns and we'd just need to handle that use case from the time of Shopify's revoke and the end of the 24 hour window the user's token is valid for.

Ruby App Template v Generators

From what we can gather, generators aren't used as frequent as our app template is (arguably we should delete the generators because it is very confusion to have two means to achieving the same goal). I see how adding this as a generator for existing apps makes a lot of sense.

We'll need to include this column in our app template as well. What's super gross is we don't actually have a migration for the users table and I believe we are trusting that devs run rails db:setup to load the schema as the starting point 🤦 Obviously we have room for improvement there so don't feel like you need to solve all the problems at once here, but when we merge this in we'll need that column at a minimum in the schema file of our template.

[gbzodek]

Thanks for the review @nelsonwittwer ❤️

My answers:

Introducing a new controller concern

Agree! I changed to add the functionality to UserSessionStorageWithScopes instead, with a config flag check_session_expiry_date that will activate the check, and trigger errors if the expiry date is not correctly stored/retrieved.

What happens when Shopify revokes a user(online) token?

Unless I am missing something, I don't think that's really a concern (or at least not more than it is today). If the token is revoked, the expiry date check will pass, but then Shopify will reject with a 401, which will trigger the auth flow as today, and the user will get a new token (or not, depending on why it was revoked I guess). Ideally yes, the app should know that it was revoked and trigger a reauth early, but it seems a bigger change than this one.

Ruby App Template v Generators

We'll need to include this column in our app template as well. What's super gross is we don't actually have a migration for the users table and I believe we are trusting that devs run rails db:setup to load the schema as the starting point 🤦 Obviously we have room for improvement there so don't feel like you need to solve all the problems at once here, but when we merge this in we'll need that column at a minimum in the schema file of our template.

The app template is not using user tokens (user_repository is not defined in the config and the user model is not there), so I am surprised to see the users table there. Shouldn't we delete it from the schema instead?

[nelsonwittwer]

This looks great! Thanks for thinking through the details on this and improving the gem 😎