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

omniauth user info is nil #43

Open
LucasCioffi opened this issue Oct 25, 2016 · 16 comments
Open

omniauth user info is nil #43

LucasCioffi opened this issue Oct 25, 2016 · 16 comments

Comments

@LucasCioffi
Copy link

LucasCioffi commented Oct 25, 2016

Thanks for this gem!

I have two problems that might be related:

  1. When logging into Slack, the user sees [app name] would like access to your Slack team but I wouldn't expect that message from identity.basic.

  2. When I use Slack to authenticate and sign into my app, all user data is nil except for the token. I have identity.basic enabled as an app permission scope in Slack's admin interface the app. Is this an error or is this normal until the app is approved in Slack's app directory? Screenshot:

screen shot 2016-10-24 at 11 33 36 pm

@ginjo
Copy link

ginjo commented Oct 26, 2016

Hi @LucasCioffi, you might check out (#42) where I've been rambling on about similar issues.

I think your 1) might be standard Slack behavior. The first time a token is issued, you should get the Slack authorization chooser page, regardless of what scope you request. After that, for basic identity.xxxx scopes, oauth should pass right thru Slack without stopping.

Number 2) is a tricky one, as there are different user-info api calls available to different scopes. I've tried to cover them all and balance efficiency with thoroughness in my fork of this gem. I'm curious to know if it helps others - or if it makes an awful mess 😜. I haven't put it up as a PR yet, so gem 'omniauth-slack', :git => 'https://github.com/ginjo/omniauth-slack.git', :branch => 'auth-hash-fixes' in your Gemfile.

@LucasCioffi
Copy link
Author

@ginjo That worked great. The auth hash had the right data in it. Thank you!

Hopefully the gem maintainers can merge in this functionality down the road.

@julienma
Copy link

I didn't try your fork, but have had some related issues. I also hope the maintainer (@kmrshntr?) will be able to sort through all the different PRs and update the gem; but this has been more than 4 months without any activity here, so might be time for an active fork to publish a new gem and start a new path from here?

@ginjo
Copy link

ginjo commented Oct 26, 2016

Glad it helped @LucasCioffi . @julienma , If you get a chance to try out my fork, let us know if it helps or breaks. I think it covers all the main issues, but my only feedback is from my own projects. Would be great to hear from @kmrshntr on what his thoughts are. I don't envy him having to sort thru all the PRs 😅 .

EDIT: If you are trying out the ginjo fork of this gem (auth-hash-fixes branch, see above for gemfile code), make sure to bundle update omniauth-slack, as I have addressed some bugs in the auth_hash and in the oauth2 client authorization url (bugs that I introduced, not existing in any other fork of this gem).

@manuraj17
Copy link

manuraj17 commented Oct 31, 2016

@ginjo Hey, thanks for your branch. I used it and got this issue solved. Although now I am having an issue where I have this error

E, [2016-10-31T16:21:15.009985 #13] ERROR -- omniauth: (slack) Authentication failure! invalid_credentials: OAuth2::Error, code_already_used: 
web_1  | {"ok":false,"error":"code_already_used"}

I am not sure if it is because I don't understand the slack oauth flow or something else, would you have any idea on how to tackle this?

@ginjo
Copy link

ginjo commented Nov 1, 2016

Hey @manuraj17,
Based on the "code_already_used" error, it looks like omniauth (or something) is requesting the token a second time. Is this happening only with the ginjo fork or also with the official gem? I'll look at my fork for clues. Meanwhile, do you have a stack trace you could post? I think omniauth has a verbose logging feature somewhere - sorry, I forget how. Also what does your /auth/slack/callback route (or action if Rails) look like?

@manuraj17
Copy link

@ginjo Yes, the token was being requested another time. I could not get the master branch working due to the issue mentioned in this issue thread, user info being nil, so I was able to get it working only using your fork - thanks for that again.
Good news - that it started working later today and unfortunately I was not able to figure what was causing the issue, although I did not change anything in the code. Maybe I am overlooking something. So, sorry for the trouble. I will let you know if I hit it again.
Thanks again for your work.

@ginjo
Copy link

ginjo commented Nov 1, 2016

@manuraj17 no worries, glad you got it working! Let me know if see the issue happen again.

@chaserx
Copy link

chaserx commented Nov 4, 2016

oh! @ginjo thanks for your fork - helped me out.

@macteo
Copy link

macteo commented Jan 11, 2017

Thanks @ginjo !

@ghost
Copy link

ghost commented Feb 21, 2017

🙏 @ginjo

@SirRawlins
Copy link

This is definitely something it'd be nice to permanently address.

As @ginjo's fork is currently behind master I've shied away from using it, but for the moment I'm having to include users:read scope just to get back the profile data on the user who has authorized the app - and this scope in turn displays a yellow warning box during the auth process which isn't the best of experiences.

@ginjo + @kmrshntr do we have an plans to bring the smarter profile retrieval into master?

@ginjo
Copy link

ginjo commented Mar 29, 2017

Hi all, @SirRawlins, My fork (ginjo) attempted to address all of the current scoping and data-retrieval issues at the time I created it. Unfortunately, I haven't brought it up to date with @kmrshntr 's master branch, yet.

I'm not sure how well my fork would merge with master, as the varied contributors (myself included) have created differing solutions to the main issues. That said, it's not a huge amount of code, so it should be possible to work out the divergent solutions. I can take a look at it and see what's involved. If my fork has value for others, I'd be happy to bring it in line with master and offer up a proper PR.

If anyone is curious about what I did, I left a rather thick documentation block inline with the code. It explains some of the sticky issues using omniauth with slack and how I approached solving those issues. The main takeaway is that I decouple information retrieval from requested scope. I retrieve as much information as possible at authorization time, regardless of what scope was requested for that authorization cycle. This works, because Slack's authorization tokens are persistent, with scopes being additive on any existing token.

@SirRawlins
Copy link

@ginjo thanks for getting back to me on this. I liked your approach as it had a fairly progressive approach, whereby it'd get as much personal information as possible given the scopes used.

I'd be interested to hear from @kmrshntr about their long-term plans for the project, and what a successful PR to solve the issue would have to look like.

@jucdn
Copy link

jucdn commented Jul 25, 2018

Hi @ginjo
Thanks for the fix.

I think your auth-hash-fixes branch is now deleted, is there other branchs on your fork to solve this one ?

Thank you

@ginjo
Copy link

ginjo commented Jul 25, 2018

Sorry, my bad!

I renamed my auth-hash-fixes branch to features-and-fixes, since the work I've done has expanded beyond the auth-hash object. If you delete & reload (re-bundle) my /ginjo/omniauth-slack fork from github, you should get the correct branch (features-and-fixes is the default branch of the fork). Or you can specify the branch explicitly.

gem 'omniauth-slack', :git => 'https://github.com/ginjo/omniauth-slack.git', :branch => 'features-and-fixes'

I've also opened an issue in the ginjo fork to track some basic notes and documentation on the fork.

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

No branches or pull requests

8 participants