-
Notifications
You must be signed in to change notification settings - Fork 407
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
fix(integrations): update base_url for gong-oauth #1770
Conversation
@hassan254-prog should this still be in draft? |
Robin had suggested I place it in draft just in case there is more work to it. |
@hassan254-prog Will this automatically work for all customers? Will there ever be a situation where a sync will fail because a customer does not yet have |
@rguldener Can we bring this out of drafts? |
Sure! I only suggested draft initially because there might have been more work needed. Feel free to move this forward. |
packages/shared/providers.yaml
Outdated
@@ -753,7 +755,7 @@ gong-oauth: | |||
disable_pkce: true | |||
token_request_auth_method: basic | |||
proxy: | |||
base_url: https://api.gong.io | |||
base_url: ${connectionConfig.api_base_url_for_customer} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned this will break existing connections that don't have the api_base_url_for_customer
field populated in the connection config.
Is there a way to fall back to https://api.gong.io
?
Would something like ${connectionConfig.api_base_url_for_customer || 'https://api.gong.io}
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bastienbeurier , I will do some test on this and see if the platform will handle such case. Incase of any changes to the platform, I will add them to this pull request. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, I checked that no one was using Gong in prod at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it still the case that nobody uses gong, because that's a breaking change. otherwise look good to me
@TBonnin , I don't have much context on this, but the last time we checked with @bastienbeurier , it was okay to merge. Could you please check if the number has changed before merging? Thank you! |
I see 12 accounts using |
I also see production usage for it. @hassan254-prog @TBonnin @khaliqgant Is there a way to have a fallback system to avoid a breaking change? Something like ${connectionConfig.api_base_url_for_customer || 'https://api.gong.io'} |
I don't think we support this syntax in providers.yaml @khaliqgant might be able to confirm. Could we populate the connection config for existing gong-auth connections with the default url? Might require synchronization between doing that and releasing this change though. |
No we don't but the idea was brought up in the past but decided against it to not have too much code like syntax in yaml which might prove difficult to maintain. |
|
GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
---|---|---|---|---|---|
8205899 | Triggered | Generic Password | 2bb094f | docker-compose.yaml | View secret |
8205899 | Triggered | Generic Password | 2bb094f | dev/docker-compose.dev.yaml | View secret |
8205899 | Triggered | Generic Password | 2bb094f | dev/docker-compose.dev.yaml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
@@ -1124,7 +1126,7 @@ gong-oauth: | |||
disable_pkce: true | |||
token_request_auth_method: basic | |||
proxy: | |||
base_url: https://api.gong.io | |||
base_url: ${connectionConfig.api_base_url_for_customer} || https://api.gong.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bodinsamuel unless I missed something this won’t work without #1873?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah wasn't aware of this, thanks 👌🏻
## Describe your changes Continuing from this, #1770 (comment). We need to have a fallback incase `api_base_url_for_customer` field is not populated in the connection configuration. I have also added a unit test for this.. --------- Co-authored-by: Khaliq <[email protected]>
Describe your changes
Gong recently made changes to their API. Gong-oauth now uses
api_base_url_for_customer
, which varies for each customer, as itsbase_url
for proxy requests. This parameter is returned in the response ofgenerate-customer-token
when exchanging code for an access token.Issue ticket number and link
Checklist before requesting a review (skip if just adding/editing APIs & templates)