clients icon indicating copy to clipboard operation
clients copied to clipboard

Auth/PM-5501 - VaultTimeoutSettingsService State Provider Migration

Open JaredSnider-Bitwarden opened this issue 3 months ago • 4 comments

Type of change

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

Objective

Resolve PM-5501 by migrating the vault timeout settings out of the StateService into StateProvider with the VaultTimeoutSettingsService owning that state.

Code changes

Core VaultTimeoutSettingsService changes

  • libs/common/src/platform/state/state-definitions.ts: Add new VAULT_TIMEOUT_SETTINGS_DISK_LOCAL state definition
  • libs/common/src/services/vault-timeout/vault-timeout-settings.state.ts: Add UserKeyDefinitions for vault timeout action and vault timeout.
  • libs/common/src/services/vault-timeout/vault-timeout-settings.state.spec.ts: Test deserialization of UserKeyDefinitions
  • libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts:
    • Wire up StateProvider while significantly refactoring the retrieval logic for both the vault timeout action and vault timeout to be truly reactive and more single responsibility.
    • Also, moved towards a design which requires a user id for all sets and gets to be safer.
  • libs/common/src/services/vault-timeout/vault-timeout-settings.service.spec.ts: Update tests
  • libs/common/src/abstractions/vault-timeout/vault-timeout-settings.service.ts: Update abstraction

Dependency Updates

  • apps/cli/src/bw.ts: Move vaultTimeoutSettingsService instantiation + deps above new ApiService usage. Add VaultTimeoutSettingsService to LoginStrategyService
  • apps/browser/src/background/main.background.ts: Move vaultTimeoutSettingsService instantiation + deps above new ApiService usage. Add VaultTimeoutSettingsService to LoginStrategyService and IdleBackground
  • apps/browser/src/background/service-factories/vault-timeout-settings-service.factory.ts: Add new deps
  • apps/browser/src/auth/background/service-factories/login-strategy-service.factory.ts: Add VaultTimeoutSettingsService to LoginStrategyService
  • apps/browser/src/platform/background/service-factories/api-service.factory.ts: Add vaultTimeoutSettingsServiceFactory to ApiServiceFactory by replacing StateServiceFactory
  • apps/cli/src/platform/services/node-api.service.ts: Replace StateService dep with VaultTimeoutSettingsService
  • libs/angular/src/services/jslib-services.module.ts: Update VaultTimeoutSettingsService, ApiService, and LoginStrategyService deps

StateService cleanup

  • libs/common/src/platform/abstractions/state.service.ts: Remove vault timeout data setters and getters
  • libs/common/src/platform/services/state.service.ts: Remove vault timeout data setters and getters
  • libs/common/src/platform/models/domain/account.ts: Remove vault timeout data from account
  • libs/common/src/platform/models/domain/global-state.ts: Remove vault timeout data from global state

User Data Migration

  • libs/common/src/state-migrations/migrations/59-migrate-vault-timeout-settings-svc-to-state-provider.ts: Migrate user vault timeout data into StateProvider. Note: we do not persist global vault timeout data as it was already almost fully deprecated.
  • libs/common/src/state-migrations/migrations/59-migrate-vault-timeout-settings-svc-to-state-provider.spec.ts: Test migration
  • libs/common/src/state-migrations/migrate.ts: Register migration

Reference Updates (replacing StateService methods with calls to VaultTimeoutSettingsService)

  • apps/browser/src/background/idle.background.ts
  • apps/browser/src/popup/settings/settings.component.ts
  • apps/desktop/src/app/app.component.ts
  • apps/web/src/app/settings/preferences.component.ts
  • libs/auth/src/common/login-strategies/login.strategy.ts: (requires updates to all login strategies)
  • libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts
  • libs/auth/src/common/login-strategies/login.strategy.spec.ts
  • libs/auth/src/common/login-strategies/password-login.strategy.spec.ts
  • libs/auth/src/common/login-strategies/password-login.strategy.ts
  • libs/auth/src/common/login-strategies/sso-login.strategy.spec.ts
  • libs/auth/src/common/login-strategies/sso-login.strategy.ts
  • libs/auth/src/common/login-strategies/user-api-login.strategy.spec.ts
  • libs/auth/src/common/login-strategies/user-api-login.strategy.ts
  • libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts
  • libs/auth/src/common/login-strategies/webauthn-login.strategy.ts
  • libs/auth/src/common/services/login-strategies/login-strategy.service.spec.ts
  • libs/auth/src/common/services/login-strategies/login-strategy.service.ts
  • libs/common/src/platform/services/crypto.service.spec.ts
  • libs/common/src/platform/services/crypto.service.ts
  • libs/common/src/platform/services/system.service.ts
  • libs/common/src/services/api.service.ts
  • libs/common/src/services/vault-timeout/vault-timeout.service.ts
  • libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts

Screenshots

Note: I tested both vault timeout actions with 1 min timers on all 3 clients which support vault timeout settings (web, desktop, and browser), but didn't record as it would be 1 min of sitting per client per action.

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

JaredSnider-Bitwarden avatar Apr 03 '24 21:04 JaredSnider-Bitwarden

Logo Checkmarx One – Scan Summary & Details8fa92740-348f-40f8-aaab-4c177e489eff

No New Or Fixed Issues Found

github-actions[bot] avatar Apr 03 '24 21:04 github-actions[bot]

Codecov Report

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

Project coverage is 27.86%. Comparing base (8e97c1c) to head (fcae4f1). Report is 1 commits behind head on main.

Files Patch % Lines
...es/vault-timeout/vault-timeout-settings.service.ts 76.56% 15 Missing :warning:
.../auth/popup/settings/account-security.component.ts 0.00% 11 Missing :warning:
libs/common/src/services/api.service.ts 0.00% 9 Missing :warning:
apps/browser/src/background/main.background.ts 0.00% 8 Missing :warning:
...mponents/settings/vault-timeout-input.component.ts 0.00% 8 Missing :warning:
...pps/desktop/src/app/accounts/settings.component.ts 0.00% 7 Missing :warning:
apps/cli/src/bw.ts 0.00% 6 Missing :warning:
apps/desktop/src/app/app.component.ts 0.00% 6 Missing :warning:
...te-vault-timeout-settings-svc-to-state-provider.ts 88.67% 0 Missing and 6 partials :warning:
apps/browser/src/background/idle.background.ts 0.00% 5 Missing :warning:
... and 6 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8604      +/-   ##
==========================================
+ Coverage   27.76%   27.86%   +0.09%     
==========================================
  Files        2421     2424       +3     
  Lines       70097    70208     +111     
  Branches    13059    13085      +26     
==========================================
+ Hits        19462    19563     +101     
- Misses      49123    49126       +3     
- Partials     1512     1519       +7     

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

codecov[bot] avatar Apr 11 '24 19:04 codecov[bot]

Additional context for reviewers as to why we had to make more changes:
@justindbaur discovered that we had client specific default values on account initialization for the vault timeout. I had to rework the vault timeout retrieval to respect the client specific default. While doing so and while testing the changes, I realized that I needed to rework the default logic for the vault timeout action to ensure the previous default of lock was respected.

I tested the fresh load of a new account on web, desktop, and browser to ensure the new defaults were respected, and they were.

JaredSnider-Bitwarden avatar Apr 23 '24 21:04 JaredSnider-Bitwarden

Further notes for reviewers: we had more work to do because we discovered that the CLI wasn't setting a vault timeout so we had to enable migrations to have access to the client type they were executing on so we could successfully migrate the CLI's undefined vault timeouts to the new "never" value. I tested logging in on main in the CLI, and swapping over to this branch, and I ensured that the migration was seamless with the user's vault still being unlocked properly.

JaredSnider-Bitwarden avatar Apr 30 '24 20:04 JaredSnider-Bitwarden