From 3e9b78b02101b4122958682359b704075248a766 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Tue, 10 Dec 2024 14:35:16 +0100 Subject: [PATCH] @odeimaiz review: adds username --- api/specs/web-server/_groups.py | 2 +- .../api_schemas_webserver/groups.py | 3 ++- .../models_library/utils/common_validators.py | 15 +++++++------ .../api/v0/openapi.yaml | 10 ++++++++- .../groups/_groups_api.py | 11 +++++++--- .../groups/_groups_db.py | 18 +++++++++++---- .../groups/_groups_handlers.py | 3 ++- .../tests/unit/isolated/test_groups_models.py | 22 +++++++++++++++++++ 8 files changed, 66 insertions(+), 18 deletions(-) diff --git a/api/specs/web-server/_groups.py b/api/specs/web-server/_groups.py index 85357f2b8c75..7e02bfeb51ca 100644 --- a/api/specs/web-server/_groups.py +++ b/api/specs/web-server/_groups.py @@ -106,7 +106,7 @@ async def add_group_user( _body: GroupUserAdd, ): """ - Adds a user to an organization group + Adds a user to an organization group using their username, user ID, or email (subject to privacy settings """ 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 476ab4aeb57f..51b0b71ea37c 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 @@ -288,6 +288,7 @@ class GroupUserAdd(InputSchema): """ uid: UserID | None = None + user_name: Annotated[IDStr | None, Field(alias="userName")] = None email: Annotated[ LowerCaseEmailStr | None, Field( @@ -296,7 +297,7 @@ class GroupUserAdd(InputSchema): ] = None _check_uid_or_email = model_validator(mode="after")( - create__check_only_one_is_set__root_validator(["uid", "email"]) + create__check_only_one_is_set__root_validator(["uid", "email", "user_name"]) ) model_config = ConfigDict( 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 23cb62739dba..d008f87e8cf1 100644 --- a/packages/models-library/src/models_library/utils/common_validators.py +++ b/packages/models-library/src/models_library/utils/common_validators.py @@ -87,7 +87,9 @@ def null_or_none_str_to_none_validator(value: Any): return value -def create__check_only_one_is_set__root_validator(alternative_field_names: list[str]): +def create__check_only_one_is_set__root_validator( + mutually_exclusive_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 @@ -104,17 +106,16 @@ def create__check_only_one_is_set__root_validator(alternative_field_names: list[ """ def _validator(cls: type[BaseModel], values): - assert set(alternative_field_names).issubset(cls.model_fields) # nosec - + assert set(mutually_exclusive_field_names).issubset( # nosec + cls.model_fields + ), f"Invalid {mutually_exclusive_field_names=} passed in the factory arguments" got = { field_name: getattr(values, field_name) - for field_name in alternative_field_names + for field_name in mutually_exclusive_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}" - ) + msg = f"Either { ' or '.join(got.keys()) } must be set, but not both. Got {got}" raise ValueError(msg) return values 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 ad0fc205f435..c5ff2184202b 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 @@ -634,7 +634,8 @@ paths: tags: - groups summary: Add Group User - description: Adds a user to an organization group + description: Adds a user to an organization group using their username, user + ID, or email (subject to privacy settings operationId: add_group_user parameters: - name: gid @@ -9911,6 +9912,13 @@ components: minimum: 0 - type: 'null' title: Uid + userName: + anyOf: + - type: string + maxLength: 100 + minLength: 1 + - type: 'null' + title: Username email: anyOf: - type: string diff --git a/services/web/server/src/simcore_service_webserver/groups/_groups_api.py b/services/web/server/src/simcore_service_webserver/groups/_groups_api.py index c636a91d66dd..1754cba44b03 100644 --- a/services/web/server/src/simcore_service_webserver/groups/_groups_api.py +++ b/services/web/server/src/simcore_service_webserver/groups/_groups_api.py @@ -1,4 +1,5 @@ from aiohttp import web +from models_library.basic_types import IDStr from models_library.emails import LowerCaseEmailStr from models_library.groups import ( AccessRightsDict, @@ -236,12 +237,17 @@ async def auto_add_user_to_product_group( ) +def _only_one_true(*args): + return sum(bool(arg) for arg in args) == 1 + + async def add_user_in_group( app: web.Application, user_id: UserID, group_id: GroupID, *, new_user_id: UserID | None = None, + new_user_name: IDStr | None = None, new_user_email: EmailStr | None = None, access_rights: AccessRightsDict | None = None, ) -> None: @@ -251,9 +257,8 @@ async def add_user_in_group( UserInGroupNotFoundError GroupsException """ - - if not new_user_id and not new_user_email: - msg = "Invalid method call, missing user id or user email" + if not _only_one_true(new_user_id, new_user_name, new_user_email): + msg = "Invalid method call, required one of these: user id, username or user email, none provided" raise GroupsError(msg=msg) if new_user_email: diff --git a/services/web/server/src/simcore_service_webserver/groups/_groups_db.py b/services/web/server/src/simcore_service_webserver/groups/_groups_db.py index db7981b83504..8a3208205a97 100644 --- a/services/web/server/src/simcore_service_webserver/groups/_groups_db.py +++ b/services/web/server/src/simcore_service_webserver/groups/_groups_db.py @@ -3,6 +3,7 @@ import sqlalchemy as sa from aiohttp import web +from models_library.basic_types import IDStr from models_library.groups import ( AccessRightsDict, Group, @@ -589,7 +590,9 @@ async def add_new_user_in_group( *, user_id: UserID, group_id: GroupID, - new_user_id: UserID, + # either user_id or user_name + new_user_id: UserID | None = None, + new_user_name: IDStr | None = None, access_rights: AccessRightsDict | None = None, ) -> None: """ @@ -602,10 +605,17 @@ async def add_new_user_in_group( ) _check_group_permissions(group, user_id, group_id, "write") + query = sa.select(sa.func.count()) + if new_user_id: + query = query.where(users.c.id == new_user_id) + elif new_user_name: + query = query.where(users.c.name == new_user_name) + else: + msg = "Either user name or id but none provided" + raise ValueError(msg) + # now check the new user exists - users_count = await conn.scalar( - sa.select(sa.func.count()).where(users.c.id == new_user_id) - ) + users_count = await conn.scalar(query) if not users_count: assert new_user_id is not None # nosec raise UserInGroupNotFoundError(uid=new_user_id, gid=group_id) diff --git a/services/web/server/src/simcore_service_webserver/groups/_groups_handlers.py b/services/web/server/src/simcore_service_webserver/groups/_groups_handlers.py index 235925114d90..8f79c18a6399 100644 --- a/services/web/server/src/simcore_service_webserver/groups/_groups_handlers.py +++ b/services/web/server/src/simcore_service_webserver/groups/_groups_handlers.py @@ -163,7 +163,7 @@ async def delete_group(request: web.Request): @login_required @permission_required("groups.*") @handle_plugin_requests_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.model_validate(request) path_params = parse_request_path_parameters_as(GroupsPathParams, request) @@ -194,6 +194,7 @@ async def add_group_user(request: web.Request): req_ctx.user_id, path_params.gid, new_user_id=added.uid, + new_user_name=added.user_name, new_user_email=added.email, ) 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 1a06552e5c16..a45c6f6ffb68 100644 --- a/services/web/server/tests/unit/isolated/test_groups_models.py +++ b/services/web/server/tests/unit/isolated/test_groups_models.py @@ -1,4 +1,5 @@ import models_library.groups +import pytest import simcore_postgres_database.models.groups from faker import Faker from models_library.api_schemas_webserver._base import OutputSchema @@ -6,6 +7,7 @@ GroupCreate, GroupGet, GroupUpdate, + GroupUserAdd, GroupUserGet, ) from models_library.groups import ( @@ -17,6 +19,7 @@ OrganizationUpdate, ) from models_library.utils.enums import enum_to_dict +from pydantic import ValidationError def test_models_library_and_postgress_database_enums_are_equivalent(): @@ -100,3 +103,22 @@ def test_input_schemas_to_models(faker: Faker): domain_model = input_schema.to_model() assert isinstance(domain_model, OrganizationUpdate) assert domain_model.name == input_schema.label + + +def test_group_user_add_options(faker: Faker): + def _only_one_true(*args): + return sum(bool(arg) for arg in args) == 1 + + input_schema = GroupUserAdd(uid=faker.pyint()) + assert input_schema.uid + assert _only_one_true(input_schema.uid, input_schema.user_name, input_schema.email) + + input_schema = GroupUserAdd(userName=faker.user_name()) + assert input_schema.user_name + assert _only_one_true(input_schema.uid, input_schema.user_name, input_schema.email) + + input_schema = GroupUserAdd(email=faker.email()) + assert _only_one_true(input_schema.uid, input_schema.user_name, input_schema.email) + + with pytest.raises(ValidationError): + GroupUserAdd(userName=faker.user_name(), email=faker.email())