RFIT-188 code review comments fixed

vineet-suri · vineet-suri · commit 80bccd3f8982 · 2020-09-16T15:29:29.000+05:30
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -45,10 +45,10 @@
     "!*/__tests__"
   ],
   "peerDependencies": {
-    "@loopback/boot": "^1.4.4",
-    "@loopback/context": "^1.20.2",
-    "@loopback/core": "^1.8.5",
-    "@loopback/rest": "^1.16.3"
+    "@loopback/boot": "^1.7.4",
+    "@loopback/context": "^3.11.2",
+    "@loopback/core": "^2.10.0",
+    "@loopback/rest": "^1.26.1"
   },
   "dependencies": {
     "@sourceloop/core": "^1.0.0-alpha.9",
@@ -57,12 +57,12 @@
     "lodash": "^4.17.15"
   },
   "devDependencies": {
-    "@loopback/boot": "^1.4.4",
-    "@loopback/build": "^2.0.3",
-    "@loopback/context": "^3.8.2",
-    "@loopback/core": "^2.9.5",
-    "@loopback/rest": "^1.16.3",
-    "@loopback/testlab": "^1.6.3",
+    "@loopback/boot": "^1.7.4",
+    "@loopback/build": "^2.1.0",
+    "@loopback/context": "^3.11.0",
+    "@loopback/core": "^2.10.0",
+    "@loopback/rest": "^1.26.1",
+    "@loopback/testlab": "^1.10.3",
     "@loopback/tslint-config": "^2.1.0",
     "@types/lodash": "^4.14.135",
     "@types/node": "^10.14.8",
diff --git a/src/component.ts b/src/component.ts
@@ -1,17 +1,21 @@
-import { bind, Binding, Component, ContextTags, CoreBindings, inject, ProviderMap } from '@loopback/core';
-import { RestApplication } from '@loopback/rest';
-import { AuthorizationBindings } from './keys';
-import { AuthorizeActionProvider } from './providers/authorization-action.provider';
-import { AuthorizationMetadataProvider } from './providers/authorization-metadata.provider';
-import { CasbinAuthorizationProvider } from './providers/casbin-authorization-action.provider';
-import { UserPermissionsProvider } from './providers/user-permissions.provider';
-import { AuthorizationConfig } from './types';
+import {
+  Binding,
+  Component,
+  CoreBindings,
+  inject,
+  ProviderMap,
+} from '@loopback/core';
+import {RestApplication} from '@loopback/rest';
+import {AuthorizationBindings} from './keys';
+import {AuthorizeActionProvider} from './providers/authorization-action.provider';
+import {AuthorizationMetadataProvider} from './providers/authorization-metadata.provider';
+import {CasbinAuthorizationProvider} from './providers/casbin-authorization-action.provider';
+import {UserPermissionsProvider} from './providers/user-permissions.provider';
+import {AuthorizationConfig} from './types';
 
-
-@bind({ tags: { [ContextTags.KEY]: AuthorizationBindings.COMPONENT.key } })
 export class AuthorizationComponent implements Component {
   providers?: ProviderMap;
-  bindings1?: Binding[];
+  bindings?: Binding[];
 
   constructor(
     @inject(CoreBindings.APPLICATION_INSTANCE)
@@ -21,24 +25,24 @@ export class AuthorizationComponent implements Component {
   ) {
     this.providers = {
       [AuthorizationBindings.AUTHORIZE_ACTION.key]: AuthorizeActionProvider,
-      [AuthorizationBindings.CASBIN_AUTHORIZE_ACTION.key]: CasbinAuthorizationProvider,
+      [AuthorizationBindings.CASBIN_AUTHORIZE_ACTION
+        .key]: CasbinAuthorizationProvider,
       [AuthorizationBindings.METADATA.key]: AuthorizationMetadataProvider,
       [AuthorizationBindings.USER_PERMISSIONS.key]: UserPermissionsProvider,
     };
 
-
     if (
       config &&
       config.allowAlwaysPaths &&
       config.allowAlwaysPaths.length > 0
     ) {
-      this.bindings1 = [
+      this.bindings = [
         Binding.bind(AuthorizationBindings.PATHS_TO_ALLOW_ALWAYS).to(
           config.allowAlwaysPaths,
         ),
       ];
     } else {
-      this.bindings1 = [
+      this.bindings = [
         Binding.bind(AuthorizationBindings.PATHS_TO_ALLOW_ALWAYS).to([
           '/explorer',
         ]),
diff --git a/src/decorators/authorize.decorator.ts b/src/decorators/authorize.decorator.ts
@@ -1,14 +1,14 @@
-import { MethodDecoratorFactory } from '@loopback/core';
-import { AuthorizationMetadata } from '../types';
-import { AUTHORIZATION_METADATA_ACCESSOR } from '../keys';
+import {MethodDecoratorFactory} from '@loopback/core';
+import {AuthorizationMetadata} from '../types';
+import {AUTHORIZATION_METADATA_ACCESSOR} from '../keys';
 
 export function authorize(metadata: AuthorizationMetadata) {
   return MethodDecoratorFactory.createDecorator<AuthorizationMetadata>(
     AUTHORIZATION_METADATA_ACCESSOR,
     {
       permissions: metadata.permissions || [],
       resource: metadata.resource || '',
-      isCasbinPolicy: metadata.isCasbinPolicy || false
+      isCasbinPolicy: metadata.isCasbinPolicy || false,
     },
   );
 }
diff --git a/src/keys.ts b/src/keys.ts
@@ -1,33 +1,25 @@
-import { BindingKey } from '@loopback/context';
-import { MetadataAccessor } from '@loopback/metadata';
+import {BindingKey} from '@loopback/context';
+import {MetadataAccessor} from '@loopback/metadata';
 import {
   AuthorizeFn,
   AuthorizationMetadata,
   UserPermissionsFn,
   AuthorizationConfig,
-  CasbinEnforcerFn,
   CasbinAuthorizeFn,
   CasbinEnforcerConfigGetterFn,
   CasbinResourceModifierFn,
 } from './types';
-import { CoreBindings } from '@loopback/core';
-import { AuthorizationComponent } from './component';
 
 /**
  * Binding keys used by this component.
  */
 export namespace AuthorizationBindings {
-
-  export const COMPONENT = BindingKey.create<AuthorizationComponent>(
-    `${CoreBindings.COMPONENTS}.AuthorizationComponent`,
-  );
-
   export const AUTHORIZE_ACTION = BindingKey.create<AuthorizeFn>(
     'sf.userAuthorization.actions.authorize',
   );
 
   export const CASBIN_AUTHORIZE_ACTION = BindingKey.create<CasbinAuthorizeFn>(
-    'sf.userCasbinAuthorization.actions.authorize',
+    'sf.userAuthorization.actions.casbin.authorize',
   );
 
   export const METADATA = BindingKey.create<AuthorizationMetadata | undefined>(
@@ -38,31 +30,22 @@ export namespace AuthorizationBindings {
     'sf.userAuthorization.actions.userPermissions',
   );
 
-  export const CASBIN_ENFORCER = BindingKey.create<CasbinEnforcerFn<string>>(
-    'sf.userCasbinAuthorization.casbinenforcer',
-  );
+  export const CASBIN_ENFORCER_CONFIG_GETTER = BindingKey.create<
+    CasbinEnforcerConfigGetterFn
+  >('sf.userAuthorization.actions.casbin.config');
 
-  export const CASBIN_ENFORCER_CONFIG_GETTER = BindingKey.create<CasbinEnforcerConfigGetterFn>(
-    'sf.userCasbinAuthorization.casbinEnforcerConfigGetter',
-  );
-
-  export const CASBIN_RESOURCE_MODIFIER_FN = BindingKey.create<CasbinResourceModifierFn>(
-    'sf.userCasbinAuthorization.casbinResourceModifierFn',
-  );
+  export const CASBIN_RESOURCE_MODIFIER_FN = BindingKey.create<
+    CasbinResourceModifierFn
+  >('sf.userAuthorization.actions.casbin.resourceModifier');
 
   export const CONFIG = BindingKey.create<AuthorizationConfig>(
     'sf.userAuthorization.config',
   );
 
   export const PATHS_TO_ALLOW_ALWAYS = 'sf.userAuthorization.allowAlways';
-
-
-  export const RESOURCE_ID = BindingKey.create<string>('sf.resourceId');
 }
 
 export const AUTHORIZATION_METADATA_ACCESSOR = MetadataAccessor.create<
   AuthorizationMetadata,
   MethodDecorator
 >('sf.userAuthorization.accessor.operationMetadata');
-
-
diff --git a/src/providers/casbin-authorization-action.provider.ts b/src/providers/casbin-authorization-action.provider.ts
@@ -1,63 +1,86 @@
-import { Getter, inject, Provider } from '@loopback/core';
-import { IAuthUserWithPermissions } from '@sourceloop/core';
+import {Getter, inject, Provider} from '@loopback/core';
+import {IAuthUserWithPermissions} from '@sourceloop/core';
 import * as casbin from 'casbin';
 import * as fs from 'fs';
 import * as path from 'path';
-import { AuthorizationBindings } from '../keys';
-import { AuthorizationMetadata, CasbinAuthorizeFn, CasbinEnforcerConfigGetterFn } from '../types';
+import {AuthorizationBindings} from '../keys';
+import {
+  AuthorizationMetadata,
+  CasbinAuthorizeFn,
+  CasbinEnforcerConfigGetterFn,
+} from '../types';
 const fsPromises = fs.promises;
 
 const DEFAULT_SCOPE = 'execute';
 
-export class CasbinAuthorizationProvider implements Provider<CasbinAuthorizeFn> {
+export class CasbinAuthorizationProvider
+  implements Provider<CasbinAuthorizeFn> {
   constructor(
     @inject.getter(AuthorizationBindings.METADATA)
     private readonly getCasbinMetadata: Getter<AuthorizationMetadata>,
     @inject.getter(AuthorizationBindings.CASBIN_ENFORCER_CONFIG_GETTER)
-    private readonly getCasbinEnforcerConfig: Getter<CasbinEnforcerConfigGetterFn>,
-  ) { }
+    private readonly getCasbinEnforcerConfig: Getter<
+      CasbinEnforcerConfigGetterFn
+    >,
+  ) {}
 
   value(): CasbinAuthorizeFn {
     return (response, resource) => this.action(response, resource);
   }
 
-  async action(user: IAuthUserWithPermissions, resource: string): Promise<boolean> {
+  async action(
+    user: IAuthUserWithPermissions,
+    resource: string,
+  ): Promise<boolean> {
     let authDecision = false;
     try {
+      // fetch decorator metadata
       const metadata: AuthorizationMetadata = await this.getCasbinMetadata();
 
+      if (!metadata.resource) {
+        return false;
+      }
+
       const subject = this.getUserName(`${user.id}`);
 
       const object = resource;
 
-      const action = metadata.permissions && metadata.permissions.length > 0 ? metadata.permissions[0] : DEFAULT_SCOPE;
+      const action =
+        metadata.permissions && metadata.permissions.length > 0
+          ? metadata.permissions[0]
+          : DEFAULT_SCOPE;
 
+      // Fetch casbin config by invoking casbin-config-getter-provider
       const fn = await this.getCasbinEnforcerConfig();
 
-      const result = await fn(user, metadata.resource);
+      const casbinConfig = await fn(user, metadata.resource);
 
       let enforcer: casbin.Enforcer;
 
+      // If casbin config policy format is being used, create enforcer
       if (metadata.isCasbinPolicy) {
-        enforcer = await casbin.newEnforcer(result.model, result.policy);
-      } else if (!metadata.isCasbinPolicy && result.allowedRes) {
-        const policy = this.createCasbinPolicy(result.allowedRes, subject, action);
+        enforcer = await casbin.newEnforcer(
+          casbinConfig.model,
+          casbinConfig.policy,
+        );
+      }
+      // In case casbin policy is coming via provider, use that to initialise enforcer
+      else if (!metadata.isCasbinPolicy && casbinConfig.allowedRes) {
+        const policy = this.createCasbinPolicy(
+          casbinConfig.allowedRes,
+          subject,
+          action,
+        );
         const baseDir = path.join(__dirname, '../../src/policy.csv');
         await fsPromises.writeFile(baseDir, policy);
 
-        enforcer = await casbin.newEnforcer(result.model, baseDir);
+        enforcer = await casbin.newEnforcer(casbinConfig.model, baseDir);
       } else {
         return false;
       }
 
-      authDecision = await enforcer.enforce(
-        subject,
-        object,
-        action,
-      );
-    }
-
-    catch (err) {
+      authDecision = await enforcer.enforce(subject, object, action);
+    } catch (err) {
       console.log(err);
     }
 
@@ -71,15 +94,19 @@ export class CasbinAuthorizationProvider implements Provider<CasbinAuthorizeFn>
     return `u${id}`;
   }
 
-  createCasbinPolicy(allowedRes: string[], subject: string, action: string): string {
+  createCasbinPolicy(
+    allowedRes: string[],
+    subject: string,
+    action: string,
+  ): string {
     //Expected format for allowedRes: ['ping', 'ping2', 'ping3'];
 
     let result = '';
     allowedRes.forEach(res => {
       const policy = `p, ${subject}, ${res}, ${action}
       `;
       result += policy;
-    })
+    });
 
     return result;
   }
diff --git a/src/providers/casbin-enforcer-config.provider.ts b/src/providers/casbin-enforcer-config.provider.ts
@@ -1,15 +1,19 @@
-import { Provider } from '@loopback/context';
+import {Provider} from '@loopback/context';
 
-import { CasbinEnforcerConfigGetterFn } from '../types';
-import { IAuthUserWithPermissions } from '@sourceloop/core';
-import { HttpErrors } from '@loopback/rest';
+import {CasbinEnforcerConfigGetterFn} from '../types';
+import {IAuthUserWithPermissions} from '@sourceloop/core';
+import {HttpErrors} from '@loopback/rest';
 
 export class CasbinEnforcerProvider
   implements Provider<CasbinEnforcerConfigGetterFn> {
-  constructor() { }
+  constructor() {}
 
   value(): CasbinEnforcerConfigGetterFn {
-    return async (authUser: IAuthUserWithPermissions, resource: string, isCasbinPolicy?: boolean) => {
+    return async (
+      authUser: IAuthUserWithPermissions,
+      resource: string,
+      isCasbinPolicy?: boolean,
+    ) => {
       throw new HttpErrors.NotImplemented(
         `CasbinEnforcerConfigGetterFn Provider is not implemented`,
       );
diff --git a/src/types.ts b/src/types.ts
