Skip to content
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

[PM-5273] Migrate state in CipherService #8314

Merged
merged 33 commits into from
Apr 16, 2024
Merged

[PM-5273] Migrate state in CipherService #8314

merged 33 commits into from
Apr 16, 2024

Conversation

LRNcardozoWDF
Copy link
Member

@LRNcardozoWDF LRNcardozoWDF commented Mar 12, 2024

Type of change

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

Objective

Migrate CipherService state to state provider, encryptedCiphers, decryptedCiphers, localData, addEditCipherInfo.

Code changes

  • state-definitions.ts: Created key for data
  • 35-move-local-data-to-state-provider.spec.ts: Created migrator for localData
  • abstractions/cipher.service.ts cipher.service.ts: Added methods to allow access to AddEditCipherInfo. Refactor decrypt method so it could be used by derived state.

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

@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Mar 12, 2024
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

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

Project coverage is 27.36%. Comparing base (62ed7e5) to head (abea832).

Files Patch % Lines
libs/common/src/vault/services/cipher.service.ts 20.40% 76 Missing and 2 partials ⚠️
...mmon/src/vault/services/key-state/ciphers.state.ts 46.66% 8 Missing ⚠️
...r/src/tools/popup/generator/generator.component.ts 0.00% 6 Missing ⚠️
apps/browser/src/popup/app.component.ts 0.00% 3 Missing ⚠️
...angular/src/vault/components/add-edit.component.ts 0.00% 3 Missing ⚠️
...common/src/vault/services/folder/folder.service.ts 33.33% 2 Missing ⚠️
...vault/popup/components/vault/add-edit.component.ts 0.00% 1 Missing ⚠️
libs/common/src/platform/models/domain/account.ts 0.00% 1 Missing ⚠️
libs/common/src/vault/models/data/cipher.data.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8314      +/-   ##
==========================================
+ Coverage   27.30%   27.36%   +0.05%     
==========================================
  Files        2341     2343       +2     
  Lines       68379    68418      +39     
  Branches    12785    12787       +2     
==========================================
+ Hits        18669    18720      +51     
+ Misses      48309    48296      -13     
- Partials     1401     1402       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bitwarden-bot
Copy link

bitwarden-bot commented Mar 13, 2024

Logo
Checkmarx One – Scan Summary & Detailsa352c66f-d22c-4ebd-9fd6-39c58443fe1c

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Angular_Improper_Type_Pipe_Usage /apps/browser/src/vault/popup/components/fido2/fido2-use-browser-link.component.html: 1 Attack Vector
MEDIUM Angular_Improper_Type_Pipe_Usage /apps/web/src/app/billing/shared/adjust-storage.component.html: 27 Attack Vector
MEDIUM Angular_Improper_Type_Pipe_Usage /apps/web/src/app/billing/organizations/adjust-subscription.component.html: 54 Attack Vector
MEDIUM Angular_Improper_Type_Pipe_Usage /apps/web/src/app/billing/organizations/adjust-subscription.component.html: 18 Attack Vector
MEDIUM Client_Privacy_Violation /apps/browser/src/background/runtime.background.ts: 331 Attack Vector
MEDIUM Client_Privacy_Violation /apps/browser/src/auth/popup/account-switching/account.component.ts: 12 Attack Vector
MEDIUM Client_Privacy_Violation /apps/browser/src/auth/popup/account-switching/account.component.ts: 12 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/auth/settings/two-factor-verify.component.html: 3 Attack Vector
MEDIUM Client_Privacy_Violation /libs/components/src/color-password/color-password.component.ts: 25 Attack Vector
MEDIUM Client_Privacy_Violation /libs/components/src/color-password/color-password.component.ts: 26 Attack Vector
MEDIUM Client_Privacy_Violation /apps/desktop/src/auth/lock.component.html: 32 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/auth/lock.component.html: 18 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/shared/add-credit.component.ts: 80 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/shared/add-credit.component.ts: 30 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/shared/add-credit.component.ts: 135 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/shared/add-credit.component.ts: 146 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/billing/shared/add-credit.component.ts: 70 Attack Vector
LOW Client_DOM_Open_Redirect /apps/desktop/src/auth/accessibility-cookie.component.html: 18 Attack Vector
LOW Client_DOM_Open_Redirect /apps/browser/src/tools/popup/generator/password-generator-history.component.ts: 18 Attack Vector
LOW Client_DOM_Open_Redirect /apps/browser/src/auth/popup/account-switching/account.component.ts: 25 Attack Vector
LOW Client_DOM_Open_Redirect /apps/browser/src/auth/popup/login-via-auth-request.component.ts: 54 Attack Vector
LOW Client_DOM_Open_Redirect /apps/desktop/src/auth/login/login-via-auth-request.component.ts: 62 Attack Vector
LOW Client_DOM_Open_Redirect /apps/browser/src/auth/popup/account-switching/current-account.component.ts: 31 Attack Vector
LOW Client_DOM_Open_Redirect /apps/browser/src/auth/popup/login-via-auth-request.component.ts: 54 Attack Vector
LOW Client_DOM_Open_Redirect /apps/desktop/src/auth/login/login-via-auth-request.component.ts: 62 Attack Vector
LOW Client_DOM_Open_Redirect /apps/browser/src/vault/popup/components/vault/password-history.component.ts: 21 Attack Vector
LOW Client_DOM_Open_Redirect /apps/browser/src/vault/popup/components/vault/attachments.component.ts: 32 Attack Vector
LOW Client_DOM_Open_Redirect /apps/browser/src/popup/settings/premium.component.ts: 27 Attack Vector
LOW Client_Hardcoded_Domain /apps/web/src/app/billing/shared/payment.component.ts: 56 Attack Vector
LOW Client_Hardcoded_Domain /apps/web/src/app/billing/shared/payment.component.ts: 56 Attack Vector

# Conflicts:
#	libs/angular/src/services/jslib-services.module.ts
@LRNcardozoWDF LRNcardozoWDF marked this pull request as ready for review March 13, 2024 12:25
@LRNcardozoWDF LRNcardozoWDF requested review from a team as code owners March 13, 2024 12:25
cagonzalezcs
cagonzalezcs previously approved these changes Mar 13, 2024
Copy link
Contributor

@cagonzalezcs cagonzalezcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things are looking solid from my side, though the CipherService, FolderService, and CipherStateService classes seem to be missing Jest test coverage in a number of places. It'd be nice to address that, though I won't block the ticket for it.

Let me know if you need any further review on this.

@MGibson1 MGibson1 self-requested a review March 14, 2024 01:41
# Conflicts:
#	libs/common/src/platform/abstractions/state.service.ts
#	libs/common/src/state-migrations/migrate.ts
@MGibson1 MGibson1 removed the request for review from justindbaur March 14, 2024 11:58
audreyality
audreyality previously approved these changes Apr 11, 2024
Copy link
Contributor

@audreyality audreyality left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requested change I asked for is implemented. There's still a nit (I left it unresolved above), but it's not blocking.

cagonzalezcs
cagonzalezcs previously approved these changes Apr 11, 2024
MGibson1
MGibson1 previously approved these changes Apr 15, 2024
gbubemismith
gbubemismith previously approved these changes Apr 16, 2024
# Conflicts:
#	libs/common/src/state-migrations/migrate.ts
@LRNcardozoWDF LRNcardozoWDF removed the needs-qa Marks a PR as requiring QA approval label Apr 16, 2024
@LRNcardozoWDF LRNcardozoWDF dismissed shane-melton’s stale review April 16, 2024 16:25

It's been approved by another Vault's team member.

@LRNcardozoWDF LRNcardozoWDF merged commit 06acdef into main Apr 16, 2024
108 of 110 checks passed
@LRNcardozoWDF LRNcardozoWDF deleted the vault/pm-5273 branch April 16, 2024 16:37
MGibson1 pushed a commit that referenced this pull request Apr 22, 2024
* PM-5273 Initial migration work for localData

* PM-5273 Encrypted and Decrypted ciphers migration to state provider

* pm-5273 Update references

* pm5273 Ensure prototype on cipher

* PM-5273 Add CipherId

* PM-5273 Remove migrated methods and updated references

* pm-5273 Fix versions

* PM-5273 Added missing options

* Conflict resolution

* Revert "Conflict resolution"

This reverts commit 0c0c203.

* PM-5273 Fix PR comments

* Pm-5273 Fix comments

* PM-5273 Changed decryptedCiphers to use ActiveUserState

* PM-5273 Fix tests

* PM-5273 Fix pr comments
djsmith85 added a commit that referenced this pull request Apr 24, 2024
With CipherService using StateProviders: #8314 - we should no longer need CipherService
djsmith85 added a commit that referenced this pull request May 2, 2024
* Remove usage of getBgService for CipherService

With CipherService using StateProviders: #8314 - we should no longer need CipherService

* Remove usage of getBgService for CollectionService

With CollectionService using StateProviders: #7732 - we should no longer need CollectionService

---------

Co-authored-by: Daniel James Smith <djsmith85@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants