diff --git a/server/channels/app/access_control.go b/server/channels/app/access_control.go index 62a4392da7ff..7e1eb5ed772f 100644 --- a/server/channels/app/access_control.go +++ b/server/channels/app/access_control.go @@ -134,7 +134,8 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model. } // Merge hidden values back in and block deletion of masked conditions. - if appErr := a.mergeStoredPolicyExpressions(rctx, policy, callerID); appErr != nil { + mergedHidden, appErr := a.mergeStoredPolicyExpressions(rctx, policy, callerID) + if appErr != nil { return nil, appErr } @@ -144,7 +145,7 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model. // clearance themselves). Masking and write-path value validation still // apply to system admins above. if !a.HasPermissionTo(callerID, model.PermissionManageSystem) { - if appErr := a.checkSelfInclusion(rctx, policy, callerID); appErr != nil { + if appErr := a.checkSelfInclusion(rctx, policy, callerID, mergedHidden); appErr != nil { return nil, appErr } } @@ -168,7 +169,6 @@ func (a *App) CreateOrUpdateAccessControlPolicy(rctx request.CTX, policy *model. // policyHasMaskedValuesForCaller returns true if policy contains any attribute values // that are not visible to callerID under the current masking rules. -// A nil policy is treated as "no hidden values" — there's nothing to protect. func (a *App) policyHasMaskedValuesForCaller(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) (bool, *model.AppError) { if policy == nil { return false, nil @@ -182,6 +182,9 @@ func (a *App) policyHasMaskedValuesForCaller(rctx request.CTX, policy *model.Acc if appErr != nil { return false, appErr } + if maskedAST == nil { + continue + } for _, cond := range maskedAST.Conditions { if cond.HasMaskedValues { return true, nil @@ -193,21 +196,21 @@ func (a *App) policyHasMaskedValuesForCaller(rctx request.CTX, policy *model.Acc } // mergeStoredPolicyExpressions re-injects hidden values from the stored policy into the -// submitted one, and blocks the save if the caller removed a condition that contained -// values they cannot see (which would silently widen access beyond what they could audit). -// No-op for new policies (not found in store). -func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) *model.AppError { +// submitted one, and blocks the save if the caller removed a condition with values they +// cannot see. No-op for new policies (not found in store). +// Returns true when at least one hidden value was re-injected. +func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) (bool, *model.AppError) { acs := a.Srv().ch.AccessControl if acs == nil { - return nil + return false, nil } existingPolicy, appErr := acs.GetPolicy(rctx, policy.ID) if appErr != nil { if appErr.StatusCode == http.StatusNotFound { - return nil + return false, nil } - return appErr + return false, appErr } // Pair submitted and stored rules by Name so that a reorder / @@ -232,6 +235,7 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces pairedNames := make(map[string]bool, len(existingPolicy.Rules)) membershipPaired := false + mergedHidden := false for i := range policy.Rules { rule := &policy.Rules[i] @@ -267,7 +271,7 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces submittedExpr := rule.Expression mergedExpr, appErr := a.mergeExpressionWithMaskedValues(rctx, policy.ID, submittedExpr, stored.Expression, callerID) if appErr != nil { - return appErr + return false, appErr } rule.Expression = mergedExpr // Hidden values were re-injected → caller was working from a @@ -277,6 +281,7 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces if mergedExpr != submittedExpr { rule.Actions = stored.Actions rule.Role = stored.Role + mergedHidden = true } } @@ -308,15 +313,14 @@ func (a *App) mergeStoredPolicyExpressions(rctx request.CTX, policy *model.Acces } hasMasked, appErr := a.expressionHasMaskedValuesForCaller(rctx, stored.Expression, callerID) if appErr != nil { - return appErr + return false, appErr } if hasMasked { - return model.NewAppError("MergeStoredPolicyExpressions", "app.pap.save_policy.masked_rule_deleted", nil, - "cannot remove a rule that contains attribute values you do not hold", http.StatusForbidden) + return false, saveForbiddenError(rctx, "MergeStoredPolicyExpressions", "masked_rule_deleted: cannot remove a rule that contains attribute values you do not hold") } } - return nil + return mergedHidden, nil } // isMembershipRule reports whether a rule fills the policy's @@ -339,6 +343,9 @@ func (a *App) expressionHasMaskedValuesForCaller(rctx request.CTX, storedExpr, c if appErr != nil { return false, appErr } + if maskedAST == nil { + return false, nil + } for _, cond := range maskedAST.Conditions { if cond.HasMaskedValues { return true, nil @@ -390,10 +397,7 @@ func (a *App) mergeExpressionWithMaskedValues(rctx request.CTX, policyID, submit mlog.String("policy_id", policyID), mlog.String("caller_id", callerID), ) - return "", model.NewAppError("mergeExpressionWithMaskedValues", - "app.pap.save_policy.advanced_expression_blocked", nil, - "this rule expression cannot be safely edited while restricted values are present", - http.StatusForbidden) + return "", saveForbiddenError(rctx, "mergeExpressionWithMaskedValues", "advanced_expression_blocked: stored rule expression cannot be safely edited while restricted values are present") } cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) @@ -437,8 +441,7 @@ func (a *App) mergeExpressionWithMaskedValues(rctx request.CTX, policyID, submit } attr := storedAST.Conditions[i].Attribute if submittedCounts[attr] < storedCounts[attr] { - return "", model.NewAppError("mergeExpressionWithMaskedValues", "app.pap.save_policy.masked_condition_deleted", nil, - "cannot remove a rule condition that contains attribute values you do not hold", http.StatusForbidden) + return "", saveForbiddenError(rctx, "mergeExpressionWithMaskedValues", "masked_condition_deleted: cannot remove a rule condition that contains attribute values you do not hold") } } @@ -498,8 +501,19 @@ func (a *App) mergeExpressionWithMaskedValues(rctx request.CTX, policyID, submit return buildCELFromConditions(mergedConditions), nil } +// saveForbiddenError logs the rejection reason internally and returns a generic 403. +func saveForbiddenError(rctx request.CTX, where, internalReason string) *model.AppError { + rctx.Logger().Info("save policy refused (see internal_reason for details)", + mlog.String("where", where), + mlog.String("internal_reason", internalReason), + ) + return model.NewAppError(where, "app.pap.save_policy.forbidden", nil, "", http.StatusForbidden) +} + // checkSelfInclusion verifies the caller satisfies all policy rules after their edit. -func (a *App) checkSelfInclusion(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) *model.AppError { +// When mergedHidden is true (hidden values were re-injected), a self-exclusion failure +// returns the generic forbidden error; otherwise the specific self_exclusion error is used. +func (a *App) checkSelfInclusion(rctx request.CTX, policy *model.AccessControlPolicy, callerID string, mergedHidden bool) *model.AppError { for _, rule := range policy.Rules { if rule.Expression == "" || rule.Expression == "true" { continue @@ -510,9 +524,10 @@ func (a *App) checkSelfInclusion(rctx request.CTX, policy *model.AccessControlPo return appErr } if !matches { - return model.NewAppError("CreateOrUpdateAccessControlPolicy", - "app.pap.save_policy.self_exclusion", nil, - "You do not satisfy one or more conditions in this policy.", http.StatusForbidden) + if mergedHidden { + return saveForbiddenError(rctx, "checkSelfInclusion", "self_exclusion: you do not satisfy one or more conditions in this policy") + } + return model.NewAppError("checkSelfInclusion", "app.pap.save_policy.self_exclusion", nil, "", http.StatusForbidden) } } @@ -541,8 +556,7 @@ func (a *App) DeleteAccessControlPolicy(rctx request.CTX, id string) *model.AppE if hasMasked, appErr := a.policyHasMaskedValuesForCaller(rctx, policy, callerID); appErr != nil { return appErr } else if hasMasked { - return model.NewAppError("DeleteAccessControlPolicy", "app.pap.delete_policy.masked_values", nil, - "policy contains attribute values you do not hold; you cannot delete this policy", http.StatusForbidden) + return model.NewAppError("DeleteAccessControlPolicy", "app.pap.delete_policy.masked_values", nil, "", http.StatusForbidden) } } } @@ -646,11 +660,46 @@ func (a *App) SimulateAccessControlPolicyForUsers(rctx request.CTX, params model // save-only invariant. The merge alone is what makes the // simulator see the unmasked policy. if a.Config().FeatureFlags.AttributeValueMasking { - if appErr := a.mergeStoredPolicyExpressions(rctx, params.Policy, rctx.Session().UserId); appErr != nil { + // Resolve the caller once here so merge and response masking use the same + // identity. An empty callerID means there is no session; we must not let + // unmasked literals propagate to the response without knowing who is asking. + callerID := "" + if s := rctx.Session(); s != nil { + callerID = s.UserId + } + if callerID == "" { + return nil, model.NewAppError("SimulateAccessControlPolicyForUsers", "api.context.session_expired.app_error", nil, "session required for simulation when attribute value masking is enabled", http.StatusUnauthorized) + } + + if _, appErr := a.mergeStoredPolicyExpressions(rctx, params.Policy, callerID); appErr != nil { return nil, appErr } + + resp, appErr := acs.SimulatePolicyForUsers(rctx, params) + if appErr != nil { + return nil, appErr + } + + if resp != nil { + enrichBlameForDraftScope(rctx, acs, params.Policy, resp) + if isThisRuleScope(params.EvaluationScope) { + filterResponseToEditingRuleScope(resp, params.RuleName) + } + + // mergeStoredPolicyExpressions re-injected the stored hidden + // values so the simulator could evaluate the real CEL — and + // enrichBlameForDraftScope just copied those unmasked + // expressions into Blame.Expression / MergedRules / the + // evaluation tree. Re-mask every literal-bearing surface + // before the response leaves the server so the caller never + // sees a value they couldn't see via the policy GET path. + a.MaskSimulationPolicyLiteralsForCaller(rctx, resp, callerID) + } + + return resp, nil } + // Masking disabled: simulate without merge or response masking. resp, appErr := acs.SimulatePolicyForUsers(rctx, params) if appErr != nil { return nil, appErr @@ -661,20 +710,6 @@ func (a *App) SimulateAccessControlPolicyForUsers(rctx request.CTX, params model if isThisRuleScope(params.EvaluationScope) { filterResponseToEditingRuleScope(resp, params.RuleName) } - - // mergeStoredPolicyExpressions re-injected the stored hidden - // values so the simulator could evaluate the real CEL — and - // enrichBlameForDraftScope just copied those unmasked - // expressions into Blame.Expression / MergedRules / the - // evaluation tree. Re-mask every literal-bearing surface - // before the response leaves the server so the caller never - // sees a value they couldn't see via the policy GET path. - // Same flag gate as the merge above: either both run or - // neither does, so the response always matches the policy - // state that produced it. - if a.Config().FeatureFlags.AttributeValueMasking { - a.MaskSimulationPolicyLiteralsForCaller(rctx, resp, rctx.Session().UserId) - } } return resp, nil @@ -1657,13 +1692,47 @@ func (a *App) GetAccessControlFieldsAutocomplete(rctx request.CTX, after string, func (a *App) UpdateAccessControlPoliciesActive(rctx request.CTX, updates []model.AccessControlPolicyActiveUpdate) ([]*model.AccessControlPolicy, *model.AppError) { acs := a.Srv().ch.AccessControl if acs == nil { - return nil, model.NewAppError("ExpressionToVisualAST", "app.pap.update_access_control_policies_active.app_error", nil, "Policy Administration Point is not initialized", http.StatusNotImplemented) + return nil, model.NewAppError("UpdateAccessControlPoliciesActive", "app.pap.update_access_control_policies_active.app_error", nil, "Policy Administration Point is not initialized", http.StatusNotImplemented) + } + + // Deactivating a policy is enforcement-equivalent to deleting it: the policy stops + // filtering membership. Mirror the delete-path guard so a caller blocked from + // deleting a policy with hidden values cannot achieve the same effect via deactivation. + if a.Config().FeatureFlags.AttributeValueMasking { + session := rctx.Session() + if session != nil { + callerID := session.UserId + for _, u := range updates { + if u.Active { + continue // activation never widens access + } + policy, appErr := acs.GetPolicy(rctx, u.ID) + if appErr != nil { + return nil, appErr + } + if hasMasked, appErr := a.policyHasMaskedValuesForCaller(rctx, policy, callerID); appErr != nil { + return nil, appErr + } else if hasMasked { + return nil, model.NewAppError("UpdateAccessControlPoliciesActive", "app.pap.delete_policy.masked_values", nil, "", http.StatusForbidden) + } + } + } } policies, err := a.Srv().Store().AccessControlPolicy().SetActiveStatusMultiple(rctx, updates) if err != nil { return nil, model.NewAppError("UpdateAccessControlPoliciesActive", "app.pap.update_access_control_policies_active.app_error", nil, err.Error(), http.StatusInternalServerError) } + + for _, policy := range policies { + switch policy.Type { + case model.AccessControlPolicyTypeChannel: + a.publishChannelPolicyEnforcedUpdate(rctx, policy.ID) + case model.AccessControlPolicyTypeParent: + a.publishChannelPolicyEnforcedForChannelPoliciesWithImport(rctx, policy.ID) + } + } + return policies, nil } @@ -2116,15 +2185,37 @@ func (a *App) BuildAccessControlSubjectForSession(rctx request.CTX, channelID st return nil, appErr } - attrs, err := a.Srv().Store().SessionAttribute().Get(rctx.Session().Id) + attrs, appErr := a.GetSessionAttributes(rctx.Session().Id) + if appErr != nil { + return nil, appErr + } + if attrs != nil { + subject.Session = attrs + } + return subject, nil +} + +// GetSessionAttributes returns the request-provided session attributes +// (user_agent_*, ip_address — see model.SessionAttributesPropertyField*) +// captured for the given session, or nil when none have been recorded. +// +// The session-attribute cache is populated by +// RefreshRequestProvidedSessionAttributesIfNeeded on each authenticated +// request (gated by FeatureFlags.SessionAttributes and the Enterprise +// Advanced license). This getter is the shared entry point that both +// the production PDP (BuildAccessControlSubjectForSession) and the +// policy-simulator's active-session snapshot read from, so the snapshot +// the simulator shows the admin matches what the live PDP would +// evaluate against. +func (a *App) GetSessionAttributes(sessionID string) (map[string]any, *model.AppError) { + attrs, err := a.Srv().Store().SessionAttribute().Get(sessionID) if err != nil { if errors.Is(err, cache.ErrKeyNotFound) { - return subject, nil + return nil, nil } - return nil, model.NewAppError("BuildAccessControlSubjectForSession", "app.access_control.build_subject_for_session.get_session_attributes.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + return nil, model.NewAppError("GetSessionAttributes", "app.access_control.get_session_attributes.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } - subject.Session = attrs - return subject, nil + return attrs, nil } // GetSubjectChannelRole returns the channel-scoped role identifier diff --git a/server/channels/app/access_control_masking.go b/server/channels/app/access_control_masking.go index 50b057fc857e..273cd6da0285 100644 --- a/server/channels/app/access_control_masking.go +++ b/server/channels/app/access_control_masking.go @@ -624,7 +624,6 @@ func celValueLiteral(value any) string { const maskedTokenValue = "--------" // validatePolicyExpressionValues checks that all submitted literal values are held by the caller. -// Returns the same generic error for every rejection to prevent value enumeration. func (a *App) validatePolicyExpressionValues(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) *model.AppError { cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) if appErr != nil { @@ -775,9 +774,7 @@ func (a *App) GetMaskedExpression(rctx request.CTX, expression string, callerID // maskConditionValuesWithToken replaces non-held values with the masked token in place, // preserving expression structure so the visual AST endpoint can still parse it. // fieldsByName is pre-fetched by the caller to avoid N+1 lookups; a missing entry -// is treated as fail-closed (whole value masked). -// maskConditionValuesWithToken replaces non-held values with the masked token in place. -// Returns true if any value was masked. +// is treated as fail-closed (whole value masked). 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 false @@ -1215,8 +1212,15 @@ func clearEvaluationTreeLiterals(node *model.PolicySimulationEvaluationNode) { } } +// maskFailClosedSentinel is the CEL expression written into a response rule when masking +// cannot safely produce a redacted version (parse failure or CPA group unavailable). +// "false" is used because it is deny-all if ever evaluated literally, matching the +// fail-closed intent. This value only ever appears in API responses — the stored DB +// expression is never overwritten by this path. +const maskFailClosedSentinel = "false" + // MaskPolicyExpressions masks non-held literal values in all policy rule expressions, in place. -// Fails closed (sets a rule to "true") if its expression cannot be parsed or masked. +// Fails closed (sets a rule to maskFailClosedSentinel) if its expression cannot be parsed or masked. func (a *App) MaskPolicyExpressions(rctx request.CTX, policy *model.AccessControlPolicy, callerID string) { cpaGroup, appErr := a.GetPropertyGroup(rctx, model.AccessControlPropertyGroupName) if appErr != nil { @@ -1227,7 +1231,7 @@ func (a *App) MaskPolicyExpressions(rctx request.CTX, policy *model.AccessContro if rule.Expression == "" || rule.Expression == "true" { continue } - policy.Rules[i].Expression = "true" + policy.Rules[i].Expression = maskFailClosedSentinel } return } @@ -1245,7 +1249,7 @@ func (a *App) MaskPolicyExpressions(rctx request.CTX, policy *model.AccessContro } ast, appErr := a.ExpressionToVisualAST(rctx, rule.Expression) if appErr != nil { - policy.Rules[i].Expression = "true" // fail closed + policy.Rules[i].Expression = maskFailClosedSentinel // fail closed: deny-all sentinel, response-only continue } asts[i] = ast diff --git a/server/channels/app/access_control_masking_test.go b/server/channels/app/access_control_masking_test.go index fd7c26081623..c17f1fd8d0af 100644 --- a/server/channels/app/access_control_masking_test.go +++ b/server/channels/app/access_control_masking_test.go @@ -833,11 +833,9 @@ func TestMaskSimulationPolicyLiteralsForCaller_GuardClauses(t *testing.T) { }) t.Run("empty callerID is a no-op", func(t *testing.T) { - // A session-less caller reaching this code path would be a - // caller-context bug; the function must refuse rather than - // run with an empty caller (which the property service would - // resolve to "no holdings" and therefore mask everything, - // effectively a stealthy DoS). + // The API layer rejects empty callerID before reaching this function. + // Pinned to ensure MaskSimulationPolicyLiteralsForCaller is safe to call + // with an empty callerID (no panic, no masking applied). resp := &model.PolicySimulationResponse{ Results: []model.PolicySimulationUserResult{{ Decisions: map[string]model.PolicySimulationActionDecision{ diff --git a/server/channels/app/access_control_test.go b/server/channels/app/access_control_test.go index 94f8191cdcd3..6ea5c119ddee 100644 --- a/server/channels/app/access_control_test.go +++ b/server/channels/app/access_control_test.go @@ -428,12 +428,12 @@ func TestCheckSelfInclusion(t *testing.T) { mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). Return([]*model.User{{Id: callerID}}, int64(1), nil).Once() - appErr := th.App.checkSelfInclusion(th.Context, policy, callerID) + appErr := th.App.checkSelfInclusion(th.Context, policy, callerID, false) require.Nil(t, appErr) mockACS.AssertExpectations(t) }) - t.Run("caller who does not satisfy the policy is rejected with 403", func(t *testing.T) { + t.Run("caller who does not satisfy the policy is rejected with specific self_exclusion error when no hidden values were merged", func(t *testing.T) { th := Setup(t).InitBasic(t) callerID := th.BasicUser.Id @@ -445,17 +445,40 @@ func TestCheckSelfInclusion(t *testing.T) { mockACS := &mocks.AccessControlServiceInterface{} th.App.Srv().ch.AccessControl = mockACS - // No users returned → caller does not satisfy → expect self_exclusion 403. + // No users returned → caller does not satisfy → expect specific self_exclusion error. mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). Return([]*model.User{}, int64(0), nil).Once() - appErr := th.App.checkSelfInclusion(th.Context, policy, callerID) + appErr := th.App.checkSelfInclusion(th.Context, policy, callerID, false) require.NotNil(t, appErr) require.Equal(t, http.StatusForbidden, appErr.StatusCode) require.Equal(t, "app.pap.save_policy.self_exclusion", appErr.Id) mockACS.AssertExpectations(t) }) + t.Run("caller who does not satisfy the policy gets opaque 403 when hidden values were merged", func(t *testing.T) { + th := Setup(t).InitBasic(t) + callerID := th.BasicUser.Id + + policy := &model.AccessControlPolicy{ + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.clearance == "TopSecret"`}, + }, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + // No users returned → caller does not satisfy → mergedHidden=true returns generic forbidden. + mockACS.On("QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything). + Return([]*model.User{}, int64(0), nil).Once() + + appErr := th.App.checkSelfInclusion(th.Context, policy, callerID, true) + require.NotNil(t, appErr) + require.Equal(t, http.StatusForbidden, appErr.StatusCode) + require.Equal(t, "app.pap.save_policy.forbidden", appErr.Id) + mockACS.AssertExpectations(t) + }) + t.Run("trivial rules (empty / 'true') are skipped without querying", func(t *testing.T) { th := Setup(t).InitBasic(t) callerID := th.BasicUser.Id @@ -472,7 +495,7 @@ func TestCheckSelfInclusion(t *testing.T) { // No query should fire for trivial expressions — if it does, the mock will fail // the test by returning the default zero-value response. - appErr := th.App.checkSelfInclusion(th.Context, policy, callerID) + appErr := th.App.checkSelfInclusion(th.Context, policy, callerID, false) require.Nil(t, appErr) mockACS.AssertNotCalled(t, "QueryUsersForExpression", mock.Anything, mock.Anything, mock.Anything) }) @@ -4475,7 +4498,7 @@ func TestMergeStoredPolicyExpressions_ActionsLocked(t *testing.T) { }, }, nil).Maybe() - mergeErr := th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID) + _, mergeErr := th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID) require.Nil(t, mergeErr) require.Len(t, submittedPolicy.Rules, 1) @@ -4486,31 +4509,17 @@ func TestMergeStoredPolicyExpressions_ActionsLocked(t *testing.T) { 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) { +// TestMergeStoredPolicyExpressions_FailClosedSentinelRejectedOnResubmit verifies that +// re-submitting the fail-closed sentinel as the rule expression is rejected on save +// when the stored expression is unparseable. +func TestMergeStoredPolicyExpressions_FailClosedSentinelRejectedOnResubmit(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"` + storedExpr := `user.attributes.TopSecret == "Value"` // ExpressionToVisualAST cannot parse this storedPolicy := &model.AccessControlPolicy{ ID: policyID, @@ -4520,13 +4529,11 @@ func TestMergeStoredPolicyExpressions_FailClosedTrueRejectedOnResubmit(t *testin }, } - // 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"}, + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: maskFailClosedSentinel}, }, } @@ -4534,16 +4541,12 @@ func TestMergeStoredPolicyExpressions_FailClosedTrueRejectedOnResubmit(t *testin 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) + _, 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") + require.NotNil(t, mergeErr) mockACS.AssertExpectations(t) } @@ -4602,7 +4605,7 @@ func TestMergeStoredPolicyExpressions_ActionsEditableWhenNoMasking(t *testing.T) }, }, nil).Maybe() - appErr = th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID) + _, appErr = th.App.mergeStoredPolicyExpressions(th.Context, submittedPolicy, callerID) require.Nil(t, appErr) require.Len(t, submittedPolicy.Rules, 1) @@ -4612,3 +4615,222 @@ func TestMergeStoredPolicyExpressions_ActionsEditableWhenNoMasking(t *testing.T) assert.Equal(t, []string{model.AccessControlPolicyActionUploadFileAttachment}, submittedPolicy.Rules[0].Actions) mockACS.AssertExpectations(t) } + +// TestUpdateAccessControlPoliciesActive_MaskingGuard verifies deactivation follows the +// same masking rules as delete, while activation is always allowed. +func TestUpdateAccessControlPoliciesActive_MaskingGuard(t *testing.T) { + t.Run("deactivation blocked when caller has masked values", func(t *testing.T) { + // policyHasMaskedValuesForCaller resolves the property group from the store, + // so this subtest uses SetupConfig + InitBasic rather than a mock store. + th := SetupConfig(t, func(cfg *model.Config) { + cfg.FeatureFlags.AttributeBasedAccessControl = true + cfg.FeatureFlags.AttributeValueMasking = true + }).InitBasic(t) + + callerID := model.NewId() + th.Context = th.Context.WithSession(&model.Session{UserId: callerID, Id: model.NewId()}).(*request.Context) + + policyID := model.NewId() + sensitivePolicy := &model.AccessControlPolicy{ + ID: policyID, + Type: model.AccessControlPolicyTypeChannel, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.f_unknown == "Secret"`}, + }, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + mockACS.On("GetPolicy", th.Context, policyID).Return(sensitivePolicy, nil).Once() + // Unknown field → fail-closed → HasMaskedValues=true. + mockACS.On("ExpressionToVisualAST", mock.Anything, mock.Anything).Return(&model.VisualExpression{ + Conditions: []model.Condition{ + {Attribute: "user.attributes.f_unknown", Operator: "==", Value: "Secret", ValueType: model.LiteralValue}, + }, + }, nil).Maybe() + + _, appErr := th.App.UpdateAccessControlPoliciesActive(th.Context, []model.AccessControlPolicyActiveUpdate{ + {ID: policyID, Active: false}, + }) + + require.NotNil(t, appErr) + require.Equal(t, http.StatusForbidden, appErr.StatusCode) + require.Equal(t, "app.pap.delete_policy.masked_values", appErr.Id) + mockACS.AssertExpectations(t) + }) + + t.Run("activation always allowed even when caller has masked values", func(t *testing.T) { + // The guard skips Active=true updates, so no property store access is needed. + thMock := SetupWithStoreMock(t) + thMock.App.UpdateConfig(func(cfg *model.Config) { + cfg.FeatureFlags.AttributeBasedAccessControl = true + cfg.FeatureFlags.AttributeValueMasking = true + }) + + callerID := model.NewId() + thMock.Context = thMock.Context.WithSession(&model.Session{UserId: callerID, Id: model.NewId()}).(*request.Context) + + channelID := model.NewId() + policy := &model.AccessControlPolicy{ + ID: channelID, + Type: model.AccessControlPolicyTypeChannel, + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.f_unknown == "Secret"`}, + }, + } + + mockStore := thMock.App.Srv().Store().(*storemocks.Store) + mockACPStore := storemocks.AccessControlPolicyStore{} + mockStore.On("AccessControlPolicy").Return(&mockACPStore) + mockACPStore.On("SetActiveStatusMultiple", thMock.Context, mock.Anything).Return([]*model.AccessControlPolicy{policy}, nil).Once() + + // Channel cache & WS broadcast + mockChannelStore := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannelStore).Maybe() + mockChannelStore.On("InvalidateChannel", channelID).Maybe() + mockChannelStore.On("Get", channelID, true).Return(&model.Channel{Id: channelID, Type: model.ChannelTypePrivate}, nil).Maybe() + + mockACS := &mocks.AccessControlServiceInterface{} + thMock.App.Srv().ch.AccessControl = mockACS + + _, appErr := thMock.App.UpdateAccessControlPoliciesActive(thMock.Context, []model.AccessControlPolicyActiveUpdate{ + {ID: channelID, Active: true}, + }) + + require.Nil(t, appErr) + mockACPStore.AssertExpectations(t) + mockACS.AssertNotCalled(t, "GetPolicy", mock.Anything, mock.Anything) + }) + + t.Run("deactivation allowed when masking flag is off", func(t *testing.T) { + thMock := SetupWithStoreMock(t) + thMock.Context = thMock.Context.WithSession(&model.Session{UserId: model.NewId(), Id: model.NewId()}).(*request.Context) + + channelID := model.NewId() + policy := &model.AccessControlPolicy{ + ID: channelID, + Type: model.AccessControlPolicyTypeChannel, + } + + mockStore := thMock.App.Srv().Store().(*storemocks.Store) + mockACPStore := storemocks.AccessControlPolicyStore{} + mockStore.On("AccessControlPolicy").Return(&mockACPStore) + mockACPStore.On("SetActiveStatusMultiple", thMock.Context, mock.Anything).Return([]*model.AccessControlPolicy{policy}, nil).Once() + + mockChannelStore := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannelStore).Maybe() + mockChannelStore.On("InvalidateChannel", channelID).Maybe() + mockChannelStore.On("Get", channelID, true).Return(&model.Channel{Id: channelID, Type: model.ChannelTypePrivate}, nil).Maybe() + + mockACS := &mocks.AccessControlServiceInterface{} + thMock.App.Srv().ch.AccessControl = mockACS + + _, appErr := thMock.App.UpdateAccessControlPoliciesActive(thMock.Context, []model.AccessControlPolicyActiveUpdate{ + {ID: channelID, Active: false}, + }) + + require.Nil(t, appErr) + // GetPolicy must never be called when masking is off. + mockACS.AssertNotCalled(t, "GetPolicy", mock.Anything, mock.Anything) + mockACPStore.AssertExpectations(t) + }) +} + +// TestUpdateAccessControlPoliciesActive_BroadcastsWebsocketEvents verifies that +// activate/deactivate fires cache invalidation and WS events for channel and parent policies. +func TestUpdateAccessControlPoliciesActive_BroadcastsWebsocketEvents(t *testing.T) { + t.Run("channel policy triggers publishChannelPolicyEnforcedUpdate", func(t *testing.T) { + thMock := SetupWithStoreMock(t) + thMock.Context = thMock.Context.WithSession(&model.Session{UserId: model.NewId(), Id: model.NewId()}).(*request.Context) + + channelID := model.NewId() + policy := &model.AccessControlPolicy{ + ID: channelID, + Type: model.AccessControlPolicyTypeChannel, + } + + mockStore := thMock.App.Srv().Store().(*storemocks.Store) + mockACPStore := storemocks.AccessControlPolicyStore{} + mockStore.On("AccessControlPolicy").Return(&mockACPStore) + mockACPStore.On("SetActiveStatusMultiple", thMock.Context, mock.Anything).Return([]*model.AccessControlPolicy{policy}, nil).Once() + + mockChannelStore := storemocks.ChannelStore{} + mockStore.On("Channel").Return(&mockChannelStore) + mockChannelStore.On("InvalidateChannel", channelID).Once() + mockChannelStore.On("Get", channelID, true).Return(&model.Channel{Id: channelID, Type: model.ChannelTypePrivate}, nil).Once() + + mockACS := &mocks.AccessControlServiceInterface{} + thMock.App.Srv().ch.AccessControl = mockACS + + _, appErr := thMock.App.UpdateAccessControlPoliciesActive(thMock.Context, []model.AccessControlPolicyActiveUpdate{ + {ID: channelID, Active: true}, + }) + + require.Nil(t, appErr) + mockChannelStore.AssertExpectations(t) + }) +} + +// TestSaveForbiddenErrorHidesInternalID verifies that saveForbiddenError always returns +// app.pap.save_policy.forbidden with an empty DetailedError, regardless of the internal reason. +func TestSaveForbiddenErrorHidesInternalID(t *testing.T) { + rctx := request.TestContext(t) + + internalReasons := []string{ + "masked_condition_deleted: detail", + "masked_rule_deleted: detail", + "self_exclusion: detail", + "advanced_expression_blocked: detail", + } + + for _, reason := range internalReasons { + appErr := saveForbiddenError(rctx, "testWhere", reason) + require.NotNil(t, appErr) + assert.Equal(t, "app.pap.save_policy.forbidden", appErr.Id, "reason %q must not appear in error ID", reason) + assert.Equal(t, http.StatusForbidden, appErr.StatusCode) + assert.Empty(t, appErr.DetailedError) + } +} + +// TestMaskPolicyExpressions_FailClosedUsesDenyAllSentinel verifies that MaskPolicyExpressions +// emits maskFailClosedSentinel ("false") on parse failure, not the open-access "true". +func TestMaskPolicyExpressions_FailClosedUsesDenyAllSentinel(t *testing.T) { + th := SetupWithStoreMock(t) + callerID := model.NewId() + + t.Run("CPA group lookup failure masks all rules to deny-all sentinel", func(t *testing.T) { + policy := &model.AccessControlPolicy{ + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: `user.attributes.secret == "X"`}, + }, + } + + // Inject a mock ACS that makes ExpressionToVisualAST fail, triggering fail-closed. + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + mockACS.On("ExpressionToVisualAST", mock.Anything, mock.Anything).Return(nil, + model.NewAppError("ExpressionToVisualAST", "app_error", nil, "parse fail", http.StatusInternalServerError)).Maybe() + + th.App.MaskPolicyExpressions(th.Context, policy, callerID) + + require.Len(t, policy.Rules, 1) + assert.Equal(t, maskFailClosedSentinel, policy.Rules[0].Expression) + }) + + t.Run("trivial true and empty rules are untouched by fail-closed path", func(t *testing.T) { + policy := &model.AccessControlPolicy{ + Rules: []model.AccessControlPolicyRule{ + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: ""}, + {Actions: []string{model.AccessControlPolicyActionMembership}, Expression: "true"}, + }, + } + + mockACS := &mocks.AccessControlServiceInterface{} + th.App.Srv().ch.AccessControl = mockACS + + th.App.MaskPolicyExpressions(th.Context, policy, callerID) + + assert.Equal(t, "", policy.Rules[0].Expression, "empty expression must stay empty") + assert.Equal(t, "true", policy.Rules[1].Expression, "open-access rule must stay \"true\"") + }) +} diff --git a/server/channels/app/properties/access_control_value_test.go b/server/channels/app/properties/access_control_value_test.go index aed0746c55f3..b966640a72b5 100644 --- a/server/channels/app/properties/access_control_value_test.go +++ b/server/channels/app/properties/access_control_value_test.go @@ -1627,8 +1627,11 @@ func TestUpdatePropertyValues_WriteAccessControl(t *testing.T) { // Verify values were NOT updated retrieved, err := th.service.GetPropertyValues(rctx1, th.CPAGroupID, []string{createdValues[0].ID, createdValues[1].ID}) require.NoError(t, err) - assert.Equal(t, `"value1"`, string(retrieved[0].Value)) - assert.Equal(t, `"value2"`, string(retrieved[1].Value)) + require.Len(t, retrieved, 2) + retrievedValue1 := findValueByID(t, retrieved, createdValues[0].ID) + assert.Equal(t, `"value1"`, string(retrievedValue1.Value)) + retrievedValue2 := findValueByID(t, retrieved, createdValues[1].ID) + assert.Equal(t, `"value2"`, string(retrievedValue2.Value)) }) t.Run("mixed protected and non-protected fields - enforces access control only on protected fields", func(t *testing.T) { @@ -1687,8 +1690,11 @@ func TestUpdatePropertyValues_WriteAccessControl(t *testing.T) { // Verify NO values were updated (atomic failure) retrieved, err := th.service.GetPropertyValues(rctx1, th.CPAGroupID, []string{createdValues[0].ID, createdValues[1].ID}) require.NoError(t, err) - assert.Equal(t, `"protected value"`, string(retrieved[0].Value)) - assert.Equal(t, `"public value"`, string(retrieved[1].Value)) + require.Len(t, retrieved, 2) + retrievedProtectedValue := findValueByID(t, retrieved, createdValues[0].ID) + assert.Equal(t, `"protected value"`, string(retrievedProtectedValue.Value)) + retrievedPublicValue := findValueByID(t, retrieved, createdValues[1].ID) + assert.Equal(t, `"public value"`, string(retrievedPublicValue.Value)) // Now try with source plugin - should succeed for both createdValues[0].Value = json.RawMessage(`"updated protected"`) @@ -1761,8 +1767,11 @@ func TestUpdatePropertyValues_WriteAccessControl(t *testing.T) { // Verify NO values were updated (atomic failure) retrieved, err := th.service.GetPropertyValues(rctx1, th.CPAGroupID, []string{createdValue1.ID, createdValue2.ID}) require.NoError(t, err) - assert.Equal(t, `"value from plugin1"`, string(retrieved[0].Value)) - assert.Equal(t, `"value from plugin2"`, string(retrieved[1].Value)) + require.Len(t, retrieved, 2) + retrievedValue1 := findValueByID(t, retrieved, createdValue1.ID) + assert.Equal(t, `"value from plugin1"`, string(retrievedValue1.Value)) + retrievedValue2 := findValueByID(t, retrieved, createdValue2.ID) + assert.Equal(t, `"value from plugin2"`, string(retrievedValue2.Value)) // Try to update both values with plugin2 - should also fail because it doesn't own field1 createdValue1.Value = json.RawMessage(`"hacked by plugin2"`) @@ -1775,8 +1784,11 @@ func TestUpdatePropertyValues_WriteAccessControl(t *testing.T) { // Verify still NO values were updated retrieved, err = th.service.GetPropertyValues(rctx1, th.CPAGroupID, []string{createdValue1.ID, createdValue2.ID}) require.NoError(t, err) - assert.Equal(t, `"value from plugin1"`, string(retrieved[0].Value)) - assert.Equal(t, `"value from plugin2"`, string(retrieved[1].Value)) + require.Len(t, retrieved, 2) + retrievedValue1 = findValueByID(t, retrieved, createdValue1.ID) + assert.Equal(t, `"value from plugin1"`, string(retrievedValue1.Value)) + retrievedValue2 = findValueByID(t, retrieved, createdValue2.ID) + assert.Equal(t, `"value from plugin2"`, string(retrievedValue2.Value)) // Each plugin can update its own value individually createdValue1.Value = json.RawMessage(`"plugin-1 updated its own"`) @@ -1791,6 +1803,18 @@ func TestUpdatePropertyValues_WriteAccessControl(t *testing.T) { }) } +func findValueByID(t *testing.T, values []*model.PropertyValue, id string) *model.PropertyValue { + t.Helper() + for _, value := range values { + if value.ID == id { + return value + } + } + + require.FailNow(t, "missing property value", "missing property value %s", id) + return nil +} + func TestUpsertPropertyValue_WriteAccessControl(t *testing.T) { th := Setup(t).RegisterCPAPropertyGroup(t) th.service.setPluginCheckerForTests(func(pluginID string) bool { return pluginID == "plugin-1" }) diff --git a/server/channels/store/sqlstore/integrity_test.go b/server/channels/store/sqlstore/integrity_test.go index 3760d6b715e6..9734f9fba23a 100644 --- a/server/channels/store/sqlstore/integrity_test.go +++ b/server/channels/store/sqlstore/integrity_test.go @@ -792,8 +792,29 @@ func TestCheckSessionsAuditsIntegrity(t *testing.T) { }) } +func orphanedRecordsWithChildIDs(records []model.OrphanedRecord, childIDs ...string) []model.OrphanedRecord { + childIDSet := make(map[string]struct{}, len(childIDs)) + for _, childID := range childIDs { + childIDSet[childID] = struct{}{} + } + + filtered := make([]model.OrphanedRecord, 0, len(childIDs)) + for _, record := range records { + if record.ChildId == nil { + continue + } + if _, ok := childIDSet[*record.ChildId]; ok { + filtered = append(filtered, record) + } + } + + return filtered +} + func TestCheckTeamsChannelsIntegrity(t *testing.T) { StoreTest(t, func(t *testing.T, rctx request.CTX, ss store.Store) { + ss.DropAllTables() + store := ss.(*SqlStore) dbmap := store.GetMaster() @@ -809,11 +830,12 @@ func TestCheckTeamsChannelsIntegrity(t *testing.T) { result := checkTeamsChannelsIntegrity(store) require.NoError(t, result.Err) data := result.Data.(model.RelationalIntegrityCheckData) - require.Len(t, data.Records, 1) + records := orphanedRecordsWithChildIDs(data.Records, channel.Id) + require.Len(t, records, 1) require.Equal(t, model.OrphanedRecord{ ParentId: &channel.TeamId, ChildId: &channel.Id, - }, data.Records[0]) + }, records[0]) dbmap.Exec(`DELETE FROM Channels WHERE Id=?`, channel.Id) }) @@ -827,11 +849,12 @@ func TestCheckTeamsChannelsIntegrity(t *testing.T) { result := checkTeamsChannelsIntegrity(store) require.NoError(t, result.Err) data := result.Data.(model.RelationalIntegrityCheckData) - require.Len(t, data.Records, 1) + records := orphanedRecordsWithChildIDs(data.Records, channel.Id, direct.Id) + require.Len(t, records, 1) require.Equal(t, model.OrphanedRecord{ ParentId: &channel.TeamId, ChildId: &channel.Id, - }, data.Records[0]) + }, records[0]) dbmap.Exec(`DELETE FROM Channels WHERE Id=?`, channel.Id) dbmap.Exec(`DELETE FROM Users WHERE Id=?`, userA.Id) dbmap.Exec(`DELETE FROM Users WHERE Id=?`, userB.Id) @@ -850,15 +873,17 @@ func TestCheckTeamsChannelsIntegrity(t *testing.T) { result := checkTeamsChannelsIntegrity(store) require.NoError(t, result.Err) data := result.Data.(model.RelationalIntegrityCheckData) - require.Len(t, data.Records, 2) - require.Equal(t, model.OrphanedRecord{ - ParentId: &channel.TeamId, - ChildId: &channel.Id, - }, data.Records[0]) - require.Equal(t, model.OrphanedRecord{ - ParentId: new("test"), - ChildId: &direct.Id, - }, data.Records[1]) + records := orphanedRecordsWithChildIDs(data.Records, channel.Id, direct.Id) + require.ElementsMatch(t, []model.OrphanedRecord{ + { + ParentId: &channel.TeamId, + ChildId: &channel.Id, + }, + { + ParentId: new("test"), + ChildId: &direct.Id, + }, + }, records) dbmap.Exec(`DELETE FROM Channels WHERE Id=?`, channel.Id) dbmap.Exec(`DELETE FROM Users WHERE Id=?`, userA.Id) dbmap.Exec(`DELETE FROM Users WHERE Id=?`, userB.Id) diff --git a/server/i18n/en.json b/server/i18n/en.json index c6f5cd38140f..9f99e34e7f8c 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -5258,14 +5258,14 @@ "id": "app.access_control.build_subject.group_id.app_error", "translation": "Failed to retrieve the access control attribute group." }, - { - "id": "app.access_control.build_subject_for_session.get_session_attributes.app_error", - "translation": "Failed to retrieve the session attributes." - }, { "id": "app.access_control.get_channel_role.app_error", "translation": "Unable to get channel role for the user. Please try again." }, + { + "id": "app.access_control.get_session_attributes.app_error", + "translation": "Failed to retrieve the session attributes." + }, { "id": "app.access_control.insufficient_permissions", "translation": "You do not have permission to manage this access control policy." @@ -7700,25 +7700,17 @@ "id": "app.pap.query_expression.app_error", "translation": "Could not query for expression." }, - { - "id": "app.pap.save_policy.advanced_expression_blocked", - "translation": "This rule expression cannot be safely edited while restricted values are present." - }, { "id": "app.pap.save_policy.app_error", "translation": "Unable to save access control policy." }, { - "id": "app.pap.save_policy.invalid_value", - "translation": "Invalid value." - }, - { - "id": "app.pap.save_policy.masked_condition_deleted", - "translation": "You cannot remove a condition that contains attribute values you do not have permission to view." + "id": "app.pap.save_policy.forbidden", + "translation": "You do not have permission to make this change to the policy." }, { - "id": "app.pap.save_policy.masked_rule_deleted", - "translation": "You cannot remove a rule that contains attribute values you do not have permission to view." + "id": "app.pap.save_policy.invalid_value", + "translation": "Invalid value." }, { "id": "app.pap.save_policy.name_exists.app_error", @@ -7732,10 +7724,6 @@ "id": "app.pap.save_policy.self_exclusion", "translation": "You do not satisfy one or more conditions in this policy." }, - { - "id": "app.pap.save_policy.user_session_unsupported.app_error", - "translation": "Session attributes are not supported for policy simulation." - }, { "id": "app.pap.search_access_control_policies.app_error", "translation": "Could not search access control policies." diff --git a/server/public/model/access_request.go b/server/public/model/access_request.go index 0ec29a754703..1abf5d493a75 100644 --- a/server/public/model/access_request.go +++ b/server/public/model/access_request.go @@ -499,16 +499,18 @@ type PolicySimulationResponse struct { // PolicySimulationUserOverride captures the per-user inputs the picker UI // sends to /access_control_policies/cel/simulate_users. The simulator // resolves each user's profile attributes from CPA storage and then layers -// session context on top: first the active-session snapshot (when -// UseActiveSession is set), then the explicit SessionOverrides map. +// session context on top: the requesting admin's resolved session +// attributes (the same user_agent_* / ip_address bag the live PDP reads +// via App.GetSessionAttributes) are applied as a baseline, then the +// explicit SessionOverrides map overrides individual keys. type PolicySimulationUserOverride struct { // UserID identifies the user to simulate against. UserID string `json:"user_id"` - // UseActiveSession injects the requesting admin's session.* attributes - // (network_status, client_type, device_managed, ip_range, platform, - // device_id) into this user's evaluation context. When the live PDP - // does not yet populate session.* on the request context this is a - // no-op; the API surface is forward-compatible. + // UseActiveSession is retained for API backward compatibility. The + // simulator now always layers the requesting admin's resolved session + // snapshot under SessionOverrides — leaving overrides empty means + // "evaluate against the session as the server resolves it" — so this + // flag is effectively a no-op. New clients should leave it unset. UseActiveSession bool `json:"use_active_session,omitempty"` // SessionOverrides replaces individual session.* attributes for this // user only. Applied on top of the active-session snapshot when both diff --git a/server/public/pluginapi/cluster/job_once_test.go b/server/public/pluginapi/cluster/job_once_test.go index 78c3ea3c7a31..e6aa40a8d7de 100644 --- a/server/public/pluginapi/cluster/job_once_test.go +++ b/server/public/pluginapi/cluster/job_once_test.go @@ -300,9 +300,13 @@ func TestScheduleOnceSequential(t *testing.T) { _, err = s.ScheduleOnce("anything", time.Now().Add(50*time.Millisecond), nil) require.NoError(t, err) - time.Sleep(70*time.Millisecond + scheduleOnceJitter) - assert.Equal(t, int32(0), atomic.LoadInt32(newCount2)) - assert.Equal(t, int32(1), atomic.LoadInt32(newCount3)) + + // Poll for the scheduled callback. A fixed sleep is flaky under the race + // detector and on loaded CI because job.run adds up to scheduleOnceJitter + // on top of the scheduled delay. + require.Eventually(t, func() bool { + return atomic.LoadInt32(newCount2) == int32(0) && atomic.LoadInt32(newCount3) == int32(1) + }, 5*time.Second, 50*time.Millisecond, "timed out waiting for scheduled callback") }) t.Run("test paging keys from the db by inserting 3 pages of jobs and starting scheduler", func(t *testing.T) { diff --git a/webapp/channels/src/components/admin_console/access_control/editors/shared.tsx b/webapp/channels/src/components/admin_console/access_control/editors/shared.tsx index c8399bb55fe3..061a7f8cf894 100644 --- a/webapp/channels/src/components/admin_console/access_control/editors/shared.tsx +++ b/webapp/channels/src/components/admin_console/access_control/editors/shared.tsx @@ -12,6 +12,9 @@ import Markdown from 'components/markdown'; import './shared.scss'; +// Sentinel emitted by the server in masked CEL expressions for values the caller cannot see. +export const MASKED_VALUE_TOKEN_LITERAL = '"--------"'; + // CEL operator constants export enum CELOperator { EQUALS = '==', 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 47778585de33..966a63ca70e8 100644 --- a/webapp/channels/src/components/admin_console/access_control/policies.tsx +++ b/webapp/channels/src/components/admin_console/access_control/policies.tsx @@ -17,14 +17,9 @@ import SectionNotice from 'components/section_notice'; import {getHistory} from 'utils/browser_history'; -import './policies.scss'; +import {MASKED_VALUE_TOKEN_LITERAL} from './editors/shared'; -// 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 = '"--------"'; +import './policies.scss'; function policyHasMaskedValues(policy: AccessControlPolicy): boolean { return policy.rules?.some((rule) => rule.expression?.includes(MASKED_VALUE_TOKEN_LITERAL)) ?? false; 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 8dc765642a2a..0c88254d576a 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 @@ -32,7 +32,7 @@ import Constants from 'utils/constants'; import ChannelList from './channel_list'; import CELEditor from '../editors/cel_editor/editor'; -import {hasUsableAttributes, isSimpleExpression} from '../editors/shared'; +import {hasUsableAttributes, isSimpleExpression, MASKED_VALUE_TOKEN_LITERAL} from '../editors/shared'; import TableEditor from '../editors/table_editor/table_editor'; import PolicyConfirmationModal from '../modals/confirmation/confirmation_modal'; @@ -82,15 +82,15 @@ function PolicyDetails({ 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]); + // Check for masked values using existingRules when available (updated after each fetch), + // falling back to the prop. The "--------" sentinel may be absent from a locally rebuilt + // CEL string even when masked values are present, so we rely on the server-sourced rules. + const hasMaskedRows = useMemo( + () => (existingRules.length > 0 ? existingRules : policy?.rules ?? []).some( + (rule) => rule.expression?.includes(MASKED_VALUE_TOKEN_LITERAL), + ), + [existingRules, policy], + ); const [channelChanges, setChannelChanges] = useState({ removed: {}, added: {}, @@ -211,10 +211,8 @@ function PolicyDetails({ 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 if (result.error.server_error_id === 'app.pap.save_policy.forbidden') { + setServerError(formatMessage({id: 'admin.access_control.edit_policy.forbidden', defaultMessage: 'You do not have permission to make this change to the policy. Contact a System Admin for assistance.'})); } else { setServerError(result.error.message); } diff --git a/webapp/channels/src/i18n/en.json b/webapp/channels/src/i18n/en.json index edf8b548aa69..c2e087f8f422 100644 --- a/webapp/channels/src/i18n/en.json +++ b/webapp/channels/src/i18n/en.json @@ -278,9 +278,8 @@ "admin.access_control.edit": "Edit", "admin.access_control.edit_policy.apply_policy": "Apply policy", "admin.access_control.edit_policy.cancel": "Cancel", + "admin.access_control.edit_policy.forbidden": "You do not have permission to make this change to the policy. Contact a System Admin for assistance.", "admin.access_control.edit_policy.invalid_value": "Invalid value.", - "admin.access_control.edit_policy.masked_condition_deleted": "You cannot remove a condition that contains attribute values you do not have permission to view.", - "admin.access_control.edit_policy.masked_rule_deleted": "You cannot remove a rule that contains attribute values you do not have permission to view.", "admin.access_control.edit_policy.name_exists": "A policy with this name already exists. Please choose a different name.", "admin.access_control.edit_policy.save": "Save", "admin.access_control.edit_policy.save_policy": "Save policy", diff --git a/webapp/platform/types/src/access_control.ts b/webapp/platform/types/src/access_control.ts index 6a9a8f334de7..82a5f30214bd 100644 --- a/webapp/platform/types/src/access_control.ts +++ b/webapp/platform/types/src/access_control.ts @@ -508,14 +508,22 @@ export type PolicyEvaluationScope = 'all' | 'this_rule'; /** * Per-user payload for the picker-driven /cel/simulate_users endpoint. The * server resolves the user's profile attributes from CPA storage and then - * layers session context on top: first the requesting admin's active-session - * snapshot (when use_active_session is true), then the explicit - * session_overrides map. Either source can be empty; both nil/empty means - * "no session context" (strict default — rules referencing session.* will - * surface as denies). + * layers session context on top: the requesting admin's resolved session + * snapshot (the same user_agent_* / ip_address bag the live PDP evaluates + * against) is applied as a baseline, then `session_overrides` overrides + * individual keys. Leaving overrides empty means "evaluate against the + * session as the server resolves it" — matching what the user would hit + * on a live request. */ export type PolicySimulationUserOverride = { user_id: string; + + /** + * Retained for API backward compatibility — the server now always + * layers the requesting admin's resolved session snapshot under + * `session_overrides`, so this flag is effectively a no-op. New + * clients should leave it unset. + */ use_active_session?: boolean; /**