From 21a6cf5d921f9f6416bf8c1c1c21b105419d003b Mon Sep 17 00:00:00 2001 From: Joseph Ware <53935796+DiamondJoseph@users.noreply.github.com> Date: Thu, 19 Dec 2024 15:30:04 +0000 Subject: [PATCH 1/4] Pass appVersion to helm package command (#766) --- .github/workflows/_container.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/_container.yml b/.github/workflows/_container.yml index 8688011c3..cdfc9a7d8 100644 --- a/.github/workflows/_container.yml +++ b/.github/workflows/_container.yml @@ -106,7 +106,6 @@ jobs: - name: package chart and push it run: | - sed -i "$ a appVersion: ${GITHUB_REF##*/}" helm/blueapi/Chart.yaml helm dependencies update helm/blueapi - helm package helm/blueapi --version ${GITHUB_REF##*/} -d /tmp/ + helm package helm/blueapi --version ${GITHUB_REF##*/} --app-version ${GITHUB_REF##*/} -d /tmp/ helm push /tmp/blueapi-${GITHUB_REF##*/}.tgz oci://ghcr.io/diamondlightsource/charts From 82b208f5622752a6dbb43de7cf1c393849040c7d Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Tue, 7 Jan 2025 09:31:21 +0000 Subject: [PATCH 2/4] Check for common permissions issues with scratch area (#765) Fixes #747 --- docs/how-to/edit-live.md | 5 + src/blueapi/cli/cli.py | 5 + src/blueapi/cli/scratch.py | 38 ++++++- src/blueapi/config.py | 30 +++++- src/blueapi/utils/__init__.py | 3 + src/blueapi/utils/file_permissions.py | 32 ++++++ tests/unit_tests/cli/test_scratch.py | 100 ++++++++++++++---- tests/unit_tests/test_cli.py | 15 +++ tests/unit_tests/test_config.py | 2 + .../unit_tests/utils/test_file_permissions.py | 65 ++++++++++++ 10 files changed, 265 insertions(+), 30 deletions(-) create mode 100644 src/blueapi/utils/file_permissions.py create mode 100644 tests/unit_tests/utils/test_file_permissions.py 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 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 d731eeeb5..e2161799b 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 get_owner_gid, is_sgid_set _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, config.required_gid) logging.info(f"Setting up scratch area: {config.root}") @@ -74,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( [ @@ -94,6 +93,37 @@ 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, required_gid: int | None) -> None: + _validate_directory(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 + 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 + set 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 chgrp + command. + """) + ) + + def _validate_directory(path: Path) -> None: if not path.exists(): raise KeyError(f"{path}: No such file or directory") diff --git a/src/blueapi/config.py b/src/blueapi/config.py index 8f53878cc..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,13 +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") - 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): diff --git a/src/blueapi/utils/__init__.py b/src/blueapi/utils/__init__.py index cf45f6936..602cea9bb 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 get_owner_gid, is_sgid_set from .invalid_config_error import InvalidConfigError from .modules import load_module_all from .serialization import serialize @@ -14,4 +15,6 @@ "BlueapiPlanModelConfig", "InvalidConfigError", "connect_devices", + "is_sgid_set", + "get_owner_gid", ] diff --git a/src/blueapi/utils/file_permissions.py b/src/blueapi/utils/file_permissions.py new file mode 100644 index 000000000..0e1921f6d --- /dev/null +++ b/src/blueapi/utils/file_permissions.py @@ -0,0 +1,32 @@ +import stat +from pathlib import Path + + +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 + + Args: + path: Path to the file to check + + Returns: + bool: True if the SGID bit is set + """ + + mask = path.stat().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 path.stat().st_gid diff --git a/tests/unit_tests/cli/test_scratch.py b/tests/unit_tests/cli/test_scratch.py index 29c5f282d..2af956c88 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 @@ -20,8 +21,17 @@ def directory_path() -> Generator[Path]: @pytest.fixture -def file_path(directory_path: Path) -> Generator[Path]: - file_path = directory_path / str(uuid.uuid4()) +def directory_path_with_sgid(directory_path: Path) -> Path: + os.chmod( + directory_path, + os.stat(directory_path).st_mode + stat.S_ISGID, + ) + return directory_path + + +@pytest.fixture +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 @@ -29,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 @@ -38,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( [ @@ -54,7 +64,7 @@ def test_scratch_install_installs_path( "install", "--no-deps", "-e", - str(directory_path), + str(directory_path_with_sgid), ] ) @@ -73,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() @@ -81,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() @@ -109,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(): @@ -149,15 +159,61 @@ def test_setup_scratch_fails_on_non_directory_root( setup_scratch(config) +def test_setup_scratch_fails_on_non_sgid_root( + directory_path: Path, +): + config = ScratchConfig(root=directory_path, repositories=[]) + with pytest.raises(PermissionError): + setup_scratch(config) + + +def test_setup_scratch_fails_on_wrong_gid( + directory_path_with_sgid: Path, +): + config = ScratchConfig( + root=directory_path_with_sgid, + required_gid=12345, + repositories=[], + ) + assert get_owner_gid(directory_path_with_sgid) != 12345 + with pytest.raises(PermissionError): + 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, +): + # 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=root, + required_gid=12345, + repositories=[], + ) + assert get_owner_gid(root) == 12345 + setup_scratch(config) + + @patch("blueapi.cli.scratch.ensure_repo") @patch("blueapi.cli.scratch.scratch_install") 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", @@ -173,15 +229,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), ] ) @@ -191,10 +247,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", diff --git a/tests/unit_tests/test_cli.py b/tests/unit_tests/test_cli.py index 10d5e03ff..fb918fb66 100644 --- a/tests/unit_tests/test_cli.py +++ b/tests/unit_tests/test_cli.py @@ -60,6 +60,21 @@ def test_main_no_params(): assert result.stdout == expected +@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]) + mock_umask.assert_called_once_with(0o002) + + @patch("requests.request") def test_connection_error_caught_by_wrapper_func( mock_requests: Mock, runner: CliRunner 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", 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..11f36f889 --- /dev/null +++ b/tests/unit_tests/utils/test_file_permissions.py @@ -0,0 +1,65 @@ +import stat +from pathlib import Path +from unittest.mock import Mock + +import pytest + +from blueapi.utils import get_owner_gid, is_sgid_set + + +@pytest.mark.parametrize( + "bits", + [ + # 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) + + +@pytest.mark.parametrize( + "bits", + [ + # 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) + + +def _mocked_is_sgid_set(bits: int) -> bool: + path = Mock(spec=Path) + path.stat().st_mode = bits + + return is_sgid_set(path) + + +def test_get_owner_gid(): + path = Mock(spec=Path) + path.stat().st_gid = 12345 + + assert get_owner_gid(path) == 12345 From 3e36c8cce546f377ecf7a632a8a05e9d7c834e64 Mon Sep 17 00:00:00 2001 From: Callum Forrester Date: Mon, 13 Jan 2025 11:38:10 +0000 Subject: [PATCH 3/4] Update dodal pin to latest (#778) Update dodal to 1.37.0 --- dev-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-requirements.txt b/dev-requirements.txt index 9a0037529..c3d2c7284 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -46,7 +46,7 @@ deepdiff==8.0.1 deepmerge==2.0 Deprecated==1.2.15 distlib==0.3.9 -dls-dodal==1.36.2 +dls-dodal==1.37.0 dnspython==2.7.0 docopt==0.6.2 doct==1.1.0 From efcb4aa7977e00cda59132950abbb881de28fc86 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 15 Jan 2025 11:24:46 +0000 Subject: [PATCH 4/4] Support inject for default plan arguments again (#773) Support inject for default plan arguments again When preparing the parameters for a plan, pydantic creates an instance of the dynamically generated BaseModel, relying on the 'Reference' types to convert from strings to instances of the devices. This includes strings that are used as default field values (default parameters created using the `inject` method). We then convert the model back to a dictionary to pass as kwargs to the plan. The previously used `model_fields_set` field on the model only iterates over the fields set via the input JSON (the user supplied arguments) and skips the fields populated via the default factories in the base model. Using the `__pydantic_fields__` class variable, allows all fields to be used including the defaults. This was previously avoided as it generated warnings for unknown types but it is possible to opt-out of these warnings. --- src/blueapi/worker/task.py | 22 ++------------------ tests/unit_tests/worker/test_task_worker.py | 23 +++++++++++++++++++++ 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/blueapi/worker/task.py b/src/blueapi/worker/task.py index 91ecbfcda..02ad15a20 100644 --- a/src/blueapi/worker/task.py +++ b/src/blueapi/worker/task.py @@ -22,7 +22,8 @@ class Task(BlueapiBaseModel): def prepare_params(self, ctx: BlueskyContext) -> Mapping[str, Any]: model = _lookup_params(ctx, self) - return _model_to_kwargs(model) + # Re-create dict manually to avoid nesting in model_dump output + return {field: getattr(model, field) for field in model.__pydantic_fields__} def do_task(self, ctx: BlueskyContext) -> None: LOGGER.info(f"Asked to run plan {self.name} with {self.params}") @@ -49,22 +50,3 @@ def _lookup_params(ctx: BlueskyContext, task: Task) -> BaseModel: model = plan.model adapter = TypeAdapter(model) return adapter.validate_python(task.params) - - -def _model_to_kwargs(model: BaseModel) -> Mapping[str, Any]: - """ - Converts an instance of BaseModel back to a dictionary that - can be passed as **kwargs. - Used instead of BaseModel.model_dump() because we don't want - the dumping to be nested and because it fires UserWarnings - about data types it is unfamiliar with - (such as ophyd devices). - - Args: - model: Pydantic model to convert to kwargs - - Returns: - Mapping[str, Any]: Dictionary that can be passed as **kwargs - """ - - return {name: getattr(model, name) for name in model.model_fields_set} diff --git a/tests/unit_tests/worker/test_task_worker.py b/tests/unit_tests/worker/test_task_worker.py index 1eae7280c..d593821dc 100644 --- a/tests/unit_tests/worker/test_task_worker.py +++ b/tests/unit_tests/worker/test_task_worker.py @@ -575,3 +575,26 @@ def test_begin_task_span_ok( task_id = worker.submit_task(_SIMPLE_TASK) with asserting_span_exporter(exporter, "begin_task", "task_id"): worker.begin_task(task_id) + + +def test_injected_devices_are_found( + fake_device: FakeDevice, + context: BlueskyContext, +): + def injected_device_plan(dev: FakeDevice = fake_device.name) -> MsgGenerator: # type: ignore + yield from () + + context.register_plan(injected_device_plan) + params = Task(name="injected_device_plan").prepare_params(context) + assert params["dev"] == fake_device + + +def test_missing_injected_devices_fail_early( + context: BlueskyContext, +): + def missing_injection(dev: FakeDevice = "does_not_exist") -> MsgGenerator: # type: ignore + yield from () + + context.register_plan(missing_injection) + with pytest.raises(ValueError): + Task(name="missing_injection").prepare_params(context)