Patchwork phabricator: split auth.url into the standard auth.schemes and auth.prefix

login
register
mail settings
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

Matt Harbison - May 15, 2018, 3:43 a.m.
# 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

It seems better to reuse the existing function to find the proper [auth] block,
even if not all of the possible settings may be of interest.

The other callers of readauthforuri() make a trip through the password database
to fetch the user from the URI.  But in the little experimenting I did here, the
username always came back as None.  Since readauthforuri() wants it to make sure
that user@prefix matches user@url, it seems that parsing the URL and pulling out
the user component should be equivalent.
via Mercurial-devel - May 15, 2018, 5:05 a.m.
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.
Matt Harbison - May 15, 2018, 5:14 a.m.
> 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.
via Mercurial-devel - May 15, 2018, 5:15 a.m.
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!
Yuya Nishihara - May 15, 2018, 11:43 a.m.
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)