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(apigateway): authorization scope is not added when not explicitly defining authorization type in method or root api #30822

Merged
merged 4 commits into from
Jul 11, 2024

Conversation

GavinZZ
Copy link
Contributor

@GavinZZ GavinZZ commented Jul 10, 2024

Issue # (if applicable)

Closes #30444

Reason for this change

The original PR caused a breaking change, we can't rollback because it was released in v2.142.0 and it fixes customers issues (partially). Simply doing a revert will be breaking for those customers again.

Description of changes

Identified the root cause and we should use AuthorizationType instead of AuthorizationTypeOption. AuthorizationType defaults to find the authorization type from the authorizer, falling back to use the auth type defined in the Method construct's options property and falling back to None.

AuthorizationTypeOptions on the other hand tries to find the auth type from Method construct's options property which can be None because it's optional.

Description of how you validated changes

New unit tests covering the changes and new integration tests covering it.

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team July 10, 2024 20:18
@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p0 labels Jul 10, 2024
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 10, 2024
@GavinZZ GavinZZ marked this pull request as ready for review July 10, 2024 20:32
@GavinZZ GavinZZ added the pr/do-not-merge This PR should not be merged at this time. label Jul 10, 2024
@moelasmar moelasmar self-requested a review July 10, 2024 23:13
@GavinZZ GavinZZ changed the title revert: fix(apigateway): set authorization scope when authorization type is Cognito fix(apigateway): authorization scope is not added when not explicitly defining authorization type in method or root api Jul 10, 2024
@GavinZZ
Copy link
Contributor Author

GavinZZ commented Jul 10, 2024

@mergify update

Copy link
Contributor

mergify bot commented Jul 10, 2024

update

✅ Branch has been successfully updated

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 231315f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@GavinZZ GavinZZ merged commit e4367a0 into main Jul 11, 2024
9 of 10 checks passed
@GavinZZ GavinZZ deleted the yuanhaoz/revert_api_scope branch July 11, 2024 00:52
moelasmar pushed a commit that referenced this pull request Jul 11, 2024
… defining authorization type in method or root api (#30822)

### Issue # (if applicable)

Closes #30444

### Reason for this change

The original PR caused a breaking change, we can't rollback because it
was released in v2.142.0 and it fixes customers issues (partially).
Simply doing a revert will be breaking for those customers again.

### Description of changes

Identified the root cause and we should use `AuthorizationType` instead
of `AuthorizationTypeOption`. `AuthorizationType` defaults to find the
authorization type from the authorizer, falling back to use the auth
type defined in the `Method` construct's options property and falling
back to `None`.

`AuthorizationTypeOptions` on the other hand tries to find the auth type
from `Method` construct's options property which can be None because
it's optional.

### Description of how you validated changes

New unit tests covering the changes and new integration tests covering
it.

### Checklist
- [ ] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@aws-cdk-automation
Copy link
Collaborator

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p0 pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

❗ NOTICE (aws-apigateway): Authorization Scopes not rendered with CognitoUserPoolsAuthorizer
3 participants