fix: Fix authorization evaluation issue on undefined value should be …

…skipped (#945)
plumier · Jun 1, 2021 · 7e9acd3 · 7e9acd3
1 parent 0e3ec31
commit 7e9acd3
Show file tree

Hide file tree

Showing 3 changed files with 105 additions and 5 deletions.
diff --git a/packages/core/src/authorization.ts b/packages/core/src/authorization.ts
@@ -121,7 +121,11 @@ class PolicyAuthorizer implements Authorizer {
             for (const policy of this.keys) {
                 if (authPolicy.equals(policy, ctx)) {
                     const authorize = await authPolicy.authorize(ctx)
-                    log.debug("%s by %s", authorize ? "AUTHORIZED" : "FORBIDDEN", authPolicy.friendlyName())
+                    log.debug("%s -> %s.%s by %s",
+                        authorize ? "AUTHORIZED" : "FORBIDDEN",
+                        ctx.metadata.current!.parent?.name ?? "",
+                        ctx.metadata.current!.name,
+                        authPolicy.friendlyName())
                     if (authorize) return true
                 }
             }
@@ -368,7 +372,7 @@ async function checkParameter(meta: PropertyReflection | ParameterReflection, va
     if (decorators.length > 0) {
         const info = createContext(ctx, value, meta)
         const allowed = await executeAuthorizer(decorators, info)
-        if(!allowed) return [ctx.path.join(".")]
+        if (!allowed) return [ctx.path.join(".")]
     }
     // if the property is a relation property just skip checking, since we allow set relation using ID
     const isRelation = meta.decorators.some((x: RelationDecorator) => x.kind === "plumier-meta:relation")
@@ -475,10 +479,9 @@ async function getAuthorize(authorizers: boolean | Authorizer, ctx: AuthorizerCo
 }
 
 async function filterType(raw: any, node: FilterNode, ctx: AuthorizerContext): Promise<any> {
-    if (raw === undefined || raw === null) return undefined
     if (node.kind === "Array") {
         const result = []
-        if(!Array.isArray(raw))
+        if (!Array.isArray(raw))
             throw new Error(`Action ${ctx.ctx.route.controller.name}.${ctx.ctx.route.action.name} expecting return value of type Array but got ${raw.constructor.name}`)
         for (const item of raw) {
             const val = await filterType(item, node.child, ctx)
@@ -491,6 +494,7 @@ async function filterType(raw: any, node: FilterNode, ctx: AuthorizerContext): P
         const result: any = {}
         for (const prop of node.properties) {
             const value = raw[prop.name]
+            if (value === null || value === undefined) continue
             const authorized = await getAuthorize(prop.authorizer, {
                 ...ctx, value,
                 parentValue: raw,

diff --git a/tests/behavior/authorization/__snapshots__/jwt-auth.spec.ts.snap b/tests/behavior/authorization/__snapshots__/jwt-auth.spec.ts.snap
@@ -486,10 +486,22 @@ Object {
 }
 `;
 
+exports[`JwtAuth Response Authorization Array Of Object Should not evaluate policy when provided empty array 1`] = `Array []`;
+
 exports[`JwtAuth Response Authorization Array Of Object Should throw error proper error message when provided non array 1`] = `
 Array [
   Array [
     [Error: Action UsersController.get expecting return value of type Array but got Number],
   ],
 ]
 `;
+
+exports[`JwtAuth Response Authorization Nested Object Should not evaluate policy if value not provided 1`] = `
+Array [
+  Array [
+    "HOLA!",
+  ],
+]
+`;
+
+exports[`JwtAuth Response Authorization Simple Object Should not evaluate policies if provided undefined value 1`] = `Array []`;
diff --git a/tests/behavior/authorization/jwt-auth.spec.ts b/tests/behavior/authorization/jwt-auth.spec.ts
@@ -2647,6 +2647,32 @@ describe("JwtAuth", () => {
                     .set("Authorization", `Bearer ${USER_TOKEN}`)
                     .expect(200, { name: "admin" })
             })
+            it("Should not evaluate policies if provided undefined value", async () => {
+                @domain()
+                class User {
+                    constructor(
+                        public name: string,
+                        @authorize.read("admin")
+                        public password?: string
+                    ) { }
+                }
+                class UsersController {
+                    @reflect.type(User)
+                    get() {
+                        return new User("admin")
+                    }
+                }
+                const fn = jest.fn()
+                const authPolicies = [authPolicy().define("admin", auth => fn("HOLA!"))]
+                const app = await fixture(UsersController)
+                    .set(new JwtAuthFacility({ secret: SECRET, authPolicies }))
+                    .initialize()
+                await Supertest(app.callback())
+                    .get("/users/get")
+                    .set("Authorization", `Bearer ${ADMIN_TOKEN}`)
+                    .expect(200, { name: "admin" })
+                expect(fn.mock.calls).toMatchSnapshot()
+            })
         })
 
         describe("Array Of Object", () => {
@@ -2844,6 +2870,32 @@ describe("JwtAuth", () => {
                     .expect(500)
                 expect(fn.mock.calls).toMatchSnapshot()
             })
+            it("Should not evaluate policy when provided empty array", async () => {
+                @domain()
+                class User {
+                    constructor(
+                        public name: string,
+                        @authorize.read("admin")
+                        public password: string
+                    ) { }
+                }
+                class UsersController {
+                    @reflect.type([User])
+                    get() {
+                        return []
+                    }
+                }
+                const fn = jest.fn()
+                const authPolicies = [authPolicy().define("admin", auth => fn("HOLA!"))]
+                const app = await fixture(UsersController)
+                    .set(new JwtAuthFacility({ secret: SECRET, authPolicies }))
+                    .initialize()
+                await Supertest(app.callback())
+                    .get("/users/get")
+                    .set("Authorization", `Bearer ${ADMIN_TOKEN}`)
+                    .expect(200)
+                expect(fn.mock.calls).toMatchSnapshot()
+            })
         })
 
         describe("Nested Object", () => {
@@ -3055,7 +3107,7 @@ describe("JwtAuth", () => {
                 @domain()
                 class Parent {
                     constructor(
-                        public name:string,
+                        public name: string,
                         @authorize.read("admin")
                         public user: User) { }
                 }
@@ -3077,6 +3129,38 @@ describe("JwtAuth", () => {
                     .set("Authorization", `Bearer ${USER_TOKEN}`)
                     .expect(200, { name: "Mimi" })
             })
+            it("Should not evaluate policy if value not provided", async () => {
+                @domain()
+                class User {
+                    constructor(
+                        public name: string,
+                        @authorize.read("admin")
+                        public password: string
+                    ) { }
+                }
+                @domain()
+                class Parent {
+                    constructor(
+                        public name: string,
+                        public user?: User) { }
+                }
+                class UsersController {
+                    @reflect.type(Parent)
+                    get() {
+                        return new Parent("Mimi", new User("admin", "secret"))
+                    }
+                }
+                const fn = jest.fn()
+                const authPolicies = [authPolicy().define("admin", auth => fn("HOLA!"))]
+                const app = await fixture(UsersController)
+                    .set(new JwtAuthFacility({ secret: SECRET, authPolicies }))
+                    .initialize()
+                await Supertest(app.callback())
+                    .get("/users/get")
+                    .set("Authorization", `Bearer ${ADMIN_TOKEN}`)
+                    .expect(200)
+                expect(fn.mock.calls).toMatchSnapshot()
+            })
         })
 
         describe("Custom Response Type", () => {