fix(apigatewayv2): defaultAuthorizer cannot be applied to HttpRoute #27576

go-to-k · 2023-10-17T14:41:40Z

This PR fixes a bug that defaultAuthorizer cannot be applied to HttpRoute without an authorizer.

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

…e is 0 is wrong

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev

Looks good overall 👍
Adding the properties to the IHttpApi interface results in a clearer implementation and it should not cause problems in my opinion.

lpizzinidev · 2023-10-23T13:32:01Z

packages/@aws-cdk/aws-apigatewayv2-alpha/lib/http/route.ts

+    if (props.authorizer) {
+      this.authBindResult = props.authorizer.bind({
+        route: this,
+        scope: this.httpApi instanceof Construct ? this.httpApi : this, // scope under the API if it's not imported
+      });
+    } else if (this.httpApi instanceof HttpApi && this.httpApi.defaultAuthorizer) { // because IHttpApi as it is does not have a defaultAuthorizer
+      this.authBindResult = this.httpApi.defaultAuthorizer.bind({
+        route: this,
+        scope: this.httpApi, // this.httpApi is also a Construct because it is an HttpApi
+      });
+    }


Suggested change

if (props.authorizer) {

this.authBindResult = props.authorizer.bind({

route: this,

scope: this.httpApi instanceof Construct ? this.httpApi : this, // scope under the API if it's not imported

});

} else if (this.httpApi instanceof HttpApi && this.httpApi.defaultAuthorizer) { // because IHttpApi as it is does not have a defaultAuthorizer

this.authBindResult = this.httpApi.defaultAuthorizer.bind({

route: this,

scope: this.httpApi, // this.httpApi is also a Construct because it is an HttpApi

});

}

const authorizer = props.authorizer ?? this.httpApi.defaultAuthorizer;

this.authBindResult = authorizer?.bind({

route: this,

scope: this.httpApi instanceof Construct ? this.httpApi : this, // scope under the API if it's not imported

});

It should be fine to add defaultAuthorizer and defaultAuthorizationScopes to the IHttpApi interface (unless I'm missing something).

lpizzinidev · 2023-10-23T13:33:19Z

packages/@aws-cdk/aws-apigatewayv2-alpha/lib/http/route.ts

+    if (this.authBindResult) {
+      if (props.authorizationScopes) {
+        authorizationScopes = Array.from(new Set([
+          ...authorizationScopes ?? [],
+          ...props.authorizationScopes,
+        ]));
+      } else if (this.httpApi instanceof HttpApi && this.httpApi.defaultAuthorizationScopes) {// because IHttpApi as it is does not have a defaultAuthorizationScopes
+        authorizationScopes = Array.from(new Set([
+          ...authorizationScopes ?? [],
+          ...this.httpApi.defaultAuthorizationScopes,
+        ]));
+      }


Suggested change

if (this.authBindResult) {

if (props.authorizationScopes) {

authorizationScopes = Array.from(new Set([

...authorizationScopes ?? [],

...props.authorizationScopes,

]));

} else if (this.httpApi instanceof HttpApi && this.httpApi.defaultAuthorizationScopes) {// because IHttpApi as it is does not have a defaultAuthorizationScopes

authorizationScopes = Array.from(new Set([

...authorizationScopes ?? [],

...this.httpApi.defaultAuthorizationScopes,

]));

}

if (this.authBindResult && (props.authorizationScopes || this.httpApi.defaultAuthorizationScopes)) {

authorizationScopes = Array.from(new Set([

...authorizationScopes ?? [],

...props.authorizationScopes ?? this.httpApi.defaultAuthorizationScopes ?? [],

]));

}

lpizzinidev · 2023-10-23T13:46:13Z

packages/@aws-cdk/aws-apigatewayv2-authorizers-alpha/test/http/integ.lambda.ts

+const httpApi = new HttpApi(stack, 'MyHttpApi');
+const httpApiWithDefaultAuthorizer = new HttpApi(stack, 'MyHttpApiWithDefaultAuthorizer', {
+  defaultAuthorizer,
+});


Can you also add defaultAuthorizationScopes to the httpApiWithDefaultAuthorizer API?

I changed integ.user-pool instead of integ.lambda because defaultAuthorizationScopes (Authorization Scopes) are only valid for COGNITO_USER_POOLS and JWT authorization types

👍 Thanks for the clarification.

go-to-k · 2023-10-23T15:42:38Z

@lpizzinidev

Thanks for your review. I changed, so please review again!

change an integ test

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev

Thanks 👍
Some documentation adjustments are needed.
Also, can you please add back the default authorizer in the lambda integration test?
Is good to have some extra coverage.

lpizzinidev · 2023-10-24T06:38:28Z

packages/@aws-cdk/aws-apigatewayv2-alpha/lib/http/api.ts

+  readonly defaultAuthorizer?: IHttpRouteAuthorizer;
+
+  /**
+   * Default OIDC scopes attached to all routes in the gateway, unless explicitly configured on the route.


Suggested change

* Default OIDC scopes attached to all routes in the gateway, unless explicitly configured on the route.

* Default OIDC scopes attached to all routes in the gateway, unless explicitly configured on the route.

* The scopes are used with a COGNITO_USER_POOLS authorizer to authorize the method invocation.

*

Can you please align this as well?

Please check.

I also added a JWT authorizer to this doc.

40819a1

Can you please provide documentation that describes the usage with a JWT authorizer?
I was only able to verify the COGNITO_USER_POOLS authorizer.

Oh... I tried in the integ.lambda with defaultAuthorizationScopes in httpApiWithDefaultAuthorizer, CFn occurred the following error message.

I'll see if I can find any documentation.

UPDATE_ROLLBACK_COMPLETE: Resource handler returned message: "Invalid Route authorization type specified. Authorization Scopes are only valid for COGNITO_USER_POOLS and JWT authorization types (Service: AmazonApiGatewayV2; Status Code: 400; Error Code: BadRequestException; Request ID: xxxxxx; Proxy: null)" (RequestToken: xxxxxx, HandlerErrorCode: GeneralServiceException)

There is If you specify the AuthorizerId property, specify CUSTOM or COGNITO_USER_POOLS for this property. in the CFn doc.

I guess this CFn message represents ~~CUSTOM~~ COGNITO_USER_POOLS as JWT.

What should we do in CDK doc?

I found only this article.

https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-jwt-authorizer.html

I'm starting to feel that COGNITO_USER_POOLS is all I need.

Yeah, I think that COGNITO_USER_POOLS is sufficient since it generates a JWT authorizer.

In the meantime, please review again as I have added only COGNITO_USER_POOLS for the message.

lpizzinidev · 2023-10-24T06:39:11Z

packages/@aws-cdk/aws-apigatewayv2-alpha/lib/http/api.ts

+  /**
+   * Default Authorizer to applied to all routes in the gateway
+   * @attribute
+   * @default - No authorizer
+   */


Suggested change

/**

* Default Authorizer to applied to all routes in the gateway

* @attribute

* @default - No authorizer

*/

/**

* Default Authorizer applied to all routes in the gateway.

*

* @attribute

* @default - no default authorizer

*/

Can you please align this as well?

Please check.

40819a1

go-to-k · 2023-10-24T10:04:17Z

Also, can you please add back the default authorizer in the lambda integration test?

OK, I changed the integ.lambda and added snapshots.

lpizzinidev

Thanks 👍

sumupitchayan

Thanks for you contribution!

mergify · 2023-10-27T15:15:36Z

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

mergify · 2023-10-27T15:20:55Z

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

aws-cdk-automation · 2023-10-27T16:12:57Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 375b130
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-10-27T16:13:18Z

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

…27576) This PR fixes a bug that `defaultAuthorizer` cannot be applied to `HttpRoute` without an authorizer. Closes #27436. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

go-to-k added 2 commits October 17, 2023 15:46

docs(secretsmanager): doc when automaticallyAfter for RotationSchedul…

21785e2

…e is 0 is wrong

fix(apigatewayv2): can't apply defaultAuthorizer to HttpRoute

de950c7

github-actions bot added bug This issue is a bug. p2 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels Oct 17, 2023

aws-cdk-automation requested a review from a team October 17, 2023 14:41

aws-cdk-automation previously requested changes Oct 17, 2023

View reviewed changes

add an integ test

1efb3d6

go-to-k added 7 commits October 18, 2023 01:24

refactor for route.ts

1e10a0d

fix miss

21e4aee

change integ tests for circular dependency

ebacc8e

tweak

aa00748

tweak

afb9bda

tweak

df3155b

add comments

9062e3a

go-to-k marked this pull request as ready for review October 19, 2023 05:57

go-to-k changed the title ~~fix(apigatewayv2): can't apply defaultAuthorizer to HttpRoute~~ fix(apigatewayv2): defaultAuthorizer can not be applied to HttpRoute Oct 19, 2023

go-to-k changed the title ~~fix(apigatewayv2): defaultAuthorizer can not be applied to HttpRoute~~ fix(apigatewayv2): defaultAuthorizer cannot be applied to HttpRoute Oct 19, 2023

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 19, 2023

go-to-k added 4 commits October 19, 2023 15:55

apply defaultAuthorizationScopes

4fc75f4

change tests

5c2208f

change comments and an integ

81bcb9c

change integ snapshots

4e76464

lpizzinidev suggested changes Oct 23, 2023

View reviewed changes

add properties to IHttpApi

ae2a40c

change integs

2a76067

change an integ test

go-to-k force-pushed the fix/apgwv2-route-authorizer branch from 41ca1bc to 2a76067 Compare October 23, 2023 15:50

changed integ.lambda.ts

5df276c

go-to-k added 5 commits October 24, 2023 01:13

change integ.user-pool

e3e3f9e

change integ.lambda.ts

afd941c

change integ.lambda

5f248aa

change integ.lambda before modification

4e5942f

change integs

0a188ee

aws-cdk-automation previously requested changes Oct 23, 2023

View reviewed changes

change integ.user-pool

9e09c7c

lpizzinidev suggested changes Oct 24, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 24, 2023

go-to-k added 2 commits October 24, 2023 18:53

change align and docs

40819a1

change integ.lambda

ee11394

go-to-k added 2 commits October 24, 2023 22:14

change message

7ea3c89

tweak for msgs

0bb478e

lpizzinidev approved these changes Oct 24, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 24, 2023

sumupitchayan approved these changes Oct 27, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 27, 2023

Merge branch 'main' into fix/apgwv2-route-authorizer

375b130

mergify bot merged commit f397071 into aws:main Oct 27, 2023
10 checks passed

github-actions bot mentioned this pull request Nov 1, 2023

Monthly PR metrics report - October 2023 #27786

Closed

go-to-k deleted the fix/apgwv2-route-authorizer branch November 22, 2023 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(apigatewayv2): defaultAuthorizer cannot be applied to HttpRoute #27576

fix(apigatewayv2): defaultAuthorizer cannot be applied to HttpRoute #27576

go-to-k commented Oct 17, 2023 •

edited

Loading

aws-cdk-automation left a comment

lpizzinidev left a comment

lpizzinidev Oct 23, 2023

lpizzinidev Oct 23, 2023

lpizzinidev Oct 23, 2023

go-to-k Oct 23, 2023 •

edited

Loading

lpizzinidev Oct 24, 2023

go-to-k commented Oct 23, 2023

aws-cdk-automation left a comment

lpizzinidev left a comment

lpizzinidev Oct 24, 2023

go-to-k Oct 24, 2023

lpizzinidev Oct 24, 2023

go-to-k Oct 24, 2023 •

edited

Loading

go-to-k Oct 24, 2023 •

edited

Loading

go-to-k Oct 24, 2023 •

edited

Loading

go-to-k Oct 24, 2023

lpizzinidev Oct 24, 2023

go-to-k Oct 24, 2023 •

edited

Loading

lpizzinidev Oct 24, 2023

go-to-k Oct 24, 2023

go-to-k commented Oct 24, 2023

lpizzinidev left a comment

sumupitchayan left a comment

mergify bot commented Oct 27, 2023

mergify bot commented Oct 27, 2023

aws-cdk-automation commented Oct 27, 2023

mergify bot commented Oct 27, 2023

fix(apigatewayv2): defaultAuthorizer cannot be applied to HttpRoute #27576

fix(apigatewayv2): defaultAuthorizer cannot be applied to HttpRoute #27576

Conversation

go-to-k commented Oct 17, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go-to-k Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go-to-k commented Oct 23, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go-to-k Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

go-to-k Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

go-to-k Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go-to-k Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go-to-k commented Oct 24, 2023

lpizzinidev left a comment

Choose a reason for hiding this comment

sumupitchayan left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 27, 2023

mergify bot commented Oct 27, 2023

aws-cdk-automation commented Oct 27, 2023

AWS CodeBuild CI Report

mergify bot commented Oct 27, 2023

go-to-k commented Oct 17, 2023 •

edited

Loading

go-to-k Oct 23, 2023 •

edited

Loading

go-to-k Oct 24, 2023 •

edited

Loading

go-to-k Oct 24, 2023 •

edited

Loading

go-to-k Oct 24, 2023 •

edited

Loading

go-to-k Oct 24, 2023 •

edited

Loading