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

@@ -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
 	}
 

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})
 }

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 {

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)

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")
 	}