From 6d608b02b5928a2eb95af1eb087c0254061800db Mon Sep 17 00:00:00 2001 From: Garrett Ladley <92384606+garrettladley@users.noreply.github.com> Date: Mon, 15 Apr 2024 17:22:58 -0400 Subject: [PATCH] feat: more informative error messages for password requirements (#549) --- backend/src/auth/password.go | 39 +++++++++++++++++++++++++ backend/src/constants/auth.go | 2 ++ backend/src/errors/auth.go | 12 ++++++++ backend/src/models/user.go | 8 ++--- backend/src/services/auth.go | 4 +++ backend/src/services/user.go | 12 ++++++++ backend/src/utilities/validator.go | 15 ---------- backend/tests/api/user_test.go | 47 +++++++++++++++++++++++------- 8 files changed, 109 insertions(+), 30 deletions(-) create mode 100644 backend/src/auth/password.go diff --git a/backend/src/auth/password.go b/backend/src/auth/password.go new file mode 100644 index 000000000..f1e6a9412 --- /dev/null +++ b/backend/src/auth/password.go @@ -0,0 +1,39 @@ +package auth + +import ( + "regexp" + "strings" + + "github.com/GenerateNU/sac/backend/src/constants" + "github.com/GenerateNU/sac/backend/src/errors" +) + +func ValidatePassword(password string) *errors.Error { + if len(password) < 8 { + return &errors.InvalidPasswordNotLongEnough + } + + if !hasDigit(password) { + return &errors.InvalidPasswordNoDigit + } + + if !hasSpecialChar(password) { + return &errors.InvalidPasswordNoSpecialCharacter + } + + return nil +} + +func hasDigit(str string) bool { + return regexp.MustCompile(`[0-9]`).MatchString(str) +} + +func hasSpecialChar(str string) bool { + for _, c := range constants.SPECIAL_CHARACTERS { + if strings.Contains(str, string(c)) { + return true + } + } + + return false +} diff --git a/backend/src/constants/auth.go b/backend/src/constants/auth.go index 236691482..1e0ca2966 100644 --- a/backend/src/constants/auth.go +++ b/backend/src/constants/auth.go @@ -6,3 +6,5 @@ const ( ACCESS_TOKEN_EXPIRY time.Duration = time.Hour * 24 * 30 // temporary TODO: change to 60 minutes REFRESH_TOKEN_EXPIRY time.Duration = time.Hour * 24 * 30 ) + +var SPECIAL_CHARACTERS = []rune{' ', '!', '"', '#', '$', '%', '&', '\'', '(', ')', '*', '+', ',', '-', '.', '/', ':', ';', '<', '=', '>', '?', '@', '[', '\\', ']', '^', '_', '`', '{', '|', '}', '~'} // see https://owasp.org/www-community/password-special-characters diff --git a/backend/src/errors/auth.go b/backend/src/errors/auth.go index 57a6cc2d7..83b3c46c4 100644 --- a/backend/src/errors/auth.go +++ b/backend/src/errors/auth.go @@ -99,4 +99,16 @@ var ( StatusCode: fiber.StatusNotFound, Message: "otp not found", } + InvalidPasswordNotLongEnough = Error{ + StatusCode: fiber.StatusBadRequest, + Message: "password must be at least 8 characters long", + } + InvalidPasswordNoDigit = Error{ + StatusCode: fiber.StatusBadRequest, + Message: "password must contain at least one digit", + } + InvalidPasswordNoSpecialCharacter = Error{ + StatusCode: fiber.StatusBadRequest, + Message: "password must contain at least one special character", + } ) diff --git a/backend/src/models/user.go b/backend/src/models/user.go index aedfd98b7..e0c4015fc 100644 --- a/backend/src/models/user.go +++ b/backend/src/models/user.go @@ -175,7 +175,7 @@ type CreateUserRequestBody struct { FirstName string `json:"first_name" validate:"required,max=255"` LastName string `json:"last_name" validate:"required,max=255"` Email string `json:"email" validate:"required,email,neu_email,max=255"` - Password string `json:"password" validate:"required,password,min=8,max=255"` + Password string `json:"password" validate:"required,max=255"` // MARK: must be validated manually // Optional fields Major0 Major `json:"major0" validate:"not_equal_if_not_empty=Major1,not_equal_if_not_empty=Major2,omitempty,max=255,oneof=africanaStudies americanSignLanguage americanSignLanguage-EnglishInterpreting appliedPhysics architecturalStudies architecture art:ArtVisualStudies behavioralNeuroscience biochemistry bioengineering biology biomedicalPhysics businessAdministration businessAdministration:Accounting businessAdministration:AccountingAndAdvisoryServices businessAdministration:BrandManagement businessAdministration:BusinessAnalytics businessAdministration:CorporateInnovation businessAdministration:EntrepreneurialStartups businessAdministration:FamilyBusiness businessAdministration:Finance businessAdministration:Fintech businessAdministration:HealthcareManagementAndConsulting businessAdministration:Management businessAdministration:ManagementInformationSystems businessAdministration:Marketing businessAdministration:MarketingAnalytics businessAdministration:SocialInnovationAndEntrepreneurship businessAdministration:SupplyChainManagement cellAndMolecularBiology chemicalEngineering chemistry civilEngineering communicationStudies computerEngineering computerScience computingAndLaw criminologyAndCriminalJustice culturalAnthropology cybersecurity dataScience design economics electricalEngineering english environmentalAndSustainabilityStudies environmentalEngineering environmentalScience environmentalStudies gameArtAndAnimation gameDesign globalAsianStudies healthScience history historyCultureAndLaw humanServices industrialEngineering internationalAffairs internationalBusiness internationalBusiness:Accounting internationalBusiness:AccountingAndAdvisoryServices internationalBusiness:BrandManagement internationalBusiness:BusinessAnalytics internationalBusiness:CorporateInnovation internationalBusiness:EntrepreneurialStartups internationalBusiness:FamilyBusiness internationalBusiness:Finance internationalBusiness:Fintech internationalBusiness:HealthcareManagementAndConsulting internationalBusiness:Management internationalBusiness:ManagementInformationSystems internationalBusiness:Marketing internationalBusiness:MarketingAnalytics internationalBusiness:SocialInnovationAndEntrepreneurship internationalBusiness:SupplyChainManagement journalism landscapeArchitecture linguistics marineBiology mathematics mechanicalEngineering mediaAndScreenStudies mediaArts music musicTechnology nursing pharmaceuticalSciences pharmacy(PharmD) philosophy physics politicalScience politicsPhilosophyEconomics psychology publicHealth publicRelations religiousStudies sociology spanish speechLanguagePathologyAndAudiology theatre"` Major1 Major `json:"major1" validate:"not_equal_if_not_empty=Major0,not_equal_if_not_empty=Major2,omitempty,max=255,oneof=africanaStudies americanSignLanguage americanSignLanguage-EnglishInterpreting appliedPhysics architecturalStudies architecture art:ArtVisualStudies behavioralNeuroscience biochemistry bioengineering biology biomedicalPhysics businessAdministration businessAdministration:Accounting businessAdministration:AccountingAndAdvisoryServices businessAdministration:BrandManagement businessAdministration:BusinessAnalytics businessAdministration:CorporateInnovation businessAdministration:EntrepreneurialStartups businessAdministration:FamilyBusiness businessAdministration:Finance businessAdministration:Fintech businessAdministration:HealthcareManagementAndConsulting businessAdministration:Management businessAdministration:ManagementInformationSystems businessAdministration:Marketing businessAdministration:MarketingAnalytics businessAdministration:SocialInnovationAndEntrepreneurship businessAdministration:SupplyChainManagement cellAndMolecularBiology chemicalEngineering chemistry civilEngineering communicationStudies computerEngineering computerScience computingAndLaw criminologyAndCriminalJustice culturalAnthropology cybersecurity dataScience design economics electricalEngineering english environmentalAndSustainabilityStudies environmentalEngineering environmentalScience environmentalStudies gameArtAndAnimation gameDesign globalAsianStudies healthScience history historyCultureAndLaw humanServices industrialEngineering internationalAffairs internationalBusiness internationalBusiness:Accounting internationalBusiness:AccountingAndAdvisoryServices internationalBusiness:BrandManagement internationalBusiness:BusinessAnalytics internationalBusiness:CorporateInnovation internationalBusiness:EntrepreneurialStartups internationalBusiness:FamilyBusiness internationalBusiness:Finance internationalBusiness:Fintech internationalBusiness:HealthcareManagementAndConsulting internationalBusiness:Management internationalBusiness:ManagementInformationSystems internationalBusiness:Marketing internationalBusiness:MarketingAnalytics internationalBusiness:SocialInnovationAndEntrepreneurship internationalBusiness:SupplyChainManagement journalism landscapeArchitecture linguistics marineBiology mathematics mechanicalEngineering mediaAndScreenStudies mediaArts music musicTechnology nursing pharmaceuticalSciences pharmacy(PharmD) philosophy physics politicalScience politicsPhilosophyEconomics psychology publicHealth publicRelations religiousStudies sociology spanish speechLanguagePathologyAndAudiology theatre"` @@ -198,12 +198,12 @@ type UpdateUserRequestBody struct { type LoginUserResponseBody struct { Email string `json:"email" validate:"required,email"` - Password string `json:"password" validate:"required,password,min=8,max=255"` + Password string `json:"password" validate:"required,max=255"` // MARK: must be validated manually } type UpdatePasswordRequestBody struct { - OldPassword string `json:"old_password" validate:"required,password,min=8,max=255"` - NewPassword string `json:"new_password" validate:"required,password,not_equal_if_not_empty=OldPassword,min=8,max=255"` + OldPassword string `json:"old_password" validate:"required,max=255"` // MARK: must be validated manually + NewPassword string `json:"new_password" validate:"required,not_equal_if_not_empty=OldPassword,max=255"` // MARK: must be validated manually } type RefreshTokenRequestBody struct { diff --git a/backend/src/services/auth.go b/backend/src/services/auth.go index aa6d627a0..30079cd8f 100644 --- a/backend/src/services/auth.go +++ b/backend/src/services/auth.go @@ -40,6 +40,10 @@ func (a *AuthService) Login(loginBody models.LoginUserResponseBody) (*models.Use return nil, nil, &errors.FailedToValidateUser } + if pwordValErr := auth.ValidatePassword(loginBody.Password); pwordValErr != nil { + return nil, nil, pwordValErr + } + user, getUserByEmailErr := transactions.GetUserByEmail(a.DB, loginBody.Email) if getUserByEmailErr != nil { return nil, nil, getUserByEmailErr diff --git a/backend/src/services/user.go b/backend/src/services/user.go index 71dbf22d9..e10b6fee4 100644 --- a/backend/src/services/user.go +++ b/backend/src/services/user.go @@ -35,6 +35,10 @@ func (u *UserService) CreateUser(userBody models.CreateUserRequestBody) (*models return nil, nil, &errors.FailedToValidateUser } + if pwordValErr := auth.ValidatePassword(userBody.Password); pwordValErr != nil { + return nil, nil, pwordValErr + } + user, err := utilities.MapRequestToModel(userBody, &models.User{}) if err != nil { return nil, nil, &errors.FailedToMapRequestToModel @@ -141,6 +145,14 @@ func (u *UserService) UpdatePassword(id string, passwordBody models.UpdatePasswo return &errors.FailedToValidateUpdatePasswordBody } + if pwordValErr := auth.ValidatePassword(passwordBody.OldPassword); pwordValErr != nil { + return pwordValErr + } + + if pwordValErr := auth.ValidatePassword(passwordBody.NewPassword); pwordValErr != nil { + return pwordValErr + } + passwordHash, err := transactions.GetUserPasswordHash(u.DB, *idAsUUID) if err != nil { return err diff --git a/backend/src/utilities/validator.go b/backend/src/utilities/validator.go index 375335dec..092165f60 100644 --- a/backend/src/utilities/validator.go +++ b/backend/src/utilities/validator.go @@ -2,7 +2,6 @@ package utilities import ( "reflect" - "regexp" "strconv" "strings" @@ -22,10 +21,6 @@ func RegisterCustomValidators() (*validator.Validate, error) { return nil, err } - if err := validate.RegisterValidation("password", validatePassword); err != nil { - return nil, err - } - if err := validate.RegisterValidation("s3_url", validateS3URL); err != nil { return nil, err } @@ -60,16 +55,6 @@ func validateEmail(fl validator.FieldLevel) bool { return true } -func validatePassword(fl validator.FieldLevel) bool { - password := fl.Field().String() - - hasMinLength := len(password) >= 8 - hasDigit, _ := regexp.MatchString(`[0-9]`, password) - hasSpecialChar, _ := regexp.MatchString(`[@#%&*+]`, password) - - return hasMinLength && hasDigit && hasSpecialChar -} - func validateS3URL(fl validator.FieldLevel) bool { return strings.HasPrefix(fl.Field().String(), "https://s3.amazonaws.com/") } diff --git a/backend/tests/api/user_test.go b/backend/tests/api/user_test.go index f6af28f17..2db88c551 100644 --- a/backend/tests/api/user_test.go +++ b/backend/tests/api/user_test.go @@ -481,7 +481,7 @@ func TestCreateUserFailsIfUserWithEmailAlreadyExists(t *testing.T) { } func AssertCreateBadDataFails(t *testing.T, jsonKey string, badValues []interface{}) { - appAssert, _, _ := CreateSampleStudent(t, nil) + appAssert := h.InitTest(t) sampleStudent, rawPassword := h.SampleStudentFactory() @@ -498,7 +498,7 @@ func AssertCreateBadDataFails(t *testing.T, jsonKey string, badValues []interfac }, h.ErrorWithTester{ Error: errors.FailedToValidateUser, - Tester: TestNumUsersRemainsAt2, + Tester: TestNumUsersRemainsAt1, }, ) } @@ -520,15 +520,40 @@ func TestCreateUserFailsOnInvalidEmail(t *testing.T) { } func TestCreateUserFailsOnInvalidPassword(t *testing.T) { - AssertCreateBadDataFails(t, - "password", - []interface{}{ - "", - "foo", - "abcdefg", - "abcdefg0", - "abcdefg@", - }) + appAssert := h.InitTest(t) + + sampleStudent, rawPassword := h.SampleStudentFactory() + + inputsWithDesiredErr := []struct { + Password string + Error errors.Error + }{ + {"0", errors.InvalidPasswordNotLongEnough}, + {"foo", errors.InvalidPasswordNotLongEnough}, + {"abcdefgh", errors.InvalidPasswordNoDigit}, + {"abcdefg0", errors.InvalidPasswordNoSpecialCharacter}, + {"abcdefg@", errors.InvalidPasswordNoDigit}, + } + + for _, inputWithDesiredErr := range inputsWithDesiredErr { + sampleUserPermutation := *h.SampleStudentJSONFactory(sampleStudent, rawPassword) + sampleUserPermutation["password"] = inputWithDesiredErr.Password + + appAssert = appAssert.TestOnErrorAndTester( + h.TestRequest{ + Method: fiber.MethodPost, + Path: "/api/v1/users/", + Body: &sampleUserPermutation, + Role: &models.Super, + }, + h.ErrorWithTester{ + Error: inputWithDesiredErr.Error, + Tester: TestNumUsersRemainsAt1, + }, + ) + } + + appAssert.Close() } func TestCreateUserFailsOnMissingFields(t *testing.T) {