fix(iam): AccountPrincipal accepts values which aren't account IDs #20292

tejasmr · 2022-05-11T14:25:51Z

Changed the type of accountId in AccountPrincipal constructor to string from any fixes #20288

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

Changed the type of accountId in AccountPrincipal constructor to string from any to fix issue #20288

gitpod-io · 2022-05-11T14:25:54Z

tejasmr · 2022-05-11T14:45:16Z

The parameter accountId was having the type 'any' and being assigned to a data member of class AccountPrincipal called principalAccount which is of type 'string | undefined'. accountId and principalAccount are meant to hold the AWS account id which should be in the form of string. Changing the type annotation of accountId as parameter in AccountPrincipal constructor won't have any other effects other than specifying the TypeScript linter the type of the parameter accountId. There doesn't seem to be any other reason for the type of accountId to be 'any' since this parameter is only part of the constructor, and will be used to create the stack dependent token and assigning to principalAccount in the constructor. The format of the stack dependent token only works with valid accountId, and the principalAccount should also be valid accountId. If you pass an object, it should give you TypeError to indicate the error in the format given to the AccountPrincipal constructor.

tejasmr · 2022-05-11T15:48:46Z

Seems like a check compatibility issue: check-api-compatibility.sh
From this, aws-iam folder, there is a .warnings.jsii.js file which is not tracked in git.
This file has the following line:

function _aws_cdk_aws_iam_AccountPrincipal(p) {
}
...
module.exports = { ..., _aws_cdk_aws_iam_AccountPrincipal, ... };

The build is failing because of this. Adding it to allowed-breaking-changes.txt

Added `incompatible-argument:@aws-cdk/aws-iam.AccountPrincipal.<initializer>` to allowed-breaking-changes.txt to fix the build error.

tejasmr · 2022-05-14T13:02:48Z

@rix0rrr can you review this PR?

rix0rrr

I agree this is poor choices in the past, but unencoded tokens (IResolvable) are accepted here, and the change would require code changes by consumers (i.e., not acceptable).

We have to change this into a run-time check, unfortunately:

if (!Token.isUnresolved(accountId) && typeof accountId !== 'string') {
  throw new Error('...');
}

tejasmr

I added this change inside the constructor and reverted back to any. Should this be a fix or a chore?

Added run time check for accountId type and added appropriate error message for that. Reverted the type annotation for accountId to any

Pull request has been modified.

rix0rrr · 2022-05-17T07:53:58Z

I added this change inside the constructor and reverted back to any. Should this be a fix or a chore?

Both are reasonable. You can call it a fix if you want to :)

rix0rrr · 2022-05-17T07:54:30Z

@aws-cdk/aws-iam: lib/principals.ts:396:10 - error TS2304: Cannot find name 'Token'.
@aws-cdk/aws-iam: 396     if (!Token.isUnresolved(accountId) && typeof accountId !== 'string') {

Fixed the error ```bash @aws-cdk/aws-iam: lib/principals.ts:395:3 - error TS2376: A 'super' call must be the first statement in the constructor when a class contains initialized properties, parameter properties, or private identifiers. ```

tejasmr · 2022-05-17T10:53:37Z

@rix0rrr can you review?

rix0rrr

Still needs a test, and the breaking changes except is no longer necessary

Pull request has been modified.

mergify · 2022-05-19T14:32:45Z

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

aws-cdk-automation · 2022-05-19T16:54:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 23fa8f8
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-05-19T16:54:42Z

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

…ws#20292) Changed the type of accountId in AccountPrincipal constructor to string from any fixes aws#20288 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/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/master/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*

---- ### 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* --- With the merge of [#20292](#20292) the accountID type was essentially required to be a string with the exception of an unencoded token. So, while the type of accountId cannot be set to string and must remain any to preserve compatibility, the hint for accountId should at least suggest that a string should be used rather than an int.

Changed type of accountId from any to string

7938032

Changed the type of accountId in AccountPrincipal constructor to string from any to fix issue #20288

github-actions bot added the p2 label May 11, 2022

aws-cdk-automation requested a review from a team May 11, 2022 14:26

aws-cdk-automation assigned Naumel May 11, 2022

tejasmr changed the title ~~Changed type of accountId from any to string~~ fix(iam): Changed type of accountId from any to string May 11, 2022

Added AccountPrincipal.<initializer> to allowed breaking changes

4f28f07

Added `incompatible-argument:@aws-cdk/aws-iam.AccountPrincipal.<initializer>` to allowed-breaking-changes.txt to fix the build error.

rix0rrr previously requested changes May 16, 2022

View reviewed changes

tejasmr commented May 16, 2022

View reviewed changes

tejasmr requested a review from rix0rrr May 16, 2022 17:56

Added run time check, reverted type to any

34e2487

Added run time check for accountId type and added appropriate error message for that. Reverted the type annotation for accountId to any

tejasmr added 2 commits May 16, 2022 23:44

Fixed formatting

6b1fb11

Changed Token to cdk.Token because of import

ad16abd

github-actions bot added the bug This issue is a bug. label May 16, 2022

tejasmr changed the title ~~fix(iam): Changed type of accountId from any to string~~ fix(iam): Type of accountId in AccountPrincipal constructor is any May 16, 2022

rix0rrr previously requested changes May 18, 2022

View reviewed changes

Removed entry of AccountPrincipal from allowed breaking changes

80bb437

Added unit test for AccountPrincipal constructor

003c271

tejasmr requested a review from rix0rrr May 18, 2022 15:27

rix0rrr approved these changes May 19, 2022

View reviewed changes

rix0rrr changed the title ~~fix(iam): Type of accountId in AccountPrincipal constructor is any~~ fix(iam): AccountPrincipal accepts values which aren't account IDs May 19, 2022

Merge branch 'master' into master

23fa8f8

mergify bot merged commit d0163f8 into aws:master May 19, 2022

rix0rrr added the effort/small Small work item – less than a day of effort label May 25, 2022

misterjacko mentioned this pull request Sep 20, 2022

docs(iam): hints at accountId as string type #22149

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iam): AccountPrincipal accepts values which aren't account IDs #20292

fix(iam): AccountPrincipal accepts values which aren't account IDs #20292

tejasmr commented May 11, 2022 •

edited

Loading

gitpod-io bot commented May 11, 2022

tejasmr commented May 11, 2022

tejasmr commented May 11, 2022 •

edited

Loading

tejasmr commented May 14, 2022

rix0rrr left a comment •

edited

Loading

tejasmr left a comment •

edited

Loading

rix0rrr commented May 17, 2022

rix0rrr commented May 17, 2022

tejasmr commented May 17, 2022

rix0rrr left a comment

mergify bot commented May 19, 2022

aws-cdk-automation commented May 19, 2022

mergify bot commented May 19, 2022

fix(iam): AccountPrincipal accepts values which aren't account IDs #20292

fix(iam): AccountPrincipal accepts values which aren't account IDs #20292

Conversation

tejasmr commented May 11, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented May 11, 2022

tejasmr commented May 11, 2022

tejasmr commented May 11, 2022 • edited Loading

tejasmr commented May 14, 2022

rix0rrr left a comment • edited Loading

Choose a reason for hiding this comment

tejasmr left a comment • edited Loading

Choose a reason for hiding this comment

rix0rrr commented May 17, 2022

rix0rrr commented May 17, 2022

tejasmr commented May 17, 2022

rix0rrr left a comment

Choose a reason for hiding this comment

mergify bot commented May 19, 2022

aws-cdk-automation commented May 19, 2022

AWS CodeBuild CI Report

mergify bot commented May 19, 2022

tejasmr commented May 11, 2022 •

edited

Loading

tejasmr commented May 11, 2022 •

edited

Loading

rix0rrr left a comment •

edited

Loading

tejasmr left a comment •

edited

Loading