fix: fix appsync permission assignment from functions #5342

r0zar · 2020-09-16T21:25:06Z

Issue #, if available:
4306

Description of changes:
The current implementation of appsync permissions is using an incorrect understanding of IAM policies for AppSync. This PR updates the IAM Policy generation process for lambdas from amplify function create/update walkthroughs.

This PR is ready for merging. 🚀

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

jamesonwilliams · 2020-09-16T21:31:25Z

PHAL @aws-amplify/amplify-cli (@kaustavghosh06 @renebrandel @UnleashedMind @yuth @attilah)

codecov · 2020-09-16T21:37:12Z

Codecov Report

Merging #5342 (e8b97a3) into master (473bea5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master    #5342    +/-   ##
========================================
  Coverage   57.05%   57.05%            
========================================
  Files         468      468            
  Lines       21489    21490     +1     
  Branches     4273     4058   -215     
========================================
+ Hits        12261    12262     +1     
- Misses       8349     8369    +20     
+ Partials      879      859    -20

Impacted Files	Coverage Δ
...amplify-cli-core/src/feature-flags/featureFlags.ts	`83.10% <100.00%> (+0.07%)`	⬆️
packages/amplify-util-mock/src/api/api.ts	`0.00% <0.00%> (ø)`
packages/graphql-mapping-template/src/print.ts	`35.29% <0.00%> (ø)`
packages/amplify-util-mock/src/storage/storage.ts	`0.00% <0.00%> (ø)`
...ges/amplify-util-mock/src/CFNParser/stack/index.ts	`0.00% <0.00%> (ø)`
...es/amplify-util-mock/src/api/resolver-overrides.ts	`0.00% <0.00%> (ø)`
...es/graphql-transformer-core/src/stripDirectives.ts	`35.29% <0.00%> (ø)`
.../amplify-cli-core/src/state-manager/pathManager.ts	`67.85% <0.00%> (ø)`
.../amplify-util-mock/src/utils/lambda/loadMinimal.ts	`0.00% <0.00%> (ø)`
...graphql-transformer-core/src/TransformerContext.ts	`25.09% <0.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 206ab32...e8b97a3. Read the comment docs.

renebrandel · 2020-09-16T23:19:06Z

hi @r0zar ! I'll try to get the team to review it on Friday. Thanks for starting this!

r0zar · 2020-10-02T10:44:41Z

@renebrandel 👋 Completed my final updates. This PR is ready for merging now AFAIK

renebrandel · 2020-10-02T18:18:36Z

@attilah will test this PR and see if we need to introduce a feature flag. At this point it looks good.

attilah · 2020-10-23T23:51:09Z

@r0zar Thanks for the PR, I went through it and I see the reason for the changes, but because of existing deployments we have to preserve the existing functionality so it needs to be put behind a feature flag that can be enabled by default for new projects and not for existing projects.

I have one concern about the generated IAM policy and that is that I can't see that the policy generation code includes the schema types. If I remember correctly you have to have access to the types as well, since if I have a policy for UpdatePost that does not implicitly grant me access to the Post type. This brings to the other thing I am missing is an E2E test which creates a function and an AppSync API and invokes a GraphQL operation.

Since this change has to go behind a feature flag we don't have to deal with the legacy permissions especially we don't have to map it.

If you need further help on the above suggested changes, ping me and I we can setup a meeting to get it through the finish line.

r0zar · 2020-10-23T23:58:48Z

Thanks for the review.

Yeah let's find some time next week so I get aligned on this and put together a todo list.

attilah · 2020-10-24T00:00:33Z

@r0zar sounds good, please test out the types/* policy I miss before that if you can.

attilah · 2020-10-30T23:29:40Z

Synced up offline with @r0zar will add the E2E tests and the Feature flag to preserve the existing functionality and will update the PR.

attilah · 2020-11-17T23:26:39Z

@r0zar how you see your time working on this, do you have an ETA (no rush) ?

r0zar · 2020-11-17T23:35:45Z

I finally had some time open up this week and was going to make a start on it. Hopefully I can knock it out in a few sessions.

attilah · 2020-11-18T02:46:16Z

@r0zar let me know if I can be any help.

r0zar · 2020-11-19T17:42:21Z

Ran a rebase to keep my changes.

r0zar · 2020-11-19T18:35:24Z

Ok, fixed the terrible rebase. 😅

packages/amplify-category-function/src/provider-utils/awscloudformation/utils/constants.ts

lgtm-com · 2020-11-19T22:25:12Z

This pull request introduces 3 alerts when merging 1c4309c into a7b7a8c - view on LGTM.com

new alerts:

3 for Expression has no effect

r0zar · 2020-11-20T02:57:52Z

🎉

attilah

Added a few comments on the recent changes, but overall almost there!

attilah · 2020-12-04T23:27:28Z

...tion/src/provider-utils/awscloudformation/service-walkthroughs/execPermissionsWalkthrough.ts

 import path from 'path';
 import * as TransformPackage from 'graphql-transformer-core';
-import _ from 'lodash';
-import { topLevelCommentPrefix, topLevelCommentSuffix, envVarPrintoutPrefix, CRUDOperation } from '../../../constants';
+import _, { get, capitalize, concat, isEmpty } from 'lodash';


Please use import _ from 'lodash'; and don't deconstruct its functions, it's more readable that way.

attilah · 2020-12-04T23:32:13Z

...ory-function/src/provider-utils/awscloudformation/service-walkthroughs/lambda-walkthrough.ts

@@ -60,7 +60,7 @@ export async function createWalkthrough(
  // list out the advanced settings before asking whether to configure them
  context.print.info('');
  context.print.success('Available advanced settings:');
-  advancedSettingsList.forEach(setting => context.print.info('- '.concat(setting)));
+  advancedSettingsList.forEach(setting => context.print.info(setting));


Why removed the '-' prefix? We rarely change prompts and DX without API reviews. Sync up on this with @renebrandel .

The - served as a pseudo-bullet-point in the unordered list of choices. I replaced that with the icons in the PR. It results in a more visually comprehensible list of choices that can be understood better internationally.

packages/amplify-category-function/src/provider-utils/awscloudformation/utils/constants.ts

attilah · 2020-12-04T23:34:12Z

...s/amplify-category-function/src/provider-utils/awscloudformation/utils/permissionMapUtils.ts

 import _ from 'lodash';

+const featureFlagPR5342 = () => FeatureFlags.getBoolean('useAppSyncPermissionsInLambdaPolicies')


Just name it normally as a variable, no need for GH reference, also FeatureFlags has a ., I'd suggest to use appSync.generateGraphQLPermissionsForCRUD since this feature flag is not for graphqlTransformer. We can debate the name but this is what it does IMHO, suggest something similar if you have a better in mind.

how do you feel about the category being iamPolicies? This distinction would be helpful to make clear it's related to how the policies are generated and managed.

attilah · 2020-12-04T23:39:22Z

...s/amplify-category-function/src/provider-utils/awscloudformation/utils/permissionMapUtils.ts

+  const permissions = _.get(permissionMap, [category, resourceName], []);
+  // If AppSync and legacy CRUD permissions BUT they opted into the feature flag, update policy.
+  if (serviceType == 'AppSync' && hasCRUDPermissions(permissions) && featureFlagPR5342()) {
+    return updatePermissionsForBackwardsCompatibility(permissionMap, category, resourceName);


Why is the FF branch of the if doing anything that is backward compatible? It should not, right?

I wrote it so that enabling a feature flag with incorrect permissions would autocorrect their policies. I now see that the way feature flags are being used in amplify is that by turning them on early, the user may deals with the consequences. I see this results in code with less 'artifacts' so I'll update to fit this design.

I moved the FF check to line 102 of execPermissionsWalkthrough.ts. Still exists as an artifact in the code, but there is no more autocorrecting of permissions. Is this the intention?

attilah · 2020-12-04T23:39:59Z

packages/amplify-cli-core/src/feature-flags/featureFlags.ts

@@ -504,6 +504,12 @@ export class FeatureFlags {
        defaultValueForExistingProjects: false,
        defaultValueForNewProjects: false,
      },
+      {
+        name: 'useAppSyncPermissionsInLambdaPolicies',


As suggested above it should go into a new appSync group.

lgtm-com · 2020-12-22T18:54:08Z

This pull request introduces 1 alert when merging c3b406d into 206ab32 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

r0zar · 2020-12-22T23:51:09Z

I squashed and force pushed the commit with only the relevant changes to keep the scope a little tighter.

r0zar · 2020-12-23T01:17:54Z

...tion/src/provider-utils/awscloudformation/service-walkthroughs/execPermissionsWalkthrough.ts

@@ -25,32 +26,31 @@ export const askExecRolePermissionsQuestions = async (
  category?: string,


This value seems to be coming in as undefined for most scenarios. Is it still used?

attilah

LGTM, thanks for hanging in there ;-)

Fixes test introduced by aws-amplify#5342

Fixes test introduced by #5342

github-actions · 2022-01-15T01:50:15Z

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 *-help channels for those types of questions.

attilah self-assigned this Oct 2, 2020

attilah self-requested a review October 2, 2020 18:19

r0zar force-pushed the bugfix/appsync-function-permissions branch from 01337da to 8810503 Compare November 19, 2020 18:34

jhockett reviewed Nov 19, 2020

View reviewed changes

packages/amplify-category-function/src/provider-utils/awscloudformation/utils/constants.ts Outdated Show resolved Hide resolved

attilah suggested changes Dec 4, 2020

View reviewed changes

This was referenced Dec 7, 2020

Function Auth Role - "Read" Permission does not give "appsync:GraphQL" access in IAM Role. #6093

Closed

Wrong Lambda Execution Role #4889

Closed

r0zar requested a review from a team as a code owner December 22, 2020 18:36

fix(amplify-category-function): update function to graphql permissions

24aa3b0

r0zar force-pushed the bugfix/appsync-function-permissions branch from c3b406d to 24aa3b0 Compare December 22, 2020 19:01

r0zar marked this pull request as draft December 23, 2020 00:28

r0zar commented Dec 23, 2020

View reviewed changes

fix(amplify-category-function): update category reference

e8b97a3

r0zar force-pushed the bugfix/appsync-function-permissions branch from 4f8ce5a to e8b97a3 Compare December 23, 2020 01:23

r0zar marked this pull request as ready for review December 23, 2020 01:26

r0zar requested review from attilah and removed request for a team December 23, 2020 01:26

attilah approved these changes Jan 14, 2021

View reviewed changes

attilah merged commit b2e2dd0 into aws-amplify:master Jan 14, 2021

ammarkarachi pushed a commit to ammarkarachi/amplify-cli that referenced this pull request Jan 19, 2021

test: fixes e2e function with api perm test

3e400cb

Fixes test introduced by aws-amplify#5342

ammarkarachi mentioned this pull request Jan 19, 2021

test: fixes e2e function with api perm test #6434

Merged

ammarkarachi pushed a commit to ammarkarachi/amplify-cli that referenced this pull request Jan 20, 2021

test: fixes e2e function with api perm test

84bbe58

Fixes test introduced by aws-amplify#5342

ammarkarachi added a commit that referenced this pull request Jan 20, 2021

test: fixes e2e function with api perm test (#6434)

df74e0f

Fixes test introduced by #5342

thibaultdalban mentioned this pull request Feb 11, 2021

regression on appsync permission assignment from functions #6595

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix appsync permission assignment from functions #5342

fix: fix appsync permission assignment from functions #5342

r0zar commented Sep 16, 2020 •

edited

Loading

jamesonwilliams commented Sep 16, 2020

codecov bot commented Sep 16, 2020 •

edited

Loading

renebrandel commented Sep 16, 2020

r0zar commented Oct 2, 2020

renebrandel commented Oct 2, 2020

attilah commented Oct 23, 2020

r0zar commented Oct 23, 2020

attilah commented Oct 24, 2020

attilah commented Oct 30, 2020

attilah commented Nov 17, 2020 •

edited

Loading

r0zar commented Nov 17, 2020

attilah commented Nov 18, 2020

r0zar commented Nov 19, 2020

r0zar commented Nov 19, 2020

lgtm-com bot commented Nov 19, 2020

r0zar commented Nov 20, 2020

attilah left a comment

attilah Dec 4, 2020

r0zar Dec 22, 2020

attilah Dec 4, 2020

r0zar Dec 22, 2020 •

edited

Loading

attilah Dec 4, 2020

r0zar Dec 22, 2020

attilah Dec 4, 2020

r0zar Dec 22, 2020

r0zar Dec 22, 2020

attilah Dec 4, 2020

lgtm-com bot commented Dec 22, 2020

r0zar commented Dec 22, 2020

r0zar Dec 23, 2020

attilah left a comment

github-actions bot commented Jan 15, 2022

		import _ from 'lodash';

		const featureFlagPR5342 = () => FeatureFlags.getBoolean('useAppSyncPermissionsInLambdaPolicies')

		@@ -25,32 +26,31 @@ export const askExecRolePermissionsQuestions = async (
		category?: string,

fix: fix appsync permission assignment from functions #5342

fix: fix appsync permission assignment from functions #5342

Conversation

r0zar commented Sep 16, 2020 • edited Loading

jamesonwilliams commented Sep 16, 2020

codecov bot commented Sep 16, 2020 • edited Loading

Codecov Report

renebrandel commented Sep 16, 2020

r0zar commented Oct 2, 2020

renebrandel commented Oct 2, 2020

attilah commented Oct 23, 2020

r0zar commented Oct 23, 2020

attilah commented Oct 24, 2020

attilah commented Oct 30, 2020

attilah commented Nov 17, 2020 • edited Loading

r0zar commented Nov 17, 2020

attilah commented Nov 18, 2020

r0zar commented Nov 19, 2020

r0zar commented Nov 19, 2020

lgtm-com bot commented Nov 19, 2020

r0zar commented Nov 20, 2020

attilah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0zar Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Dec 22, 2020

r0zar commented Dec 22, 2020

Choose a reason for hiding this comment

attilah left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 15, 2022

r0zar commented Sep 16, 2020 •

edited

Loading

codecov bot commented Sep 16, 2020 •

edited

Loading

attilah commented Nov 17, 2020 •

edited

Loading

r0zar Dec 22, 2020 •

edited

Loading