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

Use api access from shopify api #105

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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 lib/omniauth/shopify.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
require 'omniauth/shopify/version'
require 'omniauth/strategies/shopify'
require 'shopify_api'
Copy link
Contributor

@paulomarg paulomarg Jan 28, 2021

Choose a reason for hiding this comment

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

This is sort of creating a circular dependency: shopify_app => omniauth-shopify-oauth2 => shopify_api <= shopify_app. Is that a problem? I'm guessing no but thought I'd check.

Copy link
Contributor

@andyw8 andyw8 Jan 28, 2021

Choose a reason for hiding this comment

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

Yeah, I'm not sure of the consequences, but this strikes me as something to be cautious about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tanema you brought this up before in this closed PR. Do you think adding shopify_api as a dependency should mean a major release upgrade of omniauth-shopify-oauth2?

19 changes: 7 additions & 12 deletions lib/omniauth/strategies/shopify.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ class Shopify < OmniAuth::Strategies::OAuth2
# Available scopes: content themes products customers orders script_tags shipping
# read_* or write_*
DEFAULT_SCOPE = 'read_products'
SCOPE_DELIMITER = ','
MINUTE = 60
CODE_EXPIRES_AFTER = 10 * MINUTE

Expand Down Expand Up @@ -74,16 +73,11 @@ def valid_signature?
end

def valid_scope?(token)
params = options.authorize_params.merge(options_for("authorize"))
return false unless token && params[:scope] && token['scope']
expected_scope = normalized_scopes(params[:scope]).sort
(expected_scope == token['scope'].split(SCOPE_DELIMITER).sort)
end

def normalized_scopes(scopes)
scope_list = scopes.to_s.split(SCOPE_DELIMITER).map(&:strip).reject(&:empty?).uniq
ignore_scopes = scope_list.map { |scope| scope =~ /\A(unauthenticated_)?write_(.*)\z/ && "#{$1}read_#{$2}" }.compact
scope_list - ignore_scopes
config = options.authorize_params.merge(options_for("authorize"))
return false unless token && config[:scope] && token['scope']
expected_api_access = ::ShopifyAPI::ApiAccess.new(config[:scope])
actual_api_access = ::ShopifyAPI::ApiAccess.new(token['scope'])
actual_api_access == expected_api_access
end

def self.encoded_params_for_signature(params)
Expand Down Expand Up @@ -146,7 +140,8 @@ def build_access_token

def authorize_params
super.tap do |params|
params[:scope] = normalized_scopes(params[:scope] || DEFAULT_SCOPE).join(SCOPE_DELIMITER)
scopes = params[:scope] || DEFAULT_SCOPE
params[:scope] = ::ShopifyAPI::ApiAccess.new(scopes).to_s
params[:grant_options] = ['per-user'] if options[:per_user_permissions]
end
end
Expand Down
1 change: 1 addition & 0 deletions omniauth-shopify-oauth2.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Gem::Specification.new do |s|

s.add_runtime_dependency 'omniauth-oauth2', ['~> 1.5', '< 1.7.1']
s.add_runtime_dependency 'activesupport'
s.add_runtime_dependency('shopify_api', '~> 9.3')

s.add_development_dependency 'minitest', '~> 5.6'
s.add_development_dependency 'rspec', '~> 3.9.0'
Expand Down