Admit a caller by the subjects it holds, not by its identity alone - #40
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @LucaCappelletti94, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 1 day and 21 hours by commenting @sourcery-ai review. Upgrade to get a review now.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: LucaCappelletti94/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer's GuideExtends caller-aware subscriptions from identity-only authorization to independently compiled identity and subject-set authorization, including precise per-term seeding, combined hidden membership mirrors, delimiter-safe reverse translation, and end-to-end share-key coverage. Sequence diagram for subject-set subscription authorizationsequenceDiagram
participant Caller
participant Server
participant Materializer
participant Postgres
participant Replica
Caller->>Server: Subscribe(query)
Server->>Materializer: translate_subscription_sql(query)
Materializer->>Postgres: Compile identity and subject-set mappings
Server->>Server: seed_for_terms(terms)
alt Identity term
Server->>Server: typed_subscriber(identity)
else Subject-set term
Server->>Server: caller_subjects(identity, capabilities, kind)
end
Server->>Replica: Register query with precise seed
Server->>Server: mirror_predicate(identity_function, subjects_function)
Server->>Replica: Open hidden membership subscription
Replica-->>Caller: Rows granted by identity or held subject key
Flow diagram for combined membership mirroringflowchart LR
Terms["Membership terms"] --> Table["One mirror per membership table"]
Table --> Identity["Identity reach"]
Table --> Subjects["Subject-set reach"]
Identity --> Or["Joined with OR"]
Subjects --> Or
Or --> Changes["Membership changes delivered"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
5cc2820 to
68b291f
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cc2820f7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| caller_subjects( | ||
| state.principal.identity(), | ||
| state.principal.capabilities(), | ||
| kind, |
There was a problem hiding this comment.
When an identified caller registers a term reading the subject-set setting, this passes both its identity and capabilities to caller_subjects, even though CallerBinding populates app.subjects exclusively with CapabilityKey::pack(caller.capabilities()). Consequently, a direct subject-set comparison or later membership event naming the user's identity can match in the compiled term although the actual PostgreSQL predicate is false, producing live results outside the registered filter. Pass only capability subjects when seeding TermCaller::Subjects.
Useful? React with 👍 / 👎.
| if state.subs.contains_key(&label) { | ||
| return Ok(()); |
There was a problem hiding this comment.
When one session has separate subscriptions that reference the same membership table using different caller kinds, the first subscription installs a table-only hidden label and this early return prevents the second subscription from adding its reach. For example, an identity-based subscription opened first leaves a later subject-set subscription with an identity-only mirror, so key-owned membership rows are neither snapshotted nor delivered on subsequent changes; reversing registration order breaks the identity-based subscription instead. The existing mirror must be replaced or widened when the new MemberTable flags are not already covered.
Useful? React with 👍 / 👎.
| "CASE WHEN {function}() IS NOT NULL THEN {function}() <> '' \ | ||
| AND instr({column}, '{separator}') = 0 \ | ||
| AND instr('{separator}' || {function}() || '{separator}', \ | ||
| '{separator}' || {column} || '{separator}') > 0 END" |
There was a problem hiding this comment.
For deployments whose custom CapabilityKey::SEPARATOR is a single quote, interpolating the character directly between SQL quotes produces an unterminated or malformed literal in every generated instr expression. Since the trait explicitly permits deployments to choose this separator, subject-set membership subscriptions then fail while being opened; render the separator as an escaped SQLite string literal rather than inserting the raw character.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
==========================================
- Coverage 83.29% 83.29% -0.01%
==========================================
Files 114 114
Lines 25770 25937 +167
Branches 25770 25937 +167
==========================================
+ Hits 21466 21605 +139
- Misses 3076 3095 +19
- Partials 1228 1237 +9
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:
|
|
1 participant
A caller has always been a set here, an identity plus whatever share keys it holds, and the session binding has always handed Postgres both. A subscription could only ever say the first, so a membership granted to a key reached nobody, and a caller holding keys alone could not take a filter that names a membership at all. This makes the subscription say what the binding already says. Each compiled term now declares which of the caller's values its own SQL reads, and it is seeded from that one and not the other, so an identity-spelled filter keeps comparing one session value and a set-spelled filter is matched against every subject the caller holds. Seeding one from the other is what would admit rows the registered query does not return, which is the failure the split exists to prevent.
The hidden membership subscription the server opens on a client's behalf follows the same rule. It is keyed by table and opened once, so a table watched by an identity term and a set term at once is mirrored under both spellings joined by
ORrather than under whichever term was described first. Its set spelling is the guarded search pg2sqlite emits rather than the obvious substring test, because the plain one reverse translates as a position query and disagrees with the membership on an unset setting, an empty one, and a column value carrying the delimiter. A test pins that predicate against the translator rather than against a literal, so a change to the emitted idiom fails here instead of quietly widening what the mirror admits.Getting here took four fixes in the dependencies, each of which was the real obstacle rather than a detour: the caller stated as a set of subjects and the drain's future declared
Sendin subql, a session setting declared as a delimited set in pg2sqlite, and a membership policy matched against that set in rls2fga. All four are merged and this moves every workspace onto them. The end-to-end proof is a caller who owns nothing and is a member of nothing, holding only the key a team is granted to, receiving that team's rows.One piece is deliberately not here. A replica evaluating a set-spelled query locally needs its own subjects function, and the packing that renders it lives in the server crate while the client depends only on the core one. Spelling it a second time in a second crate is exactly the drift this change works to avoid, so the sharing move comes first and the client registration with it.
Summary by Sourcery
Authorize and synchronize callers according to the identity and subject keys they actually hold, rather than identity alone.
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment:
Documentation:
Tests: