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

[WIP] Add support for new multi-auth AppSync directives #1524

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@attilah
Copy link
Collaborator

commented May 27, 2019

Issue #, if available:
Enables part of #1485

Description of changes:
Enable the usage of new AppSync new authorization directives in the GraphQL schema.

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

@undefobj undefobj requested review from mikeparisstuff and yuth May 29, 2019

@attilah attilah force-pushed the attilah:multiauth-passthrough branch from 5702bdb to c171dc0 Jun 3, 2019

@mikeparisstuff

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

This looks good as it relates to allowing the pass through but here is a question: Are these directives usable when there is currently no support to configure the "AdditionalAuthProviders" property on the GraphQLApi resource? If the directives are not really usable until we can make the updates that allow you configure the additional auth providers then I say we roll these out together.

@@ -128,6 +128,11 @@ test('Test custom root query, mutation, and subscriptions.', () => {
authedField: String
@aws_auth(cognito_groups: ["Bloggers", "Readers"])

This comment has been minimized.

Copy link
@mikeparisstuff

mikeparisstuff Jun 4, 2019

Collaborator

I don't believe this test is actually deployed through CFN and I believe this would fail since the server has validation that does not allow @aws_auth and @aws_cognito_user_pools to be used at the same time. Can we move multi auth tests to their own e2e test file that actually deploys to CFN so we can test the full e2e flow?

@attilah attilah changed the title Add support for new multi-auth AppSync directives [WIP] Add support for new multi-auth AppSync directives Jun 10, 2019

@attilah

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 10, 2019

Based on the comments/discussion I'm adding the additional functionality for multi auth to this PR.

@janhesters

This comment has been minimized.

Copy link

commented Jun 27, 2019

Will fine grained control be supported?

E.g. allow Read and Write access (Queries and Mutations) to I_AM and Read and Subscriptions (Queries and Subscriptions) to Cognito User Pools?

@houmark

This comment has been minimized.

Copy link

commented Jul 14, 2019

Any updates on this? Would be great to get this merged soon...

@attilah

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 14, 2019

@houmark this is actively worked on, bear with us please ;-)

@houmark

This comment has been minimized.

Copy link

commented Jul 14, 2019

Thanks for the update @attilah. I tried using the workarounds mentioned elsewhere by other AWS devs, by adding the @aws_iam directly in the AppSync schema, but I hit not only issues with my Cognito Pool not having access and the fact that one has to remember to add the @aws_iam in the cloud is a complete showstopper for a production app. Amplify Console runs automatically and will overwrite if there are schema changes — and even if you try to remember updating this manually, you'll still have seconds or minutes of downtime in parts or the entire app.

In the end, I gave up and went back to my stable Cognito User Pool systems user which I am already using in several Lambdas.

Will this also propagate the access to all related Connections etc. automatically or is the dev responsible for adding the directive in all places?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.