diff --git a/README.md b/README.md index ec789267..12b80715 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ `baton-databricks` is a connector for Databricks built using the [Baton SDK](https://github.com/conductorone/baton-sdk). It communicates with the Databricks API, to sync data about Databricks identities (users, groups and -service principals), roles, workspaces and Unity Catalog metastores. +service principals), roles, workspaces and Unity Catalog metastores and catalogs. Check out [Baton](https://github.com/conductorone/baton) to learn more about the project in general. @@ -82,19 +82,22 @@ baton resources - Users - Roles - Metastores +- Catalogs (under their metastore) The connector fetches all resources from the account and every workspace the service principal can access. To limit the scope, pass a comma-separated list of workspaces to the `--workspaces` flag. Each entry can be a workspace name, deployment name, or numeric workspace ID, matched case-insensitively. -`--workspaces` also scopes which metastores sync: only a metastore attached to a -listed workspace is synced, so narrowing the list removes the other metastores -and their grants from ConductorOne. A list that names no workspace on the +`--workspaces` also scopes which metastores and catalogs sync: only a metastore +attached to a listed workspace is synced, so narrowing the list removes the +other metastores and their grants from ConductorOne. Catalogs must also be +visible through a listed workspace. A list that names no workspace on the account is rejected during credential validation, before any sync runs. -Metastores are Unity Catalog securables and are opt-in: the type stays off until -it is enabled for the connector in ConductorOne. +Metastores and catalogs are opt-in: each resource type stays off until enabled +for the connector in ConductorOne. Catalog sync uses the existing OAuth +credentials and configuration fields. ## Authentication @@ -113,24 +116,16 @@ To instead exclude specific workspaces from the sync, pass them to the list. Each entry can be a workspace name, deployment name, or numeric workspace ID. Excluded workspaces and their roles are skipped entirely. A metastore attached only to excluded workspaces is not synced, and its grants are removed -from ConductorOne. +from ConductorOne. Catalogs reachable only through excluded workspaces are also +left out. ## Incremental sync By default, `baton-databricks` does a full resync of every resource on every run. You can opt into an additional, cheap pathway that polls a Databricks audit log between full syncs to pick up access changes early. Full syncs still run as the -correctness backstop. Incremental sync picks up added and removed group -memberships, but deleted users, groups and service principals are only caught -by the next full sync. - -Role assignments that Databricks stores in access-control rule sets (group roles -such as `roles/group.manager`, service principal roles, and the account -`marketplace.admin` role) are also only caught by the next full sync: the -connector does not read the audit events Databricks writes for rule-set -changes. A `roles/group.manager` change, for example, is logged as -`updateRuleSet` under the `accountsAccessControl` audit service, which the -connector does not query. +correctness backstop; incremental sync does not detect deletions, which are +only caught by the next full sync. **Incremental sync is enabled by setting `--sql-warehouse-id` (or `BATON_SQL_WAREHOUSE_ID`) and disabled by leaving it unset or empty.** There is @@ -166,8 +161,7 @@ Incremental sync works only when all of the following are true: - `USE SCHEMA` and `SELECT` on the `system.access` schema - `CAN USE` on the SQL warehouse - For example, run as a metastore admin, replacing `` with the - service principal's application (client) ID: + For example, run as a metastore admin: ```sql GRANT USE CATALOG ON CATALOG system TO ``; @@ -175,21 +169,16 @@ Incremental sync works only when all of the following are true: ``` **We recommend a serverless SQL warehouse.** It starts in seconds, while a pro -or classic warehouse can take several minutes. Each statement gets up to 30 -seconds on the initial request, including warehouse startup, then up to 5 more -minutes of polling. A statement still unfinished after that is cancelled, so a -poll can take close to 6 minutes before it fails with a `DeadlineExceeded` -error and is retried on the next poll. Serverless SQL warehouses require the -Premium plan or above. +or classic warehouse can take several minutes. Each poll waits at most 5 minutes +for its statement, including warehouse startup; a poll that runs out of time +fails with a `DeadlineExceeded` error and is retried on the next poll. Serverless +SQL warehouses require the Premium plan or above. **Incremental sync covers one cloud region.** `system.access.audit` holds workspace-level events only for workspaces in the same cloud region as the workspace that queries it; account-level events are global ([system tables reference](https://docs.databricks.com/aws/en/admin/system-tables/)). The -connector finds the warehouse's workspace automatically by checking each -workspace the service principal can access, except those excluded with -`--databricks-exclude-workspaces`; the `--workspaces` allowlist does not limit -this search. It receives +connector finds the warehouse's workspace automatically, and it receives incremental updates only for workspaces in that region. Workspaces in other regions are still covered by full syncs. @@ -232,6 +221,7 @@ account ├── group ├── service principal └── metastore + └── catalog ``` A metastore exposes its privileges as entitlements (`CREATE_CATALOG`, @@ -248,21 +238,54 @@ running one in scope where it can read every privilege, which it proves by owning the metastore or by holding Admin directly on that workspace. Grant and Revoke resolve the same route from the metastore id. -A metastore syncs through a running workspace in scope. The sync fails, naming -the metastore, while an attached workspace in scope is still provisioning, or -while the metastore's workspace list cannot be read. +A metastore is synced only while a workspace this sync can use is attached to +it. The connector never guesses when the answer is unknown: + +| Attached workspace | Result | +|--------------------|--------| +| `RUNNING` and in scope | the metastore syncs through it | +| `PROVISIONING`, `NOT_PROVISIONED` | still changing, so the metastore is kept and the sync fails with `FailedPrecondition` until a workspace is `RUNNING` | +| `CANCELLING` | the workspace is being deleted and will not become `RUNNING`, so the metastore is kept and the sync fails until that workspace is gone or another attached one is `RUNNING` | +| `FAILED`, `BANNED`, or a status Databricks does not document | will not become `RUNNING`, so it is skipped; a metastore left with no other workspace is not synced | +| metastore workspace list unreadable (401, 403, 404) | unknown, so the metastore is kept and the sync fails naming that list | +| workspace permission list unreadable (401, 403, 404) | treated as not Admin on that workspace; the sync then fails with the usual missing-standing error, which does not name this list | + +A metastore is left out of the sync only when the answer is known: every +attached workspace is out of scope or unusable. Leaving it out is what makes +ConductorOne remove it and its grants, so an unknown answer must not take that +path. + +Catalog discovery visits every running workspace in scope attached to the +metastore, so catalogs bound to only some workspaces are included. Each catalog +appears once under its metastore. The built-in `system` catalog and all catalogs +Databricks identifies as `SYSTEM_CATALOG` (including `samples` when it has that +type) are excluded, regardless of ownership. ## Unity Catalog provisioning -A metastore privilege can be granted to and revoked from a user, group or -service principal. The `owner` entitlement is read-only: granting or revoking it -fails with an `InvalidArgument` error. +A metastore or catalog privilege can be granted to and revoked from a user, +group or service principal. The `owner` entitlement is read-only: granting or +revoking it fails with an `InvalidArgument` error. + +If a catalog becomes unreachable, provisioning must distinguish deletion from +a workspace binding or visibility change. Requests can fail with +`FailedPrecondition` when deletion cannot be proved. ## Unity Catalog permissions The service principal must own the metastore, or be assigned **Admin** directly on at least one running workspace attached to it. Admin inherited only through a -group is not enough. Otherwise the sync fails naming the metastore. +group is not enough. Databricks answers an under-permissioned read with HTTP 200 +and only the rows the caller may see, so the connector reads a metastore only +through a workspace that passes that check. When no attached workspace does, the +sync fails and ConductorOne keeps the grants it already has. + +For a catalog's full grants list, the service principal must own the metastore +or catalog, hold `MANAGE` on the catalog, or have Admin assigned directly on the +workspace whose admins group owns that catalog. Workspace Admin alone does not +cover other catalogs. A non-owner using `MANAGE` also needs `USE_CATALOG` for +catalog discovery. An incomplete grants list fails the sync rather than +removing grants the connector cannot see. The connector requests the `all-apis` OAuth scope. Each resource type declares the scopes, roles and privileges it needs, so `./baton-databricks capabilities` @@ -321,7 +344,7 @@ Flags: -p, --provisioning This must be set in order for provisioning actions to be enabled ($BATON_PROVISIONING) --skip-entitlements-and-grants This must be set to skip syncing of entitlements and grants ($BATON_SKIP_ENTITLEMENTS_AND_GRANTS) --skip-full-sync This must be set to skip a full sync ($BATON_SKIP_FULL_SYNC) - --sql-warehouse-id string Setting this enables incremental sync; leaving it empty disables it. ID of the Databricks SQL warehouse used to query the system.access.audit log between full syncs, so access changes show up before the next full sync (deleted users, groups and service principals are still only caught by full syncs). The warehouse can be in any workspace not excluded by databricks-exclude-workspaces, in the same cloud region as the workspaces to update incrementally; Databricks only exposes workspace-level audit events for that region. The connector discovers the warehouse's workspace automatically. ($BATON_SQL_WAREHOUSE_ID) + --sql-warehouse-id string Setting this enables incremental sync; leaving it empty disables it. ID of the Databricks SQL warehouse used to query the system.access.audit log between full syncs, so access changes show up before the next full sync (deletions are still only caught by full syncs). The warehouse can be in any workspace in the same cloud region as the workspaces to update incrementally; Databricks only exposes workspace-level audit events for that region. The connector discovers the warehouse's workspace automatically. ($BATON_SQL_WAREHOUSE_ID) --storage-engine string The storage engine to use when opening the sync c1z file: sqlite or pebble. Defaults to pebble when unset. ($BATON_STORAGE_ENGINE) --sync-resource-types strings The resource type IDs to sync ($BATON_SYNC_RESOURCE_TYPES) --sync-resources strings The resource IDs to sync ($BATON_SYNC_RESOURCES) diff --git a/baton_capabilities.json b/baton_capabilities.json index 3f948d7d..6a663099 100644 --- a/baton_capabilities.json +++ b/baton_capabilities.json @@ -35,6 +35,79 @@ ] } }, + { + "resourceType": { + "id": "catalog", + "displayName": "Catalog", + "traits": [ + "TRAIT_APP" + ], + "annotations": [ + { + "@type": "type.googleapis.com/c1.connector.v2.OptInRequired" + }, + { + "@type": "type.googleapis.com/c1.connector.v2.CapabilityPermissions", + "permissions": [ + { + "permission": "unity-catalog" + }, + { + "permission": "scim" + }, + { + "permission": "access-management" + }, + { + "permission": "provisioning" + }, + { + "permission": "Account admin" + }, + { + "permission": "Workspace admin" + }, + { + "permission": "USE_CATALOG" + } + ] + }, + { + "@type": "type.googleapis.com/c1.connector.v2.SkipEntitlements" + } + ] + }, + "capabilities": [ + "CAPABILITY_SYNC", + "CAPABILITY_PROVISION" + ], + "permissions": { + "permissions": [ + { + "permission": "unity-catalog" + }, + { + "permission": "scim" + }, + { + "permission": "access-management" + }, + { + "permission": "provisioning" + }, + { + "permission": "Account admin" + }, + { + "permission": "Workspace admin" + }, + { + "permission": "USE_CATALOG" + } + ] + }, + "optInRequired": true + }, { "resourceType": { "id": "group", diff --git a/cmd/baton-databricks/main.go b/cmd/baton-databricks/main.go index fbcfd994..54c6e96e 100644 --- a/cmd/baton-databricks/main.go +++ b/cmd/baton-databricks/main.go @@ -6,11 +6,12 @@ import ( cfg "github.com/conductorone/baton-databricks/pkg/config" "github.com/conductorone/baton-databricks/pkg/connector" "github.com/conductorone/baton-sdk/pkg/config" + "github.com/conductorone/baton-sdk/pkg/connectorrunner" ) var version = "dev" func main() { ctx := context.Background() - config.RunConnector(ctx, "baton-databricks", version, cfg.Config, connector.NewConnector) + config.RunConnector(ctx, "baton-databricks", version, cfg.Config, connector.NewConnector, connectorrunner.WithSessionStoreEnabled()) } diff --git a/config_schema.json b/config_schema.json index 65d90702..b12191ad 100644 --- a/config_schema.json +++ b/config_schema.json @@ -155,7 +155,7 @@ { "name": "sql-warehouse-id", "displayName": "SQL Warehouse ID", - "description": "Setting this enables incremental sync; leaving it empty disables it. ID of the Databricks SQL warehouse used to query the system.access.audit log between full syncs, so access changes show up before the next full sync (deleted users, groups and service principals are still only caught by full syncs). The warehouse can be in any workspace not excluded by databricks-exclude-workspaces, in the same cloud region as the workspaces to update incrementally; Databricks only exposes workspace-level audit events for that region. The connector discovers the warehouse's workspace automatically.", + "description": "Setting this enables incremental sync; leaving it empty disables it. ID of the Databricks SQL warehouse used to query the system.access.audit log between full syncs, so access changes show up before the next full sync (deletions are still only caught by full syncs). The warehouse can be in any workspace in the same cloud region as the workspaces to update incrementally; Databricks only exposes workspace-level audit events for that region. The connector discovers the warehouse's workspace automatically.", "stringField": {} } ], diff --git a/docs/connector.mdx b/docs/connector.mdx index db250bf5..ef9be667 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -11,6 +11,7 @@ sidebarTitle: "Databricks" | Resource | Sync | Provision | | :--- | :--- | :--- | | Accounts | | | +| Catalogs | | | | Groups | | | | Metastores | | | | Roles | | | @@ -28,25 +29,29 @@ Revoking the workspace **member** entitlement removes the principal's entire wor [This connector syncs non-human identities](/product/admin/nhi) and displays them on the **Identities overview** dashboard. -## Metastore access in C1 +## Metastore and catalog access in C1 -The connector syncs the Databricks [Unity Catalog](https://docs.databricks.com/aws/en/data-governance/unity-catalog/manage-privileges/privileges) -metastore as a C1 resource. Each privilege Databricks records on it -(`CREATE_CATALOG`, `READ_METADATA`, `USE_SHARE` and so on) becomes an -entitlement in C1, so you can review who holds it, request it, and revoke it -for users, groups and service principals. A grant held by a group is expandable -so C1 can resolve the group's members as holders. Ownership is shown as a -read-only `owner` entitlement — it cannot be granted or revoked through C1. +The connector syncs Databricks [Unity Catalog](https://docs.databricks.com/aws/en/data-governance/unity-catalog/manage-privileges/privileges) +metastores and the catalogs in them as C1 resources. Each privilege Databricks +records on one of them (`CREATE_CATALOG` on a metastore, `USE_CATALOG` or +`SELECT` on a catalog, and so on) becomes an entitlement in C1, so you can +review who holds it, request it, and revoke it for users, groups and service +principals. A grant held by a group is expandable so C1 can resolve the +group's members as holders. Ownership is shown as a read-only `owner` +entitlement — it cannot be granted or revoked through C1. -The Metastores resource type is turned off by default. Turn it on from the connector's configuration page in C1. +The Metastores and Catalogs resource types are turned off by default. Turn on the ones you want from the connector's configuration page in C1. Catalogs appear under their metastore and use the same OAuth credentials; no additional configuration fields are needed. **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. -- **MANAGE** and **ALL_PRIVILEGES** are not offered as entitlements, because Databricks rejects both at the metastore level. +- **MANAGE** and **ALL_PRIVILEGES** are not offered on a metastore, because Databricks rejects both at that level. +- A catalog is synced when any running workspace in scope attached to its metastore lists it, and the same catalog name is kept once per metastore. A catalog that only excluded workspaces can reach is not synced. +- The built-in `system` catalog and catalogs Databricks identifies as `SYSTEM_CATALOG` are excluded regardless of ownership. This includes `samples` when Databricks reports that type. +- If a catalog becomes unreachable, access requests can fail until the connector can distinguish deletion from a binding or visibility change. Catalog ownership or **MANAGE** alone cannot prove deletion; that check requires the **Metastore Admin**. - Unity Catalog tags are not synced. @@ -58,8 +63,6 @@ The connector authenticates with **OAuth** — an account-level service principa By default, the Databricks connector picks up access changes on its regular sync schedule. You can optionally turn on incremental sync, which checks Databricks' activity log between full syncs so that changes like new group members show up in C1 sooner. Full syncs still run as usual and remain the source of truth. Removed group memberships are picked up incrementally too, but deleted users, groups, and service principals are only reflected after the next full sync. -Role assignments that Databricks stores in access-control rule sets are also only reflected after the next full sync. These are group roles such as group manager, service principal roles, and the account `marketplace.admin` role. - Incremental sync is turned on by providing the ID of a Databricks SQL warehouse (the **SQL Warehouse ID** setting, `BATON_SQL_WAREHOUSE_ID`) and turned off by leaving it empty — there is no separate on/off switch. Ask your connector operator or C1 support contact to enable it for you. #### Incremental sync requirements @@ -75,7 +78,7 @@ Your Databricks account must meet all of the following: - **Can use** on the SQL warehouse -**We recommend a serverless SQL warehouse.** It starts in seconds, while a pro or classic warehouse can take several minutes to start. The connector waits up to about 6 minutes for the warehouse to start and each query to finish, so a slower start can delay updates until the next one. Serverless SQL warehouses require the Premium plan or above. +**We recommend a serverless SQL warehouse.** It starts in seconds, while a pro or classic warehouse can take several minutes to start. Each incremental update waits at most 5 minutes for the warehouse and query, so a slower start can delay updates until the next one. Serverless SQL warehouses require the Premium plan or above. @@ -108,7 +111,7 @@ A Databricks user with the **Account admin** role, and the **Metastore admin** r -The SQL warehouse can live in any workspace in that region that is not listed in **Exclude Workspaces**, including a workspace outside **Workspaces** — the connector finds it automatically. **Workspaces** and **Exclude Workspaces** also limit which workspaces incremental sync updates, as they do for full syncs. Changes reach C1 roughly four hours after they happen: Databricks delivers audit log entries with a delay, so the connector waits before reading them. +The SQL warehouse can live in any workspace in that region — the connector finds it automatically. Changes reach C1 roughly four hours after they happen: Databricks delivers audit log entries with a delay, so the connector waits before reading them. The incremental sync setup isn't checked when the connector starts, so a problem with it never blocks full syncs or provisioning. If something is missing, such as a permission or a warehouse that can't be found, every incremental update fails and the connector logs an error that names the cause until you fix it. @@ -175,8 +178,28 @@ There is no scope to pick in the console. The connector requests the `all-apis` +### Let the service principal read catalog privileges + +Skip this section if you are not syncing catalogs, or if the service principal is already the **Metastore Admin**. Databricks shows a catalog's full privilege list only to the metastore admin, the catalog owner, or a holder of **MANAGE** on it. Workspace **Admin** is enough only when the catalog is owned by that workspace's admins group (`_workspace_admins__`). + +Otherwise, grant **MANAGE** and **USE CATALOG** on each catalog you want synced. **MANAGE** alone is not enough: Databricks does not return a catalog at all unless the caller is the metastore admin, owns it, or holds **USE CATALOG** on it, and a catalog it does not return is one ConductorOne removes. + + + + In the workspace, open **Catalog Explorer** and open the catalog. + + + Select **Permissions**, then click **Grant**. + + + Select the service principal, check **MANAGE** and **USE CATALOG**, and confirm. + + + -Without this, the metastore sync fails with a permission error naming the metastore. Databricks returns a partial privilege list rather than an error, and C1 would treat the missing grants as revoked. +For a discovered catalog that is not excluded by the system-catalog rule above, missing these permissions fails the sync naming that catalog. Databricks would otherwise return only the privileges the caller can see, and reporting that short list would make ConductorOne delete the grants it cannot read. Failing keeps what ConductorOne already has. + +The connector asks every running workspace in scope attached to the metastore, so a catalog bound to one of those workspaces is included. A catalog the service principal cannot see on any of them is not, and ConductorOne removes it and its grants. **Done.** Here's the set of credentials you'll need when setting up the connector: @@ -234,8 +257,8 @@ To complete this task, you'll need: Optionally, narrow what the connector syncs: - - **Workspaces**: limit the sync to these workspaces, by workspace name, deployment name, or numeric workspace ID. This also scopes which metastores sync: only a metastore attached to one of these workspaces is synced, so narrowing the list removes the other metastores and their grants from C1. - - **Exclude Workspaces**: workspaces to leave out of the sync, by workspace name, deployment name, or numeric workspace ID. A metastore attached only to excluded workspaces is not synced either, and C1 removes it and its grants. + - **Workspaces**: limit the sync to these workspaces, by workspace name, deployment name, or numeric workspace ID. This also scopes which metastores and catalogs sync: only those a listed workspace reaches are synced, so narrowing the list removes the others and their grants from C1. + - **Exclude Workspaces**: workspaces to leave out of the sync, by workspace name, deployment name, or numeric workspace ID. A metastore or catalog that only excluded workspaces reach is not synced either, and C1 removes it and its grants. Fill in **Workspaces** or **Exclude Workspaces**, not both. @@ -319,7 +342,7 @@ stringData: BATON_CLIENT_ID: BATON_CLIENT_SECRET: - # Databricks account ID (required) + # Databricks account ID (required for both credential options below) BATON_ACCOUNT_ID: # Databricks credentials: OAuth @@ -335,7 +358,7 @@ stringData: # Optional: exclude specific workspaces from the sync # (workspace name, deployment name, or numeric ID). # Mutually exclusive with BATON_WORKSPACES — set one or the other, never both. - # BATON_DATABRICKS_EXCLUDE_WORKSPACES: + BATON_DATABRICKS_EXCLUDE_WORKSPACES: # Optional: setting this enables incremental sync; leave it unset to disable it. # ID of the SQL warehouse used to query the system.access.audit log between full syncs. @@ -406,4 +429,4 @@ spec: Stable binaries and container images for the Databricks connector are available from the [official download center](https://dist.conductorone.com/ConductorOne/baton-databricks). - \ No newline at end of file + diff --git a/docs/docs-info.md b/docs/docs-info.md index de5f520d..0d71529b 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -6,7 +6,7 @@ While developing the connector, please fill out this form. This information is n > The connector syncs the Databricks account, workspaces, users, groups, service principals, and roles. > - > It also syncs Unity Catalog metastores. That resource type is opt-in — it stays off until it is enabled per resource type in ConductorOne. + > It also syncs Unity Catalog metastores and their catalogs. Both resource types are opt-in — each stays off until enabled per resource type in ConductorOne. 2. Can the connector provision any resources? If so, which ones? @@ -14,7 +14,7 @@ While developing the connector, please fill out this form. This information is n > > - **User accounts**: create and delete account users. > - **Entitlements**: grant and revoke role and membership assignments on accounts, workspaces, groups, service principals, and roles. - > - **Unity Catalog privileges**: grant and revoke a metastore privilege for a user, group or service principal. The `owner` entitlement is read-only and cannot be granted or revoked. + > - **Unity Catalog privileges**: grant and revoke a metastore or catalog privilege for a user, group or service principal. The `owner` entitlement is read-only and cannot be granted or revoked. ## Connector credentials @@ -23,6 +23,7 @@ While developing the connector, please fill out this form. This information is n > The connector requires a Databricks account ID and OAuth credentials: > > - **OAuth**: a service principal OAuth client ID and client secret. Syncs the account and every workspace the service principal can access. This is the connector's only authentication method. + > - **Catalog sync** uses these same credentials and adds no configuration fields. > > Google Cloud Platform and Azure Databricks customers also provide their Databricks domain, such as `gcp.databricks.com` or `azuredatabricks.net`. Account Hostname is optional: the connector derives it from Hostname when unset. @@ -41,10 +42,12 @@ While developing the connector, please fill out this form. This information is n > The OAuth token carries scopes on top of that. The connector requests `all-apis`, which covers the four it uses: `unity-catalog` (the securables and the permissions endpoint), `scim` (account principals), `access-management` (roles and workspace permission assignments) and `provisioning` (the account workspaces listing). > > Unity Catalog adds a third layer on top of scopes and roles: standing on the securable itself. For each metastore being synced, the service principal must either own the metastore or be assigned Admin directly on at least one running workspace attached to it. Without that standing the sync fails, and ConductorOne keeps the grants it already has. + > + > Catalog grants require metastore ownership, catalog ownership, effective `MANAGE` on the catalog, or direct Admin on the workspace whose `_workspace_admins_…` group owns the catalog. `ALL_PRIVILEGES` does not include `MANAGE`. A non-owner using `MANAGE` also needs `USE_CATALOG` for discovery. * If applicable: Is the list of scopes or permissions different to sync (read) versus provision (read-write)? If so, list the difference here. - > No separate scopes: Databricks admin access covers both read (sync) and read-write (provision). + > No separate OAuth scopes. Catalog writes require metastore or catalog ownership, or `MANAGE` on the catalog; proving a catalog absent requires metastore ownership. * What level of access or permissions does the user need in order to create the credentials? (For example, must be a super administrator, must have access to the admin console, etc.) @@ -53,9 +56,13 @@ While developing the connector, please fill out this form. This information is n ## Limitations - **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. +- **Full grants visibility is required.** Metastore ownership or direct workspace Admin permits metastore reads. Catalog reads require one of the catalog permissions listed above; workspace Admin applies only when that workspace's admins group owns the catalog. Admin inherited through a group does not count. The sync fails instead of reporting a partial 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. - **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. -- **Not synced yet:** catalogs, schemas, tables, volumes, functions, registered models, external locations, storage credentials, connections, shares, recipients, and tags. +- **Catalogs are opt-in.** The resource type is off until it is turned on from the connector's configuration page in C1. +- **Catalog discovery visits every attached workspace in scope.** The connector asks each running workspace attached to the metastore, including for `ISOLATED` catalogs bound to only some workspaces. Each catalog name is kept once per metastore. A catalog bound only to workspaces the sync excludes is not listed, and ConductorOne removes it with its grants. +- **System catalogs are excluded regardless of ownership.** The built-in `system` catalog and any catalog with `CatalogType == "SYSTEM_CATALOG"` are excluded, including `samples` when it has that type. +- **Seeing a catalog needs more than `MANAGE`.** The list holds the catalogs the service principal owns or holds `USE_CATALOG` on, or every catalog when it is the metastore admin. `MANAGE` alone reads a catalog's privileges but does not make it appear. +- **Not synced yet:** schemas, tables, volumes, functions, registered models, external locations, storage credentials, connections, shares, recipients, and tags. diff --git a/pkg/config/config.go b/pkg/config/config.go index 900f0e8d..66373b64 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -62,8 +62,8 @@ var ( field.WithDescription( "Setting this enables incremental sync; leaving it empty disables it. "+ "ID of the Databricks SQL warehouse used to query the system.access.audit log between full syncs, "+ - "so access changes show up before the next full sync (deleted users, groups and service principals are still only caught by full syncs). "+ - "The warehouse can be in any workspace not excluded by databricks-exclude-workspaces, in the same cloud region as the workspaces to update incrementally; "+ + "so access changes show up before the next full sync (deletions are still only caught by full syncs). "+ + "The warehouse can be in any workspace in the same cloud region as the workspaces to update incrementally; "+ "Databricks only exposes workspace-level audit events for that region. The connector discovers the warehouse's workspace automatically.", ), field.WithDisplayName("SQL Warehouse ID"), diff --git a/pkg/connector/catalog.go b/pkg/connector/catalog.go new file mode 100644 index 00000000..f4597738 --- /dev/null +++ b/pkg/connector/catalog.go @@ -0,0 +1,759 @@ +package connector + +import ( + "context" + "errors" + "fmt" + "net/http" + "slices" + "strconv" + "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" + "github.com/conductorone/baton-sdk/pkg/connectorbuilder" + "github.com/conductorone/baton-sdk/pkg/pagination" + "github.com/conductorone/baton-sdk/pkg/session" + ent "github.com/conductorone/baton-sdk/pkg/types/entitlement" + rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/conductorone/baton-sdk/pkg/types/sessions" + "github.com/conductorone/baton-sdk/pkg/uhttp" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "google.golang.org/protobuf/proto" +) + +var _ connectorbuilder.StaticEntitlementSyncerV2 = (*catalogBuilder)(nil) + +const ( + securableTypeCatalog = "CATALOG" + + // privilegeManage is the only privilege that lets its holder read every + // privilege assignment on a catalog without owning it. + privilegeManage = "MANAGE" + + // errorCodeCatalogGone is what Databricks names in error_code when the + // catalog a request addresses does not exist. + errorCodeCatalogGone = "CATALOG_DOES_NOT_EXIST" + errorCodeMetastoreGone = "METASTORE_DOES_NOT_EXIST" + + // systemCatalog is the built-in catalog Databricks puts in every metastore. + // It is excluded independently of the connector's ownership. + systemCatalog = "system" + + // workspaceAdminsGroupPrefix starts the name of the system group Databricks + // creates for a workspace, _workspace_admins__. + workspaceAdminsGroupPrefix = "_workspace_admins_" + + profileKeyFullName = "full_name" + profileKeyCatalogName = "catalog_name" + profileKeyCatalogType = "catalog_type" + profileKeyIsolationMode = "isolation_mode" + + // profileKeyMetastoreOwner carries the owner of the metastore the catalog + // belongs to. Owning the metastore is what makes a privilege read complete, + // and the catalog syncer is only handed the metastore's id, so List writes + // the owner down instead of resolving it again on every Grants call. + profileKeyMetastoreOwner = "metastore_owner" + + // catalogWriteRemedy is what a 403 on the PATCH tells the operator. + catalogWriteRemedy = "the connector must own the metastore or the catalog, or hold MANAGE on the catalog" + + // catalogTrustRemedy is what a refusal to report a partial privilege list + // tells the operator. + catalogTrustRemedy = "the connector's service principal must own the metastore or the catalog, hold MANAGE on the catalog, " + + "or be assigned Admin directly on the workspace whose admins group owns it" +) + +// catalogPrivileges is the widest privilege set in Unity Catalog: BROWSE, +// CREATE_SCHEMA and USE_CATALOG stop here. Canonical UPPER_SNAKE names, the form +// the permissions API answers with. The PATCH accepts and echoes every entry, +// including ones missing from the databricks-sdk-go Privilege enum. +// https://docs.databricks.com/aws/en/data-governance/unity-catalog/manage-privileges/privileges +var catalogPrivileges = []string{ + "ALL_PRIVILEGES", + "APPLY_TAG", + "BROWSE", + "CREATE_SCHEMA", + "USE_CATALOG", + "USE_SCHEMA", + "CREATE_FUNCTION", + "CREATE_TABLE", + "CREATE_MATERIALIZED_VIEW", + "CREATE_MODEL", + "CREATE_VOLUME", + "CREATE_FEATURE", + "CREATE_SECRET", + "CREATE_SERVICE", + "SELECT", + "MODIFY", + "DELETE", + "INSERT", + "UPDATE", + "EXECUTE", + "REFRESH", + "READ_VOLUME", + "WRITE_VOLUME", + "READ_METADATA", + "READ_FEATURE", + "READ_SECRET", + "REFERENCE_SECRET", + "WRITE_SECRET", + "EXTERNAL_USE_SCHEMA", + "MANAGE", +} + +var catalogLevel = securableLevel{ + securableType: databricks.SecurableCatalog, + noun: "catalog", + privileges: catalogPrivileges, + writeRemedy: catalogWriteRemedy, + trustRemedy: catalogTrustRemedy, + permissionsName: catalogPermissionsName, + trusts: catalogGrantsAreTrustworthy, + goneErrorCodes: []string{errorCodeCatalogGone}, +} + +var errCatalogGone = errors.New("catalog no longer exists") + +type catalogBuilder struct { + securableOps + // workspaces is the configured allowlist, empty when none was given. + workspaces map[string]struct{} +} + +func newCatalogBuilder(client *databricks.Client, clientID string, workspaces map[string]struct{}, sync syncPrincipals) *catalogBuilder { + return &catalogBuilder{securableOps: securableOps{client: client, clientID: clientID, sync: sync, level: catalogLevel}, workspaces: workspaces} +} + +func (b *catalogBuilder) ResourceType(_ context.Context) *v2.ResourceType { + return catalogResourceType +} + +// catalogResourceID keys a catalog by its metastore. Catalog names repeat across +// metastores, so the name alone would merge two catalogs into one resource. +func catalogResourceID(metastoreID, name string) string { + return metastoreID + securableKeySeparator + name +} + +// parseCatalogID reads a catalog resource id, "{metastore_id}::{catalog_name}". +func parseCatalogID(resourceID string) (string, string, error) { + metastoreID, name, ok := strings.Cut(resourceID, securableKeySeparator) + if !ok || metastoreID == "" || name == "" { + return "", "", status.Errorf(codes.InvalidArgument, "databricks-connector: invalid catalog resource id %q", resourceID) + } + + return metastoreID, name, nil +} + +// catalogPermissionsName: the permissions endpoint addresses a catalog by its +// bare name, which is also its full name. The metastore is carried by the host. +func catalogPermissionsName(resourceID string) (string, error) { + _, name, err := parseCatalogID(resourceID) + + return name, err +} + +func catalogResource( + catalog *databricks.Catalog, + metastoreID string, + parent *v2.ResourceId, + route catalogRoute, +) (*v2.Resource, error) { + profile := map[string]any{ + profileKeyMetastoreID: metastoreID, + profileKeyMetastoreOwner: route.MetastoreOwner, + profileKeyFullName: catalog.Name, + profileKeyCatalogName: catalog.Name, + profileKeyCatalogType: catalog.CatalogType, + profileKeyIsolationMode: catalog.IsolationMode, + profileKeyOwner: catalog.Owner, + profileKeySecurableType: securableTypeCatalog, + profileKeyWorkspace: route.Workspace, + profileKeyWorkspaceID: route.WorkspaceID, + profileKeyWorkspaceName: route.WorkspaceName, + } + + resourceID := catalogResourceID(metastoreID, catalog.Name) + + return rs.NewAppResource( + catalog.Name, + catalogResourceType, + resourceID, + nil, + rs.WithResourceProfile(profile), + rs.WithParentResourceID(parent), + rs.WithAnnotation(&v2.RawId{Id: resourceID}), + ) +} + +// catalogRoute identifies one running in-scope workspace serving a metastore. +type catalogRoute struct { + Workspace string `json:"w"` + WorkspaceID string `json:"i"` + WorkspaceName string `json:"n"` + // MetastoreOwner rides along so each catalog's profile can carry it. See + // profileKeyMetastoreOwner. + MetastoreOwner string `json:"o"` +} + +// List reads a metastore's catalogs through every running in-scope workspace. +// A catalog bound to only one of them is absent from the others. +func (b *catalogBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, attr rs.SyncOpAttrs) ([]*v2.Resource, *rs.SyncOpResults, error) { + if parentResourceID.GetResourceType() != metastoreResourceType.Id { + return nil, nil, nil + } + + metastoreID := parentResourceID.GetResource() + annos := annotations.Annotations{} + bag := &pagination.Bag{} + if err := bag.Unmarshal(attr.PageToken.Token); err != nil { + return nil, nil, status.Errorf(codes.InvalidArgument, "databricks-connector: invalid catalog page token: %v", err) + } + + ctx = context.WithValue(ctx, syncSessionKey{}, attr.Session) + routes, err := b.catalogRoutes(ctx, metastoreID, attr.Session, &annos) + if err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, err + } + if bag.Current() == nil { + // The bag is a stack, so the routes go in backward to come out in order. + // The last one walked is the copy that survives the upsert, which is what + // puts its workspace on the profile Grants and provisioning read back. + for _, route := range slices.Backward(routes) { + bag.Push(pagination.PageState{ResourceTypeID: catalogResourceType.Id, ResourceID: route.Workspace}) + } + if bag.Current() == nil { + return nil, &rs.SyncOpResults{Annotations: annos}, nil + } + } + routeIndex := slices.IndexFunc(routes, func(route catalogRoute) bool { return route.Workspace == bag.ResourceID() }) + if routeIndex < 0 { + return nil, nil, status.Errorf(codes.InvalidArgument, "databricks-connector: catalog page token names an unknown workspace %s", bag.ResourceID()) + } + route := routes[routeIndex] + + catalogs, next, rateLimit, err := b.client.ListCatalogs(ctx, route.Workspace, bag.PageToken(), ResourcesPageSize, false) + noteRateLimit(&annos, rateLimit) + if err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, catalogListingError(metastoreID, route.Workspace, err) + } + + var rv []*v2.Resource + for _, catalog := range catalogs { + if catalog.Name == "" || (catalog.MetastoreID != "" && catalog.MetastoreID != metastoreID) { + continue + } + + if catalog.Name == systemCatalog || catalog.CatalogType == "SYSTEM_CATALOG" { + continue + } + + // A partial privilege list is indistinguishable from an empty one, and + // leaving the catalog out is not the safe half: ConductorOne deletes every + // resource a sync stops emitting, so the catalog, its grants, and its + // schemas, tables and volumes all go. The sync stops instead and keeps them. + trustworthy, err := catalogGrantsAreTrustworthy(ctx, b.client, securableTrust{ + ownPrincipal: b.clientID, + owner: catalog.Owner, + metastoreOwner: route.MetastoreOwner, + fullName: catalog.Name, + workspace: route.Workspace, + workspaceID: route.WorkspaceID, + }, &annos) + if err != nil { + if errors.Is(err, errCatalogGone) { + continue + } + return nil, &rs.SyncOpResults{Annotations: annos}, err + } + if !trustworthy { + return nil, &rs.SyncOpResults{Annotations: annos}, status.Errorf(codes.PermissionDenied, + "databricks-connector: cannot read every privilege on catalog %s through workspace %s; %s", + catalog.Name, route.Workspace, catalogTrustRemedy) + } + + built, err := catalogResource(&catalog, metastoreID, parentResourceID, route) + if err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, fmt.Errorf( + "databricks-connector: failed to build the resource for catalog %s: %w", catalog.Name, err) + } + rv = append(rv, built) + } + + if next != "" && next == bag.PageToken() { + return nil, &rs.SyncOpResults{Annotations: annos}, status.Errorf(codes.Internal, + "databricks-connector: listing catalogs of metastore %s repeated page token %q", metastoreID, next) + } + + nextToken, err := bag.NextToken(next) + if err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, err + } + + return rv, &rs.SyncOpResults{Annotations: annos, NextPageToken: nextToken}, nil +} + +// catalogRoutes reuses the metastore's routes from this sync. A targeted sync +// without a metastore List resolves them live and remembers the result. +func (b *catalogBuilder) catalogRoutes(ctx context.Context, metastoreID string, store sessions.SessionStore, annos *annotations.Annotations) ([]catalogRoute, error) { + key := catalogRoutesKey(metastoreID) + routes, found, err := session.GetJSON[[]catalogRoute](ctx, store, key) + if err != nil { + return nil, fmt.Errorf("databricks-connector: failed to read catalog routes for metastore %s: %w", metastoreID, err) + } + if found { + return routes, nil + } + routes, err = b.resolveCatalogRoutes(ctx, metastoreID, annos) + if err != nil { + return nil, err + } + if err := session.SetJSON(ctx, store, key, routes); err != nil { + return nil, fmt.Errorf("databricks-connector: failed to store catalog routes for metastore %s: %w", metastoreID, err) + } + return routes, nil +} + +func (b *catalogBuilder) resolveCatalogRoutes(ctx context.Context, metastoreID string, annos *annotations.Annotations) ([]catalogRoute, error) { + scope, err := scopedWorkspaces(ctx, b.client, b.workspaces, annos) + if err != nil { + return nil, err + } + usable, inScope, err := scope.attached(ctx, b.client, metastoreID, annos) + if errors.Is(err, errMetastoreGone) { + return nil, nil + } + if err != nil { + return nil, assignmentListError(metastoreID, err) + } + if len(usable) == 0 { + if inScope { + return nil, status.Errorf(codes.FailedPrecondition, "databricks-connector: metastore %s has no running workspace in sync scope", metastoreID) + } + return nil, nil + } + owner, err := metastoreOwner(ctx, b.client, metastoreID, annos) + if errors.Is(err, errMetastoreGone) { + return nil, nil + } + if err != nil { + return nil, err + } + return catalogRoutesForWorkspaces(usable, owner), nil +} + +// metastoreOwner reads the owner of the metastore the catalogs hang from. It is +// resolved once per metastore, with the route, because owning the metastore is +// what makes a catalog's privilege read complete. A metastore the account no +// longer lists is reported as errMetastoreGone: the account listing is what +// proves the metastore is gone, and provisioning answers for a deleted parent +// instead of failing every task against it. +func metastoreOwner( + ctx context.Context, + client *databricks.Client, + metastoreID string, + annos *annotations.Annotations, +) (string, error) { + metastores, rateLimit, err := client.ListMetastores(ctx) + noteRateLimit(annos, rateLimit) + if err != nil { + return "", refuseUnreadable("databricks-connector: failed to list metastores", err) + } + + for _, metastore := range metastores { + if metastore.MetastoreID == metastoreID { + return metastore.Owner, nil + } + } + + ctxzap.Extract(ctx).Debug("databricks-connector: the account no longer lists the metastore", + zap.String("metastore", metastoreID)) + + return "", errMetastoreGone +} + +// catalogThrough reads one catalog through workspace, nil when that workspace +// does not serve it. A single GET replaces draining the listing, which cost one +// page walk per catalog per workspace, and it separates the two answers the +// listing ran together: a refusal naming the catalog as nonexistent is an +// absence, and anything else is this workspace not seeing a catalog that may +// well exist. +func catalogThrough( + ctx context.Context, + client *databricks.Client, + workspace, metastoreID, catalogName string, + annos *annotations.Annotations, +) (*databricks.Catalog, error) { + catalog, rateLimit, err := client.GetCatalog(ctx, workspace, catalogName) + noteRateLimit(annos, rateLimit) + switch { + case err == nil: + case databricks.IsSecurableGone(err, errorCodeCatalogGone): + return nil, errCatalogGone + case deniesTrust(err): + return nil, err + case status.Code(err) == codes.InvalidArgument: + // The name does not survive URL assembly, or Databricks answered with a + // different catalog. Either way the answer does not describe the catalog + // asked for, and a provisioning write must not be addressed from it. + return nil, err + default: + return nil, catalogListingError(metastoreID, workspace, err) + } + + // A catalog served by this workspace but belonging to another metastore is + // not this metastore's catalog. + if catalog.MetastoreID != "" && catalog.MetastoreID != metastoreID { + return nil, nil + } + + return catalog, nil +} + +// catalogListingError reads a workspace that refuses the catalog listing as +// unknown, not as listing nothing: a short list would read as deletions in C1. +func catalogListingError(metastoreID, workspace string, err error) error { + switch apiStatus(err) { + case http.StatusUnauthorized, http.StatusForbidden, http.StatusNotFound: + return uhttp.WrapErrors(codes.FailedPrecondition, + fmt.Sprintf("databricks-connector: the catalogs of metastore %s cannot be listed through attached workspace %s, "+ + "so which catalogs are in scope is unknown; make the connector principal an Admin of that workspace, "+ + "or leave the workspace out with workspaces or databricks-exclude-workspaces", metastoreID, workspace), + err) + } + + return fmt.Errorf("databricks-connector: failed to list the catalogs of metastore %s through workspace %s: %w", + metastoreID, workspace, err) +} + +func (b *catalogBuilder) StaticEntitlements(_ context.Context, _ rs.SyncOpAttrs) ([]*v2.Entitlement, *rs.SyncOpResults, error) { + grantable := ent.WithGrantableTo(userResourceType, groupResourceType, servicePrincipalResourceType) + rv := make([]*v2.Entitlement, 0, len(catalogPrivileges)+1) + for _, privilege := range catalogPrivileges { + rv = append(rv, ent.NewPermissionEntitlement(nil, privilege, + grantable, + ent.WithDescription(fmt.Sprintf("%s privilege on a catalog in Databricks", privilege)), + )) + } + + rv = append(rv, ent.NewOwnershipEntitlement(nil, ownerEntitlement, + ent.WithDescription("Owns a catalog in Databricks"), + )) + + return rv, nil, nil +} + +func (b *catalogBuilder) Entitlements(context.Context, *v2.Resource, rs.SyncOpAttrs) ([]*v2.Entitlement, *rs.SyncOpResults, error) { + return nil, nil, nil +} + +// catalogGrantsAreTrustworthy is true for the metastore owner, the catalog +// owner, a holder of MANAGE, or a direct Admin when the workspace admins group +// owns the catalog. +func catalogGrantsAreTrustworthy( + ctx context.Context, + client *databricks.Client, + trust securableTrust, + annos *annotations.Annotations, +) (bool, error) { + // An owner holds every privilege implicitly, which is not reported as a grant. + // The metastore's owner is its admin, and reads every privilege below it. + if ownsPrincipal(trust.ownPrincipal, trust.metastoreOwner) || ownsPrincipal(trust.ownPrincipal, trust.owner) { + return true, nil + } + + // The workspace admins group acts as the owner for a direct Admin of its workspace. + if ownerWorkspaceID, ok := catalogAdminWorkspaceID(trust.owner); ok { + admin, err := isWorkspaceAdmin(ctx, client, trust.ownPrincipal, ownerWorkspaceID, annos) + if err != nil || admin { + return admin, err + } + } + + return holdsCatalogManage(ctx, client, trust, annos) +} + +func catalogAdminWorkspaceID(owner string) (string, bool) { + if !strings.HasPrefix(owner, workspaceAdminsGroupPrefix) { + return "", false + } + separator := strings.LastIndex(owner, "_") + if separator <= len(workspaceAdminsGroupPrefix) || separator == len(owner)-1 { + return "", false + } + id := owner[separator+1:] + if _, err := strconv.ParseUint(id, 10, 64); err != nil { + return "", false + } + + return id, true +} + +// holdsCatalogManage asks the permissions endpoint for the connector's own +// effective privileges on the catalog. The principal filter is effective, not +// literal: a row can be the MANAGE a group holds and this principal inherits, so +// only the privileges are read and never the principal field. The read bypasses +// the cache, because a remembered page would decide this call with the access of +// an earlier one. +// +// ALL_PRIVILEGES is not enough. Databricks leaves MANAGE out of it, so its holder +// still reads a partial list. +// https://docs.databricks.com/aws/en/data-governance/unity-catalog/manage-privileges/privileges +func holdsCatalogManage( + ctx context.Context, + client *databricks.Client, + trust securableTrust, + annos *annotations.Annotations, +) (bool, error) { + if trust.ownPrincipal == "" || trust.workspace == "" { + return false, nil + } + + var manages bool + rateLimit, err := client.ForEachUncachedPermissionsPage(ctx, trust.workspace, databricks.SecurableCatalog, trust.fullName, trust.ownPrincipal, + func(page []databricks.PrivilegeAssignment) (bool, error) { + for _, assignment := range page { + if slices.Contains(assignment.Privileges, privilegeManage) { + manages = true + + return true, nil + } + } + + return false, nil + }) + noteRateLimit(annos, rateLimit) + if err != nil { + if databricks.IsSecurableGone(err, errorCodeCatalogGone) { + return false, errCatalogGone + } + if apiStatus(err) == http.StatusNotFound { + return false, refuseUnreadable( + fmt.Sprintf("databricks-connector: failed to read the connector's own privileges on catalog %s", trust.fullName), err) + } + if !deniesTrust(err) { + return false, fmt.Errorf("databricks-connector: failed to read the connector's own privileges on catalog %s: %w", trust.fullName, err) + } + ctxzap.Extract(ctx).Debug("databricks-connector: the connector's own privileges on the catalog cannot be read, so MANAGE stays unproven", + zap.String("catalog", trust.fullName), + zap.Error(err)) + + return false, nil + } + + return manages, nil +} + +func (b *catalogBuilder) Grants(ctx context.Context, resource *v2.Resource, attr rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { + return b.syncGrants(ctx, resource, attr) +} + +func (b *catalogBuilder) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { + ctx = databricks.WithoutCache(ctx) + annos := annotations.Annotations{} + routed, err := b.entitlementOnLiveRoute(ctx, entitlement, &annos) + if err != nil { + if errors.Is(err, errMetastoreGone) || errors.Is(err, errCatalogGone) { + return annos, status.Errorf(codes.NotFound, "databricks-connector: catalog %s is no longer on metastore %s", + entitlement.GetResource().GetId().GetResource(), entitlement.GetResource().GetParentResourceId().GetResource()) + } + return annos, err + } + + return b.grant(ctx, principal.GetId(), routed) +} + +func (b *catalogBuilder) Revoke(ctx context.Context, revoked *v2.Grant) (annotations.Annotations, error) { + ctx = databricks.WithoutCache(ctx) + annos := annotations.Annotations{} + routed, err := b.entitlementOnLiveRoute(ctx, revoked.GetEntitlement(), &annos) + if err != nil { + if errors.Is(err, errMetastoreGone) || errors.Is(err, errCatalogGone) { + annos.Update(&v2.GrantAlreadyRevoked{}) + return annos, nil + } + return annos, err + } + + return b.revoke(ctx, revoked.GetPrincipal().GetId(), routed) +} + +// entitlementOnLiveRoute returns the entitlement addressed at a workspace. The +// workspace an earlier sync wrote on the profile is tried first, with one catalog +// read and no account listing: that read answers what the two listings were asked +// for, since a workspace that returns this metastore's catalog is by construction +// running and still serving it. The listings run only when the profile names no +// workspace, this sync no longer admits it, or it no longer serves the catalog. +func (b *catalogBuilder) entitlementOnLiveRoute( + ctx context.Context, + entitlement *v2.Entitlement, + annos *annotations.Annotations, +) (*v2.Entitlement, error) { + ctx = databricks.WithoutCache(ctx) + resource := entitlement.GetResource() + metastoreID, catalogName, err := parseCatalogID(resource.GetId().GetResource()) + if err != nil { + return nil, err + } + // The owner decides trust and whether a deletion can be proved, so it is read + // live on both paths. The profile is trusted for the route, never for standing. + owner, err := metastoreOwner(ctx, b.client, metastoreID, annos) + if err != nil { + return nil, err + } + + var probe catalogProbe + if route, ok := b.profileRoute(resource, owner); ok { + catalog, err := catalogThrough(ctx, b.client, route.Workspace, metastoreID, catalogName, annos) + // This read is an optimization, so no answer it gives is fatal: a catalog + // absent from this one workspace is not absent from the metastore, a refusal + // here is not a refusal everywhere, and a workspace that has stopped serving + // is what the walk exists to route around. Anything short of the catalog + // falls through, and only the walk's own errors reach the caller. + switch { + case err == nil: + case errors.Is(err, errCatalogGone): + probe = catalogProbe{workspace: route.Workspace, gone: true} + case deniesTrust(err): + probe = catalogProbe{workspace: route.Workspace, deny: err} + } + switch { + // Skipping the attachment listing means the catalog has to name this + // metastore itself: a workspace that serves it is attached to the metastore + // that owns it. An answer that omits the id proves nothing, so it falls + // through to the walk and is read again there, where the listings ran. + case catalog != nil && catalog.MetastoreID == metastoreID: + return routedEntitlement(entitlement, catalog, metastoreID, route) + case catalog != nil: + case err == nil: + probe = catalogProbe{workspace: route.Workspace} + } + } + + scope, err := scopedWorkspaces(ctx, b.client, b.workspaces, annos) + if err != nil { + return nil, err + } + usable, _, err := scope.attached(ctx, b.client, metastoreID, annos) + if err != nil { + return nil, assignmentListError(metastoreID, err) + } + if len(usable) == 0 { + return nil, status.Errorf(codes.FailedPrecondition, + "databricks-connector: metastore %s has no running workspace in sync scope to prove whether catalog %s was deleted", + metastoreID, catalogName) + } + + workspace, catalog, err := workspaceForCatalog(ctx, b.client, metastoreID, catalogName, usable, probe, annos) + if err != nil { + if errors.Is(err, errCatalogGone) { + return nil, b.confirmCatalogGone(ctx, metastoreID, catalogName, usable[0].DeploymentName, owner, annos) + } + return nil, err + } + + return routedEntitlement(entitlement, catalog, metastoreID, catalogRoute{ + Workspace: workspace.DeploymentName, + WorkspaceID: strconv.Itoa(workspace.ID), + WorkspaceName: workspace.Name, + MetastoreOwner: owner, + }) +} + +// catalogProbe is the outcome of the one read entitlementOnLiveRoute makes +// through the profile workspace. The walk carries it so that workspace is not +// read a second time and its answer still counts toward "gone everywhere". +// A zero value means nothing was read and the walk reads every workspace. +type catalogProbe struct { + workspace string + gone bool + deny error +} + +// profileRoute is the route an earlier sync wrote, when this sync still admits +// that workspace. The profile carries the three identifiers the configuration +// matches on, so the allowlist and the exclude set are both answered here without +// reading the account's workspaces or the metastore's assignments. +func (b *catalogBuilder) profileRoute(resource *v2.Resource, owner string) (catalogRoute, bool) { + profile := rs.GetProfile(resource) + route := catalogRoute{MetastoreOwner: owner} + route.Workspace, _ = rs.GetProfileStringValue(profile, profileKeyWorkspace) + route.WorkspaceID, _ = rs.GetProfileStringValue(profile, profileKeyWorkspaceID) + route.WorkspaceName, _ = rs.GetProfileStringValue(profile, profileKeyWorkspaceName) + if route.Workspace == "" { + return catalogRoute{}, false + } + if b.client.IsWorkspaceExcluded(route.WorkspaceName, route.Workspace, route.WorkspaceID) { + return catalogRoute{}, false + } + if len(b.workspaces) > 0 { + if _, ok := matchConfiguredWorkspace(b.workspaces, route.Workspace, route.WorkspaceName, route.WorkspaceID); !ok { + return catalogRoute{}, false + } + } + + return route, true +} + +// routedEntitlement copies the entitlement onto the catalog as read through route. +func routedEntitlement( + entitlement *v2.Entitlement, + catalog *databricks.Catalog, + metastoreID string, + route catalogRoute, +) (*v2.Entitlement, error) { + routed, err := catalogResource(catalog, metastoreID, entitlement.GetResource().GetParentResourceId(), route) + if err != nil { + return nil, err + } + + copyEntitlement := proto.Clone(entitlement).(*v2.Entitlement) + copyEntitlement.Resource = routed + + return copyEntitlement, nil +} + +func (b *catalogBuilder) confirmCatalogGone( + ctx context.Context, + metastoreID, catalogName, workspace, metastoreOwner string, + annos *annotations.Annotations, +) error { + ctx = databricks.WithoutCache(ctx) + if !ownsPrincipal(b.clientID, metastoreOwner) || workspace == "" { + return status.Errorf(codes.FailedPrecondition, + "databricks-connector: cannot prove whether catalog %s of metastore %s was deleted; the metastore owner must list it with include_unbound=true", + catalogName, metastoreID) + } + + pageToken := "" + for { + catalogs, next, rateLimit, err := b.client.ListCatalogs(ctx, workspace, pageToken, ResourcesPageSize, true) + noteRateLimit(annos, rateLimit) + if err != nil { + msg := fmt.Sprintf("databricks-connector: cannot prove whether catalog %s of metastore %s was deleted with include_unbound=true", + catalogName, metastoreID) + if deniesTrust(err) { + return uhttp.WrapErrors(codes.FailedPrecondition, msg, err) + } + return fmt.Errorf("%s: %w", msg, err) + } + for _, catalog := range catalogs { + if catalog.Name == catalogName && (catalog.MetastoreID == "" || catalog.MetastoreID == metastoreID) { + return status.Errorf(codes.FailedPrecondition, + "databricks-connector: catalog %s still exists but no workspace in sync scope can list it", + catalogName) + } + } + if next == "" { + return errCatalogGone + } + if next == pageToken { + return status.Errorf(codes.Internal, "databricks-connector: listing unbound catalogs repeated page token %q", next) + } + pageToken = next + } +} diff --git a/pkg/connector/catalog_session.go b/pkg/connector/catalog_session.go new file mode 100644 index 00000000..0da86e9c --- /dev/null +++ b/pkg/connector/catalog_session.go @@ -0,0 +1,26 @@ +package connector + +import ( + "strconv" + + "github.com/conductorone/baton-databricks/pkg/databricks" +) + +type syncSessionKey struct{} + +func catalogRoutesKey(metastoreID string) string { + return "unity-catalog/routes/" + metastoreID +} + +func catalogRoutesForWorkspaces(workspaces []databricks.Workspace, owner string) []catalogRoute { + routes := make([]catalogRoute, 0, len(workspaces)) + for _, workspace := range workspaces { + routes = append(routes, catalogRoute{ + Workspace: workspace.DeploymentName, + WorkspaceID: strconv.Itoa(workspace.ID), + WorkspaceName: workspace.Name, + MetastoreOwner: owner, + }) + } + return routes +} diff --git a/pkg/connector/catalog_test.go b/pkg/connector/catalog_test.go new file mode 100644 index 00000000..ee332e36 --- /dev/null +++ b/pkg/connector/catalog_test.go @@ -0,0 +1,903 @@ +package connector + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "slices" + "strconv" + "strings" + "sync" + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/pagination" + "github.com/conductorone/baton-sdk/pkg/session" + "github.com/conductorone/baton-sdk/pkg/types/grant" + rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/conductorone/baton-sdk/pkg/types/sessions" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/conductorone/baton-databricks/pkg/databricks" +) + +// fakeCatalogAccount is one metastore (ms-1) served by the RUNNING workspaces +// wsa (id 1, named prod) and wsb (id 2, named dev). Each field shapes one +// answer; the counters record what the connector asked. +type fakeCatalogAccount struct { + attached []int + // attachedStatus fails the metastore's assignment list with that status. + attachedStatus int + attachedErrorCode string + // listed maps a workspace to the catalogs its listing returns. + listed map[string][]map[string]any + // listStatus fails a workspace's catalog listing with that status. + listStatus map[string]int + // assignments maps a catalog to the privilege assignments on it. + assignments map[string][]map[string]any + // ownPrivileges answers a read filtered by principal, which is how the + // connector asks for its own effective privileges on a catalog. + ownPrivileges []map[string]any + // workspaceMembers maps a workspace id to its permission assignments, or to + // the HTTP status the account API answers the list with. + workspaceMembers map[string]any + permStatus int + // permErrorCode is the vendor error_code that rides along with permStatus. + permErrorCode string + unboundCatalogs []map[string]any + includeUnboundReads int + metastoreOwner string + // metastoreGone drops the metastore from the account listing. + metastoreGone bool + catalogGets []string + patchStatus int + users map[string]string + groups map[string]string + + routed int + catalogPages int + patches []map[string]any + permHosts []string + memberLists []string + filteredReads []string +} + +func (f *fakeCatalogAccount) handler(t *testing.T) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + path := r.URL.Path + encode := func(v any) { _ = json.NewEncoder(w).Encode(v) } + switch { + case strings.Contains(path, "/scim/v2/"): + serveSCIM(w, r, f.users, f.groups, nil, nil) + case strings.HasSuffix(path, "/metastores"): + if f.metastoreGone { + encode(map[string]any{"metastores": []map[string]any{}}) + + return + } + owner := f.metastoreOwner + if owner == "" { + owner = "eng" + } + encode(map[string]any{"metastores": []map[string]any{{"metastore_id": "ms-1", "name": "main", "owner": owner}}}) + case strings.Contains(path, "/metastores/") && strings.HasSuffix(path, "/workspaces"): + if f.attachedStatus != 0 { + w.WriteHeader(f.attachedStatus) + _ = json.NewEncoder(w).Encode(map[string]any{"error_code": f.attachedErrorCode, "message": "denied"}) + + return + } + encode(map[string]any{"workspace_ids": f.attached}) + case strings.HasSuffix(path, "/permissionassignments"): + parts := strings.Split(path, "/") + workspaceID := parts[len(parts)-2] + f.memberLists = append(f.memberLists, workspaceID) + switch v := f.workspaceMembers[workspaceID].(type) { + case int: + http.Error(w, `{"message":"denied"}`, v) + default: + encode(map[string]any{"permission_assignments": v}) + } + case strings.HasSuffix(path, "/workspaces"): + f.routed++ + encode([]map[string]any{ + {"workspace_id": 1, "workspace_name": "prod", "deployment_name": "wsa", "workspace_status": "RUNNING"}, + {"workspace_id": 2, "workspace_name": "dev", "deployment_name": "wsb", "workspace_status": "RUNNING"}, + }) + case strings.HasSuffix(path, "/unity-catalog/catalogs"): + f.serveCatalogs(w, r) + case strings.Contains(path, "/unity-catalog/catalogs/"): + f.serveCatalog(w, r, path[strings.LastIndex(path, "/")+1:]) + case strings.Contains(path, "/unity-catalog/permissions/catalog/"): + f.servePermissions(w, r, path[strings.LastIndex(path, "/")+1:]) + default: + t.Errorf("unexpected request %s %s", r.Method, path) + http.Error(w, `{"message":"unexpected"}`, http.StatusInternalServerError) + } + } +} + +func (f *fakeCatalogAccount) serveCatalogs(w http.ResponseWriter, r *http.Request) { + workspace := r.Header.Get("X-Test-Workspace") + if code, ok := f.listStatus[workspace]; ok { + http.Error(w, `{"message":"denied"}`, code) + return + } + + f.catalogPages++ + all := f.listed[workspace] + if r.URL.Query().Get("include_unbound") == "true" { + f.includeUnboundReads++ + all = f.unboundCatalogs + } + start, _ := strconv.Atoi(r.URL.Query().Get("page_token")) + size, _ := strconv.Atoi(r.URL.Query().Get("max_results")) + if size <= 0 || size > len(all)-start { + size = len(all) - start + } + next := "" + if start+size < len(all) { + next = strconv.Itoa(start + size) + } + _ = json.NewEncoder(w).Encode(map[string]any{"catalogs": all[start : start+size], "next_page_token": next}) +} + +// serveCatalog answers GET /catalogs/{name} from the same per-workspace listing, +// so a catalog bound elsewhere 404s here exactly as the workspace would answer. +// servesMetastore reports whether the workspace is still attached to ms-1. A +// workspace Databricks has detached serves none of that metastore's catalogs, so +// it answers the same 404 as a catalog that does not exist. Without this the fake +// lets a detached workspace return a catalog stamped with a metastore it is not +// attached to, which the real API cannot produce. +func (f *fakeCatalogAccount) servesMetastore(workspace string) bool { + if len(f.attached) == 0 { + return true + } + id, known := map[string]int{"wsa": 1, "wsb": 2}[workspace] + + return !known || slices.Contains(f.attached, id) +} + +func (f *fakeCatalogAccount) serveCatalog(w http.ResponseWriter, r *http.Request, name string) { + workspace := r.Header.Get("X-Test-Workspace") + f.catalogGets = append(f.catalogGets, workspace) + if code, ok := f.listStatus[workspace]; ok { + http.Error(w, `{"message":"denied"}`, code) + + return + } + if !f.servesMetastore(workspace) { + http.Error(w, `{"error_code":"CATALOG_DOES_NOT_EXIST","message":"not found"}`, http.StatusNotFound) + + return + } + for _, catalog := range f.listed[workspace] { + if catalog["name"] == name { + _ = json.NewEncoder(w).Encode(catalog) + + return + } + } + + http.Error(w, `{"error_code":"CATALOG_DOES_NOT_EXIST","message":"not found"}`, http.StatusNotFound) +} + +func (f *fakeCatalogAccount) servePermissions(w http.ResponseWriter, r *http.Request, catalog string) { + f.permHosts = append(f.permHosts, r.Header.Get("X-Test-Workspace")) + if f.permStatus != 0 { + http.Error(w, fmt.Sprintf(`{"error_code":%q,"message":"nope"}`, f.permErrorCode), f.permStatus) + return + } + if principal := r.URL.Query().Get("principal"); principal != "" { + f.filteredReads = append(f.filteredReads, principal) + _ = json.NewEncoder(w).Encode(map[string]any{"privilege_assignments": f.ownPrivileges}) + + return + } + if r.Method == http.MethodPatch { + if f.patchStatus != 0 { + http.Error(w, `{"message":"denied"}`, f.patchStatus) + return + } + body, _ := io.ReadAll(r.Body) + var changes struct { + Changes []map[string]any `json:"changes"` + } + decoder := json.NewDecoder(strings.NewReader(string(body))) + decoder.UseNumber() + _ = decoder.Decode(&changes) + for _, change := range changes.Changes { + f.patches = append(f.patches, change) + f.apply(catalog, change) + } + } + _ = json.NewEncoder(w).Encode(map[string]any{"privilege_assignments": f.assignments[catalog]}) +} + +func (f *fakeCatalogAccount) apply(catalog string, change map[string]any) { + added, _ := change["add"].([]any) + for _, privilege := range added { + f.assignments[catalog] = append(f.assignments[catalog], map[string]any{ + "principal": change["principal"], "principal_id": 1, "privileges": []string{privilege.(string)}, + }) + } + remove, ok := change["remove"].([]any) + if !ok { + return + } + kept := f.assignments[catalog][:0] + for _, assignment := range f.assignments[catalog] { + if fmt.Sprint(assignment["principal_id"]) != fmt.Sprint(change["principal_id"]) || + !slices.Contains(assignment["privileges"].([]string), remove[0].(string)) { + kept = append(kept, assignment) + } + } + f.assignments[catalog] = kept +} + +func newTestCatalogBuilder(t *testing.T, f *fakeCatalogAccount, allowlist []string, sync syncPrincipals) *catalogBuilder { + return newTestCatalogBuilderAs(t, f, allowlist, sync, testOwnPrincipal) +} + +func newTestCatalogBuilderAs(t *testing.T, f *fakeCatalogAccount, allowlist []string, sync syncPrincipals, clientID string) *catalogBuilder { + t.Setenv("BATON_HTTP_CACHE_TTL", "0") + client, _ := newWorkspaceRoutedClient(t, f.handler(t)) + + return newCatalogBuilder(client, clientID, configuredWorkspaceSet(allowlist), sync) +} + +var testMetastoreResourceID = &v2.ResourceId{ResourceType: metastoreResourceType.Id, Resource: "ms-1"} + +func catalogEntry(name, owner string) map[string]any { + return map[string]any{"name": name, "metastore_id": "ms-1", "owner": owner} +} + +func manyCatalogs(prefix string, n int) []map[string]any { + catalogs := make([]map[string]any, 0, n) + for i := range n { + catalogs = append(catalogs, catalogEntry(fmt.Sprintf("%s_%03d", prefix, i), "someone")) + } + + return catalogs +} + +// testCatalog is a catalog resource as List writes it, routed through wsa, in a +// metastore the connector's own principal owns. That is the live shape: ownership +// settles grant visibility with no probe call at all. +func testCatalog(t *testing.T, name, owner, workspace string) *v2.Resource { + t.Helper() + + return testCatalogOf(t, name, owner, testOwnPrincipal, workspace) +} + +func testCatalogOf(t *testing.T, name, owner, metastoreOwner, workspace string) *v2.Resource { + t.Helper() + r, err := catalogResource(&databricks.Catalog{Name: name, MetastoreID: "ms-1", Owner: owner}, "ms-1", testMetastoreResourceID, + catalogRoute{Workspace: workspace, WorkspaceID: "1", WorkspaceName: "prod", MetastoreOwner: metastoreOwner}) + if err != nil { + t.Fatalf("catalogResource: %v", err) + } + + return r +} + +// listAllCatalogs drives List the way the SDK does, page token by page token. +func listAllCatalogs(t *testing.T, b *catalogBuilder) ([]*v2.Resource, error) { + t.Helper() + + var all []*v2.Resource + token := "" + attrs := newTestSyncAttrs(t) + for range 1000 { + attrs.PageToken = pagination.Token{Token: token} + resources, results, err := b.List(context.Background(), testMetastoreResourceID, attrs) + if err != nil { + return all, err + } + all = append(all, resources...) + if results == nil || results.NextPageToken == "" { + return all, nil + } + token = results.NextPageToken + } + t.Fatal("List never returned an empty page token") + + return nil, nil +} + +// Catalogs are read through every running in-scope workspace. A catalog bound +// to only one of them is absent from the others, so listing a single workspace +// would drop it. A catalog both of them list is emitted twice, once per route, +// and the store collapses the copies on (external_id, sync_id). +func TestCatalogListReadsEveryWorkspace(t *testing.T) { + shared := manyCatalogs("shared", 120) + f := &fakeCatalogAccount{ + attached: []int{1, 2}, + listed: map[string][]map[string]any{ + "wsa": shared, + "wsb": append(slices.Clone(shared), manyCatalogs("only_b", 30)...), + }, + } + b := newTestCatalogBuilder(t, f, nil, syncPrincipals{}) + + resources, err := listAllCatalogs(t, b) + if err != nil { + t.Fatalf("List: %v", err) + } + + unique := map[string]*v2.Resource{} + for _, resource := range resources { + unique[resource.GetId().GetResource()] = resource + } + if len(unique) != 150 { + t.Fatalf("unique catalogs = %d, want the 120 shared catalogs plus the 30 bound only to wsb", len(unique)) + } + if _, ok := unique["ms-1::only_b_000"]; !ok { + t.Fatal("no resource with id ms-1::only_b_000") + } + if f.routed != 1 { + t.Errorf("the route was resolved %d times, want once per metastore", f.routed) + } + if want := 6; f.catalogPages != want { + t.Errorf("catalog pages read = %d, want %d (both workspaces, once each)", f.catalogPages, want) + } + + // The resource id keys the catalog by its metastore, and the profile carries + // the workspace Grants and Grant/Revoke are addressed to. + first, ok := unique["ms-1::shared_000"] + if !ok { + t.Fatalf("no resource with id ms-1::shared_000") + } + if first.GetDisplayName() != "shared_000" { + t.Errorf("display name = %q, want shared_000", first.GetDisplayName()) + } + profile := rs.GetProfile(first) + for key, want := range map[string]string{ + profileKeyMetastoreID: "ms-1", profileKeyFullName: "shared_000", profileKeyCatalogName: "shared_000", + profileKeyOwner: "someone", profileKeySecurableType: securableTypeCatalog, + // Grants reads the metastore's owner off the profile, so List writes it down. + profileKeyMetastoreOwner: "eng", + } { + if got, _ := rs.GetProfileStringValue(profile, key); got != want { + t.Errorf("profile[%s] = %q, want %q", key, got, want) + } + } + // Grants reads the workspace the route picked. A catalog listed by both + // workspaces keeps the last route to emit it, since that is the copy the + // store's upsert leaves behind. Either one serves the read: a route only + // emits a catalog it lists and can prove it reads every privilege on. + workspace, _ := rs.GetProfileStringValue(profile, profileKeyWorkspace) + if workspace != "wsb" { + t.Errorf("profile[%s] = %q, want wsb", profileKeyWorkspace, workspace) + } + if onlyB, ok := unique["ms-1::only_b_000"]; ok { + workspace, _ := rs.GetProfileStringValue(rs.GetProfile(onlyB), profileKeyWorkspace) + if workspace != "wsb" { + t.Errorf("profile[%s] = %q, want wsb for a catalog only wsb lists", profileKeyWorkspace, workspace) + } + } +} + +// A catalog whose privilege list is partial fails the sync instead of being left +// out. Leaving it out is not the quiet half: ConductorOne deletes every resource +// a sync stops emitting, so the catalog, its grants and its whole subtree would +// go, where failing keeps the state C1 already holds. +func TestCatalogListFailsOnACatalogItCannotFullyRead(t *testing.T) { + f := &fakeCatalogAccount{ + attached: []int{1}, + listed: map[string][]map[string]any{"wsa": {catalogEntry("main", "someone")}}, + workspaceMembers: map[string]any{"1": []map[string]any{{ + "principal": map[string]any{"service_principal_name": "app-9"}, + "permissions": []string{"ADMIN"}, + }}}, + } + b := newTestCatalogBuilderAs(t, f, nil, syncPrincipals{}, "app-9") + + resources, err := listAllCatalogs(t, b) + if status.Code(err) != codes.PermissionDenied { + t.Fatalf("List = %v, want PermissionDenied", err) + } + if !strings.Contains(err.Error(), "main") || !strings.Contains(err.Error(), catalogTrustRemedy) { + t.Errorf("error %q does not name the catalog and the remedy", err) + } + if len(resources) != 0 { + t.Errorf("listed %d catalogs, want none", len(resources)) + } +} + +// Built-in catalogs stay excluded even when metastore ownership changes. +func TestCatalogListSkipsBuiltInCatalogs(t *testing.T) { + f := &fakeCatalogAccount{ + attached: []int{1}, + listed: map[string][]map[string]any{"wsa": { + catalogEntry(systemCatalog, "someone"), + {"name": "samples", "metastore_id": "ms-1", "owner": "System user", "catalog_type": "SYSTEM_CATALOG"}, + catalogEntry("owned", "app-9"), + }}, + workspaceMembers: map[string]any{"1": []map[string]any{{ + "principal": map[string]any{"service_principal_name": "app-9"}, + "permissions": []string{"ADMIN"}, + }}}, + } + b := newTestCatalogBuilderAs(t, f, nil, syncPrincipals{}, "app-9") + + resources, err := listAllCatalogs(t, b) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(resources) != 1 || resources[0].GetDisplayName() != "owned" { + t.Fatalf("listed %d catalogs, want only the one it owns", len(resources)) + } + f.metastoreOwner = "app-9" + resources, err = listAllCatalogs(t, b) + if err != nil || len(resources) != 1 || resources[0].GetDisplayName() != "owned" { + t.Fatalf("List with metastore ownership = %v, %v; want only owned", resources, err) + } +} + +// Which workspaces serve the metastore decides which catalogs are in scope, so an +// assignment list the connector cannot read is unknown, not empty. Listing no +// catalogs would make ConductorOne delete the metastore's whole subtree. Only the +// NotFound has to be re-coded: the SDK reads that one as "this parent has no +// children" and skips the List as a warning. A 403 already fails the sync. +func TestCatalogListFailsWhenTheAssignmentListCannotBeRead(t *testing.T) { + for code, want := range map[int]codes.Code{ + http.StatusNotFound: codes.FailedPrecondition, + http.StatusForbidden: codes.PermissionDenied, + } { + t.Run(http.StatusText(code), func(t *testing.T) { + f := &fakeCatalogAccount{attached: []int{1}, attachedStatus: code} + b := newTestCatalogBuilder(t, f, nil, syncPrincipals{}) + + resources, err := listAllCatalogs(t, b) + if status.Code(err) != want { + t.Fatalf("List = %v, want %s", err, want) + } + if !strings.Contains(err.Error(), "ms-1") { + t.Errorf("error %q does not name the metastore", err) + } + if len(resources) != 0 { + t.Errorf("listed %d catalogs, want none", len(resources)) + } + }) + } +} + +// Grants reports the assignments stored on the catalog. List is what refuses a +// partial privilege list, so Grants does not prove visibility again. +func TestCatalogGrantsReportStoredAssignments(t *testing.T) { + // The principal filter answers with the group the privilege is inherited + // through, so the row names the group and not the connector's principal. + manageThroughAGroup := []map[string]any{ + {"principal": "data-eng", "principal_id": 11, "privileges": []string{"MANAGE"}}, + } + allPrivileges := []map[string]any{ + {"principal": "app-9", "principal_id": 3, "privileges": []string{"ALL_PRIVILEGES"}}, + } + + tests := []struct { + name string + clientID string + metastoreOwner string + catalogOwner string + ownPrivileges []map[string]any + wantGrants bool + wantMemberLists int + wantFilteredReads int + }{ + { + name: "owner of the metastore", clientID: "eng", + metastoreOwner: "eng", catalogOwner: "someone", + wantGrants: true, + }, + { + name: "owner of the catalog", clientID: "app-9", + metastoreOwner: "eng", catalogOwner: "app-9", + wantGrants: true, + }, + { + name: "manage inherited through a group", clientID: "app-9", + metastoreOwner: "eng", catalogOwner: "someone", ownPrivileges: manageThroughAGroup, + wantGrants: true, + }, + { + name: "all privileges without manage", clientID: "app-9", + metastoreOwner: "eng", catalogOwner: "someone", ownPrivileges: allPrivileges, + wantGrants: true, + }, + { + name: "none of them", clientID: "app-9", + metastoreOwner: "eng", catalogOwner: "someone", + wantGrants: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + f := &fakeCatalogAccount{ + users: map[string]string{"1": "alice@example.com"}, + workspaceMembers: map[string]any{}, + ownPrivileges: test.ownPrivileges, + assignments: map[string][]map[string]any{"main": { + {"principal": "alice@example.com", "principal_id": 1, "privileges": []string{"SELECT"}}, + }}, + } + b := newTestCatalogBuilderAs(t, f, nil, syncPrincipals{}, test.clientID) + resource := testCatalogOf(t, "main", test.catalogOwner, test.metastoreOwner, "wsa") + + grants, _, err := b.Grants(context.Background(), resource, newTestSyncAttrs(t)) + switch { + case test.wantGrants && err != nil: + t.Fatalf("Grants: %v", err) + case test.wantGrants && len(grants) == 0: + t.Fatal("Grants returned nothing, want the privileges on the catalog") + case !test.wantGrants: + if status.Code(err) != codes.PermissionDenied { + t.Fatalf("Grants = %v, want PermissionDenied", err) + } + if len(grants) != 0 { + t.Errorf("grants = %v, want none", grantKeys(grants)) + } + if !strings.Contains(err.Error(), "main") || !strings.Contains(err.Error(), catalogTrustRemedy) { + t.Errorf("error %q does not name the catalog and the remedy", err) + } + } + if len(f.memberLists) != test.wantMemberLists { + t.Errorf("workspace assignments listed %d times, want %d", len(f.memberLists), test.wantMemberLists) + } + if len(f.filteredReads) != test.wantFilteredReads { + t.Errorf("own privileges read %d times, want %d", len(f.filteredReads), test.wantFilteredReads) + } + }) + } +} + +func TestCatalogTrustChecksTheWorkspaceNamedByItsAdminsGroupOwner(t *testing.T) { + f := &fakeCatalogAccount{ + workspaceMembers: map[string]any{"2": []map[string]any{{ + "principal": map[string]any{"service_principal_name": "app-9"}, + "permissions": []string{"ADMIN"}, + }}}, + } + client, _ := newWorkspaceRoutedClient(t, f.handler(t)) + trust := securableTrust{ + ownPrincipal: "app-9", + owner: "_workspace_admins_dev_2", + fullName: "main", + workspace: "wsa", + workspaceID: "1", + } + trustworthy, err := catalogGrantsAreTrustworthy(context.Background(), client, trust, &annotations.Annotations{}) + if err != nil || !trustworthy { + t.Fatalf("catalogGrantsAreTrustworthy = %v, %v; want true", trustworthy, err) + } + if len(f.memberLists) != 1 || f.memberLists[0] != "2" { + t.Fatalf("workspace admin checks = %v, want [2]", f.memberLists) + } +} + +// A privilege list the connector cannot fully read cannot be written to either: +// the change would be decided from the subset Databricks chose to show. +func TestCatalogProvisioningRefusesAPartialPrivilegeList(t *testing.T) { + for _, verb := range []string{"grant", "revoke"} { + t.Run(verb, func(t *testing.T) { + f := catalogProvisioningAccount() + f.assignments["main"] = []map[string]any{ + {"principal": "alice@example.com", "principal_id": 1, "privileges": []string{"USE_CATALOG"}}, + } + b := newTestCatalogBuilderAs(t, f, nil, syncPrincipals{}, "app-9") + g := grant.NewGrant(testCatalogOf(t, "main", "someone", "eng", "wsa"), "USE_CATALOG", alice.GetId()) + + var err error + if verb == "grant" { + _, err = b.Grant(context.Background(), alice, g.GetEntitlement()) + } else { + _, err = b.Revoke(context.Background(), g) + } + if status.Code(err) != codes.PermissionDenied { + t.Fatalf("%s = %v, want PermissionDenied", verb, err) + } + if len(f.patches) != 0 { + t.Errorf("patches = %v, want none", f.patches) + } + }) + } +} + +func catalogProvisioningAccount() *fakeCatalogAccount { + return &fakeCatalogAccount{ + attached: []int{1}, + listed: map[string][]map[string]any{"wsa": {catalogEntry("main", "someone")}}, + users: map[string]string{"1": "alice@example.com"}, + assignments: map[string][]map[string]any{}, + } +} + +func TestCatalogRevokeNeedsAnUnboundReadToProveDeletion(t *testing.T) { + tests := []struct { + name string + owner string + unboundCatalogs []map[string]any + wantGone bool + wantReads int + }{ + {name: "deleted", owner: testOwnPrincipal, wantGone: true, wantReads: 1}, + {name: "still exists outside sync scope", owner: testOwnPrincipal, + unboundCatalogs: []map[string]any{catalogEntry("main", "someone")}, wantReads: 1}, + {name: "caller is not metastore owner", owner: "app-9"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + f := catalogProvisioningAccount() + f.listed = map[string][]map[string]any{"wsa": {}} + f.unboundCatalogs = test.unboundCatalogs + b := newTestCatalogBuilderAs(t, f, nil, syncPrincipals{}, test.owner) + g := grant.NewGrant(testCatalog(t, "main", "someone", "wsa"), "USE_CATALOG", alice.GetId()) + + annos, err := b.Revoke(context.Background(), g) + if test.wantGone { + if err != nil || !annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Fatalf("Revoke = %v, %v; want GrantAlreadyRevoked", annos, err) + } + } else if status.Code(err) != codes.FailedPrecondition || annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Fatalf("Revoke = %v, %v; want FailedPrecondition", annos, err) + } + if f.includeUnboundReads != test.wantReads { + t.Errorf("include_unbound reads = %d, want %d", f.includeUnboundReads, test.wantReads) + } + if len(f.patches) != 0 { + t.Errorf("patches = %v, want none", f.patches) + } + }) + } +} + +func TestCatalogListAndGrantsHandleCatalogDeletion404(t *testing.T) { + t.Run("List skips only the deleted catalog", func(t *testing.T) { + f := catalogProvisioningAccount() + f.permStatus = http.StatusNotFound + f.permErrorCode = errorCodeCatalogGone + b := newTestCatalogBuilderAs(t, f, nil, syncPrincipals{}, "app-9") + + resources, _, err := b.List(context.Background(), testMetastoreResourceID, newTestSyncAttrs(t)) + if err != nil || len(resources) != 0 { + t.Fatalf("List = %v, %v; want empty result and no error", resources, err) + } + }) + + t.Run("other permissions 404 fails closed", func(t *testing.T) { + f := catalogProvisioningAccount() + f.permStatus = http.StatusNotFound + f.permErrorCode = "RESOURCE_DOES_NOT_EXIST" + b := newTestCatalogBuilderAs(t, f, nil, syncPrincipals{}, "app-9") + + _, _, err := b.List(context.Background(), testMetastoreResourceID, newTestSyncAttrs(t)) + if status.Code(err) != codes.FailedPrecondition { + t.Fatalf("List error = %v, want FailedPrecondition", err) + } + }) + + t.Run("Grants ignores a catalog deleted after List", func(t *testing.T) { + f := catalogProvisioningAccount() + f.permStatus = http.StatusNotFound + f.permErrorCode = errorCodeCatalogGone + b := newTestCatalogBuilder(t, f, nil, syncPrincipals{}) + + grants, _, err := b.Grants(context.Background(), testCatalog(t, "main", "someone", "wsa"), newTestSyncAttrs(t)) + if err != nil || len(grants) != 0 { + t.Fatalf("Grants = %v, %v; want empty result and no error", grants, err) + } + }) +} + +// A metastore the account stopped listing is deleted in ConductorOne by the +// metastore syncer. Listing its catalogs reports nothing rather than failing the +// whole sync on a parent that is already going away. +func TestCatalogListIsEmptyWhenTheMetastoreIsGone(t *testing.T) { + f := catalogProvisioningAccount() + f.metastoreGone = true + f.attachedStatus = http.StatusNotFound + f.attachedErrorCode = errorCodeMetastoreGone + b := newTestCatalogBuilder(t, f, nil, syncPrincipals{}) + + resources, _, err := b.List(context.Background(), testMetastoreResourceID, newTestSyncAttrs(t)) + if err != nil || len(resources) != 0 { + t.Fatalf("List = %v, %v; want empty result and no error", resources, err) + } +} + +// The profile records the owners the last sync saw. Trust is decided from the +// owners the live read returns instead, so ownership lost since then is seen. +func TestCatalogProvisioningDecidesTrustFromTheLiveOwner(t *testing.T) { + for _, verb := range []string{"grant", "revoke"} { + t.Run(verb, func(t *testing.T) { + f := catalogProvisioningAccount() + f.metastoreOwner = "someone-else" + b := newTestCatalogBuilderAs(t, f, nil, syncPrincipals{}, "app-9") + g := grant.NewGrant(testCatalogOf(t, "main", "app-9", "app-9", "wsa"), "USE_CATALOG", alice.GetId()) + + var err error + if verb == "grant" { + _, err = b.Grant(context.Background(), alice, g.GetEntitlement()) + } else { + _, err = b.Revoke(context.Background(), g) + } + if status.Code(err) != codes.PermissionDenied { + t.Fatalf("%s = %v, want PermissionDenied after live ownership changed", verb, err) + } + if len(f.patches) != 0 { + t.Errorf("patches = %v, want none", f.patches) + } + }) + } +} + +// A workspace the operator scoped out after the sync is still on the profile. +// Writing through it would reach a workspace this run was told to leave alone. +func TestCatalogProvisioningIgnoresAnOutOfScopeProfileWorkspace(t *testing.T) { + f := &fakeCatalogAccount{ + attached: []int{1, 2}, + listed: map[string][]map[string]any{ + "wsa": {catalogEntry("main", "someone")}, + "wsb": {catalogEntry("main", "someone")}, + }, + } + b := newTestCatalogBuilder(t, f, []string{"prod"}, syncPrincipals{}) + resource, err := catalogResource(&databricks.Catalog{Name: "main", MetastoreID: "ms-1", Owner: "someone"}, + "ms-1", testMetastoreResourceID, catalogRoute{ + Workspace: "wsb", WorkspaceID: "2", WorkspaceName: "dev", MetastoreOwner: testOwnPrincipal, + }) + if err != nil { + t.Fatalf("catalogResource: %v", err) + } + entitlement := grant.NewGrant(resource, "USE_CATALOG", alice.GetId()).GetEntitlement() + + routed, err := b.entitlementOnLiveRoute(context.Background(), entitlement, &annotations.Annotations{}) + if err != nil { + t.Fatalf("entitlementOnLiveRoute: %v", err) + } + workspace, _ := rs.GetProfileStringValue(rs.GetProfile(routed.GetResource()), profileKeyWorkspace) + if workspace != "wsa" { + t.Fatalf("routed workspace = %q, want wsa", workspace) + } + if slices.Contains(f.catalogGets, "wsb") { + t.Errorf("catalog was probed through out-of-scope workspace: %v", f.catalogGets) + } +} + +// A principal that is gone from SCIM still carries its numeric id on the row, so +// the removal goes by principal_id; an absent privilege is already revoked. +func TestCatalogRevokeRemovesByIDAndIsIdempotent(t *testing.T) { + f := catalogProvisioningAccount() + f.assignments["main"] = []map[string]any{{"principal": "alice@example.com", "principal_id": 1, "privileges": []string{"USE_CATALOG"}}} + b := newTestCatalogBuilder(t, f, nil, syncPrincipals{}) + g := grant.NewGrant(testCatalog(t, "main", "someone", "wsa"), "USE_CATALOG", alice.GetId()) + + annos, err := b.Revoke(context.Background(), g) + if err != nil { + t.Fatalf("Revoke: %v", err) + } + if annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Error("revoking a held privilege reported GrantAlreadyRevoked") + } + if len(f.patches) != 1 || fmt.Sprint(f.patches[0]["principal_id"]) != "1" { + t.Fatalf("patches = %v, want one removal by principal_id 1", f.patches) + } + + annos, err = b.Revoke(context.Background(), g) + if err != nil || !annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Fatalf("second Revoke = %v, %v; want GrantAlreadyRevoked", annos, err) + } +} + +// Only the error_code proves the catalog is gone. A bare 404 on the privilege +// read also comes back from a workspace the catalog is not bound to, where the +// privilege is still held, so that one stays an error. +func TestCatalogRevokeTellsADeletedCatalogFromAnUnreachableOne(t *testing.T) { + tests := []struct { + name string + errorCode string + wantGone bool + }{ + {name: "deleted catalog", errorCode: errorCodeCatalogGone, wantGone: true}, + {name: "unbound workspace", errorCode: ""}, + {name: "some other 404", errorCode: "RESOURCE_DOES_NOT_EXIST"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + f := catalogProvisioningAccount() + f.permStatus = http.StatusNotFound + f.permErrorCode = test.errorCode + b := newTestCatalogBuilder(t, f, nil, syncPrincipals{}) + g := grant.NewGrant(testCatalog(t, "main", "someone", "wsa"), "USE_CATALOG", alice.GetId()) + + annos, err := b.Revoke(context.Background(), g) + switch { + case test.wantGone: + if err != nil || !annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Fatalf("Revoke = %v, %v; want GrantAlreadyRevoked", annos, err) + } + default: + if err == nil { + t.Fatalf("Revoke returned %v and no error; want the revoke to fail", annos) + } + if annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Error("a 404 without the gone code reported GrantAlreadyRevoked") + } + } + if len(f.patches) != 0 { + t.Errorf("patches = %v, want none", f.patches) + } + }) + } +} + +// A catalog no workspace in scope lists is not a catalog proven gone: an ISOLATED +// catalog bound to an excluded workspace, and one the connector lost USE_CATALOG +// on, both read the same way and still hold the privilege. Reporting +// GrantAlreadyRevoked would leave that privilege in Databricks while +// ConductorOne recorded it as revoked. +func TestCatalogRevokeOnAnUnlistedCatalogDoesNotReportAlreadyRevoked(t *testing.T) { + f := catalogProvisioningAccount() + // 403, not a 404 naming the catalog: this workspace does not serve the + // catalog, which says nothing about whether it exists. + f.listStatus = map[string]int{"wsa": http.StatusForbidden} + b := newTestCatalogBuilder(t, f, nil, syncPrincipals{}) + g := grant.NewGrant(testCatalog(t, "main", "someone", "wsa"), "USE_CATALOG", alice.GetId()) + + annos, err := b.Revoke(context.Background(), g) + if status.Code(err) != codes.FailedPrecondition { + t.Fatalf("Revoke = %v, want FailedPrecondition", err) + } + if annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Error("an unlisted catalog reported GrantAlreadyRevoked") + } + if len(f.patches) != 0 { + t.Errorf("patches = %v, want none", f.patches) + } +} + +// testCatalogSession is the in-memory session store the sync methods need. The +// connector keeps each metastore's catalog routes and its workspace Admin checks +// there for the length of one sync, so List has no route to walk without it. +type testCatalogSession struct { + session.NoOpSessionStore + mu sync.Mutex + values map[string][]byte +} + +func (s *testCatalogSession) Get(_ context.Context, key string, _ ...sessions.SessionStoreOption) ([]byte, bool, error) { + s.mu.Lock() + defer s.mu.Unlock() + value, found := s.values[key] + + return value, found, nil +} + +func (s *testCatalogSession) Set(_ context.Context, key string, value []byte, _ ...sessions.SessionStoreOption) error { + s.mu.Lock() + defer s.mu.Unlock() + s.values[key] = append([]byte(nil), value...) + + return nil +} + +func (s *testCatalogSession) SetMany(ctx context.Context, values map[string][]byte, opts ...sessions.SessionStoreOption) error { + for key, value := range values { + if err := s.Set(ctx, key, value, opts...); err != nil { + return err + } + } + + return nil +} + +func newTestSyncAttrs(t *testing.T) rs.SyncOpAttrs { + t.Helper() + + return rs.SyncOpAttrs{Session: &testCatalogSession{values: make(map[string][]byte)}} +} diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 09b739d2..85d84aa4 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -29,6 +29,12 @@ 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 { + workspaces := configuredWorkspaceSet(d.workspaces) + principals := syncPrincipals{ + groups: d.willSyncResourceType(groupResourceType.Id), + servicePrincipals: d.willSyncResourceType(servicePrincipalResourceType.Id), + } + return []connectorbuilder.ResourceSyncerV2{ newAccountBuilder(d.client), newGroupBuilder(d.client), @@ -36,10 +42,8 @@ func (d *Databricks) ResourceSyncers(_ context.Context) []connectorbuilder.Resou newUserBuilder(d.client), newWorkspaceBuilder(d.client, d.workspaces), newRoleBuilder(d.client), - newMetastoreBuilder(d.client, d.clientID, configuredWorkspaceSet(d.workspaces), syncPrincipals{ - groups: d.willSyncResourceType(groupResourceType.Id), - servicePrincipals: d.willSyncResourceType(servicePrincipalResourceType.Id), - }), + newMetastoreBuilder(d.client, d.clientID, workspaces, principals), + newCatalogBuilder(d.client, d.clientID, workspaces, principals), } } @@ -71,7 +75,7 @@ func (d *Databricks) Metadata(_ context.Context) (*v2.ConnectorMetadata, error) return &v2.ConnectorMetadata{ DisplayName: "Databricks", Description: "Connector syncing Databricks workspaces, users, groups, service principals and roles to Baton, " + - "along with Unity Catalog metastores and the privileges granted on them", + "along with Unity Catalog data assets — metastores and catalogs — and the privileges granted on them", AccountCreationSchema: &v2.ConnectorAccountCreationSchema{ FieldMap: map[string]*v2.ConnectorAccountCreationSchema_Field{ "email": { @@ -154,8 +158,10 @@ func (d *Databricks) Validate(ctx context.Context) (annotations.Annotations, err _, ok := matchConfiguredWorkspace(configuredWorkspaceSet(d.workspaces), w.DeploymentName, w.Name, strconv.Itoa(w.ID)) return ok }) { + // ListWorkspaces already dropped the excluded ones, so a name that is + // only excluded would otherwise be reported as absent from the account. return annos, fmt.Errorf( - "databricks-connector: none of the configured workspaces (%s) is on the account", + "databricks-connector: none of the configured workspaces (%s) is on the account and not excluded by databricks-exclude-workspaces", strings.Join(d.workspaces, ", ")) } diff --git a/pkg/connector/connector_test.go b/pkg/connector/connector_test.go index e64c4558..c38751f9 100644 --- a/pkg/connector/connector_test.go +++ b/pkg/connector/connector_test.go @@ -75,7 +75,7 @@ func TestWillSyncResourceType(t *testing.T) { // The SDK derives capabilities and Grant/Revoke dispatch from the registered // syncers, so leaving metastore out of a sync must not unregister it. -func TestResourceSyncersAlwaysRegisterTheMetastoreBuilder(t *testing.T) { +func TestResourceSyncersAlwaysRegisterTheUnityCatalogBuilders(t *testing.T) { t.Parallel() d := &Databricks{opts: &cli.ConnectorOpts{ @@ -87,7 +87,9 @@ func TestResourceSyncersAlwaysRegisterTheMetastoreBuilder(t *testing.T) { for _, syncer := range d.ResourceSyncers(ctx) { registered = append(registered, syncer.ResourceType(ctx).GetId()) } - if !slices.Contains(registered, metastoreResourceType.Id) { - t.Fatalf("registered syncers = %v, want them to include %s", registered, metastoreResourceType.Id) + for _, want := range []string{metastoreResourceType.Id, catalogResourceType.Id} { + if !slices.Contains(registered, want) { + t.Errorf("registered syncers = %v, want them to include %s", registered, want) + } } } diff --git a/pkg/connector/metastore.go b/pkg/connector/metastore.go index 726f8208..2affac3c 100644 --- a/pkg/connector/metastore.go +++ b/pkg/connector/metastore.go @@ -2,7 +2,6 @@ package connector import ( "context" - "encoding/json" "errors" "fmt" "slices" @@ -13,28 +12,26 @@ import ( v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/connectorbuilder" + "github.com/conductorone/baton-sdk/pkg/session" ent "github.com/conductorone/baton-sdk/pkg/types/entitlement" - "github.com/conductorone/baton-sdk/pkg/types/grant" rs "github.com/conductorone/baton-sdk/pkg/types/resource" - "github.com/conductorone/baton-sdk/pkg/uhttp" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/proto" ) var _ connectorbuilder.StaticEntitlementSyncerV2 = (*metastoreBuilder)(nil) type metastoreBuilder struct { - client *databricks.Client - clientID string + securableOps // workspaces is the configured allowlist, empty when none was given. workspaces map[string]struct{} - sync syncPrincipals } func newMetastoreBuilder(client *databricks.Client, clientID string, workspaces map[string]struct{}, sync syncPrincipals) *metastoreBuilder { - return &metastoreBuilder{client: client, clientID: clientID, workspaces: workspaces, sync: sync} + return &metastoreBuilder{securableOps: securableOps{client: client, clientID: clientID, sync: sync, level: metastoreLevel}, workspaces: workspaces} } func (m *metastoreBuilder) ResourceType(_ context.Context) *v2.ResourceType { @@ -65,18 +62,45 @@ func metastoreResource(_ context.Context, metastore *databricks.Metastore, paren nil, rs.WithResourceProfile(profile), rs.WithParentResourceID(parent), - rs.WithAnnotation(&v2.RawId{Id: metastore.MetastoreID}), + rs.WithAnnotation( + &v2.ChildResourceType{ResourceTypeId: catalogResourceType.Id}, + &v2.RawId{Id: metastore.MetastoreID}, + ), ) } +// metastorePermissionsName: a metastore is addressed by a bare UUID. The +// separator only appears in the composite ids of the securables below it, so +// finding one here means the wrong resource arrived. +func metastorePermissionsName(resourceID string) (string, error) { + if resourceID == "" || strings.Contains(resourceID, securableKeySeparator) { + return "", status.Errorf(codes.InvalidArgument, "databricks-connector: invalid metastore resource id %q", resourceID) + } + + return resourceID, nil +} + +var metastoreLevel = securableLevel{ + securableType: databricks.SecurableMetastore, + noun: "metastore", + privileges: metastorePrivileges, + // MANAGE is not assignable on a metastore, so naming it here would send the + // operator after a privilege Databricks rejects. The authority is ownership. + writeRemedy: "the connector must own the metastore or be Admin on an attached workspace", + trustRemedy: "the connector's service principal must own the metastore or be assigned Admin directly on the workspace", + permissionsName: metastorePermissionsName, + trusts: metastoreGrantsAreTrustworthy, +} + // List emits each metastore with the workspace its permissions are read through. // A metastore is left out only when no attached workspace is in scope or usable. // A pending workspace fails the sync instead, so ConductorOne keeps the metastore. -func (m *metastoreBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, _ rs.SyncOpAttrs) ([]*v2.Resource, *rs.SyncOpResults, error) { +func (m *metastoreBuilder) List(ctx context.Context, parentResourceID *v2.ResourceId, attr rs.SyncOpAttrs) ([]*v2.Resource, *rs.SyncOpResults, error) { if parentResourceID.GetResourceType() != accountResourceType.Id { return nil, nil, nil } + ctx = context.WithValue(ctx, syncSessionKey{}, attr.Session) annos := annotations.Annotations{} metastores, rateLimit, err := m.client.ListMetastores(ctx) noteRateLimit(&annos, rateLimit) @@ -84,25 +108,26 @@ func (m *metastoreBuilder) List(ctx context.Context, parentResourceID *v2.Resour return nil, &rs.SyncOpResults{Annotations: annos}, refuseUnreadable("databricks-connector: failed to list metastores", err) } - scoped, running, err := m.scopedWorkspaces(ctx, &annos) + scope, err := scopedWorkspaces(ctx, m.client, m.workspaces, &annos) if err != nil { return nil, &rs.SyncOpResults{Annotations: annos}, err } l := ctxzap.Extract(ctx) var rv []*v2.Resource + routes := make(map[string][]catalogRoute) for _, metastore := range metastores { if metastore.MetastoreID == "" { continue } // A 404 here does not mean the metastore is gone. Skipping it would make ConductorOne delete it. - workspace, pending, err := m.routeMetastore(ctx, metastore.MetastoreID, metastore.Owner, scoped, running, &annos) + usable, inScope, err := scope.attached(ctx, m.client, metastore.MetastoreID, &annos) if err != nil { - return nil, &rs.SyncOpResults{Annotations: annos}, err + return nil, &rs.SyncOpResults{Annotations: annos}, assignmentListError(metastore.MetastoreID, err) } - if workspace == nil { - if pending { + if len(usable) == 0 { + if inScope { return nil, &rs.SyncOpResults{Annotations: annos}, status.Errorf(codes.FailedPrecondition, "databricks-connector: metastore %s has no running workspace in sync scope", metastore.MetastoreID) } @@ -111,6 +136,11 @@ func (m *metastoreBuilder) List(ctx context.Context, parentResourceID *v2.Resour continue } + routes[catalogRoutesKey(metastore.MetastoreID)] = catalogRoutesForWorkspaces(usable, metastore.Owner) + workspace, err := pickReadableWorkspace(ctx, m.client, m.clientID, metastore.Owner, metastore.MetastoreID, usable, &annos) + if err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, err + } mr, err := metastoreResource(ctx, &metastore, parentResourceID, workspace.DeploymentName, strconv.Itoa(workspace.ID), workspace.Name) if err != nil { return nil, &rs.SyncOpResults{Annotations: annos}, err @@ -118,6 +148,9 @@ func (m *metastoreBuilder) List(ctx context.Context, parentResourceID *v2.Resour rv = append(rv, mr) } + if err := session.SetManyJSON(ctx, attr.Session, routes); err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, fmt.Errorf("databricks-connector: failed to store metastore routes: %w", err) + } return rv, &rs.SyncOpResults{Annotations: annos}, nil } @@ -143,258 +176,127 @@ func (m *metastoreBuilder) Entitlements(context.Context, *v2.Resource, rs.SyncOp } func (m *metastoreBuilder) Grants(ctx context.Context, resource *v2.Resource, attr rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { - l := ctxzap.Extract(ctx) - metastoreID := resource.GetId().GetResource() - profile := rs.GetProfile(resource) - // List writes the route. An empty one is a resource this sync did not emit. - workspace, _ := rs.GetProfileStringValue(profile, profileKeyWorkspace) - if workspace == "" { - return nil, nil, status.Errorf(codes.FailedPrecondition, - "databricks-connector: metastore %s has no workspace on its profile", metastoreID) - } + return m.syncGrants(ctx, resource, attr) +} +func (m *metastoreBuilder) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { + ctx = databricks.WithoutCache(ctx) annos := annotations.Annotations{} - pageToken := attr.PageToken.Token - assignments, nextPageToken, rateLimit, err := m.client.ListPermissions(ctx, workspace, databricks.SecurableMetastore, metastoreID, "", pageToken) - noteRateLimit(&annos, rateLimit) + routed, err := m.entitlementOnLiveRoute(ctx, entitlement, &annos) if err != nil { - // Only a NotFound is FailedPrecondition. The SDK reads NotFound as "no - // grants", and whether the metastore is gone is settled by the next List. - // A throttle or a 5xx keeps the status the client returned, so the SDK retries it. - return nil, &rs.SyncOpResults{Annotations: annos}, refuseUnreadable(fmt.Sprintf( - "databricks-connector: failed to list permissions on metastore %s through workspace %s", metastoreID, workspace), err) - } - - resolver := newPrincipalResolver(m.client, &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) - } - if principalId == nil { - l.Debug("databricks-connector: skipping a grant whose principal does not resolve", - zap.String("metastore", metastoreID), - zap.String("principal", assignment.Principal), - zap.String("principal_id", assignment.PrincipalID.String())) - continue - } - if !emitPrincipal(m.sync, principalId.GetResourceType()) { - continue + if errors.Is(err, errMetastoreGone) { + return annos, status.Errorf(codes.NotFound, + "databricks-connector: metastore %s is no longer on the account", entitlement.GetResource().GetId().GetResource()) } - options := groupSecurableGrantOptions(principalId) - for _, privilege := range assignment.Privileges { - if slices.Contains(metastorePrivileges, privilege) { - rv = append(rv, grant.NewGrant(resource, privilege, principalId, options...)) - } - } + return annos, err } - // The owner is emitted once, with the first page. - if owner, _ := rs.GetProfileStringValue(profile, profileKeyOwner); pageToken == "" && owner != "" { - principalId, err := resolver.resolve(ctx, "", owner) - if err != nil { - return nil, &rs.SyncOpResults{Annotations: annos}, fmt.Errorf("databricks-connector: %w", err) - } - if principalId != nil && emitPrincipal(m.sync, principalId.GetResourceType()) { - options := append(groupSecurableGrantOptions(principalId), grant.WithAnnotation(&v2.GrantImmutable{})) - rv = append(rv, grant.NewGrant(resource, ownerEntitlement, principalId, options...)) + return m.grant(ctx, principal.GetId(), routed) +} + +func (m *metastoreBuilder) Revoke(ctx context.Context, revoked *v2.Grant) (annotations.Annotations, error) { + ctx = databricks.WithoutCache(ctx) + annos := annotations.Annotations{} + routed, err := m.entitlementOnLiveRoute(ctx, revoked.GetEntitlement(), &annos) + if err != nil { + if errors.Is(err, errMetastoreGone) { + annos.Update(&v2.GrantAlreadyRevoked{}) + return annos, nil } - } - if nextPageToken != "" && nextPageToken == pageToken { - return nil, &rs.SyncOpResults{Annotations: annos}, status.Errorf(codes.Internal, - "databricks-connector: listing privileges on metastore %s repeated page token %q", metastoreID, nextPageToken) + return annos, err } - return rv, &rs.SyncOpResults{Annotations: annos, NextPageToken: nextPageToken}, nil -} - -type metastoreChange struct { - metastoreID string - workspace string - privilege string - scimID string - principalName string + return m.revoke(ctx, revoked.GetPrincipal().GetId(), routed) } -// resolveChange validates a Grant/Revoke and resolves what the PATCH needs. -func (m *metastoreBuilder) resolveChange( +// entitlementOnLiveRoute returns the entitlement addressed at a workspace. +// A workspace on the profile is kept when it is still running and attached, +// with ownership refreshed from the account. Otherwise the route is resolved live. +func (m *metastoreBuilder) entitlementOnLiveRoute( ctx context.Context, - principalID *v2.ResourceId, entitlement *v2.Entitlement, - isGrant bool, annos *annotations.Annotations, -) (metastoreChange, error) { - if !isValidPrincipal(principalID) { - return metastoreChange{}, status.Errorf(codes.InvalidArgument, - "databricks-connector: only users, groups and service principals can hold a metastore privilege, got %s", principalID.GetResourceType()) - } - +) (*v2.Entitlement, error) { + ctx = databricks.WithoutCache(ctx) resource := entitlement.GetResource() metastoreID := resource.GetId().GetResource() - if metastoreID == "" { - return metastoreChange{}, status.Error(codes.InvalidArgument, "databricks-connector: missing metastore resource id") - } - - privilege, err := parseEntitlementPrivilege(entitlement.GetId()) - if err != nil { - return metastoreChange{}, err - } - switch { - case privilege == ownerEntitlement: - return metastoreChange{}, status.Errorf(codes.InvalidArgument, - "databricks-connector: metastore ownership cannot be provisioned; change the owner in Databricks") - case slices.Contains(legacyPrivileges, strings.ToUpper(strings.TrimSpace(privilege))): - return metastoreChange{}, status.Errorf(codes.InvalidArgument, - "databricks-connector: %s is a legacy alias Databricks rewrites; use the explicit privilege", privilege) - case !slices.Contains(metastorePrivileges, privilege): - return metastoreChange{}, status.Errorf(codes.InvalidArgument, - "databricks-connector: %s is not a grantable metastore privilege", privilege) - } - - // Provisioning carries ids, not the profile a sync wrote. - routed, err := m.resolveMetastoreRoute(ctx, metastoreID, annos) + metastores, rateLimit, err := m.client.ListMetastores(ctx) + noteRateLimit(annos, rateLimit) if err != nil { - if errors.Is(err, errMetastoreGone) && isGrant { - return metastoreChange{}, status.Errorf(codes.NotFound, - "databricks-connector: metastore %s is no longer on the account", metastoreID) - } - - return metastoreChange{}, err + return nil, fmt.Errorf("databricks-connector: failed to list metastores: %w", err) } - workspace := routed.DeploymentName - scimID, err := unityPrincipalScimID(principalID) - if err != nil { - return metastoreChange{}, fmt.Errorf("databricks-connector: %w", err) + found := slices.IndexFunc(metastores, func(metastore databricks.Metastore) bool { + return metastore.MetastoreID == metastoreID + }) + if found < 0 { + return nil, errMetastoreGone } - principalName, err := unityPrincipalName(ctx, m.client, principalID) - if err != nil { - if !errors.Is(err, errPrincipalNotFound) { - return metastoreChange{}, fmt.Errorf("databricks-connector: failed to resolve principal %s: %w", principalID.GetResource(), err) - } - // Revoke addresses a vanished principal's rows by id. - if isGrant { - return metastoreChange{}, status.Errorf(codes.NotFound, - "databricks-connector: principal %s no longer exists in Databricks", scimID) - } - } - - return metastoreChange{ - metastoreID: metastoreID, - workspace: workspace, - privilege: privilege, - scimID: scimID, - principalName: principalName, - }, nil -} - -func (c metastoreChange) matchHeld(assignments []databricks.PrivilegeAssignment) (databricks.PrivilegeAssignment, bool) { - return matchHeldPrivilege(assignments, c.scimID, c.principalName, c.privilege) -} - -func (c metastoreChange) updateError(verb string, err error) error { - if isForbiddenError(err) { - return uhttp.WrapErrors(codes.PermissionDenied, - fmt.Sprintf("databricks-connector: cannot %s %s on metastore %s: the connector must own the metastore or be Admin on an attached workspace", - verb, c.privilege, c.metastoreID), - err) - } - - return fmt.Errorf("databricks-connector: failed to %s %s on metastore %s: %w", verb, c.privilege, c.metastoreID, err) -} - -func (m *metastoreBuilder) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { - annos := annotations.Annotations{} - change, err := m.resolveChange(ctx, principal.GetId(), entitlement, true, &annos) + profile := rs.GetProfile(resource) + profileWorkspace, _ := rs.GetProfileStringValue(profile, profileKeyWorkspace) + scope, err := scopedWorkspaces(ctx, m.client, m.workspaces, annos) if err != nil { - return annos, err + return nil, err } - - // Read past the cache: a stale page would skip a needed PATCH. - assignments, rateLimit, err := m.client.DrainPermissions(ctx, change.workspace, databricks.SecurableMetastore, change.metastoreID) - noteRateLimit(&annos, rateLimit) + usable, _, err := scope.attached(ctx, m.client, metastoreID, annos) if err != nil { - return annos, fmt.Errorf("databricks-connector: failed to read privileges on metastore %s: %w", change.metastoreID, err) + return nil, assignmentListError(metastoreID, err) } - if _, held := change.matchHeld(assignments); held { - annos.Update(&v2.GrantAlreadyExists{}) - return annos, nil + if len(usable) == 0 { + return nil, status.Errorf(codes.FailedPrecondition, + "databricks-connector: metastore %s has no running workspace in sync scope", metastoreID) } - var echoed []databricks.PrivilegeAssignment - rateLimit, err = m.client.UpdatePermissionsUntil(ctx, change.workspace, databricks.SecurableMetastore, change.metastoreID, - []databricks.PermissionsChange{{Principal: change.principalName, Add: []string{change.privilege}}}, - func(page []databricks.PrivilegeAssignment) (bool, error) { - echoed = append(echoed, page...) - _, held := change.matchHeld(echoed) - return held, nil - }) - noteRateLimit(&annos, rateLimit) - if err != nil { - return annos, change.updateError("grant", err) + var workspace *databricks.Workspace + for _, candidate := range usable { + if candidate.DeploymentName == profileWorkspace { + workspace = &candidate + break + } } - - // Databricks no-ops a PATCH it cannot apply, so only the echo confirms it. - if _, held := change.matchHeld(echoed); !held { - return annos, status.Errorf(codes.Internal, - "databricks-connector: granting %s on metastore %s was accepted but not applied", change.privilege, change.metastoreID) + liveOwner := metastores[found].Owner + profileOwner, _ := rs.GetProfileStringValue(profile, profileKeyOwner) + // The owner decides which workspaces read every privilege: the metastore's + // owner reads through any of them, anyone else only through a workspace it + // is a direct Admin of. Either can be withdrawn after the sync that recorded + // the route, so the route is kept only when it still reads right now. + if workspace != nil { + readable := ownsPrincipal(m.clientID, liveOwner) + if !readable { + readable, err = isWorkspaceAdmin(ctx, m.client, m.clientID, strconv.Itoa(workspace.ID), annos) + if err != nil { + return nil, err + } + } + if !readable { + workspace = nil + } } - return annos, nil -} - -func (m *metastoreBuilder) Revoke(ctx context.Context, revoked *v2.Grant) (annotations.Annotations, error) { - annos := annotations.Annotations{} - change, err := m.resolveChange(ctx, revoked.GetPrincipal().GetId(), revoked.GetEntitlement(), false, &annos) - if err != nil { - if errors.Is(err, errMetastoreGone) { - annos.Update(&v2.GrantAlreadyRevoked{}) - return annos, nil + if workspace == nil { + workspace, err = pickReadableWorkspace(ctx, m.client, m.clientID, liveOwner, metastoreID, usable, annos) + if err != nil { + return nil, err + } + } else { + workspaceID, _ := rs.GetProfileStringValue(profile, profileKeyWorkspaceID) + workspaceName, _ := rs.GetProfileStringValue(profile, profileKeyWorkspaceName) + if profileOwner == liveOwner && workspaceID == strconv.Itoa(workspace.ID) && workspaceName == workspace.Name { + return entitlement, nil } - - return annos, err } - var seen []databricks.PrivilegeAssignment - rateLimit, err := m.client.ForEachUncachedPermissionsPage(ctx, change.workspace, databricks.SecurableMetastore, change.metastoreID, "", - func(page []databricks.PrivilegeAssignment) (bool, error) { - seen = append(seen, page...) - matched, ok := change.matchHeld(seen) - // Only an id match is final; a name match may be superseded on a later page. - return ok && change.scimID != "" && matched.PrincipalID.String() == change.scimID && change.scimID != "0", nil - }) - noteRateLimit(&annos, rateLimit) + routed, err := metastoreResource(ctx, &metastores[found], resource.GetParentResourceId(), + workspace.DeploymentName, strconv.Itoa(workspace.ID), workspace.Name) if err != nil { - return annos, fmt.Errorf("databricks-connector: failed to read privileges on metastore %s: %w", change.metastoreID, err) - } - - // A missed row can be a partial read, and removing a privilege nobody holds is a no-op. - held, ok := change.matchHeld(seen) - removal := databricks.PermissionsChange{Principal: change.principalName, Remove: []string{change.privilege}} - switch heldID := held.PrincipalID.String(); { - case ok && heldID != "" && heldID != "0": - removal = databricks.PermissionsChange{PrincipalID: held.PrincipalID, Remove: []string{change.privilege}} - case change.principalName == "": - removal = databricks.PermissionsChange{PrincipalID: json.Number(change.scimID), Remove: []string{change.privilege}} + return nil, err } - result, rateLimit, err := m.client.UpdatePermissions(ctx, change.workspace, databricks.SecurableMetastore, change.metastoreID, - []databricks.PermissionsChange{removal}) - noteRateLimit(&annos, rateLimit) - if err != nil { - return annos, change.updateError("revoke", err) - } - if _, stillHeld := change.matchHeld(result); stillHeld { - return annos, status.Errorf(codes.Internal, - "databricks-connector: revoking %s on metastore %s was accepted but not applied", change.privilege, change.metastoreID) - } - if !ok { - annos.Update(&v2.GrantAlreadyRevoked{}) - } + copyEntitlement := proto.Clone(entitlement).(*v2.Entitlement) + copyEntitlement.Resource = routed - return annos, nil + return copyEntitlement, nil } diff --git a/pkg/connector/metastore_helpers.go b/pkg/connector/metastore_helpers.go index 4687597f..c9b72176 100644 --- a/pkg/connector/metastore_helpers.go +++ b/pkg/connector/metastore_helpers.go @@ -12,8 +12,10 @@ import ( "github.com/conductorone/baton-databricks/pkg/databricks" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/conductorone/baton-sdk/pkg/session" "github.com/conductorone/baton-sdk/pkg/types/grant" rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/conductorone/baton-sdk/pkg/types/sessions" "github.com/conductorone/baton-sdk/pkg/uhttp" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" @@ -204,25 +206,39 @@ func unityPrincipalName(ctx context.Context, c *databricks.Client, principal *v2 return name, nil } -// 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. -func (m *metastoreBuilder) scopedWorkspaces(ctx context.Context, annos *annotations.Annotations) (map[string]databricks.Workspace, map[string]bool, error) { - workspaces, rateLimit, err := m.client.ListWorkspaces(ctx) +// workspaceScope is the set of workspaces this sync may read Unity Catalog +// through, indexed by numeric id, with the RUNNING ones marked. Both the +// metastore and the catalog syncer resolve their route through it. +type workspaceScope struct { + byID map[string]databricks.Workspace + running map[string]bool +} + +// scopedWorkspaces reads the account's workspaces and keeps the ones the +// allowlist admits. ListWorkspaces already drops the excluded ones. +func scopedWorkspaces( + ctx context.Context, + client *databricks.Client, + allowlist map[string]struct{}, + annos *annotations.Annotations, +) (workspaceScope, error) { + workspaces, rateLimit, err := client.ListWorkspaces(ctx) noteRateLimit(annos, rateLimit) if err != nil { - return nil, nil, refuseUnreadable("databricks-connector: failed to list workspaces", err) + return workspaceScope{}, refuseUnreadable("databricks-connector: failed to list workspaces", err) } - scoped := make(map[string]databricks.Workspace, len(workspaces)) - running := make(map[string]bool, len(workspaces)) + scope := workspaceScope{ + byID: make(map[string]databricks.Workspace, len(workspaces)), + running: make(map[string]bool, len(workspaces)), + } for _, workspace := range workspaces { id := strconv.Itoa(workspace.ID) if workspace.DeploymentName == "" { continue } - if len(m.workspaces) > 0 { - if _, ok := matchConfiguredWorkspace(m.workspaces, workspace.DeploymentName, workspace.Name, id); !ok { + if len(allowlist) > 0 { + if _, ok := matchConfiguredWorkspace(allowlist, workspace.DeploymentName, workspace.Name, id); !ok { continue } } @@ -232,11 +248,11 @@ func (m *metastoreBuilder) scopedWorkspaces(ctx context.Context, annos *annotati if !workspaceIsUsable(workspace.Status) { continue } - scoped[id] = workspace - running[id] = workspace.Status == "" || strings.EqualFold(workspace.Status, workspaceStatusRunning) + scope.byID[id] = workspace + scope.running[id] = workspace.Status == "" || strings.EqualFold(workspace.Status, workspaceStatusRunning) } - return scoped, running, nil + return scope, nil } // workspaceIsUsable reports a status that is RUNNING or can still become it. @@ -254,99 +270,93 @@ func workspaceIsUsable(workspaceStatus string) bool { // errMetastoreGone marks a metastore the account no longer lists. 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. -func (m *metastoreBuilder) routeMetastore( +// attached lists the running in-scope workspaces the metastore is assigned to, +// in assignment order. inScope reports whether any in-scope workspace is +// attached at all, running or not: that is what tells "outside this sync" apart +// from "in this sync but not reachable right now". +func (s workspaceScope) attached( ctx context.Context, - metastoreID, owner string, - scoped map[string]databricks.Workspace, - running map[string]bool, + client *databricks.Client, + metastoreID string, annos *annotations.Annotations, -) (*databricks.Workspace, bool, error) { - ids, rateLimit, err := m.client.ListMetastoreWorkspaces(ctx, metastoreID) +) ([]databricks.Workspace, bool, error) { + ids, rateLimit, err := 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( - "databricks-connector: failed to list the workspaces assigned to metastore %s, so the sync cannot tell which serve it", - metastoreID), err) + if databricks.IsSecurableGone(err, errorCodeMetastoreGone) { + return nil, false, errMetastoreGone + } + return nil, false, err } - pending := false - var candidates []databricks.Workspace + var usable []databricks.Workspace + inScope := false for _, id := range ids { - workspace, ok := scoped[id] + workspace, ok := s.byID[id] if !ok { continue } - if running[id] { - candidates = append(candidates, workspace) - continue - } - pending = true - } - if len(candidates) == 0 { - return nil, pending, nil - } - - if ownsPrincipal(m.clientID, owner) { - return &candidates[0], false, 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 - } - if admin { - return &candidates[i], false, nil + inScope = true + if s.running[id] { + usable = append(usable, workspace) } } - return nil, false, untrustedMetastoreError(metastoreID, candidates) + return usable, inScope, nil } -// resolveMetastoreRoute routes a metastore from its id, reading owner and standing live. -func (m *metastoreBuilder) resolveMetastoreRoute( +// pickReadableWorkspace is the first running workspace the connector can read +// every privilege through. An owner sees them all; otherwise it is the first +// workspace where the connector is a direct Admin. +func pickReadableWorkspace( ctx context.Context, - metastoreID string, + client *databricks.Client, + ownPrincipal, owner, metastoreID string, + usable []databricks.Workspace, annos *annotations.Annotations, ) (*databricks.Workspace, error) { - metastores, rateLimit, err := m.client.ListMetastores(ctx) - noteRateLimit(annos, rateLimit) - if err != nil { - return nil, fmt.Errorf("databricks-connector: failed to list metastores: %w", err) + if ownsPrincipal(ownPrincipal, owner) { + return &usable[0], nil } - found := slices.IndexFunc(metastores, func(metastore databricks.Metastore) bool { - return metastore.MetastoreID == metastoreID - }) - if found < 0 { - return nil, errMetastoreGone + for _, workspace := range usable { + admin, err := isWorkspaceAdmin(ctx, client, ownPrincipal, strconv.Itoa(workspace.ID), annos) + if err != nil { + return nil, err + } + if admin { + return &workspace, nil + } } - scoped, running, err := m.scopedWorkspaces(ctx, annos) - if err != nil { - return nil, err + names := make([]string, 0, len(usable)) + for _, workspace := range usable { + names = append(names, workspace.DeploymentName) } - workspace, pending, err := m.routeMetastore(ctx, metastoreID, metastores[found].Owner, scoped, running, annos) - if err != nil { - return nil, err - } - 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" - } + return nil, status.Errorf(codes.PermissionDenied, + "databricks-connector: cannot read every privilege on metastore %s through any workspace attached to it (%s), "+ + "and Databricks answers a partial read with 200, "+ + "so reporting it would revoke the access ConductorOne cannot see; "+ + "the connector's service principal must own the metastore or be assigned Admin directly on one of those workspaces", + metastoreID, strings.Join(names, ", ")) +} - return nil, status.Errorf(codes.FailedPrecondition, - "databricks-connector: cannot route metastore %s: %s", metastoreID, reason) +// metastoreGrantsAreTrustworthy is true when the connector owns the metastore +// or is a direct Admin of the routed workspace. MANAGE is not a metastore +// privilege. +func metastoreGrantsAreTrustworthy( + ctx context.Context, + client *databricks.Client, + trust securableTrust, + annos *annotations.Annotations, +) (bool, error) { + // An owner holds every privilege implicitly, which is not reported as a grant. + if ownsPrincipal(trust.ownPrincipal, trust.owner) { + return true, nil } - return workspace, nil + return isWorkspaceAdmin(ctx, client, trust.ownPrincipal, trust.workspaceID, annos) } // ownsPrincipal reports whether name is the service principal the connector @@ -356,9 +366,8 @@ func ownsPrincipal(ownPrincipal, name string) bool { return ownPrincipal != "" && strings.EqualFold(name, ownPrincipal) } -// isWorkspaceAdmin lists the workspace's permission assignments on every call. -// An assignment list the connector may not read leaves Admin unproven, which is -// the same answer as not holding it. +// isWorkspaceAdmin remembers the direct Admin check within one sync. +// Provisioning has no sync session and always checks the current assignments. func isWorkspaceAdmin( ctx context.Context, client *databricks.Client, @@ -369,6 +378,29 @@ func isWorkspaceAdmin( return false, nil } + store, syncing := ctx.Value(syncSessionKey{}).(sessions.SessionStore) + if !syncing { + return readWorkspaceAdmin(ctx, client, ownPrincipal, workspaceID, annos) + } + key := "unity-catalog/admin/" + workspaceID + admin, found, err := session.GetJSON[bool](ctx, store, key) + if err != nil { + return false, fmt.Errorf("databricks-connector: failed to read workspace Admin check for %s: %w", workspaceID, err) + } + if found { + return admin, nil + } + admin, err = readWorkspaceAdmin(ctx, client, ownPrincipal, workspaceID, annos) + if err != nil { + return false, err + } + if err := session.SetJSON(ctx, store, key, admin); err != nil { + return false, fmt.Errorf("databricks-connector: failed to store workspace Admin check for %s: %w", workspaceID, err) + } + return admin, nil +} + +func readWorkspaceAdmin(ctx context.Context, client *databricks.Client, ownPrincipal, workspaceID string, annos *annotations.Annotations) (bool, error) { assignments, rateLimit, err := client.ListWorkspaceMembers(ctx, workspaceID) noteRateLimit(annos, rateLimit) if err != nil { @@ -435,6 +467,74 @@ func ownPrincipalScimID( return id, nil } +// assignmentListError explains a failed read of the workspaces a metastore is +// assigned to, which decides which workspaces its securables are read through. +// A 404 is not an empty assignment list: the SDK would skip the child List and +// ConductorOne would delete what it contained. +func assignmentListError(metastoreID string, err error) error { + if errors.Is(err, errMetastoreGone) { + return uhttp.WrapErrors(codes.FailedPrecondition, + fmt.Sprintf("databricks-connector: metastore %s disappeared while reading its workspace assignments", metastoreID), err) + } + + return refuseUnreadable( + fmt.Sprintf("databricks-connector: failed to list the workspaces assigned to metastore %s", metastoreID), err) +} + +// workspaceForCatalog returns the first in-scope workspace whose catalog listing +// contains catalogName. None listing it is an error, never an absence: a catalog +// bound to a workspace outside the sync, or one the connector lost USE_CATALOG +// on, is not listed either and still exists. Reporting it as gone would delete +// its subtree on a sync, and confirm a revoke that never happened. +func workspaceForCatalog( + ctx context.Context, + client *databricks.Client, + metastoreID, catalogName string, + usable []databricks.Workspace, + probe catalogProbe, + annos *annotations.Annotations, +) (*databricks.Workspace, *databricks.Catalog, error) { + lastDeny := probe.deny + allGone := len(usable) > 0 + // A workspace already read through answers for itself. Only "gone" leaves + // "gone everywhere" standing; a refusal, or a catalog this metastore does not + // own, leaves the question open however the walk ends. + if probe.workspace != "" && !probe.gone { + allGone = false + } + for _, workspace := range usable { + if workspace.DeploymentName == probe.workspace { + continue + } + catalog, err := catalogThrough(ctx, client, workspace.DeploymentName, metastoreID, catalogName, annos) + if err != nil { + if errors.Is(err, errCatalogGone) { + continue + } + allGone = false + if deniesTrust(err) { + lastDeny = err + continue + } + return nil, nil, err + } + if catalog != nil { + return &workspace, catalog, nil + } + allGone = false + } + if allGone { + return nil, nil, errCatalogGone + } + + msg := fmt.Sprintf("databricks-connector: no workspace in sync scope lists catalog %s of metastore %s, so whether it still exists is unknown", + catalogName, metastoreID) + if lastDeny != nil { + msg = fmt.Sprintf("%s: %s", msg, lastDeny.Error()) + } + return nil, nil, status.Errorf(codes.FailedPrecondition, "%s", msg) +} + // deniesTrust is an answer that withholds what the connector asked for, as // opposed to one that says the account plane is unavailable. func deniesTrust(err error) bool { @@ -446,20 +546,6 @@ func deniesTrust(err error) bool { } } -func untrustedMetastoreError(metastoreID string, tried []databricks.Workspace) error { - names := make([]string, 0, len(tried)) - for _, workspace := range tried { - names = append(names, workspace.DeploymentName) - } - - return status.Errorf(codes.PermissionDenied, - "databricks-connector: cannot read every privilege on metastore %s through any workspace attached to it (%s), "+ - "and Databricks answers a partial read with 200, "+ - "so reporting it would revoke the access ConductorOne cannot see; "+ - "the connector's service principal must own the metastore or be assigned Admin directly on one of those workspaces", - metastoreID, strings.Join(names, ", ")) -} - // principalResolver maps a privilege assignment to a C1 resource id. The id is used when present; the name is only the fallback. type principalResolver struct { client *databricks.Client diff --git a/pkg/connector/metastore_test.go b/pkg/connector/metastore_test.go index 0db0c170..7f4ee32b 100644 --- a/pkg/connector/metastore_test.go +++ b/pkg/connector/metastore_test.go @@ -12,6 +12,7 @@ import ( "testing" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/types/grant" rs "github.com/conductorone/baton-sdk/pkg/types/resource" "google.golang.org/grpc/codes" @@ -57,24 +58,26 @@ func newWorkspaceRoutedClient(t *testing.T, handler http.HandlerFunc) (*databric // fakeAccount serves the account, Unity Catalog and SCIM endpoints the metastore syncer calls. type fakeAccount struct { - metastores []map[string]any - assignments map[string]any // metastore id -> []int workspace ids, or an HTTP status - workspaces []map[string]any - permissions []map[string]any - permStatus int - patchStatus int - users map[string]string // id -> userName - groups map[string]string // id -> displayName - sps map[string]string // id -> applicationId + metastores []map[string]any + assignments map[string]any // metastore id -> []int workspace ids, or an HTTP status + assignmentErrorCode map[string]string + workspaces []map[string]any + permissions []map[string]any + permStatus int + patchStatus int + users map[string]string // id -> userName + groups map[string]string // id -> displayName + sps map[string]string // id -> applicationId // workspaceMembers maps a workspace id to its permission assignments, or to // the HTTP status the account API answers the list with. workspaceMembers map[string]any - permHosts []string - patches []map[string]any - scimGets map[string]int - memberLists []string + permHosts []string + patches []map[string]any + scimGets map[string]int + memberLists []string + metastoreLists int } func defaultFakeAccount() *fakeAccount { @@ -111,12 +114,14 @@ func (f *fakeAccount) handler(t *testing.T) http.HandlerFunc { encode := func(v any) { _ = json.NewEncoder(w).Encode(v) } switch { case strings.HasSuffix(path, "/metastores"): + f.metastoreLists++ encode(map[string]any{"metastores": f.metastores}) case strings.Contains(path, "/metastores/") && strings.HasSuffix(path, "/workspaces"): parts := strings.Split(path, "/") switch v := f.assignments[parts[len(parts)-2]].(type) { case int: - http.Error(w, `{"message":"denied"}`, v) + w.WriteHeader(v) + _ = json.NewEncoder(w).Encode(map[string]any{"error_code": f.assignmentErrorCode[parts[len(parts)-2]], "message": "denied"}) default: encode(map[string]any{"workspace_ids": v}) } @@ -165,12 +170,20 @@ func (f *fakeAccount) handler(t *testing.T) http.HandlerFunc { } func (f *fakeAccount) scim(w http.ResponseWriter, r *http.Request) { + serveSCIM(w, r, f.users, f.groups, f.sps, f.scimGets) +} + +// serveSCIM answers account SCIM by id (/Users/{id}) and by filter (id or name eq). +// gets, when given, counts the reads by id. +func serveSCIM(w http.ResponseWriter, r *http.Request, users, groups, sps map[string]string, gets map[string]int) { _, rest, _ := strings.Cut(r.URL.Path, "/scim/v2/") kind, id, _ := strings.Cut(rest, "/") 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": users, "Groups": groups, "ServicePrincipals": sps}[kind] if id != "" { - f.scimGets[kind+"/"+id]++ + if gets != nil { + gets[kind+"/"+id]++ + } if name, ok := records[id]; ok { _ = json.NewEncoder(w).Encode(map[string]any{"id": id, field: name}) return @@ -246,6 +259,161 @@ func testSyncedMetastore(t *testing.T, workspace string) *v2.Resource { return r } +// A profile workspace is kept when it still serves, with ownership refreshed +// from the account. A detached workspace falls through to the live route. +func TestMetastoreGrantKeepsAProfileWorkspaceThatStillServes(t *testing.T) { + f := defaultFakeAccount() + m := newTestMetastoreBuilderAs(t, f, nil, syncPrincipals{}, "eng") + resource, err := metastoreResource(context.Background(), &databricks.Metastore{ + MetastoreID: "ms-a", Name: "main", Owner: "eng", + }, testAccountResourceID, "wsa", "1", "prod") + if err != nil { + t.Fatalf("metastoreResource: %v", err) + } + entitlement := grant.NewGrant(resource, "CREATE_CATALOG", alice.GetId()).GetEntitlement() + + routed, err := m.entitlementOnLiveRoute(context.Background(), entitlement, &annotations.Annotations{}) + if err != nil { + t.Fatalf("entitlementOnLiveRoute: %v", err) + } + if f.metastoreLists != 1 { + t.Errorf("listed metastores %d times, want one fresh owner read", f.metastoreLists) + } + if routed != entitlement { + t.Error("a profile workspace that still serves was replaced") + } +} + +func TestMetastoreGrantReresolvesAProfileWorkspaceThatIsGone(t *testing.T) { + f := defaultFakeAccount() + m := newTestMetastoreBuilderAs(t, f, nil, syncPrincipals{}, "eng") + resource, err := metastoreResource(context.Background(), &databricks.Metastore{ + MetastoreID: "ms-a", Name: "main", Owner: "eng", + }, testAccountResourceID, "gone", "9", "old") + if err != nil { + t.Fatalf("metastoreResource: %v", err) + } + entitlement := grant.NewGrant(resource, "CREATE_CATALOG", alice.GetId()).GetEntitlement() + + routed, err := m.entitlementOnLiveRoute(context.Background(), entitlement, &annotations.Annotations{}) + if err != nil { + t.Fatalf("entitlementOnLiveRoute: %v", err) + } + if f.metastoreLists == 0 { + t.Error("a profile workspace that is not attached did not resolve the route live") + } + workspace, _ := rs.GetProfileStringValue(rs.GetProfile(routed.GetResource()), profileKeyWorkspace) + if workspace != "wsa" { + t.Errorf("workspace = %q, want wsa", workspace) + } +} + +// The profile workspace was picked under the owner the last sync saw. When the +// metastore is transferred, the connector may no longer read every privilege +// through it while another attached workspace still works, so the route is +// picked again instead of reused. Reusing it refused the write with +// PermissionDenied even though the documented rule was satisfied elsewhere. +func TestMetastoreGrantRepicksTheRouteWhenTheOwnerChanged(t *testing.T) { + f := defaultFakeAccount() + f.metastores = []map[string]any{{"metastore_id": "ms-a", "name": "main", "owner": "someone-else"}} + f.assignments = map[string]any{"ms-a": []int{1, 2}} + f.workspaces = []map[string]any{ + {"workspace_id": 1, "workspace_name": "prod", "deployment_name": "wsa", "workspace_status": "RUNNING"}, + {"workspace_id": 2, "workspace_name": "dev", "deployment_name": "wsb", "workspace_status": "RUNNING"}, + } + // Admin on wsb only, which is not the workspace the profile recorded. + f.workspaceMembers = map[string]any{"1": nil, "2": adminByAppID} + m := newTestMetastoreBuilderAs(t, f, nil, syncPrincipals{}, "app-9") + + resource, err := metastoreResource(context.Background(), &databricks.Metastore{ + MetastoreID: "ms-a", Name: "main", Owner: "app-9", + }, testAccountResourceID, "wsa", "1", "prod") + if err != nil { + t.Fatalf("metastoreResource: %v", err) + } + entitlement := grant.NewGrant(resource, "CREATE_CATALOG", alice.GetId()).GetEntitlement() + + routed, err := m.entitlementOnLiveRoute(context.Background(), entitlement, &annotations.Annotations{}) + if err != nil { + t.Fatalf("entitlementOnLiveRoute: %v", err) + } + workspace, _ := rs.GetProfileStringValue(rs.GetProfile(routed.GetResource()), profileKeyWorkspace) + if workspace != "wsb" { + t.Errorf("workspace = %q, want wsb, the one the connector is still Admin of", workspace) + } +} + +// A profile workspace the connector still reads under the live owner is kept, +// so a transfer that does not move what the connector can read costs no extra +// walk. Only the owner on the profile is refreshed. +func TestMetastoreGrantKeepsTheRouteWhenItStillReadsUnderTheNewOwner(t *testing.T) { + f := defaultFakeAccount() + f.metastores = []map[string]any{{"metastore_id": "ms-a", "name": "main", "owner": "someone-else"}} + f.assignments = map[string]any{"ms-a": []int{1, 2}} + f.workspaces = []map[string]any{ + {"workspace_id": 1, "workspace_name": "prod", "deployment_name": "wsa", "workspace_status": "RUNNING"}, + {"workspace_id": 2, "workspace_name": "dev", "deployment_name": "wsb", "workspace_status": "RUNNING"}, + } + f.workspaceMembers = map[string]any{"1": adminByAppID, "2": nil} + m := newTestMetastoreBuilderAs(t, f, nil, syncPrincipals{}, "app-9") + + resource, err := metastoreResource(context.Background(), &databricks.Metastore{ + MetastoreID: "ms-a", Name: "main", Owner: "app-9", + }, testAccountResourceID, "wsa", "1", "prod") + if err != nil { + t.Fatalf("metastoreResource: %v", err) + } + entitlement := grant.NewGrant(resource, "CREATE_CATALOG", alice.GetId()).GetEntitlement() + + routed, err := m.entitlementOnLiveRoute(context.Background(), entitlement, &annotations.Annotations{}) + if err != nil { + t.Fatalf("entitlementOnLiveRoute: %v", err) + } + workspace, _ := rs.GetProfileStringValue(rs.GetProfile(routed.GetResource()), profileKeyWorkspace) + if workspace != "wsa" { + t.Errorf("workspace = %q, want wsa kept", workspace) + } + if owner, _ := rs.GetProfileStringValue(rs.GetProfile(routed.GetResource()), profileKeyOwner); owner != "someone-else" { + t.Errorf("owner on the profile = %q, want the live someone-else", owner) + } +} + +// The owner deciding the route can stay the same while the standing that chose +// the workspace is withdrawn: a direct Admin assignment removed from the recorded +// workspace but kept on another attached one. Routing there anyway would fail the +// call with PermissionDenied, which ConductorOne reports as a failed grant. +func TestMetastoreGrantRepicksTheRouteWhenOnlyTheAdminWasRemoved(t *testing.T) { + f := defaultFakeAccount() + f.metastores = []map[string]any{{"metastore_id": "ms-a", "name": "main", "owner": "someone-else"}} + f.assignments = map[string]any{"ms-a": []int{1, 2}} + f.workspaces = []map[string]any{ + {"workspace_id": 1, "workspace_name": "prod", "deployment_name": "wsa", "workspace_status": "RUNNING"}, + {"workspace_id": 2, "workspace_name": "dev", "deployment_name": "wsb", "workspace_status": "RUNNING"}, + } + // Admin on wsb only: the assignment that picked wsa is gone. + f.workspaceMembers = map[string]any{"1": nil, "2": adminByAppID} + m := newTestMetastoreBuilderAs(t, f, nil, syncPrincipals{}, "app-9") + + // The profile carries the same owner the account reports, so nothing about + // the owner signals that the route has to be re-read. + resource, err := metastoreResource(context.Background(), &databricks.Metastore{ + MetastoreID: "ms-a", Name: "main", Owner: "someone-else", + }, testAccountResourceID, "wsa", "1", "prod") + if err != nil { + t.Fatalf("metastoreResource: %v", err) + } + entitlement := grant.NewGrant(resource, "CREATE_CATALOG", alice.GetId()).GetEntitlement() + + routed, err := m.entitlementOnLiveRoute(context.Background(), entitlement, &annotations.Annotations{}) + if err != nil { + t.Fatalf("entitlementOnLiveRoute: %v", err) + } + workspace, _ := rs.GetProfileStringValue(rs.GetProfile(routed.GetResource()), profileKeyWorkspace) + if workspace != "wsb" { + t.Errorf("workspace = %q, want wsb: wsa no longer reads even though the owner did not change", workspace) + } +} + // testMetastore is the resource a provisioning call carries: an id, no profile. func testMetastore(t *testing.T) *v2.Resource { t.Helper() @@ -274,7 +442,7 @@ func TestMetastoreListFailsWhenAnAssignmentListCannotBeRead(t *testing.T) { f.assignments = map[string]any{"ms-a": code} m := newTestMetastoreBuilder(t, f, nil, syncPrincipals{}) - resources, _, err := m.List(context.Background(), testAccountResourceID, rs.SyncOpAttrs{}) + resources, _, err := m.List(context.Background(), testAccountResourceID, newTestSyncAttrs(t)) if status.Code(err) != want { t.Fatalf("List = %v, want %s", err, want) } @@ -313,7 +481,7 @@ func TestMetastoreListSeparatesTerminalFromPendingWorkspaces(t *testing.T) { } m := newTestMetastoreBuilder(t, f, nil, syncPrincipals{}) - resources, _, err := m.List(context.Background(), testAccountResourceID, rs.SyncOpAttrs{}) + resources, _, err := m.List(context.Background(), testAccountResourceID, newTestSyncAttrs(t)) if test.wantErr != (err != nil) { t.Fatalf("List error = %v, want error: %v", err, test.wantErr) } @@ -350,7 +518,7 @@ func TestMetastoreListRoutesTheOwnerThroughTheFirstScopedWorkspace(t *testing.T) } m := newTestMetastoreBuilder(t, f, test.allowlist, syncPrincipals{}) - resources, _, err := m.List(context.Background(), testAccountResourceID, rs.SyncOpAttrs{}) + resources, _, err := m.List(context.Background(), testAccountResourceID, newTestSyncAttrs(t)) if err != nil { t.Fatalf("List: %v", err) } @@ -416,7 +584,7 @@ func TestMetastoreListRoutesOnlyThroughAWorkspaceItCanFullyRead(t *testing.T) { f.workspaceMembers = map[string]any{"1": test.members} m := newTestMetastoreBuilderAs(t, f, nil, syncPrincipals{}, test.clientID) - resources, _, err := m.List(context.Background(), testAccountResourceID, rs.SyncOpAttrs{}) + resources, _, err := m.List(context.Background(), testAccountResourceID, newTestSyncAttrs(t)) switch { case test.wantRouted && err != nil: t.Fatalf("List: %v", err) @@ -452,7 +620,7 @@ func TestMetastoreListRoutesThroughTheWorkspaceWhereStandingHolds(t *testing.T) f.workspaceMembers = map[string]any{"1": nil, "2": adminByAppID} m := newTestMetastoreBuilderAs(t, f, nil, syncPrincipals{}, "app-9") - resources, _, err := m.List(context.Background(), testAccountResourceID, rs.SyncOpAttrs{}) + resources, _, err := m.List(context.Background(), testAccountResourceID, newTestSyncAttrs(t)) if err != nil { t.Fatalf("List: %v", err) } @@ -476,7 +644,7 @@ func TestMetastoreGrantsReadThroughTheRoutedWorkspace(t *testing.T) { } m := newTestMetastoreBuilderAs(t, f, nil, syncPrincipals{}, "app-9") - grants, _, err := m.Grants(context.Background(), testSyncedMetastore(t, "wsb"), rs.SyncOpAttrs{}) + grants, _, err := m.Grants(context.Background(), testSyncedMetastore(t, "wsb"), newTestSyncAttrs(t)) if err != nil { t.Fatalf("Grants: %v", err) } @@ -509,7 +677,7 @@ func TestMetastoreGrantsKeepThePermissionsStatus(t *testing.T) { f.permStatus = test.code m := newTestMetastoreBuilder(t, f, nil, syncPrincipals{}) - grants, _, err := m.Grants(context.Background(), testSyncedMetastore(t, "wsa"), rs.SyncOpAttrs{}) + grants, _, err := m.Grants(context.Background(), testSyncedMetastore(t, "wsa"), newTestSyncAttrs(t)) if status.Code(err) != test.want { t.Fatalf("Grants = %v, want %s", err, test.want) } @@ -525,7 +693,7 @@ func TestMetastoreGrantsFailWithoutARouteOnTheProfile(t *testing.T) { f := defaultFakeAccount() m := newTestMetastoreBuilder(t, f, nil, syncPrincipals{}) - grants, _, err := m.Grants(context.Background(), testMetastore(t), rs.SyncOpAttrs{}) + grants, _, err := m.Grants(context.Background(), testMetastore(t), newTestSyncAttrs(t)) if status.Code(err) != codes.FailedPrecondition { t.Fatalf("Grants = %v, want FailedPrecondition", err) } @@ -647,6 +815,35 @@ func TestMetastoreRevoke(t *testing.T) { } } +// A metastore deleted after the sync is proven gone by its assignment list, +// which the profile workspace check reads first. +func TestMetastoreProvisioningOnADeletedMetastoreWithAProfile(t *testing.T) { + for _, verb := range []string{"grant", "revoke"} { + t.Run(verb, func(t *testing.T) { + f := defaultFakeAccount() + f.assignments["ms-a"] = http.StatusNotFound + f.assignmentErrorCode = map[string]string{"ms-a": errorCodeMetastoreGone} + m := newTestMetastoreBuilder(t, f, nil, syncPrincipals{}) + g := grant.NewGrant(testSyncedMetastore(t, "wsa"), "CREATE_CATALOG", alice.GetId()) + + if verb == "grant" { + _, err := m.Grant(context.Background(), alice, g.GetEntitlement()) + if status.Code(err) != codes.NotFound { + t.Fatalf("Grant error = %v, want NotFound", err) + } + } else { + annos, err := m.Revoke(context.Background(), g) + if err != nil || !annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Fatalf("Revoke = %v, %v; want GrantAlreadyRevoked", annos, err) + } + } + if len(f.patches) != 0 { + t.Errorf("patches = %v, want none", f.patches) + } + }) + } +} + // A row the pre-read missed may be a partial read, so the removal still goes out. func TestMetastoreRevokeWritesWhenThePreReadMissesTheRow(t *testing.T) { f := defaultFakeAccount() diff --git a/pkg/connector/raw_id_test.go b/pkg/connector/raw_id_test.go index 2809a80b..536e10f0 100644 --- a/pkg/connector/raw_id_test.go +++ b/pkg/connector/raw_id_test.go @@ -70,6 +70,13 @@ func TestResourcesHaveRawId(t *testing.T) { build(metastoreResource(ctx, &databricks.Metastore{MetastoreID: "ms1", Name: "ms"}, account, "", "", "")), "ms1", }, + { + // Keyed by metastore: catalog names repeat across metastores. + "catalog", + build(catalogResource(&databricks.Catalog{Name: "cat"}, "ms1", + &v2.ResourceId{ResourceType: metastoreResourceType.Id, Resource: "ms1"}, catalogRoute{})), + "ms1::cat", + }, } for _, tc := range cases { diff --git a/pkg/connector/resource_types.go b/pkg/connector/resource_types.go index 2e07ce95..22c94445 100644 --- a/pkg/connector/resource_types.go +++ b/pkg/connector/resource_types.go @@ -16,6 +16,7 @@ const ( const ( permissionAccountAdmin = "Account admin" permissionWorkspaceAdmin = "Workspace admin" + permissionUseCatalog = "USE_CATALOG" ) var securableScopes = []string{ @@ -85,4 +86,11 @@ var ( Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_APP}, Annotations: annotationsForStaticSecurableResourceType(securablePermissions()...), } + + catalogResourceType = &v2.ResourceType{ + Id: "catalog", + DisplayName: "Catalog", + Traits: []v2.ResourceType_Trait{v2.ResourceType_TRAIT_APP}, + Annotations: annotationsForStaticSecurableResourceType(securablePermissions(permissionUseCatalog)...), + } ) diff --git a/pkg/connector/securable_helpers.go b/pkg/connector/securable_helpers.go new file mode 100644 index 00000000..9e4e1bad --- /dev/null +++ b/pkg/connector/securable_helpers.go @@ -0,0 +1,441 @@ +package connector + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "slices" + "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" + "github.com/conductorone/baton-sdk/pkg/types/grant" + rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/conductorone/baton-sdk/pkg/uhttp" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// securableKeySeparator keys a securable by its metastore. It is not a dot: a +// securable's full name is already dot-separated. +const securableKeySeparator = "::" + +// securableLevel is the only thing the permissions endpoint needs to tell one +// Unity Catalog level from another: how a resource id addresses the securable, +// what it can be granted, and what a refusal tells the operator. +// securableOps is what the shared securable path needs: the client, the +// connector's own principal, which principal types the sync emits, and the +// level that decides the rules. Each builder embeds one, so the three shared +// operations read as methods on the builder rather than as calls taking the +// same four arguments every time. +type securableOps struct { + client *databricks.Client + clientID string + sync syncPrincipals + level securableLevel +} + +type securableLevel struct { + // securableType is the endpoint path segment, lowercase. It is not the + // uppercase securable_type a listed securable carries. + securableType string + + // noun names the level in operator-facing messages. + noun string + + privileges []string + + // writeRemedy is what a 403 on the PATCH tells the operator. + writeRemedy string + + // trustRemedy is what a refusal to report a privilege list the connector + // cannot fully read tells the operator. + trustRemedy string + + // permissionsName turns a resource id into the name the endpoint addresses + // the securable by. + permissionsName func(resourceID string) (string, error) + + // goneErrorCodes are the error_code values Databricks returns when the + // securable, or a parent of it, no longer exists. They are the only proof + // of absence: an unbound or unprivileged workspace answers 404 too. + goneErrorCodes []string + + // trusts reports whether the connector reads every privilege assignment on + // the securable. What proves that differs per level, so the rule travels + // with the level rather than being decided by the shared path. + trusts func(ctx context.Context, client *databricks.Client, trust securableTrust, annos *annotations.Annotations) (bool, error) +} + +// securableTrust is what the per-level rules read to settle a privilege list: +// who the connector authenticates as, who owns the securable and the metastore +// above it, and where the read is addressed. +type securableTrust struct { + // ownPrincipal is the applicationId of the connector's service principal. + ownPrincipal string + + owner string + + // metastoreOwner is empty on a metastore, whose own owner is in owner. + metastoreOwner string + + fullName string + workspace string + workspaceID string +} + +// ensureGrantsAreTrustworthy refuses the call unless the privilege list is +// complete. Databricks returns HTTP 200 with a partial list, and reporting it +// would make ConductorOne revoke the rest. The check is not cached. +func ensureGrantsAreTrustworthy( + ctx context.Context, + client *databricks.Client, + clientID string, + level securableLevel, + resource *v2.Resource, + fullName, workspace string, + annos *annotations.Annotations, +) error { + profile := rs.GetProfile(resource) + owner, _ := rs.GetProfileStringValue(profile, profileKeyOwner) + metastoreOwner, _ := rs.GetProfileStringValue(profile, profileKeyMetastoreOwner) + workspaceID, _ := rs.GetProfileStringValue(profile, profileKeyWorkspaceID) + + trustworthy, err := level.trusts(ctx, client, securableTrust{ + ownPrincipal: clientID, + owner: owner, + metastoreOwner: metastoreOwner, + fullName: fullName, + workspace: workspace, + workspaceID: workspaceID, + }, annos) + if err != nil { + return err + } + if !trustworthy { + return status.Errorf(codes.PermissionDenied, + "databricks-connector: cannot read every privilege on %s %s through workspace %s; %s", + level.noun, fullName, workspace, level.trustRemedy) + } + + return nil +} + +// syncSecurableGrants emits the privileges held on one securable, plus its owner. +// The workspace comes off the profile the last List wrote: every Unity Catalog +// read is addressed to a workspace host, and resolving one here would be an +// account-plane round trip per securable. +func (o securableOps) syncGrants( + ctx context.Context, + resource *v2.Resource, + attr rs.SyncOpAttrs, +) ([]*v2.Grant, *rs.SyncOpResults, error) { + client, clientID, sync, level := o.client, o.clientID, o.sync, o.level + l := ctxzap.Extract(ctx) + resourceID := resource.GetId().GetResource() + fullName, err := level.permissionsName(resourceID) + if err != nil { + return nil, nil, err + } + + profile := rs.GetProfile(resource) + workspace, _ := rs.GetProfileStringValue(profile, profileKeyWorkspace) + if workspace == "" { + return nil, nil, status.Errorf(codes.FailedPrecondition, + "databricks-connector: %s %s has no workspace on its profile", level.noun, fullName) + } + + annos := annotations.Annotations{} + pageToken := attr.PageToken.Token + // List already refused a catalog or a metastore whose privilege list is + // partial, so Grants does not prove it again. + if pageToken == "" && level.securableType != databricks.SecurableMetastore && level.securableType != databricks.SecurableCatalog { + if err := ensureGrantsAreTrustworthy(ctx, client, clientID, level, resource, fullName, workspace, &annos); err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, err + } + } + + assignments, nextPageToken, rateLimit, err := client.ListPermissions(ctx, workspace, level.securableType, fullName, "", pageToken) + noteRateLimit(&annos, rateLimit) + if err != nil { + // A securable deleted between List and this read is not a failed sync. + // Only a level that names its gone codes can reach this: with none, + // IsSecurableGone is false and every 404 still refuses the read. + if databricks.IsSecurableGone(err, level.goneErrorCodes...) { + return nil, &rs.SyncOpResults{Annotations: annos}, nil + } + return nil, &rs.SyncOpResults{Annotations: annos}, refuseUnreadable(fmt.Sprintf( + "databricks-connector: failed to list permissions on %s %s through workspace %s", level.noun, fullName, workspace), err) + } + + resolver := newPrincipalResolver(client, &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) + } + if principalId == nil { + l.Debug("databricks-connector: skipping a grant whose principal does not resolve", + zap.String("securable", resourceID), + zap.String("principal", assignment.Principal), + zap.String("principal_id", assignment.PrincipalID.String())) + + continue + } + if !emitPrincipal(sync, principalId.GetResourceType()) { + continue + } + + options := groupSecurableGrantOptions(principalId) + for _, privilege := range assignment.Privileges { + if !slices.Contains(level.privileges, privilege) { + // A privilege Databricks added after this connector shipped is not + // an entitlement, so it cannot be emitted. Say so rather than + // dropping it without a trace. + l.Debug("databricks-connector: the securable reports a privilege this connector does not model", + zap.String("securable", resourceID), + zap.String("privilege", privilege)) + + continue + } + rv = append(rv, grant.NewGrant(resource, privilege, principalId, options...)) + } + } + + // The owner is emitted once, with the first page. + if owner, _ := rs.GetProfileStringValue(profile, profileKeyOwner); pageToken == "" && owner != "" { + principalId, err := resolver.resolve(ctx, "", owner) + if err != nil { + return nil, &rs.SyncOpResults{Annotations: annos}, fmt.Errorf("databricks-connector: %w", err) + } + if principalId != nil && emitPrincipal(sync, principalId.GetResourceType()) { + options := append(groupSecurableGrantOptions(principalId), grant.WithAnnotation(&v2.GrantImmutable{})) + rv = append(rv, grant.NewGrant(resource, ownerEntitlement, principalId, options...)) + } + } + + if nextPageToken != "" && nextPageToken == pageToken { + return nil, &rs.SyncOpResults{Annotations: annos}, status.Errorf(codes.Internal, + "databricks-connector: listing privileges on %s %s repeated page token %q", level.noun, fullName, nextPageToken) + } + + return rv, &rs.SyncOpResults{Annotations: annos, NextPageToken: nextPageToken}, nil +} + +// change is one validated Grant or Revoke, resolved down to what the PATCH needs. +type change struct { + level securableLevel + fullName string + workspace string + privilege string + scimID string + principalName string +} + +// resolveChange validates a Grant/Revoke and resolves what the PATCH needs. +func resolveChange( + ctx context.Context, + client *databricks.Client, + clientID string, + level securableLevel, + principalID *v2.ResourceId, + entitlement *v2.Entitlement, + isGrant bool, + annos *annotations.Annotations, +) (change, error) { + if !isValidPrincipal(principalID) { + return change{}, status.Errorf(codes.InvalidArgument, + "databricks-connector: only users, groups and service principals can hold a %s privilege, got %s", + level.noun, principalID.GetResourceType()) + } + + resource := entitlement.GetResource() + fullName, err := level.permissionsName(resource.GetId().GetResource()) + if err != nil { + return change{}, err + } + + privilege, err := parseEntitlementPrivilege(entitlement.GetId()) + if err != nil { + return change{}, err + } + switch { + case privilege == ownerEntitlement: + return change{}, status.Errorf(codes.InvalidArgument, + "databricks-connector: %s ownership cannot be provisioned; change the owner in Databricks", level.noun) + case slices.Contains(legacyPrivileges, strings.ToUpper(strings.TrimSpace(privilege))): + return change{}, status.Errorf(codes.InvalidArgument, + "databricks-connector: %s is a legacy alias Databricks rewrites; use the explicit privilege", privilege) + case !slices.Contains(level.privileges, privilege): + return change{}, status.Errorf(codes.InvalidArgument, + "databricks-connector: %s is not a grantable %s privilege", privilege, level.noun) + } + + workspace, _ := rs.GetProfileStringValue(rs.GetProfile(resource), profileKeyWorkspace) + if workspace == "" { + return change{}, status.Errorf(codes.FailedPrecondition, + "databricks-connector: %s %s has no workspace on its profile", level.noun, fullName) + } + + // A privilege list the connector cannot fully read also cannot be safely + // written to: the PATCH would be built from the rows it is allowed to see. + if err := ensureGrantsAreTrustworthy(ctx, client, clientID, level, resource, fullName, workspace, annos); err != nil { + return change{}, err + } + + scimID, err := unityPrincipalScimID(principalID) + if err != nil { + return change{}, fmt.Errorf("databricks-connector: %w", err) + } + + principalName, err := unityPrincipalName(ctx, client, principalID) + if err != nil { + if !errors.Is(err, errPrincipalNotFound) { + return change{}, fmt.Errorf("databricks-connector: failed to resolve principal %s: %w", principalID.GetResource(), err) + } + // Revoke addresses a vanished principal's rows by id. + if isGrant { + return change{}, status.Errorf(codes.NotFound, "databricks-connector: principal %s no longer exists in Databricks", scimID) + } + } + + return change{ + level: level, + fullName: fullName, + workspace: workspace, + privilege: privilege, + scimID: scimID, + principalName: principalName, + }, nil +} + +func (c change) matchHeld(assignments []databricks.PrivilegeAssignment) (databricks.PrivilegeAssignment, bool) { + return matchHeldPrivilege(assignments, c.scimID, c.principalName, c.privilege) +} + +func (c change) updateError(verb string, err error) error { + if isForbiddenError(err) { + return uhttp.WrapErrors(codes.PermissionDenied, + fmt.Sprintf("databricks-connector: cannot %s %s on %s %s: %s", + verb, c.privilege, c.level.noun, c.fullName, c.level.writeRemedy), + err) + } + + return fmt.Errorf("databricks-connector: failed to %s %s on %s %s: %w", verb, c.privilege, c.level.noun, c.fullName, err) +} + +// grantSecurable adds one privilege. Databricks no-ops a PATCH it cannot apply, +// so the pre-read decides GrantAlreadyExists and the echo is the only proof the +// change landed. +func (o securableOps) grant( + ctx context.Context, + principal *v2.ResourceId, + entitlement *v2.Entitlement, +) (annotations.Annotations, error) { + client, clientID, level := o.client, o.clientID, o.level + annos := annotations.Annotations{} + change, err := resolveChange(ctx, client, clientID, level, principal, entitlement, true, &annos) + if err != nil { + return annos, err + } + + // Read past the cache: a stale page would skip a needed PATCH. + assignments, rateLimit, err := client.DrainPermissions(ctx, change.workspace, level.securableType, change.fullName) + noteRateLimit(&annos, rateLimit) + if err != nil { + return annos, fmt.Errorf("databricks-connector: failed to read privileges on %s %s: %w", level.noun, change.fullName, err) + } + if _, held := change.matchHeld(assignments); held { + annos.Update(&v2.GrantAlreadyExists{}) + return annos, nil + } + + var echoed []databricks.PrivilegeAssignment + rateLimit, err = client.UpdatePermissionsUntil(ctx, change.workspace, level.securableType, change.fullName, + []databricks.PermissionsChange{{Principal: change.principalName, Add: []string{change.privilege}}}, + func(page []databricks.PrivilegeAssignment) (bool, error) { + echoed = append(echoed, page...) + _, held := change.matchHeld(echoed) + return held, nil + }) + noteRateLimit(&annos, rateLimit) + if err != nil { + return annos, change.updateError("grant", err) + } + + if _, held := change.matchHeld(echoed); !held { + return annos, status.Errorf(codes.Internal, + "databricks-connector: granting %s on %s %s was accepted but not applied", change.privilege, level.noun, change.fullName) + } + + return annos, nil +} + +// revokeSecurable removes one privilege. Databricks no-ops a privilege already +// absent, so the pre-read decides GrantAlreadyRevoked and the echo confirms the +// removal. +func (o securableOps) revoke( + ctx context.Context, + principal *v2.ResourceId, + entitlement *v2.Entitlement, +) (annotations.Annotations, error) { + client, clientID, level := o.client, o.clientID, o.level + annos := annotations.Annotations{} + change, err := resolveChange(ctx, client, clientID, level, principal, entitlement, false, &annos) + if err != nil { + return annos, err + } + + var seen []databricks.PrivilegeAssignment + rateLimit, err := client.ForEachUncachedPermissionsPage(ctx, change.workspace, level.securableType, change.fullName, "", + func(page []databricks.PrivilegeAssignment) (bool, error) { + seen = append(seen, page...) + matched, ok := change.matchHeld(seen) + // Only an id match is final; a name match may be superseded on a later page. + return ok && change.scimID != "" && matched.PrincipalID.String() == change.scimID && change.scimID != "0", nil + }) + noteRateLimit(&annos, rateLimit) + if err != nil { + // Databricks names the missing securable in error_code. That is the only + // answer that proves it is gone, so there is nothing left to revoke: a + // bare 404 also comes back from a workspace the securable is not bound to. + if databricks.IsSecurableGone(err, level.goneErrorCodes...) { + annos.Update(&v2.GrantAlreadyRevoked{}) + + return annos, nil + } + + return annos, fmt.Errorf("databricks-connector: failed to read privileges on %s %s: %w", level.noun, change.fullName, err) + } + + // A missed row can be a partial read, and removing a privilege nobody holds is a no-op. + held, ok := change.matchHeld(seen) + removal := databricks.PermissionsChange{Principal: change.principalName, Remove: []string{change.privilege}} + switch heldID := held.PrincipalID.String(); { + case ok && heldID != "" && heldID != "0": + removal = databricks.PermissionsChange{PrincipalID: held.PrincipalID, Remove: []string{change.privilege}} + case change.principalName == "": + removal = databricks.PermissionsChange{PrincipalID: json.Number(change.scimID), Remove: []string{change.privilege}} + } + + result, rateLimit, err := client.UpdatePermissions(ctx, change.workspace, level.securableType, change.fullName, + []databricks.PermissionsChange{removal}) + noteRateLimit(&annos, rateLimit) + if err != nil { + return annos, change.updateError("revoke", err) + } + if _, stillHeld := change.matchHeld(result); stillHeld { + return annos, status.Errorf(codes.Internal, + "databricks-connector: revoking %s on %s %s was accepted but not applied", change.privilege, level.noun, change.fullName) + } + if !ok { + annos.Update(&v2.GrantAlreadyRevoked{}) + } + + return annos, nil +} diff --git a/pkg/databricks/client.go b/pkg/databricks/client.go index 2ff8722c..1466f64c 100644 --- a/pkg/databricks/client.go +++ b/pkg/databricks/client.go @@ -153,6 +153,27 @@ func (c *Client) IsWorkspaceNameExcluded(deploymentName string) bool { return ok } +// IsWorkspaceExcluded reports whether any identifier of one workspace matches the +// databricks-exclude-workspaces set. It is isWorkspaceExcluded for a caller that +// holds the identifiers without a Workspace value, such as a resource profile. An +// empty identifier is skipped rather than matched, so a missing one cannot widen +// the match the way a zero-value ID would. +func (c *Client) IsWorkspaceExcluded(name, deploymentName, id string) bool { + if len(c.excludeWorkspaces) == 0 { + return false + } + for _, key := range []string{name, deploymentName, id} { + if key == "" { + continue + } + if _, ok := c.excludeWorkspaces[strings.ToLower(key)]; ok { + return true + } + } + + return false +} + func (c *Client) workspaceUrl(workspaceId string) *url.URL { return &url.URL{ Scheme: "https", diff --git a/pkg/databricks/models.go b/pkg/databricks/models.go index 1d811724..fc8edf26 100644 --- a/pkg/databricks/models.go +++ b/pkg/databricks/models.go @@ -174,6 +174,14 @@ type Metastore struct { Owner string `json:"owner"` } +type Catalog struct { + Name string `json:"name"` + MetastoreID string `json:"metastore_id"` + Owner string `json:"owner"` + CatalogType string `json:"catalog_type"` + IsolationMode string `json:"isolation_mode"` +} + type PrivilegeAssignment struct { Principal string `json:"principal"` PrincipalID json.Number `json:"principal_id"` @@ -210,6 +218,11 @@ type metastoreWorkspacesResponse struct { WorkspaceIDs []json.Number `json:"workspace_ids"` } +type catalogsResponse struct { + Catalogs []Catalog `json:"catalogs"` + NextPageToken string `json:"next_page_token"` +} + type permissionsResponse struct { PrivilegeAssignments []PrivilegeAssignment `json:"privilege_assignments"` NextPageToken string `json:"next_page_token"` diff --git a/pkg/databricks/permissions_cache_test.go b/pkg/databricks/permissions_cache_test.go index a63302eb..0ec68598 100644 --- a/pkg/databricks/permissions_cache_test.go +++ b/pkg/databricks/permissions_cache_test.go @@ -51,7 +51,12 @@ func TestProvisioningPreReadsAreNotServedFromTheCache(t *testing.T) { ctx := context.Background() // Grant's pre-read: nothing is held yet, so the Grant proceeds to the PATCH. - held, _, err := c.DrainPermissions(ctx, "ws", SecurableMetastore, "ms-1") + var held []PrivilegeAssignment + _, err = c.ForEachUncachedPermissionsPage(ctx, "ws", SecurableMetastore, "ms-1", "", + func(page []PrivilegeAssignment) (bool, error) { + held = append(held, page...) + return false, nil + }) if err != nil { t.Fatalf("Grant pre-read: %v", err) } @@ -110,7 +115,8 @@ func TestASyncReadAfterAGrantSeesThePostChangeState(t *testing.T) { } ctx := context.Background() - if _, _, err := c.DrainPermissions(ctx, "ws", SecurableMetastore, "ms-1"); err != nil { + if _, err := c.ForEachUncachedPermissionsPage(ctx, "ws", SecurableMetastore, "ms-1", "", + func([]PrivilegeAssignment) (bool, error) { return false, nil }); err != nil { t.Fatalf("Grant pre-read: %v", err) } if _, _, err := c.UpdatePermissions(ctx, "ws", SecurableMetastore, "ms-1", diff --git a/pkg/databricks/request.go b/pkg/databricks/request.go index 77c6d073..d9924d26 100644 --- a/pkg/databricks/request.go +++ b/pkg/databricks/request.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/url" + "slices" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/uhttp" @@ -72,6 +73,25 @@ func (e *APIError) Unwrap() error { return e.Err } +// HasErrorCode reports a 404 whose body named one of the given error codes. +func (e *APIError) HasErrorCode(errorCodes ...string) bool { + if e == nil || e.StatusCode != http.StatusNotFound || e.ErrorCode == "" { + return false + } + + return slices.Contains(errorCodes, e.ErrorCode) +} + +// IsSecurableGone reports a 404 for a named securable that no longer exists. +// A 404 with any other error_code, or with none, is a routing or permission +// miss: Databricks answers an unbound workspace with an error too, so only the +// code proves the securable is gone. +func IsSecurableGone(err error, errorCodes ...string) bool { + var apiErr *APIError + + return errors.As(err, &apiErr) && apiErr.HasErrorCode(errorCodes...) +} + // nameWorkspace403Remedy enriches a 403 from a workspace-scoped call with the // fix: excluding the workspace scopes it out of the sync. workspaceId is empty // for account-scoped calls, where a 403 is not a per-workspace access problem, @@ -129,8 +149,9 @@ func (c *Client) Get( // The cache holds a GET 200 for an hour and a PATCH does not invalidate it, so a // read that decides whether a write still has to happen must not be served from // it: the write would be skipped against a page that predates the last change. -// uhttp still records the fresh response, so the entry a later reader sees is the -// one this call just observed. +// Cache-Control is part of the cache key, so the entry this call writes is not +// the one a normal read looks up: the shared sync entry is left as it was +// rather than refreshed. func (c *Client) GetUncached( ctx context.Context, urlAddress *url.URL, diff --git a/pkg/databricks/unity_catalog.go b/pkg/databricks/unity_catalog.go index 45a2acee..ea8703dc 100644 --- a/pkg/databricks/unity_catalog.go +++ b/pkg/databricks/unity_catalog.go @@ -20,6 +20,9 @@ const ( // https://docs.databricks.com/api/account/accountmetastoreassignments/list accountMetastoreWorkspacesEndpoint = "/api/2.0/accounts/%s/metastores/%s/workspaces" + // https://docs.databricks.com/api/workspace/catalogs/list + unityCatalogCatalogsEndpoint = "/api/2.1/unity-catalog/catalogs" + // GET https://docs.databricks.com/api/workspace/grants/get // PATCH https://docs.databricks.com/api/workspace/grants/update unityCatalogPermissionsEndpoint = "/api/2.1/unity-catalog/permissions" @@ -28,6 +31,7 @@ const ( // uppercase securable_type a listed securable carries. A metastore is // addressed by its UUID: its name is rejected. SecurableMetastore = "metastore" + SecurableCatalog = "catalog" // The permissions endpoint rejects any max_results from 1 to 149 with a 400. // Zero selects the paginated response that replaces the unpaginated form. @@ -82,6 +86,74 @@ func (c *Client) ListMetastoreWorkspaces( return ids, ratelimitData, nil } +// ListCatalogs (GET /api/2.1/unity-catalog/catalogs) reads one page of the catalogs +// of the metastore attached to workspaceId, which is a deployment name. A catalog +// bound to a subset of the workspaces is listed only by the ones it is bound to. +func (c *Client) ListCatalogs( + ctx context.Context, + workspaceId, pageToken string, + maxResults uint, + includeUnbound bool, +) ( + []Catalog, + string, + *v2.RateLimitDescription, + error, +) { + var res catalogsResponse + + u := c.workspaceUrl(workspaceId).JoinPath(unityCatalogCatalogsEndpoint) + ratelimitData, err := c.Get(ctx, u, &res, &unityCatalogVars{ + maxResults: &maxResults, + pageToken: pageToken, + includeUnbound: includeUnbound, + }) + if err != nil { + return nil, "", ratelimitData, nameWorkspace403Remedy(workspaceId, fmt.Errorf("failed to list catalogs: %w", err)) + } + + return res.Catalogs, res.NextPageToken, ratelimitData, nil +} + +// GetCatalog (GET /api/2.1/unity-catalog/catalogs/{name}) reads one catalog +// through workspaceId, which is a deployment name. It answers what draining +// ListCatalogs cannot: a 404 naming CATALOG_DOES_NOT_EXIST is the catalog not +// existing, while any other refusal is this workspace not seeing it. +// The name is checked against the assembled URL for the same reason +// permissionsUrl checks it: a name holding "?", "#" or "%" would otherwise +// address a different catalog, and a caller that writes to what comes back +// would write to that one. +func (c *Client) GetCatalog( + ctx context.Context, + workspaceId, name string, +) ( + *Catalog, + *v2.RateLimitDescription, + error, +) { + var res Catalog + + if name == "" { + return nil, nil, invalidArgument("catalog name is required") + } + + u := c.workspaceUrl(workspaceId).JoinPath(unityCatalogCatalogsEndpoint, name) + if !addressesSecurable(u, "/"+name) { + return nil, nil, invalidArgument("cannot address catalog %q: the name does not survive URL assembly", name) + } + + ratelimitData, err := c.Get(ctx, u, &res) + if err != nil { + return nil, ratelimitData, nameWorkspace403Remedy(workspaceId, fmt.Errorf("failed to read catalog %s: %w", name, err)) + } + + if res.Name != name { + return nil, ratelimitData, invalidArgument("asked Databricks for catalog %q and it answered with %q", name, res.Name) + } + + return &res, ratelimitData, nil +} + // ListPermissions (GET /api/2.1/unity-catalog/permissions/{securable_type}/{full_name}). // The principal query filters effectively, not literally: an inherited assignment // comes back naming the group it is inherited through, so a caller must read every diff --git a/pkg/databricks/unity_catalog_test.go b/pkg/databricks/unity_catalog_test.go index 3c5e66d8..856771a3 100644 --- a/pkg/databricks/unity_catalog_test.go +++ b/pkg/databricks/unity_catalog_test.go @@ -9,6 +9,8 @@ import ( "testing" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // Databricks ignores a page token it cannot deserialize and silently restarts at @@ -93,6 +95,91 @@ func TestUpdatePermissionsDrainsFromPatchToken(t *testing.T) { } } +func TestListCatalogsCanIncludeUnboundCatalogs(t *testing.T) { + t.Setenv("BATON_HTTP_CACHE_TTL", "0") + var includeUnbound string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + includeUnbound = r.URL.Query().Get("include_unbound") + _ = json.NewEncoder(w).Encode(catalogsResponse{}) + })) + t.Cleanup(srv.Close) + + client := srv.Client() + client.Transport = rewriteHost(srv.Listener.Addr().String(), client.Transport) + c, err := NewClient(context.Background(), client, "example.cloud.databricks.com", "accounts.cloud.databricks.com", "acc-1", srv.URL, &NoAuth{}, nil) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + if _, _, _, err := c.ListCatalogs(context.Background(), "ws", "", 100, true); err != nil { + t.Fatalf("ListCatalogs: %v", err) + } + if includeUnbound != "true" { + t.Fatalf("include_unbound = %q, want true", includeUnbound) + } +} + +// A catalog name holding "?", "#" or "%" does not survive URL assembly: the +// request layer unescapes the path again, so "sales?x" would read "sales". The +// caller writes privileges to whatever comes back, so the read is refused +// instead of answering with a different catalog. +func TestGetCatalogRefusesANameThatChangesTheURL(t *testing.T) { + t.Setenv("BATON_HTTP_CACHE_TTL", "0") + + var requested []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requested = append(requested, r.URL.Path) + _ = json.NewEncoder(w).Encode(Catalog{Name: "sales", MetastoreID: "ms-1"}) + })) + t.Cleanup(srv.Close) + + client := srv.Client() + client.Transport = rewriteHost(srv.Listener.Addr().String(), client.Transport) + c, err := NewClient(context.Background(), client, "example.cloud.databricks.com", "accounts.cloud.databricks.com", "acc-1", srv.URL, &NoAuth{}, nil) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + + for _, name := range []string{"sales?x", "sales#x", "sales%x", ""} { + got, _, err := c.GetCatalog(context.Background(), "ws", name) + if err == nil { + t.Fatalf("GetCatalog(%q) returned catalog %+v, want an error", name, got) + } + if status.Code(err) != codes.InvalidArgument { + t.Fatalf("GetCatalog(%q) returned %v, want InvalidArgument", name, status.Code(err)) + } + } + if len(requested) != 0 { + t.Fatalf("GetCatalog sent %v, want no request at all", requested) + } +} + +// Databricks answering with a catalog other than the one asked for is not an +// answer about the requested catalog, and Grant/Revoke rebuild the resource from +// what comes back. +func TestGetCatalogRefusesAMismatchedAnswer(t *testing.T) { + t.Setenv("BATON_HTTP_CACHE_TTL", "0") + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(Catalog{Name: "other", MetastoreID: "ms-1"}) + })) + t.Cleanup(srv.Close) + + client := srv.Client() + client.Transport = rewriteHost(srv.Listener.Addr().String(), client.Transport) + c, err := NewClient(context.Background(), client, "example.cloud.databricks.com", "accounts.cloud.databricks.com", "acc-1", srv.URL, &NoAuth{}, nil) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + + got, _, err := c.GetCatalog(context.Background(), "ws", "sales") + if err == nil { + t.Fatalf("GetCatalog returned catalog %+v, want an error", got) + } + if status.Code(err) != codes.InvalidArgument { + t.Fatalf("GetCatalog returned %v, want InvalidArgument", status.Code(err)) + } +} + type hostRewriter struct { host http.RoundTripper addr string diff --git a/pkg/databricks/vars.go b/pkg/databricks/vars.go index 9e9c7cfe..7525e62a 100644 --- a/pkg/databricks/vars.go +++ b/pkg/databricks/vars.go @@ -173,9 +173,10 @@ func NewNameVars(name string, etag string) *NameVars { // while an absent max_results asks for the deprecated unpaginated response, so // maxResults is a pointer: its presence is the signal. type unityCatalogVars struct { - principal string - maxResults *uint - pageToken string + principal string + maxResults *uint + pageToken string + includeUnbound bool } func (v *unityCatalogVars) Apply(params *url.Values) { @@ -190,4 +191,7 @@ func (v *unityCatalogVars) Apply(params *url.Values) { if v.pageToken != "" { params.Add("page_token", v.pageToken) } + if v.includeUnbound { + params.Set("include_unbound", "true") + } }