Guard role grant emission behind WillSyncResourceType - #31
Conversation
userResourceType.Grants() emits role grants as a sync optimization
(the user API response already includes role membership), but did so
unconditionally. When a customer's sync filter excludes roles, the
connector still emitted grants referencing an unsynced resource type.
Gate the emission on cli.ConnectorOpts.WillSyncResourceType("role"),
threaded from NewLambdaConnector through the Connector struct into the
user builder. Also mark the user resource type with
SkipEntitlementsAndGrants when roles aren't synced, since the user
builder has no entitlements/grants of its own otherwise.
Follows the pattern from ConductorOne/baton-linear#55.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Connector PR Review: Guard role grant emission behind WillSyncResourceTypeBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryThe new commit removes the Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
newUserBuilder already annotates the user resource type SkipEntitlementsAndGrants when roles are excluded from the sync filter, and the SDK honors that before ever calling Grants (shouldSkipGrants -> shouldSkipEntitlementsAndGrants in pkg/sync/syncer.go), so the early return was unreachable during a sync and duplicated the condition. With the guard gone the syncRoles struct field was write-only, so it is removed too; the constructor parameter stays, since it is what selects the annotation. Dropped the subtest that existed solely to pin the early return -- TestNewUserBuilder_ResourceTypeAnnotations already covers the annotation, which is the real mechanism.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Grants emits the cross-type role grant. There is no syncRoles guard here: | ||
| // when roles aren't being synced, newUserBuilder annotates the user resource | ||
| // type SkipEntitlementsAndGrants and the SDK never calls Grants() at all | ||
| // (shouldSkipGrants -> shouldSkipEntitlementsAndGrants in the SDK's | ||
| // pkg/sync/syncer.go), so a guard would be unreachable. |
There was a problem hiding this comment.
🟡 Suggestion: The claim holds for the syncer path — I verified shouldSkipGrants → shouldSkipEntitlementsAndGrants in baton-sdk v0.20.2 (pkg/sync/syncer.go:1263 and :2093) does gate on the resource type annotation — but it does not hold for the ListGrants gRPC entrypoint: pkg/connectorbuilder/resource_syncer.go:303 looks up the syncer by resource type and calls rb.Grants(...) unconditionally, with no annotation check. Any direct ListGrants call for a user resource will now hit GetUserByID and emit a grant referencing the unsynced role type. Keeping the cheap syncRoles guard was defense-in-depth for exactly that; consider restoring it rather than coupling correctness to SDK-internal syncer behavior.
| // The user builder has no entitlements or grants of its own -- its only | ||
| // Grants() output is the cross-type role grant gated below. When roles | ||
| // aren't being synced, skip entitlement/grant discovery for users entirely. |
There was a problem hiding this comment.
🟡 Suggestion: This comment is now stale — it says the role grant is "gated below", but this commit removed the if !o.syncRoles gate from Grants(). Reword to say the gating is done entirely by this annotation (or restore the guard and keep the wording).
| // TestUserGrants_RoleSyncFilter covers ConductorOne/baton-linear#55: the user | ||
| // builder emits role grants as a sync optimization, but must not do so when | ||
| // the customer's sync filter excludes the role resource type. | ||
| func TestUserGrants_RoleSyncFilter(t *testing.T) { |
There was a problem hiding this comment.
🟡 Suggestion: Two staleness nits after the guard removal. The doc comment references ConductorOne/baton-linear#55, which is a different connector's issue — it should point at the jumpcloud issue (or drop the reference). And TestUserGrants_RoleSyncFilter now contains only the positive case, so its name and "must not do so when the customer's sync filter excludes the role resource type" no longer describe what it asserts; the filter behavior lives in TestNewUserBuilder_ResourceTypeAnnotations. Renaming to something like TestUserGrants_RoleGrantEmitted would keep the intent clear.
2 participants
Summary
userResourceType.Grants()(pkg/connector/users.go) emitsrolegrants as a sync optimization since the JumpCloud user API response already includes the admin user's role — but it did so unconditionally, even when a customer's sync filter excludes theroleresource type.cli.ConnectorOpts.WillSyncResourceType("role"), threaded fromNewLambdaConnector→Connector.syncRoles→newUserBuilder.NewLambdaConnectoralready received*cli.ConnectorOptsbut never used it.SkipEntitlementsAndGrants(the user builder has no entitlements/grants of its own besides the gated role grants).Test plan
go build ./...go test ./...pkg/connector/users_test.go:TestUserGrants_RoleSyncFilter— asserts no grants (and no API call) when roles are filtered out, and a correct role grant when roles are synced (via anhttptest.Servermocking the JumpCloud user-get endpoint).TestNewUserBuilder_ResourceTypeAnnotations— assertsSkipEntitlementsAndGrantsis set on the user resource type only when roles are filtered out.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com