fix(iam): missing validation for actions added post instantiation of a policy statement

[VarunWachaspati]

Bug Description

The validation for actions/nonActions currently only exists in the constructor of the PolicyStatement class as shown below -

aws-cdk/packages/@aws-cdk/aws-iam/lib/policy-statement.ts

Lines 88 to 95 in 56ba2ab

	constructor(props: PolicyStatementProps = {}) {
	// Validate actions
	for (const action of [...props.actions || [], ...props.notActions || []]) {
	if (!/^(\*|[a-zA-Z0-9-]+:[a-zA-Z0-9*]+)$/.test(action) && !cdk.Token.isUnresolved(action)) {
	throw new Error(`Action '${action}' is invalid. An action string consists of a service namespace, a colon, and the name of an action. Action names can include wildcards.`);
	}
	}

The above validation is missing when we add an action/nonAction post instantiation of the IAM policy statement leading to discrepancy in the behaviour.
The following snippet doesn't throw any error -

const statement = new iam.PolicyStatement({ resources: ['*'] });
statement.addActions('action');
statement.addNonActions('nonaction');

Solution

• Refactored the validation in the constructor into a separate private method called validatePolicyActions()
• Executing this new validation method in the addActions() and addNonActions()
• Fixed existing unit tests which assumed the above behaviour

fixes #21821

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• This PR adds new unconventional dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

[gitpod-io]

[VarunWachaspati]

Need some guidance in fixing the following failing unit test.

aws-cdk/packages/@aws-cdk/aws-iam/test/policy-document.test.ts

Lines 324 to 336 in 1a37331

	test('addResources() will not break a list-encoded Token', () => {
	const stack = new Stack();
	const statement = new PolicyStatement();
	statement.addActions(...Lazy.list({ produce: () => ['service:a', 'service:b', 'service:c'] }));
	statement.addResources(...Lazy.list({ produce: () => ['x', 'y', 'z'] }));
	expect(stack.resolve(statement.toStatementJson())).toEqual({
	Effect: 'Allow',
	Action: ['service:a', 'service:b', 'service:c'],
	Resource: ['x', 'y', 'z'],
	});
	});

As per my understanding, Lazy.list(...) token should evaluate to true when passed to cdk.Token.isUnresolved(), but for some unknown reason it is evaluating to false instead and throwing an error as per the below logic.

aws-cdk/packages/@aws-cdk/aws-iam/lib/policy-statement.ts

Lines 232 to 233 in 1a37331

	if (!cdk.Token.isUnresolved(action) && !/^(\*|[a-zA-Z0-9-]+:[a-zA-Z0-9*]+)$/.test(action)) {
	throw new Error(`Action '${action}' is invalid. An action string consists of a service namespace, a colon, and the name of an action. Action names can include wildcards.`);

Thus causing the above unit test to fail.

Is this is a bug Token.isUnresolved() method by any chance?

[TheRealAmazonKendra]

Need some guidance in fixing the following failing unit test.

aws-cdk/packages/@aws-cdk/aws-iam/test/policy-document.test.ts

Lines 324 to 336 in 1a37331

	test('addResources() will not break a list-encoded Token', () => {
	const stack = new Stack();
	const statement = new PolicyStatement();
	statement.addActions(...Lazy.list({ produce: () => ['service:a', 'service:b', 'service:c'] }));
	statement.addResources(...Lazy.list({ produce: () => ['x', 'y', 'z'] }));
	expect(stack.resolve(statement.toStatementJson())).toEqual({
	Effect: 'Allow',
	Action: ['service:a', 'service:b', 'service:c'],
	Resource: ['x', 'y', 'z'],
	});
	});

As per my understanding, Lazy.list(...) token should evaluate to true when passed to cdk.Token.isUnresolved(), but for some unknown reason it is evaluating to false instead and throwing an error as per the below logic.

aws-cdk/packages/@aws-cdk/aws-iam/lib/policy-statement.ts

Lines 232 to 233 in 1a37331

	if (!cdk.Token.isUnresolved(action) && !/^(\*|[a-zA-Z0-9-]+:[a-zA-Z0-9*]+)$/.test(action)) {
	throw new Error(`Action '${action}' is invalid. An action string consists of a service namespace, a colon, and the name of an action. Action names can include wildcards.`);

Thus causing the above unit test to fail.
Is this is a bug Token.isUnresolved() method by any chance?

You don't seem to have any tokens in

Need some guidance in fixing the following failing unit test.

aws-cdk/packages/@aws-cdk/aws-iam/test/policy-document.test.ts

Lines 324 to 336 in 1a37331

	test('addResources() will not break a list-encoded Token', () => {
	const stack = new Stack();
	const statement = new PolicyStatement();
	statement.addActions(...Lazy.list({ produce: () => ['service:a', 'service:b', 'service:c'] }));
	statement.addResources(...Lazy.list({ produce: () => ['x', 'y', 'z'] }));
	expect(stack.resolve(statement.toStatementJson())).toEqual({
	Effect: 'Allow',
	Action: ['service:a', 'service:b', 'service:c'],
	Resource: ['x', 'y', 'z'],
	});
	});

As per my understanding, Lazy.list(...) token should evaluate to true when passed to cdk.Token.isUnresolved(), but for some unknown reason it is evaluating to false instead and throwing an error as per the below logic.

aws-cdk/packages/@aws-cdk/aws-iam/lib/policy-statement.ts

Lines 232 to 233 in 1a37331

	if (!cdk.Token.isUnresolved(action) && !/^(\*|[a-zA-Z0-9-]+:[a-zA-Z0-9*]+)$/.test(action)) {
	throw new Error(`Action '${action}' is invalid. An action string consists of a service namespace, a colon, and the name of an action. Action names can include wildcards.`);

Thus causing the above unit test to fail.
Is this is a bug Token.isUnresolved() method by any chance?

You've created a token list but then are evaluating each individual token as a string. The formatting for list vs string are different.

[VarunWachaspati]

Thank you so much @TheRealAmazonKendra for helping out.
I have updated the PR accordingly now and all existing tests are passing.
Since this was primarily a refactor, I didn't add any additional tests. But if anything else is required please let me know.

[TheRealAmazonKendra]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[VarunWachaspati]

Hi @TheRealAmazonKendra
Can we merge this PR?
If not, please let me know the next steps.

[VarunWachaspati]

Bumping up for visibility.

[TheRealAmazonKendra]

@Mergifyio update

[mergify]

update

✅ Branch has been successfully updated

[aws-cdk-automation]

The Pull Request Linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

[TheRealAmazonKendra]

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests).

Additionally, we need test cases for failure cases. The tests in this PR only covers success cases.

[VarunWachaspati]

Sorry for the delay.

• Changed the title of the PR to the best of my knowledge to reflect the nature of the bug being fixed, if it doesn't cut it please do let me know.
• Added a failure unit test case asserting that we throw an error whenever an invalid action is being added to the policy statement post instantiation it throws an error.
• Can you please guide me in adding an integration test for this change? I wasn't able to figure it out.

[Naumel]

(...)
* Can you please guide me in adding an integration test for this change? I wasn't able to figure it out.

Hello, have you looked at https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md already?

[VarunWachaspati]

Yes, I did. I was confused regarding what kind of integration should I be adding in this case, since this PR fixes a validation check that was previously missed. I have added unit tests to assert the validation.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

[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: 9b47ba5
• 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).