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

RESOLVED FIXED

Status

Developer Services
Mercurial: mozext
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → ted
(Assignee)

Comment 1

3 years ago
Created attachment 8549628 [details]
MozReview Request: bz://1120538/ted
(Assignee)

Comment 2

3 years ago
/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

3 years ago
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

3 years ago
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.
(Assignee)

Comment 5

3 years ago
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.
(Assignee)

Comment 6

3 years ago
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.)
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Comment 8

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

Comment 9

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

Pull down this commit:

hg pull review -r 57d9c56141925826c5eb1bcc77299271fbda3626
(Assignee)

Comment 10

3 years ago
With this + bug 1122513 life is pretty good.
(Assignee)

Comment 11

3 years ago
Review ping? This has been sitting here quite a while.
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.
Attachment #8549628 - Flags: review+
(Assignee)

Comment 13

3 years ago
https://hg.mozilla.org/hgcustom/version-control-tools/rev/b52359db4905
(Assignee)

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

3 years ago
Comment on attachment 8549628 [details]
MozReview Request: bz://1120538/ted
Attachment #8549628 - Attachment is obsolete: true
Attachment #8619106 - Flags: review+
Attachment #8619107 - Flags: review+
(Assignee)

Comment 15

3 years ago
Created attachment 8619106 [details]
MozReview Request: bug 1120538 - mozauth: prefer cookies from profile, use prompting as last resort
(Assignee)

Comment 16

3 years ago
Created attachment 8619107 [details]
MozReview Request: mozhg: add some tests for mozhg.auth.getbugzillaauth
You need to log in before you can comment on or make changes to this bug.