[CE-1250] fix: hydrate full Resource before local Grant/Revoke calls - #1096
Conversation
Provisioner.grant/revoke already fetch the fully-hydrated entitlement resource (to check the BatonID external-resource annotation) and the fully-hydrated principal, but then discarded both: the entitlement was passed through with its Resource as an identity-only stub, and the principal was rebuilt from a stale hand-copied field list that predates Profile/Status/CreatedAt on v2.Resource. Connectors whose Grant/Revoke needs Profile data (e.g. a resource's href) never received it via the local baton grant/revoke CLI path or baton-test's grant/revoke lifecycle test, even though the data was already in hand. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func hydrateEntitlementResource(e *v2.Entitlement, resource *v2.Resource) *v2.Entitlement { | ||
| return v2.Entitlement_builder{ | ||
| Resource: resource, | ||
| Id: e.GetId(), | ||
| DisplayName: e.GetDisplayName(), | ||
| Description: e.GetDescription(), | ||
| GrantableTo: e.GetGrantableTo(), | ||
| Annotations: e.GetAnnotations(), | ||
| Purpose: e.GetPurpose(), | ||
| Slug: e.GetSlug(), | ||
| }.Build() | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): this helper is itself a hand-maintained field allowlist — the exact fragility this PR removes from the principal path. It happens to cover all 8 current Entitlement fields, but a future proto field addition silently drops on the local grant/revoke path with nothing to catch it. proto.Clone + the hybrid setter is self-maintaining and matches existing repo usage (e.g. pkg/dotc1z/c1file_store.go:86):
| func hydrateEntitlementResource(e *v2.Entitlement, resource *v2.Resource) *v2.Entitlement { | |
| return v2.Entitlement_builder{ | |
| Resource: resource, | |
| Id: e.GetId(), | |
| DisplayName: e.GetDisplayName(), | |
| Description: e.GetDescription(), | |
| GrantableTo: e.GetGrantableTo(), | |
| Annotations: e.GetAnnotations(), | |
| Purpose: e.GetPurpose(), | |
| Slug: e.GetSlug(), | |
| }.Build() | |
| } | |
| func hydrateEntitlementResource(e *v2.Entitlement, resource *v2.Resource) *v2.Entitlement { | |
| if e == nil { | |
| return nil | |
| } | |
| hydrated, ok := proto.Clone(e).(*v2.Entitlement) | |
| if !ok { | |
| return e | |
| } | |
| hydrated.SetResource(resource) | |
| return hydrated | |
| } |
(requires adding "google.golang.org/protobuf/proto" to the imports)
| _, err = p.connector.Grant(ctx, v2.GrantManagerServiceGrantRequest_builder{ | ||
| Entitlement: entitlement.GetEntitlement(), | ||
| Principal: resource, | ||
| Entitlement: hydrateEntitlementResource(entitlement.GetEntitlement(), entitlementResource.GetResource()), |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the hydration is partial. V3EntitlementToV2 (pkg/dotc1z/engine/pebble/translate_v2.go:513-518) documents that it stubs two things — Resource and grantable_to, which comes back as ResourceType id-only stubs. This fix splices back the resource but leaves GrantableTo stubbed, so a connector that reads entitlement.GrantableTo display names or traits in Grant/Revoke still gets empty values on Pebble while getting whatever the connector emitted on SQLite — an engine-dependent divergence in the connector-visible request. Either hydrate GrantableTo via store.GetResourceType lookups too, or note the remaining stub in the helper comment so the next reader doesn't assume the entitlement is fully hydrated.
There was a problem hiding this comment.
Good catch — confirmed GrantableTo is a separate stub (V3EntitlementToV2 reduces it to ResourceType id-only entries independently of Resource). Went with your second option rather than the first: documented the remaining stub in hydrateEntitlementResource's doc comment instead of also hydrating GrantableTo via store.GetResourceType lookups. Reasoning: this PR's scope is the Profile-hydration gap that motivated it (a connector reading entitlement.Resource.Profile / principal.Profile in Grant/Revoke); hydrating GrantableTo is a distinct gap with its own round-trip cost and no known connector need yet, so I didn't want to fold an unrequested scope expansion into this fix. Happy to file a follow-up if a connector actually hits this. Pushed in afc3e6b.
| entResource := cc.grantReq.GetEntitlement().GetResource() | ||
| require.Equal(t, "Group One", entResource.GetDisplayName(), | ||
| "entitlement.Resource must be the fully hydrated group, not an identity-only stub") | ||
| require.Equal(t, "https://example.com/folders/g1", entResource.GetProfile().GetFields()["href"].GetStringValue(), | ||
| "entitlement.Resource.Profile must survive so a connector's Grant() can read it") |
There was a problem hiding this comment.
🟡 Suggestion: the tests assert the two hydration properties but never assert that the entitlement's own fields survive the rebuild. Since hydrateEntitlementResource reconstructs the entitlement from an explicit field list, a dropped or mistyped field there is silent and no test in this PR would fail. Adding require.Equal(t, "member", ...GetSlug()), "Member" for GetDisplayName(), and v2.Entitlement_PURPOSE_VALUE_ASSIGNMENT for GetPurpose() (the fixture already sets all three) would guard the allowlist cheaply.
There was a problem hiding this comment.
Applied — added assertions for Slug/DisplayName/Purpose on the hydrated entitlement in both Grant and Revoke tests. Pushed in afc3e6b.
General PR Review: fix: hydrate full Resource before local Grant/Revoke callsBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit is test-only: it adds Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Note: this run could not write the machine-readable review-state marker (the review sandbox blocked every available way to emit it), so the next review of this PR will fall back to full mode instead of incremental. |
Hand-enumerating every v2.Entitlement field risked silently dropping a future field added to the proto, with no compiler enforcement. Matches the clone+setter idiom already used elsewhere in this repo (e.g. pkg/sync/expand/topological_merge.go). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| clone := proto.Clone(e).(*v2.Entitlement) | ||
| clone.SetResource(resource) |
There was a problem hiding this comment.
🟡 Suggestion: The clone+setter idiom is the right call here (and matches the repo's existing proto.Clone(x).(*T) convention), but it trades the old builder's nil-tolerance for a panic: for a typed-nil e, proto.Clone returns Type().Zero().Interface(), the assertion succeeds as a nil *v2.Entitlement, and SetResource then dereferences it. Low confidence this is reachable today — both call sites take entitlement.GetEntitlement(), and the SQLite (pkg/dotc1z/entitlements.go:77) and Pebble (adapter_reader.go:43-49) readers both error rather than return a nil entitlement — so this is hardening, not a live bug. A one-line if e == nil { return nil } guard would close it.
There was a problem hiding this comment.
Confirmed and fixed: proto.Clone on a typed-nil *v2.Entitlement type-asserts to a non-nil interface wrapping a nil pointer, and the follow-up SetResource then dereferences it. Added if e == nil { return nil } up front, matching the suggestion on the sibling thread. Agreed this isn't reachable today (both call sites pass entitlement.GetEntitlement(), and both the SQLite and Pebble readers error before ever returning a nil entitlement) — treating this purely as hardening. Pushed in afc3e6b.
…engthen tests Addresses automated review feedback on PR #1096: - proto.Clone on a typed-nil *v2.Entitlement type-asserts to a non-nil interface holding a nil pointer; the follow-up SetResource then dereferences it. Guard e == nil explicitly, matching the human-endorsed suggestion on the thread. Not reachable today (both call sites go through entitlement.GetEntitlement() and both storage readers error before returning a nil entitlement) but cheap to close off. - Document that GrantableTo remains ResourceType id-stubs after this fix (V3EntitlementToV2 stubs it independently of Resource) rather than silently expanding this PR's scope to hydrate it too. - Assert the entitlement's own Slug/DisplayName/Purpose survive hydrateEntitlementResource's rebuild, so a future regression there would fail loudly instead of silently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| func hydrateEntitlementResource(e *v2.Entitlement, resource *v2.Resource) *v2.Entitlement { | ||
| if e == nil { | ||
| return nil | ||
| } | ||
| hydrated, ok := proto.Clone(e).(*v2.Entitlement) | ||
| if !ok { | ||
| return e | ||
| } | ||
| hydrated.SetResource(resource) | ||
| return hydrated | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: neither new branch here is reachable from the two engine tests — e is always non-nil, and proto.Clone on a non-nil *v2.Entitlement always type-asserts successfully, so the !ok fallback is dead. A tiny direct table test on hydrateEntitlementResource (nil e → nil, nil resource → cleared Resource with other fields intact, and non-nil → clone is not the input pointer) would pin the hardening contract that the end-to-end grant/revoke tests can't reach. (medium confidence, non-blocking)
There was a problem hiding this comment.
Fair point — added TestHydrateEntitlementResource, a direct table test covering nil entitlement -> nil, nil resource -> Resource cleared but other fields intact, and non-nil -> independent clone (require.NotSame against the input, plus confirming the original entitlement's Resource is untouched). Pushed in 531f54f.
The end-to-end Grant/Revoke tests never pass a nil entitlement (both call sites derive it from a store read that errors instead of returning nil), so they can't exercise the nil-guard or verify the clone is independent of its input. Pins that contract directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
FeliLucero1
changed the title
5 participants
Summary
Provisioner.grant/revoke(the localbaton grant/baton revokeCLI path — also whatbaton-test run all's grant/revoke lifecycle test drives in CI) already fetch a fully-hydratedv2.Resourcefor both the principal and the entitlement's resource, but discard the hydration before calling the connector:entitlementResourceis fetched (to check theBatonIDexternal-resource annotation) but never spliced back in —entitlement.GetEntitlement()is passed through withResourceas an identity-only stub (on the Pebble engine,V3EntitlementToV2documents this by design; seepkg/dotc1z/engine/pebble/translate_v2.go:508).Id, DisplayName, Annotations, Description, ExternalId, ParentResourceId) that predatesProfile/Status/CreatedAtonv2.Resource. Git history (866f6618,#764) shows this list has always been an incrementally-patched allowlist (e.g.ParentResourceIdwas added back in#764after initially being stripped) —Profilewas simply never revisited, not deliberately excluded.Net effect: a connector's
Grant/Revokeimplementation never receivesProfiledata for either the principal or the entitlement's resource via this path — even when the store already has it — because it's thrown away one line before the RPC call. Regression tests below fail against the pre-fix code on both storage engines (SQLite and Pebble), confirming this isn't Pebble-specific.Scope note
This fixes the local/CLI provisioning path (
pkg/provisioner/provisioner.go), which is the only Grant/Revoke call site in this repo with access to a local store. The platform-driven production path (pkg/tasks/c1api/grant.go/revoke.go) builds its request from an incoming task payload the C1 platform already constructed server-side — it never touches a local store, so there's nothing to hydrate here; if a similar gap exists there, the fix is out of this repo's reach.pkg/connectorbuilder/resource_provisioner.go(the connector-side gRPC handler) was also checked and ruled out as a fix site: it's a pure forwarding handler with no store reference in either topology, so it architecturally cannot hydrate anything itself.Edge case: resource not in store
No new failure mode is introduced.
entitlementResourcewas already fetched unconditionally before this change (for theBatonIDcheck), so a missing/deleted resource already hard-errorsgrant()/revoke()today. This PR only reuses data that was already a hard requirement.Test plan
go build ./...go vet ./pkg/provisioner/.../golangci-lint run ./pkg/provisioner/...— cleanpkg/provisioner/provisioner_test.go:TestProvisionerGrantHydratesResources/TestProvisionerRevokeHydratesResources, each run against both SQLite and Pebble engines via a real.c1zstore (not a mock) — write a group resource + user resource withProfiledata, an entitlement, and a grant, then assert the connector-bound request carries the full principal and entitlement resource, Profile included.git stashbefore committing) and pass after.go test ./pkg/provisioner/... ./pkg/tasks/local/... ./pkg/connectorbuilder/...— all pass.🤖 Generated with Claude Code