feat(kernel): support static token federation - #501
Conversation
|
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 ( |
9ae995c to
f26244c
Compare
|
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 ( |
There was a problem hiding this comment.
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.
|
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 ( |
|
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 ( |
|
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 ( |
There was a problem hiding this comment.
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.
|
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 ( |
There was a problem hiding this comment.
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.
| 'on the same connection. Pick one auth mode.', | ||
| ); | ||
| } | ||
| if (enableTokenFederation) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
|
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 ( |
There was a problem hiding this comment.
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.
| "kernel backend: a non-empty token must be supplied via `staticToken` when using `authType: 'static-token'`.", | ||
| ); | ||
| } | ||
| if (oauth.oauthClientId !== undefined || oauth.oauthClientSecret !== undefined) { |
There was a problem hiding this comment.
🔵 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.
Add kernel-backend support for
static-token, forwardingstaticTokenthrough the native bearer-token path.Kernel federation is always enabled for this path, so
enableTokenFederationis ignored. An omitted or emptyfederationClientIdselects account-wide WIF; a non-empty ID selects SP-wide WIF. Update the connection-parameter reference and add focused coverage.