CXH-2166: revert PAT auth and workspace filtering (revert #54)

[luisina-santos]

Full revert of #54, which implemented PAT (workspace-token) authentication and workspace-allowlist filtering.

Why

CXH-2109 records both of these as the connector's actual and expected behaviour:

1. PAT and username/password auth are not implemented
2. Workspace filtering is not implemented

For both, the correct resolution is to correct the documentation, not to build the feature. #54 built them instead. This reverts that and updates the docs to describe what the connector actually does.

PAT auth was already unreachable: #63 commented out the workspace-token field group and rejected the auth method in ValidateConfig, but left the implementation in the tree behind docs describing it as temporarily unavailable and expected to return. OAuth2 is now the only authentication method, and the docs say so without qualification.

Scope: verified byte-for-byte against pre-#54

These files are now identical to b38a0beb^1, the commit before #54 merged:

File	What #54 had added
pkg/config/config.go	workspaces + workspace-tokens fields, field groups, constraints, ValidateConfig
pkg/databricks/auth.go	TokenAuth / NewTokenAuth
pkg/connector/workspaces.go	workspace allowlist filtering (~60 lines)
pkg/connector/helpers.go	groupGrantParent, isGroupNotFoundError
pkg/connector/groups.go	group-not-found skip paths
pkg/connector/roles.go	grant-parent indirection
pkg/connector/account.go	grant-parent change
pkg/connector/service-principals.go	grant-parent change

pkg/databricks/client.go drops IsTokenAuth and IsWorkspaceNameExcluded (both from #54) and keeps everything added after it.

The only non-vendor files that still differ from pre-#54 are these six, and every one is post-#54 work belonging to another ticket, deliberately preserved:

• pkg/connector/connector.go — CXH-2350's fatal account-API probe (from CXH-2350: surface workspace-token sync limits and fix PAT setup docs #57, not CXH-2166: implement PAT (workspace token) authentication #54)
• pkg/databricks/request.go — CXH-2350's wrapTransportAuthError
• pkg/databricks/client.go — Azure/GCP account-host fix, and CXH-2109: graceful 403 handling, workspace exclude flag, doc cleanup #55's 403 remedy
• validate_test.go, client_test.go, request_test.go — their tests

--databricks-exclude-workspaces predates #54 and is untouched. .github/workflows/ci.yaml is untouched.

Docs

README.md, docs/connector.mdx and docs/docs-info.md drop both the PAT method and the workspace allowlist, along with the "temporarily unavailable / expected to return" framing. Scope narrowing is documented via --databricks-exclude-workspaces only. The README CLI dump, conf.gen.go and config_schema.json are regenerated from the built binary.

⚠️ Breaking change

Self-hosted deployments using workspace tokens must move to OAuth2 (#63 already broke these at runtime). Deployments using --workspaces must switch to --databricks-exclude-workspaces.

Two hunks of #54 deliberately kept: the group grant-parent fixes

The revert is complete except for two grant-parenting hunks, which are kept. Each is in its own commit so it can be reviewed, or dropped, independently of the revert.

pkg/connector/account.go — fixes a live bug. accountBuilder.Grants passed resource.ParentResourceId into groupGrantExpansion. There resource is the account itself, whose ParentResourceId is nil, so groupResourceId returned group/<id> — while groups actually sync as account/<accountId>/group/<id>. The expansion named a resource no sync emits. #54 had incidentally fixed this; reverting it would regress against main.

pkg/connector/service-principals.go — consistency. servicePrincipalBuilder.Grants passed resource.ParentResourceId into groupResourceId. Unlike the account case this is non-nil, so it is not broken under OAuth today, but it derives the parent from a different source than groupBuilder uses when syncing the group. Naming the parent from the service principal's own profile — as the rest of that method already does for workspaceId — keeps the expansion and the synced group derived from one source.

Both are kept because, unlike everything else in #54, neither is PAT or workspace-filtering work: they concern grant parenting under OAuth, which is the only auth method left.

Verification

• go build ./..., go vet ./..., go test ./pkg/... all pass
• --workspaces and --workspace-tokens are gone from --help; --databricks-exclude-workspaces remains
• Required-field enforcement intact without the field groups: no credentials → required flag(s) "account-id", "databricks-client-id", "databricks-client-secret" not set (exit 2)
• Repo-wide sweep for workspace-token / WORKSPACE_TOKENS / TokenAuth / BATON_WORKSPACES is clean apart from the README line stating PAT is not supported
• golangci-lint not run locally — the installed version rejects this repo's --out-format flag, so CI is the first lint signal

🤖 Generated with Claude Code

Review round: dead availability machinery removed

Two further commits, in response to review. Neither is part of the revert proper; both clean up state the revert left stranded.

db1a1e3a — drop the two-plane availability machinery. With OAuth the only auth method, the account plane is either reachable or Validate hard-fails, so the account/workspace availability split no longer carries information:

• Validate's per-workspace ListRoles loop set isWSAPIAvailable unconditionally inside its own body, so it reduced to len(workspaces) > 0, and only Debug-logged failures — one sequential API call per workspace on every sync start, for no signal. Removed. ListWorkspaces stays: every sync depends on it.
• IsWorkspaceAPIAvailable() had no callers anywhere in the repo. It, isWSAPIAvailable and UpdateAvailability are gone.
• isAccAPIAvailable is now set in NewClient rather than only flipping when Validate runs, so the five branches still gating sync output on it no longer depend on call ordering. The branches themselves are left in place — deleting them touches sync-output paths, which is beyond this PR.

Same commit also drops prepareClientAuth's two dead parameters, restores the blank identifier for NewConnector's unused *cli.ConnectorOpts, and removes the redundant Warn before returning an already-wrapped error from NewConnector — the last of these was #54's c29d543e cleanup that the revert had pulled back in by mistake.

cf998aec — rolesTransport's doc comment no longer referenced reachable behaviour.

One review request declined, with evidence

Restoring #54's isGroupNotFoundError skip guard in groups.go was requested on the grounds that it is a general improvement like the two kept grant-parent hunks. It isn't reachable after this revert: the only builder that ever parented groups under a workspace is minimalWorkspaceResource, which exists for token auth by its own doc comment and is reverted here. The surviving workspaceResource annotates only role as a child (as it did at b38a0beb^1), so groupBuilder.List is only ever called with the account as parent and workspaceId is always empty. Full reasoning is on the thread.

[linear-code]

CXH-2166

[github-actions]

🟡 Suggestion: this loop no longer influences anything. isWSAPIAvailable is set to true unconditionally inside the body regardless of the probe result, so it reduces to len(workspaceNames) > 0, and Client.IsWorkspaceAPIAvailable() (the only consumer of that bit) has no callers anywhere in the repo. The net effect is one ListRoles call per workspace on every sync start whose outcome is only Debug-logged — on a large account that's N sequential API calls for no signal. Either drop the loop, or make the outcome meaningful and log the failure at Warn (per the repo's L1 log-level rule): a workspace the service principal can't reach here will hard-fail later in roles.go, so a debug line invisible at the default info level is the quiet-degradation shape the criteria warn about.

[luisina-santos]

Dropped the loop in db1a1e3a. You're right that it reduced to len(workspaces) > 0 — isWSAPIAvailable = true sat outside the error branch — and IsWorkspaceAPIAvailable() had no callers anywhere in the repo, so the whole bit was write-only. Rather than raise the log level I removed the probe entirely: a workspace the service principal can't reach is already skipped during sync, so the N sequential calls bought nothing. ListWorkspaces stays, since every sync depends on it.

[github-actions]

🟡 Suggestion: hardcoding true here is correct now that OAuth is the only auth, but it leaves Client.isAccAPIAvailable as a field that defaults to false and only flips when Validate runs. Four branches still gate real sync output on it — account.go:52 (account child resource types), account.go:94/account.go:111 (account entitlements/grants), and workspaces.go:147/workspaces.go:169 (workspace-member entitlements/grants) — so they are now dead-but-load-bearing on Validate having executed first. Consider initializing isAccAPIAvailable: true in NewClient and deleting those branches (and the now-unused IsWorkspaceAPIAvailable), so the invariant isn't carried by call ordering.

[luisina-santos]

Done in db1a1e3a, along the lines you suggested: isAccAPIAvailable is now set in NewClient, so the five branches gating on it no longer depend on Validate having run first. UpdateAvailability, IsWorkspaceAPIAvailable and isWSAPIAvailable are gone. I left the gating branches themselves in place — deleting them touches sync-output paths, which is more than this revert should carry; happy to do it as a follow-up.

[github-actions]

Connector PR Review: CXH-2166: revert PAT auth and workspace filtering (revert #54)

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 10ae732f0d83.
Review mode: incremental since 56703764
View review run

Review Summary

The new commits finish collapsing Validate to the OAuth-only shape: the per-workspace probe loop is gone (replaced by a bare ListWorkspaces reachability check), UpdateAvailability / IsWorkspaceAPIAvailable / isWSAPIAvailable are deleted, isAccAPIAvailable is now pinned to true in NewClient, and the stale rolesTransport doc comment is corrected. That addresses all the prior connector.go feedback — prepareClientAuth is down to a single cfg parameter, NewConnector's unused opts is back to the blank identifier, the probe loop no longer discards its result, and the hardcoded UpdateAvailability(true, ...) is gone. I re-scanned the full PR diff for security and correctness and found no blocking issues: go.mod / go.sum are untouched, no endpoint or request/response shape changed, and the docs (README.md, docs/connector.mdx, docs/docs-info.md, config_schema.json, conf.gen.go) match the reverted surface. The earlier pkg/config/config.go:46 suggestion — no migration warning for deployments still setting BATON_WORKSPACES, which silently widen from an allowlisted subset to every reachable workspace — remains open and is not restated below.

Security Issues

None found. These commits only remove code (the workspace probe loop and two availability accessors); no credential handling, logging, or auth check is added or weakened.

Correctness Issues

None found. Verified there are no remaining references to the deleted UpdateAvailability, IsWorkspaceAPIAvailable, or isWSAPIAvailable, and the narrowed Validate still hard-fails on both account-plane calls it makes, so no sync proceeds on unusable credentials.

Suggestions

• pkg/databricks/client.go:117 — isAccAPIAvailable is now set once at construction and never mutated, making IsAccountAPIAvailable() a constant and leaving its five guard sites (account.go:51,93,110, workspaces.go:87,109) as unreachable dead branches.
• pkg/connector/validate_test.go:15 — Validate's new ListWorkspaces hard-fail path and its success path are both untested; the one remaining test short-circuits on the account check before reaching either.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/databricks/client.go`:
- Around line 117: `isAccAPIAvailable` is initialized to true in `NewClient` and, now
  that `UpdateAvailability` has been deleted, is never written again. That makes
  `IsAccountAPIAvailable()` a compile-time-constant true, so the false branches at
  `pkg/connector/account.go:51`, `account.go:93`, `account.go:110`,
  `pkg/connector/workspaces.go:87` and `workspaces.go:109` are unreachable. Either
  delete the `isAccAPIAvailable` field, the `IsAccountAPIAvailable()` accessor and
  those five guard sites so the OAuth-only invariant is structural, or keep the
  accessor and drop only the dead branches. Do not reintroduce a setter.

In `pkg/connector/validate_test.go`:
- Around line 15: `Validate` now makes two hard-failing account-plane calls (`ListRoles`
  for the account check, then `ListWorkspaces`), but only the first is covered.
  `TestValidateOAuthAccountCheckFailureReturnsError` returns before `ListWorkspaces` is
  reached, and the previous success-path test was deleted along with the token-auth
  tests. Extend `rolesTransport` to route on `req.URL.Path` so it can return a JSON
  array body for the workspaces endpoint — the current blanket roles-object body will
  not unmarshal into a `[]Workspace` slice — then add two tests: one where the
  workspaces call fails and `Validate` must return an error, and one where both calls
  succeed and `Validate` must return nil.

Review state marker omitted: this run's shell sandbox rejects the JSON marker text, so the next review will fall back to a full-diff pass. Head SHA reviewed: cf998aecf0c39368aedb9fd98a381f2caa819d7a, base 10ae732f0d83e85888afee13d6119ded4ccf889e.

[github-actions]

No blocking issues found.

[github-actions]

🟡 Suggestion: prepareClientAuth now has two dead parameters — the newly added _ context.Context, and l *zap.Logger, whose only use (l.Debug("using oauth", ...)) was deleted in this commit. NewConnector also renamed _ *cli.ConnectorOpts to opts without using it. Dropping ctx/l and restoring _ for opts would keep the signatures honest (connector mixin R2). Confidence: high; cosmetic only, revive's unused-parameter rule isn't enabled so lint won't catch it.

[luisina-santos]

Fixed in db1a1e3a — prepareClientAuth(cfg *config.Databricks), and opts is back to _. The ctxzap/zap imports went with it, since the redundant NewConnector Warn was removed in the same commit.

[github-actions]

No blocking issues found.

[sergiocorral-conductorone]

Automated review

[sergiocorral-conductorone]

[medium] This skip guard (and its twin at the Grants rule-sets call below) isn't PAT/workspace-filtering-specific, even though it's being removed along with that work. workspaceId here comes from resource.ParentResourceId.Resource — still populated today, under OAuth-only auth, whenever a group is parented to a workspace rather than the account (see groupBuilder.Entitlements/Grants in this same file). The removed comment described a real, distinct failure mode: "an orphaned or stale workspace SCIM group" causing the rule-sets/roles API to 400 with a group-not-found message. Without this guard, one stale workspace group now hard-fails the entire Entitlements/Grants call for that group (fmt.Errorf(...) propagates) instead of being warned and skipped.

This looks like the same category of thing as the two grant-parent fixes this PR already keeps independently of the revert (account.go/service-principals.go) — a genuine improvement that happened to ship inside #54, not itself PAT-or-workspace-filtering work. Worth considering keeping it (and its helpers.go/helpers_test.go counterpart) the same way, in its own commit, rather than reverting it along with everything else.

Codex verification (independent adversarial pass): CONFIRMED as an additional major — "the revert drops the isGroupNotFoundError skip path and now hard-fails on those API responses... turning that back into fatal errors can make one stale/orphaned workspace group abort an otherwise valid sync." I independently confirmed workspaceId is still reachable and non-empty under OAuth for workspace-parented groups before including this.

[luisina-santos]

Thanks for digging into this one — I went looking to keep it, but I don't think the reachability premise holds once the rest of the revert lands.

The claim is that workspaceId is "still populated today, under OAuth-only auth, whenever a group is parented to a workspace." The second half is the part that stops being true on this branch: after the revert, nothing ever parents a group to a workspace.

The chain:

1. groupBuilder.List (pkg/connector/groups.go:76) is only ever called with parents whose resource declares group as a child resource type.
2. On main there are two workspace builders. minimalWorkspaceResource annotates user/group/service_principal/role as children — and its own doc comment says why it exists: "for token auth where the Account API (and its numeric workspace IDs) is unreachable… Users, groups and service principals hang off the workspace here instead of the account." That function is CXH-2166: implement PAT (workspace token) authentication #54 code, and this PR reverts it.
3. What's left is workspaceResource (pkg/connector/workspaces.go:46 on this branch), which annotates only roleResourceType as a child. I checked it back through history: b38a0beb^1 had only role too, so this is the pre-CXH-2166: implement PAT (workspace token) authentication #54 shape being restored, not something the revert invented.
4. So post-revert the only parent that lists groups is the account (pkg/connector/account.go:53), parentResourceID.ResourceType is always account, and both workspaceId assignments — groups.go:130 in Entitlements and the isWorkspaceGroup branch in Grants — always evaluate to "".

With workspaceId empty, the rule-sets call goes to the account plane, where the "orphaned or stale workspace SCIM group" 400 the guard was written for can't arise. So unlike the two grant-parent hunks — which fix behaviour that's live under OAuth today — this one has no reachable path left to protect, and keeping it would restore a helper plus its tests for a branch that can't execute.

The distinction I'm drawing: the account.go hunk fixes a grant expansion that names a resource no sync emits, right now, under OAuth. This guard only bites when groups sync under a workspace, which is exactly the token-auth topology being removed. If workspace-parented groups ever come back, the guard should come back with them.

Happy to be wrong if you're seeing a path to a workspace-parented group that I've missed — that'd change the answer.

[github-actions]

🟡 Suggestion: With UpdateAvailability removed, isAccAPIAvailable is now set to true at construction and never mutated, so IsAccountAPIAvailable() is a constant. Every remaining guard on it — account.go:51, account.go:93, account.go:110, workspaces.go:87, workspaces.go:109 — is dead code whose false branch can no longer be reached. Consider dropping the field, the accessor, and those branches so the OAuth-only invariant is expressed in the code rather than in a comment.

[github-actions]

🟡 Suggestion: Validate gained a second hard-failing account-plane call (ListWorkspaces), but no test covers it — TestValidateOAuthAccountCheckFailureReturnsError returns before reaching it, and the success-path test was deleted with the token-auth tests. Note that rolesTransport answers every request with {"roles":[]}, which will not unmarshal into ListWorkspaces' []Workspace, so adding coverage means teaching the transport to route on path, then asserting both the workspace-list failure and the all-green success path.

[github-actions]

No blocking issues found.