[MERGE][1.0][Bug 124089] Add root_client_path parameter to SmartWSGIApp, and use it to translate paths received from clients to local paths.

John Arbash Meinel john at arbash-meinel.com
Fri Dec 7 03:42:02 GMT 2007


John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
+
+    def translate_client_path(self, client_path):
+        """Translate a path received from a network client into a local
+        relpath.
+
+        All paths received from the client *must* be translated.
+
+        :returns: a relpath that may be used with 
self._backing_transport
+        """
+        if not client_path.startswith('/'):
+            client_path = '/' + client_path
+        if client_path.startswith(self._root_client_path):
+            path = client_path[len(self._root_client_path):]
+            relpath = urlutils.joinpath('/', path)
+            assert relpath.startswith('/')
+            return '.' + relpath
+        else:
+            raise errors.PathNotChild(client_path, 
self._root_client_path)
+

^- Does this work for Windows paths? The:
If not path.startswith('/'):
   path = '/' + path

seems like it would break for "C:/..."

Same thing for all of the places where you are doing:
-    def __init__(self, sock, backing_transport):
+    def __init__(self, sock, backing_transport, root_client_path='/'):


Actually, now that I think about it, I'm a little concerned about using 
a default path of '/'. Since if someone forgets to pass it from a 
function to another one, then we end up exposing everything from '/' 
when it was meant to be restricted elsewhere.

So what about defaulting to None, and using a DeprecationWarning if it 
is not supplied.


This seems a bit weird:
-            answer = 'yes'
+            default_format = BzrDirFormat.get_default_format()
+            real_bzrdir = default_format.open(t, _found=True)
+            try:
+                real_bzrdir._format.probe_transport(t)
+            except (errors.NotBranchError, errors.UnknownFormatError):
+                answer = 'no'
+            else:
+                answer = 'yes'

You get the default format, to open it on the target location, to probe 
to see if it is really there?

What if the target is not a default format? Is that okay because only 
the default format is supported by remote?


...
=== modified file 'bzrlib/smart/request.py'
--- bzrlib/smart/request.py     2007-10-19 05:38:36 +0000
+++ bzrlib/smart/request.py     2007-12-05 23:30:57 +0000
@@ -24,20 +24,40 @@
      errors,
      registry,
      revision,
+    urlutils,
      )
  from bzrlib.bundle.serializer import write_bundle
+from bzrlib.trace import mutter
+from bzrlib.transport import get_transport
+from bzrlib.transport.chroot import ChrootServer


  class SmartServerRequest(object):
-    """Base class for request handlers."""
+    """Base class for request handlers.
+
+    To define a new request, subclass this class and override the `do` 
method
+    (and if appropriate, `do_body` as well).  Request implementors 
should take
+    care to call `translate_client_path` and 
`transport_from_client_path` as
+    appropriate when dealing with paths received from the client.
+    """

-    def __init__(self, backing_transport):
+    def __init__(self, backing_transport, root_client_path='/'):
          """Constructor.

          :param backing_transport: the base transport to be used when 
performing
              this request.
+        :param root_client_path: the client path that maps to the root 
of
+            backing_transport.  This is used to interpret relpaths 
received
+            from the client.  Clients will not be able to refer to 
paths above
+            this root.
          """
+        rcp = root_client_path
          self._backing_transport = backing_transport
+        if not root_client_path.startswith('/'):
+            root_client_path = '/' + root_client_path
+        if not root_client_path.endswith('/'):
+            root_client_path += '/'
+        self._root_client_path = root_client_path
^- You don't use "rcp" again

Also, the same issue about forcing paths to start with '/' causing 
problems on Windows. I do understand forcing them to end with a '/'.

...
=== modified file 'bzrlib/smart/server.py'
--- bzrlib/smart/server.py      2007-07-20 03:20:20 +0000
+++ bzrlib/smart/server.py      2007-12-05 23:30:57 +0000
@@ -63,6 +63,7 @@
          self.backing_transport = backing_transport
          self._started = threading.Event()
          self._stopped = threading.Event()
+        self.root_client_path = '/'

^- This doesn't quite seem right either.

...
=== modified file 'bzrlib/transport/http/wsgi.py'
--- bzrlib/transport/http/wsgi.py       2007-08-15 19:16:00 +0000
+++ bzrlib/transport/http/wsgi.py       2007-12-05 01:58:00 +0000
@@ -85,11 +85,14 @@
  class SmartWSGIApp(object):
      """A WSGI application for the bzr smart server."""

-    def __init__(self, backing_transport):
+    def __init__(self, backing_transport, root_client_path='/'):
          """Constructor.

          :param backing_transport: a transport.  Requests will be 
processed
              relative to this transport.
+        :param root_client_path: the client path that maps to the root 
of
+            backing_transport.  This is used to interpret relpaths 
received from
+            the client.
          """

^- It would be really nice to have an example here.

Also, as you mentioned, the documentation http_smart_server.txt needs to 
be updated. And it would seem that we need at least a little bit of 
testing that all supported function properly "translate_client_path()". 
It is kind of a shame that each implementation needs to repeat that 
code.

For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C20071206003415.GA23196%40steerpike.home.puzzling.org%3E



More information about the bazaar mailing list