1337987 - Remove login backend auth code that finds a user by email, rather than username

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect)

Description

[Cameron Dawson [:camd]]

Once all users have logged in with their clientId, we can remove the code that finds users by their email and then updates their username to the clientId.

Comment 1

[Ed Morley [:emorley]]

Once bug 1319246 is deployed (and so new users are created with the client ID username) we can purge users with no relevant state in bug 1331197, reducing the number of users left to migrate :-)

Comment 2

[Ed Morley [:emorley]]

The last of the legacy users have been purged in bug 1331197, so we're good to go with this :-)

Comment 3

[Yashvardhan Didwania [:ydidwania]]

Hi. Cameron, are u still working on this?

Comment 4

[Cameron Dawson [:camd]]

Ydidwania--  I'm not at the moment.  Please feel free to take is.  I would really appreciate the help on it.  :)

Comment 5

[Yashvardhan Didwania [:ydidwania]]

I found the relevant code in this file - https://github.com/mozilla/treeherder/blob/master/treeherder/auth/backends.py#L112 . It has a function _find_user_by_email (Line 54) which finds the user by their email and then updates the username. Should I go ahead and delete this function and make changes wherever it has been used?

Comment 6

[Cameron Dawson [:camd]]

Yes, remove that, and just have it fall back to this code if we hit the first exception:

    # the user doesn't already exist, create it.
    logger.warning("Creating new user: {}".format(client_id))
    return User.objects.create_user(client_id, email=email)

When you remove it, please assign the pull request to either me or Ed Morley.  I'll be on PTO till Monday 8/15, however.

Comment 7

[GitHub Autolander Bot]

Attachment: [treeherder] ydidwania:1337987 > mozilla:master

Comment 8

[Yashvardhan Didwania [:ydidwania]]

I created the pull request. however, the checks have failed because of the tests in tests/auth/test_backends.py and tests/webapp/api/test_auth.py which expect new users to be created when a two users have same client__id but different email like in the earlier code. Currently new user is created only when different client_id is encountered.

Comment 9

[Ed Morley [:emorley]]

Thank you for the PR! I've left some comments on GitHub, but will also want Cameron to take a look when he's back from vacation tomorrow.

Comment 10

[Yashvardhan Didwania [:ydidwania]]

I pushed the changes you suggested. The flake8 lint Travis failure is fixed now

Comment 11

[Cameron Dawson [:camd]]

ydidwania--  Looks like there are still some test failures.  Please fix those and I'll take another look.  :)

Comment 12

[Yashvardhan Didwania [:ydidwania]]

camd - I suggested some changes on GitHub. I am not sure whether I should go ahead with it or not. I could make those changes and push if you say so. could you take a look?

Comment 13

[Yashvardhan Didwania [:ydidwania]]

Made changes to the PR. I hope its better now

Comment 14

[Cameron Dawson [:camd]]

I think there are still a few more comments to address.  I believe I was still adding a few comments as you were fixing them.  :)

Comment 15

[Yashvardhan Didwania [:ydidwania]]

Made the necessary changes to the PR. Please take a look

Comment 16

[Cameron Dawson [:camd]]

I commented in the PR, but everything is good to go except that it needs to be rebased on master.  Please do that and I'll merge it.  :)

Comment 17

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/f6ba8d07eb4621050580c034bd3d389f20d07901
Bug 1337987 - Remove login backend auth code that finds a user by email, rather than username (#2699)

* remove login backend code that finds user by email
* Modified tests. Added test_ldap_user to conftests.py

Comment 18

[Cameron Dawson [:camd]]

Thanks!!  No worries about the commits, I just squashed them on merge.  Though, just to note: we have a policy that each commit (when merged to master) must have the bug number preceding the first-line comment.  If you do a ``git log`` in the repo, you'll see what I mean.  The on exception is the package updates that are done by a 3rd party tool.

Thanks for the hard work!!  :)

Comment 19

[Yashvardhan Didwania [:ydidwania]]

I get what you mean. Will keep that in mind next time. 
Thanks for letting me work on this! :)