From 7e5c28d62196f67b4ba1f00baac2a8ee8e22aa24 Mon Sep 17 00:00:00 2001 From: dswbx Date: Mon, 13 Oct 2025 21:03:49 +0200 Subject: [PATCH] enhance Guard and permission handling with new test cases - Updated the `Guard` class to improve context validation and permission checks, ensuring clearer error messages for unmet conditions. - Refactored the `Policy` and `RolePermission` classes to support default effects and better handle conditions and filters. - Enhanced tests in `authorize.spec.ts` and `permissions.spec.ts` to cover new permission scenarios, including guest and member role behaviors. - Added new tests for context validation in permission middleware, ensuring robust error handling for invalid contexts. - Improved utility functions for better integration with the updated permission structure. --- app/__test__/auth/authorize/authorize.spec.ts | 153 ++++++++++++++++-- .../auth/authorize/permissions.spec.ts | 116 +++++++++++-- app/__test__/core/object/object-query.spec.ts | 10 ++ app/src/auth/authorize/Guard.ts | 57 ++++--- app/src/auth/authorize/Policy.ts | 3 + app/src/auth/authorize/Role.ts | 10 +- .../auth/middlewares/permission.middleware.ts | 3 +- app/src/core/object/query/query.ts | 8 +- app/src/core/utils/runtime.ts | 9 +- 9 files changed, 317 insertions(+), 52 deletions(-) diff --git a/app/__test__/auth/authorize/authorize.spec.ts b/app/__test__/auth/authorize/authorize.spec.ts index fbf787f..5e39fb8 100644 --- a/app/__test__/auth/authorize/authorize.spec.ts +++ b/app/__test__/auth/authorize/authorize.spec.ts @@ -1,19 +1,12 @@ import { describe, expect, test } from "bun:test"; import { Guard, type GuardConfig } from "auth/authorize/Guard"; import { Permission } from "auth/authorize/Permission"; -import { Role } from "auth/authorize/Role"; -import { objectTransform } from "bknd/utils"; +import { Role, type RoleSchema } from "auth/authorize/Role"; +import { objectTransform, s } from "bknd/utils"; function createGuard( permissionNames: string[], - roles?: Record< - string, - { - permissions?: string[]; - is_default?: boolean; - implicit_allow?: boolean; - } - >, + roles?: Record>, config?: GuardConfig, ) { const _roles = roles @@ -26,7 +19,9 @@ function createGuard( } describe("authorize", () => { - const read = new Permission("read"); + const read = new Permission("read", { + filterable: true, + }); const write = new Permission("write"); test("basic", async () => { @@ -109,4 +104,140 @@ describe("authorize", () => { expect(guard.granted(read, {})).toBeUndefined(); expect(guard.granted(write, {})).toBeUndefined(); }); + + describe("cases", () => { + test("guest none, member deny if user.enabled is false", () => { + const guard = createGuard( + ["read"], + { + guest: { + is_default: true, + }, + member: { + permissions: [ + { + permission: "read", + policies: [ + { + condition: {}, + effect: "filter", + filter: { + type: "member", + }, + }, + { + condition: { + "user.enabled": false, + }, + effect: "deny", + }, + ], + }, + ], + }, + }, + { enabled: true }, + ); + + expect(() => guard.granted(read, { role: "guest" })).toThrow(); + + // member is allowed, because default role permission effect is allow + // and no deny policy is met + expect(guard.granted(read, { role: "member" })).toBeUndefined(); + + // member is allowed, because deny policy is not met + expect(guard.granted(read, { role: "member", enabled: true })).toBeUndefined(); + + // member is denied, because deny policy is met + expect(() => guard.granted(read, { role: "member", enabled: false })).toThrow(); + + // get the filter for member role + expect(guard.getPolicyFilter(read, { role: "member" })).toEqual({ + type: "member", + }); + + // get filter for guest + expect(guard.getPolicyFilter(read, {})).toBeUndefined(); + }); + + test("guest should only read posts that are public", () => { + const read = new Permission( + "read", + { + // make this permission filterable + // without this, `filter` policies have no effect + filterable: true, + }, + // expect the context to match this schema + // otherwise exit with 500 to ensure proper policy checking + s.object({ + entity: s.string(), + }), + ); + const guard = createGuard( + ["read"], + { + guest: { + // this permission is applied if no (or invalid) role is provided + is_default: true, + permissions: [ + { + permission: "read", + // effect deny means only having this permission, doesn't guarantee access + effect: "deny", + policies: [ + { + // only if this condition is met + condition: { + entity: { + $in: ["posts"], + }, + }, + // the effect is allow + effect: "allow", + }, + { + condition: { + entity: "posts", + }, + effect: "filter", + filter: { + public: true, + }, + }, + ], + }, + ], + }, + // members should be allowed to read all + member: { + permissions: [ + { + permission: "read", + }, + ], + }, + }, + { enabled: true }, + ); + + // guest can only read posts + expect(guard.granted(read, {}, { entity: "posts" })).toBeUndefined(); + expect(() => guard.granted(read, {}, { entity: "users" })).toThrow(); + + // and guests can only read public posts + expect(guard.getPolicyFilter(read, {}, { entity: "posts" })).toEqual({ + public: true, + }); + + // member can read posts and users + expect(guard.granted(read, { role: "member" }, { entity: "posts" })).toBeUndefined(); + expect(guard.granted(read, { role: "member" }, { entity: "users" })).toBeUndefined(); + + // member should not have a filter + expect( + guard.getPolicyFilter(read, { role: "member" }, { entity: "posts" }), + ).toBeUndefined(); + }); + }); }); diff --git a/app/__test__/auth/authorize/permissions.spec.ts b/app/__test__/auth/authorize/permissions.spec.ts index 2b3a5ce..f885d6e 100644 --- a/app/__test__/auth/authorize/permissions.spec.ts +++ b/app/__test__/auth/authorize/permissions.spec.ts @@ -3,7 +3,7 @@ import { s } from "bknd/utils"; import { Permission } from "auth/authorize/Permission"; import { Policy } from "auth/authorize/Policy"; import { Hono } from "hono"; -import { permission } from "auth/middlewares/permission.middleware"; +import { getPermissionRoutes, permission } from "auth/middlewares/permission.middleware"; import { auth } from "auth/middlewares/auth.middleware"; import { Guard, type GuardConfig } from "auth/authorize/Guard"; import { Role, RolePermission } from "auth/authorize/Role"; @@ -113,7 +113,8 @@ describe("Guard", () => { const r = new Role("test", [ new RolePermission(p, [ new Policy({ - filter: { a: { $eq: 1 } }, + condition: { a: { $eq: 1 } }, + filter: { foo: "bar" }, effect: "filter", }), ]), @@ -129,7 +130,7 @@ describe("Guard", () => { }, { a: 1 }, ), - ).toEqual({ a: { $eq: 1 } }); + ).toEqual({ foo: "bar" }); expect( guard.getPolicyFilter( p, @@ -158,7 +159,8 @@ describe("Guard", () => { [ new RolePermission(p, [ new Policy({ - filter: { a: { $eq: 1 } }, + condition: { a: { $eq: 1 } }, + filter: { foo: "bar" }, effect: "filter", }), ]), @@ -177,7 +179,7 @@ describe("Guard", () => { }, { a: 1 }, ), - ).toEqual({ a: { $eq: 1 } }); + ).toEqual({ foo: "bar" }); expect( guard.getPolicyFilter( p, @@ -189,7 +191,7 @@ describe("Guard", () => { ).toBeUndefined(); // if no user context given, the default role is applied // hence it can be found - expect(guard.getPolicyFilter(p, {}, { a: 1 })).toEqual({ a: { $eq: 1 } }); + expect(guard.getPolicyFilter(p, {}, { a: 1 })).toEqual({ foo: "bar" }); }); }); @@ -293,13 +295,19 @@ describe("permission middleware", () => { it("denies if user with role doesn't meet condition", async () => { const p = new Permission("test"); const r = new Role("test", [ - new RolePermission(p, [ - new Policy({ - condition: { - a: { $lt: 1 }, - }, - }), - ]), + new RolePermission( + p, + [ + new Policy({ + condition: { + a: { $lt: 1 }, + }, + // default effect is allow + }), + ], + // change default effect to deny if no condition is met + "deny", + ), ]); const hono = makeApp([p], [r], { context: { @@ -391,6 +399,88 @@ describe("permission middleware", () => { // expecting 500 because bknd should have handled it correctly expect(res.status).toBe(500); }); + + it("checks context on routes with permissions", async () => { + const make = (user: any) => { + const p = new Permission( + "test", + {}, + s.object({ + a: s.number(), + }), + ); + const r = new Role("test", [ + new RolePermission(p, [ + new Policy({ + condition: { + a: { $eq: 1 }, + }, + }), + ]), + ]); + return makeApp([p], [r]) + .use(async (c, next) => { + // @ts-expect-error + c.set("auth", { registered: true, user }); + await next(); + }) + .get( + "/valid", + permission(p, { + context: (c) => ({ + a: 1, + }), + }), + async (c) => c.text("test"), + ) + .get( + "/invalid", + permission(p, { + // @ts-expect-error + context: (c) => ({ + b: "1", + }), + }), + async (c) => c.text("test"), + ) + .get( + "/invalid2", + permission(p, { + // @ts-expect-error + context: (c) => ({}), + }), + async (c) => c.text("test"), + ) + .get( + "/invalid3", + // @ts-expect-error + permission(p), + async (c) => c.text("test"), + ); + }; + + const hono = make({ id: 0, role: "test" }); + const valid = await hono.request("/valid"); + expect(valid.status).toBe(200); + const invalid = await hono.request("/invalid"); + expect(invalid.status).toBe(500); + const invalid2 = await hono.request("/invalid2"); + expect(invalid2.status).toBe(500); + const invalid3 = await hono.request("/invalid3"); + expect(invalid3.status).toBe(500); + + { + const hono = make(null); + const valid = await hono.request("/valid"); + expect(valid.status).toBe(403); + const invalid = await hono.request("/invalid"); + expect(invalid.status).toBe(500); + const invalid2 = await hono.request("/invalid2"); + expect(invalid2.status).toBe(500); + const invalid3 = await hono.request("/invalid3"); + expect(invalid3.status).toBe(500); + } + }); }); describe("Role", () => { diff --git a/app/__test__/core/object/object-query.spec.ts b/app/__test__/core/object/object-query.spec.ts index dc03fb6..215adf8 100644 --- a/app/__test__/core/object/object-query.spec.ts +++ b/app/__test__/core/object/object-query.spec.ts @@ -66,4 +66,14 @@ describe("object-query", () => { expect(result).toBe(expected); } }); + + test("paths", () => { + const result = validate({ "user.age": { $lt: 18 } }, { user: { age: 17 } }); + expect(result).toBe(true); + }); + + test("empty filters", () => { + const result = validate({}, { user: { age: 17 } }); + expect(result).toBe(true); + }); }); diff --git a/app/src/auth/authorize/Guard.ts b/app/src/auth/authorize/Guard.ts index ac97c63..37ee842 100644 --- a/app/src/auth/authorize/Guard.ts +++ b/app/src/auth/authorize/Guard.ts @@ -160,7 +160,13 @@ export class Guard { if (!this.isEnabled()) { return; } - const { ctx, user, exists, role, rolePermission } = this.collect(permission, c, context); + const { ctx: _ctx, exists, role, rolePermission } = this.collect(permission, c, context); + + // validate context + let ctx = Object.assign({}, _ctx); + if (permission.context) { + ctx = permission.parseContext(ctx); + } $console.debug("guard: checking permission", { name: permission.name, @@ -187,32 +193,37 @@ export class Guard { throw new GuardPermissionsException( permission, undefined, - "Role does not have required permission", + `Role "${role.name}" does not have required permission`, ); } - // validate context - let ctx2 = Object.assign({}, ctx); - if (permission.context) { - ctx2 = permission.parseContext(ctx2); - } - if (rolePermission?.policies.length > 0) { $console.debug("guard: rolePermission has policies, checking"); + + // set the default effect of the role permission + let allowed = rolePermission.effect === "allow"; for (const policy of rolePermission.policies) { // skip filter policies if (policy.content.effect === "filter") continue; - // if condition unmet or effect is deny, throw - const meets = policy.meetsCondition(ctx2); - if (!meets || (meets && policy.content.effect === "deny")) { - throw new GuardPermissionsException( - permission, - policy, - "Policy does not meet condition", - ); + // if condition is met, check the effect + const meets = policy.meetsCondition(ctx); + if (meets) { + // if deny, then break early + if (policy.content.effect === "deny") { + allowed = false; + break; + + // if allow, set allow but continue checking + } else if (policy.content.effect === "allow") { + allowed = true; + } } } + + if (!allowed) { + throw new GuardPermissionsException(permission, undefined, "Policy condition unmet"); + } } $console.debug("guard allowing", { @@ -235,20 +246,24 @@ export class Guard { c: GuardContext, context?: PermissionContext

, ): PolicySchema["filter"] | undefined { - if (!permission.isFilterable()) return; + if (!permission.isFilterable()) { + $console.debug("getPolicyFilter: permission is not filterable, returning undefined"); + return; + } - const { ctx, exists, role, rolePermission } = this.collect(permission, c, context); + const { ctx: _ctx, exists, role, rolePermission } = this.collect(permission, c, context); // validate context - let ctx2 = Object.assign({}, ctx); + let ctx = Object.assign({}, _ctx); if (permission.context) { - ctx2 = permission.parseContext(ctx2); + ctx = permission.parseContext(ctx); } if (exists && role && rolePermission && rolePermission.policies.length > 0) { for (const policy of rolePermission.policies) { if (policy.content.effect === "filter") { - return policy.meetsFilter(ctx2) ? policy.content.filter : undefined; + const meets = policy.meetsCondition(ctx); + return meets ? policy.content.filter : undefined; } } } diff --git a/app/src/auth/authorize/Policy.ts b/app/src/auth/authorize/Policy.ts index fd873af..9995e20 100644 --- a/app/src/auth/authorize/Policy.ts +++ b/app/src/auth/authorize/Policy.ts @@ -5,6 +5,7 @@ export const policySchema = s .strictObject({ description: s.string(), condition: s.object({}).optional() as s.Schema<{}, query.ObjectQuery | undefined>, + // @todo: potentially remove this, and invert from rolePermission.effect effect: s.string({ enum: ["allow", "deny", "filter"], default: "allow" }), filter: s.object({}).optional() as s.Schema<{}, query.ObjectQuery | undefined>, }) @@ -25,10 +26,12 @@ export class Policy { } meetsCondition(context: object, vars?: Record) { + if (!this.content.condition) return true; return query.validate(this.replace(this.content.condition!, vars), context); } meetsFilter(subject: object, vars?: Record) { + if (!this.content.filter) return true; return query.validate(this.replace(this.content.filter!, vars), subject); } diff --git a/app/src/auth/authorize/Role.ts b/app/src/auth/authorize/Role.ts index 0cf038b..3f072a1 100644 --- a/app/src/auth/authorize/Role.ts +++ b/app/src/auth/authorize/Role.ts @@ -1,9 +1,13 @@ -import { parse, s } from "bknd/utils"; +import { s } from "bknd/utils"; import { Permission } from "./Permission"; import { Policy, policySchema } from "./Policy"; +// default effect is allow for backward compatibility +const defaultEffect = "allow"; + export const rolePermissionSchema = s.strictObject({ permission: s.string(), + effect: s.string({ enum: ["allow", "deny"], default: defaultEffect }).optional(), policies: s.array(policySchema).optional(), }); export type RolePermissionSchema = s.Static; @@ -20,12 +24,14 @@ export class RolePermission { constructor( public permission: Permission, public policies: Policy[] = [], + public effect: "allow" | "deny" = defaultEffect, ) {} toJSON() { return { permission: this.permission.name, policies: this.policies.map((p) => p.toJSON()), + effect: this.effect, }; } } @@ -45,7 +51,7 @@ export class Role { return new RolePermission(new Permission(p), []); } const policies = p.policies?.map((policy) => new Policy(policy)); - return new RolePermission(new Permission(p.permission), policies); + return new RolePermission(new Permission(p.permission), policies, p.effect); }) ?? []; return new Role(config.name, permissions, config.is_default, config.implicit_allow); } diff --git a/app/src/auth/middlewares/permission.middleware.ts b/app/src/auth/middlewares/permission.middleware.ts index ac38a08..c6e53f4 100644 --- a/app/src/auth/middlewares/permission.middleware.ts +++ b/app/src/auth/middlewares/permission.middleware.ts @@ -5,6 +5,7 @@ import type { RouterRoute } from "hono/types"; import { createMiddleware } from "hono/factory"; import type { ServerEnv } from "modules/Controller"; import type { MaybePromise } from "core/types"; +import { GuardPermissionsException } from "auth/authorize/Guard"; function getPath(reqOrCtx: Request | Context) { const req = reqOrCtx instanceof Request ? reqOrCtx : reqOrCtx.req.raw; @@ -54,7 +55,7 @@ export function permission

>( if (options?.onGranted || options?.onDenied) { let returned: undefined | void | Response; - if (threw(() => guard.granted(permission, c, context))) { + if (threw(() => guard.granted(permission, c, context), GuardPermissionsException)) { returned = await options?.onDenied?.(c); } else { returned = await options?.onGranted?.(c); diff --git a/app/src/core/object/query/query.ts b/app/src/core/object/query/query.ts index e90921d..24352c4 100644 --- a/app/src/core/object/query/query.ts +++ b/app/src/core/object/query/query.ts @@ -1,4 +1,5 @@ import type { PrimaryFieldType } from "core/config"; +import { getPath, invariant } from "bknd/utils"; export type Primitive = PrimaryFieldType | string | number | boolean; export function isPrimitive(value: any): value is Primitive { @@ -67,8 +68,9 @@ function _convert( expressions: Exps, path: string[] = [], ): FilterQuery { + invariant(typeof $query === "object", "$query must be an object"); const ExpressionConditionKeys = expressions.map((e) => e.key); - const keys = Object.keys($query); + const keys = Object.keys($query ?? {}); const operands = [OperandOr] as const; const newQuery: FilterQuery = {}; @@ -157,7 +159,7 @@ function _build( // check $and for (const [key, value] of Object.entries($and)) { for (const [$op, $v] of Object.entries(value)) { - const objValue = options.value_is_kv ? key : options.object[key]; + const objValue = options.value_is_kv ? key : getPath(options.object, key); result.$and.push(__validate($op, $v, objValue, [key])); result.keys.add(key); } @@ -165,7 +167,7 @@ function _build( // check $or for (const [key, value] of Object.entries($or ?? {})) { - const objValue = options.value_is_kv ? key : options.object[key]; + const objValue = options.value_is_kv ? key : getPath(options.object, key); for (const [$op, $v] of Object.entries(value)) { result.$or.push(__validate($op, $v, objValue, [key])); diff --git a/app/src/core/utils/runtime.ts b/app/src/core/utils/runtime.ts index 9b8e385..5b943ff 100644 --- a/app/src/core/utils/runtime.ts +++ b/app/src/core/utils/runtime.ts @@ -62,11 +62,18 @@ export function invariant(condition: boolean | any, message: string) { } } -export function threw(fn: () => any) { +export function threw(fn: () => any, instance?: new (...args: any[]) => Error) { try { fn(); return false; } catch (e) { + if (instance) { + if (e instanceof instance) { + return true; + } + // if instance given but not what expected, throw + throw e; + } return true; } }