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

The CompositePrincipal signature is inconvenient to use #2327

Closed
mitchlloyd opened this issue Apr 18, 2019 · 3 comments · Fixed by #2507
Closed

The CompositePrincipal signature is inconvenient to use #2327

mitchlloyd opened this issue Apr 18, 2019 · 3 comments · Fixed by #2507
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management feature-request A feature should be added or improved.

Comments

@mitchlloyd
Copy link
Contributor

Describe the bug
The new CompositePrincipal() constructor accepts a list of PolicyPrinciples as arguments. I'd like to be able to refactor my code to pass an array of principals, but the type is fighting me.

const compositePrincipal = new iam.CompositePrincipal(...authorizedPrincipals);
// Type error: Expected at least 1 arguments, but got 0 or more.

I realize there is a tradeoff here, since logically we'd like to have at least 1 principal in a composite principal. However, I feel that the benefit of a type error for new iam.CompositePrincipal() (rather than a synth-time error) is less than the benefit of being able to pass an array as arguments.

To Reproduce

import * as iam from '@aws-cdk/aws-iam';

const ps = [new iam.AccountPrincipal('123'), new iam.AccountPrincipal('456')];
new iam.CompositePrincipal(...ps);

Expected behavior
No type errors.

Version:

  • OS: OSX
  • Programming Language: TypeScript
  • CDK Version: 0.19.0 (build 2625a05) (but also current version)
@mitchlloyd mitchlloyd added the bug This issue is a bug. label Apr 18, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 18, 2019

That is a fair point, and it's actually an instance of a design strategy I hate myself (designing APIs for direct human use only, to the neglect of higher-level abstractions).

I concur, it should be an array with a run-time check.

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 18, 2019

Although it's honestly not that hard to work around:

new iam.CompositePrincipal(ps[0], ...ps.slice(1));

@mitchlloyd
Copy link
Contributor Author

That's what I did ^, but making consumers jump through that hoop seems unfortunate.

@RomainMuller RomainMuller added feature-request A feature should be added or improved. @aws-cdk/aws-iam Related to AWS Identity and Access Management and removed bug This issue is a bug. labels Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants