-
Notifications
You must be signed in to change notification settings - Fork 157
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
Signin with slack support #34
base: master
Are you sure you want to change the base?
Conversation
I'm not sure what happened... I was using the code from @Z-productions, and it was working for me, but now that's its merged, master isn't working for me, it's acting differently than it was. What can we do to get this moving? I would love to get Slack sign in working... |
@Petercopter I am not sure what error(s) you are seeing now but feel free to try this fork. It has been working for me (without any changes to code from this commit) in production for a month now. You may need to adjust the signatures from the oauth response. |
This fork worked great for me to get the sign in with slack! |
👍 |
Same here. |
@kandadaboggu I noticed you closed the PR without any comment. Is this a resolved issue? |
@kmrshntr Any thoughts on this PR and cutting a new gem? |
This PR adds Sign in with Slack support via an adapter pattern.
Since Slack responds with a different JSON structure with
identity
scopes andapp
scopes, this PR introduces an adapter for each different scope so it has a common API to respond withuser_info
,raw_info
,team_info
, and theuid
.For example, an app requesting the
identity.basic
scope needs to call the /api/users.identity whereas the non-identity
scopes needs to call the /api/auth.test method. These two methods return different JSON responses with different structures.*This PR will not merge with the current master. This is because another PR that added sign in with Slack support eliminated a lot of the information provided by the strategy. Information such as
user_id
andteam_id
which I believe will break a lot of current installs. I recommend replacing it with this PR since it does not eliminate the fidelity of the current information that is provided by the strategy. *