Guard workspace_role grant emission behind WillSyncResourceType - #98
Conversation
Gates cross-type grant emission from the user syncer on the customer's sync filter, so grants aren't emitted for a resource type the sync excludes. Reference: ConductorOne/baton-linear#55. Each target is guarded individually in Grants(); when every target is excluded the user resource type is annotated SkipEntitlementsAndGrants so the SDK skips the pass entirely. Flags are named skip<Type>ResourceType and stored inverted so the zero value means "sync everything" — main.go registers a zero-value Connector{} as the capabilities factory, bypassing New. Build, tests, and golangci-lint (0 issues) pass. **No resource-type annotation here, deliberately.** `workspaceResourceType` has its own `member` entitlement (grantable to user), so `SkipEntitlements` / `SkipEntitlementsAndGrants` would suppress real data. Every grant the workspace syncer emits targets `workspace_role`, so `Grants` returns early when that type is filtered out — which also skips paging through workspace users.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Every grant below targets workspace_role; skip the user pagination | ||
| // entirely when that type isn't part of the sync. | ||
| if o.skipWorkspaceRoleResourceType { | ||
| return nil, &resources.SyncOpResults{}, nil | ||
| } |
There was a problem hiding this comment.
🟠 Bug: The premise "every grant below targets workspace_role" isn't true — line 287 emits grant.NewGrant(resource, memberEntitlement, userID), a grant on the workspace's own member entitlement (the function's doc comment on line 145 even says "sets workspace memberships and workspace roles"). This is the only place workspace member grants are produced, so filtering workspace_role out of a sync now silently drops every workspace membership grant, which downstream reads as mass revocation. This is the same suppression the PR deliberately avoided by not using SkipEntitlementsAndGrants.
Suggested fix: keep the user pagination and gate only the roleResource(...) grant appends on !o.skipWorkspaceRoleResourceType, always appending the memberEntitlement grant.
| // Every grant the workspace syncer emits targets workspace_role, so when that | ||
| // type is excluded from the sync it must emit nothing — and must not page | ||
| // through users to discover that. | ||
| func TestWorkspaceBuilder_Grants_SkipWorkspaceRole(t *testing.T) { | ||
| // A nil client would panic if the guard failed to short-circuit. | ||
| b := workspaceBuilder(nil, nil, true) | ||
|
|
||
| res, err := resources.NewResource("acme", resourceTypeWorkspace, "T123") | ||
| if err != nil { | ||
| t.Fatalf("NewResource: %v", err) | ||
| } | ||
|
|
||
| grants, results, err := b.Grants(context.Background(), res, resources.SyncOpAttrs{}) | ||
| if err != nil { | ||
| t.Fatalf("Grants: %v", err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: This test asserts the behavior flagged above — that zero grants are emitted — so it locks in the loss of workspace member grants. Once the guard is narrowed to only the workspace_role grants, this should assert that the member grants still come through (e.g. with a fake/stubbed user list) rather than that Grants returns nothing.
Connector PR Review: Guard workspace_role grant emission behind WillSyncResourceTypeBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit replaces the tautological Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
The previous commit short-circuited workspaceResourceType.Grants entirely when workspace_role was excluded from the sync filter. That method emits a mix, though: workspace_role assignments *and* the workspace's own member grants. Returning early dropped every workspace membership. Gate only the role grants. workspaceRoleGrants now holds the flag-to-role mapping so it can be skipped wholesale, while the member grant is emitted unconditionally. The skip annotations (SkipGrants, SkipEntitlementsAndGrants) are not usable here for the same reason: they suppress the resource's whole grants pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| func TestWorkspaceRoleGrants_ExcludesWorkspaceMemberGrant(t *testing.T) { | ||
| workspaceID := &v2.ResourceId{ResourceType: resourceTypeWorkspace.Id, Resource: "T123"} | ||
| userID := &v2.ResourceId{ResourceType: resourceTypeUser.Id, Resource: "U123"} | ||
|
|
||
| workspace, err := resources.NewResource("acme", resourceTypeWorkspace, "T123") | ||
| if err != nil { | ||
| t.Fatalf("NewResource: %v", err) | ||
| } | ||
| memberGrant := grant.NewGrant(workspace, memberEntitlement, userID) | ||
|
|
||
| grants, err := workspaceRoleGrants(context.Background(), client.User{ID: "U123"}, workspaceID, userID) | ||
| if err != nil { | ||
| t.Fatalf("workspaceRoleGrants: %v", err) | ||
| } | ||
| for _, g := range grants { | ||
| if g.GetId() == memberGrant.GetId() { | ||
| t.Fatal("workspace member grant must be emitted unconditionally, not from the gated role helper") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: This test can never fail — grant IDs are entitlementID:principalType:principalID, and every grant workspaceRoleGrants returns is built on a workspace_role resource, so its ID can never equal the workspace:T123:member:... ID. It is a strictly weaker restatement of TestWorkspaceRoleGrants_OnlyTargetsWorkspaceRole above. The behavior actually worth pinning is the one the previous revision got wrong: that Grants still emits the workspace member grant for each non-stranger user when skipWorkspaceRoleResourceType is true. That needs a workspaceResourceType with a stubbed user source (fake businessPlusClient or an httptest server) rather than the helper in isolation — as written, Grants itself is untested at both settings of the flag.
TestWorkspaceRoleGrants_ExcludesWorkspaceMemberGrant could never fail: grant IDs are entitlementID:principalType:principalID, and every grant the helper returns is built on a workspace_role resource, so its ID could never equal the workspace member grant's. It was a weaker restatement of TestWorkspaceRoleGrants_OnlyTargetsWorkspaceRole, and Grants itself was untested at both settings of skipWorkspaceRoleResourceType. Replace it with a test that stubs users.list via slack.OptionAPIURL and drives Grants both ways, asserting the workspace member grants survive when workspace_role is filtered out. Verified it fails if the member grant is put back inside the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Gates cross-type grant emission from the user syncer on the customer's sync
filter, so grants aren't emitted for a resource type the sync excludes.
Reference: ConductorOne/baton-linear#55.
Each target is guarded individually in Grants(); when every target is excluded
the user resource type is annotated SkipEntitlementsAndGrants so the SDK skips
the pass entirely.
Flags are named skipResourceType and stored inverted so the zero value
means "sync everything" — main.go registers a zero-value Connector{} as the
capabilities factory, bypassing New.
Build, tests, and golangci-lint (0 issues) pass.
No resource-type annotation here, deliberately.
workspaceResourceTypehasits own
memberentitlement (grantable to user), soSkipEntitlements/SkipEntitlementsAndGrantswould suppress real data. Every grant the workspacesyncer emits targets
workspace_role, soGrantsreturns early when that typeis filtered out — which also skips paging through workspace users.