Skip to content

Add SSO OIDC ID token retrieval#234

Merged
natansil merged 8 commits into
mainfrom
getIdToken
Jul 21, 2026
Merged

Add SSO OIDC ID token retrieval#234
natansil merged 8 commits into
mainfrom
getIdToken

Conversation

@natansil

@natansil natansil commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add base44.asServiceRole.sso.getIdToken(userid) for the app-scoped SSO OIDC ID-token endpoint
  • return the backend's raw JSON string as a typed Promise<string>
  • document backend-function usage and test the ID-token request, raw response, service-role authorization, and on-behalf-of authentication
  • preserve getAccessToken implementation, signature, exported response type, and customer example while adding regression coverage for its existing raw-string runtime response

Backend dependency

This SDK change depends on apper PR #16698.

The SDK method requires the backend endpoint from that PR to be deployed before it can succeed.

Validation

  • npm test
  • npm run build
  • npm run lint

@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown

🚀 Package Preview Available!


Install this PR's preview build with npm:

npm i @base44-preview/sdk@0.8.39-pr.234.9e8b8b2

Prefer not to change any import paths? Install using npm alias so your code still imports @base44/sdk:

npm i "@base44/sdk@npm:@base44-preview/sdk@0.8.39-pr.234.9e8b8b2"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "@base44/sdk": "npm:@base44-preview/sdk@0.8.39-pr.234.9e8b8b2"
  }
}

Preview published to npm registry — try new features instantly!

@natansil
natansil marked this pull request as ready for review July 20, 2026 19:59
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error —— View job


I'll analyze this and get back to you.

@natansil natansil closed this Jul 21, 2026
@natansil natansil reopened this Jul 21, 2026
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @natansil's task —— View job


PR Review: Add SSO OIDC ID token retrieval

  • Read changed files (sso.ts, sso.types.ts, sso.test.ts)
  • Review code quality & best practices
  • Check for bugs/issues
  • Review test coverage
  • Post review feedback

Overall this is a clean, well-tested addition. The tests are thoughtfully written (raw response, service-role auth, on-behalf-of header). I have a few observations, one of which I'd consider a real correctness concern.


🐞 Bugs / Correctness

1. getAccessToken's declared return type is inconsistent with runtime behavior

The response interceptor in src/utils/axios-client.ts:238 returns response.data, so axios.get() resolves to the response body, not an AxiosResponse. This has two consequences:

  • getAccessToken is typed as Promise<SsoAccessTokenResponse> (sso.types.ts:46) but the new test asserts it resolves to a raw string (sso.test.ts:56expect(accessToken).toBe(rawAccessToken)). So either the backend returns a raw string (and the SsoAccessTokenResponse type + the doc example response.access_token are wrong), or the backend returns { access_token } (and the new test's mock is wrong). These two can't both be right — worth confirming against the actual backend contract.

  • Note sso.ts no longer imports AxiosResponse/AxiosInstance usage is fine, but the type annotations in sso.types.ts still import AxiosResponse (sso.types.ts:1) — is it still used? If not, it's a dead import (lint should catch this).

2. getIdToken double cast is a code smell

const response = await axios.get<string>(url);
return response as unknown as string;

Because the interceptor already unwraps to response.data, axios.get<string>(url) is typed as AxiosResponse<string> by axios' signature but is actually the string at runtime. The as unknown as string works but hides the mismatch. Consider mirroring how connectors.ts:36-41 handles the same situation (with a documented @ts-expect-error) for consistency, or centralizing the "interceptor unwraps data" typing so casts aren't needed per-call. At minimum, add a short comment explaining why the double cast is necessary (the interceptor unwrapping isn't obvious to future readers).


🔒 Security

  • No secrets logged; token is returned to the caller only. ✅
  • Endpoint is app-scoped and relies on service-role + on-behalf-of auth, which the test covers. ✅
  • Minor: consider validating userid (non-empty string) before building the URL — connectors.ts:32 does this for its param. An empty/undefined userid would currently produce a malformed URL like .../idtoken/undefined. Low severity but cheap to guard.

⚡ Performance

No concerns — single GET, no extra work.


🧪 Test Coverage

  • Good coverage of the happy path, raw-string response, and auth headers. 👍
  • Missing: an error-path test (e.g. 401/404 → Base44Error) would strengthen confidence, especially since this is auth-sensitive.
  • Suggestion: the getIdToken test at sso.test.ts:60 mocks the on-behalf-of header but doesn't assert the returned value; combining header assertion + value assertion, or adding a comment, would make intent clearer.

✨ Minor / Nits

  • createSsoModule's JSDoc still documents a userToken param (sso.ts:9) that isn't in the signature — leftover doc, should be removed.
  • Consider asserting the exact URL path once in a shared constant to avoid drift between the module and tests.

Nice work overall — the main thing to resolve before merge is the getAccessToken type-vs-runtime inconsistency (item #1), since the PR description states it "preserves the exported response type" while the new test implies a raw string.
• Branch: getIdToken

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @natansil's task —— View job


PR Review: Add SSO OIDC ID token retrieval (updated)

  • Read changed files (sso.ts, sso.types.ts, sso.test.ts)
  • Review related context (axios-client.ts, connectors.ts)
  • Analyze code quality, bugs, security, tests
  • Post review feedback

Thanks for the updates since my last pass — most of the prior feedback has been addressed. Re-review of the current state:

✅ Addressed since last review

  • Added a 404 error-path test (sso.test.ts:79) asserting Base44Error / status / code. 👍
  • Added an explanatory comment on the double cast in getIdToken (sso.ts:27-29) clarifying that the response interceptor unwraps response.data. 👍
  • Removed the dead AxiosResponse import from sso.types.ts. 👍
  • Cleaned up the stale userToken JSDoc param on createSsoModule. 👍

🐞 Correctness — the one remaining thing to resolve

getAccessToken return type still contradicts its runtime behavior.

  • Interface: getAccessToken(userid): Promise<SsoAccessTokenResponse> (sso.types.ts:44), where SsoAccessTokenResponse = { access_token: string }.
  • Runtime: the interceptor returns response.data (axios-client.ts:238), and the new test asserts the resolved value is a raw string (sso.test.ts:58expect(accessToken).toBe(rawAccessToken) with mock body JSON.stringify("access-token-123")).

So the declared type says "object with .access_token" but the test locks in "raw string". Both can't be correct. Two knock-on issues:

  • The doc example at sso.types.ts:41 (console.log(response.access_token)) would print undefined if the runtime value is really a raw string.
  • The module example at sso.types.ts:24 still uses response.data.access_token, which is inconsistent with both the interface (response.access_token) and the raw-string test.

This is the same underlying mismatch connectors.ts handles explicitly via @ts-expect-error (connectors.ts:28,40). Recommend picking one source of truth against the actual backend contract:

  • If the endpoint truly returns a raw string, change the signature to Promise<string> (matching getIdToken) and fix both doc examples; or
  • If it returns { access_token }, update the new test's mock to JSON.stringify({ access_token: rawAccessToken }) and assert accessToken.access_token.

Given the PR description says it "preserves the exported response type," option 2 (fix the test/mock) may be the intended path — but please confirm against the backend before merge.

🔒 Security

  • No token logging; token returned to caller only. ✅
  • Service-role + on-behalf-of auth is covered by a dedicated header-matching test (sso.test.ts:62). ✅
  • Minor (optional): getIdToken doesn't validate userid. connectors.ts:32 guards its param and throws early; an empty/undefined userid here would build .../idtoken/undefined. Low severity, cheap to add for parity.

⚡ Performance

No concerns — single GET.

🧪 Test Coverage

Now solid: happy path, raw-string response, auth headers, and a 404 error path. The only gap is the getAccessToken type/mock inconsistency noted above rather than missing coverage.

✨ Nits

  • Consider a shared constant for the URL paths to avoid drift between module and tests (still applies but purely cosmetic).

Overall this is close to mergeable — the sole blocker is reconciling getAccessToken's type, doc examples, and the new test against the real backend response shape (#Correctness above).

View job • Branch: getIdToken

@github-actions github-actions Bot added the docs-draft PR has auto-drafted documentation suggestions label Jul 21, 2026
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @natansil's task —— View job


PR Review: Add SSO OIDC ID token retrieval (re-review)

  • Read changed files (sso.ts, sso.types.ts, sso.test.ts)
  • Review related context (axios-client.ts, connectors.ts)
  • Analyze code quality, bugs, security, tests
  • Post review feedback

Thanks for the iterations. The implementation is noticeably simpler now — both methods are just return axios.get(url), letting the interface types describe the return shape. Re-review of the current state:

✅ Improvements since last pass

  • getIdToken no longer uses the as unknown as string double cast — it relies on the interface's Promise<string> return type. Cleaner. 👍
  • Solid test suite: happy path, raw-string response, service-role + on-behalf-of header matching, and a 404 error path asserting Base44Error. 👍

🐞 Correctness — the type mismatch is now silently hidden

The previous reviews flagged the getAccessToken type-vs-runtime inconsistency. The current code doesn't resolve it — it just makes TypeScript stop complaining:

  • sso.ts:20return axios.get(url) is called without a type parameter, so it returns AxiosResponse<any>. That any is assignable to the declared Promise<SsoAccessTokenResponse>, so the compiler is satisfied even though at runtime the interceptor (axios-client.ts:238 return response.data) resolves the promise to the raw string body.
  • The test at sso.test.ts:58 locks in that runtime reality: expect(accessToken).toBe(rawAccessToken) where the mock body is JSON.stringify("access-token-123") — a raw string.

So the declared type still says { access_token: string } while the value is actually a string. A consumer following the type would write response.access_token and get undefined at runtime. The doc examples reinforce the wrong shape:

  • sso.types.ts:24console.log(response.data.access_token)
  • sso.types.ts:41console.log(response.access_token)

Both would print undefined given the raw-string runtime behavior the test asserts. This is the same underlying problem connectors.ts chose to make explicit with @ts-expect-error (connectors.ts:28,40) rather than hide behind an untyped axios.get.

Recommendation — pick one source of truth against the real backend contract:

  • If the endpoint returns a raw string (as the test asserts): change the signature to Promise<string> to match getIdToken, drop SsoAccessTokenResponse, and fix both doc examples. This is the honest fix.
  • If it returns { access_token }: update the test mock to JSON.stringify({ access_token: rawAccessToken }) and assert accessToken.access_token, and add a type param to axios.get<SsoAccessTokenResponse>(url).

The PR description says it "preserves the exported response type," but the test contradicts that. Whichever way you go, the interface, the doc examples, and the test should all agree.

🧹 Same concern applies to getIdToken's typing (minor)

sso.ts:26 return axios.get(url) also relies on the any return being assignable to Promise<string>. It happens to be correct at runtime, but it's correct by coincidence rather than by a checked type. Adding axios.get<string>(url) wouldn't type-check against the interceptor either — so this is really the recurring "interceptor unwraps data" design gap. Not a blocker, just worth noting that the safety here is incidental.

🔒 Security

  • No token logging; tokens returned to caller only. ✅
  • Service-role + on-behalf-of auth covered by a dedicated header-matching test (sso.test.ts:62). ✅
  • Minor (optional): neither getAccessToken nor getIdToken validates userid. connectors.ts:32 guards its param and throws early; an empty/undefined userid would build .../idtoken/undefined. Low severity, cheap for parity.

⚡ Performance

No concerns — single GET each.

🧪 Test Coverage

Good. Only gaps are cosmetic:

  • The getAccessToken mock/type inconsistency above (a correctness gap, not a coverage gap).
  • Optional: an error-path test for getAccessToken mirroring the getIdToken 404 test.

✨ Nits

  • Consider a shared constant for the URL paths to avoid drift between module and tests (still applies, purely cosmetic).

Bottom line: functionally this works and is well-tested, but the getAccessToken interface/doc/runtime disagreement is still unresolved — it's just no longer visible to the compiler. Reconciling the type, the two doc examples, and the test against the actual backend shape is the one thing I'd resolve before merge.


View job • Branch: getIdToken

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @natansil's task —— View job


PR Review: Add SSO OIDC ID token retrieval

  • Read changed files (sso.ts, sso.types.ts, sso.test.ts)
  • Review related context (axios-client.ts, connectors.ts)
  • Analyze code quality, bugs, security, tests
  • Post review feedback

The implementation is clean and minimal — both methods are now just return axios.get(url), delegating the return shape to the interface types. The test suite is thorough. Already approved by @DolevEpshtein. My re-review of the current state:

✅ Strengths

  • Very small surface area; getIdToken mirrors the existing getAccessToken pattern. 👍
  • Strong tests: happy path, raw-string response, service-role + on-behalf-of header matching (sso.test.ts:62), and a 404 error path asserting Base44Error/status/code (sso.test.ts:79). 👍
  • No token logging; tokens are returned to the caller only. ✅

🐞 Correctness — the getAccessToken type/runtime mismatch persists

This has come up in prior passes and is still present. It's the one substantive item:

  • Runtime: the response interceptor returns response.data (axios-client.ts:238), so axios.get(url) resolves to the response body. The test locks this in as a raw string: sso.test.ts:58 asserts expect(accessToken).toBe("access-token-123") against a body of JSON.stringify("access-token-123").
  • Declared type: getAccessToken(userid): Promise<SsoAccessTokenResponse> where SsoAccessTokenResponse = { access_token: string } (sso.types.ts:5,44).

Because sso.ts:20 calls axios.get(url) with no type param, it returns AxiosResponse<any>, and any is assignable to Promise<SsoAccessTokenResponse> — so the compiler is happy while the runtime value is actually a string. A consumer trusting the type would write response.access_token and get undefined. The doc examples reinforce the wrong shape:

  • sso.types.ts:24console.log(response.data.access_token)
  • sso.types.ts:41console.log(response.access_token)

Both would print undefined given the raw-string behavior the test asserts.

Recommend picking one source of truth against the real backend contract and making the interface, doc examples, and test agree:

  • If it returns a raw string (as the test says): change the signature to Promise<string> to match getIdToken, drop SsoAccessTokenResponse, and fix both doc examples. Cleanest option.
  • If it returns { access_token }: update the test mock to JSON.stringify({ access_token: rawAccessToken }), assert accessToken.access_token, and add axios.get<SsoAccessTokenResponse>(url).

Note this is a pre-existing wart shared with connectors.ts, which chose to make it explicit via @ts-expect-error (connectors.ts:28,40) rather than hide it behind an untyped axios.get. Since the PR touches getAccessToken anyway, reconciling it now would be worthwhile — but it's not a regression introduced here.

🧹 getIdToken typing is correct-by-coincidence (minor)

sso.ts:26 return axios.get(url) relies on the any return being assignable to Promise<string>. It's right at runtime, but not because the type checker verified it — same underlying "interceptor unwraps data" gap. Not a blocker.

🔒 Security

  • Service-role + on-behalf-of auth covered by a dedicated header-matching test. ✅
  • Minor (optional): neither method validates userid. connectors.ts:32 guards its param and throws early; an empty/undefined userid would build .../idtoken/undefined. Low severity, cheap for parity.

⚡ Performance

No concerns — single GET each.

🧪 Test Coverage

Solid. Only optional gap: an error-path test for getAccessToken mirroring the getIdToken 404 test.

✨ Nits

  • Consider a shared constant for the URL paths to avoid drift between module and tests (purely cosmetic).

Bottom line: functionally correct and well-tested. The only thing I'd flag is the getAccessToken interface/doc/runtime disagreement — it's pre-existing and now silently type-checks, so it's not a merge blocker for this PR, but aligning the type, the two doc examples, and the test against the real backend shape would remove a real footgun for consumers.


View job • Branch: getIdToken

@natansil
natansil merged commit 0a19af1 into main Jul 21, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-draft PR has auto-drafted documentation suggestions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants