aws-amplify / amplify-cli

The AWS Amplify CLI is a toolchain for simplifying serverless web and mobile development.
Apache License 2.0
2.81k stars 819 forks source link

cognito trigger postConfirmation template causes `Invalid lambda function output : Invalid JSON` for async handler, works after removing the generated code in index.js #4341

Closed Albert-Gao closed 3 years ago

Albert-Gao commented 4 years ago

CLI version: 4.20.0

I use Amplify CLI to generate a postConfirmation cognito trigger,

exports.handler = async (event) => {
  try {
    // doing some async stuff
  } catch (e) {
    console.log('error', JSON.stringify(e))
    throw e
  }
  console.log('event2', JSON.stringify(event))

  return event
}

Every time I confirmSignUp, I received a 400 code on the clientside Invalid lambda function output: Invalid JSON

Spent a whole night, then this morning I decide to remove all the code in the index.js generated by amplify-cli:

exports.handler = (event, context, callback) => {
  const modules = process.env.MODULES.split(',');
  for (let i = 0; i < modules.length; i += 1) {
    const { handler } = require(`./${modules[i]}`);
    handler(event, context, callback);
  }
};

As the line here worries me: handler(event, context, callback), as you can see it doesn't have the await tag when calling lambda, and I don't know why we need this code in the 1st place, it might be better to explain the why part at the top rather than the logic, the logic is straightforward to understand.

I completely removed this part, and moved my handler here, everything works like a charm now.

As a rough guess, I think the current for loop doesn't compatible with async handler.

Maybe we should just remove it, and replace it with whatever blahblah.js which is the main js the user needs to edit.

So:

  1. to prevent things like this to happen
  2. remove logic that is not user's concern unless it solves some problems (but i definitely don't need this when using serverless framework)
  3. the user does not need to figure out which .js file to edit.

Thanks :)

attilah commented 4 years ago

@Albert-Gao thanks for reporting we'll look into it.

UnleashedMind commented 4 years ago

The Amplify CLI generated Lambda function for auth triggers uses "Non-async" handler instead of the async handler, for details, see AWS Lambda doc.

It follow the module package folder structure

Screen Shot 2020-08-14 at 3 00 30 PM

In which index.js is the entry point, and it has the exported handler

Screen Shot 2020-08-14 at 3 02 57 PM

And custom.js file is the place your enter your code to handle the trigger, which will be invoked by the logic in the index.js This is the template generated by the CLI

exports.handler = (event, context, callback) => {
  // insert code to be executed by your lambda trigger
  callback(null, event);
};

Since this is a Non-async handler setup, the exported handler function in custom.js shouldn't be replaced it with an "Async" style handler, because then the index.js file cannot correctly invoke it. The error you see is Cognito complaining it did not receive the correct response, which should be sent by "callback(null, event)" in the Non-async handler.

When you moved the whole logic directly into the index.js and it worked because you change the entry point handler to Async handler.

Albert-Gao commented 4 years ago

But how amplify decide if I need an async Cognito trigger or not?

In my case, I certainly need to do some async stuff to get things done.

Were you saying I should strictly follow the generated code to avoid any async stuff in Amplify Cognito trigger? 😄

The problem here is that when the generated code can not determine the future, it’d better if we just remove it. Otherwise it is just a burden. Because now your logic has a sudden dependency that meant to be work and 100% irrelevant to your business logic.

And no, to update the generated code to fit user land logic is still unacceptable.

When I use Serverless framework, there is no generated code at all and never a problem.

When you write code for an future scenario and fail to cover all cases, that is a BUG.

You guys are doing great job and this framework is awesome, maintaining a massive framework is hard. I just wish it could become better and better, thanks 🙂

UnleashedMind commented 4 years ago

Thanks for the feedbacks. The Amplify CLI provides a mechanism for you to insert your own logic into the handler, and in this case, it uses the Non-Async style handler. Both Async and Non-Async handlers works, as you've found out, it just needs to be consistent.

ctjlewis commented 3 years ago

The async handler pattern ended up being used in a template generated by Amplify CLI itself, so basically, if you deploy an Amplify backend with the "add user to groups" functionality configured, the PostConfirmation script will fail during user sign-up flow. Was quite the nightmare to work backwards from, seems to have been an active issue since 2019 or so.

SalahAdDin commented 3 years ago

Any fix with this? I'm getting the same problem with the Email verification with redirection link feature.

ctjlewis commented 3 years ago

@SalahAdDin:

I'm working on a fix in #7180. To get this working for now, you will need to manually overwrite the template Lambda generated by the Amplify CLI. Go to amplify/backend/function/CustomMessage/src/index.js, and make sure it returns the event variable:

// amplify/backend/function/CustomMessage/src/index.js
const moduleNames = process.env.MODULES.split(',');
const modules = moduleNames.map(name => require(`./${name}`));

exports.handler = (event, context, callback) => {
  for (let i = 0; i < modules.length; i += 1) {
    const { handler } = modules[i];
    handler(event, context, callback);
  }

  // ADD THIS LINE:
  return event;
};

Then push your changes with amplify push. Does that work around this issue for now?

cc @medelman17

SalahAdDin commented 3 years ago

@SalahAdDin:

I'm working on a fix in #7180. To get this working for now, you will need to manually overwrite the template Lambda generated by the Amplify CLI. Go to amplify/backend/function/CustomMessage/src/index.js, and make sure it returns the event variable:

// amplify/backend/function/CustomMessage/src/index.js
const moduleNames = process.env.MODULES.split(',');
const modules = moduleNames.map(name => require(`./${name}`));

exports.handler = (event, context, callback) => {
  for (let i = 0; i < modules.length; i += 1) {
    const { handler } = modules[i];
    handler(event, context, callback);
  }

  // ADD THIS LINE:
  return event;
};

Then push your changes with amplify push. Does that work around this issue for now?

cc @medelman17

My template code was different, but i added return event and it didn't work.

ctjlewis commented 3 years ago

Can you post the code you modified so I can see where you're at (since you said the template looked different)? Do not modify verification-link.js, as that would not help here. verification-link.js is loaded by a handler in index.js that loads it and then calls it - that is what is called as a trigger, and needs to return an event or else the CloudFormation stack will complain about invalid JSON.

Just know this is definitely a bug and not something you did.

SalahAdDin commented 3 years ago

The original:

exports.handler = (event, context, callback) => {
  // Define the URL that you want the user to be directed to after verification is complete
  if (event.triggerSource === 'CustomMessage_SignUp') {
    const { codeParameter } = event.request;
    const { region, userName } = event;
    const { clientId } = event.callerContext;
    const redirectUrl = `${process.env.REDIRECTURL}/?username=${userName}`;
    const resourcePrefix = process.env.RESOURCENAME.split('CustomMessage')[0];

    const hyphenRegions = [
      'us-east-1',
      'us-west-1',
      'us-west-2',
      'ap-southeast-1',
      'ap-southeast-2',
      'ap-northeast-1',
      'eu-west-1',
      'sa-east-1',
    ];

    const seperator = hyphenRegions.includes(region) ? '-' : '.';

    const payload = Buffer.from(
      JSON.stringify({
        userName,
        redirectUrl,
        region,
        clientId,
      })
    ).toString('base64');
    const bucketUrl = `http://${resourcePrefix}verificationbucket-${process.env.ENV}.s3-website${seperator}${region}.amazonaws.com`;
    const url = `${bucketUrl}/?data=${payload}&code=${codeParameter}`;
    const message = `${process.env.EMAILMESSAGE}. \n ${url}`;
    event.response.smsMessage = message;
    event.response.emailSubject = process.env.EMAILSUBJECT;
    event.response.emailMessage = message;
    console.log('event.response', event.response);
    callback(null, event);
  } else {
    callback(null, event);
  }
};

And i just modify both callback(null,event);.

@ctjlewis

ctjlewis commented 3 years ago

@SalahAdDin Ah yeah, that is the wrong file, but you're very close.

This handler is loaded by the handler we need to modify. Look for a file called index.js in that same directory, right next to it. It will look like the snippet I posted above. Add the return event statement and let me know if it works. (You don't need to bother reverting verification-link.js. Its return value isn't used so those statements are basically no-ops.)

If you still can't find that file, change those lines in verification-link.js from callback(null, event) to context.done(null, event) as a last-ditch workaround.

SalahAdDin commented 3 years ago
const moduleNames = process.env.MODULES.split(',');
const modules = moduleNames.map(name => require(`./${name}`));

exports.handler = (event, context, callback) => {
  for (let i = 0; i < modules.length; i += 1) {
    const { handler } = modules[i];
    handler(event, context, callback);
  }

  return event;
};

This one?

ctjlewis commented 3 years ago

Yes, perfect. amplify push and let me know how it goes.

SalahAdDin commented 3 years ago

I would like but i find the same problem with another places: https://github.com/aws-amplify/amplify-cli/issues/7196

Actually, the CLI is not so much helpful with those errors.

ctjlewis commented 3 years ago

Oh gosh, so the CLI won't even let you run amplify push?

Can you try overwriting the index.js in the various amplify/functions dirs, just as you did for the Verification Link?

There should be several of these directories, but making sure they all return the events they're supposed to should get this working in the meantime while we work on a fix.

SalahAdDin commented 3 years ago

I think the another one is this:

/* eslint-disable */
/*
 * Copyright 2019-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance with
 * the License. A copy of the License is located at
 *
 *     http://aws.amazon.com/apache2.0/
 *
 * or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
 * CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
 * and limitations under the License.
 */
const awsServerlessExpress = require('aws-serverless-express');
const app = require('./app');

const server = awsServerlessExpress.createServer(app);

exports.handler = (event, context) => {
  console.log(`EVENT: ${JSON.stringify(event)}`);
  awsServerlessExpress.proxy(server, event, context);
};
ctjlewis commented 3 years ago

Very weird. You could add a return event statement to that one also to be safe.

I did not run into that handler (nor the bug bubbling up into amplify push itself) in my own configuration and will try to reproduce later today.

Once those handlers have been patched, let me know if you can amplify push and not see any "invalid JSON" errors (either during push, or during auth).

SalahAdDin commented 3 years ago
CREATE_FAILED               MFALambdaInputs                                                        Custom::LambdaCallout      Wed Apr 28 2021 07:42:30 GMT+0300 (GMT+03:00) Response is not valid JSON                                     
UPDATE_ROLLBACK_IN_PROGRESS amplify-thesiscancer-dev-104158-auththesiscancer7a6cf6e7-14VWV365GEC8A AWS::CloudFormation::Stack Wed Apr 28 2021 07:42:31 GMT+0300 (GMT+03:00) The following resource(s) failed to create: [MFALambdaInputs]. 
⠙ Updating resources in the cloud. This may take a few minutes...

UPDATE_FAILED               auththesiscancer7a6cf6e7        AWS::CloudFormation::Stack Wed Apr 28 2021 07:42:39 GMT+0300 (GMT+03:00) Embedded stack arn:aws:cloudformation:us-east-2:295982313172:stack/amplify-thesiscancer-dev-104158-auththesiscancer7a6cf6e7-14VWV365GEC8A/fa1fce70-8174-11eb-8a21-063ea9f5776c was not successfully updated. Currently in UPDATE_ROLLBACK_IN_PROGRESS with reason: The following resource(s) failed to create: [MFALambdaInputs]. 
UPDATE_ROLLBACK_IN_PROGRESS amplify-thesiscancer-dev-104158 AWS::CloudFormation::Stack Wed Apr 28 2021 07:42:40 GMT+0300 (GMT+03:00) The following resource(s) failed to update: [auththesiscancer7a6cf6e7].    

Nothing yet.

ctjlewis commented 3 years ago

Ah, very sorry José - I will try to dig into this, and get some other eyes on it. I have been working on (and successfully patched) the same error in a different context so we're close, but not there yet. Rest assured this is not something wrong with your config, I had this same problem.

SalahAdDin commented 3 years ago

@ctjlewis So, it is related to the generated code, right?

ctjlewis commented 3 years ago

As I understand it, there are triggers generated by the CLI that do not return events when they are expected to, so these CloudFormation stacks (which run in AWS to power the different parts of the Amplify architecture) fail when they depend on the results of a previous trigger getting passed to the next, which shows up as "Invalid JSON" (because it got undefined). I had this with the Add User to Groups functionality in #7179.

But I am not an expert and am actually pretty new to this CLI and AWS itself. I will ping you ITT if I make any progress on your issue.

Dahriel commented 3 years ago

/ this file will loop through all js modules which are uploaded to the lambda resource, provided that the file names (without extension) are included in the "MODULES" env variable. "MODULES" is a comma-delimmited string. / const moduleNames = process.env.MODULES.split(','); const modules = moduleNames.map(name => require(./${name}));

exports.handler = async (event, context, callback) => { for (let i = 0; i < modules.length; i += 1) { const { handler } = modules[i];

// tmp fix seem to work await handler(event, context, callback); context.done(null, event); } };

ctjlewis commented 3 years ago

Good work around @Dahriel, return event inside the async top-level handler should also work.

Fix is ready with #7219, just waiting on review. cc @edwardfoyle @jhockett

Albert-Gao commented 3 years ago

Sorry for the late chime in, but for a lambda trigger case, why not just remove that generated code as the current fix... I mean, user land fix rather than from the Amplify codebase. :D

ctjlewis commented 3 years ago

The generated code is emitted based on templates in the CLI source code. Deleting the files in the amplify/ directory would remove the corresponding functionality completely.

We have a fix ready and are just working on the final touches. cc @medelman17

BruceWheaton commented 3 years ago

Just hit this problem, at the tail-end of working out why my post confirmation (add to group) trigger is not being added to the pool, and does not have correct policies. Any connection to this problem or co-incidence?

ctjlewis commented 3 years ago

@BruceWheaton It's totally possible it's related to this issue. The team should be merging #7219 shortly, which should resolve this for everyone, including @SalahAdDin.

cc @renebrandel

github-actions[bot] commented 2 years ago

This issue 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.