Add check to PATCH /auth/users/{id} when patching project_dirs

CHANGELOG.md

@@ -18,6 +18,7 @@ The main contents of this release are the introduction of the project sharing an
     * Reduce API logging level for some endpoints (\#3010).
     * Modify `GET /auth/current-user/allowed-viewer-paths/` logic, with `include_shared_projects` query parameter (\#3031).
     * Add validator for paths to forbid parent-directory references (\#3031).
+    * Add check to `PATCH /auth/users/{user_id}/` when patching `project_dirs` (\#3043).
     * Review job-submission endpoint (\#3041).
 * App:
     * Add `SlowResponseMiddleware` middleware (\#3035, \#3038).

fractal_server/app/routes/auth/_aux_auth.py

@@ -1,12 +1,20 @@
+from pathlib import Path
+
 from fastapi import HTTPException
 from fastapi import status
 from sqlalchemy.ext.asyncio import AsyncSession
+from sqlmodel import and_
 from sqlmodel import asc
+from sqlmodel import not_
+from sqlmodel import or_
 from sqlmodel import select
 
 from fractal_server.app.models.linkusergroup import LinkUserGroup
+from fractal_server.app.models.linkuserproject import LinkUserProjectV2
 from fractal_server.app.models.security import UserGroup
 from fractal_server.app.models.security import UserOAuth
+from fractal_server.app.models.v2.dataset import DatasetV2
+from fractal_server.app.models.v2.project import ProjectV2
 from fractal_server.app.schemas.user import UserRead
 from fractal_server.app.schemas.user_group import UserGroupRead
 from fractal_server.config import get_settings
@@ -178,3 +186,82 @@ async def _verify_user_belongs_to_group(
                     f"to UserGroup {user_group_id}"
                 ),
             )
+
+
+async def _check_project_dirs_update(
+    *,
+    old_project_dirs: list[str],
+    new_project_dirs: list[str],
+    user_id: int,
+    db: AsyncSession,
+) -> None:
+    """
+    Raises 422 if by replacing user's `project_dirs` with new ones we are
+    removing the access to a `zarr_dir` used by some dataset.
+    """
+    # Create a list of all the old project dirs that will lose privileges.
+    # E.g.:
+    #   old_project_dirs = ["/a", "/b", "/c/d", "/e/f"]
+    #   new_project_dirs = ["/a", "/c", "/e/f/g1", "/e/f/g2"]
+    #   removed_project_dirs == ["/b", "/e/f"]
+    removed_project_dirs = [
+        old_project_dir
+        for old_project_dir in old_project_dirs
+        if not any(
+            Path(old_project_dir).is_relative_to(new_project_dir)
+            for new_project_dir in new_project_dirs
+        )
+    ]
+    if removed_project_dirs:
+        # Query all the `zarr_dir`s linked to the user such that `zarr_dir`
+        # starts with one of the project dirs in `removed_project_dirs`.
+        res = await db.execute(
+            select(DatasetV2.zarr_dir)
+            .join(ProjectV2, ProjectV2.id == DatasetV2.project_id)
+            .join(
+                LinkUserProjectV2,
+                LinkUserProjectV2.project_id == ProjectV2.id,
+            )
+            .where(LinkUserProjectV2.user_id == user_id)
+            .where(
+                or_(
+                    *[
+                        DatasetV2.zarr_dir.startswith(old_project_dir)
+                        for old_project_dir in removed_project_dirs
+                    ]
+                )
+            )
+            .where(
+                and_(
+                    *[
+                        not_(DatasetV2.zarr_dir.startswith(new_project_dir))
+                        for new_project_dir in new_project_dirs
+                    ]
+                )
+            )
+        )
+        # Raise 422 if one of the query results is relative to a path in
+        # `removed_project_dirs`, but its not relative to any path in
+        # `new_project_dirs`.
+        if any(
+            (
+                any(
+                    Path(zarr_dir).is_relative_to(old_project_dir)
+                    for old_project_dir in removed_project_dirs
+                )
+                and not any(
+                    Path(zarr_dir).is_relative_to(new_project_dir)
+                    for new_project_dir in new_project_dirs
+                )
+            )
+            for zarr_dir in res.scalars().all()
+        ):
+            raise HTTPException(
+                status_code=status.HTTP_422_UNPROCESSABLE_CONTENT,
+                detail=(
+                    "You tried updating the user project_dirs, removing "
+                    f"{removed_project_dirs}. This operation is not possible, "
+                    "because it would make the user loose access to some of "
+                    "their dataset zarr directories."
+                ),
+            )

fractal_server/app/routes/auth/users.py

@@ -28,6 +28,7 @@
 from fractal_server.syringe import Inject
 
 from . import current_superuser_act
+from ._aux_auth import _check_project_dirs_update
 from ._aux_auth import _get_default_usergroup_id_or_none
 from ._aux_auth import _get_single_user_with_groups
 
@@ -74,6 +75,14 @@ async def patch_user(
                 detail=f"Profile {user_update.profile_id} not found.",
             )
 
+    if user_update.project_dirs is not None:
+        await _check_project_dirs_update(
+            old_project_dirs=user_to_patch.project_dirs,
+            new_project_dirs=user_update.project_dirs,
+            user_id=user_id,
+            db=db,
+        )
+
     # Modify user attributes
     try:
         user = await user_manager.update(

tests/no_version/test_api_users.py

@@ -1,6 +1,11 @@
 from devtools import debug
 
+from fractal_server.app.models.linkuserproject import LinkUserProjectV2
 from fractal_server.app.models.security import OAuthAccount
+from fractal_server.app.models.security import UserOAuth
+from fractal_server.app.models.v2.dataset import DatasetV2
+from fractal_server.app.models.v2.project import ProjectV2
+from fractal_server.app.schemas.v2.sharing import ProjectPermissions
 from tests.fixtures_server import PROJECT_DIR_PLACEHOLDER
 
 PREFIX = "/auth"
@@ -292,6 +297,72 @@ async def test_edit_users_as_superuser(
     assert len(users) == 0
 
 
+async def test_edit_user_project_dirs(
+    registered_superuser_client, local_resource_profile_db, db
+):
+    resource, profile = local_resource_profile_db
+
+    user = UserOAuth(
+        email="user@example.org",
+        hashed_password="12345",
+        project_dirs=["/a", "/b", "/b/c"],
+    )
+    db.add(user)
+    await db.flush()
+    project = ProjectV2(name="Project", resource_id=resource.id)
+    db.add(project)
+    await db.flush()
+    db.add(
+        LinkUserProjectV2(
+            project_id=project.id,
+            user_id=user.id,
+            is_owner=True,
+            is_verified=True,
+            permissions=ProjectPermissions.EXECUTE,
+        )
+    )
+    await db.flush()
+    dataset = DatasetV2(
+        name="Dataset",
+        project_id=project.id,
+        zarr_dir="/b/c/data/zarr",
+        images=[],
+    )
+    db.add(dataset)
+    await db.commit()
+    await db.refresh(user)
+    await db.refresh(dataset)
+
+    # Update 1
+
+    update = dict(project_dirs=["/a", "/b/c/data/"])
+    res = await registered_superuser_client.patch(
+        f"{PREFIX}/users/{user.id}/",
+        json=update,
+    )
+    assert res.status_code == 200
+    assert res.json()["project_dirs"] == update["project_dirs"]
+
+    # Update 2
+
+    update = dict(project_dirs=["/a"])
+    res = await registered_superuser_client.patch(
+        f"{PREFIX}/users/{user.id}/",
+        json=update,
+    )
+    assert res.status_code == 422
+    assert "loose access" in res.json()["detail"]
+
+    await db.delete(dataset)
+    await db.commit()
+
+    res = await registered_superuser_client.patch(
+        f"{PREFIX}/users/{user.id}/",
+        json=update,
+    )
+    assert res.status_code == 200
+
+
 async def test_add_superuser(registered_superuser_client):
     # Create non-superuser user
     res = await registered_superuser_client.post(

tests/no_version/test_unit_auth.py

@@ -3,7 +3,11 @@
 from fastapi import HTTPException
 from sqlmodel import select
 
+from fractal_server.app.models import DatasetV2
+from fractal_server.app.models import LinkUserProjectV2
+from fractal_server.app.models import ProjectV2
 from fractal_server.app.models import UserOAuth
+from fractal_server.app.routes.auth._aux_auth import _check_project_dirs_update
 from fractal_server.app.routes.auth._aux_auth import (
     _get_single_user_with_groups,
 )
@@ -14,6 +18,7 @@
 from fractal_server.app.routes.auth._aux_auth import (
     _verify_user_belongs_to_group,
 )
+from fractal_server.app.schemas.v2 import ProjectPermissions
 from fractal_server.app.security import _create_first_user
 
 
@@ -55,3 +60,149 @@ async def test_verify_user_belongs_to_group(db):
     with pytest.raises(HTTPException) as exc_info:
         await _verify_user_belongs_to_group(user_id=1, user_group_id=42, db=db)
     debug(exc_info.value)
+
+
+async def test_check_project_dirs_update(local_resource_profile_db, db):
+    # Setup
+
+    resource, _ = local_resource_profile_db
+    # Add User
+    user = UserOAuth(
+        email="user@example.org",
+        hashed_password="12345",
+        project_dirs=[
+            "/example",
+            "/foo/bar",  # dataset1
+            "/test",
+            "/test/data",  # dataset2
+            "/test-1",  # dataset3
+        ],
+    )
+    db.add(user)
+    await db.commit()
+    await db.refresh(user)
+    # Add Project
+    project = ProjectV2(name="Project", resource_id=resource.id)
+    db.add(project)
+    await db.commit()
+    await db.refresh(project)
+    db.add(
+        LinkUserProjectV2(
+            project_id=project.id,
+            user_id=user.id,
+            is_owner=True,
+            is_verified=True,
+            permissions=ProjectPermissions.EXECUTE,
+        )
+    )
+    await db.commit()
+    # Add Datasets
+    dataset1 = DatasetV2(
+        name="Dataset 3",
+        project_id=project.id,
+        zarr_dir="/foo/bar/dataset/zarr",
+    )
+    dataset2 = DatasetV2(
+        name="Dataset 1",
+        project_id=project.id,
+        zarr_dir="/test/data/dataset/zarr",
+    )
+    dataset3 = DatasetV2(
+        name="Dataset 2",
+        project_id=project.id,
+        zarr_dir="/test-1/dataset/zarr",
+    )
+    db.add_all([dataset1, dataset2, dataset3])
+    await db.commit()
+    await db.refresh(dataset1)
+    await db.refresh(dataset2)
+    await db.refresh(dataset3)
+
+    kwargs = dict(
+        old_project_dirs=user.project_dirs,
+        user_id=user.id,
+        db=db,
+    )
+
+    # Test
+
+    # Removing "/example" is OK
+    await _check_project_dirs_update(
+        new_project_dirs=[
+            "/foo/bar",  # dataset1
+            "/test",
+            "/test/data",  # dataset2
+            "/test-1",  # dataset3
+        ],
+        **kwargs,
+    )
+    # Removing "/test" is OK
+    await _check_project_dirs_update(
+        new_project_dirs=[
+            "/example",
+            "/foo/bar",  # dataset1
+            "/test/data",  # dataset2
+            "/test-1",  # dataset3
+        ],
+        **kwargs,
+    )
+    # Removing both "/example" and "/test" is OK
+    await _check_project_dirs_update(
+        new_project_dirs=[
+            "/foo/bar",  # dataset1
+            "/test/data",  # dataset2
+            "/test-1",  # dataset3
+        ],
+        **kwargs,
+    )
+    # Removing "/foo/bar" can be done after removing dataset1
+    with pytest.raises(HTTPException):
+        await _check_project_dirs_update(
+            new_project_dirs=[
+                "/example",
+                "/test",
+                "/test/data",  # dataset2
+                "/test-1",  # dataset3
+            ],
+            **kwargs,
+        )
+    await db.delete(dataset1)
+    await _check_project_dirs_update(
+        new_project_dirs=[
+            "/example",
+            "/test",
+            "/test/data",  # dataset2
+            "/test-1",  # dataset3
+        ],
+        **kwargs,
+    )
+    # Changing "/test/data" into something more specific is OK
+    await _check_project_dirs_update(
+        new_project_dirs=[
+            "/example",
+            "/foo/bar",
+            "/test",
+            "/test/data/dataset/zarr/",  # dataset2
+            "/test-1",  # dataset3
+        ],
+        **kwargs,
+    )
+    # Removing "/test/data" is OK, as long as we have "/test"
+    await _check_project_dirs_update(
+        new_project_dirs=[
+            "/example",
+            "/foo/bar",
+            "/test",  # dataset2
+            "/test-1",  # dataset3
+        ],
+        **kwargs,
+    )
+    with pytest.raises(HTTPException):
+        await _check_project_dirs_update(
+            new_project_dirs=[
+                "/example",
+                "/foo/bar",
+                "/test-1",  # dataset3
+            ],
+            **kwargs,
+        )