From 840e79d21265742ef8e32868de551ce81e9bf6ec Mon Sep 17 00:00:00 2001 From: Zsigmond Poh <57909679+zsiggg@users.noreply.github.com> Date: Sun, 12 May 2024 17:55:37 +0800 Subject: [PATCH] Change /groups/{group id}/users route (#115) * fix: inject user id into context in development env * feat: add ID column to list view of Users * feat: add role attribute to /groups/{groupId}/users response a userId could have multiple roles, if it is present in the usergroups table in multiple rows (with different groupId). however, since we specify the groupId in the /group/{groupId}/users route, it makes sense that we only retrieve users that have that groupId. - in controller, get groupId from context - for each user obtained from the users table, call GetUserRoleByID function from usergroups model - store the roles obtained in an array - pass array to updated ListFrom function from users view, which now include a role attribute in its JSON response * fix: DeleteUser in users model previously, the DeleteUser function was not working as it chained `.Delete(&user)` after `.First(&user)`. this resulted in a query as shown, and its corresponding error: * `ERROR: table name "users" specified more than once (SQLSTATE 42712) [8.059ms] [rows:1] UPDATE "users" SET "deleted_at"='2024-02-12 20:52:41.02' FROM "users" WHERE id = 4 AND "users"."deleted_at" IS NULL AND "users"."id" = 4` the intent of `.First(&user)` was likely to store the user to be deleted first in the `user` variable with the use of a `SELECT` SQL statement. however, chaining another method at the end of a finisher method likely led to some errors (see chaining [here](https://gorm.io/docs/method_chaining.html)). thus, this commit attempts to separate the two statements, preserving the initial intent of storing the user to be deleted before deleting, and then returning this user. * feat: function to update row in users model * feat: function to update row in usergroups model * feat: add /users/{userID}/role put route missing validation for parameters, but seemed unnecessary since there is only one route with parameter validation (creating user) * fix: package name for update role's params * Use range iterator for users list view * Remove unused UpdateUser function * Prevent self-update of roles * Fix error message * Add `UpdateRole` param validation * Refactor logic --------- Co-authored-by: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> --- controller/usergroups/update.go | 86 ++++++++++++++++++++++++ controller/users/list.go | 19 +++++- internal/auth/middleware.go | 5 +- internal/permissiongroups/users/users.go | 1 - internal/router/router.go | 1 + model/usergroups.go | 28 ++++++++ params/usergroups/update.go | 28 ++++++++ view/users/list.go | 17 +++-- 8 files changed, 177 insertions(+), 8 deletions(-) create mode 100644 controller/usergroups/update.go create mode 100644 params/usergroups/update.go diff --git a/controller/usergroups/update.go b/controller/usergroups/update.go new file mode 100644 index 0000000..570195c --- /dev/null +++ b/controller/usergroups/update.go @@ -0,0 +1,86 @@ +package usergroups + +import ( + "encoding/json" + "fmt" + "net/http" + "strconv" + + "github.com/go-chi/chi/v5" + "github.com/sirupsen/logrus" + "github.com/source-academy/stories-backend/controller" + "github.com/source-academy/stories-backend/internal/auth" + "github.com/source-academy/stories-backend/internal/database" + apierrors "github.com/source-academy/stories-backend/internal/errors" + userpermissiongroups "github.com/source-academy/stories-backend/internal/permissiongroups/users" + "github.com/source-academy/stories-backend/model" + usergroupparams "github.com/source-academy/stories-backend/params/usergroups" + userviews "github.com/source-academy/stories-backend/view/users" +) + +func HandleUpdateRole(w http.ResponseWriter, r *http.Request) error { + userIDStr := chi.URLParam(r, "userID") + userID, err := strconv.Atoi(userIDStr) + if err != nil { + return apierrors.ClientBadRequestError{ + Message: fmt.Sprintf("Invalid userID: %v", err), + } + } + + err = auth.CheckPermissions(r, userpermissiongroups.Update(uint(userID))) + if err != nil { + logrus.Error(err) + return apierrors.ClientForbiddenError{ + Message: fmt.Sprintf("Error updating user: %v", err), + } + } + + var params usergroupparams.UpdateRole + if err := json.NewDecoder(r.Body).Decode(¶ms); err != nil { + e, ok := err.(*json.UnmarshalTypeError) + if !ok { + logrus.Error(err) + return apierrors.ClientBadRequestError{ + Message: fmt.Sprintf("Bad JSON parsing: %v", err), + } + } + + // TODO: Investigate if we should use errors.Wrap instead + return apierrors.ClientUnprocessableEntityError{ + Message: fmt.Sprintf("Invalid JSON format: %s should be a %s.", e.Field, e.Type), + } + } + + err = params.Validate() + if err != nil { + logrus.Error(err) + return apierrors.ClientUnprocessableEntityError{ + Message: fmt.Sprintf("JSON validation failed: %v", err), + } + } + + updateModel := *params.ToModel(uint(userID)) + + // Get DB instance + db, err := database.GetDBFrom(r) + if err != nil { + logrus.Error(err) + return err + } + + // TODO: Refactor logic + userGroup, err := model.UpdateUserGroupByUserID(db, updateModel.UserID, &updateModel) + if err != nil { + logrus.Error(err) + return err + } + + user, err := model.GetUserByID(db, int(userGroup.UserID)) + if err != nil { + logrus.Error(err) + return err + } + + controller.EncodeJSONResponse(w, userviews.SummaryFrom(user, userGroup)) + return nil +} diff --git a/controller/users/list.go b/controller/users/list.go index a5a373d..c050cca 100644 --- a/controller/users/list.go +++ b/controller/users/list.go @@ -8,8 +8,10 @@ import ( "github.com/source-academy/stories-backend/controller" "github.com/source-academy/stories-backend/internal/auth" "github.com/source-academy/stories-backend/internal/database" + groupenums "github.com/source-academy/stories-backend/internal/enums/groups" apierrors "github.com/source-academy/stories-backend/internal/errors" userpermissiongroups "github.com/source-academy/stories-backend/internal/permissiongroups/users" + "github.com/source-academy/stories-backend/internal/usergroups" "github.com/source-academy/stories-backend/model" userviews "github.com/source-academy/stories-backend/view/users" ) @@ -36,6 +38,21 @@ func HandleList(w http.ResponseWriter, r *http.Request) error { return err } - controller.EncodeJSONResponse(w, userviews.ListFrom(users)) + groupId, err := usergroups.GetGroupIDFrom(r) + if err != nil { + logrus.Error(err) + return err + } + + roles := make([]groupenums.Role, len(users)) + for i, user := range users { + roles[i], err = model.GetUserRoleByID(db, user.ID, *groupId) + if err != nil { + logrus.Error(err) + return err + } + } + + controller.EncodeJSONResponse(w, userviews.ListFrom(users, roles)) return nil } diff --git a/internal/auth/middleware.go b/internal/auth/middleware.go index 88987e3..44317d8 100644 --- a/internal/auth/middleware.go +++ b/internal/auth/middleware.go @@ -28,7 +28,10 @@ func MakeMiddlewareFrom(conf *config.Config) func(http.Handler) http.Handler { // Skip auth in development mode if conf.Environment == envutils.ENV_DEVELOPMENT { return func(next http.Handler) http.Handler { - return next + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r = injectUserIDToContext(r, 1) + next.ServeHTTP(w, r) + }) } } diff --git a/internal/permissiongroups/users/users.go b/internal/permissiongroups/users/users.go index 807830a..24ca482 100644 --- a/internal/permissiongroups/users/users.go +++ b/internal/permissiongroups/users/users.go @@ -30,7 +30,6 @@ func Update(userID uint) permissions.PermissionGroup { Groups: []permissions.PermissionGroup{ userpermissions. GetRolePermission(userpermissions.CanUpdateUsers), - IsSelf{UserID: userID}, }, } } diff --git a/internal/router/router.go b/internal/router/router.go index 9c70a40..0d2c041 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -64,6 +64,7 @@ func Setup(config *config.Config, injectMiddleWares []func(http.Handler) http.Ha r.Route("/users", func(r chi.Router) { r.Get("/", handleAPIError(users.HandleList)) r.Get("/{userID}", handleAPIError(users.HandleRead)) + r.Put("/{userID}/role", handleAPIError(usergroupscontroller.HandleUpdateRole)) r.Delete("/{userID}", handleAPIError(users.HandleDelete)) r.Post("/", handleAPIError(users.HandleCreate)) r.Post("/batch", handleAPIError(usergroupscontroller.HandleBatchCreate)) diff --git a/model/usergroups.go b/model/usergroups.go index 3bab68e..25e094f 100644 --- a/model/usergroups.go +++ b/model/usergroups.go @@ -38,3 +38,31 @@ func CreateUserGroup(db *gorm.DB, userGroup *UserGroup) error { } return nil } + +func GetUserRoleByID(db *gorm.DB, userID uint, groupID uint) (groupenums.Role, error) { + userGroup, err := GetUserGroupByID(db, userID, groupID) + if err != nil { + return userGroup.Role, database.HandleDBError(err, "userGroup") + } + return userGroup.Role, nil +} + +func UpdateUserGroupByUserID(db *gorm.DB, userID uint, updates *UserGroup) (UserGroup, error) { + var userGroup UserGroup + + // Check if the user is trying to update another user's role + if updates.UserID != 0 && updates.UserID != userID { + return userGroup, database.HandleDBError(nil, "userGroup") + } + + err := db.Model(&userGroup). + Preload(clause.Associations). + Where(UserGroup{UserID: userID}). + Updates(&userGroup). + Error + + if err != nil { + return userGroup, database.HandleDBError(err, "userGroup") + } + return userGroup, nil +} diff --git a/params/usergroups/update.go b/params/usergroups/update.go new file mode 100644 index 0000000..1f3843b --- /dev/null +++ b/params/usergroups/update.go @@ -0,0 +1,28 @@ +package usergroupparams + +import ( + "fmt" + + groupenums "github.com/source-academy/stories-backend/internal/enums/groups" + "github.com/source-academy/stories-backend/model" +) + +type UpdateRole struct { + Role groupenums.Role `json:"role"` +} + +func (params *UpdateRole) ToModel(userID uint) *model.UserGroup { + return &model.UserGroup{ + UserID: userID, + Role: params.Role, + } +} + +func (params *UpdateRole) Validate() error { + // Extra params won't do anything, e.g. authorID can't be changed. + // TODO: Error on extra params? + if !params.Role.IsValid() { + return fmt.Errorf("Invalid role %s.", params.Role) + } + return nil +} diff --git a/view/users/list.go b/view/users/list.go index 654eb1f..63acd49 100644 --- a/view/users/list.go +++ b/view/users/list.go @@ -1,22 +1,29 @@ package userviews -import "github.com/source-academy/stories-backend/model" +import ( + groupenums "github.com/source-academy/stories-backend/internal/enums/groups" + "github.com/source-academy/stories-backend/model" +) type ListView struct { - Name string `json:"name"` - Username string `json:"username"` - LoginProvider string `json:"provider"` + ID uint `json:"id"` + Name string `json:"name"` + Username string `json:"username"` + LoginProvider string `json:"provider"` + Role groupenums.Role `json:"role"` } -func ListFrom(users []model.User) []ListView { +func ListFrom(users []model.User, roles []groupenums.Role) []ListView { usersListView := make([]ListView, len(users)) for i, user := range users { usersListView[i] = ListView{ // Unlike other views, we do not fallback an empty name to // the username for the users' list view. + ID: user.ID, Name: user.FullName, Username: user.Username, LoginProvider: user.LoginProvider.ToString(), + Role: roles[i], } } return usersListView