Support IBKR second-factor device selection and manual 2FA fallback#277
Support IBKR second-factor device selection and manual 2FA fallback#277onurserce wants to merge 2 commits intoVoyz:masterfrom
Conversation
Voyz
left a comment
There was a problem hiding this comment.
Hi @onurserce thanks for submitting the PR and for bearing with me.
I reviewed it and have a few points I'd like for you to address. It is really well written, my comments are mainly to do with redundancies than with incorrect implementation.
However, first I'd like to ask you:
Do I understand correctly that just changing 'ID@@xyz-field-bronze-response' to 'TAG@@select' for TWO_FA_SELECT_EL_ID is the main solution in this PR? If we were to just do this change on the current version of IBeam, would this also solve the issue?
| _SENSITIVE_CONFIG_KEYS = { | ||
| 'ACCOUNT', | ||
| 'PASSWORD', | ||
| 'KEY', | ||
| 'SECRET', | ||
| 'TOKEN', | ||
| } | ||
|
|
||
|
|
||
| def redact_config(config: dict) -> dict: | ||
| redacted = {} | ||
| for key, value in config.items(): | ||
| if any(sensitive_key in key for sensitive_key in _SENSITIVE_CONFIG_KEYS): | ||
| redacted[key] = '***REDACTED***' if value is not None else None | ||
| else: | ||
| redacted[key] = value | ||
|
|
||
| return redacted |
There was a problem hiding this comment.
While this is very useful in principle, the var.py purposefully doesn't store any sensitive env vars - they're handled separately in the secrets_handler.
References that would be ***REDACTED*** by this are things like:
SECRETS_SOURCEGCP_SECRETS_URLUSE_PAPER_ACCOUNTIBKEY_PROMO_EL_CLASS
All false positives.
So yeah, in theory a good idea, but not in this case - please remove this addition.
| @@ -1,9 +1,10 @@ | |||
| *.iml | |||
| # Local runtime secrets for docker compose | |||
There was a problem hiding this comment.
This addition seems unnecessary. Anything you had in mind or is this a leftover?
| IBEAM_PASSWORD=your_password123 | ||
| ``` | ||
|
|
||
| For a reusable starting point, copy [`env.list.example`](./env.list.example) to `env.list` and adjust the values for your account. |
There was a problem hiding this comment.
.env is one way to configure env vars for an app, but not the only one. I try to allow the user to select the way the prefer to use, rather than suggest one in such a way. This is so that there is no indication that there is a correct way to set env vars - there isn't, any way would do.
Therefore, although I understand why you'd like to provide an example, I'd discourage this kind of env.list.example additions.
|
|
||
| ### Two-factor device selection | ||
|
|
||
| Some IBKR accounts show an extra `Select Second Factor Device` dropdown after submitting the username and password. If your account does, add the following settings to `env.list`: |
There was a problem hiding this comment.
Precisely following from my other comment about .env files - I'd avoid this kind of suggestions. If we said ... add the following environment variables: we'd allow everyone to do it in whichever way they prefer. The current phrasing may suggest to some users that env.list is the only way to do this.
| def _wait_for_manual_gateway_authentication(self) -> bool: | ||
| deadline = time.time() + self.manual_two_fa_timeout | ||
| ssl_context = ssl._create_unverified_context() | ||
|
|
||
| while time.time() < deadline: | ||
| try: | ||
| response = urllib.request.urlopen( | ||
| urllib.request.Request(self.base_url + self.route_tickle, method='POST'), | ||
| context=ssl_context, | ||
| timeout=self.request_timeout, | ||
| ) | ||
| payload = json.loads(response.read().decode('utf8')) | ||
| auth_status = payload.get('iserver', {}).get('authStatus', {}) | ||
| if auth_status.get('authenticated'): | ||
| return True | ||
| except (HTTPError, URLError, TimeoutError, json.JSONDecodeError): | ||
| pass | ||
|
|
||
| time.sleep(5) | ||
|
|
||
| return False |
There was a problem hiding this comment.
Can you share what use case this addition covers?
Forgive me if I'm not seeing the logic here, but if we expect the user to participate manually in the authentication process - why would they even use IBeam? If they need to log into localhost:5000 and do it manually, they would just get the Gateway and run it themselves, wouldn't they?
| MANUAL_TWO_FA_TIMEOUT = int(os.environ.get('IBEAM_MANUAL_TWO_FA_TIMEOUT', 300)) | ||
| """How many seconds to wait for manual 2FA completion.""" | ||
|
|
||
| TWO_FA_SELECT = to_bool(os.environ.get('IBEAM_TWO_FA_SELECT', False)) |
There was a problem hiding this comment.
I'd suggest TWO_FA_USE_SELECT to make it a bit clearer this is a flag which tells IBeam to look for an HTML element, not a field in which we're expected to select something.
| targets['TWO_FA_NOTIFICATION'] = Target(cnf.TWO_FA_NOTIFICATION_EL) | ||
| targets['TWO_FA_INPUT'] = Target(cnf.TWO_FA_INPUT_EL_ID) | ||
| targets['TWO_FA_SELECT'] = Target(cnf.TWO_FA_SELECT_EL_ID) | ||
| targets['TWO_FA_INPUT_GENERIC'] = Target('PLACEHOLDER@@Code') |
There was a problem hiding this comment.
What is the use case for this TWO_FA_INPUT_GENERIC? In the PR decription you mention:
add a generic code-input fallback using placeholder text matching (PLACEHOLDER@@code) for same-page OTP variants
But in reality I can see that it is used in the new _find_two_fa_input_target which replaces
wait_and_identify_trigger(is_clickable(targets['TWO_FA_INPUT']), skip_identify=True)
Why wouldn't the user just set the env var that already is supported:
TWO_FA_INPUT=PLACEHOLDER@@Code
In the readme there is a suggestion that "... without requiring an extra env override." - if this is the only reason then I'd suggest we avoid this addition. It adds mostly redundant code and increases complexity only to save us a single line of documentation asking the user to set an env var.
If there is another use case - let me know.
| return bool(strtobool(str(value))) | ||
|
|
||
|
|
||
| def strip_quotes(value): |
| try: | ||
| select.select_by_visible_text(two_fa_select_target) | ||
| selected_text = two_fa_select_target | ||
| except Exception: | ||
| for option_text in selectable_option_texts: | ||
| if two_fa_select_target.lower() in option_text.lower(): | ||
| select.select_by_visible_text(option_text) | ||
| selected_text = option_text | ||
| break |
There was a problem hiding this comment.
What is the use case behind this retry mechanism? Is select_by_visible_text not sufficient to select the item?
| if selected_text is None and len(selectable_option_texts) == 1: | ||
| selected_text = selectable_option_texts[0] | ||
| select.select_by_visible_text(selected_text) |
There was a problem hiding this comment.
Fallback to the first available option if nothing else worked feels like silencing an error. Instead we should error out to surface the failure in the log
|
Hi @Voyz - thanks for the review. Please bear in mind that this implementation is done with the help of a coding agent. Some of your questions I believe are arising due to this (so they are a matter of quality), but others mainly concern the policies of the repository, product specification and intended use cases etc.. I mainly did this because it is useful to me, and opened the PR with the thought that it could be useful for others as well. For example, the manual login is because I did not have 2FA system in place and could not figure it out from the repository So I wanted to have a temporary solution before I work on 2FA. If 2FA is already implemented (meaning that a TOTP generator exists within IBeam), perhaps I did not take the effort to look deeply or could not understand it. If your intended goal is to make a product that is useful for a wider audience, it might be helpful to have policies, specifications, product roadmap and contributions guide (currently it's only technical) for the project. Given that many people nowadays use coding agents, this would reduce false priors and assumptions, therefore increase the quality of work. Having a better sense of what is needed, what's accepted and what not, I would be more than happy to contribute to the project. I will get back to your comments in some time. In the meantime, I wanted to ask you if you'd be willing to have a short call and discuss some of these points to create alignment. What do you think? |
|
hey @onurserce thanks for elaborating on this PR. Let me address your points:
It's fine to use AI agents, but eventually it's each contributor's responsibility to review, clean and align the produced code. While reviewing PRs it's impossible to deduce what has been AI-written and what human-written, and hence irrespectively of its source everything is reviewed at its face value. So what I'm trying to say is - bearing in mind that implementation may be AI written does not change how the PR will be reviewed. I'd even argue it should be more scrupulous, and it would have been useful to have the PR tagged as AI written from the get go (or which portions of it were AI). Naturally, I don't disagree with introducing better contributing policies - thanks for pointing it out. I bet the
It would be useful if you could point out which ones are which - policy/spec vs. AI quality. Again, on the receiving end it's hard to distinguish, hence the intent is to review everything more or less equally.
While I understand this may be useful to you temporarily, we want to evaluate additions from perspective of usefulness to the an average user. In which case, my earlier comment on manual login applies. If other users would find it useful, then there's nothing wrong with introducing a temporary solution - but we need to evaluate its usefulness.
Funnily enough, it wasn't when you opened the PR, but recently @frequentfliar
I agree - thank you for pointing this out. It's an iterative process, so feedback like this is essential to seeing what's missing.
Absolutely! I'd be very keen to listen to your feedback and brainstorm on how to make things easier for contributors. Can you drop me an email at hello@voyzan.com and let's continue the talk about the call there? |
Summary
This PR solves issue #271 by improving the IBKR login flow for accounts that do not go directly from username/password to a standard 2FA screen.
It adds support for:
Select Second Factor DevicedropdownIt also updates the docs and provides a tracked
env.list.exampletemplate for compose-based setups.Problem
Some IBKR accounts now show an extra device-selection step after submitting credentials. The existing flow did not expect this intermediate screen, and the default selector for
TWO_FA_SELECTwas incorrect for that page.A second issue is that some variants do not navigate to a separate OTP page after device selection. Instead, they stay on the same login form and reveal a code input such as
Mobile Authenticator App Code.Finally, users with device-bound authenticators may have no usable automated
IBEAM_TWO_FA_HANDLER, so the current behavior shuts IBeam down at the 2FA step even though the login could still be completed manually.Changes
Login flow
TAG@@selecttargets so the second-factor device dropdown can be identified reliablyIBEAM_TWO_FA_SELECTPLACEHOLDER@@Code) for same-page OTP variantsIBEAM_MANUAL_TWO_FA=trueand no automated 2FA handler is configuredConfig and safety
IBEAM_MANUAL_TWO_FAIBEAM_MANUAL_TWO_FA_TIMEOUTIBEAM_TWO_FA_SELECT_EL_IDandIBEAM_TWO_FA_SELECT_TARGET(in case the user enters the environment variables in quotes)Docs
env.list.examplefor clarity, keep localenv.listignoredExample runtime output
The following log sequence shows the intended behavior for the
Mobile Authenticator Appflow with manual fallback enabled:Configuration example
For accounts that show the extra device-selection step and require manual completion of 2FA:
Verification
Tested locally by rebuilding the Docker image and exercising the login flow against an IBKR account that shows:
Select Second Factor DeviceMobile Authenticator AppCode