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-5263 - TokenService State Provider Migration #7975

Merged

Conversation

JaredSnider-Bitwarden
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden commented Feb 16, 2024

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-5263 by migrating all TokenService owned data to the StateProvider framework while adding tests and refactoring as necessary.

Resolve PM-3566 by storing the access token and refresh token in secure storage for clients that support it instead of disk.

Resolve PM-6212 by migrating away from a single global state entry for 2FA tokens for n users to a global Record<email, TwoFactorTokens>

Code changes

State Provider Implementation

  • libs/common/src/platform/state/state-definitions.ts: Define token state definitions (disk, disk + web local storage, and memory required)
  • libs/common/src/auth/services/token.state.ts: Define all KeyDefinitions for TokenService state
    • Note: it is important to recognize the EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL and how the new implementation for 2FA tokens differs from the old implementation. We now have a global record which maps user emails to their respective 2FA tokens. This is a large improvement over the existing implementation which only has a single global entry for 2FA token and no corresponding user information associated with it. This will resolve issues with incorrect 2FA tokens being submitted when there are multiple users on the same computer (rare, but possible w/ the current implementation)
  • libs/common/src/auth/services/token.state.spec.ts: Token state tests which check to ensure deserialization for each KeyDefinition works
  • libs/common/src/auth/services/token.service.ts:
    • Updated TokenService to implement all get/sets/clears for access token, refresh token, 2FA token, API key client id, API key client secret
      • Note: this required accepting in VaultTimeoutAction and VaultTimeout data for set logic to avoid circular dependency issues with the VaultTimeoutSettingsService. We plan on re-evaluating this approach once the VaultTimeoutSettingService is migrated to StateProvider in Auth/PM-5501 - VaultTimeoutSettingsService State Provider Migration #7925 (it's blocked by this work). We are considering sharing KeyDefinitions between the two services so that the methods can just directly access the VaultTimeoutAction and VaultTimeout from StateProvider directly as the intermediary service.
    • Added support for storing Access Token and Refresh Token in secure storage if it is supported on the platform. Data will be slowly migrated from disk to secure storage as they use the application (on token refresh).
    • Added a DecodedAccessToken custom type so we can have typing on our Bitwarden Access Token
    • Note: we cannot use ActiveStateProvider due to circular dependency issues. We got around that by using the GlobalStateProvider and sharing the ACCOUNT_ACTIVE_ACCOUNT_ID KeyDefinition.
    • Updated clearTokens(...) to leverage new, single responsibility clear methods for each piece of state that we needed to clear
    • Extracted static TokenService.decodeToken method into auth owned, generic utility func decodeJwtTokenToJson as we needed the ability to decode non-Bitwarden owned access tokens in the libs/importer/src/importers/lastpass/access/vault.ts logic.
  • libs/common/src/auth/abstractions/token.service.ts: Update abstraction and document all behaviors.
  • libs/common/src/auth/services/token.service.spec.ts: Add tests for every code branch in TokenService
  • libs/common/src/platform/models/domain/account.ts: Remove Token state from Account
  • libs/common/src/platform/services/state.service.ts && libs/common/src/platform/abstractions/state.service.ts
    • Remove all access token, refresh token, 2FA token, API key client id, API key client secret methods as they will now be owned by the TokenService
    • Remove now unnecessary getTimeoutBasedStorageOptions
    • Inject TokenService to support methods that still require access token state.
  • libs/common/src/services/api.service.ts:
    • Add StateService
    • Clear 2FA token by passing in email
    • Update all modified TokenService method calls
  • libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts:
    • Refactor naming and update to changed method names of TokenService
    • Refactor to use setTokens
  • libs/auth/src/common/login-strategies/
    • login.strategy.spec.ts
    • user-api-login.strategy.spec.ts
    • user-api-login.strategy.ts
    • webauthn-login.strategy.spec.ts
    • sso-login.strategy.spec.ts
    • auth-request-login.strategy.spec.ts
    • password-login.strategy.spec.ts
      • Update login strategy tests to take new TokenService implementation into account.

Dependency Updates

  • apps/browser/src/auth/background/service-factories/token-service.factory.ts: Add new dependencies to token service factory.
  • apps/web/src/app/core/state/state.service.ts: Inject TokenService to web StateService
  • libs/angular/src/services/injection-tokens.ts: Create new injection token for tracking whether or not a platform supports secure storage in order to avoid having to import the PlatformUtilsService into the TokenService which creates a circular dependency issue on desktop specifically.
  • libs/angular/src/services/jslib-services.module.ts:
    • Register new SUPPORTS_SECURE_STORAGE injection token and let it use the platformUtilsService.supportsSecureStorage() method generally to get its value.
    • Update TokenService deps - Adding StateProvider + SecureStorageService + SUPPORTS_SECURE_STORAGE injection token
    • Update ApiService deps - Adding StateService
    • Update StateService deps - Adding TokenService
  • apps/browser/src/background/main.background.ts:
    • Refactor order of instantiations as TokenService required PlatformUtilsService to be implemented first
    • Inject StateService into ApiService
  • apps/browser/src/autofill/browser/main-context-menu-handler.ts: Add platformUtilsServiceOptions because StateService now requires TokenService which requires platformUtilsServiceOptions in order to know how to create it.
  • apps/browser/src/platform/background/service-factories/api-service.factory.ts: Add stateServiceFactory
  • apps/browser/src/platform/background/service-factories/state-service.factory.ts: Add tokenServiceFactory
  • apps/browser/src/platform/services/browser-state.service.ts: Add TokenService to BrowserStateService
  • apps/browser/src/platform/services/browser-state.service.spec.ts: Add TokenService to BrowserStateService tests
  • apps/browser/src/popup/services/services.module.ts: Remove getBgService<TokenService> logic as no longer required due to StateProvider implementation
  • apps/cli/src/bw.ts:
    • Refactor TokenService instantiation with new deps and place before StateService
    • Add StateService to ApiService
  • apps/cli/src/platform/services/node-api.service.ts: Add StateService to NodeApiService

Extracted decodeJwtTokenToJson(...) Utility Function from TokenService

  • libs/auth/src/common/utilities/decode-jwt-token-to-json.utility.ts: Add generic JWT to JSON func with improved error handling
  • libs/auth/src/common/utilities/decode-jwt-token-to-json.utility.spec.ts: Test function
  • libs/auth/src/common/utilities/index.ts: Add barrel file for utilities
  • libs/auth/src/common/index.ts: - Add utilities barrel file so utilities are available in auth/common imports
  • apps/desktop/src/app/services/services.module.ts:
    • Override default value of SUPPORTS_SECURE_STORAGE injection token by referencing new constant ELECTRON_SUPPORTS_SECURE_STORAGE defined in the ElectronPlatformUtilsService
    • Inject TokenService into ElectronStateService
  • apps/desktop/src/main.ts:
    • Refactor singleUserStateProvider and activeUserStateProvider out into constants similar to globalStateProvider
    • Add TokenService with proper dependencies for desktop
  • apps/desktop/src/platform/services/electron-platform-utils.service.ts: Add new exported const for ELECTRON_SUPPORTS_SECURE_STORAGE

State Migrations

  • libs/common/src/state-migrations/migrations/35-migrate-token-svc-to-state-provider.ts: Add migration to move all token state to state provider
  • libs/common/src/state-migrations/migrations/35-migrate-token-svc-to-state-provider.spec.ts: Add tests for migration and rollback
  • libs/common/src/state-migrations/migrate.ts: Register migrator
  • libs/common/src/state-migrations/migrations/3-fix-premium.ts Deleted old migration as it referenced deleted the access token on the Account (which was removed) per discussion with Platform.
  • libs/common/src/state-migrations/migrations/3-fix-premium.spec.ts: Deleted old migration tests

2FA Token Changes to support new state structure of Record<email, TwoFactorToken>

  • libs/common/src/auth/services/sso-login.service.ts: Add SSO_EMAIL to persist user entered email through SSO process on all clients + fix class not implementing its abstraction.
  • libs/common/src/auth/abstractions/sso-login.service.abstraction.ts: Add and document getSsoEmail and setSsoEmail methods
  • apps/browser/src/auth/popup/login.component.ts: Save off email on user clicking SSO button
  • libs/angular/src/auth/components/login.component.ts: Save off email on user clicking SSO button
  • libs/angular/src/auth/components/sso.component.ts: On load of SSO component (after SSO redirect), get user email from SsoLoginService and pass into base
  • libs/auth/src/common/login-strategies/login.strategy.ts: - The new tokenService.getTwoFactorToken(email) method now requires email so we must get email as a new param to buildTwoFactor(...) in order to check if they have a saved 2FA token.
  • libs/auth/src/common/models/domain/login-credentials.ts: Add email to SsoLoginCredentials
  • apps/cli/src/auth/commands/login.command.ts: Pass undefined in for email for new SsoLoginCredentials
  • libs/auth/src/common/login-strategies/auth-request-login.strategy.ts: Update AuthRequestLoginStrategy to pass email into base login strategy for use with looking up 2FA Token.
  • libs/auth/src/common/login-strategies/password-login.strategy.ts: Update PasswordLoginStrategy to pass email into base login strategy for use with looking up 2FA Token.
  • libs/auth/src/common/login-strategies/sso-login.strategy.ts: Update SsoLoginStrategy to pass email into base login strategy for use with looking up 2FA Token.

LastPass Importer Updates

  • libs/importer/src/importers/lastpass/access/vault.ts: - Remove TokenService.decodeToken and replace with decodeJwtTokenToJson utility function; remove TokenService from Vault.ts
  • libs/importer/src/components/lastpass/lastpass-direct-import.service.ts: Remove TokenService dependency from LastPassDirectImportService as it is no longer required - only Vault.ts used it.

Screenshots

Web

MP Login works

PM-5263.-.TokenSvc.State.Provider.Migration.-.Web.Login.works.mov

MP Login - Remember 2FA Token works

PM-5263.-.Web.-.Standard.Login.-.Remember.2FA.Token.Works.mov

Login with Device - Remember 2FA Token works

PM-5263.-.Web.-.Login.with.Device.-.Remember.2FA.Token.Works.mov

SSO Login - Remember 2FA Token works

PM-5263.-.Web.-.SSO.Login.-.Remember.2FA.Token.Works.mov

Browser Extension

MP Login works

PM-5263.-.TokenSvc.State.Provider.Migration.-.Browser.Extension.Login.works.mov

MP Login - Remember 2FA Token works

PM-5263.-.Browser.Extension.-.MP.Login.-.Remember.2FA.Token.Works.mov

Login with Device - Remember 2FA Token works

PM-5263.-.Browser.Extension.-.Login.with.device.-.Remember.2FA.Token.Works.mov

SSO Login - Remember 2FA Token works

PM-5263.-.Browser.Extension.-.SSO.Login.-.Remember.2FA.Token.Works.mov

Desktop

MP Login works

PM-5263.-.TokenSvc.State.Provider.Migration.-.Desktop.Login.works.mov

MP Login - Remember 2FA Token works

PM-5263.-.Desktop.-.MP.Login.-.Remember.2FA.Token.Works.mov

SSO Login - Remember 2FA Token works

PM-5263.-.Desktop.-.SSO.Login.-.Remember.2FA.Token.Works.mov

CLI

MP Login works

image

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

…itions setup (2) Ported over core state service getTimeoutBasedStorageOptions method logic into local determineStorageLocation method (3) Updated majority of methods to use state provider state
@JaredSnider-Bitwarden JaredSnider-Bitwarden changed the title PM-5263 - Token Service state migration - (1) Got key and state defin… PM-5263 - Token Service state migration Feb 16, 2024
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Feb 16, 2024
@JaredSnider-Bitwarden JaredSnider-Bitwarden changed the title PM-5263 - Token Service state migration Auth/PM-5263 - TokenService State Provider Migration Feb 16, 2024
…other state methods after migration code complete.
…ve user id as it wasn't used and it simplifies the new state provider implementation (2) Convert away from state svc to state provider state.
@bitwarden-bot
Copy link

bitwarden-bot commented Feb 16, 2024

Logo
Checkmarx One – Scan Summary & Detailsc562d38d-4307-49ef-921d-685a8353bf66

No New Or Fixed Issues Found

…Svc and TokenService: (1) For writes, require callers to pass in vault timeout data (2) For reads, we can just check both locations. This approach has 1 less state call than the previous implementation and is safe as long as the clear logic properly works and is executed anytime a user changes their vault timeout action (lock or log out) & vault timeout (numeric value)
…meout details and pass to token service when setting token info.
…rvice-state-provider-migration + state definitions merge conflict
…required by state service (2) TokenSvc - Update getToken to take an optional userId to handle another state service case (3) Add some documentation to TokenSvc abstraction.
…ervice which accessed token service state directly with calls to the new token service methods instead.
…action and vault timeout from state service in order to pass to new token service endpoints for setting API key client id and secret.
… remove account scaffold logic for clearing removed account data. The same functionality will exist in the state provider framework via lifecycle hooks cleaning up this data and users getting initialized with null data by default.
…that I missed initially to get browser building.
…Token, and clearRefreshToken based on PR feedback to remove optional user ids where possible and improve public interface (2) TokenSvc Abstraction - update docs and abstractions based on removed user ids and changed logic (3) TokenSvc tests - update tests to add new test cases, remove no longer relevant ones, and update test names.
…rvice-state-provider-migration + migrate.ts merge conflicts
jlf0dev
jlf0dev previously approved these changes Mar 14, 2024
Copy link
Member

@jlf0dev jlf0dev left a comment

Choose a reason for hiding this comment

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

Great work!

cagonzalezcs
cagonzalezcs previously approved these changes Mar 14, 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.

Providing an approval with some comments, let me know if you need a re-review.

…rvice-state-provider-migration + main.ts merge conflict resolution.
djsmith85
djsmith85 previously approved these changes Mar 15, 2024
cagonzalezcs
cagonzalezcs previously approved these changes Mar 15, 2024
…rvice-state-provider-migration + merge conflict resolution in migrate.ts
…rvice-state-provider-migration + migrate merge conflict resolution
@JaredSnider-Bitwarden JaredSnider-Bitwarden removed the needs-qa Marks a PR as requiring QA approval label Mar 15, 2024
@JaredSnider-Bitwarden
Copy link
Contributor Author

Per discussion with Auth team, we will be QA'ing this in main.

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