fix(auth): standardize CloudFormation trigger templates, prevent errors at runtime

[ctjlewis]

When using the "Add User to Group" functionality with amplify add auth, the sign-up process would throw 400 ("Invalid JSON") at the PostConfirmation Lambda, where the user was to be added to a given group (see #7179).

After working on this for a day or two and speaking with @edwardfoyle, it seemed there were two issues here:

1. The triggers (index-trigger.js in various template-dir/ directories) did not support async/await, which resulted in conflicts because many of these Lambdas used async/await syntax.

2. The top-level Lambda function in index-trigger.js, which loads various modules in the same directory (./verification-link.js), was not returning anything, which was what appears to actually have been causing the "Invalid JSON" error, as CloudFormation expects a response from async handlers. Sadly, this was present across all of the top-level auth CloudFormation triggers, which would imply that any amplify add auth stack with more than one "advanced feature" enabled would be broken by default. This appears to have been an issue for some time.

This PR addresses both issues. All of the hardcoded index-trigger.js files were manually overwritten to use the updated source, and return the event that triggered them.

The auth templates were refactored to remove redundant copies of templates which contributed to this issue (see below).

This issue causes runtime failures in Lambda for any user who enables more than one "advanced feature" in the Amplify CLI ("Add User to Group", etc.).

Description of changes

Before, each template directory had the same trigger-index.js copy-pasted into it. It had been edited in a few places to use the async handler format, and this inconsistency is what is causing this/these errors.

Now, there is a _default template, and if it is not overridden in the given template, it will pull from the default, meaning there will no longer be redundant/inconsistent copies of trigger-index.js, and they will all follow the async handler format.

Issue #, if available

See PR #7180
Closes #7179, closes #4341, #2735 (closed)

Description of how you validated changes

I created ctjlewis/amplify-dev to test the CLI for now; the relevant branch for this issue is @cloudformation-trigger.

The amplify/ directory was deleted, and then the updated amplify-dev used to add the Verification Link and Add User to Group features, pushed with amplify push, and worked as expected out-of-the-box; this can be found on the @cloudformation-triggers-patched branch. I plan to enable all other "advanced features" for auth with amplify update auth to test that their updated templates also work as expected.

Checklist

• PR description included
• yarn test passes
• Tests are changed or added
• Relevant documentation is changed or added (and PR referenced)

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

[ctjlewis]

This also explains why the "Add CAPTCHA" never worked for me, and always just let me through - as an async function called in a synchronous context, it would never throw because it would immediately resolve without being awaited.

[codecov-commenter]

Codecov Report

Merging #7219 (56c139d) into master (00d6676) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7219   +/-   ##
=======================================
  Coverage   51.79%   51.79%           
=======================================
  Files         547      547           
  Lines       27626    27626           
  Branches     5342     5342           
=======================================
  Hits        14308    14308           
  Misses      12326    12326           
  Partials      992      992           

---

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 753a737...56c139d. Read the comment docs.

[cjihrig]

Since this PR is migrating to async/await style, are the leftover callbacks still needed?

[ctjlewis]

I left it in for backwards compatibility. In the linked issues, the way we all worked around was with a context.done() call (though this breaks stacks with more than one handler, since it terminates the entire context), so there is utility in not breaking support if we can leave it in.

My understanding was the callback param was the async handler equivalent of resolving synchronously without context.done() - if someone passes the callback param, it should work as expected, seemed like a good idea to leave it in just for compatibility.

See that we actually do pass the callback param to each handler:

amplify-cli/packages/amplify-category-auth/provider-utils/awscloudformation/triggers/CreateAuthChallenge/function-template-dir/trigger-index.js

Line 56 in d89542f

await Promise.all(modules.map(module => module.handler(event, context, callback)));

If it's not passed, it doesn't make a difference.

[cjihrig]

I left it in for backwards compatibility.

My understanding is that this code is only generated once, and that users updating their projects will not regenerate it. If that's the case, then I think we should really discourage using both callbacks and async/await.

[ctjlewis]

I don't think it's immediately obvious what we should do here:

• If we remove it, we basically kill support for any old patterns (new projects will be generated with the new template but old patterns exist in guides and blog posts published on AWS), and we'd want to make it very clear to users that the handlers generated by the CLI will use the async/await form, throw any errors, return values directly.
• If we don't remove it, old code using the callback will still work as written, but we might not be able to guarantee execution order (I think?)

[cjihrig]

The more I think about this, the more I think it's a bad idea to intermingle callbacks and promises like this. For example, the trigger-index files will now be expecting the modules it invokes to return a value to indicate completion. However, if one of those modules does something like this, I think it could cause problems:

setTimeout(() => { callback(); }, 1000);
// Implicit return

[ctjlewis]

OK, since there were notes on the trigger template, which was pasted into 10 trigger template directories, rather than manually copy-paste the updates I just went ahead and added support for a default trigger template with ctjlewis#1, which involved very few changes.

addTrigger now looks for each templateFile in triggerDir, and if it does not find one, it looks for it in the default trigger template ../_default/ (relative to triggerDir). Defaults will be loaded, if missing, for any of the defined templateFiles:

amplify-cli/packages/amplify-cli/src/extensions/amplify-helpers/trigger-flow.ts

Line 75 in 63e647b

const templateFiles = ['trigger-index.js', 'package.json.ejs', 'event.json'];

---

Now there is one trigger-index.js and not 10 different versions, some async / some not. I will make the changes to the trigger-index.js default template.

[cjihrig]

Hi @ctjlewis. Just following up - are you planning to remove the callbacks in this PR?

[ctjlewis]

@cjihrig - Could you maybe make the change? I will be unavailable for the next few days. I would also add a comment to clarify it should not be used.

[ctjlewis]

@cjihrig Any updates?

[cjihrig]

@ctjlewis I also have not had time to work on this yet.

[ctjlewis]

@cjihrig This has been updated.

[cjihrig]

Can you remove callback.

[cjihrig]

Can you remove callback here.

[cjihrig]

Please remove callback.

[cjihrig]

Can you change the signature of this function to match the others.

[cjihrig]

Please remove callback.

[cjihrig]

Can you change the _default part of the file path. The leading underscore looks a bit off.

[cjihrig]

Suggested change

	const templateFiles = ['trigger-index.js', 'package.json.ejs', 'event.json'];
	const templateFiles = Object.keys(templateMap);

[cjihrig]

Same comment here about changing _default.

[cjihrig]

Could this be moved outside of the handler?

[cjihrig]

It looks like sourceFiles here is a relative path, but in the previous version, it was just the file's basename. Is that intentional?

[ctjlewis]

Yes. The comments could be updated for clarity, but it resolves each file to the default template if an override isn't found, and then resolves it relative to sourceRoot for the following call:

amplify-cli/packages/amplify-cli/src/extensions/amplify-helpers/trigger-flow.ts

Lines 99 to 103 in 56c139d

	functionTemplate: {
	sourceRoot,
	sourceFiles,
	destMap,
	},

[renebrandel]

Hi @ctjlewis - thank you again for this contribution and apologies on the turnaround time. I think we're in the final stretch of the PR here and we would be happy to finish this work during next Friday's bug bash session. Please let us know if you are okay with that.

[cjihrig]

Just a heads up, I'm going to start working on this today. Sorry for the delay.

[ctjlewis]

No worries. I just stopped using the product when I ran into this, like most other users in that thread.

And like I said, not you who should be apologizing.

[edwardfoyle]

one nit, otherwise LGTM

[fbdo]

Hi @edwardfoyle and @ctjlewis , for those (like me) that are already facing this issue, what would be steps to fix an existing project after the release of this PR?

[ctjlewis]

@fbdo - look for trigger-index.js in your amplify/ directory and update it with the new version:

amplify-cli/packages/amplify-category-auth/provider-utils/awscloudformation/triggers/function-template-dir/trigger-index.js

Lines 1 to 39 in 3e2e630

	/**
	* @fileoverview
	*
	* This CloudFormation Trigger creates a handler which awaits the other handlers
	* specified in the `MODULES` env var, located at `./${MODULE}`.
	*/
	/**
	* The names of modules to load are stored as a comma-delimited string in the
	* `MODULES` env var.
	*/
	const moduleNames = process.env.MODULES.split(',');
	/**
	* The array of imported modules.
	*/
	const modules = moduleNames.map(name => require(`./${name}`));
	/**
	* This async handler iterates over the given modules and awaits them.
	*
	* @see https://docs.aws.amazon.com/lambda/latest/dg/nodejs-handler.html#nodejs-handler-async
	*
	* @param {object} event
	*
	* The event that triggered this Lambda.
	*
	* @returns
	*
	* The handler response.
	*/
	exports.handler = async event => {
	/**
	* Instead of naively iterating over all handlers, run them concurrently with
	* `await Promise.all(...)`. This would otherwise just be determined by the
	* order of names in the `MODULES` var.
	*/
	await Promise.all(modules.map(module => module.handler(event)));
	return event;
	};

Various features (CAPTCHA, Add User to Group, etc.) also had their handlers updated, see the 24 changed files as part of this PR. They are relatively well-commented.

@renebrandel I would recommend that (even very light) documentation be added to the README / Amplify docs about how these triggers work. It should:

• prevent similar issues going forward
• help users self-resolve issues that are remain from this bug
• make it easier for the community to contribute fixes related to these triggers and associated logic

[github-actions]

👋 Hi, this pull request was referenced in the v5.1.2 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v5.1.2.