Restore http cache for subuser calls via cache-key header - #36
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // onBehalfOfOpts scopes a request to a subuser when onBehalfOf is set. The | ||
| // on-behalf-of header is folded into the cache key via WithCacheKeyHeaders on | ||
| // the client, so parent- and subuser-scoped responses stay distinct in the cache. | ||
| func onBehalfOfOpts(onBehalfOf OnBehalfOf) []uhttp.RequestOption { | ||
| opts := []uhttp.RequestOption{uhttp.WithNoCache()} | ||
| if onBehalfOf != "" { | ||
| opts = append(opts, uhttp.WithHeader(OnBehalfOfHeaderName, string(onBehalfOf))) | ||
| if onBehalfOf == "" { | ||
| return nil | ||
| } | ||
| return opts | ||
| return []uhttp.RequestOption{uhttp.WithHeader(OnBehalfOfHeaderName, string(onBehalfOf))} | ||
| } |
There was a problem hiding this comment.
🟠 Bug: Dropping WithNoCache() here also enables caching on GetSpecificTeammate, which scopeBuilder.Grant/Revoke use as the read half of a read-modify-write (helper.go:63 → scopes.go:81,101). uhttp's cache is per-BaseHttpClient, defaults to in-memory with a 1h TTL, has no write invalidation (Do only Get/Sets on GET), and ClearCaches runs only in Cleanup at end of sync — so in service mode the connector subprocess keeps the entry across provisioning tasks. Grant(B) then Grant(C) on the same teammate reads the pre-grant scopes=[A] from cache and PATCHes [A,C], silently dropping B.
Suggest keeping the request uncached specifically for the provisioning read path — e.g. give GetSpecificTeammate a no-cache variant (or a noCache bool/option param) that Grant/Revoke use, while the sync callers in teammates.go:196,252 keep the cache. The cache-key change itself is correct: NewRequest canonicalizes via req.Header.Set, and CreateCacheKey looks up http.CanonicalHeaderKey("on-behalf-of"), so the keys match.
| } | ||
|
|
||
| uhtppClient, err := uhttp.NewBaseHttpClientWithContext(ctx, httpClient) | ||
| uhtppClient, err := uhttp.NewBaseHttpClientWithContext(ctx, httpClient, uhttp.WithCacheKeyHeaders(OnBehalfOfHeaderName)) |
There was a problem hiding this comment.
🟡 Suggestion: The whole point of this change is that parent- and subuser-scoped responses no longer collide in the cache, but nothing pins that. teammates_test.go uses a fake SendGridClient, so it never exercises the real uhttp client. A test that builds a SendGridClient against an httptest server, calls GetTeammates with "" and then with a subuser, and asserts two distinct upstream hits with distinct bodies would catch a future regression (e.g. someone renaming OnBehalfOfHeaderName without updating the WithCacheKeyHeaders call) that otherwise surfaces as silently wrong sync data.
Connector PR Review: Restore http cache for subuser calls via cache-key headerBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryThe new commits address the prior blocking finding: Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Bump baton-sdk to v0.24.6 and register on-behalf-of as a cache-key header on the shared uhttp client so parent- and subuser-scoped responses no longer collide on the same key. Drops the WithNoCache workaround from onBehalfOfOpts that was needed before the SDK exposed WithCacheKeyHeaders. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Commit 6e7728b fixed the on-behalf-of cache-key collision with WithCacheKeyHeaders and dropped the blanket WithNoCache(), which re-enabled caching for GetSpecificTeammate — including the read half of the read-modify-write in scopeBuilder.Grant/Revoke. That read's Scopes become a full-list SetTeammateScopes PATCH, and uhttp caches GETs for an hour, never invalidates them on a write, and only clears caches at end-of-sync. So two provisioning tasks on the same teammate inside the TTL both compute their new scope list from the same pre-write snapshot, and the second silently drops what the first granted while still reporting success. Split GetSpecificTeammate into a shared private helper with two wrappers: the existing cacheable one for sync-time reads, and GetSpecificTeammateNoCache for reads that feed a write. getTeammateWithFreshOnBehalfOf — reached only from scope Grant/Revoke — uses the latter for both its initial read and its post-rename retry. The WithCacheKeyHeaders fix stays, so subuser-scoped sync reads keep their cache without colliding with parent scope. Tests model the cache (first response replayed, never invalidated) plus the full-replace write, and cover grant accumulation, revoke, and the GrantAlreadyExists short-circuit. All three fail against the cached read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| func (h *SendGridClient) GetSpecificTeammateNoCache(ctx context.Context, username Username, onBehalfOf OnBehalfOf) (*models.TeammateScope, error) { | ||
| return h.getSpecificTeammate(ctx, username, onBehalfOf, uhttp.WithNoCache()) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: uhttp.WithNoCache() only suppresses the cache read — BaseHttpClient.Do still writes every 200-OK GET into the cache unconditionally (vendor/.../uhttp/wrapper.go:592-596 is not gated on Cache-Control), and Cache-Control is not part of CreateCacheKey. So this "no-cache" read stores the pre-write scope list under exactly the key GetSpecificTeammate reads from, and since caches are only cleared at end-of-sync (TTL 1h), a sync running later in the same process (teammates.go:252) emits scope grants missing whatever the grant task just wrote. Simplest fixes: keep WithNoCache() on the specific-teammate endpoint entirely (the PR's goal is caching the subuser calls), or re-issue the no-cache GET after a successful SetTeammateScopes so the write-through refreshes the entry to live state. (medium confidence — depends on provisioning and sync sharing a process)
| } | ||
|
|
||
| teammate, err = client.GetSpecificTeammate(ctx, sgclient.Username(username), sgclient.OnBehalfOf(freshOnBehalfOf)) | ||
| teammate, err = client.GetSpecificTeammateNoCache(ctx, sgclient.Username(username), sgclient.OnBehalfOf(freshOnBehalfOf)) |
There was a problem hiding this comment.
🟡 Suggestion: this rename-recovery retry now re-resolves through a cached path. resolveOnBehalfOfByParentID → GetSubuserUsernameByID → GetSubusers, and GetSubusers is a plain GET that this PR made cacheable, so if the subuser list was already fetched in this process within the 1h TTL the "fresh" lookup returns the same stale username that just 404'd and the retry is a no-op. teammateBuilder.Delete (teammates.go:285) has the same exposure, and there a stale on-behalf-of produces a 404 that is swallowed as "already deleted" (teammates.go:292-295), i.e. a silently no-op delete. Consider a no-cache variant of the subuser lookup for these provisioning paths. The comments at helper.go:57-58 and :91-92 still describe these as "plain, uncached" calls and should be updated either way.
2 participants
Summary
baton-sdkfrom v0.24.4 to v0.24.6 to pick upuhttp.WithCacheKeyHeaders.on-behalf-ofas a cache-key header on the shared uhttp client so parent- and subuser-scoped responses key separately in the cache.WithNoCache()workaround fromonBehalfOfOptsthat was needed before the SDK exposed per-header cache-key control (originally added in CXP-860 Remove http-cache for requests with additional headers #34).Test plan
go build ./...go test ./...🤖 Generated with Claude Code