From f275a339676ec8d19c914bba24fc649a1c858ee8 Mon Sep 17 00:00:00 2001 From: Nick Misasi Date: Wed, 29 Apr 2026 10:17:51 -0400 Subject: [PATCH 1/4] MM-67913: fix white flash on product navigation by centralizing app__body ownership (#36186) * MM-67913: fix white flash on product navigation by centralizing app__body ownership Previously, `document.body.classList.add/remove('app__body')` was managed independently by `ChannelController` and each product plugin (Playbooks, Boards). When switching products, the plugin's cleanup removed `app__body` before the incoming tree's effect re-applied it. Because `ChannelController` is deeply nested and sometimes async, a ~170ms gap could elapse during which the body fell back to `background: $bg--gray`, producing a visible white flash behind the transparent `GlobalHeader` and LHS. Centralize `app__body` ownership in `WithUserTheme` via a new `useAppBodyClass` hook. Since `WithUserTheme` wraps both the products Switch and the `/:team` route in `root.tsx`, it stays mounted across product navigation, so the class is never removed mid-transition. `ChannelController` no longer toggles `app__body`; tests updated. Product plugins (Playbooks, Boards) should stop touching `app__body` as well; companion fixes land in their respective repos. Made-with: Cursor * MM-67913: add coverage for app body class ownership Made-with: Cursor * Update webapp/channels/src/components/theme_provider/theme_context.ts Co-authored-by: Harrison Healey * MM-67913: fix theme context hook closure Made-with: Cursor --------- Co-authored-by: Harrison Healey --- .../channel_controller.test.tsx | 12 ++--- .../channel_layout/channel_controller.tsx | 2 +- .../theme_provider/theme_context.test.tsx | 44 +++++++++++++++++++ .../theme_provider/theme_context.ts | 16 +++++++ 4 files changed, 67 insertions(+), 7 deletions(-) create mode 100644 webapp/channels/src/components/theme_provider/theme_context.test.tsx diff --git a/webapp/channels/src/components/channel_layout/channel_controller.test.tsx b/webapp/channels/src/components/channel_layout/channel_controller.test.tsx index 81b99406828..ad84e9f9079 100644 --- a/webapp/channels/src/components/channel_layout/channel_controller.test.tsx +++ b/webapp/channels/src/components/channel_layout/channel_controller.test.tsx @@ -91,17 +91,17 @@ describe('ChannelController', () => { }); describe('components/channel_layout/ChannelController', () => { - test('Should have app__body and channel-view classes by default', () => { - expect(getClassnamesForBody('')).toEqual(['app__body', 'channel-view']); + test('Should have channel-view class by default', () => { + expect(getClassnamesForBody('')).toEqual(['channel-view']); }); test('Should have os--windows class on body for windows 32 or windows 64', () => { - expect(getClassnamesForBody('Win32')).toEqual(['app__body', 'channel-view', 'os--windows']); - expect(getClassnamesForBody('Win64')).toEqual(['app__body', 'channel-view', 'os--windows']); + expect(getClassnamesForBody('Win32')).toEqual(['channel-view', 'os--windows']); + expect(getClassnamesForBody('Win64')).toEqual(['channel-view', 'os--windows']); }); test('Should have os--mac class on body for MacIntel or MacPPC', () => { - expect(getClassnamesForBody('MacIntel')).toEqual(['app__body', 'channel-view', 'os--mac']); - expect(getClassnamesForBody('MacPPC')).toEqual(['app__body', 'channel-view', 'os--mac']); + expect(getClassnamesForBody('MacIntel')).toEqual(['channel-view', 'os--mac']); + expect(getClassnamesForBody('MacPPC')).toEqual(['channel-view', 'os--mac']); }); }); diff --git a/webapp/channels/src/components/channel_layout/channel_controller.tsx b/webapp/channels/src/components/channel_layout/channel_controller.tsx index a430dc7a633..71da75b87b2 100644 --- a/webapp/channels/src/components/channel_layout/channel_controller.tsx +++ b/webapp/channels/src/components/channel_layout/channel_controller.tsx @@ -26,7 +26,7 @@ const ProductNoticesModal = makeAsyncComponent('ProductNoticesModal', lazy(() => const ResetStatusModal = makeAsyncComponent('ResetStatusModal', lazy(() => import('components/reset_status_modal'))); const MobileSidebarRight = makeAsyncComponent('MobileSidebarRight', lazy(() => import('components/mobile_sidebar_right'))); -const BODY_CLASS_FOR_CHANNEL = ['app__body', 'channel-view']; +const BODY_CLASS_FOR_CHANNEL = ['channel-view']; type Props = { shouldRenderCenterChannel: boolean; diff --git a/webapp/channels/src/components/theme_provider/theme_context.test.tsx b/webapp/channels/src/components/theme_provider/theme_context.test.tsx new file mode 100644 index 00000000000..f0a077410d2 --- /dev/null +++ b/webapp/channels/src/components/theme_provider/theme_context.test.tsx @@ -0,0 +1,44 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; + +import {render, renderHook} from 'tests/react_testing_utils'; + +import {useAppBodyClass, WithUserTheme} from './theme_context'; + +describe('useAppBodyClass', () => { + afterEach(() => { + document.body.classList.remove('app__body'); + }); + + it('adds app__body to the document body while mounted', () => { + const {unmount} = renderHook(() => useAppBodyClass()); + + expect(document.body.classList.contains('app__body')).toBe(true); + + unmount(); + + expect(document.body.classList.contains('app__body')).toBe(false); + }); +}); + +describe('WithUserTheme', () => { + afterEach(() => { + document.body.classList.remove('app__body'); + }); + + it('adds app__body to the document body while mounted', () => { + const {unmount} = render( + +
{'Themed content'}
+ , + ); + + expect(document.body.classList.contains('app__body')).toBe(true); + + unmount(); + + expect(document.body.classList.contains('app__body')).toBe(false); + }); +}); diff --git a/webapp/channels/src/components/theme_provider/theme_context.ts b/webapp/channels/src/components/theme_provider/theme_context.ts index 676cfc8f0e1..9328319ae51 100644 --- a/webapp/channels/src/components/theme_provider/theme_context.ts +++ b/webapp/channels/src/components/theme_provider/theme_context.ts @@ -24,6 +24,21 @@ export function useUserTheme() { }, [context]); } +/** + * useAppBodyClass manages the `app__body` class on the document body for as long as the calling component + * remains mounted. That class is required for much of our CSS to apply the user's theme, so it should be used + * wherever those should be used. + */ +export function useAppBodyClass() { + useEffect(() => { + document.body.classList.add('app__body'); + + return () => { + document.body.classList.remove('app__body'); + }; + }, []); +} + /** * WithUserTheme makes it so that the app will apply the user's theme instead of the default one for as long as it * remains mounted. It's used to wrap multiple routes in the Root component instead of having each of them call @@ -31,6 +46,7 @@ export function useUserTheme() { */ export function WithUserTheme({children}: {children: React.ReactNode}) { useUserTheme(); + useAppBodyClass(); return children; } From 641d5a4eb7faa8037f9050b926800a875e8927e0 Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Wed, 29 Apr 2026 16:22:12 +0200 Subject: [PATCH 2/4] [MM-68538] Wrap incoming query from the CEL -> SQL conversion with parentheses (#36293) --- .../channel_admin_self_inclusion.spec.ts | 222 ++++++++++++++++++ .../store/sqlstore/attributes_store.go | 11 +- .../store/storetest/attributes_store.go | 62 +++++ 3 files changed, 293 insertions(+), 2 deletions(-) create mode 100644 e2e-tests/playwright/specs/functional/channels/channel_settings_access_rules/channel_admin_self_inclusion.spec.ts diff --git a/e2e-tests/playwright/specs/functional/channels/channel_settings_access_rules/channel_admin_self_inclusion.spec.ts b/e2e-tests/playwright/specs/functional/channels/channel_settings_access_rules/channel_admin_self_inclusion.spec.ts new file mode 100644 index 00000000000..d9dbcdf6546 --- /dev/null +++ b/e2e-tests/playwright/specs/functional/channels/channel_settings_access_rules/channel_admin_self_inclusion.spec.ts @@ -0,0 +1,222 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +/** + * @objective Channel Settings → Access Control regression test for the + * "Test access rule" results for top-level OR rules. + * @reference MM-68538 + */ + +import {Client4} from '@mattermost/client'; +import type {Page} from '@playwright/test'; +import type {UserPropertyField} from '@mattermost/types/properties'; +import type {UserProfile} from '@mattermost/types/users'; + +import {ChannelsPage, expect, newTestPassword, test} from '@mattermost/playwright-lib'; + +const PROGRAM_OPTIONS = [ + {name: 'Artemis', color: '#FF8800'}, + {name: 'Helios', color: '#00AAFF'}, +]; + +// CEL the table editor emits for ` has any of [Artemis, Helios]`. +// See rowToCEL() in webapp/.../table_editor/table_editor.tsx. +function buildTwoValueOrExpression(attributeName: string): string { + return `("Artemis" in user.attributes.${attributeName} || "Helios" in user.attributes.${attributeName})`; +} + +function buildAliceExcludingExpression(attributeName: string): string { + return `"Helios" in user.attributes.${attributeName}`; +} + +type ProgramField = { + id: string; + optionIdByName: Record; +}; + +// Create a multiselect CPA field directly via the REST API. +async function ensureProgramMultiselectField(adminClient: Client4, fieldName: string): Promise { + const baseRoute = `${adminClient.getBaseRoute()}/custom_profile_attributes/fields`; + + const created: UserPropertyField = await (adminClient as any).doFetch(baseRoute, { + method: 'POST', + body: JSON.stringify({ + name: fieldName, + type: 'multiselect', + attrs: { + managed: 'admin', + visibility: 'when_set', + options: PROGRAM_OPTIONS, + }, + }), + }); + + const options: Array<{id: string; name: string}> = + ((created as any)?.attrs?.options as Array<{id: string; name: string}>) ?? []; + const optionIdByName: Record = {}; + for (const opt of options) { + optionIdByName[opt.name] = opt.id; + } + for (const expected of PROGRAM_OPTIONS) { + if (!optionIdByName[expected.name]) { + throw new Error( + `Multiselect field "${fieldName}" was created without an id for option "${expected.name}"; ` + + `got: ${JSON.stringify(options)}`, + ); + } + } + + return {id: created.id, optionIdByName}; +} + +async function createUserWithProgram( + adminClient: Client4, + programField: ProgramField, + program: string[], + teamId: string, + prefix: string, +): Promise { + const password = newTestPassword(); + const id = Math.random().toString(36).substring(2, 9); + const user = await adminClient.createUser( + { + email: `${prefix}-${id}@sample.mattermost.com`, + username: `${prefix}${id}`, + password, + } as UserProfile, + '', + '', + ); + user.password = password; + + // Multiselect values must be sent as option IDs not names. + const optionIds = program.map((name) => { + const optionId = programField.optionIdByName[name]; + if (!optionId) { + throw new Error(`Program option "${name}" not found in field options`); + } + return optionId; + }); + await adminClient.updateUserCustomProfileAttributesValues(user.id, {[programField.id]: optionIds}); + + // Suppress tutorials/onboarding so UI navigation is stable. + await adminClient.savePreferences(user.id, [ + {user_id: user.id, category: 'tutorial_step', name: user.id, value: '999'}, + {user_id: user.id, category: 'onboarding_task_list', name: 'onboarding_task_list_show', value: 'false'}, + {user_id: user.id, category: 'onboarding_task_list', name: 'onboarding_task_list_open', value: 'false'}, + ]); + + await adminClient.addToTeam(teamId, user.id); + return user; +} + +async function createChannelAccessRule( + adminClient: Client4, + channel: {id: string; display_name: string}, + expression: string, +) { + await (adminClient as any).doFetch(`${adminClient.getBaseRoute()}/access_control_policies`, { + method: 'PUT', + body: JSON.stringify({ + id: channel.id, + name: channel.display_name, + type: 'channel', + active: false, + revision: 1, + created_at: Date.now(), + rules: [{actions: ['membership'], expression}], + imports: [], + }), + }); +} + +async function openAccessControlSettings(channelsPage: ChannelsPage) { + const channelSettings = await channelsPage.openChannelSettings(); + const accessControlTab = channelSettings.container.getByRole('tab', {name: /Access Control/i}); + await expect(accessControlTab).toBeVisible({timeout: 10000}); + await accessControlTab.click(); + + return channelSettings; +} + +async function verifyTestAccessRuleDisabled(page: Page) { + await expect(page.getByRole('button', {name: /Test access rule/i})).toBeDisabled({timeout: 10000}); +} + +async function testAccessRuleAndVerifyUser(page: Page, username: string) { + const testButton = page.getByRole('button', {name: /Test access rule/i}); + await expect(testButton).toBeEnabled({timeout: 10000}); + await testButton.click(); + + const modal = page.locator('.TestResultsModal').filter({hasText: 'Access Rule Test Results'}); + await expect(modal).toBeVisible({timeout: 10000}); + + await modal.getByRole('textbox', {name: /Search users/i}).fill(username); + await expect(modal.getByText(`@${username}`)).toBeVisible({timeout: 10000}); +} + +test.describe('Channel Settings → Access Control', () => { + test('MM-68538 channel admin can test access rule for multiselect "has any of" with multiple values', async ({ + pw, + }) => { + await pw.skipIfNoLicense(); + + const {adminClient, team} = await pw.initSetup(); + + // Enable ABAC + user-managed attributes so the Access Control tab and + // the test access rule flow are available. + await adminClient.patchConfig({ + AccessControlSettings: { + EnableAttributeBasedAccessControl: true, + EnableUserManagedAttributes: true, + }, + }); + + // CPA name must be a valid CEL identifier segment; keep alphanumeric + underscore. + const programFieldName = `Program_mm68538_${Math.random().toString(36).substring(2, 14)}`; + const programField = await ensureProgramMultiselectField(adminClient, programFieldName); + + const aliceExcludingExpression = buildAliceExcludingExpression(programFieldName); + const twoValueOrExpression = buildTwoValueOrExpression(programFieldName); + + // Several users matching the OR. Without the fix, SearchUsers ordered + // by Users.Id ASC would return whichever of the matching users has the + // lowest Id, so populating multiple matches makes the assertion robust + // regardless of which Id alice ends up with. + const alice = await createUserWithProgram(adminClient, programField, ['Artemis'], team.id, 'alice'); + await createUserWithProgram(adminClient, programField, ['Helios'], team.id, 'bob'); + await createUserWithProgram(adminClient, programField, ['Artemis'], team.id, 'carol'); + + // Private channel where alice is the channel admin and the only one + // with the manage_channel_access_rules permission. Bob/carol exist + // purely to populate the OR-matching candidate set. + const channel = await adminClient.createChannel({ + team_id: team.id, + name: `mm68538-${Math.random().toString(36).substring(2, 8)}`, + display_name: `MM-68538 ${Math.random().toString(36).substring(2, 6)}`, + type: 'P', + } as any); + await adminClient.addToChannel(alice.id, channel.id); + await adminClient.updateChannelMemberSchemeRoles(channel.id, alice.id, true, true); + + await createChannelAccessRule(adminClient, channel, aliceExcludingExpression); + + // Alice can open Channel Settings → Access Control as channel admin + // and test the existing rule through the same UI users exercise. + const {page} = await pw.testBrowser.login(alice); + const channelsPage = new ChannelsPage(page); + await channelsPage.goto(team.name, channel.name); + await channelsPage.toBeVisible(); + + const excludingRuleSettings = await openAccessControlSettings(channelsPage); + await verifyTestAccessRuleDisabled(page); + await excludingRuleSettings.close(); + + await createChannelAccessRule(adminClient, channel, twoValueOrExpression); + + const includingRuleSettings = await openAccessControlSettings(channelsPage); + await testAccessRuleAndVerifyUser(page, alice.username); + + await includingRuleSettings.close(); + }); +}); diff --git a/server/channels/store/sqlstore/attributes_store.go b/server/channels/store/sqlstore/attributes_store.go index f73007517e6..69140069c28 100644 --- a/server/channels/store/sqlstore/attributes_store.go +++ b/server/channels/store/sqlstore/attributes_store.go @@ -96,8 +96,15 @@ func (s *SqlAttributesStore) SearchUsers(rctx request.CTX, opts model.SubjectSea count := s.getQueryBuilder().Select("COUNT(*)").From("Users").LeftJoin("AttributeView ON Users.Id = AttributeView.TargetID") if opts.Query != "" { - query = query.Where(sq.Expr(opts.Query, opts.Args...)) - count = count.Where(sq.Expr(opts.Query, opts.Args...)) + // Wrap the CEL-derived expression in parentheses so that any top-level + // OR (e.g. produced by "has any of [a, b]") does not bind across the + // AND-joined WHERE clauses appended below (SubjectID, DeleteAt, + // TeamID, ExcludeChannelMembers, Cursor, Term). Without these + // parens, "A OR B AND Users.Id = $X" would be parsed as + // "A OR (B AND Users.Id = $X)" because AND binds tighter than OR. + wrapped := "(" + opts.Query + ")" + query = query.Where(sq.Expr(wrapped, opts.Args...)) + count = count.Where(sq.Expr(wrapped, opts.Args...)) } argCount := len(opts.Args) diff --git a/server/channels/store/storetest/attributes_store.go b/server/channels/store/storetest/attributes_store.go index b82456d23a7..7f24b1981b9 100644 --- a/server/channels/store/storetest/attributes_store.go +++ b/server/channels/store/storetest/attributes_store.go @@ -448,4 +448,66 @@ func testAttributesStoreSearchUsersBySubjectID(t *testing.T, rctx request.CTX, s require.Len(t, subjects, 0, "expected 0 users when query doesn't match SubjectID") require.Equal(t, int64(0), count, "expected count 0") }) + + // Regression test: a top-level OR in the CEL-derived query (e.g. produced + // by "has any of [a, b]") used to bleed past the AND-joined SubjectID + // clause because of SQL operator precedence (AND binds tighter than OR). + // The expression must be wrapped in parentheses before it is added to + // the WHERE clause; otherwise the SubjectID filter is silently bypassed + // for the left-hand side of the OR. See ValidateExpressionAgainstRequester + // callers (channel-admin "Test access rule" self-exclusion check). + t.Run("Search users by SubjectID with top-level OR query does not bypass SubjectID filter", func(t *testing.T) { + // users[0] (u1) has property_a = value_a1 + // users[1] (u2) has property_a = value_a2 + // users[2] (u3) has property_a = value_a1 + // The OR query matches all three; SubjectID must restrict the + // result to a single user. + query := "Attributes ->> '" + testPropertyA + "' = $1::text OR Attributes ->> '" + testPropertyA + "' = $2::text" + + for _, u := range users { + subjects, count, err := ss.Attributes().SearchUsers(rctx, model.SubjectSearchOptions{ + Query: query, + Args: []any{testPropertyValueA1, testPropertyValueA2}, + SubjectID: u.Id, + Limit: 1, + }) + require.NoError(t, err, "couldn't search users by SubjectID with top-level OR query") + require.Len(t, subjects, 1, "expected exactly the requested SubjectID to be returned (subject_id=%s)", u.Id) + require.Equal(t, u.Id, subjects[0].Id, "expected SubjectID filter to be honored despite top-level OR (subject_id=%s)", u.Id) + require.Equal(t, int64(1), count, "expected count 1 when filtering by SubjectID with top-level OR query (subject_id=%s)", u.Id) + } + }) + + // Regression test: same root cause as the SubjectID test above, but for + // the implicit Users.DeleteAt = 0 filter. A top-level OR in the + // CEL-derived query must not bleed past the AND-joined DeleteAt clause + // and surface soft-deleted users. + t.Run("Search users with top-level OR query does not surface soft-deleted users", func(t *testing.T) { + // Soft-delete users[0]; revert at end of subtest. + u := users[0] + originalDeleteAt := u.DeleteAt + u.DeleteAt = model.GetMillis() + _, err := ss.User().Update(rctx, u, true) + require.NoError(t, err, "couldn't soft-delete user") + t.Cleanup(func() { + u.DeleteAt = originalDeleteAt + _, cErr := ss.User().Update(rctx, u, true) + require.NoError(t, cErr, "couldn't restore user") + }) + + // users[0] (soft-deleted) and users[2] both have property_a = value_a1. + // users[1] has property_a = value_a2. The OR query matches all three. + // AllowInactive defaults to false, so users[0] must be excluded. + query := "Attributes ->> '" + testPropertyA + "' = $1::text OR Attributes ->> '" + testPropertyA + "' = $2::text" + subjects, count, err := ss.Attributes().SearchUsers(rctx, model.SubjectSearchOptions{ + Query: query, + Args: []any{testPropertyValueA1, testPropertyValueA2}, + }) + require.NoError(t, err, "couldn't search users with top-level OR query") + require.Len(t, subjects, 2, "expected 2 active users (soft-deleted users[0] excluded)") + require.Equal(t, int64(2), count, "expected count 2 active users") + for _, s := range subjects { + require.NotEqual(t, u.Id, s.Id, "soft-deleted user must not be surfaced by top-level OR query") + } + }) } From c2ec9e967da6314375cd1df525d0ff1304741ce6 Mon Sep 17 00:00:00 2001 From: Nick Misasi Date: Wed, 29 Apr 2026 12:17:23 -0400 Subject: [PATCH 3/4] Add stronger EnableTesting warnings (#36158) * Add stronger EnableTesting warnings Co-authored-by: Nick Misasi * Keep EnableTesting translations in en only Co-authored-by: Nick Misasi * Address EnableTesting review feedback Co-authored-by: Nick Misasi --------- Co-authored-by: Cursor Agent --- api/v4/source/definitions.yaml | 2 + api/v4/source/system.yaml | 12 +++-- server/tests/README.md | 4 +- .../admin_console/admin_definition.tsx | 7 ++- .../admin_definition_enable_testing.test.tsx | 48 +++++++++++++++++++ webapp/channels/src/i18n/en.json | 3 +- 6 files changed, 69 insertions(+), 7 deletions(-) create mode 100644 webapp/channels/src/components/admin_console/admin_definition_enable_testing.test.tsx diff --git a/api/v4/source/definitions.yaml b/api/v4/source/definitions.yaml index 9f4bbabd240..09e3bac9c84 100644 --- a/api/v4/source/definitions.yaml +++ b/api/v4/source/definitions.yaml @@ -1567,6 +1567,7 @@ components: type: boolean EnableTesting: type: boolean + description: Intended only for isolated non-production environments and must never be enabled in production. EnableDeveloper: type: boolean EnableSecurityFixAlert: @@ -2049,6 +2050,7 @@ components: type: boolean EnableTesting: type: boolean + description: Intended only for isolated non-production environments and must never be enabled in production. EnableDeveloper: type: boolean EnableSecurityFixAlert: diff --git a/api/v4/source/system.yaml b/api/v4/source/system.yaml index 993fdde5005..6ca163fb9c7 100644 --- a/api/v4/source/system.yaml +++ b/api/v4/source/system.yaml @@ -195,7 +195,9 @@ mock agent availability, agent/service listings, queued completion responses, and test-only AI feature flag overrides. - This endpoint is only available when `EnableTesting` is enabled. + This endpoint is only available when `EnableTesting` is enabled. `EnableTesting` + is intended only for isolated non-production environments and must never be + enabled in production. ##### Permissions @@ -228,7 +230,9 @@ description: > Retrieve the current in-memory AI bridge test helper state used for end-to-end tests. - This endpoint is only available when `EnableTesting` is enabled. + This endpoint is only available when `EnableTesting` is enabled. `EnableTesting` + is intended only for isolated non-production environments and must never be + enabled in production. ##### Permissions @@ -252,7 +256,9 @@ description: > Reset the in-memory AI bridge test helper state used for end-to-end tests. - This endpoint is only available when `EnableTesting` is enabled. + This endpoint is only available when `EnableTesting` is enabled. `EnableTesting` + is intended only for isolated non-production environments and must never be + enabled in production. ##### Permissions diff --git a/server/tests/README.md b/server/tests/README.md index 60481b41438..6e2d7d03498 100644 --- a/server/tests/README.md +++ b/server/tests/README.md @@ -1,8 +1,8 @@ # Testing Text Processing -The text processing tests located in the [doc/developer/tests folder](https://github.com/mattermost/platform/tree/master/doc/developer/tests) are designed for use with the `/test url` command. This command posts the raw contents of a specified .md file in the doc/developer/test folder into Mattermost. +The text processing tests located in the [doc/developer/tests folder](https://github.com/mattermost/platform/tree/master/doc/developer/tests) are designed for use with the `/test url` command in isolated non-production test environments. This command posts the raw contents of a specified `.md` file in the `doc/developer/tests` folder into Mattermost. ## Turning on /test -Access the **System Console** from the Main Menu. Under *Service Settings* make sure that *Enable Testing* is set to `true`, then click **Save**. You may also change this setting from `config.json` by setting `”EnableTesting”: true`. Changing this setting requires a server restart to take effect. +Access the **System Console** from the Main Menu. Under *Environment > Developer* make sure that *Enable Testing Commands* is set to `true`, then click **Save**. You may also change this setting from `config.json` by setting `"EnableTesting": true`. This setting is intended only for isolated non-production environments with test users and sample data, and must never be enabled in production. Changing this setting requires a server restart to take effect. ## Running the Tests In the text input box in Mattermost, type: `/test url [file-name-in-testing-folder].md`. Some examples: diff --git a/webapp/channels/src/components/admin_console/admin_definition.tsx b/webapp/channels/src/components/admin_console/admin_definition.tsx index 81892c3f207..6f67f51636c 100644 --- a/webapp/channels/src/components/admin_console/admin_definition.tsx +++ b/webapp/channels/src/components/admin_console/admin_definition.tsx @@ -2161,11 +2161,16 @@ const AdminDefinition: AdminDefinitionType = { id: 'ServiceSettings', name: defineMessage({id: 'admin.developer.title', defaultMessage: 'Developer Settings'}), settings: [ + { + type: 'banner', + label: defineMessage({id: 'admin.service.testingWarning', defaultMessage: 'Warning: Testing commands are intended only for isolated non-production environments with test users and sample data. Never enable this setting in production.'}), + banner_type: 'warning', + }, { type: 'bool', key: 'ServiceSettings.EnableTesting', label: defineMessage({id: 'admin.service.testingTitle', defaultMessage: 'Enable Testing Commands:'}), - help_text: defineMessage({id: 'admin.service.testingDescription', defaultMessage: 'When true, /test slash command is enabled to load test accounts, data and text formatting. Changing this requires a server restart before taking effect.'}), + help_text: defineMessage({id: 'admin.service.testingDescription', defaultMessage: 'When true, the /test slash command is enabled to load test accounts, data, and text formatting. Use this setting only in isolated non-production environments and never in production. Changing this requires a server restart before taking effect.'}), isDisabled: it.not(it.userHasWritePermissionOnResource(RESOURCE_KEYS.ENVIRONMENT.DEVELOPER)), }, { diff --git a/webapp/channels/src/components/admin_console/admin_definition_enable_testing.test.tsx b/webapp/channels/src/components/admin_console/admin_definition_enable_testing.test.tsx new file mode 100644 index 00000000000..b7bdfe0aca4 --- /dev/null +++ b/webapp/channels/src/components/admin_console/admin_definition_enable_testing.test.tsx @@ -0,0 +1,48 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import AdminDefinition from './admin_definition'; +import type {AdminDefinitionSetting, AdminDefinitionSettingBanner} from './types'; + +describe('AdminDefinition - Enable Testing setting', () => { + const getDeveloperSettings = () => { + const developerSection = AdminDefinition.environment.subsections.developer; + const schema = developerSection?.schema; + + return (schema && 'settings' in schema && schema.settings) ? schema.settings : []; + }; + + test('includes a warning banner before the EnableTesting setting', () => { + const settings = getDeveloperSettings(); + const bannerIndex = settings.findIndex((setting: AdminDefinitionSetting) => setting.type === 'banner'); + const enableTestingIndex = settings.findIndex((setting: AdminDefinitionSetting) => setting.key === 'ServiceSettings.EnableTesting'); + + expect(bannerIndex).toBeGreaterThanOrEqual(0); + expect(enableTestingIndex).toBeGreaterThanOrEqual(0); + expect(bannerIndex).toBeLessThan(enableTestingIndex); + + const banner = settings[bannerIndex] as AdminDefinitionSettingBanner; + expect(banner.banner_type).toBe('warning'); + expect(typeof banner.label).toBe('object'); + if (banner.label && typeof banner.label === 'object') { + expect(banner.label).toMatchObject({ + id: 'admin.service.testingWarning', + defaultMessage: expect.stringContaining('Never enable this setting in production.'), + }); + } + }); + + test('has explicit non-production guidance in the EnableTesting help text', () => { + const settings = getDeveloperSettings(); + const setting = settings.find((item: AdminDefinitionSetting) => item.key === 'ServiceSettings.EnableTesting'); + + expect(setting).toBeDefined(); + expect(setting?.type).toBe('bool'); + expect(setting?.help_text).toBeDefined(); + expect(typeof setting?.help_text).toBe('object'); + expect(setting?.help_text).toMatchObject({ + id: 'admin.service.testingDescription', + defaultMessage: expect.stringContaining('never in production'), + }); + }); +}); diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index 89a67df54d9..8ee6cd233b6 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -2881,8 +2881,9 @@ "admin.service.ssoSessionHoursDesc.extendLength": "Set the number of hours from the last activity in Mattermost to the expiry of the user's session for SSO authentication, such as SAML, GitLab and OAuth 2.0. If the authentication method is SAML or GitLab, the user may automatically be logged back in to Mattermost if they are already logged in to SAML or GitLab. After changing this setting, the setting will take effect after the next time the user enters their credentials.", "admin.service.terminateSessionsOnPasswordChange.helpText": "When true, all sessions of a user will expire if their password is changed by themselves or an administrator. If password change is initiated by user, their current session is not terminated.", "admin.service.terminateSessionsOnPasswordChange.label": "Terminate Sessions on Password Change: ", - "admin.service.testingDescription": "When true, /test slash command is enabled to load test accounts, data and text formatting. Changing this requires a server restart before taking effect.", + "admin.service.testingDescription": "When true, the /test slash command is enabled to load test accounts, data, and text formatting. Use this setting only in isolated non-production environments and never in production. Changing this requires a server restart before taking effect.", "admin.service.testingTitle": "Enable Testing Commands:", + "admin.service.testingWarning": "Warning: Testing commands are intended only for isolated non-production environments with test users and sample data. Never enable this setting in production.", "admin.service.testSiteURL": "Test Live URL", "admin.service.testSiteURLFail": "Test unsuccessful: {error}", "admin.service.testSiteURLSuccess": "Test successful. This is a valid URL.", From 6c0e0fee4a337902c75b7c3dfe98f93654a59cc1 Mon Sep 17 00:00:00 2001 From: David Krauser Date: Wed, 29 Apr 2026 14:47:34 -0400 Subject: [PATCH 4/4] [MM-68464] Introduce system object type for property fields and values (#36250) --- api/v4/source/properties.yaml | 103 ++++++- .../classification_markings.spec.ts | 13 +- server/channels/api4/api.go | 10 +- server/channels/api4/properties.go | 121 ++++++-- server/channels/api4/properties_test.go | 274 ++++++++++++++++++ server/channels/app/property_value.go | 2 + server/channels/app/property_value_test.go | 7 + server/i18n/en.json | 8 + server/public/model/client4.go | 36 +++ server/public/model/property_field.go | 7 + server/public/model/property_field_test.go | 45 +++ server/public/model/property_value.go | 18 +- server/public/model/property_value_test.go | 39 +++ webapp/platform/client/src/client4.ts | 14 + 14 files changed, 671 insertions(+), 26 deletions(-) diff --git a/api/v4/source/properties.yaml b/api/v4/source/properties.yaml index 1d532e30da7..4984dfc399d 100644 --- a/api/v4/source/properties.yaml +++ b/api/v4/source/properties.yaml @@ -99,7 +99,7 @@ - properties summary: Get property fields description: > - Get a list of property fields for a specific group and object type. Requires a target_type parameter to scope the query. Filter further by target_id to narrow results. Uses cursor-based pagination. + Get a list of property fields for a specific group and object type. Requires a target_type parameter to scope the query, except when `object_type=system` — in that case `target_type` is implicit and any value supplied is ignored. Filter further by target_id to narrow results. Uses cursor-based pagination. operationId: GetPropertyFields parameters: - name: group_name @@ -283,6 +283,7 @@ description: > Get all property values for a specific target within a group. The `template` object type cannot have values and will return 400. + The `system` object type must use the dedicated `/api/v4/properties/groups/{group_name}/system/values` endpoint and will return 400 on this route. operationId: GetPropertyValues parameters: - name: group_name @@ -342,6 +343,7 @@ description: > Update one or more property values for a specific target within a group. Uses upsert semantics: creates the value if it doesn't exist, updates it if it does. All field IDs must belong to the specified group. The `template` object type cannot have values and will return 400. + The `system` object type must use the dedicated `/api/v4/properties/groups/{group_name}/system/values` endpoint and will return 400 on this route. operationId: UpdatePropertyValues parameters: - name: group_name @@ -395,3 +397,102 @@ $ref: "#/components/responses/Unauthorized" "403": $ref: "#/components/responses/Forbidden" + "/api/v4/properties/groups/{group_name}/system/values": + get: + tags: + - properties + summary: Get property values for the system + description: > + Get all property values attached to the Mattermost instance itself + within a group. System-scoped values are readable by any authenticated + user. This endpoint is the dedicated route for `system` object type; + the `{object_type}/values/{target_id}` route returns 400 for `system`. + operationId: GetSystemPropertyValues + parameters: + - name: group_name + in: path + description: The name of the property group + required: true + schema: + type: string + - name: cursor_id + in: query + description: The ID of the last property value from the previous page, for cursor-based pagination. + schema: + type: string + - name: cursor_create_at + in: query + description: The create_at timestamp of the last property value from the previous page. Must be provided together with cursor_id. + schema: + type: integer + format: int64 + - name: per_page + in: query + description: The number of property values per page. + schema: + type: integer + default: 60 + responses: + "200": + description: System property values retrieval successful + content: + application/json: + schema: + type: array + items: + $ref: "#/components/schemas/PropertyValue" + "400": + $ref: "#/components/responses/BadRequest" + "401": + $ref: "#/components/responses/Unauthorized" + patch: + tags: + - properties + summary: Update property values for the system + description: > + Update one or more property values attached to the Mattermost instance + itself. Uses upsert semantics: creates the value if it doesn't exist, + updates it if it does. Requires system administrator access. All field + IDs must reference `system` object-type fields in the specified group; + template field IDs are rejected with 400. + operationId: UpdateSystemPropertyValues + parameters: + - name: group_name + in: path + description: The name of the property group + required: true + schema: + type: string + requestBody: + content: + application/json: + schema: + type: array + items: + type: object + required: + - field_id + - value + properties: + field_id: + type: string + description: The ID of the property field + value: + description: The value to set for this property. Can be any JSON type. + description: Array of property values to update + required: true + responses: + "200": + description: System property values update successful + content: + application/json: + schema: + type: array + items: + $ref: "#/components/schemas/PropertyValue" + "400": + $ref: "#/components/responses/BadRequest" + "401": + $ref: "#/components/responses/Unauthorized" + "403": + $ref: "#/components/responses/Forbidden" diff --git a/e2e-tests/playwright/specs/functional/system_console/site_configuration/classification_markings.spec.ts b/e2e-tests/playwright/specs/functional/system_console/site_configuration/classification_markings.spec.ts index 0de3fb39d71..61c9e963835 100644 --- a/e2e-tests/playwright/specs/functional/system_console/site_configuration/classification_markings.spec.ts +++ b/e2e-tests/playwright/specs/functional/system_console/site_configuration/classification_markings.spec.ts @@ -125,7 +125,12 @@ test.describe('System Console - Classification markings', () => { /** * @objective Verify selecting a built-in preset and saving creates the classification field successfully. */ - test( + // Skipped: master regression. The CM frontend creates a property field on the + // `custom_profile_attributes` group, registered as PSAv1 in server.go, but + // the API rejects v1 groups since #36171 ("PSAv2 generic APIs blacklist"). + // Re-enable once the CPA group is migrated to v2 (or v1 template/system + // creates are allowed, mirroring the patch-route FIXME at properties.go:299). + test.skip( 'MM-T6204 classification markings: select NATO preset and save', {tag: ['@system_console', '@classification_markings']}, async ({pw}) => { @@ -161,7 +166,8 @@ test.describe('System Console - Classification markings', () => { /** * @objective When a classification field already exists, changing preset shows a warning modal; confirming applies the new preset. */ - test( + // Skipped: see MM-T6204 — saving a preset hits the v2_group_not_found check. + test.skip( 'MM-T6205 classification markings: preset change shows confirm modal then applies', {tag: ['@system_console', '@classification_markings']}, async ({pw}) => { @@ -205,7 +211,8 @@ test.describe('System Console - Classification markings', () => { /** * @objective After saving a preset, deleting a level switches the preset dropdown to Custom and save still succeeds. */ - test( + // Skipped: see MM-T6204 — saving a preset hits the v2_group_not_found check. + test.skip( 'MM-T6206 classification markings: delete level switches to custom and saves', {tag: ['@system_console', '@classification_markings']}, async ({pw}) => { diff --git a/server/channels/api4/api.go b/server/channels/api4/api.go index 1a44271616f..42a52c20788 100644 --- a/server/channels/api4/api.go +++ b/server/channels/api4/api.go @@ -171,10 +171,11 @@ type Routes struct { Agents *mux.Router // 'api/v4/agents' LLMServices *mux.Router // 'api/v4/llmservices' - Properties *mux.Router // 'api/v4/properties' - PropertyFields *mux.Router // 'api/v4/properties/groups/{group_name:[a-z][a-z0-9_]*}/{object_type:[a-z]+}/fields' - PropertyField *mux.Router // 'api/v4/properties/groups/{group_name:[a-z][a-z0-9_]*}/{object_type:[a-z]+}/fields/{field_id:[A-Za-z0-9]+}' - PropertyValues *mux.Router // 'api/v4/properties/groups/{group_name:[a-z][a-z0-9_]*}/{object_type:[a-z]+}/values/{target_id:[A-Za-z0-9]+}' + Properties *mux.Router // 'api/v4/properties' + PropertyFields *mux.Router // 'api/v4/properties/groups/{group_name:[a-z][a-z0-9_]*}/{object_type:[a-z]+}/fields' + PropertyField *mux.Router // 'api/v4/properties/groups/{group_name:[a-z][a-z0-9_]*}/{object_type:[a-z]+}/fields/{field_id:[A-Za-z0-9]+}' + PropertyValues *mux.Router // 'api/v4/properties/groups/{group_name:[a-z][a-z0-9_]*}/{object_type:[a-z]+}/values/{target_id:[A-Za-z0-9]+}' + PropertySystemValues *mux.Router // 'api/v4/properties/groups/{group_name:[a-z][a-z0-9_]*}/system/values' } type API struct { @@ -335,6 +336,7 @@ func Init(srv *app.Server) (*API, error) { api.BaseRoutes.PropertyFields = api.BaseRoutes.Properties.PathPrefix("/groups/{group_name:[a-z][a-z0-9_]*}/{object_type:[a-z]+}/fields").Subrouter() api.BaseRoutes.PropertyField = api.BaseRoutes.PropertyFields.PathPrefix("/{field_id:[A-Za-z0-9]+}").Subrouter() api.BaseRoutes.PropertyValues = api.BaseRoutes.Properties.PathPrefix("/groups/{group_name:[a-z][a-z0-9_]*}/{object_type:[a-z]+}/values/{target_id:[A-Za-z0-9]+}").Subrouter() + api.BaseRoutes.PropertySystemValues = api.BaseRoutes.Properties.PathPrefix("/groups/{group_name:[a-z][a-z0-9_]*}/system/values").Subrouter() api.InitUser() api.InitBot() diff --git a/server/channels/api4/properties.go b/server/channels/api4/properties.go index 16ee192412a..0b70e37a71d 100644 --- a/server/channels/api4/properties.go +++ b/server/channels/api4/properties.go @@ -19,12 +19,14 @@ const maxPropertyValuePatchItems = 50 func (api *API) InitProperties() { api.BaseRoutes.PropertyFields.Handle("", api.APISessionRequired(getPropertyFields)).Methods(http.MethodGet) api.BaseRoutes.PropertyValues.Handle("", api.APISessionRequired(getPropertyValues)).Methods(http.MethodGet) + api.BaseRoutes.PropertySystemValues.Handle("", api.APISessionRequired(getSystemPropertyValues)).Methods(http.MethodGet) if api.srv.Config().FeatureFlags.IntegratedBoards || api.srv.Config().FeatureFlags.ClassificationMarkings { api.BaseRoutes.PropertyFields.Handle("", api.APISessionRequired(createPropertyField)).Methods(http.MethodPost) api.BaseRoutes.PropertyField.Handle("", api.APISessionRequired(patchPropertyField)).Methods(http.MethodPatch) api.BaseRoutes.PropertyField.Handle("", api.APISessionRequired(deletePropertyField)).Methods(http.MethodDelete) api.BaseRoutes.PropertyValues.Handle("", api.APISessionRequired(patchPropertyValues)).Methods(http.MethodPatch) + api.BaseRoutes.PropertySystemValues.Handle("", api.APISessionRequired(patchSystemPropertyValues)).Methods(http.MethodPatch) } } @@ -67,6 +69,22 @@ func createPropertyField(c *Context, w http.ResponseWriter, r *http.Request) { field.ObjectType = c.Params.ObjectType field.GroupID = group.ID + // System-object fields attach to the system itself; canonicalize the + // target fields so clients can't submit inconsistent combinations. + // Permissions are likewise pinned to sysadmin: a system field's + // TargetType is "system", which makes member-level scope checks resolve + // to "any authenticated user" (see hasPropertyFieldScopeAccess), so + // honouring a member-level permission on a system field would expose + // the field's definition, options, and values to every logged-in user. + if field.ObjectType == model.PropertyFieldObjectTypeSystem { + field.TargetType = string(model.PropertyFieldTargetLevelSystem) + field.TargetID = "" + sysadmin := model.PermissionLevelSysadmin + field.PermissionField = &sysadmin + field.PermissionValues = &sysadmin + field.PermissionOptions = &sysadmin + } + // Reject protected field creation via API if field.Protected { c.Err = model.NewAppError("createPropertyField", "api.property_field.create.protected_via_api.app_error", nil, "", http.StatusBadRequest) @@ -117,9 +135,12 @@ func createPropertyField(c *Context, w http.ResponseWriter, r *http.Request) { // Set permissions based on admin status. // Permissions are not accepted from the request body; they're set by the server. // Templates default to sysadmin since they define the schema linked fields inherit. + // System-object fields likewise default to sysadmin since they attach to the + // Mattermost instance and only a system administrator should write them. isAdmin := c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) defaultLevel := model.PermissionLevelMember - if field.ObjectType == model.PropertyFieldObjectTypeTemplate { + if field.ObjectType == model.PropertyFieldObjectTypeTemplate || + field.ObjectType == model.PropertyFieldObjectTypeSystem { defaultLevel = model.PermissionLevelSysadmin } if !isAdmin { @@ -197,11 +218,16 @@ func getPropertyFields(c *Context, w http.ResponseWriter, r *http.Request) { } } - // Required target_type filter - opts.TargetType = query.Get("target_type") - if !model.IsValidPSAv2PropertyFieldTargetType(opts.TargetType) { - c.Err = model.NewAppError("getPropertyFields", "api.property_field.get.invalid_target_type.app_error", nil, "", http.StatusBadRequest) - return + // target_type filter: required in general, but implicit for the system + // object type since it can only ever live at the system level. + if c.Params.ObjectType == model.PropertyFieldObjectTypeSystem { + opts.TargetType = string(model.PropertyFieldTargetLevelSystem) + } else { + opts.TargetType = query.Get("target_type") + if !model.IsValidPSAv2PropertyFieldTargetType(opts.TargetType) { + c.Err = model.NewAppError("getPropertyFields", "api.property_field.get.invalid_target_type.app_error", nil, "", http.StatusBadRequest) + return + } } // Optional target_id filter @@ -456,32 +482,50 @@ func getPropertyValues(c *Context, w http.ResponseWriter, r *http.Request) { return } + if c.Params.ObjectType == model.PropertyFieldObjectTypeSystem { + c.Err = model.NewAppError("getPropertyValues", "api.property_value.system_use_dedicated_route.app_error", nil, "system values must use the dedicated system values endpoint", http.StatusBadRequest) + return + } + c.RequireTargetId() if c.Err != nil { return } + getPropertyValuesCore(c, w, r, c.Params.ObjectType, c.Params.TargetId) +} + +func getSystemPropertyValues(c *Context, w http.ResponseWriter, r *http.Request) { + c.RequireGroupName() + if c.Err != nil { + return + } + + getPropertyValuesCore(c, w, r, model.PropertyFieldObjectTypeSystem, model.PropertyValueSystemTargetID) +} + +func getPropertyValuesCore(c *Context, w http.ResponseWriter, r *http.Request, objectType, targetID string) { group := getV2Group(c, "getPropertyValues") if c.Err != nil { return } // Check target access based on object type - if !hasTargetAccess(c, c.Params.ObjectType, c.Params.TargetId, false) { + if !hasTargetAccess(c, objectType, targetID, false) { return } auditRec := c.MakeAuditRecord(model.AuditEventGetPropertyValues, model.AuditStatusFail) defer c.LogAuditRec(auditRec) model.AddEventParameterToAuditRec(auditRec, "group_name", c.Params.GroupName) - model.AddEventParameterToAuditRec(auditRec, "object_type", c.Params.ObjectType) - model.AddEventParameterToAuditRec(auditRec, "target_id", c.Params.TargetId) + model.AddEventParameterToAuditRec(auditRec, "object_type", objectType) + model.AddEventParameterToAuditRec(auditRec, "target_id", targetID) query := r.URL.Query() opts := model.PropertyValueSearchOpts{ - TargetIDs: []string{c.Params.TargetId}, - TargetType: c.Params.ObjectType, + TargetIDs: []string{targetID}, + TargetType: objectType, PerPage: c.Params.PerPage, } @@ -522,11 +566,29 @@ func patchPropertyValues(c *Context, w http.ResponseWriter, r *http.Request) { return } + if c.Params.ObjectType == model.PropertyFieldObjectTypeSystem { + c.Err = model.NewAppError("patchPropertyValues", "api.property_value.system_use_dedicated_route.app_error", nil, "system values must use the dedicated system values endpoint", http.StatusBadRequest) + return + } + c.RequireTargetId() if c.Err != nil { return } + patchPropertyValuesCore(c, w, r, c.Params.ObjectType, c.Params.TargetId) +} + +func patchSystemPropertyValues(c *Context, w http.ResponseWriter, r *http.Request) { + c.RequireGroupName() + if c.Err != nil { + return + } + + patchPropertyValuesCore(c, w, r, model.PropertyFieldObjectTypeSystem, model.PropertyValueSystemTargetID) +} + +func patchPropertyValuesCore(c *Context, w http.ResponseWriter, r *http.Request, objectType, targetID string) { group := getV2Group(c, "patchPropertyValues") if c.Err != nil { return @@ -534,7 +596,7 @@ func patchPropertyValues(c *Context, w http.ResponseWriter, r *http.Request) { groupID := group.ID // Check target access based on object type - if !hasTargetAccess(c, c.Params.ObjectType, c.Params.TargetId, true) { + if !hasTargetAccess(c, objectType, targetID, true) { return } @@ -581,6 +643,24 @@ func patchPropertyValues(c *Context, w http.ResponseWriter, r *http.Request) { return } + // Each field's ObjectType must match the route's objectType. Without + // this, a caller could reference a field of one type via another + // type's route (e.g. a system field via the user-values route), + // bypassing the route-level access checks and persisting values whose + // TargetType disagrees with field.ObjectType. Templates are always + // rejected because objectType is never "template" on these routes; + // keep a dedicated error for that case so the cause is clear. + for _, f := range fields { + if f.ObjectType == model.PropertyFieldObjectTypeTemplate { + c.Err = model.NewAppError("patchPropertyValues", "api.property_value.template_no_values.app_error", nil, "template fields cannot have values", http.StatusBadRequest) + return + } + if f.ObjectType != objectType { + c.Err = model.NewAppError("patchPropertyValues", "api.property_value.field_object_type_mismatch.app_error", nil, "", http.StatusBadRequest) + return + } + } + // Build field map for permission checks fieldMap := make(map[string]*model.PropertyField, len(fields)) for _, f := range fields { @@ -590,8 +670,8 @@ func patchPropertyValues(c *Context, w http.ResponseWriter, r *http.Request) { auditRec := c.MakeAuditRecord(model.AuditEventPatchPropertyValues, model.AuditStatusFail) defer c.LogAuditRec(auditRec) model.AddEventParameterToAuditRec(auditRec, "group_name", c.Params.GroupName) - model.AddEventParameterToAuditRec(auditRec, "object_type", c.Params.ObjectType) - model.AddEventParameterToAuditRec(auditRec, "target_id", c.Params.TargetId) + model.AddEventParameterToAuditRec(auditRec, "object_type", objectType) + model.AddEventParameterToAuditRec(auditRec, "target_id", targetID) // Check values permission on each field (all-or-nothing) for _, item := range items { @@ -607,10 +687,10 @@ func patchPropertyValues(c *Context, w http.ResponseWriter, r *http.Request) { values := make([]*model.PropertyValue, len(items)) for i, item := range items { values[i] = &model.PropertyValue{ - TargetID: c.Params.TargetId, + TargetID: targetID, // in PSAv2, values always point to entities of the same // type that their field.ObjectType - TargetType: c.Params.ObjectType, + TargetType: objectType, GroupID: groupID, FieldID: item.FieldID, Value: item.Value, @@ -621,7 +701,7 @@ func patchPropertyValues(c *Context, w http.ResponseWriter, r *http.Request) { connectionID := r.Header.Get(model.ConnectionId) - upserted, err := c.App.UpsertPropertyValues(c.AppContext, values, c.Params.ObjectType, c.Params.TargetId, connectionID) + upserted, err := c.App.UpsertPropertyValues(c.AppContext, values, objectType, targetID, connectionID) if err != nil { c.Err = err return @@ -694,6 +774,13 @@ func hasTargetAccess(c *Context, objectType, targetID string, write bool) bool { return false } } + case model.PropertyFieldObjectTypeSystem: + // Any authenticated user can read system-scoped property values. + // Only a system administrator can write them. + if write && !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) { + c.SetPermissionError(model.PermissionManageSystem) + return false + } case model.PropertyFieldObjectTypeTemplate: // Templates don't have value targets — this should not be reached // if value endpoints properly reject template object type diff --git a/server/channels/api4/properties_test.go b/server/channels/api4/properties_test.go index 5bee7b2c1e1..afcb0fc58c2 100644 --- a/server/channels/api4/properties_test.go +++ b/server/channels/api4/properties_test.go @@ -3260,3 +3260,277 @@ func TestLinkedProperties(t *testing.T) { } }) } + +func TestSystemObjectType(t *testing.T) { + mainHelper.Parallel(t) + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.IntegratedBoards = true + }).InitBasic(t) + + group, err := th.App.RegisterPropertyGroup(th.Context, &model.PropertyGroup{Name: "test_system_object_type", Version: model.PropertyGroupVersionV2}) + require.Nil(t, err) + + t.Run("non-admin cannot create a system field", func(t *testing.T) { + field := &model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + TargetType: string(model.PropertyFieldTargetLevelSystem), + } + _, resp, createErr := th.Client.CreatePropertyField(context.Background(), group.Name, model.PropertyFieldObjectTypeSystem, field) + require.Error(t, createErr) + CheckForbiddenStatus(t, resp) + }) + + t.Run("sysadmin creates a system field with canonical target and sysadmin-default permissions", func(t *testing.T) { + field := &model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + // Intentionally submit mismatched target fields to confirm the server canonicalizes them. + TargetType: string(model.PropertyFieldTargetLevelTeam), + TargetID: model.NewId(), + } + created, resp, createErr := th.SystemAdminClient.CreatePropertyField(context.Background(), group.Name, model.PropertyFieldObjectTypeSystem, field) + require.NoError(t, createErr) + CheckCreatedStatus(t, resp) + + require.Equal(t, model.PropertyFieldObjectTypeSystem, created.ObjectType) + require.Equal(t, string(model.PropertyFieldTargetLevelSystem), created.TargetType) + require.Empty(t, created.TargetID) + require.NotNil(t, created.PermissionField) + require.Equal(t, model.PermissionLevelSysadmin, *created.PermissionField) + require.NotNil(t, created.PermissionValues) + require.Equal(t, model.PermissionLevelSysadmin, *created.PermissionValues) + }) + + t.Run("any authenticated user can list system fields", func(t *testing.T) { + // No target_type query required when object_type=system. + fields, resp, getErr := th.Client.GetPropertyFields(context.Background(), group.Name, model.PropertyFieldObjectTypeSystem, model.PropertyFieldSearch{}) + require.NoError(t, getErr) + CheckOKStatus(t, resp) + require.NotEmpty(t, fields) + }) + + t.Run("legacy values GET route rejects system object type", func(t *testing.T) { + // Force a URL that matches {object_type}/values/{target_id}. "system" is a valid + // character set for {target_id}, so the route is reachable even though the handler + // must reject it. + _, resp, getErr := th.Client.GetPropertyValues(context.Background(), group.Name, model.PropertyFieldObjectTypeSystem, model.PropertyValueSystemTargetID, model.PropertyValueSearch{}) + require.Error(t, getErr) + CheckBadRequestStatus(t, resp) + }) + + t.Run("legacy values PATCH route rejects system object type", func(t *testing.T) { + // Mirrors the GET rejection: the legacy URL pattern matches system/system, so the + // route is reachable and the handler must reject it pointing callers to the + // dedicated system route. + items := []model.PropertyValuePatchItem{ + {FieldID: model.NewId(), Value: json.RawMessage(`"ignored"`)}, + } + _, resp, patchErr := th.SystemAdminClient.PatchPropertyValues(context.Background(), group.Name, model.PropertyFieldObjectTypeSystem, model.PropertyValueSystemTargetID, items) + require.Error(t, patchErr) + CheckBadRequestStatus(t, resp) + }) + + t.Run("system PATCH rejects template field IDs", func(t *testing.T) { + // Create the template field directly via the app layer so the test stays + // focused on the value-patch handler. Template fields are definition-only + // and must never accept values, regardless of entry point. + templateField := &model.PropertyField{ + GroupID: group.ID, + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeTemplate, + TargetType: string(model.PropertyFieldTargetLevelSystem), + PermissionField: model.NewPointer(model.PermissionLevelSysadmin), + PermissionValues: model.NewPointer(model.PermissionLevelSysadmin), + PermissionOptions: model.NewPointer(model.PermissionLevelSysadmin), + } + created, appErr := th.App.CreatePropertyField(th.Context, templateField, false, "") + require.Nil(t, appErr) + + items := []model.PropertyValuePatchItem{ + {FieldID: created.ID, Value: json.RawMessage(`"ignored"`)}, + } + _, resp, patchErr := th.SystemAdminClient.PatchSystemPropertyValues(context.Background(), group.Name, items) + require.Error(t, patchErr) + CheckBadRequestStatus(t, resp) + }) + + t.Run("system field round-trips a value via the dedicated route", func(t *testing.T) { + field := &model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + } + created, resp, createErr := th.SystemAdminClient.CreatePropertyField(context.Background(), group.Name, model.PropertyFieldObjectTypeSystem, field) + require.NoError(t, createErr) + CheckCreatedStatus(t, resp) + + items := []model.PropertyValuePatchItem{ + {FieldID: created.ID, Value: json.RawMessage(`"hello-system"`)}, + } + + // Non-admin cannot write a system value. + _, resp, patchErr := th.Client.PatchSystemPropertyValues(context.Background(), group.Name, items) + require.Error(t, patchErr) + CheckForbiddenStatus(t, resp) + + // Sysadmin writes succeed and store the sentinel TargetID. + upserted, resp, patchErr := th.SystemAdminClient.PatchSystemPropertyValues(context.Background(), group.Name, items) + require.NoError(t, patchErr) + CheckOKStatus(t, resp) + require.Len(t, upserted, 1) + require.Equal(t, model.PropertyValueSystemTargetID, upserted[0].TargetID) + require.Equal(t, model.PropertyValueTargetTypeSystem, upserted[0].TargetType) + + // Any authed user can read. + values, resp, getErr := th.Client.GetSystemPropertyValues(context.Background(), group.Name, model.PropertyValueSearch{}) + require.NoError(t, getErr) + CheckOKStatus(t, resp) + found := false + for _, v := range values { + if v.FieldID == created.ID { + require.Equal(t, model.PropertyValueSystemTargetID, v.TargetID) + require.Equal(t, json.RawMessage(`"hello-system"`), v.Value) + found = true + } + } + require.True(t, found, "expected to find the system value we just wrote") + }) + + t.Run("system value patch broadcasts system-wide", func(t *testing.T) { + field := &model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + } + created, appErr := th.App.CreatePropertyField(th.Context, &model.PropertyField{ + GroupID: group.ID, + Name: field.Name, + Type: field.Type, + ObjectType: model.PropertyFieldObjectTypeSystem, + TargetType: string(model.PropertyFieldTargetLevelSystem), + PermissionField: model.NewPointer(model.PermissionLevelSysadmin), + PermissionValues: model.NewPointer(model.PermissionLevelSysadmin), + PermissionOptions: model.NewPointer(model.PermissionLevelSysadmin), + }, false, "") + require.Nil(t, appErr) + + th.LoginBasic(t) + webSocketClient := th.CreateConnectedWebSocketClient(t) + + items := []model.PropertyValuePatchItem{ + {FieldID: created.ID, Value: json.RawMessage(`"broadcast-me"`)}, + } + _, resp, patchErr := th.SystemAdminClient.PatchSystemPropertyValues(context.Background(), group.Name, items) + require.NoError(t, patchErr) + CheckOKStatus(t, resp) + + require.Eventually(t, func() bool { + select { + case event := <-webSocketClient.EventChannel: + if event.EventType() == model.WebsocketEventPropertyValuesUpdated && + event.GetData()["object_type"] == model.PropertyFieldObjectTypeSystem { + require.Equal(t, model.PropertyValueSystemTargetID, event.GetData()["target_id"]) + // System target: broadcast should be system-wide (empty team/channel) + require.Empty(t, event.GetBroadcast().TeamId) + require.Empty(t, event.GetBroadcast().ChannelId) + return true + } + default: + return false + } + return false + }, 5*time.Second, 100*time.Millisecond) + }) + + t.Run("system field permissions are canonicalized to sysadmin even when admin requests member", func(t *testing.T) { + // Without canonicalization, an admin could POST member-level + // permissions on a system field; the per-field permission check + // would then resolve "member" against TargetType=system, which + // hasPropertyFieldScopeAccess treats as "any authenticated user", + // effectively making the field publicly mutable. + field := &model.PropertyField{ + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + PermissionField: model.NewPointer(model.PermissionLevelMember), + PermissionValues: model.NewPointer(model.PermissionLevelMember), + PermissionOptions: model.NewPointer(model.PermissionLevelMember), + } + created, resp, createErr := th.SystemAdminClient.CreatePropertyField(context.Background(), group.Name, model.PropertyFieldObjectTypeSystem, field) + require.NoError(t, createErr) + CheckCreatedStatus(t, resp) + + require.NotNil(t, created.PermissionField) + require.Equal(t, model.PermissionLevelSysadmin, *created.PermissionField) + require.NotNil(t, created.PermissionValues) + require.Equal(t, model.PermissionLevelSysadmin, *created.PermissionValues) + require.NotNil(t, created.PermissionOptions) + require.Equal(t, model.PermissionLevelSysadmin, *created.PermissionOptions) + + // With permissions canonicalized to sysadmin, the existing + // per-field permission check rejects non-admin patch/delete + // without any additional handler-level floor. + newName := model.NewId() + patch := &model.PropertyFieldPatch{Name: &newName} + _, resp, patchErr := th.Client.PatchPropertyField(context.Background(), group.Name, model.PropertyFieldObjectTypeSystem, created.ID, patch) + require.Error(t, patchErr) + CheckForbiddenStatus(t, resp) + + resp, deleteErr := th.Client.DeletePropertyField(context.Background(), group.Name, model.PropertyFieldObjectTypeSystem, created.ID) + require.Error(t, deleteErr) + CheckForbiddenStatus(t, resp) + }) + + t.Run("legacy values PATCH route rejects body referencing a system field ID", func(t *testing.T) { + // Cross-route reference: a non-system route receives a body whose + // field IDs belong to a system-typed field. Without the ObjectType + // match check this would either bypass the system-write sysadmin + // gate or persist a value whose TargetType disagrees with + // field.ObjectType. + systemField, appErr := th.App.CreatePropertyField(th.Context, &model.PropertyField{ + GroupID: group.ID, + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeSystem, + TargetType: string(model.PropertyFieldTargetLevelSystem), + PermissionField: model.NewPointer(model.PermissionLevelSysadmin), + PermissionValues: model.NewPointer(model.PermissionLevelMember), + PermissionOptions: model.NewPointer(model.PermissionLevelSysadmin), + }, false, "") + require.Nil(t, appErr) + + items := []model.PropertyValuePatchItem{ + {FieldID: systemField.ID, Value: json.RawMessage(`"smuggled"`)}, + } + // Even sysadmin should be rejected — this is a structural check on + // the route, not a permission check. + _, resp, patchErr := th.SystemAdminClient.PatchPropertyValues(context.Background(), group.Name, model.PropertyFieldObjectTypeUser, th.SystemAdminUser.Id, items) + require.Error(t, patchErr) + CheckBadRequestStatus(t, resp) + }) + + t.Run("system values PATCH route rejects body referencing a non-system field ID", func(t *testing.T) { + // Inverse of the previous case: the dedicated system route is + // reachable only by sysadmin, but it must still reject body field + // IDs whose ObjectType isn't system, otherwise rows get persisted + // with TargetType=system pointing at fields that attach to other + // entity types. + postField, appErr := th.App.CreatePropertyField(th.Context, &model.PropertyField{ + GroupID: group.ID, + Name: model.NewId(), + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypePost, + TargetType: string(model.PropertyFieldTargetLevelSystem), + PermissionField: model.NewPointer(model.PermissionLevelSysadmin), + PermissionValues: model.NewPointer(model.PermissionLevelSysadmin), + PermissionOptions: model.NewPointer(model.PermissionLevelSysadmin), + }, false, "") + require.Nil(t, appErr) + + items := []model.PropertyValuePatchItem{ + {FieldID: postField.ID, Value: json.RawMessage(`"misrouted"`)}, + } + _, resp, patchErr := th.SystemAdminClient.PatchSystemPropertyValues(context.Background(), group.Name, items) + require.Error(t, patchErr) + CheckBadRequestStatus(t, resp) + }) +} diff --git a/server/channels/app/property_value.go b/server/channels/app/property_value.go index c6cca909fc4..56238be938b 100644 --- a/server/channels/app/property_value.go +++ b/server/channels/app/property_value.go @@ -24,6 +24,8 @@ func (a *App) resolveValueBroadcastParams(rctx request.CTX, objectType, targetID return "", targetID, nil case model.PropertyFieldObjectTypeUser: return "", "", nil // system-wide + case model.PropertyFieldObjectTypeSystem: + return "", "", nil // system-wide default: return "", "", model.NewAppError( "resolveValueBroadcastParams", diff --git a/server/channels/app/property_value_test.go b/server/channels/app/property_value_test.go index f55027aa3c9..4b65660aed3 100644 --- a/server/channels/app/property_value_test.go +++ b/server/channels/app/property_value_test.go @@ -45,6 +45,13 @@ func TestResolveValueBroadcastParams(t *testing.T) { assert.Empty(t, channelID) }) + t.Run("system object type returns empty strings for system-wide broadcast", func(t *testing.T) { + teamID, channelID, err := th.App.resolveValueBroadcastParams(th.Context, model.PropertyFieldObjectTypeSystem, model.PropertyValueSystemTargetID) + require.Nil(t, err) + assert.Empty(t, teamID) + assert.Empty(t, channelID) + }) + t.Run("unknown object type returns an error", func(t *testing.T) { _, _, err := th.App.resolveValueBroadcastParams(th.Context, "unknown_type", "target123") require.NotNil(t, err) diff --git a/server/i18n/en.json b/server/i18n/en.json index 7d45fa5eb63..a08abfa3511 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -3218,6 +3218,10 @@ "id": "api.property_field.update.protected_via_api.app_error", "translation": "Cannot update a protected property field via API." }, + { + "id": "api.property_value.field_object_type_mismatch.app_error", + "translation": "One or more property fields do not match the route's object type." + }, { "id": "api.property_value.invalid_object_type.app_error", "translation": "The provided object type is not valid." @@ -3242,6 +3246,10 @@ "id": "api.property_value.patch.too_many_items.request_error", "translation": "Unable to update that many property values. Only {{.Max}} property values can be updated at once." }, + { + "id": "api.property_value.system_use_dedicated_route.app_error", + "translation": "System values must use the dedicated system values endpoint." + }, { "id": "api.property_value.target_user.forbidden.app_error", "translation": "You do not have permission to access property values for another user." diff --git a/server/public/model/client4.go b/server/public/model/client4.go index 37bee97aa28..eddecbeb4a4 100644 --- a/server/public/model/client4.go +++ b/server/public/model/client4.go @@ -686,6 +686,10 @@ func (c *Client4) propertyValuesRoute(groupName, objectType, targetID string) cl return newClientRoute("properties").Join("groups", groupName, objectType, "values", targetID) } +func (c *Client4) propertySystemValuesRoute(groupName string) clientRoute { + return newClientRoute("properties").Join("groups", groupName, PropertyFieldObjectTypeSystem, "values") +} + func (c *Client4) accessControlPoliciesRoute() clientRoute { return newClientRoute("access_control_policies") } @@ -8098,6 +8102,38 @@ func (c *Client4) PatchPropertyValues(ctx context.Context, groupName, objectType return DecodeJSONFromResponse[[]*PropertyValue](r) } +// GetSystemPropertyValues returns the property values attached to the Mattermost +// system itself in the given group. +func (c *Client4) GetSystemPropertyValues(ctx context.Context, groupName string, search PropertyValueSearch) ([]*PropertyValue, *Response, error) { + values := url.Values{} + if search.PerPage > 0 { + values.Set("per_page", strconv.Itoa(search.PerPage)) + } + if search.CursorID != "" && search.CursorCreateAt > 0 { + values.Set("cursor_id", search.CursorID) + values.Set("cursor_create_at", strconv.FormatInt(search.CursorCreateAt, 10)) + } else if search.CursorID != "" || search.CursorCreateAt > 0 { + return nil, nil, errors.New("both cursor_id and cursor_create_at must be provided together") + } + r, err := c.doAPIGetWithQuery(ctx, c.propertySystemValuesRoute(groupName), values, "") + if err != nil { + return nil, BuildResponse(r), err + } + defer closeBody(r) + return DecodeJSONFromResponse[[]*PropertyValue](r) +} + +// PatchSystemPropertyValues upserts property values attached to the Mattermost +// system itself in the given group. +func (c *Client4) PatchSystemPropertyValues(ctx context.Context, groupName string, items []PropertyValuePatchItem) ([]*PropertyValue, *Response, error) { + r, err := c.doAPIPatchJSON(ctx, c.propertySystemValuesRoute(groupName), items) + if err != nil { + return nil, BuildResponse(r), err + } + defer closeBody(r) + return DecodeJSONFromResponse[[]*PropertyValue](r) +} + func (c *Client4) GetPostPropertyValues(ctx context.Context, postId string) ([]PropertyValue, *Response, error) { r, err := c.doAPIGet(ctx, c.contentFlaggingRoute().Join("post", postId, "field_values"), "") if err != nil { diff --git a/server/public/model/property_field.go b/server/public/model/property_field.go index 0ced7631c1b..a5dae173773 100644 --- a/server/public/model/property_field.go +++ b/server/public/model/property_field.go @@ -46,6 +46,7 @@ const ( PropertyFieldObjectTypeChannel = "channel" PropertyFieldObjectTypeUser = "user" PropertyFieldObjectTypeTemplate = "template" + PropertyFieldObjectTypeSystem = "system" ) // validPermissionLevels contains all valid PermissionLevel values. @@ -64,6 +65,7 @@ var validPropertyFieldObjectTypes = []string{ PropertyFieldObjectTypeChannel, PropertyFieldObjectTypeUser, PropertyFieldObjectTypeTemplate, + PropertyFieldObjectTypeSystem, } type PropertyField struct { @@ -214,6 +216,11 @@ func (pf *PropertyField) IsValid() error { return NewAppError("PropertyField.IsValid", "model.property_field.is_valid.app_error", map[string]any{"FieldName": "target_id", "Reason": "must be a valid ID for team or channel target type"}, "id="+pf.ID, http.StatusBadRequest) } } + + // System-object fields attach to the system itself; they cannot be scoped below the system level. + if pf.ObjectType == PropertyFieldObjectTypeSystem && pf.TargetType != string(PropertyFieldTargetLevelSystem) { + return NewAppError("PropertyField.IsValid", "model.property_field.is_valid.app_error", map[string]any{"FieldName": "target_type", "Reason": "must be system for system object type"}, "id="+pf.ID, http.StatusBadRequest) + } } else { // PSAv1 properties cannot have permissions or be protected if pf.Protected { diff --git a/server/public/model/property_field_test.go b/server/public/model/property_field_test.go index 99f988d2e54..246c16e81f2 100644 --- a/server/public/model/property_field_test.go +++ b/server/public/model/property_field_test.go @@ -314,6 +314,51 @@ func TestPropertyField_IsValid(t *testing.T) { require.Error(t, pf.IsValid()) }) + t.Run("PSAv2 system ObjectType with system TargetType is valid", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + ObjectType: PropertyFieldObjectTypeSystem, + TargetType: string(PropertyFieldTargetLevelSystem), + TargetID: "", + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.NoError(t, pf.IsValid()) + }) + + t.Run("PSAv2 system ObjectType with team TargetType is invalid", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + ObjectType: PropertyFieldObjectTypeSystem, + TargetType: string(PropertyFieldTargetLevelTeam), + TargetID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + + t.Run("PSAv2 system ObjectType with channel TargetType is invalid", func(t *testing.T) { + pf := &PropertyField{ + ID: NewId(), + GroupID: NewId(), + Name: "test field", + Type: PropertyFieldTypeText, + ObjectType: PropertyFieldObjectTypeSystem, + TargetType: string(PropertyFieldTargetLevelChannel), + TargetID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pf.IsValid()) + }) + t.Run("PSAv2 team TargetType with invalid TargetID is invalid", func(t *testing.T) { pf := &PropertyField{ ID: NewId(), diff --git a/server/public/model/property_value.go b/server/public/model/property_value.go index bfd680bfb41..43d50db4170 100644 --- a/server/public/model/property_value.go +++ b/server/public/model/property_value.go @@ -18,6 +18,13 @@ const ( PropertyValueTargetTypePost = "post" PropertyValueTargetTypeUser = "user" PropertyValueTargetTypeChannel = "channel" + PropertyValueTargetTypeSystem = "system" + + // PropertyValueSystemTargetID is the canonical TargetID sentinel for + // values whose TargetType is "system". System-object values attach to + // the Mattermost instance itself rather than to a user/channel/post, + // so there is no 26-char entity ID available; this sentinel stands in. + PropertyValueSystemTargetID = "system" ) type PropertyValue struct { @@ -34,6 +41,15 @@ type PropertyValue struct { UpdatedBy string `json:"updated_by"` } +// isValidPropertyValueTargetID accepts the canonical system sentinel when +// the value targets the system, and a 26-char entity ID otherwise. +func isValidPropertyValueTargetID(targetType, targetID string) bool { + if targetType == PropertyValueTargetTypeSystem { + return targetID == PropertyValueSystemTargetID + } + return IsValidId(targetID) +} + func (pv *PropertyValue) PreSave() { if pv.ID == "" { pv.ID = NewId() @@ -50,7 +66,7 @@ func (pv *PropertyValue) IsValid() error { return NewAppError("PropertyValue.IsValid", "model.property_value.is_valid.app_error", map[string]any{"FieldName": "id", "Reason": "invalid id"}, "", http.StatusBadRequest) } - if !IsValidId(pv.TargetID) { + if !isValidPropertyValueTargetID(pv.TargetType, pv.TargetID) { return NewAppError("PropertyValue.IsValid", "model.property_value.is_valid.app_error", map[string]any{"FieldName": "target_id", "Reason": "invalid id"}, "id="+pv.ID, http.StatusBadRequest) } diff --git a/server/public/model/property_value_test.go b/server/public/model/property_value_test.go index 05b359b4053..f0bacdb13b0 100644 --- a/server/public/model/property_value_test.go +++ b/server/public/model/property_value_test.go @@ -173,6 +173,45 @@ func TestPropertyValue_IsValid(t *testing.T) { } require.NoError(t, pv.IsValid()) }) + + t.Run("system TargetType with system sentinel TargetID is valid", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: PropertyValueSystemTargetID, + TargetType: PropertyValueTargetTypeSystem, + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.NoError(t, pv.IsValid()) + }) + + t.Run("system TargetType with arbitrary TargetID is invalid", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: NewId(), + TargetType: PropertyValueTargetTypeSystem, + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) + + t.Run("non-system TargetType with system sentinel TargetID is invalid", func(t *testing.T) { + pv := &PropertyValue{ + ID: NewId(), + TargetID: PropertyValueSystemTargetID, + TargetType: PropertyValueTargetTypeChannel, + GroupID: NewId(), + FieldID: NewId(), + CreateAt: GetMillis(), + UpdateAt: GetMillis(), + } + require.Error(t, pv.IsValid()) + }) } func TestPropertyValueSearchCursor_IsValid(t *testing.T) { diff --git a/webapp/platform/client/src/client4.ts b/webapp/platform/client/src/client4.ts index 5fda93821b2..b4165d0e165 100644 --- a/webapp/platform/client/src/client4.ts +++ b/webapp/platform/client/src/client4.ts @@ -2151,6 +2151,20 @@ export default class Client4 { ); }; + getSystemPropertyValues = (groupName: string) => { + return this.doFetch>>( + `${this.getBaseRoute()}/properties/groups/${groupName}/system/values`, + {method: 'get'}, + ); + }; + + patchSystemPropertyValues = (groupName: string, items: Array<{field_id: string; value: T}>) => { + return this.doFetch>>( + `${this.getBaseRoute()}/properties/groups/${groupName}/system/values`, + {method: 'PATCH', body: JSON.stringify(items)}, + ); + }; + // Remote Clusters Routes getRemoteClusters = (options: {