✨ Source Facebook-Marketing: Add Selectable Auth #37625

JohnMikkelsonOvative · 2024-04-26T15:07:10Z

What

The Source Facebook Marketing Connector does not allow for choosing between providing an access token or using oauth, it only allows for users to define a source connection by following an oauth flow. This does not allow for use-cases in which a large organization may provision an access token to be used to authenticate to the Facebook Marketing data, rather than giving a user's personal Facebook account access.

How

The spec defines a credentials field that is a union between newly created OAuthCredentials and ServiceAccountCredentials. This allows for the user to select between using the Oauth flow and inserting Service Account Credentials.

Review guide

spec.py
config.py
source.py
sample_config.json
metadata.yaml
pyproject.toml
request_builder.py
test_ads_insights_action_product_id.py
test_videos.py
test_source.py

User Impact

The user is able to select between using the Oauth flow and inserting Service Account Credentials when setting up the Facebook Marketing Source.

Can this PR be safely reverted and rolled back?

YES 💚
NO ❌

…anges to solve build issues, and changes to solve unit testing issues.

…h 11 warnings

…e config. Updated version to reflect Airbyte versioning standard (this use case is a major version upgrade due to configuration of credentials needed to be changed)

…ebook-marketing-selectable-auth

…ebook-marketing-selectable-auth' into JohnMikkelsonOvative/feature-facebook-marketing-selectable-auth

vercel · 2024-04-26T15:07:15Z

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

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 27, 2024 0:42am

CLAassistant · 2024-04-26T15:07:17Z

All committers have signed the CLA.

JohnMikkelsonOvative · 2024-04-26T17:22:38Z

During the migration to credentials selectable configuration, the decision was made to change the naming of ACCESS_TOKEN to SERVICE_ACCOUNT_INFO. This was due to the same naming convention being used in the Source-Google-Drive. Does this make sense to the Airbyte team, or should we change that naming convention to something more agnostic?

…eting-selectable-auth

marcosmarxm

Thanks for the contribution @JohnMikkelsonOvative. It was added to the Connector team backlog and it will be reviewed in future sprint. I'll check with the project manager a timeline to make aware, ask your patience until the moment of the review. As this impact in a critical connector the review can take some time and have some back and forward, the goal is to introduce new features with the lower impact possible. In any time, feel free to reach me out in Slack or ping here to get update about the review process.

marcosmarxm · 2024-04-30T14:23:31Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py

@@ -95,7 +95,10 @@ def check_connection(self, logger: logging.Logger, config: Mapping[str, Any]) ->
            if config.start_date and config.end_date < config.start_date:
                return False, "End date must be equal or after start date."

-            api = API(access_token=config.access_token, page_size=config.page_size)
+            if config.credentials.auth_type == 'Client':


Just small suggestion is to use maybe Enum class to represent the auth type value.

bazarnov · 2024-05-08T11:34:31Z

airbyte-integrations/connectors/source-facebook-marketing/sample_files/sample_config.json

@@ -2,5 +2,8 @@
  "start_date": "2020-09-25T00:00:00Z",
  "end_date": "2021-01-01T00:00:00Z",
  "account_id": "<YOUR_ACCOUNT_ID_HERE>",
-  "access_token": "<YOUR_TOKEN_HERE>"
+  "credentials": {


This change will be a breaking change for all of the existing customers.

We should use the config migration to allow the existing Customers to continue using the existing credentials rather than forcing everyone to upgrade.

This will also touch the way the OAuth2.0 works now, because it returns the obtained credentials to the root of the config, here, thus this method should be removed from the flow because the default method returns the obtained access_token/refresh_token under the credentials object, which is exactly what the new flow will require.

bazarnov · 2024-05-08T11:35:50Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py

                oauth_config_specification=OAuthConfigSpecification(
                    complete_oauth_output_specification={
                        "type": "object",
                        "properties": {
                            "access_token": {
                                "type": "string",
-                                "path_in_connector_config": ["access_token"],
+                                "path_in_connector_config": ["credentials", "access_token"],


There is no backward compatibility for the existing Customers, we should be able to handle the access_token from the root level of the config, alongside the new credentials.access_token path.

bazarnov · 2024-05-08T12:04:16Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/source.py

@@ -95,7 +95,10 @@ def check_connection(self, logger: logging.Logger, config: Mapping[str, Any]) ->
            if config.start_date and config.end_date < config.start_date:
                return False, "End date must be equal or after start date."

-            api = API(access_token=config.access_token, page_size=config.page_size)
+            if config.credentials.auth_type == 'Client':
+                api = API(access_token=config.credentials.refresh_token, page_size=config.page_size)


Why the refresh_token is addressed here? Where we use it in the code?

The OAuth flow, will return the access_token only.
Based on this line of code

bazarnov · 2024-05-08T12:11:54Z

airbyte-integrations/connectors/source-facebook-marketing/unit_tests/integration/config.py

@@ -25,7 +25,10 @@ class ConfigBuilder:
    def __init__(self) -> None:
        self._config: MutableMapping[str, Any] = {
            "account_ids": [ACCOUNT_ID],
-            "access_token": ACCESS_TOKEN,
+            "credentials": {


There is no need to replace the tests, but extend them with the new functionality, using parametrized test case. With this changes, we loose the backward compatibility tests for the existing Customers.

bazarnov · 2024-05-08T12:14:06Z

airbyte-integrations/connectors/source-facebook-marketing/source_facebook_marketing/spec.py

+        description="Client Secret for the Facebook Marketing API",
+        airbyte_secret=True,
+    )
+    refresh_token: str = Field(


The OAuth2.0 Flow doesn't return the refresh_token, could you please refer to the official doc, where this is possible? Thank you.

bazarnov

Requesting changes, because this change will break the existing Customers.

The path should look like this:

Support both old and new way of authentication to cover the existing Customers
Write the test cases for both scenarios
Make it possible to migrate existing customers to use the new way of authentication by applying the config migration, see the example in the /config_migrations.py
Release this update (at this point, all existing Customers are covered with the config_migration, this will allow to win the time, before we update the OAuth Flow for the platform side, here by removing this method.
Done; the platform will return the access_token back to the credentials.access_token as expected after point 4 is released, which means we are good to go with the new authentication way of things.

maxi297 · 2024-05-08T12:29:57Z

@bazarnov For my personal knowledge, what does removing getDefaultOAuthOutputPath do? It is also deprecated in the interface which seems like we should move away from that

bazarnov · 2024-05-08T12:31:47Z

@bazarnov For my personal knowledge, what does removing getDefaultOAuthOutputPath do? It is also deprecated in the interface which seems like we should move away from that

You've looked to the wrong place, see this
Which is inherited from this

bazarnov · 2024-05-08T12:33:03Z

We can safely move away from using this method, once the new way is released and the existing Customers are covered with the config_migration prior to the platform change.

maxi297 · 2024-05-08T14:45:17Z

Note that this method is in FacebookOAuthFlow and this will affect Instagram and Facebook Pages as well. Is this an issue? We can probably re-implement this in the specific classes for those connectors

bazarnov · 2024-05-08T15:00:40Z

Note that this method is in FacebookOAuthFlow and this will affect Instagram and Facebook Pages as well. Is this an issue? We can probably re-implement this in the specific classes for those connectors

We can add the existing method from the base class to all of the other Facebook-related OAuth flows to keep them running as is for now, then create the follow-up issues, to modify each of them. In this way, the Instagram and Facebook Pages, will not be affected with the platform change.

bazarnov · 2024-05-27T11:16:08Z

@cmm-airbyte Could you please fix the conflicts and ensure we pass the CAT successfully before this is finally approved? Thanks.

UPDATE: dismiss this comment, because I missed the fact we work on this within another PR: #38304

JohnMikkelsonOvative and others added 5 commits April 24, 2024 16:21

EMRGE-1446 Selectable Auth working on local CLI. Still need to add ch…

eccc2a1

…anges to solve build issues, and changes to solve unit testing issues.

EMRGE-1446 Unit Tests for Selectable Auth all passing, 205 passed wit…

cda978e

…h 11 warnings

EMRGE-1446 Changed naming to reflect Facebook standard, updated sampl…

ccd385a

…e config. Updated version to reflect Airbyte versioning standard (this use case is a major version upgrade due to configuration of credentials needed to be changed)

Merge branch 'airbytehq:master' into JohnMikkelsonOvative/feature-fac…

4079845

…ebook-marketing-selectable-auth

Merge remote-tracking branch 'origin/JohnMikkelsonOvative/feature-fac…

378f275

…ebook-marketing-selectable-auth' into JohnMikkelsonOvative/feature-facebook-marketing-selectable-auth

JohnMikkelsonOvative had a problem deploying to community-ci April 26, 2024 15:07 — with GitHub Actions Error

octavia-squidington-iii added area/connectors Connector related issues community connectors/source/facebook-marketing labels Apr 26, 2024

vercel bot deployed to Preview April 26, 2024 15:11 View deployment

Merge branch 'master' into JohnMikkelsonOvative/feature-facebook-mark…

a5f4017

…eting-selectable-auth

JohnMikkelsonOvative had a problem deploying to community-ci April 26, 2024 18:32 — with GitHub Actions Error

JohnMikkelsonOvative marked this pull request as ready for review April 26, 2024 18:32

vercel bot deployed to Preview April 26, 2024 18:36 View deployment

katmarkham added the team/critical-connectors label Apr 26, 2024

Merge branch 'master' into JohnMikkelsonOvative/feature-facebook-mark…

7939c8a

…eting-selectable-auth

marcosmarxm had a problem deploying to community-ci April 27, 2024 12:38 — with GitHub Actions Failure

vercel bot deployed to Preview April 27, 2024 12:42 View deployment

marcosmarxm requested changes Apr 30, 2024

View reviewed changes

cmm-airbyte assigned cmm-airbyte and JohnMikkelsonOvative and unassigned cmm-airbyte Apr 30, 2024

cmm-airbyte self-requested a review May 3, 2024 16:42

bazarnov reviewed May 8, 2024

View reviewed changes

bazarnov requested changes May 8, 2024

View reviewed changes

cmm-airbyte mentioned this pull request May 21, 2024

✨ Source Facebook-Marketing: Add Selectable Auth (with migration & tests) #38304

Merged

2 tasks

maxi297 closed this Jun 13, 2024

RocketerJames mentioned this pull request Jun 28, 2024

[source-facebook-marketing] Unable to parse access token #40609

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Source Facebook-Marketing: Add Selectable Auth #37625

✨ Source Facebook-Marketing: Add Selectable Auth #37625

JohnMikkelsonOvative commented Apr 26, 2024 •

edited

Loading

vercel bot commented Apr 26, 2024 •

edited

Loading

CLAassistant commented Apr 26, 2024 •

edited

Loading

JohnMikkelsonOvative commented Apr 26, 2024

marcosmarxm left a comment

marcosmarxm Apr 30, 2024

bazarnov May 8, 2024

bazarnov May 8, 2024

bazarnov May 8, 2024

bazarnov May 8, 2024

bazarnov May 8, 2024

bazarnov May 8, 2024

bazarnov left a comment

maxi297 commented May 8, 2024

bazarnov commented May 8, 2024

bazarnov commented May 8, 2024

maxi297 commented May 8, 2024

bazarnov commented May 8, 2024

bazarnov commented May 27, 2024 •

edited

Loading

✨ Source Facebook-Marketing: Add Selectable Auth #37625

✨ Source Facebook-Marketing: Add Selectable Auth #37625

Conversation

JohnMikkelsonOvative commented Apr 26, 2024 • edited Loading

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

vercel bot commented Apr 26, 2024 • edited Loading

CLAassistant commented Apr 26, 2024 • edited Loading

JohnMikkelsonOvative commented Apr 26, 2024

marcosmarxm left a comment

Choose a reason for hiding this comment

marcosmarxm Apr 30, 2024

Choose a reason for hiding this comment

bazarnov May 8, 2024

Choose a reason for hiding this comment

bazarnov May 8, 2024

Choose a reason for hiding this comment

bazarnov May 8, 2024

Choose a reason for hiding this comment

bazarnov May 8, 2024

Choose a reason for hiding this comment

bazarnov May 8, 2024

Choose a reason for hiding this comment

bazarnov May 8, 2024

Choose a reason for hiding this comment

bazarnov left a comment

Choose a reason for hiding this comment

maxi297 commented May 8, 2024

bazarnov commented May 8, 2024

bazarnov commented May 8, 2024

maxi297 commented May 8, 2024

bazarnov commented May 8, 2024

bazarnov commented May 27, 2024 • edited Loading

JohnMikkelsonOvative commented Apr 26, 2024 •

edited

Loading

vercel bot commented Apr 26, 2024 •

edited

Loading

CLAassistant commented Apr 26, 2024 •

edited

Loading

bazarnov commented May 27, 2024 •

edited

Loading