Skip to content

Commit 10974d9

Browse files
authoredOct 14, 2022
fix(iam): missing validation for actions added post instantiation of a policy statement (#21906)
## Bug Description The validation for actions/nonActions currently only exists in the constructor of the PolicyStatement class as shown below - https://github.com/aws/aws-cdk/blob/56ba2ab2c2d9240b76ece17c3296488a63f0b232/packages/%40aws-cdk/aws-iam/lib/policy-statement.ts#L88-L95 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 - ```typescript 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: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*
1 parent afff407 commit 10974d9

File tree

3 files changed

+36
-23
lines changed

3 files changed

+36
-23
lines changed
 

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

+12-8
Original file line numberDiff line numberDiff line change
@@ -86,14 +86,6 @@ export class PolicyStatement {
8686
private _frozen = false;
8787

8888
constructor(props: PolicyStatementProps = {}) {
89-
// Validate actions
90-
for (const action of [...props.actions || [], ...props.notActions || []]) {
91-
92-
if (!/^(\*|[a-zA-Z0-9-]+:[a-zA-Z0-9*]+)$/.test(action) && !cdk.Token.isUnresolved(action)) {
93-
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.`);
94-
}
95-
}
96-
9789
this._sid = props.sid;
9890
this._effect = props.effect || Effect.ALLOW;
9991

@@ -154,6 +146,7 @@ export class PolicyStatement {
154146
if (actions.length > 0 && this._notAction.length > 0) {
155147
throw new Error('Cannot add \'Actions\' to policy statement if \'NotActions\' have been added');
156148
}
149+
this.validatePolicyActions(actions);
157150
this._action.push(...actions);
158151
}
159152

@@ -170,6 +163,7 @@ export class PolicyStatement {
170163
if (notActions.length > 0 && this._action.length > 0) {
171164
throw new Error('Cannot add \'NotActions\' to policy statement if \'Actions\' have been added');
172165
}
166+
this.validatePolicyActions(notActions);
173167
this._notAction.push(...notActions);
174168
}
175169

@@ -233,6 +227,16 @@ export class PolicyStatement {
233227
}
234228
}
235229

230+
private validatePolicyActions(actions: string[]) {
231+
// In case of an unresolved list of actions return early
232+
if (cdk.Token.isUnresolved(actions)) return;
233+
for (const action of actions || []) {
234+
if (!cdk.Token.isUnresolved(action) && !/^(\*|[a-zA-Z0-9-]+:[a-zA-Z0-9*]+)$/.test(action)) {
235+
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.`);
236+
}
237+
}
238+
}
239+
236240
private validatePolicyPrincipal(principal: IPrincipal) {
237241
if (principal instanceof Group) {
238242
throw new Error('Cannot use an IAM Group as the \'Principal\' or \'NotPrincipal\' in an IAM Policy');

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

+15-15
Original file line numberDiff line numberDiff line change
@@ -147,11 +147,11 @@ describe('IAM policy document', () => {
147147
const stack = new Stack();
148148
const perm = new PolicyStatement();
149149
perm.addResources('MyResource');
150-
perm.addActions('Action1', 'Action2', 'Action3');
150+
perm.addActions('service:Action1', 'service:Action2', 'service:Action3');
151151

152152
expect(stack.resolve(perm.toStatementJson())).toEqual({
153153
Effect: 'Allow',
154-
Action: ['Action1', 'Action2', 'Action3'],
154+
Action: ['service:Action1', 'service:Action2', 'service:Action3'],
155155
Resource: 'MyResource',
156156
});
157157
});
@@ -325,12 +325,12 @@ describe('IAM policy document', () => {
325325
const stack = new Stack();
326326

327327
const statement = new PolicyStatement();
328-
statement.addActions(...Lazy.list({ produce: () => ['a', 'b', 'c'] }));
328+
statement.addActions(...Lazy.list({ produce: () => ['service:a', 'service:b', 'service:c'] }));
329329
statement.addResources(...Lazy.list({ produce: () => ['x', 'y', 'z'] }));
330330

331331
expect(stack.resolve(statement.toStatementJson())).toEqual({
332332
Effect: 'Allow',
333-
Action: ['a', 'b', 'c'],
333+
Action: ['service:a', 'service:b', 'service:c'],
334334
Resource: ['x', 'y', 'z'],
335335
});
336336
});
@@ -339,15 +339,15 @@ describe('IAM policy document', () => {
339339
const stack = new Stack();
340340

341341
const statement = new PolicyStatement();
342-
statement.addActions('a');
343-
statement.addActions('a');
342+
statement.addActions('service:a');
343+
statement.addActions('service:a');
344344

345345
statement.addResources('x');
346346
statement.addResources('x');
347347

348348
expect(stack.resolve(statement.toStatementJson())).toEqual({
349349
Effect: 'Allow',
350-
Action: 'a',
350+
Action: 'service:a',
351351
Resource: 'x',
352352
});
353353
});
@@ -356,15 +356,15 @@ describe('IAM policy document', () => {
356356
const stack = new Stack();
357357

358358
const statement = new PolicyStatement();
359-
statement.addNotActions('a');
360-
statement.addNotActions('a');
359+
statement.addNotActions('service:a');
360+
statement.addNotActions('service:a');
361361

362362
statement.addNotResources('x');
363363
statement.addNotResources('x');
364364

365365
expect(stack.resolve(statement.toStatementJson())).toEqual({
366366
Effect: 'Allow',
367-
NotAction: 'a',
367+
NotAction: 'service:a',
368368
NotResource: 'x',
369369
});
370370
});
@@ -421,10 +421,10 @@ describe('IAM policy document', () => {
421421
s.addArnPrincipal('349494949494');
422422
s.addServicePrincipal('test.service');
423423
s.addResources('resource');
424-
s.addActions('action');
424+
s.addActions('service:action');
425425

426426
expect(stack.resolve(s.toStatementJson())).toEqual({
427-
Action: 'action',
427+
Action: 'service:action',
428428
Effect: 'Allow',
429429
Principal: { AWS: '349494949494', Service: 'test.service' },
430430
Resource: 'resource',
@@ -723,7 +723,7 @@ describe('IAM policy document', () => {
723723

724724
const statement = new PolicyStatement();
725725
statement.addResources('resource1', 'resource2');
726-
statement.addActions('action1', 'action2');
726+
statement.addActions('service:action1', 'service:action2');
727727
statement.addServicePrincipal('service');
728728
statement.addConditions({
729729
a: {
@@ -750,11 +750,11 @@ describe('IAM policy document', () => {
750750

751751
const statement1 = new PolicyStatement();
752752
statement1.addResources(Lazy.string({ produce: () => 'resource' }));
753-
statement1.addActions(Lazy.string({ produce: () => 'action' }));
753+
statement1.addActions(Lazy.string({ produce: () => 'service:action' }));
754754

755755
const statement2 = new PolicyStatement();
756756
statement2.addResources(Lazy.string({ produce: () => 'resource' }));
757-
statement2.addActions(Lazy.string({ produce: () => 'action' }));
757+
statement2.addActions(Lazy.string({ produce: () => 'service:action' }));
758758

759759
// WHEN
760760
p.addStatements(statement1);

‎packages/@aws-cdk/aws-iam/test/policy-statement.test.ts

+9
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,15 @@ describe('IAM policy statement', () => {
215215
.toThrow(/Cannot use an IAM Group as the 'Principal' or 'NotPrincipal' in an IAM Policy/);
216216
});
217217

218+
test('throws error when an invalid \'Action\' or \'NotAction\' is added', () => {
219+
const policyStatement = new PolicyStatement();
220+
const invalidAction = 'xyz';
221+
expect(() => policyStatement.addActions(invalidAction))
222+
.toThrow(`Action '${invalidAction}' is invalid. An action string consists of a service namespace, a colon, and the name of an action. Action names can include wildcards.`);
223+
expect(() => policyStatement.addNotActions(invalidAction))
224+
.toThrow(`Action '${invalidAction}' is invalid. An action string consists of a service namespace, a colon, and the name of an action. Action names can include wildcards.`);
225+
});
226+
218227
test('multiple identical entries render to a scalar (instead of a singleton list)', () => {
219228
const stack = new Stack();
220229
const policyStatement = new PolicyStatement({

0 commit comments

Comments
 (0)
Please sign in to comment.