From 8e8de8a611ae5276fa6ba9cfd42983f15fb2df0f Mon Sep 17 00:00:00 2001 From: Matthieu Blais Date: Fri, 9 Sep 2022 15:22:13 +0800 Subject: [PATCH 1/5] mode must be octal in paramiko function mkdir --- airflow/providers/sftp/hooks/sftp.py | 2 +- tests/providers/sftp/hooks/test_sftp.py | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/airflow/providers/sftp/hooks/sftp.py b/airflow/providers/sftp/hooks/sftp.py index 93eb3d52d45ea..09dcb8eb24f08 100644 --- a/airflow/providers/sftp/hooks/sftp.py +++ b/airflow/providers/sftp/hooks/sftp.py @@ -214,7 +214,7 @@ def create_directory(self, path: str, mode: int = 777) -> None: self.create_directory(dirname, mode) if basename: self.log.info("Creating %s", path) - conn.mkdir(path, mode=mode) + conn.mkdir(path, mode=int(str(mode), 8)) def delete_directory(self, path: str) -> None: """ diff --git a/tests/providers/sftp/hooks/test_sftp.py b/tests/providers/sftp/hooks/test_sftp.py index f3af70cc16d11..e77ef1e598993 100644 --- a/tests/providers/sftp/hooks/test_sftp.py +++ b/tests/providers/sftp/hooks/test_sftp.py @@ -107,12 +107,18 @@ def test_mkdir(self): self.hook.mkdir(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS, new_dir_name)) output = self.hook.describe_directory(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS)) assert new_dir_name in output + # test the directory has default permissions to 777 + output = self.hook.get_conn().lstat(new_dir_name) + assert output.st_mode & 0o777 == 0o777 def test_create_and_delete_directory(self): new_dir_name = "new_dir" self.hook.create_directory(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS, new_dir_name)) output = self.hook.describe_directory(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS)) assert new_dir_name in output + # test the directory has default permissions to 777 + output = self.hook.get_conn().lstat(new_dir_name) + assert output.st_mode & 0o777 == 0o777 # test directory already exists for code coverage, should not raise an exception self.hook.create_directory(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS, new_dir_name)) # test path already exists and is a file, should raise an exception From 0664b2c227d40d77f94c580c306ca37ccaf016d3 Mon Sep 17 00:00:00 2001 From: Matthieu Blais Date: Thu, 22 Sep 2022 10:02:28 +0200 Subject: [PATCH 2/5] including umask in to test permissions --- tests/providers/sftp/hooks/test_sftp.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/providers/sftp/hooks/test_sftp.py b/tests/providers/sftp/hooks/test_sftp.py index e77ef1e598993..b8d78ca7f9560 100644 --- a/tests/providers/sftp/hooks/test_sftp.py +++ b/tests/providers/sftp/hooks/test_sftp.py @@ -107,9 +107,10 @@ def test_mkdir(self): self.hook.mkdir(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS, new_dir_name)) output = self.hook.describe_directory(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS)) assert new_dir_name in output - # test the directory has default permissions to 777 + # test the directory has default permissions to 777 - umask + umask = 0o022 output = self.hook.get_conn().lstat(new_dir_name) - assert output.st_mode & 0o777 == 0o777 + assert output.st_mode & 0o777 == 0o777 - umask def test_create_and_delete_directory(self): new_dir_name = "new_dir" @@ -117,8 +118,9 @@ def test_create_and_delete_directory(self): output = self.hook.describe_directory(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS)) assert new_dir_name in output # test the directory has default permissions to 777 + umask = 0o022 output = self.hook.get_conn().lstat(new_dir_name) - assert output.st_mode & 0o777 == 0o777 + assert output.st_mode & 0o777 == 0o777 - umask # test directory already exists for code coverage, should not raise an exception self.hook.create_directory(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS, new_dir_name)) # test path already exists and is a file, should raise an exception From 19ee113404c2b0ad2110a4b64604f1c057b8451c Mon Sep 17 00:00:00 2001 From: Matthieu Blais Date: Fri, 23 Sep 2022 08:14:20 +0200 Subject: [PATCH 3/5] Fixing the test case for checking sftp folder permissions after creation by the hook. --- tests/providers/sftp/hooks/test_sftp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/providers/sftp/hooks/test_sftp.py b/tests/providers/sftp/hooks/test_sftp.py index b8d78ca7f9560..b471dc88b053f 100644 --- a/tests/providers/sftp/hooks/test_sftp.py +++ b/tests/providers/sftp/hooks/test_sftp.py @@ -109,7 +109,7 @@ def test_mkdir(self): assert new_dir_name in output # test the directory has default permissions to 777 - umask umask = 0o022 - output = self.hook.get_conn().lstat(new_dir_name) + output = self.hook.get_conn().lstat(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS, new_dir_name)) assert output.st_mode & 0o777 == 0o777 - umask def test_create_and_delete_directory(self): @@ -119,7 +119,7 @@ def test_create_and_delete_directory(self): assert new_dir_name in output # test the directory has default permissions to 777 umask = 0o022 - output = self.hook.get_conn().lstat(new_dir_name) + output = self.hook.get_conn().lstat(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS, new_dir_name)) assert output.st_mode & 0o777 == 0o777 - umask # test directory already exists for code coverage, should not raise an exception self.hook.create_directory(os.path.join(TMP_PATH, TMP_DIR_FOR_TESTS, new_dir_name)) From c7f571ee3803d39cb7014329844b827163b3ef1c Mon Sep 17 00:00:00 2001 From: Matthieu Blais Date: Sat, 8 Oct 2022 11:49:35 +0800 Subject: [PATCH 4/5] using octal mode by default for directory permissions --- airflow/providers/sftp/hooks/sftp.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/airflow/providers/sftp/hooks/sftp.py b/airflow/providers/sftp/hooks/sftp.py index 09dcb8eb24f08..57e7b8b719561 100644 --- a/airflow/providers/sftp/hooks/sftp.py +++ b/airflow/providers/sftp/hooks/sftp.py @@ -159,15 +159,16 @@ def list_directory(self, path: str) -> list[str]: files = sorted(conn.listdir(path)) return files - def mkdir(self, path: str, mode: int = 777) -> None: + def mkdir(self, path: str, mode: int = 0o777) -> None: """ Creates a directory on the remote system. + The default mode is 0777, but on some systems, the current umask value is first masked out. :param path: full path to the remote directory to create - :param mode: permissions to set the directory with + :param mode: int permissions (posix-style) of octal mode for directory """ conn = self.get_conn() - conn.mkdir(path, mode=int(str(mode), 8)) + conn.mkdir(path, mode=mode) def isdir(self, path: str) -> bool: """ @@ -195,12 +196,13 @@ def isfile(self, path: str) -> bool: result = False return result - def create_directory(self, path: str, mode: int = 777) -> None: + def create_directory(self, path: str, mode: int = 0o777) -> None: """ Creates a directory on the remote system. + The default mode is 0777, but on some systems, the current umask value is first masked out. :param path: full path to the remote directory to create - :param mode: int representation of octal mode for directory + :param mode: int permissions (posix-style) of octal mode for directory """ conn = self.get_conn() if self.isdir(path): @@ -214,7 +216,7 @@ def create_directory(self, path: str, mode: int = 777) -> None: self.create_directory(dirname, mode) if basename: self.log.info("Creating %s", path) - conn.mkdir(path, mode=int(str(mode), 8)) + conn.mkdir(path, mode=mode) def delete_directory(self, path: str) -> None: """ From bf20f858540f013d44f34d4146aa035ebebebcb1 Mon Sep 17 00:00:00 2001 From: Matthieu Blais Date: Sat, 8 Oct 2022 14:27:43 +0800 Subject: [PATCH 5/5] Update docstring to pass spellcheck during build-docs --- airflow/providers/sftp/hooks/sftp.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/airflow/providers/sftp/hooks/sftp.py b/airflow/providers/sftp/hooks/sftp.py index 57e7b8b719561..85edeee46ef68 100644 --- a/airflow/providers/sftp/hooks/sftp.py +++ b/airflow/providers/sftp/hooks/sftp.py @@ -165,7 +165,7 @@ def mkdir(self, path: str, mode: int = 0o777) -> None: The default mode is 0777, but on some systems, the current umask value is first masked out. :param path: full path to the remote directory to create - :param mode: int permissions (posix-style) of octal mode for directory + :param mode: int permissions of octal mode for directory """ conn = self.get_conn() conn.mkdir(path, mode=mode) @@ -202,7 +202,7 @@ def create_directory(self, path: str, mode: int = 0o777) -> None: The default mode is 0777, but on some systems, the current umask value is first masked out. :param path: full path to the remote directory to create - :param mode: int permissions (posix-style) of octal mode for directory + :param mode: int permissions of octal mode for directory """ conn = self.get_conn() if self.isdir(path):