Implement BYOC Oauth#220
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the tap’s Salesforce authentication from refresh-token (auth code) OAuth to OAuth2 client_credentials (“BYOC OAuth”), updating configuration, docs, and tests to align with the new flow.
Changes:
- Replace refresh-token login (and sandbox redirect support) with client_credentials login using a customer-provided
instance_url. - Update tap-tester/integration and unit tests to remove
refresh_token/is_sandboxand validate the new login behavior. - Bump version to 3.0.0 and update README + CHANGELOG for the breaking auth change.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/test_refresh_token_rotation.py | Removes refresh-token rotation tests (no longer applicable to client_credentials). |
| tests/unittests/test_multiline_critical_error_message.py | Updates test config to use instance_url and remove refresh-token/sandbox fields. |
| tests/unittests/test_client_credentials_login.py | Adds new unit tests for client_credentials login (success, payload, URL handling, retry behavior). |
| tests/test_salesforce_sync_canary.py | Updates test properties to use instance_url only (drops is_sandbox). |
| tests/test_salesforce_lookback_window.py | Updates test properties to use instance_url only (drops is_sandbox). |
| tests/sfbase.py | Updates tap-tester base expectations/type and credentials env vars for BYOC. |
| tests/base.py | Updates tap-tester base expectations/type and credentials env vars for BYOC. |
| tap_salesforce/salesforce/init.py | Implements client_credentials token request; removes refresh-token rotation logic and stops logging POST bodies. |
| tap_salesforce/init.py | Removes refresh_token config usage and passes instance_url into Salesforce. |
| setup.py | Bumps package version to 3.0.0. |
| README.md | Documents new config format (instance_url) and new auth setup guidance. |
| CHANGELOG.md | Adds 3.0.0 entry documenting the auth migration. |
Comments suppressed due to low confidence (1)
tap_salesforce/init.py:31
- The default
CONFIGdict doesn't includeinstance_url, even though later code expects it to be present (CONFIG['instance_url']). Adding the key here improves clarity and avoids surprising KeyErrors if code paths accessCONFIGbeforeargs.configis merged.
CONFIG = {
'client_id': None,
'client_secret': None,
'start_date': None
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| REQUIRED_CONFIG_KEYS = ['client_id', | ||
| 'client_secret', | ||
| 'start_date', | ||
| 'api_type'] | ||
|
|
There was a problem hiding this comment.
This is a valid review comment. Please address.
| resp = self._make_request("POST", login_url, body=login_body, headers={"Content-Type": "application/x-www-form-urlencoded"}) | ||
|
|
||
| LOGGER.info("OAuth2 login successful") | ||
|
|
||
| auth = resp.json() | ||
|
|
||
| self.access_token = auth['access_token'] | ||
| self.instance_url = auth['instance_url'] | ||
| # Salesforce may or may not return a new refresh token. If it does, | ||
| # we should update to use the new one. | ||
| new_refresh_token = auth.get('refresh_token') | ||
| if new_refresh_token and new_refresh_token != self.refresh_token: | ||
| LOGGER.info("Refresh token rotation detected. Updating refresh token.") | ||
| self.refresh_token = new_refresh_token | ||
| self._write_config() | ||
| else: | ||
| LOGGER.info("No refresh token rotation detected.") | ||
| except Exception as e: |
There was a problem hiding this comment.
As per this,
When an access token expires, attempts to use it fail. The client must obtain a new access token by using a refresh token or reinitiating the authorization flow.
This seems like valid review, please address it.
There was a problem hiding this comment.
Added back logic to generate token periodically
| login_url = 'https://test.salesforce.com/services/oauth2/token' | ||
| else: | ||
| login_url = 'https://login.salesforce.com/services/oauth2/token' | ||
| login_url = '{}/services/oauth2/token'.format(self.instance_url.rstrip('/')) |
There was a problem hiding this comment.
In UI, we have marked this as require field with uri datatype. So, no chance of empty value.
There was a problem hiding this comment.
This is a good suggestion. Please incorporate it.
There was a problem hiding this comment.
We are addressing None property issue by making it required but there are other security issues here.
SSRF / arbitrary outbound request risk:
Since instance_url comes from config/customer input, a malicious or mistaken value like:
would cause the tap to POST client credentials to:
- /services/oauth2/token
That is undesirable both because it can hit internal services and because it can leak client_id / client_secret.
Description of change
Replace existing OAuth with Client Credential OAuth.
QA steps
Risks
Rollback steps
AI generated code
https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code