fix: certificate and key management

- don't try to enable identity provider with a non existing certificate
- don't try to re-import existing certificate from keystore

src/plugins/security/certificate-and-key-management/index.ts

@@ -4,7 +4,11 @@ import * as jsonMergePatch from 'json-merge-patch';
 import * as path from 'path';
 import * as queryString from 'querystring';
 import { BrowserforcePlugin } from '../../../plugin';
-import { removeEmptyValues } from '../../utils';
+import {
+  removeEmptyValues,
+  removeNullValues,
+  semanticallyCleanObject
+} from '../../utils';
 
 const PATHS = {
   CERT_PREFIX: '0P1',
@@ -45,23 +49,27 @@ type KeyStore = {
 
 export class CertificateAndKeyManagement extends BrowserforcePlugin {
   public async retrieve(definition?: Config): Promise<Config> {
-    const response = { certificates: [], importFromKeystore: [] };
-    if (definition && definition.certificates) {
-      const certificatesList = definition.certificates
-        .map(cert => {
-          return `'${cert.name}'`;
-        })
-        .join(',');
-      const certificatesResponse = await this.org
+    const response: Config = {
+      certificates: [],
+      importFromKeystore: []
+    };
+    let existingCertificates;
+    if (
+      definition?.certificates?.length ||
+      definition?.importFromKeystore?.length
+    ) {
+      existingCertificates = await this.org
         .getConnection()
         .tooling.query<CertificateRecord>(
-          `SELECT Id, DeveloperName, MasterLabel, OptionsIsPrivateKeyExportable, KeySize FROM Certificate WHERE DeveloperName IN (${certificatesList})`,
+          `SELECT Id, DeveloperName, MasterLabel, OptionsIsPrivateKeyExportable, KeySize FROM Certificate`,
           { scanAll: false }
+          // BUG in jsforce: query acts with scanAll:true and returns deleted CustomObjects.
+          // It cannot be disabled.
         );
-      // BUG in jsforce: query acts with scanAll:true and returns deleted CustomObjects.
-      // It cannot be disabled.
+    }
+    if (definition?.certificates?.length) {
       for (const cert of definition.certificates) {
-        const existingCert = certificatesResponse.records.find(
+        const existingCert = existingCertificates.records.find(
           co => co.DeveloperName === cert.name
         );
         if (existingCert) {
@@ -75,6 +83,19 @@ export class CertificateAndKeyManagement extends BrowserforcePlugin {
         }
       }
     }
+    if (definition?.importFromKeystore?.length) {
+      for (const cert of definition.importFromKeystore) {
+        const existingCert = existingCertificates.records.find(
+          co => co.DeveloperName === cert.name
+        );
+        if (existingCert) {
+          response.importFromKeystore.push({
+            name: existingCert.DeveloperName,
+            filePath: null
+          });
+        }
+      }
+    }
     return response;
   }
 
@@ -85,19 +106,26 @@ export class CertificateAndKeyManagement extends BrowserforcePlugin {
     };
     if (state && definition && state.certificates && definition.certificates) {
       for (const cert of definition.certificates) {
-        const existingCert = state.certificates.find(
-          c => c.name === cert.DeveloperName
-        );
+        const existingCert = state.certificates.find(c => c.name === cert.name);
         if (existingCert) {
           // move id from state to definition to be retained and used
           cert._id = existingCert._id;
           delete existingCert._id;
         }
-        response.certificates.push(jsonMergePatch.generate(existingCert, cert));
+        const certDiff = semanticallyCleanObject(
+          removeNullValues(jsonMergePatch.generate(existingCert, cert)),
+          '_id'
+        );
+        if (certDiff) {
+          response.certificates.push(certDiff);
+        }
       }
     }
-    if (definition && definition.importFromKeystore) {
-      response.importFromKeystore = definition.importFromKeystore;
+    if (definition?.importFromKeystore?.length) {
+      response.importFromKeystore =
+        definition?.importFromKeystore?.filter(
+          cert => !state.importFromKeystore.find(c => c.name === cert.name)
+        ) || [];
     }
     return removeEmptyValues(response);
   }

src/plugins/security/identity-provider/index.ts

@@ -1,6 +1,6 @@
 import { SalesforceId } from 'jsforce';
 import * as jsonMergePatch from 'json-merge-patch';
-import pRetry from 'p-retry';
+import pRetry, { AbortError } from 'p-retry';
 import { BrowserforcePlugin } from '../../../plugin';
 import { removeNullValues } from '../../utils';
 
@@ -59,8 +59,10 @@ export class IdentityProvider extends BrowserforcePlugin {
             .tooling.query<CertificateRecord>(
               `SELECT Id, DeveloperName FROM Certificate WHERE DeveloperName = '${plan.certificate}'`
             );
-          if (!certsResponse.records.length) {
-            throw new Error(`Could not find Certificate '${plan.certificate}'`);
+          if (!certsResponse.totalSize) {
+            throw new AbortError(
+              `Could not find Certificate '${plan.certificate}'`
+            );
           }
           const page = await this.browserforce.openPage(PATHS.EDIT_VIEW);
           await page.waitForSelector(SELECTORS.EDIT_BUTTON);

src/plugins/security/index.e2e-spec.ts

@@ -1,3 +1,4 @@
+import { core } from '@salesforce/command';
 import * as assert from 'assert';
 import * as child from 'child_process';
 import * as path from 'path';
@@ -49,6 +50,20 @@ describe(`${CertificateAndKeyManagement.name} and ${IdentityProvider.name}`, fun
       cmd.output.toString()
     );
   });
+  it('should not do anything if self-signed certificate is already available', () => {
+    const cmd = child.spawnSync(path.resolve('bin', 'run'), [
+      'browserforce:apply',
+      '-f',
+      path.resolve(
+        path.join(__dirname, 'identity-provider', 'create-cert-and-enable.json')
+      )
+    ]);
+    assert.deepStrictEqual(cmd.status, 0, cmd.output.toString());
+    assert(
+      /no action necessary/.test(cmd.output.toString()),
+      cmd.output.toString()
+    );
+  });
   it('should disable Identity Provider', () => {
     const cmd = child.spawnSync(path.resolve('bin', 'run'), [
       'browserforce:apply',
@@ -63,25 +78,27 @@ describe(`${CertificateAndKeyManagement.name} and ${IdentityProvider.name}`, fun
       cmd.output.toString()
     );
   });
-  it.skip('should not do anything if self-signed certificate is already available', () => {
+  it('should import a cert from a keystore', () => {
     const cmd = child.spawnSync(path.resolve('bin', 'run'), [
       'browserforce:apply',
       '-f',
       path.resolve(
         path.join(
           __dirname,
           'certificate-and-key-management',
-          'create-self-signed-cert.json'
+          'import-from-keystore.json'
         )
       )
     ]);
     assert.deepStrictEqual(cmd.status, 0, cmd.output.toString());
     assert(
-      /no action necessary/.test(cmd.output.toString()),
+      /changing 'certificateAndKeyManagement' to '{"importFromKeystore":\[.*"filePath"/.test(
+        cmd.output.toString()
+      ),
       cmd.output.toString()
     );
   });
-  it('should import a cert from a keystore', () => {
+  it('should not do anything if cert is already available in keystore', () => {
     const cmd = child.spawnSync(path.resolve('bin', 'run'), [
       'browserforce:apply',
       '-f',
@@ -95,10 +112,13 @@ describe(`${CertificateAndKeyManagement.name} and ${IdentityProvider.name}`, fun
     ]);
     assert.deepStrictEqual(cmd.status, 0, cmd.output.toString());
     assert(
-      /changing 'certificateAndKeyManagement' to '{"importFromKeystore":\[.*"filePath"/.test(
-        cmd.output.toString()
-      ),
+      /no action necessary/.test(cmd.output.toString()),
       cmd.output.toString()
     );
   });
+  it('should delete certificates using Metadata API', async () => {
+    const org = await core.Org.create({});
+    const conn = org.getConnection();
+    await conn.metadata.delete('Certificate', ['identity_provider', 'Dummy']);
+  });
 });