Skip to content
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-3434] Password generator #261

Merged
merged 19 commits into from
Dec 5, 2023
Merged

Conversation

dani-garcia
Copy link
Member

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Implement password generation
The min_ fields were set to bool incorrectly I assume, so I changed them to u8.

At the moment if all the minimums turn out larger than the length, I just expand the length, but that seems wrong. I feel like it would be best to return an error in that case instead of changing any values behind the user, thoughts?

@dani-garcia dani-garcia marked this pull request as ready for review September 29, 2023 15:13
@dani-garcia dani-garcia requested a review from Hinton September 29, 2023 15:15
@bitwarden-bot
Copy link

bitwarden-bot commented Sep 29, 2023

Logo
Checkmarx One – Scan Summary & Details4e45146c-e3f6-4843-83e1-83c34ddc6a7a

No New Or Fixed Issues Found

Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good from a quick glance. I think we should try and break up the deterministic portions of the code and unit tests those. Add a few tests ensuring the generated password contains the needed characters.

crates/bitwarden/src/tool/generators/password.rs Outdated Show resolved Hide resolved
Hinton
Hinton previously approved these changes Oct 10, 2023
@audreyality
Copy link
Member

audreyality commented Oct 11, 2023

At the moment if all the minimums turn out larger than the length, I just expand the length, but that seems wrong. I feel like it would be best to return an error in that case instead of changing any values behind the user, thoughts?

Bitwarden desktop, web, and browser update the minimum password length when the minimum digit value changes. The behavior works as follows:

  • When the min digit value is greater than 9, the value is set to 9.
  • When the min special character quantity is greater than 9, the control sets its value to 9.
  • When the min digit quantity is set, [minimum password length] = [minimum digit quantity] + 2.
  • When the min special character quantity is set, if its value is greater than [minimum password length] - [minimum digit quantity], the value is set to the difference between these quantities.
  • The absolute minimum password size is 5.

This behavior was just confirmed as appropriate in PM-252, and should be preserved in your implementation.

Copy link
Member

@audreyality audreyality left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 👍 - for great changes
  • ❓ - BLOCKS REVIEW - for questions
  • ⚠️ - BLOCKS APPROVAL - for more significant problems or concerns needing attention
  • 📝 - for notes or general info
  • 💭 - for open-ended inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 - for suggestions / improvements

crates/bitwarden/src/tool/generators/password.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/tool/generators/password.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/tool/generators/password.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/tool/generators/password.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/tool/generators/password.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/tool/generators/password.rs Outdated Show resolved Hide resolved
crates/bitwarden/src/tool/generators/password.rs Outdated Show resolved Hide resolved
audreyality
audreyality previously approved these changes Oct 25, 2023
Copy link
Member

@audreyality audreyality left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just do what you're going to do.

# Conflicts:
#	crates/bitwarden/src/tool/generators/password.rs
@dani-garcia dani-garcia merged commit c006cbb into master Dec 5, 2023
39 of 40 checks passed
@dani-garcia dani-garcia deleted the ps/pm-3434-password-generator branch December 5, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants