Merge lp:~piastucki/bzr-xmloutput/fix-auth into lp:bzr-xmloutput

[Piotr Piastucki]

This commit introduces a custom UIFactory that handles get_password() and get_username() to avoid XMLRPC server hanging on waiting for user's input.
When 'run_bzr' function is invoked ValueError will be thrown whenever get_password() or get_username() is called to avoid server hang and to allow the client to ask the user for credentials.
Additionally, a new function 'run_bzr_auth' was added to make it possible to pass username and password to be used instead of throwing the error.

[Guillermo Gonzalez (verterok)]

The changes looks good.
Could you add some tests checking the behavior of the UIFactory? Also, please could you add tests for at least the 2 cases with the new "auth" command?

Thanks.

review: Needs Fixing

[Piotr Piastucki (piastucki)]

The UIFactory tests are pretty straightforward, however, in order to test new authentication mechanism an FTP/SSH account is needed. I used a publicly available test account on a public FTP server - ftp.secureftp-test.com which has been up&running for a couple of year, but the test will stop working when it is shut down.

[Guillermo Gonzalez (verterok)]

Thanks, but we can't use an external service in the tests.

I'll mark this as approved and change the tests in a different branch.

Thanks!

review: Approve

=== modified file 'service.py'
--- service.py	2011-12-12 15:11:38 +0000
+++ service.py	2013-02-23 13:29:21 +0000
@@ -134,20 +134,27 @@
 @redirect_output
 def run_bzr(argv, workdir):
     """run a regular bzr command"""
-    return _run_bzr(argv, workdir, commands.main)
+    return _run_bzr(argv, workdir, False)
 
 
 @redirect_output
 def run_bzr_xml(argv, workdir):
     """run a bzr command, but handle errors using XMLError"""
-    return _run_bzr(argv, workdir, custom_commands_main)
-
-
-def _run_bzr(argv, workdir, func):
+    return _run_bzr(argv, workdir, True)
+
+@redirect_output
+def run_bzr_xml_auth(argv, workdir, username=None, password=None):
+    """run a bzr command, but handle errors using XMLError"""
+    return _run_bzr(argv, workdir, True, username, password)
+
+def _run_bzr(argv, workdir, iscustom, username=None, password=None):
     """Actually executes the command and build the response."""
     try:
         os.chdir(workdir)
-        exitval = func(argv)
+        if iscustom:
+            exitval = custom_commands_main(argv, username, password)
+        else:
+            exitval = commands.main(argv)
         sys.stderr.flush()
         sys.stdout.flush()
         if isinstance(exitval, Fault):
@@ -165,11 +172,11 @@
         raise
 
 
-def custom_commands_main(argv):
+def custom_commands_main(argv, username=None, password=None):
     """custom commands.main that handle errors using XMLError"""
     import bzrlib.ui
-    bzrlib.ui.ui_factory = bzrlib.ui.make_ui_for_terminal(
-        sys.stdin, sys.stdout, sys.stderr)
+    from uifactory import AuthenticatingUIFactory
+    bzrlib.ui.ui_factory = AuthenticatingUIFactory(sys.stdin, sys.stdout, sys.stderr, username, password)
     try:
         _argv = []
         for a in argv[1:]:
@@ -190,6 +197,7 @@
     """register functions exposed via xmlrpc."""
     server.register_function(run_bzr, 'run_bzr_command')
     server.register_function(run_bzr_xml, 'run_bzr')
+    server.register_function(run_bzr_xml_auth, 'run_bzr_auth')
     import search
     if search.is_available:
         server.register_function(search.search, 'search')
 
=== modified file 'tests/__init__.py'
--- tests/__init__.py	2013-02-16 23:15:33 +0000
+++ tests/__init__.py	2013-02-23 13:29:21 +0000
@@ -33,6 +33,8 @@
         'test_info_xml',
         'test_service',
         'test_tags_xml',
+        'test_auth_service',
+        'test_uifactory',
         ]
     basic_tests.addTest(loader.loadTestsFromModuleNames(
             ["%s.%s" % (__name__, tmn) for tmn in testmod_names]))
 
=== added file 'tests/test_auth_service.py'
--- tests/test_auth_service.py	1970-01-01 00:00:00 +0000
+++ tests/test_auth_service.py	2013-02-23 13:29:21 +0000
@@ -0,0 +1,68 @@
+# -*- encoding: utf-8 -*-
+
+import xmlrpclib
+import threading
+
+from bzrlib import (
+    tests,
+    ui,
+    )
+
+from bzrlib.plugins.xmloutput.service import *
+
+class TestXmlRpcServerAuth(tests.TestCase):
+
+    def setUp(self):
+        tests.TestCase.setUp(self)
+        self.host = 'localhost'
+        self.port = 0
+        self._start_server()
+        self.client = xmlrpclib.Server("http://%s:%s" % (self.host,
+                                                         str(self.port)))
+    def _start_server(self):
+        self.server =  BzrXMLRPCServer((self.host, self.port))
+        self.thread = threading.Thread(target=self.server.serve_forever)
+        self.thread.setDaemon(True)
+        self.thread.start()
+        self.host, self.port = self.server.socket.getsockname()
+
+    def tearDown(self):
+        response = self.client.quit()
+        self.thread.join()
+        tests.TestCase.tearDown(self)
+
+    def test_hello(self):
+        response = self.client.hello()
+        self.assertEquals(response, "world!")
+
+    def test_run_bzr_auth(self):
+        self.disable_directory_isolation()
+        exit, out, err = self.client.run_bzr_auth(['bzr', 'xmlplugins'], '.')
+        self.assertEquals(exit, 0)
+        self.assertEquals(err, "")
+        self.assertNotEquals(out.data, "")
+
+    def test_error_instead_of_hanging(self):
+        self.disable_directory_isolation()
+        exit, out, err = self.client.run_bzr(['bzr', 'xmlls', 'ftp://[e-mail]'], '.')
+        self.assertNotEquals(exit, 0)
+        self.assertContainsRe(err, "XMLRPC authentication password:")
+        self.assertNotEquals(out.data, "")
+        exit, out, err = self.client.run_bzr_auth(['bzr', 'xmlls', 'ftp://[e-mail]'], '.')
+        self.assertNotEquals(exit, 0)
+        self.assertContainsRe(err, "XMLRPC authentication password:")
+        self.assertNotEquals(out.data, "")
+
+    def test_successful_authentication_flow(self):
+        self.disable_directory_isolation()
+        exit, out, err = self.client.run_bzr(['bzr', 'xmlls', 'ftp://[e-mail]'], '.')
+        self.assertNotEquals(exit, 0)
+        self.assertContainsRe(err, "XMLRPC authentication password:")
+        self.assertNotEquals(out.data, "")
+        exit, out, err = self.client.run_bzr_auth(['bzr', 'xmlls', 'ftp://[e-mail]'], '.',\
+                                                  'test', 'test')
+        self.assertNotEquals(exit, 0)
+        self.assertContainsRe(err, "Not a branch")
+        self.assertNotEquals(out.data, "")
+
+
 
=== added file 'tests/test_uifactory.py'
--- tests/test_uifactory.py	1970-01-01 00:00:00 +0000
+++ tests/test_uifactory.py	2013-02-23 13:29:21 +0000
@@ -0,0 +1,23 @@
+# -*- encoding: utf-8 -*-
+
+import sys
+from bzrlib import (
+    tests,
+    ui,
+    )
+from bzrlib.plugins.xmloutput.uifactory import AuthenticatingUIFactory
+
+class TestAuthenticatingUIFactory(tests.TestCase):
+
+    def test_error_throwing(self):
+        uif = AuthenticatingUIFactory(sys.stdin, sys.stdout, sys.stderr)
+        self.assertRaises(ValueError, uif.get_username, ('login'))
+        self.assertRaises(ValueError, uif.get_password, ('password'))
+
+    def test_canned_values(self):
+        uif = AuthenticatingUIFactory(sys.stdin, sys.stdout, sys.stderr, username='user123', password='pass123')
+        t = uif.get_username('login')
+        self.assertEquals(t, 'user123')
+        t = uif.get_password('password')
+        self.assertEquals(t, 'pass123')
+
 
=== added file 'uifactory.py'
--- uifactory.py	1970-01-01 00:00:00 +0000
+++ uifactory.py	2013-02-23 13:29:21 +0000
@@ -0,0 +1,51 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+#
+# Copyright (C) 2013 Piotr Piastucki
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+
+from bzrlib import ui
+from bzrlib.ui.text import TextUIFactory
+
+class AuthenticatingUIFactory(TextUIFactory):
+
+    def __init__(self,
+                 stdin=None,
+                 stdout=None,
+                 stderr=None,
+                 username=None,
+                 password=None):
+        super(AuthenticatingUIFactory, self).__init__(stdin, stdout, stderr)
+        self.username = username
+        self.password = password
+
+    def get_prompt_text(self, prompt=u'', **kwargs):
+        if type(prompt) != unicode:
+            raise ValueError("prompt %r not a unicode string" % prompt)
+        if kwargs:
+            prompt = prompt % kwargs
+        return prompt
+
+    def get_password(self, prompt=u'', **kwargs):
+        if self.password is None:
+            raise ValueError('XMLRPC authentication password:' + self.get_prompt_text(prompt, **kwargs))
+        return self.password
+
+    def get_username(self, prompt, **kwargs):
+        if self.username is None:
+            raise ValueError('XMLRPC authentication username:' + self.get_prompt_text(prompt, **kwargs))
+        return self.username
+