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 6 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
2 changes: 1 addition & 1 deletion Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ PATH
openssl
securerandom
sorbet-runtime
zeitwerk (~> 2.5)
zeitwerk (~> 2.5, < 2.6.5)

GEM
remote: https://rubygems.org/
Expand Down
12 changes: 6 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ Once your app can perform OAuth, it can now make authenticated Shopify API calls

### Breaking change notice for version 10.0.0

We've rewritten this library for v10, so that it provides all essential features for a Shopify app without requiring any other packages.
We've rewritten this library for v10, so that it provides all essential features for a Shopify app without depending on the [Active Resource](https://github.com/rails/activeresource) or [graphql-client](https://github.com/github/graphql-client) libraries.

Here are the main features it provides:

Expand All @@ -107,12 +107,12 @@ With this, a lot changed in how apps access the library. Here are the updates yo

Please see below a (non-exhaustive) list of common replacements to guide you in your updates, using the `Order` resource as an example.

| Before | After |
| --- | --- |
| `Order.find(:all, params: {param1: value1})` | `Order.all(param1: value1, session:)` |
| `Order.find(<id>)` | `Order.find(id: <id>, session:)` |
| Before | After |
| --- | --- |
| `Order.find(:all, params: {param1: value1})` | `Order.all(param1: value1, session:)` |
| `Order.find(<id>)` | `Order.find(id: <id>, session:)` |
| `order = Order.new(<id>)`<br/>`order.post(:close)` | `order = Order.new(session:)`<br/>`order.close()` |
| `order = Order.new(<id>)`<br/>`order.delete` | `Order.delete(id: <id>, session:)` |
| `order = Order.new(<id>)`<br/>`order.delete` | `Order.delete(id: <id>, session:)` |

## Breaking changes for older versions

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
# rubocop:disable Layout/LineLength
::ShopifyAPI::Context.logger.warn("SessionStorage 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("SessionStorage 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("SessionStorage has been deprecated. The ShopifyAPI will no longer have responsibility for session persistence. Consider using the `shopify_app` gem which now implements this responsibility.")

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 +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
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
2 changes: 1 addition & 1 deletion shopify_api.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ Gem::Specification.new do |s|
s.add_runtime_dependency("openssl")
s.add_runtime_dependency("securerandom")
s.add_runtime_dependency("sorbet-runtime")
s.add_runtime_dependency("zeitwerk", "~> 2.5")
s.add_runtime_dependency("zeitwerk", "~> 2.5", "< 2.6.5") # https://github.com/Shopify/shopify-api-ruby/issues/1058

s.add_development_dependency("activesupport")
s.add_development_dependency("pry-byebug")
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
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