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.
This commit is contained in:
smavros 2019-08-15 00:36:03 +02:00
parent 86870e3075
commit 7529f4803f
2 changed files with 50 additions and 41 deletions

View file

@ -1,6 +1,8 @@
package user package user
import ( import (
"fmt"
"gopkg.in/go-playground/validator.v9" "gopkg.in/go-playground/validator.v9"
) )
@ -11,13 +13,17 @@ type loginRequest struct {
Password string `form:"Password" validate:"required,min=6"` Password string `form:"Password" validate:"required,min=6"`
} }
type updateUserRequest struct { type validUpdatedRequest struct {
Username string `form:"Username" validate:"omitempty"` 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"` Role string `form:"Role" validate:"omitempty,oneof=Admin User Guest"`
Mail string `form:"Mail" validate:"omitempty,email"` Mail string `form:"Mail" validate:"omitempty,email"`
} }
type updateUserRequest struct {
validUpdatedRequest `json:"user"`
}
type validNewUser struct { type validNewUser struct {
Username string `form:"Username" validate:"required"` Username string `form:"Username" validate:"required"`
Password string `form:"Password" validate:"required,min=6"` Password string `form:"Password" validate:"required,min=6"`
@ -41,17 +47,37 @@ func (r *updateUserRequest) validate() error {
return errs return errs
} }
func (r *updateUserRequest) createUser(role interface{}) User { func (r *updateUserRequest) updatedUser(role interface{},
var u User 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 // 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 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 { func (r *addUserRequest) validate() error {

View file

@ -253,9 +253,9 @@ func updateUser(c *gin.Context) {
} }
// Find the user // Find the user
var user User var oldUser User
toBeUpdatedID, _ := common.UintParamFromCtx(c, "userID") toBeUpdatedID, _ := common.UintParamFromCtx(c, "userID")
err = user.ByID(toBeUpdatedID) err = oldUser.ByID(toBeUpdatedID)
if err != nil { if err != nil {
c.JSON(http.StatusNotFound, fmt.Sprintf("%v", err)) c.JSON(http.StatusNotFound, fmt.Sprintf("%v", err))
return 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 // 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 // going to be updated AND the role is NOT admin (is already saved
// in the context from the Authentication middleware) the operation // 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 // 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 // 3: If the update is done by a User everything can be updated
userID, _ := c.Get(common.UserIDCtx) // except Role
userRole, _ := c.Get(common.UserRoleCtx) 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{ c.JSON(http.StatusForbidden, gin.H{
"success": false, "success": false,
"message": "Invalid authorization", "message": "Invalid authorization",
@ -279,7 +280,7 @@ func updateUser(c *gin.Context) {
return return
} }
// Bind the (context) with the User struct // Bind the (context) with the updateUserRequest struct
var req updateUserRequest var req updateUserRequest
if err := c.ShouldBindJSON(&req); err != nil { if err := c.ShouldBindJSON(&req); err != nil {
c.JSON(http.StatusUnprocessableEntity, gin.H{ c.JSON(http.StatusUnprocessableEntity, gin.H{
@ -289,7 +290,7 @@ func updateUser(c *gin.Context) {
return return
} }
// Validate the request (taking into acount the role) // Validate the request based on struct updateUserRequest json tags
if err = req.validate(); err != nil { if err = req.validate(); err != nil {
c.JSON(http.StatusBadRequest, gin.H{ c.JSON(http.StatusBadRequest, gin.H{
"success": false, "success": false,
@ -298,37 +299,19 @@ func updateUser(c *gin.Context) {
return return
} }
updatedUser := req.createUser(userRole) // Create the updatedUser from oldUser considering callerRole (in
// case that the request updates the role of the old user)
// Check that the username is NOT taken updatedUser, err := req.updatedUser(callerRole, oldUser)
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)
if err != nil { 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{ c.JSON(http.StatusForbidden, gin.H{
"success": false, "success": false,
"message": "Invalid authorization. User role can only be changed by Admin", "message": fmt.Sprintf("%v", err),
}) })
return return
} }
// Finaly update the user // Finaly update the user
err = user.update(updatedUser) err = oldUser.update(updatedUser)
if err != nil { if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{ c.JSON(http.StatusInternalServerError, gin.H{
"message": "Unable to update user", "message": "Unable to update user",