From 7529f4803fe4ef8caad8dc6e225dab9b25b7039d Mon Sep 17 00:00:00 2001 From: smavros Date: Thu, 15 Aug 2019 00:36:03 +0200 Subject: [PATCH] Improves PUT /user/$ID {$updatedUser} endpoint: - Improves the semantics and scope of the updatedUser() by moving operations in updateUserRequest's updatedUser() method. - Fixes the marshaling of the updateUserRequest by renaming and embedding the already existing type. - Fixes bug of password hashing by using `omitempty` tags in the update struct. --- routes/user/userValidators.go | 44 +++++++++++++++++++++++++------- routes/user/user_endpoints.go | 47 +++++++++++------------------------ 2 files changed, 50 insertions(+), 41 deletions(-) diff --git a/routes/user/userValidators.go b/routes/user/userValidators.go index b851416..02b26c3 100644 --- a/routes/user/userValidators.go +++ b/routes/user/userValidators.go @@ -1,6 +1,8 @@ package user import ( + "fmt" + "gopkg.in/go-playground/validator.v9" ) @@ -11,13 +13,17 @@ type loginRequest struct { Password string `form:"Password" validate:"required,min=6"` } -type updateUserRequest struct { +type validUpdatedRequest struct { Username string `form:"Username" validate:"omitempty"` - Password string `form:"Password" validate:"min=6"` + 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"` } +type updateUserRequest struct { + validUpdatedRequest `json:"user"` +} + type validNewUser struct { Username string `form:"Username" validate:"required"` Password string `form:"Password" validate:"required,min=6"` @@ -41,17 +47,37 @@ func (r *updateUserRequest) validate() error { return errs } -func (r *updateUserRequest) createUser(role interface{}) User { - var u User +func (r *updateUserRequest) updatedUser(role interface{}, + oldUser User) (User, error) { + + // Use the old User as a basis for the updated User `u` + u := oldUser - u.Username = r.Username - u.Password = r.Password - u.Mail = r.Mail // Only the Admin must be able to update user's role - if role == "Admin" { + if role != "Admin" && r.Role != u.Role { + return u, fmt.Errorf("Only Admin can update user's Role") + } else { u.Role = r.Role } - return u + + // Update the username making sure is NOT taken + if err := u.ByUsername(r.Username); err == nil { + return u, fmt.Errorf("Username is alreaday taken") + } + u.Username = r.Username + + // If there is a new password then hash it and update it + if r.Password != "" { + err := u.setPassword(r.Password) + if err != nil { + return u, fmt.Errorf("Unable to encrypt new password") + } + } + + // Update male + u.Mail = r.Mail + + return u, nil } func (r *addUserRequest) validate() error { diff --git a/routes/user/user_endpoints.go b/routes/user/user_endpoints.go index a1641ce..e427ca8 100644 --- a/routes/user/user_endpoints.go +++ b/routes/user/user_endpoints.go @@ -253,9 +253,9 @@ func updateUser(c *gin.Context) { } // Find the user - var user User + var oldUser User toBeUpdatedID, _ := common.UintParamFromCtx(c, "userID") - err = user.ByID(toBeUpdatedID) + err = oldUser.ByID(toBeUpdatedID) if err != nil { c.JSON(http.StatusNotFound, fmt.Sprintf("%v", err)) return @@ -265,13 +265,14 @@ func updateUser(c *gin.Context) { // 1: If the logged in user has NOT the same id as the user that is // going to be updated AND the role is NOT admin (is already saved // in the context from the Authentication middleware) the operation - // is elegal + // is illegal // 2: If the udpate is done by the Admin every field can be updated - // 3: If the update is done by a User everything except Role - userID, _ := c.Get(common.UserIDCtx) - userRole, _ := c.Get(common.UserRoleCtx) + // 3: If the update is done by a User everything can be updated + // except Role + callerID, _ := c.Get(common.UserIDCtx) + callerRole, _ := c.Get(common.UserRoleCtx) - if toBeUpdatedID != userID && userRole != "Admin" { + if toBeUpdatedID != callerID && callerRole != "Admin" { c.JSON(http.StatusForbidden, gin.H{ "success": false, "message": "Invalid authorization", @@ -279,7 +280,7 @@ func updateUser(c *gin.Context) { return } - // Bind the (context) with the User struct + // Bind the (context) with the updateUserRequest struct var req updateUserRequest if err := c.ShouldBindJSON(&req); err != nil { c.JSON(http.StatusUnprocessableEntity, gin.H{ @@ -289,7 +290,7 @@ func updateUser(c *gin.Context) { return } - // Validate the request (taking into acount the role) + // Validate the request based on struct updateUserRequest json tags if err = req.validate(); err != nil { c.JSON(http.StatusBadRequest, gin.H{ "success": false, @@ -298,37 +299,19 @@ func updateUser(c *gin.Context) { return } - updatedUser := req.createUser(userRole) - - // Check that the username is NOT taken - err = updatedUser.ByUsername(updatedUser.Username) - if err == nil { - c.JSON(http.StatusUnprocessableEntity, gin.H{ - "message": "Username is already taken", - }) - return - } - - // Hash the password before updating it to the DB - err = updatedUser.setPassword(updatedUser.Password) + // 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 { - c.JSON(http.StatusUnprocessableEntity, gin.H{ - "message": "Unable to encrypt new password", - }) - return - } - - // To change the role of a user admin role is required - if (updatedUser.Role != user.Role) && (userRole != "Admin") { c.JSON(http.StatusForbidden, gin.H{ "success": false, - "message": "Invalid authorization. User role can only be changed by Admin", + "message": fmt.Sprintf("%v", err), }) return } // Finaly update the user - err = user.update(updatedUser) + err = oldUser.update(updatedUser) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{ "message": "Unable to update user",