fix: Make tempdir permissions inherited by descendants on Windows (#29)

Signed-off-by: Robert Luttrell <[email protected]>

src/openjd/sessions/_tempdir.py

@@ -85,23 +85,30 @@ def __init__(
             elif is_windows():
                 user = cast(WindowsSessionUser, user)
                 try:
-                    # Change permissions
                     if user.group:
                         principal_to_permit = user.group
                     else:
                         principal_to_permit = user.user
 
                     principal_sid, _, _ = win32security.LookupAccountName(None, principal_to_permit)
 
+                    # We don't want to propagate existing permissions, so create a new DACL
                     dacl = win32security.ACL()
-                    dacl.AddAccessAllowedAce(
-                        win32security.ACL_REVISION, ntsecuritycon.FILE_ALL_ACCESS, principal_sid
+
+                    # Add an ACE to the DACL giving the principal full control and enabling inheritance of the ACE
+                    dacl.AddAccessAllowedAceEx(
+                        win32security.ACL_REVISION,
+                        ntsecuritycon.OBJECT_INHERIT_ACE | ntsecuritycon.CONTAINER_INHERIT_ACE,
+                        ntsecuritycon.FILE_ALL_ACCESS,
+                        principal_sid,
                     )
 
+                    # Get the security descriptor of the tempdir
                     sd = win32security.GetFileSecurity(
                         str(self.path), win32security.DACL_SECURITY_INFORMATION
                     )
 
+                    # Set the security descriptor's DACL to the newly-created DACL
                     # Arguments:
                     # 1. bDaclPresent = 1: Indicates that the DACL is present in the security descriptor.
                     #    If set to 0, this method ignores the provided DACL and allows access to all principals.
@@ -110,6 +117,7 @@ def __init__(
                     #    If set to 1, indicates the DACL was defaulted, as in the case of permissions inherited from a parent directory.
                     sd.SetSecurityDescriptorDacl(1, dacl, 0)
 
+                    # Set the security descriptor to the tempdir
                     win32security.SetFileSecurity(
                         str(self.path), win32security.DACL_SECURITY_INFORMATION, sd
                     )

test/openjd/sessions/test_tempdir.py

@@ -99,62 +99,126 @@ class TestTempDirWindowsUser:
     @patch("openjd.sessions.WindowsSessionUser.is_process_user", return_value=True)
     def test_windows_user_with_group_permits_group(self, mock_user_match):
         # GIVEN
-        # Use a builtin account, so we can expect it to exist on any Windows machine
-        windows_user = WindowsSessionUser("Guest", group="Users")
+        # Use a builtin group, so we can expect it to exist on any Windows machine
+        windows_user = WindowsSessionUser("arbitrary_user", group="Users")
 
         # WHEN
         tempdir = TempDir(user=windows_user)
 
         # THEN
-        sd = win32security.GetFileSecurity(
-            str(tempdir.path), win32security.DACL_SECURITY_INFORMATION
-        )
-        dacl = sd.GetSecurityDescriptorDacl()
+        assert self.principal_has_full_control_of_object(str(tempdir.path), windows_user.group)
+
+    @patch("openjd.sessions.WindowsSessionUser.is_process_user", return_value=True)
+    def test_wrong_group_not_permitted(self, mock_user_match):
+        # GIVEN
+        # Use a builtin group, so we can expect it to exist on any Windows machine
+        windows_user = WindowsSessionUser("arbitrary_user", group="Users")
 
-        assert self.principal_has_full_control_in_dacl(dacl, windows_user.group)
+        # WHEN
+        tempdir = TempDir(user=windows_user)
+
+        # THEN
+        assert self.principal_has_no_permissions_on_object(str(tempdir.path), "Guests")
 
     @patch("openjd.sessions.WindowsSessionUser.is_process_user", return_value=True)
-    def test_windows_user_without_group_permits_user(self, mock_user_match):
+    def test_windows_user_with_group_permits_group_permissions_inherited(self, mock_user_match):
         # GIVEN
-        windows_user = WindowsSessionUser("Guest")
+        # Use a builtin group, so we can expect it to exist on any Windows machine
+        windows_user = WindowsSessionUser("arbitrary_user", group="Users")
 
         # WHEN
         tempdir = TempDir(user=windows_user)
+        os.mkdir(tempdir.path / "child_dir")
+        os.mkdir(tempdir.path / "child_dir" / "grandchild_dir")
+        open(tempdir.path / "child_file", "a").close()
+        open(tempdir.path / "child_dir" / "grandchild_file", "a").close()
 
         # THEN
-        sd = win32security.GetFileSecurity(
-            str(tempdir.path), win32security.DACL_SECURITY_INFORMATION
+        assert self.principal_has_full_control_of_object(str(tempdir.path), windows_user.group)
+        assert self.principal_has_full_control_of_object(
+            str(tempdir.path / "child_dir"), windows_user.group
         )
-        dacl = sd.GetSecurityDescriptorDacl()
+        assert self.principal_has_full_control_of_object(
+            str(tempdir.path / "child_file"), windows_user.group
+        )
+        assert self.principal_has_full_control_of_object(
+            str(tempdir.path / "child_dir" / "grandchild_dir"), windows_user.group
+        )
+        assert self.principal_has_full_control_of_object(
+            str(tempdir.path / "child_dir" / "grandchild_file"), windows_user.group
+        )
+
+    @patch("openjd.sessions.WindowsSessionUser.is_process_user", return_value=True)
+    def test_windows_user_without_group_permits_user(self, mock_user_match):
+        # GIVEN
+        windows_user = WindowsSessionUser("Guest")
 
-        assert self.principal_has_full_control_in_dacl(dacl, windows_user.user)
+        # WHEN
+        tempdir = TempDir(user=windows_user)
+
+        # THEN
+        assert self.principal_has_full_control_of_object(str(tempdir.path), "Guest")
 
     @patch("openjd.sessions.WindowsSessionUser.is_process_user", return_value=True)
     def test_invalid_windows_group_raises_exception(self, mock_user_match):
         # GIVEN
-        # Use a builtin account, so we can expect it to exist on any Windows machine
-        windows_user = WindowsSessionUser("Guest", group="nonexistentgroup")
+        windows_user = WindowsSessionUser("Guest", group="nonexistent_group")
 
         # THEN
         with pytest.raises(RuntimeError, match="Could not change permissions of directory"):
             TempDir(user=windows_user)
 
-    def principal_has_full_control_in_dacl(self, dacl, principal_to_check):
-        principal_to_check_sid, _, _ = win32security.LookupAccountName(None, principal_to_check)
+    @staticmethod
+    def get_aces_for_principal_on_object(object_path: str, principal_name: str):
+        """
+        Returns a list of access allowed masks and a list of access denied masks for a principal's permissions on an object.
+        Access masks for principals other than that specified by principal_name will be excluded from both lists.
+
+        Arguments:
+            object_path (str): The path to the object for which the ACE masks will be retrieved.
+            principal_name (str): The name of the principal to filter for.
+
+        Returns:
+            access_allowed_masks (List[int]): All masks allowing principal_name access to the file.
+            access_denied_masks (List[int]): All masks denying principal_name access to the file.
+        """
+        sd = win32security.GetFileSecurity(object_path, win32security.DACL_SECURITY_INFORMATION)
+
+        dacl = sd.GetSecurityDescriptorDacl()
+
+        principal_to_check_sid, _, _ = win32security.LookupAccountName(None, principal_name)
+
+        access_allowed_masks = []
+        access_denied_masks = []
 
         for i in range(dacl.GetAceCount()):
             ace = dacl.GetAce(i)
 
+            ace_type = ace[0][0]
             access_mask = ace[1]
             ace_principal_sid = ace[2]
 
-            if (
-                ace_principal_sid == principal_to_check_sid
-                and access_mask == self.FULL_CONTROL_MASK
-            ):
-                return True
+            account_name, _, _ = win32security.LookupAccountSid(None, ace_principal_sid)
+
+            if ace_principal_sid == principal_to_check_sid:
+                if ace_type == win32security.ACCESS_ALLOWED_ACE_TYPE:
+                    access_allowed_masks.append(access_mask)
+                elif ace_type == win32security.ACCESS_DENIED_ACE_TYPE:
+                    access_denied_masks.append(access_mask)
+
+        return access_allowed_masks, access_denied_masks
+
+    def principal_has_full_control_of_object(self, object_path, principal_name):
+        access_allowed_masks, access_denied_masks = self.get_aces_for_principal_on_object(
+            object_path, principal_name
+        )
+
+        return self.FULL_CONTROL_MASK in access_allowed_masks and len(access_denied_masks) == 0
+
+    def principal_has_no_permissions_on_object(self, object_path, principal_name):
+        access_allowed_masks, _ = self.get_aces_for_principal_on_object(object_path, principal_name)
 
-        return False
+        return len(access_allowed_masks) == 0
 
 
 @pytest.mark.xfail(