-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Lambda authorizers support (API, DataStore, PubSub) #8616
Conversation
646c831
to
e59299b
Compare
This pull request introduces 3 alerts when merging e59299b into 85e9b97 - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## main #8616 +/- ##
==========================================
+ Coverage 77.80% 77.83% +0.02%
==========================================
Files 240 240
Lines 17122 17161 +39
Branches 3651 3664 +13
==========================================
+ Hits 13322 13357 +35
- Misses 3675 3679 +4
Partials 125 125
Continue to review full report at Codecov.
|
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! Super clean, great work @iartemiev 🥂 Would love to learn more about the use cases and can't wait to try this out once it's available.
@@ -51,6 +57,11 @@ function getAuthRules({ | |||
|
|||
rules.forEach(rule => { | |||
switch (rule.allow) { | |||
case 'custom': | |||
if (rule.provider === 'function') { |
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 curious about the other use cases for rule.provider
? When would it not be a function?
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.
For this new custom
auth strategy, function
is the only valid provider at the moment, but we may add additional ones in the future. I added the if
statement there to make it clear where to handle a new provider if/when we extend it down the road
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.
Nice! Very scalable and forward-thinking, great stuff Ivan 😊🚀
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.
No blockers that I can see.
One thing I'd note overall, in places where we've created constants like ModelAttributeAuthProvider.FUNCTION
, we really ought to be using them for the purposes of maintainability, semantic clarity, and fat-finger proofing.
} | ||
}`; | ||
const getEventDoc = parse(GetEvent); | ||
const getEventQuery = print(getEventDoc); |
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 DRYing!
d3e32e0
to
7aeacde
Compare
Note: I'm going to do a normal PR merge instead of squashing in this case because I want API, PubSub, and DataStore to all get minor version bumps from this PR. I've rebased/fixed up the previous commit messages into 3 commits total (one per category), so the commit history will remain clean and relevant. |
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
note: opening this as a branch in upstream in order to run e2e tests.
Description of changes
Adds support for Lambda Authorizers for AppSync APIs to the API category and DataStore.
API for leveraging Lambda auth
API Category
DataStore
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.