[PM-27591] Remove orgid in vault decryption code #17099
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
quexten
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #17099 +/- ##
==========================================
+ Coverage 40.53% 40.63% +0.09%
==========================================
Files 3531 3531
Lines 101035 101073 +38
Branches 15127 15123 -4
==========================================
+ Hits 40956 41068 +112
+ Misses 58341 58272 -69
+ Partials 1738 1733 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
djsmith85
left a comment
djsmith85
left a comment
There was a problem hiding this comment.
❓ I don't see any changes related to send models. Are the changes on the send-specs necessary
@djsmith85 Yes, one of the follow-up PR's that changes |
…den/clients into km/encstring-remove-decrypt-vault
nick-livefront
left a comment
nick-livefront
left a comment
There was a problem hiding this comment.
Much improved! I'm only requesting changes for moving the assertNonNullish within the respective try/catch. If I'm missing something there feel free to point it out!
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // We will refactor the EncString.decrypt() in https://bitwarden.atlassian.net/browse/PM-3762 to remove the dependency on the organizationId. | ||
| async decrypt(encKey: SymmetricCryptoKey): Promise<CipherView> { | ||
| async decrypt(userKeyOrOrgKey: SymmetricCryptoKey): Promise<CipherView> { | ||
| assertNonNullish(userKeyOrOrgKey, "userKeyOrOrgKey", "Cipher decryption"); |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| } | ||
|
|
||
| let folderView = await folder.decrypt(); | ||
| const userKey = await firstValueFrom(this.keyService.userKey$(activeUserId)); |
There was a problem hiding this comment.
| private async getFolder(id: string) { | ||
| let decFolder: FolderView = null; | ||
| const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); | ||
| const userKey = await firstValueFrom(this.keyService.userKey$(activeUserId)); |
There was a problem hiding this comment.
| private async createFolder(req: FolderExport) { | ||
| const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); | ||
| const userKey = await this.keyService.getUserKey(activeUserId); | ||
| const userKey = await firstValueFrom(this.keyService.userKey$(activeUserId)); |
There was a problem hiding this comment.
| } | ||
| } | ||
|
|
||
| await this.decryptObj<Cipher, CipherView>( |
There was a problem hiding this comment.
❓ should we also update the decryptObj method and remove the orgId parameter since we always pass null
There was a problem hiding this comment.
Initially it was split this wait to try to have the changes limited to vault, but that failed and we had to drag in tools / platform anyways, so you're right. It can be removed. cfe2bc8
quexten
requested review from
dereknance and
djsmith85
and removed request for
jaasen-livefront
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27591
📔 Objective
This is part of a set of changes getting rid of
decrypton EncString which must be removed to get rid of active user state. This PR specifically forces the use of the passed in key, instead of relying ondecryptimplementations to dynamically get a key based on userid / orgid.The now unused parameters will be removed in a subsequent PR.
Vault will eventually drop the typescript implementation here, but until then this updates the code to no longer use orgid / userid as parameters, so that in a later PR we can get rid of it in platform code. All that is passed through is the decryption key.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes