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

RESOLVED FIXED

Status

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: camd, Assigned: ydidwania, Mentored)

Tracking

({good-first-bug})

Details

(Whiteboard: [lang=py])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Assignee: nobody → cdawson

Comment 1

2 years ago
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 :-)
Depends on: 1331197, 1319246
The last of the legacy users have been purged in bug 1331197, so we're good to go with this :-)
(Reporter)

Updated

a year ago
Mentor: cdawson
Keywords: good-first-bug
Whiteboard: [lang=py]
(Assignee)

Comment 3

a year ago
Hi. Cameron, are u still working on this?
Flags: needinfo?(cdawson)
(Reporter)

Comment 4

a year ago
Ydidwania--  I'm not at the moment.  Please feel free to take is.  I would really appreciate the help on it.  :)
Assignee: cdawson → nobody
Flags: needinfo?(cdawson)
(Assignee)

Comment 5

a year ago
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?
Flags: needinfo?(cdawson)
(Reporter)

Comment 6

a year ago
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.
Flags: needinfo?(cdawson)
Created attachment 8896212 [details] [review]
[treeherder] ydidwania:1337987 > mozilla:master
(Assignee)

Comment 8

a year ago
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.
Flags: needinfo?(cdawson)
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.
(Assignee)

Comment 10

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

Updated

a year ago
Assignee: nobody → didwaniayashvardhan
Status: NEW → ASSIGNED

Updated

a year ago
Attachment #8896212 - Flags: review?(cdawson)
(Reporter)

Comment 11

a year ago
ydidwania--  Looks like there are still some test failures.  Please fix those and I'll take another look.  :)
Flags: needinfo?(cdawson)
(Assignee)

Comment 12

a year ago
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?
Flags: needinfo?(cdawson)
(Assignee)

Comment 13

a year ago
Made changes to the PR. I hope its better now
(Reporter)

Comment 14

a year ago
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.  :)
Flags: needinfo?(cdawson)
(Assignee)

Comment 15

a year ago
Made the necessary changes to the PR. Please take a look
Flags: needinfo?(cdawson)
(Reporter)

Updated

a year ago
Attachment #8896212 - Flags: review?(cdawson) → review+
(Reporter)

Comment 16

a year ago
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

a year ago
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
(Reporter)

Comment 18

a year ago
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!!  :)
(Reporter)

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(cdawson)
Resolution: --- → FIXED
(Assignee)

Comment 19

a year ago
I get what you mean. Will keep that in mind next time. 
Thanks for letting me work on this! :)
You need to log in before you can comment on or make changes to this bug.