From f4ad7467bd77fc8bafc2eaa54b06565cd80b64e2 Mon Sep 17 00:00:00 2001 From: Catherine Smith Date: Fri, 22 Nov 2024 22:35:01 +0100 Subject: [PATCH] LA-165: Fix taxonomy create endpoints (#5533) --- .../api/api/v1/endpoints/generic_overrides.py | 131 +++++++++++----- src/fides/api/schemas/taxonomy_extensions.py | 29 +++- tests/ctl/core/test_api.py | 127 ---------------- .../v1/endpoints/test_taxonomy_overrides.py | 141 ++++++++++++++++++ 4 files changed, 260 insertions(+), 168 deletions(-) create mode 100644 tests/ops/api/v1/endpoints/test_taxonomy_overrides.py diff --git a/src/fides/api/api/v1/endpoints/generic_overrides.py b/src/fides/api/api/v1/endpoints/generic_overrides.py index 9fbde4b3e9..0635810f9f 100644 --- a/src/fides/api/api/v1/endpoints/generic_overrides.py +++ b/src/fides/api/api/v1/endpoints/generic_overrides.py @@ -1,14 +1,18 @@ -from typing import Dict, List, Optional, Type, Union +from typing import Dict, List, Optional, Union -from fastapi import APIRouter, Depends, Query, Security +from fastapi import APIRouter, Depends, HTTPException, Query, Security from fastapi_pagination import Page, Params from fastapi_pagination.ext.async_sqlalchemy import paginate as async_paginate from fideslang.models import Dataset from sqlalchemy import not_ from sqlalchemy.ext.asyncio import AsyncSession +from sqlalchemy.orm import Session from sqlalchemy.sql.expression import select from starlette import status +from starlette.status import HTTP_422_UNPROCESSABLE_ENTITY +from fides.api.api.deps import get_db +from fides.api.common_exceptions import KeyOrNameAlreadyExists from fides.api.db.base_class import get_key_from_data from fides.api.db.crud import list_resource_query from fides.api.db.ctl_session import get_async_db @@ -16,7 +20,14 @@ from fides.api.models.datasetconfig import DatasetConfig from fides.api.oauth.utils import verify_oauth_client from fides.api.schemas.filter_params import FilterParams -from fides.api.schemas.taxonomy_extensions import DataCategory, DataSubject, DataUse +from fides.api.schemas.taxonomy_extensions import ( + DataCategory, + DataCategoryCreate, + DataSubject, + DataSubjectCreate, + DataUse, + DataUseCreate, +) from fides.api.util.filter_utils import apply_filters_to_query from fides.common.api.scope_registry import ( DATA_CATEGORY_CREATE, @@ -28,6 +39,9 @@ from fides.api.models.sql_models import ( # type: ignore[attr-defined] # isort: skip Dataset as CtlDataset, + DataCategory as DataCategoryDbModel, + DataSubject as DataSubjectDbModel, + DataUse as DataUseDbModel, ) # We create routers to override specific methods in those defined in generic.py @@ -96,28 +110,6 @@ async def list_dataset_paginated( return await async_paginate(db, filtered_query, pagination_params) -async def create_with_key( - data: Union[DataUse, DataCategory, DataSubject], - model: Type[Union[DataUse, DataCategory, DataSubject]], - db: AsyncSession, -) -> Dict: - """ - Helper to create taxonomy resource when not given a fides_key. - Automatically re-enables disabled resources with the same name. - """ - # If data with same name exists but is disabled, re-enable it - disabled_resource_with_name = db.query(model).filter( - model.key == data.name, # type: ignore[union-attr] - model.active is False, - ) - if disabled_resource_with_name: - return model.update(db=db, data=data, active=True) # type: ignore[union-attr] - data.fides_key = get_key_from_data( - {"key": data.fides_key, "name": data.name}, model.__name__ - ) - return model.create(db=db, data=data.model_dump(mode="json")) # type: ignore[union-attr] - - @data_use_router.post( "/data_use", dependencies=[Security(verify_oauth_client, scopes=[DATA_USE_CREATE])], @@ -126,16 +118,35 @@ async def create_with_key( name="Create", ) async def create_data_use( - data_use: DataUse, - db: AsyncSession = Depends(get_async_db), + data_use: DataUseCreate, + db: Session = Depends(get_db), ) -> Dict: """ Create a data use. Updates existing data use if data use with name already exists and is disabled. """ if data_use.fides_key is None: - await create_with_key(data_use, DataUse, db) - - return await DataUse.create(db=db, data=data_use.model_dump(mode="json")) # type: ignore[attr-defined] + disabled_resource_with_name = ( + db.query(DataUseDbModel) + .filter( + DataUseDbModel.active.is_(False), + DataUseDbModel.name == data_use.name, + ) + .first() + ) + data_use.fides_key = get_key_from_data( + {"key": data_use.fides_key, "name": data_use.name}, DataUse.__name__ + ) + if disabled_resource_with_name: + data_use.active = True + return disabled_resource_with_name.update(db, data=data_use.model_dump(mode="json")) # type: ignore[union-attr] + try: + return DataUseDbModel.create(db=db, data=data_use.model_dump(mode="json")) # type: ignore[union-attr] + except KeyOrNameAlreadyExists: + raise HTTPException( + status_code=HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Data use with key {data_use.fides_key} or name {data_use.name} already exists.", + ) + return DataUseDbModel.create(db=db, data=data_use.model_dump(mode="json")) @data_category_router.post( @@ -146,17 +157,37 @@ async def create_data_use( name="Create", ) async def create_data_category( - data_category: DataCategory, - db: AsyncSession = Depends(get_async_db), + data_category: DataCategoryCreate, + db: Session = Depends(get_db), ) -> Dict: """ Create a data category """ if data_category.fides_key is None: - await create_with_key(data_category, DataCategory, db) - - return await DataCategory.create(db=db, data=data_category.model_dump(mode="json")) # type: ignore[attr-defined] + disabled_resource_with_name = ( + db.query(DataCategoryDbModel) + .filter( + DataCategoryDbModel.active.is_(False), + DataCategoryDbModel.name == data_category.name, + ) + .first() + ) + data_category.fides_key = get_key_from_data( + {"key": data_category.fides_key, "name": data_category.name}, + DataCategory.__name__, + ) + if disabled_resource_with_name: + data_category.active = True + return disabled_resource_with_name.update(db, data=data_category.model_dump(mode="json")) # type: ignore[union-attr] + try: + return DataCategoryDbModel.create(db=db, data=data_category.model_dump(mode="json")) # type: ignore[union-attr] + except KeyOrNameAlreadyExists: + raise HTTPException( + status_code=HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Data category with key {data_category.fides_key} or name {data_category.name} already exists.", + ) + return DataCategoryDbModel.create(db=db, data=data_category.model_dump(mode="json")) @data_subject_router.post( @@ -167,17 +198,37 @@ async def create_data_category( name="Create", ) async def create_data_subject( - data_subject: DataSubject, - db: AsyncSession = Depends(get_async_db), + data_subject: DataSubjectCreate, + db: Session = Depends(get_db), ) -> Dict: """ Create a data subject """ if data_subject.fides_key is None: - await create_with_key(data_subject, DataSubject, db) - - return await DataSubject.create(db=db, data=data_subject.model_dump(mode="json")) # type: ignore[attr-defined] + disabled_resource_with_name = ( + db.query(DataSubjectDbModel) + .filter( + DataSubjectDbModel.active.is_(False), + DataSubjectDbModel.name == data_subject.name, + ) + .first() + ) + data_subject.fides_key = get_key_from_data( + {"key": data_subject.fides_key, "name": data_subject.name}, + DataSubject.__name__, + ) + if disabled_resource_with_name: + data_subject.active = True + return disabled_resource_with_name.update(db, data=data_subject.model_dump(mode="json")) # type: ignore[union-attr] + try: + return DataSubjectDbModel.create(db=db, data=data_subject.model_dump(mode="json")) # type: ignore[union-attr] + except KeyOrNameAlreadyExists: + raise HTTPException( + status_code=HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Data subject with key {data_subject.fides_key} or name {data_subject.name} already exists.", + ) + return DataSubjectDbModel.create(db=db, data=data_subject.model_dump(mode="json")) GENERIC_OVERRIDES_ROUTER = APIRouter() diff --git a/src/fides/api/schemas/taxonomy_extensions.py b/src/fides/api/schemas/taxonomy_extensions.py index 0fdc4c9d0d..2fb5614006 100644 --- a/src/fides/api/schemas/taxonomy_extensions.py +++ b/src/fides/api/schemas/taxonomy_extensions.py @@ -2,10 +2,14 @@ Fides-specific extensions to the pydantic models of taxonomy elements as defined in fideslang. """ +from typing import List, Optional + from fideslang.models import DataCategory as BaseDataCategory from fideslang.models import DataSubject as BaseDataSubject +from fideslang.models import DataSubjectRights from fideslang.models import DataUse as BaseDataUse -from pydantic import Field +from fideslang.validation import FidesKey +from pydantic import BaseModel, Field active_field = Field( default=True, description="Indicates whether the resource is currently 'active'." @@ -22,3 +26,26 @@ class DataCategory(BaseDataCategory): class DataSubject(BaseDataSubject): active: bool = active_field + + +class TaxonomyCreateBase(BaseModel): + name: Optional[str] = None + description: str + active: bool = True + fides_key: Optional[FidesKey] = None + is_default: bool = False + tags: Optional[List[str]] = None + organization_fides_key: Optional[FidesKey] = "default_organization" + + +class DataUseCreate(TaxonomyCreateBase): + parent_key: Optional[FidesKey] = None + + +class DataCategoryCreate(TaxonomyCreateBase): + parent_key: Optional[FidesKey] = None + + +class DataSubjectCreate(TaxonomyCreateBase): + rights: Optional[DataSubjectRights] = None + automated_decisions_or_profiling: Optional[bool] = None diff --git a/tests/ctl/core/test_api.py b/tests/ctl/core/test_api.py index f63c9d037f..0cdb9e2419 100644 --- a/tests/ctl/core/test_api.py +++ b/tests/ctl/core/test_api.py @@ -3022,133 +3022,6 @@ def test_system_manager_gets_403_if_system_not_found( assert result.status_code == HTTP_403_FORBIDDEN -@pytest.mark.integration -class TestDefaultTaxonomyCrudOverrides: - @pytest.mark.parametrize("endpoint", TAXONOMY_ENDPOINTS) - def test_api_cannot_create_if_generated_fides_key_conflicts_with_existing( - self, - test_config: FidesConfig, - generate_auth_header, - endpoint: str, - ) -> None: - """Ensure we cannot create taxonomy elements if fides key conflicts with existing key""" - # get a default taxonomy element as a sample resource - resource = getattr(DEFAULT_TAXONOMY, endpoint)[0] - resource = TAXONOMY_EXTENSIONS[endpoint]( - **resource.model_dump(mode="json") - ) # cast resource to extended model - # This name will conflict with existing resource - resource.name = resource.fides_key - resource.fides_key = None - json_resource = resource.json(exclude_none=True) - token_scopes: List[str] = [f"{CLI_SCOPE_PREFIX_MAPPING[endpoint]}:{CREATE}"] - auth_header = generate_auth_header(scopes=token_scopes) - result = _api.create( - url=test_config.cli.server_url, - headers=auth_header, - resource_type=endpoint, - json_resource=json_resource, - ) - assert result.status_code == HTTP_409_CONFLICT - - @pytest.mark.parametrize("endpoint", TAXONOMY_ENDPOINTS) - def test_api_can_create_without_explicit_fides_key( - self, - test_config: FidesConfig, - generate_auth_header, - endpoint: str, - ) -> None: - """Ensure we can create taxonomy elements without specifying a fides_key""" - # get a default taxonomy element as a sample resource - resource = getattr(DEFAULT_TAXONOMY, endpoint)[0] - resource = TAXONOMY_EXTENSIONS[endpoint]( - **resource.model_dump(mode="json") - ) # cast resource to extended model - # Build unique name based on sample name - resource.name = resource.name + "my new resource" - resource.fides_key = None - json_resource = resource.json(exclude_none=True) - token_scopes: List[str] = [f"{CLI_SCOPE_PREFIX_MAPPING[endpoint]}:{CREATE}"] - auth_header = generate_auth_header(scopes=token_scopes) - result = _api.create( - url=test_config.cli.server_url, - headers=auth_header, - resource_type=endpoint, - json_resource=json_resource, - ) - assert result.status_code == 201 - assert result.json()["active"] is True - new_key = result.json()["fides_key"] - assert "my_new_resource" in new_key - - result = _api.get( - url=test_config.cli.server_url, - headers=test_config.user.auth_header, - resource_type=endpoint, - resource_id=new_key, - ) - assert result.json()["active"] is True - - @pytest.mark.parametrize("endpoint", TAXONOMY_ENDPOINTS) - def test_api_can_update_active_when_creating_with_same_name( - self, - test_config: FidesConfig, - endpoint: str, - ) -> None: - """ - If we attempt to create a new resource with the same name as an existing inactive resource, - but with no explicit fides_key, we should update the existing resource to be active - """ - resource = getattr(DEFAULT_TAXONOMY, endpoint)[0] - resource = TAXONOMY_EXTENSIONS[endpoint]( - **resource.model_dump(mode="json") - ) # cast resource to extended model - resource.active = False - json_resource = resource.json(exclude_none=True) - # First, update the existing resource as inactive so we can use it to test - result = _api.update( - url=test_config.cli.server_url, - headers=test_config.user.auth_header, - resource_type=endpoint, - json_resource=json_resource, - ) - assert result.status_code == 200 - assert result.json()["active"] is False - - # Confirm it was updated to inactive - result = _api.get( - url=test_config.cli.server_url, - headers=test_config.user.auth_header, - resource_type=endpoint, - resource_id=resource.fides_key, - ) - assert result.json()["active"] is False - - # Now attempt to create another resource with a name that will generate the same fides_key - # as the inactive resource - - resource.name = resource.name # explicitly using the same name - resource.fides_key = None - json_resource = resource.json(exclude_none=True) - result = _api.create( - url=test_config.cli.server_url, - headers=test_config.user.auth_header, - resource_type=endpoint, - json_resource=json_resource, - ) - assert result.status_code == 200 - assert result.json()["active"] is True - - # Confirm the existing resource was updated to active - result = _api.get( - url=test_config.cli.server_url, - headers=test_config.user.auth_header, - resource_type=endpoint, - resource_id=resource.fides_key, - ) - assert result.json()["active"] is True - - @pytest.mark.integration class TestDefaultTaxonomyCrud: @pytest.mark.parametrize("endpoint", TAXONOMY_ENDPOINTS) diff --git a/tests/ops/api/v1/endpoints/test_taxonomy_overrides.py b/tests/ops/api/v1/endpoints/test_taxonomy_overrides.py new file mode 100644 index 0000000000..1de888f6cb --- /dev/null +++ b/tests/ops/api/v1/endpoints/test_taxonomy_overrides.py @@ -0,0 +1,141 @@ +import json + +import pytest +from sqlalchemy.orm import Session +from starlette.testclient import TestClient + +from fides.api.models.client import ClientDetail +from fides.api.models.sql_models import DataUse +from fides.common.api.scope_registry import DATA_USE, DATA_USE_CREATE, STORAGE_READ +from fides.common.api.v1.urn_registry import V1_URL_PREFIX + + +class TestCreateDataUse: + @pytest.fixture(scope="function") + def url(self, oauth_client: ClientDetail) -> str: + return V1_URL_PREFIX + "/" + DATA_USE + + @pytest.fixture(scope="function") + def payload(self): + return { + "name": "test data use", + "description": "this is a test data use", + "is_default": False, + } + + @pytest.fixture(scope="function") + def disabled_data_use(self, db: Session): + payload = { + "name": "test data use", + "fides_key": "test_data_use", + "active": False, + "is_default": False, + "description": "Disabled data use", + } + dataUse = DataUse.create(db=db, data=payload) + yield dataUse + dataUse.delete(db) + + @pytest.fixture(scope="function") + def enabled_data_use(self, db: Session): + payload = { + "name": "test data use", + "fides_key": "test_data_use", + "active": True, + "is_default": False, + "description": "Disabled data use", + } + dataUse = DataUse.create(db=db, data=payload) + yield dataUse + dataUse.delete(db) + + def test_create_data_use_not_authenticated( + self, + api_client: TestClient, + payload, + url, + ): + response = api_client.post(url, headers={}, json=payload) + assert 401 == response.status_code + + def test_create_data_use_incorrect_scope( + self, + api_client: TestClient, + payload, + url, + generate_auth_header, + ): + auth_header = generate_auth_header([STORAGE_READ]) + response = api_client.post(url, headers=auth_header, json=payload) + assert 403 == response.status_code + + def test_create_data_use_with_fides_key( + self, + db: Session, + api_client: TestClient, + payload, + url, + generate_auth_header, + ): + auth_header = generate_auth_header([DATA_USE_CREATE]) + payload["fides_key"] = "test_data_use" + response = api_client.post(url, headers=auth_header, json=payload) + + assert 201 == response.status_code + response_body = json.loads(response.text) + + assert response_body["fides_key"] == "test_data_use" + data_use = db.query(DataUse).filter_by(fides_key="test_data_use")[0] + data_use.delete(db) + + def test_create_data_use_with_no_fides_key( + self, + db: Session, + api_client: TestClient, + payload, + url, + generate_auth_header, + ): + auth_header = generate_auth_header([DATA_USE_CREATE]) + response = api_client.post(url, headers=auth_header, json=payload) + + assert 201 == response.status_code + response_body = json.loads(response.text) + + assert response_body["fides_key"] == "test_data_use" + data_use = db.query(DataUse).filter_by(fides_key="test_data_use")[0] + data_use.delete(db) + + def test_create_data_use_with_conflicting_key( + self, + db: Session, + api_client: TestClient, + payload, + url, + enabled_data_use, + generate_auth_header, + ): + auth_header = generate_auth_header([DATA_USE_CREATE]) + response = api_client.post(url, headers=auth_header, json=payload) + + assert 422 == response.status_code + + def test_create_data_use_with_disabled_matching_name( + self, + db: Session, + api_client: TestClient, + payload, + url, + disabled_data_use, + generate_auth_header, + ): + auth_header = generate_auth_header([DATA_USE_CREATE]) + response = api_client.post(url, headers=auth_header, json=payload) + + assert 201 == response.status_code + response_body = json.loads(response.text) + + assert response_body["fides_key"] == "test_data_use" + assert response_body["description"] == "this is a test data use" + data_use = db.query(DataUse).filter_by(fides_key="test_data_use")[0] + data_use.delete(db)