clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-5382] Add Sync Purpose

Open justindbaur opened this issue 1 year ago • 6 comments

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

This will help us track which sync types are happening so we can better track when they are being to eager with their syncing.

What to review

  • That the purpose I chose for your code makes sense
  • That I spelled everything correctly

Code changes

  • ApiService - Take the new purpose and set it into a header
  • SyncService - make a purpose now required.
  • Everything else adding a purpose to the best of my ability.

Screenshots

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

justindbaur avatar Dec 19 '23 21:12 justindbaur

Can we enumerate these so they are centralized and then enforce some naming standards and clarity?

withinfocus avatar Dec 19 '23 21:12 withinfocus

Don't we include device type in headers? Why do we need the client in the purpose string?

jlf0dev avatar Dec 19 '23 23:12 jlf0dev

@withinfocus @eliykat I did think about doing something like this but was worried it would actually encourage reuse of the purpose string. Since an enum or string union is generally used as a list of allowed values but in our case it would be a list of not allowed values until you add your single entry and then that is the only value you're allowed to use.

Plus an enum would only indicate we have a duplicate if we named the identifier the same but not if we named the identifier different but still had the value the same. The union wouldn't save us at all, it will just collapse the repeated entry into one. (Ref)

@jlf0dev Very good point, for some reason I thought that was only being added to some api calls not all of them. Think it's alright to repeat the same purpose string in those cases then?

justindbaur avatar Dec 20 '23 02:12 justindbaur

Think it's alright to repeat the same purpose string in those cases then?

Seems like it would be be ok. I do like your goal here of keeping them unique enough to identify where it came from, but we might be able to find the client

jlf0dev avatar Dec 20 '23 13:12 jlf0dev

Logo Checkmarx One – Scan Summary & Details5305327a-e85d-4ea8-98bd-94beb65ca502

New Issues

Severity Issue Source File / Package Checkmarx Insight
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /apps/browser/src/background/service-factories/vault-timeout-service.factory.ts: 97 Attack Vector

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

Codecov Report

Attention: Patch coverage is 5.26316% with 72 lines in your changes are missing coverage. Please review.

Project coverage is 27.74%. Comparing base (3caa6cb) to head (bc7f072). Report is 35 commits behind head on main.

Files Patch % Lines
...ibs/common/src/vault/services/sync/sync.service.ts 0.00% 7 Missing :warning:
.../services/organization/organization-api.service.ts 0.00% 6 Missing :warning:
...pps/browser/src/auth/popup/two-factor.component.ts 0.00% 4 Missing :warning:
libs/common/src/services/api.service.ts 0.00% 4 Missing :warning:
libs/common/src/services/notifications.service.ts 0.00% 4 Missing :warning:
apps/desktop/src/app/app.component.ts 0.00% 3 Missing :warning:
...console/providers/services/web-provider.service.ts 0.00% 3 Missing :warning:
apps/browser/src/auth/popup/sso.component.ts 0.00% 2 Missing :warning:
apps/browser/src/background/main.background.ts 0.00% 2 Missing :warning:
apps/desktop/src/auth/sso.component.ts 0.00% 2 Missing :warning:
... and 31 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7289      +/-   ##
==========================================
+ Coverage   27.70%   27.74%   +0.03%     
==========================================
  Files        2392     2411      +19     
  Lines       69356    69856     +500     
  Branches    12934    12992      +58     
==========================================
+ Hits        19218    19381     +163     
- Misses      48658    48972     +314     
- Partials     1480     1503      +23     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 03 '24 06:05 codecov[bot]