refactor(authorization): improve pr

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

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": [

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);
   }
 });

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();
   }
 

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<Interceptor> {
     );
     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<UserProfile>(
+      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',
+  };
+}

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<AuthorizationDecision>;
 
 /**
@@ -233,3 +235,20 @@ export interface Enforcer {
    */
   enforce(request: AuthorizationRequest): Promise<AuthorizationDecision>;
 }
+
+/**
+ * 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;
+}