From 243964409eae73ebd511b8391ec0eaad3e70346c Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 6 Aug 2024 16:06:25 +0200 Subject: [PATCH 01/35] refactor to take into considerationg groups --- .../utils_folders.py | 83 +++++++++++++++---- 1 file changed, 65 insertions(+), 18 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 7d1db1fc54f..1d3bdf6b813 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -400,6 +400,46 @@ def _get_permissions_where_clause() -> ColumnElement | bool: async def _check_folder_and_access( + connection: SAConnection, + product_name: _ProductName, + folder_id: _FolderID, + gids: set[_GroupID], + *, + permissions: _FolderPermissions, + enforece_all_permissions: bool, +) -> _ResolvedAccessRights: + """ + Raises: + FolderNotFoundError + FolderNotSharedWithGidError + InsufficientPermissionsError + """ + resolved_access_rights: _ResolvedAccessRights | None = None + last_exception: Exception | None = None + for gid in gids: + try: + resolved_access_rights = await _check_folder_access( + connection, + product_name, + folder_id, + gid, + permissions=permissions, + enforece_all_permissions=enforece_all_permissions, + ) + except FolderAccessError as e: # noqa: PERF203 + last_exception = e + + if resolved_access_rights is None: + if last_exception: + raise last_exception + + msg = "unexpected error" + raise RuntimeError(msg) + + return resolved_access_rights + + +async def _check_folder_access( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, @@ -460,8 +500,7 @@ async def folder_create( connection: SAConnection, product_name: _ProductName, name: str, - gid: _GroupID, - *, + gid: _GroupID, # shared_via_gid TO WORK description: str = "", parent: _FolderID | None = None, required_permissions: _FolderPermissions = _requires( # noqa: B008 @@ -503,10 +542,14 @@ async def folder_create( connection, product_name, folder_id=parent, - gid=gid, + gids={gid}, permissions=required_permissions, enforece_all_permissions=False, ) + # TODO: add test to emulate Shared with Z43 and Osparc and not with MD + # -> MD_gid tries to create a folder inside this one -> should fail? + # TODO: use a set to check permissions on the parent + # "random" pick user between all grops it has access to # folder entry can now be inserted try: @@ -543,7 +586,7 @@ async def folder_share_or_update_permissions( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, - sharing_gid: _GroupID, + sharing_gid: _GroupID, # set[_GroupID] -> set of all group ids no need to pass in the sahred_via_gid *, recipient_gid: _GroupID, recipient_role: FolderAccessRole, @@ -563,7 +606,7 @@ async def folder_share_or_update_permissions( connection, product_name, folder_id=folder_id, - gid=sharing_gid, + gids={sharing_gid}, permissions=required_permissions, enforece_all_permissions=False, ) @@ -594,7 +637,7 @@ async def folder_update( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, - gid: _GroupID, + gid: _GroupID, # set[_GroupID] -> set of all group ids no need to pass in the sahred_via_gid *, name: str | None = None, description: str | None = None, @@ -613,7 +656,7 @@ async def folder_update( connection, product_name, folder_id=folder_id, - gid=gid, + gids={gid}, permissions=required_permissions, enforece_all_permissions=False, ) @@ -638,7 +681,7 @@ async def folder_delete( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, - gid: _GroupID, + gid: _GroupID, # set[_GroupID] -> set of all group ids no need to pass in the sahred_via_gid *, required_permissions: _FolderPermissions = _requires( # noqa: B008 _BasePermissions.DELETE_FOLDER @@ -657,7 +700,7 @@ async def folder_delete( connection, product_name, folder_id=folder_id, - gid=gid, + gids={gid}, permissions=required_permissions, enforece_all_permissions=False, ) @@ -686,7 +729,7 @@ async def folder_move( connection: SAConnection, product_name: _ProductName, source_folder_id: _FolderID, - gid: _GroupID, + gid: _GroupID, # this also can work with set[_GroupID] *, destination_folder_id: _FolderID | None, required_permissions_source: _FolderPermissions = _requires( # noqa: B008 @@ -708,7 +751,7 @@ async def folder_move( connection, product_name, folder_id=source_folder_id, - gid=gid, + gids={gid}, permissions=required_permissions_source, enforece_all_permissions=False, ) @@ -717,6 +760,7 @@ async def folder_move( group_type: GroupType | None = await connection.scalar( sa.select([groups.c.type]).where(groups.c.gid == source_access_gid) ) + # Might drop primary check if group_type is None or group_type != GroupType.PRIMARY: raise CannotMoveFolderSharedViaNonPrimaryGroupError( group_type=group_type, gid=source_access_gid @@ -726,7 +770,7 @@ async def folder_move( connection, product_name, folder_id=destination_folder_id, - gid=gid, + gids={gid}, permissions=required_permissions_destination, enforece_all_permissions=False, ) @@ -748,7 +792,7 @@ async def folder_add_project( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, - gid: _GroupID, + gid: _GroupID, # set[_GroupID] -> set of all group ids no need to pass in the sahred_via_gid *, project_uuid: _ProjectID, required_permissions=_requires( # noqa: B008 @@ -767,7 +811,7 @@ async def folder_add_project( connection, product_name, folder_id=folder_id, - gid=gid, + gids={gid}, permissions=required_permissions, enforece_all_permissions=False, ) @@ -797,7 +841,7 @@ async def folder_remove_project( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, - gid: _GroupID, + gid: _GroupID, # set[_GroupID] -> set of all group ids no need to pass in the sahred_via_gid *, project_uuid: _ProjectID, required_permissions=_requires( # noqa: B008 @@ -815,7 +859,7 @@ async def folder_remove_project( connection, product_name, folder_id=folder_id, - gid=gid, + gids={gid}, permissions=required_permissions, enforece_all_permissions=False, ) @@ -848,6 +892,7 @@ async def folder_list( results: list[FolderEntry] = [] async with connection.begin(): + # these two fields will not be required anly longer access_via_gid: _GroupID = gid access_via_folder_id: _FolderID | None = None @@ -857,7 +902,7 @@ async def folder_list( connection, product_name, folder_id=folder_id, - gid=gid, + gids={gid}, permissions=required_permissions, enforece_all_permissions=False, ) @@ -865,6 +910,7 @@ async def folder_list( access_via_folder_id = resolved_access_rights.folder_id subquery_my_access_rights = ( + # somehow apply max on columns sa.select( sa.func.jsonb_build_object( "read", @@ -923,7 +969,7 @@ async def folder_list( else folders_access_rights.c.traversal_parent_id == folder_id ) .where( - folders_access_rights.c.gid == access_via_gid + folders_access_rights.c.gid == access_via_gid # use In??? if folder_id is None else True ) @@ -934,6 +980,7 @@ async def folder_list( if folder_id is None else True ) + # todo add a group_by .offset(offset) .limit(limit) ) From 71dd6c3ae2184f8d246b3364a3076d8faaf37cf8 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 6 Aug 2024 17:16:14 +0200 Subject: [PATCH 02/35] wip: listing folders refactor --- .../utils_folders.py | 208 +++++++++--------- 1 file changed, 108 insertions(+), 100 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 1d3bdf6b813..90de171f591 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -5,7 +5,7 @@ from datetime import datetime from enum import Enum from functools import reduce -from typing import Any, ClassVar, TypeAlias +from typing import Any, ClassVar, Final, TypeAlias import sqlalchemy as sa from aiopg.sa.connection import SAConnection @@ -20,8 +20,10 @@ parse_obj_as, ) from pydantic.errors import PydanticErrorMixin +from sqlalchemy import Column, func from sqlalchemy.dialects import postgresql -from sqlalchemy.sql.elements import ColumnElement +from sqlalchemy.dialects.postgresql import BOOLEAN, INTEGER +from sqlalchemy.sql.elements import ColumnElement, Label from .models.folders import folders, folders_access_rights, folders_to_projects from .models.groups import GroupType, groups @@ -284,12 +286,6 @@ class FolderEntry(BaseModel): my_access_rights: _FolderPermissions access_rights: dict[_GroupID, _FolderPermissions] - access_via_gid: _GroupID = Field( - ..., - description="used to compute my_access_rights, should be used by the frotned", - ) - gid: _GroupID = Field(..., description="actual gid of this entry") - class Config: orm_mode = True @@ -871,47 +867,16 @@ async def folder_remove_project( ) -async def folder_list( - connection: SAConnection, - product_name: _ProductName, - folder_id: _FolderID | None, - gid: _GroupID, - *, - offset: NonNegativeInt, - limit: NonNegativeInt, - required_permissions=_requires(_BasePermissions.LIST_FOLDERS), # noqa: B008 -) -> list[FolderEntry]: - """ - Raises: - FolderNotFoundError - FolderNotSharedWithGidError - InsufficientPermissionsError - """ - # NOTE: when `folder_id is None` list the root folder of `gid` - - results: list[FolderEntry] = [] - - async with connection.begin(): - # these two fields will not be required anly longer - access_via_gid: _GroupID = gid - access_via_folder_id: _FolderID | None = None - - if folder_id: - # this one provides the set of access rights - resolved_access_rights = await _check_folder_and_access( - connection, - product_name, - folder_id=folder_id, - gids={gid}, - permissions=required_permissions, - enforece_all_permissions=False, - ) - access_via_gid = resolved_access_rights.gid - access_via_folder_id = resolved_access_rights.folder_id - - subquery_my_access_rights = ( - # somehow apply max on columns - sa.select( +_LIST_SELECT_FIELDS: Final[tuple[Label | Column, ...]] = ( + folders.c.id, + folders.c.name, + folders.c.description, + folders.c.created_by, + # access_rights + ( + sa.select( + sa.func.jsonb_object_agg( + folders_access_rights.c.gid, sa.func.jsonb_build_object( "read", folders_access_rights.c.read, @@ -919,68 +884,59 @@ async def folder_list( folders_access_rights.c.write, "delete", folders_access_rights.c.delete, - ).label("my_access_rights"), - ) - .where( - folders_access_rights.c.folder_id == access_via_folder_id - if access_via_gid and access_via_folder_id - else folders_access_rights.c.folder_id == folders.c.id - ) - .where(folders_access_rights.c.gid == access_via_gid) - .correlate(folders) - .scalar_subquery() + ), + ).label("access_rights"), ) + .where(folders_access_rights.c.folder_id == folders.c.id) + .correlate(folders) + .scalar_subquery() + ).label("access_rights"), + # my_access_rights + func.json_build_object( + "read", + func.max(folders_access_rights.c.read.cast(INTEGER)).cast(BOOLEAN), + "write", + func.max(folders_access_rights.c.write.cast(INTEGER)).cast(BOOLEAN), + "delete", + func.max(folders_access_rights.c.delete.cast(INTEGER)).cast(BOOLEAN), + ).label("my_access_rights"), + # access_created + func.max(folders_access_rights.c.created).label("access_created"), + # access_modified + func.max(folders_access_rights.c.modified).label("access_modified"), +) - subquery_access_rights = ( - sa.select( - sa.func.jsonb_object_agg( - folders_access_rights.c.gid, - sa.func.jsonb_build_object( - "read", - folders_access_rights.c.read, - "write", - folders_access_rights.c.write, - "delete", - folders_access_rights.c.delete, - ), - ).label("access_rights"), - ) - .where(folders_access_rights.c.folder_id == folders.c.id) - .correlate(folders) - .scalar_subquery() - ) +async def _list_root_folder_children( + connection: SAConnection, + product_name: _ProductName, + gids: set[_GroupID], + *, + offset: NonNegativeInt, + limit: NonNegativeInt, + required_permissions: _FolderPermissions, +) -> list[FolderEntry]: + results: list[FolderEntry] = [] + async with connection.begin(): query = ( - sa.select( - folders, - folders_access_rights, - folders_access_rights.c.created.label("access_created"), - folders_access_rights.c.modified.label("access_modified"), - sa.literal_column(f"{access_via_gid}").label("access_via_gid"), - subquery_my_access_rights.label("my_access_rights"), - subquery_access_rights.label("access_rights"), - ) + sa.select(*_LIST_SELECT_FIELDS) .join( folders_access_rights, folders.c.id == folders_access_rights.c.folder_id ) - .where( - folders_access_rights.c.traversal_parent_id.is_(None) - if folder_id is None - else folders_access_rights.c.traversal_parent_id == folder_id - ) - .where( - folders_access_rights.c.gid == access_via_gid # use In??? - if folder_id is None - else True - ) + .where(folders.c.product_name == product_name) + .where(folders_access_rights.c.traversal_parent_id.is_(None)) + .where(folders_access_rights.c.gid.in_(gids)) .where( _get_and_calsue_with_only_true_entries( required_permissions, folders_access_rights ) - if folder_id is None - else True ) - # todo add a group_by + .group_by( + folders.c.id, + folders.c.name, + folders.c.description, + folders.c.created_by, + ) .offset(offset) .limit(limit) ) @@ -988,4 +944,56 @@ async def folder_list( async for entry in connection.execute(query): results.append(FolderEntry.from_orm(entry)) # noqa: PERF401s - return results + return results + + +async def _list_folder_children( + connection: SAConnection, + product_name: _ProductName, + folder_id: _FolderID, + gids: set[_GroupID], + *, + offset: NonNegativeInt, + limit: NonNegativeInt, + required_permissions: _FolderPermissions, +) -> list[FolderEntry]: + return [] + + +async def folder_list( + connection: SAConnection, + product_name: _ProductName, + folder_id: _FolderID | None, + gids: set[_GroupID], + *, + offset: NonNegativeInt, + limit: NonNegativeInt, + required_permissions=_requires(_BasePermissions.LIST_FOLDERS), # noqa: B008 +) -> list[FolderEntry]: + """ + Raises: + FolderNotFoundError + FolderNotSharedWithGidError + InsufficientPermissionsError + """ + # NOTE: when `folder_id is None` list the root folder of the `gids` + + if folder_id is None: + return await _list_root_folder_children( + connection, + product_name, + gids, + offset=offset, + limit=limit, + required_permissions=required_permissions, + ) + + return await _list_folder_children( + connection, + product_name, + folder_id, + gids, + offset=offset, + limit=limit, + required_permissions=required_permissions, + ) From 6792a76e58d6dd21b78135f0313bd3180d8e8e6c Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 6 Aug 2024 18:03:51 +0200 Subject: [PATCH 03/35] refactor listing to work in all situations as expected --- .../utils_folders.py | 106 ++++++------------ 1 file changed, 37 insertions(+), 69 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 90de171f591..f4355f905bb 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -422,7 +422,8 @@ async def _check_folder_and_access( permissions=permissions, enforece_all_permissions=enforece_all_permissions, ) - except FolderAccessError as e: # noqa: PERF203 + break + except FolderAccessError as e: last_exception = e if resolved_access_rights is None: @@ -867,11 +868,14 @@ async def folder_remove_project( ) -_LIST_SELECT_FIELDS: Final[tuple[Label | Column, ...]] = ( +_LIST_GROUP_BY_FIELDS: Final[tuple[Column, ...]] = ( folders.c.id, folders.c.name, folders.c.description, folders.c.created_by, +) +_LIST_SELECT_FIELDS: Final[tuple[Label | Column, ...]] = ( + *_LIST_GROUP_BY_FIELDS, # access_rights ( sa.select( @@ -907,59 +911,6 @@ async def folder_remove_project( ) -async def _list_root_folder_children( - connection: SAConnection, - product_name: _ProductName, - gids: set[_GroupID], - *, - offset: NonNegativeInt, - limit: NonNegativeInt, - required_permissions: _FolderPermissions, -) -> list[FolderEntry]: - results: list[FolderEntry] = [] - async with connection.begin(): - query = ( - sa.select(*_LIST_SELECT_FIELDS) - .join( - folders_access_rights, folders.c.id == folders_access_rights.c.folder_id - ) - .where(folders.c.product_name == product_name) - .where(folders_access_rights.c.traversal_parent_id.is_(None)) - .where(folders_access_rights.c.gid.in_(gids)) - .where( - _get_and_calsue_with_only_true_entries( - required_permissions, folders_access_rights - ) - ) - .group_by( - folders.c.id, - folders.c.name, - folders.c.description, - folders.c.created_by, - ) - .offset(offset) - .limit(limit) - ) - - async for entry in connection.execute(query): - results.append(FolderEntry.from_orm(entry)) # noqa: PERF401s - - return results - - -async def _list_folder_children( - connection: SAConnection, - product_name: _ProductName, - folder_id: _FolderID, - gids: set[_GroupID], - *, - offset: NonNegativeInt, - limit: NonNegativeInt, - required_permissions: _FolderPermissions, -) -> list[FolderEntry]: - return [] - - async def folder_list( connection: SAConnection, product_name: _ProductName, @@ -978,22 +929,39 @@ async def folder_list( """ # NOTE: when `folder_id is None` list the root folder of the `gids` - if folder_id is None: - return await _list_root_folder_children( + if folder_id is not None: + await _check_folder_and_access( connection, product_name, - gids, - offset=offset, - limit=limit, - required_permissions=required_permissions, + folder_id=folder_id, + gids=gids, + permissions=required_permissions, + enforece_all_permissions=False, ) - return await _list_folder_children( - connection, - product_name, - folder_id, - gids, - offset=offset, - limit=limit, - required_permissions=required_permissions, + results: list[FolderEntry] = [] + + query = ( + sa.select(*_LIST_SELECT_FIELDS) + .join(folders_access_rights, folders.c.id == folders_access_rights.c.folder_id) + .where(folders.c.product_name == product_name) + .where( + folders_access_rights.c.traversal_parent_id.is_(None) + if folder_id is None + else folders_access_rights.c.traversal_parent_id == folder_id + ) + .where(folders_access_rights.c.gid.in_(gids)) + .where( + _get_and_calsue_with_only_true_entries( + required_permissions, folders_access_rights + ) + ) + .group_by(*_LIST_GROUP_BY_FIELDS) + .offset(offset) + .limit(limit) ) + + async for entry in connection.execute(query): + results.append(FolderEntry.from_orm(entry)) # noqa: PERF401s + + return results From cdc3538e9871ccb8244e2aa062e84560bb0ccb7e Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 6 Aug 2024 18:04:19 +0200 Subject: [PATCH 04/35] refactor tests --- .../tests/test_utils_folders.py | 119 +++++++++++++----- 1 file changed, 87 insertions(+), 32 deletions(-) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 7a4714b4b5a..3d6b002f094 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -1581,7 +1581,6 @@ async def _remove_folder_as(gid: _GroupID) -> None: class ExpectedValues(NamedTuple): id: _FolderID - access_via_gid: _GroupID my_access_rights: _FolderPermissions access_rights: dict[_GroupID, _FolderPermissions] @@ -1589,7 +1588,6 @@ def __hash__(self): return hash( ( self.id, - self.access_via_gid, self.my_access_rights, tuple(sorted(self.access_rights.items())), ) @@ -1600,7 +1598,6 @@ def __eq__(self, other): return False return ( self.id == other.id - and self.access_via_gid == other.access_via_gid and self.my_access_rights == other.my_access_rights and self.access_rights == other.access_rights ) @@ -1612,7 +1609,6 @@ def _assert_expected_entries( for folder_entry in folders: expected_values = ExpectedValues( folder_entry.id, - folder_entry.access_via_gid, folder_entry.my_access_rights, folder_entry.access_rights, ) @@ -1627,13 +1623,13 @@ async def _list_folder_as( connection: SAConnection, default_product_name: _ProductName, folder_id: _FolderID | None, - gid: _GroupID, + gids: set[_GroupID], offset: NonNegativeInt = ALL_IN_ONE_PAGE_OFFSET, limit: NonNegativeInt = ALL_IN_ONE_PAGE_LIMIT, ) -> list[FolderEntry]: return await folder_list( - connection, default_product_name, folder_id, gid, offset=offset, limit=limit + connection, default_product_name, folder_id, gids, offset=offset, limit=limit ) @@ -1748,11 +1744,12 @@ async def test_folder_list( for listing_gid in (gid_owner, gid_editor, gid_viewer): # list `root` for gid _assert_expected_entries( - await _list_folder_as(connection, default_product_name, None, listing_gid), + await _list_folder_as( + connection, default_product_name, None, {listing_gid} + ), expected={ ExpectedValues( folder_id_owner_folder, - listing_gid, ACCESS_RIGHTS_BY_GID[listing_gid], { gid_owner: OWNER_PERMISSIONS, @@ -1766,12 +1763,11 @@ async def test_folder_list( # list `owner_folder` for gid _assert_expected_entries( await _list_folder_as( - connection, default_product_name, folder_id_owner_folder, listing_gid + connection, default_product_name, folder_id_owner_folder, {listing_gid} ), expected={ ExpectedValues( fx, - listing_gid, ACCESS_RIGHTS_BY_GID[listing_gid], {gid_owner: OWNER_PERMISSIONS}, ) @@ -1781,12 +1777,11 @@ async def test_folder_list( # list `f10` for gid _assert_expected_entries( await _list_folder_as( - connection, default_product_name, folder_id_f10, listing_gid + connection, default_product_name, folder_id_f10, {listing_gid} ), expected={ ExpectedValues( sub_fx, - listing_gid, ACCESS_RIGHTS_BY_GID[listing_gid], {gid_owner: OWNER_PERMISSIONS}, ) @@ -1797,26 +1792,26 @@ async def test_folder_list( # 2. lisit all levels for `gid_no_access` # can always be ran but should not list any entry _assert_expected_entries( - await _list_folder_as(connection, default_product_name, None, gid_no_access), + await _list_folder_as(connection, default_product_name, None, {gid_no_access}), expected=set(), ) # there are insusficient permissions for folder_id_to_check in ALL_FOLDERS_AND_SUBFOLDERS: with pytest.raises(InsufficientPermissionsError): await _list_folder_as( - connection, default_product_name, folder_id_to_check, gid_no_access + connection, default_product_name, folder_id_to_check, {gid_no_access} ) # 3. lisit all levels for `gid_not_shared`` # can always list the contets of the "root" folder for a gid _assert_expected_entries( - await _list_folder_as(connection, default_product_name, None, gid_not_shared), + await _list_folder_as(connection, default_product_name, None, {gid_not_shared}), expected=set(), ) for folder_id_to_check in ALL_FOLDERS_AND_SUBFOLDERS: with pytest.raises(FolderNotSharedWithGidError): await _list_folder_as( - connection, default_product_name, folder_id_to_check, gid_not_shared + connection, default_product_name, folder_id_to_check, {gid_not_shared} ) # 4. list with pagination @@ -1828,7 +1823,7 @@ async def test_folder_list( connection, default_product_name, folder_id_owner_folder, - gid_owner, + {gid_owner}, offset=offset, limit=limit, ): @@ -1838,7 +1833,7 @@ async def test_folder_list( break one_shot_query = await _list_folder_as( - connection, default_product_name, folder_id_owner_folder, gid_owner + connection, default_product_name, folder_id_owner_folder, {gid_owner} ) assert len(found_folders) == len(one_shot_query) @@ -1903,11 +1898,12 @@ async def test_folder_list_shared_with_different_permissions( for listing_gid in (gid_owner_a, gid_owner_b, gid_owner_c): # list `root` for gid _assert_expected_entries( - await _list_folder_as(connection, default_product_name, None, listing_gid), + await _list_folder_as( + connection, default_product_name, None, {listing_gid} + ), expected={ ExpectedValues( folder_id_f_owner_a, - listing_gid, OWNER_PERMISSIONS, { gid_owner_a: OWNER_PERMISSIONS, @@ -1920,12 +1916,11 @@ async def test_folder_list_shared_with_different_permissions( # list `f_owner_a` for gid _assert_expected_entries( await _list_folder_as( - connection, default_product_name, folder_id_f_owner_a, listing_gid + connection, default_product_name, folder_id_f_owner_a, {listing_gid} ), expected={ ExpectedValues( folder_id_f_owner_b, - listing_gid, OWNER_PERMISSIONS, {gid_owner_b: OWNER_PERMISSIONS}, ), @@ -1934,12 +1929,11 @@ async def test_folder_list_shared_with_different_permissions( # list `f_owner_b` for gid _assert_expected_entries( await _list_folder_as( - connection, default_product_name, folder_id_f_owner_b, listing_gid + connection, default_product_name, folder_id_f_owner_b, {listing_gid} ), expected={ ExpectedValues( folder_id_f_owner_c, - listing_gid, OWNER_PERMISSIONS, { gid_owner_c: OWNER_PERMISSIONS, @@ -1951,12 +1945,11 @@ async def test_folder_list_shared_with_different_permissions( # list `f_owner_c` for gid _assert_expected_entries( await _list_folder_as( - connection, default_product_name, folder_id_f_owner_c, listing_gid + connection, default_product_name, folder_id_f_owner_c, {listing_gid} ), expected={ ExpectedValues( folder_id_f_sub_owner_c, - listing_gid, OWNER_PERMISSIONS, { gid_owner_c: OWNER_PERMISSIONS, @@ -1964,7 +1957,6 @@ async def test_folder_list_shared_with_different_permissions( ), ExpectedValues( folder_id_f_owner_level_2, - listing_gid, OWNER_PERMISSIONS, { gid_owner_level_2: OWNER_PERMISSIONS, @@ -1977,12 +1969,11 @@ async def test_folder_list_shared_with_different_permissions( # list `f_owner_c` for `gid_owner_level_2` _assert_expected_entries( await _list_folder_as( - connection, default_product_name, None, gid_owner_level_2 + connection, default_product_name, None, {gid_owner_level_2} ), expected={ ExpectedValues( folder_id_f_owner_c, - gid_owner_level_2, OWNER_PERMISSIONS, { gid_owner_c: OWNER_PERMISSIONS, @@ -1994,12 +1985,11 @@ async def test_folder_list_shared_with_different_permissions( # list `root` for `gid_owner_level_2` _assert_expected_entries( await _list_folder_as( - connection, default_product_name, folder_id_f_owner_c, gid_owner_level_2 + connection, default_product_name, folder_id_f_owner_c, {gid_owner_level_2} ), expected={ ExpectedValues( folder_id_f_sub_owner_c, - gid_owner_level_2, OWNER_PERMISSIONS, { gid_owner_c: OWNER_PERMISSIONS, @@ -2007,7 +1997,6 @@ async def test_folder_list_shared_with_different_permissions( ), ExpectedValues( folder_id_f_owner_level_2, - gid_owner_level_2, OWNER_PERMISSIONS, { gid_owner_level_2: OWNER_PERMISSIONS, @@ -2015,3 +2004,69 @@ async def test_folder_list_shared_with_different_permissions( ), }, ) + + +async def test_folder_list_multiple_entries_via_different_groups( + connection: SAConnection, + default_product_name: _ProductName, + get_unique_gids: Callable[[int], tuple[_GroupID, ...]], + make_folders: Callable[[set[MkFolder]], Awaitable[dict[str, _FolderID]]], +): + ####### + # SETUP + ####### + + (gid_z43, gid_osparc, gid_user) = get_unique_gids(3) + + folder_ids = await make_folders( + { + MkFolder( + name="f1", + gid=gid_user, + shared_with={ + gid_z43: FolderAccessRole.OWNER, + gid_osparc: FolderAccessRole.OWNER, + }, + ), + MkFolder( + name="f2", + gid=gid_z43, + shared_with={ + gid_osparc: FolderAccessRole.OWNER, + }, + ), + MkFolder( + name="f3", + gid=gid_osparc, + shared_with={ + gid_z43: FolderAccessRole.OWNER, + }, + ), + } + ) + + folder_id_f1 = folder_ids["f1"] + folder_id_f2 = folder_ids["f2"] + folder_id_f3 = folder_ids["f3"] + + entries_z43 = await _list_folder_as( + connection, default_product_name, None, {gid_z43} + ) + + assert len(entries_z43) == 3 + entries_osparc = await _list_folder_as( + connection, default_product_name, None, {gid_osparc} + ) + assert len(entries_osparc) == 3 + entries_user = await _list_folder_as( + connection, default_product_name, None, {gid_user} + ) + assert len(entries_user) == 1 + + entries_all_groups = await _list_folder_as( + connection, default_product_name, None, {gid_z43, gid_osparc, gid_user} + ) + assert len(entries_all_groups) == 3 + + assert False + # TODO: continue this one From 9fcbad04bb5a07f713da7d1f74bd2ac7dae62818 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 6 Aug 2024 18:04:44 +0200 Subject: [PATCH 05/35] refactor --- packages/postgres-database/tests/test_utils_folders.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 3d6b002f094..b036f1f64c0 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -2068,5 +2068,5 @@ async def test_folder_list_multiple_entries_via_different_groups( ) assert len(entries_all_groups) == 3 + # TODO: continue this test make sure it works as expected assert False - # TODO: continue this one From 6fca0a47e81fb601256c00dfbb84d6d43f217df6 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 09:23:26 +0200 Subject: [PATCH 06/35] sharing accepts gorups set --- .../utils_folders.py | 4 +-- .../tests/test_utils_folders.py | 36 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index f4355f905bb..bd4c6f0dd52 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -583,7 +583,7 @@ async def folder_share_or_update_permissions( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, - sharing_gid: _GroupID, # set[_GroupID] -> set of all group ids no need to pass in the sahred_via_gid + sharing_gids: set[_GroupID], *, recipient_gid: _GroupID, recipient_role: FolderAccessRole, @@ -603,7 +603,7 @@ async def folder_share_or_update_permissions( connection, product_name, folder_id=folder_id, - gids={sharing_gid}, + gids=sharing_gids, permissions=required_permissions, enforece_all_permissions=False, ) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index b036f1f64c0..7d0ae12d047 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -387,7 +387,7 @@ async def _( connection, default_product_name, root_folder_id, - root.gid, + sharing_gids={root.gid}, recipient_gid=gid, recipient_role=role, ) @@ -590,7 +590,7 @@ async def test_folder_share_or_update_permissions( connection, default_product_name, folder_id_missing, - sharing_gid=gid_owner, + sharing_gids={gid_owner}, recipient_gid=gid_share_with_error, recipient_role=FolderAccessRole.OWNER, ) @@ -607,7 +607,7 @@ async def test_folder_share_or_update_permissions( connection, default_product_name, folder_id, - sharing_gid=gid_owner, + sharing_gids={gid_owner}, recipient_gid=gid_other_owner, recipient_role=FolderAccessRole.OWNER, ) @@ -623,7 +623,7 @@ async def test_folder_share_or_update_permissions( connection, default_product_name, folder_id, - sharing_gid=gid_owner, + sharing_gids={gid_owner}, recipient_gid=gid_editor, recipient_role=FolderAccessRole.EDITOR, ) @@ -636,7 +636,7 @@ async def test_folder_share_or_update_permissions( connection, default_product_name, folder_id, - sharing_gid=gid_owner, + sharing_gids={gid_owner}, recipient_gid=gid_viewer, recipient_role=FolderAccessRole.VIEWER, ) @@ -649,7 +649,7 @@ async def test_folder_share_or_update_permissions( connection, default_product_name, folder_id, - sharing_gid=gid_owner, + sharing_gids={gid_owner}, recipient_gid=gid_no_access, recipient_role=FolderAccessRole.NO_ACCESS, ) @@ -669,7 +669,7 @@ async def test_folder_share_or_update_permissions( connection, default_product_name, folder_id, - sharing_gid=no_access_gids, + sharing_gids={no_access_gids}, recipient_gid=gid_share_with_error, recipient_role=recipient_role, ) @@ -682,7 +682,7 @@ async def test_folder_share_or_update_permissions( connection, default_product_name, folder_id, - sharing_gid=gid_share_with_error, + sharing_gids={gid_share_with_error}, recipient_gid=gid_share_with_error, recipient_role=recipient_role, ) @@ -695,7 +695,7 @@ async def test_folder_share_or_update_permissions( connection, default_product_name, folder_id, - sharing_gid=gid_other_owner, + sharing_gids={gid_other_owner}, recipient_gid=gid_to_drop_permission, recipient_role=FolderAccessRole.NO_ACCESS, ) @@ -757,7 +757,7 @@ async def test_folder_update( connection, default_product_name, folder_id, - sharing_gid=owner_gid, + sharing_gids={owner_gid}, recipient_gid=other_owner_gid, recipient_role=FolderAccessRole.OWNER, ) @@ -781,7 +781,7 @@ async def test_folder_update( connection, default_product_name, folder_id, - sharing_gid=owner_gid, + sharing_gids={owner_gid}, recipient_gid=editor_gid, recipient_role=FolderAccessRole.EDITOR, ) @@ -789,7 +789,7 @@ async def test_folder_update( connection, default_product_name, folder_id, - sharing_gid=owner_gid, + sharing_gids={owner_gid}, recipient_gid=viewer_gid, recipient_role=FolderAccessRole.VIEWER, ) @@ -797,7 +797,7 @@ async def test_folder_update( connection, default_product_name, folder_id, - sharing_gid=owner_gid, + sharing_gids={owner_gid}, recipient_gid=no_access_gid, recipient_role=FolderAccessRole.NO_ACCESS, ) @@ -873,7 +873,7 @@ async def test_folder_delete( connection, default_product_name, folder_id, - sharing_gid=owner_gid, + sharing_gids={owner_gid}, recipient_gid=other_owner_gid, recipient_role=FolderAccessRole.OWNER, ) @@ -889,7 +889,7 @@ async def test_folder_delete( connection, default_product_name, folder_id, - sharing_gid=owner_gid, + sharing_gids={owner_gid}, recipient_gid=editor_gid, recipient_role=FolderAccessRole.EDITOR, ) @@ -897,7 +897,7 @@ async def test_folder_delete( connection, default_product_name, folder_id, - sharing_gid=owner_gid, + sharing_gids={owner_gid}, recipient_gid=viewer_gid, recipient_role=FolderAccessRole.VIEWER, ) @@ -905,7 +905,7 @@ async def test_folder_delete( connection, default_product_name, folder_id, - sharing_gid=owner_gid, + sharing_gids={owner_gid}, recipient_gid=no_access_gid, recipient_role=FolderAccessRole.NO_ACCESS, ) @@ -2069,4 +2069,4 @@ async def test_folder_list_multiple_entries_via_different_groups( assert len(entries_all_groups) == 3 # TODO: continue this test make sure it works as expected - assert False + # assert False From 825cc964347263a57cf9a91994b9c7131f9ce3b7 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 09:41:52 +0200 Subject: [PATCH 07/35] folders_update accepts gids --- .../src/simcore_postgres_database/utils_folders.py | 4 ++-- .../postgres-database/tests/test_utils_folders.py | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index bd4c6f0dd52..ea24738ed6c 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -634,7 +634,7 @@ async def folder_update( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, - gid: _GroupID, # set[_GroupID] -> set of all group ids no need to pass in the sahred_via_gid + gids: set[_GroupID], *, name: str | None = None, description: str | None = None, @@ -653,7 +653,7 @@ async def folder_update( connection, product_name, folder_id=folder_id, - gids={gid}, + gids=gids, permissions=required_permissions, enforece_all_permissions=False, ) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 7d0ae12d047..1d8416a161d 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -726,7 +726,7 @@ async def test_folder_update( missing_folder_id = 1231321332 with pytest.raises(FolderNotFoundError): await folder_update( - connection, default_product_name, missing_folder_id, owner_gid + connection, default_product_name, missing_folder_id, {owner_gid} ) await _assert_folder_entires(connection, folder_count=0) @@ -736,7 +736,7 @@ async def test_folder_update( await _assert_name_and_description(connection, folder_id, name="f1", description="") # nothing changes - await folder_update(connection, default_product_name, folder_id, owner_gid) + await folder_update(connection, default_product_name, folder_id, {owner_gid}) await _assert_name_and_description(connection, folder_id, name="f1", description="") # both changed @@ -744,7 +744,7 @@ async def test_folder_update( connection, default_product_name, folder_id, - owner_gid, + {owner_gid}, name="new_folder", description="new_desc", ) @@ -765,7 +765,7 @@ async def test_folder_update( connection, default_product_name, folder_id, - owner_gid, + {owner_gid}, name="another_owner_name", description="another_owner_description", ) @@ -808,7 +808,7 @@ async def test_folder_update( connection, default_product_name, folder_id, - target_user_gid, + {target_user_gid}, name="error_name", description="error_description", ) @@ -824,7 +824,7 @@ async def test_folder_update( connection, default_product_name, folder_id, - share_with_error_gid, + {share_with_error_gid}, name="error_name", description="error_description", ) From f4001b0fd469a9942258ec61bda61acd05a38026 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 09:44:47 +0200 Subject: [PATCH 08/35] folder_delete --- .../simcore_postgres_database/utils_folders.py | 6 +++--- .../tests/test_utils_folders.py | 18 +++++++++--------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index ea24738ed6c..2ef92e9b74a 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -678,7 +678,7 @@ async def folder_delete( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, - gid: _GroupID, # set[_GroupID] -> set of all group ids no need to pass in the sahred_via_gid + gids: set[_GroupID], *, required_permissions: _FolderPermissions = _requires( # noqa: B008 _BasePermissions.DELETE_FOLDER @@ -697,7 +697,7 @@ async def folder_delete( connection, product_name, folder_id=folder_id, - gids={gid}, + gids=gids, permissions=required_permissions, enforece_all_permissions=False, ) @@ -715,7 +715,7 @@ async def folder_delete( # first remove all childeren for child_folder_id in childern_folder_ids: - await folder_delete(connection, product_name, child_folder_id, gid) + await folder_delete(connection, product_name, child_folder_id, gids) # as a last step remove the folder per se async with connection.begin(): diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 1d8416a161d..b0e62eea84b 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -854,7 +854,7 @@ async def test_folder_delete( missing_folder_id = 1231321332 with pytest.raises(FolderNotFoundError): await folder_delete( - connection, default_product_name, missing_folder_id, owner_gid + connection, default_product_name, missing_folder_id, {owner_gid} ) await _assert_folder_entires(connection, folder_count=0) @@ -862,7 +862,7 @@ async def test_folder_delete( folder_id = await folder_create(connection, default_product_name, "f1", owner_gid) await _assert_folder_entires(connection, folder_count=1) - await folder_delete(connection, default_product_name, folder_id, owner_gid) + await folder_delete(connection, default_product_name, folder_id, {owner_gid}) await _assert_folder_entires(connection, folder_count=0) # 3. other owners can delete the folder @@ -878,7 +878,7 @@ async def test_folder_delete( recipient_role=FolderAccessRole.OWNER, ) - await folder_delete(connection, default_product_name, folder_id, other_owner_gid) + await folder_delete(connection, default_product_name, folder_id, {other_owner_gid}) await _assert_folder_entires(connection, folder_count=0) # 4. non owner users cannot delete the folder @@ -914,12 +914,12 @@ async def test_folder_delete( for non_owner_gid in (editor_gid, viewer_gid, no_access_gid): with pytest.raises(InsufficientPermissionsError): await folder_delete( - connection, default_product_name, folder_id, non_owner_gid + connection, default_product_name, folder_id, {non_owner_gid} ) with pytest.raises(FolderNotSharedWithGidError): await folder_delete( - connection, default_product_name, folder_id, share_with_error_gid + connection, default_product_name, folder_id, {share_with_error_gid} ) await _assert_folder_entires(connection, folder_count=1, access_rights_count=4) @@ -992,14 +992,14 @@ async def _setup_folders() -> _FolderID: # 1. delete via `gid_owner_a` folder_id_root_folder = await _setup_folders() await folder_delete( - connection, default_product_name, folder_id_root_folder, gid_owner_a + connection, default_product_name, folder_id_root_folder, {gid_owner_a} ) await _assert_folder_entires(connection, folder_count=0) # 2. delete via shared with `gid_owner_b` folder_id_root_folder = await _setup_folders() await folder_delete( - connection, default_product_name, folder_id_root_folder, gid_owner_b + connection, default_product_name, folder_id_root_folder, {gid_owner_b} ) await _assert_folder_entires(connection, folder_count=0) @@ -1011,7 +1011,7 @@ async def _setup_folders() -> _FolderID: connection, default_product_name, folder_id_root_folder, - no_permissions_gid, + {no_permissions_gid}, ) for no_permissions_gid in (gid_not_shared,): with pytest.raises(FolderNotSharedWithGidError): @@ -1019,7 +1019,7 @@ async def _setup_folders() -> _FolderID: connection, default_product_name, folder_id_root_folder, - no_permissions_gid, + {no_permissions_gid}, ) await _assert_folder_entires(connection, folder_count=101, access_rights_count=106) From a611596c2f3b3e9c52a292c4ae8a0f40cbacb833 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 09:46:59 +0200 Subject: [PATCH 09/35] folder_move uses gids --- .../utils_folders.py | 8 ++++---- .../tests/test_utils_folders.py | 20 +++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 2ef92e9b74a..60a1f973d11 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -726,7 +726,7 @@ async def folder_move( connection: SAConnection, product_name: _ProductName, source_folder_id: _FolderID, - gid: _GroupID, # this also can work with set[_GroupID] + gids: set[_GroupID], *, destination_folder_id: _FolderID | None, required_permissions_source: _FolderPermissions = _requires( # noqa: B008 @@ -748,7 +748,7 @@ async def folder_move( connection, product_name, folder_id=source_folder_id, - gids={gid}, + gids=gids, permissions=required_permissions_source, enforece_all_permissions=False, ) @@ -767,7 +767,7 @@ async def folder_move( connection, product_name, folder_id=destination_folder_id, - gids={gid}, + gids=gids, permissions=required_permissions_destination, enforece_all_permissions=False, ) @@ -778,7 +778,7 @@ async def folder_move( .where( sa.and_( folders_access_rights.c.folder_id == source_folder_id, - folders_access_rights.c.gid == gid, + folders_access_rights.c.gid.in_(gids), ) ) .values(traversal_parent_id=destination_folder_id) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index b0e62eea84b..35ab187f1a9 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -1138,7 +1138,7 @@ async def _move_fails_not_shared_with_error( connection, default_product_name, source, - gid, + {gid}, destination_folder_id=destination, ) @@ -1150,7 +1150,7 @@ async def _move_fails_insufficient_permissions_error( connection, default_product_name, source, - gid, + {gid}, destination_folder_id=destination, ) @@ -1187,7 +1187,7 @@ async def _assert_folder_permissions( connection, default_product_name, source, - gid, + {gid}, destination_folder_id=destination, ) @@ -1200,7 +1200,7 @@ async def _assert_folder_permissions( connection, default_product_name, source, - gid, + {gid}, destination_folder_id=source_parent, ) @@ -1296,7 +1296,7 @@ async def _assert_folder_permissions( connection, default_product_name, to_move_folder_id, - to_move_gid, + {to_move_gid}, destination_folder_id=None, ) @@ -1314,7 +1314,7 @@ async def _assert_folder_permissions( connection, default_product_name, to_move_folder_id, - to_move_gid, + {to_move_gid}, destination_folder_id=None, ) @@ -1324,7 +1324,7 @@ async def _assert_folder_permissions( connection, default_product_name, folder_id_not_shared, - to_move_gid, + {to_move_gid}, destination_folder_id=None, ) @@ -1378,7 +1378,7 @@ async def _fails_to_move(gid: _GroupID, destination_folder_id: _FolderID) -> Non connection, default_product_name, folder_id_to_move, - gid, + {gid}, destination_folder_id=destination_folder_id, ) @@ -1397,7 +1397,7 @@ async def _fails_to_move(gid: _GroupID, destination_folder_id: _FolderID) -> Non connection, default_product_name, folder_id_to_move, - gid_not_shared, + {gid_not_shared}, destination_folder_id=folder_id_target_not_shared, ) @@ -1406,7 +1406,7 @@ async def _fails_to_move(gid: _GroupID, destination_folder_id: _FolderID) -> Non connection, default_product_name, folder_id_to_move, - gid_owner, + {gid_owner}, destination_folder_id=folder_id_target_owner, ) From 2abd13f6437ede5ec1b231bcc6e6c5fe5afaa3fd Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 09:51:07 +0200 Subject: [PATCH 10/35] add remove project use gids --- .../utils_folders.py | 8 +++---- .../tests/test_utils_folders.py | 21 ++++++++++++------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 60a1f973d11..c79ec415c16 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -789,7 +789,7 @@ async def folder_add_project( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, - gid: _GroupID, # set[_GroupID] -> set of all group ids no need to pass in the sahred_via_gid + gids: set[_GroupID], *, project_uuid: _ProjectID, required_permissions=_requires( # noqa: B008 @@ -808,7 +808,7 @@ async def folder_add_project( connection, product_name, folder_id=folder_id, - gids={gid}, + gids=gids, permissions=required_permissions, enforece_all_permissions=False, ) @@ -838,7 +838,7 @@ async def folder_remove_project( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, - gid: _GroupID, # set[_GroupID] -> set of all group ids no need to pass in the sahred_via_gid + gids: set[_GroupID], *, project_uuid: _ProjectID, required_permissions=_requires( # noqa: B008 @@ -856,7 +856,7 @@ async def folder_remove_project( connection, product_name, folder_id=folder_id, - gids={gid}, + gids=gids, permissions=required_permissions, enforece_all_permissions=False, ) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 35ab187f1a9..52baea189e2 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -1421,13 +1421,18 @@ async def test_move_group_non_standard_groups_raise_error( ####### # SETUP ####### + gid_sharing: _GroupID (gid_sharing,) = get_unique_gids(1) - gid_primary = (await create_fake_group(connection, type=GroupType.PRIMARY)).gid - gid_everyone = await connection.scalar( + gid_primary: _GroupID = ( + await create_fake_group(connection, type=GroupType.PRIMARY) + ).gid + gid_everyone: _GroupID | None = await connection.scalar( sa.select([groups.c.gid]).where(groups.c.type == GroupType.EVERYONE) ) assert gid_everyone - gid_standard = (await create_fake_group(connection, type=GroupType.STANDARD)).gid + gid_standard: _GroupID = ( + await create_fake_group(connection, type=GroupType.STANDARD) + ).gid folder_ids = await make_folders( { @@ -1460,7 +1465,7 @@ async def test_move_group_non_standard_groups_raise_error( connection, default_product_name, folder_id_everyone, - gid_everyone, + {gid_everyone}, destination_folder_id=folder_id_sharing_user, ) assert "EVERYONE" in f"{exc.value}" @@ -1470,7 +1475,7 @@ async def test_move_group_non_standard_groups_raise_error( connection, default_product_name, folder_id_standard, - gid_standard, + {gid_standard}, destination_folder_id=folder_id_sharing_user, ) assert "STANDARD" in f"{exc.value}" @@ -1480,7 +1485,7 @@ async def test_move_group_non_standard_groups_raise_error( connection, default_product_name, folder_id_primary, - gid_primary, + {gid_primary}, destination_folder_id=folder_id_sharing_user, ) @@ -1533,7 +1538,7 @@ async def _add_folder_as(gid: _GroupID) -> None: connection, default_product_name, folder_id_f1, - gid, + {gid}, project_uuid=project_uuid, ) assert await _is_project_present(connection, folder_id_f1, project_uuid) is True @@ -1543,7 +1548,7 @@ async def _remove_folder_as(gid: _GroupID) -> None: connection, default_product_name, folder_id_f1, - gid, + {gid}, project_uuid=project_uuid, ) assert ( From 235bdd37e4ca0088ef2d52370bbbcd333c16527c Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 09:57:25 +0200 Subject: [PATCH 11/35] refactor raised error --- .../utils_folders.py | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index c79ec415c16..0445c9cf5fa 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -46,6 +46,7 @@ * FolderNotFoundError * FolderNotSharedWithGidError * InsufficientPermissionsError + * UnexpectedFolderAccessError * BaseCreateFolderError * FolderAlreadyExistsError * ParentFolderIsNotWritableError @@ -84,6 +85,13 @@ class InsufficientPermissionsError(FolderAccessError): msg_template = "could not find a parent for folder_id={folder_id} and gid={gid}, with permissions={permissions}" +class UnexpectedFolderAccessError(FoldersError): + msg_template = ( + "Unexpected None value for resolved_access_rights={resolved_access_rights} and last_exception={last_exception}." + "Called with: product_name={product_name}, folder_id={folder_id}, gids={gids}" + ) + + class BaseCreateFolderError(FoldersError): pass @@ -409,6 +417,7 @@ async def _check_folder_and_access( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError + UnexpectedFolderAccessError """ resolved_access_rights: _ResolvedAccessRights | None = None last_exception: Exception | None = None @@ -430,8 +439,13 @@ async def _check_folder_and_access( if last_exception: raise last_exception - msg = "unexpected error" - raise RuntimeError(msg) + raise UnexpectedFolderAccessError( + resolved_access_rights=resolved_access_rights, + last_exception=last_exception, + product_name=product_name, + folder_id=folder_id, + gids=gids, + ) return resolved_access_rights @@ -450,6 +464,7 @@ async def _check_folder_access( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError + UnexpectedFolderAccessError """ folder_entry: int | None = await connection.scalar( sa.select([folders.c.id]) @@ -509,6 +524,7 @@ async def folder_create( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError + UnexpectedFolderAccessError FolderAlreadyExistsError CouldNotCreateFolderError GroupIdDoesNotExistError @@ -596,6 +612,7 @@ async def folder_share_or_update_permissions( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError + UnexpectedFolderAccessError """ # NOTE: if the `sharing_gid`` has permissions to share it can share it with any `FolderAccessRole` async with connection.begin(): @@ -647,6 +664,7 @@ async def folder_update( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError + UnexpectedFolderAccessError """ async with connection.begin(): await _check_folder_and_access( @@ -689,6 +707,7 @@ async def folder_delete( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError + UnexpectedFolderAccessError """ childern_folder_ids: list[_FolderID] = [] @@ -741,6 +760,7 @@ async def folder_move( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError + UnexpectedFolderAccessError CannotMoveFolderSharedViaNonPrimaryGroupError: """ async with connection.begin(): @@ -801,6 +821,7 @@ async def folder_add_project( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError + UnexpectedFolderAccessError ProjectAlreadyExistsInFolderError """ async with connection.begin(): @@ -850,6 +871,7 @@ async def folder_remove_project( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError + UnexpectedFolderAccessError """ async with connection.begin(): await _check_folder_and_access( @@ -926,6 +948,7 @@ async def folder_list( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError + UnexpectedFolderAccessError """ # NOTE: when `folder_id is None` list the root folder of the `gids` From eecf1f645edc37d8eaf7977f70de14b58d6c11e8 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 11:18:20 +0200 Subject: [PATCH 12/35] folder_create now uses gids --- .../utils_folders.py | 48 +++++-- .../tests/test_utils_folders.py | 120 ++++++++++++++++-- 2 files changed, 141 insertions(+), 27 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 0445c9cf5fa..d9b7b3c29ef 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -52,6 +52,7 @@ * ParentFolderIsNotWritableError * CouldNotCreateFolderError * GroupIdDoesNotExistError + * RootFolderRequiresAtLeastOnePrimaryGroupError * BaseMoveFolderError * CannotMoveFolderSharedViaNonPrimaryGroupError * BaseAddProjectError @@ -97,7 +98,7 @@ class BaseCreateFolderError(FoldersError): class FolderAlreadyExistsError(BaseCreateFolderError): - msg_template = "A folder='{folder}' with parent='{parent}' for group='{gid}' in product_name={product_name} already exists" + msg_template = "A folder='{folder}' with parent='{parent}' for gids='{gids}' in product_name={product_name} already exists" class ParentFolderIsNotWritableError(BaseCreateFolderError): @@ -112,6 +113,13 @@ class GroupIdDoesNotExistError(BaseCreateFolderError): msg_template = "Provided group id '{gid}' does not exist " +class RootFolderRequiresAtLeastOnePrimaryGroupError(BaseCreateFolderError): + msg_template = ( + "parent={parent} is None (please check) and gids={gids} did not contain a PRIMARY group. " + "Cannot create a folder isnide the 'root' directory." + ) + + class BaseMoveFolderError(FoldersError): pass @@ -512,7 +520,7 @@ async def folder_create( connection: SAConnection, product_name: _ProductName, name: str, - gid: _GroupID, # shared_via_gid TO WORK + gids: set[_GroupID], description: str = "", parent: _FolderID | None = None, required_permissions: _FolderPermissions = _requires( # noqa: B008 @@ -528,6 +536,7 @@ async def folder_create( FolderAlreadyExistsError CouldNotCreateFolderError GroupIdDoesNotExistError + RootFolderRequiresAtLeastOnePrimaryGroupError """ parse_obj_as(FolderName, name) @@ -546,23 +555,36 @@ async def folder_create( ) if entry_exists: raise FolderAlreadyExistsError( - product_name=product_name, folder=name, parent=parent, gid=gid + product_name=product_name, folder=name, parent=parent, gids=gids ) + # `permissions_gid` is compted as follows: + # - `folder has a parent?` taken from the resolved access rights of the parent folder + # - `is root folder, a.k.a. no parent?` taken from the user's primary group + permissions_gid: _GroupID | None = None if parent: - # check if parent has permissions - await _check_folder_and_access( + resolved_access_rights = await _check_folder_and_access( connection, product_name, folder_id=parent, - gids={gid}, + gids=gids, permissions=required_permissions, enforece_all_permissions=False, ) - # TODO: add test to emulate Shared with Z43 and Osparc and not with MD - # -> MD_gid tries to create a folder inside this one -> should fail? - # TODO: use a set to check permissions on the parent - # "random" pick user between all grops it has access to + permissions_gid = resolved_access_rights.gid + + if permissions_gid is None: + primary_gid: _GroupID | None = await connection.scalar( + sa.select([groups.c.gid]) + .where(groups.c.gid.in_(gids)) + .where(groups.c.type == GroupType.PRIMARY) + ) + if primary_gid is None: + raise RootFolderRequiresAtLeastOnePrimaryGroupError( + parent=parent, gids=gids + ) + + permissions_gid = primary_gid # folder entry can now be inserted try: @@ -571,7 +593,7 @@ async def folder_create( .values( name=name, description=description, - created_by=gid, + created_by=permissions_gid, product_name=product_name, ) .returning(folders.c.id) @@ -583,14 +605,14 @@ async def folder_create( await connection.execute( sa.insert(folders_access_rights).values( folder_id=folder_id, - gid=gid, + gid=permissions_gid, traversal_parent_id=parent, original_parent_id=parent, **OWNER_PERMISSIONS.to_dict(), ) ) except ForeignKeyViolation as e: - raise GroupIdDoesNotExistError(gid=gid) from e + raise GroupIdDoesNotExistError(gid=permissions_gid) from e return _FolderID(folder_id) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 52baea189e2..807374d3ec5 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -3,7 +3,6 @@ # pylint:disable=unused-variable import itertools -import secrets from collections.abc import AsyncIterable, Awaitable, Callable from copy import deepcopy from typing import NamedTuple @@ -36,6 +35,7 @@ GroupIdDoesNotExistError, InsufficientPermissionsError, InvalidFolderNameError, + RootFolderRequiresAtLeastOnePrimaryGroupError, _FolderID, _FolderPermissions, _get_and_calsue_with_only_true_entries, @@ -377,7 +377,7 @@ async def _( connection, default_product_name, root.name, - root.gid, + {root.gid}, description=root.description, parent=parent, ) @@ -426,25 +426,117 @@ async def test_folder_create( missing_gid = 10202023302 await _assert_folder_entires(connection, folder_count=expected_folder_count) with pytest.raises(GroupIdDoesNotExistError): - await folder_create(connection, product_name, "f1", missing_gid) + await folder_create(connection, product_name, "f1", {missing_gid}) await _assert_folder_entires(connection, folder_count=expected_folder_count) # 2. create a folder and a subfolder of the same name - f1_folder_id = await folder_create(connection, product_name, "f1", owner_gid) + f1_folder_id = await folder_create(connection, product_name, "f1", {owner_gid}) expected_folder_count += 1 await _assert_folder_entires(connection, folder_count=expected_folder_count) await folder_create( - connection, product_name, "f1", owner_gid, parent=f1_folder_id + connection, product_name, "f1", {owner_gid}, parent=f1_folder_id ) expected_folder_count += 1 await _assert_folder_entires(connection, folder_count=expected_folder_count) # 3. inserting already existing folder fails with pytest.raises(FolderAlreadyExistsError): - await folder_create(connection, product_name, "f1", owner_gid) + await folder_create(connection, product_name, "f1", {owner_gid}) await _assert_folder_entires(connection, folder_count=expected_folder_count) +async def test_folder_create_shared_via_groups( + connection: SAConnection, + default_product_name: _ProductName, + get_unique_gids: Callable[[int], tuple[_GroupID, ...]], + make_folders: Callable[[set[MkFolder]], Awaitable[dict[str, _FolderID]]], + create_fake_group: Callable[..., Awaitable[RowProxy]], +): + ####### + # SETUP + ####### + gid_original_z43_owner: _GroupID + (gid_original_z43_owner,) = get_unique_gids(1) + + gid_user: _GroupID = ( + await create_fake_group(connection, type=GroupType.PRIMARY) + ).gid + gid_everyone: _GroupID | None = await connection.scalar( + sa.select([groups.c.gid]).where(groups.c.type == GroupType.EVERYONE) + ) + assert gid_everyone + gid_z43: _GroupID = ( + await create_fake_group(connection, type=GroupType.STANDARD) + ).gid + + folder_ids = await make_folders( + { + MkFolder( + name="root", + gid=gid_original_z43_owner, + shared_with={ + gid_z43: FolderAccessRole.OWNER, + gid_everyone: FolderAccessRole.OWNER, + }, + ), + } + ) + + folder_id_root = folder_ids["root"] + + ####### + # TESTS + ####### + + async def _assert(folder_id: _FolderID, gids: set[_GroupID]) -> None: + result = await connection.execute( + folders_access_rights.select() + .where(folders_access_rights.c.folder_id == folder_id) + .where(folders_access_rights.c.gid.in_(gids)) + ) + rows = await result.fetchall() + assert rows is not None + assert len(rows) == 1 + + # 1. can create when using one gid with permissions + folder_id_f1 = await folder_create( + connection, + default_product_name, + "f1", + {gid_z43, gid_user}, + parent=folder_id_root, + ) + await _assert(folder_id_f1, {gid_z43}) + + folder_id_f2 = await folder_create( + connection, + default_product_name, + "f2", + {gid_everyone, gid_user}, + parent=folder_id_root, + ) + await _assert(folder_id_f2, {gid_everyone}) + + # 2. can create new folder when using both gids with permissions + folder_id_f3 = await folder_create( + connection, + default_product_name, + "f3", + {gid_z43, gid_everyone, gid_user}, + parent=folder_id_root, + ) + await _assert(folder_id_f3, {gid_everyone, gid_z43}) + + # 3. cannot create a root folder without a primary group + with pytest.raises(RootFolderRequiresAtLeastOnePrimaryGroupError): + await folder_create( + connection, + default_product_name, + "folder_in_root", + {gid_z43, gid_everyone}, + ) + + async def test__get_resolved_access_rights( connection: SAConnection, get_unique_gids: Callable[[int], tuple[_GroupID, ...]], @@ -597,7 +689,7 @@ async def test_folder_share_or_update_permissions( await _assert_folder_entires(connection, folder_count=0) # 2. share existing folder with all possible roles - folder_id = await folder_create(connection, default_product_name, "f1", gid_owner) + folder_id = await folder_create(connection, default_product_name, "f1", {gid_owner}) await _assert_folder_entires(connection, folder_count=1) await _assert_folder_permissions( connection, folder_id=folder_id, gid=gid_owner, role=FolderAccessRole.OWNER @@ -731,7 +823,7 @@ async def test_folder_update( await _assert_folder_entires(connection, folder_count=0) # 2. owner updates created fodler - folder_id = await folder_create(connection, default_product_name, "f1", owner_gid) + folder_id = await folder_create(connection, default_product_name, "f1", {owner_gid}) await _assert_folder_entires(connection, folder_count=1) await _assert_name_and_description(connection, folder_id, name="f1", description="") @@ -859,14 +951,14 @@ async def test_folder_delete( await _assert_folder_entires(connection, folder_count=0) # 2. owner deletes folder - folder_id = await folder_create(connection, default_product_name, "f1", owner_gid) + folder_id = await folder_create(connection, default_product_name, "f1", {owner_gid}) await _assert_folder_entires(connection, folder_count=1) await folder_delete(connection, default_product_name, folder_id, {owner_gid}) await _assert_folder_entires(connection, folder_count=0) # 3. other owners can delete the folder - folder_id = await folder_create(connection, default_product_name, "f1", owner_gid) + folder_id = await folder_create(connection, default_product_name, "f1", {owner_gid}) await _assert_folder_entires(connection, folder_count=1) await folder_share_or_update_permissions( @@ -882,7 +974,7 @@ async def test_folder_delete( await _assert_folder_entires(connection, folder_count=0) # 4. non owner users cannot delete the folder - folder_id = await folder_create(connection, default_product_name, "f1", owner_gid) + folder_id = await folder_create(connection, default_product_name, "f1", {owner_gid}) await _assert_folder_entires(connection, folder_count=1) await folder_share_or_update_permissions( @@ -964,12 +1056,12 @@ async def _setup_folders() -> _FolderID: folder_id_root_folder = folder_ids["root_folder"] await _assert_folder_entires(connection, folder_count=1, access_rights_count=6) - GIDS_WITH_CREATE_PERMISSIONS: tuple[_GroupID, ...] = ( + GIDS_WITH_CREATE_PERMISSIONS: set[_GroupID] = { gid_owner_a, gid_owner_b, gid_editor_a, gid_editor_b, - ) + } previous_folder_id = folder_id_root_folder for i in range(100): @@ -977,7 +1069,7 @@ async def _setup_folders() -> _FolderID: connection, default_product_name, f"f{i}", - secrets.choice(GIDS_WITH_CREATE_PERMISSIONS), + GIDS_WITH_CREATE_PERMISSIONS, parent=previous_folder_id, ) await _assert_folder_entires( From 45a36b90afc199c7306b49dbcd024beb13934337 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 11:26:19 +0200 Subject: [PATCH 13/35] refacotr tests --- .../tests/test_utils_folders.py | 35 ++++++++++++------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 807374d3ec5..f8f22813c31 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -32,7 +32,6 @@ FolderEntry, FolderNotFoundError, FolderNotSharedWithGidError, - GroupIdDoesNotExistError, InsufficientPermissionsError, InvalidFolderNameError, RootFolderRequiresAtLeastOnePrimaryGroupError, @@ -425,8 +424,8 @@ async def test_folder_create( # 1. when GID is missing no entries should be present missing_gid = 10202023302 await _assert_folder_entires(connection, folder_count=expected_folder_count) - with pytest.raises(GroupIdDoesNotExistError): - await folder_create(connection, product_name, "f1", {missing_gid}) + # with pytest.raises(GroupIdDoesNotExistError): + # await folder_create(connection, product_name, "f1", {missing_gid}) await _assert_folder_entires(connection, folder_count=expected_folder_count) # 2. create a folder and a subfolder of the same name @@ -455,8 +454,8 @@ async def test_folder_create_shared_via_groups( ####### # SETUP ####### - gid_original_z43_owner: _GroupID - (gid_original_z43_owner,) = get_unique_gids(1) + gid_original_owner: _GroupID + (gid_original_owner,) = get_unique_gids(1) gid_user: _GroupID = ( await create_fake_group(connection, type=GroupType.PRIMARY) @@ -473,7 +472,7 @@ async def test_folder_create_shared_via_groups( { MkFolder( name="root", - gid=gid_original_z43_owner, + gid=gid_original_owner, shared_with={ gid_z43: FolderAccessRole.OWNER, gid_everyone: FolderAccessRole.OWNER, @@ -1513,8 +1512,8 @@ async def test_move_group_non_standard_groups_raise_error( ####### # SETUP ####### - gid_sharing: _GroupID - (gid_sharing,) = get_unique_gids(1) + gid_original_owner: _GroupID + (gid_original_owner,) = get_unique_gids(1) gid_primary: _GroupID = ( await create_fake_group(connection, type=GroupType.PRIMARY) ).gid @@ -1530,16 +1529,28 @@ async def test_move_group_non_standard_groups_raise_error( { MkFolder( name="SHARING_USER", - gid=gid_sharing, + gid=gid_original_owner, shared_with={ gid_primary: FolderAccessRole.EDITOR, gid_everyone: FolderAccessRole.EDITOR, gid_standard: FolderAccessRole.EDITOR, }, ), - MkFolder(name="PRIMARY", gid=gid_primary), - MkFolder(name="EVERYONE", gid=gid_everyone), - MkFolder(name="STANDARD", gid=gid_standard), + MkFolder( + name="PRIMARY", + gid=gid_original_owner, + shared_with={gid_primary: FolderAccessRole.OWNER}, + ), + MkFolder( + name="EVERYONE", + gid=gid_original_owner, + shared_with={gid_everyone: FolderAccessRole.OWNER}, + ), + MkFolder( + name="STANDARD", + gid=gid_original_owner, + shared_with={gid_standard: FolderAccessRole.OWNER}, + ), } ) From 6579042f50931716a42fcd20a8a11883464aa742 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 11:44:16 +0200 Subject: [PATCH 14/35] fixed missing group error --- .../utils_folders.py | 66 ++++++++++--------- .../tests/test_utils_folders.py | 5 +- 2 files changed, 39 insertions(+), 32 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index d9b7b3c29ef..0e7731bf4c3 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -10,7 +10,6 @@ import sqlalchemy as sa from aiopg.sa.connection import SAConnection from aiopg.sa.result import RowProxy -from psycopg2.errors import ForeignKeyViolation from pydantic import ( BaseModel, ConstrainedStr, @@ -109,8 +108,8 @@ class CouldNotCreateFolderError(BaseCreateFolderError): msg_template = "Could not create folder='{folder}' and parent='{parent}'" -class GroupIdDoesNotExistError(BaseCreateFolderError): - msg_template = "Provided group id '{gid}' does not exist " +class GroupIDsDoNotExistError(BaseCreateFolderError): + msg_template = "Any of the folloing gids='{gids}' do not exist" class RootFolderRequiresAtLeastOnePrimaryGroupError(BaseCreateFolderError): @@ -574,11 +573,21 @@ async def folder_create( permissions_gid = resolved_access_rights.gid if permissions_gid is None: - primary_gid: _GroupID | None = await connection.scalar( - sa.select([groups.c.gid]) - .where(groups.c.gid.in_(gids)) - .where(groups.c.type == GroupType.PRIMARY) - ) + groups_results: list[RowProxy] | None = await ( + await connection.execute( + sa.select([groups.c.gid, groups.c.type]).where( + groups.c.gid.in_(gids) + ) + ) + ).fetchall() + + if not groups_results: + raise GroupIDsDoNotExistError(gids=gids) + + primary_gid: _GroupID | None = None + for group in groups_results: + if group["type"] == GroupType.PRIMARY: + primary_gid = group["gid"] if primary_gid is None: raise RootFolderRequiresAtLeastOnePrimaryGroupError( parent=parent, gids=gids @@ -587,32 +596,29 @@ async def folder_create( permissions_gid = primary_gid # folder entry can now be inserted - try: - folder_id = await connection.scalar( - sa.insert(folders) - .values( - name=name, - description=description, - created_by=permissions_gid, - product_name=product_name, - ) - .returning(folders.c.id) + folder_id = await connection.scalar( + sa.insert(folders) + .values( + name=name, + description=description, + created_by=permissions_gid, + product_name=product_name, ) + .returning(folders.c.id) + ) - if not folder_id: - raise CouldNotCreateFolderError(folder=name, parent=parent) + if not folder_id: + raise CouldNotCreateFolderError(folder=name, parent=parent) - await connection.execute( - sa.insert(folders_access_rights).values( - folder_id=folder_id, - gid=permissions_gid, - traversal_parent_id=parent, - original_parent_id=parent, - **OWNER_PERMISSIONS.to_dict(), - ) + await connection.execute( + sa.insert(folders_access_rights).values( + folder_id=folder_id, + gid=permissions_gid, + traversal_parent_id=parent, + original_parent_id=parent, + **OWNER_PERMISSIONS.to_dict(), ) - except ForeignKeyViolation as e: - raise GroupIdDoesNotExistError(gid=permissions_gid) from e + ) return _FolderID(folder_id) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index f8f22813c31..32a8c92c5e0 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -32,6 +32,7 @@ FolderEntry, FolderNotFoundError, FolderNotSharedWithGidError, + GroupIDsDoNotExistError, InsufficientPermissionsError, InvalidFolderNameError, RootFolderRequiresAtLeastOnePrimaryGroupError, @@ -424,8 +425,8 @@ async def test_folder_create( # 1. when GID is missing no entries should be present missing_gid = 10202023302 await _assert_folder_entires(connection, folder_count=expected_folder_count) - # with pytest.raises(GroupIdDoesNotExistError): - # await folder_create(connection, product_name, "f1", {missing_gid}) + with pytest.raises(GroupIDsDoNotExistError): + await folder_create(connection, product_name, "f1", {missing_gid}) await _assert_folder_entires(connection, folder_count=expected_folder_count) # 2. create a folder and a subfolder of the same name From 4e69e51951d94c4f719ea37a7f5dc4290b581691 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 11:47:13 +0200 Subject: [PATCH 15/35] refactor --- .../tests/test_utils_folders.py | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 32a8c92c5e0..289c6de85e1 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -238,6 +238,19 @@ async def _query_table(table_name: sa.Table, count: NonNegativeInt) -> None: await _query_table(folders_access_rights, access_rights_count or folder_count) +async def _assert_folderpermissions_exists( + connection: SAConnection, folder_id: _FolderID, gids: set[_GroupID] +) -> None: + result = await connection.execute( + folders_access_rights.select() + .where(folders_access_rights.c.folder_id == folder_id) + .where(folders_access_rights.c.gid.in_(gids)) + ) + rows = await result.fetchall() + assert rows is not None + assert len(rows) == 1 + + async def _assert_folder_permissions( connection: SAConnection, *, @@ -488,16 +501,6 @@ async def test_folder_create_shared_via_groups( # TESTS ####### - async def _assert(folder_id: _FolderID, gids: set[_GroupID]) -> None: - result = await connection.execute( - folders_access_rights.select() - .where(folders_access_rights.c.folder_id == folder_id) - .where(folders_access_rights.c.gid.in_(gids)) - ) - rows = await result.fetchall() - assert rows is not None - assert len(rows) == 1 - # 1. can create when using one gid with permissions folder_id_f1 = await folder_create( connection, @@ -506,7 +509,7 @@ async def _assert(folder_id: _FolderID, gids: set[_GroupID]) -> None: {gid_z43, gid_user}, parent=folder_id_root, ) - await _assert(folder_id_f1, {gid_z43}) + await _assert_folderpermissions_exists(connection, folder_id_f1, {gid_z43}) folder_id_f2 = await folder_create( connection, @@ -515,7 +518,7 @@ async def _assert(folder_id: _FolderID, gids: set[_GroupID]) -> None: {gid_everyone, gid_user}, parent=folder_id_root, ) - await _assert(folder_id_f2, {gid_everyone}) + await _assert_folderpermissions_exists(connection, folder_id_f2, {gid_everyone}) # 2. can create new folder when using both gids with permissions folder_id_f3 = await folder_create( @@ -525,7 +528,9 @@ async def _assert(folder_id: _FolderID, gids: set[_GroupID]) -> None: {gid_z43, gid_everyone, gid_user}, parent=folder_id_root, ) - await _assert(folder_id_f3, {gid_everyone, gid_z43}) + await _assert_folderpermissions_exists( + connection, folder_id_f3, {gid_everyone, gid_z43} + ) # 3. cannot create a root folder without a primary group with pytest.raises(RootFolderRequiresAtLeastOnePrimaryGroupError): @@ -2162,6 +2167,10 @@ async def test_folder_list_multiple_entries_via_different_groups( connection, default_product_name, None, {gid_z43} ) + ####### + # TESTS + ####### + assert len(entries_z43) == 3 entries_osparc = await _list_folder_as( connection, default_product_name, None, {gid_osparc} From 42922b32923429db18a5b67ee1b5d4c8a510313e Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 7 Aug 2024 11:53:39 +0200 Subject: [PATCH 16/35] refactor test --- .../tests/test_utils_folders.py | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 289c6de85e1..42ff568443b 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -2120,7 +2120,7 @@ async def test_folder_list_shared_with_different_permissions( ) -async def test_folder_list_multiple_entries_via_different_groups( +async def test_folder_list_in_root_with_different_groups_avoids_duplicate_entries( connection: SAConnection, default_product_name: _ProductName, get_unique_gids: Callable[[int], tuple[_GroupID, ...]], @@ -2132,7 +2132,7 @@ async def test_folder_list_multiple_entries_via_different_groups( (gid_z43, gid_osparc, gid_user) = get_unique_gids(3) - folder_ids = await make_folders( + await make_folders( { MkFolder( name="f1", @@ -2159,32 +2159,25 @@ async def test_folder_list_multiple_entries_via_different_groups( } ) - folder_id_f1 = folder_ids["f1"] - folder_id_f2 = folder_ids["f2"] - folder_id_f3 = folder_ids["f3"] - - entries_z43 = await _list_folder_as( - connection, default_product_name, None, {gid_z43} - ) - ####### # TESTS ####### - assert len(entries_z43) == 3 - entries_osparc = await _list_folder_as( - connection, default_product_name, None, {gid_osparc} - ) - assert len(entries_osparc) == 3 + # 1. gid_z43 and gid_osparc see all folders + for gid_all_folders in (gid_z43, gid_osparc): + entries_z43 = await _list_folder_as( + connection, default_product_name, None, {gid_all_folders} + ) + assert len(entries_z43) == 3 + + # 2. gid_user only sees it's own folder entries_user = await _list_folder_as( connection, default_product_name, None, {gid_user} ) assert len(entries_user) == 1 + # 3. all gids see all fodlers entries_all_groups = await _list_folder_as( connection, default_product_name, None, {gid_z43, gid_osparc, gid_user} ) assert len(entries_all_groups) == 3 - - # TODO: continue this test make sure it works as expected - # assert False From 5a3b095a75515e9b2f3cca432c6788042c1f01ad Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Mon, 19 Aug 2024 11:49:54 +0200 Subject: [PATCH 17/35] removed transaction --- .../utils_folders.py | 51 +++++++++---------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 6a79699533b..961bc10c010 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -1151,38 +1151,33 @@ async def folder_get( _BasePermissions.GET_FOLDER ), ) -> FolderEntry: - async with connection.begin(): - resolved_access_rights: _ResolvedAccessRights = await _check_folder_and_access( - connection, - product_name, - folder_id=folder_id, - gids=gids, - permissions=required_permissions, - enforece_all_permissions=False, - ) - permissions_gid: _GroupID = resolved_access_rights.gid + resolved_access_rights: _ResolvedAccessRights = await _check_folder_and_access( + connection, + product_name, + folder_id=folder_id, + gids=gids, + permissions=required_permissions, + enforece_all_permissions=False, + ) + permissions_gid: _GroupID = resolved_access_rights.gid - query = ( - sa.select(*_LIST_SELECT_FIELDS) - .join( - folders_access_rights, folders.c.id == folders_access_rights.c.folder_id - ) - .where(folders_access_rights.c.folder_id == folder_id) - .where(folders_access_rights.c.gid == permissions_gid) - .where( - _get_and_calsue_with_only_true_entries( - required_permissions, folders_access_rights - ) - if folder_id is None - else True + query = ( + sa.select(*_LIST_SELECT_FIELDS) + .join(folders_access_rights, folders.c.id == folders_access_rights.c.folder_id) + .where(folders_access_rights.c.folder_id == folder_id) + .where(folders_access_rights.c.gid == permissions_gid) + .where( + _get_and_calsue_with_only_true_entries( + required_permissions, folders_access_rights ) - .where(folders.c.product_name == product_name) - .group_by(*_LIST_GROUP_BY_FIELDS) + if folder_id is None + else True ) + .where(folders.c.product_name == product_name) + .group_by(*_LIST_GROUP_BY_FIELDS) + ) - query_result: RowProxy | None = await ( - await connection.execute(query) - ).fetchone() + query_result: RowProxy | None = await (await connection.execute(query)).fetchone() if query_result is None: raise FolderNotFoundError( From 280b9b42615788760b14b5b7555021a0490d9ba6 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Mon, 19 Aug 2024 13:56:42 +0200 Subject: [PATCH 18/35] added regression test and fixed issue --- .../utils_folders.py | 1 + .../tests/test_utils_folders.py | 52 +++++++++++++++++++ .../folders/_folders_api.py | 14 ++--- 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 961bc10c010..926b4e5f30c 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -1030,6 +1030,7 @@ async def folder_remove_project( folders.c.name, folders.c.description, folders.c.created_by, + folders_access_rights.c.traversal_parent_id, ) _LIST_SELECT_FIELDS: Final[tuple[Label | Column, ...]] = ( *_LIST_GROUP_BY_FIELDS, diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 2f103799ddb..52548f024be 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -2185,6 +2185,58 @@ async def test_folder_list_in_root_with_different_groups_avoids_duplicate_entrie assert len(entries_all_groups) == 3 +async def test_regression_list_folder_parent( + connection: SAConnection, + default_product_name: _ProductName, + get_unique_gids: Callable[[int], tuple[_GroupID, ...]], + make_folders: Callable[[set[MkFolder]], Awaitable[dict[str, _FolderID]]], +): + ####### + # SETUP + ####### + + (gid_user,) = get_unique_gids(1) + + folder_ids = await make_folders( + { + MkFolder( + name="f1", + gid=gid_user, + children={ + MkFolder( + name="f2", + gid=gid_user, + children={ + MkFolder(name="f3", gid=gid_user), + }, + ) + }, + ), + } + ) + + folder_id_f1 = folder_ids["f1"] + folder_id_f2 = folder_ids["f2"] + folder_id_f3 = folder_ids["f3"] + + ####### + # TESTS + ####### + + for folder_id in (None, folder_id_f1, folder_id_f2): + folder_content = await _list_folder_as( + connection, default_product_name, folder_id, {gid_user} + ) + assert len(folder_content) == 1 + assert folder_content[0] + assert folder_content[0].parent_folder == folder_id + + f3_content = await _list_folder_as( + connection, default_product_name, folder_id_f3, {gid_user} + ) + assert len(f3_content) == 0 + + async def test_folder_get( connection: SAConnection, default_product_name: _ProductName, diff --git a/services/web/server/src/simcore_service_webserver/folders/_folders_api.py b/services/web/server/src/simcore_service_webserver/folders/_folders_api.py index 8c0b9de3b5c..ca9d0ce9899 100644 --- a/services/web/server/src/simcore_service_webserver/folders/_folders_api.py +++ b/services/web/server/src/simcore_service_webserver/folders/_folders_api.py @@ -37,7 +37,7 @@ async def create_folder( connection, product_name=product_name, name=folder_name, - gid=user["primary_gid"], + gids={user["primary_gid"]}, description=description if description else "", parent=parent_folder_id, ) @@ -45,7 +45,7 @@ async def create_folder( connection, product_name=product_name, folder_id=folder_id, - gid=user["primary_gid"], + gids={user["primary_gid"]}, ) return FolderGet( folder_id=folder_db.id, @@ -80,7 +80,7 @@ async def get_folder( connection, product_name=product_name, folder_id=folder_id, - gid=user["primary_gid"], + gids={user["primary_gid"]}, ) return FolderGet( folder_id=folder_db.id, @@ -118,7 +118,7 @@ async def list_folders( connection, product_name=product_name, folder_id=folder_id, - gid=user["primary_gid"], + gids={user["primary_gid"]}, offset=offset, limit=limit, order_by=cast(folders_db.OrderByDict, order_by.dict()), @@ -167,7 +167,7 @@ async def update_folder( connection, product_name=product_name, folder_id=folder_id, - gid=user["primary_gid"], + gids={user["primary_gid"]}, name=name, description=description, ) @@ -175,7 +175,7 @@ async def update_folder( connection, product_name=product_name, folder_id=folder_id, - gid=user["primary_gid"], + gids={user["primary_gid"]}, ) return FolderGet( folder_id=folder_db.id, @@ -210,5 +210,5 @@ async def delete_folder( connection, product_name=product_name, folder_id=folder_id, - gid=user["primary_gid"], + gids={user["primary_gid"]}, ) From 8df9a6a7bf97047f39424bf7b501805e618fe01e Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Mon, 19 Aug 2024 15:23:16 +0200 Subject: [PATCH 19/35] fixed api calls --- .../src/simcore_service_webserver/projects/_folders_api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/web/server/src/simcore_service_webserver/projects/_folders_api.py b/services/web/server/src/simcore_service_webserver/projects/_folders_api.py index 8a13f7f46b8..8a3a1539253 100644 --- a/services/web/server/src/simcore_service_webserver/projects/_folders_api.py +++ b/services/web/server/src/simcore_service_webserver/projects/_folders_api.py @@ -52,7 +52,7 @@ async def replace_project_folder( connection, product_name=product_name, folder_id=folder_id, - gid=user["primary_gid"], + gids={user["primary_gid"]}, project_uuid=project_id, ) return @@ -62,7 +62,7 @@ async def replace_project_folder( connection, product_name=product_name, source_folder_id=_source_folder_id, - gid=user["primary_gid"], + gids={user["primary_gid"]}, project_uuid=project_id, destination_folder_id=folder_id, ) From 4b23c5189cff17be00dbf5e4dc58e14c84192986 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 09:22:40 +0200 Subject: [PATCH 20/35] using SQL alchemy 2.0 compatible queries --- .../src/simcore_postgres_database/utils_folders.py | 10 ++++------ packages/postgres-database/tests/test_utils_folders.py | 10 +++++----- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 926b4e5f30c..35c814b9c8b 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -488,7 +488,7 @@ async def _check_folder_access( UnexpectedFolderAccessError """ folder_entry: int | None = await connection.scalar( - sa.select([folders.c.id]) + sa.select(folders.c.id) .where(folders.c.id == folder_id) .where(folders.c.product_name == product_name) ) @@ -557,7 +557,7 @@ async def folder_create( async with connection.begin(): entry_exists: int | None = await connection.scalar( - sa.select([folders.c.id]) + sa.select(folders.c.id) .select_from( folders.join( folders_access_rights, @@ -591,9 +591,7 @@ async def folder_create( if permissions_gid is None: groups_results: list[RowProxy] | None = await ( await connection.execute( - sa.select([groups.c.gid, groups.c.type]).where( - groups.c.gid.in_(gids) - ) + sa.select(groups.c.gid, groups.c.type).where(groups.c.gid.in_(gids)) ) ).fetchall() @@ -819,7 +817,7 @@ async def folder_move( source_access_gid = source_access_entry.gid group_type: GroupType | None = await connection.scalar( - sa.select([groups.c.type]).where(groups.c.gid == source_access_gid) + sa.select(groups.c.type).where(groups.c.gid == source_access_gid) ) # Might drop primary check if group_type is None or group_type != GroupType.PRIMARY: diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 52548f024be..8b557a89976 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -260,7 +260,7 @@ async def _assert_folder_permissions( role: FolderAccessRole, ) -> None: result = await connection.execute( - sa.select([folders_access_rights.c.folder_id]) + sa.select(folders_access_rights.c.folder_id) .where(folders_access_rights.c.folder_id == folder_id) .where(folders_access_rights.c.gid == gid) .where( @@ -282,7 +282,7 @@ async def _assert_name_and_description( description: str, ): async with connection.execute( - sa.select([folders.c.name, folders.c.description]).where( + sa.select(folders.c.name, folders.c.description).where( folders.c.id == folder_id ) ) as result_proxy: @@ -476,7 +476,7 @@ async def test_folder_create_shared_via_groups( await create_fake_group(connection, type=GroupType.PRIMARY) ).gid gid_everyone: _GroupID | None = await connection.scalar( - sa.select([groups.c.gid]).where(groups.c.type == GroupType.EVERYONE) + sa.select(groups.c.gid).where(groups.c.type == GroupType.EVERYONE) ) assert gid_everyone gid_z43: _GroupID = ( @@ -1267,7 +1267,7 @@ async def _assert_folder_permissions( parent_folder: _FolderID, ) -> None: result = await connection.execute( - sa.select([folders_access_rights.c.folder_id]) + sa.select(folders_access_rights.c.folder_id) .where(folders_access_rights.c.folder_id == folder_id) .where(folders_access_rights.c.gid == gid) .where(folders_access_rights.c.traversal_parent_id == parent_folder) @@ -1525,7 +1525,7 @@ async def test_move_group_non_standard_groups_raise_error( await create_fake_group(connection, type=GroupType.PRIMARY) ).gid gid_everyone: _GroupID | None = await connection.scalar( - sa.select([groups.c.gid]).where(groups.c.type == GroupType.EVERYONE) + sa.select(groups.c.gid).where(groups.c.type == GroupType.EVERYONE) ) assert gid_everyone gid_standard: _GroupID = ( From 61a3dc9b3c9667f193d6123b1895d25a340d6343 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 09:23:43 +0200 Subject: [PATCH 21/35] wrongly catalogued expection --- .../src/simcore_postgres_database/utils_folders.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 35c814b9c8b..6d33952b24b 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -85,7 +85,7 @@ class InsufficientPermissionsError(FolderAccessError): msg_template = "could not find a parent for folder_id={folder_id} and gid={gid}, with permissions={permissions}" -class UnexpectedFolderAccessError(FoldersError): +class UnexpectedFolderAccessError(FolderAccessError): msg_template = ( "Unexpected None value for resolved_access_rights={resolved_access_rights} and last_exception={last_exception}." "Called with: product_name={product_name}, folder_id={folder_id}, gids={gids}" From 85b932ad84799bcf5572ba3cadbaa797d0c18631 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 09:25:37 +0200 Subject: [PATCH 22/35] corrected raised errors --- .../src/simcore_postgres_database/utils_folders.py | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 6d33952b24b..132e30bbc23 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -485,7 +485,6 @@ async def _check_folder_access( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError - UnexpectedFolderAccessError """ folder_entry: int | None = await connection.scalar( sa.select(folders.c.id) From 1dc29ac4eef28809d12c2d5adba629e57b36b6fd Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 09:41:14 +0200 Subject: [PATCH 23/35] remove remaning warnings --- .../utils_folders.py | 54 +++++++++---------- 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 132e30bbc23..197a58dfc46 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -345,24 +345,6 @@ async def _get_resolved_access_rights( # Define the anchor CTE access_rights_cte = ( sa.select( - [ - folders_access_rights.c.folder_id, - folders_access_rights.c.gid, - folders_access_rights.c.traversal_parent_id, - folders_access_rights.c.original_parent_id, - folders_access_rights.c.read, - folders_access_rights.c.write, - folders_access_rights.c.delete, - sa.literal_column("0").label("level"), - ] - ) - .where(folders_access_rights.c.folder_id == sa.bindparam("start_folder_id")) - .cte(name="access_rights_cte", recursive=True) - ) - - # Define the recursive part of the CTE - recursive = sa.select( - [ folders_access_rights.c.folder_id, folders_access_rights.c.gid, folders_access_rights.c.traversal_parent_id, @@ -370,8 +352,22 @@ async def _get_resolved_access_rights( folders_access_rights.c.read, folders_access_rights.c.write, folders_access_rights.c.delete, - sa.literal_column("access_rights_cte.level + 1").label("level"), - ] + sa.literal_column("0").label("level"), + ) + .where(folders_access_rights.c.folder_id == sa.bindparam("start_folder_id")) + .cte(name="access_rights_cte", recursive=True) + ) + + # Define the recursive part of the CTE + recursive = sa.select( + folders_access_rights.c.folder_id, + folders_access_rights.c.gid, + folders_access_rights.c.traversal_parent_id, + folders_access_rights.c.original_parent_id, + folders_access_rights.c.read, + folders_access_rights.c.write, + folders_access_rights.c.delete, + sa.literal_column("access_rights_cte.level + 1").label("level"), ).select_from( folders_access_rights.join( access_rights_cte, @@ -398,16 +394,14 @@ def _get_permissions_where_clause() -> ColumnElement | bool: # Final query to filter and order results query = ( sa.select( - [ - folder_hierarchy.c.folder_id, - folder_hierarchy.c.gid, - folder_hierarchy.c.traversal_parent_id, - folder_hierarchy.c.original_parent_id, - folder_hierarchy.c.read, - folder_hierarchy.c.write, - folder_hierarchy.c.delete, - folder_hierarchy.c.level, - ] + folder_hierarchy.c.folder_id, + folder_hierarchy.c.gid, + folder_hierarchy.c.traversal_parent_id, + folder_hierarchy.c.original_parent_id, + folder_hierarchy.c.read, + folder_hierarchy.c.write, + folder_hierarchy.c.delete, + folder_hierarchy.c.level, ) .where(_get_permissions_where_clause()) .where(folder_hierarchy.c.original_parent_id.is_(None)) From d171198cb7f07c6280cab31ef2c73a260cb790bf Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 09:55:52 +0200 Subject: [PATCH 24/35] making more user friendly --- .../utils_folders.py | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 197a58dfc46..05a290236d7 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -1,3 +1,4 @@ +import logging import re import uuid from collections.abc import Iterable @@ -29,6 +30,8 @@ from .models.groups import GroupType, groups from .utils_ordering import OrderDirection +_logger = logging.getLogger(__name__) + _ProductName: TypeAlias = str _ProjectID: TypeAlias = uuid.UUID _GroupID: TypeAlias = PositiveInt @@ -86,10 +89,7 @@ class InsufficientPermissionsError(FolderAccessError): class UnexpectedFolderAccessError(FolderAccessError): - msg_template = ( - "Unexpected None value for resolved_access_rights={resolved_access_rights} and last_exception={last_exception}." - "Called with: product_name={product_name}, folder_id={folder_id}, gids={gids}" - ) + msg_template = "Could not detirmine folder access, please contact support." class BaseCreateFolderError(FoldersError): @@ -454,13 +454,20 @@ async def _check_folder_and_access( if last_exception: raise last_exception - raise UnexpectedFolderAccessError( - resolved_access_rights=resolved_access_rights, - last_exception=last_exception, - product_name=product_name, - folder_id=folder_id, - gids=gids, + _logger.warning( + ( + "Both resolved_access_rights=%s and last_exception=%s are None. " + "Function called with product_name=%s, folder_id=%s, gids=%s, enforece_all_permissions=%s. " + "TIP: 'gids' being empty is not acceptable, please check." + ), + resolved_access_rights, + last_exception, + product_name, + folder_id, + gids, + enforece_all_permissions, ) + raise UnexpectedFolderAccessError return resolved_access_rights From 0a83b6f6642b0d28514598bc2b56af35157f46cb Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 10:07:07 +0200 Subject: [PATCH 25/35] fix error messages and typo --- .../utils_folders.py | 52 +++++++++---------- .../tests/test_utils_folders.py | 6 +-- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 05a290236d7..20b3d289633 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -108,14 +108,14 @@ class CouldNotCreateFolderError(BaseCreateFolderError): msg_template = "Could not create folder='{folder}' and parent='{parent}'" -class GroupIDsDoNotExistError(BaseCreateFolderError): - msg_template = "Any of the folloing gids='{gids}' do not exist" +class NoGroupIDFoundError(BaseCreateFolderError): + msg_template = "None of the provided gids='{gids}' was found" class RootFolderRequiresAtLeastOnePrimaryGroupError(BaseCreateFolderError): msg_template = ( - "parent={parent} is None (please check) and gids={gids} did not contain a PRIMARY group. " - "Cannot create a folder isnide the 'root' directory." + "No parent={parent} defined and groupIDs={gids} did not contain a PRIMARY group. " + "Cannot create a folder isnide the 'root' wihtout using the user's group." ) @@ -339,7 +339,7 @@ async def _get_resolved_access_rights( gid: _GroupID, *, permissions: _FolderPermissions | None, - enforece_all_permissions: bool, + enforce_all_permissions: bool, ) -> _ResolvedAccessRights | None: # Define the anchor CTE @@ -387,7 +387,7 @@ def _get_permissions_where_clause() -> ColumnElement | bool: folder_hierarchy.c.write.is_(permissions.write), folder_hierarchy.c.delete.is_(permissions.delete), ) - if enforece_all_permissions + if enforce_all_permissions else _get_and_calsue_with_only_true_entries(permissions, folder_hierarchy) ) @@ -425,7 +425,7 @@ async def _check_folder_and_access( gids: set[_GroupID], *, permissions: _FolderPermissions, - enforece_all_permissions: bool, + enforce_all_permissions: bool, ) -> _ResolvedAccessRights: """ Raises: @@ -444,7 +444,7 @@ async def _check_folder_and_access( folder_id, gid, permissions=permissions, - enforece_all_permissions=enforece_all_permissions, + enforce_all_permissions=enforce_all_permissions, ) break except FolderAccessError as e: @@ -457,7 +457,7 @@ async def _check_folder_and_access( _logger.warning( ( "Both resolved_access_rights=%s and last_exception=%s are None. " - "Function called with product_name=%s, folder_id=%s, gids=%s, enforece_all_permissions=%s. " + "Function called with product_name=%s, folder_id=%s, gids=%s, enforce_all_permissions=%s. " "TIP: 'gids' being empty is not acceptable, please check." ), resolved_access_rights, @@ -465,7 +465,7 @@ async def _check_folder_and_access( product_name, folder_id, gids, - enforece_all_permissions, + enforce_all_permissions, ) raise UnexpectedFolderAccessError @@ -479,7 +479,7 @@ async def _check_folder_access( gid: _GroupID, *, permissions: _FolderPermissions, - enforece_all_permissions: bool, + enforce_all_permissions: bool, ) -> _ResolvedAccessRights: """ Raises: @@ -503,7 +503,7 @@ async def _check_folder_access( folder_id, gid, permissions=None, - enforece_all_permissions=False, + enforce_all_permissions=False, ) if not resolved_access_rights_without_permissions: raise FolderNotSharedWithGidError(folder_id=folder_id, gid=gid) @@ -514,7 +514,7 @@ async def _check_folder_access( folder_id, gid, permissions=permissions, - enforece_all_permissions=enforece_all_permissions, + enforce_all_permissions=enforce_all_permissions, ) if resolved_access_rights is None: raise InsufficientPermissionsError( @@ -584,7 +584,7 @@ async def folder_create( folder_id=parent, gids=gids, permissions=_required_permissions, - enforece_all_permissions=False, + enforce_all_permissions=False, ) permissions_gid = resolved_access_rights.gid @@ -596,7 +596,7 @@ async def folder_create( ).fetchall() if not groups_results: - raise GroupIDsDoNotExistError(gids=gids) + raise NoGroupIDFoundError(gids=gids) primary_gid: _GroupID | None = None for group in groups_results: @@ -664,7 +664,7 @@ async def folder_share_or_update_permissions( folder_id=folder_id, gids=sharing_gids, permissions=required_permissions, - enforece_all_permissions=False, + enforce_all_permissions=False, ) # update or create permissions entry @@ -715,7 +715,7 @@ async def folder_update( folder_id=folder_id, gids=gids, permissions=_required_permissions, - enforece_all_permissions=False, + enforce_all_permissions=False, ) # do not update if nothing changed @@ -760,7 +760,7 @@ async def folder_delete( folder_id=folder_id, gids=gids, permissions=_required_permissions, - enforece_all_permissions=False, + enforce_all_permissions=False, ) # list all children then delete @@ -812,7 +812,7 @@ async def folder_move( folder_id=source_folder_id, gids=gids, permissions=required_permissions_source, - enforece_all_permissions=False, + enforce_all_permissions=False, ) source_access_gid = source_access_entry.gid @@ -831,7 +831,7 @@ async def folder_move( folder_id=destination_folder_id, gids=gids, permissions=required_permissions_destination, - enforece_all_permissions=False, + enforce_all_permissions=False, ) # set new traversa_parent_id on the source_folder_id which is equal to destination_folder_id @@ -873,7 +873,7 @@ async def folder_add_project( folder_id=folder_id, gids=gids, permissions=required_permissions, - enforece_all_permissions=False, + enforce_all_permissions=False, ) # check if already added in folder @@ -926,7 +926,7 @@ async def folder_move_project( folder_id=source_folder_id, gids=gids, permissions=_required_permissions_source, - enforece_all_permissions=False, + enforce_all_permissions=False, ) if destination_folder_id is None: @@ -947,7 +947,7 @@ async def folder_move_project( folder_id=destination_folder_id, gids=gids, permissions=_required_permissions_destination, - enforece_all_permissions=False, + enforce_all_permissions=False, ) await connection.execute( @@ -1013,7 +1013,7 @@ async def folder_remove_project( folder_id=folder_id, gids=gids, permissions=required_permissions, - enforece_all_permissions=False, + enforce_all_permissions=False, ) await connection.execute( @@ -1098,7 +1098,7 @@ async def folder_list( folder_id=folder_id, gids=gids, permissions=required_permissions, - enforece_all_permissions=False, + enforce_all_permissions=False, ) results: list[FolderEntry] = [] @@ -1156,7 +1156,7 @@ async def folder_get( folder_id=folder_id, gids=gids, permissions=required_permissions, - enforece_all_permissions=False, + enforce_all_permissions=False, ) permissions_gid: _GroupID = resolved_access_rights.gid diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 8b557a89976..0efa1acf9f5 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -32,9 +32,9 @@ FolderEntry, FolderNotFoundError, FolderNotSharedWithGidError, - GroupIDsDoNotExistError, InsufficientPermissionsError, InvalidFolderNameError, + NoGroupIDFoundError, RootFolderRequiresAtLeastOnePrimaryGroupError, _FolderID, _FolderPermissions, @@ -439,7 +439,7 @@ async def test_folder_create( # 1. when GID is missing no entries should be present missing_gid = 10202023302 await _assert_folder_entires(connection, folder_count=expected_folder_count) - with pytest.raises(GroupIDsDoNotExistError): + with pytest.raises(NoGroupIDFoundError): await folder_create(connection, product_name, "f1", {missing_gid}) await _assert_folder_entires(connection, folder_count=expected_folder_count) @@ -613,7 +613,7 @@ async def _assert_resolves_to( # NOTE: this is the more restricitve case # and we test against exact user roles, # the APIs use only a subset of the permissions ususally set to True - enforece_all_permissions=True, + enforce_all_permissions=True, ) assert resolved_parent assert resolved_parent.folder_id == expected_folder_id From da24f243c86c9e22ed6109c4fedf0de7897f0c53 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 10:15:50 +0200 Subject: [PATCH 26/35] refactor unused parameter --- .../utils_folders.py | 28 ++----------------- .../tests/test_utils_folders.py | 2 +- 2 files changed, 4 insertions(+), 26 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 20b3d289633..28f1da168d2 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -339,7 +339,7 @@ async def _get_resolved_access_rights( gid: _GroupID, *, permissions: _FolderPermissions | None, - enforce_all_permissions: bool, + enforce_all_permissions: bool = False, ) -> _ResolvedAccessRights | None: # Define the anchor CTE @@ -425,7 +425,6 @@ async def _check_folder_and_access( gids: set[_GroupID], *, permissions: _FolderPermissions, - enforce_all_permissions: bool, ) -> _ResolvedAccessRights: """ Raises: @@ -439,12 +438,7 @@ async def _check_folder_and_access( for gid in gids: try: resolved_access_rights = await _check_folder_access( - connection, - product_name, - folder_id, - gid, - permissions=permissions, - enforce_all_permissions=enforce_all_permissions, + connection, product_name, folder_id, gid, permissions=permissions ) break except FolderAccessError as e: @@ -457,7 +451,7 @@ async def _check_folder_and_access( _logger.warning( ( "Both resolved_access_rights=%s and last_exception=%s are None. " - "Function called with product_name=%s, folder_id=%s, gids=%s, enforce_all_permissions=%s. " + "Function called with product_name=%s, folder_id=%s, gids=%s. " "TIP: 'gids' being empty is not acceptable, please check." ), resolved_access_rights, @@ -465,7 +459,6 @@ async def _check_folder_and_access( product_name, folder_id, gids, - enforce_all_permissions, ) raise UnexpectedFolderAccessError @@ -479,7 +472,6 @@ async def _check_folder_access( gid: _GroupID, *, permissions: _FolderPermissions, - enforce_all_permissions: bool, ) -> _ResolvedAccessRights: """ Raises: @@ -503,7 +495,6 @@ async def _check_folder_access( folder_id, gid, permissions=None, - enforce_all_permissions=False, ) if not resolved_access_rights_without_permissions: raise FolderNotSharedWithGidError(folder_id=folder_id, gid=gid) @@ -514,7 +505,6 @@ async def _check_folder_access( folder_id, gid, permissions=permissions, - enforce_all_permissions=enforce_all_permissions, ) if resolved_access_rights is None: raise InsufficientPermissionsError( @@ -584,7 +574,6 @@ async def folder_create( folder_id=parent, gids=gids, permissions=_required_permissions, - enforce_all_permissions=False, ) permissions_gid = resolved_access_rights.gid @@ -664,7 +653,6 @@ async def folder_share_or_update_permissions( folder_id=folder_id, gids=sharing_gids, permissions=required_permissions, - enforce_all_permissions=False, ) # update or create permissions entry @@ -715,7 +703,6 @@ async def folder_update( folder_id=folder_id, gids=gids, permissions=_required_permissions, - enforce_all_permissions=False, ) # do not update if nothing changed @@ -760,7 +747,6 @@ async def folder_delete( folder_id=folder_id, gids=gids, permissions=_required_permissions, - enforce_all_permissions=False, ) # list all children then delete @@ -812,7 +798,6 @@ async def folder_move( folder_id=source_folder_id, gids=gids, permissions=required_permissions_source, - enforce_all_permissions=False, ) source_access_gid = source_access_entry.gid @@ -831,7 +816,6 @@ async def folder_move( folder_id=destination_folder_id, gids=gids, permissions=required_permissions_destination, - enforce_all_permissions=False, ) # set new traversa_parent_id on the source_folder_id which is equal to destination_folder_id @@ -873,7 +857,6 @@ async def folder_add_project( folder_id=folder_id, gids=gids, permissions=required_permissions, - enforce_all_permissions=False, ) # check if already added in folder @@ -926,7 +909,6 @@ async def folder_move_project( folder_id=source_folder_id, gids=gids, permissions=_required_permissions_source, - enforce_all_permissions=False, ) if destination_folder_id is None: @@ -947,7 +929,6 @@ async def folder_move_project( folder_id=destination_folder_id, gids=gids, permissions=_required_permissions_destination, - enforce_all_permissions=False, ) await connection.execute( @@ -1013,7 +994,6 @@ async def folder_remove_project( folder_id=folder_id, gids=gids, permissions=required_permissions, - enforce_all_permissions=False, ) await connection.execute( @@ -1098,7 +1078,6 @@ async def folder_list( folder_id=folder_id, gids=gids, permissions=required_permissions, - enforce_all_permissions=False, ) results: list[FolderEntry] = [] @@ -1156,7 +1135,6 @@ async def folder_get( folder_id=folder_id, gids=gids, permissions=required_permissions, - enforce_all_permissions=False, ) permissions_gid: _GroupID = resolved_access_rights.gid diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 0efa1acf9f5..375a0d53d15 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -612,7 +612,7 @@ async def _assert_resolves_to( permissions=permissions, # NOTE: this is the more restricitve case # and we test against exact user roles, - # the APIs use only a subset of the permissions ususally set to True + # the APIs use only a subset of the permissions set to False enforce_all_permissions=True, ) assert resolved_parent From e21055cf5529fc483d6da7cc8126d49c0ca7191a Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 10:20:52 +0200 Subject: [PATCH 27/35] order --- .../utils_folders.py | 94 +++++++++---------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 28f1da168d2..26a31384ec6 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -418,53 +418,6 @@ def _get_permissions_where_clause() -> ColumnElement | bool: ) -async def _check_folder_and_access( - connection: SAConnection, - product_name: _ProductName, - folder_id: _FolderID, - gids: set[_GroupID], - *, - permissions: _FolderPermissions, -) -> _ResolvedAccessRights: - """ - Raises: - FolderNotFoundError - FolderNotSharedWithGidError - InsufficientPermissionsError - UnexpectedFolderAccessError - """ - resolved_access_rights: _ResolvedAccessRights | None = None - last_exception: Exception | None = None - for gid in gids: - try: - resolved_access_rights = await _check_folder_access( - connection, product_name, folder_id, gid, permissions=permissions - ) - break - except FolderAccessError as e: - last_exception = e - - if resolved_access_rights is None: - if last_exception: - raise last_exception - - _logger.warning( - ( - "Both resolved_access_rights=%s and last_exception=%s are None. " - "Function called with product_name=%s, folder_id=%s, gids=%s. " - "TIP: 'gids' being empty is not acceptable, please check." - ), - resolved_access_rights, - last_exception, - product_name, - folder_id, - gids, - ) - raise UnexpectedFolderAccessError - - return resolved_access_rights - - async def _check_folder_access( connection: SAConnection, product_name: _ProductName, @@ -516,6 +469,53 @@ async def _check_folder_access( return resolved_access_rights +async def _check_folder_and_access( + connection: SAConnection, + product_name: _ProductName, + folder_id: _FolderID, + gids: set[_GroupID], + *, + permissions: _FolderPermissions, +) -> _ResolvedAccessRights: + """ + Raises: + FolderNotFoundError + FolderNotSharedWithGidError + InsufficientPermissionsError + UnexpectedFolderAccessError + """ + resolved_access_rights: _ResolvedAccessRights | None = None + last_exception: Exception | None = None + for gid in gids: + try: + resolved_access_rights = await _check_folder_access( + connection, product_name, folder_id, gid, permissions=permissions + ) + break + except FolderAccessError as e: + last_exception = e + + if resolved_access_rights is None: + if last_exception: + raise last_exception + + _logger.warning( + ( + "Both resolved_access_rights=%s and last_exception=%s are None. " + "Function called with product_name=%s, folder_id=%s, gids=%s. " + "TIP: 'gids' being empty is not acceptable, please check." + ), + resolved_access_rights, + last_exception, + product_name, + folder_id, + gids, + ) + raise UnexpectedFolderAccessError + + return resolved_access_rights + + ### ### API DB LAYER ### From 90e033cf90b29032fd8b41ffced4f84133f3744b Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 10:38:35 +0200 Subject: [PATCH 28/35] rename --- .../utils_folders.py | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 26a31384ec6..d7c20cace33 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -418,7 +418,7 @@ def _get_permissions_where_clause() -> ColumnElement | bool: ) -async def _check_folder_access( +async def _check_and_get_folder_access_by_group( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, @@ -469,7 +469,7 @@ async def _check_folder_access( return resolved_access_rights -async def _check_folder_and_access( +async def _check_and_get_folder_access( connection: SAConnection, product_name: _ProductName, folder_id: _FolderID, @@ -488,7 +488,7 @@ async def _check_folder_and_access( last_exception: Exception | None = None for gid in gids: try: - resolved_access_rights = await _check_folder_access( + resolved_access_rights = await _check_and_get_folder_access_by_group( connection, product_name, folder_id, gid, permissions=permissions ) break @@ -568,7 +568,7 @@ async def folder_create( # - `is root folder, a.k.a. no parent?` taken from the user's primary group permissions_gid: _GroupID | None = None if parent: - resolved_access_rights = await _check_folder_and_access( + resolved_access_rights = await _check_and_get_folder_access( connection, product_name, folder_id=parent, @@ -647,7 +647,7 @@ async def folder_share_or_update_permissions( """ # NOTE: if the `sharing_gid`` has permissions to share it can share it with any `FolderAccessRole` async with connection.begin(): - await _check_folder_and_access( + await _check_and_get_folder_access( connection, product_name, folder_id=folder_id, @@ -697,7 +697,7 @@ async def folder_update( UnexpectedFolderAccessError """ async with connection.begin(): - await _check_folder_and_access( + await _check_and_get_folder_access( connection, product_name, folder_id=folder_id, @@ -741,7 +741,7 @@ async def folder_delete( childern_folder_ids: list[_FolderID] = [] async with connection.begin(): - await _check_folder_and_access( + await _check_and_get_folder_access( connection, product_name, folder_id=folder_id, @@ -792,7 +792,7 @@ async def folder_move( CannotMoveFolderSharedViaNonPrimaryGroupError: """ async with connection.begin(): - source_access_entry = await _check_folder_and_access( + source_access_entry = await _check_and_get_folder_access( connection, product_name, folder_id=source_folder_id, @@ -810,7 +810,7 @@ async def folder_move( group_type=group_type, gid=source_access_gid ) if destination_folder_id: - await _check_folder_and_access( + await _check_and_get_folder_access( connection, product_name, folder_id=destination_folder_id, @@ -851,7 +851,7 @@ async def folder_add_project( ProjectAlreadyExistsInFolderError """ async with connection.begin(): - await _check_folder_and_access( + await _check_and_get_folder_access( connection, product_name, folder_id=folder_id, @@ -903,7 +903,7 @@ async def folder_move_project( CannotMoveFolderSharedViaNonPrimaryGroupError: """ async with connection.begin(): - await _check_folder_and_access( + await _check_and_get_folder_access( connection, product_name, folder_id=source_folder_id, @@ -923,7 +923,7 @@ async def folder_move_project( return async with connection.begin(): - await _check_folder_and_access( + await _check_and_get_folder_access( connection, product_name, folder_id=destination_folder_id, @@ -988,7 +988,7 @@ async def folder_remove_project( UnexpectedFolderAccessError """ async with connection.begin(): - await _check_folder_and_access( + await _check_and_get_folder_access( connection, product_name, folder_id=folder_id, @@ -1072,7 +1072,7 @@ async def folder_list( # NOTE: when `folder_id is None` list the root folder of the `gids` if folder_id is not None: - await _check_folder_and_access( + await _check_and_get_folder_access( connection, product_name, folder_id=folder_id, @@ -1129,7 +1129,7 @@ async def folder_get( _BasePermissions.GET_FOLDER ), ) -> FolderEntry: - resolved_access_rights: _ResolvedAccessRights = await _check_folder_and_access( + resolved_access_rights: _ResolvedAccessRights = await _check_and_get_folder_access( connection, product_name, folder_id=folder_id, From 73c087ee4e8fdda394416bccbcecffcc7bd15826 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 11:42:31 +0200 Subject: [PATCH 29/35] simplify function --- .../utils_folders.py | 60 ++++++++----------- 1 file changed, 24 insertions(+), 36 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index d7c20cace33..e7f84e5a28b 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -50,7 +50,7 @@ * FolderNotFoundError * FolderNotSharedWithGidError * InsufficientPermissionsError - * UnexpectedFolderAccessError + * NoAccessForGroupsFoundError * BaseCreateFolderError * FolderAlreadyExistsError * ParentFolderIsNotWritableError @@ -88,8 +88,8 @@ class InsufficientPermissionsError(FolderAccessError): msg_template = "could not find a parent for folder_id={folder_id} and gid={gid}, with permissions={permissions}" -class UnexpectedFolderAccessError(FolderAccessError): - msg_template = "Could not detirmine folder access, please contact support." +class NoAccessForGroupsFoundError(FolderAccessError): + msg_template = "No parent found for folder_id={folder_id} and gids={gids}, with permissions={permissions}" class BaseCreateFolderError(FoldersError): @@ -482,38 +482,26 @@ async def _check_and_get_folder_access( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError - UnexpectedFolderAccessError + NoAccessForGroupsFoundError """ - resolved_access_rights: _ResolvedAccessRights | None = None - last_exception: Exception | None = None + folder_access_error: FolderAccessError | None = None + for gid in gids: try: - resolved_access_rights = await _check_and_get_folder_access_by_group( + return await _check_and_get_folder_access_by_group( connection, product_name, folder_id, gid, permissions=permissions ) - break - except FolderAccessError as e: - last_exception = e + except FolderAccessError as e: # noqa: PERF203 + folder_access_error = e - if resolved_access_rights is None: - if last_exception: - raise last_exception - - _logger.warning( - ( - "Both resolved_access_rights=%s and last_exception=%s are None. " - "Function called with product_name=%s, folder_id=%s, gids=%s. " - "TIP: 'gids' being empty is not acceptable, please check." - ), - resolved_access_rights, - last_exception, - product_name, - folder_id, - gids, - ) - raise UnexpectedFolderAccessError + if folder_access_error: + raise folder_access_error - return resolved_access_rights + raise NoAccessForGroupsFoundError( + folder_id=folder_id, + gids=gids, + permissions=_only_true_permissions(permissions), + ) ### @@ -537,7 +525,7 @@ async def folder_create( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError - UnexpectedFolderAccessError + NoAccessForGroupsFoundError FolderAlreadyExistsError CouldNotCreateFolderError GroupIdDoesNotExistError @@ -643,7 +631,7 @@ async def folder_share_or_update_permissions( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError - UnexpectedFolderAccessError + NoAccessForGroupsFoundError """ # NOTE: if the `sharing_gid`` has permissions to share it can share it with any `FolderAccessRole` async with connection.begin(): @@ -694,7 +682,7 @@ async def folder_update( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError - UnexpectedFolderAccessError + NoAccessForGroupsFoundError """ async with connection.begin(): await _check_and_get_folder_access( @@ -736,7 +724,7 @@ async def folder_delete( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError - UnexpectedFolderAccessError + NoAccessForGroupsFoundError """ childern_folder_ids: list[_FolderID] = [] @@ -788,7 +776,7 @@ async def folder_move( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError - UnexpectedFolderAccessError + NoAccessForGroupsFoundError CannotMoveFolderSharedViaNonPrimaryGroupError: """ async with connection.begin(): @@ -847,7 +835,7 @@ async def folder_add_project( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError - UnexpectedFolderAccessError + NoAccessForGroupsFoundError ProjectAlreadyExistsInFolderError """ async with connection.begin(): @@ -985,7 +973,7 @@ async def folder_remove_project( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError - UnexpectedFolderAccessError + NoAccessForGroupsFoundError """ async with connection.begin(): await _check_and_get_folder_access( @@ -1067,7 +1055,7 @@ async def folder_list( FolderNotFoundError FolderNotSharedWithGidError InsufficientPermissionsError - UnexpectedFolderAccessError + NoAccessForGroupsFoundError """ # NOTE: when `folder_id is None` list the root folder of the `gids` From 1f5c1bcc5e4b9962f028fbfac6093d924c60c3e8 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 11:48:07 +0200 Subject: [PATCH 30/35] review tips --- .../src/simcore_postgres_database/utils_folders.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index e7f84e5a28b..28037a326d1 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -97,7 +97,7 @@ class BaseCreateFolderError(FoldersError): class FolderAlreadyExistsError(BaseCreateFolderError): - msg_template = "A folder='{folder}' with parent='{parent}' for gids='{gids}' in product_name={product_name} already exists" + msg_template = "A folder='{folder}' with parent='{parent}' in product_name={product_name} already exists" class ParentFolderIsNotWritableError(BaseCreateFolderError): @@ -548,13 +548,13 @@ async def folder_create( ) if entry_exists: raise FolderAlreadyExistsError( - product_name=product_name, folder=name, parent=parent, gids=gids + product_name=product_name, folder=name, parent=parent ) - # `permissions_gid` is compted as follows: + # `permissions_gid` is computed as follows: # - `folder has a parent?` taken from the resolved access rights of the parent folder # - `is root folder, a.k.a. no parent?` taken from the user's primary group - permissions_gid: _GroupID | None = None + permissions_gid = None if parent: resolved_access_rights = await _check_and_get_folder_access( connection, @@ -575,7 +575,7 @@ async def folder_create( if not groups_results: raise NoGroupIDFoundError(gids=gids) - primary_gid: _GroupID | None = None + primary_gid = None for group in groups_results: if group["type"] == GroupType.PRIMARY: primary_gid = group["gid"] From fb7f7dd58c34918935101d8f15f7fba858f41f11 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 11:49:12 +0200 Subject: [PATCH 31/35] simplify --- .../src/simcore_postgres_database/utils_folders.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 28037a326d1..5c50083743e 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -484,7 +484,7 @@ async def _check_and_get_folder_access( InsufficientPermissionsError NoAccessForGroupsFoundError """ - folder_access_error: FolderAccessError | None = None + folder_access_error = None for gid in gids: try: From 5a9e6d87435deea7eb26d2d39e36be02d8a9d23b Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Tue, 20 Aug 2024 16:41:50 +0200 Subject: [PATCH 32/35] refactor error reporting --- .../utils_folders.py | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 5c50083743e..02f5a08283e 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -77,15 +77,15 @@ class FolderAccessError(FoldersError): class FolderNotFoundError(FolderAccessError): - msg_template = "no entry found for folder_id={folder_id}, gid={gid} and product_name={product_name}" + msg_template = "no entry found for folder_id={folder_id}, gids={gids} and product_name={product_name}" class FolderNotSharedWithGidError(FolderAccessError): - msg_template = "folder_id={folder_id} was not shared with gid={gid}" + msg_template = "folder_id={folder_id} was not shared with gids={gids}" class InsufficientPermissionsError(FolderAccessError): - msg_template = "could not find a parent for folder_id={folder_id} and gid={gid}, with permissions={permissions}" + msg_template = "could not find a parent for folder_id={folder_id} and gids={gids}, with permissions={permissions}" class NoAccessForGroupsFoundError(FolderAccessError): @@ -424,6 +424,7 @@ async def _check_and_get_folder_access_by_group( folder_id: _FolderID, gid: _GroupID, *, + error_reporting_gids: set[_GroupID], permissions: _FolderPermissions, ) -> _ResolvedAccessRights: """ @@ -439,7 +440,7 @@ async def _check_and_get_folder_access_by_group( ) if not folder_entry: raise FolderNotFoundError( - folder_id=folder_id, gid=gid, product_name=product_name + folder_id=folder_id, gids=error_reporting_gids, product_name=product_name ) # check if folder was shared @@ -450,7 +451,9 @@ async def _check_and_get_folder_access_by_group( permissions=None, ) if not resolved_access_rights_without_permissions: - raise FolderNotSharedWithGidError(folder_id=folder_id, gid=gid) + raise FolderNotSharedWithGidError( + folder_id=folder_id, gids=error_reporting_gids + ) # check if there are permissions resolved_access_rights = await _get_resolved_access_rights( @@ -462,7 +465,7 @@ async def _check_and_get_folder_access_by_group( if resolved_access_rights is None: raise InsufficientPermissionsError( folder_id=folder_id, - gid=gid, + gids=error_reporting_gids, permissions=_only_true_permissions(permissions), ) @@ -489,7 +492,12 @@ async def _check_and_get_folder_access( for gid in gids: try: return await _check_and_get_folder_access_by_group( - connection, product_name, folder_id, gid, permissions=permissions + connection, + product_name, + folder_id, + gid, + error_reporting_gids=gids, + permissions=permissions, ) except FolderAccessError as e: # noqa: PERF203 folder_access_error = e From 00e7ece6e6f96d7aecef8019be1e8fb0c480d295 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 21 Aug 2024 09:11:23 +0200 Subject: [PATCH 33/35] naming --- .../utils_folders.py | 21 +++++++++--------- .../tests/test_utils_folders.py | 22 +++++++------------ 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 02f5a08283e..87e677ba8ba 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -25,6 +25,7 @@ from sqlalchemy.dialects import postgresql from sqlalchemy.dialects.postgresql import BOOLEAN, INTEGER from sqlalchemy.sql.elements import ColumnElement, Label +from sqlalchemy.sql.selectable import CTE from .models.folders import folders, folders_access_rights, folders_to_projects from .models.groups import GroupType, groups @@ -275,8 +276,8 @@ def _requires(*permissions: _FolderPermissions) -> _FolderPermissions: return _or_dicts_list(permissions) -def _get_and_calsue_with_only_true_entries( - permissions: _FolderPermissions, table +def _get_filter_for_enabled_permissions( + permissions: _FolderPermissions, table: sa.Table | CTE ) -> ColumnElement | bool: clauses: list[ColumnElement] = [] @@ -339,7 +340,7 @@ async def _get_resolved_access_rights( gid: _GroupID, *, permissions: _FolderPermissions | None, - enforce_all_permissions: bool = False, + only_enabled_permissions: bool = True, ) -> _ResolvedAccessRights | None: # Define the anchor CTE @@ -378,17 +379,17 @@ async def _get_resolved_access_rights( # Combine anchor and recursive CTE folder_hierarchy = access_rights_cte.union_all(recursive) - def _get_permissions_where_clause() -> ColumnElement | bool: + def _get_permissions_filter() -> ColumnElement | bool: if not permissions: return True return ( - sa.and_( + _get_filter_for_enabled_permissions(permissions, folder_hierarchy) + if only_enabled_permissions + else sa.and_( folder_hierarchy.c.read.is_(permissions.read), folder_hierarchy.c.write.is_(permissions.write), folder_hierarchy.c.delete.is_(permissions.delete), ) - if enforce_all_permissions - else _get_and_calsue_with_only_true_entries(permissions, folder_hierarchy) ) # Final query to filter and order results @@ -403,7 +404,7 @@ def _get_permissions_where_clause() -> ColumnElement | bool: folder_hierarchy.c.delete, folder_hierarchy.c.level, ) - .where(_get_permissions_where_clause()) + .where(_get_permissions_filter()) .where(folder_hierarchy.c.original_parent_id.is_(None)) .where(folder_hierarchy.c.gid == gid) .order_by(folder_hierarchy.c.level.asc()) @@ -1089,7 +1090,7 @@ async def folder_list( ) .where(folders_access_rights.c.gid.in_(gids)) .where( - _get_and_calsue_with_only_true_entries( + _get_filter_for_enabled_permissions( required_permissions, folders_access_rights ) ) @@ -1140,7 +1141,7 @@ async def folder_get( .where(folders_access_rights.c.folder_id == folder_id) .where(folders_access_rights.c.gid == permissions_gid) .where( - _get_and_calsue_with_only_true_entries( + _get_filter_for_enabled_permissions( required_permissions, folders_access_rights ) if folder_id is None diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 375a0d53d15..62bd9956d99 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -38,7 +38,7 @@ RootFolderRequiresAtLeastOnePrimaryGroupError, _FolderID, _FolderPermissions, - _get_and_calsue_with_only_true_entries, + _get_filter_for_enabled_permissions, _get_permissions_from_role, _get_resolved_access_rights, _GroupID, @@ -197,25 +197,19 @@ async def test_folder_create_wrong_folder_name(invalid_name: str): def test__get_where_clause(): assert isinstance( - _get_and_calsue_with_only_true_entries( - VIEWER_PERMISSIONS, folders_access_rights - ), + _get_filter_for_enabled_permissions(VIEWER_PERMISSIONS, folders_access_rights), ColumnElement, ) assert isinstance( - _get_and_calsue_with_only_true_entries( - EDITOR_PERMISSIONS, folders_access_rights - ), + _get_filter_for_enabled_permissions(EDITOR_PERMISSIONS, folders_access_rights), ColumnElement, ) assert isinstance( - _get_and_calsue_with_only_true_entries( - OWNER_PERMISSIONS, folders_access_rights - ), + _get_filter_for_enabled_permissions(OWNER_PERMISSIONS, folders_access_rights), ColumnElement, ) assert isinstance( - _get_and_calsue_with_only_true_entries( + _get_filter_for_enabled_permissions( _FolderPermissions(read=False, write=False, delete=False), folders_access_rights, ), @@ -264,7 +258,7 @@ async def _assert_folder_permissions( .where(folders_access_rights.c.folder_id == folder_id) .where(folders_access_rights.c.gid == gid) .where( - _get_and_calsue_with_only_true_entries( + _get_filter_for_enabled_permissions( _get_permissions_from_role(role), folders_access_rights ) ) @@ -612,8 +606,8 @@ async def _assert_resolves_to( permissions=permissions, # NOTE: this is the more restricitve case # and we test against exact user roles, - # the APIs use only a subset of the permissions set to False - enforce_all_permissions=True, + # the APIs use only a subset of the permissions set to True + only_enabled_permissions=False, ) assert resolved_parent assert resolved_parent.folder_id == expected_folder_id From f7c9fc15690a374e66c7ae08f2de2671ebebf699 Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 21 Aug 2024 09:17:49 +0200 Subject: [PATCH 34/35] dropped parameter used only in tests --- .../utils_folders.py | 22 +++++-------------- .../tests/test_utils_folders.py | 4 ---- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 87e677ba8ba..15ae70993fc 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -340,7 +340,6 @@ async def _get_resolved_access_rights( gid: _GroupID, *, permissions: _FolderPermissions | None, - only_enabled_permissions: bool = True, ) -> _ResolvedAccessRights | None: # Define the anchor CTE @@ -377,20 +376,7 @@ async def _get_resolved_access_rights( ) # Combine anchor and recursive CTE - folder_hierarchy = access_rights_cte.union_all(recursive) - - def _get_permissions_filter() -> ColumnElement | bool: - if not permissions: - return True - return ( - _get_filter_for_enabled_permissions(permissions, folder_hierarchy) - if only_enabled_permissions - else sa.and_( - folder_hierarchy.c.read.is_(permissions.read), - folder_hierarchy.c.write.is_(permissions.write), - folder_hierarchy.c.delete.is_(permissions.delete), - ) - ) + folder_hierarchy: CTE = access_rights_cte.union_all(recursive) # Final query to filter and order results query = ( @@ -404,7 +390,11 @@ def _get_permissions_filter() -> ColumnElement | bool: folder_hierarchy.c.delete, folder_hierarchy.c.level, ) - .where(_get_permissions_filter()) + .where( + True + if not permissions + else _get_filter_for_enabled_permissions(permissions, folder_hierarchy) + ) .where(folder_hierarchy.c.original_parent_id.is_(None)) .where(folder_hierarchy.c.gid == gid) .order_by(folder_hierarchy.c.level.asc()) diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 62bd9956d99..9bfc1d4f04e 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -604,10 +604,6 @@ async def _assert_resolves_to( target_folder_id, gid, permissions=permissions, - # NOTE: this is the more restricitve case - # and we test against exact user roles, - # the APIs use only a subset of the permissions set to True - only_enabled_permissions=False, ) assert resolved_parent assert resolved_parent.folder_id == expected_folder_id From f6902d3e200cdb4f3fc7f5f034197643b5ed77bc Mon Sep 17 00:00:00 2001 From: Andrei Neagu Date: Wed, 21 Aug 2024 09:52:11 +0200 Subject: [PATCH 35/35] refactor error handling --- .../src/simcore_postgres_database/utils_folders.py | 9 +++++---- packages/postgres-database/tests/test_utils_folders.py | 4 ++-- .../folders/_folders_handlers.py | 4 ++++ 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py index 15ae70993fc..e0f59cdcfd2 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_folders.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_folders.py @@ -1,4 +1,3 @@ -import logging import re import uuid from collections.abc import Iterable @@ -17,6 +16,7 @@ Field, NonNegativeInt, PositiveInt, + ValidationError, parse_obj_as, ) from pydantic.errors import PydanticErrorMixin @@ -31,8 +31,6 @@ from .models.groups import GroupType, groups from .utils_ordering import OrderDirection -_logger = logging.getLogger(__name__) - _ProductName: TypeAlias = str _ProjectID: TypeAlias = uuid.UUID _GroupID: TypeAlias = PositiveInt @@ -530,7 +528,10 @@ async def folder_create( GroupIdDoesNotExistError RootFolderRequiresAtLeastOnePrimaryGroupError """ - parse_obj_as(FolderName, name) + try: + parse_obj_as(FolderName, name) + except ValidationError as exc: + raise InvalidFolderNameError(name=name, reason=f"{exc}") from exc async with connection.begin(): entry_exists: int | None = await connection.scalar( diff --git a/packages/postgres-database/tests/test_utils_folders.py b/packages/postgres-database/tests/test_utils_folders.py index 9bfc1d4f04e..8c49fd9914f 100644 --- a/packages/postgres-database/tests/test_utils_folders.py +++ b/packages/postgres-database/tests/test_utils_folders.py @@ -12,7 +12,7 @@ import sqlalchemy as sa from aiopg.sa.connection import SAConnection from aiopg.sa.result import RowProxy -from pydantic import BaseModel, Field, NonNegativeInt, ValidationError +from pydantic import BaseModel, Field, NonNegativeInt from pytest_simcore.helpers.faker_factories import random_product from simcore_postgres_database.models.folders import ( folders, @@ -191,7 +191,7 @@ async def default_product_name( ], ) async def test_folder_create_wrong_folder_name(invalid_name: str): - with pytest.raises((InvalidFolderNameError, ValidationError)): + with pytest.raises(InvalidFolderNameError): await folder_create(Mock(), "mock_product", invalid_name, Mock()) diff --git a/services/web/server/src/simcore_service_webserver/folders/_folders_handlers.py b/services/web/server/src/simcore_service_webserver/folders/_folders_handlers.py index ed8f6108df6..1e67d43c6d8 100644 --- a/services/web/server/src/simcore_service_webserver/folders/_folders_handlers.py +++ b/services/web/server/src/simcore_service_webserver/folders/_folders_handlers.py @@ -27,6 +27,7 @@ from servicelib.mimetype_constants import MIMETYPE_APPLICATION_JSON from servicelib.request_keys import RQT_USERID_KEY from servicelib.rest_constants import RESPONSE_MODEL_POLICY +from simcore_postgres_database.utils_folders import FoldersError from .._constants import RQ_PRODUCT_KEY from .._meta import API_VTAG as VTAG @@ -51,6 +52,9 @@ async def wrapper(request: web.Request) -> web.StreamResponse: except FolderAccessForbiddenError as exc: raise web.HTTPForbidden(reason=f"{exc}") from exc + except FoldersError as exc: + raise web.HTTPBadRequest(reason=f"{exc}") from exc + return wrapper