Обсуждение: [pgsql-www] Google signin

I'm working on Cleaning Up Some Old Branches (TM) in the pgweb repository, and found one I did during some airport hacking that I forgot to post to people about.

It's been discussed a couple of times that we should perhaps support Google signin for community auth.

Basically, the idea behind it would be that on the login page you would both have the regular userid/password box, and also a button for "sign in with google". If somebody signs in with Google, it would automatically match it to their community account based on email address (since Google doesn't have the concept of a separate userid, and even if they did that would open up all sorts of hijacking vulnerabilities). If they didn't already have a community account, we'd offer to create one automatically and copy the main information over from the Google profile.

My implementation so far, which does  the login but not the provisioning of new accounts yet, is about 50 lines of python/django and 25 lines of javascript. So it's not very difficult to do.

The bigger question is - do we *want* to do this?

On Wed, Jul 12, 2017 at 1:23 PM, Magnus Hagander <magnus@hagander.net> wrote:

I'm working on Cleaning Up Some Old Branches (TM) in the pgweb repository, and found one I did during some airport hacking that I forgot to post to people about.

It's been discussed a couple of times that we should perhaps support Google signin for community auth.

Basically, the idea behind it would be that on the login page you would both have the regular userid/password box, and also a button for "sign in with google". If somebody signs in with Google, it would automatically match it to their community account based on email address (since Google doesn't have the concept of a separate userid, and even if they did that would open up all sorts of hijacking vulnerabilities). If they didn't already have a community account, we'd offer to create one automatically and copy the main information over from the Google profile.

My implementation so far, which does  the login but not the provisioning of new accounts yet, is about 50 lines of python/django and 25 lines of javascript. So it's not very difficult to do.

The bigger question is - do we *want* to do this?

I think it's a reasonable option, though it would open up debate on what else to support. GitHub springs to mind... 

On Wed, Jul 12, 2017 at 2:30 PM, Dave Page <dpage@pgadmin.org> wrote:

On Wed, Jul 12, 2017 at 1:23 PM, Magnus Hagander <magnus@hagander.net> wrote:

I'm working on Cleaning Up Some Old Branches (TM) in the pgweb repository, and found one I did during some airport hacking that I forgot to post to people about.

It's been discussed a couple of times that we should perhaps support Google signin for community auth.

Basically, the idea behind it would be that on the login page you would both have the regular userid/password box, and also a button for "sign in with google". If somebody signs in with Google, it would automatically match it to their community account based on email address (since Google doesn't have the concept of a separate userid, and even if they did that would open up all sorts of hijacking vulnerabilities). If they didn't already have a community account, we'd offer to create one automatically and copy the main information over from the Google profile.

My implementation so far, which does  the login but not the provisioning of new accounts yet, is about 50 lines of python/django and 25 lines of javascript. So it's not very difficult to do.

The bigger question is - do we *want* to do this?

I think it's a reasonable option, though it would open up debate on what else to support. GitHub springs to mind... 

Or facebook. Or twitter. Or Microsoft. Or whatnot.

But of all of them to pick, Google is probably the best one to start with at least, given the largest coverage (at least of people who are willing to use it for this).

I wouldn't object to supporting others as well, but it's not part of what I've hacked on so far :) 

On Jul 12, 2017, at 8:30 AM, Dave Page <dpage@pgadmin.org> wrote:

On Wed, Jul 12, 2017 at 1:23 PM, Magnus Hagander <magnus@hagander.net> wrote:

I'm working on Cleaning Up Some Old Branches (TM) in the pgweb repository, and found one I did during some airport hacking that I forgot to post to people about.

It's been discussed a couple of times that we should perhaps support Google signin for community auth.

Basically, the idea behind it would be that on the login page you would both have the regular userid/password box, and also a button for "sign in with google". If somebody signs in with Google, it would automatically match it to their community account based on email address (since Google doesn't have the concept of a separate userid, and even if they did that would open up all sorts of hijacking vulnerabilities). If they didn't already have a community account, we'd offer to create one automatically and copy the main information over from the Google profile.

My implementation so far, which does  the login but not the provisioning of new accounts yet, is about 50 lines of python/django and 25 lines of javascript. So it's not very difficult to do.

The bigger question is - do we *want* to do this?

I think it's a reasonable option, though it would open up debate on what else to support. GitHub springs to mind… 

Would this work with @postgresql.org accounts?  AFAIK they are not configured with Google services.

On 12 July 2017 at 13:23, Magnus Hagander <magnus@hagander.net> wrote:
> I've attached a screenshot of what the implementation looks like at this
> point. Obviously, CSSing and things can always be improved.

The main point of this would be to not have a new password so I find
it strange that there's still a password field at all. Maybe this is
just the CSSing you describe if you styled it so there were two
options, "log in with password" and "log in with Google" and they were

obviously two independent options. The other option with broad
coverage would be Facebook, but for our community github is also
tempting (Is OpenID still a thing?).

The big question though is whether to still require a community id at
all. If we just let anyone log in via Google and create a placeholder
account on demand if one doesn't exist then you shouldn't have to go
through the "create an account" step at all. And you shouldn't have to
remember a new userid at all.

On 12 July 2017 at 15:27, Magnus Hagander <magnus@hagander.net> wrote:
>
> That said, this is not intended to *replace* the username/password login
> part, it would be in addition to it.

In that case I think it's actively a bad idea. If people would still
have to create another password there's no point and it would
contribute to people not wanting to use common auth schemes like this.

If you just have to make a username that's not as bad (though not ideal).

Magnus Hagander <magnus@hagander.net> writes:
> On Wed, Jul 12, 2017 at 4:16 PM, Greg Stark <stark@mit.edu> wrote:
>> The big question though is whether to still require a community id at
>> all. If we just let anyone log in via Google and create a placeholder
>> account on demand if one doesn't exist then you shouldn't have to go
>> through the "create an account" step at all. And you shouldn't have to
>> remember a new userid at all.

> The point of the create an account step would be if somebody has a pg
> account under something@somewhere.com and logs in using
> mygoogle@somewhere.com they should at least get a notification before we
> create the new account. But we should make doing that trivial, as in a
> pre-filled-out signup form with the info from google/whatever and just a
> "click here to confirm" box.

I'm wondering about the security implications of this --- would it mean
that anybody with a google account could, eg, spam our wiki?

I don't mind reducing barriers to entry when we can, but recent experience
says that there has to be some barrier :-(

On 12 Jul 2017, at 15:31, Magnus Hagander <magnus@hagander.net> wrote:
<snip>
> OpenID is not, OAuth 2 is.
>
> Google, Github and Facebook all speak OAuth 2. I have working implementations for both Google and Github, so I'm sure it would be easy enough to make one for Facebook. I will see how much work it is to move that code over instead of using the Google javascript API that I did now. TBH, it's probably *easier* because it's not javascript :)

As a thought, we could implement something like Auth0 (auth0.com),
which does OAuth2 and provides a login for Google, FB, LinkedIn, GitHub,
and others.

Pro's
*****

* Pretty simple to implement
* It has a reasonable management interface for picking and choosing
  with auth providers to allow (eg we can choose GitHub, Google, FB,
  and no enable others) 

* The management interface has reasonable reporting too, to show
  user activity, stats, etc

* Free for Open Source projects

* They're PG friendly, with instructions for using PG in their setup
  docs :)

Con's
*****

* Not Open Source, though their setup examples and other supporting
  bits are on GitHub

On Wed, Jul 12, 2017 at 4:35 PM, Greg Stark <stark@mit.edu> wrote:

On 12 July 2017 at 15:27, Magnus Hagander <magnus@hagander.net> wrote:
>
> That said, this is not intended to *replace* the username/password login
> part, it would be in addition to it.

In that case I think it's actively a bad idea. If people would still
have to create another password there's no point and it would
contribute to people not wanting to use common auth schemes like this.

They would not have to make another password. It would be possible to create a community account without a password (which would then only be reachable through an oauth login from a supported provider).

 

If you just have to make a username that's not as bad (though not ideal).

That would be the idea.

On Wed, Jul 12, 2017 at 4:37 PM, Magnus Hagander <magnus@hagander.net> wrote:

On Wed, Jul 12, 2017 at 4:35 PM, Greg Stark <stark@mit.edu> wrote:

On 12 July 2017 at 15:27, Magnus Hagander <magnus@hagander.net> wrote:
>
> That said, this is not intended to *replace* the username/password login
> part, it would be in addition to it.

In that case I think it's actively a bad idea. If people would still
have to create another password there's no point and it would
contribute to people not wanting to use common auth schemes like this.

They would not have to make another password. It would be possible to create a community account without a password (which would then only be reachable through an oauth login from a supported provider).

 

If you just have to make a username that's not as bad (though not ideal).

That would be the idea.

So to show it in more detail, here is an example of the workflow of my prototype: https://www.hagander.net/tmp/flow.ogv

(note for those reading in the archives -- link will go dead in a couple of days) 

Magnus,

* Magnus Hagander (magnus@hagander.net) wrote:
> On Thu, Jul 13, 2017 at 9:18 PM, Magnus Hagander <magnus@hagander.net>
> wrote:
> > On Wed, Jul 12, 2017 at 4:37 PM, Magnus Hagander <magnus@hagander.net>
> > wrote:
> > So to show it in more detail, here is an example of the workflow of my
> > prototype: https://www.hagander.net/tmp/flow.ogv
>
> So here is the code the way it currently looks.
>
> Comments on the workflow and/or the code?
>
> Is this something we *want*? I definitely think so, as it will simplify
> things for a lot of casual users. In particular users who are on some of
> our third party systems, such as conference attendees or the new
> mailinglist system.

I'm 110% in favor of getting this going, as one of the individuals who
runs one of those third party systems..

> diff --git a/pgweb/account/forms.py b/pgweb/account/forms.py
> index 75297b1..a8ab584 100644
> --- a/pgweb/account/forms.py
> +++ b/pgweb/account/forms.py
> @@ -12,6 +12,17 @@ from recaptcha import ReCaptchaField
>  import logging
>  log = logging.getLogger(__name__)
>
> +def _clean_username(username):
> +     username = username.lower()
> +
> +     if not re.match('^[a-z0-9\.-]+$', username):
> +             raise forms.ValidationError("Invalid character in user name. Only a-z, 0-9, . and - allowed for compatibility with third party software.")
> +     try:
> +             User.objects.get(username=username)
> +     except User.DoesNotExist:
> +             return username
> +     raise forms.ValidationError("This username is already in use")

We should really have a DB constraint which enforces uniqueness on
lower(username), imv, and perhaps try to proactively do something about
accounts which have invalid characters, but that's a different
discussion.

> +# Github login
> +#  Registration: https://github.com/settings/developers
> +#
> +def oauth_login_github(request):
> +     def _github_auth_data(oa):
> +             # Github just returns full name, so we're just going to have to
> +             # split that.
> +             r = oa.get('https://api.github.com/user').json()
> +             n = r['name'].split(None, 1)

This split full-name is just what we pre-populate the form for the user
with, right?  They have the opportunity to fix it before the account is
created, if they want, and if they do change it, it's not going to
impact anything downstream?

> +# The value we store in user.password for oauth logins. This is
> +# a value that must not match any hashers.

Shouldn't that be "hashes", not "hashers"?

> +     # Don't allow users whos accounts were created via oauth to change
> +     # their email, since that would kill the connection between the
> +     # accounts.

That would be "whose", I believe.

Also, just another reason that we should be thinking about how to
support multiple email addresses associated with a single account..
That's a whole different effort though.

> +             if form.is_valid():
> +                     log.info("Creating user for {0} from {1} from oauth signin of email {2}".format(form.cleaned_data['username'], get_client_ip(request), request.session['oauth_email']))

I assume this 'is_valid()' call verifies that the username is valid
(doesn't contain any invalid characters) through calling the
"clean_username" stuff?

> +             form = SignupOauthForm(initial={
> +                     'username': request.session['oauth_email'].replace('@', '.'),
> +                     'email': request.session['oauth_email'],
> +                     'first_name': request.session['oauth_firstname'],
> +                     'last_name': request.session['oauth_lastname'],
> +             })

Given that we're offering to do this, I almost wonder if we should just
automatically do it rather than making them jump through the extra
hoop..  That would have to be after we figure out a way to have accounts
support multiple email addresses tho.

> +<p>
> +  If you with so sign up for a new account, please select a username
> +  and verify the other details:
> +</p>

Should be "wish to" ?

The rest of it looks good to me.  Would love to get this implemented
sooner rather than later...

* Magnus Hagander (magnus@hagander.net) wrote:
> On Fri, Aug 11, 2017 at 5:15 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > We should really have a DB constraint which enforces uniqueness on
> > lower(username), imv, and perhaps try to proactively do something about
> > accounts which have invalid characters, but that's a different
> > discussion.
>
> I could've sworn we had a constraint on the lowercase part. It must be in a
> different one of our systems though, because I can't see it in this one..

I wouldn't have mentioned it if I had seen one there. :)

Having it in a downstream system but not in the authorative one strikes
me as a bad idea..

> > Given that we're offering to do this, I almost wonder if we should just
> > automatically do it rather than making them jump through the extra
> > hoop..  That would have to be after we figure out a way to have accounts
> > support multiple email addresses tho.
>
> We need them to pick a username, that's why we need an extra form to be
> filled out. We could remove the name part of it to make it even simpler,
> and have them change that after the fact, but I'm not sure that actually
> simplifies things that much.
>
> We could get away with that if we could use the email address as the
> username, but that breaks with mediawiki at least.

I think you missed my point here- technically we need a username which
isn't their email address, but they don't necessairly have to pick one
and, indeed, this code actually builds one for them as a
recommendation.  Given that they don't need to know their username for
logging in with Google or similar, it really could just be something
that we internally keep track of.

That does bring up a question though- does the code make sure to avoid a
conflict with an existing username in the one that's suggested?  I
didn't see that, and we should probably do that.

* Magnus Hagander (magnus@hagander.net) wrote:
> On Mon, Aug 14, 2017 at 5:17 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Magnus Hagander (magnus@hagander.net) wrote:
> > > On Fri, Aug 11, 2017 at 5:15 PM, Stephen Frost <sfrost@snowman.net>
> > wrote:
> > > > We should really have a DB constraint which enforces uniqueness on
> > > > lower(username), imv, and perhaps try to proactively do something about
> > > > accounts which have invalid characters, but that's a different
> > > > discussion.
> > >
> > > I could've sworn we had a constraint on the lowercase part. It must be
> > in a
> > > different one of our systems though, because I can't see it in this one..
> >
> > I wouldn't have mentioned it if I had seen one there. :)
> >
> > Having it in a downstream system but not in the authorative one strikes
> > me as a bad idea..
>
> Yes. That's a recipie for trouble.

Ok, so, you going to fix it, or should I, and do we need a migration for
it..?

> > > > Given that we're offering to do this, I almost wonder if we should just
> > > > automatically do it rather than making them jump through the extra
> > > > hoop..  That would have to be after we figure out a way to have
> > accounts
> > > > support multiple email addresses tho.
> > >
> > > We need them to pick a username, that's why we need an extra form to be
> > > filled out. We could remove the name part of it to make it even simpler,
> > > and have them change that after the fact, but I'm not sure that actually
> > > simplifies things that much.
> > >
> > > We could get away with that if we could use the email address as the
> > > username, but that breaks with mediawiki at least.
> >
> > I think you missed my point here- technically we need a username which
> > isn't their email address, but they don't necessairly have to pick one
> > and, indeed, this code actually builds one for them as a
> > recommendation.  Given that they don't need to know their username for
> > logging in with Google or similar, it really could just be something
> > that we internally keep track of.
>
> We could, but that username is *visible* on systems. Anybody logging in
> that way and then using the wiki, or the cf app, or redmine, etc, will have
> that username exposed and will probably come asking to change it. And we
> can't change usernames since it's the primary key across the distributed
> system...

Hrmpf.  I agree, that's slightly annoying, but that strikes me as a
reason to make it something other than a transposed email address, not
that the idea is a terrible one.  What about something like first-name,
last-initial, followed by a number to avoid dups?

> > That does bring up a question though- does the code make sure to avoid a
> > conflict with an existing username in the one that's suggested?  I
> > didn't see that, and we should probably do that.
>
> In that case it will propose an invalid username and you will get the error
> when you hit save. That's definitely a flow that could be improved if we
> think this risks happening often.

Using the email address as the basis likely makes that unlikely (though
not impossible), but if we change the basis of the username then it may
become more likely...

I do think there's value in removing the need for this form to be filled
out, particularly for $user who doesn't have a .Org account and isn't on
the .Org site but is actually on some other website (eg:
postgresopen.org or pgconf.eu) and just wants to register for a
conference- they could possibly just hit 'login with google' or whatever
and then have everything happen magically for them without ever actually
seeing the .Org website.

On Mon, Aug 14, 2017 at 9:59 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Magnus Hagander (magnus@hagander.net) wrote:
> On Mon, Aug 14, 2017 at 5:17 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Magnus Hagander (magnus@hagander.net) wrote:
> > > On Fri, Aug 11, 2017 at 5:15 PM, Stephen Frost <sfrost@snowman.net>
> > wrote:
> > > > We should really have a DB constraint which enforces uniqueness on
> > > > lower(username), imv, and perhaps try to proactively do something about
> > > > accounts which have invalid characters, but that's a different
> > > > discussion.
> > >
> > > I could've sworn we had a constraint on the lowercase part. It must be
> > in a
> > > different one of our systems though, because I can't see it in this one..
> >
> > I wouldn't have mentioned it if I had seen one there. :)
> >
> > Having it in a downstream system but not in the authorative one strikes
> > me as a bad idea..
>
> Yes. That's a recipie for trouble.

Ok, so, you going to fix it, or should I, and do we need a migration for
it..?

I've got it on my list.

 

> > > > Given that we're offering to do this, I almost wonder if we should just
> > > > automatically do it rather than making them jump through the extra
> > > > hoop..  That would have to be after we figure out a way to have
> > accounts
> > > > support multiple email addresses tho.
> > >
> > > We need them to pick a username, that's why we need an extra form to be
> > > filled out. We could remove the name part of it to make it even simpler,
> > > and have them change that after the fact, but I'm not sure that actually
> > > simplifies things that much.
> > >
> > > We could get away with that if we could use the email address as the
> > > username, but that breaks with mediawiki at least.
> >
> > I think you missed my point here- technically we need a username which
> > isn't their email address, but they don't necessairly have to pick one
> > and, indeed, this code actually builds one for them as a
> > recommendation.  Given that they don't need to know their username for
> > logging in with Google or similar, it really could just be something
> > that we internally keep track of.
>
> We could, but that username is *visible* on systems. Anybody logging in
> that way and then using the wiki, or the cf app, or redmine, etc, will have
> that username exposed and will probably come asking to change it. And we
> can't change usernames since it's the primary key across the distributed
> system...

Hrmpf.  I agree, that's slightly annoying, but that strikes me as a
reason to make it something other than a transposed email address, not
that the idea is a terrible one.  What about something like first-name,
last-initial, followed by a number to avoid dups?

Agree that it's a bad flow for most users.However, I'm not sure *not* giving them the choice is going to be very helpful either. We can change the cf app to just use the full name for example, but there is no way we can get these names out of being very prominent on the wiki or redmine. So I think we *have* to let people choose.

We can certainly change how we create the initial suggestion for them. And maybe firstname/lastname is a better choice, I agree.

 

> > That does bring up a question though- does the code make sure to avoid a
> > conflict with an existing username in the one that's suggested?  I
> > didn't see that, and we should probably do that.
>
> In that case it will propose an invalid username and you will get the error
> when you hit save. That's definitely a flow that could be improved if we
> think this risks happening often.

Using the email address as the basis likely makes that unlikely (though
not impossible), but if we change the basis of the username then it may
become more likely...

Agreed.

Though if we have a retrying method as you suggested above we can make the check first and just try again until we present something that was at least not used when the form was *built*. Of course if somebody else signs up with that userid while we wait for them to submit the form it's still going to give them an error and require a change, but that seems acceptable.

I do think there's value in removing the need for this form to be filled
out, particularly for $user who doesn't have a .Org account and isn't on
the .Org site but is actually on some other website (eg:
postgresopen.org or pgconf.eu) and just wants to register for a
conference- they could possibly just hit 'login with google' or whatever
and then have everything happen magically for them without ever actually
seeing the .Org website.

I fully agree. I just don't see a way to do it, without changing the core of community auth to be based off email instead of username, and stopping the use of software like mediawiki. Which I don't think we are willing to do.

 

> On 15 Aug 2017, at 12:18, Magnus Hagander <magnus@hagander.net> wrote:
>
> Here's an updated patch

In the below hunk, s/decicated/dedicated/:

+a decicated account, or use one of the third party sign-in systems below.

Without being terribly well versed in Django (or Python), the logic seems quite
reasonable to me on a read through/review.

> that does this. It will try in order:
> <firstname><lastinitial>, e.g. stephenf
> <firstinitial><lasdtname>,e.g. sfrost
> <firstname><lastinitial><number>, e.g. stephenf0, stephenf1, stephenf2 etc

How about a random number instead?  Not that I see any immediate risk with
anything here, but many years of looking at logs from web attacks has taught me
that predictability is what is being tried first.

A big +1 on getting this functionality in.

On 15 August 2017 at 11:18, Magnus Hagander <magnus@hagander.net> wrote:
> On Tue, Aug 15, 2017 at 10:42 AM, Magnus Hagander <magnus@hagander.net>
> wrote:
>
> Here's an updated patch that does this. It will try in order:
> <firstname><lastinitial>, e.g. stephenf
> <firstinitial><lasdtname>,e.g. sfrost
> <firstname><lastinitial><number>, e.g. stephenf0, stephenf1, stephenf2 etc

Taking just the first name or just the last name and a number seems
fairly safe but anything that combines letters runs the risk of
creating unintentionally offensive words.

On Tue, Aug 15, 2017 at 11:34 PM, Greg Stark <stark@mit.edu> wrote:

On 15 August 2017 at 11:18, Magnus Hagander <magnus@hagander.net> wrote:
> On Tue, Aug 15, 2017 at 10:42 AM, Magnus Hagander <magnus@hagander.net>
> wrote:
>
> Here's an updated patch that does this. It will try in order:
> <firstname><lastinitial>, e.g. stephenf
> <firstinitial><lasdtname>,e.g. sfrost
> <firstname><lastinitial><number>, e.g. stephenf0, stephenf1, stephenf2 etc

Taking just the first name or just the last name and a number seems
fairly safe but anything that combines letters runs the risk of
creating unintentionally offensive words.

That's a good point. OTOH the user still gets a chance to change it before they accept it, so they can easily fix that. (And we don't generally have a filter for users who intentionally pick offensive words as their logins)