feat(api): add bucket policies and the principal access reads by pyropy · Pull Request #67 · fil-forge/hilt

pyropy · 2026-09-11T07:57:46Z

Adds the bucket policy service and the two principal reads computed from policies. A write rotates the affected principals' key delegations inside the store transaction, so the document commits only once revocations are published.

Policy service Get, Put, Write, Delete, with an Unconditional() option
GET .../principals/{id}/policies and .../access
Affected principals locked around one rotation; 409 on contention, 422 on invalid documents
hilt client policy list|access

🤖 Generated with Claude Code

pyropy · 2026-09-11T12:29:10Z

@codex review

chatgpt-codex-connector · 2026-09-11T12:29:26Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-01T11:18:30.297718Z	`939a8d0`	Manual request

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9b4bd1c26b

ℹ️ 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".

pyropy · 2026-09-16T15:07:02Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06f29c8508

ℹ️ 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".

pyropy · 2026-10-01T09:05:56Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 72cd621124

ℹ️ 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".

pyropy · 2026-10-01T11:14:08Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 939a8d09ba

ℹ️ 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".

Copilot

Copilot review overview

🟡 Changes recommended

PostgreSQL policy and delegation updates can commit independently, leaving authorization inconsistent after a late policy-write failure.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Policy and delegation updates are not atomic · New
Package comment names nonexistent policy package · New

What changed in this PR

Adds bucket-policy business logic, principal policy/access reads, and CLI support.

Changes:

Implements policy CRUD, validation, and delegation rotation.
Adds principal policy/access API and management client methods.
Adds hilt client policy list|access.

File	Description
`pkg/store/principal/principal.go`	Adds principal ID collection helper.
`pkg/store/errors.go`	Centralizes contention detection.
`pkg/fx/api.go`	Wires policy services and routes.
`pkg/client/management/management.go`	Adds principal policy/access requests.
`pkg/client/management/management_test.go`	Tests new client methods.
`pkg/api/types.go`	Defines policy/access response types.
`pkg/api/service/principal/service.go`	Uses shared contention detection.
`pkg/api/service/bucketpolicy/service.go`	Implements policy operations and rotation.
`pkg/api/service/bucketpolicy/service_test.go`	Covers policy service behavior.
`pkg/api/service/bucketpolicy/errors.go`	Defines policy service errors.
`pkg/api/policies.go`	Adds principal read handlers.
`pkg/api/policies_test.go`	Tests policy read routes.
`cmd/client/root.go`	Registers policy commands.
`cmd/client/policy/root.go`	Defines policy command group.
`cmd/client/policy/principal.go`	Implements list/access commands.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

GET, PUT and DELETE /tenants/:tenantId/buckets/:bucketName/policy manage a bucket's policy document. GET and PUT answer with a strong ETag over the canonical encoding, and every write states the version it expects: If-Match with that tag replaces or deletes, If-None-Match: * writes the first policy. A tag that does not match the stored one is 412 and writes nothing. A write carrying neither header, both, or an If-None-Match other than *, is 400; 428 is not used. A document naming a principal the tenant does not have, an action outside the policy vocabulary, or no statement at all is 422. A bucket that does not exist or belongs to another tenant is 404, and so is a bucket with no policy, under its own error name so a caller can tell the two apart. The mappers put that name in the body as code, as the tenant and access-key mappers do. Two reads answer from the same documents. GET /tenants/:tenantId/principals/:userId/policies lists every policy with a statement naming the principal or the wildcard, through the store's index. GET /tenants/:tenantId/principals/:userId/access reports the actions the principal holds on each bucket, omitting the buckets it cannot reach. Both are computed from the store on every call and make no network call. PUT and DELETE hand the store a callback that compares the old and new documents over the tenant's principals, the wildcard expanding to all of them, locks the row of every principal whose actions changed, and rotates the delegations of each of their keys over the bucket: the delegations the key held over the bucket are revoked through the revocation service, in one request for the whole write, and the ones the new actions map to are stored in their place, so the gateway drops what it cached for the key and re-reads the policy on its next request. A principal's first grant on the bucket revokes nothing. The callback runs inside the store's transaction, so the document commits only once the rotation has succeeded and a failed publish leaves the old document and the old delegations in place. The principal rows are held so a key created for one of them meanwhile is either rotated or created from the committed policy. The rotation runs under one 8 s deadline, below the 10 s a share-locked reader of the bucket waits, so a wildcard policy on a tenant with many principals cannot lock the bucket's data path out. The callback lists the tenant's principals itself, under the bucket lock, so a principal created while the write was under way is rotated too. The management client gains the policy calls, which carry and return the ETag, and `hilt client policy` gets get, create, replace, delete, list and access. A create answers 201 and a replace 200, which the RFC leaves open. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The memory principal store now bounds the Lock wait, so the cycle this test pins resolves the way it resolves on Postgres: the policy write waits on the principal the removal holds, gives up with ErrConcurrentChange, and the removal finishes. The test required both to succeed, which only ever held on memory. TestPrincipalStorePostgresLockTimeout pins the opposite for Postgres, so the two backends now agree. What the test is for is unchanged: the two writes must not wedge. Shortening LockWait keeps it at a tenth of a second instead of sitting out the bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Bucket policies are read and written over S3 (GetBucketPolicy, PutBucketPolicy, DeleteBucketPolicy) and reach Hilt as /s3/bucket/policy invocations, so the partner-key routes, their client methods and the CLI commands go. The policy service stays, gains an Unconditional write option for a PutBucketPolicy without a precondition, and still serves the two principal reads. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…l to both finish The removal now strips its policies before it locks the principal, so the two no longer wait on each other. The test goes back to requiring both writes to succeed, which is the contract, and no longer shortens the memory principal store's lock wait to break a cycle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The write validated its document against a principal list read before the store took the bucket, and only the Postgres store checked the named principals again. On memory a principal removed in that gap stayed named in the committed policy, and re-creating the id inherited its access. The store callback now requires every principal the document names to be in the list it re-reads under the bucket lock, so both backends refuse the write as an invalid policy. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

writeError mapped every invalid-argument failure from the store to an invalid policy naming a removed principal. A bucket deleted between the lookup and the write is now reported as a missing bucket when the store returns record-not-found for it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…nd publisher The principal service now revokes and deletes a removed principal's keys in one delegation write, and takes the delegation store and the revocation publisher to do it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The principal routes unescape the parameter, since Echo hands it over as it came and the management client escapes each path segment. The two policy read routes still passed it raw, so a principalId holding "/" was looked up as the literal "a%2Fb" and answered 404. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…outes The management policy routes went with the move to the S3 operations; the constant naming their path stayed and staticcheck reports it unused. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…write's grants On Postgres the add waits on the tenant lock the write holds, so a key created for the principal afterwards reads the committed policy. The callback comment credited the principal's first authorize with waiting on the bucket lock, which a listing across buckets cannot take; the tenant lock is what orders the two. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…icy routes' tenantId The package comment documented a "policy" package that does not exist. The principal policy read routes pass the tenantId through tenantParam like every other tenant route. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

… key created meanwhile waits for it Two Postgres tests pin what the joined transaction gives the policy write. A write cancelled after its rotation ran and its principal lock call returned, before the document is written, leaves the old document with its own grants: the rotation's delegation writes rolled back with it, where they used to commit on their own and leave the stored policy paired with another policy's grants. A key created for an existing principal while the first policy naming it is written, parked at the same point, waits on the principal row the write holds until its commit and is then created from the committed policy, which closes the gap filed as FIL-1389. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…tore's fn renames Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…access reads The principal routes now refuse an id holding "/" before the handler runs, so the access and policies reads answer InvalidPrincipalID for one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The bucket policy service returns plain types; the handlers map them to the PrincipalPolicy and BucketAccess wire structs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ListIDsByTenant replaces the ExternalIDs helper, so a policy write reads only external_id instead of whole principal rows it then discards. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

pyropy added this pull request to stack #62 September 11, 2026 07:57

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from a9b7317 to 7683bf3 Compare September 11, 2026 10:37

pyropy removed this pull request from stack #62 September 11, 2026 10:38

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from 7683bf3 to 9b4bd1c Compare September 11, 2026 10:59

pyropy added this pull request to stack #71 September 11, 2026 10:59

chatgpt-codex-connector Bot reviewed Sep 11, 2026

View reviewed changes

Comment thread pkg/api/service/policy/service.go Outdated

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from 9b4bd1c to 0d70c0a Compare September 11, 2026 12:46

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from 0d70c0a to df66705 Compare September 11, 2026 14:49

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from df66705 to 4041a5f Compare September 14, 2026 10:14

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from 4041a5f to f05025a Compare September 14, 2026 10:40

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from f05025a to cf9d7ab Compare September 15, 2026 15:33

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch 2 times, most recently from 4f27216 to 06f29c8 Compare September 16, 2026 14:49

chatgpt-codex-connector Bot reviewed Sep 16, 2026

View reviewed changes

Comment thread pkg/api/service/bucketpolicy/service.go Outdated

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from 06f29c8 to b4bac7d Compare September 17, 2026 13:34

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from b4bac7d to a18de58 Compare September 21, 2026 11:47

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from 262ed28 to 20f32e2 Compare September 30, 2026 16:23

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from 20f32e2 to 879b80d Compare September 30, 2026 16:55

pyropy force-pushed the srdjan/feat/iam-bucket-policies branch from 879b80d to 5ea3fa3 Compare September 30, 2026 16:58

chatgpt-codex-connector Bot reviewed Oct 1, 2026

View reviewed changes

Comment thread pkg/api/service/bucketpolicy/service.go

chatgpt-codex-connector Bot reviewed Oct 1, 2026

View reviewed changes

Comment thread pkg/api/service/bucketpolicy/service.go Outdated

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Comment thread pkg/api/service/bucketpolicy/service.go

Comment thread pkg/api/service/bucketpolicy/service.go Outdated

alanshaw approved these changes Oct 8, 2026

View reviewed changes

Comment thread pkg/api/service/bucketpolicy/service.go

Comment thread pkg/store/principal/principal.go Outdated

pyropy and others added 17 commits October 8, 2026 19:28

test(api): drop the policy route path left behind by the management r…

ee438de

…outes The management policy routes went with the move to the S3 operations; the constant naming their path stayed and staticcheck reports it unused. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

refactor(api): follow the principal store's WithLock and the policy s…

ba85932

…tore's fn renames Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

refactor(api): follow the access key principalId pointer

1b9ec3e

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

refactor(api): define the policy wire types in types.go

4e766cb

The bucket policy service returns plain types; the handlers map them to the PrincipalPolicy and BucketAccess wire structs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

refactor(store): list a tenant's principal ids without the records

ef2b67b

ListIDsByTenant replaces the ExternalIDs helper, so a policy write reads only external_id instead of whole principal rows it then discards. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Conversation

pyropy commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pyropy commented Sep 11, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

pyropy commented Sep 16, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

pyropy commented Oct 1, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

pyropy commented Oct 1, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pyropy commented Sep 11, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 11, 2026 •

edited

Loading