[PM-3726] Force migration of legacy user's encryption key #6195
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
New Issues
Fixed Issues
|
apps/web/src/app/auth/migrate-encryption/migrate-legacy-encryption.component.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/auth/migrate-encryption/migrate-legacy-encryption.component.ts
Show resolved
Hide resolved
apps/web/src/app/auth/migrate-encryption/migrate-legacy-encryption.component.ts
Show resolved
Hide resolved
apps/web/src/app/auth/migrate-encryption/migrate-legacy-encryption.service.spec.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/auth/migrate-encryption/migrate-legacy-encryption.service.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/auth/migrate-encryption/migrate-legacy-encryption.service.ts
Show resolved
Hide resolved
JaredSnider-Bitwarden
previously approved these changes
Sep 6, 2023
apps/web/src/app/auth/migrate-encryption/migrate-legacy-encryption.component.ts
Show resolved
Hide resolved
- also remove card in vault since legacy users can't login
jlf0dev
changed the title
justindbaur
previously approved these changes
Sep 6, 2023
There was a problem hiding this comment.
Platform changes look good, just one question for @djsmith85's expertise on locale file stuff.
| @@ -475,8 +475,8 @@ | |||
| "maxFileSize": { | |||
| "message": "Maximum file size is 500 MB." | |||
| }, | |||
| "updateKey": { | |||
| "message": "You cannot use this feature until you update your encryption key." | |||
| "encryptionKeyMigrationRequired": { | |||
There was a problem hiding this comment.
@djsmith85 What's the rule on updating locale keys again? Should we avoid changing the key or is this correct when we also update the message?
There was a problem hiding this comment.
@justindbaur Thanks for the ping.
If the message is changed then the key need to be changed for the translations to be removed in Crowdin. Additionally we need to check if the previous key was used elsewhere/possibly within a different context and also update all usages within the clients.
gbubemismith
previously approved these changes
Sep 6, 2023
cagonzalezcs
previously approved these changes
Sep 6, 2023
Comment on lines
+20
to
+27
| <bit-label>{{ "masterPass" | i18n }}</bit-label> | ||
| <input | ||
| id="masterPassword" | ||
| bitInput | ||
| type="password" | ||
| formControlName="masterPassword" | ||
| appAutofocus | ||
| /> |
There was a problem hiding this comment.
Accessibility Nit - Does this <bit-label> component directly relate to the subsequent <input> label somehow? Ideally the label for the form would have a for="<id> value for each input field (ie. for="masterPassword in this case)
There was a problem hiding this comment.
Based on this comment from @shane-melton, the bit-label does not need/support a for-attribute. The bit-form-field takes care of it
Comment on lines
108
to
115
| const allowedStatuses = [ | ||
| EmergencyAccessStatusType.Confirmed, | ||
| EmergencyAccessStatusType.RecoveryInitiated, | ||
| EmergencyAccessStatusType.RecoveryApproved, | ||
| ]; | ||
|
|
||
| const filteredAccesses = emergencyAccess.data.filter((d) => allowedStatuses.includes(d.status)); | ||
|
|
There was a problem hiding this comment.
Very minor nit here, but in cases where I'm attempting to identify if a value exists within an array of values, using a Set data type is usually more performant.
Suggested change
| const allowedStatuses = [ | |
| EmergencyAccessStatusType.Confirmed, | |
| EmergencyAccessStatusType.RecoveryInitiated, | |
| EmergencyAccessStatusType.RecoveryApproved, | |
| ]; | |
| const filteredAccesses = emergencyAccess.data.filter((d) => allowedStatuses.includes(d.status)); | |
| const allowedStatuses = new Set([ | |
| EmergencyAccessStatusType.Confirmed, | |
| EmergencyAccessStatusType.RecoveryInitiated, | |
| EmergencyAccessStatusType.RecoveryApproved, | |
| ]); | |
| const filteredAccesses = emergencyAccess.data.filter((d) => allowedStatuses.has(d.status)); | |
.has() identifies whether a value exists in O(1) runtime, while .includes() identifies values in O(n).
Not a blocking request, just a consideration.
djsmith85
requested changes
Sep 6, 2023
jlf0dev
dismissed stale reviews from cagonzalezcs, gbubemismith, and justindbaur
via
cdbe93c
JaredSnider-Bitwarden
previously approved these changes
Sep 6, 2023
Hinton
reviewed
Sep 7, 2023
| >{{ "learnMore" | i18n }}</a | ||
| > | ||
| </p> | ||
| <app-callout type="warning">{{ "updateEncryptionKeyWarning" | i18n }}</app-callout> |
There was a problem hiding this comment.
Should be using the bit callout component instead of the deprecated app-callout.
I wanted to avoid running this code for every route so I've only put it on the lock guard and a direct navigation on the login component. Legacy users will be sent to the lock guard anyway if they try to get around the migration.
The direction we're headed is that you shouldn't get past auth without having a User Key. Regardless, the Auth guard checks for this and redirects to the lock component if the User Key isn't found. |
Hinton
previously approved these changes
Sep 7, 2023
djsmith85
previously approved these changes
Sep 18, 2023
djsmith85
approved these changes
Sep 18, 2023
1 task
1 task
Hinton
reviewed
Sep 19, 2023
| @@ -141,10 +142,18 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { | |||
| } | |||
|
|
|||
| private async migrateKeyForNeverLockIfNeeded(): Promise<void> { | |||
| // Web can't set vault timeout to never | |||
Hinton
approved these changes
Sep 19, 2023
gbubemismith
approved these changes
Sep 19, 2023
trmartin4
pushed a commit
that referenced
this pull request
Sep 21, 2023
* [PM-3726] migrate legacy user's encryption key * [PM-3726] add 2fa support and pr feedback * [PM-3726] revert launch.json & webpack.config changes * [PM-3726] remove update key component - also remove card in vault since legacy users can't login * [PM-3726] Fix i18n & PR feedback * [PM-3726] make standalone component * [PM-3726] linter * [PM-3726] missing await * [PM-3726] logout legacy users with vault timeout to never * [PM-3726] add await * [PM-3726] skip auto key migration for legacy users * [PM-3726] pr feedback * [PM-3726] move check for web into migrate method --------- Co-authored-by: Jared Snider <[email protected]> (cherry picked from commit 8c06508)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of change
Objective
Legacy encryption users have been unable to login following the key migration. Although we included code to account for legacy accounts within the crypto service, we made the assumption during the login process that a user key should always be available.
The PR detects these accounts and creates a new migration component for them. This new migration component is only available on web currently, so other clients will have their login process short circuited with an error directing them to migrate on web.
I have also removed references to the old
UpdateKeyComponentand the card on the vault page. The migration is now required.Code changes
Screenshots
Before you submit