From 4da11e81afed0cb3919482c71f5dc3bfea50fb0f Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Thu, 30 Apr 2026 00:56:32 +0200 Subject: [PATCH] [MM-68497] Enables membership policies on public channels with advisory semantics (#36275) --- api/v4/source/channels.yaml | 47 ++ api/v4/source/users.yaml | 19 + .../abac_public_channels.spec.ts | 241 ++++++++ .../channel_admin_self_inclusion.spec.ts | 8 +- .../channel_settings_access_control.spec.ts | 67 +- .../channels/channel_settings/helpers.ts | 296 +++++++++ .../channels/team_settings/helpers.ts | 65 ++ .../abac/policies/channel_integration.spec.ts | 4 +- server/channels/api4/access_control.go | 1 - server/channels/api4/access_control_test.go | 157 ++++- server/channels/api4/channel.go | 26 + server/channels/api4/channel_test.go | 121 ++++ server/channels/api4/user.go | 29 +- server/channels/api4/user_test.go | 120 ++++ server/channels/app/access_control.go | 38 +- server/channels/app/access_control_test.go | 578 ++++++++++++++++-- server/channels/app/channel.go | 197 +++++- server/channels/app/team_access_control.go | 135 +++- .../channels/app/team_access_control_test.go | 21 +- server/i18n/en.json | 16 +- .../__snapshots__/policies.test.tsx.snap | 12 +- .../jobs/access_control_sync_job_table.tsx | 35 +- .../confirmation/confirmation_modal.tsx | 52 +- .../modals/job_details/job_details_modal.scss | 18 + .../policy_selection_modal.tsx | 4 +- .../admin_console/access_control/policies.tsx | 4 +- .../policy_details.test.tsx.snap | 20 +- .../policy_details/policy_details.tsx | 79 ++- .../channel_details.test.tsx.snap | 20 +- .../details/channel_access_control_policy.tsx | 16 +- .../channel/details/channel_modes.tsx | 45 +- .../browse_channels/browse_channels.test.tsx | 64 ++ .../browse_channels/browse_channels.tsx | 99 ++- .../src/components/browse_channels/index.ts | 5 +- .../channel_invite_modal.scss | 4 + .../channel_invite_modal.test.tsx | 225 +++++-- .../channel_invite_modal.tsx | 296 +++++++-- .../channel_members_rhs.test.tsx | 27 +- .../channel_members_rhs.tsx | 5 +- .../channel_selector_modal.tsx | 17 +- ...el_settings_access_rules_tab.test.tsx.snap | 2 +- ...channel_settings_access_rules_tab.test.tsx | 72 ++- .../channel_settings_access_rules_tab.tsx | 75 ++- .../channel_settings_info_tab.test.tsx | 27 + .../channel_settings_info_tab.tsx | 19 +- .../channel_settings_modal.test.tsx | 67 +- .../channel_settings_modal.tsx | 13 +- .../components/searchable_channel_list.tsx | 36 ++ .../system_policy_indicator.test.tsx | 22 +- .../system_policy_indicator.tsx | 116 +++- .../team_policy_confirmation_modal.tsx | 56 +- .../team_policy_editor.test.tsx | 2 +- .../team_policy_editor.tsx | 86 ++- .../team_settings_modal.tsx | 2 +- webapp/channels/src/i18n/en.json | 86 ++- .../mattermost-redux/src/actions/channels.ts | 20 + webapp/platform/client/src/client4.ts | 31 + 57 files changed, 3500 insertions(+), 465 deletions(-) create mode 100644 e2e-tests/playwright/specs/functional/channels/channel_settings/abac_public_channels.spec.ts rename e2e-tests/playwright/specs/functional/channels/{channel_settings_access_rules => channel_settings}/channel_admin_self_inclusion.spec.ts (97%) create mode 100644 e2e-tests/playwright/specs/functional/channels/channel_settings/helpers.ts diff --git a/api/v4/source/channels.yaml b/api/v4/source/channels.yaml index 140e318b40a..01bd6e5fd9f 100644 --- a/api/v4/source/channels.yaml +++ b/api/v4/source/channels.yaml @@ -995,6 +995,53 @@ $ref: "#/components/responses/Forbidden" "404": $ref: "#/components/responses/NotFound" + "/api/v4/teams/{team_id}/channels/recommended": + get: + tags: + - channels + summary: Get recommended public channels for the current user + description: | + Return the public channels on a team that have a membership policy + assigned, where the requesting user's attributes match to the policy. + + Membership policies on public channels are advisory: anyone can still join + these channels. This endpoint surfaces them as "Recommended channels" + for the requester. + + Returns an empty list if the Enterprise Advanced license is not + active, if `AccessControlSettings.EnableAttributeBasedAccessControl` + is `false`, or if the user's attributes do not match any active + public-channel policy in the team. + + __Minimum server version__: 11.8 + + ##### Permissions + Must be authenticated and have `list_team_channels` on the team. + operationId: GetRecommendedChannelsForTeam + parameters: + - name: team_id + in: path + description: Team GUID + required: true + schema: + type: string + responses: + "200": + description: Recommended channels retrieval successful + content: + application/json: + schema: + type: array + items: + $ref: "#/components/schemas/Channel" + "400": + $ref: "#/components/responses/BadRequest" + "401": + $ref: "#/components/responses/Unauthorized" + "403": + $ref: "#/components/responses/Forbidden" + "404": + $ref: "#/components/responses/NotFound" "/api/v4/teams/{team_id}/channels/deleted": get: tags: diff --git a/api/v4/source/users.yaml b/api/v4/source/users.yaml index c09f6dac650..d63c934ed42 100644 --- a/api/v4/source/users.yaml +++ b/api/v4/source/users.yaml @@ -437,6 +437,25 @@ group constrains. schema: type: boolean + - name: abac_match_only + in: query + description: > + When used with `not_in_channel`, restricts the result to users whose + attributes satisfy the channel's Attribute-Based Access Control + (ABAC) membership policy. + + + On private channels with an ABAC policy this filter is always + applied regardless of this parameter (hard gate). On public channels + with an advisory ABAC policy the full not_in_channel candidate list + is returned by default; set this to `true` to fetch only the + matching subset of candidates (for example to annotate recommended + members in the invite UI). + + + __Minimum server version__: 11.8 + schema: + type: boolean - name: without_team in: query description: Whether or not to list users that are not on any team. This option diff --git a/e2e-tests/playwright/specs/functional/channels/channel_settings/abac_public_channels.spec.ts b/e2e-tests/playwright/specs/functional/channels/channel_settings/abac_public_channels.spec.ts new file mode 100644 index 00000000000..a02fa944688 --- /dev/null +++ b/e2e-tests/playwright/specs/functional/channels/channel_settings/abac_public_channels.spec.ts @@ -0,0 +1,241 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +/** + * @objective E2E coverage for public-channel ABAC behavior: + * - Default channels are not eligible for ABAC policies (channel-settings + team-settings paths) + * - A policy on a public channel surfaces matching users a "Recommended" channel entry + * + * @reference Public-channel ABAC feature + */ + +import {ChannelsPage, expect, getAdminClient, test} from '@mattermost/playwright-lib'; + +import {enableABACConfig, enableAPIUserDeletion} from '../team_settings/helpers'; + +import { + CleanupLedger, + createPolicyAssignedToChannels, + createTrackedAttribute, + createTrackedPublicChannel, + createTrackedTeamMember, + runAccessControlSyncJob, + setChannelPolicyActive, + waitForJobCompletion, +} from './helpers'; + +test.describe('ABAC - Public channels', () => { + let ledger: CleanupLedger; + + // Enable the permanent-delete API once for the whole describe so the + // CleanupLedger's permanentDeleteUser cleanup tasks succeed instead of + // spamming "Permanent user deletion feature is not enabled" on teardown. + // `pw` is test-scoped, so we use the worker-scoped getAdminClient here. + test.beforeAll(async () => { + const {adminClient} = await getAdminClient(); + if (!adminClient) { + return; + } + await enableAPIUserDeletion(adminClient); + }); + + test.beforeEach(() => { + ledger = new CleanupLedger(); + }); + + test.afterEach(async () => { + await ledger.drain(); + }); + + test('Default channel hides Membership Policy tab in Channel Settings', async ({pw}) => { + await pw.skipIfNoLicense(); + const {adminUser, adminClient, team} = await pw.initSetup(); + await enableABACConfig(adminClient); + + // # Open Channel Settings on the default (town-square) channel + const {page} = await pw.testBrowser.login(adminUser); + const channelsPage = new ChannelsPage(page); + await channelsPage.goto(team.name, 'town-square'); + await channelsPage.toBeVisible(); + + const channelSettings = await channelsPage.openChannelSettings(); + + // * Membership Policy tab is NOT rendered for the default channel — + // ValidateChannelEligibilityForAccessControl rejects default channels + // server-side, so the tab would only let a user assemble unsaveable rules. + await expect(channelSettings.container.getByTestId('access_rules-tab-button')).not.toBeVisible(); + + await channelSettings.close(); + }); + + test('Default channel is excluded from Team Settings policy editor channel selector', async ({pw}) => { + await pw.skipIfNoLicense(); + const {adminUser, adminClient, team} = await pw.initSetup(); + await enableABACConfig(adminClient); + + // # Navigate to Team Settings → Membership Policies → New policy editor + const {page} = await pw.testBrowser.login(adminUser); + const channelsPage = new ChannelsPage(page); + await channelsPage.goto(team.name); + await channelsPage.toBeVisible(); + + const teamSettings = await channelsPage.openTeamSettings(); + await teamSettings.openAccessPoliciesTab(); + await teamSettings.container.getByRole('button', {name: 'Add policy'}).click(); + + // # Open the channel selector modal + await teamSettings.container.getByRole('button', {name: /Add channels/i}).click(); + const channelSelector = page.locator('.channel-selector-modal'); + await channelSelector.waitFor(); + + // * Both default channels (town-square, off-topic) are absent from the list. + // The component contract is `excludeDefaultChannels={true}` — assert the + // canonical channel rows by their stable test ids rather than display text. + await expect(channelSelector.locator('[data-testid="ChannelRow-town-square"]')).toHaveCount(0); + await expect(channelSelector.locator('[data-testid="ChannelRow-off-topic"]')).toHaveCount(0); + + await teamSettings.close(); + }); + + test('Public channel with matching policy surfaces as Recommended for matching user (auto-add off)', async ({ + pw, + }) => { + await pw.skipIfNoLicense(); + const {adminClient, team} = await pw.initSetup(); + await enableABACConfig(adminClient); + + // # Per-test custom profile attribute. We don't reuse a shared "Department" + // field — the server caps CPA fields at 20, and accumulating shared + // state across tests has historically saturated that limit and broken + // subsequent runs. This field gets cleaned up via the ledger. + const attribute = await createTrackedAttribute(adminClient, 'PubAbacDept', ledger); + + // # Public channel + parent policy with rule " == 'engineering'", + // policy assigned to that channel. Auto-add stays OFF (server default + // on a freshly-created child policy). + const publicChannel = await createTrackedPublicChannel(adminClient, team.id, ledger); + // CEL references the attribute by its field name (lowercased to keep + // the identifier characters CEL-safe — uppercase chars in identifiers + // are technically valid but the table-editor parser used in the webapp + // round-trips through lowercase, so this matches reality). + const expression = `user.attributes.${attribute.name} == 'engineering'`; + await createPolicyAssignedToChannels( + adminClient, + `Public Recommend ${Date.now()}`, + expression, + [publicChannel.id], + ledger, + ); + + // # Matching user — value matches the rule, member of the team but NOT + // of the channel. Auto-add is off, so they must NOT be auto-joined; + // the channel should instead appear in their Recommended list. + const matchingUser = await createTrackedTeamMember( + adminClient, + team.id, + {fieldId: attribute.id, value: 'engineering'}, + ledger, + ); + + // * Auto-add OFF contract: the matching user must NOT have been silently + // joined to the channel. Verified server-side before any UI flow so a + // regression that auto-adds them would fail here, regardless of UI + // behavior. The Browse-Channels assertion below proves the channel + // *appears as a recommendation* — but only this membership check + // distinguishes "recommended (advisory)" from "auto-added". + const preMembers = await adminClient.getChannelMembers(publicChannel.id); + expect( + preMembers.map((m: any) => m.user_id), + 'matching user must not be auto-joined when auto-add is OFF (advisory recommendation only)', + ).not.toContain(matchingUser.id); + + const {page} = await pw.testBrowser.login(matchingUser); + const channelsPage = new ChannelsPage(page); + await channelsPage.goto(team.name); + await channelsPage.toBeVisible(); + + // # Open Browse Channels and switch the type filter to "Recommended channels" + const browse = await channelsPage.openBrowseChannelsModal(); + await browse.toBeDoneLoading(); + + // The recommended menu item is keyed by `id="channelsMoreDropdownRecommended"`, + // gated by the `showRecommendedFilter` prop which is set whenever ABAC is + // enabled in client config. Using the stable id avoids brittleness from + // the dropdown's localized label. + await browse.container.locator('#menuWrapper').click(); + await page.locator('#channelsMoreDropdownRecommended').click(); + + // * The public channel appears in the Recommended results. The row id + // is generated by the SearchableChannelList component as + // `ChannelRow-${channel.name}` — assert against that, not the display + // text, which can be locale-dependent. + const channelRow = browse.container.locator(`[data-testid="ChannelRow-${publicChannel.name}"]`); + await expect(channelRow).toBeVisible({timeout: 15000}); + }); + + test('Public channel with auto-add ON adds matching users; non-matching members are NOT removed', async ({pw}) => { + // The sync job is queued and processed asynchronously — give the test + // enough headroom for the queue + evaluation + member writes. Empirically + // it finishes in <10s on a quiet dev server, but CI workers are slower. + test.setTimeout(120_000); + + await pw.skipIfNoLicense(); + const {adminClient, team} = await pw.initSetup(); + await enableABACConfig(adminClient); + + const attribute = await createTrackedAttribute(adminClient, 'PubAbacAuto', ledger); + const publicChannel = await createTrackedPublicChannel(adminClient, team.id, ledger); + const expression = `user.attributes.${attribute.name} == 'engineering'`; + await createPolicyAssignedToChannels( + adminClient, + `Public Auto-Add ${Date.now()}`, + expression, + [publicChannel.id], + ledger, + ); + + // # Auto-add ON: flip the channel-scope (child) policy's Active flag. + // The parent default is inactive, and children inherit Active=false at + // assign time, so we activate the child directly here. The sync job + // only auto-adds members for Active policies. + await setChannelPolicyActive(adminClient, publicChannel.id, true); + + // # Two users: + // - matching: Department=engineering, in team but NOT in channel. + // Sync should auto-add them. + // - nonMatching: Department=sales, already a member of the channel. + // For PUBLIC channels, ABAC is advisory: sync must NOT + // remove them. (For private channels, sync would.) + const matching = await createTrackedTeamMember( + adminClient, + team.id, + {fieldId: attribute.id, value: 'engineering'}, + ledger, + ); + const nonMatching = await createTrackedTeamMember( + adminClient, + team.id, + {fieldId: attribute.id, value: 'sales'}, + ledger, + ); + await adminClient.addToChannel(nonMatching.id, publicChannel.id); + + // # Trigger sync against this channel-scope policy and wait for it to + // reach a terminal state. We pass `publicChannel.id` (the child + // policy ID) rather than the parent ID so the sync stays scoped to + // this test and ignores any other policies on the dev server. + const job = await runAccessControlSyncJob(adminClient, publicChannel.id); + const finished = await waitForJobCompletion(adminClient, job.id); + expect(finished.status, `sync job did not succeed: ${JSON.stringify(finished)}`).toBe('success'); + + // * Membership state via API — no UI flake. + const members = await adminClient.getChannelMembers(publicChannel.id); + const memberIds = members.map((m: any) => m.user_id); + + expect(memberIds, 'matching user must be auto-added by sync').toContain(matching.id); + expect( + memberIds, + 'non-matching member must NOT be removed (public channels are advisory under ABAC)', + ).toContain(nonMatching.id); + }); +}); diff --git a/e2e-tests/playwright/specs/functional/channels/channel_settings_access_rules/channel_admin_self_inclusion.spec.ts b/e2e-tests/playwright/specs/functional/channels/channel_settings/channel_admin_self_inclusion.spec.ts similarity index 97% rename from e2e-tests/playwright/specs/functional/channels/channel_settings_access_rules/channel_admin_self_inclusion.spec.ts rename to e2e-tests/playwright/specs/functional/channels/channel_settings/channel_admin_self_inclusion.spec.ts index d9dbcdf6546..aca8fba44ba 100644 --- a/e2e-tests/playwright/specs/functional/channels/channel_settings_access_rules/channel_admin_self_inclusion.spec.ts +++ b/e2e-tests/playwright/specs/functional/channels/channel_settings/channel_admin_self_inclusion.spec.ts @@ -132,7 +132,7 @@ async function createChannelAccessRule( async function openAccessControlSettings(channelsPage: ChannelsPage) { const channelSettings = await channelsPage.openChannelSettings(); - const accessControlTab = channelSettings.container.getByRole('tab', {name: /Access Control/i}); + const accessControlTab = channelSettings.container.getByRole('tab', {name: /Membership Policy/i}); await expect(accessControlTab).toBeVisible({timeout: 10000}); await accessControlTab.click(); @@ -155,7 +155,7 @@ async function testAccessRuleAndVerifyUser(page: Page, username: string) { await expect(modal.getByText(`@${username}`)).toBeVisible({timeout: 10000}); } -test.describe('Channel Settings → Access Control', () => { +test.describe('Channel Settings → Membership Policy', () => { test('MM-68538 channel admin can test access rule for multiselect "has any of" with multiple values', async ({ pw, }) => { @@ -163,7 +163,7 @@ test.describe('Channel Settings → Access Control', () => { const {adminClient, team} = await pw.initSetup(); - // Enable ABAC + user-managed attributes so the Access Control tab and + // Enable ABAC + user-managed attributes so the Membership Policy tab and // the test access rule flow are available. await adminClient.patchConfig({ AccessControlSettings: { @@ -201,7 +201,7 @@ test.describe('Channel Settings → Access Control', () => { await createChannelAccessRule(adminClient, channel, aliceExcludingExpression); - // Alice can open Channel Settings → Access Control as channel admin + // Alice can open Channel Settings → Membership Policy as channel admin // and test the existing rule through the same UI users exercise. const {page} = await pw.testBrowser.login(alice); const channelsPage = new ChannelsPage(page); diff --git a/e2e-tests/playwright/specs/functional/channels/channel_settings/channel_settings_access_control.spec.ts b/e2e-tests/playwright/specs/functional/channels/channel_settings/channel_settings_access_control.spec.ts index 0bb8d558455..64291593fe3 100644 --- a/e2e-tests/playwright/specs/functional/channels/channel_settings/channel_settings_access_control.spec.ts +++ b/e2e-tests/playwright/specs/functional/channels/channel_settings/channel_settings_access_control.spec.ts @@ -2,8 +2,8 @@ // See LICENSE.txt for license information. /** - * @objective E2E tests for the Access Control tab in Channel Settings Modal - * @reference MM-67326 + * @objective E2E tests for the Membership Policy tab (access_rules) in Channel Settings Modal + * @reference MM-67326 — public and private channels can carry ABAC membership policies */ import {ChannelsPage, expect, test} from '@mattermost/playwright-lib'; @@ -19,8 +19,14 @@ import { setUserAttribute, addAttributeRule, createTeamAdmin, + waitForAttributeViewToInclude, } from '../team_settings/helpers'; +/** Unique CPA value so only users this test sets match the rule (avoids clashing with leftover Engineering users on the server). */ +function uniqueDepartmentValue(testId: string): string { + return `E2E-${testId}-${Date.now()}-${Math.random().toString(36).slice(2, 9)}`; +} + test.describe('Channel Settings Modal - Access Control Tab', () => { test('MM-67326_c1 Access Control tab visible for admin on private channel with ABAC enabled', async ({pw}) => { await pw.skipIfNoLicense(); @@ -62,7 +68,7 @@ test.describe('Channel Settings Modal - Access Control Tab', () => { await channelSettings.close(); }); - test('MM-67326_c3 Access Control tab hidden for public channel', async ({pw}) => { + test('MM-67326_c3 Membership Policy tab visible for admin on public channel with ABAC enabled', async ({pw}) => { await pw.skipIfNoLicense(); const {adminUser, adminClient, team} = await pw.initSetup(); await enableABACConfig(adminClient); @@ -76,8 +82,8 @@ test.describe('Channel Settings Modal - Access Control Tab', () => { const channelSettings = await channelsPage.openChannelSettings(); - // * Access Control tab is NOT visible for public channel - await expect(channelSettings.container.getByTestId('access_rules-tab-button')).not.toBeVisible(); + // * Membership Policy tab is visible on public channels when ABAC is enabled (not group-constrained / not default) + await expect(channelSettings.container.getByTestId('access_rules-tab-button')).toBeVisible(); await channelSettings.close(); }); @@ -239,15 +245,28 @@ test.describe('Channel Settings Modal - Access Control Tab', () => { await enableABACConfig(adminClient); await ensureDepartmentAttribute(adminClient); + const departmentValue = uniqueDepartmentValue('c9'); + // # Admin's Department satisfies the rule (self-exclusion check passes) - await setUserAttribute(adminClient, adminUser.id, 'Department', 'Engineering'); + await setUserAttribute(adminClient, adminUser.id, 'Department', departmentValue); // # Private channel — admin is the creator and only member const channel = await createPrivateChannel(adminClient, team.id); - // # Target user: in the team, Department=Engineering, NOT yet in the channel + // # Target user: in the team, same Department, NOT yet in the channel const targetUser = await createTeamAdmin(adminClient, team.id); - await setUserAttribute(adminClient, targetUser.id, 'Department', 'Engineering'); + await setUserAttribute(adminClient, targetUser.id, 'Department', departmentValue); + + // Save will run validateExpressionAgainstRequester and calculateMembershipChanges, + // both of which query the Postgres materialized AttributeView. The enterprise + // access-control service gates view refreshes to once per ~30s, so the brand-new + // unique CPA value above is not yet visible to CEL queries. Without this wait, + // Save hits the self-exclusion modal (admin appears unmatched against the rule + // they just satisfied via the API) and the confirmation modal never opens. + await waitForAttributeViewToInclude(adminClient, `user.attributes.Department == "${departmentValue}"`, [ + adminUser.id, + targetUser.id, + ]); const {page} = await pw.testBrowser.login(adminUser); const channelsPage = new ChannelsPage(page); @@ -262,8 +281,11 @@ test.describe('Channel Settings Modal - Access Control Tab', () => { const tab = channelSettings.container.locator('.ChannelSettingsModal__accessRulesTab'); await expect(tab).toBeVisible({timeout: 10000}); - // # Add attribute rule: Department == Engineering - await addAttributeRule(tab, page, 'Engineering'); + // # Add attribute rule (unique value → preview lists only targetUser to add) + await addAttributeRule(tab, page, departmentValue); + + // * Unsaved changes must be committed before save; otherwise handleSave can skip the confirmation path + await expect(tab.locator('[data-testid="SaveChangesPanel__save-btn"]')).toBeVisible({timeout: 15000}); // # Enable auto-add members const autoAddCheckbox = tab.locator('#autoSyncMembersCheckbox'); @@ -449,15 +471,24 @@ test.describe('Channel Settings Modal - Access Control Tab', () => { await enableABACConfig(adminClient); await ensureDepartmentAttribute(adminClient); + const departmentValue = uniqueDepartmentValue('c13'); + // # Admin satisfies the rule - await setUserAttribute(adminClient, adminUser.id, 'Department', 'Engineering'); + await setUserAttribute(adminClient, adminUser.id, 'Department', departmentValue); // # Private channel — admin is the only member const channel = await createPrivateChannel(adminClient, team.id); - // # Target user: in the team, Department=Engineering, NOT yet in the channel + // # Target user: in the team, same Department, NOT yet in the channel const memberToAdd = await createTeamAdmin(adminClient, team.id); - await setUserAttribute(adminClient, memberToAdd.id, 'Department', 'Engineering'); + await setUserAttribute(adminClient, memberToAdd.id, 'Department', departmentValue); + + // See c9: wait for the materialized AttributeView to surface admin and + // memberToAdd as matching the freshly-written CPA value before clicking Save. + await waitForAttributeViewToInclude(adminClient, `user.attributes.Department == "${departmentValue}"`, [ + adminUser.id, + memberToAdd.id, + ]); const {page} = await pw.testBrowser.login(adminUser); const channelsPage = new ChannelsPage(page); @@ -472,8 +503,9 @@ test.describe('Channel Settings Modal - Access Control Tab', () => { const tab = channelSettings.container.locator('.ChannelSettingsModal__accessRulesTab'); await expect(tab).toBeVisible({timeout: 10000}); - // # Add rule: Department == Engineering - await addAttributeRule(tab, page, 'Engineering'); + // # Add rule (unique value → only memberToAdd appears in "to add" with this server data) + await addAttributeRule(tab, page, departmentValue); + await expect(tab.locator('[data-testid="SaveChangesPanel__save-btn"]')).toBeVisible({timeout: 15000}); // # Enable auto-add so memberToAdd appears in the "to add" list const autoAddCheckbox = tab.locator('#autoSyncMembersCheckbox'); @@ -493,9 +525,6 @@ test.describe('Channel Settings Modal - Access Control Tab', () => { await expect(confirmModal).toContainText('remove 0 current channel members'); // # Click "View users" to open the detailed user list - // Note: the "add N users" count in the summary is not asserted here because leftover - // users from previous test runs may also have Department=Engineering, making the - // count non-deterministic. We verify the specific user in the Allowed tab instead. await confirmModal.getByRole('button', {name: 'View users'}).click(); // * Allowed tab is visible with at least one user (memberToAdd) @@ -503,7 +532,7 @@ test.describe('Channel Settings Modal - Access Control Tab', () => { timeout: 5000, }); - // # The Allowed tab is active by default — verify memberToAdd's username is shown + // # The Allowed tab is active by default — verify memberToAdd's username is shown (unique Dept avoids other matches) await expect(confirmModal).toContainText(memberToAdd.username, {timeout: 5000}); // # Cancel — don't actually apply diff --git a/e2e-tests/playwright/specs/functional/channels/channel_settings/helpers.ts b/e2e-tests/playwright/specs/functional/channels/channel_settings/helpers.ts new file mode 100644 index 00000000000..6e40121e6e8 --- /dev/null +++ b/e2e-tests/playwright/specs/functional/channels/channel_settings/helpers.ts @@ -0,0 +1,296 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +/** + * Lean helpers for the public-channel ABAC E2E specs. + * + * Design rules: + * - Reuse the existing primitives in `../team_settings/helpers.ts` for + * channel/policy/user creation and configuration. This file only adds + * thin wrappers that register cleanup as resources are created. + * - All test-created resources are tracked in a `CleanupLedger`. The spec's + * `test.afterEach` drains the ledger so resources are removed whether the + * test passed, failed, or threw mid-flight. + * - Cleanup is best-effort: individual failures are swallowed so one stale + * resource cannot block deletion of the others or mask the test result. + * - Custom profile attributes are scoped per test (unique names + cleanup). + * Tests must not lean on a shared `Department` field — the server caps CPA + * fields at 20, and accumulating shared state has historically saturated + * that limit and broken every subsequent run. + */ + +import type {Client4} from '@mattermost/client'; +import type {UserProfile} from '@mattermost/types/users'; + +import {newTestPassword, getRandomId} from '@mattermost/playwright-lib'; + +import {assignChannelsToPolicy, deletePolicy, unassignChannelsFromPolicy} from '../team_settings/helpers'; + +export type CleanupTask = () => Promise; + +export class CleanupLedger { + private tasks: CleanupTask[] = []; + + add(task: CleanupTask): void { + // LIFO: latest registrations run first so deletions respect dependency + // order (e.g. unassign channels from a policy before deleting the policy). + this.tasks.unshift(task); + } + + async drain(): Promise { + const tasks = this.tasks; + this.tasks = []; + for (let i = 0; i < tasks.length; i++) { + try { + await tasks[i](); + } catch (e: unknown) { + const message = e instanceof Error ? e.message : String(e); + const stack = e instanceof Error ? e.stack : undefined; + // eslint-disable-next-line no-console + console.error(`CleanupLedger.drain: cleanup task ${i} failed`, message, stack ?? ''); + // Swallow — cleanup is best-effort and must not mask the test + // result or stop subsequent cleanups from running. + } + } + } +} + +/** + * Issue a server request and surface the response body on non-2xx so the test + * report shows *what* the server rejected, not just "failed". The shipped + * helpers in `team_settings/helpers.ts` swallow these — useful in steady + * state, brutal when something legitimately broke (e.g. CPA field-limit + * exhaustion silently masquerading as a "field not found" error). + */ +async function doFetchOrThrow(client: Client4, url: string, init: RequestInit & {body?: string}): Promise { + const response = await fetch(url, { + ...init, + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${client.getToken()}`, + ...(init.headers || {}), + }, + }); + if (!response.ok) { + const text = await response.text().catch(() => ''); + throw new Error(`${init.method} ${url} -> ${response.status}: ${text}`); + } + if (response.status === 204) { + return null; + } + return response.json(); +} + +/** + * Permanently delete a user via the admin API. + * Permanent deletion requires `ServiceSettings.EnableAPIUserDeletion` to be on + * server-side; on test deployments where it is off, the call returns 4xx and + * the cleanup ledger silently swallows it. + */ +export async function permanentDeleteUser(client: Client4, userId: string): Promise { + await (client as any).doFetch(`${client.getBaseRoute()}/users/${userId}?permanent=true`, { + method: 'DELETE', + }); +} + +/** + * Create a per-test custom profile attribute (text type) with a unique name and + * register cleanup. Returns the field including its server-assigned `id`, + * which callers can plug straight into a CEL expression via + * `user.attributes.id_`. + * + * Avoids the shared `Department` slot used by other ABAC tests: those compete + * for one of 20 server-cap'd CPA slots and any leak (a failed test, an + * abandoned run) cascades into "field not found" or "limit reached" errors + * across the whole suite. + */ +export async function createTrackedAttribute( + client: Client4, + baseName: string, + ledger: CleanupLedger, +): Promise<{id: string; name: string}> { + const name = `${baseName}_${getRandomId()}`; + const field = await doFetchOrThrow(client, `${client.getBaseRoute()}/custom_profile_attributes/fields`, { + method: 'POST', + body: JSON.stringify({name, type: 'text', attrs: {visibility: 'when_set'}}), + }); + ledger.add(() => + doFetchOrThrow(client, `${client.getBaseRoute()}/custom_profile_attributes/fields/${field.id}`, { + method: 'DELETE', + }), + ); + return {id: field.id, name: field.name}; +} + +/** + * Set a CPA value on a user, addressing the field by its ID. Avoids the + * field-name-to-ID lookup in the shared `setUserAttribute` helper, which is + * unnecessary indirection now that callers hold the field handle directly. + */ +export async function setUserAttributeById(client: Client4, userId: string, fieldId: string, value: string) { + await client.updateUserCustomProfileAttributesValues(userId, {[fieldId]: value}); +} + +/** + * Create a public channel and register cleanup for it. + */ +export async function createTrackedPublicChannel(client: Client4, teamId: string, ledger: CleanupLedger) { + const id = getRandomId(); + const channel = await client.createChannel({ + team_id: teamId, + name: `pub-${id}`, + display_name: `PUB-${id}`, + type: 'O', + } as any); + ledger.add(() => client.deleteChannel(channel.id)); + return channel; +} + +/** + * Create a parent ABAC policy with the given CEL rule and assign the given + * channels in one shot. Registers cleanup so the policy (and its inherited + * channel-scope children) gets removed even if the test bails partway. + * + * Why we don't reuse `createParentPolicy` from team_settings/helpers.ts and + * patch afterwards: that helper hardcodes `expression: 'true'`, and a follow-up + * PUT to override the rule fails the server's policy-validation pipeline. + * Stamping the desired expression on the initial create is both simpler and + * matches what the system console actually does. + */ +export async function createPolicyAssignedToChannels( + client: Client4, + name: string, + expression: string, + channelIds: string[], + ledger: CleanupLedger, +) { + const policy: any = await doFetchOrThrow(client, `${client.getBaseRoute()}/access_control_policies`, { + method: 'PUT', + body: JSON.stringify({ + id: '', + name, + type: 'parent', + version: 'v0.2', + revision: 0, + rules: [{expression, actions: ['*']}], + }), + }); + + // Cleanup is self-sufficient: unassign first, then delete. The LIFO drain + // order in the spec runs this BEFORE the channel deletes, so the parent + // still has live child-scope policy rows referencing it — DeleteAccessControlPolicy + // would silently fail and leak the parent. Unassigning explicitly drops + // those children regardless of channel cleanup order. + ledger.add(async () => { + if (channelIds.length > 0) { + try { + await unassignChannelsFromPolicy(client, policy.id, channelIds); + } catch { + // Channels may already be gone (their cleanup tasks may have run + // first), in which case the child policies were auto-removed by + // cleanupChannelAccessControlPolicy. Either way, best-effort. + } + } + await deletePolicy(client, policy.id); + }); + + if (channelIds.length > 0) { + await assignChannelsToPolicy(client, policy.id, channelIds); + } + + return policy; +} + +/** + * Flip the auto-add (Active) flag on a channel-scope ABAC policy. Channel-scope + * policies share the channel's ID, so this is keyed by channelId. Active=true + * is what makes the access-control sync job auto-add matching users. + * + * Children inherit the parent's Active flag at assign time, but the parent + * default on a fresh `createPolicyAssignedToChannels` is false — flip the + * child here when the test needs auto-add ON. + */ +export async function setChannelPolicyActive(client: Client4, channelId: string, active: boolean): Promise { + await doFetchOrThrow(client, `${client.getBaseRoute()}/access_control_policies/activate`, { + method: 'PUT', + body: JSON.stringify({entries: [{id: channelId, active}]}), + }); +} + +/** + * Trigger an access-control sync job for a single policy and return the job + * record. The job is queued — call `waitForJobCompletion` to block until it + * resolves. + */ +export async function runAccessControlSyncJob(client: Client4, policyId: string): Promise { + return client.createJob({ + type: 'access_control_sync' as any, + data: {policy_id: policyId}, + }); +} + +/** + * Poll a job until it reaches a terminal status (success / error / canceled). + * Sync jobs are queued and processed asynchronously; in CI the queue can be + * idle for several seconds before pickup. The default 60s ceiling matches what + * we've seen empirically; callers can extend via `timeoutMs` when running on + * slow servers. + */ +export async function waitForJobCompletion( + client: Client4, + jobId: string, + opts: {timeoutMs?: number; pollIntervalMs?: number} = {}, +): Promise { + const timeoutMs = opts.timeoutMs ?? 60_000; + const pollIntervalMs = opts.pollIntervalMs ?? 1_000; + const deadline = Date.now() + timeoutMs; + + while (Date.now() < deadline) { + const job: any = await client.getJob(jobId); + if (job.status === 'success') { + return job; + } + if (job.status === 'error' || job.status === 'canceled') { + const detail = job?.data?.message ?? job?.message ?? job?.error ?? JSON.stringify(job?.data ?? job); + throw new Error(`Job ${jobId} finished with status ${job.status}: ${detail}`); + } + await new Promise((res) => setTimeout(res, pollIntervalMs)); + } + throw new Error(`Job ${jobId} did not reach a terminal status within ${timeoutMs}ms`); +} + +/** + * Create a user, add them to a team, optionally set a single attribute by ID, + * and bypass the tutorial / onboarding so tests can `pw.testBrowser.login()` + * straight into the channels view. Cleanup is registered for the user. + */ +export async function createTrackedTeamMember( + adminClient: Client4, + teamId: string, + attribute: {fieldId: string; value: string} | undefined, + ledger: CleanupLedger, +): Promise { + const id = getRandomId(); + const username = `pub${id}`.toLowerCase(); + const password = newTestPassword(); + + const user = await adminClient.createUser( + {email: `${username}@sample.mattermost.com`, username, password} as UserProfile & {password: string}, + '', + '', + ); + ledger.add(() => permanentDeleteUser(adminClient, user.id)); + + await adminClient.savePreferences(user.id, [ + {user_id: user.id, category: 'tutorial_step', name: user.id, value: '999'}, + {user_id: user.id, category: 'onboarding', name: 'complete', value: 'true'}, + ]); + await adminClient.addToTeam(teamId, user.id); + if (attribute) { + await setUserAttributeById(adminClient, user.id, attribute.fieldId, attribute.value); + } + + // Attach the password back to the user object so pw.testBrowser.login() can + // authenticate — the API response does not include it. + return {...user, password}; +} 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 97c0cd54404..ea83edabfee 100644 --- a/e2e-tests/playwright/specs/functional/channels/team_settings/helpers.ts +++ b/e2e-tests/playwright/specs/functional/channels/team_settings/helpers.ts @@ -16,6 +16,20 @@ export async function enableABACConfig(client: Client4) { }); } +/** + * Enable `ServiceSettings.EnableAPIUserDeletion` so test cleanup can permanently + * delete the throwaway users it created. Without this, test cleanup spams the + * console with "Permanent user deletion feature is not enabled" errors and leaks + * disabled-but-not-deleted test users on the server. + */ +export async function enableAPIUserDeletion(client: Client4) { + await client.patchConfig({ + ServiceSettings: { + EnableAPIUserDeletion: true, + }, + }); +} + export async function ensureDepartmentAttribute(client: Client4) { let fields: any[] = []; try { @@ -130,6 +144,57 @@ export async function setUserAttribute(adminClient: Client4, userId: string, fie await adminClient.updateUserCustomProfileAttributesValues(userId, {[field.id]: value}); } +/** + * Wait until all `expectedUserIds` show up as matching `expression` via the + * access-control CEL test endpoint. + * + * Why this exists: ABAC queries (validateExpressionAgainstRequester, + * calculateMembershipChanges) read from a Postgres materialized view + * (`AttributeView`). 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 + * `requester_matches: false` and surface the self-exclusion modal instead of + * the membership-changes confirmation modal. + * + * Polling forces wall-clock time to advance; the next CEL query after the 30s + * gate elapses will refresh the view and pick up the new attribute values. + * Default timeout 45s = 30s gate + 15s headroom. + */ +export async function waitForAttributeViewToInclude( + adminClient: Client4, + expression: string, + expectedUserIds: string[], + timeoutMs = 45_000, + pollIntervalMs = 1_000, +): Promise { + const deadline = Date.now() + timeoutMs; + let lastSeen = new Set(); + while (Date.now() < deadline) { + const response: any = await (adminClient as any).doFetch( + `${adminClient.getBaseRoute()}/access_control_policies/cel/test`, + { + method: 'post', + body: JSON.stringify({ + expression, + term: '', + after: '', + limit: 1000, + }), + }, + ); + lastSeen = new Set((response?.users || []).map((u: any) => u.id)); + if (expectedUserIds.every((id) => lastSeen.has(id))) { + return; + } + await new Promise((resolve) => setTimeout(resolve, pollIntervalMs)); + } + const missing = expectedUserIds.filter((id) => !lastSeen.has(id)); + throw new Error( + `AttributeView did not include users [${missing.join(', ')}] for expression "${expression}" within ${timeoutMs}ms`, + ); +} + export async function createPrivateChannel(client: Client4, teamId: string) { const id = Date.now().toString(36) + Math.random().toString(36).substring(2, 7); return client.createChannel({team_id: teamId, name: `abac-${id}`, display_name: `ABAC-${id}`, type: 'P'} as any); diff --git a/e2e-tests/playwright/specs/functional/system_console/abac/policies/channel_integration.spec.ts b/e2e-tests/playwright/specs/functional/system_console/abac/policies/channel_integration.spec.ts index a6030159fe7..c6a2899ef10 100644 --- a/e2e-tests/playwright/specs/functional/system_console/abac/policies/channel_integration.spec.ts +++ b/e2e-tests/playwright/specs/functional/system_console/abac/policies/channel_integration.spec.ts @@ -127,9 +127,7 @@ test.describe('ABAC Policies - Channel Integration', () => { await systemConsolePage.page.waitForTimeout(500); // Select policy in modal - const modal = systemConsolePage.page - .locator('[role="dialog"]') - .filter({hasText: 'Select an Access Control Policy'}); + const modal = systemConsolePage.page.locator('[role="dialog"]').filter({hasText: 'Select a Membership Policy'}); await modal.waitFor({state: 'visible', timeout: 5000}); const modalSearch = modal.locator('[data-testid="searchInput"]'); diff --git a/server/channels/api4/access_control.go b/server/channels/api4/access_control.go index cf9d08fd926..8d31d855789 100644 --- a/server/channels/api4/access_control.go +++ b/server/channels/api4/access_control.go @@ -870,7 +870,6 @@ func searchChannelsForAccessControlPolicy(c *Context, w http.ResponseWriter, r * opts := model.ChannelSearchOpts{ Deleted: props.Deleted, IncludeDeleted: props.IncludeDeleted, - Private: true, ExcludeGroupConstrained: true, TeamIds: teamIds, ParentAccessControlPolicyId: policyID, diff --git a/server/channels/api4/access_control_test.go b/server/channels/api4/access_control_test.go index b0986923b48..3f5a5a956f7 100644 --- a/server/channels/api4/access_control_test.go +++ b/server/channels/api4/access_control_test.go @@ -5,6 +5,7 @@ package api4 import ( "context" + "net/http" "os" "testing" @@ -326,6 +327,44 @@ func TestCreateAccessControlPolicy(t *testing.T) { require.NoError(t, err) CheckOKStatus(t, resp) }) + + t.Run("system admin cannot create a channel-scope policy on a team default channel", func(t *testing.T) { + // The api4 handler short-circuits validation for system admins, so the + // eligibility guard must live in the app layer. This test rides that + // path: SystemAdmin → handler skips ValidateChannelAccessControlPolicyCreation + // → CreateOrUpdateAccessControlPolicy must still reject default channels. + ok := th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterpriseAdvanced)) + require.True(t, ok, "SetLicense should return true") + + mockAccessControlService := &mocks.AccessControlServiceInterface{} + th.App.Srv().Channels().AccessControl = mockAccessControlService + // SavePolicy should never be reached — the guard rejects before that. + mockAccessControlService.On("SavePolicy", mock.Anything, mock.Anything). + Return(nil, model.NewAppError("SavePolicy", "should.not.be.called", nil, "", http.StatusInternalServerError)).Maybe() + + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.AccessControlSettings.EnableAttributeBasedAccessControl = model.NewPointer(true) + }) + + townSquare, appErr := th.App.GetChannelByName(th.Context, model.DefaultChannelName, th.BasicTeam.Id, false) + require.Nil(t, appErr) + + defaultChannelPolicy := &model.AccessControlPolicy{ + ID: townSquare.Id, + Type: model.AccessControlPolicyTypeChannel, + Name: "default-channel-policy", + Version: model.AccessControlPolicyVersionV0_3, + Revision: 1, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{"membership"}, Expression: "true"}, + }, + } + + _, resp, err := th.SystemAdminClient.CreateAccessControlPolicy(context.Background(), defaultChannelPolicy) + require.Error(t, err, "default channels must not accept ABAC policies, even for system admins") + CheckBadRequestStatus(t, resp) + mockAccessControlService.AssertNotCalled(t, "SavePolicy", mock.Anything, mock.Anything) + }) } func TestGetAccessControlPolicy(t *testing.T) { @@ -1066,20 +1105,113 @@ func TestSearchChannelsForAccessControlPolicy(t *testing.T) { require.NotNil(t, channelsResp) }) + t.Run("public channels assigned to the policy appear in search results", func(t *testing.T) { + setupLicenseAndABAC(t) + + parentPolicy := newSamplePolicy() + savedParent, err := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, parentPolicy) + require.NoError(t, err) + t.Cleanup(func() { + _ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, savedParent.ID) + }) + + // Public channels were previously hidden from this search by a hardcoded + // Private: true filter. Removing that filter is the whole point of the + // public-channel ABAC change; this test prevents regressions if someone + // re-introduces the filter in a future cleanup. + publicChannel := th.CreateChannelWithClientAndTeam(t, th.SystemAdminClient, model.ChannelTypeOpen, th.BasicTeam.Id) + childPolicy := &model.AccessControlPolicy{ + ID: publicChannel.Id, + Type: model.AccessControlPolicyTypeChannel, + Version: model.AccessControlPolicyVersionV0_3, + Revision: 1, + Imports: []string{savedParent.ID}, + Rules: []model.AccessControlPolicyRule{ + { + Expression: "user.attributes.team == 'engineering'", + Actions: []string{"membership"}, + }, + }, + } + _, err = th.App.Srv().Store().AccessControlPolicy().Save(th.Context, childPolicy) + require.NoError(t, err) + t.Cleanup(func() { + _ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, publicChannel.Id) + }) + + channelsResp, resp, err := th.SystemAdminClient.SearchChannelsForAccessControlPolicy( + context.Background(), savedParent.ID, + model.ChannelSearch{TeamIds: []string{th.BasicTeam.Id}}) + require.NoError(t, err) + CheckOKStatus(t, resp) + require.NotNil(t, channelsResp) + + channelsByID := make(map[string]*model.ChannelWithTeamData, len(channelsResp.Channels)) + for _, ch := range channelsResp.Channels { + channelsByID[ch.Id] = ch + } + require.Contains(t, channelsByID, publicChannel.Id, + "public channel assigned to the policy should appear in search results") + require.Equal(t, model.ChannelTypeOpen, channelsByID[publicChannel.Id].Type, + "expected the matched channel to be public") + + // Same fetch via the team-admin path used by the team-settings policy + // editor (?team_id=…). The team-scoped branch must also surface public + // channels — there's no longer any reason to filter them out. + th.LinkUserToTeam(t, th.TeamAdminUser, th.BasicTeam) + th.UpdateUserToTeamAdmin(t, th.TeamAdminUser, th.BasicTeam) + th.LoginTeamAdmin(t) + t.Cleanup(func() { th.LoginBasic(t) }) + + teamScopedResp, resp, err := th.Client.SearchChannelsForAccessControlPolicyForTeam( + context.Background(), savedParent.ID, th.BasicTeam.Id, model.ChannelSearch{}) + require.NoError(t, err) + CheckOKStatus(t, resp) + require.NotNil(t, teamScopedResp) + + teamChannelsByID := make(map[string]*model.ChannelWithTeamData, len(teamScopedResp.Channels)) + for _, ch := range teamScopedResp.Channels { + teamChannelsByID[ch.Id] = ch + } + require.Contains(t, teamChannelsByID, publicChannel.Id, + "team-admin policy editor must also surface public channels assigned to the policy") + }) + t.Run("team admin body TeamIds forced to authorized team", func(t *testing.T) { setupLicenseAndABAC(t) - policy := newSamplePolicy() - savedPolicy, err := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, policy) + parentPolicy := newSamplePolicy() + savedParent, err := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, parentPolicy) require.NoError(t, err) defer func() { - _ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, savedPolicy.ID) + _ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, savedParent.ID) + }() + + // Two teams, each with one private channel. The BasicTeam channel is + // linked to the parent policy so it shows up in the search; the + // otherTeam channel is unrelated. The override-correctness test then + // proves both that the BasicTeam channel IS returned (the search + // isn't trivially empty) and that the otherTeam channel is NOT + // returned even though the request body asked for it explicitly. + basicTeamChannel := th.CreateChannelWithClientAndTeam(t, th.SystemAdminClient, model.ChannelTypePrivate, th.BasicTeam.Id) + basicTeamChild := &model.AccessControlPolicy{ + ID: basicTeamChannel.Id, + Type: model.AccessControlPolicyTypeChannel, + Version: model.AccessControlPolicyVersionV0_3, + Revision: 1, + Imports: []string{savedParent.ID}, + Rules: []model.AccessControlPolicyRule{ + {Expression: "user.attributes.team == 'engineering'", Actions: []string{"membership"}}, + }, + } + _, err = th.App.Srv().Store().AccessControlPolicy().Save(th.Context, basicTeamChild) + require.NoError(t, err) + defer func() { + _ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, basicTeamChannel.Id) }() - // Create a second team with a private channel otherTeam := th.CreateTeam(t) otherChannel := th.CreateChannelWithClientAndTeam(t, th.SystemAdminClient, model.ChannelTypePrivate, otherTeam.Id) - _ = otherChannel th.LinkUserToTeam(t, th.TeamAdminUser, th.BasicTeam) th.UpdateUserToTeamAdmin(t, th.TeamAdminUser, th.BasicTeam) @@ -1089,19 +1221,26 @@ func TestSearchChannelsForAccessControlPolicy(t *testing.T) { // Attempt to search with body TeamIds pointing to a different team. // The authZ is against BasicTeam (via team_id query param), but the - // body tries to query otherTeam's channels. The fix should force + // body tries to query otherTeam's channels. The handler should force // TeamIds to BasicTeam.Id regardless of what the body says. channelsResp, resp, err := th.Client.SearchChannelsForAccessControlPolicyForTeam( - context.Background(), savedPolicy.ID, th.BasicTeam.Id, + context.Background(), savedParent.ID, th.BasicTeam.Id, model.ChannelSearch{TeamIds: []string{otherTeam.Id}}) require.NoError(t, err) CheckOKStatus(t, resp) require.NotNil(t, channelsResp) - // None of the returned channels should belong to the other team + channelsByID := make(map[string]*model.ChannelWithTeamData, len(channelsResp.Channels)) + for _, ch := range channelsResp.Channels { + channelsByID[ch.Id] = ch + } + require.Contains(t, channelsByID, basicTeamChannel.Id, + "BasicTeam channel must surface — proves the search is exercised, not just trivially empty") + require.NotContains(t, channelsByID, otherChannel.Id, + "otherTeam channel must NOT surface even though body asked for it — proves the team_id query param overrides body TeamIds") for _, ch := range channelsResp.Channels { require.Equal(t, th.BasicTeam.Id, ch.TeamId, - "team admin should only see channels from the authorized team, got channel %s from team %s", ch.Id, ch.TeamId) + "team admin must only see channels from the authorized team, got channel %s from team %s", ch.Id, ch.TeamId) } }) diff --git a/server/channels/api4/channel.go b/server/channels/api4/channel.go index 445b46ae1fa..c7fc2ad9d54 100644 --- a/server/channels/api4/channel.go +++ b/server/channels/api4/channel.go @@ -32,6 +32,7 @@ func (api *API) InitChannel() { api.BaseRoutes.ChannelsForTeam.Handle("", api.APISessionRequired(getPublicChannelsForTeam)).Methods(http.MethodGet) api.BaseRoutes.ChannelsForTeam.Handle("/deleted", api.APISessionRequired(getDeletedChannelsForTeam)).Methods(http.MethodGet) api.BaseRoutes.ChannelsForTeam.Handle("/private", api.APISessionRequired(getPrivateChannelsForTeam)).Methods(http.MethodGet) + api.BaseRoutes.ChannelsForTeam.Handle("/recommended", api.APISessionRequired(getRecommendedChannelsForTeam)).Methods(http.MethodGet) api.BaseRoutes.ChannelsForTeam.Handle("/ids", api.APISessionRequired(getPublicChannelsByIdsForTeam)).Methods(http.MethodPost) api.BaseRoutes.ChannelsForTeam.Handle("/search", api.APISessionRequiredDisableWhenBusy(searchChannelsForTeam)).Methods(http.MethodPost) api.BaseRoutes.ChannelsForTeam.Handle("/autocomplete", api.APISessionRequired(autocompleteChannelsForTeam)).Methods(http.MethodGet) @@ -1026,6 +1027,31 @@ func getPublicChannelsForTeam(c *Context, w http.ResponseWriter, r *http.Request } } +// getRecommendedChannelsForTeam returns public channels in the team with an +// active ABAC policy that the requesting user's attributes satisfy. The list +// is consumed by the "Recommended channels" feature in the browse UI. +func getRecommendedChannelsForTeam(c *Context, w http.ResponseWriter, r *http.Request) { + c.RequireTeamId() + if c.Err != nil { + return + } + + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), c.Params.TeamId, model.PermissionListTeamChannels) { + c.SetPermissionError(model.PermissionListTeamChannels) + return + } + + channels, err := c.App.GetRecommendedPublicChannelsForUser(c.AppContext, c.AppContext.Session().UserId, c.Params.TeamId) + if err != nil { + c.Err = err + return + } + + if err := json.NewEncoder(w).Encode(channels); err != nil { + c.Logger.Warn("Error while writing response", mlog.Err(err)) + } +} + func getDeletedChannelsForTeam(c *Context, w http.ResponseWriter, r *http.Request) { c.RequireTeamId() if c.Err != nil { diff --git a/server/channels/api4/channel_test.go b/server/channels/api4/channel_test.go index b03f9dd59d1..252aed001df 100644 --- a/server/channels/api4/channel_test.go +++ b/server/channels/api4/channel_test.go @@ -2400,6 +2400,127 @@ func TestGetPublicChannelsForTeam(t *testing.T) { }) } +func TestGetRecommendedChannelsForTeam(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + t.Run("without enterprise license ABAC is disabled so the endpoint returns an empty list", func(t *testing.T) { + // Be explicit about the license precondition so this subtest is + // deterministic even if a parallel test elsewhere installed one. + appErr := th.App.Srv().RemoveLicense() + require.Nil(t, appErr) + + resp, err := th.Client.DoAPIGet(context.Background(), "/teams/"+th.BasicTeam.Id+"/channels/recommended", "") + require.NoError(t, err) + require.NotNil(t, resp) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + var channels []*model.Channel + require.NoError(t, json.NewDecoder(resp.Body).Decode(&channels)) + require.Empty(t, channels) + }) + + t.Run("user must be on the team", func(t *testing.T) { + otherTeamUser := th.CreateUser(t) + client := th.CreateClient() + _, _, err := client.Login(context.Background(), otherTeamUser.Email, otherTeamUser.Password) + require.NoError(t, err) + + resp, err := client.DoAPIGet(context.Background(), "/teams/"+th.BasicTeam.Id+"/channels/recommended", "") + require.Error(t, err) + // resp can be nil on transport errors; only defer Close when we + // actually got an HTTP response back so we can assert the status. + require.NotNil(t, resp) + defer resp.Body.Close() + require.Equal(t, http.StatusForbidden, resp.StatusCode) + }) + + t.Run("returns policy-enforced channels the requester matches under enterprise license", func(t *testing.T) { + // License + ABAC config gate the endpoint; without these it short-circuits + // to an empty list (covered by the no-license subtest above). + ok := th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterpriseAdvanced)) + require.True(t, ok, "SetLicense should return true") + t.Cleanup(func() { _ = th.App.Srv().RemoveLicense() }) + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.AccessControlSettings.EnableAttributeBasedAccessControl = model.NewPointer(true) + }) + + // Wire a mock ABAC service that allows the requester for `included` + // and denies for `excluded`. This pins the api4 layer's responsibility + // (routing, permissions, response shape) to a deterministic policy + // outcome — the underlying CEL evaluation has its own coverage at the + // app layer in TestGetRecommendedPublicChannelsForUser. + mockACS := &einterfacesmocks.AccessControlServiceInterface{} + originalACS := th.App.Srv().Channels().AccessControl + th.App.Srv().Channels().AccessControl = mockACS + t.Cleanup(func() { th.App.Srv().Channels().AccessControl = originalACS }) + // PermanentDeleteChannel during cleanup calls DeletePolicy on the ACS. + mockACS.On("DeletePolicy", mock.Anything, mock.AnythingOfType("string")). + Return((*model.AppError)(nil)).Maybe() + + included, _, _ := th.SystemAdminClient.CreateChannel(context.Background(), &model.Channel{ + TeamId: th.BasicTeam.Id, + Type: model.ChannelTypeOpen, + Name: "abac-recommended-" + model.NewId(), + DisplayName: "ABAC Recommended", + }) + require.NotNil(t, included) + t.Cleanup(func() { + _ = th.App.PermanentDeleteChannel(th.Context, included) + }) + + excluded, _, _ := th.SystemAdminClient.CreateChannel(context.Background(), &model.Channel{ + TeamId: th.BasicTeam.Id, + Type: model.ChannelTypeOpen, + Name: "abac-excluded-" + model.NewId(), + DisplayName: "ABAC Excluded", + }) + require.NotNil(t, excluded) + t.Cleanup(func() { + _ = th.App.PermanentDeleteChannel(th.Context, excluded) + }) + + // Stamp channel-scope policy rows so SearchAllChannels picks them up + // as PolicyEnforced=true. The expressions are placeholders — the mock + // ACS short-circuits evaluation below. + for _, ch := range []*model.Channel{included, excluded} { + _, err := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, &model.AccessControlPolicy{ + ID: ch.Id, + Type: model.AccessControlPolicyTypeChannel, + Version: model.AccessControlPolicyVersionV0_2, + Revision: 1, + Active: true, + Rules: []model.AccessControlPolicyRule{{Actions: []string{"membership"}, Expression: "true"}}, + }) + require.NoError(t, err) + } + + mockACS.On("AccessEvaluation", mock.Anything, mock.MatchedBy(func(req model.AccessRequest) bool { + return req.Resource.ID == included.Id + })).Return(model.AccessDecision{Decision: true}, (*model.AppError)(nil)) + mockACS.On("AccessEvaluation", mock.Anything, mock.MatchedBy(func(req model.AccessRequest) bool { + return req.Resource.ID == excluded.Id + })).Return(model.AccessDecision{Decision: false}, (*model.AppError)(nil)) + + resp, err := th.Client.DoAPIGet(context.Background(), "/teams/"+th.BasicTeam.Id+"/channels/recommended", "") + require.NoError(t, err) + require.NotNil(t, resp) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + var channels []*model.Channel + require.NoError(t, json.NewDecoder(resp.Body).Decode(&channels)) + + ids := make(map[string]bool, len(channels)) + for _, ch := range channels { + ids[ch.Id] = true + } + require.True(t, ids[included.Id], "policy-allowed channel must be returned") + require.False(t, ids[excluded.Id], "policy-denied channel must be filtered out") + }) +} + func TestGetPublicChannelsByIdsForTeam(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 91d996dfcc4..f30a20560d3 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -943,8 +943,33 @@ func getUsers(c *Context, w http.ResponseWriter, r *http.Request) { return } - if ok, _ := c.App.ChannelAccessControlled(c.AppContext, notInChannelId); ok { - // Get cursor_id from query parameters for cursor-based pagination + // ABAC filtering is mandatory for private policy-enforced channels (hard gate). + // For public policy-enforced channels, ABAC is advisory — only apply the + // filter when the caller explicitly asks via abac_match_only=true so callers + // like the invite modal can fetch all team members and annotate the matching + // subset without the list being narrowed for them. + // + // Surface ChannelAccessControlled errors instead of silently swallowing + // them — a transient store / license read failure here would otherwise + // fall through to the unfiltered path and could expose users a hard-gated + // private channel was configured to hide. + abacMatchOnly, _ := strconv.ParseBool(r.URL.Query().Get("abac_match_only")) + useAbacFilter := false + enforced, enforcedErr := c.App.ChannelAccessControlled(c.AppContext, notInChannelId) + if enforcedErr != nil { + c.Err = enforcedErr + return + } + if enforced { + ch, chErr := c.App.GetChannel(c.AppContext, notInChannelId) + if chErr != nil { + c.Err = chErr + return + } + useAbacFilter = ch.Type == model.ChannelTypePrivate || abacMatchOnly + } + + if useAbacFilter { cursorId := r.URL.Query().Get("cursor_id") profiles, appErr = c.App.GetUsersNotInAbacChannel(c.AppContext, inTeamId, notInChannelId, groupConstrainedBool, cursorId, c.Params.PerPage, c.IsSystemAdmin(), restrictions) } else { diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index 64d88c7dba8..b84cb0aeeea 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -3534,6 +3534,126 @@ func TestGetUsersNotInChannel(t *testing.T) { require.NoError(t, err) } +// TestGetUsersNotInChannelAbacMatchOnly exercises the dispatcher in +// getUsers that decides whether to apply ABAC filtering based on the +// channel type and the abac_match_only query parameter. The underlying +// ABAC store path (GetUsersNotInAbacChannel) has its own coverage in +// app/user_test.go; here we only assert the dispatch wiring. +func TestGetUsersNotInChannelAbacMatchOnly(t *testing.T) { + th := Setup(t).InitBasic(t) + + ok := th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterpriseAdvanced)) + require.True(t, ok, "SetLicense should return true") + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.AccessControlSettings.EnableAttributeBasedAccessControl = true + }) + + teamId := th.BasicTeam.Id + user1 := th.CreateUser(t) + user2 := th.CreateUser(t) + th.LinkUserToTeam(t, user1, th.BasicTeam) + th.LinkUserToTeam(t, user2, th.BasicTeam) + + privateChannel := th.CreateChannelWithClientAndTeam(t, th.SystemAdminClient, model.ChannelTypePrivate, th.BasicTeam.Id) + publicChannel := th.CreateChannelWithClientAndTeam(t, th.SystemAdminClient, model.ChannelTypeOpen, th.BasicTeam.Id) + + // Add BasicUser as a member so th.Client (a regular user) has + // PermissionReadChannel on the target — the endpoint isn't sys-admin gated, + // it just requires read access on the channel. Membership is added before + // the ABAC policy is saved so the AddUserToChannel path doesn't go through + // the policy gate (which is unrelated to what we're testing here). + th.AddUserToChannel(t, th.BasicUser, privateChannel) + th.AddUserToChannel(t, th.BasicUser, publicChannel) + + saveChannelPolicy := func(channelID string) { + policy := &model.AccessControlPolicy{ + ID: channelID, + Type: model.AccessControlPolicyTypeChannel, + Revision: 1, + Version: model.AccessControlPolicyVersionV0_2, + Active: true, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{"membership"}, Expression: "true"}, + }, + } + _, err := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, policy) + require.NoError(t, err) + // PolicyEnforced is computed at channel-fetch time and cached. Adding + // BasicUser as a member above populated the cache with PolicyEnforced=false; + // invalidate so ChannelAccessControlled (the dispatcher's gate) sees the + // freshly-saved policy on subsequent reads. + th.App.Srv().Store().Channel().InvalidateChannel(channelID) + t.Cleanup(func() { + _ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, channelID) + }) + } + saveChannelPolicy(privateChannel.Id) + saveChannelPolicy(publicChannel.Id) + + mockACS := &mocks.AccessControlServiceInterface{} + originalACS := th.App.Srv().Channels().AccessControl + th.App.Srv().Channels().AccessControl = mockACS + t.Cleanup(func() { th.App.Srv().Channels().AccessControl = originalACS }) + + // QueryUsersForResource is the ABAC path; whenever it is hit, we return + // only user1 — that's the signal the dispatcher routed to the filtered + // branch. user2 only ever appears via the unfiltered store path. + // + // The third argument is pinned to the actual action constant + // (`AccessControlPolicyActionMembership`) so the mock matches the real + // call site in App.GetUsersNotInAbacChannel — using `"*"` here would + // silently never match and the whole test would PASS by accident on the + // fall-through path. + mockACS.On("QueryUsersForResource", + mock.Anything, + mock.AnythingOfType("string"), + model.AccessControlPolicyActionMembership, + mock.Anything, + ).Return([]*model.User{user1}, int64(1), nil).Maybe() + + listUsers := func(t *testing.T, channelID string, abacMatchOnly bool) []string { + t.Helper() + query := url.Values{} + query.Set("in_team", teamId) + query.Set("not_in_channel", channelID) + query.Set("page", "0") + query.Set("per_page", "200") + if abacMatchOnly { + query.Set("abac_match_only", "true") + } + resp, err := th.Client.DoAPIGet(context.Background(), "/users?"+query.Encode(), "") + require.NoError(t, err) + require.NotNil(t, resp) + defer resp.Body.Close() + + var users []*model.User + require.NoError(t, json.NewDecoder(resp.Body).Decode(&users)) + ids := make([]string, 0, len(users)) + for _, u := range users { + ids = append(ids, u.Id) + } + return ids + } + + t.Run("private policy channel: ABAC filter applied without flag", func(t *testing.T) { + ids := listUsers(t, privateChannel.Id, false) + require.Contains(t, ids, user1.Id) + require.NotContains(t, ids, user2.Id, "private policy channel must hard-gate non-matching users") + }) + + t.Run("public policy channel: full list returned without flag", func(t *testing.T) { + ids := listUsers(t, publicChannel.Id, false) + require.Contains(t, ids, user1.Id) + require.Contains(t, ids, user2.Id, "public policy channel without abac_match_only must return non-matching users so callers can annotate them") + }) + + t.Run("public policy channel: ABAC filter applied with abac_match_only=true", func(t *testing.T) { + ids := listUsers(t, publicChannel.Id, true) + require.Contains(t, ids, user1.Id) + require.NotContains(t, ids, user2.Id, "abac_match_only=true on a public policy channel must drop non-matching users") + }) +} + func TestGetUsersInGroup(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) diff --git a/server/channels/app/access_control.go b/server/channels/app/access_control.go index 1d15c76fc1e..885c1ee745c 100644 --- a/server/channels/app/access_control.go +++ b/server/channels/app/access_control.go @@ -86,6 +86,23 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model. policy.ID = model.NewId() } + // Channel-scope policies are pinned to a single channel by ID. Validate + // channel eligibility here (default / DM / GM / group-constrained / shared + // channels are ineligible) so this guard protects all callers — including + // system admins, whose request goes through the api4 handler's permission + // fast-path that skips the per-channel ValidateChannelAccessControlPolicyCreation + // check, and the parent-policy AssignAccessControlPolicyToChannels flow, + // which validates eligibility there but bypasses this entry point. + if policy.Type == model.AccessControlPolicyTypeChannel { + channel, appErr := a.GetChannel(rctx, policy.ID) + if appErr != nil { + return nil, appErr + } + if appErr := a.ValidateChannelEligibilityForAccessControl(rctx, channel); appErr != nil { + return nil, appErr + } + } + policy.Version = model.AccessControlPolicyVersionV0_3 for i, rule := range policy.Rules { for j, action := range rule.Actions { @@ -193,7 +210,7 @@ func (a *App) AssignAccessControlPolicyToChannels(rctx request.CTX, parentID str policies := make([]*model.AccessControlPolicy, 0, len(channelIDs)) for _, channel := range channels { - if appErr := ValidateChannelEligibilityForAccessControl(channel); appErr != nil { + if appErr := a.ValidateChannelEligibilityForAccessControl(rctx, channel); appErr != nil { return nil, appErr } @@ -464,12 +481,13 @@ func (a *App) publishChannelPolicyEnforcedUpdate(rctx request.CTX, channelID str } // ValidateChannelEligibilityForAccessControl checks that a channel is eligible for -// access control policy assignment: must be private, not group-constrained, not shared. -func ValidateChannelEligibilityForAccessControl(channel *model.Channel) *model.AppError { - if channel.Type != model.ChannelTypePrivate { +// access control policy assignment: must be public or private (DM/GM excluded), +// not group-constrained, not shared, and not a team default channel (e.g. town-square). +func (a *App) ValidateChannelEligibilityForAccessControl(rctx request.CTX, channel *model.Channel) *model.AppError { + if channel.Type != model.ChannelTypePrivate && channel.Type != model.ChannelTypeOpen { return model.NewAppError("ValidateChannelEligibilityForAccessControl", - "app.pap.access_control.channel_not_private", - nil, "Channel is not of type private", http.StatusBadRequest) + "app.pap.access_control.channel_type_not_supported", + nil, "Policies can only be applied to public or private channels", http.StatusBadRequest) } if channel.IsGroupConstrained() { @@ -484,6 +502,12 @@ func ValidateChannelEligibilityForAccessControl(channel *model.Channel) *model.A nil, "Channel is shared", http.StatusBadRequest) } + if slices.Contains(a.DefaultChannelNames(rctx), channel.Name) { + return model.NewAppError("ValidateChannelEligibilityForAccessControl", + "app.pap.access_control.channel_default", + nil, "Channel is a team default channel", http.StatusBadRequest) + } + return nil } @@ -500,7 +524,7 @@ func (a *App) ValidateChannelAccessControlPermission(rctx request.CTX, userID, c return model.NewAppError("ValidateChannelAccessControlPermission", "app.pap.access_control.insufficient_channel_permissions", nil, "user_id="+userID+" channel_id="+channelID, http.StatusForbidden) } - if appErr := ValidateChannelEligibilityForAccessControl(channel); appErr != nil { + if appErr := a.ValidateChannelEligibilityForAccessControl(rctx, channel); appErr != nil { return appErr } diff --git a/server/channels/app/access_control_test.go b/server/channels/app/access_control_test.go index 2e9a3c9497c..51cb7d4aa1c 100644 --- a/server/channels/app/access_control_test.go +++ b/server/channels/app/access_control_test.go @@ -124,7 +124,13 @@ func TestCreateOrUpdateAccessControlPolicy(t *testing.T) { // publishChannelPolicyEnforcedUpdate is expected to invalidate the // channel cache and reload the channel for the WS payload. mockChannelStore.On("InvalidateChannel", channelID).Once() - mockChannelStore.On("Get", channelID, true).Return(&model.Channel{Id: channelID, Type: model.ChannelTypePrivate}, nil).Once() + // Channel().Get is now hit twice during a successful save: + // 1. ValidateChannelEligibilityForAccessControl loads the channel + // to enforce the default / DM / GM / group-constrained / shared + // eligibility rules before SavePolicy. + // 2. publishChannelPolicyEnforcedUpdate reloads it after save to + // build the WS payload. + mockChannelStore.On("Get", channelID, true).Return(&model.Channel{Id: channelID, Type: model.ChannelTypePrivate}, nil).Twice() mockAccessControl := &mocks.AccessControlServiceInterface{} thMock.App.Srv().ch.AccessControl = mockAccessControl @@ -139,6 +145,7 @@ func TestCreateOrUpdateAccessControlPolicy(t *testing.T) { mockAccessControl.AssertExpectations(t) mockChannelStore.AssertCalled(t, "InvalidateChannel", channelID) mockChannelStore.AssertCalled(t, "Get", channelID, true) + mockChannelStore.AssertExpectations(t) }) t.Run("Parent-type policy does not broadcast channel-only update", func(t *testing.T) { @@ -534,18 +541,23 @@ func TestAssignAccessControlPolicyToChannels(t *testing.T) { t.Run("Error saving policy", func(t *testing.T) { ch := th.CreatePrivateChannel(t, th.BasicTeam) - - mockAccessControl := &mocks.AccessControlServiceInterface{} - th.App.Srv().ch.AccessControl = mockAccessControl - mockAccessControl.On("GetPolicy", th.Context, parentID).Return(parentPolicy, nil) - mockAccessControl.On("GetPolicy", th.Context, ch.Id).Return(parentPolicy, nil) - mockAccessControl.On("SavePolicy", th.Context, mock.Anything).Return(nil, model.NewAppError("SavePolicy", "error", nil, "save error", http.StatusInternalServerError)) - t.Cleanup(func() { appErr := th.App.PermanentDeleteChannel(th.Context, ch) require.Nil(t, appErr) }) + mockAccessControl := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockAccessControl + // Clear the mock before the channel cleanup runs (LIFO: this + // cleanup is registered after the channel cleanup so it runs + // first), so PermanentDeleteChannel's cleanupChannelAccessControlPolicy + // is a no-op and doesn't hit an unmocked DeletePolicy. + t.Cleanup(func() { th.App.Srv().ch.AccessControl = nil }) + + mockAccessControl.On("GetPolicy", th.Context, parentID).Return(parentPolicy, nil) + mockAccessControl.On("GetPolicy", th.Context, ch.Id).Return(parentPolicy, nil) + mockAccessControl.On("SavePolicy", th.Context, mock.Anything).Return(nil, model.NewAppError("SavePolicy", "error", nil, "save error", http.StatusInternalServerError)) + policies, err := th.App.AssignAccessControlPolicyToChannels(th.Context, parentID, []string{ch.Id}) require.NotNil(t, err) require.Empty(t, policies) @@ -572,33 +584,33 @@ func TestAssignAccessControlPolicyToChannels(t *testing.T) { assert.Equal(t, "app.pap.assign_access_control_policy_to_channels.app_error", err.Id) }) - t.Run("Channel is not private", func(t *testing.T) { + t.Run("Default channel is not supported", func(t *testing.T) { mockAccessControl := &mocks.AccessControlServiceInterface{} th.App.Srv().ch.AccessControl = mockAccessControl mockAccessControl.On("GetPolicy", th.Context, parentID).Return(&model.AccessControlPolicy{Type: model.AccessControlPolicyTypeParent}, nil) - // Create a public channel - publicChannel := th.CreateChannel(t, th.BasicTeam) - t.Cleanup(func() { - appErr := th.App.PermanentDeleteChannel(th.Context, publicChannel) - require.Nil(t, appErr) - }) - policies, err := th.App.AssignAccessControlPolicyToChannels(th.Context, parentID, []string{publicChannel.Id}) + townSquare, appErr := th.App.GetChannelByName(th.Context, model.DefaultChannelName, th.BasicTeam.Id, false) + require.Nil(t, appErr) + + policies, err := th.App.AssignAccessControlPolicyToChannels(th.Context, parentID, []string{townSquare.Id}) require.NotNil(t, err) assert.Nil(t, policies) - assert.Contains(t, err.Error(), "Channel is not of type private") + assert.Equal(t, "app.pap.access_control.channel_default", err.Id) }) t.Run("Channel is shared", func(t *testing.T) { - mockAccessControl := &mocks.AccessControlServiceInterface{} - th.App.Srv().ch.AccessControl = mockAccessControl - mockAccessControl.On("GetPolicy", th.Context, parentID).Return(&model.AccessControlPolicy{Type: model.AccessControlPolicyTypeParent}, nil) - privateChannel := th.CreatePrivateChannel(t, th.BasicTeam) t.Cleanup(func() { appErr := th.App.PermanentDeleteChannel(th.Context, privateChannel) require.Nil(t, appErr) }) + + mockAccessControl := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockAccessControl + t.Cleanup(func() { th.App.Srv().ch.AccessControl = nil }) + + mockAccessControl.On("GetPolicy", th.Context, parentID).Return(&model.AccessControlPolicy{Type: model.AccessControlPolicyTypeParent}, nil) + privateChannel.Shared = model.NewPointer(true) _, err := th.App.Srv().Store().Channel().Update(th.Context, privateChannel) require.NoError(t, err) @@ -641,6 +653,8 @@ func TestAssignAccessControlPolicyToChannels(t *testing.T) { mockAccessControl := &mocks.AccessControlServiceInterface{} th.App.Srv().ch.AccessControl = mockAccessControl + t.Cleanup(func() { th.App.Srv().ch.AccessControl = nil }) + mockAccessControl.On("GetPolicy", th.Context, parentID).Return(parentPolicy, nil) mockAccessControl.On("GetPolicy", th.Context, ch1.Id).Return(nil, nil) mockAccessControl.On("GetPolicy", th.Context, ch2.Id).Return(nil, nil) @@ -656,6 +670,215 @@ func TestAssignAccessControlPolicyToChannels(t *testing.T) { }) } +func TestChannelDeleteCleansUpAccessControlPolicy(t *testing.T) { + th := Setup(t).InitBasic(t) + + // Wire up a mock ACS whose DeletePolicy writes through to the store, so the + // cleanup path exercised by DeleteChannel/PermanentDeleteChannel actually + // removes the row. Without this, cleanupChannelAccessControlPolicy is a + // no-op when the enterprise service is not registered. + mockACS := &mocks.AccessControlServiceInterface{} + originalACS := th.App.Srv().ch.AccessControl + th.App.Srv().ch.AccessControl = mockACS + t.Cleanup(func() { + th.App.Srv().ch.AccessControl = originalACS + }) + mockACS.On("DeletePolicy", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("string")). + Return(func(rctx request.CTX, id string) *model.AppError { + if err := th.App.Srv().Store().AccessControlPolicy().Delete(rctx, id); err != nil { + return model.NewAppError("DeletePolicy", "test.delete", nil, err.Error(), http.StatusInternalServerError) + } + return nil + }).Maybe() + + saveChildPolicy := func(t *testing.T, channelID string) { + t.Helper() + policy := &model.AccessControlPolicy{ + ID: channelID, + Type: model.AccessControlPolicyTypeChannel, + Revision: 1, + Version: model.AccessControlPolicyVersionV0_2, + Active: true, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{"membership"}, Expression: "true"}, + }, + } + saved, err := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, policy) + require.NoError(t, err) + require.NotNil(t, saved) + } + + t.Run("Archiving a channel deletes its channel-scope policy", func(t *testing.T) { + ch := th.CreatePrivateChannel(t, th.BasicTeam) + saveChildPolicy(t, ch.Id) + + // Sanity: policy exists before archive. + fetched, err := th.App.Srv().Store().AccessControlPolicy().Get(th.Context, ch.Id) + require.NoError(t, err) + require.NotNil(t, fetched) + + // Reload via GetChannel without invalidating the cache. The channel + // was created before the policy was saved directly to the store, so + // the cached channel still reports PolicyEnforced=false. Cleanup must + // still remove the orphan policy — it no longer trusts the stale + // cached flag. + reloaded, appErr := th.App.GetChannel(th.Context, ch.Id) + require.Nil(t, appErr) + + appErr = th.App.DeleteChannel(th.Context, reloaded, th.BasicUser.Id) + require.Nil(t, appErr) + + _, err = th.App.Srv().Store().AccessControlPolicy().Get(th.Context, ch.Id) + require.Error(t, err, "channel-scope policy should be removed when the channel is archived") + }) + + t.Run("Permanently deleting a channel deletes its channel-scope policy", func(t *testing.T) { + ch := th.CreatePrivateChannel(t, th.BasicTeam) + saveChildPolicy(t, ch.Id) + + reloaded, appErr := th.App.GetChannel(th.Context, ch.Id) + require.Nil(t, appErr) + + appErr = th.App.PermanentDeleteChannel(th.Context, reloaded) + require.Nil(t, appErr) + + _, err := th.App.Srv().Store().AccessControlPolicy().Get(th.Context, ch.Id) + require.Error(t, err, "channel-scope policy should be removed when the channel is permanently deleted") + }) + + t.Run("Archiving a channel with no policy still succeeds", func(t *testing.T) { + ch := th.CreatePrivateChannel(t, th.BasicTeam) + t.Cleanup(func() { + _ = th.App.PermanentDeleteChannel(th.Context, ch) + }) + + reloaded, appErr := th.App.GetChannel(th.Context, ch.Id) + require.Nil(t, appErr) + + // cleanupChannelAccessControlPolicy intentionally calls DeletePolicy + // unconditionally when acs is non-nil — DeletePolicy itself is + // expected to be a no-op when no matching row exists. + appErr = th.App.DeleteChannel(th.Context, reloaded, th.BasicUser.Id) + require.Nil(t, appErr) + }) + + t.Run("Falls back to direct store delete when acs is nil", func(t *testing.T) { + // Swap in a nil acs for the duration of this subtest so the cleanup + // must take the store-level fallback path (e.g. running on Team + // Edition where the enterprise ABAC service is not registered). + th.App.Srv().ch.AccessControl = nil + t.Cleanup(func() { th.App.Srv().ch.AccessControl = mockACS }) + + ch := th.CreatePrivateChannel(t, th.BasicTeam) + saveChildPolicy(t, ch.Id) + + reloaded, appErr := th.App.GetChannel(th.Context, ch.Id) + require.Nil(t, appErr) + + appErr = th.App.PermanentDeleteChannel(th.Context, reloaded) + require.Nil(t, appErr) + + _, err := th.App.Srv().Store().AccessControlPolicy().Get(th.Context, ch.Id) + require.Error(t, err, "policy should be removed via the store-level fallback when acs is nil") + }) + + t.Run("Falls back to direct store delete when acs reports NotImplemented", func(t *testing.T) { + // Replace mockACS with one that always reports the operation as + // unimplemented (e.g. license-gated build of the enterprise layer); + // cleanup must still drop the orphan row through the store fallback. + notImplementedACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = notImplementedACS + t.Cleanup(func() { th.App.Srv().ch.AccessControl = mockACS }) + notImplementedACS.On("DeletePolicy", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("string")). + Return(model.NewAppError("DeletePolicy", "app.pap.not_initialized", nil, "PAP not initialized", http.StatusNotImplemented)).Once() + + ch := th.CreatePrivateChannel(t, th.BasicTeam) + saveChildPolicy(t, ch.Id) + + reloaded, appErr := th.App.GetChannel(th.Context, ch.Id) + require.Nil(t, appErr) + + appErr = th.App.PermanentDeleteChannel(th.Context, reloaded) + require.Nil(t, appErr) + + notImplementedACS.AssertCalled(t, "DeletePolicy", mock.AnythingOfType("*request.Context"), ch.Id) + notImplementedACS.AssertExpectations(t) + + _, err := th.App.Srv().Store().AccessControlPolicy().Get(th.Context, ch.Id) + require.Error(t, err, "policy should be removed via the store-level fallback when acs reports NotImplemented") + }) +} + +func TestUpdateChannelBlocksTypeConversionWhenPolicyEnforced(t *testing.T) { + th := Setup(t).InitBasic(t) + + // ABAC + license required for ChannelAccessControlled to report `enforced=true`. + ok := th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterpriseAdvanced)) + require.True(t, ok, "SetLicense should return true") + t.Cleanup(func() { _ = th.App.Srv().RemoveLicense() }) + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.AccessControlSettings.EnableAttributeBasedAccessControl = model.NewPointer(true) + }) + + mockACS := &mocks.AccessControlServiceInterface{} + originalACS := th.App.Srv().ch.AccessControl + th.App.Srv().ch.AccessControl = mockACS + t.Cleanup(func() { th.App.Srv().ch.AccessControl = originalACS }) + mockACS.On("DeletePolicy", mock.Anything, mock.AnythingOfType("string")).Return((*model.AppError)(nil)).Maybe() + + stampPolicy := func(t *testing.T, channelID string) { + t.Helper() + _, err := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, &model.AccessControlPolicy{ + ID: channelID, + Type: model.AccessControlPolicyTypeChannel, + Version: model.AccessControlPolicyVersionV0_2, + Revision: 1, + Active: true, + Rules: []model.AccessControlPolicyRule{{Actions: []string{"membership"}, Expression: "true"}}, + }) + require.NoError(t, err) + // Channel().Get is cached; PolicyEnforced is computed at fetch time + // from the AccessControlPolicies table, so an existing cached entry + // would still report `false`. Invalidate so the next Get re-computes. + th.App.Srv().Store().Channel().InvalidateChannel(channelID) + } + + t.Run("private → public is rejected when ABAC policy is attached", func(t *testing.T) { + ch := th.CreatePrivateChannel(t, th.BasicTeam) + t.Cleanup(func() { _ = th.App.PermanentDeleteChannel(th.Context, ch) }) + stampPolicy(t, ch.Id) + + patch := *ch + patch.Type = model.ChannelTypeOpen + _, appErr := th.App.UpdateChannel(th.Context, &patch) + require.NotNil(t, appErr, "type conversion must be blocked while a policy is attached") + require.Equal(t, "api.channel.update_channel.policy_enforced_type_conversion.app_error", appErr.Id) + }) + + t.Run("public → private is rejected when ABAC policy is attached", func(t *testing.T) { + ch := th.CreateChannel(t, th.BasicTeam) + t.Cleanup(func() { _ = th.App.PermanentDeleteChannel(th.Context, ch) }) + stampPolicy(t, ch.Id) + + patch := *ch + patch.Type = model.ChannelTypePrivate + _, appErr := th.App.UpdateChannel(th.Context, &patch) + require.NotNil(t, appErr, "type conversion must be blocked in either direction") + require.Equal(t, "api.channel.update_channel.policy_enforced_type_conversion.app_error", appErr.Id) + }) + + t.Run("non-type updates still succeed on policy-enforced channels", func(t *testing.T) { + ch := th.CreatePrivateChannel(t, th.BasicTeam) + t.Cleanup(func() { _ = th.App.PermanentDeleteChannel(th.Context, ch) }) + stampPolicy(t, ch.Id) + + patch := *ch + patch.Header = "updated header" + _, appErr := th.App.UpdateChannel(th.Context, &patch) + require.Nil(t, appErr, "non-type updates should pass through; the gate is type-conversion only") + }) +} + func TestUnassignPoliciesFromChannels(t *testing.T) { th := Setup(t).InitBasic(t) @@ -674,8 +897,7 @@ func TestUnassignPoliciesFromChannels(t *testing.T) { require.NoError(t, err) require.NotNil(t, parentPolicy) t.Cleanup(func() { - sErr := th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, parentPolicy.ID) - require.NoError(t, sErr) + _ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, parentPolicy.ID) }) ch1 := th.CreatePrivateChannel(t, th.BasicTeam) @@ -689,56 +911,81 @@ func TestUnassignPoliciesFromChannels(t *testing.T) { require.Nil(t, sErr) }) - childPolicy1 := &model.AccessControlPolicy{ - Type: model.AccessControlPolicyTypeChannel, - ID: ch1.Id, - Revision: 1, - Version: model.AccessControlPolicyVersionV0_2, - } - - appErrInherit1 := childPolicy1.Inherit(parentPolicy) - require.Nil(t, appErrInherit1) - childPolicy1, err = th.App.Srv().Store().AccessControlPolicy().Save(th.Context, childPolicy1) - require.NoError(t, err) - require.NotNil(t, childPolicy1) + // Clear any lingering AccessControl mock before per-channel cleanups run, + // so PermanentDeleteChannel's cleanupChannelAccessControlPolicy uses the + // store fallback (or no-ops) during teardown and doesn't call into a + // subtest mock whose Once() expectations may already be exhausted. + // Registered last at the parent level so it runs first (t.Cleanup is LIFO). t.Cleanup(func() { - sErr := th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, childPolicy1.ID) - require.NoError(t, sErr) + th.App.Srv().ch.AccessControl = nil }) - childPolicy2 := &model.AccessControlPolicy{ - Type: model.AccessControlPolicyTypeChannel, - ID: ch2.Id, - Revision: 1, - Version: model.AccessControlPolicyVersionV0_2, + // saveChildPolicy provisions a fresh child policy for the given channel, + // linked to parentPolicy, and registers a t.Cleanup that removes the row + // at the end of the calling subtest. Save is idempotent (it moves any + // existing row to history and inserts a new revision), so repeated calls + // across subtests are safe even when a previous subtest deleted the row. + saveChildPolicy := func(t *testing.T, channelID string) *model.AccessControlPolicy { + t.Helper() + child := &model.AccessControlPolicy{ + Type: model.AccessControlPolicyTypeChannel, + ID: channelID, + Revision: 1, + Version: model.AccessControlPolicyVersionV0_2, + } + require.Nil(t, child.Inherit(parentPolicy)) + saved, sErr := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, child) + require.NoError(t, sErr) + require.NotNil(t, saved) + t.Cleanup(func() { + // Idempotent: store Delete is a no-op when no row exists, which + // is exactly the case when the subtest's UnassignPoliciesFromChannels + // successfully removed it. + _ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, saved.ID) + }) + return saved } - appErrInherit2 := childPolicy2.Inherit(parentPolicy) - require.Nil(t, appErrInherit2) - childPolicy2, err = th.App.Srv().Store().AccessControlPolicy().Save(th.Context, childPolicy2) - require.NoError(t, err) - require.NotNil(t, childPolicy2) - t.Cleanup(func() { - sErr := th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, childPolicy2.ID) - require.NoError(t, sErr) - }) + // bindStoreDelete wires the mock's DeletePolicy to delegate to the real + // store. This way successful mock invocations actually drop the underlying + // row and the subtest can verify deletion at the store level — not just + // at the mock-assertion level. + bindStoreDelete := func(m *mocks.AccessControlServiceInterface) { + m.On("DeletePolicy", mock.AnythingOfType("*request.Context"), mock.AnythingOfType("string")). + Return(func(rctx request.CTX, id string) *model.AppError { + if err := th.App.Srv().Store().AccessControlPolicy().Delete(rctx, id); err != nil { + return model.NewAppError("DeletePolicy", "test.delete", nil, err.Error(), http.StatusInternalServerError) + } + return nil + }).Maybe() + } t.Run("Feature not enabled", func(t *testing.T) { + childPolicy1 := saveChildPolicy(t, ch1.Id) + childPolicy2 := saveChildPolicy(t, ch2.Id) + th.App.Srv().ch.AccessControl = nil + appErr := th.App.UnassignPoliciesFromChannels(th.Context, parentPolicy.ID, []string{ch1.Id, ch2.Id}) require.NotNil(t, appErr) assert.Equal(t, "app.pap.unassign_access_control_policy_from_channels.app_error", appErr.Id) + + // No mock available — skip mock assertions. Always verify store state: + // the function bailed before touching anything, so both rows must remain. + _, sErr := th.App.Srv().Store().AccessControlPolicy().Get(th.Context, childPolicy1.ID) + require.NoError(t, sErr, "child policy for ch1 should remain in store when feature is disabled") + _, sErr = th.App.Srv().Store().AccessControlPolicy().Get(th.Context, childPolicy2.ID) + require.NoError(t, sErr, "child policy for ch2 should remain in store when feature is disabled") }) t.Run("Error deleting policy from AccessControlService", func(t *testing.T) { + childPolicy1 := saveChildPolicy(t, ch1.Id) + childPolicy2 := saveChildPolicy(t, ch2.Id) + mockAccessControl := &mocks.AccessControlServiceInterface{} th.App.Srv().ch.AccessControl = mockAccessControl + t.Cleanup(func() { th.App.Srv().ch.AccessControl = nil }) - mockAccessControl.On("SearchPolicies", th.Context, model.AccessControlPolicySearch{ - Type: model.AccessControlPolicyTypeChannel, - ParentID: parentPolicy.ID, - Limit: 1000, - }).Return([]*model.AccessControlPolicy{childPolicy1}, mock.Anything, nil).Once() mockAccessControl.On("GetPolicy", th.Context, ch1.Id).Return(childPolicy1, nil).Once() expectedErr := model.NewAppError("DeletePolicy", "app.pap.unassign_access_control_policy_from_channels.app_error", nil, "failed to delete from acs", http.StatusInternalServerError) @@ -749,37 +996,81 @@ func TestUnassignPoliciesFromChannels(t *testing.T) { assert.Equal(t, expectedErr.Id, appErr.Id) assert.Equal(t, expectedErr.Message, appErr.Message) + // Mock assertions: service IS available so we can assert which methods + // were dispatched. The function bails on the first DeletePolicy error, + // so ch2 must NOT have been processed. mockAccessControl.AssertCalled(t, "DeletePolicy", th.Context, ch1.Id) mockAccessControl.AssertNotCalled(t, "DeletePolicy", th.Context, ch2.Id) + + // Always verify store state regardless of the mock outcome: the + // mock returned an error so the row for ch1 must still exist, and + // ch2 was never reached. + _, sErr := th.App.Srv().Store().AccessControlPolicy().Get(th.Context, childPolicy1.ID) + require.NoError(t, sErr, "child policy for ch1 should remain when DeletePolicy fails") + _, sErr = th.App.Srv().Store().AccessControlPolicy().Get(th.Context, childPolicy2.ID) + require.NoError(t, sErr, "child policy for ch2 should remain when iteration short-circuits") }) t.Run("Channel not actually a child policy", func(t *testing.T) { + childPolicy1 := saveChildPolicy(t, ch1.Id) + childPolicy2 := saveChildPolicy(t, ch2.Id) + ch3 := th.CreatePrivateChannel(t, th.BasicTeam) // Not a child of parentPolicy t.Cleanup(func() { _ = th.App.PermanentDeleteChannel(th.Context, ch3) }) mockAccessControl := &mocks.AccessControlServiceInterface{} th.App.Srv().ch.AccessControl = mockAccessControl + // Clear the mock before ch3 cleanup runs (LIFO: registered after the + // channel cleanup so it runs first), so cleanupChannelAccessControlPolicy + // during teardown takes the store fallback path. + t.Cleanup(func() { th.App.Srv().ch.AccessControl = nil }) mockAccessControl.On("GetPolicy", th.Context, ch1.Id).Return(childPolicy1, nil).Once() mockAccessControl.On("GetPolicy", th.Context, ch2.Id).Return(childPolicy2, nil).Once() - mockAccessControl.On("DeletePolicy", th.Context, ch1.Id).Return(nil).Once() - mockAccessControl.On("DeletePolicy", th.Context, ch2.Id).Return(nil).Once() + bindStoreDelete(mockAccessControl) appErr := th.App.UnassignPoliciesFromChannels(th.Context, parentPolicy.ID, []string{ch1.Id, ch2.Id, ch3.Id}) require.Nil(t, appErr) + + // Mock assertions: ch1 and ch2 are parent's children → DeletePolicy invoked; + // ch3 is not → must be skipped without ever calling DeletePolicy. + mockAccessControl.AssertCalled(t, "DeletePolicy", th.Context, ch1.Id) + mockAccessControl.AssertCalled(t, "DeletePolicy", th.Context, ch2.Id) + mockAccessControl.AssertNotCalled(t, "DeletePolicy", th.Context, ch3.Id) + + // Always verify store state — the mocked DeletePolicy delegates to the + // real store, so the rows for ch1 and ch2 must be gone. + _, sErr := th.App.Srv().Store().AccessControlPolicy().Get(th.Context, childPolicy1.ID) + require.Error(t, sErr, "child policy for ch1 should be removed from store") + _, sErr = th.App.Srv().Store().AccessControlPolicy().Get(th.Context, childPolicy2.ID) + require.Error(t, sErr, "child policy for ch2 should be removed from store") }) t.Run("Successful unassignment", func(t *testing.T) { + childPolicy1 := saveChildPolicy(t, ch1.Id) + childPolicy2 := saveChildPolicy(t, ch2.Id) + mockAccessControl := &mocks.AccessControlServiceInterface{} th.App.Srv().ch.AccessControl = mockAccessControl + t.Cleanup(func() { th.App.Srv().ch.AccessControl = nil }) - mockAccessControl.On("DeletePolicy", th.Context, ch1.Id).Return(nil).Once() - mockAccessControl.On("DeletePolicy", th.Context, ch2.Id).Return(nil).Once() mockAccessControl.On("GetPolicy", th.Context, ch1.Id).Return(childPolicy1, nil).Once() mockAccessControl.On("GetPolicy", th.Context, ch2.Id).Return(childPolicy2, nil).Once() + bindStoreDelete(mockAccessControl) appErr := th.App.UnassignPoliciesFromChannels(th.Context, parentPolicy.ID, []string{ch1.Id, ch2.Id}) require.Nil(t, appErr) + + // Mock assertions: service available, both targets must have been + // dispatched through DeletePolicy. + mockAccessControl.AssertCalled(t, "DeletePolicy", th.Context, ch1.Id) + mockAccessControl.AssertCalled(t, "DeletePolicy", th.Context, ch2.Id) + + // Always verify store-level deletion regardless of mock state. + _, sErr := th.App.Srv().Store().AccessControlPolicy().Get(th.Context, childPolicy1.ID) + require.Error(t, sErr, "child policy for ch1 should be removed from store") + _, sErr = th.App.Srv().Store().AccessControlPolicy().Get(th.Context, childPolicy2.ID) + require.Error(t, sErr, "child policy for ch2 should be removed from store") }) t.Run("Invalidate channel cache", func(t *testing.T) { @@ -883,7 +1174,7 @@ func TestValidateChannelAccessControlPermission(t *testing.T) { assert.Equal(t, "app.channel.get.existing.app_error", appErr.Id) }) - t.Run("Public channel should fail", func(t *testing.T) { + t.Run("Public channel should succeed", func(t *testing.T) { th.AddUserToChannel(t, channelAdmin, publicChannel) // Make user channel admin for public channel @@ -891,8 +1182,7 @@ func TestValidateChannelAccessControlPermission(t *testing.T) { require.Nil(t, appErr2) appErr2 = th.App.ValidateChannelAccessControlPermission(th.Context, channelAdmin.Id, publicChannel.Id) - require.NotNil(t, appErr2) - assert.Equal(t, "app.pap.access_control.channel_not_private", appErr2.Id) + require.Nil(t, appErr2) }) t.Run("Shared channel should fail", func(t *testing.T) { @@ -917,6 +1207,20 @@ func TestValidateChannelAccessControlPermission(t *testing.T) { require.NotNil(t, appErr3) assert.Equal(t, "app.pap.access_control.channel_shared", appErr3.Id) }) + + t.Run("Default channel should fail", func(t *testing.T) { + townSquare, appErr := th.App.GetChannelByName(th.Context, model.DefaultChannelName, th.BasicTeam.Id, false) + require.Nil(t, appErr) + + th.AddUserToChannel(t, channelAdmin, townSquare) + + _, appErr = th.App.UpdateChannelMemberRoles(th.Context, townSquare.Id, channelAdmin.Id, "channel_user channel_admin") + require.Nil(t, appErr) + + appErr = th.App.ValidateChannelAccessControlPermission(th.Context, channelAdmin.Id, townSquare.Id) + require.NotNil(t, appErr) + assert.Equal(t, "app.pap.access_control.channel_default", appErr.Id) + }) } func TestValidateAccessControlPolicyPermission(t *testing.T) { @@ -978,6 +1282,11 @@ func TestValidateAccessControlPolicyPermission(t *testing.T) { // Set up mock Access Control service mockAccessControl := &mocks.AccessControlServiceInterface{} th.App.Srv().ch.AccessControl = mockAccessControl + // Clear the mock before per-channel cleanups run (LIFO: registered after + // channel/policy cleanups so it runs first), so PermanentDeleteChannel's + // cleanupChannelAccessControlPolicy is a no-op during teardown. + t.Cleanup(func() { th.App.Srv().ch.AccessControl = nil }) + mockAccessControl.On("GetPolicy", th.Context, channelPolicy.ID).Return(channelPolicy, nil) mockAccessControl.On("GetPolicy", th.Context, parentPolicy.ID).Return(parentPolicy, nil) mockAccessControl.On("GetPolicy", th.Context, mock.AnythingOfType("string")).Return(nil, model.NewAppError("GetPolicy", "app.access_control_policy.get.app_error", nil, "not found", http.StatusNotFound)) @@ -1092,7 +1401,7 @@ func TestValidateChannelAccessControlPolicyCreation(t *testing.T) { assert.Equal(t, "app.access_control.insufficient_permissions", appErr.Id) }) - t.Run("Creating policy for public channel should fail", func(t *testing.T) { + t.Run("Creating policy for public channel should succeed", func(t *testing.T) { publicChannel := th.CreateChannel(t, th.BasicTeam) t.Cleanup(func() { appErr := th.App.PermanentDeleteChannel(th.Context, publicChannel) @@ -1116,8 +1425,7 @@ func TestValidateChannelAccessControlPolicyCreation(t *testing.T) { } appErr4 = th.App.ValidateChannelAccessControlPolicyCreation(th.Context, channelAdmin.Id, policy) - require.NotNil(t, appErr4) - assert.Equal(t, "app.pap.access_control.channel_not_private", appErr4.Id) + require.Nil(t, appErr4) }) t.Run("Creating policy for shared channel should fail", func(t *testing.T) { @@ -1152,6 +1460,30 @@ func TestValidateChannelAccessControlPolicyCreation(t *testing.T) { require.NotNil(t, appErr5) assert.Equal(t, "app.pap.access_control.channel_shared", appErr5.Id) }) + + t.Run("Creating policy for default channel should fail", func(t *testing.T) { + townSquare, appErr := th.App.GetChannelByName(th.Context, model.DefaultChannelName, th.BasicTeam.Id, false) + require.Nil(t, appErr) + + th.AddUserToChannel(t, channelAdmin, townSquare) + + _, appErr = th.App.UpdateChannelMemberRoles(th.Context, townSquare.Id, channelAdmin.Id, "channel_user channel_admin") + require.Nil(t, appErr) + + policy := &model.AccessControlPolicy{ + ID: townSquare.Id, + Type: model.AccessControlPolicyTypeChannel, + Version: model.AccessControlPolicyVersionV0_2, + Revision: 1, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{"membership"}, Expression: "true"}, + }, + } + + appErr = th.App.ValidateChannelAccessControlPolicyCreation(th.Context, channelAdmin.Id, policy) + require.NotNil(t, appErr) + assert.Equal(t, "app.pap.access_control.channel_default", appErr.Id) + }) } func TestTestExpressionWithChannelContext(t *testing.T) { @@ -1636,3 +1968,117 @@ func TestHasPermissionToFileAction(t *testing.T) { assert.True(t, result) }) } + +func TestGetRecommendedPublicChannelsForUser(t *testing.T) { + th := Setup(t).InitBasic(t) + + originalACS := th.App.Srv().ch.AccessControl + t.Cleanup(func() { th.App.Srv().ch.AccessControl = originalACS }) + + t.Run("returns empty when license is missing", func(t *testing.T) { + // No enterprise license set on the test server: the license short-circuit + // at the top of the function must keep the response empty without ever + // calling the access control service. + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.AccessControlSettings.EnableAttributeBasedAccessControl = model.NewPointer(true) + }) + + channels, appErr := th.App.GetRecommendedPublicChannelsForUser(th.Context, th.BasicUser.Id, th.BasicTeam.Id) + require.Nil(t, appErr) + assert.Empty(t, channels) + mockACS.AssertNotCalled(t, "AccessEvaluation", mock.Anything, mock.Anything) + }) + + t.Run("returns empty when access control service is nil", func(t *testing.T) { + ok := th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterpriseAdvanced)) + require.True(t, ok, "SetLicense should return true") + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.AccessControlSettings.EnableAttributeBasedAccessControl = model.NewPointer(true) + }) + + th.App.Srv().ch.AccessControl = nil + + channels, appErr := th.App.GetRecommendedPublicChannelsForUser(th.Context, th.BasicUser.Id, th.BasicTeam.Id) + require.Nil(t, appErr) + assert.Empty(t, channels) + }) + + t.Run("returns only channels the policy allows; tolerates per-channel eval errors", func(t *testing.T) { + ok := th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterpriseAdvanced)) + require.True(t, ok, "SetLicense should return true") + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.AccessControlSettings.EnableAttributeBasedAccessControl = model.NewPointer(true) + }) + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + + // PermanentDeleteChannel calls cleanupChannelAccessControlPolicy → DeletePolicy + // during the test cleanup phase. Allow it as a no-op so cleanups don't fail + // the test on unexpected mock calls. + mockACS.On("DeletePolicy", mock.Anything, mock.AnythingOfType("string")). + Return((*model.AppError)(nil)).Maybe() + + // Three policy-enforced public channels covering allow / deny / eval-error, + // plus one bare public channel without a policy. The bare channel must + // never reach the AccessEvaluation loop because SearchAllChannels filters + // it out via AccessControlPolicyEnforced=true. + allow := th.CreateChannel(t, th.BasicTeam) + deny := th.CreateChannel(t, th.BasicTeam) + evalErr := th.CreateChannel(t, th.BasicTeam) + bare := th.CreateChannel(t, th.BasicTeam) + t.Cleanup(func() { + for _, ch := range []*model.Channel{allow, deny, evalErr, bare} { + _ = th.App.PermanentDeleteChannel(th.Context, ch) + } + }) + + policyEnforced := func(channelID string) { + policy := &model.AccessControlPolicy{ + ID: channelID, + Type: model.AccessControlPolicyTypeChannel, + Revision: 1, + Version: model.AccessControlPolicyVersionV0_2, + Active: true, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{"membership"}, Expression: "true"}, + }, + } + _, err := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, policy) + require.NoError(t, err) + t.Cleanup(func() { + _ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, channelID) + }) + } + policyEnforced(allow.Id) + policyEnforced(deny.Id) + policyEnforced(evalErr.Id) + + mockACS.On("AccessEvaluation", mock.Anything, mock.MatchedBy(func(req model.AccessRequest) bool { + return req.Resource.ID == allow.Id && req.Action == "membership" + })).Return(model.AccessDecision{Decision: true}, (*model.AppError)(nil)) + mockACS.On("AccessEvaluation", mock.Anything, mock.MatchedBy(func(req model.AccessRequest) bool { + return req.Resource.ID == deny.Id + })).Return(model.AccessDecision{Decision: false}, (*model.AppError)(nil)) + // Per-channel evaluation errors must NOT abort the whole request — the + // channel is dropped from the recommendation list and the loop moves on. + mockACS.On("AccessEvaluation", mock.Anything, mock.MatchedBy(func(req model.AccessRequest) bool { + return req.Resource.ID == evalErr.Id + })).Return(model.AccessDecision{}, model.NewAppError("AccessEvaluation", "test.eval.error", nil, "boom", http.StatusInternalServerError)) + + channels, appErr := th.App.GetRecommendedPublicChannelsForUser(th.Context, th.BasicUser.Id, th.BasicTeam.Id) + require.Nil(t, appErr) + + ids := make([]string, 0, len(channels)) + for _, ch := range channels { + ids = append(ids, ch.Id) + } + assert.ElementsMatch(t, []string{allow.Id}, ids, + "only the channel whose policy allows the subject should be returned (deny/eval-error excluded)") + assert.NotContains(t, ids, bare.Id, "channel without a policy should never enter the candidate set") + + mockACS.AssertExpectations(t) + }) +} diff --git a/server/channels/app/channel.go b/server/channels/app/channel.go index 23a0e9994bf..8873311da8f 100644 --- a/server/channels/app/channel.go +++ b/server/channels/app/channel.go @@ -729,12 +729,30 @@ func (a *App) GetGroupChannel(rctx request.CTX, userIDs []string) (*model.Channe // UpdateChannel updates a given channel by its Id. It also publishes the CHANNEL_UPDATED event. func (a *App) UpdateChannel(rctx request.CTX, channel *model.Channel) (*model.Channel, *model.AppError) { - ok, appErr := a.ChannelAccessControlled(rctx, channel.Id) + enforced, appErr := a.ChannelAccessControlled(rctx, channel.Id) if appErr != nil { return nil, appErr } - if ok && channel.Type != model.ChannelTypePrivate { - return nil, model.NewAppError("UpdateChannel", "api.channel.update_channel.not_allowed.app_error", nil, "", http.StatusForbidden) + if enforced { + if channel.Type != model.ChannelTypePrivate && channel.Type != model.ChannelTypeOpen { + return nil, model.NewAppError("UpdateChannel", "api.channel.update_channel.not_allowed.app_error", nil, "", http.StatusForbidden) + } + + // Block public ↔ private conversion while an ABAC policy is attached. + // Public-channel and private-channel ABAC have asymmetric semantics + // (advisory recommend/auto-add vs hard-gate with member removal); a + // silent type flip would change what the existing policy actually + // does to members. The admin must remove the policy first and + // re-apply it after the conversion if they still want it. + current, getErr := a.Srv().Store().Channel().Get(channel.Id, true) + if getErr != nil { + return nil, model.NewAppError("UpdateChannel", "app.channel.get.find.app_error", nil, "", http.StatusInternalServerError).Wrap(getErr) + } + if current.Type != channel.Type { + return nil, model.NewAppError("UpdateChannel", + "api.channel.update_channel.policy_enforced_type_conversion.app_error", + nil, "channel has an active ABAC policy; remove the policy before converting between public and private", http.StatusBadRequest) + } } _, err := a.Srv().Store().Channel().Update(rctx, channel) @@ -1693,6 +1711,12 @@ func (a *App) DeleteChannel(rctx request.CTX, channel *model.Channel, userID str return model.NewAppError("DeleteChannel", "app.post_persistent_notification.delete_by_channel.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } + // Archiving a channel tears down the membership policy attached to it. + // If the channel is later restored the admin can re-apply policies; + // leaving the policy row behind would otherwise keep the sync job + // processing an archived channel. + a.cleanupChannelAccessControlPolicy(rctx, channel) + a.Srv().Platform().InvalidateCacheForChannel(channel) var message *model.WebSocketEvent @@ -3412,6 +3436,12 @@ func (a *App) PermanentDeleteChannel(rctx request.CTX, channel *model.Channel) * return model.NewAppError("PermanentDeleteChannel", "app.channel.permanent_delete.app_error", nil, "", http.StatusInternalServerError).Wrap(nErr) } + // Remove any orphaned channel-scope ABAC policy tied to this channel ID. + // cleanupChannelAccessControlPolicy intentionally does not gate on + // PolicyEnforced (see its doc comment) — the underlying DeletePolicy is a + // no-op when no row exists, so it's safe to call unconditionally. + a.cleanupChannelAccessControlPolicy(rctx, channel) + a.Srv().Platform().InvalidateCacheForChannel(channel) var message *model.WebSocketEvent @@ -4143,6 +4173,167 @@ func (a *App) ChannelAccessControlled(rctx request.CTX, channelID string) (bool, return channel.PolicyEnforced, nil } +// cleanupChannelAccessControlPolicy removes the channel-scope ABAC policy row, +// if any, for a channel being archived or permanently deleted. Orphan policy +// rows left behind would still be picked up by the sync job and surface in +// searches that filter by AccessControlPolicyEnforced. Failures are logged +// but not returned — deleting/archiving a channel must not be blocked by an +// ABAC cleanup error. +// +// We intentionally do NOT gate this on channel.PolicyEnforced: that flag is +// computed from the AccessControlPolicies table via the channel store and can +// be stale when read through the channel cache, which would cause us to skip +// cleanup and leave an orphaned policy behind. DeletePolicy is a no-op when +// no matching row exists, so calling it unconditionally is safe. +// +// When the enterprise access control service is unavailable (acs == nil) or +// reports the operation as unsupported (NotImplemented / NotAcceptable — +// e.g. running on Team Edition or under a license that gates the ABAC +// engine), we still need to remove the underlying row to avoid leaving an +// orphaned policy behind. In those cases we fall back to deleting directly +// through the access control policy store. +func (a *App) cleanupChannelAccessControlPolicy(rctx request.CTX, channel *model.Channel) { + if channel == nil || channel.Id == "" { + return + } + + useStoreFallback := false + acs := a.Srv().Channels().AccessControl + if acs == nil { + useStoreFallback = true + } else if appErr := acs.DeletePolicy(rctx, channel.Id); appErr != nil { + switch appErr.StatusCode { + case http.StatusNotImplemented, http.StatusNotAcceptable: + useStoreFallback = true + default: + rctx.Logger().Warn("Failed to delete channel ABAC policy during channel delete/archive", + mlog.String("channel_id", channel.Id), + mlog.Err(appErr), + ) + } + } + + if useStoreFallback { + if err := a.Srv().Store().AccessControlPolicy().Delete(rctx, channel.Id); err != nil { + rctx.Logger().Warn("Failed to delete channel ABAC policy during channel delete/archive", + mlog.String("channel_id", channel.Id), + mlog.Err(err), + ) + } + } +} + +// recommendedPublicChannelsScanPageSize is the per-page size used while +// paginating through policy-enforced public channels in +// GetRecommendedPublicChannelsForUser. +const recommendedPublicChannelsScanPageSize = 200 + +// recommendedPublicChannelsScanCap is a hard upper bound on the total number +// of candidate channels we will evaluate per request. Evaluation is O(n) and +// CEL programs are non-trivial; this guards against runaway scans on +// pathological deployments. In practice teams have far fewer policy-enforced +// public channels than this cap. +const recommendedPublicChannelsScanCap = 2000 + +// GetRecommendedPublicChannelsForUser returns public channels in the given team +// that have an ABAC policy assigned and whose membership rule the user +// satisfies. ABAC policies on public channels are advisory — this list is +// consumed by the "Recommended channels" feature in the Browse Channels UI. +// +// Channels the user is already a member of are intentionally NOT filtered out: +// membership filtering is a presentation concern handled by the caller (the +// Browse Channels UI has its own "Hide joined channels" preference and may +// want to show membership state alongside each recommendation). +// +// Returns an empty list when the Enterprise Advanced license or ABAC feature +// flag is not available, or when the access control service is not wired up. +func (a *App) GetRecommendedPublicChannelsForUser(rctx request.CTX, userID, teamID string) (model.ChannelList, *model.AppError) { + if l := a.License(); !model.MinimumEnterpriseAdvancedLicense(l) || !*a.Config().AccessControlSettings.EnableAttributeBasedAccessControl { + return model.ChannelList{}, nil + } + + acs := a.Srv().Channels().AccessControl + if acs == nil { + return model.ChannelList{}, nil + } + + user, appErr := a.GetUser(userID) + if appErr != nil { + return nil, appErr + } + + subject, appErr := a.BuildAccessControlSubject(rctx, user.Id, user.Roles) + if appErr != nil { + return nil, appErr + } + + // Page through policy-enforced public channels until the team is fully + // scanned or we hit the hard cap. SearchAllChannels signals "no more + // pages" by returning fewer rows than the requested page size. + candidates := make(model.ChannelListWithTeamData, 0) + for page := 0; len(candidates) < recommendedPublicChannelsScanCap; page++ { + batch, _, searchErr := a.SearchAllChannels(rctx, "", model.ChannelSearchOpts{ + TeamIds: []string{teamID}, + Public: true, + AccessControlPolicyEnforced: true, + Page: model.NewPointer(page), + PerPage: model.NewPointer(recommendedPublicChannelsScanPageSize), + }) + if searchErr != nil { + return nil, searchErr + } + if len(batch) == 0 { + break + } + + // Truncate to the remaining cap so the final candidate count never + // exceeds the documented bound. The loop condition only gates entry + // to a new iteration; without this, a final page worth of channels + // could push len past the cap by up to (pageSize - 1). + remaining := recommendedPublicChannelsScanCap - len(candidates) + if len(batch) > remaining { + candidates = append(candidates, batch[:remaining]...) + break + } + + candidates = append(candidates, batch...) + if len(batch) < recommendedPublicChannelsScanPageSize { + break + } + } + + // We intentionally do NOT filter out channels the user is already a member + // of here — the Browse Channels UI has its own "Hide joined channels" + // preference and callers may want to show membership state next to each + // recommendation. Membership filtering is a presentation concern. + recommended := make(model.ChannelList, 0, len(candidates)) + for _, channel := range candidates { + decision, evalErr := acs.AccessEvaluation(rctx, model.AccessRequest{ + Subject: *subject, + Resource: model.Resource{ + Type: model.AccessControlPolicyTypeChannel, + ID: channel.Id, + }, + Action: "membership", + }) + if evalErr != nil { + rctx.Logger().Debug("ABAC evaluation failed when computing recommended channels", + mlog.String("user_id", userID), + mlog.String("channel_id", channel.Id), + mlog.Err(evalErr), + ) + continue + } + if !decision.Decision { + continue + } + + recommended = append(recommended, &channel.Channel) + } + + return recommended, nil +} + func (a *App) handleChannelCategoryName(channel *model.Channel) { if *a.Config().ExperimentalSettings.ExperimentalChannelCategorySorting && strings.Contains(channel.DisplayName, "/") { parts := strings.Split(channel.DisplayName, "/") diff --git a/server/channels/app/team_access_control.go b/server/channels/app/team_access_control.go index c50104f393f..e1dd98c9346 100644 --- a/server/channels/app/team_access_control.go +++ b/server/channels/app/team_access_control.go @@ -69,10 +69,57 @@ func (a *App) SearchTeamAccessPolicies(rctx request.CTX, teamID, requesterID str return policies[i].ID < policies[j].ID }) - // Filter by self-inclusion. + // Single batched Channel lookup for all policies' child_ids so we don't + // issue GetChannelsByIds once per policy (N+1). + unionSet := make(map[string]struct{}) + for _, policy := range policies { + ids, ok := childIDsFromPolicyProps(policy) + if !ok { + continue + } + for _, id := range ids { + unionSet[id] = struct{}{} + } + } + union := make([]string, 0, len(unionSet)) + for id := range unionSet { + union = append(union, id) + } + sort.Strings(union) + + var idToType map[string]model.ChannelType + batchLookupFailed := false + if len(union) > 0 { + channels, err := a.Srv().Store().Channel().GetChannelsByIds(union, true) + if err != nil { + rctx.Logger().Warn("Failed to look up child channels for self-inclusion gating batch; keeping the filter on", + mlog.Err(err), + mlog.Int("policy_count", len(policies)), + ) + batchLookupFailed = true + } else { + if len(channels) != len(union) { + rctx.Logger().Warn("Partial batch child-channel lookup for self-inclusion gating", + mlog.Int("requested", len(union)), + mlog.Int("returned", len(channels)), + ) + } + idToType = make(map[string]model.ChannelType, len(channels)) + for _, ch := range channels { + idToType[ch.Id] = ch.Type + } + } + } else { + idToType = map[string]model.ChannelType{} + } + + // Filter by self-inclusion. Skip the filter for parent policies whose + // children are all public channels — public-channel ABAC is advisory and + // can never lock the requesting admin out, so a non-matching expression + // is not a reason to hide the policy from them. filtered := make([]*model.AccessControlPolicy, 0, len(policies)) for _, policy := range policies { - if len(policy.Rules) > 0 { + if len(policy.Rules) > 0 && policyAppliesToPrivateChannel(rctx, policy, idToType, batchLookupFailed) { expression := policy.Rules[0].Expression matches, matchErr := a.ValidateExpressionAgainstRequester(rctx, expression, requesterID) if matchErr != nil { @@ -241,9 +288,10 @@ func (a *App) ReconcilePolicyTeamScope(rctx request.CTX, policyID string) *model // - At least one channel provided // - All channels exist // - All channels belong to the given team -// - All channels are private +// - All channels are public or private (DM/GM excluded) // - No group-constrained channels // - No shared channels +// - No team default channels (e.g. town-square) func (a *App) ValidateTeamScopePolicyChannelAssignment(rctx request.CTX, teamID string, channelIDs []string) *model.AppError { if len(channelIDs) == 0 { return model.NewAppError("ValidateTeamScopePolicyChannelAssignment", @@ -270,7 +318,7 @@ func (a *App) ValidateTeamScopePolicyChannelAssignment(rctx request.CTX, teamID "channel does not belong to this team", http.StatusBadRequest) } - if appErr := ValidateChannelEligibilityForAccessControl(channel); appErr != nil { + if appErr := a.ValidateChannelEligibilityForAccessControl(rctx, channel); appErr != nil { return appErr } } @@ -300,3 +348,82 @@ func (a *App) ValidateTeamAdminSelfInclusion(rctx request.CTX, userID, expressio return nil } + +// childIDsFromPolicyProps reads Props["child_ids"], which is normally []string +// (set by the policy store) but may be []any after JSON round-trip. +// If ok is false, the shape is unsupported and callers should treat it as unknown. +func childIDsFromPolicyProps(policy *model.AccessControlPolicy) (ids []string, ok bool) { + if policy == nil || policy.Props == nil { + return nil, false + } + switch raw := policy.Props["child_ids"].(type) { + case []string: + return raw, true + case []any: + out := make([]string, 0, len(raw)) + for _, item := range raw { + if id, itemOk := item.(string); itemOk { + out = append(out, id) + } + } + return out, true + default: + return nil, false + } +} + +// policyAppliesToPrivateChannel reports whether a parent policy has at least +// one private child channel — used to gate the self-inclusion filter, which +// only matters when a non-matching admin could actually be locked out. Public +// channels under ABAC are advisory (no member removal, anyone can join), so +// a non-matching admin is never at risk there. +// +// idToType maps channel ID → channel type from a batched GetChannelsByIds; +// batchLookupFailed means the batch store call failed and the conservative +// answer is to apply the filter. +// +// Returns `true` (i.e. apply the filter) on metadata or lookup errors, since +// the safer fallback is to keep the existing behavior rather than silently +// expose a policy a private channel might depend on. +func policyAppliesToPrivateChannel(rctx request.CTX, policy *model.AccessControlPolicy, idToType map[string]model.ChannelType, batchLookupFailed bool) bool { + if policy == nil || policy.Props == nil { + return true + } + + childIDs, propsOk := childIDsFromPolicyProps(policy) + if !propsOk { + return true + } + + if len(childIDs) == 0 { + // Channel-less policy (newly created, not yet assigned). Until a + // channel is attached there's nothing to lock anyone out of, so the + // filter is meaningless here. + return false + } + + if batchLookupFailed { + return true + } + + found := 0 + for _, id := range childIDs { + if _, ok := idToType[id]; ok { + found++ + } + } + if found != len(childIDs) { + rctx.Logger().Warn("Partial child-channel lookup for self-inclusion gating; keeping the filter on", + mlog.String("policy_id", policy.ID), + mlog.Int("requested", len(childIDs)), + mlog.Int("returned", found), + ) + return true + } + for _, id := range childIDs { + if idToType[id] == model.ChannelTypePrivate { + return true + } + } + return false +} diff --git a/server/channels/app/team_access_control_test.go b/server/channels/app/team_access_control_test.go index 2a067028e3a..0565f0c793e 100644 --- a/server/channels/app/team_access_control_test.go +++ b/server/channels/app/team_access_control_test.go @@ -83,10 +83,9 @@ func TestValidateTeamScopePolicyChannelAssignment(t *testing.T) { require.NotNil(t, appErr) }) - t.Run("public channel returns error", func(t *testing.T) { + t.Run("public channel is eligible", func(t *testing.T) { appErr := th.App.ValidateTeamScopePolicyChannelAssignment(th.Context, th.BasicTeam.Id, []string{th.BasicChannel.Id}) - require.NotNil(t, appErr) - assert.Equal(t, "app.pap.access_control.channel_not_private", appErr.Id) + require.Nil(t, appErr) }) t.Run("channel from wrong team returns error", func(t *testing.T) { @@ -120,6 +119,15 @@ func TestValidateTeamScopePolicyChannelAssignment(t *testing.T) { assert.Equal(t, "app.pap.access_control.channel_group_constrained", appErr.Id) }) + t.Run("default channel returns error", func(t *testing.T) { + townSquare, appErr := th.App.GetChannelByName(th.Context, model.DefaultChannelName, th.BasicTeam.Id, false) + require.Nil(t, appErr) + + appErr = th.App.ValidateTeamScopePolicyChannelAssignment(th.Context, th.BasicTeam.Id, []string{townSquare.Id}) + require.NotNil(t, appErr) + assert.Equal(t, "app.pap.access_control.channel_default", appErr.Id) + }) + t.Run("valid private channel in team succeeds", func(t *testing.T) { channel := th.CreatePrivateChannel(t, th.BasicTeam) @@ -138,9 +146,12 @@ func TestValidateTeamScopePolicyChannelAssignment(t *testing.T) { t.Run("mix of valid and invalid channels returns error", func(t *testing.T) { validChannel := th.CreatePrivateChannel(t, th.BasicTeam) - appErr := th.App.ValidateTeamScopePolicyChannelAssignment(th.Context, th.BasicTeam.Id, []string{ + townSquare, appErr := th.App.GetChannelByName(th.Context, model.DefaultChannelName, th.BasicTeam.Id, false) + require.Nil(t, appErr) + + appErr = th.App.ValidateTeamScopePolicyChannelAssignment(th.Context, th.BasicTeam.Id, []string{ validChannel.Id, - th.BasicChannel.Id, // public — invalid + townSquare.Id, // default channel — invalid }) require.NotNil(t, appErr) }) diff --git a/server/i18n/en.json b/server/i18n/en.json index a08abfa3511..06664c83236 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -615,6 +615,10 @@ "id": "api.channel.update_channel.not_allowed.app_error", "translation": "Policy enforced channels cannot be updated." }, + { + "id": "api.channel.update_channel.policy_enforced_type_conversion.app_error", + "translation": "This channel has an attribute-based membership policy applied. Remove the policy before converting between public and private." + }, { "id": "api.channel.update_channel.tried.app_error", "translation": "Tried to perform an invalid update of the default channel {{.Channel}}." @@ -7298,18 +7302,22 @@ "id": "app.oauth.update_app.updating.app_error", "translation": "We encountered an error updating the app." }, + { + "id": "app.pap.access_control.channel_default", + "translation": "Membership policies cannot be applied to team default channels." + }, { "id": "app.pap.access_control.channel_group_constrained", "translation": "Channel is group constrained and cannot have access control policies applied." }, - { - "id": "app.pap.access_control.channel_not_private", - "translation": "Access control policies can only be applied to private channels." - }, { "id": "app.pap.access_control.channel_shared", "translation": "Shared channels cannot have access control policy applied." }, + { + "id": "app.pap.access_control.channel_type_not_supported", + "translation": "Access control policies can only be applied to public or private channels." + }, { "id": "app.pap.access_control.insufficient_channel_permissions", "translation": "You do not have permission to manage access control for this channel." diff --git a/webapp/channels/src/components/admin_console/access_control/__snapshots__/policies.test.tsx.snap b/webapp/channels/src/components/admin_console/access_control/__snapshots__/policies.test.tsx.snap index 3644e8b614a..2852450a854 100644 --- a/webapp/channels/src/components/admin_console/access_control/__snapshots__/policies.test.tsx.snap +++ b/webapp/channels/src/components/admin_console/access_control/__snapshots__/policies.test.tsx.snap @@ -12,10 +12,10 @@ exports[`components/admin_console/access_control/PolicyList should match snapsho class="policy-header-text" >

- Access Control Policies + Membership Policies

- Create policies containing attribute based access rules and the resources they apply to. + Create policies containing attribute-based membership rules and the channels they apply to.

} onRowClick={handleRowClick} diff --git a/webapp/channels/src/components/admin_console/access_control/modals/confirmation/confirmation_modal.tsx b/webapp/channels/src/components/admin_console/access_control/modals/confirmation/confirmation_modal.tsx index 730f2b275cc..0bb619cd7e8 100644 --- a/webapp/channels/src/components/admin_console/access_control/modals/confirmation/confirmation_modal.tsx +++ b/webapp/channels/src/components/admin_console/access_control/modals/confirmation/confirmation_modal.tsx @@ -13,12 +13,44 @@ type Props = { onExited: () => void; onConfirm: (apply: boolean) => void; channelsAffected: number; + publicChannelsAffected?: number; + privateChannelsAffected?: number; } -export default function PolicyConfirmationModal({active, onExited, onConfirm, channelsAffected}: Props) { +export default function PolicyConfirmationModal({active, onExited, onConfirm, channelsAffected, publicChannelsAffected = 0, privateChannelsAffected = 0}: Props) { const {formatMessage} = useIntl(); const [enforceImmediately, setEnforceImmediately] = useState(true); + const hasMix = publicChannelsAffected > 0 && privateChannelsAffected > 0; + const hasOnlyPublic = publicChannelsAffected > 0 && privateChannelsAffected === 0; + + let bodyText: string; + if (hasMix) { + bodyText = active ? formatMessage({ + id: 'admin.access_control.policy.save_policy_confirmation_body.mixed', + defaultMessage: 'This policy is applied to channels of mixed types. For private channels, matching users will be granted access and non-matching members will be removed. For public channels, matching users will see these channels as recommendations and will be auto-added when auto-add is enabled; no existing members will be removed.', + }) : formatMessage({ + id: 'admin.access_control.policy.save_policy_confirmation_body.mixed_inactive', + defaultMessage: 'This policy is applied to channels of mixed types. For private channels, only matching users can be added and non-matching existing members will be removed. For public channels, the policy acts as a recommendation only; no existing members will be removed.', + }); + } else if (hasOnlyPublic) { + bodyText = active ? formatMessage({ + id: 'admin.access_control.policy.save_policy_confirmation_body.public', + defaultMessage: 'Matching users will see these public channels as recommendations and, when auto-add is enabled, will be added automatically. Anyone can still join these channels; no existing members will be removed.', + }) : formatMessage({ + id: 'admin.access_control.policy.save_policy_confirmation_body.public_inactive', + defaultMessage: 'Matching users will see these public channels as recommendations only; no existing members will be removed. Turn on Active (auto-add) to add matching users automatically.', + }); + } else { + bodyText = active ? formatMessage({ + id: 'admin.access_control.policy.save_policy_confirmation_body', + defaultMessage: 'Applying this policy will allow users with the appropriate attribute values to be added to the selected channels. Existing channel members will be removed from these channels if they are not assigned the values defined in this membership policy.', + }) : formatMessage({ + id: 'admin.access_control.policy.save_policy_confirmation_body.inactive', + defaultMessage: 'Only users who match the attribute values configured below can be added to the selected channels. Existing channel members will be removed from these channels if they are not assigned the values defined in this membership policy.', + }); + } + return ( } modalSubheaderText={ @@ -63,17 +95,7 @@ export default function PolicyConfirmationModal({active, onExited, onConfirm, ch >
- {active ? ( - formatMessage({ - id: 'admin.access_control.policy.save_policy_confirmation_body', - defaultMessage: 'Applying this policy will allow users with the appropriate attribute values to be added to the selected channels. Existing channel members will be removed from these channels if they are not assigned the values defined in this access policy.', - }) - ) : ( - formatMessage({ - id: 'admin.access_control.policy.save_policy_confirmation_body.inactive', - defaultMessage: 'Only users who match the attribute values configured below can be added to the selected channels. Existing channel members will be removed from these channels if they are not assigned the values defined in this access policy.', - }) - )} + {bodyText}
@@ -94,11 +116,11 @@ export default function PolicyConfirmationModal({active, onExited, onConfirm, ch {enforceImmediately ? formatMessage({ id: 'admin.access_control.policy.channels_affected', - defaultMessage: 'Are you sure you want to save and apply the access control policy?', + defaultMessage: 'Are you sure you want to save and apply the membership policy?', }) : formatMessage({ id: 'admin.access_control.policy.save_only', - defaultMessage: 'Are you sure you want to save this access control policy?', + defaultMessage: 'Are you sure you want to save this membership policy?', }) }
diff --git a/webapp/channels/src/components/admin_console/access_control/modals/job_details/job_details_modal.scss b/webapp/channels/src/components/admin_console/access_control/modals/job_details/job_details_modal.scss index 768e9de8e4d..7cb569089ee 100644 --- a/webapp/channels/src/components/admin_console/access_control/modals/job_details/job_details_modal.scss +++ b/webapp/channels/src/components/admin_console/access_control/modals/job_details/job_details_modal.scss @@ -92,6 +92,7 @@ .error-status-content { display: flex; + min-width: 0; // allow the code block to respect the modal width height: 100%; flex-direction: column; padding: 0px 32px 32px; @@ -99,6 +100,23 @@ &__title { color: var(--error-text); } + + // Sync-job errors often contain long identifiers and paths inside a + // single JSON string. Without wrapping, one line can stretch the + // modal far beyond the viewport. Scope the wrap behavior to this + // specific code block so we don't alter the global post code styles + // used in message posts. + .post-code { + max-width: 100%; + overflow-x: auto; + + code, + .hljs { + overflow-wrap: anywhere; + white-space: pre-wrap; + word-break: break-word; + } + } } .canceled-status-content { diff --git a/webapp/channels/src/components/admin_console/access_control/modals/policy_selection/policy_selection_modal.tsx b/webapp/channels/src/components/admin_console/access_control/modals/policy_selection/policy_selection_modal.tsx index abec289c36b..a7ca8a62bd2 100644 --- a/webapp/channels/src/components/admin_console/access_control/modals/policy_selection/policy_selection_modal.tsx +++ b/webapp/channels/src/components/admin_console/access_control/modals/policy_selection/policy_selection_modal.tsx @@ -33,13 +33,13 @@ export default function PolicySelectionModal(props: Props): JSX.Element { modalHeaderText={( )} modalSubheaderText={( )} > diff --git a/webapp/channels/src/components/admin_console/access_control/policies.tsx b/webapp/channels/src/components/admin_console/access_control/policies.tsx index cd9bd60ad93..90a35bee749 100644 --- a/webapp/channels/src/components/admin_console/access_control/policies.tsx +++ b/webapp/channels/src/components/admin_console/access_control/policies.tsx @@ -353,13 +353,13 @@ export default function PolicyList(props: Props): JSX.Element {

diff --git a/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap b/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap index 20ec0a58218..446b7a410d0 100644 --- a/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap +++ b/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap @@ -13,7 +13,7 @@ exports[`components/admin_console/access_control/policy_details/PolicyDetails sh class="fa fa-angle-left back" href="/admin_console/system_attributes/membership_policies" /> - Edit Access Control Policy + Edit Membership Policy
- Access control policy name: + Membership policy name:
- Attribute-based access rules + Attribute-based membership rules
- Select user attributes and values as rules to restrict channel membership. + Select user attributes and values as rules to determine who should be in the channels this policy applies to.