From 6adfc2facfcd82b3877e6384eff92b88eb934e46 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Thu, 19 Dec 2024 11:19:12 +0000 Subject: [PATCH 01/15] Check that SGID bit is enabled for scratch area --- src/blueapi/cli/scratch.py | 21 +++++++- src/blueapi/utils/__init__.py | 2 + src/blueapi/utils/file_permissions.py | 20 +++++++ tests/unit_tests/cli/test_scratch.py | 19 ++++++- .../unit_tests/utils/test_file_permissions.py | 52 +++++++++++++++++++ 5 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 src/blueapi/utils/file_permissions.py create mode 100644 tests/unit_tests/utils/test_file_permissions.py diff --git a/src/blueapi/cli/scratch.py b/src/blueapi/cli/scratch.py index d731eeeb5..fc72f4a08 100644 --- a/src/blueapi/cli/scratch.py +++ b/src/blueapi/cli/scratch.py @@ -1,12 +1,14 @@ import logging import os import stat +import textwrap from pathlib import Path from subprocess import Popen from git import Repo from blueapi.config import ScratchConfig +from blueapi.utils import is_sgid_enabled _DEFAULT_INSTALL_TIMEOUT: float = 300.0 @@ -23,7 +25,7 @@ def setup_scratch( install_timeout: Timeout for installing packages """ - _validate_directory(config.root) + _validate_root_directory(config.root) logging.info(f"Setting up scratch area: {config.root}") @@ -94,6 +96,23 @@ def scratch_install(path: Path, timeout: float = _DEFAULT_INSTALL_TIMEOUT) -> No raise RuntimeError(f"Failed to install {path}: Exit Code: {process.returncode}") +def _validate_root_directory(root_path: Path) -> None: + _validate_directory(root_path) + + if not is_sgid_enabled(root_path): + raise PermissionError( + textwrap.dedent(f""" + The scratch area root directory ({root_path}) needs to have the + SGID permission bit enabled. This allows blueapi to clone + repositories into it while retaining the ability for + other users in an approved group to edit/delete them. + + See https://www.redhat.com/en/blog/suid-sgid-sticky-bit for how to + enable the SGID bit. + """) + ) + + def _validate_directory(path: Path) -> None: if not path.exists(): raise KeyError(f"{path}: No such file or directory") diff --git a/src/blueapi/utils/__init__.py b/src/blueapi/utils/__init__.py index cf45f6936..e60b70572 100644 --- a/src/blueapi/utils/__init__.py +++ b/src/blueapi/utils/__init__.py @@ -1,5 +1,6 @@ from .base_model import BlueapiBaseModel, BlueapiModelConfig, BlueapiPlanModelConfig from .connect_devices import connect_devices +from .file_permissions import is_sgid_enabled from .invalid_config_error import InvalidConfigError from .modules import load_module_all from .serialization import serialize @@ -14,4 +15,5 @@ "BlueapiPlanModelConfig", "InvalidConfigError", "connect_devices", + "is_sgid_enabled", ] diff --git a/src/blueapi/utils/file_permissions.py b/src/blueapi/utils/file_permissions.py new file mode 100644 index 000000000..d977af638 --- /dev/null +++ b/src/blueapi/utils/file_permissions.py @@ -0,0 +1,20 @@ +import os +import stat +from pathlib import Path + + +def is_sgid_enabled(path: Path) -> bool: + """Check if the SGID bit is enabled so that new files created + under a directory owned by a group are owned by that same group. + + See https://www.redhat.com/en/blog/suid-sgid-sticky-bit + + Args: + path: Path to the file to check + + Returns: + bool: True if the SGID bit is enabled + """ + + mask = os.stat(path).st_mode + return bool(mask & stat.S_ISGID) diff --git a/tests/unit_tests/cli/test_scratch.py b/tests/unit_tests/cli/test_scratch.py index 29c5f282d..f321466e4 100644 --- a/tests/unit_tests/cli/test_scratch.py +++ b/tests/unit_tests/cli/test_scratch.py @@ -13,12 +13,21 @@ @pytest.fixture -def directory_path() -> Generator[Path]: +def directory_path_no_permissions() -> Generator[Path]: temporary_directory = TemporaryDirectory() yield Path(temporary_directory.name) temporary_directory.cleanup() +@pytest.fixture +def directory_path(directory_path_no_permissions: Path) -> Path: + os.chmod( + directory_path_no_permissions, + os.stat(directory_path_no_permissions).st_mode + stat.S_ISGID, + ) + return directory_path_no_permissions + + @pytest.fixture def file_path(directory_path: Path) -> Generator[Path]: file_path = directory_path / str(uuid.uuid4()) @@ -149,6 +158,14 @@ def test_setup_scratch_fails_on_non_directory_root( setup_scratch(config) +def test_setup_scratch_fails_on_non_sgid_root( + directory_path_no_permissions: Path, +): + config = ScratchConfig(root=directory_path_no_permissions, repositories=[]) + with pytest.raises(PermissionError): + setup_scratch(config) + + @patch("blueapi.cli.scratch.ensure_repo") @patch("blueapi.cli.scratch.scratch_install") def test_setup_scratch_iterates_repos( diff --git a/tests/unit_tests/utils/test_file_permissions.py b/tests/unit_tests/utils/test_file_permissions.py new file mode 100644 index 000000000..8b02e7e44 --- /dev/null +++ b/tests/unit_tests/utils/test_file_permissions.py @@ -0,0 +1,52 @@ +from pathlib import Path +from unittest.mock import Mock, patch + +import pytest + +from blueapi.utils import is_sgid_enabled + + +@pytest.mark.parametrize( + "bits", + [ + 33152, # -rw-------. + 33279, # -rwxrwxrwx. + 32768, # ----------. + 33188, # -rw-r--r--. + 33024, # -r--------. + 33206, # -rw-rw-rw-. + 33060, # -r--r--r--. + 16895, # drwxrwxrwx. + 16384, # d---------. + 16768, # drw-------. + ], +) +@patch("blueapi.utils.file_permissions.os.stat") +def test_is_sgid_enabled_should_be_disabled(mock_stat: Mock, bits: int): + assert not _mocked_is_sgid_enabled(mock_stat, bits) + + +@pytest.mark.parametrize( + "bits", + [ + 34303, # -rwxrwsrwx. + 33792, # ------S---. + 34212, # -rw-r-Sr--. + 34176, # -rw---S---. + 34048, # -r----S---. + 34230, # -rw-rwSrw-. + 34084, # -r--r-Sr--. + 17919, # drwxrwsrwx. + 17408, # d-----S---. + 17792, # drw---S---. + ], +) +@patch("blueapi.utils.file_permissions.os.stat") +def test_is_sgid_enabled_should_be_enabled(mock_stat: Mock, bits: int): + assert _mocked_is_sgid_enabled(mock_stat, bits) + + +def _mocked_is_sgid_enabled(mock_stat: Mock, bits: int) -> bool: + (mock_stat_for_file := Mock()).st_mode = bits + mock_stat.return_value = mock_stat_for_file + return is_sgid_enabled(Path("/doesnt/matter")) From 0afde5aa9bbf120b1666227df000256dc90760e2 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Thu, 19 Dec 2024 14:35:05 +0000 Subject: [PATCH 02/15] qwAdd option to configure GID for scratch area --- src/blueapi/cli/scratch.py | 20 +++++++++++--- src/blueapi/config.py | 1 + src/blueapi/utils/__init__.py | 3 ++- src/blueapi/utils/file_permissions.py | 13 +++++++++ tests/unit_tests/cli/test_scratch.py | 27 +++++++++++++++++++ .../unit_tests/utils/test_file_permissions.py | 9 ++++++- 6 files changed, 68 insertions(+), 5 deletions(-) diff --git a/src/blueapi/cli/scratch.py b/src/blueapi/cli/scratch.py index fc72f4a08..52c686129 100644 --- a/src/blueapi/cli/scratch.py +++ b/src/blueapi/cli/scratch.py @@ -8,7 +8,7 @@ from git import Repo from blueapi.config import ScratchConfig -from blueapi.utils import is_sgid_enabled +from blueapi.utils import get_owner_gid, is_sgid_enabled _DEFAULT_INSTALL_TIMEOUT: float = 300.0 @@ -25,7 +25,7 @@ def setup_scratch( install_timeout: Timeout for installing packages """ - _validate_root_directory(config.root) + _validate_root_directory(config.root, config.required_gid) logging.info(f"Setting up scratch area: {config.root}") @@ -96,7 +96,7 @@ def scratch_install(path: Path, timeout: float = _DEFAULT_INSTALL_TIMEOUT) -> No raise RuntimeError(f"Failed to install {path}: Exit Code: {process.returncode}") -def _validate_root_directory(root_path: Path) -> None: +def _validate_root_directory(root_path: Path, required_gid: int | None) -> None: _validate_directory(root_path) if not is_sgid_enabled(root_path): @@ -111,6 +111,20 @@ def _validate_root_directory(root_path: Path) -> None: enable the SGID bit. """) ) + elif required_gid is not None and get_owner_gid(root_path) != required_gid: + raise PermissionError( + textwrap.dedent(f""" + The configuration requires that {root_path} be owned by the group with + ID {required_gid}. + You may be able to find this group's name by running the following + in the terminal. + + getent group 1000 | cut -d: -f1 + + You can transfer ownership, if you have sufficient permissions, with the chown + command. + """) + ) def _validate_directory(path: Path) -> None: diff --git a/src/blueapi/config.py b/src/blueapi/config.py index 8f53878cc..27d314f53 100644 --- a/src/blueapi/config.py +++ b/src/blueapi/config.py @@ -81,6 +81,7 @@ class ScratchRepository(BlueapiBaseModel): class ScratchConfig(BlueapiBaseModel): root: Path = Path("/tmp/scratch/blueapi") + required_gid: int | None = None repositories: list[ScratchRepository] = Field(default_factory=list) diff --git a/src/blueapi/utils/__init__.py b/src/blueapi/utils/__init__.py index e60b70572..9a7e86942 100644 --- a/src/blueapi/utils/__init__.py +++ b/src/blueapi/utils/__init__.py @@ -1,6 +1,6 @@ from .base_model import BlueapiBaseModel, BlueapiModelConfig, BlueapiPlanModelConfig from .connect_devices import connect_devices -from .file_permissions import is_sgid_enabled +from .file_permissions import get_owner_gid, is_sgid_enabled from .invalid_config_error import InvalidConfigError from .modules import load_module_all from .serialization import serialize @@ -16,4 +16,5 @@ "InvalidConfigError", "connect_devices", "is_sgid_enabled", + "get_owner_gid", ] diff --git a/src/blueapi/utils/file_permissions.py b/src/blueapi/utils/file_permissions.py index d977af638..9ab5e8033 100644 --- a/src/blueapi/utils/file_permissions.py +++ b/src/blueapi/utils/file_permissions.py @@ -18,3 +18,16 @@ def is_sgid_enabled(path: Path) -> bool: mask = os.stat(path).st_mode return bool(mask & stat.S_ISGID) + + +def get_owner_gid(path: Path) -> int: + """Get the GID of the owner of a file + + Args: + path: Path to the file to check + + Returns: + bool: The GID of the file owner + """ + + return os.stat(path).st_gid diff --git a/tests/unit_tests/cli/test_scratch.py b/tests/unit_tests/cli/test_scratch.py index f321466e4..e44a5a5a6 100644 --- a/tests/unit_tests/cli/test_scratch.py +++ b/tests/unit_tests/cli/test_scratch.py @@ -10,6 +10,7 @@ from blueapi.cli.scratch import ensure_repo, scratch_install, setup_scratch from blueapi.config import ScratchConfig, ScratchRepository +from blueapi.utils import get_owner_gid @pytest.fixture @@ -166,6 +167,32 @@ def test_setup_scratch_fails_on_non_sgid_root( setup_scratch(config) +def test_setup_scratch_fails_on_wrong_gid( + directory_path: Path, +): + config = ScratchConfig( + root=directory_path, + required_gid=12345, + repositories=[], + ) + assert get_owner_gid(directory_path) != 12345 + with pytest.raises(PermissionError): + setup_scratch(config) + + +def test_setup_scratch_succeeds_on_required_gid( + directory_path: Path, +): + os.chown(directory_path, uid=12345, gid=12345) + config = ScratchConfig( + root=directory_path, + required_gid=12345, + repositories=[], + ) + assert get_owner_gid(directory_path) == 12345 + setup_scratch(config) + + @patch("blueapi.cli.scratch.ensure_repo") @patch("blueapi.cli.scratch.scratch_install") def test_setup_scratch_iterates_repos( diff --git a/tests/unit_tests/utils/test_file_permissions.py b/tests/unit_tests/utils/test_file_permissions.py index 8b02e7e44..a8aa44ad9 100644 --- a/tests/unit_tests/utils/test_file_permissions.py +++ b/tests/unit_tests/utils/test_file_permissions.py @@ -3,7 +3,7 @@ import pytest -from blueapi.utils import is_sgid_enabled +from blueapi.utils import get_owner_gid, is_sgid_enabled @pytest.mark.parametrize( @@ -50,3 +50,10 @@ def _mocked_is_sgid_enabled(mock_stat: Mock, bits: int) -> bool: (mock_stat_for_file := Mock()).st_mode = bits mock_stat.return_value = mock_stat_for_file return is_sgid_enabled(Path("/doesnt/matter")) + + +@patch("blueapi.utils.file_permissions.os.stat") +def test_get_owner_gid(mock_stat: Mock): + (mock_stat_for_file := Mock()).st_gid = 12345 + mock_stat.return_value = mock_stat_for_file + assert get_owner_gid(Path("/doesnt/matter")) == 12345 From 7846c68f68c68094ddeb1738ca520c1997aa4df8 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Thu, 19 Dec 2024 15:07:41 +0000 Subject: [PATCH 03/15] Always run blueapi with a umask of 002 --- src/blueapi/cli/cli.py | 5 +++++ src/blueapi/cli/scratch.py | 3 --- tests/__init__.py | 8 ++++++++ tests/unit_tests/test_cli.py | 14 ++++++++++++++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 6a0ab67a4..2697ec63d 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -1,5 +1,7 @@ import json import logging +import os +import stat import sys from functools import wraps from pathlib import Path @@ -39,6 +41,9 @@ def main(ctx: click.Context, config: Path | None | tuple[Path, ...]) -> None: # if no command is supplied, run with the options passed + # Set umask to DLS standard + os.umask(stat.S_IWOTH) + config_loader = ConfigLoader(ApplicationConfig) if config is not None: configs = (config,) if isinstance(config, Path) else config diff --git a/src/blueapi/cli/scratch.py b/src/blueapi/cli/scratch.py index 52c686129..c995f9eae 100644 --- a/src/blueapi/cli/scratch.py +++ b/src/blueapi/cli/scratch.py @@ -76,9 +76,6 @@ def scratch_install(path: Path, timeout: float = _DEFAULT_INSTALL_TIMEOUT) -> No _validate_directory(path) - # Set umask to DLS standard - os.umask(stat.S_IWOTH) - logging.info(f"Installing {path}") process = Popen( [ diff --git a/tests/__init__.py b/tests/__init__.py index e69de29bb..b77bef519 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -0,0 +1,8 @@ +import os + + +def current_umask() -> int: + # You can't get the current umask without changing it + tmp = os.umask(0o022) + os.umask(tmp) + return tmp diff --git a/tests/unit_tests/test_cli.py b/tests/unit_tests/test_cli.py index 10d5e03ff..377c056ea 100644 --- a/tests/unit_tests/test_cli.py +++ b/tests/unit_tests/test_cli.py @@ -15,6 +15,7 @@ from requests.exceptions import ConnectionError from responses import matchers from stomp.connect import StompConnection11 as Connection +from tests import current_umask from blueapi import __version__ from blueapi.cli.cli import main @@ -60,6 +61,19 @@ def test_main_no_params(): assert result.stdout == expected +@patch("blueapi.service.main.start") +@patch("blueapi.cli.scratch.setup_scratch") +@pytest.mark.parametrize("subcommand", ["serve", "setup-scratch"]) +def test_runs_with_umask_002( + mock_setup_scratch: Mock, + mock_start: Mock, + runner: CliRunner, + subcommand: str, +): + runner.invoke(main, [subcommand]) + assert current_umask() == 0o002 + + @patch("requests.request") def test_connection_error_caught_by_wrapper_func( mock_requests: Mock, runner: CliRunner From c529b2bfb328d73352f746db43137ee2ca1e47da Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Thu, 19 Dec 2024 15:10:24 +0000 Subject: [PATCH 04/15] Fix config tests --- tests/unit_tests/test_config.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit_tests/test_config.py b/tests/unit_tests/test_config.py index 26d353a70..67517a691 100644 --- a/tests/unit_tests/test_config.py +++ b/tests/unit_tests/test_config.py @@ -277,6 +277,7 @@ def test_config_yaml_parsed(temp_yaml_config_file): }, "scratch": { "root": "/tmp/scratch/blueapi", + "required_gid": None, "repositories": [ { "name": "dodal", @@ -309,6 +310,7 @@ def test_config_yaml_parsed(temp_yaml_config_file): }, "scratch": { "root": "/tmp/scratch/blueapi", + "required_gid": None, "repositories": [ { "name": "dodal", From 91f7618c82f717e68384b092f2accf26cfa5bc1d Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Thu, 19 Dec 2024 15:18:16 +0000 Subject: [PATCH 05/15] Add docs to config fields --- src/blueapi/config.py | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/blueapi/config.py b/src/blueapi/config.py index 27d314f53..e4581a663 100644 --- a/src/blueapi/config.py +++ b/src/blueapi/config.py @@ -1,3 +1,4 @@ +import textwrap from collections.abc import Mapping from enum import Enum from functools import cached_property @@ -75,14 +76,34 @@ class RestConfig(BlueapiBaseModel): class ScratchRepository(BlueapiBaseModel): - name: str = "example" - remote_url: str = "https://github.com/example/example.git" + name: str = Field( + description="Unique name for this repository in the scratch directory", + default="example", + ) + remote_url: str = Field( + description="URL to clone from", + default="https://github.com/example/example.git", + ) class ScratchConfig(BlueapiBaseModel): - root: Path = Path("/tmp/scratch/blueapi") - required_gid: int | None = None - repositories: list[ScratchRepository] = Field(default_factory=list) + root: Path = Field( + description="The root directory of the scratch area, all repositories will " + "be cloned under this directory.", + default=Path("/tmp/scratch/blueapi"), + ) + required_gid: int | None = Field( + description=textwrap.dedent(""" + Required owner GID for the scratch directory. If supplied the setup-scratch + command will check the scratch area ownership and raise an error if it is + not owned by . + """), + default=None, + ) + repositories: list[ScratchRepository] = Field( + description="Details of repositories to be cloned and imported into blueapi", + default_factory=list, + ) class OIDCConfig(BlueapiBaseModel): From 6fa4bf30f69f694a86c507843dab823d70cc188b Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Thu, 19 Dec 2024 15:19:11 +0000 Subject: [PATCH 06/15] Add reference in docs --- docs/how-to/edit-live.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/how-to/edit-live.md b/docs/how-to/edit-live.md index 8f1eb8c7c..af8695585 100644 --- a/docs/how-to/edit-live.md +++ b/docs/how-to/edit-live.md @@ -11,6 +11,8 @@ Blueapi can be configured to install editable Python packages from a chosen dire scratch: root: /path/to/my/scratch/directory + # Required GID for the scratch area + required_gid: 12345 repositories: # Repository for DLS devices - name: dodal @@ -21,6 +23,9 @@ scratch: remote_url: https://github.com/DiamondLightSource/mx-bluesky.git ``` +Note the `required_gid` field, which is useful for stopping blueapi from locking the files it clones +to a particular owner. + ## Synchronization Blueapi will synchronize reality with the configuration if you run From b3f9a7bceea060b5b200770812e0743916ad3d07 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Mon, 6 Jan 2025 13:47:45 +0000 Subject: [PATCH 07/15] Rename sgid check util function --- src/blueapi/cli/scratch.py | 4 ++-- src/blueapi/utils/__init__.py | 4 ++-- src/blueapi/utils/file_permissions.py | 6 +++--- tests/unit_tests/utils/test_file_permissions.py | 14 +++++++------- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/blueapi/cli/scratch.py b/src/blueapi/cli/scratch.py index c995f9eae..60e89a8b3 100644 --- a/src/blueapi/cli/scratch.py +++ b/src/blueapi/cli/scratch.py @@ -8,7 +8,7 @@ from git import Repo from blueapi.config import ScratchConfig -from blueapi.utils import get_owner_gid, is_sgid_enabled +from blueapi.utils import get_owner_gid, is_sgid_set _DEFAULT_INSTALL_TIMEOUT: float = 300.0 @@ -96,7 +96,7 @@ def scratch_install(path: Path, timeout: float = _DEFAULT_INSTALL_TIMEOUT) -> No def _validate_root_directory(root_path: Path, required_gid: int | None) -> None: _validate_directory(root_path) - if not is_sgid_enabled(root_path): + if not is_sgid_set(root_path): raise PermissionError( textwrap.dedent(f""" The scratch area root directory ({root_path}) needs to have the diff --git a/src/blueapi/utils/__init__.py b/src/blueapi/utils/__init__.py index 9a7e86942..602cea9bb 100644 --- a/src/blueapi/utils/__init__.py +++ b/src/blueapi/utils/__init__.py @@ -1,6 +1,6 @@ from .base_model import BlueapiBaseModel, BlueapiModelConfig, BlueapiPlanModelConfig from .connect_devices import connect_devices -from .file_permissions import get_owner_gid, is_sgid_enabled +from .file_permissions import get_owner_gid, is_sgid_set from .invalid_config_error import InvalidConfigError from .modules import load_module_all from .serialization import serialize @@ -15,6 +15,6 @@ "BlueapiPlanModelConfig", "InvalidConfigError", "connect_devices", - "is_sgid_enabled", + "is_sgid_set", "get_owner_gid", ] diff --git a/src/blueapi/utils/file_permissions.py b/src/blueapi/utils/file_permissions.py index 9ab5e8033..982a550d7 100644 --- a/src/blueapi/utils/file_permissions.py +++ b/src/blueapi/utils/file_permissions.py @@ -3,8 +3,8 @@ from pathlib import Path -def is_sgid_enabled(path: Path) -> bool: - """Check if the SGID bit is enabled so that new files created +def is_sgid_set(path: Path) -> bool: + """Check if the SGID bit is set so that new files created under a directory owned by a group are owned by that same group. See https://www.redhat.com/en/blog/suid-sgid-sticky-bit @@ -13,7 +13,7 @@ def is_sgid_enabled(path: Path) -> bool: path: Path to the file to check Returns: - bool: True if the SGID bit is enabled + bool: True if the SGID bit is set """ mask = os.stat(path).st_mode diff --git a/tests/unit_tests/utils/test_file_permissions.py b/tests/unit_tests/utils/test_file_permissions.py index a8aa44ad9..a265bd7d0 100644 --- a/tests/unit_tests/utils/test_file_permissions.py +++ b/tests/unit_tests/utils/test_file_permissions.py @@ -3,7 +3,7 @@ import pytest -from blueapi.utils import get_owner_gid, is_sgid_enabled +from blueapi.utils import get_owner_gid, is_sgid_set @pytest.mark.parametrize( @@ -22,8 +22,8 @@ ], ) @patch("blueapi.utils.file_permissions.os.stat") -def test_is_sgid_enabled_should_be_disabled(mock_stat: Mock, bits: int): - assert not _mocked_is_sgid_enabled(mock_stat, bits) +def test_is_sgid_set_should_be_disabled(mock_stat: Mock, bits: int): + assert not _mocked_is_sgid_set(mock_stat, bits) @pytest.mark.parametrize( @@ -42,14 +42,14 @@ def test_is_sgid_enabled_should_be_disabled(mock_stat: Mock, bits: int): ], ) @patch("blueapi.utils.file_permissions.os.stat") -def test_is_sgid_enabled_should_be_enabled(mock_stat: Mock, bits: int): - assert _mocked_is_sgid_enabled(mock_stat, bits) +def test_is_sgid_set_should_be_enabled(mock_stat: Mock, bits: int): + assert _mocked_is_sgid_set(mock_stat, bits) -def _mocked_is_sgid_enabled(mock_stat: Mock, bits: int) -> bool: +def _mocked_is_sgid_set(mock_stat: Mock, bits: int) -> bool: (mock_stat_for_file := Mock()).st_mode = bits mock_stat.return_value = mock_stat_for_file - return is_sgid_enabled(Path("/doesnt/matter")) + return is_sgid_set(Path("/doesnt/matter")) @patch("blueapi.utils.file_permissions.os.stat") From c114dbfb20e4c0c52b5003cb5fe4268ebdc0eeb3 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Mon, 6 Jan 2025 13:49:59 +0000 Subject: [PATCH 08/15] Use path.stat() instead of os.stat(path) --- src/blueapi/utils/file_permissions.py | 5 ++- .../unit_tests/utils/test_file_permissions.py | 31 +++++++++---------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/blueapi/utils/file_permissions.py b/src/blueapi/utils/file_permissions.py index 982a550d7..0e1921f6d 100644 --- a/src/blueapi/utils/file_permissions.py +++ b/src/blueapi/utils/file_permissions.py @@ -1,4 +1,3 @@ -import os import stat from pathlib import Path @@ -16,7 +15,7 @@ def is_sgid_set(path: Path) -> bool: bool: True if the SGID bit is set """ - mask = os.stat(path).st_mode + mask = path.stat().st_mode return bool(mask & stat.S_ISGID) @@ -30,4 +29,4 @@ def get_owner_gid(path: Path) -> int: bool: The GID of the file owner """ - return os.stat(path).st_gid + return path.stat().st_gid diff --git a/tests/unit_tests/utils/test_file_permissions.py b/tests/unit_tests/utils/test_file_permissions.py index a265bd7d0..aaa74a6de 100644 --- a/tests/unit_tests/utils/test_file_permissions.py +++ b/tests/unit_tests/utils/test_file_permissions.py @@ -1,5 +1,5 @@ from pathlib import Path -from unittest.mock import Mock, patch +from unittest.mock import Mock import pytest @@ -21,9 +21,8 @@ 16768, # drw-------. ], ) -@patch("blueapi.utils.file_permissions.os.stat") -def test_is_sgid_set_should_be_disabled(mock_stat: Mock, bits: int): - assert not _mocked_is_sgid_set(mock_stat, bits) +def test_is_sgid_set_should_be_disabled(bits: int): + assert not _mocked_is_sgid_set(bits) @pytest.mark.parametrize( @@ -41,19 +40,19 @@ def test_is_sgid_set_should_be_disabled(mock_stat: Mock, bits: int): 17792, # drw---S---. ], ) -@patch("blueapi.utils.file_permissions.os.stat") -def test_is_sgid_set_should_be_enabled(mock_stat: Mock, bits: int): - assert _mocked_is_sgid_set(mock_stat, bits) +def test_is_sgid_set_should_be_enabled(bits: int): + assert _mocked_is_sgid_set(bits) -def _mocked_is_sgid_set(mock_stat: Mock, bits: int) -> bool: - (mock_stat_for_file := Mock()).st_mode = bits - mock_stat.return_value = mock_stat_for_file - return is_sgid_set(Path("/doesnt/matter")) +def _mocked_is_sgid_set(bits: int) -> bool: + path = Mock(spec=Path) + path.stat().st_mode = bits + return is_sgid_set(path) -@patch("blueapi.utils.file_permissions.os.stat") -def test_get_owner_gid(mock_stat: Mock): - (mock_stat_for_file := Mock()).st_gid = 12345 - mock_stat.return_value = mock_stat_for_file - assert get_owner_gid(Path("/doesnt/matter")) == 12345 + +def test_get_owner_gid(): + path = Mock(spec=Path) + path.stat().st_gid = 12345 + + assert get_owner_gid(path) == 12345 From d53079f7afb6d824f56388d13536bb67f616574a Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Mon, 6 Jan 2025 14:06:23 +0000 Subject: [PATCH 09/15] Make test IDs clearer for file permissions checks --- .../unit_tests/utils/test_file_permissions.py | 47 +++++++++++-------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/tests/unit_tests/utils/test_file_permissions.py b/tests/unit_tests/utils/test_file_permissions.py index aaa74a6de..11f36f889 100644 --- a/tests/unit_tests/utils/test_file_permissions.py +++ b/tests/unit_tests/utils/test_file_permissions.py @@ -1,3 +1,4 @@ +import stat from pathlib import Path from unittest.mock import Mock @@ -9,17 +10,20 @@ @pytest.mark.parametrize( "bits", [ - 33152, # -rw-------. - 33279, # -rwxrwxrwx. - 32768, # ----------. - 33188, # -rw-r--r--. - 33024, # -r--------. - 33206, # -rw-rw-rw-. - 33060, # -r--r--r--. - 16895, # drwxrwxrwx. - 16384, # d---------. - 16768, # drw-------. + # Files + 0o10_0600, # -rw-------. + 0o10_0777, # -rwxrwxrwx. + 0o10_0000, # ----------. + 0o10_0644, # -rw-r--r--. + 0o10_0400, # -r--------. + 0o10_0666, # -rw-rw-rw-. + 0o10_0444, # -r--r--r--. + # Directories + 0o04_0777, # drwxrwxrwx. + 0o04_0000, # d---------. + 0o04_0600, # drw-------. ], + ids=lambda p: f"{p:06o} ({stat.filemode(p)})", ) def test_is_sgid_set_should_be_disabled(bits: int): assert not _mocked_is_sgid_set(bits) @@ -28,17 +32,20 @@ def test_is_sgid_set_should_be_disabled(bits: int): @pytest.mark.parametrize( "bits", [ - 34303, # -rwxrwsrwx. - 33792, # ------S---. - 34212, # -rw-r-Sr--. - 34176, # -rw---S---. - 34048, # -r----S---. - 34230, # -rw-rwSrw-. - 34084, # -r--r-Sr--. - 17919, # drwxrwsrwx. - 17408, # d-----S---. - 17792, # drw---S---. + # Files + 0o10_2777, # -rwxrwsrwx. + 0o10_2000, # ------S---. + 0o10_2644, # -rw-r-Sr--. + 0o10_2600, # -rw---S---. + 0o10_2400, # -r----S---. + 0o10_2666, # -rw-rwSrw-. + 0o10_2444, # -r--r-Sr--. + # Directories + 0o04_2777, # drwxrwsrwx. + 0o04_2000, # d-----S---. + 0o04_2600, # drw---S---. ], + ids=lambda p: f"{p:06o} ({stat.filemode(p)})", ) def test_is_sgid_set_should_be_enabled(bits: int): assert _mocked_is_sgid_set(bits) From 6fa9b0e1408e77a5483f02e981878eaeebd8de02 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Mon, 6 Jan 2025 14:11:37 +0000 Subject: [PATCH 10/15] Mock umasking from CLI tests --- tests/__init__.py | 8 -------- tests/unit_tests/test_cli.py | 5 +++-- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/tests/__init__.py b/tests/__init__.py index b77bef519..e69de29bb 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -1,8 +0,0 @@ -import os - - -def current_umask() -> int: - # You can't get the current umask without changing it - tmp = os.umask(0o022) - os.umask(tmp) - return tmp diff --git a/tests/unit_tests/test_cli.py b/tests/unit_tests/test_cli.py index 377c056ea..fb918fb66 100644 --- a/tests/unit_tests/test_cli.py +++ b/tests/unit_tests/test_cli.py @@ -15,7 +15,6 @@ from requests.exceptions import ConnectionError from responses import matchers from stomp.connect import StompConnection11 as Connection -from tests import current_umask from blueapi import __version__ from blueapi.cli.cli import main @@ -63,15 +62,17 @@ def test_main_no_params(): @patch("blueapi.service.main.start") @patch("blueapi.cli.scratch.setup_scratch") +@patch("blueapi.cli.cli.os.umask") @pytest.mark.parametrize("subcommand", ["serve", "setup-scratch"]) def test_runs_with_umask_002( + mock_umask: Mock, mock_setup_scratch: Mock, mock_start: Mock, runner: CliRunner, subcommand: str, ): runner.invoke(main, [subcommand]) - assert current_umask() == 0o002 + mock_umask.assert_called_once_with(0o002) @patch("requests.request") From 928c7b0404b04d8a684ceee6d8ed25c51dbe5670 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Mon, 6 Jan 2025 14:12:47 +0000 Subject: [PATCH 11/15] Suggest different command for transfer of file ownership --- src/blueapi/cli/scratch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/blueapi/cli/scratch.py b/src/blueapi/cli/scratch.py index 60e89a8b3..05cc3a9b1 100644 --- a/src/blueapi/cli/scratch.py +++ b/src/blueapi/cli/scratch.py @@ -118,7 +118,7 @@ def _validate_root_directory(root_path: Path, required_gid: int | None) -> None: getent group 1000 | cut -d: -f1 - You can transfer ownership, if you have sufficient permissions, with the chown + You can transfer ownership, if you have sufficient permissions, with the chgrp command. """) ) From d4250b894f387bbdc50e0ef733d6e90e87584f12 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Mon, 6 Jan 2025 14:31:48 +0000 Subject: [PATCH 12/15] Fix fixture names in scratch tests --- tests/unit_tests/cli/test_scratch.py | 72 ++++++++++++++-------------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/tests/unit_tests/cli/test_scratch.py b/tests/unit_tests/cli/test_scratch.py index e44a5a5a6..cb3536ccc 100644 --- a/tests/unit_tests/cli/test_scratch.py +++ b/tests/unit_tests/cli/test_scratch.py @@ -14,24 +14,24 @@ @pytest.fixture -def directory_path_no_permissions() -> Generator[Path]: +def directory_path() -> Generator[Path]: temporary_directory = TemporaryDirectory() yield Path(temporary_directory.name) temporary_directory.cleanup() @pytest.fixture -def directory_path(directory_path_no_permissions: Path) -> Path: +def directory_path_with_sgid(directory_path: Path) -> Path: os.chmod( - directory_path_no_permissions, - os.stat(directory_path_no_permissions).st_mode + stat.S_ISGID, + directory_path, + os.stat(directory_path).st_mode + stat.S_ISGID, ) - return directory_path_no_permissions + return directory_path @pytest.fixture -def file_path(directory_path: Path) -> Generator[Path]: - file_path = directory_path / str(uuid.uuid4()) +def file_path(directory_path_with_sgid: Path) -> Generator[Path]: + file_path = directory_path_with_sgid / str(uuid.uuid4()) with file_path.open("w") as stream: stream.write("foo") yield file_path @@ -39,8 +39,8 @@ def file_path(directory_path: Path) -> Generator[Path]: @pytest.fixture -def nonexistant_path(directory_path: Path) -> Path: - file_path = directory_path / str(uuid.uuid4()) +def nonexistant_path(directory_path_with_sgid: Path) -> Path: + file_path = directory_path_with_sgid / str(uuid.uuid4()) assert not file_path.exists() return file_path @@ -48,13 +48,13 @@ def nonexistant_path(directory_path: Path) -> Path: @patch("blueapi.cli.scratch.Popen") def test_scratch_install_installs_path( mock_popen: Mock, - directory_path: Path, + directory_path_with_sgid: Path, ): mock_process = Mock() mock_process.returncode = 0 mock_popen.return_value = mock_process - scratch_install(directory_path, timeout=1.0) + scratch_install(directory_path_with_sgid, timeout=1.0) mock_popen.assert_called_once_with( [ @@ -64,7 +64,7 @@ def test_scratch_install_installs_path( "install", "--no-deps", "-e", - str(directory_path), + str(directory_path_with_sgid), ] ) @@ -83,7 +83,7 @@ def test_scratch_install_fails_on_nonexistant_path(nonexistant_path: Path): @pytest.mark.parametrize("code", [1, 2, 65536]) def test_scratch_install_fails_on_non_zero_exit_code( mock_popen: Mock, - directory_path: Path, + directory_path_with_sgid: Path, code: int, ): mock_process = Mock() @@ -91,16 +91,16 @@ def test_scratch_install_fails_on_non_zero_exit_code( mock_popen.return_value = mock_process with pytest.raises(RuntimeError): - scratch_install(directory_path, timeout=1.0) + scratch_install(directory_path_with_sgid, timeout=1.0) @patch("blueapi.cli.scratch.Repo") def test_repo_not_cloned_and_validated_if_found_locally( mock_repo: Mock, - directory_path: Path, + directory_path_with_sgid: Path, ): - ensure_repo("http://example.com/foo.git", directory_path) - mock_repo.assert_called_once_with(directory_path) + ensure_repo("http://example.com/foo.git", directory_path_with_sgid) + mock_repo.assert_called_once_with(directory_path_with_sgid) mock_repo.clone_from.assert_not_called() @@ -119,9 +119,9 @@ def test_repo_cloned_if_not_found_locally( @patch("blueapi.cli.scratch.Repo") def test_repo_cloned_with_correct_umask( mock_repo: Mock, - directory_path: Path, + directory_path_with_sgid: Path, ): - repo_root = directory_path / "foo" + repo_root = directory_path_with_sgid / "foo" file_path = repo_root / "a" def write_repo_files(): @@ -160,36 +160,36 @@ def test_setup_scratch_fails_on_non_directory_root( def test_setup_scratch_fails_on_non_sgid_root( - directory_path_no_permissions: Path, + directory_path: Path, ): - config = ScratchConfig(root=directory_path_no_permissions, repositories=[]) + config = ScratchConfig(root=directory_path, repositories=[]) with pytest.raises(PermissionError): setup_scratch(config) def test_setup_scratch_fails_on_wrong_gid( - directory_path: Path, + directory_path_with_sgid: Path, ): config = ScratchConfig( - root=directory_path, + root=directory_path_with_sgid, required_gid=12345, repositories=[], ) - assert get_owner_gid(directory_path) != 12345 + assert get_owner_gid(directory_path_with_sgid) != 12345 with pytest.raises(PermissionError): setup_scratch(config) def test_setup_scratch_succeeds_on_required_gid( - directory_path: Path, + directory_path_with_sgid: Path, ): - os.chown(directory_path, uid=12345, gid=12345) + os.chown(directory_path_with_sgid, uid=12345, gid=12345) config = ScratchConfig( - root=directory_path, + root=directory_path_with_sgid, required_gid=12345, repositories=[], ) - assert get_owner_gid(directory_path) == 12345 + assert get_owner_gid(directory_path_with_sgid) == 12345 setup_scratch(config) @@ -198,10 +198,10 @@ def test_setup_scratch_succeeds_on_required_gid( def test_setup_scratch_iterates_repos( mock_scratch_install: Mock, mock_ensure_repo: Mock, - directory_path: Path, + directory_path_with_sgid: Path, ): config = ScratchConfig( - root=directory_path, + root=directory_path_with_sgid, repositories=[ ScratchRepository( name="foo", @@ -217,15 +217,15 @@ def test_setup_scratch_iterates_repos( mock_ensure_repo.assert_has_calls( [ - call("http://example.com/foo.git", directory_path / "foo"), - call("http://example.com/bar.git", directory_path / "bar"), + call("http://example.com/foo.git", directory_path_with_sgid / "foo"), + call("http://example.com/bar.git", directory_path_with_sgid / "bar"), ] ) mock_scratch_install.assert_has_calls( [ - call(directory_path / "foo", timeout=120.0), - call(directory_path / "bar", timeout=120.0), + call(directory_path_with_sgid / "foo", timeout=120.0), + call(directory_path_with_sgid / "bar", timeout=120.0), ] ) @@ -235,10 +235,10 @@ def test_setup_scratch_iterates_repos( def test_setup_scratch_continues_after_failure( mock_scratch_install: Mock, mock_ensure_repo: Mock, - directory_path: Path, + directory_path_with_sgid: Path, ): config = ScratchConfig( - root=directory_path, + root=directory_path_with_sgid, repositories=[ ScratchRepository( name="foo", From 9f0ede885a85e1a9523d49a1507c8a6daf867ec3 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Mon, 6 Jan 2025 14:57:05 +0000 Subject: [PATCH 13/15] Fix test permissions issue --- tests/unit_tests/cli/test_scratch.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/unit_tests/cli/test_scratch.py b/tests/unit_tests/cli/test_scratch.py index cb3536ccc..b9dab8ff5 100644 --- a/tests/unit_tests/cli/test_scratch.py +++ b/tests/unit_tests/cli/test_scratch.py @@ -183,13 +183,16 @@ def test_setup_scratch_fails_on_wrong_gid( def test_setup_scratch_succeeds_on_required_gid( directory_path_with_sgid: Path, ): - os.chown(directory_path_with_sgid, uid=12345, gid=12345) + # We may not own the temp root in some environments + root = directory_path_with_sgid / "a-root" + os.makedirs(root) + os.chown(root, uid=12345, gid=12345) config = ScratchConfig( - root=directory_path_with_sgid, + root=root, required_gid=12345, repositories=[], ) - assert get_owner_gid(directory_path_with_sgid) == 12345 + assert get_owner_gid(root) == 12345 setup_scratch(config) From 8b7dc04f40fa31d4dc528ab0d412daeba7478f19 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Mon, 6 Jan 2025 15:48:48 +0000 Subject: [PATCH 14/15] Catch more enables --- src/blueapi/cli/scratch.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/blueapi/cli/scratch.py b/src/blueapi/cli/scratch.py index 05cc3a9b1..e2161799b 100644 --- a/src/blueapi/cli/scratch.py +++ b/src/blueapi/cli/scratch.py @@ -100,12 +100,12 @@ def _validate_root_directory(root_path: Path, required_gid: int | None) -> None: raise PermissionError( textwrap.dedent(f""" The scratch area root directory ({root_path}) needs to have the - SGID permission bit enabled. This allows blueapi to clone + SGID permission bit set. This allows blueapi to clone repositories into it while retaining the ability for other users in an approved group to edit/delete them. See https://www.redhat.com/en/blog/suid-sgid-sticky-bit for how to - enable the SGID bit. + set the SGID bit. """) ) elif required_gid is not None and get_owner_gid(root_path) != required_gid: From a9d6b743ebd156373e895a183086ce2e56411cfd Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Mon, 6 Jan 2025 16:04:08 +0000 Subject: [PATCH 15/15] Skip flaky test --- tests/unit_tests/cli/test_scratch.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/unit_tests/cli/test_scratch.py b/tests/unit_tests/cli/test_scratch.py index b9dab8ff5..2af956c88 100644 --- a/tests/unit_tests/cli/test_scratch.py +++ b/tests/unit_tests/cli/test_scratch.py @@ -180,6 +180,15 @@ def test_setup_scratch_fails_on_wrong_gid( setup_scratch(config) +@pytest.mark.skip( + reason=""" +We can't chown a tempfile in all environments, in particular it +seems to be broken in GH actions at the moment. We should +rewrite these tests to use mocks. + +See https://github.com/DiamondLightSource/blueapi/issues/770 +""" +) def test_setup_scratch_succeeds_on_required_gid( directory_path_with_sgid: Path, ):