Open
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #761 +/- ##
==========================================
+ Coverage 60.48% 60.51% +0.02%
==========================================
Files 108 108
Lines 10576 10677 +101
==========================================
+ Hits 6397 6461 +64
- Misses 3609 3642 +33
- Partials 570 574 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
blkt
reviewed
Apr 28, 2026
Comment on lines
+179
to
+183
| source, err := getManagedSource(ctx, querier) | ||
| if err != nil { | ||
| otel.RecordError(span, err) | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
question: if I got it right, this prevents reading claims for entries that are not backed by a managed source, despite them having claims themselves. Why is this necessary?
Member
Author
There was a problem hiding this comment.
Good catch — yes, it's intentional. Just pushed 20c78e2 to put the why in the godoc instead of leaving it implied.
Short version: synced-source entries derive their claims from upstream — git/api/file inherit the source row's claims (copied per entry on every sync), and kubernetes takes them from the toolhive.stacklok.dev/authz-claims annotation per CRD (sync.md §3 — K8s deliberately doesn't inherit). On top of that, every sync overwrites entry claims via ON CONFLICT DO UPDATE SET claims = EXCLUDED.claims, so even if the API let you write claims on a synced entry, the next sync would clobber it. The matching PUT is managed-source-only for the same reason; this GET keeps the symmetry.
/v1/sources/{name}/entries and /v1/registries/{name}/entries surface synced-entry claims for inspection.
Contributor
There was a problem hiding this comment.
I get the behavior is as you suggest, and it makes sense to block the PUT, but why preventing the GET? A user might have manageEntries but not manageSources, in which case it would be impossible for him to read the claims on e.g. a Kubernetes entry.
Member
Author
There was a problem hiding this comment.
Fair point, I guess the assumption I had was all of this was strictly for "managed/published" entries thus the decision. Do we want to allow the GET to work for all entries (no matter the source they come from) and of course as long as you have the necessary claims to see the given entry?
6 tasks
rdimitrov
force-pushed
the
rdimitrov/get-entry-claims
branch
from
a5cbcee to
f0a55c0
Compare
rdimitrov
changed the base branch from
main
to
rdimitrov/deny-empty-resource-claims
rdimitrov
force-pushed
the
rdimitrov/get-entry-claims
branch
4 times, most recently
from
1ef910e to
234fb61
Compare
rdimitrov
added a commit
that referenced
this pull request
Apr 29, 2026
## What this does
When `auth.authz` is configured, an unlabeled resource (no `claims`)
used to be treated as **public** — every authenticated user could see
it. This PR flips that to **default-deny**: no claims = invisible to
anyone who isn't super-admin.
This is how the list endpoints already worked. The single-resource gate
was the odd one out, so the same resource could be invisible in `GET
/v1/sources` but readable via `GET /v1/sources/{name}`. Now both paths
agree.
The bypass cases stay the same:
- Anonymous mode → no gate at all.
- No `auth.authz` configured (`skipAuthz=true`) → no gate at all.
- Super-admin → sees everything.
## What an operator needs to know
If you have `auth.authz` on **and** resources without claims (entries
from a synced Git source where the source row is untagged, an untagged
managed source, etc.), those become invisible to claim-bearing callers
after this lands. Two recovery paths:
- Super-admin tags them: `PUT /v1/entries/{type}/{name}/claims` per
entry, or `PUT /v1/sources/{name}` for sources.
- Tag the source in config (e.g. `claims: {org: "acme"}`); synced
entries inherit it.
`auth.md` §4 has the full upgrade notes.
## Bonus cleanups rolled in
- `claimsContain` now fails closed on unsupported value types
(`nil`/number/nested map/…) — defense in depth in case anything bypasses
`ValidateClaimValues`.
- `ErrNoManagedSource` → 503 (was 500) on `publishEntry` and
`deletePublishedEntry`. Matches `updateEntryClaims` and pairs with the
new `GET` in #761.
- The `s.skipAuthz` short-circuit lives in the gate helpers now, not
duplicated at every callsite. Includes a fix to the skill version
promotion loop, which previously bypassed for skipAuthz/anonymous but
not super-admin (latent bug on `main`) —
`TestGetSkillVersion_SuperAdminBypassesPromotion` covers it.
## Test plan
- [x] `task lint-fix` — clean
- [x] `task gen` + `task docs` — no diffs from generated code
- [x] `go test ./...` — green
- [x] `go test -tags=integration ./internal/authz/...` — green
- [ ] Reviewer: confirm the `internal` managed-source claim choice
(`{org: acme}`) matches the deployment story.
- [ ] Reviewer: check whether any production fixtures rely on "empty =
open" entries from synced sources — those become invisible after this
lands; super-admin or an explicit claim is the migration path.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add `GET /v1/entries/{type}/{name}/claims` so the admin UI (and other
admin tooling) can read the claims attached to a published entry name
without having to walk the full entries list. The endpoint sits in the
existing `manageEntries` role group alongside the matching `PUT`.
The service-layer call returns a non-nil claims map even when the entry
has no claims set, so the JSON response shape (`{"claims": {...}}`)
stays stable for callers. Errors are mapped uniformly: invalid entry
type → 400, missing entry → 404, no managed source configured → 503.
A new `entry.claims.read` audit event covers the read path.
Tests added at three layers — handler-level table tests, db-layer tests
against a real Postgres (including empty-claims, wrong-type, and
no-managed-source cases), and an authz integration test that exercises
the role gate end-to-end (writer + super-admin allowed; admin-without-
manageEntries, no-role, and unauthenticated rejected).
Swagger regenerated via `task docs`.
Run `validateClaimsSubsetBytes` against the entry's stored claims inside `GetEntryClaims` and surface `ErrClaimsInsufficient` as 403 in the handler. Without this, a `manageEntries` caller scoped to one team could read the claim metadata of an entry scoped to a different team even though the matching `PUT` would deny them — that asymmetry contradicts the default-deny visibility rule in `auth.md` §4. `GetEntryClaimsOptions` gains a `JWTClaims` field and `setJWTClaims` setter so the handler can plumb the caller's JWT through `WithJWTClaims`, mirroring how `UpdateEntryClaims` already works. Super-admin still bypasses uniformly via the existing helper. Tests: handler-level case for `ErrClaimsInsufficient` → 403; db-layer cases for cross-team denied / covering caller succeeds; integration sub-test exercising a cross-team writer against a platform-scoped entry. Swagger regenerated for the new 403 response.
Add `TestGetEntryClaims_PassesJWTClaimsToService` so the JWT-bearing
call shape (four mock matchers: ctx + entryType + name + jwtClaims) is
covered. The existing table test uses three matchers and would silently
break if the handler started passing a third option; the new case
locks the contract in place.
Also expand the `getEntryClaims` godoc to call out the authorization
model explicitly: role gate in middleware, JWT-subset check in the
service layer (mirrors the matching PUT), and the anonymous-mode
short-circuit. Note that the nil-claims-to-{} normalisation is dead
code in authz mode (publish forbids empty claims per auth.md §6 and
the gate denies them per §4) — so future readers don't assume it's
load-bearing for the authz path.
The godoc previously said the nil-claims-to-{} normalisation was
"unreachable in practice" under authz — that's wrong for super-admin,
who bypasses the subset check uniformly and would hit that branch
when reading legacy or synced rows that lack claims. Reword to call
out the super-admin path explicitly so the next reader doesn't trust
a stale invariant.
Add `TestGetEntryClaims_SuperAdminBypassesSubsetCheck` at the db
layer: super-admin caller with non-covering JWT claims still reads
the entry. Pairs with the existing cross-team-denied case to lock in
both halves of the bypass contract.
A reviewer asked why this endpoint hides claims for entries that aren't backed by the managed source — the existing godoc said "within the managed source" without explaining the reasoning. Spell it out so the next reader doesn't need to read the matching PUT and the sync writer to figure it out. The constraint follows from the data model. Synced-source entries derive their claims from upstream: git/api/file entries inherit the source row's claims (they're copied per entry on every sync), and kubernetes entries take their claims from the `toolhive.stacklok.dev/authz-claims` annotation per CRD (per sync.md §3, K8s sources deliberately don't inherit). On top of that, the temp-table upsert overwrites entry claims on every sync via `ON CONFLICT DO UPDATE SET claims = EXCLUDED.claims` — even if the API let you write claims on a synced entry, the next sync would clobber the write. The matching PUT handles this by being managed-source-only too. GET keeps that symmetry. The OpenAPI description gets the same clarification so it shows up in the public docs, and points readers at the source/registry list endpoints for synced-entry claims. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rdimitrov
force-pushed
the
rdimitrov/get-entry-claims
branch
from
20c78e2 to
a0b65fa
Compare
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
3 participants
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.
What this does
Adds
GET /v1/entries/{type}/{name}/claimsso admin tools can read the claims on a single entry without walking the whole entries list.Mirrors the existing
PUT.Stacked on #762
Merge #762 first; this PR rebases cleanly onto
mainafter.Authorization
Same gate as the matching
PUT:manageEntriesrole required.manageEntrieswriter with covering JWT{"claims": {...}}manageEntrieswriter with non-covering JWTmanageEntriesroleEmpty claims return
{"claims": {}}, never{"claims": null}— stable JSON shape for clients.Scope: managed source only
Same as
PUT: only entries published viaPOST /v1/entries(the managed source) are in scope. Synced-source entries (git/api/file/kubernetes) get their claims from upstream — the source manifest, or thetoolhive.stacklok.dev/authz-claimsannotation for K8s — and every sync overwrites entry claims viaON CONFLICT DO UPDATE SET claims = EXCLUDED.claims, so any API write would be ephemeral. To inspect synced-entry claims, use/v1/sources/{name}/entriesor/v1/registries/{name}/entries. The godoc on the impl spells this out (20c78e2).Test plan
task lint-fix— cleantask testforinternal/api/v1/...,internal/service/..., and the new authz integration test — greentask docsregenerated, no further diffdocs/thv-registry-api/swagger.yaml)🤖 Generated with Claude Code