From 3c7ae65f2a0ea5763f57263f85aa9db6c94986d5 Mon Sep 17 00:00:00 2001 From: jannyHou Date: Fri, 9 Aug 2019 11:59:14 -0400 Subject: [PATCH] refactor(authorization): improve pr --- packages/authorization/README.md | 27 +++++++++++ packages/authorization/package-lock.json | 6 +++ packages/authorization/package.json | 4 +- .../authorization-casbin.acceptance.ts | 14 ++++-- .../acceptance/authorization.acceptance.ts | 5 +- .../src/authorize-interceptor.ts | 47 +++++++++++++++---- packages/authorization/src/types.ts | 23 ++++++++- 7 files changed, 109 insertions(+), 17 deletions(-) diff --git a/packages/authorization/README.md b/packages/authorization/README.md index bf301837d011..a7f225f9cca2 100644 --- a/packages/authorization/README.md +++ b/packages/authorization/README.md @@ -70,6 +70,33 @@ export class MyController { } ``` +## Extract common layer(TBD) + +`@loopback/authentication` and `@loopback/authorization` shares the client +information from the request. Therefore we need another module with +types/interfaces that describe the client, like `principles`, `userProfile`, +etc... A draft PR is created for this module: see branch +https://github.com/strongloop/loopback-next/tree/security/packages/security + +Since the common module is still in progress, as the first release of +`@loopback/authorization`, we have two choices to inject a user in the +interceptor: + +- `@loopback/authorization` requires `@loopback/authentication` as a dependency. + The interceptor injects the current user using + `AuthenticationBindings.CURRENT_USER`. Then we remove this dependency in the + common layer PR, two auth modules will depend on `@loopback/security`. + + - This is what's been done in my refactor PR, `Principle` and `UserProfile` + are still decoupled, I added a convertor function to turn a user profile + into a principle. + +- The interceptor injects the user using another key not related to + `@loopback/authentication`.(_Which means the code that injects the user stays + as it is in https://github.com/strongloop/loopback-next/pull/1205_). Then we + unify the user set and injection in the common layer PR: same as the 1st + choice, two auth modules will depend on `@loopback/security`. + ## Related resources ## Contributions diff --git a/packages/authorization/package-lock.json b/packages/authorization/package-lock.json index 9d8fca2d1f1c..8bfc19f38887 100644 --- a/packages/authorization/package-lock.json +++ b/packages/authorization/package-lock.json @@ -858,6 +858,12 @@ "integrity": "sha512-D9MyoQFI7iP5VdpEyPZyjjqIJ8Y8EDNQFIFVLOmeg1rI1xiHOChyUPMPRUVfqFCerxfE+yS3vMyj37F6IdtOoQ==", "dev": true }, + "@types/node": { + "version": "10.14.15", + "resolved": "https://registry.npmjs.org/@types/node/-/node-10.14.15.tgz", + "integrity": "sha512-CBR5avlLcu0YCILJiDIXeU2pTw7UK/NIxfC63m7d7CVamho1qDEzXKkOtEauQRPMy6MI8mLozth+JJkas7HY6g==", + "dev": true + }, "ansi-styles": { "version": "3.2.1", "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-3.2.1.tgz", diff --git a/packages/authorization/package.json b/packages/authorization/package.json index 46acbf7aa539..0c10a994eefd 100644 --- a/packages/authorization/package.json +++ b/packages/authorization/package.json @@ -1,6 +1,6 @@ { "name": "@loopback/authorization", - "version": "1.0.0", + "version": "0.0.1", "description": "A LoopBack component for authorization support.", "engines": { "node": ">=8.9" @@ -27,7 +27,9 @@ "devDependencies": { "@loopback/build": "^2.0.3", "@loopback/testlab": "^1.6.3", + "@loopback/authentication": "^2.1.8", "@types/debug": "^4.1.4", + "@types/node": "10.14.15", "casbin": "^3.0.1" }, "keywords": [ diff --git a/packages/authorization/src/__tests__/acceptance/authorization-casbin.acceptance.ts b/packages/authorization/src/__tests__/acceptance/authorization-casbin.acceptance.ts index 8acc790d4ffb..766c0ccfedfb 100644 --- a/packages/authorization/src/__tests__/acceptance/authorization-casbin.acceptance.ts +++ b/packages/authorization/src/__tests__/acceptance/authorization-casbin.acceptance.ts @@ -18,6 +18,7 @@ import { Authorizer, } from '../..'; import {AuthorizationTags} from '../../keys'; +import {AuthenticationBindings, UserProfile} from '@loopback/authentication'; describe('Authorization', () => { let app: Application; @@ -26,7 +27,7 @@ describe('Authorization', () => { let events: string[]; before(givenApplicationAndAuthorizer); - beforeEach(givenRequestContext); + beforeEach(() => givenRequestContext()); it('allows placeOrder for everyone', async () => { const orderId = await invokeMethod(controller, 'placeOrder', reqCtx, [ @@ -42,6 +43,7 @@ describe('Authorization', () => { }); it('allows cancelOrder for customer_service', async () => { + givenRequestContext({id: 'customer_service', name: 'customer_service'}); const orderId = await controller.placeOrder({ customerId: 'bob', productId: 'product-a', @@ -56,7 +58,7 @@ describe('Authorization', () => { }); it('denies cancelOrder for bob', async () => { - givenRequestContext({name: 'bob'}); + givenRequestContext({id: 'bob', name: 'bob'}); const orderId = await controller.placeOrder({ customerId: 'bob', productId: 'product-a', @@ -111,10 +113,12 @@ describe('Authorization', () => { .tag(AuthorizationTags.AUTHORIZER); } - function givenRequestContext(user = {name: 'alice'}) { + function givenRequestContext( + user: UserProfile = {id: 'alice', name: 'alice'}, + ) { events = []; reqCtx = new Context(app); - reqCtx.bind('current.user').to(user); + reqCtx.bind(AuthenticationBindings.CURRENT_USER).to(user); controller = new OrderController(); } @@ -161,6 +165,6 @@ describe('Authorization', () => { __dirname, '../../../fixtures/casbin/rbac_policy.csv', ); - return await casbin.newEnforcer(conf, policy); + return casbin.newEnforcer(conf, policy); } }); diff --git a/packages/authorization/src/__tests__/acceptance/authorization.acceptance.ts b/packages/authorization/src/__tests__/acceptance/authorization.acceptance.ts index 144470748cf8..b77d9ea4788d 100644 --- a/packages/authorization/src/__tests__/acceptance/authorization.acceptance.ts +++ b/packages/authorization/src/__tests__/acceptance/authorization.acceptance.ts @@ -16,6 +16,7 @@ import { EVERYONE, } from '../..'; import {AuthorizationTags} from '../../keys'; +import {AuthenticationBindings} from '@loopback/authentication'; describe('Authorization', () => { let app: Application; @@ -86,7 +87,9 @@ describe('Authorization', () => { function givenRequestContext() { events = []; reqCtx = new Context(app); - reqCtx.bind('current.user').to({name: 'user-01'}); + reqCtx + .bind(AuthenticationBindings.CURRENT_USER) + .to({id: 'user-01', name: 'user-01'}); controller = new OrderController(); } diff --git a/packages/authorization/src/authorize-interceptor.ts b/packages/authorization/src/authorize-interceptor.ts index 18c8bee43dc7..850bea3a4569 100644 --- a/packages/authorization/src/authorize-interceptor.ts +++ b/packages/authorization/src/authorize-interceptor.ts @@ -18,7 +18,14 @@ import { import * as debugFactory from 'debug'; import {getAuthorizeMetadata} from './decorators/authorize'; import {AuthorizationTags} from './keys'; -import {AuthorizationContext, AuthorizationDecision, Authorizer} from './types'; +import { + AuthorizationContext, + AuthorizationDecision, + Authorizer, + AuthorizationError, + Principal, +} from './types'; +import {AuthenticationBindings, UserProfile} from '@loopback/authentication'; const debug = debugFactory('loopback:authorization:interceptor'); @@ -41,33 +48,46 @@ export class AuthorizationInterceptor implements Provider { ); if (!metadata) { debug('No authorization metadata is found %s', description); - return await next(); + const result = await next(); + return result; } debug('Authorization metadata for %s', description, metadata); - const user = await invocationCtx.get<{name: string}>('current.user', { - optional: true, - }); + + // retrieve it from authentication module + const user = await invocationCtx.get( + AuthenticationBindings.CURRENT_USER, + { + optional: true, + }, + ); + debug('Current user', user); + const authorizationCtx: AuthorizationContext = { - principals: user ? [{name: user.name, type: 'USER'}] : [], + principals: user ? [userToPrinciple(user)] : [], roles: [], scopes: [], resource: invocationCtx.targetName, invocationContext: invocationCtx, }; + debug('Security context for %s', description, authorizationCtx); let authorizers = await loadAuthorizers( invocationCtx, metadata.voters || [], ); + authorizers = authorizers.concat(this.authorizers); for (const fn of authorizers) { const decision = await fn(authorizationCtx, metadata); + // we can add another interceptor to process the error if (decision === AuthorizationDecision.DENY) { - throw new Error('Access denied'); + const error = new AuthorizationError('Access denied'); + error.statusCode = 401; + throw error; } } - return await next(); + return next(); } } @@ -88,3 +108,14 @@ async function loadAuthorizers( } return authorizerFunctions; } + +// This is a workaround before we extract a common layer +// for authentication and authorization. +function userToPrinciple(user: UserProfile): Principal { + return { + name: user.name || user.id, + id: user.id, + email: user.email, + type: 'USER', + }; +} diff --git a/packages/authorization/src/types.ts b/packages/authorization/src/types.ts index 009edc13f35b..fdb7aec1b497 100644 --- a/packages/authorization/src/types.ts +++ b/packages/authorization/src/types.ts @@ -183,14 +183,16 @@ export interface AuthorizationContext { /** * A function to decide if access to the target should be allowed or denied */ -export type Authorizer = +export type Authorizer< + T extends AuthorizationMetadata = AuthorizationMetadata +> = /** * @param context: Context information for authorization * @param metadata: Metadata representing requirements for authorization */ ( context: AuthorizationContext, - metadata: AuthorizationMetadata, + metadata: T, ) => Promise; /** @@ -233,3 +235,20 @@ export interface Enforcer { */ enforce(request: AuthorizationRequest): Promise; } + +/** + * The custom error class that describes the error thrown by + * the authorization module. + * Should be extracted to the common layer shared by authentication + * and authorization. + */ +export class AuthorizationError extends Error { + /** + * Machine readable code, can be understood by any clients + */ + code?: string; + /** + * The status code for HTTP requests + */ + statusCode?: number; +}