Guard role grant emission behind WillSyncResourceType - #23
Conversation
Connector PR Review: Guard role grant emission behind WillSyncResourceTypeBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit only drops the unreachable comma-ok fallback on Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
userBuilder.Grants emits role grants on roleBuilder's behalf (roleBuilder.Grants
is a no-op delegate) as a sync optimization, since the user API response
already includes the workspace role. This emission was unconditional, so a
customer whose sync filter excluded "role" would still get grants referencing
a resource type that was never synced.
Gate the emission on cli.ConnectorOpts.WillSyncResourceType("role"), following
the pattern in ConductorOne/baton-linear#55. No group-grant cross-emission
exists in this connector -- groupBuilder.Grants computes its own membership
grants independently via GetGroup, so only role needed gating.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…heck Replace the in-Grants() early-return guard (behind syncRoles) with the approach from ConductorOne/baton-linear#55: move the gate into userBuilder.ResourceType(), which now returns a clone of userResourceType carrying SkipEntitlements when "role" is being synced (matching the prior static behavior) or SkipEntitlementsAndGrants when it is not. The SDK decides whether to call Entitlements()/Grants() for a resource type from these resource-type-level annotations (verified against baton-sdk@v0.20.2's pkg/sync/syncer.go shouldSkipEntitlements / shouldSkipEntitlementsAndGrants), so gating there means Grants() is simply never invoked when "role" is excluded from the sync -- no runtime check needed inside Grants() itself, which goes back to its original unconditional body. userResourceType's static Annotations field is removed from resource_types.go since annotations are now computed dynamically per syncRoles. ResourceType() always returns a proto.Clone of the package var so repeated/differing calls can't leak annotations onto the shared pointer; this is covered by a new test alongside tests for the annotation behavior and Grants()'s now-unconditional emission. Metadata regeneration: ran `capabilities`/`config` against the built binary and diffed against baton_capabilities.json/config_schema.json -- byte-identical after JSON canonicalization, so neither file nor docs/connector.mdx needed updates. This matches expectations: with no sync filter (the case `capabilities`/`config` run under), WillSyncResourceType defaults true, so syncRoles is true and the annotation is SkipEntitlements -- identical to the previous static value. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
dd4025f to
d653009
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| syncRoles := true | ||
| if connectorOpts != nil { | ||
| syncRoles = connectorOpts.WillSyncResourceType(roleResourceType.Id) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: cli.ConnectorOpts.SyncResourceTypeIDs is only ever populated from the local --sync-resource-types flag/config (see pkg/cli/commands.go:464,670 in the SDK). In the C1 service path, the authoritative filter is the task's GetSyncFull().GetSyncResourceTypeIds(), which takes precedence over local config in pkg/tasks/c1api/full_sync.go:243-251 and is never surfaced through ConnectorOpts. So when C1/the UI narrows the sync to exclude role, WillSyncResourceType("role") still returns true here and the dangling role grant is emitted anyway — the gate only covers locally-configured filters. Worth noting in the PR description (or raising with the SDK) so the coverage limit is explicit rather than implied.
proto.Clone of a *v2.ResourceType always yields a *v2.ResourceType, so the type assertion cannot fail. Use the plain assertion, matching the other connectors.
2 participants
Summary
userBuilder.Grantsemits a role grant onroleBuilder's behalf (a permanent no-op delegate, seerole.go) as a sync optimization — the Notion user API response already includes the workspace role.roleresource type,roleBuildernever lists role resources, butuserBuilderstill emitted a grant referencingroleResourceType— a dangling reference to a type that was never synced.cli.ConnectorOpts.WillSyncResourceType("role"), following ConductorOne/baton-linear#55.WillSyncResourceTypereturnstruewhen the type is explicitly selected or when no filter is configured at all, so default/local runs are unaffected.ResourceType()annotation level, not as a runtime check insideGrants().userBuilder.ResourceType()now returns a clone ofuserResourceTypecarryingSkipEntitlementswhenroleis being synced (user has no entitlements of its own, unchanged from before), orSkipEntitlementsAndGrantswhen it isn't — which tells the SDK to skip callingEntitlements/Grantsforuserresources entirely, soGrants()itself stays unconditional and simple.groupwas also named as a target to gate, but it doesn't apply here —groupBuilder.Grantscomputes its own membership grants independently viaGetGroup, it's not a delegate ofuserBuilder. Onlyrolehas a cross-type emission in this connector.baton_capabilities.json/config_schema.json/docs/connector.mdxare unchanged — verified by rebuilding the binary and diffing: with no sync filter (the casecapabilities/configrun under),WillSyncResourceTypereturnstruefor everything, so the annotation computes to the sameSkipEntitlementsthat was already statically present.Test plan
go build ./...go vet ./...go test ./...pkg/connector/user_test.go:TestUserBuilder_Grants_AlwaysEmitsRoleGrant—Grants()emits the role grant unconditionally regardless ofsyncRoles(gating moved out ofGrants).TestUserBuilder_ResourceType_Annotations—syncRoles=true→SkipEntitlementsonly;syncRoles=false→SkipEntitlementsAndGrantsonly.TestUserBuilder_ResourceType_DoesNotMutatePackageVar— repeated calls with differentsyncRolesnever leak annotations onto the shared package-leveluserResourceTypevar.TestNewUserBuilder_SyncRolesWiring— constructor wiring (nil opts, empty filter, filter including/excluding "role") unchanged.🤖 Generated with Claude Code