fix(rbac): improve error handling in retrieving permission metadata. (#…

…1285) Signed-off-by: Oleksandr Andriienko <[email protected]>
janus-idp · Feb 28, 2024 · 77f5f0e · 77f5f0e
1 parent 1bb20a9
commit 77f5f0e
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 15 deletions.
diff --git a/plugins/rbac-backend/src/service/plugin-endpoint.test.ts b/plugins/rbac-backend/src/service/plugin-endpoint.test.ts
@@ -60,8 +60,8 @@ describe('plugin-endpoint', () => {
       const collector = new PluginPermissionMetadataCollector(
         mockPluginEndpointDiscovery,
         backendPluginIDsProviderMock,
-        config,
         logger,
+        config,
       );
       const policiesMetadata = await collector.getPluginPolicies();
 
@@ -79,8 +79,8 @@ describe('plugin-endpoint', () => {
       const collector = new PluginPermissionMetadataCollector(
         mockPluginEndpointDiscovery,
         backendPluginIDsProviderMock,
-        config,
         logger,
+        config,
       );
       const policiesMetadata = await collector.getPluginPolicies();
 
@@ -109,8 +109,8 @@ describe('plugin-endpoint', () => {
       const collector = new PluginPermissionMetadataCollector(
         mockPluginEndpointDiscovery,
         backendPluginIDsProviderMock,
-        config,
         logger,
+        config,
       );
       const policiesMetadata = await collector.getPluginPolicies();
 
@@ -148,8 +148,8 @@ describe('plugin-endpoint', () => {
       const collector = new PluginPermissionMetadataCollector(
         mockPluginEndpointDiscovery,
         backendPluginIDsProviderMock,
-        config,
         logger,
+        config,
       );
       const policiesMetadata = await collector.getPluginPolicies();
 
@@ -163,7 +163,7 @@ describe('plugin-endpoint', () => {
       ]);
     });
 
-    it('should throw error when it is not possible to retrieve permission metadata for known endpoint', async () => {
+    it('should log error when it is not possible to retrieve permission metadata for known endpoint', async () => {
       backendPluginIDsProviderMock.getPluginIds.mockReturnValue([
         'permission',
         'catalog',
@@ -184,16 +184,69 @@ describe('plugin-endpoint', () => {
         '{"permissions":[{"type":"resource","name":"policy.entity.read","attributes":{"action":"read"}}]}',
       );
 
+      const errorSpy = jest.spyOn(logger, 'error').mockClear();
       const collector = new PluginPermissionMetadataCollector(
         mockPluginEndpointDiscovery,
         backendPluginIDsProviderMock,
-        config,
         logger,
+        config,
       );
-      await expect(collector.getPluginPolicies()).rejects.toThrow(
-        'Unexpected error',
+
+      const policiesMetadata = await collector.getPluginPolicies();
+
+      expect(policiesMetadata.length).toEqual(1);
+      expect(policiesMetadata[0].pluginId).toEqual('permission');
+      expect(policiesMetadata[0].policies).toEqual([
+        {
+          permission: 'policy.entity.read',
+          policy: 'read',
+        },
+      ]);
+
+      expect(errorSpy).toHaveBeenCalledWith(
+        'Failed to retrieve permission metadata for catalog. Error: Unexpected error',
       );
     });
+
+    it('should not log error caused by non json permission metadata for known endpoint', async () => {
+      backendPluginIDsProviderMock.getPluginIds.mockReturnValue([
+        'permission',
+        'catalog',
+      ]);
+
+      mockUrlReaderService.readUrl = jest
+        .fn()
+        .mockImplementation(async (_wellKnownURL: string) => {
+          return mockReadUrlResponse;
+        });
+      bufferMock.toString
+        .mockReturnValueOnce(
+          '{"permissions":[{"type":"resource","name":"policy.entity.read","attributes":{"action":"read"}}]}',
+        )
+        .mockReturnValueOnce('non json data');
+
+      const errorSpy = jest.spyOn(logger, 'error').mockClear();
+
+      const collector = new PluginPermissionMetadataCollector(
+        mockPluginEndpointDiscovery,
+        backendPluginIDsProviderMock,
+        logger,
+        config,
+      );
+      const policiesMetadata = await collector.getPluginPolicies();
+
+      expect(policiesMetadata.length).toEqual(1);
+      expect(policiesMetadata[0].pluginId).toEqual('permission');
+      expect(policiesMetadata[0].policies).toEqual([
+        {
+          permission: 'policy.entity.read',
+          policy: 'read',
+        },
+      ]);
+
+      // workaround for https://issues.redhat.com/browse/RHIDP-1456
+      expect(errorSpy).not.toHaveBeenCalled();
+    });
   });
 
   describe('Test list plugin condition rules', () => {
@@ -203,8 +256,8 @@ describe('plugin-endpoint', () => {
       const collector = new PluginPermissionMetadataCollector(
         mockPluginEndpointDiscovery,
         backendPluginIDsProviderMock,
-        config,
         logger,
+        config,
       );
       const conditionRulesMetadata = await collector.getPluginConditionRules();
 
@@ -222,8 +275,8 @@ describe('plugin-endpoint', () => {
       const collector = new PluginPermissionMetadataCollector(
         mockPluginEndpointDiscovery,
         backendPluginIDsProviderMock,
-        config,
         logger,
+        config,
       );
       const conditionRulesMetadata = await collector.getPluginConditionRules();
 

diff --git a/plugins/rbac-backend/src/service/plugin-endpoints.ts b/plugins/rbac-backend/src/service/plugin-endpoints.ts
@@ -43,8 +43,8 @@ export class PluginPermissionMetadataCollector {
   constructor(
     private readonly discovery: PluginEndpointDiscovery,
     private readonly pluginIdProvider: PluginIdProvider,
+    private readonly logger: Logger,
     config: Config,
-    logger: Logger,
   ) {
     this.pluginIds = this.pluginIdProvider.getPluginIds();
     this.urlReader = UrlReaders.default({
@@ -97,7 +97,13 @@ export class PluginPermissionMetadataCollector {
       try {
         const permResp = await this.urlReader.readUrl(wellKnownURL);
         const permMetaDataRaw = (await permResp.buffer()).toString();
-        const permMetaData = JSON.parse(permMetaDataRaw);
+        let permMetaData;
+        try {
+          permMetaData = JSON.parse(permMetaDataRaw);
+        } catch (err) {
+          // workaround for https://issues.redhat.com/browse/RHIDP-1456
+          continue;
+        }
         if (permMetaData) {
           pluginResponses = [
             ...pluginResponses,
@@ -108,9 +114,12 @@ export class PluginPermissionMetadataCollector {
           ];
         }
       } catch (err) {
-        if (!isError(err) || err.name !== 'NotFoundError') {
-          throw err;
+        if (isError(err) && err.name === 'NotFoundError') {
+          continue;
         }
+        this.logger.error(
+          `Failed to retrieve permission metadata for ${pluginId}. ${err}`,
+        );
       }
     }
     return pluginResponses;

diff --git a/plugins/rbac-backend/src/service/policies-rest-api.ts b/plugins/rbac-backend/src/service/policies-rest-api.ts
@@ -100,8 +100,8 @@ export class PolicesServer {
     const pluginPermMetaData = new PluginPermissionMetadataCollector(
       this.discovery,
       this.pluginIdProvider,
-      this.config,
       this.logger,
+      this.config,
     );
 
     router.use(permissionsIntegrationRouter);