-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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: Add a built-in AWS_IAM authorizer for HTTP APIs #1924
feat: Add a built-in AWS_IAM authorizer for HTTP APIs #1924
Conversation
@harrisonhjones I think this is probably the best option from the 3 you have proposed. I didn't do a deep review of the code, but I think the setup you have is good. I am having another member of my team (@hawflau) look at this as well, just so we have some agreement on the approach. |
@harrisonhjones This option looks good to me. |
👍
I agree.
Got it. Will add tests.
Where do I add that? In this package or elsewhere? I see an "Update Documentation" checkbox but after a quick scan of the repo I don't see how documentation is supposed to be updated. |
2/18/2021 Update:
|
Codecov Report
@@ Coverage Diff @@
## develop #1924 +/- ##
===========================================
- Coverage 93.86% 93.82% -0.04%
===========================================
Files 89 89
Lines 5900 5913 +13
Branches 1208 1214 +6
===========================================
+ Hits 5538 5548 +10
- Misses 166 168 +2
- Partials 196 197 +1
Continue to review full report at Codecov.
|
I would need to double check, but I believe we generate docs based on https://github.com/aws/serverless-application-model/blob/develop/versions/2016-10-31.md. I will handle the communication with our doc writer about getting these things added, but updating that file will help in our process. |
Sorry about the late review @harrisonhjones! I have mixed feelings about adding a new property Couldn't we add a new |
@mgrandis The main reason @harrisonhjones and I landed on this design was because the IAM Auth is a static configuration. Forcing a customer to specify Do you still have concerns in this approach? |
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.
The looks good to me.
All right, let's move on with this approach. |
Any timeline for this feature being merged? |
@harrisonhjones It looks like there are only a few minor updates to do, do you have time to work on them? |
@mgrandis looks like the author isn't able to address this feedback, can we merge this and follow up with another PR? I would love to see this merged as it has been an issue for a while. |
Any timeline on when this PR will be merged? |
I can try to take a look at fixing this next week. Sorry for the delay getting back to y'all. |
Thank you!!!! This will be super helpful for me |
Codecov Report
@@ Coverage Diff @@
## develop #1924 +/- ##
===========================================
+ Coverage 93.58% 94.50% +0.91%
===========================================
Files 90 97 +7
Lines 6124 7073 +949
Branches 1260 1441 +181
===========================================
+ Hits 5731 6684 +953
+ Misses 183 177 -6
- Partials 210 212 +2
Continue to review full report at Codecov.
|
Thank you! Hopefully this gets merged! Really appreciate the effort. |
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.
Three small comments; two for more experienced Python folks.
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.
Just a couple small things.
Addressed PR feedback; added five new tests, removed an unnecessary error test, and updated and existing multi-auth test to include the new iam authorizer.
|
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 the contribution! One small question and everything else looks good to me.
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.
LGTM! Thanks for addressing the comments!
- Added tests - Added test-fast target - Updated documentation
56e8bd2
to
8e83fdb
Compare
Rebased off of develop, fixed merge conflicts, updated new util function that was breaking tests, squashed into a single commit, ran |
"components": { | ||
"securitySchemes": { | ||
"AWS_IAM": { | ||
"type": "oauth2", |
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.
IAM auth is oauth2 type?
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.
See the description in tests/translator/input/http_api_custom_iam_auth.yaml:
# This test-case tests what happens when an AWS_IAM authorizer is defined on an HttpApi but not enabled anywhere else.
# While the defined authorizer isn't really a true Iam authorizer (it's just a poorly-named OAuth authorizer) this shouldn't cause an error.
There are some things we need to handle internally (docs for example). I need to kick off that processes before merging. |
great work guys this is fantastic. any estimates as to when it's merged/released? I have a bit of a life/death situation here that would greatly benefit from this one |
I was just talking to my TAM about this. 👀 |
Issue #, if available: #1731
Description of changes:
This is another approach for adding AWS_IAM support to HTTP APIs created by SAM. Other approaches can be found in #1876 and #1878. This approach is based on a conversation with @jfuss.
In this version a new property is added to the Serverless HTTP API object called
EnableIamAuthorizer
. When set to true anAWS_IAM
authorizer is automatically added to the HTTP API's (internal) list of authorizers under the key "AWS_IAM". This authorizer is setup to us IAM auth. If enabled (It is not enabled by default) it can be set as the HTTP API's default authorizer and set as the authorizer for any Serverless functions that use the API.Description of how you validated changes:
I created the following test template:
I then compiled it using:
python ./bin/sam-translate.py --template-file=../project/template.yml
I then manually deployed it using CloudFormation in my personal AWS account.
Once deployed I validated all the routes had the expected auth type:
Default API
Custom API with no default auth
Custom API with default IAM auth
Checklist:
make pr
passesExamples?
Please reach out in the comments, if you want to add an example. Examples will be
added to
sam init
through https://github.com/awslabs/aws-sam-cli-app-templates/By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.