fix: Support not in access policy conditions

[paveltiunov]

Check List

• Tests have been run in packages where changes have been made if available
• Tests for the changes have been added if not covered yet
• Linter has been run for changed code
• Docs have been added / updated if required

Description of Changes Made

1. Fix: not silently dropped in YAML Python expression parser

The YAML expression parser (PythonParser.ts) silently dropped not — not x produced x instead of !x. The Not_testContext visitor checked children.length === 1 but the terminal not keyword is not a rule node, so the visitor always saw 1 child and returned it directly, ignoring the negation.

Fix: Check node.getChildCount() (which includes terminal nodes) instead of children.length (which only has rule-node results). When the node has 2 children (the not terminal + the inner expression), emit t.unaryExpression('!', ...).

Also added support for and (&&) and or (||) operators which previously threw Unsupported Python multiple children node errors.

Python	JS output	Parser node
not x	!x	Not_testContext
a and b	a && b	And_testContext
a or b	a || b	Or_testContext

File changed: packages/cubejs-schema-compiler/src/parser/PythonParser.ts

2. Test: Group-based conditional region row filtering (YAML view)

A smoke test that exercises the not fix with a YAML view using group-based conditions:

access_policy:
  - group: user_group
    conditions:
      - if: "{ security_context.auth.groups and security_context.auth.groups.includes('region_group') }"
    row_level:
      filters:
        - member: product_id
          operator: equals
          values: security_context.auth.userAttributes.allowedProductIds

  - group: user_group
    conditions:
      - if: "{ not (security_context.auth.groups and security_context.auth.groups.includes('region_group')) }"
    row_level:
      allow_all: true

A user with both user_group and region_group is correctly filtered because the conditions are mutually exclusive — includes() vs not ... includes(). Without the not fix, the second condition would silently evaluate to the same as the first, and both policies would match.

Test files:

File	Change
birdbox-fixtures/rbac/model/cubes/region_test.yaml	New cube backed by public.line_items
birdbox-fixtures/rbac/model/views/region_test_view.yaml	YAML view with group-based conditions using not
birdbox-fixtures/rbac/cube.js	Two SQL users: region_user (['user_group', 'region_group']) and region_user_no_filter (['user_group'])
test/smoke-rbac.test.ts	Three test cases

Test Results

All 79 existing RBAC tests pass. 3 new tests pass. Only 3 pre-existing Python config failures remain (unrelated).

  

[claude]

Claude finished @paveltiunov's task in 1m 16s —— View job

---

• Read all changed files
• Review code quality and best practices
• Review security considerations
• Review test coverage and quality
• Provide detailed feedback

---

Review Summary

This is a well-structured smoke test addition that validates group-based conditional row filtering. The implementation follows existing patterns in the test suite and the test scenario is clearly documented. A few observations below.

Findings

1. Unused hasRegionFilter user attribute (minor)

Both region_user and region_user_no_filter in cube.js define a hasRegionFilter boolean in userAttributes:

// cube.js:200-201
userAttributes: {
  region: 'San Francisco',
  hasRegionFilter: true,  // never referenced
},

However, the view's access policy in region_test_view.js:12 checks group membership (security_context.auth?.groups?.includes('region_group')) rather than this attribute. The hasRegionFilter attribute is never consumed by any policy or test. Consider removing it to avoid confusion, or if it was intended for a different approach, it might be worth a comment.

Fix this →

2. No REST API test coverage for the new scenario (suggestion)

Other RBAC test scenarios in this file validate behavior through both the SQL API (via PgClient) and the REST API (via @cubejs-client/core). The new region group tests only cover the SQL path. Adding REST API tests (similar to the existing masking REST tests at lines 572-783) would ensure consistency across both interfaces:

// Example: sign a JWT with region_user's context
const REGION_USER_TOKEN = sign({
  auth: {
    username: 'region_user',
    userAttributes: { region: 'San Francisco' },
    roles: [],
    groups: ['user_group', 'region_group'],
  },
}, DEFAULT_CONFIG.CUBEJS_API_SECRET, { expiresIn: '2 days' });

This is non-blocking — the SQL API tests provide good coverage of the core behavior.

3. No Tesseract coverage (informational)

The new tests are added under the Cube RBAC Engine describe block but not under Cube RBAC Engine [Tesseract]. This is likely intentional given the smoke-test scope, but worth noting if Tesseract parity is desired.

4. Code quality — looks good

• The region_test.yaml cube definition is minimal and appropriate — only exposes id, city, and count, matching exactly what the tests query.
• The region_test_view.js access policy logic is clean. Using the ternary with optional chaining (security_context.auth?.groups?.includes(...)) is a readable pattern for conditional row-level filtering.
• The three test cases form a complete validation set: (1) filtered user sees only matching rows, (2) unfiltered user sees all rows with multiple cities, (3) count comparison confirms the filter is actually restricting results.
• User definition blocks in cube.js follow the existing pattern of the file.

5. Security — no issues

The access policy correctly scopes to user_group group membership before applying any row-level logic, and the conditional filter relies on server-side security_context — no client-controlled bypass path.

Verdict

The PR is clean and well-tested. The only actionable item is the unused hasRegionFilter attribute (finding #1). The REST API and Tesseract suggestions are optional enhancements.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.96%. Comparing base (af206b4) to head (57e208c).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
.../cubejs-schema-compiler/src/parser/PythonParser.ts	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10767      +/-   ##
==========================================
- Coverage   58.01%   57.96%   -0.05%     
==========================================
  Files         215      215              
  Lines       16764    16776      +12     
  Branches     3383     3389       +6     
==========================================
  Hits         9725     9725              
- Misses       6547     6559      +12     
  Partials      492      492              

Flag	Coverage Δ
cube-backend	57.96% <0.00%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @paveltiunov's task in 1m 48s —— View job

---

• Read all changed files and gather context
• Review code quality and best practices
• Review security considerations
• Review test coverage and quality
• Review performance considerations
• Provide detailed feedback

---

Review Summary

Clean, well-structured smoke test addition. The test scenario is sound and the implementation follows existing patterns in the RBAC test suite. The earlier review's concern about unused hasRegionFilter has already been addressed — it's no longer present.

Findings

1. Test relies on data distribution assumption (low risk)

The test at smoke-rbac.test.ts:1115-1116 asserts that the unfiltered user sees more than 2 distinct product_id values:

const productIds = new Set(res.rows.map((r: any) => r.product_id));
expect(productIds.size).toBeGreaterThan(2);

This assumes the first 50 rows of public.line_items (ordered by id) contain at least 3 distinct product_id values. While this is almost certainly true for realistic test data, it's a soft coupling to the seed data. Not a blocker — just worth noting if the seed data ever changes.

2. PR description mentions city dimension that doesn't exist (nit)

The PR body states the cube "exposes id, city, and count", but the actual region_test.yaml defines id, product_id, and count. No city dimension exists. Cosmetic-only — no code impact.

3. Code quality — looks good

• region_test.yaml: Minimal cube definition with a permissive wildcard access_policy at the cube level. The real access control is correctly scoped at the view level, which is the right pattern for testing view-layer policies.
• region_test_view.js: The dual-policy approach with mutually exclusive conditions (includes vs !includes) is clean and well-structured. Optional chaining (security_context.auth?.groups?.includes(...)) is defensive. The ! negation on line 29 correctly creates the complement condition.
• cube.js: User definitions follow the established pattern exactly. region_user_no_filter correctly omits allowedProductIds from userAttributes and only has user_group (no region_group), so the second policy's allowAll branch will activate.
• smoke-rbac.test.ts: Three test cases form a complete validation triangle: (1) positive filter check, (2) negative filter check, (3) count comparison. The beforeAll/afterAll lifecycle with JEST_AFTER_ALL_DEFAULT_TIMEOUT follows existing test patterns. The block comment explaining the test scenario is thorough and appropriate for a non-obvious RBAC setup.

4. Security — no issues

The access policy correctly gates on user_group membership before evaluating conditions. The row-level filter uses server-side security_context values, so there's no client-controlled bypass. The allowedProductIds filter uses the equals operator which is safe against injection.

5. Squash opportunity (suggestion)

The branch has 8 commits with significant iteration (YAML→JS→YAML→JS conversions, reverts). Consider squashing into a single commit on merge for a cleaner history.

Verdict

Approve. The PR is clean, follows established patterns, and adds meaningful test coverage for a non-trivial RBAC scenario. No blocking issues found.
|