feat(authz): the tenant clamp is what a person may reach, not what their token says - #121
Open
AndrejK666 wants to merge 2 commits into
Conversation
The first of ADR-0018's follow-ups. Organizations were created by the browser calling account-management's createTenant directly, which cannot do this job: an organization needs a tenant *and* an owner, and a client that writes only the first produces one nobody owns and nobody sees. So one server-side operation writes all three things an owner is made of — the tenant, the membership that is the authority for organization access (ADR-0011 §2), and the owner grant the Studio PDP evaluates. There is no transaction across Postgres and account-management, so it is ordered and resumable instead: membership before grant, because an organization its creator can see but not administer is a better failure than one they cannot see at all, and a failure names the organization it created so the same call finishes it. Each write is idempotent, so resuming is safe however far the first attempt got. `membership.source` gains its third value, `creation`, beside `assignment` and `manual` — ADR-0018 §2 wants an owner's member list to show how each person got in, and creating the place is not the same as being put in it. The access-config document — the thing the PDP reads — had three private copies of its shape in this assembly: the directory that writes the owner grant, the identity gear that asks who owns an organization, and the PDP that evaluates it. Writing a fourth for this gear would have been three chances of drift becoming four, so the shape, the read and the write are now one module. The PDP keeps its own deserialization: it also carries roles and privilege expansion and sits on the authorization path, where a refactor is not free. VERIFIED ON A STAND, AND IT FOUND THE BLOCKER A caller whose token names the platform root creates an organization end to end: tenant, membership (owner/creation), grant; repeating with organization_id returns the same organization and leaves the membership count at one. An ordinary person cannot. Same code, same request, and the grant write fails with "tenant not found", because the PDP clamps every request to the subtree of `subject_tenant_id` — the home tenant of the *login* — and an organization just created under the platform root is outside it. The tenant is created and the membership is recorded; the creator then cannot administer what they own. That is the same root cause the whole ADR-0018 line is about, one level deeper than expected: not only administrative rights but the tenant clamp itself is derived from the token rather than from membership. Self-service creation therefore cannot be released to ordinary people until the clamp comes from a person's memberships, which reorders ADR-0018's follow-ups — the clamp has to move first. The operation here is correct and complete; what it needs is a policy that can see what it wrote. Signed-off-by: Andrej Kuchma <Andrej.Kuchma@constructor.tech>
…eir token says Building the create-organization operation turned up the deeper half of ADR-0018: the PDP clamps every request to the subtree of `subject_tenant_id`, the home tenant of the *login*. So a person who created an organization could not then read or administer it — the tenant and the membership were written and the owner grant failed with "tenant not found", because the thing they had just created sat outside their clamp. The clamp is now the union of that tenant and the organizations the person is a member of. Constraints in a response are OR-ed and predicates inside one are AND-ed, so "any of these tenants" needs no new platform concept: the flat arm takes a list, and `InTenantSubtree` carries a single root, so the subtree arms are one per tenant. The token's tenant stays in the union on purpose. This change can then only widen, so nothing that works today stops working — including service accounts, which have a tenant and no memberships. Removing that arm is a separate step, after the things still reading it are gone. The PDP is asked on every request, so the organization list is cached. The first version of that cache was wrong, and the stand found it: creating an organization writes the membership and then the owner grant, and the grant write was authorized against a clamp that had already cached "this person belongs to nothing" — so the creator could not finish creating their own organization until the entry expired. Memberships now carry a generation that every write moves, and a cached answer is good only while that generation holds. The age limit stays as a backstop. A failed membership read is not a denial: the caller keeps the reach they had before memberships were consulted. Failing closed there would make a database hiccup indistinguishable from a revoked membership. VERIFIED ON A STAND An ordinary person — token tenant far from the platform root — creates an organization on the first attempt, reads it back, and holds two of them. Another person's organization answers 404 to them while their own answers 200, so the clamp widened to exactly their memberships and no further. Signed-off-by: Andrej Kuchma <Andrej.Kuchma@constructor.tech>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Contributor
Author
|
Folded into #132, which carries this branch's commits unchanged along with the four pieces that finish the scope. Closing this one in favour of that is safe — nothing here is lost. |
Contributor
Author
|
Correction to the note above: this one is already in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second in order, and it is what makes the first one usable.
Stacked on #118. Both target
feature/create-your-organization, so until #118 merges into that branch this PR shows two commits —feat(organizations)from #118 andfeat(authz), which is the one to read here. Merge #118 first and this diff reduces to the clamp alone.What #118 ran into
The PDP clamps every request to the subtree of
subject_tenant_id— the home tenant of the login. A person who created an organization could not then read or administer it: the tenant and the membership were written, and the owner grant failed withtenant not found, because the thing they had just created sat outside their own clamp.That is the deeper half of ADR-0018. Administrative rights being read off the token was the visible half; the clamp is the other one, and nothing self-service works until it moves.
The change
The clamp is now the union of the token's tenant and the organizations the person is a member of.
No new platform concept was needed. Constraints in a response are OR-ed and predicates inside one are AND-ed, so "any of these tenants" is just a list of constraints: the flat arm takes a list of values, and
InTenantSubtreecarries a single root, so the subtree arms are one per tenant per supported property.The token's tenant stays in the union deliberately. The change can then only widen, so nothing that works today stops working — service accounts included, which have a tenant and no memberships. Removing that arm is a separate step, once the things still reading it are gone.
A failed membership read is not a denial: the caller keeps the reach they had before memberships were consulted. Failing closed there would make a database hiccup indistinguishable from a revoked membership.
The PDP is asked on every request, so the organization list is cached. The first version cached by subject with a TTL — and that broke the very operation this exists to enable:
So the creator could not finish creating their own organization until the entry expired. Ten seconds later the same call succeeded, which is how the cause was isolated.
Memberships now carry a generation that every write moves —
record_membership,remove_membership, andmerge, which repoints them. A cached answer is valid only while that generation holds; the age limit stays as a backstop. One atomic load per request, no database.I am flagging this rather than burying it: a cache in front of an authorization decision is exactly where staleness becomes a security property, and the first version was in the safe direction only by luck.
Verified
On a stand (own Postgres; the shared dev stack untouched), as an ordinary person whose token tenant is nowhere near the platform root:
The clamp widened to exactly their memberships and no further.
Tests: 492/492 with a real Postgres,
clippy --all-targets -- -D warningsandcargo fmt --checkclean. Four new PDP tests: the clamp covers every reachable tenant with one subtree arm per tenant per property; one tenant produces exactly the clamp it always did (this change widens and never narrows); a gear without the hierarchy capability still gets every tenant in the flat arm; a duplicate tenant is not added twice.Still not done
privilege_forstill returnsNone, so the role branch remains unreachable and no privilege is enforced — this changes what a person can reach, not what roles mean. ADR-0011 §7 is still the gate on membership-management UI.