Re: [pgsql-www] Google signin

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...