Closed Bug 1067525 Opened 7 years ago Closed 7 years ago

Mercurial extension should use token, not username & password

Categories

(MozReview Graveyard :: General, defect)

Development/Staging
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcote, Assigned: gps)

References

Details

Attachments

(3 files)

While the current system of passing the user's Bugzilla username and password along the wire from hg client to hg server to Review Board to Bugzilla works, it's not ideal.  The best solution would be an API key, support for which has already landed in upstream Bugzilla and will be brought to BMO, but in the interim we can have the hg client extension log into Bugzilla to obtain a token and then pass that along to the hg server.

Ideally it should be able to cache this token somewhere, although the server will probably have to be enhanced to detect expired tokens so that the client can log back in.
Note that Review Board supports API tokens: https://www.reviewboard.org/docs/manual/dev/webapi/2.0/authenticating/#token-based-authentication

This would be the ideal solution here.  You log into Review Board, create an API token, and put it into your hgrc, and that's passed to the server so that it can talk to Review Board without ever knowing a password (or indeed even a Bugzilla username).  In our current system, I don't believe there is any communication with Bugzilla when the review request is created, since it remains in draft mode until reviewers are manually added.  So this should work just fine for now.

Later, if we somehow allow direct publishing of review requests (with reviewers passed on the command line or in commit messages or somehow else), we'd need to ensure that the Bugzilla token has not expired, but by this time API tokens should be in BMO.
(In reply to Mark Côté [:mcote] from comment #1)
> Note that Review Board supports API tokens:
> https://www.reviewboard.org/docs/manual/dev/webapi/2.0/authenticating/#token-
> based-authentication
> 
> This would be the ideal solution here.  You log into Review Board, create an
> API token, and put it into your hgrc, and that's passed to the server so
> that it can talk to Review Board without ever knowing a password (or indeed
> even a Bugzilla username).  In our current system, I don't believe there is
> any communication with Bugzilla when the review request is created, since it
> remains in draft mode until reviewers are manually added.  So this should
> work just fine for now.

This seems like an improvement but wouldn't the ideal solution be that the
user never has to have contact with the API token? I.e., use OAuth like
the rest of the world?

> Later, if we somehow allow direct publishing of review requests (with
> reviewers passed on the command line or in commit messages or somehow else),
> we'd need to ensure that the Bugzilla token has not expired, but by this
> time API tokens should be in BMO.
Yeah, OAuth is probably a good idea down the road, but there are two main problems:

* Neither Review Board nor Bugzilla support it yet, and presumably both would have to in order for the full data flow to be in OAuth.
* The mercurial client extension would have to support OAuth in some way or another.  This is a bigger problem because it would then almost certainly require extra Python packages.  This is annoying but not terrible in Linux, but on Windows this would be a really big pain.

Regardless, I'll raise the issue of OAuth support with both upstream Bugzilla and Review Board.
Hm unfortunately API tokens in Review Board haven't actually landed yet.  Not sure when it will.  If API token support lands in BMO (the backport is in review right now), I might be able to jerry-rig something there.

Review Board is planning OAuth for version 2.2; as he says, it's tricky to get right, so it might be a few months before we see it there.
/r/170 - Bug 1067525 - Add support to rbbz for logging in with a bugzilla cookie.
/r/171 - Bug 1067525 - Use the bugzilla cookie login resource in post_reviews if a cookie is provided

Pull down these commits:

hg pull review -r 063b2c12326064f6ce64a52344e855a286c5b666
Attachment #8497100 - Flags: review?(mcote)
Attachment #8497100 - Flags: review?(gps)
https://reviewboard-dev.allizom.org/r/170/#review143

::: pylib/rbbz/rbbz/resources.py
(Diff revision 1)
> +from django.contrib.auth import login

Need license header.
Attachment #8497100 - Flags: review+
https://reviewboard-dev.allizom.org/r/169/#review146

This technically uses cookies, not tokens, but it's not particularly important since we're switching to API tokens at some point, and it's better than passing passwords.  Tested with my local RB/Bugzilla setup and it works fine.  Thanks!  Just need the hg client part now. :)
Comment on attachment 8497100 [details]
Review for review ID: bz://1067525/smacleod

Apparently another small bug with rbbz; it added a new flag instead of updating the existing one when I published my review.
Attachment #8497100 - Flags: review?(mcote)
/r/181 - bzexport: call bootstrap to enable module loading
/r/182 - bzexport: move profile finding functions into mozhg.auth
/r/183 - mozhg: clean up profile finding code
/r/184 - testing: support running Python unit tests via nose
/r/185 - mozhg: split profile directory finding code into own function
/r/186 - mozhg: refactor profiles API to return list of profiles
/r/187 - mozhg: move get_cookies_from_profile into mozhg.auth
/r/188 - mozhg: fix style in Bugzilla login cookie fetching
/r/189 - testing: run nose with -s
/r/190 - mozhg: further refactor get cookie from profile foo
/r/191 - mozhg: tests for extracting cookies from profiles
/r/192 - mozhg: add a testing backdoor for finding the Firefox profiles dir
/r/193 - mozhg: try to fetch Bugzilla cookies in mozhg.auth.getbugzillaauth()

Pull down these commits:

hg pull review -r adc620ce7142359921a8fc253b21e7d02c5ee16e
https://reviewboard-dev.allizom.org/r/180/#review147

This patch series still needs some work (I don't think RB yet handles the cookie properly). But the plumbing should be mostly there now.

Patch series is long. But each individual patch should be short and manageable.
(In reply to Gregory Szorc [:gps] from comment #14)
> https://reviewboard-dev.allizom.org/r/180/#review147
> 
> This patch series still needs some work (I don't think RB yet handles the
> cookie properly). But the plumbing should be mostly there now.
> 
> Patch series is long. But each individual patch should be short and
> manageable.

(replying bugzilla, reviewboard-dev is borked)
The RBBZ api stuff for using the cookie works, I tested that. The part in post_reviews that makes the api calls when passed a cookie wasn't tested as it's written, just the overall scheme was tested in a separate script. So, if something seems broken in the handling I'd suspect it's the post_reviews part.
FWIW, I've been banging my head against running a BMO instance as part of the version-control-tools tests again. That should hopefully help a lot testing/debugging the Bugzilla interaction.
Attachment #8497100 - Flags: review+
Comment on attachment 8497100 [details]
Review for review ID: bz://1067525/smacleod

I think I found a bug.
Attachment #8497100 - Flags: review?(gps)
Product: bugzilla.mozilla.org → Developer Services
/r/181 - mozhg: copy profile finding functions from bzexport into mozhg.auth
/r/182 - mozhg: clean up profile finding code
/r/183 - mozhg: split profile directory finding code into own function
/r/184 - mozhg: refactor profiles API to return list of profiles
/r/185 - mozhg: remove find_profile()
/r/186 - mozhg: copy get_cookies_from_profile into mozhg.auth
/r/187 - mozhg: fix style in Bugzilla login cookie fetching
/r/188 - mozhg: further refactor get cookie from profile logic
/r/189 - mozhg: tests for extracting cookies from profiles
/r/190 - mozhg: add a testing backdoor for finding the Firefox profiles dir

Pull down these commits:

hg pull review -r e7b9fec9b3f31b2cf15241f3f7b863e267d4ce5e
/r/170 - Bug 1067525 - Add support to rbbz for logging in with a bugzilla cookie. r=gps
/r/171 - Bug 1067525 - Use the bugzilla cookie login resource in post_reviews if a cookie is provided. r=gps
/r/200 - Bug 1067525 - Allow authenticating with Bugzilla Cookie when anonymous access is disabled. r=mconley

Pull down these commits:

hg pull review -r 41eeec4d14230788a34ab3a0461f2a8332d8a80a
/r/181 - mozhg: copy profile finding functions from bzexport into mozhg.auth
/r/182 - mozhg: clean up profile finding code
/r/183 - mozhg: split profile directory finding code into own function
/r/184 - mozhg: refactor profiles API to return list of profiles
/r/185 - mozhg: remove find_profile()
/r/186 - mozhg: copy get_cookies_from_profile into mozhg.auth
/r/187 - mozhg: fix style in Bugzilla login cookie fetching
/r/188 - mozhg: further refactor get cookie from profile logic
/r/189 - mozhg: tests for extracting cookies from profiles
/r/190 - mozhg: add a testing backdoor for finding the Firefox profiles dir
/r/201 - mozhg: try to fetch Bugzilla cookies in mozhg.auth.getbugzillaauth()
/r/202 - mozhg: add extra space in Bugzilla password prompt
/r/203 - mozhg: ignore profiles with no files
/r/204 - mozhg: make cookie creating functions static
/r/205 - mozhg: add Mercurial tests for Bugzilla credential searching

Pull down these commits:

hg pull review -r 893dc20de6fb72903ed1b6b9b49e3f014efb3ca3
https://reviewboard-dev.allizom.org/r/188/#review159

::: pylib/mozhg/mozhg/auth.py
(Diff revision 3)
>      ui is a mercurial.ui instance.

We no longer take `ui`.
https://reviewboard-dev.allizom.org/r/204/#review166

This seems a little strange to me, I'm curious why you didn't just move them off of the testcase?

It really doesn't matter much, so if you really like it this way I'm fine with it.
https://reviewboard-dev.allizom.org/r/204/#review169

You are right. I'll move them off the testcase.
The pre-work has been pushed. 8372d22b807e::c3d8aa9dcbcf.

Now I'll start work on the refactor of reviewboard to use cookies / tokens by default. I think this is mostly writing tests and adjusting the UX to make sure everything is sane.
Assignee: nobody → gps
Status: NEW → ASSIGNED
rbtools.api.errors.APIError: The change number specified has already been used. (HTTP 409, API Error 204)

I hate this restriction. I'm just going to upload the patch to Bugzilla...
/r/208 - reviewboard: provide a backdoor to not define Bugzilla credentials
/r/209 - reviewboard: handle auth errors gracefully

Pull down these commits:

hg pull review -r 9266be05f35408a58c29d065108adc64cb4b2edc
Comment on attachment 8505086 [details]
Review for review ID: bz://1067525/gps2

Or not.

I'll keep pushing to this review. Stay on your toes.
Attachment #8505086 - Flags: review?(smacleod)
Attachment #8505086 - Flags: review?(smacleod)
/r/208 - reviewboard: mercurial 3.2 compatibility
/r/209 - reviewboard: provide a backdoor to not define Bugzilla credentials
/r/211 - reviewboard: handle auth errors gracefully
/r/212 - mozhg: ability to define Bugzilla cookie in hgrc
/r/213 - bugzilla: add a command to obtain a login cookie
/r/214 - reviewboard: support cookie auth
/r/215 - reviewboard: add a helper function to export Bugzilla credentials
/r/216 - bugzilla: command to create new Bugzilla users
/r/217 - reviewboard: add a dump-user command to rbmanage.py
/r/218 - reviewboard: tests for automatic creation of users in Review Board
/r/219 - bugzilla: add a command to update a user's full name
/r/220 - reviewboard: add a test for change in IRC nickname
/r/221 - bugzilla: add command for updating a user's email
/r/222 - reviewboard: add test for changing of a Bugzilla user's email
/r/223 - bugzilla: add a command to update a Bugzilla user's login denied text
/r/224 - reviewboard: add a test that verifies a disabled user can't use Review Board
/r/225 - reviewboard: add a test that re-enabling a disabled user works
/r/226 - reviewboard: add a test for conflicting IRC nicknames
/r/227 - reviewboard: add a test for existing users using a conflicting username

Pull down these commits:

hg pull review -r 1833f3a6b908fa3b19581f4f317d3dde97aeb458
Comment on attachment 8505086 [details]
Review for review ID: bz://1067525/gps2

reviewboard-dev is throwing 500s again. *sigh*.

Anyway, this new patch series implements auth via cookies, adds tests for that, and adds many tests for the Bugzilla <=> Review Board user handling code from the perspective of Mercurial. I'm sure you'll be happy to learn that I found no bugs in your rbbz.models code. But then again, maybe my tests just aren't accurate. Please pay careful attention that the tests are verifying proper behavior.
Attachment #8505086 - Flags: review?(smacleod)
https://reviewboard-dev.allizom.org/r/208/#review170

::: hgext/reviewboard/client.py
(Diff revision 2)
> +# can add it explicitly once support for <3.2 has been dropped.

heh, wonder when that will be.

::: hgext/reviewboard/client.py
(Diff revision 2)
> -        return orig(repo, remote, force=force, revs=revs, newbranch=newbranch)
> +        return orig(repo, remote, force=force, revs=revs, newbranch=newbranch,
> +                **kwargs)

Indentation is strange here. Should be:
            return orig(repo, remote, force=force, revs=revs, newbranch=newbranch,
                        **kwargs)

::: hgext/reviewboard/client.py
(Diff revision 2)
> -        return orig(repo, remote, force=True, revs=revs, newbranch=newbranch)
> +        return orig(repo, remote, force=True, revs=revs, newbranch=newbranch,
> +                **kwargs)

Same as above.
https://reviewboard-dev.allizom.org/r/211/#review172

::: hgext/reviewboard/hgrb/proto.py
(Diff revision 1)
> +    def __init__(self, e, kwargs):
> +        self.e = e
> +        self.username = kwargs['username']
> +        self.password = kwargs['password']
> +        self.cookie = kwargs['cookie']

I'm not a fan of using `kwargs` here like this, instead of the traditional use of `**kwargs`. In fact, my brain initially just assumed it was the latter.

Also, the method parameters make it seem like `kwargs['username']` and friends are optional, but if one is missing we will throw.

I'd be happier if we made the arguments explicit and called using `**`, eg:
    def __init__(self, e, username, password, cookie):
        ...
    
    # and
    raise AuthError(e, **kwargs)

Or, if we'd like them to be optional/defaulted, something like:
    def __init__(self, e, username=None, password=None, cookie=None):
https://reviewboard-dev.allizom.org/r/211/#review173

> I'm not a fan of using `kwargs` here like this, instead of the traditional use of `**kwargs`. In fact, my brain initially just assumed it was the latter.
> 
> Also, the method parameters make it seem like `kwargs['username']` and friends are optional, but if one is missing we will throw.
> 
> I'd be happier if we made the arguments explicit and called using `**`, eg:
>     def __init__(self, e, username, password, cookie):
>         ...
>     
>     # and
>     raise AuthError(e, **kwargs)
> 
> Or, if we'd like them to be optional/defaulted, something like:
>     def __init__(self, e, username=None, password=None, cookie=None):

Alternative:
``` Python
def __init__(self, e, **kwargs):
    self.e = e
    self.username = kwargs.get('username', None)
    # etc.
    
# and
raise AuthError(e, **kwargs)
 ```
https://reviewboard-dev.allizom.org/r/214/#review176

Love all the testing going on, makes me so happy, and confident in the code.

::: hgext/reviewboard/hgrb/proto.py
(Diff revision 1)
>      def __init__(self, e, kwargs):
>          self.e = e
>          self.username = kwargs['username']
>          self.password = kwargs['password']
> +        self.userid = kwargs['userid']
>          self.cookie = kwargs['cookie']

Same comments about this as in /r/211/
Blocks: 1021929
https://reviewboard-dev.allizom.org/r/226/#review188

::: hgext/reviewboard/tests/test-auth.t
(Diff revision 1)
> +  $ $TESTDIR/testing/bugzilla.py create-user user3@example.com password3 'Dummy User3 [:newnick]'

We're starting to build up a lot of dependency upon previous steps of the test here (using :newnick, rather than crafting two users specifically for this test).

I don't think we have a problem, but going forward if this test grows much things might be easier to understand if we broke the cases down into their own files.

Are we looking at too high of a testing overhead (building new containers, new dbs, etc.) to really split things up into single file per single test case?
https://reviewboard-dev.allizom.org/r/226/#review190

> We're starting to build up a lot of dependency upon previous steps of the test here (using :newnick, rather than crafting two users specifically for this test).
> 
> I don't think we have a problem, but going forward if this test grows much things might be easier to understand if we broke the cases down into their own files.
> 
> Are we looking at too high of a testing overhead (building new containers, new dbs, etc.) to really split things up into single file per single test case?

I was thinking the same thing when writing these tests. This test file kind of got away from me a little bit.

I think there's room to refactor this later.

Right now there is a bit of overhead to set up the environments, so there is a benefit to consolidation. But, yeah, that's a poor excuse.
https://reviewboard-dev.allizom.org/r/207/#review191

Looks great! r=me with the issues addressed from each commit review request.

Ship-It!
Attachment #8505086 - Flags: review?(smacleod)
The latest patch series has landed. Now waiting on Jenkins to confirm it is green. That will likely take an hour.

Since the Mercurial client will now use cookies (from the Firefox profile) by default, can we call this done? Or do we need to hook up ReviewBoard's API tokens?
API-token support in Review Board hasn't been released yet, I don't think.  So yeah, sounds like this is done.

I assume you can override/fall back from finding cookies in the Firefox profile?  We should document this stuff (general MozRB usage) on the wiki, on a page off https://wiki.mozilla.org/Auto-tools/Projects/CodeReviewTool.
Yeah, you can define username/password or userid/cookie in your hgrc. If they are present, we don't do profile scanning.

I'm calling this done. Will need to write docs.

I'll add Sphinx support to version-control-tools (I've been meaning to do this for a while since the complexity has started justifying it).
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.