[pull] master from mattermost:master

e2e-tests/playwright/lib/src/ui/pages/content_review_dm.ts

@@ -17,6 +17,12 @@ export default class ContentReviewPage {
     readonly confirmRemoveMessageButton: Locator;
     readonly confirmKeepMessageButton: Locator;
     readonly confirmationModalComment: Locator;
+    readonly downloadReportCheckbox: Locator;
+    readonly formContinueButton: Locator;
+    readonly removePermanentlyButton: Locator;
+    readonly keepPermanentlyButton: Locator;
+    readonly removeWithoutReportButton: Locator;
+    readonly generatedSection: Locator;
 
     constructor(page: Page) {
         this.page = page;
@@ -33,6 +39,16 @@ export default class ContentReviewPage {
         this.confirmationModalComment = this.postActionConformationModal.getByTestId(
             'RemoveFlaggedMessageConfirmationModal__comment',
         );
+        this.downloadReportCheckbox = this.postActionConformationModal.getByTestId('download-report-checkbox');
+        this.formContinueButton = this.postActionConformationModal.getByRole('button', {name: 'Continue'});
+        this.removePermanentlyButton = this.postActionConformationModal.getByRole('button', {
+            name: 'Remove permanently',
+        });
+        this.keepPermanentlyButton = this.postActionConformationModal.getByRole('button', {name: 'Keep permanently'});
+        this.removeWithoutReportButton = this.postActionConformationModal.getByRole('button', {
+            name: 'Remove without report',
+        });
+        this.generatedSection = this.postActionConformationModal.getByTestId('generated-section');
     }
 
     async setReportCardByPostID(postID: string) {
@@ -175,4 +191,35 @@ export default class ContentReviewPage {
         await this.confirmKeepMessageButton.click();
         await this.postActionConformationModal.waitFor({state: 'hidden'});
     }
+
+    /**
+     * From the form step, advance to the report-generated step
+     * (downloadReport checkbox is on by default).
+     */
+    async submitFormAndWaitForReport() {
+        await this.formContinueButton.click();
+        await expect(this.generatedSection).toBeVisible({timeout: 30000});
+    }
+
+    async confirmRemovePermanently() {
+        await this.removePermanentlyButton.click();
+        await this.postActionConformationModal.waitFor({state: 'hidden'});
+    }
+
+    async confirmKeepPermanently() {
+        await this.keepPermanentlyButton.click();
+        await this.postActionConformationModal.waitFor({state: 'hidden'});
+    }
+
+    /**
+     * Skip-report path: uncheck the download checkbox, submit the form to reach
+     * the skip-confirm step, then confirm removal without a report.
+     */
+    async confirmRemoveWithoutReport() {
+        await this.downloadReportCheckbox.uncheck();
+        await this.confirmRemoveMessageButton.click();
+        await expect(this.removeWithoutReportButton).toBeVisible({timeout: 10000});
+        await this.removeWithoutReportButton.click();
+        await this.postActionConformationModal.waitFor({state: 'hidden'});
+    }
 }

e2e-tests/playwright/specs/functional/channels/content_flagging/deletion-report/deletion-report.spec.ts

@@ -14,8 +14,8 @@ import {setupContentFlagging, createPost} from './../support';
  * 4. Login as the reviewer and navigate to the content review DM
  * 5. Verify the deletion report summary table is posted in the reviewer's thread
  */
-test.fixme('Reviewer receives a deletion report summary after removing a flagged post', async ({pw}) => {
-    const {adminClient, team, user: reviewerUser, userClient: reviewerUserClient} = await pw.initSetup();
+test('Reviewer receives a deletion report summary after removing a flagged post', async ({pw}) => {
+    const {adminClient, team, user: reviewerUser} = await pw.initSetup();
 
     // Create author user..
     const authorUser = await pw.random.user('author');
@@ -28,18 +28,27 @@ test.fixme('Reviewer receives a deletion report summary after removing a flagged
     const message = `Sensitive 2 post by @${authorUser.username} to be removed`;
     const {post} = await createPost(adminClient, authorUserClient, team, authorUser, message);
 
-    // Flag and remove the post
+    // Flag the post
     await adminClient.flagPost(post.id, 'Classification mismatch', 'This message contains sensitive data');
 
     // Login as reviewer and navigate to content review DM
-    const {channelsPage} = await pw.testBrowser.login(reviewerUser);
+    const {channelsPage, contentReviewPage} = await pw.testBrowser.login(reviewerUser);
     await channelsPage.goto(team.name, '@content-review');
     await channelsPage.toBeVisible();
 
     const lastPost = await channelsPage.centerView.getLastPost();
     await lastPost.toContainText(message);
 
-    await reviewerUserClient.removeFlaggedPost(post.id, 'Removing: data spillage confirmed');
+    // Remove the post via the UI. The remove flow still routes through the
+    // skip-confirm step (destructive action), unlike the keep flow which now
+    // bypasses it.
+    await contentReviewPage.setReportCardByPostID(post.id);
+    await contentReviewPage.openViewDetails();
+    await contentReviewPage.waitForRHSVisible();
+    await contentReviewPage.openViewDetails();
+    await contentReviewPage.clickRemoveMessage();
+    await contentReviewPage.enterConfirmationComment('Removing: data spillage confirmed');
+    await contentReviewPage.confirmRemoveWithoutReport();
 
     await channelsPage.goto(team.name, '@content-review');
     await channelsPage.toBeVisible();

e2e-tests/playwright/specs/functional/channels/content_flagging/reviewer-actions/reviewer-actions.spec.ts

@@ -5,13 +5,13 @@ import {test} from '@mattermost/playwright-lib';
 
 import {setupContentFlagging, createPost, verifyAuthorNotification} from './../support';
 
-/** @objective Verify Retained and Removed Flagged posts do not appear in RHS after once reviewed
+/** @objective Verify Removed Flagged posts show appropriate status and do not show the post message
  * @testcase
  * 1. Create three users and add them as reviewers to a team
  * 2. Setup content flagging with the three users as reviewers
  * 3. Create a post and flag it
- * 4. As Reviewer 1, Retain the flagged post and verify the status is updated to 'Retained'
- * 5. As Reviewer 2, Verify the flagged post status is 'Retained'
+ * 4. As Reviewer 1, walk through the multi-step removal flow (form → report generated → remove permanently)
+ * 5. As Reviewer 2, verify the flagged post status is 'Removed' and the message has been replaced
  */
 test('Verify Removed Flagged posts show appropriate status and do not show the post message', async ({pw}) => {
     const {adminClient, team, user, userClient, adminUser} = await pw.initSetup();
@@ -72,7 +72,10 @@ test('Verify Removed Flagged posts show appropriate status and do not show the p
     });
     await secondContentReviewPage.clickRemoveMessage();
     await secondContentReviewPage.enterConfirmationComment(commentRemove);
-    await secondContentReviewPage.confirmRemove();
+
+    // New multi-step flow: Continue → wait for report to generate → Remove permanently
+    await secondContentReviewPage.submitFormAndWaitForReport();
+    await secondContentReviewPage.confirmRemovePermanently();
     await setupContentFlagging(adminClient, [adminUser.id, secondUserID, thirdUserID]);
 
     const {channelsPage: channelsPageThird, contentReviewPage: contentReviewPageThird} =

server/channels/api4/access_control.go

@@ -15,7 +15,7 @@ import (
 )
 
 // shouldRedactExpressions reports whether raw CEL expressions should be masked for this caller.
-// Returns true when both ABAC and attribute-value masking are enabled. Callers reading raw expressions
+// Masking is attribute-based, not permission-based: system admins who do not hold all values
 // in a policy must also receive redacted raw expressions.
 func shouldRedactExpressions(c *Context) bool {
 	return c.App.Config().FeatureFlags.AttributeBasedAccessControl &&
@@ -141,6 +141,10 @@ func createAccessControlPolicy(c *Context, w http.ResponseWriter, r *http.Reques
 	auditRec.AddEventObjectType("access_control_policy")
 	auditRec.AddEventResultState(np)
 
+	if shouldRedactExpressions(c) {
+		c.App.MaskPolicyExpressions(c.AppContext, np, c.AppContext.Session().UserId)
+	}
+
 	js, err := json.Marshal(np)
 	if err != nil {
 		c.Err = model.NewAppError("createAccessControlPolicy", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err)
@@ -193,6 +197,10 @@ func getAccessControlPolicy(c *Context, w http.ResponseWriter, r *http.Request)
 		return
 	}
 
+	if shouldRedactExpressions(c) {
+		c.App.MaskPolicyExpressions(c.AppContext, policy, c.AppContext.Session().UserId)
+	}
+
 	js, err := json.Marshal(policy)
 	if err != nil {
 		c.Err = model.NewAppError("getAccessControlPolicy", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err)
@@ -496,6 +504,12 @@ func searchAccessControlPolicies(c *Context, w http.ResponseWriter, r *http.Requ
 		policies = filtered
 	}
 
+	if shouldRedactExpressions(c) {
+		for _, p := range policies {
+			c.App.MaskPolicyExpressions(c.AppContext, p, c.AppContext.Session().UserId)
+		}
+	}
+
 	result := model.AccessControlPoliciesWithCount{
 		Policies: policies,
 		Total:    total,
@@ -628,6 +642,12 @@ func setActiveStatus(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 	auditRec.Success()
 
+	if shouldRedactExpressions(c) {
+		for _, p := range policies {
+			c.App.MaskPolicyExpressions(c.AppContext, p, c.AppContext.Session().UserId)
+		}
+	}
+
 	w.Header().Set("Content-Type", "application/json")
 	if err := json.NewEncoder(w).Encode(policies); err != nil {
 		c.Logger.Warn("Error while writing response", mlog.Err(err))

server/channels/api4/access_control_test.go

@@ -474,15 +474,22 @@ func TestDeleteAccessControlPolicy(t *testing.T) {
 
 		mockAccessControlService := &mocks.AccessControlServiceInterface{}
 		th.App.Srv().Channels().AccessControl = mockAccessControlService
-		// DeleteAccessControlPolicy resolves the policy first to decide
-		// whether to broadcast a channel access control update; return a
-		// parent policy so the channel-update path is not exercised here.
-		parentPolicy := &model.AccessControlPolicy{
-			ID:      samplePolicyID,
-			Type:    model.AccessControlPolicyTypeParent,
-			Version: model.AccessControlPolicyVersionV0_3,
+
+		// DeleteAccessControlPolicy resolves the policy first to decide whether
+		// to broadcast a channel access-control update after deletion.
+		channelPolicy := &model.AccessControlPolicy{
+			ID:       samplePolicyID,
+			Type:     model.AccessControlPolicyTypeChannel,
+			Version:  model.AccessControlPolicyVersionV0_3,
+			Revision: 1,
+			Rules: []model.AccessControlPolicyRule{
+				{
+					Expression: "user.attributes.team == 'engineering'",
+					Actions:    []string{"membership"},
+				},
+			},
 		}
-		mockAccessControlService.On("GetPolicy", mock.AnythingOfType("*request.Context"), samplePolicyID).Return(parentPolicy, nil).Times(1)
+		mockAccessControlService.On("GetPolicy", mock.AnythingOfType("*request.Context"), samplePolicyID).Return(channelPolicy, nil).Times(1)
 		mockAccessControlService.On("DeletePolicy", mock.AnythingOfType("*request.Context"), samplePolicyID).Return(nil).Times(1)
 
 		th.App.UpdateConfig(func(cfg *model.Config) {
@@ -1180,38 +1187,17 @@ func TestSearchChannelsForAccessControlPolicy(t *testing.T) {
 	t.Run("team admin body TeamIds forced to authorized team", func(t *testing.T) {
 		setupLicenseAndABAC(t)
 
-		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, 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)
+		policy := newSamplePolicy()
+		savedPolicy, err := th.App.Srv().Store().AccessControlPolicy().Save(th.Context, policy)
 		require.NoError(t, err)
 		defer func() {
-			_ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, basicTeamChannel.Id)
+			_ = th.App.Srv().Store().AccessControlPolicy().Delete(th.Context, savedPolicy.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)
@@ -1221,26 +1207,19 @@ 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 handler should force
+		// body tries to query otherTeam's channels. The fix should force
 		// TeamIds to BasicTeam.Id regardless of what the body says.
 		channelsResp, resp, err := th.Client.SearchChannelsForAccessControlPolicyForTeam(
-			context.Background(), savedParent.ID, th.BasicTeam.Id,
+			context.Background(), savedPolicy.ID, th.BasicTeam.Id,
 			model.ChannelSearch{TeamIds: []string{otherTeam.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, 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")
+		// None of the returned channels should belong to the other team
 		for _, ch := range channelsResp.Channels {
 			require.Equal(t, th.BasicTeam.Id, ch.TeamId,
-				"team admin must only see channels from the authorized team, got channel %s from team %s", ch.Id, ch.TeamId)
+				"team admin should only see channels from the authorized team, got channel %s from team %s", ch.Id, ch.TeamId)
 		}
 	})
 
@@ -1492,6 +1471,81 @@ func newParentPolicy(teamID string) *model.AccessControlPolicy {
 	}
 }
 
+// TestResponseMaskingOnPolicyEndpoints verifies that every API endpoint returning an
+// AccessControlPolicy redacts the raw CEL expression for callers who cannot see all
+// values. The risk is a future endpoint forgetting to call MaskPolicyExpressions
+// before serializing — the masked visual AST would still hide values, but the raw
+// rule.Expression in the same response would leak them in plain text. We force the
+// fail-closed branch (unknown property field) so the masking always produces the
+// "--------" sentinel without requiring a real CPA setup.
+func TestResponseMaskingOnPolicyEndpoints(t *testing.T) {
+	// SetupConfig sets FFs before route init via SetReadOnlyFF(false). Avoids
+	// os.Setenv which isn't parallel-safe.
+	th := SetupConfig(t, func(cfg *model.Config) {
+		cfg.FeatureFlags.AttributeBasedAccessControl = true
+		cfg.FeatureFlags.AttributeValueMasking = true
+	}).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 = new(true)
+	})
+
+	const sensitiveExpr = `user.attributes.f_unknown_field == "TF-Zulu"`
+	const expectedMaskedExpr = `user.attributes.f_unknown_field == "--------"`
+
+	// A condition referencing an unknown CPA field forces MaskPolicyExpressions
+	// down the fail-closed branch, which replaces the literal value with the
+	// masked-token sentinel. That gives us a deterministic assertion target
+	// without needing to seed a CPA group + protected field in this test.
+	unknownFieldAST := &model.VisualExpression{
+		Conditions: []model.Condition{
+			{
+				Attribute: "user.attributes.f_unknown_field",
+				Operator:  "==",
+				Value:     "TF-Zulu",
+				ValueType: model.LiteralValue,
+			},
+		},
+	}
+
+	newPolicy := func(id string) *model.AccessControlPolicy {
+		return &model.AccessControlPolicy{
+			ID:       id,
+			Type:     model.AccessControlPolicyTypeChannel,
+			Version:  model.AccessControlPolicyVersionV0_3,
+			Revision: 1,
+			Rules: []model.AccessControlPolicyRule{
+				{Actions: []string{"membership"}, Expression: sensitiveExpr},
+			},
+		}
+	}
+
+	t.Run("getAccessControlPolicy response is masked", func(t *testing.T) {
+		// GET is the canonical read path — masking here means the raw CEL in the
+		// policy response cannot leak values the caller couldn't already see in the
+		// visual AST. The create / search / setActive paths share the same
+		// MaskPolicyExpressions call so they're covered by inspection. Unit-testing
+		// them through the HTTP handler is impractical because
+		// validatePolicyExpressionValues rejects unknown-field references before
+		// MaskPolicyExpressions ever runs, and we can't seed a real shared_only
+		// CPA field without plugin context. End-to-end paths are covered by E2E.
+		mockACS := &mocks.AccessControlServiceInterface{}
+		th.App.Srv().Channels().AccessControl = mockACS
+		stored := newPolicy(th.BasicChannel.Id)
+		mockACS.On("GetPolicy", mock.AnythingOfType("*request.Context"), stored.ID).Return(stored, nil)
+		mockACS.On("ExpressionToVisualAST", mock.Anything, mock.Anything).Return(unknownFieldAST, nil).Maybe()
+
+		result, resp, err := th.SystemAdminClient.GetAccessControlPolicy(context.Background(), stored.ID)
+		require.NoError(t, err)
+		CheckOKStatus(t, resp)
+		require.NotEmpty(t, result.Rules)
+		require.Equal(t, expectedMaskedExpr, result.Rules[0].Expression,
+			"get response must mask the raw CEL exactly")
+	})
+}
+
 func TestCreateAccessControlPolicyTeamAdmin(t *testing.T) {
 	os.Setenv("MM_FEATUREFLAGS_ATTRIBUTEBASEDACCESSCONTROL", "true")
 	th := Setup(t).InitBasic(t)

server/channels/api4/content_flagging_report.go

@@ -41,6 +41,7 @@ func generateFlaggedPostReport(c *Context, w http.ResponseWriter, r *http.Reques
 	model.AddEventParameterToAuditRec(auditRec, "flaggedPostId", postId)
 	model.AddEventParameterToAuditRec(auditRec, "userId", userId)
 	model.AddEventParameterToAuditRec(auditRec, "comment", actionRequest.Comment)
+	model.AddEventParameterToAuditRec(auditRec, "action", actionRequest.Action)
 
 	post, appErr := c.App.GetSinglePost(c.AppContext, postId, true)
 	if appErr != nil {
@@ -65,7 +66,7 @@ func generateFlaggedPostReport(c *Context, w http.ResponseWriter, r *http.Reques
 		return
 	}
 
-	reportPath, appErr := c.App.GenerateFlaggedPostReport(c.AppContext, postId, userId, actionRequest.Comment)
+	reportPath, appErr := c.App.GenerateFlaggedPostReport(c.AppContext, postId, userId, actionRequest.Comment, actionRequest.Action)
 	if appErr != nil {
 		c.Err = appErr
 		return