From 297b298c87a2a22adab8539ecad91ba44bcc0b54 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Mon, 2 Nov 2026 15:43:48 -0300 Subject: [PATCH 1/5] fix(databricks): sync the metastore privileges held by a workspace admins group Unity Catalog writes a metastore privilege row for each workspace's built-in admins group under the alias _workspace_admins__. That principal is workspace-local, so the account SCIM API answers its id with a 404 and the row was dropped: 7 privileges on AWS and 8 on Azure were absent from the bundle while the sync exited 0 and named the drop only at Debug. A RUNNING workspace now offers its admins group as a child, and the metastore resolver maps the alias to that group through the workspace id the alias carries, rather than through the workspace the permissions were read with. The grants are immutable and provisioning refuses a workspace-local group, because Databricks does not allow a revoked privilege to be granted back to the system group. The group's membership is read-only for the same reason: an identity-federated workspace rejects a membership PATCH with 400. Resolving the alias before the account lookups skips the three reads that would always miss, which is what logged the two 404 warnings. The workspace syncer decides this from the workspace status and reads no Unity Catalog endpoint. Asking Unity Catalog which workspaces carry a metastore was the alternative, and it is rejected here: metastores are opt-in while workspaces always sync, so a 403 on that plane would fail the whole workspace type, and its role and group children with it, on any tenant that cannot read it. No such read is removed; there was none. A member the account does not hold is skipped instead of emitted, since only the account syncs users, service principals and nested groups. The filtered workspace listing reports every group in totalResults, so it asks for no second page, and a workspace in CANCELLING offers no admins group, because it never becomes RUNNING. Metastore routing treats that same status differently and keeps waiting, so a metastore is not pruned while its workspace is still being deleted. Co-Authored-By: Claude Opus 5 --- README.md | 3 + docs/connector.mdx | 1 + docs/docs-info.md | 2 + pkg/connector/connector.go | 8 +- pkg/connector/groups.go | 77 +++++- pkg/connector/groups_workspace_scope_test.go | 152 ++++++++++++ pkg/connector/metastore.go | 34 ++- pkg/connector/metastore_helpers.go | 10 +- pkg/connector/metastore_test.go | 10 +- pkg/connector/metastore_workspace_admins.go | 85 +++++++ .../metastore_workspace_admins_test.go | 233 ++++++++++++++++++ pkg/connector/raw_id_test.go | 2 +- pkg/connector/workspaces.go | 41 +-- pkg/connector/workspaces_provisioning_test.go | 3 +- pkg/connector/workspaces_test.go | 1 + 15 files changed, 633 insertions(+), 29 deletions(-) create mode 100644 pkg/connector/groups_workspace_scope_test.go create mode 100644 pkg/connector/metastore_workspace_admins.go create mode 100644 pkg/connector/metastore_workspace_admins_test.go diff --git a/README.md b/README.md index ec789267..a704ae4f 100644 --- a/README.md +++ b/README.md @@ -239,6 +239,9 @@ A metastore exposes its privileges as entitlements (`CREATE_CATALOG`, privilege held by a group is expandable, so ConductorOne resolves the group's members as holders. +Metastore privileges held by the built-in workspace admins group are synced as +read-only grants. Databricks does not allow granting them back after revocation. + `MANAGE` and `ALL_PRIVILEGES` are not offered: Databricks rejects both at the metastore level. diff --git a/docs/connector.mdx b/docs/connector.mdx index db250bf5..440ad39e 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -46,6 +46,7 @@ The Metastores resource type is turned off by default. Turn it on from the conne **Notes on Unity Catalog access in C1:** - Ownership is shown but cannot be provisioned. An owner that is not a Databricks account identity, such as a system account or a workspace admins group, is not shown at all. +- Metastore privileges held by a workspace's built-in admins group are shown as read-only grants when that workspace is in the sync scope. Databricks does not allow a revoked privilege to be granted back to this system group, and the group's own membership cannot be changed from the workspace. Manage both in Databricks. - **MANAGE** and **ALL_PRIVILEGES** are not offered as entitlements, because Databricks rejects both at the metastore level. - Unity Catalog tags are not synced. diff --git a/docs/docs-info.md b/docs/docs-info.md index de5f520d..f56f41e9 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -55,6 +55,8 @@ While developing the connector, please fill out this form. This information is n - **Direct grants only.** Databricks inherits privileges downward. The connector reports the assignments stored on the object. The inheritance endpoint does not return principal IDs. - **Direct Admin, or the sync fails.** Admin inherited through a group does not count. Databricks then returns HTTP 200 with only the privileges the caller can see. The sync fails instead of reporting that list, so ConductorOne keeps the grants it already has. - **Ownership is read-only.** It is shown and cannot be granted or revoked. An owner that is not an account identity (`System user`, `_workspace_admins_…`) is not shown. +- **Workspace admins discovery.** A RUNNING workspace declares its `admins` group as a child; the metastore resolver skips the same workspaces, so both sides agree on which ones carry the group. The workspace syncer reads no Unity Catalog endpoint, so a tenant without Unity Catalog syncs its workspaces as before. The group's members are emitted only when the account holds them, because workspace-local identities are not synced. Its membership is read-only: an identity-federated workspace answers a membership PATCH with 400 and says the group is managed in the account, so the entitlement is not offered as provisionable. A privilege row whose workspace is still provisioning or cancelling fails the sync before SCIM, so the missing group is not treated as revoked access. +- **Workspace admins privileges are read-only.** Metastore assignments to `_workspace_admins__` resolve to the existing workspace-scoped `admins` group, including its membership expansion. The alias's numeric workspace ID selects the workspace; the permissions read route need not be that workspace. Allowlisted and excluded workspaces apply. A present `principal_id` must match that workspace's `admins` group; only a name-only assignment looks up `admins` by name. These grants are immutable and Grant/Revoke reject workspace-local groups: Databricks permits revoking these system grants but does not allow granting them back. See [Workspace admin privileges](https://docs.databricks.com/aws/en/data-governance/unity-catalog/manage-privileges/admin-privileges#workspace-admin-privileges-when-workspaces-are-enabled-for-unity-catalog-automatically). - **No `MANAGE` or `ALL_PRIVILEGES` on a metastore.** Databricks rejects both, so they are not offered. - **Read through one running workspace.** Privileges are not served on the account plane. The connector reads each metastore through the first running workspace in scope where it can see every privilege. With no running workspace in scope, the metastore is not synced and ConductorOne removes it. A workspace that is still provisioning fails the sync until it is `RUNNING`. An unreadable assignment list fails the sync and ConductorOne keeps the metastore. - **`workspaces` scopes metastores.** An allowlist that names no workspace on the account fails at credential validation, before any sync. diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 09b739d2..177aa8c2 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -29,16 +29,22 @@ type Databricks struct { // ResourceSyncers returns a ResourceSyncerV2 for each resource type that should be synced from the upstream service. func (d *Databricks) ResourceSyncers(_ context.Context) []connectorbuilder.ResourceSyncerV2 { + // Both sides read one condition so they agree on which workspaces carry the group. + syncWorkspaceAdmins := d.willSyncResourceType(metastoreResourceType.Id) && + d.willSyncResourceType(groupResourceType.Id) && + d.willSyncResourceType(workspaceResourceType.Id) + return []connectorbuilder.ResourceSyncerV2{ newAccountBuilder(d.client), newGroupBuilder(d.client), newServicePrincipalBuilder(d.client), newUserBuilder(d.client), - newWorkspaceBuilder(d.client, d.workspaces), + newWorkspaceBuilder(d.client, d.workspaces, syncWorkspaceAdmins), newRoleBuilder(d.client), newMetastoreBuilder(d.client, d.clientID, configuredWorkspaceSet(d.workspaces), syncPrincipals{ groups: d.willSyncResourceType(groupResourceType.Id), servicePrincipals: d.willSyncResourceType(servicePrincipalResourceType.Id), + workspaceGroups: syncWorkspaceAdmins, }), } } diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index 38b7ba37..e5fb012b 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -82,8 +82,10 @@ func (g *groupBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId } workspaceId := "" + vars := []databricks.Vars{databricks.NewGroupAttrVars()} if parentResourceID.ResourceType == workspaceResourceType.Id { workspaceId = parentResourceID.Resource + vars = append(vars, databricks.NewFilterVars(`displayName eq "admins"`)) } bag, page, err := parsePageToken(attr.PageToken.Token, &v2.ResourceId{ResourceType: groupResourceType.Id}) @@ -94,8 +96,7 @@ func (g *groupBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId groups, total, _, err := g.client.ListGroups( ctx, workspaceId, - databricks.NewPaginationVars(page, ResourcesPageSize), - databricks.NewGroupAttrVars(), + append(vars, databricks.NewPaginationVars(page, ResourcesPageSize))..., ) if err != nil { return nil, nil, fmt.Errorf("databricks-connector: failed to list groups: %w", err) @@ -113,7 +114,11 @@ func (g *groupBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId rv = append(rv, gr) } - token := prepareNextToken(page, len(groups), total) + // The workspace listing is filtered to one group, so the total is not a page count. + token := "" + if workspaceId == "" { + token = prepareNextToken(page, len(groups), total) + } nextPage, err := bag.NextToken(token) if err != nil { return nil, nil, fmt.Errorf("databricks-connector: failed to create next page token: %w", err) @@ -136,12 +141,20 @@ func (g *groupBuilder) Entitlements(ctx context.Context, resource *v2.Resource, // membership entitlement - for group members memberAssignmentOptions := []ent.EntitlementOption{ - ent.WithGrantableTo(userResourceType, groupResourceType, servicePrincipalResourceType), ent.WithDisplayName(fmt.Sprintf("%s %s", resource.DisplayName, groupMemberEntitlement)), ent.WithDescription(fmt.Sprintf("%s %s in Databricks", resource.DisplayName, groupMemberEntitlement)), } + // An identity-federated workspace rejects a membership PATCH with 400, so offering + // the entitlement as grantable would only produce failing requests. + if workspaceId == "" { + memberAssignmentOptions = append(memberAssignmentOptions, + ent.WithGrantableTo(userResourceType, groupResourceType, servicePrincipalResourceType)) + } rv = append(rv, ent.NewAssignmentEntitlement(resource, groupMemberEntitlement, memberAssignmentOptions...)) + if workspaceId != "" { + return rv, nil, nil + } _, groupId, err := parseResourceId(resource.Id.Resource) if err != nil { @@ -168,6 +181,37 @@ func (g *groupBuilder) Entitlements(ctx context.Context, resource *v2.Resource, return rv, nil, nil } +// accountHolds reports whether the account plane holds the member a workspace group +// lists. The lookups are filtered lists, so an id of the wrong kind answers with no +// match rather than the 400 a by-id read returns. +func (g *groupBuilder) accountHolds(ctx context.Context, memberType, memberID string) (bool, *v2.RateLimitDescription, error) { + switch memberType { + case "Users": + userName, rateLimitData, err := g.client.FindUsername(ctx, "", memberID) + if err != nil { + return false, rateLimitData, fmt.Errorf("databricks-connector: failed to look up user %s on the account: %w", memberID, err) + } + + return userName != "", rateLimitData, nil + case "Groups": + displayName, rateLimitData, err := g.client.FindGroupDisplayName(ctx, "", memberID) + if err != nil { + return false, rateLimitData, fmt.Errorf("databricks-connector: failed to look up group %s on the account: %w", memberID, err) + } + + return displayName != "", rateLimitData, nil + case "ServicePrincipals": + appID, rateLimitData, err := g.client.FindServicePrincipalAppID(ctx, "", memberID) + if err != nil { + return false, rateLimitData, fmt.Errorf("databricks-connector: failed to look up service principal %s on the account: %w", memberID, err) + } + + return appID != "", rateLimitData, nil + default: + return false, nil, fmt.Errorf("databricks-connector: invalid member type: %s", memberType) + } +} + // Grants return all grants relevant to the group. // Databricks Groups have membership and role permissions grants (granting identity resource some permission to this specific group, e.g. group manager). func (g *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, attr rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { @@ -227,11 +271,31 @@ func (g *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, attr r var resourceId *v2.ResourceId var anns []protoreflect.ProtoMessage + // Only the account syncs members, so one it does not hold is workspace-local + // and has no resource to point a grant at. + if isWorkspaceGroup { + held, rateLimitData, err := g.accountHolds(ctx, memberType, memberID) + annos.WithRateLimiting(rateLimitData) + if err != nil { + return nil, nil, err + } + if !held { + l.Debug("databricks-connector: skipping a workspace-local member the account does not hold", + zap.String("group", resource.Id.Resource), + zap.String("member", member.Ref)) + continue + } + } + switch memberType { case "Users": resourceId = &v2.ResourceId{ResourceType: userResourceType.Id, Resource: memberID} case "Groups": - rid, expandAnnotation, err := groupGrantExpansion(ctx, memberID, parentId) + memberParent := parentId + if isWorkspaceGroup { + memberParent = &v2.ResourceId{ResourceType: accountResourceType.Id, Resource: g.client.GetAccountId()} + } + rid, expandAnnotation, err := groupGrantExpansion(ctx, memberID, memberParent) if err != nil { return rv, nil, err } @@ -252,7 +316,7 @@ func (g *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, attr r } // role permissions grants are only emitted with the first page of members - if cursor != "" { + if cursor != "" || isWorkspaceGroup { return rv, &rs.SyncOpResults{NextPageToken: nextPage, Annotations: annos}, nil } @@ -488,7 +552,6 @@ func (g *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations if err != nil { return nil, fmt.Errorf("databricks-connector: failed to parse entitlement resource id: %w", err) } - if parentResourceId == nil { return nil, fmt.Errorf("databricks-connector: parent resource id not found") } diff --git a/pkg/connector/groups_workspace_scope_test.go b/pkg/connector/groups_workspace_scope_test.go new file mode 100644 index 00000000..93744353 --- /dev/null +++ b/pkg/connector/groups_workspace_scope_test.go @@ -0,0 +1,152 @@ +package connector + +import ( + "context" + "encoding/json" + "net/http" + "slices" + "strings" + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + rs "github.com/conductorone/baton-sdk/pkg/types/resource" +) + +// workspaceScimServer serves one workspace's SCIM plane and the account plane beside it, +// so a member can exist in the workspace and be absent from the account. +type workspaceScimServer struct { + // adminsMembers are the $ref values the workspace reports for the admins group. + adminsMembers []string + // accountHolds are the ids the account plane answers for, by SCIM kind. + accountHolds map[string][]string +} + +const testAdminsGroupID = "9" + +func (s *workspaceScimServer) handler(t *testing.T) http.HandlerFunc { + t.Helper() + + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + encode := func(v any) { _ = json.NewEncoder(w).Encode(v) } + account := strings.Contains(r.URL.Path, "/accounts/") + _, rest, _ := strings.Cut(r.URL.Path, "/scim/v2/") + kind, id, _ := strings.Cut(rest, "/") + + if account { + filter := r.URL.Query().Get("filter") + _, value, _ := strings.Cut(filter, " eq ") + value = strings.Trim(value, `"`) + if slices.Contains(s.accountHolds[kind], value) { + encode(map[string]any{"totalResults": 1, "Resources": []map[string]any{ + {"id": value, "userName": "u" + value, "displayName": "g" + value, "applicationId": "a" + value}, + }}) + return + } + encode(map[string]any{"totalResults": 0}) + return + } + + if kind != "Groups" { + t.Errorf("unexpected workspace request %s", r.URL.Path) + http.Error(w, `{"message":"unexpected"}`, http.StatusInternalServerError) + return + } + members := make([]map[string]any, 0, len(s.adminsMembers)) + for _, ref := range s.adminsMembers { + _, value, _ := strings.Cut(ref, "/") + members = append(members, map[string]any{"$ref": ref, "value": value}) + } + group := map[string]any{"id": testAdminsGroupID, "displayName": "admins", "members": members} + if id != "" { + encode(group) + return + } + // A real workspace reports every group it has in totalResults, not the filtered count. + encode(map[string]any{"totalResults": 40, "Resources": []map[string]any{group}}) + } +} + +func testWorkspaceGroupResource(t *testing.T) *v2.Resource { + t.Helper() + r, err := rs.NewGroupResource("admins", groupResourceType, + "workspace/wsa/group/"+testAdminsGroupID, nil, + rs.WithParentResourceID(&v2.ResourceId{ResourceType: workspaceResourceType.Id, Resource: "wsa"})) + if err != nil { + t.Fatalf("NewGroupResource: %v", err) + } + + return r +} + +// A workspace listing is filtered to the one admins group, so the reported total is not +// a page count and asking for a second page would re-emit the same resource. +func TestWorkspaceGroupListStopsAfterTheAdminsGroup(t *testing.T) { + srv := &workspaceScimServer{} + client, _ := newWorkspaceRoutedClient(t, srv.handler(t)) + g := newGroupBuilder(client) + + resources, results, err := g.List(context.Background(), + &v2.ResourceId{ResourceType: workspaceResourceType.Id, Resource: "wsa"}, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(resources) != 1 || resources[0].GetId().GetResource() != "workspace/wsa/group/"+testAdminsGroupID { + t.Fatalf("resources = %v, want the one workspace-scoped admins group", resources) + } + if token := results.NextPageToken; token != "" { + t.Errorf("next page token = %q, want empty: totalResults counts every group, not the filtered one", token) + } +} + +// Only the account syncs members, so a workspace-local one has no resource to point at. +// Emitting it anyway would leave a grant on a principal the bundle never carries. +func TestWorkspaceGroupGrantsSkipMembersTheAccountDoesNotHold(t *testing.T) { + srv := &workspaceScimServer{ + adminsMembers: []string{"Users/100", "Groups/200", "ServicePrincipals/300", "Groups/9001"}, + accountHolds: map[string][]string{ + "Users": {"100"}, "Groups": {"200"}, "ServicePrincipals": {"300"}, + }, + } + client, _ := newWorkspaceRoutedClient(t, srv.handler(t)) + g := newGroupBuilder(client) + + grants, _, err := g.Grants(context.Background(), testWorkspaceGroupResource(t), rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + + got := make([]string, 0, len(grants)) + for _, grant := range grants { + got = append(got, grant.GetPrincipal().GetId().GetResourceType()+"/"+grant.GetPrincipal().GetId().GetResource()) + } + slices.Sort(got) + want := []string{ + "group/account/" + testAccountId + "/group/200", + "service_principal/300", + "user/100", + } + if !slices.Equal(got, want) { + t.Errorf("member grants = %v, want %v (group 9001 is workspace-local and is not synced)", got, want) + } +} + +// An identity-federated workspace answers a membership PATCH with 400 and says the group +// is managed in the account, so the connector must not offer the entitlement as +// provisionable. The account-scoped twin stays grantable. +func TestWorkspaceGroupMembershipIsNotOffered(t *testing.T) { + srv := &workspaceScimServer{} + client, _ := newWorkspaceRoutedClient(t, srv.handler(t)) + g := newGroupBuilder(client) + + entitlements, _, err := g.Entitlements(context.Background(), testWorkspaceGroupResource(t), rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Entitlements: %v", err) + } + if len(entitlements) != 1 { + t.Fatalf("entitlements = %d, want only membership", len(entitlements)) + } + if got := entitlements[0].GetGrantableTo(); len(got) != 0 { + t.Errorf("workspace group membership is grantable to %v, want nothing", got) + } +} diff --git a/pkg/connector/metastore.go b/pkg/connector/metastore.go index 726f8208..6d6d072a 100644 --- a/pkg/connector/metastore.go +++ b/pkg/connector/metastore.go @@ -166,11 +166,26 @@ func (m *metastoreBuilder) Grants(ctx context.Context, resource *v2.Resource, at } resolver := newPrincipalResolver(m.client, &annos) + workspaceAdmins := &workspaceAdminResolver{metastore: m, annos: &annos} var rv []*v2.Grant for _, assignment := range assignments { - principalId, err := resolver.resolve(ctx, assignment.PrincipalID.String(), assignment.Principal) - if err != nil { - return nil, &rs.SyncOpResults{Annotations: annos}, fmt.Errorf("databricks-connector: %w", err) + // The alias names its own workspace, so resolving it first skips three account + // lookups that would always miss. + var principalId *v2.ResourceId + var err error + workspaceAdmin := false + if m.sync.workspaceGroups && strings.HasPrefix(assignment.Principal, workspaceAdminsPrefix) { + principalId, err = workspaceAdmins.resolve(ctx, assignment) + if err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, err + } + workspaceAdmin = principalId != nil + } + if principalId == nil { + principalId, err = resolver.resolve(ctx, assignment.PrincipalID.String(), assignment.Principal) + if err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, fmt.Errorf("databricks-connector: %w", err) + } } if principalId == nil { l.Debug("databricks-connector: skipping a grant whose principal does not resolve", @@ -184,6 +199,9 @@ func (m *metastoreBuilder) Grants(ctx context.Context, resource *v2.Resource, at } options := groupSecurableGrantOptions(principalId) + if workspaceAdmin { + options = append(options, grant.WithAnnotation(&v2.GrantImmutable{})) + } for _, privilege := range assignment.Privileges { if slices.Contains(metastorePrivileges, privilege) { rv = append(rv, grant.NewGrant(resource, privilege, principalId, options...)) @@ -231,6 +249,16 @@ func (m *metastoreBuilder) resolveChange( return metastoreChange{}, status.Errorf(codes.InvalidArgument, "databricks-connector: only users, groups and service principals can hold a metastore privilege, got %s", principalID.GetResourceType()) } + if principalID.GetResourceType() == groupResourceType.Id { + parent, _, err := parseResourceId(principalID.GetResource()) + if err != nil { + return metastoreChange{}, fmt.Errorf("databricks-connector: failed to parse group resource id: %w", err) + } + if parent.GetResourceType() == workspaceResourceType.Id { + return metastoreChange{}, status.Error(codes.FailedPrecondition, + "databricks-connector: metastore privileges held by workspace-local groups are read-only; manage them in Databricks") + } + } resource := entitlement.GetResource() metastoreID := resource.GetId().GetResource() diff --git a/pkg/connector/metastore_helpers.go b/pkg/connector/metastore_helpers.go index 4687597f..c27df6d3 100644 --- a/pkg/connector/metastore_helpers.go +++ b/pkg/connector/metastore_helpers.go @@ -79,6 +79,7 @@ var ( type syncPrincipals struct { groups bool servicePrincipals bool + workspaceGroups bool } func emitPrincipal(sync syncPrincipals, resourceType string) bool { @@ -204,6 +205,13 @@ func unityPrincipalName(ctx context.Context, c *databricks.Client, principal *v2 return name, nil } +// workspaceIsRunning reports whether a workspace serves its workspace-plane APIs. The +// account API omits the status unless the workspace is changing state, so empty is +// RUNNING. +func workspaceIsRunning(status string) bool { + return status == "" || strings.EqualFold(status, workspaceStatusRunning) +} + // scopedWorkspaces indexes, by numeric id, the workspaces this sync may read // through, and marks which of them are RUNNING. ListWorkspaces already drops the // excluded ones. @@ -233,7 +241,7 @@ func (m *metastoreBuilder) scopedWorkspaces(ctx context.Context, annos *annotati continue } scoped[id] = workspace - running[id] = workspace.Status == "" || strings.EqualFold(workspace.Status, workspaceStatusRunning) + running[id] = workspaceIsRunning(workspace.Status) } return scoped, running, nil diff --git a/pkg/connector/metastore_test.go b/pkg/connector/metastore_test.go index 0db0c170..a3d5db4d 100644 --- a/pkg/connector/metastore_test.go +++ b/pkg/connector/metastore_test.go @@ -71,6 +71,10 @@ type fakeAccount struct { // the HTTP status the account API answers the list with. workspaceMembers map[string]any + // workspaceGroups maps a deployment name to the SCIM groups that workspace serves. + // A workspace-local group is only there: the account answers its id with a 404. + workspaceGroups map[string]map[string]string + permHosts []string patches []map[string]any scimGets map[string]int @@ -167,8 +171,12 @@ func (f *fakeAccount) handler(t *testing.T) http.HandlerFunc { func (f *fakeAccount) scim(w http.ResponseWriter, r *http.Request) { _, rest, _ := strings.Cut(r.URL.Path, "/scim/v2/") kind, id, _ := strings.Cut(rest, "/") + groups := f.groups + if local, ok := f.workspaceGroups[r.Header.Get("X-Test-Workspace")]; ok { + groups = local + } field, records := map[string]string{"Users": "userName", "Groups": "displayName", "ServicePrincipals": "applicationId"}[kind], - map[string]map[string]string{"Users": f.users, "Groups": f.groups, "ServicePrincipals": f.sps}[kind] + map[string]map[string]string{"Users": f.users, "Groups": groups, "ServicePrincipals": f.sps}[kind] if id != "" { f.scimGets[kind+"/"+id]++ if name, ok := records[id]; ok { diff --git a/pkg/connector/metastore_workspace_admins.go b/pkg/connector/metastore_workspace_admins.go new file mode 100644 index 00000000..445246c1 --- /dev/null +++ b/pkg/connector/metastore_workspace_admins.go @@ -0,0 +1,85 @@ +package connector + +import ( + "context" + "strings" + + "github.com/conductorone/baton-databricks/pkg/databricks" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +const workspaceAdminsPrefix = "_workspace_admins_" + +// workspaceAdminResolver handles Unity Catalog's system alias for the workspace +// admins group. Its scope is one Grants page, not a cache shared across syncs. +type workspaceAdminResolver struct { + metastore *metastoreBuilder + annos *annotations.Annotations + workspaces map[string]databricks.Workspace + running map[string]bool +} + +func (r *workspaceAdminResolver) resolve(ctx context.Context, assignment databricks.PrivilegeAssignment) (*v2.ResourceId, error) { + // The alias ends in the numeric workspace ID. Names contain underscores and change, + // so neither the name nor the read route identifies the workspace. + workspaceID := assignment.Principal[strings.LastIndex(assignment.Principal, "_")+1:] + if workspaceID == "" { + return nil, nil + } + if r.workspaces == nil { + workspaces, running, err := r.metastore.scopedWorkspaces(ctx, r.annos) + if err != nil { + return nil, err + } + r.workspaces = workspaces + r.running = running + } + workspace, ok := r.workspaces[workspaceID] + if !ok { + return nil, nil // The workspace's group is outside the configured sync scope. + } + if !r.running[workspaceID] { + // A workspace being deleted never becomes RUNNING, so waiting fails every sync. + if strings.EqualFold(workspace.Status, workspaceStatusCancelling) { + return nil, nil + } + + return nil, status.Errorf(codes.FailedPrecondition, + "databricks-connector: workspace %s is %s; cannot resolve its metastore admins privileges until it is RUNNING", + workspace.DeploymentName, workspace.Status) + } + + id := assignment.PrincipalID.String() + if id == "" || id == "0" { + var rateLimit *v2.RateLimitDescription + var err error + id, rateLimit, err = r.metastore.client.FindGroupID(ctx, workspace.DeploymentName, "admins") + noteRateLimit(r.annos, rateLimit) + if err != nil { + return nil, refuseUnreadable("databricks-connector: failed to resolve workspace admins group", err) + } + if id == "" { + return nil, nil + } + } else { + name, rateLimit, err := r.metastore.client.FindGroupDisplayName(ctx, workspace.DeploymentName, id) + noteRateLimit(r.annos, rateLimit) + if err != nil { + return nil, refuseUnreadable("databricks-connector: failed to resolve workspace admins group", err) + } + if name != "admins" { + return nil, nil // Never map an alias to another group or reuse a deleted ID's name. + } + } + + return &v2.ResourceId{ + ResourceType: groupResourceType.Id, + Resource: groupResourceId(ctx, id, &v2.ResourceId{ + ResourceType: workspaceResourceType.Id, + Resource: workspace.DeploymentName, + }), + }, nil +} diff --git a/pkg/connector/metastore_workspace_admins_test.go b/pkg/connector/metastore_workspace_admins_test.go new file mode 100644 index 00000000..5e6c38ac --- /dev/null +++ b/pkg/connector/metastore_workspace_admins_test.go @@ -0,0 +1,233 @@ +package connector + +import ( + "context" + "net/http" + "slices" + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" + ent "github.com/conductorone/baton-sdk/pkg/types/entitlement" + rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// syncEverything is the scope a deployment with metastores turned on runs with. +var syncEverything = syncPrincipals{groups: true, servicePrincipals: true, workspaceGroups: true} + +// The metastore in defaultFakeAccount is owned by an account group, so every Grants call +// emits that owner grant beside whatever the privilege rows resolve to. +var ownerGrant = "account/acc-1/group/2 metastore:ms-a:owner" + +// adminsAliasAccount is defaultFakeAccount carrying the privilege row Unity Catalog +// writes for a workspace's built-in admins group: the account answers the principal's id +// with a 404 and only workspace wsa serves it. +func adminsAliasAccount(principalID any) *fakeAccount { + f := defaultFakeAccount() + f.permissions = []map[string]any{ + {"principal": "_workspace_admins_prod_1", "principal_id": principalID, "privileges": []string{"CREATE_CATALOG", "CREATE_SHARE"}}, + } + f.workspaceGroups = map[string]map[string]string{"wsa": {"9": "admins"}} + + return f +} + +// The row CXH-3600 reports, and every reason the connector may decline to resolve it. A +// row it cannot place must never land on another principal, and must never point at a +// group this sync does not emit as a resource. +func TestMetastoreGrantsResolveTheWorkspaceAdminsAlias(t *testing.T) { + adminsGrants := []string{ + ownerGrant, + "workspace/wsa/group/9 metastore:ms-a:CREATE_CATALOG", + "workspace/wsa/group/9 metastore:ms-a:CREATE_SHARE", + } + + for _, tc := range []struct { + name string + account func() *fakeAccount + sync syncPrincipals + allow []string + want []string + wantErr codes.Code + }{ + { + name: "by principal id", + account: func() *fakeAccount { return adminsAliasAccount(9) }, + sync: syncEverything, + want: adminsGrants, + }, + { + name: "by name when the row carries no id", + account: func() *fakeAccount { return adminsAliasAccount(0) }, + sync: syncEverything, + want: adminsGrants, + }, + { + // Never map the alias onto another group, or onto a reused id's new owner. + name: "id is not the workspace's admins group", + account: func() *fakeAccount { + f := adminsAliasAccount(9) + f.workspaceGroups = map[string]map[string]string{"wsa": {"9": "engineering"}} + return f + }, + sync: syncEverything, + want: []string{ownerGrant}, + }, + { + // An allowlisted sync emits no resource for wsb, so its admins group is not there. + name: "workspace outside the allowlist", + account: func() *fakeAccount { + f := adminsAliasAccount(9) + f.workspaces = append(f.workspaces, map[string]any{ + "workspace_id": 2, "workspace_name": "dev", "deployment_name": "wsb", "workspace_status": "RUNNING", + }) + f.permissions[0]["principal"] = "_workspace_admins_dev_2" + return f + }, + sync: syncEverything, + allow: []string{"wsa"}, + want: []string{ownerGrant}, + }, + { + name: "workspace groups not in sync scope", + account: func() *fakeAccount { return adminsAliasAccount(9) }, + sync: syncPrincipals{groups: true, servicePrincipals: true}, + want: []string{ownerGrant}, + }, + { + // A workspace being deleted never becomes RUNNING, so holding the sync for it + // would fail every run from then on. + name: "workspace is being deleted", + account: func() *fakeAccount { + f := adminsAliasAccount(9) + f.workspaces = []map[string]any{ + {"workspace_id": 1, "workspace_name": "prod", "deployment_name": "wsa", "workspace_status": workspaceStatusCancelling}, + } + return f + }, + sync: syncEverything, + want: []string{ownerGrant}, + }, + { + // One that can still come back is worth waiting for: reporting the metastore + // without its privileges would read as revoked access. + name: "workspace is still provisioning", + account: func() *fakeAccount { + f := adminsAliasAccount(9) + f.workspaces = []map[string]any{ + {"workspace_id": 1, "workspace_name": "prod", "deployment_name": "wsa", "workspace_status": workspaceStatusProvisioning}, + } + return f + }, + sync: syncEverything, + wantErr: codes.FailedPrecondition, + }, + } { + t.Run(tc.name, func(t *testing.T) { + m := newTestMetastoreBuilder(t, tc.account(), tc.allow, tc.sync) + + grants, _, err := m.Grants(context.Background(), testSyncedMetastore(t, "wsa"), rs.SyncOpAttrs{}) + if status.Code(err) != tc.wantErr { + t.Fatalf("Grants error = %v, want %v", err, tc.wantErr) + } + if tc.wantErr != codes.OK { + return + } + if got := grantKeys(grants); !slices.Equal(got, tc.want) { + t.Fatalf("grants = %v, want %v", got, tc.want) + } + for _, g := range grants { + if g.GetPrincipal().GetId().GetResource() != "workspace/wsa/group/9" { + continue + } + annos := annotations.Annotations(g.GetAnnotations()) + if !annos.Contains(&v2.GrantImmutable{}) { + t.Errorf("%s is not immutable: Databricks does not allow granting a system privilege back", + g.GetEntitlement().GetId()) + } + } + }) + } +} + +// Revoking one of these is irreversible, so neither side of provisioning attempts it. +func TestMetastoreProvisioningRejectsAWorkspaceLocalGroup(t *testing.T) { + f := adminsAliasAccount(9) + m := newTestMetastoreBuilder(t, f, nil, syncEverything) + principal, err := rs.NewResourceID(groupResourceType, "workspace/wsa/group/9") + if err != nil { + t.Fatalf("NewResourceID: %v", err) + } + entitlement := ent.NewPermissionEntitlement(testSyncedMetastore(t, "wsa"), "CREATE_CATALOG") + + for _, tc := range []struct { + name string + call func() error + }{ + {"grant", func() error { + _, err := m.Grant(context.Background(), &v2.Resource{Id: principal}, entitlement) + return err + }}, + {"revoke", func() error { + _, err := m.Revoke(context.Background(), &v2.Grant{ + Principal: &v2.Resource{Id: principal}, + Entitlement: entitlement, + }) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + if err := tc.call(); status.Code(err) != codes.FailedPrecondition { + t.Errorf("%s error = %v, want FailedPrecondition", tc.name, err) + } + if len(f.patches) != 0 { + t.Errorf("%s wrote %v, want no write", tc.name, f.patches) + } + }) + } +} + +// Metastores are opt-in while workspaces always sync, so reading Unity Catalog to decide +// what a workspace offers turned a green sync red for every tenant that cannot read it. +// Only a RUNNING workspace serves SCIM, so only it offers its admins group. +func TestWorkspaceListDoesNotDependOnUnityCatalog(t *testing.T) { + f := defaultFakeAccount() + f.assignments = map[string]any{"ms-a": http.StatusForbidden} + f.workspaces = append(f.workspaces, map[string]any{ + "workspace_id": 2, "workspace_name": "dev", "deployment_name": "wsb", "workspace_status": workspaceStatusProvisioning, + }) + client, _ := newWorkspaceRoutedClient(t, f.handler(t)) + + for _, syncAdmins := range []bool{false, true} { + w := newWorkspaceBuilder(client, nil, syncAdmins) + resources, _, err := w.List(context.Background(), testAccountResourceID, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("List(syncAdmins=%v) = %v, want no error when Unity Catalog is unreadable", syncAdmins, err) + } + if len(resources) != 2 { + t.Fatalf("List(syncAdmins=%v) returned %d workspaces, want 2", syncAdmins, len(resources)) + } + if got := declaresChild(resources[0], groupResourceType.Id); got != syncAdmins { + t.Errorf("RUNNING workspace declares group child = %v, want %v", got, syncAdmins) + } + if declaresChild(resources[1], groupResourceType.Id) { + t.Error("a workspace that is still provisioning offers an admins group it cannot serve") + } + } +} + +// declaresChild reports whether a resource offers resourceType as a child to sync. +func declaresChild(r *v2.Resource, resourceType string) bool { + child := &v2.ChildResourceType{} + for _, a := range r.GetAnnotations() { + if a.MessageIs(child) { + if err := a.UnmarshalTo(child); err == nil && child.GetResourceTypeId() == resourceType { + return true + } + } + } + + return false +} diff --git a/pkg/connector/raw_id_test.go b/pkg/connector/raw_id_test.go index 2809a80b..1f234b20 100644 --- a/pkg/connector/raw_id_test.go +++ b/pkg/connector/raw_id_test.go @@ -42,7 +42,7 @@ func TestResourcesHaveRawId(t *testing.T) { }, { "workspace", - build(workspaceResource(ctx, &databricks.Workspace{Name: "ws", DeploymentName: "ws-1"}, account)), + build(workspaceResource(ctx, &databricks.Workspace{Name: "ws", DeploymentName: "ws-1"}, account, false)), "ws-1", }, { diff --git a/pkg/connector/workspaces.go b/pkg/connector/workspaces.go index 4ed74764..3b835ff9 100644 --- a/pkg/connector/workspaces.go +++ b/pkg/connector/workspaces.go @@ -31,16 +31,17 @@ func principalID(a databricks.WorkspaceAssignment) int { } type workspaceBuilder struct { - client *databricks.Client - resourceType *v2.ResourceType - workspaces map[string]struct{} + client *databricks.Client + resourceType *v2.ResourceType + workspaces map[string]struct{} + syncWorkspaceAdmins bool } func (w *workspaceBuilder) ResourceType(ctx context.Context) *v2.ResourceType { return workspaceResourceType } -func workspaceResource(_ context.Context, workspace *databricks.Workspace, parent *v2.ResourceId) (*v2.Resource, error) { +func workspaceResource(_ context.Context, workspace *databricks.Workspace, parent *v2.ResourceId, syncAdmins bool) (*v2.Resource, error) { profile := map[string]any{ "workspace_id": workspace.ID, } @@ -61,10 +62,20 @@ func workspaceResource(_ context.Context, workspace *databricks.Workspace, paren if err != nil { return nil, err } + if syncAdmins { + resource.Annotations = append(resource.Annotations, annotations.New(&v2.ChildResourceType{ResourceTypeId: groupResourceType.Id})...) + } return resource, nil } +// declaresAdmins reports whether this workspace offers its admins group as a child to +// sync. Only a RUNNING workspace serves SCIM, and the metastore syncer resolves the +// privilege alias through the same predicate. +func (w *workspaceBuilder) declaresAdmins(workspace databricks.Workspace) bool { + return w.syncWorkspaceAdmins && workspaceIsRunning(workspace.Status) +} + // List returns all the workspaces from the database as resource objects. func (w *workspaceBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, _ rs.SyncOpAttrs) ([]*v2.Resource, *rs.SyncOpResults, error) { if parentResourceID == nil { @@ -73,11 +84,12 @@ func (w *workspaceBuilder) List(ctx context.Context, parentResourceID *v2.Resour var rv []*v2.Resource - workspaces, _, err := w.client.ListWorkspaces(ctx) + annos := annotations.Annotations{} + workspaces, rateLimit, err := w.client.ListWorkspaces(ctx) + noteRateLimit(&annos, rateLimit) if err != nil { - return nil, nil, fmt.Errorf("databricks-connector: failed to list workspaces: %w", err) + return nil, &rs.SyncOpResults{Annotations: annos}, fmt.Errorf("databricks-connector: failed to list workspaces: %w", err) } - matchedConfigured := make(map[string]struct{}, len(w.workspaces)) for _, workspace := range workspaces { // Skip workspaces outside the configured set when one was provided. @@ -91,7 +103,7 @@ func (w *workspaceBuilder) List(ctx context.Context, parentResourceID *v2.Resour wCopy := workspace - wr, err := workspaceResource(ctx, &wCopy, parentResourceID) + wr, err := workspaceResource(ctx, &wCopy, parentResourceID, w.declaresAdmins(workspace)) if err != nil { return nil, nil, err } @@ -120,7 +132,7 @@ func (w *workspaceBuilder) List(ctx context.Context, parentResourceID *v2.Resour ) } - return rv, nil, nil + return rv, &rs.SyncOpResults{Annotations: annos}, nil } func configuredWorkspaceNames(configured map[string]struct{}) []string { @@ -282,7 +294,7 @@ func (w *workspaceBuilder) Get(ctx context.Context, resourceId *v2.ResourceId, p return nil, annos, fmt.Errorf("databricks-connector: failed to get workspace %s: %w", resourceId.Resource, err) } - resource, err := workspaceResource(ctx, workspace, parentResourceId) + resource, err := workspaceResource(ctx, workspace, parentResourceId, w.declaresAdmins(*workspace)) if err != nil { return nil, annos, err } @@ -397,10 +409,11 @@ func configuredWorkspaceSet(workspaces []string) map[string]struct{} { return wMap } -func newWorkspaceBuilder(client *databricks.Client, workspaces []string) *workspaceBuilder { +func newWorkspaceBuilder(client *databricks.Client, workspaces []string, syncWorkspaceAdmins bool) *workspaceBuilder { return &workspaceBuilder{ - client: client, - resourceType: workspaceResourceType, - workspaces: configuredWorkspaceSet(workspaces), + client: client, + resourceType: workspaceResourceType, + workspaces: configuredWorkspaceSet(workspaces), + syncWorkspaceAdmins: syncWorkspaceAdmins, } } diff --git a/pkg/connector/workspaces_provisioning_test.go b/pkg/connector/workspaces_provisioning_test.go index 568cc117..c1869b5a 100644 --- a/pkg/connector/workspaces_provisioning_test.go +++ b/pkg/connector/workspaces_provisioning_test.go @@ -77,7 +77,7 @@ func newWorkspaceBuilderWithMock(t *testing.T, m *workspaceAssignmentsServer) *w if err != nil { t.Fatalf("NewClient: %v", err) } - return newWorkspaceBuilder(c, nil) + return newWorkspaceBuilder(c, nil, false) } func existingUserAssignment() []map[string]any { @@ -92,6 +92,7 @@ func testWorkspaceMemberEntitlement(t *testing.T) *v2.Entitlement { r, err := workspaceResource(context.Background(), &databricks.Workspace{ID: testProvisioningWorkspaceID, Name: "ws", DeploymentName: "ws-deploy"}, &v2.ResourceId{ResourceType: accountResourceType.Id, Resource: testAccountId}, + false, ) if err != nil { t.Fatalf("workspaceResource: %v", err) diff --git a/pkg/connector/workspaces_test.go b/pkg/connector/workspaces_test.go index 3d3bc54c..71029704 100644 --- a/pkg/connector/workspaces_test.go +++ b/pkg/connector/workspaces_test.go @@ -28,6 +28,7 @@ func testWorkspace(t *testing.T) *v2.Resource { r, err := workspaceResource(context.Background(), &databricks.Workspace{ID: testProvisioningWorkspaceID, Name: "ws", DeploymentName: "ws-deploy"}, &v2.ResourceId{ResourceType: accountResourceType.Id, Resource: testAccountId}, + false, ) if err != nil { t.Fatalf("workspaceResource: %v", err) From e9e81e10287ea548a8d84d3b9d10d72b8cc336c3 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 3 Nov 2026 15:35:34 -0300 Subject: [PATCH 2/5] fix(databricks): qualify the workspace admins group name and say which workspace holds a metastore Databricks names the built-in group "admins" in every workspace, so a metastore shared by several of them listed one holder per workspace under one indistinguishable name, and only the parent resource told them apart in an access review. A workspace-scoped group is now named "admins ()". The unqualified name stays on the resource profile and the resource ID does not change, so the grants are unchanged: 875 on AWS and 98 on Azure before and after. The name costs one account workspace listing per sync, which the response cache already holds. The docs said a privilege row whose workspace is provisioning or cancelling fails the sync before SCIM. The resolver skips a cancelling workspace instead. Routing treats CANCELLING as pending so the metastore is not pruned while the deletion runs: pruning would report the metastore to ConductorOne as gone and delete its grants, and the wait ends on its own when the account stops listing the workspace. That reason lived in the Unity Catalog layer and was lost when the layer was folded into the metastore syncer, which left a comment grouping CANCELLING with the terminal statuses while the code did not. It is restored. The error that covers the wait said only that the metastore has no running workspace in sync scope, which reads as a misconfigured scope, and the other path said the workspaces are on their way to RUNNING, which a workspace being deleted is not. Routing now returns the workspaces that hold the metastore with their status, and both messages name them. Co-Authored-By: Claude Opus 5 --- docs/docs-info.md | 4 +- pkg/connector/group_patch_test.go | 2 +- pkg/connector/groups.go | 52 ++++++++++++-- pkg/connector/groups_test.go | 4 +- pkg/connector/groups_workspace_scope_test.go | 67 +++++++++++++++++++ pkg/connector/metastore.go | 5 +- pkg/connector/metastore_helpers.go | 41 +++++++----- pkg/connector/metastore_test.go | 46 +++++++++++++ .../metastore_workspace_admins_test.go | 4 +- pkg/connector/raw_id_test.go | 4 +- pkg/connector/workspaces_provisioning_test.go | 1 + 11 files changed, 197 insertions(+), 33 deletions(-) diff --git a/docs/docs-info.md b/docs/docs-info.md index f56f41e9..9937d538 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -55,9 +55,9 @@ While developing the connector, please fill out this form. This information is n - **Direct grants only.** Databricks inherits privileges downward. The connector reports the assignments stored on the object. The inheritance endpoint does not return principal IDs. - **Direct Admin, or the sync fails.** Admin inherited through a group does not count. Databricks then returns HTTP 200 with only the privileges the caller can see. The sync fails instead of reporting that list, so ConductorOne keeps the grants it already has. - **Ownership is read-only.** It is shown and cannot be granted or revoked. An owner that is not an account identity (`System user`, `_workspace_admins_…`) is not shown. -- **Workspace admins discovery.** A RUNNING workspace declares its `admins` group as a child; the metastore resolver skips the same workspaces, so both sides agree on which ones carry the group. The workspace syncer reads no Unity Catalog endpoint, so a tenant without Unity Catalog syncs its workspaces as before. The group's members are emitted only when the account holds them, because workspace-local identities are not synced. Its membership is read-only: an identity-federated workspace answers a membership PATCH with 400 and says the group is managed in the account, so the entitlement is not offered as provisionable. A privilege row whose workspace is still provisioning or cancelling fails the sync before SCIM, so the missing group is not treated as revoked access. +- **Workspace admins discovery.** A RUNNING workspace declares its `admins` group as a child; the metastore resolver skips the same workspaces, so both sides agree on which ones carry the group. The group is named `admins ()`, because Databricks gives every workspace a group with the same name and one metastore can be shared by several of them. The unqualified name stays on the resource profile. The workspace syncer reads no Unity Catalog endpoint, so a tenant without Unity Catalog syncs its workspaces as before. The group's members are emitted only when the account holds them, because workspace-local identities are not synced. Its membership is read-only: an identity-federated workspace answers a membership PATCH with 400 and says the group is managed in the account, so the entitlement is not offered as provisionable. A privilege row whose workspace is still provisioning fails the sync before SCIM, so the missing group is not treated as revoked access. A row whose workspace is cancelling is skipped, because that workspace never becomes RUNNING. - **Workspace admins privileges are read-only.** Metastore assignments to `_workspace_admins__` resolve to the existing workspace-scoped `admins` group, including its membership expansion. The alias's numeric workspace ID selects the workspace; the permissions read route need not be that workspace. Allowlisted and excluded workspaces apply. A present `principal_id` must match that workspace's `admins` group; only a name-only assignment looks up `admins` by name. These grants are immutable and Grant/Revoke reject workspace-local groups: Databricks permits revoking these system grants but does not allow granting them back. See [Workspace admin privileges](https://docs.databricks.com/aws/en/data-governance/unity-catalog/manage-privileges/admin-privileges#workspace-admin-privileges-when-workspaces-are-enabled-for-unity-catalog-automatically). - **No `MANAGE` or `ALL_PRIVILEGES` on a metastore.** Databricks rejects both, so they are not offered. -- **Read through one running workspace.** Privileges are not served on the account plane. The connector reads each metastore through the first running workspace in scope where it can see every privilege. With no running workspace in scope, the metastore is not synced and ConductorOne removes it. A workspace that is still provisioning fails the sync until it is `RUNNING`. An unreadable assignment list fails the sync and ConductorOne keeps the metastore. +- **Read through one running workspace.** Privileges are not served on the account plane. The connector reads each metastore through the first running workspace in scope where it can see every privilege. With no running workspace in scope, the metastore is not synced and ConductorOne removes it. A workspace that is still provisioning fails the sync until it is `RUNNING`. A workspace being deleted does the same, and the error names it and its status: pruning the metastore while the deletion runs would report it as gone and delete its grants, and the wait ends on its own when the account stops listing the workspace. An unreadable assignment list fails the sync and ConductorOne keeps the metastore. - **`workspaces` scopes metastores.** An allowlist that names no workspace on the account fails at credential validation, before any sync. - **Not synced yet:** catalogs, schemas, tables, volumes, functions, registered models, external locations, storage credentials, connections, shares, recipients, and tags. diff --git a/pkg/connector/group_patch_test.go b/pkg/connector/group_patch_test.go index 94e4bacb..02e61a09 100644 --- a/pkg/connector/group_patch_test.go +++ b/pkg/connector/group_patch_test.go @@ -302,7 +302,7 @@ func testWorkspaceGroup(t *testing.T) *v2.Resource { r, err := groupResource(context.Background(), &databricks.Group{ BaseResponse: databricks.BaseResponse{ID: "g-target"}, DisplayName: "Target Group", - }, parent) + }, parent, "") if err != nil { t.Fatalf("groupResource: %v", err) } diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index e5fb012b..a4c2b4f8 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -40,7 +40,12 @@ func groupResourceId(_ context.Context, groupId string, parentResourceId *v2.Res return strings.Join([]string{parentResourceId.ResourceType, parentResourceId.Resource, groupResourceType.Id, groupId}, "/") } -func groupResource(ctx context.Context, group *databricks.Group, parent *v2.ResourceId) (*v2.Resource, error) { +// groupResource builds a group resource. workspaceName qualifies the display name of a +// workspace-scoped group: Databricks names the built-in group "admins" in every +// workspace, so a metastore shared by several of them would otherwise list each holder +// of the same privileges under one indistinguishable name. It is empty for an account +// group. The unqualified name stays in the profile. +func groupResource(ctx context.Context, group *databricks.Group, parent *v2.ResourceId, workspaceName string) (*v2.Resource, error) { profile := map[string]any{ "display_name": group.DisplayName, "group_id": group.ID, @@ -59,8 +64,13 @@ func groupResource(ctx context.Context, group *databricks.Group, parent *v2.Reso // under workspaces stays unique for match_baton_id. options = append(options, rs.WithAnnotation(&v2.RawId{Id: groupId})) + displayName := group.DisplayName + if workspaceName != "" { + displayName = fmt.Sprintf("%s (%s)", group.DisplayName, workspaceName) + } + resource, err := rs.NewGroupResource( - group.DisplayName, + displayName, groupResourceType, groupId, nil, @@ -74,6 +84,25 @@ func groupResource(ctx context.Context, group *databricks.Group, parent *v2.Reso return resource, nil } +// workspaceDisplayName returns the name of the workspace a group hangs off, or an empty +// string for an account group. GetWorkspace reuses the account workspace listing, which +// the workspace syncer has already made in this sync, so it answers from the cache. +func (g *groupBuilder) workspaceDisplayName(ctx context.Context, deploymentName string, annos *annotations.Annotations) (string, error) { + if deploymentName == "" { + return "", nil + } + + workspace, rateLimitData, err := g.client.GetWorkspace(ctx, deploymentName) + if rateLimitData != nil { + annos.WithRateLimiting(rateLimitData) + } + if err != nil { + return "", fmt.Errorf("databricks-connector: failed to read the workspace %s a group hangs off: %w", deploymentName, err) + } + + return workspace.Name, nil +} + // List returns all the groups from the database as resource objects. // Groups include a GroupTrait because they are the 'shape' of a standard group. func (g *groupBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, attr rs.SyncOpAttrs) ([]*v2.Resource, *rs.SyncOpResults, error) { @@ -102,11 +131,17 @@ func (g *groupBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId return nil, nil, fmt.Errorf("databricks-connector: failed to list groups: %w", err) } + annos := annotations.Annotations{} + workspaceName, err := g.workspaceDisplayName(ctx, workspaceId, &annos) + if err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, err + } + var rv []*v2.Resource for _, group := range groups { gCopy := group - gr, err := groupResource(ctx, &gCopy, parentResourceID) + gr, err := groupResource(ctx, &gCopy, parentResourceID, workspaceName) if err != nil { return nil, nil, err } @@ -124,7 +159,7 @@ func (g *groupBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId return nil, nil, fmt.Errorf("databricks-connector: failed to create next page token: %w", err) } - return rv, &rs.SyncOpResults{NextPageToken: nextPage}, nil + return rv, &rs.SyncOpResults{NextPageToken: nextPage, Annotations: annos}, nil } // Entitlements return all entitlements relevant to the group. @@ -374,9 +409,14 @@ func (g *groupBuilder) Get(ctx context.Context, resourceId *v2.ResourceId, paren return nil, annos, fmt.Errorf("databricks-connector: failed to get group %s: %w", groupId.Resource, err) } - resource, err := groupResource(ctx, group, parentResourceId) + workspaceName, err := g.workspaceDisplayName(ctx, workspaceId, &annos) + if err != nil { + return nil, annos, err + } + + resource, err := groupResource(ctx, group, parentResourceId, workspaceName) if err != nil { - return nil, nil, err + return nil, annos, err } ctxzap.Extract(ctx).Debug("databricks-connector: Get: refreshed group", diff --git a/pkg/connector/groups_test.go b/pkg/connector/groups_test.go index 6a2fbdf4..3c4fa33f 100644 --- a/pkg/connector/groups_test.go +++ b/pkg/connector/groups_test.go @@ -50,7 +50,7 @@ func testTargetGroupResource(t *testing.T, nativeGroupID string) *v2.Resource { BaseResponse: databricks.BaseResponse{ID: nativeGroupID}, DisplayName: "Target Group", } - r, err := groupResource(context.Background(), group, parent) + r, err := groupResource(context.Background(), group, parent, "") if err != nil { t.Fatalf("groupResource: %v", err) } @@ -439,7 +439,7 @@ func newTestGroupBuilder(t *testing.T, members *[]databricks.Member, ruleSetCall func testAccountGroup(t *testing.T) *v2.Resource { t.Helper() parent := &v2.ResourceId{ResourceType: accountResourceType.Id, Resource: testAccountId} - g, err := groupResource(context.Background(), &databricks.Group{ID: "g1", DisplayName: "all users"}, parent) + g, err := groupResource(context.Background(), &databricks.Group{ID: "g1", DisplayName: "all users"}, parent, "") if err != nil { t.Fatalf("groupResource: %v", err) } diff --git a/pkg/connector/groups_workspace_scope_test.go b/pkg/connector/groups_workspace_scope_test.go index 93744353..ceda7cb5 100644 --- a/pkg/connector/groups_workspace_scope_test.go +++ b/pkg/connector/groups_workspace_scope_test.go @@ -8,6 +8,7 @@ import ( "strings" "testing" + "github.com/conductorone/baton-databricks/pkg/databricks" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" rs "github.com/conductorone/baton-sdk/pkg/types/resource" ) @@ -19,6 +20,9 @@ type workspaceScimServer struct { adminsMembers []string // accountHolds are the ids the account plane answers for, by SCIM kind. accountHolds map[string][]string + // workspaceName is the name the account lists for deployment "wsa". Empty keeps the + // default, so a test that does not care about the display name reads the same. + workspaceName string } const testAdminsGroupID = "9" @@ -30,6 +34,16 @@ func (s *workspaceScimServer) handler(t *testing.T) http.HandlerFunc { w.Header().Set("Content-Type", "application/json") encode := func(v any) { _ = json.NewEncoder(w).Encode(v) } account := strings.Contains(r.URL.Path, "/accounts/") + if strings.HasSuffix(r.URL.Path, "/workspaces") { + name := s.workspaceName + if name == "" { + name = "prod" + } + encode([]map[string]any{ + {"workspace_id": 1, "workspace_name": name, "deployment_name": "wsa", "workspace_status": "RUNNING"}, + }) + return + } _, rest, _ := strings.Cut(r.URL.Path, "/scim/v2/") kind, id, _ := strings.Cut(rest, "/") @@ -150,3 +164,56 @@ func TestWorkspaceGroupMembershipIsNotOffered(t *testing.T) { t.Errorf("workspace group membership is grantable to %v, want nothing", got) } } + +// Databricks names the built-in group "admins" in every workspace. A metastore shared by +// several of them lists one holder per workspace for the same privileges, so the name has +// to say which workspace it came from. An account group keeps the name Databricks gives it. +func TestWorkspaceGroupNameNamesItsWorkspace(t *testing.T) { + srv := &workspaceScimServer{workspaceName: "prod"} + client, _ := newWorkspaceRoutedClient(t, srv.handler(t)) + g := newGroupBuilder(client) + + resources, _, err := g.List(context.Background(), + &v2.ResourceId{ResourceType: workspaceResourceType.Id, Resource: "wsa"}, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(resources) != 1 { + t.Fatalf("listed %d groups, want 1", len(resources)) + } + if got := resources[0].DisplayName; got != "admins (prod)" { + t.Errorf("display name = %q, want %q", got, "admins (prod)") + } + // The unqualified name stays on the resource profile for anything matching on the + // Databricks name, and the resource ID is untouched. + if profile, _ := rs.GetProfileStringValue(resources[0].GetProfile(), "display_name"); profile != "admins" { + t.Errorf("profile display_name = %q, want the unqualified %q", profile, "admins") + } + if got := resources[0].GetId().GetResource(); got != "workspace/wsa/group/"+testAdminsGroupID { + t.Errorf("resource id = %q, want it unchanged by the display name", got) + } +} + +// Two workspaces holding the same privileges must not read as one principal, and an +// account group must not grow a suffix. +func TestGroupNameQualifiedOnlyUnderAWorkspace(t *testing.T) { + ctx := context.Background() + for _, tc := range []struct { + name, workspaceName, want string + parent *v2.ResourceId + }{ + {"account group", "", "admins", &v2.ResourceId{ResourceType: accountResourceType.Id, Resource: "acc-1"}}, + {"workspace prod", "prod", "admins (prod)", &v2.ResourceId{ResourceType: workspaceResourceType.Id, Resource: "wsa"}}, + {"workspace dev", "dev", "admins (dev)", &v2.ResourceId{ResourceType: workspaceResourceType.Id, Resource: "wsb"}}, + } { + t.Run(tc.name, func(t *testing.T) { + r, err := groupResource(ctx, &databricks.Group{ID: "9", DisplayName: "admins"}, tc.parent, tc.workspaceName) + if err != nil { + t.Fatalf("groupResource: %v", err) + } + if r.DisplayName != tc.want { + t.Errorf("display name = %q, want %q", r.DisplayName, tc.want) + } + }) + } +} diff --git a/pkg/connector/metastore.go b/pkg/connector/metastore.go index 6d6d072a..0b18c603 100644 --- a/pkg/connector/metastore.go +++ b/pkg/connector/metastore.go @@ -102,9 +102,10 @@ func (m *metastoreBuilder) List(ctx context.Context, parentResourceID *v2.Resour return nil, &rs.SyncOpResults{Annotations: annos}, err } if workspace == nil { - if pending { + if len(pending) > 0 { return nil, &rs.SyncOpResults{Annotations: annos}, status.Errorf(codes.FailedPrecondition, - "databricks-connector: metastore %s has no running workspace in sync scope", metastore.MetastoreID) + "databricks-connector: metastore %s has no running workspace in sync scope, waiting on %s", + metastore.MetastoreID, strings.Join(pending, ", ")) } l.Debug("databricks-connector: metastore has no workspace in sync scope; skipping it", zap.String("metastore", metastore.MetastoreID)) diff --git a/pkg/connector/metastore_helpers.go b/pkg/connector/metastore_helpers.go index c27df6d3..8afe306e 100644 --- a/pkg/connector/metastore_helpers.go +++ b/pkg/connector/metastore_helpers.go @@ -30,7 +30,11 @@ const ( // Statuses the account API documents for a workspace. PROVISIONING and // NOT_PROVISIONED can still become RUNNING. CANCELLING is a workspace being - // deleted: it will not become RUNNING. FAILED and BANNED cannot either. + // deleted, so it will not, but it still holds its metastore in the sync: + // pruning the metastore while the deletion runs would report it to + // ConductorOne as gone and delete its grants, and the status settles on its + // own when the account stops listing the workspace. FAILED and BANNED are the + // terminal set. workspaceStatusRunning = "RUNNING" workspaceStatusProvisioning = "PROVISIONING" workspaceStatusNotProvisioned = "NOT_PROVISIONED" @@ -247,9 +251,11 @@ func (m *metastoreBuilder) scopedWorkspaces(ctx context.Context, annos *annotati return scoped, running, nil } -// workspaceIsUsable reports a status that is RUNNING or can still become it. -// FAILED and BANNED are terminal, and a status Databricks has not documented -// takes that path too: treating one as pending would fail the sync on every run. +// workspaceIsUsable reports a status that still holds a metastore in the sync: +// RUNNING, one that can become it, or CANCELLING, which must not prune while the +// deletion runs. FAILED and BANNED are terminal, and a status Databricks has not +// documented takes that path too: treating one as pending would fail the sync on +// every run with nothing to wait for. func workspaceIsUsable(workspaceStatus string) bool { switch strings.ToUpper(workspaceStatus) { case "", workspaceStatusRunning, workspaceStatusProvisioning, workspaceStatusNotProvisioned, workspaceStatusCancelling: @@ -263,26 +269,29 @@ func workspaceIsUsable(workspaceStatus string) bool { var errMetastoreGone = errors.New("metastore is no longer on the account") // routeMetastore picks a workspace the connector can read every privilege through: -// it owns the metastore, or it is a direct Admin there. A nil workspace with no -// error means none is running and in scope; pending says one can still become RUNNING. +// it owns the metastore, or it is a direct Admin there. A nil workspace with no error +// means none is running and in scope, and the returned slice names the in-scope +// workspaces that still hold the metastore, as " ()". It is +// what the caller reports, so the operator reads the status rather than guessing at a +// misconfiguration. func (m *metastoreBuilder) routeMetastore( ctx context.Context, metastoreID, owner string, scoped map[string]databricks.Workspace, running map[string]bool, annos *annotations.Annotations, -) (*databricks.Workspace, bool, error) { +) (*databricks.Workspace, []string, error) { ids, rateLimit, err := m.client.ListMetastoreWorkspaces(ctx, metastoreID) noteRateLimit(annos, rateLimit) if err != nil { // A 404 here is not an empty assignment list. NotFound on this List makes // the SDK skip the metastore, and ConductorOne deletes it. - return nil, false, refuseUnreadable(fmt.Sprintf( + return nil, nil, refuseUnreadable(fmt.Sprintf( "databricks-connector: failed to list the workspaces assigned to metastore %s, so the sync cannot tell which serve it", metastoreID), err) } - pending := false + var pending []string var candidates []databricks.Workspace for _, id := range ids { workspace, ok := scoped[id] @@ -293,27 +302,27 @@ func (m *metastoreBuilder) routeMetastore( candidates = append(candidates, workspace) continue } - pending = true + pending = append(pending, fmt.Sprintf("%s (%s)", workspace.DeploymentName, workspace.Status)) } if len(candidates) == 0 { return nil, pending, nil } if ownsPrincipal(m.clientID, owner) { - return &candidates[0], false, nil + return &candidates[0], nil, nil } for i := range candidates { admin, err := isWorkspaceAdmin(ctx, m.client, m.clientID, strconv.Itoa(candidates[i].ID), annos) if err != nil { - return nil, false, err + return nil, nil, err } if admin { - return &candidates[i], false, nil + return &candidates[i], nil, nil } } - return nil, false, untrustedMetastoreError(metastoreID, candidates) + return nil, nil, untrustedMetastoreError(metastoreID, candidates) } // resolveMetastoreRoute routes a metastore from its id, reading owner and standing live. @@ -346,8 +355,8 @@ func (m *metastoreBuilder) resolveMetastoreRoute( } if workspace == nil { reason := "no workspace in sync scope is attached to it" - if pending { - reason = "every workspace in sync scope attached to it is still on its way to RUNNING" + if len(pending) > 0 { + reason = fmt.Sprintf("no workspace in sync scope attached to it is RUNNING: %s", strings.Join(pending, ", ")) } return nil, status.Errorf(codes.FailedPrecondition, diff --git a/pkg/connector/metastore_test.go b/pkg/connector/metastore_test.go index a3d5db4d..36fba007 100644 --- a/pkg/connector/metastore_test.go +++ b/pkg/connector/metastore_test.go @@ -750,3 +750,49 @@ func TestMetastoreProvisioningRefusesAPartialPrivilegeList(t *testing.T) { }) } } + +// A cancelling workspace holds its metastore rather than pruning it, because pruning +// during the deletion would report the metastore to ConductorOne as gone and delete its +// grants. The wait is bounded: the account stops listing the workspace when the deletion +// finishes, and the metastore is then left out of the sync with no error. The error that +// covers the wait names the workspace and its status, so it does not read as a +// misconfigured sync scope. +func TestMetastoreCancellingWorkspaceHoldsTheMetastoreUntilItIsGone(t *testing.T) { + cancelling := func() *fakeAccount { + f := defaultFakeAccount() + f.assignments = map[string]any{"ms-a": []int{1}} + f.workspaces = []map[string]any{ + {"workspace_id": 1, "workspace_name": "prod", "deployment_name": "wsa", "workspace_status": workspaceStatusCancelling}, + } + return f + } + + t.Run("while cancelling, the sync waits and says on what", func(t *testing.T) { + m := newTestMetastoreBuilder(t, cancelling(), nil, syncEverything) + resources, _, err := m.List(context.Background(), testAccountResourceID, rs.SyncOpAttrs{}) + if err == nil { + t.Fatal("List succeeded, want the metastore held back while its workspace is deleted") + } + if len(resources) != 0 { + t.Errorf("listed %d metastores, want none", len(resources)) + } + for _, want := range []string{"wsa", workspaceStatusCancelling} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not name %q", err, want) + } + } + }) + + t.Run("once the account stops listing it, the sync recovers", func(t *testing.T) { + f := cancelling() + f.workspaces = nil + m := newTestMetastoreBuilder(t, f, nil, syncEverything) + resources, _, err := m.List(context.Background(), testAccountResourceID, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("List = %v, want no error once the workspace is gone", err) + } + if len(resources) != 0 { + t.Errorf("listed %d metastores, want none: nothing in scope serves it", len(resources)) + } + }) +} diff --git a/pkg/connector/metastore_workspace_admins_test.go b/pkg/connector/metastore_workspace_admins_test.go index 5e6c38ac..3c5114b0 100644 --- a/pkg/connector/metastore_workspace_admins_test.go +++ b/pkg/connector/metastore_workspace_admins_test.go @@ -190,8 +190,8 @@ func TestMetastoreProvisioningRejectsAWorkspaceLocalGroup(t *testing.T) { } // Metastores are opt-in while workspaces always sync, so reading Unity Catalog to decide -// what a workspace offers turned a green sync red for every tenant that cannot read it. -// Only a RUNNING workspace serves SCIM, so only it offers its admins group. +// what a workspace offers would fail the whole workspace type on a tenant that cannot read +// that plane. Only a RUNNING workspace serves SCIM, so only it offers its admins group. func TestWorkspaceListDoesNotDependOnUnityCatalog(t *testing.T) { f := defaultFakeAccount() f.assignments = map[string]any{"ms-a": http.StatusForbidden} diff --git a/pkg/connector/raw_id_test.go b/pkg/connector/raw_id_test.go index 1f234b20..921c1527 100644 --- a/pkg/connector/raw_id_test.go +++ b/pkg/connector/raw_id_test.go @@ -32,12 +32,12 @@ func TestResourcesHaveRawId(t *testing.T) { }{ { "account group", - build(groupResource(ctx, &databricks.Group{BaseResponse: databricks.BaseResponse{ID: "g1"}}, account)), + build(groupResource(ctx, &databricks.Group{BaseResponse: databricks.BaseResponse{ID: "g1"}}, account, "")), "account/acc-1/group/g1", }, { "workspace group", - build(groupResource(ctx, &databricks.Group{BaseResponse: databricks.BaseResponse{ID: "g1"}}, workspace)), + build(groupResource(ctx, &databricks.Group{BaseResponse: databricks.BaseResponse{ID: "g1"}}, workspace, "")), "workspace/ws-1/group/g1", }, { diff --git a/pkg/connector/workspaces_provisioning_test.go b/pkg/connector/workspaces_provisioning_test.go index c1869b5a..b2bc6a6f 100644 --- a/pkg/connector/workspaces_provisioning_test.go +++ b/pkg/connector/workspaces_provisioning_test.go @@ -115,6 +115,7 @@ func testProvisioningGroup(t *testing.T) *v2.Resource { r, err := groupResource(context.Background(), &databricks.Group{ID: testProvisioningPrincipalID, DisplayName: "admins"}, &v2.ResourceId{ResourceType: accountResourceType.Id, Resource: testAccountId}, + "", ) if err != nil { t.Fatalf("groupResource: %v", err) From 7ac05f215cf577bf3e6ed58bd326caf526aa2239 Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 3 Nov 2026 17:30:16 -0300 Subject: [PATCH 3/5] docs(databricks): say why metastore routing waits for a cancelling workspace Two comments still described CANCELLING as dropped from scope, which is what workspaceIsUsable deliberately does not do: dropping the workspace prunes the metastores that routed through it, and ConductorOne then deletes grants that are still in force while the deletion runs. The comment on the status constants already says this; the one on the scope loop and the one on the status test did not. The two answers are both intentional. The workspace syncer offers no admins group for a cancelling workspace, because it will never be RUNNING. Metastore routing keeps it, because the metastore is still there until the account stops listing the workspace. No behaviour changes. Co-Authored-By: Claude Opus 5 --- pkg/connector/metastore_helpers.go | 8 +++++--- pkg/connector/metastore_test.go | 6 ++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/connector/metastore_helpers.go b/pkg/connector/metastore_helpers.go index 8afe306e..77c2e6cb 100644 --- a/pkg/connector/metastore_helpers.go +++ b/pkg/connector/metastore_helpers.go @@ -238,9 +238,11 @@ func (m *metastoreBuilder) scopedWorkspaces(ctx context.Context, annos *annotati continue } } - // A workspace that will not become RUNNING is left out of scope entirely, - // so a metastore attached only to one is skipped rather than failing every - // sync. Only a status that can still change keeps the metastore waiting. + // A workspace that will never serve its APIs is left out of scope, which + // lets ConductorOne prune the metastores that routed through it. A status + // that can still change keeps the metastore waiting instead, and so does + // CANCELLING, because pruning a metastore while its workspace is still + // being deleted would delete grants that are still in force. if !workspaceIsUsable(workspace.Status) { continue } diff --git a/pkg/connector/metastore_test.go b/pkg/connector/metastore_test.go index 36fba007..5a1bfbce 100644 --- a/pkg/connector/metastore_test.go +++ b/pkg/connector/metastore_test.go @@ -296,8 +296,10 @@ func TestMetastoreListFailsWhenAnAssignmentListCannotBeRead(t *testing.T) { } } -// A workspace that will not become RUNNING leaves the metastore out of the sync -// instead of failing it on every run; one that can still change keeps it waiting. +// A terminal workspace leaves the metastore out of the sync instead of failing it +// on every run. One that can still change keeps it waiting, and so does +// CANCELLING: the deletion is still running, and pruning the metastore now would +// delete grants that are still in force. func TestMetastoreListSeparatesTerminalFromPendingWorkspaces(t *testing.T) { tests := []struct { workspaceStatus string From f373bd1cfa2b049c1db6e2b51ca7b5f8de74aa6d Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Tue, 3 Nov 2026 17:55:03 -0300 Subject: [PATCH 4/5] test(databricks): make the workspace group mock apply the SCIM filter The mock answered every workspace group listing with the admins group and never read the filter, so both List tests passed whether or not `displayName eq "admins"` was sent. It now serves three groups and filters them: dropping the filter fails TestWorkspaceGroupListStopsAfterTheAdminsGroup and TestWorkspaceGroupNameNamesItsWorkspace instead of passing silently. The group name is a shared constant now, because the listing filter and the alias check in metastore_workspace_admins.go have to agree on it and each spelled it out. The List godoc said it returns all groups, which stopped being true under a workspace parent, and the alias comment in metastore Grants said the alias skips three account lookups without saying that a row it cannot resolve still runs them. Co-Authored-By: Claude Opus 5 --- pkg/connector/groups.go | 14 ++++++++++--- pkg/connector/groups_workspace_scope_test.go | 21 +++++++++++++++++++- pkg/connector/metastore.go | 6 ++++-- pkg/connector/metastore_workspace_admins.go | 4 ++-- 4 files changed, 37 insertions(+), 8 deletions(-) diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index a4c2b4f8..4551ca10 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -40,6 +40,11 @@ func groupResourceId(_ context.Context, groupId string, parentResourceId *v2.Res return strings.Join([]string{parentResourceId.ResourceType, parentResourceId.Resource, groupResourceType.Id, groupId}, "/") } +// workspaceAdminsGroupName is the name Databricks gives the built-in admins +// group in every workspace. The group is addressed by this name, not by an id, +// so the listing filter and the alias check must agree on it. +const workspaceAdminsGroupName = "admins" + // groupResource builds a group resource. workspaceName qualifies the display name of a // workspace-scoped group: Databricks names the built-in group "admins" in every // workspace, so a metastore shared by several of them would otherwise list each holder @@ -103,8 +108,11 @@ func (g *groupBuilder) workspaceDisplayName(ctx context.Context, deploymentName return workspace.Name, nil } -// List returns all the groups from the database as resource objects. -// Groups include a GroupTrait because they are the 'shape' of a standard group. +// List returns groups as resource objects, with a GroupTrait because they are the +// 'shape' of a standard group. Under the account it returns every group. Under a +// workspace parent it returns only the built-in admins group, on a single page: +// the other workspace-local groups are not synced, and the privileges this one +// holds on a metastore are what the workspace parent exists for. func (g *groupBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, attr rs.SyncOpAttrs) ([]*v2.Resource, *rs.SyncOpResults, error) { if parentResourceID == nil { return nil, nil, nil @@ -114,7 +122,7 @@ func (g *groupBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId vars := []databricks.Vars{databricks.NewGroupAttrVars()} if parentResourceID.ResourceType == workspaceResourceType.Id { workspaceId = parentResourceID.Resource - vars = append(vars, databricks.NewFilterVars(`displayName eq "admins"`)) + vars = append(vars, databricks.NewFilterVars(`displayName eq "`+workspaceAdminsGroupName+`"`)) } bag, page, err := parsePageToken(attr.PageToken.Token, &v2.ResourceId{ResourceType: groupResourceType.Id}) diff --git a/pkg/connector/groups_workspace_scope_test.go b/pkg/connector/groups_workspace_scope_test.go index ceda7cb5..55873ba6 100644 --- a/pkg/connector/groups_workspace_scope_test.go +++ b/pkg/connector/groups_workspace_scope_test.go @@ -76,8 +76,27 @@ func (s *workspaceScimServer) handler(t *testing.T) http.HandlerFunc { encode(group) return } + // A workspace holds many groups and only the filtered one may come back, so + // the mock applies the filter: without it the listing answers with all of + // them and a caller that dropped the filter fails instead of passing. + all := []map[string]any{ + {"id": "wg-eng", "displayName": "engineering"}, + group, + {"id": "wg-ds", "displayName": "data-science"}, + } + matched := all + if filter := r.URL.Query().Get("filter"); filter != "" { + _, want, _ := strings.Cut(filter, " eq ") + want = strings.Trim(want, `"`) + matched = nil + for _, candidate := range all { + if candidate["displayName"] == want { + matched = append(matched, candidate) + } + } + } // A real workspace reports every group it has in totalResults, not the filtered count. - encode(map[string]any{"totalResults": 40, "Resources": []map[string]any{group}}) + encode(map[string]any{"totalResults": 40, "Resources": matched}) } } diff --git a/pkg/connector/metastore.go b/pkg/connector/metastore.go index 0b18c603..603396ce 100644 --- a/pkg/connector/metastore.go +++ b/pkg/connector/metastore.go @@ -170,8 +170,10 @@ func (m *metastoreBuilder) Grants(ctx context.Context, resource *v2.Resource, at workspaceAdmins := &workspaceAdminResolver{metastore: m, annos: &annos} var rv []*v2.Grant for _, assignment := range assignments { - // The alias names its own workspace, so resolving it first skips three account - // lookups that would always miss. + // The alias names its own workspace, so resolving it first answers without the + // three account lookups that would always miss. It answers nothing when the + // workspace is out of scope, is not RUNNING, has no admins group or carries + // another name, and the account resolver still runs for those rows. var principalId *v2.ResourceId var err error workspaceAdmin := false diff --git a/pkg/connector/metastore_workspace_admins.go b/pkg/connector/metastore_workspace_admins.go index 445246c1..d87830a5 100644 --- a/pkg/connector/metastore_workspace_admins.go +++ b/pkg/connector/metastore_workspace_admins.go @@ -56,7 +56,7 @@ func (r *workspaceAdminResolver) resolve(ctx context.Context, assignment databri if id == "" || id == "0" { var rateLimit *v2.RateLimitDescription var err error - id, rateLimit, err = r.metastore.client.FindGroupID(ctx, workspace.DeploymentName, "admins") + id, rateLimit, err = r.metastore.client.FindGroupID(ctx, workspace.DeploymentName, workspaceAdminsGroupName) noteRateLimit(r.annos, rateLimit) if err != nil { return nil, refuseUnreadable("databricks-connector: failed to resolve workspace admins group", err) @@ -70,7 +70,7 @@ func (r *workspaceAdminResolver) resolve(ctx context.Context, assignment databri if err != nil { return nil, refuseUnreadable("databricks-connector: failed to resolve workspace admins group", err) } - if name != "admins" { + if name != workspaceAdminsGroupName { return nil, nil // Never map an alias to another group or reuse a deleted ID's name. } } From f6cc3aa8d1f68685155d57b5d64af5af2c6ffa5c Mon Sep 17 00:00:00 2001 From: Mateo Hernandez Date: Wed, 4 Nov 2026 13:39:38 -0300 Subject: [PATCH 5/5] perf(databricks): resolve a workspace admins page in one read per member type accountHolds asked the account SCIM endpoint about one member at a time, so a workspace admins group of n members cost n filtered reads. The members of a page are now resolved in one request per member type, batched at 100 "id eq" terms because Databricks refuses a SCIM filter above 200 nodes and an n-term chain is 2n-1 of them. A filtered read that answers fewer rows than it matched now fails instead of reading as "the account does not hold these ids". The members a short page left out would be skipped, and a membership grant missing from a sync is revoked access in ConductorOne. The test mock now applies the whole or-chain rather than its first term, which a caller that dropped the rest would otherwise have passed. Co-Authored-By: Claude Opus 5 --- pkg/connector/groups.go | 87 ++++++++----- pkg/connector/groups_workspace_scope_test.go | 126 +++++++++++++++++-- pkg/databricks/client.go | 93 ++++++++++++++ 3 files changed, 266 insertions(+), 40 deletions(-) diff --git a/pkg/connector/groups.go b/pkg/connector/groups.go index 4551ca10..369be6ca 100644 --- a/pkg/connector/groups.go +++ b/pkg/connector/groups.go @@ -224,35 +224,51 @@ func (g *groupBuilder) Entitlements(ctx context.Context, resource *v2.Resource, return rv, nil, nil } -// accountHolds reports whether the account plane holds the member a workspace group -// lists. The lookups are filtered lists, so an id of the wrong kind answers with no -// match rather than the 400 a by-id read returns. -func (g *groupBuilder) accountHolds(ctx context.Context, memberType, memberID string) (bool, *v2.RateLimitDescription, error) { - switch memberType { - case "Users": - userName, rateLimitData, err := g.client.FindUsername(ctx, "", memberID) +// splitMemberRef splits a SCIM member ref such as "Users/123" into its type and id. +func splitMemberRef(ref string) (string, string, error) { + memberType, memberID, found := strings.Cut(ref, "/") + if !found || memberType == "" || memberID == "" || strings.Contains(memberID, "/") { + return "", "", fmt.Errorf("databricks-connector: invalid member format of %s", ref) + } + + return memberType, memberID, nil +} + +// accountHeld reports, per member type, which member ids the account holds. It resolves the +// whole page in one request per type, and a member the account does not hold is absent. +func (g *groupBuilder) accountHeld(ctx context.Context, members []databricks.Member) (map[string]map[string]struct{}, *v2.RateLimitDescription, error) { + idsByType := make(map[string][]string) + for _, member := range members { + memberType, memberID, err := splitMemberRef(member.Ref) if err != nil { - return false, rateLimitData, fmt.Errorf("databricks-connector: failed to look up user %s on the account: %w", memberID, err) + return nil, nil, err } - return userName != "", rateLimitData, nil - case "Groups": - displayName, rateLimitData, err := g.client.FindGroupDisplayName(ctx, "", memberID) - if err != nil { - return false, rateLimitData, fmt.Errorf("databricks-connector: failed to look up group %s on the account: %w", memberID, err) + switch memberType { + case "Users", "Groups", "ServicePrincipals": + idsByType[memberType] = append(idsByType[memberType], memberID) + default: + return nil, nil, fmt.Errorf("databricks-connector: invalid member type: %s", memberType) + } + } + + held := make(map[string]map[string]struct{}, len(idsByType)) + var rateLimitData *v2.RateLimitDescription + + for memberType, ids := range idsByType { + heldIDs, typeRateLimitData, err := g.client.FindAccountHeldIDs(ctx, memberType, ids) + if typeRateLimitData != nil { + rateLimitData = typeRateLimitData } - return displayName != "", rateLimitData, nil - case "ServicePrincipals": - appID, rateLimitData, err := g.client.FindServicePrincipalAppID(ctx, "", memberID) if err != nil { - return false, rateLimitData, fmt.Errorf("databricks-connector: failed to look up service principal %s on the account: %w", memberID, err) + return nil, rateLimitData, fmt.Errorf("databricks-connector: failed to look up %s on the account: %w", memberType, err) } - return appID != "", rateLimitData, nil - default: - return false, nil, fmt.Errorf("databricks-connector: invalid member type: %s", memberType) + held[memberType] = heldIDs } + + return held, rateLimitData, nil } // Grants return all grants relevant to the group. @@ -303,26 +319,33 @@ func (g *groupBuilder) Grants(ctx context.Context, resource *v2.Resource, attr r zap.String("cursor", cursor), ) + // Only the account syncs members, so one it does not hold is workspace-local and has no + // resource to point a grant at. + var held map[string]map[string]struct{} + if isWorkspaceGroup { + heldByType, rateLimitData, err := g.accountHeld(ctx, members) + if rateLimitData != nil { + annos.WithRateLimiting(rateLimitData) + } + if err != nil { + return nil, nil, err + } + + held = heldByType + } + for _, member := range members { // member.Ref contains the type and ID separated by "/", e.g., "Users/123" or "Groups/456" - pp := strings.Split(member.Ref, "/") - if len(pp) != 2 { - return nil, nil, fmt.Errorf("databricks-connector: invalid member format of %s", member.Ref) + memberType, memberID, err := splitMemberRef(member.Ref) + if err != nil { + return nil, nil, err } - memberType, memberID := pp[0], pp[1] var resourceId *v2.ResourceId var anns []protoreflect.ProtoMessage - // Only the account syncs members, so one it does not hold is workspace-local - // and has no resource to point a grant at. if isWorkspaceGroup { - held, rateLimitData, err := g.accountHolds(ctx, memberType, memberID) - annos.WithRateLimiting(rateLimitData) - if err != nil { - return nil, nil, err - } - if !held { + if _, ok := held[memberType][memberID]; !ok { l.Debug("databricks-connector: skipping a workspace-local member the account does not hold", zap.String("group", resource.Id.Resource), zap.String("member", member.Ref)) diff --git a/pkg/connector/groups_workspace_scope_test.go b/pkg/connector/groups_workspace_scope_test.go index 55873ba6..1e25d34a 100644 --- a/pkg/connector/groups_workspace_scope_test.go +++ b/pkg/connector/groups_workspace_scope_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "net/http" "slices" + "strconv" "strings" "testing" @@ -23,6 +24,13 @@ type workspaceScimServer struct { // workspaceName is the name the account lists for deployment "wsa". Empty keeps the // default, so a test that does not care about the display name reads the same. workspaceName string + // accountReads counts the filtered reads the connector sends to the account plane. + accountReads int + // maxFilterTerms refuses a filter wider than Databricks does. Zero accepts any width. + maxFilterTerms int + // pageCap serves fewer rows than matched while still reporting the true totalResults, the + // way a server that clamps the page would. Zero serves every match. + pageCap int } const testAdminsGroupID = "9" @@ -48,16 +56,36 @@ func (s *workspaceScimServer) handler(t *testing.T) http.HandlerFunc { kind, id, _ := strings.Cut(rest, "/") if account { - filter := r.URL.Query().Get("filter") - _, value, _ := strings.Cut(filter, " eq ") - value = strings.Trim(value, `"`) - if slices.Contains(s.accountHolds[kind], value) { - encode(map[string]any{"totalResults": 1, "Resources": []map[string]any{ - {"id": value, "userName": "u" + value, "displayName": "g" + value, "applicationId": "a" + value}, - }}) + // One row per term it holds, as the account does: matching only the first term would + // let a caller that dropped the rest still pass. + s.accountReads++ + terms := strings.Split(r.URL.Query().Get("filter"), " or ") + // Databricks counts an n-term chain as 2n-1 nodes and refuses above 200. + if s.maxFilterTerms > 0 && len(terms) > s.maxFilterTerms { + w.WriteHeader(http.StatusBadRequest) + encode(map[string]any{ + "schemas": []string{"urn:ietf:params:scim:api:messages:2.0:Error"}, + "scimType": "InvalidFilter", + "detail": "SCIM filter exceeds max nodes: 200", + "status": "400", + }) return } - encode(map[string]any{"totalResults": 0}) + var resources []map[string]any + for _, term := range terms { + _, value, _ := strings.Cut(term, " eq ") + value = strings.Trim(strings.TrimSpace(value), `"`) + if slices.Contains(s.accountHolds[kind], value) { + resources = append(resources, map[string]any{ + "id": value, "userName": "u" + value, "displayName": "g" + value, "applicationId": "a" + value, + }) + } + } + matched := len(resources) + if s.pageCap > 0 && len(resources) > s.pageCap { + resources = resources[:s.pageCap] + } + encode(map[string]any{"totalResults": matched, "Resources": resources}) return } @@ -236,3 +264,85 @@ func TestGroupNameQualifiedOnlyUnderAWorkspace(t *testing.T) { }) } } + +// Member resolution reads the account once per member type, splits a list wider than one +// filter, and fails a clamped page instead of reading it as "the account does not hold these". +func TestWorkspaceGroupGrantsResolveMembersInBatches(t *testing.T) { + users := func(n int) ([]string, []string) { + refs := make([]string, 0, n) + ids := make([]string, 0, n) + for i := range n { + id := strconv.Itoa(1000 + i) + refs = append(refs, "Users/"+id) + ids = append(ids, id) + } + + return refs, ids + } + manyRefs, manyIDs := users(250) + + for _, tt := range []struct { + name string + members []string + holds map[string][]string + pageCap int + wantGrants int + wantReads int + wantErr string + }{ + { + name: "one read per member type, not one per member", + members: []string{"Users/100", "Users/101", "Users/102", "Users/103", "Groups/200", "Groups/201", "ServicePrincipals/300"}, + holds: map[string][]string{ + "Users": {"100", "101", "102", "103"}, "Groups": {"200", "201"}, "ServicePrincipals": {"300"}, + }, + wantGrants: 7, + wantReads: 3, + }, + { + name: "a list wider than one filter is split", + members: manyRefs, + holds: map[string][]string{"Users": manyIDs}, + wantGrants: 250, + wantReads: 3, + }, + { + name: "a clamped page fails instead of dropping members", + members: []string{"Users/100", "Users/101", "Users/102"}, + holds: map[string][]string{"Users": {"100", "101", "102"}}, + pageCap: 2, + wantErr: "of 3 matched Users", + }, + } { + t.Run(tt.name, func(t *testing.T) { + srv := &workspaceScimServer{ + adminsMembers: tt.members, accountHolds: tt.holds, + maxFilterTerms: 100, pageCap: tt.pageCap, + } + client, _ := newWorkspaceRoutedClient(t, srv.handler(t)) + + grants, _, err := newGroupBuilder(client).Grants(context.Background(), testWorkspaceGroupResource(t), rs.SyncOpAttrs{}) + + if tt.wantErr != "" { + if err == nil { + t.Fatalf("Grants returned %d grants, want an error naming the short page", len(grants)) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("error = %v, want it to contain %q", err, tt.wantErr) + } + + return + } + + if err != nil { + t.Fatalf("Grants: %v", err) + } + if len(grants) != tt.wantGrants { + t.Errorf("grants = %d, want %d", len(grants), tt.wantGrants) + } + if srv.accountReads != tt.wantReads { + t.Errorf("account reads = %d, want %d for %d members", srv.accountReads, tt.wantReads, len(tt.members)) + } + }) + } +} diff --git a/pkg/databricks/client.go b/pkg/databricks/client.go index 2ff8722c..28329778 100644 --- a/pkg/databricks/client.go +++ b/pkg/databricks/client.go @@ -587,6 +587,99 @@ func (c *Client) FindServicePrincipalAppID( return servicePrincipals[0].ApplicationID, ratelimitData, nil } +// scimFilterMaxTerms is the widest "id eq" or-chain the account accepts: Databricks refuses a +// SCIM filter above 200 nodes and an n-term chain is 2n-1 of them. +const scimFilterMaxTerms = 100 + +// scimIDFilter builds the "id eq ... or id eq ..." filter that reads ids in one request. +func scimIDFilter(ids []string) string { + terms := make([]string, 0, len(ids)) + for _, id := range ids { + terms = append(terms, "id eq "+strconv.Quote(id)) + } + + return strings.Join(terms, " or ") +} + +// FindAccountHeldIDs reports which of ids the account holds as memberType, one request per +// batch of scimFilterMaxTerms rather than one per id. An id the account does not hold is +// absent from the set, so a page shorter than the filter matched is an error, not an answer. +func (c *Client) FindAccountHeldIDs( + ctx context.Context, + memberType string, + ids []string, +) ( + map[string]struct{}, + *v2.RateLimitDescription, + error, +) { + switch memberType { + case "Users", "Groups", "ServicePrincipals": + default: + return nil, nil, fmt.Errorf("invalid member type: %s", memberType) + } + + held := make(map[string]struct{}, len(ids)) + var rateLimitData *v2.RateLimitDescription + + for start := 0; start < len(ids); start += scimFilterMaxTerms { + end := min(start+scimFilterMaxTerms, len(ids)) + batch := ids[start:end] + + vars := []Vars{ + &PaginationVars{Count: uint(len(batch))}, + NewFilterVars(scimIDFilter(batch)), + } + + var ( + found []string + matched uint + batchRateLimitData *v2.RateLimitDescription + err error + ) + + switch memberType { + case "Users": + var users []User + users, matched, batchRateLimitData, err = c.ListUsers(ctx, "", vars...) + for _, u := range users { + found = append(found, u.ID) + } + case "Groups": + var groups []Group + groups, matched, batchRateLimitData, err = c.ListGroups(ctx, "", vars...) + for _, g := range groups { + found = append(found, g.ID) + } + case "ServicePrincipals": + var servicePrincipals []ServicePrincipal + servicePrincipals, matched, batchRateLimitData, err = c.ListServicePrincipals(ctx, "", vars...) + for _, sp := range servicePrincipals { + found = append(found, sp.ID) + } + } + + if batchRateLimitData != nil { + rateLimitData = batchRateLimitData + } + + if err != nil { + return nil, rateLimitData, err + } + + if matched > uint(len(found)) { + return nil, rateLimitData, fmt.Errorf( + "account returned %d of %d matched %s for a filtered read of %d ids", len(found), matched, memberType, len(batch)) + } + + for _, id := range found { + held[id] = struct{}{} + } + } + + return held, rateLimitData, nil +} + func (c *Client) ListRoles( ctx context.Context, workspaceId string,