-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PROD-2134: Adds validation for taxonomy labels #4982
Changes from all commits
6010aad
1128ab1
5f0fe85
79d9f96
e171407
61b7cea
f5091b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the same happened here |
||
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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,13 @@ | |
|
||
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 | ||
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 | ||
|
@@ -20,6 +21,8 @@ | |
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 +51,54 @@ | |
return system | ||
|
||
|
||
async def validate_data_labels( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wasn't sure what the best name for this was so suggestions are appreciated 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good to me, a docstring added to this function will add further clarification There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added the docstring in 61b7cea |
||
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( | ||
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}", | ||
) | ||
|
||
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 | ||
""" | ||
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]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, putting this in a list so validate_data_labels can be reused 👍 |
||
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( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
running the
static_checks
command caused this changeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including this cleanup here 👍