✨ Source Facebook-Marketing: Add Selectable Auth by JohnMikkelsonOvative · Pull Request #37625 · airbytehq/airbyte

John Mikkelson (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.

John Mikkelson (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

M. Marx (marcosmarxm)

Thanks for the contribution John Mikkelson (@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.

M. Marx (marcosmarxm) · 2024-04-30T14:23:31Z

                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.

Baz (bazarnov) · 2024-05-08T11:34:31Z

  "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.

Baz (bazarnov) · 2024-05-08T11:35:50Z

                            "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.

Baz (bazarnov) · 2024-05-08T12:04:16Z


-            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

Baz (bazarnov) · 2024-05-08T12:11:54Z

        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.

Baz (bazarnov) · 2024-05-08T12:14:06Z

+        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.

Baz (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.

Maxime Carbonneau-Leclerc (maxi297) · 2024-05-08T12:29:57Z

Baz (@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

Baz (bazarnov) · 2024-05-08T12:31:47Z

Baz (@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

Baz (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.

Maxime Carbonneau-Leclerc (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

Baz (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.

Baz (bazarnov) · 2024-05-27T11:16:08Z

Cristina Mariscal (@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

John Mikkelson (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

John Mikkelson (JohnMikkelsonOvative) temporarily deployed to community-ci April 26, 2024 15:07 — with GitHub Actions Inactive

Octavia Squidington III (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

John Mikkelson (JohnMikkelsonOvative) temporarily deployed to community-ci April 26, 2024 18:32 — with GitHub Actions Inactive

John Mikkelson (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

Kat Wilson (katmarkham) added the team/critical-connectors label Apr 26, 2024

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

7939c8a

…eting-selectable-auth

M. Marx (marcosmarxm) temporarily deployed to community-ci April 27, 2024 12:38 — with GitHub Actions Inactive

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

M. Marx (marcosmarxm) suggested changes Apr 30, 2024

View reviewed changes

Cristina Mariscal (cmm-airbyte) assigned Cristina Mariscal (cmm-airbyte) and John Mikkelson (JohnMikkelsonOvative) and unassigned Cristina Mariscal (cmm-airbyte) Apr 30, 2024

Cristina Mariscal (cmm-airbyte) self-requested a review May 3, 2024 16:42

Baz (bazarnov) reviewed May 8, 2024

View reviewed changes

Baz (bazarnov) requested changes May 8, 2024

View reviewed changes

Cristina Mariscal (cmm-airbyte) mentioned this pull request May 21, 2024

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

Merged

2 tasks

Maxime Carbonneau-Leclerc (maxi297) closed this Jun 13, 2024

James (RocketerJames) mentioned this pull request Jun 28, 2024

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

Closed

1 task

Conversation

John Mikkelson (JohnMikkelsonOvative) commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

Uh oh!

vercel Bot commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

John Mikkelson (JohnMikkelsonOvative) commented Apr 26, 2024

Uh oh!

M. Marx (marcosmarxm) left a comment

Choose a reason for hiding this comment

Uh oh!

M. Marx (marcosmarxm) Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

Baz (bazarnov) May 8, 2024

Choose a reason for hiding this comment

Uh oh!

Baz (bazarnov) May 8, 2024

Choose a reason for hiding this comment

Uh oh!

Baz (bazarnov) May 8, 2024

Choose a reason for hiding this comment

Uh oh!

Baz (bazarnov) May 8, 2024

Choose a reason for hiding this comment

Uh oh!

Baz (bazarnov) May 8, 2024

Choose a reason for hiding this comment

Uh oh!

Baz (bazarnov) May 8, 2024

Choose a reason for hiding this comment

Uh oh!

Baz (bazarnov) left a comment

Choose a reason for hiding this comment

Uh oh!

Maxime Carbonneau-Leclerc (maxi297) commented May 8, 2024

Uh oh!

Baz (bazarnov) commented May 8, 2024

Uh oh!

Baz (bazarnov) commented May 8, 2024

Uh oh!

Maxime Carbonneau-Leclerc (maxi297) commented May 8, 2024

Uh oh!

Baz (bazarnov) commented May 8, 2024

Uh oh!

Baz (bazarnov) commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

John Mikkelson (JohnMikkelsonOvative) commented Apr 26, 2024 •

edited

Loading

vercel Bot commented Apr 26, 2024 •

edited

Loading

CLAassistant commented Apr 26, 2024 •

edited

Loading

Baz (bazarnov) commented May 27, 2024 •

edited

Loading