Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added masking_strategy_override at field level #5446

Merged
merged 42 commits into from
Nov 18, 2024
Merged

Added masking_strategy_override at field level #5446

merged 42 commits into from
Nov 18, 2024

Conversation

Linker44
Copy link
Contributor

@Linker44 Linker44 commented Nov 1, 2024 •
edited
Loading

Closes #LA-60

Description Of Changes

This pr adds masking_strategy_overrides at the dataset field-level.

Many masking strict erasure requests fail because some fields dont have the correct format. This is caused by applying the same masking to each field when some of them should remain with a certain format.

ex: an endpoint will only succeed at updating an email field if it has an email format.

          - name: email_address
            data_categories: [user.contact.email]
            fidesops_meta:
              data_type: string
              masking_strategy_override:
                strategy: random_string_rewrite
                configuration:
                  length: 10
                  format_preservation:
                    suffix: "@test.com"

Code Changes

  • Added field-level masking strategy override.
  • Added Testing for update_value_map method in QueryConfig.
  • Added Testing for dsr database erasure request.
  • Added Validation for masking strategies that require secrets.
  • Added Testing for masking strategies that require secrets validation.
  • Renamed class MaskingOverride to something more appropiate (MaskingTruncation).

Steps to Confirm

Confirm masking override validation on fides admin:

  • Try to create a dataset that contains a field-level masking_strategy_override that requires secrets. It should fail to create it
  • Try to update a dataset so that it contains a field-level masking_strategy_override that requires secrets. It should fail to create it
  • Try to create a dataset that contains a field-level masking_strategy_override that doesn’t require secrets. It should succeed

Confirm masking override validation on startup (nox -s dev):

  • Update one of the existing datasets so that it contains a field-level masking_strategy_override that requires secrets. It should raise a validation error on the terminal and fail to create it.
  • Update one of the existing datasets so that it contains a field-level masking_strategy_override that doesn't requires secrets. It should succeed.

Confirm masking override is working:

  • Alter a dsr saas dataset in a user collection field so that it contains a field_level_masking override that doesnt require secrets -> create a system with the integration. (make sure to have the enviroment variable FIDES__DEV_MODE set to true to see the request body in the terminal) -> make an erasure request -> Confirm that the request has the correct masking on the specified field.

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation:
    • documentation complete, PR opened in fidesdocs
    • documentation issue created in fidesdocs
    • if there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md
  • For API changes, the Postman collection has been updated
  • If there are any database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!

@vercel Vercel
Copy link

vercel bot commented Nov 1, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Nov 18, 2024 4:24pm

@Linker44 Linker44 self-assigned this Nov 1, 2024
@cypress Cypress
Copy link

cypress bot commented Nov 4, 2024 •
edited
Loading

fides    Run #11040

Run Properties:  status check passed Passed #11040  •  git commit 7d643dc386 ℹ️: Merge b31509540c43bb1e5a1727fa9796c865e264460e into 523c1ab716c666feeca4636b844c...
Project fides
Branch Review refs/pull/5446/merge
Run status status check passed Passed #11040
Run duration 00m 40s
Commit git commit 7d643dc386 ℹ️: Merge b31509540c43bb1e5a1727fa9796c865e264460e into 523c1ab716c666feeca4636b844c...
Committer Facundo Lopez Janza
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

@codecov Codecov
Copy link

codecov bot commented Nov 5, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 78.33333% with 13 lines in your changes missing coverage. Please review.

Project coverage is 79.16%. Comparing base (52ac330) to head (1d0382c).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/fides/api/api/v1/endpoints/router_factory.py 30.00% 7 Missing ⚠️
src/fides/api/service/connectors/query_config.py 78.57% 1 Missing and 2 partials ⚠️
src/fides/api/util/data_category.py 81.81% 1 Missing and 1 partial ⚠️
src/fides/core/api.py 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5446      +/-   ##
==========================================
- Coverage   85.20%   79.16%   -6.04%     
==========================================
  Files         386      387       +1     
  Lines       24247    24303      +56     
  Branches     2644     2654      +10     
==========================================
- Hits        20659    19239    -1420     
- Misses       3033     4550    +1517     
+ Partials      555      514      -41

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Linker44 Linker44 marked this pull request as ready for review November 7, 2024 02:07
@Linker44 Linker44 requested a review from galvana November 7, 2024 02:07
@Linker44 Linker44 requested a review from a team November 7, 2024 19:08
Linker44
Linker44 commented Nov 7, 2024
View reviewed changes
src/fides/api/api/v1/endpoints/dataset_endpoints.py
@@ -417,6 +418,7 @@ def create_or_update_dataset(
# when a ctl_dataset is being linked to a Saas Connector.
_validate_saas_dataset(connection_config, dataset) # type: ignore
# Try to find an existing DatasetConfig matching the given connection & key
validate_masking_strategy_override(dataset)
Copy link
Contributor Author

@Linker44 Linker44 Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dataset creation and update happens in many places so this validation gets repeated all over. this is one of those cases where a service would come in handy.

@Linker44 Linker44 dismissed galvana’s stale review November 12, 2024 14:31

changes implemented

@galvana galvana added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Nov 12, 2024
galvana
galvana approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@galvana galvana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the recommended changes, this looks good to me!

@galvana galvana self-requested a review November 13, 2024 20:14
galvana
galvana approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@galvana galvana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Linker44, I just asked that you revert the debugging changes pytest_ctl, then we're good to go!

@Linker44 Linker44 merged commit 861201b into main Nov 18, 2024
40 checks passed
@Linker44 Linker44 deleted the field_level_masking branch November 18, 2024 18:08
@cypress Cypress
Copy link

cypress bot commented Nov 18, 2024

fides    Run #11047

Run Properties:  status check passed Passed #11047  •  git commit 861201b96c: Added masking_strategy_override at field level (#5446)
Project fides
Branch Review main
Run status status check passed Passed #11047
Run duration 00m 35s
Commit git commit 861201b96c: Added masking_strategy_override at field level (#5446)
Committer Facundo Lopez Janza
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 4
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@galvana galvana galvana approved these changes

Assignees

@Linker44 Linker44

Labels
run unsafe ci checks Runs fides-related CI checks that require sensitive credentials
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Linker44 @galvana