From b570956baf6e1e561b9d51a17136743568851234 Mon Sep 17 00:00:00 2001 From: Sonja Happ Date: Thu, 12 Sep 2019 10:18:42 +0200 Subject: [PATCH] improve user package - testing more error return codes - remove some errors from code - remove unnecessary code - require a username of at least 3 letters/digits --- routes/user/authenticate_endpoint.go | 11 +- routes/user/user_endpoints.go | 102 +++++++---------- routes/user/user_methods.go | 10 +- routes/user/user_test.go | 162 +++++++++++++++++++++++++-- routes/user/user_validators.go | 9 +- 5 files changed, 204 insertions(+), 90 deletions(-) diff --git a/routes/user/authenticate_endpoint.go b/routes/user/authenticate_endpoint.go index 7aa1182..9ccb69c 100644 --- a/routes/user/authenticate_endpoint.go +++ b/routes/user/authenticate_endpoint.go @@ -26,10 +26,7 @@ func RegisterAuthenticate(r *gin.RouterGroup) { // @Tags authentication // @Param inputUser body user.loginRequest true "loginRequest of user" // @Success 200 {object} docs.ResponseAuthenticate "JSON web token, success status, message and authenticated user object" -// @Failure 400 {object} docs.ResponseError "Bad request" // @Failure 401 {object} docs.ResponseError "Unauthorized" -// @Failure 404 {object} docs.ResponseError "Not found" -// @Failure 422 {object} docs.ResponseError "Unprocessable entity." // @Failure 500 {object} docs.ResponseError "Internal server error." // @Router /authenticate [post] func authenticate(c *gin.Context) { @@ -37,13 +34,13 @@ func authenticate(c *gin.Context) { // Bind the response (context) with the loginRequest struct var credentials loginRequest if err := c.ShouldBindJSON(&credentials); err != nil { - helper.UnprocessableEntityError(c, err.Error()) + helper.UnauthorizedError(c, "Wrong username or password") return } // Validate the login request if errs := credentials.validate(); errs != nil { - helper.BadRequestError(c, errs.Error()) + helper.UnauthorizedError(c, "Wrong username or password") return } @@ -51,14 +48,14 @@ func authenticate(c *gin.Context) { var user User err := user.ByUsername(credentials.Username) if err != nil { - helper.NotFoundError(c, "User not found") + helper.UnauthorizedError(c, "Wrong username or password") return } // Validate the password err = user.validatePassword(credentials.Password) if err != nil { - helper.UnauthorizedError(c, "Invalid password") + helper.UnauthorizedError(c, "Wrong username or password") return } diff --git a/routes/user/user_endpoints.go b/routes/user/user_endpoints.go index 8a77d52..2197d56 100644 --- a/routes/user/user_endpoints.go +++ b/routes/user/user_endpoints.go @@ -4,7 +4,6 @@ import ( "fmt" "git.rwth-aachen.de/acs/public/villas/villasweb-backend-go/helper" "net/http" - "strconv" "strings" "time" @@ -47,11 +46,10 @@ func getUsers(c *gin.Context) { db := database.GetDB() var users []database.User err = db.Order("ID asc").Find(&users).Error - if helper.DBError(c, err) { - return + if !helper.DBError(c, err) { + c.JSON(http.StatusOK, gin.H{"users": users}) } - c.JSON(http.StatusOK, gin.H{"users": users}) } // AddUser godoc @@ -83,7 +81,7 @@ func addUser(c *gin.Context) { // Validate the request if err = req.validate(); err != nil { - helper.UnprocessableEntityError(c, err.Error()) + helper.BadRequestError(c, err.Error()) return } @@ -100,18 +98,16 @@ func addUser(c *gin.Context) { // Hash the password before saving it to the DB err = newUser.setPassword(newUser.Password) if err != nil { - helper.UnprocessableEntityError(c, "Unable to encrypt the password") + helper.InternalServerError(c, "Unable to encrypt the password") return } // Save the user in the DB err = newUser.save() - if err != nil { - helper.DBError(c, err) - return + if !helper.DBError(c, err) { + c.JSON(http.StatusOK, gin.H{"user": newUser.User}) } - c.JSON(http.StatusOK, gin.H{"user": newUser.User}) } // UpdateUser godoc @@ -131,24 +127,16 @@ func addUser(c *gin.Context) { // @Router /users/{userID} [put] func updateUser(c *gin.Context) { - err := database.ValidateRole(c, database.ModelUser, database.Update) - if err != nil { - helper.UnprocessableEntityError(c, err.Error()) - return - } + // no need to validate the role since updating a single user is role independent + //err := database.ValidateRole(c, database.ModelUser, database.Update) + //if err != nil { + // helper.UnprocessableEntityError(c, err.Error()) + // return + //} // Get the user's (to be updated) ID from the context - var oldUser User - toBeUpdatedID, err := strconv.Atoi(c.Param("userID")) + toBeUpdatedID, err := helper.GetIDOfElement(c, "userID", "path", -1) if err != nil { - helper.NotFoundError(c, fmt.Sprintf("Could not get user's ID from context")) - return - } - - // Find the user - err = oldUser.ByID(uint(toBeUpdatedID)) - if err != nil { - helper.DBError(c, err) return } @@ -162,18 +150,10 @@ func updateUser(c *gin.Context) { // except Role // Get caller's ID from context - callerID, exists := c.Get(database.UserIDCtx) - if !exists { - helper.NotFoundError(c, fmt.Sprintf("Could not get caller's ID from context")) - return - } + callerID, _ := c.Get(database.UserIDCtx) // Get caller's Role from context - callerRole, exists := c.Get(database.UserRoleCtx) - if !exists { - helper.NotFoundError(c, fmt.Sprintf("Could not get caller's Role from context")) - return - } + callerRole, _ := c.Get(database.UserRoleCtx) if uint(toBeUpdatedID) != callerID && callerRole != "Admin" { helper.ForbiddenError(c, "Invalid authorization") @@ -183,7 +163,7 @@ func updateUser(c *gin.Context) { // Bind the (context) with the updateUserRequest struct var req updateUserRequest if err := c.ShouldBindJSON(&req); err != nil { - helper.UnprocessableEntityError(c, fmt.Sprintf("%v", err)) + helper.BadRequestError(c, fmt.Sprintf("%v", err)) return } @@ -193,27 +173,33 @@ func updateUser(c *gin.Context) { return } + // Find the user + var oldUser User + err = oldUser.ByID(uint(toBeUpdatedID)) + if helper.DBError(c, err) { + return + } + // Create the updatedUser from oldUser considering callerRole (in // case that the request updates the role of the old user) updatedUser, err := req.updatedUser(callerRole, oldUser) if err != nil { if strings.Contains(err.Error(), "Admin") { helper.ForbiddenError(c, err.Error()) - - } else if strings.Contains(err.Error(), "Username") || strings.Contains(err.Error(), "password") { + } else if strings.Contains(err.Error(), "Username") { helper.BadRequestError(c, err.Error()) + } else { // password encryption failed + helper.InternalServerError(c, err.Error()) } return } // Finally update the user err = oldUser.update(updatedUser) - if err != nil { - helper.DBError(c, err) - return + if !helper.DBError(c, err) { + c.JSON(http.StatusOK, gin.H{"user": updatedUser.User}) } - c.JSON(http.StatusOK, gin.H{"user": updatedUser.User}) } // GetUser godoc @@ -230,15 +216,15 @@ func updateUser(c *gin.Context) { // @Router /users/{userID} [get] func getUser(c *gin.Context) { - err := database.ValidateRole(c, database.ModelUser, database.Read) - if err != nil { - helper.UnprocessableEntityError(c, err.Error()) - return - } + // role validation not needed because updating a single user is role-independent + //err := database.ValidateRole(c, database.ModelUser, database.Read) + //if err != nil { + // helper.UnprocessableEntityError(c, err.Error()) + // return + //} - id, err := strconv.Atoi(c.Param("userID")) + id, err := helper.GetIDOfElement(c, "userID", "path", -1) if err != nil { - helper.NotFoundError(c, fmt.Sprintf("Could not get user's ID from context")) return } @@ -252,12 +238,10 @@ func getUser(c *gin.Context) { var user User err = user.ByID(uint(id)) - if err != nil { - helper.DBError(c, err) - return + if !helper.DBError(c, err) { + c.JSON(http.StatusOK, gin.H{"user": user.User}) } - c.JSON(http.StatusOK, gin.H{"user": user.User}) } // DeleteUser godoc @@ -280,25 +264,21 @@ func deleteUser(c *gin.Context) { } var user User - id, err := strconv.Atoi(c.Param("userID")) + id, err := helper.GetIDOfElement(c, "userID", "path", -1) if err != nil { - helper.NotFoundError(c, fmt.Sprintf("Could not get user's ID from context")) return } // Check that the user exist err = user.ByID(uint(id)) - if err != nil { - helper.DBError(c, err) + if helper.DBError(c, err) { return } // Try to remove user err = user.remove() - if err != nil { - helper.DBError(c, err) - return + if !helper.DBError(c, err) { + c.JSON(http.StatusOK, gin.H{"user": user.User}) } - c.JSON(http.StatusOK, gin.H{"user": user.User}) } diff --git a/routes/user/user_methods.go b/routes/user/user_methods.go index 77d1f82..f00f864 100644 --- a/routes/user/user_methods.go +++ b/routes/user/user_methods.go @@ -30,19 +30,13 @@ func (u *User) remove() error { func (u *User) ByUsername(username string) error { db := database.GetDB() err := db.Find(u, "Username = ?", username).Error - if err != nil { - return err - } - return nil + return err } func (u *User) ByID(id uint) error { db := database.GetDB() err := db.Find(u, id).Error - if err != nil { - return fmt.Errorf("User with id=%v does not exist", id) - } - return nil + return err } func (u *User) setPassword(password string) error { diff --git a/routes/user/user_test.go b/routes/user/user_test.go index 1518ff2..f89aac6 100644 --- a/routes/user/user_test.go +++ b/routes/user/user_test.go @@ -48,30 +48,30 @@ func TestAuthenticate(t *testing.T) { assert.NoError(t, database.DBAddAdminAndUserAndGuest(db)) // try to authenticate with non JSON body - // should result in unprocessable entity + // should result in unauthorized w1 := httptest.NewRecorder() body, _ := json.Marshal("This is no JSON") req, err := http.NewRequest("POST", "/api/authenticate", bytes.NewBuffer(body)) assert.NoError(t, err) req.Header.Set("Content-Type", "application/json") router.ServeHTTP(w1, req) - assert.Equalf(t, 422, w1.Code, "Response body: \n%v\n", w1.Body) + assert.Equalf(t, 401, w1.Code, "Response body: \n%v\n", w1.Body) malformedCredentials := helper.Credentials{ Username: "TEST1", } - // try to authenticate with non JSON body - // should result in bad request + // try to authenticate with malformed credentials + // should result in unauthorized w2 := httptest.NewRecorder() body, _ = json.Marshal(malformedCredentials) req, err = http.NewRequest("POST", "/api/authenticate", bytes.NewBuffer(body)) assert.NoError(t, err) req.Header.Set("Content-Type", "application/json") router.ServeHTTP(w2, req) - assert.Equal(t, 400, w2.Code, w2.Body) + assert.Equal(t, 401, w2.Code, w2.Body) // try to authenticate with a username that does not exist in the DB - // should result in not found + // should result in unauthorized malformedCredentials.Username = "NOTEXIST" malformedCredentials.Password = "blablabla" w3 := httptest.NewRecorder() @@ -80,7 +80,7 @@ func TestAuthenticate(t *testing.T) { assert.NoError(t, err) req.Header.Set("Content-Type", "application/json") router.ServeHTTP(w3, req) - assert.Equal(t, 404, w3.Code, w3.Body) + assert.Equal(t, 401, w3.Code, w3.Body) // try to authenticate with a correct user name and a wrong password // should result in unauthorized @@ -94,6 +94,11 @@ func TestAuthenticate(t *testing.T) { router.ServeHTTP(w4, req) assert.Equal(t, 401, w4.Code, w4.Body) + // authenticate as admin + _, err = helper.AuthenticateForTest(router, + "/api/authenticate", "POST", helper.AdminCredentials) + assert.NoError(t, err) + } func TestAddGetUser(t *testing.T) { @@ -107,14 +112,83 @@ func TestAddGetUser(t *testing.T) { "/api/authenticate", "POST", helper.AdminCredentials) assert.NoError(t, err) - // test POST user/ $newUser + // try to POST with non JSON body + // should result in bad request + code, resp, err := helper.TestEndpoint(router, token, + "/api/users", "POST", "This is not JSON") + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + + wrongUser := UserRequest{ + Username: "ab", + Password: "123456", + Mail: "bla@test.com", + Role: "Guest", + } + // try POST with too short username + // should result in bad request + code, resp, err = helper.TestEndpoint(router, token, + "/api/users", "POST", helper.KeyModels{"user": wrongUser}) + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + + // try POST with too short password + // should result in bad request + wrongUser.Username = "Longenoughusername" + wrongUser.Password = "short" + code, resp, err = helper.TestEndpoint(router, token, + "/api/users", "POST", helper.KeyModels{"user": wrongUser}) + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + + // try POST with wrong role + // should result in bad request + wrongUser.Password = "longenough" + wrongUser.Role = "ThisIsNotARole" + code, resp, err = helper.TestEndpoint(router, token, + "/api/users", "POST", helper.KeyModels{"user": wrongUser}) + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + + //try POST with wrong email + // should result in bad request + wrongUser.Mail = "noemailaddress" + wrongUser.Role = "Guest" + code, resp, err = helper.TestEndpoint(router, token, + "/api/users", "POST", helper.KeyModels{"user": wrongUser}) + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + + // try POST with missing required fields + // should result in bad request + wrongUser.Mail = "" + wrongUser.Role = "" + wrongUser.Password = "" + code, resp, err = helper.TestEndpoint(router, token, + "/api/users", "POST", helper.KeyModels{"user": wrongUser}) + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + + // try POST with username that is already taken + // should result in unprocessable entity + wrongUser.Mail = "test@test.com" + wrongUser.Role = "Guest" + wrongUser.Password = "blablabla" + wrongUser.Username = "User_A" + code, resp, err = helper.TestEndpoint(router, token, + "/api/users", "POST", helper.KeyModels{"user": wrongUser}) + assert.NoError(t, err) + assert.Equalf(t, 422, code, "Response body: \n%v\n", resp) + newUser := UserRequest{ Username: "Alice483", Password: "th1s_I5_@lice#", Mail: "mail@domain.com", Role: "User", } - code, resp, err := helper.TestEndpoint(router, token, + + // test POST user/ $newUser + code, resp, err = helper.TestEndpoint(router, token, "/api/users", "POST", helper.KeyModels{"user": newUser}) assert.NoError(t, err) assert.Equalf(t, 200, code, "Response body: \n%v\n", resp) @@ -142,6 +216,13 @@ func TestAddGetUser(t *testing.T) { // Compare GET's response with the newUser (Password omitted) err = helper.CompareResponse(resp, helper.KeyModels{"user": newUser}) assert.NoError(t, err) + + // try to GET user with invalid user ID + // should result in bad request + code, resp, err = helper.TestEndpoint(router, token, + fmt.Sprintf("/api/users/bla"), "GET", nil) + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) } func TestUsersNotAllowedActions(t *testing.T) { @@ -238,6 +319,19 @@ func TestGetAllUsers(t *testing.T) { assert.NoError(t, err) assert.Equal(t, finalNumber, initialNumber+1) + + // authenticate as normal user + token, err = helper.AuthenticateForTest(router, + "/api/authenticate", "POST", helper.UserACredentials) + assert.NoError(t, err) + + // try to get all users as normal user + // should result in unprocessable entity eror + code, resp, err = helper.TestEndpoint(router, token, + "/api/users", "GET", nil) + assert.NoError(t, err) + assert.Equalf(t, 422, code, "Response body: \n%v\n", resp) + } func TestModifyAddedUserAsUser(t *testing.T) { @@ -274,6 +368,20 @@ func TestModifyAddedUserAsUser(t *testing.T) { }) assert.NoError(t, err) + // Try PUT with invalid user ID in path + // Should return a bad request + code, resp, err = helper.TestEndpoint(router, token, fmt.Sprintf("/api/users/blabla"), "PUT", + helper.KeyModels{"user": newUser}) + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + + // Try to PUT a non JSON body + // Should return a bad request + code, resp, err = helper.TestEndpoint(router, token, + fmt.Sprintf("/api/users/%v", newUserID), "PUT", "This is no JSON") + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + // Turn password member of newUser to empty string so it is omitted // in marshaling. The password will never be included in the // response and if is non empty in request we will not be able to do @@ -377,8 +485,27 @@ func TestInvalidUserUpdate(t *testing.T) { newUserID, err := helper.GetResponseID(resp) assert.NoError(t, err) + // try PUT with userID that does not exist + // should result in not found + modRequest := UserRequest{Password: "longenough"} + code, resp, err = helper.TestEndpoint(router, token, + fmt.Sprintf("/api/users/%v", newUserID+1), "PUT", + helper.KeyModels{"user": modRequest}) + assert.NoError(t, err) + assert.Equalf(t, 404, code, "Response body: \n%v\n", resp) + + // try to PUT with username that is already taken + // should result in bad request + modRequest.Password = "" + modRequest.Username = "User_A" + code, resp, err = helper.TestEndpoint(router, token, + fmt.Sprintf("/api/users/%v", newUserID), "PUT", + helper.KeyModels{"user": modRequest}) + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + // modify newUser's password with INVALID password - modRequest := UserRequest{Password: "short"} + modRequest.Password = "short" code, resp, err = helper.TestEndpoint(router, token, fmt.Sprintf("/api/users/%v", newUserID), "PUT", helper.KeyModels{"user": modRequest}) @@ -400,6 +527,7 @@ func TestInvalidUserUpdate(t *testing.T) { helper.KeyModels{"user": modRequest}) assert.NoError(t, err) assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + } func TestModifyAddedUserAsAdmin(t *testing.T) { @@ -510,6 +638,20 @@ func TestDeleteUser(t *testing.T) { newUserID, err := helper.GetResponseID(resp) assert.NoError(t, err) + // try to DELETE with invalid ID + // should result in bad request + code, resp, err = helper.TestEndpoint(router, token, + fmt.Sprintf("/api/users/bla"), "DELETE", nil) + assert.NoError(t, err) + assert.Equalf(t, 400, code, "Response body: \n%v\n", resp) + + // try to DELETE with ID that does not exist + // should result in not found + code, resp, err = helper.TestEndpoint(router, token, + fmt.Sprintf("/api/users/%v", newUserID+1), "DELETE", nil) + assert.NoError(t, err) + assert.Equalf(t, 404, code, "Response body: \n%v\n", resp) + // Count the number of all the users returned initialNumber, err := helper.LengthOfResponse(router, token, "/api/users", "GET", nil) diff --git a/routes/user/user_validators.go b/routes/user/user_validators.go index 45c12eb..f3e750f 100644 --- a/routes/user/user_validators.go +++ b/routes/user/user_validators.go @@ -10,11 +10,11 @@ var validate *validator.Validate type loginRequest struct { Username string `form:"Username" validate:"required"` - Password string `form:"Password" validate:"required,min=6"` + Password string `form:"Password" validate:"required"` } type validUpdatedRequest struct { - Username string `form:"Username" validate:"omitempty"` + Username string `form:"Username" validate:"omitempty,min=3"` Password string `form:"Password" validate:"omitempty,min=6"` Role string `form:"Role" validate:"omitempty,oneof=Admin User Guest"` Mail string `form:"Mail" validate:"omitempty,email"` @@ -25,7 +25,7 @@ type updateUserRequest struct { } type validNewUser struct { - Username string `form:"Username" validate:"required"` + Username string `form:"Username" validate:"required,min=3"` Password string `form:"Password" validate:"required,min=6"` Role string `form:"Role" validate:"required,oneof=Admin User Guest"` Mail string `form:"Mail" validate:"required,email"` @@ -63,7 +63,8 @@ func (r *updateUserRequest) updatedUser(role interface{}, } // Update the username making sure is NOT taken - if err := u.ByUsername(r.Username); err == nil { + var testUser User + if err := testUser.ByUsername(r.Username); err == nil { return u, fmt.Errorf("Username is alreaday taken") }