[MM-70153] Fix error when deleting the only remaining channel permission rule (#37899)

* [MM-70153] Fix error deleting the only remaining channel permission rule Deleting the last permission rule in a channel's Permissions Policy tab left the channel policy with no rules and no imports, which the server rejects with "Unable to save access control policy." Mirror the Membership Policy tab and delete the channel policy instead when the resulting policy would be empty, returning the channel to standard access. Co-authored-by: mattermost-code <matty-code@mattermost.com> * [MM-70153] Add tests for removing the last channel permission rule Covers deleting the empty channel policy on save, preserving a remaining membership rule, and surfacing a non-404 delete failure. Co-authored-by: mattermost-code <matty-code@mattermost.com> * [MM-70153] Cover 404-as-success and imports-present branches Add tests confirming a 404 delete is treated as success and that a policy with remaining imports is saved rather than deleted when the last permission rule is removed. Co-authored-by: mattermost-code <matty-code@mattermost.com> * [MM-70153] Reset originalActive after empty channel policy delete After deleting an emptied channel policy, clear the stale active flag so a subsequent save in the same tab session does not re-enable membership auto-sync. Mirrors the Membership Policy tab empty-delete path. Co-authored-by: mattermost-code <matty-code@mattermost.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: mattermost-code <matty-code@mattermost.com>
2026-09-01 15:00:08 +08:00 · 2026-08-10 17:31:58 +00:00
parent 7e7ec9a61c
commit 60c44fc48a
2 changed files with 250 additions and 0 deletions
@@ -356,6 +356,226 @@ describe('components/channel_settings_modal/ChannelSettingsPermissionsPolicyTab'
        expect(screen.queryByTestId(`permissions-policy-editor-action-${ACCESS_CONTROL_ACTION_DOWNLOAD_FILE}`)).not.toBeInTheDocument();
    });

+    test('deletes the channel policy when saving after removing the only permission rule', async () => {
+        // A channel whose policy holds a single permission rule and nothing
+        // else (no membership rule, no imports). Removing that rule would leave
+        // an empty policy, which the server rejects with
+        // "Unable to save access control policy." (MM-70153).
+        mockActions.getChannelPolicy.mockResolvedValue({
+            data: {
+                id: 'channel_id',
+                rules: [{
+                    name: 'Only rule',
+                    role: ACCESS_CONTROL_CHANNEL_ROLE_USER,
+                    actions: [ACCESS_CONTROL_ACTION_UPLOAD_FILE],
+                    expression: 'user.attributes.department == "eng"',
+                }],
+                imports: [],
+            },
+        });
+        mockActions.deleteChannelPolicy.mockResolvedValue({data: true});
+
+        renderWithContext(<ChannelSettingsPermissionsPolicyTab {...baseProps}/>, initialState);
+
+        // Delete the only rule from the list.
+        await userEvent.click(await screen.findByTestId(/^permissions-policy-row-delete-/));
+        expect(await screen.findByText('No permission rules yet. Click "Add rule" to create one.')).toBeInTheDocument();
+
+        // Save the now-empty rule set.
+        await userEvent.click(await screen.findByTestId('SaveChangesPanel__save-btn'));
+
+        // The empty policy is removed rather than saved, so no error appears.
+        await waitFor(() => {
+            expect(mockActions.deleteChannelPolicy).toHaveBeenCalledWith('channel_id');
+        });
+        expect(mockActions.saveChannelPolicy).not.toHaveBeenCalled();
+        expect(screen.queryByText('Unable to save access control policy.')).not.toBeInTheDocument();
+        expect(await screen.findByText('Settings saved')).toBeInTheDocument();
+    });
+
+    test('does not re-send a stale active flag when recreating a policy after empty delete', async () => {
+        // Regression: after deleting an active channel policy via the empty-
+        // rules path, adding a rule and saving again (without closing the tab)
+        // must create the new policy with active:false. Leaving originalActive
+        // as true would silently re-enable membership auto-sync.
+        mockActions.getChannelPolicy.mockResolvedValue({
+            data: {
+                id: 'channel_id',
+                active: true,
+                rules: [{
+                    name: 'Only rule',
+                    role: ACCESS_CONTROL_CHANNEL_ROLE_USER,
+                    actions: [ACCESS_CONTROL_ACTION_UPLOAD_FILE],
+                    expression: 'user.attributes.department == "eng"',
+                }],
+                imports: [],
+            },
+        });
+        mockActions.deleteChannelPolicy.mockResolvedValue({data: true});
+        mockActions.saveChannelPolicy.mockResolvedValue({
+            data: {
+                rules: [{
+                    name: 'Recreated rule',
+                    role: ACCESS_CONTROL_CHANNEL_ROLE_USER,
+                    actions: [ACCESS_CONTROL_ACTION_UPLOAD_FILE],
+                    expression: EXPRESSION,
+                }],
+            },
+        });
+
+        renderWithContext(<ChannelSettingsPermissionsPolicyTab {...baseProps}/>, initialState);
+
+        // Delete the only rule and persist (triggers channel-policy delete).
+        await userEvent.click(await screen.findByTestId(/^permissions-policy-row-delete-/));
+        await userEvent.click(await screen.findByTestId('SaveChangesPanel__save-btn'));
+        await waitFor(() => {
+            expect(mockActions.deleteChannelPolicy).toHaveBeenCalledWith('channel_id');
+        });
+        expect(await screen.findByText('Settings saved')).toBeInTheDocument();
+
+        // Without closing the tab, add a new rule and save again.
+        const addRuleButton = await screen.findByTestId('permissions-policy-add-rule');
+        await waitFor(() => expect(addRuleButton).toBeEnabled());
+        await userEvent.click(addRuleButton);
+        await screen.findByTestId('table-editor');
+        act(() => {
+            latestTableEditorProps().onChange(EXPRESSION);
+        });
+        await userEvent.click(screen.getByTestId(`cpp-add-permission-${ACCESS_CONTROL_ACTION_UPLOAD_FILE}`));
+        await userEvent.type(screen.getByTestId('permissions-policy-editor-name'), 'Recreated rule');
+        await userEvent.click(screen.getByTestId('permissions-policy-editor-save'));
+
+        expect(await screen.findByText('Recreated rule')).toBeInTheDocument();
+        await userEvent.click(await screen.findByTestId('SaveChangesPanel__save-btn'));
+
+        await waitFor(() => {
+            expect(mockActions.saveChannelPolicy).toHaveBeenCalledTimes(1);
+        });
+        expect(mockActions.saveChannelPolicy.mock.calls[0][0].active).toBe(false);
+    });
+
+    test('saves the remaining membership rule when the last permission rule is removed', async () => {
+        // When a membership rule remains, removing the last permission rule must
+        // still save the policy (keeping the membership rule) rather than delete
+        // the whole channel policy.
+        mockActions.getChannelPolicy.mockResolvedValue({
+            data: {
+                id: 'channel_id',
+                rules: [
+                    {actions: ['membership'], expression: 'user.attributes.team == "ops"'},
+                    {
+                        name: 'Only permission rule',
+                        role: ACCESS_CONTROL_CHANNEL_ROLE_USER,
+                        actions: [ACCESS_CONTROL_ACTION_UPLOAD_FILE],
+                        expression: 'user.attributes.department == "eng"',
+                    },
+                ],
+                imports: [],
+            },
+        });
+        mockActions.saveChannelPolicy.mockResolvedValue({data: {rules: [{actions: ['membership'], expression: 'user.attributes.team == "ops"'}]}});
+
+        renderWithContext(<ChannelSettingsPermissionsPolicyTab {...baseProps}/>, initialState);
+
+        // Remove the single permission rule (the membership rule is not listed here).
+        await userEvent.click(await screen.findByTestId(/^permissions-policy-row-delete-/));
+        expect(await screen.findByText('No permission rules yet. Click "Add rule" to create one.')).toBeInTheDocument();
+
+        await userEvent.click(await screen.findByTestId('SaveChangesPanel__save-btn'));
+
+        // The policy is saved with only the membership rule; it is not deleted.
+        await waitFor(() => {
+            expect(mockActions.saveChannelPolicy).toHaveBeenCalledTimes(1);
+        });
+        const savedPolicy = mockActions.saveChannelPolicy.mock.calls[0][0];
+        expect(savedPolicy.rules).toEqual([{actions: ['membership'], expression: 'user.attributes.team == "ops"'}]);
+        expect(mockActions.deleteChannelPolicy).not.toHaveBeenCalled();
+    });
+
+    test('treats a 404 from the delete as success when removing the only permission rule', async () => {
+        // The rule may have been added and removed before any policy was
+        // persisted, so a 404 on delete means "already gone" — an effective
+        // success, not an error.
+        mockActions.getChannelPolicy.mockResolvedValue({
+            data: {
+                id: 'channel_id',
+                rules: [{
+                    name: 'Only rule',
+                    role: ACCESS_CONTROL_CHANNEL_ROLE_USER,
+                    actions: [ACCESS_CONTROL_ACTION_UPLOAD_FILE],
+                    expression: 'user.attributes.department == "eng"',
+                }],
+                imports: [],
+            },
+        });
+        mockActions.deleteChannelPolicy.mockResolvedValue({error: {status_code: 404}});
+
+        renderWithContext(<ChannelSettingsPermissionsPolicyTab {...baseProps}/>, initialState);
+
+        await userEvent.click(await screen.findByTestId(/^permissions-policy-row-delete-/));
+        await userEvent.click(await screen.findByTestId('SaveChangesPanel__save-btn'));
+
+        expect(await screen.findByText('Settings saved')).toBeInTheDocument();
+        expect(mockActions.saveChannelPolicy).not.toHaveBeenCalled();
+    });
+
+    test('saves (not deletes) the policy when imports remain after removing the last permission rule', async () => {
+        // Imports keep the policy valid even with no rules, so the policy must be
+        // saved (preserving its imports) rather than deleted.
+        mockActions.getChannelPolicy.mockResolvedValue({
+            data: {
+                id: 'channel_id',
+                rules: [{
+                    name: 'Only rule',
+                    role: ACCESS_CONTROL_CHANNEL_ROLE_USER,
+                    actions: [ACCESS_CONTROL_ACTION_UPLOAD_FILE],
+                    expression: 'user.attributes.department == "eng"',
+                }],
+                imports: ['parent-policy-id'],
+            },
+        });
+        mockActions.saveChannelPolicy.mockResolvedValue({data: {rules: [], imports: ['parent-policy-id']}});
+
+        renderWithContext(<ChannelSettingsPermissionsPolicyTab {...baseProps}/>, initialState);
+
+        await userEvent.click(await screen.findByTestId(/^permissions-policy-row-delete-/));
+        await userEvent.click(await screen.findByTestId('SaveChangesPanel__save-btn'));
+
+        await waitFor(() => {
+            expect(mockActions.saveChannelPolicy).toHaveBeenCalledTimes(1);
+        });
+        const savedPolicy = mockActions.saveChannelPolicy.mock.calls[0][0];
+        expect(savedPolicy.rules).toEqual([]);
+        expect(savedPolicy.imports).toEqual(['parent-policy-id']);
+        expect(mockActions.deleteChannelPolicy).not.toHaveBeenCalled();
+    });
+
+    test('surfaces an error when deleting the emptied channel policy fails', async () => {
+        // If the backend delete fails for a real reason (not a 404), the tab must
+        // surface the error instead of silently reporting success.
+        mockActions.getChannelPolicy.mockResolvedValue({
+            data: {
+                id: 'channel_id',
+                rules: [{
+                    name: 'Only rule',
+                    role: ACCESS_CONTROL_CHANNEL_ROLE_USER,
+                    actions: [ACCESS_CONTROL_ACTION_UPLOAD_FILE],
+                    expression: 'user.attributes.department == "eng"',
+                }],
+                imports: [],
+            },
+        });
+        mockActions.deleteChannelPolicy.mockResolvedValue({error: {message: 'boom', status_code: 500}});
+
+        renderWithContext(<ChannelSettingsPermissionsPolicyTab {...baseProps}/>, initialState);
+
+        await userEvent.click(await screen.findByTestId(/^permissions-policy-row-delete-/));
+        await userEvent.click(await screen.findByTestId('SaveChangesPanel__save-btn'));
+
+        expect(await screen.findByText('boom')).toBeInTheDocument();
+        expect(mockActions.saveChannelPolicy).not.toHaveBeenCalled();
+    });
+
    test('preserves in-progress edits to an existing rule when validation fails', async () => {
        // Seed an existing permission rule so the non-404 load branch and the
        // edit path are exercised.
@@ -497,6 +497,36 @@ function ChannelSettingsPermissionsPolicyTab({
        const rulesWithMembership = buildRulesWithMembership(originalAllRules, originalMembershipExpression);
        const finalRules = buildRulesWithPermissionRules(rulesWithMembership, persistedPermissionRules);

+        // Deleting the last permission rule can leave the channel policy with no
+        // rules and no imports (a channel that only ever had permission rules and
+        // no membership rule). The server rejects an empty policy with
+        // "Unable to save access control policy." (AccessControlPolicy.IsValid:
+        // "Policy must either import or define rules"), so mirror the Membership
+        // Policy tab and delete the channel policy instead — returning the
+        // channel to standard access.
+        if (finalRules.length === 0 && originalImports.length === 0) {
+            const deleteResult = await actions.deleteChannelPolicy(channel.id);
+
+            // A 404 means the policy was never persisted (rules added and removed
+            // before any save), which is an effective success for this flow.
+            if (deleteResult.error && deleteResult.error.status_code !== 404) {
+                setFormError(deleteResult.error.message || formatMessage({
+                    id: 'channel_settings.permissions_policy.save_error',
+                    defaultMessage: 'Failed to save permission rules',
+                }));
+                return SAVE_RESULT_ERROR;
+            }
+
+            setOriginalAllRules([]);
+
+            // Mirror the Membership Policy tab's empty-delete path: once the
+            // channel policy is gone, the next save in this tab session must
+            // not re-POST the stale active flag from the deleted policy.
+            setOriginalActive(false);
+            setOriginalRulesJSON(JSON.stringify(persistedPermissionRules));
+            return SAVE_RESULT_SAVED;
+        }
+
        const policy = {
            id: channel.id,
            name: channel.display_name,