diff --git a/backend/src/analytic/analytic.controller.ts b/backend/src/analytic/analytic.controller.ts index 7c64eea9..f3993ba3 100644 --- a/backend/src/analytic/analytic.controller.ts +++ b/backend/src/analytic/analytic.controller.ts @@ -9,8 +9,12 @@ import { Logger, Query, OnModuleInit, + UseGuards, } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; import { AnalyticService } from './analytic.service'; +import { OwnershipGuard } from '../common/guards/ownership.guard'; +import { Ownership } from '../common/decorators/ownership.decorator'; import type { PaginatedUserPuzzleHistory } from './analytic.service'; class RecordSolveDto { @@ -71,6 +75,8 @@ export class AnalyticController implements OnModuleInit { } @Get('users/:userId/history') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) async getUserPuzzleHistory( @Param('userId') userId: string, ): Promise> { @@ -86,6 +92,8 @@ export class AnalyticController implements OnModuleInit { } @Get('users/:userId/history/paginated') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) async getUserPuzzleHistoryPaginated( @Param('userId') userId: string, @Query('page') page?: string, diff --git a/backend/src/api-key/api-key.controller.spec.ts b/backend/src/api-key/api-key.controller.spec.ts index 3379bd61..ede66c5c 100644 --- a/backend/src/api-key/api-key.controller.spec.ts +++ b/backend/src/api-key/api-key.controller.spec.ts @@ -8,6 +8,8 @@ describe('ApiKeyController', () => { const mockKey = { key: 'k-1', + keyHash: 'hash-1', + keyHint: 'k-1', ownerLabel: 'owner', status: ApiKeyStatus.ACTIVE, createdAt: new Date(), @@ -16,6 +18,7 @@ describe('ApiKeyController', () => { beforeEach(async () => { serviceMock = { generateApiKey: jest.fn().mockReturnValue(mockKey), + rotateApiKey: jest.fn().mockReturnValue({ ...mockKey, key: 'k-2' }), revokeApiKey: jest.fn().mockReturnValue({ ...mockKey, status: ApiKeyStatus.REVOKED }), getAllApiKeys: jest.fn().mockReturnValue([mockKey]), validateApiKey: jest.fn().mockReturnValue(true), @@ -39,6 +42,12 @@ describe('ApiKeyController', () => { expect(serviceMock.generateApiKey).toHaveBeenCalledWith('owner', true, undefined); }); + it('rotates API key', () => { + const res = controller.rotateApiKey('k-1', { isAdmin: true }); + expect(res.key).toBe('k-2'); + expect(serviceMock.rotateApiKey).toHaveBeenCalledWith('k-1', true); + }); + it('revokes API key', () => { const res = controller.revokeApiKey('k-1', { isAdmin: true }); expect(res.status).toBe(ApiKeyStatus.REVOKED); diff --git a/backend/src/api-key/api-key.controller.ts b/backend/src/api-key/api-key.controller.ts index 12b72426..738f0c0f 100644 --- a/backend/src/api-key/api-key.controller.ts +++ b/backend/src/api-key/api-key.controller.ts @@ -10,7 +10,12 @@ import { Logger, Query, } from '@nestjs/common'; -import { ApiKeyService, ApiKey, ApiKeyStatus } from './api-key.service'; +import { + ApiKeyService, + ApiKey, + ApiKeyRecord, + ApiKeyStatus, +} from './api-key.service'; import { IsString, IsNotEmpty, @@ -51,7 +56,7 @@ export class ApiKeyController { @HttpCode(HttpStatus.CREATED) generateApiKey(@Body() generateApiKeyDto: GenerateApiKeyDto): ApiKey { this.logger.log( - `Received request to generate API key: ${JSON.stringify(generateApiKeyDto)}`, + `Received request to generate API key for: ${generateApiKeyDto.ownerLabel}`, ); return this.apiKeyService.generateApiKey( generateApiKeyDto.ownerLabel, @@ -62,20 +67,32 @@ export class ApiKeyController { ); } + @Post('rotate/:key') + @HttpCode(HttpStatus.OK) + rotateApiKey( + @Param('key') key: string, + @Body() adminActionDto: AdminActionDto, + ): ApiKey { + this.logger.log( + `Received request to rotate API key (isAdmin: ${adminActionDto.isAdmin}).`, + ); + return this.apiKeyService.rotateApiKey(key, adminActionDto.isAdmin); + } + @Post('revoke/:key') @HttpCode(HttpStatus.OK) revokeApiKey( @Param('key') key: string, @Body() adminActionDto: AdminActionDto, - ): ApiKey { + ): ApiKeyRecord { this.logger.log( - `Received request to revoke API key ${key} (isAdmin: ${adminActionDto.isAdmin}).`, + `Received request to revoke API key (isAdmin: ${adminActionDto.isAdmin}).`, ); return this.apiKeyService.revokeApiKey(key, adminActionDto.isAdmin); } @Get('all') - getAllApiKeys(@Query() adminActionDto: AdminActionDto): ApiKey[] { + getAllApiKeys(@Query() adminActionDto: AdminActionDto): ApiKeyRecord[] { this.logger.log( `Received request to get all API keys (isAdmin: ${adminActionDto.isAdmin}).`, ); diff --git a/backend/src/api-key/api-key.guard.spec.ts b/backend/src/api-key/api-key.guard.spec.ts new file mode 100644 index 00000000..3f4bb053 --- /dev/null +++ b/backend/src/api-key/api-key.guard.spec.ts @@ -0,0 +1,71 @@ +import { Test } from '@nestjs/testing'; +import { UnauthorizedException } from '@nestjs/common'; +import { APIKeyGuard } from './api-key.guard'; +import { ApiKeyService } from './api-key.service'; + +function mockContext(request: Record): any { + return { + switchToHttp: () => ({ getRequest: () => request }), + }; +} + +describe('APIKeyGuard', () => { + let guard: APIKeyGuard; + let serviceMock: { validateApiKey: jest.Mock }; + + beforeEach(async () => { + serviceMock = { validateApiKey: jest.fn() }; + const moduleRef = await Test.createTestingModule({ + providers: [ + APIKeyGuard, + { provide: ApiKeyService, useValue: serviceMock }, + ], + }).compile(); + guard = moduleRef.get(APIKeyGuard); + }); + + it('rejects requests without an x-api-key header', async () => { + const ctx = mockContext({ headers: {}, path: '/api-keys/protected' }); + await expect(guard.canActivate(ctx)).rejects.toThrow(UnauthorizedException); + }); + + it('passes the route pattern to the service for scope checks', async () => { + serviceMock.validateApiKey.mockReturnValue(true); + const ctx = mockContext({ + headers: { 'x-api-key': 'sh_valid' }, + route: { path: '/api-keys/protected' }, + path: '/api/v1/api-keys/protected', + }); + await expect(guard.canActivate(ctx)).resolves.toBe(true); + expect(serviceMock.validateApiKey).toHaveBeenCalledWith( + 'sh_valid', + '/api-keys/protected', + ); + }); + + it('rejects keys the service deems invalid for the endpoint', async () => { + serviceMock.validateApiKey.mockReturnValue(false); + const ctx = mockContext({ + headers: { 'x-api-key': 'sh_scoped-elsewhere' }, + route: { path: '/api-keys/protected' }, + }); + await expect(guard.canActivate(ctx)).rejects.toThrow(UnauthorizedException); + expect(serviceMock.validateApiKey).toHaveBeenCalledWith( + 'sh_scoped-elsewhere', + '/api-keys/protected', + ); + }); + + it('falls back to the raw path when no route pattern is available', async () => { + serviceMock.validateApiKey.mockReturnValue(true); + const ctx = mockContext({ + headers: { 'x-api-key': 'sh_valid' }, + path: '/some/raw/path', + }); + await expect(guard.canActivate(ctx)).resolves.toBe(true); + expect(serviceMock.validateApiKey).toHaveBeenCalledWith( + 'sh_valid', + '/some/raw/path', + ); + }); +}); diff --git a/backend/src/api-key/api-key.guard.ts b/backend/src/api-key/api-key.guard.ts index 34f2cc7e..ee9a946a 100644 --- a/backend/src/api-key/api-key.guard.ts +++ b/backend/src/api-key/api-key.guard.ts @@ -23,10 +23,15 @@ export class APIKeyGuard implements CanActivate { throw new UnauthorizedException('API Key missing'); } - const isValid = this.apiKeyService.validateApiKey(apiKey); + // Match against the route pattern (e.g. `/api-keys/protected`) so + // scope checks are stable regardless of the global `/api/v1` prefix. + const endpoint = + (request.route?.path as string | undefined) ?? request.path; + + const isValid = this.apiKeyService.validateApiKey(apiKey, endpoint); if (!isValid) { - this.logger.warn(`Invalid API Key: ${apiKey}`); + this.logger.warn('Invalid API Key for requested endpoint.'); throw new UnauthorizedException('Invalid API Key'); } diff --git a/backend/src/api-key/api-key.service.spec.ts b/backend/src/api-key/api-key.service.spec.ts index 26de070d..2aad7c16 100644 --- a/backend/src/api-key/api-key.service.spec.ts +++ b/backend/src/api-key/api-key.service.spec.ts @@ -1,6 +1,15 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { UnauthorizedException, NotFoundException, BadRequestException } from '@nestjs/common'; -import { ApiKeyService, ApiKeyStatus } from './api-key.service'; +import { + UnauthorizedException, + NotFoundException, + BadRequestException, +} from '@nestjs/common'; +import { + ApiKeyService, + ApiKeyStatus, + hashApiKey, + constantTimeEqual, +} from './api-key.service'; describe('ApiKeyService', () => { let service: ApiKeyService; @@ -26,11 +35,133 @@ describe('ApiKeyService', () => { }); it('throws UnauthorizedException if non-admin attempts generation', () => { - expect(() => service.generateApiKey('my-app', false)).toThrow(UnauthorizedException); + expect(() => service.generateApiKey('my-app', false)).toThrow( + UnauthorizedException, + ); }); it('throws BadRequestException if owner label is empty', () => { - expect(() => service.generateApiKey(' ', true)).toThrow(BadRequestException); + expect(() => service.generateApiKey(' ', true)).toThrow( + BadRequestException, + ); + }); + + it('never persists the raw secret — only a hash and a display hint', () => { + const keyObj = service.generateApiKey('hash-me', true); + const stored = service.getAllApiKeys(true); + const record = stored.find((r) => r.keyHash === hashApiKey(keyObj.key)); + + expect(record).toBeDefined(); + expect(record!.keyHint).toBe(keyObj.key.slice(-4)); + // The raw key must not appear anywhere in stored records. + for (const r of stored) { + expect(JSON.stringify(r)).not.toContain(keyObj.key); + expect(r.keyHash).not.toBe(keyObj.key); + } + expect(hashApiKey(keyObj.key)).not.toBe(keyObj.key); + }); + + it('generates unique raw secrets', () => { + const a = service.generateApiKey('a', true); + const b = service.generateApiKey('b', true); + expect(a.key).not.toBe(b.key); + }); + }); + + describe('constant-time verification', () => { + it('accepts a matching digest', () => { + const key = 'sh_secret-value'; + expect(constantTimeEqual(hashApiKey(key), hashApiKey(key))).toBe(true); + }); + + it('rejects a different digest', () => { + expect( + constantTimeEqual(hashApiKey('sh_one'), hashApiKey('sh_two')), + ).toBe(false); + }); + + it('rejects inputs of different lengths without throwing', () => { + expect(constantTimeEqual('a', 'bb')).toBe(false); + }); + + it('rejects a tampered key during validation', () => { + const keyObj = service.generateApiKey('tamper', true); + const tampered = `${keyObj.key.slice(0, -1)}x`; + expect(service.validateApiKey(tampered)).toBe(false); + expect(service.validateApiKey(keyObj.key)).toBe(true); + }); + }); + + describe('scope enforcement', () => { + it('allows an unscoped key on any endpoint', () => { + const keyObj = service.generateApiKey('unscoped', true); + expect(service.validateApiKey(keyObj.key, '/anything')).toBe(true); + }); + + it('allows a scoped key only on its permitted endpoints', () => { + const keyObj = service.generateApiKey('scoped', true, undefined, 1000, 100, [ + '/api-keys/protected', + ]); + expect(service.validateApiKey(keyObj.key, '/api-keys/protected')).toBe( + true, + ); + expect(service.validateApiKey(keyObj.key, '/api-keys/protected/x')).toBe( + true, + ); + expect(service.validateApiKey(keyObj.key, '/other/route')).toBe(false); + }); + + it('supports wildcard scopes', () => { + const keyObj = service.generateApiKey('wildcard', true, undefined, 1000, 100, [ + '/puzzles/*', + ]); + expect(service.validateApiKey(keyObj.key, '/puzzles')).toBe(true); + expect(service.validateApiKey(keyObj.key, '/puzzles/42')).toBe(true); + expect(service.validateApiKey(keyObj.key, '/users/1')).toBe(false); + }); + }); + + describe('rotateApiKey', () => { + it('invalidates the old secret and issues a new one', () => { + const keyObj = service.generateApiKey('rotate-me', true); + expect(service.validateApiKey(keyObj.key)).toBe(true); + + const rotated = service.rotateApiKey(keyObj.key, true); + expect(rotated.key).not.toBe(keyObj.key); + expect(rotated.ownerLabel).toBe('rotate-me'); + expect(service.validateApiKey(keyObj.key)).toBe(false); + expect(service.validateApiKey(rotated.key)).toBe(true); + }); + + it('preserves metadata (quota, scopes) across rotation', () => { + const keyObj = service.generateApiKey('rotate-meta', true, undefined, 500, 50, [ + '/api-keys/protected', + ]); + const rotated = service.rotateApiKey(keyObj.key, true); + expect(rotated.monthlyRequestQuota).toBe(500); + expect(rotated.rateLimitPerMinute).toBe(50); + expect(rotated.scopedEndpoints).toEqual(['/api-keys/protected']); + }); + + it('throws UnauthorizedException if non-admin attempts rotation', () => { + const keyObj = service.generateApiKey('rotate-no', true); + expect(() => service.rotateApiKey(keyObj.key, false)).toThrow( + UnauthorizedException, + ); + }); + + it('throws NotFoundException for an unknown key', () => { + expect(() => service.rotateApiKey('sh_unknown', true)).toThrow( + NotFoundException, + ); + }); + + it('throws BadRequestException when rotating a revoked key', () => { + const keyObj = service.generateApiKey('rotate-revoked', true); + service.revokeApiKey(keyObj.key, true); + expect(() => service.rotateApiKey(keyObj.key, true)).toThrow( + BadRequestException, + ); }); }); @@ -43,28 +174,38 @@ describe('ApiKeyService', () => { }); it('throws UnauthorizedException if non-admin attempts revocation', () => { - expect(() => service.revokeApiKey('some-key', false)).toThrow(UnauthorizedException); + expect(() => service.revokeApiKey('some-key', false)).toThrow( + UnauthorizedException, + ); }); it('throws NotFoundException if key does not exist', () => { - expect(() => service.revokeApiKey('non-existent-key', true)).toThrow(NotFoundException); + expect(() => service.revokeApiKey('non-existent-key', true)).toThrow( + NotFoundException, + ); }); it('throws BadRequestException if key is already revoked', () => { const keyObj = service.generateApiKey('revoke-target', true); service.revokeApiKey(keyObj.key, true); - expect(() => service.revokeApiKey(keyObj.key, true)).toThrow(BadRequestException); + expect(() => service.revokeApiKey(keyObj.key, true)).toThrow( + BadRequestException, + ); }); }); describe('validateApiKey', () => { it('returns false for expired keys', () => { - const expiredKey = service.generateApiKey('expired-owner', true, new Date(Date.now() - 1000)); + const expiredKey = service.generateApiKey( + 'expired-owner', + true, + new Date(Date.now() - 1000), + ); expect(service.validateApiKey(expiredKey.key)).toBe(false); }); it('returns false for unknown keys', () => { - expect(service.validateApiKey('invalid-uuid')).toBe(false); + expect(service.validateApiKey('sh_invalid')).toBe(false); }); }); @@ -74,8 +215,19 @@ describe('ApiKeyService', () => { expect(keys.length).toBeGreaterThanOrEqual(2); }); + it('never exposes raw secrets in listings', () => { + const keys = service.getAllApiKeys(true); + for (const k of keys) { + expect((k as any).key).toBeUndefined(); + expect(k.keyHash).toBeDefined(); + expect(k.keyHint).toBeDefined(); + } + }); + it('throws UnauthorizedException for non-admin', () => { - expect(() => service.getAllApiKeys(false)).toThrow(UnauthorizedException); + expect(() => service.getAllApiKeys(false)).toThrow( + UnauthorizedException, + ); }); }); }); diff --git a/backend/src/api-key/api-key.service.ts b/backend/src/api-key/api-key.service.ts index 63680fb7..dc93b42f 100644 --- a/backend/src/api-key/api-key.service.ts +++ b/backend/src/api-key/api-key.service.ts @@ -5,15 +5,19 @@ import { NotFoundException, BadRequestException, } from '@nestjs/common'; -import { v4 as uuidv4 } from 'uuid'; +import { createHash, randomBytes, timingSafeEqual } from 'crypto'; export enum ApiKeyStatus { ACTIVE = 'active', REVOKED = 'revoked', } -export interface ApiKey { - key: string; +/** A stored API key record. Never contains the raw secret. */ +export interface ApiKeyRecord { + /** SHA-256 hex digest of the raw secret — the only thing persisted. */ + keyHash: string; + /** Last 4 characters of the raw secret, for display/identification. */ + keyHint: string; ownerLabel: string; status: ApiKeyStatus; createdAt: Date; @@ -24,11 +28,43 @@ export interface ApiKey { scopedEndpoints: string[]; } +/** What `generate`/`rotate` return: the record plus the raw secret (once). */ +export interface ApiKey extends ApiKeyRecord { + key: string; +} + +/** + * SHA-256 the raw key. The digest (never the raw key) is what is stored and + * what lookups compare against. + */ +export function hashApiKey(key: string): string { + return createHash('sha256').update(key, 'utf8').digest('hex'); +} + +/** + * Constant-time comparison of two hex digests. Used when verifying a + * presented key against its stored hash so timing does not leak how close + * an attacker's guess is. + */ +export function constantTimeEqual(a: string, b: string): boolean { + const bufA = Buffer.from(a, 'utf8'); + const bufB = Buffer.from(b, 'utf8'); + if (bufA.length !== bufB.length) { + return false; + } + return timingSafeEqual(bufA, bufB); +} + +function generateRawKey(): string { + return `sh_${randomBytes(24).toString('hex')}`; +} + @Injectable() export class ApiKeyService { private readonly logger = new Logger(ApiKeyService.name); - private apiKeys = new Map(); + /** keyHash -> record. Raw secrets are never stored. */ + private apiKeys = new Map(); constructor() { this.seedData(); @@ -62,10 +98,11 @@ export class ApiKeyService { throw new BadRequestException('Owner label cannot be empty.'); } - const newKey = uuidv4(); - const apiKey: ApiKey = { - key: newKey, - ownerLabel, + const rawKey = generateRawKey(); + const record: ApiKeyRecord = { + keyHash: hashApiKey(rawKey), + keyHint: rawKey.slice(-4), + ownerLabel: ownerLabel.trim(), status: ApiKeyStatus.ACTIVE, createdAt: new Date(), expiresAt, @@ -74,22 +111,60 @@ export class ApiKeyService { requestsThisMonth: 0, scopedEndpoints, }; - this.apiKeys.set(newKey, apiKey); - this.logger.log(`Generated new API key for ${ownerLabel}: ${newKey}`); - return apiKey; + this.apiKeys.set(record.keyHash, record); + // Never log the raw secret — only the display hint. + this.logger.log( + `Generated new API key for ${record.ownerLabel} (hint: ...${record.keyHint})`, + ); + // The raw secret is returned exactly once, at creation time. + return { ...record, key: rawKey }; + } + + /** + * Rotation: mints a fresh secret for an existing key while preserving its + * metadata (owner, quota, scopes). The old secret stops working + * immediately because the stored hash is replaced. + */ + rotateApiKey(key: string, isAdmin: boolean): ApiKey { + if (!isAdmin) { + throw new UnauthorizedException( + 'Only administrators can rotate API keys.', + ); + } + + const record = this.findRecordByKey(key); + if (!record) { + throw new NotFoundException(`API Key not found.`); + } + if (record.status === ApiKeyStatus.REVOKED) { + throw new BadRequestException( + 'Cannot rotate a revoked API key. Generate a new one instead.', + ); + } + + const newRawKey = generateRawKey(); + const rotated: ApiKeyRecord = { + ...record, + keyHash: hashApiKey(newRawKey), + keyHint: newRawKey.slice(-4), + }; + this.apiKeys.delete(record.keyHash); + this.apiKeys.set(rotated.keyHash, rotated); + this.logger.log(`Rotated API key for ${rotated.ownerLabel}`); + return { ...rotated, key: newRawKey }; } checkQuota(key: string): boolean { - const apiKey = this.apiKeys.get(key); - if (!apiKey) return false; - return apiKey.requestsThisMonth < apiKey.monthlyRequestQuota; + const record = this.findRecordByKey(key); + if (!record) return false; + return record.requestsThisMonth < record.monthlyRequestQuota; } incrementRequestCount(key: string): void { - const apiKey = this.apiKeys.get(key); - if (apiKey) { - apiKey.requestsThisMonth += 1; - this.apiKeys.set(key, apiKey); + const record = this.findRecordByKey(key); + if (record) { + record.requestsThisMonth += 1; + this.apiKeys.set(record.keyHash, record); } } @@ -98,63 +173,77 @@ export class ApiKeyService { limit: number; remaining: number; } { - const apiKey = this.apiKeys.get(key); - if (!apiKey) { + const record = this.findRecordByKey(key); + if (!record) { return { used: 0, limit: 0, remaining: 0 }; } return { - used: apiKey.requestsThisMonth, - limit: apiKey.monthlyRequestQuota, - remaining: Math.max( - 0, - apiKey.monthlyRequestQuota - apiKey.requestsThisMonth, - ), + used: record.requestsThisMonth, + limit: record.monthlyRequestQuota, + remaining: Math.max(0, record.monthlyRequestQuota - record.requestsThisMonth), }; } - revokeApiKey(key: string, isAdmin: boolean): ApiKey { + revokeApiKey(key: string, isAdmin: boolean): ApiKeyRecord { if (!isAdmin) { throw new UnauthorizedException( 'Only administrators can revoke API keys.', ); } - const apiKey = this.apiKeys.get(key); - if (!apiKey) { - throw new NotFoundException(`API Key "${key}" not found.`); + const record = this.findRecordByKey(key); + if (!record) { + throw new NotFoundException(`API Key not found.`); } - if (apiKey.status === ApiKeyStatus.REVOKED) { - throw new BadRequestException(`API Key "${key}" is already revoked.`); + if (record.status === ApiKeyStatus.REVOKED) { + throw new BadRequestException(`API Key is already revoked.`); } - apiKey.status = ApiKeyStatus.REVOKED; - this.apiKeys.set(key, apiKey); - this.logger.log(`API Key "${key}" revoked.`); - return apiKey; + record.status = ApiKeyStatus.REVOKED; + this.apiKeys.set(record.keyHash, record); + this.logger.log(`API Key revoked (owner: ${record.ownerLabel}).`); + return record; } - validateApiKey(key: string): boolean { - this.logger.log(`Validating API Key: ${key}`); - const apiKey = this.apiKeys.get(key); + /** + * Verifies a presented key in constant time against its stored hash and + * enforces status/expiry/scope. `endpoint` (e.g. the request route path) + * is optional; when provided, keys with `scopedEndpoints` must cover it. + */ + validateApiKey(key: string, endpoint?: string): boolean { + const hash = hashApiKey(key); + const record = this.apiKeys.get(hash); - if (!apiKey) { - this.logger.warn(`API Key "${key}" not found.`); + if (!record) { + this.logger.warn(`API key validation failed (hint: ...${key.slice(-4)}).`); + return false; + } + // Constant-time comparison of the digest against the stored hash. + if (!constantTimeEqual(hash, record.keyHash)) { + this.logger.warn(`API key hash mismatch (hint: ...${record.keyHint}).`); return false; } - if (apiKey.status === ApiKeyStatus.REVOKED) { - this.logger.warn(`API Key "${key}" is revoked.`); + if (record.status === ApiKeyStatus.REVOKED) { + this.logger.warn(`API key is revoked (hint: ...${record.keyHint}).`); return false; } - if (apiKey.expiresAt && apiKey.expiresAt < new Date()) { - this.logger.warn(`API Key "${key}" has expired.`); + if (record.expiresAt && record.expiresAt < new Date()) { + this.logger.warn(`API key has expired (hint: ...${record.keyHint}).`); + return false; + } + if (endpoint && !this.isEndpointAllowed(record, endpoint)) { + this.logger.warn( + `API key not allowed for endpoint ${endpoint} (hint: ...${record.keyHint}).`, + ); return false; } - this.logger.log(`API Key "${key}" is valid.`); + this.logger.log(`API key is valid (hint: ...${record.keyHint}).`); return true; } - getAllApiKeys(isAdmin: boolean): ApiKey[] { + /** Returns stored records only — raw secrets are never exposed again. */ + getAllApiKeys(isAdmin: boolean): ApiKeyRecord[] { if (!isAdmin) { throw new UnauthorizedException( 'Only administrators can view all API keys.', @@ -162,4 +251,23 @@ export class ApiKeyService { } return Array.from(this.apiKeys.values()); } + + private findRecordByKey(key: string): ApiKeyRecord | undefined { + return this.apiKeys.get(hashApiKey(key)); + } + + private isEndpointAllowed(record: ApiKeyRecord, endpoint: string): boolean { + if (!record.scopedEndpoints || record.scopedEndpoints.length === 0) { + return true; // unscoped keys may call any endpoint + } + return record.scopedEndpoints.some((allowed) => { + if (allowed === '*') return true; + if (allowed.endsWith('*')) { + const prefix = allowed.slice(0, -1); // e.g. '/puzzles/' + const base = prefix.replace(/\/+$/, ''); // e.g. '/puzzles' + return endpoint === base || endpoint.startsWith(prefix); + } + return endpoint === allowed || endpoint.startsWith(`${allowed}/`); + }); + } } diff --git a/backend/src/audit-log/audit-log.controller.spec.ts b/backend/src/audit-log/audit-log.controller.spec.ts new file mode 100644 index 00000000..35d2f860 --- /dev/null +++ b/backend/src/audit-log/audit-log.controller.spec.ts @@ -0,0 +1,80 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { GUARDS_METADATA } from '@nestjs/common/constants'; +import { AuditLogController } from './audit-log.controller'; +import { AuditLogService } from './audit-log.service'; +import { JwtAuthGuard } from '../admin/guards/jwt-auth.guard'; +import { RolesGuard } from '../admin/guards/roles.guard'; +import { AdminRole } from '../admin/admin-role.enum'; +import { ROLES_KEY } from '../admin/roles.decorator'; + +describe('AuditLogController', () => { + let controller: AuditLogController; + let serviceMock: { + findAll: jest.Mock; + purgeOlderThan: jest.Mock; + }; + + beforeEach(async () => { + serviceMock = { + findAll: jest.fn().mockResolvedValue([]), + purgeOlderThan: jest.fn().mockResolvedValue(3), + }; + + const module: TestingModule = await Test.createTestingModule({ + controllers: [AuditLogController], + providers: [{ provide: AuditLogService, useValue: serviceMock }], + }).compile(); + + controller = module.get(AuditLogController); + }); + + it('should be defined', () => { + expect(controller).toBeDefined(); + }); + + describe('access control', () => { + it('requires admin JWT authentication at the controller level', () => { + const guards = Reflect.getMetadata(GUARDS_METADATA, AuditLogController); + expect(guards).toContain(JwtAuthGuard); + expect(guards).toContain(RolesGuard); + }); + + it('restricts all audit-log routes to admin roles', () => { + const roles = Reflect.getMetadata(ROLES_KEY, AuditLogController); + expect(roles).toEqual([AdminRole.ADMIN, AdminRole.SUPERADMIN]); + }); + }); + + describe('getAuditLogs', () => { + it('delegates to the service with parsed date filters', async () => { + await controller.getAuditLogs({ + userId: 'user-1', + action: 'login', + startDate: '2026-01-01T00:00:00.000Z', + endDate: '2026-01-31T00:00:00.000Z', + }); + expect(serviceMock.findAll).toHaveBeenCalledWith({ + userId: 'user-1', + action: 'login', + startDate: new Date('2026-01-01T00:00:00.000Z'), + endDate: new Date('2026-01-31T00:00:00.000Z'), + }); + }); + + it('leaves dates undefined when not provided', async () => { + await controller.getAuditLogs({}); + expect(serviceMock.findAll).toHaveBeenCalledWith({ + startDate: undefined, + endDate: undefined, + }); + }); + }); + + describe('purgeOlderThan', () => { + it('purges records older than the requested retention window', async () => { + const result = await controller.purgeOlderThan(90); + expect(serviceMock.purgeOlderThan).toHaveBeenCalledWith(90); + expect(result).toEqual({ purged: 3, retentionDays: 90 }); + }); + }); +}); diff --git a/backend/src/audit-log/audit-log.controller.ts b/backend/src/audit-log/audit-log.controller.ts index 37e4d7c6..ff6564ba 100644 --- a/backend/src/audit-log/audit-log.controller.ts +++ b/backend/src/audit-log/audit-log.controller.ts @@ -1,14 +1,43 @@ -import { Controller, Get, Query } from '@nestjs/common'; -import { ApiTags, ApiQuery } from '@nestjs/swagger'; -import { AuditLogService } from './audit-log.service'; +import { + Controller, + Delete, + Get, + Param, + ParseIntPipe, + Query, + Res, + UseGuards, +} from '@nestjs/common'; +import { + ApiBearerAuth, + ApiOperation, + ApiParam, + ApiQuery, + ApiResponse, + ApiTags, +} from '@nestjs/swagger'; +import { Response } from 'express'; +import { AuditLogService, DEFAULT_RETENTION_DAYS } from './audit-log.service'; import { FilterAuditLogDto } from './Dto/filter-audit-log.dto'; +import { JwtAuthGuard } from '../admin/guards/jwt-auth.guard'; +import { RolesGuard } from '../admin/guards/roles.guard'; +import { Roles } from '../admin/roles.decorator'; +import { AdminRole } from '../admin/admin-role.enum'; @ApiTags('Audit Logs') +@ApiBearerAuth() @Controller('admin/audit-logs') +@UseGuards(JwtAuthGuard, RolesGuard) +@Roles(AdminRole.ADMIN, AdminRole.SUPERADMIN) export class AuditLogController { constructor(private readonly auditLogService: AuditLogService) {} @Get() + @ApiOperation({ + summary: 'List audit logs (admin only)', + description: + 'Audit logs are restricted to administrators; ordinary users cannot read or modify them.', + }) async getAuditLogs(@Query() filter: FilterAuditLogDto) { return this.auditLogService.findAll({ ...filter, @@ -16,4 +45,57 @@ export class AuditLogController { endDate: filter.endDate ? new Date(filter.endDate) : undefined, }); } + + @Get('export') + @ApiOperation({ summary: 'Export audit logs as CSV (admin only)' }) + async exportAuditLogs( + @Query() filter: FilterAuditLogDto, + @Res() res: Response, + ) { + const logs = await this.auditLogService.findAll({ + ...filter, + startDate: filter.startDate ? new Date(filter.startDate) : undefined, + endDate: filter.endDate ? new Date(filter.endDate) : undefined, + }); + + const escapeCsv = (value: unknown): string => { + const str = value == null ? '' : String(value); + return /[",\n]/.test(str) ? `"${str.replace(/"/g, '""')}"` : str; + }; + + const header = ['id', 'userId', 'action', 'timestamp', 'meta']; + const rows = logs.map((log) => + [ + escapeCsv(log.id), + escapeCsv(log.userId), + escapeCsv(log.action), + escapeCsv(log.timestamp?.toISOString()), + escapeCsv(JSON.stringify(log.meta ?? {})), + ].join(','), + ); + + res.setHeader('Content-Type', 'text/csv; charset=utf-8'); + res.setHeader( + 'Content-Disposition', + 'attachment; filename="audit-logs.csv"', + ); + res.send([header.join(','), ...rows].join('\n')); + } + + @Delete('older-than/:days') + @ApiOperation({ + summary: 'Purge audit logs older than N days (admin only)', + description: `Enforces the retention policy (default ${DEFAULT_RETENTION_DAYS} days).`, + }) + @ApiParam({ name: 'days', description: 'Retention window in days' }) + @ApiResponse({ + status: 200, + description: 'Returns the number of purged audit log records', + }) + async purgeOlderThan( + @Param('days', ParseIntPipe) days: number, + ): Promise<{ purged: number; retentionDays: number }> { + const purged = await this.auditLogService.purgeOlderThan(days); + return { purged, retentionDays: days }; + } } diff --git a/backend/src/audit-log/audit-log.service.spec.ts b/backend/src/audit-log/audit-log.service.spec.ts new file mode 100644 index 00000000..15ce129d --- /dev/null +++ b/backend/src/audit-log/audit-log.service.spec.ts @@ -0,0 +1,104 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { AuditLogService, DEFAULT_RETENTION_DAYS } from './audit-log.service'; +import { AuditLog } from './entities/audit-log.entity'; + +describe('AuditLogService', () => { + let service: AuditLogService; + let repo: { + create: jest.Mock; + save: jest.Mock; + find: jest.Mock; + createQueryBuilder: jest.Mock; + }; + + beforeEach(async () => { + repo = { + create: jest.fn((data) => ({ ...data })), + save: jest.fn((log) => Promise.resolve({ id: 'log-1', ...log })), + find: jest.fn(() => Promise.resolve([])), + createQueryBuilder: jest.fn(), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + AuditLogService, + { provide: getRepositoryToken(AuditLog), useValue: repo }, + ], + }).compile(); + + service = module.get(AuditLogService); + }); + + it('should be defined', () => { + expect(service).toBeDefined(); + }); + + describe('createLog', () => { + it('persists a new append-only log entry', async () => { + const log = await service.createLog('user-1', 'puzzle.submitted', { + puzzleId: 'p-1', + }); + expect(repo.create).toHaveBeenCalledWith({ + userId: 'user-1', + action: 'puzzle.submitted', + meta: { puzzleId: 'p-1' }, + }); + expect(log.id).toBe('log-1'); + }); + }); + + describe('findAll', () => { + it('filters by userId and action', async () => { + await service.findAll({ userId: 'user-1', action: 'login' }); + expect(repo.find).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ + userId: 'user-1', + action: expect.anything(), + }), + order: { timestamp: 'DESC' }, + }), + ); + }); + + it('passes date range bounds to Between', async () => { + const start = new Date('2026-01-01'); + const end = new Date('2026-01-31'); + await service.findAll({ startDate: start, endDate: end }); + const arg = repo.find.mock.calls[0][0]; + expect(arg.where.timestamp).toBeDefined(); + // Between produces a FindOperator — verify the two bounds + expect(arg.where.timestamp._value).toEqual([start, end]); + }); + }); + + describe('purgeOlderThan', () => { + it('deletes logs older than the retention window', async () => { + const qb = { + delete: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue({ affected: 7 }), + }; + repo.createQueryBuilder.mockReturnValue(qb); + + const purged = await service.purgeOlderThan(DEFAULT_RETENTION_DAYS); + expect(purged).toBe(7); + expect(qb.where).toHaveBeenCalledWith( + 'timestamp < :cutoff', + expect.objectContaining({ cutoff: expect.any(Date) }), + ); + }); + + it('returns 0 when nothing was purged', async () => { + const qb = { + delete: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue({ affected: 0 }), + }; + repo.createQueryBuilder.mockReturnValue(qb); + + await expect(service.purgeOlderThan(30)).resolves.toBe(0); + }); + }); +}); diff --git a/backend/src/audit-log/audit-log.service.ts b/backend/src/audit-log/audit-log.service.ts index 82509636..b06c7ffe 100644 --- a/backend/src/audit-log/audit-log.service.ts +++ b/backend/src/audit-log/audit-log.service.ts @@ -3,6 +3,12 @@ import { InjectRepository } from '@nestjs/typeorm'; import { Repository, Like, Between } from 'typeorm'; import { AuditLog } from './entities/audit-log.entity'; +/** + * Default retention window (in days). Logs older than this are considered + * expired and are purged by `purgeOlderThan` / the admin retention endpoint. + */ +export const DEFAULT_RETENTION_DAYS = 90; + @Injectable() export class AuditLogService { constructor( @@ -10,6 +16,11 @@ export class AuditLogService { private readonly auditRepo: Repository, ) {} + /** + * Append-only: the audit log is only ever written by the platform (via + * `createLog`). There are no public update/delete mutations — retention + * cleanup happens exclusively through the admin-only `purgeOlderThan`. + */ async createLog(userId: string, action: string, meta?: Record) { const log = this.auditRepo.create({ userId, action, meta }); return this.auditRepo.save(log); @@ -29,4 +40,19 @@ export class AuditLogService { return this.auditRepo.find({ where, order: { timestamp: 'DESC' } }); } + + /** + * Retention enforcement: deletes every record older than `days` days and + * returns the number of purged rows. Intended to be invoked by admins + * (or a scheduled job) so the log never grows unbounded. + */ + async purgeOlderThan(days: number): Promise { + const cutoff = new Date(Date.now() - days * 24 * 60 * 60 * 1000); + const result = await this.auditRepo + .createQueryBuilder() + .delete() + .where('timestamp < :cutoff', { cutoff }) + .execute(); + return result.affected ?? 0; + } } diff --git a/backend/src/common/decorators/ownership.decorator.ts b/backend/src/common/decorators/ownership.decorator.ts new file mode 100644 index 00000000..e2de87ce --- /dev/null +++ b/backend/src/common/decorators/ownership.decorator.ts @@ -0,0 +1,29 @@ +import { SetMetadata } from '@nestjs/common'; + +/** + * Metadata key used by {@link OwnershipGuard} to locate the `userId` that a + * route operates on. + */ +export const OWNERSHIP_KEY = 'ownership'; + +export interface OwnershipConfig { + /** Name of the route parameter holding the target user id (e.g. `userId`). */ + param?: string; + /** Name of the body field holding the target user id (e.g. `userId`). */ + body?: string; +} + +/** + * Declares which `userId` a route operates on so the global + * {@link OwnershipGuard} can enforce that callers only touch their own data + * (unless they carry an explicit administrative role). + * + * @example + * ```ts + * @UseGuards(AuthGuard('jwt'), OwnershipGuard) + * @Ownership({ param: 'userId' }) + * getUserPoints(@Param('userId') userId: string) { ... } + * ``` + */ +export const Ownership = (config: OwnershipConfig) => + SetMetadata(OWNERSHIP_KEY, config); diff --git a/backend/src/common/guards/ownership.guard.spec.ts b/backend/src/common/guards/ownership.guard.spec.ts new file mode 100644 index 00000000..f36ad7ab --- /dev/null +++ b/backend/src/common/guards/ownership.guard.spec.ts @@ -0,0 +1,142 @@ +import { Test } from '@nestjs/testing'; +import { + ExecutionContext, + ForbiddenException, + UnauthorizedException, +} from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { OwnershipGuard } from './ownership.guard'; +import { AdminRole } from '../../admin/admin-role.enum'; + +function mockContext( + request: Record, + config?: unknown, +): ExecutionContext { + return { + switchToHttp: () => ({ getRequest: () => request }), + getHandler: () => ({}), + getClass: () => ({}), + } as unknown as ExecutionContext; +} + +describe('OwnershipGuard', () => { + let guard: OwnershipGuard; + let reflector: { getAllAndOverride: jest.Mock }; + + beforeEach(async () => { + reflector = { getAllAndOverride: jest.fn() }; + const moduleRef = await Test.createTestingModule({ + providers: [ + OwnershipGuard, + { provide: Reflector, useValue: reflector }, + ], + }).compile(); + guard = moduleRef.get(OwnershipGuard); + }); + + describe('when no @Ownership metadata is present', () => { + it('allows the request through', () => { + reflector.getAllAndOverride.mockReturnValue(undefined); + const ctx = mockContext({ user: undefined }); + expect(guard.canActivate(ctx)).toBe(true); + }); + }); + + describe('route param ownership', () => { + beforeEach(() => { + reflector.getAllAndOverride.mockReturnValue({ param: 'userId' }); + }); + + it('allows a user to access their own data', () => { + const ctx = mockContext({ + user: { id: 'user-1' }, + params: { userId: 'user-1' }, + }); + expect(guard.canActivate(ctx)).toBe(true); + }); + + it('allows a user whose JWT exposes sub to access their own data', () => { + const ctx = mockContext({ + user: { sub: 'user-1' }, + params: { userId: 'user-1' }, + }); + expect(guard.canActivate(ctx)).toBe(true); + }); + + it('forbids accessing another user data', () => { + const ctx = mockContext({ + user: { id: 'user-1' }, + params: { userId: 'user-2' }, + }); + expect(() => guard.canActivate(ctx)).toThrow(ForbiddenException); + }); + + it('throws UnauthorizedException when no caller is authenticated', () => { + const ctx = mockContext({ user: undefined, params: { userId: 'user-2' } }); + expect(() => guard.canActivate(ctx)).toThrow(UnauthorizedException); + }); + + it('allows an admin to access any user data', () => { + const ctx = mockContext({ + user: { id: 'admin-1', role: AdminRole.ADMIN }, + params: { userId: 'user-2' }, + }); + expect(guard.canActivate(ctx)).toBe(true); + }); + + it('allows a superadmin to access any user data', () => { + const ctx = mockContext({ + user: { id: 'admin-1', role: AdminRole.SUPERADMIN }, + params: { userId: 'user-2' }, + }); + expect(guard.canActivate(ctx)).toBe(true); + }); + }); + + describe('body ownership', () => { + beforeEach(() => { + reflector.getAllAndOverride.mockReturnValue({ body: 'userId' }); + }); + + it('allows a user to act on their own id in the body', () => { + const ctx = mockContext({ + user: { id: 'user-1' }, + body: { userId: 'user-1' }, + }); + expect(guard.canActivate(ctx)).toBe(true); + }); + + it('forbids acting on another user id in the body', () => { + const ctx = mockContext({ + user: { id: 'user-1' }, + body: { userId: 'user-2' }, + }); + expect(() => guard.canActivate(ctx)).toThrow(ForbiddenException); + }); + + it('allows an admin to act on any user id in the body', () => { + const ctx = mockContext({ + user: { id: 'admin-1', role: AdminRole.ADMIN }, + body: { userId: 'user-2' }, + }); + expect(guard.canActivate(ctx)).toBe(true); + }); + }); + + describe('edge cases', () => { + it('passes through when the target userId is missing', () => { + reflector.getAllAndOverride.mockReturnValue({ param: 'userId' }); + const ctx = mockContext({ user: { id: 'user-1' }, params: {} }); + expect(guard.canActivate(ctx)).toBe(true); + }); + + it('compares ids as strings', () => { + reflector.getAllAndOverride.mockReturnValue({ param: 'userId' }); + const ctx = mockContext({ + user: { id: 42 }, + params: { userId: '42' }, + }); + expect(guard.canActivate(ctx)).toBe(true); + }); + }); +}); diff --git a/backend/src/common/guards/ownership.guard.ts b/backend/src/common/guards/ownership.guard.ts new file mode 100644 index 00000000..6fc070c5 --- /dev/null +++ b/backend/src/common/guards/ownership.guard.ts @@ -0,0 +1,75 @@ +import { + CanActivate, + ExecutionContext, + ForbiddenException, + Injectable, + UnauthorizedException, +} from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { AdminRole } from '../../admin/admin-role.enum'; +import { OWNERSHIP_KEY, OwnershipConfig } from '../decorators/ownership.decorator'; + +/** + * Roles that are explicitly allowed to access any user's data. Mirrors + * `AdminRole` so user tokens (which carry no `role` claim) can never pass. + */ +const ADMIN_ROLES = new Set([AdminRole.ADMIN, AdminRole.SUPERADMIN]); + +/** + * Enforces that a route which operates on a specific `userId` (declared via + * `@Ownership({ param: 'userId' })` or `@Ownership({ body: 'userId' })`) can + * only be invoked by the owner of that id — unless the caller carries an + * explicit administrative role. + * + * The authenticated caller is read from `request.user`: + * - user JWTs validated through the passport `jwt` strategy expose the full + * user entity (`.id`); + * - user JWTs decoded by `AuthMiddleware` expose the raw payload (`.sub`); + * - admin JWTs expose the admin entity (`.id`) plus a `.role` claim. + */ +@Injectable() +export class OwnershipGuard implements CanActivate { + constructor(private readonly reflector: Reflector) {} + + canActivate(context: ExecutionContext): boolean { + const config = this.reflector.getAllAndOverride( + OWNERSHIP_KEY, + [context.getHandler(), context.getClass()], + ); + if (!config) { + return true; + } + + const request = context.switchToHttp().getRequest(); + const user = request.user; + + // An explicitly authenticated administrator may access any user's data. + if (user?.role && ADMIN_ROLES.has(user.role)) { + return true; + } + + const callerId = user?.id ?? user?.sub; + if (!callerId) { + throw new UnauthorizedException( + 'Authentication required to access this resource', + ); + } + + const targetUserId = config.param + ? request.params?.[config.param] + : config.body + ? request.body?.[config.body] + : undefined; + + // No target id on the request — let the route handler validate it. + if (!targetUserId) { + return true; + } + + if (String(callerId) !== String(targetUserId)) { + throw new ForbiddenException('You can only access your own data'); + } + + return true; + } +} diff --git a/backend/src/multiplayer-queue/multiplayer-queue.controller.ts b/backend/src/multiplayer-queue/multiplayer-queue.controller.ts index 7135f2cf..abd772b9 100644 --- a/backend/src/multiplayer-queue/multiplayer-queue.controller.ts +++ b/backend/src/multiplayer-queue/multiplayer-queue.controller.ts @@ -6,7 +6,11 @@ import { Param, HttpCode, HttpStatus, + UseGuards, } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; +import { OwnershipGuard } from '../common/guards/ownership.guard'; +import { Ownership } from '../common/decorators/ownership.decorator'; import { ApiTags, ApiOperation, ApiResponse, ApiParam } from '@nestjs/swagger'; import type { MultiplayerQueueService } from './multiplayer-queue.service'; import type { JoinQueueDto } from './dto/join-queue.dto'; @@ -38,6 +42,8 @@ export class MultiplayerQueueController { @Delete('leave/:userId') @HttpCode(HttpStatus.NO_CONTENT) + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) @ApiOperation({ summary: 'Leave the multiplayer queue' }) @ApiParam({ name: 'userId', description: 'User ID to remove from queue' }) @ApiResponse({ status: 204, description: 'Successfully left queue' }) @@ -47,6 +53,8 @@ export class MultiplayerQueueController { } @Get('status/:userId') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) @ApiOperation({ summary: 'Get queue status for a user' }) @ApiParam({ name: 'userId', description: 'User ID to check status' }) @ApiResponse({ diff --git a/backend/src/progress/progress.controller.ts b/backend/src/progress/progress.controller.ts index 4cc0d28c..a1915596 100644 --- a/backend/src/progress/progress.controller.ts +++ b/backend/src/progress/progress.controller.ts @@ -1,7 +1,10 @@ -import { Controller, Get, Param, ParseUUIDPipe } from '@nestjs/common'; +import { Controller, Get, Param, ParseUUIDPipe, UseGuards } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; import { ProgressService } from './progress.service'; import { ProgressResponseDto } from './dto/progress-response.dto'; import { ApiOperation, ApiResponse, ApiTags } from '@nestjs/swagger'; +import { OwnershipGuard } from '../common/guards/ownership.guard'; +import { Ownership } from '../common/decorators/ownership.decorator'; @ApiTags('Progress') @Controller('users') @@ -9,6 +12,8 @@ export class ProgressController { constructor(private readonly progressService: ProgressService) {} @Get(':id/progress') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'id' }) @ApiOperation({ summary: 'Get user progress' }) @ApiResponse({ status: 200, diff --git a/backend/src/puzzle-dependency/puzzle-dependency.controller.ts b/backend/src/puzzle-dependency/puzzle-dependency.controller.ts index c158c84c..a4496f0f 100644 --- a/backend/src/puzzle-dependency/puzzle-dependency.controller.ts +++ b/backend/src/puzzle-dependency/puzzle-dependency.controller.ts @@ -9,7 +9,11 @@ import { Query, HttpStatus, ParseUUIDPipe, + UseGuards, } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; +import { OwnershipGuard } from '../common/guards/ownership.guard'; +import { Ownership } from '../common/decorators/ownership.decorator'; import { ApiTags, ApiOperation, @@ -134,6 +138,8 @@ export class PuzzleDependencyController { // Eligibility and Completion Endpoints @Post('check-eligibility') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ body: 'userId' }) @ApiOperation({ summary: 'Check if a user is eligible to access a puzzle' }) @ApiResponse({ status: HttpStatus.OK, @@ -147,6 +153,8 @@ export class PuzzleDependencyController { } @Post('mark-completed') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ body: 'userId' }) @ApiOperation({ summary: 'Mark a puzzle as completed for a user' }) @ApiResponse({ status: HttpStatus.CREATED, @@ -160,6 +168,8 @@ export class PuzzleDependencyController { } @Get('user/:userId/completed') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) @ApiOperation({ summary: 'Get all puzzles completed by a user' }) @ApiParam({ name: 'userId', description: 'User ID' }) @ApiResponse({ @@ -171,6 +181,8 @@ export class PuzzleDependencyController { } @Get('user/:userId/unlocked') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) @ApiOperation({ summary: 'Get all puzzles unlocked for a user' }) @ApiParam({ name: 'userId', description: 'User ID' }) @ApiQuery({ diff --git a/backend/src/reward-shop/reward-shop.controller.ts b/backend/src/reward-shop/reward-shop.controller.ts index ecd8697d..6696a97d 100644 --- a/backend/src/reward-shop/reward-shop.controller.ts +++ b/backend/src/reward-shop/reward-shop.controller.ts @@ -8,7 +8,12 @@ import { HttpCode, HttpStatus, Logger, + UseGuards, + BadRequestException, } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; +import { OwnershipGuard } from '../common/guards/ownership.guard'; +import { Ownership } from '../common/decorators/ownership.decorator'; import { RewardShopService, ShopItem, Purchase } from './reward-shop.service'; import { IsString, @@ -80,6 +85,8 @@ export class RewardShopController { @Post('purchase') @HttpCode(HttpStatus.CREATED) + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ body: 'userId' }) purchaseItem(@Body() purchaseDto: PurchaseItemDto): Purchase { this.logger.log( `Received purchase request: ${JSON.stringify(purchaseDto)}`, @@ -89,6 +96,8 @@ export class RewardShopController { } @Get('users/:userId/points') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) getUserPoints(@Param('userId') userId: string): { userId: string; points: number; @@ -100,6 +109,8 @@ export class RewardShopController { @Post('users/:userId/add-points') @HttpCode(HttpStatus.NO_CONTENT) + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) addPoints( @Param('userId') userId: string, @Body('amount') amount: number, diff --git a/backend/src/reward/reward.controller.ts b/backend/src/reward/reward.controller.ts index 46a80243..08caf91f 100644 --- a/backend/src/reward/reward.controller.ts +++ b/backend/src/reward/reward.controller.ts @@ -9,6 +9,9 @@ import { HttpStatus, UseGuards, } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; +import { OwnershipGuard } from '../common/guards/ownership.guard'; +import { Ownership } from '../common/decorators/ownership.decorator'; import { ApiTags, ApiOperation, @@ -120,6 +123,8 @@ export class RewardController { } @Get('user/:userId/claims') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) @ApiOperation({ summary: 'Get all claims for a user' }) @ApiParam({ name: 'userId', description: 'User ID' }) @ApiResponse({ diff --git a/backend/src/streak/controllers/streak.controller.ts b/backend/src/streak/controllers/streak.controller.ts index bb46ed9e..0ef70fbe 100644 --- a/backend/src/streak/controllers/streak.controller.ts +++ b/backend/src/streak/controllers/streak.controller.ts @@ -8,7 +8,11 @@ import { HttpCode, ParseIntPipe, DefaultValuePipe, + UseGuards, } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; +import { OwnershipGuard } from '../../common/guards/ownership.guard'; +import { Ownership } from '../../common/decorators/ownership.decorator'; import type { StreakService } from '../services/streak.service'; import type { RecordActivityDto } from '../dto/record-activity.dto'; import type { StreakCalculationConfig } from '../services/streak-calculation.service'; @@ -34,6 +38,8 @@ export class StreakController { } @Get('user/:userId') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) async getUserStreak( @Param('userId') userId: string, @Query('timezoneOffset', new DefaultValuePipe(0), ParseIntPipe) @@ -86,6 +92,8 @@ export class StreakController { } @Get('user/:userId/history') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) async getUserStreakHistory( @Param('userId') userId: string, @Query('days', new DefaultValuePipe(30), ParseIntPipe) days: number, diff --git a/backend/src/time-trial/time-trial.controller.ts b/backend/src/time-trial/time-trial.controller.ts index e5cfa8b4..3fb53e75 100644 --- a/backend/src/time-trial/time-trial.controller.ts +++ b/backend/src/time-trial/time-trial.controller.ts @@ -1,5 +1,8 @@ -import { Body, Controller, Get, Param, Post } from '@nestjs/common'; +import { Body, Controller, Get, Param, Post, UseGuards } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; import { TimetrialService } from './providers/timetrial.service'; +import { OwnershipGuard } from '../common/guards/ownership.guard'; +import { Ownership } from '../common/decorators/ownership.decorator'; import { ApiBody, ApiOperation, @@ -14,6 +17,8 @@ export class TimeTrialController { constructor(private readonly timeTrialService: TimetrialService) {} @Post('start') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ body: 'userId' }) @ApiOperation({ summary: 'Start a new time trial for a puzzle' }) @ApiBody({ schema: { @@ -57,6 +62,8 @@ export class TimeTrialController { } @Get('results/:userId') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) @ApiOperation({ summary: 'Get all time trial results for a user' }) @ApiParam({ name: 'userId', diff --git a/backend/src/user-ranking/user-ranking.controller.ts b/backend/src/user-ranking/user-ranking.controller.ts index 82db9f2d..f2eeda0b 100644 --- a/backend/src/user-ranking/user-ranking.controller.ts +++ b/backend/src/user-ranking/user-ranking.controller.ts @@ -1,5 +1,8 @@ -import { Controller, Get, Param } from '@nestjs/common'; +import { Controller, Get, Param, UseGuards } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; import { UserRankingService } from './user-ranking.service'; +import { OwnershipGuard } from '../common/guards/ownership.guard'; +import { Ownership } from '../common/decorators/ownership.decorator'; import { UserRankDto } from './dto/create-user-ranking.dto'; import { ApiOperation, ApiResponse, ApiTags } from '@nestjs/swagger'; @@ -9,6 +12,8 @@ export class UserRankingController { constructor(private readonly userRankingService: UserRankingService) {} @Get(':id/rank') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'id' }) @ApiOperation({ summary: 'Get user rank' }) @ApiResponse({ status: 200, diff --git a/backend/src/user-reaction/user-reaction.controller.ts b/backend/src/user-reaction/user-reaction.controller.ts index 7a999bd9..8e75abaa 100644 --- a/backend/src/user-reaction/user-reaction.controller.ts +++ b/backend/src/user-reaction/user-reaction.controller.ts @@ -8,7 +8,11 @@ import { Query, HttpCode, HttpStatus, + UseGuards, } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; +import { OwnershipGuard } from '../common/guards/ownership.guard'; +import { Ownership } from '../common/decorators/ownership.decorator'; import { ApiTags, ApiOperation, @@ -56,6 +60,8 @@ export class UserReactionController { } @Get('user/:userId') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) @ApiOperation({ summary: 'Get all reactions by specific user' }) @ApiParam({ name: 'userId', description: 'User ID' }) @ApiResponse({ @@ -70,6 +76,8 @@ export class UserReactionController { } @Get('user/:userId/content/:contentId') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) @ApiOperation({ summary: 'Get specific reaction by user and content' }) @ApiParam({ name: 'userId', description: 'User ID' }) @ApiParam({ name: 'contentId', description: 'Content ID' }) @@ -166,6 +174,8 @@ export class UserReactionController { @Delete('user/:userId/content/:contentId') @HttpCode(HttpStatus.NO_CONTENT) + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'userId' }) @ApiOperation({ summary: 'Remove reaction by user and content' }) @ApiParam({ name: 'userId', description: 'User ID' }) @ApiParam({ name: 'contentId', description: 'Content ID' }) diff --git a/backend/src/user-report-card/user-report-card.controller.ts b/backend/src/user-report-card/user-report-card.controller.ts index 8bda41b6..ecae55f7 100644 --- a/backend/src/user-report-card/user-report-card.controller.ts +++ b/backend/src/user-report-card/user-report-card.controller.ts @@ -8,7 +8,11 @@ import { HttpStatus, HttpException, Query, + UseGuards, } from '@nestjs/common'; +import { AuthGuard } from '@nestjs/passport'; +import { OwnershipGuard } from '../common/guards/ownership.guard'; +import { Ownership } from '../common/decorators/ownership.decorator'; import { ApiTags, ApiOperation, @@ -25,6 +29,8 @@ export class UserReportCardController { constructor(private readonly reportCardService: UserReportCardService) {} @Get(':id/report-card') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'id' }) @ApiOperation({ summary: 'Get user report card', description: @@ -58,6 +64,8 @@ export class UserReportCardController { } @Post(':id/report-card') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'id' }) @ApiOperation({ summary: 'Create user report card', description: 'Creates a new report card for a user or returns existing one', @@ -81,6 +89,8 @@ export class UserReportCardController { } @Put(':id/report-card/progress') + @UseGuards(AuthGuard('jwt'), OwnershipGuard) + @Ownership({ param: 'id' }) @ApiOperation({ summary: 'Update user progress', description: "Updates the progress statistics for a user's report card", diff --git a/onchain/contracts/stellar_hunts/src/test.rs b/onchain/contracts/stellar_hunts/src/test.rs index 3cb1aa45..01e18a23 100644 --- a/onchain/contracts/stellar_hunts/src/test.rs +++ b/onchain/contracts/stellar_hunts/src/test.rs @@ -689,5 +689,105 @@ fn test_schema_version() { let admin = new_admin(&e); client.init(&admin); - assert_eq!(client.get_schema_version(), 1); + assert_eq!(client.get_schema_version(), crate::CURRENT_SCHEMA_VERSION); +} + +/// A legacy deployment that never wrote the SchemaVersion key must report +/// version 0 so tooling can detect pre-versioning state. +#[test] +fn test_schema_version_zero_before_init() { + let env = Env::default(); + let contract_id = env.register_contract(None, StellarHunts); + let client = StellarHuntsClient::new(&env, &contract_id); + + assert_eq!(client.get_schema_version(), 0); +} + +// --------------------------------------------------------------------- +// Storage compatibility (see onchain/docs/storage-versioning.md) +// --------------------------------------------------------------------- + +/// State written by a pre-versioning deployment (Question.version == 0) +/// must still be readable by the current contract. +#[test] +fn test_legacy_question_readable() { + let env = Env::default(); + let admin = new_admin(&env); + let contract_id = env.register_contract(None, StellarHunts); + let client = StellarHuntsClient::new(&env, &contract_id); + client.init(&admin); + + // Write a Question exactly as an old (unversioned) contract would have: + // version field = 0, question stored under DataKey::Question(7). + env.as_contract(&contract_id, || { + let legacy = crate::Question { + question_id: 7, + question: b(&env, "Legacy question?"), + hashed_answer: env.crypto().sha256(&b(&env, "legacy-answer")).into(), + level: crate::Levels::Easy, + hint: b(&env, "legacy hint"), + version: 0, + }; + env.storage() + .persistent() + .set(&crate::DataKey::Question(7), &legacy); + }); + + let got = client.get_question(&7u64); + assert_eq!(got.question_id, 7); + assert_eq!(got.version, 0); + assert_eq!(got.question, b(&env, "Legacy question?")); + assert_eq!(got.level, crate::Levels::Easy); +} + +/// `LevelProgress` values written to storage must round-trip field-for-field +/// through the public view. Appending a field in a future schema version +/// must preserve every existing field (documented in +/// onchain/docs/storage-versioning.md). +#[test] +fn test_level_progress_roundtrip_compat() { + let env = Env::default(); + let admin = new_admin(&env); + let contract_id = env.register_contract(None, StellarHunts); + let client = StellarHuntsClient::new(&env, &contract_id); + client.init(&admin); + + let player = user(&env); + let level = crate::Levels::Medium; + + let lp = crate::LevelProgress { + player: player.clone(), + level: level.clone(), + last_question_index: 3, + is_completed: true, + attempts: 5, + nft_minted: true, + last_attempt_ledger: 12345, + }; + + env.as_contract(&contract_id, || { + env.storage().persistent().set( + &crate::DataKey::PlayerLevelProgress(player.clone(), level.clone()), + &lp, + ); + }); + + let got = client.get_player_level_progress(&player, &level); + assert_eq!(got.player, player); + assert_eq!(got.level, level); + assert_eq!(got.last_question_index, 3); + assert!(got.is_completed); + assert_eq!(got.attempts, 5); + assert!(got.nft_minted); + assert_eq!(got.last_attempt_ledger, 12345); +} + +/// The numeric discriminants of `Levels` are persisted in storage and in +/// event payloads, so they must never be reordered or renumbered. +#[test] +fn test_levels_discriminants_stable() { + assert_eq!(crate::Levels::Easy as u32, 1); + assert_eq!(crate::Levels::Medium as u32, 2); + assert_eq!(crate::Levels::Hard as u32, 3); + assert_eq!(crate::Levels::Master as u32, 4); } diff --git a/onchain/docs/storage-versioning.md b/onchain/docs/storage-versioning.md new file mode 100644 index 00000000..3e872962 --- /dev/null +++ b/onchain/docs/storage-versioning.md @@ -0,0 +1,118 @@ +# Contract Storage Versioning + +This document defines the migration strategy for on-chain storage used by the +StellarHunts Soroban contracts (`stellar_hunts`, `stellar_hunts_nft`, +`stellar_hunts_receiver`) and the rules for evolving storage keys and +serialized types without breaking existing state. + +## Principles + +1. **Stable keys.** All persistent/instance storage is addressed through a + `#[contracttype]` key enum (`DataKey` / `NftDataKey`). The variant name and + its payloads are part of the serialized key, so renaming a variant creates + a *new* key and orphans the old data. + +2. **Versioned state.** Each contract records its schema version in instance + storage under `DataKey::SchemaVersion` (set at `init`) and exposes it via + `get_schema_version()`. A missing key means version `0` (pre-versioning + legacy deployment). + +3. **Never reuse a key for different meaning.** If the meaning of a stored + value changes, introduce a new key (or a versioned struct) instead of + overwriting an existing key with a different shape. + +4. **Backward compatibility is the default.** Reads must keep working for + state written by older contract versions; migrations upgrade data lazily + (on next read) or eagerly (during an admin-maintained migration), never by + silently dropping or corrupting old state. + +## Migration strategy for storage keys + +- A key is derived from the enum variant plus its payloads, e.g. + `Question(u64)` → `Question(question_id)`. The pair + (variant name, payload types) must stay stable for as long as any deployed + state may reference it. +- **Renaming a variant** = new key. To migrate: read under the old key, write + under the new key, then remove the old key (eager migration), or keep both + and migrate on first access (lazy migration). +- Keep the well-known keys referenced by off-chain integrations and emitted + events stable: + - `Question(u64)`, `QuestionCount`, `QuestionPerLevel`, + `QuestionsByLevel(Levels, u32)` + - `PlayerProgress(Address)`, `PlayerLevelProgress(Address, Levels)` + - `Badge(Address, Levels)`, `BadgeData(Address, Levels)` + +## Evolving serialized types + +Soroban `#[contracttype]` structs are XDR-serialized: the field layout is +part of the serialization. Two safe ways to evolve a stored struct: + +1. **Per-record version field (preferred).** Keep an explicit `version` + field on the struct itself, e.g. `Question.version`. Writers stamp the + current version; readers that encounter an older `version` can run the + appropriate upgrade. This is how `Question` is already handled. +2. **Versioned key suffixes.** When a struct changes incompatibly, store the + new shape under a new key (e.g. `PlayerProgressV2(Address)`) while the + reader falls back to the legacy key. Old keys are eventually purged by a + migration. + +### Changing enums + +- `Levels` uses **explicit discriminants** (`Easy = 1` … `Master = 4`). The + numeric value is what is persisted, so discriminants must never be + reordered or renumbered — append new variants only. +- Changing an enum's *meaning* is a breaking change: bump the schema version + and migrate any stored state that references the old variant. + +### Compatibility checklist + +| Change | Allowed? | Requirement | +| --- | --- | --- | +| Add a field to a stored struct | Yes (with care) | Bump `CURRENT_SCHEMA_VERSION`; readers tolerate the previous layout | +| Append a new enum variant | Yes | Never reorder or renumber existing discriminants | +| Reorder/renumber enum variants | No | Breaks existing state | +| Remove a struct field | No | Breaking; migrate stored data first | +| Reuse a key for different data | No | Use a new/versioned key instead | +| Rename a key variant | No | New key; migrate old data explicitly | + +## Contract-specific notes + +### `stellar_hunts` +- `CURRENT_SCHEMA_VERSION = 1`, stored under `DataKey::SchemaVersion` at + `init` and readable via `get_schema_version()`. +- `Question` carries a per-record `version` field stamped with + `CURRENT_SCHEMA_VERSION` at write time; readers treat an older `version` + as legacy-format data. +- `PlayerProgress` and `LevelProgress` currently have no version field — + adding one (or switching to `PlayerProgressV2`/`LevelProgressV2` keys) + requires a schema-version bump and a migration that seeds the new field + from existing state. + +### `stellar_hunts_nft` +- `Badge(Address, Levels)` is a presence flag; `BadgeData(Address, Levels)` + stores `minted_at` + `minter`. Both keys are append-only in practice + (badges are never unminted), so evolving `BadgeData` by adding fields is + backward compatible — old rows deserialize with defaults only if the new + fields are optional; otherwise bump and migrate. + +### `stellar_hunts_receiver` +- Stateless mock (no storage). Nothing to version. + +## Test expectations + +The compatibility suite in `stellar_hunts/src/test.rs` locks in these +guarantees: + +- `test_schema_version` / schema-version tests — `get_schema_version()` + returns `CURRENT_SCHEMA_VERSION` after `init` and `0` for a legacy + deployment that never wrote the key. +- `test_legacy_question_readable` — a `Question` written with + `version: 0` (pre-versioning format) is still returned by + `get_question`, proving reads are backward compatible. +- `test_level_progress_roundtrip_compat` — a `LevelProgress` written + directly to storage round-trips through `get_player_level_progress` + field-for-field, so appending a field in the future must preserve all + existing fields. +- `test_levels_discriminants_stable` — `Levels` numeric discriminants + (Easy=1, Medium=2, Hard=3, Master=4) never change, protecting both stored + state and event payloads.