From 4cc2055a198ed3ab9122c21e9ae7b9013405d841 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 9 Feb 2023 11:17:24 -0500 Subject: [PATCH 01/21] Support API overrides for messaging-related config properties. This includes a ConfigProxy construct that allows for config property resolution, which is required now that we are allowing API overwrites of config properties set via "traditional" config means. --- ...a_add_config_set_column_to_application_.py | 36 ++ src/fides/api/main.py | 9 + src/fides/api/ops/api/deps.py | 6 + .../ops/api/v1/endpoints/config_endpoints.py | 5 +- .../v1/endpoints/consent_request_endpoints.py | 21 +- .../api/ops/api/v1/endpoints/drp_endpoints.py | 4 +- .../identity_verification_endpoints.py | 6 +- .../v1/endpoints/privacy_request_endpoints.py | 46 +- .../api/ops/models/application_config.py | 129 +++++- src/fides/api/ops/models/storage.py | 6 +- .../api/ops/schemas/application_config.py | 54 ++- src/fides/api/ops/service/_verification.py | 4 +- .../messaging/message_dispatch_service.py | 13 +- .../privacy_request/request_runner_service.py | 13 +- .../privacy_request/request_service.py | 6 +- src/fides/core/config/config_proxy.py | 86 ++++ .../api/v1/endpoints/test_config_endpoints.py | 123 ++++- .../test_consent_request_endpoints.py | 7 +- .../test_identity_verification_endpoints.py | 14 +- .../test_privacy_request_endpoints.py | 19 +- tests/ops/conftest.py | 24 +- tests/ops/models/test_application_config.py | 435 +++++++++++++++++- .../request_runner_service_test.py | 7 +- 23 files changed, 987 insertions(+), 86 deletions(-) create mode 100644 src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py create mode 100644 src/fides/core/config/config_proxy.py diff --git a/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py b/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py new file mode 100644 index 0000000000..6c33ffc42f --- /dev/null +++ b/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py @@ -0,0 +1,36 @@ +"""add config_set column to application settings + +Revision ID: c9ee230fa6da +Revises: 5d62bab40b71 +Create Date: 2023-02-01 15:13:52.133075 + +""" +import sqlalchemy as sa +import sqlalchemy_utils +from alembic import op + +# revision identifiers, used by Alembic. +revision = "c9ee230fa6da" +down_revision = "5d62bab40b71" +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.add_column( + "applicationconfig", + sa.Column( + "config_set", + sqlalchemy_utils.types.encrypted.encrypted_type.StringEncryptedType(), + nullable=False, + ), + ) + + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column("applicationconfig", "config_set") + # ### end Alembic commands ### diff --git a/src/fides/api/main.py b/src/fides/api/main.py index 88400cbcc0..6650253ef7 100644 --- a/src/fides/api/main.py +++ b/src/fides/api/main.py @@ -53,6 +53,7 @@ FunctionalityNotConfigured, RedisConnectionError, ) +from fides.api.ops.models.application_config import ApplicationConfig from fides.api.ops.schemas.analytics import Event, ExtraData from fides.api.ops.service.connectors.saas.connector_registry_service import ( load_registry, @@ -241,6 +242,14 @@ async def setup_server() -> None: logger.error("Error creating parent user: {}", str(e)) raise FidesError(f"Error creating parent user: {str(e)}") + logger.info("Loading config settings into database...") + try: + db = get_api_session() + ApplicationConfig.set_config_set_config(db, CONFIG) + except Exception as e: + logger.error("Error occurred writing config settings to database: {}", str(e)) + return + logger.info("Validating SaaS connector templates...") try: registry = load_registry(registry_file) diff --git a/src/fides/api/ops/api/deps.py b/src/fides/api/ops/api/deps.py index ae561def81..fbb0d866ef 100644 --- a/src/fides/api/ops/api/deps.py +++ b/src/fides/api/ops/api/deps.py @@ -1,11 +1,13 @@ from typing import Generator +from fastapi import Depends from sqlalchemy.orm import Session from fides.api.ops.common_exceptions import FunctionalityNotConfigured from fides.api.ops.util.cache import get_cache as get_redis_connection from fides.core.config import FidesConfig from fides.core.config import get_config as get_app_config +from fides.core.config.config_proxy import ConfigProxy from fides.lib.db.session import get_db_engine, get_db_session _engine = None @@ -36,6 +38,10 @@ def get_api_session() -> Session: return db +def get_config_proxy(db: Session = Depends(get_db)) -> ConfigProxy: + return ConfigProxy(db) + + def get_cache() -> Generator: """Return a connection to our redis cache""" if not CONFIG.redis.enabled: diff --git a/src/fides/api/ops/api/v1/endpoints/config_endpoints.py b/src/fides/api/ops/api/v1/endpoints/config_endpoints.py index ca53612001..9f5cc25317 100644 --- a/src/fides/api/ops/api/v1/endpoints/config_endpoints.py +++ b/src/fides/api/ops/api/v1/endpoints/config_endpoints.py @@ -43,6 +43,7 @@ def get_config( status_code=HTTP_200_OK, dependencies=[Security(verify_oauth_client, scopes=[scopes.CONFIG_UPDATE])], response_model=ApplicationConfigSchema, + response_model_exclude_unset=True, ) def update_settings( *, @@ -56,8 +57,8 @@ def update_settings( i.e. true PATCH behavior. """ logger.info("Updating application settings") - updated_settings: ApplicationConfig = ApplicationConfig.create_or_update( - db, data={"api_set": data.dict()} + db, data={"api_set": data.dict(exclude_none=True)} ) + print(updated_settings.api_set) return updated_settings.api_set diff --git a/src/fides/api/ops/api/v1/endpoints/consent_request_endpoints.py b/src/fides/api/ops/api/v1/endpoints/consent_request_endpoints.py index e5d6ac9a85..35873f28c7 100644 --- a/src/fides/api/ops/api/v1/endpoints/consent_request_endpoints.py +++ b/src/fides/api/ops/api/v1/endpoints/consent_request_endpoints.py @@ -17,7 +17,7 @@ ) from fides.api.ctl.database.seed import DEFAULT_CONSENT_POLICY -from fides.api.ops.api.deps import get_db +from fides.api.ops.api.deps import get_config_proxy, get_db from fides.api.ops.api.v1.endpoints.privacy_request_endpoints import ( create_privacy_request_func, ) @@ -58,6 +58,7 @@ from fides.api.ops.util.logger import Pii from fides.api.ops.util.oauth_util import verify_oauth_client from fides.core.config import get_config +from fides.core.config.config_proxy import ConfigProxy router = APIRouter(tags=["Consent"], prefix=V1_URL_PREFIX) @@ -73,6 +74,7 @@ def create_consent_request( *, db: Session = Depends(get_db), + config_proxy: ConfigProxy = Depends(get_config_proxy), data: Identity, ) -> ConsentRequestResponse: """Creates a verification code for the user to verify access to manage consent preferences.""" @@ -97,7 +99,7 @@ def create_consent_request( } consent_request = ConsentRequest.create(db, data=consent_request_data) - if CONFIG.execution.subject_identity_verification_required: + if config_proxy.execution.subject_identity_verification_required: try: send_verification_code_to_user(db, consent_request, data) except MessageDispatchException as exc: @@ -168,11 +170,14 @@ def consent_request_verify( }, ) def get_consent_preferences_no_id( - *, db: Session = Depends(get_db), consent_request_id: str + *, + db: Session = Depends(get_db), + config_proxy: ConfigProxy = Depends(get_config_proxy), + consent_request_id: str, ) -> ConsentPreferences: """Returns the current consent preferences if successful.""" - if CONFIG.execution.subject_identity_verification_required: + if config_proxy.execution.subject_identity_verification_required: raise HTTPException( status_code=HTTP_400_BAD_REQUEST, detail="Retrieving consent preferences without identity verification is " @@ -267,6 +272,7 @@ def queue_privacy_request_to_propagate_consent( logger.info("Executable consent options: {}", executable_data_uses) privacy_request_results: BulkPostPrivacyRequests = create_privacy_request_func( db=db, + config_proxy=ConfigProxy(db), data=[ PrivacyRequestCreate( identity=identity, @@ -358,7 +364,7 @@ def _get_or_create_provided_identity( identity_data: Identity, ) -> ProvidedIdentity: """Based on target identity type, retrieves or creates associated ProvidedIdentity""" - target_identity_type: str = infer_target_identity_type(identity_data) + target_identity_type: str = infer_target_identity_type(db, identity_data) if target_identity_type == ProvidedIdentityType.email.value and identity_data.email: identity = ProvidedIdentity.filter( @@ -418,6 +424,7 @@ def _get_or_create_provided_identity( def infer_target_identity_type( + db: Session, identity_data: Identity, ) -> str: """ @@ -428,7 +435,7 @@ def infer_target_identity_type( """ if identity_data.email and identity_data.phone_number: messaging_method = get_messaging_method( - CONFIG.notifications.notification_service_type + ConfigProxy(db).notifications.notification_service_type ) if messaging_method == MessagingMethod.EMAIL: target_identity_type = ProvidedIdentityType.email.value @@ -458,7 +465,7 @@ def _get_consent_request_and_provided_identity( status_code=HTTP_404_NOT_FOUND, detail="Consent request not found" ) - if CONFIG.execution.subject_identity_verification_required: + if ConfigProxy(db).execution.subject_identity_verification_required: try: consent_request.verify_identity(verification_code) except IdentityVerificationException as exc: diff --git a/src/fides/api/ops/api/v1/endpoints/drp_endpoints.py b/src/fides/api/ops/api/v1/endpoints/drp_endpoints.py index c656e871b0..5e9f3b3c3e 100644 --- a/src/fides/api/ops/api/v1/endpoints/drp_endpoints.py +++ b/src/fides/api/ops/api/v1/endpoints/drp_endpoints.py @@ -47,6 +47,7 @@ from fides.api.ops.util.logger import Pii from fides.api.ops.util.oauth_util import verify_oauth_client from fides.core.config import get_config +from fides.core.config.config_proxy import ConfigProxy router = APIRouter(tags=["DRP"], prefix=urls.V1_URL_PREFIX) CONFIG = get_config() @@ -63,6 +64,7 @@ async def create_drp_privacy_request( *, cache: FidesopsRedis = Depends(deps.get_cache), db: Session = Depends(deps.get_db), + config_proxy: ConfigProxy = Depends(deps.get_config_proxy), data: DrpPrivacyRequestCreate, ) -> PrivacyRequestDRPStatusResponse: """ @@ -91,7 +93,7 @@ async def create_drp_privacy_request( ) privacy_request_kwargs: Dict[str, Any] = build_required_privacy_request_kwargs( - None, policy.id + None, policy.id, config_proxy.execution.subject_identity_verification_required ) try: diff --git a/src/fides/api/ops/api/v1/endpoints/identity_verification_endpoints.py b/src/fides/api/ops/api/v1/endpoints/identity_verification_endpoints.py index 4d9f023a07..a4920d94c3 100644 --- a/src/fides/api/ops/api/v1/endpoints/identity_verification_endpoints.py +++ b/src/fides/api/ops/api/v1/endpoints/identity_verification_endpoints.py @@ -10,7 +10,7 @@ IdentityVerificationConfigResponse, ) from fides.api.ops.util.api_router import APIRouter -from fides.core.config import get_config +from fides.core.config.config_proxy import ConfigProxy router = APIRouter(tags=["Identity Verification"], prefix=urls.V1_URL_PREFIX) @@ -22,11 +22,11 @@ def get_id_verification_config( *, db: Session = Depends(deps.get_db), + config_proxy: ConfigProxy = Depends(deps.get_config_proxy) ) -> IdentityVerificationConfigResponse: """Returns id verification config.""" - config = get_config() messaging_config: Optional[MessagingConfig] = db.query(MessagingConfig).first() return IdentityVerificationConfigResponse( - identity_verification_required=config.execution.subject_identity_verification_required, + identity_verification_required=config_proxy.execution.subject_identity_verification_required, valid_email_config_exists=bool(messaging_config and messaging_config.secrets), ) diff --git a/src/fides/api/ops/api/v1/endpoints/privacy_request_endpoints.py b/src/fides/api/ops/api/v1/endpoints/privacy_request_endpoints.py index 9d92683135..4bb7feabbb 100644 --- a/src/fides/api/ops/api/v1/endpoints/privacy_request_endpoints.py +++ b/src/fides/api/ops/api/v1/endpoints/privacy_request_endpoints.py @@ -147,6 +147,7 @@ from fides.api.ops.util.logger import Pii from fides.api.ops.util.oauth_util import verify_callback_oauth, verify_oauth_client from fides.core.config import get_config +from fides.core.config.config_proxy import ConfigProxy from fides.lib.models.audit_log import AuditLog, AuditLogAction from fides.lib.models.client import ClientDetail @@ -180,6 +181,7 @@ def get_privacy_request_or_error( def create_privacy_request( *, db: Session = Depends(deps.get_db), + config_proxy: ConfigProxy = Depends(deps.get_config_proxy), data: conlist(PrivacyRequestCreate, max_items=50) = Body(...), # type: ignore ) -> BulkPostPrivacyRequests: """ @@ -187,7 +189,7 @@ def create_privacy_request( or report failure and execute them within the Fidesops system. You cannot update privacy requests after they've been created. """ - return create_privacy_request_func(db, data, False) + return create_privacy_request_func(db, config_proxy, data, False) @router.post( @@ -199,6 +201,7 @@ def create_privacy_request( def create_privacy_request_authenticated( *, db: Session = Depends(deps.get_db), + config_proxy: ConfigProxy = Depends(deps.get_config_proxy), data: conlist(PrivacyRequestCreate, max_items=50) = Body(...), # type: ignore ) -> BulkPostPrivacyRequests: """ @@ -207,11 +210,13 @@ def create_privacy_request_authenticated( You cannot update privacy requests after they've been created. This route requires authentication instead of using verification codes. """ - return create_privacy_request_func(db, data, True) + return create_privacy_request_func(db, config_proxy, data, True) def _send_privacy_request_receipt_message_to_user( - policy: Optional[Policy], to_identity: Optional[Identity] + policy: Optional[Policy], + to_identity: Optional[Identity], + service_type: Optional[str], ) -> None: """Helper function to send request receipt message to the user""" if not to_identity: @@ -239,7 +244,7 @@ def _send_privacy_request_receipt_message_to_user( action_type=MessagingActionType.PRIVACY_REQUEST_RECEIPT, body_params=RequestReceiptBodyParams(request_types=request_types), ).dict(), - "service_type": CONFIG.notifications.notification_service_type, + "service_type": service_type, "to_identity": to_identity.dict(), }, ) @@ -1151,6 +1156,7 @@ def _send_privacy_request_review_message_to_user( action_type: MessagingActionType, identity_data: Dict[str, Any], rejection_reason: Optional[str], + service_type: Optional[str], ) -> None: """Helper method to send review notification message to user, shared between approve and deny""" if not identity_data: @@ -1174,7 +1180,7 @@ def _send_privacy_request_review_message_to_user( if action_type is MessagingActionType.PRIVACY_REQUEST_REVIEW_DENY else None, ).dict(), - "service_type": CONFIG.notifications.notification_service_type, + "service_type": service_type, "to_identity": to_identity.dict(), }, ) @@ -1189,6 +1195,7 @@ def verify_identification_code( privacy_request_id: str, *, db: Session = Depends(deps.get_db), + config_proxy: ConfigProxy = Depends(deps.get_config_proxy), provided_code: VerificationCode, ) -> PrivacyRequestResponse: """Verify the supplied identity verification code. @@ -1205,9 +1212,11 @@ def verify_identification_code( policy: Optional[Policy] = Policy.get( db=db, object_id=privacy_request.policy_id ) - if CONFIG.notifications.send_request_receipt_notification: + if config_proxy.notifications.send_request_receipt_notification: _send_privacy_request_receipt_message_to_user( - policy, privacy_request.get_persisted_identity() + policy, + privacy_request.get_persisted_identity(), + config_proxy.notifications.notification_service_type, ) except IdentityVerificationException as exc: raise HTTPException(status_code=HTTP_400_BAD_REQUEST, detail=exc.message) @@ -1217,7 +1226,7 @@ def verify_identification_code( logger.info("Identity verified for {}.", privacy_request.id) - if not CONFIG.execution.require_manual_request_approval: + if not config_proxy.execution.require_manual_request_approval: AuditLog.create( db=db, data={ @@ -1240,6 +1249,7 @@ def verify_identification_code( def approve_privacy_request( *, db: Session = Depends(deps.get_db), + config_proxy: ConfigProxy = Depends(deps.get_config_proxy), client: ClientDetail = Security( verify_oauth_client, scopes=[PRIVACY_REQUEST_REVIEW], @@ -1264,11 +1274,12 @@ def _approve_request(privacy_request: PrivacyRequest) -> None: "message": "", }, ) - if CONFIG.notifications.send_request_review_notification: + if config_proxy.notifications.send_request_review_notification: _send_privacy_request_review_message_to_user( action_type=MessagingActionType.PRIVACY_REQUEST_REVIEW_APPROVE, identity_data=privacy_request.get_cached_identity_data(), rejection_reason=None, + service_type=config_proxy.notifications.notification_service_type, ) queue_privacy_request(privacy_request_id=privacy_request.id) @@ -1288,6 +1299,7 @@ def _approve_request(privacy_request: PrivacyRequest) -> None: def deny_privacy_request( *, db: Session = Depends(deps.get_db), + config_proxy: ConfigProxy = Depends(deps.get_config_proxy), client: ClientDetail = Security( verify_oauth_client, scopes=[PRIVACY_REQUEST_REVIEW], @@ -1314,11 +1326,12 @@ def _deny_request( "message": privacy_requests.reason, }, ) - if CONFIG.notifications.send_request_review_notification: + if config_proxy.notifications.send_request_review_notification: _send_privacy_request_review_message_to_user( action_type=MessagingActionType.PRIVACY_REQUEST_REVIEW_DENY, identity_data=privacy_request.get_cached_identity_data(), rejection_reason=privacy_requests.reason, + service_type=config_proxy.notifications.notification_service_type, ) return review_privacy_request( @@ -1559,6 +1572,7 @@ def resume_privacy_request_from_requires_input( def create_privacy_request_func( db: Session, + config_proxy: ConfigProxy, data: conlist(PrivacyRequestCreate), # type: ignore authenticated: bool = False, ) -> BulkPostPrivacyRequests: @@ -1616,7 +1630,9 @@ def create_privacy_request_func( continue kwargs = build_required_privacy_request_kwargs( - privacy_request_data.requested_at, policy.id + privacy_request_data.requested_at, + policy.id, + config_proxy.execution.subject_identity_verification_required, ) for field in optional_fields: attr = getattr(privacy_request_data, field) @@ -1644,7 +1660,7 @@ def create_privacy_request_func( if ( not authenticated - and CONFIG.execution.subject_identity_verification_required + and config_proxy.execution.subject_identity_verification_required ): send_verification_code_to_user( db, privacy_request, privacy_request_data.identity @@ -1653,10 +1669,12 @@ def create_privacy_request_func( continue # Skip further processing for this privacy request if ( not authenticated - and CONFIG.notifications.send_request_receipt_notification + and config_proxy.notifications.send_request_receipt_notification ): _send_privacy_request_receipt_message_to_user( - policy, privacy_request_data.identity + policy, + privacy_request_data.identity, + config_proxy.notifications.notification_service_type, ) if not CONFIG.execution.require_manual_request_approval: AuditLog.create( diff --git a/src/fides/api/ops/models/application_config.py b/src/fides/api/ops/models/application_config.py index 0d66768d3a..9349fde682 100644 --- a/src/fides/api/ops/models/application_config.py +++ b/src/fides/api/ops/models/application_config.py @@ -1,7 +1,10 @@ from __future__ import annotations -from typing import Any, Dict +from json import loads +from typing import Any, Dict, Optional +from loguru import logger +from pydantic.utils import deep_update from sqlalchemy import Boolean, CheckConstraint, Column from sqlalchemy.ext.mutable import MutableDict from sqlalchemy.orm import Session @@ -11,7 +14,7 @@ ) from fides.api.ops.db.base_class import JSONTypeOverride -from fides.core.config import get_config +from fides.core.config import FidesConfig, get_config from fides.lib.db.base_class import Base CONFIG = get_config() @@ -36,6 +39,18 @@ class ApplicationConfig(Base): nullable=False, default={}, ) # store as encrypted JSON blob since config settings may have sensitive data + config_set = Column( + MutableDict.as_mutable( + StringEncryptedType( + JSONTypeOverride, + CONFIG.security.app_encryption_key, + AesGcmEngine, + "pkcs5", + ) + ), + nullable=False, + default={}, + ) # store as encrypted JSON blob since config settings may have sensitive data single_row = Column( Boolean, default=True, nullable=False ) # used to constrain table to one row @@ -60,14 +75,29 @@ def create_or_update( # type: ignore[override] def update(self, db: Session, data: Dict[str, Any]) -> ApplicationConfig: # type: ignore[override] """ - Updates the config record, merging contents of the JSON column + Updates the config record, merging contents of the particular JSON column that + corresponds to the updated data. + + Given `data` dict should have either/both an `api_set` and/or `config_set` key at the top-level. + Those keys will then point to `dict`s of data to update, i.e. merge, with the existing contents + of the `api_set` or `config_set` columns, respectively. """ - incoming_config = data["api_set"] - if not isinstance(incoming_config, dict): - raise ValueError("`api_set` column must be a dictionary") + if "api_set" in data: + incoming_api_config = data["api_set"] + if not isinstance(incoming_api_config, dict): + raise ValueError("`api_set` column must be a dictionary") + + # update, i.e. merge, the `api_set` dict + self.api_set = deep_update(self.api_set, incoming_api_config) + + if "config_set" in data: + incoming_config_config = data["config_set"] + if not isinstance(incoming_config_config, dict): + raise ValueError("`config_set` column must be a dictionary") + + # update, i.e. merge, the `config_set` dict + self.config_set = deep_update(self.config_set, incoming_config_config) - # update, i.e. merge, the `api_set` dict - self.api_set.update(incoming_config) self.save(db=db) return self @@ -82,3 +112,86 @@ def get_api_set_config(cls, db: Session) -> Dict[str, Any]: if config_record: return config_record.api_set return {} + + @classmethod + def get_config_set_config(cls, db: Session) -> Dict[str, Any]: + """ + Utility method to get the config_set config dict + + An empty `dict` will be returned if no config settings have been set through config + """ + config_record = db.query(cls).first() + if config_record: + return config_record.config_set + return {} + + @classmethod + def set_api_set_config( + cls, db: Session, api_set_dict: Dict[str, Any] + ) -> ApplicationConfig: + """ + Utility method to set the `api_set` column on the `applicationconfig` + db record with the provided dictionary of data. + + Updates are *merged* with any existing `api_set` data in the db. + """ + return cls.create_or_update(db, data={"api_set": api_set_dict}) + + @classmethod + def set_config_set_config( + cls, db: Session, config: FidesConfig + ) -> ApplicationConfig: + """ + Utility method to set the `config_set` column on the `applicationconfig` + db record by serializing the given `FidesConfig` to a JSON blob. + """ + # need to do this deserialization round-trip to get us a dict + # that's serializable as JSON in the db: + # pydantic's `.dict()` is NOT serializable as JSON -- + # see https://github.com/pydantic/pydantic/issues/1409 + config_dict = loads(config.json()) + return cls.create_or_update(db, data={"config_set": config_dict}) + + @classmethod + def get_resolved_config_property( + cls, db: Session, config_property: str, default_value: Any = None + ) -> Optional[Any]: + """ + Gets the 'resolved' config property based on api-set and config-set configs. + `config_property` is a dot-separated path to the config property, + e.g. `notifications.notification_service_type`. + + Api-set values get priority over config-set, in case of conflict. + """ + config_record = db.query(cls).first() + if config_record: + api_prop = _get_property(config_record.api_set, config_property) + if api_prop is None: + logger.info(f"No API-set {config_property} property found") + return _get_property( + config_record.config_set, config_property, default_value + ) + return api_prop + logger.warning("No config record found!") + return default_value + + +def _get_property( + properties: Any, property_path: str = None, default_value: Any = None +) -> Any: + """ + Internal helper to recursively traverse a property dict/value with a + dot-separated `property_path` string + """ + if property_path: + if not isinstance(properties, dict): + raise ValueError("Invalid path specified") + path_split = property_path.split(".", maxsplit=1) + index = path_split.pop(0) + try: + return _get_property( + properties[index], path_split[0] if path_split else None + ) + except KeyError: + return default_value + return properties diff --git a/src/fides/api/ops/models/storage.py b/src/fides/api/ops/models/storage.py index d446c93e31..3d0630144e 100644 --- a/src/fides/api/ops/models/storage.py +++ b/src/fides/api/ops/models/storage.py @@ -14,7 +14,6 @@ ) from fides.api.ops.db.base_class import JSONTypeOverride -from fides.api.ops.models.application_config import ApplicationConfig from fides.api.ops.schemas.storage.storage import ResponseFormat, StorageType from fides.api.ops.schemas.storage.storage_secrets_docs_only import ( possible_storage_secrets, @@ -22,6 +21,7 @@ from fides.api.ops.util.logger import Pii from fides.api.ops.util.storage_util import get_schema_for_secrets from fides.core.config import get_config +from fides.core.config.config_proxy import ConfigProxy from fides.lib.db.base_class import Base CONFIG = get_config() @@ -166,9 +166,7 @@ def get_active_default_storage_config(db: Session) -> Optional[StorageConfig]: via _either_ API or env var/toml, i.e. to be properly reconciled with the global pydantic config module. For now, though, we just rely on this being set through API. """ - api_app_settings = ApplicationConfig.get_api_set_config(db) active_default_storage_type = ( - api_app_settings.get("storage", {}).get("active_default_storage_type") - or StorageType.local + ConfigProxy(db).storage.active_default_storage_type or StorageType.local ) return get_default_storage_config_by_type(db, active_default_storage_type) diff --git a/src/fides/api/ops/schemas/application_config.py b/src/fides/api/ops/schemas/application_config.py index c1814c0812..289aba7119 100644 --- a/src/fides/api/ops/schemas/application_config.py +++ b/src/fides/api/ops/schemas/application_config.py @@ -1,6 +1,8 @@ from __future__ import annotations -from pydantic import Extra, validator +from typing import Dict, Optional + +from pydantic import Extra, root_validator, validator from fides.api.ops.schemas.storage.storage import StorageType from fides.lib.schemas.base_class import BaseSchema @@ -28,6 +30,46 @@ def validate_storage_type(cls, storage_type: StorageType) -> StorageType: return storage_type +# TODO: the below models classes are "duplicates" of the pydantic +# models that drive the application config. this is to allow every field +# to be optional on the API model, since we want PATCH functionality. +# ideally, we'd not need to duplicate the config modelclasses, and instead +# just make all fields optional by default for the API models. + + +class NotificationApplicationConfig(BaseSchema): + """ + API model - configuration settings for data subject and/or data processor notifications + """ + + send_request_completion_notification: Optional[bool] + send_request_receipt_notification: Optional[bool] + send_request_review_notification: Optional[bool] + notification_service_type: Optional[str] + + @validator("notification_service_type", pre=True) + @classmethod + def validate_notification_service_type(cls, value: Optional[str]) -> Optional[str]: + """Ensure the provided type is a valid value.""" + if value: + valid_values = ["MAILGUN", "TWILIO_TEXT", "TWILIO_EMAIL"] + value = value.upper() # force uppercase for safety + + if value not in valid_values: + raise ValueError( + f"Invalid NOTIFICATION_SERVICE_TYPE provided '{value}', must be one of: {', '.join([level for level in valid_values])}" + ) + + return value + + +class ExecutionApplicationConfig(BaseSchema): + subject_identity_verification_required: bool + + class Config: + extra = Extra.forbid + + class ApplicationConfig(BaseSchema): """ Application config settings update body is an arbitrary dict (JSON object) @@ -37,7 +79,15 @@ class ApplicationConfig(BaseSchema): the application config that is properly hooked up to the global pydantic config module. """ - storage: StorageApplicationConfig + storage: Optional[StorageApplicationConfig] + notifications: Optional[NotificationApplicationConfig] + execution: Optional[ExecutionApplicationConfig] + + @root_validator(pre=True) + def validate_not_empty(cls, values: Dict) -> Dict: + if not values: + raise ValueError("Config body cannot be empty!") + return values class Config: extra = Extra.forbid diff --git a/src/fides/api/ops/service/_verification.py b/src/fides/api/ops/service/_verification.py index da4fa45d1e..3e4b011c2f 100644 --- a/src/fides/api/ops/service/_verification.py +++ b/src/fides/api/ops/service/_verification.py @@ -13,6 +13,7 @@ generate_id_verification_code, ) from fides.core.config import get_config +from fides.core.config.config_proxy import ConfigProxy CONFIG = get_config() @@ -21,6 +22,7 @@ def send_verification_code_to_user( db: Session, request: ConsentRequest | PrivacyRequest, to_identity: Identity | None ) -> str: """Generate and cache a verification code, and then message the user""" + config_proxy = ConfigProxy(db) verification_code = generate_id_verification_code() request.cache_identity_verification_code(verification_code) messaging_action_type = ( @@ -32,7 +34,7 @@ def send_verification_code_to_user( db, action_type=messaging_action_type, to_identity=to_identity, - service_type=CONFIG.notifications.notification_service_type, + service_type=config_proxy.notifications.notification_service_type, message_body_params=SubjectIdentityVerificationBodyParams( verification_code=verification_code, verification_code_ttl_seconds=CONFIG.redis.identity_verification_code_ttl_seconds, diff --git a/src/fides/api/ops/service/messaging/message_dispatch_service.py b/src/fides/api/ops/service/messaging/message_dispatch_service.py index 886266b3dc..325dedcbe0 100644 --- a/src/fides/api/ops/service/messaging/message_dispatch_service.py +++ b/src/fides/api/ops/service/messaging/message_dispatch_service.py @@ -41,6 +41,7 @@ from fides.api.ops.tasks import MESSAGING_QUEUE_NAME, DatabaseTask, celery_app from fides.api.ops.util.logger import Pii from fides.core.config import get_config +from fides.core.config.config_proxy import ConfigProxy CONFIG = get_config() @@ -49,6 +50,7 @@ def check_and_dispatch_error_notifications(db: Session) -> None: + config_proxy = ConfigProxy(db) privacy_request_notifications = PrivacyRequestNotifications.all(db=db) if not privacy_request_notifications: return None @@ -60,7 +62,7 @@ def check_and_dispatch_error_notifications(db: Session) -> None: return None email_config = ( - CONFIG.notifications.notification_service_type in EMAIL_MESSAGING_SERVICES + config_proxy.notifications.notification_service_type in EMAIL_MESSAGING_SERVICES ) if ( @@ -77,7 +79,7 @@ def check_and_dispatch_error_notifications(db: Session) -> None: unsent_errors=len(unsent_errors) ), ).dict(), - "service_type": CONFIG.notifications.notification_service_type, + "service_type": config_proxy.notifications.notification_service_type, "to_identity": {"email": email}, }, ) @@ -154,12 +156,9 @@ def dispatch_message( body_params=message_body_params, ) else: - logger.error( - "Notification service type is not valid: {}", - CONFIG.notifications.notification_service_type, - ) + logger.error("Notification service type is not valid: {}", service_type) raise MessageDispatchException( - f"Notification service type is not valid: {CONFIG.notifications.notification_service_type}" + f"Notification service type is not valid: {service_type}" ) messaging_service: MessagingServiceType = messaging_config.service_type # type: ignore logger.info( diff --git a/src/fides/api/ops/service/privacy_request/request_runner_service.py b/src/fides/api/ops/service/privacy_request/request_runner_service.py index 2fc32c4602..14f7c82457 100644 --- a/src/fides/api/ops/service/privacy_request/request_runner_service.py +++ b/src/fides/api/ops/service/privacy_request/request_runner_service.py @@ -76,6 +76,7 @@ from fides.api.ops.util.logger import Pii, _log_exception, _log_warning from fides.api.ops.util.wrappers import sync from fides.core.config import get_config +from fides.core.config.config_proxy import ConfigProxy from fides.lib.db.session import get_db_session from fides.lib.models.audit_log import AuditLog, AuditLogAction from fides.lib.schemas.base_class import BaseSchema @@ -443,7 +444,7 @@ async def run_privacy_request( if not proceed: return - if CONFIG.notifications.send_request_completion_notification: + if ConfigProxy(session).notifications.send_request_completion_notification: try: initiate_privacy_request_completion_email( session, policy, access_result_urls, identity_data @@ -508,7 +509,11 @@ def initiate_privacy_request_completion_email( :param access_result_urls: list of urls generated by access request upload :param identity_data: Dict of identity data """ - if not (identity_data.get(ProvidedIdentityType.email.value) or identity_data.get(ProvidedIdentityType.phone_number.value)): + config_proxy = ConfigProxy(session) + if not ( + identity_data.get(ProvidedIdentityType.email.value) + or identity_data.get(ProvidedIdentityType.phone_number.value) + ): raise IdentityNotFoundException( "Identity email or phone number was not found, so request completion message could not be sent." ) @@ -522,7 +527,7 @@ def initiate_privacy_request_completion_email( db=session, action_type=MessagingActionType.PRIVACY_REQUEST_COMPLETE_ACCESS, to_identity=to_identity, - service_type=CONFIG.notifications.notification_service_type, + service_type=config_proxy.notifications.notification_service_type, message_body_params=AccessRequestCompleteBodyParams( download_links=access_result_urls ), @@ -532,7 +537,7 @@ def initiate_privacy_request_completion_email( db=session, action_type=MessagingActionType.PRIVACY_REQUEST_COMPLETE_DELETION, to_identity=to_identity, - service_type=CONFIG.notifications.notification_service_type, + service_type=config_proxy.notifications.notification_service_type, message_body_params=None, ) diff --git a/src/fides/api/ops/service/privacy_request/request_service.py b/src/fides/api/ops/service/privacy_request/request_service.py index 9dbe62ff78..33fbda00cb 100644 --- a/src/fides/api/ops/service/privacy_request/request_service.py +++ b/src/fides/api/ops/service/privacy_request/request_service.py @@ -22,7 +22,9 @@ def build_required_privacy_request_kwargs( - requested_at: Optional[datetime], policy_id: str + requested_at: Optional[datetime], + policy_id: str, + verification_required: bool, ) -> Dict[str, Any]: """Build kwargs required for creating privacy request @@ -31,7 +33,7 @@ def build_required_privacy_request_kwargs( """ status = ( PrivacyRequestStatus.identity_unverified - if CONFIG.execution.subject_identity_verification_required + if verification_required else PrivacyRequestStatus.pending ) return { diff --git a/src/fides/core/config/config_proxy.py b/src/fides/core/config/config_proxy.py new file mode 100644 index 0000000000..a58f503235 --- /dev/null +++ b/src/fides/core/config/config_proxy.py @@ -0,0 +1,86 @@ +from __future__ import annotations + +from typing import Any, Optional + +from sqlalchemy.orm import Session + +from fides.api.ops.models.application_config import ApplicationConfig +from fides.api.ops.schemas.storage.storage import StorageType + + +class ConfigProxyBase: + """ + Base class that's used to make config proxy classes that corresopnd + to our config/settings sub-sections. + + Config proxy classes are a construct to allow for accessing "resolved" + config properties based on api-set and "traditional" config-set mechanisms. + Config proxy classes allow these "resolved" properties to be looked up + as if they were a "normal" pydantic config object, i.e. with dot notation, + e.g. `ConfigProxy(db).notifications.notification_service_type` + """ + + prefix: str + + def __init__(self, db: Session) -> None: + self._db = db + + def __getattribute__(self, __name: str) -> Any: + """ + This allows us to retrieve resolved config properties when + using the config proxy as a "normal" config object, i.e. with dot notation, + e.g. `config_proxy.notifications.notification_service_type + """ + if __name in ("_db", "prefix"): + return object.__getattribute__(self, __name) + return ApplicationConfig.get_resolved_config_property( + self._db, f"{self.prefix}.{__name}" + ) + + +class NotificationSettingsProxy(ConfigProxyBase): + prefix = "notifications" + + send_request_completion_notification: bool + send_request_receipt_notification: bool + send_request_review_notification: bool + notification_service_type: Optional[str] + + +class ExecutionSettingsProxy(ConfigProxyBase): + prefix = "execution" + + subject_identity_verification_required: bool + + +class StorageSettingsProxy(ConfigProxyBase): + prefix = "storage" + + active_default_storage_type: StorageType + + +class ConfigProxy: + """ + ConfigProxy instances allow access to "resolved" config properties + based on resolution between api-set and "traditional" config-set mechanisms. + ConfigProxy instances allow these "resolved" properties to be looked up + as if they were a "normal" pydantic config object, i.e. with dot notation, + e.g. `ConfigProxy(db).notifications.notification_service_type` + + To instantiate a `ConfigProxy`, you must have an active db `Session`, + since the `ConfigProxy` will rely on the db for property resolution. + + Instantiating a `ConfigProxy` is itself a cheap operation, + i.e. there is minimal resource overhead with instantiating a new `ConfigProxy`. + The `ConfigProxy` is a thin wrapper above the given db `Session` - + it relies on the given db `Session` and for all of its state. + + Lookups (i.e. attribute access) with the `ConfigProxy` do leverage + the underlying ORM model, but any db calls that are needed should be straightforward - + it leverages only a fixed single-row table holding the config state. + """ + + def __init__(self, db: Session) -> None: + self.notifications = NotificationSettingsProxy(db) + self.execution = ExecutionSettingsProxy(db) + self.storage = StorageSettingsProxy(db) diff --git a/tests/ops/api/v1/endpoints/test_config_endpoints.py b/tests/ops/api/v1/endpoints/test_config_endpoints.py index bd27848f75..9f1fd4b326 100644 --- a/tests/ops/api/v1/endpoints/test_config_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_config_endpoints.py @@ -1,3 +1,5 @@ +from typing import Any + import pytest from sqlalchemy.orm import Session from starlette.testclient import TestClient @@ -15,7 +17,16 @@ def url(self) -> str: @pytest.fixture(scope="function") def payload(self): - return {"storage": {"active_default_storage_type": StorageType.s3.value}} + return { + "storage": {"active_default_storage_type": StorageType.s3.value}, + "notifications": { + "notification_service_type": "TWILIO_TEXT", + "send_request_completion_notification": True, + "send_request_receipt_notification": True, + "send_request_review_notification": True, + }, + "execution": {"subject_identity_verification_required": True}, + } def test_patch_application_config_unauthenticated( self, api_client: TestClient, payload, url @@ -35,7 +46,7 @@ def test_patch_application_config_with_invalid_key( api_client: TestClient, generate_auth_header, url, - payload, + payload: dict[str, Any], ): auth_header = generate_auth_header([scopes.CONFIG_UPDATE]) response = api_client.patch( @@ -55,6 +66,12 @@ def test_patch_application_config_with_invalid_key( response = api_client.patch(url, headers=auth_header, json=payload) assert response.status_code == 422 + # and a nested bad key + payload.pop("bad_key") + payload["storage"]["bad_key"] = "12345" + response = api_client.patch(url, headers=auth_header, json=payload) + assert response.status_code == 422 + def test_patch_application_config_with_invalid_value( self, api_client: TestClient, generate_auth_header, url ): @@ -119,25 +136,113 @@ def test_patch_application_config( ) assert response.status_code == 200 response_settings = response.json() - assert response_settings["storage"]["active_default_storage_type"] == "s3" + assert response_settings["storage"] == payload["storage"] + assert response_settings["execution"] == payload["execution"] + assert response_settings["notifications"] == payload["notifications"] db_settings = db.query(ApplicationConfig).first() - assert db_settings.api_set["storage"]["active_default_storage_type"] == "s3" + assert db_settings.api_set["storage"] == payload["storage"] + assert db_settings.api_set["execution"] == payload["execution"] + assert db_settings.api_set["notifications"] == payload["notifications"] - payload["storage"]["active_default_storage_type"] = "local" + # try PATCHing a single property + updated_payload = {"storage": {"active_default_storage_type": "local"}} response = api_client.patch( url, headers=auth_header, - json=payload, + json=updated_payload, ) assert response.status_code == 200 response_settings = response.json() assert response_settings["storage"]["active_default_storage_type"] == "local" + # ensure other properties were not impacted + assert response_settings["execution"] == payload["execution"] + assert response_settings["notifications"] == payload["notifications"] db.refresh(db_settings) + # ensure property was updated on backend assert db_settings.api_set["storage"]["active_default_storage_type"] == "local" + # but other properties were not impacted + assert db_settings.api_set["execution"] == payload["execution"] + assert db_settings.api_set["notifications"] == payload["notifications"] + + # try PATCHing multiple properties in the same nested object + updated_payload = { + "execution": {"subject_identity_verification_required": False}, + "notifications": { + "notification_service_type": "MAILGUN", + "send_request_completion_notification": False, + }, + } + response = api_client.patch( + url, + headers=auth_header, + json=updated_payload, + ) + assert response.status_code == 200 + response_settings = response.json() + assert response_settings["storage"]["active_default_storage_type"] == "local" + assert ( + response_settings["execution"]["subject_identity_verification_required"] + is False + ) + assert ( + response_settings["notifications"]["notification_service_type"] == "MAILGUN" + ) + assert ( + response_settings["notifications"]["send_request_completion_notification"] + is False + ) + # ensure other properties were not impacted + assert ( + response_settings["notifications"]["send_request_receipt_notification"] + is True + ) - # TODO: once our schema allows for more than just a single settings field, - # we need to test that the PATCH can specify one or many settings fields - # and only update the fields that are passed + db.refresh(db_settings) + # ensure property was updated on backend + assert db_settings.api_set["storage"]["active_default_storage_type"] == "local" + assert ( + db_settings.api_set["execution"]["subject_identity_verification_required"] + is False + ) + assert ( + db_settings.api_set["notifications"]["notification_service_type"] + == "MAILGUN" + ) + assert ( + db_settings.api_set["notifications"]["send_request_completion_notification"] + is False + ) + # ensure other properties were not impacted + assert ( + db_settings.api_set["notifications"]["send_request_receipt_notification"] + is True + ) + + def test_patch_application_config_notifications_properties( + self, + api_client: TestClient, + generate_auth_header, + url, + payload, + db: Session, + ): + + payload = {"notifications": {"send_request_completion_notification": False}} + auth_header = generate_auth_header([scopes.CONFIG_UPDATE]) + response = api_client.patch( + url, + headers=auth_header, + json=payload, + ) + assert response.status_code == 200 + response_settings = response.json() + # this should look exactly like the payload - no additional properties + assert response_settings["notifications"] == payload["notifications"] + assert "execution" not in response_settings + db_settings = db.query(ApplicationConfig).first() + # this should look exactly like the payload - no additional properties + assert db_settings.api_set["notifications"] == payload["notifications"] + assert "execution" not in db_settings.api_set class TestGetApplicationConfigApiSet: diff --git a/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py b/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py index 005e5655d3..8a540084c0 100644 --- a/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py @@ -16,6 +16,7 @@ CONSENT_REQUEST_VERIFY, V1_URL_PREFIX, ) +from fides.api.ops.models.application_config import ApplicationConfig from fides.api.ops.models.privacy_request import ( Consent, ConsentRequest, @@ -61,20 +62,22 @@ def url(self) -> str: return f"{V1_URL_PREFIX}{CONSENT_REQUEST}" @pytest.fixture(scope="function") - def set_notification_service_type_to_none(self): + def set_notification_service_type_to_none(self, db): """Overrides autouse fixture to remove default notification service type""" original_value = CONFIG.notifications.notification_service_type CONFIG.notifications.notification_service_type = None + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.notification_service_type = original_value @pytest.fixture(scope="function") - def set_notification_service_type_to_twilio_sms(self): + def set_notification_service_type_to_twilio_sms(self, db): """Overrides autouse fixture to set notification service type to twilio sms""" original_value = CONFIG.notifications.notification_service_type CONFIG.notifications.notification_service_type = ( MessagingServiceType.TWILIO_TEXT.value ) + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.notification_service_type = original_value diff --git a/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py b/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py index 07005a150a..4877141411 100644 --- a/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py @@ -2,6 +2,7 @@ from starlette.testclient import TestClient from fides.api.ops.api.v1.urn_registry import ID_VERIFICATION_CONFIG, V1_URL_PREFIX +from fides.api.ops.models.application_config import ApplicationConfig from fides.core.config import get_config @@ -11,11 +12,22 @@ def url(self) -> str: return V1_URL_PREFIX + ID_VERIFICATION_CONFIG @pytest.fixture(scope="function") - def subject_identity_verification_required(self): + def subject_identity_verification_required(self, db): """Override autouse fixture to enable identity verification for tests""" config = get_config() original_value = config.execution.subject_identity_verification_required config.execution.subject_identity_verification_required = True + ApplicationConfig.set_config_set_config(db, config) + yield + config.execution.subject_identity_verification_required = original_value + + @pytest.fixture(scope="function") + def subject_identity_verification_required_via_api(self, db): + """Override autouse fixture to enable identity verification via APIfor tests""" + config = get_config() + original_value = config.execution.subject_identity_verification_required + config.execution.subject_identity_verification_required = False + ApplicationConfig.set_config_set_config(db, config) yield config.execution.subject_identity_verification_required = original_value diff --git a/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py b/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py index 49146d7e99..6a94026e60 100644 --- a/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py @@ -52,6 +52,7 @@ ) from fides.api.ops.graph.config import CollectionAddress from fides.api.ops.graph.graph import DatasetGraph +from fides.api.ops.models.application_config import ApplicationConfig from fides.api.ops.models.connectionconfig import ConnectionConfig from fides.api.ops.models.datasetconfig import DatasetConfig from fides.api.ops.models.policy import ActionType, CurrentStep, Policy @@ -1790,10 +1791,11 @@ def url(self, db, privacy_request): return V1_URL_PREFIX + PRIVACY_REQUEST_APPROVE @pytest.fixture(scope="function") - def privacy_request_review_notification_enabled(self): + def privacy_request_review_notification_enabled(self, db): """Enable request review notification""" original_value = CONFIG.notifications.send_request_review_notification CONFIG.notifications.send_request_review_notification = True + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.send_request_review_notification = original_value @@ -2007,10 +2009,11 @@ def url(self, db, privacy_request): return V1_URL_PREFIX + PRIVACY_REQUEST_DENY @pytest.fixture(autouse=True, scope="function") - def privacy_request_review_notification_enabled(self): + def privacy_request_review_notification_enabled(self, db): """Enable request review notification""" original_value = CONFIG.notifications.send_request_review_notification CONFIG.notifications.send_request_review_notification = True + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.send_request_review_notification = original_value @@ -2956,10 +2959,11 @@ def url(self, db, privacy_request): ) @pytest.fixture(scope="function") - def privacy_request_receipt_notification_enabled(self): + def privacy_request_receipt_notification_enabled(self, db): """Enable request receipt""" original_value = CONFIG.notifications.send_request_receipt_notification CONFIG.notifications.send_request_receipt_notification = True + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.send_request_receipt_notification = original_value @@ -3239,10 +3243,11 @@ def url(self, oauth_client: ClientDetail, policy) -> str: return V1_URL_PREFIX + PRIVACY_REQUESTS @pytest.fixture(scope="function") - def subject_identity_verification_required(self): + def subject_identity_verification_required(self, db): """Override autouse fixture to enable identity verification for tests""" original_value = CONFIG.execution.subject_identity_verification_required CONFIG.execution.subject_identity_verification_required = True + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original_value @@ -3793,10 +3798,11 @@ def url(self, oauth_client: ClientDetail, policy) -> str: return V1_URL_PREFIX + PRIVACY_REQUESTS @pytest.fixture(scope="function") - def privacy_request_receipt_notification_enabled(self): + def privacy_request_receipt_notification_enabled(self, db): """Enable request receipt notification""" original_value = CONFIG.notifications.send_request_receipt_notification CONFIG.notifications.send_request_receipt_notification = True + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.send_request_receipt_notification = original_value @@ -3909,9 +3915,10 @@ def url(self) -> str: return f"{V1_URL_PREFIX}{PRIVACY_REQUEST_AUTHENTICATED}" @pytest.fixture - def verification_config(self): + def verification_config(self, db): original = CONFIG.execution.subject_identity_verification_required CONFIG.execution.subject_identity_verification_required = True + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original diff --git a/tests/ops/conftest.py b/tests/ops/conftest.py index 6380894823..d87c41fe8d 100644 --- a/tests/ops/conftest.py +++ b/tests/ops/conftest.py @@ -19,6 +19,7 @@ from fides.api.ops.util.cache import get_cache from fides.core.api import db_action from fides.core.config import get_config +from fides.core.config.config_proxy import ConfigProxy from fides.lib.cryptography.schemas.jwt import ( JWE_ISSUED_AT, JWE_PAYLOAD_CLIENT_ID, @@ -78,6 +79,11 @@ def db(api_client) -> Generator: engine.dispose() +@pytest.fixture(scope="session") +def config_proxy(db) -> ConfigProxy: + return ConfigProxy(db) + + @pytest.fixture(autouse=True) def clear_db_tables(db): """Clear data from tables between tests. @@ -276,54 +282,60 @@ def require_manual_request_approval(): @pytest.fixture(scope="function") -def subject_identity_verification_required(): +def subject_identity_verification_required(db): """Enable identity verification.""" original_value = CONFIG.execution.subject_identity_verification_required CONFIG.execution.subject_identity_verification_required = True + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original_value @pytest.fixture(autouse=True, scope="function") -def subject_identity_verification_not_required(): +def subject_identity_verification_not_required(db): """Disable identity verification for most tests unless overridden""" original_value = CONFIG.execution.subject_identity_verification_required CONFIG.execution.subject_identity_verification_required = False + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original_value @pytest.fixture(autouse=True, scope="function") -def privacy_request_complete_email_notification_disabled(): +def privacy_request_complete_email_notification_disabled(db): """Disable request completion email for most tests unless overridden""" original_value = CONFIG.notifications.send_request_completion_notification CONFIG.notifications.send_request_completion_notification = False + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.send_request_completion_notification = original_value @pytest.fixture(autouse=True, scope="function") -def privacy_request_receipt_notification_disabled(): +def privacy_request_receipt_notification_disabled(db): """Disable request receipt notification for most tests unless overridden""" original_value = CONFIG.notifications.send_request_receipt_notification CONFIG.notifications.send_request_receipt_notification = False + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.send_request_receipt_notification = original_value @pytest.fixture(autouse=True, scope="function") -def privacy_request_review_notification_disabled(): +def privacy_request_review_notification_disabled(db): """Disable request review notification for most tests unless overridden""" original_value = CONFIG.notifications.send_request_review_notification CONFIG.notifications.send_request_review_notification = False + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.send_request_review_notification = original_value @pytest.fixture(scope="function", autouse=True) -def set_notification_service_type_mailgun(): +def set_notification_service_type_mailgun(db): """Set default notification service type""" original_value = CONFIG.notifications.notification_service_type CONFIG.notifications.notification_service_type = MessagingServiceType.MAILGUN.value + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.notification_service_type = original_value diff --git a/tests/ops/models/test_application_config.py b/tests/ops/models/test_application_config.py index 90253cdf36..c5adf10560 100644 --- a/tests/ops/models/test_application_config.py +++ b/tests/ops/models/test_application_config.py @@ -1,19 +1,32 @@ +from json import dumps from typing import Any, Dict import pytest from sqlalchemy.orm import Session -from fides.api.ops.models.application_config import ApplicationConfig +from fides.api.ops.models.application_config import ApplicationConfig, _get_property +from fides.core.config import get_config +from fides.core.config.config_proxy import ConfigProxy + +CONFIG = get_config() class TestApplicationConfigModel: @pytest.fixture(scope="function") def example_config_dict(self) -> Dict[str, str]: - return {"setting1": "value1", "setting2": "value2"} + return { + "setting1": "value1", + "setting2": "value2", + "setting3": { + "nested_setting_1": "nested_value_1", + "nested_setting_2": "nested_value_2", + }, + } + # TODO - parameterize everything to test for `config_set` as well @pytest.fixture(scope="function") - def example_config_record(self) -> Dict[str, Any]: - return {"api_set": {"setting1": "value1", "setting2": "value2"}} + def example_config_record(self, example_config_dict) -> Dict[str, Any]: + return {"api_set": example_config_dict} def test_create_or_update_keeps_single_record( self, @@ -34,7 +47,7 @@ def test_create_or_update_keeps_single_record( # make an arbitrary change to settings dict # and assert create_or_update just updated the same single record - example_config_record["api_set"]["setting3"] = "value3" + example_config_record["api_set"]["setting4"] = "value4" new_config_record: ApplicationConfig = ApplicationConfig.create_or_update( db, data=example_config_record ) @@ -72,6 +85,120 @@ def test_create_or_update_merges_settings_dict( assert config_record_db.api_set["setting2"] == "value2" assert config_record_db.id == config_record.id + def test_create_or_update_can_remove_entry( + self, + db: Session, + example_config_record: Dict[str, Any], + ): + config_record: ApplicationConfig = ApplicationConfig.create_or_update( + db, data=example_config_record + ) + assert config_record.api_set == example_config_record["api_set"] + assert len(db.query(ApplicationConfig).all()) == 1 + config_record_db = db.query(ApplicationConfig).first() + assert config_record_db.api_set == example_config_record["api_set"] + assert config_record_db.id == config_record.id + + # remove a setting by setting it to `None` + new_config_record = {"api_set": {"setting1": None}} + config_record: ApplicationConfig = ApplicationConfig.create_or_update( + db, data=new_config_record + ) + assert config_record.api_set["setting1"] is None + assert config_record.api_set["setting2"] == "value2" + assert len(db.query(ApplicationConfig).all()) == 1 + config_record_db = db.query(ApplicationConfig).first() + assert config_record_db.api_set["setting1"] is None + assert config_record_db.api_set["setting2"] == "value2" + assert config_record_db.id == config_record.id + + def test_create_or_update_merges_nested_properties( + self, + db: Session, + example_config_record: Dict[str, Any], + ): + config_record: ApplicationConfig = ApplicationConfig.create_or_update( + db, data=example_config_record + ) + assert config_record.api_set == example_config_record["api_set"] + assert len(db.query(ApplicationConfig).all()) == 1 + config_record_db = db.query(ApplicationConfig).first() + assert config_record_db.api_set == example_config_record["api_set"] + assert config_record_db.id == config_record.id + + # update a single nested property + new_config_record = { + "api_set": {"setting3": {"nested_setting_1": "new_nested_value_1"}} + } + config_record: ApplicationConfig = ApplicationConfig.create_or_update( + db, data=new_config_record + ) + assert config_record.api_set["setting1"] == "value1" + assert config_record.api_set["setting2"] == "value2" + assert ( + config_record.api_set["setting3"]["nested_setting_1"] + == "new_nested_value_1" + ) + assert config_record.api_set["setting3"]["nested_setting_2"] == "nested_value_2" + assert len(db.query(ApplicationConfig).all()) == 1 + config_record_db = db.query(ApplicationConfig).first() + assert config_record_db.api_set["setting1"] == "value1" + assert config_record_db.api_set["setting2"] == "value2" + assert ( + config_record_db.api_set["setting3"]["nested_setting_1"] + == "new_nested_value_1" + ) + assert ( + config_record_db.api_set["setting3"]["nested_setting_2"] == "nested_value_2" + ) + assert config_record_db.id == config_record.id + + # add a nested property + new_config_record = { + "api_set": {"setting3": {"nested_setting_3": "nested_value_3"}} + } + config_record: ApplicationConfig = ApplicationConfig.create_or_update( + db, data=new_config_record + ) + assert config_record.api_set["setting1"] == "value1" + assert config_record.api_set["setting2"] == "value2" + assert ( + config_record.api_set["setting3"]["nested_setting_1"] + == "new_nested_value_1" + ) + assert config_record.api_set["setting3"]["nested_setting_2"] == "nested_value_2" + assert config_record.api_set["setting3"]["nested_setting_3"] == "nested_value_3" + assert len(db.query(ApplicationConfig).all()) == 1 + config_record_db = db.query(ApplicationConfig).first() + assert config_record_db.api_set["setting1"] == "value1" + assert config_record_db.api_set["setting2"] == "value2" + assert ( + config_record_db.api_set["setting3"]["nested_setting_1"] + == "new_nested_value_1" + ) + assert ( + config_record_db.api_set["setting3"]["nested_setting_2"] == "nested_value_2" + ) + assert ( + config_record_db.api_set["setting3"]["nested_setting_3"] == "nested_value_3" + ) + assert config_record_db.id == config_record.id + + # change a nested property to non-nested + new_config_record = {"api_set": {"setting3": "value3"}} + config_record: ApplicationConfig = ApplicationConfig.create_or_update( + db, data=new_config_record + ) + assert config_record.api_set["setting1"] == "value1" + assert config_record.api_set["setting2"] == "value2" + assert config_record.api_set["setting3"] == "value3" + assert len(db.query(ApplicationConfig).all()) == 1 + config_record_db = db.query(ApplicationConfig).first() + assert config_record_db.api_set["setting1"] == "value1" + assert config_record_db.api_set["setting2"] == "value2" + assert config_record.api_set["setting3"] == "value3" + assert config_record_db.id == config_record.id + def test_get_api_config_nothing_set( self, db: Session, @@ -83,3 +210,301 @@ def test_get_api_config(self, db: Session, example_config_record: Dict[str, Any] assert ( ApplicationConfig.get_api_set_config(db) == example_config_record["api_set"] ) + + def test_set_config_set_config(self, db): + ApplicationConfig.set_config_set_config(db, CONFIG) + # check a few specific config properties of different types on the database record + db_config = ApplicationConfig.get_config_set_config(db) + assert ( + db_config["notifications"]["notification_service_type"] + == CONFIG.notifications.notification_service_type + ) + assert ( + db_config["notifications"]["send_request_completion_notification"] + == CONFIG.notifications.send_request_completion_notification + ) + assert ( + db_config["execution"]["masking_strict"] == CONFIG.execution.masking_strict + ) + assert db_config["redis"]["port"] == CONFIG.redis.port + assert db_config["security"]["cors_origins"] == CONFIG.security.cors_origins + # and perform a general check on the config objects + assert dumps(db_config) == CONFIG.json() + + # change a few config values, set the db record, ensure it's updated + notification_service_type = CONFIG.notifications.notification_service_type + execution_strict = CONFIG.execution.masking_strict + CONFIG.notifications.notification_service_type = ( + notification_service_type + "_changed" + ) + CONFIG.execution.masking_strict = ( + not execution_strict + ) # since it's a boolean, switch the value + + # these shouldn't be equal before we update the db record + assert ( + db_config["notifications"]["notification_service_type"] + != CONFIG.notifications.notification_service_type + ) + assert ( + db_config["execution"]["masking_strict"] != CONFIG.execution.masking_strict + ) + # now we update the db record and all values should align again + ApplicationConfig.set_config_set_config(db, CONFIG) + db_config = ApplicationConfig.get_config_set_config(db) + assert ( + db_config["notifications"]["notification_service_type"] + == CONFIG.notifications.notification_service_type + ) + assert ( + db_config["notifications"]["send_request_completion_notification"] + == CONFIG.notifications.send_request_completion_notification + ) + assert ( + db_config["execution"]["masking_strict"] == CONFIG.execution.masking_strict + ) + assert db_config["redis"]["port"] == CONFIG.redis.port + assert db_config["security"]["cors_origins"] == CONFIG.security.cors_origins + assert dumps(db_config) == CONFIG.json() + + # reset config values to initial values to ensure we don't mess up any state + CONFIG.notifications.notification_service_type = notification_service_type + CONFIG.execution.masking_strict = execution_strict + + +class TestApplicationConfigResolution: + @pytest.mark.parametrize( + "prop_dict, prop_path, expected_value", + [ + ({}, "spam", None), + ({"spam": "ham"}, "eggs", None), + ({"spam": "ham"}, "spam", "ham"), + ({"spam": "ham", "eggs": "overeasy"}, "spam", "ham"), + ({"spam": "ham", "eggs": "overeasy"}, "eggs", "overeasy"), + ({"spam": 1}, "spam", 1), + ({"spam": False}, "spam", False), + ({"spam": ["ham", "eggs"]}, "spam", ["ham", "eggs"]), + ({"spam": "ham"}, "spam.", "ham"), + ({"spam": {"spam1": "ham1"}}, "spam", {"spam1": "ham1"}), + ({"spam": {"spam1": "ham1"}}, "spam.spam1", "ham1"), + ({"spam": {"spam1": "ham1"}, "eggs": "overeasy"}, "spam.spam1", "ham1"), + ({"spam": {"spam1": "ham1"}, "eggs": "overeasy"}, "spam.spam1.", "ham1"), + ( + {"spam": {"spam1": {"spam2": "ham2"}}, "eggs": "overeasy"}, + "spam.spam1.spam2", + "ham2", + ), + ( + {"spam": {"spam1": {"spam2": "ham2"}}, "eggs": "overeasy"}, + "spam.spam1", + {"spam2": "ham2"}, + ), + ( + {"spam": {"spam1": {"spam2": "ham2", "eggs": "overeasy"}}}, + "spam.spam1.spam2", + "ham2", + ), + ( + {"spam": {"spam1": {"spam2": "ham2", "eggs": "overeasy"}}}, + "spam.spam1.eggs", + "overeasy", + ), + ( + {"spam": {"spam1": {"spam2": "ham2", "eggs": ["over", "easy"]}}}, + "spam.spam1.eggs", + ["over", "easy"], + ), + ({"spam": {"spam1": {"spam2": "ham2", "eggs": 1}}}, "spam.spam1.eggs", 1), + ( + {"spam": {"spam1": {"spam2": "ham2", "eggs": False}}}, + "spam.spam1.eggs", + False, + ), + ], + ) + def test_get_property(self, prop_dict, prop_path, expected_value): + assert _get_property(prop_dict, prop_path) == expected_value + + @pytest.mark.parametrize( + "prop_dict, prop_path, expected_value, default_value", + [ + ({}, "spam", "ham", "ham"), + ({"spam": "ham"}, "eggs", "ham", "ham"), + ], + ) + def test_get_property_default( + self, prop_dict, prop_path, expected_value, default_value + ): + assert _get_property(prop_dict, prop_path, default_value) == expected_value + + @pytest.fixture(scope="function") + def example_config_dict(self) -> Dict[str, str]: + return { + "setting1": "value1", + "setting2": "value2", + "notifications": { + "notification_service_type": "twilio_email", + "nested_setting_2": "nested_value_2", + }, + } + + @pytest.fixture(scope="function") + def example_config_record(self, example_config_dict) -> Dict[str, Any]: + return {"api_set": example_config_dict} + + @pytest.fixture + def insert_example_config_record(self, db, example_config_record) -> Dict[str, Any]: + config_record: ApplicationConfig = ApplicationConfig.create_or_update( + db, data=example_config_record + ) + assert config_record.api_set == example_config_record["api_set"] + assert len(db.query(ApplicationConfig).all()) == 1 + config_record_db = db.query(ApplicationConfig).first() + assert config_record_db.api_set == example_config_record["api_set"] + assert config_record_db.id == config_record.id + return config_record.api_set + + @pytest.fixture + def insert_app_config(self, db): + ApplicationConfig.set_config_set_config(db, CONFIG) + + @pytest.mark.usefixtures("insert_app_config") + def test_get_resolved_config_property(self, db, insert_example_config_record): + notification_service_type = ApplicationConfig.get_resolved_config_property( + db, "notifications.notification_service_type" + ) + assert ( + notification_service_type + == insert_example_config_record["notifications"][ + "notification_service_type" + ] + ) + + # ensure a value that we did not override via API still has + # its original value returned + send_request_completion_notification = ( + ApplicationConfig.get_resolved_config_property( + db, "notifications.send_request_completion_notification" + ) + ) + assert ( + send_request_completion_notification + == CONFIG.notifications.send_request_completion_notification + ) + + +class TestConfigProxy: + @pytest.fixture(scope="function") + def example_config_dict(self) -> Dict[str, str]: + return { + "setting1": "value1", + "setting2": "value2", + "notifications": { + "notification_service_type": "twilio_email", + "nested_setting_2": "nested_value_2", + }, + } + + @pytest.fixture(scope="function") + def example_config_record(self, example_config_dict) -> Dict[str, Any]: + return {"api_set": example_config_dict} + + @pytest.fixture + def insert_example_config_record(self, db, example_config_record) -> Dict[str, Any]: + config_record: ApplicationConfig = ApplicationConfig.create_or_update( + db, data=example_config_record + ) + assert config_record.api_set == example_config_record["api_set"] + assert len(db.query(ApplicationConfig).all()) == 1 + config_record_db = db.query(ApplicationConfig).first() + assert config_record_db.api_set == example_config_record["api_set"] + assert config_record_db.id == config_record.id + return config_record.api_set + + @pytest.fixture + def insert_app_config(self, db): + ApplicationConfig.set_config_set_config(db, CONFIG) + + @pytest.fixture + def insert_app_config_set_notification_service_type_none(self, db): + service_type = CONFIG.notifications.notification_service_type + CONFIG.notifications.notification_service_type = None + ApplicationConfig.set_config_set_config(db, CONFIG) + yield + # set back to original value + CONFIG.notifications.notification_service_type = service_type + + @pytest.fixture + def insert_app_config_set_notifications_none(self, db): + notifications = CONFIG.notifications + CONFIG.notifications = None + ApplicationConfig.set_config_set_config(db, CONFIG) + yield + # set back to original value + CONFIG.notifications = notifications + + @pytest.mark.usefixtures("insert_app_config_set_notification_service_type_none") + def test_config_proxy_none_set(self, config_proxy: ConfigProxy): + + # we've explicitly made sure this property is not set via either traditional config + # variable or via API + assert config_proxy.notifications.notification_service_type is None + + @pytest.mark.usefixtures("insert_app_config_set_notification_service_type_none") + def test_config_proxy_none_set_whole_section(self, config_proxy: ConfigProxy): + + # we've explicitly made sure this whole section is not set via either traditional config + # variable or via API + + # this property should be None because it's optional + assert config_proxy.notifications.notification_service_type is None + + # this property is not optional, so we just check it against its default from the traditional config + assert ( + config_proxy.notifications.send_request_completion_notification + is CONFIG.notifications.send_request_review_notification + ) + + @pytest.mark.usefixtures("insert_app_config") + def test_config_proxy( + self, config_proxy: ConfigProxy, insert_example_config_record + ): + + notification_service_type = config_proxy.notifications.notification_service_type + assert ( + notification_service_type + == insert_example_config_record["notifications"][ + "notification_service_type" + ] + ) + + # ensure a value that we did not override via API still has + # its original value returned + send_request_completion_notification = ( + config_proxy.notifications.send_request_completion_notification + ) + assert ( + send_request_completion_notification + == CONFIG.notifications.send_request_completion_notification + ) + + @pytest.mark.usefixtures("insert_app_config") + def test_config_proxy_unset( + self, db, config_proxy: ConfigProxy, insert_example_config_record + ): + notification_service_type = config_proxy.notifications.notification_service_type + assert ( + notification_service_type + == insert_example_config_record["notifications"][ + "notification_service_type" + ] + ) + + # unset the API-set notification service type property + ApplicationConfig.set_api_set_config( + db, api_set_dict={"notifications": {"notification_service_type": None}} + ) + notification_service_type = config_proxy.notifications.notification_service_type + assert ( + notification_service_type == CONFIG.notifications.notification_service_type + ) diff --git a/tests/ops/service/privacy_request/request_runner_service_test.py b/tests/ops/service/privacy_request/request_runner_service_test.py index a7ae2ded4f..f8a7d474ff 100644 --- a/tests/ops/service/privacy_request/request_runner_service_test.py +++ b/tests/ops/service/privacy_request/request_runner_service_test.py @@ -15,6 +15,7 @@ PrivacyRequestPaused, ) from fides.api.ops.graph.graph import DatasetGraph +from fides.api.ops.models.application_config import ApplicationConfig from fides.api.ops.models.connectionconfig import AccessLevel from fides.api.ops.models.messaging import MessagingConfig from fides.api.ops.models.policy import CurrentStep, PolicyPostWebhook @@ -67,10 +68,11 @@ @pytest.fixture(scope="function") -def privacy_request_complete_email_notification_enabled(): +def privacy_request_complete_email_notification_enabled(db): """Enable request completion email""" original_value = CONFIG.notifications.send_request_completion_notification CONFIG.notifications.send_request_completion_notification = True + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.send_request_completion_notification = original_value @@ -1850,10 +1852,11 @@ def test_email_connector_no_updates_needed( class TestPrivacyRequestsEmailNotifications: @pytest.fixture(scope="function") - def privacy_request_complete_email_notification_enabled(self): + def privacy_request_complete_email_notification_enabled(self, db): """Enable request completion email""" original_value = CONFIG.notifications.send_request_completion_notification CONFIG.notifications.send_request_completion_notification = True + ApplicationConfig.set_config_set_config(db, CONFIG) yield CONFIG.notifications.send_request_completion_notification = original_value From 4d2d561894c67f444bb4d33ecd3016461b3627e3 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 9 Feb 2023 11:50:45 -0500 Subject: [PATCH 02/21] update changelog --- CHANGELOG.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a809cac7ae..57187472d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,10 @@ The types of changes are: * Privacy Center * The consent config default value can depend on whether Global Privacy Control is enabled. [#2341](https://github.com/ethyca/fides/pull/2341) +* Backend + * Add default storage configuration functionality and associated APIs [#2438](https://github.com/ethyca/fides/pull/2438) + * Add API support for messaging config properties [#2551](https://github.com/ethyca/fides/pull/2551) + ### Changed * Update Admin UI to show all action types (access, erasure, consent, update) [#2523](https://github.com/ethyca/fides/pull/2523) @@ -55,8 +59,6 @@ The types of changes are: * Issue addressing missing field in dataset migration [#2510](https://github.com/ethyca/fides/pull/2510) -### Added -* Add default storage configuration functionality and associated APIs [#2438](https://github.com/ethyca/fides/pull/2438) ## [2.6.1](https://github.com/ethyca/fides/compare/2.6.0...2.6.1) From c7f4334e86a792516ed7dda18ce904bfab596926 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 9 Feb 2023 12:03:07 -0500 Subject: [PATCH 03/21] add config_set column to db yml --- .fides/db_dataset.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.fides/db_dataset.yml b/.fides/db_dataset.yml index c4a58c5850..ff63f37dab 100644 --- a/.fides/db_dataset.yml +++ b/.fides/db_dataset.yml @@ -45,6 +45,10 @@ dataset: data_categories: - system.operations data_qualifier: aggregated.anonymized.unlinked_pseudonymized.pseudonymized.identified + - name: config_set + data_categories: + - system.operations + data_qualifier: aggregated.anonymized.unlinked_pseudonymized.pseudonymized.identified - name: updated_at data_categories: - system.operations From 5885b857abd155d05f2a8581990b525d05b7463b Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 9 Feb 2023 12:03:24 -0500 Subject: [PATCH 04/21] add future annotations import for python 3.8 compatibility --- tests/ops/api/v1/endpoints/test_config_endpoints.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ops/api/v1/endpoints/test_config_endpoints.py b/tests/ops/api/v1/endpoints/test_config_endpoints.py index 9f1fd4b326..16acbae17d 100644 --- a/tests/ops/api/v1/endpoints/test_config_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_config_endpoints.py @@ -1,3 +1,5 @@ +from __future__ import annotations + from typing import Any import pytest From 1c9e9b46bcadb121686c3286c05a55ffd47359a8 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 9 Feb 2023 16:55:33 -0500 Subject: [PATCH 05/21] some cleanup based on code review --- src/fides/api/main.py | 2 +- .../ops/api/v1/endpoints/config_endpoints.py | 2 +- .../api/ops/models/application_config.py | 14 +++-- .../api/ops/schemas/application_config.py | 2 +- .../test_consent_request_endpoints.py | 4 +- .../test_identity_verification_endpoints.py | 4 +- .../test_privacy_request_endpoints.py | 12 ++--- tests/ops/conftest.py | 12 ++--- tests/ops/models/test_application_config.py | 53 +++++-------------- .../request_runner_service_test.py | 4 +- 10 files changed, 39 insertions(+), 70 deletions(-) diff --git a/src/fides/api/main.py b/src/fides/api/main.py index 6650253ef7..5794ca8e71 100644 --- a/src/fides/api/main.py +++ b/src/fides/api/main.py @@ -245,7 +245,7 @@ async def setup_server() -> None: logger.info("Loading config settings into database...") try: db = get_api_session() - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) except Exception as e: logger.error("Error occurred writing config settings to database: {}", str(e)) return diff --git a/src/fides/api/ops/api/v1/endpoints/config_endpoints.py b/src/fides/api/ops/api/v1/endpoints/config_endpoints.py index 9f5cc25317..f4baa67efd 100644 --- a/src/fides/api/ops/api/v1/endpoints/config_endpoints.py +++ b/src/fides/api/ops/api/v1/endpoints/config_endpoints.py @@ -33,7 +33,7 @@ def get_config( logger.info("Getting the exposable Fides configuration") if api_set: logger.info("Retrieving api-set application settings") - return ApplicationConfig.get_api_set_config(db) + return ApplicationConfig.get_api_set(db) config = censor_config(get_app_config()) return config diff --git a/src/fides/api/ops/models/application_config.py b/src/fides/api/ops/models/application_config.py index 9349fde682..4ba25c4373 100644 --- a/src/fides/api/ops/models/application_config.py +++ b/src/fides/api/ops/models/application_config.py @@ -102,7 +102,7 @@ def update(self, db: Session, data: Dict[str, Any]) -> ApplicationConfig: # typ return self @classmethod - def get_api_set_config(cls, db: Session) -> Dict[str, Any]: + def get_api_set(cls, db: Session) -> Dict[str, Any]: """ Utility method to get the api_set config settings dict @@ -114,7 +114,7 @@ def get_api_set_config(cls, db: Session) -> Dict[str, Any]: return {} @classmethod - def get_config_set_config(cls, db: Session) -> Dict[str, Any]: + def get_config_set(cls, db: Session) -> Dict[str, Any]: """ Utility method to get the config_set config dict @@ -126,7 +126,7 @@ def get_config_set_config(cls, db: Session) -> Dict[str, Any]: return {} @classmethod - def set_api_set_config( + def update_api_set( cls, db: Session, api_set_dict: Dict[str, Any] ) -> ApplicationConfig: """ @@ -138,9 +138,7 @@ def set_api_set_config( return cls.create_or_update(db, data={"api_set": api_set_dict}) @classmethod - def set_config_set_config( - cls, db: Session, config: FidesConfig - ) -> ApplicationConfig: + def update_config_set(cls, db: Session, config: FidesConfig) -> ApplicationConfig: """ Utility method to set the `config_set` column on the `applicationconfig` db record by serializing the given `FidesConfig` to a JSON blob. @@ -177,7 +175,7 @@ def get_resolved_config_property( def _get_property( - properties: Any, property_path: str = None, default_value: Any = None + properties: Any, property_path: str | None = None, default_value: Any = None ) -> Any: """ Internal helper to recursively traverse a property dict/value with a @@ -185,7 +183,7 @@ def _get_property( """ if property_path: if not isinstance(properties, dict): - raise ValueError("Invalid path specified") + raise ValueError("Invalid property lookup") path_split = property_path.split(".", maxsplit=1) index = path_split.pop(0) try: diff --git a/src/fides/api/ops/schemas/application_config.py b/src/fides/api/ops/schemas/application_config.py index 289aba7119..b6e5c7fe1f 100644 --- a/src/fides/api/ops/schemas/application_config.py +++ b/src/fides/api/ops/schemas/application_config.py @@ -52,7 +52,7 @@ class NotificationApplicationConfig(BaseSchema): def validate_notification_service_type(cls, value: Optional[str]) -> Optional[str]: """Ensure the provided type is a valid value.""" if value: - valid_values = ["MAILGUN", "TWILIO_TEXT", "TWILIO_EMAIL"] + valid_values = ("MAILGUN", "TWILIO_TEXT", "TWILIO_EMAIL") value = value.upper() # force uppercase for safety if value not in valid_values: diff --git a/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py b/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py index 8a540084c0..14a420bca5 100644 --- a/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py @@ -66,7 +66,7 @@ def set_notification_service_type_to_none(self, db): """Overrides autouse fixture to remove default notification service type""" original_value = CONFIG.notifications.notification_service_type CONFIG.notifications.notification_service_type = None - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.notification_service_type = original_value @@ -77,7 +77,7 @@ def set_notification_service_type_to_twilio_sms(self, db): CONFIG.notifications.notification_service_type = ( MessagingServiceType.TWILIO_TEXT.value ) - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.notification_service_type = original_value diff --git a/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py b/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py index 4877141411..3ca179b313 100644 --- a/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py @@ -17,7 +17,7 @@ def subject_identity_verification_required(self, db): config = get_config() original_value = config.execution.subject_identity_verification_required config.execution.subject_identity_verification_required = True - ApplicationConfig.set_config_set_config(db, config) + ApplicationConfig.update_config_set(db, config) yield config.execution.subject_identity_verification_required = original_value @@ -27,7 +27,7 @@ def subject_identity_verification_required_via_api(self, db): config = get_config() original_value = config.execution.subject_identity_verification_required config.execution.subject_identity_verification_required = False - ApplicationConfig.set_config_set_config(db, config) + ApplicationConfig.update_config_set(db, config) yield config.execution.subject_identity_verification_required = original_value diff --git a/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py b/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py index 6a94026e60..f21ce573ce 100644 --- a/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py @@ -1795,7 +1795,7 @@ def privacy_request_review_notification_enabled(self, db): """Enable request review notification""" original_value = CONFIG.notifications.send_request_review_notification CONFIG.notifications.send_request_review_notification = True - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_review_notification = original_value @@ -2013,7 +2013,7 @@ def privacy_request_review_notification_enabled(self, db): """Enable request review notification""" original_value = CONFIG.notifications.send_request_review_notification CONFIG.notifications.send_request_review_notification = True - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_review_notification = original_value @@ -2963,7 +2963,7 @@ def privacy_request_receipt_notification_enabled(self, db): """Enable request receipt""" original_value = CONFIG.notifications.send_request_receipt_notification CONFIG.notifications.send_request_receipt_notification = True - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_receipt_notification = original_value @@ -3247,7 +3247,7 @@ def subject_identity_verification_required(self, db): """Override autouse fixture to enable identity verification for tests""" original_value = CONFIG.execution.subject_identity_verification_required CONFIG.execution.subject_identity_verification_required = True - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original_value @@ -3802,7 +3802,7 @@ def privacy_request_receipt_notification_enabled(self, db): """Enable request receipt notification""" original_value = CONFIG.notifications.send_request_receipt_notification CONFIG.notifications.send_request_receipt_notification = True - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_receipt_notification = original_value @@ -3918,7 +3918,7 @@ def url(self) -> str: def verification_config(self, db): original = CONFIG.execution.subject_identity_verification_required CONFIG.execution.subject_identity_verification_required = True - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original diff --git a/tests/ops/conftest.py b/tests/ops/conftest.py index d87c41fe8d..7117cb205f 100644 --- a/tests/ops/conftest.py +++ b/tests/ops/conftest.py @@ -286,7 +286,7 @@ def subject_identity_verification_required(db): """Enable identity verification.""" original_value = CONFIG.execution.subject_identity_verification_required CONFIG.execution.subject_identity_verification_required = True - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original_value @@ -296,7 +296,7 @@ def subject_identity_verification_not_required(db): """Disable identity verification for most tests unless overridden""" original_value = CONFIG.execution.subject_identity_verification_required CONFIG.execution.subject_identity_verification_required = False - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original_value @@ -306,7 +306,7 @@ def privacy_request_complete_email_notification_disabled(db): """Disable request completion email for most tests unless overridden""" original_value = CONFIG.notifications.send_request_completion_notification CONFIG.notifications.send_request_completion_notification = False - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_completion_notification = original_value @@ -316,7 +316,7 @@ def privacy_request_receipt_notification_disabled(db): """Disable request receipt notification for most tests unless overridden""" original_value = CONFIG.notifications.send_request_receipt_notification CONFIG.notifications.send_request_receipt_notification = False - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_receipt_notification = original_value @@ -326,7 +326,7 @@ def privacy_request_review_notification_disabled(db): """Disable request review notification for most tests unless overridden""" original_value = CONFIG.notifications.send_request_review_notification CONFIG.notifications.send_request_review_notification = False - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_review_notification = original_value @@ -336,6 +336,6 @@ def set_notification_service_type_mailgun(db): """Set default notification service type""" original_value = CONFIG.notifications.notification_service_type CONFIG.notifications.notification_service_type = MessagingServiceType.MAILGUN.value - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.notification_service_type = original_value diff --git a/tests/ops/models/test_application_config.py b/tests/ops/models/test_application_config.py index c5adf10560..f49cf4ffce 100644 --- a/tests/ops/models/test_application_config.py +++ b/tests/ops/models/test_application_config.py @@ -203,32 +203,16 @@ def test_get_api_config_nothing_set( self, db: Session, ): - assert ApplicationConfig.get_api_set_config(db) == {} + assert ApplicationConfig.get_api_set(db) == {} def test_get_api_config(self, db: Session, example_config_record: Dict[str, Any]): ApplicationConfig.create_or_update(db, data=example_config_record) - assert ( - ApplicationConfig.get_api_set_config(db) == example_config_record["api_set"] - ) + assert ApplicationConfig.get_api_set(db) == example_config_record["api_set"] - def test_set_config_set_config(self, db): - ApplicationConfig.set_config_set_config(db, CONFIG) + def test_update_config_set(self, db): + ApplicationConfig.update_config_set(db, CONFIG) # check a few specific config properties of different types on the database record - db_config = ApplicationConfig.get_config_set_config(db) - assert ( - db_config["notifications"]["notification_service_type"] - == CONFIG.notifications.notification_service_type - ) - assert ( - db_config["notifications"]["send_request_completion_notification"] - == CONFIG.notifications.send_request_completion_notification - ) - assert ( - db_config["execution"]["masking_strict"] == CONFIG.execution.masking_strict - ) - assert db_config["redis"]["port"] == CONFIG.redis.port - assert db_config["security"]["cors_origins"] == CONFIG.security.cors_origins - # and perform a general check on the config objects + db_config = ApplicationConfig.get_config_set(db) assert dumps(db_config) == CONFIG.json() # change a few config values, set the db record, ensure it's updated @@ -250,21 +234,8 @@ def test_set_config_set_config(self, db): db_config["execution"]["masking_strict"] != CONFIG.execution.masking_strict ) # now we update the db record and all values should align again - ApplicationConfig.set_config_set_config(db, CONFIG) - db_config = ApplicationConfig.get_config_set_config(db) - assert ( - db_config["notifications"]["notification_service_type"] - == CONFIG.notifications.notification_service_type - ) - assert ( - db_config["notifications"]["send_request_completion_notification"] - == CONFIG.notifications.send_request_completion_notification - ) - assert ( - db_config["execution"]["masking_strict"] == CONFIG.execution.masking_strict - ) - assert db_config["redis"]["port"] == CONFIG.redis.port - assert db_config["security"]["cors_origins"] == CONFIG.security.cors_origins + ApplicationConfig.update_config_set(db, CONFIG) + db_config = ApplicationConfig.get_config_set(db) assert dumps(db_config) == CONFIG.json() # reset config values to initial values to ensure we don't mess up any state @@ -366,7 +337,7 @@ def insert_example_config_record(self, db, example_config_record) -> Dict[str, A @pytest.fixture def insert_app_config(self, db): - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) @pytest.mark.usefixtures("insert_app_config") def test_get_resolved_config_property(self, db, insert_example_config_record): @@ -423,13 +394,13 @@ def insert_example_config_record(self, db, example_config_record) -> Dict[str, A @pytest.fixture def insert_app_config(self, db): - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) @pytest.fixture def insert_app_config_set_notification_service_type_none(self, db): service_type = CONFIG.notifications.notification_service_type CONFIG.notifications.notification_service_type = None - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield # set back to original value CONFIG.notifications.notification_service_type = service_type @@ -438,7 +409,7 @@ def insert_app_config_set_notification_service_type_none(self, db): def insert_app_config_set_notifications_none(self, db): notifications = CONFIG.notifications CONFIG.notifications = None - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield # set back to original value CONFIG.notifications = notifications @@ -501,7 +472,7 @@ def test_config_proxy_unset( ) # unset the API-set notification service type property - ApplicationConfig.set_api_set_config( + ApplicationConfig.update_api_set_config( db, api_set_dict={"notifications": {"notification_service_type": None}} ) notification_service_type = config_proxy.notifications.notification_service_type diff --git a/tests/ops/service/privacy_request/request_runner_service_test.py b/tests/ops/service/privacy_request/request_runner_service_test.py index f8a7d474ff..ae086fcc5c 100644 --- a/tests/ops/service/privacy_request/request_runner_service_test.py +++ b/tests/ops/service/privacy_request/request_runner_service_test.py @@ -72,7 +72,7 @@ def privacy_request_complete_email_notification_enabled(db): """Enable request completion email""" original_value = CONFIG.notifications.send_request_completion_notification CONFIG.notifications.send_request_completion_notification = True - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_completion_notification = original_value @@ -1856,7 +1856,7 @@ def privacy_request_complete_email_notification_enabled(self, db): """Enable request completion email""" original_value = CONFIG.notifications.send_request_completion_notification CONFIG.notifications.send_request_completion_notification = True - ApplicationConfig.set_config_set_config(db, CONFIG) + ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_completion_notification = original_value From 62feb226c7ac12c14210a1640749b9ade9731e2c Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Fri, 10 Feb 2023 16:34:41 +0100 Subject: [PATCH 06/21] update broken test reference --- tests/ops/models/test_application_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ops/models/test_application_config.py b/tests/ops/models/test_application_config.py index f49cf4ffce..75cd265770 100644 --- a/tests/ops/models/test_application_config.py +++ b/tests/ops/models/test_application_config.py @@ -472,7 +472,7 @@ def test_config_proxy_unset( ) # unset the API-set notification service type property - ApplicationConfig.update_api_set_config( + ApplicationConfig.update_api_set( db, api_set_dict={"notifications": {"notification_service_type": None}} ) notification_service_type = config_proxy.notifications.notification_service_type From 67535f52751ac34c7c2f5c99b5598f0ca8693aef Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Mon, 20 Feb 2023 15:30:20 +0000 Subject: [PATCH 07/21] censor api set config endpoint response --- .../ops/api/v1/endpoints/config_endpoints.py | 2 +- src/fides/core/config/__init__.py | 18 +++++++++++------- src/fides/core/config/utils.py | 3 +++ 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/fides/api/ops/api/v1/endpoints/config_endpoints.py b/src/fides/api/ops/api/v1/endpoints/config_endpoints.py index f4baa67efd..7c919b0f30 100644 --- a/src/fides/api/ops/api/v1/endpoints/config_endpoints.py +++ b/src/fides/api/ops/api/v1/endpoints/config_endpoints.py @@ -33,7 +33,7 @@ def get_config( logger.info("Getting the exposable Fides configuration") if api_set: logger.info("Retrieving api-set application settings") - return ApplicationConfig.get_api_set(db) + return censor_config(ApplicationConfig.get_api_set(db)) config = censor_config(get_app_config()) return config diff --git a/src/fides/core/config/__init__.py b/src/fides/core/config/__init__.py index 94fbb1db86..0fb93f59bf 100644 --- a/src/fides/core/config/__init__.py +++ b/src/fides/core/config/__init__.py @@ -5,7 +5,7 @@ from functools import lru_cache from os import getenv -from typing import Any, Dict, Optional, Tuple +from typing import Any, Dict, Optional, Tuple, Union import toml from loguru import logger as log @@ -93,18 +93,22 @@ def log_all_config_values(self) -> None: ) -def censor_config(config: FidesConfig) -> Dict[str, Any]: +def censor_config(config: Union[FidesConfig, Dict[str, Any]]) -> Dict[str, Any]: """ Returns a config that is safe to expose over the API. This function will strip out any keys not specified in the `CONFIG_KEY_ALLOWLIST` above. """ - as_dict = config.dict() + if not isinstance(config, Dict): + as_dict = config.dict() + else: + as_dict = config filtered: Dict[str, Any] = {} for key, value in CONFIG_KEY_ALLOWLIST.items(): - data = as_dict[key] - filtered[key] = {} - for field in value: - filtered[key][field] = data[field] + if key in as_dict: + data = as_dict[key] + filtered[key] = {} + for field in value: + filtered[key][field] = data[field] return filtered diff --git a/src/fides/core/config/utils.py b/src/fides/core/config/utils.py index 4d83562e21..2fde426a0f 100644 --- a/src/fides/core/config/utils.py +++ b/src/fides/core/config/utils.py @@ -50,4 +50,7 @@ def get_dev_mode() -> bool: "task_retry_backoff", "require_manual_request_approval", ], + "storage": [ + "active_default_storage_type", + ], } From b6084629ef8a5dfa43db644b2a14e4cb608a9b25 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Mon, 20 Feb 2023 15:31:45 +0000 Subject: [PATCH 08/21] ensure api config is reset in tests --- .../ops/api/v1/endpoints/test_consent_request_endpoints.py | 2 ++ .../v1/endpoints/test_identity_verification_endpoints.py | 2 ++ .../ops/api/v1/endpoints/test_privacy_request_endpoints.py | 6 ++++++ tests/ops/conftest.py | 6 ++++++ tests/ops/models/test_application_config.py | 2 ++ .../service/privacy_request/request_runner_service_test.py | 2 ++ 6 files changed, 20 insertions(+) diff --git a/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py b/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py index 14a420bca5..8e065bddb7 100644 --- a/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_consent_request_endpoints.py @@ -69,6 +69,7 @@ def set_notification_service_type_to_none(self, db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.notification_service_type = original_value + ApplicationConfig.update_config_set(db, CONFIG) @pytest.fixture(scope="function") def set_notification_service_type_to_twilio_sms(self, db): @@ -80,6 +81,7 @@ def set_notification_service_type_to_twilio_sms(self, db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.notification_service_type = original_value + ApplicationConfig.update_config_set(db, CONFIG) @pytest.mark.usefixtures( "messaging_config", diff --git a/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py b/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py index 3ca179b313..6a798c9352 100644 --- a/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_identity_verification_endpoints.py @@ -20,6 +20,7 @@ def subject_identity_verification_required(self, db): ApplicationConfig.update_config_set(db, config) yield config.execution.subject_identity_verification_required = original_value + ApplicationConfig.update_config_set(db, config) @pytest.fixture(scope="function") def subject_identity_verification_required_via_api(self, db): @@ -30,6 +31,7 @@ def subject_identity_verification_required_via_api(self, db): ApplicationConfig.update_config_set(db, config) yield config.execution.subject_identity_verification_required = original_value + ApplicationConfig.update_config_set(db, config) def test_get_config_with_verification_required_no_email_config( self, diff --git a/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py b/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py index f21ce573ce..7a63d16a83 100644 --- a/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py @@ -1798,6 +1798,7 @@ def privacy_request_review_notification_enabled(self, db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_review_notification = original_value + ApplicationConfig.update_config_set(db, CONFIG) def test_approve_privacy_request_not_authenticated(self, url, api_client): response = api_client.patch(url) @@ -2016,6 +2017,7 @@ def privacy_request_review_notification_enabled(self, db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_review_notification = original_value + ApplicationConfig.update_config_set(db, CONFIG) def test_deny_privacy_request_not_authenticated(self, url, api_client): response = api_client.patch(url) @@ -2966,6 +2968,7 @@ def privacy_request_receipt_notification_enabled(self, db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_receipt_notification = original_value + ApplicationConfig.update_config_set(db, CONFIG) def test_incorrect_privacy_request_status(self, api_client, url, privacy_request): request_body = {"code": self.code} @@ -3250,6 +3253,7 @@ def subject_identity_verification_required(self, db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original_value + ApplicationConfig.update_config_set(db, CONFIG) def test_create_privacy_request_no_email_config( self, @@ -3805,6 +3809,7 @@ def privacy_request_receipt_notification_enabled(self, db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_receipt_notification = original_value + ApplicationConfig.update_config_set(db, CONFIG) @mock.patch( "fides.api.ops.service.privacy_request.request_runner_service.run_privacy_request.delay" @@ -3921,6 +3926,7 @@ def verification_config(self, db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original + ApplicationConfig.update_config_set(db, CONFIG) @mock.patch( "fides.api.ops.service.privacy_request.request_runner_service.run_privacy_request.delay" diff --git a/tests/ops/conftest.py b/tests/ops/conftest.py index 7117cb205f..3959daa8f2 100644 --- a/tests/ops/conftest.py +++ b/tests/ops/conftest.py @@ -289,6 +289,7 @@ def subject_identity_verification_required(db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original_value + ApplicationConfig.update_config_set(db, CONFIG) @pytest.fixture(autouse=True, scope="function") @@ -299,6 +300,7 @@ def subject_identity_verification_not_required(db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.execution.subject_identity_verification_required = original_value + ApplicationConfig.update_config_set(db, CONFIG) @pytest.fixture(autouse=True, scope="function") @@ -309,6 +311,7 @@ def privacy_request_complete_email_notification_disabled(db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_completion_notification = original_value + ApplicationConfig.update_config_set(db, CONFIG) @pytest.fixture(autouse=True, scope="function") @@ -319,6 +322,7 @@ def privacy_request_receipt_notification_disabled(db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_receipt_notification = original_value + ApplicationConfig.update_config_set(db, CONFIG) @pytest.fixture(autouse=True, scope="function") @@ -329,6 +333,7 @@ def privacy_request_review_notification_disabled(db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_review_notification = original_value + ApplicationConfig.update_config_set(db, CONFIG) @pytest.fixture(scope="function", autouse=True) @@ -339,3 +344,4 @@ def set_notification_service_type_mailgun(db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.notification_service_type = original_value + ApplicationConfig.update_config_set(db, CONFIG) diff --git a/tests/ops/models/test_application_config.py b/tests/ops/models/test_application_config.py index 75cd265770..4c1b8a0cc5 100644 --- a/tests/ops/models/test_application_config.py +++ b/tests/ops/models/test_application_config.py @@ -404,6 +404,7 @@ def insert_app_config_set_notification_service_type_none(self, db): yield # set back to original value CONFIG.notifications.notification_service_type = service_type + ApplicationConfig.update_config_set(db, CONFIG) @pytest.fixture def insert_app_config_set_notifications_none(self, db): @@ -413,6 +414,7 @@ def insert_app_config_set_notifications_none(self, db): yield # set back to original value CONFIG.notifications = notifications + ApplicationConfig.update_config_set(db, CONFIG) @pytest.mark.usefixtures("insert_app_config_set_notification_service_type_none") def test_config_proxy_none_set(self, config_proxy: ConfigProxy): diff --git a/tests/ops/service/privacy_request/request_runner_service_test.py b/tests/ops/service/privacy_request/request_runner_service_test.py index ae086fcc5c..e89cb740e7 100644 --- a/tests/ops/service/privacy_request/request_runner_service_test.py +++ b/tests/ops/service/privacy_request/request_runner_service_test.py @@ -75,6 +75,7 @@ def privacy_request_complete_email_notification_enabled(db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_completion_notification = original_value + ApplicationConfig.update_config_set(db, CONFIG) @mock.patch( @@ -1859,6 +1860,7 @@ def privacy_request_complete_email_notification_enabled(self, db): ApplicationConfig.update_config_set(db, CONFIG) yield CONFIG.notifications.send_request_completion_notification = original_value + ApplicationConfig.update_config_set(db, CONFIG) @pytest.mark.integration_postgres @pytest.mark.integration From 7ff0ba0662349fe25b972fa12bf0e0280150f7b4 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Mon, 20 Feb 2023 15:33:19 +0000 Subject: [PATCH 09/21] use utility to update api config --- src/fides/api/ops/api/v1/endpoints/config_endpoints.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/fides/api/ops/api/v1/endpoints/config_endpoints.py b/src/fides/api/ops/api/v1/endpoints/config_endpoints.py index 7c919b0f30..ea1a4512f1 100644 --- a/src/fides/api/ops/api/v1/endpoints/config_endpoints.py +++ b/src/fides/api/ops/api/v1/endpoints/config_endpoints.py @@ -57,8 +57,7 @@ def update_settings( i.e. true PATCH behavior. """ logger.info("Updating application settings") - updated_settings: ApplicationConfig = ApplicationConfig.create_or_update( - db, data={"api_set": data.dict(exclude_none=True)} + update_config: ApplicationConfig = ApplicationConfig.update_api_set( + db, data.dict(exclude_none=True) ) - print(updated_settings.api_set) - return updated_settings.api_set + return update_config.api_set From db04c1b41e686f755704031f889e63af44559b8c Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Mon, 20 Feb 2023 15:34:25 +0000 Subject: [PATCH 10/21] use pydash get for nested lookup --- .../api/ops/models/application_config.py | 28 +------- tests/ops/models/test_application_config.py | 66 +------------------ 2 files changed, 4 insertions(+), 90 deletions(-) diff --git a/src/fides/api/ops/models/application_config.py b/src/fides/api/ops/models/application_config.py index 4ba25c4373..4c318eca20 100644 --- a/src/fides/api/ops/models/application_config.py +++ b/src/fides/api/ops/models/application_config.py @@ -5,6 +5,7 @@ from loguru import logger from pydantic.utils import deep_update +from pydash.objects import get from sqlalchemy import Boolean, CheckConstraint, Column from sqlalchemy.ext.mutable import MutableDict from sqlalchemy.orm import Session @@ -163,33 +164,10 @@ def get_resolved_config_property( """ config_record = db.query(cls).first() if config_record: - api_prop = _get_property(config_record.api_set, config_property) + api_prop = get(config_record.api_set, config_property) if api_prop is None: logger.info(f"No API-set {config_property} property found") - return _get_property( - config_record.config_set, config_property, default_value - ) + return get(config_record.config_set, config_property, default_value) return api_prop logger.warning("No config record found!") return default_value - - -def _get_property( - properties: Any, property_path: str | None = None, default_value: Any = None -) -> Any: - """ - Internal helper to recursively traverse a property dict/value with a - dot-separated `property_path` string - """ - if property_path: - if not isinstance(properties, dict): - raise ValueError("Invalid property lookup") - path_split = property_path.split(".", maxsplit=1) - index = path_split.pop(0) - try: - return _get_property( - properties[index], path_split[0] if path_split else None - ) - except KeyError: - return default_value - return properties diff --git a/tests/ops/models/test_application_config.py b/tests/ops/models/test_application_config.py index 4c1b8a0cc5..a68b7e012d 100644 --- a/tests/ops/models/test_application_config.py +++ b/tests/ops/models/test_application_config.py @@ -4,7 +4,7 @@ import pytest from sqlalchemy.orm import Session -from fides.api.ops.models.application_config import ApplicationConfig, _get_property +from fides.api.ops.models.application_config import ApplicationConfig from fides.core.config import get_config from fides.core.config.config_proxy import ConfigProxy @@ -244,70 +244,6 @@ def test_update_config_set(self, db): class TestApplicationConfigResolution: - @pytest.mark.parametrize( - "prop_dict, prop_path, expected_value", - [ - ({}, "spam", None), - ({"spam": "ham"}, "eggs", None), - ({"spam": "ham"}, "spam", "ham"), - ({"spam": "ham", "eggs": "overeasy"}, "spam", "ham"), - ({"spam": "ham", "eggs": "overeasy"}, "eggs", "overeasy"), - ({"spam": 1}, "spam", 1), - ({"spam": False}, "spam", False), - ({"spam": ["ham", "eggs"]}, "spam", ["ham", "eggs"]), - ({"spam": "ham"}, "spam.", "ham"), - ({"spam": {"spam1": "ham1"}}, "spam", {"spam1": "ham1"}), - ({"spam": {"spam1": "ham1"}}, "spam.spam1", "ham1"), - ({"spam": {"spam1": "ham1"}, "eggs": "overeasy"}, "spam.spam1", "ham1"), - ({"spam": {"spam1": "ham1"}, "eggs": "overeasy"}, "spam.spam1.", "ham1"), - ( - {"spam": {"spam1": {"spam2": "ham2"}}, "eggs": "overeasy"}, - "spam.spam1.spam2", - "ham2", - ), - ( - {"spam": {"spam1": {"spam2": "ham2"}}, "eggs": "overeasy"}, - "spam.spam1", - {"spam2": "ham2"}, - ), - ( - {"spam": {"spam1": {"spam2": "ham2", "eggs": "overeasy"}}}, - "spam.spam1.spam2", - "ham2", - ), - ( - {"spam": {"spam1": {"spam2": "ham2", "eggs": "overeasy"}}}, - "spam.spam1.eggs", - "overeasy", - ), - ( - {"spam": {"spam1": {"spam2": "ham2", "eggs": ["over", "easy"]}}}, - "spam.spam1.eggs", - ["over", "easy"], - ), - ({"spam": {"spam1": {"spam2": "ham2", "eggs": 1}}}, "spam.spam1.eggs", 1), - ( - {"spam": {"spam1": {"spam2": "ham2", "eggs": False}}}, - "spam.spam1.eggs", - False, - ), - ], - ) - def test_get_property(self, prop_dict, prop_path, expected_value): - assert _get_property(prop_dict, prop_path) == expected_value - - @pytest.mark.parametrize( - "prop_dict, prop_path, expected_value, default_value", - [ - ({}, "spam", "ham", "ham"), - ({"spam": "ham"}, "eggs", "ham", "ham"), - ], - ) - def test_get_property_default( - self, prop_dict, prop_path, expected_value, default_value - ): - assert _get_property(prop_dict, prop_path, default_value) == expected_value - @pytest.fixture(scope="function") def example_config_dict(self) -> Dict[str, str]: return { From d83e46f9157b2f4818413bfeafcc6ca2e303be48 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Mon, 20 Feb 2023 15:34:40 +0000 Subject: [PATCH 11/21] use enums in pydantic model for clarity --- .../api/ops/schemas/application_config.py | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/fides/api/ops/schemas/application_config.py b/src/fides/api/ops/schemas/application_config.py index b6e5c7fe1f..244d172466 100644 --- a/src/fides/api/ops/schemas/application_config.py +++ b/src/fides/api/ops/schemas/application_config.py @@ -1,34 +1,28 @@ from __future__ import annotations +from enum import Enum from typing import Dict, Optional from pydantic import Extra, root_validator, validator -from fides.api.ops.schemas.storage.storage import StorageType +from fides.api.ops.schemas.messaging.messaging import MessagingServiceType from fides.lib.schemas.base_class import BaseSchema +class StorageTypeApiAccepted(Enum): + """Enum for storage destination types accepted in API updates""" + + s3 = "s3" + local = "local" # local should be used for testing only, not for processing real-world privacy requests + + class StorageApplicationConfig(BaseSchema): - active_default_storage_type: StorageType + active_default_storage_type: StorageTypeApiAccepted class Config: use_enum_values = True extra = Extra.forbid - @validator("active_default_storage_type") - @classmethod - def validate_storage_type(cls, storage_type: StorageType) -> StorageType: - """ - For now, only `local` and `s3` storage types are supported - as an `active_default_storage_type` - """ - valid_storage_types = (StorageType.local.value, StorageType.s3.value) - if storage_type not in valid_storage_types: - raise ValueError( - f"Only '{valid_storage_types}' are supported as `active_default_storage_type`s" - ) - return storage_type - # TODO: the below models classes are "duplicates" of the pydantic # models that drive the application config. this is to allow every field @@ -52,12 +46,12 @@ class NotificationApplicationConfig(BaseSchema): def validate_notification_service_type(cls, value: Optional[str]) -> Optional[str]: """Ensure the provided type is a valid value.""" if value: - valid_values = ("MAILGUN", "TWILIO_TEXT", "TWILIO_EMAIL") value = value.upper() # force uppercase for safety - - if value not in valid_values: + try: + MessagingServiceType[value] + except ValueError: raise ValueError( - f"Invalid NOTIFICATION_SERVICE_TYPE provided '{value}', must be one of: {', '.join([level for level in valid_values])}" + f"Invalid notification.notification_service_type provided '{value}', must be one of: {', '.join([service_type.name for service_type in MessagingServiceType])}" ) return value From c0815a72ef9a9e99d217d8f545ff431ced15c4a8 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Mon, 20 Feb 2023 15:34:53 +0000 Subject: [PATCH 12/21] fix comment typo --- src/fides/core/config/config_proxy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fides/core/config/config_proxy.py b/src/fides/core/config/config_proxy.py index a58f503235..27d5db78d0 100644 --- a/src/fides/core/config/config_proxy.py +++ b/src/fides/core/config/config_proxy.py @@ -10,7 +10,7 @@ class ConfigProxyBase: """ - Base class that's used to make config proxy classes that corresopnd + Base class that's used to make config proxy classes that correspond to our config/settings sub-sections. Config proxy classes are a construct to allow for accessing "resolved" From a1fbe7aac2436f41dd9d2a1166682b2abfc91f23 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Mon, 20 Feb 2023 18:35:45 -0500 Subject: [PATCH 13/21] update down revision on migration --- .../c9ee230fa6da_add_config_set_column_to_application_.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py b/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py index 6c33ffc42f..05bb2aead4 100644 --- a/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py +++ b/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py @@ -1,7 +1,7 @@ """add config_set column to application settings Revision ID: c9ee230fa6da -Revises: 5d62bab40b71 +Revises: 8e198eb13802 Create Date: 2023-02-01 15:13:52.133075 """ @@ -11,7 +11,7 @@ # revision identifiers, used by Alembic. revision = "c9ee230fa6da" -down_revision = "5d62bab40b71" +down_revision = "8e198eb13802" branch_labels = None depends_on = None From dbd3321b2dc5ae07037369d9e4e4040215908dd7 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Tue, 21 Feb 2023 08:24:42 -0500 Subject: [PATCH 14/21] fix up notification service type validation --- .../api/ops/schemas/application_config.py | 17 ++++++++-------- .../api/v1/endpoints/test_config_endpoints.py | 20 +++++++++++++++++++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/src/fides/api/ops/schemas/application_config.py b/src/fides/api/ops/schemas/application_config.py index 244d172466..4c5159f293 100644 --- a/src/fides/api/ops/schemas/application_config.py +++ b/src/fides/api/ops/schemas/application_config.py @@ -43,16 +43,15 @@ class NotificationApplicationConfig(BaseSchema): @validator("notification_service_type", pre=True) @classmethod - def validate_notification_service_type(cls, value: Optional[str]) -> Optional[str]: + def validate_notification_service_type(cls, value: str) -> Optional[str]: """Ensure the provided type is a valid value.""" - if value: - value = value.upper() # force uppercase for safety - try: - MessagingServiceType[value] - except ValueError: - raise ValueError( - f"Invalid notification.notification_service_type provided '{value}', must be one of: {', '.join([service_type.name for service_type in MessagingServiceType])}" - ) + value = value.upper() # force uppercase for safety + try: + MessagingServiceType[value] + except KeyError: + raise ValueError( + f"Invalid notification.notification_service_type provided '{value}', must be one of: {', '.join([service_type.name for service_type in MessagingServiceType])}" + ) return value diff --git a/tests/ops/api/v1/endpoints/test_config_endpoints.py b/tests/ops/api/v1/endpoints/test_config_endpoints.py index 16acbae17d..0a04a2cc3c 100644 --- a/tests/ops/api/v1/endpoints/test_config_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_config_endpoints.py @@ -246,6 +246,26 @@ def test_patch_application_config_notifications_properties( assert db_settings.api_set["notifications"] == payload["notifications"] assert "execution" not in db_settings.api_set + def test_patch_application_config_invalid_notification_type( + self, + api_client: TestClient, + generate_auth_header, + url, + payload, + db: Session, + ): + + payload = { + "notifications": {"notification_service_type": "invalid_service_type"} + } + auth_header = generate_auth_header([scopes.CONFIG_UPDATE]) + response = api_client.patch( + url, + headers=auth_header, + json=payload, + ) + assert response.status_code == 422 + class TestGetApplicationConfigApiSet: @pytest.fixture(scope="function") From 95066db9cedfee06f748750f24ded6deaa7ff8c8 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Tue, 21 Feb 2023 08:37:11 -0500 Subject: [PATCH 15/21] improve test coverage --- tests/ops/models/test_application_config.py | 30 ++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/ops/models/test_application_config.py b/tests/ops/models/test_application_config.py index a68b7e012d..a64b63e1a5 100644 --- a/tests/ops/models/test_application_config.py +++ b/tests/ops/models/test_application_config.py @@ -199,12 +199,20 @@ def test_create_or_update_merges_nested_properties( assert config_record.api_set["setting3"] == "value3" assert config_record_db.id == config_record.id - def test_get_api_config_nothing_set( + def test_get_api_set_nothing_set( self, db: Session, ): + db.query(ApplicationConfig).delete() assert ApplicationConfig.get_api_set(db) == {} + def test_get_config_set_nothing_set( + self, + db: Session, + ): + db.query(ApplicationConfig).delete() + assert ApplicationConfig.get_config_set(db) == {} + def test_get_api_config(self, db: Session, example_config_record: Dict[str, Any]): ApplicationConfig.create_or_update(db, data=example_config_record) assert ApplicationConfig.get_api_set(db) == example_config_record["api_set"] @@ -242,6 +250,19 @@ def test_update_config_set(self, db): CONFIG.notifications.notification_service_type = notification_service_type CONFIG.execution.masking_strict = execution_strict + def test_update_config_nondict_errors( + self, db: Session, example_config_record: Dict[str, Any] + ): + """ + Test coverage for error path if non-dict is passed into `update` method + as either the `config_set` key or the `api_set` key + """ + app_config = ApplicationConfig.create_or_update(db, data=example_config_record) + with pytest.raises(ValueError): + app_config.update(db, {"config_set": "invalid"}) + with pytest.raises(ValueError): + app_config.update(db, {"api_set": "invalid"}) + class TestApplicationConfigResolution: @pytest.fixture(scope="function") @@ -299,6 +320,13 @@ def test_get_resolved_config_property(self, db, insert_example_config_record): == CONFIG.notifications.send_request_completion_notification ) + def test_get_resolved_config_property_no_config_record(self, db: Session): + db.query(ApplicationConfig).delete() + notification_service_type = ApplicationConfig.get_resolved_config_property( + db, "notifications.notification_service_type" + ) + assert notification_service_type is None + class TestConfigProxy: @pytest.fixture(scope="function") From fec1aaab058892323ff5b1a7182f2a8063826d6d Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Tue, 21 Feb 2023 08:37:29 -0500 Subject: [PATCH 16/21] improve error handling in bootstrap --- src/fides/api/main.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/fides/api/main.py b/src/fides/api/main.py index 4b44d83e32..124ba89df6 100644 --- a/src/fides/api/main.py +++ b/src/fides/api/main.py @@ -251,7 +251,9 @@ async def setup_server() -> None: ApplicationConfig.update_config_set(db, CONFIG) except Exception as e: logger.error("Error occurred writing config settings to database: {}", str(e)) - return + raise FidesError( + f"Error occurred writing config settings to database: {str(e)}" + ) logger.info("Validating SaaS connector templates...") try: From adb29e30ec356aa6478886994fd01d34f4db797f Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Tue, 21 Feb 2023 08:44:05 -0500 Subject: [PATCH 17/21] fix up changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f077df7f9..2760f4fa7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ The types of changes are: * Fides API * Access and erasure support for Braintree [#2223](https://github.com/ethyca/fides/pull/2223) * Added route to send a test message [#2585](https://github.com/ethyca/fides/pull/2585) + * Add default storage configuration functionality and associated APIs [#2438](https://github.com/ethyca/fides/pull/2438) * Admin UI * Custom Metadata [#2536](https://github.com/ethyca/fides/pull/2536) From e74454299f21de16172a76b562bc1b19530f501f Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Tue, 21 Feb 2023 16:22:57 -0500 Subject: [PATCH 18/21] make sure to close session used in main --- src/fides/api/main.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/fides/api/main.py b/src/fides/api/main.py index 124ba89df6..e07aa2c9b1 100644 --- a/src/fides/api/main.py +++ b/src/fides/api/main.py @@ -254,6 +254,8 @@ async def setup_server() -> None: raise FidesError( f"Error occurred writing config settings to database: {str(e)}" ) + finally: + db.close() logger.info("Validating SaaS connector templates...") try: From e890bf28dbc7bc10cdb4cd1708a943edc6c57155 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Tue, 21 Feb 2023 18:37:25 -0500 Subject: [PATCH 19/21] add missing column update to migration --- ...c9ee230fa6da_add_config_set_column_to_application_.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py b/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py index 05bb2aead4..1ae1e464df 100644 --- a/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py +++ b/src/fides/api/ctl/migrations/versions/c9ee230fa6da_add_config_set_column_to_application_.py @@ -17,7 +17,6 @@ def upgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.add_column( "applicationconfig", sa.Column( @@ -27,10 +26,12 @@ def upgrade(): ), ) - # ### end Alembic commands ### + # include this update here to make up for an earlier miss when creating the table + op.alter_column("applicationconfig", "api_set", nullable=False) def downgrade(): - # ### commands auto generated by Alembic - please adjust! ### op.drop_column("applicationconfig", "config_set") - # ### end Alembic commands ### + + # add a downgrade here for consistency + op.alter_column("applicationconfig", "api_set", nullable=True) From b053049e474d08de4dfc2e72a738d6d7fabf0135 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Tue, 21 Feb 2023 19:05:39 -0500 Subject: [PATCH 20/21] add endpoint to reset api app settings --- .../ops/api/v1/endpoints/config_endpoints.py | 23 ++- .../api/ops/models/application_config.py | 14 ++ .../api/v1/endpoints/test_config_endpoints.py | 135 ++++++++++++++++++ 3 files changed, 171 insertions(+), 1 deletion(-) diff --git a/src/fides/api/ops/api/v1/endpoints/config_endpoints.py b/src/fides/api/ops/api/v1/endpoints/config_endpoints.py index ea1a4512f1..d95cd3693a 100644 --- a/src/fides/api/ops/api/v1/endpoints/config_endpoints.py +++ b/src/fides/api/ops/api/v1/endpoints/config_endpoints.py @@ -1,4 +1,4 @@ -from typing import Any, Dict +from typing import Any, Dict, Optional from fastapi import Depends from fastapi.params import Security @@ -61,3 +61,24 @@ def update_settings( db, data.dict(exclude_none=True) ) return update_config.api_set + + +@router.delete( + urls.CONFIG, + status_code=HTTP_200_OK, + dependencies=[Security(verify_oauth_client, scopes=[scopes.CONFIG_UPDATE])], + response_model=Dict, +) +def reset_settings( + *, + db: Session = Depends(deps.get_db), +) -> dict: + """ + Resets the global application settings record. + + Only the "api-set" values are cleared, "config-set" values are + not updated via any API calls + """ + logger.info("Resetting api-set application settings") + update_config: Optional[ApplicationConfig] = ApplicationConfig.clear_api_set(db) + return update_config.api_set if update_config else {} diff --git a/src/fides/api/ops/models/application_config.py b/src/fides/api/ops/models/application_config.py index 4c318eca20..79f8223671 100644 --- a/src/fides/api/ops/models/application_config.py +++ b/src/fides/api/ops/models/application_config.py @@ -138,6 +138,20 @@ def update_api_set( """ return cls.create_or_update(db, data={"api_set": api_set_dict}) + @classmethod + def clear_api_set(cls, db: Session) -> Optional[ApplicationConfig]: + """ + Utility method to set the `api_set` column on the `applicationconfig` + db record to an empty dict + + """ + existing_record = db.query(cls).first() + if existing_record: + existing_record.api_set = {} + existing_record.save(db) + return existing_record + return None + @classmethod def update_config_set(cls, db: Session, config: FidesConfig) -> ApplicationConfig: """ diff --git a/tests/ops/api/v1/endpoints/test_config_endpoints.py b/tests/ops/api/v1/endpoints/test_config_endpoints.py index 0a04a2cc3c..5d39cd89f5 100644 --- a/tests/ops/api/v1/endpoints/test_config_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_config_endpoints.py @@ -325,15 +325,150 @@ def test_get_application_config( url, headers=auth_header, params={"api_set": True}, +class TestDeleteApplicationConfig: + @pytest.fixture(scope="function") + def url(self) -> str: + return urls.V1_URL_PREFIX + urls.CONFIG + + @pytest.fixture(scope="function") + def payload(self): + return { + "storage": {"active_default_storage_type": StorageType.s3.value}, + "notifications": { + "notification_service_type": "TWILIO_TEXT", + "send_request_completion_notification": True, + "send_request_receipt_notification": True, + "send_request_review_notification": True, + }, + "execution": {"subject_identity_verification_required": True}, + } + + def test_reset_application_config( + self, + api_client: TestClient, + generate_auth_header, + url, + db: Session, + payload, + ): + # first we PATCH in some settings + auth_header = generate_auth_header([scopes.CONFIG_UPDATE]) + response = api_client.patch( + url, + headers=auth_header, json=payload, ) assert response.status_code == 200 + + # then we test that we can GET them + auth_header = generate_auth_header([scopes.CONFIG_READ]) + response = api_client.get( + url, + headers=auth_header, + params={"api_set": True}, + ) + assert response.status_code == 200 response_settings = response.json() assert ( response_settings["storage"]["active_default_storage_type"] == payload["storage"]["active_default_storage_type"] ) + # then we delete them + auth_header = generate_auth_header([scopes.CONFIG_UPDATE]) + response = api_client.delete( + url, + headers=auth_header, + ) + assert response.status_code == 200 + + # then we ensure they are no longer returned + auth_header = generate_auth_header([scopes.CONFIG_READ]) + response = api_client.get( + url, + headers=auth_header, + params={"api_set": True}, + ) + assert response.status_code == 200 + response_settings = response.json() + assert response_settings == {} + + # and cleared from the db + db_settings = db.query(ApplicationConfig).first() + # this should be empty + assert db_settings.api_set == {} + + def test_reset_application_config_non_existing( + self, + api_client: TestClient, + generate_auth_header, + url, + db: Session, + ): + """ + Test that a DELETE works even if no 'api-set' settings have been set yet + """ + # we ensure they are not returned + auth_header = generate_auth_header([scopes.CONFIG_READ]) + response = api_client.get( + url, + headers=auth_header, + params={"api_set": True}, + ) + assert response.status_code == 200 + response_settings = response.json() + assert response_settings == {} + + # and nothing in the db + db_settings = db.query(ApplicationConfig).first() + # this should be empty + assert db_settings.api_set == {} + + # actually run the delete + auth_header = generate_auth_header([scopes.CONFIG_UPDATE]) + response = api_client.delete( + url, + headers=auth_header, + ) + assert response.status_code == 200 + + # we ensure they are still not returned + auth_header = generate_auth_header([scopes.CONFIG_READ]) + response = api_client.get( + url, + headers=auth_header, + params={"api_set": True}, + ) + assert response.status_code == 200 + response_settings = response.json() + assert response_settings == {} + + # and still nothing in the db + db_settings = db.query(ApplicationConfig).first() + # this should be empty + assert db_settings.api_set == {} + + # now actually delete the application config record + db_settings.delete(db) + # and ensure the delete call doesn't error + auth_header = generate_auth_header([scopes.CONFIG_UPDATE]) + response = api_client.delete( + url, + headers=auth_header, + ) + assert response.status_code == 200 + + # and ensure the GET works but still returns nothing + auth_header = generate_auth_header([scopes.CONFIG_READ]) + response = api_client.get( + url, + headers=auth_header, + params={"api_set": True}, + ) + assert response.status_code == 200 + response_settings = response.json() + assert response_settings == {} + class TestGetConnections: @pytest.fixture(scope="function") From 48f0394497cb0a6ad9717a5a128080aff775db0c Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Tue, 21 Feb 2023 19:05:58 -0500 Subject: [PATCH 21/21] fix bug with censor_config --- src/fides/core/config/__init__.py | 3 +- .../api/v1/endpoints/test_config_endpoints.py | 43 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/fides/core/config/__init__.py b/src/fides/core/config/__init__.py index 0fb93f59bf..1ee61f2928 100644 --- a/src/fides/core/config/__init__.py +++ b/src/fides/core/config/__init__.py @@ -108,7 +108,8 @@ def censor_config(config: Union[FidesConfig, Dict[str, Any]]) -> Dict[str, Any]: data = as_dict[key] filtered[key] = {} for field in value: - filtered[key][field] = data[field] + if field in data: + filtered[key][field] = data[field] return filtered diff --git a/tests/ops/api/v1/endpoints/test_config_endpoints.py b/tests/ops/api/v1/endpoints/test_config_endpoints.py index 5d39cd89f5..69f1ba0086 100644 --- a/tests/ops/api/v1/endpoints/test_config_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_config_endpoints.py @@ -276,6 +276,10 @@ def url(self) -> str: def payload(self): return {"storage": {"active_default_storage_type": StorageType.s3.value}} + @pytest.fixture(scope="function") + def payload_single_notification_property(self): + return {"notifications": {"notification_service_type": "twilio_email"}} + def test_get_application_config_unauthenticated(self, api_client: TestClient, url): response = api_client.get(url, headers={}) assert 401 == response.status_code @@ -309,6 +313,7 @@ def test_get_application_config( url, db: Session, payload, + payload_single_notification_property, ): # first we PATCH in some settings auth_header = generate_auth_header([scopes.CONFIG_UPDATE]) @@ -325,6 +330,44 @@ def test_get_application_config( url, headers=auth_header, params={"api_set": True}, + ) + assert response.status_code == 200 + response_settings = response.json() + assert ( + response_settings["storage"]["active_default_storage_type"] + == payload["storage"]["active_default_storage_type"] + ) + + # now PATCH in a single notification property + auth_header = generate_auth_header([scopes.CONFIG_UPDATE]) + response = api_client.patch( + url, + headers=auth_header, + json=payload_single_notification_property, + ) + assert response.status_code == 200 + + # then we test that we can GET it + auth_header = generate_auth_header([scopes.CONFIG_READ]) + response = api_client.get( + url, + headers=auth_header, + params={"api_set": True}, + ) + assert response.status_code == 200 + response_settings = response.json() + assert ( + response_settings["storage"]["active_default_storage_type"] + == payload["storage"]["active_default_storage_type"] + ) + assert ( + response_settings["notifications"]["notification_service_type"] + == payload_single_notification_property["notifications"][ + "notification_service_type" + ].upper() + ) + + class TestDeleteApplicationConfig: @pytest.fixture(scope="function") def url(self) -> str: