From 5fb687c9c27390b3b93809dcb526c0eb10fb158d Mon Sep 17 00:00:00 2001 From: bruno Date: Wed, 27 Nov 2024 17:30:54 -0300 Subject: [PATCH 1/6] Adding check for client credential authorization strategy --- src/fides/api/models/connectionconfig.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/fides/api/models/connectionconfig.py b/src/fides/api/models/connectionconfig.py index 0859c516b2..a246bb707d 100644 --- a/src/fides/api/models/connectionconfig.py +++ b/src/fides/api/models/connectionconfig.py @@ -220,10 +220,11 @@ def authorized(self) -> bool: return False # hard-coding to avoid cyclic dependency - if authentication.strategy != "oauth2_authorization_code": + #TODO: Check what do they mean by cyclic dependency + if authentication.strategy not in ["oauth2_authorization_code", "oauth2_client_credentials"]: return False - return bool(self.secrets and self.secrets.get("access_token")) + return bool(self.secrets and 'access_token' in self.secrets.keys()) @property def name_or_key(self) -> str: From 61db350a23d7662fec1cebe0d4c240aa2990e063 Mon Sep 17 00:00:00 2001 From: bruno Date: Wed, 27 Nov 2024 17:31:31 -0300 Subject: [PATCH 2/6] Adding Logs to record behaviour --- src/fides/api/api/v1/endpoints/system.py | 11 ++++++++++- src/fides/api/util/connection_util.py | 4 ++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/fides/api/api/v1/endpoints/system.py b/src/fides/api/api/v1/endpoints/system.py index 266f23e1c5..d5dfcbeab2 100644 --- a/src/fides/api/api/v1/endpoints/system.py +++ b/src/fides/api/api/v1/endpoints/system.py @@ -140,6 +140,8 @@ def patch_connections( Otherwise, a new ConnectionConfiguration will be created for you. """ system = get_system(db, fides_key) + logger.info("Patching connection configs for system '{}'", system.fides_key) + logger.debug("Connection configs: {}", configs[0].secrets) return patch_connection_configs(db, configs, system) @@ -172,6 +174,9 @@ def patch_connection_secrets( connection_config: ConnectionConfig = get_connection_config_or_error( db, system.connection_configs.key ) + logger.info(f"Oauth Log: For Key '{connection_config.key}'. secrets: {connection_config.secrets}") + logger.info(f"Oauth Log: unvalidated_secrets: {unvalidated_secrets}") + # Inserts unchanged sensitive values. The FE does not send masked values sensitive secrets. if connection_config.secrets is not None: for key, value in connection_config.secrets.items(): @@ -187,16 +192,20 @@ def patch_connection_secrets( db, unvalidated_secrets, connection_config ).model_dump(mode="json") + logger.info(f"Oauth Log: validated_secrets: {validated_secrets}") + + for key, value in validated_secrets.items(): connection_config.secrets[key] = value # type: ignore # Deauthorize an OAuth connection when the secrets are updated. This is necessary because # the existing access tokens may not be valid anymore. This only applies to SaaS connection - # configurations that use the "oauth2_authorization_code" authentication strategy. + # configurations that uses an Oauth authentication strategy. if ( connection_config.authorized and connection_config.connection_type == ConnectionType.saas ): + logger.info(f"Removing access Token {connection_config.secrets['access_token']}") del connection_config.secrets["access_token"] # Save validated secrets, regardless of whether they've been verified. diff --git a/src/fides/api/util/connection_util.py b/src/fides/api/util/connection_util.py index caf4b986f7..7acf4f0d55 100644 --- a/src/fides/api/util/connection_util.py +++ b/src/fides/api/util/connection_util.py @@ -172,6 +172,7 @@ def patch_connection_configs( if config.connection_type == "saas": if config.secrets: + logger.info("Oauth Log: Registered New Config Secrets") # This is here rather than with the get_connection_config_or_error because # it will also throw an HTTPException if validation fails and we don't want # to catch it in this case. @@ -180,6 +181,8 @@ def patch_connection_configs( db, config.secrets, existing_connection_config ) else: + # Refactor idea: Transform this into a function for readability + logger.info("Oauth Log: Into Non existing connection config branch") if not config.saas_connector_type: raise HTTPException( status_code=HTTP_422_UNPROCESSABLE_ENTITY, @@ -252,6 +255,7 @@ def patch_connection_configs( config_dict = config.model_dump(serialize_as_any=True, exclude_unset=True) config_dict.pop("saas_connector_type", None) + if existing_connection_config: config_dict = { key: value From e83a24ea820992a41d2887b15a641bb7f1ed3849 Mon Sep 17 00:00:00 2001 From: bruno Date: Thu, 5 Dec 2024 15:00:44 -0300 Subject: [PATCH 3/6] Revert "Adding Logs to record behaviour" This reverts commit 61db350a23d7662fec1cebe0d4c240aa2990e063. --- src/fides/api/api/v1/endpoints/system.py | 11 +---------- src/fides/api/util/connection_util.py | 4 ---- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/fides/api/api/v1/endpoints/system.py b/src/fides/api/api/v1/endpoints/system.py index d5dfcbeab2..266f23e1c5 100644 --- a/src/fides/api/api/v1/endpoints/system.py +++ b/src/fides/api/api/v1/endpoints/system.py @@ -140,8 +140,6 @@ def patch_connections( Otherwise, a new ConnectionConfiguration will be created for you. """ system = get_system(db, fides_key) - logger.info("Patching connection configs for system '{}'", system.fides_key) - logger.debug("Connection configs: {}", configs[0].secrets) return patch_connection_configs(db, configs, system) @@ -174,9 +172,6 @@ def patch_connection_secrets( connection_config: ConnectionConfig = get_connection_config_or_error( db, system.connection_configs.key ) - logger.info(f"Oauth Log: For Key '{connection_config.key}'. secrets: {connection_config.secrets}") - logger.info(f"Oauth Log: unvalidated_secrets: {unvalidated_secrets}") - # Inserts unchanged sensitive values. The FE does not send masked values sensitive secrets. if connection_config.secrets is not None: for key, value in connection_config.secrets.items(): @@ -192,20 +187,16 @@ def patch_connection_secrets( db, unvalidated_secrets, connection_config ).model_dump(mode="json") - logger.info(f"Oauth Log: validated_secrets: {validated_secrets}") - - for key, value in validated_secrets.items(): connection_config.secrets[key] = value # type: ignore # Deauthorize an OAuth connection when the secrets are updated. This is necessary because # the existing access tokens may not be valid anymore. This only applies to SaaS connection - # configurations that uses an Oauth authentication strategy. + # configurations that use the "oauth2_authorization_code" authentication strategy. if ( connection_config.authorized and connection_config.connection_type == ConnectionType.saas ): - logger.info(f"Removing access Token {connection_config.secrets['access_token']}") del connection_config.secrets["access_token"] # Save validated secrets, regardless of whether they've been verified. diff --git a/src/fides/api/util/connection_util.py b/src/fides/api/util/connection_util.py index 7acf4f0d55..caf4b986f7 100644 --- a/src/fides/api/util/connection_util.py +++ b/src/fides/api/util/connection_util.py @@ -172,7 +172,6 @@ def patch_connection_configs( if config.connection_type == "saas": if config.secrets: - logger.info("Oauth Log: Registered New Config Secrets") # This is here rather than with the get_connection_config_or_error because # it will also throw an HTTPException if validation fails and we don't want # to catch it in this case. @@ -181,8 +180,6 @@ def patch_connection_configs( db, config.secrets, existing_connection_config ) else: - # Refactor idea: Transform this into a function for readability - logger.info("Oauth Log: Into Non existing connection config branch") if not config.saas_connector_type: raise HTTPException( status_code=HTTP_422_UNPROCESSABLE_ENTITY, @@ -255,7 +252,6 @@ def patch_connection_configs( config_dict = config.model_dump(serialize_as_any=True, exclude_unset=True) config_dict.pop("saas_connector_type", None) - if existing_connection_config: config_dict = { key: value From 36a02af72585cff7bd47aa7b2ef160244dcb2d1a Mon Sep 17 00:00:00 2001 From: bruno Date: Thu, 5 Dec 2024 15:01:21 -0300 Subject: [PATCH 4/6] Removing unnecesary comment --- src/fides/api/models/connectionconfig.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/fides/api/models/connectionconfig.py b/src/fides/api/models/connectionconfig.py index a246bb707d..1758222b88 100644 --- a/src/fides/api/models/connectionconfig.py +++ b/src/fides/api/models/connectionconfig.py @@ -220,7 +220,6 @@ def authorized(self) -> bool: return False # hard-coding to avoid cyclic dependency - #TODO: Check what do they mean by cyclic dependency if authentication.strategy not in ["oauth2_authorization_code", "oauth2_client_credentials"]: return False From 773f75e4f5a689dba3b2d3e9e7280beec68c04a3 Mon Sep 17 00:00:00 2001 From: bruno Date: Thu, 5 Dec 2024 15:08:13 -0300 Subject: [PATCH 5/6] Updated changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a29e3703b..4b63425662 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,8 @@ The types of changes are: ## [Unreleased](https://github.com/ethyca/fides/compare/2.51.0...main) - +### Fixed +- saas connections based on `oauth_client_credentials` are properly updating their access token when we edit their secrets. ## [2.51.0](https://github.com/ethyca/fides/compare/2.50.0...2.51.0) From 5ca7eb0d763e08923acda332e9d11b207ad01eaf Mon Sep 17 00:00:00 2001 From: Bruno Gutierrez Rios Date: Fri, 6 Dec 2024 16:21:49 -0300 Subject: [PATCH 6/6] Update CHANGELOG.md Co-authored-by: Adrian Galvan --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b63425662..a2db829ae0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ The types of changes are: ### Fixed -- saas connections based on `oauth_client_credentials` are properly updating their access token when we edit their secrets. +- SaaS integrations using `oauth_client_credentials` now properly update their access token when editing the secrets. ## [2.51.0](https://github.com/ethyca/fides/compare/2.50.0...2.51.0)