CXH-2166: implement PAT (workspace token) authentication - #54
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: CXH-2166: implement PAT (workspace token) authenticationBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit adds I also re-scanned the full PR diff for security and correctness: The four findings from the previous review are all still open in the current tree — Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Re-add the personal-access-token auth path removed in e84a1ae so the connector matches its docs. Workspace tokens authenticate per-workspace against the Databricks Workspace API and scope the sync to the workspaces those tokens belong to; OAuth stays the default. - config: restore workspaces + workspace-tokens fields and the OAuth/token constraints; OAuth client id/secret are no longer hard-required - auth: restore TokenAuth, selecting the token by workspace host prefix so Azure dotted deployment names match correctly - connector: token-aware Validate and prepareClientAuth; thread workspaces through to the workspace builder - workspace builder: build minimal workspace resources from the configured list when the Account API is unavailable (token auth)
Replace the OAuth/workspace-token field relationships with field groups so the config validation and C1 setup UI match each auth mode cleanly. Drop the two noisy info logs in prepareClientAuth to debug level, guard NewTokenAuth against a shorter tokens slice than workspaces, and add test coverage for both.
Field groups only validate the selected auth-method's fields, so setting both databricks-client-id and workspace-tokens together no longer failed validation the way the old field relationships did. Add the check back in ValidateConfig, which always runs regardless of which group is selected.
Reflects the additional baton-sdk flags picked up by rebasing onto main.
c69ed42 to
8b12e6a
Compare
…orkspace match, refresh generated schema Groups sync parented under the workspace when the account API is unavailable (token auth), but roleBuilder and servicePrincipalBuilder still built account-parented (or unparented) group grant principals, so those grants referenced resources that were never synced. Also add an aggregate warning when a configured --workspaces filter matches nothing, broaden the match to name/deployment-name/numeric-ID like the exclude-list matcher, and regenerate config_schema.json plus the README flag dump to match pkg/config/config.go.
| if w.client.IsTokenAuth() { | ||
| for workspace := range w.workspaces { | ||
| if w.client.IsWorkspaceExcluded(workspace) { | ||
| continue | ||
| } | ||
|
|
||
| ws := &databricks.Workspace{DeploymentName: workspace} | ||
|
|
||
| wr, err := minimalWorkspaceResource(ctx, ws, parentResourceID) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| rv = append(rv, wr) | ||
| } | ||
|
|
||
| return rv, nil, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the new "sync will be empty" warning below only covers the account-API path. Under token auth this branch returns early, so if every configured workspace is also listed in databricks-exclude-workspaces the connector returns zero workspaces (and therefore zero users/groups/SPs) with no log at all — the same silent empty sync the warning was added to catch. Consider warning here too when len(w.workspaces) > 0 && len(rv) == 0.
| func minimalWorkspaceResource(_ context.Context, workspace *databricks.Workspace, parent *v2.ResourceId) (*v2.Resource, error) { | ||
| return rs.NewGroupResource( | ||
| workspace.DeploymentName, | ||
| workspaceResourceType, | ||
| workspace.DeploymentName, | ||
| nil, | ||
| rs.WithParentResourceID(parent), | ||
| rs.WithAnnotation( | ||
| &v2.ChildResourceType{ResourceTypeId: userResourceType.Id}, | ||
| &v2.ChildResourceType{ResourceTypeId: groupResourceType.Id}, | ||
| &v2.ChildResourceType{ResourceTypeId: servicePrincipalResourceType.Id}, | ||
| &v2.ChildResourceType{ResourceTypeId: roleResourceType.Id}, | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): this commit scopes group IDs to the workspace (workspace/<deployment>/group/<gid>), but users and service principals synced under these workspace parents still use bare workspace-local SCIM IDs (users.go:72, service-principals.go:54) with no parent set. With multiple workspaces under token auth, the same person yields two distinct user resources when the SCIM IDs differ, and — since workspace-local SCIM IDs are only unique per workspace when identity federation is off — two different people can collapse into one resource if the IDs collide. Worth either scoping these IDs the same way groups are, or documenting that token auth assumes federated (account-consistent) SCIM IDs.
…ants Warn when every configured workspace is excluded under token auth (previously silent), stop IsWorkspaceExcluded from false-matching on a synthetic zero-value ID, trust an explicit auth-method selection in the OAuth/workspace-token mutual-exclusion check instead of rejecting leftover fields from the unselected group, fix marketplace-admin group grants to parent under the account the same way groups actually sync, and distinguish an intentionally excluded workspace from one that truly wasn't found in the account-API debug log.
…e sync Live testing against a real tenant under workspace-token auth hit a group present in the workspace SCIM listing that the rule-sets API doesn't recognize (Databricks had auto-generated an orphaned clone of a group). Since token auth can only sync groups scoped to a workspace, and that path had no prior handler for this response, one bad group reference aborted the entire sync instead of just that group's role data. Treat a "not found" 400 from the roles/rule-sets lookup as skip-and-continue, matching how the connector already treats other stale references.
| if !errors.As(err, &apiErr) { | ||
| return false | ||
| } | ||
| return apiErr.StatusCode == http.StatusBadRequest && strings.Contains(apiErr.Message, "not found") |
There was a problem hiding this comment.
🟡 Suggestion: this matches any HTTP 400 whose message happens to contain the lowercase substring not found, which is broader than "this group ID is unknown to the rule-sets API". A 400 about the workspace or the role payload (accounts/<acct>/groups/<id> is assembled from several parts) would match too, and both call sites then drop all role entitlements/grants for the group. Consider keying off the API's error_code / Detail field, or requiring the group ID to appear in the message, and add a table-driven test for this helper — the other new helpers in this PR (groupGrantParent, NewTokenAuth) each got one.
| ruleSets, rateLimitDataRuleSets, err := g.client.ListRuleSets(ctx, workspaceId, GroupsType, groupId.Resource) | ||
| if err != nil { | ||
| if isGroupNotFoundError(err) { | ||
| l.Debug("databricks-connector: skipping role rule sets for group not recognized by the rule-sets API", |
There was a problem hiding this comment.
🟡 Suggestion: this is a skip-and-continue that silently drops every role rule-set grant for the group, but it is logged at Debug, so a production sync (default info) shows nothing at all when role grants disappear. Skip-and-continue degradation should be Warn so it is visible, ideally with logarithmic sampling plus a total_occurrences field since it can fire once per group. Same applies to the Debug skip at groups.go:153 in Entitlements.
| var annotations []protoreflect.ProtoMessage | ||
| if resourceId.ResourceType == groupResourceType.Id { | ||
| rid, expandAnnotation, err := groupGrantExpansion(ctx, resourceId.Resource, resource.ParentResourceId) | ||
| groupParentResourceId, err := groupGrantParent(a.client.IsAccountAPIAvailable(), a.client.GetAccountId(), "") |
There was a problem hiding this comment.
🟡 Suggestion: this fixes the parenting mismatch, but Grants already returns early at line 110 when !IsAccountAPIAvailable(), so the flag is always true here and the workspace branch is unreachable. If that guard ever changes, the hardcoded "" would produce a principal ID of workspace//group/<id> (empty workspace segment) rather than an obvious failure. Consider calling rs.NewResourceID(accountResourceType, a.client.GetAccountId()) directly here, or hoisting the computation out of the per-principal loop so the intent is explicit.
…dead branch isGroupNotFoundError now requires "group" in the message alongside "not found" so it doesn't swallow unrelated 400s, with test coverage. The two skip-and-continue paths in groups.go log at Warn instead of Debug so degraded syncs are visible at the default log level. account.go's group grant parent now calls rs.NewResourceID directly since the account API guard earlier in the method already guarantees it.
| return false | ||
| } | ||
| return apiErr.StatusCode == http.StatusBadRequest && |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the two halves of this predicate use different casing rules — "not found" is matched case-sensitively while "group" is lowercased first. If Databricks ever returns "Group 12345 Not Found" (or any other capitalization), the guard silently stops matching and the sync hard-fails again instead of skipping the group. Consider lowering the message once and matching both substrings against it.
| l.Warn("databricks-connector: skipping role rule sets for group not recognized by the rule-sets API", | ||
| zap.String("group_id", groupId.Resource), | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): raising this (and the matching one at Entitlements, line 153) to Warn fixes the invisibility problem, but this fires once per unrecognized group. Under token auth an account with many workspace-local SCIM groups can hit it for most groups in the sync, producing thousands of identical warnings. Per the repo's logging criteria, a per-resource warning like this should use logarithmic sampling (1st, 10th, 100th, then every 1000th) with a total_occurrences field rather than one line per group.
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
why you warning instead of debug?
| // get all assignable roles for this specific group resource | ||
| roles, _, err := g.client.ListRoles(ctx, workspaceId, GroupsType, groupId.Resource) | ||
| if err != nil { | ||
| if isGroupNotFoundError(err) { |
There was a problem hiding this comment.
[Connector] isGroupNotFoundError isn't scoped to the workspace-token path it's meant for
Heads up — this new skip-on-"not found" logic isn't actually limited to the workspace-token auth case the doc comment describes ("an orphaned or stale workspace SCIM group"). Neither this call site nor the one in Grants (groups.go:239) checks workspaceId != "" or client.IsTokenAuth() before swallowing the error, so it fires for account-parented (OAuth) groups too.
I traced the client code: doRequest builds APIError the exact same way for account-scoped and workspace-scoped calls (no branching by endpoint in pkg/databricks/request.go), so a 400 whose message contains "...group...not found..." from the account-level rule-sets/roles API for a stale/unrecognized group reference would hit this same silent-skip path. That means an existing OAuth customer could now get an empty entitlements/grants result for an affected group instead of a loud sync failure, which cuts against "existing setups are unaffected" in the PR description. There's also no test covering this branch for the account-parented case (helpers_test.go's new TestIsGroupNotFoundError only unit-tests the classifier itself, not this wiring).
Easy fix: both call sites already have workspaceId (or isWorkspaceGroup in Grants) in scope right above the check, so if workspaceId != "" && isGroupNotFoundError(err) should do it — plus a regression test for the OAuth/account-parented case.
| // role permissions grants | ||
| ruleSets, rateLimitDataRuleSets, err := g.client.ListRuleSets(ctx, workspaceId, GroupsType, groupId.Resource) | ||
| if err != nil { | ||
| if isGroupNotFoundError(err) { |
There was a problem hiding this comment.
[Connector] Same gap as Entitlements above
Same issue as groups.go:152 — this isGroupNotFoundError check isn't gated to the workspace-parented case either, and isWorkspaceGroup is already sitting right above at line 187-189 if you want to gate it here too.
| if !errors.As(err, &apiErr) { | ||
| return false | ||
| } | ||
| return apiErr.StatusCode == http.StatusBadRequest && |
There was a problem hiding this comment.
[Connector] Message-substring match instead of the existing structured error field
Small nit while you're in here: this matches on strings.Contains(apiErr.Message, ...) (free text), but this same codebase already has a structured way to classify API errors elsewhere — apiErr.Detail == databricks.AlreadyExists shows up in groups.go. If there's a stable Detail value for "group not found", might be worth using that instead so this doesn't quietly break if Databricks ever tweaks the wording of the message.
There was a problem hiding this comment.
casing bug's fixed. on the Detail idea, i checked the actual response: the rule-sets API returns error_code: BAD_REQUEST and message: "Group <id> not found" with no top-level detail field, so apiErr.Detail is empty here. error_code is the real structured field but it's the generic BAD_REQUEST, so nothing but the message actually pins it to group-not-found. kept the message match for that reason.
|
|
||
| // Mirrors how groupBuilder parents synced groups: account when its API is | ||
| // reachable, otherwise the workspace (token auth). | ||
| func groupGrantParent(accountAPIAvailable bool, accountId, workspaceId string) (*v2.ResourceId, error) { |
There was a problem hiding this comment.
[Connector] Group-parenting policy now lives in two places
groupGrantParent re-encodes the same "groups live under account when the Account API is available, otherwise under the workspace" rule that accountResource() (account.go) already expresses via its conditional ChildResourceType list. Two independent places now carry the same policy — if one changes and the other doesn't, group grant parenting and group sync parenting could silently drift apart. Not blocking, but might be worth having one derive from the other, or at least a comment cross-referencing both.
There was a problem hiding this comment.
added cross-ref comments in both groupGrantParent and accountResource so they stay in sync.
| // databricks-exclude-workspaces set. Checks the name only, not via | ||
| // isWorkspaceExcluded: that also matches on ID, and a zero-value ID here would | ||
| // let an exclude entry of "0" match every workspace. | ||
| func (c *Client) IsWorkspaceExcluded(deploymentName string) bool { |
There was a problem hiding this comment.
[Client] Workspace-name matching now has 3 slightly different implementations
We've now got three ways of answering "does this workspace name match a configured one": the original isWorkspaceExcluded (matches on name/deployment-name/ID), this new exported IsWorkspaceExcluded (name-only, to dodge the zero-ID collision — good catch, but the near-identical name to the existing method is easy to mix up), and matchConfiguredWorkspace in workspaces.go (case-insensitive EqualFold scan, no pre-lowering). None of them share code. Not asking for a big refactor, but a more distinct name (e.g. IsWorkspaceNameExcluded) would help future readers not confuse this with the Workspace-struct-based original.
There was a problem hiding this comment.
renamed to IsWorkspaceNameExcluded. fixed.
| for workspace := range w.workspaces { | ||
| configured = append(configured, workspace) | ||
| } | ||
| ctxzap.Extract(ctx).Warn("databricks-connector: all configured workspaces are excluded, sync will be empty", |
There was a problem hiding this comment.
[Connector] Same warning-slice-building loop appears twice
This 4-line "collect configured workspace names for the warning log" loop is duplicated verbatim further down (around the "none of the configured workspaces matched" warning). Could pull it into a tiny helper (mapKeys(w.workspaces) or slices.Collect(maps.Keys(...))) so both warning sites are one-liners. Not blocking, just a nice cleanup while this code's already being touched.
| } | ||
| // With an explicit workspace list (always the case for token auth), validate each | ||
| // configured workspace. Otherwise discover every workspace from the Account API. | ||
| if len(d.workspaces) > 0 { |
There was a problem hiding this comment.
[Connector] Both Validate() branches run the identical inner loop
The len(d.workspaces) > 0 vs. ListWorkspaces()-discovery branches here differ only in how the list of workspace deployment names is obtained — the loop body itself (ListRoles(ctx, name, "", ""), same error check, same isWSAPIAvailable = true) is copy-pasted in both. Could resolve a single []string of names first, then run one loop over it, and cut ~10 duplicated lines. Not blocking.
| workspaces that are associated with provided tokens and all workspaces that are | ||
| in the list of workspaces. | ||
|
|
||
| When authenticating with `--workspace-tokens` instead of the OAuth client ID and |
There was a problem hiding this comment.
[Docs] Separate from this PR — "Username and password" auth is still documented but doesn't exist
Not something this PR needs to fix, but since you're already cleaning up the auth docs in this exact file: further up (README.md's "Bearer auth"/basic-auth paragraphs, around the intro section) this README still describes a third auth option — username/password ("Basic auth") — and there's genuinely zero implementation of it anywhere in the codebase (no BasicAuth type in pkg/databricks/auth.go, no username/password fields in pkg/config/config.go). Might be worth a follow-up to either implement it or drop the claim, since a customer following these docs for that option will configure something that silently doesn't exist.
There was a problem hiding this comment.
I pulled the username/password bits from the README in this PR. databricks EOL'd basic auth back in 2024 and it was never implemented here, so the docs were just stale.
https://docs.databricks.com/aws/en/security/auth/password-deprecation
| - Account ID | ||
| - Personal access token | ||
| - Workspace ID for the Databricks workspace you're syncing | ||
| - Deployment name of the Databricks workspace you're syncing (the subdomain in the workspace URL, not the workspace ID) |
There was a problem hiding this comment.
[Docs] Same "Username and password" gap as README.md
Same note as README.md: this page's auth-choices list (and the Kubernetes secret example further down) also still promises a "Username and password" auth method with BATON_USERNAME/BATON_PASSWORD that has no implementation anywhere in the code. Not this PR's job to fix, just flagging it since you're touching these exact docs for the PAT fix already.
There was a problem hiding this comment.
same, pulled the username/password bits from connector.mdx too.
Gate the rule-sets/roles 400 skip in Entitlements and Grants to workspace-parented groups only, so an OAuth/account-parented group hitting the same error shape surfaces a real sync failure instead of being silently skipped. Log the skip as a single Warn naming the group id, matching the portfolio norm, no counter. Fix a casing bug in isGroupNotFoundError's message match, cross-reference the duplicated account/workspace parenting policy, rename IsWorkspaceExcluded to IsWorkspaceNameExcluded, and dedupe two small duplicated loops flagged in review.
Add a FieldsMutuallyExclusive constraint so a config cannot set both the workspaces allowlist and the databricks-exclude-workspaces denylist, and note the exclusivity in both field descriptions. Regenerate config_schema.json.
README and connector.mdx documented a username/password (basic auth) method that the connector doesn't implement and Databricks retired (basic auth reached end of life 2024-07-10). Drop the auth choice, credential set, config-form step, CLI examples, and the BATON_USERNAME/BATON_PASSWORD Kubernetes secret block so the docs match the two auth methods the connector actually supports (OAuth and workspace token).
| BATON_WORKSPACES: <Deployment name of the Databricks workspace you're syncing, not the workspace ID> | ||
|
|
||
| # Optional: comma-separated workspaces to exclude from sync (workspace name, deployment name, or numeric ID) | ||
| BATON_DATABRICKS_EXCLUDE_WORKSPACES: <workspace-a,workspace-b> |
There was a problem hiding this comment.
🟡 Suggestion: this commit adds field.WithConstraints(field.FieldsMutuallyExclusive(WorkspacesField, ExcludeWorkspacesField)), and the SDK enforces it globally (pkg/field/validation.go → validateConstraints, present > 1 → hard error), so setting BATON_WORKSPACES and BATON_DATABRICKS_EXCLUDE_WORKSPACES together now fails startup with fields marked as mutually exclusive were set. Option 2 above requires BATON_WORKSPACES, so a workspace-token user who follows this example verbatim can never use this "Optional" line. Reword it to say the exclude list only applies to OAuth syncs that do not set BATON_WORKSPACES.
|
|
||
| if w.client.IsTokenAuth() { | ||
| for workspace := range w.workspaces { | ||
| if w.client.IsWorkspaceNameExcluded(workspace) { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): with the new mutual-exclusion constraint, workspaces and databricks-exclude-workspaces can never both be set, and token auth always has workspaces populated (ValidateConfig requires it to match workspace-tokens in length). So IsWorkspaceNameExcluded here can never return true, the len(rv) == 0 "all configured workspaces are excluded" warning below (line 103) is unreachable, and the same is true of the exclude branch at line 148. Worth dropping the dead branches or, if excluding a subset of a --workspaces list is still intended, relaxing the constraint instead.
| in the list of workspaces. | ||
|
|
||
| When authenticating with `--workspace-tokens` instead of the OAuth client ID and | ||
| secret, also pass `--auth-method workspace-token` (or set |
There was a problem hiding this comment.
🟡 Suggestion: while cleaning up these auth docs, the paragraph just above (lines 88-96) is now stale. --workspaces takes deployment names, not "workspace hostnames" (per the field description), and NewTokenAuth pairs --workspaces/--workspace-tokens positionally with ValidateConfig requiring equal lengths — so "use both flags at the same time … sync with all workspaces associated with provided tokens and all workspaces in the list" no longer describes the behavior. The --help dump further down (lines 147/175) is also stale relative to the new "Mutually exclusive with …" descriptions in pkg/config/config.go.
Add a FieldsDependentOn constraint so the schema declares workspaces as required whenever workspace-tokens is provided, matching the field description. Use FieldsDependentOn rather than FieldsRequiredTogether: the latter is symmetric and would also reject a valid OAuth config that sets --workspaces alone to scope the sync. The ValidateConfig length check stays, since the constraint only enforces presence, not the positional equal-length pairing.
7 participants
The Databricks connector can again authenticate with per-workspace personal access tokens, matching the setup docs, so customers who use workspace tokens instead of OAuth can connect. Additive change: OAuth stays the default and existing setups are unaffected.