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 8 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
15 changes: 11 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,13 @@ def setup(
@scope = Auth::AuthScopes.new(scope)
@is_embedded = is_embedded
@session_storage = session_storage
if @session_storage
# rubocop:disable Layout/LineLength
::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. " \
"Consider using the `shopify_app` gem which now owns this responsiblity.")
nelsonwittwer marked this conversation as resolved.
Show resolved Hide resolved
# rubocop:enable Layout/LineLength
end
@logger = logger
@private_shop = private_shop
@user_agent_prefix = user_agent_prefix
Expand Down Expand Up @@ -111,7 +118,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
49 changes: 30 additions & 19 deletions lib/shopify_api/utils/session_utils.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# typed: strict
# frozen_string_literal: true

# rubocop:disable Layout/LineLength

module ShopifyAPI
module Utils
class SessionUtils
Expand All @@ -17,12 +19,14 @@ class << self
).returns(T.nilable(Auth::Session))
end
def load_current_session(auth_header: nil, cookies: nil, is_online: false)
::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.load_current_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")
nelsonwittwer marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need these now that we have the check in Context?

Copy link
Contributor

Choose a reason for hiding this comment

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

(also - extra space before 'will')


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 +37,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)
::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_current_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")
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
::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_current_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")
::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_current_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now implements this responsibility.")


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 +52,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)
::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.load_offline_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")
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
::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.load_offline_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")
::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.load_offline_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now implements this responsibility.")


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 +67,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
::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_offline_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")
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
::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_offline_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now owns this responsiblity.")
::ShopifyAPI::Context.logger.warn("ShopifyAPI::Utils::SessionUtils.delete_offline_session has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now implements this responsibility.")


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,7 +124,23 @@ def offline_session_id(shop)
def cookie_session_id(cookies)
cookies[Auth::Oauth::SessionCookie::SESSION_COOKIE_NAME]
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

Auth::Session.new(
shop: T.must(Context.private_shop),
access_token: Context.api_secret_key,
scope: Context.scope.to_a,
)
end
end
end
end
end
# rubocop:enable Layout/LineLength
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
14 changes: 7 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 Down Expand Up @@ -148,15 +148,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 +235,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