From 9d2c93a1cd28cbae77f72d8b48499cef79cd123e Mon Sep 17 00:00:00 2001 From: Aryan Bhokare <92683836+aryan-bhokare@users.noreply.github.com> Date: Thu, 27 Jun 2024 21:21:24 +0530 Subject: [PATCH] Added strict validation for username and password in backend. (#4670) * Added strict validation for username and password in backend. Signed-off-by: aryan * fixed silly mistake Signed-off-by: aryan * some requested changes. Signed-off-by: aryan * modified tests Signed-off-by: aryan * small change Signed-off-by: aryan * Update message string in chaoscenter/authentication/api/handlers/doc.go Co-authored-by: Vedant Shrotria Signed-off-by: Aryan Bhokare <92683836+aryan-bhokare@users.noreply.github.com> * Update error message chaoscenter/authentication/pkg/utils/sanitizers.go Co-authored-by: Vedant Shrotria Signed-off-by: Aryan Bhokare <92683836+aryan-bhokare@users.noreply.github.com> * Modified swagger with requested changes. Signed-off-by: aryan * added negative tests for requested functions and fixed some conflicts. Signed-off-by: aryan --------- Signed-off-by: aryan Signed-off-by: Aryan Bhokare <92683836+aryan-bhokare@users.noreply.github.com> Co-authored-by: Vedant Shrotria Co-authored-by: Saranya Jena --- chaoscenter/authentication/api/docs/docs.go | 21 +++++++- .../authentication/api/docs/swagger.json | 21 +++++++- .../authentication/api/docs/swagger.yaml | 19 ++++++- .../authentication/api/handlers/doc.go | 7 ++- .../api/handlers/rest/user_handlers.go | 18 +++++-- .../api/handlers/rest/user_handlers_test.go | 54 +++++++++++++++---- .../authentication/pkg/utils/errors.go | 5 +- .../authentication/pkg/utils/sanitizers.go | 26 +++++++-- 8 files changed, 148 insertions(+), 23 deletions(-) diff --git a/chaoscenter/authentication/api/docs/docs.go b/chaoscenter/authentication/api/docs/docs.go index 72808deae4f..0ca1a140a93 100644 --- a/chaoscenter/authentication/api/docs/docs.go +++ b/chaoscenter/authentication/api/docs/docs.go @@ -940,6 +940,12 @@ const docTemplate = `{ "$ref": "#/definitions/response.ErrInvalidRequest" } }, + "401": { + "description": "Unauthorized", + "schema": { + "$ref": "#/definitions/response.ErrStrictUsernamePolicyViolation" + } + }, "500": { "description": "Internal Server Error", "schema": { @@ -1214,7 +1220,20 @@ const docTemplate = `{ }, "message": { "type": "string", - "example": "Please ensure the password is 8 characters long and has 1 digit, 1 lowercase alphabet, 1 uppercase alphabet and 1 special character" + "example": "Please ensure the password is atleast 8 characters and atmost 16 characters long and has atleast 1 digit, 1 lowercase alphabet, 1 uppercase alphabet and 1 special character" + } + } + }, + "response.ErrStrictUsernamePolicyViolation": { + "type": "object", + "properties": { + "code": { + "type": "integer", + "example": 401 + }, + "message": { + "type": "string", + "example": "The username should be atleast 3 characters long and atmost 16 characters long." } } }, diff --git a/chaoscenter/authentication/api/docs/swagger.json b/chaoscenter/authentication/api/docs/swagger.json index b7f1967169d..162d7c7cd8d 100644 --- a/chaoscenter/authentication/api/docs/swagger.json +++ b/chaoscenter/authentication/api/docs/swagger.json @@ -930,6 +930,12 @@ "$ref": "#/definitions/response.ErrInvalidRequest" } }, + "401": { + "description": "Unauthorized", + "schema": { + "$ref": "#/definitions/response.ErrStrictUsernamePolicyViolation" + } + }, "500": { "description": "Internal Server Error", "schema": { @@ -1204,7 +1210,20 @@ }, "message": { "type": "string", - "example": "Please ensure the password is 8 characters long and has 1 digit, 1 lowercase alphabet, 1 uppercase alphabet and 1 special character" + "example": "Please ensure the password is atleast 8 characters and atmost 16 characters long and has atleast 1 digit, 1 lowercase alphabet, 1 uppercase alphabet and 1 special character" + } + } + }, + "response.ErrStrictUsernamePolicyViolation": { + "type": "object", + "properties": { + "code": { + "type": "integer", + "example": 401 + }, + "message": { + "type": "string", + "example": "The username should be atleast 3 characters long and atmost 16 characters long." } } }, diff --git a/chaoscenter/authentication/api/docs/swagger.yaml b/chaoscenter/authentication/api/docs/swagger.yaml index 57a58954e60..9abda944883 100644 --- a/chaoscenter/authentication/api/docs/swagger.yaml +++ b/chaoscenter/authentication/api/docs/swagger.yaml @@ -82,8 +82,19 @@ definitions: example: 401 type: integer message: - example: Please ensure the password is 8 characters long and has 1 digit, - 1 lowercase alphabet, 1 uppercase alphabet and 1 special character + example: Please ensure the password is atleast 8 characters and atmost 16 + characters long and has atleast 1 digit, 1 lowercase alphabet, 1 uppercase + alphabet and 1 special character + type: string + type: object + response.ErrStrictUsernamePolicyViolation: + properties: + code: + example: 401 + type: integer + message: + example: The username should be atleast 3 characters long and atmost 16 characters + long. type: string type: object response.ErrUnauthorized: @@ -762,6 +773,10 @@ paths: description: Bad Request schema: $ref: '#/definitions/response.ErrInvalidRequest' + "401": + description: Unauthorized + schema: + $ref: '#/definitions/response.ErrStrictUsernamePolicyViolation' "500": description: Internal Server Error schema: diff --git a/chaoscenter/authentication/api/handlers/doc.go b/chaoscenter/authentication/api/handlers/doc.go index 7163177974b..6d6c927525b 100644 --- a/chaoscenter/authentication/api/handlers/doc.go +++ b/chaoscenter/authentication/api/handlers/doc.go @@ -97,7 +97,12 @@ type ErrUserDeactivated struct { type ErrStrictPasswordPolicyViolation struct { Code int `json:"code" example:"401"` - Message string `json:"message" example:"Please ensure the password is 8 characters long and has 1 digit, 1 lowercase alphabet, 1 uppercase alphabet and 1 special character"` + Message string `json:"message" example:"Please ensure the password is atleast 8 characters and atmost 16 characters long and has atleast 1 digit, 1 lowercase alphabet, 1 uppercase alphabet and 1 special character"` +} + +type ErrStrictUsernamePolicyViolation struct { + Code int `json:"code" example:"401"` + Message string `json:"message" example:"The username should be atleast 3 characters long and atmost 16 characters long."` } type ErrEmptyProjectName struct { diff --git a/chaoscenter/authentication/api/handlers/rest/user_handlers.go b/chaoscenter/authentication/api/handlers/rest/user_handlers.go index c5826c9813d..48055413db9 100644 --- a/chaoscenter/authentication/api/handlers/rest/user_handlers.go +++ b/chaoscenter/authentication/api/handlers/rest/user_handlers.go @@ -28,6 +28,8 @@ const BearerSchema = "Bearer " // @Failure 400 {object} response.ErrInvalidRequest // @Failure 401 {object} response.ErrUnauthorized // @Failure 400 {object} response.ErrInvalidEmail +// @Failure 401 {object} response.ErrStrictPasswordPolicyViolation +// @Failure 401 {object} response.ErrStrictUsernamePolicyViolation // @Failure 401 {object} response.ErrUserExists // @Failure 500 {object} response.ErrServerError // @Success 200 {object} response.UserResponse{} @@ -59,6 +61,12 @@ func CreateUser(service services.ApplicationService) gin.HandlerFunc { c.JSON(utils.ErrorStatusCodes[utils.ErrInvalidRequest], presenter.CreateErrorResponse(utils.ErrInvalidRequest)) return } + //username validation + err = utils.ValidateStrictUsername(userRequest.Username) + if err != nil { + c.JSON(utils.ErrorStatusCodes[utils.ErrStrictUsernamePolicyViolation], presenter.CreateErrorResponse(utils.ErrStrictUsernamePolicyViolation)) + return + } // Assigning UID to user uID := uuid.Must(uuid.NewRandom()).String() @@ -108,6 +116,8 @@ func CreateUser(service services.ApplicationService) gin.HandlerFunc { // @Accept json // @Produce json // @Failure 400 {object} response.ErrInvalidRequest +// @Failure 401 {object} response.ErrStrictPasswordPolicyViolation +// @Failure 401 {object} response.ErrStrictUsernamePolicyViolation // @Failure 500 {object} response.ErrServerError // @Success 200 {object} response.MessageResponse{} // @Router /update/details [post] @@ -414,17 +424,17 @@ func UpdatePassword(service services.ApplicationService) gin.HandlerFunc { } username := c.MustGet("username").(string) userPasswordRequest.Username = username - if utils.StrictPasswordPolicy { + if userPasswordRequest.NewPassword != "" { err := utils.ValidateStrictPassword(userPasswordRequest.NewPassword) if err != nil { c.JSON(utils.ErrorStatusCodes[utils.ErrStrictPasswordPolicyViolation], presenter.CreateErrorResponse(utils.ErrStrictPasswordPolicyViolation)) return } - } - if userPasswordRequest.NewPassword == "" { + } else { c.JSON(utils.ErrorStatusCodes[utils.ErrInvalidRequest], presenter.CreateErrorResponse(utils.ErrInvalidRequest)) return } + err = service.UpdatePassword(&userPasswordRequest, true) if err != nil { log.Info(err) @@ -478,7 +488,7 @@ func ResetPassword(service services.ApplicationService) gin.HandlerFunc { c.JSON(utils.ErrorStatusCodes[utils.ErrServerError], presenter.CreateErrorResponse(utils.ErrPasswordNotUpdated)) } - if utils.StrictPasswordPolicy { + if userPasswordRequest.NewPassword != "" { err := utils.ValidateStrictPassword(userPasswordRequest.NewPassword) if err != nil { c.JSON(utils.ErrorStatusCodes[utils.ErrStrictPasswordPolicyViolation], presenter.CreateErrorResponse(utils.ErrStrictPasswordPolicyViolation)) diff --git a/chaoscenter/authentication/api/handlers/rest/user_handlers_test.go b/chaoscenter/authentication/api/handlers/rest/user_handlers_test.go index e0f19334790..bd9063d5db9 100644 --- a/chaoscenter/authentication/api/handlers/rest/user_handlers_test.go +++ b/chaoscenter/authentication/api/handlers/rest/user_handlers_test.go @@ -55,7 +55,7 @@ func TestCreateUser(t *testing.T) { name: "successfully", inputBody: &entities.User{ Username: "newUser", - Password: "validPassword123", + Password: "ValidPassword@1", Email: "newuser@example.com", Name: "John Doe", Role: entities.RoleUser, @@ -68,7 +68,7 @@ func TestCreateUser(t *testing.T) { Email: "newuser@example.com", Name: "John Doe", Role: entities.RoleUser, - }, nil) + }, nil).Once() }, expectedCode: 200, }, @@ -80,8 +80,12 @@ func TestCreateUser(t *testing.T) { c, _ := gin.CreateTestContext(w) c.Set("role", tc.mockRole) if tc.inputBody != nil { - b, _ := json.Marshal(tc.inputBody) + b, err := json.Marshal(tc.inputBody) + if err != nil { + t.Fatalf("could not marshal input body: %v", err) + } c.Request = httptest.NewRequest(http.MethodPost, "/users", bytes.NewBuffer(b)) + c.Request.Header.Set("Content-Type", "application/json") } tc.given() @@ -477,13 +481,22 @@ func TestUpdatePassword(t *testing.T) { }{ { name: "Successfully update password", - givenBody: `{"oldPassword":"oldPass", "newPassword":"newPass"}`, + givenBody: `{"oldPassword":"oldPass@123", "newPassword":"newPass@123"}`, givenUsername: "testUser", givenStrictPassword: false, givenServiceResponse: nil, expectedCode: http.StatusOK, expectedOutput: `{"message":"password has been updated successfully"}`, }, + { + name: "Invalid new password", + givenBody: `{"oldPassword":"oldPass@123", "newPassword":"short"}`, + givenUsername: "testUser", + givenStrictPassword: false, + givenServiceResponse: errors.New("invalid password"), + expectedCode: utils.ErrorStatusCodes[utils.ErrStrictPasswordPolicyViolation], + expectedOutput: `{"error":"password_policy_violation","errorDescription":"Please ensure the password is atleast 8 characters long and atmost 16 characters long and has atleast 1 digit, 1 lowercase alphabet, 1 uppercase alphabet and 1 special character"}`, + }, } for _, tt := range tests { @@ -498,8 +511,8 @@ func TestUpdatePassword(t *testing.T) { userPassword := entities.UserPassword{ Username: tt.givenUsername, - OldPassword: "oldPass", - NewPassword: "newPass", + OldPassword: "oldPass@123", + NewPassword: "newPass@123", } user := &entities.User{ ID: "testUID", @@ -532,11 +545,11 @@ func TestResetPassword(t *testing.T) { expectedCode int }{ { - name: "Non-admin role", + name: "Admin role", inputBody: &entities.UserPassword{ Username: "testUser", OldPassword: "", - NewPassword: "validPassword123", + NewPassword: "ValidPass@123", }, mockRole: "admin", mockUID: "testUID", @@ -559,7 +572,7 @@ func TestResetPassword(t *testing.T) { inputBody: &entities.UserPassword{ Username: "testUser", OldPassword: "", - NewPassword: "validPassword123", + NewPassword: "validPass@123", }, mockRole: "user", mockUID: "testUID", @@ -581,6 +594,29 @@ func TestResetPassword(t *testing.T) { mockUsername: "adminUser", expectedCode: utils.ErrorStatusCodes[utils.ErrInvalidRequest], }, + { + name: "Admin role wrong password", + inputBody: &entities.UserPassword{ + Username: "testUser", + OldPassword: "", + NewPassword: "short", + }, + mockRole: "admin", + mockUID: "testUID", + mockUsername: "adminUser", + given: func() { + user := &entities.User{ + ID: "testUID", + Username: "testUser", + Email: "test@example.com", + IsInitialLogin: false, + } + service.On("GetUser", "testUID").Return(user, nil) + service.On("IsAdministrator", mock.AnythingOfType("*entities.User")).Return(nil) + service.On("UpdatePassword", mock.AnythingOfType("*entities.UserPassword"), false).Return(nil) + }, + expectedCode: utils.ErrorStatusCodes[utils.ErrStrictPasswordPolicyViolation], + }, } for _, tt := range tests { diff --git a/chaoscenter/authentication/pkg/utils/errors.go b/chaoscenter/authentication/pkg/utils/errors.go index 8ffe1b4c72d..4f2eff8d4b3 100644 --- a/chaoscenter/authentication/pkg/utils/errors.go +++ b/chaoscenter/authentication/pkg/utils/errors.go @@ -10,6 +10,7 @@ var ( ErrServerError AppError = errors.New("server_error") ErrInvalidRequest AppError = errors.New("invalid_request") ErrStrictPasswordPolicyViolation AppError = errors.New("password_policy_violation") + ErrStrictUsernamePolicyViolation AppError = errors.New("username_policy_violation") ErrUnauthorized AppError = errors.New("unauthorized") ErrUserExists AppError = errors.New("user_exists") ErrUserNotFound AppError = errors.New("user does not exist") @@ -32,6 +33,7 @@ var ErrorStatusCodes = map[AppError]int{ ErrUnauthorized: 401, ErrUserExists: 401, ErrStrictPasswordPolicyViolation: 401, + ErrStrictUsernamePolicyViolation: 401, ErrUserNotFound: 400, ErrProjectNotFound: 400, ErrUpdatingAdmin: 400, @@ -50,7 +52,8 @@ var ErrorDescriptions = map[AppError]string{ ErrInvalidRequest: "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed", ErrUnauthorized: "The user does not have requested authorization to access this resource", ErrUserExists: "This username is already assigned to another user", - ErrStrictPasswordPolicyViolation: "Please ensure the password is 8 characters long and has 1 digit, 1 lowercase alphabet, 1 uppercase alphabet and 1 special character", + ErrStrictPasswordPolicyViolation: "Please ensure the password is atleast 8 characters long and atmost 16 characters long and has atleast 1 digit, 1 lowercase alphabet, 1 uppercase alphabet and 1 special character", + ErrStrictUsernamePolicyViolation: "The username should be atleast 3 characters long and atmost 16 characters long.", ErrEmptyProjectName: "Project name can't be empty", ErrInvalidRole: "Role is invalid", ErrProjectNotFound: "This project does not exist", diff --git a/chaoscenter/authentication/pkg/utils/sanitizers.go b/chaoscenter/authentication/pkg/utils/sanitizers.go index 2b5e80d7879..c21bfdd9ce3 100644 --- a/chaoscenter/authentication/pkg/utils/sanitizers.go +++ b/chaoscenter/authentication/pkg/utils/sanitizers.go @@ -13,20 +13,25 @@ func SanitizeString(input string) string { /* ValidateStrictPassword represents and checks for the following patterns: -- Input is at least 8 characters long -- Input contains at least one special character +- Input is at least 8 characters long and at most 16 characters long +- Input contains at least one special character of these @$!%*?_& - Input contains at least one digit - Input contains at least one uppercase alphabet - Input contains at least one lowercase alphabet */ func ValidateStrictPassword(input string) error { if len(input) < 8 { - return fmt.Errorf("password is less than 8 characters") + return fmt.Errorf("password length is less than 8 characters") } + + if len(input) > 16 { + return fmt.Errorf("password length is more than 16 characters") + } + digits := `[0-9]{1}` lowerAlphabets := `[a-z]{1}` capitalAlphabets := `[A-Z]{1}` - specialCharacters := `[!@#~$%^&*()+|_]{1}` + specialCharacters := `[@$!%*?_&]{1}` if b, err := regexp.MatchString(digits, input); !b || err != nil { return fmt.Errorf("password does not contain digits") } @@ -41,3 +46,16 @@ func ValidateStrictPassword(input string) error { } return nil } + +// Username must start with a letter - ^[a-zA-Z] +// Allow letters, digits, underscores, and hyphens - [a-zA-Z0-9_-] +// Ensure the length of the username is between 3 and 16 characters (1 character is already matched above) - {2,15}$ + +func ValidateStrictUsername(username string) error { + // Ensure username doesn't contain special characters (only letters, numbers, and underscores are allowed) + if matched, _ := regexp.MatchString(`^[a-zA-Z][a-zA-Z0-9_-]{2,15}$`, username); !matched { + return fmt.Errorf("username can only contain letters, numbers, and underscores") + } + + return nil +}