GitHub authentication

[nickbruun]

We rely on GitHub authentication for most developer-related services, so I thought it would be nice if Freight also support it (great job, btw!) This pull request in its current form adds rudimentary support for GitHub authentication, limiting the user to either a specific team ID or organization ID, depending on the granularity desired.

To select the authentication backend used, I've added a configuration variable AUTH_BACKEND which defaults to google for Google authentication, and can also be github for Github authentication. I've also added some initialization time checks for the validity of the configuration of AUTH_BACKEND and Github authentication-specific configuration variables. For now, I've defaulted to RuntimeError, but I don't know if you have a preferred exception in this case.

I've also subclassed OAuth2WebServerFlow for the case of GitHub, as fetching both the user and team after the code/access token exchange has completed, is necessary. Lastly, I've added a check after the exchange to make sure that the user is limited to whatever team or organization is specified.

A few notes/questions:

• GitHub's only properly stable user identifier is the user ID. Right now, I've continued to use e-mail addresses as the identification, but in an ideal world, one would rely on the user ID. What are your thoughts on this? One possibility I can think of is to add a table with many-to-one relationship to User for a hypothetical number of authentication backends.
• Given the different checks, I'd propose a basic class-based abstraction of an authentication backend. What are your thoughts on this? Mostly, these backends would abstract the OAuth flow but also perform access checks (domain check in the case of Google, or team/organization check in the case of GitHub) but would be a good structural component in my opinion. I'd gladly add that as part of this PR if you agree.

I purposefully haven't done any documentation updates yet, so consider this PR a "for your consideration" at the moment. I'll do proper test coverage and documentation once the details are settled, of course :)

Let me know what you think!

[dcramer]

I think in general this is good. I would say we should kill the if ... == 'google' bit and push that into some class that controls this.

While Sentry isn't a great example (as its much more complex), that's what we did there:

https://github.com/getsentry/sentry/blob/master/src/sentry/auth/providers/oauth2.py#L130

[nickbruun]

Perfect. I'll update my PR :)

[marksteve]

👍 This would be awesome

[nickbruun]

@dcramer do you want it generalized to that extent, or are you okay with provider-specific class implementations of say a OAuth2Provider class, that provides similar endpoints to the oauth2client but also performs access checks?

[mitsuhiko]

That's fine, it just needs to be done in a way where you don't need to patch the entire code to get a custom provider in if needed. A dictionary with provider names -> setup functions would already be a good start.

[nickbruun]

Sure. Just wanted to check what level of abstraction you were going for ;)

[nickbruun]

Alright, abstracted the providers. Let me know, what you think. If we're good, I'll add a bunch of tests to make sure everything works as expected.

[dcramer]

I would make this something like OAuth2Provider.validate().

[nickbruun]

Makes sense

[dcramer]

My comments are fairly nitpicky, and tbh its probably more work than its worth to make this super flexible.

I think in an ideal (read: ideal) world, provider would be something like a Flask app that gets instantiated and attached to the application object (there's some common mechanism of binding state, its a bit gross though). This would happen at runtime (via configure_auth or something), and then call validate on the provider to ensure settings are correct. This would also pass in the app.config values.

[nickbruun]

That's an interesting idea. So you propose having separate Flask apps for each provider, inheriting from the same base?

[dcramer]

Sorry not separate apps, but extensions (which get bound to the app).

If you imagine something like app.state['authprovider'] = current provider. The simple version is just:

class Ext(object):
  def init_app(self, app):
    self.app = app
    # ...

ext = Ext()
ext.init_app(app)

[nickbruun]

Just so I'm clear: are you proposing the actual provider be an extension that the auth views then access by app.state['authprovider']? Also, I can't seem to find anything on the state attribute - am I missing something?

[dcramer]

@nickbruun It's a pattern I've seen in various Flask extensions. I don't quite recall which ones off hand, and there might be better ways. That said, you could approach it from my example above, where you have the instance of the extension (the auth provider here), and you simple call init_app like others when the app is created. At that point you could validate configuration thats specific to the given provider, and then in places where you would check "is the provider google" you would simply call out the provider assuming a given API interface.

[nickbruun]

Alright, I've tried to follow the pattern you set out to the best of my understanding. How does this compare to what you were thinking?

[dcramer]

this is good, and if we want to extend it in the future we can just have AUTH_BACKEND support a full class path or similar

[dcramer]

In all files can you ensure the following exists at the top:

from __future__ import absolute_import

[dcramer]

In general this looks good. Theres a few pep8 things I can clean up (i.e. import orders) but its nbd.

When I have time I'll pull this down and give it a quick QA pass, but the general approach looks good to me.

[nickbruun]

I've updated the imports in the auth module. Let me know how you get along with testing it out :)

[thoas]

We should advance on this subject, we need to check why unittest doesn't pass first.

@nickbruun are you still available to finish this PR?