fix(rbac): fix more sonar cloud issue for rbac-backend

.changeset/friendly-swans-return.md

@@ -0,0 +1,5 @@
+---
+"@janus-idp/backstage-plugin-rbac-backend": patch
+---
+
+Fix some sonar cloud issues.

plugins/rbac-backend/src/conditional-aliases/alias-resolver.ts

@@ -12,9 +12,7 @@ import {
   ConditionalAliases,
 } from '@janus-idp/backstage-plugin-rbac-common';
 
-interface Predicate<T> {
-  (item: T): boolean;
-}
+type Predicate<T> = (item: T) => boolean;
 
 function isOwnerRefsAlias(value: PermissionRuleParam): boolean {
   const alias = `${CONDITION_ALIAS_SIGN}${ConditionalAliases.OWNER_REFS}`;

plugins/rbac-backend/src/database/casbin-adapter-factory.ts

@@ -22,32 +22,32 @@ export class CasbinDBAdapterFactory {
     const client = databaseConfig?.getOptionalString('client');
 
     let adapter;
-    if (client === 'pg') {
+    if (databaseConfig && client === 'pg') {
       const dbName =
         await this.databaseClient.client.config.connection.database;
       const schema =
         (await this.databaseClient.client.searchPath?.[0]) ?? 'public';
 
-      const ssl = this.handlePostgresSSL(databaseConfig!);
+      const ssl = this.handlePostgresSSL(databaseConfig);
 
       adapter = await TypeORMAdapter.newAdapter({
         type: 'postgres',
-        host: databaseConfig?.getString('connection.host'),
-        port: databaseConfig?.getNumber('connection.port'),
-        username: databaseConfig?.getString('connection.user'),
-        password: databaseConfig?.getString('connection.password'),
+        host: databaseConfig.getString('connection.host'),
+        port: databaseConfig.getNumber('connection.port'),
+        username: databaseConfig.getString('connection.user'),
+        password: databaseConfig.getString('connection.password'),
         ssl,
         database: dbName,
         schema: schema,
       });
     }
 
-    if (client === 'better-sqlite3') {
+    if (databaseConfig && client === 'better-sqlite3') {
       let storage;
-      if (typeof databaseConfig?.get('connection')?.valueOf() === 'string') {
-        storage = databaseConfig?.getString('connection');
-      } else if (databaseConfig?.has('connection.directory')) {
-        const storageDir = databaseConfig?.getString('connection.directory');
+      if (typeof databaseConfig.get('connection')?.valueOf() === 'string') {
+        storage = databaseConfig.getString('connection');
+      } else if (databaseConfig.has('connection.directory')) {
+        const storageDir = databaseConfig.getString('connection.directory');
         storage = resolve(storageDir, DEFAULT_SQLITE3_STORAGE_FILE_NAME);
       }
 

plugins/rbac-backend/src/database/role-metadata.ts

@@ -143,7 +143,7 @@ export class DataBaseRoleMetadataStorage implements RoleMetadataStorage {
 
     await trx<RoleMetadataDao>(ROLE_METADATA_TABLE)
       .delete()
-      .whereIn('id', [metadataDao.id!]);
+      .whereIn('id', [metadataDao.id]);
   }
 }
 

plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts

@@ -224,7 +224,7 @@ describe('CSVFileWatcher', () => {
       await enforcerDelegate.addPolicy(legacyPermission);
       await enforcerDelegate.addGroupingPolicies(
         [['user:default/guest', 'role:default/legacy']],
-        legacyRoleMetadata!,
+        legacyRoleMetadata,
       );
       roleMetadataStorageMock.filterRoleMetadata = jest
         .fn()

plugins/rbac-backend/src/file-permissions/csv-file-watcher.ts

@@ -46,7 +46,8 @@ type CSVFilePolicies = {
 
 export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
   private currentContent: string[][];
-  private csvFilePolicies: CSVFilePolicies;
+
+  private readonly csvFilePolicies: CSVFilePolicies;
 
   constructor(
     filePath: string | undefined,
@@ -94,6 +95,7 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
     if (!this.filePath) {
       return;
     }
+
     let content: string[][] = [];
     // If the file is set load the file contents
     content = this.parse();
@@ -103,33 +105,59 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
       new FileAdapter(this.filePath),
     );
 
-    // Check for any old policies that will need to be removed by checking if
-    // the policy no longer exists in the temp enforcer (csv file)
+    await this.processOldPolicies(tempEnforcer);
+    await this.processNewPolicies(tempEnforcer);
+
+    await this.migrateLegacyMetadata(tempEnforcer);
+
+    // We pass current here because this is during initialization and it has not changed yet
+    await this.updatePolicies(content, tempEnforcer);
+
+    if (this.allowReload) {
+      this.watchFile();
+    }
+  }
+
+  private async processOldPolicies(tempEnforcer: Enforcer): Promise<void> {
     const roleMetadatas =
       await this.roleMetadataStorage.filterRoleMetadata('csv-file');
     const fileRoles = roleMetadatas.map(meta => meta.roleEntityRef);
 
     if (fileRoles.length > 0) {
-      const groupingPoliciesToRemove =
-        await this.enforcer.getFilteredGroupingPolicy(1, ...fileRoles);
-      for (const gPolicy of groupingPoliciesToRemove) {
-        if (!(await tempEnforcer.hasGroupingPolicy(...gPolicy))) {
-          this.csvFilePolicies.removedGroupPolicies.push(gPolicy);
-        }
+      await this.checkPoliciesToRemove(fileRoles, tempEnforcer);
+      await this.checkGroupingPoliciesToRemove(fileRoles, tempEnforcer);
+    }
+  }
+
+  private async checkPoliciesToRemove(
+    fileRoles: string[],
+    tempEnforcer: Enforcer,
+  ): Promise<void> {
+    const policiesToRemove = await this.enforcer.getFilteredPolicy(
+      0,
+      ...fileRoles,
+    );
+    for (const policy of policiesToRemove) {
+      if (!(await tempEnforcer.hasPolicy(...policy))) {
+        this.csvFilePolicies.removedPolicies.push(policy);
       }
-      const policiesToRemove = await this.enforcer.getFilteredPolicy(
-        0,
-        ...fileRoles,
-      );
-      for (const policy of policiesToRemove) {
-        if (!(await tempEnforcer.hasPolicy(...policy))) {
-          this.csvFilePolicies.removedPolicies.push(policy);
-        }
+    }
+  }
+
+  private async checkGroupingPoliciesToRemove(
+    fileRoles: string[],
+    tempEnforcer: Enforcer,
+  ): Promise<void> {
+    const groupingPoliciesToRemove =
+      await this.enforcer.getFilteredGroupingPolicy(1, ...fileRoles);
+    for (const gPolicy of groupingPoliciesToRemove) {
+      if (!(await tempEnforcer.hasGroupingPolicy(...gPolicy))) {
+        this.csvFilePolicies.removedGroupPolicies.push(gPolicy);
       }
     }
+  }
 
-    // Check for any new policies that need to be added by checking if
-    // the policy does not currently exist in the enforcer
+  private async processNewPolicies(tempEnforcer: Enforcer): Promise<void> {
     const policiesToAdd = await tempEnforcer.getPolicy();
     const groupPoliciesToAdd = await tempEnforcer.getGroupingPolicy();
 
@@ -144,15 +172,6 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
         this.csvFilePolicies.addedGroupPolicies.push(groupPolicy);
       }
     }
-
-    await this.migrateLegacyMetadata(tempEnforcer);
-
-    // We pass current here because this is during initialization and it has not changed yet
-    await this.updatePolicies(content, tempEnforcer);
-
-    if (this.allowReload) {
-      this.watchFile();
-    }
   }
 
   // Check for policies that might need to be updated
@@ -201,11 +220,15 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
    * It will finally call updatePolicies with the new content.
    */
   async onChange(): Promise<void> {
+    if (!this.filePath) {
+      throw new Error('File path is not specified');
+    }
+
     const newContent = this.parse();
 
     const tempEnforcer = await newEnforcer(
       newModelFromString(MODEL),
-      new FileAdapter(this.filePath!),
+      new FileAdapter(this.filePath),
     );
 
     const currentFlatContent = this.currentContent.flatMap(data => {
@@ -280,6 +303,10 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
    * @param tempEnforcer Temporary enforcer for checking for duplicates when adding policies
    */
   private async addPermissionPolicies(tempEnforcer: Enforcer): Promise<void> {
+    if (!this.filePath) {
+      throw new Error('File path is not specified');
+    }
+
     for (const policy of this.csvFilePolicies.addedPolicies) {
       const transformedPolicy = transformArrayToPolicy(policy);
       const metadata = await this.roleMetadataStorage.findRoleMetadata(
@@ -305,7 +332,7 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
       err = await checkForDuplicatePolicies(
         tempEnforcer,
         policy,
-        this.filePath!,
+        this.filePath,
       );
       if (err) {
         this.logger.warn(err.message);
@@ -367,6 +394,10 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
    * @param tempEnforcer Temporary enforcer for checking for duplicates when adding policies
    */
   private async addRoles(tempEnforcer: Enforcer): Promise<void> {
+    if (!this.filePath) {
+      throw new Error('File path is not specified');
+    }
+
     for (const groupPolicy of this.csvFilePolicies.addedGroupPolicies) {
       let err = await validateGroupingPolicy(
         groupPolicy,
@@ -383,7 +414,7 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
       err = await checkForDuplicateGroupPolicies(
         tempEnforcer,
         groupPolicy,
-        this.filePath!,
+        this.filePath,
       );
       if (err) {
         this.logger.warn(err.message);

plugins/rbac-backend/src/file-permissions/yaml-conditional-file-watcher.ts

@@ -212,7 +212,7 @@ export class YamlConditinalPoliciesFileWatcher extends AbstractFileWatcher<
             condition.permissionMapping,
           )
         )[0];
-        await this.conditionalStorage.deleteCondition(conditionToDelete.id!);
+        await this.conditionalStorage.deleteCondition(conditionToDelete.id);
 
         await this.auditLogger.auditLog<ConditionAuditInfo>({
           message: `Deleted conditional permission policy`,

plugins/rbac-backend/src/policies/permission-policy.ts

@@ -58,8 +58,6 @@ const evaluatePermMsg = (
   } and action '${toPermissionAction(permission.attributes)}'`;
 
 export class RBACPermissionPolicy implements PermissionPolicy {
-  private readonly superUserList?: string[];
-
   public static async build(
     logger: LoggerService,
     auditLogger: AuditLogger,
@@ -162,10 +160,8 @@ export class RBACPermissionPolicy implements PermissionPolicy {
     private readonly enforcer: EnforcerDelegate,
     private readonly auditLogger: AuditLogger,
     private readonly conditionStorage: ConditionalStorage,
-    superUserList?: string[],
-  ) {
-    this.superUserList = superUserList;
-  }
+    private readonly superUsers: string[],
+  ) {}
 
   async handle(
     request: PolicyQuery,
@@ -278,13 +274,13 @@ export class RBACPermissionPolicy implements PermissionPolicy {
     return false;
   }
 
-  private isAuthorized = async (
+  private readonly isAuthorized = async (
     userIdentity: string,
     permission: string,
     action: string,
     roles: string[],
   ): Promise<boolean> => {
-    if (this.superUserList!.includes(userIdentity)) {
+    if (this.superUsers.includes(userIdentity)) {
       return true;
     }
 

plugins/rbac-backend/src/role-manager/ancestor-search-memo.ts

@@ -17,14 +17,14 @@ export type ASMGroup = Relation | Entity;
 // Also AncestorSearchMemo supports detection cycle dependencies between groups in the graph.
 //
 export class AncestorSearchMemo {
-  private graph: Graph;
+  private readonly graph: Graph;
 
-  private catalogApi: CatalogApi;
-  private catalogDBClient: Knex;
-  private auth: AuthService;
+  private readonly catalogApi: CatalogApi;
+  private readonly catalogDBClient: Knex;
+  private readonly auth: AuthService;
 
-  private userEntityRef: string;
-  private maxDepth?: number;
+  private readonly userEntityRef: string;
+  private readonly maxDepth?: number;
 
   constructor(
     userEntityRef: string,

plugins/rbac-backend/src/role-manager/role-manager.test.ts

@@ -73,9 +73,12 @@ describe('BackstageRoleManager', () => {
 
   describe('unimplemented methods', () => {
     it('should throw an error for syncedHasLink', () => {
-      expect(() =>
-        roleManager.syncedHasLink!('user:default/role1', 'user:default/role2'),
-      ).toThrow('Method "syncedHasLink" not implemented.');
+      expect(() => {
+        if (!roleManager.syncedHasLink) {
+          throw new Error('Method "syncedHasLink" is undefined.');
+        }
+        roleManager.syncedHasLink('user:default/role1', 'user:default/role2');
+      }).toThrow('Method "syncedHasLink" not implemented.');
     });
 
     it('should throw an error for getUsers', async () => {

plugins/rbac-backend/src/role-manager/role-manager.ts

@@ -10,8 +10,8 @@ import { AncestorSearchMemo } from './ancestor-search-memo';
 import { RoleMemberList } from './member-list';
 
 export class BackstageRoleManager implements RoleManager {
-  private allRoles: Map<string, RoleMemberList>;
-  private maxDepth?: number;
+  private readonly allRoles: Map<string, RoleMemberList>;
+  private readonly maxDepth?: number;
   constructor(
     private readonly catalogApi: CatalogApi,
     private readonly logger: LoggerService,
@@ -23,7 +23,7 @@ export class BackstageRoleManager implements RoleManager {
     this.allRoles = new Map<string, RoleMemberList>();
     const rbacConfig = this.config.getOptionalConfig('permission.rbac');
     this.maxDepth = rbacConfig?.getOptionalNumber('maxDepth');
-    if (this.maxDepth !== undefined && this.maxDepth! < 0) {
+    if (this.maxDepth !== undefined && this.maxDepth < 0) {
       throw new Error(
         'Max Depth for RBAC group hierarchy must be greater than or equal to zero',
       );
@@ -325,7 +325,7 @@ export class BackstageRoleManager implements RoleManager {
       this.allRoles.delete(name2);
     }
 
-    if (currentRole && currentRole.hasMember(name1)) {
+    if (currentRole?.hasMember(name1)) {
       return true;
     }