feat!: remove group property from WindowsSessionUser (#102)

Summary:

The design of the WindowsSessionUser is currently heavily influenced by
the posix design in that it requires a Group for setting directory ACLs
of the Session Working Directory. Windows has finer grained ACLs for
controlling access to files and directories, so we should be exposing an
interface that works better with that instead.

Solution:

We no longer accept a group in the WindowsSessionUser. The Session
Working Directory's ACLS are now set to:
1. Process owner -- full control
2. Given user -- modify/read/write/execute

**BREAKING CHANGE**
The "group" property of WindowsSessionUser has been removed.

Signed-off-by: Daniel Neilson <[email protected]>

src/openjd/sessions/_embedded_files.py

@@ -77,8 +77,10 @@ def write_file_for_user(
         if user is not None:
             user = cast(WindowsSessionUser, user)
             process_user = get_process_user()
-            WindowsPermissionHelper.set_permissions_full_control(
-                str(filename), [process_user, user.user]
+            WindowsPermissionHelper.set_permissions(
+                str(filename),
+                principals_full_control=[process_user],
+                principals_modify_access=[user.user],
             )
 
 

src/openjd/sessions/_session.py

@@ -162,6 +162,23 @@ class Session(object):
     module's LOG Logger at log level INFO. The LogRecords sent to the log have an
     extra attribute named "session_id" whose value is the session_id that was passed
     to the constructor of the Session.
+
+    Each Session has its own temporary working directory, referred to as the Session
+    Working Directory. Instantiating this class creates this directory for the duration
+    of the Session, and you are expected to call the cleanup() method on your Session
+    to delete that directory when done (this is done automatically if using the Session
+    as a context manager).
+    On POSIX:
+    - The Session Working Directory's owner is the process owner.
+    - If a PosixSessionUser is provided, then the group of that user is the group owner
+      of the the directory.
+    On Windows:
+    - All files and directories within the Session Working Directory inherit the
+      directory's ACL.
+    - The Session Working Directory's ACL is set so that the process owner has full
+      control
+    - If a WindowsSessionUser is provided then the user within that SessionUser
+      is also given Modify access.
     """
 
     _state: SessionState

src/openjd/sessions/_session_user.py

@@ -112,9 +112,16 @@ class WindowsSessionUser(SessionUser):
        includes a logon session obtained by ssh-ing into the host), then you must instantiate this
        class with a username + logon_token; providing a password is not allowed in Session 0. To
        create a logon_token, you will want to look in to the LogonUser family of Win32 system APIs.
+
+    The user provided in this class directly influences the Directory ACL of the Session Working
+    Directory that is created. The created directory:
+    1. Has Full Control by the owner of the calling process; and
+    2. Has Modify access by the provided user.
+    The Session working directory will also be set so that all child directories and files
+    inherit these permissions.
     """
 
-    __slots__ = ("user", "group", "password", "logon_token")
+    __slots__ = ("user", "password", "logon_token")
 
     user: str
     """
@@ -123,13 +130,6 @@ class with a username + logon_token; providing a password is not allowed in Sess
     ex: localUser, domain\\domainUser
     """
 
-    group: str
-    """
-    Group name of the identity to run the Session's subprocesses under.
-    This can be just a group name for a local group, or a domain group in down-level logon form.
-    ex: localGroup, domain\\domainGroup
-    """
-
     password: Optional[str]
     """
     Password of the identity to run the Session's subprocess(es) under.
@@ -146,7 +146,6 @@ def __init__(
         self,
         user: str,
         *,
-        group: Optional[str] = None,
         password: Optional[str] = None,
         logon_token: Optional[HANDLE] = None,
     ) -> None:
@@ -158,12 +157,6 @@ def __init__(
                 or a domain's UPN.
                 ex: localUser, domain\\domainUser, [email protected]
 
-            group (Optional[str]):
-                Group name of the identity to run the Session's subprocesses under.
-                This can be just a group name for a local group, or a domain group in down-level format.
-                ex: localGroup, domain\\domainGroup
-                Defaults to the username if not provided.
-
             password (Optional[str]):
                 Password of the identity to run the Session's subprocess under. This argument is mutually-exclusive with the
                 "logon_token" argument.
@@ -184,7 +177,6 @@ def __init__(
             )
 
         self.user = user
-        self.group = group if group else user
 
         domain, username_without_domain = self._split_domain_and_username(user)
 

src/openjd/sessions/_tempdir.py

@@ -122,12 +122,10 @@ def __init__(
             elif is_windows():
                 user = cast(WindowsSessionUser, user)
                 try:
-                    principal_to_permit = user.group
-
-                    process_user = get_process_user()
-
-                    WindowsPermissionHelper.set_permissions_full_control(
-                        str(self.path), [principal_to_permit, process_user]
+                    WindowsPermissionHelper.set_permissions(
+                        str(self.path),
+                        principals_full_control=[get_process_user()],
+                        principals_modify_access=[user.user],
                     )
                 except Exception as err:
                     raise RuntimeError(

src/openjd/sessions/_windows_permission_helper.py

@@ -13,29 +13,52 @@ class WindowsPermissionHelper:
     """
 
     @staticmethod
-    def set_permissions_full_control(file_path, principals_to_permit):
+    def set_permissions(
+        file_path: str,
+        *,
+        principals_full_control: list[str] = [],
+        principals_modify_access: list[str] = [],
+    ):
         """
-        Grants full control over the object at file_path to all principals in principals_to_permit.
+        Grants access control over the object at file_path.
         Sets flags so both child files and directories inherit these permissions.
 
         Arguments:
             file_path (str): The path to the file or directory.
-            principals_to_permit (List[str]): The names of the principals to permit.
+            principals_full_control (List[str]): The names of the principals to permit Full Control access.
+            principals_modify_access (List[str]): The names of the principals to permit Modify access
 
         Raises:
             RuntimeError if there is a problem modifying the security attributes.
         """
         try:
             # We don't want to propagate existing permissions, so create a new DACL
             dacl = win32security.ACL()
-            for principal in principals_to_permit:
+            for principal in principals_full_control:
                 user_or_group_sid, _, _ = win32security.LookupAccountName(None, principal)
 
                 # 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,
+                    ntsecuritycon.FILE_ALL_ACCESS,  # = 0x1F01FF
+                    user_or_group_sid,
+                )
+            for principal in principals_modify_access:
+                user_or_group_sid, _, _ = win32security.LookupAccountName(None, principal)
+
+                # 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,
+                    # Values of these constants defined in winnt.h
+                    # Constant value after ORs: 0x1301FF
+                    # Delta from FILE_ALL_ACCESS is 0xC0000 = WRITE_DAC(0x40000) | WRITE_OWNER(0x80000)
+                    ntsecuritycon.FILE_GENERIC_READ  # = 0x120089
+                    | ntsecuritycon.FILE_GENERIC_WRITE  # = 0x120116
+                    | ntsecuritycon.FILE_GENERIC_EXECUTE  # = 0x1200A0
+                    | ntsecuritycon.DELETE  # = 0x10000
+                    | ntsecuritycon.FILE_DELETE_CHILD,  # = 0x0040
                     user_or_group_sid,
                 )
 
@@ -54,6 +77,8 @@ def set_permissions_full_control(file_path, principals_to_permit):
             sd.SetSecurityDescriptorDacl(1, dacl, 0)
 
             # Set the security descriptor to the tempdir
+            # Note: This completely overwrites the DACL; so, if we don't provide a permission above then
+            # the DACL doesn't have it.
             win32security.SetFileSecurity(
                 str(file_path), win32security.DACL_SECURITY_INFORMATION, sd
             )

test/openjd/sessions/test_embedded_files.py

@@ -9,7 +9,7 @@
 from openjd.sessions._os_checker import is_posix, is_windows
 import pytest
 
-from utils.windows_acl_helper import principal_has_full_control_of_object
+from utils.windows_acl_helper import MODIFY_READ_WRITE_MASK, principal_has_access_to_object
 
 from openjd.model import SymbolTable
 from openjd.model.v2023_09 import DataString as DataString_2023_09
@@ -358,9 +358,9 @@ def test_changes_owner(self, tmp_path: Path, windows_user: WindowsSessionUser) -
 
             # THEN
             assert os.path.exists(filename)
-            assert principal_has_full_control_of_object(
-                str(filename), windows_user.user
-            ), "Windows user has full control"
+            assert principal_has_access_to_object(
+                str(filename), windows_user.user, MODIFY_READ_WRITE_MASK
+            ), "Windows user has access"
             with open(filename, "r") as file:
                 result_contents = file.read()
             assert result_contents == testdata, "File contents are as expected"
@@ -588,9 +588,9 @@ class Datum:
                     result_contents = file.read()
                 assert result_contents == data.data, "File contents are as expected"
                 # Check file permissions
-                assert principal_has_full_control_of_object(
-                    filename, windows_user.user
-                ), "Windows user has full control"
+                assert principal_has_access_to_object(
+                    filename, windows_user.user, MODIFY_READ_WRITE_MASK
+                ), "Windows user has access"
 
         def test_resolves_symbols(self, tmp_path: Path) -> None:
             # Tests that the set of files can reference themselves and each other

test/openjd/sessions/test_session.py

@@ -371,10 +371,14 @@ def test_cleanup_windows_user(
         with open(working_dir_file_path, "w") as f:
             f.write("File content")
 
-        WindowsPermissionHelper.set_permissions_full_control(subdir_path, [windows_user.user])
-        WindowsPermissionHelper.set_permissions_full_control(subdir_file_path, [windows_user.user])
-        WindowsPermissionHelper.set_permissions_full_control(
-            working_dir_file_path, [windows_user.user]
+        WindowsPermissionHelper.set_permissions(
+            subdir_path, principals_full_control=[windows_user.user]
+        )
+        WindowsPermissionHelper.set_permissions(
+            subdir_file_path, principals_full_control=[windows_user.user]
+        )
+        WindowsPermissionHelper.set_permissions(
+            working_dir_file_path, principals_full_control=[windows_user.user]
         )
 
         session.cleanup()

test/openjd/sessions/test_session_user.py

@@ -28,11 +28,7 @@ class TestWindowsSessionUser:
         return_value=False,
     )
     def test_user_not_converted(self, mock_is_process_user, mock_validate_username, user):
-        windows_session_user = WindowsSessionUser(
-            user,
-            password="password",
-            group="test_group",
-        )
+        windows_session_user = WindowsSessionUser(user, password="password")
 
         assert windows_session_user.user == user
 
@@ -41,7 +37,7 @@ def test_no_password_impersonation_throws_exception(self):
             RuntimeError,
             match="Must supply a password or logon token. User is not the process owner.",
         ):
-            WindowsSessionUser("nonexistent_user", group="test_group")
+            WindowsSessionUser("nonexistent_user")
 
     @pytest.mark.skipif(
         tests_are_in_windows_session_0(),