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): Role import doesn't fail when forgetting the region in the ARN #13821

Merged
merged 7 commits into from
Mar 30, 2021
17 changes: 16 additions & 1 deletion packages/@aws-cdk/aws-iam/test/role.from-role-arn.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import '@aws-cdk/assert/jest';
import { App, CfnElement, Lazy, Stack } from '@aws-cdk/core';
import { App, Aws, CfnElement, Lazy, Stack } from '@aws-cdk/core';
import { AnyPrincipal, ArnPrincipal, IRole, Policy, PolicyStatement, Role } from '../lib';

/* eslint-disable quote-props */
Expand Down Expand Up @@ -519,6 +519,21 @@ describe('IAM Role.fromRoleArn', () => {
});
});
});

describe('for an incorrect ARN', () => {
beforeEach(() => {
roleStack = new Stack(app, 'RoleStack');
});

describe("that accidentally skipped the 'region' fragment of the ARN", () => {
test('throws an exception, indicating that error', () => {
expect(() => {
Role.fromRoleArn(roleStack, 'Role',
`arn:${Aws.PARTITION}:iam:${Aws.ACCOUNT_ID}:role/AwsCicd-${Aws.REGION}-CodeBuildRole`);
}).toThrow(/The `resource` component \(6th component\) of an ARN is required:/);
});
});
});
});

function somePolicyStatement() {
Expand Down
28 changes: 18 additions & 10 deletions packages/@aws-cdk/core/lib/arn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,36 +286,44 @@ function parseToken(arnToken: string, sep: string = '/', hasName: boolean = true
* Validate that a string is either unparseable or looks mostly like an ARN
*/
function parseArnShape(arn: string): 'token' | string[] {
const components = arn.split(':');
const looksLikeArn = arn.startsWith('arn:') && components.length >= 6;
// assume anything that starts with 'arn:' is an ARN,
// so we can report better errors
const looksLikeArn = arn.startsWith('arn:');

if (!looksLikeArn) {
if (Token.isUnresolved(arn)) { return 'token'; }
throw new Error(`ARNs must start with "arn:" and have at least 6 components: ${arn}`);
if (Token.isUnresolved(arn)) {
return 'token';
} else {
throw new Error(`ARNs must start with "arn:" and have at least 6 components: ${arn}`);
}
}

// If the ARN merely contains Tokens, but otherwise *looks* mostly like an ARN,
// it's a string of the form 'arn:${partition}:service:${region}:${account}:abc/xyz'.
// it's a string of the form 'arn:${partition}:service:${region}:${account}:resource/xyz'.
// Parse fields out to the best of our ability.
// Tokens won't contain ":", so this won't break them.
const components = arn.split(':');

const [/* arn */, partition, service, /* region */ , /* account */ , resource] = components;
// const [/* arn */, partition, service, /* region */ , /* account */ , resource] = components;

const partition = components.length > 1 ? components[1] : undefined;
if (!partition) {
throw new Error('The `partition` component (2nd component) is required: ' + arn);
throw new Error('The `partition` component (2nd component) of an ARN is required: ' + arn);
}

const service = components.length > 2 ? components[2] : undefined;
if (!service) {
throw new Error('The `service` component (3rd component) is required: ' + arn);
throw new Error('The `service` component (3rd component) of an ARN is required: ' + arn);
}

const resource = components.length > 5 ? components[5] : undefined;
if (!resource) {
throw new Error('The `resource` component (6th component) is required: ' + arn);
throw new Error('The `resource` component (6th component) of an ARN is required: ' + arn);
}

// Region can be missing in global ARNs (such as used by IAM)

// Account can be missing in some ARN types (such as used for S3 buckets)

return components;
}
}
6 changes: 3 additions & 3 deletions packages/@aws-cdk/core/test/arn.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,19 +110,19 @@ nodeunitShim({

'if the ARN doesnt have enough components'(test: Test) {
const stack = new Stack();
test.throws(() => stack.parseArn('arn:is:too:short'), /ARNs must.*have at least 6 components.*arn:is:too:short/);
test.throws(() => stack.parseArn('arn:is:too:short'), /The `resource` component \(6th component\) of an ARN is required/);
test.done();
},

'if "service" is not specified'(test: Test) {
const stack = new Stack();
test.throws(() => stack.parseArn('arn:aws::4:5:6'), /The `service` component \(3rd component\) is required/);
test.throws(() => stack.parseArn('arn:aws::4:5:6'), /The `service` component \(3rd component\) of an ARN is required/);
test.done();
},

'if "resource" is not specified'(test: Test) {
const stack = new Stack();
test.throws(() => stack.parseArn('arn:aws:service:::'), /The `resource` component \(6th component\) is required/);
test.throws(() => stack.parseArn('arn:aws:service:::'), /The `resource` component \(6th component\) of an ARN is required/);
test.done();
},
},
Expand Down