-
Notifications
You must be signed in to change notification settings - Fork 71
Implement BYOC Oauth #220
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
base: master
Are you sure you want to change the base?
Implement BYOC Oauth #220
Changes from all commits
b98781c
7bd1d1e
4895762
83a206c
0123f2a
9c81a91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,14 +19,13 @@ | |
|
|
||
| LOGGER = singer.get_logger() | ||
|
|
||
| REQUIRED_CONFIG_KEYS = ['refresh_token', | ||
| 'client_id', | ||
| REQUIRED_CONFIG_KEYS = ['client_id', | ||
| 'client_secret', | ||
| 'start_date', | ||
| 'api_type'] | ||
| 'api_type', | ||
| 'instance_url'] | ||
|
|
||
|
Comment on lines
+22
to
27
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a valid review comment. Please address.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| CONFIG = { | ||
| 'refresh_token': None, | ||
| 'client_id': None, | ||
| 'client_secret': None, | ||
| 'start_date': None | ||
|
|
@@ -403,17 +402,16 @@ def main_impl(): | |
| lookback_window = int(lookback_window) if lookback_window else None | ||
|
|
||
| sf = Salesforce( | ||
| refresh_token=CONFIG['refresh_token'], | ||
| sf_client_id=CONFIG['client_id'], | ||
| sf_client_secret=CONFIG['client_secret'], | ||
| quota_percent_total=CONFIG.get('quota_percent_total'), | ||
| quota_percent_per_run=CONFIG.get('quota_percent_per_run'), | ||
| is_sandbox=CONFIG.get('is_sandbox'), | ||
| select_fields_by_default=CONFIG.get('select_fields_by_default'), | ||
| default_start_date=CONFIG.get('start_date'), | ||
| api_type=CONFIG.get('api_type'), | ||
| lookback_window=lookback_window, | ||
| config_path=args.config_path) | ||
| config_path=args.config_path, | ||
| instance_url=CONFIG['instance_url']) | ||
| sf.login() | ||
|
|
||
| if args.discover: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| # The minimum expiration setting for SF Refresh Tokens is 15 minutes | ||
| REFRESH_TOKEN_EXPIRATION_PERIOD = 900 | ||
|
|
||
|
|
||
| BULK_API_TYPE = "BULK" | ||
| REST_API_TYPE = "REST" | ||
| BATCH_DESCRIBE_SIZE = 25 | ||
|
|
@@ -212,27 +213,25 @@ def field_to_property_schema(field, mdata): # pylint:disable=too-many-branches | |
| class Salesforce(): | ||
| # pylint: disable=too-many-instance-attributes,too-many-arguments,too-many-positional-arguments | ||
| def __init__(self, | ||
| refresh_token=None, | ||
| token=None, | ||
| sf_client_id=None, | ||
| sf_client_secret=None, | ||
| quota_percent_per_run=None, | ||
| quota_percent_total=None, | ||
| is_sandbox=None, | ||
| select_fields_by_default=None, | ||
| default_start_date=None, | ||
| api_type=None, | ||
| lookback_window=None, | ||
| config_path=None): | ||
| config_path=None, | ||
| instance_url=None): | ||
| self.api_type = api_type.upper() if api_type else None | ||
| self.refresh_token = refresh_token | ||
| self.token = token | ||
| self.config_path = config_path | ||
| self.sf_client_id = sf_client_id | ||
| self.sf_client_secret = sf_client_secret | ||
| self.session = requests.Session() | ||
| self.access_token = None | ||
| self.instance_url = None | ||
| self.instance_url = instance_url | ||
| if isinstance(quota_percent_per_run, str) and quota_percent_per_run.strip() == '': | ||
| quota_percent_per_run = None | ||
| if isinstance(quota_percent_total, str) and quota_percent_total.strip() == '': | ||
|
|
@@ -241,7 +240,6 @@ def __init__(self, | |
| quota_percent_per_run) if quota_percent_per_run is not None else 25 | ||
| self.quota_percent_total = float( | ||
| quota_percent_total) if quota_percent_total is not None else 80 | ||
| self.is_sandbox = is_sandbox is True or (isinstance(is_sandbox, str) and is_sandbox.lower() == 'true') | ||
| self.select_fields_by_default = select_fields_by_default is True or (isinstance(select_fields_by_default, str) and select_fields_by_default.lower() == 'true') | ||
| self.default_start_date = default_start_date | ||
| self.rest_requests_attempted = 0 | ||
|
|
@@ -257,15 +255,6 @@ def __init__(self, | |
| def _get_standard_headers(self): | ||
| return {"Authorization": "Bearer {}".format(self.access_token)} | ||
|
|
||
| def _write_config(self): | ||
| """Save updated config (with new token) back to config file.""" | ||
| with open(self.config_path, 'r', encoding='utf-8') as f: | ||
| config = json.load(f) | ||
| config['refresh_token'] = self.refresh_token | ||
| with open(self.config_path, 'w', encoding='utf-8') as f: | ||
| json.dump(config, f, indent=2) | ||
| LOGGER.info("Updated config file with new refresh token.") | ||
|
|
||
| # pylint: disable=anomalous-backslash-in-string,line-too-long | ||
| def check_rest_quota_usage(self, headers): | ||
| match = re.search('^api-usage=(\d+)/(\d+)$', headers.get('Sforce-Limit-Info')) | ||
|
|
@@ -314,7 +303,7 @@ def _make_request(self, http_method, url, headers=None, body=None, stream=False, | |
| params=params, | ||
| timeout=request_timeout,) | ||
| elif http_method == "POST": | ||
| LOGGER.info("Making %s request to %s with body %s", http_method, url, body) | ||
| LOGGER.info("Making %s request to %s", http_method, url) | ||
| resp = self.session.post(url, | ||
| headers=headers, | ||
| data=body, | ||
|
|
@@ -343,35 +332,21 @@ def _make_request(self, http_method, url, headers=None, body=None, stream=False, | |
| return resp | ||
|
|
||
| def login(self): | ||
| if self.is_sandbox: | ||
| 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('/')) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In UI, we have marked this as require field with uri datatype. So, no chance of empty value.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good suggestion. Please incorporate it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
That is undesirable both because it can hit internal services and because it can leak client_id / client_secret. |
||
|
|
||
| login_body = {'grant_type': 'refresh_token', 'client_id': self.sf_client_id, | ||
| 'client_secret': self.sf_client_secret, 'refresh_token': self.refresh_token} | ||
|
|
||
| LOGGER.info("Attempting login via OAuth2") | ||
| login_body = {'grant_type': 'client_credentials', | ||
| 'client_id': self.sf_client_id, | ||
| 'client_secret': self.sf_client_secret} | ||
| LOGGER.info("Attempting login via OAuth2 client_credentials flow") | ||
|
|
||
| resp = None | ||
| try: | ||
| resp = self._make_request("POST", login_url, body=login_body, headers={"Content-Type": "application/x-www-form-urlencoded"}) | ||
|
|
||
| LOGGER.info("OAuth2 login successful") | ||
| LOGGER.info("OAuth2 login successful using Client Credentials flow") | ||
|
|
||
| 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: | ||
|
Comment on lines
344
to
350
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per this,
This seems like valid review, please address it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added back logic to generate token periodically |
||
| error_message = str(e) | ||
| if resp is None and hasattr(e, 'response') and e.response is not None: #pylint:disable=no-member | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.