fix(job_attachment)!: use username instead of group for Windows file …

…permissions setting (#196)

Signed-off-by: Gahyun Suh <132245153+gahyusuh@users.noreply.github.com>

scripted_tests/set_file_permission_for_windows.py

@@ -12,33 +12,33 @@
 from deadline.job_attachments.os_file_permission import (
     WindowsFileSystemPermissionSettings,
     WindowsPermissionEnum,
-    _set_fs_group_for_windows,
+    _set_fs_permission_for_windows,
 )
 
 """
-This script is to test a `_set_fs_group_for_windows()` function in Job Attachment module,
-which is for setting file permissions and group ownership on Windows.
+This script is to test a `_set_fs_permission_for_windows()` function in Job Attachment module,
+which is for setting file permissions and ownership on Windows.
 
 Prerequisites
 -------------
-Before running the test, prepare the target group, target user (a user who is a member of
-the target group), and disjoint user (not a member of the target group.)
+Before running the test, prepare a target user and a disjoint user.
 
 How to Run
 ----------
 To execute this script, run the following command from the root location:
 python ./scripted_tests/set_file_permission_for_windows.py \
     -n <the_number_of_files_to_create_for_test> \
-    -g <target_group_name> \
-    -f <file_permission> -d <directory_permission> \
-    -u <target_user_name> -du <disjoint_user_name>
+    -f <file_permission> \
+    -d <directory_permission> \
+    -u <target_user_name> \
+    -du <disjoint_user_name>
 
 Note: The `-f` and `-d` flags are optional.
 
 Then, the command will do the following:
 1. Installs `pywin32`, which is a required package for the testing.
 2. Creates a temporary directory and creates the specified number of files in it.
-2. The script will add the given target group to the owner list for the specified files.
+2. The script will add the given target user to the owner list for the specified files.
 3. It will then verify (1) whether the target user has Read/Write access to these files,
    and (2) that the disjoint user does not have access.
 
@@ -60,7 +60,6 @@
 def run_test():
     parser = argparse.ArgumentParser()
     parser.add_argument("-n", "--num_files", type=int, required=True)
-    parser.add_argument("-g", "--group", required=True, type=str)
     parser.add_argument("-f", "--file_permission", required=False, type=str, default="FULL_CONTROL")
     parser.add_argument("-d", "--dir_permission", required=False, type=str, default="FULL_CONTROL")
     parser.add_argument("-u", "--target_user", required=True, type=str)
@@ -90,21 +89,20 @@ def run_test():
             files.append(str(file_path))
 
         print("Temporary files created.")
-        print("Running test: Setting file permissions and group ownership...")
+        print("Running test: Setting file permissions...")
         start_time = time.perf_counter()
 
         fs_permission_settings = WindowsFileSystemPermissionSettings(
             os_user=args.target_user,
-            os_group=args.group,
             dir_mode=dir_permission,
             file_mode=file_permission,
         )
-        _set_fs_group_for_windows(
+        _set_fs_permission_for_windows(
             file_paths=files,
             local_root=temp_root_dir,
             fs_permission_settings=fs_permission_settings,
         )
-        print("File permissions and group ownership set.")
+        print("File permissions set.")
         print(f"Total running time for {num_files} files: {time.perf_counter() - start_time}")
 
         print("Checking file permissions...")

src/deadline/job_attachments/download.py

@@ -56,7 +56,7 @@
     PosixFileSystemPermissionSettings,
     WindowsFileSystemPermissionSettings,
     _set_fs_group_for_posix,
-    _set_fs_group_for_windows,
+    _set_fs_permission_for_windows,
 )
 from ._utils import _is_relative_to, _join_s3_paths
 
@@ -791,7 +791,7 @@ def _set_fs_group(
     else:  # if os.name is not "posix"
         if not isinstance(fs_permission_settings, WindowsFileSystemPermissionSettings):
             raise TypeError("The file system permission settings must be specific to Windows.")
-        _set_fs_group_for_windows(
+        _set_fs_permission_for_windows(
             file_paths=file_paths,
             local_root=local_root,
             fs_permission_settings=fs_permission_settings,

src/deadline/job_attachments/os_file_permission.py

@@ -4,9 +4,9 @@
 from pathlib import Path
 import shutil
 import sys
-from dataclasses import dataclass
+from dataclasses import dataclass, field
 from enum import Enum
-from typing import List, Set, Union
+from typing import List, Optional, Set, Union
 
 from .exceptions import AssetSyncError, PathOutsideDirectoryError
 from ._utils import _is_relative_to
@@ -58,10 +58,12 @@ class WindowsFileSystemPermissionSettings:
         file_mode (WindowsPermissionEnum): The permission mode to be added to files.
     """
 
+    # TODO: Remove the optional `os_group` once the jobRunAsUser - windows - group field
+    # is removed from our APIs
     os_user: str
-    os_group: str
     dir_mode: WindowsPermissionEnum
     file_mode: WindowsPermissionEnum
+    os_group: Optional[str] = field(default=None)
 
 
 # A union of different file system permission settings that are based on the underlying OS.
@@ -104,38 +106,38 @@ def _set_fs_group_for_posix(
         _change_permission_for_posix(str(dir_path), os_group, dir_mode)
 
 
-def _set_fs_group_for_windows(
+def _set_fs_permission_for_windows(
     file_paths: List[str],
     local_root: str,
     fs_permission_settings: WindowsFileSystemPermissionSettings,
 ) -> None:
-    os_group = fs_permission_settings.os_group
+    os_user = fs_permission_settings.os_user
     dir_mode = fs_permission_settings.dir_mode
     file_mode = fs_permission_settings.file_mode
 
     # A set that stores the unique directory paths where permissions need to be changed.
     dir_paths_to_change_fs_group: Set[Path] = set()
 
-    # 1. Set group ownership and permissions for each file.
+    # 1. Set permissions for each file.
     for file_path_str in file_paths:
         # The file path must be relative to the root path (ie. local_root).
         if not _is_relative_to(file_path_str, local_root):
             raise PathOutsideDirectoryError(
                 f"The provided path '{file_path_str}' is not under the root directory: {local_root}"
             )
 
-        _change_permission_for_windows(file_path_str, os_group, file_mode)
+        _change_permission_for_windows(file_path_str, os_user, file_mode)
 
         # Add the parent directories of each file to the set of directories whose
-        # group ownership and permissions will be changed.
+        # permissions will be changed.
         path_components = Path(file_path_str).relative_to(local_root).parents
         for path_component in path_components:
             path_to_change = Path(local_root).joinpath(path_component)
             dir_paths_to_change_fs_group.add(path_to_change)
 
-    # 2. Set group ownership and permissions for the directories in the path starting from root.
+    # 2. Set permissions for the directories in the path starting from root.
     for dir_path in dir_paths_to_change_fs_group:
-        _change_permission_for_windows(str(dir_path), os_group, dir_mode)
+        _change_permission_for_windows(str(dir_path), os_user, dir_mode)
 
 
 def _change_permission_for_posix(
@@ -153,7 +155,7 @@ def _change_permission_for_posix(
 
 def _change_permission_for_windows(
     path: str,
-    os_group: str,
+    os_user: str,
     mode: WindowsPermissionEnum,
 ) -> None:
     if sys.platform != "win32":
@@ -163,21 +165,21 @@ def _change_permission_for_windows(
 
     try:
         con_mode = _get_ntsecuritycon_mode(mode)
-        # Lookup the group's SID (Security Identifier)
-        group_sid = win32security.LookupAccountName(None, os_group)[0]
+        # Lookup the user's SID (Security Identifier)
+        user_sid = win32security.LookupAccountName(None, os_user)[0]
         # Get existing DACL (Discretionary Access Control List). If dacl is none, create a new one.
         sd = win32security.GetFileSecurity(path, win32security.DACL_SECURITY_INFORMATION)
         dacl = sd.GetSecurityDescriptorDacl()
         if dacl is None:
             dacl = win32security.ACL()
         # Add new ACE (Access Control Entry)
-        dacl.AddAccessAllowedAce(win32security.ACL_REVISION, con_mode, group_sid)
-        # Set the new DACL
+        dacl.AddAccessAllowedAce(win32security.ACL_REVISION, con_mode, user_sid)
+        # Set the modified DACL to the security descriptor
         sd.SetSecurityDescriptorDacl(1, dacl, 0)
         win32security.SetFileSecurity(path, win32security.DACL_SECURITY_INFORMATION, sd)
     except win32security.error as e:
         raise AssetSyncError(
-            f"Failed to set group ownership and permissions for file or directory ({path}): {e}"
+            f"Failed to set permissions for file or directory ({path}): {e}"
         ) from e
 
 

test/unit/deadline_job_attachments/test_download.py

@@ -833,7 +833,6 @@ def test_download_files_from_manifests_with_fs_permission_settings_windows(
 
         fs_permission_settings = WindowsFileSystemPermissionSettings(
             os_user="test-user",
-            os_group="test-group",
             dir_mode=WindowsPermissionEnum.FULL_CONTROL,
             file_mode=WindowsPermissionEnum.FULL_CONTROL,
         )
@@ -863,7 +862,7 @@ def test_download_files_from_manifests_with_fs_permission_settings_windows(
             str(
                 call(
                     str(path),
-                    "test-group",
+                    "test-user",
                     WindowsPermissionEnum.FULL_CONTROL,
                 )
             )
@@ -1003,10 +1002,9 @@ def test_download_files_from_manifests_have_correct_group_windows(
         manifest = decode_manifest(manifest_str)
         manifests_by_root = {str(tmp_path): manifest}
 
-        # Use a builtin group, so we can expect it to exist on any Windows machine
+        # Use a builtin user 'Guest', so we can expect it to exist on any Windows machine
         fs_permission_settings = WindowsFileSystemPermissionSettings(
-            os_user="test-user",
-            os_group="Users",
+            os_user="Guest",
             dir_mode=WindowsPermissionEnum.FULL_CONTROL,
             file_mode=WindowsPermissionEnum.FULL_CONTROL,
         )
@@ -1027,28 +1025,28 @@ def test_download_files_from_manifests_have_correct_group_windows(
             tmp_path / rel_path for rel_path in self.TARGET_PERMISSION_CHANGE_PATHS_RELATIVE
         ]
 
-        # Verify that the group ownership and permissions of files downloaded through Job Attachment
+        # Verify that the user ownership and permissions of files downloaded through Job Attachment
         # have been appropriately modified.
         for path in expected_changed_paths:
             # Get the file's security information
             sd = win32security.GetFileSecurity(str(path), win32security.DACL_SECURITY_INFORMATION)
             # Get the discretionary access control list (DACL)
             dacl = sd.GetSecurityDescriptorDacl()
-            # Get groups and permissions info from ACE
-            group_permissions: dict[str, int] = {}
+            # Get the permissions info from ACE
+            permission_mapping: dict[str, int] = {}
             for ace_no in range(dacl.GetAceCount()):
                 trustee_sid = dacl.GetAce(ace_no)[2]
                 trustee_name, _, _ = win32security.LookupAccountSid(None, trustee_sid)
                 if trustee_name:
                     trustee = {
                         "TrusteeForm": win32security.TRUSTEE_IS_SID,
-                        "TrusteeType": win32security.TRUSTEE_IS_GROUP,
+                        "TrusteeType": win32security.TRUSTEE_IS_USER,
                         "Identifier": trustee_sid,
                     }
                     result = dacl.GetEffectiveRightsFromAcl(trustee)
-                    group_permissions[trustee_name] = result
-            assert group_permissions["Users"] == ntsecuritycon.FILE_ALL_ACCESS
-            assert "Guests" not in group_permissions
+                    permission_mapping[trustee_name] = result
+            assert "Guest" in permission_mapping
+            assert permission_mapping["Guest"] == ntsecuritycon.FILE_ALL_ACCESS
 
     @mock_sts
     def test_download_task_output(