#2812 Implement Google OAuth authentication
Closed by pingou. Opened by bkabrda.
bkabrda/pagure add-google-auth  into  master

Download 2812.patch

This PR implements authentication using Google OAuth via flask-oauth library. The approach that I took was tricking the Pagure authentication machinery into thinking that the user who logged through Google was a FAS user (by creating a fake flask.g.fas_user), hence the changes are contained in only couple of methods in pagure/__init__.py.
If you don't fancy adding the new imports to the pagure/__init__.py file, I guess I could create some new utility functions and add them somewhere to pagure/lib.

Should we use a different error message since this is now used for more than just OpenID?

You really want to also add the "openid" scope.

Is there any reason you hardcoded this to use Google, rather than just allowing the urls and name to be configurable?

I'm not sure I'd default to google :)

Following Patrick's questions, could we make this generic enough that it works with other oauth2? (say ipsilon or... github? :D)

Make sure that this url is sanitized, or we become an open redirect.

Note: we have pagure.is_safe_url which may be able to help

Yeah, I didn't notice this. Will do.

Yeah, you're right. I'll do that.

Well, the reason was that I just needed to have Google auth, but I guess that I can do this configurable and thus allow any kind of oauth IdP.

Good point, will do.

Eh, sorry for the default, that shouldn't have got there ;)
I'll try to have a look at making this more general and thus allowing any kind of oauth2 provider.

2 new commits added

  • Verify oauth callback return url
  • Turn the Google OAuth authentication into general OAuth2

Ok, so I think I addressed all review comments. The slight issue with being able to work with arbitrary oauth2 provider is that they use different keys in returned data. Because of that, I took the approach of making the admin provide lambda expression in settings that can extract username/email from responses of IdP server. I have to admit I don't particularly like this solution, but I couldn't think of a better solution - I'm open to suggestions here.
Also, the error messages are now very general, as keys in returned data also diverge for error responses.
I tested this with Google and Github as IdP and both work fine - example settings are in the default config.

I'm thinking we may want to have a dedicated page for setting up pagure with oauth and include these examples in there rather than in the default_config file here.

Rather than the lambda I wonder if we shouldn't use some kind of plugin approach, maybe a little like either the plugins (git hooks) or the git auth module (using entry points)

Yeah, the plugins make a very good sense I think. I'll work on that.

1 new commit added

  • Use plugin system for different oauth IdPs

Ok, here's another round that uses a plugin approach for oauth providers. Please let me know If this is ok, I'll add docstrings + documentation after that.

Since this is not doing anything with groups, should it be one indentation level lower?

Since we merged #2820 which brings it OIDC support, I'm going to close this one without merging it.

If someone asks for OAuth2 support we can always bring it back to life :)

Thanks @bkabrda for working on this!

Pull-Request has been closed by pingou

Metadata