diff --git a/packages/postgres-database/src/simcore_postgres_database/migration/versions/8a742f3efdd9_new_tags_priority_column.py b/packages/postgres-database/src/simcore_postgres_database/migration/versions/8a742f3efdd9_new_tags_priority_column.py new file mode 100644 index 00000000000..abede70a281 --- /dev/null +++ b/packages/postgres-database/src/simcore_postgres_database/migration/versions/8a742f3efdd9_new_tags_priority_column.py @@ -0,0 +1,27 @@ +"""new tags priority column + +Revision ID: 8a742f3efdd9 +Revises: 10729e07000d +Create Date: 2024-10-02 15:23:27.446241+00:00 + +""" +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "8a742f3efdd9" +down_revision = "10729e07000d" +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column("tags", sa.Column("priority", sa.Integer(), nullable=True)) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column("tags", "priority") + # ### end Alembic commands ### diff --git a/packages/postgres-database/src/simcore_postgres_database/models/services_tags.py b/packages/postgres-database/src/simcore_postgres_database/models/services_tags.py index 6a3ea828eea..c774cdcd317 100644 --- a/packages/postgres-database/src/simcore_postgres_database/models/services_tags.py +++ b/packages/postgres-database/src/simcore_postgres_database/models/services_tags.py @@ -14,13 +14,13 @@ "service_key", sa.String, nullable=False, - doc="Service Key Identifier", + doc="Key name identifier for the service, without specifiying its versions", ), sa.Column( "service_version", sa.String, nullable=False, - doc="Service version", + doc="Version of the service. Combined with 'service_key', it forms a unique identifier for this service.", ), # Tag sa.Column( @@ -28,6 +28,7 @@ sa.BigInteger, sa.ForeignKey(tags.c.id, onupdate="CASCADE", ondelete="CASCADE"), nullable=False, + doc="Identifier of the tag assigned to this specific service (service_key, service_version).", ), # Constraints sa.ForeignKeyConstraint( diff --git a/packages/postgres-database/src/simcore_postgres_database/models/tags.py b/packages/postgres-database/src/simcore_postgres_database/models/tags.py index ce05e68f198..da7c788e02d 100644 --- a/packages/postgres-database/src/simcore_postgres_database/models/tags.py +++ b/packages/postgres-database/src/simcore_postgres_database/models/tags.py @@ -2,11 +2,11 @@ from .base import metadata -# -# tags: a way to mark any entity (e.g. a project, ...) -# this can be used to perform operations as filter, select, compare, etc -# tags = sa.Table( + # + # A way to mark any entity (e.g. a project, ...) + # this can be used to perform operations as filter, select, compare, etc + # "tags", metadata, sa.Column( @@ -14,23 +14,30 @@ sa.BigInteger(), nullable=False, primary_key=True, + doc="Unique identifier for each tag.", ), - sa.Column( - "name", - sa.String(), - nullable=False, - doc="display name", - ), + sa.Column("name", sa.String(), nullable=False, doc="The display name of the tag."), sa.Column( "description", sa.String(), nullable=True, - doc="description displayed", + doc="A brief description displayed for the tag.", ), sa.Column( "color", sa.String(), nullable=False, - doc="Hex color (see https://www.color-hex.com/)", + doc="Hexadecimal color code representing the tag (e.g., #FF5733).", + ), + sa.Column( + "priority", + sa.Integer(), + nullable=True, + doc=( + "Explicit ordering priority when displaying tags. " + "Tags with a lower value are displayed first. " + "If NULL, tags are considered to have the lowest priority and " + "are displayed after non-NULL values, ordered by their ID (reflecting creation order)." + ), ), ) diff --git a/packages/postgres-database/src/simcore_postgres_database/models/tags_access_rights.py b/packages/postgres-database/src/simcore_postgres_database/models/tags_access_rights.py index 9efb4123f0d..9078a9254f1 100644 --- a/packages/postgres-database/src/simcore_postgres_database/models/tags_access_rights.py +++ b/packages/postgres-database/src/simcore_postgres_database/models/tags_access_rights.py @@ -22,7 +22,7 @@ name="fk_tag_to_group_tag_id", ), nullable=False, - doc="Tag unique ID", + doc="References the unique identifier of the tag that these access rights apply to.", ), sa.Column( "group_id", @@ -34,7 +34,7 @@ name="fk_tag_to_group_group_id", ), nullable=False, - doc="Group unique ID", + doc="References the unique identifier of the group that has access rights to the tag.", ), # ACCESS RIGHTS --- sa.Column( @@ -42,22 +42,24 @@ sa.Boolean(), nullable=False, server_default=sa.sql.expression.true(), - doc="If true, group can *read* a tag." - "This column can be used to set the tag invisible", + doc="Indicates whether the group has permission to view the tag. " + "A value of 'True' allows the group to access the tag's details.", ), sa.Column( "write", sa.Boolean(), nullable=False, server_default=sa.sql.expression.false(), - doc="If true, group can *create* and *update* a tag", + doc="Indicates whether the group has permission to modify the tag. " + "A value of 'True' grants write access to the group.", ), sa.Column( "delete", sa.Boolean(), nullable=False, server_default=sa.sql.expression.false(), - doc="If true, group can *delete* the tag", + doc="Indicates whether the group has permission to delete the tag. " + "A value of 'True' allows the group to remove the tag.", ), # TIME STAMPS ---- column_created_datetime(timezone=False), diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_tags.py b/packages/postgres-database/src/simcore_postgres_database/utils_tags.py index 0a8b3e4ac28..7421f25de0f 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_tags.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_tags.py @@ -1,14 +1,13 @@ """ Repository pattern, errors and data structures for models.tags """ -import itertools -from dataclasses import dataclass from typing import TypedDict -from aiopg.sa.connection import SAConnection +from sqlalchemy.ext.asyncio import AsyncConnection, AsyncEngine +from .utils_repos import pass_or_acquire_connection, transaction_context from .utils_tags_sql import ( - count_users_with_access_rights_stmt, + count_groups_with_given_access_rights_stmt, create_tag_stmt, delete_tag_stmt, get_tag_stmt, @@ -49,15 +48,16 @@ class TagDict(TypedDict, total=True): delete: bool -@dataclass(frozen=True) class TagsRepo: - user_id: int # Determines access-rights + def __init__(self, engine: AsyncEngine): + self.engine = engine async def access_count( self, - conn: SAConnection, - tag_id: int, + connection: AsyncConnection | None = None, *, + user_id: int, + tag_id: int, read: bool | None = None, write: bool | None = None, delete: bool | None = None, @@ -66,11 +66,12 @@ async def access_count( Returns 0 if tag does not match access Returns >0 if it does and represents the number of groups granting this access to the user """ - count_stmt = count_users_with_access_rights_stmt( - user_id=self.user_id, tag_id=tag_id, read=read, write=write, delete=delete - ) - permissions_count: int | None = await conn.scalar(count_stmt) - return permissions_count if permissions_count else 0 + async with pass_or_acquire_connection(self.engine, connection) as conn: + count_stmt = count_groups_with_given_access_rights_stmt( + user_id=user_id, tag_id=tag_id, read=read, write=write, delete=delete + ) + permissions_count: int | None = await conn.scalar(count_stmt) + return permissions_count if permissions_count else 0 # # CRUD operations @@ -78,85 +79,187 @@ async def access_count( async def create( self, - conn: SAConnection, + connection: AsyncConnection | None = None, *, + user_id: int, name: str, color: str, description: str | None = None, # =nullable read: bool = True, write: bool = True, delete: bool = True, + priority: int | None = None, ) -> TagDict: - values = { + """Creates tag and defaults to full access rights to `user_id`""" + values: dict[str, str | int] = { "name": name, "color": color, } if description: values["description"] = description + if priority is not None: + values["priority"] = priority - async with conn.begin(): + async with transaction_context(self.engine, connection) as conn: # insert new tag insert_stmt = create_tag_stmt(**values) result = await conn.execute(insert_stmt) - tag = await result.first() + tag = result.first() assert tag # nosec # take tag ownership access_stmt = set_tag_access_rights_stmt( tag_id=tag.id, - user_id=self.user_id, + user_id=user_id, read=read, write=write, delete=delete, ) result = await conn.execute(access_stmt) - access = await result.first() - assert access + access = result.first() + assert access # nosec - return TagDict(itertools.chain(tag.items(), access.items())) # type: ignore + return TagDict( + id=tag.id, + name=tag.name, + description=tag.description, + color=tag.color, + read=access.read, + write=access.write, + delete=access.delete, + ) - async def list_all(self, conn: SAConnection) -> list[TagDict]: - stmt_list = list_tags_stmt(user_id=self.user_id) - return [TagDict(row.items()) async for row in conn.execute(stmt_list)] # type: ignore + async def list_all( + self, + connection: AsyncConnection | None = None, + *, + user_id: int, + ) -> list[TagDict]: + async with pass_or_acquire_connection(self.engine, connection) as conn: + stmt_list = list_tags_stmt(user_id=user_id) + result = await conn.stream(stmt_list) + return [ + TagDict( + id=row.id, + name=row.name, + description=row.description, + color=row.color, + read=row.read, + write=row.write, + delete=row.delete, + ) + async for row in result + ] - async def get(self, conn: SAConnection, tag_id: int) -> TagDict: - stmt_get = get_tag_stmt(user_id=self.user_id, tag_id=tag_id) - result = await conn.execute(stmt_get) - row = await result.first() - if not row: - msg = f"{tag_id=} not found: either no access or does not exists" - raise TagNotFoundError(msg) - return TagDict(row.items()) # type: ignore + async def get( + self, + connection: AsyncConnection | None = None, + *, + user_id: int, + tag_id: int, + ) -> TagDict: + stmt_get = get_tag_stmt(user_id=user_id, tag_id=tag_id) + async with pass_or_acquire_connection(self.engine, connection) as conn: + result = await conn.execute(stmt_get) + row = result.first() + if not row: + msg = f"{tag_id=} not found: either no access or does not exists" + raise TagNotFoundError(msg) + return TagDict( + id=row.id, + name=row.name, + description=row.description, + color=row.color, + read=row.read, + write=row.write, + delete=row.delete, + ) async def update( self, - conn: SAConnection, + connection: AsyncConnection | None = None, + *, + user_id: int, tag_id: int, **fields, ) -> TagDict: - updates = { - name: value - for name, value in fields.items() - if name in {"name", "color", "description"} - } + async with transaction_context(self.engine, connection) as conn: + updates = { + name: value + for name, value in fields.items() + if name in {"name", "color", "description", "priority"} + } - if not updates: - # no updates == get - return await self.get(conn, tag_id=tag_id) + if not updates: + # no updates == get + return await self.get(conn, user_id=user_id, tag_id=tag_id) - update_stmt = update_tag_stmt(user_id=self.user_id, tag_id=tag_id, **updates) - result = await conn.execute(update_stmt) - row = await result.first() - if not row: - msg = f"{tag_id=} not updated: either no access or not found" - raise TagOperationNotAllowedError(msg) + update_stmt = update_tag_stmt(user_id=user_id, tag_id=tag_id, **updates) + result = await conn.execute(update_stmt) + row = result.first() + if not row: + msg = f"{tag_id=} not updated: either no access or not found" + raise TagOperationNotAllowedError(msg) - return TagDict(row.items()) # type: ignore + return TagDict( + id=row.id, + name=row.name, + description=row.description, + color=row.color, + read=row.read, + write=row.write, + delete=row.delete, + ) - async def delete(self, conn: SAConnection, tag_id: int) -> None: - stmt_delete = delete_tag_stmt(user_id=self.user_id, tag_id=tag_id) + async def delete( + self, + connection: AsyncConnection | None = None, + *, + user_id: int, + tag_id: int, + ) -> None: + stmt_delete = delete_tag_stmt(user_id=user_id, tag_id=tag_id) + async with transaction_context(self.engine, connection) as conn: + deleted = await conn.scalar(stmt_delete) + if not deleted: + msg = f"Could not delete {tag_id=}. Not found or insuficient access." + raise TagOperationNotAllowedError(msg) + + # + # ACCESS RIGHTS + # + + async def create_access_rights( + self, + connection: AsyncConnection | None = None, + *, + user_id: int, + tag_id: int, + group_id: int, + read: bool, + write: bool, + delete: bool, + ): + raise NotImplementedError - deleted = await conn.scalar(stmt_delete) - if not deleted: - msg = f"Could not delete {tag_id=}. Not found or insuficient access." - raise TagOperationNotAllowedError(msg) + async def update_access_rights( + self, + connection: AsyncConnection | None = None, + *, + user_id: int, + tag_id: int, + group_id: int, + read: bool, + write: bool, + delete: bool, + ): + raise NotImplementedError + + async def delete_access_rights( + self, + connection: AsyncConnection | None = None, + *, + user_id: int, + tag_id: int, + ): + raise NotImplementedError diff --git a/packages/postgres-database/src/simcore_postgres_database/utils_tags_sql.py b/packages/postgres-database/src/simcore_postgres_database/utils_tags_sql.py index 05a1e93ca33..bd727a0dcc3 100644 --- a/packages/postgres-database/src/simcore_postgres_database/utils_tags_sql.py +++ b/packages/postgres-database/src/simcore_postgres_database/utils_tags_sql.py @@ -23,9 +23,6 @@ ] -_COLUMNS = _TAG_COLUMNS + _ACCESS_RIGHTS_COLUMNS - - def _join_user_groups_tag(*, access_condition, tag_id: int, user_id: int): return user_to_groups.join( tags_access_rights, @@ -57,24 +54,42 @@ def get_tag_stmt( user_id: int, tag_id: int, ): - return sa.select(*_COLUMNS).select_from( - _join_user_to_given_tag( - access_condition=tags_access_rights.c.read.is_(True), - tag_id=tag_id, - user_id=user_id, + return ( + sa.select( + *_TAG_COLUMNS, + # aggregation ensures MOST PERMISSIVE policy of access-rights + sa.func.bool_or(tags_access_rights.c.read).label("read"), + sa.func.bool_or(tags_access_rights.c.write).label("write"), + sa.func.bool_or(tags_access_rights.c.delete).label("delete") + ) + .select_from( + _join_user_to_given_tag( + access_condition=tags_access_rights.c.read.is_(True), + tag_id=tag_id, + user_id=user_id, + ) ) + .group_by(tags.c.id) ) def list_tags_stmt(*, user_id: int): return ( - sa.select(*_COLUMNS) + sa.select( + *_TAG_COLUMNS, + # aggregation ensures MOST PERMISSIVE policy of access-rights + sa.func.bool_or(tags_access_rights.c.read).label("read"), + sa.func.bool_or(tags_access_rights.c.write).label("write"), + sa.func.bool_or(tags_access_rights.c.delete).label("delete") + ) .select_from( _join_user_to_tags( access_condition=tags_access_rights.c.read.is_(True), user_id=user_id, ) ) + .group_by(tags.c.id) # makes it tag.id uniqueness + .order_by(tags.c.priority.nulls_last()) .order_by(tags.c.id) ) @@ -83,7 +98,7 @@ def create_tag_stmt(**values): return tags.insert().values(**values).returning(*_TAG_COLUMNS) -def count_users_with_access_rights_stmt( +def count_groups_with_given_access_rights_stmt( *, user_id: int, tag_id: int, @@ -92,7 +107,7 @@ def count_users_with_access_rights_stmt( delete: bool | None ): """ - How many users are given these access permissions + How many groups (from this user_id) are given EXACTLY these access permissions """ access = [] if read is not None: @@ -146,7 +161,7 @@ def update_tag_stmt(*, user_id: int, tag_id: int, **updates): & (user_to_groups.c.uid == user_id) ) .values(**updates) - .returning(*_COLUMNS) + .returning(*_TAG_COLUMNS, *_ACCESS_RIGHTS_COLUMNS) ) @@ -166,6 +181,11 @@ def delete_tag_stmt(*, user_id: int, tag_id: int): ) +# +# PROJECT TAGS +# + + def get_tags_for_project_stmt(*, project_index: int): return sa.select(projects_tags.c.tag_id).where( projects_tags.c.project_id == project_index @@ -183,6 +203,11 @@ def add_tag_to_project_stmt(*, project_index: int, tag_id: int): ) +# +# SERVICE TAGS +# + + def get_tags_for_services_stmt(*, key: str, version: str): return sa.select(services_tags.c.tag_id).where( (services_tags.c.service_key == key) diff --git a/packages/postgres-database/tests/test_utils_tags.py b/packages/postgres-database/tests/test_utils_tags.py index 2b99c1939fe..26f9a301f76 100644 --- a/packages/postgres-database/tests/test_utils_tags.py +++ b/packages/postgres-database/tests/test_utils_tags.py @@ -27,9 +27,11 @@ get_tag_stmt, get_tags_for_project_stmt, get_tags_for_services_stmt, + list_tags_stmt, set_tag_access_rights_stmt, update_tag_stmt, ) +from sqlalchemy.ext.asyncio import AsyncEngine @pytest.fixture @@ -75,7 +77,11 @@ async def other_user( async def test_tags_access_with_primary_groups( - connection: SAConnection, user: RowProxy, group: RowProxy, other_user: RowProxy + asyncpg_engine: AsyncEngine, + connection: SAConnection, + user: RowProxy, + group: RowProxy, + other_user: RowProxy, ): conn = connection @@ -102,22 +108,29 @@ async def test_tags_access_with_primary_groups( ), ] - tags_repo = TagsRepo(user_id=user.id) + tags_repo = TagsRepo(asyncpg_engine) # repo has access assert ( - await tags_repo.access_count(conn, tag_id, read=True, write=True, delete=True) + await tags_repo.access_count( + user_id=user.id, tag_id=tag_id, read=True, write=True, delete=True + ) + == 1 + ) + assert ( + await tags_repo.access_count( + user_id=user.id, tag_id=tag_id, read=True, write=True + ) == 1 ) - assert await tags_repo.access_count(conn, tag_id, read=True, write=True) == 1 - assert await tags_repo.access_count(conn, tag_id, read=True) == 1 - assert await tags_repo.access_count(conn, tag_id, write=True) == 1 + assert await tags_repo.access_count(user_id=user.id, tag_id=tag_id, read=True) == 1 + assert await tags_repo.access_count(user_id=user.id, tag_id=tag_id, write=True) == 1 # changing access conditions assert ( await tags_repo.access_count( - conn, - tag_id, + user_id=user.id, + tag_id=tag_id, read=True, write=True, delete=False, # <--- @@ -128,15 +141,20 @@ async def test_tags_access_with_primary_groups( # user will have NO access to other user's tags even matching access rights assert ( await tags_repo.access_count( - conn, other_tag_id, read=True, write=True, delete=True + user_id=user.id, tag_id=other_tag_id, read=True, write=True, delete=True ) == 0 ) async def test_tags_access_with_multiple_groups( - connection: SAConnection, user: RowProxy, group: RowProxy, other_user: RowProxy + asyncpg_engine: AsyncEngine, + connection: SAConnection, + user: RowProxy, + group: RowProxy, + other_user: RowProxy, ): + conn = connection (tag_id, other_tag_id, group_tag_id, everyone_tag_id) = [ @@ -182,30 +200,58 @@ async def test_tags_access_with_multiple_groups( ), ] - tags_repo = TagsRepo(user_id=user.id) - other_repo = TagsRepo(user_id=other_user.id) + tags_repo = TagsRepo(asyncpg_engine) + other_repo = TagsRepo(asyncpg_engine) # tag_id assert ( - await tags_repo.access_count(conn, tag_id, read=True, write=True, delete=True) + await tags_repo.access_count( + user_id=user.id, tag_id=tag_id, read=True, write=True, delete=True + ) == 1 ) assert ( - await other_repo.access_count(conn, tag_id, read=True, write=True, delete=True) + await other_repo.access_count( + user_id=other_user.id, tag_id=tag_id, read=True, write=True, delete=True + ) == 0 ) # other_tag_id - assert await tags_repo.access_count(conn, other_tag_id, read=True) == 0 - assert await other_repo.access_count(conn, other_tag_id, read=True) == 1 + assert ( + await tags_repo.access_count(user_id=user.id, tag_id=other_tag_id, read=True) + == 0 + ) + assert ( + await other_repo.access_count( + user_id=other_user.id, tag_id=other_tag_id, read=True + ) + == 1 + ) # group_tag_id - assert await tags_repo.access_count(conn, group_tag_id, read=True) == 1 - assert await other_repo.access_count(conn, group_tag_id, read=True) == 0 + assert ( + await tags_repo.access_count(user_id=user.id, tag_id=group_tag_id, read=True) + == 1 + ) + assert ( + await other_repo.access_count( + user_id=other_user.id, tag_id=group_tag_id, read=True + ) + == 0 + ) # everyone_tag_id - assert await tags_repo.access_count(conn, everyone_tag_id, read=True) == 1 - assert await other_repo.access_count(conn, everyone_tag_id, read=True) == 1 + assert ( + await tags_repo.access_count(user_id=user.id, tag_id=everyone_tag_id, read=True) + == 1 + ) + assert ( + await other_repo.access_count( + user_id=other_user.id, tag_id=everyone_tag_id, read=True + ) + == 1 + ) # now group adds read for all tags for t in (tag_id, other_tag_id, everyone_tag_id): @@ -218,19 +264,29 @@ async def test_tags_access_with_multiple_groups( delete=False, ) - assert await tags_repo.access_count(conn, tag_id, read=True) == 2 - assert await tags_repo.access_count(conn, other_tag_id, read=True) == 1 - assert await tags_repo.access_count(conn, everyone_tag_id, read=True) == 2 + assert await tags_repo.access_count(user_id=user.id, tag_id=tag_id, read=True) == 2 + assert ( + await tags_repo.access_count(user_id=user.id, tag_id=other_tag_id, read=True) + == 1 + ) + assert ( + await tags_repo.access_count(user_id=user.id, tag_id=everyone_tag_id, read=True) + == 2 + ) async def test_tags_repo_list_and_get( - connection: SAConnection, user: RowProxy, group: RowProxy, other_user: RowProxy + asyncpg_engine: AsyncEngine, + connection: SAConnection, + user: RowProxy, + group: RowProxy, + other_user: RowProxy, ): conn = connection - tags_repo = TagsRepo(user_id=user.id) + tags_repo = TagsRepo(asyncpg_engine) # (1) no tags - listed_tags = await tags_repo.list_all(conn) + listed_tags = await tags_repo.list_all(user_id=user.id) assert not listed_tags # (2) one tag @@ -247,7 +303,7 @@ async def test_tags_repo_list_and_get( ) ] - listed_tags = await tags_repo.list_all(conn) + listed_tags = await tags_repo.list_all(user_id=user.id) assert listed_tags assert [t["id"] for t in listed_tags] == expected_tags_ids @@ -265,7 +321,7 @@ async def test_tags_repo_list_and_get( ) ) - listed_tags = await tags_repo.list_all(conn) + listed_tags = await tags_repo.list_all(user_id=user.id) assert {t["id"] for t in listed_tags} == set(expected_tags_ids) # (4) add another tag from a differnt user @@ -282,7 +338,7 @@ async def test_tags_repo_list_and_get( # same as before prev_listed_tags = listed_tags - listed_tags = await tags_repo.list_all(conn) + listed_tags = await tags_repo.list_all(user_id=user.id) assert listed_tags == prev_listed_tags # (5) add a global tag @@ -297,7 +353,7 @@ async def test_tags_repo_list_and_get( delete=False, ) - listed_tags = await tags_repo.list_all(conn) + listed_tags = await tags_repo.list_all(user_id=user.id) assert listed_tags == [ { "id": 1, @@ -328,8 +384,8 @@ async def test_tags_repo_list_and_get( }, ] - other_repo = TagsRepo(user_id=other_user.id) - assert await other_repo.list_all(conn) == [ + other_repo = TagsRepo(asyncpg_engine) + assert await other_repo.list_all(user_id=other_user.id) == [ { "id": 3, "name": "T3", @@ -351,7 +407,7 @@ async def test_tags_repo_list_and_get( ] # exclusive to user - assert await tags_repo.get(conn, tag_id=2) == { + assert await tags_repo.get(user_id=user.id, tag_id=2) == { "id": 2, "name": "T2", "description": "tag via std group", @@ -363,9 +419,9 @@ async def test_tags_repo_list_and_get( # exclusive ot other user with pytest.raises(TagNotFoundError): - assert await tags_repo.get(conn, tag_id=3) + assert await tags_repo.get(user_id=user.id, tag_id=3) - assert await other_repo.get(conn, tag_id=3) == { + assert await other_repo.get(user_id=other_user.id, tag_id=3) == { "id": 3, "name": "T3", "description": "tag for 2", @@ -376,14 +432,71 @@ async def test_tags_repo_list_and_get( } # a common tag - assert await tags_repo.get(conn, tag_id=4) == await other_repo.get(conn, tag_id=4) + assert await tags_repo.get(user_id=user.id, tag_id=4) == await other_repo.get( + user_id=user.id, tag_id=4 + ) + + +async def test_tags_repo_uniquely_list_or_get_shared_tags( + asyncpg_engine: AsyncEngine, + connection: SAConnection, + user: RowProxy, + group: RowProxy, +): + conn = connection + tags_repo = TagsRepo(asyncpg_engine) + + # (1) create a tag which cannot be written + expected_tag_id = await create_tag( + conn, + name="T1", + description=f"tag for {user.id}", + color="blue", + group_id=user.primary_gid, + read=True, + write=False, # <-- cannot write + delete=True, + ) + + got = await tags_repo.get(user_id=user.id, tag_id=expected_tag_id) + assert got + assert got["id"] == expected_tag_id + assert got["read"] is True + assert got["write"] is False # <-- + assert got["delete"] is True + + # (2) share with standard group + await create_tag_access( + conn, + tag_id=expected_tag_id, + group_id=group.gid, + read=True, + write=True, # < -- group can write + delete=False, + ) + + # checks that the agregattion is the MOST permisive + # checks that user_id has now full access via its primary and its stadard group + got = await tags_repo.get(user_id=user.id, tag_id=expected_tag_id) + assert got + assert got["id"] == expected_tag_id + assert got["read"] is True + assert got["write"] is True # <-- + assert got["delete"] is True + + user_tags = await tags_repo.list_all(user_id=user.id) + assert user_tags == [got] async def test_tags_repo_update( - connection: SAConnection, user: RowProxy, group: RowProxy, other_user: RowProxy + asyncpg_engine: AsyncEngine, + connection: SAConnection, + user: RowProxy, + group: RowProxy, + other_user: RowProxy, ): conn = connection - tags_repo = TagsRepo(user_id=user.id) + tags_repo = TagsRepo(asyncpg_engine) # Tags with different access rights readonly_tid, readwrite_tid, other_tid = [ @@ -420,10 +533,12 @@ async def test_tags_repo_update( ] with pytest.raises(TagOperationNotAllowedError): - await tags_repo.update(conn, tag_id=readonly_tid, description="modified") + await tags_repo.update( + user_id=user.id, tag_id=readonly_tid, description="modified" + ) assert await tags_repo.update( - conn, tag_id=readwrite_tid, description="modified" + user_id=user.id, tag_id=readwrite_tid, description="modified" ) == { "id": readwrite_tid, "name": "T2", @@ -435,14 +550,20 @@ async def test_tags_repo_update( } with pytest.raises(TagOperationNotAllowedError): - await tags_repo.update(conn, tag_id=other_tid, description="modified") + await tags_repo.update( + user_id=user.id, tag_id=other_tid, description="modified" + ) async def test_tags_repo_delete( - connection: SAConnection, user: RowProxy, group: RowProxy, other_user: RowProxy + asyncpg_engine: AsyncEngine, + connection: SAConnection, + user: RowProxy, + group: RowProxy, + other_user: RowProxy, ): conn = connection - tags_repo = TagsRepo(user_id=user.id) + tags_repo = TagsRepo(asyncpg_engine) # Tags with different access rights readonly_tid, delete_tid, other_tid = [ @@ -480,28 +601,32 @@ async def test_tags_repo_delete( # cannot delete with pytest.raises(TagOperationNotAllowedError): - await tags_repo.delete(conn, tag_id=readonly_tid) + await tags_repo.delete(user_id=user.id, tag_id=readonly_tid) # can delete - await tags_repo.get(conn, tag_id=delete_tid) - await tags_repo.delete(conn, tag_id=delete_tid) + await tags_repo.get(user_id=user.id, tag_id=delete_tid) + await tags_repo.delete(user_id=user.id, tag_id=delete_tid) with pytest.raises(TagNotFoundError): - await tags_repo.get(conn, tag_id=delete_tid) + await tags_repo.get(user_id=user.id, tag_id=delete_tid) # cannot delete with pytest.raises(TagOperationNotAllowedError): - await tags_repo.delete(conn, tag_id=other_tid) + await tags_repo.delete(user_id=user.id, tag_id=other_tid) async def test_tags_repo_create( - connection: SAConnection, user: RowProxy, group: RowProxy, other_user: RowProxy + asyncpg_engine: AsyncEngine, + connection: SAConnection, + user: RowProxy, + group: RowProxy, + other_user: RowProxy, ): conn = connection - tags_repo = TagsRepo(user_id=user.id) + tags_repo = TagsRepo(asyncpg_engine) tag_1 = await tags_repo.create( - conn, + user_id=user.id, name="T1", description="my first tag", color="pink", @@ -546,6 +671,11 @@ def _check(func_smt, **kwargs): service_key = "simcore/services/comp/isolve" service_version = "2.0.85" + _check( + list_tags_stmt, + user_id=user_id, + ) + _check( get_tag_stmt, user_id=user_id, diff --git a/services/web/server/VERSION b/services/web/server/VERSION index 8298bb08b2d..f8287cf9564 100644 --- a/services/web/server/VERSION +++ b/services/web/server/VERSION @@ -1 +1 @@ -0.43.0 +0.43.1 diff --git a/services/web/server/setup.cfg b/services/web/server/setup.cfg index a2bfd28d955..4e3589a674b 100644 --- a/services/web/server/setup.cfg +++ b/services/web/server/setup.cfg @@ -1,5 +1,5 @@ [bumpversion] -current_version = 0.43.0 +current_version = 0.43.1 commit = True message = services/webserver api version: {current_version} → {new_version} tag = False diff --git a/services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml b/services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml index 90aeff0748b..13aa1b47dad 100644 --- a/services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml +++ b/services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml @@ -2,7 +2,7 @@ openapi: 3.0.2 info: title: simcore-service-webserver description: Main service with an interface (http-API & websockets) to the web front-end - version: 0.43.0 + version: 0.43.1 servers: - url: '' description: webserver @@ -11931,6 +11931,9 @@ components: title: Color pattern: ^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$ type: string + priority: + title: Priority + type: integer TagGet: title: TagGet required: @@ -12020,6 +12023,9 @@ components: title: Color pattern: ^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$ type: string + priority: + title: Priority + type: integer TaskCounts: title: TaskCounts type: object diff --git a/services/web/server/src/simcore_service_webserver/db/plugin.py b/services/web/server/src/simcore_service_webserver/db/plugin.py index b3dcdacf280..a01efdbc40c 100644 --- a/services/web/server/src/simcore_service_webserver/db/plugin.py +++ b/services/web/server/src/simcore_service_webserver/db/plugin.py @@ -8,7 +8,7 @@ from servicelib.aiohttp.application_keys import APP_AIOPG_ENGINE_KEY from servicelib.aiohttp.application_setup import ModuleCategory, app_module_setup -from . import _aiopg +from . import _aiopg, _asyncpg _logger = logging.getLogger(__name__) @@ -20,6 +20,10 @@ is_service_enabled = _aiopg.is_service_enabled +# asyncpg helpers +get_asyncpg_engine = _asyncpg.get_async_engine + + @app_module_setup( "simcore_service_webserver.db", ModuleCategory.ADDON, @@ -34,3 +38,4 @@ def setup_db(app: web.Application): # init engines app.cleanup_ctx.append(_aiopg.postgres_cleanup_ctx) + app.cleanup_ctx.append(_asyncpg.postgres_cleanup_ctx) diff --git a/services/web/server/src/simcore_service_webserver/tags/_api.py b/services/web/server/src/simcore_service_webserver/tags/_api.py new file mode 100644 index 00000000000..6f3a74853e7 --- /dev/null +++ b/services/web/server/src/simcore_service_webserver/tags/_api.py @@ -0,0 +1,58 @@ +""" _api: implements `tags` plugin **service layer** +""" + +from aiohttp import web +from models_library.basic_types import IdInt +from models_library.users import UserID +from servicelib.aiohttp.db_asyncpg_engine import get_async_engine +from simcore_postgres_database.utils_tags import TagsRepo +from sqlalchemy.ext.asyncio import AsyncEngine + +from .schemas import TagCreate, TagGet, TagUpdate + + +async def create_tag( + app: web.Application, user_id: UserID, new_tag: TagCreate +) -> TagGet: + engine: AsyncEngine = get_async_engine(app) + + repo = TagsRepo(engine) + tag = await repo.create( + user_id=user_id, + read=True, + write=True, + delete=True, + **new_tag.dict(exclude_unset=True), + ) + return TagGet.from_db(tag) + + +async def list_tags( + app: web.Application, + user_id: UserID, +) -> list[TagGet]: + engine: AsyncEngine = get_async_engine(app) + repo = TagsRepo(engine) + tags = await repo.list_all(user_id=user_id) + return [TagGet.from_db(t) for t in tags] + + +async def update_tag( + app: web.Application, user_id: UserID, tag_id: IdInt, tag_updates: TagUpdate +) -> TagGet: + engine: AsyncEngine = get_async_engine(app) + + repo = TagsRepo(engine) + tag = await repo.update( + user_id=user_id, + tag_id=tag_id, + **tag_updates.dict(exclude_unset=True), + ) + return TagGet.from_db(tag) + + +async def delete_tag(app: web.Application, user_id: UserID, tag_id: IdInt): + engine: AsyncEngine = get_async_engine(app) + + repo = TagsRepo(engine) + await repo.delete(user_id=user_id, tag_id=tag_id) diff --git a/services/web/server/src/simcore_service_webserver/tags/_handlers.py b/services/web/server/src/simcore_service_webserver/tags/_handlers.py index 07925f4749e..5492d5e468e 100644 --- a/services/web/server/src/simcore_service_webserver/tags/_handlers.py +++ b/services/web/server/src/simcore_service_webserver/tags/_handlers.py @@ -1,7 +1,6 @@ import functools from aiohttp import web -from aiopg.sa.engine import Engine from pydantic import parse_obj_as from servicelib.aiohttp.requests_validation import ( parse_request_body_as, @@ -12,17 +11,15 @@ from simcore_postgres_database.utils_tags import ( TagNotFoundError, TagOperationNotAllowedError, - TagsRepo, ) from .._meta import API_VTAG as VTAG -from ..db.plugin import get_database_engine from ..login.decorators import login_required from ..security.decorators import permission_required from ..utils_aiohttp import envelope_json_response +from . import _api from .schemas import ( TagCreate, - TagGet, TagGroupCreate, TagGroupGet, TagGroupPathParams, @@ -42,34 +39,31 @@ async def wrapper(request: web.Request) -> web.StreamResponse: raise web.HTTPNotFound(reason=f"{exc}") from exc except TagOperationNotAllowedError as exc: - raise web.HTTPUnauthorized(reason=f"{exc}") from exc + raise web.HTTPForbidden(reason=f"{exc}") from exc return wrapper routes = web.RouteTableDef() +# +# tags CRUD standard operations +# + @routes.post(f"/{VTAG}/tags", name="create_tag") @login_required @permission_required("tag.crud.*") @_handle_tags_exceptions async def create_tag(request: web.Request): - engine: Engine = get_database_engine(request.app) + assert request.app # nosec req_ctx = TagRequestContext.parse_obj(request) new_tag = await parse_request_body_as(TagCreate, request) - repo = TagsRepo(user_id=req_ctx.user_id) - async with engine.acquire() as conn: - tag = await repo.create( - conn, - read=True, - write=True, - delete=True, - **new_tag.dict(exclude_unset=True), - ) - model = TagGet.from_db(tag) - return envelope_json_response(model) + created = await _api.create_tag( + request.app, user_id=req_ctx.user_id, new_tag=new_tag + ) + return envelope_json_response(created) @routes.get(f"/{VTAG}/tags", name="list_tags") @@ -77,15 +71,10 @@ async def create_tag(request: web.Request): @permission_required("tag.crud.*") @_handle_tags_exceptions async def list_tags(request: web.Request): - engine: Engine = get_database_engine(request.app) - req_ctx = TagRequestContext.parse_obj(request) - repo = TagsRepo(user_id=req_ctx.user_id) - async with engine.acquire() as conn: - tags = await repo.list_all(conn) - return envelope_json_response( - [TagGet.from_db(t).dict(by_alias=True) for t in tags] - ) + req_ctx = TagRequestContext.parse_obj(request) + got = await _api.list_tags(request.app, user_id=req_ctx.user_id) + return envelope_json_response(got) @routes.patch(f"/{VTAG}/tags/{{tag_id}}", name="update_tag") @@ -93,18 +82,17 @@ async def list_tags(request: web.Request): @permission_required("tag.crud.*") @_handle_tags_exceptions async def update_tag(request: web.Request): - engine: Engine = get_database_engine(request.app) req_ctx = TagRequestContext.parse_obj(request) path_params = parse_request_path_parameters_as(TagPathParams, request) tag_updates = await parse_request_body_as(TagUpdate, request) - repo = TagsRepo(user_id=req_ctx.user_id) - async with engine.acquire() as conn: - tag = await repo.update( - conn, path_params.tag_id, **tag_updates.dict(exclude_unset=True) - ) - model = TagGet.from_db(tag) - return envelope_json_response(model) + updated = await _api.update_tag( + request.app, + user_id=req_ctx.user_id, + tag_id=path_params.tag_id, + tag_updates=tag_updates, + ) + return envelope_json_response(updated) @routes.delete(f"/{VTAG}/tags/{{tag_id}}", name="delete_tag") @@ -112,17 +100,21 @@ async def update_tag(request: web.Request): @permission_required("tag.crud.*") @_handle_tags_exceptions async def delete_tag(request: web.Request): - engine: Engine = get_database_engine(request.app) req_ctx = TagRequestContext.parse_obj(request) path_params = parse_request_path_parameters_as(TagPathParams, request) - repo = TagsRepo(user_id=req_ctx.user_id) - async with engine.acquire() as conn: - await repo.delete(conn, tag_id=path_params.tag_id) + await _api.delete_tag( + request.app, user_id=req_ctx.user_id, tag_id=path_params.tag_id + ) raise web.HTTPNoContent(content_type=MIMETYPE_APPLICATION_JSON) +# +# tags ACCESS RIGHTS is exposed as a sub-resource groups +# + + @routes.get(f"/{VTAG}/tags/{{tag_id}}/groups", name="list_tag_groups") @login_required @permission_required("tag.crud.*") diff --git a/services/web/server/src/simcore_service_webserver/tags/schemas.py b/services/web/server/src/simcore_service_webserver/tags/schemas.py index 1c62cd25d89..01663e0d337 100644 --- a/services/web/server/src/simcore_service_webserver/tags/schemas.py +++ b/services/web/server/src/simcore_service_webserver/tags/schemas.py @@ -25,12 +25,14 @@ class TagUpdate(InputSchema): name: str | None = None description: str | None = None color: ColorStr | None = None + priority: int | None = None class TagCreate(InputSchema): name: str description: str | None = None color: ColorStr + priority: int | None = None class TagAccessRights(OutputSchema): diff --git a/services/web/server/tests/unit/with_dbs/01/test_db.py b/services/web/server/tests/unit/with_dbs/01/test_db.py index 4b4dce7b134..a9204b460bf 100644 --- a/services/web/server/tests/unit/with_dbs/01/test_db.py +++ b/services/web/server/tests/unit/with_dbs/01/test_db.py @@ -7,12 +7,9 @@ import aiopg.sa import asyncpg -import pytest import sqlalchemy as sa import yaml -from aiohttp import web from aiohttp.test_utils import TestServer -from pytest_mock import MockFixture, MockType from simcore_service_webserver.application_settings import ( ApplicationSettings, get_application_settings, @@ -21,36 +18,12 @@ from simcore_service_webserver.db.plugin import ( is_service_enabled, is_service_responsive, - setup_db, ) from simcore_service_webserver.login.storage import AsyncpgStorage, get_plugin_storage from sqlalchemy.ext.asyncio import AsyncEngine -@pytest.fixture -def mock_asyncpg_in_setup_db(mocker: MockFixture) -> MockType: - - original_setup = setup_db - - mock_setup_db = mocker.patch( - "simcore_service_webserver.application.setup_db", autospec=True - ) - - def _wrapper_setup_db(app: web.Application): - original_setup(app) - - # NEW engine ! - app.cleanup_ctx.append(_asyncpg.postgres_cleanup_ctx) - - mock_setup_db.side_effect = _wrapper_setup_db - return mock_setup_db - - -async def test_all_pg_engines_in_app( - mock_asyncpg_in_setup_db: MockType, web_server: TestServer -): - assert mock_asyncpg_in_setup_db.called - +async def test_all_pg_engines_in_app(web_server: TestServer): app = web_server.app assert app diff --git a/services/web/server/tests/unit/with_dbs/03/tags/test_tags.py b/services/web/server/tests/unit/with_dbs/03/tags/test_tags.py index 5e480b2777f..eff7bc5c532 100644 --- a/services/web/server/tests/unit/with_dbs/03/tags/test_tags.py +++ b/services/web/server/tests/unit/with_dbs/03/tags/test_tags.py @@ -5,7 +5,6 @@ from collections.abc import AsyncIterator, Callable, Iterator -from http import HTTPStatus from typing import Any import pytest @@ -46,12 +45,16 @@ def fake_tags(faker: Faker) -> list[dict[str, Any]]: ] -@pytest.mark.parametrize("user_role,expected", [(UserRole.USER, status.HTTP_200_OK)]) +@pytest.fixture +def user_role() -> UserRole: + # All tests in test_tags assume USER's role + # i.e. Used in `logged_user` and `user_project` + return UserRole.USER + + async def test_tags_to_studies( client: TestClient, - logged_user: UserInfoDict, user_project: ProjectDict, - expected: HTTPStatus, fake_tags: dict[str, Any], catalog_subsystem_mock: Callable[[list[ProjectDict]], None], ): @@ -64,7 +67,7 @@ async def test_tags_to_studies( for tag in fake_tags: url = client.app.router["create_tag"].url_for() resp = await client.post(f"{url}", json=tag) - added_tag, _ = await assert_status(resp, expected) + added_tag, _ = await assert_status(resp, status.HTTP_200_OK) added_tags.append(added_tag) # Add tag to study @@ -72,7 +75,7 @@ async def test_tags_to_studies( project_uuid=user_project.get("uuid"), tag_id=str(added_tag.get("id")) ) resp = await client.post(f"{url}") - data, _ = await assert_status(resp, expected) + data, _ = await assert_status(resp, status.HTTP_200_OK) # Tag is included in response assert added_tag["id"] in data["tags"] @@ -87,7 +90,8 @@ async def test_tags_to_studies( exclude_unset=True, ) user_project["folderId"] = None - data = await assert_get_same_project(client, user_project, expected) + + data = await assert_get_same_project(client, user_project, status.HTTP_200_OK) # Delete tag0 url = client.app.router["delete_tag"].url_for(tag_id=str(added_tags[0].get("id"))) @@ -96,7 +100,7 @@ async def test_tags_to_studies( # Get project and check that tag is no longer there user_project["tags"].remove(added_tags[0]["id"]) - data = await assert_get_same_project(client, user_project, expected) + data = await assert_get_same_project(client, user_project, status.HTTP_200_OK) assert added_tags[0].get("id") not in data.get("tags") # Remove tag1 from project @@ -104,11 +108,11 @@ async def test_tags_to_studies( project_uuid=user_project.get("uuid"), tag_id=str(added_tags[1].get("id")) ) resp = await client.post(f"{url}") - await assert_status(resp, expected) + await assert_status(resp, status.HTTP_200_OK) # Get project and check that tag is no longer there user_project["tags"].remove(added_tags[1]["id"]) - data = await assert_get_same_project(client, user_project, expected) + data = await assert_get_same_project(client, user_project, status.HTTP_200_OK) assert added_tags[1].get("id") not in data.get("tags") # Delete tag1 @@ -140,11 +144,6 @@ async def everybody_tag_id(client: TestClient) -> AsyncIterator[int]: await delete_tag(conn, tag_id=tag_id) -@pytest.fixture -def user_role() -> UserRole: - return UserRole.USER - - async def test_read_tags( client: TestClient, logged_user: UserInfoDict, @@ -181,8 +180,10 @@ async def test_create_and_update_tags( assert user_role == UserRole.USER + # (1) create tag + url = client.app.router["create_tag"].url_for() resp = await client.post( - f"{client.app.router['create_tag'].url_for()}", + f"{url}", json={"name": "T", "color": "#f00"}, ) created, _ = await assert_status(resp, status.HTTP_200_OK) @@ -195,6 +196,7 @@ async def test_create_and_update_tags( "accessRights": {"read": True, "write": True, "delete": True}, } + # (2) update created tag url = client.app.router["update_tag"].url_for(tag_id=f"{created['id']}") resp = await client.patch( f"{url}", @@ -205,10 +207,72 @@ async def test_create_and_update_tags( created.update(description="This is my tag") assert updated == created + # (3) Cannot update tag because it has not enough access rights url = client.app.router["update_tag"].url_for(tag_id=f"{everybody_tag_id}") resp = await client.patch( f"{url}", json={"description": "I have NO WRITE ACCESS TO THIS TAG"}, ) - _, error = await assert_status(resp, status.HTTP_401_UNAUTHORIZED) + _, error = await assert_status(resp, status.HTTP_403_FORBIDDEN) assert error + + +async def test_create_tags_with_order_index( + client: TestClient, + logged_user: UserInfoDict, + user_role: UserRole, + _clean_tags_table: None, +): + assert client.app + + assert user_role == UserRole.USER + + # (1) create tags but set the order in reverse order of creation + url = client.app.router["create_tag"].url_for() + num_tags = 3 + expected_tags: list[Any] = [None] * num_tags + for creation_index, priority_index in enumerate(range(num_tags - 1, -1, -1)): + resp = await client.post( + f"{url}", + json={ + "name": f"T{creation_index}-{priority_index}", + "description": f"{creation_index=}, {priority_index=}", + "color": "#f00", + "priority": priority_index, + }, + ) + created, _ = await assert_status(resp, status.HTTP_200_OK) + expected_tags[priority_index] = created + + url = client.app.router["list_tags"].url_for() + resp = await client.get(f"{url}") + got, _ = await assert_status(resp, status.HTTP_200_OK) + assert got == expected_tags + + # (2) lets update all priorities in reverse order + for new_order_index, tag in enumerate(reversed(expected_tags)): + url = client.app.router["update_tag"].url_for(tag_id=f"{tag['id']}") + resp = await client.patch( + f"{url}", + json={"priority": new_order_index}, + ) + updated, _ = await assert_status(resp, status.HTTP_200_OK) + # NOTE: priority is not included in TagGet for now + assert updated == tag + + url = client.app.router["list_tags"].url_for() + resp = await client.get(f"{url}") + got, _ = await assert_status(resp, status.HTTP_200_OK) + assert got == expected_tags[::-1] + + # (3) new tag without priority should get last (because is last created) + resp = await client.post( + f"{url}", + json={"name": "New", "color": "#f00", "description": "w/o priority"}, + ) + last_created, _ = await assert_status(resp, status.HTTP_200_OK) + + url = client.app.router["list_tags"].url_for() + resp = await client.get(f"{url}") + got, _ = await assert_status(resp, status.HTTP_200_OK) + assert got == [*expected_tags[::-1], last_created]