-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[PM-22781] Implement autoconnect to desktop app on app start #15266
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: main
Are you sure you want to change the base?
Conversation
New Issues (4)Checkmarx found the following issues in this Pull Request
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #15266 +/- ##
==========================================
+ Coverage 36.90% 37.16% +0.26%
==========================================
Files 3225 3277 +52
Lines 93192 94429 +1237
Branches 14016 14228 +212
==========================================
+ Hits 34388 35098 +710
- Misses 57380 57880 +500
- Partials 1424 1451 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
One minor thing from me. Might be a good idea to have @mzieniukbw look at this one as well, as I'm less familiar with this area.
apps/browser/src/key-management/biometrics/background-browser-biometrics.service.ts
Show resolved
Hide resolved
Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
Good point, I have requested maciej's review. |
try { | ||
await this.nativeMessagingBackground().connect(); | ||
} catch (e) { | ||
this.logService.warning( |
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.
When desktop app is not running (linux, windows only), this will spam errors on console in here and in
this.logService.error("NativeMessaging port disconnected because of error: " + error); |
Isn't it a bit too much ?
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.
That is fair. I'm supressing the error here now.
filter(([_]) => !this.nativeMessagingBackground().connected), | ||
switchMap(async () => { | ||
try { | ||
await this.nativeMessagingBackground().connect(); |
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.
❓ ideally, this should only be working when extension is actually in background, not open. We already try to connect via sending a message over IPC every 1 second on unlock page.
🤔 I think this is fine, but got me wondered, since unlock page is on extension main thread and this is background worker thread, is it possible for connect()
to be in race condition between the two and potentially create some unexpected behaviour, like connected twice ?
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 changed this to:
- Run only every 30 seconds in background
- Disable running on a timer while the popup is open
I wonder whether in the long run it makes sense to decouple calling from connecting, and make it so that:
- If is not connected, make the interface return sensible states (throw for unlock / return "unavailable" for getStatus)
- Always only connect via background autoconnect, but have the interval change between something longer (like 30-60 seconds), and something shorter (5 seconds or less?) depending on whether the popup is open.
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.
If someone just opened Bitwarden desktop app, unlocked and opens browser extension, then the connect request should happen on request, if not connected, like it is right now. (no change).
Otherwise background check would have to be much more frequent for good UX, like maybe even every second. In fact, if we would additionally check for connecting
flag, then i don't see much risk in moving the connect
entirely here.
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.
Added the connecting
check to the connect function, so that even calling it multiple times is expected to be safe.
…rden/clients into km/pm-22871/autoconnect-to-desktop
// If it is open, then sending regular IPC messages will already attempt to connect. | ||
const backgroundTimer = timer(0, this.BACKGROUND_POLLING_INTERVAL).pipe( | ||
concatMap(async () => { | ||
return await BrowserApi.isPopupOpen(); |
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 think this returns false when the extension window is open, but not in popup mode.
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.
Removed this and polling is now always happening.
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22871
📔 Objective
Autostart is not working after a process reload, because the desktop app is not yet connected, and so the unlock options initially show biometrics as unavailable (it then becomes available just a moment later), so the lock components defaults to MP/PIN. There a numerous ways to fix this, in this case this PR adds autoconnect code. As long as biometrics is enabled in the settings, the browser will attempt to auto-connect to the desktop app in the background.
This means that IPC to the desktop app is available directly, without any wait, also fixing auto-prompt as a side-effect.
Tested on MacOS.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes