fix(s3): updating blockPublicAccess to enable default true settings (under feature flag)

[IkeNefcy]

V3 but I think we got there

Issue

Closes #32811

Reason for this change

By default when you create an s3 bucket, all public access is already blocked. However if you then use CDK to specify 1 or more access point you want to unblock, all undefined block types will be auto set to false, and when it deploys you will see everything uncheck even if you only wanted to uncheck 1 thing.

So to fix this we should instead default all values to true when at least 1 option is specified, to mimic to experience when a user in the console unchecks the boxes.

Description of changes

deprecating BLOCK_ACLS method of BlockPublicAccess. Adding BLOCK_ACLS_ONLY.

  public static readonly BLOCK_ACLS_ONLY = new BlockPublicAccess({
    blockPublicAcls: true,
    blockPublicPolicy: false,
    ignorePublicAcls: true,
    restrictPublicBuckets: false,
  });

This is just a general revamp to match what the feature will bring, it's separate from the feature itself. The point being that for any shortcut methods like this, we should be specifying all 4 options to ensure the default true behavior remains.

Created function setBlockPublicAccessDefaults()

  /**
   * Function to set the blockPublicAccessOptions to a true default if not defined.
   * If no blockPublicAccessOptions are specified at all, this is already the case as an s3 default in aws
   * @see https://docs.aws.amazon.com/AmazonS3/latest/userguide/access-control-block-public-access.html
   */
  private setBlockPublicAccessDefaults(blockPublicAccessOptions: BlockPublicAccessOptions) {
    return {
      blockPublicAcls: blockPublicAccessOptions.blockPublicAcls ?? true,
      blockPublicPolicy: blockPublicAccessOptions.blockPublicPolicy ?? true,
      ignorePublicAcls: blockPublicAccessOptions.ignorePublicAcls ?? true,
      restrictPublicBuckets: blockPublicAccessOptions.restrictPublicBuckets ?? true,
    };
  }

but this method is only called if the FF is enabled

    let blockPublicAccess: BlockPublicAccessOptions | undefined = props.blockPublicAccess;
    if (props.blockPublicAccess && FeatureFlags.of(this).isEnabled(cxapi.S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE)) {
      blockPublicAccess = this.setBlockPublicAccessDefaults(props.blockPublicAccess);
    }

Of course the FF itself was added.

Description of how you validated changes

Added tests that are duplicates of others, just testing for both behaviors with and without the FF.

  describe('bucket with custom block public access setting', () => {
    ...
    test('S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE Enabled', () => {
      const app = new cdk.App({
        context: {
          [cxapi.S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE]: true,
        },
      });
      const stack = new cdk.Stack(app);
      new s3.Bucket(stack, 'MyBucket', {
        blockPublicAccess: new s3.BlockPublicAccess({ restrictPublicBuckets: false }),
      });

      Template.fromStack(stack).templateMatches({
        'Resources': {
          'MyBucketF68F3FF0': {
            'Type': 'AWS::S3::Bucket',
            'Properties': {
              'PublicAccessBlockConfiguration': {
                'BlockPublicAcls': true,
                'BlockPublicPolicy': true,
                'IgnorePublicAcls': true,
                'RestrictPublicBuckets': false,
              },
            },
            'DeletionPolicy': 'Retain',
            'UpdateReplacePolicy': 'Retain',
          },
        },
      });
    });

  describe('bucket with custom block public access setting', () => {
    ...
    test('S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE Enabled', () => {
      const app = new cdk.App({
        context: {
          [cxapi.S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE]: true,
        },
      });
      const stack = new cdk.Stack(app);
      new s3.Bucket(stack, 'MyBucket', {
        blockPublicAccess: new s3.BlockPublicAccess({ restrictPublicBuckets: false }),
      });

      Template.fromStack(stack).templateMatches({
        'Resources': {
          'MyBucketF68F3FF0': {
            'Type': 'AWS::S3::Bucket',
            'Properties': {
              'PublicAccessBlockConfiguration': {
                'BlockPublicAcls': true,
                'BlockPublicPolicy': true,
                'IgnorePublicAcls': true,
                'RestrictPublicBuckets': false,
              },
            },
            'DeletionPolicy': 'Retain',
            'UpdateReplacePolicy': 'Retain',
          },
        },
      });
    });

Also added an integ that just tests different combinations of the blocking.

aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket-block-access.ts

Lines 1 to 42 in 51ffe21

	import { IntegTest } from '@aws-cdk/integ-tests-alpha';
	import { App, Stack } from 'aws-cdk-lib';
	import { BlockPublicAccess, Bucket } from 'aws-cdk-lib/aws-s3';
	import { S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE } from 'aws-cdk-lib/cx-api';
	const app = new App({
	context: {
	[S3_BLOCK_PUBLIC_ACCESS_OPTION_AUTO_TRUE]: true,
	},
	});
	const stack = new Stack(app, 'aws-cdk-s3-bucket-block-access');
	new Bucket(stack, 'blockAcls', {
	blockPublicAccess: BlockPublicAccess.BLOCK_ACLS,
	});
	new Bucket(stack, 'blockAclsOnly', {
	blockPublicAccess: BlockPublicAccess.BLOCK_ACLS_ONLY,
	});
	new Bucket(stack, 'blockAll', {
	blockPublicAccess: BlockPublicAccess.BLOCK_ALL,
	});
	// in the next 2 tests, because not all 4 options are blocked, those left undefined should default to true
	new Bucket(stack, 'blockOnly2', {
	blockPublicAccess: new BlockPublicAccess({
	blockPublicAcls: false,
	blockPublicPolicy: false,
	}),
	});
	new Bucket(stack, 'blockOnly1', {
	blockPublicAccess: new BlockPublicAccess({
	blockPublicPolicy: false,
	}),
	});
	new IntegTest(app, 'testBlockingBucketAccess', {
	testCases: [stack],
	});

There was no BlockPublicAccess integ before so I did not add the context for the FF disabled anywhere. The tests should still be working since it's not used that often. But if the team needs me to, I can add a 2nd integ with the old behavior

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.98%. Comparing base (2dc8cc7) to head (255742b).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33702   +/-   ##
=======================================
  Coverage   83.98%   83.98%           
=======================================
  Files         120      120           
  Lines        6976     6976           
  Branches     1178     1178           
=======================================
  Hits         5859     5859           
  Misses       1005     1005           
  Partials      112      112           

Flag	Coverage Δ
suite.unit	83.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	∅ <ø> (∅)
packages/aws-cdk-lib/core	83.98% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[IkeNefcy]

Don't approve this until #33796 is merged, so I can handle the conflicts

[IkeNefcy]

Another ff was added without a readme..

[IkeNefcy]

please wait for #33867 so I can resolve conflict

[QuantumNeuralCoder]

lgtm

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[mergify]

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[IkeNefcy]

    Mergify doesn't have permission to update

    For security reasons, Mergify can't update this pull request. Try updating locally.
    GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/pr-linter-exemption-labeler.yml without workflows permission

hmm there's a new one

Pull request has been modified.

[QuantumNeuralCoder]

Please hold of making any changes. We are looking into this

[samson-keung]

Approving on @QuantumNeuralCoder's behalf as the auto-merge should work now.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[samson-keung]

@IkeNefcy can you fix the merge conflicts? Thank you!

[IkeNefcy]

Will do in a few

Pull request has been modified.

[samson-keung]

Approving again as merge conflict is addressed

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: d6853e4
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.