fedora-infra / anitya

A cross-distribution upstream release monitoring project
https://release-monitoring.org
GNU General Public License v2.0
250 stars 105 forks source link

RFE: Add GitHub as a 3rd party login option #485

Open ncoghlan opened 7 years ago

ncoghlan commented 7 years ago

@shainer pointed out that it would make sense to offer GitHub as an easy-to-select login choice, and that seems reasonable to me.

pypingou commented 7 years ago

Github supports openid? I thought they did 0Auth2?

ncoghlan commented 7 years ago

Clarified the title - I just meant 3rd party login support in general, rather than specifically using the original OpenID protocol.

The required OAuth2 underpinnings in the live web service are going to be needed to properly support OpenID Connect anyway: https://github.com/release-monitoring/anitya/issues/442

jeremycline commented 7 years ago

This will be my first time really working with 3rd party authentication so I want to throw out what I'm thinking seems sane to see if people already know the "right" way to do this or see immediate flaws in my pre-coffee Monday morning thinking.

Would the proper way to do this be:

  1. Add a database table for users. It wouldn't contain a whole lot since we don't really have user settings or anything like that, but we do want to identify who did what to which projects (not something we can easily do at the moment AFAIK).

  2. Add a database table or tables for each 3rd party login where we relate its unique identifier (the "sub" field in OpenID Connect) to the login provider and the user. That way a user can associate a lot of 3rd party login with one account.

  3. When a user logs in, we check to see if we know about that unique identifier for that login provider and from there find the user.

Some immediate problems I see with that approach are that I might authenticate with GitHub, log out, re-authenticate with Fedora, and then I have two separate user accounts running around behind the scenes. It seems like we could overcome that, though, by resolving duplicates when additional 3rd party logins are linked.

Or maybe that's entirely too complicated?

ncoghlan commented 7 years ago

http://stackoverflow.com/questions/6666267/architecture-for-merging-multiple-user-accounts-together has a good discussion of some of the trade-offs involved here.

In Anitya's case, email addresses are already used as the unique identifier for user actions, and they're captured in the automatic logging. Both the original OpenID web login support and the OpenID Connect support for the API ensure that the email ID is set appropriately for any write actions.

So I think your design sounds sensible, and one way to build towards doing that sensibly would be to:

  1. Link auto-generated numeric user IDs to particular email addresses based on the existing authentication functionality - the "Primary Email" would be the only field in the initial user profiles
  2. To start, simply reject attempts to use different authentication providers with the same primary email (this won't be a problem for the initial period where GitHub is the only supported OAuth2 provider)
  3. At some later date, add a "link additional accounts" flow to the user profile page (once there is a user profile page). Linking an additional account would require reauthenticating with a previously linked account to minimise the risk of account hijacking.

I also went looking at various Flask authentication and user management libraries, and couldn't see anything suitable for our needs here - all of the ones I could find started with the assumption that you wanted local user account management (with login forms, password reset emails, etc), rather than relying solely on external identity management services. Even the "social auth" libraries assumed that was in addition to local auth, rather than being the sole supported access mechanism.

Note: much of what needs to be built for this will also be needed to properly support OpenID Connect in the live web service: https://github.com/release-monitoring/anitya/issues/442

pypingou commented 7 years ago

You may want to talk with @puiterwijk about social login since I seem to remember he has something for this.

jeremycline commented 7 years ago

One thing we might want to think about is the OAuth2 scopes we're currently requiring to interact with the Anitya APIs. The Fedora Ipsilon deployment can offer these scopes, but GitHub doesn't issue tokens with the scopes we're asking for. Do we want to make having a Fedora account a requirement for write access to the Anitya API?

One thing @pypingou suggested during our hackfest last week was to follow in Pagure's footsteps and do our own API tokens that aren't OAuth 2.0 tokens since we're aiming to support multiple 3rd party Identity Providers. That was we can let users issue tokens with ACL restrictions regardless of their Identity Provider. It sounds appealing to me, but what does everyone else think?

ncoghlan commented 7 years ago

The need to register OAuth2 scopes with the provider does indeed make it difficult to rely on them for authorization in addition to authentication - it would be far more convenient to keep those definitions local to the app.

So +1 from me for managing the API tokens within the scope of Anitya itself.

glensc commented 7 years ago

just to throw some comment on https://github.com/release-monitoring/anitya/issues/485#issuecomment-298342467

having email as primary key is very wrong. for example i changed my email in FAS, what happens in anytya on that? personality split? outdated email?

jeremycline commented 7 years ago

having email as primary key is very wrong. for example i changed my email in FAS, what happens in anytya on that? personality split? outdated email?

In OpenID Connect/OAuth 2 there's a uniquely identifying field ("sub" in the OpenID JWT) which we'll use rather than the email. This should make it fine to change your email in FAS, GitHub, etc. As long as you authenticate through the same provider, we'll recognize you. If you mix and match providers and don't have the same email on both FAS and GitHub (for example), we won't know you're the same person and you'll have two Anitya users, but that's what the "merge" feature will be for.

jeremycline commented 7 years ago

@ncoghlan did your investigations include python-social-auth? Based on my investigation, you don't need to do local users with passwords, you just need a table of users. Of course, I haven't actually tried that yet, so I could certainly be wrong.

@puiterwijk, is that what @pypingou was referring to in https://github.com/release-monitoring/anitya/issues/485#issuecomment-298549346? I see you're making PRs to the project.

ncoghlan commented 7 years ago

@jeremycline I'm not sure - either I was focusing on Flask-specific libraries, and hence missed that as a general Python option with Flask support, or else I just misread their docs and thought you needed local user auth as a pre-requisite.

Either way, I'd be delighted to learn I was wrong, as this seemed to be far too common a problem to not already have a decent library out there to handle it :)

puiterwijk commented 7 years ago

@jeremycline That is not what @pypingou was referring to, no. I was working on a hosted service where websites only need to auth agains that service, and that service than allows people to login with their social accounts.

My contributions to python-social-auth was because of other projects making use of it that broke.

pypingou commented 7 years ago

I was working on a hosted service

Is the project dead ? Or just not a priority atm ?

simonvanderveldt commented 4 years ago

Has there been any progress made on this? I'd like to add some projects but don't really fancy creating another account.

Or does this

Zlopez added this to Nice to have in Maintenance mode on Apr 10, 2019

mean the whole project is in maintenance mode?

Zlopez commented 4 years ago

The maintenance mode didn't started yet. I just don't have enough time to work on this full time.

So I'm mostly trying to solve bugs and most pressing issues. Not really getting to implementation of any enhancements.

simonvanderveldt commented 4 years ago

@Zlopez OK, clear. Thanks for the update