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/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/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/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/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/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/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 b46a9eb05d3..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." @@ -7492,6 +7496,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 +7544,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 +7560,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 +7600,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." 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) {