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

Auth - PM-7392 & PM-7436 - Token Service - Desktop - Add disk fallback for secure storage failures #8913

Conversation

JaredSnider-Bitwarden
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden commented Apr 25, 2024

Type of change

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

Objective

To resolve PM-7392 (intermittent secure storage failures on Windows 10/11) and PM-7436 (some Linux distros do not have a secure storage provider configured so storing the access token encryption key / refresh token fails). This changeset should handle both scenarios and fallback to disk if the error is caught on initial set. If the error is caught on data retrieval after update, we will log the user out, and upon a subsequent login, the users should be able to login and have the set fallback to disk properly.

Code changes

Core TokenService changes

  • libs/common/src/auth/services/token.service.ts: Add disk fallback logic to refresh and access token methods + logic to log the user out in non-recoverable scenarios.
  • libs/common/src/auth/services/token.service.spec.ts: Add tests for new disk fallback scenarios
  • libs/common/src/auth/abstractions/token.service.ts: Update abstraction to replace undefined w/ null
  • libs/common/src/auth/enums/logout-reason.enum.ts: Add new enum to map reason for logging out to a translation to show the user in a toast
  • apps/desktop/src/locales/en/messages.json: Add translations for new error messages
  • apps/desktop/src/app/app.component.ts: Update desktop logout to delay for 5 seconds to show logout reasons in a toast if they exist.

Dependency Updates

  • apps/desktop/src/main/window.main.ts: Remove unused StateService reference so we could fix desktop main.ts circular dependency issue
  • apps/desktop/src/main.ts: Re-order deps to put the MessagingService on the TokenService.
  • apps/cli/src/bw.ts: Add the MessagingService to the TokenService.
  • apps/browser/src/background/main.background.ts: Add the MessagingService to the TokenService.
  • apps/browser/src/auth/background/service-factories/token-service.factory.ts: Add the MessagingService to the TokenService.

EncString improvements

  • libs/common/src/platform/models/domain/enc-string.ts:
    • isSerializedEncString - Add null checks
    • parseEncryptedString - Add better handling for scenarios discovered when passing in a valid JWT access token that isn't a valid encrypted string
  • libs/common/src/platform/models/domain/enc-string.spec.ts: Add tests for all isSerializedEncString scenarios

Screenshots

Logs the user out if the access token cannot be decrypted due to either the access token key not coming out of secure storage properly or secure storage not being configured

PM-7392.-.TokenSvc.Secure.Storage.Disk.Fallback.-.logs.the.user.out.if.the.access.token.cannot.be.decrypted.v2.mov

Falls back to disk if we fail to set the access token key into secure storage or if it can't be read back out

PM-7392.-.TokenSvc.Secure.Storage.Disk.Fallback.-.works.for.access.token.if.it.fails.to.set.in.secure.storage.mov

Falls back to disk properly if we fail to set the refresh token for whatever reason in secure storage or if it fails to be read back out immediately

PM-7392.-.TokenSvc.Secure.Storage.Disk.Fallback.-.works.for.refresh.token.if.it.fails.to.set.in.secure.storage.mov

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 Apr 25, 2024
Copy link

codecov bot commented Apr 25, 2024

Codecov Report

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

Project coverage is 28.08%. Comparing base (f3e780a) to head (895c825).
Report is 17 commits behind head on main.

Files Patch % Lines
apps/desktop/src/app/app.component.ts 0.00% 22 Missing ⚠️
apps/web/src/app/app.component.ts 0.00% 10 Missing ⚠️
libs/common/src/services/api.service.ts 0.00% 8 Missing ⚠️
apps/browser/src/popup/app.component.ts 0.00% 7 Missing ⚠️
apps/browser/src/background/main.background.ts 0.00% 5 Missing ⚠️
libs/angular/src/services/jslib-services.module.ts 0.00% 3 Missing ⚠️
...ibs/common/src/vault/services/sync/sync.service.ts 0.00% 3 Missing ⚠️
libs/common/src/services/notifications.service.ts 0.00% 2 Missing ⚠️
apps/cli/src/service-container.ts 66.66% 1 Missing ⚠️
apps/desktop/src/main.ts 0.00% 1 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8913      +/-   ##
==========================================
- Coverage   28.18%   28.08%   -0.10%     
==========================================
  Files        2397     2422      +25     
  Lines       70642    71347     +705     
  Branches    13203    13321     +118     
==========================================
+ Hits        19910    20038     +128     
- Misses      49172    49738     +566     
- Partials     1560     1571      +11     

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

Copy link
Contributor

github-actions bot commented Apr 25, 2024

Logo
Checkmarx One – Scan Summary & Detailsc9f8c11c-48e6-425d-8d76-eac2e6425d90

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: [1268](https://github.com/bitwarden/clients/blob/auth/pm-7392/token-service-add-secure-storage-fallback//.github/workflows/build-desktop.yml# L1268) Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: [1222](https://github.com/bitwarden/clients/blob/auth/pm-7392/token-service-add-secure-storage-fallback//.github/workflows/build-desktop.yml# L1222) Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...

Fixed Issues

Severity Issue Source File / Package
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: [1210](https://github.com/bitwarden/clients/blob/auth/pm-7392/token-service-add-secure-storage-fallback//.github/workflows/build-desktop.yml# L1210)
MEDIUM Unpinned Actions Full Length Commit SHA /build-desktop.yml: [1256](https://github.com/bitwarden/clients/blob/auth/pm-7392/token-service-add-secure-storage-fallback//.github/workflows/build-desktop.yml# L1256)

JaredSnider-Bitwarden and others added 20 commits May 1, 2024 11:07
@trmartin4 trmartin4 linked an issue May 3, 2024 that may be closed by this pull request
1 task
@trmartin4 trmartin4 linked an issue May 3, 2024 that may be closed by this pull request
1 task
jprusik
jprusik previously approved these changes May 28, 2024
jprusik
jprusik previously approved these changes Jun 3, 2024
shane-melton
shane-melton previously approved these changes Jun 3, 2024
Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

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

Vault change looks good!

@JaredSnider-Bitwarden JaredSnider-Bitwarden merged commit f691854 into main Jun 3, 2024
63 of 65 checks passed
@JaredSnider-Bitwarden JaredSnider-Bitwarden deleted the auth/pm-7392/token-service-add-secure-storage-fallback branch June 3, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Marks a PR as requiring QA approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows 10 Desktop Fails to Sync or Login Error when syncing the vault on Linux desktop
7 participants