Re: [pgsql-www] Google signin

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

> > 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?
>
> Yes. And once the account is created they can keep changing it if they got
> it wrong. It'll automatically populate the downstream authentication
> systems once they log in there.

Right, good stuff.

> > > +# 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"?
>
> No, it's actually referring to "hashers" which is what the different django
> hashing implementations are called.

Ah, silly Python.

> > > +     # 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.
>
> Sure :)

:)

> > 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.
> >
>
> Yes, let's avoid that level of feature creep...

Yeah, yeah.

> > > +             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?
> >
>
> Yes.

Ok.

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

> > > +<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" ?
>
> Yup.

:)

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

Thank *you* for working on this!  Would really be a huge help for the
downstream systems, as discussed.

Thanks again!

Stephen