From 44c3e59b7324cdcfaa80c95da3f049577c1a12d5 Mon Sep 17 00:00:00 2001 From: Christophe Haen Date: Wed, 28 Aug 2024 13:02:58 +0200 Subject: [PATCH 1/2] fix: do not used nested policy for SandboxPolicy (#283) --- .../routers/job_manager/access_policies.py | 18 ++---------------- .../diracx/routers/job_manager/sandboxes.py | 16 ++++++++++------ .../tests/jobs/test_wms_access_policy.py | 12 ------------ 3 files changed, 12 insertions(+), 34 deletions(-) diff --git a/diracx-routers/src/diracx/routers/job_manager/access_policies.py b/diracx-routers/src/diracx/routers/job_manager/access_policies.py index 40d6a75b..671996f0 100644 --- a/diracx-routers/src/diracx/routers/job_manager/access_policies.py +++ b/diracx-routers/src/diracx/routers/job_manager/access_policies.py @@ -97,8 +97,8 @@ async def policy( class SandboxAccessPolicy(BaseAccessPolicy): - """Policy for the sandbox - It delegates most of it to the WMSPolicy. + """Policy for the sandbox. + They are similar to the WMS access policies. """ @staticmethod @@ -108,25 +108,11 @@ async def policy( /, *, action: ActionType | None = None, - job_db: JobDB | None = None, sandbox_metadata_db: SandboxMetadataDB | None = None, pfns: list[str] | None = None, required_prefix: str | None = None, - job_ids: list[int] | None = None, - check_wms_permissions: CheckWMSPolicyCallable | None = None, ): - assert action, "action is a mandatory parameter" - - # if we pass the job_db or job_ids, - # delegate the check to the WMSAccessPolicy - if job_db or job_ids: - # Make sure that check_wms_permission is set - # It should always be by fastapi Depends, - # but not when we test the policy in itself - assert check_wms_permissions - return check_wms_permissions(action=action, job_db=job_db, job_ids=job_ids) - assert sandbox_metadata_db, "sandbox_metadata_db is a mandatory parameter" assert pfns, "pfns is a mandatory parameter" diff --git a/diracx-routers/src/diracx/routers/job_manager/sandboxes.py b/diracx-routers/src/diracx/routers/job_manager/sandboxes.py index 6e62379c..d8fcf530 100644 --- a/diracx-routers/src/diracx/routers/job_manager/sandboxes.py +++ b/diracx-routers/src/diracx/routers/job_manager/sandboxes.py @@ -26,7 +26,11 @@ from diracx.core.settings import ServiceSettingsBase from ..utils.users import AuthorizedUserInfo, verify_dirac_access_token -from .access_policies import ActionType, CheckSandboxPolicyCallable +from .access_policies import ( + ActionType, + CheckSandboxPolicyCallable, + CheckWMSPolicyCallable, +) if TYPE_CHECKING: from types_aiobotocore_s3.client import S3Client @@ -221,7 +225,7 @@ async def get_job_sandboxes( job_id: int, sandbox_metadata_db: SandboxMetadataDB, job_db: JobDB, - check_permissions: CheckSandboxPolicyCallable, + check_permissions: CheckWMSPolicyCallable, ) -> dict[str, list[Any]]: """Get input and output sandboxes of given job.""" await check_permissions(action=ActionType.READ, job_db=job_db, job_ids=[job_id]) @@ -241,7 +245,7 @@ async def get_job_sandbox( sandbox_metadata_db: SandboxMetadataDB, job_db: JobDB, sandbox_type: Literal["input", "output"], - check_permissions: CheckSandboxPolicyCallable, + check_permissions: CheckWMSPolicyCallable, ) -> list[Any]: """Get input or output sandbox of given job.""" await check_permissions(action=ActionType.READ, job_db=job_db, job_ids=[job_id]) @@ -259,7 +263,7 @@ async def assign_sandbox_to_job( sandbox_metadata_db: SandboxMetadataDB, job_db: JobDB, settings: SandboxStoreSettings, - check_permissions: CheckSandboxPolicyCallable, + check_permissions: CheckWMSPolicyCallable, ): """Map the pfn as output sandbox to job.""" await check_permissions(action=ActionType.MANAGE, job_db=job_db, job_ids=[job_id]) @@ -277,7 +281,7 @@ async def unassign_job_sandboxes( job_id: int, sandbox_metadata_db: SandboxMetadataDB, job_db: JobDB, - check_permissions: CheckSandboxPolicyCallable, + check_permissions: CheckWMSPolicyCallable, ): """Delete single job sandbox mapping.""" await check_permissions(action=ActionType.MANAGE, job_db=job_db, job_ids=[job_id]) @@ -289,7 +293,7 @@ async def unassign_bulk_jobs_sandboxes( jobs_ids: Annotated[list[int], Query()], sandbox_metadata_db: SandboxMetadataDB, job_db: JobDB, - check_permissions: CheckSandboxPolicyCallable, + check_permissions: CheckWMSPolicyCallable, ): """Delete bulk jobs sandbox mapping.""" await check_permissions(action=ActionType.MANAGE, job_db=job_db, job_ids=jobs_ids) diff --git a/diracx-routers/tests/jobs/test_wms_access_policy.py b/diracx-routers/tests/jobs/test_wms_access_policy.py index 9f878a1a..40e05d29 100644 --- a/diracx-routers/tests/jobs/test_wms_access_policy.py +++ b/diracx-routers/tests/jobs/test_wms_access_policy.py @@ -223,18 +223,6 @@ async def summary_other_vo(*args): ) -async def test_sandbox_access_policy_delegate_to_wms(job_db): - """We expect that the policy delegates to the WMS policy when given job info - This will trigger an Assert as the WMSAccessPolicy is None - in these tests. - """ - normal_user = AuthorizedUserInfo(properties=[NORMAL_USER], **base_payload) - with pytest.raises(AssertionError): - await SandboxAccessPolicy.policy( - SANDBOX_POLICY_NAME, normal_user, action=ActionType.CREATE, job_db=job_db - ) - - async def test_sandbox_access_policy_create(sandbox_db): admin_user = AuthorizedUserInfo(properties=[JOB_ADMINISTRATOR], **base_payload) From 2903afb4627d9231b15462cd8c70b2859a936d9e Mon Sep 17 00:00:00 2001 From: Christophe Haen Date: Wed, 28 Aug 2024 14:31:40 +0200 Subject: [PATCH 2/2] feat: introduce DevlopmentSettings, and only crash in CI for missed access policy (#275) --- .github/workflows/main.yml | 2 +- diracx-core/src/diracx/core/config/__init__.py | 1 - diracx-core/src/diracx/core/settings.py | 10 ++++++++++ diracx-routers/src/diracx/routers/__init__.py | 11 ++++++++++- .../src/diracx/routers/access_policies.py | 11 ++++++++--- .../src/diracx/routers/auth/well_known.py | 6 ++++-- .../src/diracx/routers/dependencies.py | 5 +++++ .../tests/auth/test_legacy_exchange.py | 8 +++++++- diracx-routers/tests/jobs/test_sandboxes.py | 1 + diracx-routers/tests/test_generic.py | 7 ++++++- diracx-routers/tests/test_job_manager.py | 1 + diracx-testing/src/diracx/testing/__init__.py | 18 +++++++++++++++++- 12 files changed, 70 insertions(+), 11 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 99124126..98fe855a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -78,7 +78,7 @@ jobs: - name: Start demo run: | git clone https://github.com/DIRACGrid/diracx-charts.git ../diracx-charts - ../diracx-charts/run_demo.sh --enable-open-telemetry --enable-coverage --exit-when-done --set-value developer.autoReload=false $PWD + ../diracx-charts/run_demo.sh --enable-open-telemetry --enable-coverage --exit-when-done --set-value developer.autoReload=false --ci-values ../diracx-charts/demo/ci_values.yaml $PWD - name: Debugging information run: | DIRACX_DEMO_DIR=$PWD/../diracx-charts/.demo diff --git a/diracx-core/src/diracx/core/config/__init__.py b/diracx-core/src/diracx/core/config/__init__.py index 1fdbfe68..889efb2f 100644 --- a/diracx-core/src/diracx/core/config/__init__.py +++ b/diracx-core/src/diracx/core/config/__init__.py @@ -204,7 +204,6 @@ def __hash__(self): @cachedmethod(lambda self: self._pull_cache) def _pull(self): """Git pull from remote repo.""" - print("CHRIS PULL") self.repo.remotes.origin.pull() def latest_revision(self) -> tuple[str, datetime]: diff --git a/diracx-core/src/diracx/core/settings.py b/diracx-core/src/diracx/core/settings.py index 936dad05..f451b72c 100644 --- a/diracx-core/src/diracx/core/settings.py +++ b/diracx-core/src/diracx/core/settings.py @@ -78,3 +78,13 @@ def create(cls) -> Self: async def lifetime_function(self) -> AsyncIterator[None]: """A context manager that can be used to run code at startup and shutdown.""" yield + + +class DevelopmentSettings(ServiceSettingsBase): + """Settings for the Development Configuration that can influence run time.""" + + model_config = SettingsConfigDict(env_prefix="DIRACX_DEV_") + + # When then to true (only for demo/CI), crash if an access policy isn't + # called + crash_on_missed_access_policy: bool = False diff --git a/diracx-routers/src/diracx/routers/__init__.py b/diracx-routers/src/diracx/routers/__init__.py index 2ff15cde..033d188a 100644 --- a/diracx-routers/src/diracx/routers/__init__.py +++ b/diracx-routers/src/diracx/routers/__init__.py @@ -13,7 +13,15 @@ from collections.abc import AsyncGenerator from functools import partial from logging import Formatter, StreamHandler -from typing import Any, Awaitable, Callable, Iterable, Sequence, TypeVar, cast +from typing import ( + Any, + Awaitable, + Callable, + Iterable, + Sequence, + TypeVar, + cast, +) import dotenv from cachetools import TTLCache @@ -139,6 +147,7 @@ def create_app_inner( # Please see ServiceSettingsBase for more details available_settings_classes: set[type[ServiceSettingsBase]] = set() + for service_settings in all_service_settings: cls = type(service_settings) assert cls not in available_settings_classes diff --git a/diracx-routers/src/diracx/routers/access_policies.py b/diracx-routers/src/diracx/routers/access_policies.py index cb4d5a7f..2568d6b3 100644 --- a/diracx-routers/src/diracx/routers/access_policies.py +++ b/diracx-routers/src/diracx/routers/access_policies.py @@ -26,6 +26,7 @@ from fastapi import Depends from diracx.core.extensions import select_from_extension +from diracx.routers.dependencies import DevelopmentSettings from diracx.routers.utils.users import AuthorizedUserInfo, verify_dirac_access_token # FastAPI bug: @@ -99,6 +100,7 @@ def check_permissions( policy: Callable, policy_name: str, user_info: Annotated[AuthorizedUserInfo, Depends(verify_dirac_access_token)], + dev_settings: DevelopmentSettings, ): """This wrapper just calls the actual implementation, but also makes sure that the policy has been called. @@ -120,6 +122,7 @@ async def wrapped_policy(**kwargs): try: yield wrapped_policy finally: + if not has_been_called: # TODO nice error message with inspect # That should really not happen @@ -128,9 +131,11 @@ async def wrapped_policy(**kwargs): "(PS: I hope you are in a CI)", flush=True, ) - # Sleep a bit to make sure the flush happened - time.sleep(1) - os._exit(1) + # If enable, just crash, meanly + if dev_settings.crash_on_missed_access_policy: + # Sleep a bit to make sure the flush happened + time.sleep(1) + os._exit(1) def open_access(f): diff --git a/diracx-routers/src/diracx/routers/auth/well_known.py b/diracx-routers/src/diracx/routers/auth/well_known.py index 285bb6c5..a6abb462 100644 --- a/diracx-routers/src/diracx/routers/auth/well_known.py +++ b/diracx-routers/src/diracx/routers/auth/well_known.py @@ -3,7 +3,7 @@ from fastapi import Request from typing_extensions import TypedDict -from ..dependencies import Config +from ..dependencies import Config, DevelopmentSettings from ..fastapi_classes import DiracxRouter from ..utils.users import AuthSettings @@ -17,7 +17,6 @@ async def openid_configuration( request: Request, config: Config, settings: AuthSettings, - # check_permissions: OpenAccessPolicyCallable, ): """OpenID Connect discovery endpoint.""" # await check_permissions() @@ -65,17 +64,20 @@ class VOInfo(TypedDict): class Metadata(TypedDict): virtual_organizations: dict[str, VOInfo] + development_settings: DevelopmentSettings @router.get("/dirac-metadata") async def installation_metadata( config: Config, # check_permissions: OpenAccessPolicyCallable, + dev_settings: DevelopmentSettings, ) -> Metadata: """Get metadata about the dirac installation.""" # await check_permissions() metadata: Metadata = { "virtual_organizations": {}, + "development_settings": dev_settings, } for vo, vo_info in config.Registry.items(): groups: dict[str, GroupInfo] = { diff --git a/diracx-routers/src/diracx/routers/dependencies.py b/diracx-routers/src/diracx/routers/dependencies.py index 7a67b94f..418aedfb 100644 --- a/diracx-routers/src/diracx/routers/dependencies.py +++ b/diracx-routers/src/diracx/routers/dependencies.py @@ -18,6 +18,7 @@ from diracx.core.config import Config as _Config from diracx.core.config import ConfigSource from diracx.core.properties import SecurityProperty +from diracx.core.settings import DevelopmentSettings as _DevelopmentSettings from diracx.db.sql import AuthDB as _AuthDB from diracx.db.sql import JobDB as _JobDB from diracx.db.sql import JobLoggingDB as _JobLoggingDB @@ -46,3 +47,7 @@ def add_settings_annotation(cls: T) -> T: AvailableSecurityProperties = Annotated[ set[SecurityProperty], Depends(SecurityProperty.available_properties) ] + +DevelopmentSettings = Annotated[ + _DevelopmentSettings, Depends(_DevelopmentSettings.create) +] diff --git a/diracx-routers/tests/auth/test_legacy_exchange.py b/diracx-routers/tests/auth/test_legacy_exchange.py index cce3ebeb..62a4acdf 100644 --- a/diracx-routers/tests/auth/test_legacy_exchange.py +++ b/diracx-routers/tests/auth/test_legacy_exchange.py @@ -9,7 +9,13 @@ DIRAC_CLIENT_ID = "myDIRACClientID" pytestmark = pytest.mark.enabled_dependencies( - ["AuthDB", "AuthSettings", "ConfigSource", "BaseAccessPolicy"] + [ + "AuthDB", + "AuthSettings", + "ConfigSource", + "BaseAccessPolicy", + "DevelopmentSettings", + ] ) diff --git a/diracx-routers/tests/jobs/test_sandboxes.py b/diracx-routers/tests/jobs/test_sandboxes.py index 2422d900..e5da8f03 100644 --- a/diracx-routers/tests/jobs/test_sandboxes.py +++ b/diracx-routers/tests/jobs/test_sandboxes.py @@ -21,6 +21,7 @@ "SandboxStoreSettings", "WMSAccessPolicy", "SandboxAccessPolicy", + "DevelopmentSettings", ] ) diff --git a/diracx-routers/tests/test_generic.py b/diracx-routers/tests/test_generic.py index 9f31064d..1f09b2a5 100644 --- a/diracx-routers/tests/test_generic.py +++ b/diracx-routers/tests/test_generic.py @@ -1,7 +1,12 @@ import pytest pytestmark = pytest.mark.enabled_dependencies( - ["ConfigSource", "AuthSettings", "OpenAccessPolicy"] + [ + "ConfigSource", + "AuthSettings", + "OpenAccessPolicy", + "DevelopmentSettings", + ] ) diff --git a/diracx-routers/tests/test_job_manager.py b/diracx-routers/tests/test_job_manager.py index 5a32a64f..1eca4170 100644 --- a/diracx-routers/tests/test_job_manager.py +++ b/diracx-routers/tests/test_job_manager.py @@ -78,6 +78,7 @@ "TaskQueueDB", "SandboxMetadataDB", "WMSAccessPolicy", + "DevelopmentSettings", ] ) diff --git a/diracx-testing/src/diracx/testing/__init__.py b/diracx-testing/src/diracx/testing/__init__.py index 3e550527..9cc62f07 100644 --- a/diracx-testing/src/diracx/testing/__init__.py +++ b/diracx-testing/src/diracx/testing/__init__.py @@ -18,6 +18,7 @@ import requests if TYPE_CHECKING: + from diracx.core.settings import DevelopmentSettings from diracx.routers.job_manager.sandboxes import SandboxStoreSettings from diracx.routers.utils.users import AuthorizedUserInfo, AuthSettings @@ -76,6 +77,13 @@ def fernet_key() -> str: return Fernet.generate_key().decode() +@pytest.fixture(scope="session") +def test_dev_settings() -> DevelopmentSettings: + from diracx.core.settings import DevelopmentSettings + + yield DevelopmentSettings() + + @pytest.fixture(scope="session") def test_auth_settings(private_key_pem, fernet_key) -> AuthSettings: from diracx.routers.utils.users import AuthSettings @@ -141,6 +149,7 @@ def __init__( with_config_repo, test_auth_settings, test_sandbox_settings, + test_dev_settings, ): from diracx.core.config import ConfigSource from diracx.core.extensions import select_from_extension @@ -171,6 +180,7 @@ def enrich_tokens(access_payload: dict, refresh_payload: dict): self._cache_dir = tmp_path_factory.mktemp("empty-dbs") self.test_auth_settings = test_auth_settings + self.test_dev_settings = test_dev_settings all_access_policies = { e.name: [AlwaysAllowAccessPolicy] @@ -183,6 +193,7 @@ def enrich_tokens(access_payload: dict, refresh_payload: dict): all_service_settings=[ test_auth_settings, test_sandbox_settings, + test_dev_settings, ], database_urls=database_urls, os_database_conn_kwargs={ @@ -346,13 +357,18 @@ def session_client_factory( test_sandbox_settings, with_config_repo, tmp_path_factory, + test_dev_settings, ): """TODO. ---- """ yield ClientFactory( - tmp_path_factory, with_config_repo, test_auth_settings, test_sandbox_settings + tmp_path_factory, + with_config_repo, + test_auth_settings, + test_sandbox_settings, + test_dev_settings, )