diff --git a/e2e-tests/.ci/server.generate.sh b/e2e-tests/.ci/server.generate.sh index 3fc7a52bf0a..fbc3426ed40 100755 --- a/e2e-tests/.ci/server.generate.sh +++ b/e2e-tests/.ci/server.generate.sh @@ -70,6 +70,7 @@ services: MM_FEATUREFLAGS_CLASSIFICATIONMARKINGS: "true" MM_FEATUREFLAGS_INTEGRATEDBOARDS: "true" MM_FEATUREFLAGS_PROPERTYFIELDRANK: "true" + MM_FEATUREFLAGS_RESOURCEATTRIBUTESINPOLICIES: "true" MM_FEATUREFLAGS_ATTRIBUTEVALUEMASKING: "true" MM_FEATUREFLAGS_WYSIWYGEDITOR: "true" MM_FEATUREFLAGS_RECURRINGSCHEDULEDPOSTS: "true" diff --git a/e2e-tests/playwright/lib/src/containers/env_baseline.ts b/e2e-tests/playwright/lib/src/containers/env_baseline.ts index 638fdad1c07..f587a969ea4 100644 --- a/e2e-tests/playwright/lib/src/containers/env_baseline.ts +++ b/e2e-tests/playwright/lib/src/containers/env_baseline.ts @@ -22,6 +22,7 @@ export const SERVER_ENV_BASELINE: Record = { MM_FEATUREFLAGS_PERMISSIONPOLICIES: 'true', MM_FEATUREFLAGS_PROPERTYFIELDRANK: 'true', MM_FEATUREFLAGS_RECURRINGSCHEDULEDPOSTS: 'true', + MM_FEATUREFLAGS_RESOURCEATTRIBUTESINPOLICIES: 'true', MM_FEATUREFLAGS_TEAMMEMBERSHIPACCESSCONTROL: 'true', MM_FEATUREFLAGS_WYSIWYGEDITOR: 'true', }; diff --git a/e2e-tests/playwright/specs/functional/channels/team_settings/helpers.ts b/e2e-tests/playwright/specs/functional/channels/team_settings/helpers.ts index a705c6af139..342b3496974 100644 --- a/e2e-tests/playwright/specs/functional/channels/team_settings/helpers.ts +++ b/e2e-tests/playwright/specs/functional/channels/team_settings/helpers.ts @@ -165,7 +165,7 @@ export async function setUserAttribute(adminClient: Client4, userId: string, fie * * Why this exists: ABAC queries (validateExpressionAgainstRequester, * calculateMembershipChanges) read from a Postgres materialized view - * (`AttributeView`). The enterprise access-control service refreshes that view + * (`UserAttributeView`). The enterprise access-control service refreshes that view * at most once every 30 seconds — so freshly-written CPA values are not visible * until the next refresh tick. A test that writes a brand-new CPA value and * then immediately clicks "Save" on a rule referencing that value will hit @@ -206,7 +206,7 @@ export async function waitForAttributeViewToInclude( } const missing = expectedUserIds.filter((id) => !lastSeen.has(id)); throw new Error( - `AttributeView did not include users [${missing.join(', ')}] for expression "${expression}" within ${timeoutMs}ms`, + `UserAttributeView did not include users [${missing.join(', ')}] for expression "${expression}" within ${timeoutMs}ms`, ); } @@ -240,7 +240,7 @@ export async function waitForAttributeViewToExclude( } const stillPresent = excludedUserIds.filter((id) => lastSeen.has(id)); throw new Error( - `AttributeView still includes users [${stillPresent.join(', ')}] for expression "${expression}" after ${timeoutMs}ms`, + `UserAttributeView still includes users [${stillPresent.join(', ')}] for expression "${expression}" after ${timeoutMs}ms`, ); } diff --git a/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/authoring.spec.ts b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/authoring.spec.ts new file mode 100644 index 00000000000..8f776144dbb --- /dev/null +++ b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/authoring.spec.ts @@ -0,0 +1,87 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {expect, test} from '@mattermost/playwright-lib'; + +import {enableUserManagedAttributes} from '../support'; +import {enableTeamMembershipPolicies} from '../teams/helpers'; + +import {createChannelTextField, createParentPolicyViaAPI, expectAssignTeamsDenied} from './helpers'; + +/** + * Authoring round-trip for resource.attributes.* over the real HTTP boundary + * (Playwright drives a live server with the enterprise engine, so SavePolicy / + * cel-check validation runs for real — the api4 Go tests mock the engine and + * cannot). + * + * One representative case per rule; the exhaustive save-validation matrix + * (multiselect reject, rank scale-match, permission-policy accept) lives in the + * enterprise engine unit tests. + */ +test.describe('ABAC resource.attributes - authoring', {tag: ['@abac', '@abac_resource_attributes']}, () => { + test('accepts a parent policy mixing user and resource attributes', async ({pw}) => { + await pw.skipIfNoLicense(); + await pw.skipIfFeatureFlagNotSet('ResourceAttributesInPolicies', true); + + const {adminClient} = await pw.initSetup(); + await enableUserManagedAttributes(adminClient); + await adminClient.patchConfig({ + AccessControlSettings: {EnableAttributeBasedAccessControl: true}, + } as Parameters[0]); + + const attr = `region${pw.random.id()}`; + await createChannelTextField(adminClient, attr); + + // Save succeeds and returns a policy id — the round-trip accepts a + // mixed user/resource expression on a parent policy. + const policyId = await createParentPolicyViaAPI(adminClient, { + name: `Accept Resource ${pw.random.id()}`, + expression: `resource.attributes.${attr} == "us"`, + }); + expect(policyId).toBeTruthy(); + }); + + test('rejects has(resource.attributes.*) at check time', async ({pw}) => { + await pw.skipIfNoLicense(); + await pw.skipIfFeatureFlagNotSet('ResourceAttributesInPolicies', true); + + const {adminClient} = await pw.initSetup(); + await enableUserManagedAttributes(adminClient); + await adminClient.patchConfig({ + AccessControlSettings: {EnableAttributeBasedAccessControl: true}, + } as Parameters[0]); + + const attr = `region${pw.random.id()}`; + await createChannelTextField(adminClient, attr); + + // Absence is handled by deny-on-miss, so has() guards on resource + // attributes are rejected. cel/check surfaces the error to the editor. + // Assert the reason, not just that validation failed: a bare count also + // passes on an unrelated compile or engine error, and would have kept + // passing while the feature flag denied every resource reference. + const errors = await adminClient.checkAccessControlExpression(`has(resource.attributes.${attr})`); + expect(errors.length).toBeGreaterThan(0); + expect(errors[0].message).toContain('has() is not supported on resource attributes'); + }); + + test('rejects assigning a resource parent to a team', async ({pw}) => { + await pw.skipIfNoLicense(); + await pw.skipIfFeatureFlagNotSet('ResourceAttributesInPolicies', true); + + const {adminClient, team} = await pw.initSetup(); + await enableUserManagedAttributes(adminClient); + await enableTeamMembershipPolicies(adminClient); + + const attr = `region${pw.random.id()}`; + await createChannelTextField(adminClient, attr); + + const policyId = await createParentPolicyViaAPI(adminClient, { + name: `Team Boundary ${pw.random.id()}`, + expression: `resource.attributes.${attr} == "us"`, + }); + + // A team's resource is a team, which has no CPA attributes, so a parent + // that references resource.attributes.* must not be importable by a team. + await expectAssignTeamsDenied(adminClient, policyId, [team.id]); + }); +}); diff --git a/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/helpers.ts b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/helpers.ts new file mode 100644 index 00000000000..d73f4ac0713 --- /dev/null +++ b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/helpers.ts @@ -0,0 +1,295 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {expect, type Page} from '@playwright/test'; +import type {Client4} from '@mattermost/client'; + +/** + * Helpers for exercising resource.attributes.* (channel custom profile + * attributes) end to end. The subject of a policy stays the user; these helpers + * provision the *resource* side — channel-object-type CPA fields in the + * access_control group and per-channel values — plus API-driven policy authoring + * so a spec can exercise the resource side without also driving the policy + * editor UI. + * + * The server under test must run with MM_FEATUREFLAGS_RESOURCEATTRIBUTESINPOLICIES + * set: the feature is flag-gated, and a feature flag cannot be turned on through + * the config API (the config store restores flags on write), so no amount of + * patchConfig in a spec substitutes for the environment variable. Both CI paths + * set it — SERVER_ENV_BASELINE for the testcontainers stack the Playwright suite + * runs on, and e2e-tests/.ci/server.generate.sh for the docker-compose one — and + * every spec here guards with skipIfFeatureFlagNotSet so a server without it + * skips rather than failing on a policy save the server refuses. + */ + +const PROPERTY_GROUP = 'access_control'; +const CHANNEL_OBJECT_TYPE = 'channel'; +const USER_OBJECT_TYPE = 'user'; +const TEMPLATE_OBJECT_TYPE = 'template'; + +/** + * Create a channel-object-type text CPA field in the access_control group. The + * ABAC materialized view surfaces it as resource.attributes.. Marked + * admin-managed so SavePolicy's name normalization accepts a reference to it + * (channel fields have no user-managed-attributes toggle). Returns the field id. + */ +export async function createChannelTextField(adminClient: Client4, name: string): Promise { + const field = await adminClient.createPropertyField(PROPERTY_GROUP, CHANNEL_OBJECT_TYPE, { + name, + type: 'text', + target_type: 'system', + target_id: '', + attrs: {managed: 'admin'}, + } as Parameters[2]); + return field.id; +} + +/** + * Set a single channel's value for a channel CPA field. A refresh-before-read + * is not needed for the sync lane (the sync job reads a freshly refreshed + * matview), but note the matview is refreshed on a timer, so set values before + * triggering the sync job. + */ +export async function setChannelAttributeValue( + adminClient: Client4, + channelId: string, + fieldId: string, + value: string, +): Promise { + await adminClient.patchPropertyValues(PROPERTY_GROUP, CHANNEL_OBJECT_TYPE, channelId, [{field_id: fieldId, value}]); +} + +type ParentPolicyOptions = { + name: string; + expression: string; + version?: string; +}; + +/** + * Create a parent membership policy via the REST API. A parent is the reusable + * rule-carrier: assign channels to it (assignChannelsToPolicy) to put those + * channels under its rules. Returns the created policy id. + */ +export async function createParentPolicyViaAPI(adminClient: Client4, opts: ParentPolicyOptions): Promise { + // The version sent here is advisory: CreateOrUpdateAccessControlPolicy + // overwrites it (v0.3, bumped to v0.4 only for permission-action rules), and + // no version gates resource.attributes.* — that's the + // ResourceAttributesInPolicies flag plus the "not a team policy" rule. + const policy = await adminClient.updateOrCreateAccessControlPolicy({ + id: '', + name: opts.name, + type: 'parent', + version: opts.version ?? 'v0.3', + revision: 0, + active: true, + rules: [{expression: opts.expression, actions: ['membership']}], + }); + return policy.id; +} + +/** + * Assign channels to a parent policy (creates the per-channel child policy that + * imports the parent and flips enforcement on). Mirrors the System Console + * "Add channels" flow. + */ +export async function assignChannelsToPolicy( + adminClient: Client4, + policyId: string, + channelIds: string[], +): Promise { + // The /assign endpoint returns 200 with an empty body but a JSON content + // type, which Client4.doFetch chokes on ("Unexpected end of JSON input"). + // Use a raw request and check status, matching the pattern the rest of the + // ABAC e2e helpers use for these no-body endpoints. + const res = await fetch(`${adminClient.getBaseRoute()}/access_control_policies/${policyId}/assign`, { + method: 'POST', + headers: {'Content-Type': 'application/json', Authorization: `Bearer ${adminClient.getToken()}`}, + body: JSON.stringify({channel_ids: channelIds}), + }); + if (!res.ok) { + throw new Error(`assign channels failed: ${res.status} ${await res.text()}`); + } +} + +/** + * Trigger an access_control_sync job. Pass a policyId to target the channels + * governed by that policy; omit it for a global sweep. Returns the job id. + */ +export async function triggerSyncJob(adminClient: Client4, policyId?: string): Promise { + const job = await adminClient.createAccessControlSyncJob(policyId ? {policy_id: policyId} : {}); + return job.id; +} + +export type MultiselectScale = { + templateId: string; + userFieldId: string; + userFieldName: string; + channelFieldId: string; + channelFieldName: string; + // Server-assigned option ids keyed by option name. Both the user and channel + // fields inherit these exact ids from the template (that's what "shared option + // scale" means), so hasAnyOf/hasAllOf compares the same id space on both sides. + optionIds: Record; +}; + +/** + * Provision a shared-scale multiselect setup for a list-vs-list (hasAnyOf / + * hasAllOf) rule: a template multiselect field plus a user field and a channel + * field that both link to it via linked_field_id. All three live in the + * access_control group — a user CPA field in the custom_profile_attributes group + * cannot carry a linked_field_id (the server rejects it), and the scale-match + * validation requires the receiver (user) and argument (channel) fields to share + * a non-nil, equal linked_field_id. The linked user field still surfaces as + * user.attributes. to the engine. Returns the field ids/names and the + * option ids (identical across all three fields). + */ +export async function createLinkedMultiselectScale( + adminClient: Client4, + baseName: string, + optionNames: string[], +): Promise { + const template = await adminClient.createPropertyField(PROPERTY_GROUP, TEMPLATE_OBJECT_TYPE, { + name: `${baseName}_tmpl`, + type: 'multiselect', + target_type: 'system', + target_id: '', + attrs: {options: optionNames.map((name) => ({id: '', name, color: '#0000ff'}))}, + } as Parameters[2]); + + const userFieldName = `${baseName}_user`; + const userField = await adminClient.createPropertyField(PROPERTY_GROUP, USER_OBJECT_TYPE, { + name: userFieldName, + type: 'multiselect', + target_type: 'system', + target_id: '', + linked_field_id: template.id, + } as Parameters[2]); + + const channelFieldName = `${baseName}_chan`; + const channelField = await adminClient.createPropertyField(PROPERTY_GROUP, CHANNEL_OBJECT_TYPE, { + name: channelFieldName, + type: 'multiselect', + target_type: 'system', + target_id: '', + linked_field_id: template.id, + } as Parameters[2]); + + const optionIds: Record = {}; + for (const opt of (template.attrs?.options ?? []) as Array<{id: string; name: string}>) { + optionIds[opt.name] = opt.id; + } + + return { + templateId: template.id, + userFieldId: userField.id, + userFieldName, + channelFieldId: channelField.id, + channelFieldName, + optionIds, + }; +} + +/** + * Set a user's multiselect value (list of option ids) for an access_control-group + * user field. Multiselect values are stored as option ids, not names. + */ +export async function setUserMultiselectValue( + adminClient: Client4, + userId: string, + fieldId: string, + optionIds: string[], +): Promise { + await adminClient.patchPropertyValues(PROPERTY_GROUP, USER_OBJECT_TYPE, userId, [ + {field_id: fieldId, value: optionIds}, + ]); +} + +export async function setChannelMultiselectValue( + adminClient: Client4, + channelId: string, + fieldId: string, + optionIds: string[], +): Promise { + await adminClient.patchPropertyValues(PROPERTY_GROUP, CHANNEL_OBJECT_TYPE, channelId, [ + {field_id: fieldId, value: optionIds}, + ]); +} + +/** + * Open an existing membership policy in the System Console editor by id. The + * parent-policy editor page carries the rule table and the Test-access-rule + * button, so UI specs provision the policy over the API and drive only the + * feature under test here. + */ +export async function openPolicyEditor(page: Page, policyId: string): Promise { + await page.goto(`/admin_console/system_attributes/membership_policies/edit_policy/${policyId}`); + await page.waitForLoadState('networkidle'); +} + +type ApiError = {status_code?: number; server_error_id?: string}; + +/** + * Run an API call that the server is expected to refuse, and assert it failed + * for the named reason. A bare `try { … } catch {}` proves only that something + * threw, so it passes just as happily on a 500, a transport fault, or a gating + * rejection that never reached the rule under test — which is how these specs + * once passed with the feature flag off. + */ +async function expectRejection( + call: () => Promise, + expected: {statusCode: number; serverErrorId: string}, + because: string, +): Promise { + let error: ApiError | undefined; + try { + await call(); + } catch (err) { + error = err as ApiError; + } + expect(error, `expected the server to refuse: ${because}`).toBeDefined(); + expect(error?.status_code, because).toBe(expected.statusCode); + expect(error?.server_error_id, because).toBe(expected.serverErrorId); +} + +/** + * Assert the runtime PDP refuses to add a user to a channel. The "user is not in + * the channel" check that usually follows can pass on its own, because the sync + * job already removed them — only this establishes that the add was refused. + */ +export async function expectAddToChannelDenied(adminClient: Client4, userId: string, channelId: string): Promise { + await expectRejection( + () => adminClient.addToChannel(userId, channelId), + {statusCode: 403, serverErrorId: 'api.channel.add_user.to.channel.rejected'}, + 'the runtime PDP denies a non-matching user', + ); +} + +/** + * Assert assigning a parent policy to a team is refused. The team-boundary + * rejection carries its own error id, so pinning it rules out a permission, + * feature-gate or generic server error reaching this assertion instead. + */ +export async function expectAssignTeamsDenied( + adminClient: Client4, + policyId: string, + teamIds: string[], +): Promise { + await expectRejection( + () => adminClient.assignTeamsToAccessControlPolicy(policyId, teamIds), + {statusCode: 400, serverErrorId: 'app.pap.save_policy.team_resource_attributes'}, + 'a team cannot import a parent that references resource.attributes.*', + ); +} + +/** + * Assert a policy save is refused because its expression still carries the + * masked-value sentinel. That rejection has its own error id, distinguishing + * it from other save failures rather than reporting only the general one. + */ +export async function expectMaskedTokenRejected(adminClient: Client4, opts: ParentPolicyOptions): Promise { + await expectRejection( + () => createParentPolicyViaAPI(adminClient, opts), + {statusCode: 400, serverErrorId: 'app.pap.save_policy.masked_token_in_expression'}, + 'a masked sentinel cannot be resolved to a stored value', + ); +} diff --git a/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/masking.spec.ts b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/masking.spec.ts new file mode 100644 index 00000000000..61f125a819f --- /dev/null +++ b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/masking.spec.ts @@ -0,0 +1,54 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {test} from '@mattermost/playwright-lib'; + +import {enableUserManagedAttributes} from '../support'; + +import {createChannelTextField, expectMaskedTokenRejected} from './helpers'; + +/** + * Attribute-value masking on the resource.attributes.* write path. + * + * The masked-value sentinel ("--------", model.MaskingTokenValue) is a + * response-only placeholder the server substitutes for literals a caller can't + * see. It must never round-trip back into storage: saving a policy whose + * resource.attributes.* condition still carries the sentinel is rejected. This + * is the write-path half of the symmetric masking gate, asserted over HTTP. + * + * The read-path (caller-relative redaction of resource literals in GET / + * visual_ast / simulate) reuses the shared-template bridge and protected / + * shared_only fields, which require direct DB provisioning; that path is covered + * by the app-layer resolver test (TestAppMaskingResolver_ChannelFieldUsesUserHoldings), + * the CEL-walker/visual-AST masking tests, and the existing user-attribute + * masking Playwright suite whose mechanics the resource path shares. + */ +test.describe('ABAC resource.attributes - masking write path', {tag: ['@abac', '@abac_masking']}, () => { + test('rejects saving a resource.attributes condition carrying the masked sentinel', async ({pw}) => { + await pw.skipIfNoLicense(); + await pw.skipIfFeatureFlagNotSet('ResourceAttributesInPolicies', true); + + // The sentinel rejection under test lives inside the server's + // AttributeValueMasking branch, so the flag has to be on. It cannot be + // turned on from here (the config store restores feature flags on + // write), so guard rather than try to set it. + await pw.skipIfFeatureFlagNotSet('AttributeValueMasking', true); + + const {adminClient} = await pw.initSetup(); + await enableUserManagedAttributes(adminClient); + await adminClient.patchConfig({ + AccessControlSettings: {EnableAttributeBasedAccessControl: true}, + } as Parameters[0]); + + const attr = `region${pw.random.id()}`; + await createChannelTextField(adminClient, attr); + + // The 8-dash sentinel is server-generated and never a real value, so a + // submitted expression containing it cannot be resolved to a stored + // value and is rejected. + await expectMaskedTokenRejected(adminClient, { + name: `Masked Sentinel ${pw.random.id()}`, + expression: `resource.attributes.${attr} == "--------"`, + }); + }); +}); diff --git a/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/membership_sync.spec.ts b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/membership_sync.spec.ts new file mode 100644 index 00000000000..8254c147636 --- /dev/null +++ b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/membership_sync.spec.ts @@ -0,0 +1,143 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {expect, test, verifyUserInChannel} from '@mattermost/playwright-lib'; + +import type {CustomProfileAttribute} from '../../../channels/custom_profile_attributes/helpers'; +import {setupCustomProfileAttributeFields} from '../../../channels/custom_profile_attributes/helpers'; +import { + createPrivateChannelForABAC, + createUserForABAC, + enableUserManagedAttributes, + waitForPolicySyncJob, +} from '../support'; + +import { + assignChannelsToPolicy, + createChannelTextField, + createParentPolicyViaAPI, + expectAddToChannelDenied, + setChannelAttributeValue, + triggerSyncJob, +} from './helpers'; + +/** + * resource.attributes.* membership sync + enforcement (lane agreement). + * + * A policy that mixes the requesting user's attribute with the accessed + * channel's attribute (user.attributes. == resource.attributes.) must: + * - remove only the members whose value differs from the channel's (SQL sync lane), + * - let an admin add a matching user but block a non-matching one (runtime PDP lane), + * - remove everyone when the channel doesn't set the referenced attribute (deny-on-miss). + * + * Authoring/sync are driven through the REST API so the tests don't depend on + * the table-editor's RHS constraints. + */ +test.describe('ABAC resource.attributes - membership sync', {tag: ['@abac', '@abac_resource_attributes']}, () => { + test('mixed user/resource scalar policy syncs and enforces joins', async ({pw}) => { + test.setTimeout(120000); + await pw.skipIfNoLicense(); + await pw.skipIfFeatureFlagNotSet('ResourceAttributesInPolicies', true); + + const {adminClient, team} = await pw.initSetup(); + await enableUserManagedAttributes(adminClient); + await adminClient.patchConfig({ + AccessControlSettings: {EnableAttributeBasedAccessControl: true}, + } as Parameters[0]); + + // Same field name on both object types → user.attributes. compared + // to resource.attributes.. + const attr = `region${pw.random.id()}`; + const userAttribute: CustomProfileAttribute[] = [{name: attr, type: 'text', value: ''}]; + const attributeFieldsMap = await setupCustomProfileAttributeFields(adminClient, userAttribute); + const channelFieldId = await createChannelTextField(adminClient, attr); + + const matchingUserNotInChannel = await createUserForABAC(adminClient, attributeFieldsMap, [ + {name: attr, type: 'text', value: 'us'}, + ]); + const matchingUserInChannel = await createUserForABAC(adminClient, attributeFieldsMap, [ + {name: attr, type: 'text', value: 'us'}, + ]); + const nonMatchingUserInChannel = await createUserForABAC(adminClient, attributeFieldsMap, [ + {name: attr, type: 'text', value: 'eu'}, + ]); + for (const u of [matchingUserNotInChannel, matchingUserInChannel, nonMatchingUserInChannel]) { + await adminClient.addToTeam(team.id, u.id); + } + + const channel = await createPrivateChannelForABAC(adminClient, team.id); + await setChannelAttributeValue(adminClient, channel.id, channelFieldId, 'us'); + await adminClient.addToChannel(matchingUserInChannel.id, channel.id); + await adminClient.addToChannel(nonMatchingUserInChannel.id, channel.id); + + const policyId = await createParentPolicyViaAPI(adminClient, { + name: `Resource Region ${pw.random.id()}`, + expression: `user.attributes.${attr} == resource.attributes.${attr}`, + }); + await assignChannelsToPolicy(adminClient, policyId, [channel.id]); + + await triggerSyncJob(adminClient, policyId); + await waitForPolicySyncJob(adminClient, policyId); + + // SQL sync lane on a private channel with an active policy: + // - the non-matching (eu) member is removed, + // - the matching (us) member stays, + // - the matching (us) user who was only in the team is auto-added — + // an active private-channel policy pulls in matching team members. + expect(await verifyUserInChannel(adminClient, matchingUserInChannel.id, channel.id)).toBe(true); + expect(await verifyUserInChannel(adminClient, nonMatchingUserInChannel.id, channel.id)).toBe(false); + expect(await verifyUserInChannel(adminClient, matchingUserNotInChannel.id, channel.id)).toBe(true); + + // Runtime lane agrees with the sync result: re-adding the matching user + // succeeds, while adding the non-matching user is blocked. + await adminClient.addToChannel(matchingUserNotInChannel.id, channel.id); + expect(await verifyUserInChannel(adminClient, matchingUserNotInChannel.id, channel.id)).toBe(true); + + await expectAddToChannelDenied(adminClient, nonMatchingUserInChannel.id, channel.id); + expect(await verifyUserInChannel(adminClient, nonMatchingUserInChannel.id, channel.id)).toBe(false); + }); + + test('deny-on-miss removes all members when the channel attribute is absent', async ({pw}) => { + test.setTimeout(120000); + await pw.skipIfNoLicense(); + await pw.skipIfFeatureFlagNotSet('ResourceAttributesInPolicies', true); + + const {adminClient, team} = await pw.initSetup(); + await enableUserManagedAttributes(adminClient); + await adminClient.patchConfig({ + AccessControlSettings: {EnableAttributeBasedAccessControl: true}, + } as Parameters[0]); + + const attr = `region${pw.random.id()}`; + const attributeFieldsMap = await setupCustomProfileAttributeFields(adminClient, [ + {name: attr, type: 'text', value: ''}, + ]); + + // Channel field exists so the reference resolves at save time, but the + // channel below never sets a value → the referenced field is missing for + // that channel → deny-on-miss. + await createChannelTextField(adminClient, attr); + + const member = await createUserForABAC(adminClient, attributeFieldsMap, [ + {name: attr, type: 'text', value: 'us'}, + ]); + await adminClient.addToTeam(team.id, member.id); + + const channel = await createPrivateChannelForABAC(adminClient, team.id); + await adminClient.addToChannel(member.id, channel.id); + expect(await verifyUserInChannel(adminClient, member.id, channel.id)).toBe(true); + + const policyId = await createParentPolicyViaAPI(adminClient, { + name: `Resource DenyOnMiss ${pw.random.id()}`, + expression: `user.attributes.${attr} == resource.attributes.${attr}`, + }); + await assignChannelsToPolicy(adminClient, policyId, [channel.id]); + + await triggerSyncJob(adminClient, policyId); + await waitForPolicySyncJob(adminClient, policyId); + + // The channel is missing the referenced attribute, so the whole policy + // denies and every governed member is removed — fail-secure. + expect(await verifyUserInChannel(adminClient, member.id, channel.id)).toBe(false); + }); +}); diff --git a/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/multiselect_sync.spec.ts b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/multiselect_sync.spec.ts new file mode 100644 index 00000000000..bd360a69145 --- /dev/null +++ b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/multiselect_sync.spec.ts @@ -0,0 +1,156 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {expect, test, verifyUserInChannel} from '@mattermost/playwright-lib'; + +import { + createPrivateChannelForABAC, + createUserForABAC, + enableUserManagedAttributes, + waitForPolicySyncJob, +} from '../support'; + +import { + assignChannelsToPolicy, + createLinkedMultiselectScale, + createParentPolicyViaAPI, + expectAddToChannelDenied, + setChannelMultiselectValue, + setUserMultiselectValue, + triggerSyncJob, +} from './helpers'; + +/** + * List-vs-list (multiselect) resource targets end to end: a rule comparing a + * multiselect user attribute against a multiselect channel attribute with + * hasAnyOf / hasAllOf, authored here over the real HTTP boundary. Both fields + * link to a shared template (equal linked_field_id) so they share one option-id + * scale — the only shape the save-time validation accepts. + * + * Each test asserts the two evaluation lanes agree on the same fixtures: + * - SQL sync lane: a non-matching member is removed, a matching member stays, + * and a matching team-only user is auto-added to the assigned private channel. + * - Runtime PDP lane: re-adding a matching user succeeds; adding a non-matching + * user is blocked. + * + * hasAnyOf = the lists intersect. hasAllOf = the channel's list is a subset of + * the user's (the user holds every value the channel requires). + */ +test.describe('ABAC resource.attributes - multiselect targets', {tag: ['@abac', '@abac_resource_attributes']}, () => { + test('has any of syncs and enforces on list intersection', async ({pw}) => { + test.setTimeout(120000); + await pw.skipIfNoLicense(); + await pw.skipIfFeatureFlagNotSet('ResourceAttributesInPolicies', true); + + const {adminClient, team} = await pw.initSetup(); + await enableUserManagedAttributes(adminClient); + await adminClient.patchConfig({ + AccessControlSettings: {EnableAttributeBasedAccessControl: true}, + } as Parameters[0]); + + const scale = await createLinkedMultiselectScale(adminClient, `region${pw.random.id()}`, [ + 'alpha', + 'beta', + 'gamma', + ]); + const {alpha, beta, gamma} = scale.optionIds; + + // Channel requires [alpha, beta]. has-any-of matches a user sharing >=1. + const matchInChannel = await createUserForABAC(adminClient, {}, []); // [beta] -> shares beta + const matchTeamOnly = await createUserForABAC(adminClient, {}, []); // [alpha] -> shares alpha + const nonMatch = await createUserForABAC(adminClient, {}, []); // [gamma] -> disjoint + await setUserMultiselectValue(adminClient, matchInChannel.id, scale.userFieldId, [beta]); + await setUserMultiselectValue(adminClient, matchTeamOnly.id, scale.userFieldId, [alpha]); + await setUserMultiselectValue(adminClient, nonMatch.id, scale.userFieldId, [gamma]); + for (const u of [matchInChannel, matchTeamOnly, nonMatch]) { + await adminClient.addToTeam(team.id, u.id); + } + + const channel = await createPrivateChannelForABAC(adminClient, team.id); + await setChannelMultiselectValue(adminClient, channel.id, scale.channelFieldId, [alpha, beta]); + await adminClient.addToChannel(matchInChannel.id, channel.id); + await adminClient.addToChannel(nonMatch.id, channel.id); + + const policyId = await createParentPolicyViaAPI(adminClient, { + name: `HasAnyOf ${pw.random.id()}`, + expression: `user.attributes.${scale.userFieldName}.hasAnyOf(resource.attributes.${scale.channelFieldName})`, + }); + await assignChannelsToPolicy(adminClient, policyId, [channel.id]); + + await triggerSyncJob(adminClient, policyId); + await waitForPolicySyncJob(adminClient, policyId); + + // SQL sync lane: the disjoint member is removed, the intersecting member + // stays, and the intersecting team-only user is auto-added — an active + // private-channel policy pulls in matching team members. + expect(await verifyUserInChannel(adminClient, matchInChannel.id, channel.id)).toBe(true); + expect(await verifyUserInChannel(adminClient, nonMatch.id, channel.id)).toBe(false); + expect(await verifyUserInChannel(adminClient, matchTeamOnly.id, channel.id)).toBe(true); + + // Runtime PDP lane agrees with the sync verdict: re-adding the + // intersecting user succeeds, while the disjoint one is blocked. + await adminClient.addToChannel(matchTeamOnly.id, channel.id); + expect(await verifyUserInChannel(adminClient, matchTeamOnly.id, channel.id)).toBe(true); + await expectAddToChannelDenied(adminClient, nonMatch.id, channel.id); + expect(await verifyUserInChannel(adminClient, nonMatch.id, channel.id)).toBe(false); + }); + + test('has all of syncs and enforces on channel-list subset', async ({pw}) => { + test.setTimeout(120000); + await pw.skipIfNoLicense(); + await pw.skipIfFeatureFlagNotSet('ResourceAttributesInPolicies', true); + + const {adminClient, team} = await pw.initSetup(); + await enableUserManagedAttributes(adminClient); + await adminClient.patchConfig({ + AccessControlSettings: {EnableAttributeBasedAccessControl: true}, + } as Parameters[0]); + + const scale = await createLinkedMultiselectScale(adminClient, `clr${pw.random.id()}`, [ + 'alpha', + 'beta', + 'gamma', + ]); + const {alpha, beta, gamma} = scale.optionIds; + + // Channel requires [alpha, beta]. has-all-of matches only a user holding + // BOTH — a user with just alpha does not (missing beta). + const matchInChannel = await createUserForABAC(adminClient, {}, []); // [alpha, beta, gamma] superset + const matchTeamOnly = await createUserForABAC(adminClient, {}, []); // [alpha, beta] exact + const nonMatch = await createUserForABAC(adminClient, {}, []); // [alpha] missing beta + await setUserMultiselectValue(adminClient, matchInChannel.id, scale.userFieldId, [alpha, beta, gamma]); + await setUserMultiselectValue(adminClient, matchTeamOnly.id, scale.userFieldId, [alpha, beta]); + await setUserMultiselectValue(adminClient, nonMatch.id, scale.userFieldId, [alpha]); + for (const u of [matchInChannel, matchTeamOnly, nonMatch]) { + await adminClient.addToTeam(team.id, u.id); + } + + const channel = await createPrivateChannelForABAC(adminClient, team.id); + await setChannelMultiselectValue(adminClient, channel.id, scale.channelFieldId, [alpha, beta]); + await adminClient.addToChannel(matchInChannel.id, channel.id); + await adminClient.addToChannel(nonMatch.id, channel.id); + + const policyId = await createParentPolicyViaAPI(adminClient, { + name: `HasAllOf ${pw.random.id()}`, + expression: `user.attributes.${scale.userFieldName}.hasAllOf(resource.attributes.${scale.channelFieldName})`, + }); + await assignChannelsToPolicy(adminClient, policyId, [channel.id]); + + await triggerSyncJob(adminClient, policyId); + await waitForPolicySyncJob(adminClient, policyId); + + // SQL sync lane: the superset member stays, the member missing a required + // value is removed, and the exact-match team-only user is auto-added — an + // active private-channel policy pulls in matching team members. + expect(await verifyUserInChannel(adminClient, matchInChannel.id, channel.id)).toBe(true); + expect(await verifyUserInChannel(adminClient, nonMatch.id, channel.id)).toBe(false); + expect(await verifyUserInChannel(adminClient, matchTeamOnly.id, channel.id)).toBe(true); + + // Runtime PDP lane agrees: re-adding the exact-match user succeeds, the + // subset-missing user is blocked. + await adminClient.addToChannel(matchTeamOnly.id, channel.id); + expect(await verifyUserInChannel(adminClient, matchTeamOnly.id, channel.id)).toBe(true); + await expectAddToChannelDenied(adminClient, nonMatch.id, channel.id); + expect(await verifyUserInChannel(adminClient, nonMatch.id, channel.id)).toBe(false); + }); +}); diff --git a/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/test_picker.spec.ts b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/test_picker.spec.ts new file mode 100644 index 00000000000..72ea69bead3 --- /dev/null +++ b/e2e-tests/playwright/specs/functional/system_console/abac/resource_attributes/test_picker.spec.ts @@ -0,0 +1,112 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {expect, test} from '@mattermost/playwright-lib'; + +import {setupCustomProfileAttributeFields} from '../../../channels/custom_profile_attributes/helpers'; +import { + assertAccessControlAutocompleteContains, + createPrivateChannelForABAC, + createUserForABAC, + enableUserManagedAttributes, +} from '../support'; + +import {createChannelTextField, createParentPolicyViaAPI, openPolicyEditor, setChannelAttributeValue} from './helpers'; + +/** + * Test-matching-users channel picker end to end. + * + * A parent policy that references resource.attributes.* has no channel scope of + * its own, so "Test access rule" cannot resolve the rule until the admin picks a + * concrete channel. The shared test modal therefore opens a channel-picker step + * first; the picked channel's attribute values are threaded into the matching-users + * query. This asserts that flow over the real UI + HTTP: the picker finds a + * channel by name, choosing it resolves the rule against that channel (a matching + * user appears, a non-matching one does not), and the back arrow returns to the + * picker. + * + * The matching-users query reads the attribute materialized view, refreshed on a + * throttled (~30s) cadence, so the first read after setting values may lag — the + * matching assertion re-searches until the view catches up. + */ +test.describe('ABAC resource.attributes - test picker', {tag: ['@abac', '@abac_resource_attributes']}, () => { + test('picker resolves a resource rule against the chosen channel', async ({pw}) => { + test.setTimeout(120000); + await pw.skipIfNoLicense(); + await pw.skipIfFeatureFlagNotSet('ResourceAttributesInPolicies', true); + + const {adminUser, adminClient, team} = await pw.initSetup(); + await enableUserManagedAttributes(adminClient); + await adminClient.patchConfig({ + AccessControlSettings: {EnableAttributeBasedAccessControl: true}, + } as Parameters[0]); + + // Same field name on both object types so user.attributes. compares + // to resource.attributes.. + const attr = `region${pw.random.id()}`; + const fieldsMap = await setupCustomProfileAttributeFields(adminClient, [{name: attr, type: 'text', value: ''}]); + const channelFieldId = await createChannelTextField(adminClient, attr); + + const userUS = await createUserForABAC(adminClient, fieldsMap, [{name: attr, type: 'text', value: 'us'}]); + const userEU = await createUserForABAC(adminClient, fieldsMap, [{name: attr, type: 'text', value: 'eu'}]); + await adminClient.addToTeam(team.id, userUS.id); + await adminClient.addToTeam(team.id, userEU.id); + + const channelUS = await createPrivateChannelForABAC(adminClient, team.id); + await setChannelAttributeValue(adminClient, channelUS.id, channelFieldId, 'us'); + + const policyId = await createParentPolicyViaAPI(adminClient, { + name: `Picker ${pw.random.id()}`, + expression: `user.attributes.${attr} == resource.attributes.${attr}`, + }); + + // Fail fast if the editor won't see the attribute (keeps the Test button disabled). + await assertAccessControlAutocompleteContains(adminClient, [attr]); + + const {systemConsolePage} = await pw.testBrowser.login(adminUser); + const page = systemConsolePage.page; + await openPolicyEditor(page, policyId); + + const testButton = page.getByRole('button', {name: /test access rule/i}); + await expect(testButton).toBeVisible({timeout: 10000}); + await expect(testButton).toBeEnabled({timeout: 15000}); + await testButton.click(); + + const modal = page.locator('.TestResultsModal'); + + // Picker step: no channel scope on the parent, so the picker precedes the + // members list. + await expect(modal.getByText('Select a channel to test against')).toBeVisible({timeout: 10000}); + const channelSearch = modal.locator('.TestChannelPicker__search-input'); + await channelSearch.fill(channelUS.display_name); + const channelRow = modal.locator('.TestChannelPicker__row', {hasText: channelUS.display_name}); + await expect(channelRow).toBeVisible({timeout: 10000}); + await channelRow.click(); + + // Members step: the rule now resolves against channelUS (region == "us"). + await expect(modal.getByText('Access Rule Test Results')).toBeVisible({timeout: 10000}); + await expect(modal.locator('.TestResultsModal__back')).toBeVisible(); + + const memberSearch = modal.locator('input[placeholder*="Search" i]').first(); + await expect(async () => { + await memberSearch.fill(userUS.username); + await expect(modal.locator('.more-modal__name', {hasText: userUS.username})).toBeVisible({timeout: 3000}); + }).toPass({timeout: 60000, intervals: [3000]}); + + // The non-matching user (region == "eu") is not admitted against this channel. + // Wait for the search round-trip before asserting absence: the previous + // row is cleared the moment the term changes, so toHaveCount(0) would + // pass on that intermediate empty render even if userEU were admitted. + const euSearch = page.waitForResponse( + (r) => r.url().includes('/access_control_policies/cel/test') && r.request().method() === 'POST', + {timeout: 15000}, + ); + await memberSearch.fill(userEU.username); + await euSearch; + await expect(modal.locator('.more-modal__name', {hasText: userEU.username})).toHaveCount(0); + + // The back arrow returns to the picker (only present because it preceded). + await modal.locator('.TestResultsModal__back').click(); + await expect(modal.getByText('Select a channel to test against')).toBeVisible({timeout: 10000}); + }); +}); diff --git a/e2e-tests/playwright/specs/functional/system_console/abac/support.ts b/e2e-tests/playwright/specs/functional/system_console/abac/support.ts index 5d9617c8675..a9054ec2b26 100644 --- a/e2e-tests/playwright/specs/functional/system_console/abac/support.ts +++ b/e2e-tests/playwright/specs/functional/system_console/abac/support.ts @@ -366,6 +366,69 @@ export async function createPrivateChannelForABAC(client: Client4, teamId: strin return channel; } +/** + * Fill a single-value condition in the table editor's value cell. + * + * A free-text attribute's value editor renders one of two ways depending on + * what comparison targets are available: + * - an always-visible inline input (.values-editor__simple-input), when there + * are no comparable channel-attribute targets; or + * - a dropdown (valueSelectorMenuButton) whose "Add value..." field is only + * revealed after the menu is opened, when channel attributes are offered as + * comparison targets. + * + * Since channel (resource) attributes are system-wide, whether the dropdown + * appears depends on data that other specs may have created on the shared + * server. Handle both variants so the helper is agnostic to that. + * + * An attribute that carries options also renders the dropdown, but with a + * filter field rather than "Add value..." — pick the option from the menu + * instead of calling this. + */ +export async function fillSingleConditionValue(page: Page, value: string): Promise { + const inlineInput = page.locator('.values-editor__simple-input').first(); + const menuButton = page.locator('[data-testid="valueSelectorMenuButton"]').first(); + + // Wait for whichever variant rendered after the operator was chosen. + await page + .locator('.values-editor__simple-input, [data-testid="valueSelectorMenuButton"]') + .first() + .waitFor({state: 'visible', timeout: 10000}); + + if (await inlineInput.isVisible().catch(() => false)) { + await inlineInput.fill(value); + await inlineInput.press('Tab'); // commit (onBlur) + await page.waitForTimeout(300); + return; + } + + // Dropdown variant: open the menu, then fill the "Add value..." field inside it. + // Match on the accessible name, not the placeholder: the menu autofocuses this + // input, and Input only sets a placeholder attribute while unfocused (the text + // moves into the floating legend), so a placeholder locator resolves to nothing. + await menuButton.click({force: true}); + const menuInput = page.locator('input[aria-label*="Add value" i], input[placeholder*="Add value" i]').first(); + + // A click that lands while a sibling menu (e.g. the operator selector the caller + // just used) is still closing is spent dismissing that menu instead, leaving this + // one shut. Re-click once rather than requiring every caller to pause first. The + // gate has to be a waitFor, not isVisible(), which returns immediately and would + // toggle the menu straight back shut. + try { + await menuInput.waitFor({state: 'visible', timeout: 3000}); + } catch { + await menuButton.click({force: true}); + await menuInput.waitFor({state: 'visible', timeout: 10000}); + } + await menuInput.fill(value); + + // Tab commits (input onBlur) and closes the menu (menu closeMenuOnTab), so + // the dropdown doesn't overlay later actions. Enter would commit but leave + // the menu open, and Escape is swallowed by the input's stopPropagation. + await menuInput.press('Tab'); + await page.waitForTimeout(300); +} + /** * Create basic policy using Table Editor (Simple mode) */ @@ -493,10 +556,7 @@ export async function createBasicPolicy( await page.waitForTimeout(300); } else { // Single-value operator - const valueInput = page.locator('.values-editor__simple-input, input[placeholder*="Add value" i]').first(); - await valueInput.waitFor({state: 'visible', timeout: 10000}); - await valueInput.fill(options.value); - await page.waitForTimeout(500); + await fillSingleConditionValue(page, options.value); } } // end if (clickedAddAttribute) diff --git a/e2e-tests/playwright/specs/functional/system_console/abac/user_attributes/display_name_in_selector.spec.ts b/e2e-tests/playwright/specs/functional/system_console/abac/user_attributes/display_name_in_selector.spec.ts index 67efd098668..c0d626b3667 100644 --- a/e2e-tests/playwright/specs/functional/system_console/abac/user_attributes/display_name_in_selector.spec.ts +++ b/e2e-tests/playwright/specs/functional/system_console/abac/user_attributes/display_name_in_selector.spec.ts @@ -11,7 +11,7 @@ import { deleteCustomProfileAttributes, setupCustomProfileAttributeFields, } from '../../../channels/custom_profile_attributes/helpers'; -import {getPolicyIdByName} from '../support'; +import {fillSingleConditionValue, getPolicyIdByName} from '../support'; type FieldsMap = Record; @@ -154,10 +154,7 @@ test.describe('ABAC Attribute Selector - display_name rendering and filtering', await operatorMenu.waitFor({state: 'visible', timeout: 5000}); await operatorMenu.locator('li:has-text("is")').first().click(); - const valueInput = page.locator('.values-editor__simple-input').first(); - await valueInput.waitFor({state: 'visible', timeout: 10000}); - await valueInput.fill('engineering'); - await valueInput.press('Tab'); + await fillSingleConditionValue(page, 'engineering'); const saveButton = page.getByRole('button', {name: 'Save'}); await expect(saveButton).toBeEnabled({timeout: 10000}); diff --git a/server/channels/api4/access_control.go b/server/channels/api4/access_control.go index 7b4d6d18edb..3d4dd338d5f 100644 --- a/server/channels/api4/access_control.go +++ b/server/channels/api4/access_control.go @@ -426,6 +426,9 @@ func testExpression(c *Context, w http.ResponseWriter, r *http.Request) { Cursor: model.SubjectCursor{ TargetID: checkExpressionRequest.After, }, + // Carry the channel so a resource.attributes.* expression resolves + // against that channel's values; ignored for resource-free expressions. + ResourceID: channelId, } // Scope results to a team's current members only for callers authorized on @@ -1310,6 +1313,14 @@ func getFieldsAutocomplete(c *Context, w http.ResponseWriter, r *http.Request) { teamID := r.URL.Query().Get("team_id") + // Pulls channel-object-type CPA fields — the ones a rule references as + // resource.attributes.* — for a caller with no single channel to scope by, + // such as an editor for a policy that many channels import. Only meaningful + // without a channelId (a channel scope already includes them), and in that + // case the permission check below requires either ManageSystem or + // team-admin access-rule permission on team_id (via teamAdminCELContextOK). + includeResourceFields := r.URL.Query().Get("include_resource_fields") == "true" + hasSystemPermission := c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionManageSystem) if !hasSystemPermission { if !teamAdminCELContextOK(c, channelId, teamID) { @@ -1348,7 +1359,7 @@ func getFieldsAutocomplete(c *Context, w http.ResponseWriter, r *http.Request) { var ac []*model.PropertyField var appErr *model.AppError - ac, appErr = c.App.GetAccessControlFieldsAutocomplete(c.AppContext, after, limit, c.AppContext.Session().UserId) + ac, appErr = c.App.GetAccessControlFieldsAutocomplete(c.AppContext, channelId, includeResourceFields, after, limit, c.AppContext.Session().UserId) if appErr != nil { c.Err = appErr diff --git a/server/channels/api4/access_control_test.go b/server/channels/api4/access_control_test.go index 7eaf8cb0a9f..ec28c4d05ba 100644 --- a/server/channels/api4/access_control_test.go +++ b/server/channels/api4/access_control_test.go @@ -3284,6 +3284,110 @@ func TestSimulatePolicyForUsers(t *testing.T) { }) } +// TestGetFieldsAutocompleteResourceFields exercises the HTTP boundary of +// GET /cel/autocomplete/fields for channel-object-type (resource.attributes.*) +// fields: a channel scope or the include_resource_fields flag surfaces them +// (tagged with their ObjectType), neither excludes them, and the permission +// gating holds. This endpoint resolves fields from the property store directly +// and does not go through the mocked AccessControl engine. +func TestGetFieldsAutocompleteResourceFields(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + // Setup() only clears the read-only guard on feature flags when it is handed a + // config updater, so do it explicitly before flipping one. + th.ConfigStore.SetReadOnlyFF(false) + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.FeatureFlags.ResourceAttributesInPolicies = true + }) + + group, appErr := th.App.GetPropertyGroup(th.Context, model.AccessControlPropertyGroupName) + require.Nil(t, appErr) + + mkField := func(objectType string) *model.PropertyField { + f, appErr := th.App.CreatePropertyField(th.Context, &model.PropertyField{ + GroupID: group.ID, + Name: "region" + model.NewId(), + Type: model.PropertyFieldTypeText, + ObjectType: objectType, + TargetType: string(model.PropertyFieldTargetLevelSystem), + }, false, "") + require.Nil(t, appErr) + return f + } + userField := mkField(model.PropertyFieldObjectTypeUser) + channelField := mkField(model.PropertyFieldObjectTypeChannel) + + const base = "/access_control_policies/cel/autocomplete/fields" + + get := func(t *testing.T, client *model.Client4, query string) []*model.PropertyField { + t.Helper() + resp, err := client.DoAPIGet(context.Background(), base+query, "") + require.NoError(t, err) + defer resp.Body.Close() + var fields []*model.PropertyField + require.NoError(t, json.NewDecoder(resp.Body).Decode(&fields)) + return fields + } + + find := func(fields []*model.PropertyField, id string) *model.PropertyField { + for _, f := range fields { + if f.ID == id { + return f + } + } + return nil + } + + t.Run("channel scope surfaces channel fields tagged by object type", func(t *testing.T) { + fields := get(t, th.SystemAdminClient, "?limit=100&channelId="+th.BasicChannel.Id) + require.NotNil(t, find(fields, userField.ID), "user field must be present when channel-scoped") + ch := find(fields, channelField.ID) + require.NotNil(t, ch, "channel field must be present when channel-scoped") + require.Equal(t, model.PropertyFieldObjectTypeChannel, ch.ObjectType, "channel field must carry its ObjectType") + }) + + t.Run("include_resource_fields surfaces channel fields with no channel scope", func(t *testing.T) { + fields := get(t, th.SystemAdminClient, "?limit=100&include_resource_fields=true") + require.NotNil(t, find(fields, channelField.ID), "channel field must be present when include_resource_fields=true") + }) + + t.Run("channel fields excluded without a channel scope or the flag", func(t *testing.T) { + fields := get(t, th.SystemAdminClient, "?limit=100") + require.NotNil(t, find(fields, userField.ID), "user field must still be present") + require.Nil(t, find(fields, channelField.ID), "channel field must be excluded when neither channelId nor the flag is set") + }) + + t.Run("channel fields excluded on both paths while the feature is off", func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.FeatureFlags.ResourceAttributesInPolicies = false + }) + defer th.App.UpdateConfig(func(cfg *model.Config) { + cfg.FeatureFlags.ResourceAttributesInPolicies = true + }) + + scoped := get(t, th.SystemAdminClient, "?limit=100&channelId="+th.BasicChannel.Id) + require.Nil(t, find(scoped, channelField.ID), "channel scope must not surface a channel field while off") + require.NotNil(t, find(scoped, userField.ID), "user fields must keep working while off") + + asked := get(t, th.SystemAdminClient, "?limit=100&include_resource_fields=true") + require.Nil(t, find(asked, channelField.ID), "include_resource_fields must not surface a channel field while off") + }) + + t.Run("regular user without manage-system is denied when unscoped", func(t *testing.T) { + resp, err := th.Client.DoAPIGet(context.Background(), base+"?limit=100", "") + require.Error(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusForbidden, resp.StatusCode) + }) + + t.Run("regular user without channel-manage-access-rules is denied for a channel", func(t *testing.T) { + resp, err := th.Client.DoAPIGet(context.Background(), base+"?limit=100&channelId="+th.BasicChannel.Id, "") + require.Error(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusForbidden, resp.StatusCode) + }) +} + func mustMarshal(t *testing.T, v any) []byte { t.Helper() b, err := json.Marshal(v) diff --git a/server/channels/app/access_control.go b/server/channels/app/access_control.go index 9f90d5a45ed..13619fc7da2 100644 --- a/server/channels/app/access_control.go +++ b/server/channels/app/access_control.go @@ -21,6 +21,22 @@ import ( const attributeViewRefreshInterval = 30 * time.Second const accessControlChildPolicySearchLimit = 1000 +// ResourceAttributesInPoliciesEnabled reports whether access rules may compare a +// user's attributes against the accessed channel's. It gates authoring only: the +// autocomplete endpoint stops offering channel-object-type fields, so no editor +// can build such a rule, and the engine rejects one at save time. Evaluation of a +// rule already stored is deliberately not gated — those rules deny for any channel +// missing a value, so dropping enforcement would remove every member of every +// channel the policy governs. +// +// Only the flag is checked here. The licence and AccessControlSettings gates are +// applied by the surfaces that need them, and ABAC is unusable without both +// anyway; re-checking them here would make the autocomplete endpoint stricter +// than it is for user attributes today. +func (a *App) ResourceAttributesInPoliciesEnabled() bool { + return a.Config().FeatureFlags.ResourceAttributesInPolicies +} + func (a *App) GetChannelsForPolicy(rctx request.CTX, policyID string, cursor model.AccessControlPolicyCursor, limit int) ([]*model.ChannelWithTeamData, int64, *model.AppError) { policy, appErr := a.GetAccessControlPolicy(rctx, policyID) if appErr != nil { @@ -687,6 +703,41 @@ func isThisRuleScope(scope string) bool { // indexed by field name. const userAttributesPathPrefix = "user.attributes." +// resourceAttributesPathPrefix is the analogous prefix for the accessed +// resource's custom attributes (e.g. `resource.attributes.Sensitivity`). +// A simulator leaf carrying this prefix records the target channel's own +// attribute value, which must be hidden when the channel field is +// protected — separately from the user side, since a channel field's +// visibility can differ from a same-named user field. +const resourceAttributesPathPrefix = "resource.attributes." + +// protectedCPAAttributes bundles the sets of protected CPA field names +// per attribute root. User and resource (channel) fields are tracked +// separately because a channel field's visibility/access mode can differ +// from a user field that happens to share its name, so a leaf must be +// matched against the set for its own root. +type protectedCPAAttributes struct { + user map[string]struct{} + resource map[string]struct{} +} + +func (p protectedCPAAttributes) isEmpty() bool { + return len(p.user) == 0 && len(p.resource) == 0 +} + +// set returns the protected-name set for a CPA object type, or nil for an +// object type that never surfaces in a simulation trace. +func (p protectedCPAAttributes) set(objectType string) map[string]struct{} { + switch objectType { + case model.PropertyFieldObjectTypeUser: + return p.user + case model.PropertyFieldObjectTypeChannel: + return p.resource + default: + return nil + } +} + // RedactSimulationAttributesForCaller strips attribute values from a // PolicySimulationResponse on every surface the picker exposes // (top-level user/session Attributes maps AND the per-leaf @@ -708,7 +759,7 @@ const userAttributesPathPrefix = "user.attributes." // - `access_mode == "shared_only"`: the underlying property // service computes an intersection of the caller's and target's // values on read. The simulator does NOT call the property -// service (it reads from AttributeView directly), so we +// service (it reads from UserAttributeView directly), so we // conservatively redact these values rather than ship them // unfiltered. // @@ -747,41 +798,56 @@ func (a *App) RedactSimulationAttributesForCaller(rctx request.CTX, resp *model. clearAllEvaluationTreeActualValues(resp) return } - if len(protected) == 0 { + if protected.isEmpty() { return } - stripProtectedAttributes(resp, protected) + // Top-level Attributes maps hold only the simulated user's own + // snapshot (resource attributes never appear there), so they are + // pruned against the user set alone. The evaluation trees can carry + // both user.attributes.* and resource.attributes.* leaves, so the + // tree walker matches each against the set for its root. + stripProtectedAttributes(resp, protected.user) redactProtectedEvaluationTreeActualValues(resp, protected) } -// protectedCPAFieldNamesForCaller returns the set of CPA field names -// whose contents must be hidden from a non-system-admin caller. The -// set includes both `visibility: hidden` fields and any field whose -// `access_mode` is not public (source_only / shared_only). The -// simulator's AttributeView populates its per-user map keyed by -// `pf.Name` (see db/migrations/postgres/000137_update_attribute_view.up.sql), +// protectedCPAFieldNamesForCaller returns, per attribute root, the set of +// CPA field names whose contents must be hidden from a non-system-admin +// caller. The set includes both `visibility: hidden` fields and any field +// whose `access_mode` is not public (source_only / shared_only). The +// simulator's UserAttributeView populates its per-user map keyed by +// `pf.Name` (see db/migrations/postgres/000216_split_attribute_view_by_object_type.up.sql), // and the evaluation-tree walker likewise records `user.attributes.` -// on each leaf — so matching by name is correct for both. -func (a *App) protectedCPAFieldNamesForCaller(rctx request.CTX) (map[string]struct{}, error) { +// on each leaf — so matching by name is correct for the user set. Channel +// fields are matched the same way, by name, against their own set: the +// walker records those as `resource.attributes.`. +func (a *App) protectedCPAFieldNamesForCaller(rctx request.CTX) (protectedCPAAttributes, error) { + protected := protectedCPAAttributes{ + user: map[string]struct{}{}, + resource: map[string]struct{}{}, + } + group, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) if appErr != nil { - return nil, appErr + return protected, appErr } propertyFields, appErr := a.SearchPropertyFields(rctx, group.ID, model.PropertyFieldSearchOpts{ - ObjectTypes: []string{model.PropertyFieldObjectTypeUser}, + ObjectTypes: []string{model.PropertyFieldObjectTypeUser, model.PropertyFieldObjectTypeChannel}, PerPage: model.AccessControlGroupFieldLimit + 5, }) if appErr != nil { - return nil, appErr + return protected, appErr } - protected := map[string]struct{}{} for _, pf := range propertyFields { if pf == nil { continue } + set := protected.set(pf.ObjectType) + if set == nil { + continue + } f, err := model.NewCPAFieldFromPropertyField(pf) if err != nil { // Fail-closed: an unparseable field is treated as protected @@ -789,13 +855,14 @@ func (a *App) protectedCPAFieldNamesForCaller(rctx request.CTX) (map[string]stru rctx.Logger().Warn("Failed to parse property field for CPA protection check; treating as protected", mlog.String("field_name", pf.Name), mlog.String("field_id", pf.ID), + mlog.String("object_type", pf.ObjectType), mlog.Err(err), ) - protected[pf.Name] = struct{}{} + set[pf.Name] = struct{}{} continue } if cpaFieldIsProtectedForChannelAdmin(f) { - protected[f.Name] = struct{}{} + set[f.Name] = struct{}{} } } return protected, nil @@ -900,18 +967,20 @@ func stripProtectedAttributes(resp *model.PolicySimulationResponse, protected ma // EvaluationTree (and the per-rule subtrees attached under // MergedRules) on every result and session decision in `resp`. For // each leaf node whose `Attribute` references a protected CPA field -// (path format `user.attributes.`), the leaf's `ActualValue` -// is blanked. +// (path format `user.attributes.` or `resource.attributes.`), +// the leaf's `ActualValue` is blanked. // // Why ActualValue and nothing else: // - `Attribute` is the path; it already appears in the rule's // `Expression`, which the channel admin can see. // - `ExpectedValue` is the literal from the rule (e.g. `"il5"`), -// not the user's data — also already in `Expression`. -// - `ActualValue` is the only field that records the target user's -// concrete attribute value. That's the one we must redact. -func redactProtectedEvaluationTreeActualValues(resp *model.PolicySimulationResponse, protected map[string]struct{}) { - if resp == nil || len(protected) == 0 { +// not the user's or channel's data — also already in `Expression`. +// - `ActualValue` is the only field that records the target's +// concrete attribute value — the user's for a `user.attributes.*` +// leaf, the accessed channel's for a `resource.attributes.*` leaf. +// That's the one we must redact. +func redactProtectedEvaluationTreeActualValues(resp *model.PolicySimulationResponse, protected protectedCPAAttributes) { + if resp == nil || protected.isEmpty() { return } for i := range resp.Results { @@ -929,7 +998,7 @@ func redactProtectedEvaluationTreeActualValues(resp *model.PolicySimulationRespo } } -func redactProtectedActualValuesInDecision(dec *model.PolicySimulationActionDecision, protected map[string]struct{}) { +func redactProtectedActualValuesInDecision(dec *model.PolicySimulationActionDecision, protected protectedCPAAttributes) { for i := range dec.Blame { b := &dec.Blame[i] if b.EvaluationTree != nil { @@ -945,9 +1014,9 @@ func redactProtectedActualValuesInDecision(dec *model.PolicySimulationActionDeci // redactProtectedActualValuesInTree recursively walks `node` and // blanks the `ActualValue` on every leaf whose `Attribute` resolves -// to a CPA field in `protected`. Operates in place on the tree -// pointer the response shares with its parent blame entry. -func redactProtectedActualValuesInTree(node *model.PolicySimulationEvaluationNode, protected map[string]struct{}) { +// to a protected CPA field. Operates in place on the tree pointer the +// response shares with its parent blame entry. +func redactProtectedActualValuesInTree(node *model.PolicySimulationEvaluationNode, protected protectedCPAAttributes) { if node == nil { return } @@ -959,20 +1028,22 @@ func redactProtectedActualValuesInTree(node *model.PolicySimulationEvaluationNod } } -// isProtectedAttributePath returns true when `path` is the canonical -// CEL form `user.attributes.` and `` is in `protected`. -// Returns false for empty paths and for any path that doesn't carry -// the user-attribute prefix (other shapes — function-call leaves, -// constant comparisons — are not user data). -func isProtectedAttributePath(path string, protected map[string]struct{}) bool { - if path == "" || len(protected) == 0 { +// isProtectedAttributePath returns true when `path` is a canonical CPA +// leaf reference — `user.attributes.` or `resource.attributes.` +// — whose `` is in the protected set for that root. Each root is +// matched against its own set so a channel field's visibility can't be +// inferred from a same-named user field. Returns false for empty paths +// and for any path that doesn't carry a CPA prefix (function-call +// leaves, constant comparisons, native selectors — not custom data). +func isProtectedAttributePath(path string, protected protectedCPAAttributes) bool { + if path == "" || protected.isEmpty() { return false } - name, ok := strings.CutPrefix(path, userAttributesPathPrefix) - if !ok || name == "" { + objectType, name, ok := splitCPAAttribute(path) + if !ok { return false } - _, found := protected[name] + _, found := protected.set(objectType)[name] return found } @@ -1724,16 +1795,32 @@ func (a *App) GetAccessControlPolicyAttributes(rctx request.CTX, channelID strin return attributes, nil } -func (a *App) GetAccessControlFieldsAutocomplete(rctx request.CTX, after string, limit int, callerID string) ([]*model.PropertyField, *model.AppError) { +func (a *App) GetAccessControlFieldsAutocomplete(rctx request.CTX, channelID string, includeResourceFields bool, after string, limit int, callerID string) ([]*model.PropertyField, *model.AppError) { group, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) if appErr != nil { return nil, model.NewAppError("GetAccessControlAutoComplete", "app.pap.get_access_control_auto_complete.app_error", nil, "", http.StatusInternalServerError).Wrap(appErr) } + // A policy references the requesting user (user.attributes.*) and the + // accessed resource (resource.attributes.*). Resource attributes are + // channel-object-type CPA fields, so include them when a channel is in + // scope, or when the caller explicitly asks for them — a policy that many + // channels import has no single channel to scope by, and still needs the + // fields to author against. Each returned field carries its ObjectType, so + // a caller can tell the two namespaces apart. + // + // Both paths are gated: while resource attributes are off, omitting the + // channel object type is what keeps every editor from offering one, since + // each authoring surface is driven by what this endpoint returns. + objectTypes := []string{model.PropertyFieldObjectTypeUser} + if (channelID != "" || includeResourceFields) && a.ResourceAttributesInPoliciesEnabled() { + objectTypes = append(objectTypes, model.PropertyFieldObjectTypeChannel) + } + // Use property app layer to enforce access control rctxWithCaller := RequestContextWithCallerID(rctx, callerID) fields, appErr := a.SearchPropertyFields(rctxWithCaller, group.ID, model.PropertyFieldSearchOpts{ - ObjectType: model.PropertyFieldObjectTypeUser, + ObjectTypes: objectTypes, Cursor: model.PropertyFieldSearchCursor{ PropertyFieldID: after, CreateAt: 1, @@ -2350,8 +2437,17 @@ func (a *App) TestExpressionWithChannelContext(rctx request.CTX, expression stri currentUserID := session.UserId - // SECURITY: First check if the channel admin themselves matches this expression - // If they don't match, they shouldn't be able to see users who do + // Only return results if the requesting admin matches the expression + // themselves. This blocks the obvious probe: an admin who is not, say, + // "TopSecret" cannot run clearance == "TopSecret" just to discover who is. + // + // It is a speed bump, not a hard boundary. An admin can OR in a term they + // always satisfy (user.id == "" || ) to pass this check, then + // read the returned match count to answer yes/no questions about values they + // cannot see directly. That kind of enumeration is unavoidable in any feature + // that reveals who a rule matches, so we accept it. The thing we actually + // protect -- raw attribute values and options -- is masked when a policy is + // read, not here. adminMatches, appErr := a.ValidateExpressionAgainstRequester(rctx, expression, currentUserID) if appErr != nil { return nil, 0, appErr @@ -2371,9 +2467,9 @@ func (a *App) TestExpressionWithChannelContext(rctx request.CTX, expression stri return a.TestExpression(rctx, expression, opts) } -// TestExpressionWithTeamContext tests expressions for team admins with the same -// info-leak guard as the channel variant: a team admin may only see users who -// match an expression that they themselves match. +// TestExpressionWithTeamContext is the team-admin counterpart of +// TestExpressionWithChannelContext and applies the same self-match check; see +// that function's comment for what the check does and does not protect. func (a *App) TestExpressionWithTeamContext(rctx request.CTX, expression string, opts model.SubjectSearchOptions) ([]*model.User, int64, *model.AppError) { session := rctx.Session() if session == nil { @@ -2382,7 +2478,8 @@ func (a *App) TestExpressionWithTeamContext(rctx request.CTX, expression string, currentUserID := session.UserId - // SECURITY: a team admin who doesn't match the expression must not learn who does. + // Same self-match check as the channel variant (see its comment): a partial + // guard against casual probing, not a confidentiality boundary. adminMatches, appErr := a.ValidateExpressionAgainstRequester(rctx, expression, currentUserID) if appErr != nil { return nil, 0, appErr @@ -2446,7 +2543,7 @@ func (a *App) BuildAccessControlSubject(rctx request.CTX, userID string, roles s return nil, model.NewAppError("BuildAccessControlSubject", "app.access_control.build_subject.group_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - subject, storeErr := a.Srv().Store().Attributes().GetSubject(rctx, userID, group.ID) + subject, storeErr := a.Srv().Store().Attributes().GetSubject(rctx, userID, group.ID, model.PropertyFieldObjectTypeUser) if storeErr != nil { var nfErr *store.ErrNotFound if errors.As(storeErr, &nfErr) { @@ -2642,9 +2739,10 @@ func ResolveSystemRole(roles string) string { return model.SystemUserRoleId } -// refreshAttributeViewIfStale refreshes the materialized AttributeView if the last -// refresh was more than attributeViewRefreshInterval ago. The refresh is non-blocking: -// if another goroutine is already refreshing, this call returns immediately. +// refreshAttributeViewIfStale refreshes the attribute materialized views if the +// last refresh was more than attributeViewRefreshInterval ago. The refresh is +// non-blocking: if another goroutine is already refreshing, this call returns +// immediately. func (a *App) refreshAttributeViewIfStale(rctx request.CTX) { ch := a.Srv().Channels() diff --git a/server/channels/app/access_control_masking.go b/server/channels/app/access_control_masking.go index e6ce4584fa2..481a25065e9 100644 --- a/server/channels/app/access_control_masking.go +++ b/server/channels/app/access_control_masking.go @@ -5,6 +5,7 @@ package app import ( "encoding/json" + "maps" "net/http" "strings" @@ -39,44 +40,58 @@ func (a *App) GetMaskedVisualAST(rctx request.CTX, expression string, callerID s // Embed callerID in context so GetPropertyFieldByName applies per-caller option filtering. rctxWithCaller := RequestContextWithCallerID(rctx, callerID) - // Pre-fetch all referenced fields once to avoid N+1 DB queries across conditions. - fieldsByName := a.fetchConditionFields(rctxWithCaller, visualAST.Conditions, cpaGroupID) + // Pre-fetch the holdings-bearing field for each referenced condition once to + // avoid N+1 DB queries across conditions. + fieldsByKey := a.fetchConditionFields(rctxWithCaller, visualAST.Conditions, cpaGroupID) for i := range visualAST.Conditions { - a.maskConditionValues(rctxWithCaller, callerID, &visualAST.Conditions[i], cpaGroupID, fieldsByName) + a.maskConditionValues(rctxWithCaller, callerID, &visualAST.Conditions[i], cpaGroupID, fieldsByKey) } return visualAST, nil } -// fetchConditionFields collects unique field names from conditions and fetches each once. -// Lookup failures are logged and omitted from the returned map; read-path callers treat -// missing entries as fail-closed (mask the value). Write-path callers should additionally -// call requireAllFieldsResolved to refuse to proceed when any referenced field is missing. -func (a *App) fetchConditionFields(rctx request.CTX, conditions []model.Condition, cpaGroupID string) map[string]*model.PropertyField { - seen := make(map[string]bool) +// maskingHoldings pairs the field whose per-caller values determine which +// literals are visible (holdings) with the access mode that governs the +// reference. For a channel attribute these come from different fields: the +// access mode is the channel field's own (it defines whether the attribute is +// protected), while holdings are read from the user-side sibling — see +// holdingsFieldFor. +type maskingHoldings struct { + field *model.PropertyField + accessMode string +} + +// fetchConditionFields collects the unique CPA references from conditions and +// fetches, for each, the holdings/access-mode pair that determines its +// visibility (see holdingsFieldFor). The map is keyed by +// "/" so a user and a channel reference sharing a name +// resolve independently. Lookup failures are logged and omitted; read-path +// callers treat missing entries as fail-closed (mask the value). +func (a *App) fetchConditionFields(rctx request.CTX, conditions []model.Condition, cpaGroupID string) map[string]*maskingHoldings { + type ref struct{ objectType, fieldName string } + seen := make(map[ref]struct{}) for _, c := range conditions { if c.ValueType == model.AttrValue { continue } - if name := extractFieldName(c.Attribute); name != "" { - seen[name] = true + if objectType, fieldName, ok := splitCPAAttribute(c.Attribute); ok { + seen[ref{objectType, fieldName}] = struct{}{} } } - fields := make(map[string]*model.PropertyField, len(seen)) - for name := range seen { - // Scope to user CPA fields so a name shared across object types in this - // group resolves deterministically. - field, appErr := a.GetPropertyFieldByNameForObjectType(rctx, cpaGroupID, "", model.PropertyFieldObjectTypeUser, name) + fields := make(map[string]*maskingHoldings, len(seen)) + for r := range seen { + h, appErr := a.holdingsFieldFor(rctx, cpaGroupID, r.objectType, r.fieldName) if appErr != nil { rctx.Logger().Warn("Failed to look up field for masking, failing closed", - mlog.String("field_name", name), + mlog.String("object_type", r.objectType), + mlog.String("field_name", r.fieldName), mlog.Err(appErr), ) continue } - fields[name] = field + fields[holdingsKey(r.objectType, r.fieldName)] = h } return fields } @@ -105,34 +120,152 @@ func newMaskingResolver(a *App, rctx request.CTX, callerID string) (*appMaskingR }, nil } -func (r *appMaskingResolver) Resolve(fieldName string) (*model.MaskingFieldInfo, error) { - if info, ok := r.cache[fieldName]; ok { +func (r *appMaskingResolver) Resolve(objectType, fieldName string) (*model.MaskingFieldInfo, error) { + // The cache key includes the object type: a user field and a channel field + // can share a name yet have different visibility. + cacheKey := holdingsKey(objectType, fieldName) + if info, ok := r.cache[cacheKey]; ok { return info, nil } - // Scope to user CPA fields so a name shared across object types in this - // group resolves deterministically. - field, appErr := r.app.GetPropertyFieldByNameForObjectType(r.rctxWithCaller, r.cpaGroupID, "", model.PropertyFieldObjectTypeUser, fieldName) + + h, appErr := r.app.holdingsFieldFor(r.rctxWithCaller, r.cpaGroupID, objectType, fieldName) if appErr != nil { return nil, appErr } - info := r.fieldToMaskingInfo(field) - r.cache[fieldName] = info + info := r.fieldToMaskingInfo(h) + r.cache[cacheKey] = info return info, nil } -func (r *appMaskingResolver) fieldToMaskingInfo(field *model.PropertyField) *model.MaskingFieldInfo { +// holdingsFieldFor returns the holdings-bearing field plus the access mode that +// governs a reference to (objectType, fieldName). The field is fetched with +// caller context so its options are already filtered to the caller's holdings. +// +// For a user reference both come from the field itself. For a channel reference +// they come from different fields: the access mode is the CHANNEL field's own — +// whether the attribute is protected is a property of the channel field, and a +// linked field does NOT inherit access mode from its template at creation, so +// reading it off the user sibling can silently under-protect. Holdings, by +// contrast, must come from the user-side sibling linked to the same template, +// because users never hold channel-side values directly. +// +// A caller's held option names are only pre-filtered onto the sibling by the +// read path when the sibling is itself shared_only. If the channel field is +// shared_only but the sibling is not, the sibling's option list is unfiltered, +// so its options are dropped (fail closed); text holdings are queried directly +// and are unaffected. When a channel field is unlinked or has no user sibling, +// its own (caller-filtered) field is used — for shared_only that yields no +// visible values (the caller holds nothing channel-side), the fail-closed +// direction. +func (a *App) holdingsFieldFor(rctx request.CTX, groupID, objectType, fieldName string) (*maskingHoldings, *model.AppError) { + var lookupType string + switch objectType { + case model.PropertyFieldObjectTypeChannel: + lookupType = model.PropertyFieldObjectTypeChannel + case model.PropertyFieldObjectTypeUser: + lookupType = model.PropertyFieldObjectTypeUser + default: + // Only user./resource. CPA roots reach here (see cpaAttributeRoots); fail + // loud rather than silently treating an unexpected type as a user lookup. + return nil, model.NewAppError("holdingsFieldFor", "app.pap.masking.unknown_object_type.app_error", map[string]any{"ObjectType": objectType}, "", http.StatusInternalServerError) + } + + field, appErr := a.GetPropertyFieldByNameForObjectType(rctx, groupID, "", lookupType, fieldName) + if appErr != nil { + return nil, appErr + } + if lookupType != model.PropertyFieldObjectTypeChannel { + return &maskingHoldings{field: field, accessMode: field.GetAccessMode()}, nil + } + + accessMode := field.GetAccessMode() + if field.LinkedFieldID != nil && *field.LinkedFieldID != "" { + sibling, appErr := a.userSiblingField(rctx, groupID, *field.LinkedFieldID) + if appErr != nil { + return nil, appErr + } + if sibling != nil { + // The sibling supplies the caller's held values, but its options are + // only held-filtered by the read path when the sibling is itself + // shared_only. If the channel field is protected but the sibling is + // not, the sibling's option list is unfiltered — drop it so no + // unheld option name leaks (text holdings query the caller directly + // and are unaffected). + if accessMode == model.PropertyAccessModeSharedOnly && + sibling.GetAccessMode() != model.PropertyAccessModeSharedOnly && + sibling.Type.SupportsOptions() { + sibling = fieldWithEmptyOptions(sibling) + } + return &maskingHoldings{field: sibling, accessMode: accessMode}, nil + } + } + return &maskingHoldings{field: field, accessMode: accessMode}, nil +} + +// fieldWithEmptyOptions returns a shallow copy of f with an empty options list, +// so extractVisibleOptionNames yields nothing. Used to fail closed without +// mutating the (possibly cached) source field. +func fieldWithEmptyOptions(f *model.PropertyField) *model.PropertyField { + cp := *f + cp.Attrs = make(model.StringInterface, len(f.Attrs)+1) + maps.Copy(cp.Attrs, f.Attrs) + cp.Attrs[model.PropertyFieldAttributeOptions] = []any{} + return &cp +} + +// userSiblingField returns the user-object-type CPA field linked to the same +// template (linkedFieldID), fetched through rctx so its options are filtered to +// that caller's holdings. Returns nil when no such field exists. +func (a *App) userSiblingField(rctx request.CTX, groupID, linkedFieldID string) (*model.PropertyField, *model.AppError) { + fields, appErr := a.SearchPropertyFields(rctx, groupID, model.PropertyFieldSearchOpts{ + ObjectTypes: []string{model.PropertyFieldObjectTypeUser}, + LinkedFieldID: linkedFieldID, + PerPage: 2, + }) + if appErr != nil { + return nil, appErr + } + // The masking decision uses this sibling's held values to gate a channel + // field's option names, so it must resolve to exactly one field. Nothing + // enforces LinkedFieldID uniqueness at the DB level, so a second match would + // make the choice depend on store order and could disclose a value via the + // "wrong" sibling. Error on ambiguity rather than guess — the caller fails + // this field closed (precompute logs and skips it, masking every value). + // PerPage:2 is enough to detect the second match. + var match *model.PropertyField + for _, f := range fields { + if f == nil { + continue + } + if match != nil { + return nil, model.NewAppError("userSiblingField", "app.pap.masking.ambiguous_sibling.app_error", map[string]any{"LinkedFieldID": linkedFieldID}, "multiple user fields link the same template", http.StatusInternalServerError) + } + match = f + } + return match, nil +} + +func (r *appMaskingResolver) fieldToMaskingInfo(h *maskingHoldings) *model.MaskingFieldInfo { info := &model.MaskingFieldInfo{} - switch field.GetAccessMode() { + // h.accessMode is authoritative (the channel field's own for a channel + // reference); h.field supplies the caller's held values. + switch h.accessMode { case model.PropertyAccessModePublic: info.Access = model.MaskingFieldAccessPublic case model.PropertyAccessModeSourceOnly: info.Access = model.MaskingFieldAccessSourceOnly case model.PropertyAccessModeSharedOnly: info.Access = model.MaskingFieldAccessSharedOnly - if field.Type == model.PropertyFieldTypeSelect || field.Type == model.PropertyFieldTypeMultiselect { - info.VisibleValues = extractVisibleOptionNames(field) + + // Same split as maskConditionValues, through the same predicate: an + // options-bearing field's visible values are its caller-filtered option + // names, anything else's are the caller's stored text values. Spelling the + // type list out here instead would drop rank — whose values are options + // too — and the two paths would disagree about what a caller can see. + if h.field.Type.SupportsOptions() { + info.VisibleValues = extractVisibleOptionNames(h.field) } else { - info.VisibleValues = r.app.getCallerTextValues(r.rctxWithCaller, r.callerID, field, r.cpaGroupID) + info.VisibleValues = r.app.getCallerTextValues(r.rctxWithCaller, r.callerID, h.field, r.cpaGroupID) } default: info.Access = model.MaskingFieldAccessUnknown @@ -151,18 +284,21 @@ func (r *appMaskingResolver) fieldToMaskingInfo(field *model.PropertyField) *mod // The condition's value is either visible in full (the caller's stored // text value matches it exactly) or fully masked. No partial chip behavior // is possible because there's no multi-value list to filter. -func (a *App) maskConditionValues(rctx request.CTX, callerID string, condition *model.Condition, cpaGroupID string, fieldsByName map[string]*model.PropertyField) { +func (a *App) maskConditionValues(rctx request.CTX, callerID string, condition *model.Condition, cpaGroupID string, fieldsByKey map[string]*maskingHoldings) { // AttrValue conditions compare two attributes (e.g. user.attr1 == user.attr2) — no literal values to mask. if condition.ValueType == model.AttrValue { return } - fieldName := extractFieldName(condition.Attribute) - if fieldName == "" { + objectType, fieldName, ok := splitCPAAttribute(condition.Attribute) + if !ok { return } - field, ok := fieldsByName[fieldName] + // h pairs the holdings-bearing field (the user sibling for a channel + // attribute) with the authoritative access mode (the channel field's own), + // keyed by object type so a shared name does not collide across roots. + h, ok := fieldsByKey[holdingsKey(objectType, fieldName)] if !ok { // Fail closed: field lookup failed at prefetch time. condition.Value = nil @@ -170,17 +306,17 @@ func (a *App) maskConditionValues(rctx request.CTX, callerID string, condition * return } - switch field.GetAccessMode() { + switch h.accessMode { case model.PropertyAccessModePublic: // no-op case model.PropertyAccessModeSourceOnly: condition.Value = nil condition.HasMaskedValues = true case model.PropertyAccessModeSharedOnly: - if field.Type.SupportsOptions() { - filterConditionValues(condition, extractVisibleOptionNames(field)) + if h.field.Type.SupportsOptions() { + filterConditionValues(condition, extractVisibleOptionNames(h.field)) } else { - filterConditionValues(condition, a.getCallerTextValues(rctx, callerID, field, cpaGroupID)) + filterConditionValues(condition, a.getCallerTextValues(rctx, callerID, h.field, cpaGroupID)) } default: // Unknown access mode: fail closed. @@ -189,16 +325,32 @@ func (a *App) maskConditionValues(rctx request.CTX, callerID string, condition * } } -// extractFieldName strips the "user.attributes." prefix from a CEL attribute -// reference, returning just the property field name. Returns the empty string -// if the attribute is not a user-attribute reference. -func extractFieldName(attribute string) string { - const prefix = "user.attributes." - name := strings.TrimPrefix(attribute, prefix) - if name == attribute || name == "" { - return "" +// cpaAttributeRoots maps a CEL attribute-path prefix to the PropertyField +// object type whose CPA schema backs it: user.attributes.* is the requesting +// user, resource.attributes.* is the accessed channel. +var cpaAttributeRoots = []struct{ prefix, objectType string }{ + {userAttributesPathPrefix, model.PropertyFieldObjectTypeUser}, + {resourceAttributesPathPrefix, model.PropertyFieldObjectTypeChannel}, +} + +// splitCPAAttribute splits a CEL attribute path into its CPA object type and +// field name. ok is false for any path that is not a non-empty custom-attribute +// selector (native selectors such as user.email or resource.id carry no +// ".attributes." segment and own no maskable literal). +func splitCPAAttribute(attribute string) (objectType, fieldName string, ok bool) { + for _, r := range cpaAttributeRoots { + if name, found := strings.CutPrefix(attribute, r.prefix); found && name != "" { + return r.objectType, name, true + } } - return name + return "", "", false +} + +// holdingsKey keys the prefetched-holdings map and the resolver cache. Object +// type is part of the key because a user field and a channel field can share +// a name and differ in visibility. +func holdingsKey(objectType, fieldName string) string { + return objectType + "/" + fieldName } // extractVisibleOptionNames pulls option names from a pre-filtered PropertyField's @@ -589,11 +741,11 @@ func (a *App) maskSimulationEvaluationTree(node *model.PolicySimulationEvaluatio // caller cannot see that value. Uses mc.resolver so field info is cached across // all leaves in the trace — no per-leaf DB calls. Fails closed on resolver error. func (a *App) maskLeafActualValue(node *model.PolicySimulationEvaluationNode, mc *simulationMaskContext) { - fieldName := extractFieldName(node.Attribute) - if fieldName == "" { + objectType, fieldName, ok := splitCPAAttribute(node.Attribute) + if !ok { return } - info, err := mc.resolver.Resolve(fieldName) + info, err := mc.resolver.Resolve(objectType, fieldName) if err != nil { node.ActualValue = maskedTokenValue return diff --git a/server/channels/app/access_control_masking_test.go b/server/channels/app/access_control_masking_test.go index 83d8c545268..dafdec55a8e 100644 --- a/server/channels/app/access_control_masking_test.go +++ b/server/channels/app/access_control_masking_test.go @@ -17,28 +17,6 @@ import ( "github.com/mattermost/mattermost/server/v8/einterfaces/mocks" ) -func TestExtractFieldName(t *testing.T) { - tests := []struct { - name string - attribute string - expected string - }{ - {"standard attribute path", "user.attributes.Program", "Program"}, - {"multi-word field", "user.attributes.Clearance Level", "Clearance Level"}, - {"no prefix", "Program", ""}, - {"partial prefix", "user.attributes.", ""}, - {"empty string", "", ""}, - {"different prefix", "team.attributes.Program", ""}, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - result := extractFieldName(tc.attribute) - assert.Equal(t, tc.expected, result) - }) - } -} - // Note: tests for field.GetAccessMode() live in model/property_access_test.go, // where the method is defined (TestPropertyFieldGetAccessMode). @@ -389,6 +367,13 @@ func TestMaskConditionValues(t *testing.T) { return &model.PropertyField{Type: fieldType, Attrs: attrs} } + // hold wraps a field as its own holdings source with its own access mode — + // the direct-field case these unit tests exercise (channel-sibling access-mode + // resolution is covered by the store-backed tests). + hold := func(f *model.PropertyField) *maskingHoldings { + return &maskingHoldings{field: f, accessMode: f.GetAccessMode()} + } + options := []any{ map[string]any{"id": "id1", "name": "Alpha"}, map[string]any{"id": "id2", "name": "Bravo"}, @@ -411,7 +396,7 @@ func TestMaskConditionValues(t *testing.T) { Value: "Engineering", ValueType: model.LiteralValue, } - a.maskConditionValues(rctx, "caller", condition, "", map[string]*model.PropertyField{}) + a.maskConditionValues(rctx, "caller", condition, "", map[string]*maskingHoldings{}) assert.Equal(t, "Engineering", condition.Value) assert.False(t, condition.HasMaskedValues) }) @@ -422,7 +407,7 @@ func TestMaskConditionValues(t *testing.T) { Value: "Alpha", ValueType: model.LiteralValue, } - a.maskConditionValues(rctx, "caller", condition, "", map[string]*model.PropertyField{}) + a.maskConditionValues(rctx, "caller", condition, "", map[string]*maskingHoldings{}) assert.Nil(t, condition.Value) assert.True(t, condition.HasMaskedValues) }) @@ -433,8 +418,8 @@ func TestMaskConditionValues(t *testing.T) { Value: "Alpha", ValueType: model.LiteralValue, } - fields := map[string]*model.PropertyField{ - "Program": makeField(model.PropertyAccessModePublic, model.PropertyFieldTypeSelect, options), + fields := map[string]*maskingHoldings{ + "user/Program": hold(makeField(model.PropertyAccessModePublic, model.PropertyFieldTypeSelect, options)), } a.maskConditionValues(rctx, "caller", condition, "", fields) assert.Equal(t, "Alpha", condition.Value) @@ -447,8 +432,8 @@ func TestMaskConditionValues(t *testing.T) { Value: "Top Secret", ValueType: model.LiteralValue, } - fields := map[string]*model.PropertyField{ - "Clearance": makeField(model.PropertyAccessModeSourceOnly, model.PropertyFieldTypeSelect, options), + fields := map[string]*maskingHoldings{ + "user/Clearance": hold(makeField(model.PropertyAccessModeSourceOnly, model.PropertyFieldTypeSelect, options)), } a.maskConditionValues(rctx, "caller", condition, "", fields) assert.Nil(t, condition.Value) @@ -461,8 +446,8 @@ func TestMaskConditionValues(t *testing.T) { Value: "Alpha", ValueType: model.LiteralValue, } - fields := map[string]*model.PropertyField{ - "Location": makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeSelect, options), + fields := map[string]*maskingHoldings{ + "user/Location": hold(makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeSelect, options)), } a.maskConditionValues(rctx, "caller", condition, "", fields) // "Alpha" is in the field options so it is visible @@ -476,8 +461,8 @@ func TestMaskConditionValues(t *testing.T) { Value: "Charlie", ValueType: model.LiteralValue, } - fields := map[string]*model.PropertyField{ - "Location": makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeSelect, options), + fields := map[string]*maskingHoldings{ + "user/Location": hold(makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeSelect, options)), } a.maskConditionValues(rctx, "caller", condition, "", fields) assert.Nil(t, condition.Value) @@ -490,8 +475,8 @@ func TestMaskConditionValues(t *testing.T) { Value: []any{"Alpha", "Charlie"}, ValueType: model.LiteralValue, } - fields := map[string]*model.PropertyField{ - "Programs": makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeMultiselect, options), + fields := map[string]*maskingHoldings{ + "user/Programs": hold(makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeMultiselect, options)), } a.maskConditionValues(rctx, "caller", condition, "", fields) values, ok := condition.Value.([]any) @@ -506,16 +491,57 @@ func TestMaskConditionValues(t *testing.T) { Value: "Alpha", ValueType: model.LiteralValue, } - fields := map[string]*model.PropertyField{ - "Program": { + fields := map[string]*maskingHoldings{ + "user/Program": hold(&model.PropertyField{ Type: model.PropertyFieldTypeSelect, Attrs: model.StringInterface{model.PropertyAttrsAccessMode: "future_unknown_mode"}, - }, + }), } a.maskConditionValues(rctx, "caller", condition, "", fields) assert.Nil(t, condition.Value) assert.True(t, condition.HasMaskedValues) }) + + t.Run("resource attribute: keyed by channel object type, public passes through", func(t *testing.T) { + condition := &model.Condition{ + Attribute: "resource.attributes.Sensitivity", + Value: "Alpha", + ValueType: model.LiteralValue, + } + fields := map[string]*maskingHoldings{ + "channel/Sensitivity": hold(makeField(model.PropertyAccessModePublic, model.PropertyFieldTypeSelect, options)), + } + a.maskConditionValues(rctx, "caller", condition, "", fields) + assert.Equal(t, "Alpha", condition.Value) + assert.False(t, condition.HasMaskedValues) + }) + + t.Run("resource attribute: shared_only value the caller does not hold is masked", func(t *testing.T) { + condition := &model.Condition{ + Attribute: "resource.attributes.Sensitivity", + Value: "secret", + ValueType: model.LiteralValue, + } + // The prefetched field is the holdings-bearing sibling; "secret" is not + // among the caller's visible options, so it masks. + fields := map[string]*maskingHoldings{ + "channel/Sensitivity": hold(makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeSelect, options)), + } + a.maskConditionValues(rctx, "caller", condition, "", fields) + assert.Nil(t, condition.Value) + assert.True(t, condition.HasMaskedValues) + }) + + t.Run("resource attribute missing from prefetch map: fail-closed", func(t *testing.T) { + condition := &model.Condition{ + Attribute: "resource.attributes.Sensitivity", + Value: "Alpha", + ValueType: model.LiteralValue, + } + a.maskConditionValues(rctx, "caller", condition, "", map[string]*maskingHoldings{}) + assert.Nil(t, condition.Value) + assert.True(t, condition.HasMaskedValues) + }) } // TestMaskConditionValues_SharedOnlyText covers the shared_only + text-field branch of @@ -564,7 +590,7 @@ func TestMaskConditionValues_SharedOnlyText(t *testing.T) { }) require.Nil(t, appErr) - fieldsByName := map[string]*model.PropertyField{createdField.Name: createdField} + fieldsByName := map[string]*maskingHoldings{model.PropertyFieldObjectTypeUser + "/" + createdField.Name: {field: createdField, accessMode: createdField.GetAccessMode()}} t.Run("caller's own value passes through", func(t *testing.T) { condition := &model.Condition{ @@ -1254,3 +1280,294 @@ func TestMaskSimulationPolicyLiteralsForCaller_CompoundOrPreserved(t *testing.T) assert.Equal(t, blame.EvaluationTree.Expression, blame.Expression) mockACS.AssertExpectations(t) } + +// TestSplitCPAAttribute pins the CPA attribute-path splitter that routes a leaf +// to the right object type. user.attributes.* → user, resource.attributes.* → +// channel; everything else (native selectors, empty suffix) is not a CPA leaf. +func TestSplitCPAAttribute(t *testing.T) { + mainHelper.Parallel(t) + + cases := []struct { + path string + wantObject string + wantField string + wantOK bool + }{ + {"user.attributes.Clearance", model.PropertyFieldObjectTypeUser, "Clearance", true}, + {"resource.attributes.Sensitivity", model.PropertyFieldObjectTypeChannel, "Sensitivity", true}, + {"user.email", "", "", false}, + {"resource.id", "", "", false}, + {"session.network_status", "", "", false}, + {"user.attributes.", "", "", false}, + {"resource.attributes.", "", "", false}, + {"", "", "", false}, + } + for _, c := range cases { + objectType, fieldName, ok := splitCPAAttribute(c.path) + assert.Equal(t, c.wantOK, ok, "ok mismatch for %q", c.path) + assert.Equal(t, c.wantObject, objectType, "objectType mismatch for %q", c.path) + assert.Equal(t, c.wantField, fieldName, "fieldName mismatch for %q", c.path) + } +} + +// TestAppMaskingResolver_ChannelFieldUsesUserHoldings verifies the shared-template +// bridge: a resource.attributes. reference is masked by the caller's own +// USER-side holdings for the linked template, since users never hold channel +// values directly. A shared_only channel field whose user sibling the caller +// partially holds must expose only the held option values. +// +// Run over both option-bearing types the clearance/classification pairing uses. +// Rank is the one that regresses if the resolver spells its type list out instead +// of asking Type.SupportsOptions(): a rank field would fall through to the +// caller's raw text values, which hold the option ID rather than its name, so +// every option would read as hidden. +func TestAppMaskingResolver_ChannelFieldUsesUserHoldings(t *testing.T) { + mainHelper.Parallel(t) + + for _, fieldType := range []model.PropertyFieldType{model.PropertyFieldTypeSelect, model.PropertyFieldTypeRank} { + t.Run(string(fieldType), func(t *testing.T) { + assertChannelFieldUsesUserHoldings(t, fieldType) + }) + } +} + +func assertChannelFieldUsesUserHoldings(t *testing.T, fieldType model.PropertyFieldType) { + t.Helper() + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + rctx := request.TestContext(t) + + cpaGroup, gErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, gErr) + groupID := cpaGroup.ID + + callerID := model.NewId() + + optA := model.NewId() + optB := model.NewId() + options := []any{ + map[string]any{"id": optA, "name": "A", "rank": 1}, + map[string]any{"id": optB, "name": "B", "rank": 2}, + } + + // Template field that both the user and channel fields link to. + tmpl, sErr := th.Store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: celSafeName(), + Type: fieldType, + ObjectType: model.PropertyFieldObjectTypeTemplate, + TargetType: string(model.PropertyFieldTargetLevelSystem), + Attrs: model.StringInterface{model.PropertyFieldAttributeOptions: options}, + }) + require.NoError(t, sErr) + + linkedAttrs := func() model.StringInterface { + return model.StringInterface{ + model.PropertyFieldAttributeOptions: options, + model.PropertyAttrsProtected: true, + model.PropertyAttrsAccessMode: model.PropertyAccessModeSharedOnly, + model.PropertyAttrsSourcePluginID: "com.mattermost.uas-plugin", + } + } + + // Store-level Create bypasses the CPA access-control layer so protected / + // shared_only linked fields can be written directly (the same shortcut the + // other shared_only masking tests take). Reads still filter per caller. + userField, sErr := th.Store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: celSafeName(), + Type: fieldType, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + LinkedFieldID: &tmpl.ID, + Attrs: linkedAttrs(), + }) + require.NoError(t, sErr) + + channelFieldName := celSafeName() + _, sErr = th.Store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: channelFieldName, + Type: fieldType, + ObjectType: model.PropertyFieldObjectTypeChannel, + TargetType: string(model.PropertyFieldTargetLevelSystem), + LinkedFieldID: &tmpl.ID, + Attrs: linkedAttrs(), + }) + require.NoError(t, sErr) + + // Caller holds option A on the USER field (users never hold channel values). + // Written store-level because the field is protected (app-layer writes to + // protected fields are plugin-only). + _, vErr := th.Store.PropertyValue().Create(&model.PropertyValue{ + TargetID: callerID, + TargetType: model.PropertyValueTargetTypeUser, + GroupID: groupID, + FieldID: userField.ID, + Value: json.RawMessage(`"` + optA + `"`), + }) + require.NoError(t, vErr) + + resolver, rErr := newMaskingResolver(th.App, rctx, callerID) + require.Nil(t, rErr) + + info, err := resolver.Resolve(model.PropertyFieldObjectTypeChannel, channelFieldName) + require.NoError(t, err) + require.Equal(t, model.MaskingFieldAccessSharedOnly, info.Access) + assert.False(t, info.IsValueHidden("A"), "value the caller holds user-side must be visible on the channel field") + assert.True(t, info.IsValueHidden("B"), "value the caller does not hold must be hidden on the channel field") +} + +// TestAppMaskingResolver_AmbiguousUserSibling guards the fail-open hole where two +// user fields link the same template. Nothing enforces LinkedFieldID uniqueness +// at the DB level, so picking the "first" sibling would make channel-field +// visibility depend on store order and could leak a value via the wrong sibling. +// Resolution must error instead of guessing. +func TestAppMaskingResolver_AmbiguousUserSibling(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + rctx := request.TestContext(t) + + cpaGroup, gErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, gErr) + groupID := cpaGroup.ID + + callerID := model.NewId() + + optA := model.NewId() + options := []any{map[string]any{"id": optA, "name": "A"}} + + tmpl, sErr := th.Store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: celSafeName(), + Type: model.PropertyFieldTypeSelect, + ObjectType: model.PropertyFieldObjectTypeTemplate, + TargetType: string(model.PropertyFieldTargetLevelSystem), + Attrs: model.StringInterface{model.PropertyFieldAttributeOptions: options}, + }) + require.NoError(t, sErr) + + linkedAttrs := func() model.StringInterface { + return model.StringInterface{ + model.PropertyFieldAttributeOptions: options, + model.PropertyAttrsProtected: true, + model.PropertyAttrsAccessMode: model.PropertyAccessModeSharedOnly, + model.PropertyAttrsSourcePluginID: "com.mattermost.uas-plugin", + } + } + + // Two user fields link the same template — the ambiguity this test triggers. + for range 2 { + _, sErr = th.Store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: celSafeName(), + Type: model.PropertyFieldTypeSelect, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + LinkedFieldID: &tmpl.ID, + Attrs: linkedAttrs(), + }) + require.NoError(t, sErr) + } + + channelFieldName := celSafeName() + _, sErr = th.Store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: channelFieldName, + Type: model.PropertyFieldTypeSelect, + ObjectType: model.PropertyFieldObjectTypeChannel, + TargetType: string(model.PropertyFieldTargetLevelSystem), + LinkedFieldID: &tmpl.ID, + Attrs: linkedAttrs(), + }) + require.NoError(t, sErr) + + resolver, rErr := newMaskingResolver(th.App, rctx, callerID) + require.Nil(t, rErr) + + // The Resolve path propagates the ambiguity error rather than guessing a + // sibling (the batch precompute path instead logs and fails the field closed). + _, err := resolver.Resolve(model.PropertyFieldObjectTypeChannel, channelFieldName) + require.Error(t, err) + var appErr *model.AppError + require.ErrorAs(t, err, &appErr) + require.Equal(t, "app.pap.masking.ambiguous_sibling.app_error", appErr.Id) +} + +// TestAppMaskingResolver_ChannelFieldProtectedSiblingPublic guards the fail-open +// hole where the channel field is protected (shared_only) but its user-side +// sibling is public (the default when access mode is unset — linked fields do +// NOT inherit access mode at creation). The access mode must come from the +// channel field, not the sibling, and because a public sibling's options are +// not held-filtered, the select values fail closed (all hidden) rather than +// leaking every option name. +func TestAppMaskingResolver_ChannelFieldProtectedSiblingPublic(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + rctx := request.TestContext(t) + + cpaGroup, gErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, gErr) + groupID := cpaGroup.ID + + callerID := model.NewId() + + optA := model.NewId() + optB := model.NewId() + options := []any{ + map[string]any{"id": optA, "name": "A"}, + map[string]any{"id": optB, "name": "B"}, + } + + tmpl, sErr := th.Store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: celSafeName(), + Type: model.PropertyFieldTypeSelect, + ObjectType: model.PropertyFieldObjectTypeTemplate, + TargetType: string(model.PropertyFieldTargetLevelSystem), + Attrs: model.StringInterface{model.PropertyFieldAttributeOptions: options}, + }) + require.NoError(t, sErr) + + // User sibling: PUBLIC (access mode unset) — the misconfiguration the + // resolver must not trust for access mode. + _, sErr = th.Store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: celSafeName(), + Type: model.PropertyFieldTypeSelect, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + LinkedFieldID: &tmpl.ID, + Attrs: model.StringInterface{model.PropertyFieldAttributeOptions: options}, + }) + require.NoError(t, sErr) + + // Channel field: shared_only (protected). + channelFieldName := celSafeName() + _, sErr = th.Store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: channelFieldName, + Type: model.PropertyFieldTypeSelect, + ObjectType: model.PropertyFieldObjectTypeChannel, + TargetType: string(model.PropertyFieldTargetLevelSystem), + LinkedFieldID: &tmpl.ID, + Attrs: model.StringInterface{ + model.PropertyFieldAttributeOptions: options, + model.PropertyAttrsProtected: true, + model.PropertyAttrsAccessMode: model.PropertyAccessModeSharedOnly, + }, + }) + require.NoError(t, sErr) + + resolver, rErr := newMaskingResolver(th.App, rctx, callerID) + require.Nil(t, rErr) + + info, err := resolver.Resolve(model.PropertyFieldObjectTypeChannel, channelFieldName) + require.NoError(t, err) + require.Equal(t, model.MaskingFieldAccessSharedOnly, info.Access, + "access mode must come from the protected channel field, not the public sibling") + assert.True(t, info.IsValueHidden("A"), "protected channel field with a public sibling must fail closed, not leak option names") + assert.True(t, info.IsValueHidden("B"), "protected channel field with a public sibling must fail closed, not leak option names") +} diff --git a/server/channels/app/access_control_test.go b/server/channels/app/access_control_test.go index 3e3f52b4dff..6eeab165b86 100644 --- a/server/channels/app/access_control_test.go +++ b/server/channels/app/access_control_test.go @@ -3424,9 +3424,14 @@ func TestCPAFieldIsProtectedForChannelAdmin(t *testing.T) { func TestRedactProtectedActualValuesInTree(t *testing.T) { mainHelper.Parallel(t) - protected := map[string]struct{}{ - "Clearance": {}, - "NetworkZone": {}, + protected := protectedCPAAttributes{ + user: map[string]struct{}{ + "Clearance": {}, + "NetworkZone": {}, + }, + resource: map[string]struct{}{ + "Sensitivity": {}, + }, } t.Run("redacts ActualValue on protected leaves at every depth", func(t *testing.T) { @@ -3453,6 +3458,19 @@ func TestRedactProtectedActualValuesInTree(t *testing.T) { Attribute: "user.attributes.Region", ActualValue: "us", }, + // Protected channel attribute: blanked against the + // resource set, not the user set. + { + Kind: model.PolicySimulationEvaluationKindCompare, + Attribute: "resource.attributes.Sensitivity", + ActualValue: "secret", + }, + // Public channel attribute: preserved. + { + Kind: model.PolicySimulationEvaluationKindCompare, + Attribute: "resource.attributes.Region", + ActualValue: "us-east", + }, }, }, { @@ -3479,6 +3497,10 @@ func TestRedactProtectedActualValuesInTree(t *testing.T) { assert.Empty(t, tree.Children[1].Children[0].ActualValue, "NetworkZone leaf must be blanked") assert.Equal(t, "us", tree.Children[1].Children[1].ActualValue, "Region leaf must be preserved") + // Protected channel attribute blanked; public channel attribute preserved. + assert.Empty(t, tree.Children[1].Children[2].ActualValue, "resource Sensitivity leaf must be blanked") + assert.Equal(t, "us-east", tree.Children[1].Children[3].ActualValue, "resource Region leaf must be preserved") + // Function leaf with no attribute path is left alone. assert.Equal(t, "some-internal-value", tree.Children[2].ActualValue, "non-user-attribute leaf must be preserved") }) @@ -3495,7 +3517,7 @@ func TestRedactProtectedActualValuesInTree(t *testing.T) { Attribute: "user.attributes.Clearance", ActualValue: "il5", } - redactProtectedActualValuesInTree(tree, nil) + redactProtectedActualValuesInTree(tree, protectedCPAAttributes{}) // Helper itself is unconditional but the public entry point // short-circuits before calling it with an empty set — @@ -3509,16 +3531,32 @@ func TestRedactProtectedActualValuesInTree(t *testing.T) { // paths, empty paths, and empty protected sets. func TestIsProtectedAttributePath(t *testing.T) { mainHelper.Parallel(t) - protected := map[string]struct{}{"Clearance": {}} + // Clearance is protected only on the user side, Sensitivity only on the + // resource side — so a leaf must match the set for its own root. + protected := protectedCPAAttributes{ + user: map[string]struct{}{"Clearance": {}}, + resource: map[string]struct{}{"Sensitivity": {}}, + } t.Run("returns true for the canonical user.attributes. form", func(t *testing.T) { assert.True(t, isProtectedAttributePath("user.attributes.Clearance", protected)) }) - t.Run("returns false for non-user-attribute paths", func(t *testing.T) { - // Resource / session / channel paths must not collide with - // the user-attributes namespace — only `user.attributes.*` - // is in scope for the CPA visibility filter. + t.Run("returns true for the canonical resource.attributes. form", func(t *testing.T) { + assert.True(t, isProtectedAttributePath("resource.attributes.Sensitivity", protected)) + }) + + t.Run("matches each root against its own set, not the other's", func(t *testing.T) { + // Clearance is protected user-side but not resource-side, and + // vice versa for Sensitivity — the sets must not cross. + assert.False(t, isProtectedAttributePath("resource.attributes.Clearance", protected)) + assert.False(t, isProtectedAttributePath("user.attributes.Sensitivity", protected)) + }) + + t.Run("returns false for non-CPA paths", func(t *testing.T) { + // Session / native / bare resource selectors carry no + // `.attributes.` segment — only `user.attributes.*` and + // `resource.attributes.*` are in scope for the CPA filter. assert.False(t, isProtectedAttributePath("session.network_status", protected)) assert.False(t, isProtectedAttributePath("resource.id", protected)) assert.False(t, isProtectedAttributePath("channel.member_count", protected)) @@ -3526,13 +3564,16 @@ func TestIsProtectedAttributePath(t *testing.T) { t.Run("returns false for paths whose suffix is not in the protected set", func(t *testing.T) { assert.False(t, isProtectedAttributePath("user.attributes.Region", protected)) + assert.False(t, isProtectedAttributePath("resource.attributes.Region", protected)) }) t.Run("returns false for empty inputs", func(t *testing.T) { assert.False(t, isProtectedAttributePath("", protected)) - assert.False(t, isProtectedAttributePath("user.attributes.Clearance", nil)) + assert.False(t, isProtectedAttributePath("user.attributes.Clearance", protectedCPAAttributes{})) assert.False(t, isProtectedAttributePath("user.attributes.", protected), "empty suffix must not match — that's a malformed path, not a protected reference") + assert.False(t, isProtectedAttributePath("resource.attributes.", protected), + "empty suffix must not match — that's a malformed path, not a protected reference") }) } @@ -5416,7 +5457,7 @@ func TestGetAccessControlFieldsAutocomplete_ExcludesNonUserFields(t *testing.T) require.NoError(t, err) } - fields, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, strings.Repeat("0", 26), 100, th.BasicUser.Id) + fields, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, "", false, strings.Repeat("0", 26), 100, th.BasicUser.Id) require.Nil(t, appErr) for _, f := range fields { @@ -5431,6 +5472,116 @@ func TestGetAccessControlFieldsAutocomplete_ExcludesNonUserFields(t *testing.T) assert.Contains(t, fieldIDs, userField.ID, "user CPA field must appear in autocomplete results") } +// When scoped to a channel, autocomplete additionally returns channel-object-type +// CPA fields (resource.attributes.*), each tagged with its ObjectType, alongside +// the user fields. +func TestGetAccessControlFieldsAutocomplete_IncludesChannelFieldsWhenScoped(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + th.ConfigStore.SetReadOnlyFF(false) + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.FeatureFlags.ResourceAttributesInPolicies = true + }) + + rctx := request.TestContext(t) + + cpaGroup, cErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, cErr) + + userField, appErr := th.App.CreatePropertyField(rctx, &model.PropertyField{ + GroupID: cpaGroup.ID, + Name: celSafeName(), + Type: model.PropertyFieldTypeSelect, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + }, false, "") + require.Nil(t, appErr) + + channelField, appErr := th.App.CreatePropertyField(rctx, &model.PropertyField{ + GroupID: cpaGroup.ID, + Name: celSafeName(), + Type: model.PropertyFieldTypeSelect, + ObjectType: model.PropertyFieldObjectTypeChannel, + TargetType: string(model.PropertyFieldTargetLevelSystem), + }, false, "") + require.Nil(t, appErr) + + fields, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, th.BasicChannel.Id, false, strings.Repeat("0", 26), 100, th.BasicUser.Id) + require.Nil(t, appErr) + + byID := make(map[string]*model.PropertyField, len(fields)) + for _, f := range fields { + byID[f.ID] = f + } + require.Contains(t, byID, userField.ID, "user CPA field must appear when scoped to a channel") + require.Contains(t, byID, channelField.ID, "channel CPA field must appear when scoped to a channel") + assert.Equal(t, model.PropertyFieldObjectTypeChannel, byID[channelField.ID].ObjectType, + "channel field must be tagged with its ObjectType") +} + +// includeResourceFields=true is how a caller with no channel to scope by asks +// for channel-object-type fields anyway. Verify the flag alone (channelID="") +// surfaces channel fields, and that without it channel fields are excluded. +func TestGetAccessControlFieldsAutocomplete_IncludeResourceFieldsFlag(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + th.ConfigStore.SetReadOnlyFF(false) + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.FeatureFlags.ResourceAttributesInPolicies = true + }) + + rctx := request.TestContext(t) + + cpaGroup, cErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, cErr) + + channelField, appErr := th.App.CreatePropertyField(rctx, &model.PropertyField{ + GroupID: cpaGroup.ID, + Name: celSafeName(), + Type: model.PropertyFieldTypeSelect, + ObjectType: model.PropertyFieldObjectTypeChannel, + TargetType: string(model.PropertyFieldTargetLevelSystem), + }, false, "") + require.Nil(t, appErr) + + contains := func(fields []*model.PropertyField, id string) bool { + for _, f := range fields { + if f.ID == id { + return true + } + } + return false + } + + withFlag, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, "", true, strings.Repeat("0", 26), 100, th.BasicUser.Id) + require.Nil(t, appErr) + assert.True(t, contains(withFlag, channelField.ID), + "channel field must appear when includeResourceFields=true even with no channel scope") + + withoutFlag, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, "", false, strings.Repeat("0", 26), 100, th.BasicUser.Id) + require.Nil(t, appErr) + assert.False(t, contains(withoutFlag, channelField.ID), + "channel field must not appear without a channel scope or the flag") + + // With the feature off, neither entry path offers a channel field. This is what + // keeps every editor from being able to author a resource rule, so assert both + // the request flag and a channel scope are powerless on their own. + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.FeatureFlags.ResourceAttributesInPolicies = false + }) + + gatedByRequestFlag, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, "", true, strings.Repeat("0", 26), 100, th.BasicUser.Id) + require.Nil(t, appErr) + assert.False(t, contains(gatedByRequestFlag, channelField.ID), + "channel field must not appear via includeResourceFields while the feature is off") + assert.NotEmpty(t, gatedByRequestFlag, "user attributes must still be returned while the feature is off") + + gatedByScope, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, th.BasicChannel.Id, false, strings.Repeat("0", 26), 100, th.BasicUser.Id) + require.Nil(t, appErr) + assert.False(t, contains(gatedByScope, channelField.ID), + "channel field must not appear via a channel scope while the feature is off") +} + // Verify that the team join path (channelID="") produces a subject with no // channel-scoped role — the user holds no channel role at team-join time, so // attaching one would produce a misleading subject for the membership decision. @@ -5479,7 +5630,7 @@ func TestGetAccessControlFieldsAutocompleteNativeAttributes(t *testing.T) { t.Run("first page prepends native attributes", func(t *testing.T) { // The API maps an empty first page to a 26-zero sentinel cursor. - fields, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, strings.Repeat("0", 26), 50, anonymousCallerId) + fields, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, "", false, strings.Repeat("0", 26), 50, anonymousCallerId) require.Nil(t, appErr) seen := map[string]bool{} @@ -5497,7 +5648,7 @@ func TestGetAccessControlFieldsAutocompleteNativeAttributes(t *testing.T) { }) t.Run("subsequent pages omit native attributes", func(t *testing.T) { - fields, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, cpaField.ID, 50, anonymousCallerId) + fields, appErr := th.App.GetAccessControlFieldsAutocomplete(rctx, "", false, cpaField.ID, 50, anonymousCallerId) require.Nil(t, appErr) for _, f := range fields { isNative, _ := f.Attrs[model.NativeAttributeAttrMarker].(bool) diff --git a/server/channels/app/migrations_test.go b/server/channels/app/migrations_test.go index 336a16193c6..3405c653799 100644 --- a/server/channels/app/migrations_test.go +++ b/server/channels/app/migrations_test.go @@ -545,6 +545,9 @@ func TestDoSetupSessionAttributesProperties(t *testing.T) { } require.Len(t, trimmed, len(persisted)-1) field.Attrs[model.PropertyFieldAttributeOptions] = trimmed + // System-caller context, same as the seed and the sibling upgrade + // subtests: SessionAttributesHook otherwise reads the existing field + // through RequestContextWithMaster, which panics on a nil rctx. _, _, _, err := th.Server.propertyService.UpdatePropertyFields(properties.SystemCallerContext(th.Context), group.ID, []*model.PropertyField{field}) require.NoError(t, err) diff --git a/server/channels/app/plugin_access_control.go b/server/channels/app/plugin_access_control.go index aa95718fe42..e186f99d15e 100644 --- a/server/channels/app/plugin_access_control.go +++ b/server/channels/app/plugin_access_control.go @@ -427,7 +427,9 @@ func (a *App) GetPluginAccessControlFieldsAutocomplete(rctx request.CTX, pluginI after = strings.Repeat("0", 26) } - return a.GetAccessControlFieldsAutocomplete(rctx, after, limit, actingUserID) + // No channel scope and no opt-in to resource fields: a plugin policy is not + // evaluated against a channel, so only user attributes are offered. + return a.GetAccessControlFieldsAutocomplete(rctx, "", false, after, limit, actingUserID) } // GetPluginAccessControlVisualAST converts a CEL expression to the visual diff --git a/server/channels/app/properties/access_control.go b/server/channels/app/properties/access_control.go index 7e3db6da476..50bcc9bbb9a 100644 --- a/server/channels/app/properties/access_control.go +++ b/server/channels/app/properties/access_control.go @@ -1341,10 +1341,9 @@ func (h *AccessControlHook) filterSharedOnlyScalarValue(field *model.PropertyFie // applyFieldReadAccessControl applies read access control to a single field. // Returns the field with options filtered based on the caller's access permissions. // - Public fields: returned as-is -// - User-editable fields (PermissionValues=member): returned as-is so users can see all choices -// - Source-only fields: returned with empty options if caller is not the source plugin +// - Any access mode when the caller is the field's source plugin: returned as-is // - Shared-only fields: returned with options filtered using filterSharedOnlyFieldOptions -// - Unknown access modes: treated as source-only (secure default) +// - Source-only or unknown access modes: returned with empty options (secure default) func (h *AccessControlHook) applyFieldReadAccessControl(field *model.PropertyField, callerID string) *model.PropertyField { if h.hasUnrestrictedFieldReadAccess(field, callerID) { return field diff --git a/server/channels/db/migrations/migrations.list b/server/channels/db/migrations/migrations.list index 06ca4ab90d3..6baf610d170 100644 --- a/server/channels/db/migrations/migrations.list +++ b/server/channels/db/migrations/migrations.list @@ -425,3 +425,5 @@ channels/db/migrations/postgres/000214_drop_channelmembers_autotranslation.down. channels/db/migrations/postgres/000214_drop_channelmembers_autotranslation.up.sql channels/db/migrations/postgres/000215_drop_channelmembers_autotranslation_column.down.sql channels/db/migrations/postgres/000215_drop_channelmembers_autotranslation_column.up.sql +channels/db/migrations/postgres/000216_split_attribute_view_by_object_type.down.sql +channels/db/migrations/postgres/000216_split_attribute_view_by_object_type.up.sql diff --git a/server/channels/db/migrations/postgres/000216_split_attribute_view_by_object_type.down.sql b/server/channels/db/migrations/postgres/000216_split_attribute_view_by_object_type.down.sql new file mode 100644 index 00000000000..c9abd0abce9 --- /dev/null +++ b/server/channels/db/migrations/postgres/000216_split_attribute_view_by_object_type.down.sql @@ -0,0 +1,44 @@ +-- Restore the single combined AttributeView matview (the 000200 definition, +-- user-scoped with the rank branch) and drop the per-object-type views. +DROP MATERIALIZED VIEW IF EXISTS UserAttributeView; +DROP MATERIALIZED VIEW IF EXISTS ChannelAttributeView; + +CREATE MATERIALIZED VIEW IF NOT EXISTS AttributeView AS +SELECT + pv.GroupID, + pv.TargetID, + pv.TargetType, + jsonb_object_agg( + pf.Name, + CASE + WHEN pf.Type = 'select' THEN ( + SELECT to_jsonb(options.name) + FROM jsonb_to_recordset(pf.Attrs->'options') AS options(id text, name text) + WHERE options.id = pv.Value #>> '{}' + LIMIT 1 + ) + WHEN pf.Type = 'multiselect' AND jsonb_typeof(pv.Value) = 'array' THEN ( + SELECT jsonb_agg(option_names.name) + FROM jsonb_array_elements_text(pv.Value) AS option_id + JOIN jsonb_to_recordset(pf.Attrs->'options') AS option_names(id text, name text) + ON option_id = option_names.id + ) + WHEN pf.Type = 'rank' THEN ( + SELECT jsonb_build_object( + 'name', options.name, + 'rank', options.rank + ) + FROM jsonb_to_recordset(pf.Attrs->'options') + AS options(id text, name text, rank int) + WHERE options.id = pv.Value #>> '{}' + LIMIT 1 + ) + ELSE pv.Value + END + ) AS Attributes +FROM PropertyValues pv +LEFT JOIN PropertyFields pf ON pf.ID = pv.FieldID +WHERE (pv.DeleteAt = 0 OR pv.DeleteAt IS NULL) + AND (pf.DeleteAt = 0 OR pf.DeleteAt IS NULL) + AND pf.ObjectType = 'user' +GROUP BY pv.GroupID, pv.TargetID, pv.TargetType; diff --git a/server/channels/db/migrations/postgres/000216_split_attribute_view_by_object_type.up.sql b/server/channels/db/migrations/postgres/000216_split_attribute_view_by_object_type.up.sql new file mode 100644 index 00000000000..7372cce5326 --- /dev/null +++ b/server/channels/db/migrations/postgres/000216_split_attribute_view_by_object_type.up.sql @@ -0,0 +1,89 @@ +-- Split the single AttributeView matview into per-object-type views so ABAC +-- policies can reference channel attributes independently of user attributes: +-- a channel and a user can each carry a field of the same name without the two +-- colliding in one keyed-by-name row. Splitting also makes it possible to later +-- refresh only the view whose attributes changed (callers refresh both today). +-- UserAttributeView keeps the existing user-scoped definition; ChannelAttributeView +-- is the identical shape scoped to channels. The two SELECTs differ only in the +-- pf.ObjectType filter. +DROP MATERIALIZED VIEW IF EXISTS AttributeView; + +CREATE MATERIALIZED VIEW IF NOT EXISTS UserAttributeView AS +SELECT + pv.GroupID, + pv.TargetID, + pv.TargetType, + jsonb_object_agg( + pf.Name, + CASE + WHEN pf.Type = 'select' THEN ( + SELECT to_jsonb(options.name) + FROM jsonb_to_recordset(pf.Attrs->'options') AS options(id text, name text) + WHERE options.id = pv.Value #>> '{}' + LIMIT 1 + ) + WHEN pf.Type = 'multiselect' AND jsonb_typeof(pv.Value) = 'array' THEN ( + SELECT jsonb_agg(option_names.name) + FROM jsonb_array_elements_text(pv.Value) AS option_id + JOIN jsonb_to_recordset(pf.Attrs->'options') AS option_names(id text, name text) + ON option_id = option_names.id + ) + WHEN pf.Type = 'rank' THEN ( + SELECT jsonb_build_object( + 'name', options.name, + 'rank', options.rank + ) + FROM jsonb_to_recordset(pf.Attrs->'options') + AS options(id text, name text, rank int) + WHERE options.id = pv.Value #>> '{}' + LIMIT 1 + ) + ELSE pv.Value + END + ) AS Attributes +FROM PropertyValues pv +LEFT JOIN PropertyFields pf ON pf.ID = pv.FieldID +WHERE (pv.DeleteAt = 0 OR pv.DeleteAt IS NULL) + AND (pf.DeleteAt = 0 OR pf.DeleteAt IS NULL) + AND pf.ObjectType = 'user' +GROUP BY pv.GroupID, pv.TargetID, pv.TargetType; + +CREATE MATERIALIZED VIEW IF NOT EXISTS ChannelAttributeView AS +SELECT + pv.GroupID, + pv.TargetID, + pv.TargetType, + jsonb_object_agg( + pf.Name, + CASE + WHEN pf.Type = 'select' THEN ( + SELECT to_jsonb(options.name) + FROM jsonb_to_recordset(pf.Attrs->'options') AS options(id text, name text) + WHERE options.id = pv.Value #>> '{}' + LIMIT 1 + ) + WHEN pf.Type = 'multiselect' AND jsonb_typeof(pv.Value) = 'array' THEN ( + SELECT jsonb_agg(option_names.name) + FROM jsonb_array_elements_text(pv.Value) AS option_id + JOIN jsonb_to_recordset(pf.Attrs->'options') AS option_names(id text, name text) + ON option_id = option_names.id + ) + WHEN pf.Type = 'rank' THEN ( + SELECT jsonb_build_object( + 'name', options.name, + 'rank', options.rank + ) + FROM jsonb_to_recordset(pf.Attrs->'options') + AS options(id text, name text, rank int) + WHERE options.id = pv.Value #>> '{}' + LIMIT 1 + ) + ELSE pv.Value + END + ) AS Attributes +FROM PropertyValues pv +LEFT JOIN PropertyFields pf ON pf.ID = pv.FieldID +WHERE (pv.DeleteAt = 0 OR pv.DeleteAt IS NULL) + AND (pf.DeleteAt = 0 OR pf.DeleteAt IS NULL) + AND pf.ObjectType = 'channel' +GROUP BY pv.GroupID, pv.TargetID, pv.TargetType; diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index 2c8e38659c6..718671354ce 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -852,11 +852,11 @@ func (s *RetryLayerAttributesStore) GetChannelMembersToRemove(rctx request.CTX, } -func (s *RetryLayerAttributesStore) GetSubject(rctx request.CTX, ID string, groupID string) (*model.Subject, error) { +func (s *RetryLayerAttributesStore) GetSubject(rctx request.CTX, ID string, groupID string, objectType string) (*model.Subject, error) { tries := 0 for { - result, err := s.AttributesStore.GetSubject(rctx, ID, groupID) + result, err := s.AttributesStore.GetSubject(rctx, ID, groupID, objectType) if err == nil { return result, nil } diff --git a/server/channels/store/sqlstore/attributes_store.go b/server/channels/store/sqlstore/attributes_store.go index a855fafd61f..b4e2e161196 100644 --- a/server/channels/store/sqlstore/attributes_store.go +++ b/server/channels/store/sqlstore/attributes_store.go @@ -24,6 +24,27 @@ type SqlAttributesStore struct { selectQueryBuilder sq.SelectBuilder } +// attributeViewsByObjectType is the single source of truth for the +// object-type-to-materialized-view mapping: both the per-type lookup and the +// refresh sweep read it, so adding an object type cannot leave a new view +// perpetually stale. +var attributeViewsByObjectType = map[string]string{ + model.PropertyFieldObjectTypeChannel: "ChannelAttributeView", + model.PropertyFieldObjectTypeUser: "UserAttributeView", +} + +// attributeViewFor maps a property-field object type to its materialized view. +// An unrecognized type is an error rather than a fallback to the user view: a +// silent coercion would surface a future miswire as a wrong-but-successful user +// lookup. +func attributeViewFor(objectType string) (string, error) { + view, ok := attributeViewsByObjectType[objectType] + if !ok { + return "", errors.Errorf("unknown object type %q for attribute view", objectType) + } + return view, nil +} + func attributesSliceColumns(prefix ...string) []string { var p string if len(prefix) == 1 { @@ -56,21 +77,38 @@ func newSqlAttributesStore(sqlStore *SqlStore, metrics einterfaces.MetricsInterf metrics: metrics, } - s.selectQueryBuilder = s.getQueryBuilder().Select(attributesSliceColumns()...).From("AttributeView") + // The From clause is chosen per call by object type (GetSubject), so the + // shared builder carries only the Select. + s.selectQueryBuilder = s.getQueryBuilder().Select(attributesSliceColumns()...) return s } func (s *SqlAttributesStore) RefreshAttributes() error { - if _, err := s.GetMaster().Exec("REFRESH MATERIALIZED VIEW AttributeView"); err != nil { - return errors.Wrap(err, "error refreshing materialized view AttributeView") + // Both per-object-type views (migration 000216) refresh on one cadence, so a + // caller does not have to know which object types it is about to read. They + // refresh in separate statements, though, so this is not an atomic snapshot: + // a read straddling a refresh can see the two views a generation apart. Every + // consumer tolerates that — values are eventually consistent and the + // membership sync re-runs on cadence and converges. Refreshing only the view + // whose attributes actually changed is a scale follow-up. Refresh order is + // unspecified (map iteration) and does not matter: each REFRESH is its own + // independent statement. + for _, view := range attributeViewsByObjectType { + if _, err := s.GetMaster().Exec("REFRESH MATERIALIZED VIEW " + view); err != nil { + return errors.Wrapf(err, "error refreshing materialized view %s", view) + } } return nil } -func (s *SqlAttributesStore) GetSubject(rctx request.CTX, ID, groupID string) (*model.Subject, error) { - query := s.selectQueryBuilder.Where(sq.And{sq.Eq{"TargetID": ID}, sq.Eq{"GroupID": groupID}}) +func (s *SqlAttributesStore) GetSubject(rctx request.CTX, ID, groupID, objectType string) (*model.Subject, error) { + view, err := attributeViewFor(objectType) + if err != nil { + return nil, err + } + query := s.selectQueryBuilder.From(view).Where(sq.And{sq.Eq{"TargetID": ID}, sq.Eq{"GroupID": groupID}}) q, args, err := query.ToSql() if err != nil { @@ -98,10 +136,10 @@ func (s *SqlAttributesStore) GetSubject(rctx request.CTX, ID, groupID string) (* func (s *SqlAttributesStore) SearchUsers(rctx request.CTX, opts model.SubjectSearchOptions) ([]*model.User, int64, error) { query := s.getQueryBuilder(). - Select(getUsersColumns()...).From("Users").LeftJoin("AttributeView ON Users.Id = AttributeView.TargetID"). + Select(getUsersColumns()...).From("Users").LeftJoin("UserAttributeView ON Users.Id = UserAttributeView.TargetID"). OrderBy("Users.Id ASC") - count := s.getQueryBuilder().Select("COUNT(*)").From("Users").LeftJoin("AttributeView ON Users.Id = AttributeView.TargetID") + count := s.getQueryBuilder().Select("COUNT(*)").From("Users").LeftJoin("UserAttributeView ON Users.Id = UserAttributeView.TargetID") if opts.Query != "" { // Wrap the CEL-derived expression in parentheses so that any top-level @@ -147,7 +185,7 @@ func (s *SqlAttributesStore) SearchUsers(rctx request.CTX, opts model.SubjectSea if opts.Cursor.TargetID != "" { argCount++ - // Paginate on Users.Id (the ORDER BY column), not AttributeView.TargetID. + // Paginate on Users.Id (the ORDER BY column), not UserAttributeView.TargetID. // The cursor value is a user id, and TargetID comes from a LEFT JOIN so it // is NULL for users with no custom-attribute row — comparing against it // silently drops those users (e.g. matches of a native-only policy). @@ -200,14 +238,14 @@ func (s *SqlAttributesStore) GetChannelMembersToRemove(rctx request.CTX, channel // Join Users so native-attribute expressions (e.g. Users.EmailVerified) // resolve here, mirroring SearchUsers on the add path. LeftJoin("Users ON Users.Id = ChannelMembers.UserId"). - LeftJoin("AttributeView ON ChannelMembers.UserId = AttributeView.TargetID"). + LeftJoin("UserAttributeView ON ChannelMembers.UserId = UserAttributeView.TargetID"). OrderBy("ChannelMembers.UserId ASC") if opts.Query != "" { // A member is removed when they do NOT satisfy the policy; a NULL result // (e.g. a missing custom attribute) counts as "does not satisfy" via // COALESCE. We must not additionally remove members just because they - // lack an AttributeView row — a native-only policy matches against the + // lack a UserAttributeView row — a native-only policy matches against the // Users table, so a user with zero custom attributes can still satisfy it. query = query.Where(sq.Expr(fmt.Sprintf("NOT COALESCE((%s), FALSE)", opts.Query), opts.Args...)) } @@ -247,7 +285,7 @@ func (s *SqlAttributesStore) GetTeamMembersToRemove(rctx request.CTX, teamID str // Join Users so native-attribute expressions (e.g. Users.EmailVerified) // resolve here, mirroring SearchUsers on the add path. LeftJoin("Users ON Users.Id = TeamMembers.UserId"). - LeftJoin("AttributeView ON TeamMembers.UserId = AttributeView.TargetID"). + LeftJoin("UserAttributeView ON TeamMembers.UserId = UserAttributeView.TargetID"). Where("TeamMembers.DeleteAt = 0"). OrderBy("TeamMembers.UserId ASC") @@ -255,7 +293,7 @@ func (s *SqlAttributesStore) GetTeamMembersToRemove(rctx request.CTX, teamID str // A member is removed when they do NOT satisfy the policy; a NULL result // (e.g. a missing custom attribute) counts as "does not satisfy" via // COALESCE. We must not additionally remove members just because they - // lack an AttributeView row — a native-only policy matches against the + // lack a UserAttributeView row — a native-only policy matches against the // Users table, so a user with zero custom attributes can still satisfy it. query = query.Where(sq.Expr(fmt.Sprintf("NOT COALESCE((%s), FALSE)", opts.Query), opts.Args...)) } diff --git a/server/channels/store/sqlstore/migration_000185_test.go b/server/channels/store/sqlstore/migration_000185_test.go index bb6a37fa990..ab0b5e635fd 100644 --- a/server/channels/store/sqlstore/migration_000185_test.go +++ b/server/channels/store/sqlstore/migration_000185_test.go @@ -149,15 +149,17 @@ func TestMigration000185(t *testing.T) { assert.Equal(t, targetUserID, val.TargetID, "value TargetID should be unchanged") assert.Equal(t, "user", val.TargetType, "value TargetType should be unchanged") - // Verify: AttributeView exists and includes the ObjectType filter (user-type fields only). + // Verify: the user attribute matview exists and includes the ObjectType + // filter (user-type fields only). Since migration 000216 the single + // AttributeView is split per object type; the user row lives in UserAttributeView. var viewDef string - err = master.Get(&viewDef, "SELECT definition FROM pg_matviews WHERE matviewname = 'attributeview'") - require.NoError(t, err, "AttributeView should exist") + err = master.Get(&viewDef, "SELECT definition FROM pg_matviews WHERE matviewname = 'userattributeview'") + require.NoError(t, err, "UserAttributeView should exist") assert.Contains(t, viewDef, "pf.objecttype", "view definition should filter by pf.ObjectType") // Verify: materialized view contains expected data after refresh. - _, err = master.ExecNoTimeout("REFRESH MATERIALIZED VIEW AttributeView") - require.NoError(t, err, "refreshing AttributeView should succeed") + _, err = master.ExecNoTimeout("REFRESH MATERIALIZED VIEW UserAttributeView") + require.NoError(t, err, "refreshing UserAttributeView should succeed") var viewRow struct { GroupID string `db:"groupid"` @@ -165,8 +167,8 @@ func TestMigration000185(t *testing.T) { TargetType string `db:"targettype"` Attributes []byte `db:"attributes"` } - err = master.Get(&viewRow, "SELECT GroupID, TargetID, TargetType, Attributes FROM AttributeView WHERE TargetID = ?", targetUserID) - require.NoError(t, err, "AttributeView should contain a row for the target user") + err = master.Get(&viewRow, "SELECT GroupID, TargetID, TargetType, Attributes FROM UserAttributeView WHERE TargetID = ?", targetUserID) + require.NoError(t, err, "UserAttributeView should contain a row for the target user") assert.Equal(t, groupID, viewRow.GroupID) assert.Equal(t, targetUserID, viewRow.TargetID) assert.Equal(t, "user", viewRow.TargetType) @@ -321,10 +323,13 @@ func TestMigration000185NoOpOnFreshDB(t *testing.T) { _, err = master.ExecNoTimeout(upSQL) assert.NoError(t, err, "up migration should be a safe no-op on fresh DB") - // Even with no CPA data, the view should be (re)created. - var viewExists bool - require.NoError(t, master.Get(&viewExists, "SELECT EXISTS (SELECT 1 FROM pg_matviews WHERE matviewname = 'attributeview')")) - assert.True(t, viewExists, "AttributeView should exist after up migration on fresh DB") + // The attribute matviews exist in the final schema. Since migration 000216 + // the single AttributeView is split into per-object-type views. + for _, view := range []string{"userattributeview", "channelattributeview"} { + var viewExists bool + require.NoError(t, master.Get(&viewExists, "SELECT EXISTS (SELECT 1 FROM pg_matviews WHERE matviewname = $1)", view)) + assert.True(t, viewExists, "%s should exist after up migration on fresh DB", view) + } _, err = master.ExecNoTimeout(downSQL) assert.NoError(t, err, "down migration should be a safe no-op on fresh DB") diff --git a/server/channels/store/sqlstore/migration_000216_test.go b/server/channels/store/sqlstore/migration_000216_test.go new file mode 100644 index 00000000000..dc230cb5d6a --- /dev/null +++ b/server/channels/store/sqlstore/migration_000216_test.go @@ -0,0 +1,116 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package sqlstore + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/mlog" +) + +func matviewExists(t *testing.T, s *SqlStore, name string) bool { + t.Helper() + var count int + err := s.GetMaster().Get(&count, + "SELECT COUNT(*) FROM pg_matviews WHERE lower(matviewname) = lower($1)", name) + require.NoError(t, err) + return count > 0 +} + +// TestMigration000216 verifies the split of AttributeView into per-object-type +// views: after the migration both UserAttributeView and ChannelAttributeView +// exist (and the combined AttributeView is gone), each filtering to its own +// ObjectType. The down migration restores the single combined view. +func TestMigration000216(t *testing.T) { + logger := mlog.CreateTestLogger(t) + + settings, err := makeSqlSettings(model.DatabaseDriverPostgres) + if err != nil { + t.Skip(err) + } + + store, err := New(*settings, logger, nil) + require.NoError(t, err) + defer store.Close() + + // New() applies all migrations, so 000216 is already in effect. + require.True(t, matviewExists(t, store, "UserAttributeView"), "UserAttributeView should exist after migration") + require.True(t, matviewExists(t, store, "ChannelAttributeView"), "ChannelAttributeView should exist after migration") + require.False(t, matviewExists(t, store, "AttributeView"), "combined AttributeView should be gone after migration") + + // Seed one user-scoped and one channel-scoped attribute in the same group. + group, err := store.PropertyGroup().Register(&model.PropertyGroup{Name: model.NewId(), Version: model.PropertyGroupVersionV1}) + require.NoError(t, err) + groupID := group.ID + + userField, err := store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: "user_prop", + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + }) + require.NoError(t, err) + channelField, err := store.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: "channel_prop", + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeChannel, + TargetType: string(model.PropertyFieldTargetLevelSystem), + }) + require.NoError(t, err) + + userTarget := model.NewId() + channelTarget := model.NewId() + userVal, err := store.PropertyValue().Create(&model.PropertyValue{ + TargetID: userTarget, TargetType: model.PropertyValueTargetTypeUser, + GroupID: groupID, FieldID: userField.ID, Value: []byte(`"u"`), + }) + require.NoError(t, err) + channelVal, err := store.PropertyValue().Create(&model.PropertyValue{ + TargetID: channelTarget, TargetType: model.PropertyValueTargetTypeChannel, + GroupID: groupID, FieldID: channelField.ID, Value: []byte(`"c"`), + }) + require.NoError(t, err) + + t.Cleanup(func() { + store.PropertyValue().Delete(groupID, userVal.ID) //nolint:errcheck + store.PropertyValue().Delete(groupID, channelVal.ID) //nolint:errcheck + store.PropertyField().Delete(groupID, userField.ID) //nolint:errcheck + store.PropertyField().Delete(groupID, channelField.ID) //nolint:errcheck + }) + + require.NoError(t, store.Attributes().RefreshAttributes()) + + countInView := func(view, targetID string) int { + var c int + gErr := store.GetMaster().Get(&c, "SELECT COUNT(*) FROM "+view+" WHERE TargetID = $1", targetID) + require.NoError(t, gErr) + return c + } + + require.Equal(t, 1, countInView("UserAttributeView", userTarget), "user row should be in UserAttributeView") + require.Equal(t, 0, countInView("UserAttributeView", channelTarget), "channel row should not be in UserAttributeView") + require.Equal(t, 1, countInView("ChannelAttributeView", channelTarget), "channel row should be in ChannelAttributeView") + require.Equal(t, 0, countInView("ChannelAttributeView", userTarget), "user row should not be in ChannelAttributeView") + + // Down then up round-trips the view topology. + downSQL := readMigrationSQL(t, "000216_split_attribute_view_by_object_type.down.sql") + upSQL := readMigrationSQL(t, "000216_split_attribute_view_by_object_type.up.sql") + + _, err = store.GetMaster().Exec(downSQL) + require.NoError(t, err) + require.True(t, matviewExists(t, store, "AttributeView"), "down should recreate AttributeView") + require.False(t, matviewExists(t, store, "UserAttributeView"), "down should drop UserAttributeView") + require.False(t, matviewExists(t, store, "ChannelAttributeView"), "down should drop ChannelAttributeView") + + _, err = store.GetMaster().Exec(upSQL) + require.NoError(t, err) + require.True(t, matviewExists(t, store, "UserAttributeView"), "up should recreate UserAttributeView") + require.True(t, matviewExists(t, store, "ChannelAttributeView"), "up should recreate ChannelAttributeView") + require.False(t, matviewExists(t, store, "AttributeView"), "up should drop AttributeView") +} diff --git a/server/channels/store/store.go b/server/channels/store/store.go index cf386ecd106..d1110d11267 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -1243,7 +1243,7 @@ type AccessControlPolicyStore interface { type AttributesStore interface { RefreshAttributes() error - GetSubject(rctx request.CTX, ID, groupID string) (*model.Subject, error) + GetSubject(rctx request.CTX, ID, groupID, objectType string) (*model.Subject, error) SearchUsers(rctx request.CTX, opts model.SubjectSearchOptions) ([]*model.User, int64, error) GetChannelMembersToRemove(rctx request.CTX, channelID string, opts model.SubjectSearchOptions) ([]*model.ChannelMember, error) GetTeamMembersToRemove(rctx request.CTX, teamID string, opts model.SubjectSearchOptions) ([]*model.TeamMember, error) diff --git a/server/channels/store/storetest/attributes_store.go b/server/channels/store/storetest/attributes_store.go index 1bb85aa37a3..755f2ae3c0a 100644 --- a/server/channels/store/storetest/attributes_store.go +++ b/server/channels/store/storetest/attributes_store.go @@ -32,6 +32,8 @@ var ( func TestAttributesStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) { t.Run("RefreshAndGet", func(t *testing.T) { testAttributesStoreRefresh(t, rctx, ss) }) + t.Run("GetChannelSubject", func(t *testing.T) { testAttributesStoreGetChannelSubject(t, rctx, ss) }) + t.Run("EmptyMultiselect", func(t *testing.T) { testAttributesStoreEmptyMultiselect(t, rctx, ss) }) t.Run("SearchUsers", func(t *testing.T) { testAttributesStoreSearchUsers(t, rctx, ss, s) }) t.Run("SearchUsersBySubjectID", func(t *testing.T) { testAttributesStoreSearchUsersBySubjectID(t, rctx, ss, s) }) t.Run("GetChannelMembersToRemove", func(t *testing.T) { testAttributesStoreGetChannelMembersToRemove(t, rctx, ss, s) }) @@ -211,7 +213,7 @@ func testAttributesStoreRefresh(t *testing.T, rctx request.CTX, ss store.Store) // Check if the attributes are set correctly for _, user := range users { - subject, err := ss.Attributes().GetSubject(rctx, user.Id, groupID) + subject, err := ss.Attributes().GetSubject(rctx, user.Id, groupID, model.PropertyFieldObjectTypeUser) require.NoError(t, err, "couldn't get subject") require.Equal(t, user.Id, subject.ID) @@ -220,13 +222,141 @@ func testAttributesStoreRefresh(t *testing.T, rctx request.CTX, ss store.Store) }) t.Run("Get non-existing subject", func(t *testing.T) { - subject, err := ss.Attributes().GetSubject(rctx, "non-existing-id", groupID) + subject, err := ss.Attributes().GetSubject(rctx, "non-existing-id", groupID, model.PropertyFieldObjectTypeUser) require.Error(t, err, "expected error when getting non-existing subject") require.IsType(t, &store.ErrNotFound{}, err, "expected not found error") require.Nil(t, subject, "expected nil subject for non-existing ID") }) } +// testAttributesStoreGetChannelSubject verifies the per-object-type view split +// (migration 000216): a channel-scoped attribute is readable via +// GetSubject(..., "channel") from ChannelAttributeView, and is *not* visible +// through the user view — proving the two views filter by ObjectType. +func testAttributesStoreGetChannelSubject(t *testing.T, rctx request.CTX, ss store.Store) { + group, err := ss.PropertyGroup().Register(&model.PropertyGroup{Name: model.NewId(), Version: model.PropertyGroupVersionV1}) + require.NoError(t, err) + groupID := group.ID + + field, err := ss.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: "channel_prop", + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeChannel, + TargetType: string(model.PropertyFieldTargetLevelSystem), + }) + require.NoError(t, err) + + channelID := model.NewId() + val, err := json.Marshal("channel_value") + require.NoError(t, err) + pv, err := ss.PropertyValue().Create(&model.PropertyValue{ + TargetID: channelID, + TargetType: model.PropertyValueTargetTypeChannel, + GroupID: groupID, + FieldID: field.ID, + Value: val, + }) + require.NoError(t, err) + + t.Cleanup(func() { + require.NoError(t, ss.PropertyValue().Delete(groupID, pv.ID)) + require.NoError(t, ss.PropertyField().Delete(groupID, field.ID)) + }) + + require.NoError(t, ss.Attributes().RefreshAttributes()) + + t.Run("channel view returns channel attributes", func(t *testing.T) { + subject, err := ss.Attributes().GetSubject(rctx, channelID, groupID, model.PropertyFieldObjectTypeChannel) + require.NoError(t, err) + require.Equal(t, channelID, subject.ID) + require.Equal(t, model.PropertyValueTargetTypeChannel, subject.Type) + require.Equal(t, "channel_value", subject.Attributes["channel_prop"]) + }) + + t.Run("user view does not see the channel row", func(t *testing.T) { + subject, err := ss.Attributes().GetSubject(rctx, channelID, groupID, model.PropertyFieldObjectTypeUser) + require.Error(t, err) + require.IsType(t, &store.ErrNotFound{}, err) + require.Nil(t, subject) + }) +} + +// testAttributesStoreEmptyMultiselect pins the fail-closed contract for +// multiselect attributes (migration 000216): an empty multiselect resolves to +// NULL in the matview, never an empty array. The view builds the value with +// jsonb_agg over the joined option rows, and jsonb_agg over zero rows yields +// NULL — there is no NULLIF/COALESCE. A refactor that wrapped it in +// COALESCE(..., '[]') would silently flip fail-closed to fail-open (an empty tag +// set would start satisfying an "in"/membership rule) with the rest of the suite +// still green, so assert the NULL directly. +func testAttributesStoreEmptyMultiselect(t *testing.T, rctx request.CTX, ss store.Store) { + group, err := ss.PropertyGroup().Register(&model.PropertyGroup{Name: model.NewId(), Version: model.PropertyGroupVersionV1}) + require.NoError(t, err) + groupID := group.ID + + optID1, optID2 := model.NewId(), model.NewId() + field, err := ss.PropertyField().Create(&model.PropertyField{ + GroupID: groupID, + Name: "tags", + Type: model.PropertyFieldTypeMultiselect, + Attrs: map[string]any{"options": []any{ + map[string]any{"id": optID1, "name": "blue", "color": ""}, + map[string]any{"id": optID2, "name": "green", "color": ""}, + }}, + ObjectType: model.PropertyFieldObjectTypeChannel, + TargetType: string(model.PropertyFieldTargetLevelSystem), + }) + require.NoError(t, err) + + emptyChannelID := model.NewId() + fullChannelID := model.NewId() + + emptyVal, err := json.Marshal([]string{}) + require.NoError(t, err) + fullVal, err := json.Marshal([]string{optID1, optID2}) + require.NoError(t, err) + + pvEmpty, err := ss.PropertyValue().Create(&model.PropertyValue{ + TargetID: emptyChannelID, + TargetType: model.PropertyValueTargetTypeChannel, + GroupID: groupID, + FieldID: field.ID, + Value: emptyVal, + }) + require.NoError(t, err) + pvFull, err := ss.PropertyValue().Create(&model.PropertyValue{ + TargetID: fullChannelID, + TargetType: model.PropertyValueTargetTypeChannel, + GroupID: groupID, + FieldID: field.ID, + Value: fullVal, + }) + require.NoError(t, err) + + t.Cleanup(func() { + require.NoError(t, ss.PropertyValue().Delete(groupID, pvEmpty.ID)) + require.NoError(t, ss.PropertyValue().Delete(groupID, pvFull.ID)) + require.NoError(t, ss.PropertyField().Delete(groupID, field.ID)) + }) + + require.NoError(t, ss.Attributes().RefreshAttributes()) + + t.Run("empty multiselect resolves to NULL, not []", func(t *testing.T) { + subject, err := ss.Attributes().GetSubject(rctx, emptyChannelID, groupID, model.PropertyFieldObjectTypeChannel) + require.NoError(t, err) + // nil (JSON null), never []any{} — an empty slice would satisfy a + // membership rule and open the fail-open hole this test guards. + require.Nil(t, subject.Attributes["tags"]) + }) + + t.Run("populated multiselect resolves to option names", func(t *testing.T) { + subject, err := ss.Attributes().GetSubject(rctx, fullChannelID, groupID, model.PropertyFieldObjectTypeChannel) + require.NoError(t, err) + require.ElementsMatch(t, []any{"blue", "green"}, subject.Attributes["tags"]) + }) +} + func testAttributesStoreSearchUsers(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) { users, _, cleanup := createTestUsers(t, rctx, ss) t.Cleanup(cleanup) @@ -315,14 +445,14 @@ func testAttributesStoreSearchUsers(t *testing.T, rctx request.CTX, ss store.Sto }) // Regression test: pagination must page on Users.Id, not the LEFT-JOINed - // AttributeView.TargetID. A user with no custom-attribute row has a NULL + // UserAttributeView.TargetID. A user with no custom-attribute row has a NULL // TargetID, so a "TargetID > cursor" predicate silently drops them from // every page. Native-only policies (e.g. user.verified) match exactly such // users, and the membership sync always seeds a cursor, so this manifested // as the sync adding zero members while the Test modal (no cursor) showed // the full set. t.Run("Search paginates users with no attribute row", func(t *testing.T) { - // A fresh user with no custom attributes => no AttributeView row. + // A fresh user with no custom attributes => no UserAttributeView row. u := model.User{Email: MakeEmail(), Username: model.NewUsername()} _, err := ss.User().Save(rctx, &u) require.NoError(t, err, "couldn't save attribute-less user") @@ -333,7 +463,7 @@ func testAttributesStoreSearchUsers(t *testing.T, rctx request.CTX, ss store.Sto require.NoError(t, ss.Attributes().RefreshAttributes(), "couldn't refresh attributes") // Native-style predicate against the Users table; matches the user above - // despite the missing AttributeView row. + // despite the missing UserAttributeView row. subjects, _, err := ss.Attributes().SearchUsers(rctx, model.SubjectSearchOptions{ Query: "Users.Email = $1::text", Args: []any{u.Email}, @@ -433,9 +563,9 @@ func testAttributesStoreGetChannelMembersToRemove(t *testing.T, rctx request.CTX }) // Regression test: a native-attribute policy resolves against the Users - // table (the query is joined to Users), and a member with no AttributeView + // table (the query is joined to Users), and a member with no UserAttributeView // row who satisfies it must NOT be removed. Before the fix, native columns - // failed to resolve (no Users join) and the "OR AttributeView.TargetID IS + // failed to resolve (no Users join) and the "OR UserAttributeView.TargetID IS // NULL" clause removed attribute-less members outright. t.Run("native policy keeps members with no attribute row", func(t *testing.T) { extra := model.User{Email: MakeEmail(), Username: model.NewUsername()} @@ -554,9 +684,9 @@ func testAttributesStoreGetTeamMembersToRemove(t *testing.T, rctx request.CTX, s }) // Regression test: a native-attribute policy resolves against the Users - // table (the query is joined to Users), and a member with no AttributeView + // table (the query is joined to Users), and a member with no UserAttributeView // row who satisfies it must NOT be removed. Before the fix, native columns - // failed to resolve (no Users join) and the "OR AttributeView.TargetID IS + // failed to resolve (no Users join) and the "OR UserAttributeView.TargetID IS // NULL" clause removed attribute-less members outright. t.Run("native policy keeps members with no attribute row", func(t *testing.T) { extra := model.User{Email: MakeEmail(), Username: model.NewUsername()} diff --git a/server/channels/store/storetest/mocks/AttributesStore.go b/server/channels/store/storetest/mocks/AttributesStore.go index 5600c93fd63..a88283c81b0 100644 --- a/server/channels/store/storetest/mocks/AttributesStore.go +++ b/server/channels/store/storetest/mocks/AttributesStore.go @@ -45,9 +45,9 @@ func (_m *AttributesStore) GetChannelMembersToRemove(rctx request.CTX, channelID return r0, r1 } -// GetSubject provides a mock function with given fields: rctx, ID, groupID -func (_m *AttributesStore) GetSubject(rctx request.CTX, ID string, groupID string) (*model.Subject, error) { - ret := _m.Called(rctx, ID, groupID) +// GetSubject provides a mock function with given fields: rctx, ID, groupID, objectType +func (_m *AttributesStore) GetSubject(rctx request.CTX, ID string, groupID string, objectType string) (*model.Subject, error) { + ret := _m.Called(rctx, ID, groupID, objectType) if len(ret) == 0 { panic("no return value specified for GetSubject") @@ -55,19 +55,19 @@ func (_m *AttributesStore) GetSubject(rctx request.CTX, ID string, groupID strin var r0 *model.Subject var r1 error - if rf, ok := ret.Get(0).(func(request.CTX, string, string) (*model.Subject, error)); ok { - return rf(rctx, ID, groupID) + if rf, ok := ret.Get(0).(func(request.CTX, string, string, string) (*model.Subject, error)); ok { + return rf(rctx, ID, groupID, objectType) } - if rf, ok := ret.Get(0).(func(request.CTX, string, string) *model.Subject); ok { - r0 = rf(rctx, ID, groupID) + if rf, ok := ret.Get(0).(func(request.CTX, string, string, string) *model.Subject); ok { + r0 = rf(rctx, ID, groupID, objectType) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.Subject) } } - if rf, ok := ret.Get(1).(func(request.CTX, string, string) error); ok { - r1 = rf(rctx, ID, groupID) + if rf, ok := ret.Get(1).(func(request.CTX, string, string, string) error); ok { + r1 = rf(rctx, ID, groupID, objectType) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index bf5e2a20780..91cd8059bd2 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -790,10 +790,10 @@ func (s *TimerLayerAttributesStore) GetChannelMembersToRemove(rctx request.CTX, return result, err } -func (s *TimerLayerAttributesStore) GetSubject(rctx request.CTX, ID string, groupID string) (*model.Subject, error) { +func (s *TimerLayerAttributesStore) GetSubject(rctx request.CTX, ID string, groupID string, objectType string) (*model.Subject, error) { start := time.Now() - result, err := s.AttributesStore.GetSubject(rctx, ID, groupID) + result, err := s.AttributesStore.GetSubject(rctx, ID, groupID, objectType) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil { diff --git a/server/i18n/en.json b/server/i18n/en.json index 28ab15d31f3..bdaa1d1f133 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -7940,6 +7940,14 @@ "id": "app.pap.mask_expression.app_error", "translation": "Could not mask policy expression." }, + { + "id": "app.pap.masking.ambiguous_sibling.app_error", + "translation": "Multiple user attributes link the same template, so channel-attribute visibility cannot be determined unambiguously." + }, + { + "id": "app.pap.masking.unknown_object_type.app_error", + "translation": "Cannot resolve attribute visibility for an unknown object type." + }, { "id": "app.pap.merge_expression.app_error", "translation": "Could not merge policy expression with stored masked values." @@ -7984,10 +7992,18 @@ "id": "app.pap.save_policy.name_exists.app_error", "translation": "A policy with this name already exists. Please choose a different name." }, + { + "id": "app.pap.save_policy.parent_resource_attributes_team_assigned", + "translation": "This access rule uses channel attributes, which teams do not have. Remove the teams assigned to this policy before saving." + }, { "id": "app.pap.save_policy.resolver_error", "translation": "Could not initialise attribute resolver for policy save." }, + { + "id": "app.pap.save_policy.resource_attributes_disabled", + "translation": "Access rules cannot use channel attributes on this server. This capability is not enabled." + }, { "id": "app.pap.save_policy.rule_name_unique.app_error", "translation": "Permission rule names must be unique within the policy." @@ -7996,6 +8012,10 @@ "id": "app.pap.save_policy.self_exclusion", "translation": "You do not satisfy one or more conditions in this policy." }, + { + "id": "app.pap.save_policy.team_resource_attributes", + "translation": "This access rule uses channel attributes, which teams do not have. Access rules with channel attributes can only be applied to channels." + }, { "id": "app.pap.search_access_control_policies.app_error", "translation": "Could not search access control policies." @@ -8004,6 +8024,10 @@ "id": "app.pap.simulate.attribute_refresh", "translation": "Failed to refresh attributes for the simulation." }, + { + "id": "app.pap.simulate.channel_required_for_resource", + "translation": "A channel is required to simulate a policy that references channel attributes." + }, { "id": "app.pap.simulate.compile_failed", "translation": "Failed to compile the policy for simulation." diff --git a/server/public/model/access_control_masking.go b/server/public/model/access_control_masking.go index 8d501658c7e..e662f8d0884 100644 --- a/server/public/model/access_control_masking.go +++ b/server/public/model/access_control_masking.go @@ -55,12 +55,17 @@ func (info *MaskingFieldInfo) IsValueHidden(lit string) bool { } } -// MaskingFieldResolver answers field-visibility questions for a named property -// attribute (the suffix after "user.attributes.", e.g. "department"). +// MaskingFieldResolver answers field-visibility questions for a property +// attribute identified by its CPA object type (PropertyFieldObjectTypeUser for +// user.attributes.*, PropertyFieldObjectTypeChannel for resource.attributes.*) +// and field name (the suffix after the ".attributes." segment, e.g. +// "department"). The object type is required because a user field and a channel +// field can share a name but differ in visibility, so each must be resolved +// against its own CPA schema. // // Implementations must be fail-closed: return a non-nil error for any lookup // that cannot be proven safe. The walker treats any resolver error as a // reason to mask all literals for that field. type MaskingFieldResolver interface { - Resolve(fieldName string) (*MaskingFieldInfo, error) + Resolve(objectType, fieldName string) (*MaskingFieldInfo, error) } diff --git a/server/public/model/access_request.go b/server/public/model/access_request.go index c09a787e15f..0220790e108 100644 --- a/server/public/model/access_request.go +++ b/server/public/model/access_request.go @@ -191,6 +191,13 @@ type SubjectSearchOptions struct { // normal user-search path. Zero value (false) preserves the full-name search // used by privileged callers (e.g. the admin CEL tester). ExcludeFullNames bool `json:"exclude_full_names,omitempty"` + // ResourceID is the channel whose custom attributes an ad-hoc expression test + // resolves resource.attributes.* against, so a resource-referencing expression + // can be previewed against one specific channel's values. Set from the + // channelId on a cel/test request. Unused when the expression references no + // resource attributes. Stored-policy search paths derive the resource from + // the policy itself and ignore this. + ResourceID string `json:"resource_id,omitempty"` } type SubjectCursor struct { diff --git a/server/public/model/config.go b/server/public/model/config.go index f285a391201..dd4ef790398 100644 --- a/server/public/model/config.go +++ b/server/public/model/config.go @@ -4167,7 +4167,7 @@ func (s *AccessControlSettings) isValid() *AppError { if *s.SyncJobIntervalSeconds < 60 { return NewAppError("Config.IsValid", "model.config.is_valid.access_control_sync_interval.app_error", nil, "", http.StatusBadRequest) } - // Refresh interval is designed to avoid spamming a refresh of the AttributeView in the database. + // Refresh interval is designed to avoid spamming a refresh of the attribute materialized views in the database. // Minimum is set to 0, so an operator can effectively disable this protection if desired. if *s.AttributeRefreshIntervalSeconds < 0 { return NewAppError("Config.IsValid", "model.config.is_valid.access_control_attribute_refresh_interval.app_error", nil, "", http.StatusBadRequest) diff --git a/server/public/model/feature_flags.go b/server/public/model/feature_flags.go index c074eac4275..9da0b195a8a 100644 --- a/server/public/model/feature_flags.go +++ b/server/public/model/feature_flags.go @@ -133,6 +133,16 @@ type FeatureFlags struct { TeamMembershipAccessControl bool + // FEATURE_FLAG_REMOVAL: ResourceAttributesInPolicies - Remove this when the + // feature is GA. Gates access rules that compare a user's attributes against + // the accessed channel's (resource.attributes.*): when off, the autocomplete + // endpoint omits channel-object-type fields, so no editor offers them, and + // saving a policy that references one is rejected. It does NOT gate + // evaluation — a rule stored while the flag was on keeps being enforced, + // because such rules deny on a missing channel value and silently dropping + // enforcement would empty every channel the policy governs. + ResourceAttributesInPolicies bool + // Enable the new mm_blocks Interactive Messages framework MmBlocksEnabled bool @@ -169,6 +179,7 @@ func (f *FeatureFlags) SetDefaults() { f.AttributeValueMasking = true f.PermissionPolicies = true f.TeamMembershipAccessControl = true + f.ResourceAttributesInPolicies = false f.ChannelPermissionPolicies = true f.PolicySimulation = true f.ContentFlagging = true diff --git a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.test.tsx b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.test.tsx index cf41a791e8a..7a173d800f6 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.test.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.test.tsx @@ -130,7 +130,7 @@ describe('CELEditor', () => { await userEvent.click(screen.getByRole('button', {name: /test access rule/i})); await waitFor(() => { - expect(mockSearch).toHaveBeenCalledWith(expression, '', '', 50); + expect(mockSearch).toHaveBeenCalledWith(expression, '', '', 50, undefined); }); expect(searchUsersForExpression).not.toHaveBeenCalled(); }); diff --git a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx index 37f2aac3009..29732da9028 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx @@ -8,7 +8,6 @@ import {FormattedMessage, useIntl} from 'react-intl'; import type {AccessControlTestResult, CELExpressionError} from '@mattermost/types/access_control'; import {SESSION_ATTRIBUTES_OBJECT_TYPE, USER_OBJECT_TYPE} from '@mattermost/types/properties_user'; -import {searchUsersForExpression} from 'mattermost-redux/actions/access_control'; import {debounce} from 'mattermost-redux/actions/helpers'; import {Client4} from 'mattermost-redux/client'; import type {ActionResult} from 'mattermost-redux/types/actions'; @@ -16,8 +15,7 @@ import type {ActionResult} from 'mattermost-redux/types/actions'; import {MonacoLanguageProvider} from './language_provider'; import CELHelpModal from '../../modals/cel_help/cel_help_modal'; -import TestResultsModal from '../../modals/policy_test/test_modal'; -import {TestButton, HelpText} from '../shared'; +import {TestButton, TestResults, HelpText} from '../shared'; import './editor.scss'; @@ -116,8 +114,9 @@ export interface CELEditorActions { /** Overrides Client4.checkAccessControlExpression. */ checkExpression?: (expression: string) => Promise; - /** Overrides the searchUsersForExpression thunk backing the built-in TestResultsModal. */ - searchUsers?: (expression: string, term: string, after: string, limit: number) => Promise>; + /** Overrides the searchUsersForExpression thunk backing the built-in TestResultsModal. + * Receives the test modal's chosen channel id as the trailing arg. */ + searchUsers?: (expression: string, term: string, after: string, limit: number, channelId?: string) => Promise>; } export interface CELEditorProps { @@ -131,6 +130,17 @@ export interface CELEditorProps { disabled?: boolean; userAttributes: CELUserAttribute[]; + /** + * Channel-object-type attributes exposed as the resource.attributes.* + * autocomplete root, letting a policy compare the requesting user against + * the accessed channel (e.g. user.attributes.clearance >= + * resource.attributes.minClearance). Empty for editors with no channel + * fields in scope (e.g. team policies), which then get no resource root. + */ + resourceAttributes?: Array<{ + attribute: string; + }>; + /** * When provided, the built-in expression-only TestResultsModal is * suppressed and the test button forwards its click to the parent. @@ -161,6 +171,7 @@ function CELEditor({ teamId, disabled = false, userAttributes, + resourceAttributes = [], onTestClick, testButtonLabel, hasMaskedRows = false, @@ -181,6 +192,15 @@ function CELEditor({ const schemas = buildCELSchemas(userAttributes); + // Only declare the resource.attributes.* root when channel fields are in + // scope, so editors that can't reference a resource (e.g. team policies) + // don't offer an empty root. + if (resourceAttributes.length > 0) { + const validName = (attr: string) => !attr.includes(' ') && attr.trim() !== ''; + schemas.resource = ['attributes']; + schemas['resource.attributes'] = resourceAttributes.map((attr) => attr.attribute).filter(validName); + } + const injectedCheckExpression = actions?.checkExpression; const editorRef = useRef(null); @@ -478,39 +498,34 @@ function CELEditor({ /> - setEditorState((prev) => ({...prev, showTestResults: true})))} - label={testButtonLabel} - disabled={disabled || hasMaskedRows || !editorState.expression || !editorState.isValid || editorState.isValidating} - disabledTooltip={ - hasMaskedRows ? - intl.formatMessage({ +
+ setEditorState((prev) => ({...prev, showTestResults: true})))} + label={testButtonLabel} + disabled={disabled || hasMaskedRows || !editorState.expression || !editorState.isValid || editorState.isValidating} + disabledTooltip={ + hasMaskedRows ? intl.formatMessage({ id: 'admin.access_control.cel_editor.masked_values_tooltip', defaultMessage: 'Test is unavailable because this policy contains restricted attribute values.', - }) : - undefined - } - /> + }) : undefined + } + /> +
{/* Built-in expression-only modal. Suppressed when the * parent provided an `onTestClick` override (used by the * permission-rule editor, which renders its own dual-lane - * SimulateAccessModal). */} + * SimulateAccessModal). With no channelId, a resource.attributes.* + * rule gets a channel-picker step inside the modal before the + * members list. */} {!onTestClick && editorState.showTestResults && ( - setEditorState((prev) => ({...prev, showTestResults: false}))} + {}, - searchUsers: (term: string, after: string, limit: number) => { - if (actions?.searchUsers) { - // Wrap in a thunk so TestResultsModal can dispatch it unchanged. - const search = actions.searchUsers; - return () => search(editorState.expression, term, after, limit); - } - return searchUsersForExpression(editorState.expression, term, after, limit, channelId, teamId); - }, - }} + onExited={() => setEditorState((prev) => ({...prev, showTestResults: false}))} + searchUsers={actions?.searchUsers} /> )} {showHelpModal && ( diff --git a/webapp/channels/src/components/admin_console/access_control/editors/shared.scss b/webapp/channels/src/components/admin_console/access_control/editors/shared.scss index 8ddbd00beeb..3cad00e64a8 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/shared.scss +++ b/webapp/channels/src/components/admin_console/access_control/editors/shared.scss @@ -54,6 +54,14 @@ } } +// Positions the Test button on the right of an editor's action row. +.access-control-test-controls { + display: flex; + flex-shrink: 0; + align-items: center; + gap: 8px; +} + .editor__help-text { color: var(--center-channel-color-72); font-size: 12px; diff --git a/webapp/channels/src/components/admin_console/access_control/editors/shared.test.tsx b/webapp/channels/src/components/admin_console/access_control/editors/shared.test.tsx index 2c83d83bee8..1cf8923d13a 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/shared.test.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/shared.test.tsx @@ -7,7 +7,7 @@ import type {UserPropertyField} from '@mattermost/types/properties_user'; import {renderWithContext, screen} from 'tests/react_testing_utils'; -import {TestButton, celPrefixForField, excludeSessionAttributes, hasUsableAttributes, isSimpleCondition, isSimpleExpression, mergeSessionAttributes, toCELEditorAttributes, allowedOperatorLabelsForField, defaultOperatorForField, isNativeBooleanField, isFieldAdvertisedOperator, isNativeMethodOperator, isValidYoungerThanDaysValue, OperatorLabel} from './shared'; +import {TestButton, celPrefixForField, excludeSessionAttributes, hasUsableAttributes, isSimpleCondition, isSimpleExpression, mergeSessionAttributes, referencesResourceAttributes, toCELEditorAttributes, allowedOperatorLabelsForField, defaultOperatorForField, isNativeBooleanField, isFieldAdvertisedOperator, isNativeMethodOperator, isValidYoungerThanDaysValue, OperatorLabel} from './shared'; const makeField = (name: string, attrs: Partial, type: UserPropertyField['type'] = 'text'): UserPropertyField => ({ id: `id-${name}`, @@ -768,3 +768,22 @@ describe('isNativeMethodOperator', () => { expect(isNativeMethodOperator('not an operator')).toBe(false); }); }); + +describe('referencesResourceAttributes', () => { + test('true for an actual resource attribute reference', () => { + expect(referencesResourceAttributes('user.attributes.clearance >= resource.attributes.minClearance')).toBe(true); + }); + + test('false when the prefix only appears inside a quoted literal', () => { + expect(referencesResourceAttributes('user.attributes.note == "resource.attributes.minClearance"')).toBe(false); + expect(referencesResourceAttributes("user.attributes.note == 'resource.attributes.minClearance'")).toBe(false); + }); + + test('true when a real reference coexists with a quoted literal', () => { + expect(referencesResourceAttributes('user.attributes.note == "resource.attributes.x" && user.attributes.c >= resource.attributes.min')).toBe(true); + }); + + test('false for a resource-free expression', () => { + expect(referencesResourceAttributes('user.attributes.team == "Sales"')).toBe(false); + }); +}); diff --git a/webapp/channels/src/components/admin_console/access_control/editors/shared.tsx b/webapp/channels/src/components/admin_console/access_control/editors/shared.tsx index ee7a96250af..9313a465b1a 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/shared.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/shared.tsx @@ -7,16 +7,31 @@ import type {MessageDescriptor} from 'react-intl'; import {Button} from '@mattermost/shared/components/button'; import {WithTooltip} from '@mattermost/shared/components/tooltip'; +import type {AccessControlTestResult} from '@mattermost/types/access_control'; import type {UserPropertyField} from '@mattermost/types/properties_user'; import {isSessionAttributeField} from '@mattermost/types/properties_user'; +import {searchUsersForExpression} from 'mattermost-redux/actions/access_control'; +import type {ActionResult} from 'mattermost-redux/types/actions'; + import Markdown from 'components/markdown'; +import TestResultsModal from '../modals/policy_test/test_modal'; + import './shared.scss'; // Sentinel emitted by the server in masked CEL expressions for values the caller cannot see. export const MASKED_VALUE_TOKEN_LITERAL = '"--------"'; +// The accessed channel's attributes, the comparison target for a rule about the +// requesting user (whose own attributes are USER_ATTRIBUTE_CEL_PREFIX, below). +export const RESOURCE_ATTRIBUTES_PREFIX = 'resource.attributes.'; + +// value_type on a visual-AST condition. Matches model.ValueType: 0 = literal, +// 1 = attribute reference (the RHS is another attribute path, e.g. a +// resource.attributes.* selector rather than a quoted constant). +export const VISUAL_AST_ATTRIBUTE_VALUE_TYPE = 1; + // CEL operator constants export enum CELOperator { EQUALS = '==', @@ -296,11 +311,24 @@ const CEL_STRING = String.raw`(?:"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*')`; // unescaped quotes that the previous `\[.*?\]` matcher would accept. const CEL_STRING_LIST = String.raw`\[\s*(?:${CEL_STRING}(?:\s*,\s*${CEL_STRING})*)?\s*\]`; +// A selector reading an attribute of the channel being accessed, which a +// comparison may use in place of a literal. +const RESOURCE_SELECTOR = String.raw`resource\.attributes\.\w+`; + // The first pattern accepts ==, != and the ranked ordinal operators -// (>=, <=, >, <) against a quoted value. >= / <= precede > / < in the -// alternation so the two-char forms match before the one-char ones. +// (>=, <=, >, <) against either a quoted value or a resource.attributes.* +// selector (comparing the user attribute to the accessed channel's). >= / <= +// precede > / < in the alternation so the two-char forms match before the +// one-char ones. const SIMPLE_CONDITION_PATTERNS: RegExp[] = [ - new RegExp(String.raw`^user\.(?:attributes|session)\.\w+\s*(==|!=|>=|<=|>|<)\s*${CEL_STRING}$`), + new RegExp(String.raw`^user\.(?:attributes|session)\.\w+\s*(==|!=|>=|<=|>|<)\s*(?:${CEL_STRING}|${RESOURCE_SELECTOR})$`), + + // Multiselect list-vs-list against the accessed channel's attribute, + // stored verbatim as a member call: the receiver is the user's multiselect + // attribute and the single argument is a resource.attributes.* selector + // (never a literal — that form is the in-chain below). + new RegExp(String.raw`^user\.(?:attributes|session)\.\w+\.(?:hasAnyOf|hasAllOf)\(${RESOURCE_SELECTOR}\)$`), + new RegExp(String.raw`^user\.(?:attributes|session)\.\w+\s+in\s+${CEL_STRING_LIST}$`), new RegExp(String.raw`^((${CEL_STRING_LIST})|${CEL_STRING})\s+in\s+user\.(?:attributes|session)\.\w+$`), new RegExp(String.raw`^user\.(?:attributes|session)\.\w+\.startsWith\(${CEL_STRING}.*?\)$`), @@ -451,6 +479,66 @@ export function TestButton({onClick, disabled, disabledTooltip, label}: TestButt return button; } +// True when an expression compares against the accessed channel's attributes. +// Such a rule can only be tested against a concrete channel's values, so the +// test modal must resolve one — the editor's own scope, or a channel picked in +// the modal's first step. +export function referencesResourceAttributes(expression: string): boolean { + // Strip quoted string literals first so a value like + // "resource.attributes.minClearance" is not mistaken for an actual + // attribute reference (which would wrongly force a test channel). + // Simple quote stripping; doesn't handle escaped quotes inside a literal, + // which these editors never emit — parse the AST if that ever changes. + const withoutLiterals = expression.replace(/'[^']*'|"[^"]*"/g, ''); + return withoutLiterals.includes(RESOURCE_ATTRIBUTES_PREFIX); +} + +interface TestResultsProps { + expression: string; + + /** Channel to resolve resource.attributes.* against, when the editor has + * one of its own (channel settings). When absent and the rule references + * resource.attributes.*, the modal opens a channel-picker step first and + * threads the chosen id into the search. */ + channelId?: string; + teamId?: string; + isStacked?: boolean; + onExited: () => void; + + /** Plugin override for the members search, forwarded from + * CELEditorActions.searchUsers. When provided it replaces the built-in + * searchUsersForExpression thunk. The picker's chosen channel id is + * threaded in as the trailing arg so a resource.attributes.* rule can be + * resolved against it (the override may ignore it if it resolves its own). */ + searchUsers?: (expression: string, term: string, after: string, limit: number, channelId?: string) => Promise>; +} + +// The built-in expression test/simulate results modal. +export function TestResults({expression, channelId, teamId, isStacked, onExited, searchUsers}: TestResultsProps): JSX.Element { + const requireChannel = !channelId && referencesResourceAttributes(expression); + return ( + {}, + searchUsers: (term: string, after: string, limit: number, pickedChannelId?: string) => { + if (searchUsers) { + // Wrap in a thunk so TestResultsModal can dispatch it unchanged. + // Thread the picker's channel (falling back to the editor's own + // scope) so a resource.attributes.* rule resolves against it — + // without this, such a rule tested here fails to sqlize server-side. + const search = searchUsers; + return () => search(expression, term, after, limit, pickedChannelId ?? channelId); + } + return searchUsersForExpression(expression, term, after, limit, pickedChannelId ?? channelId, teamId); + }, + }} + /> + ); +} + export function AddAttributeButton({onClick, disabled}: AddAttributeButtonProps): JSX.Element { return ( + )); + } + + return ( +
+
+ + setTerm(e.target.value)} + /> +
+
+ {listContent} +
+
+ ); +} diff --git a/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.scss b/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.scss index 15049b6ef69..810a076a621 100644 --- a/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.scss +++ b/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.scss @@ -9,9 +9,48 @@ .GenericModal__body { padding: 0 !important; + // The members-step search (SearchableUserList's default filter row) is + // matched to the channel-picker search so the two steps line up: same + // 32px inset (the .modal-header gutter, so the search lines up with the + // title above it), same 12px top offset, same bordered input with an + // inset magnify icon. The default row has no icon element, so it's drawn + // with a ::before (compass-icons magnify glyph). .filter-row { - padding: 0 16px; - margin: 5px 0 10px; + padding: 0 32px; + margin: 12px 0 10px; + } + + .filter-row .col-xs-12, + .filter-row .col-sm-12 { + padding: 0; + } + + .filter-row .col-xs-12 { + position: relative; + + // z-index lifts the glyph above the input, whose opaque + // background would otherwise paint over it and leave the 38px + // padding below looking like unexplained blank space. + &::before { + position: absolute; + z-index: 1; + top: 50%; + left: 12px; + color: rgba(var(--center-channel-color-rgb), 0.56); + content: "\f0349"; + font-family: "compass-icons"; + font-size: 18px; + pointer-events: none; + transform: translateY(-50%); + } + } + + // Left padding leaves room for the ::before magnify icon; the border, + // radius, and focus ring come from the shared .form-control styling + // (matched by the picker input below). + .filter-row .filter-textbox { + height: 40px; + padding: 0 12px 0 38px; } .filtered-user-list .filter-row span.member-count{ @@ -19,4 +58,146 @@ opacity: 0.8; } } + + // The back button leads the title in normal flow, so it starts at the + // .modal-header gutter (32px) like every other element in the modal. Pulling + // it out of flow into that gutter instead would keep the title at the same + // x-position across steps, but the gutter is only as wide as the button plus + // its gap, leaving the arrow flush against the modal edge. + &__title { + display: flex; + align-items: center; + gap: 8px; + } + + &__back { + display: inline-flex; + align-items: center; + justify-content: center; + padding: 0; + border: none; + background: transparent; + color: rgba(var(--center-channel-color-rgb), 0.75); + cursor: pointer; + + &:hover { + color: var(--center-channel-color); + } + + .icon { + font-size: 18px; + line-height: 1; + } + } +} + +.TestChannelPicker { + display: flex; + + // Match the members step's SearchableUserList (.filtered-user-list) so the + // modal body is the same fixed height on both steps and doesn't jump on + // transition. The search stays put; only the list scrolls internally. + height: calc(90vh - 120px); + flex-direction: column; + + // Matched to the members-step search (see .GenericModal__body .filter-row): + // same 32px inset, 12px top, bordered input with an inset magnify icon. + &__search { + position: relative; + flex: 0 0 auto; + margin: 12px 32px 10px; + + .icon-magnify { + position: absolute; + top: 50%; + left: 12px; + color: rgba(var(--center-channel-color-rgb), 0.56); + font-size: 18px; + pointer-events: none; + transform: translateY(-50%); + } + } + + &__search-input { + width: 100%; + height: 40px; + padding: 0 12px 0 38px; + border: 1px solid rgba(var(--center-channel-color-rgb), 0.12); + border-radius: 4px; + background: transparent; + + &:focus { + border-color: var(--button-bg); + box-shadow: 0 0 0 1px var(--button-bg); + outline: none; + } + } + + &__list { + min-height: 0; + flex: 1; + overflow-y: auto; + } + + &__empty { + padding: 32px 0; + color: rgba(var(--center-channel-color-rgb), 0.56); + text-align: center; + } + + &__row { + display: flex; + width: 100%; + align-items: center; + + // 32px matches .more-modal__row, the members step's list row. + padding: 8px 32px; + border: none; + background: transparent; + cursor: pointer; + text-align: left; + + &:hover { + background: rgba(var(--center-channel-color-rgb), 0.08); + } + + // Keyboard focus needs an indicator of its own — the hover wash alone is + // too faint to locate a row with. Scoped to :focus-visible so clicking a + // row does not leave a ring behind, and inset because the row is + // full-bleed inside &__list: overflow-y makes overflow-x compute to auto, + // which would clip an outset ring or a UA outline on the sides. + &:focus-visible { + background: rgba(var(--center-channel-color-rgb), 0.08); + box-shadow: inset 0 0 0 2px var(--sidebar-text-active-border); + outline: none; + } + } + + &__row-icon { + flex: 0 0 auto; + margin-right: 12px; + color: rgba(var(--center-channel-color-rgb), 0.75); + } + + &__row-text { + display: flex; + min-width: 0; + flex: 1; + flex-direction: column; + } + + &__row-name { + overflow: hidden; + color: var(--center-channel-color); + text-overflow: ellipsis; + white-space: nowrap; + } + + &__row-team { + overflow: hidden; + color: rgba(var(--center-channel-color-rgb), 0.56); + font-size: 12px; + text-overflow: ellipsis; + white-space: nowrap; + } } diff --git a/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.test.tsx b/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.test.tsx index ee606f92273..3fbe2ca8f2e 100644 --- a/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.test.tsx +++ b/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.test.tsx @@ -5,11 +5,26 @@ import React from 'react'; import type {UserProfile} from '@mattermost/types/users'; -import {renderWithContext, screen, userEvent, waitFor} from 'tests/react_testing_utils'; +import {act, renderWithContext, screen, userEvent, waitFor} from 'tests/react_testing_utils'; import {TestHelper} from 'utils/test_helper'; import TestResultsModal from './test_modal'; +// Mock the channel-picker step so the modal's step logic can be tested in +// isolation (the picker's own search/rendering is covered by its own test). +jest.mock('./test_channel_picker', () => { + return function MockTestChannelPicker({onSelect}: {onSelect: (channelId: string) => void}) { + return ( + + ); + }; +}); + // Mock the SearchableUserList component jest.mock('components/searchable_user_list/searchable_user_list_container', () => { return function MockSearchableUserList({ @@ -113,7 +128,7 @@ describe('TestResultsModal', () => { renderWithContext(); await waitFor(() => { - expect(mockSearchUsers).toHaveBeenCalledWith('', '', 50); + expect(mockSearchUsers).toHaveBeenCalledWith('', '', 50, undefined); }); }); @@ -144,7 +159,7 @@ describe('TestResultsModal', () => { await userEvent.type(searchInput, 'test search'); await waitFor(() => { - expect(mockSearchUsers).toHaveBeenCalledWith('test search', '', 50); + expect(mockSearchUsers).toHaveBeenCalledWith('test search', '', 50, undefined); }); }); @@ -171,7 +186,7 @@ describe('TestResultsModal', () => { // The nextPage function gets called with page 1 (second page), but since it's above USERS_PER_PAGE (10) // but less than USERS_TO_FETCH (50), it should use the cursor logic and call with the last user's ID await waitFor(() => { - expect(mockSearchUsers).toHaveBeenLastCalledWith('', 'user2', 50); + expect(mockSearchUsers).toHaveBeenLastCalledWith('', 'user2', 50, undefined); }); }); @@ -300,7 +315,7 @@ describe('TestResultsModal', () => { await userEvent.type(searchInput, 'search1'); await waitFor(() => { - expect(mockSearchUsers).toHaveBeenCalledWith('search1', '', 50); + expect(mockSearchUsers).toHaveBeenCalledWith('search1', '', 50, undefined); }); // Perform second search @@ -308,7 +323,7 @@ describe('TestResultsModal', () => { await userEvent.type(searchInput, 'search2'); await waitFor(() => { - expect(mockSearchUsers).toHaveBeenCalledWith('search2', '', 50); + expect(mockSearchUsers).toHaveBeenCalledWith('search2', '', 50, undefined); }); }); @@ -328,4 +343,140 @@ describe('TestResultsModal', () => { expect(dialog).toHaveAttribute('aria-label', 'Access Rule Test Results'); }); }); + + describe('channel-picker step', () => { + it('opens the members list directly (no picker) when requireChannel is false', async () => { + renderWithContext(); + + await waitFor(() => { + expect(screen.getByTestId('searchable-user-list')).toBeInTheDocument(); + }); + expect(screen.queryByTestId('mock-pick-channel')).not.toBeInTheDocument(); + expect(screen.getByText('Access Rule Test Results')).toBeInTheDocument(); + }); + + it('shows the picker step first (no user fetch) when requireChannel is true', async () => { + renderWithContext( + , + ); + + await waitFor(() => { + expect(screen.getByTestId('mock-pick-channel')).toBeInTheDocument(); + }); + + // Picker step defers the members fetch until a channel is chosen. + expect(mockSearchUsers).not.toHaveBeenCalled(); + expect(screen.queryByTestId('searchable-user-list')).not.toBeInTheDocument(); + expect(screen.getByText('Select a channel to test against')).toBeInTheDocument(); + }); + + it('threads the picked channel id into the members search', async () => { + renderWithContext( + , + ); + + const pick = await screen.findByTestId('mock-pick-channel'); + await userEvent.click(pick); + + await waitFor(() => { + expect(screen.getByTestId('searchable-user-list')).toBeInTheDocument(); + }); + expect(mockSearchUsers).toHaveBeenCalledWith('', '', 50, 'picked-channel'); + }); + + it('keeps a members search scoped to the picked channel', async () => { + renderWithContext( + , + ); + + await userEvent.click(await screen.findByTestId('mock-pick-channel')); + await screen.findByTestId('searchable-user-list'); + + // handleSearch passes no channel override, so the picked channel has + // to come from the channelId the fetchUsers callback closed over. + await userEvent.type(screen.getByTestId('search-input'), 'ann'); + + await waitFor(() => { + expect(mockSearchUsers).toHaveBeenLastCalledWith('ann', '', 50, 'picked-channel'); + }); + }); + + it('ignores a stale in-flight response after the channel is re-picked', async () => { + const laterUser = TestHelper.getUserMock({ + id: 'user3', + username: 'testuser3', + email: 'test3@example.com', + }); + + let resolveStale: (value: unknown) => void = () => {}; + mockSearchUsers. + mockReturnValueOnce(() => new Promise((resolve) => { + resolveStale = resolve; + })). + mockReturnValueOnce(() => Promise.resolve({data: {users: [laterUser], total: 1}})); + + renderWithContext( + , + ); + + // First pick's response is withheld, then the admin goes back and + // picks again — the second fetch supersedes the first. + await userEvent.click(await screen.findByTestId('mock-pick-channel')); + await userEvent.click(await screen.findByLabelText('Back to channel selection')); + await userEvent.click(await screen.findByTestId('mock-pick-channel')); + + await waitFor(() => { + expect(screen.getByTestId('user-user3')).toBeInTheDocument(); + }); + + // The superseded response landing late must not replace the results. + await act(async () => { + resolveStale({data: {users: mockUsers, total: 2}}); + }); + + expect(screen.getByTestId('user-count')).toHaveTextContent('Showing 1 of 1 users'); + expect(screen.queryByTestId('user-user1')).not.toBeInTheDocument(); + }); + + it('shows a back arrow in the members view that returns to the picker', async () => { + renderWithContext( + , + ); + + const pick = await screen.findByTestId('mock-pick-channel'); + await userEvent.click(pick); + + const back = await screen.findByLabelText('Back to channel selection'); + await userEvent.click(back); + + await waitFor(() => { + expect(screen.getByTestId('mock-pick-channel')).toBeInTheDocument(); + }); + expect(screen.queryByTestId('searchable-user-list')).not.toBeInTheDocument(); + }); + + it('does not show a back arrow in a members-only modal', async () => { + renderWithContext(); + + await waitFor(() => { + expect(screen.getByTestId('searchable-user-list')).toBeInTheDocument(); + }); + expect(screen.queryByLabelText('Back to channel selection')).not.toBeInTheDocument(); + }); + }); }); diff --git a/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.tsx b/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.tsx index 48ef4ceb30b..527bd641043 100644 --- a/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.tsx +++ b/webapp/channels/src/components/admin_console/access_control/modals/policy_test/test_modal.tsx @@ -1,8 +1,8 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React, {useEffect, useState, useCallback} from 'react'; -import {FormattedMessage} from 'react-intl'; +import React, {useEffect, useRef, useState, useCallback} from 'react'; +import {FormattedMessage, useIntl} from 'react-intl'; import {useDispatch} from 'react-redux'; import {GenericModal} from '@mattermost/components'; @@ -16,6 +16,8 @@ import SearchableUserList from 'components/searchable_user_list/searchable_user_ import type {ModalData} from 'types/actions'; import type {ActionFuncAsync} from 'types/store'; +import TestChannelPicker from './test_channel_picker'; + import './test_modal.scss'; const USERS_TO_FETCH = 50; @@ -24,8 +26,17 @@ const USERS_PER_PAGE = 10; type Props = { onExited: () => void; isStacked?: boolean; + + /** + * Show a channel-picker step before the members list. Used for a + * resource.attributes.* rule the editor has no channel scope for: the + * picked channel id is threaded into searchUsers so the rule can be + * resolved against that channel's attribute values. When false the modal + * opens straight to the members list, unchanged. + */ + requireChannel?: boolean; actions: { - searchUsers: (term: string, after: string, limit: number) => ActionFuncAsync; + searchUsers: (term: string, after: string, limit: number, channelId?: string) => ActionFuncAsync; openModal?:

(modalData: ModalData

) => void; }; }; @@ -33,8 +44,10 @@ type Props = { function TestResultsModal({ onExited, isStacked = false, + requireChannel = false, actions, }: Props): JSX.Element { + const {formatMessage} = useIntl(); const dispatch = useDispatch(); const [term, setTerm] = useState(''); const [users, setUsers] = useState([]); @@ -42,9 +55,24 @@ function TestResultsModal({ const [loading, setLoading] = useState(true); const [cursorHistory, setCursorHistory] = useState([]); // Stores the 'after' cursor for page 1, page 2, etc. - const fetchUsers = useCallback(async (searchTerm: string, cursor: string, reset: boolean = false) => { + // Channel chosen in the picker step. Undefined until a channel is picked + // (or always, when requireChannel is false — the editor already supplies + // its channel through searchUsers). + const [channelId, setChannelId] = useState(undefined); + const showPicker = requireChannel && !channelId; + + // Guards against a slow earlier request overwriting a newer one's results — + // e.g. back out of the members list and pick a different channel before the + // first channel's fetch resolves. + const requestSeq = useRef(0); + + const fetchUsers = useCallback(async (searchTerm: string, cursor: string, reset: boolean = false, channelOverride?: string) => { + const seq = ++requestSeq.current; setLoading(true); - const result: ActionResult = await dispatch(actions.searchUsers(searchTerm, cursor, USERS_TO_FETCH)); + const result: ActionResult = await dispatch(actions.searchUsers(searchTerm, cursor, USERS_TO_FETCH, channelOverride ?? channelId)); + if (seq !== requestSeq.current) { + return; + } if (result?.data) { const newUsers = result.data.users; if (reset) { @@ -58,12 +86,29 @@ function TestResultsModal({ setTotal(0); } setLoading(false); - }, [dispatch, actions]); + }, [dispatch, actions, channelId]); useEffect(() => { - fetchUsers(term, ''); + // The picker step defers the initial fetch until a channel is chosen + // (handled in handleChannelSelected). + if (!requireChannel) { + fetchUsers('', ''); + } }, []); + const handleChannelSelected = (selectedChannelId: string) => { + setChannelId(selectedChannelId); + setTerm(''); + setCursorHistory([]); + fetchUsers('', '', true, selectedChannelId); + }; + + const handleBack = () => { + setChannelId(undefined); + setUsers([]); + setTotal(0); + }; + const handleSearch = (newTerm: string) => { setCursorHistory([]); setTerm(newTerm); @@ -82,13 +127,39 @@ function TestResultsModal({ fetchUsers(term, cursorForNextPage); }; - const modalTitle = ( + const pickerTitle = ( + + ); + + const resultsTitle = ( ); + // The back arrow shows only when requireChannel is true, so the admin can + // return to the channel picker; a modal invoked with a channel already + // fixed renders no back arrow. + const modalTitle = showPicker ? pickerTitle : ( + + {requireChannel && ( + + )} + {resultsTitle} + + ); + return ( - + {showPicker ? ( + + ) : ( + + )} ); } diff --git a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.test.tsx b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.test.tsx index d8583e6dba9..b290549199d 100644 --- a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.test.tsx +++ b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.test.tsx @@ -3,6 +3,7 @@ import React from 'react'; +import type {AccessControlPolicy} from '@mattermost/types/access_control'; import type {ChannelWithTeamData} from '@mattermost/types/channels'; import {useChannelAccessControlActions} from 'hooks/useChannelAccessControlActions'; @@ -248,6 +249,105 @@ describe('components/admin_console/access_control/policy_details/PolicyDetails', expect(passedNames).not.toContain('network_name'); }); + test('never hands the editor the stored marker form of a rank rule', async () => { + // Regression guard. The `policy` prop is the copy the policies list left + // in the store, and the list is filled by the search endpoint, which + // returns rules in their stored form: a rank comparison is stored + // desugared as `_rank_ge(...)`. /cel/visual_ast rejects that marker call, + // so seeding the editor with it fired a doomed parse whose failure flipped + // the editor into Advanced mode. Only fetchPolicy's rehydrated expression + // may reach the editor. + const storedForm = '_rank_ge(user.attributes.clearance, "Secret", "sxrgeknhajds3qdt5hhrm4fy3h")'; + const rehydratedForm = 'user.attributes.clearance >= "Secret"'; + + MockedTableEditor.mockClear(); + const props = { + ...defaultProps, + policy: { + id: 'policy1', + name: 'Policy 1', + type: 'parent', + rules: [{actions: ['membership'], expression: storedForm}], + } as unknown as AccessControlPolicy, + actions: { + ...defaultProps.actions, + fetchPolicy: jest.fn().mockResolvedValue({ + data: { + id: 'policy1', + name: 'Policy 1', + rules: [{actions: ['membership'], expression: rehydratedForm}], + }, + }), + }, + }; + + renderWithContext(); + + await waitFor(() => { + const values = MockedTableEditor.mock.calls.map((call) => call[0].value); + expect(values).toContain(rehydratedForm); + }); + + const values = MockedTableEditor.mock.calls.map((call) => call[0].value); + expect(values).not.toContain(storedForm); + }); + + describe('channel attribute warning notice', () => { + const NOTICE_TITLE = 'Channels without this attribute lose all members'; + + // A channel with no value for a referenced channel attribute denies every + // member, so the notice fires as soon as the rule references a channel + // attribute — before any channel is assigned, without inspecting values. + const renderWithPolicy = (expression: string) => { + const props = { + ...defaultProps, + actions: { + ...defaultProps.actions, + fetchPolicy: jest.fn().mockResolvedValue({ + data: { + id: 'policy1', + name: 'Policy 1', + rules: [{actions: ['membership'], expression}], + }, + }), + searchChannels: jest.fn().mockResolvedValue({ + data: { + channels: [], + total_count: 0, + }, + }), + }, + }; + return renderWithContext(); + }; + + test('shows as soon as a channel attribute is referenced, before any channel is assigned', async () => { + renderWithPolicy('user.attributes.clearance == resource.attributes.minClearance'); + + expect(await screen.findByText(NOTICE_TITLE)).toBeInTheDocument(); + }); + + test('stays hidden when the rule only references user attributes', async () => { + renderWithPolicy('user.attributes.clearance == "Secret"'); + + await waitFor(() => { + expect(screen.getByTestId('table-editor')).toBeInTheDocument(); + }); + expect(screen.queryByText(NOTICE_TITLE)).not.toBeInTheDocument(); + }); + + test('stays hidden when resource.attributes appears only inside a string literal', async () => { + // referencesResourceAttributes strips quoted literals first, so a value + // that happens to spell an attribute path is not a reference. + renderWithPolicy('user.attributes.name == "resource.attributes.minClearance"'); + + await waitFor(() => { + expect(screen.getByTestId('table-editor')).toBeInTheDocument(); + }); + expect(screen.queryByText(NOTICE_TITLE)).not.toBeInTheDocument(); + }); + }); + test('hasMaskedRows derivation survives Simple → Advanced → Simple mode toggles', async () => { // Regression guard: hasMaskedRows must come from the expression itself, // not from a TableEditor lifecycle callback. Toggling editor modes diff --git a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx index c3571b97f2d..4385655e69d 100644 --- a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx +++ b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx @@ -13,6 +13,7 @@ import type {ChannelSearchOpts, ChannelWithTeamData} from '@mattermost/types/cha import type {AccessControlSettings} from '@mattermost/types/config'; import type {JobTypeBase} from '@mattermost/types/jobs'; import type {UserPropertyField} from '@mattermost/types/properties_user'; +import {CHANNEL_ATTRIBUTES_OBJECT_TYPE} from '@mattermost/types/properties_user'; import type {Team} from '@mattermost/types/teams'; import type {ActionResult} from 'mattermost-redux/types/actions'; @@ -33,7 +34,7 @@ import Constants from 'utils/constants'; import ChannelList from './channel_list'; import CELEditor from '../editors/cel_editor/editor'; -import {excludeSessionAttributes, hasUsableAttributes, isSimpleExpression, toCELEditorAttributes, MASKED_VALUE_TOKEN_LITERAL} from '../editors/shared'; +import {excludeSessionAttributes, hasUsableAttributes, isSimpleExpression, referencesResourceAttributes, toCELEditorAttributes, MASKED_VALUE_TOKEN_LITERAL} from '../editors/shared'; import TableEditor from '../editors/table_editor/table_editor'; import PolicyConfirmationModal from '../modals/confirmation/confirmation_modal'; @@ -82,7 +83,17 @@ function PolicyDetails({ accessControlSettings, }: PolicyDetailsProps): JSX.Element { const [policyName, setPolicyName] = useState(policy?.name || ''); - const [expression, setExpression] = useState(getMembershipRule(policy?.rules)?.expression || ''); + + // Deliberately not seeded from `policy`: that copy comes from the policies + // list, which is filled by the search endpoint, and search returns a rule in + // its *stored* form. A rank comparison is stored desugared into the marker + // call `_rank_ge(user.attributes.x, "Secret", "")`, which + // /cel/visual_ast rejects — so seeding it makes TableEditor fire a doomed + // parse on mount, and whether that 400 or fetchPolicy below resolves first + // decides whether the editor flips to Advanced mode. fetchPolicy (the single + // GET, which rehydrates markers back to operators) is the only source, and it + // sets every other field seeded here too. + const [expression, setExpression] = useState(''); const [existingRules, setExistingRules] = useState(policy?.rules || []); const [autoSyncMembership, setAutoSyncMembership] = useState(policy?.active || false); const [serverError, setServerError] = useState(undefined); @@ -145,8 +156,26 @@ function PolicyDetails({ ), []); - // Check if there are any usable attributes for ABAC - const noUsableAttributes = attributesLoaded && !hasUsableAttributes(autocompleteResult, accessControlSettings.EnableUserManagedAttributes); + // The autocomplete mixes the requesting user's attributes (user.attributes.*) + // and the accessed channel's attributes (resource.attributes.*), tagged by + // object_type. Split them: user fields drive the left picker and the + // user.attributes.* autocomplete; channel fields back resource.attributes.*. + const {userFields, resourceFields} = useMemo(() => { + const uf: UserPropertyField[] = []; + const rf: UserPropertyField[] = []; + for (const f of autocompleteResult) { + if (f.object_type === CHANNEL_ATTRIBUTES_OBJECT_TYPE) { + rf.push(f); + } else { + uf.push(f); + } + } + return {userFields: uf, resourceFields: rf}; + }, [autocompleteResult]); + + // Check if there are any usable user attributes for ABAC (channel fields + // are comparison targets, not standalone rules, so they don't count). + const noUsableAttributes = attributesLoaded && !hasUsableAttributes(userFields, accessControlSettings.EnableUserManagedAttributes); useEffect(() => { loadPage(); @@ -164,7 +193,8 @@ function PolicyDetails({ const loadPage = async (): Promise => { // Fetch autocomplete fields first, as they are general and needed for both new and existing policies. - const fieldsPromise = abacActions.getAccessControlFields('', 100).then((result) => { + // Parent policies reference resource.attributes.* against many channels, so request channel fields too. + const fieldsPromise = abacActions.getAccessControlFields('', 100, true).then((result) => { if (result.data) { setAutocompleteResult(excludeSessionAttributes(result.data)); } @@ -425,6 +455,14 @@ function PolicyDetails({ ); }; + // A channel that has no value for a referenced channel attribute cannot be + // evaluated, and the server fails closed by denying the whole rule — so every + // member of that channel loses access. Warn as soon as the rule references any + // channel attribute, before any channel is assigned, without checking which + // channels actually carry a value: reading that would cost one property-values + // request per assigned channel (the endpoint takes a single target id). + const showChannelAttributeWarning = referencesResourceAttributes(expression); + // Deletion is blocked while the policy still has ANY assigned resource — // channels or teams. Teams aren't editable from this editor (MVF), so a // linked team must be removed from the per-team System Console page first. @@ -522,6 +560,21 @@ function PolicyDetails({ }} /> )} + {showChannelAttributeWarning && (

+ + } + text={formatMessage({ + id: 'admin.access_control.policy.edit_policy.channel_attribute_notice.text', + defaultMessage: 'If an assigned channel is missing the referenced attribute, every member of that channel is removed.', + })} + /> +
)} {}} disabled={noUsableAttributes} hasMaskedRows={hasMaskedRows} - userAttributes={toCELEditorAttributes(autocompleteResult, accessControlSettings.EnableUserManagedAttributes)} + userAttributes={toCELEditorAttributes(userFields, accessControlSettings.EnableUserManagedAttributes)} + resourceAttributes={resourceFields.map((attr) => ({ + attribute: attr.name, + }))} /> ) : ( ({ + getHistory: () => ({push: mockHistoryPush}), +})); + function makePropertyField(overrides: Partial = {}): PropertyField { return { id: 'field1', @@ -98,6 +106,35 @@ function makeChannelLinkedField(overrides: Partial = {}): Propert }; } +// A "Clearance" user field linked to the classification template ('field1'). +function makeUserLinkedField(overrides: Partial = {}): PropertyField { + return { + id: 'clearance_field1', + group_id: CLASSIFICATIONS_GROUP_NAME, + name: CLEARANCE_FIELD_NAME, + type: 'rank', + attrs: {}, + target_id: '', + target_type: CLASSIFICATIONS_FIELD_TARGET_TYPE, + object_type: CLASSIFICATIONS_USER_OBJECT_TYPE, + linked_field_id: 'field1', + create_at: 5000, + update_at: 5000, + delete_at: 0, + created_by: 'user1', + updated_by: 'user1', + ...overrides, + }; +} + +// State with ABAC enabled, which reveals the clearance attribute checkbox. +const ABAC_STATE = { + entities: { + users: {currentUserId: MOCK_USER_ID}, + admin: {config: {AccessControlSettings: {EnableAttributeBasedAccessControl: true}}}, + }, +}; + function makeSystemValue(fieldId: string, optionId: string): PropertyValue { return { id: 'value1', @@ -471,6 +508,57 @@ describe('ClassificationMarkings component', () => { ).toBeInTheDocument(); }); + test('should hide the informational notice while the clearance attribute is enabled', async () => { + const field = makePropertyField({attrs: {options: [{id: 'lvl1', name: 'UNCLASSIFIED', color: '#007A33', rank: 1}]}}); + const linked = makeLinkedField({attrs: {actions: []}}); + const channel = makeChannelLinkedField(); + const clearance = makeUserLinkedField(); + + // Clearance exists, so the levels are enforced and the notice is untrue. + // Second user-object-type page comes back empty to end pagination. + let userCalls = 0; + jest.spyOn(Client4, 'getPropertyFields').mockImplementation(async (_group, objectType) => { + switch (objectType) { + case CLASSIFICATIONS_TEMPLATE_OBJECT_TYPE: + return [field]; + case CLASSIFICATIONS_SYSTEM_OBJECT_TYPE: + return [linked]; + case CLASSIFICATIONS_CHANNEL_OBJECT_TYPE: + return [channel]; + default: + return (userCalls++ % 2 === 0) ? [clearance] : []; + } + }); + + renderWithContext(, ABAC_STATE); + await screen.findByTestId('clearanceAttributeCheckbox'); + + expect(screen.getByTestId('clearanceAttributeCheckbox')).toBeChecked(); + expect( + screen.queryByRole('heading', {name: 'Classification markings are informational only'}), + ).not.toBeInTheDocument(); + + // ...and it comes back the moment enforcement is turned off again. + await userEvent.setup().click(screen.getByTestId('clearanceAttributeCheckbox')); + expect( + await screen.findByRole('heading', {name: 'Classification markings are informational only'}), + ).toBeInTheDocument(); + await act(async () => {}); + }); + + test('should navigate to the membership policies page from the clearance help text', async () => { + const field = makePropertyField({attrs: {options: [{id: 'lvl1', name: 'UNCLASSIFIED', color: '#007A33', rank: 1}]}}); + jest.spyOn(Client4, 'getPropertyFields').mockImplementation(async (_group, objectType) => { + return objectType === CLASSIFICATIONS_TEMPLATE_OBJECT_TYPE ? [field] : []; + }); + + renderWithContext(, ABAC_STATE); + await screen.findByTestId('clearanceAttributeCheckbox'); + + await userEvent.setup().click(screen.getByText('membership policy')); + expect(mockHistoryPush).toHaveBeenCalledWith('/admin_console/system_attributes/membership_policies'); + }); + test('should render disabled state when no existing field', async () => { jest.spyOn(Client4, 'getPropertyFields').mockResolvedValueOnce([]); @@ -1055,7 +1143,8 @@ describe('GlobalClassificationIndicators section', () => { jest.spyOn(Client4, 'getPropertyFields'). mockResolvedValueOnce([field]). mockResolvedValueOnce([linked]). - mockResolvedValueOnce([]); + mockResolvedValueOnce([]). // channel-linked field lookup during disable -> none + mockResolvedValueOnce([]); // clearance user field lookup during disable -> none const deleteOrder: string[] = []; const deleteFieldSpy = jest.spyOn(Client4, 'deletePropertyField'); @@ -1205,6 +1294,111 @@ describe('Channel classification linked field branches', () => { expect(fieldsById[existingChannelField.id]).toEqual(existingChannelField); }); + test('should create the linked Clearance user field on save when the clearance checkbox is enabled (ABAC on)', async () => { + const field = makePropertyField({attrs: {options: [{id: 'lvl1', name: 'UNCLASSIFIED', color: '#007A33', rank: 1}]}}); + const linked = makeLinkedField({attrs: {actions: []}}); + const channel = makeChannelLinkedField(); + + // No existing clearance field; everything else already exists (patch, not create). + jest.spyOn(Client4, 'getPropertyFields').mockImplementation(async (_group, objectType) => { + switch (objectType) { + case CLASSIFICATIONS_TEMPLATE_OBJECT_TYPE: + return [field]; + case CLASSIFICATIONS_SYSTEM_OBJECT_TYPE: + return [linked]; + case CLASSIFICATIONS_CHANNEL_OBJECT_TYPE: + return [channel]; + default: + return []; // user object type: no clearance yet + } + }); + jest.spyOn(Client4, 'patchPropertyField'). + mockResolvedValueOnce(makePropertyField({attrs: {options: [{id: 'lvl1', name: 'UNCLASSIFIED', color: '#007A33', rank: 1}]}})). + mockResolvedValueOnce(makeLinkedField({attrs: {actions: []}})); + const createdClearance = makeUserLinkedField(); + const createSpy = jest.spyOn(Client4, 'createPropertyField').mockResolvedValue(createdClearance); + + const {store} = renderWithContext(, ABAC_STATE); + await screen.findByTestId('clearanceAttributeCheckbox'); + + const user = userEvent.setup(); + await user.click(screen.getByTestId('clearanceAttributeCheckbox')); + await user.click(await screen.findByText('Save')); + + await waitFor(() => { + expect(createSpy).toHaveBeenCalledWith( + CLASSIFICATIONS_GROUP_NAME, + CLASSIFICATIONS_USER_OBJECT_TYPE, + expect.objectContaining({ + name: CLEARANCE_FIELD_NAME, + type: 'rank', + linked_field_id: field.id, + attrs: expect.objectContaining({managed: 'admin', display_name: CLEARANCE_FIELD_DISPLAY_NAME}), + }), + ); + }); + await act(async () => {}); + + // Pushed into Redux eagerly, like every other field this save touches, so + // a consumer reading the properties slice sees it without a reload. + expect(store.getState().entities.properties.fields.byId[createdClearance.id]).toEqual(createdClearance); + }); + + test('should delete the linked Clearance user field on save when the clearance checkbox is disabled (ABAC on)', async () => { + const field = makePropertyField({attrs: {options: [{id: 'lvl1', name: 'UNCLASSIFIED', color: '#007A33', rank: 1}]}}); + const linked = makeLinkedField({attrs: {actions: []}}); + const channel = makeChannelLinkedField(); + const clearance = makeUserLinkedField(); + + // Two linked clearance fields: this UI only ever creates one, but every + // match must be deleted — leaving one behind would keep enforcement live + // while the saved state records it as off. + const extraClearance = makeUserLinkedField({id: 'clearance_field2', name: 'clearance_dupe'}); + + // Clearance exists: return both on the first user-object-type page, then an + // empty page to end pagination (per fetchUserLinkedFields invocation). + let userCalls = 0; + jest.spyOn(Client4, 'getPropertyFields').mockImplementation(async (_group, objectType) => { + switch (objectType) { + case CLASSIFICATIONS_TEMPLATE_OBJECT_TYPE: + return [field]; + case CLASSIFICATIONS_SYSTEM_OBJECT_TYPE: + return [linked]; + case CLASSIFICATIONS_CHANNEL_OBJECT_TYPE: + return [channel]; + default: + return (userCalls++ % 2 === 0) ? [clearance, extraClearance] : []; + } + }); + jest.spyOn(Client4, 'patchPropertyField'). + mockResolvedValueOnce(makePropertyField({attrs: {options: [{id: 'lvl1', name: 'UNCLASSIFIED', color: '#007A33', rank: 1}]}})). + mockResolvedValueOnce(makeLinkedField({attrs: {actions: []}})); + const deleteSpy = jest.spyOn(Client4, 'deletePropertyField').mockResolvedValue({status: 'OK'}); + + renderWithContext(, ABAC_STATE); + await screen.findByTestId('clearanceAttributeCheckbox'); + + const user = userEvent.setup(); + const checkbox = screen.getByTestId('clearanceAttributeCheckbox'); + expect(checkbox).toBeChecked(); + await user.click(checkbox); + await user.click(await screen.findByText('Save')); + + await waitFor(() => { + expect(deleteSpy).toHaveBeenCalledWith( + CLASSIFICATIONS_GROUP_NAME, + CLASSIFICATIONS_USER_OBJECT_TYPE, + clearance.id, + ); + expect(deleteSpy).toHaveBeenCalledWith( + CLASSIFICATIONS_GROUP_NAME, + CLASSIFICATIONS_USER_OBJECT_TYPE, + extraClearance.id, + ); + }); + await act(async () => {}); + }); + test('should delete channel-linked field before linked and template when disabling', async () => { const field = makePropertyField({ attrs: {options: [{id: 'lvl1', name: 'UNCLASSIFIED', color: '#007A33', rank: 1}]}, @@ -1215,7 +1409,8 @@ describe('Channel classification linked field branches', () => { jest.spyOn(Client4, 'getPropertyFields'). mockResolvedValueOnce([field]). // template field load mockResolvedValueOnce([linked]). // linked field load - mockResolvedValueOnce([channel]); // channel field lookup during disable + mockResolvedValueOnce([channel]). // channel field lookup during disable + mockResolvedValueOnce([]); // clearance user field lookup during disable -> none const deleteOrder: string[] = []; jest.spyOn(Client4, 'deletePropertyField').mockImplementation(async (_group, objectType, id) => { @@ -1274,7 +1469,8 @@ describe('Channel classification linked field branches', () => { jest.spyOn(Client4, 'getPropertyFields'). mockResolvedValueOnce([field]). mockResolvedValueOnce([linked]). - mockResolvedValueOnce([]); // no channel field exists + mockResolvedValueOnce([]). // no channel field exists + mockResolvedValueOnce([]); // no clearance user field exists const deletedTypes: string[] = []; jest.spyOn(Client4, 'deletePropertyField').mockImplementation(async (_group, objectType) => { @@ -1310,6 +1506,64 @@ describe('Channel classification linked field branches', () => { console.error = origError; } }); + + test('should delete an existing clearance field when disabling even with ABAC off', async () => { + const field = makePropertyField({ + attrs: {options: [{id: 'lvl1', name: 'UNCLASSIFIED', color: '#007A33', rank: 1}]}, + }); + const linked = makeLinkedField({attrs: {actions: []}}); + const clearance = makeUserLinkedField(); + + // ABAC is off now, but the clearance field was created while it was on. + // Skipping its deletion would leave a dependent and fail the template delete. + let userCalls = 0; + jest.spyOn(Client4, 'getPropertyFields').mockImplementation(async (_group, objectType) => { + switch (objectType) { + case CLASSIFICATIONS_TEMPLATE_OBJECT_TYPE: + return [field]; + case CLASSIFICATIONS_SYSTEM_OBJECT_TYPE: + return [linked]; + case CLASSIFICATIONS_CHANNEL_OBJECT_TYPE: + return []; + default: + return (userCalls++ % 2 === 0) ? [clearance] : []; + } + }); + + const deletedIds: string[] = []; + jest.spyOn(Client4, 'deletePropertyField').mockImplementation(async (_group, _objectType, id) => { + deletedIds.push(id); + return {status: 'OK'}; + }); + + const origError = console.error; + console.error = (...args: Parameters) => { + if (typeof args[0] === 'string' && args[0].includes('not configured to support act')) { + return; + } + origError(...args); + }; + + try { + renderWithContext(, BASE_STATE); + await screen.findByText('Global Classification Indicators'); + + const user = userEvent.setup(); + + await act(async () => { + await user.click(screen.getByTestId('classificationEnabledfalse')); + }); + await act(async () => { + await user.click(screen.getByText('Save')); + }); + + await waitFor(() => { + expect(deletedIds).toEqual([clearance.id, linked.id, field.id]); + }); + } finally { + console.error = origError; + } + }); }); describe('Custom preset caching and dropdown visibility', () => { diff --git a/webapp/channels/src/components/admin_console/classification_markings/classification_markings.tsx b/webapp/channels/src/components/admin_console/classification_markings/classification_markings.tsx index 17b2afb4caf..559c36ba6c7 100644 --- a/webapp/channels/src/components/admin_console/classification_markings/classification_markings.tsx +++ b/webapp/channels/src/components/admin_console/classification_markings/classification_markings.tsx @@ -10,6 +10,7 @@ import {PlusIcon} from '@mattermost/compass-icons/components'; import type {PropertyField} from '@mattermost/types/properties'; import PropertyTypes from 'mattermost-redux/action_types/properties'; +import {getAccessControlSettings} from 'mattermost-redux/selectors/entities/access_control'; import {getCurrentUserId} from 'mattermost-redux/selectors/entities/users'; import {setNavigationBlocked} from 'actions/admin_actions'; @@ -23,6 +24,8 @@ import LoadingScreen from 'components/loading_screen'; import SectionNotice from 'components/section_notice'; import AdminHeader from 'components/widgets/admin_console/admin_header'; +import {getHistory} from 'utils/browser_history'; + import { AddLevelButton, AddLevelButtonRow, @@ -34,6 +37,8 @@ import ClassificationLevelsTable from './components/classification_levels_table' import GlobalClassificationIndicators from './components/global_classification_indicators'; import type {GlobalBannerConfig} from './utils'; import { + CLEARANCE_FIELD_DISPLAY_NAME, + CLEARANCE_FIELD_NAME, DEFAULT_GLOBAL_BANNER, DISPLAY_BANNER_TOP, actionsToGlobalBanner, @@ -41,13 +46,16 @@ import { fetchClassificationField, fetchLinkedClassificationField, fetchSystemClassificationValue, + fetchUserLinkedFields, processClassificationField, saveCreateChannelLinkedField, saveCreateField, saveCreateLinkedField, + saveCreateUserLinkedField, saveDeleteChannelLinkedField, saveDeleteField, saveDeleteLinkedField, + saveDeleteUserLinkedField, savePatchField, savePatchLinkedField, saveUpsertSystemValue, @@ -59,12 +67,17 @@ import {PENDING_LEVEL_PREFIX, PRESET_CUSTOM, PRESET_EMPTY, presets} from './util import SaveChangesPanel from '../save_changes_panel'; import {AdminSection, AdminWrapper, SectionHeader, SectionHeading} from '../system_properties/controls'; +const MEMBERSHIP_POLICIES_URL = '/admin_console/system_attributes/membership_policies'; + const msg = defineMessages({ pageTitle: {id: 'admin.sidebar.classificationMarkings', defaultMessage: 'Classification Markings'}, enableTitle: {id: 'admin.classification_markings.enable.title', defaultMessage: 'Enable classification markings'}, enableDescription: {id: 'admin.classification_markings.enable.description', defaultMessage: 'Use this to enable classification markings as banners at the system and channel level. You can pre-select text and colors for your banner, as well as set a default option for consistency.'}, presetTitle: {id: 'admin.classification_markings.preset.title', defaultMessage: 'Classification preset'}, presetDescription: {id: 'admin.classification_markings.preset.description', defaultMessage: 'Select a classification preset from the dropdown menu based on your country affiliation. This will help tailor the options to your specific needs. You can also create custom classification levels.'}, + clearanceTitle: {id: 'admin.classification_markings.enforcement.clearance.title', defaultMessage: 'Clearance attribute'}, + clearanceCheckbox: {id: 'admin.classification_markings.enforcement.clearance.checkbox', defaultMessage: 'Enable clearance attribute'}, + clearanceHelp: {id: 'admin.classification_markings.enforcement.clearance.help', defaultMessage: 'Creates a ranked "Clearance" user attribute linked to these classification levels. Channel membership can then be managed with a corresponding membership policy.'}, levelsTitle: {id: 'admin.classification_markings.levels.title', defaultMessage: 'Classification levels'}, levelsDescription: {id: 'admin.classification_markings.levels.description', defaultMessage: 'Text and colors for different classification levels that will be used in the system'}, informationalNoticeTitle: {id: 'admin.classification_markings.notice.title', defaultMessage: 'Classification markings are informational only'}, @@ -93,6 +106,7 @@ export default function ClassificationMarkings({disabled}: Props) { const {formatMessage} = useIntl(); const dispatch = useDispatch(); const currentUserId = useSelector(getCurrentUserId); + const abacEnabled = Boolean(useSelector(getAccessControlSettings)?.EnableAttributeBasedAccessControl); const [loading, setLoading] = useState(true); const [loadError, setLoadError] = useState(); @@ -102,6 +116,8 @@ export default function ClassificationMarkings({disabled}: Props) { const [existingLinkedField, setExistingLinkedField] = useState(null); const [enabled, setEnabled] = useState(false); + const [clearanceEnabled, setClearanceEnabled] = useState(false); + const [initialClearanceEnabled, setInitialClearanceEnabled] = useState(false); const [presetId, setPresetId] = useState(PRESET_EMPTY); const [levels, setLevels] = useState([]); const [globalBanner, setGlobalBanner] = useState({...DEFAULT_GLOBAL_BANNER}); @@ -120,6 +136,9 @@ export default function ClassificationMarkings({disabled}: Props) { if (!enabled) { return false; } + if (clearanceEnabled !== initialClearanceEnabled) { + return true; + } if ( globalBanner.enabled !== initialGlobalBanner.enabled || globalBanner.placement !== initialGlobalBanner.placement || @@ -134,7 +153,7 @@ export default function ClassificationMarkings({disabled}: Props) { const initial = initialLevels[i]; return level.name !== initial.name || level.color !== initial.color || level.id !== initial.id || level.rank !== initial.rank; }); - }, [enabled, initialEnabled, levels, initialLevels, globalBanner, initialGlobalBanner]); + }, [enabled, initialEnabled, clearanceEnabled, initialClearanceEnabled, levels, initialLevels, globalBanner, initialGlobalBanner]); useEffect(() => { dispatch(setNavigationBlocked(hasChanges)); @@ -176,8 +195,21 @@ export default function ClassificationMarkings({disabled}: Props) { banner = actionsToGlobalBanner(actions, levelId); } + // Clearance is an ABAC-only concept; only probe for it when + // ABAC is on (keeps the load path untouched otherwise). + let hasClearance = false; + if (abacEnabled) { + const clearanceFields = await fetchUserLinkedFields(field.id); + if (cancelled) { + return; + } + hasClearance = clearanceFields.length > 0; + } + setExistingField(field); setExistingLinkedField(linkedField ?? null); + setClearanceEnabled(hasClearance); + setInitialClearanceEnabled(hasClearance); setEnabled(true); setInitialEnabled(true); setLevels(result.levels); @@ -205,12 +237,28 @@ export default function ClassificationMarkings({disabled}: Props) { return () => { cancelled = true; }; - }, [currentUserId]); + }, [currentUserId, abacEnabled]); const handleClassificationEnabledChange = useCallback((_id: string, value: boolean) => { setEnabled(value); }, []); + const handleClearanceChange = useCallback((e: React.ChangeEvent) => { + setClearanceEnabled(e.target.checked); + }, []); + + const membershipPolicyLink = useCallback((chunks: React.ReactNode) => ( + { + e.preventDefault(); + getHistory().push(MEMBERSHIP_POLICIES_URL); + }} + > + {chunks} + + ), []); + const applyPreset = useCallback((newPresetId: string) => { if (newPresetId === PRESET_CUSTOM) { setPresetId(PRESET_CUSTOM); @@ -422,8 +470,30 @@ export default function ClassificationMarkings({disabled}: Props) { dispatch({type: PropertyTypes.RECEIVED_PROPERTY_FIELDS, data: {fields: [savedTemplate, savedLinked, savedChannelField]}}); } + // Clearance user field: create or delete to match the checkbox. The + // template exists now (savedTemplate), which the linked field + // requires — this is why creation is deferred to save. Re-fetch to + // avoid duplicating if the initial load missed it. ABAC-gated to + // match the section's visibility. + if (abacEnabled) { + // Delete every match, not just the first: this UI creates one, but a + // second linked field made another way would otherwise survive and + // reappear on reload while enforcement records itself as disabled. + const currentClearance = await fetchUserLinkedFields(savedTemplate.id); + if (clearanceEnabled && currentClearance.length === 0) { + const savedClearance = await saveCreateUserLinkedField(savedTemplate.id, CLEARANCE_FIELD_NAME, CLEARANCE_FIELD_DISPLAY_NAME); + dispatch({type: PropertyTypes.RECEIVED_PROPERTY_FIELDS, data: {fields: [savedClearance]}}); + } else if (!clearanceEnabled) { + for (const cf of currentClearance) { + await saveDeleteUserLinkedField(cf.id); // eslint-disable-line no-await-in-loop + dispatch({type: PropertyTypes.PROPERTY_FIELD_DELETED, data: {fieldId: cf.id}}); + } + } + } + setExistingField(savedTemplate); setExistingLinkedField(savedLinked); + setInitialClearanceEnabled(clearanceEnabled); setLevels(result.levels); setInitialLevels(result.levels); setPresetId(result.presetId); @@ -432,12 +502,21 @@ export default function ClassificationMarkings({disabled}: Props) { setInitialEnabled(true); } else if (templateField) { // Linked fields must be deleted before the template (deletion protection). - // Order: channel field -> system field -> template. + // Order: channel field -> clearance user field -> system field -> template. const channelField = await fetchChannelClassificationField(); if (channelField) { await saveDeleteChannelLinkedField(channelField.id); dispatch({type: PropertyTypes.PROPERTY_FIELD_DELETED, data: {fieldId: channelField.id}}); } + + // Not ABAC-gated, unlike the create path: a clearance field created + // while ABAC was on outlives the setting, and leaving it behind makes + // the template delete below fail on its dependents. + const clearanceFields = await fetchUserLinkedFields(templateField.id); + for (const cf of clearanceFields) { + await saveDeleteUserLinkedField(cf.id); // eslint-disable-line no-await-in-loop + dispatch({type: PropertyTypes.PROPERTY_FIELD_DELETED, data: {fieldId: cf.id}}); + } if (linkedField) { await saveDeleteLinkedField(linkedField.id); dispatch({type: PropertyTypes.PROPERTY_FIELD_DELETED, data: {fieldId: linkedField.id}}); @@ -447,6 +526,8 @@ export default function ClassificationMarkings({disabled}: Props) { setExistingField(null); setExistingLinkedField(null); + setClearanceEnabled(false); + setInitialClearanceEnabled(false); setInitialEnabled(false); setInitialLevels([]); setLevels([]); @@ -455,7 +536,7 @@ export default function ClassificationMarkings({disabled}: Props) { setGlobalBanner({...DEFAULT_GLOBAL_BANNER}); setInitialGlobalBanner({...DEFAULT_GLOBAL_BANNER}); } - }, [enabled, existingField, existingLinkedField, levels, globalBanner, dispatch]); + }, [enabled, abacEnabled, clearanceEnabled, existingField, existingLinkedField, levels, globalBanner, dispatch]); const handleSave = useCallback(async () => { setSaveError(undefined); @@ -521,14 +602,19 @@ export default function ClassificationMarkings({disabled}: Props) { - - } - text={formatMessage(msg.informationalNoticeBody)} - /> - + {/* Only true while nothing enforces the levels. The clearance attribute + feeds a membership policy, so once it is on the markings do decide + access and this notice would contradict the section below. */} + {!clearanceEnabled && ( + + } + text={formatMessage(msg.informationalNoticeBody)} + /> + + )}
e.preventDefault()} @@ -577,6 +663,30 @@ export default function ClassificationMarkings({disabled}: Props) { )} + {enabled && abacEnabled && ( + } + helpText={ + + } + setByEnv={false} + > + + + )}
{enabled && ( diff --git a/webapp/channels/src/components/admin_console/classification_markings/utils/index.ts b/webapp/channels/src/components/admin_console/classification_markings/utils/index.ts index 66b4e942919..9ba60ab6503 100644 --- a/webapp/channels/src/components/admin_console/classification_markings/utils/index.ts +++ b/webapp/channels/src/components/admin_console/classification_markings/utils/index.ts @@ -338,3 +338,80 @@ export async function saveCreateChannelLinkedField(templateFieldId: string): Pro export async function saveDeleteChannelLinkedField(fieldId: string): Promise { await Client4.deletePropertyField(CLASSIFICATIONS_GROUP_NAME, CLASSIFICATIONS_CHANNEL_OBJECT_TYPE, fieldId); } + +// --- User field API (clearance attribute for classification enforcement) --- +// +// A user field linked to the template mirrors the classification levels as its +// option scale, so ABAC policies can gate access with `user.attributes.` +// compared against a channel's classification. Values are inherited wholesale +// from the template (no per-level mapping), exactly like the channel field. + +export const CLASSIFICATIONS_USER_OBJECT_TYPE = 'user'; + +// Default name/label for the clearance field created from this page. The name +// is fixed: CEL rule authors write it directly as user.attributes.clearance, so +// renaming it would break existing rules. It is lowercase like +// CLASSIFICATIONS_CHANNEL_FIELD_NAME; the display name is the label the System +// Console and profile popovers show. +export const CLEARANCE_FIELD_NAME = 'clearance'; +export const CLEARANCE_FIELD_DISPLAY_NAME = 'Clearance'; + +/** + * Fetches every live user field linked to the given classification template. + * The enforcement checkbox is on when this returns anything, and disabling + * classification markings deletes all of them, so it returns the whole set + * rather than just the first match. + */ +export async function fetchUserLinkedFields(templateFieldId: string): Promise { + const maxItems = 500; + let fetched = 0; + let cursorId: string | undefined; + let cursorCreateAt: number | undefined; + const matches: PropertyField[] = []; + + while (fetched < maxItems) { + const fields = await Client4.getPropertyFields( // eslint-disable-line no-await-in-loop + CLASSIFICATIONS_GROUP_NAME, + CLASSIFICATIONS_USER_OBJECT_TYPE, + CLASSIFICATIONS_FIELD_TARGET_TYPE, + CLASSIFICATIONS_FIELD_TARGET_ID, + {cursorId, cursorCreateAt}, + ); + for (const f of fields) { + if (f.delete_at === 0 && f.linked_field_id === templateFieldId) { + matches.push(f); + } + } + if (fields.length === 0) { + return matches; + } + + fetched += fields.length; + const last = fields[fields.length - 1]; + cursorId = last.id; + cursorCreateAt = last.create_at; + } + + return matches; +} + +export async function saveCreateUserLinkedField(templateFieldId: string, name: string, displayName: string): Promise { + return Client4.createPropertyField(CLASSIFICATIONS_GROUP_NAME, CLASSIFICATIONS_USER_OBJECT_TYPE, { + name, + type: 'rank' as PropertyField['type'], + target_type: CLASSIFICATIONS_FIELD_TARGET_TYPE, + target_id: CLASSIFICATIONS_FIELD_TARGET_ID, + linked_field_id: templateFieldId, + + // Admin-managed: clearance is assigned by an admin/integration, so users + // cannot self-edit their own value. + attrs: {managed: 'admin', display_name: displayName}, + permission_field: 'admin', + permission_values: 'admin', + permission_options: 'admin', + }); +} + +export async function saveDeleteUserLinkedField(fieldId: string): Promise { + await Client4.deletePropertyField(CLASSIFICATIONS_GROUP_NAME, CLASSIFICATIONS_USER_OBJECT_TYPE, fieldId); +} diff --git a/webapp/channels/src/components/admin_console/permission_policies/policy_details/permission_policy_details.test.tsx b/webapp/channels/src/components/admin_console/permission_policies/policy_details/permission_policy_details.test.tsx index da9201b9d25..2391fdadef0 100644 --- a/webapp/channels/src/components/admin_console/permission_policies/policy_details/permission_policy_details.test.tsx +++ b/webapp/channels/src/components/admin_console/permission_policies/policy_details/permission_policy_details.test.tsx @@ -153,6 +153,28 @@ describe('components/admin_console/permission_policies/policy_details/Permission jest.clearAllMocks(); }); + test('shows the load failure and hides the editor when fetchPolicy errors, even with a store-copy policy prop', async () => { + mockFetchPolicy.mockResolvedValue({error: {message: 'failed'}}); + + renderWithContext( + , + ); + + expect(await screen.findByText('Failed to load policy')).toBeInTheDocument(); + expect(screen.getByText('failed')).toBeInTheDocument(); + expect(screen.queryByTestId('table-editor')).not.toBeInTheDocument(); + expect(screen.queryByTestId('cel-editor')).not.toBeInTheDocument(); + }); + test('merges the fetched enabled session attributes into the table-mode picker', async () => { renderWithContext(); diff --git a/webapp/channels/src/components/admin_console/permission_policies/policy_details/permission_policy_details.tsx b/webapp/channels/src/components/admin_console/permission_policies/policy_details/permission_policy_details.tsx index c32a4a41363..d451230a502 100644 --- a/webapp/channels/src/components/admin_console/permission_policies/policy_details/permission_policy_details.tsx +++ b/webapp/channels/src/components/admin_console/permission_policies/policy_details/permission_policy_details.tsx @@ -11,6 +11,7 @@ import {buttonClassNames} from '@mattermost/shared/components/button'; import type {AccessControlPolicy, AccessControlPolicyRule} from '@mattermost/types/access_control'; import type {AccessControlSettings} from '@mattermost/types/config'; import type {UserPropertyField} from '@mattermost/types/properties_user'; +import {CHANNEL_ATTRIBUTES_OBJECT_TYPE} from '@mattermost/types/properties_user'; import {isPolicySimulationEnabled} from 'mattermost-redux/selectors/entities/general'; import type {ActionResult} from 'mattermost-redux/types/actions'; @@ -118,7 +119,13 @@ function PermissionPolicyDetails({ sessionAttributesEnabled, }: PermissionPolicyDetailsProps): JSX.Element { const [policyName, setPolicyName] = useState(policy?.name || ''); - const [expression, setExpression] = useState(policy?.rules?.[0]?.expression || ''); + + // Not seeded from `policy`: the list leaves the search endpoint's copy in the + // store, and search returns rules in their stored form — a rank comparison is + // stored desugared as `_rank_ge(...)`, which /cel/visual_ast rejects, so the + // editor would fire a doomed parse on mount. fetchPolicy below is the only + // source; it also sets the name, role and permissions seeded here. + const [expression, setExpression] = useState(''); const [selectedRole, setSelectedRole] = useState(policy?.roles?.[0] || 'system_user'); const [selectedPermissions, setSelectedPermissions] = useState( getPermissionActions(policy?.rules || []), @@ -131,6 +138,7 @@ function PermissionPolicyDetails({ const [attributesLoaded, setAttributesLoaded] = useState(false); const [showDeleteConfirmationModal, setShowDeleteConfirmationModal] = useState(false); const [pageLoaded, setPageLoaded] = useState(false); + const [loadFailed, setLoadFailed] = useState(false); const [showTest, setShowTest] = useState(false); const {formatMessage} = useIntl(); @@ -143,9 +151,27 @@ function PermissionPolicyDetails({ // the channel-settings Permissions Policy tab. const policySimulationEnabled = useSelector(isPolicySimulationEnabled); + // The autocomplete mixes the requesting user's attributes (user.attributes.*) + // and the accessed channel's attributes (resource.attributes.*), tagged by + // object_type. Permission policies are channel-scoped, so they may reference + // resource.attributes.*; split so user fields drive rules and channel fields + // are comparison targets. + const {userFields, resourceFields} = useMemo(() => { + const uf: UserPropertyField[] = []; + const rf: UserPropertyField[] = []; + for (const f of autocompleteResult) { + if (f.object_type === CHANNEL_ATTRIBUTES_OBJECT_TYPE) { + rf.push(f); + } else { + uf.push(f); + } + } + return {userFields: uf, resourceFields: rf}; + }, [autocompleteResult]); + // Permission policies can reference session attributes (e.g. user.session.ip_address), // so the editor stays usable even without any configured user attributes when SessionAttributes is on. - const noUsableAttributes = attributesLoaded && !sessionAttributesEnabled && !hasUsableAttributes(autocompleteResult, accessControlSettings.EnableUserManagedAttributes); + const noUsableAttributes = attributesLoaded && !sessionAttributesEnabled && !hasUsableAttributes(userFields, accessControlSettings.EnableUserManagedAttributes); const sessionFields = useEnabledSessionAttributeFields(sessionAttributesEnabled); const mergedAttributes = useMemo( @@ -162,7 +188,11 @@ function PermissionPolicyDetails({ // are recognized as simple and open in table mode. const loadPage = async (): Promise => { - const fieldsPromise = abacActions.getAccessControlFields('', 100).then((result) => { + setLoadFailed(false); + + // Permission policies can reference resource.attributes.* (the accessed + // channel), so request channel fields too. + const fieldsPromise = abacActions.getAccessControlFields('', 100, true).then((result) => { if (result.data) { setAutocompleteResult(result.data); } @@ -172,6 +202,7 @@ function PermissionPolicyDetails({ if (policyId) { const policyPromise = actions.fetchPolicy(policyId).then((result: ActionResult) => { if (result.error) { + setLoadFailed(true); setServerError(result.error.message || formatMessage({ id: 'admin.permission_policies.edit.error.load', defaultMessage: 'Failed to load policy', @@ -316,7 +347,23 @@ function PermissionPolicyDetails({ /> - {pageLoaded ? ( + {pageLoaded && loadFailed && ( +
+
+
+ +
+
+
+ )} + {pageLoaded && !loadFailed && ( <>
@@ -538,7 +585,10 @@ function PermissionPolicyDetails({ }} onValidate={() => {}} disabled={noUsableAttributes} - userAttributes={toCELEditorAttributes(mergedAttributes, accessControlSettings.EnableUserManagedAttributes)} + userAttributes={toCELEditorAttributes(mergeSessionAttributes(userFields, sessionFields), accessControlSettings.EnableUserManagedAttributes)} + resourceAttributes={resourceFields.map((attr) => ({ + attribute: attr.name, + }))} // Both editor modes route the test // button through SimulateAccessModal: @@ -830,7 +880,8 @@ function PermissionPolicyDetails({ )}
- ) : ( + )} + {!pageLoaded && (
diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.test.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.test.tsx index 5eab0407580..bf6db71fedb 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.test.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.test.tsx @@ -418,6 +418,47 @@ describe('UserPropertyDotMenu', () => { expect(updateField).not.toHaveBeenCalled(); }); + it('disables the options-editing actions for a template-linked field but leaves the rest editable', async () => { + const linkedField: UserPropertyField = { + ...baseField, + id: 'template-linked-field', + type: 'rank', + linked_field_id: 'template-field-id', + }; + + renderComponent(linkedField); + + await userEvent.click(screen.getByTestId(`user-property-field_dotmenu-${linkedField.id}`)); + + // type and options are owned by the template + expect(screen.getByRole('menuitem', {name: /Edit ranking/})).toHaveAttribute('aria-disabled', 'true'); + expect(screen.getByRole('menuitem', {name: /Link attribute to AD\/LDAP/})).toHaveAttribute('aria-disabled', 'true'); + expect(screen.getByRole('menuitem', {name: /Link attribute to SAML/})).toHaveAttribute('aria-disabled', 'true'); + expect(screen.getAllByText('Managed by a linked attribute template')).toHaveLength(3); + + // everything else stays editable + expect(screen.getByRole('menuitem', {name: /Visibility/})).not.toHaveAttribute('aria-disabled', 'true'); + expect(within(screen.getByRole('menuitemcheckbox', {name: /Editable by users/})).getByRole('button')).toBeEnabled(); + expect(screen.getByRole('menuitem', {name: /Duplicate attribute/})).not.toHaveAttribute('aria-disabled', 'true'); + expect(screen.getByRole('menuitem', {name: /Delete attribute/})).not.toHaveAttribute('aria-disabled', 'true'); + }); + + it('does not open the ranking modal for a template-linked field', async () => { + const linkedField: UserPropertyField = { + ...baseField, + id: 'template-linked-ranking', + type: 'rank', + linked_field_id: 'template-field-id', + }; + + renderComponent(linkedField); + + await userEvent.click(screen.getByTestId(`user-property-field_dotmenu-${linkedField.id}`)); + screen.getByRole('menuitem', {name: /Edit ranking/}).click(); + + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); + it('handles field duplication', async () => { renderComponent(); @@ -439,6 +480,28 @@ describe('UserPropertyDotMenu', () => { }); }); + it('duplicating a template-linked field produces an unlinked copy', async () => { + const linkedField: UserPropertyField = { + ...baseField, + id: 'template-linked-duplicate', + type: 'rank', + linked_field_id: 'template-field-id', + }; + + renderComponent(linkedField); + + await userEvent.click(screen.getByTestId(`user-property-field_dotmenu-${linkedField.id}`)); + await userEvent.click(screen.getByText(/Duplicate attribute/)); + + // A copy is a standalone field. Carrying the link over would make it + // inherit a type and option set the create request cannot send, and would + // leave a second dependent on the template. + await waitFor(() => { + expect(createField).toHaveBeenCalledWith(expect.objectContaining({name: 'test_field_copy'})); + }); + expect(createField.mock.calls[0][0]).not.toHaveProperty('linked_field_id'); + }); + it('duplicate produces _2 suffix when base name is already taken', async () => { const existingCopy = { ...baseField, diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.tsx index 934af104bde..c7024861cc2 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_dot_menu.tsx @@ -1,6 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. +import type {ReactElement, ReactNode} from 'react'; import React from 'react'; import {FormattedMessage, useIntl} from 'react-intl'; import {useDispatch} from 'react-redux'; @@ -20,7 +21,7 @@ import {slugifyForCEL} from 'utils/properties'; import AttributeModal from './attribute_modal'; import RankedSchemaModal from './ranked_schema_modal'; import {useUserPropertyFieldDelete} from './user_properties_delete_modal'; -import {isCreatePending} from './user_properties_utils'; +import {isCreatePending, isLinkedField} from './user_properties_utils'; import './user_properties_dot_menu.scss'; @@ -110,6 +111,15 @@ export const useAttributeLinkModal = (field: UserPropertyField, updateField: Pro const menuId = 'user-property-field_dotmenu'; +// Menu.Item renders a second label child as help text under the primary label — +// the same treatment the "Editable by users" item uses when it's locked. +const withLinkedHelp = (label: ReactElement, isLinked: boolean, help: ReactNode): ReactElement => (isLinked ? ( + <> + {label} + {help} + +) : label); + const DotMenu = ({ field, canCreate, @@ -138,6 +148,19 @@ const DotMenu = ({ const isSynced = Boolean(field.attrs.ldap || field.attrs.saml); + // Linked fields take their type and options from the template they link to. + // That rules out editing the ranking (options) and linking to AD/LDAP or SAML + // (both coerce the field to `text`) — the server rejects either change. The + // rest of this menu (visibility, editable-by-users, duplicate, delete) is + // unaffected. + const isLinked = isLinkedField(field); + const linkedHelp = ( + + ); + // Owner-managed fields (e.g. SCIM-provisioned) are read-only in this // screen: ownership and values are governed by the owning integration, so // they behave like synced fields for the "Editable by users" toggle and @@ -148,9 +171,16 @@ const DotMenu = ({ const handleDuplicate = () => { const name = `${slugifyForCEL(field.name)}_copy`; - const attrs = {...field.attrs}; - delete attrs.owners; - createField({...field, attrs, name}); + const duplicate = {...field, attrs: {...field.attrs}, name}; + + // A copy is a standalone field, not a second holder of the original's + // provenance: owners belong to the integration that assigned them, and a + // template link would make the copy inherit a type and option set the + // create request cannot carry anyway. + delete duplicate.attrs.owners; + delete duplicate.linked_field_id; + + createField(duplicate); }; const handleDelete = () => { @@ -235,14 +265,15 @@ const DotMenu = ({ {field.type === 'rank' && ( } - labels={( + labels={withLinkedHelp(( - )} + ), isLinked, linkedHelp)} /> )} } + disabled={isLinked} onClick={() => promptEditLdapLink()} - labels={field.attrs.ldap ? ( + labels={withLinkedHelp(field.attrs.ldap ? ( - )} + ), isLinked, linkedHelp)} />, } + disabled={isLinked} onClick={() => promptEditSamlLink()} - labels={field.attrs.saml ? ( + labels={withLinkedHelp(field.attrs.saml ? ( - )} + ), isLinked, linkedHelp)} />, ])} diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_rank_values.test.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_rank_values.test.tsx index c6e020fcafe..32e4084e3c7 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_rank_values.test.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_rank_values.test.tsx @@ -92,6 +92,19 @@ describe('UserPropertyRankValues', () => { ]); }); + it('locks the chips and hides the add input when the field is linked to a template', () => { + renderWithContext( + , + ); + + expect(screen.getByTestId('rank-chip-a')).toBeDisabled(); + expect(screen.queryByTestId('rank-chip-a-remove')).not.toBeInTheDocument(); + expect(screen.queryByTestId('user-property-rank-values__add-input')).not.toBeInTheDocument(); + }); + it('does not show the duplicate error while the label still matches the option\'s own name', async () => { renderWithContext( { const ascOptions = useMemo(() => sortOptionsByRankAsc(options), [options]); const sortedRanks = useMemo(() => ascOptions.map((option) => option.rank ?? 0), [ascOptions]); - const isDisabled = field.delete_at !== 0; + // Linked fields inherit their options from the template they link to; the + // server rejects an options change on them. + const isDisabled = field.delete_at !== 0 || isLinkedField(field); const addInputRef = useRef(null); diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_type_menu.test.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_type_menu.test.tsx index 6993039cc67..8b6154571ea 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_type_menu.test.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_type_menu.test.tsx @@ -85,6 +85,12 @@ describe('UserPropertyTypeMenu', () => { expect(menuButton).toBeDisabled(); }); + it('disables menu button when the field is linked to a template', () => { + renderComponent({...baseField, linked_field_id: 'template-field-id'}); + + expect(screen.getByTestId('fieldTypeSelectorMenuButton')).toBeDisabled(); + }); + it('changes field type when a new type is selected', async () => { renderComponent(); diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_type_menu.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_type_menu.tsx index 68a517b973a..c06d103d697 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_type_menu.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_type_menu.tsx @@ -17,6 +17,8 @@ import type {IDMappedObjects} from '@mattermost/types/utilities'; import useGetFeatureFlagValue from 'components/common/hooks/useGetFeatureFlagValue'; import * as Menu from 'components/menu'; +import {isLinkedField} from './user_properties_utils'; + import './user_properties_type_menu.scss'; interface Props { @@ -61,7 +63,10 @@ const SelectType = (props: Props) => { const CurrentTypeIcon = currentTypeDescriptor.icon; const isProtected = Boolean(props.field.attrs?.protected); - const isDisabled = props.field.delete_at !== 0 || isProtected; + + // Linked fields take their type from the template they link to; the server + // rejects a type change on them. + const isDisabled = props.field.delete_at !== 0 || isProtected || isLinkedField(props.field); return ( { expect(pendingNames).toEqual(['Text', 'Text_2']); }); }); + +describe('isLinkedField', () => { + it('is true only when the field links to a template field', () => { + expect(isLinkedField({})).toBe(false); + expect(isLinkedField({linked_field_id: ''})).toBe(false); + expect(isLinkedField({linked_field_id: 'template-field-id'})).toBe(true); + }); +}); + +describe('newPendingField', () => { + // newPendingField doesn't strip the link, so a pending field keeps whatever + // it was handed. That only reaches the collection, not the server: the + // commit path sends name/type/attrs, and every linked field is created + // directly via Client4.createPropertyField (classification_markings/utils). + // Dropping the link is specific to duplication and lives in the dot menu's + // handleDuplicate — see user_properties_dot_menu.test.tsx. + it('keeps an explicitly requested template link', () => { + const pending = newPendingField({ + name: 'clearance', + type: 'rank', + linked_field_id: 'template-field-id', + } as UserPropertyFieldPatch & Pick); + + expect(pending.linked_field_id).toBe('template-field-id'); + expect(isLinkedField(pending)).toBe(true); + }); +}); diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_utils.ts b/webapp/channels/src/components/admin_console/system_properties/user_properties_utils.ts index f3244941e07..cd924ef8072 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_utils.ts +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_utils.ts @@ -314,6 +314,15 @@ export const isDeletePending = ) => Boolean(field.linked_field_id); + export const newPendingId = () => `${PENDING}${generateId()}`; export const newPendingField = (patch: UserPropertyFieldPatch & Pick): UserPropertyField => { diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_values.test.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_values.test.tsx index 8656835ab10..709ddc159aa 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_values.test.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_values.test.tsx @@ -150,6 +150,13 @@ describe('UserPropertyValues', () => { expect(option.closest('div[aria-disabled]')).toBeInTheDocument(); }); + it('is disabled when the field is linked to a template', () => { + renderComponent({...baseField, linked_field_id: 'template-field-id'}); + + const option = screen.getByText('Option 1'); + expect(option.closest('div[aria-disabled]')).toBeInTheDocument(); + }); + it('shows LDAP sync information when field has LDAP attribute', () => { const ldapField = { ...baseField, diff --git a/webapp/channels/src/components/admin_console/system_properties/user_properties_values.tsx b/webapp/channels/src/components/admin_console/system_properties/user_properties_values.tsx index 0f4c12d74af..2c174b031c5 100644 --- a/webapp/channels/src/components/admin_console/system_properties/user_properties_values.tsx +++ b/webapp/channels/src/components/admin_console/system_properties/user_properties_values.tsx @@ -27,6 +27,7 @@ import {DangerText} from './controls'; import './user_properties_values.scss'; import {useAttributeLinkModal} from './user_properties_dot_menu'; import UserPropertyRankValues from './user_properties_rank_values'; +import {isLinkedField} from './user_properties_utils'; type Props = { field: UserPropertyField; @@ -136,20 +137,29 @@ const UserPropertyValues = ({ ); }); + // Editing an LDAP/SAML link rewrites the field as `text`, which the + // server refuses on a linked field (its type comes from the template). + // The dot menu already disables the same action when linked; render the + // chip as plain text here so this cell doesn't offer it either. + const editable = !isLinkedField(field); + const editProps = (onEdit: () => void) => (editable ? { + onClick: onEdit, + onKeyDown: (e: React.KeyboardEvent) => { + if (isKeyPressed(e, Constants.KeyCodes.ENTER) || isKeyPressed(e, Constants.KeyCodes.SPACE)) { + onEdit(); + } + }, + role: 'button', + tabIndex: 0, + } : {}); + const syncedProperties = [ field.attrs.ldap && ( promptEditLdapLink()} - onKeyDown={(e) => { - if (isKeyPressed(e, Constants.KeyCodes.ENTER) || isKeyPressed(e, Constants.KeyCodes.SPACE)) { - promptEditLdapLink(); - } - }} - role='button' - tabIndex={0} + {...editProps(promptEditLdapLink)} > promptEditSamlLink()} - onKeyDown={(e) => { - if (isKeyPressed(e, Constants.KeyCodes.ENTER) || isKeyPressed(e, Constants.KeyCodes.SPACE)) { - promptEditSamlLink(); - } - }} - role='button' - tabIndex={0} + {...editProps(promptEditSamlLink)} > Promise>; + getAccessControlFields: (after: string, limit: number, includeResourceFields?: boolean) => Promise>; getVisualAST: (expression: string) => Promise>; - searchUsers: (expression: string, term: string, after: string, limit: number) => Promise>; + searchUsers: (expression: string, term: string, after: string, limit: number, channelIdOverride?: string) => Promise>; getChannelPolicy: (channelId: string) => Promise>; saveChannelPolicy: (policy: AccessControlPolicy) => Promise>; deleteChannelPolicy: (policyId: string) => Promise; @@ -57,16 +57,19 @@ export const useChannelAccessControlActions = (channelId?: string, teamId?: stri const dispatch = useDispatch(); return useMemo(() => ({ - getAccessControlFields: (after: string, limit: number) => { - return dispatch(getAccessControlFields(after, limit, channelId, teamId)); + getAccessControlFields: (after: string, limit: number, includeResourceFields?: boolean) => { + return dispatch(getAccessControlFields(after, limit, channelId, teamId, includeResourceFields)); }, getVisualAST: (expression: string) => { return dispatch(getVisualAST(expression, channelId, teamId)); }, - searchUsers: (expression: string, term: string, after: string, limit: number) => { - return dispatch(searchUsersForExpression(expression, term, after, limit, channelId, teamId)); + searchUsers: (expression: string, term: string, after: string, limit: number, channelIdOverride?: string) => { + // A channel picked in the test modal (for a resource.attributes.* rule + // in an editor with no channel scope of its own) takes precedence over + // the hook's scoped channel. + return dispatch(searchUsersForExpression(expression, term, after, limit, channelIdOverride ?? channelId, teamId)); }, getChannelPolicy: (channelId: string) => { diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index 88865b5db24..0172769ff0c 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -311,6 +311,8 @@ "admin.access_control.policy.channels_affected": "Are you sure you want to save and apply the membership policy?", "admin.access_control.policy.edit_policy.access_rules.subtitle": "Select user attributes and values that qualifying users must have", "admin.access_control.policy.edit_policy.access_rules.title": "Attribute-based membership rules", + "admin.access_control.policy.edit_policy.channel_attribute_notice.text": "If an assigned channel is missing the referenced attribute, every member of that channel is removed.", + "admin.access_control.policy.edit_policy.channel_attribute_notice.title": "Channels without this attribute lose all members", "admin.access_control.policy.edit_policy.channel_selector.addChannels": "Add channels", "admin.access_control.policy.edit_policy.channel_selector.remove": "Remove", "admin.access_control.policy.edit_policy.channel_selector.subtitle": "Add channels that this membership policy will apply to.", @@ -467,6 +469,9 @@ "admin.access_control.table_editor.operator.version_less_than": "version is less than", "admin.access_control.table_editor.operator.younger_than": "younger than (days)", "admin.access_control.table_editor.remove_row": "Remove row", + "admin.access_control.table_editor.rhs.channel_attributes_section": "Channel attributes", + "admin.access_control.table_editor.rhs.channel_target_label": "Channel: {name}", + "admin.access_control.table_editor.rhs.values_section": "Values", "admin.access_control.table_editor.selector.custom_attributes": "Custom attributes", "admin.access_control.table_editor.selector.filter_attributes": "Search attributes...", "admin.access_control.table_editor.selector.filter_operators": "Search operators...", @@ -484,6 +489,11 @@ "admin.access_control.table_editor.values": "Values", "admin.access_control.table_editor.values.create_placeholder": "Type to create value", "admin.access_control.table_editor.values.select_values": "Select values...", + "admin.access_control.test.channel_picker.back": "Back to channel selection", + "admin.access_control.test.channel_picker.error": "Could not load channels. Check your connection and try again.", + "admin.access_control.test.channel_picker.no_results": "No channels found", + "admin.access_control.test.channel_picker.search": "Search channels", + "admin.access_control.test.channel_picker.title": "Select a channel to test against", "admin.access_control.testResults": "Access Rule Test Results", "admin.accesscontrol.enableAuditLogging.desc": "When enabled, attribute-based access control policy decisions are written to the server audit log. Requires server audit logging to be active.", "admin.accesscontrol.enableAuditLogging.disabled": "When enabled, attribute-based access control policy decisions are written to the server audit log. This setting requires attribute-based access control to be enabled and server audit logging to be active (enable file audit logging or configure an advanced audit logging target).", @@ -828,6 +838,9 @@ "admin.classification_markings.enable.false": "False", "admin.classification_markings.enable.title": "Enable classification markings", "admin.classification_markings.enable.true": "True", + "admin.classification_markings.enforcement.clearance.checkbox": "Enable clearance attribute", + "admin.classification_markings.enforcement.clearance.help": "Creates a ranked \"Clearance\" user attribute linked to these classification levels. Channel membership can then be managed with a corresponding membership policy.", + "admin.classification_markings.enforcement.clearance.title": "Clearance attribute", "admin.classification_markings.error.delete_has_dependents": "Cannot disable classification markings while channel classifications exist. Remove all channel classification markings first.", "admin.classification_markings.error.duplicate_name": "Classification level names must be unique. Duplicate: {name}", "admin.classification_markings.error.empty_name": "All classification levels must have a name.", @@ -3588,6 +3601,7 @@ "admin.system_properties.user_properties.dotmenu.editable_by_users.owner_managed_synced_help": "Managed by an integration and synced via AD/LDAP or SAML", "admin.system_properties.user_properties.dotmenu.editable_by_users.synced_help": "Synced attributes are managed by AD/LDAP or SAML", "admin.system_properties.user_properties.dotmenu.label": "Select an action", + "admin.system_properties.user_properties.dotmenu.linked.help": "Managed by a linked attribute template", "admin.system_properties.user_properties.dotmenu.saml.edit_link.label": "Edit SAML link", "admin.system_properties.user_properties.dotmenu.saml.link_property.label": "Link attribute to SAML", "admin.system_properties.user_properties.dotmenu.saml.modal.helpText": "The attribute in the SAML server used to sync as a custom attribute in user's profile in Mattermost.", diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/access_control.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/access_control.ts index e7dbec39170..96a85b80b92 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/access_control.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/access_control.ts @@ -175,9 +175,9 @@ export function getTeamAccessControlPolicy(teamId: string) { }); } -export function getAccessControlFields(after: string, limit: number, channelId?: string, teamId?: string) { +export function getAccessControlFields(after: string, limit: number, channelId?: string, teamId?: string, includeResourceFields?: boolean) { return bindClientFunc({ - clientFunc: () => Client4.getAccessControlFields(after, limit, channelId, teamId), + clientFunc: () => Client4.getAccessControlFields(after, limit, channelId, teamId, includeResourceFields), params: [], }); } diff --git a/webapp/platform/client/src/client4.test.ts b/webapp/platform/client/src/client4.test.ts index 6c2cd56e538..04f4b3d8308 100644 --- a/webapp/platform/client/src/client4.test.ts +++ b/webapp/platform/client/src/client4.test.ts @@ -111,6 +111,36 @@ describe('Client4', () => { }); }); + describe('access control field autocomplete', () => { + let client: Client4; + + beforeEach(() => { + client = new Client4(); + client.setUrl('http://mattermost.example.com'); + }); + + test('getAccessControlFields sends include_resource_fields when requested', async () => { + const fields = [{id: 'f1', name: 'classification'}]; + nock(client.getBaseRoute()). + get('/access_control_policies/cel/autocomplete/fields'). + query({after: '', limit: '100', include_resource_fields: 'true'}). + reply(200, fields); + + const result = await client.getAccessControlFields('', 100, undefined, undefined, true); + expect(result).toEqual(fields); + }); + + test('getAccessControlFields omits include_resource_fields by default', async () => { + nock(client.getBaseRoute()). + get('/access_control_policies/cel/autocomplete/fields'). + query((q) => q.include_resource_fields === undefined && q.after === '' && q.limit === '100'). + reply(200, []); + + const result = await client.getAccessControlFields('', 100); + expect(result).toEqual([]); + }); + }); + describe('content flagging routes', () => { let client: Client4; diff --git a/webapp/platform/client/src/client4.ts b/webapp/platform/client/src/client4.ts index b2245ebfc67..29b1ed19ba7 100644 --- a/webapp/platform/client/src/client4.ts +++ b/webapp/platform/client/src/client4.ts @@ -5208,7 +5208,7 @@ export default class Client4 { return this.createJob(job); }; - getAccessControlFields = (after: string, limit: number, channelId?: string, teamId?: string) => { + getAccessControlFields = (after: string, limit: number, channelId?: string, teamId?: string, includeResourceFields?: boolean) => { const params = new URLSearchParams({after, limit: limit.toString()}); if (channelId) { params.append('channelId', channelId); @@ -5217,6 +5217,12 @@ export default class Client4 { params.append('team_id', teamId); } + // Parent policies reference resource.attributes.* (channel-object-type + // fields) without a single channel to scope by; ask for them explicitly. + if (includeResourceFields) { + params.append('include_resource_fields', 'true'); + } + return this.doFetch( `${this.getBaseRoute()}/access_control_policies/cel/autocomplete/fields?${params.toString()}`, {method: 'get'}, diff --git a/webapp/platform/types/src/properties_user.ts b/webapp/platform/types/src/properties_user.ts index f888144fe2b..8834efe2147 100644 --- a/webapp/platform/types/src/properties_user.ts +++ b/webapp/platform/types/src/properties_user.ts @@ -27,6 +27,11 @@ export const SESSION_ATTRIBUTES_OBJECT_TYPE = 'session'; // object type; session attributes are the exception (`session`). export const USER_OBJECT_TYPE = 'user'; +// Channel-targeted attributes. On the ABAC autocomplete these are the fields a +// policy references as `resource.attributes.*` — the accessed channel's values, +// as opposed to the requesting user's (`USER_OBJECT_TYPE`). +export const CHANNEL_ATTRIBUTES_OBJECT_TYPE = 'channel'; + /** * Session attributes are the only property fields targeting the `session` * object type, so identity is keyed off `object_type` rather than the group