Closed
Bug 1160517
Opened 9 years ago
Closed 9 years ago
Mozreview needs to associate users with ldap accounts
Categories
(MozReview Graveyard :: General, defect, P1)
MozReview Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: dminor, Assigned: smacleod)
References
Details
Attachments
(8 files)
40 bytes,
text/x-review-board-request
|
Details | |
40 bytes,
text/x-review-board-request
|
Details | |
40 bytes,
text/x-review-board-request
|
Details | |
40 bytes,
text/x-review-board-request
|
Details | |
40 bytes,
text/x-review-board-request
|
Details | |
40 bytes,
text/x-review-board-request
|
dminor
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
Details | |
40 bytes,
text/x-review-board-request
|
Details |
Right now we can safely assume that all Mozreview users have L1 group membership as that is required to push to the review repository. Once we start importing github requests this will no longer be the case and we need to gate some functionality (e.g. autolanding) on being a member of the appropriate group. Autoland can check this as well, but from usability point of view it would be nice to disable functionality rather than have it fail later on. Bug 1066207 is an example of request for credentials to be able to do this check. This ability is typically limited to servers running within the internal network or to a specific ip address if external.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
mozreview: Create model for storing MozReview user information (Bug 1160517). r?mcote Having a MozReviewUserProfile models allows MozReview to tie extra information to a Review Board user. Before this commit that has been done using a separate table for the specific information (such as the BugzillaUserMap). In order to support querying ldap for user permissions an ldap username must be stored for each Review Board user. Rather than make a specific table for this, introduce a generic MozReviewUserProfile and store the information there.
Attachment #8621119 -
Flags: review?(mcote)
Assignee | ||
Comment 2•9 years ago
|
||
mozreview: Add middleware to attach MozReviewUserProfile to request (Bug 1160517). r?mcote Rather than have many parts of the codebase making database queries to retrieve the MozReviewUserProfile for the requesting user, middleware will make the query once and attach it to the request object.
Attachment #8621120 -
Flags: review?(mcote)
Assignee | ||
Comment 3•9 years ago
|
||
mozreview: Add resource for retrieving and modifying ldap associations (Bug 1160517). r?mcote r?mdoglio The mercurial server requires a way to update a users ldap association when they push to the review server. Introduce new API endpoints for updating and querying a users ldap username. Users may only view their own association, but not modify it, through this API - unless they have the mozreview.modify_ldap_association permission, which allows viewing and modifying the ldap username of every account. This permission should only be given to a special account used by the review mercurial server.
Attachment #8621121 -
Flags: review?(mdoglio)
Attachment #8621121 -
Flags: review?(mcote)
Attachment #8621121 -
Flags: review?(dminor)
Assignee | ||
Comment 4•9 years ago
|
||
mozreview: Add user for hg server to talk to Review Board (Bug 1160517). r?gps r?dminor The hg review server needs a special user account for associating ldap usernames through the Web API. Automatically create this account with the needed permissions and tell the hg server about it.
Attachment #8621122 -
Flags: review?(gps)
Attachment #8621122 -
Flags: review?(dminor)
Assignee | ||
Comment 5•9 years ago
|
||
mozreview: Add mach command for dumping ldap association (Bug 1160517). r?dminor In order to test how interactions influence a users ldap association we need a way to query it. Add a command to print the ldap username associated with a MozReview user. We always use a hardcoded username/password when querying for ldap credentials as only admins or users with a special permission may retrieve ldap usernames for users besides themselves.
Attachment #8621123 -
Flags: review?(dminor)
Assignee | ||
Comment 6•9 years ago
|
||
mozreview: Add function to update ldap association (Bug 1160517). r?gps r?dminor Add `associate_ldap_username` function to pushhooks.py which will update a users associated ldap_username through the web API. This is a new function rather than a modification to post_reviews so that we could theoretically introduce a new wireprotocommand dedicated to associating your ldap account (which could be used by non hg developers, rather than having to push actual code). To start we'll probably just use it when submitting a review.
Attachment #8621124 -
Flags: review?(gps)
Attachment #8621124 -
Flags: review?(dminor)
Assignee | ||
Comment 7•9 years ago
|
||
mozreview: Update ldap username when posting to Review Board (Bug 1160517). r?gps r?dminor Since the review server is being pushed to over ssh authenticated with ldap, we can trust that the USER environment variable contains the pushers ldap username (i.e. they have proven control of that username). Associate this ldap username with the pushers Review Board account.
Attachment #8621125 -
Flags: review?(gps)
Attachment #8621125 -
Flags: review?(dminor)
Assignee | ||
Comment 8•9 years ago
|
||
mozreview: Test ldap association (Bug 1160517). r?gps Test that pushing properly associates an ldap username with the provided MozReview account.
Attachment #8621126 -
Flags: review?(gps)
Comment 9•9 years ago
|
||
Comment on attachment 8621122 [details] MozReview Request: mozreview: Add user for hg server to talk to Review Board (Bug 1160517). r=gps r=dminor https://reviewboard.mozilla.org/r/10891/#review9515 LGTM. ::: hgext/reviewboard/server.py:218 (Diff revision 1) > + if not ui.config('reviewboard', 'username', None): > + raise util.Abort(_('Please set reviewboard.username to the username ' > + 'for priveleged communications with Review Board.')) > + > + if not ui.config('reviewboard', 'password', None): > + raise util.Abort(_('Please set reviewboard.password to the password ' > + 'for priveleged communications with Review Board.')) We don't yet have reviewboard-hg1.dmz.scl3.mozilla.com fully under Ansible control. So we need to add the username and password manually before we deploy this or else things will blow up. Something to keep in mind.
Attachment #8621122 -
Flags: review?(gps) → review+
Comment 10•9 years ago
|
||
Comment on attachment 8621123 [details] MozReview Request: mozreview: Add mach command for dumping ldap association (Bug 1160517). r=gps r=dminor https://reviewboard.mozilla.org/r/10893/#review9517 ::: testing/vcttesting/reviewboard/mach_commands.py:498 (Diff revision 1) > + @Command('dump-user-ldap', category='reviewboard', > + description='Print the ldap username of a Review Board user.') I wonder if we shouldn't have a generic "dump user" command instead.
Attachment #8621123 -
Flags: review+
Reporter | ||
Updated•9 years ago
|
Attachment #8621123 -
Flags: review?(dminor)
Reporter | ||
Comment 11•9 years ago
|
||
Comment on attachment 8621123 [details] MozReview Request: mozreview: Add mach command for dumping ldap association (Bug 1160517). r=gps r=dminor https://reviewboard.mozilla.org/r/10893/#review9521 ::: testing/vcttesting/reviewboard/mach_commands.py:510 (Diff revision 1) > + print e nit: please use print(e) Most of the other commands use: print('API Error: %s: %s: %s' % (e.http_status, e.error_code, 615 e.rsp['err']['msg'])) It makes more sense to just use print(e) but for consistency you might want to follow that format and then we can do a mass fix later.
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8621124 [details] MozReview Request: mozreview: Add function to update ldap association (Bug 1160517). r=gps r?dminor https://reviewboard.mozilla.org/r/10895/#review9531 ::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:446 (Diff revision 1) > + (p_username/p_password and username/passowrd/cookie/userid password ::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:457 (Diff revision 1) > + if not any([username, password, userid, cookie]): if not any([username, password]) or not any([userid, cookie]): ::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:430 (Diff revision 1) > +def associate_ldap_username(url, ldap_username, p_username, p_password, I know you want to keep things short, but maybe use ldap_admin_username and ldap_admin_password?
Attachment #8621124 -
Flags: review?(dminor)
Assignee | ||
Comment 13•9 years ago
|
||
https://reviewboard.mozilla.org/r/10893/#review9535 > I wonder if we shouldn't have a generic "dump user" command instead. We do, I'm probably being lazy and not wanting to deal with all the changed tests...
Comment 14•9 years ago
|
||
Comment on attachment 8621124 [details] MozReview Request: mozreview: Add function to update ldap association (Bug 1160517). r=gps r?dminor https://reviewboard.mozilla.org/r/10895/#review9537 LGTM. ::: pylib/reviewboardmods/reviewboardmods/pushhooks.py:484 (Diff revision 1) > + return False Do we not want to log on error?
Attachment #8621124 -
Flags: review?(gps) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8621125 [details] MozReview Request: mozreview: Update ldap username when posting to Review Board (Bug 1160517). r=gps r=dminor https://reviewboard.mozilla.org/r/10897/#review9539 Ship It!
Attachment #8621125 -
Flags: review?(gps) → review+
Comment 16•9 years ago
|
||
Comment on attachment 8621126 [details] MozReview Request: mozreview: Test ldap association (Bug 1160517). r=gps https://reviewboard.mozilla.org/r/10899/#review9541 Ship It!
Attachment #8621126 -
Flags: review?(gps) → review+
Reporter | ||
Comment 17•9 years ago
|
||
Comment on attachment 8621125 [details] MozReview Request: mozreview: Update ldap username when posting to Review Board (Bug 1160517). r=gps r=dminor https://reviewboard.mozilla.org/r/10897/#review9543 ::: hgext/reviewboard/hgrb/proto.py:274 (Diff revision 1) > + ldap_username = os.environ.get('USER') This seems clunky, can we use repo.ui.username() here?
Attachment #8621125 -
Flags: review?(dminor)
Reporter | ||
Comment 18•9 years ago
|
||
Comment on attachment 8621121 [details] MozReview Request: mozreview: Add resource for retrieving and modifying ldap associations (Bug 1160517). r=dminor r=mcote r=mdoglio https://reviewboard.mozilla.org/r/10889/#review9617 Ship It!
Attachment #8621121 -
Flags: review?(dminor) → review+
Reporter | ||
Comment 19•9 years ago
|
||
Comment on attachment 8621122 [details] MozReview Request: mozreview: Add user for hg server to talk to Review Board (Bug 1160517). r=gps r=dminor https://reviewboard.mozilla.org/r/10891/#review9619 Ship It!
Attachment #8621122 -
Flags: review?(dminor) → review+
Comment 20•9 years ago
|
||
Comment on attachment 8621119 [details] MozReview Request: mozreview: Create model for storing MozReview user information (Bug 1160517). r=mcote https://reviewboard.mozilla.org/r/10885/#review9651 Just curious--you didn't think we should put this in its own directory, like the other models are? Not a big deal though, so r+ing anyway.
Attachment #8621119 -
Flags: review?(mcote) → review+
Comment 21•9 years ago
|
||
Comment on attachment 8621120 [details] MozReview Request: mozreview: Add middleware to attach MozReviewUserProfile to request (Bug 1160517). r=mcote https://reviewboard.mozilla.org/r/10887/#review9653 ::: pylib/mozreview/mozreview/middleware.py:1 (Diff revision 1) > +from mozreview.models import get_profile, MozReviewUserProfile Kind of weird to have get_profile in a models module, no?
Attachment #8621120 -
Flags: review?(mcote)
Assignee | ||
Comment 22•9 years ago
|
||
https://reviewboard.mozilla.org/r/10887/#review9655 > Kind of weird to have get_profile in a models module, no? I wasn't too boethered by it. Have any suggestions?
Comment 23•9 years ago
|
||
https://reviewboard.mozilla.org/r/10887/#review9707 > I wasn't too boethered by it. Have any suggestions? Just following the other things there (autoland, bugzilla), maybe move the new model and get_profile to separate files in a profiles directory or something? Not a big deal, though, just feels more consistent.
Comment 24•9 years ago
|
||
Comment on attachment 8621121 [details] MozReview Request: mozreview: Add resource for retrieving and modifying ldap associations (Bug 1160517). r=dminor r=mcote r=mdoglio https://reviewboard.mozilla.org/r/10889/#review9709 Ship It!
Attachment #8621121 -
Flags: review?(mcote) → review+
Comment 25•9 years ago
|
||
https://reviewboard.mozilla.org/r/10887/#review9747 > Just following the other things there (autoland, bugzilla), maybe move the new model and get_profile to separate files in a profiles directory or something? Not a big deal, though, just feels more consistent. Actually I just realized that we have get_or_create_bugzilla_users() in models as well. But still might want to consider moving the model + function to a file in a new dir, for consistency.
Comment 26•9 years ago
|
||
Comment on attachment 8621121 [details] MozReview Request: mozreview: Add resource for retrieving and modifying ldap associations (Bug 1160517). r=dminor r=mcote r=mdoglio https://reviewboard.mozilla.org/r/10889/#review9827 Ship It!
Attachment #8621121 -
Flags: review?(mdoglio) → review+
Comment 27•9 years ago
|
||
Comment on attachment 8621120 [details] MozReview Request: mozreview: Add middleware to attach MozReviewUserProfile to request (Bug 1160517). r=mcote https://reviewboard.mozilla.org/r/10887/#review10121 Ship It!
Attachment #8621120 -
Flags: review+
Assignee | ||
Comment 28•9 years ago
|
||
https://reviewboard.mozilla.org/r/10893/#review10123 > We do, I'm probably being lazy and not wanting to deal with all the changed tests... Actually, I'm going to leave it as two separate commands due to the special permissions around querying a users ldap username. We can think harder on this and figure it out later. I've filed Bug 1175994
Assignee | ||
Comment 29•9 years ago
|
||
https://reviewboard.mozilla.org/r/10893/#review10125 > nit: please use print(e) > > Most of the other commands use: print('API Error: %s: %s: %s' % (e.http_status, e.error_code, > 615 e.rsp['err']['msg'])) > > It makes more sense to just use print(e) but for consistency you might want to follow that format and then we can do a mass fix later. This shouldn't ever fail, so I'm just going to remove the exception catching (I was mostly using it during implementation to test various things).
Assignee | ||
Comment 30•9 years ago
|
||
https://reviewboard.mozilla.org/r/10895/#review10133 > Do we not want to log on error? I'm going to take care of this in a followup. I figure we can deploy the code which starts associating users silently, and then when we roll out github / restricting based on ldap we'll already have a bunch of users associated. Until the rollout I think logging the failure cases isn't as impotant. Filed Bug 1176009
Assignee | ||
Comment 31•9 years ago
|
||
https://reviewboard.mozilla.org/r/10897/#review10135 > This seems clunky, can we use repo.ui.username() here? `repo.ui.username()` comes from hgrc files, and it dictates the author of a commit, this env USER corresponds to the ldap username and is set by the ssh server after authenticating with the public key.
Assignee | ||
Comment 32•9 years ago
|
||
Comment on attachment 8621119 [details] MozReview Request: mozreview: Create model for storing MozReview user information (Bug 1160517). r=mcote mozreview: Create model for storing MozReview user information (Bug 1160517). r=mcote Having a MozReviewUserProfile models allows MozReview to tie extra information to a Review Board user. Before this commit that has been done using a separate table for the specific information (such as the BugzillaUserMap). In order to support querying ldap for user permissions an ldap username must be stored for each Review Board user. Rather than make a specific table for this, introduce a generic MozReviewUserProfile and store the information there.
Attachment #8621119 -
Attachment description: MozReview Request: mozreview: Create model for storing MozReview user information (Bug 1160517). r?mcote → MozReview Request: mozreview: Create model for storing MozReview user information (Bug 1160517). r=mcote
Attachment #8621119 -
Flags: review+
Assignee | ||
Comment 33•9 years ago
|
||
Comment on attachment 8621120 [details] MozReview Request: mozreview: Add middleware to attach MozReviewUserProfile to request (Bug 1160517). r=mcote mozreview: Add middleware to attach MozReviewUserProfile to request (Bug 1160517). r=mcote Rather than have many parts of the codebase making database queries to retrieve the MozReviewUserProfile for the requesting user, middleware will make the query once and attach it to the request object.
Attachment #8621120 -
Attachment description: MozReview Request: mozreview: Add middleware to attach MozReviewUserProfile to request (Bug 1160517). r?mcote → MozReview Request: mozreview: Add middleware to attach MozReviewUserProfile to request (Bug 1160517). r=mcote
Attachment #8621120 -
Flags: review+
Assignee | ||
Comment 34•9 years ago
|
||
Comment on attachment 8621121 [details] MozReview Request: mozreview: Add resource for retrieving and modifying ldap associations (Bug 1160517). r=dminor r=mcote r=mdoglio mozreview: Add resource for retrieving and modifying ldap associations (Bug 1160517). r=dminor r=mcote r=mdoglio The mercurial server requires a way to update a users ldap association when they push to the review server. Introduce new API endpoints for updating and querying a users ldap username. Users may only view their own association, but not modify it, through this API - unless they have the mozreview.modify_ldap_association permission, which allows viewing and modifying the ldap username of every account. This permission should only be given to a special account used by the review mercurial server.
Attachment #8621121 -
Attachment description: MozReview Request: mozreview: Add resource for retrieving and modifying ldap associations (Bug 1160517). r?mcote r?mdoglio → MozReview Request: mozreview: Add resource for retrieving and modifying ldap associations (Bug 1160517). r=dminor r=mcote r=mdoglio
Attachment #8621121 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8621122 -
Attachment description: MozReview Request: mozreview: Add user for hg server to talk to Review Board (Bug 1160517). r?gps r?dminor → MozReview Request: mozreview: Add user for hg server to talk to Review Board (Bug 1160517). r=gps r=dminor
Attachment #8621122 -
Flags: review+
Assignee | ||
Comment 35•9 years ago
|
||
Comment on attachment 8621122 [details] MozReview Request: mozreview: Add user for hg server to talk to Review Board (Bug 1160517). r=gps r=dminor mozreview: Add user for hg server to talk to Review Board (Bug 1160517). r=gps r=dminor The hg review server needs a special user account for associating ldap usernames through the Web API. Automatically create this account with the needed permissions and tell the hg server about it.
Assignee | ||
Comment 36•9 years ago
|
||
Comment on attachment 8621123 [details] MozReview Request: mozreview: Add mach command for dumping ldap association (Bug 1160517). r=gps r=dminor mozreview: Add mach command for dumping ldap association (Bug 1160517). r=gps r=dminor In order to test how interactions influence a users ldap association we need a way to query it. Add a command to print the ldap username associated with a MozReview user. We always use a hardcoded username/password when querying for ldap credentials as only admins or users with a special permission may retrieve ldap usernames for users besides themselves.
Attachment #8621123 -
Attachment description: MozReview Request: mozreview: Add mach command for dumping ldap association (Bug 1160517). r?dminor → MozReview Request: mozreview: Add mach command for dumping ldap association (Bug 1160517). r=gps r=dminor
Attachment #8621123 -
Flags: review+
Assignee | ||
Comment 37•9 years ago
|
||
Comment on attachment 8621124 [details] MozReview Request: mozreview: Add function to update ldap association (Bug 1160517). r=gps r?dminor mozreview: Add function to update ldap association (Bug 1160517). r=gps r?dminor Add `associate_ldap_username` function to pushhooks.py which will update a users associated ldap_username through the web API. This is a new function rather than a modification to post_reviews so that we could theoretically introduce a new wireprotocommand dedicated to associating your ldap account (which could be used by non hg developers, rather than having to push actual code). To start we'll probably just use it when submitting a review.
Attachment #8621124 -
Attachment description: MozReview Request: mozreview: Add function to update ldap association (Bug 1160517). r?gps r?dminor → MozReview Request: mozreview: Add function to update ldap association (Bug 1160517). r=gps r?dminor
Attachment #8621124 -
Flags: review+ → review?(dminor)
Assignee | ||
Comment 38•9 years ago
|
||
Comment on attachment 8621125 [details] MozReview Request: mozreview: Update ldap username when posting to Review Board (Bug 1160517). r=gps r=dminor mozreview: Update ldap username when posting to Review Board (Bug 1160517). r=gps r=dminor Since the review server is being pushed to over ssh authenticated with ldap, we can trust that the USER environment variable contains the pushers ldap username (i.e. they have proven control of that username). Associate this ldap username with the pushers Review Board account.
Attachment #8621125 -
Attachment description: MozReview Request: mozreview: Update ldap username when posting to Review Board (Bug 1160517). r?gps r?dminor → MozReview Request: mozreview: Update ldap username when posting to Review Board (Bug 1160517). r=gps r=dminor
Attachment #8621125 -
Flags: review+
Assignee | ||
Comment 39•9 years ago
|
||
Comment on attachment 8621126 [details] MozReview Request: mozreview: Test ldap association (Bug 1160517). r=gps mozreview: Test ldap association (Bug 1160517). r=gps Test that pushing properly associates an ldap username with the provided MozReview account.
Attachment #8621126 -
Attachment description: MozReview Request: mozreview: Test ldap association (Bug 1160517). r?gps → MozReview Request: mozreview: Test ldap association (Bug 1160517). r=gps
Attachment #8621126 -
Flags: review+
Reporter | ||
Comment 40•9 years ago
|
||
Comment on attachment 8621124 [details] MozReview Request: mozreview: Add function to update ldap association (Bug 1160517). r=gps r?dminor https://reviewboard.mozilla.org/r/10895/#review10141 Ship It!
Attachment #8621124 -
Flags: review?(dminor) → review+
Assignee | ||
Comment 41•9 years ago
|
||
Morphing so we can land this stuff and start gathering ldap accounts before we require their use. I'll post the querying code to Bug 1160520
Summary: Mozreview needs to be able check LDAP group membership → Mozreview needs to associate users with ldap accounts
Assignee | ||
Comment 42•9 years ago
|
||
https://hg.mozilla.org/hgcustom/version-control-tools/pushloghtml?changeset=83cee78bea9b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Product: Developer Services → MozReview
You need to log in
before you can comment on or make changes to this bug.
Description
•