Implement SDK generator engines #14654
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## tools/pm-18793/port-credential-generator-service-to-providers #14654 +/- ##
==================================================================================================
- Coverage 36.34% 5.76% -30.59%
==================================================================================================
Files 3183 28 -3155
Lines 92565 1736 -90829
Branches 16676 0 -16676
==================================================================================================
- Hits 33645 100 -33545
+ Misses 56491 1636 -54855
+ Partials 2429 0 -2429 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adudek-bw
changed the base branch from
main
to
tools/pm-18793/port-credential-generator-service-to-providers
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
| return new GeneratedCredential( | ||
| password, | ||
| "password", | ||
| Date.now(), |
There was a problem hiding this comment.
⛏️ This should use an injected date method; it shouldn't call Date.now() directly.
|
|
||
| return new GeneratedCredential( | ||
| password, | ||
| "password", |
There was a problem hiding this comment.
⛏️ This should be Type.password, not a hard-coded string.
|
|
||
| return new GeneratedCredential( | ||
| passphrase, | ||
| "password", |
There was a problem hiding this comment.
⛏️ This should be Type.password, not a hard-coded string.
| /** A password composed of random characters, retrieved from SDK */ | ||
| sdkPassword: "sdkpassword", | ||
|
|
||
| /** A password composed of random words from the EFF word list, retrieved from SDK */ | ||
| sdkPassphrase: "sdkpassphrase", | ||
|
|
There was a problem hiding this comment.
⛏️ Make sure the casing is consistent between the name and value
| getGeneratorDependencies(sdk: BitwardenClient): GeneratorDependencyProvider { | ||
| const provider = this.provide.generator; | ||
| provider.sdk = sdk; | ||
| return provider; | ||
| } |
There was a problem hiding this comment.
this.provide.generator is a dependency that may be shared across generator instances. Assigning sdk to it makes the SDK instance visible across all providers.
🎨 It's likely that you'll need to change CredentialGeneratorProviders so that it uses a method to create a specialized GeneratorDependencyProvider with the SDK instance.
Think broadly about how that design might work. Something that produces an observable dependency you can inject on line 114 is probably best.
|
🎟️ Tracking
📔 Objective
📸 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