From b5375cd81512e319639f1e2bee89589ad288cec8 Mon Sep 17 00:00:00 2001 From: David Voisin Date: Mon, 16 Sep 2024 15:41:24 +0200 Subject: [PATCH 1/6] [STORY-648] feat: raise DB user passwords minimum length to 24 --- db/users/create.go | 2 +- db/users/utils.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/db/users/create.go b/db/users/create.go index 1c4550bb..aa940df1 100644 --- a/db/users/create.go +++ b/db/users/create.go @@ -57,7 +57,7 @@ func CreateUser(ctx context.Context, app, addonUUID, username string, readonly b isPasswordGenerated := false if password == "" { isPasswordGenerated = true - password = gopassword.Generate(64) + password = gopassword.Generate() confirmedPassword = password } diff --git a/db/users/utils.go b/db/users/utils.go index d3be88ba..5a326014 100644 --- a/db/users/utils.go +++ b/db/users/utils.go @@ -87,8 +87,8 @@ func isPasswordValid(password, confirmedPassword string) (string, bool) { if password != confirmedPassword { return "Password confirmation doesn't match", false } - if len(password) < 8 || len(password) > 64 { - return "Password must contain between 8 and 64 characters", false + if len(password) < 24 || len(password) > 64 { + return "Password must contain between 24 and 64 characters", false } return "", true } From 1a8fec1c0a43e62a9efa9509e64ba758a0df177e Mon Sep 17 00:00:00 2001 From: David Voisin Date: Mon, 16 Sep 2024 17:00:14 +0200 Subject: [PATCH 2/6] add unit tests --- db/users/create.go | 2 +- db/users/update_password.go | 2 +- db/users/utils.go | 8 +-- db/users/utils_test.go | 108 ++++++++++++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 6 deletions(-) create mode 100644 db/users/utils_test.go diff --git a/db/users/create.go b/db/users/create.go index aa940df1..b48546cd 100644 --- a/db/users/create.go +++ b/db/users/create.go @@ -22,7 +22,7 @@ func CreateUser(ctx context.Context, app, addonUUID, username string, readonly b return nil } - if usernameValidation, ok := isUsernameValid(username); !ok { + if usernameValidation, ok := IsUsernameValid(username); !ok { io.Error(usernameValidation) return nil } diff --git a/db/users/update_password.go b/db/users/update_password.go index 6d893581..328cafa9 100644 --- a/db/users/update_password.go +++ b/db/users/update_password.go @@ -21,7 +21,7 @@ func UpdateUserPassword(ctx context.Context, app, addonUUID, username string) er return nil } - if usernameValidation, ok := isUsernameValid(username); !ok { + if usernameValidation, ok := IsUsernameValid(username); !ok { io.Error(usernameValidation) return nil } diff --git a/db/users/utils.go b/db/users/utils.go index 5a326014..c5439b4e 100644 --- a/db/users/utils.go +++ b/db/users/utils.go @@ -49,7 +49,7 @@ func askForPasswordWithRetry(ctx context.Context, remainingRetries int) (string, return "", "", errors.Wrap(ctx, err, "ask for password") } - passwordValidation, ok := isPasswordValid(password, confirmedPassword) + passwordValidation, ok := IsPasswordValid(password, confirmedPassword) if !ok { if remainingRetries == 1 { return "", "", errors.Newf(ctx, "%s. Too many retries", passwordValidation) @@ -79,7 +79,7 @@ func askForPassword(ctx context.Context) (string, string, error) { return string(password), string(confirmedPassword), nil } -func isPasswordValid(password, confirmedPassword string) (string, bool) { +func IsPasswordValid(password, confirmedPassword string) (string, bool) { if password == "" && confirmedPassword == "" { return "", true } @@ -93,9 +93,9 @@ func isPasswordValid(password, confirmedPassword string) (string, bool) { return "", true } -func isUsernameValid(username string) (string, bool) { +func IsUsernameValid(username string) (string, bool) { if len(username) < 6 || len(username) > 32 { - return "name must contain between 6 and 32 characters", false + return "Name must contain between 6 and 32 characters", false } return "", true } diff --git a/db/users/utils_test.go b/db/users/utils_test.go new file mode 100644 index 00000000..2f399d42 --- /dev/null +++ b/db/users/utils_test.go @@ -0,0 +1,108 @@ +package users_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/Scalingo/cli/db/users" +) + +func Test_IsPasswordValid(t *testing.T) { + testPasswords := map[string]struct { + password string + confirmation string + expectedValidity bool + expectedMessage string + }{ + "empty": { + password: "", + confirmation: "", + expectedValidity: true, + expectedMessage: "", + }, + "confirmation doesn't match": { + password: "abc", + confirmation: "aBc", + expectedValidity: false, + expectedMessage: "Password confirmation doesn't match", + }, + "too short": { + password: "123456789a123456789b123", + confirmation: "123456789a123456789b123", + expectedValidity: false, + expectedMessage: "Password must contain between 24 and 64 characters", + }, + "too long": { + password: "123456789a123456789b123456789c123456789d123456789e123456789f12345", + confirmation: "123456789a123456789b123456789c123456789d123456789e123456789f12345", + expectedValidity: false, + expectedMessage: "Password must contain between 24 and 64 characters", + }, + "valid, short password": { + password: "123456789a123456789b1234", + confirmation: "123456789a123456789b1234", + expectedValidity: true, + expectedMessage: "", + }, + "valid, log password ": { + password: "123456789a123456789b123456789c123456789d123456789e123456789f1234", + confirmation: "123456789a123456789b123456789c123456789d123456789e123456789f1234", + expectedValidity: true, + expectedMessage: "", + }, + } + + for name, testCase := range testPasswords { + t.Run(name, func(t *testing.T) { + message, isValid := users.IsPasswordValid(testCase.password, testCase.confirmation) + + assert.Equal(t, testCase.expectedValidity, isValid) + assert.Equal(t, testCase.expectedMessage, message) + }) + } +} + +func Test_IsUsernameValid(t *testing.T) { + testPasswords := map[string]struct { + username string + expectedValidity bool + expectedMessage string + }{ + "empty": { + username: "", + expectedValidity: false, + expectedMessage: "Name must contain between 6 and 32 characters", + }, + "too short": { + username: "12345", + expectedValidity: false, + expectedMessage: "Name must contain between 6 and 32 characters", + }, + "too long": { + username: "123456789a123456789b123456789c123", + expectedValidity: false, + expectedMessage: "Name must contain between 6 and 32 characters", + }, + "valid, short username": { + username: "123456", + expectedValidity: true, + expectedMessage: "", + }, + "valid, long username": { + username: "123456789a123456789b123456789c12", + expectedValidity: true, + expectedMessage: "", + }, + } + + for name, testCase := range testPasswords { + t.Run(name, func(t *testing.T) { + message, isValid := users.IsUsernameValid(testCase.username) + + assert.Equal(t, testCase.expectedValidity, isValid) + assert.Equal(t, testCase.expectedMessage, message) + }) + } + +} From 32ad501b35738cb3e20ce28d84b8b74f0add8512 Mon Sep 17 00:00:00 2001 From: David Voisin Date: Mon, 16 Sep 2024 17:04:06 +0200 Subject: [PATCH 3/6] linter --- db/users/utils_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/db/users/utils_test.go b/db/users/utils_test.go index 2f399d42..ce74e16a 100644 --- a/db/users/utils_test.go +++ b/db/users/utils_test.go @@ -104,5 +104,4 @@ func Test_IsUsernameValid(t *testing.T) { assert.Equal(t, testCase.expectedMessage, message) }) } - } From bb04b1c2aea17b120e01326e2bfac9a31cf13e34 Mon Sep 17 00:00:00 2001 From: David Voisin Date: Mon, 16 Sep 2024 17:17:04 +0200 Subject: [PATCH 4/6] update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 380dd769..87c5777e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ### To be Released +* feat(database/users): raise minimum user password length to 24 + ### 1.33.0 * fix(one-off): remove async field from the run command ([PR#1060](https://github.com/Scalingo/cli/pull/1060)) From 9f13e0df6b76e1531aad9a8bb181d6cb0e06c847 Mon Sep 17 00:00:00 2001 From: David Voisin Date: Tue, 17 Sep 2024 11:52:37 +0200 Subject: [PATCH 5/6] review unit tests, test in the same package --- db/users/create.go | 2 +- db/users/update_password.go | 2 +- db/users/utils.go | 6 +++--- db/users/utils_test.go | 12 +++++------- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/db/users/create.go b/db/users/create.go index b48546cd..aa940df1 100644 --- a/db/users/create.go +++ b/db/users/create.go @@ -22,7 +22,7 @@ func CreateUser(ctx context.Context, app, addonUUID, username string, readonly b return nil } - if usernameValidation, ok := IsUsernameValid(username); !ok { + if usernameValidation, ok := isUsernameValid(username); !ok { io.Error(usernameValidation) return nil } diff --git a/db/users/update_password.go b/db/users/update_password.go index 328cafa9..6d893581 100644 --- a/db/users/update_password.go +++ b/db/users/update_password.go @@ -21,7 +21,7 @@ func UpdateUserPassword(ctx context.Context, app, addonUUID, username string) er return nil } - if usernameValidation, ok := IsUsernameValid(username); !ok { + if usernameValidation, ok := isUsernameValid(username); !ok { io.Error(usernameValidation) return nil } diff --git a/db/users/utils.go b/db/users/utils.go index c5439b4e..0cb3efc0 100644 --- a/db/users/utils.go +++ b/db/users/utils.go @@ -49,7 +49,7 @@ func askForPasswordWithRetry(ctx context.Context, remainingRetries int) (string, return "", "", errors.Wrap(ctx, err, "ask for password") } - passwordValidation, ok := IsPasswordValid(password, confirmedPassword) + passwordValidation, ok := isPasswordValid(password, confirmedPassword) if !ok { if remainingRetries == 1 { return "", "", errors.Newf(ctx, "%s. Too many retries", passwordValidation) @@ -79,7 +79,7 @@ func askForPassword(ctx context.Context) (string, string, error) { return string(password), string(confirmedPassword), nil } -func IsPasswordValid(password, confirmedPassword string) (string, bool) { +func isPasswordValid(password, confirmedPassword string) (string, bool) { if password == "" && confirmedPassword == "" { return "", true } @@ -93,7 +93,7 @@ func IsPasswordValid(password, confirmedPassword string) (string, bool) { return "", true } -func IsUsernameValid(username string) (string, bool) { +func isUsernameValid(username string) (string, bool) { if len(username) < 6 || len(username) > 32 { return "Name must contain between 6 and 32 characters", false } diff --git a/db/users/utils_test.go b/db/users/utils_test.go index ce74e16a..18bc87fc 100644 --- a/db/users/utils_test.go +++ b/db/users/utils_test.go @@ -1,14 +1,12 @@ -package users_test +package users import ( "testing" "github.com/stretchr/testify/assert" - - "github.com/Scalingo/cli/db/users" ) -func Test_IsPasswordValid(t *testing.T) { +func Test_isPasswordValid(t *testing.T) { testPasswords := map[string]struct { password string confirmation string @@ -55,7 +53,7 @@ func Test_IsPasswordValid(t *testing.T) { for name, testCase := range testPasswords { t.Run(name, func(t *testing.T) { - message, isValid := users.IsPasswordValid(testCase.password, testCase.confirmation) + message, isValid := isPasswordValid(testCase.password, testCase.confirmation) assert.Equal(t, testCase.expectedValidity, isValid) assert.Equal(t, testCase.expectedMessage, message) @@ -63,7 +61,7 @@ func Test_IsPasswordValid(t *testing.T) { } } -func Test_IsUsernameValid(t *testing.T) { +func Test_isUsernameValid(t *testing.T) { testPasswords := map[string]struct { username string expectedValidity bool @@ -98,7 +96,7 @@ func Test_IsUsernameValid(t *testing.T) { for name, testCase := range testPasswords { t.Run(name, func(t *testing.T) { - message, isValid := users.IsUsernameValid(testCase.username) + message, isValid := isUsernameValid(testCase.username) assert.Equal(t, testCase.expectedValidity, isValid) assert.Equal(t, testCase.expectedMessage, message) From 898eda60ad2f6533bcfcad5f1a81725801f1000b Mon Sep 17 00:00:00 2001 From: David Voisin Date: Tue, 17 Sep 2024 11:59:06 +0200 Subject: [PATCH 6/6] add current PR in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87c5777e..04c2574f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### To be Released -* feat(database/users): raise minimum user password length to 24 +* feat(database/users): raise minimum user password length to 24 ([PR#1077](https://github.com/Scalingo/cli/pull/1077)) ### 1.33.0