diff --git a/pkg/connector/client/client.go b/pkg/connector/client/client.go index 10a722a9..5123e45b 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 } @@ -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( 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") +}