Skip to content

Guard scope grant emission behind WillSyncResourceType #35

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laurenleach merged 1 commit into from
Aug 14, 2026
Merged

Guard scope grant emission behind WillSyncResourceType #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions pkg/connector/connector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,16 @@ type SendGridClient interface {
type Connector struct {
client SendGridClient
ignoreSubusers bool
// skipScopeResourceType reports whether scope is excluded from the sync
// filter. Named for the skip condition so the zero value is safe: main.go
// registers a zero-value Connector{} as the capabilities factory.
skipScopeResourceType bool
}

// ResourceSyncers returns a ResourceSyncerV2 for each resource type that should be synced from the upstream service.
func (d *Connector) ResourceSyncers(ctx context.Context) []connectorbuilder.ResourceSyncerV2 {
return []connectorbuilder.ResourceSyncerV2{
newTeammateBuilder(d.client),
newTeammateBuilder(d.client, d.skipScopeResourceType),
newTeammateInvitationBuilder(d.client),
newScopeBuilder(d.client),
newSubuserBuilder(d.client, d.ignoreSubusers),
Expand Down Expand Up @@ -120,19 +124,20 @@ func (d *Connector) Validate(ctx context.Context) (annotations.Annotations, erro
}

// New returns a new instance of the connector.
func New(ctx context.Context, sgClient SendGridClient, ignoreSubusers bool) (*Connector, error) {
func New(ctx context.Context, sgClient SendGridClient, ignoreSubusers bool, skipScopeResourceType bool) (*Connector, error) {

@github-actions github-actions Bot Aug 7, 2026

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.

🟡 Suggestion: New now takes two adjacent, same-typed bool params (ignoreSubusers, skipScopeResourceType). Callers can silently swap them with no compile error, and this list will keep growing as more resource types get gated. Consider a small options struct (or variadic Opt funcs) so each flag is named at the call site.

if sgClient == nil {
return nil, ErrSendgridClientNotProvided
}

return &Connector{
client: sgClient,
ignoreSubusers: ignoreSubusers,
client: sgClient,
ignoreSubusers: ignoreSubusers,
skipScopeResourceType: skipScopeResourceType,
}, nil
}

// NewLambdaConnector creates a new connector from config for lambda/containerized deployment.
func NewLambdaConnector(ctx context.Context, cfg *config.Sendgrid, _ *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) {
func NewLambdaConnector(ctx context.Context, cfg *config.Sendgrid, opts *cli.ConnectorOpts) (connectorbuilder.ConnectorBuilderV2, []connectorbuilder.Opt, error) {
l := ctxzap.Extract(ctx)

sendGridApiKey := cfg.SendgridApiKey
Expand Down Expand Up @@ -161,7 +166,10 @@ func NewLambdaConnector(ctx context.Context, cfg *config.Sendgrid, _ *cli.Connec
return nil, nil, fmt.Errorf("baton-sendgrid: error creating client: %w", err)
}

cb, err := New(ctx, sendGridClient, sendgridIgnoreSubusers)
// nil opts means no filter, so nothing is skipped.
skipScopeResourceType := opts != nil && !opts.WillSyncResourceType(scopeResourceType.Id)

cb, err := New(ctx, sendGridClient, sendgridIgnoreSubusers, skipScopeResourceType)
if err != nil {
return nil, nil, fmt.Errorf("baton-sendgrid: error creating connector: %w", err)
}
Expand Down
15 changes: 11 additions & 4 deletions pkg/connector/teammates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ const (

type teammateBuilder struct {
client SendGridClient
// skipScopeResourceType reports whether scope is excluded from the sync
// filter. Only the scope emission below is gated: teammates have their own
// entitlements and subuser grants, so the resource-type-level skip
// annotations would suppress real data and are deliberately not used here.
skipScopeResourceType bool
}

func (u *teammateBuilder) ResourceType(ctx context.Context) *v2.ResourceType {
Expand Down Expand Up @@ -241,8 +246,9 @@ func (u *teammateBuilder) Grants(ctx context.Context, resource *v2.Resource, opt
rv = append(rv, grants...)
}

// Scope grants — only on the first (and only) page to avoid duplicate API calls.
if opts.PageToken.Token == "" {
// Scope grants — only on the first (and only) page to avoid duplicate API
// calls, and only when scope is in the sync filter.
if opts.PageToken.Token == "" && !u.skipScopeResourceType {

@github-actions github-actions Bot Aug 7, 2026

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.

🟡 Suggestion: the scope target is now gated, but the other cross-type emission in this same method — GetTeammatesSubAccess + createGrantSubuserFromTeammate (lines 231–247), whose grant principal is a subuser resource — is not. If subuser is excluded from the sync filter (or ignore_subusers is set, where subuserBuilder.List returns nothing), those grants still reference principals that were never synced. Pre-existing for the ignore_subusers path, but the same WillSyncResourceType(subuserResourceType.Id) guard would make this method internally consistent and skip the per-teammate subuser-access call too.

specificTeammate, err := u.client.GetSpecificTeammate(ctx, sgclient.Username(username), sgclient.OnBehalfOf(onBehalfOf))
if err != nil {
return nil, nil, fmt.Errorf("baton-sendgrid: failed to get teammate %s: %w", username, err)
Expand Down Expand Up @@ -293,9 +299,10 @@ func (u *teammateBuilder) Delete(ctx context.Context, resourceId *v2.ResourceId,
return nil, nil
}

func newTeammateBuilder(client SendGridClient) *teammateBuilder {
func newTeammateBuilder(client SendGridClient, skipScopeResourceType bool) *teammateBuilder {
return &teammateBuilder{
client: client,
client: client,
skipScopeResourceType: skipScopeResourceType,
}
}

Expand Down
45 changes: 40 additions & 5 deletions pkg/connector/teammates_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func TestTeammateBuilder_List_RootTeammates(t *testing.T) {
},
}

tb := newTeammateBuilder(client)
tb := newTeammateBuilder(client, false)
resources := drainTeammateList(t, tb, nil, nil)

require.Len(t, resources, 2)
Expand All @@ -222,7 +222,7 @@ func TestTeammateBuilder_List_SubuserTeammates(t *testing.T) {
},
}

tb := newTeammateBuilder(client)
tb := newTeammateBuilder(client, false)

sub1ResourceID, err := rs.NewResourceID(subuserResourceType, 1)
require.NoError(t, err)
Expand Down Expand Up @@ -261,7 +261,7 @@ func TestTeammateBuilder_List_TeammateRestrictedToMultipleSubusers(t *testing.T)
},
}

tb := newTeammateBuilder(client)
tb := newTeammateBuilder(client, false)
session := newFakeSessionStore()

sub1ResourceID, err := rs.NewResourceID(subuserResourceType, 1)
Expand Down Expand Up @@ -298,7 +298,7 @@ func TestTeammateBuilder_Grants_SubuserAccessForbidden(t *testing.T) {
resource, err := teammateResource(&models.Teammate{Username: "local-1", Email: "local-1@example.com"}, sub1ResourceID, "sub1")
require.NoError(t, err)

tb := newTeammateBuilder(client)
tb := newTeammateBuilder(client, false)
grants, results, err := tb.Grants(context.Background(), resource, rs.SyncOpAttrs{PageToken: pagination.Token{}})

require.NoError(t, err, "a 403 from subuser_access must not abort Grants for a subuser-only teammate")
Expand All @@ -325,8 +325,43 @@ func TestTeammateBuilder_Grants_SubuserAccessOtherErrorPropagates(t *testing.T)
resource, err := teammateResource(&models.Teammate{Username: "local-1", Email: "local-1@example.com"}, sub1ResourceID, "sub1")
require.NoError(t, err)

tb := newTeammateBuilder(client)
tb := newTeammateBuilder(client, false)
_, _, err = tb.Grants(context.Background(), resource, rs.SyncOpAttrs{PageToken: pagination.Token{}})

require.Error(t, err, "only PermissionDenied should be tolerated, other errors must still propagate")
}

// scopeCallRecorder records whether the scope lookup was attempted.
type scopeCallRecorder struct {
fakeSendGridClient
called bool
}

func (f *scopeCallRecorder) GetSpecificTeammate(_ context.Context, _ sgclient.Username, _ sgclient.OnBehalfOf) (*models.TeammateScope, error) {
f.called = true
return &models.TeammateScope{Teammate: models.Teammate{Username: "u1"}, Scopes: []string{"mail.send"}}, nil
}

// Scope grants are cross-type. When scope is excluded from the sync filter the
// connector must not even make the per-teammate scope lookup. Subuser grants
// are unaffected: teammates own those.
func TestTeammateBuilder_Grants_SkipScopeResourceType(t *testing.T) {
ctx := context.Background()
res, err := teammateResource(&models.Teammate{Username: "u1", Email: "u1@example.com"}, nil, "")
require.NoError(t, err)

filtered := &scopeCallRecorder{}
tb := newTeammateBuilder(filtered, true)
grants, _, err := tb.Grants(ctx, res, rs.SyncOpAttrs{})
require.NoError(t, err)
require.False(t, filtered.called, "scope lookup must be skipped when scope is filtered out")
for _, g := range grants {
require.NotEqual(t, scopeResourceType.Id, g.GetEntitlement().GetResource().GetId().GetResourceType())
}

inScope := &scopeCallRecorder{}
tb = newTeammateBuilder(inScope, false)
_, _, err = tb.Grants(ctx, res, rs.SyncOpAttrs{})
require.NoError(t, err)
require.True(t, inScope.called, "scope lookup must run when scope is in the sync filter")
Comment on lines +362 to +366

@github-actions github-actions Bot Aug 7, 2026

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.

🟡 Suggestion: the positive branch discards grants and only asserts inScope.called. That passes even if the emission loop stopped producing scope grants entirely (e.g. a regression in the SendGridScopes lookup or scopeResource). Capture the grants and assert at least one has GetEntitlement().GetResource().GetId().GetResourceType() == scopeResourceType.Id — "mail.send" is a real entry in SendGridScopes, so exactly one scope grant is expected here.

}
Loading