search

brendanashworth / generate-password

NodeJS library for generating cryptographically-secure passwords.
MIT License
354 stars 67 forks source link

Adds repeatCharacterLimit option #5

Closed mleanos closed 6 years ago

mleanos commented 9 years ago

Added repeatCharacterLimit option to restrict repetitive characters to the specified amount. This option is disabled by default.

mleanos commented 9 years ago

Related to #4

I wasn't sure what to do here... https://github.com/brendanashworth/generate-password/pull/5/files#diff-d429632d5f0874d417c9e8e8cc0a3c5dR21

By requiring the repeatCharacterLimit option to be at least 2, if this option was provided, we can avoid any serious performance issues. However, it could be left to the user of this package to decide. They should understand the implications if they provided repeatCharacterLimit: 1. IMO, this wouldn't be a likely use case.

mleanos commented 9 years ago

I just realized there's an issue with this approach.

It's possible to remove 3 repeating characters from the generated password, and the result having 3 or more repeating characters...

For example: "f3dirssuuus48cDd343Dspqw" will become "f3dirsss48cDd343Dspqw"

mleanos commented 9 years ago

This should handle the issue described in my last comment.

I'm not sure how else this could be handled. https://github.com/brendanashworth/generate-password/pull/5/files#diff-d429632d5f0874d417c9e8e8cc0a3c5dR51

mleanos commented 9 years ago

@brendanashworth I addressed your line comment, and fixed a mistake in the repeatingCharacters regexp logic.

How do you like this approach overall? If you're fine with merging this, I can squash my commits beforehand.

mleanos commented 9 years ago

I didn't see your last line comment until tonight. I changed the option check to conform to the rest of the options. I also squashed.

WDYT?

brendanashworth commented 8 years ago

@mleanos sorry about dropping the ball here. I must have forgotten about this PR — I like where you're going with it, it looks great. If you wouldn't mind adding a test, I can merge this in pretty soon.

mleanos commented 8 years ago

I'll spend some time on this over the next couple of days. And no worries. I can understand how this fell under the radar.

brendanashworth commented 8 years ago

Ping @mleanos this requires a little bit of work, are you still up to implementing this?

brendanashworth commented 6 years ago

I'm going to close this, feel free to reopen if you'd like to continue this.