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.

Sign up for GitHub

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

add unit test for oac permission levels #31225

Merged
merged 1 commit into from
Aug 28, 2024
Merged

add unit test for oac permission levels #31225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions packages/aws-cdk-lib/aws-cloudfront-origins/lib/s3-bucket-origin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@ export abstract class S3BucketOrigin extends cloudfront.OriginBase {

/**
* Create a S3 Origin with Origin Access Identity (OAI) configured
* OAI is a legacy feature and we **strongly** recommend you to use OAC via `withOriginAccessControl()`
* unless it is not supported in your required region (e.g. China regions).
*/
public static withOriginAccessIdentity(bucket: IBucket, props?: S3BucketOriginWithOAIProps): cloudfront.IOrigin {
return new class extends S3BucketOrigin {
Expand Down
66 changes: 66 additions & 0 deletions packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,72 @@ describe('S3BucketOrigin', () => {
});
});
});
describe('when specifying READ, WRITE, and DELETE origin access levels', () => {
it('should add the correct permissions to bucket policy', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected this case to have been covered by the existing unit test: https://github.com/samson-keung/aws-cdk/blob/4c9d7196370351c9f4f7a61b82e2981520b0f516/packages/aws-cdk-lib/aws-cloudfront-origins/test/s3-bucket-origin.test.ts#L20

Or do we need this one to focus on the security aspect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I was thinking this is scoped to checking the permissions and covers DELETE permissions as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest modifying the existing unit test to cover the DELETE case so we do not have overlapping tests. But with that said, I don't think it is a blocked to have overlapping tests either.

const stack = new Stack();
const bucket = new s3.Bucket(stack, 'MyBucket');
const origin = origins.S3BucketOrigin.withOriginAccessControl(bucket, {
originAccessLevels: [cloudfront.AccessLevel.READ, cloudfront.AccessLevel.WRITE, cloudfront.AccessLevel.DELETE],
});
const distribution = new cloudfront.Distribution(stack, 'MyDistribution', {
defaultBehavior: { origin },
});
Template.fromStack(stack).hasResourceProperties('AWS::S3::BucketPolicy', {
PolicyDocument: {
Statement: [
{
Action: [
's3:GetObject',
's3:PutObject',
's3:DeleteObject',
],
Effect: 'Allow',
Principal: {
Service: 'cloudfront.amazonaws.com',
},
Condition: {
StringEquals: {
'AWS:SourceArn': {
'Fn::Join': [
'',
[
'arn:',
{
Ref: 'AWS::Partition',
},
':cloudfront::',
{
Ref: 'AWS::AccountId',
},
':distribution/',
{
Ref: 'MyDistribution6271DFB5',
},
],
],
},
},
},
Resource: {
'Fn::Join': [
'',
[
{
'Fn::GetAtt': [
'MyBucketF68F3FF0',
'Arn',
],
},
'/*',
],
],
},
},
],
},
});
})
})
});

describe('withOriginAccessIdentity', () => {
Expand Down
Loading