refactor(toolkit-lib): remove requireApproval option from diff (#372)

`requireApproval` is deprecated and has been moved to the `CliIoHost`.
this PR removes the last place where `requireApproval` was necessary in
`tmp-toolkit-helpers`, in `formatSecurityDiff`. `formatSecurityDiff` now
makes no _decisions_ on what to print; it returns the diff and a
`permissionChangeType` for the `IoHost` to interpret. this is a slight
behavior change, so all consumers of `formatSecurityDiff have been
updated accordingly (`deploy` and `diff` in `toolkit-lib` and in the
CLI).

`requireApproval` is now a vestigial structure only in use in the CLI. 

---
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license

---------

Co-authored-by: Momo Kornher <[email protected]>

Author: kaizencc

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cdk-lib-security-related-changes-without-a-cli-are-expected-to-fail-when-approval-is-required.integtest.ts

@@ -13,9 +13,6 @@ integTest(
       neverRequireApproval: false,
     });
 
-    expect(stdErr).toContain(
-      'This deployment will make potentially sensitive changes according to your current security approval level',
-    );
     expect(stdErr).toContain(
       '"--require-approval" is enabled and stack includes security-sensitive updates',
     );

packages/@aws-cdk/tmp-toolkit-helpers/src/api/diff/diff-formatter.ts

@@ -9,21 +9,26 @@ import {
 } from '@aws-cdk/cloudformation-diff';
 import type * as cxapi from '@aws-cdk/cx-api';
 import * as chalk from 'chalk';
+import { PermissionChangeType } from '../../payloads';
 import type { NestedStackTemplates } from '../cloudformation';
 import type { IoHelper } from '../io/private';
 import { IoDefaultMessages } from '../io/private';
-import { RequireApproval } from '../require-approval';
 import { StringWriteStream } from '../streams';
-import { ToolkitError } from '../toolkit-error';
 
 /**
  * Output of formatSecurityDiff
  */
 interface FormatSecurityDiffOutput {
   /**
-   * Complete formatted security diff, if it is prompt-worthy
+   * Complete formatted security diff
    */
-  readonly formattedDiff?: string;
+  readonly formattedDiff: string;
+
+  /**
+   * The type of permission changes in the security diff.
+   * The IoHost will use this to decide whether or not to print.
+   */
+  readonly permissionChangeType: PermissionChangeType;
 }
 
 /**
@@ -57,16 +62,6 @@ interface DiffFormatterProps {
   readonly templateInfo: TemplateInfo;
 }
 
-/**
- * Properties specific to formatting the security diff
- */
-interface FormatSecurityDiffOptions {
-  /**
-   * The approval level of the security diff
-   */
-  readonly requireApproval: RequireApproval;
-}
-
 /**
  * PRoperties specific to formatting the stack diff
  */
@@ -169,6 +164,42 @@ export class DiffFormatter {
     return this._diffs;
   }
 
+  /**
+   * Get or creates the diff of a stack.
+   * If it creates the diff, it stores the result in a map for
+   * easier retreval later.
+   */
+  private diff(stackName?: string, oldTemplate?: any) {
+    const realStackName = stackName ?? this.stackName;
+
+    if (!this._diffs[realStackName]) {
+      this._diffs[realStackName] = fullDiff(
+        oldTemplate ?? this.oldTemplate,
+        this.newTemplate.template,
+        this.changeSet,
+        this.isImport,
+      );
+    }
+    return this._diffs[realStackName];
+  }
+
+  /**
+   * Return whether the diff has security-impacting changes that need confirmation.
+   *
+   * If no stackName is given, then the root stack name is used.
+   */
+  private permissionType(stackName?: string): PermissionChangeType {
+    const diff = this.diff(stackName);
+
+    if (diff.permissionsBroadened) {
+      return PermissionChangeType.BROADENING;
+    } else if (diff.permissionsAnyChanges) {
+      return PermissionChangeType.NON_BROADENING;
+    } else {
+      return PermissionChangeType.NONE;
+    }
+  }
+
   /**
    * Format the stack diff
    */
@@ -191,8 +222,7 @@ export class DiffFormatter {
     nestedStackTemplates: { [nestedStackLogicalId: string]: NestedStackTemplates } | undefined,
     options: ReusableStackDiffOptions,
   ) {
-    let diff = fullDiff(oldTemplate, this.newTemplate.template, this.changeSet, this.isImport);
-    this._diffs[stackName] = diff;
+    let diff = this.diff(stackName, oldTemplate);
 
     // The stack diff is formatted via `Formatter`, which takes in a stream
     // and sends its output directly to that stream. To faciliate use of the
@@ -277,51 +307,27 @@ export class DiffFormatter {
   /**
    * Format the security diff
    */
-  public formatSecurityDiff(options: FormatSecurityDiffOptions): FormatSecurityDiffOutput {
-    const ioDefaultHelper = new IoDefaultMessages(this.ioHelper);
+  public formatSecurityDiff(): FormatSecurityDiffOutput {
+    const diff = this.diff();
 
-    const diff = fullDiff(this.oldTemplate, this.newTemplate.template, this.changeSet);
-    this._diffs[this.stackName] = diff;
-
-    if (diffRequiresApproval(diff, options.requireApproval)) {
-      // The security diff is formatted via `Formatter`, which takes in a stream
-      // and sends its output directly to that stream. To faciliate use of the
-      // global CliIoHost, we create our own stream to capture the output of
-      // `Formatter` and return the output as a string for the consumer of
-      // `formatSecurityDiff` to decide what to do with it.
-      const stream = new StringWriteStream();
-
-      stream.write(format(`Stack ${chalk.bold(this.stackName)}\n`));
-
-      // eslint-disable-next-line max-len
-      ioDefaultHelper.warning(`This deployment will make potentially sensitive changes according to your current security approval level (--require-approval ${options.requireApproval}).`);
-      ioDefaultHelper.warning('Please confirm you intend to make the following modifications:\n');
-      try {
-        // formatSecurityChanges updates the stream with the formatted security diff
-        formatSecurityChanges(stream, diff, buildLogicalToPathMap(this.newTemplate));
-      } finally {
-        stream.end();
-      }
-      // store the stream containing a formatted stack diff
-      const formattedDiff = stream.toString();
-      return { formattedDiff };
-    }
-    return {};
-  }
-}
+    // The security diff is formatted via `Formatter`, which takes in a stream
+    // and sends its output directly to that stream. To faciliate use of the
+    // global CliIoHost, we create our own stream to capture the output of
+    // `Formatter` and return the output as a string for the consumer of
+    // `formatSecurityDiff` to decide what to do with it.
+    const stream = new StringWriteStream();
 
-/**
- * Return whether the diff has security-impacting changes that need confirmation
- *
- * TODO: Filter the security impact determination based off of an enum that allows
- * us to pick minimum "severities" to alert on.
- */
-function diffRequiresApproval(diff: TemplateDiff, requireApproval: RequireApproval) {
-  switch (requireApproval) {
-    case RequireApproval.NEVER: return false;
-    case RequireApproval.ANY_CHANGE: return diff.permissionsAnyChanges;
-    case RequireApproval.BROADENING: return diff.permissionsBroadened;
-    default: throw new ToolkitError(`Unrecognized approval level: ${requireApproval}`);
+    stream.write(format(`Stack ${chalk.bold(this.stackName)}\n`));
+
+    try {
+      // formatSecurityChanges updates the stream with the formatted security diff
+      formatSecurityChanges(stream, diff, buildLogicalToPathMap(this.newTemplate));
+    } finally {
+      stream.end();
+    }
+    // store the stream containing a formatted stack diff
+    const formattedDiff = stream.toString();
+    return { formattedDiff, permissionChangeType: this.permissionType() };
   }
 }
 

packages/@aws-cdk/tmp-toolkit-helpers/test/api/diff/diff.test.ts

@@ -2,7 +2,6 @@ import type * as cxapi from '@aws-cdk/cx-api';
 import * as chalk from 'chalk';
 import { DiffFormatter } from '../../../src/api/diff/diff-formatter';
 import { IoHelper, IoDefaultMessages } from '../../../src/api/io/private';
-import { RequireApproval } from '../../../src/api/require-approval';
 
 jest.mock('../../../src/api/io/private/messages', () => ({
   IoDefaultMessages: jest.fn(),
@@ -214,7 +213,7 @@ describe('formatSecurityDiff', () => {
     } as any;
   });
 
-  test('returns empty object when no security changes exist', () => {
+  test('returns information on security changes for the IoHost to interpret', () => {
     // WHEN
     const formatter = new DiffFormatter({
       ioHelper: mockIoHelper,
@@ -223,16 +222,14 @@ describe('formatSecurityDiff', () => {
         newTemplate: mockNewTemplate,
       },
     });
-    const result = formatter.formatSecurityDiff({
-      requireApproval: RequireApproval.BROADENING,
-    });
+    const result = formatter.formatSecurityDiff();
 
     // THEN
-    expect(result.formattedDiff).toBeUndefined();
+    expect(result.permissionChangeType).toEqual('none');
     expect(mockIoDefaultMessages.warning).not.toHaveBeenCalled();
   });
 
-  test('formats diff when permissions are broadened and approval level is BROADENING', () => {
+  test('returns formatted diff for broadening security changes', () => {
     // WHEN
     const formatter = new DiffFormatter({
       ioHelper: mockIoHelper,
@@ -241,12 +238,10 @@ describe('formatSecurityDiff', () => {
         newTemplate: mockNewTemplate,
       },
     });
-    const result = formatter.formatSecurityDiff({
-      requireApproval: RequireApproval.BROADENING,
-    });
+    const result = formatter.formatSecurityDiff();
 
     // THEN
-    expect(result.formattedDiff).toBeDefined();
+    expect(result.permissionChangeType).toEqual('broadening');
     const sanitizedDiff = result.formattedDiff!.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '').trim();
     expect(sanitizedDiff).toBe(
       'Stack test-stack\n' +
@@ -265,59 +260,4 @@ describe('formatSecurityDiff', () => {
       '(NOTE: There may be security-related changes not in this list. See https://github.com/aws/aws-cdk/issues/1299)',
     );
   });
-
-  test('formats diff for any security change when approval level is ANY_CHANGE', () => {
-    // WHEN
-    const formatter = new DiffFormatter({
-      ioHelper: mockIoHelper,
-      templateInfo: {
-        oldTemplate: {},
-        newTemplate: mockNewTemplate,
-      },
-    });
-    const result = formatter.formatSecurityDiff({
-      requireApproval: RequireApproval.ANY_CHANGE,
-    });
-
-    // THEN
-    expect(result.formattedDiff).toBeDefined();
-    expect(mockIoDefaultMessages.warning).toHaveBeenCalledWith(
-      expect.stringContaining('potentially sensitive changes'),
-    );
-    const sanitizedDiff = result.formattedDiff!.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '').trim();
-    expect(sanitizedDiff).toBe(
-      'Stack test-stack\n' +
-      'IAM Statement Changes\n' +
-      '┌───┬─────────────┬────────┬────────────────┬──────────────────────────────┬───────────┐\n' +
-      '│   │ Resource    │ Effect │ Action         │ Principal                    │ Condition │\n' +
-      '├───┼─────────────┼────────┼────────────────┼──────────────────────────────┼───────────┤\n' +
-      '│ + │ ${Role.Arn} │ Allow  │ sts:AssumeRole │ Service:lambda.amazonaws.com │           │\n' +
-      '└───┴─────────────┴────────┴────────────────┴──────────────────────────────┴───────────┘\n' +
-      'IAM Policy Changes\n' +
-      '┌───┬──────────┬──────────────────────────────────────────────────────────────────┐\n' +
-      '│   │ Resource │ Managed Policy ARN                                               │\n' +
-      '├───┼──────────┼──────────────────────────────────────────────────────────────────┤\n' +
-      '│ + │ ${Role}  │ arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole │\n' +
-      '└───┴──────────┴──────────────────────────────────────────────────────────────────┘\n' +
-      '(NOTE: There may be security-related changes not in this list. See https://github.com/aws/aws-cdk/issues/1299)',
-    );
-  });
-
-  test('returns empty object when approval level is NEVER', () => {
-    // WHEN
-    const formatter = new DiffFormatter({
-      ioHelper: mockIoHelper,
-      templateInfo: {
-        oldTemplate: {},
-        newTemplate: mockNewTemplate,
-      },
-    });
-    const result = formatter.formatSecurityDiff({
-      requireApproval: RequireApproval.NEVER,
-    });
-
-    // THEN
-    expect(result.formattedDiff).toBeUndefined();
-    expect(mockIoDefaultMessages.warning).not.toHaveBeenCalled();
-  });
 });

packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts

@@ -1,16 +1,14 @@
-import type { DescribeChangeSetOutput } from '@aws-cdk/cloudformation-diff';
-import { fullDiff } from '@aws-cdk/cloudformation-diff';
 import type * as cxapi from '@aws-cdk/cx-api';
 import * as fs from 'fs-extra';
 import * as uuid from 'uuid';
 import type { ChangeSetDiffOptions, DiffOptions, LocalFileDiffOptions } from '..';
 import { DiffMethod } from '..';
 import type { Deployments, ResourcesToImport, IoHelper, SdkProvider, StackCollection, TemplateInfo } from '../../../api/shared-private';
 import { ResourceMigrator, IO, removeNonImportResources, cfnApi } from '../../../api/shared-private';
-import { PermissionChangeType, ToolkitError } from '../../../api/shared-public';
+import { ToolkitError } from '../../../api/shared-public';
 import { deserializeStructure, formatErrorMessage } from '../../../private/util';
 
-export function makeTemplateInfos(
+export function prepareDiff(
   ioHelper: IoHelper,
   stacks: StackCollection,
   deployments: Deployments,
@@ -147,26 +145,6 @@ async function changeSetDiff(
   }
 }
 
-/**
- * Return whether the diff has security-impacting changes that need confirmation.
- */
-export function determinePermissionType(
-  oldTemplate: any,
-  newTemplate: cxapi.CloudFormationStackArtifact,
-  changeSet?: DescribeChangeSetOutput,
-): PermissionChangeType {
-  // @todo return a printable version of the full diff.
-  const diff = fullDiff(oldTemplate, newTemplate.template, changeSet);
-
-  if (diff.permissionsBroadened) {
-    return PermissionChangeType.BROADENING;
-  } else if (diff.permissionsAnyChanges) {
-    return PermissionChangeType.NON_BROADENING;
-  } else {
-    return PermissionChangeType.NONE;
-  }
-}
-
 /**
  * Appends all properties from obj2 to obj1.
  * obj2 values take priority in the case of collisions.