From 3a97760acdc70361f9609383d626aea80bdff682 Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Mon, 2 May 2022 19:35:17 -0500 Subject: [PATCH 01/12] adds drp action type to db, adds docs, tests --- docs/fidesops/docs/guides/policies.md | 11 +- .../api/v1/endpoints/policy_endpoints.py | 4 +- src/fidesops/common_exceptions.py | 4 + src/fidesops/models/policy.py | 120 ++++++++++-- src/fidesops/schemas/policy.py | 3 + ...6819d7_adds_drp_policy_column_to_policy.py | 40 ++++ .../api/v1/endpoints/test_policy_endpoints.py | 175 +++++++++++++++++- .../test_privacy_request_endpoints.py | 3 + tests/fixtures/application_fixtures.py | 50 +++++ tests/models/test_policy.py | 21 +++ 10 files changed, 415 insertions(+), 16 deletions(-) create mode 100644 src/migrations/versions/0e0b346819d7_adds_drp_policy_column_to_policy.py diff --git a/docs/fidesops/docs/guides/policies.md b/docs/fidesops/docs/guides/policies.md index 99db8f3b2..708e097b4 100644 --- a/docs/fidesops/docs/guides/policies.md +++ b/docs/fidesops/docs/guides/policies.md @@ -42,12 +42,21 @@ PATCH /api/v1/policy [ { "name": "User Email Address", - "key": "user_email_address_polcy" + "key": "user_email_address_polcy", + "drp_action": "access" // optional } ] ``` This policy is subtly different from the concept of a Policy in [Fidesctl](https://github.com/ethyca/fides). A [Fidesctl policy](https://ethyca.github.io/fides/language/resources/policy/) dictates which data categories can be stored where. A Fidesops policy, on the other hand, dictates how to access, mask or erase data that matches specific data categories for privacy requests. +### Policy Attributes +- `Policy.name`: User-friendly name for your Policy. +- `Policy.key`: Unique key by which to reference the Policy. +- `Policy.drp_action` (optional): Which DRP action is this Policy handling? DRP action is only needed if you intend on using Fidesops as provider for the Data Rights Protocol (DRP). Read more about DRP [here](https://github.com/consumer-reports-digital-lab/data-rights-protocol). + - `access`: A data subject access request. Must be used with an `access` Rule. + - `deletion`: A data subject erasure request. Must be used with an `erasure` Rule. + + ## Add an Access Rule to your Policy The policy creation operation returns a Policy key, which we'll use to add a Rule: diff --git a/src/fidesops/api/v1/endpoints/policy_endpoints.py b/src/fidesops/api/v1/endpoints/policy_endpoints.py index 6bbd15ec7..5a6f9e088 100644 --- a/src/fidesops/api/v1/endpoints/policy_endpoints.py +++ b/src/fidesops/api/v1/endpoints/policy_endpoints.py @@ -20,6 +20,7 @@ PolicyValidationError, RuleTargetValidationError, RuleValidationError, + DrpActionValidationError, ) from fidesops.models.client import ClientDetail from fidesops.models.policy import ActionType, Policy, Rule, RuleTarget @@ -114,9 +115,10 @@ def create_or_update_policies( "name": policy_data["name"], "key": policy_data.get("key"), "client_id": client.id, + "drp_action": policy_data.get("drp_action"), }, ) - except KeyOrNameAlreadyExists as exc: + except (KeyOrNameAlreadyExists, DrpActionValidationError) as exc: logger.warning("Create/update failed for policy: %s", exc) failure = { "message": exc.args[0], diff --git a/src/fidesops/common_exceptions.py b/src/fidesops/common_exceptions.py index 5f4547766..1659a5326 100644 --- a/src/fidesops/common_exceptions.py +++ b/src/fidesops/common_exceptions.py @@ -83,6 +83,10 @@ class KeyOrNameAlreadyExists(Exception): """A resource already exists with this key or name.""" +class DrpActionValidationError(Exception): + """A resource already exists with this DRP Action.""" + + class KeyValidationError(Exception): """The resource you're trying to create has a key specified but not a name specified.""" diff --git a/src/fidesops/models/policy.py b/src/fidesops/models/policy.py index bdd7c9f01..78912a473 100644 --- a/src/fidesops/models/policy.py +++ b/src/fidesops/models/policy.py @@ -60,6 +60,21 @@ class ActionType(EnumType): update = "update" +class DrpAction(EnumType): + """ + Enum to hold valid DRP actions. For more details, see: + https://github.com/consumer-reports-digital-lab/data-rights-protocol#301-supported-rights-actions + """ + + access = "access" + deletion = "deletion" + # below are not supported + sale_opt_out = "sale:opt_out" + sale_opt_in = "sale:opt_in" + access_categories = "access:categories" + access_specific = "access:specific" + + PseudonymizationPolicy = SupportedMaskingStrategies """ *Deprecated*: The method by which to pseudonymize data. @@ -70,33 +85,67 @@ class is referenced in multiple database migrations. This class is to be removed """ +def _validate_drp_action( + drp_action: Optional[str], drp_action_exists: bool, class_name: str +) -> None: + """Check that DRP action is supported""" + if not drp_action: + return + if drp_action_exists: + raise common_exceptions.DrpActionValidationError( + f"DRP Action {drp_action} already exists in {class_name}." + ) + if drp_action in [ + DrpAction.sale_opt_in.value, + DrpAction.sale_opt_out.value, + DrpAction.access_categories.value, + DrpAction.access_specific.value, + ]: + raise common_exceptions.DrpActionValidationError( + f"{drp_action} action is not supported at this time." + ) + + def _validate_rule( action_type: Optional[str], storage_destination_id: Optional[str], masking_strategy: Optional[Dict[str, Union[str, Dict[str, str]]]], + drp_action: Optional[DrpAction], ) -> None: """Check that the rule's action_type and storage_destination are valid.""" if not action_type: raise common_exceptions.RuleValidationError("action_type is required.") - - if action_type == ActionType.erasure.value and storage_destination_id is not None: + if drp_action: + _validate_rule_with_drp_action(action_type, drp_action) + if action_type == ActionType.erasure.value: + if storage_destination_id is not None: + raise common_exceptions.RuleValidationError( + "Erasure Rules cannot have storage destinations." + ) + if masking_strategy is None: + raise common_exceptions.RuleValidationError( + "Erasure Rules must have masking strategies." + ) + if action_type == ActionType.access.value: + if storage_destination_id is None: + raise common_exceptions.RuleValidationError( + "Access Rules must have a storage destination." + ) + if action_type in [ActionType.consent.value, ActionType.update.value]: raise common_exceptions.RuleValidationError( - "Erasure Rules cannot have storage destinations." + f"{action_type} Rules are not supported at this time." ) - if action_type == ActionType.erasure.value and masking_strategy is None: - raise common_exceptions.RuleValidationError( - "Erasure Rules must have masking strategies." - ) - if action_type == ActionType.access.value and storage_destination_id is None: +def _validate_rule_with_drp_action(rule_action: str, drp_action: DrpAction) -> None: + """Validate that rule action matches drp action""" + if drp_action == DrpAction.deletion and rule_action != ActionType.erasure.value: raise common_exceptions.RuleValidationError( - "Access Rules must have a storage destination." + "Since the associated Policy has a DRP deletion action, this rule must have the matching access action_type." ) - - if action_type in [ActionType.consent.value, ActionType.update.value]: + if drp_action == DrpAction.access and rule_action != ActionType.access.value: raise common_exceptions.RuleValidationError( - f"{action_type} Rules are not supported at this time." + "Since the associated Policy has a DRP access action, this rule must have the matching erasure action_type." ) @@ -105,6 +154,7 @@ class Policy(Base): name = Column(String, unique=True, nullable=False) key = Column(String, index=True, unique=True, nullable=False) + drp_action = Column(EnumColumn(DrpAction), index=True, unique=True, nullable=True) client_id = Column( String, ForeignKey(ClientDetail.id_field_path), @@ -115,6 +165,42 @@ class Policy(Base): backref="policies", ) # Which client created the Policy + @classmethod + def create_or_update(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: + """Overrides base create or update to add custom error for drp action already exists""" + db_obj = None + if data.get("id") is not None: + # If `id` has been included in `data`, preference that + db_obj = cls.get(db=db, id=data["id"]) + elif data.get("key") is not None: + # Otherwise, try with `key` + db_obj = cls.get_by(db=db, field="key", value=data["key"]) + + if db_obj: + other_objs = db.query(cls).filter(id != db_obj.id) # pylint: disable=W0143 + _validate_drp_action( + data["drp_action"], + bool( + other_objs + and other_objs.filter_by(drp_action=data["drp_action"]).first() + ), + cls.name, + ) + db_obj.update(db=db, data=data) + else: + if hasattr(cls, "drp_action"): + data["drp_action"] = data.get("drp_action", None) + _validate_drp_action( + data["drp_action"], + bool( + db.query(cls).filter_by(drp_action=data["drp_action"]).first() + ), + cls.name, + ) + db_obj = cls.create(db=db, data=data) + + return db_obj + def delete(self, db: Session) -> Optional[FidesopsBase]: """Cascade delete all rules on deletion of a Policy.""" _ = [rule.delete(db=db) for rule in self.rules] @@ -246,16 +332,19 @@ def save(self, db: Session) -> FidesopsBase: action_type=self.action_type, storage_destination_id=self.storage_destination_id, masking_strategy=self.masking_strategy, + drp_action=self.policy.drp_action, ) return super().save(db=db) @classmethod def create(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: """Validate this object's data before deferring to the superclass on update""" + associated_policy = db.query(Policy).filter_by(id=data["policy_id"]).first() _validate_rule( action_type=data.get("action_type"), storage_destination_id=data.get("storage_destination_id"), masking_strategy=data.get("masking_strategy"), + drp_action=associated_policy.drp_action if associated_policy else None, ) return super().create(db=db, data=data) @@ -293,6 +382,11 @@ def create_or_update(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: raise common_exceptions.RuleValidationError( f"Rule with identifier {identifier} belongs to another policy." ) + associated_policy = db.query(Policy).filter_by(id=data["policy_id"]).first() + if associated_policy and associated_policy.drp_action: + _validate_rule_with_drp_action( + data.get("action_type"), associated_policy.drp_action + ) db_obj.update(db=db, data=data) else: db_obj = cls.create(db=db, data=data) @@ -419,7 +513,7 @@ def update(self, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: """Validate data_category on object update.""" updated_data_category = data.get("data_category") if data.get("name") is None: - # Don't pass explciit `None` through for `name` because the field + # Don't pass explcit `None` through for `name` because the field # is non-nullable del data["name"] diff --git a/src/fidesops/schemas/policy.py b/src/fidesops/schemas/policy.py index ea1f8a95c..3df31eef0 100644 --- a/src/fidesops/schemas/policy.py +++ b/src/fidesops/schemas/policy.py @@ -3,6 +3,7 @@ from fidesops.models.policy import ( ActionType, + DrpAction, ) from fidesops.schemas.api import BulkResponse, BulkUpdateFailed from fidesops.schemas.base_class import BaseSchema @@ -86,12 +87,14 @@ class Policy(BaseSchema): name: str key: Optional[FidesOpsKey] + drp_action: Optional[DrpAction] class PolicyResponse(Policy): """A holistic view of a Policy record, including all foreign keys by default.""" rules: Optional[List[RuleResponse]] + drp_action: Optional[DrpAction] class BulkPutRuleTargetResponse(BulkResponse): diff --git a/src/migrations/versions/0e0b346819d7_adds_drp_policy_column_to_policy.py b/src/migrations/versions/0e0b346819d7_adds_drp_policy_column_to_policy.py new file mode 100644 index 000000000..292f71d91 --- /dev/null +++ b/src/migrations/versions/0e0b346819d7_adds_drp_policy_column_to_policy.py @@ -0,0 +1,40 @@ +"""adds drp_policy column to policy + +Revision ID: 0e0b346819d7 +Revises: 530fb8533ca4 +Create Date: 2022-04-28 20:36:01.314299 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. + + +revision = "0e0b346819d7" +down_revision = "530fb8533ca4" +branch_labels = None +depends_on = None + + +def upgrade(): + drpaction = postgresql.ENUM( + "access", + "deletion", + "sale_opt_out", + "sale_opt_in", + "access_categories", + "access_specific", + name="drpaction", + create_type=False, + ) + drpaction.create(op.get_bind()) + op.add_column("policy", sa.Column("drp_action", drpaction, nullable=True)) + op.create_index(op.f("ix_policy_drp_action"), "policy", ["drp_action"], unique=True) + + +def downgrade(): + op.drop_index(op.f("ix_policy_drp_action"), table_name="policy") + op.drop_column("policy", "drp_action") + op.execute("DROP TYPE drpaction;") diff --git a/tests/api/v1/endpoints/test_policy_endpoints.py b/tests/api/v1/endpoints/test_policy_endpoints.py index d26c58e05..b6bc48ba1 100644 --- a/tests/api/v1/endpoints/test_policy_endpoints.py +++ b/tests/api/v1/endpoints/test_policy_endpoints.py @@ -19,7 +19,7 @@ ActionType, Policy, Rule, - RuleTarget, + RuleTarget, DrpAction, ) from fidesops.service.masking.strategy.masking_strategy_nullify import NULL_REWRITE from fidesops.util.data_category import DataCategory, generate_fides_data_categories @@ -142,6 +142,27 @@ def test_get_invalid_policy( ) assert resp.status_code == 404 + def test_get_policy_returns_drp_action( + self, api_client: TestClient, generate_auth_header, policy_drp_action, url + ): + auth_header = generate_auth_header(scopes=[scopes.POLICY_READ]) + resp = api_client.get( + V1_URL_PREFIX + POLICY_DETAIL_URI.format(policy_key=policy_drp_action.key), + headers=auth_header, + ) + assert resp.status_code == 200 + data = resp.json() + print(json.dumps(resp.json(), indent=2)) + assert data["key"] == policy_drp_action.key + assert data["drp_action"] == DrpAction.access.value + assert "rules" in data + assert len(data["rules"]) == 1 + + rule = data["rules"][0] + assert rule["key"] == "access_request_rule_drp" + assert rule["action_type"] == "access" + assert rule["storage_destination"]["type"] == "s3" + def test_get_policy_returns_rules( self, api_client: TestClient, generate_auth_header, policy, url ): @@ -303,6 +324,136 @@ def test_create_policy_with_duplicate_key( data = resp.json() assert len(data["failed"]) == 2 + def test_create_policy_with_duplicate_drp_action( + self, + url, + api_client: TestClient, + generate_auth_header, + policy_drp_action, + storage_config, + ): + data = [ + { + "name": "policy with pre-existing drp action", + "action_type": ActionType.access.value, + "drp_action": DrpAction.access.value + } + ] + auth_header = generate_auth_header(scopes=[scopes.POLICY_CREATE_OR_UPDATE]) + resp = api_client.patch(url, json=data, headers=auth_header) + assert resp.status_code == 200 + + data = resp.json() + assert len(data["failed"]) == 1 + + def test_update_policy_with_duplicate_drp_action( + self, + db, + url, + api_client: TestClient, + generate_auth_header, + policy_drp_action, + storage_config, + ): + # creates a new drp policy + data = [ + { + "key": "erasure_drp_policy", + "name": "erasure drp policy", + "action_type": ActionType.erasure.value, + "drp_action": DrpAction.deletion.value + } + ] + auth_header = generate_auth_header(scopes=[scopes.POLICY_CREATE_OR_UPDATE]) + valid_drp_resp = api_client.patch(url, json=data, headers=auth_header) + valid_response_data = valid_drp_resp.json()["succeeded"] + assert valid_drp_resp.status_code == 200 + + # try to update the above policy with a pre-existing drp action + data = [ + { + "key": "erasure_drp_policy", + "name": "policy with pre-existing drp action", + "action_type": ActionType.access.value, + "drp_action": DrpAction.access.value + } + ] + auth_header = generate_auth_header(scopes=[scopes.POLICY_CREATE_OR_UPDATE]) + resp = api_client.patch(url, json=data, headers=auth_header) + assert resp.status_code == 200 + + data = resp.json() + assert len(data["failed"]) == 1 + + pol = Policy.filter( + db=db, conditions=(Policy.key == valid_response_data[0]["key"]) + ).first() + pol.delete(db=db) + + def test_update_policy_with_drp_action( + self, + url, + api_client: TestClient, + generate_auth_header, + policy, + storage_config, + ): + data = [ + { + "key": policy.key, + "name": "updated name", + "action_type": ActionType.access.value, + "drp_action": DrpAction.access.value + } + ] + auth_header = generate_auth_header(scopes=[scopes.POLICY_CREATE_OR_UPDATE]) + resp = api_client.patch(url, json=data, headers=auth_header) + assert resp.status_code == 200 + response_data = resp.json()["succeeded"] + assert len(response_data) == 1 + + def test_create_policy_invalid_drp_action( + self, url, api_client: TestClient, payload, generate_auth_header, storage_config + ): + payload = [ + { + "name": "policy 1", + "action_type": "erasure", + "drp_action": "invalid", + "data_category": DataCategory("user.provided.identifiable").value, + "storage_destination_key": storage_config.key, + } + ] + auth_header = generate_auth_header(scopes=[scopes.POLICY_CREATE_OR_UPDATE]) + resp = api_client.patch(url, json=payload, headers=auth_header) + assert resp.status_code == 422 + + response_body = json.loads(resp.text) + assert "value is not a valid enumeration member; permitted: 'access', 'deletion', 'sale:opt_out', 'sale:opt_in', 'access:categories', 'access:specific'" == response_body["detail"][0]["msg"] + + def test_create_policy_with_drp_action( + self, db, url, api_client: TestClient, payload, generate_auth_header, storage_config + ): + payload = [ + { + "name": "policy 1", + "action_type": "erasure", + "drp_action": "deletion", + "data_category": DataCategory("user.provided.identifiable").value, + "storage_destination_key": storage_config.key, + } + ] + auth_header = generate_auth_header(scopes=[scopes.POLICY_CREATE_OR_UPDATE]) + resp = api_client.patch(url, json=payload, headers=auth_header) + assert resp.status_code == 200 + response_data = resp.json()["succeeded"] + assert len(response_data) == 1 + + pol = Policy.filter( + db=db, conditions=(Policy.key == response_data[0]["key"]) + ).first() + pol.delete(db=db) + def test_create_policy_creates_key( self, db, api_client: TestClient, generate_auth_header, storage_config, url ): @@ -436,6 +587,28 @@ def test_create_rules_invalid_policy( resp = api_client.patch(url, headers=auth_header, json=data) assert resp.status_code == 404 + def test_create_rules_mismatching_drp_policy( + self, api_client: TestClient, generate_auth_header, policy_drp_action, storage_config + ): + data = [ + { + "name": "test access rule", + "action_type": ActionType.erasure.value, + "storage_destination_key": storage_config.key, + } + ] + url = V1_URL_PREFIX + RULE_CREATE_URI.format(policy_key=policy_drp_action.key) + auth_header = generate_auth_header(scopes=[scopes.RULE_CREATE_OR_UPDATE]) + resp = api_client.patch( + url, + json=data, + headers=auth_header, + ) + + assert resp.status_code == 200 + response_data = resp.json()["failed"] + assert len(response_data) == 1 + def test_create_rules_limit_exceeded( self, api_client: TestClient, generate_auth_header, url, storage_config ): diff --git a/tests/api/v1/endpoints/test_privacy_request_endpoints.py b/tests/api/v1/endpoints/test_privacy_request_endpoints.py index e536991cc..71b2ff174 100644 --- a/tests/api/v1/endpoints/test_privacy_request_endpoints.py +++ b/tests/api/v1/endpoints/test_privacy_request_endpoints.py @@ -478,6 +478,7 @@ def test_get_privacy_requests_by_id( "reviewed_by": None, "reviewer": None, "policy": { + "drp_action": None, "name": privacy_request.policy.name, "key": privacy_request.policy.key, }, @@ -522,6 +523,7 @@ def test_get_privacy_requests_by_partial_id( "reviewed_by": None, "reviewer": None, "policy": { + "drp_action": None, "name": privacy_request.policy.name, "key": privacy_request.policy.key, }, @@ -790,6 +792,7 @@ def test_verbose_privacy_requests( "reviewed_by": None, "reviewer": None, "policy": { + "drp_action": None, "name": privacy_request.policy.name, "key": privacy_request.policy.key, }, diff --git a/tests/fixtures/application_fixtures.py b/tests/fixtures/application_fixtures.py index dd4054705..ca9e9a894 100644 --- a/tests/fixtures/application_fixtures.py +++ b/tests/fixtures/application_fixtures.py @@ -513,6 +513,56 @@ def policy( pass +@pytest.fixture(scope="function") +def policy_drp_action( + db: Session, + oauth_client: ClientDetail, + storage_config: StorageConfig, +) -> Generator: + access_request_policy = Policy.create( + db=db, + data={ + "name": "example access request policy drp", + "key": "example_access_request_policy_drp", + "drp_action": "access", + "client_id": oauth_client.id, + }, + ) + + access_request_rule = Rule.create( + db=db, + data={ + "action_type": ActionType.access.value, + "client_id": oauth_client.id, + "name": "Access Request Rule DRP", + "policy_id": access_request_policy.id, + "storage_destination_id": storage_config.id, + }, + ) + + rule_target = RuleTarget.create( + db=db, + data={ + "client_id": oauth_client.id, + "data_category": DataCategory("user.provided.identifiable").value, + "rule_id": access_request_rule.id, + }, + ) + yield access_request_policy + try: + rule_target.delete(db) + except ObjectDeletedError: + pass + try: + access_request_rule.delete(db) + except ObjectDeletedError: + pass + try: + access_request_policy.delete(db) + except ObjectDeletedError: + pass + + @pytest.fixture(scope="function") def erasure_policy_string_rewrite( db: Session, diff --git a/tests/models/test_policy.py b/tests/models/test_policy.py index 3ee07ed85..f017f3a56 100644 --- a/tests/models/test_policy.py +++ b/tests/models/test_policy.py @@ -145,6 +145,27 @@ def test_create_access_rule_with_no_storage_destination_is_invalid( assert exc.value.args[0] == "Access Rules must have a storage destination." +def test_create_erasure_rule_invalid( + db: Session, + policy_drp_action: Policy, +) -> None: + with pytest.raises(RuleValidationError) as exc: + rule = Rule.create( + db=db, + data={ + "action_type": ActionType.erasure.value, + "client_id": policy_drp_action.client_id, + "name": "Invalid Erasure Rule due to Policy being associated with the access DRP Action", + "policy_id": policy_drp_action.id, + "masking_strategy": { + "strategy": NULL_REWRITE, + "configuration": {}, + }, + }, + ) + assert exc.value.args[0] == "Since the associated Policy has a DRP access action, this rule must have the matching erasure action_type." + + def test_consent_action_is_unsupported( db: Session, policy: Policy, From dc21acf03dc4f13205c2d8f265f6f9969256507d Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Wed, 4 May 2022 15:17:52 -0500 Subject: [PATCH 02/12] fix lint, tests --- docs/fidesops/docs/guides/policies.md | 4 +-- src/fidesops/models/policy.py | 26 +++---------------- src/fidesops/schemas/policy.py | 6 +++++ ...5078badb90b9_adds_drp_action_to_policy.py} | 16 ++++++------ .../test_privacy_request_endpoints.py | 1 + tests/models/test_policy.py | 21 --------------- 6 files changed, 20 insertions(+), 54 deletions(-) rename src/migrations/versions/{0e0b346819d7_adds_drp_policy_column_to_policy.py => 5078badb90b9_adds_drp_action_to_policy.py} (81%) diff --git a/docs/fidesops/docs/guides/policies.md b/docs/fidesops/docs/guides/policies.md index 708e097b4..aabdd7ff1 100644 --- a/docs/fidesops/docs/guides/policies.md +++ b/docs/fidesops/docs/guides/policies.md @@ -53,8 +53,8 @@ This policy is subtly different from the concept of a Policy in [Fidesctl](https - `Policy.name`: User-friendly name for your Policy. - `Policy.key`: Unique key by which to reference the Policy. - `Policy.drp_action` (optional): Which DRP action is this Policy handling? DRP action is only needed if you intend on using Fidesops as provider for the Data Rights Protocol (DRP). Read more about DRP [here](https://github.com/consumer-reports-digital-lab/data-rights-protocol). - - `access`: A data subject access request. Must be used with an `access` Rule. - - `deletion`: A data subject erasure request. Must be used with an `erasure` Rule. + - `access`: A data subject access request. Should be used with an `access` Rule. + - `deletion`: A data subject erasure request. Should be used with an `erasure` Rule. ## Add an Access Rule to your Policy diff --git a/src/fidesops/models/policy.py b/src/fidesops/models/policy.py index 78912a473..cdb58aae6 100644 --- a/src/fidesops/models/policy.py +++ b/src/fidesops/models/policy.py @@ -110,13 +110,10 @@ def _validate_rule( action_type: Optional[str], storage_destination_id: Optional[str], masking_strategy: Optional[Dict[str, Union[str, Dict[str, str]]]], - drp_action: Optional[DrpAction], ) -> None: """Check that the rule's action_type and storage_destination are valid.""" if not action_type: raise common_exceptions.RuleValidationError("action_type is required.") - if drp_action: - _validate_rule_with_drp_action(action_type, drp_action) if action_type == ActionType.erasure.value: if storage_destination_id is not None: raise common_exceptions.RuleValidationError( @@ -137,18 +134,6 @@ def _validate_rule( ) -def _validate_rule_with_drp_action(rule_action: str, drp_action: DrpAction) -> None: - """Validate that rule action matches drp action""" - if drp_action == DrpAction.deletion and rule_action != ActionType.erasure.value: - raise common_exceptions.RuleValidationError( - "Since the associated Policy has a DRP deletion action, this rule must have the matching access action_type." - ) - if drp_action == DrpAction.access and rule_action != ActionType.access.value: - raise common_exceptions.RuleValidationError( - "Since the associated Policy has a DRP access action, this rule must have the matching erasure action_type." - ) - - class Policy(Base): """A set of constraints to apply to a privacy request""" @@ -184,7 +169,7 @@ def create_or_update(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: other_objs and other_objs.filter_by(drp_action=data["drp_action"]).first() ), - cls.name, + cls.__name__.lower(), ) db_obj.update(db=db, data=data) else: @@ -195,7 +180,7 @@ def create_or_update(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: bool( db.query(cls).filter_by(drp_action=data["drp_action"]).first() ), - cls.name, + cls.__name__.lower(), ) db_obj = cls.create(db=db, data=data) @@ -339,7 +324,7 @@ def save(self, db: Session) -> FidesopsBase: @classmethod def create(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: """Validate this object's data before deferring to the superclass on update""" - associated_policy = db.query(Policy).filter_by(id=data["policy_id"]).first() + associated_policy: Optional[Policy] = db.query(Policy).filter_by(id=data["policy_id"]).first() _validate_rule( action_type=data.get("action_type"), storage_destination_id=data.get("storage_destination_id"), @@ -382,11 +367,6 @@ def create_or_update(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: raise common_exceptions.RuleValidationError( f"Rule with identifier {identifier} belongs to another policy." ) - associated_policy = db.query(Policy).filter_by(id=data["policy_id"]).first() - if associated_policy and associated_policy.drp_action: - _validate_rule_with_drp_action( - data.get("action_type"), associated_policy.drp_action - ) db_obj.update(db=db, data=data) else: db_obj = cls.create(db=db, data=data) diff --git a/src/fidesops/schemas/policy.py b/src/fidesops/schemas/policy.py index 3df31eef0..8ee82a9b1 100644 --- a/src/fidesops/schemas/policy.py +++ b/src/fidesops/schemas/policy.py @@ -89,6 +89,12 @@ class Policy(BaseSchema): key: Optional[FidesOpsKey] drp_action: Optional[DrpAction] + class Config: + """Populate models with the raw value of enum fields, rather than the enum itself""" + + use_enum_values = True + orm_mode = True + class PolicyResponse(Policy): """A holistic view of a Policy record, including all foreign keys by default.""" diff --git a/src/migrations/versions/0e0b346819d7_adds_drp_policy_column_to_policy.py b/src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py similarity index 81% rename from src/migrations/versions/0e0b346819d7_adds_drp_policy_column_to_policy.py rename to src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py index 292f71d91..780ec79ed 100644 --- a/src/migrations/versions/0e0b346819d7_adds_drp_policy_column_to_policy.py +++ b/src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py @@ -1,19 +1,19 @@ -"""adds drp_policy column to policy +"""adds DRP action to policy -Revision ID: 0e0b346819d7 -Revises: 530fb8533ca4 -Create Date: 2022-04-28 20:36:01.314299 +Revision ID: 5078badb90b9 +Revises: 90070db16d05 +Create Date: 2022-05-04 17:22:46.500067 """ from alembic import op import sqlalchemy as sa -from sqlalchemy.dialects import postgresql -# revision identifiers, used by Alembic. +# revision identifiers, used by Alembic. +from sqlalchemy.dialects import postgresql -revision = "0e0b346819d7" -down_revision = "530fb8533ca4" +revision = "5078badb90b9" +down_revision = "90070db16d05" branch_labels = None depends_on = None diff --git a/tests/api/v1/endpoints/test_privacy_request_endpoints.py b/tests/api/v1/endpoints/test_privacy_request_endpoints.py index 71b2ff174..827669687 100644 --- a/tests/api/v1/endpoints/test_privacy_request_endpoints.py +++ b/tests/api/v1/endpoints/test_privacy_request_endpoints.py @@ -1521,6 +1521,7 @@ def test_resume_privacy_request( "reviewed_by": None, "reviewer": None, "policy": { + "drp_action": None, "key": privacy_request.policy.key, "name": privacy_request.policy.name, }, diff --git a/tests/models/test_policy.py b/tests/models/test_policy.py index f017f3a56..3ee07ed85 100644 --- a/tests/models/test_policy.py +++ b/tests/models/test_policy.py @@ -145,27 +145,6 @@ def test_create_access_rule_with_no_storage_destination_is_invalid( assert exc.value.args[0] == "Access Rules must have a storage destination." -def test_create_erasure_rule_invalid( - db: Session, - policy_drp_action: Policy, -) -> None: - with pytest.raises(RuleValidationError) as exc: - rule = Rule.create( - db=db, - data={ - "action_type": ActionType.erasure.value, - "client_id": policy_drp_action.client_id, - "name": "Invalid Erasure Rule due to Policy being associated with the access DRP Action", - "policy_id": policy_drp_action.id, - "masking_strategy": { - "strategy": NULL_REWRITE, - "configuration": {}, - }, - }, - ) - assert exc.value.args[0] == "Since the associated Policy has a DRP access action, this rule must have the matching erasure action_type." - - def test_consent_action_is_unsupported( db: Session, policy: Policy, From 34a141429e91d29f0d687900edd800d884a32af6 Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Wed, 4 May 2022 15:25:04 -0500 Subject: [PATCH 03/12] format --- src/fidesops/models/policy.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/fidesops/models/policy.py b/src/fidesops/models/policy.py index cdb58aae6..4af1d599a 100644 --- a/src/fidesops/models/policy.py +++ b/src/fidesops/models/policy.py @@ -324,7 +324,9 @@ def save(self, db: Session) -> FidesopsBase: @classmethod def create(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: """Validate this object's data before deferring to the superclass on update""" - associated_policy: Optional[Policy] = db.query(Policy).filter_by(id=data["policy_id"]).first() + associated_policy: Optional[Policy] = ( + db.query(Policy).filter_by(id=data["policy_id"]).first() + ) _validate_rule( action_type=data.get("action_type"), storage_destination_id=data.get("storage_destination_id"), From d51fd3138bd654efc39ba67afb9e905076c0ea32 Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Wed, 4 May 2022 15:31:40 -0500 Subject: [PATCH 04/12] remove unneeded func arg --- src/fidesops/models/policy.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/fidesops/models/policy.py b/src/fidesops/models/policy.py index 4af1d599a..1cca73edd 100644 --- a/src/fidesops/models/policy.py +++ b/src/fidesops/models/policy.py @@ -316,8 +316,7 @@ def save(self, db: Session) -> FidesopsBase: _validate_rule( action_type=self.action_type, storage_destination_id=self.storage_destination_id, - masking_strategy=self.masking_strategy, - drp_action=self.policy.drp_action, + masking_strategy=self.masking_strategy ) return super().save(db=db) @@ -330,8 +329,7 @@ def create(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: _validate_rule( action_type=data.get("action_type"), storage_destination_id=data.get("storage_destination_id"), - masking_strategy=data.get("masking_strategy"), - drp_action=associated_policy.drp_action if associated_policy else None, + masking_strategy=data.get("masking_strategy") ) return super().create(db=db, data=data) From 7d7f50a8253ddc43c16fb154722c3fa90aba65ac Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Wed, 4 May 2022 15:54:58 -0500 Subject: [PATCH 05/12] lint --- src/fidesops/models/policy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fidesops/models/policy.py b/src/fidesops/models/policy.py index 1cca73edd..41d67118e 100644 --- a/src/fidesops/models/policy.py +++ b/src/fidesops/models/policy.py @@ -316,7 +316,7 @@ def save(self, db: Session) -> FidesopsBase: _validate_rule( action_type=self.action_type, storage_destination_id=self.storage_destination_id, - masking_strategy=self.masking_strategy + masking_strategy=self.masking_strategy, ) return super().save(db=db) @@ -329,7 +329,7 @@ def create(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: _validate_rule( action_type=data.get("action_type"), storage_destination_id=data.get("storage_destination_id"), - masking_strategy=data.get("masking_strategy") + masking_strategy=data.get("masking_strategy"), ) return super().create(db=db, data=data) From 9d1ee927faa3101cdcb301527803d8959fbc957b Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Wed, 4 May 2022 16:02:48 -0500 Subject: [PATCH 06/12] more lint --- src/fidesops/models/policy.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/fidesops/models/policy.py b/src/fidesops/models/policy.py index 41d67118e..9540961dd 100644 --- a/src/fidesops/models/policy.py +++ b/src/fidesops/models/policy.py @@ -323,9 +323,6 @@ def save(self, db: Session) -> FidesopsBase: @classmethod def create(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: """Validate this object's data before deferring to the superclass on update""" - associated_policy: Optional[Policy] = ( - db.query(Policy).filter_by(id=data["policy_id"]).first() - ) _validate_rule( action_type=data.get("action_type"), storage_destination_id=data.get("storage_destination_id"), From e0927f51cb3f1be6c204bf063f195c8931877b62 Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Fri, 6 May 2022 10:31:03 -0500 Subject: [PATCH 07/12] CR changes --- src/fidesops/db/base_class.py | 24 +++++++++++++-------- src/fidesops/models/policy.py | 39 +++++------------------------------ 2 files changed, 20 insertions(+), 43 deletions(-) diff --git a/src/fidesops/db/base_class.py b/src/fidesops/db/base_class.py index 263fee1dc..152adeffa 100644 --- a/src/fidesops/db/base_class.py +++ b/src/fidesops/db/base_class.py @@ -176,14 +176,10 @@ def create(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: return cls.persist_obj(db, db_obj) @classmethod - def create_or_update(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: - """ - Create an object, or update the existing version. There's an edge case where - `data["id"]` and `data["key"]` can point at different records, in which case - this method will attempt to update the fetched record with the key of another, - and a `KeyOrNameAlreadyExists` error will be thrown. If neither `key`, nor `id` are - passed in, leave `db_obj` as None and assume we are creating a new object. - """ + def get_by_key_or_id( + cls, db: Session, *, data: Dict[str, Any] + ) -> Optional[FidesopsBase]: + """Retrieves db object by id, if provided, otherwise attempts by key""" db_obj = None if data.get("id") is not None: # If `id` has been included in `data`, preference that @@ -191,12 +187,22 @@ def create_or_update(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: elif data.get("key") is not None: # Otherwise, try with `key` db_obj = cls.get_by(db=db, field="key", value=data["key"]) + return db_obj + @classmethod + def create_or_update(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: + """ + Create an object, or update the existing version. There's an edge case where + `data["id"]` and `data["key"]` can point at different records, in which case + this method will attempt to update the fetched record with the key of another, + and a `KeyOrNameAlreadyExists` error will be thrown. If neither `key`, nor `id` are + passed in, leave `db_obj` as None and assume we are creating a new object. + """ + db_obj: FidesopsBase = cls.get_by_key_or_id(db=db, data=data) if db_obj: db_obj.update(db=db, data=data) else: db_obj = cls.create(db=db, data=data) - return db_obj @classmethod diff --git a/src/fidesops/models/policy.py b/src/fidesops/models/policy.py index 9540961dd..e72f2f19a 100644 --- a/src/fidesops/models/policy.py +++ b/src/fidesops/models/policy.py @@ -85,16 +85,10 @@ class is referenced in multiple database migrations. This class is to be removed """ -def _validate_drp_action( - drp_action: Optional[str], drp_action_exists: bool, class_name: str -) -> None: +def _validate_drp_action(drp_action: Optional[str]) -> None: """Check that DRP action is supported""" if not drp_action: return - if drp_action_exists: - raise common_exceptions.DrpActionValidationError( - f"DRP Action {drp_action} already exists in {class_name}." - ) if drp_action in [ DrpAction.sale_opt_in.value, DrpAction.sale_opt_out.value, @@ -153,37 +147,14 @@ class Policy(Base): @classmethod def create_or_update(cls, db: Session, *, data: Dict[str, Any]) -> FidesopsBase: """Overrides base create or update to add custom error for drp action already exists""" - db_obj = None - if data.get("id") is not None: - # If `id` has been included in `data`, preference that - db_obj = cls.get(db=db, id=data["id"]) - elif data.get("key") is not None: - # Otherwise, try with `key` - db_obj = cls.get_by(db=db, field="key", value=data["key"]) - + db_obj = cls.get_by_key_or_id(db=db, data=data) + if hasattr(cls, "drp_action"): + data["drp_action"] = data.get("drp_action", None) + _validate_drp_action(data["drp_action"]) if db_obj: - other_objs = db.query(cls).filter(id != db_obj.id) # pylint: disable=W0143 - _validate_drp_action( - data["drp_action"], - bool( - other_objs - and other_objs.filter_by(drp_action=data["drp_action"]).first() - ), - cls.__name__.lower(), - ) db_obj.update(db=db, data=data) else: - if hasattr(cls, "drp_action"): - data["drp_action"] = data.get("drp_action", None) - _validate_drp_action( - data["drp_action"], - bool( - db.query(cls).filter_by(drp_action=data["drp_action"]).first() - ), - cls.__name__.lower(), - ) db_obj = cls.create(db=db, data=data) - return db_obj def delete(self, db: Session) -> Optional[FidesopsBase]: From 1b011d4aae2ef94b2bf615329713a9c0f7ccc716 Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Fri, 6 May 2022 10:43:38 -0500 Subject: [PATCH 08/12] update alembic down migration number --- .../versions/5078badb90b9_adds_drp_action_to_policy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py b/src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py index 780ec79ed..7b89c5016 100644 --- a/src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py +++ b/src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py @@ -1,7 +1,7 @@ """adds DRP action to policy Revision ID: 5078badb90b9 -Revises: 90070db16d05 +Revises: 29a7d707163a Create Date: 2022-05-04 17:22:46.500067 """ @@ -13,7 +13,7 @@ from sqlalchemy.dialects import postgresql revision = "5078badb90b9" -down_revision = "90070db16d05" +down_revision = "29a7d707163a" branch_labels = None depends_on = None From 734b543852c84f551fdd590686e850a59eb8c9dd Mon Sep 17 00:00:00 2001 From: Cole Isaac Date: Fri, 6 May 2022 12:10:36 -0400 Subject: [PATCH 09/12] minor docs updates for drp --- docs/fidesops/docs/guides/policies.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/fidesops/docs/guides/policies.md b/docs/fidesops/docs/guides/policies.md index aabdd7ff1..64cee4718 100644 --- a/docs/fidesops/docs/guides/policies.md +++ b/docs/fidesops/docs/guides/policies.md @@ -50,12 +50,13 @@ PATCH /api/v1/policy This policy is subtly different from the concept of a Policy in [Fidesctl](https://github.com/ethyca/fides). A [Fidesctl policy](https://ethyca.github.io/fides/language/resources/policy/) dictates which data categories can be stored where. A Fidesops policy, on the other hand, dictates how to access, mask or erase data that matches specific data categories for privacy requests. ### Policy Attributes -- `Policy.name`: User-friendly name for your Policy. -- `Policy.key`: Unique key by which to reference the Policy. -- `Policy.drp_action` (optional): Which DRP action is this Policy handling? DRP action is only needed if you intend on using Fidesops as provider for the Data Rights Protocol (DRP). Read more about DRP [here](https://github.com/consumer-reports-digital-lab/data-rights-protocol). - - `access`: A data subject access request. Should be used with an `access` Rule. - - `deletion`: A data subject erasure request. Should be used with an `erasure` Rule. - +| Attribute | Description | +|---|---| +| `Policy.name` | User-friendly name for your Policy. | +| `Policy.key` | Unique key by which to reference the Policy. | +| `Policy.drp_action` | Optional. A [Data Rights Protocol](https://github.com/consumer-reports-digital-lab/data-rights-protocol) action to associate to this policy. | +| `access` | A data subject access request. Should be used with an `access` Rule. | +| `deletion` | A data subject erasure request. Should be used with an `erasure` Rule. | ## Add an Access Rule to your Policy The policy creation operation returns a Policy key, which we'll use to add a Rule: From 71665b99c67b3fa7cd68b151a8f94f45d82d1a5e Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Fri, 6 May 2022 14:29:38 -0500 Subject: [PATCH 10/12] integration tests and updating alembic --- src/fidesops/api/v1/endpoints/policy_endpoints.py | 2 +- .../versions/5078badb90b9_adds_drp_action_to_policy.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fidesops/api/v1/endpoints/policy_endpoints.py b/src/fidesops/api/v1/endpoints/policy_endpoints.py index 5a6f9e088..a96ac2b6e 100644 --- a/src/fidesops/api/v1/endpoints/policy_endpoints.py +++ b/src/fidesops/api/v1/endpoints/policy_endpoints.py @@ -216,7 +216,7 @@ def create_or_update_rules( "masking_strategy": masking_strategy_data, }, ) - except KeyOrNameAlreadyExists as exc: + except (KeyOrNameAlreadyExists, IntegrityError) as exc: logger.warning( f"Create/update failed for rule '{schema.key}' on policy {policy_key}: {exc}" ) diff --git a/src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py b/src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py index 7b89c5016..8b5d282ea 100644 --- a/src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py +++ b/src/migrations/versions/5078badb90b9_adds_drp_action_to_policy.py @@ -1,7 +1,7 @@ """adds DRP action to policy Revision ID: 5078badb90b9 -Revises: 29a7d707163a +Revises: c98da12d76f8 Create Date: 2022-05-04 17:22:46.500067 """ @@ -13,7 +13,7 @@ from sqlalchemy.dialects import postgresql revision = "5078badb90b9" -down_revision = "29a7d707163a" +down_revision = "c98da12d76f8" branch_labels = None depends_on = None From fa6c82a20d9245537f16889fc9c7b94613aa5386 Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Fri, 6 May 2022 14:42:10 -0500 Subject: [PATCH 11/12] catches integrity error at endpoint level --- src/fidesops/api/v1/endpoints/policy_endpoints.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fidesops/api/v1/endpoints/policy_endpoints.py b/src/fidesops/api/v1/endpoints/policy_endpoints.py index a96ac2b6e..02bc83213 100644 --- a/src/fidesops/api/v1/endpoints/policy_endpoints.py +++ b/src/fidesops/api/v1/endpoints/policy_endpoints.py @@ -118,7 +118,7 @@ def create_or_update_policies( "drp_action": policy_data.get("drp_action"), }, ) - except (KeyOrNameAlreadyExists, DrpActionValidationError) as exc: + except (KeyOrNameAlreadyExists, DrpActionValidationError, IntegrityError) as exc: logger.warning("Create/update failed for policy: %s", exc) failure = { "message": exc.args[0], @@ -216,7 +216,7 @@ def create_or_update_rules( "masking_strategy": masking_strategy_data, }, ) - except (KeyOrNameAlreadyExists, IntegrityError) as exc: + except KeyOrNameAlreadyExists as exc: logger.warning( f"Create/update failed for rule '{schema.key}' on policy {policy_key}: {exc}" ) From c41c2dc3134169a44029a785fa973f13614eac42 Mon Sep 17 00:00:00 2001 From: eastandwestwind Date: Fri, 6 May 2022 14:58:07 -0500 Subject: [PATCH 12/12] lint --- src/fidesops/api/v1/endpoints/policy_endpoints.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/fidesops/api/v1/endpoints/policy_endpoints.py b/src/fidesops/api/v1/endpoints/policy_endpoints.py index 02bc83213..83cf52c19 100644 --- a/src/fidesops/api/v1/endpoints/policy_endpoints.py +++ b/src/fidesops/api/v1/endpoints/policy_endpoints.py @@ -118,7 +118,11 @@ def create_or_update_policies( "drp_action": policy_data.get("drp_action"), }, ) - except (KeyOrNameAlreadyExists, DrpActionValidationError, IntegrityError) as exc: + except ( + KeyOrNameAlreadyExists, + DrpActionValidationError, + IntegrityError, + ) as exc: logger.warning("Create/update failed for policy: %s", exc) failure = { "message": exc.args[0],