singingwolfboy / flask-dance

Doing the OAuth dance with style using Flask, requests, and oauthlib.
https://pypi.python.org/pypi/Flask-Dance/
MIT License
1.01k stars 158 forks source link

Slack v2: granular permissions #295

Open svandegar opened 4 years ago

svandegar commented 4 years ago

Hi!

I'm currently using Flask-Dance for our Slackbot authentication. They recently added new authorisation and token endpoints: Slack documentation

I'd like to help with Flask-Dance update but will need some guidance. For example, I was wondering if the preferred solution would be to create a "Slack_v2" contrib or to add an option in the current contrib to make it switch to the new endpoints.

Cheers

singingwolfboy commented 4 years ago

Great! We're always looking for people interested in helping out. 😄

I think that adding an option to the existing Slack config is probably the best way to go. It should default to the existing (old) endpoints, but allow you to switch to the new one whenever you want. Maybe something like this pseudocode:

def make_slack_blueprint(
    # existing args...
    version="1"    # default to the old version, for back-compat
):
    if version == "1":
        authorization_url = "https://slack.com/oauth/authorize"
        token_url = "https://slack.com/api/oauth.access"
    elif version == "2":
        authorization_url = "insert new url here"
        token_url = "insert new url here"
    else:
        raise ValueError("invalid version")

In the next major version of Flask-Dance, we can change the default value of version, so that people will get the new OAuth flow by default.

Does that make sense, and do you have any questions?

daenney commented 4 years ago

I'm a bit torn about this. From a backwards compatibility argument, I agree that the defaults ought to stay on the old URLs. But I also dislike the fact that our library defaults to having you set up a classic app, which you'll then later have to migrate away from.

How bad would it be to flip the logic, with a v3.1.0 release and an entry in the CHANGELOG.rst that for Classic apps you now have to pass version=1?

daenney commented 4 years ago

Unhelpfully Slack hasn't indicated how long they'll let classic apps be a thing, so there's a bit of a question as to at which point we should switch the defaults.

If we don't switch the defaults, we should really add some additional prose to https://flask-dance.readthedocs.io/en/latest/providers.html#module-flask_dance.contrib.slack about ensuring you set up a new/v2 app.

daenney commented 4 years ago

For reference:

singingwolfboy commented 4 years ago

How bad would it be to flip the logic, with a v3.1.0 release and an entry in the CHANGELOG.rst that for Classic apps you now have to pass version=1?

We can do that -- but that's a v4.0 release, because it's not backwards compatible. Which is OK! There's nothing wrong with incrementing the major version number, and we don't have to wait a certain period of time or have a certain number of 3.X releases to do so.

I think it's better to have a 3.1 release where the default is Slack version 1, so that anyone who cares about doing this upgrade slowly can upgrade Flask-Dance independently of updating their code. But we could follow it up right away with a 4.0 release, where literally the only change is changing the default Slack version.

svandegar commented 4 years ago

Great! We're always looking for people interested in helping out.

I think that adding an option to the existing Slack config is probably the best way to go. It should default to the existing (old) endpoints, but allow you to switch to the new one whenever you want. Maybe something like this pseudocode:

def make_slack_blueprint(
    # existing args...
    version="1"    # default to the old version, for back-compat
):
    if version == "1":
        authorization_url = "https://slack.com/oauth/authorize"
        token_url = "https://slack.com/api/oauth.access"
    elif version == "2":
        authorization_url = "insert new url here"
        token_url = "insert new url here"
    else:
        raise ValueError("invalid version")

In the next major version of Flask-Dance, we can change the default value of version, so that people will get the new OAuth flow by default.

Does that make sense, and do you have any questions?

Totally make sense. I'll do that and make a PR. (will be my first PR, so please be indulgent :) )

svandegar commented 4 years ago

How bad would it be to flip the logic, with a v3.1.0 release and an entry in the CHANGELOG.rst that for Classic apps you now have to pass version=1?

We can do that -- but that's a v4.0 release, because it's not backwards compatible. Which is OK! There's nothing wrong with incrementing the major version number, and we don't have to wait a certain period of time or have a certain number of 3.X releases to do so.

I think it's better to have a 3.1 release where the default is Slack version 1, so that anyone who cares about doing this upgrade slowly can upgrade Flask-Dance independently of updating their code. But we could follow it up right away with a 4.0 release, where literally the only change is changing the default Slack version.

This makes sense to me. Slack v2 is still very new.