🎨 webserver: cache authenticated calls (#7826)

Author: pcrespov

.github/copilot-instructions.md

@@ -52,4 +52,4 @@ This document provides guidelines and best practices for using GitHub Copilot in
 - [Python Coding Conventions](../docs/coding-conventions.md)
 - [Environment Variables Guide](../docs/env-vars.md)
 - [Steps to Upgrade Python](../docs/steps-to-upgrade-python.md)
-- [Node.js Installation Script](../scripts/install_nodejs_14.bash)
+- [Pydantic Annotated fields](../docs/llm-prompts/pydantic-annotated-fields.md)

services/web/server/src/simcore_service_webserver/security/_authz_db.py

@@ -2,15 +2,14 @@
 from typing import TypedDict
 
 import sqlalchemy as sa
-from aiopg.sa import Engine
-from aiopg.sa.result import ResultProxy
 from models_library.basic_types import IdInt
 from models_library.products import ProductName
 from models_library.users import UserID
 from pydantic import TypeAdapter
 from simcore_postgres_database.models.groups import user_to_groups
 from simcore_postgres_database.models.products import products
 from simcore_postgres_database.models.users import UserRole
+from sqlalchemy.ext.asyncio import AsyncEngine
 
 from ..db.models import UserStatus, users
 
@@ -22,33 +21,38 @@ class AuthInfoDict(TypedDict, total=True):
     role: UserRole
 
 
-async def get_active_user_or_none(engine: Engine, email: str) -> AuthInfoDict | None:
+async def get_active_user_or_none(
+    engine: AsyncEngine, *, email: str
+) -> AuthInfoDict | None:
     """Gets a user with email if ACTIVE othewise return None
 
     Raises:
-        DatabaseError: unexpected errors found in https://github.com/ITISFoundation/osparc-simcore/issues/880 and https://github.com/ITISFoundation/osparc-simcore/pull/1160
+        DatabaseError: unexpected errors found in
+        https://github.com/ITISFoundation/osparc-simcore/issues/880 and
+        https://github.com/ITISFoundation/osparc-simcore/pull/1160
     """
-    async with engine.acquire() as conn:
-        result: ResultProxy = await conn.execute(
+    async with engine.connect() as conn:
+        result = await conn.execute(
             sa.select(users.c.id, users.c.role).where(
                 (users.c.email == email) & (users.c.status == UserStatus.ACTIVE)
             )
         )
-        row = await result.fetchone()
-        assert (
-            row is None or TypeAdapter(IdInt).validate_python(row.id) is not None   # nosec
+        row = result.one_or_none()
+
+        assert (  # nosec
+            row is None or TypeAdapter(IdInt).validate_python(row.id) is not None
         )
-        assert (
-            row is None or TypeAdapter(UserRole).validate_python(row.role) is not None  # nosec
+        assert (  # nosec
+            row is None or TypeAdapter(UserRole).validate_python(row.role) is not None
         )
 
         return AuthInfoDict(id=row.id, role=row.role) if row else None
 
 
 async def is_user_in_product_name(
-    engine: Engine, user_id: UserID, product_name: ProductName
+    engine: AsyncEngine, *, user_id: UserID, product_name: ProductName
 ) -> bool:
-    async with engine.acquire() as conn:
+    async with engine.connect() as conn:
         return (
             await conn.scalar(
                 sa.select(users.c.id)

services/web/server/src/simcore_service_webserver/security/_authz_policy.py

@@ -12,9 +12,11 @@
 )
 from models_library.products import ProductName
 from models_library.users import UserID
-from simcore_postgres_database.aiopg_errors import DatabaseError
+from servicelib.aiohttp.db_asyncpg_engine import get_async_engine
+from servicelib.logging_errors import create_troubleshotting_log_kwargs
+from simcore_postgres_database.aiopg_errors import DatabaseError as AiopgDatabaseError
+from sqlalchemy.exc import DatabaseError as SQLAlchemyDatabaseError
 
-from ..db.plugin import get_database_engine
 from ._authz_access_model import (
     AuthContextDict,
     OptionalContext,
@@ -27,17 +29,31 @@
 
 _logger = logging.getLogger(__name__)
 
-# Keeps a cache during bursts to avoid stress on the database
 _SECOND = 1  # in seconds
-_AUTHZ_BURST_CACHE_TTL: Final = 5 * _SECOND
+_MINUTE: Final = 60 * _SECOND
+_AUTHZ_BURST_CACHE_TTL: Final = (
+    # WARNING: TLL=0 means it never expires
+    # Rationale:
+    #   a user's access to a product does not change that frequently
+    #   Keeps a cache during bursts to avoid stress on the database
+    30
+    * _MINUTE
+)
 
 
 @contextlib.contextmanager
 def _handle_exceptions_as_503():
     try:
         yield
-    except DatabaseError as err:
-        _logger.exception("Auth unavailable due to database error")
+    except (AiopgDatabaseError, SQLAlchemyDatabaseError) as err:
+        _logger.exception(
+            **create_troubleshotting_log_kwargs(
+                "Auth unavailable due to database error",
+                error=err,
+                tip="Check database connection",
+            )
+        )
+
         raise web.HTTPServiceUnavailable(text=MSG_AUTH_NOT_AVAILABLE) from err
 
 
@@ -57,7 +73,9 @@ async def _get_auth_or_none(self, *, email: str) -> AuthInfoDict | None:
             web.HTTPServiceUnavailable: if database raises an exception
         """
         with _handle_exceptions_as_503():
-            return await get_active_user_or_none(get_database_engine(self._app), email)
+            return await get_active_user_or_none(
+                get_async_engine(self._app), email=email
+            )
 
     @cached(
         ttl=_AUTHZ_BURST_CACHE_TTL,
@@ -73,7 +91,7 @@ async def _has_access_to_product(
         """
         with _handle_exceptions_as_503():
             return await is_user_in_product_name(
-                get_database_engine(self._app), user_id, product_name
+                get_async_engine(self._app), user_id=user_id, product_name=product_name
             )
 
     @property
@@ -117,11 +135,6 @@ async def permits(
         :return: True if user has permission to execute this operation within the given context
         """
         if identity is None or permission is None:
-            _logger.debug(
-                "Invalid %s of %s. Denying access.",
-                f"{identity=}",
-                f"{permission=}",
-            )
             return False
 
         auth_info = await self._get_auth_or_none(email=identity)

services/web/server/tests/unit/isolated/test_security__authz.py

@@ -243,7 +243,7 @@ async def test_check_access_expressions(access_model: RoleBasedAccessModel):
 def mock_db(mocker: MockerFixture) -> MagicMock:
 
     mocker.patch(
-        "simcore_service_webserver.security._authz_policy.get_database_engine",
+        "simcore_service_webserver.security._authz_policy.get_async_engine",
         autospec=True,
         return_value="FAKE-ENGINE",
     )

services/web/server/tests/unit/isolated/test_security_api.py

@@ -243,7 +243,7 @@ async def basic_db_funs_mocked(client: TestClient, mocker: MockerFixture) -> Non
     await clean_auth_policy_cache(client.app)
 
     mocker.patch(
-        "simcore_service_webserver.security._authz_policy.get_database_engine",
+        "simcore_service_webserver.security._authz_policy.get_async_engine",
         autospec=True,
     )
 

services/web/server/tests/unit/with_dbs/03/login/test_login_auth.py

@@ -2,12 +2,15 @@
 # pylint: disable=unused-argument
 # pylint: disable=unused-variable
 
+import asyncio
 import json
 import time
+from collections.abc import AsyncIterator, Callable
+from contextlib import AsyncExitStack
 from http import HTTPStatus
 
 import pytest
-from aiohttp.test_utils import TestClient
+from aiohttp.test_utils import TestClient, TestServer
 from cryptography import fernet
 from faker import Faker
 from pytest_simcore.helpers.assert_checks import assert_status
@@ -198,3 +201,73 @@ def _build_proxy_session_cookie(identity: str):
 
         if not error:
             assert data["login"] == user["email"]
+
+
+@pytest.fixture
+async def multiple_users(
+    client: TestClient, num_users: int = 5
+) -> AsyncIterator[list[dict[str, str]]]:
+    """Fixture that creates multiple test users with an AsyncExitStack for cleanup."""
+    async with AsyncExitStack() as exit_stack:
+        users = []
+        for _ in range(num_users):
+            # Use enter_async_context to properly register each NewUser context manager
+            user_ctx = await exit_stack.enter_async_context(NewUser(app=client.app))
+            users.append(
+                {
+                    "email": user_ctx["email"],
+                    "password": user_ctx["raw_password"],
+                }
+            )
+
+        yield users
+        # AsyncExitStack will automatically clean up all users when exiting
+
+
+async def test_multiple_users_login_logout_concurrently(
+    web_server: TestServer,
+    client: TestClient,
+    multiple_users: list[dict[str, str]],
+    aiohttp_client: Callable,
+):
+    """Test multiple users can login concurrently and properly get logged out."""
+    assert client.app
+
+    # URLs
+    login_url = client.app.router["auth_login"].url_for().path
+    profile_url = client.app.router["get_my_profile"].url_for().path
+    logout_url = client.app.router["auth_logout"].url_for().path
+
+    async def user_session_flow(user_creds):
+        # Create a new client for each user to ensure isolated sessions
+        user_client = await aiohttp_client(web_server)
+
+        # Login
+        login_resp = await user_client.post(
+            login_url,
+            json={"email": user_creds["email"], "password": user_creds["password"]},
+        )
+        login_data, _ = await assert_status(login_resp, status.HTTP_200_OK)
+        assert MSG_LOGGED_IN in login_data["message"]
+
+        # Access profile (cookies are automatically sent by the client)
+        profile_resp = await user_client.get(profile_url)
+        profile_data, _ = await assert_status(profile_resp, status.HTTP_200_OK)
+        assert profile_data["login"] == user_creds["email"]
+
+        # Logout
+        logout_resp = await user_client.post(logout_url)
+        await assert_status(logout_resp, status.HTTP_200_OK)
+
+        # Try to access profile after logout
+        profile_after_logout_resp = await user_client.get(profile_url)
+        _, error = await assert_status(
+            profile_after_logout_resp, status.HTTP_401_UNAUTHORIZED
+        )
+
+        # No need to manually close the client as aiohttp_client fixture handles cleanup
+
+    await user_session_flow(multiple_users[0])
+
+    # Run all user flows concurrently
+    await asyncio.gather(*(user_session_flow(user) for user in multiple_users))

services/web/server/tests/unit/with_dbs/03/test_users_rest_profiles.py

@@ -36,6 +36,8 @@
 from simcore_service_webserver.users._preferences_service import (
     get_frontend_user_preferences_aggregation,
 )
+from sqlalchemy.exc import OperationalError as SQLAlchemyOperationalError
+from sqlalchemy.ext.asyncio import AsyncConnection
 
 
 @pytest.fixture
@@ -554,6 +556,14 @@ def mock_failing_database_connection(mocker: Mock) -> MagicMock:
     conn_execute.side_effect = OperationalError(
         "MOCK: server closed the connection unexpectedly"
     )
+
+    aysncpg_conn_execute = mocker.patch.object(AsyncConnection, "execute")
+    aysncpg_conn_execute.side_effect = SQLAlchemyOperationalError(
+        statement="MOCK statement",
+        params=(),
+        orig=OperationalError("MOCK: server closed the connection unexpectedly"),
+    )
+
     return conn_execute