From f0360a838af51ab1e204ed05b0bcb9234f530a41 Mon Sep 17 00:00:00 2001 From: Harshil Sharma <18575143+harshilsharma63@users.noreply.github.com> Date: Mon, 18 May 2026 20:24:50 +0530 Subject: [PATCH 1/3] Data spillage report generation UI (#36340) * Added base fr report generation * WIP * implemented UI flow * implemented UI flow * restructured the modal code into sub components * Refactoring and cleanup * lint fixes, added new tests * i18n fix * test fix * Updated test * CI * Several improvements * WIP * Added tests * Addressed some security enhancements * Created zip writer entery later * Improved a test to check for file content * Improved error handling * Made a geneeric function * Updated classes * accepting comment in report API * Added more tests * Integrated new API param * Removed an unnecessary check * Made a geneeric function * Made a geneeric function * Made the comment body not required and updated API docs * Updated report generation API call in download report button * Included decision in report and removed confirmation when keeping message * Updated test * Add explicit wait for removeWithoutReportButton visibility in test Prevent race condition by waiting for the button to be visible after UI transitions to skip-confirm step before clicking it. Co-authored-by: Maria A Nunez * PR Feedback * explicitelly added return statement * Included actor details in report * Updated tests --------- Co-authored-by: maria.nunez Co-authored-by: Cursor Agent Co-authored-by: Mattermost Build --- .../lib/src/ui/pages/content_review_dm.ts | 47 +++ .../deletion-report/deletion-report.spec.ts | 19 +- .../reviewer-actions/reviewer-actions.spec.ts | 11 +- .../channels/api4/content_flagging_report.go | 3 +- .../api4/content_flagging_report_test.go | 41 ++ .../channels/app/content_flagging_report.go | 46 ++- .../app/content_flagging_report_test.go | 95 ++++- server/public/model/content_flagging.go | 6 + .../public/model/content_flagging_report.go | 3 + .../data_spillage_download_report.scss | 16 + .../data_spillage_download_report.test.tsx | 111 ++++++ .../data_spillage_download_report.tsx | 131 +++++++ .../data_spillage_report.test.tsx | 33 ++ .../data_spillage_report.tsx | 61 ++- .../properties_card_view.test.tsx | 99 +++++ .../properties_card_view.tsx | 43 ++- .../body_main_action_text.tsx | 62 +++ .../error_step/error_step_body.scss | 14 + .../error_step/error_step_body.tsx | 74 ++++ .../error_step/error_step_footer.tsx | 57 +++ .../flagged_message_body.scss | 14 + .../flagged_message_body.tsx | 62 +++ .../form_step/form_step_body.scss | 30 ++ .../form_step/form_step_body.tsx | 83 ++++ .../form_step/form_step_footer.scss | 21 + .../form_step/form_step_footer.tsx | 82 ++++ .../generated_step/generated_step_body.tsx | 61 +++ .../generated_step/generated_step_footer.tsx | 66 ++++ .../generating_step/generating_step_body.tsx | 63 +++ .../generating_step_footer.tsx | 58 +++ ...ve_flagged_message_confirmation_modal.scss | 56 ++- ...lagged_message_confirmation_modal.test.tsx | 278 +++++++++++++- ...ove_flagged_message_confirmation_modal.tsx | 358 +++++++++++------- .../report_notice.scss | 76 ++++ .../report_notice.tsx | 30 ++ .../skip_confirm_step_body.tsx | 42 ++ .../skip_confirm_step_footer.scss | 8 + .../skip_confirm_step_footer.tsx | 44 +++ webapp/channels/src/i18n/en.json | 25 +- webapp/platform/client/src/client4.test.ts | 19 + webapp/platform/client/src/client4.ts | 17 + 41 files changed, 2212 insertions(+), 253 deletions(-) create mode 100644 webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.scss create mode 100644 webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.test.tsx create mode 100644 webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.tsx create mode 100644 webapp/channels/src/components/properties_card_view/properties_card_view.test.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/body_main_action_text.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_body.scss create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_body.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_footer.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/flagged_message_body.scss create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/flagged_message_body.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_body.scss create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_body.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_footer.scss create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_footer.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/generated_step/generated_step_body.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/generated_step/generated_step_footer.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/generating_step/generating_step_body.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/generating_step/generating_step_footer.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/report_notice.scss create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/report_notice.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_body.tsx create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_footer.scss create mode 100644 webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_footer.tsx diff --git a/e2e-tests/playwright/lib/src/ui/pages/content_review_dm.ts b/e2e-tests/playwright/lib/src/ui/pages/content_review_dm.ts index 613ef8dc3ff..4f090f1760e 100644 --- a/e2e-tests/playwright/lib/src/ui/pages/content_review_dm.ts +++ b/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'}); + } } diff --git a/e2e-tests/playwright/specs/functional/channels/content_flagging/deletion-report/deletion-report.spec.ts b/e2e-tests/playwright/specs/functional/channels/content_flagging/deletion-report/deletion-report.spec.ts index 32e6da12477..30ad41099fd 100644 --- a/e2e-tests/playwright/specs/functional/channels/content_flagging/deletion-report/deletion-report.spec.ts +++ b/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(); diff --git a/e2e-tests/playwright/specs/functional/channels/content_flagging/reviewer-actions/reviewer-actions.spec.ts b/e2e-tests/playwright/specs/functional/channels/content_flagging/reviewer-actions/reviewer-actions.spec.ts index e363300ef84..232412e4ebd 100644 --- a/e2e-tests/playwright/specs/functional/channels/content_flagging/reviewer-actions/reviewer-actions.spec.ts +++ b/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} = diff --git a/server/channels/api4/content_flagging_report.go b/server/channels/api4/content_flagging_report.go index f09f99ff868..8ceb32cb4c6 100644 --- a/server/channels/api4/content_flagging_report.go +++ b/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 diff --git a/server/channels/api4/content_flagging_report_test.go b/server/channels/api4/content_flagging_report_test.go index fc7bf3c8b4b..05a1dc87cc3 100644 --- a/server/channels/api4/content_flagging_report_test.go +++ b/server/channels/api4/content_flagging_report_test.go @@ -7,9 +7,11 @@ import ( "archive/zip" "bytes" "context" + "io" "net/http" "testing" + "github.com/goccy/go-yaml" "github.com/mattermost/mattermost/server/public/model" "github.com/stretchr/testify/require" ) @@ -128,6 +130,45 @@ func TestGenerateFlaggedPostReport(t *testing.T) { require.True(t, foundAttachment, "attachment for the flagged post should be present in the report archive") }) + t.Run("Should include reviewer decision from request action", func(t *testing.T) { + appErr := setBasicCommonReviewerConfig(th) + require.Nil(t, appErr) + + post := th.CreatePost(t) + flagPostViaAPI(t, client, post.Id) + + report, resp, err := client.GenerateFlaggedPostReport(context.Background(), post.Id, &model.FlagContentActionRequest{ + Comment: "investigation note", + Action: model.ContentFlaggingActionRemove, + }) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.NotEmpty(t, report) + + zr, err := zip.NewReader(bytes.NewReader(report), int64(len(report))) + require.NoError(t, err) + + var review model.FlaggedPostReportContentReview + var found bool + for _, f := range zr.File { + if f.Name != "content_review.yaml" { + continue + } + rc, err := f.Open() + require.NoError(t, err) + b, err := io.ReadAll(rc) + require.NoError(t, err) + _ = rc.Close() + require.NoError(t, yaml.Unmarshal(b, &review)) + found = true + break + } + require.True(t, found, "content_review.yaml should be present in the report archive") + require.Equal(t, "remove", review.ActorDecision) + require.Equal(t, th.BasicUser.Id, review.ActorUserId) + require.Equal(t, th.BasicUser.Username, review.ActorUsername) + }) + t.Run("Should include edit history entries in the generated report", func(t *testing.T) { appErr := setBasicCommonReviewerConfig(th) require.Nil(t, appErr) diff --git a/server/channels/app/content_flagging_report.go b/server/channels/app/content_flagging_report.go index c9cd28f5134..a821b1b4593 100644 --- a/server/channels/app/content_flagging_report.go +++ b/server/channels/app/content_flagging_report.go @@ -33,7 +33,7 @@ const ( // GenerateFlaggedPostReport builds a ZIP archive of a flagged post's data into a // temporary file and returns the file path. The caller is responsible for // removing the file when the response has been served. -func (a *App) GenerateFlaggedPostReport(rctx request.CTX, postID, generatedByUserID, comment string) (string, *model.AppError) { +func (a *App) GenerateFlaggedPostReport(rctx request.CTX, postID, generatedByUserID, comment, action string) (string, *model.AppError) { if appErr := a.ensureActorCommentForReport(rctx, postID, comment); appErr != nil { return "", appErr } @@ -51,7 +51,7 @@ func (a *App) GenerateFlaggedPostReport(rctx request.CTX, postID, generatedByUse zw := zip.NewWriter(tmp) - if appErr := a.writeFlaggedPostReport(rctx, zw, postID, generatedByUserID); appErr != nil { + if appErr := a.writeFlaggedPostReport(rctx, zw, postID, generatedByUserID, action); appErr != nil { _ = zw.Close() cleanup() return "", appErr @@ -73,7 +73,7 @@ func (a *App) GenerateFlaggedPostReport(rctx request.CTX, postID, generatedByUse return tmpPath, nil } -func (a *App) writeFlaggedPostReport(rctx request.CTX, zw *zip.Writer, postID, generatedByUserID string) *model.AppError { +func (a *App) writeFlaggedPostReport(rctx request.CTX, zw *zip.Writer, postID, generatedByUserID, action string) *model.AppError { rc, appErr := a.loadFlaggedPostReportContext(rctx, postID) if appErr != nil { return appErr @@ -89,7 +89,7 @@ func (a *App) writeFlaggedPostReport(rctx request.CTX, zw *zip.Writer, postID, g if appErr := a.writeEditHistorySection(rctx, zw, rc, seenFiles); appErr != nil { return appErr } - if appErr := a.writeContentReviewEntry(rctx, zw, rc.Post); appErr != nil { + if appErr := a.writeContentReviewEntry(rctx, zw, rc.Post, generatedByUserID, action); appErr != nil { return appErr } if appErr := a.writeReportMetadataEntry(zw, generatedByUserID); appErr != nil { @@ -183,8 +183,8 @@ func (a *App) writeEditHistorySection(rctx request.CTX, zw *zip.Writer, rc *mode return nil } -func (a *App) writeContentReviewEntry(rctx request.CTX, zw *zip.Writer, post *model.Post) *model.AppError { - payload, appErr := a.buildContentReviewYAML(rctx, post) +func (a *App) writeContentReviewEntry(rctx request.CTX, zw *zip.Writer, post *model.Post, generatedByUserID, action string) *model.AppError { + payload, appErr := a.buildContentReviewYAML(rctx, post, generatedByUserID, action) if appErr != nil { return appErr } @@ -199,16 +199,18 @@ func (a *App) writeContentReviewEntry(rctx request.CTX, zw *zip.Writer, post *mo // already present (set by a prior keep/remove or report-generation), it is // preserved so the existing reviewer note is never overwritten. func (a *App) ensureActorCommentForReport(rctx request.CTX, postID, comment string) *model.AppError { + if comment == "" { + return nil + } + existing, appErr := a.GetPostContentFlaggingPropertyValue(postID, contentFlaggingPropertyNameActorComment) if appErr != nil && appErr.StatusCode != http.StatusNotFound { return appErr } + if existing != nil { return nil } - if comment == "" { - return nil - } groupID, gErr := a.ContentFlaggingGroupId() if gErr != nil { @@ -233,6 +235,7 @@ func (a *App) ensureActorCommentForReport(rctx request.CTX, postID, comment stri Value: json.RawMessage(commentBytes), }, } + if _, appErr := a.CreatePropertyValues(rctx, propertyValues); appErr != nil { return model.NewAppError("ensureActorCommentForReport", "app.data_spillage.create_property_values.app_error", nil, "", http.StatusInternalServerError).Wrap(appErr) } @@ -279,7 +282,7 @@ func buildPostYAML(post *model.Post, channel *model.Channel, team *model.Team, a return out } -func (a *App) buildContentReviewYAML(rctx request.CTX, post *model.Post) (model.FlaggedPostReportContentReview, *model.AppError) { +func (a *App) buildContentReviewYAML(rctx request.CTX, post *model.Post, generatedByUserID, pendingAction string) (model.FlaggedPostReportContentReview, *model.AppError) { out := model.FlaggedPostReportContentReview{} values, appErr := a.GetPostContentFlaggingPropertyValues(post.Id) @@ -332,13 +335,32 @@ func (a *App) buildContentReviewYAML(rctx request.CTX, post *model.Post) (model. } } - // Reviewer details: prefer the actor (the one who took the keep/remove action) - // when present, otherwise fall back to the assigned reviewer. reviewerID := decodePropertyString(rctx, byName, contentFlaggingPropertyNameReviewerUserID) out.ReviewerUserID = reviewerID out.ReviewerComment = decodePropertyString(rctx, byName, contentFlaggingPropertyNameActorComment) out.ActionTime = decodePropertyInt64(rctx, byName, contentFlaggingPropertyNameActionTime) + // We want to include the actor details only when an action is being performed - retain or delete the quarantined post. + if pendingAction != "" { + if u, uErr := a.GetUser(generatedByUserID); uErr == nil { + out.ActorUsername = u.Username + out.ActorUserId = u.Id + } else { + rctx.Logger().Warn("Failed to fetch report generator user for flagged post report", mlog.String("user_id", generatedByUserID), mlog.Err(uErr)) + } + } + + switch decodePropertyString(rctx, byName, ContentFlaggingPropertyNameStatus) { + case model.ContentFlaggingStatusRetained: + out.ActorDecision = model.ContentFlaggingActionKeep + case model.ContentFlaggingStatusRemoved: + out.ActorDecision = model.ContentFlaggingActionRemove + default: + if pendingAction == model.ContentFlaggingActionKeep || pendingAction == model.ContentFlaggingActionRemove { + out.ActorDecision = pendingAction + } + } + if reviewerID != "" { if u, uErr := a.GetUser(reviewerID); uErr == nil { out.ReviewerUsername = u.Username diff --git a/server/channels/app/content_flagging_report_test.go b/server/channels/app/content_flagging_report_test.go index 6253c79a79c..cc463e0023d 100644 --- a/server/channels/app/content_flagging_report_test.go +++ b/server/channels/app/content_flagging_report_test.go @@ -52,7 +52,7 @@ func TestGenerateFlaggedPostReport(t *testing.T) { appErr := setBaseConfig(th) require.Nil(t, appErr) - path, appErr := th.App.GenerateFlaggedPostReport(th.Context, model.NewId(), th.BasicUser.Id, "") + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, model.NewId(), th.BasicUser.Id, "", "") require.NotNil(t, appErr) require.Empty(t, path) }) @@ -63,7 +63,7 @@ func TestGenerateFlaggedPostReport(t *testing.T) { post := setupFlaggedPost(t, th) - path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, model.NewId(), "") + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, model.NewId(), "", "") require.NotNil(t, appErr) require.Empty(t, path) }) @@ -74,7 +74,7 @@ func TestGenerateFlaggedPostReport(t *testing.T) { post := setupFlaggedPost(t, th) - path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "") + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", "") require.Nil(t, appErr) require.NotEmpty(t, path) @@ -93,7 +93,7 @@ func TestGenerateFlaggedPostReport(t *testing.T) { post := setupFlaggedPost(t, th) - path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "") + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", "") require.Nil(t, appErr) entries := readReportZip(t, path) @@ -113,7 +113,7 @@ func TestGenerateFlaggedPostReport(t *testing.T) { post := setupFlaggedPost(t, th) - path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "") + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", "") require.Nil(t, appErr) entries := readReportZip(t, path) @@ -131,7 +131,7 @@ func TestGenerateFlaggedPostReport(t *testing.T) { post := setupFlaggedPost(t, th) - path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "") + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", "") require.Nil(t, appErr) entries := readReportZip(t, path) @@ -143,6 +143,85 @@ func TestGenerateFlaggedPostReport(t *testing.T) { require.Equal(t, "spam", review.ReporterReason) require.Equal(t, "This is spam content", review.ReporterComment) require.Greater(t, review.ReportTimestamp, int64(0)) + require.Empty(t, review.ActorDecision) + require.Empty(t, review.ActorUserId) + require.Empty(t, review.ActorUsername) + }) + + t.Run("content_review.yaml records remove decision after permanent delete", func(t *testing.T) { + appErr := setBaseConfig(th) + require.Nil(t, appErr) + + post := setupFlaggedPost(t, th) + + appErr = th.App.PermanentDeleteFlaggedPost(th.Context, &model.FlagContentActionRequest{Comment: "violates policy"}, th.SystemAdminUser.Id, post) + require.Nil(t, appErr) + + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", "") + require.Nil(t, appErr) + + entries := readReportZip(t, path) + var review model.FlaggedPostReportContentReview + require.NoError(t, yaml.Unmarshal(entries["content_review.yaml"], &review)) + require.Equal(t, "remove", review.ActorDecision) + require.Empty(t, review.ActorUserId) + require.Empty(t, review.ActorUsername) + }) + + t.Run("content_review.yaml records keep decision after keep action", func(t *testing.T) { + appErr := setBaseConfig(th) + require.Nil(t, appErr) + + post := setupFlaggedPost(t, th) + + appErr = th.App.KeepFlaggedPost(th.Context, &model.FlagContentActionRequest{Comment: "looks fine"}, th.SystemAdminUser.Id, post) + require.Nil(t, appErr) + + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", "") + require.Nil(t, appErr) + + entries := readReportZip(t, path) + var review model.FlaggedPostReportContentReview + require.NoError(t, yaml.Unmarshal(entries["content_review.yaml"], &review)) + require.Equal(t, "keep", review.ActorDecision) + require.Empty(t, review.ActorUserId) + require.Empty(t, review.ActorUsername) + }) + + t.Run("content_review.yaml uses pending action when status is not yet committed", func(t *testing.T) { + appErr := setBaseConfig(th) + require.Nil(t, appErr) + + post := setupFlaggedPost(t, th) + + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", model.ContentFlaggingActionRemove) + require.Nil(t, appErr) + + entries := readReportZip(t, path) + var review model.FlaggedPostReportContentReview + require.NoError(t, yaml.Unmarshal(entries["content_review.yaml"], &review)) + require.Equal(t, "remove", review.ActorDecision) + require.Equal(t, th.BasicUser.Id, review.ActorUserId) + require.Equal(t, th.BasicUser.Username, review.ActorUsername) + }) + + t.Run("content_review.yaml ignores invalid pending action", func(t *testing.T) { + appErr := setBaseConfig(th) + require.Nil(t, appErr) + + post := setupFlaggedPost(t, th) + + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", "bogus") + require.Nil(t, appErr) + + entries := readReportZip(t, path) + var review model.FlaggedPostReportContentReview + require.NoError(t, yaml.Unmarshal(entries["content_review.yaml"], &review)) + require.Empty(t, review.ActorDecision) + // Actor details are still populated whenever a pending action is supplied, + // even when the value isn't a recognised decision. + require.Equal(t, th.BasicUser.Id, review.ActorUserId) + require.Equal(t, th.BasicUser.Username, review.ActorUsername) }) t.Run("includes file attachments for the base post", func(t *testing.T) { @@ -181,7 +260,7 @@ func TestGenerateFlaggedPostReport(t *testing.T) { appErr = th.App.FlagPost(th.Context, post, th.BasicTeam.Id, th.BasicUser2.Id, flagData) require.Nil(t, appErr) - path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "") + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", "") require.Nil(t, appErr) entries := readReportZip(t, path) @@ -217,7 +296,7 @@ func TestGenerateFlaggedPostReport(t *testing.T) { appErr = th.App.FlagPost(th.Context, post, th.BasicTeam.Id, th.BasicUser2.Id, flagData) require.Nil(t, appErr) - path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "") + path, appErr := th.App.GenerateFlaggedPostReport(th.Context, post.Id, th.BasicUser.Id, "", "") require.Nil(t, appErr) entries := readReportZip(t, path) diff --git a/server/public/model/content_flagging.go b/server/public/model/content_flagging.go index 073a4b8c6c2..207aa0fb629 100644 --- a/server/public/model/content_flagging.go +++ b/server/public/model/content_flagging.go @@ -26,6 +26,11 @@ const ( ContentFlaggingStatusRetained = "Retained" ) +const ( + ContentFlaggingActionKeep = "keep" + ContentFlaggingActionRemove = "remove" +) + type FlagContentRequest struct { Reason string `json:"reason"` Comment string `json:"comment,omitempty"` @@ -53,6 +58,7 @@ func (f *FlagContentRequest) IsValid(commentRequired bool, validReasons []string type FlagContentActionRequest struct { Comment string `json:"comment,omitempty"` + Action string `json:"action,omitempty"` } func (f *FlagContentActionRequest) IsValid(commentRequired bool) *AppError { diff --git a/server/public/model/content_flagging_report.go b/server/public/model/content_flagging_report.go index 247566112a3..aaae6e650d2 100644 --- a/server/public/model/content_flagging_report.go +++ b/server/public/model/content_flagging_report.go @@ -74,6 +74,9 @@ type FlaggedPostReportContentReview struct { ReviewerUsername string `yaml:"reviewer_username,omitempty"` ReviewerComment string `yaml:"reviewer_comment,omitempty"` ActionTime int64 `yaml:"action_time,omitempty"` + ActorDecision string `yaml:"actor_decision,omitempty"` + ActorUserId string `yaml:"actor_user_id,omitempty"` + ActorUsername string `yaml:"actor_username,omitempty"` } // FlaggedPostReportMetadata is the on-disk shape for report_metadata.yaml. diff --git a/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.scss b/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.scss new file mode 100644 index 00000000000..52ae8cb64ea --- /dev/null +++ b/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.scss @@ -0,0 +1,16 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +.DataSpillageDownloadReport { + display: flex; + + .btn { + display: inline-flex; + align-items: center; + gap: 4px; + } + + .LoadingSpinner { + line-height: 0; + } +} diff --git a/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.test.tsx b/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.test.tsx new file mode 100644 index 00000000000..37fff00deaf --- /dev/null +++ b/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.test.tsx @@ -0,0 +1,111 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {screen, waitFor} from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import React from 'react'; + +import {Client4} from 'mattermost-redux/client'; + +import DataSpillageDownloadReport from 'components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report'; + +import {renderWithContext} from 'tests/react_testing_utils'; + +describe('DataSpillageDownloadReport', () => { + const flaggedPostId = 'flagged_post_id'; + + let originalCreateObjectURL: typeof URL.createObjectURL; + let originalRevokeObjectURL: typeof URL.revokeObjectURL; + + beforeEach(() => { + jest.clearAllMocks(); + + Client4.generateFlaggedPostReport = jest.fn().mockResolvedValue(new Blob(['report'], {type: 'application/zip'})); + + originalCreateObjectURL = URL.createObjectURL; + originalRevokeObjectURL = URL.revokeObjectURL; + URL.createObjectURL = jest.fn().mockReturnValue('blob:mock-url'); + URL.revokeObjectURL = jest.fn(); + + // eslint-disable-next-line no-console + console.error = jest.fn(); + }); + + afterEach(() => { + URL.createObjectURL = originalCreateObjectURL; + URL.revokeObjectURL = originalRevokeObjectURL; + }); + + test('renders idle Download Report button', () => { + renderWithContext(); + + const button = screen.getByTestId('data-spillage-action-download-report'); + expect(button).toBeVisible(); + expect(button).toHaveTextContent('Download Report'); + expect(button).not.toBeDisabled(); + }); + + test('click triggers download and returns to idle on success', async () => { + renderWithContext(); + + await userEvent.click(screen.getByTestId('data-spillage-action-download-report')); + + await waitFor(() => { + expect(Client4.generateFlaggedPostReport).toHaveBeenCalledWith( + flaggedPostId, + '', + undefined, + expect.any(AbortSignal), + ); + }); + await waitFor(() => { + expect(URL.createObjectURL).toHaveBeenCalled(); + expect(URL.revokeObjectURL).toHaveBeenCalledWith('blob:mock-url'); + }); + + // Returns to idle state + await waitFor(() => { + expect(screen.getByTestId('data-spillage-action-download-report')).toHaveTextContent('Download Report'); + }); + }); + + test('shows error state when request rejects', async () => { + Client4.generateFlaggedPostReport = jest.fn().mockRejectedValue(new Error('boom')); + + renderWithContext(); + + await userEvent.click(screen.getByTestId('data-spillage-action-download-report')); + + await waitFor(() => { + expect(screen.getByTestId('data-spillage-action-download-report')).toHaveTextContent('Generation failed. Try again.'); + }); + expect(URL.createObjectURL).not.toHaveBeenCalled(); + }); + + test('aborts in-flight request on unmount', async () => { + // Hold the request promise open until we unmount + let resolveRequest: (value: Blob) => void = () => {}; + const requestPromise = new Promise((resolve) => { + resolveRequest = resolve; + }); + Client4.generateFlaggedPostReport = jest.fn().mockReturnValue(requestPromise); + + const {unmount} = renderWithContext(); + + await userEvent.click(screen.getByTestId('data-spillage-action-download-report')); + + // While generating, button is disabled and shows generating label + await waitFor(() => { + expect(screen.getByTestId('data-spillage-action-download-report')).toHaveTextContent('Generating report…'); + }); + expect(screen.getByTestId('data-spillage-action-download-report')).toBeDisabled(); + + unmount(); + + // Resolving after unmount should not trigger a download + resolveRequest(new Blob(['report'])); + await Promise.resolve(); + await Promise.resolve(); + expect(URL.createObjectURL).not.toHaveBeenCalled(); + }); +}); diff --git a/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.tsx b/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.tsx new file mode 100644 index 00000000000..e17524f37ae --- /dev/null +++ b/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report.tsx @@ -0,0 +1,131 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import classNames from 'classnames'; +import React, {useCallback, useEffect, useRef, useState} from 'react'; +import {FormattedMessage} from 'react-intl'; + +import {Client4} from 'mattermost-redux/client'; + +import LoadingSpinner from 'components/widgets/loading/loading_spinner'; + +import './data_spillage_download_report.scss'; + +type Props = { + flaggedPostId: string; +} + +type Status = 'idle' | 'generating' | 'error'; + +export default function DataSpillageDownloadReport({flaggedPostId}: Props) { + const [status, setStatus] = useState('idle'); + const abortControllerRef = useRef(null); + + useEffect(() => { + // Cleanup function to cancel in-progress API calls + return () => { + abortControllerRef.current?.abort(); + }; + }, []); + + const handleClick = useCallback(async () => { + if (status === 'generating') { + return; + } + + const controller = new AbortController(); + abortControllerRef.current?.abort(); + abortControllerRef.current = controller; + + setStatus('generating'); + + let blob: Blob | undefined; + + try { + blob = await Client4.generateFlaggedPostReport(flaggedPostId, '', undefined, controller.signal); + if (controller.signal.aborted) { + return; + } + } catch (err) { + if (controller.signal.aborted) { + return; + } + + // eslint-disable-next-line no-console + console.error(err); + setStatus('error'); + return; + } + + if (controller.signal.aborted || !blob) { + return; + } + + const downloadUrl = URL.createObjectURL(blob); + const a = document.createElement('a'); + a.href = downloadUrl; + a.download = `flagged-post-${flaggedPostId}-${Date.now()}.zip`; + document.body.appendChild(a); + a.click(); + a.remove(); + URL.revokeObjectURL(downloadUrl); + + setStatus('idle'); + }, [flaggedPostId, status]); + + let icon; + let label; + let buttonClass; + + switch (status) { + case 'generating': + icon = ; + label = ( + + ); + buttonClass = 'btn-tertiary'; + break; + case 'error': + icon = ; + label = ( + + ); + buttonClass = 'btn-danger'; + break; + case 'idle': + default: + icon = ; + label = ( + + ); + buttonClass = 'btn-tertiary'; + break; + } + + return ( +
+ +
+ ); +} diff --git a/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_report.test.tsx b/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_report.test.tsx index cf0ee772e6c..6d586f12dfd 100644 --- a/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_report.test.tsx +++ b/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_report.test.tsx @@ -430,4 +430,37 @@ describe('components/post_view/data_spillage_report/DataSpillageReport', () => { expect(screen.queryByTestId('data-spillage-action-remove-message')).toBeVisible(); expect(screen.queryByTestId('data-spillage-action-keep-message')).toBeVisible(); }); + + describe.each([ + ['Pending', true], + ['Retained', false], + ['Removed', false], + ])('Download Report button when status is %s', (status, expectActions) => { + it('is rendered in action rows in RHS mode', async () => { + usePostContentFlaggingValues.mockReturnValue( + postContentFlaggingValues.map((v) => + (v.field_id === contentFlaggingFields.status.id ? {...v, value: status} : v), + ), + ); + + renderWithContext( + , + baseState, + ); + + await act(async () => {}); + + expect(screen.getByTestId('data-spillage-action-download-report')).toBeVisible(); + expect(screen.getByTestId('data-spillage-action-download-report')).toHaveTextContent('Download Report'); + + if (expectActions) { + expect(screen.queryByTestId('data-spillage-action')).toBeVisible(); + } else { + expect(screen.queryByTestId('data-spillage-action')).not.toBeInTheDocument(); + } + }); + }); }); diff --git a/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_report.tsx b/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_report.tsx index c7fcfd9ab53..bab114dfb7e 100644 --- a/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_report.tsx +++ b/webapp/channels/src/components/post_view/data_spillage_report/data_spillage_report.tsx @@ -2,7 +2,7 @@ // See LICENSE.txt for license information. import React, {useMemo} from 'react'; -import {useIntl} from 'react-intl'; +import {FormattedMessage, useIntl} from 'react-intl'; import {ContentFlaggingStatus} from '@mattermost/types/content_flagging'; import type {Post} from '@mattermost/types/posts'; @@ -16,15 +16,17 @@ import {useGetContentFlaggingChannel, useGetContentFlaggingTeam, useGetFlaggedPo import {useContentFlaggingFields, usePostContentFlaggingValues} from 'components/common/hooks/useContentFlaggingFields'; import {useUser} from 'components/common/hooks/useUser'; import DataSpillageAction from 'components/post_view/data_spillage_report/data_spillage_actions/data_spillage_actions'; -import type {PropertiesCardViewMetadata} from 'components/properties_card_view/properties_card_view'; +import DataSpillageDownloadReport from 'components/post_view/data_spillage_report/data_spillage_download_report/data_spillage_download_report'; +import type {ActionRow, PropertiesCardViewMetadata} from 'components/properties_card_view/properties_card_view'; import PropertiesCardView from 'components/properties_card_view/properties_card_view'; import {DataSpillagePropertyNames} from 'utils/constants'; -import './data_spillage_report.scss'; import DataSpillageFooter from './data_spillage_footer/data_spillage_footer'; import {getSyntheticPropertyFields, getSyntheticPropertyValues} from './synthetic_data'; +import './data_spillage_report.scss'; + // The order of fields to be displayed in the report, from top to bottom. const orderedFieldName = [ 'status', @@ -115,8 +117,7 @@ export function DataSpillageReport({post, isRHS}: Props) { fetchDeletedPost: true, channel, team, - generateFileDownloadUrl: - generateFileDownloadUrl(reportedPostId), + generateFileDownloadUrl: generateFileDownloadUrl(reportedPostId), }, reporting_comment: { placeholder: formatMessage({ @@ -157,24 +158,44 @@ export function DataSpillageReport({post, isRHS}: Props) { ); }, [isRHS, post, reportedPostId]); - const actionRow = useMemo(() => { - if (!reportedPost || !reportingUser) { - return null; + const actionRows = useMemo(() => { + if (!reportedPost) { + return []; } - let showActionRow; - if (!propertyFields || !propertyValues) { - showActionRow = true; - } else { - const status = propertyValues.find((value) => value.field_id === propertyFields.status.id)?.value as string | undefined; - showActionRow = reportedPost && reportingUser && status && (status === ContentFlaggingStatus.Pending || status === ContentFlaggingStatus.Assigned); + const rows: ActionRow[] = []; + + rows.push({ + label: ( + + ), + content: , + }); + + const statusFieldId = propertyFields.status?.id; + const status = statusFieldId ? (propertyValues.find((value) => value.field_id === statusFieldId)?.value as string | undefined) : undefined; + + if (reportingUser && (status === ContentFlaggingStatus.Pending || status === ContentFlaggingStatus.Assigned)) { + rows.push({ + label: ( + + ), + content: ( + + ), + }); } - return showActionRow ? ( - ) : null; + return rows; }, [propertyFields, propertyValues, reportedPost, reportingUser]); return ( @@ -189,7 +210,7 @@ export function DataSpillageReport({post, isRHS}: Props) { propertyValues={propertyValues} fieldOrder={orderedFieldName} shortModeFieldOrder={shortModeFieldOrder} - actionsRow={actionRow} + actionRows={actionRows} mode={mode} metadata={metadata} footer={footer} diff --git a/webapp/channels/src/components/properties_card_view/properties_card_view.test.tsx b/webapp/channels/src/components/properties_card_view/properties_card_view.test.tsx new file mode 100644 index 00000000000..ae16fe73b00 --- /dev/null +++ b/webapp/channels/src/components/properties_card_view/properties_card_view.test.tsx @@ -0,0 +1,99 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {screen} from '@testing-library/react'; +import React from 'react'; + +import type {NameMappedPropertyFields, PropertyField, PropertyValue} from '@mattermost/types/properties'; + +import {renderWithContext} from 'tests/react_testing_utils'; + +import PropertiesCardView from './properties_card_view'; +import type {ActionRow} from './properties_card_view'; + +describe('PropertiesCardView actionRows', () => { + const propertyFields: NameMappedPropertyFields = { + status: { + id: 'status_field_id', + group_id: 'group_id', + name: 'status', + type: 'text', + attrs: null, + target_id: '', + target_type: '', + create_at: 0, + update_at: 0, + delete_at: 0, + } as unknown as PropertyField, + }; + + const propertyValues: Array> = [ + { + id: 'value_id', + field_id: 'status_field_id', + value: 'Pending', + } as PropertyValue, + ]; + + const baseProps = { + title: 'Card Title', + propertyFields, + propertyValues, + fieldOrder: ['status'], + shortModeFieldOrder: ['status'], + }; + + const actionRows: ActionRow[] = [ + {label: 'Report', content:
{'r'}
, testId: 'report-row'}, + {label: 'Actions', content:
{'a'}
, testId: 'actions-row'}, + ]; + + test('renders action rows in full mode with labels and content', () => { + renderWithContext( + , + ); + + const reportRow = screen.getByTestId('report-row'); + expect(reportRow).toBeVisible(); + expect(reportRow).toHaveTextContent('Report'); + expect(screen.getByTestId('report-content')).toBeVisible(); + + const actionsRow = screen.getByTestId('actions-row'); + expect(actionsRow).toBeVisible(); + expect(actionsRow).toHaveTextContent('Actions'); + expect(screen.getByTestId('actions-content')).toBeVisible(); + }); + + test('does not render action rows in short mode', () => { + renderWithContext( + , + ); + + expect(screen.queryByTestId('report-row')).not.toBeInTheDocument(); + expect(screen.queryByTestId('actions-row')).not.toBeInTheDocument(); + }); + + test('skips action rows whose content is falsy', () => { + renderWithContext( + , + ); + + expect(screen.queryByTestId('empty-row')).not.toBeInTheDocument(); + expect(screen.getByTestId('report-row')).toBeVisible(); + }); +}); diff --git a/webapp/channels/src/components/properties_card_view/properties_card_view.tsx b/webapp/channels/src/components/properties_card_view/properties_card_view.tsx index 9c523eef636..0ff71be05ef 100644 --- a/webapp/channels/src/components/properties_card_view/properties_card_view.tsx +++ b/webapp/channels/src/components/properties_card_view/properties_card_view.tsx @@ -117,6 +117,12 @@ const fieldNameMessages = defineMessages({ }, }); +export type ActionRow = { + label: React.ReactNode; + content: React.ReactNode; + testId?: string; +}; + type Props = { title: React.ReactNode; propertyFields: NameMappedPropertyFields; @@ -124,12 +130,12 @@ type Props = { shortModeFieldOrder: Array; propertyValues: Array>; mode?: 'short' | 'full'; - actionsRow?: React.ReactNode; + actionRows?: ActionRow[]; metadata?: PropertiesCardViewMetadata; footer?: React.ReactNode; } -export default function PropertiesCardView({title, propertyFields, fieldOrder, shortModeFieldOrder, propertyValues, mode, actionsRow, metadata, footer}: Props) { +export default function PropertiesCardView({title, propertyFields, fieldOrder, shortModeFieldOrder, propertyValues, mode, actionRows, metadata, footer}: Props) { const orderedRows = useMemo(() => { const hasRequiredData = Object.keys(propertyFields).length > 0 && @@ -161,6 +167,22 @@ export default function PropertiesCardView({title, propertyFields, fieldOrder, s filter((row): row is OrderedRow => row !== null); }, [fieldOrder, mode, propertyFields, propertyValues, shortModeFieldOrder]); + const actionRowsMemo = useMemo(() => { + return actionRows?.map(({label, content, testId}, idx) => ( + content ? ( +
+
{label}
+
{content}
+
+ ) : null + )); + }, [actionRows]); + return (
-
- -
- -
- {actionsRow} -
-
- } - + {mode === 'full' && actionRowsMemo} {footer} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/body_main_action_text.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/body_main_action_text.tsx new file mode 100644 index 00000000000..008f24716ee --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/body_main_action_text.tsx @@ -0,0 +1,62 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; +import {useIntl} from 'react-intl'; + +import type {Post} from '@mattermost/types/posts'; +import type {UserProfile} from '@mattermost/types/users'; + +import AtMention from 'components/at_mention'; +import {useChannel} from 'components/common/hooks/useChannel'; +import {useUser} from 'components/common/hooks/useUser'; + +type Props = { + action: 'keep' | 'remove'; + flaggedPost: Post; + reportingUser: UserProfile; +}; + +export default function BodyMainActionText({ + action, + flaggedPost, + reportingUser, +}: Props) { + const {formatMessage} = useIntl(); + const flaggedPostAuthor = useUser(flaggedPost.user_id); + const flaggedPostChannel = useChannel(flaggedPost.channel_id); + + const values = { + flaggedPostChannel: flaggedPostChannel?.display_name, + reportingUser: ( + + ), + flaggedPostAuthor: ( + + ), + }; + + let body; + + if (action === 'remove') { + body = formatMessage( + { + id: 'keep_remove_quarantined_content_modal.action_remove.body', + defaultMessage: + 'You are about to remove a message authored by {flaggedPostAuthor} posted in the {flaggedPostChannel} channel and quarantined for review by {reportingUser}.', + }, + values, + ); + } else { + body = formatMessage( + { + id: 'keep_remove_quarantined_content_modal.action_keep.body', + defaultMessage: + 'You are about to keep a quarantined message authored by {flaggedPostAuthor} posted in the {flaggedPostChannel} channel and quarantined for review by {reportingUser}.', + }, + values, + ); + } + + return

{body}

; +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_body.scss b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_body.scss new file mode 100644 index 00000000000..ba4e7a3d952 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_body.scss @@ -0,0 +1,14 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +.KeepRemoveFlaggedMessageConfirmationModal .body { + .ErrorStepBody { + display: flex; + flex-direction: column; + gap: 8px; + + .errorRetryBtn { + width: min-content; + } + } +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_body.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_body.tsx new file mode 100644 index 00000000000..75ca9451ca2 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_body.tsx @@ -0,0 +1,74 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; +import {FormattedMessage, useIntl} from 'react-intl'; + +import type {ContentFlaggingConfig} from '@mattermost/types/content_flagging'; +import type {Post} from '@mattermost/types/posts'; +import type {UserProfile} from '@mattermost/types/users'; + +import FlaggedMessageBody from '../flagged_message_body'; +import ReportNotice from '../report_notice'; + +import './error_step_body.scss'; + +type BodyProps = { + action: 'keep' | 'remove'; + flaggedPost: Post; + reportingUser: UserProfile; + contentFlaggingConfig: ContentFlaggingConfig | undefined; + onRetry: () => void; +}; + +export default function ErrorStepBody({ + action, + flaggedPost, + reportingUser, + contentFlaggingConfig, + onRetry, +}: BodyProps) { + const {formatMessage} = useIntl(); + const tryAgainText = formatMessage({ + id: 'keep_remove_quarantined_content_modal.try_again.button_text', + defaultMessage: 'Try again', + }); + + return ( + <> + + } + title={ + + } + body={ +
+ + +
+ } + /> + + ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_footer.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_footer.tsx new file mode 100644 index 00000000000..49dd6a6dabd --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/error_step/error_step_footer.tsx @@ -0,0 +1,57 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import classNames from 'classnames'; +import React from 'react'; +import {useIntl} from 'react-intl'; + +type FooterProps = { + action: 'keep' | 'remove'; + onSkip: () => void; + onBack: () => void; +}; + +export default function ErrorStepFooter({action, onSkip, onBack}: FooterProps) { + const {formatMessage} = useIntl(); + + const skipText = formatMessage({id: 'keep_remove_quarantined_content_modal.skip_report_download.button_text', defaultMessage: 'Skip report download'}); + const removePermanentlyText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_remove.permanent_button_text', defaultMessage: 'Remove permanently'}); + const keepPermanentlyText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_keep.permanent_button_text', defaultMessage: 'Keep permanently'}); + const backText = formatMessage({id: 'keep_remove_quarantined_content_modal.back.button_text', defaultMessage: 'Back'}); + + const permanentText = action === 'remove' ? removePermanentlyText : keepPermanentlyText; + const permanentClass = action === 'remove' ? 'btn-danger' : 'btn-primary'; + + return ( +
+
+ +
+
+ + +
+
+ ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/flagged_message_body.scss b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/flagged_message_body.scss new file mode 100644 index 00000000000..c231f50bc78 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/flagged_message_body.scss @@ -0,0 +1,14 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +.KeepRemoveFlaggedMessageConfirmationModal .body { + .section.message_body { + p { + margin: 0 0 12px; + + &:last-child { + margin-bottom: 0; + } + } + } +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/flagged_message_body.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/flagged_message_body.tsx new file mode 100644 index 00000000000..32ff397b990 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/flagged_message_body.tsx @@ -0,0 +1,62 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React, {useMemo} from 'react'; +import {useIntl} from 'react-intl'; + +import type {ContentFlaggingConfig} from '@mattermost/types/content_flagging'; +import type {Post} from '@mattermost/types/posts'; +import type {UserProfile} from '@mattermost/types/users'; + +import BodyMainActionText from 'components/remove_flagged_message_confirmation_modal/body_main_action_text'; + +import './flagged_message_body.scss'; + +type Props = { + action: 'keep' | 'remove'; + flaggedPost: Post; + reportingUser: UserProfile; + contentFlaggingConfig: ContentFlaggingConfig | undefined; +}; + +export default function FlaggedMessageBody({action, flaggedPost, reportingUser, contentFlaggingConfig}: Props) { + const {formatMessage} = useIntl(); + + const subtext = useMemo(() => { + if (action === 'remove') { + if (contentFlaggingConfig?.notify_reporter_on_removal) { + return formatMessage({ + id: 'keep_remove_quarantined_content_modal.action_remove.subtext.notify_reporter', + defaultMessage: 'If you confirm, the message will be removed from the channel and a notification will be sent to the reporter. This action cannot be reverted.', + }); + } + return formatMessage({ + id: 'keep_remove_quarantined_content_modal.action_remove.subtext.no_notify_reporter', + defaultMessage: 'If you confirm, the message will be removed from the channel. This action cannot be reverted.', + }); + } else if (contentFlaggingConfig?.notify_reporter_on_dismissal) { + return formatMessage({ + id: 'keep_remove_quarantined_content_modal.action_keep.subtext.notify_reporter', + defaultMessage: 'If you confirm, the message will be visible to all channel members and a notification will be sent to the reporter.', + }); + } + return formatMessage({ + id: 'keep_remove_quarantined_content_modal.action_keep.subtext.no_notify_reporter', + defaultMessage: 'If you confirm, the message will be visible to all channel members.', + }); + }, [action, contentFlaggingConfig?.notify_reporter_on_dismissal, contentFlaggingConfig?.notify_reporter_on_removal, formatMessage]); + + return ( +
+ +

{subtext}

+
+ ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_body.scss b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_body.scss new file mode 100644 index 00000000000..1fa0ed7ca76 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_body.scss @@ -0,0 +1,30 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +.KeepRemoveFlaggedMessageConfirmationModal .body { + .section { + &.comment_section { + display: flex; + flex-direction: column; + gap: 8px; + } + + .section_title { + color: var(--center-channel-color); + font-size: 14px; + font-weight: 600; + } + } + + button#PreviewInputTextButton { + position: absolute; + z-index: 2; + top: 8px; + right: 8px; + } + + textarea#RemoveFlaggedMessageConfirmationModal__comment { + min-height: 90px !important; + max-height: 400px; + } +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_body.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_body.tsx new file mode 100644 index 00000000000..95c4d0aeffe --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_body.tsx @@ -0,0 +1,83 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; +import {useIntl} from 'react-intl'; + +import type {ContentFlaggingConfig} from '@mattermost/types/content_flagging'; +import type {Post} from '@mattermost/types/posts'; +import type {UserProfile} from '@mattermost/types/users'; + +import type {TextboxElement} from 'components/textbox'; +import AdvancedTextbox from 'components/widgets/advanced_textbox/advanced_textbox'; + +import FlaggedMessageBody from '../flagged_message_body'; + +import './form_step_body.scss'; + +type BodyProps = { + action: 'keep' | 'remove'; + flaggedPost: Post; + reportingUser: UserProfile; + contentFlaggingConfig: ContentFlaggingConfig | undefined; + comment: string; + commentError: string; + showCommentPreview: boolean; + onCommentChange: (e: React.ChangeEvent) => void; + onToggleCommentPreview: () => void; +}; + +export function FormStepBody({ + action, + flaggedPost, + reportingUser, + contentFlaggingConfig, + comment, + commentError, + showCommentPreview, + onCommentChange, + onToggleCommentPreview, +}: BodyProps) { + const {formatMessage} = useIntl(); + + const requiredTitle = formatMessage({id: 'remove_flag_post_confirm_modal.required_comment.title', defaultMessage: 'Comment (required)'}); + const optionalTitle = formatMessage({id: 'remove_flag_post_confirm_modal.optional_comment.title', defaultMessage: 'Comment (optional)'}); + const sectionTitle = contentFlaggingConfig?.reviewer_comment_required ? requiredTitle : optionalTitle; + + const commentPlaceholder = formatMessage({id: 'keep_remove_quarantined_content_modal.comment.placeholder', defaultMessage: 'Add your comment here'}); + + return ( + <> + + +
+
+ {sectionTitle} +
+ + {}} + hasError={false} + errorMessage={commentError} + maxLength={1000} + /> +
+ + ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_footer.scss b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_footer.scss new file mode 100644 index 00000000000..ae40347f1e3 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_footer.scss @@ -0,0 +1,21 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +.KeepRemoveFlaggedMessageConfirmationModal { + .download_report_checkbox { + display: flex; + align-items: center; + margin: 0; + cursor: pointer; + font-size: 16px; + font-weight: 400; + gap: 10px; + + input[type='checkbox'] { + width: 20px; + height: 20px; + margin: 0; + cursor: pointer; + } + } +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_footer.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_footer.tsx new file mode 100644 index 00000000000..45fce09c172 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/form_step/form_step_footer.tsx @@ -0,0 +1,82 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import classNames from 'classnames'; +import React from 'react'; +import {FormattedMessage, useIntl} from 'react-intl'; + +import './form_step_footer.scss'; + +type FooterProps = { + action: 'keep' | 'remove'; + downloadReport: boolean; + submitting: boolean; + onToggleDownloadReport: (e: React.ChangeEvent) => void; + onCancel: () => void; + onPrimary: () => void; +}; + +export function FormStepFooter({ + action, + downloadReport, + submitting, + onToggleDownloadReport, + onCancel, + onPrimary, +}: FooterProps) { + const {formatMessage} = useIntl(); + + const cancelText = formatMessage({id: 'generic_modal.cancel', defaultMessage: 'Cancel'}); + const continueText = formatMessage({id: 'keep_remove_quarantined_content_modal.continue.button_text', defaultMessage: 'Continue'}); + const removeText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_remove.button_text', defaultMessage: 'Remove message'}); + const keepText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_keep.button_text', defaultMessage: 'Keep message'}); + + const actionText = action === 'remove' ? removeText : keepText; + const uncheckedClass = action === 'remove' ? 'btn-danger' : 'btn-primary'; + + const primaryText = downloadReport ? continueText : actionText; + const primaryClass = downloadReport ? 'btn-primary' : uncheckedClass; + + return ( +
+
+ +
+
+ + +
+
+ ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generated_step/generated_step_body.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generated_step/generated_step_body.tsx new file mode 100644 index 00000000000..9f9daa9a0bb --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generated_step/generated_step_body.tsx @@ -0,0 +1,61 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; +import {FormattedMessage} from 'react-intl'; + +import type {ContentFlaggingConfig} from '@mattermost/types/content_flagging'; +import type {Post} from '@mattermost/types/posts'; +import type {UserProfile} from '@mattermost/types/users'; + +import FlaggedMessageBody from '../flagged_message_body'; +import ReportNotice from '../report_notice'; + +type BodyProps = { + action: 'keep' | 'remove'; + flaggedPost: Post; + reportingUser: UserProfile; + contentFlaggingConfig: ContentFlaggingConfig | undefined; +}; + +export default function GeneratedStepBody({ + action, + flaggedPost, + reportingUser, + contentFlaggingConfig, +}: BodyProps) { + return ( + <> + + } + title={ + + } + body={ + action === 'remove' ? ( + + ) : ( + + ) + } + /> + + ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generated_step/generated_step_footer.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generated_step/generated_step_footer.tsx new file mode 100644 index 00000000000..379e3221395 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generated_step/generated_step_footer.tsx @@ -0,0 +1,66 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import classNames from 'classnames'; +import React from 'react'; +import {useIntl} from 'react-intl'; + +type FooterProps = { + action: 'keep' | 'remove'; + submitting: boolean; + onDownloadAgain: () => void; + onBack: () => void; + onPermanent: () => void; +}; + +export default function GeneratedStepFooter({ + action, + submitting, + onDownloadAgain, + onBack, + onPermanent, +}: FooterProps) { + const {formatMessage} = useIntl(); + + const downloadAgainText = formatMessage({id: 'keep_remove_quarantined_content_modal.download_again.button_text', defaultMessage: 'Download again'}); + const backText = formatMessage({id: 'keep_remove_quarantined_content_modal.back.button_text', defaultMessage: 'Back'}); + const removePermanentlyText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_remove.permanent_button_text', defaultMessage: 'Remove permanently'}); + const keepPermanentlyText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_keep.permanent_button_text', defaultMessage: 'Keep permanently'}); + + const permanentText = action === 'remove' ? removePermanentlyText : keepPermanentlyText; + const permanentClass = action === 'remove' ? 'btn-danger' : 'btn-primary'; + + return ( +
+
+ +
+
+ + +
+
+ ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generating_step/generating_step_body.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generating_step/generating_step_body.tsx new file mode 100644 index 00000000000..5fef111efbd --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generating_step/generating_step_body.tsx @@ -0,0 +1,63 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; +import {FormattedMessage} from 'react-intl'; + +import type {ContentFlaggingConfig} from '@mattermost/types/content_flagging'; +import type {Post} from '@mattermost/types/posts'; +import type {UserProfile} from '@mattermost/types/users'; + +import LoadingSpinner from 'components/widgets/loading/loading_spinner'; + +import FlaggedMessageBody from '../flagged_message_body'; +import ReportNotice from '../report_notice'; + +type BodyProps = { + action: 'keep' | 'remove'; + flaggedPost: Post; + reportingUser: UserProfile; + contentFlaggingConfig: ContentFlaggingConfig | undefined; +}; + +export function GeneratingStepBody({ + action, + flaggedPost, + reportingUser, + contentFlaggingConfig, +}: BodyProps) { + return ( + <> + + } + title={ + + } + body={ + action === 'remove' ? ( + + ) : ( + + ) + } + /> + + ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generating_step/generating_step_footer.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generating_step/generating_step_footer.tsx new file mode 100644 index 00000000000..e3583df9503 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/generating_step/generating_step_footer.tsx @@ -0,0 +1,58 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import classNames from 'classnames'; +import React from 'react'; +import {useIntl} from 'react-intl'; + +type FooterProps = { + action: 'keep' | 'remove'; + onSkip: () => void; + onBack: () => void; +}; + +export function GeneratingStepFooter({action, onSkip, onBack}: FooterProps) { + const {formatMessage} = useIntl(); + + const skipText = formatMessage({id: 'keep_remove_quarantined_content_modal.skip_report_download.button_text', defaultMessage: 'Skip report download'}); + const backText = formatMessage({id: 'keep_remove_quarantined_content_modal.back.button_text', defaultMessage: 'Back'}); + const removePermanentlyText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_remove.permanent_button_text', defaultMessage: 'Remove permanently'}); + const keepPermanentlyText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_keep.permanent_button_text', defaultMessage: 'Keep permanently'}); + + const permanentText = + action === 'remove' ? removePermanentlyText : keepPermanentlyText; + const permanentClass = action === 'remove' ? 'btn-danger' : 'btn-primary'; + + return ( +
+
+ +
+
+ + +
+
+ ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.scss b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.scss index 789c131481c..e48dbe5cd64 100644 --- a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.scss +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.scss @@ -2,41 +2,22 @@ // See LICENSE.txt for license information. .KeepRemoveFlaggedMessageConfirmationModal { + width: 704px; + .modal-content { width: 704px; } + .modal-footer { + // Allow the custom footer row to span the full width and use space-between layout. + justify-content: stretch; + } + .body { display: flex; flex-direction: column; gap: 24px; - .section{ - &.comment_section { - display: flex; - flex-direction: column; - gap: 8px; - } - - .section_title { - color: var(--center-channel-color); - font-size: 14px; - font-weight: 600; - } - } - - button#PreviewInputTextButton { - position: absolute; - z-index: 2; - top: 8px; - right: 8px; - } - - textarea#RemoveFlaggedMessageConfirmationModal__comment { - min-height: 90px !important; - max-height: 400px; - } - .request_error { display: flex; width: 90%; @@ -45,11 +26,24 @@ font-size: 12px; } } -} - - - - + .ModalFooterRow { + display: flex; + width: 100%; + align-items: center; + justify-content: space-between; + gap: 8px; + &__left, + &__right { + display: flex; + align-items: center; + gap: 8px; + } + .skipReportBtn, + .skipReportBtn:hover { + color: var(--error-text); + } + } +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.test.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.test.tsx index 7d927db3a82..485c3a2407a 100644 --- a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.test.tsx +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.test.tsx @@ -52,6 +52,17 @@ describe('KeepRemoveFlaggedMessageConfirmationModal', () => { const onExited = jest.fn(); + let originalCreateObjectURL: typeof URL.createObjectURL; + let originalRevokeObjectURL: typeof URL.revokeObjectURL; + + const mockReportSuccess = () => { + Client4.generateFlaggedPostReport = jest.fn().mockResolvedValue(new Blob(['report'], {type: 'application/zip'})); + }; + + const mockReportFailure = () => { + Client4.generateFlaggedPostReport = jest.fn().mockRejectedValue(new Error('boom')); + }; + beforeEach(() => { jest.clearAllMocks(); @@ -63,10 +74,22 @@ describe('KeepRemoveFlaggedMessageConfirmationModal', () => { Client4.removeFlaggedPost = jest.fn().mockResolvedValue({}); Client4.keepFlaggedPost = jest.fn().mockResolvedValue({}); + mockReportSuccess(); + + originalCreateObjectURL = URL.createObjectURL; + originalRevokeObjectURL = URL.revokeObjectURL; + URL.createObjectURL = jest.fn().mockReturnValue('blob:mock-url'); + URL.revokeObjectURL = jest.fn(); + // eslint-disable-next-line no-console console.error = jest.fn(); }); + afterEach(() => { + URL.createObjectURL = originalCreateObjectURL; + URL.revokeObjectURL = originalRevokeObjectURL; + }); + describe('remove action', () => { test('should render modal with remove action content', () => { renderWithContext( @@ -80,7 +103,9 @@ describe('KeepRemoveFlaggedMessageConfirmationModal', () => { expect(screen.getByTestId('keep-remove-flagged-message-confirmation-modal')).toBeVisible(); expect(screen.getByRole('heading', {name: 'Remove message from channel'})).toBeVisible(); - expect(screen.getByRole('button', {name: 'Remove message'})).toBeVisible(); + + // Default form step shows the "Continue" primary button (download checkbox is on by default) + expect(screen.getByRole('button', {name: 'Continue'})).toBeVisible(); }); test('should show notification subtext when notify_reporter_on_removal is true', () => { @@ -118,7 +143,7 @@ describe('KeepRemoveFlaggedMessageConfirmationModal', () => { expect(subtext).toHaveTextContent(/the message will be removed from the channel. This action cannot be reverted./); }); - test('should call Client4.removeFlaggedPost on confirm', async () => { + test('should call Client4.removeFlaggedPost via download flow on Remove permanently', async () => { renderWithContext( { />, ); - const confirmButton = screen.getByRole('button', {name: 'Remove message'}); - await userEvent.click(confirmButton); + // Form step with checkbox checked → click Continue triggers report fetch + await userEvent.click(screen.getByRole('button', {name: 'Continue'})); + + await waitFor(() => { + expect(Client4.generateFlaggedPostReport).toHaveBeenCalledWith( + flaggedPost.id, + '', + 'remove', + expect.any(AbortSignal), + ); + }); + + await waitFor(() => { + expect(screen.getByTestId('generated-section')).toBeVisible(); + }); + + await userEvent.click(screen.getByRole('button', {name: 'Remove permanently'})); await waitFor(() => { expect(Client4.removeFlaggedPost).toHaveBeenCalledWith(flaggedPost.id, ''); }); expect(onExited).toHaveBeenCalled(); }); + + test('should go through skip-confirm when checkbox is unchecked', async () => { + renderWithContext( + , + ); + + await userEvent.click(screen.getByTestId('download-report-checkbox')); + + // Button label changes to "Remove message" when checkbox unchecked + await userEvent.click(screen.getByRole('button', {name: 'Remove message'})); + + await waitFor(() => { + expect(screen.getByTestId('skip-confirm-body')).toBeVisible(); + }); + + await userEvent.click(screen.getByRole('button', {name: 'Remove without report'})); + + await waitFor(() => { + expect(Client4.removeFlaggedPost).toHaveBeenCalledWith(flaggedPost.id, ''); + }); + expect(Client4.generateFlaggedPostReport).not.toHaveBeenCalled(); + }); + + test('should show error step when report generation fails and allow retry', async () => { + mockReportFailure(); + + renderWithContext( + , + ); + + await userEvent.click(screen.getByRole('button', {name: 'Continue'})); + + await waitFor(() => { + expect(screen.getByTestId('error-section')).toBeVisible(); + }); + + // Switch to success and retry + mockReportSuccess(); + await userEvent.click(screen.getByTestId('error-retry-button')); + + await waitFor(() => { + expect(screen.getByTestId('generated-section')).toBeVisible(); + }); + }); }); describe('keep action', () => { @@ -150,7 +244,7 @@ describe('KeepRemoveFlaggedMessageConfirmationModal', () => { ); expect(screen.getByTestId('keep-remove-flagged-message-confirmation-modal')).toBeVisible(); - expect(screen.getByRole('button', {name: 'Keep message'})).toBeVisible(); + expect(screen.getByRole('button', {name: 'Continue'})).toBeVisible(); }); test('should show notification subtext when notify_reporter_on_dismissal is true', () => { @@ -188,7 +282,7 @@ describe('KeepRemoveFlaggedMessageConfirmationModal', () => { expect(subtext).toHaveTextContent(/the message will be visible to all channel members./); }); - test('should call Client4.keepFlaggedPost on confirm', async () => { + test('should call Client4.keepFlaggedPost via download flow on Keep permanently', async () => { renderWithContext( { />, ); - const confirmButton = screen.getByRole('button', {name: 'Keep message'}); - await userEvent.click(confirmButton); + await userEvent.click(screen.getByRole('button', {name: 'Continue'})); + + await waitFor(() => { + expect(screen.getByTestId('generated-section')).toBeVisible(); + }); + + await userEvent.click(screen.getByRole('button', {name: 'Keep permanently'})); await waitFor(() => { expect(Client4.keepFlaggedPost).toHaveBeenCalledWith(flaggedPost.id, ''); }); expect(onExited).toHaveBeenCalled(); }); + + test('should call Client4.keepFlaggedPost directly without skip-confirm when checkbox is unchecked', async () => { + renderWithContext( + , + ); + + await userEvent.click(screen.getByTestId('download-report-checkbox')); + await userEvent.click(screen.getByRole('button', {name: 'Keep message'})); + + await waitFor(() => { + expect(Client4.keepFlaggedPost).toHaveBeenCalledWith(flaggedPost.id, ''); + }); + expect(screen.queryByTestId('skip-confirm-body')).not.toBeInTheDocument(); + expect(Client4.generateFlaggedPostReport).not.toHaveBeenCalled(); + }); + + test('skip from generating step calls keepFlaggedPost directly without skip-confirm', async () => { + Client4.generateFlaggedPostReport = jest.fn().mockReturnValue(new Promise(() => {})); + + renderWithContext( + , + ); + + await userEvent.click(screen.getByRole('button', {name: 'Continue'})); + + await waitFor(() => { + expect(screen.getByTestId('generating-section')).toBeVisible(); + }); + + await userEvent.click(screen.getByTestId('generating-skip-button')); + + await waitFor(() => { + expect(Client4.keepFlaggedPost).toHaveBeenCalledWith(flaggedPost.id, ''); + }); + expect(screen.queryByTestId('skip-confirm-body')).not.toBeInTheDocument(); + }); }); describe('comment section', () => { @@ -259,17 +404,119 @@ describe('KeepRemoveFlaggedMessageConfirmationModal', () => { />, ); - const confirmButton = screen.getByRole('button', {name: 'Remove message'}); - await userEvent.click(confirmButton); + await userEvent.click(screen.getByRole('button', {name: 'Continue'})); await waitFor(() => { expect(screen.getByText('Please add a comment.')).toBeVisible(); }); + expect(Client4.generateFlaggedPostReport).not.toHaveBeenCalled(); expect(Client4.removeFlaggedPost).not.toHaveBeenCalled(); expect(onExited).not.toHaveBeenCalled(); }); }); + describe('step transitions', () => { + test('should pass typed comment to action API', async () => { + renderWithContext( + , + ); + + await userEvent.type(screen.getByPlaceholderText('Add your comment here'), 'looks fine'); + await userEvent.click(screen.getByRole('button', {name: 'Continue'})); + + await waitFor(() => { + expect(screen.getByTestId('generated-section')).toBeVisible(); + }); + await userEvent.click(screen.getByRole('button', {name: 'Remove permanently'})); + + await waitFor(() => { + expect(Client4.removeFlaggedPost).toHaveBeenCalledWith(flaggedPost.id, 'looks fine'); + }); + }); + + test('clicking "Download again" on generated step retriggers report fetch', async () => { + renderWithContext( + , + ); + + await userEvent.click(screen.getByRole('button', {name: 'Continue'})); + await waitFor(() => { + expect(screen.getByTestId('generated-section')).toBeVisible(); + }); + const initialCallCount = (Client4.generateFlaggedPostReport as jest.Mock).mock.calls.length; + + await userEvent.click(screen.getByTestId('generated-download-again-button')); + + await waitFor(() => { + expect((Client4.generateFlaggedPostReport as jest.Mock).mock.calls.length).toBeGreaterThan(initialCallCount); + }); + await waitFor(() => { + expect(screen.getByTestId('generated-section')).toBeVisible(); + }); + }); + + test('skip from generating step routes to skip-confirm', async () => { + // Hold the request open so we can interact with the generating footer + Client4.generateFlaggedPostReport = jest.fn().mockReturnValue(new Promise(() => {})); + + renderWithContext( + , + ); + + await userEvent.click(screen.getByRole('button', {name: 'Continue'})); + + await waitFor(() => { + expect(screen.getByTestId('generating-section')).toBeVisible(); + }); + + await userEvent.click(screen.getByTestId('generating-skip-button')); + + await waitFor(() => { + expect(screen.getByTestId('skip-confirm-body')).toBeVisible(); + }); + expect(screen.queryByTestId('generated-section')).not.toBeInTheDocument(); + }); + + test('back from skip-confirm returns to form step', async () => { + renderWithContext( + , + ); + + await userEvent.click(screen.getByTestId('download-report-checkbox')); + await userEvent.click(screen.getByRole('button', {name: 'Remove message'})); + + await waitFor(() => { + expect(screen.getByTestId('skip-confirm-body')).toBeVisible(); + }); + + await userEvent.click(screen.getByTestId('skip-confirm-back-button')); + + await waitFor(() => { + expect(screen.getByRole('button', {name: 'Remove message'})).toBeVisible(); + }); + }); + }); + describe('error handling', () => { test('should show request error when API call fails', async () => { const errorMessage = 'Failed to remove flagged post'; @@ -284,8 +531,15 @@ describe('KeepRemoveFlaggedMessageConfirmationModal', () => { />, ); - const confirmButton = screen.getByRole('button', {name: 'Remove message'}); - await userEvent.click(confirmButton); + // Skip download path so we go directly to API call + await userEvent.click(screen.getByTestId('download-report-checkbox')); + await userEvent.click(screen.getByRole('button', {name: 'Remove message'})); + + await waitFor(() => { + expect(screen.getByTestId('skip-confirm-body')).toBeVisible(); + }); + + await userEvent.click(screen.getByRole('button', {name: 'Remove without report'})); await waitFor(() => { const errorElement = screen.getByTestId( diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.tsx index c633a83c529..e746f2fb1aa 100644 --- a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.tsx +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/remove_flagged_message_confirmation_modal.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React, {useCallback} from 'react'; +import React, {useCallback, useEffect, useRef, useState} from 'react'; import {useIntl} from 'react-intl'; import {GenericModal} from '@mattermost/components'; @@ -11,16 +11,22 @@ import type {UserProfile} from '@mattermost/types/users'; import {Client4} from 'mattermost-redux/client'; -import AtMention from 'components/at_mention'; import {useChannel} from 'components/common/hooks/useChannel'; import {useContentFlaggingConfig} from 'components/common/hooks/useContentFlaggingFields'; -import {useUser} from 'components/common/hooks/useUser'; import type {TextboxElement} from 'components/textbox'; -import AdvancedTextbox from 'components/widgets/advanced_textbox/advanced_textbox'; -import './remove_flagged_message_confirmation_modal.scss'; +import ErrorStepBody from './error_step/error_step_body'; +import ErrorStepFooter from './error_step/error_step_footer'; +import {FormStepBody} from './form_step/form_step_body'; +import {FormStepFooter} from './form_step/form_step_footer'; +import GeneratedStepBody from './generated_step/generated_step_body'; +import GeneratedStepFooter from './generated_step/generated_step_footer'; +import {GeneratingStepBody} from './generating_step/generating_step_body'; +import {GeneratingStepFooter} from './generating_step/generating_step_footer'; +import {SkipConfirmStepBody} from './skip_confirm_step/skip_confirm_step_body'; +import {SkipConfirmStepFooter} from './skip_confirm_step/skip_confirm_step_footer'; -const noop = () => {}; +import './remove_flagged_message_confirmation_modal.scss'; type Props = { action: 'keep' | 'remove'; @@ -29,18 +35,28 @@ type Props = { reportingUser: UserProfile; } +type Step = 'form' | 'skip_confirm' | 'generating' | 'generated' | 'error'; + export default function KeepRemoveFlaggedMessageConfirmationModal({action, onExited, flaggedPost, reportingUser}: Props) { const {formatMessage} = useIntl(); - const flaggedPostAuthor = useUser(flaggedPost.user_id); const flaggedPostChannel = useChannel(flaggedPost.channel_id); const contentFlaggingConfig = useContentFlaggingConfig(flaggedPostChannel?.team_id || ''); - const [comment, setComment] = React.useState(''); - const [commentError, setCommentError] = React.useState(''); - const [requestError, setRequestError] = React.useState(''); - const [submitting, setSubmitting] = React.useState(false); - const [showCommentPreview, setShowCommentPreview] = React.useState(false); + const [comment, setComment] = useState(''); + const [commentError, setCommentError] = useState(''); + const [requestError, setRequestError] = useState(''); + const [submitting, setSubmitting] = useState(false); + const [showCommentPreview, setShowCommentPreview] = useState(false); + const [downloadReport, setDownloadReport] = useState(true); + const [step, setStep] = useState('form'); + + const abortControllerRef = useRef(null); + + const handleClose = useCallback(() => { + abortControllerRef.current?.abort(); + onExited(); + }, [onExited]); const handleCommentChange = useCallback((e: React.ChangeEvent) => { setComment(e.target.value); @@ -56,104 +72,26 @@ export default function KeepRemoveFlaggedMessageConfirmationModal({action, onExi setShowCommentPreview((prev) => !prev); }, []); - const removeActionLabel = formatMessage({id: 'keep_remove_quarantined_content_modal.action_remove.title', defaultMessage: 'Remove message from channel'}); - const keepActionLabel = formatMessage({id: 'keep_remove_quarantined_content_modal.action_keep.title', defaultMessage: 'Keep message'}); - - const removeActionBody = formatMessage({ - id: 'keep_remove_quarantined_content_modal.action_remove.body', - defaultMessage: 'You are about to remove a message authored by {flaggedPostAuthor} posted in the {flaggedPostChannel} channel and quarantined for review by {reportingUser}.', - }, { - flaggedPostChannel: flaggedPostChannel?.display_name, - reportingUser: , - flaggedPostAuthor: , - }); - const keepActionBody = formatMessage({ - id: 'keep_remove_quarantined_content_modal.action_keep.body', - defaultMessage: 'You are about to keep a quarantined message authored by {flaggedPostAuthor} posted in the {flaggedPostChannel} channel and quarantined for review by {reportingUser}.', - }, { - flaggedPostChannel: flaggedPostChannel?.display_name, - reportingUser: , - flaggedPostAuthor: , - }); - - const removeActionBodySubTextReporterNotification = formatMessage({ - id: 'keep_remove_quarantined_content_modal.action_remove.subtext.notify_reporter', - defaultMessage: 'If you confirm, the message will be removed from the channel and a notification will be sent to the reporter. This action cannot be reverted.', - }); - const removeActionBodySubTextNoReporterNotification = formatMessage({ - id: 'keep_remove_quarantined_content_modal.action_remove.subtext.no_notify_reporter', - defaultMessage: 'If you confirm, the message will be removed from the channel. This action cannot be reverted.', - }); - - const keepActionBodySubTextReporterNotification = formatMessage({ - id: 'keep_remove_quarantined_content_modal.action_keep.subtext.notify_reporter', - defaultMessage: 'If you confirm, the message will be visible to all channel members and a notification will be sent to the reporter.', - }); - const keepActionBodySubTextNoReporterNotification = formatMessage({ - id: 'keep_remove_quarantined_content_modal.action_keep.subtext.no_notify_reporter', - defaultMessage: 'If you confirm, the message will be visible to all channel members.', - }); - - const requiredCommentSectionTitle = formatMessage({id: 'remove_flag_post_confirm_modal.required_comment.title', defaultMessage: 'Comment (required)'}); - const optionalCommentSectionTitle = formatMessage({id: 'remove_flag_post_confirm_modal.optional_comment.title', defaultMessage: 'Comment (optional)'}); - - const commentPlaceholder = formatMessage({id: 'keep_remove_quarantined_content_modal.comment.placeholder', defaultMessage: 'Add your comment here'}); - const removeMessageButtonText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_remove.button_text', defaultMessage: 'Remove message'}); - const keepMessageButtonText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_keep.button_text', defaultMessage: 'Keep message'}); - - let label; - let subtext; - let body; - let buttonText; - let confirmButtonVariant; - - if (action === 'remove') { - label = removeActionLabel; - body = removeActionBody; - buttonText = removeMessageButtonText; - confirmButtonVariant = 'destructive' as const; - - if (contentFlaggingConfig?.notify_reporter_on_removal) { - subtext = removeActionBodySubTextReporterNotification; - } else { - subtext = removeActionBodySubTextNoReporterNotification; - } - } else { - label = keepActionLabel; - body = keepActionBody; - buttonText = keepMessageButtonText; - - if (contentFlaggingConfig?.notify_reporter_on_dismissal) { - subtext = keepActionBodySubTextReporterNotification; - } else { - subtext = keepActionBodySubTextNoReporterNotification; - } - } + const handleToggleDownloadReport = useCallback((e: React.ChangeEvent) => { + setDownloadReport(e.target.checked); + }, []); const validateForm = useCallback(() => { - let hasErrors = false; - if (contentFlaggingConfig?.reviewer_comment_required && comment.trim() === '') { setCommentError(formatMessage({id: 'keep_remove_quarantined_content_modal.comment_required.error', defaultMessage: 'Please add a comment.'})); - hasErrors = true; - } else { - setCommentError(''); + return true; } - - return hasErrors; + setCommentError(''); + return false; }, [comment, contentFlaggingConfig?.reviewer_comment_required, formatMessage]); - const handleConfirm = useCallback(async () => { - const hasError = validateForm(); - if (hasError) { - return; - } - + const callActionAPI = useCallback(async () => { const actionFunc = action === 'remove' ? Client4.removeFlaggedPost : Client4.keepFlaggedPost; try { setSubmitting(true); + setRequestError(''); await actionFunc(flaggedPost.id, comment); - onExited(); + handleClose(); } catch (error) { // eslint-disable-next-line no-console console.error(error); @@ -161,7 +99,181 @@ export default function KeepRemoveFlaggedMessageConfirmationModal({action, onExi } finally { setSubmitting(false); } - }, [action, comment, flaggedPost.id, onExited, validateForm]); + }, [action, comment, flaggedPost.id, handleClose]); + + const handleFormPrimary = useCallback(() => { + if (validateForm()) { + return; + } + setRequestError(''); + if (downloadReport) { + setStep('generating'); + } else if (action === 'keep') { + callActionAPI(); + } else { + setStep('skip_confirm'); + } + }, [validateForm, downloadReport, action, callActionAPI]); + + const handleSkipConfirmBack = useCallback(() => { + setRequestError(''); + setStep('form'); + }, []); + + const handleSkipFromGenerating = useCallback(() => { + abortControllerRef.current?.abort(); + setRequestError(''); + if (action === 'keep') { + callActionAPI(); + } else { + setStep('skip_confirm'); + } + }, [action, callActionAPI]); + + const handleBackToForm = useCallback(() => { + abortControllerRef.current?.abort(); + setRequestError(''); + setStep('form'); + }, []); + + const handleRetryGeneration = useCallback(() => { + setRequestError(''); + setStep('generating'); + }, []); + + useEffect(() => { + if (step !== 'generating') { + return undefined; + } + + const controller = new AbortController(); + abortControllerRef.current = controller; + + (async () => { + try { + const blob = await Client4.generateFlaggedPostReport(flaggedPost.id, comment, action, controller.signal); + if (controller.signal.aborted) { + return; + } + + const downloadUrl = URL.createObjectURL(blob); + const a = document.createElement('a'); + a.href = downloadUrl; + a.download = `flagged-post-${flaggedPost.id}-${Date.now()}.zip`; + document.body.appendChild(a); + a.click(); + a.remove(); + URL.revokeObjectURL(downloadUrl); + + setStep('generated'); + } catch (err) { + if (controller.signal.aborted) { + return; + } + + // eslint-disable-next-line no-console + console.error(err); + setStep('error'); + } + })(); + + return () => { + controller.abort(); + }; + }, [step, flaggedPost.id, comment, action]); + + const removeLabel = formatMessage({id: 'keep_remove_quarantined_content_modal.action_remove.title', defaultMessage: 'Remove message from channel'}); + const keepLabel = formatMessage({id: 'keep_remove_quarantined_content_modal.action_keep.title', defaultMessage: 'Keep message'}); + + const removeWithoutReportLabel = formatMessage({id: 'keep_remove_quarantined_content_modal.action_remove_without_report.title', defaultMessage: 'Remove without report?'}); + + const bodyContentProps = { + action, + flaggedPost, + reportingUser, + contentFlaggingConfig, + }; + + let label = action === 'remove' ? removeLabel : keepLabel; + let modalBody: React.ReactNode = null; + let footer: React.ReactNode = null; + + switch (step) { + case 'form': + modalBody = ( + + ); + footer = ( + + ); + break; + case 'skip_confirm': + label = removeWithoutReportLabel; + modalBody = ( + ); + footer = ( + + ); + break; + case 'generating': + modalBody = ; + footer = ( + + ); + break; + case 'generated': + modalBody = ; + footer = ( + + ); + break; + case 'error': + modalBody = ( + + ); + footer = ( + + ); + break; + } return (
-
- {body} -
-
- {subtext} -
- -
-
- {contentFlaggingConfig?.reviewer_comment_required ? requiredCommentSectionTitle : optionalCommentSectionTitle} -
- - {}} - hasError={false} - errorMessage={commentError} - maxLength={1000} - /> -
- {requestError && + {modalBody} + {requestError && (
{requestError}
- } + )}
); diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/report_notice.scss b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/report_notice.scss new file mode 100644 index 00000000000..e888ae03d50 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/report_notice.scss @@ -0,0 +1,76 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +.KeepRemoveFlaggedMessageConfirmationModal { + .ReportNotice { + display: flex; + align-items: flex-start; + padding: 16px; + border: 1px solid; + border-radius: 4px; + gap: 12px; + + &__icon { + display: flex; + width: 20px; + height: 20px; + flex-shrink: 0; + align-items: center; + justify-content: center; + + .icon { + font-size: 20px; + line-height: 1; + } + } + + &__body { + display: flex; + min-width: 0; + flex: 1 1 0; + flex-direction: column; + gap: 8px; + } + + &__title { + color: var(--center-channel-color); + font-size: 14px; + font-weight: 600; + line-height: 20px; + } + + &__text { + color: var(--center-channel-color); + font-size: 14px; + font-weight: 400; + line-height: 20px; + } + + &--info { + border-color: rgba(var(--sidebar-text-active-border-rgb), 0.16); + background: rgba(var(--button-bg-rgb), 0.04); + + .ReportNotice__icon { + color: var(--button-bg); + } + } + + &--success { + border-color: rgba(var(--online-indicator-rgb), 0.16); + background: rgba(var(--online-indicator-rgb), 0.08); + + .ReportNotice__icon { + color: var(--online-indicator); + } + } + + &--warning { + border-color: rgba(var(--away-indicator-rgb), 0.16); + background: rgba(var(--away-indicator-rgb), 0.08); + + .ReportNotice__icon { + color: var(--away-indicator); + } + } + } +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/report_notice.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/report_notice.tsx new file mode 100644 index 00000000000..97617dfe6e7 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/report_notice.tsx @@ -0,0 +1,30 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import classNames from 'classnames'; +import React from 'react'; + +import './report_notice.scss'; + +type Props = { + variant: 'info' | 'success' | 'warning'; + icon: React.ReactNode; + title: React.ReactNode; + body: React.ReactNode; + testId?: string; +}; + +export default function ReportNotice({variant, icon, title, body, testId}: Props) { + return ( +
+
{icon}
+
+
{title}
+
{body}
+
+
+ ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_body.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_body.tsx new file mode 100644 index 00000000000..0684b0c3612 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_body.tsx @@ -0,0 +1,42 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; +import {useIntl} from 'react-intl'; + +import type {Post} from '@mattermost/types/posts'; +import type {UserProfile} from '@mattermost/types/users'; + +import BodyMainActionText from 'components/remove_flagged_message_confirmation_modal/body_main_action_text'; + +type BodyProps = { + flaggedPost: Post; + reportingUser: UserProfile; +}; + +export function SkipConfirmStepBody({ + flaggedPost, + reportingUser, +}: BodyProps) { + const {formatMessage} = useIntl(); + + const text = formatMessage({ + id: 'keep_remove_quarantined_content_modal.action_remove.skip_confirm.body', + defaultMessage: + 'You are proceeding with content removal without downloading a report. Any subsequently generated report will not contain the original message contents. This action cannot be reverted.', + }); + + return ( +
+ + {text} +
+ ); +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_footer.scss b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_footer.scss new file mode 100644 index 00000000000..c6c10103996 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_footer.scss @@ -0,0 +1,8 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +.KeepRemoveFlaggedMessageConfirmationModal { + .ModalFooterRow.ModalFooterRow--end { + justify-content: flex-end; + } +} diff --git a/webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_footer.tsx b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_footer.tsx new file mode 100644 index 00000000000..651ec3e5971 --- /dev/null +++ b/webapp/channels/src/components/remove_flagged_message_confirmation_modal/skip_confirm_step/skip_confirm_step_footer.tsx @@ -0,0 +1,44 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; +import {useIntl} from 'react-intl'; + +import './skip_confirm_step_footer.scss'; + +type FooterProps = { + submitting: boolean; + onBack: () => void; + onPrimary: () => void; +}; + +export function SkipConfirmStepFooter({submitting, onBack, onPrimary}: FooterProps) { + const {formatMessage} = useIntl(); + + const backText = formatMessage({id: 'keep_remove_quarantined_content_modal.back.button_text', defaultMessage: 'Back'}); + const primaryText = formatMessage({id: 'keep_remove_quarantined_content_modal.action_remove_without_report.button_text', defaultMessage: 'Remove without report'}); + + return ( +
+
+ + +
+
+ ); +} diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index 3b418757e21..65f3402ac17 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -4458,8 +4458,13 @@ "custom_status.suggestions.working_from_home": "Working from home", "data_spillage_report_post.reporting_comment.placeholder": "No comment", "data_spillage_report_post.title": "{user} submitted a message for review", + "data_spillage_report.download_report.button_text": "Download Report", + "data_spillage_report.download_report.failed.button_text": "Generation failed. Try again.", + "data_spillage_report.download_report.generating.button_text": "Generating report…", "data_spillage_report.keep_message.button_text": "Keep message", "data_spillage_report.remove_message.button_text": "Remove message", + "data_spillage_report.row.actions.label": "Actions", + "data_spillage_report.row.report.label": "Report", "data_spillage_report.view_details.button_text": "View details", "date_separator.today": "Today", "date_separator.tomorrow": "Tomorrow", @@ -5267,16 +5272,35 @@ "katex.error": "Couldn't compile your Latex code. Please review the syntax and try again.", "keep_remove_quarantined_content_modal.action_keep.body": "You are about to keep a quarantined message authored by {flaggedPostAuthor} posted in the {flaggedPostChannel} channel and quarantined for review by {reportingUser}.", "keep_remove_quarantined_content_modal.action_keep.button_text": "Keep message", + "keep_remove_quarantined_content_modal.action_keep.generated.body": "The report should now be downloading on your device. Once it is downloaded, you can keep the message permanently.", + "keep_remove_quarantined_content_modal.action_keep.generating.body": "Please wait for the report to download before you keep the message permanently.", + "keep_remove_quarantined_content_modal.action_keep.permanent_button_text": "Keep permanently", "keep_remove_quarantined_content_modal.action_keep.subtext.no_notify_reporter": "If you confirm, the message will be visible to all channel members.", "keep_remove_quarantined_content_modal.action_keep.subtext.notify_reporter": "If you confirm, the message will be visible to all channel members and a notification will be sent to the reporter.", "keep_remove_quarantined_content_modal.action_keep.title": "Keep message", + "keep_remove_quarantined_content_modal.action_remove_without_report.button_text": "Remove without report", + "keep_remove_quarantined_content_modal.action_remove_without_report.title": "Remove without report?", "keep_remove_quarantined_content_modal.action_remove.body": "You are about to remove a message authored by {flaggedPostAuthor} posted in the {flaggedPostChannel} channel and quarantined for review by {reportingUser}.", "keep_remove_quarantined_content_modal.action_remove.button_text": "Remove message", + "keep_remove_quarantined_content_modal.action_remove.generated.body": "The report should now be downloading on your device. Once it is downloaded, you can remove the message permanently.", + "keep_remove_quarantined_content_modal.action_remove.generating.body": "Please wait for the report to download before you remove the message permanently. There will be no way to recover the message contents once it is removed.", + "keep_remove_quarantined_content_modal.action_remove.permanent_button_text": "Remove permanently", + "keep_remove_quarantined_content_modal.action_remove.skip_confirm.body": "You are proceeding with content removal without downloading a report. Any subsequently generated report will not contain the original message contents. This action cannot be reverted.", "keep_remove_quarantined_content_modal.action_remove.subtext.no_notify_reporter": "If you confirm, the message will be removed from the channel. This action cannot be reverted.", "keep_remove_quarantined_content_modal.action_remove.subtext.notify_reporter": "If you confirm, the message will be removed from the channel and a notification will be sent to the reporter. This action cannot be reverted.", "keep_remove_quarantined_content_modal.action_remove.title": "Remove message from channel", + "keep_remove_quarantined_content_modal.back.button_text": "Back", "keep_remove_quarantined_content_modal.comment_required.error": "Please add a comment.", "keep_remove_quarantined_content_modal.comment.placeholder": "Add your comment here", + "keep_remove_quarantined_content_modal.continue.button_text": "Continue", + "keep_remove_quarantined_content_modal.download_again.button_text": "Download again", + "keep_remove_quarantined_content_modal.download_report_checkbox.label": "Download quarantined message report", + "keep_remove_quarantined_content_modal.error.body": "We were unable to generate and download the report to your device.", + "keep_remove_quarantined_content_modal.error.title": "Report could not be generated", + "keep_remove_quarantined_content_modal.generated.title": "Report generated", + "keep_remove_quarantined_content_modal.generating.title": "Generating report…", + "keep_remove_quarantined_content_modal.skip_report_download.button_text": "Skip report download", + "keep_remove_quarantined_content_modal.try_again.button_text": "Try again", "last_users_message.added_to_channel.type": "were **added to the channel** by {actor}.", "last_users_message.added_to_team.type": "were **added to the team** by {actor}.", "last_users_message.first": "{firstUser} and ", @@ -5903,7 +5927,6 @@ "promote_to_user_modal.desc": "This action promotes the guest {username} to a member. It will allow the user to join public channels and interact with users outside of the channels they are currently members of. Are you sure you want to promote guest {username} to member?", "promote_to_user_modal.promote": "Promote", "promote_to_user_modal.title": "Promote guest {username} to member", - "property_card.actions_row.label": "Actions", "property_card.field.action_time.label": "Reviewed at", "property_card.field.actor_comment.label": "Reviewer's comment", "property_card.field.actor_user_id.label": "Reviewed by", diff --git a/webapp/platform/client/src/client4.test.ts b/webapp/platform/client/src/client4.test.ts index 8aa37520ee5..5f6a2174537 100644 --- a/webapp/platform/client/src/client4.test.ts +++ b/webapp/platform/client/src/client4.test.ts @@ -255,6 +255,25 @@ describe('Client4', () => { expect(result[1]).toEqual({user_id: 'dummy-user-id', channel_id: 'channel2', roles: 'channel_user channel_admin'}); expect(result[2]).toEqual({user_id: 'dummy-user-id', channel_id: 'channel3', roles: 'channel_user'}); }); + + test('should parse ZIP responses as blobs', async () => { + const client = new Client4(); + client.setUrl('http://mattermost.example.com'); + + const postId = 'dummy-post-id'; + const zipData = Buffer.from('zip contents'); + + nock(client.getBaseRoute()). + post(`/content_flagging/post/${postId}/report`, {comment: 'investigation note'}). + reply(200, zipData, {'Content-Type': 'application/zip'}); + + const result = await client.generateFlaggedPostReport(postId, 'investigation note'); + + expect(typeof result.text).toBe('function'); + expect(result.size).toEqual(zipData.length); + expect(result.type).toEqual('application/zip'); + expect(await result.text()).toEqual('zip contents'); + }); }); }); diff --git a/webapp/platform/client/src/client4.ts b/webapp/platform/client/src/client4.ts index 3ca3afe26e6..1c7d16a424a 100644 --- a/webapp/platform/client/src/client4.ts +++ b/webapp/platform/client/src/client4.ts @@ -4631,6 +4631,8 @@ export default class Client4 { const text = await response.text(); const objects = text.trim().split('\n'); data = objects.map((obj) => JSON.parse(obj)); + } else if (contentType === 'application/zip') { + data = await response.blob(); } else { data = await response.text(); } @@ -5083,6 +5085,21 @@ export default class Client4 { {method: 'get'}, ); }; + + getFlaggedPostReportUrl = (postId: string) => { + return `${this.getContentFlaggingRoute()}/post/${postId}/report`; + }; + + generateFlaggedPostReport = (postId: string, comment: string, action?: 'keep' | 'remove', signal?: AbortSignal): Promise => { + return this.doFetch( + this.getFlaggedPostReportUrl(postId), + { + method: 'post', + body: JSON.stringify({comment, action}), + signal, + }, + ); + }; } export function parseAndMergeNestedHeaders(originalHeaders: any) { From d4471bece166ea55aa605e750cfa1ac4a9580eb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20V=C3=A9lez?= Date: Mon, 18 May 2026 17:33:13 +0200 Subject: [PATCH 2/3] Mm 68503 be abac mask save path masking (#36513) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * MM-68501 - implement GetMaskedVisualAST and wire API handler Co-Authored-By: Claude Opus 4.6 (1M context) * add missing test and fix style issues * fix styles * implement coderabbit feedback * MM-68501 - PR review: split masking file, model-level access mode, reject contradictory config Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68501 - apply shared_only filter to non-option field values (binary masking) * MM-68501 - consolidate masking flag check and log corrupt text value during masking * MM-68503 - add CEL utilities, write-path validation, and merge helpers Combined set of helpers consumed by BE-5's save path: CEL construction / serialization - extractStringValues, buildCELFromConditions, conditionToCEL, celStringLiteral, celValueLiteral. Used to rebuild a CEL string from a VisualExpression, including for GetMaskedExpression on the read-side of policy GET / search responses. Merge-on-save helpers - getHiddenValues (per-condition, with pre-fetched fields map for N+1 avoidance) — finds which stored values are not visible to the caller. - mergeConditionValues — re-injects the hidden values into a submitted condition without duplicates. - Together, these let BE-5 preserve attribute values the caller cannot see while still letting them edit the visible parts of a policy. Write-path value-hold validation - validatePolicyExpressionValues, invalidValueError, validateConditionValues. - Generic "Invalid value." error on every rejection — no signal about whether the value exists or is merely not held (prevents enumeration). - Rejects the masked-token sentinel "--------" if submitted as a literal. These all live in access_control_masking.go alongside the masking primitives that BE-2 introduced. i18n entries added for the two new error IDs (app.pap.save_policy.invalid_value, app.pap.validate_expression_values.app_error). Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68503 - handle the masked-token sentinel in validation and merge When the GET /policies endpoint returns a policy via MaskPolicyExpressions, the raw expression contains the masked-token sentinel "--------" in place of hidden values. If the frontend round-trips that expression unchanged back to the server (e.g., the admin only modified channel assignment, not the rules), the sentinel reaches the save path. The previous code in validateConditionValues rejected the sentinel as "Invalid value." This blocks the legitimate round-trip case. Fix: - validateConditionValues: treat the sentinel as a placeholder and skip it during visibility / source-only / unknown-mode checks. Other values are still validated normally. - mergeConditionValues: strip the sentinel from submitted values before appending hidden values, so it never propagates to the stored result. Both array and single-value forms (string == "--------") are handled. TestMaskedTokenRejection (which asserted the old rejection behavior) is replaced by TestMaskedTokenConstant which only verifies the sentinel string itself. Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68504 - integrate save-path masking: 403 block on delete, merge-on-save, response masking Save path (CreateOrUpdateAccessControlPolicy): * validatePolicyExpressionValues runs on the submitted expression before merge so re-injected hidden values are never validated against the caller's holdings. * mergeStoredPolicyExpressions re-injects hidden values from the stored policy and blocks (HTTP 403) any attempt to remove a condition that contained values the caller cannot see — closes the row-deletion gap in classified environments. * mergeExpressionWithMaskedValues unwraps single-element arrays for scalar operators after restoring the stored operator (avoids "attr == [val]" invalid CEL when the frontend submits "attr in []" as the masked-row placeholder for an originally-scalar condition). * checkSelfInclusion is bypassed for system admins (they may legitimately write conditions for values they do not hold); masking and value-hold validation still apply to system admins. Delete path (DeleteAccessControlPolicy): * Same masked-values 403 block — a caller with masked values cannot delete the policy at all (UI Delete button is also disabled in FE-3). Response masking: * createAccessControlPolicy and setAccessControlPolicyActiveStatus run MaskPolicyExpressions on the response so even a save reply doesn't leak the values the caller does not hold. GetMaskedExpression, maskConditionValuesWithToken, replaceHiddenValuesWithToken, MaskPolicyExpressions live alongside the rest of the masking helpers in access_control_masking.go. team_access_control.go: corrects ValidateChannelEligibilityForAccessControl call site (drops the spurious receiver and rctx; it's a package-level helper that only takes channel). Co-Authored-By: Claude Opus 4.7 (1M context) * MM-68503 - address PR review: batch field fetches, propagate errors, fail-closed write path * MM-68503 - restore team-admin api4 tests accidentally dropped during BE-5 rebuild * MM-68503 - address review and CodeRabbit feedback on save-path masking * add tests for delete masking, self-inclusion, GET mask * add assertions to strengten tests * fail-closed guard for advanced expressions in merge-on-save, plus helper unit tests, and FF/test-helper cleanups * Refactor access control methods to use GetPropertyGroup for CPA group ID retrieval --------- Co-authored-by: Claude Opus 4.6 (1M context) Co-authored-by: Mattermost Build --- server/channels/api4/access_control.go | 22 +- server/channels/api4/access_control_test.go | 144 ++-- server/channels/app/access_control.go | 317 ++++++++- server/channels/app/access_control_masking.go | 629 +++++++++++++++++- .../channels/app/access_control_merge_test.go | 474 +++++++++++++ server/channels/app/access_control_test.go | 150 +++++ .../app/access_control_validation_test.go | 231 +++++++ server/i18n/en.json | 32 + 8 files changed, 1944 insertions(+), 55 deletions(-) create mode 100644 server/channels/app/access_control_merge_test.go create mode 100644 server/channels/app/access_control_validation_test.go diff --git a/server/channels/api4/access_control.go b/server/channels/api4/access_control.go index c8bb59038b2..af0c87b23ac 100644 --- a/server/channels/api4/access_control.go +++ b/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)) diff --git a/server/channels/api4/access_control_test.go b/server/channels/api4/access_control_test.go index 8b09d036ce2..0a4d5251666 100644 --- a/server/channels/api4/access_control_test.go +++ b/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) diff --git a/server/channels/app/access_control.go b/server/channels/app/access_control.go index eb942db16ba..99df3abc372 100644 --- a/server/channels/app/access_control.go +++ b/server/channels/app/access_control.go @@ -112,6 +112,41 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model. } } + // ABAC is gated at route registration; only check masking here. Masking is + // attribute-based: edits are allowed with masked values present as long as + // the caller doesn't drop a condition holding values they couldn't see. + if a.Config().FeatureFlags.AttributeValueMasking { + session := rctx.Session() + if session == nil { + return nil, model.NewAppError("CreateOrUpdateAccessControlPolicy", "api.context.session_expired.app_error", nil, "session required for masking validation", http.StatusUnauthorized) + } + callerID := session.UserId + + // Validate submitted values BEFORE merge: only the values the caller + // actually submitted should be checked against their holdings. Running + // validation after merge would reject the re-injected hidden values + // (e.g. Bravo, Charlie) that the caller legitimately cannot see. + if appErr := a.validatePolicyExpressionValues(rctx, policy, callerID); appErr != nil { + return nil, appErr + } + + // Merge hidden values back in and block deletion of masked conditions. + if appErr := a.mergeStoredPolicyExpressions(rctx, policy, callerID); appErr != nil { + return nil, appErr + } + + // Self-inclusion check applies only to non-admins. System admins may + // legitimately set conditions for attributes they do not personally hold + // (e.g., creating a "Clearance == Top Secret" rule without holding that + // clearance themselves). Masking and write-path value validation still + // apply to system admins above. + if !a.HasPermissionTo(callerID, model.PermissionManageSystem) { + if appErr := a.checkSelfInclusion(rctx, policy, callerID); appErr != nil { + return nil, appErr + } + } + } + var appErr *model.AppError policy, appErr = acs.SavePolicy(rctx, policy) if appErr != nil { @@ -128,6 +163,266 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model. return policy, nil } +// policyHasMaskedValuesForCaller returns true if policy contains any attribute values +// that are not visible to callerID under the current masking rules. +// A nil policy is treated as "no hidden values" — there's nothing to protect. +func (a *App) policyHasMaskedValuesForCaller(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) (bool, *model.AppError) { + if policy == nil { + return false, nil + } + + for _, rule := range policy.Rules { + if rule.Expression == "" || rule.Expression == "true" { + continue + } + maskedAST, appErr := a.GetMaskedVisualAST(rctx, rule.Expression, callerID) + if appErr != nil { + return false, appErr + } + for _, cond := range maskedAST.Conditions { + if cond.HasMaskedValues { + return true, nil + } + } + } + + return false, nil +} + +// mergeStoredPolicyExpressions re-injects hidden values from the stored policy into the +// submitted one, and blocks the save if the caller removed a condition that contained +// values they cannot see (which would silently widen access beyond what they could audit). +// No-op for new policies (not found in store). +func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) *model.AppError { + acs := a.Srv().ch.AccessControl + if acs == nil { + return nil + } + + existingPolicy, appErr := acs.GetPolicy(rctx, policy.ID) + if appErr != nil { + if appErr.StatusCode == http.StatusNotFound { + return nil + } + return appErr + } + + for i, rule := range policy.Rules { + if i >= len(existingPolicy.Rules) { + continue + } + storedExpr := existingPolicy.Rules[i].Expression + if storedExpr == "" || storedExpr == "true" { + continue + } + mergedExpr, appErr := a.mergeExpressionWithMaskedValues(rctx, policy.ID, rule.Expression, storedExpr, callerID) + if appErr != nil { + return appErr + } + policy.Rules[i].Expression = mergedExpr + } + + // Any stored rules beyond the submitted set were dropped by the caller. If any of those + // contain values the caller cannot see, block the save — otherwise we'd silently widen + // access by removing a rule whose hidden conditions the caller could not audit. + if len(existingPolicy.Rules) > len(policy.Rules) { + for i := len(policy.Rules); i < len(existingPolicy.Rules); i++ { + storedExpr := existingPolicy.Rules[i].Expression + if storedExpr == "" || storedExpr == "true" { + continue + } + hasMasked, appErr := a.expressionHasMaskedValuesForCaller(rctx, storedExpr, callerID) + if appErr != nil { + return appErr + } + if hasMasked { + return model.NewAppError("mergeStoredPolicyExpressions", "app.pap.save_policy.masked_rule_deleted", nil, + "cannot remove a rule that contains attribute values you do not hold", http.StatusForbidden) + } + } + } + + return nil +} + +// expressionHasMaskedValuesForCaller reports whether storedExpr contains any value the caller cannot see. +func (a *App) expressionHasMaskedValuesForCaller(rctx request.CTX, storedExpr, callerID string) (bool, *model.AppError) { + maskedAST, appErr := a.GetMaskedVisualAST(rctx, storedExpr, callerID) + if appErr != nil { + return false, appErr + } + for _, cond := range maskedAST.Conditions { + if cond.HasMaskedValues { + return true, nil + } + } + return false, nil +} + +// mergeExpressionWithMaskedValues re-injects hidden values into submittedExpr and +// returns 403 if the caller dropped a condition with values they cannot see. +// +// Two fail-closed shortcuts before the merge: +// 1. Caller has no masked values on storedExpr → return submitted as-is. +// 2. storedExpr isn't faithfully representable by the Visual AST (|| or grouping +// would flatten into ANDs on rebuild) → accept only no-op saves (e.g., rename), +// reject real edits. Role-neutral: masking is attribute-based, so a sysadmin +// without the values lands here too. +// +// Stopgap until the canonical CEL AST walker refactor. +func (a *App) mergeExpressionWithMaskedValues(rctx request.CTX, policyID, submittedExpr, storedExpr, callerID string) (string, *model.AppError) { + hasMasked, appErr := a.expressionHasMaskedValuesForCaller(rctx, storedExpr, callerID) + if appErr != nil { + return "", appErr + } + if !hasMasked { + return submittedExpr, nil + } + + submittedAST, appErr := a.ExpressionToVisualAST(rctx, submittedExpr) + if appErr != nil { + return "", appErr + } + + storedAST, appErr := a.ExpressionToVisualAST(rctx, storedExpr) + if appErr != nil { + return "", appErr + } + + if !isVisualASTRepresentable(storedExpr, storedAST) { + masked, maskErr := a.GetMaskedExpression(rctx, storedExpr, callerID) + if maskErr != nil { + return "", maskErr + } + if normalizedEqual(submittedExpr, masked) { + // no-op edit (e.g., rename) — keep stored expression as-is + return storedExpr, nil + } + rctx.Logger().Info("save refused: stored rule not representable by Visual AST", + mlog.String("policy_id", policyID), + mlog.String("caller_id", callerID), + ) + return "", model.NewAppError("mergeExpressionWithMaskedValues", + "app.pap.save_policy.advanced_expression_blocked", nil, + "this rule expression cannot be safely edited while restricted values are present", + http.StatusForbidden) + } + + cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + if appErr != nil { + return "", model.NewAppError("mergeExpressionWithMaskedValues", "app.pap.merge_expression.app_error", nil, "", http.StatusInternalServerError).Wrap(appErr) + } + cpaGroupID := cpaGroup.ID + + rctxWithCaller := RequestContextWithCallerID(rctx, callerID) + + // Pre-fetch fields once for all stored conditions. We require every referenced field + // to resolve — proceeding with an incomplete map would silently strip hidden values + // from stored conditions and bypass the masked-condition-delete block. + fieldsByName := a.fetchConditionFields(rctxWithCaller, storedAST.Conditions, cpaGroupID) + if appErr := requireAllFieldsResolved(rctxWithCaller, storedAST.Conditions, fieldsByName); appErr != nil { + return "", appErr + } + + // Count submitted conditions per attribute. A simple set isn't enough because the parser + // can produce two conditions on the same attribute (e.g. `attr in [...] && attr == "x"`); + // dropping one of them while keeping the other must still trigger the deletion guard if + // the dropped condition had hidden values. + submittedCounts := make(map[string]int, len(submittedAST.Conditions)) + for _, cond := range submittedAST.Conditions { + submittedCounts[cond.Attribute]++ + } + + storedCounts := make(map[string]int, len(storedAST.Conditions)) + for _, cond := range storedAST.Conditions { + storedCounts[cond.Attribute]++ + } + + // Block deletion of any stored condition that has hidden values for this caller. + // We walk stored conditions and, when one with hidden values appears, require that + // the submitted set still has at least as many conditions on the same attribute as + // stored had — otherwise some stored condition was dropped. + for i := range storedAST.Conditions { + hidden := a.getHiddenValues(rctxWithCaller, callerID, &storedAST.Conditions[i], cpaGroupID, fieldsByName) + if len(hidden) == 0 { + continue + } + attr := storedAST.Conditions[i].Attribute + if submittedCounts[attr] < storedCounts[attr] { + return "", model.NewAppError("mergeExpressionWithMaskedValues", "app.pap.save_policy.masked_condition_deleted", nil, + "cannot remove a rule condition that contains attribute values you do not hold", http.StatusForbidden) + } + } + + // Match submitted conditions to stored ones by attribute (in order), merge hidden values. + storedByAttr := make(map[string][]model.Condition) + for _, cond := range storedAST.Conditions { + storedByAttr[cond.Attribute] = append(storedByAttr[cond.Attribute], cond) + } + + matchCount := make(map[string]int) + var mergedConditions []model.Condition + + for _, submitted := range submittedAST.Conditions { + storedList, found := storedByAttr[submitted.Attribute] + if !found { + mergedConditions = append(mergedConditions, submitted) + continue + } + + matchIdx := matchCount[submitted.Attribute] + matchCount[submitted.Attribute]++ + + if matchIdx >= len(storedList) { + mergedConditions = append(mergedConditions, submitted) + continue + } + + stored := storedList[matchIdx] + hiddenValues := a.getHiddenValues(rctxWithCaller, callerID, &stored, cpaGroupID, fieldsByName) + merged := mergeConditionValues(submitted, hiddenValues) + merged.Operator = stored.Operator + merged.AttributeType = stored.AttributeType + // Frontend emits "attr in []" as the placeholder for any fully-masked row + // regardless of the stored operator. After we restore the original operator, + // the value shape may not match (e.g., "==" with a []any value). Normalize + // scalar operators to a single string from the array. + if isScalarOperator(merged.Operator) { + if arr, ok := merged.Value.([]any); ok { + if len(arr) == 0 { + merged.Value = nil + } else if s, ok := arr[0].(string); ok { + merged.Value = s + } + } + } + mergedConditions = append(mergedConditions, merged) + } + + return buildCELFromConditions(mergedConditions), nil +} + +// checkSelfInclusion verifies the caller satisfies all policy rules after their edit. +func (a *App) checkSelfInclusion(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) *model.AppError { + for _, rule := range policy.Rules { + if rule.Expression == "" || rule.Expression == "true" { + continue + } + + matches, appErr := a.ValidateExpressionAgainstRequester(rctx, rule.Expression, callerID) + if appErr != nil { + return appErr + } + if !matches { + return model.NewAppError("CreateOrUpdateAccessControlPolicy", + "app.pap.save_policy.self_exclusion", nil, + "You do not satisfy one or more conditions in this policy.", http.StatusForbidden) + } + } + + return nil +} + func (a *App) DeleteAccessControlPolicy(rctx request.CTX, id string) *model.AppError { acs := a.Srv().ch.AccessControl if acs == nil { @@ -142,6 +437,20 @@ func (a *App) DeleteAccessControlPolicy(rctx request.CTX, id string) *model.AppE return appErr } + // ABAC is gated at route registration; only check masking here. + if a.Config().FeatureFlags.AttributeValueMasking { + session := rctx.Session() + if session != nil { + callerID := session.UserId + if hasMasked, appErr := a.policyHasMaskedValuesForCaller(rctx, policy, callerID); appErr != nil { + return appErr + } else if hasMasked { + return model.NewAppError("DeleteAccessControlPolicy", "app.pap.delete_policy.masked_values", nil, + "policy contains attribute values you do not hold; you cannot delete this policy", http.StatusForbidden) + } + } + } + var affectedChannelIDs []string if policy != nil && policy.Type != model.AccessControlPolicyTypeChannel { affectedChannelIDs = a.channelPolicyIDsWithImport(rctx, id) @@ -371,14 +680,6 @@ func (a *App) UpdateAccessControlPoliciesActive(rctx request.CTX, updates []mode if err != nil { return nil, model.NewAppError("UpdateAccessControlPoliciesActive", "app.pap.update_access_control_policies_active.app_error", nil, err.Error(), http.StatusInternalServerError) } - - for _, policy := range policies { - // only channel policies use the active state - if policy.Type == model.AccessControlPolicyTypeChannel { - a.publishChannelPolicyEnforcedUpdate(rctx, policy.ID) - } - } - return policies, nil } diff --git a/server/channels/app/access_control_masking.go b/server/channels/app/access_control_masking.go index c64af04d9a6..f82c9128374 100644 --- a/server/channels/app/access_control_masking.go +++ b/server/channels/app/access_control_masking.go @@ -5,7 +5,9 @@ package app import ( "encoding/json" + "fmt" "net/http" + "strconv" "strings" "github.com/mattermost/mattermost/server/public/model" @@ -50,7 +52,9 @@ func (a *App) GetMaskedVisualAST(rctx request.CTX, expression string, callerID s } // fetchConditionFields collects unique field names from conditions and fetches each once. -// Fields that fail lookup are omitted; maskConditionValues treats missing entries as fail-closed. +// Lookup failures are logged and omitted from the returned map; read-path callers treat +// missing entries as fail-closed (mask the value). Write-path callers should additionally +// call requireAllFieldsResolved to refuse to proceed when any referenced field is missing. func (a *App) fetchConditionFields(rctx request.CTX, conditions []model.Condition, cpaGroupID string) map[string]*model.PropertyField { seen := make(map[string]bool) for _, c := range conditions { @@ -77,6 +81,31 @@ func (a *App) fetchConditionFields(rctx request.CTX, conditions []model.Conditio return fields } +// requireAllFieldsResolved returns the generic invalid-value error if any condition +// references a field name missing from fieldsByName. Write-path callers use this to refuse +// the save rather than silently strip hidden values from conditions whose fields could not +// be resolved. We return the same generic 400 used by the rest of write-path validation so +// unknown/deleted fields don't leak an enumeration signal distinct from hidden-value +// rejection — the actual field name is logged for operator diagnostics instead. +func requireAllFieldsResolved(rctx request.CTX, conditions []model.Condition, fieldsByName map[string]*model.PropertyField) *model.AppError { + for _, c := range conditions { + if c.ValueType == model.AttrValue { + continue + } + name := extractFieldName(c.Attribute) + if name == "" { + continue + } + if _, ok := fieldsByName[name]; !ok { + rctx.Logger().Warn("Field referenced by condition could not be resolved during write-path validation", + mlog.String("field_name", name), + ) + return invalidValueError() + } + } + return nil +} + // maskConditionValues applies masking to a single condition in place. // // Masking semantics differ by field type: @@ -246,3 +275,601 @@ func filterConditionValues(condition *model.Condition, visibleNames map[string]s } } } + +// getHiddenValues returns the subset of stored condition values not visible to callerID. +// fieldsByName is pre-fetched by the caller to avoid N+1 lookups; a missing entry is +// treated as fail-closed (no hidden values injected for that condition). +func (a *App) getHiddenValues(rctx request.CTX, callerID string, stored *model.Condition, cpaGroupID string, fieldsByName map[string]*model.PropertyField) []string { + if stored.ValueType == model.AttrValue { + return nil + } + + fieldName := extractFieldName(stored.Attribute) + if fieldName == "" { + return nil + } + + field, ok := fieldsByName[fieldName] + if !ok { + return nil + } + + switch field.GetAccessMode() { + case model.PropertyAccessModeSourceOnly: + return extractStringValues(stored.Value) + case model.PropertyAccessModeSharedOnly: + var visibleNames map[string]struct{} + if field.Type == model.PropertyFieldTypeSelect || field.Type == model.PropertyFieldTypeMultiselect { + visibleNames = extractVisibleOptionNames(field) + } else { + visibleNames = a.getCallerTextValues(rctx, callerID, field, cpaGroupID) + } + var hidden []string + for _, val := range extractStringValues(stored.Value) { + if _, visible := visibleNames[val]; !visible { + hidden = append(hidden, val) + } + } + return hidden + default: + return nil + } +} + +// isScalarOperator reports whether the operator expects a single value (not a list). +// Used by merge-on-save to normalize the value shape after restoring the stored operator. +func isScalarOperator(op string) bool { + switch op { + case "==", "!=", ">", ">=", "<", "<=", "contains", "startsWith", "endsWith": + return true + } + return false +} + +// mergeConditionValues appends hiddenValues into the submitted condition's values, +// deduplicating. A nil submitted value is restored from hidden values alone. +func mergeConditionValues(submitted model.Condition, hiddenValues []string) model.Condition { + if len(hiddenValues) == 0 { + return submitted + } + + merged := submitted + + switch v := submitted.Value.(type) { + case []any: + // Strip the masked-token sentinel from submitted values: it's the + // server's own placeholder for hidden values (from a masked GET), + // not a real value, and we're about to re-inject the actual stored + // hidden values from hiddenValues. + seen := make(map[string]struct{}) + cleaned := make([]any, 0, len(v)) + for _, item := range v { + if s, ok := item.(string); ok { + if s == maskedTokenValue { + continue + } + seen[s] = struct{}{} + } + cleaned = append(cleaned, item) + } + result := make([]any, 0, len(cleaned)+len(hiddenValues)) + result = append(result, cleaned...) + for _, hidden := range hiddenValues { + if _, exists := seen[hidden]; !exists { + result = append(result, hidden) + } + } + merged.Value = result + + case string: + // Empty string and the masked-token sentinel both mean "no real value + // submitted here"; restore from hidden values. + if (v == "" || v == maskedTokenValue) && len(hiddenValues) > 0 { + merged.Value = hiddenValues[0] + } + + case nil: + if len(hiddenValues) == 1 { + merged.Value = hiddenValues[0] + } else if len(hiddenValues) > 1 { + result := make([]any, 0, len(hiddenValues)) + for _, h := range hiddenValues { + result = append(result, h) + } + merged.Value = result + } + } + + return merged +} + +// containsNonStringLiteral reports whether the condition value contains any +// non-string element (numeric, boolean, etc.). Used by the write-path to reject +// type-mismatched literals on property-backed conditions — without this guard, +// extractStringValues would silently drop such elements and let invalid CEL +// bypass the source_only / shared_only checks. +func containsNonStringLiteral(value any) bool { + switch v := value.(type) { + case nil, string: + return false + case []any: + for _, item := range v { + if _, ok := item.(string); !ok { + return true + } + } + return false + default: + // numeric, boolean, etc. + return true + } +} + +// extractStringValues converts a condition's Value to a slice of strings. +// Non-string elements are silently dropped — write-path callers should pair +// this with containsNonStringLiteral to reject type-mismatched literals first. +func extractStringValues(value any) []string { + switch v := value.(type) { + case []any: + var result []string + for _, item := range v { + if s, ok := item.(string); ok { + result = append(result, s) + } + } + return result + case string: + return []string{v} + default: + return nil + } +} + +// buildCELFromConditions reconstructs a CEL expression from conditions, joined with " && ". +func buildCELFromConditions(conditions []model.Condition) string { + if len(conditions) == 0 { + return "true" + } + + parts := make([]string, 0, len(conditions)) + for _, cond := range conditions { + cel := conditionToCEL(cond) + if cel != "" { + parts = append(parts, cel) + } + } + + if len(parts) == 0 { + return "true" + } + + return strings.Join(parts, " && ") +} + +// isVisualASTRepresentable reports whether buildCELFromConditions(ast) round-trips +// back to originalExpr. False means merging would silently rewrite the shape +// (typically || or grouping that the AST flattens into ANDs). Stopgap until the +// canonical AST walker lands. +func isVisualASTRepresentable(originalExpr string, ast *model.VisualExpression) bool { + if ast == nil || len(ast.Conditions) == 0 { + return originalExpr == "" || originalExpr == "true" + } + return normalizedEqual(originalExpr, buildCELFromConditions(ast.Conditions)) +} + +// normalizedEqual compares two CEL expressions modulo whitespace and quote style. +// Unbalanced quotes on either side count as not-equal (fail-closed). +func normalizedEqual(a, b string) bool { + na, okA := normalizeForComparison(a) + if !okA { + return false + } + nb, okB := normalizeForComparison(b) + if !okB { + return false + } + return na == nb +} + +// normalizeForComparison strips whitespace outside string literals and rewrites +// single quotes to double. String contents are preserved verbatim. Returns +// ok=false on unbalanced quotes. +func normalizeForComparison(s string) (string, bool) { + var b strings.Builder + b.Grow(len(s)) + var quote byte // 0 outside string literal; '"' or '\'' inside + for i := 0; i < len(s); i++ { + c := s[i] + switch { + case quote == 0 && (c == '"' || c == '\''): + quote = c + b.WriteByte('"') + case quote != 0 && c == '\\' && i+1 < len(s): + // keep escapes verbatim + b.WriteByte(c) + b.WriteByte(s[i+1]) + i++ + case quote != 0 && c == quote: + b.WriteByte('"') + quote = 0 + case quote == 0 && (c == ' ' || c == '\t' || c == '\n' || c == '\r'): + // drop whitespace outside strings + default: + b.WriteByte(c) + } + } + if quote != 0 { + return "", false + } + return b.String(), true +} + +// conditionToCEL converts a single Condition to its CEL string representation. +func conditionToCEL(cond model.Condition) string { + attr := cond.Attribute + + switch cond.Operator { + case "==", "!=", ">", ">=", "<", "<=": + if cond.Value == nil { + return "" + } + return attr + " " + cond.Operator + " " + celValueLiteral(cond.Value) + + case "in": + values := extractStringValues(cond.Value) + if len(values) == 0 { + return "" + } + if cond.AttributeType == "multiselect" { + // multiselect: "v1" in attr && "v2" in attr + inParts := make([]string, 0, len(values)) + for _, v := range values { + inParts = append(inParts, celStringLiteral(v)+" in "+attr) + } + return strings.Join(inParts, " && ") + } + // select: attr in ["v1", "v2"] + valLiterals := make([]string, 0, len(values)) + for _, v := range values { + valLiterals = append(valLiterals, celStringLiteral(v)) + } + return attr + " in [" + strings.Join(valLiterals, ", ") + "]" + + case "hasAnyOf": + values := extractStringValues(cond.Value) + if len(values) == 0 { + return "" + } + orParts := make([]string, 0, len(values)) + for _, v := range values { + orParts = append(orParts, celStringLiteral(v)+" in "+attr) + } + if len(orParts) == 1 { + return orParts[0] + } + return "(" + strings.Join(orParts, " || ") + ")" + + case "hasAllOf": + values := extractStringValues(cond.Value) + if len(values) == 0 { + return "" + } + andParts := make([]string, 0, len(values)) + for _, v := range values { + andParts = append(andParts, celStringLiteral(v)+" in "+attr) + } + return strings.Join(andParts, " && ") + + case "contains", "startsWith", "endsWith": + if cond.Value == nil { + return "" + } + return attr + "." + cond.Operator + "(" + celValueLiteral(cond.Value) + ")" + + default: + if cond.Value == nil { + return "" + } + return attr + " " + cond.Operator + " " + celValueLiteral(cond.Value) + } +} + +// celStringLiteral wraps s in a CEL-compatible double-quoted string literal. +// strconv.Quote produces Go syntax that overlaps with CEL's escape grammar +// (backslash, double quote, \a \b \f \n \r \t \v, \xHH, \uHHHH, \UHHHHHHHH), +// so it safely round-trips strings containing control characters, embedded +// quotes, or non-ASCII content — none of which the previous naive ReplaceAll +// handled. Attribute values that legitimately contain newlines or tabs would +// have produced broken CEL otherwise. +func celStringLiteral(s string) string { + return strconv.Quote(s) +} + +// celValueLiteral returns the CEL literal for a condition value. +func celValueLiteral(value any) string { + switch v := value.(type) { + case string: + return celStringLiteral(v) + case float64: + // 'g' with precision -1 produces the shortest representation that + // round-trips back to v exactly. Avoids the precision loss from + // fmt.Sprintf("%f") which rounds to six fractional digits. + return strconv.FormatFloat(v, 'g', -1, 64) + case int: + return fmt.Sprintf("%d", v) + case int64: + return fmt.Sprintf("%d", v) + case bool: + if v { + return "true" + } + return "false" + case nil: + return "null" + default: + return fmt.Sprintf("%v", v) + } +} + +// maskedTokenValue is the sentinel the frontend uses for masked values; never a valid attribute value. +const maskedTokenValue = "--------" + +// validatePolicyExpressionValues checks that all submitted literal values are held by the caller. +// Returns the same generic error for every rejection to prevent value enumeration. +func (a *App) validatePolicyExpressionValues(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) *model.AppError { + cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + if appErr != nil { + return model.NewAppError("validatePolicyExpressionValues", "app.pap.validate_expression_values.app_error", nil, "", http.StatusInternalServerError).Wrap(appErr) + } + cpaGroupID := cpaGroup.ID + + rctxWithCaller := RequestContextWithCallerID(rctx, callerID) + + // Parse all rule ASTs first and collect every referenced field so we can + // pre-fetch in a single pass, avoiding N+1 lookups across conditions. + rulesASTs := make([]*model.VisualExpression, 0, len(policy.Rules)) + var allConditions []model.Condition + for _, rule := range policy.Rules { + if rule.Expression == "" || rule.Expression == "true" { + continue + } + visualAST, appErr := a.ExpressionToVisualAST(rctx, rule.Expression) + if appErr != nil { + return appErr + } + rulesASTs = append(rulesASTs, visualAST) + allConditions = append(allConditions, visualAST.Conditions...) + } + + fieldsByName := a.fetchConditionFields(rctxWithCaller, allConditions, cpaGroupID) + if appErr := requireAllFieldsResolved(rctxWithCaller, allConditions, fieldsByName); appErr != nil { + return appErr + } + + for _, visualAST := range rulesASTs { + for _, cond := range visualAST.Conditions { + if appErr := a.validateConditionValues(rctxWithCaller, &cond, cpaGroupID, fieldsByName); appErr != nil { + return appErr + } + } + } + + return nil +} + +// invalidValueError returns the same generic 400 for all write-path rejections (no enumeration leakage). +func invalidValueError() *model.AppError { + return model.NewAppError("validatePolicyExpressionValues", "app.pap.save_policy.invalid_value", nil, "Invalid value.", http.StatusBadRequest) +} + +// validateConditionValues checks that all literal values in a single condition are held by the caller. +// fieldsByName is pre-fetched by the caller to avoid N+1 lookups; a missing entry means the field +// could not be resolved (deleted, or DB error at prefetch time) — rejected with the generic error. +func (a *App) validateConditionValues(rctx request.CTX, cond *model.Condition, cpaGroupID string, fieldsByName map[string]*model.PropertyField) *model.AppError { + if cond.ValueType == model.AttrValue { + return nil + } + + // The masked-token sentinel is what the server itself emits when masking the + // raw CEL of policy GET / search responses. If the frontend round-trips a GET + // response back to us unchanged (e.g. the admin only modified channel + // assignment, not the rules), it will appear here. Skip it during validation; + // mergeConditionValues will strip it from the merged result and re-inject the + // actual hidden values from the stored policy. + values := extractStringValues(cond.Value) + nonTokenValues := make([]string, 0, len(values)) + for _, v := range values { + if v != maskedTokenValue { + nonTokenValues = append(nonTokenValues, v) + } + } + + fieldName := extractFieldName(cond.Attribute) + if fieldName == "" { + return nil + } + + field, ok := fieldsByName[fieldName] + if !ok { + return invalidValueError() // reject unknown fields to prevent probing + } + + // Property-backed conditions must use string literals. Numeric / boolean values + // would be silently dropped by extractStringValues above, letting them bypass the + // source_only / shared_only checks. Reject them with the same generic error. + if containsNonStringLiteral(cond.Value) { + return invalidValueError() + } + + switch field.GetAccessMode() { + case model.PropertyAccessModePublic: + return nil + case model.PropertyAccessModeSourceOnly: + if len(nonTokenValues) > 0 { + return invalidValueError() + } + return nil + case model.PropertyAccessModeSharedOnly: + var visibleNames map[string]struct{} + if field.Type == model.PropertyFieldTypeSelect || field.Type == model.PropertyFieldTypeMultiselect { + visibleNames = extractVisibleOptionNames(field) + } else { + callerID, _ := CallerIDFromRequestContext(rctx) + visibleNames = a.getCallerTextValues(rctx, callerID, field, cpaGroupID) + } + for _, v := range nonTokenValues { + if _, visible := visibleNames[v]; !visible { + return invalidValueError() + } + } + return nil + default: + if len(nonTokenValues) > 0 { + return invalidValueError() + } + return nil + } +} + +func (a *App) GetMaskedExpression(rctx request.CTX, expression string, callerID string) (string, *model.AppError) { + if expression == "" || expression == "true" { + return expression, nil + } + + visualAST, appErr := a.ExpressionToVisualAST(rctx, expression) + if appErr != nil { + return "", appErr + } + + cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + if appErr != nil { + return "", appErr + } + cpaGroupID := cpaGroup.ID + + rctxWithCaller := RequestContextWithCallerID(rctx, callerID) + fieldsByName := a.fetchConditionFields(rctxWithCaller, visualAST.Conditions, cpaGroupID) + + for i := range visualAST.Conditions { + a.maskConditionValuesWithToken(rctxWithCaller, callerID, &visualAST.Conditions[i], cpaGroupID, fieldsByName) + } + + return buildCELFromConditions(visualAST.Conditions), nil +} + +// maskConditionValuesWithToken replaces non-held values with the masked token in place, +// preserving expression structure so the visual AST endpoint can still parse it. +// fieldsByName is pre-fetched by the caller to avoid N+1 lookups; a missing entry +// is treated as fail-closed (whole value masked). +func (a *App) maskConditionValuesWithToken(rctx request.CTX, callerID string, condition *model.Condition, cpaGroupID string, fieldsByName map[string]*model.PropertyField) { + if condition.ValueType == model.AttrValue { + return + } + + fieldName := extractFieldName(condition.Attribute) + if fieldName == "" { + return + } + + field, ok := fieldsByName[fieldName] + if !ok { + condition.Value = maskedTokenValue // fail closed + return + } + + switch field.GetAccessMode() { + case model.PropertyAccessModePublic: + return + case model.PropertyAccessModeSourceOnly: + condition.Value = maskedTokenValue + case model.PropertyAccessModeSharedOnly: + var visibleNames map[string]struct{} + if field.Type == model.PropertyFieldTypeSelect || field.Type == model.PropertyFieldTypeMultiselect { + visibleNames = extractVisibleOptionNames(field) + } else { + visibleNames = a.getCallerTextValues(rctx, callerID, field, cpaGroupID) + } + replaceHiddenValuesWithToken(condition, visibleNames) + default: + condition.Value = maskedTokenValue + } +} + +// replaceHiddenValuesWithToken keeps visible values and appends a single masked token if any were hidden. +// One token regardless of count prevents count-based inference about the number of hidden values. +func replaceHiddenValuesWithToken(condition *model.Condition, visibleNames map[string]struct{}) { + switch v := condition.Value.(type) { + case []any: + var result []any + hasMasked := false + for _, val := range v { + if strVal, ok := val.(string); ok { + if _, visible := visibleNames[strVal]; visible { + result = append(result, val) + } else { + hasMasked = true + } + } else { + result = append(result, val) + } + } + if hasMasked { + result = append(result, maskedTokenValue) + } + condition.Value = result + case string: + if _, visible := visibleNames[v]; !visible { + condition.Value = maskedTokenValue + } + } +} + +// MaskPolicyExpressions masks non-held literal values in all policy rule expressions, in place. +// Fails closed (sets a rule to "true") if its expression cannot be parsed or masked. +func (a *App) MaskPolicyExpressions(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) { + cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + if appErr != nil { + rctx.Logger().Warn("MaskPolicyExpressions: failed to resolve CPA group, masking all rules closed", + mlog.Err(appErr), + ) + for i, rule := range policy.Rules { + if rule.Expression == "" || rule.Expression == "true" { + continue + } + policy.Rules[i].Expression = "true" + } + return + } + cpaGroupID := cpaGroup.ID + + rctxWithCaller := RequestContextWithCallerID(rctx, callerID) + + // Parse each rule's AST once and collect all conditions so we can pre-fetch + // every referenced field in a single pass, avoiding N+1 lookups across rules. + asts := make([]*model.VisualExpression, len(policy.Rules)) + var allConditions []model.Condition + for i, rule := range policy.Rules { + if rule.Expression == "" || rule.Expression == "true" { + continue + } + ast, appErr := a.ExpressionToVisualAST(rctx, rule.Expression) + if appErr != nil { + policy.Rules[i].Expression = "true" // fail closed + continue + } + asts[i] = ast + allConditions = append(allConditions, ast.Conditions...) + } + + fieldsByName := a.fetchConditionFields(rctxWithCaller, allConditions, cpaGroupID) + + for i, ast := range asts { + if ast == nil { + continue + } + for j := range ast.Conditions { + a.maskConditionValuesWithToken(rctxWithCaller, callerID, &ast.Conditions[j], cpaGroupID, fieldsByName) + } + policy.Rules[i].Expression = buildCELFromConditions(ast.Conditions) + } +} diff --git a/server/channels/app/access_control_merge_test.go b/server/channels/app/access_control_merge_test.go new file mode 100644 index 00000000000..4f21749378d --- /dev/null +++ b/server/channels/app/access_control_merge_test.go @@ -0,0 +1,474 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package app + +import ( + "testing" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestBuildCELFromConditions(t *testing.T) { + t.Run("empty conditions returns true", func(t *testing.T) { + result := buildCELFromConditions(nil) + assert.Equal(t, "true", result) + }) + + t.Run("equals operator", func(t *testing.T) { + conditions := []model.Condition{ + {Attribute: "user.attributes.Team", Operator: "==", Value: "Engineering", ValueType: model.LiteralValue}, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `user.attributes.Team == "Engineering"`, result) + }) + + t.Run("not equals operator", func(t *testing.T) { + conditions := []model.Condition{ + {Attribute: "user.attributes.Location", Operator: "!=", Value: "Building 7", ValueType: model.LiteralValue}, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `user.attributes.Location != "Building 7"`, result) + }) + + t.Run("in operator with select field", func(t *testing.T) { + conditions := []model.Condition{ + { + Attribute: "user.attributes.Department", + Operator: "in", + Value: []any{"Sales", "Engineering", "Legal"}, + ValueType: model.LiteralValue, + AttributeType: "select", + }, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `user.attributes.Department in ["Sales", "Engineering", "Legal"]`, result) + }) + + t.Run("in operator with multiselect field", func(t *testing.T) { + conditions := []model.Condition{ + { + Attribute: "user.attributes.Programs", + Operator: "in", + Value: []any{"Alpha", "Bravo"}, + ValueType: model.LiteralValue, + AttributeType: "multiselect", + }, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `"Alpha" in user.attributes.Programs && "Bravo" in user.attributes.Programs`, result) + }) + + t.Run("hasAnyOf operator", func(t *testing.T) { + conditions := []model.Condition{ + { + Attribute: "user.attributes.Programs", + Operator: "hasAnyOf", + Value: []any{"Alpha", "Bravo"}, + ValueType: model.LiteralValue, + AttributeType: "multiselect", + }, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `("Alpha" in user.attributes.Programs || "Bravo" in user.attributes.Programs)`, result) + }) + + t.Run("hasAnyOf with single value omits parens", func(t *testing.T) { + conditions := []model.Condition{ + { + Attribute: "user.attributes.Programs", + Operator: "hasAnyOf", + Value: []any{"Alpha"}, + ValueType: model.LiteralValue, + AttributeType: "multiselect", + }, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `"Alpha" in user.attributes.Programs`, result) + }) + + t.Run("hasAllOf operator", func(t *testing.T) { + conditions := []model.Condition{ + { + Attribute: "user.attributes.Programs", + Operator: "hasAllOf", + Value: []any{"Alpha", "Bravo"}, + ValueType: model.LiteralValue, + AttributeType: "multiselect", + }, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `"Alpha" in user.attributes.Programs && "Bravo" in user.attributes.Programs`, result) + }) + + t.Run("contains operator", func(t *testing.T) { + conditions := []model.Condition{ + {Attribute: "user.attributes.Email", Operator: "contains", Value: "@company.com", ValueType: model.LiteralValue}, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `user.attributes.Email.contains("@company.com")`, result) + }) + + t.Run("startsWith operator", func(t *testing.T) { + conditions := []model.Condition{ + {Attribute: "user.attributes.Name", Operator: "startsWith", Value: "Dr.", ValueType: model.LiteralValue}, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `user.attributes.Name.startsWith("Dr.")`, result) + }) + + t.Run("endsWith operator", func(t *testing.T) { + conditions := []model.Condition{ + {Attribute: "user.attributes.Email", Operator: "endsWith", Value: ".gov", ValueType: model.LiteralValue}, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `user.attributes.Email.endsWith(".gov")`, result) + }) + + t.Run("multiple conditions joined with &&", func(t *testing.T) { + conditions := []model.Condition{ + {Attribute: "user.attributes.Team", Operator: "==", Value: "Engineering", ValueType: model.LiteralValue}, + {Attribute: "user.attributes.Location", Operator: "!=", Value: "Remote", ValueType: model.LiteralValue}, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `user.attributes.Team == "Engineering" && user.attributes.Location != "Remote"`, result) + }) + + t.Run("string with special characters is escaped", func(t *testing.T) { + conditions := []model.Condition{ + {Attribute: "user.attributes.Team", Operator: "==", Value: `Team "Alpha"`, ValueType: model.LiteralValue}, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `user.attributes.Team == "Team \"Alpha\""`, result) + }) + + t.Run("boolean value", func(t *testing.T) { + conditions := []model.Condition{ + {Attribute: "user.attributes.Active", Operator: "==", Value: true, ValueType: model.LiteralValue}, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, `user.attributes.Active == true`, result) + }) + + t.Run("empty in-list produces no output", func(t *testing.T) { + conditions := []model.Condition{ + {Attribute: "user.attributes.Department", Operator: "in", Value: []any{}, ValueType: model.LiteralValue, AttributeType: "select"}, + } + result := buildCELFromConditions(conditions) + assert.Equal(t, "true", result) + }) + + t.Run("masked token in values produces valid CEL", func(t *testing.T) { + conds := []model.Condition{{ + Attribute: "user.attributes.Program", + Operator: "in", + Value: []any{"Alpha", maskedTokenValue}, + ValueType: model.LiteralValue, + AttributeType: "select", + }} + result := buildCELFromConditions(conds) + assert.Contains(t, result, "Alpha") + assert.Contains(t, result, maskedTokenValue) + }) +} + +func TestNormalizeForComparison(t *testing.T) { + t.Run("strips whitespace outside string literals", func(t *testing.T) { + got, ok := normalizeForComparison(` a == "b" `) + require.True(t, ok) + assert.Equal(t, `a=="b"`, got) + }) + + t.Run("preserves whitespace inside string literals", func(t *testing.T) { + got, ok := normalizeForComparison(`a == "hello world"`) + require.True(t, ok) + assert.Equal(t, `a=="hello world"`, got) + }) + + t.Run("canonicalizes single quotes to double quotes", func(t *testing.T) { + single, ok := normalizeForComparison(`a == 'foo'`) + require.True(t, ok) + double, ok := normalizeForComparison(`a == "foo"`) + require.True(t, ok) + assert.Equal(t, single, double) + }) + + t.Run("preserves escape sequences inside string literals", func(t *testing.T) { + got, ok := normalizeForComparison(`a == "he said \"hi\""`) + require.True(t, ok) + assert.Equal(t, `a=="he said \"hi\""`, got) + }) + + t.Run("unbalanced quote returns ok=false", func(t *testing.T) { + _, ok := normalizeForComparison(`a == "unterminated`) + assert.False(t, ok) + }) + + t.Run("empty string normalizes to empty", func(t *testing.T) { + got, ok := normalizeForComparison("") + require.True(t, ok) + assert.Equal(t, "", got) + }) +} + +func TestIsVisualASTRepresentable(t *testing.T) { + t.Run("empty AST on empty expression is representable", func(t *testing.T) { + assert.True(t, isVisualASTRepresentable("", &model.VisualExpression{})) + }) + + t.Run("empty AST on 'true' is representable", func(t *testing.T) { + assert.True(t, isVisualASTRepresentable("true", &model.VisualExpression{})) + }) + + t.Run("simple equals condition round-trips cleanly", func(t *testing.T) { + ast := &model.VisualExpression{Conditions: []model.Condition{ + {Attribute: "user.attributes.team", Operator: "==", Value: "Engineering", ValueType: model.LiteralValue}, + }} + assert.True(t, isVisualASTRepresentable(`user.attributes.team == "Engineering"`, ast)) + }) + + t.Run("simple AND chain of two conditions round-trips cleanly", func(t *testing.T) { + ast := &model.VisualExpression{Conditions: []model.Condition{ + {Attribute: "user.attributes.team", Operator: "==", Value: "Engineering", ValueType: model.LiteralValue}, + {Attribute: "user.attributes.role", Operator: "==", Value: "Admin", ValueType: model.LiteralValue}, + }} + assert.True(t, isVisualASTRepresentable( + `user.attributes.team == "Engineering" && user.attributes.role == "Admin"`, + ast, + )) + }) + + t.Run("|| in original but AST flattens to AND is NOT representable", func(t *testing.T) { + // Pretend the parser flattened `a == "X" || b == "Y"` into two AND-joined + // conditions. The round-trip would emit `&&`, mismatch detected. + ast := &model.VisualExpression{Conditions: []model.Condition{ + {Attribute: "user.attributes.team", Operator: "==", Value: "X", ValueType: model.LiteralValue}, + {Attribute: "user.attributes.role", Operator: "==", Value: "Y", ValueType: model.LiteralValue}, + }} + assert.False(t, isVisualASTRepresentable( + `user.attributes.team == "X" || user.attributes.role == "Y"`, + ast, + )) + }) + + t.Run("grouping in original is NOT representable when AST flattens it", func(t *testing.T) { + ast := &model.VisualExpression{Conditions: []model.Condition{ + {Attribute: "user.attributes.a", Operator: "==", Value: "1", ValueType: model.LiteralValue}, + {Attribute: "user.attributes.b", Operator: "==", Value: "2", ValueType: model.LiteralValue}, + {Attribute: "user.attributes.c", Operator: "==", Value: "3", ValueType: model.LiteralValue}, + }} + assert.False(t, isVisualASTRepresentable( + `(user.attributes.a == "1" && user.attributes.b == "2") || user.attributes.c == "3"`, + ast, + )) + }) + + t.Run("hasAnyOf with multiple values is representable (|| within a single condition)", func(t *testing.T) { + // `("Alpha" in attr || "Bravo" in attr)` is the canonical serialization + // for a hasAnyOf condition. It contains || syntactically but the AST + // reduces it to one condition that round-trips identically. + ast := &model.VisualExpression{Conditions: []model.Condition{ + { + Attribute: "user.attributes.Programs", + Operator: "hasAnyOf", + Value: []any{"Alpha", "Bravo"}, + ValueType: model.LiteralValue, + AttributeType: "multiselect", + }, + }} + assert.True(t, isVisualASTRepresentable( + `("Alpha" in user.attributes.Programs || "Bravo" in user.attributes.Programs)`, + ast, + )) + }) + + t.Run("unbalanced quote in original is NOT representable", func(t *testing.T) { + ast := &model.VisualExpression{Conditions: []model.Condition{ + {Attribute: "user.attributes.team", Operator: "==", Value: "X", ValueType: model.LiteralValue}, + }} + assert.False(t, isVisualASTRepresentable(`user.attributes.team == "unterminated`, ast)) + }) +} + +func TestExtractStringValues(t *testing.T) { + t.Run("slice of strings", func(t *testing.T) { + result := extractStringValues([]any{"Alpha", "Bravo", "Charlie"}) + assert.Equal(t, []string{"Alpha", "Bravo", "Charlie"}, result) + }) + + t.Run("single string", func(t *testing.T) { + result := extractStringValues("Alpha") + assert.Equal(t, []string{"Alpha"}, result) + }) + + t.Run("nil", func(t *testing.T) { + result := extractStringValues(nil) + assert.Nil(t, result) + }) + + t.Run("mixed types in slice", func(t *testing.T) { + result := extractStringValues([]any{"Alpha", 42, "Bravo"}) + assert.Equal(t, []string{"Alpha", "Bravo"}, result) + }) + + t.Run("non-string non-slice", func(t *testing.T) { + result := extractStringValues(42) + assert.Nil(t, result) + }) +} + +func TestCelStringLiteral(t *testing.T) { + assert.Equal(t, `"hello"`, celStringLiteral("hello")) + assert.Equal(t, `"hello \"world\""`, celStringLiteral(`hello "world"`)) + assert.Equal(t, `"path\\to\\file"`, celStringLiteral(`path\to\file`)) + assert.Equal(t, `""`, celStringLiteral("")) + + // Control characters must be escaped or the emitted CEL literal won't parse. + assert.Equal(t, `"line1\nline2"`, celStringLiteral("line1\nline2")) + assert.Equal(t, `"col1\tcol2"`, celStringLiteral("col1\tcol2")) + assert.Equal(t, `"carriage\rreturn"`, celStringLiteral("carriage\rreturn")) +} + +func TestCelValueLiteral(t *testing.T) { + assert.Equal(t, `"hello"`, celValueLiteral("hello")) + assert.Equal(t, "true", celValueLiteral(true)) + assert.Equal(t, "false", celValueLiteral(false)) + assert.Equal(t, "42", celValueLiteral(int(42))) + assert.Equal(t, "42", celValueLiteral(int64(42))) + assert.Equal(t, "3.14", celValueLiteral(float64(3.14))) + assert.Equal(t, "null", celValueLiteral(nil)) + + // Float precision must round-trip — %f would round 0.123456789 to 0.123457. + assert.Equal(t, "0.123456789", celValueLiteral(float64(0.123456789))) +} + +func TestContainsNonStringLiteral(t *testing.T) { + assert.False(t, containsNonStringLiteral(nil)) + assert.False(t, containsNonStringLiteral("Alpha")) + assert.False(t, containsNonStringLiteral([]any{"Alpha", "Bravo"})) + + assert.True(t, containsNonStringLiteral(float64(1))) + assert.True(t, containsNonStringLiteral(true)) + assert.True(t, containsNonStringLiteral(int64(7))) + assert.True(t, containsNonStringLiteral([]any{"Alpha", 1.0})) +} + +func TestConditionToCEL_NilValue(t *testing.T) { + // A condition whose Value was masked to nil (e.g. all options hidden) must be dropped + // rather than emitting `attr == null`, which is invalid CEL for string attributes. + nilValueOps := []string{"==", "!=", ">", ">=", "<", "<=", "contains", "startsWith", "endsWith", "unknownOp"} + for _, op := range nilValueOps { + cond := model.Condition{ + Attribute: "user.attributes.Clearance", + Operator: op, + Value: nil, + ValueType: model.LiteralValue, + } + assert.Equal(t, "", conditionToCEL(cond), "operator %q with nil value must produce empty string", op) + } +} + +func TestConditionToCEL_UnknownOperatorWithValue(t *testing.T) { + // An unknown operator with a non-nil value produces a best-effort CEL expression. + // buildCELFromConditions will include it as-is; if the operator is truly unknown + // the downstream CEL engine will reject the expression during validation. + // This documents the intended (pass-through) behaviour for forward-compatibility. + cond := model.Condition{ + Attribute: "user.attributes.Clearance", + Operator: "futureOp", + Value: "Secret", + ValueType: model.LiteralValue, + } + result := conditionToCEL(cond) + assert.Equal(t, `user.attributes.Clearance futureOp "Secret"`, result) +} + +func TestMergeConditionValues(t *testing.T) { + t.Run("no hidden values returns submitted as-is", func(t *testing.T) { + submitted := model.Condition{Attribute: "user.attributes.Program", Operator: "in", Value: []any{"Alpha"}} + result := mergeConditionValues(submitted, nil) + assert.Equal(t, []any{"Alpha"}, result.Value) + }) + + t.Run("appends hidden values without duplicates", func(t *testing.T) { + submitted := model.Condition{Attribute: "user.attributes.Program", Operator: "in", Value: []any{"Alpha"}} + result := mergeConditionValues(submitted, []string{"Bravo", "Charlie"}) + values, ok := result.Value.([]any) + require.True(t, ok) + assert.Len(t, values, 3) + assert.Contains(t, values, "Alpha") + assert.Contains(t, values, "Bravo") + assert.Contains(t, values, "Charlie") + }) + + t.Run("deduplicates overlapping values", func(t *testing.T) { + submitted := model.Condition{Attribute: "user.attributes.Program", Operator: "in", Value: []any{"Alpha", "Bravo"}} + result := mergeConditionValues(submitted, []string{"Bravo", "Charlie"}) + values, ok := result.Value.([]any) + require.True(t, ok) + assert.Len(t, values, 3) + }) + + t.Run("restores hidden values when submitted is nil (fully-masked placeholder)", func(t *testing.T) { + submitted := model.Condition{Attribute: "user.attributes.Program", Operator: "in", Value: nil} + result := mergeConditionValues(submitted, []string{"Bravo", "Charlie"}) + values, ok := result.Value.([]any) + require.True(t, ok) + assert.Len(t, values, 2) + }) + + t.Run("restores single hidden value when submitted is nil", func(t *testing.T) { + submitted := model.Condition{Attribute: "user.attributes.Location", Operator: "==", Value: nil} + result := mergeConditionValues(submitted, []string{"Building 7"}) + assert.Equal(t, "Building 7", result.Value) + }) +} + +func TestGetHiddenValues(t *testing.T) { + var a *App + + options := []any{ + map[string]any{"id": "id1", "name": "Alpha"}, + map[string]any{"id": "id2", "name": "Bravo"}, + } + makeField := func(accessMode string, fieldType model.PropertyFieldType) *model.PropertyField { + attrs := model.StringInterface{model.PropertyAttrsAccessMode: accessMode} + if fieldType == model.PropertyFieldTypeSelect || fieldType == model.PropertyFieldTypeMultiselect { + attrs[model.PropertyFieldAttributeOptions] = options + } + return &model.PropertyField{Type: fieldType, Attrs: attrs} + } + + t.Run("AttrValue condition: returns nil immediately", func(t *testing.T) { + stored := &model.Condition{Attribute: "user.attributes.Team", Value: "user.attributes.Dept", ValueType: model.AttrValue} + assert.Nil(t, a.getHiddenValues(nil, "caller", stored, "", nil)) + }) + + t.Run("field missing from prefetch map: returns nil (fail closed)", func(t *testing.T) { + stored := &model.Condition{Attribute: "user.attributes.Program", Value: []any{"Alpha", "Bravo"}, ValueType: model.LiteralValue} + assert.Nil(t, a.getHiddenValues(nil, "caller", stored, "", map[string]*model.PropertyField{})) + }) + + t.Run("source_only: all stored values treated as hidden", func(t *testing.T) { + stored := &model.Condition{Attribute: "user.attributes.Clearance", Value: []any{"Top Secret", "Secret"}, ValueType: model.LiteralValue} + fields := map[string]*model.PropertyField{"Clearance": makeField(model.PropertyAccessModeSourceOnly, model.PropertyFieldTypeSelect)} + result := a.getHiddenValues(nil, "caller", stored, "", fields) + assert.Equal(t, []string{"Top Secret", "Secret"}, result) + }) + + t.Run("shared_only select: values absent from options are hidden", func(t *testing.T) { + stored := &model.Condition{Attribute: "user.attributes.Program", Value: []any{"Alpha", "Charlie"}, ValueType: model.LiteralValue} + fields := map[string]*model.PropertyField{"Program": makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeSelect)} + result := a.getHiddenValues(nil, "caller", stored, "", fields) + assert.Equal(t, []string{"Charlie"}, result) + }) + + t.Run("public field: no values hidden", func(t *testing.T) { + stored := &model.Condition{Attribute: "user.attributes.Dept", Value: []any{"Eng", "Sales"}, ValueType: model.LiteralValue} + fields := map[string]*model.PropertyField{"Dept": makeField(model.PropertyAccessModePublic, model.PropertyFieldTypeSelect)} + result := a.getHiddenValues(nil, "caller", stored, "", fields) + assert.Nil(t, result) + }) +} diff --git a/server/channels/app/access_control_test.go b/server/channels/app/access_control_test.go index 4f8c11bbe79..fa5e6ca210c 100644 --- a/server/channels/app/access_control_test.go +++ b/server/channels/app/access_control_test.go @@ -319,6 +319,156 @@ func TestDeleteAccessControlPolicy(t *testing.T) { mockChannelStore.AssertNotCalled(t, "InvalidateChannel", mock.Anything) mockChannelStore.AssertNotCalled(t, "Get", mock.Anything, mock.Anything) }) + + t.Run("Caller with masked values is blocked from deleting (403)", func(t *testing.T) { + // When AttributeValueMasking is on and the caller cannot see all values in the + // policy, the delete must be refused with the masked_values 403. This closes + // the gap where a delegated admin could remove a policy whose conditions they + // could not audit. Forcing an unknown-field reference in the rule makes + // GetMaskedVisualAST fail-closed (HasMaskedValues=true) without requiring a + // full CPA setup for the test. + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.AttributeBasedAccessControl = true + cfg.FeatureFlags.AttributeValueMasking = true + }).InitBasic(t) + + callerID := model.NewId() + th.Context = th.Context.WithSession(&model.Session{UserId: callerID, Id: model.NewId()}).(*request.Context) + + policyID := model.NewId() + sensitivePolicy := &model.AccessControlPolicy{ + ID: policyID, + Type: model.AccessControlPolicyTypeChannel, + Version: model.AccessControlPolicyVersionV0_3, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.f_unknown_field == "Secret"`}, + }, + } + + mockAccessControl := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockAccessControl + mockAccessControl.On("GetPolicy", th.Context, policyID).Return(sensitivePolicy, nil).Once() + // Force GetMaskedVisualAST → maskConditionValues → fail-closed (unknown field). + mockAccessControl.On("ExpressionToVisualAST", mock.Anything, mock.Anything).Return(&model.VisualExpression{ + Conditions: []model.Condition{ + {Attribute: "user.attributes.f_unknown_field", Operator: "==", Value: "Secret", ValueType: model.LiteralValue}, + }, + }, nil).Maybe() + + appErr := th.App.DeleteAccessControlPolicy(th.Context, policyID) + require.NotNil(t, appErr) + require.Equal(t, http.StatusForbidden, appErr.StatusCode) + require.Equal(t, "app.pap.delete_policy.masked_values", appErr.Id) + + mockAccessControl.AssertNotCalled(t, "DeletePolicy", mock.Anything, mock.Anything) + mockAccessControl.AssertExpectations(t) + }) + + t.Run("Masking flag off: delete proceeds for callers that would otherwise be blocked", func(t *testing.T) { + // Belt-and-braces: with AttributeValueMasking off, the masking guard must not + // fire — the policy deletes normally even if the caller wouldn't have seen all + // values. Guards against accidentally inverting the flag condition. + thMock := SetupWithStoreMock(t) + // Note: SetupWithStoreMock doesn't take a config callback. Feature flags + // default to false, which is exactly the state this test wants. + + thMock.Context = thMock.Context.WithSession(&model.Session{UserId: model.NewId(), Id: model.NewId()}).(*request.Context) + + channelID := model.NewId() + channelPolicy := &model.AccessControlPolicy{ + ID: channelID, + Type: model.AccessControlPolicyTypeChannel, + Version: model.AccessControlPolicyVersionV0_3, + } + + mockStore := thMock.App.Srv().Store().(*storemocks.Store) + mockChannelStore := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannelStore) + mockChannelStore.On("InvalidateChannel", channelID).Once() + mockChannelStore.On("Get", channelID, true).Return(&model.Channel{Id: channelID, Type: model.ChannelTypePrivate}, nil).Once() + + mockAccessControl := &mocks.AccessControlServiceInterface{} + thMock.App.Srv().ch.AccessControl = mockAccessControl + mockAccessControl.On("GetPolicy", thMock.Context, channelID).Return(channelPolicy, nil).Once() + mockAccessControl.On("DeletePolicy", thMock.Context, channelID).Return(nil).Once() + + appErr := thMock.App.DeleteAccessControlPolicy(thMock.Context, channelID) + require.Nil(t, appErr) + mockAccessControl.AssertExpectations(t) + mockChannelStore.AssertExpectations(t) + }) +} + +// TestCheckSelfInclusion verifies the self-exclusion guard: non-admin callers must +// satisfy their own policy after saving, or the save is refused with 403 +// self_exclusion. Sysadmins are exempt at the call site +// (CreateOrUpdateAccessControlPolicy), not inside checkSelfInclusion itself — this +// test exercises the function directly. +func TestCheckSelfInclusion(t *testing.T) { + t.Run("caller who satisfies the policy passes", func(t *testing.T) { + th := Setup(t).InitBasic(t) + callerID := th.BasicUser.Id + + policy := &model.AccessControlPolicy{ + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.team == "ops"`}, + }, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + // QueryUsersForExpression returns the caller → matches → no error. + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return([]*model.User{{Id: callerID}}, int64(1), nil).Once() + + appErr := th.App.checkSelfInclusion(th.Context, policy, callerID) + require.Nil(t, appErr) + mockACS.AssertExpectations(t) + }) + + t.Run("caller who does not satisfy the policy is rejected with 403", func(t *testing.T) { + th := Setup(t).InitBasic(t) + callerID := th.BasicUser.Id + + policy := &model.AccessControlPolicy{ + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.team == "ops"`}, + }, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + // No users returned → caller does not satisfy → expect self_exclusion 403. + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return([]*model.User{}, int64(0), nil).Once() + + appErr := th.App.checkSelfInclusion(th.Context, policy, callerID) + require.NotNil(t, appErr) + require.Equal(t, http.StatusForbidden, appErr.StatusCode) + require.Equal(t, "app.pap.save_policy.self_exclusion", appErr.Id) + mockACS.AssertExpectations(t) + }) + + t.Run("trivial rules (empty / 'true') are skipped without querying", func(t *testing.T) { + th := Setup(t).InitBasic(t) + callerID := th.BasicUser.Id + + policy := &model.AccessControlPolicy{ + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: ""}, + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: "true"}, + }, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + // No query should fire for trivial expressions — if it does, the mock will fail + // the test by returning the default zero-value response. + + appErr := th.App.checkSelfInclusion(th.Context, policy, callerID) + require.Nil(t, appErr) + mockACS.AssertNotCalled(t, "QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything) + }) } func TestGetChannelsForPolicy(t *testing.T) { diff --git a/server/channels/app/access_control_validation_test.go b/server/channels/app/access_control_validation_test.go new file mode 100644 index 00000000000..05823ef7743 --- /dev/null +++ b/server/channels/app/access_control_validation_test.go @@ -0,0 +1,231 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package app + +import ( + "encoding/json" + "testing" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/request" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestInvalidValueError(t *testing.T) { + err := invalidValueError() + require.NotNil(t, err) + assert.Equal(t, "app.pap.save_policy.invalid_value", err.Id) + assert.Equal(t, 400, err.StatusCode) + assert.Equal(t, "Invalid value.", err.DetailedError) +} + +func TestMaskedTokenConstant(t *testing.T) { + // The masked-token sentinel must be the eight-dash string the frontend + // renders for hidden chips and the server emits when masking raw CEL + // on GET / search responses. + assert.Equal(t, "--------", maskedTokenValue) +} + +func TestGenericErrorConsistency(t *testing.T) { + // All rejection reasons must produce identical errors to prevent enumeration. + err1 := invalidValueError() + err2 := invalidValueError() + + assert.Equal(t, err1.Id, err2.Id) + assert.Equal(t, err1.StatusCode, err2.StatusCode) + assert.Equal(t, err1.DetailedError, err2.DetailedError) +} + +func TestValidateConditionValues(t *testing.T) { + rctx := request.TestContext(t) + + // nil App is safe for every branch except shared_only + text (which calls + // a.getCallerTextValues → a.SearchPropertyValues). Those paths are covered + // by the integration tests in access_control_test.go. + var a *App + + makeField := func(accessMode string, fieldType model.PropertyFieldType, options []any) *model.PropertyField { + attrs := model.StringInterface{model.PropertyAttrsAccessMode: accessMode} + if options != nil { + attrs[model.PropertyFieldAttributeOptions] = options + } + return &model.PropertyField{Name: "Team", Type: fieldType, Attrs: attrs} + } + + selectOptions := []any{ + map[string]any{"id": "id1", "name": "Alpha"}, + map[string]any{"id": "id2", "name": "Bravo"}, + } + + t.Run("AttrValue conditions are skipped (no literals to validate)", func(t *testing.T) { + cond := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "==", + Value: "user.attributes.Department", + ValueType: model.AttrValue, + } + err := a.validateConditionValues(rctx, cond, "groupID", nil) + assert.Nil(t, err) + }) + + t.Run("non-attribute references are skipped", func(t *testing.T) { + cond := &model.Condition{ + Attribute: "channel.id", + Operator: "==", + Value: "X", + ValueType: model.LiteralValue, + } + err := a.validateConditionValues(rctx, cond, "groupID", nil) + assert.Nil(t, err) + }) + + t.Run("unknown field is rejected with the generic error", func(t *testing.T) { + cond := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "==", + Value: "Alpha", + ValueType: model.LiteralValue, + } + err := a.validateConditionValues(rctx, cond, "groupID", map[string]*model.PropertyField{}) + require.NotNil(t, err) + assert.Equal(t, "app.pap.save_policy.invalid_value", err.Id) + }) + + t.Run("public field allows any value", func(t *testing.T) { + field := makeField(model.PropertyAccessModePublic, model.PropertyFieldTypeSelect, selectOptions) + fields := map[string]*model.PropertyField{"Team": field} + cond := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "==", + Value: "anything", + ValueType: model.LiteralValue, + } + err := a.validateConditionValues(rctx, cond, "groupID", fields) + assert.Nil(t, err) + }) + + t.Run("source_only field rejects any literal value", func(t *testing.T) { + field := makeField(model.PropertyAccessModeSourceOnly, model.PropertyFieldTypeSelect, selectOptions) + fields := map[string]*model.PropertyField{"Team": field} + cond := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "==", + Value: "Alpha", + ValueType: model.LiteralValue, + } + err := a.validateConditionValues(rctx, cond, "groupID", fields) + require.NotNil(t, err) + assert.Equal(t, "app.pap.save_policy.invalid_value", err.Id) + }) + + t.Run("source_only field allows the masked-token sentinel (round-tripped from GET)", func(t *testing.T) { + field := makeField(model.PropertyAccessModeSourceOnly, model.PropertyFieldTypeSelect, selectOptions) + fields := map[string]*model.PropertyField{"Team": field} + cond := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "==", + Value: maskedTokenValue, + ValueType: model.LiteralValue, + } + err := a.validateConditionValues(rctx, cond, "groupID", fields) + assert.Nil(t, err, "the sentinel is stripped/re-injected at merge; validation must let it through") + }) + + t.Run("shared_only select: held value passes, non-held rejected, token allowed", func(t *testing.T) { + field := makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeSelect, selectOptions) + fields := map[string]*model.PropertyField{"Team": field} + + // "Alpha" is in the visible-options set (caller holds it) + ok := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "==", + Value: "Alpha", + ValueType: model.LiteralValue, + } + require.Nil(t, a.validateConditionValues(rctx, ok, "groupID", fields)) + + // "Charlie" is not in the visible-options set → rejected + bad := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "==", + Value: "Charlie", + ValueType: model.LiteralValue, + } + err := a.validateConditionValues(rctx, bad, "groupID", fields) + require.NotNil(t, err) + assert.Equal(t, "app.pap.save_policy.invalid_value", err.Id) + + // Masked-token sentinel passes through (handled by merge, not validation) + tokenCond := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "==", + Value: maskedTokenValue, + ValueType: model.LiteralValue, + } + assert.Nil(t, a.validateConditionValues(rctx, tokenCond, "groupID", fields)) + }) + + t.Run("source_only field rejects non-string literals (numeric, bool)", func(t *testing.T) { + field := makeField(model.PropertyAccessModeSourceOnly, model.PropertyFieldTypeSelect, selectOptions) + fields := map[string]*model.PropertyField{"Team": field} + cond := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "==", + Value: float64(1), + ValueType: model.LiteralValue, + } + err := a.validateConditionValues(rctx, cond, "groupID", fields) + require.NotNil(t, err, "numeric literal must not slip through extractStringValues silently") + assert.Equal(t, "app.pap.save_policy.invalid_value", err.Id) + }) + + t.Run("shared_only field rejects non-string literals", func(t *testing.T) { + field := makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeSelect, selectOptions) + fields := map[string]*model.PropertyField{"Team": field} + cond := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "==", + Value: true, + ValueType: model.LiteralValue, + } + err := a.validateConditionValues(rctx, cond, "groupID", fields) + require.NotNil(t, err) + assert.Equal(t, "app.pap.save_policy.invalid_value", err.Id) + }) + + t.Run("shared_only multiselect: array values are validated element by element", func(t *testing.T) { + field := makeField(model.PropertyAccessModeSharedOnly, model.PropertyFieldTypeMultiselect, selectOptions) + fields := map[string]*model.PropertyField{"Team": field} + + allHeld, _ := json.Marshal([]any{"Alpha", "Bravo"}) + cond := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "in", + Value: parseAny(t, allHeld), + ValueType: model.LiteralValue, + } + require.Nil(t, a.validateConditionValues(rctx, cond, "groupID", fields)) + + mixed, _ := json.Marshal([]any{"Alpha", "Charlie"}) + cond2 := &model.Condition{ + Attribute: "user.attributes.Team", + Operator: "in", + Value: parseAny(t, mixed), + ValueType: model.LiteralValue, + } + err := a.validateConditionValues(rctx, cond2, "groupID", fields) + require.NotNil(t, err, "any non-held element must trigger rejection") + assert.Equal(t, "app.pap.save_policy.invalid_value", err.Id) + }) +} + +// parseAny round-trips a JSON-encoded value back to the untyped interface{} +// shape that the visual-AST parser produces for condition.Value. +func parseAny(t *testing.T, raw []byte) any { + t.Helper() + var v any + require.NoError(t, json.Unmarshal(raw, &v)) + return v +} diff --git a/server/i18n/en.json b/server/i18n/en.json index b46a9eb05d3..782435e4a87 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -7492,6 +7492,10 @@ "id": "app.pap.delete_policy.app_error", "translation": "Unable to delete access control policy." }, + { + "id": "app.pap.delete_policy.masked_values", + "translation": "You cannot delete this policy because it contains attribute values you do not have permission to view." + }, { "id": "app.pap.expression_to_visual_ast.app_error", "translation": "Could not genereate visual AST from expression." @@ -7536,6 +7540,10 @@ "id": "app.pap.is_ready.app_error", "translation": "Access control service is not ready." }, + { + "id": "app.pap.merge_expression.app_error", + "translation": "Could not merge policy expression." + }, { "id": "app.pap.missing_attribute.app_error", "translation": "An attribute is missing from the expression." @@ -7548,14 +7556,34 @@ "id": "app.pap.query_expression.app_error", "translation": "Could not query for expression." }, + { + "id": "app.pap.save_policy.advanced_expression_blocked", + "translation": "This rule expression cannot be safely edited while restricted values are present." + }, { "id": "app.pap.save_policy.app_error", "translation": "Unable to save access control policy." }, + { + "id": "app.pap.save_policy.invalid_value", + "translation": "Invalid value." + }, + { + "id": "app.pap.save_policy.masked_condition_deleted", + "translation": "You cannot remove a condition that contains attribute values you do not have permission to view." + }, + { + "id": "app.pap.save_policy.masked_rule_deleted", + "translation": "You cannot remove a rule that contains attribute values you do not have permission to view." + }, { "id": "app.pap.save_policy.name_exists.app_error", "translation": "A policy with this name already exists. Please choose a different name." }, + { + "id": "app.pap.save_policy.self_exclusion", + "translation": "You do not satisfy one or more conditions in this policy." + }, { "id": "app.pap.search_access_control_policies.app_error", "translation": "Could not search access control policies." @@ -7568,6 +7596,10 @@ "id": "app.pap.update_access_control_policies_active.app_error", "translation": "Could not update active status of access control policies." }, + { + "id": "app.pap.validate_expression_values.app_error", + "translation": "Could not validate policy expression values." + }, { "id": "app.pdp.access_evaluation.app_error", "translation": "Failed evaluate access control policy." From 9bd77d3fc4d2af3f7f0259a205174e76a1a276e1 Mon Sep 17 00:00:00 2001 From: Julien Tant <785518+JulienTant@users.noreply.github.com> Date: Mon, 18 May 2026 09:01:00 -0700 Subject: [PATCH 3/3] MM-68702: Reject demoting bot accounts to guest (#36487) * MM-68702: Reject demoting bot accounts to guest Deny DemoteUserToGuest when the target is a bot so User Managers cannot degrade bot capabilities via guest conversion without bot administration permissions. Adds API error string and tests. Co-authored-by: Julien Tant * Fix TestDemoteUserToGuest bot subtest: enable bot creation in config Default test config disables bot accounts; enable ServiceSettings EnableBotAccountCreation for the subtest and restore afterward. Co-authored-by: Julien Tant --------- Co-authored-by: Cursor Agent Co-authored-by: Julien Tant --- server/channels/api4/user_test.go | 28 ++++++++++++++++++++++++++++ server/channels/app/user.go | 4 ++++ server/channels/app/user_test.go | 12 ++++++++++++ server/i18n/en.json | 4 ++++ 4 files changed, 48 insertions(+) diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index fddc86cb308..c0024292995 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -6961,6 +6961,34 @@ func TestDemoteUserToGuest(t *testing.T) { require.NoError(t, err) }) + t.Run("cannot demote bot account", func(t *testing.T) { + th.App.Srv().SetLicense(model.NewTestLicense("guest_accounts")) + + prevBotCreation := *th.App.Config().ServiceSettings.EnableBotAccountCreation + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableBotAccountCreation = true + }) + defer th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableBotAccountCreation = prevBotCreation + }) + + createdBot, resp, err := th.SystemAdminClient.CreateBot(context.Background(), &model.Bot{ + Username: "botdemote" + model.NewId(), + DisplayName: "Demote Test Bot", + Description: "test", + }) + require.NoError(t, err) + CheckCreatedStatus(t, resp) + defer func() { + appErr := th.App.PermanentDeleteBot(th.Context, createdBot.UserId) + require.Nil(t, appErr) + }() + + demoteResp, err := th.SystemAdminClient.DemoteUserToGuest(context.Background(), createdBot.UserId) + CheckBadRequestStatus(t, demoteResp) + CheckErrorID(t, err, "api.user.demote_user_to_guest.bot_not_allowed.app_error") + }) + th.TestForSystemAdminAndLocal(t, func(t *testing.T, c *model.Client4) { _, _, err := c.GetUser(context.Background(), user.Id, "") require.NoError(t, err) diff --git a/server/channels/app/user.go b/server/channels/app/user.go index 325e3bdd1a1..22060540f07 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -2742,6 +2742,10 @@ func (a *App) PromoteGuestToUser(rctx request.CTX, user *model.User, requestorId // DemoteUserToGuest Convert user's roles and all his membership's roles from // regular user roles to guest roles. func (a *App) DemoteUserToGuest(rctx request.CTX, user *model.User) *model.AppError { + if user.IsBot { + return model.NewAppError("DemoteUserToGuest", "api.user.demote_user_to_guest.bot_not_allowed.app_error", nil, "", http.StatusBadRequest) + } + demotedUser, nErr := a.ch.srv.userService.DemoteUserToGuest(user) a.InvalidateCacheForUser(user.Id) if nErr != nil { diff --git a/server/channels/app/user_test.go b/server/channels/app/user_test.go index 88b7b19461e..f82cd9915c6 100644 --- a/server/channels/app/user_test.go +++ b/server/channels/app/user_test.go @@ -2012,6 +2012,18 @@ func TestDemoteUserToGuest(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) + t.Run("Must reject bot user", func(t *testing.T) { + bot := th.CreateBot(t) + user, err := th.App.GetUser(bot.UserId) + require.Nil(t, err) + require.True(t, user.IsBot) + + appErr := th.App.DemoteUserToGuest(th.Context, user) + require.NotNil(t, appErr) + assert.Equal(t, "api.user.demote_user_to_guest.bot_not_allowed.app_error", appErr.Id) + assert.Equal(t, http.StatusBadRequest, appErr.StatusCode) + }) + t.Run("Must invalidate channel stats cache when demoting a user", func(t *testing.T) { user := th.CreateUser(t) require.Equal(t, "system_user", user.Roles) diff --git a/server/i18n/en.json b/server/i18n/en.json index 782435e4a87..01bb249870f 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -4654,6 +4654,10 @@ "id": "api.user.demote_user_to_guest.already_guest.app_error", "translation": "Unable to convert the user to guest because is already a guest." }, + { + "id": "api.user.demote_user_to_guest.bot_not_allowed.app_error", + "translation": "Bot accounts cannot be converted to guest accounts." + }, { "id": "api.user.email_to_ldap.not_available.app_error", "translation": "AD/LDAP not available on this server."