Restore http cache for subuser calls via cache-key header

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
 	}
@@ -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
@@ -405,14 +428,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(

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)

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
 }
 

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")
+}