Skip to content

Commit d9085cc

Browse files
authored
fix(s3): resolve synthesis error in BucketPolicy.fromCfnBucketPolicy() (#35633)
### Issue # (if applicable) Closes [#34322](#34322) ### Reason for this change Fixes synthesis error: 'Supplied properties not correct for CfnBucketPolicyProps: policyDocument: required but missing' ### Description of changes - Fix synthesis error where duplicate CfnBucketPolicy was created without policyDocument - Ensure PolicyDocument is properly passed from original CfnBucketPolicy to constructor - Maintains existing behavior while fixing synthesis bug ### Description of how you validated changes - Add test to verify synthesis works and duplicate resources are created as expected ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 765a0d2 commit d9085cc

File tree

2 files changed

+52
-10
lines changed

2 files changed

+52
-10
lines changed

packages/aws-cdk-lib/aws-s3/lib/bucket-policy.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,13 @@ export interface BucketPolicyProps {
1919
* @default - RemovalPolicy.DESTROY.
2020
*/
2121
readonly removalPolicy?: RemovalPolicy;
22+
23+
/**
24+
* Policy document to apply to the bucket.
25+
*
26+
* @default - A new empty PolicyDocument will be created.
27+
*/
28+
readonly document?: PolicyDocument;
2229
}
2330

2431
/**
@@ -83,10 +90,9 @@ export class BucketPolicy extends Resource implements IBucketPolicyRef {
8390
bucket = Bucket.fromBucketName(cfnBucketPolicy, '@FromCfnBucket', cfnBucketPolicy.bucket);
8491
}
8592

86-
const ret = new class extends BucketPolicy {
87-
public readonly document = PolicyDocument.fromJson(cfnBucketPolicy.policyDocument);
88-
}(cfnBucketPolicy, id, {
93+
const ret = new BucketPolicy(cfnBucketPolicy, id, {
8994
bucket,
95+
document: PolicyDocument.fromJson(cfnBucketPolicy.policyDocument),
9096
});
9197

9298
// mark the Bucket as having this Policy
@@ -101,7 +107,7 @@ export class BucketPolicy extends Resource implements IBucketPolicyRef {
101107
* For more information, see Access Policy Language Overview in the Amazon
102108
* Simple Storage Service Developer Guide.
103109
*/
104-
public readonly document = new PolicyDocument();
110+
public readonly document: PolicyDocument;
105111

106112
/** The Bucket this Policy applies to. */
107113
public readonly bucket: IBucket;
@@ -114,6 +120,7 @@ export class BucketPolicy extends Resource implements IBucketPolicyRef {
114120
addConstructMetadata(this, props);
115121

116122
this.bucket = props.bucket;
123+
this.document = props.document ?? new PolicyDocument();
117124

118125
this.resource = new CfnBucketPolicy(this, 'Resource', {
119126
bucket: this.bucket.bucketName,

packages/aws-cdk-lib/aws-s3/test/bucket-policy.test.ts

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -129,27 +129,29 @@ describe('bucket policy', () => {
129129
});
130130

131131
test('fails if bucket policy has no actions', () => {
132-
const app = new App();
133-
const stack = new Stack(app, 'my-stack');
132+
const stack = new Stack();
134133
const myBucket = new s3.Bucket(stack, 'MyBucket');
135134
myBucket.addToResourcePolicy(new PolicyStatement({
136135
resources: [myBucket.bucketArn],
137136
principals: [new AnyPrincipal()],
138137
}));
139138

140-
expect(() => app.synth()).toThrow(/A PolicyStatement must specify at least one \'action\' or \'notAction\'/);
139+
expect(() => {
140+
Template.fromStack(stack).toJSON();
141+
}).toThrow(/A PolicyStatement must specify at least one \'action\' or \'notAction\'/);
141142
});
142143

143144
test('fails if bucket policy has no IAM principals', () => {
144-
const app = new App();
145-
const stack = new Stack(app, 'my-stack');
145+
const stack = new Stack();
146146
const myBucket = new s3.Bucket(stack, 'MyBucket');
147147
myBucket.addToResourcePolicy(new PolicyStatement({
148148
resources: [myBucket.bucketArn],
149149
actions: ['s3:GetObject*'],
150150
}));
151151

152-
expect(() => app.synth()).toThrow(/A PolicyStatement used in a resource-based policy must specify at least one IAM principal/);
152+
expect(() => {
153+
Template.fromStack(stack).toJSON();
154+
}).toThrow(/A PolicyStatement used in a resource-based policy must specify at least one IAM principal/);
153155
});
154156

155157
describe('fromCfnBucketPolicy()', () => {
@@ -176,6 +178,39 @@ describe('bucket policy', () => {
176178
expect(bucketPolicy.bucket.bucketName).toBe('hardcoded-name');
177179
});
178180

181+
test('should synthesize without errors and create duplicate cfn resource', () => {
182+
const testStack = new Stack();
183+
const cfnBucketPolicy = new s3.CfnBucketPolicy(testStack, 'TestBucketPolicy', {
184+
policyDocument: {
185+
'Statement': [
186+
{
187+
'Action': 's3:*',
188+
'Effect': 'Deny',
189+
'Principal': {
190+
'AWS': '*',
191+
},
192+
'Resource': '*',
193+
},
194+
],
195+
'Version': '2012-10-17',
196+
},
197+
bucket: 'test-bucket',
198+
});
199+
200+
s3.BucketPolicy.fromCfnBucketPolicy(cfnBucketPolicy);
201+
202+
// Verify that two CfnBucketPolicy resources are created
203+
const template = Template.fromStack(testStack);
204+
const bucketPolicies = template.findResources('AWS::S3::BucketPolicy');
205+
expect(Object.keys(bucketPolicies)).toHaveLength(2);
206+
207+
// Both should have valid policy documents
208+
Object.values(bucketPolicies).forEach((policy: any) => {
209+
expect(policy.Properties.PolicyDocument).toBeDefined();
210+
expect(policy.Properties.PolicyDocument.Statement).toBeDefined();
211+
});
212+
});
213+
179214
function bucketPolicyForBucketNamed(name: string): CfnBucketPolicy {
180215
return new s3.CfnBucketPolicy(stack, `CfnBucketPolicy-${name}`, {
181216
policyDocument: {

0 commit comments

Comments
 (0)