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

Add redirect_uri to access_token params #3

Merged
merged 1 commit into from
Jul 24, 2017

Conversation

gdong42
Copy link
Contributor

@gdong42 gdong42 commented Jul 22, 2017

This change is to overcome redirect_uri missing issue caused by this commit in omniauth-oauth2: omniauth/omniauth-oauth2@2615267#diff-1894759d724182a93ca97be91b43a7bc

callback_url is added back to build up access_token params for weibo strategy.

@erickguan
Copy link
Owner

Thanks for investigation. The strategy file is imported (copy & paste) from https://github.com/beenhero/omniauth-weibo-oauth2/blob/master/lib/omniauth/strategies/weibo.rb#L89-L97. The difference lies in these lines. Did you checkout the new version?

@gdong42
Copy link
Contributor Author

gdong42 commented Jul 24, 2017

@fantasticfears Yes, I did. It does not work - I think it requires to setup redirect_uri in provider config accourding to https://github.com/beenhero/omniauth-weibo-oauth2/blob/master/README.md#configuration

provider :weibo, ENV['WEIBO_KEY'], ENV['WEIBO_SECRET'],
         token_params: {redirect_uri: "http://127.0.0.1:3000/auth/weibo/callback" }

Not sure if above is set in the plugin.

The issue was also described in this issue: beenhero/omniauth-weibo-oauth2#21
This pull request ( beenhero/omniauth-weibo-oauth2#24 ) proposed a fix, but the solution was not working as well ( as tested locally on my site ). So I have to trace up to omniauth-oauth2 project and find the solution.

Thanks,
Gan

@@ -226,5 +226,10 @@ def authorize_params
end
end
end

def callback_url
options[:redirect_uri] || (full_host + script_name + callback_path)
Copy link
Owner

Choose a reason for hiding this comment

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

nitpick, should call super instead of (full_host + script_name + callback_path)

Copy link
Owner

Choose a reason for hiding this comment

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

It also seems omniauth-oauth2 has some problems. Anyway, it's not relevant here. omniauth/omniauth-oauth2#100

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nitpick, should call super instead of (full_host + script_name + callback_path)

not working... just tested with my branch https://github.com/gdong42/discourse-chinese-localization-pack/tree/temp
not sure if I'm using super in the correct way, though - I'm quite new to Ruby
I guess it might be due to the super callback_url was removed in the change that the pull request you posted was trying to add back

Copy link
Owner

Choose a reason for hiding this comment

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

Ok...I don't get their problem. I'm happy to merge this. Thank you!

@erickguan erickguan merged commit 5216aca into erickguan:master Jul 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants