ENG-3084: Add Go PBAC policy engine library and fides pbac CLI#7926
ENG-3084: Add Go PBAC policy engine library and fides pbac CLI#7926thabofletcher wants to merge 21 commits intomainfrom
Conversation
Go library implementing the PBAC purpose-overlap evaluation algorithm and fideslang policy rule evaluation. Provides two packages: - pkg/pbac: Purpose evaluation (set intersection of consumer vs dataset purposes) - pkg/fideslang: Fideslang policy rule evaluation (taxonomy hierarchy matching) - cmd/fides-evaluate: CLI binary for stdin/file-based evaluation The library is designed to be imported by the fidesplus sidecar for high-throughput HTTP evaluation (~13x faster than Python).
The fideslang package implemented the old policy rule evaluation (taxonomy hierarchy matching against privacy declarations). This is replaced by the Access Policy v2 system which will be added to pkg/pbac/ as the next step in the PBAC pipeline.
Implements the policy evaluation algorithm from IMPLEMENTATION_GUIDE.md as step 7 of the PBAC pipeline — filtering violations through access policies created via the UI. Features: - Priority-ordered, first-decisive-match-wins evaluation - Match blocks with any/all operators and taxonomy hierarchy matching - Unless conditions: consent, geo_location, data_flow (AND logic) - ALLOW + unless triggered = DENY (decisive) - DENY + unless triggered = SUPPRESSED (continues to next policy) - Audit trail of evaluated policies 25 tests covering priority ordering, match modes, all three unless condition types, multi-constraint AND logic, and taxonomy matching.
Library tests (pkg/pbac/): - edge_cases_test.go: empty datasets, nil collections, multiple collections counted separately, duplicate purpose keys, sorted output, non-nil slices for JSON, EffectivePurposes inheritance, policy catch-all, match all/any combined, unless without context, deny action only on deny, context field resolution (nested, missing, non-string) CLI integration tests (cmd/fides-evaluate/): - Builds the binary once, exercises both purpose and policies commands via stdin and file input - Purpose: compliant, violation, gap, collections, multiple datasets - Policies: allow, deny, no-decision, unless-inverts, priority ordering - Error handling: no args, unknown command, invalid JSON, missing file, empty input, output is valid JSON Also adds 'policies' subcommand to the CLI. 63 tests total, all passing.
Drops the standalone Go binary (cmd/fides-evaluate/) and adds PBAC evaluation to the existing fides CLI: fides pbac evaluate-purpose — purpose overlap check (stdin/file) fides pbac evaluate-policies — access policy evaluation (stdin/file) The CLI calls the Python evaluation engine directly. Performance is not the concern here — the Go sidecar handles API throughput. The CLI is for local testing and debugging. Adds: - src/fides/cli/commands/pbac.py — Click command group - src/fides/service/pbac/policies/evaluate.py — Python policy v2 engine (mirrors Go implementation in policy-engine/pkg/pbac/) - tests for both the evaluation engine and CLI passthrough
Go library (50 tests) — adds: - data_subject match dimension - three-dimension match (use + category + subject) - consent not_opt_in / not_opt_out requirements - data_flow none_of operator Python evaluation (42 tests) — adds: - data_subject match + three-dimension match - match any+all combined across dimensions - consent not_opt_in / not_opt_out - geo not_in operator - data_flow none_of operator - nested context field resolution 92 automated tests total across Go and Python, covering every rule, match mode, unless condition type, and operator.
Runs go build, go vet, and go test on pull_request/push when policy-engine/ files change. Uses setup-go with module caching.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
thabofletcher
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7926 +/- ##
========================================
Coverage 85.03% 85.03%
========================================
Files 629 631 +2
Lines 40971 41176 +205
Branches 4763 4800 +37
========================================
+ Hits 34838 35013 +175
- Misses 5052 5071 +19
- Partials 1081 1092 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…verage
CI blockers:
- Fix mypy: click.utils.LazyFile → typing.TextIO in pbac.py
- Run ruff format on all Python files
Logic fixes:
- Deterministic output: sort dataset keys before iterating Go map
- Stable sort: sort.Slice → sort.SliceStable for policy priority ties
- Enabled default: *bool pointer so nil (omitted) defaults to true,
matching Python's p.get("enabled", True)
- Complete audit trail: decisive policy now included in EvaluatedPolicies
for both ALLOW-inverted-to-DENY and normal decisive paths
- Empty taxonomy guard: taxonomyMatch("", x) returns false in both Go
and Python to prevent accidental catch-all
Code smells:
- Replace interface{} return from checkAccess with typed accessCheckResult
- Remove unused GapUnconfiguredConsumer from Go (service-layer concern)
- Document Constraint.Operator field: which operators apply to which types
Missing tests (Go + Python):
- MatchDimension with both Any and All on same dimension
- Duplicate Unless constraints (AND of identical = no-op)
- Empty taxonomy match key never matches
- Enabled defaults to true when omitted
54 Go tests + 48 Python tests, all passing.
Critical fixes: - evaluate.py now uses typed dataclasses (ParsedPolicy, AccessEvaluationRequest, PolicyEvaluationResult) instead of raw dicts - InProcessAccessPolicyEvaluator conforms to the existing AccessPolicyEvaluator Protocol from interface.py - JSON conversion (parsed_policy_from_dict, request_from_dict, result_to_dict) pushed to CLI boundary, not inside the service - CLI evaluate-policies now constructs typed objects from JSON, matching the pattern used by evaluate-purpose - Lazy import removed — all imports at module level Significant fixes: - CI: add go mod tidy verification step - Go: document EvaluatePoliciesRequest/EvaluatePurposeRequest as sidecar types (used by fidesplus companion PR) - Go + Python: add priority tie-breaking test (stable sort preserves insertion order) 55 Go tests + 51 Python tests, all passing.
- Go CI: remove go.sum from git diff check (no external deps, file doesn't exist) - Ruff: fix import block formatting (extra blank line) - ParsedPolicy.decision: change from str to PolicyDecision enum so invalid values fail at construction time rather than evaluation time - .gitignore: add test_report.xml (pytest junitxml artifact)
Tests were constructing ParsedPolicy with decision="ALLOW"/"DENY" strings, but the field is now PolicyDecision. This caused 'str' object has no attribute 'value' in evaluate_policies.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — Go PBAC Policy Engine (#7926)
This is a well-structured addition: a standalone Go library (policy-engine/pkg/pbac) implementing purpose-based access control evaluation, mirrored in Python for the CLI fallback, with thorough test coverage on both sides. The algorithm documentation (rules 1–7 in both implementations) is clear and the Go/Python parity is a good discipline.
Critical
actions/checkout@v6 will break CI — v6 does not exist; latest stable is v4. Every run of the new policy_engine_checks.yml workflow will fail at the Checkout step until this is corrected. See inline comment.
Findings
-
No
gofmtcheck in CI — The workflow runsgo vetbut notgofmt. There are alignment inconsistencies inpolicy_types.go(Constraint.Field/Values,EvaluatedPolicyInfo.UnlessTriggered) that would be caught bytest -z "$(gofmt -l .)". These won't cause build failures but will accumulate over time. -
PolicyDecision()ValueError not caught inparsed_policy_from_dict— An unknown decision string in the CLI JSON input triggers a raw PythonValueErrortraceback. Should be caught and surfaced as a clean error message. -
Audit trail omits non-matching policies — In both implementations,
EvaluatedPoliciesonly includes policies that passedmatchesRequest. A caller cannot distinguish "no policies loaded" from "many policies loaded, none applicable". AddingMatched: falseentries for skipped policies would make the audit log complete. -
result_to_dictalways emits"action": null— Even for ALLOW decisions, the serialized output always contains"action": null. The Go equivalent would omit the field viajson:",omitempty". Callers doingif result["action"]will get unexpected behavior.
Positive Notes
- The taxonomy prefix-match logic (
taxonomyMatch) is correctly guarded against accidental catch-all (empty matchKey → false) and uses a dot-boundary check rather than a raw string prefix, preventing"user"from matching"user_data". sort.SliceStableis the right choice for priority ordering — it preserves original policy order for equal-priority policies, which is an important tie-breaking guarantee.- The
ensureViolations/ensureGapshelpers that return[]T{}instead of nil are a good JSON serialization practice. - Isolating the Go engine with
working-directory: policy-engineand a separatego.modis clean — the Python monorepo remains unaffected.
🔬 Codegraph: unavailable
💡 Write /code-review in a comment to re-run this review.
| if unlessTriggered { | ||
| if policy.Decision == PolicyAllow { | ||
| // ALLOW inverted to DENY — decisive, stop | ||
| key := policy.Key |
There was a problem hiding this comment.
policy-engine/pkg/pbac/policy_evaluate.go:44
Policies that don't satisfy matchesRequest are silently skipped and never appear in EvaluatedPolicies. This means the audit trail only shows policies that matched, making it impossible for a caller to distinguish "no policies exist" from "many policies exist but none matched".
Consider appending a Matched: false entry for each enabled policy that was evaluated but didn't match, so the full evaluation path is visible in the audit output.
There was a problem hiding this comment.
I'm not sure this is a business case that even needs to be handled. Do we need to keep track of different types of unmatched requests in the audit trail @galvana?
- CI: add gofmt check and include go.sum in module verification diff - Fix gofmt alignment drift in Constraint and EvaluatedPolicyInfo structs - parsed_policy_from_dict: raise InvalidPolicyError with friendly message on bad decision values (instead of raw ValueError) - CLI: catch InvalidPolicyError for clean error output on bad JSON - result_to_dict: omit null action (matches Go omitempty behavior)
go.sum doesn't exist when there are no external deps, and git diff fails on a nonexistent file. Only check go.mod.
Addresses review items from code-review-policy-engine-go-library.md: #1 — Guard Python eval helpers against malformed context. Added isinstance checks in _eval_consent (consent must be dict), _eval_data_flow (flows_map must be dict, direction_flows must be list). Matches Go's type-assertion behavior of returning false on type mismatch instead of crashing. #2 — Remove unused `from dataclasses import asdict` import. #3 — Reject NO_DECISION as policy input. parsed_policy_from_dict now raises InvalidPolicyError if decision is NO_DECISION. Prevents policies that would break the "no policy matched" contract. #4 — Remove dead `reason` field from PolicyEvaluationResult in both Go and Python. Never populated, never serialized. #5 — Log warning on unknown constraint type in _evaluate_constraint instead of silently returning False. Helps catch misspelled types. #12 — Wrap evaluate-purpose input parsing in try/except for TypeError and AttributeError so malformed input (e.g. purpose_keys as a string instead of list) produces a friendly error instead of a traceback.
NO_DECISION is an output state meaning 'no policy matched', not a valid policy decision. Policies with decision=NO_DECISION are now silently skipped during evaluation (same as disabled policies). Matches the Python-side validation added in the prior commit.
|
Attaching files used during development/review @galvana review and response |
There was a problem hiding this comment.
Code Review: Reject NO_DECISION policies in Go EvaluatePolicies
The change is correct and safe. The core logic — filtering NO_DECISION policies during the enabled pre-pass so they produce NO_DECISION output (fail-safe) rather than a false match or panic — is the right approach. The test TestPolicy_NoDecisionPoliciesSkipped covers the new behaviour.
Findings
Structural nesting makes NO_DECISION guard harder to reason about (see inline on policy_evaluate.go:31)
The NO_DECISION check is nested inside the Enabled branch. A disabled policy with Decision=NO_DECISION is dropped for the wrong reason (disabled, not invalid), and the guard becomes unreachable for disabled policies if the filter is ever restructured. Flattening to two independent guards would make the intent explicit.
No observability when a policy is silently skipped (see inline on policy_evaluate.go:29)
A misconfigured NO_DECISION policy leaves no trace — no log entry, no EvaluatedPolicies entry. It's indistinguishable from "policy list was empty." A log line or an INVALID result in the audit trail would help operators diagnose misconfigured data.
Missing Python test for decision="NO_DECISION" input (see inline on evaluate.py:181)
The Python parsed_policy_from_dict explicitly rejects NO_DECISION, but there is no test asserting this. The existing invalid-decision test only covers unrecognised strings, not the enum constant that must be rejected. A parity test with the new Go test would close this gap.
Minor: test file placement (see inline on edge_cases_test.go:751)
TestPolicy_NoDecisionPoliciesSkipped lives in edge_cases_test.go but exercises EvaluatePolicies, which is otherwise tested in policy_evaluate_test.go.
Summary
No critical issues. The main actionable gaps are: (1) adding a Python test for parsed_policy_from_dict({"decision": "NO_DECISION"}) to match the new Go test, and (2) adding some form of observability (log line or audit entry) when a policy is silently skipped for having an invalid decision.
🔬 Codegraph: unavailable
💡 Write /code-review in a comment to re-run this review.
| // not a valid policy decision. Skip rather than panic so | ||
| // callers with misconfigured data get NO_DECISION (safe) | ||
| // rather than a false match. | ||
| if p.Decision != PolicyAllow && p.Decision != PolicyDeny { |
There was a problem hiding this comment.
policy-engine/pkg/pbac/policy_evaluate.go:29
When a policy is skipped because its decision is NO_DECISION, there is no log entry, no error, and the skipped policy does not appear in EvaluatedPolicies. From an operator's perspective this looks identical to "the policy list was empty." A misconfigured policy is effectively invisible.
At minimum, consider adding a structured log line when a policy is skipped for invalid decision so it's observable in production without requiring a debugger. If the audit trail matters for your use case, you could also include skipped-invalid policies in EvaluatedPolicies with Result: "INVALID" to make them distinguishable from simply-unmatched policies.
2 participants
Ticket ENG-3084
Description Of Changes
Introduces a Go library implementing the PBAC evaluation engine for high-throughput policy evaluation, plus Python CLI commands for local testing.
Architecture:
policy-engine/pkg/pbac/— Go library with two evaluation steps that always run together as part of the PBAC pipeline:src/fides/cli/commands/pbac.py— Python CLI:fides pbac evaluate-purposeandfides pbac evaluate-policiessrc/fides/service/pbac/policies/evaluate.py— Python implementation of the Access Policy v2 evaluator (mirrors the Go implementation)The Go library is imported by a fidesplus sidecar for API-level throughput (~13x faster than Python). The CLI goes through Python directly — performance isn't the concern for CLI usage.
Code Changes
policy-engine/pkg/pbac/(new Go modulegithub.com/ethyca/fides/policy-engine)types.go/evaluate.go— Purpose-overlap evaluationpolicy_types.go/policy_evaluate.go— Access Policy v2 evaluationevaluate_test.go,policy_evaluate_test.go,edge_cases_test.gosrc/fides/cli/commands/pbac.pyregistered insrc/fides/cli/__init__.pysrc/fides/service/pbac/policies/evaluate.pytest_evaluate_access_policies.pyandtest_pbac_cli.py.github/workflows/policy_engine_checks.ymlrunsgo build,go vet,go teston policy-engine/ changes92 automated tests total covering every evaluation rule, match mode (any/all), all three unless condition types (consent with opt_in/opt_out/not_opt_in/not_opt_out, geo_location with in/not_in, data_flow with any_of/none_of), taxonomy hierarchy matching, priority ordering, and CLI I/O.
Steps to Confirm
cd policy-engine && go test ./... -vPre-Merge Checklist
CHANGELOG.mdupdated