Skip to content

Commit

Permalink
fix(servicecatalog): Portfolio fails validation when passed Tokens as…
Browse files Browse the repository at this point in the history
… its properties (aws#15208)

We currently do not check if inputs are token when validating them which might throw errors
if it is given a `ref`, adding them in here and corresponding unit tests that do not throw validation errors
when given tokens with invalid inputs.

Also updated one test which had mismatched description.

Also moved core to qualified import for better consistency. 

Testing done
------------------
* `yarn build && yarn lint & yarn test`


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
arcrank committed Jun 22, 2021
1 parent 16b8a4e commit ce727c1
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 17 deletions.
12 changes: 6 additions & 6 deletions packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as iam from '@aws-cdk/aws-iam';
import { IResource, Names, Resource, Stack } from '@aws-cdk/core';
import * as cdk from '@aws-cdk/core';
import { AcceptLanguage } from './common';
import { hashValues } from './private/util';
import { InputValidator } from './private/validation';
Expand Down Expand Up @@ -29,7 +29,7 @@ export interface PortfolioShareOptions {
/**
* A Service Catalog portfolio.
*/
export interface IPortfolio extends IResource {
export interface IPortfolio extends cdk.IResource {
/**
* The ARN of the portfolio.
* @attribute
Expand Down Expand Up @@ -68,7 +68,7 @@ export interface IPortfolio extends IResource {
shareWithAccount(accountId: string, options?: PortfolioShareOptions): void;
}

abstract class PortfolioBase extends Resource implements IPortfolio {
abstract class PortfolioBase extends cdk.Resource implements IPortfolio {
public abstract readonly portfolioArn: string;
public abstract readonly portfolioId: string;
private readonly associatedPrincipals: Set<string> = new Set();
Expand Down Expand Up @@ -156,7 +156,7 @@ export class Portfolio extends PortfolioBase {
* @param portfolioArn the Amazon Resource Name of the existing portfolio.
*/
public static fromPortfolioArn(scope: Construct, id: string, portfolioArn: string): IPortfolio {
const arn = Stack.of(scope).parseArn(portfolioArn);
const arn = cdk.Stack.of(scope).parseArn(portfolioArn);
const portfolioId = arn.resourceName;

if (!portfolioId) {
Expand Down Expand Up @@ -193,15 +193,15 @@ export class Portfolio extends PortfolioBase {
acceptLanguage: props.acceptLanguage,
});
this.portfolioId = this.portfolio.ref;
this.portfolioArn = Stack.of(this).formatArn({
this.portfolioArn = cdk.Stack.of(this).formatArn({
service: 'servicecatalog',
resource: 'portfolio',
resourceName: this.portfolioId,
});
}

protected generateUniqueHash(value: string): string {
return hashValues(Names.nodeUniqueId(this.portfolio.node), value);
return hashValues(cdk.Names.nodeUniqueId(this.portfolio.node), value);
}

private validatePortfolioProps(props: PortfolioProps) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import * as cdk from '@aws-cdk/core';

/**
* Class to validate that inputs match requirements.
*/
Expand All @@ -6,7 +8,7 @@ export class InputValidator {
* Validates length is between allowed min and max lengths.
*/
public static validateLength(resourceName: string, inputName: string, minLength: number, maxLength: number, inputString?: string): void {
if (inputString !== undefined && (inputString.length < minLength || inputString.length > maxLength)) {
if (!cdk.Token.isUnresolved(inputString) && inputString !== undefined && (inputString.length < minLength || inputString.length > maxLength)) {
throw new Error(`Invalid ${inputName} for resource ${resourceName}, must have length between ${minLength} and ${maxLength}, got: '${this.truncateString(inputString, 100)}'`);
}
}
Expand All @@ -17,4 +19,4 @@ export class InputValidator {
}
return string;
}
}
}
63 changes: 54 additions & 9 deletions packages/@aws-cdk/aws-servicecatalog/test/portfolio.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import '@aws-cdk/assert-internal/jest';
import { Stack, Tags } from '@aws-cdk/core';
import * as iam from '@aws-cdk/aws-iam';
import * as cdk from '@aws-cdk/core';
import * as servicecatalog from '../lib';

describe('Portfolio', () => {
let stack: Stack;
let stack: cdk.Stack;

beforeEach(() => {
stack = new Stack();
stack = new cdk.Stack();
});

describe('portfolio creation and importing', () => {
Expand Down Expand Up @@ -68,11 +68,10 @@ describe('Portfolio', () => {
test('fails portfolio creation with long name', () => {
expect(() => {
new servicecatalog.Portfolio(stack, 'MyPortfolio', {
displayName: 'DisplayName',
displayName: 'DisplayName'.repeat(1000),
providerName: 'testProvider',
description: 'A portfolio for some products'.repeat(1000),
});
}).toThrowError(/Invalid portfolio description for resource Default\/MyPortfolio/);
}).toThrowError(/Invalid portfolio display name for resource Default\/MyPortfolio/);
}),

test('fails portfolio creation with invalid provider name', () => {
Expand All @@ -94,8 +93,54 @@ describe('Portfolio', () => {
description: description,
});
}).toThrowError(/Invalid portfolio description for resource Default\/MyPortfolio/);
}),

test('portfolio creation with token description does not throw validation error and creates', () => {
const tokenDescription = new cdk.CfnParameter(stack, 'Description');

new servicecatalog.Portfolio(stack, 'MyPortfolio', {
displayName: 'testPortfolio',
providerName: 'testProvider',
description: tokenDescription.valueAsString,
});

expect(stack).toHaveResourceLike('AWS::ServiceCatalog::Portfolio', {
Description: {
Ref: 'Description',
},
});
}),

test('portfolio creation with token display name does not throw validation error and creates', () => {
const tokenDisplayName = new cdk.CfnParameter(stack, 'DisplayName');

new servicecatalog.Portfolio(stack, 'MyPortfolio', {
displayName: tokenDisplayName.valueAsString,
providerName: 'testProvider',
});

expect(stack).toHaveResourceLike('AWS::ServiceCatalog::Portfolio', {
DisplayName: {
Ref: 'DisplayName',
},
});
}),

test('portfolio creation with token provider name does not throw validation error and creates', () => {
const tokenProviderName = new cdk.CfnParameter(stack, 'ProviderName');

new servicecatalog.Portfolio(stack, 'MyPortfolio', {
displayName: 'testPortfolio',
providerName: tokenProviderName.valueAsString,
});

expect(stack).toHaveResourceLike('AWS::ServiceCatalog::Portfolio', {
ProviderName: {
Ref: 'ProviderName',
},
});
});
}),
});

describe('portfolio methods and associations', () => {
let portfolio: servicecatalog.Portfolio;
Expand All @@ -108,8 +153,8 @@ describe('Portfolio', () => {
});

test('portfolio with tags', () => {
Tags.of(portfolio).add('myTestKey1', 'myTestKeyValue1');
Tags.of(portfolio).add('myTestKey2', 'myTestKeyValue2');
cdk.Tags.of(portfolio).add('myTestKey1', 'myTestKeyValue1');
cdk.Tags.of(portfolio).add('myTestKey2', 'myTestKeyValue2');

expect(stack).toHaveResourceLike('AWS::ServiceCatalog::Portfolio', {
Tags: [
Expand Down

0 comments on commit ce727c1

Please sign in to comment.