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

[Key Connector] Add support for key connector and OTP #2156

Merged
merged 14 commits into from
Nov 9, 2021

Conversation

eliykat
Copy link
Member

@eliykat eliykat commented Nov 3, 2021

Type of change

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

Objective

Add support for user verification using OTP instead of master password. Required to support the crypto agent.

Equivalent changes in web for reference: bitwarden/web#1256.

Code changes

  • src/background/main.background.ts - add KeyConnectorService, update deps
  • src/popup/accounts/remove-password.component.(html|ts) - add remove password component - browser needs its own template, it otherwise just uses the jslib base class
  • src/popup/accounts/sso.component.ts - this onSuccessfulLogin callback runs after the user successfully authenticates with SSO. Currently, this refreshes all open tabs/windows belonging to the extension - I'm not sure why, but I assume it's so that it'll display the unlock page properly. With Key Connector, the user is already unlocked and all this does is dump the keys out of memory, locking the vault again. So: don't refresh if the vault is already unlocked.
  • src/popup/app-routing.module.ts - update routing to include remove-password.component.
  • src/popup/app.component.ts - receives the convertAccountToKeyConnector message sent by syncService. However, sync occurs in the background, so we need some way to persist this message when the user opens the popup. We do this by saving to storageService and checking for it in the CanActivate guard (which is in the jslib PR). If it's detected by CanActivate, then the user is taken to the remove password component.
  • src/popup/app.module.ts - update components, order components alphabetically
  • src/popup/settings/export.component.html - update to use VerifyMasterPasswordComponent if required (which adds OTP support), update to use reactive forms (per changes in jslib)
  • src/popup/settings/settings.component.(html|ts) - hide "Change Master Password" if using Key Connector
  • src/popup/vault/add-edit.component.(html|ts) - hide "Master Password Reprompt" option if using Key Connector

Screenshots

@Hinton can you please provide?

Testing requirements

General testing requirements for crypto-agent changes to be provided to QA.

Before you submit

  • I have checked for linting errors (npm run lint) (required)
  • This change requires a documentation update (notify the documentation team)
  • This change has particular deployment requirements (notify the DevOps team)

@Hinton Hinton marked this pull request as ready for review November 5, 2021 13:32
@eliykat eliykat changed the title Add support for crypto-agent OTP [Key Connector] Add support for key connector and OTP Nov 8, 2021
Copy link
Member Author

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

@Hinton a couple of comments below. Nice solution to the 'state' problem I think.

  • Did you mean to bump jslib on this branch before the jslib PR is merged?

src/popup/app-routing.module.ts Outdated Show resolved Hide resolved
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.

3 participants