Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cdp): add missing oauth scopes warning #26738

Merged
merged 17 commits into from
Dec 13, 2024
Merged

Conversation

MarconLP
Copy link
Member

@MarconLP MarconLP commented Dec 6, 2024

Problem

https://posthoghelp.zendesk.com/agent/tickets/21531

Changes

2024-12-06 at 21 11 29

  • add a requiredScopes field to the oauth input
  • saves the authorized scopes of each hubspot integration inside of the config
  • shows a scope missing warning
  • won't show a warning at all if the authorized scopes array is empty

Does this work well for both Cloud and self-hosted?

How did you test this code?

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

  • chromium: 0 added, 4 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

github-actions bot commented Dec 6, 2024

Size Change: 0 B

Total Size: 1.11 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.11 MB

compressed-size-action

@MarconLP MarconLP marked this pull request as ready for review December 9, 2024 09:40
@MarconLP MarconLP requested a review from a team December 9, 2024 12:05
Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this is pretty good, just would love to think a bit bigger than just hubspot - this same issue applies to all of our oauth integrations

posthog/models/integration.py Show resolved Hide resolved
}),
}}
>
Required scopes are missing: [{missingScopes.join(', ')}]. Note that some features may not be available
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like the hubspot part could be omitted and instead just left out as it makes this needlessly unique to hubspot when it could for sure work (and should) for all the other oauth things

Copy link
Member Author

@MarconLP MarconLP Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it could work for multiple oauth integrations, but the way to get the authorized scopes is really different as it seems:

  1. Slack - In the x-oauth-scopes headers from the token info URL.
  2. Hubspot - Under the scopes key from the token info URL.
  3. Google - Under the scope key from the token info URL OR under the scope key from the token URL.

Will look into it.

Copy link
Member Author

@MarconLP MarconLP Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed some changes to generalize the logic. We'll grab authorized tokens from multiple possible locations:
370d97a (#26738)

I've also added requiredScopes to our existing templates with oauth integrations.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@MarconLP MarconLP changed the title feat(cdp): add hubspot scopes missing warning feat(cdp): add missing oauth scopes warning Dec 11, 2024
Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - trusting you've done some decent testing to see that this actually works for slack etc.

frontend/src/lib/integrations/IntegrationScopesWarning.tsx Outdated Show resolved Hide resolved
@MarconLP
Copy link
Member Author

Yes - Verified that it works with all of our current integrations: salesforce, google, slack, & hubspot.
2024-12-12 at 14 04 03

@MarconLP MarconLP enabled auto-merge (squash) December 12, 2024 13:12
@MarconLP MarconLP merged commit 2c92fe2 into master Dec 13, 2024
96 checks passed
@MarconLP MarconLP deleted the hubspot-scopes-warning branch December 13, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants