-
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
feat(apigatewayv2): http api - jwt and cognito user pool authorizers #10972
Conversation
Part 1 of the work needed to make #10534 happen.
@shivlaks Can you take a look at this? |
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.
Thanks for submitting this PR! Please find my first round of comments below -
Also not sure if a partial L2 is possible as I didn't implement all the properties.
It's not necessary to implement all properties. We're ok with taking in PRs that don't implement all functionality.
However, it should be possible to set a very minimal authorizer up. It seems in your case, customers won't be able to set the authorizer up since they can't yet connect the authorizer to the route. This is necessary in this PR.
Also not sure if I should rename it to just Authorizer (wasn't sure if it clashed with v1 authorizers)
They're in separate modules. No collision.
@nija-at authorizerType is needed by CfnRouteProps. What's the best way to make this available? I was thinking to expose |
@iRoachie Thanks for this! Will it be possible for you to add an integration test for this? |
Just for reference, this is PR has in more changes now
|
@ayush987goyal Sureeeee as soon as I find out how to do so |
How would I go about doing this? First time having to do an integration test |
You can take a look to the v1 authorizer integration test here. Also read the contribution guide around integration tests. |
You can define this within each type of authorizer and access it within the |
@ayush987goyal I made and ran the integration test. Let me know if I need to change anything |
@nija-at Lemme know if everything is good after regarding your review |
Okay outside of tests and docs, let me know if there's anything in the implementation that needs changing. |
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.
Thanks for picking this back up. Took a quick scan through the code.
Remove fromHttpJwtAuthorizerId from HttpJwtAuthorizer and implement on the base HttpAuthorizer class
@nija-at changes made |
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.
@iRoachie - the build is failing. can you take a look?
Besides that, just small updates to the README. Otherwise, looks good.
Co-authored-by: Niranjan Jayakar <nija@amazon.com>
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Finally green 😅 needed to add the Authorizer package to monocdk, decdk and aws-cdk-lib. |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…ws#10972) Part 1 of the work needed to make aws#10534 happen. - Also not sure if a partial L2 is possible as I didn't implement all the properties. - Also not sure if I should rename it to just `Authorizer` (wasn't sure if it clashed with v1 authorizers) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Part 1 of the work needed to make #10534 happen.
Authorizer
(wasn't sure if it clashed with v1 authorizers)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license