1319246 - Allow login with plain email from Auth0

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect)

Description

[Cameron Dawson [:camd]]

We support login only for email addresses with LDAP access.  We should discuss adding support for raw emails that will give limited (not sheriff) access.

Comment 1

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:auth-passwordless > mozilla:master

Comment 2

[Ed Morley [:emorley]]

Comment on attachment 8816264 [details] [review]
[treeherder] mozilla:auth-passwordless > mozilla:master

Think this is almost there - have left some comments :-)

Comment 3

[Cameron Dawson [:camd]]

Comment on attachment 8816264 [details] [review]
[treeherder] mozilla:auth-passwordless > mozilla:master

I believe I've addressed everything in your review.  Please take a look at your convenience.  :)

Comment 5

[Cameron Dawson [:camd]]

Hi Dustin-- I'm wondering if you'd help Ed and I determine the right approach here:

---------------------------
From edmorley on Dec 2, 2016 Member on PR https://github.com/mozilla/treeherder/pull/2011

Should we explicitly trim off the email/ prefix rather than just extracting any email-like string? It just seems like this regex would extract an email from eg fake/[e-mail]/fake which may or may not be covered by the protections that Dustin mentioned in the Vidyo call.

@djmitche, does this look ok to you? Or would extracting the email from auth:create-client:email/[e-mail]/* be preferred?

Also, I think we said it might be safer to allow using clientID, but to never match against an existing user, so they couldn't inherit sheriff permissions, or someone else's Treeherder Hawk secrets?
---------------------------

Back to Cam speaking:
My current impl extracts the email from the clientId and either logs them in (if the clientId exists) or create a new user for them if not.  Even if the clientId was fake/[e-mail]/fake, we would log them in.  (It also will convert old users if they have the mozilla-user scope)

I admit, the details of the vidyo call we had is fading in my memory, so apologies that this is a repeat question.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Let me be perfectly clear: if you are trying to look at strings in the list of scopes, you're doing it wrong.  The only thing you can do with scopes is ask "does the user have scope $xyz".

Extracting the email from the clientId gives you an $xyz to ask about.  I'd rather look for "[^/]*/([^/]*)" and take group 1 than just look for anything email-like, but that's probably not too important.  The important thing is to ask if the user has a scope, where the scope is based on that email.  For Mozilla LDAP users, as we've seen, that's `assume:mozilla-user:<email>`.

For non-mozilla users, I think the intent was to just use the clientId as their username -- so no need to match scopes at all.

If that's not enough, then we can consider adding an `assume:verified-email:<email>` scope that you could look for.

Comment 7

[Cameron Dawson [:camd]]

Comment on attachment 8816264 [details] [review]
[treeherder] mozilla:auth-passwordless > mozilla:master

Hey Ed--  I had put this on the back-burner while I finished the shared queue ownership in Pulse Guardian in Jan.  But I've made the updates now and tested it.  It seems to work well.  :)

Comment 8

[Ed Morley [:emorley]]

Comment on attachment 8816264 [details] [review]
[treeherder] mozilla:auth-passwordless > mozilla:master

I've left some comments on the PR.

This takes a while to page back in each time we have a review cycle, since I'm wanting to make sure my understanding is complete given it's security-critical (there's been about a month gap after each of the two previous reviews before the next review is requested).

From the sounds of it a shorter review cycle this time is what you were aiming for - if so, that would really help me out :-)

Comment 9

[Cameron Dawson [:camd]]

Ed-- Yeah, I really apologize for the time-lag.  I won't let this lag again.

Comment 10

[Cameron Dawson [:camd]]

Comment on attachment 8816264 [details] [review]
[treeherder] mozilla:auth-passwordless > mozilla:master

Hey Ed-- Sorry for all the back and forth on this one.  I believe I addressed all your concerns.  I re-worked the backend tests as they were a little squirly.  I hope this is easier to follow and more thorough..  :)

Comment 11

[Ed Morley [:emorley]]

Comment on attachment 8816264 [details] [review]
[treeherder] mozilla:auth-passwordless > mozilla:master

Looking awesome and now great test coverage - thank you!

Comment 13

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/f38fa97c53213359691540bd14802ee71bd4a599
Bug 1319246 - Allow login with plain email from Auth0 (#2011)

This will also  migrate the username of existing users logging 
in with LDAP to be the ``clientId`` we get from the login.

Comment 14

[Ed Morley [:emorley]]

I've performed some basic testing on stage and appears to be working well:
* my existing is_staff login was migrated to the client-id based username after logging in with Okta
* then after I confirmed logged in via auth0 email login (a) worked, and (b) a new account was correctly created rather than allowing access to the existing privileged account.

Comment 15

[Ed Morley [:emorley]]

s/logged/logging/

I'll see about doing a prod push tomorrow to get this rolled out :-)

Comment 16

[Ed Morley [:emorley]]

Deployed to prod :-)