kmrshntr / omniauth-slack

omniauth-slack
MIT License
95 stars 154 forks source link

Get scope list from X-OAuth-Scopes header #41

Open ginjo opened 8 years ago

ginjo commented 8 years ago

According to Slack docs, the full set of authorized scopes for a token is returned with every api request in the X-OAuth-Scopes header. I can't find that information in the auth_hash returned from omniauth-slack. Is there a way to access that information - and info from other slack-specific headers - in the omniuath-slack oauth_hash, or maybe in the env? Would it make sense to include all slack-specific headers in the auth_hash? I do remember seeing a list of scopes somewhere, once, so maybe I'm looking in the wrong place.

ginjo commented 8 years ago

Following up with my initial post, I'm expanding the focus of my initial concern to include more of the raw oauth2 response information returned from Slack.

Currently omniauth-slack populates the auth_hash with a somewhat arbitrary set of data, based on the original authorization, but not containing the body of the original authorization. I find this overly-abstracted, and I would rather see the raw_info section of the omniauth auth_hash contain the full body (or the full response object) from the oauth2 gem. Decorating the omniauth auth_hash with data from subsequent api requests is ok, but the actual authorization response data should always be available in the auth_hash - especially since this contains the definitive set of authorized scopes for the token being requested.

Regarding additional api requests to decorate the auth_hash, it might be better to decide which requests to make based on actual authorized token scopes. For example, I might request identity.basic yet get a token with a more fully featured set of scopes (users:read,team:read,etc...). In that case, I'd like to see more information than what is returned by the users.identity api call.

To sum up my thoughts:

Auth_hash structure should be based on all existing scopes, not just on the recently requested scopes.

Raw_info should contain the response from the initial oauth.access api request, including all (or most) of the nitty-gritty from oauth2 gem. At the very least, raw_info should contain all of the token data returned from the oauth.access request, including the list of authorized scopes.

Sorry if this ruffles feathers across the various gems involved here. No disrespect meant - I appreciate all of the hard work the community has put into these gems. If it turns out my needs are atypical and do not represent the larger community, I'm ok with that. If I come up with something worthy in my forking adventures, I'll gist or PR the project.

ginjo commented 8 years ago

Ok, I think I have it worked out. I cloned the project and addressed the issues I was having. In the process, I hopefully fixed (and didn't break) some other issues as well. I added some documentation to the Slack class that explains some of the slack/oauth2/omniauth pitfalls. The project is here.

Highlights of the mods in this branch:

julienma commented 8 years ago

@ginjo does your fork works with the Sign-in flow?

I noticed it's based on current version of this project, which includes #32, which introduced some issues ith the Sign-in flow. These issues were fixed (at least for me and some others, see discussion on https://github.com/kmrshntr/omniauth-slack/pull/34) in #34, which is a PR waiting to be merged.

ginjo commented 8 years ago

@julienma, yes this fork should work with the sign-in flow and the add-to-slack flow.

I was originally using #32, and then I switched to #34. #34 was working well for me, and I like the adapter-pattern that it introduced. What I wanted, however, was something that made less of a divide between Slack's "flows". So this fork doesn't think so much in terms of flow, but rather, it tries to get each important piece of data any way it can, based on the token's available scopes. I tried to balance efficiency (make as few api requests as possible) with thoroughness (get as much info as possible), while also trying to retain expected behavior and schema. I also really needed the authorized scopes in the credentials section of the auth_hash.

One thing I keep coming back to is why am I trying to decide what data goes into the info-hash (other than the basics returned by Slack's oauth.access method)? I'd like to explore adding an option to let the user of omniauth-slack decide for themselves what API calls are made after the initial authorization and how the resulting data is mapped to the info-hash. Maybe some kind of plugin? A config object? Additional omniauth provider options? Runtime options? Or maybe that's what #34 was going for... and I'm just trying to reinvent the wheel now. :smile:

ginjo commented 7 years ago

In a recent conversation with Slack support, I learned that directing the oauth2 authorization request to the Slack team subdomain is a perfectly valid practice and is the only way to narrow the focus of what team we are trying to authorize against (given only a team subdomain name and not the team ID).

https://myotherteam.slack.com/oauth/authorize...

This solves a big problem I was having: how to allow the user of my Slack app to sign in to a different team, while at the same time keeping them within the oauth flow of my Slack app.

I perused the omniauth, oauth2, and omniauth-slack gems for a way to include this subdomain at runtime, but it doesn't look like that is supported as of yet. So I added basic support for setting this subdomain to my fork of omniauth-slack, and now you can do this.

https://my.app.com/auth/slack?subdomain=myotherteam

I'm not sure if I added this feature in the best way possible, but I kept the mods simple. The feature works well for me and hasn't caused any ugly side effects. For transparency's sake, here is the single method I added to omniauth-slack (slack.rb) to make this work.

def client
  super.tap do |c|
    session['omniauth.subdomain'] = request.params['subdomain'] if request.params['subdomain']
    c.site = "https://#{session['omniauth.subdomain']}.slack.com" if session['omniauth.subdomain']
  end
end