diff --git a/CHANGELOG.md b/CHANGELOG.md index a116cae0bd..f8e4e6fc9f 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/fractal_server/app/routes/auth/_aux_auth.py b/fractal_server/app/routes/auth/_aux_auth.py index 3d03a6420d..1fc1702073 100644 --- a/fractal_server/app/routes/auth/_aux_auth.py +++ b/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." + ), + ) diff --git a/fractal_server/app/routes/auth/users.py b/fractal_server/app/routes/auth/users.py index b2c6d2b8a1..dfcae76927 100644 --- a/fractal_server/app/routes/auth/users.py +++ b/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( diff --git a/tests/no_version/test_api_users.py b/tests/no_version/test_api_users.py index cd12643d80..88a7dc4bf2 100644 --- a/tests/no_version/test_api_users.py +++ b/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( diff --git a/tests/no_version/test_unit_auth.py b/tests/no_version/test_unit_auth.py index 1800b584bd..161b52f39d 100644 --- a/tests/no_version/test_unit_auth.py +++ b/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, + )