Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Files beneath base #9

Merged
merged 4 commits into from

2 participants

@allenap

This is the solution to issue #6 that we're shipping right now to support MAAS. It takes the approach that writes to a non-existent directory are permitted, with the directory being created just in time. People using python-tx-tftp could choose to subclass the default writer to change that behaviour, but actually I don't mind what the policy is, because we're not using it for writes.

@allenap

I wrote this a while ago, before I knew that you preferred to avoid inlineCallbacks. If you like the direction of the rest of the code I'll rewrite the couple of tests I've used it on.

@shylent shylent commented on the diff
tftp/backend.py
@@ -267,7 +270,7 @@ def get_reader(self, file_name):
if not self.can_read:
raise Unsupported("Reading not supported")
try:
- target_path = self.base.child(file_name)
+ target_path = self.base.descendant(file_name.split("/"))
@shylent Owner
shylent added a note

I think, its better to avoid using the string's "split" method on filesystem paths, since there is a specialized method for that. This also (AFAIK) breaks this code for Windows.
I would write this line as target_path = self.base.preauthChild(file_name) (see http://twistedmatrix.com/documents/current/api/twisted.python.filepath.FilePath.html#preauthChild)

@allenap
allenap added a note

The path it's splitting is the one given over the wire, not necessarily a local filesystem path, which I've assumed uses forward slashes as directory separators. Do Windows clients request paths with backslash separators?

@shylent Owner
shylent added a note

AFAIK, "filename" is an opaque string according to TFTP spec (or at least it does not contradict this statement). So the client may pass whatever separator he feels like. Seems, that they are actually quite likely to use backslash as a separator (WinPE PXE boot comes to mind).

Nevertheless, I don't think, that this should stop us from merging this. If the need arises to translate backslash-separated paths into unix-style paths, it should be implemented separately as a new feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shylent shylent commented on the diff
tftp/backend.py
@@ -283,7 +286,7 @@ def get_writer(self, file_name):
if not self.can_write:
raise Unsupported("Writing not supported")
try:
- target_path = self.base.child(file_name)
+ target_path = self.base.descendant(file_name.split("/"))
@shylent Owner
shylent added a note

Same thing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shylent
Owner

Hope you don't mind my comments on the code. As for inlineCallbacks, well, there already are some tests employing this technique (even in the very module you've changed - tftp.test.test_backend), so, while I am kind of against using inlineCallbacks, there's nothing to do about it in this pull request.

@allenap

Hope you don't mind my comments on the code.

Heh, of course not. I'm just sorry that I've taken a long time to respond.

@allenap

shylent, can you look at this again? Fwiw, python-tx-tftp in Ubuntu (12.10 and after, proposed for 12.04) ships with this patch.

@shylent shylent merged commit 9adf983 into shylent:master
@allenap allenap deleted the allenap:files-beneath-base branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 43 additions and 20 deletions.
  1. +5 −2 tftp/backend.py
  2. +38 −18 tftp/test/test_backend.py
View
7 tftp/backend.py
@@ -194,6 +194,9 @@ class FilesystemWriter(object):
def __init__(self, file_path):
if file_path.exists():
raise FileExists(file_path)
+ file_dir = file_path.parent()
+ if not file_dir.exists():
+ file_dir.makedirs()
self.file_path = file_path
self.destination_file = self.file_path.open('w')
self.temp_destination = tempfile.TemporaryFile()
@@ -267,7 +270,7 @@ def get_reader(self, file_name):
if not self.can_read:
raise Unsupported("Reading not supported")
try:
- target_path = self.base.child(file_name)
+ target_path = self.base.descendant(file_name.split("/"))
@shylent Owner
shylent added a note

I think, its better to avoid using the string's "split" method on filesystem paths, since there is a specialized method for that. This also (AFAIK) breaks this code for Windows.
I would write this line as target_path = self.base.preauthChild(file_name) (see http://twistedmatrix.com/documents/current/api/twisted.python.filepath.FilePath.html#preauthChild)

@allenap
allenap added a note

The path it's splitting is the one given over the wire, not necessarily a local filesystem path, which I've assumed uses forward slashes as directory separators. Do Windows clients request paths with backslash separators?

@shylent Owner
shylent added a note

AFAIK, "filename" is an opaque string according to TFTP spec (or at least it does not contradict this statement). So the client may pass whatever separator he feels like. Seems, that they are actually quite likely to use backslash as a separator (WinPE PXE boot comes to mind).

Nevertheless, I don't think, that this should stop us from merging this. If the need arises to translate backslash-separated paths into unix-style paths, it should be implemented separately as a new feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
except InsecurePath, e:
raise AccessViolation("Insecure path: %s" % e)
return FilesystemReader(target_path)
@@ -283,7 +286,7 @@ def get_writer(self, file_name):
if not self.can_write:
raise Unsupported("Writing not supported")
try:
- target_path = self.base.child(file_name)
+ target_path = self.base.descendant(file_name.split("/"))
@shylent Owner
shylent added a note

Same thing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
except InsecurePath, e:
raise AccessViolation("Insecure path: %s" % e)
return FilesystemWriter(target_path)
View
56 tftp/test/test_backend.py
@@ -7,7 +7,6 @@
from twisted.python.filepath import FilePath
from twisted.internet.defer import inlineCallbacks
from twisted.trial import unittest
-import os.path
import shutil
import tempfile
@@ -20,43 +19,57 @@ class BackendSelection(unittest.TestCase):
def setUp(self):
- self.temp_dir = tempfile.mkdtemp()
- self.existing_file_name = os.path.join(self.temp_dir, 'foo')
- with open(self.existing_file_name, 'w') as f:
- f.write(self.test_data)
+ self.temp_dir = FilePath(tempfile.mkdtemp())
+ self.existing_file_name = self.temp_dir.descendant(("dir", "foo"))
+ self.existing_file_name.parent().makedirs()
+ self.existing_file_name.setContent(self.test_data)
@inlineCallbacks
def test_read_supported_by_default(self):
- b = FilesystemSynchronousBackend(self.temp_dir)
- reader = yield b.get_reader('foo')
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
+ reader = yield b.get_reader('dir/foo')
self.assertTrue(IReader.providedBy(reader))
@inlineCallbacks
def test_write_supported_by_default(self):
- b = FilesystemSynchronousBackend(self.temp_dir)
- writer = yield b.get_writer('bar')
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
+ writer = yield b.get_writer('dir/bar')
self.assertTrue(IWriter.providedBy(writer))
def test_read_unsupported(self):
- b = FilesystemSynchronousBackend(self.temp_dir, can_read=False)
- return self.assertFailure(b.get_reader('foo'), Unsupported)
+ b = FilesystemSynchronousBackend(self.temp_dir.path, can_read=False)
+ return self.assertFailure(b.get_reader('dir/foo'), Unsupported)
def test_write_unsupported(self):
- b = FilesystemSynchronousBackend(self.temp_dir, can_write=False)
- return self.assertFailure(b.get_writer('bar'), Unsupported)
+ b = FilesystemSynchronousBackend(self.temp_dir.path, can_write=False)
+ return self.assertFailure(b.get_writer('dir/bar'), Unsupported)
def test_insecure_reader(self):
- b = FilesystemSynchronousBackend(self.temp_dir)
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
return self.assertFailure(
b.get_reader('../foo'), AccessViolation)
def test_insecure_writer(self):
- b = FilesystemSynchronousBackend(self.temp_dir)
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
return self.assertFailure(
b.get_writer('../foo'), AccessViolation)
+ @inlineCallbacks
+ def test_read_ignores_leading_and_trailing_slashes(self):
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
+ reader = yield b.get_reader('/dir/foo/')
+ segments_from_root = reader.file_path.segmentsFrom(self.temp_dir)
+ self.assertEqual(["dir", "foo"], segments_from_root)
+
+ @inlineCallbacks
+ def test_write_ignores_leading_and_trailing_slashes(self):
+ b = FilesystemSynchronousBackend(self.temp_dir.path)
+ writer = yield b.get_writer('/dir/bar/')
+ segments_from_root = writer.file_path.segmentsFrom(self.temp_dir)
+ self.assertEqual(["dir", "bar"], segments_from_root)
+
def tearDown(self):
- shutil.rmtree(self.temp_dir)
+ shutil.rmtree(self.temp_dir.path)
class Reader(unittest.TestCase):
@@ -126,12 +139,19 @@ class Writer(unittest.TestCase):
def setUp(self):
self.temp_dir = FilePath(tempfile.mkdtemp())
self.existing_file_name = self.temp_dir.child('foo')
- with self.existing_file_name.open('w') as f:
- f.write(self.test_data)
+ self.existing_file_name.setContent(self.test_data)
def test_write_existing_file(self):
self.assertRaises(FileExists, FilesystemWriter, self.temp_dir.child('foo'))
+ def test_write_to_non_existent_directory(self):
+ new_directory = self.temp_dir.child("new")
+ new_file = new_directory.child("baz")
+ self.assertFalse(new_directory.exists())
+ FilesystemWriter(new_file).finish()
+ self.assertTrue(new_directory.exists())
+ self.assertTrue(new_file.exists())
+
def test_finished_write(self):
w = FilesystemWriter(self.temp_dir.child('bar'))
w.write(self.test_data)
Something went wrong with that request. Please try again.