Closed Bug 1287988 Opened 8 years ago Closed 8 years ago

automatic ldap association shouldn't replace existing valid associations

Categories

(MozReview Graveyard :: General, defect)

Production
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glob, Assigned: glob)

References

Details

Attachments

(4 files)

markh uses a different email address for his scm3 account than the one he uses on bugzilla.

every time he logs into review board the association is updated to his moco ldap address and he loses his scm3 permissions in review board.


i suspect the fix for this is to update ldap.associate_employee_ldap() so it doesn't replace an existing association if the current value in mozilla_profile is found in the scm3 ldap group.
Assignee: nobody → glob
Ensure that if a user's ldap association is correct, we don't overwrite the
stored link.

This resolves a bug impacting Mozilla employees who use a different email
address in LDAP vs BMO.

Review commit: https://reviewboard.mozilla.org/r/66402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66402/
Comment on attachment 8773644 [details]
mozreview: Allow passing an existing ldap connection to query_scm_group (bug 1287988)

https://reviewboard.mozilla.org/r/66400/#review64234

::: pylib/mozreview/mozreview/ldap/__init__.py:55
(Diff revision 1)
> -    if not l:
> +    if not ldap_connection:
> +        ldap_connection = get_ldap_connection()
> +        if not ldap_connection:
> -        return False
> +            return False

```Python
ldap_connection = ldap_connection or get_ldap_connection()
if not ldap_connection:
    return False
```
Attachment #8773644 - Flags: review?(smacleod) → review+
Comment on attachment 8773645 [details]
mozreview: Don't overwrite valid ldap associations (bug 1287988)

https://reviewboard.mozilla.org/r/66402/#review64260

::: pylib/mozreview/mozreview/ldap/__init__.py:121
(Diff revision 1)
>      from mozreview.models import get_profile
> +    mozreview_profile = get_profile(user)
> +
> +    # Don't overwrite existing and valid associations.
> +    if mozreview_profile.ldap_username and query_scm_group(
> +            mozreview_profile.ldap_username, 'scm_level_1', ldap_connection):

I'm curious why we only keep the current one if it has scm_level_1? I don't *think* this will cause any problems, but it seems safer to me to just not overwrite.

We do have the manual association command if someone really needs to overwrite an old association for some reason.

If we never overwrite, it'll save us another code update if we've overlooked some case here.
Attachment #8773645 - Flags: review?(smacleod) → review+
Comment on attachment 8773646 [details]
mozreview: add test to ensure we don't overwrite existing ldap associations (bug 1287988)

https://reviewboard.mozilla.org/r/66404/#review64262

::: hgext/reviewboard/tests/test-ldap-association.t:349
(Diff revision 1)
> +  $ mozreview exec rbweb grep -i ldap /reviewboard/logs/reviewboard.log | tail -n 1
> +  ????-??-?? ??:??:??,??? - INFO -  - Associating user: user7@example.com already associated with ldap_username: uSeven@example.org (glob)

heh :)
Attachment #8773646 - Flags: review?(smacleod) → review+
https://reviewboard.mozilla.org/r/66402/#review64260

> I'm curious why we only keep the current one if it has scm_level_1? I don't *think* this will cause any problems, but it seems safer to me to just not overwrite.
> 
> We do have the manual association command if someone really needs to overwrite an old association for some reason.
> 
> If we never overwrite, it'll save us another code update if we've overlooked some case here.

Just to be clear, I do not consider this issue to be a landing blocker. Feel free to drop if you are not convinced.
https://reviewboard.mozilla.org/r/66402/#review64260

> Just to be clear, I do not consider this issue to be a landing blocker. Feel free to drop if you are not convinced.

i want to keep the current association only if it's valid, so there's instances where i think we need to overwrite an existing but invalid entry.

i'll see how easy it is to change it to an "exists" check, instead of a level check.
Comment on attachment 8773644 [details]
mozreview: Allow passing an existing ldap connection to query_scm_group (bug 1287988)

https://reviewboard.mozilla.org/r/66400/#review64642
Attachment #8773644 - Flags: review+
https://reviewboard.mozilla.org/r/66402/#review64260

> i want to keep the current association only if it's valid, so there's instances where i think we need to overwrite an existing but invalid entry.
> 
> i'll see how easy it is to change it to an "exists" check, instead of a level check.

i'll push a revision that just checks if a user exists, instead of for an scm level.  this means we need to perform up to three ldap queries instead of one.
This user_exists method checks for the user in o=com (Mozilla Corp), o=org
(Mozilla Foundation), and o=net (contributors).

Review commit: https://reviewboard.mozilla.org/r/67638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67638/
Attachment #8775452 - Flags: review?(smacleod)
Attachment #8775452 - Flags: review?(gps)
Attachment #8773644 - Flags: review+
Comment on attachment 8773644 [details]
mozreview: Allow passing an existing ldap connection to query_scm_group (bug 1287988)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66400/diff/1-2/
Comment on attachment 8773645 [details]
mozreview: Don't overwrite valid ldap associations (bug 1287988)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66402/diff/1-2/
Comment on attachment 8773646 [details]
mozreview: add test to ensure we don't overwrite existing ldap associations (bug 1287988)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66404/diff/1-2/
Attachment #8775452 - Flags: review?(smacleod) → review+
Comment on attachment 8775452 [details]
ldap: Add a 'user_exists' method to ldap library (bug 1287988)

https://reviewboard.mozilla.org/r/67638/#review65500

This looks good tech wise except for `return False`.

::: pylib/mozreview/mozreview/ldap/__init__.py:78
(Diff revision 1)
> +def user_exists(username, ldap_connection=None):
> +    """Returns true if a user exists in LDAP (MoCo, MoFo, or contrib)."""
> +
> +    ldap_connection = ldap_connection or get_ldap_connection()
> +    if not ldap_connection:
> +        return False

It's a bit weird to return False here. This could potentially lead to scenarios where LDAP being down results in bad things happening. Although I suppose the code that looks at this return value also performs an LDAP lookup, which will presumably also fail. Still, this feels dangerous in that connection failure could lead to unexpected results.

What are your thoughts?
Attachment #8775452 - Flags: review?(gps) → review+
https://reviewboard.mozilla.org/r/67638/#review65500

> It's a bit weird to return False here. This could potentially lead to scenarios where LDAP being down results in bad things happening. Although I suppose the code that looks at this return value also performs an LDAP lookup, which will presumably also fail. Still, this feels dangerous in that connection failure could lead to unexpected results.
> 
> What are your thoughts?

good point.  i'll raise an exception here instead.
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/79a0fa5d54c4
mozreview: Allow passing an existing ldap connection to query_scm_group r=gps,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/754adf9a6a85
ldap: Add a 'user_exists' method to ldap library r=gps,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/46d752dae29b
mozreview: Don't overwrite valid ldap associations r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/610a71840c81
mozreview: add test to ensure we don't overwrite existing ldap associations r=smacleod
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: