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.

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

fix(iam): Error for addToPolicy and addToPrincipalPolicy in User.fromUserName corrected... #11046

Closed
wants to merge 16 commits into from
Closed
14 changes: 4 additions & 10 deletions packages/@aws-cdk/aws-iam/lib/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,19 +145,13 @@ export class User extends Resource implements IIdentity, IUser {
public readonly userArn: string = arn;
public readonly assumeRoleAction: string = 'sts:AssumeRole';
public readonly policyFragment: PrincipalPolicyFragment = new ArnPrincipal(arn).policyFragment;
private defaultPolicy?: Policy;

public addToPolicy(statement: PolicyStatement): boolean {
return this.addToPrincipalPolicy(statement).statementAdded;
public addToPolicy(_statement: PolicyStatement): boolean {
throw new Error('Cannot add imported User to policy');
ariain marked this conversation as resolved.
Show resolved Hide resolved
}

public addToPrincipalPolicy(statement: PolicyStatement): AddToPrincipalPolicyResult {
if (!this.defaultPolicy) {
this.defaultPolicy = new Policy(this, 'Policy');
this.defaultPolicy.attachToUser(this);
}
this.defaultPolicy.addStatements(statement);
return { statementAdded: true, policyDependable: this.defaultPolicy };
public addToPrincipalPolicy(_statement: PolicyStatement): AddToPrincipalPolicyResult {
throw new Error('Cannot add imported User to principal policy');
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message is the wrong way around.

Cannot add policy to imported User principal makes more sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sort of fine having this throw an error, but as discussed in #10913 this could just be made to work.

If you add an AWS::IAM::Policy and you give it the right UserName, it becomes possible to add an identity policy to an existing identity, as far as I can tell.

Why don't we just do that?

Maybe the correct fix is to figure out where the exception that was mentioned in #10913 is thrown, and make sure that error isn't thrown anymore. Because the content of addToPrincipalPolicy mostly seems correct, except it probably throws when attachToUser is called... (?)

Copy link
Author

Choose a reason for hiding this comment

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

The error message will be common with addToPolicy function so maybe we can throw this message Cannot add policy to imported User.
If we don't want to throw an error then there are several problems here if we try to correct the functionality because its not like other functions are working. The attachToUser will call to attachInlinePolicy which will throw an error, I can try to fix that function...

Copy link
Author

Choose a reason for hiding this comment

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

So, should I check for the correct functionality or throw an error instead??

}

public addToGroup(_group: IGroup): void {
Expand Down
45 changes: 44 additions & 1 deletion packages/@aws-cdk/aws-iam/test/user.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import '@aws-cdk/assert/jest';
import { App, SecretValue, Stack } from '@aws-cdk/core';
import { ManagedPolicy, User } from '../lib';
import { ManagedPolicy, Policy, PolicyStatement, User } from '../lib';

describe('IAM user', () => {
test('default user', () => {
Expand Down Expand Up @@ -93,4 +93,47 @@ describe('IAM user', () => {
'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':user/MyUserName']],
});
});

test('imported user cannot be added to policy', () => {
// GIVEN
const stack = new Stack();

// WHEN
const user = User.fromUserName(stack, 'import', 'MyUserName');

// THEN
expect(() => stack.resolve(user.addToPolicy(new PolicyStatement()))).toThrowError(
new Error('Cannot add imported User to policy'),
);
});

test('imported user cannot be added to principal policy', () => {
// GIVEN
const stack = new Stack();

// WHEN
const user = User.fromUserName(stack, 'import', 'MyUserName');

// THEN
expect(() => stack.resolve(user.addToPrincipalPolicy(
new PolicyStatement(),
))).toThrowError(
new Error('Cannot add imported User to principal policy'),
);
});

test('inline ploicy cannot be added to an imported user', () => {
// GIVEN
const stack = new Stack();

// WHEN
const user = User.fromUserName(stack, 'import', 'MyUserName');

// THEN
expect(() => stack.resolve(user.attachInlinePolicy(
new Policy(stack, 'testPolicy'),
))).toThrowError(
new Error('Cannot add inline policy to imported User'),
);
});
});