1272580 - automate ldap/bmo association for mozilla employees

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[:glob ✱]

the ldap entries for mozilla employees have a field for their bugzilla email address.  we should use that to automatically associate accounts with ldap.

this should happen during the login process, and can also happen periodically via cron.

the query we'd need to perform is:
-b 'o=com,dc=mozilla' '(|(bugzillaEmail=$addr)(mail=$addr))'

the ldap account we use to bind already has the access required to execute this.

Comment 1

[:glob ✱]

there are currently 330 users without ldap associations.

checking against the phonebook we can automatically fix 175 of these (53%), which is pretty good considering how many non-employees users there are.

Comment 2

[:glob ✱]

Attachment: mozreview: add ldap function for finding matching employees (bug 1272580);

Adds a function to our ldap library that, for a given email address, returns
all matching ldap accounts.  Matching is performed against the 'mail' and
'bugzillaEmail' attributes.

Review commit: https://reviewboard.mozilla.org/r/55040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55040/

Comment 3

[:glob ✱]

Attachment: mozreview: add rest resource to automatically associate employees with ldap (bug 1272580);

Adds a rest resource that automatically associates Review Board users with
matching employee LDAP accounts.  Supports both association of individual users
and all users.

Review commit: https://reviewboard.mozilla.org/r/55042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55042/

Comment 4

[:glob ✱]

Attachment: mozreview: add mach command to trigger automatic employee ldap association (bug 1272580);

Add a mach command to call the ldap_association resource.

Review commit: https://reviewboard.mozilla.org/r/55044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55044/

Comment 5

[:glob ✱]

Attachment: mozreview: perform automatic employee ldap associations on login (bug 1272580);

Perform automatic association of employee accounts against LDAP when they log
in via BMO's auth callback.

Review commit: https://reviewboard.mozilla.org/r/55046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55046/

Comment 6

[:glob ✱]

Attachment: mozreview: add test for automatic employee ldap (bug 1272580);

Review commit: https://reviewboard.mozilla.org/r/55048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55048/

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8756218 [details]
mozreview: add ldap function for finding matching employees (bug 1272580);

https://reviewboard.mozilla.org/r/55040/#review51842

This seems OK. About the only thing I can think of is adding a filter on objectClass presence to ensure the DN is part of a known type. But I don't think we do this elsewhere, so this should be OK.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8756219 [details]
mozreview: add rest resource to automatically associate employees with ldap (bug 1272580);

https://reviewboard.mozilla.org/r/55042/#review51844

I like the approach. You may want to flag smacleod for a review, as I don't have the RB web API code paged in right now.

::: pylib/mozreview/mozreview/ldap/resources.py:216
(Diff revision 1)
> +            # This is designed to be run from cron, so there's minimal logging
> +            # and results.

I think omitting logs for no-op is fine. But I don't there will be much activity once this has run once. I think it is important to establish an audit trail of what this is doing. So please log when things are updated.

::: pylib/mozreview/mozreview/ldap/resources.py:230
(Diff revision 1)
> +        else:
> +            # We can afford to be slightly more verbose when dealing with
> +            # a single user.
> +            try:
> +                user = User.objects.get(email=email)
> +            except ObjectDoesNotExist:
> +                logger.info('Could not update ldap association: target user '
> +                            '%s does not exist.' % email)
> +                return DOES_NOT_EXIST
> +            try:
> +                ldap_username, updated = self.associate_user(user)
> +                if updated:
> +                    logging.info(
> +                        'Associating user: %s with ldap_username: %s'
> +                        % (user.email, ldap_username))
> +                else:
> +                    logging.info(
> +                        'Associating user: %s already associated with '
> +                        'ldap_username: %s' % (user.email, ldap_username))
> +                result[self.item_result_key] = {
> +                    'user': {
> +                        'ldap_username': ldap_username,
> +                        'updated': updated,
> +                    }
> +                }
> +            except self.AssociationException as e:
> +                logger.info('Could not update ldap association: %s' % str(e))
> +                return 500, {'error': [str(e)]}

I think you should use the same code path for updating users. Always create a list and iterate over it. You can have a special case to omit logging if the user parameter was passed if you really need it.

::: pylib/mozreview/mozreview/ldap/resources.py:262
(Diff revision 1)
> +                return 500, {'error': [str(e)]}
> +
> +        return 200, result
> +
> +    def associate_user(self, user):
> +        ldap_users = find_employee_ldap(user.email)

In the previous patch, we created a new LDAP connection for each call to this function. Now that I see it can be called in bulk, I think we should pass an LDAP connection instance into that function to cut down on latency.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8756220 [details]
mozreview: add mach command to trigger automatic employee ldap association (bug 1272580);

https://reviewboard.mozilla.org/r/55044/#review51850

This looks good. I'm holding off on r+ until I see changes requested in previous commits.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8756221 [details]
mozreview: perform automatic employee ldap associations on login (bug 1272580);

https://reviewboard.mozilla.org/r/55046/#review51852

::: pylib/mozreview/mozreview/views.py:242
(Diff revision 1)
> +        ldap_users = find_employee_ldap(user.email)
> +        if ldap_users and len(ldap_users) == 1:
> +            ldap_username = ldap_users[0]
> +            mozreview_profile = get_profile(user)
> +            if mozreview_profile.ldap_username != ldap_username:
> +                mozreview_profile.ldap_username = ldap_username
> +                mozreview_profile.save()
> +                logging.info('Associating user: %s with ldap_username: %s'
> +                             % (user.email, ldap_username))

Hmmm. I've seen this code before. Any chance it can be consolidated?

::: pylib/mozreview/mozreview/views.py:252
(Diff revision 1)
> +                mozreview_profile.ldap_username = ldap_username
> +                mozreview_profile.save()
> +                logging.info('Associating user: %s with ldap_username: %s'
> +                             % (user.email, ldap_username))
> +    except Exception as e:
> +        logging.error('exception while performing ldap association: %s'

There is a logging.exception that logs the current exception with its stack. It is quite nice.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8756222 [details]
mozreview: add test for automatic employee ldap (bug 1272580);

https://reviewboard.mozilla.org/r/55048/#review51854

This needs a test for multiple LDAP entries having the same Bugzilla address.

Comment 12

[Steven MacLeod [:smacleod]]

Comment on attachment 8756218 [details]
mozreview: add ldap function for finding matching employees (bug 1272580);

https://reviewboard.mozilla.org/r/55040/#review51878

::: pylib/mozreview/mozreview/ldap/__init__.py:72
(Diff revision 1)
> +    """For the given email address, return a matching Mozilla employee's
> +    LDAP address (if found).  Both the mail and bugzillaEmail LDAP
> +    attributes will be checked.
> +    """

dosctrings should have a one line summary and then expanded information after a blank.

::: pylib/mozreview/mozreview/ldap/__init__.py:90
(Diff revision 1)
> +            matches.append(result[1]['mail'][0])
> +        return matches

blank line after indented blocks please

Comment 13

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/55042/#review51880

::: pylib/mozreview/mozreview/ldap/resources.py:175
(Diff revision 1)
> +    If a Review Board user's email address is provided via the --email
> +    argument, then only that user will be updated.  Without an --email
> +    argument all users will be updated.

Generally we refer to them as "fields" rather than "arguments" with command line argument styling (They are multi-part form data fields in the POST request)

::: pylib/mozreview/mozreview/ldap/resources.py:214
(Diff revision 1)
> +        if not request.user.has_perm('mozreview.modify_ldap_association'):
> +            logger.info('Could not update ldap association: permission '
> +                        'denied for user: %s' % (request.user.id))
> +            return PERMISSION_DENIED
> +
> +        result = {'links': self.get_links(request=request)}

you might as well pass the `list_child_resources` so if this resource ever gets children a change won't have to be made here.

::: pylib/mozreview/mozreview/ldap/resources.py:221
(Diff revision 1)
> +            # This is designed to be run from cron, so there's minimal logging
> +            # and results.
> +            result['updated'] = 0
> +            result['skipped'] = 0
> +            result['errors'] = 0
> +            for user in User.objects.all():

This is going to be *a lot* of queries to ldap... maybe we should instead join with the `MozReviewUserProfile`s and only attempt to associate users who have an empty association?

::: pylib/mozreview/mozreview/ldap/resources.py:261
(Diff revision 1)
> +                logger.info('Could not update ldap association: %s' % str(e))
> +                return 500, {'error': [str(e)]}
> +
> +        return 200, result
> +
> +    def associate_user(self, user):

can you give this a docstring please.

::: pylib/mozreview/mozreview/ldap/resources.py:272
(Diff revision 1)
> +        if mozreview_profile.ldap_username != ldap_username:
> +            mozreview_profile.ldap_username = ldap_username
> +            mozreview_profile.save()
> +            updated = True

I wonder if we should make the `mozreview_profile.ldap_username == ''` case distinct from the non empty, but different, `mozreview_profile.ldap_username` case... I'm not aware of employees having multiple ldap accounts, is that a thing?

In the case where they don't match it might be worth logging and investigating, it could indicate someone has associated with another employees account.

Doing this probably wouldn't actually matter much in practice, but it'd be easy to just switch over. If you feel strongly against this though feel free to drop.

Comment 14

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/55042/#review51880

Ah and forgot to say, the RB web API stuff looks fine to me.

Comment 15

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/55044/#review52244

::: testing/vcttesting/reviewboard/mach_commands.py:670
(Diff revision 1)
> +            args = {'email': email}
> +            r = assoc.create(**args)

might as well combine this into a single line, you can even just write it as `r = assoc.create(email=email)`

Comment 16

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/55046/#review52246

::: pylib/mozreview/mozreview/views.py:238
(Diff revision 1)
>      response = HttpResponseRedirect(redirect)
>      response.delete_cookie('bmo_auth_secret')
>      return response
> +
> +
> +def associate_employee_ldap(user):

Should we maybe just check to see if their profile is already associated with an ldap email and not bother querying if that's the case?

::: pylib/mozreview/mozreview/views.py:239
(Diff revision 1)
> +    """Try to link the authenticated user with their Mozilla employee LDAP
> +    account (if one exists)."""

single line summary.

Comment 17

[:glob ✱]

https://reviewboard.mozilla.org/r/55042/#review51880

> I wonder if we should make the `mozreview_profile.ldap_username == ''` case distinct from the non empty, but different, `mozreview_profile.ldap_username` case... I'm not aware of employees having multiple ldap accounts, is that a thing?
> 
> In the case where they don't match it might be worth logging and investigating, it could indicate someone has associated with another employees account.
> 
> Doing this probably wouldn't actually matter much in practice, but it'd be easy to just switch over. If you feel strongly against this though feel free to drop.

employees only have one ldap account under o=com,dc=mozilla.
if an employee has someone else's bmo email in their bugzillaEmail field i think this will result in more than one ldap entry matching that account so the association won't happen.
i'll fix up the other issues and revisit this once they have been cleared.

Comment 18

[:glob ✱]

https://reviewboard.mozilla.org/r/55042/#review51880

> This is going to be *a lot* of queries to ldap... maybe we should instead join with the `MozReviewUserProfile`s and only attempt to associate users who have an empty association?

we need to update more than just users who have an empty association; we need to update existing associations if, for example, someone changes their BMO login.
but you're right, it should still work if we iterate all users with MozReviewUserProfiles instead.

Comment 19

[:glob ✱]

https://reviewboard.mozilla.org/r/55046/#review52246

> Should we maybe just check to see if their profile is already associated with an ldap email and not bother querying if that's the case?

we need to support updating existing entries.

Comment 20

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/55046/#review52246

> we need to support updating existing entries.

Do we though? do ldap emails really ever change? if their BMO email changes that's fine, it should be the same account still right? or is it common for employees to abandon BMO accounts for a new one with a different email?

Comment 21

[:glob ✱]

https://reviewboard.mozilla.org/r/55046/#review52246

> Do we though? do ldap emails really ever change? if their BMO email changes that's fine, it should be the same account still right? or is it common for employees to abandon BMO accounts for a new one with a different email?

i have seen employees abandon bmo accounts (especially when they switch to/from persona due to its automatic account creation shenanigans).  thankfully it's an infrequent occurrence.

during login i think we should always check the association even if there's a current value.  it's quick (especially in comparison to the other steps required during login), and it's a safety net should something go wrong with the cron job/something else.

Comment 22

[:glob ✱]

https://reviewboard.mozilla.org/r/55042/#review51880

> employees only have one ldap account under o=com,dc=mozilla.
> if an employee has someone else's bmo email in their bugzillaEmail field i think this will result in more than one ldap entry matching that account so the association won't happen.
> i'll fix up the other issues and revisit this once they have been cleared.

i'll add code to log when we replace an existing ldap association.

Comment 23

[:glob ✱]

Comment on attachment 8756218 [details]
mozreview: add ldap function for finding matching employees (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55040/diff/1-2/

Comment 24

[:glob ✱]

Comment on attachment 8756219 [details]
mozreview: add rest resource to automatically associate employees with ldap (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55042/diff/1-2/

Comment 25

[:glob ✱]

Comment on attachment 8756220 [details]
mozreview: add mach command to trigger automatic employee ldap association (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55044/diff/1-2/

Comment 26

[:glob ✱]

Comment on attachment 8756221 [details]
mozreview: perform automatic employee ldap associations on login (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55046/diff/1-2/

Comment 27

[:glob ✱]

Comment on attachment 8756222 [details]
mozreview: add test for automatic employee ldap (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55048/diff/1-2/

Comment 28

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/55040/#review53254

Comment 29

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/55042/#review53252

This is mostly good. I'm holding off on r+ because given the security implications of user management code, I only want to grant r+ to what lands.

::: pylib/mozreview/mozreview/ldap/resources.py:182
(Diff revision 2)
> +    If a Review Board user's email address is provided via the --email
> +    field, then only that user will be updated.  Without an --email

`--email`? This isn't a CLI :)

::: pylib/mozreview/mozreview/ldap/resources.py:219
(Diff revision 2)
> +        self.ldap_connection = get_ldap_connection()
> +        if not self.ldap_connection:

You shouldn't need to assign the connection to `self`. By doing so, this potentially increases the scope of the connection and leaves a number of LDAP connections open longer than they should be. Although this may not be a concern if ReviewBoard destroys the class instance after every HTTP request. Still, it doesn't need to be an instance variable.

::: pylib/mozreview/mozreview/models.py:89
(Diff revision 2)
> +
> +
> +def associate_employee_ldap(user, ldap_connection=None):
> +    """Assoicate the user if we find them in the employee LDAP scope.
> +
> +    For the given email address, find a matching Mozilla employee's

Nit: Argument is a User instance, not an email address.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8756220 [details]
mozreview: add mach command to trigger automatic employee ldap association (bug 1272580);

https://reviewboard.mozilla.org/r/55044/#review53258

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8756221 [details]
mozreview: perform automatic employee ldap associations on login (bug 1272580);

https://reviewboard.mozilla.org/r/55046/#review53262

Mostly good. Holding off on r+ because I want to only r+ the final code.

::: pylib/mozreview/mozreview/views.py:232
(Diff revision 2)
> +        if updated:
> +            logging.info('Associating user: %s with ldap_username: %s'
> +                         % (user.email, ldap_username))

This logging is redundant with the logging in `associate_employee_ldap()`, no?

Comment 32

[Gregory Szorc [:gps]]

Comment on attachment 8756222 [details]
mozreview: add test for automatic employee ldap (bug 1272580);

https://reviewboard.mozilla.org/r/55048/#review53266

Looks mostly good!

::: hgext/reviewboard/tests/test-ldap-association.t:306
(Diff revision 2)
> +  $ rbmanage dump-user conflict > /dev/null
> +
> +Trigger automatic association
> +
> +  $ rbmanage associate-employee-ldap --email [e-mail]
> +  LDAP association failed.

If you feel inclined, you could cat the last N lines of ReviewBoard's logs to ensure logging looks sane. Given the importance of having an audit trail for security foo, this feels like something worth testing.

I'll leave it up to you though.

Comment 33

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/55048/#review53268

Sorry to scope bloat, but there should be a test for changing LDAP association (as opposed to mearly creating it).

Comment 34

[:glob ✱]

https://reviewboard.mozilla.org/r/55046/#review53262

> This logging is redundant with the logging in `associate_employee_ldap()`, no?

no, but it should be.  i'll move logging when an update is made into associate_employee_ldap().

Comment 35

[:glob ✱]

https://reviewboard.mozilla.org/r/55048/#review53266

> If you feel inclined, you could cat the last N lines of ReviewBoard's logs to ensure logging looks sane. Given the importance of having an audit trail for security foo, this feels like something worth testing.
> 
> I'll leave it up to you though.

i felt inclined.

Comment 36

[:glob ✱]

https://reviewboard.mozilla.org/r/55048/#review53268

doesn't sound like scope bloat; working in it.

Comment 37

[:glob ✱]

Comment on attachment 8756218 [details]
mozreview: add ldap function for finding matching employees (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55040/diff/2-3/

Comment 38

[:glob ✱]

Comment on attachment 8756219 [details]
mozreview: add rest resource to automatically associate employees with ldap (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55042/diff/2-3/

Comment 39

[:glob ✱]

Comment on attachment 8756220 [details]
mozreview: add mach command to trigger automatic employee ldap association (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55044/diff/2-3/

Comment 40

[:glob ✱]

Comment on attachment 8756221 [details]
mozreview: perform automatic employee ldap associations on login (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55046/diff/2-3/

Comment 41

[:glob ✱]

Comment on attachment 8756222 [details]
mozreview: add test for automatic employee ldap (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55048/diff/2-3/

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 8756221 [details]
mozreview: perform automatic employee ldap associations on login (bug 1272580);

https://reviewboard.mozilla.org/r/55046/#review54406

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 8756222 [details]
mozreview: add test for automatic employee ldap (bug 1272580);

https://reviewboard.mozilla.org/r/55048/#review54410

Comment 44

[Steven MacLeod [:smacleod]]

Comment on attachment 8756219 [details]
mozreview: add rest resource to automatically associate employees with ldap (bug 1272580);

https://reviewboard.mozilla.org/r/55042/#review55952

::: pylib/mozreview/mozreview/ldap/resources.py:244
(Diff revision 3)
> +            users = [profile.user
> +                     for profile in MozReviewUserProfile.objects.all()]

When you use djangos ORM managers to request objects, it can do intelligent things when you just iterate over it. What you're doing here is forcing it to load everything into memory as a list of all the objects. I'm not 100% on whether it will be a big deal performance wise here, but it's probably best to use `MozReviewUserProfile.objects.all()` as part of the actual for loop.

Also, since what you actually want is the user objects associated with them, I'm pretty sure there is a way to tell the ORM to fetch that as part of the original query, rather than having to do a new database query for every row you get.

::: pylib/mozreview/mozreview/models.py:86
(Diff revision 3)
>              ("enable_autoland",
>               "Can enable or disable autoland for a repository"),
>          )
> +
> +
> +def associate_employee_ldap(user, ldap_connection=None):

I'd prefer we stick this in the `ldap` module, rather than `models.py` - it keeps any code which actually makes changes to association contained in a single place.

Comment 45

[:glob ✱]

https://reviewboard.mozilla.org/r/55042/#review55952

> When you use djangos ORM managers to request objects, it can do intelligent things when you just iterate over it. What you're doing here is forcing it to load everything into memory as a list of all the objects. I'm not 100% on whether it will be a big deal performance wise here, but it's probably best to use `MozReviewUserProfile.objects.all()` as part of the actual for loop.
> 
> Also, since what you actually want is the user objects associated with them, I'm pretty sure there is a way to tell the ORM to fetch that as part of the original query, rather than having to do a new database query for every row you get.

changed it to fetching an array of user-ids, then creating the user objects within the loop.  as this is for the part of the code what will be cronned, performance isn't a great concern; and for the single-user use-case, it's highly likely the data will still be in a cache.

Comment 46

[:glob ✱]

Comment on attachment 8756218 [details]
mozreview: add ldap function for finding matching employees (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55040/diff/3-4/

Comment 47

[:glob ✱]

Comment on attachment 8756219 [details]
mozreview: add rest resource to automatically associate employees with ldap (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55042/diff/3-4/

Comment 48

[:glob ✱]

Comment on attachment 8756220 [details]
mozreview: add mach command to trigger automatic employee ldap association (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55044/diff/3-4/

Comment 49

[:glob ✱]

Comment on attachment 8756221 [details]
mozreview: perform automatic employee ldap associations on login (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55046/diff/3-4/

Comment 50

[:glob ✱]

Comment on attachment 8756222 [details]
mozreview: add test for automatic employee ldap (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55048/diff/3-4/

Comment 51

[Steven MacLeod [:smacleod]]

Comment on attachment 8756219 [details]
mozreview: add rest resource to automatically associate employees with ldap (bug 1272580);

https://reviewboard.mozilla.org/r/55042/#review59182

::: pylib/mozreview/mozreview/ldap/resources.py:247
(Diff revision 4)
> +        for user_id in user_ids:
> +            try:
> +                user = User.objects.get(id=user_id)

```Python
for user in User.objects.filter(id__in=user_ids):
   ...
```
Will fetch all of the users in a single query rather than a new database query for each user object.

Not a huge deal, but might speed things up a little... although in this case we might hit the limit for query size or `IN` filtering...

Another option is
```Python
# email specified case:
user_ids = User.objects.filter(email=email).values_list('id')
# ...

# email not specified case:
user_ids = MozReviewUserProfile.objects.values_list('user_id')
# ...

for user in User.objects.filter(id__in=user_ids):
    # ...
```

Which will fetch the ids as a subquery avoiding the size limit (although I think MySQL isn't the fastest at subqueries).

This probably won't matter, but I might as well bring it up in case you were interested in some ways to optimize the database calls using the ORM and not custom SQL. Feel free to swap to one of these solutions or drop this issue.

Comment 52

[:glob ✱]

https://reviewboard.mozilla.org/r/55042/#review59182

> ```Python
> for user in User.objects.filter(id__in=user_ids):
>    ...
> ```
> Will fetch all of the users in a single query rather than a new database query for each user object.
> 
> Not a huge deal, but might speed things up a little... although in this case we might hit the limit for query size or `IN` filtering...
> 
> Another option is
> ```Python
> # email specified case:
> user_ids = User.objects.filter(email=email).values_list('id')
> # ...
> 
> # email not specified case:
> user_ids = MozReviewUserProfile.objects.values_list('user_id')
> # ...
> 
> for user in User.objects.filter(id__in=user_ids):
>     # ...
> ```
> 
> Which will fetch the ids as a subquery avoiding the size limit (although I think MySQL isn't the fastest at subqueries).
> 
> This probably won't matter, but I might as well bring it up in case you were interested in some ways to optimize the database calls using the ORM and not custom SQL. Feel free to swap to one of these solutions or drop this issue.

> for user in User.objects.filter(id__in=user_ids):
> Will fetch all of the users in a single query rather than a new database query for each user object.

it will, but that's something that i want to avoid here.

as this is for the part of the code what will be cronned, performance isn't a great concern, instead i think it's better to optimise around reducing the memory footprint.

Comment 53

[:glob ✱]

Comment on attachment 8756218 [details]
mozreview: add ldap function for finding matching employees (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55040/diff/4-5/

Comment 54

[:glob ✱]

Comment on attachment 8756219 [details]
mozreview: add rest resource to automatically associate employees with ldap (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55042/diff/4-5/

Comment 55

[:glob ✱]

Comment on attachment 8756220 [details]
mozreview: add mach command to trigger automatic employee ldap association (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55044/diff/4-5/

Comment 56

[:glob ✱]

Comment on attachment 8756221 [details]
mozreview: perform automatic employee ldap associations on login (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55046/diff/4-5/

Comment 57

[:glob ✱]

Comment on attachment 8756222 [details]
mozreview: add test for automatic employee ldap (bug 1272580);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55048/diff/4-5/

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/d73f4a6f5a83
mozreview: add ldap function for finding matching employees ; r=gps,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/2eb3632222c1
mozreview: add rest resource to automatically associate employees with ldap ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/8f7b99dde772
mozreview: add mach command to trigger automatic employee ldap association ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/be3241810b5d
mozreview: perform automatic employee ldap associations on login ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/90913970a704
mozreview: add test for automatic employee ldap ; r=gps