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)
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.
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)
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/
Review commit: https://reviewboard.mozilla.org/r/66404/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66404/
Comment hidden (metoo) |
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
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•8 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 11•8 years ago
|
||
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•8 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•8 years ago
|
||
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•8 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•8 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•8 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/
Updated•8 years ago
|
Attachment #8775452 -
Flags: review?(smacleod) → review+
Comment 17•8 years ago
|
||
Comment on attachment 8775452 [details] ldap: Add a 'user_exists' method to ldap library (bug 1287988) https://reviewboard.mozilla.org/r/67638/#review64864
Comment 18•8 years ago
|
||
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•8 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•8 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
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•