From 0193bfd7de03a3e50118bf8ffdda419d9d64c53a Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Fri, 9 Dec 2022 11:43:13 +0530 Subject: [PATCH] [MM-47378] Respond with bad requests for wrong query parameters 'roles' in getUsers (#21569) * Respond with bad requests for wrong query parameters in roles * Revert "Respond with bad requests for wrong query parameters in roles" This reverts commit d8374d94e0b1f61ad445127010f9780475c48d1a. * Add GetUser client function to query with channel_id and roles * Return bad parameters error on invalid roles * Make client function generic, lint fixes * i18n strings addition * Validate 'role', add stricter check for comma separated roles --- api4/user.go | 40 +++++++++++++++++++++++++ api4/user_local.go | 73 ++++++++++++++++++++++++++++++++++++++++++++++ api4/user_test.go | 14 +++++++++ i18n/en.json | 4 +++ model/client4.go | 18 ++++++++++++ 5 files changed, 149 insertions(+) diff --git a/api4/user.go b/api4/user.go index 15af912ab7..e0e32708b3 100644 --- a/api4/user.go +++ b/api4/user.go @@ -693,14 +693,44 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) { c.SetInvalidURLParam("inactive") } + roleNamesAll := []string{} + // MM-47378: validate 'role' related parameters + if role != "" || rolesString != "" || channelRolesString != "" || teamRolesString != "" { + // fetch all role names + rolesAll, err := c.App.GetAllRoles() + if err != nil { + c.Err = model.NewAppError("Api4.getUsers", "api.user.get_users.validation.app_error", nil, "Error fetching roles during validation.", http.StatusBadRequest) + return + } + for _, role := range rolesAll { + roleNamesAll = append(roleNamesAll, role.Name) + } + } roles := []string{} var rolesValid bool + if role != "" { + roles, rolesValid = model.CleanRoleNames([]string{role}) + if !rolesValid { + c.SetInvalidParam("role") + return + } + roleValid := utils.StringInSlice(role, roleNamesAll) + if !roleValid { + c.SetInvalidParam("role") + return + } + } if rolesString != "" { roles, rolesValid = model.CleanRoleNames(strings.Split(rolesString, ",")) if !rolesValid { c.SetInvalidParam("roles") return } + validRoleNames := utils.StringArrayIntersection(roleNamesAll, roles) + if len(validRoleNames) != len(roles) { + c.SetInvalidParam("roles") + return + } } channelRoles := []string{} if channelRolesString != "" && inChannelId != "" { @@ -709,6 +739,11 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) { c.SetInvalidParam("channelRoles") return } + validRoleNames := utils.StringArrayIntersection(roleNamesAll, channelRoles) + if len(validRoleNames) != len(channelRoles) { + c.SetInvalidParam("channelRoles") + return + } } teamRoles := []string{} if teamRolesString != "" && inTeamId != "" { @@ -717,6 +752,11 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) { c.SetInvalidParam("teamRoles") return } + validRoleNames := utils.StringArrayIntersection(roleNamesAll, teamRoles) + if len(validRoleNames) != len(teamRoles) { + c.SetInvalidParam("teamRoles") + return + } } restrictions, appErr := c.App.GetViewUsersRestrictions(c.AppContext.Session().UserId) diff --git a/api4/user_local.go b/api4/user_local.go index 50a451ea96..79578f2484 100644 --- a/api4/user_local.go +++ b/api4/user_local.go @@ -7,11 +7,13 @@ import ( "encoding/json" "net/http" "strconv" + "strings" "github.com/mattermost/mattermost-server/v6/audit" "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/shared/mlog" "github.com/mattermost/mattermost-server/v6/store" + "github.com/mattermost/mattermost-server/v6/utils" ) func (api *API) InitUserLocal() { @@ -56,7 +58,78 @@ func localGetUsers(c *Context, w http.ResponseWriter, r *http.Request) { active := r.URL.Query().Get("active") inactive := r.URL.Query().Get("inactive") role := r.URL.Query().Get("role") + rolesString := r.URL.Query().Get("roles") + channelRolesString := r.URL.Query().Get("channel_roles") + teamRolesString := r.URL.Query().Get("team_roles") sort := r.URL.Query().Get("sort") + roleNamesAll := []string{} + // MM-47378: validate 'role' related parameters + if role != "" || rolesString != "" || channelRolesString != "" || teamRolesString != "" { + // fetch all role names + rolesAll, err := c.App.GetAllRoles() + if err != nil { + c.Err = model.NewAppError("Api4.getUsers", "api.user.get_users.validation.app_error", nil, "Error fetching roles during validation.", http.StatusBadRequest) + return + } + for _, role := range rolesAll { + roleNamesAll = append(roleNamesAll, role.Name) + } + } + + var roles []string + var rolesValid bool + + if role != "" { + _, rolesValid = model.CleanRoleNames([]string{role}) + if !rolesValid { + c.SetInvalidParam("role") + return + } + roleValid := utils.StringInSlice(role, roleNamesAll) + if !roleValid { + c.SetInvalidParam("role") + return + } + } + + if rolesString != "" { + roles, rolesValid = model.CleanRoleNames(strings.Split(rolesString, ",")) + if !rolesValid { + c.SetInvalidParam("roles") + return + } + validRoleNames := utils.StringArrayIntersection(roleNamesAll, roles) + if len(validRoleNames) != len(roles) { + c.SetInvalidParam("roles") + return + } + } + var channelRoles []string + if channelRolesString != "" && inChannelId != "" { + channelRoles, rolesValid = model.CleanRoleNames(strings.Split(channelRolesString, ",")) + if !rolesValid { + c.SetInvalidParam("channelRoles") + return + } + validRoleNames := utils.StringArrayIntersection(roleNamesAll, channelRoles) + if len(validRoleNames) != len(channelRoles) { + c.SetInvalidParam("channelRoles") + return + } + } + var teamRoles []string + if teamRolesString != "" && inTeamId != "" { + teamRoles, rolesValid = model.CleanRoleNames(strings.Split(teamRolesString, ",")) + if !rolesValid { + c.SetInvalidParam("teamRoles") + return + } + validRoleNames := utils.StringArrayIntersection(roleNamesAll, teamRoles) + if len(validRoleNames) != len(teamRoles) { + c.SetInvalidParam("teamRoles") + return + } + } if notInChannelId != "" && inTeamId == "" { c.SetInvalidURLParam("team_id") diff --git a/api4/user_test.go b/api4/user_test.go index 17195f7d6d..871f2c56e8 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -2410,6 +2410,20 @@ func TestGetUsers(t *testing.T) { // Check default params for page and per_page _, err = client.DoAPIGet("/users", "") require.NoError(t, err) + + // Check role params validity + _, _, err = client.GetUsersWithCustomQueryParameters(0, 5, "in_channel=random_channel_id&channel_roles=random_role_doesnt_exist", "") + require.Error(t, err) + require.Equal(t, err.Error(), ": Invalid or missing channelRoles in request body.") + _, _, err = client.GetUsersWithCustomQueryParameters(0, 5, "in_team=random_channel_id&team_roles=random_role_doesnt_exist", "") + require.Error(t, err) + require.Equal(t, err.Error(), ": Invalid or missing teamRoles in request body.") + _, _, err = client.GetUsersWithCustomQueryParameters(0, 5, "roles=random_role_doesnt_exist%2Csystem_user", "") + require.Error(t, err) + require.Equal(t, err.Error(), ": Invalid or missing roles in request body.") + _, _, err = client.GetUsersWithCustomQueryParameters(0, 5, "role=random_role_doesnt_exist", "") + require.Error(t, err) + require.Equal(t, err.Error(), ": Invalid or missing role in request body.") }) th.Client.Logout() diff --git a/i18n/en.json b/i18n/en.json index 43b65baa82..d1399190f6 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -4131,6 +4131,10 @@ "id": "api.user.get_user_by_email.permissions.app_error", "translation": "Unable to get user by email." }, + { + "id": "api.user.get_users.validation.app_error", + "translation": "Error fetching roles during validation." + }, { "id": "api.user.invalidate_verify_email_tokens.error", "translation": "Unable to get tokens by type when invalidating email verification tokens" diff --git a/model/client4.go b/model/client4.go index 5347048f17..511bc44a3f 100644 --- a/model/client4.go +++ b/model/client4.go @@ -1060,6 +1060,24 @@ func (c *Client4) GetUsers(page int, perPage int, etag string) ([]*User, *Respon return list, BuildResponse(r), nil } +// GetUsersWithChannelRoles returns a page of users on the system. Page counting starts at 0. +func (c *Client4) GetUsersWithCustomQueryParameters(page int, perPage int, queryParameters, etag string) ([]*User, *Response, error) { + query := fmt.Sprintf("?page=%v&per_page=%v&%v", page, perPage, queryParameters) + r, err := c.DoAPIGet(c.usersRoute()+query, etag) + if err != nil { + return nil, BuildResponse(r), err + } + defer closeBody(r) + var list []*User + if r.StatusCode == http.StatusNotModified { + return list, BuildResponse(r), nil + } + if err := json.NewDecoder(r.Body).Decode(&list); err != nil { + return nil, nil, NewAppError("GetUsers", "api.unmarshal_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + return list, BuildResponse(r), nil +} + // GetUsersInTeam returns a page of users on a team. Page counting starts at 0. func (c *Client4) GetUsersInTeam(teamId string, page int, perPage int, etag string) ([]*User, *Response, error) { query := fmt.Sprintf("?in_team=%v&page=%v&per_page=%v", teamId, page, perPage)