Improve policy admin client and related tools

.pylintrc

@@ -258,7 +258,7 @@ spelling-store-unknown-words=no
 [FORMAT]
 
 # Maximum number of characters on a single line.
-max-line-length=100
+max-line-length=80
 
 # Regexp for a line that is allowed to be longer than the limit.
 ignore-long-lines=^\s*(# )?<?https?://\S+>?$

qrexec/client.py

@@ -49,10 +49,9 @@ def call(dest: str, rpcname: str, arg: Optional[str] = None, *, input=None):
     """
     # pylint: disable=redefined-builtin
 
-    command = make_command(dest, rpcname, arg)
-    return subprocess.check_output(
-        command, **prepare_subprocess_kwds(input)
-    ).decode()
+    command = make_command(dest=dest, rpcname=rpcname, arg=arg)
+    kwds = prepare_subprocess_kwds(input, for_popen=False, for_check=True)
+    return subprocess.check_output(command, **kwds).decode()
 
 
 async def call_async(dest, rpcname, arg=None, *, input=None):
@@ -71,26 +70,10 @@ async def call_async(dest, rpcname, arg=None, *, input=None):
     """
     # pylint: disable=redefined-builtin
 
-    command = make_command(dest, rpcname, arg)
-
-    if input is None:
-        stdin = subprocess.DEVNULL
-        to_communicate = None
-    elif isinstance(input, bytes):
-        stdin = subprocess.PIPE
-        to_communicate = input
-    elif isinstance(input, str):
-        stdin = subprocess.PIPE
-        to_communicate = input.encode()
-    else:
-        # Assume this is a file
-        stdin = input
-        to_communicate = None
-
-    process = await asyncio.create_subprocess_exec(
-        *command, stdin=stdin, stdout=subprocess.PIPE
-    )
-
+    command = make_command(dest=dest, rpcname=rpcname, arg=arg)
+    kwds = prepare_subprocess_kwds(input)
+    to_communicate = kwds.pop("input")
+    process = await asyncio.create_subprocess_exec(*command, **kwds)
     stdout, _stderr = await process.communicate(to_communicate)
     if process.returncode != 0:
         raise subprocess.CalledProcessError(process.returncode, command)

qrexec/policy/admin.py

@@ -38,12 +38,44 @@ class PolicyAdminException(Exception):
     """
 
 
-class PolicyAdminTokenException(Exception):
+class PolicyAdminTokenException(PolicyAdminException):
     """
     A token check exception, indicating that a file is in unexpected state.
     """
 
 
+class PolicyAdminFileNotFoundException(PolicyAdminException):
+    """
+    Policy cannot be found.
+    """
+
+
+class PolicyAdminProtocolException(PolicyAdminException):
+    """
+    Client sent an invalid request that it should have known to not send in the
+    first place.
+    """
+
+
+class PolicyAdminSyntaxException(PolicyAdminProtocolException):
+    """
+    Client sent an invalid policy. That it should have known to not send in the
+    first place. Clients should validate the policy before sending them.
+    """
+
+
+class PolicyAdminInvalidFileNameException(PolicyAdminProtocolException):
+    """
+    Client sent a policy with invalid name.
+    """
+
+
+class PolicyAdminInvalidFilePathException(PolicyAdminProtocolException):
+    """
+    Client sent a policy using a path out of bounds.
+    """
+
+
 def method(service_name, *, no_arg=False, no_payload=False):
     def decorator(func):
         func.api_service_name = service_name
@@ -76,8 +108,8 @@ class PolicyAdmin:
     # pylint: disable=no-self-use
 
     def __init__(self, policy_path):
-        self.policy_path = policy_path
-        self.include_path = policy_path / "include"
+        self.policy_path = Path(policy_path)
+        self.include_path = self.policy_path / "include"
 
     def handle_request(
         self, service_name: str, arg: str, payload: bytes
@@ -89,29 +121,29 @@ def handle_request(
         """
 
         if not all(char in RPCNAME_ALLOWED_CHARSET for char in arg):
-            raise PolicyAdminException(
+            raise PolicyAdminProtocolException(
                 'Invalid argument: "{}"\n'
                 "Valid characters are letters, numbers, dot, plus, hyphen and "
                 "underline".format(arg)
             )
 
         func = self._find_method(service_name)
         if not func:
-            raise PolicyAdminException(
+            raise PolicyAdminProtocolException(
                 "unrecognized method: {}".format(service_name)
             )
 
         args = []
 
         if func.api_no_arg:
             if arg != "":
-                raise PolicyAdminException("Unexpected argument")
+                raise PolicyAdminProtocolException("Unexpected argument")
         else:
             args.append(arg)
 
         if func.api_no_payload:
             if payload != b"":
-                raise PolicyAdminException("Unexpected payload")
+                raise PolicyAdminProtocolException("Unexpected payload")
         else:
             args.append(payload)
 
@@ -178,7 +210,7 @@ def policy_include_get(self, arg):
 
     def _common_get(self, path: Path) -> bytes:
         if not path.is_file():
-            raise PolicyAdminException("Not found: {}".format(path))
+            raise PolicyAdminFileNotFoundException("Not found: {}".format(path))
 
         data = path.read_bytes()
         token = compute_token(data)
@@ -198,7 +230,7 @@ def policy_include_replace(self, arg, payload):
 
     def _common_replace(self, path: Path, payload: bytes) -> bytes:
         if b"\n" not in payload:
-            raise PolicyAdminException(
+            raise PolicyAdminProtocolException(
                 "Payload needs to include first line with token"
             )
         token, data = payload.split(b"\n", 1)
@@ -234,7 +266,7 @@ def _common_remove(self, path: Path, payload: str) -> None:
         self._check_token(payload, path)
 
         if not path.is_file():
-            raise PolicyAdminException("Not found: {}".format(path))
+            raise PolicyAdminFileNotFoundException("Not found: {}".format(path))
 
         self._validate(path, None)
 
@@ -245,10 +277,10 @@ def _common_remove(self, path: Path, payload: str) -> None:
     @method("policy.GetFiles", no_payload=True)
     def policy_get_files(self, arg):
         if not isinstance(arg, str) or not arg:
-            raise PolicyAdminException("Service cannot be empty.")
+            raise PolicyAdminProtocolException("Service cannot be empty.")
         invalid_chars = get_invalid_characters(arg, disallowed="+")
         if invalid_chars:
-            raise PolicyAdminException(
+            raise PolicyAdminProtocolException(
                 "Service {!r} contains invalid characters: {!r}".format(
                     arg, invalid_chars
                 )
@@ -276,15 +308,15 @@ def policy_get_files(self, arg):
 
     def _get_path(self, arg: str, dir_path: str, suffix: str) -> Path:
         if not re.compile(r"^[\w-]+$").match(arg):
-            raise PolicyAdminException(
+            raise PolicyAdminInvalidFileNameException(
                 f"Invalid policy file name: {arg}\n"
                 "Names must contain only alphanumeric characters, "
                 "underscore and hyphen."
             )
         path = dir_path / (arg + suffix)
         path = path.resolve()
         if path.parent != dir_path:
-            raise PolicyAdminException(
+            raise PolicyAdminInvalidFilePathException(
                 "Expecting a path inside {}".format(dir_path)
             )
 
@@ -296,7 +328,7 @@ def _validate(self, path: Path, content: Optional[str]):
                 policy_path=self.policy_path, overrides={path: content}
             )
         except PolicySyntaxError as exc:
-            raise PolicyAdminException(
+            raise PolicyAdminSyntaxException(
                 "Policy change validation failed: {}".format(exc)
             ) from exc
 
@@ -312,7 +344,7 @@ def _check_token(self, token: bytes, path: Path):
             return
 
         if not token.startswith(b"sha256:"):
-            raise PolicyAdminException("Unrecognized token")
+            raise PolicyAdminProtocolException("Unrecognized token")
 
         if not path.exists():
             raise PolicyAdminTokenException(

qrexec/policy/admin_client.py

@@ -34,41 +34,115 @@
 """
 
 from typing import List, Tuple
+from warnings import warn
+from subprocess import CalledProcessError
 
 from ..client import call
 
+# The PolicyAdmin*Exception is used by issubclass(..., PolicyAdminException).
+# pylint: disable=unused-import
+from .admin import (
+    PolicyAdminException,
+    PolicyAdminTokenException,
+    PolicyAdminFileNotFoundException,
+    PolicyAdminProtocolException,
+    PolicyAdminSyntaxException,
+    PolicyAdminInvalidFileNameException,
+    PolicyAdminInvalidFilePathException,
+)
+
 
 class PolicyClient:
-    def policy_list(self) -> List[str]:
-        return self.call("policy.List").rstrip("\n").split("\n")
+    def policy_list(self, is_include: bool = False) -> List[str]:
+        rpc = "List"
+        return self.call(rpc, is_include=is_include).rstrip("\n").split("\n")
 
     def policy_include_list(self) -> List[str]:
-        return self.call("policy.include.List").rstrip("\n").split("\n")
+        warn(
+            "Method 'policy_include_list()' is deprecated, use 'policy_list()' "
+            "instead",
+            DeprecationWarning,
+        )
+        return self.policy_list(is_include=True)
 
-    def policy_get(self, name: str) -> Tuple[str, str]:
-        token, content = self.call("policy.Get", name).split("\n", 1)
+    def policy_get(
+        self, name: str, is_include: bool = False
+    ) -> Tuple[str, str]:
+        rpc = "Get"
+        token, content = self.call(
+            service_name=rpc, arg=name, is_include=is_include
+        ).split("\n", 1)
         return content, token
 
     def policy_include_get(self, name: str) -> Tuple[str, str]:
-        token, content = self.call("policy.include.Get", name).split("\n", 1)
-        return content, token
+        warn(
+            "Method 'policy_include_get()' is deprecated, use 'policy_get()' "
+            "instead",
+            DeprecationWarning,
+        )
+        return self.policy_get(name=name, is_include=True)
 
-    def policy_replace(self, name: str, content: str, token="any"):
-        self.call("policy.Replace", name, token + "\n" + content)
+    def policy_replace(
+        self,
+        name: str,
+        content: str,
+        token: str = "any",
+        is_include: bool = False,
+    ):
+        rpc = "Replace"
+        self.call(
+            service_name=rpc,
+            arg=name,
+            payload=token + "\n" + content,
+            is_include=is_include,
+        )
 
     def policy_include_replace(self, name: str, content: str, token="any"):
-        self.call("policy.include.Replace", name, token + "\n" + content)
+        warn(
+            "Method 'policy_include_replace()' is deprecated, use "
+            "'policy_replace()' instead",
+            DeprecationWarning,
+        )
+        return self.policy_replace(
+            name=name, content=content, token=token, is_include=True
+        )
 
-    def policy_remove(self, name: str, token="any"):
-        self.call("policy.Remove", name, token)
+    def policy_remove(
+        self, name: str, token: str = "any", is_include: bool = False
+    ):
+        rpc = "Remove"
+        self.call(
+            service_name=rpc, arg=name, payload=token, is_include=is_include
+        )
 
     def policy_include_remove(self, name: str, token="any"):
-        self.call("policy.include.Remove", name, token)
+        warn(
+            "Method 'policy_include_remove()' is deprecated, use "
+            "'policy_remove()' instead",
+            DeprecationWarning,
+        )
+        return self.policy_remove(name=name, token=token, is_include=True)
 
-    def policy_get_files(self, name: str):
-        result = self.call("policy.GetFiles", name)
+    def policy_get_files(self, name: str) -> list:
+        rpc = "GetFiles"
+        result = self.call(service_name=rpc, arg=name)
         return [] if result == "" else result.rstrip("\n").split("\n")
 
     @staticmethod
-    def call(service_name, arg=None, payload=""):
-        return call("dom0", service_name, arg, input=payload)
+    def call(service_name, arg=None, payload="", is_include: bool = False):
+        if is_include:
+            service_name = "include." + service_name
+        service_name = "policy." + service_name
+        try:
+            return call(
+                dest="dom0", rpcname=service_name, arg=arg, input=payload
+            )
+        except CalledProcessError as exc:
+            stderr_exc = exc.stderr.decode()
+            stderr_exc_type = stderr_exc.split(" ")[0]
+            if stderr_exc_type in globals():
+                exception = globals()[stderr_exc_type]
+                if issubclass(exception, PolicyAdminException):
+                    stderr_exc_msg = stderr_exc[len(exception.__name__ + " ") :]
+                    raise exception(stderr_exc_msg) from exc
+            raise