From b755e033216f770a2ba3f1ed624e4921054d67d4 Mon Sep 17 00:00:00 2001 From: Andrew Jackson Date: Mon, 26 Jun 2023 16:50:17 -0400 Subject: [PATCH 01/12] Initial optional name work --- ...da6_make_connectionconfig_name_optional.py | 36 +++++++++++++++++++ src/fides/api/models/connectionconfig.py | 2 +- .../connection_config.py | 6 ++-- 3 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py diff --git a/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py b/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py new file mode 100644 index 00000000000..27d21da95e9 --- /dev/null +++ b/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py @@ -0,0 +1,36 @@ +"""make ConnectionConfig.name optional + +Revision ID: 7315b9d7fda6 +Revises: 2be84e68df32 +Create Date: 2023-06-26 20:39:29.953904 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '7315b9d7fda6' +down_revision = '2be84e68df32' +branch_labels = None +depends_on = None + + +def upgrade(): + pass + op.alter_column( + "connectionconfig", "name", nullable=True + ) + + op.drop_index( + op.f("ix_connectionconfig_name"), table_name="connectionconfig" + ) + +def downgrade(): + op.alter_column( + "connectionconfig", "name", nullable=False + ) + + op.create_index( + op.f("ix_connectionconfig_name"), "connectionconfig", ["name"], unique=True + ) diff --git a/src/fides/api/models/connectionconfig.py b/src/fides/api/models/connectionconfig.py index efdc575829a..5500312c557 100644 --- a/src/fides/api/models/connectionconfig.py +++ b/src/fides/api/models/connectionconfig.py @@ -105,7 +105,7 @@ class ConnectionConfig(Base): Stores credentials to connect fidesops to an engineer's application databases. """ - name = Column(String, index=True, unique=True, nullable=False) + name = Column(String, nullable=True) key = Column(String, index=True, unique=True, nullable=False) description = Column(String, index=True, nullable=True) connection_type = Column(Enum(ConnectionType), nullable=False) diff --git a/src/fides/api/schemas/connection_configuration/connection_config.py b/src/fides/api/schemas/connection_configuration/connection_config.py index 1f3fe26d0c9..8ab65baeaf6 100644 --- a/src/fides/api/schemas/connection_configuration/connection_config.py +++ b/src/fides/api/schemas/connection_configuration/connection_config.py @@ -20,7 +20,7 @@ class CreateConnectionConfiguration(BaseModel): Note that secrets are *NOT* allowed to be supplied here. """ - name: str + name: Optional[str] key: Optional[FidesKey] connection_type: ConnectionType access: AccessLevel @@ -93,7 +93,7 @@ class ConnectionConfigurationResponse(BaseModel): Do *NOT* add "secrets" to this schema. """ - name: str + name: Optional[str] key: FidesKey description: Optional[str] connection_type: ConnectionType @@ -128,7 +128,7 @@ class BulkPatchConnectionConfigurationWithSecrets(BulkResponse): class SaasConnectionTemplateValues(BaseModel): """Schema with values to create both a Saas ConnectionConfig and DatasetConfig from a template""" - name: str # For ConnectionConfig + name: Optional[str] # For ConnectionConfig key: Optional[FidesKey] # For ConnectionConfig description: Optional[str] # For ConnectionConfig secrets: connection_secrets_schemas # For ConnectionConfig From 82ae7f1269d21d4d0a8e408fbf71ab0bf9d8f9d0 Mon Sep 17 00:00:00 2001 From: Andrew Jackson Date: Tue, 27 Jun 2023 10:29:26 -0400 Subject: [PATCH 02/12] Add test for optional name --- .../api/v1/endpoints/test_connection_config_endpoints.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/ops/api/v1/endpoints/test_connection_config_endpoints.py b/tests/ops/api/v1/endpoints/test_connection_config_endpoints.py index f9cbd3cd816..a6eaae979e6 100644 --- a/tests/ops/api/v1/endpoints/test_connection_config_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_connection_config_endpoints.py @@ -707,6 +707,14 @@ def test_patch_http_connection_successful_analytics( assert call_args[5] is None + def test_patch_connections_no_name(self, api_client, generate_auth_header, url, payload): + auth_header = generate_auth_header(scopes=[CONNECTION_CREATE_OR_UPDATE]) + del payload[0]["name"] + response = api_client.patch(url, headers=auth_header, json=payload) + assert 200 == response.status_code + assert response.json()["succeeded"][0]["name"] is None + + class TestGetConnections: @pytest.fixture(scope="function") def url(self, oauth_client: ClientDetail, policy) -> str: From fa97492cebed43abb3b58d80a81910e026edc3b6 Mon Sep 17 00:00:00 2001 From: Andrew Jackson Date: Tue, 27 Jun 2023 10:42:23 -0400 Subject: [PATCH 03/12] Fix type hint & format --- ...7fda6_make_connectionconfig_name_optional.py | 17 ++++++----------- src/fides/api/models/connectionconfig.py | 4 +++- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py b/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py index 27d21da95e9..478d230f2e3 100644 --- a/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py +++ b/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py @@ -10,26 +10,21 @@ # revision identifiers, used by Alembic. -revision = '7315b9d7fda6' -down_revision = '2be84e68df32' +revision = "7315b9d7fda6" +down_revision = "2be84e68df32" branch_labels = None depends_on = None def upgrade(): pass - op.alter_column( - "connectionconfig", "name", nullable=True - ) + op.alter_column("connectionconfig", "name", nullable=True) + + op.drop_index(op.f("ix_connectionconfig_name"), table_name="connectionconfig") - op.drop_index( - op.f("ix_connectionconfig_name"), table_name="connectionconfig" - ) def downgrade(): - op.alter_column( - "connectionconfig", "name", nullable=False - ) + op.alter_column("connectionconfig", "name", nullable=False) op.create_index( op.f("ix_connectionconfig_name"), "connectionconfig", ["name"], unique=True diff --git a/src/fides/api/models/connectionconfig.py b/src/fides/api/models/connectionconfig.py index 5500312c557..3dc7d0a546f 100644 --- a/src/fides/api/models/connectionconfig.py +++ b/src/fides/api/models/connectionconfig.py @@ -155,10 +155,12 @@ class ConnectionConfig(Base): ) @property - def system_key(self) -> str: + def system_key(self) -> Optional[str]: """Property for caching a system identifier for systems (or connector names as a fallback) for consent reporting""" if self.system: return self.system.fides_key + # TODO: Remove this fallback once all connection configs are linked to systems + # This will always be None in the future. `self.system` will always be set. return self.name @classmethod From 4fbcfa1612da9591010810c5dc91c36bbf62b17a Mon Sep 17 00:00:00 2001 From: Andrew Jackson Date: Tue, 27 Jun 2023 12:08:23 -0400 Subject: [PATCH 04/12] Fix saas creation issues --- src/fides/api/models/connectionconfig.py | 10 +--------- .../connectors/saas/connector_registry_service.py | 3 +-- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/src/fides/api/models/connectionconfig.py b/src/fides/api/models/connectionconfig.py index 3dc7d0a546f..57bad62b7ee 100644 --- a/src/fides/api/models/connectionconfig.py +++ b/src/fides/api/models/connectionconfig.py @@ -14,7 +14,7 @@ ) from fides.api.common_exceptions import KeyOrNameAlreadyExists -from fides.api.db.base_class import Base, FidesBase, JSONTypeOverride, get_key_from_data +from fides.api.db.base_class import Base, FidesBase, JSONTypeOverride from fides.api.models.sql_models import System # type: ignore[attr-defined] from fides.api.schemas.policy import ActionType from fides.api.schemas.saas.saas_config import SaaSConfig @@ -171,7 +171,6 @@ def create_without_saving( # Build properly formatted key/name for ConnectionConfig. # Borrowed from OrmWrappedFidesBase.create if hasattr(cls, "key"): - data["key"] = get_key_from_data(data, cls.__name__) if db.query(cls).filter_by(key=data["key"]).first(): raise KeyOrNameAlreadyExists( f"Key {data['key']} already exists in {cls.__name__}. Keys will be snake-cased names if not provided. " @@ -179,13 +178,6 @@ def create_without_saving( "" ) - if hasattr(cls, "name"): - data["name"] = data.get("name") - if db.query(cls).filter_by(name=data["name"]).first(): - raise KeyOrNameAlreadyExists( - f"Name {data['name']} already exists in {cls.__name__}." - ) - # Create db_obj = cls(**data) # type: ignore return db_obj diff --git a/src/fides/api/service/connectors/saas/connector_registry_service.py b/src/fides/api/service/connectors/saas/connector_registry_service.py index 93c16863fc6..8fbf90786a8 100644 --- a/src/fides/api/service/connectors/saas/connector_registry_service.py +++ b/src/fides/api/service/connectors/saas/connector_registry_service.py @@ -323,8 +323,7 @@ def create_connection_config_from_template_no_save( ) data = { - "name": template_values.name, - "key": template_values.key, + "key": template_values.instance_key, "description": template_values.description, "connection_type": ConnectionType.saas, "access": AccessLevel.write, From 054986d663a5c0a243819118acf5db3e10e331ed Mon Sep 17 00:00:00 2001 From: Andrew Jackson Date: Tue, 27 Jun 2023 12:09:12 -0400 Subject: [PATCH 05/12] Update migration This makes the migration not error out if a developer gets a weird inbetween state like I did --- .../7315b9d7fda6_make_connectionconfig_name_optional.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py b/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py index 478d230f2e3..738da94c9e4 100644 --- a/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py +++ b/src/fides/api/alembic/migrations/versions/7315b9d7fda6_make_connectionconfig_name_optional.py @@ -26,6 +26,7 @@ def upgrade(): def downgrade(): op.alter_column("connectionconfig", "name", nullable=False) - op.create_index( - op.f("ix_connectionconfig_name"), "connectionconfig", ["name"], unique=True + conn = op.get_bind() + conn.execute( + "CREATE UNIQUE INDEX IF NOT EXISTS ix_connectionconfig_name ON connectionconfig (name);" ) From 4f230b224e9bbf4592241ef726b534cf8254230a Mon Sep 17 00:00:00 2001 From: Andrew Jackson Date: Tue, 27 Jun 2023 14:37:41 -0400 Subject: [PATCH 06/12] fix dataset tests --- tests/fixtures/saas/connection_template_fixtures.py | 12 +++--------- .../v1/endpoints/test_connection_config_endpoints.py | 1 - 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/tests/fixtures/saas/connection_template_fixtures.py b/tests/fixtures/saas/connection_template_fixtures.py index dfa3d8a31c7..292fe3864a8 100644 --- a/tests/fixtures/saas/connection_template_fixtures.py +++ b/tests/fixtures/saas/connection_template_fixtures.py @@ -31,7 +31,6 @@ def secondary_sendgrid_instance(db): connection_config, dataset_config = instantiate_connector( db, "sendgrid", - "sendgrid_connection_config_secondary", "secondary_sendgrid_instance", "Sendgrid ConnectionConfig description", secrets, @@ -49,7 +48,6 @@ def secondary_mailchimp_instance(db): """ connection_config, dataset_config = instantiate_mailchimp( db, - "mailchimp_connection_config_secondary", "secondary_mailchimp_instance", ) yield connection_config, dataset_config @@ -67,7 +65,6 @@ def tertiary_mailchimp_instance(db): """ connection_config, dataset_config = instantiate_mailchimp( db, - "mailchimp_connection_config_tertiary", "tertiary_mailchimp_instance", ) yield connection_config, dataset_config @@ -84,7 +81,6 @@ def instantiate_mailchimp(db, key, fides_key) -> tuple[ConnectionConfig, Dataset return instantiate_connector( db, "mailchimp", - key, fides_key, "Mailchimp ConnectionConfig description", secrets, @@ -94,7 +90,6 @@ def instantiate_mailchimp(db, key, fides_key) -> tuple[ConnectionConfig, Dataset def instantiate_connector( db, connector_type, - key, fides_key, description, secrets, @@ -106,15 +101,14 @@ def instantiate_connector( ConnectorTemplate ] = ConnectorRegistry.get_connector_template(connector_type) template_vals = SaasConnectionTemplateValues( - name=key, - key=key, + key=fides_key, description=description, secrets=secrets, instance_key=fides_key, ) connection_config = ConnectionConfig.filter( - db=db, conditions=(ConnectionConfig.key == key) + db=db, conditions=(ConnectionConfig.key == fides_key) ).first() assert connection_config is None @@ -140,7 +134,7 @@ def instantiate_connector( ) connection_config = ConnectionConfig.filter( - db=db, conditions=(ConnectionConfig.key == key) + db=db, conditions=(ConnectionConfig.key == fides_key) ).first() assert connection_config is not None dataset_config = DatasetConfig.filter( diff --git a/tests/ops/api/v1/endpoints/test_connection_config_endpoints.py b/tests/ops/api/v1/endpoints/test_connection_config_endpoints.py index a6eaae979e6..fd047b9a154 100644 --- a/tests/ops/api/v1/endpoints/test_connection_config_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_connection_config_endpoints.py @@ -1287,7 +1287,6 @@ def test_delete_saas_connection_config( connection_config, dataset_config = instantiate_connector( db, "sendgrid", - "sendgrid_connection_config_secondary", "secondary_sendgrid_instance", "Sendgrid ConnectionConfig description", secrets, From 04266ead6e850285c9536fa169eb37cc413b7208 Mon Sep 17 00:00:00 2001 From: Andrew Jackson Date: Tue, 27 Jun 2023 15:37:12 -0400 Subject: [PATCH 07/12] Fix test failures --- .../saas/connector_registry_service.py | 4 +++- .../saas/connection_template_fixtures.py | 2 +- tests/ops/api/v1/endpoints/test_system.py | 18 +++++++----------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/fides/api/service/connectors/saas/connector_registry_service.py b/src/fides/api/service/connectors/saas/connector_registry_service.py index 8fbf90786a8..a7283b89801 100644 --- a/src/fides/api/service/connectors/saas/connector_registry_service.py +++ b/src/fides/api/service/connectors/saas/connector_registry_service.py @@ -323,12 +323,14 @@ def create_connection_config_from_template_no_save( ) data = { - "key": template_values.instance_key, + "key": template_values.key if template_values.key else template_values.instance_key, "description": template_values.description, "connection_type": ConnectionType.saas, "access": AccessLevel.write, "saas_config": config_from_template, } + if template_values.name: + data["name"] = template_values.name if system_id: data["system_id"] = system_id diff --git a/tests/fixtures/saas/connection_template_fixtures.py b/tests/fixtures/saas/connection_template_fixtures.py index 292fe3864a8..04aeeac7042 100644 --- a/tests/fixtures/saas/connection_template_fixtures.py +++ b/tests/fixtures/saas/connection_template_fixtures.py @@ -72,7 +72,7 @@ def tertiary_mailchimp_instance(db): connection_config.delete(db) -def instantiate_mailchimp(db, key, fides_key) -> tuple[ConnectionConfig, DatasetConfig]: +def instantiate_mailchimp(db, fides_key) -> tuple[ConnectionConfig, DatasetConfig]: secrets = { "domain": "test_mailchimp_domain", "username": "test_mailchimp_username", diff --git a/tests/ops/api/v1/endpoints/test_system.py b/tests/ops/api/v1/endpoints/test_system.py index d9d81d0dfd7..a27fed9f313 100644 --- a/tests/ops/api/v1/endpoints/test_system.py +++ b/tests/ops/api/v1/endpoints/test_system.py @@ -298,15 +298,13 @@ def test_connection_config_key_already_exists( ): auth_header = generate_auth_header(scopes=[SAAS_CONNECTION_INSTANTIATE]) request_body = { - "instance_key": "secondary_mailchimp_instance", + "instance_key": connection_config.key, "secrets": { "domain": "test_mailchimp_domain", "username": "test_mailchimp_username", "api_key": "test_mailchimp_api_key", }, - "name": connection_config.name, "description": "Mailchimp ConnectionConfig description", - "key": connection_config.key, } resp = api_client.post( base_url.format(saas_connector_type="mailchimp"), @@ -332,25 +330,23 @@ def test_connection_config_name_already_exists( }, "name": connection_config.name, "description": "Mailchimp ConnectionConfig description", - "key": "brand_new_key", } resp = api_client.post( base_url.format(saas_connector_type="mailchimp"), headers=auth_header, json=request_body, ) - assert resp.status_code == 400 - assert ( - f"Name {connection_config.name} already exists in ConnectionConfig" - in resp.json()["detail"] - ) + # names don't have to be unique + assert resp.status_code == 200 + def test_create_connection_from_template_without_supplying_connection_key( self, db, generate_auth_header, api_client, base_url ): auth_header = generate_auth_header(scopes=[SAAS_CONNECTION_INSTANTIATE]) + instance_key = "secondary_mailchimp_instance" request_body = { - "instance_key": "secondary_mailchimp_instance", + "instance_key": instance_key, "secrets": { "domain": "test_mailchimp_domain", "username": "test_mailchimp_username", @@ -377,7 +373,7 @@ def test_create_connection_from_template_without_supplying_connection_key( assert connection_config is not None assert dataset_config is not None - assert connection_config.key == "mailchimp_connector" + assert connection_config.key == instance_key dataset_config.delete(db) connection_config.delete(db) From 44b6dfc96a64deac102e6eabbd9c3745fd5d4424 Mon Sep 17 00:00:00 2001 From: Andrew Jackson Date: Tue, 27 Jun 2023 16:33:19 -0400 Subject: [PATCH 08/12] Fix test failures --- .../endpoints/test_connection_template_endpoints.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/ops/api/v1/endpoints/test_connection_template_endpoints.py b/tests/ops/api/v1/endpoints/test_connection_template_endpoints.py index 225d3050642..6efcffc3821 100644 --- a/tests/ops/api/v1/endpoints/test_connection_template_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_connection_template_endpoints.py @@ -1143,18 +1143,17 @@ def test_connection_config_name_already_exists( headers=auth_header, json=request_body, ) - assert resp.status_code == 400 - assert ( - f"Name {connection_config.name} already exists in ConnectionConfig" - in resp.json()["detail"] - ) + # names don't have to be unique + assert resp.status_code == 200 + def test_create_connection_from_template_without_supplying_connection_key( self, db, generate_auth_header, api_client, base_url ): auth_header = generate_auth_header(scopes=[SAAS_CONNECTION_INSTANTIATE]) + instance_key = "secondary_mailchimp_instance" request_body = { - "instance_key": "secondary_mailchimp_instance", + "instance_key": instance_key, "secrets": { "domain": "test_mailchimp_domain", "username": "test_mailchimp_username", @@ -1181,7 +1180,7 @@ def test_create_connection_from_template_without_supplying_connection_key( assert connection_config is not None assert dataset_config is not None - assert connection_config.key == "mailchimp_connector" + assert connection_config.key == instance_key dataset_config.delete(db) connection_config.delete(db) From 676992487f4f9cffaf9daf1cbfd968146962dafc Mon Sep 17 00:00:00 2001 From: Andrew Jackson Date: Tue, 27 Jun 2023 16:37:10 -0400 Subject: [PATCH 09/12] Remove fields --- .../datastore-connection.slice.ts | 2 +- .../forms/ConnectorParameters.tsx | 6 +-- .../forms/ConnectorParametersForm.tsx | 53 ------------------- 3 files changed, 3 insertions(+), 58 deletions(-) diff --git a/clients/admin-ui/src/features/datastore-connections/datastore-connection.slice.ts b/clients/admin-ui/src/features/datastore-connections/datastore-connection.slice.ts index 905d62720d6..6a936ae2bb2 100644 --- a/clients/admin-ui/src/features/datastore-connections/datastore-connection.slice.ts +++ b/clients/admin-ui/src/features/datastore-connections/datastore-connection.slice.ts @@ -89,7 +89,7 @@ const initialState: DatastoreConnectionParams = { }; export type CreateSaasConnectionConfig = { - connectionConfig: CreateSaasConnectionConfigRequest; + connectionConfig: Omit; systemFidesKey: string; }; diff --git a/clients/admin-ui/src/features/datastore-connections/system_portal_config/forms/ConnectorParameters.tsx b/clients/admin-ui/src/features/datastore-connections/system_portal_config/forms/ConnectorParameters.tsx index f98122d5f23..bb97031ed82 100644 --- a/clients/admin-ui/src/features/datastore-connections/system_portal_config/forms/ConnectorParameters.tsx +++ b/clients/admin-ui/src/features/datastore-connections/system_portal_config/forms/ConnectorParameters.tsx @@ -53,9 +53,8 @@ const createSaasConnector = async ( systemFidesKey: string, createSaasConnectorFunc: any ) => { - const connectionConfig: CreateSaasConnectionConfigRequest = { + const connectionConfig: Omit = { description: values.description, - name: values.name, instance_key: formatKey(values.instance_key as string), saas_connector_type: connectionOption.identifier, secrets: {}, @@ -91,7 +90,7 @@ export const patchConnectionConfig = async ( ? formatKey(values.instance_key as string) : connectionConfig?.key; - const params1: Omit = { + const params1: Omit = { access: AccessLevel.WRITE, connection_type: (connectionOption.type === SystemType.SAAS ? connectionOption.type @@ -99,7 +98,6 @@ export const patchConnectionConfig = async ( description: values.description, disabled: false, key, - name: values.name, }; const payload = await patchFunc({ systemFidesKey, diff --git a/clients/admin-ui/src/features/datastore-connections/system_portal_config/forms/ConnectorParametersForm.tsx b/clients/admin-ui/src/features/datastore-connections/system_portal_config/forms/ConnectorParametersForm.tsx index 6db16340072..49a89cdbf25 100644 --- a/clients/admin-ui/src/features/datastore-connections/system_portal_config/forms/ConnectorParametersForm.tsx +++ b/clients/admin-ui/src/features/datastore-connections/system_portal_config/forms/ConnectorParametersForm.tsx @@ -267,59 +267,6 @@ const ConnectorParametersForm: React.FC = ({ {(props: FormikProps) => (
- validateField("Name", value)} - > - {({ field }: { field: FieldInputProps }) => ( - - {getFormLabel("name", "Name")} - - - {props.errors.name} - - - - - - )} - - {/* Description */} - - {({ field }: { field: FieldInputProps }) => ( - - {getFormLabel("description", "Description")} -