Tighten validation on channel member role updates (#37075) (#37248)

Automatic Merge

server/channels/api4/channel.go

@@ -2124,7 +2124,7 @@ func updateChannelMemberRoles(c *Context, w http.ResponseWriter, r *http.Request
 	props := model.MapFromJSON(r.Body)
 
 	newRoles := props["roles"]
-	if !(model.IsValidUserRoles(newRoles)) {
+	if !model.IsValidChannelMemberRoles(newRoles) {
 		c.SetInvalidParam("roles")
 		return
 	}

server/channels/api4/channel_test.go

@@ -5001,6 +5001,84 @@ func TestUpdateChannelRoles(t *testing.T) {
 	CheckForbiddenStatus(t, resp)
 }
 
+func TestUpdateChannelMemberRolesRejectsNonChannelScopedRoles(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+	client := th.Client
+
+	const channelAdmin = "channel_user channel_admin"
+	const channelMember = "channel_user"
+
+	channel := th.CreatePublicChannel(t)
+
+	_, appErr := th.App.AddUserToChannel(th.Context, th.BasicUser2, channel, false)
+	require.Nil(t, appErr)
+
+	invalidRoles := []struct {
+		name  string
+		roles string
+	}{
+		{name: "system manager with channel user", roles: channelMember + " " + model.SystemManagerRoleId},
+		{name: "system user manager with channel user", roles: channelMember + " " + model.SystemUserManagerRoleId},
+		{name: "system admin with channel admin", roles: channelAdmin + " " + model.SystemAdminRoleId},
+		{name: "team user with channel user", roles: channelMember + " " + model.TeamUserRoleId},
+		{name: "team admin with channel user", roles: channelMember + " " + model.TeamAdminRoleId},
+		{name: "team post all with channel user", roles: channelMember + " " + model.TeamPostAllRoleId},
+		{name: "system post all with channel user", roles: channelMember + " " + model.SystemPostAllRoleId},
+		{name: "system read only admin with channel user", roles: channelMember + " " + model.SystemReadOnlyAdminRoleId},
+		{name: "custom group user with channel user", roles: channelMember + " " + model.CustomGroupUserRoleId},
+	}
+
+	for _, tc := range invalidRoles {
+		t.Run("rejects "+tc.name, func(t *testing.T) {
+			memberBefore, _, err := client.GetChannelMember(context.Background(), channel.Id, th.BasicUser2.Id, "")
+			require.NoError(t, err)
+			rolesBefore := memberBefore.Roles
+
+			resp, err := client.UpdateChannelRoles(context.Background(), channel.Id, th.BasicUser2.Id, tc.roles)
+			require.Error(t, err)
+			CheckBadRequestStatus(t, resp)
+
+			memberAfter, _, err := client.GetChannelMember(context.Background(), channel.Id, th.BasicUser2.Id, "")
+			require.NoError(t, err)
+			require.Equal(t, rolesBefore, memberAfter.Roles)
+		})
+	}
+
+	validRoles := []struct {
+		name  string
+		roles string
+	}{
+		{name: "channel member", roles: channelMember},
+		{name: "channel admin", roles: channelAdmin},
+	}
+
+	for _, tc := range validRoles {
+		t.Run("accepts "+tc.name, func(t *testing.T) {
+			_, err := client.UpdateChannelRoles(context.Background(), channel.Id, th.BasicUser2.Id, tc.roles)
+			require.NoError(t, err)
+
+			member, _, err := client.GetChannelMember(context.Background(), channel.Id, th.BasicUser2.Id, "")
+			require.NoError(t, err)
+			require.Equal(t, tc.roles, member.Roles)
+		})
+	}
+
+	t.Run("rejects system manager assigned by system admin", func(t *testing.T) {
+		memberBefore, _, err := th.SystemAdminClient.GetChannelMember(context.Background(), channel.Id, th.BasicUser2.Id, "")
+		require.NoError(t, err)
+		rolesBefore := memberBefore.Roles
+
+		resp, err := th.SystemAdminClient.UpdateChannelRoles(context.Background(), channel.Id, th.BasicUser2.Id, channelMember+" "+model.SystemManagerRoleId)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+
+		memberAfter, _, err := th.SystemAdminClient.GetChannelMember(context.Background(), channel.Id, th.BasicUser2.Id, "")
+		require.NoError(t, err)
+		require.Equal(t, rolesBefore, memberAfter.Roles)
+	})
+}
+
 func TestUpdateChannelMemberSchemeRoles(t *testing.T) {
 	mainHelper.Parallel(t)
 	th := Setup(t).InitBasic(t)

server/channels/app/channel.go

@@ -1380,7 +1380,10 @@ func (a *App) updateChannelMemberRolesInternal(rctx request.CTX, channelID strin
 		}
 
 		if !role.SchemeManaged {
-			// The role is not scheme-managed, so it's OK to apply it to the explicit roles field.
+			if model.IsBuiltInRole(roleName) && !model.IsChannelScopedBuiltInRole(roleName) {
+				err = model.NewAppError("UpdateChannelMemberRoles", "api.channel.update_channel_member_roles.scheme_role.app_error", nil, "role_name="+roleName, http.StatusBadRequest)
+				return nil, err
+			}
 			newExplicitRoles = append(newExplicitRoles, roleName)
 		} else {
 			// The role is scheme-managed, so need to check if it is part of the scheme for this channel or not.

server/channels/app/channel_test.go

@@ -1677,6 +1677,30 @@ func TestUpdateChannelMemberRolesChangingGuest(t *testing.T) {
 	})
 }
 
+func TestUpdateChannelMemberRolesRejectsOutOfScopeBuiltInRoles(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	user := model.User{Email: strings.ToLower(model.NewId()) + "success+test@example.com", Nickname: "Tester", Username: "tester" + model.NewId(), Password: model.NewTestPassword(), AuthService: ""}
+	ruser, _ := th.App.CreateUser(th.Context, &user)
+
+	_, _, appErr := th.App.AddUserToTeam(th.Context, th.BasicTeam.Id, ruser.Id, "")
+	require.Nil(t, appErr)
+
+	_, appErr = th.App.AddUserToChannel(th.Context, ruser, th.BasicChannel, false)
+	require.Nil(t, appErr)
+
+	// CustomGroupUserRoleId is a built-in role whose role.BuiltIn flag is false, so it must
+	// be rejected via the shared model.IsBuiltInRole predicate rather than the BuiltIn flag.
+	// This guards the app-layer path used by plugins and bulk import, which bypass the API check.
+	for _, roleName := range []string{model.SystemManagerRoleId, model.SystemCustomGroupAdminRoleId, model.CustomGroupUserRoleId} {
+		_, appErr = th.App.UpdateChannelMemberRoles(th.Context, th.BasicChannel.Id, ruser.Id, model.ChannelUserRoleId+" "+roleName)
+		require.NotNilf(t, appErr, "expected rejection for role %s", roleName)
+		require.Equal(t, "api.channel.update_channel_member_roles.scheme_role.app_error", appErr.Id)
+		require.Equal(t, http.StatusBadRequest, appErr.StatusCode)
+	}
+}
+
 func TestUpdateChannelMemberRolesRequireUser(t *testing.T) {
 	mainHelper.Parallel(t)
 	th := Setup(t).InitBasic(t)

server/public/model/role.go

@@ -49,6 +49,7 @@ func init() {
 		ChannelAdminRoleId,
 
 		CustomGroupUserRoleId,
+		SystemCustomGroupAdminRoleId,
 
 		PlaybookAdminRoleId,
 		PlaybookMemberRoleId,
@@ -56,6 +57,11 @@ func init() {
 		RunMemberRoleId,
 	}, NewSystemRoleIDs...)
 
+	builtInRoleSet = make(map[string]bool, len(BuiltInSchemeManagedRoleIDs))
+	for _, id := range BuiltInSchemeManagedRoleIDs {
+		builtInRoleSet[id] = true
+	}
+
 	// When updating the values here, the values in mattermost-redux must also be updated.
 	SysconsoleAncillaryPermissions = map[string][]*Permission{
 		PermissionSysconsoleReadAboutEditionAndLicense.Id: {
@@ -872,6 +878,44 @@ func IsValidRoleName(roleName string) bool {
 	return true
 }
 
+// builtInRoleSet is the O(1) lookup set for BuiltInSchemeManagedRoleIDs, built in init().
+// Despite its name, BuiltInSchemeManagedRoleIDs is the canonical list of built-in role
+// IDs and not all of its entries are scheme-managed (roughly half have SchemeManaged: false,
+// e.g. custom_group_user). It is used as the single source of truth for "is this a built-in
+// role", independent of the per-role BuiltIn/SchemeManaged flags.
+var builtInRoleSet map[string]bool
+
+// IsBuiltInRole reports whether roleName is a built-in role, using
+// BuiltInSchemeManagedRoleIDs as the source of truth. This is the predicate shared by
+// IsValidChannelMemberRoles and the app-layer channel member role validation so both
+// layers agree on which roles are built-in.
+func IsBuiltInRole(roleName string) bool {
+	return builtInRoleSet[roleName]
+}
+
+// IsChannelScopedBuiltInRole returns true for the three built-in roles that are
+// valid inside a channel-member role list.
+func IsChannelScopedBuiltInRole(roleName string) bool {
+	return roleName == ChannelGuestRoleId || roleName == ChannelUserRoleId || roleName == ChannelAdminRoleId
+}
+
+// IsValidChannelMemberRoles reports whether roles are valid for a channel member.
+// IsValidUserRoles is format validation only; this additionally rejects any built-in
+// role (per IsBuiltInRole) that is not channel-scoped.
+func IsValidChannelMemberRoles(channelMemberRoles string) bool {
+	if !IsValidUserRoles(channelMemberRoles) {
+		return false
+	}
+
+	for roleName := range strings.FieldsSeq(channelMemberRoles) {
+		if IsBuiltInRole(roleName) && !IsChannelScopedBuiltInRole(roleName) {
+			return false
+		}
+	}
+
+	return true
+}
+
 func MakeDefaultRoles() map[string]*Role {
 	roles := make(map[string]*Role)
 

server/public/model/role_test.go

@@ -536,6 +536,59 @@ func TestRoleIsValid(t *testing.T) {
 	})
 }
 
+func TestIsValidChannelMemberRoles(t *testing.T) {
+	tests := []struct {
+		name  string
+		roles string
+		valid bool
+	}{
+		{name: "channel user only", roles: ChannelUserRoleId, valid: true},
+		{name: "channel user and admin", roles: ChannelUserRoleId + " " + ChannelAdminRoleId, valid: true},
+		{name: "channel guest only", roles: ChannelGuestRoleId, valid: true},
+		{name: "custom role with channel user", roles: ChannelUserRoleId + " custom_role", valid: true},
+		{name: "prefixed custom team role with channel user", roles: ChannelUserRoleId + " team_custom", valid: true},
+		{name: "prefixed custom system role with channel user", roles: ChannelUserRoleId + " system_custom", valid: true},
+		{name: "team user with channel user", roles: ChannelUserRoleId + " " + TeamUserRoleId, valid: false},
+		{name: "team post all with channel user", roles: ChannelUserRoleId + " " + TeamPostAllRoleId, valid: false},
+		{name: "system user with channel user", roles: ChannelUserRoleId + " " + SystemUserRoleId, valid: false},
+		{name: "system manager with channel user", roles: ChannelUserRoleId + " " + SystemManagerRoleId, valid: false},
+		{name: "system post all with channel user", roles: ChannelUserRoleId + " " + SystemPostAllRoleId, valid: false},
+		{name: "system read only admin with channel user", roles: ChannelUserRoleId + " " + SystemReadOnlyAdminRoleId, valid: false},
+		{name: "custom group user with channel user", roles: ChannelUserRoleId + " " + CustomGroupUserRoleId, valid: false},
+		{name: "system custom group admin with channel user", roles: ChannelUserRoleId + " " + SystemCustomGroupAdminRoleId, valid: false},
+		{name: "invalid role name", roles: "invalid-role", valid: false},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.valid, IsValidChannelMemberRoles(tc.roles))
+		})
+	}
+}
+
+func TestIsBuiltInRole(t *testing.T) {
+	tests := []struct {
+		name     string
+		roleName string
+		builtIn  bool
+	}{
+		{name: "channel user", roleName: ChannelUserRoleId, builtIn: true},
+		{name: "system manager", roleName: SystemManagerRoleId, builtIn: true},
+		{name: "system custom group admin", roleName: SystemCustomGroupAdminRoleId, builtIn: true},
+		// custom_group_user is built-in even though its role.BuiltIn flag is false.
+		{name: "custom group user", roleName: CustomGroupUserRoleId, builtIn: true},
+		{name: "custom role", roleName: "custom_role", builtIn: false},
+		{name: "prefixed custom system role", roleName: "system_custom", builtIn: false},
+		{name: "empty", roleName: "", builtIn: false},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.builtIn, IsBuiltInRole(tc.roleName))
+		})
+	}
+}
+
 func TestManageAgentPermissionsDefaultRoles(t *testing.T) {
 	roles := MakeDefaultRoles()