UpdatePermission renames instead of updating metadata (and drops the metadata it is given)
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 344
- Forks
- 47
- Avg merge
- 4d 4h
- Merged PRs (30d)
- 26
Description
What
The UpdatePermission handler (internal/api/v1beta1connect/permission.go) has two problems.
-
It writes
nameandnamespace_namewith no grammar check.CreatePermissionnow runsschema.ValidateCustomPermission, but the update path runs no check at all. It parses the request key into a namespace and a name, then passes them straight to the service. A key that splits into three non-empty parts but breaks the grammar goes through unchecked:resource.Order.get(uppercase)resource.order_item.get(underscore in a part)compute.machine.owner(reserved verb)- any key whose flattened
service_resource_verbis longer than 64 characters
-
The repository
Update(internal/store/postgres/permission_repository.go) only setsname,namespace_name, andupdated_at. It never writesmetadata. So the metadata the handler builds is dropped. The only thing the endpoint actually changes is the identity, which is a rename.
Why it matters
A rename to a value SpiceDB will not accept still succeeds in Postgres. On the next server boot, MigrateSchema merges every permission row into the SpiceDB schema. The bad row fails to compile, so the server will not start until someone fixes the row by hand.
Scope and risk
Low. The reconciler never calls UpdatePermission. Its client interface has only ListPermissions, CreatePermission, and DeletePermission, and a permission is identity only (added or deleted, never updated). So only a direct API caller can hit this. It is not on the GitOps path.
Suggested fix
Two options:
- Validate the key. Resolve the key to a namespace and a name and run
schema.ValidateCustomPermissionbefore the write, the same asCreatePermission. Minimal, but it still allows a rename to a valid key. - Preferred: stop renaming. A permission's identity is fixed once it is created, so the update path should change only metadata. Make the handler and the repository
Updatewrite onlymetadatabyidand never touchnameornamespace_name. This makes the endpoint do what it claims, drops the dependence on the deprecatednameandnamespacefields, and fixes the dropped-metadata bug at the same time.
Context
Found during review of the reconcile permission PRs (#1889 added the shared schema.ValidateCustomPermission; #1892 switched the Permission kind to the key form). It was agreed there to track this as a follow-up rather than widen those PRs.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with internal/api/v1beta1connect/permission.go and compare UpdatePermission with CreatePermission, then trace Update in internal/store/postgres/permission_repository.go. Verify how metadata and identity fields are handled and check the referenced schema.ValidateCustomPermission path. Done means updates preserve permission identity, persist metadata, and reject invalid keys without breaking the existing reconcile path.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- api, backend, database
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 70/100