clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-6426] Create TaskSchedulerService and update long lived timeouts in the extension to leverage the new service

Open cagonzalezcs opened this issue 1 year ago • 5 comments

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 TaskSchedulerService class
  • apps/browser/src/background/main.background.ts: Adding the BrowserTaskSchedulerService class 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 the TaskSchedulerService class.
  • apps/browser/src/manifest.json: Adding the alarms permission to manifest v2
  • apps/browser/src/platform/browser/browser-api.ts: Adding promisified implementations of the chrome.alarms API methods.
  • apps/browser/src/platform/services/browser-task-scheduler.service.ts: Browser extension-specific extension of the TaskSchedulerService class, handles setup and logic surrounding chrome.alarms.
  • apps/browser/src/popup/services/services.module.ts: Adding the TaskSchedulerService and BrowserTaskSchedulerService classes 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 TaskSchedulerService class 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 TaskSchedulerService class
  • libs/common/src/platform/enums/scheduled-task-name.enum.ts: Adding a set of ScheduledTaskNames that are used when creating an alarm for that specific timeout/interval.
  • libs/common/src/platform/services/system.service.ts: Implementing the clearClipboardTimeout in this class to leverage the TaskSchedulerService class.
  • libs/common/src/platform/services/task-scheduler.service.ts: The base implementation of the TaskSchedulerService class, allows users to create setTimeouts and setIntervals, as well as clear them.
  • libs/common/src/platform/state/state-definitions.ts: Adding a TASK_SCHEDULER_DISK state value that handles re-creating lost chrome.alarms through state references.
  • libs/common/src/services/event/event-upload.service.ts: Updating the interval that handles uploading events to use the TaskSchedulerService class
  • libs/common/src/services/notifications.service.ts: Updating the notifications reconnect timeout to leverage the TaskSchedulerService class
  • libs/common/src/vault/services/fido2/fido2-client.service.ts: Updating the FIdo2 AbortTimeout to leverage the TaskSchedulerService class.

cagonzalezcs avatar Apr 01 '24 17:04 cagonzalezcs

Logo Checkmarx One – Scan Summary & Detailsc56d0f1a-ea77-4c25-8ce7-bfb13bae1596

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/reports/pages/reused-passwords-report.component.ts: 47 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/reports/pages/weak-passwords-report.component.ts: 22 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/reports/pages/reused-passwords-report.component.ts: 19 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/reports/pages/reused-passwords-report.component.html: 95 Attack Vector

github-actions[bot] avatar Apr 01 '24 17:04 github-actions[bot]

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).

Files Patch % Lines
...ibs/common/src/platform/services/system.service.ts 0.00% 13 Missing :warning:
apps/browser/src/background/main.background.ts 0.00% 10 Missing :warning:
libs/common/src/services/notifications.service.ts 0.00% 9 Missing :warning:
...s/task-scheduler/browser-task-scheduler.service.ts 93.49% 7 Missing and 1 partial :warning:
.../common/src/services/event/event-upload.service.ts 0.00% 6 Missing :warning:
...rc/platform/services/fido2/fido2-client.service.ts 73.68% 1 Missing and 4 partials :warning:
...rc/services/vault-timeout/vault-timeout.service.ts 0.00% 4 Missing :warning:
...rc/services/vault-timeout/vault-timeout.service.ts 57.14% 3 Missing :warning:
...lipboard/generate-password-to-clipboard-command.ts 84.61% 1 Missing and 1 partial :warning:
apps/browser/src/popup/services/services.module.ts 0.00% 2 Missing :warning:
... and 3 more
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.

codecov[bot] avatar Apr 02 '24 15:04 codecov[bot]

Need to dig into this a bit more, removing it from "Ready to Review" until then.

cagonzalezcs avatar Apr 17 '24 21:04 cagonzalezcs

A couple other questions I had during review:

  • It seems like registerTaskHandler is marked as returning void but was called like void tss.registerTaskHandler like 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 TaskSchedulerService might 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 catch be chained and log the error over just void.

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.

cagonzalezcs avatar May 07 '24 14:05 cagonzalezcs

@justindbaur

I've introduced your requested changes, reworking the file structure as requested. This is ready to review.

cagonzalezcs avatar Jun 13 '24 13:06 cagonzalezcs

@cagonzalezcs Sorry one additional followup - any reason this wasn't implemented in vault timeout?

jlf0dev avatar Jul 10 '24 13:07 jlf0dev

@jlf0dev Hmm, the TaskSchedulerService should be set up with vault timeout here - https://github.com/bitwarden/clients/pull/8566/files#diff-91b0f82adb497c10aa17477f2d6c2bb08d21b49558856b8327a2d7e4ab1c715dR45

cagonzalezcs avatar Jul 10 '24 16:07 cagonzalezcs