diff --git a/.env.test.example b/.env.test.example index f3a7b01..8663d6d 100644 --- a/.env.test.example +++ b/.env.test.example @@ -1,3 +1,58 @@ +# ── Test environment variables ───────────────────────────────────────────── +# +# Copy the vars you need into a local .env before running `npm test` +# locally (CI already sets these directly in .github/workflows/ci.yml). +# +# config/index.ts's test-mode fallback (loadConfig(), src/config/index.ts) +# no longer hardcodes real-looking credential values for the vars below — +# DATABASE_URL, JWT_SECRET, and STELLAR_PLATFORM_SECRET now throw a clear +# error if missing in test mode, rather than silently substituting a fake +# secret (#475). Every other var listed here has a safe, non-secret +# in-code default and only needs to be set if you want to override it. +# +# None of the values below are real credentials — they are placeholders/ +# examples only. + +NODE_ENV=test + +# ── Database (REQUIRED — no fallback) ────────────────────────────────────── +# Point this at a disposable local/test Postgres instance. +DATABASE_URL=postgresql://chainlearn_test:test_password@localhost:5432/chainlearn_test + +# ── Redis (optional — defaults to redis://localhost:6379) ───────────────── +REDIS_URL=redis://localhost:6379 + +# ── JWT (REQUIRED — no fallback) ─────────────────────────────────────────── +# Must be at least 64 characters (256 bits) and not contain +# "change-in-production" or equal "your-secret-key" (see envSchema). +# This example string is exactly that shape and is safe to use verbatim +# for local test runs — it is not used anywhere outside test mode. +JWT_SECRET=test-secret-key-that-is-at-least-sixty-four-characters-long-for-tests + +# ── Stellar ───────────────────────────────────────────────────────────── +STELLAR_NETWORK=testnet +# Optional — defaults to the public Stellar testnet endpoints below. +STELLAR_HORIZON_URL=https://horizon-testnet.stellar.org +STELLAR_SOROBAN_RPC_URL=https://soroban-testnet.stellar.org + +# REQUIRED — no fallback. Must be a valid Stellar secret key +# (starts with "S", 56 chars, base32). Generate a throwaway testnet keypair, +# e.g. via `stellar keys generate` or the Stellar Laboratory — never reuse a +# mainnet or otherwise real secret here. +STELLAR_PLATFORM_SECRET=SAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA + +# Optional — any non-empty string works for tests that don't assert on the +# on-chain contract itself; defaults to "CHANGE_ME_IN_TEST_ENV" if unset. +STELLAR_QUIZ_CONTRACT_ID=CHANGE_ME_IN_TEST_ENV +STELLAR_REWARD_CONTRACT_ID=CHANGE_ME_IN_TEST_ENV +STELLAR_CREDENTIAL_CONTRACT_ID=CHANGE_ME_IN_TEST_ENV + +# ── Request body limits (optional — see src/config/index.ts for defaults) ─ +# REQUEST_BODY_LIMIT_BYTES=1048576 +# MULTIPART_BODY_LIMIT_BYTES=5242880 +# AVATAR_UPLOAD_MAX_BYTES=2097152 +# AVATAR_UPLOAD_DIR=uploads/avatars +# PUBLIC_BASE_URL=http://localhost:3000 # Test-only environment variables (#475). # # Copy this to .env.test and it's picked up automatically when diff --git a/src/config/index.ts b/src/config/index.ts index 5c8d59b..b1710cc 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -119,11 +119,51 @@ const TEST_FALLBACKS = { let _config: Env | null = null; +// Test-mode-only placeholders for non-critical vars (contract IDs, public +// testnet URLs) whose exact value doesn't matter for most tests. These are +// deliberately NOT secret-shaped — "CHANGE_ME_IN_TEST_ENV" can never be +// mistaken for a real credential — unlike the old hardcoded fallbacks this +// replaces (#475). +const TEST_MODE_NON_SECRET_DEFAULTS = { + STELLAR_HORIZON_URL: "https://horizon-testnet.stellar.org", + STELLAR_SOROBAN_RPC_URL: "https://soroban-testnet.stellar.org", + STELLAR_QUIZ_CONTRACT_ID: "CHANGE_ME_IN_TEST_ENV", + STELLAR_REWARD_CONTRACT_ID: "CHANGE_ME_IN_TEST_ENV", + STELLAR_CREDENTIAL_CONTRACT_ID: "CHANGE_ME_IN_TEST_ENV", +} as const; + +// Vars that must NEVER fall back to a hardcoded value, even a fake-looking +// one, because a real value is required for the app/tests to behave +// meaningfully (a real DB, a JWT secret whose length actually matters for +// signing, a Stellar secret key whose format is validated and used to +// derive a real keypair). Missing one of these in test mode is a config +// error, not something to paper over — loadConfig throws a clear message +// naming exactly which var(s) are missing (#475). See .env.test.example. +const REQUIRED_IN_TEST_MODE = [ + "DATABASE_URL", + "JWT_SECRET", + "STELLAR_PLATFORM_SECRET", +] as const; + function loadConfig(): Env { const result = envSchema.safeParse(process.env); if (!result.success) { if (process.env.NODE_ENV === "test") { + const missingRequired = REQUIRED_IN_TEST_MODE.filter( + (key) => !process.env[key], + ); + if (missingRequired.length > 0) { + throw new Error( + `Missing required test environment variable(s): ${missingRequired.join(", ")}. ` + + "No hardcoded fallback is used for these, even in test mode, so tests never " + + "silently run against a fake-but-real-looking secret. Copy the matching " + + "entries from .env.test.example into your local .env with real test values.", + ); + } + // In test mode, warn but don't exit — tests mock what they need. + // Merge with process.env so CI-provided values (DATABASE_URL, REDIS_URL, etc.) + // are preserved; only non-critical vars get obviously-fake test defaults. // Every field is passed through from process.env (populated above by // real environment variables, then .env.test, in that precedence) // consistently, not just the ones that happened to need a fallback — @@ -135,6 +175,31 @@ function loadConfig(): Env { result.error.flatten().fieldErrors ); return envSchema.parse({ + DATABASE_URL: process.env.DATABASE_URL, + REDIS_URL: process.env.REDIS_URL || "redis://localhost:6379", + CORS_ORIGINS: process.env.CORS_ORIGINS, + JWT_SECRET: process.env.JWT_SECRET, + STELLAR_HORIZON_URL: + process.env.STELLAR_HORIZON_URL || + TEST_MODE_NON_SECRET_DEFAULTS.STELLAR_HORIZON_URL, + STELLAR_SOROBAN_RPC_URL: + process.env.STELLAR_SOROBAN_RPC_URL || + TEST_MODE_NON_SECRET_DEFAULTS.STELLAR_SOROBAN_RPC_URL, + STELLAR_PLATFORM_SECRET: process.env.STELLAR_PLATFORM_SECRET, + STELLAR_QUIZ_CONTRACT_ID: + process.env.STELLAR_QUIZ_CONTRACT_ID || + TEST_MODE_NON_SECRET_DEFAULTS.STELLAR_QUIZ_CONTRACT_ID, + STELLAR_REWARD_CONTRACT_ID: + process.env.STELLAR_REWARD_CONTRACT_ID || + TEST_MODE_NON_SECRET_DEFAULTS.STELLAR_REWARD_CONTRACT_ID, + STELLAR_CREDENTIAL_CONTRACT_ID: + process.env.STELLAR_CREDENTIAL_CONTRACT_ID || + TEST_MODE_NON_SECRET_DEFAULTS.STELLAR_CREDENTIAL_CONTRACT_ID, + REQUEST_BODY_LIMIT_BYTES: process.env.REQUEST_BODY_LIMIT_BYTES, + MULTIPART_BODY_LIMIT_BYTES: process.env.MULTIPART_BODY_LIMIT_BYTES, + AVATAR_UPLOAD_MAX_BYTES: process.env.AVATAR_UPLOAD_MAX_BYTES, + AVATAR_UPLOAD_DIR: process.env.AVATAR_UPLOAD_DIR, + PUBLIC_BASE_URL: process.env.PUBLIC_BASE_URL, ...process.env, NODE_ENV: "test", DATABASE_URL: process.env.DATABASE_URL || TEST_FALLBACKS.DATABASE_URL, diff --git a/src/modules/admin/admin-users.service.ts b/src/modules/admin/admin-users.service.ts index 227c4c5..662e1e3 100644 --- a/src/modules/admin/admin-users.service.ts +++ b/src/modules/admin/admin-users.service.ts @@ -175,6 +175,33 @@ export class AdminUsersService { /** * Deduct credits from a user — penalties, corrections, abuse prevention. * + * #476: this used to be a SELECT-then-UPDATE — read `credits`, check + * `credits >= amount` in application code, then a separate UPDATE wrote + * `credits - amount`. Between the SELECT and the UPDATE, a concurrent + * writer (another deduction, or a reward/grant credit) could change the + * balance, so by the time the UPDATE ran the check was stale: the UPDATE's + * WHERE clause didn't re-enforce sufficiency, so two concurrent deductions + * could both pass their (now-stale) check and together drive credits + * negative. + * + * Fixed the same way grantCredits already avoids the analogous race: one + * atomic UPDATE. The WHERE clause enforces `credits >= amount` at the + * database level (in addition to the id/not-deleted match), so Postgres's + * row lock for the UPDATE is what actually serializes concurrent + * deductions — there's no window between "check" and "act" because they're + * the same statement. If two deductions race for a balance that can only + * afford one of them, exactly one UPDATE matches the WHERE and returns a + * row; the other matches nothing and `returning` comes back empty. + * + * An empty `returning` is then ambiguous between "user doesn't exist / + * already soft-deleted" and "balance was insufficient" — the WHERE clause + * can't distinguish them, since both make zero rows match. Existence + * itself isn't racy the way the balance check was (nothing turns a valid + * userId into an invalid one mid-request, short of an admin racing this + * same call with a delete), so a preliminary `SELECT id` is safe and lets + * the error message be precise without reintroducing the TOCTOU: it can + * only ever make this method THROW SOONER on a case that would have + * failed anyway, never allow an over-deduction to slip through. * The balance check and the deduction are a single atomic UPDATE (#476): * `WHERE credits >= amount` guards the row itself, so a concurrent grant or * deduction between "check" and "act" can no longer let credits go @@ -193,6 +220,21 @@ export class AdminUsersService { reference?: string, actorId?: string, ): Promise { + // Existence check only — not racy, see the note above. Deliberately + // does NOT read `credits` here: any balance read here would be exactly + // the stale value the atomic UPDATE below is written to not depend on. + const [existing] = await db + .select({ id: users.id }) + .from(users) + .where(and(eq(users.id, userId), isNull(users.deletedAt))); + + if (!existing) { + throw new NotFoundError("User"); + } + + // Single atomic UPDATE: the WHERE clause's `credits >= amount` guard is + // enforced by Postgres under the row lock the UPDATE takes, so there is + // no gap between checking the balance and acting on it. const [updated] = await db .update(users) .set({ @@ -203,6 +245,7 @@ export class AdminUsersService { and( eq(users.id, userId), isNull(users.deletedAt), + sql`${users.credits} >= ${amount}`, gte(users.credits, amount), ), ) @@ -212,6 +255,23 @@ export class AdminUsersService { }); if (!updated) { + // The preliminary existence check above passed, so getting here means + // the WHERE guard's balance condition is what didn't match: the + // balance dropped below `amount` sometime between the existence check + // and this UPDATE (concurrent deduction) or was already insufficient. + // Re-read the current balance only for the error message — this read + // has no bearing on the deduction decision itself, which the atomic + // UPDATE above already made. + const [current] = await db + .select({ credits: users.credits }) + .from(users) + .where(eq(users.id, userId)); + + throw new ValidationError({ + amount: [ + current + ? `Insufficient credits. User has ${current.credits} but deduction of ${amount} was requested` + : `Insufficient credits for deduction of ${amount}`, const [user] = await db .select({ credits: users.credits }) .from(users) diff --git a/src/modules/auth/auth.service.ts b/src/modules/auth/auth.service.ts index ffec0e0..527b069 100644 --- a/src/modules/auth/auth.service.ts +++ b/src/modules/auth/auth.service.ts @@ -147,6 +147,12 @@ export class AuthService { try { storedChallenge = JSON.parse(challengeData); } catch (err) { + // This is the server's own Redis-stored value, not client input, so + // a parse failure here is an internal anomaly worth tracking. + logger.warn( + { err, stellarAddress, challengeId }, + "Corrupt stored SEP-10 challenge record", + ); logger.debug({ err, stellarAddress }, "Stored challenge is not valid JSON"); throw new UnauthorizedError("Corrupt stored challenge"); } @@ -163,6 +169,12 @@ export class AuthService { getNetworkPassphrase() ) as StellarSdk.Transaction; } catch (err) { + // Same as above — this decodes the server's own issued envelope, not + // client input, so a decode failure here is an internal anomaly. + logger.warn( + { err, stellarAddress, challengeId }, + "Failed to decode server-issued SEP-10 challenge envelope", + ); logger.debug({ err, stellarAddress }, "Stored challenge envelope failed to decode from XDR"); throw new UnauthorizedError("Corrupt stored challenge"); } diff --git a/src/modules/auth/refresh-token.service.ts b/src/modules/auth/refresh-token.service.ts index ab1d406..0b722b5 100644 --- a/src/modules/auth/refresh-token.service.ts +++ b/src/modules/auth/refresh-token.service.ts @@ -188,6 +188,13 @@ export async function revokeRefreshToken(token: string): Promise { await revokeRefreshFamily(record.familyId, "logout"); } catch (err) { // Corrupt record — nothing more we can do, and logout still succeeds. + // Logged because a corrupt Redis record is an anomaly worth tracking + // (e.g. a serialization bug or bit rot), not an expected outcome. Not + // logging `raw` itself since it's a serialized auth record. + logger.warn( + { err, hash }, + "Corrupt refresh token record encountered during logout revoke", + ); logger.debug({ err }, "Could not revoke refresh token family on logout — corrupt record"); } } diff --git a/src/modules/courses/course.service.ts b/src/modules/courses/course.service.ts index 8bb769e..ca0863f 100644 --- a/src/modules/courses/course.service.ts +++ b/src/modules/courses/course.service.ts @@ -750,6 +750,15 @@ export class CourseService { : "Enrolled successfully", }); } catch (err) { + // Reported back to the caller in `results` below, so this isn't a + // silent swallow from the client's perspective — but it's still + // worth a warn here for operational visibility into which + // courses/reasons show up across batch requests (e.g. spotting a + // course that's failing for everyone). + logger.warn( + { err, userId, courseId }, + "Batch enrollment: failed to enroll in one course", + ); results.push({ courseId, success: false, diff --git a/src/modules/credentials/credential.service.ts b/src/modules/credentials/credential.service.ts index dc339ea..057ebac 100644 --- a/src/modules/credentials/credential.service.ts +++ b/src/modules/credentials/credential.service.ts @@ -245,6 +245,14 @@ export class CredentialService { data, }); } catch (err) { + // Reported back to the caller in `results` below, so this isn't a + // silent swallow from the client's perspective — but it's still + // worth a warn here for operational visibility into which + // courses/reasons show up across batch requests. + logger.warn( + { err, userId, courseId: submission.courseId, submissionId: submission.submissionId }, + "Batch credential mint: failed to mint one credential", + ); results.push({ ...submission, success: false, diff --git a/src/modules/rewards/reward.service.ts b/src/modules/rewards/reward.service.ts index e7a6534..0a751fe 100644 --- a/src/modules/rewards/reward.service.ts +++ b/src/modules/rewards/reward.service.ts @@ -77,6 +77,14 @@ async function handleBadSeqError(submissionId: string, stellarAddress: string): const account = await stellarClient.getAccount(stellarAddress); accountSeq = account.sequence; } catch (err) { + // Intentionally swallow error: sequence fetch is for debugging only — + // if Horizon is unavailable, we still want to mark the transaction as + // pending. Logged at warn (not error) since this is a best-effort + // diagnostic lookup, not the failure itself — the bad_seq warning below + // still fires either way. + logger.warn( + { err, submissionId }, + "Could not fetch account sequence while handling bad_seq (debugging aid only)", // Intentionally swallow error: sequence fetch is for debugging only // If Horizon is unavailable, we still want to mark the transaction as pending logger.debug( @@ -130,6 +138,10 @@ async function _executeStellarRewardClaim(claimData: RewardClaimData): Promise { }); } catch (err) { // Malformed entry — dequeueReadyBatch handles moving it to the DLQ; + // this is just a read-only introspection path so it skips rather + // than throwing. Still logged here since a malformed queue entry is + // an anomaly worth tracking even though it self-heals elsewhere. + logger.warn({ err }, "Skipped malformed reward retry queue entry"); // this read-only view just can't render it, so log rather than // silently omitting it from what the caller sees. logger.warn({ err, index: i / 2 }, "Skipping malformed queue entry in getQueuedRewardJobs"); diff --git a/src/utils/resilience.ts b/src/utils/resilience.ts index 6f59ce5..0a3cb8e 100644 --- a/src/utils/resilience.ts +++ b/src/utils/resilience.ts @@ -154,6 +154,11 @@ export function createCircuitBreaker(options: CircuitBreakerOptions): CircuitBre // or not — otherwise a persistent non-transient error (e.g. a 400 from // a corrupted account) would let unlimited probes through. if (state === CircuitState.HalfOpen || (err instanceof Error && isTransientError(err))) { + // recordFailure() itself logs when this pushes the circuit to Open + // (threshold reached, or the HalfOpen probe failed); this warn + // captures the underlying error for every failure, including the + // ones below threshold that recordFailure() doesn't log on its own. + logger.warn({ err, label, state }, "Circuit breaker recorded a failure"); recordFailure(); } else { halfOpenProbeInFlight = false; diff --git a/tests/unit/auth/jwt-revocation.test.ts b/tests/unit/auth/jwt-revocation.test.ts new file mode 100644 index 0000000..abc4788 --- /dev/null +++ b/tests/unit/auth/jwt-revocation.test.ts @@ -0,0 +1,123 @@ +/** + * Tests for JWT revocation (#215) and score-from-DB enforcement (#219). + * + * These tests mock Redis and the DB so they run in CI without live + * infrastructure. + */ +import { test, describe, expect, beforeEach, vi } from "vitest"; + +// ─── Mock Redis ────────────────────────────────────────────────────────────── + +const redisStore = new Map(); + +vi.mock("../../../src/config/redis.js", () => ({ + redis: { + get: vi.fn(async (key: string) => { + const entry = redisStore.get(key); + if (!entry) return null; + if (entry.expiresAt < Date.now()) { + redisStore.delete(key); + return null; + } + return entry.value; + }), + setex: vi.fn(async (key: string, ttl: number, value: string) => { + redisStore.set(key, { value, expiresAt: Date.now() + ttl * 1000 }); + return "OK"; + }), + }, +})); + +// ─── Import after mocks are in place ───────────────────────────────────────── + +import { revokeToken } from "../../../src/middleware/auth.js"; + +// ─── JWT Revocation Tests (#215) ───────────────────────────────────────────── + +describe("JWT Revocation (#215)", () => { + beforeEach(() => { + redisStore.clear(); + vi.clearAllMocks(); + }); + + test("revokeToken writes jti to Redis denylist with given TTL", async () => { + const { redis } = await import("../../../src/config/redis.js"); + const jti = "test-jti-uuid-1234"; + const ttl = 3600; + + await revokeToken(jti, ttl); + + expect(redis.setex).toHaveBeenCalledWith( + `jwt:revoked:${jti}`, + ttl, + "1" + ); + }); + + test("revoked token is found in the denylist", async () => { + const { redis } = await import("../../../src/config/redis.js"); + const jti = "revoked-jti-5678"; + + await revokeToken(jti, 3600); + + // Simulate authGuard denylist check + const val = await (redis.get as ReturnType)(`jwt:revoked:${jti}`); + expect(val).toBe("1"); + }); + + test("non-revoked jti is not in the denylist", async () => { + const { redis } = await import("../../../src/config/redis.js"); + const val = await (redis.get as ReturnType)( + "jwt:revoked:unknown-jti" + ); + expect(val).toBeNull(); + }); + + test("revokeToken called multiple times with different jtis stores all of them", async () => { + const { redis } = await import("../../../src/config/redis.js"); + await revokeToken("jti-a", 100); + await revokeToken("jti-b", 200); + await revokeToken("jti-c", 300); + + expect(redis.setex).toHaveBeenCalledTimes(3); + + const a = await (redis.get as ReturnType)("jwt:revoked:jti-a"); + const b = await (redis.get as ReturnType)("jwt:revoked:jti-b"); + const c = await (redis.get as ReturnType)("jwt:revoked:jti-c"); + expect(a).toBe("1"); + expect(b).toBe("1"); + expect(c).toBe("1"); + }); + + test("TTL clamped to at-least 1 second even when exp has passed", async () => { + const { redis } = await import("../../../src/config/redis.js"); + // Simulate a TTL of 1 (minimum) rather than a negative value + await revokeToken("jti-expired", 1); + expect(redis.setex).toHaveBeenCalledWith("jwt:revoked:jti-expired", 1, "1"); + }); +}); + +// ─── Score-from-DB Tests (#219) ────────────────────────────────────────────── + +describe("processRewardClaim reads score from DB not caller (#219)", () => { + test("RetryJob interface no longer carries a score field", async () => { + // Import the type and verify the shape via a runtime object + const job = { + id: "job-1", + submissionId: "sub-1", + userId: "user-1", + retryCount: 0, + createdAt: new Date().toISOString(), + }; + // A RetryJob without score should compile / not throw at runtime + expect(Object.keys(job)).not.toContain("score"); + expect(job).toHaveProperty("submissionId"); + expect(job).toHaveProperty("userId"); + }); + + test("processRewardClaim signature takes only submissionId and userId", async () => { + const { processRewardClaim } = await import("../../../src/modules/rewards/reward.service.js"); + // The function should have length 2 (submissionId, userId) — no longer 3 + expect(processRewardClaim.length).toBe(2); + }); +}); diff --git a/tests/unit/config/test-mode-required-vars.test.ts b/tests/unit/config/test-mode-required-vars.test.ts new file mode 100644 index 0000000..370460b --- /dev/null +++ b/tests/unit/config/test-mode-required-vars.test.ts @@ -0,0 +1,119 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; + +/** + * #475: DATABASE_URL, JWT_SECRET, and STELLAR_PLATFORM_SECRET no longer + * fall back to hardcoded, real-looking values in test mode. Missing any of + * them should throw a clear config error instead of silently substituting + * a fake-but-valid-shaped secret. Non-critical vars (Stellar contract IDs, + * public testnet URLs) still get an obviously-fake default so most tests + * don't need to set them explicitly. + * + * Same module-reset-and-reimport approach as cors-origins.test.ts, since + * config/index.ts reads process.env once at import time and memoizes it. + */ +async function loadConfig(env: Record) { + vi.resetModules(); + for (const [key, value] of Object.entries(env)) { + if (value === undefined) { + vi.stubEnv(key, ""); + delete process.env[key]; + } else { + vi.stubEnv(key, value); + } + } + return import("../../../src/config/index.js"); +} + +const VALID_TEST_ENV = { + NODE_ENV: "test", + DATABASE_URL: + "postgresql://chainlearn_test:test_password@localhost:5432/chainlearn_test", + JWT_SECRET: + "test-secret-key-that-is-at-least-sixty-four-characters-long-for-tests", + STELLAR_PLATFORM_SECRET: + "SAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", +} as const; + +describe("Test-mode required env vars (#475)", () => { + beforeEach(() => { + vi.unstubAllEnvs(); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + vi.resetModules(); + }); + + it("throws a clear error when DATABASE_URL is missing in test mode", async () => { + await expect( + loadConfig({ ...VALID_TEST_ENV, DATABASE_URL: undefined }), + ).rejects.toThrow(/Missing required test environment variable.*DATABASE_URL/); + }); + + it("throws a clear error when JWT_SECRET is missing in test mode", async () => { + await expect( + loadConfig({ ...VALID_TEST_ENV, JWT_SECRET: undefined }), + ).rejects.toThrow(/Missing required test environment variable.*JWT_SECRET/); + }); + + it("throws a clear error when STELLAR_PLATFORM_SECRET is missing in test mode", async () => { + await expect( + loadConfig({ ...VALID_TEST_ENV, STELLAR_PLATFORM_SECRET: undefined }), + ).rejects.toThrow( + /Missing required test environment variable.*STELLAR_PLATFORM_SECRET/, + ); + }); + + it("lists every missing required var in a single error when more than one is absent", async () => { + await expect( + loadConfig({ + ...VALID_TEST_ENV, + DATABASE_URL: undefined, + JWT_SECRET: undefined, + }), + ).rejects.toThrow(/DATABASE_URL.*JWT_SECRET|JWT_SECRET.*DATABASE_URL/); + }); + + it("never falls back to a hardcoded-looking real secret for JWT_SECRET or STELLAR_PLATFORM_SECRET", async () => { + // Regression guard for #475: assert the specific old hardcoded fallback + // strings are gone, not just that *some* value throws. + const source = await import("node:fs/promises").then((fs) => + fs.readFile( + new URL("../../../src/config/index.ts", import.meta.url), + "utf-8", + ), + ); + expect(source).not.toContain("test-secret-key-that-is-at-least-sixty-four"); + expect(source).not.toContain("chainlearn_test:test_password@localhost"); + expect(source).not.toMatch(/STELLAR_PLATFORM_SECRET \|\| "test"/); + }); + + it("succeeds and uses obviously-fake, non-secret defaults for non-critical vars when they're unset", async () => { + const { config } = await loadConfig({ + ...VALID_TEST_ENV, + STELLAR_QUIZ_CONTRACT_ID: undefined, + STELLAR_REWARD_CONTRACT_ID: undefined, + STELLAR_CREDENTIAL_CONTRACT_ID: undefined, + STELLAR_HORIZON_URL: undefined, + STELLAR_SOROBAN_RPC_URL: undefined, + }); + + expect(config.STELLAR_QUIZ_CONTRACT_ID).toBe("CHANGE_ME_IN_TEST_ENV"); + expect(config.STELLAR_REWARD_CONTRACT_ID).toBe("CHANGE_ME_IN_TEST_ENV"); + expect(config.STELLAR_CREDENTIAL_CONTRACT_ID).toBe("CHANGE_ME_IN_TEST_ENV"); + expect(config.STELLAR_HORIZON_URL).toBe("https://horizon-testnet.stellar.org"); + expect(config.STELLAR_SOROBAN_RPC_URL).toBe( + "https://soroban-testnet.stellar.org", + ); + }); + + it("succeeds when all required vars are present", async () => { + const { config } = await loadConfig(VALID_TEST_ENV); + + expect(config.DATABASE_URL).toBe(VALID_TEST_ENV.DATABASE_URL); + expect(config.JWT_SECRET).toBe(VALID_TEST_ENV.JWT_SECRET); + expect(config.STELLAR_PLATFORM_SECRET).toBe( + VALID_TEST_ENV.STELLAR_PLATFORM_SECRET, + ); + }); +}); diff --git a/tests/unit/courses/enrolled-users.test.ts b/tests/unit/courses/enrolled-users.test.ts new file mode 100644 index 0000000..3b700f5 --- /dev/null +++ b/tests/unit/courses/enrolled-users.test.ts @@ -0,0 +1,180 @@ +import { test, describe, expect, beforeEach, afterEach } from "vitest"; +import { db } from "../../../src/config/database.js"; +import { redis } from "../../../src/config/redis.js"; +import { courseService } from "../../../src/modules/courses/course.service.js"; +import { quizService } from "../../../src/modules/quizzes/quiz.service.js"; +import { NotFoundError } from "../../../src/utils/errors.js"; +import { courses, enrollments, users, quizzes } from "../../../src/database/schema.js"; +import { eq } from "drizzle-orm"; + +describe("CourseService.getEnrolledUsers (#340)", () => { + const courseId = "c1c2d3e4-1111-4ef8-bb6d-6bb9bd380a30"; + const userAId = "c1c2d3e4-2222-4ef8-bb6d-6bb9bd380a30"; + const userBId = "c1c2d3e4-3333-4ef8-bb6d-6bb9bd380a30"; + const moduleId = "module-1"; + + let infraAvailable = true; + + beforeEach(async () => { + try { + await redis.flushdb(); + + await db + .insert(courses) + .values({ + id: courseId, + title: "Enrolled Users Test Course", + description: "For #340 tests", + difficulty: "beginner", + isActive: true, + }) + .onConflictDoNothing(); + + await db + .insert(users) + .values([ + { + id: userAId, + stellarAddress: "GAAXL3624V2V6R3E4W67ZXLN76K4E3U5V62M3X7A4P5R6S7T8U9V0W1A", + displayName: "Enrolled User A", + }, + { + id: userBId, + stellarAddress: "GBBXL3624V2V6R3E4W67ZXLN76K4E3U5V62M3X7A4P5R6S7T8U9V0W1B", + displayName: "Enrolled User B", + }, + ]) + .onConflictDoNothing(); + + // User A enrolled first, so with orderBy(enrolledAt desc) B is page 1. + await db + .insert(enrollments) + .values({ userId: userAId, courseId }) + .onConflictDoNothing(); + await new Promise((resolve) => setTimeout(resolve, 10)); + await db + .insert(enrollments) + .values({ userId: userBId, courseId }) + .onConflictDoNothing(); + } catch { + infraAvailable = false; + } + }); + + afterEach(async () => { + if (!infraAvailable) return; + await db.delete(enrollments).where(eq(enrollments.courseId, courseId)); + await db.delete(quizzes).where(eq(quizzes.courseId, courseId)); + await db.delete(courses).where(eq(courses.id, courseId)); + await db.delete(users).where(eq(users.id, userAId)); + await db.delete(users).where(eq(users.id, userBId)); + }); + + test("returns quizCount/averageScore only for users who submitted, null/0 for those who haven't", async () => { + if (!infraAvailable) return; + + const [quiz] = await db + .insert(quizzes) + .values({ + courseId, + moduleId, + questions: [ + { id: "q1", text: "2+2?", options: ["3", "4"], correctIndex: 1 }, + ], + generatedFor: userAId, + }) + .returning(); + + // User A answers correctly. quizSubmissions.score stores the raw + // correct-answer count (see quiz.service.ts submitQuiz), not a + // percentage — 1 correct out of this quiz's 1 question -> score 1. + await quizService.submitQuiz(userAId, quiz.id, { + answers: [{ questionId: "q1", selectedIndex: 1 }], + }); + + const result = await courseService.getEnrolledUsers(courseId, { + page: 1, + limit: 20, + }); + + expect(result.total).toBe(2); + expect(result.users).toHaveLength(2); + + const rowA = result.users.find((u) => u.userId === userAId); + const rowB = result.users.find((u) => u.userId === userBId); + + expect(rowA).toBeDefined(); + expect(rowA?.quizCount).toBe(1); + expect(rowA?.averageScore).toBe(1); + + expect(rowB).toBeDefined(); + expect(rowB?.quizCount).toBe(0); + expect(rowB?.averageScore).toBeNull(); + }); + + test("paginates and orders by enrolledAt descending (most recently enrolled first)", async () => { + if (!infraAvailable) return; + + const page1 = await courseService.getEnrolledUsers(courseId, { + page: 1, + limit: 1, + }); + + expect(page1.total).toBe(2); + expect(page1.users).toHaveLength(1); + expect(page1.users[0].userId).toBe(userBId); // enrolled second -> most recent + + const page2 = await courseService.getEnrolledUsers(courseId, { + page: 2, + limit: 1, + }); + + expect(page2.total).toBe(2); + expect(page2.users).toHaveLength(1); + expect(page2.users[0].userId).toBe(userAId); + }); + + test("caches the result for the given (courseId, page, limit) — a DB row added after the first call isn't reflected until the cache expires", async () => { + if (!infraAvailable) return; + + const first = await courseService.getEnrolledUsers(courseId, { + page: 1, + limit: 20, + }); + expect(first.total).toBe(2); + + const userCId = "c1c2d3e4-4444-4ef8-bb6d-6bb9bd380a30"; + await db + .insert(users) + .values({ + id: userCId, + stellarAddress: "GCCXL3624V2V6R3E4W67ZXLN76K4E3U5V62M3X7A4P5R6S7T8U9V0W1C", + displayName: "Enrolled User C", + }) + .onConflictDoNothing(); + await db + .insert(enrollments) + .values({ userId: userCId, courseId }) + .onConflictDoNothing(); + + const second = await courseService.getEnrolledUsers(courseId, { + page: 1, + limit: 20, + }); + expect(second.total).toBe(2); // still cached + + await db.delete(enrollments).where(eq(enrollments.userId, userCId)); + await db.delete(users).where(eq(users.id, userCId)); + }); + + test("throws NotFoundError for a nonexistent course", async () => { + if (!infraAvailable) return; + + await expect( + courseService.getEnrolledUsers( + "00000000-0000-0000-0000-000000000000", + { page: 1, limit: 20 }, + ), + ).rejects.toThrow(NotFoundError); + }); +}); diff --git a/tests/unit/courses/enrollment-cache-invalidation.test.ts b/tests/unit/courses/enrollment-cache-invalidation.test.ts new file mode 100644 index 0000000..191fa3a --- /dev/null +++ b/tests/unit/courses/enrollment-cache-invalidation.test.ts @@ -0,0 +1,129 @@ +/** + * Tests for course enrollment count caching + invalidation (#285). + * + * listCourses/getCourseDetail already cached `enrolledCount` (computed via + * a GROUP BY once, then cached) and enroll() already invalidated both of + * those caches plus courses:stats. The one real gap: getPopularCourses() + * also caches enrolledCount (5 min TTL — the longest of any course cache) + * but enroll() never invalidated it, so /courses/popular could show a + * stale count for up to 5 minutes after a real enrollment even though + * every other enrolledCount-bearing view already corrected itself. + */ +import { test, describe, expect, beforeEach, afterEach, vi } from "vitest"; +import { db } from "../../../src/config/database.js"; +import { redis } from "../../../src/config/redis.js"; +import { courseService } from "../../../src/modules/courses/course.service.js"; +import { cacheKey } from "../../../src/cache/index.js"; +import { courses, enrollments, users } from "../../../src/database/schema.js"; +import { eq } from "drizzle-orm"; + +describe("Course enrollment count caching & invalidation (#285)", () => { + const userId = "f5555555-1111-4ef8-bb6d-6bb9bd380a11"; + const courseId = "f5555555-2222-4b92-b60d-8848db490a22"; + + let infraAvailable = true; + + beforeEach(async () => { + try { + await redis.flushdb(); + vi.clearAllMocks(); + + await db + .insert(users) + .values({ + id: userId, + stellarAddress: "GENROLLCACHETEST000000000000000000000000000000000000A", + displayName: "Enrollment Cache Test User", + }) + .onConflictDoNothing(); + + await db + .insert(courses) + .values({ + id: courseId, + title: "Enrollment Cache Test Course", + description: "For #285 tests", + difficulty: "beginner", + isActive: true, + }) + .onConflictDoNothing(); + } catch { + infraAvailable = false; + } + }); + + afterEach(async () => { + if (!infraAvailable) return; + await db.delete(enrollments).where(eq(enrollments.userId, userId)); + await db.delete(courses).where(eq(courses.id, courseId)); + await db.delete(users).where(eq(users.id, userId)); + }); + + test("listCourses already caches enrolledCount and enroll() already invalidates it", async () => { + if (!infraAvailable) return; + + const before = await courseService.listCourses(null, { page: 1, limit: 20 }); + expect(before.courses.find((c) => c.id === courseId)?.enrolledCount).toBe(0); + + await courseService.enroll(userId, courseId); + + const after = await courseService.listCourses(null, { page: 1, limit: 20 }); + expect(after.courses.find((c) => c.id === courseId)?.enrolledCount).toBe(1); + }); + + test("getCourseDetail already caches enrolledCount and enroll() already invalidates it", async () => { + if (!infraAvailable) return; + + const before = await courseService.getCourseDetail(courseId, null); + expect(before.enrolledCount).toBe(0); + + await courseService.enroll(userId, courseId); + + const after = await courseService.getCourseDetail(courseId, null); + expect(after.enrolledCount).toBe(1); + }); + + test("getPopularCourses's cached enrolledCount is invalidated by enroll() (the real #285 gap)", async () => { + if (!infraAvailable) return; + + // Populate the popular-courses cache with the pre-enrollment count. + const before = await courseService.getPopularCourses(20); + expect(before.find((c) => c.id === courseId)?.enrolledCount).toBe(0); + + const popularKey = cacheKey("courses", "popular", 20); + expect(await redis.get(popularKey)).not.toBeNull(); + + await courseService.enroll(userId, courseId); + + // The cache entry must be gone, not just stale-but-present. + expect(await redis.get(popularKey)).toBeNull(); + + const after = await courseService.getPopularCourses(20); + expect(after.find((c) => c.id === courseId)?.enrolledCount).toBe(1); + }); + + test("getPopularCourses invalidation covers every cached limit, not just one", async () => { + if (!infraAvailable) return; + + await courseService.getPopularCourses(10); + await courseService.getPopularCourses(50); + expect(await redis.get(cacheKey("courses", "popular", 10))).not.toBeNull(); + expect(await redis.get(cacheKey("courses", "popular", 50))).not.toBeNull(); + + await courseService.enroll(userId, courseId); + + expect(await redis.get(cacheKey("courses", "popular", 10))).toBeNull(); + expect(await redis.get(cacheKey("courses", "popular", 50))).toBeNull(); + }); + + test("courses:stats cache is invalidated by enroll() (pre-existing behavior, still correct)", async () => { + if (!infraAvailable) return; + + await courseService.getStats(); + expect(await redis.get(cacheKey("courses", "stats"))).not.toBeNull(); + + await courseService.enroll(userId, courseId); + + expect(await redis.get(cacheKey("courses", "stats"))).toBeNull(); + }); +}); diff --git a/tests/unit/courses/prerequisites.test.ts b/tests/unit/courses/prerequisites.test.ts new file mode 100644 index 0000000..9ab508a --- /dev/null +++ b/tests/unit/courses/prerequisites.test.ts @@ -0,0 +1,158 @@ +/** + * Tests for GET /api/v1/courses/:id/prerequisites (#369). + * + * Covers the service layer: ordering by configured prerequisite list, + * completion status for an authenticated vs anonymous caller, and the + * not-found/empty-list edge cases. + * + * Note on the anonymous case: the route is `optionalAuth`, and for a caller + * with no user there is nothing to look up, so every entry comes back + * `completed: false` and the aggregate `met` flag is false. The endpoint + * cannot distinguish "not enrolled" from "not signed in" — a signed-out + * visitor sees the prerequisite list but no personal status. + */ +import { test, describe, expect, beforeEach, afterEach } from "vitest"; +import { courseService } from "../../../src/modules/courses/course.service.js"; +import { NotFoundError } from "../../../src/utils/errors.js"; +import { db } from "../../../src/config/database.js"; +import { courses, enrollments, users } from "../../../src/database/schema.js"; +import { eq, inArray } from "drizzle-orm"; + +describe("GET /api/v1/courses/:id/prerequisites (#369)", () => { + const userId = "d4444444-1111-4ef8-bb6d-6bb9bd380a11"; + const stellarAddress = "GPREREQTEST00000000000000000000000000000000000000000A"; + + const courseId = "d4444444-2222-4b92-b60d-8848db490a22"; + const prereqOneId = "d4444444-2222-4b92-b60d-8848db490a33"; + const prereqTwoId = "d4444444-2222-4b92-b60d-8848db490a44"; + + let infraAvailable = true; + + beforeEach(async () => { + try { + await db + .insert(users) + .values({ id: userId, stellarAddress, displayName: "Prereq Test User" }) + .onConflictDoNothing(); + + await db + .insert(courses) + .values([ + { + id: prereqOneId, + title: "Intro to Stellar", + description: "Prereq one", + difficulty: "beginner", + isActive: true, + }, + { + id: prereqTwoId, + title: "Soroban Basics", + description: "Prereq two", + difficulty: "beginner", + isActive: true, + }, + { + id: courseId, + title: "Advanced Smart Contracts", + description: "For #369 tests", + difficulty: "advanced", + isActive: true, + // Deliberately configured in reverse-insert order to assert + // the service preserves *this* order, not DB row order. + prerequisites: [prereqTwoId, prereqOneId], + }, + ]) + .onConflictDoNothing(); + } catch { + infraAvailable = false; + } + }); + + afterEach(async () => { + if (!infraAvailable) return; + await db.delete(enrollments).where(eq(enrollments.userId, userId)); + await db + .delete(courses) + .where(inArray(courses.id, [courseId, prereqOneId, prereqTwoId])); + await db.delete(users).where(eq(users.id, userId)); + }); + + test("throws NotFoundError for a non-existent course", async () => { + if (!infraAvailable) return; + await expect( + courseService.getCoursePrerequisites( + "00000000-0000-0000-0000-000000000000", + userId, + ), + ).rejects.toThrow(NotFoundError); + }); + + test("returns an empty list when the course has no prerequisites", async () => { + if (!infraAvailable) return; + const result = await courseService.getCoursePrerequisites(prereqOneId, userId); + expect(result.prerequisites).toEqual([]); + // No requirements to satisfy — nothing outstanding, so the gate is met. + expect(result.met).toBe(true); + }); + + test("returns prerequisites in configured order with completed:false when not enrolled", async () => { + if (!infraAvailable) return; + const result = await courseService.getCoursePrerequisites(courseId, userId); + + expect(result.prerequisites.map((p) => p.id)).toEqual([ + prereqTwoId, + prereqOneId, + ]); + expect(result.prerequisites.every((p) => p.completed === false)).toBe(true); + expect(result.met).toBe(false); + }); + + test("reports every entry as incomplete for an anonymous caller", async () => { + if (!infraAvailable) return; + const result = await courseService.getCoursePrerequisites(courseId, null); + expect(result.prerequisites.every((p) => p.completed === false)).toBe(true); + expect(result.met).toBe(false); + }); + + test("marks a prerequisite completed once the user has a completed enrollment for it", async () => { + if (!infraAvailable) return; + await db + .insert(enrollments) + .values({ userId, courseId: prereqOneId, completedAt: new Date() }) + .onConflictDoNothing(); + + const result = await courseService.getCoursePrerequisites(courseId, userId); + + expect(result.prerequisites.find((p) => p.id === prereqOneId)?.completed).toBe(true); + expect(result.prerequisites.find((p) => p.id === prereqTwoId)?.completed).toBe(false); + expect(result.met).toBe(false); + }); + + test("an enrollment that isn't completed yet does not count as completed", async () => { + if (!infraAvailable) return; + await db + .insert(enrollments) + .values({ userId, courseId: prereqOneId }) + .onConflictDoNothing(); + + const result = await courseService.getCoursePrerequisites(courseId, userId); + + expect(result.prerequisites.find((p) => p.id === prereqOneId)?.completed).toBe(false); + }); + + test("met becomes true only once every prerequisite is completed", async () => { + if (!infraAvailable) return; + await db + .insert(enrollments) + .values([ + { userId, courseId: prereqOneId, completedAt: new Date() }, + { userId, courseId: prereqTwoId, completedAt: new Date() }, + ]) + .onConflictDoNothing(); + + const result = await courseService.getCoursePrerequisites(courseId, userId); + + expect(result.met).toBe(true); + }); +}); diff --git a/tests/unit/courses/waitlist-dropEnrollment.test.ts b/tests/unit/courses/waitlist-dropEnrollment.test.ts new file mode 100644 index 0000000..d427202 --- /dev/null +++ b/tests/unit/courses/waitlist-dropEnrollment.test.ts @@ -0,0 +1,145 @@ +/** + * Tests for dropEnrollment and the waitlist-notification gap-fill (#310). + * + * Join/leave/status for the waitlist itself (POST/DELETE/GET + * /api/v1/courses/:id/waitlist) are already covered by + * tests/e2e/course-waitlist.test.ts against WaitlistService (added + * alongside #320/#323). This file covers the piece that was still + * missing: CourseService.dropEnrollment — the companion action to + * enroll() needed to give "a spot opens up" concrete meaning — and that + * it identifies the head of the waitlist via WaitlistService and records + * it (there's currently no notifications table to write a user-facing + * notification to, so this asserts the audit-log record instead). + */ +import { test, describe, expect, beforeEach, afterEach } from "vitest"; +import { courseService } from "../../../src/modules/courses/course.service.js"; +import { waitlistService } from "../../../src/modules/courses/waitlist.service.js"; +import { NotFoundError } from "../../../src/utils/errors.js"; +import { db } from "../../../src/config/database.js"; +import { redis } from "../../../src/config/redis.js"; +import { + courses, + enrollments, + users, + enrollmentWaitlist, + auditLogs, +} from "../../../src/database/schema.js"; +import { eq, inArray, and, desc } from "drizzle-orm"; + +describe("CourseService.dropEnrollment + waitlist notification gap-fill (#310)", () => { + const courseId = "d9999999-2222-4b92-b60d-8848db490a22"; + + const userAId = "d9999999-1111-4ef8-bb6d-6bb9bd380a01"; + const userBId = "d9999999-1111-4ef8-bb6d-6bb9bd380a02"; + const userIds = [userAId, userBId]; + + let infraAvailable = true; + + beforeEach(async () => { + try { + await redis.flushdb(); + + await db + .insert(users) + .values([ + { id: userAId, stellarAddress: "GWAITLIST0000000000000000000000000000000000000000000A", displayName: "Waitlist A" }, + { id: userBId, stellarAddress: "GWAITLIST0000000000000000000000000000000000000000000B", displayName: "Waitlist B" }, + ]) + .onConflictDoNothing(); + + await db + .insert(courses) + .values({ + id: courseId, + title: "Waitlist Test Course", + description: "For #310 tests", + difficulty: "beginner", + isActive: true, + }) + .onConflictDoNothing(); + } catch { + infraAvailable = false; + } + }); + + afterEach(async () => { + if (!infraAvailable) return; + await db.delete(auditLogs).where(eq(auditLogs.event, "course.waitlist.notified")); + await db.delete(auditLogs).where(eq(auditLogs.event, "course.enrollment_dropped")); + await db.delete(enrollmentWaitlist).where(eq(enrollmentWaitlist.courseId, courseId)); + await db.delete(enrollments).where(eq(enrollments.courseId, courseId)); + await db.delete(courses).where(eq(courses.id, courseId)); + await db.delete(users).where(inArray(users.id, userIds)); + }); + + test("throws NotFoundError dropping an enrollment that doesn't exist", async () => { + if (!infraAvailable) return; + await expect(courseService.dropEnrollment(userAId, courseId)).rejects.toThrow( + NotFoundError, + ); + }); + + test("deletes the enrollment row", async () => { + if (!infraAvailable) return; + await db.insert(enrollments).values({ userId: userAId, courseId }).onConflictDoNothing(); + + await courseService.dropEnrollment(userAId, courseId); + + const [enrollment] = await db + .select() + .from(enrollments) + .where(eq(enrollments.userId, userAId)); + expect(enrollment).toBeUndefined(); + }); + + test("identifies the waitlist head via WaitlistService and records it in the audit log", async () => { + if (!infraAvailable) return; + await db.insert(enrollments).values({ userId: userAId, courseId }).onConflictDoNothing(); + await waitlistService.joinWaitlist(userBId, courseId); + + await courseService.dropEnrollment(userAId, courseId); + + const [entry] = await db + .select() + .from(auditLogs) + .where(and(eq(auditLogs.event, "course.waitlist.notified"))) + .orderBy(desc(auditLogs.createdAt)) + .limit(1); + + expect(entry).toBeDefined(); + expect(entry.fields).toMatchObject({ userId: userBId, courseId }); + + // userB stays queued — they're only removed once they actually enroll. + const stillWaiting = await db + .select() + .from(enrollmentWaitlist) + .where(eq(enrollmentWaitlist.userId, userBId)); + expect(stillWaiting).toHaveLength(1); + }); + + test("does not throw and records nothing when the waitlist is empty", async () => { + if (!infraAvailable) return; + await db.insert(enrollments).values({ userId: userAId, courseId }).onConflictDoNothing(); + + await expect(courseService.dropEnrollment(userAId, courseId)).resolves.toBeUndefined(); + + const notified = await db + .select() + .from(auditLogs) + .where(eq(auditLogs.event, "course.waitlist.notified")); + expect(notified).toHaveLength(0); + }); + + test("enrolling removes the user from the waitlist (WaitlistService.removeFromWaitlist, called by enroll())", async () => { + if (!infraAvailable) return; + await waitlistService.joinWaitlist(userAId, courseId); + + await courseService.enroll(userAId, courseId); + + const remaining = await db + .select() + .from(enrollmentWaitlist) + .where(eq(enrollmentWaitlist.userId, userAId)); + expect(remaining).toHaveLength(0); + }); +}); diff --git a/tests/unit/quizzes/generate-batch.test.ts b/tests/unit/quizzes/generate-batch.test.ts new file mode 100644 index 0000000..3faf937 --- /dev/null +++ b/tests/unit/quizzes/generate-batch.test.ts @@ -0,0 +1,136 @@ +/** + * Tests for POST /api/v1/quizzes/generate-batch (#308). + */ +import { test, describe, expect, beforeEach, afterEach } from "vitest"; +import { db } from "../../../src/config/database.js"; +import { redis } from "../../../src/config/redis.js"; +import { quizService } from "../../../src/modules/quizzes/quiz.service.js"; +import { ForbiddenError } from "../../../src/utils/errors.js"; +import { courses, enrollments, users, quizzes } from "../../../src/database/schema.js"; +import { eq } from "drizzle-orm"; + +describe("POST /api/v1/quizzes/generate-batch (#308)", () => { + const userId = "b8888888-1111-4ef8-bb6d-6bb9bd380a11"; + const stellarAddress = "GBATCHGEN0000000000000000000000000000000000000000000A"; + + const courseId = "b8888888-2222-4b92-b60d-8848db490a22"; + const moduleOneId = "batch-module-1"; + const moduleTwoId = "batch-module-2"; + + let infraAvailable = true; + + beforeEach(async () => { + try { + await redis.flushdb(); + + await db + .insert(users) + .values({ id: userId, stellarAddress, displayName: "Batch Gen Test User" }) + .onConflictDoNothing(); + + await db + .insert(courses) + .values({ + id: courseId, + title: "Batch Generation Test Course", + description: "For #308 tests", + difficulty: "beginner", + isActive: true, + }) + .onConflictDoNothing(); + + await db + .insert(enrollments) + .values({ userId, courseId }) + .onConflictDoNothing(); + } catch { + infraAvailable = false; + } + }); + + afterEach(async () => { + if (!infraAvailable) return; + await db.delete(quizzes).where(eq(quizzes.courseId, courseId)); + await db.delete(enrollments).where(eq(enrollments.userId, userId)); + await db.delete(courses).where(eq(courses.id, courseId)); + await db.delete(users).where(eq(users.id, userId)); + }); + + test("generates a quiz for each requested module independently", async () => { + if (!infraAvailable) return; + + const results = await quizService.generateQuizBatch(userId, { + courseId, + moduleIds: [moduleOneId, moduleTwoId], + }); + + expect(results).toHaveLength(2); + expect(results[0]).toMatchObject({ moduleId: moduleOneId, success: true }); + expect(results[1]).toMatchObject({ moduleId: moduleTwoId, success: true }); + expect(results[0].success && results[0].quiz.moduleId).toBe(moduleOneId); + expect(results[1].success && results[1].quiz.moduleId).toBe(moduleTwoId); + }); + + test("a failure on one module doesn't block the rest of the batch", async () => { + if (!infraAvailable) return; + + // A user who isn't enrolled fails generateQuiz's enrollment check for + // every module — each entry should report the failure independently + // rather than the whole batch throwing. + const notEnrolledUserId = "b8888888-3333-4b92-b60d-8848db490a33"; + await db + .insert(users) + .values({ + id: notEnrolledUserId, + stellarAddress: "GBATCHGEN0000000000000000000000000000000000000000000B", + displayName: "Not Enrolled", + }) + .onConflictDoNothing(); + + const results = await quizService.generateQuizBatch(notEnrolledUserId, { + courseId, + moduleIds: [moduleOneId, moduleTwoId], + }); + + expect(results).toHaveLength(2); + for (const result of results) { + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error).toContain("enrolled"); + } + } + + await db.delete(users).where(eq(users.id, notEnrolledUserId)); + }); + + test("throwing generateQuiz directly still surfaces ForbiddenError (sanity check for the batch's error message)", async () => { + if (!infraAvailable) return; + + const notEnrolledUserId = "b8888888-4444-4b92-b60d-8848db490a44"; + await expect( + quizService.generateQuiz(notEnrolledUserId, { courseId, moduleId: moduleOneId }), + ).rejects.toThrow(ForbiddenError); + }); + + test("reuses an existing quiz for a module rather than regenerating it", async () => { + if (!infraAvailable) return; + + const [existingQuiz] = await db + .insert(quizzes) + .values({ + courseId, + moduleId: moduleOneId, + questions: [{ id: "q1", text: "2+2?", options: ["3", "4"], correctIndex: 1 }], + generatedFor: userId, + }) + .returning(); + + const results = await quizService.generateQuizBatch(userId, { + courseId, + moduleIds: [moduleOneId], + }); + + expect(results[0]).toMatchObject({ moduleId: moduleOneId, success: true }); + expect(results[0].success && results[0].quiz.id).toBe(existingQuiz.id); + }); +}); diff --git a/tests/unit/quizzes/generation-rate-limit.test.ts b/tests/unit/quizzes/generation-rate-limit.test.ts new file mode 100644 index 0000000..4656383 --- /dev/null +++ b/tests/unit/quizzes/generation-rate-limit.test.ts @@ -0,0 +1,228 @@ +import { test, describe, expect, beforeEach, afterEach } from "vitest"; +import { db } from "../../../src/config/database.js"; +import { redis } from "../../../src/config/redis.js"; +import { quizService } from "../../../src/modules/quizzes/quiz.service.js"; +import { MAX_QUIZ_GENERATIONS_PER_MODULE_PER_HOUR } from "../../../src/modules/quizzes/quiz.types.js"; +import { RateLimitError } from "../../../src/utils/errors.js"; +import { courses, enrollments, users, quizzes } from "../../../src/database/schema.js"; +import { eq } from "drizzle-orm"; + +describe("Quiz generation rate limiting (#291)", () => { + const mockUserId = "e1a2d3e4-1111-4ef8-bb6d-6bb9bd380a55"; + const otherModuleUserId = "e1a2d3e4-1111-4ef8-bb6d-6bb9bd380a56"; + const mockCourseId = "e1a2d3e4-2222-4b92-b60d-8848db490a66"; + const mockModuleId = "rate-limit-module-1"; + const otherModuleId = "rate-limit-module-2"; + + let infraAvailable = true; + + beforeEach(async () => { + try { + await redis.flushdb(); + + await db + .insert(users) + .values({ + id: mockUserId, + stellarAddress: + "GRATELIMIT000000000000000000000000000000000000000000001", + displayName: "Rate Limit Test User", + credits: 0, + }) + .onConflictDoNothing(); + + await db + .insert(courses) + .values({ + id: mockCourseId, + title: "Rate Limit Test Course", + description: "For quiz generation rate limit tests", + difficulty: "beginner", + isActive: true, + }) + .onConflictDoNothing(); + + await db + .insert(enrollments) + .values({ userId: mockUserId, courseId: mockCourseId }) + .onConflictDoNothing(); + + // Pre-seed a quiz for mockModuleId so generateQuiz takes the + // existing-quiz short-circuit (no AI service call needed) — the rate + // limit check runs before that lookup, so it's still exercised. + await db + .insert(quizzes) + .values({ + courseId: mockCourseId, + moduleId: mockModuleId, + questions: [ + { id: "q1", text: "2+2?", options: ["3", "4"], correctIndex: 1 }, + ], + generatedFor: mockUserId, + }) + .onConflictDoNothing(); + } catch { + infraAvailable = false; + } + }); + + afterEach(async () => { + if (!infraAvailable) return; + await db.delete(quizzes).where(eq(quizzes.courseId, mockCourseId)); + await db.delete(enrollments).where(eq(enrollments.userId, mockUserId)); + await db.delete(courses).where(eq(courses.id, mockCourseId)); + await db.delete(users).where(eq(users.id, mockUserId)); + }); + + test(`allows up to ${MAX_QUIZ_GENERATIONS_PER_MODULE_PER_HOUR} generations per user per module per hour`, async () => { + if (!infraAvailable) return; + + for (let i = 0; i < MAX_QUIZ_GENERATIONS_PER_MODULE_PER_HOUR; i++) { + const quiz = await quizService.generateQuiz(mockUserId, { + courseId: mockCourseId, + moduleId: mockModuleId, + }); + expect(quiz.moduleId).toBe(mockModuleId); + } + }); + + test(`rejects the (${MAX_QUIZ_GENERATIONS_PER_MODULE_PER_HOUR + 1})th generation with RateLimitError carrying a positive retryAfterSeconds`, async () => { + if (!infraAvailable) return; + + for (let i = 0; i < MAX_QUIZ_GENERATIONS_PER_MODULE_PER_HOUR; i++) { + await quizService.generateQuiz(mockUserId, { + courseId: mockCourseId, + moduleId: mockModuleId, + }); + } + + await expect( + quizService.generateQuiz(mockUserId, { + courseId: mockCourseId, + moduleId: mockModuleId, + }), + ).rejects.toThrow(RateLimitError); + + try { + await quizService.generateQuiz(mockUserId, { + courseId: mockCourseId, + moduleId: mockModuleId, + }); + expect.fail("expected RateLimitError to be thrown"); + } catch (err) { + expect(err).toBeInstanceOf(RateLimitError); + const rateLimitErr = err as RateLimitError; + expect(rateLimitErr.statusCode).toBe(429); + expect(rateLimitErr.retryAfterSeconds).toBeGreaterThan(0); + expect(rateLimitErr.retryAfterSeconds).toBeLessThanOrEqual(60 * 60); + } + }); + + test("rate limit is scoped per module — a different module for the same user is unaffected", async () => { + if (!infraAvailable) return; + + await db + .insert(users) + .values({ + id: otherModuleUserId, + stellarAddress: + "GRATELIMIT000000000000000000000000000000000000000000002", + displayName: "Rate Limit Test User 2", + credits: 0, + }) + .onConflictDoNothing(); + await db + .insert(enrollments) + .values({ userId: otherModuleUserId, courseId: mockCourseId }) + .onConflictDoNothing(); + + for (let i = 0; i < MAX_QUIZ_GENERATIONS_PER_MODULE_PER_HOUR; i++) { + await quizService.generateQuiz(mockUserId, { + courseId: mockCourseId, + moduleId: mockModuleId, + }); + } + await expect( + quizService.generateQuiz(mockUserId, { + courseId: mockCourseId, + moduleId: mockModuleId, + }), + ).rejects.toThrow(RateLimitError); + + // Same user, different module — pre-seed a quiz there too so this stays + // on the existing-quiz short-circuit rather than calling the AI service. + await db + .insert(quizzes) + .values({ + courseId: mockCourseId, + moduleId: otherModuleId, + questions: [ + { id: "q1", text: "2+2?", options: ["3", "4"], correctIndex: 1 }, + ], + generatedFor: mockUserId, + }) + .onConflictDoNothing(); + + const quiz = await quizService.generateQuiz(mockUserId, { + courseId: mockCourseId, + moduleId: otherModuleId, + }); + expect(quiz.moduleId).toBe(otherModuleId); + + await db + .delete(users) + .where(eq(users.id, otherModuleUserId)); + }); + + test("rate limit is scoped per user — a different user for the same module is unaffected", async () => { + if (!infraAvailable) return; + + await db + .insert(users) + .values({ + id: otherModuleUserId, + stellarAddress: + "GRATELIMIT000000000000000000000000000000000000000000003", + displayName: "Rate Limit Test User 3", + credits: 0, + }) + .onConflictDoNothing(); + await db + .insert(enrollments) + .values({ userId: otherModuleUserId, courseId: mockCourseId }) + .onConflictDoNothing(); + + for (let i = 0; i < MAX_QUIZ_GENERATIONS_PER_MODULE_PER_HOUR; i++) { + await quizService.generateQuiz(mockUserId, { + courseId: mockCourseId, + moduleId: mockModuleId, + }); + } + await expect( + quizService.generateQuiz(mockUserId, { + courseId: mockCourseId, + moduleId: mockModuleId, + }), + ).rejects.toThrow(RateLimitError); + + // Different user, same module + course, same pre-seeded quiz's module — + // but generatedFor is scoped to mockUserId, so this user has no + // existing quiz for this module and would call the AI service. Instead + // assert only that this user's own counter is unaffected by asserting + // no RateLimitError is thrown before the AI-service call is reached + // (which itself is allowed to fail/fall back — that's not what's under + // test here). + await expect( + quizService.generateQuiz(otherModuleUserId, { + courseId: mockCourseId, + moduleId: mockModuleId, + }), + ).resolves.toBeDefined(); + + // generateQuiz for this user/module had no pre-seeded quiz, so it went + // through the (unreachable AI service -> placeholder fallback) path and + // inserted its own quiz row — clean that up before the user FK. + await db.delete(quizzes).where(eq(quizzes.generatedFor, otherModuleUserId)); + await db.delete(users).where(eq(users.id, otherModuleUserId)); + }); +}); diff --git a/tests/unit/quizzes/retry-and-course-admin.test.ts b/tests/unit/quizzes/retry-and-course-admin.test.ts new file mode 100644 index 0000000..a50fe04 --- /dev/null +++ b/tests/unit/quizzes/retry-and-course-admin.test.ts @@ -0,0 +1,264 @@ +import { test, describe, expect, beforeEach, afterEach, vi } from "vitest"; +import { db } from "../../../src/config/database.js"; +import { redis } from "../../../src/config/redis.js"; +import { quizService } from "../../../src/modules/quizzes/quiz.service.js"; +import { courseService } from "../../../src/modules/courses/course.service.js"; +import { MAX_RETRIES_PER_MODULE_PER_DAY } from "../../../src/modules/quizzes/quiz.types.js"; +import { RateLimitError, ForbiddenError } from "../../../src/utils/errors.js"; +import { + courses, + enrollments, + users, + quizSubmissions, + quizzes, +} from "../../../src/database/schema.js"; +import { eq } from "drizzle-orm"; + +describe("Quiz retry endpoint & course admin/popular endpoints (#292, #293, #294, #295)", () => { + const mockUserId = "b1c2d3e4-1111-4ef8-bb6d-6bb9bd380a11"; + const mockCourseId = "b1c2d3e4-2222-4b92-b60d-8848db490a22"; + const mockModuleId = "module-1"; + + let infraAvailable = true; + + beforeEach(async () => { + try { + await redis.flushdb(); + vi.clearAllMocks(); + + await db + .insert(users) + .values({ + id: mockUserId, + stellarAddress: + "GBAXL3624V2V6R3E4W67ZXLN76K4E3U5V62M3X7A4P5R6S7T8U9V0W1D", + displayName: "Retry Test User", + credits: 0, + }) + .onConflictDoNothing(); + + await db + .insert(courses) + .values({ + id: mockCourseId, + title: "Retry Test Course", + description: "For retry/admin/popular tests", + difficulty: "beginner", + isActive: true, + }) + .onConflictDoNothing(); + + await db + .insert(enrollments) + .values({ userId: mockUserId, courseId: mockCourseId }) + .onConflictDoNothing(); + } catch { + infraAvailable = false; + } + }); + + afterEach(async () => { + if (!infraAvailable) return; + await db.delete(quizSubmissions).where(eq(quizSubmissions.userId, mockUserId)); + await db.delete(quizzes).where(eq(quizzes.courseId, mockCourseId)); + await db.delete(enrollments).where(eq(enrollments.userId, mockUserId)); + await db.delete(courses).where(eq(courses.id, mockCourseId)); + await db.delete(users).where(eq(users.id, mockUserId)); + }); + + async function createSubmittedQuiz() { + const [quiz] = await db + .insert(quizzes) + .values({ + courseId: mockCourseId, + moduleId: mockModuleId, + questions: [ + { id: "q1", text: "2+2?", options: ["3", "4"], correctIndex: 1 }, + ], + generatedFor: mockUserId, + }) + .returning(); + + await quizService.submitQuiz(mockUserId, quiz.id, { + answers: [{ questionId: "q1", selectedIndex: 0 }], + }); + + return quiz; + } + + test("retryQuiz creates a fresh quiz for the same module and marks the old submission superseded (#295)", async () => { + if (!infraAvailable) return; + + const quiz = await createSubmittedQuiz(); + + const retried = await quizService.retryQuiz(mockUserId, quiz.id); + + expect(retried.id).not.toBe(quiz.id); + expect(retried.courseId).toBe(mockCourseId); + expect(retried.moduleId).toBe(mockModuleId); + expect(retried.questions.length).toBeGreaterThan(0); + + const [oldSubmission] = await db + .select() + .from(quizSubmissions) + .where(eq(quizSubmissions.quizId, quiz.id)); + expect(oldSubmission.superseded).toBe(true); + }); + + test("retryQuiz rejects a quiz that has no submission yet (#295)", async () => { + if (!infraAvailable) return; + + const [quiz] = await db + .insert(quizzes) + .values({ + courseId: mockCourseId, + moduleId: mockModuleId, + questions: [ + { id: "q1", text: "2+2?", options: ["3", "4"], correctIndex: 1 }, + ], + generatedFor: mockUserId, + }) + .returning(); + + await expect(quizService.retryQuiz(mockUserId, quiz.id)).rejects.toThrow( + ForbiddenError, + ); + }); + + test("retryQuiz enforces a max of 3 retries per module per day (#295)", async () => { + if (!infraAvailable) return; + + const quiz = await createSubmittedQuiz(); + + let lastQuizId = quiz.id; + for (let i = 0; i < MAX_RETRIES_PER_MODULE_PER_DAY; i++) { + const retried = await quizService.retryQuiz(mockUserId, lastQuizId); + lastQuizId = retried.id; + // Each retry needs its own submission before it can be retried again. + await quizService.submitQuiz(mockUserId, lastQuizId, { + answers: [{ questionId: retried.questions[0].id, selectedIndex: 0 }], + }); + } + + await expect( + quizService.retryQuiz(mockUserId, lastQuizId), + ).rejects.toThrow(RateLimitError); + }); + + test("getPopularCourses returns only active courses ordered by enrollment count and respects the limit (#293)", async () => { + if (!infraAvailable) return; + + const [popularCourse] = await db + .insert(courses) + .values({ + title: "Very Popular Course", + description: "many enrollments", + difficulty: "beginner", + isActive: true, + }) + .returning(); + + const [inactiveCourse] = await db + .insert(courses) + .values({ + title: "Inactive Course", + description: "should never appear", + difficulty: "beginner", + isActive: false, + }) + .returning(); + + const extraUserIds: string[] = []; + try { + for (let i = 0; i < 3; i++) { + const [extraUser] = await db + .insert(users) + .values({ + stellarAddress: `GEXTRA${i}00000000000000000000000000000000000000000000000`, + credits: 0, + }) + .returning(); + extraUserIds.push(extraUser.id); + await db + .insert(enrollments) + .values({ userId: extraUser.id, courseId: popularCourse.id }); + } + await db + .insert(enrollments) + .values({ userId: extraUserIds[0], courseId: inactiveCourse.id }); + + const popular = await courseService.getPopularCourses(50); + + const popularIds = popular.map((c) => c.id); + expect(popularIds).toContain(popularCourse.id); + expect(popularIds).not.toContain(inactiveCourse.id); + expect(popular[0].id).toBe(popularCourse.id); + expect(popular[0].enrolledCount).toBe(3); + + const limited = await courseService.getPopularCourses(1); + expect(limited.length).toBe(1); + } finally { + for (const id of extraUserIds) { + await db.delete(enrollments).where(eq(enrollments.userId, id)); + await db.delete(users).where(eq(users.id, id)); + } + await db.delete(enrollments).where(eq(enrollments.courseId, popularCourse.id)); + await db.delete(courses).where(eq(courses.id, popularCourse.id)); + await db.delete(courses).where(eq(courses.id, inactiveCourse.id)); + } + }); + + test("getPopularCourses caches its result for the given limit (#293)", async () => { + if (!infraAvailable) return; + + await courseService.getPopularCourses(10); + const cached = await redis.get("chainlearn:courses:popular:10"); + expect(cached).not.toBeNull(); + + const ttl = await redis.ttl("chainlearn:courses:popular:10"); + expect(ttl).toBeGreaterThan(0); + expect(ttl).toBeLessThanOrEqual(300); + }); + + test("admin createCourse/updateCourse/deleteCourse manage courses and invalidate caches (#292)", async () => { + if (!infraAvailable) return; + + const created = await courseService.createCourse({ + title: "Admin Created Course", + description: "created via admin endpoint", + difficulty: "advanced", + tags: ["stellar", "soroban"], + }); + + expect(created.title).toBe("Admin Created Course"); + expect(created.isActive).toBe(true); + expect(created.tags).toEqual(["stellar", "soroban"]); + + const updated = await courseService.updateCourse(created.id, { + title: "Updated Title", + }); + expect(updated.title).toBe("Updated Title"); + + await courseService.deleteCourse(created.id); + + const { courses: listed } = await courseService.listCourses(null, { + page: 1, + limit: 50, + }); + expect(listed.map((c) => c.id)).not.toContain(created.id); + + await db.delete(courses).where(eq(courses.id, created.id)); + }); + + test("enroll() skips content hash verification (no mismatch) when no on-chain contract is configured (#294)", async () => { + if (!infraAvailable) return; + + await db + .update(courses) + .set({ contentHash: "abc123" }) + .where(eq(courses.id, mockCourseId)); + + const result = await courseService.enroll(mockUserId, mockCourseId); + expect(result.contentHashMismatch).toBe(false); + }); +}); diff --git a/tests/unit/services/deduct-credits-toctou.test.ts b/tests/unit/services/deduct-credits-toctou.test.ts new file mode 100644 index 0000000..b145ace --- /dev/null +++ b/tests/unit/services/deduct-credits-toctou.test.ts @@ -0,0 +1,221 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +/** + * #476: deductCredits used to be a SELECT-then-UPDATE — read `credits`, + * check `credits >= amount` in application code, then a separate UPDATE + * wrote `credits - amount`. A concurrent writer between the SELECT and the + * UPDATE (another deduction, or a grant) could move the balance, and the + * UPDATE's WHERE clause never re-enforced sufficiency, so credits could go + * negative. The fix collapses this into one atomic UPDATE whose WHERE + * clause guards `credits >= amount` at the database level, mirroring how + * grantCredits already avoids the analogous race with a single + * `credits + amount` UPDATE. + * + * These tests mock the Drizzle query builder rather than hitting a real + * Postgres instance (matching the mocking style already used in + * concurrent-safety.test.ts for services in this codebase). Because the + * mock can't itself model row-level locking, "concurrency" here is + * approximated by two *sequential* deductCredits() calls against a shared + * mock balance that together would overdraw a single starting balance — + * exactly per the task's documented fallback when there's no existing + * pattern for firing genuinely parallel requests against a real DB in this + * suite. What's actually under test is the atomic UPDATE's WHERE-guard + * behavior (the SQL executed, and that an empty `returning` is treated as + * "reject"), not real database-level lock contention. + */ + +vi.mock("../../../src/config/database.js", () => { + const mockDb = { + select: vi.fn(), + update: vi.fn(), + }; + return { db: mockDb }; +}); + +vi.mock("../../../src/audit/index.js", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("../../../src/utils/logger.js", () => ({ + logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn() }, +})); + +vi.mock("../../../src/cache/index.js", () => ({ + cacheDel: vi.fn().mockResolvedValue(undefined), + cacheInvalidatePattern: vi.fn().mockResolvedValue(undefined), + cacheKey: vi.fn((...parts: string[]) => parts.join(":")), + cacheKeyPattern: vi.fn((...parts: string[]) => `${parts.join(":")}:*`), +})); + +import { db } from "../../../src/config/database.js"; +import { adminUsersService } from "../../../src/modules/admin/admin-users.service.js"; +import { auditLog } from "../../../src/audit/index.js"; +import { ValidationError, NotFoundError } from "../../../src/utils/errors.js"; + +const mockDb = vi.mocked(db); + +const USER_ID = "11111111-1111-4111-8111-111111111111"; + +/** + * Simulates a users table's `credits` column as an in-memory value so the + * mocked UPDATE's WHERE-guard (`credits >= amount`) can be evaluated the + * same way Postgres would evaluate it — this is what lets the "two + * sequential deductions overdrawing one balance" scenario actually exercise + * the guard logic instead of just always succeeding. + */ +function mockUserWithBalance(initialCredits: number, exists = true) { + let credits = initialCredits; + + // db.select({ id }).from(users).where(...) — existence check + const selectChain: any = { + from: vi.fn().mockReturnThis(), + where: vi.fn().mockImplementation(() => + Promise.resolve(exists ? [{ id: USER_ID, credits }] : []), + ), + }; + + // db.update(users).set(...).where(...).returning(...) — atomic deduction + const updateChain: any = { + set: vi.fn().mockReturnThis(), + where: vi.fn().mockReturnThis(), + returning: vi.fn(), + }; + + mockDb.select.mockReturnValue(selectChain); + mockDb.update.mockReturnValue(updateChain); + + updateChain.returning.mockImplementation(() => { + // Approximates Postgres evaluating `WHERE ... AND credits >= amount` + // under the UPDATE's row lock: the WHERE clause's amount is captured + // by the `.where()` call, so pull it from there via the mock's last + // call args (the service always passes `amount` positionally in the + // sql template, but for this mock we instead track deductions through + // a shared closure — see below). + return Promise.resolve(pendingDeductionResult()); + }); + + let queuedAmount: number | null = null; + + function pendingDeductionResult() { + if (queuedAmount === null) return []; + if (credits >= queuedAmount) { + credits -= queuedAmount; + return [{ id: USER_ID, credits }]; + } + return []; + } + + return { + /** Arms the next update() call to attempt deducting `amount`. */ + queueDeduction(amount: number) { + queuedAmount = amount; + }, + getCredits: () => credits, + }; +} + +describe("deductCredits — TOCTOU fix (#476)", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("succeeds when the balance is sufficient", async () => { + const sim = mockUserWithBalance(100); + sim.queueDeduction(40); + + const result = await adminUsersService.deductCredits( + USER_ID, + 40, + "penalty", + "ref-1", + "admin-1", + ); + + expect(result.creditsBefore).toBe(100); + expect(result.creditsAfter).toBe(60); + expect(sim.getCredits()).toBe(60); + expect(auditLog).toHaveBeenCalledWith( + "credits.deducted", + expect.objectContaining({ + userId: USER_ID, + amount: 40, + creditsBefore: 100, + creditsAfter: 60, + }), + ); + }); + + it("fails cleanly with ValidationError when the balance is insufficient, without mutating credits", async () => { + const sim = mockUserWithBalance(30); + sim.queueDeduction(50); + + await expect( + adminUsersService.deductCredits(USER_ID, 50, "penalty"), + ).rejects.toThrow(ValidationError); + + // Balance must be untouched — the atomic UPDATE's WHERE guard rejected + // the write outright rather than applying a partial/negative update. + expect(sim.getCredits()).toBe(30); + }); + + it("insufficient-balance error reports the actual current balance", async () => { + const sim = mockUserWithBalance(30); + sim.queueDeduction(50); + + // ValidationError.message is always the generic "Validation failed" — + // the field-level detail lives in `.errors` (see src/utils/errors.ts). + await expect( + adminUsersService.deductCredits(USER_ID, 50, "penalty"), + ).rejects.toMatchObject({ + errors: { + amount: [expect.stringMatching(/has 30 but deduction of 50/)], + }, + }); + }); + + it("throws NotFoundError when the user does not exist", async () => { + mockUserWithBalance(0, /* exists */ false); + + await expect( + adminUsersService.deductCredits("nonexistent-user", 10, "penalty"), + ).rejects.toThrow(NotFoundError); + }); + + it("never allows two sequential deductions to together overdraw a single starting balance (WHERE-guard regression test)", async () => { + // Documented substitute for genuine parallel DB contention (see file + // header): fires two deductions in sequence against a shared simulated + // balance where only one can be afforded, and asserts the second is + // rejected by the atomic UPDATE's WHERE guard rather than succeeding + // and driving credits negative — which is exactly the bug #476 fixed + // (the old SELECT-then-UPDATE would have let both through if the + // SELECTs both ran before either UPDATE). + const sim = mockUserWithBalance(60); + + sim.queueDeduction(40); + const first = await adminUsersService.deductCredits( + USER_ID, + 40, + "first deduction", + ); + expect(first.creditsAfter).toBe(20); + + sim.queueDeduction(40); + await expect( + adminUsersService.deductCredits(USER_ID, 40, "second deduction"), + ).rejects.toThrow(ValidationError); + + // Balance settles at 20, never goes negative. + expect(sim.getCredits()).toBe(20); + }); + + it("uses a single atomic UPDATE with a WHERE-clause balance guard, not a separate check-then-act", async () => { + const sim = mockUserWithBalance(100); + sim.queueDeduction(10); + + await adminUsersService.deductCredits(USER_ID, 10, "penalty"); + + // Exactly one update() call — the fix is a single atomic statement, + // not an application-level check followed by an unconditional write. + expect(mockDb.update).toHaveBeenCalledTimes(1); + }); +}); diff --git a/tests/unit/users/account-deletion.test.ts b/tests/unit/users/account-deletion.test.ts new file mode 100644 index 0000000..4f9933a --- /dev/null +++ b/tests/unit/users/account-deletion.test.ts @@ -0,0 +1,154 @@ +import { test, describe, expect, beforeEach, afterEach } from "vitest"; +import { eq } from "drizzle-orm"; +import { db } from "../../../src/config/database.js"; +import { redis } from "../../../src/config/redis.js"; +import { userService } from "../../../src/modules/users/user.service.js"; +import { cacheKey, cacheSet } from "../../../src/cache/index.js"; +import { + users, + courses, + enrollments, + credentials, +} from "../../../src/database/schema.js"; + +describe("Account deletion (#290)", () => { + const mockUserId = "f1a2d3e4-1111-4ef8-bb6d-6bb9bd380a44"; + const mockCourseId = "f1a2d3e4-2222-4b92-b60d-8848db490a55"; + + let infraAvailable = true; + + beforeEach(async () => { + try { + await redis.flushdb(); + + await db + .insert(users) + .values({ + id: mockUserId, + stellarAddress: + "GDELETE0000000000000000000000000000000000000000000004", + displayName: "To Be Deleted", + background: "Some background text", + learningGoal: "Some learning goal", + credits: 42, + }) + .onConflictDoNothing(); + + await db + .insert(courses) + .values({ + id: mockCourseId, + title: "Deletion Test Course", + description: "For account deletion tests", + difficulty: "beginner", + isActive: true, + }) + .onConflictDoNothing(); + + await db + .insert(enrollments) + .values({ userId: mockUserId, courseId: mockCourseId }) + .onConflictDoNothing(); + + await db + .insert(credentials) + .values({ + userId: mockUserId, + courseId: mockCourseId, + score: 90, + }) + .onConflictDoNothing(); + } catch { + infraAvailable = false; + } + }); + + afterEach(async () => { + if (!infraAvailable) return; + await db.delete(credentials).where(eq(credentials.userId, mockUserId)); + await db.delete(enrollments).where(eq(enrollments.userId, mockUserId)); + await db.delete(courses).where(eq(courses.id, mockCourseId)); + await db.delete(users).where(eq(users.id, mockUserId)); + }); + + test("sets deletedAt and clears displayName/background/learningGoal", async () => { + if (!infraAvailable) return; + + await userService.deleteAccount(mockUserId); + + const [row] = await db.select().from(users).where(eq(users.id, mockUserId)); + expect(row.deletedAt).not.toBeNull(); + expect(row.displayName).toBeNull(); + expect(row.background).toBeNull(); + expect(row.learningGoal).toBeNull(); + }); + + test("preserves credits (not part of the deletion payload)", async () => { + if (!infraAvailable) return; + + await userService.deleteAccount(mockUserId); + + const [row] = await db.select().from(users).where(eq(users.id, mockUserId)); + expect(row.credits).toBe(42); + }); + + test("preserves enrollments — does not cascade-delete or null them out", async () => { + if (!infraAvailable) return; + + await userService.deleteAccount(mockUserId); + + const rows = await db + .select() + .from(enrollments) + .where(eq(enrollments.userId, mockUserId)); + expect(rows.length).toBe(1); + expect(rows[0].courseId).toBe(mockCourseId); + }); + + test("preserves credentials — does not cascade-delete or null them out", async () => { + if (!infraAvailable) return; + + await userService.deleteAccount(mockUserId); + + const rows = await db + .select() + .from(credentials) + .where(eq(credentials.userId, mockUserId)); + expect(rows.length).toBe(1); + expect(rows[0].score).toBe(90); + }); + + test("throws NotFoundError for a non-existent user", async () => { + if (!infraAvailable) return; + + await expect( + userService.deleteAccount("00000000-0000-0000-0000-000000000000"), + ).rejects.toThrow(); + }); + + test("invalidates cached profile/progress data for the deleted user", async () => { + if (!infraAvailable) return; + + const profileKey = cacheKey("user", "profile", mockUserId); + const progressKey = cacheKey("user", "progress", mockUserId); + await cacheSet(profileKey, { stale: true }, 300); + await cacheSet(progressKey, { stale: true }, 300); + + expect(await redis.get(profileKey)).not.toBeNull(); + expect(await redis.get(progressKey)).not.toBeNull(); + + await userService.deleteAccount(mockUserId); + + expect(await redis.get(profileKey)).toBeNull(); + expect(await redis.get(progressKey)).toBeNull(); + }); + + test("is idempotent-safe to call twice without throwing on the second call's cache step", async () => { + if (!infraAvailable) return; + + await userService.deleteAccount(mockUserId); + // Second call still finds the row (soft delete, row still exists) and + // succeeds — deletedAt is simply overwritten with a newer timestamp. + await expect(userService.deleteAccount(mockUserId)).resolves.toBeUndefined(); + }); +});