Skip to content
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

Move Session Storage to shopify_app #1055

Merged
merged 19 commits into from
Nov 18, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
Note: For changes to the API, see https://shopify.dev/changelog?filter=api

## Unreleased
- [#1055](https://github.com/Shopify/shopify-api-ruby/pull/1055) Makes session_storage optional. Configuring the API with session_storage is now deprecated. Session persistence is handled by the [shopify_app gem](https://github.com/Shopify/shopify_app) if using Rails.

## Version 12.2.1
- [#1045](https://github.com/Shopify/shopify-api-ruby/pull/1045) Fixes bug with host/host_name requirement.
Expand Down
5 changes: 1 addition & 4 deletions lib/shopify_api/auth/oauth.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,7 @@ def validate_auth_callback(cookies:, auth_query:)
)
end

unless Context.session_storage.store_session(session)
raise Errors::SessionStorageError,
"Session could not be saved. Please check your session storage implementation."
end
Context.session_storage&.store_session(session)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably an edge case, but should we be retaining the previous behaviour of raising if store_session returns nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new normal is for Context.session_storage to be nil as it is deprecated. It's my hope that this will be nil :)


{ session: session, cookie: cookie }
end
Expand Down
13 changes: 9 additions & 4 deletions lib/shopify_api/context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ class Context
@api_secret_key = T.let("", String)
@api_version = T.let(LATEST_SUPPORTED_ADMIN_VERSION, String)
@scope = T.let(Auth::AuthScopes.new, Auth::AuthScopes)
@session_storage = T.let(ShopifyAPI::Auth::FileSessionStorage.new, ShopifyAPI::Auth::SessionStorage)
@is_private = T.let(false, T::Boolean)
@private_shop = T.let(nil, T.nilable(String))
@is_embedded = T.let(true, T::Boolean)
@logger = T.let(Logger.new($stdout), Logger)
@notified_missing_resources_folder = T.let({}, T::Hash[String, T::Boolean])
@active_session = T.let(Concurrent::ThreadLocalVar.new { nil }, Concurrent::ThreadLocalVar)
@session_storage = T.let(nil, T.nilable(ShopifyAPI::Auth::SessionStorage))
@user_agent_prefix = T.let(nil, T.nilable(String))
@old_api_secret_key = T.let(nil, T.nilable(String))

Expand All @@ -32,8 +32,8 @@ class << self
scope: T.any(T::Array[String], String),
is_private: T::Boolean,
is_embedded: T::Boolean,
session_storage: ShopifyAPI::Auth::SessionStorage,
logger: Logger,
session_storage: T.nilable(ShopifyAPI::Auth::SessionStorage),
host_name: T.nilable(String),
host: T.nilable(String),
private_shop: T.nilable(String),
Expand All @@ -48,8 +48,8 @@ def setup(
scope:,
is_private:,
is_embedded:,
session_storage:,
logger: Logger.new($stdout),
session_storage: nil,
host_name: nil,
host: ENV["HOST"] || "https://#{host_name}",
private_shop: nil,
Expand All @@ -69,6 +69,11 @@ def setup(
@scope = Auth::AuthScopes.new(scope)
@is_embedded = is_embedded
@session_storage = session_storage
if @session_storage
::ShopifyAPI::Context.logger.warn("SessionStorage has been deprecated. " \
nelsonwittwer marked this conversation as resolved.
Show resolved Hide resolved
"The ShopifyAPI will no longer have responsibility for session persistence. " \
"Upgrading to `shopify_app` 21.3 will allow you to remove session_storage.")
nelsonwittwer marked this conversation as resolved.
Show resolved Hide resolved
end
@logger = logger
@private_shop = private_shop
@user_agent_prefix = user_agent_prefix
Expand Down Expand Up @@ -111,7 +116,7 @@ def load_rest_resources(api_version:)
sig { returns(Auth::AuthScopes) }
attr_reader :scope

sig { returns(ShopifyAPI::Auth::SessionStorage) }
sig { returns(T.nilable(ShopifyAPI::Auth::SessionStorage)) }
attr_reader :session_storage

sig { returns(Logger) }
Expand Down
53 changes: 34 additions & 19 deletions lib/shopify_api/utils/session_utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,13 @@ class << self
).returns(T.nilable(Auth::Session))
end
def load_current_session(auth_header: nil, cookies: nil, is_online: false)
validate_session_storage_for_deprecated_utils
return load_private_session if Context.private?

session_id = current_session_id(auth_header, cookies, is_online)
return nil unless session_id

Context.session_storage.load_session(session_id)
T.must(Context.session_storage).load_session(session_id)
end

sig do
Expand All @@ -33,10 +34,12 @@ def load_current_session(auth_header: nil, cookies: nil, is_online: false)
).returns(T::Boolean)
end
def delete_current_session(auth_header: nil, cookies: nil, is_online: false)
validate_session_storage_for_deprecated_utils

session_id = current_session_id(auth_header, cookies, is_online)
return false unless session_id

Context.session_storage.delete_session(session_id)
T.must(Context.session_storage).delete_session(session_id)
end

sig do
Expand All @@ -46,8 +49,10 @@ def delete_current_session(auth_header: nil, cookies: nil, is_online: false)
).returns(T.nilable(Auth::Session))
end
def load_offline_session(shop:, include_expired: false)
validate_session_storage_for_deprecated_utils

session_id = offline_session_id(shop)
session = Context.session_storage.load_session(session_id)
session = T.must(Context.session_storage).load_session(session_id)
return nil if session && !include_expired && session.expires && T.must(session.expires) < Time.now

session
Expand All @@ -59,23 +64,10 @@ def load_offline_session(shop:, include_expired: false)
).returns(T::Boolean)
end
def delete_offline_session(shop:)
session_id = offline_session_id(shop)
Context.session_storage.delete_session(session_id)
end

private

sig { returns(Auth::Session) }
def load_private_session
unless Context.private_shop
raise Errors::SessionNotFoundError, "Could not load private shop, Context.private_shop is nil."
end
validate_session_storage_for_deprecated_utils

Auth::Session.new(
shop: T.must(Context.private_shop),
access_token: Context.api_secret_key,
scope: Context.scope.to_a,
)
session_id = offline_session_id(shop)
T.must(Context.session_storage).delete_session(session_id)
end

sig do
Expand Down Expand Up @@ -129,6 +121,29 @@ def offline_session_id(shop)
def cookie_session_id(cookies)
cookies[Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME]
end

private

sig { void }
def validate_session_storage_for_deprecated_utils
unless Context.session_storage
raise ShopifyAPI::Errors::SessionStorageError,
"session_storage is required in ShopifyAPI::Context when using deprecated Session utility methods."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"session_storage is required in ShopifyAPI::Context when using deprecated Session utility methods."
"session_storage is required in ShopifyAPI::Context when using deprecated Session utility methods."

If these session utility methods are deprecated, what's the recommended replacement? Presumably something in shopify_app? Should we point them towards that or does that create too much of a link between shopify-api-ruby and shopify_app?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this particular error will be super hard to reach. This error was put in place mainly because the sorbet one wasn't pretty. I think the upgrade logs we have in the Context.setup call should help give actionable steps on upgrade path.

end
end

sig { returns(Auth::Session) }
def load_private_session
unless Context.private_shop
raise Errors::SessionNotFoundError, "Could not load private shop, Context.private_shop is nil."
end

Auth::Session.new(
shop: T.must(Context.private_shop),
access_token: Context.api_secret_key,
scope: Context.scope.to_a,
)
end
end
end
end
Expand Down
14 changes: 0 additions & 14 deletions test/auth/oauth_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -285,20 +285,6 @@ def test_validate_auth_callback_bad_http_response
end
end

def test_validate_auth_callback_save_session_fails
stub_request(:post, "https://#{@shop}/admin/oauth/access_token")
.with(body: @access_token_request)
.to_return(body: @offline_token_response.to_json, headers: { content_type: "application/json" })

modify_context(is_embedded: true, session_storage: TestHelpers::FakeSessionStorage.new(
sessions: { @session_cookie => ShopifyAPI::Auth::Session.new(shop: @shop, id: @session_cookie,
state: @callback_state) }, error_on_save: true
))
assert_raises(ShopifyAPI::Errors::SessionStorageError) do
ShopifyAPI::Auth::Oauth.validate_auth_callback(cookies: @cookies, auth_query: @auth_query)
end
end

private

def verify_oauth_begin(auth_route:, cookie:, is_online:, scope: ShopifyAPI::Context.scope)
Expand Down
45 changes: 38 additions & 7 deletions test/utils/session_utils_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,20 @@ def setup
@cookie_id = "1234-this-is-a-cookie-session-id"
@cookies = { ShopifyAPI::Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME => @cookie_id }
@online_session = ShopifyAPI::Auth::Session.new(id: @cookie_id, shop: @shop, is_online: true)
ShopifyAPI::Context.session_storage.store_session(@online_session)
T.must(ShopifyAPI::Context.session_storage).store_session(@online_session)

@online_embedded_session_id = "#{@shop}_#{@jwt_payload[:sub]}"
@online_embedded_session = ShopifyAPI::Auth::Session.new(id: @online_embedded_session_id, shop: @shop,
is_online: true)
ShopifyAPI::Context.session_storage.store_session(@online_embedded_session)
T.must(ShopifyAPI::Context.session_storage).store_session(@online_embedded_session)

@offline_cookie_id = "offline_#{@shop}"
@offline_cookies = { ShopifyAPI::Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME => @offline_cookie_id }
@offline_session = ShopifyAPI::Auth::Session.new(id: @offline_cookie_id, shop: @shop, is_online: false)
ShopifyAPI::Context.session_storage.store_session(@offline_session)
T.must(ShopifyAPI::Context.session_storage).store_session(@offline_session)

@offline_embedded_session = ShopifyAPI::Auth::Session.new(id: "offline_#{@shop}", shop: @shop, is_online: false)
ShopifyAPI::Context.session_storage.store_session(@offline_embedded_session)
T.must(ShopifyAPI::Context.session_storage).store_session(@offline_embedded_session)
end

def test_gets_the_current_session_from_cookies_for_non_embedded_apps
Expand All @@ -57,6 +57,37 @@ def test_returns_nil_if_there_is_no_session_for_non_embedded_apps
))
end

def test_raise_error_when_session_storage_missing_when_using_deprecated_session_utils
ShopifyAPI::Context.stubs(:session_storage).returns(nil)

assert_raises(ShopifyAPI::Errors::SessionStorageError) do
ShopifyAPI::Utils::SessionUtils.load_current_session(
auth_header: @jwt_header,
is_online: true,
)
end

assert_raises(ShopifyAPI::Errors::SessionStorageError) do
ShopifyAPI::Utils::SessionUtils.delete_current_session(
auth_header: @jwt_header,
cookies: @cookies,
is_online: true,
)
end

assert_raises(ShopifyAPI::Errors::SessionStorageError) do
ShopifyAPI::Utils::SessionUtils.load_offline_session(
shop: @shop,
)
end

assert_raises(ShopifyAPI::Errors::SessionStorageError) do
ShopifyAPI::Utils::SessionUtils.delete_offline_session(
shop: @shop,
)
end
end

def test_gets_the_current_session_from_auth_header_for_embedded_apps
modify_context(is_embedded: true)
add_session(is_online: true)
Expand Down Expand Up @@ -148,15 +179,15 @@ def test_loads_offline_session_by_shop
def test_returns_nil_for_expired_offline_session
offline_session = ShopifyAPI::Auth::Session.new(id: "offline_#{@shop}", shop: @shop, is_online: false,
expires: Time.now - 60)
ShopifyAPI::Context.session_storage.store_session(offline_session)
T.must(ShopifyAPI::Context.session_storage).store_session(offline_session)
loaded_session = ShopifyAPI::Utils::SessionUtils.load_offline_session(shop: @shop)
assert_nil(loaded_session)
end

def test_loads_expired_offline_session_if_requested
offline_session = ShopifyAPI::Auth::Session.new(id: "offline_#{@shop}", shop: @shop, is_online: false,
expires: Time.now - 60)
ShopifyAPI::Context.session_storage.store_session(offline_session)
T.must(ShopifyAPI::Context.session_storage).store_session(offline_session)
loaded_session = ShopifyAPI::Utils::SessionUtils.load_offline_session(shop: @shop, include_expired: true)
assert_equal(offline_session, loaded_session)
end
Expand Down Expand Up @@ -235,7 +266,7 @@ def test_load_private_session_no_private_shop

def add_session(is_online:)
another_session = ShopifyAPI::Auth::Session.new(shop: @shop, is_online: is_online)
ShopifyAPI::Context.session_storage.store_session(another_session)
T.must(ShopifyAPI::Context.session_storage).store_session(another_session)
end

def create_jwt_header(api_secret_key)
Expand Down