clients
clients copied to clipboard
[PM-6426] Create TaskSchedulerService and update long lived timeouts in the extension to leverage the new service
Type of change
- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [x] Other
Objective
The objective of this PR is to leverage the chrome.alarms API for any delayed or repeating tasks that are long-lived. In this case, we are considered "long-lived" tasks to be tasks that delay their execution by 1 minute or longer.
This implementation creates a TaskSchedulerService that can be leveraged in common lib classes. The base implementation simply sets methods that leverages setTimeout or setInterval to create delayed execution of tasks, as well as a method to clear those based on a timeout ID.
This based TaskSchedulerService class is then extended to the browser extension specific BrowserTaskSchedulerService which leverages the chorme.alarms API for longer lived delayed tasks. This implementation also takes into account "recovered tasks" that might have been lost due to a lack of service worker persistence. We recover and reset alarms baed on values in local storage that indicate an alarm should be set if for some reason that alarm is unset in an undesirable way.
The following areas of the codebase leverage this new TaskSchedulerService class:
- SyncTimeout - main.background.ts
- ClearClipboard SystemService - system.service.ts
- ClearClipboard GeneratePasswordToClipboard - generate-password-to-clipboard-command.ts
- Fido2 setAbortTimeouts - fido2-client.service.ts
- NotificationsService reconnectTimer - notifications.service.ts
- LoginStrategy sessionTimeoutLength - login-strategy.service.ts
- EventUploadService when calling this.uploadEvents - event-upload.service.ts
Code changes
I'm going to outline the files that affect behavior directly, the rest are Jest tests, service factories that build the new TaskSchedulerService class, or small changes that do not affect behavior significantly.
- apps/browser/src/autofill/clipboard/generate-password-to-clipboard-command.ts: Adjusting the implementation within this file to leverage the
TaskSchedulerServiceclass - apps/browser/src/background/main.background.ts: Adding the
BrowserTaskSchedulerServiceclass as a dependency to the various classes that leverage it, and adjusting how we handle scheduling the next sync of a user's vault through usage of theTaskSchedulerServiceclass. - apps/browser/src/manifest.json: Adding the
alarmspermission to manifest v2 - apps/browser/src/platform/browser/browser-api.ts: Adding promisified implementations of the
chrome.alarmsAPI methods. - apps/browser/src/platform/services/browser-task-scheduler.service.ts: Browser extension-specific extension of the
TaskSchedulerServiceclass, handles setup and logic surroundingchrome.alarms. - apps/browser/src/popup/services/services.module.ts: Adding the
TaskSchedulerServiceandBrowserTaskSchedulerServiceclasses as dependencies, and ensuring that we update dependency references in classes that use them. - apps/desktop/src/app/services/services.module.ts: Updating dependency references to
TaskSchedulerService - libs/angular/src/services/jslib-services.module.ts: Adding the
TaskSchedulerServiceclass as a dependency, and ensuring that we update dependency references in classes that use it. - libs/auth/src/common/services/login-strategies/login-strategy.service.ts: Updating how we handle triggering the session timeout to levarege the
TaskSchedulerServiceclass - libs/common/src/platform/enums/scheduled-task-name.enum.ts: Adding a set of
ScheduledTaskNamesthat are used when creating an alarm for that specific timeout/interval. - libs/common/src/platform/services/system.service.ts: Implementing the
clearClipboardTimeoutin this class to leverage theTaskSchedulerServiceclass. - libs/common/src/platform/services/task-scheduler.service.ts: The base implementation of the
TaskSchedulerServiceclass, allows users to create setTimeouts and setIntervals, as well as clear them. - libs/common/src/platform/state/state-definitions.ts: Adding a
TASK_SCHEDULER_DISKstate value that handles re-creating lostchrome.alarmsthrough state references. - libs/common/src/services/event/event-upload.service.ts: Updating the interval that handles uploading events to use the
TaskSchedulerServiceclass - libs/common/src/services/notifications.service.ts: Updating the notifications reconnect timeout to leverage the
TaskSchedulerServiceclass - libs/common/src/vault/services/fido2/fido2-client.service.ts: Updating the FIdo2 AbortTimeout to leverage the
TaskSchedulerServiceclass.
Checkmarx One – Scan Summary & Details – c56d0f1a-ea77-4c25-8ce7-bfb13bae1596
New Issues
| Severity | Issue | Source File / Package | Checkmarx Insight |
|---|---|---|---|
![]() |
Client_Privacy_Violation | /apps/web/src/app/tools/reports/pages/reused-passwords-report.component.ts: 47 | Attack Vector |
![]() |
Client_Privacy_Violation | /apps/web/src/app/tools/reports/pages/weak-passwords-report.component.ts: 22 | Attack Vector |
![]() |
Client_Privacy_Violation | /apps/web/src/app/tools/reports/pages/reused-passwords-report.component.ts: 19 | Attack Vector |
![]() |
Client_Privacy_Violation | /apps/web/src/app/tools/reports/pages/reused-passwords-report.component.html: 95 | Attack Vector |
Codecov Report
Attention: Patch coverage is 78.28947% with 66 lines in your changes missing coverage. Please review.
Project coverage is 30.36%. Comparing base (
5fcf4bb) to head (94bbcbe).
Additional details and impacted files
@@ Coverage Diff @@
## main #8566 +/- ##
==========================================
+ Coverage 30.17% 30.36% +0.19%
==========================================
Files 2589 2594 +5
Lines 75822 76014 +192
Branches 14201 14229 +28
==========================================
+ Hits 22876 23082 +206
+ Misses 51168 51149 -19
- Partials 1778 1783 +5
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Need to dig into this a bit more, removing it from "Ready to Review" until then.
A couple other questions I had during review:
- It seems like
registerTaskHandleris marked as returningvoidbut was called likevoid tss.registerTaskHandlerlike it was promise returning. I'd really love for it to sync just so we can guarantee that listeners are added synchronously.- Why does it seem like the
TaskSchedulerServicemight not be injected? It's called with null coalescing a lot. I think we will always want an instance available.- If we are forced into sync over async then I'd much rather a
catchbe chained and log the error over justvoid.
Ah you're right, all of the registerTaskHandler calls a synchronous and should be referenced as such. I think at one point I was registering tasks based on userId, but realized that wasn't a good approach and would unnecessarily duplicate the task handlers. I removed the need for an await, but forgot to update the call point.
As for TaskSchedulerService being nullish.... we run into duplicate registrations due to getBgService and the re-instantiation of MainBackground... We do have fallbacks for duplicate alarm registrations though, but that's the reason I went with an optional introduction. If that isn't a concern, I can update the implementation to force the dependency to be instantiated.
@justindbaur
I've introduced your requested changes, reworking the file structure as requested. This is ready to review.
@cagonzalezcs Sorry one additional followup - any reason this wasn't implemented in vault timeout?
@jlf0dev Hmm, the TaskSchedulerService should be set up with vault timeout here - https://github.com/bitwarden/clients/pull/8566/files#diff-91b0f82adb497c10aa17477f2d6c2bb08d21b49558856b8327a2d7e4ab1c715dR45
