Merge pull request #1779 from fractal-analytics-platform/use-new-sett…

…ings

Validate SSH user settings in API

fractal_server/app/models/user_settings.py

@@ -5,6 +5,17 @@
 
 
 class UserSettings(SQLModel, table=True):
+    """
+    Comprehensive list of user settings.
+
+    Attributes:
+        ssh_host: SSH-reachable host where a SLURM client is available.
+        ssh_username: User on `ssh_host`.
+        ssh_private_key_path: Path of private SSH key for `ssh_username`.
+        ssh_tasks_dir: Task-venvs base folder on `ssh_host`.
+        ssh_jobs_dir: Jobs base folder on `ssh_host`.
+    """
+
     __tablename__ = "user_settings"
 
     id: Optional[int] = Field(default=None, primary_key=True)

fractal_server/app/routes/api/v2/_aux_functions.py

@@ -23,6 +23,10 @@
 from ....schemas.v2 import JobStatusTypeV2
 from fractal_server.app.models import UserOAuth
 from fractal_server.images import Filters
+from fractal_server.logger import set_logger
+
+
+logger = set_logger(__name__)
 
 
 async def _get_project_check_owner(

fractal_server/app/routes/api/v2/submit.py

@@ -27,6 +27,7 @@
 from ....schemas.v2 import JobCreateV2
 from ....schemas.v2 import JobReadV2
 from ....schemas.v2 import JobStatusTypeV2
+from ...aux.validate_user_settings import validate_user_settings
 from ._aux_functions import _get_dataset_check_owner
 from ._aux_functions import _get_workflow_check_owner
 from ._aux_functions import clean_app_job_list_v2
@@ -109,8 +110,14 @@ async def apply_workflow(
             ),
         )
 
-    # If backend is SLURM, check that the user has required attributes
+    # Validate user settings (which will eventually replace the block below,
+    # where check required user attributes)
     FRACTAL_RUNNER_BACKEND = settings.FRACTAL_RUNNER_BACKEND
+    await validate_user_settings(
+        user=user, backend=settings.FRACTAL_RUNNER_BACKEND, db=db
+    )
+
+    # If backend is SLURM, check that the user has required attributes
     if FRACTAL_RUNNER_BACKEND == "slurm":
         if not user.slurm_user:
             raise HTTPException(

fractal_server/app/routes/api/v2/task_collection.py

@@ -25,6 +25,7 @@
 from ....schemas.v2 import CollectionStatusV2
 from ....schemas.v2 import TaskCollectPipV2
 from ....schemas.v2 import TaskReadV2
+from ...aux.validate_user_settings import validate_user_settings
 from fractal_server.app.models import UserOAuth
 from fractal_server.app.routes.auth import current_active_user
 from fractal_server.app.routes.auth import current_active_verified_user
@@ -41,7 +42,6 @@
 from fractal_server.tasks.v2.endpoint_operations import inspect_package
 from fractal_server.tasks.v2.utils import get_python_interpreter_v2
 
-
 router = APIRouter()
 
 logger = set_logger(__name__)
@@ -107,6 +107,11 @@ async def collect_tasks_pip(
             detail=f"Invalid task-collection object. Original error: {e}",
         )
 
+    # Validate user settings (backend-specific)
+    await validate_user_settings(
+        user=user, backend=settings.FRACTAL_RUNNER_BACKEND, db=db
+    )
+
     # END of SSH/non-SSH common part
 
     if settings.FRACTAL_RUNNER_BACKEND == "slurm_ssh":

fractal_server/app/routes/aux/validate_user_settings.py

@@ -0,0 +1,41 @@
+from fastapi import HTTPException
+from fastapi import status
+from pydantic import ValidationError
+
+from fractal_server.app.db import AsyncSession
+from fractal_server.app.models import UserOAuth
+from fractal_server.app.models import UserSettings
+from fractal_server.app.routes.api.v2._aux_functions import logger
+from fractal_server.user_settings import SlurmSshUserSettings
+
+
+async def validate_user_settings(
+    *, user: UserOAuth, backend: str, db: AsyncSession
+):
+    """
+    FIXME docstring
+    """
+    # First: check that the foreign-key exists. TODO: remove this check,
+    # after this column is made required
+    if user.user_settings_id is None:
+        raise HTTPException(
+            status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
+            detail=f"Error: user '{user.email}' has no settings.",
+        )
+
+    user_settings = await db.get(UserSettings, user.user_settings_id)
+
+    if backend == "slurm_ssh":
+        try:
+            SlurmSshUserSettings(**user_settings.model_dump())
+        except ValidationError as e:
+            error_msg = (
+                "User settings are not valid for "
+                f"FRACTAL_RUNNER_BACKEND='{backend}'. "
+                f"Original error: {str(e)}"
+            )
+            logger.warning(error_msg)
+            raise HTTPException(
+                status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
+                detail=error_msg,
+            )

fractal_server/user_settings.py

@@ -0,0 +1,22 @@
+# TODO: move this file to the appropriate path
+from pydantic import BaseModel
+
+
+class SlurmSshUserSettings(BaseModel):
+    """
+    Subset of user settings which must be present for task collection and job
+    execution when using the Slurm-SSH runner.
+
+    Attributes:
+        ssh_host: SSH-reachable host where a SLURM client is available.
+        ssh_username: User on `ssh_host`.
+        ssh_private_key_path: Path of private SSH key for `ssh_username`.
+        ssh_tasks_dir: Task-venvs base folder on `ssh_host`.
+        ssh_jobs_dir: Jobs base folder on `ssh_host`.
+    """
+
+    ssh_host: str
+    ssh_username: str
+    ssh_private_key_path: str
+    ssh_tasks_dir: str
+    ssh_jobs_dir: str

tests/fixtures_server.py

@@ -232,7 +232,8 @@ async def MockCurrentUser(app, db):
     from fractal_server.app.routes.auth import current_active_verified_user
     from fractal_server.app.routes.auth import current_active_user
     from fractal_server.app.routes.auth import current_active_superuser
-    from fractal_server.app.routes.auth import UserOAuth
+    from fractal_server.app.models import UserOAuth
+    from fractal_server.app.models import UserSettings
 
     def _random_email():
         return f"{random.randint(0, 100000000)}@example.org"
@@ -243,8 +244,8 @@ class _MockCurrentUser:
         Context managed user override
         """
 
-        name: str = "User Name"
         user_kwargs: Optional[dict[str, Any]] = None
+        user_settings_dict: Optional[dict[str, Any]] = None
         email: Optional[str] = field(default_factory=_random_email)
         previous_dependencies: dict = field(default_factory=dict)
 
@@ -259,32 +260,40 @@ async def __aenter__(self):
                         f"User with id {self.user_kwargs['id']} doesn't exist"
                     )
                 self.user = db_user
+                # Removing objects from test db session, so that we can operate
+                # on them from other sessions
+                db.expunge(self.user)
             else:
                 # Create new user
-                defaults = dict(
+                user_attributes = dict(
                     email=self.email,
                     hashed_password="fake_hashed_password",
                     slurm_user="test01",
                 )
-                if self.user_kwargs:
-                    defaults.update(self.user_kwargs)
-                self.user = UserOAuth(name=self.name, **defaults)
+                if self.user_kwargs is not None:
+                    user_attributes.update(self.user_kwargs)
+                self.user = UserOAuth(**user_attributes)
+
+                # Create new user_settings object and associate it to user
+                user_settings_dict = self.user_settings_dict or {}
+                user_settings = UserSettings(**user_settings_dict)
+                self.user.settings = user_settings
 
                 try:
                     db.add(self.user)
                     await db.commit()
-                    await db.refresh(self.user)
                 except IntegrityError:
                     # Safety net, in case of non-unique email addresses
                     await db.rollback()
                     self.user.email = _random_email()
                     db.add(self.user)
                     await db.commit()
-                    await db.refresh(self.user)
+                await db.refresh(self.user)
 
-            # Removing object from test db session, so that we can operate
-            # on user from other sessions
-            db.expunge(self.user)
+                # Removing objects from test db session, so that we can operate
+                # on them from other sessions
+                db.expunge(user_settings)
+                db.expunge(self.user)
 
             # Find out which dependencies should be overridden, and store their
             # pre-override value

tests/no_version/test_unit_user_settings.py

@@ -1,7 +1,12 @@
+import pytest
 from devtools import debug
+from fastapi import HTTPException
 
 from fractal_server.app.models import UserOAuth
 from fractal_server.app.models import UserSettings
+from fractal_server.app.routes.aux.validate_user_settings import (
+    validate_user_settings,
+)
 
 
 async def test_unit_link_user_to_settings(db):
@@ -80,3 +85,64 @@ async def test_unit_link_user_to_settings(db):
     assert user_D.settings is not None
 
     # FIXME: Test delete cascade
+
+
+async def test_validate_user_settings(db):
+    common_attributes = dict(
+        hashed_password="xxx",
+        is_active=True,
+        is_superuser=False,
+        is_verified=True,
+    )
+
+    # Prepare users
+    user_without_settings = UserOAuth(email="a@a.a", **common_attributes)
+    db.add(user_without_settings)
+    await db.commit()
+    await db.refresh(user_without_settings)
+
+    invalid_settings = UserSettings()
+    user_with_invalid_settings = UserOAuth(
+        email="b@b.b",
+        **common_attributes,
+        settings=invalid_settings,
+    )
+    db.add(user_with_invalid_settings)
+    await db.commit()
+    await db.refresh(user_with_invalid_settings)
+
+    valid_settings = UserSettings(
+        ssh_host="x",
+        ssh_jobs_dir="/x",
+        ssh_private_key_path="/x",
+        ssh_tasks_dir="/x",
+        ssh_username="x",
+    )
+    user_with_valid_settings = UserOAuth(
+        email="c@c.c",
+        **common_attributes,
+        settings=valid_settings,
+    )
+    db.add(user_with_valid_settings)
+    await db.commit()
+    await db.refresh(user_with_valid_settings)
+
+    debug(user_without_settings)
+    debug(user_with_invalid_settings)
+    debug(user_with_valid_settings)
+
+    with pytest.raises(HTTPException, match="has no settings"):
+        await validate_user_settings(
+            user=user_without_settings, backend="slurm_ssh", db=db
+        )
+
+    with pytest.raises(
+        HTTPException, match="validation errors for SlurmSshUserSettings"
+    ):
+        await validate_user_settings(
+            user=user_with_invalid_settings, backend="slurm_ssh", db=db
+        )
+
+    await validate_user_settings(
+        user=user_with_valid_settings, backend="slurm_ssh", db=db
+    )

tests/v2/03_api/test_api_task_collection_ssh.py

@@ -7,6 +7,7 @@
 
 
 PREFIX = "api/v2/task"
+SLURM_USER = "test01"
 
 
 async def test_task_collection_ssh_from_pypi(
@@ -19,6 +20,8 @@ async def test_task_collection_ssh_from_pypi(
     tmp777_path: Path,
     fractal_ssh: FractalSSH,
     current_py_version: str,
+    slurmlogin_ip,
+    ssh_keys,
 ):
 
     # Define and create remote working directory
@@ -39,7 +42,18 @@ async def test_task_collection_ssh_from_pypi(
     }
     override_settings_factory(**settings_overrides)
 
-    async with MockCurrentUser(user_kwargs=dict(is_verified=True)):
+    user_settings_dict = dict(
+        ssh_host=slurmlogin_ip,
+        ssh_username=SLURM_USER,
+        ssh_private_key_path=ssh_keys["private"],
+        ssh_tasks_dir=(tmp777_path / "tasks").as_posix(),
+        ssh_jobs_dir=(tmp777_path / "artifacts").as_posix(),
+    )
+
+    async with MockCurrentUser(
+        user_kwargs=dict(is_verified=True),
+        user_settings_dict=user_settings_dict,
+    ):
 
         # CASE 1: successful collection
 

tests/v2/08_full_workflow/common_functions.py

@@ -497,6 +497,7 @@ async def workflow_with_non_python_task(
     tmp777_path: Path,
     additional_user_kwargs=None,
     this_should_fail: bool = False,
+    user_settings_dict: Optional[dict] = None,
 ) -> str:
     """
     Run a non-python-task Fractal job.
@@ -510,7 +511,9 @@ async def workflow_with_non_python_task(
         user_kwargs.update(additional_user_kwargs)
     debug(user_kwargs)
 
-    async with MockCurrentUser(user_kwargs=user_kwargs) as user:
+    async with MockCurrentUser(
+        user_kwargs=user_kwargs, user_settings_dict=user_settings_dict
+    ) as user:
         # Create project
         project = await project_factory_v2(user)
         project_id = project.id

tests/v2/08_full_workflow/test_full_workflow_slurm_ssh_v2.py

@@ -39,13 +39,22 @@ async def test_workflow_with_non_python_task_slurm_ssh(
         FRACTAL_SLURM_CONFIG_FILE=testdata_path / "slurm_config.json",
     )
 
+    user_settings_dict = dict(
+        ssh_host=slurmlogin_ip,
+        ssh_username=SLURM_USER,
+        ssh_private_key_path=ssh_keys["private"],
+        ssh_tasks_dir=(tmp777_path / "tasks").as_posix(),
+        ssh_jobs_dir=(tmp777_path / "artifacts").as_posix(),
+    )
+
     connection = get_ssh_connection()
     app.state.fractal_ssh = FractalSSH(connection=connection)
 
     monkeypatch.setattr("sys.stdin", io.StringIO(""))
 
     await workflow_with_non_python_task(
         MockCurrentUser=MockCurrentUser,
+        user_settings_dict=user_settings_dict,
         client=client,
         testdata_path=testdata_path,
         project_factory_v2=project_factory_v2,
@@ -91,6 +100,14 @@ async def test_workflow_with_non_python_task_slurm_ssh_fail(
         FRACTAL_SLURM_CONFIG_FILE=testdata_path / "slurm_config.json",
     )
 
+    user_settings_dict = dict(
+        ssh_host=slurmlogin_ip,
+        ssh_username=SLURM_USER,
+        ssh_private_key_path="/invalid/path",
+        ssh_tasks_dir=(tmp777_path / "tasks").as_posix(),
+        ssh_jobs_dir=(tmp777_path / "artifacts").as_posix(),
+    )
+
     connection = get_ssh_connection()
     app.state.fractal_ssh = FractalSSH(connection=connection)
 
@@ -99,6 +116,7 @@ async def test_workflow_with_non_python_task_slurm_ssh_fail(
     job_logs = await workflow_with_non_python_task(
         MockCurrentUser=MockCurrentUser,
         client=client,
+        user_settings_dict=user_settings_dict,
         testdata_path=testdata_path,
         project_factory_v2=project_factory_v2,
         dataset_factory_v2=dataset_factory_v2,