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)

defect

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.
Blocks: 1160520
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
Depends on: 1168476
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)
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)
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)
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)
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)
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)
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)
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 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 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+
Attachment #8621123 - Flags: review?(dminor)
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.
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)
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 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 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 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+
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)
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+
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 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 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)
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?
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 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+
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 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 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+
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
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).
Blocks: 1176008
Blocks: 1175994
Blocks: 1176009
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
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.
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+
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+
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+
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+
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.
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+
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)
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+
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+
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+
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
https://hg.mozilla.org/hgcustom/version-control-tools/pushloghtml?changeset=83cee78bea9b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: