Add capitalized words to memorable passwords #1985
Conversation
Memorable passwords generated with --strict will now have at least one capitalized word. Closes #1984 RELEASE_NOTES=[FEATURE] Add capitalized words to memorable passwords Signed-off-by: Elijah J. Passmore <elijah@elijahjpassmore.com>
There was a problem hiding this comment.
@dominikschulz I'd love to have your input regarding the possibility of WriteString to error out and us not forwarding it currently. Should we also refactor the Generate*Password functions to return (string, error) and get rid of the nil in the return ..., nil instead?
| @@ -4,16 +4,34 @@ import "strings" | |||
|
|
|||
| // GenerateMemorablePassword will generate a memorable password | |||
| // with a minimum length | |||
| func GenerateMemorablePassword(minLength int, symbols bool) string { | |||
| func GenerateMemorablePassword(minLength int, symbols bool, capitals bool) string { | |||
There was a problem hiding this comment.
That's a public API change, but it seems only Gopass and Gopass JSON-API (which don't use that function) is using this package currently, so that should be fine.
We might want to increment a minor version instead of a patch version because of that tho.
There was a problem hiding this comment.
We should probably make it a minor bump, yes. But otherwise I wouldn't be concerned about this change. This package is only used by gopass itself and gopass-jsonapi: https://pkg.go.dev/github.com/gopasspw/gopass@v1.12.8/pkg/pwgen?tab=importedby
That being said I also wouldn't set the bar too high here. This package doesn't have a very good interface anyway and is likely due for a major redesign at some point. The interfaces aren't very consistent. Cryptic uses an object while the others can only be called directly. Either all methods should be static or all object methods where the configuration goes into the object. Maybe with static helpers that use the most common settings, like the http package does.
| if c.Bool("strict") { | ||
| return pwgen.GenerateMemorablePassword(pwlen, symbols, true), nil | ||
| } | ||
| return pwgen.GenerateMemorablePassword(pwlen, symbols, false), nil |
There was a problem hiding this comment.
@dominikschulz actually we're using WriteString in there which can return an error... Why aren't we returning errors in the Generate*Password functions and instead always return nil here??
There was a problem hiding this comment.
It's signature has an error argument, yes.
But at least with the current implementation it will never return an error: https://cs.opensource.google/go/go/+/refs/tags/go1.17.1:src/strings/builder.go;l=125;drc=43652dc46f770253b3603f47165b1568b439b0b5
|
Thank you for the contribution @elijahjpassmore, that's really appreciated. I've just got a few comments, if you could review and change things accordingly, that'd be great. |
Co-authored-by: Yolan Romailler <AnomalRoil@users.noreply.github.com>
Signed-off-by: Elijah J. Passmore <elijah@elijahjpassmore.com>
Co-authored-by: Yolan Romailler <AnomalRoil@users.noreply.github.com>
|
Thanks @AnomalRoil, I've made the changes you suggested. If you have any other issues or questions, feel free to let me know. |
| if c.Bool("strict") { | ||
| return pwgen.GenerateMemorablePassword(pwlen, symbols, true), nil | ||
| } | ||
| return pwgen.GenerateMemorablePassword(pwlen, symbols, false), nil |
There was a problem hiding this comment.
It's signature has an error argument, yes.
But at least with the current implementation it will never return an error: https://cs.opensource.google/go/go/+/refs/tags/go1.17.1:src/strings/builder.go;l=125;drc=43652dc46f770253b3603f47165b1568b439b0b5
| @@ -4,16 +4,34 @@ import "strings" | |||
|
|
|||
| // GenerateMemorablePassword will generate a memorable password | |||
| // with a minimum length | |||
| func GenerateMemorablePassword(minLength int, symbols bool) string { | |||
| func GenerateMemorablePassword(minLength int, symbols bool, capitals bool) string { | |||
There was a problem hiding this comment.
We should probably make it a minor bump, yes. But otherwise I wouldn't be concerned about this change. This package is only used by gopass itself and gopass-jsonapi: https://pkg.go.dev/github.com/gopasspw/gopass@v1.12.8/pkg/pwgen?tab=importedby
That being said I also wouldn't set the bar too high here. This package doesn't have a very good interface anyway and is likely due for a major redesign at some point. The interfaces aren't very consistent. Cryptic uses an object while the others can only be called directly. Either all methods should be static or all object methods where the configuration goes into the object. Maybe with static helpers that use the most common settings, like the http package does.
Memorable passwords generated with --strict will now have at least one capitalized word. Closes gopasspw#1984 RELEASE_NOTES=[FEATURE] Add capitalized words to memorable passwords Signed-off-by: Elijah J. Passmore <elijah@elijahjpassmore.com> Co-authored-by: Yolan Romailler <AnomalRoil@users.noreply.github.com>
Memorable passwords generated with --strict will now have at least one
capitalized word.
Closes #1984
RELEASE_NOTES=[FEATURE] Add capitalized words to memorable passwords