-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PS-11868] Require key for enc string decryption #10981
Conversation
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.
These folder updates are example usages. We can remove from this PR to limit scope.
encryptedProperties: TEncryptedKeys[], | ||
key: SymmetricCryptoKey, | ||
encryptService: EncryptService, | ||
_: Constructor<TThis> = this.constructor as Constructor<TThis>, |
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.
The type inferrer has a lot of trouble with this
type on derived classes. It seems to always resolve to the parent class when using in the derived class. This pattern allows us to specify the specific Domain
implementation by providing a constructor as an optional 5th argument. See the Folder
changes for an example of its value.
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #10981 +/- ##
==========================================
+ Coverage 35.14% 35.16% +0.02%
==========================================
Files 2690 2690
Lines 83666 83692 +26
Branches 15887 15890 +3
==========================================
+ Hits 29403 29429 +26
Misses 53297 53297
Partials 966 966 ☔ View full report in Codecov by Sentry. |
New Issues
Fixed Issues
|
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.
Just one question on if we should throw an error sooner in one place.
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.
This looks good 🚀
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-11868
📔 Objective
Adds a new interface to enc string that requires a key and
EncryptService
to encrypt, rather than trying to determine keys and grabbing services from a global container.Additionally uses this interface through the base
Domain
class that also requires a key and encrypt service. Additionally, this new interface is type safe to decrypt onlyEncString
s on the object and avoids mutation of a passed in object.⏰ 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