Skip to content

Commit

Permalink
Requested changes
Browse files Browse the repository at this point in the history
  • Loading branch information
RomainMuller committed Oct 15, 2018
1 parent d68f8ce commit 769c28b
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 55 deletions.
78 changes: 27 additions & 51 deletions packages/@aws-cdk/aws-iam/lib/optimize-statements.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export function optimizeStatements(statements: any[]): any[] {
/**
* A Policy statement, with convenience functions make it easier to de-duplicate, normalize, ...
*/
class Statement implements Deduplicable, Mergeable {
class Statement implements Deduplicable {
private readonly sid?: string;

private readonly effect: 'Allow' | 'Deny';
Expand Down Expand Up @@ -54,20 +54,15 @@ class Statement implements Deduplicable, Mergeable {
}
}

public mergeable(other: Statement, allowSid = false): boolean {
return (allowSid || this.sid == null)
public subsumes(other: Statement): boolean {
return this.sid == null
&& this.sid === other.sid
&& this.effect === other.effect
&& this._allSuperceded(other.actions, this.actions)
&& _allSuperceded(other.actions, this.actions)
&& _deepEqual(this.condition, other.condition)
&& _deepEqual(this.principal, other.principal);
}

public supercedes(other: Statement): boolean {
return this.mergeable(other, true /* allow Sid to be present, if equal */)
&& this._allSuperceded(other.resources, this.resources);
}

public merge(other: Statement): void {
this.resources.push(...other.resources);
}
Expand Down Expand Up @@ -143,17 +138,6 @@ class Statement implements Deduplicable, Mergeable {
return compact;
}
}

/**
* Verifies if a list of entities is entirely covered by another.
* @param left a list of de-duplicable entities
* @param right a list of de-duplicable entities.
* @returns ``true`` if all entries from ``left`` are superceded by at least one entry from ``right``.
*/
private _allSuperceded<T extends Deduplicable>(left: T[], right: T[]): boolean {
if (left.length === 0) { return right.length === 0; }
return left.find(item => right.find(e => e.supercedes(item)) == null) == null;
}
}

/**
Expand All @@ -169,14 +153,16 @@ class Glob implements Deduplicable {
this.regexp = _toRegExp(this.expression);
}

public supercedes(other: Glob): boolean {
public subsumes(other: Glob): boolean {
if (!this.regexp || typeof other.expression !== 'string') {
return _deepEqual(this.expression, other.expression);
}
return this.regexp.test(other.expression)
// Use the shortest expression (** matches what * does, and * is better)
&& (this.expression as string).length <= other.expression.length;
}

public merge(_other: Glob): void { /* Nothing to do here */ }
}

/**
Expand All @@ -188,30 +174,25 @@ interface Deduplicable {
* @param other the other entity to check.
* @returns ``true`` if this entry supercedes (can safely replace) ``other``.
*/
supercedes(other: this): boolean;
}
subsumes(other: this): boolean;

/**
* Entities that can be merged.
*/
interface Mergeable {
/**
* Checks whether an entity is compatible for merging in this one.
* @param other the other entity to check.
* @returns ``true`` if ``other`` can safely be merged into this entity.
*/
mergeable(other: this): boolean;
/**
* Merges ``other`` in this entity. It is the caller's responsibility to have
* verified the safety of doing so by having called ``mergeable`` before.
* @param other the entity to merge in.
*/
merge(other: this): void;
}
function _isMergeable<T>(x: T): x is T & Mergeable {
return x != null
&& typeof (x as any).mergeable === 'function'
&& typeof (x as any).merge === 'function';

/**
* Verifies if a list of entities is entirely covered by another.
* @param left a list of de-duplicable entities
* @param right a list of de-duplicable entities.
* @returns ``true`` if all entries from ``left`` are superceded by at least one entry from ``right``.
*/
function _allSuperceded<T extends Deduplicable>(left: T[], right: T[]): boolean {
if (left.length === 0) { return right.length === 0; }
return left.find(item => right.find(e => e.subsumes(item)) == null) == null;
}

/**
Expand All @@ -221,21 +202,16 @@ function _isMergeable<T>(x: T): x is T & Mergeable {
*/
function _deduplicate<T extends Deduplicable>(array: T[]): T[] {
const result = new Array<T>();
for (let i = 0 ; i < array.length ; i++) {
const element = array[i];
let redundant = false;
for (let j = i + 1 ; j < array.length ; j++) {
const candidate = array[j];
if (candidate.supercedes(element)) {
redundant = true;
break;
} else if (_isMergeable(candidate) && _isMergeable(element) && candidate.mergeable(element)) {
candidate.merge(element);
redundant = true;
break;
while (array.length > 0) {
result.push(array.splice(0, 1)[0]);
const last = result[result.length - 1];
for (let i = 0 ; i < array.length ; ) {
if (last.subsumes(array[i])) {
last.merge(array.splice(i, 1)[0]);
} else {
i++;
}
}
if (!redundant) { result.push(element); }
}
return result;
}
Expand Down Expand Up @@ -277,7 +253,7 @@ function _deepEqual(e1: unknown, e2: unknown): boolean {
function _toRegExp(glob: unknown): RegExp | undefined {
if (typeof glob !== 'string') { return undefined; }
const parts = glob.split('*');
return new RegExp(`^${parts.map(_quote).join('.*')}\$`);
return new RegExp('^' + parts.map(_quote).join('[^:]*') + '$');

function _quote(text: string): string {
// RegExp special characters: \ ^$ . | ? * + ( ) [ ] { }
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-iam/lib/role.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export interface RoleProps {
* cicrular dependencies that could otherwise be introduced).
* @default No inline policies.
*/
policies?: { [name: string]: PolicyDocument };
inlinePolicies?: { [name: string]: PolicyDocument };

/**
* The path associated with this role. For information about IAM paths, see
Expand Down Expand Up @@ -121,7 +121,7 @@ export class Role extends Construct implements IIdentityResource, IPrincipal, ID
const role = new cloudformation.RoleResource(this, 'Resource', {
assumeRolePolicyDocument: this.assumeRolePolicy as any,
managedPolicyArns: undefinedIfEmpty(() => this.managedPolicyArns),
policies: _flatten(props.policies),
policies: _flatten(props.inlinePolicies),
path: props.path,
roleName: props.roleName,
maxSessionDuration: props.maxSessionDurationSec,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export class NotificationsResourceHandler extends cdk.Construct {
resourceName: 'service-role/AWSLambdaBasicExecutionRole',
})
],
policies: {
inlinePolicies: {
// handler allows to put bucket notification on s3 buckets.
allowPutBucketNotification: new iam.PolicyDocument()
.addStatement(new iam.PolicyStatement()
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/cdk/lib/cloudformation/fn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export class FnJoin extends Fn {
if (this.listOfValues.length === 1) {
return this.listOfValues[0];
}
return Fn.prototype.resolve.call(this);
return super.resolve();
}
}

Expand Down

0 comments on commit 769c28b

Please sign in to comment.