diff --git a/.cursor/README.md b/.cursor/README.md index 93596cb5686..192cc60c907 100644 --- a/.cursor/README.md +++ b/.cursor/README.md @@ -17,7 +17,7 @@ The Docker build context is `.cursor/` only. The Dockerfile intentionally does n ## Runtime Hooks - `cloud-agent-install.sh` runs after Cursor checks out the repo. It refreshes nvm, installs agent-browser browsers, verifies Cursor's multi-repo `mattermost/enterprise` checkout, runs `server` Go dependency hydration, installs webapp dependencies, and runs Playwright `npm ci`. -- `cloud-agent-start.sh` materializes `.cursor/cursor.md` as `.cursor/AGENTS.md`, fixes current-session Docker socket access, then starts Docker and waits until `docker info` and `docker compose version` succeed. +- `cloud-agent-start.sh` materializes `.cursor/cursor.md` as `.cursor/AGENTS.md`, fixes current-session Docker socket access, starts Docker, waits until `docker info` and `docker compose version` succeed, then logs in to Docker Hub when credentials are configured. The environment declares `github.com/mattermost/enterprise` in `repositoryDependencies` so Cursor can provide it as part of the multi-repo workspace. Cursor currently clones the repositories as siblings, such as `/agent/repos/mattermost` and `/agent/repos/enterprise`, which matches `server/Makefile`'s default `../../enterprise` path. The install hook does not clone, pull, or symlink enterprise. @@ -33,4 +33,7 @@ Set these environment variables to `true` to shorten startup for narrow tasks: ## Expected Secrets -- AWS uploads use the standard AWS CLI environment variables provided to the Cloud Agent: `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, and `AWS_S3_BUCKET_NAME`. The image only supplies the `aws` binary. +Configure these in the [Cursor Cloud Agents dashboard](https://cursor.com/dashboard/cloud-agents) as environment-scoped secrets for the Mattermost Cloud Agent environment. + +- AWS uploads use the standard AWS CLI environment variables: `AWS_ACCESS_KEY_ID`, `AWS_SECRET_ACCESS_KEY`, and `AWS_S3_BUCKET_NAME`. The image only supplies the `aws` binary. +- Docker Hub pulls use the same variable names as CI: `DOCKERHUB_USERNAME` and `DOCKERHUB_TOKEN`. The start hook runs `docker login` after `dockerd` is ready. Mark `DOCKERHUB_TOKEN` as **redacted** in the dashboard. When both are set, agents can pull the full default `make start-docker` image set without hitting anonymous rate limits. diff --git a/.cursor/cursor.md b/.cursor/cursor.md index 895f1f0db19..e9fa3332185 100644 --- a/.cursor/cursor.md +++ b/.cursor/cursor.md @@ -58,7 +58,8 @@ The Mattermost server is expected at `http://localhost:8065`. The webapp dev ser ``` - When the server starts and `MM_LICENSE` is present in the environment, the server applies that license automatically. If `MM_LICENSE` is not set, starting the server automatically applies an Entry license, which provides nearly all functionality needed for development. -- `ENABLED_DOCKER_SERVICES='postgres redis'` avoids optional local-dev services such as Prometheus, Grafana, Loki, Minio, Azurite, and OpenLDAP. This is useful in Cloud when Docker Hub rate limits block the default `make start-docker` dependency set. +- When `DOCKERHUB_USERNAME` and `DOCKERHUB_TOKEN` are configured as Cloud Agent secrets, `cloud-agent-start.sh` logs in to Docker Hub and the full default `make start-docker` dependency set can be used without trimming services. +- `ENABLED_DOCKER_SERVICES='postgres redis'` avoids optional local-dev services such as Prometheus, Grafana, Loki, Minio, Azurite, and OpenLDAP. Use this fallback when Docker Hub credentials are unavailable and anonymous pulls hit rate limits. - If the first-user signup UI is flaky but the server is already healthy, seed local state with `mmctl` and then log in through the browser: ```bash diff --git a/.cursor/scripts/cloud-agent-start.sh b/.cursor/scripts/cloud-agent-start.sh index 268888e5d85..f1478521bf0 100755 --- a/.cursor/scripts/cloud-agent-start.sh +++ b/.cursor/scripts/cloud-agent-start.sh @@ -34,6 +34,21 @@ ensure_docker_socket_access() { fi } +docker_login_if_configured() { + if [ -z "${DOCKERHUB_USERNAME:-}" ] || [ -z "${DOCKERHUB_TOKEN:-}" ]; then + log "Docker Hub credentials not configured; anonymous pulls may hit rate limits." + return 0 + fi + + log "Logging in to Docker Hub as ${DOCKERHUB_USERNAME}." + if echo "${DOCKERHUB_TOKEN}" | docker login -u "${DOCKERHUB_USERNAME}" --password-stdin >/tmp/docker-login.log 2>&1; then + log "Docker Hub login succeeded." + else + log "Docker Hub login failed; see /tmp/docker-login.log." + tail -n 20 /tmp/docker-login.log >&2 || true + fi +} + if [ -f /proc/sys/kernel/apparmor_restrict_unprivileged_userns ]; then sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 >/dev/null 2>&1 || \ log "Could not relax AppArmor user namespace restriction; openldap-based tests may need a larger host profile." @@ -44,6 +59,7 @@ ensure_docker_socket_access if docker info >/dev/null 2>&1; then log "Docker is already running." docker compose version + docker_login_if_configured exit 0 fi @@ -64,6 +80,7 @@ for _ in {1..60}; do log "Docker is ready." docker version docker compose version + docker_login_if_configured exit 0 fi diff --git a/server/channels/app/access_control.go b/server/channels/app/access_control.go index 99df3abc372..de36aa68401 100644 --- a/server/channels/app/access_control.go +++ b/server/channels/app/access_control.go @@ -211,7 +211,8 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces if i >= len(existingPolicy.Rules) { continue } - storedExpr := existingPolicy.Rules[i].Expression + storedRule := existingPolicy.Rules[i] + storedExpr := storedRule.Expression if storedExpr == "" || storedExpr == "true" { continue } @@ -220,6 +221,15 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces return appErr } policy.Rules[i].Expression = mergedExpr + // If hidden values were re-injected into the expression, the caller was + // working from a masked view of this rule. Lock Actions to the stored + // value too — without this, a caller who sees "--------" could swap the + // action type (e.g., "membership" → "upload_file_attachment") and the + // merge would restore the hidden CEL value while silently removing the + // original access restriction. + if mergedExpr != rule.Expression { + policy.Rules[i].Actions = storedRule.Actions + } } // Any stored rules beyond the submitted set were dropped by the caller. If any of those @@ -387,8 +397,16 @@ func (a *App) mergeExpressionWithMaskedValues(rctx request.CTX, policyID, submit // 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. + // + // When the stored scalar value is hidden, always use hiddenValues[0] directly + // rather than taking arr[0] from the merged list. Without this guard a crafted + // submission of `in ["caller-visible"]` would pass validateConditionValues, + // land in mergeConditionValues as a []any, and arr[0] would be the attacker's + // value — silently overwriting the stored hidden value. if isScalarOperator(merged.Operator) { - if arr, ok := merged.Value.([]any); ok { + if len(hiddenValues) > 0 { + merged.Value = hiddenValues[0] + } else if arr, ok := merged.Value.([]any); ok { if len(arr) == 0 { merged.Value = nil } else if s, ok := arr[0].(string); ok { @@ -645,6 +663,33 @@ func (a *App) GetAccessControlPolicyAttributes(rctx request.CTX, channelID strin return nil, appErr } + if len(attributes) == 0 { + return attributes, nil + } + + // Strip source_only and shared_only fields: their values must not be + // exposed to channel members through the invite modal / members sidebar. + // Fail closed: if the CPA group or a field cannot be resolved, omit that + // field rather than leaking its values. + cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + if appErr != nil { + return map[string][]string{}, nil + } + + for fieldName := range attributes { + // Read directly from the store so this security filter sees the raw + // access_mode, unaffected by property read hooks for the request caller. + field, fieldErr := a.Srv().Store().PropertyField().GetFieldByName(rctx.Context(), cpaGroup.ID, "", fieldName) + if fieldErr != nil { + delete(attributes, fieldName) + continue + } + switch field.GetAccessMode() { + case model.PropertyAccessModeSourceOnly, model.PropertyAccessModeSharedOnly: + delete(attributes, fieldName) + } + } + return attributes, nil } diff --git a/server/channels/app/access_control_masking.go b/server/channels/app/access_control_masking.go index f82c9128374..2faca231918 100644 --- a/server/channels/app/access_control_masking.go +++ b/server/channels/app/access_control_masking.go @@ -362,9 +362,11 @@ func mergeConditionValues(submitted model.Condition, hiddenValues []string) mode 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 { + // For scalar conditions the caller cannot edit a value they cannot see. + // Always restore the stored hidden value regardless of what was submitted, + // preventing a crafted save from overwriting a hidden stored value with a + // different caller-visible string that passes validateConditionValues. + if len(hiddenValues) > 0 { merged.Value = hiddenValues[0] } @@ -545,6 +547,13 @@ func conditionToCEL(cond model.Condition) string { orParts = append(orParts, celStringLiteral(v)+" in "+attr) } if len(orParts) == 1 { + // When the sole value is the masked-token sentinel, duplicate it into a + // two-branch OR so that the parser can recover hasAnyOf on the next read. + // A standalone "tok in attr" is promoted to hasAllOf by + // mergeMultiselectConditions, which would display the wrong operator in the UI. + if values[0] == maskedTokenValue { + return "(" + orParts[0] + " || " + orParts[0] + ")" + } return orParts[0] } return "(" + strings.Join(orParts, " || ") + ")" @@ -750,8 +759,14 @@ func (a *App) GetMaskedExpression(rctx request.CTX, expression string, callerID rctxWithCaller := RequestContextWithCallerID(rctx, callerID) fieldsByName := a.fetchConditionFields(rctxWithCaller, visualAST.Conditions, cpaGroupID) + hasMasked := false for i := range visualAST.Conditions { - a.maskConditionValuesWithToken(rctxWithCaller, callerID, &visualAST.Conditions[i], cpaGroupID, fieldsByName) + if a.maskConditionValuesWithToken(rctxWithCaller, callerID, &visualAST.Conditions[i], cpaGroupID, fieldsByName) { + hasMasked = true + } + } + if !hasMasked { + return expression, nil } return buildCELFromConditions(visualAST.Conditions), nil @@ -761,27 +776,30 @@ func (a *App) GetMaskedExpression(rctx request.CTX, expression string, callerID // 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) { +// maskConditionValuesWithToken replaces non-held values with the masked token in place. +// Returns true if any value was masked. +func (a *App) maskConditionValuesWithToken(rctx request.CTX, callerID string, condition *model.Condition, cpaGroupID string, fieldsByName map[string]*model.PropertyField) bool { if condition.ValueType == model.AttrValue { - return + return false } fieldName := extractFieldName(condition.Attribute) if fieldName == "" { - return + return false } field, ok := fieldsByName[fieldName] if !ok { condition.Value = maskedTokenValue // fail closed - return + return true } switch field.GetAccessMode() { case model.PropertyAccessModePublic: - return + return false case model.PropertyAccessModeSourceOnly: condition.Value = maskedTokenValue + return true case model.PropertyAccessModeSharedOnly: var visibleNames map[string]struct{} if field.Type == model.PropertyFieldTypeSelect || field.Type == model.PropertyFieldTypeMultiselect { @@ -789,15 +807,17 @@ func (a *App) maskConditionValuesWithToken(rctx request.CTX, callerID string, co } else { visibleNames = a.getCallerTextValues(rctx, callerID, field, cpaGroupID) } - replaceHiddenValuesWithToken(condition, visibleNames) + return replaceHiddenValuesWithToken(condition, visibleNames) default: condition.Value = maskedTokenValue + return true } } // 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{}) { +// Returns true if any value was replaced. +func replaceHiddenValuesWithToken(condition *model.Condition, visibleNames map[string]struct{}) bool { switch v := condition.Value.(type) { case []any: var result []any @@ -817,11 +837,14 @@ func replaceHiddenValuesWithToken(condition *model.Condition, visibleNames map[s result = append(result, maskedTokenValue) } condition.Value = result + return hasMasked case string: if _, visible := visibleNames[v]; !visible { condition.Value = maskedTokenValue + return true } } + return false } // MaskPolicyExpressions masks non-held literal values in all policy rule expressions, in place. @@ -867,9 +890,14 @@ func (a *App) MaskPolicyExpressions(rctx request.CTX, policy *model.AccessContro if ast == nil { continue } + hasMasked := false for j := range ast.Conditions { - a.maskConditionValuesWithToken(rctxWithCaller, callerID, &ast.Conditions[j], cpaGroupID, fieldsByName) + if a.maskConditionValuesWithToken(rctxWithCaller, callerID, &ast.Conditions[j], cpaGroupID, fieldsByName) { + hasMasked = true + } + } + if hasMasked { + policy.Rules[i].Expression = buildCELFromConditions(ast.Conditions) } - 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 index 4f21749378d..28cc6ae161b 100644 --- a/server/channels/app/access_control_merge_test.go +++ b/server/channels/app/access_control_merge_test.go @@ -89,6 +89,24 @@ func TestBuildCELFromConditions(t *testing.T) { assert.Equal(t, `"Alpha" in user.attributes.Programs`, result) }) + t.Run("hasAnyOf with single masked-token value emits duplicate OR to preserve operator through re-parse", func(t *testing.T) { + // A sole masked-token sentinel must round-trip as hasAnyOf. Without the + // duplicate, a standalone "tok in attr" is promoted to hasAllOf by + // mergeMultiselectConditions, showing the wrong operator in the table editor. + conditions := []model.Condition{ + { + Attribute: "user.attributes.Programs", + Operator: "hasAnyOf", + Value: []any{maskedTokenValue}, + ValueType: model.LiteralValue, + AttributeType: "multiselect", + }, + } + result := buildCELFromConditions(conditions) + expected := `("` + maskedTokenValue + `" in user.attributes.Programs || "` + maskedTokenValue + `" in user.attributes.Programs)` + assert.Equal(t, expected, result) + }) + t.Run("hasAllOf operator", func(t *testing.T) { conditions := []model.Condition{ { @@ -424,6 +442,45 @@ func TestMergeConditionValues(t *testing.T) { result := mergeConditionValues(submitted, []string{"Building 7"}) assert.Equal(t, "Building 7", result.Value) }) + + t.Run("scalar: hidden value wins over empty submitted string", func(t *testing.T) { + submitted := model.Condition{Attribute: "user.attributes.Location", Operator: "!=", Value: ""} + result := mergeConditionValues(submitted, []string{"Building 7"}) + assert.Equal(t, "Building 7", result.Value) + }) + + t.Run("scalar: hidden value wins over masked-token submitted string", func(t *testing.T) { + submitted := model.Condition{Attribute: "user.attributes.Location", Operator: "!=", Value: maskedTokenValue} + result := mergeConditionValues(submitted, []string{"Building 7"}) + assert.Equal(t, "Building 7", result.Value) + }) + + t.Run("scalar: hidden value wins over caller-visible submitted string (security: prevents overwrite)", func(t *testing.T) { + // A crafted save can submit a caller-held value that passes validateConditionValues. + // mergeConditionValues must still restore the stored hidden value so the caller + // cannot overwrite a shared_only scalar they cannot see. + submitted := model.Condition{Attribute: "user.attributes.Location", Operator: "!=", Value: "Building 1"} + result := mergeConditionValues(submitted, []string{"Building 7"}) + assert.Equal(t, "Building 7", result.Value) + }) + + t.Run("scalar via []any: mergeConditionValues appends hidden value last; isScalarOperator block must use hiddenValues[0] not arr[0]", func(t *testing.T) { + // Second attack vector: a crafted submission of `in ["Building 1"]` (a list, + // not a string) also passes validateConditionValues for a shared_only caller. + // mergeConditionValues produces ["Building 1", "Building 7"] — the caller's + // value comes first. The isScalarOperator normalization in + // mergeExpressionWithMaskedValues must use hiddenValues[0] directly rather + // than arr[0], otherwise the attacker's value wins. + submitted := model.Condition{Attribute: "user.attributes.Location", Operator: "in", Value: []any{"Building 1"}} + result := mergeConditionValues(submitted, []string{"Building 7"}) + values, ok := result.Value.([]any) + require.True(t, ok) + // Hidden value is appended after the submitted one — arr[0] would be "Building 1". + // The fix in mergeExpressionWithMaskedValues (isScalarOperator block) must + // pick hiddenValues[0] = "Building 7" instead of arr[0]. + assert.Equal(t, "Building 1", values[0], "submitted value is first in merged list") + assert.Equal(t, "Building 7", values[1], "hidden value is appended last") + }) } func TestGetHiddenValues(t *testing.T) { diff --git a/server/channels/app/access_control_test.go b/server/channels/app/access_control_test.go index fa5e6ca210c..151d107fe1c 100644 --- a/server/channels/app/access_control_test.go +++ b/server/channels/app/access_control_test.go @@ -2232,3 +2232,313 @@ func TestGetRecommendedPublicChannelsForUser(t *testing.T) { mockACS.AssertExpectations(t) }) } + +// TestGetAccessControlPolicyAttributes_MaskedFieldsFiltered verifies that +// source_only and shared_only attribute fields are stripped from the response +// of GetAccessControlPolicyAttributes so their values are never exposed to +// regular channel members through the invite modal or members sidebar. +func TestGetAccessControlPolicyAttributes_MaskedFieldsFiltered(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + + rctx := request.TestContext(t) + + cpaGroup, cErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, cErr) + + permNone := model.PermissionLevelNone + + makeField := func(name, accessMode string) { + protected := accessMode == model.PropertyAccessModeSourceOnly || accessMode == model.PropertyAccessModeSharedOnly + f := &model.PropertyField{ + GroupID: cpaGroup.ID, + Name: name, + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + Protected: protected, + Attrs: model.StringInterface{model.PropertyAttrsAccessMode: accessMode}, + } + if protected { + f.PermissionField = &permNone + f.Attrs[model.PropertyAttrsProtected] = true + _, err := th.App.Srv().Store().PropertyField().Create(f) + require.NoError(t, err) + } else { + _, appErr := th.App.CreatePropertyField(rctx, f, false, "") + require.Nil(t, appErr) + } + } + + makeField("PublicField", model.PropertyAccessModePublic) + makeField("SourceField", model.PropertyAccessModeSourceOnly) + makeField("SharedField", model.PropertyAccessModeSharedOnly) + + channelID := model.NewId() + rawAttributes := map[string][]string{ + "PublicField": {"Engineering"}, + "SourceField": {"TopSecret"}, + "SharedField": {"Alpha", "Bravo"}, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + mockACS.On("GetPolicyRuleAttributes", mock.Anything, channelID, model.AccessControlPolicyActionMembership). + Return(rawAttributes, nil).Once() + + result, appErr := th.App.GetAccessControlPolicyAttributes(th.Context, channelID, model.AccessControlPolicyActionMembership) + require.Nil(t, appErr) + + // Only the public field should survive. + assert.Equal(t, map[string][]string{"PublicField": {"Engineering"}}, result) + assert.NotContains(t, result, "SourceField") + assert.NotContains(t, result, "SharedField") + mockACS.AssertExpectations(t) +} + +// TestGetAccessControlPolicyAttributes_PublicFieldsPassThrough verifies that +// public attribute fields are returned unchanged. +func TestGetAccessControlPolicyAttributes_PublicFieldsPassThrough(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + + rctx := request.TestContext(t) + + cpaGroup, cErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, cErr) + + fieldName := "f_" + model.NewId()[:8] + field := &model.PropertyField{ + GroupID: cpaGroup.ID, + Name: fieldName, + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + Attrs: model.StringInterface{model.PropertyAttrsAccessMode: model.PropertyAccessModePublic}, + } + _, appErr := th.App.CreatePropertyField(rctx, field, false, "") + require.Nil(t, appErr) + + channelID := model.NewId() + rawAttributes := map[string][]string{fieldName: {"Engineering", "Sales"}} + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + mockACS.On("GetPolicyRuleAttributes", mock.Anything, channelID, model.AccessControlPolicyActionMembership). + Return(rawAttributes, nil).Once() + + result, appErr := th.App.GetAccessControlPolicyAttributes(th.Context, channelID, model.AccessControlPolicyActionMembership) + require.Nil(t, appErr) + assert.Equal(t, rawAttributes, result) + mockACS.AssertExpectations(t) +} + +// TestMergeStoredPolicyExpressions_ActionsLocked verifies that a caller who +// cannot see all values in a stored rule cannot change that rule's Actions. +// The attack: submit a PUT with the same masked expression but a different +// action type — the merge would restore the hidden CEL value while silently +// accepting the caller's action, removing the original access restriction. +func TestMergeStoredPolicyExpressions_ActionsLocked(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + + rctx := request.TestContext(t) + + // Insert a source_only field directly into the store to bypass the property + // service hook that restricts protected-field creation to plugin callers. + cpaGroup, cErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, cErr) + + fieldName := "f_" + model.NewId()[:8] + permNone := model.PermissionLevelNone + field := &model.PropertyField{ + GroupID: cpaGroup.ID, + Name: fieldName, + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + Protected: true, + PermissionField: &permNone, + Attrs: model.StringInterface{ + model.PropertyAttrsAccessMode: model.PropertyAccessModeSourceOnly, + model.PropertyAttrsProtected: true, + }, + } + _, storeErr := th.App.Srv().Store().PropertyField().Create(field) + require.NoError(t, storeErr) + + callerID := model.NewId() + policyID := model.NewId() + + storedExpr := `user.attributes.` + fieldName + ` == "TopSecret"` + maskedExpr := `user.attributes.` + fieldName + ` == "--------"` + + storedPolicy := &model.AccessControlPolicy{ + ID: policyID, + Type: model.AccessControlPolicyTypeParent, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: storedExpr}, + }, + } + + // Attacker submits the masked expression unchanged but swaps the action. + submittedPolicy := &model.AccessControlPolicy{ + ID: policyID, + Type: model.AccessControlPolicyTypeParent, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionUploadFileAttachment}, Expression: maskedExpr}, + }, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + + mockACS.On("GetPolicy", mock.Anything, policyID).Return(storedPolicy, nil).Once() + mockACS.On("ExpressionToVisualAST", mock.Anything, storedExpr).Return(&model.VisualExpression{ + Conditions: []model.Condition{ + {Attribute: "user.attributes." + fieldName, Operator: "==", Value: "TopSecret", ValueType: model.LiteralValue}, + }, + }, nil).Maybe() + mockACS.On("ExpressionToVisualAST", mock.Anything, maskedExpr).Return(&model.VisualExpression{ + Conditions: []model.Condition{ + {Attribute: "user.attributes." + fieldName, Operator: "==", Value: maskedTokenValue, ValueType: model.LiteralValue}, + }, + }, nil).Maybe() + + mergeErr := th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID) + require.Nil(t, mergeErr) + + require.Len(t, submittedPolicy.Rules, 1) + // Expression must be restored to the real stored value. + assert.Equal(t, storedExpr, submittedPolicy.Rules[0].Expression) + // Actions must be locked to the stored value, not the attacker's. + assert.Equal(t, []string{model.AccessControlPolicyActionMembership}, submittedPolicy.Rules[0].Actions) + mockACS.AssertExpectations(t) +} + +// TestMergeStoredPolicyExpressions_FailClosedTrueRejectedOnResubmit verifies the +// claim from the PR review: if MaskPolicyExpressions emitted "true" for a rule +// because the stored expression could not be parsed (fail-closed), a caller who +// re-submits that "true" unchanged will be blocked on the save path. +// +// How it works: +// 1. MaskPolicyExpressions (GET path) calls ExpressionToVisualAST on the stored +// expression; on parse failure it sets the rule to "true" (fail-closed). +// 2. The caller sees "true" in the GET response and re-submits it. +// 3. On the save path, mergeExpressionWithMaskedValues calls +// expressionHasMaskedValuesForCaller, which calls GetMaskedVisualAST, which +// calls ExpressionToVisualAST on the *stored* expression again. +// 4. That second parse also fails → error propagates → save is blocked. +// "true" is never written to the DB. +func TestMergeStoredPolicyExpressions_FailClosedTrueRejectedOnResubmit(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + + callerID := model.NewId() + policyID := model.NewId() + + // A stored expression that ExpressionToVisualAST cannot parse. + // In production this is guarded by save-time validation, but defensive + // code paths must still protect against it. + storedExpr := `user.attributes.TopSecret == "Value"` + + storedPolicy := &model.AccessControlPolicy{ + ID: policyID, + Type: model.AccessControlPolicyTypeParent, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: storedExpr}, + }, + } + + // Caller re-submits "true" — what MaskPolicyExpressions emitted as the + // fail-closed value when it could not parse the stored expression on GET. + submittedPolicy := &model.AccessControlPolicy{ + ID: policyID, + Type: model.AccessControlPolicyTypeParent, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: "true"}, + }, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + + mockACS.On("GetPolicy", mock.Anything, policyID).Return(storedPolicy, nil).Once() + // Simulate the same parse failure that would have triggered fail-closed on GET. + parseErr := model.NewAppError("ExpressionToVisualAST", "app.pap.expression_to_visual_ast.app_error", nil, "simulated parse failure", http.StatusInternalServerError) + mockACS.On("ExpressionToVisualAST", mock.Anything, storedExpr).Return(nil, parseErr).Maybe() + + mergeErr := th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID) + + // Save must be blocked. The error returned here causes UpdateAccessControlPolicy + // to abort before any DB write — the in-memory struct may still hold "true" + // but it never reaches the store. + require.NotNil(t, mergeErr, "expected mergeStoredPolicyExpressions to return an error when stored expression is unparseable") + mockACS.AssertExpectations(t) +} + +// TestMergeStoredPolicyExpressions_ActionsEditableWhenNoMasking verifies that +// a caller who holds all values in a rule can freely change its Actions. +func TestMergeStoredPolicyExpressions_ActionsEditableWhenNoMasking(t *testing.T) { + th := Setup(t).InitBasic(t) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuEnterprise)) + + rctx := request.TestContext(t) + + // Create a public field — values are always visible, so no masking occurs. + cpaGroup, cErr := th.App.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) + require.Nil(t, cErr) + + fieldName := "f_" + model.NewId()[:8] + field := &model.PropertyField{ + GroupID: cpaGroup.ID, + Name: fieldName, + Type: model.PropertyFieldTypeText, + ObjectType: model.PropertyFieldObjectTypeUser, + TargetType: string(model.PropertyFieldTargetLevelSystem), + Attrs: model.StringInterface{model.PropertyAttrsAccessMode: model.PropertyAccessModePublic}, + } + _, appErr := th.App.CreatePropertyField(rctx, field, false, "") + require.Nil(t, appErr) + + callerID := model.NewId() + policyID := model.NewId() + + expr := `user.attributes.` + fieldName + ` == "Engineering"` + + storedPolicy := &model.AccessControlPolicy{ + ID: policyID, + Type: model.AccessControlPolicyTypeParent, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: expr}, + }, + } + // Caller legitimately changes the action on a rule with no masked values. + submittedPolicy := &model.AccessControlPolicy{ + ID: policyID, + Type: model.AccessControlPolicyTypeParent, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionUploadFileAttachment}, Expression: expr}, + }, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + + mockACS.On("GetPolicy", mock.Anything, policyID).Return(storedPolicy, nil).Once() + mockACS.On("ExpressionToVisualAST", mock.Anything, expr).Return(&model.VisualExpression{ + Conditions: []model.Condition{ + {Attribute: "user.attributes." + fieldName, Operator: "==", Value: "Engineering", ValueType: model.LiteralValue}, + }, + }, nil).Maybe() + + appErr = th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID) + require.Nil(t, appErr) + + require.Len(t, submittedPolicy.Rules, 1) + // Expression unchanged (no masking, submitted passes through). + assert.Equal(t, expr, submittedPolicy.Rules[0].Expression) + // Actions must NOT be locked — caller's submitted value stands. + assert.Equal(t, []string{model.AccessControlPolicyActionUploadFileAttachment}, submittedPolicy.Rules[0].Actions) + mockACS.AssertExpectations(t) +} diff --git a/server/channels/app/properties/access_control_field_test.go b/server/channels/app/properties/access_control_field_test.go index fbb4e597605..e940a3a830e 100644 --- a/server/channels/app/properties/access_control_field_test.go +++ b/server/channels/app/properties/access_control_field_test.go @@ -1506,7 +1506,3 @@ func TestLinkedPropertyField_SecurityInheritance(t *testing.T) { assert.False(t, model.IsPropertyFieldProtected(linked)) }) } - -// The previous "member-writable shared_only" early-return in applyFieldReadAccessControl -// was removed in favor of rejecting that contradictory configuration at validation time -// (see TestValidatePropertyFieldAccessMode in server/public/model/property_access_test.go). diff --git a/server/channels/app/remote_cluster.go b/server/channels/app/remote_cluster.go index 4f7b144b2fe..b87ba525eaf 100644 --- a/server/channels/app/remote_cluster.go +++ b/server/channels/app/remote_cluster.go @@ -8,6 +8,7 @@ import ( "encoding/base64" "fmt" "net/http" + "time" "github.com/pkg/errors" @@ -19,6 +20,34 @@ import ( "github.com/mattermost/mattermost/server/public/shared/request" ) +// pluginRemoteInitialPingDelay is how long the framework waits after +// RegisterPluginForSharedChannels returns before firing the first ping to +// the newly created or restored plugin remote. The delay gives the +// calling plugin a chance to record the returned RemoteId in its own +// state, so the synchronous OnSharedChannelsPing hook the framework +// invokes can resolve the remote. Without the delay, the first ping for +// every freshly registered SiteURL fails and the remote stays offline +// until the periodic pingLoop refreshes it (up to PingFreq, default 1 +// minute). Declared as a var, not const, so tests can shorten it. +var pluginRemoteInitialPingDelay = 5 * time.Second + +// schedulePluginRemoteInitialPing schedules a single deferred ping for a +// freshly registered or restored plugin remote. The goroutine is launched +// via Server.Go so it cannot outlive the server. The remote is re-read +// before the ping fires because the plugin may have unregistered it +// inside the delay window; pinging a soft-deleted row is harmless but +// produces a stray "ping failed" warning. +func (a *App) schedulePluginRemoteInitialPing(rcService remotecluster.RemoteClusterServiceIFace, rc *model.RemoteCluster) { + a.Srv().Go(func() { + time.Sleep(pluginRemoteInitialPingDelay) + current, err := a.Srv().Store().RemoteCluster().Get(rc.RemoteId, true) + if err != nil || current.DeleteAt != 0 { + return + } + rcService.PingNow(current) + }) +} + func (a *App) RegisterPluginForSharedChannels(rctx request.CTX, opts model.RegisterPluginOpts) (remoteID string, err error) { // When SiteURL is empty, fall back to the legacy single-remote behavior // using the "plugin_" prefix. This preserves compatibility for older plugins @@ -59,6 +88,18 @@ func (a *App) RegisterPluginForSharedChannels(rctx request.CTX, opts model.Regis if _, err = a.Srv().Store().RemoteCluster().Update(rc); err != nil { return "", err } + + // Ping the restored plugin remote so its LastPingAt is refreshed + // before sync attempts. Deferred via a goroutine (see + // schedulePluginRemoteInitialPing) so the caller has a chance + // to record the returned RemoteId before the synchronous + // OnSharedChannelsPing hook fires. Without this the restored + // remote stays offline until the next pingLoop iteration (up to + // PingFreq), causing transient sync failures. + rcService, _ := a.GetRemoteClusterService() + if rcService != nil { + a.schedulePluginRemoteInitialPing(rcService, rc) + } return rc.RemoteId, nil } @@ -86,13 +127,19 @@ func (a *App) RegisterPluginForSharedChannels(rctx request.CTX, opts model.Regis mlog.String("site_url", opts.SiteURL), ) - // Ping the plugin remote immediately if the service is running. - // If the service is not available the ping will happen once the - // service starts. This is expected since plugins start before the - // service. + // Ping the new plugin remote, deferred via a goroutine so the + // calling plugin has a chance to record the returned RemoteId + // before the synchronous OnSharedChannelsPing hook fires for the + // first time. A synchronous ping here would invoke the hook + // before the caller's return statement, the plugin would fail to + // resolve the new RemoteId, the ping would be recorded as failed, + // and the remote would stay offline until the next pingLoop + // iteration (up to PingFreq, default 1 minute). If the service is + // not yet running the ping will fire from the periodic pingLoop + // once the service starts. rcService, _ := a.GetRemoteClusterService() if rcService != nil { - rcService.PingNow(rcSaved) + a.schedulePluginRemoteInitialPing(rcService, rcSaved) } return rcSaved.RemoteId, nil diff --git a/server/channels/app/remote_cluster_test.go b/server/channels/app/remote_cluster_test.go index f4b5ec604dd..4a5637d4a1d 100644 --- a/server/channels/app/remote_cluster_test.go +++ b/server/channels/app/remote_cluster_test.go @@ -6,13 +6,39 @@ package app import ( "strings" "testing" + "time" "github.com/stretchr/testify/require" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/v8/channels/testlib" + "github.com/mattermost/mattermost/server/v8/platform/services/remotecluster" ) +// Shorten the deferred initial ping for tests so RegisterPluginForSharedChannels +// teardown does not block on a 5s goroutine. No test in this package needs the +// production headroom. The value is large enough that even a slow runner where +// RegisterPluginForSharedChannels takes a couple hundred milliseconds still +// has comfortable margin before the deferred goroutine fires. +func init() { + pluginRemoteInitialPingDelay = 500 * time.Millisecond +} + +// pingTrackingRCService wraps a real RemoteClusterServiceIFace and records the +// time of every PingNow call without forwarding it. Embedding the interface +// satisfies the other methods by delegation. +type pingTrackingRCService struct { + remotecluster.RemoteClusterServiceIFace + pings chan time.Time +} + +func (p *pingTrackingRCService) PingNow(rc *model.RemoteCluster) { + select { + case p.pings <- time.Now(): + default: + } +} + func setupRemoteCluster(tb testing.TB) *TestHelper { return SetupConfig(tb, func(cfg *model.Config) { *cfg.ConnectedWorkspacesSettings.EnableRemoteClusterService = true @@ -221,6 +247,46 @@ func TestRegisterPluginForSharedChannels(t *testing.T) { require.Equal(t, id1, id2) }) + t.Run("re-registering a soft-deleted SiteURL restores the row and pings the remote (MM-68838)", func(t *testing.T) { + pluginID := "com.test.restore-" + model.NewId() + siteURL := "nats://restore-" + model.NewId() + + // 1. Initial registration. + id1, err := th.App.RegisterPluginForSharedChannels(th.Context, model.RegisterPluginOpts{ + Displayname: "restore test plugin", + PluginID: pluginID, + CreatorID: th.BasicUser.Id, + SiteURL: siteURL, + }) + require.NoError(t, err) + + // 2. Unregister soft-deletes the row. + require.NoError(t, th.App.UnregisterPluginRemoteForSharedChannels(pluginID, id1)) + + rcDeleted, err := th.App.Srv().Store().RemoteCluster().Get(id1, true) + require.NoError(t, err) + require.NotZero(t, rcDeleted.DeleteAt, "row should be soft-deleted after unregister") + + // 3. Re-register the same SiteURL. The restore path must run. + id2, err := th.App.RegisterPluginForSharedChannels(th.Context, model.RegisterPluginOpts{ + Displayname: "restore test plugin", + PluginID: pluginID, + CreatorID: th.BasicUser.Id, + SiteURL: siteURL, + }) + require.NoError(t, err) + require.Equal(t, id1, id2, "restore path must reuse the existing remoteID") + + // 4. The row must be restored (DeleteAt cleared). PingNow is called + // inside the restore branch; the actual ping fails in unit tests + // because no plugin process is loaded to answer OnSharedChannelsPing, + // so we cannot assert on LastPingAt here. The presence of the call + // is what fixes MM-68838 (offline-for-PingFreq window on restart). + rcRestored, err := th.App.Srv().Store().RemoteCluster().Get(id2, false) + require.NoError(t, err) + require.Zero(t, rcRestored.DeleteAt, "row should be restored after re-register") + }) + t.Run("multi-remote registration returns distinct remoteIDs", func(t *testing.T) { pluginID := "com.test.multi-" + model.NewId() @@ -322,3 +388,117 @@ func TestUnregisterPluginForSharedChannelsBulk(t *testing.T) { require.NoError(t, err) require.Empty(t, remotes) } + +// TestRegisterPluginForSharedChannelsPingIsDeferred guards the race fix. +// A synchronous PingNow inside RegisterPluginForSharedChannels invoked the +// plugin's OnSharedChannelsPing hook before the calling plugin could record +// the returned RemoteId, so the very first ping always failed and the remote +// stayed offline for ~PingFreq (1 minute). The fix is to defer the initial +// ping to a goroutine. Both the new-row branch and the soft-delete-restore +// branch must defer. +func TestRegisterPluginForSharedChannelsPingIsDeferred(t *testing.T) { + mainHelper.Parallel(t) + th := setupRemoteCluster(t).InitBasic(t) + + tracker := &pingTrackingRCService{ + RemoteClusterServiceIFace: th.Server.remoteClusterService, + pings: make(chan time.Time, 8), + } + original := th.Server.remoteClusterService + th.Server.remoteClusterService = tracker + t.Cleanup(func() { th.Server.remoteClusterService = original }) + + // Generous upper bound on real wall-time variance under load: the + // production delay is 5s; init() overrides to 100ms; we wait up to + // delay + 2s for the ping to actually arrive. + const arrivalGrace = 2 * time.Second + delay := pluginRemoteInitialPingDelay + + // drain consumes any pending ping timestamps so a later sub-case does + // not see a stale one from an earlier sub-case. + drain := func(ch <-chan time.Time) { + for { + select { + case <-ch: + default: + return + } + } + } + + assertDeferred := func(t *testing.T, registerStart time.Time) { + t.Helper() + // Phase 1: no ping in the first half of the delay (proves async). + var prematurePing bool + select { + case <-tracker.pings: + prematurePing = true + case <-time.After(delay / 2): + } + require.False(t, prematurePing, "PingNow fired synchronously inside RegisterPluginForSharedChannels; the deferral is broken") + // Phase 2: a ping arrives within delay + grace, and not before delay. + var pingAt time.Time + var pingArrived bool + select { + case pingAt = <-tracker.pings: + pingArrived = true + case <-time.After(delay + arrivalGrace): + } + require.True(t, pingArrived, "expected PingNow to fire within delay + grace, but it never did") + elapsed := pingAt.Sub(registerStart) + require.GreaterOrEqual(t, elapsed, delay, + "PingNow fired %s after Register returned, before the configured delay of %s", elapsed, delay) + } + + t.Run("new-row branch defers the initial ping", func(t *testing.T) { + drain(tracker.pings) + + start := time.Now() + _, err := th.App.RegisterPluginForSharedChannels(th.Context, model.RegisterPluginOpts{ + Displayname: "deferred ping plugin", + PluginID: "com.test.deferred-" + model.NewId(), + CreatorID: th.BasicUser.Id, + SiteURL: "nats://deferred-" + model.NewId(), + }) + require.NoError(t, err) + assertDeferred(t, start) + }) + + t.Run("soft-delete restore branch defers the ping (MM-68838)", func(t *testing.T) { + drain(tracker.pings) + + pluginID := "com.test.restore-defer-" + model.NewId() + siteURL := "nats://restore-defer-" + model.NewId() + + // Initial register to create the row; consume its deferred ping. + id1, err := th.App.RegisterPluginForSharedChannels(th.Context, model.RegisterPluginOpts{ + Displayname: "restore defer plugin", + PluginID: pluginID, + CreatorID: th.BasicUser.Id, + SiteURL: siteURL, + }) + require.NoError(t, err) + var initialPingArrived bool + select { + case <-tracker.pings: + initialPingArrived = true + case <-time.After(delay + arrivalGrace): + } + require.True(t, initialPingArrived, "initial register's deferred ping never arrived") + + // Unregister soft-deletes the row. + require.NoError(t, th.App.UnregisterPluginRemoteForSharedChannels(pluginID, id1)) + drain(tracker.pings) + + // Re-register: the restore branch must also defer. + start := time.Now() + _, err = th.App.RegisterPluginForSharedChannels(th.Context, model.RegisterPluginOpts{ + Displayname: "restore defer plugin", + PluginID: pluginID, + CreatorID: th.BasicUser.Id, + SiteURL: siteURL, + }) + require.NoError(t, err) + assertDeferred(t, start) + }) +} diff --git a/server/channels/store/sqlstore/access_control_policy_store.go b/server/channels/store/sqlstore/access_control_policy_store.go index 0ae4983afeb..d2324f6f1ea 100644 --- a/server/channels/store/sqlstore/access_control_policy_store.go +++ b/server/channels/store/sqlstore/access_control_policy_store.go @@ -79,8 +79,12 @@ func (s *storeAccessControlPolicy) toModel() (*model.AccessControlPolicy, error) } func fromModel(policy *model.AccessControlPolicy) (*storeAccessControlPolicy, error) { + imports := policy.Imports + if imports == nil { + imports = []string{} + } data, err := json.Marshal(&accessControlPolicyV0_1{ - Imports: policy.Imports, + Imports: imports, Rules: policy.Rules, Roles: policy.Roles, Scope: policy.Scope, @@ -706,7 +710,7 @@ func (s *SqlAccessControlPolicyStore) SearchPolicies(rctx request.CTX, opts mode condition := sq.Expr(`Id IN ( SELECT parent_id FROM ( - SELECT ch.TeamId, jsonb_array_elements_text(cp.Data -> 'imports') AS parent_id + SELECT ch.TeamId, jsonb_array_elements_text(COALESCE(NULLIF(cp.Data -> 'imports', 'null'::jsonb), '[]'::jsonb)) AS parent_id FROM AccessControlPolicies cp JOIN Channels ch ON ch.Id = cp.Id WHERE cp.Type = 'channel' diff --git a/server/public/plugin/client_rpc.go b/server/public/plugin/client_rpc.go index c0c03f93df7..196b43ed2b3 100644 --- a/server/public/plugin/client_rpc.go +++ b/server/public/plugin/client_rpc.go @@ -1260,6 +1260,7 @@ func (s *apiRPCServer) ReceiveSharedChannelAttachmentSyncMsg(args *Z_ReceiveShar defer dataReader.Close() returns.A, returns.B = hook.ReceiveSharedChannelAttachmentSyncMsg(args.A, args.B, args.C, dataReader) + returns.B = encodableError(returns.B) return nil } diff --git a/server/public/plugin/client_rpc_test.go b/server/public/plugin/client_rpc_test.go new file mode 100644 index 00000000000..179b392e0f8 --- /dev/null +++ b/server/public/plugin/client_rpc_test.go @@ -0,0 +1,46 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +import ( + "bytes" + "encoding/gob" + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestReceiveSharedChannelAttachmentSyncMsgReturns_GobRoundTrip pins the fix +// for the gob-encoding bug in apiRPCServer.ReceiveSharedChannelAttachmentSyncMsg. +// The hook may return errors wrapped with fmt.Errorf("...%w", err), producing +// values of the unexported type *fmt.wrapError that gob refuses to encode. +// The RPC server must run the error through encodableError before assigning +// it to the returns struct. Without that, the RPC connection breaks and +// every subsequent plugin to server call returns zero values. +func TestReceiveSharedChannelAttachmentSyncMsgReturns_GobRoundTrip(t *testing.T) { + wrapped := fmt.Errorf("attachment sync failed: %w", errors.New("upstream boom")) + + t.Run("raw wrapped error fails to gob-encode (reproduces the bug)", func(t *testing.T) { + returns := Z_ReceiveSharedChannelAttachmentSyncMsgReturns{B: wrapped} + + var buf bytes.Buffer + err := gob.NewEncoder(&buf).Encode(&returns) + require.Error(t, err, "raw *fmt.wrapError must not be gob-encodable; if this assertion ever fails the bug guarded by encodableError no longer exists") + require.Contains(t, err.Error(), "fmt.wrapError") + }) + + t.Run("encodableError-wrapped error round-trips through gob", func(t *testing.T) { + returns := Z_ReceiveSharedChannelAttachmentSyncMsgReturns{B: encodableError(wrapped)} + + var buf bytes.Buffer + require.NoError(t, gob.NewEncoder(&buf).Encode(&returns)) + + var decoded Z_ReceiveSharedChannelAttachmentSyncMsgReturns + require.NoError(t, gob.NewDecoder(&buf).Decode(&decoded)) + require.Error(t, decoded.B) + require.Equal(t, wrapped.Error(), decoded.B.Error()) + }) +} diff --git a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.scss b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.scss index f90ee84f51e..b84f0b3e328 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.scss +++ b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.scss @@ -1,6 +1,24 @@ .cel-editor { margin-bottom: 24px; + &__masked-banner { + display: flex; + align-items: center; + padding: 10px 16px; + border-radius: 4px; + margin-bottom: 8px; + background-color: rgba(var(--center-channel-color-rgb), 0.08); + color: rgba(var(--center-channel-color-rgb), 0.72); + font-size: 13px; + gap: 8px; + line-height: 20px; + + .icon { + color: var(--dnd-indicator); + font-size: 18px; + } + } + &__container { position: relative; display: flex; diff --git a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx index bdfe077bc8b..9326d718682 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/cel_editor/editor.tsx @@ -80,6 +80,7 @@ interface CELEditorProps { attribute: string; values: string[]; }>; + hasMaskedRows?: boolean; } // TODO: this is just a sample schema for the editor, we need to get the actual schema from the server @@ -94,6 +95,7 @@ function CELEditor({ teamId, disabled = false, userAttributes, + hasMaskedRows = false, }: CELEditorProps): JSX.Element { const intl = useIntl(); const [editorState, setEditorState] = useState({ @@ -257,12 +259,12 @@ function CELEditor({ }; }, []); // Only run once on mount - // Update the editor's readOnly state when disabled prop changes + // Update the editor's readOnly state when disabled or hasMaskedRows changes useEffect(() => { if (monacoRef.current) { - monacoRef.current.updateOptions({readOnly: disabled}); + monacoRef.current.updateOptions({readOnly: disabled || hasMaskedRows}); } - }, [disabled]); + }, [disabled, hasMaskedRows]); // Helper function to determine current validation state const getValidationState = useCallback(() => { @@ -338,6 +340,19 @@ function CELEditor({
+ {hasMaskedRows && ( +
+ + +
+ )} +
setEditorState((prev) => ({...prev, showTestResults: true}))} - disabled={disabled || !editorState.expression || !editorState.isValid || editorState.isValidating} + disabled={disabled || hasMaskedRows || !editorState.expression || !editorState.isValid || editorState.isValidating} + disabledTooltip={ + hasMaskedRows ? + intl.formatMessage({ + id: 'admin.access_control.cel_editor.masked_values_tooltip', + defaultMessage: 'Test is unavailable because this policy contains restricted attribute values.', + }) : + undefined + } />
{editorState.showTestResults && ( diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/masked_chip.test.tsx b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/masked_chip.test.tsx new file mode 100644 index 00000000000..c657c435e1d --- /dev/null +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/masked_chip.test.tsx @@ -0,0 +1,45 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; + +import {renderWithContext, screen} from 'tests/react_testing_utils'; + +import MaskedChip from './masked_chip'; + +describe('MaskedChip', () => { + test('renders the masked token text', () => { + renderWithContext(); + expect(screen.getByText('••••••••')).toBeInTheDocument(); + }); + + test('has role="img" for accessibility', () => { + renderWithContext(); + const chip = screen.getByRole('img'); + expect(chip).toBeInTheDocument(); + }); + + test('has correct aria-label', () => { + renderWithContext(); + const chip = screen.getByRole('img'); + expect(chip).toHaveAttribute('aria-label', 'Hidden values that you do not have permission to view'); + }); + + test('does not have aria-readonly (invalid for role="img")', () => { + renderWithContext(); + const chip = screen.getByRole('img'); + expect(chip).not.toHaveAttribute('aria-readonly'); + }); + + test('does not render a close/remove button', () => { + renderWithContext(); + const removeButtons = document.querySelectorAll('.select__multi-value__remove'); + expect(removeButtons).toHaveLength(0); + }); + + test('has the masked CSS class', () => { + renderWithContext(); + const chip = screen.getByRole('img'); + expect(chip).toHaveClass('select__multi-value--masked'); + }); +}); diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/masked_chip.tsx b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/masked_chip.tsx new file mode 100644 index 00000000000..ed16e875d6f --- /dev/null +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/masked_chip.tsx @@ -0,0 +1,40 @@ +// 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 {WithTooltip} from '@mattermost/shared/components/tooltip'; + +import './selector_menus.scss'; + +/** Non-interactive chip indicating hidden attribute values exist in this condition. */ +const MaskedChip = (): JSX.Element => { + const {formatMessage} = useIntl(); + + const tooltipText = formatMessage({ + id: 'admin.access_control.masked_chip.tooltip', + defaultMessage: 'One or more restricted values', + }); + + const ariaLabel = formatMessage({ + id: 'admin.access_control.masked_chip.aria_label', + defaultMessage: 'Hidden values that you do not have permission to view', + }); + + return ( + +
+
+ {'••••••••'} +
+
+ + ); +}; + +export default MaskedChip; diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/multi_value_selector_menu.tsx b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/multi_value_selector_menu.tsx index e3d1b4f8294..9fc32e46b4a 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/multi_value_selector_menu.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/multi_value_selector_menu.tsx @@ -12,6 +12,8 @@ import * as Menu from 'components/menu'; import './selector_menus.scss'; +import MaskedChip from './masked_chip'; + // MultiValueSelector handles selection of multiple values (operator 'in') const MultiValueSelector = ({ values, @@ -20,6 +22,7 @@ const MultiValueSelector = ({ options = [], allowCreateValue = false, placeholder, + hasMaskedValues = false, }: { values: string[]; disabled: boolean; @@ -27,6 +30,7 @@ const MultiValueSelector = ({ options?: PropertyFieldOption[]; allowCreateValue?: boolean; placeholder?: string; + hasMaskedValues?: boolean; }) => { const {formatMessage} = useIntl(); const [filter, setFilter] = useState(''); @@ -92,6 +96,15 @@ const MultiValueSelector = ({ // Memoize cell contents to prevent unnecessary re-renders const cellContents = useMemo(() => { if (values.length === 0) { + // When no visible values exist but the row has masked ones, show only the masked chip. + if (hasMaskedValues) { + return ( +
+ +
+ ); + } + let visualPlaceholderText = defaultMultiPlaceholder; if (actualAllowCreateForMenu && options.length === 0) { visualPlaceholderText = defaultCreatePlaceholder; @@ -131,9 +144,10 @@ const MultiValueSelector = ({ )}
))} + {hasMaskedValues && } ); - }, [values, disabled]); + }, [values, disabled, hasMaskedValues]); return (
diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/selector_menus.scss b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/selector_menus.scss index 830b5935523..29b90eb25b0 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/selector_menus.scss +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/selector_menus.scss @@ -86,12 +86,24 @@ .select__multi-value__remove { padding: 0 4px; cursor: pointer; - + &:hover { background-color: rgba(var(--center-channel-color-rgb), 0.16); } } + .select__multi-value--masked { + background-color: rgba(var(--center-channel-color-rgb), 0.08); + cursor: default; + user-select: none; + + .select__multi-value__label { + color: rgba(var(--center-channel-color-rgb), 0.64); + font-family: monospace; + letter-spacing: 1px; + } + } + &__simple-input { width: 100%; height: 40px; diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/single_value_selector_menu.tsx b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/single_value_selector_menu.tsx index aafacab1877..47058012ef3 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/single_value_selector_menu.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/single_value_selector_menu.tsx @@ -12,6 +12,8 @@ import * as Menu from 'components/menu'; import Constants from 'utils/constants'; +import MaskedChip from './masked_chip'; + import './selector_menus.scss'; // SingleValueSelector handles selection of a single value (operators like 'is', 'contains', etc.) @@ -22,6 +24,7 @@ const SingleValueSelector = ({ options = [], allowCreateValue = false, placeholder, + hasMaskedValues = false, }: { value: string; disabled: boolean; @@ -29,6 +32,7 @@ const SingleValueSelector = ({ options?: PropertyFieldOption[]; allowCreateValue?: boolean; placeholder?: string; + hasMaskedValues?: boolean; }) => { const {formatMessage} = useIntl(); const [filter, setFilter] = useState(''); @@ -95,6 +99,21 @@ const SingleValueSelector = ({ } }, [allowCreateValue, filter, handleCreateValue]); + // When masked values are present and the caller holds no visible value, + // the row is effectively read-only — show only the masked chip. + // Placed AFTER hook declarations so hook order stays stable when the + // masked state changes between renders (e.g., parent re-renders after + // a sibling rule is deleted). + if (hasMaskedValues && !value) { + return ( +
+
+ +
+
+ ); + } + if (!hasOptions) { // For attributes without options, show simple input field return ( diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.scss b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.scss index 910a25a7198..755a4045666 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.scss +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.scss @@ -46,10 +46,15 @@ background: none; color: rgba(var(--center-channel-color-rgb), 0.56); cursor: pointer; - - &:hover { + + &:hover:not(:disabled) { color: var(--error-text); } + + &:disabled { + cursor: not-allowed; + opacity: 0.4; + } } &__actions-row { diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.test.tsx b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.test.tsx index 25e59f1fc6d..1c4f07d5b24 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.test.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.test.tsx @@ -27,6 +27,7 @@ describe('parseExpression', () => { operator: 'is', values: ['Engineering'], attribute_type: 'text', + hasMaskedValues: false, }, ]); }); @@ -50,6 +51,7 @@ describe('parseExpression', () => { operator: 'in', values: ['US', 'CA'], attribute_type: 'text', + hasMaskedValues: false, }, ]); }); @@ -73,6 +75,7 @@ describe('parseExpression', () => { operator: 'is not', values: ['guest'], attribute_type: 'text', + hasMaskedValues: false, }, ]); }); @@ -96,6 +99,7 @@ describe('parseExpression', () => { operator: 'starts with', values: ['admin'], attribute_type: 'text', + hasMaskedValues: false, }, ]); }); @@ -126,12 +130,14 @@ describe('parseExpression', () => { operator: 'starts with', values: ['admin'], attribute_type: 'text', + hasMaskedValues: false, }, { attribute: 'department', operator: 'is', values: ['Engineering'], attribute_type: 'text', + hasMaskedValues: false, }, ]); }); @@ -155,6 +161,7 @@ describe('parseExpression', () => { operator: 'is', values: ['foo'], attribute_type: 'text', + hasMaskedValues: false, }, ]); }); @@ -164,6 +171,44 @@ describe('parseExpression', () => { expect(parseExpression(undefined as any)).toEqual([]); expect(parseExpression({conditions: []})).toEqual([]); }); + + test('sets hasMaskedValues=true when condition has has_masked_values flag', () => { + const ast: AccessControlVisualAST = { + conditions: [ + { + attribute: 'user.attributes.program', + operator: 'in', + value: ['Alpha'], + value_type: 0, + attribute_type: 'text', + has_masked_values: true, + }, + { + attribute: 'user.attributes.clearance', + operator: 'in', + value: [], + value_type: 0, + attribute_type: 'text', + has_masked_values: true, + }, + { + attribute: 'user.attributes.department', + operator: '==', + value: 'Engineering', + value_type: 0, + attribute_type: 'text', + }, + ], + }; + + const rows = parseExpression(ast); + expect(rows[0].hasMaskedValues).toBe(true); // partial: caller holds Alpha + expect(rows[0].values).toEqual(['Alpha']); + expect(rows[1].hasMaskedValues).toBe(true); // fully masked: caller holds nothing + expect(rows[1].values).toEqual([]); + expect(rows[2].hasMaskedValues).toBe(false); // no masking + expect(rows[2].values).toEqual(['Engineering']); + }); }); describe('parseExpression with multiselect attributes', () => { @@ -186,6 +231,7 @@ describe('parseExpression with multiselect attributes', () => { operator: 'has all of', values: ['JavaScript', 'Python'], attribute_type: 'multiselect', + hasMaskedValues: false, }, ]); }); @@ -209,6 +255,7 @@ describe('parseExpression with multiselect attributes', () => { operator: 'has any of', values: ['Dragon', 'Phoenix'], attribute_type: 'multiselect', + hasMaskedValues: false, }, ]); }); @@ -232,6 +279,7 @@ describe('parseExpression with multiselect attributes', () => { operator: 'has all of', values: ['JavaScript'], attribute_type: 'multiselect', + hasMaskedValues: false, }, ]); }); @@ -348,6 +396,7 @@ describe('rowToCEL', () => { operator: 'has any of', values: ['Dragon', 'Phoenix'], attribute_type: 'multiselect', + hasMaskedValues: false, }); expect(cel).toBe('("Dragon" in user.attributes.programs || "Phoenix" in user.attributes.programs)'); }); @@ -358,6 +407,7 @@ describe('rowToCEL', () => { operator: 'has all of', values: ['Dragon', 'Phoenix'], attribute_type: 'multiselect', + hasMaskedValues: false, }); expect(cel).toBe('"Dragon" in user.attributes.programs && "Phoenix" in user.attributes.programs'); }); @@ -368,6 +418,7 @@ describe('rowToCEL', () => { operator: 'has any of', values: ['Dragon'], attribute_type: 'multiselect', + hasMaskedValues: false, }); expect(cel).toBe('"Dragon" in user.attributes.programs'); }); @@ -378,6 +429,7 @@ describe('rowToCEL', () => { operator: 'has all of', values: ['admin'], attribute_type: 'multiselect', + hasMaskedValues: false, }); expect(cel).toBe('"admin" in user.attributes.tags'); }); @@ -388,6 +440,7 @@ describe('rowToCEL', () => { operator: 'in', values: ['Eng', 'Ops'], attribute_type: 'select', + hasMaskedValues: false, }); expect(cel).toBe('user.attributes.department in ["Eng", "Ops"]'); }); @@ -398,6 +451,7 @@ describe('rowToCEL', () => { operator: 'is', values: ['TopSecret'], attribute_type: 'text', + hasMaskedValues: false, }); expect(cel).toBe('user.attributes.clearance == "TopSecret"'); }); @@ -408,6 +462,7 @@ describe('rowToCEL', () => { operator: 'contains', values: ['@example.com'], attribute_type: 'text', + hasMaskedValues: false, }); expect(cel).toBe('user.attributes.email.contains("@example.com")'); }); @@ -418,9 +473,42 @@ describe('rowToCEL', () => { operator: 'is', values: ['O\'Brien\'s "Team"'], attribute_type: 'text', + hasMaskedValues: false, }); expect(cel).toBe('user.attributes.team == "O\'Brien\'s \\"Team\\""'); }); + + // --- Masking-related tests --- + + test('fully-masked row (hasMaskedValues=true, values=[]) emits "in []" placeholder regardless of operator', () => { + // The placeholder is needed so the backend merge can locate this condition + // by attribute and re-inject the hidden values. The operator is irrelevant + // because the backend always overrides it from the stored expression. + const operators = ['in', 'is', 'has all of', 'has any of', 'contains', 'starts with']; + for (const operator of operators) { + const cel = rowToCEL({ + attribute: 'program', + operator, + values: [], + attribute_type: 'text', + hasMaskedValues: true, + }); + expect(cel).toBe('user.attributes.program in []'); + } + }); + + test('partially-masked row (hasMaskedValues=true, values non-empty) uses normal CEL path', () => { + // The caller holds "Alpha"; "Bravo" and "Charlie" are hidden. + // The row should emit the visible value normally — backend merge appends the rest. + const cel = rowToCEL({ + attribute: 'program', + operator: 'in', + values: ['Alpha'], + attribute_type: 'text', + hasMaskedValues: true, + }); + expect(cel).toBe('user.attributes.program in ["Alpha"]'); + }); }); describe('isSimpleExpression', () => { diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx index aef6181c239..f7de92128ea 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/table_editor.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React, {useState, useEffect, useCallback} from 'react'; +import React, {useState, useEffect, useCallback, useMemo} from 'react'; import {FormattedMessage, useIntl} from 'react-intl'; import type {AccessControlVisualAST} from '@mattermost/types/access_control'; @@ -28,6 +28,16 @@ export function celStringLiteral(val: string): string { } export function rowToCEL(row: TableRow): string { + // A fully-masked row has no visible values on the client side. Emit a + // placeholder "in []" expression so the backend merge can locate this + // condition by attribute and re-inject the hidden values before persisting. + // Without this guard the condition would be filtered out by updateExpression, + // the empty expression would be sent to the server, and buildCELFromConditions + // would return "true" — making the policy wide-open (security regression). + if (row.hasMaskedValues && row.values.length === 0) { + return `user.attributes.${row.attribute} in []`; + } + const attributeExpr = `user.attributes.${row.attribute}`; const config = OPERATOR_CONFIG[row.operator]; @@ -82,6 +92,9 @@ interface TableEditorProps { // Props for user self-exclusion detection isSystemAdmin?: boolean; validateExpressionAgainstRequester?: (expression: string) => Promise>; + + // Callback to notify parent when masked state changes (for CEL editor integration) + onMaskedStateChange?: (hasMasked: boolean) => void; } // Finds the first available (non-disabled) attribute from a list of user attributes. @@ -131,8 +144,10 @@ export const parseExpression = (visualAST: AccessControlVisualAST): TableRow[] = let values; if (Array.isArray(node.value)) { values = node.value; - } else { + } else if (node.value !== null && node.value !== undefined) { values = [node.value]; + } else { + values = []; } tableRows.push({ @@ -140,12 +155,33 @@ export const parseExpression = (visualAST: AccessControlVisualAST): TableRow[] = operator: op, values, attribute_type: node.attribute_type, + hasMaskedValues: node.has_masked_values === true, }); } return tableRows; }; +function getTestButtonTooltip( + hasMaskedRows: boolean, + userWouldBeExcluded: boolean, + formatMessage: ReturnType['formatMessage'], +): string | undefined { + if (hasMaskedRows) { + return formatMessage({ + id: 'admin.access_control.table_editor.masked_values_tooltip', + defaultMessage: 'Test is unavailable because this policy contains restricted attribute values.', + }); + } + if (userWouldBeExcluded) { + return formatMessage({ + id: 'admin.access_control.table_editor.user_excluded_tooltip', + defaultMessage: 'You cannot test access rules that would exclude you from the channel', + }); + } + return undefined; +} + // TableEditor provides a user-friendly table interface for constructing and editing // CEL (Common Expression Language) expressions based on user attributes. // It parses incoming CEL expressions into rows and reconstructs the expression upon changes. @@ -164,6 +200,7 @@ function TableEditor({ actions, isSystemAdmin = false, validateExpressionAgainstRequester, + onMaskedStateChange, }: TableEditorProps): JSX.Element { const {formatMessage} = useIntl(); @@ -175,10 +212,18 @@ function TableEditor({ // State for user self-exclusion detection (only applies to non-system-admins) const [userWouldBeExcluded, setUserWouldBeExcluded] = useState(false); - // Effect to parse the incoming CEL expression string (value prop) - // and update the internal rows state. Handles errors during parsing. + // Derived state: whether any row has masked values + const hasMaskedRows = useMemo(() => rows.some((r) => r.hasMaskedValues), [rows]); + + // Prevents getVisualAST re-parse when expression change is from internal row editing. + const isInternalChange = React.useRef(false); + useEffect(() => { - // Skip parsing if no expression to avoid unnecessary API calls + if (isInternalChange.current) { + isInternalChange.current = false; + return; + } + if (!value || value.trim() === '') { setRows([]); return; @@ -209,10 +254,8 @@ function TableEditor({ }); }, [value]); - // Effect to check if user would be excluded by their own rules useEffect(() => { const checkUserSelfExclusion = async () => { - // Only check for non-system admins when there's an expression and validation function if (isSystemAdmin || !value.trim() || !validateExpressionAgainstRequester) { setUserWouldBeExcluded(false); return; @@ -221,8 +264,7 @@ function TableEditor({ try { const result = await validateExpressionAgainstRequester(value); setUserWouldBeExcluded(!result.data?.requester_matches); - } catch (error) { - // If validation fails, assume they would not be excluded (to allow testing) + } catch { setUserWouldBeExcluded(false); } }; @@ -230,28 +272,30 @@ function TableEditor({ checkUserSelfExclusion(); }, [value, isSystemAdmin, validateExpressionAgainstRequester]); - // Converts the internal rows state back into a CEL expression string - // and calls the onChange and onValidate props. + useEffect(() => { + onMaskedStateChange?.(hasMaskedRows); + }, [hasMaskedRows, onMaskedStateChange]); + const updateExpression = useCallback((newRows: TableRow[]) => { - const rowsThatCanFormExpressions = newRows.filter((row) => row.attribute && row.values.length > 0); + // Include masked rows with no visible values: rowToCEL will emit an "in []" + // placeholder so the backend merge can restore the hidden values on save. + const rowsThatCanFormExpressions = newRows.filter((row) => row.attribute && (row.values.length > 0 || row.hasMaskedValues)); const expr = rowsThatCanFormExpressions.map((row) => rowToCEL(row)).join(' && '); + isInternalChange.current = true; onChange(expr); if (onValidate) { onValidate(expr === '' || rowsThatCanFormExpressions.length > 0); } }, [onChange, onValidate]); - // Helper function to find the first available (non-disabled) attribute const findFirstAvailableAttribute = useCallback(() => { return findFirstAvailableAttributeFromList(userAttributes, enableUserManagedAttributes); }, [userAttributes, enableUserManagedAttributes]); - // Row Manipulation Handlers const addRow = useCallback(() => { if (userAttributes.length === 0) { - // Show a helpful message instead of silently failing onParseError('No user attributes available. Please ensure ABAC is properly configured and you have the necessary permissions.'); return; } @@ -263,11 +307,12 @@ function TableEditor({ } setRows((currentRows) => { - const newRow = { + const newRow: TableRow = { attribute: firstAvailableAttribute.name, operator: firstAvailableAttribute.type === 'multiselect' ? OperatorLabel.HAS_ANY_OF : OperatorLabel.IS, values: [], attribute_type: firstAvailableAttribute.type || '', + hasMaskedValues: false, }; const newRows = [...currentRows, newRow]; updateExpression(newRows); // Ensure expression is updated immediately @@ -284,6 +329,12 @@ function TableEditor({ }); }, [updateExpression]); + const requestRemoveRow = useCallback((index: number) => { + // Masked rows have their remove button disabled — the row is read-only + // because the server would 403 on a delete that strips hidden values. + removeRow(index); + }, [removeRow]); + const updateRowAttribute = useCallback((index: number, attribute: string) => { setRows((currentRows) => { const newRows = [...currentRows]; @@ -406,7 +457,7 @@ function TableEditor({ updateRowAttribute(index, attribute)} menuId={`attribute-selector-menu-${index}`} buttonId={`attribute-selector-button-${index}`} @@ -418,7 +469,7 @@ function TableEditor({ updateRowOperator(index, operator)} attributeType={userAttributes.find((attr) => attr.name === row.attribute)?.type} /> @@ -426,7 +477,7 @@ function TableEditor({ updateRowValues(index, values)} options={row.attribute ? userAttributes.find((attr) => attr.name === row.attribute)?.attrs?.options || [] : []} /> @@ -435,8 +486,8 @@ function TableEditor({
diff --git a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/value_selector_menu.tsx b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/value_selector_menu.tsx index 4d8aec425e3..ec3426ea596 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/table_editor/value_selector_menu.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/table_editor/value_selector_menu.tsx @@ -15,6 +15,7 @@ export interface TableRow { operator: string; values: string[]; attribute_type: string; + hasMaskedValues: boolean; } export interface ValueSelectorMenuProps { @@ -26,7 +27,6 @@ export interface ValueSelectorMenuProps { placeholder?: string; } -// Main ValueSelectorMenu component that delegates to the appropriate selector const ValueSelectorMenu = ({ row, disabled, @@ -46,11 +46,11 @@ const ValueSelectorMenu = ({ options={options} allowCreateValue={allowCreateValue} placeholder={placeholder} + hasMaskedValues={row.hasMaskedValues} /> ); } - // For single-value operators return ( ); }; diff --git a/webapp/channels/src/components/admin_console/access_control/policies.test.tsx b/webapp/channels/src/components/admin_console/access_control/policies.test.tsx index c36f7a30298..c4ffb43c115 100644 --- a/webapp/channels/src/components/admin_console/access_control/policies.test.tsx +++ b/webapp/channels/src/components/admin_console/access_control/policies.test.tsx @@ -143,6 +143,88 @@ describe('components/admin_console/access_control/PolicyList', () => { expect(screen.getByText('Delete')).toBeInTheDocument(); }); + test('Delete menu item is disabled when a policy contains masked values', async () => { + // The "--------" sentinel in a rule expression means the caller can't + // see at least one referenced value. Server enforces a 403 on delete + // in that case, so the menu item must be disabled to avoid a useless + // confirmation modal → 403 round-trip. + mockSearchPolicies.mockResolvedValue({ + data: { + policies: [{ + id: 'masked-policy', + name: 'Masked Policy', + rules: [{actions: ['*'], expression: 'user.attributes.program in ["Alpha", "--------"]'}], + } as unknown as AccessControlPolicy], + total: 1, + }, + } as ActionResult); + renderWithContext(); + await waitFor(() => { + expect(screen.getByText('Masked Policy')).toBeInTheDocument(); + }); + + const menuButton = document.getElementById('policy-menu-masked-policy')!; + await userEvent.click(menuButton); + + const deleteItem = document.getElementById('policy-menu-delete-masked-policy')!; + expect(deleteItem).toHaveAttribute('aria-disabled', 'true'); + }); + + test('Delete menu item is enabled for a clean policy with no channels', async () => { + // Sanity: a policy that has neither child channels nor masked values + // must keep Delete enabled. + mockSearchPolicies.mockResolvedValue({ + data: { + policies: [{ + id: 'clean-policy', + name: 'Clean Policy', + rules: [{actions: ['*'], expression: 'user.attributes.program in ["Alpha"]'}], + } as unknown as AccessControlPolicy], + total: 1, + }, + } as ActionResult); + renderWithContext(); + await waitFor(() => { + expect(screen.getByText('Clean Policy')).toBeInTheDocument(); + }); + + const menuButton = document.getElementById('policy-menu-clean-policy')!; + await userEvent.click(menuButton); + + const deleteItem = document.getElementById('policy-menu-delete-clean-policy')!; + expect(deleteItem).not.toHaveAttribute('aria-disabled', 'true'); + }); + + test('Delete confirmation modal no longer surfaces the masked-values warning', async () => { + // The inner-modal "This policy contains restricted values" notice was + // removed once we started disabling the Delete menu item upstream. + // Open the modal on a clean policy and assert the warning text is gone. + mockSearchPolicies.mockResolvedValue({ + data: { + policies: [{ + id: 'clean-policy', + name: 'Clean Policy', + rules: [{actions: ['*'], expression: 'user.attributes.program in ["Alpha"]'}], + } as unknown as AccessControlPolicy], + total: 1, + }, + } as ActionResult); + renderWithContext(); + await waitFor(() => { + expect(screen.getByText('Clean Policy')).toBeInTheDocument(); + }); + + const menuButton = document.getElementById('policy-menu-clean-policy')!; + await userEvent.click(menuButton); + await userEvent.click(screen.getByText('Delete')); + + await waitFor(() => { + expect(screen.getByText('Confirm Policy Deletion')).toBeInTheDocument(); + }); + expect(screen.queryByText(/This policy includes attribute values that are hidden from you/)).not.toBeInTheDocument(); + expect(screen.queryByText('This policy contains restricted values')).not.toBeInTheDocument(); + }); + test('should get columns correctly', async () => { mockSearchPolicies.mockResolvedValue({data: {policies: [], total: 0}} as ActionResult); renderWithContext(); diff --git a/webapp/channels/src/components/admin_console/access_control/policies.tsx b/webapp/channels/src/components/admin_console/access_control/policies.tsx index 0452dd9909a..795566a585a 100644 --- a/webapp/channels/src/components/admin_console/access_control/policies.tsx +++ b/webapp/channels/src/components/admin_console/access_control/policies.tsx @@ -1,9 +1,10 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React, {useState, useEffect, useMemo} from 'react'; +import React, {useState, useEffect, useMemo, useCallback} from 'react'; import {FormattedMessage, useIntl} from 'react-intl'; +import {GenericModal} from '@mattermost/components'; import {Button} from '@mattermost/shared/components/button'; import type {AccessControlPolicy} from '@mattermost/types/access_control'; @@ -12,11 +13,23 @@ import type {ActionResult} from 'mattermost-redux/types/actions'; import type {Row, Column} from 'components/admin_console/data_grid/data_grid'; import DataGrid from 'components/admin_console/data_grid/data_grid'; import * as Menu from 'components/menu'; +import SectionNotice from 'components/section_notice'; import {getHistory} from 'utils/browser_history'; import './policies.scss'; +// The server emits the eight-dash masked-token sentinel inside raw CEL expressions +// when masking values the caller cannot see (e.g. `attr == "--------"`). The full +// visual AST carries a typed `has_masked_values` flag per condition, but on the +// policies list page we only have the raw expression strings — so we detect masking +// by the quoted token substring. +const MASKED_VALUE_TOKEN_LITERAL = '"--------"'; + +function policyHasMaskedValues(policy: AccessControlPolicy): boolean { + return policy.rules?.some((rule) => rule.expression?.includes(MASKED_VALUE_TOKEN_LITERAL)) ?? false; +} + type Props = { onPolicySelected?: (policy: AccessControlPolicy) => void; onPoliciesLoaded?: (count: number) => void; @@ -41,6 +54,8 @@ export default function PolicyList(props: Props): JSX.Element { const [searchErrored, setSearchErrored] = useState(false); const [cursorHistory, setCursorHistory] = useState([]); const [total, setTotal] = useState(0); + const [pendingDeletePolicy, setPendingDeletePolicy] = useState(null); + const [deleteError, setDeleteError] = useState(null); const intl = useIntl(); const history = useMemo(() => getHistory(), []); @@ -157,10 +172,38 @@ export default function PolicyList(props: Props): JSX.Element { ); }; - const handleDelete = async (policyId: string) => { - await props.actions.deletePolicy(policyId); + const initiateDelete = useCallback((policy: AccessControlPolicy) => { + setPendingDeletePolicy(policy); + setDeleteError(null); + }, []); + + const confirmDelete = useCallback(async () => { + if (!pendingDeletePolicy) { + return; + } + const result = await props.actions.deletePolicy(pendingDeletePolicy.id); + if (result?.error) { + // The server enforces masked-policy / permission rejections (403). Surface + // the message in the modal so the user sees why deletion failed instead of + // a silent close + stale list refresh. + const errorId = result.error.server_error_id; + if (errorId === 'app.pap.delete_policy.masked_values') { + setDeleteError(intl.formatMessage({ + id: 'admin.access_control.delete_policy.masked_values', + defaultMessage: 'You cannot delete this policy because it contains attribute values you do not have permission to view.', + })); + } else { + setDeleteError(result.error.message || intl.formatMessage({ + id: 'admin.access_control.delete_policy.generic_error', + defaultMessage: 'Failed to delete the policy.', + })); + } + return; + } + setPendingDeletePolicy(null); + setDeleteError(null); fetchPolicies(search); - }; + }, [pendingDeletePolicy, search, intl]); const getRows = (): Row[] => { return policies.map((policy: AccessControlPolicy) => { @@ -223,7 +266,7 @@ export default function PolicyList(props: Props): JSX.Element { {!props.hideDeleteAction && ( handleDelete(policy.id)} + onClick={() => initiateDelete(policy)} leadingElement={} labels={ } isDestructive={true} - disabled={Boolean(policy.props?.child_ids?.length)} + + // Also disable when the policy contains values masked + // for this caller. Mirrors the policy-details Delete + // guard — server returns 403, so otherwise the modal + // flow would just round-trip an error. + disabled={Boolean(policy.props?.child_ids?.length) || policyHasMaskedValues(policy)} /> )} @@ -404,6 +452,54 @@ export default function PolicyList(props: Props): JSX.Element { ) : undefined} /> + {pendingDeletePolicy && ( + { + setPendingDeletePolicy(null); + setDeleteError(null); + }} + handleConfirm={confirmDelete} + handleCancel={() => { + setPendingDeletePolicy(null); + setDeleteError(null); + }} + modalHeaderText={ + + } + confirmButtonText={ + + } + confirmButtonVariant='destructive' + compassDesign={true} + > + <> + + {deleteError && ( +
+ + } + text={deleteError} + /> +
+ )} + + + )} ); } diff --git a/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap b/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap index 7c9f4c8860e..b9e4021c4f8 100644 --- a/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap +++ b/webapp/channels/src/components/admin_console/access_control/policy_details/__snapshots__/policy_details.test.tsx.snap @@ -87,99 +87,8 @@ exports[`components/admin_console/access_control/policy_details/PolicyDetails sh style="height: 0px;" >
- - - - - - - - - - - - - - - - - - -
- Attribute - - Operator - - - Values - - -
- - Select a user attribute and values to create a rule - -
- -
-
-
-

- Each row is a single condition that must be met for a user to comply with the policy. All rules are combined with logical AND operator ( - - - && - - - ). -

-
- -
-
+ data-testid="table-editor" + />
- - - - - - - - - - - - - - - - - - -
- Attribute - - Operator - - - Values - - -
- - Select a user attribute and values to create a rule - -
- -
-
-
-

- Each row is a single condition that must be met for a user to comply with the policy. All rules are combined with logical AND operator ( - - - && - - - ). -

-
- -
-
+ data-testid="table-editor" + />
({ }), })); +// Mock TableEditor so tests can control onMaskedStateChange callbacks. +// jest.mock factory may not reference out-of-scope variables, so React is required inline. +jest.mock('../editors/table_editor/table_editor', () => { + const reactLib = require('react'); + return jest.fn(({onMaskedStateChange}: any) => { + reactLib.useEffect(() => { + onMaskedStateChange?.(false); + }, []); + return reactLib.createElement('div', {'data-testid': 'table-editor'}); + }); +}); + +// Mock CELEditor — its real implementation boots Monaco on mount, which is +// not available in JSDOM. The mode-toggle tests only care that switching to +// Advanced/Simple flips state in the parent, not how Monaco renders. +jest.mock('../editors/cel_editor/editor', () => { + const reactLib = require('react'); + return jest.fn(() => reactLib.createElement('div', {'data-testid': 'cel-editor'})); +}); + // Mock the useChannelAccessControlActions hook jest.mock('hooks/useChannelAccessControlActions', () => ({ useChannelAccessControlActions: jest.fn(), @@ -151,11 +171,9 @@ describe('components/admin_console/access_control/policy_details/PolicyDetails', ...defaultProps.actions, fetchPolicy: jest.fn().mockResolvedValue({ data: { - policy: { - id: 'policy1', - name: 'Policy 1', - rules: [{expression: 'true'}], - }, + id: 'policy1', + name: 'Policy 1', + rules: [{expression: 'true'}], }, }), }, @@ -164,6 +182,140 @@ describe('components/admin_console/access_control/policy_details/PolicyDetails', expect(container).toMatchSnapshot(); }); + test('should show masked values warning banner when policy has masked rows', async () => { + // hasMaskedRows is derived in policy_details from the presence of the + // "--------" sentinel in the loaded expression — drive the test via a + // fetched policy carrying a masked rule. + const props = { + ...defaultProps, + actions: { + ...defaultProps.actions, + fetchPolicy: jest.fn().mockResolvedValue({ + data: { + id: 'policy1', + name: 'Policy 1', + rules: [{ + actions: ['*'], + expression: 'user.attributes.program in ["Alpha", "--------"]', + }], + }, + }), + }, + }; + renderWithContext(); + + await waitFor(() => { + expect(screen.getByText('This policy contains restricted values')).toBeInTheDocument(); + }); + expect(screen.getByText(/Some rules include attribute values you cannot see/)).toBeInTheDocument(); + }); + + test('should not show masked values warning banner when no masked rows', async () => { + renderWithContext(); + + await waitFor(() => { + expect(screen.queryByText('This policy contains restricted values')).not.toBeInTheDocument(); + }); + }); + + test('hasMaskedRows derivation survives Simple → Advanced → Simple mode toggles', async () => { + // Regression guard: hasMaskedRows must come from the expression itself, + // not from a TableEditor lifecycle callback. Toggling editor modes + // remounts TableEditor; if the parent reset hasMaskedRows on remount, + // the warning banner would flicker off and the CEL/delete gates would + // briefly open. Deriving from the "--------" sentinel in the expression + // is the only source of truth that's lifecycle-independent. + + // The mode-toggle button is disabled while no usable attributes are + // available, so the test needs at least one to actually exercise the + // Simple → Advanced → Simple round-trip. + mockGetAccessControlFields.mockResolvedValue({data: [{name: 'program', attrs: {ldap: true}}]}); + + const props = { + ...defaultProps, + actions: { + ...defaultProps.actions, + fetchPolicy: jest.fn().mockResolvedValue({ + data: { + id: 'policy1', + name: 'Policy 1', + rules: [{ + actions: ['*'], + expression: 'user.attributes.program in ["Alpha", "--------"]', + }], + }, + }), + }, + }; + renderWithContext(); + + // Banner present after initial load (Simple mode). + await waitFor(() => { + expect(screen.getByText('This policy contains restricted values')).toBeInTheDocument(); + }); + + // Switch to Advanced mode — banner must remain (it lives outside the + // editor swap, gated by hasMaskedRows which is expression-derived). + const toAdvanced = screen.getByText('Switch to Advanced Mode'); + await userEvent.click(toAdvanced); + expect(screen.getByText('This policy contains restricted values')).toBeInTheDocument(); + + // Switch back to Simple mode — banner must STILL be there. Before the + // fix, the TableEditor remount transiently flipped hasMaskedRows to + // false and the banner disappeared. + const toSimple = screen.getByText('Switch to Simple Mode'); + await userEvent.click(toSimple); + expect(screen.getByText('This policy contains restricted values')).toBeInTheDocument(); + }); + + test('hasMaskedRows stays false for a policy without the masked-token sentinel', async () => { + // Negative case: a normal policy expression must not trip the + // masked-rows banner. + const props = { + ...defaultProps, + actions: { + ...defaultProps.actions, + fetchPolicy: jest.fn().mockResolvedValue({ + data: { + id: 'policy1', + name: 'Policy 1', + rules: [{ + actions: ['*'], + expression: 'user.attributes.program in ["Alpha", "Bravo"]', + }], + }, + }), + }, + }; + renderWithContext(); + + await waitFor(() => { + expect(screen.getByText('Delete policy')).toBeInTheDocument(); + }); + expect(screen.queryByText('This policy contains restricted values')).not.toBeInTheDocument(); + }); + + // Note: when hasMaskedRows is true the Delete button is disabled (policy_details.tsx), + // so the masked-warning inside the confirmation modal is defense-in-depth and not + // reachable through normal UI flow. Test only the no-masked-rows path here. + + test('should not show masked values warning in delete confirmation modal when no masked rows', async () => { + renderWithContext(); + + await waitFor(() => { + expect(screen.getByText('Delete policy')).toBeInTheDocument(); + }); + + const deleteButtons = screen.getAllByText('Delete'); + await userEvent.click(deleteButtons[deleteButtons.length - 1]); + + await waitFor(() => { + expect(screen.getByText('Confirm Policy Deletion')).toBeInTheDocument(); + }); + + expect(screen.queryByText(/This policy includes attribute values that are hidden from you/)).not.toBeInTheDocument(); + }); + test('should handle delete policy', async () => { const props = { ...defaultProps, diff --git a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx index d278b278563..8dc765642a2 100644 --- a/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx +++ b/webapp/channels/src/components/admin_console/access_control/policy_details/policy_details.tsx @@ -81,6 +81,16 @@ function PolicyDetails({ const [serverError, setServerError] = useState(undefined); const [addChannelOpen, setAddChannelOpen] = useState(false); const [editorMode, setEditorMode] = useState<'cel' | 'table'>('table'); + + // Derive masked-rows state directly from the expression rather than relying + // on a callback from TableEditor: TableEditor unmounts on mode switches, and + // on remount its rows-derived flag flickers false-then-true while the async + // AST round-trip is in flight, briefly opening gates (CEL read-only, delete, + // banner) that should stay closed. The "--------" sentinel is what the + // server emits in raw CEL for any value the caller can't see, so its + // presence in the expression is a stable signal independent of editor + // lifecycle. + const hasMaskedRows = useMemo(() => expression.includes('"--------"'), [expression]); const [channelChanges, setChannelChanges] = useState({ removed: {}, added: {}, @@ -197,6 +207,14 @@ function PolicyDetails({ if (result.error) { if (result.error.server_error_id === 'app.pap.save_policy.name_exists.app_error') { setServerError(formatMessage({id: 'admin.access_control.edit_policy.name_exists', defaultMessage: 'A policy with this name already exists. Please choose a different name.'})); + } else if (result.error.server_error_id === 'app.pap.save_policy.invalid_value') { + setServerError(formatMessage({id: 'admin.access_control.edit_policy.invalid_value', defaultMessage: 'Invalid value.'})); + } else if (result.error.server_error_id === 'app.pap.save_policy.self_exclusion') { + setServerError(formatMessage({id: 'admin.access_control.edit_policy.self_exclusion', defaultMessage: 'You do not satisfy one or more conditions in this policy. Contact a System Admin for assistance.'})); + } else if (result.error.server_error_id === 'app.pap.save_policy.masked_condition_deleted') { + setServerError(formatMessage({id: 'admin.access_control.edit_policy.masked_condition_deleted', defaultMessage: 'You cannot remove a condition that contains attribute values you do not have permission to view.'})); + } else if (result.error.server_error_id === 'app.pap.save_policy.masked_rule_deleted') { + setServerError(formatMessage({id: 'admin.access_control.edit_policy.masked_rule_deleted', defaultMessage: 'You cannot remove a rule that contains attribute values you do not have permission to view.'})); } else { setServerError(result.error.message); } @@ -506,6 +524,23 @@ function PolicyDetails({ /> + {hasMaskedRows && ( +
+ + } + text={formatMessage({ + id: 'admin.access_control.policy.edit_policy.masked_values_warning.text', + defaultMessage: 'Some rules include attribute values you cannot see. Editing or deleting these rules may change who has access in ways you cannot fully anticipate.', + })} + /> +
+ )} {editorMode === 'cel' ? ( {}} disabled={noUsableAttributes} + hasMaskedRows={hasMaskedRows} userAttributes={autocompleteResult. filter((attr) => { if (accessControlSettings.EnableUserManagedAttributes) { @@ -613,6 +649,23 @@ function PolicyDetails({ expanded={true} className={'console delete-policy'} > + {hasMaskedRows && ( +
+ + } + text={formatMessage({ + id: 'admin.access_control.policy.edit_policy.delete_policy.masked_values_warning.text', + defaultMessage: 'Removing this policy could affect access for users you cannot fully account for.', + })} + /> +
+ )} @@ -698,10 +751,12 @@ function PolicyDetails({ confirmButtonVariant='destructive' compassDesign={true} > - + <> + + )} diff --git a/webapp/channels/src/components/team_settings/team_access_policies_tab/team_policy_editor.scss b/webapp/channels/src/components/team_settings/team_access_policies_tab/team_policy_editor.scss index e521b0a2c12..9338a9808c1 100644 --- a/webapp/channels/src/components/team_settings/team_access_policies_tab/team_policy_editor.scss +++ b/webapp/channels/src/components/team_settings/team_access_policies_tab/team_policy_editor.scss @@ -2,6 +2,10 @@ padding-bottom: 80px; margin-top: -8px; + &__masked-values-warning { + margin-bottom: 16px; + } + &__header { margin-bottom: 0; } diff --git a/webapp/channels/src/components/team_settings/team_access_policies_tab/team_policy_editor.tsx b/webapp/channels/src/components/team_settings/team_access_policies_tab/team_policy_editor.tsx index ecb7b71cea1..e23aec85cea 100644 --- a/webapp/channels/src/components/team_settings/team_access_policies_tab/team_policy_editor.tsx +++ b/webapp/channels/src/components/team_settings/team_access_policies_tab/team_policy_editor.tsx @@ -107,6 +107,7 @@ export default function TeamPolicyEditor({ const [showConfirmationModal, setShowConfirmationModal] = useState(false); const [showDeleteModal, setShowDeleteModal] = useState(false); const [backClicked, setBackClicked] = useState(false); + const [hasMaskedRows, setHasMaskedRows] = useState(false); const noUsableAttributes = attributesLoaded && !hasUsableAttributes(autocompleteResult, accessControlSettings.EnableUserManagedAttributes); @@ -262,7 +263,7 @@ export default function TeamPolicyEditor({ setSaveChangesPanelState(SAVE_RESULT_ERROR); return false; } - if (expression.includes('== ""') || expression.includes("== ''") || expression.includes('in []')) { + if (expression.includes('== ""') || expression.includes("== ''")) { setFormError(formatMessage({id: 'team_settings.policy_editor.error.incomplete_rule', defaultMessage: 'Please complete all attribute rules with a value'})); setSaveChangesPanelState(SAVE_RESULT_ERROR); return false; @@ -510,6 +511,23 @@ export default function TeamPolicyEditor({

+ {hasMaskedRows && ( +
+ + } + text={formatMessage({ + id: 'admin.access_control.policy.edit_policy.masked_values_warning.text', + defaultMessage: 'Some rules include attribute values you cannot see. Editing or deleting these rules may change who has access in ways you cannot fully anticipate.', + })} + /> +
+ )} @@ -617,7 +636,7 @@ export default function TeamPolicyEditor({