Adding support for configuring enabled actions #4374

galvana · 2023-11-02T02:43:54Z

Description Of Changes

⚠️ Requires the corresponding branch in fidesplus to test https://github.com/ethyca/fidesplus/pull/1205

Adding the ability to specify which privacy actions (access, erasure, consent) to enable for a system integration. The new Enabled actions field is a multi-select with the options being the actions supported by the integration.

Code Changes

Updated the ConnectorParamaters component to call the fidesplus versions of the system integration endpoints if fidesplus is available
Updated the ConnectorParameterForm to conditionally render the Enabled actions field if fidesplus is enabled AND there is more than one action to choose from. The option to disable an action won't be available if it's the only action.
Added fidesplus versions of patchSystemConnectionConfigs and createSaasConnectionConfig to plus.slice.ts
Updated select models with supported_actions and enabled_actions fields
Refactored the has_access, has_erasure, has_consent logic in connection_type.py and moved it to saas_config.py
Minor cleanup of the connector_registry_service.py
Minor field name fixes (SSH required)

Steps to Confirm

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
Issue Requirements are Met
Update CHANGELOG.md

…e connector template

galvana · 2023-11-02T17:47:11Z

src/fides/api/service/connectors/saas/connector_registry_service.py

-                config_file = os.path.join("data/saas/config", file)
-                config_dict = load_config(config_file)
-                connector_type = config_dict["type"]
-                human_readable = config_dict["name"]
-                user_guide = config_dict.get("user_guide")
-                authentication = config_dict["client_config"].get("authentication")
+                config_path = os.path.join("data/saas/config", file)
+                config = SaaSConfig(**load_config(config_path))
+
+                connector_type = config.type
+
+                authentication = config.client_config.authentication


Refactoring to use a SaaSConfig instead of a Dict this way it's more consistent with the way it's done in other places in this file.

that seems good 👍

galvana · 2023-11-02T17:48:07Z

src/fides/api/util/connection_type.py

-        return bool(
-            (ActionType.consent in action_types and has_consent)
-            or (ActionType.access in action_types and has_access)
-            or (ActionType.erasure in action_types and has_erasure)


This logic was simplified and moved to the SaaSConfig https://github.com/ethyca/fides/pull/4374/files#diff-e02882cee5e6ee2325e4d613abed561b386e3e15064d0a1cbb41785a5d3f6d1aR483

nice! like the refactor here 👍

galvana · 2023-11-02T17:48:45Z

src/fides/api/util/connection_type.py

@@ -157,6 +128,7 @@ def saas_request_type_filter(connection_type: str) -> bool:
                    identifier=item,
                    type=SystemType.database,
                    human_readable=ConnectionType(item).human_readable,
+                    supported_actions=[ActionType.access, ActionType.erasure],


DB connectors will support access and erasure by default

galvana · 2023-11-02T17:50:14Z

tests/ops/api/v1/endpoints/test_connection_template_endpoints.py

@@ -1005,7 +1005,7 @@ def test_get_connection_secret_schema_mysql(
                    "type": "string",
                },
                "ssh_required": {
-                    "title": "SSH Required",
+                    "title": "SSH required",


Changing SSH required to use sentence-case to be consistent with the other field labels.

how dare you!

galvana · 2023-11-02T17:51:59Z

src/fides/api/service/connectors/saas/connector_registry_service.py

+    enabled_actions = getattr(template_values, "enabled_actions", None)
+    if enabled_actions is not None:
+        data["enabled_actions"] = enabled_actions


This function is used by both fides and fidesplus endpoints so we need to perform this attribute check to map the enabled_actions attribute which is only found in fidesplus.

yeah, this is an interesting pattern. i can't say i love it, it feels like a development footgun moving forward - we're implying (in a pretty cryptic manner) that someone developing here in fides knows about something defined over in fidesplus. i'm wouldn't be surprised if we do that in some other places already, but my gut-instinct tells me it's something we should try to avoid if possible.

i'll make a broader comment on the PR at this point, since i think this is a holistic concern - not tied to this particular code block, even though this is where it's manifested

galvana · 2023-11-02T18:05:19Z

clients/admin-ui/src/types/api/models/ConnectionConfigurationResponse.ts

These two model changes were generated by openapi:generate but I didn't keep the other changes to avoid a huge diff.

cypress · 2023-11-02T18:25:04Z

Passing run #5175 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `796bf00` into `1e6d9fb`...
Project: fides	Commit: `cfc8d136ef ℹ️`
Status: Passed	Duration: 00:54 💡
Started: Nov 10, 2023 3:38 AM	Ended: Nov 10, 2023 3:38 AM

Review all test suite changes for PR #4374 ↗︎

adamsachs

@galvana broadly this looks good, i just get concerned with any sort of reverse dependency of OSS on plus. i know it's not a hard dependency, but see my inline comment for the concern - basically it's a maintainability/development concern. having an expectation within the BE OSS codebase that we need to be aware of things defined in plus sets a bad precedent. we may do it in other places, but i don't think it should be encouraged.

i know we're trying to layer in a plus-only feature on top of a lot of OSS plumbing that i'm sure needs to stay in OSS, or at least can't be quickly moved out. so if we need to do this here, i'm not going to totally hold this up. i know we're not purists on this, nor should we be - i just want to see if there's anything we can think of to untangle this.

what's first popping into my head - how about some sort of override/extension of create_connection_config_from_template_no_save in plus that has the enabled action handling tacked on top of the rest of the function defined in OSS? basically just keeping all enabled_action logic in plus...

happy to talk this one over. maybe my concern's misplaced/overblown. in any case, i don't think this path forward is totally unviable -- if we need to do it, we can. it just adds some tech debt and feels to me like an antipattern we should strive to avoid, if we can.

edit: i've got a tweak that would address the main point of my concern, and i think it'd be cleaner code generally. still untested though! let me know what you think... #4386 and https://github.com/ethyca/fidesplus/pull/1210

adamsachs · 2023-11-03T23:47:10Z

src/fides/api/service/connectors/saas/connector_registry_service.py

+    enabled_actions = getattr(template_values, "enabled_actions", None)
+    if enabled_actions is not None:
+        data["enabled_actions"] = enabled_actions


yeah, this is an interesting pattern. i can't say i love it, it feels like a development footgun moving forward - we're implying (in a pretty cryptic manner) that someone developing here in fides knows about something defined over in fidesplus. i'm wouldn't be surprised if we do that in some other places already, but my gut-instinct tells me it's something we should try to avoid if possible.

i'll make a broader comment on the PR at this point, since i think this is a holistic concern - not tied to this particular code block, even though this is where it's manifested

adamsachs · 2023-11-03T23:48:22Z

tests/ops/api/v1/endpoints/test_connection_template_endpoints.py

@@ -1005,7 +1005,7 @@ def test_get_connection_secret_schema_mysql(
                    "type": "string",
                },
                "ssh_required": {
-                    "title": "SSH Required",
+                    "title": "SSH required",


how dare you!

adamsachs · 2023-11-03T23:54:42Z

src/fides/api/util/connection_type.py

-        return bool(
-            (ActionType.consent in action_types and has_consent)
-            or (ActionType.access in action_types and has_access)
-            or (ActionType.erasure in action_types and has_erasure)


nice! like the refactor here 👍

adamsachs · 2023-11-03T23:54:59Z

src/fides/api/service/connectors/saas/connector_registry_service.py

-                config_file = os.path.join("data/saas/config", file)
-                config_dict = load_config(config_file)
-                connector_type = config_dict["type"]
-                human_readable = config_dict["name"]
-                user_guide = config_dict.get("user_guide")
-                authentication = config_dict["client_config"].get("authentication")
+                config_path = os.path.join("data/saas/config", file)
+                config = SaaSConfig(**load_config(config_path))
+
+                connector_type = config.type
+
+                authentication = config.client_config.authentication


that seems good 👍

vercel · 2023-11-06T17:51:25Z

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Nov 10, 2023 3:23am

codecov · 2023-11-06T22:28:16Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (1e6d9fb) 87.08% compared to head (796bf00) 87.09%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4374   +/-   ##
=======================================
  Coverage   87.08%   87.09%           
=======================================
  Files         329      329           
  Lines       20309    20354   +45     
  Branches     2610     2621   +11     
=======================================
+ Hits        17687    17728   +41     
- Misses       2160     2163    +3     
- Partials      462      463    +1

Files	Coverage Δ
src/fides/api/common_exceptions.py	`91.48% <100.00%> (+0.09%)`	⬆️
src/fides/api/models/manual_webhook.py	`100.00% <100.00%> (ø)`
...emas/connection_configuration/connection_config.py	`100.00% <100.00%> (ø)`
...nnection_configuration/connection_secrets_mysql.py	`100.00% <ø> (ø)`
...ction_configuration/connection_secrets_postgres.py	`100.00% <ø> (ø)`
...ction_configuration/connection_secrets_redshift.py	`100.00% <ø> (ø)`
...ection_configuration/connection_type_system_map.py	`100.00% <100.00%> (ø)`
...ction_configuration/saas_config_template_values.py	`100.00% <100.00%> (ø)`
src/fides/api/schemas/saas/connector_template.py	`84.61% <100.00%> (+1.28%)`	⬆️
src/fides/api/schemas/saas/saas_config.py	`95.88% <100.00%> (+0.19%)`	⬆️
... and 5 more

... and 1 file with indirect coverage changes

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

adamsachs

ok from a BE perspective this is looking good to me! i had a few comments on the plus PR of things you may look to tweak, but i don't think any of those would require updates here.

a couple of notes that we just need to consider before merging:

reminder to update the readme :)
i haven't done any code review on the FE pieces, so it may be good to have a FE dev specifically review those?
i haven't gotten a chance to perform any UAT on this - and that feels like that'd probably be a good idea, particularly if there hasn't been any FE code review! i can get to that tomorrow if that's OK?

allisonking · 2023-11-08T16:15:23Z

taking a look! getting an error once I select an integration:

allisonking · 2023-11-08T19:22:05Z

taking a look! getting an error once I select an integration:

Never mind, I think I needed to run fidesplus with dev_prerelease to get this working!

allisonking

this is looking good! the only thing that might be odd is when I try to autogenerate TS types off of your fidesplus branch, the enabled_actions field gets deleted from CreateConnectionConfigurationWithSecrets—is that expected?

allisonking · 2023-11-08T17:16:19Z

clients/admin-ui/cypress/e2e/system-integrations.cy.ts

+    it("should not display disabled actions field", () => {
+      cy.getByTestId("enabled-actions").should("not.exist");
+    });
+  });


thanks for adding these tests! 💯

allisonking · 2023-11-08T19:27:34Z

...ui/src/features/datastore-connections/system_portal_config/forms/ConnectorParametersForm.tsx

+                  <Field
+                    id="enabled_actions"
+                    name="enabled_actions"
+                    validate={(value: string[]) => {


normally it'd be better to do this as part of a validation schema, but I don't think this form has one... it does validation pretty oddly, so this is in line with the component, just making a note!

galvana · 2023-11-09T00:34:54Z

this is looking good! the only thing that might be odd is when I try to autogenerate TS types off of your fidesplus branch, the enabled_actions field gets deleted from CreateConnectionConfigurationWithSecrets—is that expected?

Yes @allisonking, that's expected, enabled_actions was moved to the fidesplus model CreateConnectionConfigurationWithSecretsExtended

Co-authored-by: Adam Sachs <adam@ethyca.com>

adamsachs · 2023-11-10T14:07:33Z

UAT tested this piece on RC, looks good! still haven't yet confirmed the effect of enabled actions but the form functionality (including saving/persisting changes) looks good 👍

galvana added 3 commits November 1, 2023 19:43

Adding support for configuring enabled actions

61bb5ca

Merge branch 'main' into PROD-1249-configure-enabled-actions

cf04051

Deriving supported actions from the SaaS config and adding them to th…

53dd134

…e connector template

galvana commented Nov 2, 2023

View reviewed changes

galvana added 2 commits November 2, 2023 10:56

Adding erasure as a supported action for manual connectors

379dd29

Partial update of types from openapi:generate

bec8ab0

galvana commented Nov 2, 2023

View reviewed changes

galvana added 2 commits November 2, 2023 11:14

Fixing type error

370d2d7

Removing unused imports

56c310b

galvana added 3 commits November 2, 2023 16:15

Adding tests

8736349

Merge branch 'main' into PROD-1249-configure-enabled-actions

809142d

Fixing error message for enabled actions field

48f6a77

galvana marked this pull request as ready for review November 3, 2023 03:43

galvana requested a review from adamsachs November 3, 2023 03:44

adamsachs reviewed Nov 3, 2023

View reviewed changes

adamsachs mentioned this pull request Nov 4, 2023

implement enabled actions support in a more extensible manner #4386

Merged

9 tasks

implement enabled actions support in a more extensible manner (#4386)

cf45f14

Fixing tests

8433ed6

galvana requested a review from adamsachs November 6, 2023 22:45

adamsachs reviewed Nov 7, 2023

View reviewed changes

Merge branch 'main' into PROD-1249-configure-enabled-actions

685681b

galvana requested a review from allisonking November 8, 2023 01:50

vercel bot deployed to Preview November 8, 2023 01:51 View deployment

allisonking approved these changes Nov 8, 2023

View reviewed changes

Updating field

4526520

vercel bot deployed to Preview November 9, 2023 19:25 View deployment

galvana added 2 commits November 9, 2023 12:11

Adding enabled actions to DSR workflow (#4400)

c8da2ca

Merge branch 'main' into PROD-1249-configure-enabled-actions

d49dc9c

vercel bot deployed to Preview November 9, 2023 20:21 View deployment

galvana added 5 commits November 9, 2023 12:21

Updating change log

df426b2

Fixing tests

48586a2

Fixing enabled action test

dac2d6a

Removing dependency on mongodb fixtures

8daddb9

Simplifying expression

796bf00

galvana merged commit 22614a5 into main Nov 10, 2023
45 of 46 checks passed

galvana deleted the PROD-1249-configure-enabled-actions branch November 10, 2023 04:04

galvana added a commit that referenced this pull request Nov 10, 2023

Adding support for configuring enabled actions (#4374)

ce3dd39

Co-authored-by: Adam Sachs <adam@ethyca.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for configuring enabled actions #4374

Adding support for configuring enabled actions #4374

galvana commented Nov 2, 2023 •

edited

Loading

galvana Nov 2, 2023

adamsachs Nov 3, 2023

galvana Nov 2, 2023 •

edited

Loading

adamsachs Nov 3, 2023

galvana Nov 2, 2023

galvana Nov 2, 2023

adamsachs Nov 3, 2023

galvana Nov 2, 2023

adamsachs Nov 3, 2023

galvana Nov 2, 2023

cypress bot commented Nov 2, 2023 •

edited

Loading

adamsachs left a comment •

edited

Loading

adamsachs Nov 3, 2023

adamsachs Nov 3, 2023

adamsachs Nov 3, 2023

adamsachs Nov 3, 2023

vercel bot commented Nov 6, 2023 •

edited

Loading

codecov bot commented Nov 6, 2023 •

edited

Loading

adamsachs left a comment

allisonking commented Nov 8, 2023

allisonking commented Nov 8, 2023

allisonking left a comment

allisonking Nov 8, 2023

allisonking Nov 8, 2023

galvana commented Nov 9, 2023

adamsachs commented Nov 10, 2023

Adding support for configuring enabled actions #4374

Adding support for configuring enabled actions #4374

Conversation

galvana commented Nov 2, 2023 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galvana Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Nov 2, 2023 • edited Loading

Passing run #5175 ↗︎

Review all test suite changes for PR #4374 ↗︎

adamsachs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Nov 6, 2023 • edited Loading

codecov bot commented Nov 6, 2023 • edited Loading

Codecov Report

adamsachs left a comment

Choose a reason for hiding this comment

allisonking commented Nov 8, 2023

allisonking commented Nov 8, 2023

allisonking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galvana commented Nov 9, 2023

adamsachs commented Nov 10, 2023

galvana commented Nov 2, 2023 •

edited

Loading

galvana Nov 2, 2023 •

edited

Loading

cypress bot commented Nov 2, 2023 •

edited

Loading

adamsachs left a comment •

edited

Loading

vercel bot commented Nov 6, 2023 •

edited

Loading

codecov bot commented Nov 6, 2023 •

edited

Loading