Guard role, source, warehouse, function, space grant emission behind WillSyncResourceType - #20
Guard role, source, warehouse, function, space grant emission behind WillSyncResourceType#20laurenleach wants to merge 5 commits into
Conversation
Connector PR Review: Guard role, source, warehouse, function, space grant emission behind WillSyncResourceTypeBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe full PR diff was scanned for security and correctness; the new commit ( Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsReviewed head SHA |
| func newGroupBuilder(c *client.Client) *groupBuilder { | ||
| return &groupBuilder{client: c} | ||
| func newGroupBuilder(c *client.Client, cliOpts *cli.ConnectorOpts) *groupBuilder { | ||
| return &groupBuilder{client: c, cliOpts: cliOpts} |
There was a problem hiding this comment.
should this method get only the syncRoles boolean value?
|
|
||
| func newUserBuilder(c *client.Client) *userBuilder { | ||
| return &userBuilder{client: c} | ||
| func newUserBuilder(c *client.Client, cliOpts *cli.ConnectorOpts) *userBuilder { |
There was a problem hiding this comment.
same here, why we need the entire connector opts struct?
…WillSyncResourceType userBuilder.Grants() and groupBuilder.Grants() (group-roles phase) emit grants for other resource types (role, source, warehouse, function, space) as a sync optimization, since the user/group API response already includes permission data for those types. This emission was unconditional, so a customer syncing users but excluding e.g. roles would still get grants referencing the unsynced role type -- wasted work and dangling grants. Gate cross-type grant emission behind cli.ConnectorOpts.WillSyncResourceType, mirroring ConductorOne/baton-linear#55. groupBuilder was found to have the identical bug while implementing the userBuilder fix and is included here. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
cdc589e to
f71a48c
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Group-members phase grants target the "user" resource type; only | ||
| // count cross-type (group-roles phase) grants. | ||
| if g.Entitlement.Resource.Id.ResourceType == userResourceType.Id { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this filter never matches. Group-members-phase grants have the group as Entitlement.Resource (the user is the principal), so Entitlement.Resource.Id.ResourceType is "group", never "user". Since driveGroupRolesPhase already returns only phase-2 grants the check is dead code, but the comment is misleading and the helper would silently fail to exclude membership grants if it were ever fed both phases — compare against groupResourceType.Id (or drop the filter and rename the helper).
| targetTypeID := scopeResourceType.Id | ||
| if res.Type == ResourceTypeWorkspace { | ||
| targetTypeID = roleResourceType.Id | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: res.Type == ResourceTypeWorkspace is case-sensitive, while getScopeResourceType normalizes with strings.ToUpper. If the API ever returns "workspace" in another casing, scopeResourceType still resolves to the workspace type but this check fails, so the gate would test "workspace" and the grant would be emitted on the workspace resource instead of role:{id}:member. The mismatch is pre-existing at line 140, but the new gate inherits it — normalizing res.Type once (e.g. strings.EqualFold or an upperType := strings.ToUpper(res.Type) local) would keep both branches consistent. Same applies in groups.go:215.
Addresses review: builders no longer take the whole connector opts struct. newSkipCrossTypeGrants precomputes, once, whether each cross-type target (role, source, warehouse, function, space) is excluded from the sync; builders receive that set and consult it per scope. Also adds the resource-type annotation, built in the constructor: SkipEntitlements normally, escalating to SkipEntitlementsAndGrants when every target is excluded. userResourceType no longer declares SkipEntitlements itself so Update() owns it.
| // newGroupBuilder builds the syncer. Its only grants are cross-type, so when every target | ||
| // resource type is excluded the grants pass is skipped entirely. | ||
| func newGroupBuilder(c *client.Client, skipTargets skipCrossTypeGrants) *groupBuilder { | ||
| rt := proto.Clone(groupResourceType).(*v2.ResourceType) | ||
| annos := annotations.Annotations(rt.GetAnnotations()) | ||
| if skipTargets.all() { | ||
| annos.Update(&v2.SkipEntitlementsAndGrants{}) | ||
| } else { | ||
| annos.Update(&v2.SkipEntitlements{}) | ||
| } |
There was a problem hiding this comment.
🟠 Bug: The comment "Its only grants are cross-type" is not true for groupBuilder — the group-members phase (line 160-181) emits the group's own membership grants with user principals, and the PR description explicitly says that phase was left untouched. With --sync-resource-type user,group, all five entries in crossTypeGrantTargets are excluded so all() is true, the group resource type gets SkipEntitlementsAndGrants, and the SDK's shouldSkipGrants → shouldSkipEntitlementsAndGrants short-circuits the whole grants pass for group resources — silently dropping group membership grants even though user is in the sync set. Suggest always using SkipEntitlements here (the all()/SkipEntitlementsAndGrants shortcut is only sound for userBuilder, whose grants really are all cross-type).
| if skipTargets.all() { | ||
| annos.Update(&v2.SkipEntitlementsAndGrants{}) | ||
| } else { | ||
| annos.Update(&v2.SkipEntitlements{}) |
There was a problem hiding this comment.
🟡 Suggestion: Adding SkipEntitlements to the group resource type buys nothing — groupBuilder.Entitlements already returns an empty slice — while the type now advertises "no entitlements" even though StaticEntitlements still emits the member entitlement that the membership grants and the GrantExpandable annotation (lines 194-202) reference. The SDK's static-entitlement pass ignores this annotation today, so it works, but the advertised contract and the emitted data now disagree. Consider leaving group's annotations alone.
| grants := driveGroupRolesPhase(t, ctx, b, groupResource) | ||
| got := groupRoleGrantTargetTypes(t, grants) | ||
| require.Equal(t, []string{"source"}, got) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Neither test file covers the skipTargets.all() branch (e.g. SyncResourceTypeIDs: []string{"user", "group"}), which is the new code path that swaps SkipEntitlements for SkipEntitlementsAndGrants. A test asserting the annotations returned by ResourceType(ctx) for both branches — and that group membership grants still survive when only user/group are synced — would have caught the group regression flagged above.
groupBuilder.Grants emits a mix: the cross-type role/source/warehouse/ function/space grants *and* the group's own member grants. Annotating the group resource type with SkipEntitlementsAndGrants when every cross-type target was filtered out made the SDK skip the whole grants pass, silently dropping group membership — data no resource-type filter should affect. Always annotate with SkipEntitlements only (the member entitlement comes from StaticEntitlements, so Entitlements is empty either way) and let the existing per-target filtering in Grants handle the cross-type grants. newUserBuilder keeps using skipTargets.all(): all of its grants are cross-type, so skipping the whole pass there is correct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // newGroupBuilder builds the syncer. Its only grants are cross-type, so when every target | ||
| // resource type is excluded the grants pass is skipped entirely. |
There was a problem hiding this comment.
🟡 Suggestion: This doc comment is a leftover copy of newUserBuilder's and now directly contradicts the body it sits above — the latest commit deliberately stopped skipping the grants pass for groups because the group-members phase emits the group's own membership grants. Reword to match, e.g. "newGroupBuilder builds the syncer. Cross-type grants are filtered per-target in Grants; the grants pass itself is never skipped because Grants also emits the group's own member grants."
| if res.Type == ResourceTypeWorkspace { | ||
| targetTypeID = roleResourceType.Id | ||
| } | ||
| if b.skipTargets.skip(targetTypeID) { |
There was a problem hiding this comment.
🟡 Suggestion: When every cross-type target is excluded (skipTargets.all()), the group-roles phase still issues a GetGroup call per group (line 185) and then discards 100% of the resulting grants here. userBuilder avoids that via SkipEntitlementsAndGrants, but groups can't, so an early if b.skipTargets.all() { break } before the fetch (falling through to bag.NextToken("")) would eliminate N wasted API calls — which is the wasted-work the PR set out to remove.
|
|
||
| got := userGrantTargetTypes(t, grants) | ||
| require.Equal(t, []string{"source"}, got) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: groups_test.go now pins the group annotation branch, but nothing covers newUserBuilder's skipTargets.all() branch — the one path that swaps SkipEntitlements for SkipEntitlementsAndGrants. A case with SyncResourceTypeIDs: []string{"user", "group"} asserting ResourceType(ctx) carries SkipEntitlementsAndGrants (and only SkipEntitlements otherwise) would lock in the behavior this PR's annotation logic depends on.
The grant guard changes the user resource type's declared annotations, so baton_capabilities.json no longer matches the committed docs and validate_metadata fails the docs-freshness check. Regenerate the metadata from the binary and document the entitlement behavior the metadata now declares. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…coverage - newGroupBuilder's doc comment was a leftover copy of newUserBuilder's and contradicted the body, which deliberately stopped skipping the grants pass for groups. Reword it, and narrow skipCrossTypeGrants.all()'s comment to match: only userBuilder's grants are entirely cross-type. - The group-roles phase still issued one GetGroup per group and then discarded every resulting grant when all cross-type targets were excluded. Return early instead; the group's own member grants come from the group-members phase and are unaffected. - Cover both gaps: pin newUserBuilder's SkipEntitlements / SkipEntitlementsAndGrants branches, and assert the group fetch is actually skipped by counting requests (one client per case, since uhttp caches). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // path ends in /users, so a suffix check distinguishes the two. | ||
| counting := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.URL.Path == "/groups/g1" { | ||
| groupFetches++ |
There was a problem hiding this comment.
🟡 Suggestion: groupFetches is written from the httptest handler goroutine and read from the test goroutine at lines 260/263/265 with no synchronization — there is no happens-before edge between the two, so go test -race can flag this. The repo's CI (go test -v -covermode=count -json ./...) doesn't pass -race, so nothing fails today, but a local -race run would. Switching to var groupFetches atomic.Int64 with Add(1) / Load() / Store(0) removes the risk.
| // Wrap the fixture server so GET /groups/{id} can be counted. The members | ||
| // path ends in /users, so a suffix check distinguishes the two. |
There was a problem hiding this comment.
🟡 Suggestion: This comment describes a check the code doesn't do — line 239 is an exact-equality match on /groups/g1, not a suffix check on /users. Same stale-comment class as the newGroupBuilder doc fixed in this commit; rewording to "the members path is /groups/g1/users, so exact-matching /groups/g1 counts only the detail fetch" keeps it accurate.
|
|
||
| case "group-roles": | ||
| if b.skipTargets.all() { | ||
| // Every cross-type target is excluded, so this phase would fetch |
There was a problem hiding this comment.
can we return nil, nil, nil here? you don't have to build an empty next page token i guess
| // newGroupBuilder builds the syncer. Cross-type grants are filtered per-target | ||
| // in Grants; the grants pass itself is never skipped, because Grants also emits | ||
| // the group's own member grants. | ||
| func newGroupBuilder(c *client.Client, skipTargets skipCrossTypeGrants) *groupBuilder { |
There was a problem hiding this comment.
it is not needed if we unconditionally skip just entitlements, keep it at resource_type level
2 participants
Summary
userBuilder.Grants()(pkg/connector/users.go) andgroupBuilder.Grants()(pkg/connector/groups.go, the "group-roles" pagination phase) both emit grants for other resource types (role, source, warehouse, function, space) as a sync optimization — the user/group API response already includes permission data for those types.This emission was unconditional. If a customer's sync filter excludes a target type (e.g. they sync users but not roles), the connector still emitted grants referencing a type it isn't syncing — wasted work and dangling grants.
This PR gates that cross-type grant emission behind
cli.ConnectorOpts.WillSyncResourceType, following the reference pattern in ConductorOne/baton-linear#55.Target types gated
role(workspace-scoped permissions, which map to arole:{id}:membergrant)sourcewarehousefunctionspaceBuilders fixed
userBuilder(the originally reported bug)groupBuilder— found during implementation to have the identical bug in its "group-roles" pagination phase (not originally named in the task brief; included after confirming the fix pattern applies identically). The "group-members" phase, which emits the group's own membership grants, is untouched.Changes
pkg/connector/helpers.go: addedwillSyncResourceType(cliOpts, resourceTypeID), a nil-safe wrapper aroundcli.ConnectorOpts.WillSyncResourceType(nilcliOptsmeans "sync everything", matching direct-construction test usage).pkg/connector/connector.go:Connectornow carriescliOpts, threaded through fromNew()and passed tonewUserBuilder/newGroupBuilder. All other builder constructions are unchanged.pkg/connector/users.go:userBuildergets acliOptsfield;Grants()skips emitting a cross-type grant when the target type won't be synced.pkg/connector/groups.go:groupBuildergets acliOptsfield; the"group-roles"phase ofGrants()gets the identical gating.resource_types.goannotations are untouched — there are 5 independently-filterable targets here, so a singleSkipEntitlementsAndGrants/SkipEntitlementsannotation toggle doesn't fit; the gating lives purely inGrants().Tests
Added:
pkg/connector/users_test.go— spins up anhttptest.ServerforGET /users/{id}returning permissions scoped to WORKSPACE (role), SOURCE, WAREHOUSE, FUNCTION, and SPACE. Verifies all 5 cross-type grants are emitted with no filter (nil,&cli.ConnectorOpts{}, and an explicit emptySyncResourceTypeIDs), and that only thesourcegrant survives whenSyncResourceTypeIDs: []string{"user", "source"}is set.pkg/connector/groups_test.go— same coverage for the two-phasegroupBuilder.Grants()pagination: drives the"group-members"phase to get the next-page token, then the"group-roles"phase, asserting the same unfiltered/filtered behavior. Group membership grants are excluded from the assertion (only cross-type grants are checked).No existing tests were weakened or removed.
Test plan
go build ./...go vet ./...gofmt -lcleango test ./... -count=1(all packages pass, including the two new test files)🤖 Generated with Claude Code