-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[PM-23085] Use SDK to get rotated cipher data #15670
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
base: main
Are you sure you want to change the base?
Conversation
Great job, no security vulnerabilities found in this Pull Request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a couple of conflicts and the tools changes seemed very familiar. I assume the same work as #15337 and this PR just needs to be updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Seems like there might be parts of the importers that might check for the null
sentinel values it sets here. Did you find any?
343ab09
to
d1af2d8
Compare
Removed myself as a reviewer, as a review from tools is no longer needed, after updating from main. |
…n to better reflect intended use case
No longer impacts tools owned code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 🔥 Changes look good! I have approved the SDK PR, will approve this once you update the SDK version
@gbubemismith Updated the |
@shane-melton I think some tests are failing |
@gbubemismith 🤦 I forgot to rename the method in the cipher service spec, should be good to go now! |
@@ -32,6 +33,18 @@ export abstract class CipherEncryptionService { | |||
userId: UserId, | |||
): Promise<EncryptionContext | undefined>; | |||
|
|||
/** | |||
* Encrypts cipher for a given userId with a new key for key rotation. | |||
* @param model The cipher view encrypt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param model The cipher view encrypt | |
* @param model The cipher view to encrypt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f4d8370 Thanks!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 🚀 🚀
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15670 +/- ##
==========================================
- Coverage 37.85% 37.78% -0.08%
==========================================
Files 3309 3309
Lines 94008 94021 +13
Branches 14248 14250 +2
==========================================
- Hits 35585 35524 -61
- Misses 56924 56998 +74
Partials 1499 1499 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
PM-23085
📔 Objective
Add a new
encryptCipherForRotation
method to the cipher encryption service that utilizes the SDK to encrypt a cipher with a specific key for key rotation support.Requires SDK PR: bitwarden/sdk-internal#350
⏰ 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