Skip to content

feat(kernel): support static token federation - #501

Open
vuanhphung wants to merge 7 commits into
mainfrom
vu-phung/kernel-federation-client-id
Open

feat(kernel): support static token federation#501
vuanhphung wants to merge 7 commits into
mainfrom
vu-phung/kernel-federation-client-id

Conversation

@vuanhphung

@vuanhphung vuanhphung commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Add kernel-backend support for static-token, forwarding staticToken through the native bearer-token path.

Kernel federation is always enabled for this path, so enableTokenFederation is ignored. An omitted or empty federationClientId selects account-wide WIF; a non-empty ID selects SP-wide WIF. Update the connection-parameter reference and add focused coverage.

@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

Comment thread lib/contracts/IDBSQLClient.ts
@vuanhphung
vuanhphung force-pushed the vu-phung/kernel-federation-client-id branch from 9ae995c to f26244c Compare August 19, 2026 20:18
@vuanhphung vuanhphung changed the title feat(kernel): forward identity federation client ID feat(kernel): support static token federation Aug 19, 2026
@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — the kernel static-token support is correct, forwards identityFederationClientId only when enableTokenFederation && federationClientId, and has focused unit coverage; docs and the unsupported-mode error are updated consistently. One low-severity consistency note: the new branch lacks the ambiguous-combo guard its sibling PAT branch has.

Comment thread lib/kernel/KernelAuth.ts Outdated
@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Medium · 1 Low

Focused, well-tested addition that maps static-token onto the kernel's native Pat bearer mode and forwards federationClientId. Code is correct and consistent with the existing PAT branch; one medium concern about how the account-wide-WIF (enableTokenFederation: true with no federationClientId) case is conveyed to the native binding, plus a minor validation-asymmetry note.

Comment thread lib/kernel/KernelAuth.ts Outdated
Comment thread lib/kernel/KernelAuth.ts
@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — a clean, well-tested addition of kernel static-token support. The option-builder validates the token via the shared blank/reserved predicate, rejects OAuth-field conflicts, correctly gates identityFederationClientId behind enableTokenFederation, and maps empty/omitted client IDs to account-wide WIF; unit coverage exercises every arm and the docs are updated consistently. One low-severity cross-backend federation-semantics note is filed inline.

Comment thread lib/kernel/KernelAuth.ts Outdated
Comment thread lib/kernel/KernelAuth.ts Outdated
'on the same connection. Pick one auth mode.',
);
}
if (enableTokenFederation) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guard seem cannot work for all scenario, the kernel does not have a flag of enableTokenFederation, so kernel will try to token exchange no matter what.
I think we should either add the enableTokenFederation flag in kernel, or just remove the enableTokenFederation check here and call out for kernel that field is ignored and always enabled.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eric-wang-1990 I think removing enableTokenFederation is the simplest given that no other driver has this field. I updated to call out this is ignored in kernel mode

@github-actions

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — the static-token → native PAT bearer path is clean, correctly maps federationClientId (empty → account-wide via || undefined), ignores enableTokenFederation as documented, and has focused unit coverage. One low: the new branch omits the token-vs-staticToken conflict guard that the sibling auth branches enforce, so a stray token is silently ignored.

Comment thread lib/kernel/KernelAuth.ts
"kernel backend: a non-empty token must be supplied via `staticToken` when using `authType: 'static-token'`.",
);
}
if (oauth.oauthClientId !== undefined || oauth.oauthClientSecret !== undefined) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low — The static-token branch guards against conflicting OAuth credentials (oauthClientId/oauthClientSecret) but does not guard against a conflicting token (PAT). The other two branches are symmetric about ambiguity: access-token rejects when OAuth fields are also set, and databricks-oauth rejects when token is also set. Here, if a caller supplies both authType: 'static-token' + staticToken and a token, the token is silently dropped and static-token auth is used with no diagnostic.

This matters most for a user migrating from PAT who leaves token in their config while switching authType to static-token — a likely misconfiguration that the symmetric guard elsewhere is designed to surface. Consider rejecting token alongside staticToken for parity with the other arms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants