-
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
[PM-15994] Move encryption to km ownership #12576
base: main
Are you sure you want to change the base?
Conversation
Fixed Issues (5)Great job! The following issues were fixed 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.
Approved for Autofill concerns
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.
SymmetricCryptoKey
EncString
EncArrayBuffer
EncryptedString
EncryptionType
seem to sometimes use relative imports. If you could please check them that would be appreciated.
libs/auth/src/common/login-strategies/webauthn-login.strategy.ts
Outdated
Show resolved
Hide resolved
libs/common/src/auth/abstractions/device-trust.service.abstraction.ts
Outdated
Show resolved
Hide resolved
libs/common/src/auth/abstractions/master-password.service.abstraction.ts
Outdated
Show resolved
Hide resolved
.../src/auth/models/response/user-decryption-options/webauthn-prf-decryption-option.response.ts
Outdated
Show resolved
Hide resolved
libs/common/src/auth/services/master-password/fake-master-password.service.ts
Outdated
Show resolved
Hide resolved
libs/common/src/auth/services/master-password/master-password.service.ts
Outdated
Show resolved
Hide resolved
9caa941
8a91f82
to
d914f60
Compare
Note: At this point the PR had to be updated to move dependencies to |
@quexten due to the size of this PR, the Auth team has had issues with tooling to properly perform a review. Could you please see if you could reasonably break this up into multiple PRs for review? Perhaps a single service at a time? |
I'll look into it. I believe in the first version of this PR it made sense to move all dependencies at once, since we were moving them to a separate package, leading to quite the mess of imports otherwise, but since it retargets the common library now, I think we can break it up. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-15994
📔 Objective
Moves a lot of the remaining crypto domain from platform to km, since km will own the corresponding sdk code in the long term too. (Further, this also cleans up some internal km ownership and file structure in the process).
Specifically transferred:
📸 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