diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diffable.ts b/packages/@aws-cdk/cloudformation-diff/lib/diffable.ts index 7bc87b51144ef..445f6adcef7c3 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diffable.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diffable.ts @@ -44,13 +44,13 @@ interface Eq { /** * Whether a collection contains some element (by value) */ -function contains>(element: T, xs: T[]) { +function contains>(element: T, xs: T[]): boolean { return xs.some(x => x.equal(element)); } /** * Return collection except for elements */ -function difference>(collection: T[], elements: T[]) { +function difference>(collection: T[], elements: T[]): T[] { return collection.filter(x => !contains(x, elements)); } diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts index 824e9403c95d4..291ab9803f4c9 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/iam-changes.ts @@ -4,8 +4,8 @@ import { PropertyChange, PropertyMap, ResourceChange } from '../diff/types'; import { DiffableCollection } from '../diffable'; import { renderIntrinsics } from '../render-intrinsics'; import { deepRemoveUndefined, dropIfEmpty, flatMap, makeComparator } from '../util'; -import { ManagedPolicyAttachment, ManagedPolicyJson, parseManagedPolicies } from './managed-policy'; -import { parseLambdaPermission, parseStatements, renderCondition, Statement, StatementJson, Targets } from './statement'; +import { ManagedPolicyAttachment, ManagedPolicyJson } from './managed-policy'; +import { parseLambdaPermission, parseStatements, Statement, StatementJson } from './statement'; export interface IamChangesProps { propertyChanges: PropertyChange[]; @@ -69,23 +69,25 @@ export class IamChanges { // First generate all lines, then sort on Resource so that similar resources are together for (const statement of this.statements.additions) { + const renderedStatement = statement.render(); ret.push([ '+', - renderTargets(statement.resources), - statement.effect, - renderTargets(statement.actions), - renderTargets(statement.principals), - renderCondition(statement.condition), + renderedStatement.resource, + renderedStatement.effect, + renderedStatement.action, + renderedStatement.principal, + renderedStatement.condition, ].map(s => colors.green(s))); } for (const statement of this.statements.removals) { + const renderedStatement = statement.render(); ret.push([ colors.red('-'), - renderTargets(statement.resources), - statement.effect, - renderTargets(statement.actions), - renderTargets(statement.principals), - renderCondition(statement.condition), + renderedStatement.resource, + renderedStatement.effect, + renderedStatement.action, + renderedStatement.principal, + renderedStatement.condition, ].map(s => colors.red(s))); } @@ -125,14 +127,17 @@ export class IamChanges { } /** - * Return a machine-readable version of the changes + * Return a machine-readable version of the changes. + * This is only used in tests. + * + * @internal */ - public toJson(): IamChangesJson { + public _toJson(): IamChangesJson { return deepRemoveUndefined({ - statementAdditions: dropIfEmpty(this.statements.additions.map(s => s.toJson())), - statementRemovals: dropIfEmpty(this.statements.removals.map(s => s.toJson())), - managedPolicyAdditions: dropIfEmpty(this.managedPolicies.additions.map(s => s.toJson())), - managedPolicyRemovals: dropIfEmpty(this.managedPolicies.removals.map(s => s.toJson())), + statementAdditions: dropIfEmpty(this.statements.additions.map(s => s._toJson())), + statementRemovals: dropIfEmpty(this.statements.removals.map(s => s._toJson())), + managedPolicyAdditions: dropIfEmpty(this.managedPolicies.additions.map(s => s._toJson())), + managedPolicyRemovals: dropIfEmpty(this.managedPolicies.removals.map(s => s._toJson())), }); } @@ -184,7 +189,11 @@ export class IamChanges { const appliesToPrincipal = 'AWS:${' + logicalId + '}'; return flatMap(policies, (policy: any) => { - return defaultPrincipal(appliesToPrincipal, parseStatements(renderIntrinsics(policy.PolicyDocument.Statement))); + // check if the Policy itself is not an intrinsic, like an Fn::If + const unparsedStatement = policy.PolicyDocument?.Statement + ? policy.PolicyDocument.Statement + : policy; + return defaultPrincipal(appliesToPrincipal, parseStatements(renderIntrinsics(unparsedStatement))); }); } @@ -234,11 +243,11 @@ export class IamChanges { }); } - private readManagedPolicies(policyArns: string[] | undefined, logicalId: string): ManagedPolicyAttachment[] { + private readManagedPolicies(policyArns: any, logicalId: string): ManagedPolicyAttachment[] { if (!policyArns) { return []; } const rep = '${' + logicalId + '}'; - return parseManagedPolicies(rep, renderIntrinsics(policyArns)); + return ManagedPolicyAttachment.parseManagedPolicies(rep, renderIntrinsics(policyArns)); } private readLambdaStatements(properties?: PropertyMap): Statement[] { @@ -266,16 +275,6 @@ function defaultResource(resource: string, statements: Statement[]) { return statements; } -/** - * Render into a summary table cell - */ -function renderTargets(targets: Targets): string { - if (targets.not) { - return targets.values.map(s => `NOT ${s}`).join('\n'); - } - return targets.values.join('\n'); -} - export interface IamChangesJson { statementAdditions?: StatementJson[]; statementRemovals?: StatementJson[]; diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts index 121b2eae1efd3..f0d54140398bc 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/managed-policy.ts @@ -1,13 +1,25 @@ export class ManagedPolicyAttachment { + public static parseManagedPolicies(identityArn: string, arns: string | string[]): ManagedPolicyAttachment[] { + return typeof arns === 'string' + ? [new ManagedPolicyAttachment(identityArn, arns)] + : arns.map((arn: string) => new ManagedPolicyAttachment(identityArn, arn)); + } + constructor(public readonly identityArn: string, public readonly managedPolicyArn: string) { } - public equal(other: ManagedPolicyAttachment) { + public equal(other: ManagedPolicyAttachment): boolean { return this.identityArn === other.identityArn && this.managedPolicyArn === other.managedPolicyArn; } - public toJson() { + /** + * Return a machine-readable version of the changes. + * This is only used in tests. + * + * @internal + */ + public _toJson(): ManagedPolicyJson { return { identityArn: this.identityArn, managedPolicyArn: this.managedPolicyArn }; } } @@ -16,7 +28,3 @@ export interface ManagedPolicyJson { identityArn: string; managedPolicyArn: string; } - -export function parseManagedPolicies(identityArn: string, arns: string[]): ManagedPolicyAttachment[] { - return arns.map((arn: string) => new ManagedPolicyAttachment(identityArn, arn)); -} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts b/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts index c9c950875e8bc..ea89ad4e597ee 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/iam/statement.ts @@ -32,36 +32,76 @@ export class Statement { */ public readonly condition?: any; - constructor(statement: UnknownMap) { - this.sid = expectString(statement.Sid); - this.effect = expectEffect(statement.Effect); - this.resources = new Targets(statement, 'Resource', 'NotResource'); - this.actions = new Targets(statement, 'Action', 'NotAction'); - this.principals = new Targets(statement, 'Principal', 'NotPrincipal'); - this.condition = statement.Condition; + private readonly serializedIntrinsic: string | undefined; + + constructor(statement: UnknownMap | string) { + if (typeof statement === 'string') { + this.sid = undefined; + this.effect = Effect.Unknown; + this.resources = new Targets({}, '', ''); + this.actions = new Targets({}, '', ''); + this.principals = new Targets({}, '', ''); + this.condition = undefined; + this.serializedIntrinsic = statement; + } else { + this.sid = expectString(statement.Sid); + this.effect = expectEffect(statement.Effect); + this.resources = new Targets(statement, 'Resource', 'NotResource'); + this.actions = new Targets(statement, 'Action', 'NotAction'); + this.principals = new Targets(statement, 'Principal', 'NotPrincipal'); + this.condition = statement.Condition; + this.serializedIntrinsic = undefined; + } } /** * Whether this statement is equal to the other statement */ - public equal(other: Statement) { + public equal(other: Statement): boolean { return (this.sid === other.sid && this.effect === other.effect + && this.serializedIntrinsic === other.serializedIntrinsic && this.resources.equal(other.resources) && this.actions.equal(other.actions) && this.principals.equal(other.principals) && deepEqual(this.condition, other.condition)); } - public toJson(): StatementJson { - return deepRemoveUndefined({ - sid: this.sid, - effect: this.effect, - resources: this.resources.toJson(), - principals: this.principals.toJson(), - actions: this.actions.toJson(), - condition: this.condition, - }); + public render(): RenderedStatement { + return this.serializedIntrinsic + ? { + resource: this.serializedIntrinsic, + effect: '', + action: '', + principal: this.principals.render(), // these will be replaced by the call to replaceEmpty() from IamChanges + condition: '', + } + : { + resource: this.resources.render(), + effect: this.effect, + action: this.actions.render(), + principal: this.principals.render(), + condition: renderCondition(this.condition), + }; + } + + /** + * Return a machine-readable version of the changes. + * This is only used in tests. + * + * @internal + */ + public _toJson(): StatementJson { + return this.serializedIntrinsic + ? this.serializedIntrinsic + : deepRemoveUndefined({ + sid: this.sid, + effect: this.effect, + resources: this.resources._toJson(), + principals: this.principals._toJson(), + actions: this.actions._toJson(), + condition: this.condition, + }); } /** @@ -76,6 +116,14 @@ export class Statement { } } +export interface RenderedStatement { + readonly resource: string; + readonly effect: string; + readonly action: string; + readonly principal: string; + readonly condition: string; +} + export interface StatementJson { sid?: string; effect: string; @@ -199,7 +247,22 @@ export class Targets { this.values.sort(); } - public toJson(): TargetsJson { + /** + * Render into a summary table cell + */ + public render(): string { + return this.not + ? this.values.map(s => `NOT ${s}`).join('\n') + : this.values.join('\n'); + } + + /** + * Return a machine-readable version of the changes. + * This is only used in tests. + * + * @internal + */ + public _toJson(): TargetsJson { return { not: this.not, values: this.values }; } } @@ -243,7 +306,7 @@ function forceListOfStrings(x: unknown): string[] { /** * Render the Condition column */ -export function renderCondition(condition: any) { +export function renderCondition(condition: any): string { if (!condition || Object.keys(condition).length === 0) { return ''; } const jsonRepresentation = JSON.stringify(condition, undefined, 2); diff --git a/packages/@aws-cdk/cloudformation-diff/test/iam/detect-changes.test.ts b/packages/@aws-cdk/cloudformation-diff/test/iam/detect-changes.test.ts index 1cf62c8d5286b..fd457cd701b66 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/iam/detect-changes.test.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/iam/detect-changes.test.ts @@ -14,7 +14,7 @@ test('shows new AssumeRolePolicyDocument', () => { })); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ statementAdditions: [ { effect: 'Allow', @@ -41,7 +41,7 @@ test('implicitly knows principal of identity policy for all resource types', () })); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ statementAdditions: [ { effect: 'Allow', @@ -73,7 +73,7 @@ test('policies on an identity object', () => { })); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ statementAdditions: [ { effect: 'Allow', @@ -100,7 +100,7 @@ test('if policy is attached to multiple roles all are shown', () => { })); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ statementAdditions: [ { effect: 'Allow', @@ -131,7 +131,7 @@ test('correctly parses Lambda permissions', () => { })); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ statementAdditions: [ { effect: 'Allow', @@ -162,7 +162,7 @@ test('implicitly knows resource of (queue) resource policy even if * given', () })); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ statementAdditions: [ { effect: 'Allow', @@ -189,7 +189,7 @@ test('finds sole statement removals', () => { }), {}); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ statementRemovals: [ { effect: 'Allow', @@ -233,7 +233,7 @@ test('finds one of many statement removals', () => { })); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ statementRemovals: [ { effect: 'Allow', @@ -254,7 +254,7 @@ test('finds policy attachments', () => { })); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ managedPolicyAdditions: [ { identityArn: '${SomeRole}', @@ -279,7 +279,7 @@ test('finds policy removals', () => { })); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ managedPolicyRemovals: [ { identityArn: '${SomeRole}', @@ -314,7 +314,7 @@ test('queuepolicy queue change counts as removal+addition', () => { })); // THEN - expect(diff.iamChanges.toJson()).toEqual({ + expect(diff.iamChanges._toJson()).toEqual({ statementAdditions: [ { effect: 'Allow', @@ -333,3 +333,90 @@ test('queuepolicy queue change counts as removal+addition', () => { ], }); }); + +test('supports Fn::If in the top-level property value of Role', () => { + // WHEN + const diff = diffTemplate({}, template({ + MyRole: role({ + AssumeRolePolicyDocument: poldoc({ + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { Service: 'lambda.amazonaws.com' }, + }), + ManagedPolicyArns: { + 'Fn::If': [ + 'SomeCondition', + ['then-managed-policy-arn'], + ['else-managed-policy-arn'], + ], + }, + }), + })); + + // THEN + expect(diff.iamChanges._toJson()).toEqual({ + managedPolicyAdditions: [ + { + identityArn: '${MyRole}', + managedPolicyArn: '{"Fn::If":["SomeCondition",["then-managed-policy-arn"],["else-managed-policy-arn"]]}', + }, + ], + statementAdditions: [ + { + effect: 'Allow', + principals: { not: false, values: ['Service:lambda.amazonaws.com'] }, + actions: { not: false, values: ['sts:AssumeRole'] }, + resources: { + not: false, + values: ['${MyRole.Arn}'], + }, + }, + ], + }); +}); + +test('supports Fn::If in the elements of an array-typed property of Role', () => { + // WHEN + const diff = diffTemplate({}, template({ + MyRole: role({ + AssumeRolePolicyDocument: poldoc({ + Action: 'sts:AssumeRole', + Effect: 'Allow', + Principal: { Service: 'lambda.amazonaws.com' }, + }), + Policies: [ + { + 'Fn::If': [ + 'SomeCondition', + { + PolicyName: 'S3', + PolicyDocument: poldoc({ + Effect: 'Allow', + Action: 's3:GetObject', + Resource: '*', + }), + }, + { + Ref: 'AWS::NoValue', + }, + ], + }, + ], + }), + })); + + // THEN + const changedStatements = diff.iamChanges.summarizeStatements(); + + // there are 2 rows of changes + // (one for the AssumeRolePolicyDocument, + // one for the Policies), + // plus a row of headers + expect(changedStatements.length).toBe(3); + + const changedPolicies = changedStatements[2]; + const resourceColumn = 1, principalColumn = 4; + + expect(changedPolicies[resourceColumn]).toContain('{"Fn::If":["SomeCondition",{"PolicyName":"S3","PolicyDocument":{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:GetObject","Resource":"*"}]}}]}'); + expect(changedPolicies[principalColumn]).toContain('AWS:${MyRole}'); +});