From e8d1b89055f940064e85ace2a0b31d0bda2c5e4d Mon Sep 17 00:00:00 2001 From: Klaudia Blazyczek Date: Fri, 31 Jul 2026 09:02:21 +0200 Subject: [PATCH 1/4] feat(iam): audit authorisation denials reported by client-side guards --- .../audit-unauthorized-access-attempts.md | 9 + docs/catalog.json | 7 + .../contracts/adapters/admin-permission.ts | 10 +- .../core/src/contracts/adapters/rate-limit.ts | 1 + packages/core/src/iam/AGENTS.md | 1 + .../core/src/iam/__tests__/iam.router.test.ts | 28 +++ .../src/iam/__tests__/iam.service.test.ts | 14 +- packages/core/src/iam/contract/index.ts | 10 + packages/core/src/iam/router/index.ts | 5 + packages/core/src/iam/service/iam.service.ts | 19 ++ .../server/auth/__tests__/admin-guard.test.ts | 173 +++++++++++++++++- packages/core/src/server/auth/admin-guard.ts | 138 ++++++++++---- .../core/src/server/runtime/create-app.ts | 1 + packages/core/src/testing/mock.ts | 1 + 14 files changed, 378 insertions(+), 39 deletions(-) create mode 100644 .changeset/audit-unauthorized-access-attempts.md create mode 100644 packages/core/src/iam/__tests__/iam.router.test.ts diff --git a/.changeset/audit-unauthorized-access-attempts.md b/.changeset/audit-unauthorized-access-attempts.md new file mode 100644 index 00000000..d48517a1 --- /dev/null +++ b/.changeset/audit-unauthorized-access-attempts.md @@ -0,0 +1,9 @@ +--- +'@openora/core': minor +--- + +Authorisation denials that never reached `AdminGuard.assert()` were invisible to the audit log: a client-side page guard that blocks navigation before sending any request, and the iam service's own super-admin / grant-escalation checks (`NotSuperAdminError`, `GrantEscalationError`), both bypassed the guard and produced no `identity.user.unauthorized_access` event. + +- `AdminGuard.recordDeniedAccess(caller, resource, level)` lets a caller report a denial after the fact. It re-derives the actions for `(resource, level)` and re-checks each one against the caller's real grants - read via the new `AdminPermissionResolver.getFreshGrants` (bypasses the grants cache, so a just-granted permission can't be mis-reported as denied during the cache-purge window) - before emitting the actually-missing action (not just the first one a level expands to, which could be an action the caller already holds). A caller can only ever self-report a denial it genuinely hit; it cannot forge an entry for a resource it can access. Throttled 60s per `(user, resource, level)` via an atomic `RATE_LIMITER.consume` (fixed-window, safe under concurrent requests). +- New iam route `reportAccessDenied` (`POST /iam/access-denied`) exposes it to any admin session. +- `IamService.assertSuperAdmin` and the no-escalation check in `setRolePermissions` now emit `identity.user.unauthorized_access` before throwing, so these service-level denials are audited the same way `AdminGuard` denials always have been. diff --git a/docs/catalog.json b/docs/catalog.json index 3dabed31..e8ef0229 100644 --- a/docs/catalog.json +++ b/docs/catalog.json @@ -163,6 +163,7 @@ "iam.listInvitations", "iam.listRoles", "iam.previewEffectivePermissions", + "iam.reportAccessDenied", "iam.setRolePermissions", "iam.unassignRole", "iam.updateRole" @@ -672,6 +673,7 @@ "identity.user.phone_login", "identity.user.reactivated", "identity.user.registered", + "identity.user.unauthorized_access", "identity.user.unlocked", "notifications.created", "player.level.changed", @@ -1379,6 +1381,10 @@ "name": "replacePlayerTagSchema", "file": "packages/core/src/contracts/schemas/tag.ts" }, + { + "name": "ReportAccessDeniedSchema", + "file": "packages/core/src/iam/contract/index.ts" + }, { "name": "RequestKycResubmissionInputSchema", "file": "packages/core/src/compliance/contract/index.ts" @@ -1749,6 +1755,7 @@ "POST /compliance/players/{userId}/self-exclusion/lift", "POST /gaming/rounds/start", "POST /gaming/rounds/{roundId}/end", + "POST /iam/access-denied", "POST /iam/assignments", "POST /iam/assignments/force-logout", "POST /iam/effective-permissions", diff --git a/packages/core/src/contracts/adapters/admin-permission.ts b/packages/core/src/contracts/adapters/admin-permission.ts index fdd3ece2..c052f6bd 100644 --- a/packages/core/src/contracts/adapters/admin-permission.ts +++ b/packages/core/src/contracts/adapters/admin-permission.ts @@ -8,8 +8,16 @@ export type AdminGrant = { resource: string; action: string }; export type AdminPermissionResolver = { // Returns the effective grants for an admin user, or null if the user has no - // DB-backed role assignment (caller should fall back to static roles). + // DB-backed role assignment (caller should fall back to static roles). May be + // served from a cache - fine for the per-request authorization hot path. getGrants(userId: string): Promise; + // Same contract as getGrants but bypasses any caching layer. Optional; callers + // for whom a stale read is merely a hot-path perf trade-off (assert()) use + // getGrants, but a caller re-verifying a claim against the CURRENT grant state + // (e.g. AdminGuard.recordDeniedAccess, checking it isn't recording a denial for + // a permission the caller was just given) must not be fooled by a not-yet-purged + // cache entry and should call this instead. Falls back to getGrants when unset. + getFreshGrants?(userId: string): Promise; }; export const ADMIN_PERMISSION_RESOLVER: Token = createToken( diff --git a/packages/core/src/contracts/adapters/rate-limit.ts b/packages/core/src/contracts/adapters/rate-limit.ts index b7f2c230..dab75039 100644 --- a/packages/core/src/contracts/adapters/rate-limit.ts +++ b/packages/core/src/contracts/adapters/rate-limit.ts @@ -21,6 +21,7 @@ export const RATE_LIMIT_KEYS = { WALLET_MUTATION: 'wallet-mutation', CHAT_ROOM_JOIN: 'chat-room-join', CHAT_SEND: 'chat-send', + ACCESS_DENIED_REPORT: 'access-denied-report', } as const; export type RateLimitKeyPrefix = (typeof RATE_LIMIT_KEYS)[keyof typeof RATE_LIMIT_KEYS]; diff --git a/packages/core/src/iam/AGENTS.md b/packages/core/src/iam/AGENTS.md index f1e1015b..45aadfa3 100644 --- a/packages/core/src/iam/AGENTS.md +++ b/packages/core/src/iam/AGENTS.md @@ -25,6 +25,7 @@ Three totally ordered levels: `no_access` < `read` < `read_write`. Storage is sp - Override invitation email: `ctx.provide(SEND_EMAIL, ...)` in a later-loading overlay. - React to onboarding: `ctx.events.on('iam.invitation.accepted', ...)`. +- `reportAccessDenied` (`POST /iam/access-denied`) lets a client-side page guard - which never sends the underlying request, so `AdminGuard` never sees the attempt - report a denial after the fact. It only requires a valid admin session (`assert(context)`) and delegates to `AdminGuard.recordDeniedAccess(caller, resource, level)`, which re-derives the actions for `(resource, level)` via `levelToActions` (falling back to the full action set for a view-less module + `read`, eg `content`) and re-checks each one against a FRESH (uncached) read of the caller's grants - `AdminPermissionResolver.getFreshGrants` - before emitting the actually-missing action. This double-guards against forgery: a stale cached `getGrants` could still say "denied" for a permission the caller was just handed, and reporting the wrong action (rather than the one truly missing) would misrepresent a partial grant as a full denial. Throttled 60s per `(user, resource, level)` via an atomic `RATE_LIMITER.consume` (not the `CACHE` port - a non-atomic get+set throttle races under concurrent requests). `assertSuperAdmin` and the no-escalation check in `setRolePermissions` emit the same `identity.user.unauthorized_access` event before throwing `NotSuperAdminError`/`GrantEscalationError`, so service-level denials that never reach `AdminGuard.assert()` are audited too. - Add permission modules: edit `statement` in `packages/core/src/server/auth/permissions.ts` - catalog, levels, and validation all derive from it. - Each `iam.role.*` payload carries an explicit `actorId` (the envelope does not); the audit module subscribes to all of them. - Binds `ADMIN_PLAYER_ACTIVITY` (`adapters/admin-player-activity.ts`) - the back-office player-activity report (registrations over time, DAU/WAU/MAU trend, 7d/30d retention cohorts). It reads the `user`/`session` tables via identity's read-only `/schema` subpath rather than identity binding the port itself - iam already `dependsOn: ['identity']` and centralizes the other admin-reporting-style ports, so this keeps that a single seam for admin-console. "Active" is defined as a session row whose `updatedAt` falls in the window (better-auth refreshes it on continued use) - see the one-line comment on `getActiveUsersTrend` before changing it, this is a deliberate simplification, not a hard requirement. diff --git a/packages/core/src/iam/__tests__/iam.router.test.ts b/packages/core/src/iam/__tests__/iam.router.test.ts new file mode 100644 index 00000000..fcccc347 --- /dev/null +++ b/packages/core/src/iam/__tests__/iam.router.test.ts @@ -0,0 +1,28 @@ +import { describe, it, expect, vi } from 'vitest'; +import { call } from '@orpc/server'; +import type { AdminGuard } from '@openora/core/server'; +import { mock, testContext, adminCaller } from '../../testing/mock.js'; +import { createIamRouter } from '../router/index.js'; +import type { IamService } from '../service/iam.service.js'; + +const CTX = testContext(); + +describe('iam router - reportAccessDenied', () => { + it('asserts a plain admin session, then delegates to AdminGuard.recordDeniedAccess', async () => { + const caller = adminCaller(); + const recordDeniedAccess = vi.fn().mockResolvedValue({ recorded: true }); + const assert = vi.fn().mockResolvedValue(caller); + const adminGuard = mock({ assert, recordDeniedAccess }); + const router = createIamRouter(mock({}), adminGuard); + + const result = await call( + router.reportAccessDenied, + { resource: 'game', level: 'read' }, + { context: CTX }, + ); + + expect(result).toEqual({ recorded: true }); + expect(assert).toHaveBeenCalledWith(CTX); + expect(recordDeniedAccess).toHaveBeenCalledWith(caller, 'game', 'read'); + }); +}); diff --git a/packages/core/src/iam/__tests__/iam.service.test.ts b/packages/core/src/iam/__tests__/iam.service.test.ts index e9e8ead9..299fabca 100644 --- a/packages/core/src/iam/__tests__/iam.service.test.ts +++ b/packages/core/src/iam/__tests__/iam.service.test.ts @@ -197,9 +197,10 @@ describe('DbAdminPermissionResolver caching (real Redis read-through)', () => { }); describe('IamService.setRolePermissions', () => { - it('rejects a non-super-admin caller', async () => { + it('rejects a non-super-admin caller and audits the denial', async () => { const role = await seedRole(); - const svc = new IamService(db.drizzle, makeEventBus(), makeEmail()); + const events = makeEventBus(); + const svc = new IamService(db.drizzle, events, makeEmail()); await expect( svc.setRolePermissions({ roleId: role.id, @@ -207,6 +208,15 @@ describe('IamService.setRolePermissions', () => { caller: SUPPORT_CALLER, }), ).rejects.toBeInstanceOf(NotSuperAdminError); + expect(events.emit).toHaveBeenCalledWith( + 'identity.user.unauthorized_access', + expect.objectContaining({ + userId: SUPPORT_CALLER.userId, + resource: 'admin', + action: 'update', + role: 'support', + }), + ); }); it('throws RoleNotFoundError when the role does not exist', async () => { diff --git a/packages/core/src/iam/contract/index.ts b/packages/core/src/iam/contract/index.ts index ddc69216..7010cfa5 100644 --- a/packages/core/src/iam/contract/index.ts +++ b/packages/core/src/iam/contract/index.ts @@ -74,6 +74,10 @@ export const EffectivePermissionsSchema = z.object({ permissions: z.array(RolePermissionLevelSchema), }); +export const ReportAccessDeniedSchema = z.object({ + recorded: z.boolean(), +}); + export const IAM_ROLE_SORT_BY_VALUES = ['name', 'createdAt', 'key'] as const; export const IamRoleSortBySchema = z.enum(IAM_ROLE_SORT_BY_VALUES).default('name'); export type IamRoleSortBy = z.infer; @@ -189,6 +193,11 @@ export const iamContract = { getMyPermissions: oc .route({ method: 'GET', path: '/iam/my-permissions' }) .output(EffectivePermissionsSchema), + + reportAccessDenied: oc + .route({ method: 'POST', path: '/iam/access-denied' }) + .input(z.object({ resource: z.string(), level: PermissionLevelSchema })) + .output(ReportAccessDeniedSchema), }; export type AdminRole = z.infer; @@ -201,3 +210,4 @@ export type Catalog = z.infer; export type GrantInput = z.infer; export type RolePermissionLevel = z.infer; export type EffectivePermissions = z.infer; +export type ReportAccessDenied = z.infer; diff --git a/packages/core/src/iam/router/index.ts b/packages/core/src/iam/router/index.ts index 78709188..b8758390 100644 --- a/packages/core/src/iam/router/index.ts +++ b/packages/core/src/iam/router/index.ts @@ -114,5 +114,10 @@ export function createIamRouter(svc: IamService, adminGuard: AdminGuard) { const caller = await adminGuard.assert(context); return svc.previewEffectivePermissions({ userId: caller.userId }); }), + + reportAccessDenied: os.reportAccessDenied.handler(async ({ input, context }) => { + const caller = await adminGuard.assert(context); + return adminGuard.recordDeniedAccess(caller, input.resource, input.level); + }), }); } diff --git a/packages/core/src/iam/service/iam.service.ts b/packages/core/src/iam/service/iam.service.ts index 3d348c89..2a17d40f 100644 --- a/packages/core/src/iam/service/iam.service.ts +++ b/packages/core/src/iam/service/iam.service.ts @@ -187,6 +187,12 @@ export class DbAdminPermissionResolver implements AdminPermissionResolver { ); } + // Bypasses the cache entirely - for callers that must not act on a not-yet-purged + // stale entry (see the AdminPermissionResolver.getFreshGrants doc). + getFreshGrants(userId: User['id']): Promise { + return this.loadGrants(userId); + } + private async loadGrants(userId: User['id']): Promise { // One indexed join (on admin_role_assignment_user_id_idx) replaces the old // 2 + N-per-role fan-out. leftJoin keeps super-admin roles (no permission rows) @@ -293,10 +299,22 @@ export class IamService { private async assertSuperAdmin(caller: Caller) { if (!(await this.isSuperAdmin(caller))) { + this.emitDenied(caller, 'admin', 'update'); throw new NotSuperAdminError(); } } + private emitDenied(caller: Caller, resource: string, action: string) { + this.events.emit('identity.user.unauthorized_access', { + userId: caller.userId, + resource, + action, + ip: caller.ip, + userAgent: caller.userAgent, + role: caller.role, + }); + } + listCatalog() { return buildCatalog(); } @@ -488,6 +506,7 @@ export class IamService { } const have = callerMap[g.resource] ?? 'no_access'; if (!isLevelSufficient(have, g.level)) { + this.emitDenied(input.caller, g.resource, 'update'); throw new GrantEscalationError(); } } diff --git a/packages/core/src/server/auth/__tests__/admin-guard.test.ts b/packages/core/src/server/auth/__tests__/admin-guard.test.ts index 7dd7d7b4..020a5c28 100644 --- a/packages/core/src/server/auth/__tests__/admin-guard.test.ts +++ b/packages/core/src/server/auth/__tests__/admin-guard.test.ts @@ -1,9 +1,13 @@ import { describe, it, expect, vi, beforeAll, afterAll, beforeEach } from 'vitest'; import { randomUUID } from 'node:crypto'; import { sql } from 'drizzle-orm'; -import type { AdminPermissionResolver } from '@openora/core/contracts'; +import type { + AdminPermissionResolver, + RateLimiterAdapter, + RateLimitKey, +} from '@openora/core/contracts'; import { createTestDb, type TestDb } from '@openora/core/testing'; -import { mock, makeEventBus } from '../../../testing/mock.js'; +import { mock, makeEventBus, adminCaller } from '../../../testing/mock.js'; import { AdminGuard } from '../admin-guard.js'; import type { SessionResolver } from '../session-resolver.js'; @@ -16,16 +20,37 @@ const ADMIN_HEADERS = { 'x-real-ip': '127.0.0.1', 'user-agent': 'Mozilla/5.0' }; function makeGuard({ userId, grants, -}: { userId?: string; grants?: { resource: string; action: string }[] } = {}) { + rateLimiter, +}: { + userId?: string; + grants?: { resource: string; action: string }[]; + rateLimiter?: RateLimiterAdapter; +} = {}) { const events = makeEventBus(); const sessions = mock({ resolveUserId: vi.fn(async () => userId) }); const permissionResolver = grants ? mock({ getGrants: vi.fn(async () => grants) }) : undefined; - const guard = new AdminGuard(db.drizzle, sessions, permissionResolver, events); + const guard = new AdminGuard(db.drizzle, sessions, permissionResolver, events, rateLimiter); return { guard, events }; } +/** In-memory fixed-window RateLimiterAdapter double - state carries across calls within one test. */ +function fakeStatefulRateLimiter(): RateLimiterAdapter { + const counts = new Map(); + return mock>({ + consume: vi.fn(async (key: string, opts: { limit: number }) => { + const count = (counts.get(key) ?? 0) + 1; + counts.set(key, count); + const allowed = count <= opts.limit; + return { allowed, retryAfterMs: allowed ? 0 : 1000 }; + }), + reset: vi.fn(async (key: string) => { + counts.delete(key); + }), + }); +} + async function seedUser(role: string) { const id = randomUUID(); await db.drizzle.db.execute(sql`INSERT INTO "user" (id, role) VALUES (${id}, ${role})`); @@ -197,3 +222,143 @@ describe('AdminGuard.assert - DB grants (real PG)', () => { ); }); }); + +describe('AdminGuard.recordDeniedAccess', () => { + it('emits and records when the caller genuinely lacks the level (static role fallback)', async () => { + const { guard, events } = makeGuard(); + const caller = adminCaller({ role: 'support' }); // supportRole has no `game` grant + + await expect(guard.recordDeniedAccess(caller, 'game', 'read')).resolves.toEqual({ + recorded: true, + }); + expect(events.emit).toHaveBeenCalledWith( + 'identity.user.unauthorized_access', + expect.objectContaining({ + userId: caller.userId, + resource: 'game', + action: 'view', + role: 'support', + }), + ); + }); + + it('is a no-op when the caller already holds the level (static role fallback) - anti-forgery', async () => { + const { guard, events } = makeGuard(); + const caller = adminCaller({ role: 'admin' }); // adminRole grants game:view/enable/disable + + await expect(guard.recordDeniedAccess(caller, 'game', 'read')).resolves.toEqual({ + recorded: false, + }); + expect(events.emit).not.toHaveBeenCalled(); + }); + + it('is a no-op when DB grants already cover the level - anti-forgery via the DB path', async () => { + const { guard, events } = makeGuard({ grants: [{ resource: 'game', action: 'view' }] }); + const caller = adminCaller({ role: 'support' }); // static role would deny, DB grants allow + + await expect(guard.recordDeniedAccess(caller, 'game', 'read')).resolves.toEqual({ + recorded: false, + }); + expect(events.emit).not.toHaveBeenCalled(); + }); + + it('throws BAD_REQUEST for a resource with no entry in the permission statement', async () => { + const { guard } = makeGuard(); + const caller = adminCaller(); + + await expect(guard.recordDeniedAccess(caller, 'not-a-resource', 'read')).rejects.toThrow( + expect.objectContaining({ code: 'BAD_REQUEST' }), + ); + }); + + it('throws BAD_REQUEST for level: no_access', async () => { + const { guard } = makeGuard(); + const caller = adminCaller(); + + await expect(guard.recordDeniedAccess(caller, 'game', 'no_access')).rejects.toThrow( + expect.objectContaining({ code: 'BAD_REQUEST' }), + ); + }); + + it('reports the actually-missing action, not just the first action for the level - partial grant', async () => { + // `game` grants view/enable/disable at read_write; a caller who holds only + // `view` is missing enable/disable, NOT view - the audit must name a real gap. + const { guard, events } = makeGuard({ grants: [{ resource: 'game', action: 'view' }] }); + const caller = adminCaller({ role: 'support' }); + + await expect(guard.recordDeniedAccess(caller, 'game', 'read_write')).resolves.toEqual({ + recorded: true, + }); + expect(events.emit).toHaveBeenCalledWith( + 'identity.user.unauthorized_access', + expect.objectContaining({ resource: 'game', action: 'enable' }), + ); + expect(events.emit).not.toHaveBeenCalledWith( + 'identity.user.unauthorized_access', + expect.objectContaining({ action: 'view' }), + ); + }); + + it('falls back to the full action set for a view-less resource + read (content)', async () => { + // `content` has no `view` action, so levelToActions('content', 'read') is [] - + // that must not be mistaken for "nothing to check" (which would wrongly no-op). + const { guard, events } = makeGuard(); + const caller = adminCaller({ role: 'support' }); // supportRole has no `content` grant + + await expect(guard.recordDeniedAccess(caller, 'content', 'read')).resolves.toEqual({ + recorded: true, + }); + expect(events.emit).toHaveBeenCalledWith( + 'identity.user.unauthorized_access', + expect.objectContaining({ resource: 'content' }), + ); + }); + + it('re-checks against FRESH grants, not a stale cached getGrants - anti-forgery under a TOCTOU race', async () => { + // Simulates the window between a real grant and its async cache purge: getGrants + // (cached) still says "no game grant"; getFreshGrants (uncached, direct read) + // says the caller was just given it. The report must trust the fresh read. + const getGrants = vi.fn(async () => []); + const getFreshGrants = vi.fn(async () => [{ resource: 'game', action: 'view' }]); + const permissionResolver = mock({ getGrants, getFreshGrants }); + const events = makeEventBus(); + const sessions = mock({ resolveUserId: vi.fn(async () => undefined) }); + const guard = new AdminGuard(db.drizzle, sessions, permissionResolver, events); + const caller = adminCaller({ role: 'support' }); + + await expect(guard.recordDeniedAccess(caller, 'game', 'read')).resolves.toEqual({ + recorded: false, + }); + expect(getFreshGrants).toHaveBeenCalledWith(caller.userId); + expect(getGrants).not.toHaveBeenCalled(); + expect(events.emit).not.toHaveBeenCalled(); + }); + + it('throttles repeated denials for the same (user, resource, level) within the window', async () => { + const rateLimiter = fakeStatefulRateLimiter(); + const { guard, events } = makeGuard({ rateLimiter }); + const caller = adminCaller({ role: 'support' }); + + await expect(guard.recordDeniedAccess(caller, 'game', 'read')).resolves.toEqual({ + recorded: true, + }); + await expect(guard.recordDeniedAccess(caller, 'game', 'read')).resolves.toEqual({ + recorded: false, + }); + await expect(guard.recordDeniedAccess(caller, 'game', 'read')).resolves.toEqual({ + recorded: false, + }); + expect(events.emit).toHaveBeenCalledTimes(1); + }); + + it('does not throttle across different resources for the same user', async () => { + const rateLimiter = fakeStatefulRateLimiter(); + const { guard, events } = makeGuard({ rateLimiter }); + const caller = adminCaller({ role: 'support' }); + + await guard.recordDeniedAccess(caller, 'game', 'read'); + await guard.recordDeniedAccess(caller, 'game-config', 'read'); + + expect(events.emit).toHaveBeenCalledTimes(2); + }); +}); diff --git a/packages/core/src/server/auth/admin-guard.ts b/packages/core/src/server/auth/admin-guard.ts index 2501130f..87e6e65b 100644 --- a/packages/core/src/server/auth/admin-guard.ts +++ b/packages/core/src/server/auth/admin-guard.ts @@ -2,14 +2,20 @@ import { ORPCError } from '@orpc/server'; import { createToken, AuthGuardReasonSchema, + RATE_LIMIT_KEYS, + makeRateLimitKey, type Token, type AdminPermissionResolver, + type AdminGrant, type ClientMeta, + type RateLimiterAdapter, + type RateLimitKey, } from '@openora/core/contracts'; import { DrizzleService } from '../db/index.js'; import { sql } from 'drizzle-orm'; import { SessionResolver } from './session-resolver.js'; -import { roles, type ResourceName, type ActionOf } from './permissions.js'; +import { statement, roles, type ResourceName, type ActionOf } from './permissions.js'; +import { levelToActions, type PermissionLevel } from './permission-levels.js'; import type { OssContext, EventBus } from '../kernel/index.js'; import { extractClientMeta } from '../kernel/router-utils.js'; @@ -17,6 +23,8 @@ export const ADMIN_GUARD: Token = createToken('ADMIN_GUARD'); export type AdminCaller = { userId: string; role: string } & ClientMeta; +const DENIED_ACCESS_THROTTLE_MS = 60_000; + /** * The single admin-enforcement point - every admin route calls `assert()` as its * first line, never re-implementing the role check. Overload without @@ -37,6 +45,8 @@ export class AdminGuard { // When bound (iam module loaded), grants come from DB; otherwise falls back to static roles. private readonly permissionResolver?: AdminPermissionResolver, private readonly events?: EventBus, + // Throttles recordDeniedAccess(); unbound just means no throttling (best-effort). + private readonly rateLimiter?: RateLimiterAdapter, ) {} async assert(context: unknown): Promise; @@ -105,43 +115,107 @@ export class AdminGuard { } if (resource !== undefined && action !== undefined) { - const grants = this.permissionResolver - ? await this.permissionResolver.getGrants(userId) - : null; - - if (grants !== null) { - const allowed = grants.some((g) => g.resource === resource && g.action === action); - if (!allowed) { - this.emitUnauthorized(userId, userRecord.role, resource, action, ip, userAgent); - throw new ORPCError('FORBIDDEN', { - message: `Missing permission: ${String(resource)}:${String(action)}`, - data: { - reason: AuthGuardReasonSchema.enum.permission_denied, - resource: String(resource), - action: String(action), - }, - }); - } - } else { - // Bootstrap path: seed admin has user.role='admin' but no DB assignment row. DB revocation is NOT authoritative while a static role still grants - revoke the static role to fully deny. - const check = userRole.authorize({ [resource]: [action] }); - if (!check.success) { - this.emitUnauthorized(userId, userRecord.role, resource, action, ip, userAgent); - throw new ORPCError('FORBIDDEN', { - message: `Missing permission: ${String(resource)}:${String(action)}`, - data: { - reason: AuthGuardReasonSchema.enum.permission_denied, - resource: String(resource), - action: String(action), - }, - }); - } + const grants = await this.resolveGrants(userId); + const allowed = this.checkGrant(grants, userRole, resource, action); + if (!allowed) { + this.emitUnauthorized(userId, userRecord.role, resource, action, ip, userAgent); + throw new ORPCError('FORBIDDEN', { + message: `Missing permission: ${String(resource)}:${String(action)}`, + data: { + reason: AuthGuardReasonSchema.enum.permission_denied, + resource: String(resource), + action: String(action), + }, + }); } } return { userId, role: userRecord.role, ip, userAgent }; } + async recordDeniedAccess( + caller: AdminCaller, + resource: string, + level: PermissionLevel, + ): Promise<{ recorded: boolean }> { + const knownActions = statement[resource as ResourceName] as readonly string[] | undefined; + if (!knownActions) { + throw new ORPCError('BAD_REQUEST', { message: `Unknown resource: ${resource}` }); + } + if (level === 'no_access') { + throw new ORPCError('BAD_REQUEST', { message: 'level must not be no_access' }); + } + // levelToActions('read') on a view-less module (eg `content`) is deliberately [] + // (see permission-levels.ts) - fall back to the full action set rather than + // treating that as "nothing to check", which would wrongly no-op the report. + const derivedActions = levelToActions(resource, level); + const actions = derivedActions.length > 0 ? derivedActions : knownActions; + + const userRole = roles[caller.role as keyof typeof roles]; + // Fresh (uncached) grants: a cached `getGrants` could still reflect the + // pre-grant state for up to its TTL, which would let a caller who was JUST + // given this permission report a denial it no longer has - self-forging a + // false audit entry in the gap before the cache purge lands. + const grants = await this.resolveFreshGrants(caller.userId); + const missingAction = actions.find( + (action) => !this.checkGrant(grants, userRole, resource, action), + ); + if (!missingAction) { + return { recorded: false }; + } + + if (this.rateLimiter) { + const key = makeRateLimitKey( + RATE_LIMIT_KEYS.ACCESS_DENIED_REPORT, + `${caller.userId}:${resource}:${level}`, + ); + const { allowed } = await this.rateLimiter.consume(key, { + limit: 1, + windowMs: DENIED_ACCESS_THROTTLE_MS, + }); + if (!allowed) { + return { recorded: false }; + } + } + + this.emitUnauthorized( + caller.userId, + caller.role, + resource, + missingAction, + caller.ip, + caller.userAgent, + ); + return { recorded: true }; + } + + private resolveGrants(userId: string): Promise { + return this.permissionResolver + ? this.permissionResolver.getGrants(userId) + : Promise.resolve(null); + } + + private resolveFreshGrants(userId: string): Promise { + if (!this.permissionResolver) { + return Promise.resolve(null); + } + return this.permissionResolver.getFreshGrants + ? this.permissionResolver.getFreshGrants(userId) + : this.permissionResolver.getGrants(userId); + } + + private checkGrant( + grants: AdminGrant[] | null, + userRole: (typeof roles)[keyof typeof roles] | undefined, + resource: string, + action: string, + ): boolean { + if (grants !== null) { + return grants.some((g) => g.resource === resource && g.action === action); + } + return userRole?.authorize({ [resource]: [action] }).success ?? false; + } + private emitUnauthorized( userId: string, role: string | undefined, diff --git a/packages/core/src/server/runtime/create-app.ts b/packages/core/src/server/runtime/create-app.ts index 2ddb0a39..fb33ff88 100644 --- a/packages/core/src/server/runtime/create-app.ts +++ b/packages/core/src/server/runtime/create-app.ts @@ -279,6 +279,7 @@ export async function createApp(config: CreateAppConfig): Promise { // has() avoids throwing on an unbound token so boot works without the iam module. c.has(ADMIN_PERMISSION_RESOLVER) ? c.get(ADMIN_PERMISSION_RESOLVER) : undefined, c.get(EVENT_BUS), + c.has(RATE_LIMITER) ? c.get(RATE_LIMITER) : undefined, ), ); if (config.igaming) { diff --git a/packages/core/src/testing/mock.ts b/packages/core/src/testing/mock.ts index fba4bfd4..20920a01 100644 --- a/packages/core/src/testing/mock.ts +++ b/packages/core/src/testing/mock.ts @@ -131,4 +131,5 @@ export const makeAdminGuard = ( } return adminCaller(options.caller); }), + recordDeniedAccess: vi.fn(async () => ({ recorded: true })), }); From ef56d0a9c5fe5dedcf15f8d7a1cd810f1cf923de Mon Sep 17 00:00:00 2001 From: Klaudia Blazyczek Date: Fri, 31 Jul 2026 09:16:02 +0200 Subject: [PATCH 2/4] chore: clear unimportant comments --- .../server/auth/__tests__/admin-guard.test.ts | 16 ++++------------ packages/core/src/server/auth/admin-guard.ts | 8 -------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/packages/core/src/server/auth/__tests__/admin-guard.test.ts b/packages/core/src/server/auth/__tests__/admin-guard.test.ts index 020a5c28..c09d3aec 100644 --- a/packages/core/src/server/auth/__tests__/admin-guard.test.ts +++ b/packages/core/src/server/auth/__tests__/admin-guard.test.ts @@ -35,7 +35,6 @@ function makeGuard({ return { guard, events }; } -/** In-memory fixed-window RateLimiterAdapter double - state carries across calls within one test. */ function fakeStatefulRateLimiter(): RateLimiterAdapter { const counts = new Map(); return mock>({ @@ -226,7 +225,7 @@ describe('AdminGuard.assert - DB grants (real PG)', () => { describe('AdminGuard.recordDeniedAccess', () => { it('emits and records when the caller genuinely lacks the level (static role fallback)', async () => { const { guard, events } = makeGuard(); - const caller = adminCaller({ role: 'support' }); // supportRole has no `game` grant + const caller = adminCaller({ role: 'support' }); await expect(guard.recordDeniedAccess(caller, 'game', 'read')).resolves.toEqual({ recorded: true, @@ -244,7 +243,7 @@ describe('AdminGuard.recordDeniedAccess', () => { it('is a no-op when the caller already holds the level (static role fallback) - anti-forgery', async () => { const { guard, events } = makeGuard(); - const caller = adminCaller({ role: 'admin' }); // adminRole grants game:view/enable/disable + const caller = adminCaller({ role: 'admin' }); await expect(guard.recordDeniedAccess(caller, 'game', 'read')).resolves.toEqual({ recorded: false, @@ -254,7 +253,7 @@ describe('AdminGuard.recordDeniedAccess', () => { it('is a no-op when DB grants already cover the level - anti-forgery via the DB path', async () => { const { guard, events } = makeGuard({ grants: [{ resource: 'game', action: 'view' }] }); - const caller = adminCaller({ role: 'support' }); // static role would deny, DB grants allow + const caller = adminCaller({ role: 'support' }); await expect(guard.recordDeniedAccess(caller, 'game', 'read')).resolves.toEqual({ recorded: false, @@ -281,8 +280,6 @@ describe('AdminGuard.recordDeniedAccess', () => { }); it('reports the actually-missing action, not just the first action for the level - partial grant', async () => { - // `game` grants view/enable/disable at read_write; a caller who holds only - // `view` is missing enable/disable, NOT view - the audit must name a real gap. const { guard, events } = makeGuard({ grants: [{ resource: 'game', action: 'view' }] }); const caller = adminCaller({ role: 'support' }); @@ -300,10 +297,8 @@ describe('AdminGuard.recordDeniedAccess', () => { }); it('falls back to the full action set for a view-less resource + read (content)', async () => { - // `content` has no `view` action, so levelToActions('content', 'read') is [] - - // that must not be mistaken for "nothing to check" (which would wrongly no-op). const { guard, events } = makeGuard(); - const caller = adminCaller({ role: 'support' }); // supportRole has no `content` grant + const caller = adminCaller({ role: 'support' }); await expect(guard.recordDeniedAccess(caller, 'content', 'read')).resolves.toEqual({ recorded: true, @@ -315,9 +310,6 @@ describe('AdminGuard.recordDeniedAccess', () => { }); it('re-checks against FRESH grants, not a stale cached getGrants - anti-forgery under a TOCTOU race', async () => { - // Simulates the window between a real grant and its async cache purge: getGrants - // (cached) still says "no game grant"; getFreshGrants (uncached, direct read) - // says the caller was just given it. The report must trust the fresh read. const getGrants = vi.fn(async () => []); const getFreshGrants = vi.fn(async () => [{ resource: 'game', action: 'view' }]); const permissionResolver = mock({ getGrants, getFreshGrants }); diff --git a/packages/core/src/server/auth/admin-guard.ts b/packages/core/src/server/auth/admin-guard.ts index 87e6e65b..730995aa 100644 --- a/packages/core/src/server/auth/admin-guard.ts +++ b/packages/core/src/server/auth/admin-guard.ts @@ -45,7 +45,6 @@ export class AdminGuard { // When bound (iam module loaded), grants come from DB; otherwise falls back to static roles. private readonly permissionResolver?: AdminPermissionResolver, private readonly events?: EventBus, - // Throttles recordDeniedAccess(); unbound just means no throttling (best-effort). private readonly rateLimiter?: RateLimiterAdapter, ) {} @@ -145,17 +144,10 @@ export class AdminGuard { if (level === 'no_access') { throw new ORPCError('BAD_REQUEST', { message: 'level must not be no_access' }); } - // levelToActions('read') on a view-less module (eg `content`) is deliberately [] - // (see permission-levels.ts) - fall back to the full action set rather than - // treating that as "nothing to check", which would wrongly no-op the report. const derivedActions = levelToActions(resource, level); const actions = derivedActions.length > 0 ? derivedActions : knownActions; const userRole = roles[caller.role as keyof typeof roles]; - // Fresh (uncached) grants: a cached `getGrants` could still reflect the - // pre-grant state for up to its TTL, which would let a caller who was JUST - // given this permission report a denial it no longer has - self-forging a - // false audit entry in the gap before the cache purge lands. const grants = await this.resolveFreshGrants(caller.userId); const missingAction = actions.find( (action) => !this.checkGrant(grants, userRole, resource, action), From d5ac8c994b03de6b775a7979ece02ba0a2777527 Mon Sep 17 00:00:00 2001 From: Klaudia Blazyczek Date: Fri, 31 Jul 2026 12:33:40 +0200 Subject: [PATCH 3/4] chore: update codeowners --- CODEOWNERS | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/CODEOWNERS b/CODEOWNERS index e0d7879d..d80597fe 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -1,12 +1 @@ -# Code owners for oss (the platform core). -# GitHub uses this to request reviewers on PRs into protected branches. -# -# Ownership by area, not by person - team can change: -# - Backend core (contracts, domains, server, tooling) -> platform maintainer + backend dev -# - Frontend SDK (react consumption layer) -> frontend owner, reviewed with the maintainer -# Consumers read this repo as upstream; they don't edit it. - -* @zaxovaiko @agniev-a-hub @klaudia-blazyczek-blurify - -# Frontend SDK (react consumption layer) -/packages/core/src/react/ @klaudia-blazyczek-blurify @zaxovaiko +* @zaxovaiko @agniev-a-hub @klaudia-blazyczek-blurify @marek-chmielowski-blurify From a217e180a0babdd48b858404dd51384142cc74cf Mon Sep 17 00:00:00 2001 From: Volodymyr Zakhovaiko Date: Fri, 31 Jul 2026 14:40:28 +0200 Subject: [PATCH 4/4] docs(rules): streamline PR descriptions --- .github/pull_request_template.md | 16 ++++++---------- .rulesync/rules/conventions.md | 19 ++++++------------- 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 501a4a71..e76db6d6 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,19 +1,15 @@ ## Summary - + ## Why - + -## Worth knowing +## Alternatives considered - + -- [ ] `pnpm verify` is green (typecheck + lint + boundaries + module-shape + tests) -- [ ] `pnpm check:drift` is green (catalog / OpenAPI not stale) - run `pnpm regen` if not -- [ ] New cross-module talk goes through events / command ports / contracts / the `/schema` subpath (no direct module imports) -- [ ] New data tables carry `tenantId` and are RLS-covered (`pnpm regen` runs `gen:rls`) -- [ ] No secrets, real player data, or internal/customer names added +## Risks -Closes ANPI-XXX / BF-XXX + diff --git a/.rulesync/rules/conventions.md b/.rulesync/rules/conventions.md index cb9f8d2e..cf732d67 100644 --- a/.rulesync/rules/conventions.md +++ b/.rulesync/rules/conventions.md @@ -148,20 +148,13 @@ Goal: code that is clean, separated, scalable, and extendible - easy to understa ## 5. Comments and documentation -- **Zero comments. A comment is an exception you must justify, not a nicety.** Assume the answer is "no comment" and let the code carry the meaning. -- **The only thing that earns one: a fact the code CANNOT contain** - an external system's behaviour, a third-party bug, a spec/regulatory constraint. The test is whether a careful reader would otherwise "fix" the code and break it. `// Stripe rounds half-to-even; mirror it so our totals reconcile.` -- **A reason is not a fact - it does not earn a comment.** Why this order, why 2 retries and not 4, why not the obvious approach, what a block does, what changed: all of that goes in the commit message, the PR description, or an ADR. Those are versioned and reviewed; an inline rationale is neither, and it rots in place. Naming the thing well (`PLAYER_FACING_TIMEOUT_MS`) beats a paragraph above it. -- **If a block needs a comment to be understood, rename or extract first** - a comment is the fallback after that fails, never the first move. Writing one is the signal that the naming or the decomposition is wrong. -- **Never in tests.** A test name states the behaviour and the assertions state the evidence. Seeded values, fixture choices and timing tricks get named constants or helpers, not narration. -- **Same bar in config, CI and infra files** (`turbo.json`, workflow YAML, compose). Step names and keys are self-describing; step ordering and tuning rationale belong in the commit that introduced them. -- **Never** restate a name (`// increment the counter`), narrate steps (`// step 2`), announce edits (`// added for X`), or divide sections (`// ---`, `// ===`). -- **JSDoc on every exported function/class >~15 lines or with non-obvious params.** Multiline `/** ... */` block (opening and closing on their own lines). Document the surprising contract, not the name. -- **`// TODO:` for deferred work, `// FIXME:` for known-broken code** - greppable, with context and an issue key where one exists. Never bare. +- **No comments by default.** Code, tests, config, and CI must be clear through names, structure, and decomposition. Do not add explanatory comments, rationale, narration, section dividers, or JSDoc. +- **The only exception is unfinished work that remains after this PR:** use a short `// TODO:` for deferred work or `// FIXME:` for known-broken code. Include an issue key when one exists; never leave either bare. ```ts - // TODO: replace polling with the webhook once BE ships it (ABC-312) - // FIXME: race - two admins approving the same withdrawal double-credit the player + // TODO: replace polling after ABC-312 + // FIXME: concurrent approvals can double-credit ``` -- **`// mock:` marks placeholder data / stubbed behavior** so throwaway code stays findable. `// mock: fixed rate until the FX adapter lands` +- **Fix it in this PR when practical; otherwise keep the exception short and actionable.** Never use comments to describe what code does or why an approach was chosen - put decision context in the PR description or ADR instead. ## 6. Structure and boundaries @@ -222,7 +215,7 @@ Headless repo - only the SDK consumption layer (hooks, typed client, auth, realt - **One PR = one concern.** Stage files explicitly; never `git add -A` with foreign changes in the tree. - **Green before review:** `pnpm verify` passes; `pnpm regen` after any contract/schema change. - **Branch off `dev`; never commit directly to `dev`/`stage`.** Promotion chain `dev -> stage` + release tags. Never push without an explicit per-action confirmation. -- **PR description carries intent:** what / why / acceptance criteria / ticket key. +- **PR description is reviewer-oriented:** Summary, Why, Alternatives considered, and Risks. Do not repeat CI commands, test counts, check status, or the diff itself. Include manual evidence only when CI cannot provide it. - **No sensitive/internal data in titles, descriptions, or commits** - they are the public record. Bare ticket key (`ABC-45`), never the URL; no internal links, hostnames, secrets, PII. When in doubt, leave it out. ## 13. Enforcement