-
Notifications
You must be signed in to change notification settings - Fork 685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle invalid session token #1823
Changes from all commits
d3a0aa1
ed222c9
ce9e370
231b1a7
3bcaff0
a233974
b65aac4
f804df3
d925d90
9712272
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
<!DOCTYPE html> | ||
<html lang="en"> | ||
<head> | ||
<meta charset="utf-8" /> | ||
<title><%= ShopifyApp.configuration.application_name %></title> | ||
<%= yield :head %> | ||
<script | ||
data-api-key="<%= ShopifyApp.configuration.api_key %>" | ||
src="https://cdn.shopify.com/shopifycloud/app-bridge.js"> | ||
</script> | ||
<%= csrf_meta_tags %> | ||
</head> | ||
|
||
<body> | ||
<%= yield %> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,17 +5,23 @@ module TokenExchange | |
extend ActiveSupport::Concern | ||
include ShopifyApp::AdminAPI::WithTokenRefetch | ||
|
||
INVALID_SHOPIFY_ID_TOKEN_ERRORS = [ | ||
ShopifyAPI::Errors::CookieNotFoundError, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would we ever have a cookie present for token exchange? If it's only meant to run for embedded apps there should never be any cookies involved, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea cookies are never involved.. but the way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm that's fair. I guess we can't change this without it potentially breaking apps out there, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea exactly :( |
||
ShopifyAPI::Errors::InvalidJwtTokenError, | ||
].freeze | ||
|
||
def activate_shopify_session(&block) | ||
retrieve_session_from_token_exchange if current_shopify_session.blank? || should_exchange_expired_token? | ||
|
||
begin | ||
ShopifyApp::Logger.debug("Activating Shopify session") | ||
ShopifyAPI::Context.activate_session(current_shopify_session) | ||
with_token_refetch(current_shopify_session, shopify_id_token, &block) | ||
ensure | ||
ShopifyApp::Logger.debug("Deactivating session") | ||
ShopifyAPI::Context.deactivate_session | ||
end | ||
ShopifyApp::Logger.debug("Activating Shopify session") | ||
ShopifyAPI::Context.activate_session(current_shopify_session) | ||
with_token_refetch(current_shopify_session, shopify_id_token, &block) | ||
rescue *INVALID_SHOPIFY_ID_TOKEN_ERRORS => e | ||
ShopifyApp::Logger.debug("Responding to invalid Shopify ID token: #{e.message}") | ||
respond_to_invalid_shopify_id_token unless performed? | ||
ensure | ||
ShopifyApp::Logger.debug("Deactivating session") | ||
ShopifyAPI::Context.deactivate_session | ||
end | ||
|
||
def should_exchange_expired_token? | ||
|
@@ -47,9 +53,6 @@ def current_shopify_domain | |
def retrieve_session_from_token_exchange | ||
@current_shopify_session = nil | ||
ShopifyApp::Auth::TokenExchange.perform(shopify_id_token) | ||
# TODO: Rescue JWT validation errors when bounce page is ready | ||
# rescue ShopifyAPI::Errors::InvalidJwtTokenError | ||
# respond_to_invalid_shopify_id_token | ||
end | ||
|
||
def shopify_id_token | ||
|
@@ -61,23 +64,28 @@ def id_token_header | |
end | ||
|
||
def respond_to_invalid_shopify_id_token | ||
# TODO: Implement this method to handle invalid session tokens | ||
return redirect_to_bounce_page if request.headers["HTTP_AUTHORIZATION"].blank? | ||
|
||
ShopifyApp::Logger.debug("Responding to invalid Shopify ID token with unauthorized response") | ||
response.set_header("X-Shopify-Retry-Invalid-Session-Request", 1) | ||
unauthorized_response = { message: :unauthorized } | ||
render(json: { errors: [unauthorized_response] }, status: :unauthorized) | ||
end | ||
|
||
# if request.xhr? | ||
# response.set_header("X-Shopify-Retry-Invalid-Session-Request", 1) | ||
# unauthorized_response = { message: :unauthorized } | ||
# render(json: { errors: [unauthorized_response] }, status: :unauthorized) | ||
# else | ||
# patch_session_token_url = "#{ShopifyAPI::Context.host}/patch_session_token" | ||
# patch_session_token_params = request.query_parameters.except(:id_token) | ||
def redirect_to_bounce_page | ||
ShopifyApp::Logger.debug("Redirecting to bounce page for patching Shopify ID token") | ||
patch_shopify_id_token_url = "#{ShopifyApp.configuration.root_url}/patch_shopify_id_token" | ||
patch_shopify_id_token_params = request.query_parameters.except(:id_token) | ||
|
||
# bounce_url = "#{ShopifyAPI::Context.host}#{request.path}?#{patch_session_token_params.to_query}" | ||
bounce_url = "#{request.path}?#{patch_shopify_id_token_params.to_query}" | ||
|
||
# # App Bridge will trigger a fetch to the URL in shopify-reload, with a new session token in headers | ||
# patch_session_token_params["shopify-reload"] = bounce_url | ||
# App Bridge will trigger a fetch to the URL in shopify-reload, with a new session token in headers | ||
patch_shopify_id_token_params["shopify-reload"] = bounce_url | ||
|
||
# redirect_to("#{patch_session_token_url}?#{patch_session_token_params.to_query}", allow_other_host: true) | ||
# end | ||
redirect_to( | ||
"#{patch_shopify_id_token_url}?#{patch_shopify_id_token_params.to_query}", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wish there was a way to use the Rails URL helper method |
||
allow_other_host: true, | ||
) | ||
end | ||
|
||
def online_token_configured? | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
require "test_helper" | ||
require "action_controller" | ||
require "action_controller/base" | ||
require "json" | ||
|
||
class ApiClass | ||
def self.perform; end | ||
|
@@ -17,10 +18,19 @@ def index | |
render(plain: "OK") | ||
end | ||
|
||
def reloaded_path | ||
render(plain: "OK") | ||
end | ||
|
||
def make_api_call | ||
ApiClass.perform | ||
render(plain: "OK") | ||
end | ||
|
||
def ensure_render | ||
ensure | ||
render(plain: "OK") | ||
end | ||
end | ||
|
||
class MockPostAuthenticateTasks | ||
|
@@ -178,13 +188,147 @@ class TokenExchangeControllerTest < ActionController::TestCase | |
end | ||
end | ||
|
||
[ | ||
ShopifyAPI::Errors::InvalidJwtTokenError, | ||
ShopifyAPI::Errors::CookieNotFoundError, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we shouldn't respond to invalid cookies with a bounce page now that we'll be using the method that exclusively checks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea once I use the new utility method, we won't be handling |
||
].each do |invalid_shopify_id_token_error| | ||
test "Redirects to bounce page if Shopify ID token is invalid with #{invalid_shopify_id_token_error}" do | ||
ShopifyApp.configuration.root_url = "/my-root" | ||
ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).raises(invalid_shopify_id_token_error) | ||
request.headers["HTTP_AUTHORIZATION"] = nil | ||
|
||
params = { shop: @shop, my_param: "for-keeps", id_token: "dont-include-this-id-token" } | ||
reload_url = CGI.escape("/reloaded_path?my_param=for-keeps&shop=#{@shop}") | ||
expected_redirect_url = "/my-root/patch_shopify_id_token"\ | ||
"?my_param=for-keeps&shop=#{@shop}"\ | ||
"&shopify-reload=#{reload_url}" | ||
|
||
with_application_test_routes do | ||
get :reloaded_path, params: params | ||
assert_redirected_to expected_redirect_url | ||
end | ||
end | ||
|
||
test "Responds with unauthorized if Shopify Id token is invalid with #{invalid_shopify_id_token_error} and authorization header exists" do | ||
ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).raises(invalid_shopify_id_token_error) | ||
request.headers["HTTP_AUTHORIZATION"] = @id_token_in_header | ||
expected_response = { errors: [{ message: :unauthorized }] } | ||
|
||
with_application_test_routes do | ||
get :make_api_call, params: { shop: @shop } | ||
|
||
assert_response :unauthorized | ||
assert_equal expected_response.to_json, response.body | ||
assert_equal 1, response.headers["X-Shopify-Retry-Invalid-Session-Request"] | ||
end | ||
end | ||
|
||
test "Redirects to bounce page from Token Exchange if Shopify ID token is invalid with #{invalid_shopify_id_token_error}" do | ||
ShopifyApp.configuration.root_url = "/my-root" | ||
ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(nil, @offline_session_id) | ||
ShopifyApp::Auth::TokenExchange.expects(:perform).raises(invalid_shopify_id_token_error) | ||
request.headers["HTTP_AUTHORIZATION"] = nil | ||
|
||
params = { shop: @shop, my_param: "for-keeps", id_token: "dont-include-this-id-token" } | ||
reload_url = CGI.escape("/reloaded_path?my_param=for-keeps&shop=#{@shop}") | ||
expected_redirect_url = "/my-root/patch_shopify_id_token"\ | ||
"?my_param=for-keeps&shop=#{@shop}"\ | ||
"&shopify-reload=#{reload_url}" | ||
|
||
with_application_test_routes do | ||
get :reloaded_path, params: params | ||
assert_redirected_to expected_redirect_url | ||
end | ||
end | ||
|
||
test "Responds with unauthorized from Token Exchange if Shopify Id token is invalid with #{invalid_shopify_id_token_error} and authorization header exists" do | ||
ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(nil, @offline_session_id) | ||
ShopifyApp::Auth::TokenExchange.expects(:perform).raises(invalid_shopify_id_token_error) | ||
|
||
expected_response = { errors: [{ message: :unauthorized }] } | ||
|
||
with_application_test_routes do | ||
get :make_api_call, params: { shop: @shop } | ||
|
||
assert_response :unauthorized | ||
assert_equal expected_response.to_json, response.body | ||
assert_equal 1, response.headers["X-Shopify-Retry-Invalid-Session-Request"] | ||
end | ||
end | ||
|
||
test "Redirects to bounce page from with_token_refetch if Shopify ID token is invalid with #{invalid_shopify_id_token_error}" do | ||
ShopifyApp.configuration.root_url = "/my-root" | ||
ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(@offline_session_id) | ||
ShopifyApp::Auth::TokenExchange.stubs(:perform) | ||
request.headers["HTTP_AUTHORIZATION"] = nil | ||
|
||
@controller.expects(:with_token_refetch).raises(invalid_shopify_id_token_error) | ||
|
||
params = { shop: @shop, my_param: "for-keeps", id_token: "dont-include-this-id-token" } | ||
reload_url = CGI.escape("/reloaded_path?my_param=for-keeps&shop=#{@shop}") | ||
expected_redirect_url = "/my-root/patch_shopify_id_token"\ | ||
"?my_param=for-keeps&shop=#{@shop}"\ | ||
"&shopify-reload=#{reload_url}" | ||
|
||
with_application_test_routes do | ||
get :reloaded_path, params: params | ||
assert_redirected_to expected_redirect_url | ||
end | ||
end | ||
|
||
test "Responds with unauthorized from with_token_refetch if Shopify Id token is invalid with #{invalid_shopify_id_token_error} and authorization header exists" do | ||
ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(@offline_session_id) | ||
ShopifyApp::Auth::TokenExchange.stubs(:perform) | ||
expected_response = { errors: [{ message: :unauthorized }] } | ||
|
||
@controller.expects(:with_token_refetch).raises(invalid_shopify_id_token_error) | ||
|
||
with_application_test_routes do | ||
get :make_api_call, params: { shop: @shop } | ||
|
||
assert_response :unauthorized | ||
assert_equal expected_response.to_json, response.body | ||
assert_equal 1, response.headers["X-Shopify-Retry-Invalid-Session-Request"] | ||
end | ||
end | ||
|
||
test "Does not redirect to bounce page if redirect/response has been performed already - #{invalid_shopify_id_token_error}" do | ||
ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(@offline_session_id) | ||
ShopifyApp::Auth::TokenExchange.stubs(:perform) | ||
request.headers["HTTP_AUTHORIZATION"] = nil | ||
|
||
@controller.expects(:with_token_refetch).raises(invalid_shopify_id_token_error) | ||
@controller.stubs(:performed?).returns(true) | ||
|
||
with_application_test_routes do | ||
get :ensure_render, params: { shop: @shop } | ||
assert_response :ok | ||
end | ||
end | ||
|
||
test "Does not respond 401 if redirect/response has been performed already - #{invalid_shopify_id_token_error}" do | ||
ShopifyAPI::Utils::SessionUtils.stubs(:current_session_id).returns(@offline_session_id) | ||
ShopifyApp::Auth::TokenExchange.stubs(:perform) | ||
|
||
@controller.expects(:with_token_refetch).raises(invalid_shopify_id_token_error) | ||
@controller.stubs(:performed?).returns(true) | ||
|
||
with_application_test_routes do | ||
get :ensure_render, params: { shop: @shop } | ||
assert_response :ok | ||
end | ||
end | ||
end | ||
|
||
private | ||
|
||
def with_application_test_routes | ||
with_routing do |set| | ||
set.draw do | ||
get "/" => "token_exchange#index" | ||
get "/make_api_call" => "token_exchange#make_api_call" | ||
get "/reloaded_path" => "token_exchange#reloaded_path" | ||
get "/ensure_render" => "token_exchange#ensure_render" | ||
end | ||
yield | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the docs mention
EnsureInstalled
provides theinstalled_shop_session
method andEnsureHasSession
providescurrent_shopify_session
. I thinkTokenExchange#activate_shopify_session
providescurrent_shopify_session
, right? Should it be different forEnsureInstalled
? Or maybe provide both methods?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yess, so
EnsureInstalled
concern itself implements the installed_shop_session method, so it'll still be able to be used. Having TokenExchange enabled, they'll just get both 'current_shopify_sessionand
installed_shop_session` !