Merge pull request #1197 from Shopify/revised-scope-strategies

rezaansyed · web-flow · commit daebcc6c62d9 · 2021-03-04T17:39:33.000-05:00
Use access scopes strategies to handle changes to scopes requested by app
diff --git a/README.md b/README.md
@@ -103,6 +103,7 @@ You can find documentation on gem usage, concepts, mixins, installation, and mor
   * [Generators](/docs/shopify_app/generators.md)
   * [ScriptTags](/docs/shopify_app/script-tags.md)
   * [Session repository](/docs/shopify_app/session-repository.md)
+  * [Handling changes in access scopes](/docs/shopify_app/handling-access-scopes-changes.md)
   * [Testing](/docs/shopify_app/testing.md)
   * [Webhooks](/docs/shopify_app/webhooks.md)
 
diff --git a/app/controllers/concerns/shopify_app/shop_access_scopes_verification.rb b/app/controllers/concerns/shopify_app/shop_access_scopes_verification.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module ShopifyApp
+  module ShopAccessScopesVerification
+    extend ActiveSupport::Concern
+
+    included do
+      before_action :login_on_scope_changes
+    end
+
+    protected
+
+    def login_on_scope_changes
+      redirect_to(shop_login) if scopes_mismatch?
+    end
+
+    private
+
+    def scopes_mismatch?
+      ShopifyApp.configuration.shop_access_scopes_strategy.update_access_scopes?(current_shopify_domain)
+    end
+
+    def current_shopify_domain
+      return if params[:shop].blank?
+      ShopifyApp::Utils.sanitize_shop_domain(params[:shop])
+    end
+
+    def shop_login
+      ShopifyApp::Utils.shop_login_url(shop: params[:shop], return_to: request.fullpath)
+    end
+  end
+end
diff --git a/app/controllers/shopify_app/callback_controller.rb b/app/controllers/shopify_app/callback_controller.rb
@@ -76,10 +76,20 @@ def start_user_token_flow?
       if jwt_request?
         false
       else
-        ShopifyApp::SessionRepository.user_storage.present? && user_session.blank?
+        return false unless ShopifyApp::SessionRepository.user_storage.present?
+        update_user_access_scopes?
       end
     end
 
+    def update_user_access_scopes?
+      return true if user_session.blank?
+      user_access_scopes_strategy.update_access_scopes?(user_id: session[:user_id])
+    end
+
+    def user_access_scopes_strategy
+      ShopifyApp.configuration.user_access_scopes_strategy
+    end
+
     def jwt_request?
       jwt_shopify_domain || jwt_shopify_user_id
     end
diff --git a/docs/shopify_app/handling-access-scopes-changes.md b/docs/shopify_app/handling-access-scopes-changes.md
@@ -0,0 +1,8 @@
+# Handling changes in access scopes
+The Shopify App gem provides handling changes to scopes for both shop/offline and user/online tokens. To enable your app to login via OAuth on scope changes, you can set the following configuration flag:
+```ruby
+ShopifyApp.configuration.reauth_on_access_scope_changes = true
+```
+
+## ShopAccessScopesVerification
+The `ShopifyApp::ShopAccessScopesVerification` concern helps merchants grant new access scopes requested by the app. The concern compares the current access scopes granted by the shop and compares them with the scopes requested by the app. If there is a mismatch in configuration, the merchant is redirected to login via OAuth and grant the net new scopes.
diff --git a/docs/shopify_app/session-repository.md b/docs/shopify_app/session-repository.md
@@ -78,13 +78,11 @@ end
 provider :shopify,
   ...
   setup: lambda { |env|
-    ...
-    # Add this line
-    strategy.options[:per_user_permissions] = strategy.session[:user_tokens]
-    ...
+    configuration = ShopifyApp::OmniauthConfiguration.new(env['omniauth.strategy'], Rack::Request.new(env))
+    configuration.build_options
   }
 
 # In the `shopify_app.rb` initializer:
 config.shop_session_repository = {YOUR_SHOP_MODEL_CLASS}
 config.user_session_repository = {YOUR_USER_MODEL_CLASS}
-```
+```
diff --git a/lib/generators/shopify_app/home_controller/templates/home_controller.rb b/lib/generators/shopify_app/home_controller/templates/home_controller.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class HomeController < AuthenticatedController
+  include ShopifyApp::ShopAccessScopesVerification
+
   def index
     @products = ShopifyAPI::Product.find(:all, params: { limit: 10 })
     @webhooks = ShopifyAPI::Webhook.find(:all)
diff --git a/lib/generators/shopify_app/home_controller/templates/unauthenticated_home_controller.rb b/lib/generators/shopify_app/home_controller/templates/unauthenticated_home_controller.rb
@@ -3,6 +3,7 @@
 class HomeController < ApplicationController
   include ShopifyApp::EmbeddedApp
   include ShopifyApp::RequireKnownShop
+  include ShopifyApp::ShopAccessScopesVerification
 
   def index
     @shop_origin = current_shopify_domain
diff --git a/lib/generators/shopify_app/install/templates/shopify_app.rb.tt b/lib/generators/shopify_app/install/templates/shopify_app.rb.tt
@@ -7,6 +7,9 @@ ShopifyApp.configure do |config|
   config.after_authenticate_job = false
   config.api_version = "<%= @api_version %>"
   config.shop_session_repository = 'Shop'
+
+  config.reauth_on_access_scope_changes = true
+
   config.allow_jwt_authentication = <%= !with_cookie_authentication? %>
   config.allow_cookie_authentication = <%= with_cookie_authentication? %>
 
diff --git a/lib/generators/shopify_app/install/templates/shopify_provider.rb.tt b/lib/generators/shopify_app/install/templates/shopify_provider.rb.tt
@@ -3,16 +3,6 @@
       ShopifyApp.configuration.secret,
       scope: ShopifyApp.configuration.scope,
       setup: lambda { |env|
-        strategy = env['omniauth.strategy']
-
-        shopify_auth_params = strategy.session['shopify.omniauth_params']&.with_indifferent_access
-        shop = if shopify_auth_params.present?
-          "https://#{shopify_auth_params[:shop]}"
-        else
-          ''
-        end
-
-        strategy.options[:client_options][:site] = shop
-        strategy.options[:old_client_secret] = ShopifyApp.configuration.old_secret
-        strategy.options[:per_user_permissions] = strategy.session[:user_tokens]
+        configuration = ShopifyApp::OmniAuthConfiguration.new(env['omniauth.strategy'], Rack::Request.new(env))
+        configuration.build_options
       }
diff --git a/lib/shopify_app.rb b/lib/shopify_app.rb
@@ -61,6 +61,11 @@ def self.use_webpacker?
   require 'shopify_app/session/user_session_storage'
   require 'shopify_app/session/user_session_storage_with_scopes'
 
+  # access scopes strategies
+  require 'shopify_app/access_scopes/shop_strategy'
+  require 'shopify_app/access_scopes/user_strategy'
+  require 'shopify_app/access_scopes/noop_strategy'
+
   # omniauth_configuration
   require 'shopify_app/omniauth/omniauth_configuration'
 end
diff --git a/lib/shopify_app/access_scopes/noop_strategy.rb b/lib/shopify_app/access_scopes/noop_strategy.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+module ShopifyApp
+  module AccessScopes
+    class NoopStrategy
+      class << self
+        def update_access_scopes?(*_args)
+          false
+        end
+      end
+    end
+  end
+end
diff --git a/lib/shopify_app/access_scopes/shop_strategy.rb b/lib/shopify_app/access_scopes/shop_strategy.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module ShopifyApp
+  module AccessScopes
+    class ShopStrategy
+      class << self
+        def update_access_scopes?(shop_domain)
+          shop_access_scopes = shop_access_scopes(shop_domain)
+          configuration_access_scopes != shop_access_scopes
+        end
+
+        private
+
+        def shop_access_scopes(shop_domain)
+          ShopifyApp::SessionRepository.retrieve_shop_session_by_shopify_domain(shop_domain)&.access_scopes
+        end
+
+        def configuration_access_scopes
+          ShopifyAPI::ApiAccess.new(ShopifyApp.configuration.shop_access_scopes)
+        end
+      end
+    end
+  end
+end
diff --git a/lib/shopify_app/access_scopes/user_strategy.rb b/lib/shopify_app/access_scopes/user_strategy.rb
@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+module ShopifyApp
+  module AccessScopes
+    class UserStrategy
+      class InvalidInput < StandardError; end
+
+      class << self
+        def update_access_scopes?(user_id: nil, shopify_user_id: nil)
+          return update_access_scopes_for_user_id?(user_id) if user_id
+          return update_access_scopes_for_shopify_user_id?(shopify_user_id) if shopify_user_id
+          raise(InvalidInput, '#update_access_scopes? requires user_id or shopify_user_id parameter inputs')
+        end
+
+        private
+
+        def update_access_scopes_for_user_id?(user_id)
+          user_access_scopes = user_access_scopes_by_user_id(user_id)
+          configuration_access_scopes != user_access_scopes
+        end
+
+        def update_access_scopes_for_shopify_user_id?(shopify_user_id)
+          user_access_scopes = user_access_scopes_by_shopify_user_id(shopify_user_id)
+          configuration_access_scopes != user_access_scopes
+        end
+
+        def user_access_scopes_by_user_id(user_id)
+          ShopifyApp::SessionRepository.retrieve_user_session(user_id)&.access_scopes
+        end
+
+        def user_access_scopes_by_shopify_user_id(shopify_user_id)
+          ShopifyApp::SessionRepository.retrieve_user_session_by_shopify_user_id(shopify_user_id)&.access_scopes
+        end
+
+        def configuration_access_scopes
+          ShopifyAPI::ApiAccess.new(ShopifyApp.configuration.user_access_scopes)
+        end
+      end
+    end
+  end
+end
diff --git a/lib/shopify_app/configuration.rb b/lib/shopify_app/configuration.rb
@@ -18,6 +18,8 @@ class Configuration
     attr_accessor :after_authenticate_job
     attr_accessor :api_version
 
+    attr_accessor :reauth_on_access_scope_changes
+
     # customise urls
     attr_accessor :root_url
     attr_writer :login_url
@@ -72,6 +74,16 @@ def shop_session_repository
       ShopifyApp::SessionRepository.shop_storage
     end
 
+    def shop_access_scopes_strategy
+      return ShopifyApp::AccessScopes::NoopStrategy unless reauth_on_access_scope_changes
+      ShopifyApp::AccessScopes::ShopStrategy
+    end
+
+    def user_access_scopes_strategy
+      return ShopifyApp::AccessScopes::NoopStrategy unless reauth_on_access_scope_changes
+      ShopifyApp::AccessScopes::UserStrategy
+    end
+
     def has_webhooks?
       webhooks.present?
     end
diff --git a/lib/shopify_app/omniauth/omniauth_configuration.rb b/lib/shopify_app/omniauth/omniauth_configuration.rb
@@ -50,7 +50,7 @@ def default_request_online_tokens?
     end
 
     def update_shop_scopes?
-      false
+      ShopifyApp.configuration.shop_access_scopes_strategy.update_access_scopes?(shop_domain)
     end
 
     def shop_domain
diff --git a/lib/shopify_app/utils.rb b/lib/shopify_app/utils.rb
@@ -20,5 +20,17 @@ def self.fetch_known_api_versions
     rescue ActiveResource::ConnectionError
       logger.error("[ShopifyAPI::ApiVersion] Unable to fetch api_versions from Shopify")
     end
+
+    def self.shop_login_url(shop:, return_to:)
+      return ShopifyApp.configuration.login_url unless shop
+      url = URI(ShopifyApp.configuration.login_url)
+
+      url.query = URI.encode_www_form(
+        shop: shop,
+        return_to: return_to,
+      )
+
+      url.to_s
+    end
   end
 end
diff --git a/test/controllers/callback_controller_test.rb b/test/controllers/callback_controller_test.rb
@@ -20,6 +20,8 @@ class CallbackControllerTest < ActionController::TestCase
       @routes = ShopifyApp::Engine.routes
       ShopifyApp.configuration = nil
       ShopifyApp.configuration.embedded_app = true
+      ShopifyApp.configuration.reauth_on_access_scope_changes = true
+      mock_user_scopes_match_strategy
 
       I18n.locale = :en
 
@@ -374,6 +376,16 @@ class CallbackControllerTest < ActionController::TestCase
       assert_response :ok
     end
 
+    test "#callback redirects to login for user token flow if user session access scopes mismatch by user_id" do
+      mock_user_scopes_mismatch_strategy
+      mock_shopify_user_omniauth
+      _session = mock_user_session
+
+      get :callback
+
+      assert_redirected_to ShopifyApp.configuration.login_url
+    end
+
     private
 
     def mock_shopify_jwt
diff --git a/test/controllers/concerns/shop_access_scopes_verification_test.rb b/test/controllers/concerns/shop_access_scopes_verification_test.rb
@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+require 'test_helper'
+
+class ScopesVerificationController < ActionController::Base
+  include ShopifyApp::ShopAccessScopesVerification
+
+  def index
+    render(plain: "OK")
+  end
+end
+
+class ShopAccessScopesVerificationControllertest < ActionController::TestCase
+  tests ScopesVerificationController
+
+  setup do
+    ShopifyApp.configuration.reauth_on_access_scope_changes = true
+    @shopify_domain = 'test-shop.myshopify.com'
+
+    Rails.application.routes.draw do
+      get '/', to: 'scopes_verification#index'
+    end
+  end
+
+  test '#login_on_scope_changes does nothing if shop scopes match' do
+    mock_shop_scopes_match_strategy
+
+    get :index, params: { shop: @shopify_domain }
+
+    assert_response :ok
+  end
+
+  test '#login_on_scope_changes redirects to login when scopes do not match' do
+    mock_shop_scopes_mismatch_strategy
+
+    get :index, params: { shop: @shopify_domain }
+
+    assert_redirected_to expected_redirect_url
+  end
+
+  private
+
+  def expected_redirect_url
+    login_url = URI(ShopifyApp.configuration.login_url)
+    login_url.query = URI.encode_www_form(
+      shop: @shopify_domain,
+      return_to: request.fullpath,
+    )
+    login_url.to_s
+  end
+end
diff --git a/test/generators/home_controller_generator_test.rb b/test/generators/home_controller_generator_test.rb
diff --git a/test/shopify_app/access_scopes/noop_strategy_test.rb b/test/shopify_app/access_scopes/noop_strategy_test.rb
diff --git a/test/shopify_app/access_scopes/shop_strategy_test.rb b/test/shopify_app/access_scopes/shop_strategy_test.rb
diff --git a/test/shopify_app/access_scopes/user_strategy_test.rb b/test/shopify_app/access_scopes/user_strategy_test.rb
diff --git a/test/shopify_app/configuration_test.rb b/test/shopify_app/configuration_test.rb
diff --git a/test/shopify_app/omniauth/omniauth_configuration_test.rb b/test/shopify_app/omniauth/omniauth_configuration_test.rb
diff --git a/test/support/access_scopes_strategy_test_helpers.rb b/test/support/access_scopes_strategy_test_helpers.rb
diff --git a/test/test_helper.rb b/test/test_helper.rb
