Skip to content

Commit

Permalink
chore: proposed refactor for authorizers design (#5584)
Browse files Browse the repository at this point in the history
* simplify authorizers class design

- rename `AuthorizerBase` to `Authorizer`. This class should actually have the `CfnAuthorizer` instantiation, but will only be introduced when an additional authorizer is included.
- simplify `AuthorizerBase` dramatically
- move logic to cache `restApiId` from `AuthorizerBase` to `TokenAuthorizer`. When an additional authorizer is added, we will refactor.
- remove the usage `Authorizer.token`. It is non-idiomatic in this context since we support one authorizer reused multiple times.

* moved Authorizer to authorizer.ts

* fix broken references and types

Co-authored-by: Niranjan Jayakar <16217941+nija-at@users.noreply.github.com>
  • Loading branch information
Elad Ben-Israel and nija-at committed Jan 2, 2020
1 parent 2dd3e72 commit 962039d
Show file tree
Hide file tree
Showing 9 changed files with 69 additions and 226 deletions.
34 changes: 21 additions & 13 deletions packages/@aws-cdk/aws-apigateway/lib/authorizer.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,20 @@
import { Construct } from '@aws-cdk/core';
import { AuthorizerBase, TokenAuthorizer, TokenAuthorizerProps } from './authorizers';
import { Resource } from '@aws-cdk/core';
import { AuthorizationType } from './method';
import { RestApi } from './restapi';

/**
* Base class for all custom authorizers
*/
export abstract class Authorizer extends Resource implements IAuthorizer {
public readonly abstract authorizerId: string;
public readonly authorizationType?: AuthorizationType = AuthorizationType.CUSTOM;

/**
* Called when the authorizer is used from a specific REST API.
* @internal
*/
public abstract _attachToApi(restApi: RestApi): void;
}

/**
* Represents an API Gateway authorizer.
Expand All @@ -10,17 +25,10 @@ export interface IAuthorizer {
* @attribute
*/
readonly authorizerId: string;
}

/**
* Base class for all custom authorizers
*/
export class Authorizer {

/**
* Return a new token authorizer with the specified properties.
* The required authorization type of this authorizer. If not specified,
* `authorizationType` is required when the method is defined.
*/
public static token(scope: Construct, id: string, props: TokenAuthorizerProps): AuthorizerBase {
return new TokenAuthorizer(scope, id, props);
}
}
readonly authorizationType?: AuthorizationType;
}

This file was deleted.

3 changes: 1 addition & 2 deletions packages/@aws-cdk/aws-apigateway/lib/authorizers/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
export * from './lambda';
export * from './authorizer-base';
export * from './lambda';
31 changes: 19 additions & 12 deletions packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import * as iam from '@aws-cdk/aws-iam';
import * as lambda from '@aws-cdk/aws-lambda';
import { Construct, Duration, Stack } from '@aws-cdk/core';
import { Construct, Duration, Lazy, Stack } from '@aws-cdk/core';
import { CfnAuthorizer } from '../apigateway.generated';
import { AuthorizationType, Method } from '../method';
import { AuthorizerBase, AuthorizerConfig } from './authorizer-base';
import { Authorizer, IAuthorizer } from '../authorizer';
import { RestApi } from '../restapi';

/**
* Properties for TokenAuthorizer
Expand Down Expand Up @@ -68,7 +68,7 @@ export interface TokenAuthorizerProps {
*
* @resource AWS::ApiGateway::Authorizer
*/
export class TokenAuthorizer extends AuthorizerBase {
export class TokenAuthorizer extends Authorizer implements IAuthorizer {

/**
* The id of the authorizer.
Expand All @@ -81,16 +81,20 @@ export class TokenAuthorizer extends AuthorizerBase {
*/
public readonly authorizerArn: string;

private restApiId?: string;

constructor(scope: Construct, id: string, props: TokenAuthorizerProps) {
super(scope, id);

if (props.resultsCacheTtl && props.resultsCacheTtl.toSeconds() > 3600) {
throw new Error(`Lambda authorizer property 'cacheTtl' must not be greater than 3600 seconds (1 hour)`);
}

const restApiId = Lazy.stringValue({ produce: () => this.restApiId });

const resource = new CfnAuthorizer(this, 'Resource', {
name: props.authorizerName,
restApiId: this.restApiId,
restApiId,
type: 'TOKEN',
authorizerUri: `arn:aws:apigateway:${Stack.of(this).region}:lambda:path/2015-03-31/functions/${props.handler.functionArn}/invocations`,
authorizerCredentials: props.assumeRole ? props.assumeRole.roleArn : undefined,
Expand All @@ -103,7 +107,7 @@ export class TokenAuthorizer extends AuthorizerBase {

this.authorizerArn = Stack.of(this).formatArn({
service: 'execute-api',
resource: this.restApiId,
resource: restApiId,
resourceName: `authorizers/${this.authorizerId}`
});

Expand All @@ -125,12 +129,15 @@ export class TokenAuthorizer extends AuthorizerBase {
}

/**
* Configuration needed to bind this authorizer to a {@link Method}.
* Attaches this authorizer to a specific REST API.
*
* @internal
*/
protected authorizerConfig(_: Method): AuthorizerConfig {
return {
authorizerId: this.authorizerId,
authorizationType: AuthorizationType.CUSTOM,
};
public _attachToApi(restApi: RestApi) {
if (this.restApiId && this.restApiId !== restApi.restApiId) {
throw new Error(`Cannot attach authorizer to two different rest APIs`);
}

this.restApiId = restApi.restApiId;
}
}
30 changes: 14 additions & 16 deletions packages/@aws-cdk/aws-apigateway/lib/method.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { Construct, Resource, Stack } from '@aws-cdk/core';
import { CfnMethod, CfnMethodProps } from './apigateway.generated';
import { IAuthorizer } from './authorizer';
import { AuthorizerBase } from './authorizers';
import { Authorizer, IAuthorizer } from './authorizer';
import { ConnectionType, Integration } from './integration';
import { MockIntegration } from './integrations/mock';
import { MethodResponse } from './methodresponse';
Expand All @@ -27,7 +26,7 @@ export interface MethodOptions {
* However, specifying an authorization type using this property that conflicts with what is expected by the {@link Authorizer}
* will result in an error.
*
* @default None open access
* @default - open access unless `authorizer` is specified
*/
readonly authorizationType?: AuthorizationType;

Expand Down Expand Up @@ -126,20 +125,19 @@ export class Method extends Resource {

const defaultMethodOptions = props.resource.defaultMethodOptions || {};
const authorizer = options.authorizer || defaultMethodOptions.authorizer;
const authorizerId = authorizer?.authorizerId;

let authorizationType;
let authorizerId;
if (AuthorizerBase._isAuthorizer(authorizer)) {
const authConfig = authorizer._bind(this);
if (options.authorizationType && options.authorizationType !== authConfig.authorizationType) {
throw new Error(`${this.resource}/${this.httpMethod} - Authorization type is set to ${options.authorizationType} ` +
`which is different from what is required by the authorizer [${authConfig.authorizationType}]`);
}
authorizationType = authConfig.authorizationType;
authorizerId = authConfig.authorizerId;
} else {
authorizationType = options.authorizationType || defaultMethodOptions.authorizationType || AuthorizationType.NONE;
authorizerId = authorizer ? authorizer.authorizerId : undefined;
const authorizationTypeOption = options.authorizationType || defaultMethodOptions.authorizationType;
const authorizationType = authorizer?.authorizationType || authorizationTypeOption || AuthorizationType.NONE;

// if the authorizer defines an authorization type and we also have an explicit option set, check that they are the same
if (authorizer?.authorizationType && authorizationTypeOption && authorizer?.authorizationType !== authorizationTypeOption) {
throw new Error(`${this.resource}/${this.httpMethod} - Authorization type is set to ${authorizationTypeOption} ` +
`which is different from what is required by the authorizer [${authorizer.authorizationType}]`);
}

if (authorizer instanceof Authorizer) {
authorizer._attachToApi(this.restApi);
}

const methodProps: CfnMethodProps = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as iam from '@aws-cdk/aws-iam';
import * as lambda from '@aws-cdk/aws-lambda';
import { App, Stack } from '@aws-cdk/core';
import * as path from 'path';
import { AuthorizationType, Authorizer, MockIntegration, PassthroughBehavior, RestApi } from '../../lib';
import { AuthorizationType, MockIntegration, PassthroughBehavior, RestApi, TokenAuthorizer } from '../../lib';

// Against the RestApi endpoint from the stack output, run
// `curl -s -o /dev/null -w "%{http_code}" <url>` should return 401
Expand All @@ -22,7 +22,7 @@ const role = new iam.Role(stack, 'authorizerRole', {
assumedBy: new iam.ServicePrincipal('apigateway.amazonaws.com')
});

const authorizer = Authorizer.token(stack, 'MyAuthorizer', {
const authorizer = new TokenAuthorizer(stack, 'MyAuthorizer', {
handler: authorizerFn,
assumeRole: role,
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as lambda from '@aws-cdk/aws-lambda';
import { App, Stack } from '@aws-cdk/core';
import * as path from 'path';
import { AuthorizationType, Authorizer, MockIntegration, PassthroughBehavior, RestApi } from '../../lib';
import { MockIntegration, PassthroughBehavior, RestApi, TokenAuthorizer } from '../../lib';

// Against the RestApi endpoint from the stack output, run
// `curl -s -o /dev/null -w "%{http_code}" <url>` should return 401
Expand All @@ -19,7 +19,7 @@ const authorizerFn = new lambda.Function(stack, 'MyAuthorizerFunction', {

const restapi = new RestApi(stack, 'MyRestApi');

const authorizer = Authorizer.token(stack, 'MyAuthorizer', {
const authorizer = new TokenAuthorizer(stack, 'MyAuthorizer', {
handler: authorizerFn,
});

Expand All @@ -35,6 +35,5 @@ restapi.root.addMethod('ANY', new MockIntegration({
methodResponses: [
{ statusCode: '200' }
],
authorizer,
authorizationType: AuthorizationType.CUSTOM
authorizer
});

This file was deleted.

Loading

0 comments on commit 962039d

Please sign in to comment.