diff --git a/pyproject.toml b/pyproject.toml index 90525402..f82a1a03 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -41,6 +41,7 @@ dependencies = [ "survey>=4.0,<6.0", "typing_extensions", "watchdog>=2.0.1", + "xattr", ] [project.readme] diff --git a/src/maestral/sync.py b/src/maestral/sync.py index 07e4b2d1..5fe97746 100644 --- a/src/maestral/sync.py +++ b/src/maestral/sync.py @@ -77,7 +77,6 @@ from .constants import ( EXCLUDED_DIR_NAMES, MIGNORE_FILE, FILE_CACHE, - IS_MACOS, ) from .exceptions import ( SyncError, @@ -3585,14 +3584,7 @@ class SyncEngine: # Preserve permissions of the destination file if we are only syncing an # update to the file content (Dropbox ID of the file remains the same). old_entry = self.get_index_entry(event.dbx_path_lower) - preserve_permissions = bool(old_entry and event.dbx_id == old_entry.dbx_id) - - if preserve_permissions: - # Ignore FileModifiedEvent when changing permissions. - # Note that two FileModifiedEvents may be emitted on macOS. - ignore_events.append(FileModifiedEvent(event.local_path)) - if IS_MACOS: - ignore_events.append(FileModifiedEvent(event.local_path)) + preserve_metadata = bool(old_entry and event.dbx_id == old_entry.dbx_id) if isfile(event.local_path): # Ignore FileDeletedEvent when replacing old file. @@ -3607,7 +3599,8 @@ class SyncEngine: move( tmp_fname, event.local_path, - preserve_dest_permissions=preserve_permissions, + keep_target_permissions=preserve_metadata, + keep_target_xattrs=preserve_metadata, raise_error=True, ) diff --git a/src/maestral/utils/path.py b/src/maestral/utils/path.py index 49efc49c..db036620 100644 --- a/src/maestral/utils/path.py +++ b/src/maestral/utils/path.py @@ -14,6 +14,9 @@ import platform from stat import S_ISDIR from typing import List, Optional, Tuple, Callable, Iterator, Iterable, Union +# third party imports +import xattr + # local imports from .hashing import DropboxContentHasher @@ -355,7 +358,8 @@ def move( src_path: str, dest_path: str, raise_error: bool = False, - preserve_dest_permissions: bool = False, + keep_target_permissions: bool = False, + keep_target_xattrs: bool = False, ) -> Optional[OSError]: """ Moves a file or folder from ``src_path`` to ``dest_path``. If either the source or @@ -369,22 +373,31 @@ def move( by the move. Any existing **empty** folder will be replaced if the source is also a folder. :param raise_error: Whether to raise errors or return them. - :param preserve_dest_permissions: Whether to apply the permissions of the source - path to the destination path. Permissions will not be set recursively and may - will be set for symlinks if this is not supported by the platform, i.e., if - ``os.chmod not in os.supports_follow_symlinks``. + :param keep_target_permissions: Whether to preserve the permissions of a file at the + destination, if any. + :param keep_target_xattrs: Whether to preserve the extended attributes of a file at + the destination, if any. :returns: Any caught exception during the move. """ err: Optional[OSError] = None - orig_mode: Optional[int] = None - if preserve_dest_permissions: - # save dest permissions + if keep_target_permissions: try: - orig_mode = os.lstat(dest_path).st_mode & 0o777 + dest_mode = os.lstat(dest_path).st_mode & 0o777 + follow_symlinks = os.chmod not in os.supports_follow_symlinks + os.chmod(src_path, dest_mode, follow_symlinks=follow_symlinks) except (FileNotFoundError, NotADirectoryError): pass + if keep_target_xattrs: + try: + dest_attrs = xattr.xattr(dest_path) + for key, value in dest_attrs.iteritems(): + xattr.setxattr(src_path, key, value) + except OSError: + # Fail gracefully if extended attributes are not supported by the system. + pass + try: os.rename(src_path, dest_path) except FileNotFoundError: @@ -392,15 +405,6 @@ def move( pass except OSError as exc: err = exc - else: - if orig_mode: - # Reapply dest permissions. If the dest is a symlink, only apply permissions - # if this is supported for symlinks by the platform. - try: - if os.chmod in os.supports_follow_symlinks: - os.chmod(dest_path, orig_mode, follow_symlinks=False) - except OSError: - pass if raise_error and err: raise err diff --git a/tests/linked/integration/test_sync.py b/tests/linked/integration/test_sync.py index 2698d3d0..07f3cfb4 100644 --- a/tests/linked/integration/test_sync.py +++ b/tests/linked/integration/test_sync.py @@ -450,10 +450,6 @@ def test_excluded_folder_cleared_on_deletion(m: Maestral) -> None: assert_no_errors(m) -@pytest.mark.skipif( - os.chmod not in os.supports_follow_symlinks, - reason="chmod does not support follow_symlinks=False", -) def test_unix_permissions(m: Maestral) -> None: """ Tests that a newly downloaded file is created with default permissions for our diff --git a/tests/offline/utils/test_path.py b/tests/offline/utils/test_path.py index 360f7f77..a90246a6 100644 --- a/tests/offline/utils/test_path.py +++ b/tests/offline/utils/test_path.py @@ -1,3 +1,7 @@ +import os +import stat + +import xattr import pytest from maestral.utils.path import ( @@ -5,10 +9,15 @@ from maestral.utils.path import ( get_existing_equivalent_paths, is_fs_case_sensitive, is_child, + move, ) from maestral.utils.appdirs import get_home_dir +def touch(path: str) -> None: + open(path, "w").close() + + def test_normalized_path_exists(tmp_path): # Assert that an existing path is found, even when a different casing is used. @@ -79,3 +88,34 @@ def test_is_child(): assert is_child("/parent/path/child/", "/parent/path") assert not is_child("/parent/path", "/parent/path") assert not is_child("/path1", "/path2") + + +def test_move_preserves_permissions(tmp_path): + src_path = str(tmp_path / "source.txt") + dest_path = str(tmp_path / "dest.txt") + + touch(src_path) + touch(dest_path) + + os.chmod(dest_path, stat.S_IEXEC) + + move(src_path, dest_path, keep_target_permissions=True) + + assert bool(os.stat(dest_path).st_mode & stat.S_IEXEC) + + +def test_move_preserves_xattrs(tmp_path): + src_path = str(tmp_path / "source.txt") + dest_path = str(tmp_path / "dest.txt") + + touch(src_path) + touch(dest_path) + + try: + xattr.setxattr(dest_path, "com.samschott.maestral.test", "value".encode()) + except OSError: + pytest.skip("Xattr not supported by this system") + + move(src_path, dest_path, keep_target_xattrs=True) + + assert xattr.getxattr(dest_path, "com.samschott.maestral.test") == "value".encode()