From 6e7728b2d8bbf286d99b60c41449c3b4d5be4071 Mon Sep 17 00:00:00 2001 From: subencheng Date: Thu, 20 Aug 2026 14:33:49 -0700 Subject: [PATCH 1/2] Restore http cache for subuser calls via cache-key header 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) --- pkg/connector/client/client.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/connector/client/client.go b/pkg/connector/client/client.go index 10a722a9..b2143150 100644 --- a/pkg/connector/client/client.go +++ b/pkg/connector/client/client.go @@ -92,7 +92,7 @@ func NewClient(ctx context.Context, baseUrl, apiKey string) (*SendGridClient, er return nil, err } - uhtppClient, err := uhttp.NewBaseHttpClientWithContext(ctx, httpClient) + uhtppClient, err := uhttp.NewBaseHttpClientWithContext(ctx, httpClient, uhttp.WithCacheKeyHeaders(OnBehalfOfHeaderName)) if err != nil { return nil, err } @@ -405,14 +405,14 @@ func getTokenValue(pToken *pagination.Token) (int, error) { return value, nil } -// onBehalfOfOpts always disables caching: uhttp's cache key ignores the -// on-behalf-of header, so parent- and subuser-scoped calls would otherwise collide. +// 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))} } func (h *SendGridClient) doRequest( From 90ccfff88ad9c1f1c91c2dabd4667cc45a9c0902 Mon Sep 17 00:00:00 2001 From: subencheng Date: Mon, 24 Aug 2026 14:07:54 -0700 Subject: [PATCH 2/2] Bypass http cache for the scope grant/revoke read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- pkg/connector/client/client.go | 27 ++++++- pkg/connector/connector.go | 3 + pkg/connector/helper.go | 9 ++- pkg/connector/scopes_test.go | 139 +++++++++++++++++++++++++++++++++ 4 files changed, 174 insertions(+), 4 deletions(-) create mode 100644 pkg/connector/scopes_test.go diff --git a/pkg/connector/client/client.go b/pkg/connector/client/client.go index b2143150..5123e45b 100644 --- a/pkg/connector/client/client.go +++ b/pkg/connector/client/client.go @@ -140,8 +140,31 @@ func (h *SendGridClient) DeleteTeammate(ctx context.Context, username Username, // GetSpecificTeammate Retrieve a specific teammate with scopes. // onBehalfOf, when non-empty, scopes the lookup to a subuser. Pass "" to look up -// the teammate at parent scope. +// the teammate at parent scope. The response is http-cached, so this is for +// read-only callers (sync); reads whose result feeds a write must use +// GetSpecificTeammateNoCache. func (h *SendGridClient) GetSpecificTeammate(ctx context.Context, username Username, onBehalfOf OnBehalfOf) (*models.TeammateScope, error) { + return h.getSpecificTeammate(ctx, username, onBehalfOf) +} + +// GetSpecificTeammateNoCache is GetSpecificTeammate with the http cache +// bypassed, for the read half of the read-modify-write in scope Grant/Revoke. +// SetTeammateScopes replaces a teammate's entire scope list, so that read must +// see the live list: uhttp caches GETs for an hour, never invalidates them on a +// write, and only clears caches at end-of-sync — so a cached read would let +// back-to-back provisioning tasks on the same teammate each build their new +// scope list from a pre-write snapshot, silently dropping whatever the previous +// task granted. +func (h *SendGridClient) GetSpecificTeammateNoCache(ctx context.Context, username Username, onBehalfOf OnBehalfOf) (*models.TeammateScope, error) { + return h.getSpecificTeammate(ctx, username, onBehalfOf, uhttp.WithNoCache()) +} + +func (h *SendGridClient) getSpecificTeammate( + ctx context.Context, + username Username, + onBehalfOf OnBehalfOf, + extraOpts ...uhttp.RequestOption, +) (*models.TeammateScope, error) { uri := h.getUrl(fmt.Sprintf(SpecificTeammateEndpoint, username)) var requestResponse models.TeammateScope @@ -151,7 +174,7 @@ func (h *SendGridClient) GetSpecificTeammate(ctx context.Context, username Usern uri, &requestResponse, nil, - onBehalfOfOpts(onBehalfOf)..., + append(onBehalfOfOpts(onBehalfOf), extraOpts...)..., ) if err != nil { return nil, err diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 9969bbc5..4ea70e6b 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -35,6 +35,9 @@ type SendGridClient interface { InviteTeammate(ctx context.Context, email string, scopes []string, isAdmin bool) (*models.TeammateInvitation, error) GetSpecificTeammate(ctx context.Context, username client.Username, onBehalfOf client.OnBehalfOf) (*models.TeammateScope, error) + // GetSpecificTeammateNoCache bypasses the http cache; required for reads + // that feed a write, since SetTeammateScopes replaces the whole scope list. + GetSpecificTeammateNoCache(ctx context.Context, username client.Username, onBehalfOf client.OnBehalfOf) (*models.TeammateScope, error) GetTeammates(ctx context.Context, pToken *pagination.Token, onBehalfOf client.OnBehalfOf) ([]*models.Teammate, string, error) DeleteTeammate(ctx context.Context, username client.Username, onBehalfOf client.OnBehalfOf) error GetTeammatesSubAccess(ctx context.Context, username client.Username, pToken *pagination.Token, onBehalfOf client.OnBehalfOf) ([]*models.TeammateSubuser, string, error) diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index d87fcdc6..f90b67b7 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -59,8 +59,13 @@ func teammateOnBehalfOf(ctx context.Context, client SendGridClient, resource *v2 // time — not worth caching. Returns the teammate and the on-behalf-of value // that worked, so callers can reuse it for follow-up calls (e.g. // SetTeammateScopes). +// +// The teammate read itself deliberately bypasses the http cache: every caller +// is a provisioning path that turns the returned Scopes into a full-list +// SetTeammateScopes write, and a cached read would make consecutive tasks on +// the same teammate overwrite each other's scopes. func getTeammateWithFreshOnBehalfOf(ctx context.Context, client SendGridClient, principal *v2.Resource, username, onBehalfOf string) (*models.TeammateScope, string, error) { - teammate, err := client.GetSpecificTeammate(ctx, sgclient.Username(username), sgclient.OnBehalfOf(onBehalfOf)) + teammate, err := client.GetSpecificTeammateNoCache(ctx, sgclient.Username(username), sgclient.OnBehalfOf(onBehalfOf)) if err == nil || onBehalfOf == "" || status.Code(err) != codes.NotFound { return teammate, onBehalfOf, err } @@ -72,7 +77,7 @@ func getTeammateWithFreshOnBehalfOf(ctx context.Context, client SendGridClient, return nil, onBehalfOf, err } - teammate, err = client.GetSpecificTeammate(ctx, sgclient.Username(username), sgclient.OnBehalfOf(freshOnBehalfOf)) + teammate, err = client.GetSpecificTeammateNoCache(ctx, sgclient.Username(username), sgclient.OnBehalfOf(freshOnBehalfOf)) return teammate, freshOnBehalfOf, err } diff --git a/pkg/connector/scopes_test.go b/pkg/connector/scopes_test.go new file mode 100644 index 00000000..70930bd0 --- /dev/null +++ b/pkg/connector/scopes_test.go @@ -0,0 +1,139 @@ +package connector + +import ( + "context" + "slices" + "testing" + + sgclient "github.com/conductorone/baton-sendgrid/pkg/connector/client" + "github.com/conductorone/baton-sendgrid/pkg/connector/models" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/stretchr/testify/require" +) + +// scopeStateFake models a teammate's scopes in SendGrid together with the http +// cache sitting in front of the read: GetSpecificTeammate replays the first +// response it ever produced (uhttp caches GETs for an hour and never +// invalidates them on a write), while GetSpecificTeammateNoCache always +// reports live state. SetTeammateScopes replaces the entire list, as the real +// PATCH does — so a provisioning read served from cache silently drops +// whatever the previous task wrote. +type scopeStateFake struct { + fakeSendGridClient + + live []string + cached []string + cachedFilled bool + // cachedReads counts reads that went through the cacheable variant, which + // no provisioning path may use. + cachedReads int +} + +func (f *scopeStateFake) GetSpecificTeammate(_ context.Context, username sgclient.Username, _ sgclient.OnBehalfOf) (*models.TeammateScope, error) { + f.cachedReads++ + if !f.cachedFilled { + f.cached = slices.Clone(f.live) + f.cachedFilled = true + } + + return &models.TeammateScope{ + Teammate: models.Teammate{Username: string(username)}, + Scopes: slices.Clone(f.cached), + }, nil +} + +func (f *scopeStateFake) GetSpecificTeammateNoCache(_ context.Context, username sgclient.Username, _ sgclient.OnBehalfOf) (*models.TeammateScope, error) { + return &models.TeammateScope{ + Teammate: models.Teammate{Username: string(username)}, + Scopes: slices.Clone(f.live), + }, nil +} + +func (f *scopeStateFake) SetTeammateScopes(_ context.Context, _ sgclient.Username, scopes []string, _ bool, _ sgclient.OnBehalfOf) error { + f.live = slices.Clone(scopes) + return nil +} + +func scopeEntitlementFor(t *testing.T, scope string) *v2.Entitlement { + t.Helper() + + scopeRs, err := scopeResource(Scope(scope)) + require.NoError(t, err) + + return &v2.Entitlement{Resource: scopeRs, Slug: assignedEntitlement} +} + +func teammatePrincipal(t *testing.T) *v2.Resource { + t.Helper() + + principal, err := teammateResource( + &models.Teammate{Username: "alice@example.com", Email: "alice@example.com"}, + nil, + "", + ) + require.NoError(t, err) + + return principal +} + +// Two grants for the same teammate arriving back-to-back: the second must read +// the scope list the first one wrote, not a pre-write snapshot, or it hands +// SetTeammateScopes a full list that's missing the first grant. +func TestScopeBuilder_Grant_ConsecutiveGrantsDoNotOverwriteEachOther(t *testing.T) { + ctx := context.Background() + client := &scopeStateFake{live: []string{"alerts.read", "mail.send"}} + principal := teammatePrincipal(t) + sb := newScopeBuilder(client) + + _, _, err := sb.Grant(ctx, principal, scopeEntitlementFor(t, "api_keys.read")) + require.NoError(t, err) + + _, _, err = sb.Grant(ctx, principal, scopeEntitlementFor(t, "billing.read")) + require.NoError(t, err) + + require.ElementsMatch(t, []string{"alerts.read", "mail.send", "api_keys.read", "billing.read"}, client.live) + require.Zero(t, client.cachedReads, "the read feeding SetTeammateScopes must bypass the http cache") +} + +// Revoke reads the same way: it removes one scope from the live list, so a +// cached read would resurrect scopes revoked by an earlier task and drop ones +// granted by it. +func TestScopeBuilder_Revoke_ReadsLiveScopes(t *testing.T) { + ctx := context.Background() + client := &scopeStateFake{live: []string{"alerts.read", "mail.send"}} + principal := teammatePrincipal(t) + sb := newScopeBuilder(client) + + _, _, err := sb.Grant(ctx, principal, scopeEntitlementFor(t, "api_keys.read")) + require.NoError(t, err) + + _, err = sb.Revoke(ctx, &v2.Grant{ + Principal: principal, + Entitlement: scopeEntitlementFor(t, "mail.send"), + }) + require.NoError(t, err) + + require.ElementsMatch(t, []string{"alerts.read", "api_keys.read"}, client.live) + require.Zero(t, client.cachedReads, "the read feeding SetTeammateScopes must bypass the http cache") +} + +// The idempotency short-circuits decide off the same read, so they must also +// see live state: a scope granted by a previous task must be reported as +// already granted rather than re-written. +func TestScopeBuilder_Grant_AlreadyGrantedUsesLiveScopes(t *testing.T) { + ctx := context.Background() + client := &scopeStateFake{live: []string{"alerts.read"}} + principal := teammatePrincipal(t) + sb := newScopeBuilder(client) + + _, _, err := sb.Grant(ctx, principal, scopeEntitlementFor(t, "api_keys.read")) + require.NoError(t, err) + + grants, annos, err := sb.Grant(ctx, principal, scopeEntitlementFor(t, "api_keys.read")) + require.NoError(t, err) + require.Empty(t, grants) + require.True(t, annos.Contains(&v2.GrantAlreadyExists{})) + require.ElementsMatch(t, []string{"alerts.read", "api_keys.read"}, client.live) + require.Zero(t, client.cachedReads, "the read feeding SetTeammateScopes must bypass the http cache") +}