Merge pull request #1802 from fractal-analytics-platform/protect-db-g…

…et-and-small-fixes

Alwasy call `verify_user_has_settings` before `db.get(UserSettings, user.user_settings_id`)

fractal_server/app/models/user_settings.py

@@ -11,27 +11,28 @@ class UserSettings(SQLModel, table=True):
     Comprehensive list of user settings.
 
     Attributes:
+        id: ID of database object
+        slurm_accounts:
+            List of SLURM accounts, to be used upon Fractal job submission.
         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`.
+        slurm_user: Local user, to be impersonated via `sudo -u`
+        cache_dir: Folder where `slurm_user` can write.
     """
 
     __tablename__ = "user_settings"
 
     id: Optional[int] = Field(default=None, primary_key=True)
-
-    # SSH-SLURM
+    slurm_accounts: list[str] = Field(
+        sa_column=Column(JSON, server_default="[]", nullable=False)
+    )
     ssh_host: Optional[str] = None
     ssh_username: Optional[str] = None
     ssh_private_key_path: Optional[str] = None
     ssh_tasks_dir: Optional[str] = None
     ssh_jobs_dir: Optional[str] = None
-
-    # SUDO-SLURM
     slurm_user: Optional[str] = None
-    slurm_accounts: list[str] = Field(
-        sa_column=Column(JSON, server_default="[]", nullable=False)
-    )
     cache_dir: Optional[str] = None

fractal_server/app/routes/auth/current_user.py

@@ -11,6 +11,7 @@
 from ...schemas.user import UserRead
 from ...schemas.user import UserUpdate
 from ...schemas.user import UserUpdateStrict
+from ..aux.validate_user_settings import verify_user_has_settings
 from ._aux_auth import _get_single_user_with_group_names
 from fractal_server.app.models import UserOAuth
 from fractal_server.app.models import UserSettings
@@ -74,6 +75,8 @@ async def get_current_user_settings(
     current_user: UserOAuth = Depends(current_active_user),
     db: AsyncSession = Depends(get_async_db),
 ) -> UserSettingsReadStrict:
+
+    verify_user_has_settings(current_user)
     user_settings = await db.get(UserSettings, current_user.user_settings_id)
     return user_settings
 
@@ -86,6 +89,8 @@ async def patch_current_user_settings(
     current_user: UserOAuth = Depends(current_active_user),
     db: AsyncSession = Depends(get_async_db),
 ) -> UserSettingsReadStrict:
+
+    verify_user_has_settings(current_user)
     current_user_settings = await db.get(
         UserSettings, current_user.user_settings_id
     )

fractal_server/app/routes/auth/users.py

@@ -19,6 +19,7 @@
 from ...schemas.user import UserRead
 from ...schemas.user import UserUpdate
 from ...schemas.user import UserUpdateWithNewGroupIds
+from ..aux.validate_user_settings import verify_user_has_settings
 from ._aux_auth import _get_single_user_with_group_ids
 from fractal_server.app.models import LinkUserGroup
 from fractal_server.app.models import UserGroup
@@ -211,6 +212,7 @@ async def get_user_settings(
 ) -> UserSettingsRead:
 
     user = await _user_or_404(user_id=user_id, db=db)
+    verify_user_has_settings(user)
     user_settings = await db.get(UserSettings, user.user_settings_id)
     return user_settings
 
@@ -225,6 +227,7 @@ async def patch_user_settings(
     db: AsyncSession = Depends(get_async_db),
 ) -> UserSettingsRead:
     user = await _user_or_404(user_id=user_id, db=db)
+    verify_user_has_settings(user)
     user_settings = await db.get(UserSettings, user.user_settings_id)
 
     for k, v in settings_update.dict(exclude_unset=True).items():

fractal_server/app/routes/aux/validate_user_settings.py

@@ -13,6 +13,23 @@
 logger = set_logger(__name__)
 
 
+def verify_user_has_settings(user: UserOAuth) -> None:
+    """
+    Check that the `user.user_settings_id` foreign-key is set.
+
+    NOTE: This check will become useless when we make the foreign-key column
+    required, but for the moment (as of v2.6.0) we have to keep it in place.
+
+    Arguments:
+        user: The user to be checked.
+    """
+    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.",
+        )
+
+
 async def validate_user_settings(
     *, user: UserOAuth, backend: str, db: AsyncSession
 ) -> UserSettings:
@@ -28,14 +45,7 @@ async def validate_user_settings(
         `UserSetting` object associated to `user`, if valid.
     """
 
-    # Check that the foreign-key exists. NOTE: This check will become
-    # useless when we make the foreign-key column required, but for the
-    # moment (as of v2.6.0) we have to keep it in place.
-    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.",
-        )
+    verify_user_has_settings(user)
 
     user_settings = await db.get(UserSettings, user.user_settings_id)
 

fractal_server/app/user_settings.py

@@ -13,6 +13,8 @@ class SlurmSshUserSettings(BaseModel):
         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`.
+        slurm_accounts:
+            List of SLURM accounts, to be used upon Fractal job submission.
     """
 
     ssh_host: str
@@ -30,7 +32,9 @@ class SlurmSudoUserSettings(BaseModel):
 
     Attributes:
         slurm_user: User to be impersonated via `sudo -u`.
-        cache_dir:
+        cache_dir: Folder where `slurm_user` can write.
+        slurm_accounts:
+            List of SLURM accounts, to be used upon Fractal job submission.
     """
 
     slurm_user: str

tests/no_version/test_unit_user_settings.py

@@ -84,8 +84,6 @@ async def test_unit_link_user_to_settings(db):
     assert user_D.user_settings_id is not None
     assert user_D.settings is not None
 
-    # FIXME: Test delete cascade
-
 
 async def test_validate_user_settings(db):
     common_attributes = dict(