♻️ Refactor access rights evaluation functions for clarity and consistency

Author: pcrespov

services/catalog/src/simcore_service_catalog/core/background_tasks.py

@@ -76,11 +76,13 @@ def _by_version(t: tuple[ServiceKey, ServiceVersion]) -> Version:
             (
                 owner_gid,
                 service_access_rights,
-            ) = await access_rights.evaluate_default_policy(app, service_metadata)
+            ) = await access_rights.evaluate_service_ownership_and_rights(
+                app, service_metadata
+            )
 
             # AUTO-UPGRADE PATCH policy
             inherited_access_rights = await access_rights.evaluate_auto_upgrade_policy(
-                service_metadata, services_repo
+                service_metadata=service_metadata, services_repo=services_repo
             )
 
             service_access_rights += inherited_access_rights

services/catalog/src/simcore_service_catalog/service/access_rights.py

@@ -8,6 +8,7 @@
 
 import arrow
 from fastapi import FastAPI
+from models_library.groups import GroupID
 from models_library.services import ServiceMetaDataPublished
 from models_library.services_types import ServiceKey, ServiceVersion
 from packaging.version import Version
@@ -41,21 +42,32 @@ async def _is_old_service(app: FastAPI, service: ServiceMetaDataPublished) -> bo
     return bool(service_build_data < _LEGACY_SERVICES_DATE)
 
 
-async def evaluate_default_policy(
+async def evaluate_service_ownership_and_rights(
     app: FastAPI, service: ServiceMetaDataPublished
-) -> tuple[PositiveInt | None, list[ServiceAccessRightsDB]]:
-    """Given a service, it returns the owner's group-id (gid) and a list of access rights following
-    default access-rights policies
-
-    - DEFAULT Access Rights policies:
-        1. All services published in osparc prior 19.08.2020 will be visible to everyone (refered as 'old service').
-        2. Services published after 19.08.2020 will be visible ONLY to his/her owner
-        3. Front-end services are have execute-access to everyone
+) -> tuple[GroupID | None, list[ServiceAccessRightsDB]]:
+    """Evaluates the owner (group_id) and the access rights for a service
+
+    This function determines:
+    1. Who owns the service (based on contact or author email)
+    2. Who can access the service based on the following rules:
+       - All services published before August 19, 2020 (_LEGACY_SERVICES_DATE) are accessible to everyone
+       - Services published after August 19, 2020 are only accessible to their owner
+       - Frontend services are accessible to everyone regardless of publication date
+
+    Args:
+        app: FastAPI application instance containing database engine and settings
+        service: Service metadata including key, version, contact and authors information
+
+    Returns:
+        A tuple containing:
+        - The owner's group ID (gid) if found, None otherwise
+        - A list of ServiceAccessRightsDB objects representing the default access rights
+          for the service, including who can execute and/or modify the service
 
     Raises:
-        HTTPException: from calls to director's rest API. Maps director errors into catalog's server error
-        SQLAlchemyError: from access to pg database
-        ValidationError: from pydantic model errors
+        HTTPException: If there's an error communicating with the director API
+        SQLAlchemyError: If there's an error accessing the database
+        ValidationError: If there's an error validating the Pydantic models
     """
     db_engine: AsyncEngine = app.state.engine
 
@@ -83,7 +95,7 @@ async def evaluate_default_policy(
         if possible_gid and not owner_gid:
             owner_gid = possible_gid
     if not owner_gid:
-        _logger.warning("service %s:%s has no owner", service.key, service.version)
+        _logger.warning("Service %s:%s has no owner", service.key, service.version)
     else:
         group_ids.append(owner_gid)
 
@@ -106,16 +118,29 @@ async def evaluate_default_policy(
 
 
 async def evaluate_auto_upgrade_policy(
-    service_metadata: ServiceMetaDataPublished, services_repo: ServicesRepository
+    services_repo: ServicesRepository, *, service_metadata: ServiceMetaDataPublished
 ) -> list[ServiceAccessRightsDB]:
-    # AUTO-UPGRADE PATCH policy:
-    #
-    #  - Any new patch released, inherits the access rights from previous compatible version
-    #  - IDEA: add as option in the publication contract, i.e. in ServiceDockerData?
-    #  - Does NOT apply to front-end services
-    #
-    # SEE https://github.com/ITISFoundation/osparc-simcore/issues/2244)
-    #
+    """
+    Evaluates the access rights for a service based on the auto-upgrade patch policy.
+
+    The AUTO-UPGRADE PATCH policy ensures that:
+      - Any new patch release of a service automatically inherits the access rights from the previous compatible version.
+      - This policy does NOT apply to frontend services.
+
+    Args:
+        services_repo: Instance of ServicesRepository for database access.
+        service_metadata: Metadata of the service being evaluated.
+
+    Returns:
+        A list of ServiceAccessRightsDB objects representing the inherited access rights for the new patch version.
+        Returns an empty list if the service is a frontend service or if no previous compatible version is found.
+
+    Notes:
+        - The policy is described in https://github.com/ITISFoundation/osparc-simcore/issues/2244
+        - Inheritance is only for patch releases (i.e., same major and minor version).
+        - Future improvement: Consider making this behavior configurable in the service publication contract.
+
+    """
     if _is_frontend_service(service_metadata):
         return []
 
@@ -129,8 +154,8 @@ async def evaluate_auto_upgrade_policy(
 
     previous_release = None
     for release in latest_releases:
-        # NOTE: latest_release is sorted from newer to older
-        # Here we search for the previous version patched by new-version
+        # latest_releases is sorted from newer to older
+        # Find the previous version that is patched by new_version
         if is_patch_release(new_version, release.version):
             previous_release = release
             break

services/catalog/tests/unit/with_dbs/conftest.py

@@ -168,13 +168,13 @@ async def other_user(
     faker: Faker,
 ) -> AsyncIterator[dict[str, Any]]:
 
-    _user = random_user(fake=faker, id=user_id + 1)
+    _other_user = random_user(fake=faker, id=user_id + 1)
     async with insert_and_get_row_lifespan(  # pylint:disable=contextmanager-generator-missing-cleanup
         sqlalchemy_async_engine,
         table=users,
-        values=_user,
+        values=_other_user,
         pk_col=users.c.id,
-        pk_value=_user["id"],
+        pk_value=_other_user["id"],
     ) as row:
         yield row
 

services/catalog/tests/unit/with_dbs/test_service_access_rights.py

@@ -6,15 +6,16 @@
 
 import simcore_service_catalog.service.access_rights
 from fastapi import FastAPI
-from models_library.groups import GroupAtDB
+from models_library.groups import GroupAtDB, GroupID
 from models_library.products import ProductName
 from models_library.services import ServiceMetaDataPublished, ServiceVersion
 from pydantic import TypeAdapter
+from pytest_mock import MockerFixture
 from simcore_service_catalog.models.services_db import ServiceAccessRightsDB
 from simcore_service_catalog.repository.services import ServicesRepository
 from simcore_service_catalog.service.access_rights import (
     evaluate_auto_upgrade_policy,
-    evaluate_default_policy,
+    evaluate_service_ownership_and_rights,
     reduce_access_rights,
 )
 from sqlalchemy.ext.asyncio import AsyncEngine
@@ -84,12 +85,12 @@ def test_reduce_access_rights():
 
 async def test_auto_upgrade_policy(
     sqlalchemy_async_engine: AsyncEngine,
-    user_groups_ids: list[int],
+    user_groups_ids: list[GroupID],
     target_product: ProductName,
     other_product: ProductName,
     services_db_tables_injector: Callable,
     create_fake_service_data: Callable,
-    mocker,
+    mocker: MockerFixture,
 ):
     everyone_gid, user_gid, team_gid = user_groups_ids
 
@@ -165,7 +166,7 @@ async def test_auto_upgrade_policy(
     services_repo = ServicesRepository(app.state.engine)
 
     # DEFAULT policies
-    owner_gid, service_access_rights = await evaluate_default_policy(
+    owner_gid, service_access_rights = await evaluate_service_ownership_and_rights(
         app, new_service_metadata
     )
     assert owner_gid == user_gid