From 9c6068d7d668704eb53ffff420acb589aa0d853b Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 5 Nov 2024 18:46:35 +0100 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Enhanced=20groups/organiza?= =?UTF-8?q?tions=20web-api=20specs=20and=20validation=20=20=F0=9F=9A=A8=20?= =?UTF-8?q?(#6640)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/specs/web-server/_groups.py | 101 ++++-- .../api_schemas_webserver/groups.py | 78 ++++- .../models_library/utils/common_validators.py | 35 ++ .../test_api_schemas_webserver_groups.py | 27 ++ .../tests/test_archiving_utils.py | 9 +- .../service-library/tests/test_async_utils.py | 20 +- .../tests/unit/test_api_rest_containers.py | 5 +- services/storage/tests/unit/test_utils.py | 18 +- .../api/v0/openapi.yaml | 304 +++++++++++------- .../simcore_service_webserver/groups/_db.py | 38 ++- .../groups/_handlers.py | 113 ++++--- .../groups/_utils.py | 5 +- .../simcore_service_webserver/groups/api.py | 8 +- .../groups/exceptions.py | 20 +- .../users/schemas.py | 4 +- .../tests/unit/isolated/test_groups_models.py | 6 +- .../tests/unit/with_dbs/01/test_groups.py | 10 +- 17 files changed, 530 insertions(+), 271 deletions(-) create mode 100644 packages/models-library/tests/test_api_schemas_webserver_groups.py diff --git a/api/specs/web-server/_groups.py b/api/specs/web-server/_groups.py index 02c04b663fa..1f8d7e15f56 100644 --- a/api/specs/web-server/_groups.py +++ b/api/specs/web-server/_groups.py @@ -8,14 +8,21 @@ from fastapi import APIRouter, Depends, status from models_library.api_schemas_webserver.groups import ( - AllUsersGroups, + GroupCreate, + GroupGet, + GroupUpdate, GroupUserGet, - UsersGroup, + MyGroupsGet, ) from models_library.generics import Envelope -from models_library.users import GroupID, UserID from simcore_service_webserver._meta import API_VTAG -from simcore_service_webserver.groups._handlers import _ClassifiersQuery +from simcore_service_webserver.groups._handlers import ( + GroupUserAdd, + GroupUserUpdate, + _ClassifiersQuery, + _GroupPathParams, + _GroupUserPathParams, +) from simcore_service_webserver.scicrunch.models import ResearchResource, ResourceHit router = APIRouter( @@ -28,51 +35,66 @@ @router.get( "/groups", - response_model=Envelope[AllUsersGroups], + response_model=Envelope[MyGroupsGet], ) async def list_groups(): - ... + """ + List all groups (organizations, primary, everyone and products) I belong to + """ @router.post( "/groups", - response_model=Envelope[UsersGroup], + response_model=Envelope[GroupGet], status_code=status.HTTP_201_CREATED, ) -async def create_group(): - ... +async def create_group(_b: GroupCreate): + """ + Creates an organization group + """ @router.get( "/groups/{gid}", - response_model=Envelope[UsersGroup], + response_model=Envelope[GroupGet], ) -async def get_group(gid: GroupID): - ... +async def get_group(_p: Annotated[_GroupPathParams, Depends()]): + """ + Get an organization group + """ @router.patch( "/groups/{gid}", - response_model=Envelope[UsersGroup], + response_model=Envelope[GroupGet], ) -async def update_group(gid: GroupID, _update: UsersGroup): - ... +async def update_group( + _p: Annotated[_GroupPathParams, Depends()], + _b: GroupUpdate, +): + """ + Updates organization groups + """ @router.delete( "/groups/{gid}", status_code=status.HTTP_204_NO_CONTENT, ) -async def delete_group(gid: GroupID): - ... +async def delete_group(_p: Annotated[_GroupPathParams, Depends()]): + """ + Deletes organization groups + """ @router.get( "/groups/{gid}/users", response_model=Envelope[list[GroupUserGet]], ) -async def get_group_users(gid: GroupID): - ... +async def get_all_group_users(_p: Annotated[_GroupPathParams, Depends()]): + """ + Gets users in organization groups + """ @router.post( @@ -80,10 +102,12 @@ async def get_group_users(gid: GroupID): status_code=status.HTTP_204_NO_CONTENT, ) async def add_group_user( - gid: GroupID, - _new: GroupUserGet, + _p: Annotated[_GroupPathParams, Depends()], + _b: GroupUserAdd, ): - ... + """ + Adds a user to an organization group + """ @router.get( @@ -91,10 +115,11 @@ async def add_group_user( response_model=Envelope[GroupUserGet], ) async def get_group_user( - gid: GroupID, - uid: UserID, + _p: Annotated[_GroupUserPathParams, Depends()], ): - ... + """ + Gets specific user in an organization group + """ @router.patch( @@ -102,12 +127,12 @@ async def get_group_user( response_model=Envelope[GroupUserGet], ) async def update_group_user( - gid: GroupID, - uid: UserID, - _update: GroupUserGet, + _p: Annotated[_GroupUserPathParams, Depends()], + _b: GroupUserUpdate, ): - # FIXME: update type - ... + """ + Updates user (access-rights) to an organization group + """ @router.delete( @@ -115,10 +140,16 @@ async def update_group_user( status_code=status.HTTP_204_NO_CONTENT, ) async def delete_group_user( - gid: GroupID, - uid: UserID, + _p: Annotated[_GroupUserPathParams, Depends()], ): - ... + """ + Removes a user from an organization group + """ + + +# +# Classifiers +# @router.get( @@ -126,8 +157,8 @@ async def delete_group_user( response_model=Envelope[dict[str, Any]], ) async def get_group_classifiers( - gid: GroupID, - _query: Annotated[_ClassifiersQuery, Depends()], + _p: Annotated[_GroupPathParams, Depends()], + _q: Annotated[_ClassifiersQuery, Depends()], ): ... diff --git a/packages/models-library/src/models_library/api_schemas_webserver/groups.py b/packages/models-library/src/models_library/api_schemas_webserver/groups.py index e0b6d3fbb37..55107be55c5 100644 --- a/packages/models-library/src/models_library/api_schemas_webserver/groups.py +++ b/packages/models-library/src/models_library/api_schemas_webserver/groups.py @@ -1,13 +1,20 @@ from contextlib import suppress from typing import Any, ClassVar -from pydantic import AnyUrl, BaseModel, Field, ValidationError, parse_obj_as, validator +from pydantic import ( + AnyUrl, + BaseModel, + Field, + ValidationError, + parse_obj_as, + root_validator, + validator, +) from ..emails import LowerCaseEmailStr - -# -# GROUPS MODELS defined in OPENAPI specs -# +from ..users import UserID +from ..utils.common_validators import create__check_only_one_is_set__root_validator +from ._base import InputSchema, OutputSchema class GroupAccessRights(BaseModel): @@ -29,7 +36,7 @@ class Config: } -class UsersGroup(BaseModel): +class GroupGet(OutputSchema): gid: int = Field(..., description="the group ID") label: str = Field(..., description="the group name") description: str = Field(..., description="the group description") @@ -45,7 +52,7 @@ class UsersGroup(BaseModel): @validator("thumbnail", pre=True) @classmethod - def sanitize_legacy_data(cls, v): + def _sanitize_legacy_data(cls, v): if v: # Enforces null if thumbnail is not valid URL or empty with suppress(ValidationError): @@ -86,11 +93,23 @@ class Config: } -class AllUsersGroups(BaseModel): - me: UsersGroup | None = None - organizations: list[UsersGroup] | None = None - all: UsersGroup | None = None - product: UsersGroup | None = None +class GroupCreate(InputSchema): + label: str + description: str + thumbnail: AnyUrl | None = None + + +class GroupUpdate(InputSchema): + label: str | None = None + description: str | None = None + thumbnail: AnyUrl | None = None + + +class MyGroupsGet(OutputSchema): + me: GroupGet + organizations: list[GroupGet] | None = None + all: GroupGet + product: GroupGet | None = None class Config: schema_extra: ClassVar[dict[str, Any]] = { @@ -158,3 +177,38 @@ class Config: }, } } + + +class GroupUserAdd(InputSchema): + """ + Identify the user with either `email` or `uid` — only one. + """ + + uid: UserID | None = None + email: LowerCaseEmailStr | None = None + + _check_uid_or_email = root_validator(allow_reuse=True)( + create__check_only_one_is_set__root_validator(["uid", "email"]) + ) + + class Config: + schema_extra: ClassVar[dict[str, Any]] = { + "examples": [{"uid": 42}, {"email": "foo@email.com"}] + } + + +class GroupUserUpdate(InputSchema): + # NOTE: since it is a single item, it is required. Cannot + # update for the moment partial attributes e.g. {read: False} + access_rights: GroupAccessRights + + class Config: + schema_extra: ClassVar[dict[str, Any]] = { + "example": { + "accessRights": { + "read": True, + "write": False, + "delete": False, + }, + } + } diff --git a/packages/models-library/src/models_library/utils/common_validators.py b/packages/models-library/src/models_library/utils/common_validators.py index c60586dfa92..f1d754de5dc 100644 --- a/packages/models-library/src/models_library/utils/common_validators.py +++ b/packages/models-library/src/models_library/utils/common_validators.py @@ -16,6 +16,8 @@ class MyModel(BaseModel): """ import enum +import functools +import operator from typing import Any @@ -69,3 +71,36 @@ def null_or_none_str_to_none_validator(value: Any): if isinstance(value, str) and value.lower() in ("null", "none"): return None return value + + +def create__check_only_one_is_set__root_validator(alternative_field_names: list[str]): + """Ensure exactly one and only one of the alternatives is set + + NOTE: a field is considered here `unset` when it is `not None`. When None + is used to indicate something else, please do not use this validator. + + This is useful when you want to give the client alternative + ways to set the same thing e.g. set the user by email or id or username + and each of those has a different field + + NOTE: Alternatevely, the previous example can also be solved using a + single field as `user: Email | UserID | UserName` + + SEE test_uid_or_email_are_set.py for more details + """ + + def _validator(cls, values): + assert set(alternative_field_names).issubset(cls.__fields__) # nosec + + got = { + field_name: values.get(field_name) for field_name in alternative_field_names + } + + if not functools.reduce(operator.xor, (v is not None for v in got.values())): + msg = ( + f"Either { 'or'.join(got.keys()) } must be set, but not both. Got {got}" + ) + raise ValueError(msg) + return values + + return _validator diff --git a/packages/models-library/tests/test_api_schemas_webserver_groups.py b/packages/models-library/tests/test_api_schemas_webserver_groups.py new file mode 100644 index 00000000000..c254ef40cf4 --- /dev/null +++ b/packages/models-library/tests/test_api_schemas_webserver_groups.py @@ -0,0 +1,27 @@ +from typing import Any + +import pytest +from models_library.api_schemas_webserver.groups import GroupUserAdd +from pydantic import ValidationError + +unset = object() + + +@pytest.mark.parametrize("uid", [1, None, unset]) +@pytest.mark.parametrize("email", ["user@foo.com", None, unset]) +def test_uid_or_email_are_set(uid: Any, email: Any): + kwargs = {} + if uid != unset: + kwargs["uid"] = uid + if email != unset: + kwargs["email"] = email + + none_are_defined = kwargs.get("uid") is None and kwargs.get("email") is None + both_are_defined = kwargs.get("uid") is not None and kwargs.get("email") is not None + + if none_are_defined or both_are_defined: + with pytest.raises(ValidationError, match="not both"): + GroupUserAdd(**kwargs) + else: + got = GroupUserAdd(**kwargs) + assert bool(got.email) ^ bool(got.uid) diff --git a/packages/service-library/tests/test_archiving_utils.py b/packages/service-library/tests/test_archiving_utils.py index 3996be43ca5..3a94c9fd22c 100644 --- a/packages/service-library/tests/test_archiving_utils.py +++ b/packages/service-library/tests/test_archiving_utils.py @@ -7,7 +7,6 @@ import hashlib import itertools import os -import random import secrets import string import tempfile @@ -35,14 +34,14 @@ def _print_tree(path: Path, level=0): @pytest.fixture -def dir_with_random_content() -> Iterable[Path]: +def dir_with_random_content(faker: Faker) -> Iterable[Path]: def random_string(length: int) -> str: return "".join(secrets.choice(string.ascii_letters) for i in range(length)) def make_files_in_dir(dir_path: Path, file_count: int) -> None: for _ in range(file_count): (dir_path / f"{random_string(8)}.bin").write_bytes( - os.urandom(random.randint(1, 10)) + os.urandom(faker.random_int(1, 10)) ) def ensure_dir(path_to_ensure: Path) -> Path: @@ -53,13 +52,13 @@ def make_subdirectory_with_content(subdir_name: Path, max_file_count: int) -> No subdir_name = ensure_dir(subdir_name) make_files_in_dir( dir_path=subdir_name, - file_count=random.randint(1, max_file_count), + file_count=faker.random_int(1, max_file_count), ) def make_subdirectories_with_content( subdir_name: Path, max_subdirectories_count: int, max_file_count: int ) -> None: - subdirectories_count = random.randint(1, max_subdirectories_count) + subdirectories_count = faker.random_int(1, max_subdirectories_count) for _ in range(subdirectories_count): make_subdirectory_with_content( subdir_name=subdir_name / f"{random_string(4)}", diff --git a/packages/service-library/tests/test_async_utils.py b/packages/service-library/tests/test_async_utils.py index cf2d051fe95..902cbcc9b82 100644 --- a/packages/service-library/tests/test_async_utils.py +++ b/packages/service-library/tests/test_async_utils.py @@ -8,7 +8,7 @@ from collections import deque from dataclasses import dataclass from time import time -from typing import Any, Optional +from typing import Any import pytest from faker import Faker @@ -59,24 +59,25 @@ def _compensate_for_slow_systems(number: float) -> float: async def test_context_aware_dispatch( - sleep_duration: float, - ensure_run_in_sequence_context_is_empty: None, + sleep_duration: float, ensure_run_in_sequence_context_is_empty: None, faker: Faker ) -> None: @run_sequentially_in_context(target_args=["c1", "c2", "c3"]) async def orderly(c1: Any, c2: Any, c3: Any, control: Any) -> None: _ = (c1, c2, c3) await asyncio.sleep(sleep_duration) - context = dict(c1=c1, c2=c2, c3=c3) + context = {"c1": c1, "c2": c2, "c3": c3} await locked_stores[make_key_from_context(context)].push(control) def make_key_from_context(context: dict) -> str: return ".".join([f"{k}:{v}" for k, v in context.items()]) def make_context(): - return dict( - c1=random.randint(0, 10), c2=random.randint(0, 10), c3=random.randint(0, 10) - ) + return { + "c1": faker.random_int(0, 10), + "c2": faker.random_int(0, 10), + "c3": faker.random_int(0, 10), + } contexts = [make_context() for _ in range(10)] @@ -116,7 +117,8 @@ class DidFailException(Exception): @run_sequentially_in_context(target_args=["will_fail"]) async def sometimes_failing(will_fail: bool) -> bool: if will_fail: - raise DidFailException("I was instructed to fail") + msg = "I was instructed to fail" + raise DidFailException(msg) return True for x in range(100): @@ -201,7 +203,7 @@ class ObjectWithPropos: @run_sequentially_in_context(target_args=["object_with_props.attr1"]) async def test_attribute( - object_with_props: ObjectWithPropos, other_attr: Optional[int] = None + object_with_props: ObjectWithPropos, other_attr: int | None = None ) -> str: return object_with_props.attr1 diff --git a/services/dynamic-sidecar/tests/unit/test_api_rest_containers.py b/services/dynamic-sidecar/tests/unit/test_api_rest_containers.py index f16b883de15..83e7a803f10 100644 --- a/services/dynamic-sidecar/tests/unit/test_api_rest_containers.py +++ b/services/dynamic-sidecar/tests/unit/test_api_rest_containers.py @@ -5,7 +5,6 @@ import asyncio import json -import random from collections.abc import AsyncIterable from inspect import signature from pathlib import Path @@ -303,9 +302,9 @@ async def attachable_networks_and_ids(faker: Faker) -> AsyncIterable[dict[str, s @pytest.fixture -def mock_aiodocker_containers_get(mocker: MockerFixture) -> int: +def mock_aiodocker_containers_get(mocker: MockerFixture, faker: Faker) -> int: """raises a DockerError with a random HTTP status which is also returned""" - mock_status_code = random.randint(1, 999) # noqa: S311 + mock_status_code = faker.random_int(1, 999) async def mock_get(*args: str, **kwargs: Any) -> None: raise aiodocker.exceptions.DockerError( diff --git a/services/storage/tests/unit/test_utils.py b/services/storage/tests/unit/test_utils.py index 1b71a1c29d5..03dc7e2e630 100644 --- a/services/storage/tests/unit/test_utils.py +++ b/services/storage/tests/unit/test_utils.py @@ -6,9 +6,8 @@ import datetime -import random +from collections.abc import Callable from pathlib import Path -from typing import Callable from uuid import uuid4 import pytest @@ -17,6 +16,7 @@ from models_library.projects import ProjectID from models_library.projects_nodes_io import NodeID, SimcoreS3FileID from pydantic import ByteSize, HttpUrl, parse_obj_as +from pytest_simcore.helpers.faker_factories import DEFAULT_FAKER from simcore_service_storage.constants import S3_UNDEFINED_OR_EXTERNAL_MULTIPART_ID from simcore_service_storage.models import ETag, FileMetaData, S3BucketName, UploadID from simcore_service_storage.simcore_s3_dsm import SimcoreS3DataManager @@ -46,24 +46,30 @@ async def test_download_files(tmp_path: Path, httpbin_base_url: HttpUrl): [ (-1, None, None, None, False), (0, None, None, None, False), - (random.randint(1, 1000000), None, None, None, False), + (DEFAULT_FAKER.random_int(1, 1000000), None, None, None, False), (-1, "some_valid_entity_tag", None, None, False), (0, "some_valid_entity_tag", None, None, False), ( - random.randint(1, 1000000), + DEFAULT_FAKER.random_int(1, 1000000), "some_valid_entity_tag", "som_upload_id", None, False, ), ( - random.randint(1, 1000000), + DEFAULT_FAKER.random_int(1, 1000000), "some_valid_entity_tag", None, datetime.datetime.utcnow(), False, ), - (random.randint(1, 1000000), "some_valid_entity_tag", None, None, True), + ( + DEFAULT_FAKER.random_int(1, 1000000), + "some_valid_entity_tag", + None, + None, + True, + ), ], ) def test_file_entry_valid( 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 0df5e076a76..82a18aebb3b 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 @@ -454,6 +454,8 @@ paths: tags: - groups summary: List Groups + description: List all groups (organizations, primary, everyone and products) + I belong to operationId: list_groups responses: '200': @@ -461,24 +463,32 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/Envelope_AllUsersGroups_' + $ref: '#/components/schemas/Envelope_MyGroupsGet_' post: tags: - groups summary: Create Group + description: Creates an organization group operationId: create_group + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/GroupCreate' + required: true responses: '201': description: Successful Response content: application/json: schema: - $ref: '#/components/schemas/Envelope_UsersGroup_' + $ref: '#/components/schemas/Envelope_GroupGet_' /v0/groups/{gid}: get: tags: - groups summary: Get Group + description: Get an organization group operationId: get_group parameters: - required: true @@ -495,11 +505,12 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/Envelope_UsersGroup_' + $ref: '#/components/schemas/Envelope_GroupGet_' delete: tags: - groups summary: Delete Group + description: Deletes organization groups operationId: delete_group parameters: - required: true @@ -517,6 +528,7 @@ paths: tags: - groups summary: Update Group + description: Updates organization groups operationId: update_group parameters: - required: true @@ -531,7 +543,7 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/UsersGroup' + $ref: '#/components/schemas/GroupUpdate' required: true responses: '200': @@ -539,13 +551,14 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/Envelope_UsersGroup_' + $ref: '#/components/schemas/Envelope_GroupGet_' /v0/groups/{gid}/users: get: tags: - groups - summary: Get Group Users - operationId: get_group_users + summary: Get All Group Users + description: Gets users in organization groups + operationId: get_all_group_users parameters: - required: true schema: @@ -566,6 +579,7 @@ paths: tags: - groups summary: Add Group User + description: Adds a user to an organization group operationId: add_group_user parameters: - required: true @@ -580,7 +594,7 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/GroupUserGet' + $ref: '#/components/schemas/GroupUserAdd' required: true responses: '204': @@ -590,6 +604,7 @@ paths: tags: - groups summary: Get Group User + description: Gets specific user in an organization group operationId: get_group_user parameters: - required: true @@ -619,6 +634,7 @@ paths: tags: - groups summary: Delete Group User + description: Removes a user from an organization group operationId: delete_group_user parameters: - required: true @@ -644,6 +660,7 @@ paths: tags: - groups summary: Update Group User + description: Updates user (access-rights) to an organization group operationId: update_group_user parameters: - required: true @@ -666,7 +683,7 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/GroupUserGet' + $ref: '#/components/schemas/GroupUserUpdate' required: true responses: '200': @@ -4311,7 +4328,7 @@ paths: '403': description: ProjectInvalidRightsError '404': - description: UserDefaultWalletNotFoundError, ProjectNotFoundError + description: ProjectNotFoundError, UserDefaultWalletNotFoundError '409': description: ProjectTooManyProjectOpenedError '422': @@ -6022,54 +6039,6 @@ components: queued: title: Queued type: boolean - AllUsersGroups: - title: AllUsersGroups - type: object - properties: - me: - $ref: '#/components/schemas/UsersGroup' - organizations: - title: Organizations - type: array - items: - $ref: '#/components/schemas/UsersGroup' - all: - $ref: '#/components/schemas/UsersGroup' - product: - $ref: '#/components/schemas/UsersGroup' - example: - me: - gid: '27' - label: A user - description: A very special user - accessRights: - read: true - write: true - delete: true - organizations: - - gid: '15' - label: ITIS Foundation - description: The Foundation for Research on Information Technologies in - Society - accessRights: - read: true - write: false - delete: false - - gid: '16' - label: Blue Fundation - description: Some foundation - accessRights: - read: true - write: false - delete: false - all: - gid: '0' - label: All - description: Open to all users - accessRights: - read: true - write: false - delete: false Annotation: title: Annotation required: @@ -7134,14 +7103,6 @@ components: type: object properties: {} additionalProperties: false - Envelope_AllUsersGroups_: - title: Envelope[AllUsersGroups] - type: object - properties: - data: - $ref: '#/components/schemas/AllUsersGroups' - error: - title: Error Envelope_AnyUrl_: title: Envelope[AnyUrl] type: object @@ -7290,6 +7251,14 @@ components: $ref: '#/components/schemas/GetWalletAutoRecharge' error: title: Error + Envelope_GroupGet_: + title: Envelope[GroupGet] + type: object + properties: + data: + $ref: '#/components/schemas/GroupGet' + error: + title: Error Envelope_GroupUserGet_: title: Envelope[GroupUserGet] type: object @@ -7338,6 +7307,14 @@ components: $ref: '#/components/schemas/LoginNextPage' error: title: Error + Envelope_MyGroupsGet_: + title: Envelope[MyGroupsGet] + type: object + properties: + data: + $ref: '#/components/schemas/MyGroupsGet' + error: + title: Error Envelope_NodeCreated_: title: Envelope[NodeCreated] type: object @@ -7587,14 +7564,6 @@ components: $ref: '#/components/schemas/UserProfile' error: title: Error - Envelope_UsersGroup_: - title: Envelope[UsersGroup] - type: object - properties: - data: - $ref: '#/components/schemas/UsersGroup' - error: - title: Error Envelope_WalletGetWithAvailableCredits_: title: Envelope[WalletGetWithAvailableCredits] type: object @@ -8657,6 +8626,91 @@ components: title: Delete type: boolean description: defines acesss rights for the user + GroupCreate: + title: GroupCreate + required: + - label + - description + type: object + properties: + label: + title: Label + type: string + description: + title: Description + type: string + thumbnail: + title: Thumbnail + maxLength: 65536 + minLength: 1 + type: string + format: uri + GroupGet: + title: GroupGet + required: + - gid + - label + - description + - accessRights + type: object + properties: + gid: + title: Gid + type: integer + description: the group ID + label: + title: Label + type: string + description: the group name + description: + title: Description + type: string + description: the group description + thumbnail: + title: Thumbnail + maxLength: 65536 + minLength: 1 + type: string + description: url to the group thumbnail + format: uri + accessRights: + $ref: '#/components/schemas/GroupAccessRights' + inclusionRules: + title: Inclusionrules + type: object + additionalProperties: + type: string + description: Maps user's column and regular expression + GroupUpdate: + title: GroupUpdate + type: object + properties: + label: + title: Label + type: string + description: + title: Description + type: string + thumbnail: + title: Thumbnail + maxLength: 65536 + minLength: 1 + type: string + format: uri + GroupUserAdd: + title: GroupUserAdd + type: object + properties: + uid: + title: Uid + exclusiveMinimum: true + type: integer + minimum: 0 + email: + title: Email + type: string + format: email + description: "Identify the user with either `email` or `uid` \u2014 only one." GroupUserGet: title: GroupUserGet required: @@ -8701,6 +8755,19 @@ components: read: true write: false delete: false + GroupUserUpdate: + title: GroupUserUpdate + required: + - accessRights + type: object + properties: + accessRights: + $ref: '#/components/schemas/GroupAccessRights' + example: + accessRights: + read: true + write: false + delete: false HardwareInfo: title: HardwareInfo required: @@ -8993,6 +9060,57 @@ components: type: string format: color additionalProperties: false + MyGroupsGet: + title: MyGroupsGet + required: + - me + - all + type: object + properties: + me: + $ref: '#/components/schemas/GroupGet' + organizations: + title: Organizations + type: array + items: + $ref: '#/components/schemas/GroupGet' + all: + $ref: '#/components/schemas/GroupGet' + product: + $ref: '#/components/schemas/GroupGet' + example: + me: + gid: '27' + label: A user + description: A very special user + accessRights: + read: true + write: true + delete: true + organizations: + - gid: '15' + label: ITIS Foundation + description: The Foundation for Research on Information Technologies in + Society + accessRights: + read: true + write: false + delete: false + - gid: '16' + label: Blue Fundation + description: Some foundation + accessRights: + read: true + write: false + delete: false + all: + gid: '0' + label: All + description: Open to all users + accessRights: + read: true + write: false + delete: false NoAuthentication: title: NoAuthentication type: object @@ -10156,7 +10274,7 @@ components: - ADMIN type: string groups: - $ref: '#/components/schemas/AllUsersGroups' + $ref: '#/components/schemas/MyGroupsGet' gravatar_id: title: Gravatar Id type: string @@ -12587,42 +12705,6 @@ components: - DELETED type: string description: An enumeration. - UsersGroup: - title: UsersGroup - required: - - gid - - label - - description - - accessRights - type: object - properties: - gid: - title: Gid - type: integer - description: the group ID - label: - title: Label - type: string - description: the group name - description: - title: Description - type: string - description: the group description - thumbnail: - title: Thumbnail - maxLength: 65536 - minLength: 1 - type: string - description: url to the group thumbnail - format: uri - accessRights: - $ref: '#/components/schemas/GroupAccessRights' - inclusionRules: - title: Inclusionrules - type: object - additionalProperties: - type: string - description: Maps user's column and regular expression Viewer: title: Viewer required: diff --git a/services/web/server/src/simcore_service_webserver/groups/_db.py b/services/web/server/src/simcore_service_webserver/groups/_db.py index 38bbb4e7d7c..d27d7a8a441 100644 --- a/services/web/server/src/simcore_service_webserver/groups/_db.py +++ b/services/web/server/src/simcore_service_webserver/groups/_db.py @@ -7,6 +7,7 @@ from models_library.groups import GroupAtDB from models_library.users import GroupID, UserID from pydantic import parse_obj_as +from simcore_postgres_database.errors import UniqueViolation from simcore_postgres_database.utils_products import get_or_create_product_group from sqlalchemy import and_, literal_column from sqlalchemy.dialects.postgresql import insert @@ -20,7 +21,11 @@ convert_groups_db_to_schema, convert_groups_schema_to_db, ) -from .exceptions import GroupNotFoundError, UserInGroupNotFoundError +from .exceptions import ( + GroupNotFoundError, + UserAlreadyInGroupError, + UserInGroupNotFoundError, +) _DEFAULT_PRODUCT_GROUP_ACCESS_RIGHTS = AccessRightsDict( read=False, @@ -50,7 +55,7 @@ async def _get_user_group( ) group = await result.fetchone() if not group: - raise GroupNotFoundError(gid) + raise GroupNotFoundError(gid=gid) assert isinstance(group, RowProxy) # nosec return group @@ -305,24 +310,31 @@ async def add_new_user_in_group( # first check if the group exists group: RowProxy = await _get_user_group(conn, user_id, gid) check_group_permissions(group, user_id, gid, "write") + # now check the new user exists users_count = await conn.scalar( sa.select(sa.func.count()).where(users.c.id == new_user_id) ) if not users_count: assert new_user_id is not None # nosec - raise UserInGroupNotFoundError(new_user_id, gid) + raise UserInGroupNotFoundError(uid=new_user_id, gid=gid) # add the new user to the group now user_access_rights = _DEFAULT_GROUP_READ_ACCESS_RIGHTS if access_rights: user_access_rights.update(access_rights) - await conn.execute( - # pylint: disable=no-value-for-parameter - user_to_groups.insert().values( - uid=new_user_id, gid=group.gid, access_rights=user_access_rights + + try: + await conn.execute( + # pylint: disable=no-value-for-parameter + user_to_groups.insert().values( + uid=new_user_id, gid=group.gid, access_rights=user_access_rights + ) ) - ) + except UniqueViolation as exc: + raise UserAlreadyInGroupError( + uid=new_user_id, gid=gid, user_id=user_id, access_rights=access_rights + ) from exc async def _get_user_in_group_permissions( @@ -336,7 +348,7 @@ async def _get_user_in_group_permissions( ) the_user: RowProxy | None = await result.fetchone() if not the_user: - raise UserInGroupNotFoundError(the_user_id_in_group, gid) + raise UserInGroupNotFoundError(uid=the_user_id_in_group, gid=gid) return the_user @@ -358,8 +370,12 @@ async def update_user_in_group( user_id: UserID, gid: GroupID, the_user_id_in_group: int, - new_values_for_user_in_group: dict, + access_rights: dict, ) -> dict[str, str]: + if not access_rights: + msg = f"Cannot update empty {access_rights}" + raise ValueError(msg) + # first check if the group exists group: RowProxy = await _get_user_group(conn, user_id, gid) check_group_permissions(group, user_id, gid, "write") @@ -368,7 +384,7 @@ async def update_user_in_group( conn, gid, the_user_id_in_group ) # modify the user access rights - new_db_values = {"access_rights": new_values_for_user_in_group["accessRights"]} + new_db_values = {"access_rights": access_rights} await conn.execute( # pylint: disable=no-value-for-parameter user_to_groups.update() diff --git a/services/web/server/src/simcore_service_webserver/groups/_handlers.py b/services/web/server/src/simcore_service_webserver/groups/_handlers.py index b6284bed8bf..32a9c4c1a40 100644 --- a/services/web/server/src/simcore_service_webserver/groups/_handlers.py +++ b/services/web/server/src/simcore_service_webserver/groups/_handlers.py @@ -5,21 +5,23 @@ from aiohttp import web from models_library.api_schemas_webserver.groups import ( - AllUsersGroups, + GroupCreate, + GroupGet, + GroupUpdate, + GroupUserAdd, GroupUserGet, - UsersGroup, + GroupUserUpdate, + MyGroupsGet, ) -from models_library.emails import LowerCaseEmailStr from models_library.users import GroupID, UserID -from models_library.utils.json_serialization import json_dumps from pydantic import BaseModel, Extra, Field, parse_obj_as from servicelib.aiohttp import status from servicelib.aiohttp.requests_validation import ( + parse_request_body_as, parse_request_path_parameters_as, parse_request_query_parameters_as, ) from servicelib.aiohttp.typing_extension import Handler -from servicelib.mimetype_constants import MIMETYPE_APPLICATION_JSON from .._constants import RQ_PRODUCT_KEY, RQT_USERID_KEY from .._meta import API_VTAG @@ -36,6 +38,7 @@ from ._classifiers import GroupClassifierRepository, build_rrids_tree_view from .exceptions import ( GroupNotFoundError, + UserAlreadyInGroupError, UserInGroupNotFoundError, UserInsufficientRightsError, ) @@ -55,13 +58,21 @@ async def wrapper(request: web.Request) -> web.StreamResponse: return await handler(request) except UserNotFoundError as exc: - raise web.HTTPNotFound(reason=f"User {exc.uid} not found") from exc + raise web.HTTPNotFound( + reason=f"User {exc.uid or exc.email} not found" + ) from exc except GroupNotFoundError as exc: - raise web.HTTPNotFound(reason=f"Group {exc.gid} not found") from exc + gid = getattr(exc, "gid", "") + raise web.HTTPNotFound(reason=f"Group {gid} not found") from exc except UserInGroupNotFoundError as exc: - raise web.HTTPNotFound(reason=f"User not found in group {exc.gid}") from exc + gid = getattr(exc, "gid", "") + raise web.HTTPNotFound(reason=f"User not found in group {gid}") from exc + + except UserAlreadyInGroupError as exc: + gid = getattr(exc, "gid", "") + raise web.HTTPConflict(reason=f"User is already in group {gid}") from exc except UserInsufficientRightsError as exc: raise web.HTTPForbidden from exc @@ -77,11 +88,9 @@ async def wrapper(request: web.Request) -> web.StreamResponse: @permission_required("groups.read") @_handle_groups_exceptions async def list_groups(request: web.Request): - """Lists my groups - - List of the groups I belonged to """ - + List all groups (organizations, primary, everyone and products) I belong to + """ product: Product = get_current_product(request) req_ctx = _GroupsRequestContext.parse_obj(request) @@ -89,7 +98,7 @@ async def list_groups(request: web.Request): request.app, req_ctx.user_id ) - result = { + my_group = { "me": primary_group, "organizations": user_groups, "all": all_group, @@ -98,14 +107,20 @@ async def list_groups(request: web.Request): if product.group_id: with suppress(GroupNotFoundError): - result["product"] = await api.get_product_group_for_user( + # Product is optional + my_group["product"] = await api.get_product_group_for_user( app=request.app, user_id=req_ctx.user_id, product_gid=product.group_id, ) - assert parse_obj_as(AllUsersGroups, result) is not None # nosec - return result + assert parse_obj_as(MyGroupsGet, my_group) is not None # nosec + return envelope_json_response(my_group) + + +# +# Organization groups +# class _GroupPathParams(BaseModel): @@ -120,13 +135,13 @@ class Config: @permission_required("groups.read") @_handle_groups_exceptions async def get_group(request: web.Request): - """Get one group details""" + """Get an organization group""" req_ctx = _GroupsRequestContext.parse_obj(request) path_params = parse_request_path_parameters_as(_GroupPathParams, request) group = await api.get_user_group(request.app, req_ctx.user_id, path_params.gid) - assert parse_obj_as(UsersGroup, group) is not None # nosec - return group + assert parse_obj_as(GroupGet, group) is not None # nosec + return envelope_json_response(group) @routes.post(f"/{API_VTAG}/groups", name="create_group") @@ -134,15 +149,15 @@ async def get_group(request: web.Request): @permission_required("groups.*") @_handle_groups_exceptions async def create_group(request: web.Request): - """Creates organization groups""" + """Creates an organization group""" req_ctx = _GroupsRequestContext.parse_obj(request) - new_group = await request.json() + create = await parse_request_body_as(GroupCreate, request) + new_group = create.dict(exclude_unset=True) created_group = await api.create_user_group(request.app, req_ctx.user_id, new_group) - assert parse_obj_as(UsersGroup, created_group) is not None # nosec - raise web.HTTPCreated( - text=json_dumps({"data": created_group}), content_type=MIMETYPE_APPLICATION_JSON - ) + assert parse_obj_as(GroupGet, created_group) is not None # nosec + + return envelope_json_response(created_group, status_cls=web.HTTPCreated) @routes.patch(f"/{API_VTAG}/groups/{{gid}}", name="update_group") @@ -150,14 +165,16 @@ async def create_group(request: web.Request): @permission_required("groups.*") @_handle_groups_exceptions async def update_group(request: web.Request): + """Updates organization groups""" req_ctx = _GroupsRequestContext.parse_obj(request) path_params = parse_request_path_parameters_as(_GroupPathParams, request) - new_group_values = await request.json() + update: GroupUpdate = await parse_request_body_as(GroupUpdate, request) + new_group_values = update.dict(exclude_unset=True) updated_group = await api.update_user_group( request.app, req_ctx.user_id, path_params.gid, new_group_values ) - assert parse_obj_as(UsersGroup, updated_group) is not None # nosec + assert parse_obj_as(GroupGet, updated_group) is not None # nosec return envelope_json_response(updated_group) @@ -166,6 +183,7 @@ async def update_group(request: web.Request): @permission_required("groups.*") @_handle_groups_exceptions async def delete_group(request: web.Request): + """Deletes organization groups""" req_ctx = _GroupsRequestContext.parse_obj(request) path_params = parse_request_path_parameters_as(_GroupPathParams, request) @@ -173,11 +191,17 @@ async def delete_group(request: web.Request): return web.json_response(status=status.HTTP_204_NO_CONTENT) -@routes.get(f"/{API_VTAG}/groups/{{gid}}/users", name="get_group_users") +# +# Users in organization groups (i.e. members of an organization) +# + + +@routes.get(f"/{API_VTAG}/groups/{{gid}}/users", name="get_all_group_users") @login_required @permission_required("groups.*") @_handle_groups_exceptions -async def get_group_users(request: web.Request): +async def get_all_group_users(request: web.Request): + """Gets users in organization groups""" req_ctx = _GroupsRequestContext.parse_obj(request) path_params = parse_request_path_parameters_as(_GroupPathParams, request) @@ -198,23 +222,14 @@ async def add_group_user(request: web.Request): """ req_ctx = _GroupsRequestContext.parse_obj(request) path_params = parse_request_path_parameters_as(_GroupPathParams, request) - new_user_in_group = await request.json() - - assert "uid" in new_user_in_group or "email" in new_user_in_group # nosec - - new_user_id = new_user_in_group["uid"] if "uid" in new_user_in_group else None - new_user_email = ( - parse_obj_as(LowerCaseEmailStr, new_user_in_group["email"]) - if "email" in new_user_in_group - else None - ) + added: GroupUserAdd = await parse_request_body_as(GroupUserAdd, request) await api.add_user_in_group( request.app, req_ctx.user_id, path_params.gid, - new_user_id=new_user_id, - new_user_email=new_user_email, + new_user_id=added.uid, + new_user_email=added.email, ) return web.json_response(status=status.HTTP_204_NO_CONTENT) @@ -233,7 +248,7 @@ class Config: @_handle_groups_exceptions async def get_group_user(request: web.Request): """ - Gets specific user in group + Gets specific user in an organization group """ req_ctx = _GroupsRequestContext.parse_obj(request) path_params = parse_request_path_parameters_as(_GroupUserPathParams, request) @@ -249,18 +264,16 @@ async def get_group_user(request: web.Request): @permission_required("groups.*") @_handle_groups_exceptions async def update_group_user(request: web.Request): - """ - Modify specific user in group - """ req_ctx = _GroupsRequestContext.parse_obj(request) path_params = parse_request_path_parameters_as(_GroupUserPathParams, request) - new_values_for_user_in_group = await request.json() + update: GroupUserUpdate = await parse_request_body_as(GroupUserUpdate, request) + user = await api.update_user_in_group( request.app, - req_ctx.user_id, - path_params.gid, - path_params.uid, - new_values_for_user_in_group, + user_id=req_ctx.user_id, + gid=path_params.gid, + the_user_id_in_group=path_params.uid, + access_rights=update.access_rights.dict(), ) assert parse_obj_as(GroupUserGet, user) is not None # nosec return envelope_json_response(user) diff --git a/services/web/server/src/simcore_service_webserver/groups/_utils.py b/services/web/server/src/simcore_service_webserver/groups/_utils.py index 163a4923437..4f0f3ad759f 100644 --- a/services/web/server/src/simcore_service_webserver/groups/_utils.py +++ b/services/web/server/src/simcore_service_webserver/groups/_utils.py @@ -24,8 +24,9 @@ def check_group_permissions( group: RowProxy, user_id: int, gid: int, permission: str ) -> None: if not group.access_rights[permission]: - msg = f"User {user_id} has insufficient rights for {permission} access to group {gid}" - raise UserInsufficientRightsError(msg) + raise UserInsufficientRightsError( + user_id=user_id, gid=gid, permission=permission + ) def convert_groups_db_to_schema( diff --git a/services/web/server/src/simcore_service_webserver/groups/api.py b/services/web/server/src/simcore_service_webserver/groups/api.py index e0a837f8399..f2cbeb00094 100644 --- a/services/web/server/src/simcore_service_webserver/groups/api.py +++ b/services/web/server/src/simcore_service_webserver/groups/api.py @@ -139,7 +139,7 @@ async def add_user_in_group( if not new_user_id and not new_user_email: msg = "Invalid method call, missing user id or user email" - raise GroupsError(msg) + raise GroupsError(msg=msg) async with get_database_engine(app).acquire() as conn: if new_user_email: @@ -148,7 +148,7 @@ async def add_user_in_group( if not new_user_id: msg = "Missing new user in arguments" - raise GroupsError(msg) + raise GroupsError(msg=msg) return await _db.add_new_user_in_group( conn, @@ -173,7 +173,7 @@ async def update_user_in_group( user_id: UserID, gid: GroupID, the_user_id_in_group: int, - new_values_for_user_in_group: dict, + access_rights: dict, ) -> dict[str, str]: async with get_database_engine(app).acquire() as conn: return await _db.update_user_in_group( @@ -181,7 +181,7 @@ async def update_user_in_group( user_id=user_id, gid=gid, the_user_id_in_group=the_user_id_in_group, - new_values_for_user_in_group=new_values_for_user_in_group, + access_rights=access_rights, ) diff --git a/services/web/server/src/simcore_service_webserver/groups/exceptions.py b/services/web/server/src/simcore_service_webserver/groups/exceptions.py index 7556a8cae85..f30bf56de30 100644 --- a/services/web/server/src/simcore_service_webserver/groups/exceptions.py +++ b/services/web/server/src/simcore_service_webserver/groups/exceptions.py @@ -4,28 +4,22 @@ class GroupsError(WebServerBaseError): - msg_template = "{msg}" - - def __init__(self, msg: str | None = None): - super().__init__(msg=msg or "Unexpected error occured in projects subpackage") + msg_template = "Groups plugin errored: {msg}" class GroupNotFoundError(GroupsError): msg_template = "Group with id {gid} not found" - def __init__(self, gid, **extras): - super().__init__(**extras) - self.gid = gid - class UserInsufficientRightsError(GroupsError): - ... + msg = ( + "User {user_id} has insufficient rights for {permission} access to group {gid}" + ) class UserInGroupNotFoundError(GroupsError): msg_template = "User id {uid} in Group {gid} not found" - def __init__(self, uid, gid, **extras): - super().__init__(**extras) - self.uid = uid - self.gid = gid + +class UserAlreadyInGroupError(GroupsError): + msg_template = "User `{uid}` is already in Group `{gid}`" diff --git a/services/web/server/src/simcore_service_webserver/users/schemas.py b/services/web/server/src/simcore_service_webserver/users/schemas.py index 53c9ee9b756..ef8973b3abf 100644 --- a/services/web/server/src/simcore_service_webserver/users/schemas.py +++ b/services/web/server/src/simcore_service_webserver/users/schemas.py @@ -3,7 +3,7 @@ from uuid import UUID from models_library.api_schemas_webserver._base import OutputSchema -from models_library.api_schemas_webserver.groups import AllUsersGroups +from models_library.api_schemas_webserver.groups import MyGroupsGet from models_library.api_schemas_webserver.users_preferences import AggregatedPreferences from models_library.emails import LowerCaseEmailStr from models_library.users import FirstNameStr, LastNameStr, UserID @@ -65,7 +65,7 @@ class ProfileGet(BaseModel): last_name: LastNameStr | None = None login: LowerCaseEmailStr role: Literal["ANONYMOUS", "GUEST", "USER", "TESTER", "PRODUCT_OWNER", "ADMIN"] - groups: AllUsersGroups | None = None + groups: MyGroupsGet | None = None gravatar_id: str | None = None expiration_date: date | None = Field( default=None, diff --git a/services/web/server/tests/unit/isolated/test_groups_models.py b/services/web/server/tests/unit/isolated/test_groups_models.py index 10b49979b1c..3a5fd5e91ce 100644 --- a/services/web/server/tests/unit/isolated/test_groups_models.py +++ b/services/web/server/tests/unit/isolated/test_groups_models.py @@ -1,6 +1,6 @@ import models_library.groups import simcore_postgres_database.models.groups -from models_library.api_schemas_webserver.groups import UsersGroup +from models_library.api_schemas_webserver.groups import GroupGet from models_library.utils.enums import enum_to_dict @@ -14,7 +14,7 @@ def test_models_library_and_postgress_database_enums_are_equivalent(): def test_sanitize_legacy_data(): - users_group_1 = UsersGroup.parse_obj( + users_group_1 = GroupGet.parse_obj( { "gid": "27", "label": "A user", @@ -26,7 +26,7 @@ def test_sanitize_legacy_data(): assert users_group_1.thumbnail is None - users_group_2 = UsersGroup.parse_obj( + users_group_2 = GroupGet.parse_obj( { "gid": "27", "label": "A user", diff --git a/services/web/server/tests/unit/with_dbs/01/test_groups.py b/services/web/server/tests/unit/with_dbs/01/test_groups.py index f6e41225ff7..f616dee6110 100644 --- a/services/web/server/tests/unit/with_dbs/01/test_groups.py +++ b/services/web/server/tests/unit/with_dbs/01/test_groups.py @@ -4,7 +4,6 @@ # pylint: disable=unused-argument # pylint: disable=unused-variable -import random from collections.abc import AsyncIterator, Callable from contextlib import AsyncExitStack from copy import deepcopy @@ -160,7 +159,7 @@ async def test_list_groups( ) response = await client.patch( f"{url}", - json={"access_rights": {"read": True, "write": True, "delete": True}}, + json={"accessRights": {"read": True, "write": True, "delete": True}}, ) await assert_status(response, status.HTTP_403_FORBIDDEN) @@ -285,6 +284,7 @@ async def test_add_remove_users_from_group( logged_user: UserInfoDict, user_role: UserRole, expected: ExpectedResponse, + faker: Faker, ): assert client.app new_group = { @@ -295,7 +295,7 @@ async def test_add_remove_users_from_group( } # check that our group does not exist - url = client.app.router["get_group_users"].url_for(gid=new_group["gid"]) + url = client.app.router["get_all_group_users"].url_for(gid=new_group["gid"]) assert f"{url}" == f"/{API_VTAG}/groups/{new_group['gid']}/users" resp = await client.get(f"{url}") data, error = await assert_status(resp, expected.not_found) @@ -323,7 +323,7 @@ async def test_add_remove_users_from_group( assert assigned_group["accessRights"] == _DEFAULT_GROUP_OWNER_ACCESS_RIGHTS # check that our user is in the group of users - get_group_users_url = client.app.router["get_group_users"].url_for( + get_group_users_url = client.app.router["get_all_group_users"].url_for( gid=f"{assigned_group['gid']}" ) assert ( @@ -345,7 +345,7 @@ async def test_add_remove_users_from_group( assert ( f"{add_group_user_url}" == f"/{API_VTAG}/groups/{assigned_group['gid']}/users" ) - num_new_users = random.randint(1, 10) + num_new_users = faker.random_int(1, 10) created_users_list = [] async with AsyncExitStack() as users_stack: