| Submitter | Matt Harbison |
|---|---|
| Date | May 15, 2018, 3:43 a.m. |
| Message ID | <b7849cc51d8d8040f233.1526355822@Envy> |
| Download | mbox | patch |
| Permalink | /patch/31591/ |
| State | Accepted |
| Headers | show |
Comments
On Mon, May 14, 2018 at 8:43 PM Matt Harbison <mharbison72@gmail.com> wrote: > # HG changeset patch > # User Matt Harbison <matt_harbison@yahoo.com> > # Date 1526353230 14400 > # Mon May 14 23:00:30 2018 -0400 > # Node ID b7849cc51d8d8040f233328ee46a6466dff207cd > # Parent b5d62ec33438171e00030ec1442f91fbf496adf6 > phabricator: split auth.url into the standard auth.schemes and auth.prefix > Kyle (on CC) noticed earlier today that the fact that the "phabricator.auth" section has a period in its name means it can't be specified on the command line using --config (as far as he could tell, anyway). The timing of this patch made me assume it was in response to that finding (maybe Kyle had reached out to you, I thought), but it's not about fixing that issue, as far as I can tell, which confused me quite a bit for a while :) Anyway, I thought I might at least use this as an opportunity to make you aware of that issue.
> On May 15, 2018, at 1:05 AM, Martin von Zweigbergk <martinvonz@google.com> wrote: > > > >> On Mon, May 14, 2018 at 8:43 PM Matt Harbison <mharbison72@gmail.com> wrote: >> # HG changeset patch >> # User Matt Harbison <matt_harbison@yahoo.com> >> # Date 1526353230 14400 >> # Mon May 14 23:00:30 2018 -0400 >> # Node ID b7849cc51d8d8040f233328ee46a6466dff207cd >> # Parent b5d62ec33438171e00030ec1442f91fbf496adf6 >> phabricator: split auth.url into the standard auth.schemes and auth.prefix > > Kyle (on CC) noticed earlier today that the fact that the "phabricator.auth" section has a period in its name means it can't be specified on the command line using --config (as far as he could tell, anyway). The timing of this patch made me assume it was in response to that finding (maybe Kyle had reached out to you, I thought), but it's not about fixing that issue, as far as I can tell, which confused me quite a bit for a while :) Anyway, I thought I might at least use this as an opportunity to make you aware of that issue. Yuya mentioned that over the weekend, so I moved it to [auth]. When he queued that this morning, he mentioned that [auth] has these fields instead of a URL. So, this is the follow up to the fix you mentioned.
On Mon, May 14, 2018 at 10:14 PM Matt Harbison <mharbison72@gmail.com> wrote: > > On May 15, 2018, at 1:05 AM, Martin von Zweigbergk <martinvonz@google.com> > wrote: > > > > On Mon, May 14, 2018 at 8:43 PM Matt Harbison <mharbison72@gmail.com> > wrote: > >> # HG changeset patch >> # User Matt Harbison <matt_harbison@yahoo.com> >> # Date 1526353230 14400 >> # Mon May 14 23:00:30 2018 -0400 >> # Node ID b7849cc51d8d8040f233328ee46a6466dff207cd >> # Parent b5d62ec33438171e00030ec1442f91fbf496adf6 >> phabricator: split auth.url into the standard auth.schemes and auth.prefix >> > > Kyle (on CC) noticed earlier today that the fact that the > "phabricator.auth" section has a period in its name means it can't be > specified on the command line using --config (as far as he could tell, > anyway). The timing of this patch made me assume it was in response to that > finding (maybe Kyle had reached out to you, I thought), but it's not about > fixing that issue, as far as I can tell, which confused me quite a bit for > a while :) Anyway, I thought I might at least use this as an opportunity to > make you aware of that issue. > > > Yuya mentioned that over the weekend, so I moved it to [auth]. When he > queued that this morning, he mentioned that [auth] has these fields instead > of a URL. So, this is the follow up to the fix you mentioned. > Aha, that's why the example in your patch said just "[auth]". Great :) Thanks for fixing!
On Mon, 14 May 2018 23:43:42 -0400, Matt Harbison wrote: > # HG changeset patch > # User Matt Harbison <matt_harbison@yahoo.com> > # Date 1526353230 14400 > # Mon May 14 23:00:30 2018 -0400 > # Node ID b7849cc51d8d8040f233328ee46a6466dff207cd > # Parent b5d62ec33438171e00030ec1442f91fbf496adf6 > phabricator: split auth.url into the standard auth.schemes and auth.prefix Queued, thanks.
Patch
diff --git a/contrib/phabricator.py b/contrib/phabricator.py --- a/contrib/phabricator.py +++ b/contrib/phabricator.py @@ -32,7 +32,9 @@ Config:: curlcmd = curl --connect-timeout 2 --retry 3 --silent [auth] - example.url = https://phab.example.com/ + example.schemes = https + example.prefix = phab.example.com + # API token. Get it from https://$HOST/conduit/login/ example.phabtoken = cli-xxxxxxxxxxxxxxxxxxxxxxxxxxxx """ @@ -51,6 +53,7 @@ from mercurial import ( context, encoding, error, + httpconnection as httpconnectionmod, mdiff, obsutil, parser, @@ -135,7 +138,7 @@ def readlegacytoken(repo, url): def readurltoken(repo): """return conduit url, token and make sure they exist - Currently read from [phabricator] config section. In the future, it might + Currently read from [auth] config section. In the future, it might make sense to read from .arcconfig and .arcrc as well. """ url = repo.ui.config('phabricator', 'url') @@ -143,22 +146,15 @@ def readurltoken(repo): raise error.Abort(_('config %s.%s is required') % ('phabricator', 'url')) - groups = {} - for key, val in repo.ui.configitems('auth'): - if '.' not in key: - repo.ui.warn(_("ignoring invalid [auth] key '%s'\n") - % key) - continue - group, setting = key.rsplit('.', 1) - groups.setdefault(group, {})[setting] = val + res = httpconnectionmod.readauthforuri(repo.ui, url, util.url(url).user) + token = None - token = None - for group, auth in groups.iteritems(): - if url != auth.get('url'): - continue + if res: + group, auth = res + + repo.ui.debug("using auth.%s.* for authentication\n" % group) + token = auth.get('phabtoken') - if token: - break if not token: token = readlegacytoken(repo, url)