993233 - Use BMO API keys

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Mark Côté [:mcote]]

It would be nice to be able to log into Review Board via web API using a valid Bugzilla auth token instead of passing Bugzilla login info.  It isn't critical, though, as long as the transport is encrypted, just nice to have.

See http://www.reviewboard.org/docs/manual/dev/webapi/2.0/authenticating/ and https://github.com/djblets/djblets/blob/master/djblets/webapi/auth.py

Note that this presupposes REST support; the current implementation of rbbz uses XMLRPC, since native REST wasn't available when work started.

Comment 1

[Mark Côté [:mcote]]

This should probably wait until we have proper API keys in BMO, at some undetermined point in the future.

Comment 2

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: User-creation mach command creates API key (Bug 993233). r?smacleod

Comment 3

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Add api_key field to BugzillaUserMap (Bug 993233). r?smacleod

Comment 4

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: View for BMO auth delegation callback (Bug 993233). r?smacleod

Comment 5

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Authenticate to BMO with API key (Bug 993233). r?smacleod

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9883/#review8819

::: testing/vcttesting/mozreview_mach_commands.py:163
(Diff revision 1)
> -        print('Created user %s' % u['bugzilla']['id'])
> +        print('Created user %s with API key %s' % (u['bugzilla']['id'],
> +                                                   u['bugzilla']['api_key']))

Will this be deterministic? if not it's going to cause serious issues with the test output - Also, this should be changing the test output but you're not including the test updates in this commit - please make sure tests pass on every commit.

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9885/#review8823

Clearing review flag for now as this is just a WIP posting for my benefit.

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review8825

Clearing review flag for now as this is just a WIP posting for my benefit.

Comment 9

[Steven MacLeod [:smacleod]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9889/#review8827

Clearing review flag for now as this is just a WIP posting for my benefit.

Comment 11

[Mark Côté [:mcote]]

Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r?gps

Comment 12

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r?gps

Comment 13

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r?dminor

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.

Comment 14

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r?smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.

Comment 15

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add or update API key when getting or creating Bugzilla users (bug 993233). r?smacleod

Comment 16

[Mark Côté [:mcote]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Comment 17

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r?smacleod

Comment 18

[Mark Côté [:mcote]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

Note: we should probably not deploy this until bug 1175643 has landed in
BMO and the MozReview callback function is updated appropriately.  This will
be a minor change, though, so the bulk of this commit will still apply and
hence can be reviewed now.

Comment 19

[Mark Côté [:mcote]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: User-creation mach command creates API key (Bug 993233). r?dminor

Fix up tests to ensure API keys are created where needed.

Comment 20

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r?smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.

Comment 21

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Do not use Bugzilla cookies for anything other than verifying a login (bug 993233). r?smacleod

Comment 22

[Dan Minor [:dminor]]

Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

https://reviewboard.mozilla.org/r/13273/#review11977

This looks good, but I think the commit description could use a sentence or two on the motivation behind moving the code.

Comment 23

[Dan Minor [:dminor]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9883/#review12007

::: testing/vcttesting/mozreview.py:481
(Diff revision 2)
> +             email]).strip()

I think you should put an assert here to verify that the api key is what we expect to see so the failure is localized here if something goes wrong.

::: testing/vcttesting/mozreview.py:486
(Diff revision 2)
> +

You should assert that the return code here is a 200 as well.

::: testing/vcttesting/mozreview.py:477
(Diff revision 2)
> +    def create_user_api_key(self, email):

Add a comment or docstring explaining this has the side effect of logging the user in using the api key.

::: testing/vcttesting/mozreview.py:262
(Diff revision 2)
> +        self.create_user_api_key(bugzilla.username)

I think you should either add a self.admin_api_key to store the result of this or add a comment to the effect that this has the side effect of authenticating using the api key.

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9885/#review12009

We should at least document the setting of auth_bz_xmlrpc_url in ansible/roles/docker-rbweb/files/entrypoint.py because its existence after this patch only serves to confuse. Ideally, we would delete that assignment. Although, it may interfere with the activation of the MozReview extension. Best to double check things still work after removing the assignment!

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

https://reviewboard.mozilla.org/r/13271/#review12011

Ship It!

Comment 26

[Gregory Szorc [:gps]]

Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

https://reviewboard.mozilla.org/r/13273/#review12013

Ship It!

Comment 27

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/13275/#review12015

So we only support 1 API key per user? I'm not sure if you've had the RRA, but 1 API key per device per service would be the ideal security I would be aiming for. That way, if a device is compromised, you still have valid API keys on other devices. This assumes a compromised API key can't be used to obtain other API keys, however. I'm not sure how Bugzilla API keys work!

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13279/#review12017

Ship It!

Comment 29

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9883/#review12021

I really wish you would have split out the create-ldap-user -> create-user conversion to its own commit. This diff is a bit much to grok.

::: testing/vcttesting/mozreview.py:484
(Diff revision 2)
> +                     'mozreview/bmo_auth_callback/?client_api_login=%s'
> +                     '&client_api_key=%s' % (email, api_key))

Are API keys really sent to the server as part of the URL in the query string? That means they'll get logged on the server. Not cool.

Also, there is a way to pass query string or POST parameters to requests.get(). This will handle encoding, etc. Use it or face the consequences when URL unsafe characters start appearing in emails or API keys.

Comment 30

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/13283/#review12023

I'm pretty certain there are some docs references to cookies that should be removed.

Comment 31

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/13283/#review12025

Also, the reviewboard client extension will use cookies automatically if they are present. We need to update a lot of code on client and server to not use cookies. I can do this if you need help.

Comment 32

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/9883/#review12021

> Are API keys really sent to the server as part of the URL in the query string? That means they'll get logged on the server. Not cool.
> 
> Also, there is a way to pass query string or POST parameters to requests.get(). This will handle encoding, etc. Use it or face the consequences when URL unsafe characters start appearing in emails or API keys.

This is how they work right now, but I agree it's terrible, which is why we decided to do https://bugzilla.mozilla.org/show_bug.cgi?id=1175643, creating a one-time token used to fetch the API key.  I don't plan on landing API-key support in MozReview until that bug is fixed and deployed to BMO, but it should be a relatively small change from MozReview's perspective, so I wanted to start this review process now.

Comment 33

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/9883/#review12021

I can do that if you like.

Comment 34

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/13283/#review12025

So something that I've been meaning to clarify: this commit series is just about using API keys for Review Board -> BMO communication.  It doesn't touch * -> Review Board, e.g. pushing to the hg review repo, which continues to use Bugzilla password or cookie, just to identify with Review Board.  We definitely need to fix that as well, but this commit series is the first step and at least removes the storage of cookies from Review Board (and removes expired-session errors).

Comment 35

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/13275/#review12015

To build upon my earlier comment, this commit series doesn't implement the standard use of API keys, which, as I understand it, is a way for client A on a specific machine to authenticate/identify with service B without storing an authoritative password.  This commit series is just replacing the storage (in the Review Board database) of a temporary (read: inconvenient) and not easily revokable session token with an easily revokable and long-lived API key that has (slightly) restricted permissions (restricted to API usage only, which is a subset of actions that can be performed via the UI, to which a session cookie grants full access).  We plan on restricting access based on MozReview-specific API keys this quarter to build upon this layer (see bug 1184332).

Adding API keys to access Review Board itself is a separate, subsequent (and important) goal.  Please ping me if you'd like to discuss further.

Comment 36

[Steven MacLeod [:smacleod]]

Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

https://reviewboard.mozilla.org/r/13273/#review12077

Ship It!

Comment 37

[Steven MacLeod [:smacleod]]

Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13275/#review12079

::: pylib/mozreview/mozreview/bugzilla/models.py:25
(Diff revision 1)
> +    api_key = models.CharField(max_length=40, null=True)

I want to say that all new per user data goes in the MozReviewUserProfile table, but I'd be willing to let this slide for this field since it's directly tied to the bugzilla auth.

Could you add a comment to this model though indicating that it should not be updated going forward, and all new data should be put in the profile table?

Comment 38

[Steven MacLeod [:smacleod]]

Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13277/#review12081

::: pylib/mozreview/mozreview/bugzilla/models.py:35
(Diff revision 1)
> -def get_or_create_bugzilla_users(user_data):
> +def get_or_create_bugzilla_users(user_data, api_key=None):

This seems like quite the footgun (allowing multiple users, but giving them all the same API key). I could see a security bug slipping in quite easily if we mess up a call to this function.

I'd prefer we make assigning the api key a more explicit operation for only a single user.

Comment 39

[Steven MacLeod [:smacleod]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9889/#review12087

::: pylib/mozreview/mozreview/bugzilla/client.py:60
(Diff revision 2)
> -    def __init__(self, login=None, logincookie=None, xmlrpc_url=None):
> +    def __init__(self, user_id=None, xmlrpc_url=None):

Something I appreciated about this class, and used for some manual scripting / testing, was that I could import and use it outside the context of Review Board / MozReview.

By passing the user id and doing a db lookup we lose that functionality. Could you refactor this a bit so that it takes an API key instead of looking it up with the user id.

Comment 40

[Steven MacLeod [:smacleod]]

Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13279/#review12133

Ship It!

Comment 41

[Steven MacLeod [:smacleod]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review12135

::: pylib/mozreview/mozreview/views.py:17
(Diff revision 2)
> +    bmo_login = request.GET.get('client_api_login', None)

Can we call this `bmo_username` instead, I like it being more explicit so we don't have to lookup whether it is a user id or whatnot.

::: pylib/mozreview/mozreview/views.py:51
(Diff revision 2)
> +    if not user.is_active:
> +        logging.warning('Validated API key but user %s is inactive.' %
> +                        bmo_login)
> +        return HttpResponseBadRequest()

I'm not really sure what this flag is used for / does in the bmo sense, but would it be worth notifying the user they can't login because their account is inactive?

::: pylib/mozreview/mozreview/extension.py:116
(Diff revision 2)
> +        urlpatterns = patterns('',
> +                               url(r'^mozreview/', include('mozreview.urls')))
> +        URLHook(self, urlpatterns)

cut out the intermediate `urlpatterns` variable.

Comment 42

[Steven MacLeod [:smacleod]]

Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13281/#review12137

Please use "hg mv" / "hg cp" when renaming / copying files so that the diffs displayed will be intelligent. Right now mozreview.js looks like a giant add to me rather than a rename then not modified *or* modifies (I'd rather not have to inspect every line manually).

::: pylib/mozreview/mozreview/templatetags/bugzilla.py:21
(Diff revision 1)
> +    callback_uri = request.build_absolute_uri(reverse('bmo-auth-callback'))

do we need to encode this?

::: pylib/mozreview/mozreview/templatetags/bugzilla.py:30
(Diff revision 1)
> +    uri = '%s://%s%sauth.cgi?description=mozreview&callback=%s' % (
> +        u.scheme, u.netloc.rstrip('/'), bugzilla_root, callback_uri)

aren't there urllib methods for building querystring urls that deal with encoding and everything? might be better to use one of those.

::: pylib/mozreview/mozreview/tests/test_login.py:20
(Diff revision 1)
> -        self.assertInvalidLogin()
> +        # Because Bugzilla requires an email address, the form can't
> +        # even be submitted.

I don't understand this comment.

::: testing/vcttesting/unittest.py:146
(Diff revision 1)
> +    def load_bzurl(self, path):
> +        """Load the specified Review Board URL."""

This function's name vs docstring is problematic.

::: testing/vcttesting/unittest.py:155
(Diff revision 1)
> +    def verify_bzurl(self, path):

docstring please

Comment 43

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/13283/#review12025

You should make your commit messages more verbose - this distinction and explanation should exist there.

Comment 44

[Steven MacLeod [:smacleod]]

Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13283/#review12141

It's still possible (and necessary) to login with cookies through the API (the BugzillaCookieLoginResource allows this). We should make that clear in the commit message.

Comment 45

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/9885/#review12009

Yeah, it appears to be needed to create the admin user.  I get a few test failures without it, so I'll leave it in and add a comment.

Comment 46

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/9887/#review12135

> Can we call this `bmo_username` instead, I like it being more explicit so we don't have to lookup whether it is a user id or whatnot.

This is a Bugzilla-specific term, but yeah makes sense.

> I'm not really sure what this flag is used for / does in the bmo sense, but would it be worth notifying the user they can't login because their account is inactive?

This is actually from Review Board/Django (https://docs.djangoproject.com/en/1.7/ref/contrib/auth/#django.contrib.auth.models.User.is_active).  What you're saying probably still applies though... but given that we defer to Bugzilla for everything account-related right now, I put this in just for completeness.  I'll add a TODO, though, to note it, just in case we some day disable users only in Review Board.

Comment 47

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/9883/#review12007

> Add a comment or docstring explaining this has the side effect of logging the user in using the api key.

I added a note, although I don't think there's much a side effect here. True, the user is logged in, but we don't preserve the session cookie anywhere, so it shouldn't affect subsequent interactions with Review Board.

> I think you should either add a self.admin_api_key to store the result of this or add a comment to the effect that this has the side effect of authenticating using the api key.

I'm not sure what you mean here.  As I noted in the comment below, I don't think this really has any side effects.  We don't need to record the API key anywhere; it's used implicitly in future interactions with Bugzilla whenever the admin user (or any other user with a registered API key) logs into Review Board using the API.

Comment 48

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/9883/#review12021

I split the test-user-creation modifications into their own commit and flagged you (coming shortly).

Comment 49

[Mark Côté [:mcote]]

Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

The public URL is required in order to redirect the user to Bugzilla to
log in.

Comment 50

[Mark Côté [:mcote]]

Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

Comment 51

[Mark Côté [:mcote]]

Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

This is part of a larger effort to consolidate all Review Board customizations
in a single extension (mozreview).

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.

Comment 52

[Mark Côté [:mcote]]

Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r?smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.

Comment 53

[Mark Côté [:mcote]]

Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add or update API key when getting or creating Bugzilla users (bug 993233). r?smacleod

Comment 54

[Mark Côté [:mcote]]

Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

Comment 55

[Mark Côté [:mcote]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

Note: we should probably not deploy this until bug 1175643 has landed in
BMO and the MozReview callback function is updated appropriately.  This will
be a minor change, though, so the bulk of this commit will still apply and
hence can be reviewed now.

Comment 56

[Mark Côté [:mcote]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r?gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.

Comment 57

[Mark Côté [:mcote]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.

Comment 58

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r?smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.

Comment 59

[Mark Côté [:mcote]]

Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r?smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.

Comment 60

[Mark Côté [:mcote]]

Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Do not use Bugzilla cookies for anything other than verifying a login (bug 993233). r?smacleod

This just changes the mechanism that the rbbz and mozreview extensions use
for communicating with Bugzilla. For the purposes of identification with
MozReview, logging in with username and password or login cookies is still
possible via the API and with /account/login/?full=true (or any other value
for "full"). The latter is intended only for the admin account, as it will
not cause an API key to be generated and sent over from Bugzilla, meaning
most calls from Review Board to Bugzilla will fail.

Comment 61

[Gregory Szorc [:gps]]

Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9885/#review12437

Ship It!

Comment 62

[Gregory Szorc [:gps]]

Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

https://reviewboard.mozilla.org/r/13271/#review12439

Ship It!

Comment 63

[Gregory Szorc [:gps]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

https://reviewboard.mozilla.org/r/9889/#review12441

Nice!

Comment 64

[Steven MacLeod [:smacleod]]

Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13275/#review12667

Ship It!

Comment 65

[Steven MacLeod [:smacleod]]

Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13277/#review12671

::: pylib/mozreview/mozreview/bugzilla/models.py:118
(Diff revision 2)
> +def set_bugzilla_api_key(user, api_key):

The commit message needs to be rewritten to reflect what this commit now does.

Comment 66

[Steven MacLeod [:smacleod]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review12669

Looks good for the most part, just a few small nits.

::: pylib/mozreview/mozreview/models.py:19
(Diff revision 3)
> +    'set_bugzilla_api_key',

This stuff should be in the commit that creates the function.

::: pylib/mozreview/mozreview/views.py:50
(Diff revision 3)
> +    user = users[0]

We should assert that this user matches the username which we're supposed to be logging in with.

::: pylib/mozreview/mozreview/views.py:54
(Diff revision 3)
> +    if not user.is_active:
> +        # TODO: We should probably display a warning to the user, but we don't
> +        # currently use Review Board's is_active flag, so this isn't terribly
> +        # important. This clause is here just for completeness at the
> +        # moment.
> +        logging.warning('Validated API key but user %s is inactive.' %
> +                        bmo_username)
> +        return HttpResponseBadRequest()

I think we should move this to before the call where we associate the API key.

::: pylib/mozreview/mozreview/views.py:13
(Diff revision 3)
> +def bmo_auth_callback(request):

It might be good to catch any bugzilla errors due to stupid deadlocks on bugzilla's end, or other unexpected things like bugzilla going down during someone logging in, and display something nice to the user.

Comment 67

[Steven MacLeod [:smacleod]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9883/#review12677

::: testing/vcttesting/mozreview.py:494
(Diff revision 3)
> +        count = 1
> +        url = self.reviewboard_url + 'mozreview/bmo_auth_callback/'
> +        params = {'client_api_login': email, 'client_api_key': api_key}
> +        r = requests.get(url, params=params)
> +
> +        while r.status_code == 500 and count < 3:
> +            count += 1
> +            r = requests.get(url, params=params)
> +
> +        assert r.status_code == 200
> +
> +        return api_key

I think something like the following would be cleaner:
```
        url = self.reviewboard_url + 'mozreview/bmo_auth_callback/'
        params = {'client_api_login': email, 'client_api_key': api_key}

        for i in range(3):
            if requests.get(url, params=params).status_code == 200:
                return api_key
        else:
            # Raise / return some error here
```

::: testing/vcttesting/mozreview_mach_commands.py:208
(Diff revision 3)
> +                           api_key=not no_api_key)

"not no api key" - heh... I *guess* I'm fine with it, but it makes me feel dirty.

::: hgext/reviewboard/tests/test-auth.t:151
(Diff revision 3)
> -  dashboard_columns: 
> +  dashboard_columns: selected,new_updates,ship_it,my_comments,summary,submitter,last_updated_since
>    default_use_rich_text: None
>    extra_data: {}
>    first_time_setup_done: 0
>    group_columns: 
> -  id: 2
> +  id: 3
>    is_private: 1
>    open_an_issue: 1
>    review_request_columns: 
>    should_send_email: 1
>    should_send_own_updates: 1
>    show_closed: 1
> -  sort_dashboard_columns: 
> +  sort_dashboard_columns: -last_updated

I don't really understand these changes? is it because the user is no being logged in before this runs or something? what's going on here?

::: hgext/reviewboard/tests/test-user-apis.t:40
(Diff revision 3)
> +    url: /users/default%2B5/
> +    username: default+5
> +  - id: 4

Again, I don't see why this is changing, can you explain?

Comment 68

[Steven MacLeod [:smacleod]]

Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13693/#review12679

Ship It!

Comment 69

[Steven MacLeod [:smacleod]]

Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13281/#review12683

::: pylib/mozreview/mozreview/static/mozreview/js/logout.js:3
(Diff revision 2)
> +  $("#accountnav li").css("visibility", "visible");

Why is this required?

::: pylib/mozreview/mozreview/templatetags/bugzilla.py:9
(Diff revision 2)
> +from djblets.siteconfig.models import SiteConfiguration
> +
> +register = template.Library()

2 blanks between?

::: testing/vcttesting/unittest.py:11
(Diff revision 2)
>  import unittest
>  
> +from urllib import urlencode

why the blank?

Comment 70

[Steven MacLeod [:smacleod]]

Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13283/#review12685

I'm still not 100% happy with the commit message summary - how about "Stop storing bugzilla cookies used for initial authentication"

Comment 71

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/13281/#review12683

> why the blank?

For some reason I thought there was a convention to add a line between 'import foo' and 'from foo import bar' sections, but apparently I just made that up.

> Why is this required?

It's a similar trick to what you've used.  Because we have to override the logout link to go to our custom view (in order to not have it try to log back in again, which would immediately succeed if the user also had an active Bugzilla session) in JavaScript, owing to there being no nice way to modify the template, I am hiding the #accountnav items (there's no ID for the logout item specifically) via CSS in common.less (https://reviewboard.mozilla.org/r/13281/diff/2#1).  After the JS executes and modifies the logout link, I unhide them.

It might be overkill since a user would have to be very fast to hit the logout link before it was updated, but it feels like the right thing to do.

Comment 72

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/9883/#review12677

> Again, I don't see why this is changing, can you explain?

See above.

> I don't really understand these changes? is it because the user is no being logged in before this runs or something? what's going on here?

This is a result of the fact that 'mozreview create-user' now logs the user in once, by directly calling the auth-delegation callback, which in turn causes a redirect to the dashboard, which explains the changes to dashboard_columns.  An extra user is also created in Review Board (the "default" user in helpers.sh) where before only an entry in Bugzilla was created until the user was actually used in a Review Board context.  This explains the incremented user id above (and in your comment below).

> "not no api key" - heh... I *guess* I'm fine with it, but it makes me feel dirty.

Heh I know, but I wanted the 'api_key' arg to the internal function to match the others (defaulting to True) and require the user to explicitly say "don't create an API key".  Since it's a boolean option it ended up being weird like this.

Comment 73

[Mark Côté [:mcote]]

Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

The public URL is required in order to redirect the user to Bugzilla to
log in.

Comment 74

[Mark Côté [:mcote]]

Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

Comment 75

[Mark Côté [:mcote]]

Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

This is part of a larger effort to consolidate all Review Board customizations
in a single extension (mozreview).

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.

Comment 76

[Mark Côté [:mcote]]

Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.

Comment 77

[Mark Côté [:mcote]]

Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

Comment 78

[Mark Côté [:mcote]]

Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

Comment 79

[Mark Côté [:mcote]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

Note: we should probably not deploy this until bug 1175643 has landed in
BMO and the MozReview callback function is updated appropriately.  This will
be a minor change, though, so the bulk of this commit will still apply and
hence can be reviewed now.

Comment 80

[Mark Côté [:mcote]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.

Comment 81

[Mark Côté [:mcote]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.

Comment 82

[Mark Côté [:mcote]]

Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.

Comment 83

[Mark Côté [:mcote]]

Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.

Comment 84

[Mark Côté [:mcote]]

Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.

Comment 85

[Gregory Szorc [:gps]]

Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

https://reviewboard.mozilla.org/r/13273/#review12987

Ship It!

Comment 86

[Gregory Szorc [:gps]]

Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

https://reviewboard.mozilla.org/r/13279/#review12989

Ship It!

Comment 87

[Dan Minor [:dminor]]

Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

https://reviewboard.mozilla.org/r/13273/#review13023

Ship It!

Comment 88

[Steven MacLeod [:smacleod]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9883/#review14193

Ship It!

Comment 89

[Steven MacLeod [:smacleod]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review14195

as discussed in irc I will review when updated

Comment 90

[Mark Côté [:mcote]]

Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

The public URL is required in order to redirect the user to Bugzilla to
log in.

Comment 91

[Mark Côté [:mcote]]

Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

Comment 92

[Mark Côté [:mcote]]

Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

This is part of a larger effort to consolidate all Review Board customizations
in a single extension (mozreview).

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.

Comment 93

[Mark Côté [:mcote]]

Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.

Comment 94

[Mark Côté [:mcote]]

Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

Comment 95

[Mark Côté [:mcote]]

Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

Comment 96

[Mark Côté [:mcote]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

It requires a new table, mozreview_unverifiedbugzillaapikey, defined as such:

CREATE TABLE "mozreview_unverifiedbugzillaapikey" (
    "id" integer NOT NULL PRIMARY KEY,
    "login" varchar(255) NOT NULL,
    "api_key" varchar(40) NOT NULL,
    "remote_addr" char(39) NOT NULL,
    "timestamp" datetime NOT NULL
);
CREATE INDEX "mozreview_unverifiedbugzillaapikey_26440ee6" ON "mozreview_unverifiedbugzillaapikey" ("login");

Comment 97

[Mark Côté [:mcote]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.

Comment 98

[Mark Côté [:mcote]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.

Comment 99

[Mark Côté [:mcote]]

Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.

Comment 100

[Mark Côté [:mcote]]

Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.

Comment 101

[Mark Côté [:mcote]]

Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.

Comment 102

[Steven MacLeod [:smacleod]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review14665

::: pylib/mozreview/mozreview/bugzilla/models.py:38
(Diff revision 5)
> +class UnverifiedBugzillaApiKey(models.Model):

docstring

::: pylib/mozreview/mozreview/bugzilla/models.py:39
(Diff revision 5)
> +    login = models.CharField(max_length=255, db_index=True)

why "login" here? it seems like this value is named "login", "bmo_username", and "client_api_login" throughout this patch. Can we consolidate these into a nice descriptive name (at least the names we control)

::: pylib/mozreview/mozreview/bugzilla/models.py:48
(Diff revision 5)
> +        s = '%s%s%s%s%s' % (self.login, self.api_key, self.remote_addr,
> +                            self.timestamp, settings.SECRET_KEY)
> +        return hashlib.md5(s).hexdigest()

This stuff needs a serious docstring / justification for hash choice and the security requirements around this.

as it is I'm not comfortable with the secret key / api key being part of this hash.

to me it seems this purpose is just some random GUID to match up the keys, can we just generate something like that instead?

::: pylib/mozreview/mozreview/views.py:32
(Diff revision 5)
> +    This will change slightly when bug 1175643 lands in BMO.

Is this comment still valid?

::: pylib/mozreview/mozreview/views.py:42
(Diff revision 5)
> +def get_bmo_auth_callback(request):

this really needs a docstring explaining how this fits into everything, it's quite a large function.

I'd love to see the high level steps called out for how this process works.

::: pylib/mozreview/mozreview/views.py:123
(Diff revision 5)
> +def post_bmo_auth_callback(request):

docstring please

Comment 103

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/9887/#review14665

> This stuff needs a serious docstring / justification for hash choice and the security requirements around this.
> 
> as it is I'm not comfortable with the secret key / api key being part of this hash.
> 
> to me it seems this purpose is just some random GUID to match up the keys, can we just generate something like that instead?

Yeah in my original design I wanted something reproducible that we didn't have to store... but of course we have to store the API key itself until it's validated.  So yeah, I'll just generate a UUID and store that.

Comment 104

[Mark Côté [:mcote]]

Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

The public URL is required in order to redirect the user to Bugzilla to
log in.

Comment 105

[Mark Côté [:mcote]]

Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

Comment 106

[Mark Côté [:mcote]]

Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

This is part of a larger effort to consolidate all Review Board customizations
in a single extension (mozreview).

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.

Comment 107

[Mark Côté [:mcote]]

Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.

Comment 108

[Mark Côté [:mcote]]

Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

Comment 109

[Mark Côté [:mcote]]

Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

Comment 110

[Mark Côté [:mcote]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

It requires a new table, mozreview_unverifiedbugzillaapikey, defined as such:

CREATE TABLE "mozreview_unverifiedbugzillaapikey" (
    "id" integer NOT NULL PRIMARY KEY,
    "bmo_username" varchar(255) NOT NULL,
    "api_key" varchar(40) NOT NULL,
    "timestamp" datetime NOT NULL,
    "callback_result" varchar(36) NOT NULL
);
CREATE INDEX "mozreview_unverifiedbugzillaapikey_26440ee6" ON "mozreview_unverifiedbugzillaapikey" ("bmo_username");

Comment 111

[Mark Côté [:mcote]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.

Comment 112

[Mark Côté [:mcote]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.

Comment 113

[Mark Côté [:mcote]]

Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.

Comment 114

[Mark Côté [:mcote]]

Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.

Comment 115

[Mark Côté [:mcote]]

Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.

Comment 116

[Steven MacLeod [:smacleod]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review15101

::: pylib/mozreview/mozreview/views.py:45
(Diff revision 6)
> +    back to MozReview.  This handler then returns a unique result string,

the fact that "This handler *then* returns ..." comes after "when the user is redirected back to MozReview" makes the ordering a little confusing / ambiguous.

Can you reword things a bit to make them more clear?

::: pylib/mozreview/mozreview/views.py:99
(Diff revision 6)
> +    if not unverified_keys:
> +        msg = 'No unverified keys found.'
> +        logging.error(msg)
> +        return show_error_page(request, msg)

Errors like this means either we're being maliciously attacked, or something went very wrong. We should avoid explaining exactly what has failed when the process fails, as none of it is really user actionable, and it could help attackers.

Example: I can make requests to this endpoint with email addresses and check if they have an unverified key or not. Since we don't actually clean these up I could find a user with one and try to guess the uuid or something (I don't think there is an actual viable attack here, but I'd rather not provide information that could help)

::: pylib/mozreview/mozreview/views.py:112
(Diff revision 6)
> +        msg = 'Callback result does not match.'

ya, the user facing error should be generic, and just tell them to retry logging in or something.

::: pylib/mozreview/mozreview/views.py:127
(Diff revision 6)
> +        return show_error_page(request, 'Error validating API key: %s' % e.msg)

just log it, generic error.

::: pylib/mozreview/mozreview/views.py:134
(Diff revision 6)
> +        return show_error_page(request, 'Error getting user data: %s' % e.msg)

At this point we've verified the API key, so the login is *kind* of authentic now, so you could show more verbose errors at this point if you want (I'd only suggest it if they're actionable though)

Comment 117

[Mark Côté [:mcote]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

It requires a new table, mozreview_unverifiedbugzillaapikey, defined as such:

CREATE TABLE "mozreview_unverifiedbugzillaapikey" (
    "id" integer NOT NULL PRIMARY KEY,
    "bmo_username" varchar(255) NOT NULL,
    "api_key" varchar(40) NOT NULL,
    "timestamp" datetime NOT NULL,
    "callback_result" varchar(36) NOT NULL
);
CREATE INDEX "mozreview_unverifiedbugzillaapikey_26440ee6" ON "mozreview_unverifiedbugzillaapikey" ("bmo_username");

Comment 118

[Mark Côté [:mcote]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.

Comment 119

[Mark Côté [:mcote]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.

Comment 120

[Mark Côté [:mcote]]

Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.

Comment 121

[Mark Côté [:mcote]]

Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.

Comment 122

[Mark Côté [:mcote]]

Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.

Comment 123

[Steven MacLeod [:smacleod]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review15137

::: pylib/mozreview/mozreview/views.py:101
(Diff revisions 6 - 7)
> -        msg = 'No unverified keys found.'
> +        logging.error('No unverified keys found.')

It would be useful to give information about which username etc so we can actually followup a bit more.

::: pylib/mozreview/mozreview/views.py:112
(Diff revisions 6 - 7)
> -        msg = 'Callback result does not match.'
> +        logging.error('Callback result does not match.')

Maybe we should log the unmatched callback_results here, so we could track what's going on? or at least the one that was provided by the request?

Or maybe that's insecure... hmmm

Comment 124

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/9887/#review15137

> Maybe we should log the unmatched callback_results here, so we could track what's going on? or at least the one that was provided by the request?
> 
> Or maybe that's insecure... hmmm

I'll add this if needed later.  I did add the username though.

Comment 125

[Mark Côté [:mcote]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

It requires a new table, mozreview_unverifiedbugzillaapikey, defined as such:

CREATE TABLE "mozreview_unverifiedbugzillaapikey" (
    "id" integer NOT NULL PRIMARY KEY,
    "bmo_username" varchar(255) NOT NULL,
    "api_key" varchar(40) NOT NULL,
    "timestamp" datetime NOT NULL,
    "callback_result" varchar(36) NOT NULL
);
CREATE INDEX "mozreview_unverifiedbugzillaapikey_26440ee6" ON "mozreview_unverifiedbugzillaapikey" ("bmo_username");

Comment 126

[Mark Côté [:mcote]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.

Comment 127

[Mark Côté [:mcote]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.

Comment 128

[Mark Côté [:mcote]]

Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.

Comment 129

[Mark Côté [:mcote]]

Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.

Comment 130

[Mark Côté [:mcote]]

Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.

Comment 131

[Steven MacLeod [:smacleod]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

https://reviewboard.mozilla.org/r/9887/#review15155

Ship It!

Comment 132

[Mark Côté [:mcote]]

Comment on attachment 8614312 [details]
MozReview Request: mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

mozreview: Use Bugzilla's public URL in the rbweb container (bug 993233). r=gps

The public URL is required in order to redirect the user to Bugzilla to
log in.

Comment 133

[Mark Côté [:mcote]]

Comment on attachment 8634076 [details]
MozReview Request: mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

mozreview: Enable auth delegation in bmoweb (bug 993233). r=gps

Comment 134

[Mark Côté [:mcote]]

Comment on attachment 8634077 [details]
MozReview Request: mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

mozreview: Move Bugzilla client code over to mozreview extension (bug 993233). r=dminor

This is part of a larger effort to consolidate all Review Board customizations
in a single extension (mozreview).

Bump mozreview to 0.1.1alpha and rbbz to 0.2.9.

Comment 135

[Mark Côté [:mcote]]

Comment on attachment 8634078 [details]
MozReview Request: mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

mozreview: Add api_key field to BugzillaUserMap (bug 993233). r=smacleod

Bump mozreview version to 0.1.2alpha.  This requires a database schema
migration to add a nullable varchar(40) field with the name api_key to the
mozreview_bugzillausermap table.

Comment 136

[Mark Côté [:mcote]]

Comment on attachment 8634079 [details]
MozReview Request: mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

mozreview: Add function to store Bugzilla API keys (bug 993233). r=smacleod

Comment 137

[Mark Côté [:mcote]]

Comment on attachment 8634080 [details]
MozReview Request: mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

mozreview: Add function to Bugzilla client to validate api keys (bug 993233). r=smacleod

Comment 138

[Mark Côté [:mcote]]

Comment on attachment 8614313 [details]
MozReview Request: mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

mozreview: Add auth-delegation callback view (bug 993233). r?smacleod

This provides the necessary endpoint for Bugzilla's auth delegation.  It
calls BugzillaBackend.authenticate() to verify the API key and, if it checks
out, calls login() to create a Review Board session.

The local docker setup has been updated to set this new endpoint in the
Bugzilla config.

It requires a new table, mozreview_unverifiedbugzillaapikey, defined as such:

CREATE TABLE "mozreview_unverifiedbugzillaapikey" (
    "id" integer NOT NULL PRIMARY KEY,
    "bmo_username" varchar(255) NOT NULL,
    "api_key" varchar(40) NOT NULL,
    "timestamp" datetime NOT NULL,
    "callback_result" varchar(36) NOT NULL
);
CREATE INDEX "mozreview_unverifiedbugzillaapikey_26440ee6" ON "mozreview_unverifiedbugzillaapikey" ("bmo_username");

Comment 139

[Mark Côté [:mcote]]

Comment on attachment 8614314 [details]
MozReview Request: mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

mozreview: Use "mozreview" instead of "adminbugzilla" when creating users (bug 993233). r=gps

The "mozreview create-user" mach command has options to create all aspects
of a mozreview user, so we consolidate all the "adminbugzilla create-user"
and "mozreview create-ldap-user" calls into just "mozreview create-user"
for simplicity.

Comment 140

[Mark Côté [:mcote]]

Comment on attachment 8614311 [details]
MozReview Request: mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

mozreview: Authenticate to BMO with API key (bug 993233). r?smacleod

Send Bugzilla API keys with authenticated requests rather than cookies.
Fix up tests to ensure API keys are created where needed.

Comment 141

[Mark Côté [:mcote]]

Comment on attachment 8636774 [details]
MozReview Request: mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

mozreview: Rename common.js to init_rr.js and clean it up (bug 993233). r=smacleod

"common.js" is misleading because it is only used on the review-request view,
and furthermore it doesn't describe what it does.

Comment 142

[Mark Côté [:mcote]]

Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.

Comment 143

[Mark Côté [:mcote]]

Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.

Comment 144

[Mark Côté [:mcote]]

Comment on attachment 8634081 [details]
MozReview Request: mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

mozreview: Override login page to use Bugzilla auth delegation (bug 993233). r=smacleod

Passing a "full" query-string parameter with any value, e.g. "full=true",
in the login URL results in the full login page being displayed. This exists
primarily to allow the admin user to log in, which does not have a
corresponding Bugzilla account.

Also override the "Log Out" link to not redirect to the login page, since if
you still have an open Bugzilla session, you'll be automatically logged back
into Review Board.

Selenium tests have been updated, but not all appear to be passing on tip
right now, so there are still failures.

Comment 145

[Mark Côté [:mcote]]

Comment on attachment 8634082 [details]
MozReview Request: mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

mozreview: Stop storing Bugzilla cookies (bug 993233). r=smacleod

Since all communication is now done via API keys, we no longer need to
store cookies in the user object.

For the purposes of identification with MozReview, logging in with username
and password or login cookies is still possible via the API and with
/account/login/?full=true (or any other value for "full"). The latter is
intended only for the admin account, as it will not cause an API key to be
generated and sent over from Bugzilla, meaning most calls from Review Board
to Bugzilla will fail. In both cases, the cookie/password is only used to
verify the account with Bugzilla and is discarded immediately after.

Comment 147

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/9887/#review15361

::: pylib/mozreview/mozreview/views.py:61
(Diff revision 9)
> +        timestamp=timezone.now(),

I believe you can default the timestamp on the model to whenever it's created, that way you don't have to perform this. I don't really care too much though, so use your judgement.

Comment 148

[Mark Côté [:mcote]]

hg.mozilla.org/hgcustom/version-control-tools/rev/6a8be31971d4
http://hg.mozilla.org/hgcustom/version-control-tools/rev/1676d03eb402
http://hg.mozilla.org/hgcustom/version-control-tools/rev/1fc3bd36d415
http://hg.mozilla.org/hgcustom/version-control-tools/rev/ee7747552bec
http://hg.mozilla.org/hgcustom/version-control-tools/rev/7dc2df9f7387
http://hg.mozilla.org/hgcustom/version-control-tools/rev/b3492a5daedf (accidentally squashed the auth-delegation callback view commit into this one :)
http://hg.mozilla.org/hgcustom/version-control-tools/rev/2af905e3dfef
http://hg.mozilla.org/hgcustom/version-control-tools/rev/8fd986fa5873
http://hg.mozilla.org/hgcustom/version-control-tools/rev/79325f01fc50
http://hg.mozilla.org/hgcustom/version-control-tools/rev/caf5861bf537
http://hg.mozilla.org/hgcustom/version-control-tools/rev/4a370dc3fec8

Comment 149

[Mark Côté [:mcote]]

This is live on prod.