Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(iam): conditions parameters accept array values #21009

Merged
merged 6 commits into from
Jul 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions packages/@aws-cdk/aws-iam/lib/policy-statement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { IConstruct } from 'constructs';
import { Group } from './group';
import {
AccountPrincipal, AccountRootPrincipal, AnyPrincipal, ArnPrincipal, CanonicalUserPrincipal,
FederatedPrincipal, IPrincipal, PrincipalBase, PrincipalPolicyFragment, ServicePrincipal, ServicePrincipalOpts,
FederatedPrincipal, IPrincipal, PrincipalBase, PrincipalPolicyFragment, ServicePrincipal, ServicePrincipalOpts, validateConditionObject,
} from './principals';
import { normalizeStatement } from './private/postprocess-policy-document';
import { LITERAL_STRING_KEY, mergePrincipal, sum } from './util';
Expand Down Expand Up @@ -380,6 +380,8 @@ export class PolicyStatement {
*/
public addCondition(key: string, value: Condition) {
this.assertNotFrozen('addCondition');
validateConditionObject(value);

const existingValue = this._condition[key];
this._condition[key] = existingValue ? { ...existingValue, ...value } : value;
}
Expand Down Expand Up @@ -670,19 +672,15 @@ export enum Effect {
* Condition for when an IAM policy is in effect. Maps from the keys in a request's context to
* a string value or array of string values. See the Conditions interface for more details.
*/
export type Condition = any;
export type Condition = unknown;

// NOTE! We'd ideally like to type this as `Record<string, any>`, because the
// API expects a map which can take either strings or lists of strings.
//
// However, if we were to change this right now, the Java bindings for CDK would
// emit a type of `Map<String, Object>`, but the most common types people would
// instantiate would be an `ImmutableMap<String, String>` which would not be
// assignable to `Map<String, Object>`. The types don't have a built-in notion
// of co-contravariance, you have to indicate that on the type. So jsii would first
// need to emit the type as `Map<String, ? extends Object>`.
// NOTE! We would have liked to have typed this as `Record<string, unknown>`, but in some places
// of the code we are assuming we can pass a `CfnJson` object into where a `Condition` is expected,
// and that wouldn't typecheck anymore.
//
// Feature request in https://github.com/aws/jsii/issues/1517
// Needs to be `unknown` instead of `any` so that the type of `Conditions` is
// `Record<string, unknown>`; if it had been `Record<string, any>`, TypeScript would have allowed
// passing an array into `conditions` arguments (where it needs to be a map).

/**
* Conditions for when an IAM Policy is in effect, specified in the following structure:
Expand Down Expand Up @@ -877,4 +875,4 @@ class OrderedSet<A> {
public direct(): readonly A[] {
return this.array;
}
}
}
26 changes: 25 additions & 1 deletion packages/@aws-cdk/aws-iam/lib/principals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,15 @@ export class PrincipalWithConditions extends PrincipalAdapter {
* Add a condition to the principal
*/
public addCondition(key: string, value: Condition) {
validateConditionObject(value);

const existingValue = this.additionalConditions[key];
this.additionalConditions[key] = existingValue ? { ...existingValue, ...value } : value;
if (!existingValue) {
this.additionalConditions[key] = value;
}
validateConditionObject(existingValue);

this.additionalConditions[key] = { ...existingValue, ...value };
}

/**
Expand Down Expand Up @@ -335,6 +342,9 @@ export class PrincipalWithConditions extends PrincipalAdapter {
throw new Error(`multiple "${operator}" conditions cannot be merged if one of them contains an unresolved token`);
}

validateConditionObject(existing);
validateConditionObject(condition);

mergedConditions[operator] = { ...existing, ...condition };
});
return mergedConditions;
Expand Down Expand Up @@ -913,3 +923,17 @@ class ServicePrincipalToken implements cdk.IResolvable {
return `<${this.service}>`;
}
}

/**
* Validate that the given value is a valid Condition object
*
* The type of `Condition` should have been different, but it's too late for that.
*
* Also, the IAM library relies on being able to pass in a `CfnJson` instance for
* a `Condition`.
*/
export function validateConditionObject(x: unknown): asserts x is Record<string, unknown> {
if (!x || typeof x !== 'object' || Array.isArray(x)) {
throw new Error('A Condition should be represented as a map of operator to value');
}
}
14 changes: 11 additions & 3 deletions packages/@aws-cdk/aws-lambda/lib/function-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -618,9 +618,9 @@ export abstract class FunctionBase extends Resource implements IFunction, ec2.IC

if (!conditions) { return undefined; }

const sourceArn = conditions.ArnLike ? conditions.ArnLike['aws:SourceArn'] : undefined;
const sourceAccount = conditions.StringEquals ? conditions.StringEquals['aws:SourceAccount'] : undefined;
const principalOrgID = conditions.StringEquals ? conditions.StringEquals['aws:PrincipalOrgID'] : undefined;
const sourceArn = requireString(requireObject(conditions.ArnLike)?.['aws:SourceArn']);
const sourceAccount = requireString(requireObject(conditions.StringEquals)?.['aws:SourceAccount']);
const principalOrgID = requireString(requireObject(conditions.StringEquals)?.['aws:PrincipalOrgID']);

// PrincipalOrgID cannot be combined with any other conditions
if (principalOrgID && (sourceArn || sourceAccount)) {
Expand Down Expand Up @@ -768,3 +768,11 @@ class LatestVersion extends FunctionBase implements IVersion {
return addAlias(this, this, aliasName, options);
}
}

function requireObject(x: unknown): Record<string, unknown> | undefined {
return x && typeof x === 'object' && !Array.isArray(x) ? x as any : undefined;
}

function requireString(x: unknown): string | undefined {
return x && typeof x === 'string' ? x : undefined;
}