-
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-17665] Move cryptofunction service to km #13285
base: main
Are you sure you want to change the base?
Conversation
Great job, no security vulnerabilities found in this Pull Request |
…bitwarden/clients into km/pm-17665/move-cryptofunction-service
@@ -50,6 +50,7 @@ component_management: | |||
name: Key Management - Crypto | |||
paths: | |||
- libs/common/src/key-management/crypto/** | |||
- libs/node/src/services/** |
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.
Please also add below to key-management
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
@@ -1,4 +1,4 @@ | |||
import { CryptoFunctionService } from "../../../platform/abstractions/crypto-function.service"; | |||
import { CryptoFunctionService } from "../../../key-management/crypto/abstractions/crypto-function.service"; |
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.
Should this not be a relative import?
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.
I double checked with @Thomas-Avery since you raised the same comment on the encrypt service PR. If something is within a package - in this case libs/common/
, then we should use relative imports. In this case the import is in libs/common/src/auth/services/webauthn-login/webauthn-login-prf-key.service.ts
, importing libs/common/src/key-management/crypto/abstractions/crypto-function-service.ts
, so both are under the libs/common
package and we should use a relative import.
If we were importing cross package (i.e in libs/common
we would be importing something from libs/key-management
/@bitwarden/key-management
), then we would not use relative imports.
I don't know if this is just a key-management policy, or applies to all teams. Maybe @Thomas-Avery can chime in.
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.
Thank you for the clarification and that all tracks. I apologize for not scrutinizing these imports further than a surface level observation.
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, non blocking comment.
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.
Vault changes look good to go!
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-17665
📔 Objective
Moves cryptofunctionservice to KM ownership.
📸 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