From 6010aad893b1335eee13ac0857e713e837711d97 Mon Sep 17 00:00:00 2001 From: Eliana Rosselli Date: Thu, 13 Jun 2024 12:44:38 -0300 Subject: [PATCH 1/7] Fixes PROD-2134 - Adds validation for data_categories and data_subjects in system creation --- ...dd_property_id_to_privacy_request_model.py | 3 +- .../v1/endpoints/consent_request_endpoints.py | 6 +- src/fides/api/db/system.py | 39 +++++-- tests/ctl/core/test_api.py | 101 ++++++++++++++++++ 4 files changed, 132 insertions(+), 17 deletions(-) diff --git a/src/fides/api/alembic/migrations/versions/a3c173391603_add_property_id_to_privacy_request_model.py b/src/fides/api/alembic/migrations/versions/a3c173391603_add_property_id_to_privacy_request_model.py index 40ae8fe64c..c8058f9086 100644 --- a/src/fides/api/alembic/migrations/versions/a3c173391603_add_property_id_to_privacy_request_model.py +++ b/src/fides/api/alembic/migrations/versions/a3c173391603_add_property_id_to_privacy_request_model.py @@ -6,9 +6,8 @@ """ -from alembic import op import sqlalchemy as sa - +from alembic import op # revision identifiers, used by Alembic. revision = "a3c173391603" diff --git a/src/fides/api/api/v1/endpoints/consent_request_endpoints.py b/src/fides/api/api/v1/endpoints/consent_request_endpoints.py index 11566f3db9..82a27d737d 100644 --- a/src/fides/api/api/v1/endpoints/consent_request_endpoints.py +++ b/src/fides/api/api/v1/endpoints/consent_request_endpoints.py @@ -41,7 +41,7 @@ ) from fides.api.models.property import Property from fides.api.oauth.utils import verify_oauth_client -from fides.api.schemas.messaging.messaging import MessagingMethod, MessagingActionType +from fides.api.schemas.messaging.messaging import MessagingActionType, MessagingMethod from fides.api.schemas.privacy_request import BulkPostPrivacyRequests from fides.api.schemas.privacy_request import Consent as ConsentSchema from fides.api.schemas.privacy_request import ( @@ -56,9 +56,7 @@ ) from fides.api.schemas.redis_cache import Identity from fides.api.service._verification import send_verification_code_to_user -from fides.api.service.messaging.message_dispatch_service import ( - message_send_enabled, -) +from fides.api.service.messaging.message_dispatch_service import message_send_enabled from fides.api.util.api_router import APIRouter from fides.api.util.consent_util import ( get_or_create_fides_user_device_id_provided_identity, diff --git a/src/fides/api/db/system.py b/src/fides/api/db/system.py index a31528460b..bbc25f5036 100644 --- a/src/fides/api/db/system.py +++ b/src/fides/api/db/system.py @@ -10,6 +10,7 @@ from fastapi import HTTPException from fideslang.models import Cookies as CookieSchema from fideslang.models import System as SystemSchema +from fideslang.validation import FidesKey from loguru import logger as log from sqlalchemy import and_, delete, insert, select, update from sqlalchemy.ext.asyncio import AsyncSession @@ -17,9 +18,12 @@ from sqlalchemy.sql.elements import BinaryExpression from starlette.status import HTTP_400_BAD_REQUEST, HTTP_404_NOT_FOUND +from fides.api.db.base import Base # type: ignore[attr-defined] from fides.api.db.crud import create_resource, get_resource, update_resource from fides.api.models.sql_models import ( # type: ignore[attr-defined] Cookies, + DataCategory, + DataSubject, DataUse, PrivacyDeclaration, System, @@ -48,27 +52,40 @@ def get_system(db: Session, fides_key: str) -> System: return system +async def validate_data_labels( + db: AsyncSession, sql_model: Base, labels: List[FidesKey] +) -> None: + for label in labels: + try: + await get_resource( + sql_model=sql_model, + fides_key=label, + async_session=db, + ) + except NotFoundError: + raise HTTPException( + status_code=HTTP_400_BAD_REQUEST, + detail=f"Invalid privacy declaration referencing unknown {sql_model.__name__} {label}", + ) + + async def validate_privacy_declarations(db: AsyncSession, system: SystemSchema) -> None: """ Ensure that the `PrivacyDeclaration`s on the provided `System` resource are valid: - that they reference valid `DataUse` records + - that they reference valid `DataCategory` records - that there are not "duplicate" `PrivacyDeclaration`s as defined by their "logical ID" If not, a `400` is raised """ logical_ids = set() for privacy_declaration in system.privacy_declarations: - try: - await get_resource( - sql_model=DataUse, - fides_key=privacy_declaration.data_use, - async_session=db, - ) - except NotFoundError: - raise HTTPException( - status_code=HTTP_400_BAD_REQUEST, - detail=f"Invalid privacy declaration referencing unknown DataUse {privacy_declaration.data_use}", - ) + await validate_data_labels(db, DataUse, [privacy_declaration.data_use]) + await validate_data_labels( + db, DataCategory, privacy_declaration.data_categories + ) + await validate_data_labels(db, DataSubject, privacy_declaration.data_subjects) + logical_id = privacy_declaration_logical_id(privacy_declaration) if logical_id in logical_ids: raise HTTPException( diff --git a/tests/ctl/core/test_api.py b/tests/ctl/core/test_api.py index ed517f2ec3..67986e3e50 100644 --- a/tests/ctl/core/test_api.py +++ b/tests/ctl/core/test_api.py @@ -589,6 +589,107 @@ def test_system_create_system_already_exists( len(System.all(db)) == 1 ) # ensure our system wasn't created, still only one system + def test_system_create_invalid_data_category( + self, test_config, db, generate_auth_header + ): + auth_header = generate_auth_header(scopes=[SYSTEM_CREATE]) + + result = _api.create( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=json.dumps( + { + "fides_key": "system_key", + "system_type": "system", + "privacy_declarations": [ + { + "fides_key": "test", + "data_categories": [ + "user.name.first", + "user.name.nickname", + "user.name.last", + ], # Nickname does not exist + "data_use": "marketing", + } + ], + } + ), + ) + + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": "Invalid privacy declaration referencing unknown DataCategory user.name.nickname" + } + + assert not System.all(db) # ensure our system wasn't created + + def test_system_create_invalid_data_subject( + self, test_config, db, generate_auth_header + ): + auth_header = generate_auth_header(scopes=[SYSTEM_CREATE]) + + result = _api.create( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=json.dumps( + { + "fides_key": "system_key", + "system_type": "system", + "privacy_declarations": [ + { + "fides_key": "test", + "data_categories": ["user.name.first", "user.name.last"], + "data_use": "marketing", + "data_subjects": [ + "employee", + "intern", + ], # Intern does not exist + } + ], + } + ), + ) + + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": "Invalid privacy declaration referencing unknown DataSubject intern" + } + + assert not System.all(db) # ensure our system wasn't created + + def test_system_create_invalid_data_use( + self, test_config, db, generate_auth_header + ): + auth_header = generate_auth_header(scopes=[SYSTEM_CREATE]) + + result = _api.create( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=json.dumps( + { + "fides_key": "system_key", + "system_type": "system", + "privacy_declarations": [ + { + "fides_key": "test", + "data_categories": ["user.name.first", "user.name.last"], + "data_use": "marketing.measure", # Invalid data use + } + ], + } + ), + ) + + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": "Invalid privacy declaration referencing unknown DataUse marketing.measure" + } + + assert not System.all(db) # ensure our system wasn't created + async def test_system_create( self, generate_auth_header, db, test_config, system_create_request_body ): From 1128ab115b5f69c63f713dfa17f499ff439413e5 Mon Sep 17 00:00:00 2001 From: Eliana Rosselli Date: Thu, 13 Jun 2024 14:40:19 -0300 Subject: [PATCH 2/7] Also throw 400 if taxonomy element is invalid --- src/fides/api/db/system.py | 9 ++- tests/ctl/core/test_api.py | 155 ++++++++++++++++++++++++++++++++++++- 2 files changed, 162 insertions(+), 2 deletions(-) diff --git a/src/fides/api/db/system.py b/src/fides/api/db/system.py index bbc25f5036..5710f27d5a 100644 --- a/src/fides/api/db/system.py +++ b/src/fides/api/db/system.py @@ -57,7 +57,7 @@ async def validate_data_labels( ) -> None: for label in labels: try: - await get_resource( + resource = await get_resource( sql_model=sql_model, fides_key=label, async_session=db, @@ -68,12 +68,19 @@ async def validate_data_labels( detail=f"Invalid privacy declaration referencing unknown {sql_model.__name__} {label}", ) + if not resource.active: + raise HTTPException( + status_code=HTTP_400_BAD_REQUEST, + detail=f"Invalid privacy declaration referencing inactive {sql_model.__name__} {label}", + ) + async def validate_privacy_declarations(db: AsyncSession, system: SystemSchema) -> None: """ Ensure that the `PrivacyDeclaration`s on the provided `System` resource are valid: - that they reference valid `DataUse` records - that they reference valid `DataCategory` records + - that they reference valid `DataSubject` records - that there are not "duplicate" `PrivacyDeclaration`s as defined by their "logical ID" If not, a `400` is raised diff --git a/tests/ctl/core/test_api.py b/tests/ctl/core/test_api.py index 67986e3e50..98110f0733 100644 --- a/tests/ctl/core/test_api.py +++ b/tests/ctl/core/test_api.py @@ -24,13 +24,14 @@ HTTP_422_UNPROCESSABLE_ENTITY, ) from starlette.testclient import TestClient +from sqlalchemy.orm import Session from fides.api.api.v1.endpoints import health from fides.api.db.crud import get_resource from fides.api.db.system import create_system from fides.api.models.connectionconfig import ConnectionConfig from fides.api.models.datasetconfig import DatasetConfig -from fides.api.models.sql_models import Dataset, PrivacyDeclaration, System +from fides.api.models.sql_models import Dataset, PrivacyDeclaration, System, DataCategory as DataCategoryModel, DataUse as DataUseModel, DataSubject as DataSubjectModel from fides.api.models.system_history import SystemHistory from fides.api.models.tcf_purpose_overrides import TCFPurposeOverride from fides.api.oauth.roles import OWNER, VIEWER @@ -514,6 +515,53 @@ def system_create_request_body(self) -> SystemSchema: ], ) + + @pytest.fixture(scope="function", name="data_category") + def fixture_inactive_data_category(self, db: Session) -> typing.Generator: + """ + Fixture that yields an inactive data category and then deletes it for each test run. + """ + fides_key = "foo" + data_category = DataCategoryModel.create(db=db, data={ + "fides_key": fides_key, + "active": False, + }) + + yield data_category + + data_category.delete(db) + + + @pytest.fixture(scope="function", name="data_use") + def fixture_inactive_data_use(self, db: Session) -> typing.Generator: + """ + Fixture that yields an inactive data category and then deletes it for each test run. + """ + fides_key = "foo" + data_use = DataUseModel.create(db=db, data={ + "fides_key": fides_key, + "active": False, + }) + + yield data_use + + data_use.delete(db) + + @pytest.fixture(scope="function", name="data_subject") + def fixture_inactive_data_subject(self, db: Session) -> typing.Generator: + """ + Fixture that yields an inactive data category and then deletes it for each test run. + """ + fides_key = "foo" + data_subject = DataSubjectModel.create(db=db, data={ + "fides_key": fides_key, + "active": False, + }) + + yield data_subject + + data_subject.delete(db) + def test_system_create_not_authenticated( self, test_config, @@ -690,6 +738,111 @@ def test_system_create_invalid_data_use( assert not System.all(db) # ensure our system wasn't created + def test_system_create_inactive_data_category( + self, test_config, data_category, db, generate_auth_header + ): + auth_header = generate_auth_header(scopes=[SYSTEM_CREATE]) + + result = _api.create( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=json.dumps( + { + "fides_key": "system_key", + "system_type": "system", + "privacy_declarations": [ + { + "fides_key": "test", + "data_categories": [ + "user.name.first", + "user.name.last", + data_category.fides_key, + ], + "data_use": "marketing", + } + ], + } + ), + ) + + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": f"Invalid privacy declaration referencing inactive DataCategory {data_category.fides_key}" + } + + assert not System.all(db) # ensure our system wasn't created + + def test_system_create_inactive_data_use( + self, test_config, data_use, db, generate_auth_header + ): + auth_header = generate_auth_header(scopes=[SYSTEM_CREATE]) + + result = _api.create( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=json.dumps( + { + "fides_key": "system_key", + "system_type": "system", + "privacy_declarations": [ + { + "fides_key": "test", + "data_categories": [ + "user.name.first", + "user.name.last", + ], + "data_use": data_use.fides_key, + } + ], + } + ), + ) + + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": f"Invalid privacy declaration referencing inactive DataUse {data_use.fides_key}" + } + + assert not System.all(db) # ensure our system wasn't created + + def test_system_create_inactive_data_subject( + self, test_config, data_subject, db, generate_auth_header + ): + auth_header = generate_auth_header(scopes=[SYSTEM_CREATE]) + + result = _api.create( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=json.dumps( + { + "fides_key": "system_key", + "system_type": "system", + "privacy_declarations": [ + { + "fides_key": "test", + "data_categories": [ + "user.name.first", + "user.name.last", + ], + "data_use": "marketing", + "data_subjects": [data_subject.fides_key], + } + ], + } + ), + ) + + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": f"Invalid privacy declaration referencing inactive DataSubject {data_subject.fides_key}" + } + + assert not System.all(db) # ensure our system wasn't created + + async def test_system_create( self, generate_auth_header, db, test_config, system_create_request_body ): From 5f0fe8553923d2ad3642d84f7a8a49da842d89e1 Mon Sep 17 00:00:00 2001 From: Eliana Rosselli Date: Thu, 13 Jun 2024 14:45:27 -0300 Subject: [PATCH 3/7] Run static checks --- tests/ctl/core/test_api.py | 44 +++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/tests/ctl/core/test_api.py b/tests/ctl/core/test_api.py index 98110f0733..59502466e3 100644 --- a/tests/ctl/core/test_api.py +++ b/tests/ctl/core/test_api.py @@ -13,6 +13,7 @@ from fideslang.models import PrivacyDeclaration as PrivacyDeclarationSchema from fideslang.models import System as SystemSchema from pytest import MonkeyPatch +from sqlalchemy.orm import Session from starlette.status import ( HTTP_200_OK, HTTP_201_CREATED, @@ -24,14 +25,17 @@ HTTP_422_UNPROCESSABLE_ENTITY, ) from starlette.testclient import TestClient -from sqlalchemy.orm import Session from fides.api.api.v1.endpoints import health from fides.api.db.crud import get_resource from fides.api.db.system import create_system from fides.api.models.connectionconfig import ConnectionConfig from fides.api.models.datasetconfig import DatasetConfig -from fides.api.models.sql_models import Dataset, PrivacyDeclaration, System, DataCategory as DataCategoryModel, DataUse as DataUseModel, DataSubject as DataSubjectModel +from fides.api.models.sql_models import DataCategory as DataCategoryModel +from fides.api.models.sql_models import Dataset +from fides.api.models.sql_models import DataSubject as DataSubjectModel +from fides.api.models.sql_models import DataUse as DataUseModel +from fides.api.models.sql_models import PrivacyDeclaration, System from fides.api.models.system_history import SystemHistory from fides.api.models.tcf_purpose_overrides import TCFPurposeOverride from fides.api.oauth.roles import OWNER, VIEWER @@ -515,33 +519,37 @@ def system_create_request_body(self) -> SystemSchema: ], ) - @pytest.fixture(scope="function", name="data_category") def fixture_inactive_data_category(self, db: Session) -> typing.Generator: """ Fixture that yields an inactive data category and then deletes it for each test run. """ fides_key = "foo" - data_category = DataCategoryModel.create(db=db, data={ - "fides_key": fides_key, - "active": False, - }) + data_category = DataCategoryModel.create( + db=db, + data={ + "fides_key": fides_key, + "active": False, + }, + ) yield data_category data_category.delete(db) - @pytest.fixture(scope="function", name="data_use") def fixture_inactive_data_use(self, db: Session) -> typing.Generator: """ Fixture that yields an inactive data category and then deletes it for each test run. """ fides_key = "foo" - data_use = DataUseModel.create(db=db, data={ - "fides_key": fides_key, - "active": False, - }) + data_use = DataUseModel.create( + db=db, + data={ + "fides_key": fides_key, + "active": False, + }, + ) yield data_use @@ -553,10 +561,13 @@ def fixture_inactive_data_subject(self, db: Session) -> typing.Generator: Fixture that yields an inactive data category and then deletes it for each test run. """ fides_key = "foo" - data_subject = DataSubjectModel.create(db=db, data={ - "fides_key": fides_key, - "active": False, - }) + data_subject = DataSubjectModel.create( + db=db, + data={ + "fides_key": fides_key, + "active": False, + }, + ) yield data_subject @@ -842,7 +853,6 @@ def test_system_create_inactive_data_subject( assert not System.all(db) # ensure our system wasn't created - async def test_system_create( self, generate_auth_header, db, test_config, system_create_request_body ): From 79d9f9620632695a4a9518723af2de46373af6e4 Mon Sep 17 00:00:00 2001 From: Eliana Rosselli Date: Thu, 13 Jun 2024 15:07:00 -0300 Subject: [PATCH 4/7] Add missing tests --- tests/ctl/core/test_api.py | 250 +++++++++++++++++++++++++++++-------- 1 file changed, 196 insertions(+), 54 deletions(-) diff --git a/tests/ctl/core/test_api.py b/tests/ctl/core/test_api.py index 59502466e3..33546e58b6 100644 --- a/tests/ctl/core/test_api.py +++ b/tests/ctl/core/test_api.py @@ -78,6 +78,63 @@ def get_existing_key(test_config: FidesConfig, resource_type: str) -> int: ).json()[-1]["fides_key"] +@pytest.fixture(scope="function", name="data_category") +def fixture_inactive_data_category(db: Session) -> typing.Generator: + """ + Fixture that yields an inactive data category and then deletes it for each test run. + """ + fides_key = "foo" + data_category = DataCategoryModel.create( + db=db, + data={ + "fides_key": fides_key, + "active": False, + }, + ) + + yield data_category + + data_category.delete(db) + + +@pytest.fixture(scope="function", name="data_use") +def fixture_inactive_data_use(db: Session) -> typing.Generator: + """ + Fixture that yields an inactive data category and then deletes it for each test run. + """ + fides_key = "foo" + data_use = DataUseModel.create( + db=db, + data={ + "fides_key": fides_key, + "active": False, + }, + ) + + yield data_use + + data_use.delete(db) + + +@pytest.fixture(scope="function", name="data_subject") +def fixture_inactive_data_subject(db: Session) -> typing.Generator: + """ + Fixture that yields an inactive data category and then deletes it for each test run. + """ + fides_key = "foo" + data_subject = DataSubjectModel.create( + db=db, + data={ + "fides_key": fides_key, + "active": False, + }, + ) + + yield data_subject + + data_subject.delete(db) + + # Unit Tests @pytest.mark.unit def test_generate_resource_urls_no_id(test_config: FidesConfig) -> None: @@ -519,60 +576,6 @@ def system_create_request_body(self) -> SystemSchema: ], ) - @pytest.fixture(scope="function", name="data_category") - def fixture_inactive_data_category(self, db: Session) -> typing.Generator: - """ - Fixture that yields an inactive data category and then deletes it for each test run. - """ - fides_key = "foo" - data_category = DataCategoryModel.create( - db=db, - data={ - "fides_key": fides_key, - "active": False, - }, - ) - - yield data_category - - data_category.delete(db) - - @pytest.fixture(scope="function", name="data_use") - def fixture_inactive_data_use(self, db: Session) -> typing.Generator: - """ - Fixture that yields an inactive data category and then deletes it for each test run. - """ - fides_key = "foo" - data_use = DataUseModel.create( - db=db, - data={ - "fides_key": fides_key, - "active": False, - }, - ) - - yield data_use - - data_use.delete(db) - - @pytest.fixture(scope="function", name="data_subject") - def fixture_inactive_data_subject(self, db: Session) -> typing.Generator: - """ - Fixture that yields an inactive data category and then deletes it for each test run. - """ - fides_key = "foo" - data_subject = DataSubjectModel.create( - db=db, - data={ - "fides_key": fides_key, - "active": False, - }, - ) - - yield data_subject - - data_subject.delete(db) - def test_system_create_not_authenticated( self, test_config, @@ -1576,10 +1579,149 @@ def test_system_update_privacy_declaration_invalid_data_use( json_resource=system_update_request_body.json(exclude_none=True), ) assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": "Invalid privacy declaration referencing unknown DataUse invalid_data_use" + } + + # assert the system's privacy declaration has not been updated + db.refresh(system) + assert system.privacy_declarations[0].data_use == "marketing.advertising" + + def test_system_update_privacy_declaration_invalid_data_category( + self, + system, + test_config, + system_update_request_body, + generate_role_header, + db, + ): + auth_header = generate_role_header(roles=[OWNER]) + system_update_request_body.privacy_declarations[0].data_categories = [ + "invalid_data_category" + ] + result = _api.update( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=system_update_request_body.json(exclude_none=True), + ) + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": "Invalid privacy declaration referencing unknown DataCategory invalid_data_category" + } + # assert the system's privacy declaration has not been updated + db.refresh(system) + assert system.privacy_declarations[0].data_categories == [ + "user.device.cookie_id" + ] + + def test_system_update_privacy_declaration_invalid_data_subject( + self, + system, + test_config, + system_update_request_body, + generate_role_header, + db, + ): + auth_header = generate_role_header(roles=[OWNER]) + system_update_request_body.privacy_declarations[0].data_subjects = [ + "invalid_data_subject" + ] + result = _api.update( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=system_update_request_body.json(exclude_none=True), + ) + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": "Invalid privacy declaration referencing unknown DataSubject invalid_data_subject" + } + # assert the system's privacy declaration has not been updated + db.refresh(system) + assert system.privacy_declarations[0].data_subjects == ["customer"] + + def test_system_update_privacy_declaration_inactive_data_use( + self, + system, + test_config, + system_update_request_body, + data_use, + generate_role_header, + db, + ): + auth_header = generate_role_header(roles=[OWNER]) + system_update_request_body.privacy_declarations[0].data_use = data_use.fides_key + result = _api.update( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=system_update_request_body.json(exclude_none=True), + ) + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": f"Invalid privacy declaration referencing inactive DataUse {data_use.fides_key}" + } # assert the system's privacy declaration has not been updated db.refresh(system) assert system.privacy_declarations[0].data_use == "marketing.advertising" + def test_system_update_privacy_declaration_inactive_data_category( + self, + system, + test_config, + system_update_request_body, + data_category, + generate_role_header, + db, + ): + auth_header = generate_role_header(roles=[OWNER]) + system_update_request_body.privacy_declarations[0].data_categories = [ + data_category.fides_key + ] + result = _api.update( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=system_update_request_body.json(exclude_none=True), + ) + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": f"Invalid privacy declaration referencing inactive DataCategory {data_category.fides_key}" + } + # assert the system's privacy declaration has not been updated + db.refresh(system) + assert system.privacy_declarations[0].data_categories == [ + "user.device.cookie_id" + ] + + def test_system_update_privacy_declaration_inactive_data_subject( + self, + system, + test_config, + system_update_request_body, + data_subject, + generate_role_header, + db, + ): + auth_header = generate_role_header(roles=[OWNER]) + system_update_request_body.privacy_declarations[0].data_subjects = [ + data_subject.fides_key + ] + result = _api.update( + url=test_config.cli.server_url, + headers=auth_header, + resource_type="system", + json_resource=system_update_request_body.json(exclude_none=True), + ) + assert result.status_code == HTTP_400_BAD_REQUEST + assert result.json() == { + "detail": f"Invalid privacy declaration referencing inactive DataSubject {data_subject.fides_key}" + } + # assert the system's privacy declaration has not been updated + db.refresh(system) + assert system.privacy_declarations[0].data_subjects == ["customer"] + def test_system_update_privacy_declaration_invalid_duplicate( self, system, From e1714075d602a67fb12c47bce755a438c71c1ca6 Mon Sep 17 00:00:00 2001 From: Eliana Rosselli Date: Thu, 13 Jun 2024 15:13:30 -0300 Subject: [PATCH 5/7] Fix tests --- tests/ctl/core/test_api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/ctl/core/test_api.py b/tests/ctl/core/test_api.py index 33546e58b6..75eb33d975 100644 --- a/tests/ctl/core/test_api.py +++ b/tests/ctl/core/test_api.py @@ -83,7 +83,7 @@ def fixture_inactive_data_category(db: Session) -> typing.Generator: """ Fixture that yields an inactive data category and then deletes it for each test run. """ - fides_key = "foo" + fides_key = "inactive_data_category" data_category = DataCategoryModel.create( db=db, data={ @@ -102,7 +102,7 @@ def fixture_inactive_data_use(db: Session) -> typing.Generator: """ Fixture that yields an inactive data category and then deletes it for each test run. """ - fides_key = "foo" + fides_key = "inactive_data_use" data_use = DataUseModel.create( db=db, data={ @@ -121,7 +121,7 @@ def fixture_inactive_data_subject(db: Session) -> typing.Generator: """ Fixture that yields an inactive data category and then deletes it for each test run. """ - fides_key = "foo" + fides_key = "inactive_data_subject" data_subject = DataSubjectModel.create( db=db, data={ From 61b7ceafff86969b9559b7096d11b50f5758f143 Mon Sep 17 00:00:00 2001 From: Eliana Rosselli Date: Fri, 14 Jun 2024 10:52:34 -0300 Subject: [PATCH 6/7] Implement Code Review suggestions --- CHANGELOG.md | 1 + src/fides/api/db/system.py | 12 +++-- .../connection_configuration/__init__.py | 5 +- tests/ctl/core/test_api.py | 46 +++++++++---------- 4 files changed, 36 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4164964cde..e99953a31d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ The types of changes are: - Fixed "add" icons on some buttons being wrong size [#4975](https://github.com/ethyca/fides/pull/4975) - Masked "Keyfile credentials" input on integration config form [#4971](https://github.com/ethyca/fides/pull/4971) - Fixed ability to update consent preferences after they've previously been set [#4984](https://github.com/ethyca/fides/pull/4984) +- Fixed validations for privacy declaration taxonomy labels when creating/updating a System [#4982](https://github.com/ethyca/fides/pull/4982) ## [2.38.0](https://github.com/ethyca/fides/compare/2.37.0...2.38.0) diff --git a/src/fides/api/db/system.py b/src/fides/api/db/system.py index 5710f27d5a..4d662fdfb6 100644 --- a/src/fides/api/db/system.py +++ b/src/fides/api/db/system.py @@ -4,7 +4,7 @@ import copy from datetime import datetime -from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple, Type, Union from deepdiff import DeepDiff from fastapi import HTTPException @@ -18,7 +18,6 @@ from sqlalchemy.sql.elements import BinaryExpression from starlette.status import HTTP_400_BAD_REQUEST, HTTP_404_NOT_FOUND -from fides.api.db.base import Base # type: ignore[attr-defined] from fides.api.db.crud import create_resource, get_resource, update_resource from fides.api.models.sql_models import ( # type: ignore[attr-defined] Cookies, @@ -53,8 +52,15 @@ def get_system(db: Session, fides_key: str) -> System: async def validate_data_labels( - db: AsyncSession, sql_model: Base, labels: List[FidesKey] + db: AsyncSession, + sql_model: Union[Type[DataUse], Type[DataSubject], Type[DataCategory]], + labels: List[FidesKey], ) -> None: + """ + Given a model and a list of FidesKeys, check that for each Fides Key + there is a model instance with that key and the active attribute set to True. + If any of the keys don't exist or exist but are not active, raise a 400 error + """ for label in labels: try: resource = await get_resource( diff --git a/src/fides/api/schemas/connection_configuration/__init__.py b/src/fides/api/schemas/connection_configuration/__init__.py index ea95fe5f67..c3fa117f66 100644 --- a/src/fides/api/schemas/connection_configuration/__init__.py +++ b/src/fides/api/schemas/connection_configuration/__init__.py @@ -87,7 +87,8 @@ SaaSSchemaFactory as SaaSSchemaFactory, ) from fides.api.schemas.connection_configuration.connection_secrets_scylla import ( - ScyllaSchema, ScyllaDocsSchema, + ScyllaDocsSchema, + ScyllaSchema, ) from fides.api.schemas.connection_configuration.connection_secrets_snowflake import ( SnowflakeDocsSchema as SnowflakeDocsSchema, @@ -175,5 +176,5 @@ def get_connection_secrets_schema( FidesDocsSchema, SovrnDocsSchema, DynamoDBDocsSchema, - ScyllaDocsSchema + ScyllaDocsSchema, ] diff --git a/tests/ctl/core/test_api.py b/tests/ctl/core/test_api.py index 75eb33d975..351cedde03 100644 --- a/tests/ctl/core/test_api.py +++ b/tests/ctl/core/test_api.py @@ -78,7 +78,7 @@ def get_existing_key(test_config: FidesConfig, resource_type: str) -> int: ).json()[-1]["fides_key"] -@pytest.fixture(scope="function", name="data_category") +@pytest.fixture(scope="function", name="inactive_data_category") def fixture_inactive_data_category(db: Session) -> typing.Generator: """ Fixture that yields an inactive data category and then deletes it for each test run. @@ -97,10 +97,10 @@ def fixture_inactive_data_category(db: Session) -> typing.Generator: data_category.delete(db) -@pytest.fixture(scope="function", name="data_use") +@pytest.fixture(scope="function", name="inactive_data_use") def fixture_inactive_data_use(db: Session) -> typing.Generator: """ - Fixture that yields an inactive data category and then deletes it for each test run. + Fixture that yields an inactive data use and then deletes it for each test run. """ fides_key = "inactive_data_use" data_use = DataUseModel.create( @@ -116,10 +116,10 @@ def fixture_inactive_data_use(db: Session) -> typing.Generator: data_use.delete(db) -@pytest.fixture(scope="function", name="data_subject") +@pytest.fixture(scope="function", name="inactive_data_subject") def fixture_inactive_data_subject(db: Session) -> typing.Generator: """ - Fixture that yields an inactive data category and then deletes it for each test run. + Fixture that yields an inactive data subject and then deletes it for each test run. """ fides_key = "inactive_data_subject" data_subject = DataSubjectModel.create( @@ -753,7 +753,7 @@ def test_system_create_invalid_data_use( assert not System.all(db) # ensure our system wasn't created def test_system_create_inactive_data_category( - self, test_config, data_category, db, generate_auth_header + self, test_config, inactive_data_category, db, generate_auth_header ): auth_header = generate_auth_header(scopes=[SYSTEM_CREATE]) @@ -771,7 +771,7 @@ def test_system_create_inactive_data_category( "data_categories": [ "user.name.first", "user.name.last", - data_category.fides_key, + inactive_data_category.fides_key, ], "data_use": "marketing", } @@ -782,13 +782,13 @@ def test_system_create_inactive_data_category( assert result.status_code == HTTP_400_BAD_REQUEST assert result.json() == { - "detail": f"Invalid privacy declaration referencing inactive DataCategory {data_category.fides_key}" + "detail": f"Invalid privacy declaration referencing inactive DataCategory {inactive_data_category.fides_key}" } assert not System.all(db) # ensure our system wasn't created def test_system_create_inactive_data_use( - self, test_config, data_use, db, generate_auth_header + self, test_config, inactive_data_use, db, generate_auth_header ): auth_header = generate_auth_header(scopes=[SYSTEM_CREATE]) @@ -807,7 +807,7 @@ def test_system_create_inactive_data_use( "user.name.first", "user.name.last", ], - "data_use": data_use.fides_key, + "data_use": inactive_data_use.fides_key, } ], } @@ -816,13 +816,13 @@ def test_system_create_inactive_data_use( assert result.status_code == HTTP_400_BAD_REQUEST assert result.json() == { - "detail": f"Invalid privacy declaration referencing inactive DataUse {data_use.fides_key}" + "detail": f"Invalid privacy declaration referencing inactive DataUse {inactive_data_use.fides_key}" } assert not System.all(db) # ensure our system wasn't created def test_system_create_inactive_data_subject( - self, test_config, data_subject, db, generate_auth_header + self, test_config, inactive_data_subject, db, generate_auth_header ): auth_header = generate_auth_header(scopes=[SYSTEM_CREATE]) @@ -842,7 +842,7 @@ def test_system_create_inactive_data_subject( "user.name.last", ], "data_use": "marketing", - "data_subjects": [data_subject.fides_key], + "data_subjects": [inactive_data_subject.fides_key], } ], } @@ -851,7 +851,7 @@ def test_system_create_inactive_data_subject( assert result.status_code == HTTP_400_BAD_REQUEST assert result.json() == { - "detail": f"Invalid privacy declaration referencing inactive DataSubject {data_subject.fides_key}" + "detail": f"Invalid privacy declaration referencing inactive DataSubject {inactive_data_subject.fides_key}" } assert not System.all(db) # ensure our system wasn't created @@ -1646,12 +1646,12 @@ def test_system_update_privacy_declaration_inactive_data_use( system, test_config, system_update_request_body, - data_use, + inactive_data_use, generate_role_header, db, ): auth_header = generate_role_header(roles=[OWNER]) - system_update_request_body.privacy_declarations[0].data_use = data_use.fides_key + system_update_request_body.privacy_declarations[0].data_use = inactive_data_use.fides_key result = _api.update( url=test_config.cli.server_url, headers=auth_header, @@ -1660,7 +1660,7 @@ def test_system_update_privacy_declaration_inactive_data_use( ) assert result.status_code == HTTP_400_BAD_REQUEST assert result.json() == { - "detail": f"Invalid privacy declaration referencing inactive DataUse {data_use.fides_key}" + "detail": f"Invalid privacy declaration referencing inactive DataUse {inactive_data_use.fides_key}" } # assert the system's privacy declaration has not been updated db.refresh(system) @@ -1671,13 +1671,13 @@ def test_system_update_privacy_declaration_inactive_data_category( system, test_config, system_update_request_body, - data_category, + inactive_data_category, generate_role_header, db, ): auth_header = generate_role_header(roles=[OWNER]) system_update_request_body.privacy_declarations[0].data_categories = [ - data_category.fides_key + inactive_data_category.fides_key ] result = _api.update( url=test_config.cli.server_url, @@ -1687,7 +1687,7 @@ def test_system_update_privacy_declaration_inactive_data_category( ) assert result.status_code == HTTP_400_BAD_REQUEST assert result.json() == { - "detail": f"Invalid privacy declaration referencing inactive DataCategory {data_category.fides_key}" + "detail": f"Invalid privacy declaration referencing inactive DataCategory {inactive_data_category.fides_key}" } # assert the system's privacy declaration has not been updated db.refresh(system) @@ -1700,13 +1700,13 @@ def test_system_update_privacy_declaration_inactive_data_subject( system, test_config, system_update_request_body, - data_subject, + inactive_data_subject, generate_role_header, db, ): auth_header = generate_role_header(roles=[OWNER]) system_update_request_body.privacy_declarations[0].data_subjects = [ - data_subject.fides_key + inactive_data_subject.fides_key ] result = _api.update( url=test_config.cli.server_url, @@ -1716,7 +1716,7 @@ def test_system_update_privacy_declaration_inactive_data_subject( ) assert result.status_code == HTTP_400_BAD_REQUEST assert result.json() == { - "detail": f"Invalid privacy declaration referencing inactive DataSubject {data_subject.fides_key}" + "detail": f"Invalid privacy declaration referencing inactive DataSubject {inactive_data_subject.fides_key}" } # assert the system's privacy declaration has not been updated db.refresh(system) From f5091b24b756542762763b9c3a74c96005f4e50d Mon Sep 17 00:00:00 2001 From: Eliana Rosselli Date: Fri, 14 Jun 2024 11:23:53 -0300 Subject: [PATCH 7/7] Fix breaking tests --- tests/ctl/core/test_api.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/ctl/core/test_api.py b/tests/ctl/core/test_api.py index 351cedde03..31bc741ce8 100644 --- a/tests/ctl/core/test_api.py +++ b/tests/ctl/core/test_api.py @@ -88,6 +88,7 @@ def fixture_inactive_data_category(db: Session) -> typing.Generator: db=db, data={ "fides_key": fides_key, + "name": "Inactive Category", "active": False, }, ) @@ -107,6 +108,7 @@ def fixture_inactive_data_use(db: Session) -> typing.Generator: db=db, data={ "fides_key": fides_key, + "name": "Inactive Use", "active": False, }, ) @@ -126,6 +128,7 @@ def fixture_inactive_data_subject(db: Session) -> typing.Generator: db=db, data={ "fides_key": fides_key, + "name": "Inactive Subject", "active": False, }, ) @@ -1651,7 +1654,9 @@ def test_system_update_privacy_declaration_inactive_data_use( db, ): auth_header = generate_role_header(roles=[OWNER]) - system_update_request_body.privacy_declarations[0].data_use = inactive_data_use.fides_key + system_update_request_body.privacy_declarations[0].data_use = ( + inactive_data_use.fides_key + ) result = _api.update( url=test_config.cli.server_url, headers=auth_header,