-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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): set authorization scope when authorization type is Cognito #30035
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally lgtm, just some clarifying questions
831322b
to
46ab0e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
This wiped our scopes, even though we have
This warning log was swallowed by CI/CD but still deployed
|
// When AuthorizationType is None, there shouldn't be any AuthorizationScope since AuthorizationScope should only | ||
// be applied to COGNITO_USER_POOLS AuthorizationType. | ||
const defaultScopes = options.authorizationScopes ?? defaultMethodOptions.authorizationScopes; | ||
const authorizationScopes = authorizationTypeOption === AuthorizationType.COGNITO ? defaultScopes : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By utilizing authorizationTypeOption instead of authorizationType, you are now forcing people to implement an optional parameter for something was previously set by the authorizer, that was previously recommended to not be set explicitly. It is explicitly stated in the documentation, that the authorizer will set the authorizationType, see line 30 of this file. This is a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this just broke our prod environment after a dependabot update - it should be marked as a breaking change 🙁
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. |
Issue # (if applicable)
Closes #29781
Reason for this change
Authorization scope is set even when the auth type is None. This will cause deployment failure
Description of changes
Check when auth type is not Cognito, set auth scope to none. Not a breaking change because original templates cannot deploy.
Description of how you validated changes
All existing and new tests pass.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license