Adds support for OAuth 2.0 #106
Conversation
authenticating with the IBKR Client Portal API. This allows for fully headless operation without needing manual gateway logins. Key changes: - New `ibind.oauth.oauth2` module with `OAuth2Config` and token management. - Integrated OAuth 2.0 into `IbkrClient`, including automatic SSO bearer token acquisition and session handling. - Added new environment variables for OAuth 2.0 configuration (see README). - Included a new example `examples/rest_09_oauth2.py` demonstrating usage. - Added an end-to-end test script `test/e2e/test_ibind_oauth2.py` (loads credentials from root `.env`). - Updated README.md with information on OAuth 2.0 setup and usage. - Updated pyproject.toml and requirements-dev.txt for python-dotenv.
|
This is great! Thanks so much for putting the time and effort into this! Before I go deep and review the code itself, I'd like to understand how we can gain confidence in this change for IBind. Things I'm curious about:
What do you think? |
|
Hi Wes, Thanks so much for taking a look and for the positive feedback! I'm happy to clarify these points:
Python Code I was sent:
I hope this provides a clearer picture. This implementation has been working reliably for me, and I've aimed to align it closely with existing |
Voyz
left a comment
Voyz
left a comment
There was a problem hiding this comment.
@climbercarmich what a fantastic initiative and contribution 👏 First of all, thank you for putting in the time and writing all this! It's awesome how you've attempted to follow various coding standards already present in the library, and did due diligence on providing thorough comments and updating things where necessary. Honestly, superb job! 🙌
I finished the review - I know it's a lot so please feel free to take your time addressing these points.
One comment that I want to leave here as it applies to the entire change is that I'd change almost all of the _LOGGER.info calls to _LOGGER.debug calls, or get rid of them completely. I think that the info calls should inform the user of something that is noteworthy for them - for example that OAuth2.0 will be used, or that their IP will be automatically deduced - but not of the result of every step along the process if things go nominally.
Also, thanks @weklund for dropping in some great questions. I think I can add 2 cents to question number two:
How we as IBind maintainers plan to throughly test this as not all of us has an IBKR account that can support OAuth use
In all fairness, we don't. OAuth 1.0a was also contributed without me having access to OAuth and it worked out well. This means that we need to count on users doing these verifications for us. This makes the maintenance more difficult, but frankly I think it's the only way to do it, and we just need to accept it. Ideally, I'd have several accounts, some with OAuth some without, but I don't currently have resources to cover so many bases on my own - this may change as we share the maintenance responsibility more.
examples/rest_09_oauth2.py
Outdated
| # --- Add python-dotenv logic to load .env from project root --- | ||
| SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) | ||
| # Project root is one level up from the 'examples' directory | ||
| PROJECT_ROOT = os.path.dirname(SCRIPT_DIR) | ||
| dotenv_path_project_root = os.path.join(PROJECT_ROOT, '.env') | ||
|
|
||
| try: | ||
| from dotenv import load_dotenv | ||
| if os.path.exists(dotenv_path_project_root): | ||
| print(f"Loading environment variables from project root: {dotenv_path_project_root}") | ||
| load_dotenv(dotenv_path=dotenv_path_project_root) | ||
| else: | ||
| print(f".env file not found in project root ({PROJECT_ROOT}). Will rely on system environment or pre-set vars.") | ||
| except ImportError: | ||
| print("python-dotenv library not found. Cannot load .env file. Ensure it is installed (`uv pip install python-dotenv`).") | ||
| # --- End of python-dotenv logic --- |
There was a problem hiding this comment.
Loading in environment variables is straightforward enough that each user can figure it out on their own. Dontenv is one way of doing it, but not the only one, and it does introduce a dependency unnecessarily. Hence, I think we shouldn't suggest using it in this way
examples/rest_09_oauth2.py
Outdated
| # Check if core OAuth2 env vars are present to decide which path to take for the example | ||
| # This is just for the example's logic; in your own code, you'd choose one method. | ||
| if (os.getenv('IBIND_OAUTH2_CLIENT_ID') and | ||
| os.getenv('IBIND_OAUTH2_CLIENT_KEY_ID') and | ||
| os.getenv('IBIND_OAUTH2_PRIVATE_KEY_PEM') and | ||
| os.getenv('IBIND_OAUTH2_USERNAME')): | ||
| print("Found OAuth 2.0 environment variables. Initializing IbkrClient with use_oauth=True.") | ||
| print("Ensure IBIND_USE_OAUTH is also set to True in your environment, or use_oauth=True is passed explicitly.") | ||
| # When using env vars, IbkrClient will internally create an OAuth2Config if it detects | ||
| # that it should be using OAuth2 based on some logic (e.g. if specific OAuth2 vars are present) | ||
| # or if you explicitly pass an OAuth2Config instance. | ||
| # For this example, assuming IBIND_USE_OAUTH=True is set in the environment | ||
| # and IbkrClient correctly picks OAuth2Config based on available env vars. | ||
| # A more explicit way when relying on env vars for OAuth2 would be: | ||
| # client = IbkrClient(cacert=cacert, use_oauth=True, oauth_config=OAuth2Config()) | ||
| # This tells it to definitely use OAuth2Config, which will then pull from env vars. | ||
| client = IbkrClient(cacert=cacert, use_oauth=True, oauth_config=OAuth2Config()) | ||
| print("IbkrClient initialized. Will attempt API calls.") | ||
| else: | ||
| print("Core OAuth 2.0 environment variables not found.") | ||
| print("Please set IBIND_OAUTH2_CLIENT_ID, IBIND_OAUTH2_CLIENT_KEY_ID, IBIND_OAUTH2_PRIVATE_KEY_PEM, and IBIND_OAUTH2_USERNAME.") | ||
| print("Alternatively, configure OAuth2Config dynamically in the script (see example comments).") | ||
| print("Exiting example as configuration is missing.") | ||
| client = None # Ensure client is None if not configured |
There was a problem hiding this comment.
This is a little too verbose. We already specified what env vars are required in the header of this file and we can safely assume the user understood it the first time around. While it definitely gets points for double checking the environment, it does introduce noise and make the example less readable. Also, that if client: check later may be confusing to some. I think the examples should be as minimal as possible, to act as an aid in documentation, even if it is at a cost of some good practices. In other words: the code you've built is absolutely valid and useful, just a little too involved for an example file.
examples/rest_09_oauth2.py
Outdated
| print('\n#### Attempting to tickle (check connection/authentication) ####') | ||
| tickle_result = client.tickle() | ||
| if tickle_result and tickle_result.data: | ||
| print(f'Tickle successful: {tickle_result.data}') | ||
| else: | ||
| print(f'Tickle call did not return expected data or failed. Result: {tickle_result}') | ||
|
|
||
| print('\n\n#### get_accounts ####') | ||
| # Ensure account_id is set if operations require it, though portfolio_accounts might not. | ||
| # client.account_id = 'YourSpecificAccountId' # If needed for other calls | ||
| accounts_result = client.portfolio_accounts() | ||
| if accounts_result and accounts_result.data: | ||
| accounts = accounts_result.data | ||
| print(accounts) | ||
| # Set account_id from the first found account for subsequent calls if needed | ||
| if isinstance(accounts, list) and len(accounts) > 0 and 'accountId' in accounts[0]: | ||
| client.account_id = accounts[0]['accountId'] | ||
| print(f"Set client.account_id to: {client.account_id}") | ||
| else: | ||
| print("Could not determine accountId from portfolio_accounts response.") | ||
| else: | ||
| print(f'get_accounts call did not return expected data or failed. Result: {accounts_result}') | ||
|
|
||
| # Check authentication status if available | ||
| if hasattr(client, 'authentication_status'): | ||
| print('\n\n#### authentication_status ####') | ||
| auth_status_result = client.authentication_status() | ||
| if auth_status_result and auth_status_result.data: | ||
| print(auth_status_result.data) | ||
| else: | ||
| print(f'authentication_status call did not return expected data or failed. Result: {auth_status_result}') | ||
|
|
||
| if client.account_id: # Proceed only if account_id was set | ||
| print('\n\n#### get_ledger ####') | ||
| ledger_result = client.get_ledger(client.account_id) | ||
| if ledger_result and ledger_result.data: | ||
| ledger = ledger_result.data | ||
| for currency, subledger in ledger.items(): | ||
| print(f'\t Ledger currency: {currency}') | ||
| print(f'\t cash balance: {subledger.get("cashbalance", "N/A")}') | ||
| print(f'\t net liquidation value: {subledger.get("netliquidationvalue", "N/A")}') | ||
| print(f'\t stock market value: {subledger.get("stockmarketvalue", "N/A")}') | ||
| print() | ||
| else: | ||
| print(f'get_ledger call did not return expected data or failed. Result: {ledger_result}') | ||
|
|
||
| print('\n#### get_positions ####') | ||
| positions_result = client.positions(client.account_id) | ||
| if positions_result and positions_result.data: | ||
| positions = positions_result.data | ||
| for position in positions: | ||
| print(f'\t Position {position.get("ticker", "N/A")}: {position.get("position", 0)} (${position.get("mktValue", 0.0)})') | ||
| else: | ||
| print(f'get_positions call did not return expected data or failed. Result: {positions_result}') | ||
| else: | ||
| print("\nSkipping Ledger and Positions calls as client.account_id is not set.") | ||
|
|
||
| except Exception as e: | ||
| print(f"\nAn error occurred during API calls: {e}") | ||
| import traceback | ||
| traceback.print_exc() | ||
| finally: | ||
| if hasattr(client, 'close'): | ||
| print("\nClosing client session.") | ||
| client.close() # Important to close to allow oauth_shutdown to be called |
There was a problem hiding this comment.
Similarly as in the other comment, showcasing all these functionalities is unnecessary in this example file. The focus really only should be on OAuth 2.0, followed by a simple call to verify that the setup was successful: more of "hey look, it worked!", less of "and now you can do all these things with it!". Again, nothing wrong with the code you wrote in principle, just too long for an example file.
ibind/client/ibkr_client.py
Outdated
| auto_register_shutdown: bool = var.IBIND_AUTO_REGISTER_SHUTDOWN, | ||
| use_oauth: bool = var.IBIND_USE_OAUTH, | ||
| oauth_config: 'OAuthConfig' = None, | ||
| oauth_config: Union[OAuthConfig, OAuth1aConfig, OAuth2Config] = None, # Updated typing |
There was a problem hiding this comment.
that # Updated typing seems like a WIP note left behind
ibind/client/ibkr_client.py
Outdated
| from ibind.oauth import OAuthConfig # Base class | ||
| from ibind.oauth.oauth1a import OAuth1aConfig | ||
| from ibind.oauth.oauth2 import OAuth2Config, authenticate_oauth2 |
There was a problem hiding this comment.
oauth 1.0a and now also 2.0 are optional extensions of IBind, they require additional dependencies to be installed, hence importing these modules has to happen only once _use_oauth is set to True. Doing it here will cause an import error for users who don't use OAuth. See the way it is carried out currently on the master branch
ibind/var.py
Outdated
| """ IBKR WebSocket API URL when using OAuth 2.0. Defaults to IBIND_WS_URL or wss://api.ibkr.com/v1/api/ws in OAuth2Config. """ | ||
|
|
||
| IBIND_OAUTH2_IP_ADDRESS = os.getenv('IBIND_OAUTH2_IP_ADDRESS', None) | ||
| """ Optional: Pre-configured IP address for OAuth 2.0. If None, it will be auto-fetched. """ |
There was a problem hiding this comment.
The Optional: prefix here is redundant, a lot of these env vars are optional, indicated by None default
pyproject.toml
Outdated
| dev = [ | ||
| "python-dotenv>=0.21" # For .env loading in tests/scripts |
There was a problem hiding this comment.
Similar to my earlier comment, I wouldn't add this. It can be set up by the user (or in this case - the developer) based on their preferences
requirements-dev.txt
Outdated
| ruff>=0.9.4,<0.10.0 | ||
| bandit>=1.8.2,<2.0.0 | ||
|
|
||
| python-dotenv>=1.0.1,<2.0.0 |
There was a problem hiding this comment.
Super useful that you've prepared and shared this 🙌 Despite this being an e2e test, I'd still suggest it is built using a testing suite such as unittest - with setup, acting and assertions. Note we're currently discussing introducing pytest in #96.
|
A separate shout out to @hughandersen who introduced the OAuth 1.0a and helped set up foundation for how this PR could be introduced. Additionally, kudos to @janfrederik who's accurately deduced that OAuth 2.0 may soon become a viable option against my suggestions that it may not be the case - your comment helped us not having to rewrite a bunch of things right now, thanks and good foresight! 🙌 |
|
Thanks @Voyz, the addition of OAuth 1.0a to your package seems to have been well received. |
Thank you, @Voyz.
I hope they roll out that soon to individual accounts as well. oauth2 has been a while sitting there at IBKR... |
This commit addresses all "Easy" and "Medium" effort tasks outlined in the `ibind_oauth2_imp_plan.md`. These changes include: - Dependency updates (pytest, dotenv removal). - Numerous logging level adjustments (info to debug). - Code simplifications and clarifications in `IbkrClient` and `oauth2.py`. - Refinement of OAuth 2.0 header handling and request payload for SSO. - Conditional imports for OAuth modules. - Docstring and comment improvements. - Restructuring of E2E tests using pytest. - Streamlining of the OAuth 2.0 example script. As a result of these improvements and fixes, all OAuth 2.0 E2E tests in `test/e2e/test_ibind_oauth2.py` are now passing. Ref: exponencia-capital-engine/docs/trading/ibind_oauth2_imp_plan.md
This commit completes two significant refactoring tasks outlined in the
OAuth 2.0 implementation plan (docs/trading/ibind_oauth2_imp_plan.md):
1. Refactored `IbkrClient.oauth_init` for OAuth 2.0:
- Moved the brokerage session initialization logic (after obtaining
the SSO token) into a new dedicated function,
`establish_oauth2_brokerage_session`, within
`ibind/oauth/oauth2.py`.
- This function now accepts the `IbkrClient` instance and performs
the necessary /sso/validate and brokerage session initialization
calls, improving clarity and reducing nesting in `IbkrClient`.
2. Refactored `ibind.oauth.oauth2.OAuth2Handler`:
- The `OAuth2Handler` class now uses the main `IbkrClient` instance
for making all HTTP requests, instead of managing its own
`requests.Session`.
- This change standardizes request handling, logging, and error
checking, leveraging the existing mechanisms within `RestClient`
and `IbkrClient`.
These changes enhance the modularity and maintainability of the OAuth 2.0
authentication flow. E2E tests for OAuth 2.0 pass successfully with
these modifications.
|
Thanks again for taking the time to provide such detailed feedback and for your insightful questions – it's really appreciated! I've pushed up some updates to this PR that I hope address everything you've raised. I've also merged in the latest from Addressing @weklund's questions:
Addressing @Voyz's feedback:
Summary of Recent Changes:
I think these updates cover the points from your review and help make the OAuth 2.0 support more solid. Let me know what you think, or if there’s anything else! Thanks! |
Voyz
left a comment
Voyz
left a comment
There was a problem hiding this comment.
Great progress @climbercarmich, following up with another round of review. Appreciate your patience, as I was away on short holiday last week 🙏
Great job on refactoring of the OAuth2.0 logic - it is much more readable now and it really helped me review it. Thanks! I helped you simplify it a little more still.
You mention:
For instance, OAuth dependency imports are now conditional (so users who don't need OAuth won't hit an ImportError)
This doesn't seem to be the case. The OAuth modules are still imported globally and will cause an error if we don't install it correctly. You want to move:
from ibind.oauth.oauth1a import OAuth1aConfig, generate_oauth_headers, req_live_session_token
from ibind.oauth.oauth2 import OAuth2Config, authenticate_oauth2, establish_oauth2_brokerage_sessionto
if self._use_oauth:
from ibind.oauth.oauth1a import OAuth1aConfig, generate_oauth_headers, req_live_session_token
from ibind.oauth.oauth2 import OAuth2Config, authenticate_oauth2, establish_oauth2_brokerage_sessionupdated dataclass comments
That also doesn't seem to be the case, but maybe I didn't express myself correctly. I'd suggest changing:
class OAuth2Config(OAuthConfig):
"""
Dataclass encapsulating OAuth 2.0 configuration parameters.
"""
# --- Core OAuth 2.0 Parameters ---
client_id: str = var.IBIND_OAUTH2_CLIENT_ID
client_key_id: str = var.IBIND_OAUTH2_CLIENT_KEY_IDto:
class OAuth2Config(OAuthConfig):
"""
Dataclass encapsulating OAuth 2.0 configuration parameters.
"""
client_id: str = var.IBIND_OAUTH2_CLIENT_ID
""" OAuth 2.0 Client ID. """
client_key_id: str = var.IBIND_OAUTH2_CLIENT_KEY_ID
""" OAuth 2.0 Client Key ID. """Other than that, fantastic job with things like rewriting the e2e test file, removing dotenv dependency, changing logs from info to debug, and overall pushing this PR forward 👏
| request_params = filter_none(kwargs) | ||
| attempt = 1 | ||
| while attempt <= self._max_retries: | ||
| current_base_url = base_url if base_url is not None else self.base_url | ||
| request_url = f'{current_base_url}{endpoint.lstrip("/")}' | ||
|
|
||
| base_url = base_url if base_url is not None else self.base_url | ||
| all_headers = self._get_headers(method, request_url) # Get base headers from subclass | ||
| if extra_headers: | ||
| all_headers.update(extra_headers) # Add/override with any specific extra_headers | ||
|
|
||
| endpoint = endpoint.lstrip('/') | ||
| url = f'{base_url}{endpoint}' | ||
|
|
||
| headers = self._get_headers(request_method=method, request_url=url) | ||
| headers = {**headers, **(extra_headers or {})} | ||
|
|
||
| # we want to allow default values used by IBKR, so we remove all None parameters | ||
| kwargs = filter_none(kwargs) | ||
|
|
||
| # choose which function should be used to make a reqeust based on use_session field | ||
| request_function = self._session.request if self.use_session else requests.request | ||
|
|
||
| # we repeat the request attempts in case of ReadTimeouts up to max_retries | ||
| for attempt in range(self._max_retries + 1): | ||
| if log: | ||
| self.logger.info(f'{method} {url} {kwargs}{" (attempt: " + str(attempt) + ")" if attempt > 0 else ""}') | ||
|
|
||
| try: | ||
| response = request_function(method, url, verify=self.cacert, headers=headers, timeout=self._timeout, **kwargs) | ||
| result = Result(request={'url': url, **kwargs}) | ||
| return self._process_response(response, result) | ||
| self.logger.info(f'Attempt {attempt}: {method} {request_url} Headers: {all_headers} Kwargs: {request_params}') | ||
|
|
||
| except ReadTimeout as e: | ||
| if attempt >= self._max_retries: | ||
| raise TimeoutError(f'{self}: Reached max retries ({self._max_retries}) for {method} {url} {kwargs}') from e | ||
| result = Result(request={ | ||
| 'method': method, | ||
| 'url': request_url, | ||
| 'headers': all_headers, | ||
| 'params': request_params | ||
| }) | ||
|
|
||
| self.logger.info(f'{self}: Timeout for {method} {url} {kwargs}, retrying attempt {attempt + 1}/{self._max_retries}') | ||
| _LOGGER.info(f'{self}: Timeout for {method} {url} {kwargs}, retrying attempt {attempt + 1}/{self._max_retries}') | ||
|
|
||
| continue # Continue to the next iteration for a retry | ||
|
|
||
| except (ConnectionError, ChunkedEncodingError) as e: | ||
| self.logger.warning( | ||
| f'{self}: Connection error detected, resetting session and retrying attempt {attempt + 1}/{self._max_retries} :: {str(e)}' | ||
| ) | ||
| _LOGGER.warning( | ||
| f'{self}: Connection error detected, resetting session and retrying attempt {attempt + 1}/{self._max_retries} :: {str(e)}' | ||
| ) | ||
| self.close() | ||
| try: | ||
| if self.use_session: | ||
| self.make_session() # Recreate session automatically | ||
| continue # Retry the request with a fresh session | ||
|
|
||
| except ExternalBrokerError: | ||
| raise | ||
|
|
||
| response = self._session.request( | ||
| method, | ||
| request_url, | ||
| headers=all_headers, | ||
| timeout=self._timeout, | ||
| verify=self.cacert, | ||
| **request_params | ||
| ) | ||
| else: | ||
| response = requests.request( | ||
| method, | ||
| request_url, | ||
| headers=all_headers, | ||
| timeout=self._timeout, | ||
| verify=self.cacert, | ||
| **request_params | ||
| ) | ||
| return self._process_response(response, result) | ||
| except (ReadTimeout, Timeout) as e: | ||
| self.logger.warning(f'Attempt {attempt} timed out for {method} {request_url}: {e}') | ||
| if attempt == self._max_retries: | ||
| raise TimeoutError(f'{self}: Max retries reached for {method} {request_url}') from e | ||
| attempt += 1 | ||
| except ChunkedEncodingError as e: | ||
| # Treat ChunkedEncodingError similar to a timeout for retry purposes | ||
| self.logger.warning(f'Attempt {attempt} failed with ChunkedEncodingError for {method} {request_url}: {e}') | ||
| if attempt == self._max_retries: | ||
| raise ExternalBrokerError(f'{self}: Max retries reached for {method} {request_url} after ChunkedEncodingError') from e | ||
| attempt += 1 | ||
| except Exception as e: | ||
| self.logger.exception(e) | ||
| raise ExternalBrokerError(f'{self}: request error: {str(e)}') from e | ||
| self.logger.error(f'{self}: Request {method} {request_url} failed: {e}') | ||
| raise ExternalBrokerError(f'{self}: Request {method} {request_url} failed') from e | ||
| # This line should not be reached if logic is correct, but as a fallback: | ||
| raise ExternalBrokerError(f'{self}: Request {method} {request_url} failed after {self._max_retries} retries without specific exception.') |
There was a problem hiding this comment.
I'm struggling to understand this rewrite. Is there any change here that is required for the OAuth 2.0 flow we're introducing? It seems like just replacing one way of doing requests by another way. Some changes you introduce raise some questions:
- moving url and header generation to within the attempts loop; these stay constant on each attempt and shouldn't need regeneration on each attempt, don't they?
- adding
if attempt == self._max_retries:breaking in ChunkedEncodingError - what's the reasoning behind it? - adding Timeout exception on top of ReadTimeout - do we ever run into 'Timeout'?
| auto_register_shutdown: bool = var.IBIND_AUTO_REGISTER_SHUTDOWN, | ||
| use_oauth: bool = var.IBIND_USE_OAUTH, | ||
| oauth_config: 'OAuthConfig' = None, | ||
| oauth_config: Optional[Union[OAuthConfig, OAuth1aConfig, OAuth2Config]] = None, |
There was a problem hiding this comment.
OAuth1aConfig and OAuth2Config are concrete implementations of OAuthConfig - there is no need for this change, is there?
| from ibind.oauth.oauth1a import OAuth1aConfig, generate_oauth_headers, req_live_session_token | ||
| from ibind.oauth.oauth2 import OAuth2Config, authenticate_oauth2, establish_oauth2_brokerage_session |
There was a problem hiding this comment.
These need to be imported only if user sets use_oauth to True. OAuth is an optional extension, some users don't have its dependencies installed. I already commented on this in the previous review, please look into how the current master branch is handing this.
|
|
||
| self._use_oauth = use_oauth | ||
| self.oauth_config = oauth_config | ||
| self.account_id = account_id # Set account_id early for logger |
|
|
||
| if url is None: | ||
| url = f'https://{host}:{port}{base_route}' | ||
| if self.oauth_config is None: |
There was a problem hiding this comment.
this is placed outside of if self._use_oauth:, which will cause to the exception always being thrown for users who don't use Oauth
| except Exception as e_init_generic: | ||
| _LOGGER.error(f"{client}: A generic error occurred during initialize_brokerage_session(compete=True): {e_init_generic}") | ||
|
|
||
| except Exception as e_validate_sequence: |
There was a problem hiding this comment.
this try/catch is too large, and non-specific. I'd recommend removing it
| _LOGGER.error(f"An unexpected error occurred during OAuth 2.0 authentication process: {e}", exc_info=True) | ||
| return None | ||
|
|
||
| def establish_oauth2_brokerage_session(client: 'IbkrClient') -> None: |
There was a problem hiding this comment.
I took the liberty to address the issues I've raised in this function and reduce its size by turning the reattempt process into a for loop:
def establish_oauth2_brokerage_session(client: 'IbkrClient', max_retries:int=2) -> None:
"""
Establishes the brokerage session for an OAuth 2.0 authenticated client.
This involves validating the SSO session and then initializing the brokerage session.
"""
_LOGGER.debug(f"{client}: OAuth 2.0: Attempting to establish brokerage session (/sso/validate and initialize).")
validation_result = client.validate()
_LOGGER.debug(f"{client}: /sso/validate result: {validation_result.data if validation_result else 'No result'}")
if not (validation_result.data.get('RESULT') or validation_result.data.get('authenticated')):
_LOGGER.warning(
f"{client}: /sso/validate did not indicate a clear success. "
f"Cannot proceed with brokerage session initialization. "
f"Validation data: {validation_result}"
)
return
_LOGGER.debug(f"{client}: /sso/validate successful. Now attempting to initialize brokerage session.")
for attempt in range(max_retries + 1):
try:
_LOGGER.debug(f"{client}: Calling initialize_brokerage_session(compete=True).")
init_result = client.initialize_brokerage_session(compete=True)
_LOGGER.debug(f"{client}: initialize_brokerage_session(compete=True) result: {init_result}")
auth_status_after_init = client.authentication_status()
_LOGGER.debug(f"{client}: /iserver/auth/status (after compete=True init): {auth_status_after_init}")
if not auth_status_after_init.data.get('authenticated'):
_LOGGER.warning(f"{client}: Still not authenticated after compete=True init.")
except ExternalBrokerError as e_init_compete_true:
_LOGGER.error(f"{client}: establishing oauth2 brokerage session failed: {e_init_compete_true}")
if attempt >= max_retries:
raise e_init_compete_true
if e_init_compete_true.status_code == 500 and "failed to generate sso dh token" in str(e_init_compete_true):
_LOGGER.info(f"{client}: Retrying initialize_brokerage_session with compete=False due to DH token error. Retrying attempt {attempt + 1}/{max_retries}")
continue
else:
# we don't retry for non-DH token errors
break
# else: other error from compete=True, not the DH token one. We just log it above.
except Exception as e_init_generic:
_LOGGER.error(f"{client}: A generic error occurred during initialize_brokerage_session(compete=True): {e_init_generic}")
# we don't retry for generic errors
break| "requests>=2.31", | ||
| "websocket-client>=1.7" | ||
| "websocket-client>=1.7", | ||
| "pycryptodome>=3.21" # Essential for OAuth 2.0 JWT signing |
There was a problem hiding this comment.
this needs to be in the [project.optional-dependencies] as OAuth is not included in the default install
| oauth = [ | ||
| 'pycryptodome>=3.21', | ||
| 'urllib3>=2.3', | ||
| dev = [ |
There was a problem hiding this comment.
this change also doesn't make sense, we need these dependencies
| # Configure basic logging for the test script itself - Pytest handles this. | ||
| # logger = logging.getLogger(__name__) | ||
| # logging.basicConfig(level=logging.INFO, format='%(asctime)s - %(name)s - %(levelname)s - %(message)s') |
There was a problem hiding this comment.
This is already called above, isn't it?
|
@climbercarmich thanks for the great work! |
|
hey @climbercarmich just wanted to follow up on this as this contribution is great and I wouldn't want it to get left behind - although I understand if you're unavailable at the moment to look into it more. I know I've left detailed PR reviews here, so if you'd need more help feel free to reach out to me at |
|
Dear ibind maintainer team, I would like to voice my support for a timely addition of oauth 2. We have a ibkr business account with oauth 2 (setup through ibkr support email), we were also sent the same implementation examples: (Websocket tester.txt Our internal implementation / fork matches @climbercarmich implementation ~90% and we would switch to the official implementation right away once it is merged. |
|
hey @arau-j thanks for showing your support to this PR 👍 It has been last reviewed in May but hasn't received further updates from its author - I'm assuming he's been busy and couldn't pick it up again. Should anyone feel like continuing their work, I'm happy to provide support and review further updates to this PR |
7 participants
Hi maintainers,
Closes #102 .
This PR extends IBind's authentication capabilities by adding support for the OAuth 2.0 Client Credentials Grant flow. This complements the existing OAuth 1.0a implementation and further enables fully headless authentication with the IBKR Client Portal API. I've aimed to follow the project's contribution guidelines in preparing this feature.
Key changes include:
ibind.oauth.oauth2module withOAuth2Configfor configuration and logic for token management.IbkrClient, handling automatic SSO bearer token acquisition and session management.examples/rest_09_oauth2.py, to demonstrate practical usage.test/e2e/test_ibind_oauth2.py(which loads credentials from a root.envfile), to verify the E2E functionality.README.mdwith information on OAuth 2.0 setup and usage.pyproject.tomlandrequirements-dev.txtfor dependencies (pycryptodomefor core functionality,python-dotenvfor examples/dev).Further Notes:
Please let me know if you have any questions or feedback.
Thanks!