fix(s3): access denied when adding an event notification to a s3 buck…

…et (#4219) * fix(s3): fix potential access denied when adding an event notification to a s3 bucket Sometimes the custom resource finishes creating before the iam role of the lambda function is created, after which the custom resource directly executes the lambda function. This results in an access denied error. fixes #3318 * fix(s3): update expected results with dependencies on iam role and default policy * fix(s3): update expected results with dependencies on iam role and default policy for aws-lambda-event-sources package
aws · Oct 16, 2019 · 6f22446 · 6f22446
1 parent f5daa6e
commit 6f22446
Show file tree

Hide file tree

Showing 7 changed files with 64 additions and 14 deletions.
diff --git a/packages/@aws-cdk/aws-lambda-event-sources/test/integ.s3.expected.json b/packages/@aws-cdk/aws-lambda-event-sources/test/integ.s3.expected.json
@@ -73,9 +73,9 @@
       }
     },
     "B08E7C7AF": {
-      "DeletionPolicy": "Delete",
+      "Type": "AWS::S3::Bucket",
       "UpdateReplacePolicy": "Delete",
-      "Type": "AWS::S3::Bucket"
+      "DeletionPolicy": "Delete"
     },
     "BNotificationsEB8DA980": {
       "Type": "Custom::S3BucketNotifications",
@@ -187,7 +187,11 @@
         },
         "Runtime": "nodejs8.10",
         "Timeout": 300
-      }
+      },
+      "DependsOn": [
+        "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleDefaultPolicy2CF63D36",
+        "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC"
+      ]
     }
   }
-}
+}
diff --git a/packages/@aws-cdk/aws-s3-notifications/test/integ.notifications.expected.json b/packages/@aws-cdk/aws-s3-notifications/test/integ.notifications.expected.json
@@ -1,9 +1,9 @@
 {
   "Resources": {
     "Bucket83908E77": {
-      "DeletionPolicy": "Delete",
+      "Type": "AWS::S3::Bucket",
       "UpdateReplacePolicy": "Delete",
-      "Type": "AWS::S3::Bucket"
+      "DeletionPolicy": "Delete"
     },
     "BucketNotifications8F2E257D": {
       "Type": "Custom::S3BucketNotifications",
@@ -222,12 +222,16 @@
         },
         "Runtime": "nodejs8.10",
         "Timeout": 300
-      }
+      },
+      "DependsOn": [
+        "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleDefaultPolicy2CF63D36",
+        "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC"
+      ]
     },
     "Bucket25524B414": {
-      "DeletionPolicy": "Delete",
+      "Type": "AWS::S3::Bucket",
       "UpdateReplacePolicy": "Delete",
-      "Type": "AWS::S3::Bucket"
+      "DeletionPolicy": "Delete"
     },
     "Bucket2NotificationsD9BA2A77": {
       "Type": "Custom::S3BucketNotifications",
@@ -274,4 +278,4 @@
       ]
     }
   }
-}
+}
diff --git a/packages/@aws-cdk/aws-s3-notifications/test/lambda/integ.bucket-notifications.expected.json b/packages/@aws-cdk/aws-s3-notifications/test/lambda/integ.bucket-notifications.expected.json
@@ -246,7 +246,11 @@
         },
         "Runtime": "nodejs8.10",
         "Timeout": 300
-      }
+      },
+      "DependsOn": [
+        "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleDefaultPolicy2CF63D36",
+        "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC"
+      ]
     }
   }
 }
diff --git a/packages/@aws-cdk/aws-s3-notifications/test/sns/integ.sns-bucket-notifications.expected.json b/packages/@aws-cdk/aws-s3-notifications/test/sns/integ.sns-bucket-notifications.expected.json
@@ -205,7 +205,11 @@
         },
         "Runtime": "nodejs8.10",
         "Timeout": 300
-      }
+      },
+      "DependsOn": [
+        "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleDefaultPolicy2CF63D36",
+        "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC"
+      ]
     }
   }
 }
diff --git a/packages/@aws-cdk/aws-s3-notifications/test/sqs/integ.bucket-notifications.expected.json b/packages/@aws-cdk/aws-s3-notifications/test/sqs/integ.bucket-notifications.expected.json
@@ -192,7 +192,11 @@
         },
         "Runtime": "nodejs8.10",
         "Timeout": 300
-      }
+      },
+      "DependsOn": [
+        "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleDefaultPolicy2CF63D36",
+        "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC"
+      ]
     },
     "Bucket25524B414": {
       "Type": "AWS::S3::Bucket",

diff --git a/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts b/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts
@@ -84,6 +84,8 @@ export class NotificationsResourceHandler extends cdk.Construct {
       }
     });
 
+    resource.node.addDependency(role);
+
     this.functionArn = resource.getAtt('Arn').toString();
   }
 }

diff --git a/packages/@aws-cdk/aws-s3/test/test.notification.ts b/packages/@aws-cdk/aws-s3/test/test.notification.ts
@@ -1,4 +1,4 @@
-import { expect, haveResource } from '@aws-cdk/assert';
+import { expect, haveResource, haveResourceLike, ResourcePart } from '@aws-cdk/assert';
 import cdk = require('@aws-cdk/core');
 import { Test } from 'nodeunit';
 import s3 = require('../lib');
@@ -64,6 +64,34 @@ export = {
     test.done();
   },
 
+  'the notification lambda handler must depend on the role to prevent executing too early'(test: Test) {
+    const stack = new cdk.Stack();
+
+    const bucket = new s3.Bucket(stack, 'MyBucket');
+
+    bucket.addEventNotification(s3.EventType.OBJECT_CREATED, {
+      bind: () => ({
+        arn: 'ARN',
+        type: s3.BucketNotificationDestinationType.TOPIC
+      })
+    });
+
+    expect(stack).to(haveResourceLike('AWS::Lambda::Function', {
+      Type: "AWS::Lambda::Function",
+      Properties: {
+        Role: {
+          "Fn::GetAtt": [
+            "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC",
+            "Arn"
+          ]
+        },
+      }, DependsOn: [ "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleDefaultPolicy2CF63D36",
+      "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC" ]
+     }, ResourcePart.CompleteDefinition ) );
+
+    test.done();
+  },
+
   'throws with multiple prefix rules in a filter'(test: Test) {
     const stack = new cdk.Stack();