feat: expose Temporal Cloud user groups - #85
Conversation
- New group resource type (TRAIT_GROUP) with member entitlement - Groups sync via GetUserGroups; membership via GetUserGroupMembers - Expandable grants for group assignments to account roles (UpdateUserGroup provisioning) and namespace permissions (SetUserGroupNamespaceAccess) - Multi-phase pagination for Grants (users then groups phase) - SCIM/Google group membership marked immutable (IdP-managed) - Graceful PermissionDenied handling for non-admin API keys - capabilities.json and README updated Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Groups are an opt-in resource type via capabilities.json rather than a config flag. Existing installations won't sync groups until enabled. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| var groupResourceType = &v2.ResourceType{ | ||
| Id: "group", | ||
| DisplayName: "User Group", | ||
| Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_GROUP}, | ||
| } |
There was a problem hiding this comment.
🟠 Bug: groupResourceType carries no annotations, but baton_capabilities.json hand-declares "opt_in_required": true for group. The SDK derives that field solely from annos.Contains(&v2.OptInRequired{}) on the resource type (connectorbuilder.go), so the binary emits opt_in_required: false — the validate_metadata check is already failing with "baton_capabilities.json differs from binary output". Beyond the CI failure, the gate does not actually exist: every existing installation starts syncing groups on upgrade instead of opting in.
| var groupResourceType = &v2.ResourceType{ | |
| Id: "group", | |
| DisplayName: "User Group", | |
| Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_GROUP}, | |
| } | |
| var groupResourceType = &v2.ResourceType{ | |
| Id: "group", | |
| DisplayName: "User Group", | |
| Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_GROUP}, | |
| Annotations: annotations.New(&v2.OptInRequired{}), | |
| } |
| package config | ||
|
|
||
| import "reflect" | ||
| import "reflect" |
There was a problem hiding this comment.
🟡 Suggestion: This is a generated file (// Code generated by baton-sdk. DO NOT EDIT!!!) and the only change is a stray trailing space after import "reflect". Revert it — regeneration will drop it anyway. Same for the unrelated blank-line churn in pkg/config/config.go:32 and pkg/connector/connector.go.
| currentRole := spec.GetAccess().GetAccountAccess().GetRole() | ||
| if slices.Contains(immutableAccountRoles, currentRole) { | ||
| zap.L().Info("baton-temporalcloud: group has immutable role, skipping grant", zap.String("group_id", groupID)) | ||
| return nil, nil, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: When the group already holds an immutable role this returns nil, nil, nil — no grant, no annotation, no error — so C1 records the provisioning task as succeeded while nothing changed. Returning an error (as the sibling newRole immutability check on line 292 does) or at minimum a GrantAlreadyExists annotation would surface the no-op. Also prefer ctxzap.Extract(ctx) over the global zap.L() so the log line carries request context. This mirrors the pre-existing user path at line 219, so consider fixing both.
| var downgradedRole *identityv1.AccountAccess | ||
| switch ar { | ||
| case identityv1.AccountAccess_ROLE_ADMIN: | ||
| downgradedRole = &identityv1.AccountAccess{Role: identityv1.AccountAccess_ROLE_DEVELOPER} | ||
| case identityv1.AccountAccess_ROLE_DEVELOPER: | ||
| downgradedRole = &identityv1.AccountAccess{Role: identityv1.AccountAccess_ROLE_READ} | ||
| case identityv1.AccountAccess_ROLE_READ: | ||
| downgradedRole = nil | ||
| default: | ||
| return nil, fmt.Errorf("baton-temporalcloud: invalid account role %s", ar) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Revoking ROLE_READ sets Access.AccountAccess = nil, while the user path (line 401-402) explicitly refuses the same operation because it would delete the account. It is not established that UpdateUserGroup accepts a nil AccountAccess — it may reject the request or leave the group at ROLE_UNSPECIFIED. Worth confirming against the Temporal Cloud API and either erroring out symmetrically with the user path or documenting the intended semantics.
| ur, err := fetchUserResource(ctx, o.client, userID) | ||
| if err != nil { | ||
| if status.Code(err) == codes.NotFound { | ||
| ctxzap.Extract(ctx).Warn("baton-temporalcloud: skipping group member without matching user", | ||
| zap.String("group_id", r.GetId().GetResource()), | ||
| zap.String("user_id", userID)) | ||
| continue | ||
| } | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| g, err := createUserGroupMemberGrant(r, ur, isImmutablyProvisionedGroup(r)) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| rv = append(rv, g) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: This issues one GetUser call per group member on every page, so a 1000-member group costs 1000 sequential round trips per sync and multiplies rate-limit pressure. GetUserGroupMembers already returns the member id, which is the stable user resource id — building the principal from &v2.ResourceId{ResourceType: userResourceType.Id, Resource: userID} would avoid the fetch entirely, at the cost of not being able to skip members with no matching user.
| func (o *namespaceBuilder) listGroupNamespaceGrants(ctx context.Context, resource *v2.Resource, bag *pagination.Bag) ([]*v2.Grant, string, error) { | ||
| l := ctxzap.Extract(ctx) | ||
| nsID := resource.GetId().GetResource() | ||
|
|
||
| req := &cloudservicev1.GetUserGroupsRequest{Namespace: resource.GetDisplayName()} | ||
| if bag.PageToken() != "" { | ||
| req.PageToken = bag.PageToken() | ||
| } | ||
|
|
||
| resp, err := o.client.GetUserGroups(ctx, req) |
There was a problem hiding this comment.
🟡 Suggestion: This group phase (and its account-role twin) runs unconditionally, independent of whether the group resource type is opted in. On an install that has not enabled groups, GetUserGroups is still called and group-principal grants are still emitted for group resources that were never synced. Consider gating the group phases on the same signal that gates the group resource type, so opting out is actually a no-op rather than "sync groups grants but not groups".
| // paginateGrants advances the pagination bag used by multi-phase grants syncs. | ||
| // When the current API page is exhausted it moves to the next phase; when no | ||
| // phases remain it returns no results to end the sync. | ||
| func paginateGrants(rv []*v2.Grant, bag *pagination.Bag, pageToken string) ([]*v2.Grant, *rs.SyncOpResults, error) { | ||
| if pageToken != "" { | ||
| if err := bag.Next(pageToken); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| } else { | ||
| bag.Pop() | ||
| } | ||
|
|
||
| token, err := bag.Marshal() | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| if token == "" { | ||
| return rv, nil, nil | ||
| } | ||
| return rv, &rs.SyncOpResults{NextPageToken: token}, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: paginateGrants is the riskiest new logic in this PR — a wrong Next/Pop/Marshal interaction silently truncates a sync phase or loops forever — and it is entirely untestable-by-inspection at the call sites. helpers_test.go already has the table-driven pattern; a few cases (non-empty token stays in phase, empty token advances to the next phase, empty token on the last phase returns nil results) would lock the behaviour down. groupKindFromSpec and protoUserGroupToResource are similarly cheap to cover.
| func (o *namespaceBuilder) revokeNamespaceAccessFromUser( | ||
| ctx context.Context, _ *v2.Grant, userID string, userType string, | ||
| namespaceID string, namespaceType string, entitlementID string, | ||
| ) (annotations.Annotations, error) { |
There was a problem hiding this comment.
🟡 Suggestion: Both revokeNamespaceAccessFromUser and revokeNamespaceAccessFromGroup take a *v2.Grant that is discarded (_), while every field they need is already passed separately. Dropping the parameter from both signatures removes the dead argument at the two call sites.
….json The SDK detects opt_in_required from the annotations on the ResourceType definition, not a top-level json property. Regenerated from binary output. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| return rv, resp.GetNextPageToken(), nil | ||
| } | ||
|
|
||
| func (o *namespaceBuilder) listGroupNamespaceGrants(ctx context.Context, resource *v2.Resource, bag *pagination.Bag) ([]*v2.Grant, string, error) { |
There was a problem hiding this comment.
🟠 Bug: groupResourceType carries OptInRequired, so when a customer does not opt in, the SDK's sync filter excludes group — groupBuilder.List never runs — yet this phase (and listGroupAccountRoleGrants) still emits grants whose principal is a group, plus GrantExpandable annotations pointing at group:<id>:member entitlements that were never synced. That produces dangling grant principals (ingest invariant I9: a warning normally, a hard failure in fail-fast mode) on every sync.
New() already receives opts *cli.ConnectorOpts; store it and skip the groups phase when !opts.WillSyncResourceType(groupResourceType.Id) so the phase is pushed only when groups are actually synced.
| case namespacePhaseGroups: | ||
| rv, nextPageToken, err = o.listGroupNamespaceGrants(ctx, resource, bag) | ||
| default: | ||
| return nil, nil, fmt.Errorf("baton-temporalcloud: unexpected namespace grants pagination phase %q", bag.ResourceTypeID()) |
There was a problem hiding this comment.
🟡 Suggestion: this default hard-fails on any page state whose ResourceTypeID is not one of the two new phase constants. A sync checkpointed by the previous connector version stored ResourceTypeID: "namespace" (and "account-role" in account_roles.go), so a sync resumed after this upgrade errors out with unexpected namespace grants pagination phase "namespace" instead of continuing. Consider treating an unrecognized/legacy phase ID as the users phase rather than returning an error.
| continue | ||
| } | ||
|
|
||
| ur, err := fetchUserResource(ctx, o.client, userID) |
There was a problem hiding this comment.
🟡 Suggestion: this is an N+1 — one GetUser round trip per member, per group, and the same users are refetched for every group they belong to. The fetched user is only used for user.GetId() and g.Principal, and the syncer only needs the principal's ResourceId. Building &v2.ResourceId{ResourceType: userResourceType.Id, Resource: userID} directly removes the call (and the NotFound special case with it), since users are synced by their own builder.
| // isAlreadyExistsError reports whether the API call returned an already-exists | ||
| // error, which is treated as idempotent success. | ||
| func isAlreadyExistsError(err error) bool { | ||
| if status.Code(err) == codes.AlreadyExists { | ||
| return true | ||
| } | ||
| return strings.Contains(err.Error(), "already exists") || strings.Contains(err.Error(), "already a member") | ||
| } | ||
|
|
||
| // isNotFoundError reports whether the API call returned a not-found error. | ||
| func isNotFoundError(err error) bool { | ||
| if status.Code(err) == codes.NotFound { | ||
| return true | ||
| } | ||
| return strings.Contains(err.Error(), "not found") || strings.Contains(err.Error(), "not a member") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the substring fallbacks are broader than intended. RemoveUserGroupMember on a group that was deleted upstream returns something like group not found, which isNotFoundError matches, so Revoke reports GrantAlreadyRevoked for a failure that is not "the member was already gone". The Temporal Cloud API returns proper gRPC status codes, so the status.Code(err) check alone is sufficient here; dropping the strings.Contains fallbacks avoids masking real errors.
| package config | ||
|
|
||
| import "reflect" | ||
| import "reflect" |
There was a problem hiding this comment.
🟡 Suggestion: this is an accidental edit to a generated file (DO NOT EDIT!!!) — a trailing space was added after import "reflect". It is unrelated to the group feature, will be reverted by the next go generate, and is not gofmt-clean. Same for the stray extra blank line added at pkg/config/config.go:31.
| import "reflect" | |
| import "reflect" |
| if slices.Contains(immutableAccountRoles, currentRole) { | ||
| zap.L().Info("baton-temporalcloud: group has immutable role, skipping grant", zap.String("group_id", groupID)) | ||
| return nil, nil, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: two things here. zap.L() is the global no-op-by-default logger — use ctxzap.Extract(ctx) so the message carries sync/request context (the user path at line 219 has the same problem, but this is new code). More importantly, returning nil, nil, nil reports success to C1 while nothing was granted; an error (as the newRole immutable branch above already does) makes the refusal visible to the requester.
Connector PR Review: feat: expose Temporal Cloud user groupsBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe new commit only touches Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsNote: the machine-readable review-state marker could not be written on this run — the sandbox blocked every shell payload containing it. The reviewed SHAs are recorded in the review-mode line above; the next run may fall back to a full review. |
Blocking: - Gate group grant phases on issuer.WillSyncResourceType so group-principal grants are not emitted when the group type is OptInRequired but not enabled. Review additions: - Legacy page state tolerance in Grants default switch cases. - Drop N+1 GetUser per group member; use principal ResourceId only. - Remove substring error fallbacks; rely on gRPC status codes. - Replace zap.L() with ctxzap.Extract(ctx) for context-carrying log. - Return an error when group holds an immutible role (not nil/nil/nil). - Drop unused *v2.Grant param from revokeNamespaceAccessFromUser/Group. - Simplify createUserGroupMemberGrant (no error return). Co-authored-by: c1-squire-dev[bot] \<c1-squire-dev[bot]@users.noreply.github.com>
| accountCreationSettings: AccountCreationSettings{ | ||
| DefaultAccountRole: defaultRole, | ||
| }, | ||
| syncGroups: opts.WillSyncResourceType(groupResourceType.Id), |
There was a problem hiding this comment.
🟡 Suggestion: WillSyncResourceType returns true whenever no explicit sync filter is set (vendor/.../pkg/cli/cli.go:119-125), and it does not account for OptInRequired. The SDK's own doc on SyncResourceTypeIDs says an empty slice means "sync everything the connector advertises (subject to OptInRequired{} annotations)" — so in the empty-filter case syncGroups becomes true while group (which is OptInRequired) may still be skipped, re-introducing dangling group principals and GrantExpandable refs to unsynced group:<id>:member entitlements. Fail safe by also requiring an explicit filter: syncGroups: opts.SyncFilterIsExplicit() && opts.WillSyncResourceType(groupResourceType.Id). (medium confidence)
| if err != nil { | ||
| if status.Code(err) == codes.PermissionDenied { | ||
| ctxzap.Extract(ctx).Warn("baton-temporalcloud: cannot list user groups with the current API key, skipping groups", zap.Error(err)) | ||
| return nil, nil, nil | ||
| } | ||
| return nil, nil, fmt.Errorf("baton-temporalcloud: failed to list user groups: %w", err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: swallowing PermissionDenied here returns zero group resources with no next-page token, which is indistinguishable from "this account has no groups". A transient or newly-restricted 403 will make every previously synced group — and every group-derived grant on namespaces and account roles — look deleted downstream. Since group is already OptInRequired (the operator explicitly asked for groups), returning the error is safer than silently emptying the resource type. (high confidence)
| resp, err := o.client.GetUserGroupMembers(ctx, req) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("baton-temporalcloud: failed to list user group members: %w", err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Grants is the only new group code path with no PermissionDenied branch — List (line 58), listGroupNamespaceGrants (namespaces.go:175) and listGroupAccountRoleGrants (account_roles.go:174) all degrade gracefully. GetUserGroupMembers can be authorized separately from GetUserGroups, so an API key that can list groups but not their members will hard-fail the entire sync, defeating the graceful-degradation goal stated in the PR description. Handle 403 the same way here. (high confidence)
| if kind := groupKindFromSpec(groupResp.GetGroup().GetSpec()); kind != groupKindCloud { | ||
| return nil, nil, fmt.Errorf("baton-temporalcloud: %s groups are managed by the external identity provider and cannot be provisioned", kind) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: group-kind handling is inconsistent in three places for the "unknown kind" case (groupKindFromSpec returns "" for any future group type). Entitlements only marks scim/google immutable (isImmutablyProvisionedGroup, line 269-272), Grants only marks GrantImmutable when kind is neither cloud nor "" (line 136), but Grant rejects everything except cloud. So a group of an unrecognized kind is advertised as fully grantable yet every grant fails — with a message that renders as baton-temporalcloud: groups are managed by... (empty %s). Consider treating "" as immutable in all three, and returning a gRPC status.Error(codes.InvalidArgument, ...) here per P4. (high confidence)
| return o.revokeAccountRoleFromUser(ctx, g, accountRoleID, ar, accountID) | ||
| } | ||
|
|
||
| func (o *accountRoleBuilder) revokeAccountRoleFromUser(ctx context.Context, g *v2.Grant, accountRoleID string, ar identityv1.AccountAccess_Role, accountID string) (annotations.Annotations, error) { |
There was a problem hiding this comment.
🟡 Suggestion: accountID is never used in this function body — the role is already resolved into ar by the caller. Drop the parameter (and the argument at line 382) per R2. (high confidence)
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| \*The Account Owner and Finance Manager roles are synced but cannot be provisioned. | ||
| \*\*User Groups are opt-in and require an API key with the Owner or Global Admin account role. Membership of SCIM and Google groups is externally managed and cannot be provisioned by C1. |
There was a problem hiding this comment.
🟡 Suggestion: These two footnotes are consecutive lines with no blank line between them, so Markdown/MDX treats them as one paragraph and renders the soft line break as a space — they will display run-together on a single line. Separate them with a blank line (confidence: high).
| \*The Account Owner and Finance Manager roles are synced but cannot be provisioned. | |
| \*\*User Groups are opt-in and require an API key with the Owner or Global Admin account role. Membership of SCIM and Google groups is externally managed and cannot be provisioned by C1. | |
| \*The Account Owner and Finance Manager roles are synced but cannot be provisioned. | |
| \*\*User Groups are opt-in and require an API key with the Owner or Global Admin account role. Membership of SCIM and Google groups is externally managed and cannot be provisioned by C1. |
| func (o *groupBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.Annotations, error) { | ||
| e := g.GetEntitlement() | ||
| groupID := e.GetResource().GetId().GetResource() | ||
| userID := g.GetPrincipal().GetId().GetResource() | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: Grant guards on group kind (line 156) and refuses non-cloud groups, but Revoke calls RemoveUserGroupMember unconditionally. Combined with the unknown-kind ("") case — which Grants does not mark GrantImmutable and Entitlements does not mark EntitlementImmutable — a revoke on an IdP-owned or unrecognized group reaches the Temporal API instead of being rejected locally. Mirror the groupKindFromSpec check here via a GetUserGroup lookup (confidence: medium).
1 participant
What
Adds user groups as a first-class resource type in the Temporal Cloud connector, enabling C1 to sync, display, and provision group membership and group-level access.
Changes
groupresource type (TRAIT_GROUP) — syncs all Temporal Cloud user groups (cloud, SCIM, Google)GrantExpandableannotations so group members transitively inherit access. Provisioning via UpdateUserGroup and SetUserGroupNamespaceAccessbaton_capabilities.jsonandREADME.mdupdatedHow to test
Build and run against a Temporal Cloud account with owner/admin credentials. Verify:
baton resourceslists groups alongside users, namespaces, account roles