From 5ac58b14a37dd19c9edca147196f06afa123d95e Mon Sep 17 00:00:00 2001 From: Omer Zuarets Date: Tue, 22 Oct 2024 15:33:12 +0300 Subject: [PATCH 1/2] add the usage of external data manager in the role assignments API --- horizon/data_manager/policy_store.py | 28 ++++++- horizon/local/api.py | 59 +++++++++------ horizon/tests/test_local_api.py | 109 ++++++++++++++++++++++++--- 3 files changed, 163 insertions(+), 33 deletions(-) diff --git a/horizon/data_manager/policy_store.py b/horizon/data_manager/policy_store.py index b6b8320b..184503cc 100644 --- a/horizon/data_manager/policy_store.py +++ b/horizon/data_manager/policy_store.py @@ -1,5 +1,5 @@ import time -from typing import Optional, Iterator, Callable +from typing import Optional, Iterator, Callable, Any import aiohttp from aiohttp import ClientSession @@ -115,3 +115,29 @@ async def _apply_data_update( f"Data update applied to External Data Manager: status={res.status} duration={elapsed_time_ms:.2f}ms" ) return res + + async def list_facts_by_type( + self, + fact_type: str, + page: int = 1, + per_page: int = 30, + filters: dict[str, Any] | None = None, + ) -> aiohttp.ClientResponse: + logger.info("Performing list facts for '{fact_type}' fact type from the External Data Manager", + fact_type=fact_type) + query_params = { + "page": page, + "per_page": per_page, + } | (filters or {}) + res = await self.client.get( + f"/v1/facts/{fact_type}", + params=query_params, + ) + if res.status != 200: + logger.error( + "Failed to list '{fact_type}' facts from External Data Manager: {res}", + fact_type=fact_type, + res=await res.text(), + ) + res.raise_for_status() + return res diff --git a/horizon/local/api.py b/horizon/local/api.py index 76b30902..1aaff116 100644 --- a/horizon/local/api.py +++ b/horizon/local/api.py @@ -1,6 +1,7 @@ from typing import Any, Dict, List, Optional, cast from fastapi import APIRouter, Depends, HTTPException, status, Query +from loguru import logger from opal_client.policy_store.base_policy_store_client import BasePolicyStoreClient from opal_client.policy_store.policy_store_client_factory import ( DEFAULT_POLICY_STORE_GETTER, @@ -9,14 +10,15 @@ from starlette.responses import Response from horizon.authentication import enforce_pdp_token +from horizon.config import sidecar_config +from horizon.data_manager.policy_store import DataManagerPolicyStoreClient from horizon.local.schemas import ( Message, SyncedRole, - SyncedUser, RoleAssignment, ListRoleAssignmentsFilters, ListRoleAssignmentsPDPBody, - WrappedResponse, + WrappedResponse, ListRoleAssignmentsPagination, ) @@ -90,27 +92,27 @@ async def list_role_assignments( user: Optional[str] = Query( None, description="optional user filter, " - "will only return role assignments granted to this user.", + "will only return role assignments granted to this user.", ), role: Optional[str] = Query( None, description="optional role filter, " - "will only return role assignments granting this role.", + "will only return role assignments granting this role.", ), tenant: Optional[str] = Query( None, description="optional tenant filter, " - "will only return role assignments granted in that tenant.", + "will only return role assignments granted in that tenant.", ), resource: Optional[str] = Query( None, description="optional resource **type** filter, " - "will only return role assignments granted on that resource type.", + "will only return role assignments granted on that resource type.", ), resource_instance: Optional[str] = Query( None, description="optional resource instance filter, " - "will only return role assignments granted on that resource instance.", + "will only return role assignments granted on that resource instance.", ), page: int = Query( default=1, @@ -136,25 +138,38 @@ async def list_role_assignments( resource=resource, resource_instance=resource_instance, ).dict(exclude_none=True) - pagination = ListRoleAssignmentsFilters.construct( + pagination = ListRoleAssignmentsPagination.construct( page=page, per_page=per_page, ) - # the type hint of the get_data_with_input is incorrect, it claims it returns a dict but it - # actually returns a Response - result = cast( - Response | Dict, - await policy_store.get_data_with_input( - "/permit/api/role_assignments/list_role_assignments", - ListRoleAssignmentsPDPBody.construct( - filters=filters, pagination=pagination + async def legacy_list_role_assignments() -> list[RoleAssignment]: + # the type hint of the get_data_with_input is incorrect, it claims it returns a dict but it + # actually returns a Response + result = cast( + Response | Dict, + await policy_store.get_data_with_input( + "/permit/api/role_assignments/list_role_assignments", + ListRoleAssignmentsPDPBody.construct( + filters=filters, pagination=pagination + ), ), - ), - ) - if isinstance(result, Response): - return parse_raw_as(WrappedResponse, result.body).result + ) + if isinstance(result, Response): + return parse_raw_as(WrappedResponse, result.body).result + else: + return parse_obj_as(WrappedResponse, result).result + + if sidecar_config.ENABLE_EXTERNAL_DATA_MANAGER: + if not isinstance(policy_store, DataManagerPolicyStoreClient): + logger.warning( + "External Data Manager is enabled by policy store is not set to {store_type}", + store_type=DataManagerPolicyStoreClient.__name__ + ) + return await legacy_list_role_assignments() + else: + res = await policy_store.list_facts_by_type("role_assignments") + return parse_obj_as(list[RoleAssignment], await res.json()) else: - return parse_obj_as(WrappedResponse, result).result - + return await legacy_list_role_assignments() return router diff --git a/horizon/tests/test_local_api.py b/horizon/tests/test_local_api.py index 17ee3236..3ac08f58 100644 --- a/horizon/tests/test_local_api.py +++ b/horizon/tests/test_local_api.py @@ -1,29 +1,23 @@ -import asyncio -import random -from contextlib import asynccontextmanager - -import aiohttp import pytest from aioresponses import aioresponses from fastapi import FastAPI from fastapi.testclient import TestClient +from loguru import logger from opal_client.client import OpalClient from opal_client.config import opal_client_config -from starlette import status from horizon.config import sidecar_config -from horizon.enforcer.api import stats_manager -from horizon.enforcer.schemas import * +from horizon.data_manager.client import DataManagerClient from horizon.pdp import PermitPDP class MockPermitPDP(PermitPDP): - def __init__(self): + def __init__(self, opal: OpalClient | None = None): self._setup_temp_logger() # sidecar_config.OPA_BEARER_TOKEN_REQUIRED = False # self._configure_inline_opa_config() - self._opal = OpalClient() + self._opal = opal or OpalClient() sidecar_config.API_KEY = "mock_api_key" app: FastAPI = self._opal.app @@ -32,6 +26,13 @@ def __init__(self): self._app: FastAPI = app +class MockDataManagerPermitPDP(MockPermitPDP): + def __init__(self): + super().__init__(opal=DataManagerClient( + shard_id=sidecar_config.SHARD_ID, data_topics=self._fix_data_topics() + )) + + sidecar = MockPermitPDP() @@ -64,6 +65,94 @@ async def test_list_role_assignments() -> None: headers={"authorization": f"Bearer {sidecar_config.API_KEY}"}, ) + m.assert_called_once() + assert response.status_code == 200 + res_json = response.json() + assert len(res_json) == 1 + assert res_json[0] == { + "user": "user1", + "role": "role1", + "tenant": "tenant1", + "resource_instance": "resource_instance1", + } + + +@pytest.mark.asyncio +async def test_list_role_assignments_wrong_data_manager_config() -> None: + _sidecar = MockDataManagerPermitPDP() + # the ENABLE_EXTERNAL_DATA_MANAGER is set to True after the PDP was created + # this causes the PDP to be without the DataManagerPolicyStoreClient - it is a uniquely rare case + # that will probably never happen as this config is managed either by a remote config or env var + sidecar_config.ENABLE_EXTERNAL_DATA_MANAGER = True + _client = TestClient(_sidecar._app) + with aioresponses() as m: + # 'http://localhost:8181/v1/data/permit/api/role_assignments/list_role_assignments' + opa_url = f"{opal_client_config.POLICY_STORE_URL}/v1/data/permit/api/role_assignments/list_role_assignments" + + # Test valid response from OPA + m.post( + opa_url, + status=200, + repeat=True, + payload={ + "result": [ + { + "user": "user1", + "role": "role1", + "tenant": "tenant1", + "resource_instance": "resource_instance1", + } + ] + }, + ) + + response = _client.get( + "/local/role_assignments", + headers={"authorization": f"Bearer {sidecar_config.API_KEY}"}, + ) + + m.assert_called_once() + assert response.status_code == 200 + res_json = response.json() + assert len(res_json) == 1 + assert res_json[0] == { + "user": "user1", + "role": "role1", + "tenant": "tenant1", + "resource_instance": "resource_instance1", + } + + +@pytest.mark.asyncio +async def test_list_role_assignments_external_data_store() -> None: + sidecar_config.ENABLE_EXTERNAL_DATA_MANAGER = True + _sidecar = MockDataManagerPermitPDP() + _client = TestClient(_sidecar._app) + with aioresponses() as m: + # The policy store client of the data manager has base url configured, this means that the url + # we need to mock is '/v1/facts/role_assignments' - without the base url server + data_manager_url = f"/v1/facts/role_assignments?page=1" + logger.info("mocking data manager url: {}", data_manager_url) + # Test valid response from OPA + m.get( + data_manager_url, + status=200, + repeat=True, + payload=[ + { + "user": "user1", + "role": "role1", + "tenant": "tenant1", + "resource_instance": "resource_instance1", + } + ] + ) + + response = _client.get( + "/local/role_assignments", + headers={"authorization": f"Bearer {sidecar_config.API_KEY}"}, + ) + assert response.status_code == 200 res_json = response.json() assert len(res_json) == 1 From 666b0dadf972ab078a66e86fde398bc535a1994a Mon Sep 17 00:00:00 2001 From: Omer Zuarets Date: Sun, 27 Oct 2024 14:46:31 +0200 Subject: [PATCH 2/2] fix pre-commit --- horizon/data_manager/policy_store.py | 12 +++++++----- horizon/local/api.py | 16 +++++++++------- horizon/tests/test_local_api.py | 10 ++++++---- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/horizon/data_manager/policy_store.py b/horizon/data_manager/policy_store.py index 184503cc..07b0bb40 100644 --- a/horizon/data_manager/policy_store.py +++ b/horizon/data_manager/policy_store.py @@ -123,12 +123,14 @@ async def list_facts_by_type( per_page: int = 30, filters: dict[str, Any] | None = None, ) -> aiohttp.ClientResponse: - logger.info("Performing list facts for '{fact_type}' fact type from the External Data Manager", - fact_type=fact_type) + logger.info( + "Performing list facts for '{fact_type}' fact type from the External Data Manager", + fact_type=fact_type, + ) query_params = { - "page": page, - "per_page": per_page, - } | (filters or {}) + "page": page, + "per_page": per_page, + } | (filters or {}) res = await self.client.get( f"/v1/facts/{fact_type}", params=query_params, diff --git a/horizon/local/api.py b/horizon/local/api.py index 1aaff116..a8aaa881 100644 --- a/horizon/local/api.py +++ b/horizon/local/api.py @@ -18,7 +18,8 @@ RoleAssignment, ListRoleAssignmentsFilters, ListRoleAssignmentsPDPBody, - WrappedResponse, ListRoleAssignmentsPagination, + WrappedResponse, + ListRoleAssignmentsPagination, ) @@ -92,27 +93,27 @@ async def list_role_assignments( user: Optional[str] = Query( None, description="optional user filter, " - "will only return role assignments granted to this user.", + "will only return role assignments granted to this user.", ), role: Optional[str] = Query( None, description="optional role filter, " - "will only return role assignments granting this role.", + "will only return role assignments granting this role.", ), tenant: Optional[str] = Query( None, description="optional tenant filter, " - "will only return role assignments granted in that tenant.", + "will only return role assignments granted in that tenant.", ), resource: Optional[str] = Query( None, description="optional resource **type** filter, " - "will only return role assignments granted on that resource type.", + "will only return role assignments granted on that resource type.", ), resource_instance: Optional[str] = Query( None, description="optional resource instance filter, " - "will only return role assignments granted on that resource instance.", + "will only return role assignments granted on that resource instance.", ), page: int = Query( default=1, @@ -164,7 +165,7 @@ async def legacy_list_role_assignments() -> list[RoleAssignment]: if not isinstance(policy_store, DataManagerPolicyStoreClient): logger.warning( "External Data Manager is enabled by policy store is not set to {store_type}", - store_type=DataManagerPolicyStoreClient.__name__ + store_type=DataManagerPolicyStoreClient.__name__, ) return await legacy_list_role_assignments() else: @@ -172,4 +173,5 @@ async def legacy_list_role_assignments() -> list[RoleAssignment]: return parse_obj_as(list[RoleAssignment], await res.json()) else: return await legacy_list_role_assignments() + return router diff --git a/horizon/tests/test_local_api.py b/horizon/tests/test_local_api.py index 3ac08f58..5cca027a 100644 --- a/horizon/tests/test_local_api.py +++ b/horizon/tests/test_local_api.py @@ -28,9 +28,11 @@ def __init__(self, opal: OpalClient | None = None): class MockDataManagerPermitPDP(MockPermitPDP): def __init__(self): - super().__init__(opal=DataManagerClient( - shard_id=sidecar_config.SHARD_ID, data_topics=self._fix_data_topics() - )) + super().__init__( + opal=DataManagerClient( + shard_id=sidecar_config.SHARD_ID, data_topics=self._fix_data_topics() + ) + ) sidecar = MockPermitPDP() @@ -145,7 +147,7 @@ async def test_list_role_assignments_external_data_store() -> None: "tenant": "tenant1", "resource_instance": "resource_instance1", } - ] + ], ) response = _client.get(