From 6b8d4a444bd747a8942c62c9066c797cc2ec36a1 Mon Sep 17 00:00:00 2001 From: FemtosecondLaser <38204088+FemtosecondLaser@users.noreply.github.com> Date: Wed, 20 Oct 2021 15:26:16 +0100 Subject: [PATCH 1/6] Modified ensure_directory_exists() to check if the directory is writable by the process. --- lbry/extras/cli.py | 3 +++ tests/unit/test_cli.py | 46 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/lbry/extras/cli.py b/lbry/extras/cli.py index c263a84d9..a33543faf 100644 --- a/lbry/extras/cli.py +++ b/lbry/extras/cli.py @@ -226,6 +226,9 @@ def get_argument_parser(): def ensure_directory_exists(path: str): if not os.path.isdir(path): pathlib.Path(path).mkdir(parents=True, exist_ok=True) + use_effective_ids = os.access in os.supports_effective_ids + if not os.access(path, os.W_OK, effective_ids=use_effective_ids): + raise PermissionError(f"The following directory is not writable: {path}") LOG_MODULES = 'lbry', 'aioupnp' diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 935364f05..2f9b7219c 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -3,16 +3,18 @@ import tempfile import shutil import contextlib import logging +import pathlib from io import StringIO from unittest import TestCase from unittest.mock import patch +from pyfakefs.fake_filesystem_unittest import TestCase as FakeFSTestCase from types import SimpleNamespace from contextlib import asynccontextmanager import docopt from lbry.testcase import AsyncioTestCase -from lbry.extras.cli import normalize_value, main, setup_logging +from lbry.extras.cli import normalize_value, main, setup_logging, ensure_directory_exists from lbry.extras.system_info import get_platform from lbry.extras.daemon.daemon import Daemon from lbry.conf import Config @@ -202,3 +204,45 @@ class DaemonDocsTests(TestCase): pass if failures: self.fail("\n" + "\n".join(failures)) + + +class DirectoryTests(FakeFSTestCase): + + def setUp(self): + self.setUpPyfakefs() + + def test_when_dir_does_not_exist_then_it_is_created(self): + dir_path = "dir" + ensure_directory_exists(dir_path) + self.assertTrue(os.path.exists(dir_path)) + + def test_when_parent_dir_does_not_exist_then_dir_is_created_with_parent(self): + dir_path = os.path.join("parent_dir", "dir") + ensure_directory_exists(dir_path) + self.assertTrue(os.path.exists(dir_path)) + + def test_when_dir_exists_then_it_still_exists(self): + dir_path = "dir" + pathlib.Path(dir_path).mkdir() + ensure_directory_exists(dir_path) + self.assertTrue(os.path.exists(dir_path)) + + def test_when_non_writable_dir_exists_then_raise(self): + dir_path = "dir" + pathlib.Path(dir_path).mkdir(mode=0o555) # creates a non-writable, readable and executable dir + with self.assertRaises(PermissionError): + ensure_directory_exists(dir_path) + + def test_when_dir_exists_and_writable_then_no_raise(self): + dir_path = "dir" + pathlib.Path(dir_path).mkdir(mode=0o777) # creates a writable, readable and executable dir + try: + ensure_directory_exists(dir_path) + except (FileExistsError, PermissionError) as err: + self.fail(f"{type(err).__name__} was raised") + + def test_when_file_exists_at_path_then_raise(self): + file_path = "file.extension" + self.fs.create_file(file_path) + with self.assertRaises(FileExistsError): + ensure_directory_exists(file_path) From 9c5f5aefb0cef222c2b70cc44fe805130461a343 Mon Sep 17 00:00:00 2001 From: FemtosecondLaser <38204088+FemtosecondLaser@users.noreply.github.com> Date: Thu, 21 Oct 2021 00:27:31 +0100 Subject: [PATCH 2/6] removed redundant tests renamed a test to be more specific about the kind of the precondition --- tests/unit/test_cli.py | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 2f9b7219c..3fa6550c5 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -211,22 +211,11 @@ class DirectoryTests(FakeFSTestCase): def setUp(self): self.setUpPyfakefs() - def test_when_dir_does_not_exist_then_it_is_created(self): - dir_path = "dir" - ensure_directory_exists(dir_path) - self.assertTrue(os.path.exists(dir_path)) - def test_when_parent_dir_does_not_exist_then_dir_is_created_with_parent(self): dir_path = os.path.join("parent_dir", "dir") ensure_directory_exists(dir_path) self.assertTrue(os.path.exists(dir_path)) - def test_when_dir_exists_then_it_still_exists(self): - dir_path = "dir" - pathlib.Path(dir_path).mkdir() - ensure_directory_exists(dir_path) - self.assertTrue(os.path.exists(dir_path)) - def test_when_non_writable_dir_exists_then_raise(self): dir_path = "dir" pathlib.Path(dir_path).mkdir(mode=0o555) # creates a non-writable, readable and executable dir @@ -241,7 +230,7 @@ class DirectoryTests(FakeFSTestCase): except (FileExistsError, PermissionError) as err: self.fail(f"{type(err).__name__} was raised") - def test_when_file_exists_at_path_then_raise(self): + def test_when_non_dir_file_exists_at_path_then_raise(self): file_path = "file.extension" self.fs.create_file(file_path) with self.assertRaises(FileExistsError): From 837f91d830c73c0978bca75ec7f28418186046c3 Mon Sep 17 00:00:00 2001 From: FemtosecondLaser <38204088+FemtosecondLaser@users.noreply.github.com> Date: Thu, 21 Oct 2021 00:31:02 +0100 Subject: [PATCH 3/6] renamed the test class to be more specific about the sut --- tests/unit/test_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 3fa6550c5..287e476d2 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -206,7 +206,7 @@ class DaemonDocsTests(TestCase): self.fail("\n" + "\n".join(failures)) -class DirectoryTests(FakeFSTestCase): +class EnsureDirectoryExistsTests(FakeFSTestCase): def setUp(self): self.setUpPyfakefs() From 2b5838aa011e19c7dfde58ea5dfb72e0ed849d3f Mon Sep 17 00:00:00 2001 From: FemtosecondLaser <38204088+FemtosecondLaser@users.noreply.github.com> Date: Sat, 23 Oct 2021 02:52:58 +0100 Subject: [PATCH 4/6] Changed the tests to execute against a real file system instead of a fake one. --- tests/unit/test_cli.py | 45 +++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 287e476d2..55c12ea8c 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -7,7 +7,6 @@ import pathlib from io import StringIO from unittest import TestCase from unittest.mock import patch -from pyfakefs.fake_filesystem_unittest import TestCase as FakeFSTestCase from types import SimpleNamespace from contextlib import asynccontextmanager @@ -206,32 +205,34 @@ class DaemonDocsTests(TestCase): self.fail("\n" + "\n".join(failures)) -class EnsureDirectoryExistsTests(FakeFSTestCase): - - def setUp(self): - self.setUpPyfakefs() +class EnsureDirectoryExistsTests(TestCase): def test_when_parent_dir_does_not_exist_then_dir_is_created_with_parent(self): - dir_path = os.path.join("parent_dir", "dir") - ensure_directory_exists(dir_path) - self.assertTrue(os.path.exists(dir_path)) + with tempfile.TemporaryDirectory() as temp_dir: + dir_path = os.path.join(temp_dir, "parent_dir", "dir") + ensure_directory_exists(dir_path) + self.assertTrue(os.path.exists(dir_path)) def test_when_non_writable_dir_exists_then_raise(self): - dir_path = "dir" - pathlib.Path(dir_path).mkdir(mode=0o555) # creates a non-writable, readable and executable dir - with self.assertRaises(PermissionError): - ensure_directory_exists(dir_path) + with tempfile.TemporaryDirectory() as temp_dir: + dir_path = os.path.join(temp_dir, "dir") + pathlib.Path(dir_path).mkdir(mode=0o555) # creates a non-writable, readable and executable dir + with self.assertRaises(PermissionError): + ensure_directory_exists(dir_path) def test_when_dir_exists_and_writable_then_no_raise(self): - dir_path = "dir" - pathlib.Path(dir_path).mkdir(mode=0o777) # creates a writable, readable and executable dir - try: - ensure_directory_exists(dir_path) - except (FileExistsError, PermissionError) as err: - self.fail(f"{type(err).__name__} was raised") + with tempfile.TemporaryDirectory() as temp_dir: + dir_path = os.path.join(temp_dir, "dir") + pathlib.Path(dir_path).mkdir(mode=0o777) # creates a writable, readable and executable dir + try: + ensure_directory_exists(dir_path) + except (FileExistsError, PermissionError) as err: + self.fail(f"{type(err).__name__} was raised") def test_when_non_dir_file_exists_at_path_then_raise(self): - file_path = "file.extension" - self.fs.create_file(file_path) - with self.assertRaises(FileExistsError): - ensure_directory_exists(file_path) + with tempfile.TemporaryDirectory() as temp_dir: + file_path = os.path.join(temp_dir, "file.extension") + with open(file_path, 'x'): + pass + with self.assertRaises(FileExistsError): + ensure_directory_exists(file_path) From d87f9672fa63819b167200db0429acd2595f88c2 Mon Sep 17 00:00:00 2001 From: FemtosecondLaser <38204088+FemtosecondLaser@users.noreply.github.com> Date: Sat, 23 Oct 2021 13:12:49 +0100 Subject: [PATCH 5/6] Improved the readability of the tests. --- tests/unit/test_cli.py | 45 +++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 55c12ea8c..cf6d91002 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -207,32 +207,33 @@ class DaemonDocsTests(TestCase): class EnsureDirectoryExistsTests(TestCase): + def setUp(self): + self.temp_dir = tempfile.mkdtemp() + + def tearDown(self): + shutil.rmtree(self.temp_dir) + def test_when_parent_dir_does_not_exist_then_dir_is_created_with_parent(self): - with tempfile.TemporaryDirectory() as temp_dir: - dir_path = os.path.join(temp_dir, "parent_dir", "dir") - ensure_directory_exists(dir_path) - self.assertTrue(os.path.exists(dir_path)) + dir_path = os.path.join(self.temp_dir, "parent_dir", "dir") + ensure_directory_exists(dir_path) + self.assertTrue(os.path.exists(dir_path)) def test_when_non_writable_dir_exists_then_raise(self): - with tempfile.TemporaryDirectory() as temp_dir: - dir_path = os.path.join(temp_dir, "dir") - pathlib.Path(dir_path).mkdir(mode=0o555) # creates a non-writable, readable and executable dir - with self.assertRaises(PermissionError): - ensure_directory_exists(dir_path) + dir_path = os.path.join(self.temp_dir, "dir") + pathlib.Path(dir_path).mkdir(mode=0o555) # creates a non-writable, readable and executable dir + with self.assertRaises(PermissionError): + ensure_directory_exists(dir_path) def test_when_dir_exists_and_writable_then_no_raise(self): - with tempfile.TemporaryDirectory() as temp_dir: - dir_path = os.path.join(temp_dir, "dir") - pathlib.Path(dir_path).mkdir(mode=0o777) # creates a writable, readable and executable dir - try: - ensure_directory_exists(dir_path) - except (FileExistsError, PermissionError) as err: - self.fail(f"{type(err).__name__} was raised") + dir_path = os.path.join(self.temp_dir, "dir") + pathlib.Path(dir_path).mkdir(mode=0o777) # creates a writable, readable and executable dir + try: + ensure_directory_exists(dir_path) + except (FileExistsError, PermissionError) as err: + self.fail(f"{type(err).__name__} was raised") def test_when_non_dir_file_exists_at_path_then_raise(self): - with tempfile.TemporaryDirectory() as temp_dir: - file_path = os.path.join(temp_dir, "file.extension") - with open(file_path, 'x'): - pass - with self.assertRaises(FileExistsError): - ensure_directory_exists(file_path) + file_path = os.path.join(self.temp_dir, "file.extension") + pathlib.Path(file_path).touch() + with self.assertRaises(FileExistsError): + ensure_directory_exists(file_path) From 07f76f7ad1b497397818c34ae8c6922f6382cbdd Mon Sep 17 00:00:00 2001 From: FemtosecondLaser <38204088+FemtosecondLaser@users.noreply.github.com> Date: Tue, 26 Oct 2021 11:17:52 +0100 Subject: [PATCH 6/6] Added an integration test covering the following scenario: On start, if download dir is non-writable - daemon terminates with a helpful message. --- tests/integration/other/test_cli.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/integration/other/test_cli.py b/tests/integration/other/test_cli.py index 7de635fc6..968665333 100644 --- a/tests/integration/other/test_cli.py +++ b/tests/integration/other/test_cli.py @@ -1,4 +1,6 @@ import contextlib +import os +import tempfile from io import StringIO from lbry.testcase import AsyncioTestCase @@ -37,3 +39,9 @@ class CLIIntegrationTest(AsyncioTestCase): cli.main(["--api", "localhost:5299", "status"]) actual_output = actual_output.getvalue() self.assertIn("is_running", actual_output) + + def test_when_download_dir_non_writable_on_start_then_daemon_dies_with_helpful_msg(self): + with tempfile.TemporaryDirectory() as download_dir: + os.chmod(download_dir, mode=0o555) # makes download dir non-writable, readable and executable + with self.assertRaisesRegex(PermissionError, f"The following directory is not writable: {download_dir}"): + cli.main(["start", "--download-dir", download_dir])