feat(schema-compiler): require includes or excludes in accessPolicy memberLevel - #11934
paveltiunov wants to merge 6 commits into
Conversation
…emberLevel
`memberLevel.includes` defaults to '*', so `memberLevel: {}` silently grants
access to every member of the cube -- the opposite of how an empty block reads.
It is also a silent no-op for `memberMasking` in the same policy: members
granted in full by `memberLevel` are never masked, so a policy pairing an empty
`memberLevel` with `memberMasking` compiles without complaint and returns the
raw value for the member it was meant to mask.
Require `memberLevel` to spell out at least one of `includes` or `excludes`
when it is present. Policies that omit `memberLevel` entirely are unaffected
and still grant all members.
BREAKING CHANGE: a data model with an empty `member_level` block in an
`access_policy` no longer compiles. Use `includes: '*'` to keep granting all
members, or `excludes` to grant all but some.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017GxjcRUJBFdm1JBch2Fcr1
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
github-actions
Bot
added
docs
|
Claude finished @paveltiunov's task in 2m 53s —— View job Sound change, incomplete against the bug it targets — 1 medium, 1 low (comment length), 1 top-level note. 2 inline comments posted. Full review
FindingsMedium — the rule blocks one spelling of the footgun, and the error message recommends another (
The shape rule still has independent value ( Low — comment length ( Top-level note — breaking change, no deprecation window. A model with an empty Verified, no issue
Not verifiedCould not run the test suite — Review threadsThe unresolved-thread listing returned 0 of my own threads (no prior threads); 0 resolved, 0 re-affirmed. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
`includes: '*'` grants every member exactly as an empty `memberLevel` does, so recommending it as the blanket fix steers a policy that also defines `memberMasking` straight into the same silent no-op: the member is granted unconditionally, so the mask never fires. Point at `excludes` for members the policy intends to mask, and say why. Same caveat added to the reference docs. Tighten the schema comment, which attributed the dead mask to emptiness when any unconditional grant causes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017GxjcRUJBFdm1JBch2Fcr1
|
Claude finished @paveltiunov's task in 4m 20s —— View job Review: request changes — 1 medium (inaccurate "never masked" claim in the error message and docs), 2 lowFull review
Findings1. "A member granted by memberLevel is never masked" is false for policies with row filters — medium
That is the shipped conditional-masking feature, exercised by Raised as a reply on the existing open thread for the message itself; posted inline on the docs 2. New test pins the inaccurate wording — low
3.
Notes (no inline comment)
Verification
|
…sked
"A member granted by memberLevel is never masked" is false when the granting
policy also carries rowLevel filters: `hasUnconditionalFullAccess` requires
`inFullAccess && (!rowLevel || rowLevel.allowAll)`, so a policy granting all
members with row filters still falls through to the masking branch and renders
`CASE WHEN {rowFilter} THEN {value} ELSE {mask} END` -- the conditional_masking
fixture shape.
As written the message told those users to move the member into `excludes`,
which revokes access instead of masking it. Say what is actually true: a
granted member is unmasked on every row the policy grants, so only a member
that must always be masked belongs in `excludes`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017GxjcRUJBFdm1JBch2Fcr1
|
Claude finished @paveltiunov's task in 4m 12s —— View job Review complete — the rule is sound; 0 high, 0 medium, 2 low (deprecation path, concepts-page doc gap)Full review
VerdictThe rule itself is correct and the semantics claim in the new message and docs checks out against the runtime. Checked and clean:
Findings
Both posted inline. Not run
Review threads4 of my own unresolved threads returned; 3 resolved (2 fixed by |
| }) | ||
| // `includes` defaults to '*' in CubeEvaluator.prepareAccessPolicy, so an empty | ||
| // memberLevel grants every member — the opposite of how it reads. | ||
| .or('includes', 'excludes') |
There was a problem hiding this comment.
A model shape that compiles on the current release stops compiling on upgrade, with no release where the user is told before it breaks. memberLevel: {} is not a syntax error the user can be assumed to have never written — it is the shape that reads as "nothing configured", which is exactly why it is worth rejecting, and equally why some models in the field will have it.
ErrorReporter already supports the softer landing (ErrorReporter.ts:63, public warning(...)); CubeValidator only ever calls errorReporter.error(formatErrorMessage(result.error)) at CubeValidator.ts:1440, so nothing here can degrade to a warning without special-casing this one Joi rule out of the schema result — e.g. checking the empty memberLevel in prepareAccessPolicy (where includes || '*' lives and where the memberMasking-without-memberLevel diagnostic already sits, CubeEvaluator.ts:411) and reporting it as a warning for one minor, error after.
Your call as maintainer — a hard error in a minor is defensible for a rule this narrow, and the PR body does call the break out. Flagging it because the cost lands at upgrade time on a model that was silently wrong rather than at authoring time, and the same message reads fine as a warning.
There was a problem hiding this comment.
Agreed this is the author's call, and it's with them — I've raised it twice and it's stated as an open question in the PR body. Not landing either way unilaterally.
Confirmed your mechanics, which are the useful part here, because they make the choice more expensive than it looks:
ErrorReporter.warningexists (ErrorReporter.ts:63).CubeValidatornever calls it —.errorat1440,1461,1531only, and1440is the single funnel for every Joi result. There's no per-rule severity, so the.or()can't degrade to a warning in place.CubeEvaluator.ts:412is thememberMasking-without-memberLeveldiagnostic insideprepareAccessPolicy, which is the natural home for a warning-based version — detection would be!memberLevel.includes && !memberLevel.excludes, checked beforeincludes || '*'resolves it.
So the warning route isn't a severity flip on this rule, it's moving the check out of the schema into prepareAccessPolicy and dropping the .or() — a different change, with the rule no longer expressed where the rest of the policy shape is. That's the real trade-off against a hard error in a minor, and worth stating plainly rather than leaving it to read as a one-liner.
One correction to your framing: the cost lands at upgrade time, but not on a model that is silently wrong in the sense of broken — a model with memberLevel: {} behaves today exactly as includes: '*'. It's wrong only where it also carries memberMasking, which is the case this PR exists for. A model using memberLevel: {} as a plain "grant everything" is working as intended and would break on upgrade purely for spelling. That arguably strengthens the deprecation argument rather than weakening it.
Generated by Claude Code
… page
The concepts page enumerates the widening defaults ("a policy with no
member_level spans every member") without the new constraint. The sentence
stays true for an omitted member_level, but a reader can take it as licence to
write an empty block, which now fails to compile with the rule documented only
on the reference page.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017GxjcRUJBFdm1JBch2Fcr1
|
Claude finished @paveltiunov's task in 5m 16s —— View job Review complete — the rule is correct and well-targeted. 1 medium (new), 2 prior threads re-affirmed, 0 blockers.Full review
What holds upThe core claim checks out at the source. The corrected message wording — "unmasked on every row the policy grants" — is accurate for both shapes: with no Scope check on the breaking change: every Finding posted
Review threadsThe listing returned 2 threads first-authored by
One further concern deliberately not posted: Not verifiedTests were not run — |
|
CI note — Every one of the 97 failures is the same error, before any test body runs: 97 failed / 50 skipped / 147 total — the SQL Server container rejected authentication, so the suite never reached a query. Why it isn't this PR's:
Not re-running it manually: the failure is on No fix ported because there is nothing to port — this isn't a code failure with a known fix elsewhere. Generated by Claude Code |
|
CI note 2 — 19 failed / 24 skipped / 102 passed / 145 total. Every failure is the same error: 102 tests passed, so this is not a connectivity or credentials problem — the suite ran, and only the pre-aggregation-dependent tests failed. Why it isn't this PR's:
What I verified vs. inferred: verified that this PR's diff contains no query-orchestrator changes, that the driver fixtures carry no access policies, and that No fix ported because none exists to port — there's no revert or follow-up PR for Not re-running manually: cc @waralexrom as the author of #11629, in case this is already known. Generated by Claude Code |
The custom `object.missing` message replaced Joi's default wholesale, dropping
`{{#label}}` with it, so the error never said which policy was wrong. Worse,
`formatErrorMessageFromDetails` keys its dedupe map on the message text, so two
policies with an empty `memberLevel` in the same cube produced byte-identical
messages and collapsed into a single line naming neither:
before: 1 error, names accessPolicy[0]? no, accessPolicy[1]? no
after: 2 errors, names accessPolicy[0]? yes, accessPolicy[1]? yes
A cube with several policies now points at the one to edit. The assertions move
off the `memberLevel must ...` prefix, since the rendered label is quoted ahead
of it and `formatErrorMessage` rewrites it to `(label = value)` in the reasons
list, and a new case pins both labels and the un-deduped count.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017GxjcRUJBFdm1JBch2Fcr1
|
Claude finished @paveltiunov's task in 5m 25s —— View job Approve — 0 high, 0 medium, 1 low. The rule, the message and both docs pages check out; the one note is test hygiene.Full review
What I verifiedThe rule is in the right place and cannot misfire. Cube The error message's advice is correct, including the case it previously got wrong. Traced The "unmasked on every row the policy grants" phrasing also survives the Nothing in the repo breaks. Swept every Docs. The concepts-page parenthetical sits inside the "no Comment lengths. Both source and test comments are 2-3 lines; none exceeds the limit. Finding
Not run
One consequence I could therefore only infer, not confirm: Review threadsThe listing returned 3 threads authored by me. Resolved 2 — On that open thread, for the record: I agree with the PR body that this is the author's call. The breaking surface is narrow — every |
…t pins The comment claimed `ssn` is granted in full and the mask never fires, which is applyRowLevelSecurity behaviour the test never reaches -- it stops at `result.error`. The rule reads only memberLevel, so the fixture's memberMasking is inert to it. Say what the case actually pins: that the motivating combination stays rejected. The masking no-op itself belongs to applyRowLevelSecurity in server-core, which schema-compiler cannot reach -- the dependency runs the other way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017GxjcRUJBFdm1JBch2Fcr1
|
Claude finished @paveltiunov's task in 4m 14s —— View job Review round on
|
|
CI note 3 — Unlike the others, exactly 1 test failed (34 skipped, 115 passed, 150 total) — and it's the one every pre-aggregated test depends on: Two things worth drawing out: 1. Same root cause as the snowflake failures, different surface. Snowflake's 19 failures were all "No pre-aggregation partitions were built yet" — the downstream symptom of builds not landing. BigQuery catches it at the build step itself. Both are pre-aggregation builds failing; only the point of detection differs. 2. The error text is Either reading points at Running total of driver checks red, across
The pre-agg failure count is invariant at 19 across every commit — it does not track anything in this branch. @waralexrom — flagging again given the second driver family; this looks like it wants eyes on #11629 independent of this PR, since anything branching off current master will hit it. Generated by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11934 +/- ##
=======================================
Coverage 61.23% 61.23%
=======================================
Files 247 247
Lines 19824 19824
Branches 4043 4043
=======================================
Hits 12140 12140
Misses 7112 7112
Partials 572 572
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CI update on Everything this PR could plausibly affect is green on the current head:
Correction to CI note 1: Correction to CI note 3: What actually remains red: So the blast radius is narrower than my earlier notes implied — two snowflake-azure checks, not a broad driver-suite outage. Apologies for the noise on the ones that turned out to be transient; I'd rather correct them than leave three notes overstating the situation. Generated by Claude Code |
|
Retracting half of my last comment. I said 2 failed / 34 skipped / 114 passed (previously 1 failed / 115 passed). CI note 3's analysis stands as originally written; the retraction I offered for it does not. Corrected standing on
So: one of the two problems was transient, the other is real and spans two driver families on the current head. That is the shape I described in CI notes 2 and 3, and only the mssql part of CI note 1 has aged out. Everything this PR can affect remains green — Sorry for the churn across these notes — I over-corrected before the bigquery job had finished, which I should have waited for rather than reporting an in-progress check as evidence of recovery. Generated by Claude Code |
This branch was successfully deployed
3 participants
Check List
Description of Changes Made
memberLevel.includesdefaults to'*'(CubeEvaluator.prepareAccessPolicy,policy.memberLevel.includes || '*'), somemberLevel: {}silently expands to every measure, dimension and segment of the cube — the opposite of how an empty block reads.It is also a silent no-op for
memberMaskingin the same policy when that policy has no restrictiverowLevel.applyRowLevelSecuritymasks a member only when no granting policy gives it unconditional full access, and an emptymemberLevelwith norowLevelgives exactly that. So this compiles clean today and returns the raw value for the member it was meant to mask:This PR requires
memberLevelto spell out at least one ofincludesorexcludeswhen it is present. The intent above is expressible asmemberLevel: { excludes: ['ssn'] }, which masks correctly.Scope of the rule — deliberately a presence rule, not a resolved-grant rule
It rejects a
memberLevelblock that configures nothing. It does not reject every spelling that resolves to a full grant:memberLevel: { includes: '*' }stays valid, because that is an explicit statement of intent rather than an accident. Pairingincludes: '*'withmemberMaskingstill yields the no-op, which the error message now warns about rather than recommending.The adjacent shapes were checked and need no rule of their own:
{ excludes: [] }and{ includes: [], excludes: [] }were already rejected before this PR (excludescarries.required()on its array item, so an empty array fails, unlikeincludes), and{ excludes: '*' }grants nothing, so masking applies normally.Behaviour
memberLevel: {}+memberMasking: { includes: ['ssn'] }ssnreturned unmaskedmemberLevel: { excludes: ['ssn'] }+ same maskingssnmaskedmemberLevel: { includes: [...] | '*' },excludes, or bothmemberLevel: { includes: '*' }+rowLevel.filters+ maskingCASE WHEN … ELSE mask)memberLevelkey at allmember_level:with an empty body (parses asnull)must be of type object)The error message keeps Joi's
{{#label}}, so a cube with several policies names the offending one. Without it,formatErrorMessage's dedupe (keyed on message text) collapsed two broken policies into a single line naming neither.Breaking change
A data model with an empty
member_levelblock in anaccess_policyno longer compiles. Useincludes: '*'to keep granting all members, orexcludesto grant all but some. Nothing in this repository relies on it — everymember_levelin fixtures, tests, birdbox models and docs already setsincludesorexcludes.Open question for the reviewer: whether this warrants an
errorReporter.warningrelease before the hard error. The policy's runtime behaviour is unchanged by this PR, so a warning round would cost nothing in the interim. Note it is not a severity flip in place —CubeValidatorfunnels every Joi result through a singleerrorReporter.error(CubeValidator.ts:1440), so a warning means moving the check intoCubeEvaluator.prepareAccessPolicyand dropping the.or().Testing
cube-validator.test.ts: the two rejections (bare{}, and{}paired withmemberMasking), the five shapes that must stay valid, the omitted-memberLevelcase, one pinning the error message's guidance so a reword can't reintroduce an inaccurate claim, and one pinning that several empty policies are each named rather than deduped away.cubejs-schema-compilerunit suite green on the default (Tesseract) planner: 49/49 suites, 975/975 tests, 110/110 snapshots.transpiledFieldsPatternsre-run, per the note inCubeValidator.tsabout schema-shape changes.oxlintclean on the changed files.compiler.compile()and throughapplyRowLevelSecurity+getSql, not only at the Joi layer — including the conditional-masking shape (includes: '*'+rowLevel.filters) to confirm the error message's wording holds there.CI
The red driver checks (
mssql,snowflake-export-bucket-azure{,-prefix},bigquery-export-bucket-gcs) are not this PR's — see the CI notes in the comments. The pre-aggregation failures reproduce with an invariant count across four commits whose only deltas were docs prose and an error string, and point at42bfe75(#11629), which rewrotePreAggregationLoader.ts/PreAggregations.tsshortly before this branch. This PR touches nothing inpackages/cubejs-query-orchestrator/.Follow-ups, deliberately not in this PR
memberMaskinghas the identicalincludes || '*'default, somemberMasking: {}silently means "mask every member".memberMasking.includeswhile also granted unconditionally bymemberLevel. It has to be restricted to explicitly-named members: a blanket overlap rule would reject the documentedmember_level: { includes: [status, count] }+member_masking: { includes: "*" }pattern, where the wildcard necessarily overlaps the granted members and masking works correctly today.applyRowLevelSecurity's masking branch anywhere (CompilerApi.test.tscovers onlydispose()), which is why this no-op could exist unnoticed. That belongs incubejs-server-core/test/unit/.🤖 Generated with Claude Code
https://claude.ai/code/session_017GxjcRUJBFdm1JBch2Fcr1