automatic ldap association shouldn't replace existing valid associations

RESOLVED FIXED

Status

MozReview
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

Production

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

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

Updated

2 years ago
Assignee: nobody → glob
(Assignee)

Comment 1

2 years ago
Created attachment 8773644 [details]
mozreview: Allow passing an existing ldap connection to query_scm_group (bug 1287988)

Review commit: https://reviewboard.mozilla.org/r/66400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66400/
Attachment #8773644 - Flags: review?(smacleod)
Attachment #8773645 - Flags: review?(smacleod)
Attachment #8773646 - Flags: review?(smacleod)
(Assignee)

Comment 2

2 years ago
Created attachment 8773645 [details]
mozreview: Don't overwrite valid ldap associations (bug 1287988)

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/
(Assignee)

Comment 3

2 years ago
Created attachment 8773646 [details]
mozreview: add test to ensure we don't overwrite existing ldap associations (bug 1287988)

Review commit: https://reviewboard.mozilla.org/r/66404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66404/
Comment hidden (metoo)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1288998
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.
(Assignee)

Comment 10

2 years ago
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+
(Assignee)

Comment 12

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

Comment 13

2 years ago
Created attachment 8775452 [details]
ldap: Add a 'user_exists' method to ldap library (bug 1287988)

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+
(Assignee)

Comment 14

2 years ago
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/
(Assignee)

Comment 15

2 years ago
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/
(Assignee)

Comment 16

2 years ago
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/#review64864
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+
(Assignee)

Comment 19

2 years ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1288989
You need to log in before you can comment on or make changes to this bug.