Patchwork D2231: narrow: fix for getting the username when running http server

login
register
mail settings
Submitter phabricator
Date Feb. 13, 2018, 7:40 p.m.
Message ID <differential-rev-PHID-DREV-cindb7rezfah66ywwkpd-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27840/
State Superseded
Headers show

Comments

phabricator - Feb. 13, 2018, 7:40 p.m.
idlsoft created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2231

AFFECTED FILES
  hgext/narrow/narrowbundle2.py

CHANGE DETAILS




To: idlsoft, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 14, 2018, 6:03 a.m.
indygreg added inline comments.

INLINE COMMENTS

> narrowbundle2.py:331
> +    ui = repo.ui
> +    username = ui.shortuser(ui.environ.get('REMOTE_USER') or ui.username())
> +    user_includes = ui.configlist(

`REMOTE_USER` is an HTTP-ism and I think it is a layering violation to look for `REMOTE_USER` in code that is supposed to be protocol agnostic.

What I'm trying to say is I think we'll need to tweak this ACL code further when it is moved to core.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2231

To: idlsoft, #hg-reviewers, durin42
Cc: indygreg, mercurial-devel
phabricator - Feb. 15, 2018, 10:47 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> indygreg wrote in narrowbundle2.py:331
> `REMOTE_USER` is an HTTP-ism and I think it is a layering violation to look for `REMOTE_USER` in code that is supposed to be protocol agnostic.
> 
> What I'm trying to say is I think we'll need to tweak this ACL code further when it is moved to core.

But you're okay with this version for now? This commit is currently the bottom-most draft commit in the "committed" repo, so please accept it if you're okay with it. I would have accepted, but I saw this comment and wasn't sure if you thought this was bad enough to not queue it.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2231

To: idlsoft, #hg-reviewers, durin42
Cc: martinvonz, indygreg, mercurial-devel
phabricator - Feb. 15, 2018, 11:32 p.m.
indygreg added a comment.


  I think it's fine for now. I accepted this commit.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2231

To: idlsoft, #hg-reviewers, durin42
Cc: martinvonz, indygreg, mercurial-devel
phabricator - Feb. 16, 2018, 12:01 a.m.
idlsoft added inline comments.

INLINE COMMENTS

> martinvonz wrote in narrowbundle2.py:331
> But you're okay with this version for now? This commit is currently the bottom-most draft commit in the "committed" repo, so please accept it if you're okay with it. I would have accepted, but I saw this comment and wasn't sure if you thought this was bad enough to not queue it.

Ideally I would have liked to have a `userid` method in repo.ui (or something like that), and have it used by both `narrow`  and `acl` extension.
But I think this deserves a longer discussion, hence the quick fix.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2231

To: idlsoft, #hg-reviewers, durin42
Cc: martinvonz, indygreg, mercurial-devel

Patch

diff --git a/hgext/narrow/narrowbundle2.py b/hgext/narrow/narrowbundle2.py
--- a/hgext/narrow/narrowbundle2.py
+++ b/hgext/narrow/narrowbundle2.py
@@ -327,13 +327,14 @@ 
             part.addparam('treemanifest', '1')
 
 def applyacl_narrow(repo, kwargs):
-    username = repo.ui.shortuser(repo.ui.username())
-    user_includes = repo.ui.configlist(
+    ui = repo.ui
+    username = ui.shortuser(ui.environ.get('REMOTE_USER') or ui.username())
+    user_includes = ui.configlist(
         _NARROWACL_SECTION, username + '.includes',
-        repo.ui.configlist(_NARROWACL_SECTION, 'default.includes'))
-    user_excludes = repo.ui.configlist(
+        ui.configlist(_NARROWACL_SECTION, 'default.includes'))
+    user_excludes = ui.configlist(
         _NARROWACL_SECTION, username + '.excludes',
-        repo.ui.configlist(_NARROWACL_SECTION, 'default.excludes'))
+        ui.configlist(_NARROWACL_SECTION, 'default.excludes'))
     if not user_includes:
         raise error.Abort(_("{} configuration for user {} is empty")
                           .format(_NARROWACL_SECTION, username))