Patchwork [STABLE] cmdserver: forcibly use L channel to read password input (issue3161)

login
register
mail settings
Submitter Yuya Nishihara
Date April 27, 2014, 8:11 a.m.
Message ID <6a43c60ad9af8d752474.1398586262@gimlet>
Download mbox | patch
Permalink /patch/4443/
State Accepted
Commit 9336bc7dca8e231ec13c5a57436a4f59370c052d
Headers show

Comments

Yuya Nishihara - April 27, 2014, 8:11 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1398503586 -32400
#      Sat Apr 26 18:13:06 2014 +0900
# Branch stable
# Node ID 6a43c60ad9af8d7524740a9ae185970759235eab
# Parent  faf1ce941b0cc2e699929d33ac494b5e934146c4
cmdserver: forcibly use L channel to read password input (issue3161)

Command server is designed to use the channel protocol even if the server
process is accessible to tty, whereas vanilla hg should be able to read
password from tty in that case.  So it isn't enough to swap sys.stdin:

    # works only if the server process is detached from the console
    sys.stdin = self.fin
    getpass.getpass('')
    sys.stdin = oldin

or test isatty:

    # vanilla hg can't talk to tty if stdin is redirected
    if self._isatty(self.fin):
        return getpass.getpass('')
    else:
        ...

Since ui.nontty flag is undocumented and command-server channels don't provide
isatty(), this change won't affect the other uses of ui._isatty().

issue3161 also suggests to provide some context of messages.  I think it can
be implemented by using the generic templating function.
Augie Fackler - April 29, 2014, 3:20 p.m.
On Sun, Apr 27, 2014 at 05:11:02PM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1398503586 -32400
> #      Sat Apr 26 18:13:06 2014 +0900
> # Branch stable
> # Node ID 6a43c60ad9af8d7524740a9ae185970759235eab
> # Parent  faf1ce941b0cc2e699929d33ac494b5e934146c4
> cmdserver: forcibly use L channel to read password input (issue3161)

This all looks reasonable to me on a high-level pass, but I'll defer
to mpm as to wether it belongs in stable this late in the cycle.

>
> Command server is designed to use the channel protocol even if the server
> process is accessible to tty, whereas vanilla hg should be able to read
> password from tty in that case.  So it isn't enough to swap sys.stdin:
>
>     # works only if the server process is detached from the console
>     sys.stdin = self.fin
>     getpass.getpass('')
>     sys.stdin = oldin
>
> or test isatty:
>
>     # vanilla hg can't talk to tty if stdin is redirected
>     if self._isatty(self.fin):
>         return getpass.getpass('')
>     else:
>         ...
>
> Since ui.nontty flag is undocumented and command-server channels don't provide
> isatty(), this change won't affect the other uses of ui._isatty().
>
> issue3161 also suggests to provide some context of messages.  I think it can
> be implemented by using the generic templating function.
>
> diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> --- a/mercurial/commandserver.py
> +++ b/mercurial/commandserver.py
> @@ -187,14 +187,20 @@ class server(object):
>          # copy the uis so changes (e.g. --config or --verbose) don't
>          # persist between requests
>          copiedui = self.ui.copy()
> +        uis = [copiedui]
>          if self.repo:
>              self.repo.baseui = copiedui
>              # clone ui without using ui.copy because this is protected
>              repoui = self.repoui.__class__(self.repoui)
>              repoui.copy = copiedui.copy # redo copy protection
> +            uis.append(repoui)
>              self.repo.ui = self.repo.dirstate._ui = repoui
>              self.repo.invalidateall()
>
> +        for ui in uis:
> +            # any kind of interaction must use server channels
> +            ui.setconfig('ui', 'nontty', 'true', 'commandserver')
> +
>          req = dispatch.request(args[:], copiedui, self.repo, self.cin,
>                                 self.cout, self.cerr)
>
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -689,7 +689,12 @@ class ui(object):
>              return default
>          try:
>              self.write_err(self.label(prompt or _('password: '), 'ui.prompt'))
> -            return getpass.getpass('')
> +            # disable getpass() only if explicitly specified. it's still valid
> +            # to interact with tty even if fin is not a tty.
> +            if self.configbool('ui', 'nontty'):
> +                return self.fin.readline().rstrip('\n')
> +            else:
> +                return getpass.getpass('')
>          except EOFError:
>              raise util.Abort(_('response expected'))
>      def status(self, *msg, **opts):
> diff --git a/tests/test-commandserver.py b/tests/test-commandserver.py
> --- a/tests/test-commandserver.py
> +++ b/tests/test-commandserver.py
> @@ -294,6 +294,11 @@ def mqoutsidechanges(server):
>      # repo.mq should be recreated to point to new queue
>      runcommand(server, ['qqueue', '--active'])
>
> +def getpass(server):
> +    readchannel(server)
> +    runcommand(server, ['debuggetpass', '--config', 'ui.interactive=True'],
> +               input=cStringIO.StringIO('1234\n'))
> +
>  def startwithoutrepo(server):
>      readchannel(server)
>      runcommand(server, ['init', 'repo2'])
> @@ -334,6 +339,19 @@ if __name__ == '__main__':
>      hgrc.write('[extensions]\nmq=\n')
>      hgrc.close()
>      check(mqoutsidechanges)
> +    dbg = open('dbgui.py', 'w')
> +    dbg.write('from mercurial import cmdutil, commands\n'
> +              'commands.norepo += " debuggetpass"\n'
> +              'cmdtable = {}\n'
> +              'command = cmdutil.command(cmdtable)\n'
> +              '@command("debuggetpass")\n'
> +              'def debuggetpass(ui):\n'
> +              '    ui.write("%s\\n" % ui.getpass())\n')
> +    dbg.close()
> +    hgrc = open('.hg/hgrc', 'a')
> +    hgrc.write('[extensions]\ndbgui=dbgui.py\n')
> +    hgrc.close()
> +    check(getpass)
>
>      os.chdir('..')
>      check(hellomessage)
> diff --git a/tests/test-commandserver.py.out b/tests/test-commandserver.py.out
> --- a/tests/test-commandserver.py.out
> +++ b/tests/test-commandserver.py.out
> @@ -81,6 +81,7 @@ defaults.tag=-d "0 0"
>  ui.slash=True
>  ui.interactive=False
>  ui.foo=bar
> +ui.nontty=true
>   runcommand init foo
>   runcommand -R foo showconfig ui defaults
>  defaults.backout=-d "0 0"
> @@ -89,6 +90,7 @@ defaults.shelve=--date "0 0"
>  defaults.tag=-d "0 0"
>  ui.slash=True
>  ui.interactive=False
> +ui.nontty=true
>
>  testing hookoutput:
>
> @@ -238,6 +240,11 @@ patch queue now empty
>   runcommand qqueue --active
>  foo
>
> +testing getpass:
> +
> + runcommand debuggetpass --config ui.interactive=True
> +password: 1234
> +
>  testing hellomessage:
>
>  o, 'capabilities: getencoding runcommand\nencoding: ***'
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - April 29, 2014, 7:02 p.m.
On Sun, 2014-04-27 at 17:11 +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1398503586 -32400
> #      Sat Apr 26 18:13:06 2014 +0900
> # Branch stable
> # Node ID 6a43c60ad9af8d7524740a9ae185970759235eab
> # Parent  faf1ce941b0cc2e699929d33ac494b5e934146c4
> cmdserver: forcibly use L channel to read password input (issue3161)

Queued for stable, thanks.

Patch

diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -187,14 +187,20 @@  class server(object):
         # copy the uis so changes (e.g. --config or --verbose) don't
         # persist between requests
         copiedui = self.ui.copy()
+        uis = [copiedui]
         if self.repo:
             self.repo.baseui = copiedui
             # clone ui without using ui.copy because this is protected
             repoui = self.repoui.__class__(self.repoui)
             repoui.copy = copiedui.copy # redo copy protection
+            uis.append(repoui)
             self.repo.ui = self.repo.dirstate._ui = repoui
             self.repo.invalidateall()
 
+        for ui in uis:
+            # any kind of interaction must use server channels
+            ui.setconfig('ui', 'nontty', 'true', 'commandserver')
+
         req = dispatch.request(args[:], copiedui, self.repo, self.cin,
                                self.cout, self.cerr)
 
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -689,7 +689,12 @@  class ui(object):
             return default
         try:
             self.write_err(self.label(prompt or _('password: '), 'ui.prompt'))
-            return getpass.getpass('')
+            # disable getpass() only if explicitly specified. it's still valid
+            # to interact with tty even if fin is not a tty.
+            if self.configbool('ui', 'nontty'):
+                return self.fin.readline().rstrip('\n')
+            else:
+                return getpass.getpass('')
         except EOFError:
             raise util.Abort(_('response expected'))
     def status(self, *msg, **opts):
diff --git a/tests/test-commandserver.py b/tests/test-commandserver.py
--- a/tests/test-commandserver.py
+++ b/tests/test-commandserver.py
@@ -294,6 +294,11 @@  def mqoutsidechanges(server):
     # repo.mq should be recreated to point to new queue
     runcommand(server, ['qqueue', '--active'])
 
+def getpass(server):
+    readchannel(server)
+    runcommand(server, ['debuggetpass', '--config', 'ui.interactive=True'],
+               input=cStringIO.StringIO('1234\n'))
+
 def startwithoutrepo(server):
     readchannel(server)
     runcommand(server, ['init', 'repo2'])
@@ -334,6 +339,19 @@  if __name__ == '__main__':
     hgrc.write('[extensions]\nmq=\n')
     hgrc.close()
     check(mqoutsidechanges)
+    dbg = open('dbgui.py', 'w')
+    dbg.write('from mercurial import cmdutil, commands\n'
+              'commands.norepo += " debuggetpass"\n'
+              'cmdtable = {}\n'
+              'command = cmdutil.command(cmdtable)\n'
+              '@command("debuggetpass")\n'
+              'def debuggetpass(ui):\n'
+              '    ui.write("%s\\n" % ui.getpass())\n')
+    dbg.close()
+    hgrc = open('.hg/hgrc', 'a')
+    hgrc.write('[extensions]\ndbgui=dbgui.py\n')
+    hgrc.close()
+    check(getpass)
 
     os.chdir('..')
     check(hellomessage)
diff --git a/tests/test-commandserver.py.out b/tests/test-commandserver.py.out
--- a/tests/test-commandserver.py.out
+++ b/tests/test-commandserver.py.out
@@ -81,6 +81,7 @@  defaults.tag=-d "0 0"
 ui.slash=True
 ui.interactive=False
 ui.foo=bar
+ui.nontty=true
  runcommand init foo
  runcommand -R foo showconfig ui defaults
 defaults.backout=-d "0 0"
@@ -89,6 +90,7 @@  defaults.shelve=--date "0 0"
 defaults.tag=-d "0 0"
 ui.slash=True
 ui.interactive=False
+ui.nontty=true
 
 testing hookoutput:
 
@@ -238,6 +240,11 @@  patch queue now empty
  runcommand qqueue --active
 foo
 
+testing getpass:
+
+ runcommand debuggetpass --config ui.interactive=True
+password: 1234
+
 testing hellomessage:
 
 o, 'capabilities: getencoding runcommand\nencoding: ***'