Skip to content

Add callback_url def#1

Merged
luvtechno merged 1 commit intomasterfrom
takose/bugfix
Jun 6, 2018
Merged

Add callback_url def#1
luvtechno merged 1 commit intomasterfrom
takose/bugfix

Conversation

@takose
Copy link
Copy Markdown
Contributor

@takose takose commented Jun 6, 2018

Why

https://github.com/wantedly/corporate-site-rails/pull/524
callback_url methodはwantedly strategyに実装してなかったけど本当は必要だった,oauth2 のstrategyに実装されていたので動いていたが, omniauth/omniauth-oauth2#28 で消され,corporate siteでエラーになってしまっていた

What

callback_url methodを追加

end

def callback_url
full_host + script_name + callback_path
Copy link
Copy Markdown
Contributor Author

@takose takose Jun 6, 2018

Choose a reason for hiding this comment

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

@takose takose requested a review from luvtechno June 6, 2018 07:35
@takose takose self-assigned this Jun 6, 2018
@luvtechno
Copy link
Copy Markdown
Contributor

該当の変更がはいった omniauth-oauth2 の PR: omniauth/omniauth-oauth2#70

なおしたかった問題はこれ omniauth/omniauth-oauth2#28

omniauth gem の master には OmniAuth::Strategy#callback_url が存在する。

https://github.com/omniauth/omniauth/blob/8179ba796aae82f857f63b50ae848a3fbe369b4d/lib/omniauth/strategy.rb#L438-L440

これをみると、自分たちのStrategyでoverrideせずに、OmniAuth::Strategy で定義されているやつをそのまま使ったほうがよいような気がしたのですが、 OmniAuth::Strategy#callback_url が使われないのはなんでなんでしょう? omniauth gem が古いのかな?

@takose
Copy link
Copy Markdown
Contributor Author

takose commented Jun 6, 2018

ほんとだ..
omniauth-wantedlyの依存してるv1.0でも存在するのでおかしいですね
https://github.com/omniauth/omniauth/blob/v1.0.0/lib/omniauth/strategy.rb#L378
もう少しみてみます

@takose
Copy link
Copy Markdown
Contributor Author

takose commented Jun 6, 2018

omniauth/omniauth-oauth2#100callback_urlがまた実装されてますが,1年半くらいmergeされてないです.
このPRの解決策が正しいわけではないけど,omniauth-oauth2で今後のアップデートがあまり期待できないため,これでmergeしてしまったらよいかなと思いました

@luvtechno luvtechno merged commit 435e27d into master Jun 6, 2018
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.

2 participants