search

msa7 / multi_auth

Standardized multi-provider OAuth authentication
MIT License
111 stars 23 forks source link

add check for urls key in google provider. fixes #11 #12

Closed jwoertink closed 5 years ago

jwoertink commented 6 years ago

If the json response has the urls key, it will add it in, otherwise just skip it.

msa7 commented 6 years ago

Do you have time to update specs?

jwoertink commented 6 years ago

Yeah, I can take a look at the specs. Those failing specs are unrelated to this PR though. Do you care if they're lumped in?

jwoertink commented 6 years ago

hmm.. Actually, I'm a little confused on this one.

Missing hash key: HTTP::Headers::Key(@name="Host") (KeyError)
from spec/providers/twitter_spec.cr:31:7 in '->'

I added a Host header in to the stub similar to the google spec has, but that didn't do anything. It seems the error might be more with the OAuth::Consumer. I went down the rabbit hole starting here to here

I'm not sure if this is a crystal issue or not.

rwojsznis commented 6 years ago

Hi @jwoertink, if I recall it was related do webmock.cr and Crystal 0.26.0 🤔 can you maybe try bumping version? 🙏

jwoertink commented 6 years ago

@emq It's currently pulling master branch of webmock.cr, so it shows latest commit. Maybe there's a bug in webmock then?

rwojsznis commented 6 years ago

Strange, I could swear I had exact same issue somewhere in my toy-app related to webmock, but maybe I'm just randomly passing blame here 😅 Will try to cross check and report back 🙊

jwoertink commented 6 years ago

You may not be wrong. The only strange thing though pointing to OAuth is that all the other providers use OAuth::Client, but the failing twitter uses OAuth::Consumer.

rwojsznis commented 6 years ago

So I revisited my old toy project and the exact same issue (also related to stubbing calls to twitter's API) is still there it seems ;)

It seems very similar issue was fixed in webmock.cr via https://github.com/manastech/webmock.cr/pull/25

But that line:

https://github.com/manastech/webmock.cr/blob/v0.10.0/src/webmock/core_ext.cr#L12

Should happen before run_before_request_callbacks as signature that requires host header is added as before callback to our http client (godda@#$* callbacks).

I'm guessing fix should happen within webmock.cr, but I'm not like 100% sure - that thing broke around Crystal 0.26.x afaik 😕

/cc @jwoertink

jwoertink commented 6 years ago

Yeah, as I was digging around, that was my guess too. I'm gonna check it out once 0.27 is on homebrew and see if it's still doing it.

msa7 commented 5 years ago

sorry for delay. Specs should be fixed in master. Could you please rebase

jwoertink commented 5 years ago

updated