-
Notifications
You must be signed in to change notification settings - Fork 47
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
[SM-1371] Expose a new generate_secret function #936
Conversation
No New Or Fixed Issues Found |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #936 +/- ##
==========================================
- Coverage 57.98% 57.80% -0.19%
==========================================
Files 197 199 +2
Lines 13647 13690 +43
==========================================
Hits 7913 7913
- Misses 5734 5777 +43 โ View full report in Codecov by Sentry. |
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.
Looks good, one thing to take a look at.
|
||
/// When set to true, the generated secret will not contain ambiguous characters. | ||
/// The ambiguous characters are: I, O, l, 0, 1 | ||
pub avoid_ambiguous: bool, // TODO: Should we rename this to include_all_characters? |
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.
pub avoid_ambiguous: bool, // TODO: Should we rename this to include_all_characters? | |
pub avoid_ambiguous: bool, |
๐ญ
If I'm understanding this comment correctly, include_all_characters
would be the inverse of avoid_ambigous
?
If include_all_characters
is set to true
I would expect I, O, l, 0, 1
to be used.
If avoid_ambigous
is set to true
I would expect I, O, l, 0, 1
NOT to be used.
My thought is to leave it the same as the PasswordGeneratorRequest
.
Regardless of what we go with, the TODO
should be removed to avoid future confusion.
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 would propose just exposing the password and passphrase generator with the same API as the password manager. That removes the maintenance burden of this functionality from the SM team and ensures it will accurately match what's available in the password manager.
## ๐๏ธ Tracking https://bitwarden.atlassian.net/browse/SM-1371 ## ๐ Objective Exposes the `password` generator function for generating secrets. This PR also fixes typos in `crates/bitwarden-generators/src/password.rs`. Alternate solution with an abstraction layer between SM and Bitwarden core generation functionality: #936 ## โฐ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## ๐ฆฎ Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - ๐ (`:+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
Closing due to alternate solution we are going with here: |
๐๏ธ Tracking
https://bitwarden.atlassian.net/browse/SM-1371
๐ Objective
Adds a new function to the
bitwarden-sm
crate, allowing the user to generate a secret value. This is just a wrapper around the internalpassword
generation functionality frombitwarden-generators
.This PR also fixes typos in
crates/bitwarden-generators/src/password.rs
.Alternate solution without the abstraction layer: #986
โฐ Reminders before review
team
๐ฆฎ 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 confirmedissue 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