Implement SDK generator engines #15063
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
| @@ -115,7 +116,7 @@ const SYSTEM_SERVICE_PROVIDER = new SafeInjectionToken<SystemServiceProvider>("S | |||
| }), | |||
| safeProvider({ | |||
| provide: GENERATOR_SERVICE_PROVIDER, | |||
| useFactory: ( | |||
| useFactory: async ( | |||
There was a problem hiding this comment.
🤔 I'm pretty sure that factories can't be async.
There was a problem hiding this comment.
Oops! Do you know of a way to unwrap Promise variables in a non-async factory? Or maybe which part of the code in which the feature flag can be unwrapped?
There was a problem hiding this comment.
Usually you'll want to unwrap it using the concatAsync operator in an observable pipe. The contributing docs has a complete list of options. Ignore the subscribe framing. The important part is the synchronous context.
libs/tools/generator/core/src/metadata/password/sdk-eff-word-list.ts
Outdated
Show resolved
Hide resolved
libs/tools/generator/core/src/metadata/password/sdk-random-password.ts
Outdated
Show resolved
Hide resolved
| private metadataBySdkFlag( | ||
| algorithm: GeneratorMetadata<unknown & object>, | ||
| ): GeneratorMetadata<unknown & object> { | ||
| if (this.useSdkService) { | ||
| if (algorithm.id == "password") { | ||
| return sdkPassword; | ||
| } else if (algorithm.id == "passphrase") { | ||
| return sdkPassphrase; | ||
| } | ||
| } | ||
| return algorithm; | ||
| } |
There was a problem hiding this comment.
💭 Most of the code in the metadata class is pretty high-level. It doesn't work directly with algorithms. It inspects types and directs lookups to externally-provided data. This code stands out because it's so low-level.
♻️ It's fully encapsulated, and the SDK doesn't have data to push into the provider, so this is totally cool. We'll just need to think about how to upgrade it when we start interning the metadata into the SDK.
🤔 Or maybe the trouble is that we should be swapping the engines during create on the stock password metadata.
❓ Thoughts?
There was a problem hiding this comment.
Yeah it def is pretty low level compared to the rest of the code. Ohhh, swapping them out at the metadata is an interesting idea. Would that mean that the creates functions for the non-sdk engines would check to see if the feature flag is enabled and if so create the sdk engine?
There was a problem hiding this comment.
I haven't thought out how it would work whatsoever. It would certainly need to be aware which mode it's using in order to know which version to create, but it wouldn't necessarily need the feature flag to do that. You could, for example, conditionally load the SDK based on the flag. If the SDK is loaded, it uses the SDK. Otherwise it uses the builtin version.
If you really want to get wild with it, you could lower it into the engine.
These ideas are all half-baked. If any of them sound feasible you can pursue them, but it's not necessary.
There was a problem hiding this comment.
Ahhhhh I like that
…ist.ts Co-authored-by: ✨ Audrey ✨ <[email protected]>
…sword.ts Co-authored-by: ✨ Audrey ✨ <[email protected]>
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-19976
📔 Objective
Hook up new SDK password/passphrase generation engines and put them behind a feature flag.
📸 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