1120538 - mozauth should use prompting for a bugzilla password only as a last resort

Status: RESOLVED FIXED

(Developer Services :: Mercurial: mozext, defect)

Description

[(not currently active) Ted Mielczarek]

Currently "mach mercurial-setup" prompts you to fill in a bugzilla username and puts it in your ~/.hgrc. It also configures bzpost for you. Then (if you do no other configuration) every time you push you will get prompted for your bugzilla password.

The code in mozauth is written such that if it finds a username in your hgrc it prompts for a password:
http://hg.mozilla.org/hgcustom/version-control-tools/file/8c36430488c8/pylib/mozhg/mozhg/auth.py#l54

It should instead try to find a login cookie from a Firefox profile and only as a last resort fall back to prompting.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bz://1120538/ted

Comment 2

[(not currently active) Ted Mielczarek]

/r/2543 - mozhg: add some tests for mozhg.auth.getbugzillaauth
/r/2545 - bug 1120538 - mozauth: prefer cookies from profile, use prompting as last resort

Pull down these commits:

hg pull review -r 88d9e9b88089140a3b062b9b9cd12576131d8f9b

Comment 3

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/2543/#review1695

I'm not a fan of mocking the Mercurial ui object. Did you see test-hg-auth.t?

As an alternative, you could split `getbugzillaauth` into 2 functions: one that talks Mercurial APIs and one that doesn't. I argue it should be this way. It kind of organically grew into where it is today. We'll eventually need to abstract the logic to support grabbing cookies from other services.

Comment 4

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/2545/#review1697

I would be shocked if this didn't break .t tests.

Again, I prefer testing this via .t tests. If you split the API, it is appropriate to test non-Mercurial Python with Python unit tests.

Comment 5

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/2545/#review1703

All the tests pass locally for me (modulo whatever gets skipped). test-hg-auth.t passes, but it's also not testing this specific case, so I'm not surprised. I'll change the tests here to be .t tests since that seems straightforward.

Comment 6

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/2543/#review1701

I mocked the ui object mostly because we're only using it for very simple stuff there (config, output, prompt). I...probably should have read that test more thoroughly, it does cover most of the work here.

I'm not really sure I see the value in splitting that function to be Mercurial-agnostic right now. It feels like code churn without value. I'm happy to ditch this changeset if it's not giving us extra test coverage above what the .t test provides (I can sanity check that we're covering all the same things and update the .t test instead.)

Comment 7

[(not currently active) Ted Mielczarek]

So I'm going to address the review comments, but using this patch locally has revealed another problem--bzpost doesn't know how to use cookie auth, because Bugsy doesn't know how to use cookie auth. I will probably shave that yak, but gps noted that bug 1121764 is an issue he hit when changing bzexport to use the bugzilla REST API directly--there are corner cases where cookie auth alone will not let us do everything we want due to deficiencies in the REST API.

Comment 8

[(not currently active) Ted Mielczarek]

Oh, except I just read the updates on that bug from yesterday and it sounds like we can totally work around that. Hooray!

Comment 9

[(not currently active) Ted Mielczarek]

/r/2545 - bug 1120538 - mozauth: prefer cookies from profile, use prompting as last resort

Pull down this commit:

hg pull review -r 57d9c56141925826c5eb1bcc77299271fbda3626

Comment 10

[(not currently active) Ted Mielczarek]

With this + bug 1122513 life is pretty good.

Comment 11

[(not currently active) Ted Mielczarek]

Review ping? This has been sitting here quite a while.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8549628 [details]
MozReview Request: bz://1120538/ted

https://reviewboard.mozilla.org/r/2541/#review4329

There wasn't an r? flag so I didn't see this :/

Looks good.

Comment 13

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/b52359db4905

Comment 15

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bug 1120538 - mozauth: prefer cookies from profile, use prompting as last resort

Comment 16

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: mozhg: add some tests for mozhg.auth.getbugzillaauth