Skip to content

Commit

Permalink
Detect controller concern clash
Browse files Browse the repository at this point in the history
  • Loading branch information
andyw8 committed Nov 9, 2022
1 parent 4fedb60 commit 9d7e60a
Show file tree
Hide file tree
Showing 7 changed files with 41 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ Unreleased
----------
* Fixes a bug with `EnsureAuthenticatedLinks` causing deep links to not work [#1549](https://github.com/Shopify/shopify_app/pull/1549)
* Ensure online token is properly used when using `current_shopify_session` [#1566](https://github.com/Shopify/shopify_app/pull/1566)
* Detect the use of incompatible controller concerns [#1560](https://github.com/Shopify/shopify_app/pull/1560). Depending on how your app is structured, this may result in an error on startup even though the app was working correctly. However, it is helping exposing an existing flaw that may have caused issues in future.

21.2.0 (Oct 25, 2022)
----------
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/concerns/shopify_app/require_known_shop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ module RequireKnownShop
include ShopifyApp::RedirectForEmbedded

included do
if ancestors.include?(ShopifyApp::LoginProtection)
raise ConfigurationError,
"You are attempting to use both RequireKnownShop and LoginProtection in the same controller. "\
"These are not compatible."
end

before_action :check_shop_domain
before_action :check_shop_known
end
Expand Down
6 changes: 6 additions & 0 deletions lib/shopify_app/controller_concerns/login_protection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ module LoginProtection
include ShopifyApp::SanitizedParams

included do
if ancestors.include?(ShopifyApp::RequireKnownShop)
raise ConfigurationError,
"You are attempting to use both RequireKnownShop and LoginProtection in the same controller. "\
"These are not compatible."
end

after_action :set_test_cookie
rescue_from ShopifyAPI::Errors::HttpResponseError, with: :handle_http_error
end
Expand Down
16 changes: 16 additions & 0 deletions test/controllers/concerns/require_known_shop_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,20 @@ def index

assert_response :ok
end

test "detects incompatible controller concerns" do
assert_raises ShopifyApp::ConfigurationError do
Class.new(ApplicationController) do
include ShopifyApp::RequireKnownShop
include ShopifyApp::LoginProtection
end
end

assert_raises ShopifyApp::ConfigurationError do
Class.new(ApplicationController) do
include ShopifyApp::RequireKnownShop
include ShopifyApp::Authenticated # since this indirectly includes LoginProtection
end
end
end
end
1 change: 0 additions & 1 deletion test/dummy/app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
# frozen_string_literal: true

class ApplicationController < ActionController::Base
include ShopifyApp::LoginProtection
end
3 changes: 3 additions & 0 deletions test/dummy/app/controllers/home_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# frozen_string_literal: true

class HomeController < ApplicationController
include ShopifyApp::EmbeddedApp
include ShopifyApp::RequireKnownShop

def index
"index"
end
Expand Down
9 changes: 9 additions & 0 deletions test/shopify_app/controller_concerns/login_protection_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,15 @@ class LoginProtectionControllerTest < ActionController::TestCase
end
end

test "detects incompatible controller concerns" do
assert_raises ShopifyApp::ConfigurationError do
Class.new(ApplicationController) do
include ShopifyApp::LoginProtection
include ShopifyApp::RequireKnownShop
end
end
end

private

def assert_fullpage_redirected(shop_domain, _response)
Expand Down

0 comments on commit 9d7e60a

Please sign in to comment.