[MM-28717] Refactor applyMultiRoleFilters to use sq builder (#15500)

* Refactor apply multi role filters and add role filters to get all profiles * Add some tests * Fix tests * Fix lint * Trigger CI * Rename param to make more sense * Tie get filtered user stats to usermanagement read users * Dont filter out other system roles when searching for team members or team admins only filter out system admins * add new permissions * add migration * fix test * remove system roles as default permissions * implement changes discussed with dennis * add read only and fix i18n * use model consts instead of strings * turn the permissions into pseudo constants * Update read only default permissions Co-authored-by: Mattermod <mattermod@users.noreply.github.com> Co-authored-by: Hossein Ahmadian-Yazdi <hyazdi1997@gmail.com>
2025-02-25 18:55:24 -06:00 · 2020-11-13 10:57:57 -05:00
parent 1d15900f84
commit c9a4a475d3
11 changed files with 268 additions and 126 deletions
--- a/api4/role.go
+++ b/api4/role.go
@@ -10,6 +10,24 @@ import (
 	"github.com/mattermost/mattermost-server/v5/model"
 )

+var allowedPermissions = []string{
+	model.PERMISSION_CREATE_TEAM.Id,
+	model.PERMISSION_MANAGE_INCOMING_WEBHOOKS.Id,
+	model.PERMISSION_MANAGE_OUTGOING_WEBHOOKS.Id,
+	model.PERMISSION_MANAGE_SLASH_COMMANDS.Id,
+	model.PERMISSION_MANAGE_OAUTH.Id,
+	model.PERMISSION_MANAGE_SYSTEM_WIDE_OAUTH.Id,
+	model.PERMISSION_CREATE_EMOJIS.Id,
+	model.PERMISSION_DELETE_EMOJIS.Id,
+	model.PERMISSION_EDIT_OTHERS_POSTS.Id,
+}
+
+var notAllowedPermissions = []string{
+	model.PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_SYSTEM_ROLES.Id,
+	model.PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_SYSTEM_ROLES.Id,
+	model.PERMISSION_MANAGE_ROLES.Id,
+}
+
 func (api *API) InitRole() {
 	api.BaseRoutes.Roles.Handle("/{role_id:[A-Za-z0-9]+}", api.ApiSessionRequiredTrustRequester(getRole)).Methods("GET")
 	api.BaseRoutes.Roles.Handle("/name/{role_name:[a-z0-9_]+}", api.ApiSessionRequiredTrustRequester(getRoleByName)).Methods("GET")
@@ -92,22 +110,17 @@ func patchRole(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 	auditRec.AddMeta("role", oldRole)

+	if oldRole.Name == model.SYSTEM_ADMIN_ROLE_ID {
+		c.Err = model.NewAppError("Api4.PatchRoles", "api.roles.patch_roles.admin_role.error", nil, "", http.StatusNotImplemented)
+		return
+	}
+
+	isGuest := oldRole.Name == model.SYSTEM_GUEST_ROLE_ID || oldRole.Name == model.TEAM_GUEST_ROLE_ID || oldRole.Name == model.CHANNEL_GUEST_ROLE_ID
 	if c.App.Srv().License() == nil && patch.Permissions != nil {
-		if oldRole.Name == "system_guest" || oldRole.Name == "team_guest" || oldRole.Name == "channel_guest" {
+		if isGuest {
 			c.Err = model.NewAppError("Api4.PatchRoles", "api.roles.patch_roles.license.error", nil, "", http.StatusNotImplemented)
 			return
 		}
-		allowedPermissions := []string{
-			model.PERMISSION_CREATE_TEAM.Id,
-			model.PERMISSION_MANAGE_INCOMING_WEBHOOKS.Id,
-			model.PERMISSION_MANAGE_OUTGOING_WEBHOOKS.Id,
-			model.PERMISSION_MANAGE_SLASH_COMMANDS.Id,
-			model.PERMISSION_MANAGE_OAUTH.Id,
-			model.PERMISSION_MANAGE_SYSTEM_WIDE_OAUTH.Id,
-			model.PERMISSION_CREATE_EMOJIS.Id,
-			model.PERMISSION_DELETE_EMOJIS.Id,
-			model.PERMISSION_EDIT_OTHERS_POSTS.Id,
-		}

 		changedPermissions := model.PermissionsChangedByPatch(oldRole, patch)
 		for _, permission := range changedPermissions {
@@ -125,14 +138,39 @@ func patchRole(c *Context, w http.ResponseWriter, r *http.Request) {
 		}
 	}

-	if c.App.Srv().License() != nil && (oldRole.Name == "system_guest" || oldRole.Name == "team_guest" || oldRole.Name == "channel_guest") && !*c.App.Srv().License().Features.GuestAccountsPermissions {
+	if patch.Permissions != nil {
+		deltaPermissions := model.PermissionsChangedByPatch(oldRole, patch)
+
+		for _, permission := range deltaPermissions {
+			notAllowed := false
+			for _, notAllowedPermission := range notAllowedPermissions {
+				if permission == notAllowedPermission {
+					notAllowed = true
+				}
+			}
+
+			if notAllowed {
+				c.Err = model.NewAppError("Api4.PatchRoles", "api.roles.patch_roles.not_allowed_permission.error", nil, "Cannot add or remove permission: "+permission, http.StatusNotImplemented)
+				return
+			}
+		}
+	}
+
+	if c.App.Srv().License() != nil && isGuest && !*c.App.Srv().License().Features.GuestAccountsPermissions {
 		c.Err = model.NewAppError("Api4.PatchRoles", "api.roles.patch_roles.license.error", nil, "", http.StatusNotImplemented)
 		return
 	}

-	if !c.App.SessionHasPermissionTo(*c.App.Session(), model.PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_PERMISSIONS) {
-		c.SetPermissionError(model.PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_PERMISSIONS)
-		return
+	if oldRole.Name == model.TEAM_ADMIN_ROLE_ID || oldRole.Name == model.CHANNEL_ADMIN_ROLE_ID || oldRole.Name == model.SYSTEM_USER_ROLE_ID || oldRole.Name == model.TEAM_USER_ROLE_ID || oldRole.Name == model.CHANNEL_USER_ROLE_ID || oldRole.Name == model.SYSTEM_GUEST_ROLE_ID || oldRole.Name == model.TEAM_GUEST_ROLE_ID || oldRole.Name == model.CHANNEL_GUEST_ROLE_ID {
+		if !c.App.SessionHasPermissionTo(*c.App.Session(), model.PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_PERMISSIONS) {
+			c.SetPermissionError(model.PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_PERMISSIONS)
+			return
+		}
+	} else {
+		if !c.App.SessionHasPermissionTo(*c.App.Session(), model.PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_SYSTEM_ROLES) {
+			c.SetPermissionError(model.PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_SYSTEM_ROLES)
+			return
+		}
 	}

 	role, err := c.App.PatchRole(oldRole, patch)
--- a/api4/role_test.go
+++ b/api4/role_test.go
@@ -177,6 +177,43 @@ func TestPatchRole(t *testing.T) {
 		Permissions: &[]string{"manage_system", "create_public_channel", "manage_incoming_webhooks", "manage_outgoing_webhooks"},
 	}

+	th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) {
+
+		// Cannot edit a system admin
+		adminRole, err := th.App.Srv().Store.Role().GetByName("system_admin")
+		assert.Nil(t, err)
+		defer th.App.Srv().Store.Job().Delete(adminRole.Id)
+
+		_, resp := client.PatchRole(adminRole.Id, patch)
+		CheckNotImplementedStatus(t, resp)
+
+		// Cannot give other roles read / write to system roles or manage roles because only system admin can do these actions
+		systemManager, err := th.App.Srv().Store.Role().GetByName("system_manager")
+		assert.Nil(t, err)
+		defer th.App.Srv().Store.Job().Delete(systemManager.Id)
+
+		patchWriteSystemRoles := &model.RolePatch{
+			Permissions: &[]string{model.PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_SYSTEM_ROLES.Id},
+		}
+
+		_, resp = client.PatchRole(systemManager.Id, patchWriteSystemRoles)
+		CheckNotImplementedStatus(t, resp)
+
+		patchReadSystemRoles := &model.RolePatch{
+			Permissions: &[]string{model.PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_SYSTEM_ROLES.Id},
+		}
+
+		_, resp = client.PatchRole(systemManager.Id, patchReadSystemRoles)
+		CheckNotImplementedStatus(t, resp)
+
+		patchManageRoles := &model.RolePatch{
+			Permissions: &[]string{model.PERMISSION_MANAGE_ROLES.Id},
+		}
+
+		_, resp = client.PatchRole(systemManager.Id, patchManageRoles)
+		CheckNotImplementedStatus(t, resp)
+	})
+
 	th.TestForSystemAdminAndLocal(t, func(t *testing.T, client *model.Client4) {
 		received, resp := client.PatchRole(role.Id, patch)
 		CheckNoError(t, resp)
--- a/api4/user.go
+++ b/api4/user.go
@@ -585,8 +585,8 @@ func getFilteredUsersStats(c *Context, w http.ResponseWriter, r *http.Request) {
 		TeamRoles:          teamRoles,
 	}

-	if !c.App.SessionHasPermissionTo(*c.App.Session(), model.PERMISSION_MANAGE_SYSTEM) {
-		c.SetPermissionError(model.PERMISSION_MANAGE_SYSTEM)
+	if !c.App.SessionHasPermissionTo(*c.App.Session(), model.PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_USERS) {
+		c.SetPermissionError(model.PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_USERS)
 		return
 	}

--- a/app/permissions_migrations.go
+++ b/app/permissions_migrations.go
@@ -502,6 +502,15 @@ func (a *App) getAddConvertChannelPermissionsMigration() (permissionsMap, error)
 	}, nil
 }

+func (a *App) getSystemRolesPermissionsMigration() (permissionsMap, error) {
+	return permissionsMap{
+		permissionTransformation{
+			On:  isRole(model.SYSTEM_ADMIN_ROLE_ID),
+			Add: []string{model.PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_SYSTEM_ROLES.Id, model.PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_SYSTEM_ROLES.Id},
+		},
+	}, nil
+}
+
 func (a *App) getAddManageSharedChannelsPermissionsMigration() (permissionsMap, error) {
 	return permissionsMap{
 		permissionTransformation{
@@ -531,6 +540,7 @@ func (a *App) DoPermissionsMigrations() error {
 		{Key: model.MIGRATION_KEY_ADD_SYSTEM_CONSOLE_PERMISSIONS, Migration: a.getAddSystemConsolePermissionsMigration},
 		{Key: model.MIGRATION_KEY_ADD_CONVERT_CHANNEL_PERMISSIONS, Migration: a.getAddConvertChannelPermissionsMigration},
 		{Key: model.MIGRATION_KEY_ADD_MANAGE_SHARED_CHANNEL_PERMISSIONS, Migration: a.getAddManageSharedChannelsPermissionsMigration},
+		{Key: model.MIGRATION_KEY_ADD_SYSTEM_ROLES_PERMISSIONS, Migration: a.getSystemRolesPermissionsMigration},
 	}

 	for _, migration := range PermissionsMigrations {
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -1954,10 +1954,18 @@
    "id": "api.restricted_system_admin",
    "translation": "This action is forbidden to a restricted system admin."
  },
+  {
+    "id": "api.roles.patch_roles.admin_role.error",
+    "translation": "System Admin role cannot be edited or changed"
+  },
  {
    "id": "api.roles.patch_roles.license.error",
    "translation": "Your license does not support advanced permissions."
  },
+  {
+    "id": "api.roles.patch_roles.not_allowed_permission.error",
+    "translation": "One or more of the following permissions that you are trying to add or remove is not allowed"
+  },
  {
    "id": "api.scheme.create_scheme.license.error",
    "translation": "Your license does not support creating permissions schemes."
--- a/model/migration.go
+++ b/model/migration.go
@@ -21,5 +21,6 @@ const (
 	MIGRATION_KEY_ADD_SYSTEM_CONSOLE_PERMISSIONS              = "add_system_console_permissions"
 	MIGRATION_KEY_SIDEBAR_CATEGORIES_PHASE_2                  = "migration_sidebar_categories_phase_2"
 	MIGRATION_KEY_ADD_CONVERT_CHANNEL_PERMISSIONS             = "add_convert_channel_permissions"
+	MIGRATION_KEY_ADD_SYSTEM_ROLES_PERMISSIONS                = "add_system_roles_permissions"
 	MIGRATION_KEY_ADD_MANAGE_SHARED_CHANNEL_PERMISSIONS       = "manage_shared_channel_permissions"
 )
--- a/model/permission.go
+++ b/model/permission.go
@@ -122,6 +122,9 @@ var PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_CHANNELS *Permission
 var PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_PERMISSIONS *Permission
 var PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_PERMISSIONS *Permission

+var PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_SYSTEM_ROLES *Permission
+var PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_SYSTEM_ROLES *Permission
+
 var PERMISSION_SYSCONSOLE_READ_ENVIRONMENT *Permission
 var PERMISSION_SYSCONSOLE_WRITE_ENVIRONMENT *Permission

@@ -757,6 +760,18 @@ func initializePermissions() {
 		"authentication.permissions.use_group_mentions.description",
 		PermissionScopeSystem,
 	}
+	PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_SYSTEM_ROLES = &Permission{
+		"sysconsole_read_user_management_system_roles",
+		"authentication.permissions.use_group_mentions.name",
+		"authentication.permissions.use_group_mentions.description",
+		PermissionScopeSystem,
+	}
+	PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_SYSTEM_ROLES = &Permission{
+		"sysconsole_write_user_management_system_roles",
+		"authentication.permissions.use_group_mentions.name",
+		"authentication.permissions.use_group_mentions.description",
+		PermissionScopeSystem,
+	}
 	PERMISSION_SYSCONSOLE_READ_ENVIRONMENT = &Permission{
 		"sysconsole_read_environment",
 		"authentication.permissions.use_group_mentions.name",
@@ -862,6 +877,7 @@ func initializePermissions() {
 		PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_TEAMS,
 		PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_CHANNELS,
 		PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_PERMISSIONS,
+		PERMISSION_SYSCONSOLE_READ_USERMANAGEMENT_SYSTEM_ROLES,
 		PERMISSION_SYSCONSOLE_READ_ENVIRONMENT,
 		PERMISSION_SYSCONSOLE_READ_SITE,
 		PERMISSION_SYSCONSOLE_READ_AUTHENTICATION,
@@ -879,6 +895,7 @@ func initializePermissions() {
 		PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_TEAMS,
 		PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_CHANNELS,
 		PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_PERMISSIONS,
+		PERMISSION_SYSCONSOLE_WRITE_USERMANAGEMENT_SYSTEM_ROLES,
 		PERMISSION_SYSCONSOLE_WRITE_ENVIRONMENT,
 		PERMISSION_SYSCONSOLE_WRITE_SITE,
 		PERMISSION_SYSCONSOLE_WRITE_AUTHENTICATION,
--- a/model/role.go
+++ b/model/role.go
@@ -133,6 +133,7 @@ func init() {
 		PERMISSION_SYSCONSOLE_READ_SITE.Id,
 		PERMISSION_SYSCONSOLE_READ_AUTHENTICATION.Id,
 		PERMISSION_SYSCONSOLE_READ_PLUGINS.Id,
+		PERMISSION_SYSCONSOLE_READ_COMPLIANCE.Id,
 		PERMISSION_SYSCONSOLE_READ_INTEGRATIONS.Id,
 		PERMISSION_SYSCONSOLE_READ_EXPERIMENTAL.Id,
 	}
--- a/store/sqlstore/user_store.go
+++ b/store/sqlstore/user_store.go
@@ -412,6 +412,7 @@ func (us SqlUserStore) GetAllProfiles(options *model.UserGetOptions) ([]*model.U
 	query = applyViewRestrictionsFilter(query, options.ViewRestrictions, true)

 	query = applyRoleFilter(query, options.Role, isPostgreSQL)
+	query = applyMultiRoleFilters(query, options.Roles, []string{}, []string{}, isPostgreSQL)

 	if options.Inactive {
 		query = query.Where("u.DeleteAt != 0")
@@ -451,119 +452,75 @@ func applyRoleFilter(query sq.SelectBuilder, role string, isPostgreSQL bool) sq.
 	return query.Where("u.Roles LIKE ? ESCAPE '*'", roleParam)
 }

-func applyMultiRoleFilters(query sq.SelectBuilder, roles []string, teamRoles []string, channelRoles []string) sq.SelectBuilder {
-	queryString := ""
-	if len(roles) > 0 && roles[0] != "" {
-		schemeGuest := false
-		schemeAdmin := false
-		schemeUser := false
+func applyMultiRoleFilters(query sq.SelectBuilder, systemRoles []string, teamRoles []string, channelRoles []string, isPostgreSQL bool) sq.SelectBuilder {
+	sqOr := sq.Or{}

-		for _, role := range roles {
+	if len(systemRoles) > 0 && systemRoles[0] != "" {
+		for _, role := range systemRoles {
+			queryRole := wildcardSearchTerm(role)
 			switch role {
-			case model.SYSTEM_ADMIN_ROLE_ID:
-				schemeAdmin = true
 			case model.SYSTEM_USER_ROLE_ID:
-				schemeUser = true
-			case model.SYSTEM_GUEST_ROLE_ID:
-				schemeGuest = true
-			}
-		}
-
-		if schemeAdmin || schemeUser || schemeGuest {
-			if schemeAdmin && schemeUser {
-				queryString += `(u.Roles LIKE '%system_user%' OR u.Roles LIKE '%system_admin%') `
-			} else if schemeAdmin {
-				queryString += `(u.Roles LIKE '%system_admin%') `
-			} else if schemeUser {
-				queryString += `(u.Roles LIKE '%system_user%' AND u.Roles NOT LIKE '%system_admin%') `
-			}
-
-			if schemeGuest {
-				if queryString != "" {
-					queryString += "OR "
+				// If querying for a `system_user` ensure that the user is only a system_user.
+				sqOr = append(sqOr, sq.Eq{"u.Roles": role})
+			case model.SYSTEM_GUEST_ROLE_ID, model.SYSTEM_ADMIN_ROLE_ID, model.SYSTEM_USER_MANAGER_ROLE_ID, model.SYSTEM_READ_ONLY_ADMIN_ROLE_ID, model.SYSTEM_MANAGER_ROLE_ID:
+				// If querying for any other roles search using a wildcard.
+				if isPostgreSQL {
+					sqOr = append(sqOr, sq.ILike{"u.Roles": queryRole})
+				} else {
+					sqOr = append(sqOr, sq.Like{"u.Roles": queryRole})
 				}
-				queryString += `(u.Roles LIKE '%system_guest%') `
 			}
+
 		}
 	}

 	if len(channelRoles) > 0 && channelRoles[0] != "" {
-		schemeGuest := false
-		schemeAdmin := false
-		schemeUser := false
 		for _, channelRole := range channelRoles {
 			switch channelRole {
 			case model.CHANNEL_ADMIN_ROLE_ID:
-				schemeAdmin = true
-			case model.CHANNEL_USER_ROLE_ID:
-				schemeUser = true
-			case model.CHANNEL_GUEST_ROLE_ID:
-				schemeGuest = true
-			}
-		}
-
-		if schemeAdmin || schemeUser || schemeGuest {
-			if queryString != "" {
-				queryString += "OR "
-			}
-			if schemeAdmin && schemeUser {
-				queryString += `(cm.SchemeUser = true AND u.Roles = 'system_user')`
-			} else if schemeAdmin {
-				queryString += `(cm.SchemeAdmin = true AND u.Roles = 'system_user')`
-			} else if schemeUser {
-				queryString += `(cm.SchemeUser = true AND cm.SchemeAdmin = false AND u.Roles = 'system_user')`
-			}
-
-			if schemeGuest {
-				if queryString != "" && queryString[len(queryString)-3:] != "OR " {
-					queryString += "OR "
+				if isPostgreSQL {
+					sqOr = append(sqOr, sq.And{sq.Eq{"cm.SchemeAdmin": true}, sq.NotILike{"u.Roles": wildcardSearchTerm(model.SYSTEM_ADMIN_ROLE_ID)}})
+				} else {
+					sqOr = append(sqOr, sq.And{sq.Eq{"cm.SchemeAdmin": true}, sq.NotLike{"u.Roles": wildcardSearchTerm(model.SYSTEM_ADMIN_ROLE_ID)}})
 				}
-				queryString += `(cm.SchemeGuest = true AND u.Roles = 'system_guest')`
+			case model.CHANNEL_USER_ROLE_ID:
+				if isPostgreSQL {
+					sqOr = append(sqOr, sq.And{sq.Eq{"cm.SchemeUser": true}, sq.Eq{"cm.SchemeAdmin": false}, sq.NotILike{"u.Roles": wildcardSearchTerm(model.SYSTEM_ADMIN_ROLE_ID)}})
+				} else {
+					sqOr = append(sqOr, sq.And{sq.Eq{"cm.SchemeUser": true}, sq.Eq{"cm.SchemeAdmin": false}, sq.NotLike{"u.Roles": wildcardSearchTerm(model.SYSTEM_ADMIN_ROLE_ID)}})
+				}
+			case model.CHANNEL_GUEST_ROLE_ID:
+				sqOr = append(sqOr, sq.Eq{"cm.SchemeGuest": true})
 			}
 		}
 	}

 	if len(teamRoles) > 0 && teamRoles[0] != "" {
-		schemeAdmin := false
-		schemeUser := false
-		schemeGuest := false
 		for _, teamRole := range teamRoles {
 			switch teamRole {
 			case model.TEAM_ADMIN_ROLE_ID:
-				schemeAdmin = true
-			case model.TEAM_USER_ROLE_ID:
-				schemeUser = true
-			case model.TEAM_GUEST_ROLE_ID:
-				schemeGuest = true
-			}
-		}
-
-		if schemeAdmin || schemeUser || schemeGuest {
-			if queryString != "" {
-				queryString += "OR "
-			}
-			if schemeAdmin && schemeUser {
-				queryString += `(tm.SchemeUser = true AND u.Roles = 'system_user')`
-			} else if schemeAdmin {
-				queryString += `(tm.SchemeAdmin = true AND u.Roles = 'system_user')`
-			} else if schemeUser {
-				queryString += `(tm.SchemeUser = true AND tm.SchemeAdmin = false AND u.Roles = 'system_user')`
-			}
-
-			if schemeGuest {
-				if queryString != "" && queryString[len(queryString)-3:] != "OR " {
-					queryString += "OR "
+				if isPostgreSQL {
+					sqOr = append(sqOr, sq.And{sq.Eq{"tm.SchemeAdmin": true}, sq.NotILike{"u.Roles": wildcardSearchTerm(model.SYSTEM_ADMIN_ROLE_ID)}})
+				} else {
+					sqOr = append(sqOr, sq.And{sq.Eq{"tm.SchemeAdmin": true}, sq.NotLike{"u.Roles": wildcardSearchTerm(model.SYSTEM_ADMIN_ROLE_ID)}})
 				}
-				queryString += `(tm.SchemeGuest = true AND u.Roles = 'system_guest')`
+			case model.TEAM_USER_ROLE_ID:
+				if isPostgreSQL {
+					sqOr = append(sqOr, sq.And{sq.Eq{"tm.SchemeUser": true}, sq.Eq{"tm.SchemeAdmin": false}, sq.NotILike{"u.Roles": wildcardSearchTerm(model.SYSTEM_ADMIN_ROLE_ID)}})
+				} else {
+					sqOr = append(sqOr, sq.And{sq.Eq{"tm.SchemeUser": true}, sq.Eq{"tm.SchemeAdmin": false}, sq.NotLike{"u.Roles": wildcardSearchTerm(model.SYSTEM_ADMIN_ROLE_ID)}})
+				}
+			case model.TEAM_GUEST_ROLE_ID:
+				sqOr = append(sqOr, sq.Eq{"tm.SchemeGuest": true})
 			}
 		}
 	}

-	if queryString != "" {
-		query = query.Where("(" + queryString + ")")
+	if len(sqOr) > 0 {
+		return query.Where(sqOr)
+	} else {
+		return query
 	}
-
-	return query
 }

 func applyChannelGroupConstrainedFilter(query sq.SelectBuilder, channelId string) sq.SelectBuilder {
@@ -633,7 +590,7 @@ func (us SqlUserStore) GetProfiles(options *model.UserGetOptions) ([]*model.User
 	query = applyViewRestrictionsFilter(query, options.ViewRestrictions, true)

 	query = applyRoleFilter(query, options.Role, isPostgreSQL)
-	query = applyMultiRoleFilters(query, options.Roles, options.TeamRoles, options.ChannelRoles)
+	query = applyMultiRoleFilters(query, options.Roles, options.TeamRoles, options.ChannelRoles, isPostgreSQL)

 	if options.Inactive {
 		query = query.Where("u.DeleteAt != 0")
@@ -1228,7 +1185,7 @@ func (us SqlUserStore) Count(options model.UserCountOptions) (int64, error) {
 		query = query.LeftJoin("ChannelMembers AS cm ON u.Id = cm.UserId").Where("cm.ChannelId = ?", options.ChannelId)
 	}
 	query = applyViewRestrictionsFilter(query, options.ViewRestrictions, false)
-	query = applyMultiRoleFilters(query, options.Roles, options.TeamRoles, options.ChannelRoles)
+	query = applyMultiRoleFilters(query, options.Roles, options.TeamRoles, options.ChannelRoles, isPostgreSQL)

 	if isPostgreSQL {
 		query = query.PlaceholderFormat(sq.Dollar)
@@ -1461,7 +1418,7 @@ func (us SqlUserStore) performSearch(query sq.SelectBuilder, term string, option
 	isPostgreSQL := us.DriverName() == model.DATABASE_DRIVER_POSTGRES

 	query = applyRoleFilter(query, options.Role, isPostgreSQL)
-	query = applyMultiRoleFilters(query, options.Roles, options.TeamRoles, options.ChannelRoles)
+	query = applyMultiRoleFilters(query, options.Roles, options.TeamRoles, options.ChannelRoles, isPostgreSQL)

 	if !options.AllowInactive {
 		query = query.Where("u.DeleteAt = 0")
--- a/store/storetest/user_store.go
+++ b/store/storetest/user_store.go
@@ -368,6 +368,7 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 	u1, err := ss.User().Save(&model.User{
 		Email:    MakeEmail(),
 		Username: "u1" + model.NewId(),
+		Roles:    model.SYSTEM_USER_ROLE_ID,
 	})
 	require.Nil(t, err)
 	defer func() { require.Nil(t, ss.User().PermanentDelete(u1.Id)) }()
@@ -375,6 +376,7 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 	u2, err := ss.User().Save(&model.User{
 		Email:    MakeEmail(),
 		Username: "u2" + model.NewId(),
+		Roles:    model.SYSTEM_USER_ROLE_ID,
 	})
 	require.Nil(t, err)
 	defer func() { require.Nil(t, ss.User().PermanentDelete(u2.Id)) }()
@@ -423,14 +425,15 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 		Email:    MakeEmail(),
 		Username: "u7" + model.NewId(),
 		DeleteAt: model.GetMillis(),
+		Roles:    model.SYSTEM_USER_ROLE_ID,
 	})
 	require.Nil(t, err)
 	defer func() { require.Nil(t, ss.User().PermanentDelete(u7.Id)) }()

 	t.Run("get offset 0, limit 100", func(t *testing.T) {
 		options := &model.UserGetOptions{Page: 0, PerPage: 100}
-		actual, err := ss.User().GetAllProfiles(options)
-		require.Nil(t, err)
+		actual, userErr := ss.User().GetAllProfiles(options)
+		require.Nil(t, userErr)

 		require.Equal(t, []*model.User{
 			sanitized(u1),
@@ -444,19 +447,19 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 	})

 	t.Run("get offset 0, limit 1", func(t *testing.T) {
-		actual, err := ss.User().GetAllProfiles(&model.UserGetOptions{
+		actual, userErr := ss.User().GetAllProfiles(&model.UserGetOptions{
 			Page:    0,
 			PerPage: 1,
 		})
-		require.Nil(t, err)
+		require.Nil(t, userErr)
 		require.Equal(t, []*model.User{
 			sanitized(u1),
 		}, actual)
 	})

 	t.Run("get all", func(t *testing.T) {
-		actual, err := ss.User().GetAll()
-		require.Nil(t, err)
+		actual, userErr := ss.User().GetAll()
+		require.Nil(t, userErr)

 		require.Equal(t, []*model.User{
 			u1,
@@ -474,8 +477,8 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {

 		uNew := &model.User{}
 		uNew.Email = MakeEmail()
-		_, err := ss.User().Save(uNew)
-		require.Nil(t, err)
+		_, userErr := ss.User().Save(uNew)
+		require.Nil(t, userErr)
 		defer func() { require.Nil(t, ss.User().PermanentDelete(uNew.Id)) }()

 		updatedEtag := ss.User().GetEtagForAllProfiles()
@@ -483,12 +486,12 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 	})

 	t.Run("filter to system_admin role", func(t *testing.T) {
-		actual, err := ss.User().GetAllProfiles(&model.UserGetOptions{
+		actual, userErr := ss.User().GetAllProfiles(&model.UserGetOptions{
 			Page:    0,
 			PerPage: 10,
 			Role:    "system_admin",
 		})
-		require.Nil(t, err)
+		require.Nil(t, userErr)
 		require.Equal(t, []*model.User{
 			sanitized(u5),
 			sanitized(u6),
@@ -496,25 +499,25 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 	})

 	t.Run("filter to system_admin role, inactive", func(t *testing.T) {
-		actual, err := ss.User().GetAllProfiles(&model.UserGetOptions{
+		actual, userErr := ss.User().GetAllProfiles(&model.UserGetOptions{
 			Page:     0,
 			PerPage:  10,
 			Role:     "system_admin",
 			Inactive: true,
 		})
-		require.Nil(t, err)
+		require.Nil(t, userErr)
 		require.Equal(t, []*model.User{
 			sanitized(u6),
 		}, actual)
 	})

 	t.Run("filter to inactive", func(t *testing.T) {
-		actual, err := ss.User().GetAllProfiles(&model.UserGetOptions{
+		actual, userErr := ss.User().GetAllProfiles(&model.UserGetOptions{
 			Page:     0,
 			PerPage:  10,
 			Inactive: true,
 		})
-		require.Nil(t, err)
+		require.Nil(t, userErr)
 		require.Equal(t, []*model.User{
 			sanitized(u6),
 			sanitized(u7),
@@ -522,12 +525,12 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 	})

 	t.Run("filter to active", func(t *testing.T) {
-		actual, err := ss.User().GetAllProfiles(&model.UserGetOptions{
+		actual, userErr := ss.User().GetAllProfiles(&model.UserGetOptions{
 			Page:    0,
 			PerPage: 10,
 			Active:  true,
 		})
-		require.Nil(t, err)
+		require.Nil(t, userErr)
 		require.Equal(t, []*model.User{
 			sanitized(u1),
 			sanitized(u2),
@@ -538,18 +541,87 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 	})

 	t.Run("try to filter to active and inactive", func(t *testing.T) {
-		actual, err := ss.User().GetAllProfiles(&model.UserGetOptions{
+		actual, userErr := ss.User().GetAllProfiles(&model.UserGetOptions{
 			Page:     0,
 			PerPage:  10,
 			Inactive: true,
 			Active:   true,
 		})
-		require.Nil(t, err)
+		require.Nil(t, userErr)
 		require.Equal(t, []*model.User{
 			sanitized(u6),
 			sanitized(u7),
 		}, actual)
 	})
+
+	u8, err := ss.User().Save(&model.User{
+		Email:    MakeEmail(),
+		Username: "u8" + model.NewId(),
+		DeleteAt: model.GetMillis(),
+		Roles:    "system_user_manager system_user",
+	})
+	require.Nil(t, err)
+	defer func() { require.Nil(t, ss.User().PermanentDelete(u8.Id)) }()
+
+	u9, err := ss.User().Save(&model.User{
+		Email:    MakeEmail(),
+		Username: "u9" + model.NewId(),
+		DeleteAt: model.GetMillis(),
+		Roles:    "system_manager system_user",
+	})
+	require.Nil(t, err)
+	defer func() { require.Nil(t, ss.User().PermanentDelete(u9.Id)) }()
+
+	u10, err := ss.User().Save(&model.User{
+		Email:    MakeEmail(),
+		Username: "u10" + model.NewId(),
+		DeleteAt: model.GetMillis(),
+		Roles:    "system_read_only_admin system_user",
+	})
+	require.Nil(t, err)
+	defer func() { require.Nil(t, ss.User().PermanentDelete(u10.Id)) }()
+
+	t.Run("filter by system_user_manager role", func(t *testing.T) {
+		actual, userErr := ss.User().GetAllProfiles(&model.UserGetOptions{
+			Page:    0,
+			PerPage: 10,
+			Roles:   []string{"system_user_manager"},
+		})
+		require.Nil(t, userErr)
+		require.Equal(t, []*model.User{
+			sanitized(u8),
+		}, actual)
+	})
+
+	t.Run("filter by multiple system roles", func(t *testing.T) {
+		actual, userErr := ss.User().GetAllProfiles(&model.UserGetOptions{
+			Page:    0,
+			PerPage: 10,
+			Roles:   []string{"system_manager", "system_user_manager", "system_read_only_admin", "system_admin"},
+		})
+		require.Nil(t, userErr)
+		require.Equal(t, []*model.User{
+			sanitized(u10),
+			sanitized(u5),
+			sanitized(u6),
+			sanitized(u8),
+			sanitized(u9),
+		}, actual)
+	})
+
+	t.Run("filter by system_user only", func(t *testing.T) {
+		actual, userErr := ss.User().GetAllProfiles(&model.UserGetOptions{
+			Page:    0,
+			PerPage: 10,
+			Roles:   []string{"system_user"},
+		})
+		require.Nil(t, userErr)
+		require.Equal(t, []*model.User{
+			sanitized(u1),
+			sanitized(u2),
+			sanitized(u7),
+		}, actual)
+	})
 }

 func testUserStoreGetProfiles(t *testing.T, ss store.Store) {
@@ -2437,7 +2509,7 @@ func testUserStoreSearch(t *testing.T, ss store.Store) {
 	u2 := &model.User{
 		Username: "jim2-bobby" + model.NewId(),
 		Email:    MakeEmail(),
-		Roles:    "system_user",
+		Roles:    "system_user system_user_manager",
 	}
 	_, err = ss.User().Save(u2)
 	require.Nil(t, err)
--- a/testlib/store.go
+++ b/testlib/store.go
@@ -46,6 +46,7 @@ func GetMockStoreForSetupFunctions() *mocks.Store {
 	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_USE_GROUP_MENTIONS_PERMISSION).Return(&model.System{Name: model.MIGRATION_KEY_ADD_USE_GROUP_MENTIONS_PERMISSION, Value: "true"}, nil)
 	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_SYSTEM_CONSOLE_PERMISSIONS).Return(&model.System{Name: model.MIGRATION_KEY_ADD_SYSTEM_CONSOLE_PERMISSIONS, Value: "true"}, nil)
 	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_CONVERT_CHANNEL_PERMISSIONS).Return(&model.System{Name: model.MIGRATION_KEY_ADD_CONVERT_CHANNEL_PERMISSIONS, Value: "true"}, nil)
+	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_SYSTEM_ROLES_PERMISSIONS).Return(&model.System{Name: model.MIGRATION_KEY_ADD_SYSTEM_ROLES_PERMISSIONS, Value: "true"}, nil)
 	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_MANAGE_SHARED_CHANNEL_PERMISSIONS).Return(&model.System{Name: model.MIGRATION_KEY_ADD_MANAGE_SHARED_CHANNEL_PERMISSIONS, Value: "true"}, nil)
 	systemStore.On("Get").Return(make(model.StringMap), nil)
 	systemStore.On("Save", mock.AnythingOfType("*model.System")).Return(nil)