feat(apigateway): lambda token authorizer #5197

nija-at · 2019-11-26T14:36:11Z

Authorizers provide functionality to manage and control access to
specific or all methods on a RestApi endpoint.

The lambda token authorizer expects the authorization token to be part
of the request's header and passes this to a lambda function that can
then either allow or deny access the requester access to the resource.

https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-use-lambda-authorizer.html

closes #1402

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

mergify · 2019-11-26T14:37:28Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-11-26T15:05:14Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

PR title: we normally don't use the term L2 formally because everything we implement is effectively at the L2 layer. Therefore I would change the title to feat(apigateway): lambda token authorizers.

packages/@aws-cdk/aws-apigateway/README.md

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

packages/@aws-cdk/aws-apigateway/lib/method.ts

packages/@aws-cdk/aws-apigateway/package.json

packages/@aws-cdk/aws-apigateway/test/authorizers/integ.token-authorizer.handler/index.ts

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

eladb

See my comment under this: https://github.com/aws/aws-cdk/pull/5197/files#r350887652

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

packages/@aws-cdk/aws-apigateway/package.json

eladb · 2019-11-27T12:45:28Z

packages/@aws-cdk/aws-apigateway/lib/authorizer.ts

+  /**
+   * Returns a lazy that will resolve to the restApiId at the time of synthesis.
+   */
+  protected get restApiId(): string {


OK, I think the base class is basically not needed. The IAuthorizer interface should implement the "bind" pattern (like lambda.Code) and allow derived classes to access the RestApi object. There is no need to store it at the base class.

This is also inline with my comment below about authorizers reporting their own type, which will make the API much cleaner.

So instead of:

authorizationType: apigateway.AuthorizationType.CUSTOM, authorizer: auth

The eventual API I would envision is:

authorizer: apigw.Authorizer.custom(lambdaHandler, { /* ... */ });

Or for IAM:

authorizer: apigw.Authorizer.iam()

aws-cdk-automation · 2019-11-27T17:57:35Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-11-27T17:57:53Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

Still think we should do Authorizer.token

packages/@aws-cdk/aws-apigateway/lib/authorizer.ts

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

…iam() experience

aws-cdk-automation · 2019-11-29T11:27:51Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

packages/@aws-cdk/aws-apigateway/lib/authorizer.ts

packages/@aws-cdk/aws-apigateway/lib/authorizers/authorizer-base.ts

eladb · 2019-12-12T07:29:54Z

packages/@aws-cdk/aws-apigateway/lib/authorizers/authorizer-base.ts

+   * Check if the given object is of type CustomAuthorizer
+   * @internal
+   */
+  public static _isAuthorizer(x: any): x is AuthorizerBase {


Why is this internal? Wouldn't users want this functionality as well?

packages/@aws-cdk/aws-apigateway/lib/authorizers/authorizer-base.ts

eladb · 2019-12-12T07:31:33Z

packages/@aws-cdk/aws-apigateway/lib/authorizers/authorizer-base.ts

+   * The authorizer ID.
+   * @attribute
+   */
+  public abstract readonly authorizerId: string;


This seems like a duplication, no? Also available in AuthorizerConfig...

Required since this is a subclass of IAuthorizer. Typescript enforces that all abstract subclasses either implement abstract methods and properties, or re-indicate them as being abstract.

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

packages/@aws-cdk/aws-apigateway/test/authorizers/test.lambda.ts

mergify · 2019-12-16T12:33:45Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:00:59Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:53:14Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:38:57Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:52:39Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:17:42Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

* Authorizer -> Authorization * drop using Physical Name

aws-cdk-automation · 2019-12-19T13:43:55Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-19T14:25:25Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-30T13:59:57Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

* 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>

aws-cdk-automation · 2020-01-02T10:09:59Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-01-02T10:35:58Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

A few comments were marked as resolved but the code still wasn't changed. Please make sure to only resolve comments that are applied.

packages/@aws-cdk/aws-apigateway/README.md

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts

packages/@aws-cdk/aws-apigateway/package.json

eladb · 2020-01-02T12:14:59Z

packages/@aws-cdk/aws-apigateway/test/test.restapi.ts

@@ -586,23 +586,23 @@ export = {
      ResourceId: { Ref: "myapichildA0A65412" },
      Integration: { Type: 'AWS' },
      AuthorizerId: 'AUTHID',
-      AuthorizationType: 'COGNITO_USER_POOLS',


Why was this test modified?

CF doc for Method states in the the AuthorizationType property section - "If you specify the AuthorizerId property, specify CUSTOM for this property. "

The original change contained validation to this effect and hence the test needed to be modified.

Since then, I think there is a documentation error here because the rule is not true when using cognito for authentication. In this case, both the AuthenticationType property on the Method and type property on the Authorizer should be COGNITO_USER_POOL.

I've reverted changes to this test file. At some point in the future, we might need additional validation, but I'd rather not mix this up with this (already long) PR.

packages/@aws-cdk/aws-apigateway/test/test.restapi.ts

nija-at · 2020-01-02T14:35:16Z

A few comments were marked as resolved but the code still wasn't changed. Please make sure to only resolve comments that are applied.

I am in the process of modifying the PR. The resolved comments have been addressed locally on my workspace, but the code is not ready to be pushed yet.
Resolving comments and pushing code updates cannot be performed atomically. Once it's all ready for another round of review, I will either 're-request review' or 'dismiss review'.

Code updated

eladb

Please provide a bit more detail in the PR description

aws-cdk-automation · 2020-01-02T15:53:03Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-02T15:53:42Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2020-01-02T16:28:23Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-01-02T16:28:48Z

Thank you for contributing! Your pull request is now being automatically merged.

netroy · 2020-01-07T11:57:38Z

@nija-at @eladb
looks like Authorizer name isn't optional.
CFN docs seem inconsistent with the APIGateway API docs (both v1 & v2).

This is breaking the aws-apigateway integration tests as well.
another related issue: #5678

I can send a quick PR to make either make name required, or define a default.
What would you prefer?

nija-at · 2020-01-08T10:54:49Z

Internal reference: issues/CFN-30099

feat(apigateway): L2 support for lambda token authorizers

2c1bbda

nija-at requested review from RomainMuller, rix0rrr and eladb November 26, 2019 14:36

nija-at self-assigned this Nov 26, 2019

mergify bot added the contribution/core This is a PR that came from AWS. label Nov 26, 2019

nija-at mentioned this pull request Nov 26, 2019

[apigateway] add support for lambda token authorizer #1402

Closed

nija-at changed the title ~~feat(apigateway): L2 support for lambda token authorizers~~ feat(apigateway): l2 support for lambda token authorizers Nov 26, 2019

eladb suggested changes Nov 26, 2019

View reviewed changes

nija-at changed the title ~~feat(apigateway): l2 support for lambda token authorizers~~ feat(apigateway): lambda token authorizers Nov 26, 2019

nija-at changed the title ~~feat(apigateway): lambda token authorizers~~ feat(apigateway): lambda token authorizer Nov 26, 2019

eladb suggested changes Nov 27, 2019

View reviewed changes

Niranjan Jayakar and others added 2 commits November 27, 2019 17:33

Address PR comments

69473d3

Merge branch 'master' into nija-at/apigateway-lambdaauthorizer

6b69b57

eladb suggested changes Nov 27, 2019

View reviewed changes

packages/@aws-cdk/aws-apigateway/lib/authorizer.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-apigateway/lib/authorizers/lambda.ts Outdated Show resolved Hide resolved

Niranjan Jayakar added 3 commits November 28, 2019 14:50

More PR feedback

4143ec3

Restructure binding

3188df6

Restructuring classes to allow for Authorizer.token() and Authorizer.…

8b9d8e7

…iam() experience

eladb reviewed Dec 12, 2019

View reviewed changes

PR feedback

845f31e

* Authorizer -> Authorization * drop using Physical Name

Niranjan Jayakar added 2 commits December 19, 2019 13:54

Merge branch 'master' into apigateway-lambdaauthorizer

092250d

Switch to eslint recommended import style

0e908a8

Merge branch 'master' into nija-at/apigateway-lambdaauthorizer

2dd3e72

Elad Ben-Israel and others added 2 commits January 2, 2020 10:07

Merge branch 'master' into nija-at/apigateway-lambdaauthorizer

16c37fe

eladb previously requested changes Jan 2, 2020

View reviewed changes

Documentation updates & PR feedback

bf58683

nija-at requested a review from eladb January 2, 2020 15:25

nija-at removed request for RomainMuller and rix0rrr January 2, 2020 15:27

eladb approved these changes Jan 2, 2020

View reviewed changes

nija-at added pr/do-not-merge This PR should not be merged at this time. and removed pr/do-not-merge This PR should not be merged at this time. labels Jan 2, 2020

Merge branch 'master' into nija-at/apigateway-lambdaauthorizer

0bbb279

mergify bot merged commit 5c16744 into master Jan 2, 2020

mergify bot deleted the nija-at/apigateway-lambdaauthorizer branch January 2, 2020 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apigateway): lambda token authorizer #5197

feat(apigateway): lambda token authorizer #5197

nija-at commented Nov 26, 2019 •

edited

Loading

mergify bot commented Nov 26, 2019

aws-cdk-automation commented Nov 26, 2019

eladb left a comment

eladb left a comment

eladb Nov 27, 2019

aws-cdk-automation commented Nov 27, 2019

aws-cdk-automation commented Nov 27, 2019

eladb left a comment

aws-cdk-automation commented Nov 29, 2019

eladb Dec 12, 2019

eladb Dec 12, 2019

nija-at Dec 17, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 19, 2019

aws-cdk-automation commented Dec 19, 2019

aws-cdk-automation commented Dec 30, 2019

aws-cdk-automation commented Jan 2, 2020

aws-cdk-automation commented Jan 2, 2020

eladb left a comment

eladb Jan 2, 2020

nija-at Jan 2, 2020

nija-at commented Jan 2, 2020

eladb left a comment

aws-cdk-automation commented Jan 2, 2020

mergify bot commented Jan 2, 2020

aws-cdk-automation commented Jan 2, 2020

mergify bot commented Jan 2, 2020

netroy commented Jan 7, 2020

nija-at commented Jan 8, 2020

feat(apigateway): lambda token authorizer #5197

feat(apigateway): lambda token authorizer #5197

Conversation

nija-at commented Nov 26, 2019 • edited Loading

mergify bot commented Nov 26, 2019

aws-cdk-automation commented Nov 26, 2019

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

eladb Nov 27, 2019

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 27, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 27, 2019

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 29, 2019

AWS CodeBuild CI Report

eladb Dec 12, 2019

Choose a reason for hiding this comment

eladb Dec 12, 2019

Choose a reason for hiding this comment

nija-at Dec 17, 2019

Choose a reason for hiding this comment

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 19, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 19, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 30, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

eladb Jan 2, 2020

Choose a reason for hiding this comment

nija-at Jan 2, 2020

Choose a reason for hiding this comment

nija-at commented Jan 2, 2020

eladb left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 2, 2020

aws-cdk-automation commented Jan 2, 2020

AWS CodeBuild CI Report

mergify bot commented Jan 2, 2020

netroy commented Jan 7, 2020

nija-at commented Jan 8, 2020

nija-at commented Nov 26, 2019 •

edited

Loading