Skip to content

CXP-860 Remove http-cache for requests with additional headers - #34

Merged
JavierCarnelli-ConductorOne merged 2 commits into
mainfrom
fix/cxp-860
Aug 7, 2026
Merged

CXP-860 Remove http-cache for requests with additional headers #34
JavierCarnelli-ConductorOne merged 2 commits into
mainfrom
fix/cxp-860

Conversation

@JavierCarnelli-ConductorOne

Copy link
Copy Markdown
Contributor

uhttp's cache key doesn't include the on-behalf-of header, so every subuser's teammate list request collided with the parent account's cached response, silently suppressing every sub-account-only teammate. Bypass the cache for on-behalf-of requests with uhttp.WithNoCache().

GetTeammatesSubAccess also returns 403 by design for a teammate that only exists inside a subuser (subuser_access only applies to parent-account teammates), which aborted the whole Grants sync once those teammates started being emitted. Tolerate PermissionDenied there the same way listSubuserTeammates already does for List.

Description

  • Bug fix
  • New feature

Useful links:

…rt (CXP-860)

uhttp's cache key doesn't include the on-behalf-of header, so every
subuser's teammate list request collided with the parent account's
cached response, silently suppressing every sub-account-only teammate.
Bypass the cache for on-behalf-of requests with uhttp.WithNoCache().

GetTeammatesSubAccess also returns 403 by design for a teammate that
only exists inside a subuser (subuser_access only applies to
parent-account teammates), which aborted the whole Grants sync once
those teammates started being emitted. Tolerate PermissionDenied there
the same way listSubuserTeammates already does for List.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Aug 7, 2026

Copy link
Copy Markdown

CXP-860

Comment thread pkg/connector/client/client.go
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Connector PR Review: CXP-860 Remove http-cache for requests with additional headers

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 06bc0d9a6ae9.
Review mode: incremental since 569331df
View review run

Review Summary

The new commit rewrites onBehalfOfOpts (pkg/connector/client/client.go:410-416) to attach uhttp.WithNoCache() unconditionally and to append the on-behalf-of header only when a subuser is set. This directly addresses the previous blocking finding: parent-scope calls no longer read from the uhttp cache, so a subuser-scoped GetSpecificTeammate/GetTeammates 200 can no longer be served back to isParentScopeTeammate and silently suppress a teammate. All five on-behalf-of-capable endpoints (DeleteTeammate, GetSpecificTeammate, GetTeammates, GetTeammatesSubAccess, SetTeammateScopes) route through this helper, so the collision path is closed. The full PR diff was re-scanned for security and correctness and no new issues were found; the earlier suggestion about unit coverage for onBehalfOfOpts remains open but is not re-filed here as a new finding.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found — see review comments.

@carolinaroncaglia

Copy link
Copy Markdown
Contributor

Tested this branch against the live SendGrid account — built from 569331d, cache left in its default state (no BATON_DISABLE_HTTP_CACHE, no BATON_HTTP_CACHE_BACKEND).

It works. teammate goes from 7 to 16: the 9 sub-account teammates are emitted with their subuser as parentResourceId, with provisioned_scope=subuser and subuser_username set. Exit 0, grants 421 -> 530. The PermissionDenied tolerance fires exactly 9 times, one per sub-account teammate, so that branch is exercised live and not only in the unit tests. Parent-scope regression is clean — a per-principal grant diff against a v0.1.3 bundle shows the only principals that changed are the 9 new sub-account teammates; the 7 parent-scope teammates and the 2 subusers are byte-identical.

One thing worth considering before this gets approved: the sync went from 0 WARN to 18 WARN. That is 2 per sub-account teammate, and both are expected by design:

  • 9 x 404 from isParentScopeTeammate -> GetSpecificTeammate(username, "") (pkg/connector/teammates.go:190-197, called from List at line 109) — a 404 is the answer "does not exist at parent scope".
  • 9 x 403 from GetTeammatesSubAccess — the exact case this PR now tolerates.

The connector swallows both immediately, but each line carries a full response-header dump (~1.4 KB), and together they are ~89% of the log: 73 lines / 28 KB at info level, versus 61 lines / 12 KB and zero warns on v0.1.3. That works out to roughly 2,000 lines / 2.8 MB per sync at 1,000 sub-account teammates.

Not a defect, and the WARN is emitted by uhttp (uhttp/wrapper.go:513) rather than by the connector — but these are the two call sites where the connector already knows the non-200 is expected, so quieting them there is cheaper now than after the release.

For reviewers: this is a connector-side workaround. The underlying SDK behaviour — uhttp.CreateCacheKey ignoring request headers outside its fixed allowlist — is tracked separately as CE-1056.

@carolinaroncaglia

Copy link
Copy Markdown
Contributor

One more note for reviewers, unrelated to the WARN observation above: this branch also bumps baton-sdk from v0.20.6 to v0.22.0 in go.mod, which is not called out in the description. Two minor versions affect the whole connector rather than just this feature, so it is worth running a full regression pass against the release and not only the sub-account cases.

For the record, the bump does not change the behaviour this PR works around — uhttp.CreateCacheKey is byte-identical in v0.20.6, v0.21.0 and v0.22.0 (same 42 lines, md5 ad748bc9271c4b461b6d69877ab723ff on the extracted function, and the vendored copy on this branch matches). So WithNoCache() is still what does the work here. Tracked in CE-1056.

@JavierCarnelli-ConductorOne
JavierCarnelli-ConductorOne dismissed github-actions[bot]’s stale review August 7, 2026 14:01

Requires re-review. Concern addressed

…XP-860)

uhttp's cache key ignores the on-behalf-of header, so a subuser-scoped
write (no-cache read, unconditional write) could poison the shared
cache entry for a later parent-scope read on the same path+query,
causing isParentScopeTeammate to wrongly conclude a teammate exists at
parent scope and suppress it from the sync.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@JavierCarnelli-ConductorOne
JavierCarnelli-ConductorOne merged commit c185ab8 into main Aug 7, 2026
11 checks passed
@JavierCarnelli-ConductorOne
JavierCarnelli-ConductorOne deleted the fix/cxp-860 branch August 7, 2026 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants