diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index 65ae528..b731ddb 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -4,6 +4,17 @@ on: branches: - 'develop' - 'main' + +# A pull request opened from a fork does not receive the `testing` environment's +# secrets or variables, so every `vars.*` and `secrets.*` below resolves to an +# empty string on those runs. The `|| '...'` fallbacks keep the job +# self-sufficient; where the environment is available, its values still win. +# +# The fallbacks are not arbitrary. MONGO_URI is hard-coded, so the container the +# suite talks to has to listen on 27017 and hold `space_testing_db` for the +# tests to connect at all - the two variables cannot hold anything else without +# breaking the run. JWT_SECRET and JWT_SALT are only a key and a pbkdf2 salt for +# tokens minted and verified inside the same run, so any non-empty pair works. jobs: build: name: Test @@ -22,14 +33,14 @@ jobs: uses: SpicyPizza/create-envfile@v2.0 with: envkey_ENVIRONMENT: "testing" - envkey_DATABASE_NAME: ${{ vars.CI_MONGO_INITDB_DATABASE }} + envkey_DATABASE_NAME: ${{ vars.CI_MONGO_INITDB_DATABASE || 'space_testing_db' }} envkey_MONGO_URI: mongodb://localhost:27017/space_testing_db?authSource=space_testing_db envkey_ADMIN_USER: "admin" envkey_ADMIN_PASSWORD: "4dm1n" envkey_REDIS_URL: "redis://localhost:6379" - envkey_JWT_SECRET: ${{ secrets.CI_JWT_SECRET }} + envkey_JWT_SECRET: ${{ secrets.CI_JWT_SECRET || 'ci_test_secret' }} envkey_JWT_EXPIRATION: "1h" - envkey_JWT_SALT: ${{ secrets.CI_JWT_SALT }} + envkey_JWT_SALT: ${{ secrets.CI_JWT_SALT || 'ci_test_salt' }} directory: . file_name: api/.env @@ -40,8 +51,8 @@ jobs: uses: supercharge/mongodb-github-action@1.10.0 with: mongodb-version: '7.0.16' - mongodb-db: ${{ vars.CI_MONGO_INITDB_DATABASE }} - mongodb-port: ${{ vars.CI_MONGO_PORT }} + mongodb-db: ${{ vars.CI_MONGO_INITDB_DATABASE || 'space_testing_db' }} + mongodb-port: ${{ vars.CI_MONGO_PORT || '27017' }} - name: Start Redis uses: shogo82148/actions-setup-redis@v1 diff --git a/CITATION.cff b/CITATION.cff new file mode 100644 index 0000000..d106274 --- /dev/null +++ b/CITATION.cff @@ -0,0 +1,27 @@ +cff-version: 1.2.0 +title: "SPACE" +message: "If you use this software, please cite it as below." +type: software +authors: + - family-names: García-Fernández + given-names: Alejandro + email: agarcia29@us.es + orcid: https://orcid.org/0009-0005-0778-1432 + - family-names: Parejo + given-names: José Antonio + email: japarejo@us.es + orcid: https://orcid.org/0000-0002-4708-4606 + - family-names: Ruiz-Cortés + given-names: Antonio + email: aruiz@us.es + orcid: https://orcid.org/0000-0001-9827-1834 +repository-code: "https://github.com/isa-group/space" +license: MIT +version: 1.5.0 +date-released: "2026-07-31" +keywords: + - pricing-driven self-adaptation + - self-adaptive systems + - cloud computing + - software pricing + - service-oriented architecture diff --git a/api/package.json b/api/package.json index 147e6db..bf6c105 100644 --- a/api/package.json +++ b/api/package.json @@ -58,7 +58,7 @@ "multer": "1.4.5-lts.2", "nock": "^14.0.10", "node-fetch": "^3.3.2", - "pricing4ts": "^0.10.3", + "pricing4ts": "^0.11.1", "redis": "^4.7.0", "socket.io": "^4.8.1", "uuid": "^11.1.0" diff --git a/api/pnpm-lock.yaml b/api/pnpm-lock.yaml index 082c3c9..a6c07c0 100644 --- a/api/pnpm-lock.yaml +++ b/api/pnpm-lock.yaml @@ -63,8 +63,8 @@ importers: specifier: ^3.3.2 version: 3.3.2 pricing4ts: - specifier: ^0.10.3 - version: 0.10.3(@openfeature/core@1.8.0)(minizinc@4.4.3) + specifier: ^0.11.1 + version: 0.11.1(@openfeature/core@1.8.0)(minizinc@4.4.3) redis: specifier: ^4.7.0 version: 4.7.1 @@ -1783,8 +1783,8 @@ packages: engines: {node: '>=14'} hasBin: true - pricing4ts@0.10.3: - resolution: {integrity: sha512-RU9s3RvU7u3jByJsciihDZgF3XQCi2GWCyqMwU6wB+Hx/oRO5Wq+FidCYQadgExDMwD9KAkP/AXIPUCnolk95Q==} + pricing4ts@0.11.1: + resolution: {integrity: sha512-Q2lTrkhvgAGyefr9dh1qN7XL3hQ+NdU5OGHjD00yHMB2YOnbfV3gMfpc5hkaxjd93R1OGUqnfExQlHxesAq0wg==} peerDependencies: minizinc: ^4.3.5 @@ -2124,6 +2124,7 @@ packages: uuid@10.0.0: resolution: {integrity: sha512-8XkAphELsDnEGrDxUOHB3RGvXz6TeuYSGEZBOjtTtPm2lwhGBjLgOzLHB63IUWfBpNucQjND6d3AOudO+H3RWQ==} + deprecated: uuid@10 and below is no longer supported. For ESM codebases, update to uuid@latest. For CommonJS codebases, use uuid@11 (but be aware this version will likely be deprecated in 2028). hasBin: true uuid@11.1.0: @@ -3940,7 +3941,7 @@ snapshots: prettier@3.5.3: {} - pricing4ts@0.10.3(@openfeature/core@1.8.0)(minizinc@4.4.3): + pricing4ts@0.11.1(@openfeature/core@1.8.0)(minizinc@4.4.3): dependencies: '@openfeature/server-sdk': 1.18.0(@openfeature/core@1.8.0) '@types/papaparse': 5.3.16 diff --git a/api/src/main/middlewares/AuthMiddleware.ts b/api/src/main/middlewares/AuthMiddleware.ts index 687c108..bb68ec2 100644 --- a/api/src/main/middlewares/AuthMiddleware.ts +++ b/api/src/main/middlewares/AuthMiddleware.ts @@ -8,6 +8,7 @@ import { import { matchPath, extractApiPath } from '../utils/routeMatcher'; import { LeanOrganization, OrganizationMember, OrganizationUserRole } from '../types/models/Organization'; import { HttpMethod, OrganizationApiKeyRole } from '../types/permissions'; +import { LeanUser } from '../types/models/User'; /** * Middleware to authenticate API Keys (both User and Organization types) @@ -19,6 +20,18 @@ import { HttpMethod, OrganizationApiKeyRole } from '../types/permissions'; * Sets req.user for User API Keys * Sets req.org for Organization API Keys */ +/** + * A credential that was read and found wanting. + * + * Distinguished from every other failure on purpose. Authentication reads the + * database, so anything that can go wrong with the database - a dropped + * connection, a replica-set election, a Mongo that is simply not running - + * surfaces as an exception here too. Answering 401 to those says the caller + * sent a bad key, which is untrue and sends whoever is debugging to look at + * their credentials. + */ +class InvalidApiKeyError extends Error {} + const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: NextFunction) => { const apiKey = req.headers['x-api-key'] as string; @@ -40,11 +53,25 @@ const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: N return checkPermissions(req, res, next); } catch (err: any) { - if (!res.headersSent) { + if (res.headersSent) { + return; + } + + if (err instanceof InvalidApiKeyError) { return res.status(401).json({ error: err.message || 'Invalid API Key', }); } + + // Anything else got as far as trying and could not finish - almost always + // the database. 503 rather than 401, because the credential was never + // judged, and Retry-After because it is worth trying again. + console.error('Authentication could not be completed:', err); + res.setHeader('Retry-After', '5'); + return res.status(503).json({ + error: 'Space cannot verify credentials right now.', + details: err?.message ?? String(err), + }); } }; @@ -54,16 +81,43 @@ const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: N async function authenticateUserApiKey(req: Request, apiKey: string): Promise { const userService = container.resolve('userService'); - const user = await userService.findByApiKey(apiKey); - - if (!user) { - throw new Error('Invalid User API Key'); - } + const user = await rejectionOrOutage( + () => userService.findByApiKey(apiKey), + 'Invalid User API Key' + ); req.user = user; req.authType = 'user'; } +/** + * Run a credential lookup, telling a refusal apart from a failure. + * + * `UserService.findByApiKey` reports an unknown key by throwing rather than by + * returning nothing, using the `INVALID DATA:` prefix this codebase gives to a + * caller's own mistake. That has to keep answering 401. Anything else thrown by + * a lookup is the database being unable to answer, which is the case this + * middleware exists to stop reporting as a bad credential. + */ +async function rejectionOrOutage(lookup: () => Promise, absent: string): Promise { + let found: T; + + try { + found = await lookup(); + } catch (err: any) { + if (typeof err?.message === 'string' && err.message.startsWith('INVALID DATA:')) { + throw new InvalidApiKeyError(err.message); + } + throw err; + } + + if (!found) { + throw new InvalidApiKeyError(absent); + } + + return found; +} + /** * Authenticates an Organization API Key and populates req.org */ @@ -71,11 +125,10 @@ async function authenticateOrgApiKey(req: Request, apiKey: string): Promise organizationRepository.findByApiKey(apiKey), + 'Invalid Organization API Key' + ); req.org = { id: result.id!, diff --git a/api/src/main/repositories/mongoose/ContractRepository.ts b/api/src/main/repositories/mongoose/ContractRepository.ts index ac5ad13..02e89e5 100644 --- a/api/src/main/repositories/mongoose/ContractRepository.ts +++ b/api/src/main/repositories/mongoose/ContractRepository.ts @@ -185,6 +185,46 @@ class ContractRepository extends RepositoryBase { return contract ? toPlainObject(contract.toJSON()) : null; } + /** + * Add to several usage levels of one contract in a single atomic update. + * + * `$inc` is evaluated by the database against the stored document rather than + * against a copy the process read earlier, so concurrent increments compose + * instead of overwriting one another. Read-modify-write cannot do this from + * the application: two callers who read the same value both write the same + * total, and one consumption disappears. + * + * Every path is required to exist by the filter, so a limit that is not part + * of the contract matches no document and is reported to the caller instead + * of being created by the update - `$inc` would otherwise happily add the + * field. That check and the increment are one operation, so a limit cannot be + * validated and then vanish before the write. + * + * @param increments usage level path (`service.limit`) to amount to add. + * @returns the contract as it is after the increment, or null when the filter + * matched nothing. + */ + async incrementUsageLevels( + userId: string, + increments: Record + ): Promise { + const filter: Record = { 'userContact.userId': userId }; + const inc: Record = {}; + + for (const [path, amount] of Object.entries(increments)) { + filter[`usageLevels.${path}.consumed`] = { $exists: true }; + inc[`usageLevels.${path}.consumed`] = amount; + } + + const contract = await ContractMongoose.findOneAndUpdate( + filter, + { $inc: inc }, + { new: true } + ); + + return contract ? toPlainObject(contract.toJSON()) : null; + } + async changeServiceName(oldServiceName: string, newServiceName: string, organizationId: string): Promise { const oldServiceKey = oldServiceName.toLowerCase(); const newServiceKey = newServiceName.toLowerCase(); diff --git a/api/src/main/routes/HealthcheckRoutes.ts b/api/src/main/routes/HealthcheckRoutes.ts index c4a92d0..e89b8f3 100644 --- a/api/src/main/routes/HealthcheckRoutes.ts +++ b/api/src/main/routes/HealthcheckRoutes.ts @@ -1,18 +1,49 @@ import express from 'express'; +import mongoose from 'mongoose'; + +/** + * Mongoose reports 1 when the driver has a usable connection. + * + * 2 is "still connecting", which matters at start-up: a container answering + * "healthy" while it is still dialling is one an orchestrator will start + * sending traffic to. + */ +const CONNECTED = 1; const loadFileRoutes = function (app: express.Application) { const baseUrl = '/api/v1'; // Public route for authentication (does not require API Key) - app - .route(`${baseUrl}/healthcheck`) - .get( - (req: any, res: any) => { - res.status(200).json({ - message: 'Service is up and running!', - }); - } - ); + app.route(`${baseUrl}/healthcheck`).get(async (req: any, res: any) => { + // The database is not a detail of this service, it is the service: every + // authenticated route reads it before it can answer anything. A check that + // proves only the HTTP listener is up reports a Space that cannot serve a + // single request as healthy, and keeps reporting it indefinitely while + // every call fails. + if (mongoose.connection.readyState !== CONNECTED) { + return res.status(503).json({ + message: 'Service is up but cannot reach its database.', + database: 'disconnected', + }); + } + + try { + // readyState is what the driver believes. A ping is what the database + // says, and the two disagree when a connection has gone stale. + await mongoose.connection.db!.admin().ping(); + } catch (error: any) { + return res.status(503).json({ + message: 'Service is up but its database is not answering.', + database: 'unreachable', + details: error?.message ?? String(error), + }); + } + + res.status(200).json({ + message: 'Service is up and running!', + database: 'connected', + }); + }); }; export default loadFileRoutes; diff --git a/api/src/main/services/ContractService.ts b/api/src/main/services/ContractService.ts index bb0b6cb..0df8ee4 100644 --- a/api/src/main/services/ContractService.ts +++ b/api/src/main/services/ContractService.ts @@ -459,38 +459,95 @@ class ContractService { usageLimitId: string, expectedConsumption: number ): Promise { - let contract = await this.cacheService.get(`contracts.${userId}`); + await this._applyExpectedConsumptions(userId, { [usageLimitId]: expectedConsumption }); + } - if (!contract) { - contract = await this.contractRepository.findByUserId(userId); + /** + * Apply several expected consumptions to a contract in one atomic update. + * + * Read-modify-write loses consumptions, and it does so in two ways. Applying + * the limits of one evaluation in turn made each application read the whole + * contract, change a single usage level in its own copy and write the whole + * contract back, so an evaluation touching two limits recorded one of them. + * Batching the limits into a single read and write fixes that, but not the + * case of two requests arriving together: both read the same consumed value, + * both write the same total, and one consumption is gone. + * + * The database is the only place that can settle this, so the increment is + * handed to it as `$inc` and evaluated against the stored document. Whatever + * order concurrent calls arrive in, every one of them is added. + */ + async _applyExpectedConsumptions( + userId: string, + expectedConsumptions: Record + ): Promise { + const usageLimitIds = Object.keys(expectedConsumptions); + if (usageLimitIds.length === 0) { + return; } - if (!contract) { - throw new Error(`Contract with userId ${userId} not found`); + const targets = usageLimitIds.map(usageLimitId => { + const serviceName: string = usageLimitId.split('-')[0]; + const usageLimit: string = usageLimitId.split('-')[1]; + + return { serviceName, usageLimit, amount: expectedConsumptions[usageLimitId] }; + }); + + const increments: Record = {}; + for (const { serviceName, usageLimit, amount } of targets) { + increments[`${serviceName}.${usageLimit}`] = amount; + } + + const updatedContract = await this.contractRepository.incrementUsageLevels(userId, increments); + + if (!updatedContract) { + // The update requires the contract and every named usage level to exist, + // so it matched nothing. Which of the two is missing only matters for the + // message, and is worth a read to get right. + await this._explainMissingUsageLevels(userId, targets); } - const serviceName: string = usageLimitId.split('-')[0]; - const usageLimit: string = usageLimitId.split('-')[1]; + const appliedAt = new Date().getTime(); - if (contract.usageLevels[serviceName][usageLimit]) { + for (const { serviceName, usageLimit, amount } of targets) { + // What the level held before this call, for `_revertExpectedConsumption`. + // Derived from the result rather than from a prior read, so it is this + // caller's own contribution that gets taken back even if others landed in + // between. await this.cacheService.set( - `${new Date().getTime()}.usageLevels.${userId}.${serviceName}.${usageLimit}`, - contract.usageLevels[serviceName][usageLimit].consumed, + `${appliedAt}.usageLevels.${userId}.${serviceName}.${usageLimit}`, + updatedContract!.usageLevels[serviceName][usageLimit].consumed - amount, 120 ); // 120 secs = 2 mins + } - contract.usageLevels[serviceName][usageLimit].consumed += expectedConsumption; + await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour + } - const updatedContract = await this.contractRepository.update(userId, contract); + /** + * Say which part of the contract was missing, having established that one was. + */ + private async _explainMissingUsageLevels( + userId: string, + targets: { serviceName: string; usageLimit: string }[] + ): Promise { + const contract = await this.contractRepository.findByUserId(userId); - if (!updatedContract) { - throw new Error(`Failed to update contract for userId ${userId}`); - } + if (!contract) { + throw new Error(`Contract with userId ${userId} not found`); + } - await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour - } else { - throw new Error(`Usage level ${usageLimit} not found in contract for userId ${userId}`); + const missing = targets.find( + ({ serviceName, usageLimit }) => !contract.usageLevels[serviceName]?.[usageLimit] + ); + + if (missing) { + throw new Error( + `Usage level ${missing.usageLimit} not found in contract for userId ${userId}` + ); } + + throw new Error(`Failed to update contract for userId ${userId}`); } async _revertExpectedConsumption( diff --git a/api/src/main/utils/feature-evaluation/featureEvaluation.ts b/api/src/main/utils/feature-evaluation/featureEvaluation.ts index fd0fd9e..61dc3b4 100644 --- a/api/src/main/utils/feature-evaluation/featureEvaluation.ts +++ b/api/src/main/utils/feature-evaluation/featureEvaluation.ts @@ -61,20 +61,21 @@ async function evaluateFeature( } } - // Then apply all consumptions after validation has passed + // Then apply all consumptions after validation has passed. + // + // In one call rather than one per limit: each application reads the whole + // contract, increments one usage level and writes the whole contract + // back, so running them concurrently made every one of them start from + // the same state and only the last write survive. if (options.userId) { const contractService: ContractService = container.resolve('contractService'); - const limits = Object.keys(featureEvaluation.used); - await Promise.all( - limits.map(limit => - contractService._applyExpectedConsumption( - options.userId!, - limit, - expectedConsumption[limit] - ) - ) - ); + const consumptions: Record = {}; + for (const limit of Object.keys(featureEvaluation.used)) { + consumptions[limit] = expectedConsumption[limit]; + } + + await contractService._applyExpectedConsumptions(options.userId, consumptions); } } } @@ -226,7 +227,7 @@ function _buildSuccessResult( subscriptionContext[limitKey], expectedConsumption[limitKey] ); - if (!updatedUsageLevel) { + if (updatedUsageLevel === undefined) { return _createErrorResult( 'INVALID_EXPECTED_CONSUMPTION', `No expectedConsumption value was provided for limit '${limitKey}', which is used in the evaluation of feature '${featureId}'. Please note that if you provide an expectedConsumption for any limit, you must provide it for all limits involved in that feature's evaluation.` @@ -250,7 +251,10 @@ function _updateUsageLevel( currentUsageLevel: number, expectedConsumption?: number ): number | undefined { - if (!expectedConsumption) { + // `undefined` means the caller did not mention this limit, which is the + // error the caller is told about. `0` means they mentioned it and it costs + // nothing - a different statement, and one a falsy check cannot tell apart. + if (expectedConsumption === undefined || expectedConsumption === null) { return undefined; } diff --git a/api/src/test/contract.expected-consumption.test.ts b/api/src/test/contract.expected-consumption.test.ts new file mode 100644 index 0000000..d903f54 --- /dev/null +++ b/api/src/test/contract.expected-consumption.test.ts @@ -0,0 +1,262 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import container from '../main/config/container'; + +/** + * Applying expected consumption to usage limits. + * + * Read-modify-write loses consumptions in two ways. Applying the limits of one + * evaluation in turn made each application read the whole contract, change one + * usage level in its own copy and write the whole contract back, so an + * evaluation touching two limits recorded one of them. Batching them into a + * single read and write fixed that, and left the other: two requests arriving + * together both read the same consumed value, both write the same total, and + * one consumption disappears. + * + * The increment is now handed to the database as `$inc`, which is the only + * place it can be settled. + * + * These tests work against a stubbed repository and cache so they can assert on + * the reads and writes themselves, which is where the defect lived. The stub + * models the database honestly: `incrementUsageLevels` adds to whatever is + * *stored* at the moment it runs, which is exactly the guarantee `$inc` gives + * and exactly the one an application-side read-modify-write cannot. + */ + +function aContract() { + return { + userContact: { userId: 'user1', username: 'user1' }, + contractedServices: { petclinic: '2025' }, + subscriptionPlans: { petclinic: 'BASIC' }, + usageLevels: { + petclinic: { + maxPets: { consumed: 0 }, + maxVisits: { consumed: 0 }, + }, + }, + }; +} + +const copy = (value: any) => JSON.parse(JSON.stringify(value)); + +function withStubs(contract: any) { + const state = { current: contract }; + let writes = 0; + let reads = 0; + let increments = 0; + + const contractRepository = { + findByUserId: vi.fn(async () => { + reads += 1; + return copy(state.current); + }), + + // Kept so a regression to read-modify-write is visible rather than a crash. + // The await between reading and writing is what any real round trip has, + // and what lets a second caller slip in between the two. + update: vi.fn(async (_userId: string, updated: any) => { + writes += 1; + await Promise.resolve(); + state.current = copy(updated); + return copy(state.current); + }), + + incrementUsageLevels: vi.fn(async (_userId: string, byPath: Record) => { + increments += 1; + await Promise.resolve(); + + // The filter requires every path to exist; a miss matches no document. + for (const path of Object.keys(byPath)) { + const [serviceName, usageLimit] = path.split('.'); + if (!state.current.usageLevels[serviceName]?.[usageLimit]) { + return null; + } + } + + for (const [path, amount] of Object.entries(byPath)) { + const [serviceName, usageLimit] = path.split('.'); + state.current.usageLevels[serviceName][usageLimit].consumed += amount; + } + + return copy(state.current); + }), + }; + + const cacheService = { + get: vi.fn(async () => null), + set: vi.fn(async () => undefined), + del: vi.fn(async () => undefined), + }; + + const original = container.resolve.bind(container); + vi.spyOn(container, 'resolve').mockImplementation((name: any) => { + if (name === 'contractRepository') return contractRepository as any; + if (name === 'cacheService') return cacheService as any; + return original(name); + }); + + return { + state, + contractRepository, + cacheService, + counts: () => ({ reads, writes, increments }), + consumed: () => state.current.usageLevels.petclinic, + }; +} + +async function aService() { + const { default: ContractService } = await import('../main/services/ContractService'); + return new (ContractService as any)(); +} + +describe('Applying expected consumption', () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + it('records every limit, not just the last one written', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 3, + }); + + expect(stubs.consumed().maxPets.consumed).toBe(1); + expect(stubs.consumed().maxVisits.consumed).toBe(3); + }); + + it('loses nothing when two requests arrive together', async () => { + // The case the batch alone could not fix, and the reason for `$inc`: two + // callers spending the same limit at the same time. Under read-modify-write + // both start from 0, both write 1, and one consumption is gone. + const stubs = withStubs(aContract()); + const service = await aService(); + + await Promise.all([ + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1), + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1), + ]); + + expect(stubs.consumed().maxPets.consumed).toBe(2); + }); + + it('loses nothing across many concurrent requests', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await Promise.all( + Array.from({ length: 20 }, () => + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1) + ) + ); + + expect(stubs.consumed().maxPets.consumed).toBe(20); + }); + + it('composes concurrent requests that touch different limits', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await Promise.all([ + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1), + service._applyExpectedConsumption('user1', 'petclinic-maxVisits', 1), + ]); + + expect(stubs.consumed().maxPets.consumed).toBe(1); + expect(stubs.consumed().maxVisits.consumed).toBe(1); + }); + + it('touches the contract once however many limits there are', async () => { + // Not only correctness: one round trip instead of one per limit, and no + // read at all, since the database does the arithmetic. + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 1, + }); + + expect(stubs.counts()).toEqual({ reads: 0, writes: 0, increments: 1 }); + }); + + it('still applies a single limit', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumption('user1', 'petclinic-maxPets', 2); + + expect(stubs.consumed().maxPets.consumed).toBe(2); + }); + + it('does nothing at all when given nothing', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', {}); + + expect(stubs.counts()).toEqual({ reads: 0, writes: 0, increments: 0 }); + }); + + it('refuses the whole request when one limit does not exist', async () => { + // Rather than applying the valid ones and then failing, which would leave + // the contract half-updated. The check is part of the same operation, so a + // limit cannot be validated and then vanish before the write. + const stubs = withStubs(aContract()); + const service = await aService(); + + await expect( + service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-nosuchlimit': 1, + }) + ).rejects.toThrow(/not found in contract/); + + expect(stubs.consumed().maxPets.consumed).toBe(0); + }); + + it('says so when there is no contract at all', async () => { + const stubs = withStubs(aContract()); + stubs.contractRepository.incrementUsageLevels.mockResolvedValue(null); + stubs.contractRepository.findByUserId.mockResolvedValue(null); + const service = await aService(); + + await expect( + service._applyExpectedConsumptions('user1', { 'petclinic-maxPets': 1 }) + ).rejects.toThrow(/Contract with userId user1 not found/); + }); + + it('keeps the previous value of every limit for reverting', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 1, + }); + + const cachedKeys = stubs.cacheService.set.mock.calls.map((call: any[]) => call[0]); + expect(cachedKeys.some((key: string) => key.includes('maxPets'))).toBe(true); + expect(cachedKeys.some((key: string) => key.includes('maxVisits'))).toBe(true); + }); + + it('records this caller’s own starting point, not whatever it read', async () => { + // The snapshot kept for reverting is derived from the result of the + // increment, so it is this caller's contribution that gets taken back even + // when other calls landed in between. + const contract = aContract(); + contract.usageLevels.petclinic.maxPets.consumed = 7; + + const stubs = withStubs(contract); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { 'petclinic-maxPets': 3 }); + + const snapshot = (stubs.cacheService.set.mock.calls as any[][]).find(call => + String(call[0]).includes('maxPets') + ); + expect(snapshot?.[1]).toBe(7); + expect(stubs.consumed().maxPets.consumed).toBe(10); + }); +}); diff --git a/api/src/test/feature-evaluation.zero-consumption.test.ts b/api/src/test/feature-evaluation.zero-consumption.test.ts new file mode 100644 index 0000000..153205f --- /dev/null +++ b/api/src/test/feature-evaluation.zero-consumption.test.ts @@ -0,0 +1,112 @@ +import { describe, it, expect } from 'vitest'; +import { evaluateFeature } from '../main/utils/feature-evaluation/featureEvaluation'; +import type { + EvaluationContext, + FeatureEvaluationResult, + PricingContext, + SubscriptionContext, +} from '../main/types/models/FeatureEvaluation'; + +/** + * An expected consumption of zero. + * + * A caller who provides `expectedConsumption` must provide it for every limit + * involved in the feature's evaluation, or be refused. So the only way to say + * "this limit takes part in the evaluation but this call does not spend it" is + * to pass zero - which two falsy checks rejected as if the limit had been left + * out altogether. + * + * The second of those checks also refused a perfectly ordinary positive + * consumption, whenever the current usage level happened to be zero: a brand + * new contract, or the first call of a renewal period. + */ + +const FEATURE = 'petclinic-pets'; +const LIMIT = 'petclinic-maxPets'; + +const EXPRESSION = `subscriptionContext['${LIMIT}'] < pricingContext['usageLimits']['${LIMIT}']`; + +const pricingContext: PricingContext = { + features: { [FEATURE]: true }, + usageLimits: { [LIMIT]: 10 }, +}; + +const evaluationContext: EvaluationContext = { [FEATURE]: EXPRESSION }; + +/** @param usageLevel what the contract has consumed so far. */ +async function evaluate(usageLevel: number, expectedConsumption?: Record) { + const subscriptionContext: SubscriptionContext = { [LIMIT]: usageLevel }; + + return (await evaluateFeature(FEATURE, pricingContext, subscriptionContext, evaluationContext, { + simple: false, + expectedConsumption, + // No userId, so nothing is written: this is about the verdict, not the + // bookkeeping that follows it. + })) as FeatureEvaluationResult; +} + +describe('expectedConsumption of zero', () => { + it('is accepted, and leaves the usage level where it was', async () => { + const result = await evaluate(5, { [LIMIT]: 0 }); + + expect(result.error).toBeNull(); + expect(result.eval).toBe(true); + expect(result.used).toEqual({ [LIMIT]: 5 }); + }); + + it('is accepted on a contract that has consumed nothing yet', async () => { + // Both zeroes at once: the usage level and the consumption. This is the + // case a `!updatedUsageLevel` check gets wrong even after `0 + 0` has been + // computed correctly. + const result = await evaluate(0, { [LIMIT]: 0 }); + + expect(result.error).toBeNull(); + expect(result.used).toEqual({ [LIMIT]: 0 }); + }); + + it('is not reported as a missing value', async () => { + const result = await evaluate(0, { [LIMIT]: 0 }); + + expect(result.error?.code).not.toBe('INVALID_EXPECTED_CONSUMPTION'); + }); +}); + +describe('expectedConsumption on an untouched usage level', () => { + it('adds to a usage level of zero', async () => { + // Not about zero consumption at all: a plain consumption of 1 on a brand + // new contract. `1` is truthy, but only because the addition happens to + // leave a truthy total. + const result = await evaluate(0, { [LIMIT]: 1 }); + + expect(result.error).toBeNull(); + expect(result.used).toEqual({ [LIMIT]: 1 }); + }); + + it('adds to a non-zero usage level, as before', async () => { + const result = await evaluate(5, { [LIMIT]: 3 }); + + expect(result.used).toEqual({ [LIMIT]: 8 }); + }); +}); + +describe('expectedConsumption that really is missing', () => { + it('is still refused when the limit is left out', async () => { + const result = await evaluate(5, { 'petclinic-someOtherLimit': 1 }); + + expect(result.error?.code).toBe('INVALID_EXPECTED_CONSUMPTION'); + }); + + it('reports the current usage level when no consumption is given at all', async () => { + const result = await evaluate(5, undefined); + + expect(result.error).toBeNull(); + expect(result.used).toEqual({ [LIMIT]: 5 }); + }); + + it('treats an empty object as no consumption', async () => { + const result = await evaluate(5, {}); + + expect(result.error).toBeNull(); + expect(result.used).toEqual({ [LIMIT]: 5 }); + }); +}); diff --git a/api/src/test/middlewares/authOutage.test.ts b/api/src/test/middlewares/authOutage.test.ts new file mode 100644 index 0000000..890ec45 --- /dev/null +++ b/api/src/test/middlewares/authOutage.test.ts @@ -0,0 +1,93 @@ +import request from 'supertest'; +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { Server } from 'http'; +import { getApp, shutdownApp, baseUrl } from '../utils/testApp'; +import { createTestUser, deleteTestUser } from '../utils/users/userTestUtils'; +import container from '../../main/config/container'; +import { LeanUser } from '../../main/types/models/User'; + +/** + * Telling "your key is wrong" apart from "we could not check your key". + * + * Authentication reads the database, so a dropped connection, a replica-set + * election or a Mongo that is simply not running all surface as an exception in + * the same place an unknown key does. Answering 401 to those states something + * untrue about the caller's credential and sends whoever is debugging to look + * at their API key, which is the one place the problem is not. + * + * Both directions are pinned here, because the interesting part of the change + * is the boundary: a refusal must stay a refusal. + */ +describe('Authentication when the database cannot answer', function () { + let app: Server; + let user: LeanUser; + + beforeAll(async function () { + app = await getApp(); + user = await createTestUser('ADMIN'); + }); + + afterAll(async function () { + await deleteTestUser(user.username); + vi.restoreAllMocks(); + await shutdownApp(); + }); + + it('answers 503, not 401, when the lookup fails', async function () { + const userService: any = container.resolve('userService'); + const outage = vi + .spyOn(userService, 'findByApiKey') + .mockRejectedValue(new Error('MongooseServerSelectionError: connection timed out')); + + try { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', user.apiKey); + + expect(response.status).toBe(503); + expect(response.body.error).toContain('cannot verify credentials'); + } finally { + outage.mockRestore(); + } + }); + + it('asks the caller to try again', async function () { + const userService: any = container.resolve('userService'); + const outage = vi + .spyOn(userService, 'findByApiKey') + .mockRejectedValue(new Error('MongooseServerSelectionError: connection timed out')); + + try { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', user.apiKey); + + // A 503 without Retry-After tells a client nothing about whether waiting + // is worth it. + expect(response.headers['retry-after']).toBeDefined(); + } finally { + outage.mockRestore(); + } + }); + + it('still answers 401 to a key that was read and rejected', async function () { + // The regression this pairs with. `UserService.findByApiKey` reports an + // unknown key by throwing rather than by returning nothing, so narrowing + // the catch to a dedicated error type is not by itself enough to keep this + // a 401. + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', 'usr_nosuchkeyatall'); + + expect(response.status).toBe(401); + expect(response.body.error).toContain('INVALID DATA: Invalid API Key'); + }); + + it('still answers 401 to a key of no recognisable kind', async function () { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', 'not-a-prefixed-key'); + + expect(response.status).toBe(401); + }); +}); diff --git a/scripts/test-rate-limit-production.ts b/scripts/test-rate-limit-production.ts new file mode 100644 index 0000000..53ef7b4 --- /dev/null +++ b/scripts/test-rate-limit-production.ts @@ -0,0 +1,76 @@ +import https from "node:https"; + +const TOTAL_REQUESTS = 200; +const CONCURRENT_REQUESTS = 50; + +const BASE_URL = + process.env.RATE_LIMIT_URL ?? + "https://space.score.us.es/api/v1/healthcheck"; + +process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; + +function makeRequest(): Promise { + return new Promise((resolve) => { + const req = https.get(BASE_URL, (res) => { + resolve(res.statusCode ?? 0); + res.resume(); + }); + + req.on("error", () => { + resolve(0); + }); + }); +} + +async function runBatch(size: number): Promise { + return Promise.all( + Array.from({ length: size }, () => makeRequest()) + ); +} + +async function main(): Promise { + const results: number[] = []; + + for ( + let i = 0; + i < TOTAL_REQUESTS; + i += CONCURRENT_REQUESTS + ) { + const batch = await runBatch(CONCURRENT_REQUESTS); + results.push(...batch); + } + + const grouped = results.reduce>( + (acc, code) => { + acc[code] = (acc[code] ?? 0) + 1; + return acc; + }, + {} + ); + + console.log("Response distribution:"); + console.log(grouped); + + const has429 = (grouped[429] ?? 0) > 0; + const has200 = (grouped[200] ?? 0) > 0; + + if (!has429) { + console.error( + "Rate limiting did not trigger any 429 responses." + ); + + process.exit(1); + } + + if (!has200) { + console.error( + "All requests failed. Backend may be unhealthy." + ); + + process.exit(1); + } + + console.log("Rate limiting is working."); +} + +main(); \ No newline at end of file