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
vuanhphung
changed the title
|
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 ( |
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
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.
|
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.
|
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 kernel arm is correctly structured (token validation, OAuth-conflict rejection, federation forwarding) with solid unit coverage, and the doc/contract updates are consistent. One low-severity note: federationClientId skips the isBlankOrReserved normalization applied to the token, so malformed-but-truthy values ('undefined', whitespace) would be forwarded as a literal WIF client id.
| 'on the same connection. Pick one auth mode.', | ||
| ); | ||
| } | ||
| base.identityFederationClientId = federationClientId || undefined; |
There was a problem hiding this comment.
🔵 Low — federationClientId is forwarded with only a falsy-check (federationClientId || undefined), unlike staticToken (and token), which are guarded with isBlankOrReserved. As a result the same malformed-input cases the module deliberately screens elsewhere leak through here: federationClientId: 'undefined', 'null', or whitespace-only (' ') are all truthy, so they're passed to the native binding as a literal SP-wide WIF client id instead of collapsing to account-wide WIF. A export FED_ID="$UNSET" shell mishap would silently select the wrong federation mode and surface as an opaque kernel-side error rather than an actionable one. Consider normalizing with the existing predicate, e.g. base.identityFederationClientId = typeof federationClientId === 'string' && !isBlankOrReserved(federationClientId) ? federationClientId : undefined;, matching the module's own rationale for isBlankOrReserved.
2 participants
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.