Merge branch 'main' into sqs-kms-policy-condition

mergify[bot] · web-flow · commit 48fa29661631 · 2022-07-08T13:47:10.000Z
diff --git a/packages/@aws-cdk/aws-iam/lib/policy-statement.ts b/packages/@aws-cdk/aws-iam/lib/policy-statement.ts
@@ -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';
@@ -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;
   }
@@ -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:
@@ -877,4 +875,4 @@ class OrderedSet<A> {
   public direct(): readonly A[] {
     return this.array;
   }
-}
+}
diff --git a/packages/@aws-cdk/aws-iam/lib/principals.ts b/packages/@aws-cdk/aws-iam/lib/principals.ts
@@ -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 };
   }
 
   /**
@@ -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;
@@ -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');
+  }
+}
diff --git a/packages/@aws-cdk/aws-lambda/lib/function-base.ts b/packages/@aws-cdk/aws-lambda/lib/function-base.ts
@@ -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)) {
@@ -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;
+}
