From b889dcebf5ad4eda49fea347b0a327c2fa9ec1d2 Mon Sep 17 00:00:00 2001 From: Tommaso Comparin <3862206+tcompa@users.noreply.github.com> Date: Tue, 24 Sep 2024 11:39:24 +0200 Subject: [PATCH 1/4] Alwasy call `verify_user_has_settings` before `db.get(UserSettings, user.user_settings_id`) cc @mfranzon --- .../app/routes/auth/current_user.py | 5 ++++ fractal_server/app/routes/auth/users.py | 3 +++ .../app/routes/aux/validate_user_settings.py | 26 +++++++++++++------ 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/fractal_server/app/routes/auth/current_user.py b/fractal_server/app/routes/auth/current_user.py index 5c5f2dbba..314392259 100644 --- a/fractal_server/app/routes/auth/current_user.py +++ b/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 ) diff --git a/fractal_server/app/routes/auth/users.py b/fractal_server/app/routes/auth/users.py index a2e4d287c..d32adade3 100644 --- a/fractal_server/app/routes/auth/users.py +++ b/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(): diff --git a/fractal_server/app/routes/aux/validate_user_settings.py b/fractal_server/app/routes/aux/validate_user_settings.py index ac2c9badf..333b3cc17 100644 --- a/fractal_server/app/routes/aux/validate_user_settings.py +++ b/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) From c6713dc12af921bfccd87a0543ccf02ff878ce52 Mon Sep 17 00:00:00 2001 From: Tommaso Comparin <3862206+tcompa@users.noreply.github.com> Date: Tue, 24 Sep 2024 11:42:37 +0200 Subject: [PATCH 2/4] Improve docstring [skip ci] --- fractal_server/app/models/user_settings.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/fractal_server/app/models/user_settings.py b/fractal_server/app/models/user_settings.py index 2b566026c..acff62a8c 100644 --- a/fractal_server/app/models/user_settings.py +++ b/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 From 6abad91835d7b0045d0f1b849cd0b3d3869e40f1 Mon Sep 17 00:00:00 2001 From: Tommaso Comparin <3862206+tcompa@users.noreply.github.com> Date: Tue, 24 Sep 2024 11:43:33 +0200 Subject: [PATCH 3/4] Improve docstrings [skip ci] --- fractal_server/app/user_settings.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fractal_server/app/user_settings.py b/fractal_server/app/user_settings.py index 786dce7af..22b8e1f49 100644 --- a/fractal_server/app/user_settings.py +++ b/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 From 5deec354e26dbec90507370313c8fbec9e8a539d Mon Sep 17 00:00:00 2001 From: Tommaso Comparin <3862206+tcompa@users.noreply.github.com> Date: Tue, 24 Sep 2024 11:43:58 +0200 Subject: [PATCH 4/4] Remove FIXME [skip ci] --- tests/no_version/test_unit_user_settings.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/no_version/test_unit_user_settings.py b/tests/no_version/test_unit_user_settings.py index a77707869..67dc0a91d 100644 --- a/tests/no_version/test_unit_user_settings.py +++ b/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(