Repository navigation
fix(identity): generalize #2730 fix — route all user-scoped handlers through tiered resolver - #2736
Merged
Merged
Conversation
…2730) Task 2: replace the old resolveUserSapId + sapId-only SELECT pattern in the 9 bucket-A READ handlers with resolveDbUserScoped, so null-sapId IAS/social/GitHub users (whose identity is tied to a UserIdentities (iss,sub) link, not a sapId) see their own data instead of empty/zero fallbacks. Handlers converted: getProgress, findTaskProgressByUserAndTasksIds, countCompletedMissionsTotal, countCompletedMissionsPercent, getEventProgress, getAppSpaceProgress, before('READ','TaskRecords'), before('READ','LearningPreferences'), getKhorosProfile. The two before('READ') filters preserve their fail-closed req.query.where predicate on miss. getAppSpaceProgress.authenticated now uses Boolean(dbUser) instead of Boolean(sapId) (#2314 intent preserved). Also adds a fallback in srv/lib/resolve-db-user.js to load resolveDbUserScoped and getOrProvisionDbUser directly from packages/core when the workspace @tutorials/core node_modules copy predates these helpers (worktree isolation). TDD: new test/unit/developer-service-identity-reads.test.js covers all 9 handlers with a null-sapId identity-link user; 6/9 failed pre-fix, all 9 pass post-fix.
…; revert runtime hack (#2730) Fix-round 1 for Task 2. The runtime fallback in srv/lib/resolve-db-user.js (importing directly from ../../packages/core/) masked a missing barrel export: resolveDbUserScoped and getOrProvisionDbUser were added to packages/core/resolve-db-user.js in Task 1 but never added to the packages/core barrel (index.js). This means: - @tutorials/core subpath imports returned undefined for these two functions - esbuild's core.bundle.mjs (built from index.js) omitted them entirely - On CF deploy (where packages/ is absent), every converted handler would have thrown "resolveDbUserScoped is not a function" Fix: 1. Add both names to the barrel export line in packages/core/index.js 2. Revert srv/lib/resolve-db-user.js to plain mod.resolveDbUserScoped / mod.getOrProvisionDbUser — the bundle now has them, so no workaround needed 3. Regenerated all three core bundles (srv/lib/_shared/, srv-qa/lib/_shared/, srv-mcp/lib/_shared/) — grep counts: resolveDbUserScoped=2, getOrProvisionDbUser=2 (bundles are gitignored; regenerate with `node scripts/bundle-shared.cjs`) Verification: `grep -c resolveDbUserScoped srv/lib/_shared/core.bundle.mjs` → 2 (was 0) All 11 tests pass: developer-service-identity-reads (9) + resolve-db-user-scoped (2).
… drop duplicated mint blocks (#2730) - doCompleteStep: replace sapId SELECT + hand-rolled mint with getOrProvisionDbUser - doResetTutorialProgress: same; preserve no-op-on-miss (returns { newAttemptNumber:1 }) - createTaskRecord: use resolveDbUserScoped (find-only legacy endpoint, 404 on miss) - setLearningPreferences: use getOrProvisionDbUser (removes sapId null guard + mint) - setPreferredEventRegion: same - setKhorosLink: use getOrProvisionDbUser inside the persist try-block; remove outer sapId guard - clearKhorosLink: use resolveDbUserScoped (clear-only, no-op on miss) Narrows hasToken in getOrProvisionDbUser to exclude emailFromUser(user)'s user.id fallback — a basic-auth user with email-shaped user.id (e.g. the #1614 overflow regression test) must NOT trip the provisionDbUser path. Only real OIDC signals (iss+sub or explicit attr.email/name) qualify as "token present". Also fixes the resolveDbUserScoped @PARAM docstring (was 'uses req.user + cds.tx(req)'; impl is 'threads req.tx when present, falls back to ambient otherwise'). TDD: test/unit/developer-service-identity-writes.test.js verifies: 1. Token user (iss+sub, null sapId) → updates existing identity-link row, no dup minted 2. setPreferredEventRegion, setKhorosLink, clearKhorosLink — no 401 for null-sapId user 3. Token-less MCP user (bare user.id, no authInfo/attr) → hasToken=false, sapId branch fires All 81 regression tests pass (reset/complete/khoros/mcp/khoros/puzzle suites).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Follow-up to #2730 / PR #2734 (the
getMyFavoritesfix, already merged to DEV). That fix revealed a systemic pattern: ~16 user-scoped handlers insrv/developer-service.jsresolved the DB user viaresolveUserSapId+SELECT where({ sapId })only. For social / IAS / GitHub logins (the Devtoberfest/TechEd audience),Users.sapIdis NULL — identity lives in the(issuer, subject)link (UserIdentities, #2641 / GitHub OIDC shim). So every one of these handlers silently returned empty/zero (reads) or 401/duplicate-row (writes) for those users. Favorites was just the first symptom.Fix
Two reusable helpers in
packages/core/resolve-db-user.js, and every user-scoped site routed through them:resolveDbUserScoped(req, cols?)— read-only tiered resolve (pin →(iss,sub)link → sapId). 9 reads:getProgress,findTaskProgressByUserAndTasksIds,countCompletedMissionsTotal/Percent,getEventProgress,getAppSpaceProgress, the twobefore('READ')row filters (TaskRecords, LearningPreferences — fail-closed preserved),getKhorosProfile.getOrProvisionDbUser(req, {sapId}, cols?)— write get-or-create; prefers tieredprovisionDbUserwhen a token is present, falls back to the sapId path only for the token-less MCP*Forpath. 7 writes + thedoCompleteStep/doResetTutorialProgressshared helpers; deletes ~4 duplicated hand-rolled "mint a Users row" blocks (net −63 lines in developer-service.js).Both helpers added to the
@tutorials/corebarrel so they land in the production_sharedbundle (regenerated at deploy viabuild:all → prebuild:shared-bundle).Why so many places (root cause of the pattern)
resolveUserSapIdis a claim extractor, reused as if it were a user resolver. The real resolver (provisionDbUser/resolveUser) arrived with the IAS migration (#2641) butdeveloper-service.jswas never swept. This PR completes that sweep and centralizes the pattern so it can't drift back.Testing
(iss,sub)identity-link users through the real handlers viasrv.tx: reads return the user's data (not empty), writes land on the canonicalUsers.IDwith no duplicate row, MCP token-less path still uses the sapId branch.hasTokennarrowed to drop theemailFromUser(user.id)false-positive (it was minting duplicate null-sapId rows for legacy sapId-keyed users). Verified safe: every real OIDC/IAS/GitHub-shim login carriesiss+sub; only basic-auth/MCP service users (which must take the sapId branch) are affected.Scope notes
origin/DEV. Not touched here.attr.dbUserId; a NULL-sapId PAT reaching a non-*Forwrite handler would mint a spurioussapId=emailrow. No PAT write tool routes there today; worth a tracking issue when PAT write tools land.Plan + full review ledger:
docs/superpowers/plans/2026-10-12-identity-resolution-fix.md.🤖 Generated with Claude Code