Add a state param to create_permission_url #467
Conversation
| @@ -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) | |||
There was a problem hiding this comment.
Moving to named parameters seemed better long term but makes for sort of a wonky interface right now.
There was a problem hiding this comment.
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" })
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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)}"
endThere was a problem hiding this comment.
Do I need to make changes to the readme or were you planning on handling that?
There was a problem hiding this comment.
Changes to the README where necessary would be super appreciated as part of this PR
lib/shopify_api/session.rb
Outdated
| 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) | ||
| params = {client_id: api_key, scope: scope.join(',')} |
There was a problem hiding this comment.
Seems like we're only using ruby 2+ now so we can switch off the old style hashes right?
drewish
force-pushed
the
oauth-state
branch
from
5c0cf5b to
8865119
Compare
Fixes Shopify#466 Includes some style fixes.
drewish
force-pushed
the
oauth-state
branch
from
8865119 to
0ce3b80
Compare
drewish
changed the title
|
@drewish just ran into the lack of this feature - and not using omniauth in my codebase if I don't need to looks like you need to sign the CLA? since this is so old, I'm happy to open a new pull request inspired by it if you've moved on - lmk |
|
Yeah not using this gem any more so I’ll close it and let someone else give it a clean shot. |
|
@drewish awesome thanks! I'll pick the idea up |
Fixes #466