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.

Sign up for GitHub

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

Add a state param to create_permission_url #467

Closed
wants to merge 1 commit into from
Closed

Add a state param to create_permission_url #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions lib/shopify_api/session.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,10 @@ def initialize(url, token = nil, extra = {})
self.extra = extra
end

def create_permission_url(scope, redirect_uri = nil)
params = {:client_id => api_key, :scope => scope.join(',')}
def create_permission_url(scope, redirect_uri = nil, state: nil)
Copy link
Author

Choose a reason for hiding this comment

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

Moving to named parameters seemed better long term but makes for sort of a wonky interface right now.

Copy link

@richter-alex richter-alex Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What would we think about instead consolidating into an options hash? CC @jamiemtdwyer

def create_permission_url(scope, options = {})
      params = {:client_id => api_key, :scope => scope.join(',')}
      params[:redirect_uri] = options[:redirect_uri] if options[:redirect_uri]
      params[:state] = options[:state] if options[:state]
      "#{site}/oauth/authorize?#{parameterize(params)}"
end

create_permission_url(["read_orders"], { redirect_uri: "baz.myshopify.com", state: "foobar" })

Copy link
Contributor

Choose a reason for hiding this comment

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

Consolidating into an options hash isn't a bad idea. I like it because it's future proof, it allows extending this in the future to support generating online access tokens without changing the public API again. That being said, I think I would keep redirect_uri and actually remove the defaulting to nil, since redirect_uri is required now.

I don't much like the idea of mixing keyword and positional arguments.

Copy link
Author

Choose a reason for hiding this comment

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

@jamiemtdwyer Just so I'm clear before I try to go get all those tests changed around: You're okay with a breaking to the interface that makes the redirect_uri a required parameter? So we'll have a method like:

    def create_permission_url(scope, redirect_uri, state: nil)
      params = { client_id: api_key, scope: scope.join(','), redirect_uri: redirect_uri }
      params[:state] = state if state
      "#{site}/oauth/authorize?#{parameterize(params)}"
    end

Copy link
Author

Choose a reason for hiding this comment

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

Do I need to make changes to the readme or were you planning on handling that?

Copy link

Choose a reason for hiding this comment

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

Changes to the README where necessary would be super appreciated as part of this PR

params = { client_id: api_key, scope: scope.join(',') }
params[:redirect_uri] = redirect_uri if redirect_uri
params[:state] = state if state
"#{site}/oauth/authorize?#{parameterize(params)}"
end

Expand Down
24 changes: 19 additions & 5 deletions test/session_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,35 +74,49 @@ def setup
end

test "create_permission_url returns correct url with single scope no redirect uri" do
ShopifyAPI::Session.setup(:api_key => "My_test_key", :secret => "My test secret")
ShopifyAPI::Session.setup(api_key: "My_test_key", secret: "My test secret")
session = ShopifyAPI::Session.new('http://localhost.myshopify.com')
scope = ["write_products"]
permission_url = session.create_permission_url(scope)
assert_equal "https://localhost.myshopify.com/admin/oauth/authorize?client_id=My_test_key&scope=write_products", permission_url
end

test "create_permission_url returns correct url with single scope and redirect uri" do
ShopifyAPI::Session.setup(:api_key => "My_test_key", :secret => "My test secret")
ShopifyAPI::Session.setup(api_key: "My_test_key", secret: "My test secret")
session = ShopifyAPI::Session.new('http://localhost.myshopify.com')
scope = ["write_products"]
permission_url = session.create_permission_url(scope, "http://my_redirect_uri.com")
assert_equal "https://localhost.myshopify.com/admin/oauth/authorize?client_id=My_test_key&scope=write_products&redirect_uri=http://my_redirect_uri.com", permission_url
end

test "create_permission_url returns correct url with dual scope no redirect uri" do
ShopifyAPI::Session.setup(:api_key => "My_test_key", :secret => "My test secret")
ShopifyAPI::Session.setup(api_key: "My_test_key", secret: "My test secret")
session = ShopifyAPI::Session.new('http://localhost.myshopify.com')
scope = ["write_products","write_customers"]
permission_url = session.create_permission_url(scope)
assert_equal "https://localhost.myshopify.com/admin/oauth/authorize?client_id=My_test_key&scope=write_products,write_customers", permission_url
end

test "create_permission_url returns correct url with no scope no redirect uri" do
ShopifyAPI::Session.setup(:api_key => "My_test_key", :secret => "My test secret")
ShopifyAPI::Session.setup(api_key: "My_test_key", secret: "My test secret")
session = ShopifyAPI::Session.new('http://localhost.myshopify.com')
scope = []
permission_url = session.create_permission_url(scope)
assert_equal "https://localhost.myshopify.com/admin/oauth/authorize?client_id=My_test_key&scope=", permission_url
assert_equal(
"https://localhost.myshopify.com/admin/oauth/authorize?client_id=My_test_key&scope=",
permission_url
)
end

test "create_permission_url returns correct url with state" do
ShopifyAPI::Session.setup(api_key: "My_test_key", secret: "My test secret")
session = ShopifyAPI::Session.new('http://localhost.myshopify.com')
scope = []
permission_url = session.create_permission_url(scope, nil, state: "My nonce")
assert_equal(
"https://localhost.myshopify.com/admin/oauth/authorize?client_id=My_test_key&scope=&state=My%20nonce",
permission_url
)
end

test "raise exception if code invalid in request token" do
Expand Down