Merge branch 'master' into enh/servicelib

pcrespov · web-flow · commit 35f22ea2033c · 2023-12-12T12:20:32.000+01:00
diff --git a/services/storage/src/simcore_service_storage/simcore_s3_dsm.py b/services/storage/src/simcore_service_storage/simcore_s3_dsm.py
@@ -223,7 +223,7 @@ async def list_files(  # noqa C901
 
     async def get_file(self, user_id: UserID, file_id: StorageFileID) -> FileMetaData:
         async with self.engine.acquire() as conn, conn.begin():
-            can: AccessRights | None = await get_file_access_rights(
+            can: AccessRights = await get_file_access_rights(
                 conn, int(user_id), file_id
             )
             if can.read:
@@ -249,9 +249,7 @@ async def create_file_upload_links(
         is_directory: bool,
     ) -> UploadLinks:
         async with self.engine.acquire() as conn, conn.begin() as transaction:
-            can: AccessRights | None = await get_file_access_rights(
-                conn, user_id, file_id
-            )
+            can: AccessRights = await get_file_access_rights(conn, user_id, file_id)
             if not can.write:
                 raise FileAccessRightError(access_right="write", file_id=file_id)
 
@@ -263,7 +261,14 @@ async def create_file_upload_links(
             )
 
             # ensure file is deleted first in case it already exists
-            await self.delete_file(user_id=user_id, file_id=file_id)
+            await self.delete_file(
+                user_id=user_id,
+                file_id=file_id,
+                # NOTE: bypassing check since the project access rights don't play well
+                # with collaborators
+                # SEE https://github.com/ITISFoundation/osparc-simcore/issues/5159
+                enforce_access_rights=False,
+            )
 
             # initiate the file meta data table
             fmd = await self._create_fmd_for_upload(
@@ -331,7 +336,7 @@ async def abort_file_upload(
         file_id: StorageFileID,
     ) -> None:
         async with self.engine.acquire() as conn, conn.begin():
-            can: AccessRights | None = await get_file_access_rights(
+            can: AccessRights = await get_file_access_rights(
                 conn, int(user_id), file_id
             )
             if not can.delete or not can.write:
@@ -367,7 +372,7 @@ async def complete_file_upload(
         uploaded_parts: list[UploadedPart],
     ) -> FileMetaData:
         async with self.engine.acquire() as conn:
-            can: AccessRights | None = await get_file_access_rights(
+            can: AccessRights = await get_file_access_rights(
                 conn, int(user_id), file_id
             )
             if not can.write:
@@ -427,9 +432,7 @@ async def create_file_download_link(
     async def __ensure_read_access_rights(
         conn: SAConnection, user_id: UserID, storage_file_id: StorageFileID
     ) -> None:
-        can: AccessRights | None = await get_file_access_rights(
-            conn, user_id, storage_file_id
-        )
+        can: AccessRights = await get_file_access_rights(conn, user_id, storage_file_id)
         if not can.read:
             # NOTE: this is tricky. A user with read access can download and data!
             # If write permission would be required, then shared projects as views cannot
@@ -486,13 +489,29 @@ async def _get_link_for_directory_fmd(
             raise S3KeyNotFoundError(key=file_id, bucket=self.simcore_bucket_name)
         return await self.__get_link(parse_obj_as(SimcoreS3FileID, file_id), link_type)
 
-    async def delete_file(self, user_id: UserID, file_id: StorageFileID):
+    async def delete_file(
+        self,
+        user_id: UserID,
+        file_id: StorageFileID,
+        *,
+        enforce_access_rights: bool = True,
+    ):
+        #   _   _  ___ _____ _____
+        #  | \ | |/ _ \_   _| ____|
+        #  |  \| | | | || | |  _|
+        #  | |\  | |_| || | | |___
+        #  |_| \_|\___/ |_| |_____|
+        # NOTE: (a very big one)
+        # `enforce_access_rights` is set to False because permissions are based on "project access rights"
+        # they should be based on data access rights (which is currently not present)
+        # Only use this in those circumstances where a collaborator requires to delete a file (the current
+        # permissions model will not allow him to do so, even though this is a legitimate action)
+        # SEE https://github.com/ITISFoundation/osparc-simcore/issues/5159
         async with self.engine.acquire() as conn, conn.begin():
-            can: AccessRights | None = await get_file_access_rights(
-                conn, user_id, file_id
-            )
-            if not can.delete:
-                raise FileAccessRightError(access_right="delete", file_id=file_id)
+            if enforce_access_rights:
+                can: AccessRights = await get_file_access_rights(conn, user_id, file_id)
+                if not can.delete:
+                    raise FileAccessRightError(access_right="delete", file_id=file_id)
 
             with suppress(FileMetaDataNotFoundError):
                 file: FileMetaDataAtDB = await db_file_meta_data.get(
diff --git a/services/storage/tests/fixtures/data_models.py b/services/storage/tests/fixtures/data_models.py
@@ -3,10 +3,12 @@
 # pylint:disable=redefined-outer-name
 
 
+from contextlib import asynccontextmanager
 from typing import Any, AsyncIterator, Awaitable, Callable
 
 import pytest
 import sqlalchemy as sa
+from aiopg.sa.connection import SAConnection
 from aiopg.sa.engine import Engine
 from faker import Faker
 from models_library.projects import ProjectID
@@ -16,16 +18,16 @@
 from simcore_postgres_database.storage_models import projects, users
 
 
-@pytest.fixture
-async def user_id(aiopg_engine: Engine) -> AsyncIterator[UserID]:
+@asynccontextmanager
+async def user_context(aiopg_engine: Engine, *, name: str) -> UserID:
     # inject a random user in db
 
     # NOTE: Ideally this (and next fixture) should be done via webserver API but at this point
     # in time, the webserver service would bring more dependencies to other services
     # which would turn this test too complex.
 
     # pylint: disable=no-value-for-parameter
-    stmt = users.insert().values(**random_user(name="test")).returning(users.c.id)
+    stmt = users.insert().values(**random_user(name=name)).returning(users.c.id)
     print(str(stmt))
     async with aiopg_engine.acquire() as conn:
         result = await conn.execute(stmt)
@@ -38,6 +40,12 @@ async def user_id(aiopg_engine: Engine) -> AsyncIterator[UserID]:
         await conn.execute(users.delete().where(users.c.id == row.id))
 
 
+@pytest.fixture
+async def user_id(aiopg_engine: Engine) -> AsyncIterator[UserID]:
+    async with user_context(aiopg_engine, name="test") as new_user_id:
+        yield new_user_id
+
+
 @pytest.fixture
 async def create_project(
     user_id: UserID, aiopg_engine: Engine
@@ -74,6 +82,59 @@ async def project_id(
     return ProjectID(project["uuid"])
 
 
+@pytest.fixture
+async def collaborator_id(aiopg_engine: Engine) -> AsyncIterator[UserID]:
+    async with user_context(aiopg_engine, name="collaborator") as new_user_id:
+        yield new_user_id
+
+
+@pytest.fixture
+def share_with_collaborator(
+    aiopg_engine: Engine,
+    collaborator_id: UserID,
+    user_id: UserID,
+    project_id: ProjectID,
+) -> Callable[[], Awaitable[None]]:
+    async def _get_user_group(conn: SAConnection, query_user: int) -> int:
+        result = await conn.execute(
+            sa.select(users.c.primary_gid).where(users.c.id == query_user)
+        )
+        row = await result.fetchone()
+        assert row
+        primary_gid: int = row[users.c.primary_gid]
+        return primary_gid
+
+    async def _() -> None:
+        async with aiopg_engine.acquire() as conn:
+            result = await conn.execute(
+                sa.select(projects.c.access_rights).where(
+                    projects.c.uuid == f"{project_id}"
+                )
+            )
+            row = await result.fetchone()
+            assert row
+            access_rights: dict[str, Any] = row[projects.c.access_rights]
+
+            access_rights[await _get_user_group(conn, user_id)] = {
+                "read": True,
+                "write": True,
+                "delete": True,
+            }
+            access_rights[await _get_user_group(conn, collaborator_id)] = {
+                "read": True,
+                "write": True,
+                "delete": False,
+            }
+
+            await conn.execute(
+                projects.update()
+                .where(projects.c.uuid == f"{project_id}")
+                .values(access_rights=access_rights)
+            )
+
+    return _
+
+
 @pytest.fixture
 async def create_project_node(
     user_id: UserID, aiopg_engine: Engine, faker: Faker
diff --git a/services/storage/tests/unit/test_dsm_dsmcleaner.py b/services/storage/tests/unit/test_dsm_dsmcleaner.py
@@ -23,7 +23,10 @@
 from pytest_simcore.helpers.utils_parametrizations import byte_size_ids
 from simcore_postgres_database.storage_models import file_meta_data
 from simcore_service_storage import db_file_meta_data
-from simcore_service_storage.exceptions import FileMetaDataNotFoundError
+from simcore_service_storage.exceptions import (
+    FileAccessRightError,
+    FileMetaDataNotFoundError,
+)
 from simcore_service_storage.models import (
     FileMetaData,
     MultiPartUploadLinks,
@@ -86,6 +89,72 @@ async def test_clean_expired_uploads_aborts_dangling_multipart_uploads(
     assert not await storage_s3_client.list_ongoing_multipart_uploads(storage_s3_bucket)
 
 
+@pytest.mark.parametrize(
+    "file_size",
+    [ByteSize(0), parse_obj_as(ByteSize, "10Mib"), parse_obj_as(ByteSize, "100Mib")],
+    ids=byte_size_ids,
+)
+@pytest.mark.parametrize(
+    "link_type, is_directory",
+    [
+        # NOTE: directories are handled only as LinkType.S3
+        (LinkType.S3, True),
+        (LinkType.S3, False),
+        (LinkType.PRESIGNED, False),
+    ],
+)
+@pytest.mark.parametrize("checksum", [None, _faker.sha256()])
+async def test_regression_collaborator_creates_file_upload_links(
+    disabled_dsm_cleaner_task,
+    aiopg_engine: Engine,
+    simcore_s3_dsm: SimcoreS3DataManager,
+    simcore_file_id: SimcoreS3FileID,
+    simcore_directory_id: SimcoreS3FileID,
+    user_id: UserID,
+    link_type: LinkType,
+    file_size: ByteSize,
+    is_directory: bool,
+    storage_s3_client: StorageS3Client,
+    storage_s3_bucket: S3BucketName,
+    checksum: SHA256Str | None,
+    collaborator_id: UserID,
+    share_with_collaborator: Callable[[], Awaitable[None]],
+):
+    file_or_directory_id = simcore_directory_id if is_directory else simcore_file_id
+
+    await simcore_s3_dsm.create_file_upload_links(
+        user_id,
+        file_or_directory_id,
+        link_type,
+        file_size,
+        sha256_checksum=checksum,
+        is_directory=is_directory,
+    )
+
+    # collaborators don't have access
+    with pytest.raises(FileAccessRightError):
+        await simcore_s3_dsm.create_file_upload_links(
+            collaborator_id,
+            file_or_directory_id,
+            link_type,
+            file_size,
+            sha256_checksum=checksum,
+            is_directory=is_directory,
+        )
+
+    await share_with_collaborator()
+
+    # collaborator have access
+    await simcore_s3_dsm.create_file_upload_links(
+        collaborator_id,
+        file_or_directory_id,
+        link_type,
+        file_size,
+        sha256_checksum=checksum,
+        is_directory=is_directory,
+    )
+
+
 @pytest.mark.parametrize(
     "file_size",
     [ByteSize(0), parse_obj_as(ByteSize, "10Mib"), parse_obj_as(ByteSize, "100Mib")],
diff --git a/services/web/server/tests/integration/02/scicrunch/test_scicrunch__resolver.py b/services/web/server/tests/integration/02/scicrunch/test_scicrunch__resolver.py
@@ -16,6 +16,9 @@
 from simcore_service_webserver.scicrunch.settings import SciCrunchSettings
 
 
+@pytest.mark.skip(
+    "requires a fix see case https://github.com/ITISFoundation/osparc-simcore/issues/5160"
+)
 @pytest.mark.parametrize(
     "name,rrid",
     TOOL_CITATIONS + ANTIBODY_CITATIONS + PLAMID_CITATIONS + ORGANISM_CITATIONS,
